diff mbox series

[2/4] upload-pack.c: allow banning certain object filter(s)

Message ID f0982d24e74155f6c0e405e5e3ae8c3e579f798a.1593720075.git.me@ttaylorr.com (mailing list archive)
State New, archived
Headers show
Series upload-pack: custom allowed object filters | expand

Commit Message

Taylor Blau July 2, 2020, 8:06 p.m. UTC
Git clients may ask the server for a partial set of objects, where the
set of objects being requested is refined by one or more object filters.
Server administrators can configure 'git upload-pack' to allow or ban
these filters by setting the 'uploadpack.allowFilter' variable to
'true' or 'false', respectively.

However, administrators using bitmaps may wish to allow certain kinds of
object filters, but ban others. Specifically, they may wish to allow
object filters that can be optimized by the use of bitmaps, while
rejecting other object filters which aren't and represent a perceived
performance degradation (as well as an increased load factor on the
server).

Allow configuring 'git upload-pack' to support object filters on a
case-by-case basis by introducing two new configuration variables:

  - 'uploadpack.filter.allow'
  - 'uploadpack.filter.<kind>.allow'

where '<kind>' may be one of 'blobNone', 'blobLimit', 'tree', and so on.

Setting the second configuration variable for any valid value of
'<kind>' explicitly allows or disallows restricting that kind of object
filter.

If a client requests the object filter <kind> and the respective
configuration value is not set, 'git upload-pack' will default to the
value of 'uploadpack.filter.allow', which itself defaults to 'true' to
maintain backwards compatibility. Note that this differs from
'uploadpack.allowfilter', which controls whether or not the 'filter'
capability is advertised.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config/uploadpack.txt | 16 ++++++
 t/t5616-partial-clone.sh            | 26 ++++++++++
 upload-pack.c                       | 78 +++++++++++++++++++++++++++++
 3 files changed, 120 insertions(+)

Comments

Jeff King July 8, 2020, 8:45 a.m. UTC | #1
On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:

> +static void parse_object_filter_config(const char *var, const char *value,
> +				       struct upload_pack_data *data)
> +{
> +	struct strbuf spec = STRBUF_INIT;
> +	const char *sub, *key;
> +	size_t sub_len;
> +
> +	if (parse_config_key(var, "uploadpack", &sub, &sub_len, &key))
> +		return;
> +	if (!sub || !skip_prefix(sub, "filter.", &sub))
> +		return;

Just while I'm thinking about the config name and case-sensitivity (from
the cover letter): if we did want to use this scheme, then
skip_iprefix() would make this behave more like a regular part of the
section name.

But I'd prefer to just do away with it by using a scheme that doesn't
have the extra layer of dots.

> +	if (sub != key)
> +		strbuf_add(&spec, sub, key - sub - 1);
> +	strbuf_tolower(&spec);

On the flip side, I'd actually consider _not_ matching the filter name
case-insensitively. We don't do so elsewhere (e.g., "git rev-list
--filter=BLOB:NONE" will complain).

-Peff
SZEDER Gábor July 15, 2020, 10 a.m. UTC | #2
On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:
> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> index 8a27452a51..5dcd0b5656 100755
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -235,6 +235,32 @@ test_expect_success 'implicitly construct combine: filter with repeated flags' '
>  	test_cmp unique_types.expected unique_types.actual
>  '
>  
> +test_expect_success 'upload-pack fails banned object filters' '
> +	# Test case-insensitivity by intentional use of "blob:None" rather than
> +	# "blob:none".
> +	test_config -C srv.bare uploadpack.filter.blob:None.allow false &&
> +	test_must_fail git clone --no-checkout --filter=blob:none \
> +		"file://$(pwd)/srv.bare" pc3 2>err &&
> +	test_i18ngrep "filter '\''blob:none'\'' not supported" err
> +'
> +
> +test_expect_success 'upload-pack fails banned combine object filters' '
> +	test_config -C srv.bare uploadpack.filter.allow false &&
> +	test_config -C srv.bare uploadpack.filter.combine.allow true &&
> +	test_config -C srv.bare uploadpack.filter.tree.allow true &&
> +	test_config -C srv.bare uploadpack.filter.blob:none.allow false &&
> +	test_must_fail git clone --no-checkout --filter=tree:1 \
> +		--filter=blob:none "file://$(pwd)/srv.bare" pc3 2>err &&
> +	test_i18ngrep "filter '\''blob:none'\'' not supported" err
> +'
> +
> +test_expect_success 'upload-pack fails banned object filters with fallback' '
> +	test_config -C srv.bare uploadpack.filter.allow false &&
> +	test_must_fail git clone --no-checkout --filter=blob:none \
> +		"file://$(pwd)/srv.bare" pc3 2>err &&
> +	test_i18ngrep "filter '\''blob:none'\'' not supported" err
> +'

These three tests are very flaky: 'git upload-pack' can error out
while clone is still sending packets (usually the 'done' line),
resulting in SIGPIPE and frequent CI failures.  Running this test
script with '-r 1,2,17-19 --stress' tends to fail in a couple of
seconds.

Using 'test_must_fail ok=sigpipe', as you did in the test in the last
patch, avoids the test failure caused by SIGPIPE, of course, but,
unfortunately, all three tests remain flaky, because the expected
error message sometimes doesn't make it to 'git clone's stderr, e.g.:

  expecting success of 5616.19 'upload-pack fails banned object filters with fallback': 
          test_config -C srv.bare uploadpack.filter.allow false &&
          test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
                  "file://$(pwd)/srv.bare" pc3 2>err &&
          test_i18ngrep "filter 'blob:none' not supported" err
  
  + test_config -C srv.bare uploadpack.filter.allow false
  + pwd
  + test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none file:///home/szeder/src/git/t/trash directory.t5616-partial-clone.stress-2/srv.bare pc3
  + test_i18ngrep filter 'blob:none' not supported err
  error: 'grep filter 'blob:none' not supported err' didn't find a match in:
  Cloning into 'pc3'...
  fatal: git upload-pack: banned object filter requested
  error: last command exited with $?=1
  not ok 19 - upload-pack fails banned object filters with fallback


Once upon a time I had a PoC patch to deal with 'git upload-pack'
aborting while 'git fetch' is still send_request()-ing, by catching
the write error to the closed connection and trying read any pending
ERR packets; Christian cleaned it up and submitted it with a proper
commit message in

  https://public-inbox.org/git/20200422163357.27056-1-chriscool@tuxfamily.org/

but it haven't been picked up yet.  Disappointingly, that patch
doesn't solve these issues...  I haven't looked what's going on
(perhaps 'git clone' does something differently than 'git fetch'?  no
idea)
Jeff King July 15, 2020, 10:55 a.m. UTC | #3
On Wed, Jul 15, 2020 at 12:00:43PM +0200, SZEDER Gábor wrote:

> Once upon a time I had a PoC patch to deal with 'git upload-pack'
> aborting while 'git fetch' is still send_request()-ing, by catching
> the write error to the closed connection and trying read any pending
> ERR packets; Christian cleaned it up and submitted it with a proper
> commit message in
> 
>   https://public-inbox.org/git/20200422163357.27056-1-chriscool@tuxfamily.org/
> 
> but it haven't been picked up yet.  Disappointingly, that patch
> doesn't solve these issues...  I haven't looked what's going on
> (perhaps 'git clone' does something differently than 'git fetch'?  no
> idea)

I suspect it is that fetch ignores SIGPIPE, but clone does not. So even
when we see a 141 exit code from fetch, it is probably generated
synthetically from exit(141) after we saw EPIPE. And your patch works
there because we have a chance to pump the read-side of the pipe,
whereas in git-clone we die immediately via the signal.

Probably git-clone should ignore SIGPIPE during the network transfer
portion of the process for the same reasons given in 143588949c (fetch:
ignore SIGPIPE during network operation, 2019-03-03).

-Peff
Taylor Blau July 20, 2020, 8:05 p.m. UTC | #4
On Wed, Jul 08, 2020 at 04:45:27AM -0400, Jeff King wrote:
> On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:
>
> > +static void parse_object_filter_config(const char *var, const char *value,
> > +				       struct upload_pack_data *data)
> > +{
> > +	struct strbuf spec = STRBUF_INIT;
> > +	const char *sub, *key;
> > +	size_t sub_len;
> > +
> > +	if (parse_config_key(var, "uploadpack", &sub, &sub_len, &key))
> > +		return;
> > +	if (!sub || !skip_prefix(sub, "filter.", &sub))
> > +		return;
>
> Just while I'm thinking about the config name and case-sensitivity (from
> the cover letter): if we did want to use this scheme, then
> skip_iprefix() would make this behave more like a regular part of the
> section name.
>
> But I'd prefer to just do away with it by using a scheme that doesn't
> have the extra layer of dots.

Yeah. I definitely flip-flopped on this when preparing this for the
list. I still feel like 'uploadpackfilter' is gross, so I was hoping to
send it with 'uploadpack.filter' (which reads nicely, but forces us to
write some gross code), but both options are frustrating to me for
different reasons.

Playing around with it more, though, I think that uploadpackfilter is
our best bet. I'd hate to introduce a string manipulation bug by munging
the reuslt of 'parse_config_key()', which is so clearly designed for
something like 'uploadpackfilter[.<filter>].allow'.

> > +	if (sub != key)
> > +		strbuf_add(&spec, sub, key - sub - 1);
> > +	strbuf_tolower(&spec);
>
> On the flip side, I'd actually consider _not_ matching the filter name
> case-insensitively. We don't do so elsewhere (e.g., "git rev-list
> --filter=BLOB:NONE" will complain).

I dropped the case-insensitive matching in the latest revision. I don't
think that we need to be overly accommodating here.

> -Peff

Thanks,
Taylor
Taylor Blau July 20, 2020, 8:07 p.m. UTC | #5
On Wed, Jul 15, 2020 at 06:55:21AM -0400, Jeff King wrote:
> On Wed, Jul 15, 2020 at 12:00:43PM +0200, SZEDER Gábor wrote:
>
> > Once upon a time I had a PoC patch to deal with 'git upload-pack'
> > aborting while 'git fetch' is still send_request()-ing, by catching
> > the write error to the closed connection and trying read any pending
> > ERR packets; Christian cleaned it up and submitted it with a proper
> > commit message in
> >
> >   https://public-inbox.org/git/20200422163357.27056-1-chriscool@tuxfamily.org/
> >
> > but it haven't been picked up yet.  Disappointingly, that patch
> > doesn't solve these issues...  I haven't looked what's going on
> > (perhaps 'git clone' does something differently than 'git fetch'?  no
> > idea)
>
> I suspect it is that fetch ignores SIGPIPE, but clone does not. So even
> when we see a 141 exit code from fetch, it is probably generated
> synthetically from exit(141) after we saw EPIPE. And your patch works
> there because we have a chance to pump the read-side of the pipe,
> whereas in git-clone we die immediately via the signal.

Heh. I was hoping to be rid of those errors with Christian's patches,
but it sounds like the problem is coming from outside of 'upload-pack'
and instead in 'clone'.

That reasoning seems sound to me, but I'd rather not touch clone in this
patch series if I don't have to. What I'd rather do is something like:

  - Introduce this patch series with the 'test_must_fail ok=sigpipe',
    and no error checking.

  - Modify clone to swallow these errors and eat a packet or two.

  - Then, drop the 'ok=sigpipe' from t5616 after 'git clone' is a little
    bit smarter here.

Maybe more steps than is strictly necessary, but I think it keeps the
scope of the review on this series reasonable, which is a tradeoff that
I'm willing to make.

> Probably git-clone should ignore SIGPIPE during the network transfer
> portion of the process for the same reasons given in 143588949c (fetch:
> ignore SIGPIPE during network operation, 2019-03-03).
>
> -Peff

Thanks,
Taylor
Jeff King July 20, 2020, 8:21 p.m. UTC | #6
On Mon, Jul 20, 2020 at 04:07:39PM -0400, Taylor Blau wrote:

> Heh. I was hoping to be rid of those errors with Christian's patches,
> but it sounds like the problem is coming from outside of 'upload-pack'
> and instead in 'clone'.

Yes, it's definitely on the client side.

> That reasoning seems sound to me, but I'd rather not touch clone in this
> patch series if I don't have to. What I'd rather do is something like:
> 
>   - Introduce this patch series with the 'test_must_fail ok=sigpipe',
>     and no error checking.
> 
>   - Modify clone to swallow these errors and eat a packet or two.
> 
>   - Then, drop the 'ok=sigpipe' from t5616 after 'git clone' is a little
>     bit smarter here.
> 
> Maybe more steps than is strictly necessary, but I think it keeps the
> scope of the review on this series reasonable, which is a tradeoff that
> I'm willing to make.

I think that's reasonable.

It doesn't look like the "pump the read side after EPIPE" patch is even
on the "seen" branch yet, and you'd need to further extend it to clone.
It's definitely a big enough change that it's better not to lump it in
with this series.

-Peff
SZEDER Gábor July 22, 2020, 9:17 a.m. UTC | #7
On Mon, Jul 20, 2020 at 04:07:39PM -0400, Taylor Blau wrote:
> What I'd rather do is something like:
> 
>   - Introduce this patch series with the 'test_must_fail ok=sigpipe',
>     and no error checking.

die_if_using_banned_filter() shows two error messages: a "fancy" one
is sent to the client in the ERR packet, including which particular
filter is not supported/allowed, and a simple 

  die(_("git upload-pack: banned object filter requested"));

If this die() were to show the same fancy error message as in the ERR
packet, then it would always make it to 'git clone's stderr in the
tests, so the tests could reliably check that 'git upload-pack' died
for the expected reason.

>   - Modify clone to swallow these errors and eat a packet or two.
> 
>   - Then, drop the 'ok=sigpipe' from t5616 after 'git clone' is a little
>     bit smarter here.
> 
> Maybe more steps than is strictly necessary, but I think it keeps the
> scope of the review on this series reasonable, which is a tradeoff that
> I'm willing to make.
> 
> > Probably git-clone should ignore SIGPIPE during the network transfer
> > portion of the process for the same reasons given in 143588949c (fetch:
> > ignore SIGPIPE during network operation, 2019-03-03).
> >
> > -Peff
> 
> Thanks,
> Taylor
SZEDER Gábor July 22, 2020, 9:21 a.m. UTC | #8
On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:
> +static void die_if_using_banned_filter(struct upload_pack_data *data)
> +{
> +	struct list_objects_filter_options *banned = banned_filter(data,
> +								   &data->filter_options);
> +	if (!banned)
> +		return;
> +
> +	packet_writer_error(&data->writer, _("filter '%s' not supported\n"),

Should we really translate the content of the ERR packet?

> +			    list_object_filter_config_name(banned->choice));
> +	die(_("git upload-pack: banned object filter requested"));
> +}
> +
>  static void receive_needs(struct upload_pack_data *data,
>  			  struct packet_reader *reader)
>  {
Taylor Blau July 22, 2020, 8:15 p.m. UTC | #9
On Wed, Jul 22, 2020 at 11:17:58AM +0200, SZEDER Gábor wrote:
> On Mon, Jul 20, 2020 at 04:07:39PM -0400, Taylor Blau wrote:
> > What I'd rather do is something like:
> >
> >   - Introduce this patch series with the 'test_must_fail ok=sigpipe',
> >     and no error checking.
>
> die_if_using_banned_filter() shows two error messages: a "fancy" one
> is sent to the client in the ERR packet, including which particular
> filter is not supported/allowed, and a simple
>
>   die(_("git upload-pack: banned object filter requested"));
>
> If this die() were to show the same fancy error message as in the ERR
> packet, then it would always make it to 'git clone's stderr in the
> tests, so the tests could reliably check that 'git upload-pack' died
> for the expected reason.

Beautiful idea. I changed this in my fork, and I'll send it to this
thread after 2.28 is out, since I don't want to create a distraction in
the meantime.

> >   - Modify clone to swallow these errors and eat a packet or two.
> >
> >   - Then, drop the 'ok=sigpipe' from t5616 after 'git clone' is a little
> >     bit smarter here.
> >
> > Maybe more steps than is strictly necessary, but I think it keeps the
> > scope of the review on this series reasonable, which is a tradeoff that
> > I'm willing to make.
> >
> > > Probably git-clone should ignore SIGPIPE during the network transfer
> > > portion of the process for the same reasons given in 143588949c (fetch:
> > > ignore SIGPIPE during network operation, 2019-03-03).
> > >
> > > -Peff
> >
> > Thanks,
> > Taylor

Thanks,
Taylor
Taylor Blau July 22, 2020, 8:16 p.m. UTC | #10
On Wed, Jul 22, 2020 at 11:21:29AM +0200, SZEDER Gábor wrote:
> On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:
> > +static void die_if_using_banned_filter(struct upload_pack_data *data)
> > +{
> > +	struct list_objects_filter_options *banned = banned_filter(data,
> > +								   &data->filter_options);
> > +	if (!banned)
> > +		return;
> > +
> > +	packet_writer_error(&data->writer, _("filter '%s' not supported\n"),
>
> Should we really translate the content of the ERR packet?

s/ERR packet/dying message after your other piece of advice, but I think
so. This is eventually going to make its way to users, so translating it
seems to make sense to me.

I guess they would get it in the localization of the server, not the
client, but that doesn't sway me away from this, either.

> > +			    list_object_filter_config_name(banned->choice));
> > +	die(_("git upload-pack: banned object filter requested"));
> > +}
> > +
> >  static void receive_needs(struct upload_pack_data *data,
> >  			  struct packet_reader *reader)
> >  {

Thanks,
Taylor
Junio C Hamano July 23, 2020, 1:41 a.m. UTC | #11
Taylor Blau <me@ttaylorr.com> writes:

>> If this die() were to show the same fancy error message as in the ERR
>> packet, then it would always make it to 'git clone's stderr in the
>> tests, so the tests could reliably check that 'git upload-pack' died
>> for the expected reason.
>
> Beautiful idea. I changed this in my fork, and I'll send it to this
> thread after 2.28 is out, since I don't want to create a distraction in
> the meantime.

Thanks.  

As long as everybody understands that "distraction" will immediately
gets backburnered when a regression relevant to the upcoming release
is found, however, I think performing work as usual is a good thing.
After all, we often find glitches in the current code by trying to
build on it, in addition to using it.
Taylor Blau July 23, 2020, 1:50 a.m. UTC | #12
On Wed, Jul 22, 2020 at 06:41:33PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> >> If this die() were to show the same fancy error message as in the ERR
> >> packet, then it would always make it to 'git clone's stderr in the
> >> tests, so the tests could reliably check that 'git upload-pack' died
> >> for the expected reason.
> >
> > Beautiful idea. I changed this in my fork, and I'll send it to this
> > thread after 2.28 is out, since I don't want to create a distraction in
> > the meantime.
>
> Thanks.
>
> As long as everybody understands that "distraction" will immediately
> gets backburnered when a regression relevant to the upcoming release
> is found, however, I think performing work as usual is a good thing.
> After all, we often find glitches in the current code by trying to
> build on it, in addition to using it.

Fair enough ;-). I sent v2 a few minutes ago, but forgot to attach it to
this thread as a reply. So, I suppose we can pick up the review there,
instead.

Thanks,
Taylor
SZEDER Gábor July 23, 2020, 7:51 a.m. UTC | #13
On Wed, Jul 22, 2020 at 04:16:34PM -0400, Taylor Blau wrote:
> On Wed, Jul 22, 2020 at 11:21:29AM +0200, SZEDER Gábor wrote:
> > On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:
> > > +static void die_if_using_banned_filter(struct upload_pack_data *data)
> > > +{
> > > +	struct list_objects_filter_options *banned = banned_filter(data,
> > > +								   &data->filter_options);
> > > +	if (!banned)
> > > +		return;
> > > +
> > > +	packet_writer_error(&data->writer, _("filter '%s' not supported\n"),
> >
> > Should we really translate the content of the ERR packet?
> 
> s/ERR packet/dying message after your other piece of advice, but I think
> so. This is eventually going to make its way to users, so translating it
> seems to make sense to me.
> 
> I guess they would get it in the localization of the server, not the
> client, but that doesn't sway me away from this, either.

Please note that the contents of other ERR packets are not translated,
either.

$ git grep -A1 -h packet_writer_error origin/seen:upload-pack.c
			packet_writer_error(&data->writer,
					    "upload-pack: not our ref %s",
--
			packet_writer_error(&data->writer,
					    "upload-pack: not our ref %s",
--
			packet_writer_error(writer,
					    "upload-pack: not our ref %s",
--
			packet_writer_error(writer, "unknown ref %s", arg);
			die("unknown ref %s", arg);


> > > +			    list_object_filter_config_name(banned->choice));
> > > +	die(_("git upload-pack: banned object filter requested"));
> > > +}
> > > +
> > >  static void receive_needs(struct upload_pack_data *data,
> > >  			  struct packet_reader *reader)
> > >  {
> 
> Thanks,
> Taylor
Taylor Blau July 23, 2020, 2:13 p.m. UTC | #14
On Thu, Jul 23, 2020 at 09:51:44AM +0200, SZEDER Gábor wrote:
> On Wed, Jul 22, 2020 at 04:16:34PM -0400, Taylor Blau wrote:
> > On Wed, Jul 22, 2020 at 11:21:29AM +0200, SZEDER Gábor wrote:
> > > On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:
> > > > +static void die_if_using_banned_filter(struct upload_pack_data *data)
> > > > +{
> > > > +	struct list_objects_filter_options *banned = banned_filter(data,
> > > > +								   &data->filter_options);
> > > > +	if (!banned)
> > > > +		return;
> > > > +
> > > > +	packet_writer_error(&data->writer, _("filter '%s' not supported\n"),
> > >
> > > Should we really translate the content of the ERR packet?
> >
> > s/ERR packet/dying message after your other piece of advice, but I think
> > so. This is eventually going to make its way to users, so translating it
> > seems to make sense to me.
> >
> > I guess they would get it in the localization of the server, not the
> > client, but that doesn't sway me away from this, either.
>
> Please note that the contents of other ERR packets are not translated,
> either.
>
> $ git grep -A1 -h packet_writer_error origin/seen:upload-pack.c
> 			packet_writer_error(&data->writer,
> 					    "upload-pack: not our ref %s",
> --
> 			packet_writer_error(&data->writer,
> 					    "upload-pack: not our ref %s",
> --
> 			packet_writer_error(writer,
> 					    "upload-pack: not our ref %s",
> --
> 			packet_writer_error(writer, "unknown ref %s", arg);
> 			die("unknown ref %s", arg);

Fair enough... if there are other changes to make I'll drop the
translation, otherwise this should be easy enough to apply when queuing.

Junio, if you'd rather I reroll the series again, too, that's fine as
well.

>
> > > > +			    list_object_filter_config_name(banned->choice));
> > > > +	die(_("git upload-pack: banned object filter requested"));
> > > > +}
> > > > +
> > > >  static void receive_needs(struct upload_pack_data *data,
> > > >  			  struct packet_reader *reader)
> > > >  {
> >
> > Thanks,
> > Taylor

Thanks,
Taylor
diff mbox series

Patch

diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
index ed1c835695..fd4970306c 100644
--- a/Documentation/config/uploadpack.txt
+++ b/Documentation/config/uploadpack.txt
@@ -57,6 +57,22 @@  uploadpack.allowFilter::
 	If this option is set, `upload-pack` will support partial
 	clone and partial fetch object filtering.
 
+uploadpack.filter.allow::
+	Provides a default value for unspecified object filters (see: the
+	below configuration variable).
+	Defaults to `true`.
+
+uploadpack.filter.<filter>.allow::
+	Explicitly allow or ban the object filter corresponding to
+	`<filter>`, where `<filter>` may be one of: `blob:none`,
+	`blob:limit`, `tree`, `sparse:oid`, or `combine`. If using
+	combined filters, both `combine` and all of the nested filter
+	kinds must be allowed.  Defaults to `uploadpack.filter.allow`.
++
+Note that the dot between 'filter' and '<filter>' is both non-standard
+and intentional. This is done to avoid a parsing ambiguity when
+specifying this configuration as an argument to Git's top-level `-c`.
+
 uploadpack.allowRefInWant::
 	If this option is set, `upload-pack` will support the `ref-in-want`
 	feature of the protocol version 2 `fetch` command.  This feature
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 8a27452a51..5dcd0b5656 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -235,6 +235,32 @@  test_expect_success 'implicitly construct combine: filter with repeated flags' '
 	test_cmp unique_types.expected unique_types.actual
 '
 
+test_expect_success 'upload-pack fails banned object filters' '
+	# Test case-insensitivity by intentional use of "blob:None" rather than
+	# "blob:none".
+	test_config -C srv.bare uploadpack.filter.blob:None.allow false &&
+	test_must_fail git clone --no-checkout --filter=blob:none \
+		"file://$(pwd)/srv.bare" pc3 2>err &&
+	test_i18ngrep "filter '\''blob:none'\'' not supported" err
+'
+
+test_expect_success 'upload-pack fails banned combine object filters' '
+	test_config -C srv.bare uploadpack.filter.allow false &&
+	test_config -C srv.bare uploadpack.filter.combine.allow true &&
+	test_config -C srv.bare uploadpack.filter.tree.allow true &&
+	test_config -C srv.bare uploadpack.filter.blob:none.allow false &&
+	test_must_fail git clone --no-checkout --filter=tree:1 \
+		--filter=blob:none "file://$(pwd)/srv.bare" pc3 2>err &&
+	test_i18ngrep "filter '\''blob:none'\'' not supported" err
+'
+
+test_expect_success 'upload-pack fails banned object filters with fallback' '
+	test_config -C srv.bare uploadpack.filter.allow false &&
+	test_must_fail git clone --no-checkout --filter=blob:none \
+		"file://$(pwd)/srv.bare" pc3 2>err &&
+	test_i18ngrep "filter '\''blob:none'\'' not supported" err
+'
+
 test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '
 	rm -rf src dst &&
 	git init src &&
diff --git a/upload-pack.c b/upload-pack.c
index 39d0cf00be..a5f56d73cc 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -88,6 +88,7 @@  struct upload_pack_data {
 	enum allow_uor allow_uor;
 
 	struct list_objects_filter_options filter_options;
+	struct string_list allowed_filters;
 
 	struct packet_writer writer;
 
@@ -103,6 +104,7 @@  struct upload_pack_data {
 	unsigned no_progress : 1;
 	unsigned use_include_tag : 1;
 	unsigned allow_filter : 1;
+	unsigned allow_filter_fallback : 1;
 
 	unsigned done : 1;					/* v2 only */
 	unsigned allow_ref_in_want : 1;				/* v2 only */
@@ -120,6 +122,7 @@  static void upload_pack_data_init(struct upload_pack_data *data)
 	struct string_list deepen_not = STRING_LIST_INIT_DUP;
 	struct string_list uri_protocols = STRING_LIST_INIT_DUP;
 	struct object_array extra_edge_obj = OBJECT_ARRAY_INIT;
+	struct string_list allowed_filters = STRING_LIST_INIT_DUP;
 
 	memset(data, 0, sizeof(*data));
 	data->symref = symref;
@@ -131,6 +134,8 @@  static void upload_pack_data_init(struct upload_pack_data *data)
 	data->deepen_not = deepen_not;
 	data->uri_protocols = uri_protocols;
 	data->extra_edge_obj = extra_edge_obj;
+	data->allowed_filters = allowed_filters;
+	data->allow_filter_fallback = 1;
 	packet_writer_init(&data->writer, 1);
 
 	data->keepalive = 5;
@@ -147,6 +152,7 @@  static void upload_pack_data_clear(struct upload_pack_data *data)
 	string_list_clear(&data->deepen_not, 0);
 	object_array_clear(&data->extra_edge_obj);
 	list_objects_filter_release(&data->filter_options);
+	string_list_clear(&data->allowed_filters, 1);
 
 	free((char *)data->pack_objects_hook);
 }
@@ -983,6 +989,47 @@  static int process_deepen_not(const char *line, struct string_list *deepen_not,
 	return 0;
 }
 
+static int allows_filter_choice(struct upload_pack_data *data,
+				enum list_objects_filter_choice c)
+{
+	const char *key = list_object_filter_config_name(c);
+	struct string_list_item *item = string_list_lookup(&data->allowed_filters,
+							   key);
+	if (item)
+		return (intptr_t) item->util;
+	return data->allow_filter_fallback;
+}
+
+static struct list_objects_filter_options *banned_filter(
+	struct upload_pack_data *data,
+	struct list_objects_filter_options *opts)
+{
+	size_t i;
+
+	if (!allows_filter_choice(data, opts->choice))
+		return opts;
+
+	if (opts->choice == LOFC_COMBINE)
+		for (i = 0; i < opts->sub_nr; i++) {
+			struct list_objects_filter_options *sub = &opts->sub[i];
+			if (banned_filter(data, sub))
+				return sub;
+		}
+	return NULL;
+}
+
+static void die_if_using_banned_filter(struct upload_pack_data *data)
+{
+	struct list_objects_filter_options *banned = banned_filter(data,
+								   &data->filter_options);
+	if (!banned)
+		return;
+
+	packet_writer_error(&data->writer, _("filter '%s' not supported\n"),
+			    list_object_filter_config_name(banned->choice));
+	die(_("git upload-pack: banned object filter requested"));
+}
+
 static void receive_needs(struct upload_pack_data *data,
 			  struct packet_reader *reader)
 {
@@ -1013,6 +1060,7 @@  static void receive_needs(struct upload_pack_data *data,
 				die("git upload-pack: filtering capability not negotiated");
 			list_objects_filter_die_if_populated(&data->filter_options);
 			parse_list_objects_filter(&data->filter_options, arg);
+			die_if_using_banned_filter(data);
 			continue;
 		}
 
@@ -1169,6 +1217,33 @@  static int find_symref(const char *refname, const struct object_id *oid,
 	return 0;
 }
 
+static void parse_object_filter_config(const char *var, const char *value,
+				       struct upload_pack_data *data)
+{
+	struct strbuf spec = STRBUF_INIT;
+	const char *sub, *key;
+	size_t sub_len;
+
+	if (parse_config_key(var, "uploadpack", &sub, &sub_len, &key))
+		return;
+	if (!sub || !skip_prefix(sub, "filter.", &sub))
+		return;
+
+	if (sub != key)
+		strbuf_add(&spec, sub, key - sub - 1);
+	strbuf_tolower(&spec);
+
+	if (!strcmp(key, "allow")) {
+		if (spec.len)
+			string_list_insert(&data->allowed_filters, spec.buf)->util =
+				(void *)(intptr_t)git_config_bool(var, value);
+		else
+			data->allow_filter_fallback = git_config_bool(var, value);
+	}
+
+	strbuf_release(&spec);
+}
+
 static int upload_pack_config(const char *var, const char *value, void *cb_data)
 {
 	struct upload_pack_data *data = cb_data;
@@ -1208,6 +1283,8 @@  static int upload_pack_config(const char *var, const char *value, void *cb_data)
 			return git_config_string(&data->pack_objects_hook, var, value);
 	}
 
+	parse_object_filter_config(var, value, data);
+
 	return parse_hide_refs_config(var, value, "uploadpack");
 }
 
@@ -1388,6 +1465,7 @@  static void process_args(struct packet_reader *request,
 		if (data->allow_filter && skip_prefix(arg, "filter ", &p)) {
 			list_objects_filter_die_if_populated(&data->filter_options);
 			parse_list_objects_filter(&data->filter_options, p);
+			die_if_using_banned_filter(data);
 			continue;
 		}