On 17/04/2020, Loïc wrote: > On 17/04/2020, Damien Miller wrote : >> On Wed, 15 Apr 2020, Loïc wrote: >> >>> Hello, >>> >>> In one recent change >>> (https://anongit.mindrot.org/openssh.git/commit/?id=2b13d3934d5803703c04803ca3a93078ecb5b715), >>> I noticed a regression. >>> >>> If ssh-keygen is given a private file without passphrase and without the >>> corresponding .pub file, I doesn't extract the comment after the commit, >>> while it did before: >>> >>> Before the commit: >>> >>> $ ./ssh-keygen -q -t dsa -N '' -C foobar -f test_dsa >>> $ ./ssh-keygen -l -f test_dsa >>> 1024 SHA256:/E/JUVD3FO4vHYs+8RfXJW+ah4H4bHcBrCRKFcfZSJk foobar (DSA) >>> $ rm test_dsa.pub >>> $ ./ssh-keygen -l -f test_dsa >>> 1024 SHA256:/E/JUVD3FO4vHYs+8RfXJW+ah4H4bHcBrCRKFcfZSJk foobar (DSA) >>> >>> Last command after the commit: >>> >>> $ ./ssh-keygen -l -f test_dsa >>> 1024 SHA256:/E/JUVD3FO4vHYs+8RfXJW+ah4H4bHcBrCRKFcfZSJk no comment (DSA) >>> >>> It is due to the fact that the 'sshkey_load_public' function is now >>> finishing by sshkey_load_public_from_private, which is not failing on a >>> (new format) private file. Previously, if did fail and so the >>> fingerprint_private function was calling sshkey_load_private without >>> passphrase as a fallback. >>> >>> >>> I suggest to move the fallback inside the sshkey_load_public, so to call >>> the sshkey_load_private without passphrase in the sshkey_load_public >>> before extracting the public key from the private file. >>> >>> Here is the suggested patch below. >> IMO it's easier to flip the order of operations in >> ssh-keygen.c:fingerprint_private(): > Yes, your patch is simpler. Unfortunately, it also has a regression when > the private key file is in the old format which doesn't contain the > comment and when the .pub is not removed which is a more common case > probably: > > On latest git: > > $ ./ssh-keygen -q -m PEM -t dsa -N '' -C foobar -f test_dsa > $ ./ssh-keygen -l -f test_dsa > 1024 SHA256:Yqp+0QYlbsfJotozWtbWVHv+WAAu2PEFwo2ZTeRPzv8 no comment (DSA) > > With openssh version 8.2: > > $ ssh-keygen -l -f test_dsa > 1024 SHA256:Yqp+0QYlbsfJotozWtbWVHv+WAAu2PEFwo2ZTeRPzv8 foobar (DSA) Hi, In order to help catching those regressions, here is a new regression test file. It is successful on V_8_2 tag of the git but failing on master (https://anongit.mindrot.org/openssh.git/commit/?id=32f2d0aad42c15e19bd3b07496076ca891573a58), and differently on https://anongit.mindrot.org/openssh.git/commit/?id=094dd513f4b42e6a3cebefd18d1837eb709b4d99 Hope it helps. Subject: [PATCH] Add regression test for comment extraction from private key file --- regress/keygen-comment.sh | 55 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 regress/keygen-comment.sh diff --git a/regress/keygen-comment.sh b/regress/keygen-comment.sh new file mode 100644 index 000000000000..6bd3a69e2857 --- /dev/null +++ b/regress/keygen-comment.sh @@ -0,0 +1,55 @@ +# Placed in the Public Domain. + +tid="Comment extraction from private key" + +S1="secret1" + +check_fingerprint () { + file="$1" + comment="$2" + CMD="${SSHKEYGEN} -l -E sha256 -f $file" + $CMD > $OBJ/$t-fgp + if [ $? -ne 0 ]; then + fail "ssh-keygen -l failed for $t-key" + fi + grep -qE "^([0-9]+) SHA256:(.){43} ${comment} \(.*\)$" $OBJ/$t-fgp + if [ $? -ne 0 ]; then + trace "Expected comment is: \"${comment}\"" + trace "ssh-keygen -l output is \"$( cat $OBJ/$t-fgp )\"" + fail "comment is not correctly recovered for $t-key" + fi + rm -f $OBJ/$t-fgp +} + +for fmt in '' RFC4716 PKCS8 PEM; do + for t in $SSH_KEYTYPES; do + trace "generating $t key in '$fmt' format" + rm -f $OBJ/$t-key + comment="foo bar" + ${SSHKEYGEN} -q ${fmt:+-m} $fmt -N '' -C "${comment}" -t $t -f $OBJ/$t-key + if [ $? -eq 0 ]; then + if [ "$fmt" = "PKCS8" ] || [ "$fmt" = "PEM" ] && grep -q -- "-----BEGIN OPENSSH PRIVATE KEY-----" $OBJ/$t-key ; then + # some key types like ed25519 and *@openssh.com are never + # stored in old formats, it is not useful to test them twice + continue + fi + # fingerprint using public key file if available + trace "fingerprinting $t key" + check_fingerprint $OBJ/$t-key "${comment}" + # fingerprint using public key file forced + trace "fingerprinting $t key using public key file" + check_fingerprint $OBJ/$t-key.pub "${comment}" + # Output fingerprint using only private file + trace "fingerprinting $t key using private key file" + rm -f $OBJ/$t-key.pub + if [ "$fmt" = "PKCS8" ] || [ "$fmt" = "PEM" ]; then + comment="no comment" + trace "comment cannot be recovered in the $fmt format private key" + fi + check_fingerprint $OBJ/$t-key "${comment}" + else + fail "ssh-keygen for $t-key failed" + fi + rm -f $OBJ/$t-key $OBJ/$t-key.pub + done +done -- 2.17.1 _______________________________________________ openssh-unix-dev mailing list openssh-unix-dev@xxxxxxxxxxx https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
