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

[Jonathan Tan <jonathantanmy@google.com>]

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.