git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Brandon Williams <bwilliamseng@gmail.com>,
	Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
Date: Fri, 14 Dec 2018 12:08:02 +0100	[thread overview]
Message-ID: <87mup81i3x.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <87o99o1iot.fsf@evledraar.gmail.com>


On Fri, Dec 14 2018, Ævar Arnfjörð Bjarmason wrote:

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

I sent that too fast. What I meant is that there's 3
settings. uploadpack.{allowTipSHA1InWant,allowReachableSHA1InWant,allowAnySHA1InWant}. Those
are, respectively, cheap/expensive/cheap to serve up.

We could make them cheap/cheap/cheap by just making the first two a
synonym for the 3rd (allowAnySHA1InWant), because as noted above

Also, parts of the v2 code, e.g. this in 685fbd3291 ("fetch-pack:
perform a fetch using v2", 2018-03-15):

	/* v2 supports these by default */
	allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;

Seem to have intended to turn on allowReachableSHA1InWant, not
allowAnySHA1InWant, but apparently that's what we're doing?

In any case, regardless of what v2 intended there anything except having
allowAnySHA1InWant on by default seems irresponsible given x) us
documenting in "SECURITY" that it doesn't really work y) even if it did,
there being a *tiny* minority of people who'd in some way care about
this feature who don't also run some sort of web UI on the git server
that'll be bypassing this setting no matter if it worked as intended for
the wire protocol.

  reply	other threads:[~2018-12-14 11:08 UTC|newest]

Thread overview: 73+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-11 10:42 [PATCH 0/3] protocol v2 and hidden refs Jeff King
2018-12-11 10:43 ` [PATCH 1/3] serve: pass "config context" through to individual commands Jeff King
2018-12-14  2:09   ` Junio C Hamano
2018-12-14  8:20     ` Jeff King
2018-12-15  0:31       ` Junio C Hamano
2018-12-16 10:25         ` Jeff King
2018-12-16 11:12           ` Junio C Hamano
2018-12-18 12:47             ` Jeff King
2018-12-14  8:36   ` Jonathan Nieder
2018-12-14  8:55     ` Jeff King
2018-12-14  9:28       ` Jonathan Nieder
2018-12-14  9:55         ` Jeff King
2018-12-11 10:43 ` [PATCH 2/3] parse_hide_refs_config: handle NULL section Jeff King
2018-12-14  2:11   ` Junio C Hamano
2018-12-11 10:44 ` [PATCH 3/3] upload-pack: support hidden refs with protocol v2 Jeff King
2018-12-11 11:45 ` [PATCH 0/3] protocol v2 and hidden refs Ævar Arnfjörð Bjarmason
2018-12-11 13:55   ` Jeff King
2018-12-11 21:21     ` [PATCH 0/3] Add a GIT_TEST_PROTOCOL_VERSION=X test mode Ævar Arnfjörð Bjarmason
2018-12-11 21:24       ` Ævar Arnfjörð Bjarmason
2018-12-11 21:21     ` [PATCH 1/3] tests: add a special setup where for protocol.version Ævar Arnfjörð Bjarmason
2018-12-12  0:27       ` [PATCH 0/3] Some fixes and improvements Jonathan Tan
2018-12-12  0:27         ` [PATCH 1/3] squash this into your patch Jonathan Tan
2018-12-12  0:27         ` [PATCH 2/3] builtin/fetch-pack: support protocol version 2 Jonathan Tan
2018-12-12  0:27         ` [PATCH 3/3] also squash this into your patch Jonathan Tan
2018-12-13  2:49         ` [PATCH 0/3] Some fixes and improvements Junio C Hamano
2018-12-13 15:58           ` [PATCH v2 0/8] protocol v2 fixes Ævar Arnfjörð Bjarmason
2018-12-17 22:40             ` [PATCH v3 0/4] " Ævar Arnfjörð Bjarmason
2018-12-18 12:48               ` Jeff King
2018-12-17 22:40             ` [PATCH v3 1/4] serve: pass "config context" through to individual commands Ævar Arnfjörð Bjarmason
2018-12-17 22:40             ` [PATCH v3 2/4] parse_hide_refs_config: handle NULL section Ævar Arnfjörð Bjarmason
2018-12-17 22:40             ` [PATCH v3 3/4] upload-pack: support hidden refs with protocol v2 Ævar Arnfjörð Bjarmason
2018-12-17 22:40             ` [PATCH v3 4/4] fetch-pack: support protocol version 2 Ævar Arnfjörð Bjarmason
2019-01-08 19:45               ` Junio C Hamano
2019-01-08 20:38                 ` Jonathan Tan
2019-01-08 21:14                   ` Jeff King
2018-12-13 15:58           ` [PATCH v2 1/8] serve: pass "config context" through to individual commands Ævar Arnfjörð Bjarmason
2018-12-13 15:58           ` [PATCH v2 2/8] parse_hide_refs_config: handle NULL section Ævar Arnfjörð Bjarmason
2018-12-13 15:58           ` [PATCH v2 3/8] upload-pack: support hidden refs with protocol v2 Ævar Arnfjörð Bjarmason
2018-12-13 15:58           ` [PATCH v2 4/8] tests: add a check for unportable env --unset Ævar Arnfjörð Bjarmason
2018-12-13 15:58           ` [PATCH v2 5/8] tests: add a special setup where for protocol.version Ævar Arnfjörð Bjarmason
2018-12-13 19:48             ` Jonathan Tan
2018-12-13 15:58           ` [PATCH v2 6/8] tests: mark & fix tests broken under GIT_TEST_PROTOCOL_VERSION=1 Ævar Arnfjörð Bjarmason
2018-12-13 15:58           ` [PATCH v2 7/8] builtin/fetch-pack: support protocol version 2 Ævar Arnfjörð Bjarmason
2018-12-14 10:17             ` Jeff King
2018-12-13 15:58           ` [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2 Ævar Arnfjörð Bjarmason
2018-12-13 16:08             ` Ævar Arnfjörð Bjarmason
2018-12-14  2:18               ` Junio C Hamano
2018-12-14 10:12               ` Jeff King
2018-12-14 10:55                 ` Ævar Arnfjörð Bjarmason
2018-12-14 11:08                   ` Ævar Arnfjörð Bjarmason [this message]
2018-12-17 19:59                     ` Jeff King
2018-12-17 19:57                   ` Jeff King
2018-12-17 22:16                     ` [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true Ævar Arnfjörð Bjarmason
2018-12-17 22:34                       ` David Turner
2018-12-17 22:57                         ` Ævar Arnfjörð Bjarmason
2018-12-17 23:07                           ` David Turner
2018-12-17 23:14                     ` [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2 Jonathan Nieder
2018-12-17 23:36                       ` Ævar Arnfjörð Bjarmason
2018-12-18  0:02                         ` Jonathan Nieder
2018-12-18  9:28                           ` Ævar Arnfjörð Bjarmason
2018-12-18 12:41                             ` Jeff King
2018-12-18 12:36                       ` Jeff King
2018-12-18 13:10                         ` Ævar Arnfjörð Bjarmason
2018-12-26 22:14                           ` Junio C Hamano
2018-12-27 11:26                             ` Ævar Arnfjörð Bjarmason
2018-12-27 17:10                               ` Jonathan Nieder
2018-12-11 21:21     ` [PATCH 2/3] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=1 Ævar Arnfjörð Bjarmason
2018-12-11 21:21     ` [PATCH 3/3] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2 Ævar Arnfjörð Bjarmason
2018-12-13 19:53 ` [PATCH 0/3] protocol v2 and hidden refs Jonathan Tan
2018-12-14  8:35   ` Jeff King
2018-12-15 19:53     ` Ævar Arnfjörð Bjarmason
2018-12-16 10:40       ` Jeff King
2018-12-16 11:47         ` Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87mup81i3x.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=bwilliamseng@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jonathantanmy@google.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).