Thanks for reporting; I've added the author of that commit to the cc.
I think this is definitely a regression in the output we're giving, but
it also points to a bug in the behavior (we fail to forget about bad
credentials; see below).
The motivation from the commit is:
When the username and password are supplied in a url like this
https://myuser:secret@git.exampe/myrepo.git and the server supports the
negotiate authenticaten method, git does not fall back to basic auth and
libcurl hardly tries to authenticate with the negotiate method.
Stop using the Negotiate authentication method after the first failure
because if it fails on the first try it will never succeed.
So it is focused on the case when the credentials came in the URL,
before the first contact with the server (where we'd get an HTTP 401).
And the diff moves the negotiate check earlier in the function, before
we see if we already have credentials:
So in that case, we'd clear the GSSNEGOTIATE bit and return HTTP_REAUTH,
and the caller will try again. Makes sense for the use case described.
But imagine we didn't get a username/password in the URL. The first
request will return REAUTH because of this moved code path (just as it
would have before, because http.auth.{username,password} are not set).
And then we'll get a credential from the user or from a helper and try
again. But this time, if we fail, we'll return HTTP_REAUTH again! We
never hit the "if (http_auth.username && http_auth.password)" check at
all. And hence we never return HTTP_NOAUTH (which gives us the more
useful "authentication failed" message), nor the credential_reject()
line (which informs helpers to stop caching a known-bad password).
We can see it like this. First, seed the cache with a bad password (in
this case a bogus token to elicit a 401 response from GitHub). In real
life, this would be a password stored from a previous successful
attempt, but which was invalidated in the meantime. Note the empty value
for credential.helper clears the list of any regular helpers you'd use
for github.com.
echo url=https://peff:ghp_000000000000000000000000000000000000@github.com |
git -c credential.helper= \
-c credential.helper=cache \
credential approve
Now make a request that requires auth; we expect it to fail since our
credential is bad.
git -c credential.helper= \
-c credential.helper=cache \
ls-remote https://github.com/peff/foo
which yields:
remote: Invalid username or password.
fatal: unable to access 'https://github.com/peff/foo/': The requested URL returned error: 401
as expected (except for the error message). But we also expect it to
clear the bogus credential from the helper, so that if we run ls-remote
again, it will prompt us. But it doesn't! With git v2.32.0-rc0, you can
run that ls-remote as many times as you want, and it will always fail.
Whereas if you switch to 1b0d9545bb8^, on the second run it will
correctly prompt you for an updated password.
I think for this to work, we would need to figure out from libcurl's
response that GSSNEGOTIATE was the problem for our particular request,
and only trigger the bit-clearing and HTTP_REAUTH if that was true. I'm
not sure if that's possible, though.
I suspect we could hack around it by pessimistically guessing that
GSSNEGOTIATE was the problem. But I'm worried that making that work
would require up to three requests (one to find out we need auth, one to
remove the GSSNEGOTIATE bit, and one to retry with a username/password).
That seems like punishing people with servers that don't even care about
Negotiate for no reason.
So perhaps somebody can come up with something clever, but I suspect we
may need to just revert this for the v2.32 release, and re-break the
case that 1b0d9545bb8 was trying to solve.
We probably should beef up the tests around http's credential-rejection,
too, to catch this regression.
-Peff
From: Jeff King <hidden> Date: 2021-05-18 06:26:49
On Tue, May 18, 2021 at 01:50:27AM -0400, Jeff King wrote:
So perhaps somebody can come up with something clever, but I suspect we
may need to just revert this for the v2.32 release, and re-break the
case that 1b0d9545bb8 was trying to solve.
We probably should beef up the tests around http's credential-rejection,
too, to catch this regression.
Here are some patches. The first one adds the tests, and I think is an
obvious improvement.
The second one does the revert. I'd be quite happy if somebody wants to
figure out how to fix it in a way that addresses the original problem,
and then we can replace that. But in the meantime, I think it makes
sense to prepare the revert, as we wouldn't want to release v2.32.0 with
the bug.
[1/2]: t5551: test http interaction with credential helpers
[2/2]: Revert "remote-curl: fall back to basic auth if Negotiate fails"
http.c | 15 +++++++-------
t/t5551-http-fetch-smart.sh | 41 +++++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+), 8 deletions(-)
-Peff
From: Jeff King <hidden> Date: 2021-05-18 06:27:38
We test authentication with http, and we independently test that
credential helpers work, but we don't have any tests that cover the
two features working together. Let's add two:
1. Make sure that a successful request asks the helper to save the
credential. This works as expected.
2. Make sure that a failed request asks the helper to forget the
credential. This is marked as expect_failure, as it was recently
regressed by 1b0d9545bb (remote-curl: fall back to basic auth if
Negotiate fails, 2021-03-22). The symptom here is that the second
request should prompt the user, but doesn't.
Signed-off-by: Jeff King <redacted>
---
t/t5551-http-fetch-smart.sh | 41 +++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
@@ -517,4 +517,45 @@ test_expect_success 'server-side error detected' 'test_i18ngrep"server-side error"actual'+test_expect_success'http auth remembers successful credentials''+rm-f.git-credentials&&+test_configcredential.helperstore&&++# the first request prompts the user...+set_askpassuser@hostpass@host&&+gitls-remote"$HTTPD_URL/auth/smart/repo.git">/dev/null&&+expect_askpassbothuser@host&&++# ...and the second one uses the stored value rather than+# prompting the user.+set_askpassbogus-userbogus-pass&&+gitls-remote"$HTTPD_URL/auth/smart/repo.git">/dev/null&&+expect_askpassnone+'++test_expect_failure'http auth forgets bogus credentials''+# seed credential store with bogus values. In real life,+# this would probably come from a password which worked+# for a previous request.+rm-f.git-credentials&&+test_configcredential.helperstore&&+{+echo"url=$HTTPD_URL"&&+echo"username=bogus"&&+echo"password=bogus"+}|gitcredentialapprove&&++# we expect this to use the bogus values and fail, never even+# prompting the user...+set_askpassuser@hostpass@host&&+test_must_failgitls-remote"$HTTPD_URL/auth/smart/repo.git">/dev/null&&+expect_askpassnone&&++# ...but now we should have forgotten the bad value, causing+# us to prompt the user again.+set_askpassuser@hostpass@host&&+gitls-remote"$HTTPD_URL/auth/smart/repo.git">/dev/null&&+expect_askpassbothuser@host+'+ test_done
From: Jeff King <hidden> Date: 2021-05-18 06:27:56
This reverts commit 1b0d9545bb85912a16b367229d414f55d140d3be.
That commit does fix the situation it intended to (avoiding Negotiate
even when the credentials were provided in the URL), but it creates a
more serious regression: we now never hit the conditional for "we had a
username and password, tried them, but the server still gave us a 401".
That has two bad effects:
1. we never call credential_reject(), and thus a bogus credential
stored by a helper will live on forever
2. we never return HTTP_NOAUTH, so the error message the user gets is
"The requested URL returned error: 401", instead of "Authentication
failed".
Doing this correctly seems non-trivial, as we don't know whether the
Negotiate auth was a problem. Since this is a regression in the upcoming
v2.23.0 release (for which we're in -rc0), let's revert for now and work
on a fix separately.
(Note that this isn't a pure revert; the previous commit added a test
showing the regression, so we can now flip it to expect_success).
Reported-by: Ben Humphreys <redacted>
Signed-off-by: Jeff King <redacted>
---
http.c | 15 +++++++--------
t/t5551-http-fetch-smart.sh | 2 +-
2 files changed, 8 insertions(+), 9 deletions(-)
@@ -533,7 +533,7 @@ test_expect_success 'http auth remembers successful credentials' 'expect_askpassnone'-test_expect_failure'http auth forgets bogus credentials''+test_expect_success'http auth forgets bogus credentials''# seed credential store with bogus values. In real life,# this would probably come from a password which worked# for a previous request.
This reverts commit 1b0d9545bb85912a16b367229d414f55d140d3be.
That commit does fix the situation it intended to (avoiding Negotiate
even when the credentials were provided in the URL), but it creates a
more serious regression: we now never hit the conditional for "we had a
username and password, tried them, but the server still gave us a 401".
That has two bad effects:
1. we never call credential_reject(), and thus a bogus credential
stored by a helper will live on forever
2. we never return HTTP_NOAUTH, so the error message the user gets is
"The requested URL returned error: 401", instead of "Authentication
failed".
Doing this correctly seems non-trivial, as we don't know whether the
Negotiate auth was a problem. Since this is a regression in the upcoming
v2.23.0 release (for which we're in -rc0), let's revert for now and work
on a fix separately.
I think the revert is sufficiently justified here.
(Note that this isn't a pure revert; the previous commit added a test
showing the regression, so we can now flip it to expect_success).
Keeping the test is excellent, because it gives us a way to confirm
that a second attempt at a fix is at least as good as the first.
The only thing that could improve this situation is to add a test
that checks the bug that the previous version introduced, so that
the next round doesn't repeat the mistake. That can be deferred
because it is more important that we get this fix in time for the
next release candidate.
Thanks,
-Stolee
From: Jeff King <hidden> Date: 2021-05-19 14:15:38
On Wed, May 19, 2021 at 09:58:50AM -0400, Derrick Stolee wrote:
quoted
(Note that this isn't a pure revert; the previous commit added a test
showing the regression, so we can now flip it to expect_success).
Keeping the test is excellent, because it gives us a way to confirm
that a second attempt at a fix is at least as good as the first.
The only thing that could improve this situation is to add a test
that checks the bug that the previous version introduced, so that
the next round doesn't repeat the mistake. That can be deferred
because it is more important that we get this fix in time for the
next release candidate.
Re-reading what I wrote, I think "the previous commit" may be ambiguous.
The original commit which introduced the bug (and which we're reverting
here) didn't include a test at all. In patch 1/2 of this series (what
I'm calling "the previous commit"), I provided a test which shows the
regression. And now this revert shows that we fixed it (by flipping from
expect_failure to expect_success).
So I think I've already done what you're asking (if I understand it
correctly).
It probably would be a little easier to follow by reverting first, and
then adding in the expect_success test on top to future-proof us. But by
doing it in the other order, it was easy to see the test demonstrate the
behavior before and after the revert.
-Peff
On Wed, May 19, 2021 at 09:58:50AM -0400, Derrick Stolee wrote:
quoted
quoted
(Note that this isn't a pure revert; the previous commit added a test
showing the regression, so we can now flip it to expect_success).
Keeping the test is excellent, because it gives us a way to confirm
that a second attempt at a fix is at least as good as the first.
The only thing that could improve this situation is to add a test
that checks the bug that the previous version introduced, so that
the next round doesn't repeat the mistake. That can be deferred
because it is more important that we get this fix in time for the
next release candidate.
Re-reading what I wrote, I think "the previous commit" may be ambiguous.
The original commit which introduced the bug (and which we're reverting
here) didn't include a test at all. In patch 1/2 of this series (what
I'm calling "the previous commit"), I provided a test which shows the
regression. And now this revert shows that we fixed it (by flipping from
expect_failure to expect_success).
So I think I've already done what you're asking (if I understand it
correctly).
Ah. For some reason my email client didn't thread your messages
together, so I saw this as a one-off patch (ignoring the 2/2 part
of the message, of course).
Thanks,
-Stolee
From: brian m. carlson <hidden> Date: 2021-05-19 00:12:59
On 2021-05-18 at 05:50:26, Jeff King wrote:
quoted hunk
So it is focused on the case when the credentials came in the URL,
before the first contact with the server (where we'd get an HTTP 401).
And the diff moves the negotiate check earlier in the function, before
we see if we already have credentials:
So in that case, we'd clear the GSSNEGOTIATE bit and return HTTP_REAUTH,
and the caller will try again. Makes sense for the use case described.
But imagine we didn't get a username/password in the URL. The first
request will return REAUTH because of this moved code path (just as it
would have before, because http.auth.{username,password} are not set).
And then we'll get a credential from the user or from a helper and try
again. But this time, if we fail, we'll return HTTP_REAUTH again! We
never hit the "if (http_auth.username && http_auth.password)" check at
all. And hence we never return HTTP_NOAUTH (which gives us the more
useful "authentication failed" message), nor the credential_reject()
line (which informs helpers to stop caching a known-bad password).
I think what we'd want to do in this case is to only call HTTP_REAUTH if
we actually cleared CURLAUTH_GSSNEGOTIATE. Maybe something like this:
@@ -1650,18 +1650,18 @@ static int handle_curl_result(struct slot_results *results)}elseif(missing_target(results))returnHTTP_MISSING_TARGET;elseif(results->http_code==401){+intused_negotiate=0;#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY+if(http_auth_methods&CURLAUTH_GSSNEGOTIATE)+used_negotiate=1;http_auth_methods&=~CURLAUTH_GSSNEGOTIATE;-if(results->auth_avail){-http_auth_methods&=results->auth_avail;-http_auth_methods_restricted=1;-returnHTTP_REAUTH;-}#endif-if(http_auth.username&&http_auth.password){+if(!used_negotiate&&http_auth.username&&http_auth.password){credential_reject(&http_auth);returnHTTP_NOAUTH;}else{+http_auth_methods&=results->auth_avail;+http_auth_methods_restricted=1;returnHTTP_REAUTH;}}else{
That, of course, is totally untested, and I don't have Basic auth
fallback set up on my server with Kerberos, so I can't test it.
I suspect we could hack around it by pessimistically guessing that
GSSNEGOTIATE was the problem. But I'm worried that making that work
would require up to three requests (one to find out we need auth, one to
remove the GSSNEGOTIATE bit, and one to retry with a username/password).
That seems like punishing people with servers that don't even care about
Negotiate for no reason.
I think my proposal above does that, but I'm not sure. If Negotiate
wasn't set, we won't need to make a third request, since we'll have
known the supported mechanisms as part of the original 401. If they do
support both, then three requests will be required if they have to fall
back to Basic auth, but then they're only paying the price for the
environment they have.
If we aren't already reading the supported mechanisms out of the initial
401, then we'll need the third request, but that would be silly and we
should just avoid doing that.
So perhaps somebody can come up with something clever, but I suspect we
may need to just revert this for the v2.32 release, and re-break the
case that 1b0d9545bb8 was trying to solve.
Yeah, I think this is the right solution for the problem until somebody
with a suitable mixed auth environment shows up and can test. Your
patches seemed reasonable and, as always, well explained.
--
brian m. carlson (he/him or they/them)
Houston, Texas, US
From: Jeff King <hidden> Date: 2021-05-19 11:49:57
On Wed, May 19, 2021 at 12:12:52AM +0000, brian m. carlson wrote:
quoted
But imagine we didn't get a username/password in the URL. The first
request will return REAUTH because of this moved code path (just as it
would have before, because http.auth.{username,password} are not set).
And then we'll get a credential from the user or from a helper and try
again. But this time, if we fail, we'll return HTTP_REAUTH again! We
never hit the "if (http_auth.username && http_auth.password)" check at
all. And hence we never return HTTP_NOAUTH (which gives us the more
useful "authentication failed" message), nor the credential_reject()
line (which informs helpers to stop caching a known-bad password).
I think what we'd want to do in this case is to only call HTTP_REAUTH if
we actually cleared CURLAUTH_GSSNEGOTIATE. Maybe something like this:
[...]
Yeah, that was my instinct, too, but...
quoted
I suspect we could hack around it by pessimistically guessing that
GSSNEGOTIATE was the problem. But I'm worried that making that work
would require up to three requests (one to find out we need auth, one to
remove the GSSNEGOTIATE bit, and one to retry with a username/password).
That seems like punishing people with servers that don't even care about
Negotiate for no reason.
I think my proposal above does that, but I'm not sure. If Negotiate
wasn't set, we won't need to make a third request, since we'll have
known the supported mechanisms as part of the original 401. If they do
support both, then three requests will be required if they have to fall
back to Basic auth, but then they're only paying the price for the
environment they have.
If we aren't already reading the supported mechanisms out of the initial
401, then we'll need the third request, but that would be silly and we
should just avoid doing that.
Yeah, I was worried that just clearing the bit results in the extra
round-trip. I think we do clear bits based on what the other side showed
us. That's the:
http_auth_methods &= results->auth_avail;
in the code being discussed. But it seems like we'd want to do that as
part of setting the "used negotiate" flag in your sample patch. I.e.,:
if (http_auth_methods & results->auth_avail & CURLAUTH_GSSNEGOTIATE)
used_negotiate = 1;
But it's entirely possible I don't understand the subtleties around
unsetting GSSNEGOTIATE in the first place (it's not something I've ever
used myself).
quoted
So perhaps somebody can come up with something clever, but I suspect we
may need to just revert this for the v2.32 release, and re-break the
case that 1b0d9545bb8 was trying to solve.
Yeah, I think this is the right solution for the problem until somebody
with a suitable mixed auth environment shows up and can test. Your
patches seemed reasonable and, as always, well explained.