Re: [RFC/PATCH] receive-pack.c: only accept push-cert if push_cert_nonce was advertised

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 9, 2015, 23:05 UTC
Message-ID: <xmqqfvbjblmd.fsf@gitster.dls.corp.google.com>
In-Reply-To: <xmqqsifjbmu2.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 32 quoted lines

>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index 4c069c5..628d13a 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -1276,7 +1276,8 @@ static struct command *read_head_info(struct sha1_array *shallow)
>>  				use_atomic = 1;
>>  		}
>>  
>> -		if (!strcmp(line, "push-cert")) {
>> +		if (push_cert_nonce &&
>> +		    !strcmp(line, "push-cert")) {
>>  			int true_flush = 0;
>>  			char certbuf[1024];
>
> This implementation is somewhat questionable.
>
> The server knows how to parse "push-cert" line, knows that what
> follows after that line up to "push-cert-end" line are shaped very
> differently from protocol commands outside the push-cert block.  In
> other words, it knows how to parse the request meant for the capable
> server; it just wants to refuse to serve that request.
>
> The patched code will make it fail by hoping that queue_command()
> that only handles "40-hex 40-hex ref" will reject the line that
> begins with "push-cert".  Instead of relying on such a hidden
> dependency, wouldn't it be cleaner to actually parse the push-cert
> block and then at the end notice and explictly say "Your requests
> were syntactically correct, but I am not going to honor your request
> to use the push-cert extension, because I never told you that I'd
> offer you that capability", instead of rejecting the request with "I
> was expecting old/new/ref but you sent a line with 'push-cert' on
> it; what are you talking about?"

Note that I said "somewhat" questionable, not "horribly broken", because another part of me wants to say that this implementation may be better than "parse and reject" from security point of view. The pusher cannot tell if the push failed because the server is old to know about the extension, or the server is new but is refusing, by observing the failure.

But the other side of the same coin is that it makes it harder to diagnose the failure.

When the protocol exchange gets to this state, in practice, we know we are talking with somebody who has push privilege into the repository, so revealing a bit more about the server by taking "parse and reject" route may be a reasonable price to give diagnosis that is more useful to help the users in this case, though.