diff mbox series

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

Message ID 7d1d131ff5b43559c8a750ebdfd6faaba93c1ad1.1627501009.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series ssh signing: Add commit & tag signing/verification via SSH keys using ssh-keygen | expand

Commit Message

Fabian Stelzer July 28, 2021, 7:36 p.m. UTC 
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.

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
 gpg-interface.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 gpg-interface.h |  6 ++++++
 send-pack.c     |  8 ++++----
 3 files changed, 56 insertions(+), 4 deletions(-)

Comments

Junio C Hamano July 28, 2021, 9:34 p.m. UTC | #1 
"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.
Fabian Stelzer July 29, 2021, 8:21 a.m. UTC | #2 
On 28.07.21 23:34, Junio C Hamano wrote:
> "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.
> 

Fixed

>>   
>> +/* Returns a textual but unique representation ot the signing key */
> 
> "ot" -> "of".
> 

Fixed

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

i will put the two strcmp(ssh) ifs on my todo list to also replace with 
a callback function.

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

Previously the push certs contained the configured user.signingkey as 
"pusher". For gpg this is usually the key id. (e.g.: ABCDEF01)
For ssh signing this can now be a file path which would not make much 
sense to put into the push cert. I did not use the public ssh key since 
the file can also contain an encrypted private key, so i would have to 
ask ssh-keygen for the public key anyway.
Since the ssh fingerprint more resembles the gpg key id i used it instead.

As far as i understand the actual contents of the "pusher" header is not 
really relevant for the push-cert. The unique nonce is important but 
besides that its just a signed text blob.
If we don't care about having a local users file path in this header we 
could drop this commit.
diff mbox series

Patch

diff --git a/gpg-interface.c b/gpg-interface.c
index 3afacb48900..ec48a37b6cc 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -470,6 +470,41 @@  int git_gpg_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
+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 */
+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();
+	}
+}
+
 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);
 	datestamp(&cert);
 	strbuf_addch(&cert, '\n');
 	if (args->url && *args->url) {
@@ -374,7 +374,7 @@  static int generate_push_cert(struct strbuf *req_buf,
 	if (!update_seen)
 		goto free_return;
 
-	if (sign_buffer(&cert, &cert, signing_key))
+	if (sign_buffer(&cert, &cert, get_signing_key()))
 		die(_("failed to sign the push certificate"));
 
 	packet_buf_write(req_buf, "push-cert%c%s", 0, cap_string);
@@ -386,7 +386,7 @@  static int generate_push_cert(struct strbuf *req_buf,
 	packet_buf_write(req_buf, "push-cert-end\n");
 
 free_return:
-	free(signing_key);
+	free(signing_key_id);
 	strbuf_release(&cert);
 	return update_seen;
 }