Re: [PATCH 2/3] receive-pack: verify push options in cert

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Jonathan Tan wrote:

> In commit f6a4e61 ("push: accept push options", 2016-07-14), send-pack
> was taught to include push options both within the signed cert (if the
> push is a signed push) and outside the signed cert; however,
> receive-pack ignores push options within the cert, only handling push
> options outside the cert.

Yikes.  Thanks for fixing it.

[...]
> --- a/Documentation/git-receive-pack.txt
> +++ b/Documentation/git-receive-pack.txt
> @@ -106,6 +106,16 @@ the following environment variables:
>  	Also read about `receive.certNonceSlop` variable in
>  	linkgit:git-config[1].
>  
> +`GIT_PUSH_CERT_OPTION_STATUS`::
> +`BAD`;;
> +	For backwards compatibility reasons, when accepting a signed
> +	push, receive-pack requires that push options be written both
> +	inside and outside the certificate. ("git push" does this
> +	automatically.) `BAD` is returned if they are inconsistent.

In this manual page the reader doesn't need to know it's for backword
compatibility.  It is simply what the protocol requires.

The protocol doc and especially the commit message are better places
to talk about rationale (as you already are doing nicely in patch 3).

> +`OK`;;
> +	The push options inside and outside the certificate are
> +	consistent.

Hm.  I would have thought it would be simpler to simply reject the
push without invoking hooks in the BAD case.  But
GIT_PUSH_CERT_NONCE_STATUS provides precedent for this, forcing me to
think longer.

Is the idea that invoking the hook despite a bad client is a way to
provide an opportunity to audit log the error?

... okay, I've thought longer.  I think this is a different kind of
error than a bad nonce: for example, a bad nonce might be the result
of a misconfiguration that makes the client get the wrong nonce or a
sign of a caller trying to brute-force a stable nonce.  For
comparison, this error is a more simple protocol violation, like a
malformed pkt-line.

When we see a malformed pkt-line, we error out and do not invoke the
pre-receive hook.  For this error I think we should behave the same
way: show an error to the user and abort the connection, without
invoking hooks.

[...]
> --- a/t/t5534-push-signed.sh
> +++ b/t/t5534-push-signed.sh
> @@ -124,6 +124,21 @@ test_expect_success GPG 'signed push sends push certificate' '
>  	test_cmp expect dst/push-cert-status
>  '
>  
> +test_expect_success GPG 'signed push reports push option status in receive hook' '

Is there a straightforward way to test the error case?  (If there
isn't, I can live with that --- it would just be nice to have a test
that the behavior introduced here is preserved.)

Thanks and hope that helps,
Jonathan