Re: [RFC PATCH 0/2] upload-pack.c: limit allowed filter choices

[Taylor Blau <me@ttaylorr.com>]

On Fri, Apr 17, 2020 at 11:41:48AM +0200, Christian Couder wrote:
> Hi Taylor and Peff,
>
> On Wed, Mar 18, 2020 at 11:18 AM Jeff King <peff@peff.net> wrote:
> >
> > On Tue, Mar 17, 2020 at 02:39:05PM -0600, Taylor Blau wrote:
> >
> > > Of course, I would be happy to send along our patches. They are included
> > > in the series below, and correspond roughly to what we are running at
> > > GitHub. (For us, there have been a few more clean-ups and additional
> > > patches, but I squashed them into 2/2 below).
>
> Thanks for the patches, and sorry for the delay in responding!

No need to apologize. Clearly these had slipped my mind, too :).

> > > The approach is roughly that we have:
> > >
> > >   - 'uploadpack.filter.allow' -> specifying the default for unspecified
> > >     filter choices, itself defaulting to true in order to maintain
> > >     backwards compatibility, and
> > >
> > >   - 'uploadpack.filter.<filter>.allow' -> specifying whether or not each
> > >     filter kind is allowed or not. (Originally this was given as 'git
> > >     config uploadpack.filter=blob:none.allow true', but this '=' is
> > >     ambiguous to configuration given over '-c', which itself uses an '='
> > >     to separate keys from values.)
> >
> > One thing that's a little ugly here is the embedded dot in the
> > subsection (i.e., "filter.<filter>"). It makes it look like a four-level
> > key, but really there is no such thing in Git.  But everything else we
> > tried was even uglier.
> >
> > I think we want to declare a real subsection for each filter and not
> > just "uploadpack.filter.<filter>". That gives us room to expand to other
> > config options besides "allow" later on if we need to.
> >
> > We don't want to claim "uploadpack.allow" and "uploadpack.<filter>.allow";
> > that's too generic.
> >
> > Likewise "filter.allow" is too generic.
> >
> > We could do "uploadpackfilter.allow" and "uploadpackfilter.<filter>.allow",
> > but that's both ugly _and_ separates these options from the rest of
> > uploadpack.*.
>
> What do you think about something like:
>
> [promisorFilter "noBlobs"]
>         type = blob:none
>         uploadpack = true # maybe "allow" could also mean "true" here
>         ...
> ?

I'm not sure about introducing a layer of indirection here with
"noBlobs". It's nice that it could perhaps be enabled/disabled for
different builtins (e.g., by adding 'revList = false', say), but I'm not
convinced that this is improving all of those cases, either.

For example, what happens if I have something like:

  [uploadpack "filter.tree"]
    maxDepth = 1
    allow = true

but I want to use a different value of maxDepth for, say, rev-list? I'd
rather have two sections (each for the 'tree' filter, but scoped to
'upload-pack' and 'rev-list' separately) than write something like:

  [promisorFilter "treeDepth"]
          type = tree
          uploadpack = true
          uploadpackMaxDepth = 1
          revList = true
          revListMaxDepth = 0
          ...

So, yeah, the current system is not great because it has the '.' in the
second component. I am definitely eager to hear other suggestions about
naming it differently, but I think that the general structure is on
track.

One thing that I can think of (other than replacing the '.' with another
delimiting character other than '=') is renaming the key from
'uploadPack' to 'uploadPackFilter'. I believe that this was suggested by
Juino (?) earlier in the thread. I think that it's a fine resolution to
this, but I'm also not opposed to what is currently written in too above patches.

> > > I noted in the second patch that there is the unfortunate possibility of
> > > encountering a SIGPIPE when trying to write the ERR sideband back to a
> > > client who requested a non-supported filter. Peff and I have had some
> > > discussion off-list about resurrecting SZEDZER's work which makes room
> > > in the buffer by reading one packet back from the client when the server
> > > encounters a SIGPIPE. It is for this reason that I am marking the series
> > > as 'RFC'.
> >
> > For reference, the patch I was thinking of was this:
> >
> >   https://lore.kernel.org/git/20190830121005.GI8571@szeder.dev/
>
> Are you using the patches in this series with or without something
> like the above patch? I am ok to resend this patch series including
> the above patch (crediting Szeder) if you use something like it.

We're not using them, but without them we suffer from a problem that if
we can get a SIGPIPE when writing the "sorry, I don't support that
filter" message back to the client, then they won't receive it.

Szeder's patches help address that issue by catching the SIGPIPE and
popping off enough from the client buffer so that we can write the
message out before dying.

I appreciate your offer to resubmit the series on my behalf, but I was
already planning on doing this myself and wouldn't want to burden you
with another to-do. I'll be happy to take it on myself, probably within
a week or so.

> Thanks,
> Christian.

Thanks,
Taylor