Re: [PATCH v3 1/9] Add commit, tag & push signing via SSH keys

> "Fabian Stelzer via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Fabian Stelzer <fs@gigacodes.de>
>> Subject: [PATCH v3 1/9] Add commit, tag & push signing via SSH keys
> If you chose "ssh signing:" as the common prefix for the series, use
> it consistently with this step, too.

>
>> 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.
>>
>> Start with adding the new signature format, new config options and
>> rename some fields for consistency.
> OK.
>
>> This feature makes git signing much more accessible to the average user.
>> Usually they have a SSH Key for pushing code already. Using it
>> for signing commits allows us to verify not only the transport but the
>> pushed code as well.
> Drop this paragraph or at least tone it down.  It may hold true only
> around your immediate circle but it is far from clear and obvious.
> I'd expect more people push over https:// than ssh://.
>
> We do not really require a new feature to make much more accessible
> for wide average user---making it just a bit more accessible to
> folks in your immediate circle is perfectly fine, as long as you are
> not harming other people ;-)

>
>> In our corporate environemnt we use PIV x509 Certs on Yubikeys for email
>> signing/encryption and ssh keys which i think is quite common
> Upcase "I".
>
>> (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.
> All of the above promises a wonderful new world, but what is left
> unclear is with this step alone how much of the new world we already
> gain.  When you ask others to read and understand your code, please
> give them a bit more hint to guide them what to expect and where you
> are taking them next.
>
>> diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
>> index 0f66818e0f8..1d7b64fa021 100644
>> --- a/fmt-merge-msg.c
>> +++ b/fmt-merge-msg.c
>> @@ -527,10 +527,10 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
>>   			len = payload.len;
>>   			if (check_signature(payload.buf, payload.len, sig.buf,
>>   					 sig.len, &sigc) &&
>> -				!sigc.gpg_output)
>> +				!sigc.output)
>>   				strbuf_addstr(&sig, "gpg verification failed.\n");
>>   			else
>> -				strbuf_addstr(&sig, sigc.gpg_output);
>> +				strbuf_addstr(&sig, sigc.output);
> These are "rename some fields for consistency" the proposed log
> message promised.  Makes sense, as you are taking the sigc structure
> away from pgp/gpg dependency.
>
>> diff --git a/gpg-interface.c b/gpg-interface.c
>> index 127aecfc2b0..3c9a48c8e7e 100644
>> --- a/gpg-interface.c
>> +++ b/gpg-interface.c
>> @@ -8,6 +8,7 @@
>>   #include "tempfile.h"
>>   
>>   static char *configured_signing_key;
>> +const char *ssh_allowed_signers, *ssh_revocation_file;
> Very likely these want to be file-scope statics?

>
>>   static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;
>>   
>>   struct gpg_format {
>> @@ -35,6 +36,14 @@ static const char *x509_sigs[] = {
>>   	NULL
>>   };
>>   
>> +static const char *ssh_verify_args[] = {
>> +	NULL
>> +};
> A blank line is missing from here.
>
>> +static const char *ssh_sigs[] = {
>> +	"-----BEGIN SSH SIGNATURE-----",
>> +	NULL
>> +};
>> +
>>   static struct gpg_format gpg_format[] = {
>>   	{ .name = "openpgp", .program = "gpg",
>>   	  .verify_args = openpgp_verify_args,
>> @@ -44,6 +53,9 @@ static struct gpg_format gpg_format[] = {
>>   	  .verify_args = x509_verify_args,
>>   	  .sigs = x509_sigs
>>   	},
>> +	{ .name = "ssh", .program = "ssh-keygen",
>> +	  .verify_args = ssh_verify_args,
>> +	  .sigs = ssh_sigs },
>>   };
>>   
>>   static struct gpg_format *use_format = &gpg_format[0];
>> @@ -72,7 +84,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 +269,15 @@ 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_signature(struct signature_check *sigc, struct gpg_format *fmt,
>> +	const char *payload, size_t payload_size,
>> +	const char *signature, size_t signature_size)
>>   {
> What is this hunk about?  The more generic name "verify-signed-buffer"
> is rescinded and gets replaced by a more GPG/PGP specific helper?
>
> You'd need to help readers a bit more by explaining in the proposed
> log message that you shifted the boundary of responsibility between
> check_signature() and verify_signed_buffer()---it used to be that
> the latter inspected the signed payload to see if it a valid GPG/PGP
> signature before doing GPG specific validation, but you want to make
> the former responsible for calling get_format_by_sig(), so that you
> can dispatch a totally new backend that sits next to this GPG
> specific one.
>
>>   	struct child_process gpg = CHILD_PROCESS_INIT;
>> -	struct gpg_format *fmt;
>>   	struct tempfile *temp;
>>   	int ret;
>> -	struct strbuf buf = STRBUF_INIT;
>> +	struct strbuf gpg_out = STRBUF_INIT;
>> +	struct strbuf gpg_err = STRBUF_INIT;
>>   
>>   	temp = mks_tempfile_t(".git_vtag_tmpXXXXXX");
>>   	if (!temp)
>> @@ -279,29 +290,28 @@ 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,
>> -		     "--status-fd=1",
>> -		     "--verify", temp->filename.buf, "-",
>> -		     NULL);
>> -
>> -	if (!gpg_status)
>> -		gpg_status = &buf;
>> +			"--status-fd=1",
>> +			"--verify", temp->filename.buf, "-",
>> +			NULL);
> What is going on around here?  Ahh, an unnecessary indentation
> change is fooling the diff and made the patch unreadable.  Sigh...
>
>>   	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_out, 0,
>> +				&gpg_err, 0);
> What is this change about?  Is it another unnecessary indentation
> change?  Please make sure you keep distraction to your readers to
> the minimum.
>
>> @@ -309,35 +319,36 @@ 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) {
>> +		error(_("bad/incompatible signature '%s'"), signature);
>> +		return -1;
>> +	}
>> +
>> +	if (!strcmp(fmt->name, "ssh")) {
>> +		status = verify_ssh_signature(sigc, fmt, payload, plen, signature, slen);
>> +	} else {
>> +		status = verify_gpg_signature(sigc, fmt, payload, plen, signature, slen);
>> +	}
> OK, so get_format_by_sig() now is used to dispatch to the right
> backend.  Which sort of makes sense, but ...
>
>   * "ssh" is the newcomer; it has no right to come before the
>     battle-tested existing one.
>
>   * If we are dispatching via "fmt" variable, we should add
>     fmt->verify() method to each of these formats, so that we don't
>     have to switch based on the name.
>
> IOW, this part should just be
>
> 	fmt = get_format_by_sig(signature);
> 	if (!fmt)
> 		return error(_("...bad signature..."));
> 	fmt->verify_signature(sigc, fmt, payload, plen, signature, slen);

>   
>> +	if (status && !sigc->output)
>> +		return !!status;
>> +
>>   	status |= sigc->result != 'G';
>>   	status |= sigc->trust_level < configured_min_trust_level;
> By the way, there is no verify_ssh_signature() function defined at
> this step [1/9], so this won't compile from the source at all.
> Please make sure that each step builds and passes tests.

>
> If I were doing this patch, I probably would NOT do anything related
> to "ssh" in this step.  Probably just doing
>
>   - rename gpg_* variables to generic names in codepaths that _will_
>     become generic in future steps (like "check_signature()"
>     function);
>
>   - introduce verify_signature member to the fmt struct;
>
>   - hoist get_format_by_sig()'s callsite to check_signature() from
>     its callee.
>
> would be sufficient amount of work for the first step.  Call that a
> preliminary refactoring and clean-up.
>
> And then in the second and subsequent steps, you may start adding
> additional code to support ssh signing, including the new instance
> of fmt that has verify_ssh_signature() as its verify_signature
> method, etc.
>
> Introducing ssh_allowed_signers and ssh_revocation_file at this step
> is way premature.  Nobody uses them in this step, the code that uses
> them is already referenced but missing (hence the code does not
> build), so they are only there to frustrate readers wondering what
> they are for and how they will be used.
>
> Thanks.