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

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

[Jonathan Tan]

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(-)

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

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

[Jeff King]

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

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

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

[Jeff King]

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.

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

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

[Jonathan Tan]

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

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

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

[Jeff King]

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.

-Peff

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

[Jonathan Tan]

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

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

[Junio C Hamano]

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.

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.

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

[Jonathan Tan]

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

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

> 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

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

[Jonathan Tan]

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>
---
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(-)

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

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

[Jeff King]

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

> 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

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

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

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

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

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

[Jonathan Tan]

> > +	# 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/