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

From: Jeff King <peff@peff.net>
Date: May 11, 2017, 09:59 UTC
Message-ID: <20170511095925.grmyagv4hesxqprj@sigill.intra.peff.net>
In-Reply-To: <20170510170044.GX28740@aiede.svl.corp.google.com>

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

Show 14 quoted lines

> > 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