From fea08bb24499dc76e3fb58895ca0f015714a1d53 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 2 Aug 2022 17:54:47 +0100 Subject: [PATCH] Windows Pageant: use nicer key-type strings. If you load a certified key into Windows Pageant, the official SSH id for the key type is so long that it overflows its space in the list box and overlaps the key fingerprint hash. This commit introduces yet another footling little ssh_keyalg method which returns a shorter human-readable description of the key type, and uses that in the Windows Pageant list box only. (Not in the Unix Pageant list, though, because being output to stdout, that seems like something people are more likely to want to machine-read, which firstly means we shouldn't change it lightly, and secondly, if we did change it we'd want to avoid having a variable number of spaces in the replacement key type text.) --- crypto/dsa.c | 3 +++ crypto/ecc-ssh.c | 25 ++++++++++++++++++++----- crypto/openssh-certs.c | 10 ++++++++++ crypto/rsa.c | 3 +++ ssh.h | 3 +++ windows/pageant.c | 20 ++++++++++++++++---- 6 files changed, 55 insertions(+), 9 deletions(-) diff --git a/crypto/dsa.c b/crypto/dsa.c index e304a38f..c1514ac0 100644 --- a/crypto/dsa.c +++ b/crypto/dsa.c @@ -490,6 +490,8 @@ static void dsa_sign(ssh_key *key, ptrlen data, unsigned flags, BinarySink *bs) mp_free(s); } +static char *dsa_alg_desc(const ssh_keyalg *self) { return dupstr("DSA"); } + const ssh_keyalg ssh_dsa = { .new_pub = dsa_new_pub, .new_priv = dsa_new_priv, @@ -508,6 +510,7 @@ const ssh_keyalg ssh_dsa = { .pubkey_bits = dsa_pubkey_bits, .supported_flags = nullkey_supported_flags, .alternate_ssh_id = nullkey_alternate_ssh_id, + .alg_desc = dsa_alg_desc, .ssh_id = "ssh-dss", .cache_id = "dss", }; diff --git a/crypto/ecc-ssh.c b/crypto/ecc-ssh.c index 17fde8e9..feab9d01 100644 --- a/crypto/ecc-ssh.c +++ b/crypto/ecc-ssh.c @@ -325,6 +325,9 @@ struct ecsign_extra { const unsigned char *oid; int oidlen; + /* Human-readable algorithm description */ + const char *alg_desc; + /* Some EdDSA instances prefix a string to all hash preimages, to * disambiguate which signature variant they're being used with */ ptrlen hash_prefix; @@ -1251,9 +1254,16 @@ static void eddsa_sign(ssh_key *key, ptrlen data, mp_free(s); } +static char *ec_alg_desc(const ssh_keyalg *self) +{ + const struct ecsign_extra *extra = + (const struct ecsign_extra *)self->extra; + return dupstr(extra->alg_desc); +} + static const struct ecsign_extra sign_extra_ed25519 = { ec_ed25519, &ssh_sha512, - NULL, 0, PTRLEN_DECL_LITERAL(""), + NULL, 0, "Ed25519", PTRLEN_DECL_LITERAL(""), }; const ssh_keyalg ssh_ecdsa_ed25519 = { .new_pub = eddsa_new_pub, @@ -1273,6 +1283,7 @@ const ssh_keyalg ssh_ecdsa_ed25519 = { .pubkey_bits = ec_shared_pubkey_bits, .supported_flags = nullkey_supported_flags, .alternate_ssh_id = nullkey_alternate_ssh_id, + .alg_desc = ec_alg_desc, .ssh_id = "ssh-ed25519", .cache_id = "ssh-ed25519", .extra = &sign_extra_ed25519, @@ -1280,7 +1291,7 @@ const ssh_keyalg ssh_ecdsa_ed25519 = { static const struct ecsign_extra sign_extra_ed448 = { ec_ed448, &ssh_shake256_114bytes, - NULL, 0, PTRLEN_DECL_LITERAL("SigEd448\0\0"), + NULL, 0, "Ed448", PTRLEN_DECL_LITERAL("SigEd448\0\0"), }; const ssh_keyalg ssh_ecdsa_ed448 = { .new_pub = eddsa_new_pub, @@ -1300,6 +1311,7 @@ const ssh_keyalg ssh_ecdsa_ed448 = { .pubkey_bits = ec_shared_pubkey_bits, .supported_flags = nullkey_supported_flags, .alternate_ssh_id = nullkey_alternate_ssh_id, + .alg_desc = ec_alg_desc, .ssh_id = "ssh-ed448", .cache_id = "ssh-ed448", .extra = &sign_extra_ed448, @@ -1311,7 +1323,7 @@ static const unsigned char nistp256_oid[] = { }; static const struct ecsign_extra sign_extra_nistp256 = { ec_p256, &ssh_sha256, - nistp256_oid, lenof(nistp256_oid), + nistp256_oid, lenof(nistp256_oid), "NIST p256", }; const ssh_keyalg ssh_ecdsa_nistp256 = { .new_pub = ecdsa_new_pub, @@ -1331,6 +1343,7 @@ const ssh_keyalg ssh_ecdsa_nistp256 = { .pubkey_bits = ec_shared_pubkey_bits, .supported_flags = nullkey_supported_flags, .alternate_ssh_id = nullkey_alternate_ssh_id, + .alg_desc = ec_alg_desc, .ssh_id = "ecdsa-sha2-nistp256", .cache_id = "ecdsa-sha2-nistp256", .extra = &sign_extra_nistp256, @@ -1342,7 +1355,7 @@ static const unsigned char nistp384_oid[] = { }; static const struct ecsign_extra sign_extra_nistp384 = { ec_p384, &ssh_sha384, - nistp384_oid, lenof(nistp384_oid), + nistp384_oid, lenof(nistp384_oid), "NIST p384", }; const ssh_keyalg ssh_ecdsa_nistp384 = { .new_pub = ecdsa_new_pub, @@ -1362,6 +1375,7 @@ const ssh_keyalg ssh_ecdsa_nistp384 = { .pubkey_bits = ec_shared_pubkey_bits, .supported_flags = nullkey_supported_flags, .alternate_ssh_id = nullkey_alternate_ssh_id, + .alg_desc = ec_alg_desc, .ssh_id = "ecdsa-sha2-nistp384", .cache_id = "ecdsa-sha2-nistp384", .extra = &sign_extra_nistp384, @@ -1373,7 +1387,7 @@ static const unsigned char nistp521_oid[] = { }; static const struct ecsign_extra sign_extra_nistp521 = { ec_p521, &ssh_sha512, - nistp521_oid, lenof(nistp521_oid), + nistp521_oid, lenof(nistp521_oid), "NIST p521", }; const ssh_keyalg ssh_ecdsa_nistp521 = { .new_pub = ecdsa_new_pub, @@ -1393,6 +1407,7 @@ const ssh_keyalg ssh_ecdsa_nistp521 = { .pubkey_bits = ec_shared_pubkey_bits, .supported_flags = nullkey_supported_flags, .alternate_ssh_id = nullkey_alternate_ssh_id, + .alg_desc = ec_alg_desc, .ssh_id = "ecdsa-sha2-nistp521", .cache_id = "ecdsa-sha2-nistp521", .extra = &sign_extra_nistp521, diff --git a/crypto/openssh-certs.c b/crypto/openssh-certs.c index 5e86f928..351b17d6 100644 --- a/crypto/openssh-certs.c +++ b/crypto/openssh-certs.c @@ -216,6 +216,7 @@ static int opensshcert_pubkey_bits(const ssh_keyalg *self, ptrlen blob); static unsigned opensshcert_supported_flags(const ssh_keyalg *self); static const char *opensshcert_alternate_ssh_id(const ssh_keyalg *self, unsigned flags); +static char *opensshcert_alg_desc(const ssh_keyalg *self); static const ssh_keyalg *opensshcert_related_alg(const ssh_keyalg *self, const ssh_keyalg *base); @@ -269,6 +270,7 @@ static const ssh_keyalg *opensshcert_related_alg(const ssh_keyalg *self, .pubkey_bits = opensshcert_pubkey_bits, \ .supported_flags = opensshcert_supported_flags, \ .alternate_ssh_id = opensshcert_alternate_ssh_id, \ + .alg_desc = opensshcert_alg_desc, \ .related_alg = opensshcert_related_alg, \ .ssh_id = ssh_alg_id_prefix "-cert-v01@openssh.com", \ .cache_id = "opensshcert-" ssh_key_id_prefix, \ @@ -898,6 +900,14 @@ static const char *opensshcert_alternate_ssh_id(const ssh_keyalg *self, return self->ssh_id; } +static char *opensshcert_alg_desc(const ssh_keyalg *self) +{ + char *base_desc = ssh_keyalg_desc(self->base_alg); + char *our_desc = dupcat(base_desc, " cert"); + sfree(base_desc); + return our_desc; +} + static const ssh_keyalg *opensshcert_related_alg(const ssh_keyalg *self, const ssh_keyalg *base) { diff --git a/crypto/rsa.c b/crypto/rsa.c index 635c9efb..88748288 100644 --- a/crypto/rsa.c +++ b/crypto/rsa.c @@ -859,6 +859,8 @@ const char *ssh_rsa_alternate_ssh_id(const ssh_keyalg *self, unsigned flags) return self->ssh_id; } +static char *rsa2_alg_desc(const ssh_keyalg *self) { return dupstr("RSA"); } + static const struct ssh2_rsa_extra rsa_extra = { 0 }, rsa_sha256_extra = { SSH_AGENT_RSA_SHA2_256 }, @@ -880,6 +882,7 @@ static const struct ssh2_rsa_extra .components = rsa2_components, \ .base_key = nullkey_base_key, \ .pubkey_bits = rsa2_pubkey_bits, \ + .alg_desc = rsa2_alg_desc, \ .cache_id = "rsa2" const ssh_keyalg ssh_rsa = { diff --git a/ssh.h b/ssh.h index bf4f04d7..fb9d7453 100644 --- a/ssh.h +++ b/ssh.h @@ -857,6 +857,7 @@ struct ssh_keyalg { int (*pubkey_bits) (const ssh_keyalg *self, ptrlen blob); unsigned (*supported_flags) (const ssh_keyalg *self); const char *(*alternate_ssh_id) (const ssh_keyalg *self, unsigned flags); + char *(*alg_desc)(const ssh_keyalg *self); /* The following methods can be NULL if !is_certificate */ const ssh_keyalg *(*related_alg)(const ssh_keyalg *self, const ssh_keyalg *base); @@ -925,6 +926,8 @@ static inline const unsigned ssh_keyalg_supported_flags(const ssh_keyalg *self) static inline const char *ssh_keyalg_alternate_ssh_id( const ssh_keyalg *self, unsigned flags) { return self->alternate_ssh_id(self, flags); } +static inline char *ssh_keyalg_desc(const ssh_keyalg *self) +{ return self->alg_desc(self); } static inline const ssh_keyalg *ssh_keyalg_related_alg( const ssh_keyalg *self, const ssh_keyalg *base) { return self->related_alg(self, base); } diff --git a/windows/pageant.c b/windows/pageant.c index 47992c28..aea98389 100644 --- a/windows/pageant.c +++ b/windows/pageant.c @@ -346,7 +346,7 @@ static void keylist_update_callback( * Expect the fingerprint to contain two words: bit count and * hash. */ - put_dataz(disp->alg, "ssh1"); + put_dataz(disp->alg, "SSH-1"); put_datapl(disp->bits, ptrlen_get_word(&fingerprint, " ")); put_datapl(disp->hash, ptrlen_get_word(&fingerprint, " ")); break; @@ -357,7 +357,21 @@ static void keylist_update_callback( * Expect the fingerprint to contain three words: algorithm * name, bit count, hash. */ - put_datapl(disp->alg, ptrlen_get_word(&fingerprint, " ")); + const ssh_keyalg *alg = pubkey_blob_to_alg( + ptrlen_from_strbuf(key->blob)); + + ptrlen keytype_word = ptrlen_get_word(&fingerprint, " "); + if (alg) { + /* Use our own human-legible algorithm names if available, + * because they fit better in the space. (Certificate key + * algorithm names in particular are terribly long.) */ + char *alg_desc = ssh_keyalg_desc(alg); + put_dataz(disp->alg, alg_desc); + sfree(alg_desc); + } else { + put_datapl(disp->alg, keytype_word); + } + put_datapl(disp->bits, ptrlen_get_word(&fingerprint, " ")); put_datapl(disp->hash, ptrlen_get_word(&fingerprint, " ")); @@ -368,8 +382,6 @@ static void keylist_update_callback( * overlap into the bits column without colliding with * pointless text. */ - const ssh_keyalg *alg = pubkey_blob_to_alg( - ptrlen_from_strbuf(key->blob)); if (!(alg == &ssh_dsa || alg == &ssh_rsa)) strbuf_clear(disp->bits); }