* Git 2.23.0-rc0 HTTP authentication failure - error message change @ 2021-05-18 3:07 Ben Humphreys 2021-05-18 5:50 ` Jeff King 0 siblings, 1 reply; 11+ messages in thread From: Ben Humphreys @ 2021-05-18 3:07 UTC (permalink / raw) To: git Hi, I've run 2.32.0-rc0 through the Bitbucket Server test matrix and it picked up a change in error message that perhaps is unintentional. On 2.31.1: $ git ls-remote http://notauser@localhost:7990/bitbucket/scm/project_1/rep_1.git Password for 'http://notauser@localhost:7990': fatal: Authentication failed for 'http://localhost:7990/bitbucket/scm/project_1/rep_1.git/' and on 2.32.0-rc0: $ git ls-remote http://notauser@localhost:7990/bitbucket/scm/project_1/rep_1.git Password for 'http://notauser@localhost:7990': fatal: unable to access 'http://localhost:7990/bitbucket/scm/project_1/rep_1.git/': The requested URL returned error: 401 This appears to be a side effect of 1b0d9545bb8, if I revert that commit the old behavior returns. Certainly we can update our pattern matching to match the new message, but I wonder if the old message is preferred for folks who are not familiar with HTTP status codes. Best Regards, Ben Humphreys ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Git 2.23.0-rc0 HTTP authentication failure - error message change 2021-05-18 3:07 Git 2.23.0-rc0 HTTP authentication failure - error message change Ben Humphreys @ 2021-05-18 5:50 ` Jeff King 2021-05-18 6:26 ` [PATCH 0/2] fix v2.32.0-rc0 bug with Negotiate / HTTP_NOAUTH Jeff King 2021-05-19 0:12 ` Git 2.23.0-rc0 HTTP authentication failure - error message change brian m. carlson 0 siblings, 2 replies; 11+ messages in thread From: Jeff King @ 2021-05-18 5:50 UTC (permalink / raw) To: Ben Humphreys; +Cc: Junio C Hamano, Christopher Schenk, git On Tue, May 18, 2021 at 01:07:36PM +1000, Ben Humphreys wrote: > I've run 2.32.0-rc0 through the Bitbucket Server test matrix and it picked up a > change in error message that perhaps is unintentional. > > On 2.31.1: > $ git ls-remote http://notauser@localhost:7990/bitbucket/scm/project_1/rep_1.git > Password for 'http://notauser@localhost:7990': > fatal: Authentication failed for 'http://localhost:7990/bitbucket/scm/project_1/rep_1.git/' > > and on 2.32.0-rc0: > $ git ls-remote http://notauser@localhost:7990/bitbucket/scm/project_1/rep_1.git > Password for 'http://notauser@localhost:7990': > fatal: unable to access 'http://localhost:7990/bitbucket/scm/project_1/rep_1.git/': The requested URL returned error: 401 > > This appears to be a side effect of 1b0d9545bb8, if I revert that commit the old > behavior returns. Certainly we can update our pattern matching to match the new message, > but I wonder if the old message is preferred for folks who are not familiar with HTTP > status codes. 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: diff --git a/http.c b/http.c index 0e31fc21bc..19c203d0ca 100644 --- a/http.c +++ b/http.c @@ -1641,17 +1641,18 @@ static int handle_curl_result(struct slot_results *results) } else if (missing_target(results)) return HTTP_MISSING_TARGET; else if (results->http_code == 401) { +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY + http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE; + if (results->auth_avail) { + http_auth_methods &= results->auth_avail; + http_auth_methods_restricted = 1; + return HTTP_REAUTH; + } +#endif if (http_auth.username && http_auth.password) { credential_reject(&http_auth); return HTTP_NOAUTH; } else { -#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY - http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE; - if (results->auth_avail) { - http_auth_methods &= results->auth_avail; - http_auth_methods_restricted = 1; - } -#endif return HTTP_REAUTH; } } else { 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 0/2] fix v2.32.0-rc0 bug with Negotiate / HTTP_NOAUTH 2021-05-18 5:50 ` Jeff King @ 2021-05-18 6:26 ` Jeff King 2021-05-18 6:27 ` [PATCH 1/2] t5551: test http interaction with credential helpers Jeff King ` (2 more replies) 2021-05-19 0:12 ` Git 2.23.0-rc0 HTTP authentication failure - error message change brian m. carlson 1 sibling, 3 replies; 11+ messages in thread From: Jeff King @ 2021-05-18 6:26 UTC (permalink / raw) To: Ben Humphreys; +Cc: Junio C Hamano, Christopher Schenk, git 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] t5551: test http interaction with credential helpers 2021-05-18 6:26 ` [PATCH 0/2] fix v2.32.0-rc0 bug with Negotiate / HTTP_NOAUTH Jeff King @ 2021-05-18 6:27 ` Jeff King 2021-05-18 6:27 ` [PATCH 2/2] Revert "remote-curl: fall back to basic auth if Negotiate fails" Jeff King 2021-05-19 1:12 ` [PATCH 0/2] fix v2.32.0-rc0 bug with Negotiate / HTTP_NOAUTH Junio C Hamano 2 siblings, 0 replies; 11+ messages in thread From: Jeff King @ 2021-05-18 6:27 UTC (permalink / raw) To: Ben Humphreys; +Cc: Junio C Hamano, Christopher Schenk, git 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 <peff@peff.net> --- t/t5551-http-fetch-smart.sh | 41 +++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 984dba22af..1de87e4ffe 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -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_config credential.helper store && + + # the first request prompts the user... + set_askpass user@host pass@host && + git ls-remote "$HTTPD_URL/auth/smart/repo.git" >/dev/null && + expect_askpass both user@host && + + # ...and the second one uses the stored value rather than + # prompting the user. + set_askpass bogus-user bogus-pass && + git ls-remote "$HTTPD_URL/auth/smart/repo.git" >/dev/null && + expect_askpass none +' + +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_config credential.helper store && + { + echo "url=$HTTPD_URL" && + echo "username=bogus" && + echo "password=bogus" + } | git credential approve && + + # we expect this to use the bogus values and fail, never even + # prompting the user... + set_askpass user@host pass@host && + test_must_fail git ls-remote "$HTTPD_URL/auth/smart/repo.git" >/dev/null && + expect_askpass none && + + # ...but now we should have forgotten the bad value, causing + # us to prompt the user again. + set_askpass user@host pass@host && + git ls-remote "$HTTPD_URL/auth/smart/repo.git" >/dev/null && + expect_askpass both user@host +' + test_done -- 2.32.0.rc0.421.g64c9147932 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] Revert "remote-curl: fall back to basic auth if Negotiate fails" 2021-05-18 6:26 ` [PATCH 0/2] fix v2.32.0-rc0 bug with Negotiate / HTTP_NOAUTH Jeff King 2021-05-18 6:27 ` [PATCH 1/2] t5551: test http interaction with credential helpers Jeff King @ 2021-05-18 6:27 ` Jeff King 2021-05-19 13:58 ` Derrick Stolee 2021-05-19 1:12 ` [PATCH 0/2] fix v2.32.0-rc0 bug with Negotiate / HTTP_NOAUTH Junio C Hamano 2 siblings, 1 reply; 11+ messages in thread From: Jeff King @ 2021-05-18 6:27 UTC (permalink / raw) To: Ben Humphreys; +Cc: Junio C Hamano, Christopher Schenk, git 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 <behumphreys@atlassian.com> Signed-off-by: Jeff King <peff@peff.net> --- http.c | 15 +++++++-------- t/t5551-http-fetch-smart.sh | 2 +- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/http.c b/http.c index c83bc33a5f..8119247149 100644 --- a/http.c +++ b/http.c @@ -1650,18 +1650,17 @@ static int handle_curl_result(struct slot_results *results) } else if (missing_target(results)) return HTTP_MISSING_TARGET; else if (results->http_code == 401) { -#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY - http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE; - if (results->auth_avail) { - http_auth_methods &= results->auth_avail; - http_auth_methods_restricted = 1; - return HTTP_REAUTH; - } -#endif if (http_auth.username && http_auth.password) { credential_reject(&http_auth); return HTTP_NOAUTH; } else { +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY + http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE; + if (results->auth_avail) { + http_auth_methods &= results->auth_avail; + http_auth_methods_restricted = 1; + } +#endif return HTTP_REAUTH; } } else { diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 1de87e4ffe..4f87d90c5b 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -533,7 +533,7 @@ test_expect_success 'http auth remembers successful credentials' ' expect_askpass none ' -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. -- 2.32.0.rc0.421.g64c9147932 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] Revert "remote-curl: fall back to basic auth if Negotiate fails" 2021-05-18 6:27 ` [PATCH 2/2] Revert "remote-curl: fall back to basic auth if Negotiate fails" Jeff King @ 2021-05-19 13:58 ` Derrick Stolee 2021-05-19 14:14 ` Jeff King 0 siblings, 1 reply; 11+ messages in thread From: Derrick Stolee @ 2021-05-19 13:58 UTC (permalink / raw) To: Jeff King, Ben Humphreys; +Cc: Junio C Hamano, Christopher Schenk, git On 5/18/2021 2:27 AM, Jeff King wrote: > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] Revert "remote-curl: fall back to basic auth if Negotiate fails" 2021-05-19 13:58 ` Derrick Stolee @ 2021-05-19 14:14 ` Jeff King 2021-05-19 14:53 ` Derrick Stolee 0 siblings, 1 reply; 11+ messages in thread From: Jeff King @ 2021-05-19 14:14 UTC (permalink / raw) To: Derrick Stolee; +Cc: Ben Humphreys, Junio C Hamano, Christopher Schenk, git On Wed, May 19, 2021 at 09:58:50AM -0400, Derrick Stolee wrote: > > (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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] Revert "remote-curl: fall back to basic auth if Negotiate fails" 2021-05-19 14:14 ` Jeff King @ 2021-05-19 14:53 ` Derrick Stolee 0 siblings, 0 replies; 11+ messages in thread From: Derrick Stolee @ 2021-05-19 14:53 UTC (permalink / raw) To: Jeff King; +Cc: Ben Humphreys, Junio C Hamano, Christopher Schenk, git On 5/19/2021 10:14 AM, Jeff King wrote: > On Wed, May 19, 2021 at 09:58:50AM -0400, Derrick Stolee wrote: > >>> (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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] fix v2.32.0-rc0 bug with Negotiate / HTTP_NOAUTH 2021-05-18 6:26 ` [PATCH 0/2] fix v2.32.0-rc0 bug with Negotiate / HTTP_NOAUTH Jeff King 2021-05-18 6:27 ` [PATCH 1/2] t5551: test http interaction with credential helpers Jeff King 2021-05-18 6:27 ` [PATCH 2/2] Revert "remote-curl: fall back to basic auth if Negotiate fails" Jeff King @ 2021-05-19 1:12 ` Junio C Hamano 2 siblings, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2021-05-19 1:12 UTC (permalink / raw) To: Jeff King; +Cc: Ben Humphreys, Christopher Schenk, git Jeff King <peff@peff.net> writes: > 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. I'll queue these directly on top of the original topic, so that even a merge of the branch by mistake down to maint won't break. I saw brian has already floated an idea to solve the original issue, and I'd encourage people to help finding a proper solution, but let's leave the actual application of such a change for the next cycle. Thanks. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Git 2.23.0-rc0 HTTP authentication failure - error message change 2021-05-18 5:50 ` Jeff King 2021-05-18 6:26 ` [PATCH 0/2] fix v2.32.0-rc0 bug with Negotiate / HTTP_NOAUTH Jeff King @ 2021-05-19 0:12 ` brian m. carlson 2021-05-19 11:49 ` Jeff King 1 sibling, 1 reply; 11+ messages in thread From: brian m. carlson @ 2021-05-19 0:12 UTC (permalink / raw) To: Jeff King; +Cc: Ben Humphreys, Junio C Hamano, Christopher Schenk, git [-- Attachment #1: Type: text/plain, Size: 4775 bytes --] On 2021-05-18 at 05:50:26, Jeff King wrote: > 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: > > diff --git a/http.c b/http.c > index 0e31fc21bc..19c203d0ca 100644 > --- a/http.c > +++ b/http.c > @@ -1641,17 +1641,18 @@ static int handle_curl_result(struct slot_results *results) > } else if (missing_target(results)) > return HTTP_MISSING_TARGET; > else if (results->http_code == 401) { > +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY > + http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE; > + if (results->auth_avail) { > + http_auth_methods &= results->auth_avail; > + http_auth_methods_restricted = 1; > + return HTTP_REAUTH; > + } > +#endif > if (http_auth.username && http_auth.password) { > credential_reject(&http_auth); > return HTTP_NOAUTH; > } else { > -#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY > - http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE; > - if (results->auth_avail) { > - http_auth_methods &= results->auth_avail; > - http_auth_methods_restricted = 1; > - } > -#endif > return HTTP_REAUTH; > } > } else { > > 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: diff --git a/http.c b/http.c index c83bc33a5f..e9fead8cd8 100644 --- a/http.c +++ b/http.c @@ -1650,18 +1650,18 @@ static int handle_curl_result(struct slot_results *results) } else if (missing_target(results)) return HTTP_MISSING_TARGET; else if (results->http_code == 401) { + int used_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; - return HTTP_REAUTH; - } #endif - if (http_auth.username && http_auth.password) { + if (!used_negotiate && http_auth.username && http_auth.password) { credential_reject(&http_auth); return HTTP_NOAUTH; } else { + http_auth_methods &= results->auth_avail; + http_auth_methods_restricted = 1; return HTTP_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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 262 bytes --] ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: Git 2.23.0-rc0 HTTP authentication failure - error message change 2021-05-19 0:12 ` Git 2.23.0-rc0 HTTP authentication failure - error message change brian m. carlson @ 2021-05-19 11:49 ` Jeff King 0 siblings, 0 replies; 11+ messages in thread From: Jeff King @ 2021-05-19 11:49 UTC (permalink / raw) To: brian m. carlson; +Cc: Ben Humphreys, Junio C Hamano, Christopher Schenk, git On Wed, May 19, 2021 at 12:12:52AM +0000, brian m. carlson wrote: > > 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... > > 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). > > 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. Thanks for taking a look! -Peff ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-05-19 14:53 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-18 3:07 Git 2.23.0-rc0 HTTP authentication failure - error message change Ben Humphreys 2021-05-18 5:50 ` Jeff King 2021-05-18 6:26 ` [PATCH 0/2] fix v2.32.0-rc0 bug with Negotiate / HTTP_NOAUTH Jeff King 2021-05-18 6:27 ` [PATCH 1/2] t5551: test http interaction with credential helpers Jeff King 2021-05-18 6:27 ` [PATCH 2/2] Revert "remote-curl: fall back to basic auth if Negotiate fails" Jeff King 2021-05-19 13:58 ` Derrick Stolee 2021-05-19 14:14 ` Jeff King 2021-05-19 14:53 ` Derrick Stolee 2021-05-19 1:12 ` [PATCH 0/2] fix v2.32.0-rc0 bug with Negotiate / HTTP_NOAUTH Junio C Hamano 2021-05-19 0:12 ` Git 2.23.0-rc0 HTTP authentication failure - error message change brian m. carlson 2021-05-19 11:49 ` Jeff King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).