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

Junio C Hamano [off-list ref] writes:

quoted

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.