Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s

[Jeff King <peff@peff.net>]

On Wed, May 10, 2017 at 10:00:44AM -0700, Jonathan Nieder wrote:

> > Right, makes sense.  I wondered if GitHub should be turning on
> > allowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide
> > some internal refs[1], and they're things that users wouldn't want to
> > fetch. The problem for your case really is just on the client side, and
> > this patch fixes it.
> [...]
> > [1] The reachability checks from upload-pack don't actually do much on
> >     GitHub, because you can generally access the objects via the API or
> >     the web site anyway. So I'm not really opposed to turning on
> >     allowTipSHA1InWant if it would be useful for users, but after
> >     Jonathan's patch I don't see how it would be.
> 
> Given that, what would make me really happy is if github enables
> uploadpack.allowAnySHA1InWant.  That would be useful for me, at least.

One of my hesitations is that we've actually considered moving in the
opposite direction. The object storage for all of the repositories in a
network is shared, so I can fork git.git, push up malicious crap, and
then point people to:

  https://github.com/git/git/commit/$sha1

and it resolves. Obviously there's a social-engineering component to any
such attack, but it's not great. And even without security in mind, it's
potentially confusing. So we've looked at enforcing reachability from
the refs of git/git for a case like that. There's some collateral
damage, though (e.g., people might actually want to look at unreferenced
objects after a force-push). And there are complications around things
like refs/pull (which could still come from another fork, but which you
might reasonably want to reference as part of a PR in the context of
git/git).

Turning on allowAnySHA1InWant brings that confusion to "git fetch", too.
To some degree it's already there for refs/pull, but with the current
client you at least know that you're fetching PR refs (and they're not
even fetched by default). Whereas after Jonathan Tan's patch, you can
social-engineer somebody into:

  git fetch https://github.com/git/git $sha1

if you open a PR that points to some malicious $sha1. I don't think
that's a reason not to take his patch, though.

Arguably refs/pull/ is an abomination that mixes up ownership and should
be destroyed. There really isn't a great alternative, though, short of
representing it as a completely separate repository (which would mean
anybody fetching those refs would have to make a separate fetch
request).

But even leaving all the refs/pull stuff aside, allowAnySHA1InWant does
seem to increase that confusion, and I don't see a way around it short
of never sharing objects between repositories at all. So I think at most
we'd do allowReachableSHA1InWant.

-Peff