[v6,1/9] ssh signing: preliminary refactoring and clean-up

Commit Message

Fabian Stelzer July 28, 2021, 7:36 p.m. UTC

From: Fabian Stelzer <[e-mail]>

Openssh v8.2p1 added some new options to ssh-keygen for signature
creation and verification. These allow us to use ssh keys for git
signatures easily.

In our corporate environment we use PIV x509 Certs on Yubikeys for email
signing/encryption and ssh keys which I think is quite common
(at least for the email part). This way we can establish the correct
trust for the SSH Keys without setting up a separate GPG Infrastructure
(which is still quite painful for users) or implementing x509 signing
support for git (which lacks good forwarding mechanisms).
Using ssh agent forwarding makes this feature easily usable in todays
development environments where code is often checked out in remote VMs / containers.
In such a setup the keyring & revocationKeyring can be centrally
generated from the x509 CA information and distributed to the users.

To be able to implement new signing formats this commit:
 - makes the sigc structure more generic by renaming "gpg_output" to
   "output"
 - introduces function pointers in the gpg_format structure to call
   format specific signing and verification functions
 - moves format detection from verify_signed_buffer into the check_signature
   api function and calls the format specific verify
 - renames and wraps sign_buffer to handle format specific signing logic
   as well

Signed-off-by: Fabian Stelzer <[e-mail]>
---
 fmt-merge-msg.c |   6 +--
 gpg-interface.c | 104 +++++++++++++++++++++++++++++-------------------
 gpg-interface.h |   2 +-
 log-tree.c      |   8 ++--
 pretty.c        |   4 +-
 5 files changed, 74 insertions(+), 50 deletions(-)

Comments

Jonathan Tan July 28, 2021, 10:32 p.m. UTC | #1

I think this patch set is beyond the "is this a good idea in general"
phase (in particular, I think that being able to sign Git commits by
using SSH infrastructure is very useful), so I'll proceed to critiquing
the commits in more detail.

Firstly, in commit messages, the left side of the colon is usually the
name of the subsystem - in this case, "gpg-interface".

> To be able to implement new signing formats this commit:
>  - makes the sigc structure more generic by renaming "gpg_output" to
>    "output"
>  - introduces function pointers in the gpg_format structure to call
>    format specific signing and verification functions
>  - moves format detection from verify_signed_buffer into the check_signature
>    api function and calls the format specific verify
>  - renames and wraps sign_buffer to handle format specific signing logic
>    as well

I think that this commit should be further split up - in particular, it
is hard for reviewers to verify that there is no difference in
functionality before and after this commit. I already spotted one
difference - perhaps there are more. For me, splitting the above 4
points into 4 commits would be an acceptable split.

> diff --git a/gpg-interface.c b/gpg-interface.c
> index 127aecfc2b0..31cf4ba3938 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -15,6 +15,12 @@ struct gpg_format {
>  	const char *program;
>  	const char **verify_args;
>  	const char **sigs;
> +	int (*verify_signed_buffer)(struct signature_check *sigc,
> +				    struct gpg_format *fmt, const char *payload,
> +				    size_t payload_size, const char *signature,
> +				    size_t signature_size);
> +	int (*sign_buffer)(struct strbuf *buffer, struct strbuf *signature,
> +			   const char *signing_key);
>  };

[snip]

>  static struct gpg_format gpg_format[] = {
> -	{ .name = "openpgp", .program = "gpg",
> -	  .verify_args = openpgp_verify_args,
> -	  .sigs = openpgp_sigs
> +	{
> +		.name = "openpgp",
> +		.program = "gpg",
> +		.verify_args = openpgp_verify_args,
> +		.sigs = openpgp_sigs,
> +		.verify_signed_buffer = verify_gpg_signed_buffer,
> +		.sign_buffer = sign_buffer_gpg,
>  	},
> -	{ .name = "x509", .program = "gpgsm",
> -	  .verify_args = x509_verify_args,
> -	  .sigs = x509_sigs
> +	{
> +		.name = "x509",
> +		.program = "gpgsm",
> +		.verify_args = x509_verify_args,
> +		.sigs = x509_sigs,
> +		.verify_signed_buffer = verify_gpg_signed_buffer,
> +		.sign_buffer = sign_buffer_gpg,
>  	},
>  };

I think that verify_signed_buffer and sign_buffer should replace
verify_args and sigs, not be alongside them. In particular, I see from
later patches that a new entry will be introduced for SSH, and the
corresponding new "verify" function does not use verify_args or sigs.

> @@ -279,10 +300,6 @@ static int verify_signed_buffer(const char *payload, size_t payload_size,
>  		return -1;
>  	}
>  
> -	fmt = get_format_by_sig(signature);
> -	if (!fmt)
> -		BUG("bad signature '%s'", signature);

Here is the difference in functionality that I spotted. Here, lack of
fmt is fatal...

> @@ -309,35 +330,32 @@ static int verify_signed_buffer(const char *payload, size_t payload_size,
>  int check_signature(const char *payload, size_t plen, const char *signature,
>  	size_t slen, struct signature_check *sigc)
>  {
> -	struct strbuf gpg_output = STRBUF_INIT;
> -	struct strbuf gpg_status = STRBUF_INIT;
> +	struct gpg_format *fmt;
>  	int status;
>  
>  	sigc->result = 'N';
>  	sigc->trust_level = -1;
>  
> -	status = verify_signed_buffer(payload, plen, signature, slen,
> -				      &gpg_output, &gpg_status);
> -	if (status && !gpg_output.len)
> -		goto out;
> -	sigc->payload = xmemdupz(payload, plen);
> -	sigc->gpg_output = strbuf_detach(&gpg_output, NULL);
> -	sigc->gpg_status = strbuf_detach(&gpg_status, NULL);
> -	parse_gpg_output(sigc);
> +	fmt = get_format_by_sig(signature);
> +	if (!fmt)
> +		return error(_("bad/incompatible signature '%s'"), signature);

...but here it is not.

Junio C Hamano July 29, 2021, 12:58 a.m. UTC | #2

Jonathan Tan <[e-mail]> writes:

>> -	fmt = get_format_by_sig(signature);
>> -	if (!fmt)
>> -		BUG("bad signature '%s'", signature);
>
> Here is the difference in functionality that I spotted. Here, lack of
> fmt is fatal...
>
>> +	fmt = get_format_by_sig(signature);
>> +	if (!fmt)
>> +		return error(_("bad/incompatible signature '%s'"), signature);
>
> ...but here it is not.

While I was reviewing this step, I was assumign that the callers
would respond to this error return appropriately.  If it is not the
case, then we do have to fix that.

The original's use of BUG() is wrong in any case, I woud think.  The
"signature" there is an external input, so we were reporting a data
error (it should have been die()), not a program logic error.

Thanks.

Fabian Stelzer July 29, 2021, 7:44 a.m. UTC | #3

On 29.07.21 02:58, Junio C Hamano wrote:
> Jonathan Tan <[e-mail]> writes:
> 
>>> -	fmt = get_format_by_sig(signature);
>>> -	if (!fmt)
>>> -		BUG("bad signature '%s'", signature);
>>
>> Here is the difference in functionality that I spotted. Here, lack of
>> fmt is fatal...
>>
>>> +	fmt = get_format_by_sig(signature);
>>> +	if (!fmt)
>>> +		return error(_("bad/incompatible signature '%s'"), signature);
>>
>> ...but here it is not.
> 
> While I was reviewing this step, I was assumign that the callers
> would respond to this error return appropriately.  If it is not the
> case, then we do have to fix that.
> 
> The original's use of BUG() is wrong in any case, I woud think.  The
> "signature" there is an external input, so we were reporting a data
> error (it should have been die()), not a program logic error.
> 
> Thanks.
> 

My intention was to actually change this behavior (i should have made 
that clear in the commit message). When the current git version 
encounters an unknown signature format it will BUG() and leave the user 
with a coredump.
I assume that some repos will have multiple different signature formats 
in their history in the future and the effect i would have liked was to 
only mark the commits with unknown signatures as bad/unknown when using 
git log --show-signature for example.
I have checked all the calls to check_signature and unfortunately the 
result check is really inconsistent. Some ignore it completely, others 
check but still then dereference fields from the sigcheck struct.
So for now a die() is probably the correct way to go.

In the future we might want to differentiate between verifying a single 
commit (in which case we can die()) and a list of commits. Or fix all 
the calls to check_signature to check the return code.

Thanks

Fabian Stelzer July 29, 2021, 8:43 a.m. UTC | #4

On 29.07.21 00:32, Jonathan Tan wrote:
> I think this patch set is beyond the "is this a good idea in general"
> phase (in particular, I think that being able to sign Git commits by
> using SSH infrastructure is very useful), so I'll proceed to critiquing
> the commits in more detail.

Thanks for your help.

> 
> Firstly, in commit messages, the left side of the colon is usually the
> name of the subsystem - in this case, "gpg-interface".
> 

The docs call this "name of the component you're working on". Since this 
code does not actually change any gpg functionality (at least it should 
not) i think gpg-interface in the commits might be a bit misleading.


>> To be able to implement new signing formats this commit:
>>   - makes the sigc structure more generic by renaming "gpg_output" to
>>     "output"
>>   - introduces function pointers in the gpg_format structure to call
>>     format specific signing and verification functions
>>   - moves format detection from verify_signed_buffer into the check_signature
>>     api function and calls the format specific verify
>>   - renames and wraps sign_buffer to handle format specific signing logic
>>     as well
> 
> I think that this commit should be further split up - in particular, it
> is hard for reviewers to verify that there is no difference in
> functionality before and after this commit. I already spotted one
> difference - perhaps there are more. For me, splitting the above 4
> points into 4 commits would be an acceptable split.
> 

The rename can of course be easily separated. The others would probably 
require some code in between commits that's not present in the final 
patch result to make the individual commits compile / work. Otherwise 
those would only add unused code with the last commit then actually 
using everything. I don't think that would make things easier to verify, 
would it?


> [snip]
> 
>>   static struct gpg_format gpg_format[] = {
>> -	{ .name = "openpgp", .program = "gpg",
>> -	  .verify_args = openpgp_verify_args,
>> -	  .sigs = openpgp_sigs
>> +	{
>> +		.name = "openpgp",
>> +		.program = "gpg",
>> +		.verify_args = openpgp_verify_args,
>> +		.sigs = openpgp_sigs,
>> +		.verify_signed_buffer = verify_gpg_signed_buffer,
>> +		.sign_buffer = sign_buffer_gpg,
>>   	},
>> -	{ .name = "x509", .program = "gpgsm",
>> -	  .verify_args = x509_verify_args,
>> -	  .sigs = x509_sigs
>> +	{
>> +		.name = "x509",
>> +		.program = "gpgsm",
>> +		.verify_args = x509_verify_args,
>> +		.sigs = x509_sigs,
>> +		.verify_signed_buffer = verify_gpg_signed_buffer,
>> +		.sign_buffer = sign_buffer_gpg,
>>   	},
>>   };
> 
> I think that verify_signed_buffer and sign_buffer should replace
> verify_args and sigs, not be alongside them. In particular, I see from
> later patches that a new entry will be introduced for SSH, and the
> corresponding new "verify" function does not use verify_args or sigs.
> 

I kept the verify_args since i would either have to duplicate the 
verify_gpg_signed_buffer for gpg & gpgsm or have an if within deciding 
what format to use.
Also this is something that we might want to make a configuration option 
in the future and pass to ssh-keygen as well (there are a couple of -O 
options for it users might want)

sigs is still needed for the parse_signed_buffer api function.

Patch

diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index 0f66818e0f8..fb300bb4b67 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -526,11 +526,11 @@  static void fmt_merge_msg_sigs(struct strbuf *out)
 			buf = payload.buf;
 			len = payload.len;
 			if (check_signature(payload.buf, payload.len, sig.buf,
-					 sig.len, &sigc) &&
-				!sigc.gpg_output)
+					    sig.len, &sigc) &&
+			    !sigc.output)
 				strbuf_addstr(&sig, "gpg verification failed.\n");
 			else
-				strbuf_addstr(&sig, sigc.gpg_output);
+				strbuf_addstr(&sig, sigc.output);
 		}
 		signature_check_clear(&sigc);
 
diff --git a/gpg-interface.c b/gpg-interface.c
index 127aecfc2b0..31cf4ba3938 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -15,6 +15,12 @@  struct gpg_format {
 	const char *program;
 	const char **verify_args;
 	const char **sigs;
+	int (*verify_signed_buffer)(struct signature_check *sigc,
+				    struct gpg_format *fmt, const char *payload,
+				    size_t payload_size, const char *signature,
+				    size_t signature_size);
+	int (*sign_buffer)(struct strbuf *buffer, struct strbuf *signature,
+			   const char *signing_key);
 };
 
 static const char *openpgp_verify_args[] = {
@@ -35,14 +41,29 @@  static const char *x509_sigs[] = {
 	NULL
 };
 
+static int verify_gpg_signed_buffer(struct signature_check *sigc,
+				    struct gpg_format *fmt, const char *payload,
+				    size_t payload_size, const char *signature,
+				    size_t signature_size);
+static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
+			   const char *signing_key);
+
 static struct gpg_format gpg_format[] = {
-	{ .name = "openpgp", .program = "gpg",
-	  .verify_args = openpgp_verify_args,
-	  .sigs = openpgp_sigs
+	{
+		.name = "openpgp",
+		.program = "gpg",
+		.verify_args = openpgp_verify_args,
+		.sigs = openpgp_sigs,
+		.verify_signed_buffer = verify_gpg_signed_buffer,
+		.sign_buffer = sign_buffer_gpg,
 	},
-	{ .name = "x509", .program = "gpgsm",
-	  .verify_args = x509_verify_args,
-	  .sigs = x509_sigs
+	{
+		.name = "x509",
+		.program = "gpgsm",
+		.verify_args = x509_verify_args,
+		.sigs = x509_sigs,
+		.verify_signed_buffer = verify_gpg_signed_buffer,
+		.sign_buffer = sign_buffer_gpg,
 	},
 };
 
@@ -72,7 +93,7 @@  static struct gpg_format *get_format_by_sig(const char *sig)
 void signature_check_clear(struct signature_check *sigc)
 {
 	FREE_AND_NULL(sigc->payload);
-	FREE_AND_NULL(sigc->gpg_output);
+	FREE_AND_NULL(sigc->output);
 	FREE_AND_NULL(sigc->gpg_status);
 	FREE_AND_NULL(sigc->signer);
 	FREE_AND_NULL(sigc->key);
@@ -257,16 +278,16 @@  error:
 	FREE_AND_NULL(sigc->key);
 }
 
-static int verify_signed_buffer(const char *payload, size_t payload_size,
-				const char *signature, size_t signature_size,
-				struct strbuf *gpg_output,
-				struct strbuf *gpg_status)
+static int verify_gpg_signed_buffer(struct signature_check *sigc,
+				    struct gpg_format *fmt, const char *payload,
+				    size_t payload_size, const char *signature,
+				    size_t signature_size)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
-	struct gpg_format *fmt;
 	struct tempfile *temp;
 	int ret;
-	struct strbuf buf = STRBUF_INIT;
+	struct strbuf gpg_stdout = STRBUF_INIT;
+	struct strbuf gpg_stderr = STRBUF_INIT;
 
 	temp = mks_tempfile_t(".git_vtag_tmpXXXXXX");
 	if (!temp)
@@ -279,10 +300,6 @@  static int verify_signed_buffer(const char *payload, size_t payload_size,
 		return -1;
 	}
 
-	fmt = get_format_by_sig(signature);
-	if (!fmt)
-		BUG("bad signature '%s'", signature);
-
 	strvec_push(&gpg.args, fmt->program);
 	strvec_pushv(&gpg.args, fmt->verify_args);
 	strvec_pushl(&gpg.args,
@@ -290,18 +307,22 @@  static int verify_signed_buffer(const char *payload, size_t payload_size,
 		     "--verify", temp->filename.buf, "-",
 		     NULL);
 
-	if (!gpg_status)
-		gpg_status = &buf;
-
 	sigchain_push(SIGPIPE, SIG_IGN);
-	ret = pipe_command(&gpg, payload, payload_size,
-			   gpg_status, 0, gpg_output, 0);
+	ret = pipe_command(&gpg, payload, payload_size, &gpg_stdout, 0,
+			   &gpg_stderr, 0);
 	sigchain_pop(SIGPIPE);
 
 	delete_tempfile(&temp);
 
-	ret |= !strstr(gpg_status->buf, "\n[GNUPG:] GOODSIG ");
-	strbuf_release(&buf); /* no matter it was used or not */
+	ret |= !strstr(gpg_stdout.buf, "\n[GNUPG:] GOODSIG ");
+	sigc->payload = xmemdupz(payload, payload_size);
+	sigc->output = strbuf_detach(&gpg_stderr, NULL);
+	sigc->gpg_status = strbuf_detach(&gpg_stdout, NULL);
+
+	parse_gpg_output(sigc);
+
+	strbuf_release(&gpg_stdout);
+	strbuf_release(&gpg_stderr);
 
 	return ret;
 }
@@ -309,35 +330,32 @@  static int verify_signed_buffer(const char *payload, size_t payload_size,
 int check_signature(const char *payload, size_t plen, const char *signature,
 	size_t slen, struct signature_check *sigc)
 {
-	struct strbuf gpg_output = STRBUF_INIT;
-	struct strbuf gpg_status = STRBUF_INIT;
+	struct gpg_format *fmt;
 	int status;
 
 	sigc->result = 'N';
 	sigc->trust_level = -1;
 
-	status = verify_signed_buffer(payload, plen, signature, slen,
-				      &gpg_output, &gpg_status);
-	if (status && !gpg_output.len)
-		goto out;
-	sigc->payload = xmemdupz(payload, plen);
-	sigc->gpg_output = strbuf_detach(&gpg_output, NULL);
-	sigc->gpg_status = strbuf_detach(&gpg_status, NULL);
-	parse_gpg_output(sigc);
+	fmt = get_format_by_sig(signature);
+	if (!fmt)
+		return error(_("bad/incompatible signature '%s'"), signature);
+
+	status = fmt->verify_signed_buffer(sigc, fmt, payload, plen, signature,
+					   slen);
+
+	if (status && !sigc->output)
+		return !!status;
+
 	status |= sigc->result != 'G';
 	status |= sigc->trust_level < configured_min_trust_level;
 
- out:
-	strbuf_release(&gpg_status);
-	strbuf_release(&gpg_output);
-
 	return !!status;
 }
 
 void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 {
-	const char *output = flags & GPG_VERIFY_RAW ?
-		sigc->gpg_status : sigc->gpg_output;
+	const char *output = flags & GPG_VERIFY_RAW ? sigc->gpg_status :
+							    sigc->output;
 
 	if (flags & GPG_VERIFY_VERBOSE && sigc->payload)
 		fputs(sigc->payload, stdout);
@@ -441,6 +459,12 @@  const char *get_signing_key(void)
 }
 
 int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
+{
+	return use_format->sign_buffer(buffer, signature, signing_key);
+}
+
+static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
+		    const char *signing_key)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
 	int ret;
diff --git a/gpg-interface.h b/gpg-interface.h
index 80567e48948..feac4decf8b 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -17,7 +17,7 @@  enum signature_trust_level {
 
 struct signature_check {
 	char *payload;
-	char *gpg_output;
+	char *output;
 	char *gpg_status;
 
 	/*
diff --git a/log-tree.c b/log-tree.c
index 7b823786c2c..20af9bd1c82 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -513,10 +513,10 @@  static void show_signature(struct rev_info *opt, struct commit *commit)
 
 	status = check_signature(payload.buf, payload.len, signature.buf,
 				 signature.len, &sigc);
-	if (status && !sigc.gpg_output)
+	if (status && !sigc.output)
 		show_sig_lines(opt, status, "No signature\n");
 	else
-		show_sig_lines(opt, status, sigc.gpg_output);
+		show_sig_lines(opt, status, sigc.output);
 	signature_check_clear(&sigc);
 
  out:
@@ -583,8 +583,8 @@  static int show_one_mergetag(struct commit *commit,
 		/* could have a good signature */
 		status = check_signature(payload.buf, payload.len,
 					 signature.buf, signature.len, &sigc);
-		if (sigc.gpg_output)
-			strbuf_addstr(&verify_message, sigc.gpg_output);
+		if (sigc.output)
+			strbuf_addstr(&verify_message, sigc.output);
 		else
 			strbuf_addstr(&verify_message, "No signature\n");
 		signature_check_clear(&sigc);
diff --git a/pretty.c b/pretty.c
index b1ecd039cef..daa71394efd 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1432,8 +1432,8 @@  static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 			check_commit_signature(c->commit, &(c->signature_check));
 		switch (placeholder[1]) {
 		case 'G':
-			if (c->signature_check.gpg_output)
-				strbuf_addstr(sb, c->signature_check.gpg_output);
+			if (c->signature_check.output)
+				strbuf_addstr(sb, c->signature_check.output);
 			break;
 		case '?':
 			switch (c->signature_check.result) {