Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add checkkeylength command #27

Merged
merged 4 commits into from
Jun 1, 2021
Merged

Conversation

emadurandal
Copy link
Contributor

RSA公開鍵の鍵長を確認するためのコマンドを追加してみました。

ご検討よろしくおねがいします。

@yoshi389111
Copy link
Collaborator

採用するしないは、KEINOS さんが判断だろうとは思いますが、気になるところを数点。

手元(Ubuntu)では動きは問題ないですが、 #25 をみると mac では動かないようなので、-e md5sum-e md5 などに which をつけた方が良いと思います。

また、bin/checkkeylength に実行権限をつけた方が良いと思います。

.github/run-lint.sh の LIST_SCRIPT_NO_EXT にも追加したほうが良いと思います。
(現状、このスクリプトで PR 時に lint を掛けるようにしているため)

いかがでしょうか?

@emadurandal
Copy link
Contributor Author

@yoshi389111 ご指摘ありがとうございます。 ご指摘の3点対応しました。

  • whichをつける対応
  • bin/checkkeylength に実行権限をつける(Gitの内容として反映されるのかよく知りませんが)
  • .github/run-lint.sh の LIST_SCRIPT_NO_EXT に追加

@emadurandal emadurandal mentioned this pull request Jun 1, 2021
@KEINOS
Copy link
Member

KEINOS commented Jun 1, 2021

鍵の長さチェックは強度確認含め欲しいなぁと思っていました。

後述する気になる点が 1 つありますが、LGTM なので入れちゃいましょう!

#23 でディスカッションしているセキュリティ上の判断基準ですが、考えてみれば、その 1 つ(短すぎる鍵は warn するなど)にも使えそうですね。

気になる点というのが、すでに check コマンドがあるのでコマンド名が気になります。

例えば check keylength KEINOS とサブコマンド風にするか check --keylength KEINOS ~/.ssh/id_rsa みたいにオプションが使えれば直感的な気がします。

むしろ、check 事項の 1 つとして標準で組み込んじゃってもいいかもしれません。

ただ、それ自体は check スクリプト内で checkkeylength を呼び出せればいいだけなので、それは後に考えましょう。今は Let's go to 久美子んじゃいましょう!

# bin/check

...

echo "$@" | grep keylength 2>/dev/null 1>/dev/null && {
    "${PATH_DIR_BIN}/checkkeylength" "$2" || {
        echo >&2 "公開鍵の鍵長の取得に失敗しました"
        exit 1
    }
    exit 0
}

もしかすると、今後このコマンドはデフォルトで他のコマンドのチェックに使われることになるかもしれません。その場合は関数化されてコマンドとしては消えるかもしれません。それだけはご承知おきください。(消えるとしても v2.0 だと思いますが) m( _ _ )m

SHIPIT

@yoshi389111
Copy link
Collaborator

実行権限が付いていないように思いますが、それ以外は問題なさそうです(Ubuntuで確認)

実行権限はあとでもよさそうなきがするので

LGTM

@KEINOS
Copy link
Member

KEINOS commented Jun 1, 2021

確かに実行権限付いてないですね。まぁ、マージ後につけましょう。

@KEINOS KEINOS merged commit 1da7e3b into Qithub-BOT:master Jun 1, 2021
KEINOS added a commit that referenced this pull request Jun 1, 2021
@emadurandal
Copy link
Contributor Author

おお、マージありがとうございます。

もしかすると、今後このコマンドはデフォルトで他のコマンドのチェックに使われることになるかもしれません。その場合は関数化されてコマンドとしては消えるかもしれません。それだけはご承知おきください。(消えるとしても v2.0 だと思いますが) m( _ _ )m

あ、そこらへんはもう全然お好きにやっちゃってくださいー。

@emadurandal emadurandal deleted the feat/check-keylength branch June 7, 2021 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants