Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> No, there is no good objective reason. I added it just after the atomic
> flag as that is what I implemented.
>
> Is there a reason for a particular order of capabilities? I always considered
> it a set of strings, i.e. any order is valid and there is no preference in
> which way to put it.

That is correct, but "there is no inherent order or grouping" does
not lead to "hence it is OK to put a new thing at a random place in
the middle."

If a reviewer sees a new thing at some specific point in a
collection, when the collection is understood not to have any
specific order or grouping, it makes the reviewer doubt the
belief--there must be a reason why this was placed not at the end;
otherwise a new thing wouldn't be placed randomly at the middle.

If you place a new thing at the end, it still leaves ambiguity; it
may be there because there is no inherent order or grouping, or the
new one is sorts later than the one that used to be at the last or
somehow related to it.  

> I agree on this for all content that can be modified by the user
> (e.g. files in the work tree such as .gitmodules), but the .git/config
> file cannot be changed remotely. So I wonder how an attack would
> look like for a hosting provider or anyone else?
> We still rely on a sane system and trust /etc/gitconfig
> so we do trust the host/admin but not the user?

It is not "we" do trust; it is "we let host/admin trust itself while
making sure that they can protect themselves from their users".

>> More importantly, if we plan to make this configurable and not make
>> the limit a hardwired constant of the wire protocol, it may be
>> better to advertise push-options capability with the limit, e.g.
>> "push-options=32" (or even "push-options=1024/32"), so that the
>> client side can count and abort early?
>
> Yeah we may want to start out with a strict format here indicating
> the parameters used for evaluating the size.
> So what do these numbers mean?
>
> I assume (and hence I should document that,) that the first (1024)
> is the maximum number of bytes per push option. The second
> number (32) is the number of push options (not the number of pkts,
> as one push option may take more than one pkt if the first number is
> larger than 65k, see the NEEDSWORK comment.)
>
> Do we really need 2 numbers, or could we just have one number
> describing the maximum total size in bytes before the remote rejects
> the connection?

That's for you to decide.  push-options=32 is probably OK but it
will prevent a well-behaving "git push" from catching a request that
will be rejected, if you are basing the receiver side decision on
the other number.

The suggestion was primarily my reaction after seeing the two new
calls to die() on the receiver side, whose message I was not sure
will be given to the user who is pushing, i.e.

> When not going over ssh://, does the user see these messages?

Your "git push" will be collecting the options in a string-list
while parsing the options, so it would be able to check immediately
upon seeing the advertised capability if it will trigger this
protocol error even before making the request, which would be a good
thing to do, and I am reasonably sure we can give a better error
message if we do that on the local side without having the receiver
to tell you (for one thing, we can i18n the local side error
message more easily).