Re: [PATCH v3 04/35] upload-pack: convert to a builtin

From: Jeff King <peff@peff.net>
Date: Mar 12, 2018, 23:52 UTC
Message-ID: <20180312235200.GF1968@sigill.intra.peff.net>
In-Reply-To: <20180312233747.GB235366@aiede.svl.corp.google.com>

On Mon, Mar 12, 2018 at 04:37:47PM -0700, Jonathan Nieder wrote:

Show 16 quoted lines

> Jeff King wrote:
> 
> > We could even give it an environment variable, which would allow
> > something like:
> >
> >   tar xf maybe-evil.git.tar
> >   cd maybe-evil
> >   export GIT_TRUST_REPO=false
> >   git log
> [...]
> As an internal implementation detail, this is so obviously fragile
> that it wouldn't give me any feeling of security. ;-)  So it should be
> strictly an improvement.
> 
> As a public-facing feature, I suspect it's a bad idea for exactly that
> reason.

So that pretty much kills off the GIT_TRUST_REPO idea, I guess.

Show 5 quoted lines

> FWIW for pager specifically I am going for a whitelisting approach:
> new commands would have to explicitly set ALLOW_PAGER if they want to
> respect pager config.  That doesn't guarantee people think about it
> again as things evolve but it should at least help with getting the
> right setting for new plumbing.

I suspect we'd be about as well off with the "don't trust the repo" internal flag. Touching the ALLOW_PAGER setup code is about as likely to set off red flags for the developers (or reviewers) as code that checks the "trust" flag.

Forcing a whitelist on ALLOW_PAGER _is_ more likely to catch people adding new commands. But I don't think we actually want to add more commands to the "safe to run in a malicious repo" list. It's already a slightly sketchy concept. This is really all about upload-pack and its existing promises.

But ALLOW_PAGER would _just_ fix the pager issue. When we inevitably find another problem spot, it won't help us there. But a global "trust" flag might.

I dunno. I guess I'm OK with either approach, but it seems like the global trust flag has more room to grow.

-Peff