Re: [PATCH] ssh signing: support non ssh-* keytypes
From: Taylor Blau <hidden>
Date: 2021-11-17 17:51:57
On Wed, Nov 17, 2021 at 05:27:27PM +0100, Fabian Stelzer wrote:
quoted hunk ↗ jump to hunk
The user.signingKey config for ssh signing supports either a path to a file containing the key or for the sake of convenience a literal string with the ssh public key. To differentiate between those two cases we check if the first few characters contain "ssh-" which is unlikely to be the start of a path. ssh supports other key types which are not prefixed with "ssh-" and will currently be treated as a file path and therefore fail to load. To remedy this we move the prefix check into its own function and add the other key types. "ssh -Q key" can be used to show a list of all supported types. Signed-off-by: Fabian Stelzer <redacted> --- gpg-interface.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)diff --git a/gpg-interface.c b/gpg-interface.c index 3e7255a2a9..dd1df9f4ee 100644 --- a/gpg-interface.c +++ b/gpg-interface.c@@ -707,6 +707,16 @@ int git_gpg_config(const char *var, const char *value, void *cb) return 0; } +/* Determines wether key contains a literal ssh key or a path to a file */
Nit: s/wether/whether. I had to re-read this comment before I realized that the "or a path to a file" isn't checked by this function, but is what we assume to be true if this function returns 0. So I don't think anything you wrote there is wrong, but it may be clearer to just say "Returns 1 if `key` contains a literal SSH key, 0 otherwise."
quoted hunk ↗ jump to hunk
+static int is_literal_ssh_key(const char *key) { + return ( + starts_with(key, "ssh-") || + starts_with(key, "ecdsa-") || + starts_with(key, "sk-ssh-") || + starts_with(key, "sk-ecdsa-") + ); +}
The outer-most parenthesis are unnecessary, but help with line wrapping.
Equally OK would have been:
return starts_with(...) ||
starts_with(...) ||
and so on, but it doesn't matter much one way or the other.
quoted hunk ↗ jump to hunk
+ static char *get_ssh_key_fingerprint(const char *signing_key) { struct child_process ssh_keygen = CHILD_PROCESS_INIT;@@ -719,7 +729,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key) * With SSH Signing this can contain a filename or a public key * For textual representation we usually want a fingerprint */ - if (starts_with(signing_key, "ssh-")) { + if (is_literal_ssh_key(signing_key)) {
This (and all other replacements) are straightforward and exhaustive. It would be nice to see an additional test confirming that we treat, for e.g., literal ECDSA keys correctly. Thanks, Taylor