[PATCH] regression of comment extraction in private key file without passphrase

[Loïc]

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.

I'm open to any suggestion on the correct fix to do.

And other idea would be to add the comment in the public key part of the
private key file (new format). I think it is a good idea, the comment
isn't private any way since it is present in the public key file.

I'm ok to implement this idea if you think it is worth it. Or any other
suggestion.


Note that the upper commit is very useful because it does extract the
fingerprint from a private file with passphrase while previously
ssh-keygen failed with the unsatisfying error "test_dsa is not a key
file". Thanks for it !

Regards

Loïc

---
 authfile.c   | 5 +++++
 ssh-keygen.c | 6 +-----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/authfile.c b/authfile.c
index 50fa48e4a3b6..9e6e2a00a896 100644
--- a/authfile.c
+++ b/authfile.c
@@ -304,6 +304,11 @@ sshkey_load_public(const char *filename, struct
sshkey **keyp, char **commentp)
     if ((r = sshkey_try_load_public(keyp, pubfile, commentp)) == 0)
         goto out;
 
+    /* If the comment is wanted, try loading the private key with no
passphrase,

+        since it contains the comment while the public key in the
private file doesn't */
+    if (commentp != NULL && (r = sshkey_load_private(filename, NULL,
keyp, commentp)) == 0)
+        goto out;
+
     /* finally, try to extract public key from private key file */
     if ((r = sshkey_load_pubkey_from_private(filename, keyp)) == 0)
         goto out;
diff --git a/ssh-keygen.c b/ssh-keygen.c
index 802fd25c286f..cb7872fb9165 100644
--- a/ssh-keygen.c
+++ b/ssh-keygen.c
@@ -917,11 +917,7 @@ fingerprint_private(const char *path)
         fatal("%s: %s", path, strerror(errno));
     if ((r = sshkey_load_public(path, &public, &comment)) != 0) {
         debug("load public \"%s\": %s", path, ssh_err(r));
-        if ((r = sshkey_load_private(path, NULL,
-            &public, &comment)) != 0) {
-            debug("load private \"%s\": %s", path, ssh_err(r));
-            fatal("%s is not a key file.", path);
-        }
+        fatal("%s is not a key file.", path);
     }
 
     fingerprint_one_key(public, comment);
-- 
2.17.1