Re: [PATCH v6 4/9] ssh signing: provide a textual representation of the signing key

From: Junio C Hamano <gitster@pobox.com>
To: "Fabian Stelzer via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Han-Wen Nienhuys" <hanwen@google.com>,
	"Fabian Stelzer" <fs@gigacodes.de>,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	"Randall S. Becker" <rsbecker@nexbridge.com>,
	"Bagas Sanjaya" <bagasdotme@gmail.com>,
	"Hans Jerry Illikainen" <hji@dyntopia.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Felipe Contreras" <felipe.contreras@gmail.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"Gwyneth Morgan" <gwymor@tilde.club>
Subject: Re: [PATCH v6 4/9] ssh signing: provide a textual representation of the signing key
Date: Wed, 28 Jul 2021 14:34:56 -0700	[thread overview]
Message-ID: <xmqqeebi86m7.fsf@gitster.g> (raw)
In-Reply-To: <7d1d131ff5b43559c8a750ebdfd6faaba93c1ad1.1627501009.git.gitgitgadget@gmail.com> (Fabian Stelzer via GitGitGadget's message of "Wed, 28 Jul 2021 19:36:44 +0000")

"Fabian Stelzer via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Fabian Stelzer <fs@gigacodes.de>
>
> for ssh the user.signingkey can be a filename/path or even a literal ssh pubkey.
> in push certs and textual output we prefer the ssh fingerprint instead.

These sentences that lack the initial capital letters would look
unusual and distracting in our "git log --no-merges" stream.

> +static char *get_ssh_key_fingerprint(const char *signing_key)
> +{
> +	struct child_process ssh_keygen = CHILD_PROCESS_INIT;
> +	int ret = -1;
> +	struct strbuf fingerprint_stdout = STRBUF_INIT;
> +	struct strbuf **fingerprint;
> +
> +	/*
> +	 * With SSH Signing this can contain a filename or a public key
> +	 * For textual representation we usually want a fingerprint
> +	 */
> +	if (istarts_with(signing_key, "ssh-")) {
> +		strvec_pushl(&ssh_keygen.args, "ssh-keygen", "-lf", "-", NULL);
> +		ret = pipe_command(&ssh_keygen, signing_key,
> +				   strlen(signing_key), &fingerprint_stdout, 0,
> +				   NULL, 0);
> +	} else {
> +		strvec_pushl(&ssh_keygen.args, "ssh-keygen", "-lf",
> +			     configured_signing_key, NULL);
> +		ret = pipe_command(&ssh_keygen, NULL, 0, &fingerprint_stdout, 0,
> +				   NULL, 0);
> +	}
> +
> +	if (!!ret)
> +		die_errno(_("failed to get the ssh fingerprint for key '%s'"),
> +			  signing_key);
> +
> +	fingerprint = strbuf_split_max(&fingerprint_stdout, ' ', 3);
> +	if (!fingerprint[1])
> +		die_errno(_("failed to get the ssh fingerprint for key '%s'"),
> +			  signing_key);
> +
> +	return strbuf_detach(fingerprint[1], NULL);
> +}
> +
>  /* Returns the first public key from an ssh-agent to use for signing */
>  static char *get_default_ssh_signing_key(void)
>  {
> @@ -490,6 +525,17 @@ static char *get_default_ssh_signing_key(void)
>  	return "";
>  }
>  
> +/* Returns a textual but unique representation ot the signing key */

"ot" -> "of".

> +const char *get_signing_key_id(void)
> +{
> +	if (!strcmp(use_format->name, "ssh")) {
> +		return get_ssh_key_fingerprint(get_signing_key());
> +	} else {
> +		/* GPG/GPGSM only store a key id on this variable */
> +		return get_signing_key();

Hmph, we could ask gpg key fingerprint if we wanted to, and we
cannot tell why "ssh" side needs a separate "key" and "key_id"
while "gpg" side does not.  Hopefully it will become clear as we
read on?

Again, dispatching on use_format->name looked rather unexpected.

> +	}
> +}
> +
>  const char *get_signing_key(void)
>  {
>  	if (configured_signing_key)
> diff --git a/gpg-interface.h b/gpg-interface.h
> index feac4decf8b..beefacbb1e9 100644
> --- a/gpg-interface.h
> +++ b/gpg-interface.h
> @@ -64,6 +64,12 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
>  int git_gpg_config(const char *, const char *, void *);
>  void set_signing_key(const char *);
>  const char *get_signing_key(void);
> +
> +/*
> + * Returns a textual unique representation of the signing key in use
> + * Either a GPG KeyID or a SSH Key Fingerprint
> + */
> +const char *get_signing_key_id(void);
>  int check_signature(const char *payload, size_t plen,
>  		    const char *signature, size_t slen,
>  		    struct signature_check *sigc);
> diff --git a/send-pack.c b/send-pack.c
> index 5a79e0e7110..50cca7e439b 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -341,13 +341,13 @@ static int generate_push_cert(struct strbuf *req_buf,
>  {
>  	const struct ref *ref;
>  	struct string_list_item *item;
> -	char *signing_key = xstrdup(get_signing_key());
> +	char *signing_key_id = xstrdup(get_signing_key_id());
>  	const char *cp, *np;
>  	struct strbuf cert = STRBUF_INIT;
>  	int update_seen = 0;
>  
>  	strbuf_addstr(&cert, "certificate version 0.1\n");
> -	strbuf_addf(&cert, "pusher %s ", signing_key);
> +	strbuf_addf(&cert, "pusher %s ", signing_key_id);

Ahh...  We do not send GPG fingerprint in push certificate but you
want to use the fingerprint when signing with SSH keys, and that is
where the need for signing_key_id comes from?

OK.