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

[Stefan Beller <sbeller@google.com>]

On Sun, Jul 10, 2016 at 10:06 AM, Shawn Pearce <spearce@spearce.org> wrote:
> On Fri, Jul 8, 2016 at 5:31 PM, Stefan Beller <sbeller@google.com> wrote:
>> +
>> +       /* NEEDSWORK: expose the limitations to be configurable. */
>> +       int max_options = 32;
>> +
>> +       /*
>> +        * NEEDSWORK: expose the limitations to be configurable;
>> +        * Once the limit can be lifted, include a way for payloads
>> +        * larger than one pkt, e.g allow a payload of up to
>> +        * LARGE_PACKET_MAX - 1 only, and reserve the last byte
>> +        * to indicate whether the next pkt continues with this
>> +        * push option.
>> +        */
>> +       int max_size = 1024;
>
> Instead of this, what about a new config variable
> receive.maxCommandBytes[1] that places a limit on the number of bytes
> of pkt-line data the client can supply in both the command list ("old
> new ref"), push signature framing, and option list?

Including the whole command list is pretty smart as it actually tackles the
DoS problem as a whole. We shortly discussed having just one upper bound
limit for the push options alone, but we were distracted by the discussion
on whether to advertise this number or just reject it on the server side
after it filled up so much data.

The design here with two bounds was used to not care about the oversized
push options for now. (I mean we can still just reject larger push
options even when
having a receive.maxCommandBytes setting.)

>
> Memory demands for the server are proportional to the data sent. A
> simple byte limit lets the user make the decision about how this gets
> used. Longer ref names or option values means fewer refs or options
> can be sent. Shorter ref names or option values means more values or
> options can be sent.
>
> I studied a lot of repositories[2] and most use ref names under 200
> bytes in length. A 3 MiB default for receive.maxCommandBytes gives
> users something like 11,115 references in a single git push invocation
> if they used all 200 bytes in every name. Most users don't have ref
> names this long. Unlike a cap on each ref, it allows users to use the
> full 65449 bytes in a reference name available in pkt-line, but you
> can only send 48 such references. Likewise for options. :)

In an earlier discussion Jeff said roughly "either make it work well,
or don't make it work at all, i.e. why are git push options better
than a `git push .. && curl <server>/REST-API` thing?"

And by having this design we could punt on the corner cases with
transmitting arbitrary large push options/binaries for now and claim
it's another next step that needs to be done when adding the config
option for it. By having a single receive.maxCommandBytes setting
we would sweep that problem under the rug and people could wonder
why it fails with the large push option.

As said in an earlier email as a side note, we could think about introducing
a v2 pkt line format which starts with a variable int to indicate the packet
size, such that the payload is not bound up to 64k.

I think 3MiB is a bit much for everyday use though and not enough for
corner cases?

>
>
> [1] I may propose this to JGit.
> [2] More than 3M, but maybe Peff has access to more.