| Message ID | 20190321174719.151877-1-jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] t5551: delete auth-for-pack-but-not-refs test | expand |
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/
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
> 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/
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
> 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.
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.
> 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
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 \
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(-)