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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 18, 2021, 03:09 UTC
Message-ID: <xmqq4k8a2m97.fsf@gitster.g>
In-Reply-To: <20211117162727.650857-1-fs@gigacodes.de>

Fabian Stelzer <fs@gigacodes.de> writes:

Show 9 quoted lines

> +/* Determines wether key contains a literal ssh key or a path to a file */
> +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-")
> +	);
> +}

A more forward looking thing you could do is to

 (1) grandfather the convention "any string that begins with 'ssh-'
     is taken as a ssh literal key".

 (2) refrain from spreading such an unstructured mess by picking a
     reserved prefix, say "ssh-key::" and have all other kinds of
     ssh keys use the convention.

making the above function look more like

    static int is_literal_ssh_key(const char *string, const char **key)
    {
	if (skip_prefix(string, "ssh-key::", key)
	    return 1;
	if (starts_with(string, "ssh-")) {
	    key = string;
	    return 1;
	}
	return 0;
    }

so that the caller can extract the literal key from the string that specifies either the literal key or path to the file. This will futureproof us in two axis. When SSH adds types of keys using new algo, we do not have to add it to is_literal_ssh_key() function. Also when another crypto suite other than GPG and SSH comes, we won't repeat the "bare 'ssh-' prefix is reserved by ssh, and different kind in the same suite may have to consume more reserved prefixes" mistake---it would make it more natural for us to pick "literal keys from any variant of that new FOO crypto suite are written with 'foo-key::' prefix" if we did so right now. It would have been better if we didn't have to do the grandfathering, but I am assuming that the ship has already sailed?

Show 9 quoted lines

> @@ -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)) {
> 		strvec_pushl(&ssh_keygen.args, "ssh-keygen", "-lf", "-", NULL);
> 		ret = pipe_command(&ssh_keygen, signing_key,
> 				   strlen(signing_key), &fingerprint_stdout, 0,

This part needs a bit of adjustment if we go the "is_literal_ssh_key() is not just a boolean but is used to strip the prefix to signal the kind of key" route, but the necessary adjustment should be trivial.