Re: [PATCH] ssh signing: support non ssh-* keytypes

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 17, 2021, 17:51 UTC
Message-ID: <YZVBODo2BgrXd8XK@nand.local>
In-Reply-To: <20211117162727.650857-1-fs@gigacodes.de>

On Wed, Nov 17, 2021 at 05:27:27PM +0100, Fabian Stelzer wrote:

Show 24 quoted lines

> 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 <fs@gigacodes.de>
> ---
>  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."

Show 8 quoted lines

> +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.

Show 10 quoted lines

> +
>  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