Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

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