| Message ID | 20181213155817.27666-9-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/8] serve: pass "config context" through to individual commands | expand |
On Thu, Dec 13 2018, Æ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? > test_i18ngrep "Server does not allow request for unadvertised object" err > ' > @@ -788,7 +791,7 @@ test_expect_success 'shallow clone exclude tag two' ' > ' > > test_expect_success 'fetch exclude tag one' ' > - git -C shallow12 fetch --shallow-exclude one origin && > + GIT_TEST_PROTOCOL_VERSION= git -C shallow12 fetch --shallow-exclude one origin && > git -C shallow12 log --pretty=tformat:%s origin/master >actual && > test_write_lines three two >expected && > test_cmp expected actual > @@ -806,7 +809,7 @@ test_expect_success 'fetching deepen' ' > git -C deepen log --pretty=tformat:%s master >actual && > echo three >expected && > test_cmp expected actual && > - git -C deepen fetch --deepen=1 && > + GIT_TEST_PROTOCOL_VERSION= git -C deepen fetch --deepen=1 && > git -C deepen log --pretty=tformat:%s origin/master >actual && > cat >expected <<-\EOF && > four > diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh > index 4ca48f0276..0e90a90294 100755 > --- a/t/t5503-tagfollow.sh > +++ b/t/t5503-tagfollow.sh > @@ -56,7 +56,7 @@ test_expect_success 'fetch A (new commit : 1 connection)' ' > rm -f $U && > ( > cd cloned && > - GIT_TRACE_PACKET=$UPATH git fetch && > + GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch && > test $A = $(git rev-parse --verify origin/master) > ) && > get_needs $U >actual && > @@ -86,7 +86,7 @@ test_expect_success 'fetch C, T (new branch, tag : 1 connection)' ' > rm -f $U && > ( > cd cloned && > - GIT_TRACE_PACKET=$UPATH git fetch && > + GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch && > test $C = $(git rev-parse --verify origin/cat) && > test $T = $(git rev-parse --verify tag1) && > test $A = $(git rev-parse --verify tag1^0) > @@ -122,7 +122,7 @@ test_expect_success 'fetch B, S (commit and tag : 1 connection)' ' > rm -f $U && > ( > cd cloned && > - GIT_TRACE_PACKET=$UPATH git fetch && > + GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch && > test $B = $(git rev-parse --verify origin/master) && > test $B = $(git rev-parse --verify tag2^0) && > test $S = $(git rev-parse --verify tag2) > @@ -146,7 +146,7 @@ test_expect_success 'new clone fetch master and tags' ' > cd clone2 && > git init && > git remote add origin .. && > - GIT_TRACE_PACKET=$UPATH git fetch && > + GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch && > test $B = $(git rev-parse --verify origin/master) && > test $S = $(git rev-parse --verify tag2) && > test $B = $(git rev-parse --verify tag2^0) && > diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh > index ca69636fd5..7b480587c9 100755 > --- a/t/t5512-ls-remote.sh > +++ b/t/t5512-ls-remote.sh > @@ -223,7 +223,7 @@ test_expect_success 'ls-remote --symref' ' > $(git rev-parse refs/tags/mark1.10) refs/tags/mark1.10 > $(git rev-parse refs/tags/mark1.2) refs/tags/mark1.2 > EOF > - git ls-remote --symref >actual && > + GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref >actual && > test_cmp expect actual > ' > > @@ -243,7 +243,7 @@ test_expect_failure 'ls-remote with filtered symref (--heads)' ' > 1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/foo > 1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/master > EOF > - git ls-remote --symref --heads . >actual && > + GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref --heads . >actual && > test_cmp expect actual > ' > > @@ -252,9 +252,9 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' ' > 1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/foo > 1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/master > EOF > - git ls-remote --symref --heads . >actual && > + GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref --heads . >actual && > test_cmp expect actual && > - git ls-remote --symref . "refs/heads/*" >actual && > + GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref . "refs/heads/*" >actual && > test_cmp expect actual > ' > > diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh > index 36b0dbc01c..f095555c3e 100755 > --- a/t/t5515-fetch-merge-logic.sh > +++ b/t/t5515-fetch-merge-logic.sh This one should really be looked at by someone more familiar with v2. Looks scary that we have different merge results with it. > @@ -147,7 +147,7 @@ do > do > git update-ref -d "$refname" "$val" > done > - git fetch "$@" >/dev/null > + GIT_TEST_PROTOCOL_VERSION= git fetch "$@" >/dev/null > cat .git/FETCH_HEAD > } >"$actual_f" && > git show-ref >"$actual_r" && > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh > index 08cdee0b95..1d1b717cd5 100755 > --- a/t/t5516-fetch-push.sh > +++ b/t/t5516-fetch-push.sh > @@ -1129,7 +1129,7 @@ do > ' > done > > -test_expect_success 'fetch exact SHA1' ' > +test_expect_success 'fetch exact SHA1 in protocol v0' ' > mk_test testrepo heads/master hidden/one && > git push testrepo master:refs/hidden/one && > ( > @@ -1148,7 +1148,8 @@ test_expect_success 'fetch exact SHA1' ' > test_must_fail git cat-file -t $the_commit && > > # fetching the hidden object should fail by default > - test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err && > + test_must_fail env GIT_TEST_PROTOCOL_VERSION= \ > + git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err && > test_i18ngrep "Server does not allow request for unadvertised object" err && > test_must_fail git rev-parse --verify refs/heads/copy && > > @@ -1205,7 +1206,8 @@ do > mk_empty shallow && > ( > cd shallow && > - test_must_fail git fetch --depth=1 ../testrepo/.git $SHA1 && > + test_must_fail env GIT_TEST_PROTOCOL_VERSION= \ > + git fetch --depth=1 ../testrepo/.git $SHA1 && > git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true && > git fetch --depth=1 ../testrepo/.git $SHA1 && > git cat-file commit $SHA1 > @@ -1233,15 +1235,18 @@ do > mk_empty shallow && > ( > cd shallow && > - test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 && > - test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_1 && > + test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \ > + git fetch ../testrepo/.git $SHA1_3 && > + test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \ > + git fetch ../testrepo/.git $SHA1_1 && > git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true && > git fetch ../testrepo/.git $SHA1_1 && > git cat-file commit $SHA1_1 && > test_must_fail git cat-file commit $SHA1_2 && > git fetch ../testrepo/.git $SHA1_2 && > git cat-file commit $SHA1_2 && > - test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 > + test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \ > + git fetch ../testrepo/.git $SHA1_3 > ) > ' Ditto all this stuff. > [...] > diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh > index e87164aa8f..a555e38845 100755 > --- a/t/t7406-submodule-update.sh > +++ b/t/t7406-submodule-update.sh > @@ -943,7 +943,8 @@ test_expect_success 'submodule update clone shallow submodule outside of depth' > cd super3 && > sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp && > mv -f .gitmodules.tmp .gitmodules && > - test_must_fail git submodule update --init --depth=1 2>actual && > + test_must_fail env GIT_TEST_PROTOCOL_VERSION= \ > + git submodule update --init --depth=1 2>actual && > test_i18ngrep "Direct fetching of that commit failed." actual && > git -C ../submodule config uploadpack.allowReachableSHA1InWant true && > git submodule update --init --depth=1 >actual && And this one and various other shallow things look odd, are shallow fetches different under v2?
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> + # 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? UnfortunatelyYes (see Peff's three-patch series).
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. -Peff
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.
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.
On Fri, Dec 14, 2018 at 11:55:30AM +0100, Ævar Arnfjörð Bjarmason wrote: > > 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. IMHO those security guarantees there are overrated (due to delta guessing attacks, though things are not quite as bad if the attacker can't actually push to the repo). But I agree that people do assume it's the case. I was certainly surprised by the v2 behavior, and I don't remember that aspect being discussed. (I suspect it was mostly because in the stateless world, the "fetch" operation requires iterating all of the refs again. That's made even harder by the fact that "ls-refs" takes arguments now, and might have only advertised a subset of the refs. How can "fetch" know which ones were advertised?). > a) Subject to the "SECURITY" section of git-fetch, i.e. maybe this > doesn't even work in the face of a determined attacker. Yeah, this is the part I was thinking about above. > 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). Actually, we don't ever prune unless a user asks us to (and certainly users do ask us to after accidentally pushing secret stuff). In GitHub's case, though, we always tell people to consider anything pushed to a public repo to be non-secret, even if it was exposed for only a few seconds. My understanding is that there are literally clients watching the public feeds and sucking down repo data looking for these kinds of mistakes. The /commit/<id> thing has been a frequent topic of discussion within GitHub over the years, and we have looked at actually doing reachability checks. They're expensive, though bitmaps help. > 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. No real argument from me. I have always thought those security guarantees were BS. However, I do think that's all orthogonal to the issue I was mentioning, which is that it looks like repo A wants to serve you object X, even if that repo never saw the object. That's unique to shared storage schemes, though. IMHO this is also a user education issue (it's a question of trust: refs are the source of trust, and objects don't need to be trusted because they're immutable). But it's pretty subtle for people who don't know the inner workings of Git. -Peff
On Fri, Dec 14, 2018 at 12:08:02PM +0100, Ævar Arnfjörð Bjarmason wrote: > 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? That I don't know. Maybe Brandon can answer what the intent was. I agree that turning on the reachability check would make it behavior more or less the same as v0 (at some additional cost to walk the refs again, but v0 smart-http already had to do that). -Peff
Hi, Jeff King wrote: > On Fri, Dec 14, 2018 at 11:55:30AM +0100, Ævar Arnfjörð Bjarmason wrote: >> More importantly this bypasses the security guarantee we've had with the >> default of uploadpack.allowAnySHA1InWant=false. > > IMHO those security guarantees there are overrated (due to delta > guessing attacks, though things are not quite as bad if the attacker > can't actually push to the repo). Do you have a proof of concept for delta guessing? My understanding was that without using a broken hash (e.g. uncorrected SHA-1), it is not feasible to carry out. JGit checks delta bases in received thin packs for reachability as well. > But I agree that people do assume it's the case. I was certainly > surprised by the v2 behavior, and I don't remember that aspect being > discussed. IMHO it's a plain bug (either in implementation or documentation). [...] >> 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. > > No real argument from me. I have always thought those security > guarantees were BS. This would make per-branch ACLs (as implemented both by Gerrit and gitolite) an essentially useless feature, so please no. I would be all for changing the default, but making turning off allowReachableSHA1InWant an unsupported deprecated thing is a step too far, in my opinion. Is there somewhere that we can document these kinds of invariants or goals so that we don't have to keep repeating the same discussions? Thanks, Jonathan
On Mon, Dec 17 2018, Jonathan Nieder wrote: > Hi, > > Jeff King wrote: >> On Fri, Dec 14, 2018 at 11:55:30AM +0100, Ævar Arnfjörð Bjarmason wrote: > >>> More importantly this bypasses the security guarantee we've had with the >>> default of uploadpack.allowAnySHA1InWant=false. >> >> IMHO those security guarantees there are overrated (due to delta >> guessing attacks, though things are not quite as bad if the attacker >> can't actually push to the repo). > > Do you have a proof of concept for delta guessing? My understanding > was that without using a broken hash (e.g. uncorrected SHA-1), it is > not feasible to carry out. > > JGit checks delta bases in received thin packs for reachability as > well. > >> But I agree that people do assume it's the case. I was certainly >> surprised by the v2 behavior, and I don't remember that aspect being >> discussed. > > IMHO it's a plain bug (either in implementation or documentation). > > [...] >>> 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. >> >> No real argument from me. I have always thought those security >> guarantees were BS. > > This would make per-branch ACLs (as implemented both by Gerrit and > gitolite) an essentially useless feature, so please no. Doesn't Gerrit always use JGit? The discussion upthread is about what we should do about git.git's version of this feature since we document it doesn't do its job from a security / ACL standpoint, but that doesn't preclude other server implementations from having a working version of this. But if gitolite is relying on this to hide refs, and if our security docs are to be believed, then it's just providing security through obscurity. Like you I'm curious about a POC. The patch I submitted in <20181217221625.1523-1-avarab@gmail.com> takes the "SECURITY" docs at face value. I.e. I think it's dangerous to expose this as-is given the scary non-promise they make, but perhaps we'll find that this actually works well enough in some cases, or there's other non-security uses for this (e.g. simply discouraging users from cloning certain things, e.g. for cache performance purposes). > I would be all for changing the default, but making turning off > allowReachableSHA1InWant an unsupported deprecated thing is a step too > far, in my opinion. > > Is there somewhere that we can document these kinds of invariants or > goals so that we don't have to keep repeating the same discussions? > > Thanks, > Jonathan
Ævar Arnfjörð Bjarmason wrote: > On Mon, Dec 17 2018, Jonathan Nieder wrote: >> This would make per-branch ACLs (as implemented both by Gerrit and >> gitolite) an essentially useless feature, so please no. > > Doesn't Gerrit always use JGit? > > The discussion upthread is about what we should do about git.git's > version of this feature since we document it doesn't do its job from a > security / ACL standpoint, but that doesn't preclude other server > implementations from having a working version of this. > > But if gitolite is relying on this to hide refs, and if our security > docs are to be believed, then it's just providing security through > obscurity. Yes, Gerrit uses JGit. JGit shares configuration with Git, so if we make that configuration in Git warn "This is just a placebo", then people will naturally assume that it's just a placebo in JGit, too. More importantly, if Git upstream stops caring about this use case, then the protocol and other features can evolve in directions that make it even harder to support. I am willing to take on some of the burden of keeping it working, even when I do not run any servers that use plain Git (I do interact with many). > Like you I'm curious about a POC. The patch I submitted in > <20181217221625.1523-1-avarab@gmail.com> takes the "SECURITY" docs at > face value. One of the difficulties that security engineers have to deal with is living without absolutes. In other words, their experience is constant risk/benefit tradeoffs: if you want 0% risk, then don't use a computer. ;-) If someone has thoughts on what would lead people to be comfortable with removing the SECURITY notice, then I'm happy to listen. As already mentioned, I am strongly against abandoning this use case. Jonathan
On Tue, Dec 18 2018, Jonathan Nieder wrote: > Ævar Arnfjörð Bjarmason wrote: >> On Mon, Dec 17 2018, Jonathan Nieder wrote: > >>> This would make per-branch ACLs (as implemented both by Gerrit and >>> gitolite) an essentially useless feature, so please no. >> >> Doesn't Gerrit always use JGit? >> >> The discussion upthread is about what we should do about git.git's >> version of this feature since we document it doesn't do its job from a >> security / ACL standpoint, but that doesn't preclude other server >> implementations from having a working version of this. >> >> But if gitolite is relying on this to hide refs, and if our security >> docs are to be believed, then it's just providing security through >> obscurity. > > Yes, Gerrit uses JGit. JGit shares configuration with Git, so if we > make that configuration in Git warn "This is just a placebo", then > people will naturally assume that it's just a placebo in JGit, too. Aside from the merits of this change it sounds like a good idea to document the server options JGit shares with us somewhere, or have a test mode similar to what I added in (but didn't follow-up on integrating) in https://public-inbox.org/git/20170516203712.15921-1-avarab@gmail.com/ > More importantly, if Git upstream stops caring about this use case, > then the protocol and other features can evolve in directions that > make it even harder to support. I am willing to take on some of the > burden of keeping it working, even when I do not run any servers that > use plain Git (I do interact with many). > >> Like you I'm curious about a POC. The patch I submitted in >> <20181217221625.1523-1-avarab@gmail.com> takes the "SECURITY" docs at >> face value. > > One of the difficulties that security engineers have to deal with is > living without absolutes. In other words, their experience is > constant risk/benefit tradeoffs: if you want 0% risk, then don't use a > computer. ;-) > > If someone has thoughts on what would lead people to be comfortable > with removing the SECURITY notice, then I'm happy to listen. As > already mentioned, I am strongly against abandoning this use case. For me this would be docs backed up by tests (which can start as a ML reply) showing a case where this actually works to hide data. I.e. have a repo with "master" and a "root-password" branch, where the "root-password" branch has content that's irresistible to "git repack" for delta purposes, do we end up sending the "root-password" content over on a fetch even when that branch isn't advertised & forbidden? Or, if that fails are there ways to make it work? E.g. using hidden/* in combination with delta islands?
On Mon, Dec 17, 2018 at 03:14:52PM -0800, Jonathan Nieder wrote: > > IMHO those security guarantees there are overrated (due to delta > > guessing attacks, though things are not quite as bad if the attacker > > can't actually push to the repo). > > Do you have a proof of concept for delta guessing? My understanding > was that without using a broken hash (e.g. uncorrected SHA-1), it is > not feasible to carry out. I think we may be talking about two different things. I mean an attack where you want to know what is in object X, so you ask the server for object Y and tell it that you already have X. If the sender generates a delta against X, that tells you something about what's in X. For a pure read-only server, you're restricted to the Y's that are already in the repo. So how effective this is depends on what's in X, and what Y's are available. For a case where X is in a victim repo you cannot make arbitrary writes to, but you _can_ make the victim repo aware of other objects (e.g., by opening a pull request that creates a ref), then you can iteratively provide many Y's, improving your guess about X in each iteration. For a case where the victim repo has fully shared storage (GitHub, and probably other hosts; I'm not sure if it's available yet, but GitLab is clearly working on shared-storage too), you can probably skip all that and just push a ref pointing to X with an empty pack (Git just cares that it has all of the objects afterwards, not that you pushed them). None of those care about the quality of the hash (they do assume you know the hash of X already, but then so does fetching by object id). And no, I've never written a proof-of-concept for that. It would depend largely on the data you're trying to extract. E.g., if you think X contains "root:XXXXXX", then you might hope to ask for "root:AXXXXX", then "root:BXXXXX", etc. You know you've got a hit when the delta gets smaller. So the complexity for guessing N bytes becomes 256*N, rather than 256^N. > > But I agree that people do assume it's the case. I was certainly > > surprised by the v2 behavior, and I don't remember that aspect being > > discussed. > > IMHO it's a plain bug (either in implementation or documentation). Or both. :) The behavior and the documentation seem to agree. > [...] > >> 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. > > > > No real argument from me. I have always thought those security > > guarantees were BS. > > This would make per-branch ACLs (as implemented both by Gerrit and > gitolite) an essentially useless feature, so please no. I think Ævar's argument is that those are providing a false sense of security now (at least for read permissions). Let me clarify my position a little. I won't claim the existing scheme provides _no_ value (especially the pure read-only case above is less dicey than the others). It's mostly that the protections are very hand-wavy. I don't trust them _now_, and I have little faith that other innocent-looking changes to the object negotiation and the packing code will not significantly weaken them. There's no security boundary expressed within Git's code, so there's a very high chance of information leaking accidentally. A trustable system would have boundaries built in from the ground up. Enough people seem to believe otherwise (i.e., that the hand-waving arguments are worth _something_) that I've never pushed to actually change the default behavior. But if Ævar wants to try to do so, I'm not going to stand in my way (hence my "no argument from me"). > I would be all for changing the default, but making turning off > allowReachableSHA1InWant an unsupported deprecated thing is a step too > far, in my opinion. Yes, I agree if we were to go down this road, it probably makes sense to flip the knobs and let them be "unflipped" if the user wants. > Is there somewhere that we can document these kinds of invariants or > goals so that we don't have to keep repeating the same discussions? It's not clear to me that there's consensus on the invariants or goals. ;) -Peff
On Tue, Dec 18, 2018 at 10:28:27AM +0100, Ævar Arnfjörð Bjarmason wrote: > I.e. have a repo with "master" and a "root-password" branch, where the > "root-password" branch has content that's irresistible to "git repack" > for delta purposes, do we end up sending the "root-password" content > over on a fetch even when that branch isn't advertised & forbidden? > > Or, if that fails are there ways to make it work? E.g. using hidden/* in > combination with delta islands? Delta islands wouldn't generally help here. They only tell us not to store on-disk deltas that fetching clients aren't likely to be able to reuse (i.e, they want X but will generally not have Y, so don't make a delta there). In the attacks I mentioned in my previous email, the deltas would usually be computed on the fly for each fetch. So the lack of on-disk deltas across "security boundaries" wouldn't help. You could disable on-the-fly delta compression, but the resulting packs are much larger (and you'd think it would save some server CPU, but experiments I've done show that it helps a lot less than you'd think, since we often end up zlib-deflating more bytes). -Peff
On Tue, Dec 18 2018, Jeff King wrote: > On Mon, Dec 17, 2018 at 03:14:52PM -0800, Jonathan Nieder wrote: > >> > IMHO those security guarantees there are overrated (due to delta >> > guessing attacks, though things are not quite as bad if the attacker >> > can't actually push to the repo). >> >> Do you have a proof of concept for delta guessing? My understanding >> was that without using a broken hash (e.g. uncorrected SHA-1), it is >> not feasible to carry out. > > I think we may be talking about two different things. I mean an attack > where you want to know what is in object X, so you ask the server for > object Y and tell it that you already have X. If the sender generates a > delta against X, that tells you something about what's in X. > > For a pure read-only server, you're restricted to the Y's that are > already in the repo. So how effective this is depends on what's in X, > and what Y's are available. > > For a case where X is in a victim repo you cannot make arbitrary writes > to, but you _can_ make the victim repo aware of other objects (e.g., by > opening a pull request that creates a ref), then you can iteratively > provide many Y's, improving your guess about X in each iteration. > > For a case where the victim repo has fully shared storage (GitHub, and > probably other hosts; I'm not sure if it's available yet, but GitLab is > clearly working on shared-storage too), you can probably skip all that > and just push a ref pointing to X with an empty pack (Git just cares > that it has all of the objects afterwards, not that you pushed them). > > None of those care about the quality of the hash (they do assume you > know the hash of X already, but then so does fetching by object id). > > And no, I've never written a proof-of-concept for that. It would depend > largely on the data you're trying to extract. E.g., if you think X > contains "root:XXXXXX", then you might hope to ask for "root:AXXXXX", > then "root:BXXXXX", etc. You know you've got a hit when the delta gets > smaller. So the complexity for guessing N bytes becomes 256*N, rather > than 256^N. > >> > But I agree that people do assume it's the case. I was certainly >> > surprised by the v2 behavior, and I don't remember that aspect being >> > discussed. >> >> IMHO it's a plain bug (either in implementation or documentation). > > Or both. :) The behavior and the documentation seem to agree. > >> [...] >> >> 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. >> > >> > No real argument from me. I have always thought those security >> > guarantees were BS. >> >> This would make per-branch ACLs (as implemented both by Gerrit and >> gitolite) an essentially useless feature, so please no. > > I think Ævar's argument is that those are providing a false sense of > security now (at least for read permissions). > > Let me clarify my position a little. > > I won't claim the existing scheme provides _no_ value (especially the > pure read-only case above is less dicey than the others). It's mostly > that the protections are very hand-wavy. I don't trust them _now_, and I > have little faith that other innocent-looking changes to the object > negotiation and the packing code will not significantly weaken them. > There's no security boundary expressed within Git's code, so there's a > very high chance of information leaking accidentally. A trustable system > would have boundaries built in from the ground up. > > Enough people seem to believe otherwise (i.e., that the hand-waving > arguments are worth _something_) that I've never pushed to actually > change the default behavior. But if Ævar wants to try to do so, I'm not > going to stand in my way (hence my "no argument from me"). FWIW I don't really care about this, I don't rely on uploadpack.allow{Tip,Reachable,Any}SHA1InWant=false I'm just on the side-quest of: 1. Try protocol v2 2. Discover that v2 implictly has uploadpack.allowAnySHA1InWant=true enabled 3. Some people (including Jonathan) can reasonable read our docs / seem to have understood this to be a security mechanism 4. What are we going to do about that v1 & v2 discrepancy? [You are here!] The genreal ways I see forward from that are: A) Say that v2 has a security issue and that this is a feature that works in some circumstances, but given Jeff's explanation here we should at least improve our "SECURITY" docs to be less handwaivy. B) Improve security docs, turn uploadpack.allowAnySHA1InWant=true on by default, allow people to turn it off. C) Like B) but deprecate uploadpack.allow{Tip,Reachable,Any}SHA1InWant=false. This is my patch upthread D-Z) ??? I'm not set on C), and yeah it's probably overzelous to just rip the thing out, but then what should we do? >> I would be all for changing the default, but making turning off >> allowReachableSHA1InWant an unsupported deprecated thing is a step too >> far, in my opinion. > > Yes, I agree if we were to go down this road, it probably makes sense to > flip the knobs and let them be "unflipped" if the user wants. > >> Is there somewhere that we can document these kinds of invariants or >> goals so that we don't have to keep repeating the same discussions? > > It's not clear to me that there's consensus on the invariants or goals. > ;) > > -Peff
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > The genreal ways I see forward from that are: > > A) Say that v2 has a security issue and that this is a feature that > works in some circumstances, but given Jeff's explanation here we > should at least improve our "SECURITY" docs to be less handwaivy. > > B) Improve security docs, turn uploadpack.allowAnySHA1InWant=true on by > default, allow people to turn it off. > > C) Like B) but deprecate > uploadpack.allow{Tip,Reachable,Any}SHA1InWant=false. This is my > patch upthread > > D-Z) ??? > > > I'm not set on C), and yeah it's probably overzelous to just rip the > thing out, but then what should we do? Hmph. The other overzealous thing you could do is to strenthen A and "fix" the security issue in v2? Which letter comes before A in the alphabet? ;-)
On Wed, Dec 26 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> The genreal ways I see forward from that are: >> >> A) Say that v2 has a security issue and that this is a feature that >> works in some circumstances, but given Jeff's explanation here we >> should at least improve our "SECURITY" docs to be less handwaivy. >> >> B) Improve security docs, turn uploadpack.allowAnySHA1InWant=true on by >> default, allow people to turn it off. >> >> C) Like B) but deprecate >> uploadpack.allow{Tip,Reachable,Any}SHA1InWant=false. This is my >> patch upthread >> >> D-Z) ??? >> >> >> I'm not set on C), and yeah it's probably overzelous to just rip the >> thing out, but then what should we do? > > Hmph. The other overzealous thing you could do is to strenthen A > and "fix" the security issue in v2? Which letter comes before A in > the alphabet? ;-) Sure, but that being useful is predicated on this supposed security mechanism being useful and not just security-through-obscurity, as noted in side-threads I don't think we have a convincing argument either way (and the one we do have is more on the "it's not secure" side). Of course we had that with v1 all along, but now that v2 is in released versions and in this insecure mode, we have a reason to closely look at whether we need to be issuing security releases, or doubling down on the "SECURITY" wording in git-fetch and then not carrying the mode forward.
Hi, Ævar Arnfjörð Bjarmason wrote: > On Wed, Dec 26 2018, Junio C Hamano wrote: >> Hmph. The other overzealous thing you could do is to strenthen A >> and "fix" the security issue in v2? Which letter comes before A in >> the alphabet? ;-) Yes, agreed. This is what I was hinting at in [1] with "it's a plain bug". > Sure, but that being useful is predicated on this supposed security > mechanism being useful and not just security-through-obscurity, as noted > in side-threads I don't think we have a convincing argument either way > (and the one we do have is more on the "it's not secure" side). > > Of course we had that with v1 all along, but now that v2 is in released > versions and in this insecure mode, we have a reason to closely look at > whether we need to be issuing security releases, or doubling down on the > "SECURITY" wording in git-fetch and then not carrying the mode forward. Just for the record, as I've already said, I would be strongly against removing this feature. I know of multiple populations that make use of it, and removing it would not serve them well. Changing defaults and documentation is a separate story. Sincerely, Jonathan [1] https://public-inbox.org/git/20181217231452.GA13835@google.com/
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 && test_i18ngrep "Server does not allow request for unadvertised object" err ' @@ -788,7 +791,7 @@ test_expect_success 'shallow clone exclude tag two' ' ' test_expect_success 'fetch exclude tag one' ' - git -C shallow12 fetch --shallow-exclude one origin && + GIT_TEST_PROTOCOL_VERSION= git -C shallow12 fetch --shallow-exclude one origin && git -C shallow12 log --pretty=tformat:%s origin/master >actual && test_write_lines three two >expected && test_cmp expected actual @@ -806,7 +809,7 @@ test_expect_success 'fetching deepen' ' git -C deepen log --pretty=tformat:%s master >actual && echo three >expected && test_cmp expected actual && - git -C deepen fetch --deepen=1 && + GIT_TEST_PROTOCOL_VERSION= git -C deepen fetch --deepen=1 && git -C deepen log --pretty=tformat:%s origin/master >actual && cat >expected <<-\EOF && four diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh index 4ca48f0276..0e90a90294 100755 --- a/t/t5503-tagfollow.sh +++ b/t/t5503-tagfollow.sh @@ -56,7 +56,7 @@ test_expect_success 'fetch A (new commit : 1 connection)' ' rm -f $U && ( cd cloned && - GIT_TRACE_PACKET=$UPATH git fetch && + GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch && test $A = $(git rev-parse --verify origin/master) ) && get_needs $U >actual && @@ -86,7 +86,7 @@ test_expect_success 'fetch C, T (new branch, tag : 1 connection)' ' rm -f $U && ( cd cloned && - GIT_TRACE_PACKET=$UPATH git fetch && + GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch && test $C = $(git rev-parse --verify origin/cat) && test $T = $(git rev-parse --verify tag1) && test $A = $(git rev-parse --verify tag1^0) @@ -122,7 +122,7 @@ test_expect_success 'fetch B, S (commit and tag : 1 connection)' ' rm -f $U && ( cd cloned && - GIT_TRACE_PACKET=$UPATH git fetch && + GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch && test $B = $(git rev-parse --verify origin/master) && test $B = $(git rev-parse --verify tag2^0) && test $S = $(git rev-parse --verify tag2) @@ -146,7 +146,7 @@ test_expect_success 'new clone fetch master and tags' ' cd clone2 && git init && git remote add origin .. && - GIT_TRACE_PACKET=$UPATH git fetch && + GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch && test $B = $(git rev-parse --verify origin/master) && test $S = $(git rev-parse --verify tag2) && test $B = $(git rev-parse --verify tag2^0) && diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh index ca69636fd5..7b480587c9 100755 --- a/t/t5512-ls-remote.sh +++ b/t/t5512-ls-remote.sh @@ -223,7 +223,7 @@ test_expect_success 'ls-remote --symref' ' $(git rev-parse refs/tags/mark1.10) refs/tags/mark1.10 $(git rev-parse refs/tags/mark1.2) refs/tags/mark1.2 EOF - git ls-remote --symref >actual && + GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref >actual && test_cmp expect actual ' @@ -243,7 +243,7 @@ test_expect_failure 'ls-remote with filtered symref (--heads)' ' 1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/foo 1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/master EOF - git ls-remote --symref --heads . >actual && + GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref --heads . >actual && test_cmp expect actual ' @@ -252,9 +252,9 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' ' 1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/foo 1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/master EOF - git ls-remote --symref --heads . >actual && + GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref --heads . >actual && test_cmp expect actual && - git ls-remote --symref . "refs/heads/*" >actual && + GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref . "refs/heads/*" >actual && test_cmp expect actual ' diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh index 36b0dbc01c..f095555c3e 100755 --- a/t/t5515-fetch-merge-logic.sh +++ b/t/t5515-fetch-merge-logic.sh @@ -147,7 +147,7 @@ do do git update-ref -d "$refname" "$val" done - git fetch "$@" >/dev/null + GIT_TEST_PROTOCOL_VERSION= git fetch "$@" >/dev/null cat .git/FETCH_HEAD } >"$actual_f" && git show-ref >"$actual_r" && diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 08cdee0b95..1d1b717cd5 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1129,7 +1129,7 @@ do ' done -test_expect_success 'fetch exact SHA1' ' +test_expect_success 'fetch exact SHA1 in protocol v0' ' mk_test testrepo heads/master hidden/one && git push testrepo master:refs/hidden/one && ( @@ -1148,7 +1148,8 @@ test_expect_success 'fetch exact SHA1' ' test_must_fail git cat-file -t $the_commit && # fetching the hidden object should fail by default - test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err && + test_must_fail env GIT_TEST_PROTOCOL_VERSION= \ + git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err && test_i18ngrep "Server does not allow request for unadvertised object" err && test_must_fail git rev-parse --verify refs/heads/copy && @@ -1205,7 +1206,8 @@ do mk_empty shallow && ( cd shallow && - test_must_fail git fetch --depth=1 ../testrepo/.git $SHA1 && + test_must_fail env GIT_TEST_PROTOCOL_VERSION= \ + git fetch --depth=1 ../testrepo/.git $SHA1 && git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true && git fetch --depth=1 ../testrepo/.git $SHA1 && git cat-file commit $SHA1 @@ -1233,15 +1235,18 @@ do mk_empty shallow && ( cd shallow && - test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 && - test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_1 && + test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \ + git fetch ../testrepo/.git $SHA1_3 && + test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \ + git fetch ../testrepo/.git $SHA1_1 && git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true && git fetch ../testrepo/.git $SHA1_1 && git cat-file commit $SHA1_1 && test_must_fail git cat-file commit $SHA1_2 && git fetch ../testrepo/.git $SHA1_2 && git cat-file commit $SHA1_2 && - test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 + test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \ + git fetch ../testrepo/.git $SHA1_3 ) ' done diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index 6faf17e17a..85b3022ce6 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -127,7 +127,8 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' ' git init notshallow && ( cd notshallow && - git fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/*&& + GIT_TEST_PROTOCOL_VERSION= \ + git fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/* && git for-each-ref --format="%(refname)" >actual.refs && cat <<EOF >expect.refs && refs/remotes/shallow/no-shallow diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh index 5fbf67c446..a121e19bc7 100755 --- a/t/t5539-fetch-http-shallow.sh +++ b/t/t5539-fetch-http-shallow.sh @@ -67,7 +67,8 @@ test_expect_success 'no shallow lines after receiving ACK ready' ' cd clone && git checkout --orphan newnew && test_commit new-too && - GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" git fetch --depth=2 && + GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" GIT_TEST_PROTOCOL_VERSION= \ + git fetch --depth=2 && grep "fetch-pack< ACK .* ready" ../trace && ! grep "fetch-pack> done" ../trace ) @@ -114,7 +115,7 @@ test_expect_success 'shallow clone exclude tag two' ' ' test_expect_success 'fetch exclude tag one' ' - git -C shallow12 fetch --shallow-exclude one origin && + GIT_TEST_PROTOCOL_VERSION= git -C shallow12 fetch --shallow-exclude one origin && git -C shallow12 log --pretty=tformat:%s origin/master >actual && test_write_lines three two >expected && test_cmp expected actual @@ -128,14 +129,14 @@ test_expect_success 'fetching deepen' ' test_commit two && test_commit three && mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" && - git clone --depth 1 $HTTPD_URL/smart/shallow-deepen.git deepen && + GIT_TEST_PROTOCOL_VERSION= git clone --depth 1 $HTTPD_URL/smart/shallow-deepen.git deepen && mv "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" .git && test_commit four && git -C deepen log --pretty=tformat:%s master >actual && echo three >expected && test_cmp expected actual && mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" && - git -C deepen fetch --deepen=1 && + GIT_TEST_PROTOCOL_VERSION= git -C deepen fetch --deepen=1 && git -C deepen log --pretty=tformat:%s origin/master >actual && cat >expected <<-\EOF && four diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh index 5475afc052..180c9005b7 100755 --- a/t/t5541-http-push-smart.sh +++ b/t/t5541-http-push-smart.sh @@ -45,14 +45,19 @@ test_expect_success 'no empty path components' ' # In the URL, add a trailing slash, and see if git appends yet another # slash. cd "$ROOT_PATH" && - git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone && + # Other protocol versions may make different requests, so perform this + # clone with the default protocol. + GIT_TEST_PROTOCOL_VERSION= git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone && check_access_log exp ' test_expect_success 'clone remote repository' ' rm -rf test_repo_clone && - git clone $HTTPD_URL/smart/test_repo.git test_repo_clone && + # Other protocol versions may make different requests, so perform this + # clone with the default protocol. + GIT_TEST_PROTOCOL_VERSION= git clone $HTTPD_URL/smart/test_repo.git test_repo_clone && + ( cd test_repo_clone && git config push.default matching ) diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 8630b0cc39..a51993f35a 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -43,7 +43,8 @@ test_expect_success 'clone http repository' ' < Cache-Control: no-cache, max-age=0, must-revalidate < Content-Type: application/x-git-upload-pack-result EOF - GIT_TRACE_CURL=true git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err && + GIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION= \ + git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err && test_cmp file clone/file && tr '\''\015'\'' Q <err | sed -e " @@ -92,7 +93,7 @@ test_expect_success 'fetch changes via http' ' echo content >>file && git commit -a -m two && git push public && - (cd clone && git pull) && + (cd clone && GIT_TEST_PROTOCOL_VERSION= git pull) && test_cmp file clone/file ' @@ -143,7 +144,7 @@ test_expect_success 'clone from auth-only-for-push repository' ' test_expect_success 'clone from auth-only-for-objects repository' ' echo two >expect && set_askpass user@host pass@host && - git clone --bare "$HTTPD_URL/auth-fetch/smart/repo.git" half-auth && + GIT_TEST_PROTOCOL_VERSION= git clone --bare "$HTTPD_URL/auth-fetch/smart/repo.git" half-auth && expect_askpass both user@host && git --git-dir=half-auth log -1 --format=%s >actual && test_cmp expect actual @@ -151,7 +152,7 @@ test_expect_success 'clone from auth-only-for-objects repository' ' test_expect_success 'no-op half-auth fetch does not require a password' ' set_askpass wrong && - git --git-dir=half-auth fetch && + GIT_TEST_PROTOCOL_VERSION= git --git-dir=half-auth fetch && expect_askpass none ' @@ -187,7 +188,7 @@ test_expect_success 'create namespaced refs' ' ' test_expect_success 'smart clone respects namespace' ' - git clone "$HTTPD_URL/smart_namespace/repo.git" ns-smart && + GIT_TEST_PROTOCOL_VERSION= git clone "$HTTPD_URL/smart_namespace/repo.git" ns-smart && echo namespaced >expect && git --git-dir=ns-smart/.git log -1 --format=%s >actual && test_cmp expect actual @@ -214,7 +215,7 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set EOF git config http.cookiefile cookies.txt && git config http.savecookies true && - git ls-remote $HTTPD_URL/smart_cookies/repo.git master && + GIT_TEST_PROTOCOL_VERSION= git ls-remote $HTTPD_URL/smart_cookies/repo.git master && tail -3 cookies.txt | sort >cookies_tail.txt && test_cmp expect_cookies.txt cookies_tail.txt ' @@ -306,7 +307,8 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' ' git init --bare test_reachable.git && git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" && - test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" + test_must_fail env GIT_TEST_PROTOCOL_VERSION= \ + git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" ' test_expect_success 'test allowanysha1inwant with unreachable' ' @@ -325,7 +327,8 @@ test_expect_success 'test allowanysha1inwant with unreachable' ' git init --bare test_reachable.git && git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" && - test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" && + test_must_fail env GIT_TEST_PROTOCOL_VERSION= \ + git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" && git -C "$server" config uploadpack.allowanysha1inwant 1 && git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh index 30857b84a8..979e6583d4 100755 --- a/t/t5552-skipping-fetch-negotiator.sh +++ b/t/t5552-skipping-fetch-negotiator.sh @@ -127,7 +127,7 @@ test_expect_success 'use ref advertisement to filter out commits' ' # not need to send any ancestors of "c3", but we still need to send "c3" # itself. test_config -C client fetch.negotiationalgorithm skipping && - trace_fetch client origin to_fetch && + GIT_TEST_PROTOCOL_VERSION= trace_fetch client origin to_fetch && have_sent c5 c4^ c2side && have_not_sent c4 c4^^ c4^^^ ' @@ -189,7 +189,7 @@ test_expect_success 'do not send "have" with ancestors of commits that server AC test_commit -C server commit-on-b1 && test_config -C client fetch.negotiationalgorithm skipping && - trace_fetch client "$(pwd)/server" to_fetch && + GIT_TEST_PROTOCOL_VERSION= trace_fetch client "$(pwd)/server" to_fetch && grep " fetch" trace && # fetch-pack sends 2 requests each containing 16 "have" lines before diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh index 7466aad111..c42ee245cd 100755 --- a/t/t5570-git-daemon.sh +++ b/t/t5570-git-daemon.sh @@ -190,7 +190,7 @@ test_expect_success 'daemon log records all attributes' ' EOF >daemon.log && GIT_OVERRIDE_VIRTUAL_HOST=localhost \ - git -c protocol.version=1 \ + GIT_TEST_PROTOCOL_VERSION= git -c protocol.version=1 \ ls-remote "$GIT_DAEMON_URL/interp.git" && grep -i extended.attribute daemon.log | cut -d" " -f2- >actual && test_cmp expect actual diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index e87164aa8f..a555e38845 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -943,7 +943,8 @@ test_expect_success 'submodule update clone shallow submodule outside of depth' cd super3 && sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp && mv -f .gitmodules.tmp .gitmodules && - test_must_fail git submodule update --init --depth=1 2>actual && + test_must_fail env GIT_TEST_PROTOCOL_VERSION= \ + git submodule update --init --depth=1 2>actual && test_i18ngrep "Direct fetching of that commit failed." actual && git -C ../submodule config uploadpack.allowReachableSHA1InWant true && git submodule update --init --depth=1 >actual &&