From: Junio C Hamano <gitster@pobox•com>
To: Stefan Beller <sbeller@google•com>
Cc: git@vger•kernel.org
Subject: Re: [RFC/PATCH] receive-pack.c: only accept push-cert if push_cert_nonce was advertised
Date: Fri, 09 Jan 2015 14:39:33 -0800 [thread overview]
Message-ID: <xmqqsifjbmu2.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1420836437-11068-1-git-send-email-sbeller@google.com> (Stefan Beller's message of "Fri, 9 Jan 2015 12:47:17 -0800")
Stefan Beller <sbeller@google•com> writes:
> If the server did not advertise the capability to have signed pushes
> it should not accept signed pushes as stated in
> Documentation/technical/protocol-capabilities.txt:
>
> Client will then send a space separated list of capabilities it wants
> to be in effect. The client MUST NOT ask for capabilities the server
> did not say it supports.
>
> Server MUST diagnose and abort if capabilities it does not understand
> was sent. Server MUST NOT ignore capabilities that client requested
> and server advertised. As a consequence of these rules, server MUST
> NOT advertise capabilities it does not understand.
>
> After rereading the second paragraph I think they should also be reworded to
>
> Server MUST diagnose and abort if capabilities it did not advertise
> was sent.
Except for s/was sent/was requested/, I think that rule makes sense
very much.
> 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?"
next prev parent reply other threads:[~2015-01-09 22:39 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-01-09 20:47 [RFC/PATCH] receive-pack.c: only accept push-cert if push_cert_nonce was advertised Stefan Beller
2015-01-09 22:39 ` Junio C Hamano [this message]
2015-01-09 23:05 ` Junio C Hamano
2015-01-09 23:15 ` Stefan Beller
2015-01-09 23:57 ` Junio C Hamano
2015-01-10 0:31 ` [PATCH] receive-pack.c: don't miss exporting unsolicited push certificates Stefan Beller
2015-01-10 1:52 ` Junio C Hamano
2015-01-10 3:55 ` Stefan Beller
2015-01-12 19:07 ` Junio C Hamano
2015-01-14 0:11 ` Stefan Beller
2015-01-14 18:08 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqsifjbmu2.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=sbeller@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox