On Fri, Dec 14 2018, Jeff King wrote:
> On Thu, Dec 13, 2018 at 05:08:43PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> Now that we have this maybe we should discuss why these tests show
>> different things:
>>
>> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
>> > index 086f2c40f6..8b1217ea26 100755
>> > --- a/t/t5500-fetch-pack.sh
>> > +++ b/t/t5500-fetch-pack.sh
>> > @@ -628,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a
>> > test_commit -C server 6 &&
>> >
>> > git init client &&
>> > - test_must_fail git -C client fetch-pack ../server \
>> > +
>> > + # Other protocol versions (e.g. 2) allow fetching an unadvertised
>> > + # object, so run this test with the default protocol version (0).
>> > + test_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \
>> > $(git -C server rev-parse refs/heads/master^) 2>err &&
>>
>> What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for
>> v2 all the time?
>
> Yeah, I actually didn't realize it until working on the earlier series,
> but this is the documented behavior:
>
> $ git grep -hC3 'want <oid>' Documentation/technical/protocol-v2.txt
>
> A `fetch` request can take the following arguments:
>
> want <oid>
> Indicates to the server an object which the client wants to
> retrieve. Wants can be anything and are not limited to
> advertised objects.
>
> An interesting implication of this at GitHub (and possibly other
> hosters) is that it exposes objects from shared storage via unexpected
> repos. If I fork torvalds/linux to peff/linux and push up object X, a v0
> fetch will (by default) refuse to serve it to me. But v2 will happily
> hand it over, which may confuse people into thinking that the object is
> (or at least at some point was) in Linus's repository.
More importantly this bypasses the security guarantee we've had with the
default of uploadpack.allowAnySHA1InWant=false.
If I push a id_rsa to a topic branch on $githost and immediately realize
my mistake and delete it, someone with access to a log of SHA-1s
(e.g. an IRC bot spewing out from/to commits) won't be able to pull it
down. This is why we have that as a setting in the first place
(f8edeaa05d ("upload-pack: optionally allow fetching any sha1",
2016-11-11)).
Of course this is:
a) Subject to the "SECURITY" section of git-fetch, i.e. maybe this
doesn't even work in the face of a determined attacker.
b) Hosting sites like GitHub, GitLab, Gitweb etc. aren't doing
reachability checks when you view /commit/<id>. If I delete my topic
branch it'll be viewable until the next GC kicks in (which would
also be the case with uploadpack.allowAnySHA1InWant=true).
So I wonder how much we need to care about this in practice, but for
some configurations protocol v2 definitely breaks a security promise
we've been making, now whether that promise was always BS due to "a)"
above is another matter.
I'm inclined to say that in the face of that "SECURITY" section we
should just:
* Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by
default. Make saying uploadpack.allowReachableSHA1InWant=false warn
with "this won't work, see SECURITY...".
* The uploadpack.allowTipSHA1InWant setting will also be turned on by
default, and will be much faster, since it'll just degrade to
uploadpack.allowReachableSHA1InWant=true and we won't need any
reachability check. We'll also warn saying that setting it is
useless.
Otherwise what's th point of these settings given what we're talking
about in that "SECURITY" section? It seems to just lure users into a
false sense of security. Better to not make promises we're not confident
we can keep, and say that if you push your id_rsa you need to run a
gc/prune on the server.
