Re: [PATCH 2/3] receive-pack: verify push options in cert
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- May 6, 2017, 00:10 UTC
- Message-ID
- <20170506001002.GK28740@aiede.svl.corp.google.com>
- In-Reply-To
- <01e098de2da2e5f0b341ab6d967d032d840c365e.1494027001.git.jonathantanmy@google.com>
Hi,
Jonathan Tan wrote:
Show 5 quoted lines
> 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.
[...]
Show 12 quoted lines
> --- 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.
[...]
Show 7 quoted lines
> --- 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