threads / rfc / 50797

RFC patcht5551: delete auth-for-pack-but-not-refs test

Subject: [RFC PATCH] t5551: delete auth-for-pack-but-not-refs test

## tl;dr

12 messages between Mar 21, 2019 and Apr 8, 2019. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Jonathan Tan· Mar 21, 2019, 17:47 UTC · lore

When using protocol v0, upload-pack over HTTP permits a "half-auth" configuration in which, at the web server layer, the info/refs path is not protected by authentication but the git-upload-pack path is, so that a user can perform fetches that do not download any objects without authentication, but still needs authentication to download objects.

2e736fd5e9 ("remote-curl: retry failed requests for auth even with gzip", 2012-10-31) added a test for this, stating that this leaks information about the repository but makes it occasionally more convenient for users that use manual credential entry.

Protocol v2 does not support this, because both ref and pack are obtained from the git-upload-pack path.

Because this configuration is not supported by all protocol versions, and because this configuration seems to be of limited usefulness (only useful for people who use manual credential entry and on servers that are OK with exposing refs but not objects, and even in this case, helps only in a no-op fetch), delete the test that verifies that this configuration works.

This issue was discovered by the GIT_TEST_PROTOCOL_VERSION patches.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
This is on master. (It is reasonable to apply it even in the absence of
GIT_TEST_PROTOCOL_VERSION.)

Marking this as RFC, because my opinion is that the usefulness of this feature is limited (as you can see in the commit message), but others may have a use case that I haven't thought about.

The alternative is to add GIT_TEST_PROTOCOL_VERSION=0 to this test (and
rebase it to a branch that has GIT_TEST_PROTOCOL_VERSION).
---
 t/t5551-http-fetch-smart.sh | 6 ------
 1 file changed, 6 deletions(-)

Show changes to t/t5551-http-fetch-smart.sh +0 −6

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index ba83e567e5..3be9c0063b 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -149,12 +149,6 @@ test_expect_success 'clone from auth-only-for-objects repository' '
 	test_cmp expect actual
 '
 
-test_expect_success 'no-op half-auth fetch does not require a password' '
-	set_askpass wrong &&
-	git --git-dir=half-auth fetch &&
-	expect_askpass none
-'
-
 test_expect_success 'redirects send auth to new location' '
 	set_askpass user@host pass@host &&
 	git -c credential.useHttpPath=true \

-- 
2.21.0.155.ge902e9bcae.dirty

Jeff King· Mar 21, 2019, 19:55 UTC · re: Jonathan Tan · lore

Re: [RFC PATCH] t5551: delete auth-for-pack-but-not-refs test

On Thu, Mar 21, 2019 at 10:47:19AM -0700, Jonathan Tan wrote:

Show 13 quoted lines

> When using protocol v0, upload-pack over HTTP permits a "half-auth"
> configuration in which, at the web server layer, the info/refs path is
> not protected by authentication but the git-upload-pack path is, so that
> a user can perform fetches that do not download any objects without
> authentication, but still needs authentication to download objects.
> 
> 2e736fd5e9 ("remote-curl: retry failed requests for auth even with
> gzip", 2012-10-31) added a test for this, stating that this leaks
> information about the repository but makes it occasionally more
> convenient for users that use manual credential entry.
> 
> Protocol v2 does not support this, because both ref and pack are
> obtained from the git-upload-pack path.

I have mixed feelings. I agree that this this is not a setup we really want to recommend. But it did come out of somebody's real-world case[1]. It would be nice to know if it got broken, even if v2 doesn't support it.

I am a little confused about v2 here, though. It should hit the initial info/refs endpoint the same as usual. If it's a noop fetch, then it's done. Otherwise, we'd hit the git-upload-pack and expect to require authentication. That should work after your switch to using post_rpc, shouldn't it?

And I guess it does, because you did not delete the test before "clone from auth-only-for-objects repository", which would actually do the second half of that conversation, and require authentication. You're only deleting the part that does the noop fetch.

Puzzled...

-Peff

[1] https://public-inbox.org/git/CAHtLG6Q+XO=LhnKw4hhwtOe2ROeDN1Kg=JN5GTQqdvYjk-Sv4g@mail.gmail.com/

Jeff King· Mar 21, 2019, 20:02 UTC · re: Jeff King · lore

Re: [RFC PATCH] t5551: delete auth-for-pack-but-not-refs test

On Thu, Mar 21, 2019 at 03:55:37PM -0400, Jeff King wrote:

Show 12 quoted lines

> I am a little confused about v2 here, though. It should hit the initial
> info/refs endpoint the same as usual. If it's a noop fetch, then it's
> done. Otherwise, we'd hit the git-upload-pack and expect to require
> authentication. That should work after your switch to using post_rpc,
> shouldn't it?
> 
> And I guess it does, because you did not delete the test before "clone
> from auth-only-for-objects repository", which would actually do the
> second half of that conversation, and require authentication. You're
> only deleting the part that does the noop fetch.
> 
> Puzzled...

Ah, nevermind. I forgot that v2 really will make two requests for this noop case. The initial endpoint hit only returns the capabilities and nothing else. And that's inherent in the protocol.

I'm OK with dropping this test, but I think my reasoning is slightly different than what you wrote. The interesting bits are actually happening in the test _before_ this one, which are exercising the gzipped auth-retry code. And that happens in both protocol v0 and v2.

The test you're deleting is basically just verifying that our apache config is indeed "half-auth". Because in v0, the server is never even going to ask for credentials, so no interesting code paths in the client are triggered. So it's not actually testing anything of interest.

-Peff

Jonathan Tan· Mar 21, 2019, 20:24 UTC · re: Jeff King · lore

Re: [RFC PATCH] t5551: delete auth-for-pack-but-not-refs test

Show 18 quoted lines

> On Thu, Mar 21, 2019 at 03:55:37PM -0400, Jeff King wrote:
> 
> > I am a little confused about v2 here, though. It should hit the initial
> > info/refs endpoint the same as usual. If it's a noop fetch, then it's
> > done. Otherwise, we'd hit the git-upload-pack and expect to require
> > authentication. That should work after your switch to using post_rpc,
> > shouldn't it?
> > 
> > And I guess it does, because you did not delete the test before "clone
> > from auth-only-for-objects repository", which would actually do the
> > second half of that conversation, and require authentication. You're
> > only deleting the part that does the noop fetch.
> > 
> > Puzzled...
> 
> Ah, nevermind. I forgot that v2 really will make two requests for this
> noop case. The initial endpoint hit only returns the capabilities and
> nothing else. And that's inherent in the protocol.

Yes, you're right.

Show 9 quoted lines

> I'm OK with dropping this test, but I think my reasoning is slightly
> different than what you wrote. The interesting bits are actually
> happening in the test _before_ this one, which are exercising the
> gzipped auth-retry code. And that happens in both protocol v0 and v2.
> 
> The test you're deleting is basically just verifying that our apache
> config is indeed "half-auth". Because in v0, the server is never even
> going to ask for credentials, so no interesting code paths in the client
> are triggered. So it's not actually testing anything of interest.

If both of us want to drop this test, that's great :-) but for clarification: in addition to verifying that our apache config is "half-auth", this test also verifies that in a no-op fetch, we don't hit the path that is guarded by an authentication requirement. This seems significant to me in light of the link you provided in your prior email [1].

[1] https://public-inbox.org/git/20190321195536.GC19427@sigill.intra.peff.net/

Jeff King· Mar 21, 2019, 21:48 UTC · re: Jonathan Tan · lore

Re: [RFC PATCH] t5551: delete auth-for-pack-but-not-refs test

On Thu, Mar 21, 2019 at 01:24:35PM -0700, Jonathan Tan wrote:

Show 11 quoted lines

> > The test you're deleting is basically just verifying that our apache
> > config is indeed "half-auth". Because in v0, the server is never even
> > going to ask for credentials, so no interesting code paths in the client
> > are triggered. So it's not actually testing anything of interest.
> 
> If both of us want to drop this test, that's great :-) but for
> clarification: in addition to verifying that our apache config is
> "half-auth", this test also verifies that in a no-op fetch, we don't hit
> the path that is guarded by an authentication requirement. This seems
> significant to me in light of the link you provided in your prior email
> [1].

Yeah, I suppose it does. I just never really thought of that as a plausible regression to introduce, given the way the v0 protocol works. :)

Although in a sense it is interesting, because it did reveal something about v2 that we hadn't considered. I don't think it's worth addressing (especially now), but had we been doing cross-protocol tests sooner, we might have looked at it more in the design phase.

So I would also be OK with just marking it as as v0-only test.

-Peff

Jonathan Tan· Mar 21, 2019, 22:36 UTC · re: Jeff King · lore

Re: [RFC PATCH] t5551: delete auth-for-pack-but-not-refs test

Show 23 quoted lines

> On Thu, Mar 21, 2019 at 01:24:35PM -0700, Jonathan Tan wrote:
> 
> > > The test you're deleting is basically just verifying that our apache
> > > config is indeed "half-auth". Because in v0, the server is never even
> > > going to ask for credentials, so no interesting code paths in the client
> > > are triggered. So it's not actually testing anything of interest.
> > 
> > If both of us want to drop this test, that's great :-) but for
> > clarification: in addition to verifying that our apache config is
> > "half-auth", this test also verifies that in a no-op fetch, we don't hit
> > the path that is guarded by an authentication requirement. This seems
> > significant to me in light of the link you provided in your prior email
> > [1].
> 
> Yeah, I suppose it does. I just never really thought of that as a
> plausible regression to introduce, given the way the v0 protocol works. :)
> 
> Although in a sense it is interesting, because it did reveal something
> about v2 that we hadn't considered. I don't think it's worth addressing
> (especially now), but had we been doing cross-protocol tests sooner, we
> might have looked at it more in the design phase.
> 
> So I would also be OK with just marking it as as v0-only test.

OK - I'll wait to see what others think. Thanks for looking at this so far.

Junio C Hamano· Mar 22, 2019, 02:20 UTC · re: Jonathan Tan · lore

Re: [RFC PATCH] t5551: delete auth-for-pack-but-not-refs test

Jonathan Tan <jonathantanmy@google.com> writes:

Show 6 quoted lines

> Because this configuration is not supported by all protocol versions,
> and because this configuration seems to be of limited usefulness (only
> useful for people who use manual credential entry and on servers that
> are OK with exposing refs but not objects, and even in this case, helps
> only in a no-op fetch), delete the test that verifies that this
> configuration works.

A possible and different conclusion that naturally follow your first "because" could be "fix protocol versions whose support of this configuration is broken", and your second "because" is with "seems to be", that makes it quite weak.

Quite honestly, I hate to see a proposed log message that downplays its potential negative effects without sufficient justification.

Isn't this feature primarily for those who want to poll from an automated job (and naturally you want to assign as little privilege as possible to such an automated job) with ls-remote and only run an authenticated fetch, perhaps manually, with or without cred helper, when the automated polling job finds there is something worthwhile to fetch? What this test is checking seems to be a quite effective way to achieve that useful workflow, at least to me, and offhand I do not think of other ways to easily achieve the same.

The "ls-remote" communication may not even touch any outside network but may be happening all within a single organization, in which case "are OK with exposing refs" is making a security mountain out of a molehill. If it were a truly problematic hole that makes it a security issue, wouldn't we deleting this test and at the same time plugging the hole for earlier protocol versions?

Having said all that, I do not care too deeply. Would a much better longer-term solution for those who want to poll and fetch only when there is something new be to allow clients to subscribe to a feed that hangs while there is nothing new, and lets the upstream side continuously feed incremental updates to the receiving client, as its refs are updated, or something? As long as such a thing is in our vision (it is OK if nobody is currently working on it) to become replacement, I do not think it is a huge loss to lose the ability for unauthenticated ls-remote with authenticated fetch.

Thanks.

Jonathan Tan· Mar 22, 2019, 17:20 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH] t5551: delete auth-for-pack-but-not-refs test

Show 16 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > Because this configuration is not supported by all protocol versions,
> > and because this configuration seems to be of limited usefulness (only
> > useful for people who use manual credential entry and on servers that
> > are OK with exposing refs but not objects, and even in this case, helps
> > only in a no-op fetch), delete the test that verifies that this
> > configuration works.
> 
> A possible and different conclusion that naturally follow your first
> "because" could be "fix protocol versions whose support of this
> configuration is broken", and your second "because" is with "seems
> to be", that makes it quite weak.
> 
> Quite honestly, I hate to see a proposed log message that downplays
> its potential negative effects without sufficient justification.

That is true, and that's fair.

Show 15 quoted lines

> Isn't this feature primarily for those who want to poll from an
> automated job (and naturally you want to assign as little privilege
> as possible to such an automated job) with ls-remote and only run an
> authenticated fetch, perhaps manually, with or without cred helper,
> when the automated polling job finds there is something worthwhile
> to fetch?  What this test is checking seems to be a quite effective
> way to achieve that useful workflow, at least to me, and offhand I
> do not think of other ways to easily achieve the same.
> 
> The "ls-remote" communication may not even touch any outside network
> but may be happening all within a single organization, in which case
> "are OK with exposing refs" is making a security mountain out of a
> molehill.  If it were a truly problematic hole that makes it a
> security issue, wouldn't we deleting this test and at the same time
> plugging the hole for earlier protocol versions?

Thanks - that's a reasonable use case.

Show 9 quoted lines

> Having said all that, I do not care too deeply.  Would a much better
> longer-term solution for those who want to poll and fetch only when
> there is something new be to allow clients to subscribe to a feed
> that hangs while there is nothing new, and lets the upstream side
> continuously feed incremental updates to the receiving client, as
> its refs are updated, or something?  As long as such a thing is in
> our vision (it is OK if nobody is currently working on it) to become
> replacement, I do not think it is a huge loss to lose the ability for
> unauthenticated ls-remote with authenticated fetch.

I just remembered that one way we can support the existing use case is to inline the ls-refs call that we make as an Extra Parameter [1]. This would restore the ability to obtain refs through only the info/refs path.

Perhaps this is the component in our vision that we need - I'll write another patch that merely forces GIT_TEST_PROTOCOL_VERSION=0 and has a NEEDSWORK comment that explains this.

[1] https://github.com/git/git/blob/master/Documentation/technical/pack-protocol.txt

Jonathan Tan· Mar 22, 2019, 19:01 UTC · re: Jonathan Tan · lore

[PATCH v2] t5551: mark half-auth no-op fetch test as v0-only

But protocol v2 does not support this, because both ref and pack are obtained from the git-upload-pack path.

Mark the test verifying this behavior as protocol v0-only, with a description of what needs to be done to make v2 support this.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
This is based on master + jt/test-protocol-version, but should apply
cleanly to many places.

After reading Peff's and Junio's replies, it seems that it would be
better to retain this test. So here's a version that retains the test
but just marks it as v0, with an explanation as to what needs to be done
to make v2 be compatible with it.
---
 t/t5551-http-fetch-smart.sh | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Show changes to t/t5551-http-fetch-smart.sh +11 −1

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index a685d3edb6..c760514716 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -164,7 +164,17 @@ 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 &&
+
+	# NEEDSWORK: When using HTTP(S), protocol v0 supports a "half-auth"
+	# configuration with authentication required only when downloading
+	# objects and not refs, by having the HTTP server only require
+	# authentication for the "git-upload-pack" path and not "info/refs".
+	# This is not possible with protocol v2, since both objects and refs
+	# are obtained from the "git-upload-pack" path. A solution to this is
+	# to teach the server and client to be able to inline ls-refs requests
+	# as an Extra Parameter (see pack-protocol.txt), so that "info/refs"
+	# can serve refs, just like it does in protocol v0.
+	GIT_TEST_PROTOCOL_VERSION=0 git --git-dir=half-auth fetch &&
 	expect_askpass none
 '

-- 
2.21.0.155.ge902e9bcae.dirty

Jeff King· Mar 23, 2019, 07:05 UTC · re: Jonathan Tan · lore

Re: [PATCH v2] t5551: mark half-auth no-op fetch test as v0-only

On Fri, Mar 22, 2019 at 12:01:39PM -0700, Jonathan Tan wrote:

Show 11 quoted lines

> When using protocol v0, upload-pack over HTTP permits a "half-auth"
> configuration in which, at the web server layer, the info/refs path is
> not protected by authentication but the git-upload-pack path is, so that
> a user can perform fetches that do not download any objects without
> authentication, but still needs authentication to download objects.
> 
> But protocol v2 does not support this, because both ref and pack are
> obtained from the git-upload-pack path.
> 
> Mark the test verifying this behavior as protocol v0-only, with a
> description of what needs to be done to make v2 support this.

Thanks, this looks like a fine solution to me.

I do not really expect anyone will ever get around to this NEEDSWORK, but we can see. :)

-Peff

Jonathan Nieder· Apr 6, 2019, 11:31 UTC · re: Jonathan Tan · lore

Re: [PATCH v2] t5551: mark half-auth no-op fetch test as v0-only

Hi,

Jonathan Tan wrote:

Show 13 quoted lines

> When using protocol v0, upload-pack over HTTP permits a "half-auth"
> configuration in which, at the web server layer, the info/refs path is
> not protected by authentication but the git-upload-pack path is, so that
> a user can perform fetches that do not download any objects without
> authentication, but still needs authentication to download objects.
>
> But protocol v2 does not support this, because both ref and pack are
> obtained from the git-upload-pack path.
>
> Mark the test verifying this behavior as protocol v0-only, with a
> description of what needs to be done to make v2 support this.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>

Thanks for the analysis.  Makes sense.

Show 19 quoted lines

> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -164,7 +164,17 @@ 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 &&
> +
> +	# NEEDSWORK: When using HTTP(S), protocol v0 supports a "half-auth"
> +	# configuration with authentication required only when downloading
> +	# objects and not refs, by having the HTTP server only require
> +	# authentication for the "git-upload-pack" path and not "info/refs".
> +	# This is not possible with protocol v2, since both objects and refs
> +	# are obtained from the "git-upload-pack" path. A solution to this is
> +	# to teach the server and client to be able to inline ls-refs requests
> +	# as an Extra Parameter (see pack-protocol.txt), so that "info/refs"
> +	# can serve refs, just like it does in protocol v0.
> +	GIT_TEST_PROTOCOL_VERSION=0 git --git-dir=half-auth fetch &&
>  	expect_askpass none

I suspect it's fine if protocol v2 never supports this. Can we change the NEEDSWORK comment to say that the protocol v2 spec should document the lack of support for half-auth?

With or without such a change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Jonathan Tan· Apr 8, 2019, 17:01 UTC · re: Jonathan Nieder · lore

Re: [PATCH v2] t5551: mark half-auth no-op fetch test as v0-only

Show 18 quoted lines

> > +	# NEEDSWORK: When using HTTP(S), protocol v0 supports a "half-auth"
> > +	# configuration with authentication required only when downloading
> > +	# objects and not refs, by having the HTTP server only require
> > +	# authentication for the "git-upload-pack" path and not "info/refs".
> > +	# This is not possible with protocol v2, since both objects and refs
> > +	# are obtained from the "git-upload-pack" path. A solution to this is
> > +	# to teach the server and client to be able to inline ls-refs requests
> > +	# as an Extra Parameter (see pack-protocol.txt), so that "info/refs"
> > +	# can serve refs, just like it does in protocol v0.
> > +	GIT_TEST_PROTOCOL_VERSION=0 git --git-dir=half-auth fetch &&
> >  	expect_askpass none
> 
> I suspect it's fine if protocol v2 never supports this.  Can we change
> the NEEDSWORK comment to say that the protocol v2 spec should document
> the lack of support for half-auth?
> 
> With or without such a change,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks for your review. Junio brought up a possible workflow that uses this feature [1] and while I don't know if we will ever get around to supporting half-auth in v2, it seems better to me to leave the possibility open.

[1] https://public-inbox.org/git/xmqq36nfsl8t.fsf@gitster-ct.c.googlers.com/