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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 7, 2016, 22:06 UTC
Message-ID: <xmqqh9c1nlvm.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kbkv5P0wP2kKt9gzmZBe1DjLSB8JpZD66DT_Xd4NKqmKQ@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 6 quoted lines

> 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.

Show 6 quoted lines

> 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".

Show 19 quoted lines

>> 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).