{"thread":{"id":"55715","subject":"Git 2.23.0-rc0 HTTP authentication failure - error message change","startedAt":"2021-05-18T03:07:43Z","lastAt":"2021-05-19T14:53:51Z","messageCount":11,"participants":["Ben Humphreys","Jeff King","brian m. carlson","Junio C Hamano","Derrick Stolee"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"424810","messageId":"YKMvePOPqjwiXeQ+@C02WD045HTDG","threadId":"55715","inReplyTo":null,"subject":"Git 2.23.0-rc0 HTTP authentication failure - error message change","fromName":"Ben Humphreys","fromEmail":"behumphreys@atlassian.com","sentAt":"2021-05-18T03:07:36Z","receivedAt":"2021-05-18T03:07:43Z","isPatch":false,"sender":{"key":"behumphreys@atlassian.com","avatar":"https://gravatar.com/avatar/5cb49575477c33416331403d493bb02758031b2a4e5183491395b02cea9e8236?d=mp&s=160"},"body":"Hi,\n\nI've run 2.32.0-rc0 through the Bitbucket Server test matrix and it picked up a\nchange in error message that perhaps is unintentional.\n\nOn 2.31.1:\n  $ git ls-remote http://notauser@localhost:7990/bitbucket/scm/project_1/rep_1.git\n  Password for 'http://notauser@localhost:7990':\n  fatal: Authentication failed for 'http://localhost:7990/bitbucket/scm/project_1/rep_1.git/'\n\nand on 2.32.0-rc0:\n  $ git ls-remote http://notauser@localhost:7990/bitbucket/scm/project_1/rep_1.git\n  Password for 'http://notauser@localhost:7990':\n  fatal: unable to access 'http://localhost:7990/bitbucket/scm/project_1/rep_1.git/': The requested URL returned error: 401\n\nThis appears to be a side effect of 1b0d9545bb8, if I revert that commit the old\nbehavior returns. Certainly we can update our pattern matching to match the new message,\nbut I wonder if the old message is preferred for folks who are not familiar with HTTP\nstatus codes.\n\nBest Regards,\nBen Humphreys\n"},{"id":"424825","messageId":"YKNVop80H8xSTCjz@coredump.intra.peff.net","threadId":"55715","inReplyTo":"YKMvePOPqjwiXeQ+@C02WD045HTDG","subject":"Re: Git 2.23.0-rc0 HTTP authentication failure - error message change","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-18T05:50:26Z","receivedAt":"2021-05-18T05:50:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 18, 2021 at 01:07:36PM +1000, Ben Humphreys wrote:\n\n> I've run 2.32.0-rc0 through the Bitbucket Server test matrix and it picked up a\n> change in error message that perhaps is unintentional.\n> \n> On 2.31.1:\n>   $ git ls-remote http://notauser@localhost:7990/bitbucket/scm/project_1/rep_1.git\n>   Password for 'http://notauser@localhost:7990':\n>   fatal: Authentication failed for 'http://localhost:7990/bitbucket/scm/project_1/rep_1.git/'\n> \n> and on 2.32.0-rc0:\n>   $ git ls-remote http://notauser@localhost:7990/bitbucket/scm/project_1/rep_1.git\n>   Password for 'http://notauser@localhost:7990':\n>   fatal: unable to access 'http://localhost:7990/bitbucket/scm/project_1/rep_1.git/': The requested URL returned error: 401\n> \n> This appears to be a side effect of 1b0d9545bb8, if I revert that commit the old\n> behavior returns. Certainly we can update our pattern matching to match the new message,\n> but I wonder if the old message is preferred for folks who are not familiar with HTTP\n> status codes.\n\nThanks for reporting; I've added the author of that commit to the cc.\n\nI think this is definitely a regression in the output we're giving, but\nit also points to a bug in the behavior (we fail to forget about bad\ncredentials; see below).\n\nThe motivation from the commit is:\n\n    When the username and password are supplied in a url like this\n    https://myuser:secret@git.exampe/myrepo.git and the server supports the\n    negotiate authenticaten method, git does not fall back to basic auth and\n    libcurl hardly tries to authenticate with the negotiate method.\n\n    Stop using the Negotiate authentication method after the first failure\n    because if it fails on the first try it will never succeed.\n\nSo it is focused on the case when the credentials came in the URL,\nbefore the first contact with the server (where we'd get an HTTP 401).\nAnd the diff moves the negotiate check earlier in the function, before\nwe see if we already have credentials:\n\ndiff --git a/http.c b/http.c\nindex 0e31fc21bc..19c203d0ca 100644\n--- a/http.c\n+++ b/http.c\n@@ -1641,17 +1641,18 @@ static int handle_curl_result(struct slot_results *results)\n \t} else if (missing_target(results))\n \t\treturn HTTP_MISSING_TARGET;\n \telse if (results->http_code == 401) {\n+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n+\t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n+\t\tif (results->auth_avail) {\n+\t\t\thttp_auth_methods &= results->auth_avail;\n+\t\t\thttp_auth_methods_restricted = 1;\n+\t\t\treturn HTTP_REAUTH;\n+\t\t}\n+#endif\n \t\tif (http_auth.username && http_auth.password) {\n \t\t\tcredential_reject(&http_auth);\n \t\t\treturn HTTP_NOAUTH;\n \t\t} else {\n-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n-\t\t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n-\t\t\tif (results->auth_avail) {\n-\t\t\t\thttp_auth_methods &= results->auth_avail;\n-\t\t\t\thttp_auth_methods_restricted = 1;\n-\t\t\t}\n-#endif\n \t\t\treturn HTTP_REAUTH;\n \t\t}\n \t} else {\n\nSo in that case, we'd clear the GSSNEGOTIATE bit and return HTTP_REAUTH,\nand the caller will try again. Makes sense for the use case described.\n\nBut imagine we didn't get a username/password in the URL. The first\nrequest will return REAUTH because of this moved code path (just as it\nwould have before, because http.auth.{username,password} are not set).\nAnd then we'll get a credential from the user or from a helper and try\nagain. But this time, if we fail, we'll return HTTP_REAUTH again! We\nnever hit the \"if (http_auth.username && http_auth.password)\" check at\nall. And hence we never return HTTP_NOAUTH (which gives us the more\nuseful \"authentication failed\" message), nor the credential_reject()\nline (which informs helpers to stop caching a known-bad password).\n\nWe can see it like this. First, seed the cache with a bad password (in\nthis case a bogus token to elicit a 401 response from GitHub). In real\nlife, this would be a password stored from a previous successful\nattempt, but which was invalidated in the meantime. Note the empty value\nfor credential.helper clears the list of any regular helpers you'd use\nfor github.com.\n\n  echo url=https://peff:ghp_000000000000000000000000000000000000@github.com |\n  git -c credential.helper= \\\n      -c credential.helper=cache \\\n      credential approve\n\nNow make a request that requires auth; we expect it to fail since our\ncredential is bad.\n\n  git -c credential.helper= \\\n        -c credential.helper=cache \\\n        ls-remote https://github.com/peff/foo\n\nwhich yields:\n\n  remote: Invalid username or password.\n  fatal: unable to access 'https://github.com/peff/foo/': The requested URL returned error: 401\n\nas expected (except for the error message). But we also expect it to\nclear the bogus credential from the helper, so that if we run ls-remote\nagain, it will prompt us. But it doesn't! With git v2.32.0-rc0, you can\nrun that ls-remote as many times as you want, and it will always fail.\n\nWhereas if you switch to 1b0d9545bb8^, on the second run it will\ncorrectly prompt you for an updated password.\n\nI think for this to work, we would need to figure out from libcurl's\nresponse that GSSNEGOTIATE was the problem for our particular request,\nand only trigger the bit-clearing and HTTP_REAUTH if that was true. I'm\nnot sure if that's possible, though.\n\nI suspect we could hack around it by pessimistically guessing that\nGSSNEGOTIATE was the problem. But I'm worried that making that work\nwould require up to three requests (one to find out we need auth, one to\nremove the GSSNEGOTIATE bit, and one to retry with a username/password).\nThat seems like punishing people with servers that don't even care about\nNegotiate for no reason.\n\nSo perhaps somebody can come up with something clever, but I suspect we\nmay need to just revert this for the v2.32 release, and re-break the\ncase that 1b0d9545bb8 was trying to solve.\n\nWe probably should beef up the tests around http's credential-rejection,\ntoo, to catch this regression.\n\n-Peff\n"},{"id":"424829","messageId":"YKNeJ+NKUbD5ixA9@coredump.intra.peff.net","threadId":"55715","inReplyTo":"YKNVop80H8xSTCjz@coredump.intra.peff.net","subject":"[PATCH 0/2] fix v2.32.0-rc0 bug with Negotiate / HTTP_NOAUTH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-18T06:26:47Z","receivedAt":"2021-05-18T06:26:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 18, 2021 at 01:50:27AM -0400, Jeff King wrote:\n\n> So perhaps somebody can come up with something clever, but I suspect we\n> may need to just revert this for the v2.32 release, and re-break the\n> case that 1b0d9545bb8 was trying to solve.\n> \n> We probably should beef up the tests around http's credential-rejection,\n> too, to catch this regression.\n\nHere are some patches. The first one adds the tests, and I think is an\nobvious improvement.\n\nThe second one does the revert. I'd be quite happy if somebody wants to\nfigure out how to fix it in a way that addresses the original problem,\nand then we can replace that. But in the meantime, I think it makes\nsense to prepare the revert, as we wouldn't want to release v2.32.0 with\nthe bug.\n\n  [1/2]: t5551: test http interaction with credential helpers\n  [2/2]: Revert \"remote-curl: fall back to basic auth if Negotiate fails\"\n\n http.c                      | 15 +++++++-------\n t/t5551-http-fetch-smart.sh | 41 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 8 deletions(-)\n\n-Peff\n"},{"id":"424830","messageId":"YKNeWOCaa31qRVqk@coredump.intra.peff.net","threadId":"55715","inReplyTo":"YKNeJ+NKUbD5ixA9@coredump.intra.peff.net","subject":"[PATCH 1/2] t5551: test http interaction with credential helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-18T06:27:36Z","receivedAt":"2021-05-18T06:27:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We test authentication with http, and we independently test that\ncredential helpers work, but we don't have any tests that cover the\ntwo features working together. Let's add two:\n\n  1. Make sure that a successful request asks the helper to save the\n     credential. This works as expected.\n\n  2. Make sure that a failed request asks the helper to forget the\n     credential. This is marked as expect_failure, as it was recently\n     regressed by 1b0d9545bb (remote-curl: fall back to basic auth if\n     Negotiate fails, 2021-03-22). The symptom here is that the second\n     request should prompt the user, but doesn't.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5551-http-fetch-smart.sh | 41 +++++++++++++++++++++++++++++++++++++\n 1 file changed, 41 insertions(+)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 984dba22af..1de87e4ffe 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -517,4 +517,45 @@ test_expect_success 'server-side error detected' '\n \ttest_i18ngrep \"server-side error\" actual\n '\n \n+test_expect_success 'http auth remembers successful credentials' '\n+\trm -f .git-credentials &&\n+\ttest_config credential.helper store &&\n+\n+\t# the first request prompts the user...\n+\tset_askpass user@host pass@host &&\n+\tgit ls-remote \"$HTTPD_URL/auth/smart/repo.git\" >/dev/null &&\n+\texpect_askpass both user@host &&\n+\n+\t# ...and the second one uses the stored value rather than\n+\t# prompting the user.\n+\tset_askpass bogus-user bogus-pass &&\n+\tgit ls-remote \"$HTTPD_URL/auth/smart/repo.git\" >/dev/null &&\n+\texpect_askpass none\n+'\n+\n+test_expect_failure 'http auth forgets bogus credentials' '\n+\t# seed credential store with bogus values. In real life,\n+\t# this would probably come from a password which worked\n+\t# for a previous request.\n+\trm -f .git-credentials &&\n+\ttest_config credential.helper store &&\n+\t{\n+\t\techo \"url=$HTTPD_URL\" &&\n+\t\techo \"username=bogus\" &&\n+\t\techo \"password=bogus\"\n+\t} | git credential approve &&\n+\n+\t# we expect this to use the bogus values and fail, never even\n+\t# prompting the user...\n+\tset_askpass user@host pass@host &&\n+\ttest_must_fail git ls-remote \"$HTTPD_URL/auth/smart/repo.git\" >/dev/null &&\n+\texpect_askpass none &&\n+\n+\t# ...but now we should have forgotten the bad value, causing\n+\t# us to prompt the user again.\n+\tset_askpass user@host pass@host &&\n+\tgit ls-remote \"$HTTPD_URL/auth/smart/repo.git\" >/dev/null &&\n+\texpect_askpass both user@host\n+'\n+\n test_done\n-- \n2.32.0.rc0.421.g64c9147932\n\n"},{"id":"424831","messageId":"YKNeXq3JzxYWkxKl@coredump.intra.peff.net","threadId":"55715","inReplyTo":"YKNeJ+NKUbD5ixA9@coredump.intra.peff.net","subject":"[PATCH 2/2] Revert \"remote-curl: fall back to basic auth if Negotiate fails\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-18T06:27:42Z","receivedAt":"2021-05-18T06:27:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This reverts commit 1b0d9545bb85912a16b367229d414f55d140d3be.\n\nThat commit does fix the situation it intended to (avoiding Negotiate\neven when the credentials were provided in the URL), but it creates a\nmore serious regression: we now never hit the conditional for \"we had a\nusername and password, tried them, but the server still gave us a 401\".\nThat has two bad effects:\n\n 1. we never call credential_reject(), and thus a bogus credential\n    stored by a helper will live on forever\n\n 2. we never return HTTP_NOAUTH, so the error message the user gets is\n    \"The requested URL returned error: 401\", instead of \"Authentication\n    failed\".\n\nDoing this correctly seems non-trivial, as we don't know whether the\nNegotiate auth was a problem. Since this is a regression in the upcoming\nv2.23.0 release (for which we're in -rc0), let's revert for now and work\non a fix separately.\n\n(Note that this isn't a pure revert; the previous commit added a test\nshowing the regression, so we can now flip it to expect_success).\n\nReported-by: Ben Humphreys <behumphreys@atlassian.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c                      | 15 +++++++--------\n t/t5551-http-fetch-smart.sh |  2 +-\n 2 files changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex c83bc33a5f..8119247149 100644\n--- a/http.c\n+++ b/http.c\n@@ -1650,18 +1650,17 @@ static int handle_curl_result(struct slot_results *results)\n \t} else if (missing_target(results))\n \t\treturn HTTP_MISSING_TARGET;\n \telse if (results->http_code == 401) {\n-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n-\t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n-\t\tif (results->auth_avail) {\n-\t\t\thttp_auth_methods &= results->auth_avail;\n-\t\t\thttp_auth_methods_restricted = 1;\n-\t\t\treturn HTTP_REAUTH;\n-\t\t}\n-#endif\n \t\tif (http_auth.username && http_auth.password) {\n \t\t\tcredential_reject(&http_auth);\n \t\t\treturn HTTP_NOAUTH;\n \t\t} else {\n+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n+\t\t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n+\t\t\tif (results->auth_avail) {\n+\t\t\t\thttp_auth_methods &= results->auth_avail;\n+\t\t\t\thttp_auth_methods_restricted = 1;\n+\t\t\t}\n+#endif\n \t\t\treturn HTTP_REAUTH;\n \t\t}\n \t} else {\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 1de87e4ffe..4f87d90c5b 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -533,7 +533,7 @@ test_expect_success 'http auth remembers successful credentials' '\n \texpect_askpass none\n '\n \n-test_expect_failure 'http auth forgets bogus credentials' '\n+test_expect_success 'http auth forgets bogus credentials' '\n \t# seed credential store with bogus values. In real life,\n \t# this would probably come from a password which worked\n \t# for a previous request.\n-- \n2.32.0.rc0.421.g64c9147932\n"},{"id":"424905","messageId":"YKRYBLeIlgILfHFj@camp.crustytoothpaste.net","threadId":"55715","inReplyTo":"YKNVop80H8xSTCjz@coredump.intra.peff.net","subject":"Re: Git 2.23.0-rc0 HTTP authentication failure - error message change","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-05-19T00:12:52Z","receivedAt":"2021-05-19T00:12:59Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-05-18 at 05:50:26, Jeff King wrote:\n> So it is focused on the case when the credentials came in the URL,\n> before the first contact with the server (where we'd get an HTTP 401).\n> And the diff moves the negotiate check earlier in the function, before\n> we see if we already have credentials:\n> \n> diff --git a/http.c b/http.c\n> index 0e31fc21bc..19c203d0ca 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1641,17 +1641,18 @@ static int handle_curl_result(struct slot_results *results)\n>  \t} else if (missing_target(results))\n>  \t\treturn HTTP_MISSING_TARGET;\n>  \telse if (results->http_code == 401) {\n> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n> +\t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n> +\t\tif (results->auth_avail) {\n> +\t\t\thttp_auth_methods &= results->auth_avail;\n> +\t\t\thttp_auth_methods_restricted = 1;\n> +\t\t\treturn HTTP_REAUTH;\n> +\t\t}\n> +#endif\n>  \t\tif (http_auth.username && http_auth.password) {\n>  \t\t\tcredential_reject(&http_auth);\n>  \t\t\treturn HTTP_NOAUTH;\n>  \t\t} else {\n> -#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n> -\t\t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n> -\t\t\tif (results->auth_avail) {\n> -\t\t\t\thttp_auth_methods &= results->auth_avail;\n> -\t\t\t\thttp_auth_methods_restricted = 1;\n> -\t\t\t}\n> -#endif\n>  \t\t\treturn HTTP_REAUTH;\n>  \t\t}\n>  \t} else {\n> \n> So in that case, we'd clear the GSSNEGOTIATE bit and return HTTP_REAUTH,\n> and the caller will try again. Makes sense for the use case described.\n> \n> But imagine we didn't get a username/password in the URL. The first\n> request will return REAUTH because of this moved code path (just as it\n> would have before, because http.auth.{username,password} are not set).\n> And then we'll get a credential from the user or from a helper and try\n> again. But this time, if we fail, we'll return HTTP_REAUTH again! We\n> never hit the \"if (http_auth.username && http_auth.password)\" check at\n> all. And hence we never return HTTP_NOAUTH (which gives us the more\n> useful \"authentication failed\" message), nor the credential_reject()\n> line (which informs helpers to stop caching a known-bad password).\n\nI think what we'd want to do in this case is to only call HTTP_REAUTH if\nwe actually cleared CURLAUTH_GSSNEGOTIATE.  Maybe something like this:\n\ndiff --git a/http.c b/http.c\nindex c83bc33a5f..e9fead8cd8 100644\n--- a/http.c\n+++ b/http.c\n@@ -1650,18 +1650,18 @@ static int handle_curl_result(struct slot_results *results)\n \t} else if (missing_target(results))\n \t\treturn HTTP_MISSING_TARGET;\n \telse if (results->http_code == 401) {\n+\t\tint used_negotiate = 0;\n #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n+\t\tif (http_auth_methods & CURLAUTH_GSSNEGOTIATE)\n+\t\t\tused_negotiate = 1;\n \t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n-\t\tif (results->auth_avail) {\n-\t\t\thttp_auth_methods &= results->auth_avail;\n-\t\t\thttp_auth_methods_restricted = 1;\n-\t\t\treturn HTTP_REAUTH;\n-\t\t}\n #endif\n-\t\tif (http_auth.username && http_auth.password) {\n+\t\tif (!used_negotiate && http_auth.username && http_auth.password) {\n \t\t\tcredential_reject(&http_auth);\n \t\t\treturn HTTP_NOAUTH;\n \t\t} else {\n+\t\t\thttp_auth_methods &= results->auth_avail;\n+\t\t\thttp_auth_methods_restricted = 1;\n \t\t\treturn HTTP_REAUTH;\n \t\t}\n \t} else {\n\nThat, of course, is totally untested, and I don't have Basic auth\nfallback set up on my server with Kerberos, so I can't test it.\n\n> I suspect we could hack around it by pessimistically guessing that\n> GSSNEGOTIATE was the problem. But I'm worried that making that work\n> would require up to three requests (one to find out we need auth, one to\n> remove the GSSNEGOTIATE bit, and one to retry with a username/password).\n> That seems like punishing people with servers that don't even care about\n> Negotiate for no reason.\n\nI think my proposal above does that, but I'm not sure.  If Negotiate\nwasn't set, we won't need to make a third request, since we'll have\nknown the supported mechanisms as part of the original 401.  If they do\nsupport both, then three requests will be required if they have to fall\nback to Basic auth, but then they're only paying the price for the\nenvironment they have.\n\nIf we aren't already reading the supported mechanisms out of the initial\n401, then we'll need the third request, but that would be silly and we\nshould just avoid doing that.\n\n> So perhaps somebody can come up with something clever, but I suspect we\n> may need to just revert this for the v2.32 release, and re-break the\n> case that 1b0d9545bb8 was trying to solve.\n\nYeah, I think this is the right solution for the problem until somebody\nwith a suitable mixed auth environment shows up and can test.  Your\npatches seemed reasonable and, as always, well explained.\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"},{"id":"424909","messageId":"xmqqbl971q8g.fsf@gitster.g","threadId":"55715","inReplyTo":"YKNeJ+NKUbD5ixA9@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] fix v2.32.0-rc0 bug with Negotiate / HTTP_NOAUTH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-19T01:12:15Z","receivedAt":"2021-05-19T01:12:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Here are some patches. The first one adds the tests, and I think is an\n> obvious improvement.\n>\n> The second one does the revert. I'd be quite happy if somebody wants to\n> figure out how to fix it in a way that addresses the original problem,\n> and then we can replace that. But in the meantime, I think it makes\n> sense to prepare the revert, as we wouldn't want to release v2.32.0 with\n> the bug.\n\nI'll queue these directly on top of the original topic, so that even\na merge of the branch by mistake down to maint won't break.\n\nI saw brian has already floated an idea to solve the original issue,\nand I'd encourage people to help finding a proper solution, but\nlet's leave the actual application of such a change for the next\ncycle.\n\nThanks.\n\n"},{"id":"424953","messageId":"YKT7Ynfc3F4egWwR@coredump.intra.peff.net","threadId":"55715","inReplyTo":"YKRYBLeIlgILfHFj@camp.crustytoothpaste.net","subject":"Re: Git 2.23.0-rc0 HTTP authentication failure - error message change","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-19T11:49:54Z","receivedAt":"2021-05-19T11:49:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 19, 2021 at 12:12:52AM +0000, brian m. carlson wrote:\n\n> > But imagine we didn't get a username/password in the URL. The first\n> > request will return REAUTH because of this moved code path (just as it\n> > would have before, because http.auth.{username,password} are not set).\n> > And then we'll get a credential from the user or from a helper and try\n> > again. But this time, if we fail, we'll return HTTP_REAUTH again! We\n> > never hit the \"if (http_auth.username && http_auth.password)\" check at\n> > all. And hence we never return HTTP_NOAUTH (which gives us the more\n> > useful \"authentication failed\" message), nor the credential_reject()\n> > line (which informs helpers to stop caching a known-bad password).\n> \n> I think what we'd want to do in this case is to only call HTTP_REAUTH if\n> we actually cleared CURLAUTH_GSSNEGOTIATE.  Maybe something like this:\n> [...]\n\nYeah, that was my instinct, too, but...\n\n> > I suspect we could hack around it by pessimistically guessing that\n> > GSSNEGOTIATE was the problem. But I'm worried that making that work\n> > would require up to three requests (one to find out we need auth, one to\n> > remove the GSSNEGOTIATE bit, and one to retry with a username/password).\n> > That seems like punishing people with servers that don't even care about\n> > Negotiate for no reason.\n> \n> I think my proposal above does that, but I'm not sure.  If Negotiate\n> wasn't set, we won't need to make a third request, since we'll have\n> known the supported mechanisms as part of the original 401.  If they do\n> support both, then three requests will be required if they have to fall\n> back to Basic auth, but then they're only paying the price for the\n> environment they have.\n> \n> If we aren't already reading the supported mechanisms out of the initial\n> 401, then we'll need the third request, but that would be silly and we\n> should just avoid doing that.\n\nYeah, I was worried that just clearing the bit results in the extra\nround-trip. I think we do clear bits based on what the other side showed\nus. That's the:\n\n  http_auth_methods &= results->auth_avail;\n\nin the code being discussed. But it seems like we'd want to do that as\npart of setting the \"used negotiate\" flag in your sample patch. I.e.,:\n\n  if (http_auth_methods & results->auth_avail & CURLAUTH_GSSNEGOTIATE)\n          used_negotiate = 1;\n\nBut it's entirely possible I don't understand the subtleties around\nunsetting GSSNEGOTIATE in the first place (it's not something I've ever\nused myself).\n\n> > So perhaps somebody can come up with something clever, but I suspect we\n> > may need to just revert this for the v2.32 release, and re-break the\n> > case that 1b0d9545bb8 was trying to solve.\n> \n> Yeah, I think this is the right solution for the problem until somebody\n> with a suitable mixed auth environment shows up and can test.  Your\n> patches seemed reasonable and, as always, well explained.\n\nThanks for taking a look!\n\n-Peff\n"},{"id":"424960","messageId":"43035278-5742-607a-57bd-971685deece8@gmail.com","threadId":"55715","inReplyTo":"YKNeXq3JzxYWkxKl@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] Revert \"remote-curl: fall back to basic auth if Negotiate fails\"","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-19T13:58:50Z","receivedAt":"2021-05-19T13:58:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/18/2021 2:27 AM, Jeff King wrote:\n> This reverts commit 1b0d9545bb85912a16b367229d414f55d140d3be.\n> \n> That commit does fix the situation it intended to (avoiding Negotiate\n> even when the credentials were provided in the URL), but it creates a\n> more serious regression: we now never hit the conditional for \"we had a\n> username and password, tried them, but the server still gave us a 401\".\n> That has two bad effects:\n> \n>  1. we never call credential_reject(), and thus a bogus credential\n>     stored by a helper will live on forever\n> \n>  2. we never return HTTP_NOAUTH, so the error message the user gets is\n>     \"The requested URL returned error: 401\", instead of \"Authentication\n>     failed\".\n> \n> Doing this correctly seems non-trivial, as we don't know whether the\n> Negotiate auth was a problem. Since this is a regression in the upcoming\n> v2.23.0 release (for which we're in -rc0), let's revert for now and work\n> on a fix separately.\n\nI think the revert is sufficiently justified here.\n \n> (Note that this isn't a pure revert; the previous commit added a test\n> showing the regression, so we can now flip it to expect_success).\n\nKeeping the test is excellent, because it gives us a way to confirm\nthat a second attempt at a fix is at least as good as the first.\n\nThe only thing that could improve this situation is to add a test\nthat checks the bug that the previous version introduced, so that\nthe next round doesn't repeat the mistake. That can be deferred\nbecause it is more important that we get this fix in time for the\nnext release candidate.\n\nThanks,\n-Stolee\n"},{"id":"424961","messageId":"YKUdRNH+uLBxAi+m@coredump.intra.peff.net","threadId":"55715","inReplyTo":"43035278-5742-607a-57bd-971685deece8@gmail.com","subject":"Re: [PATCH 2/2] Revert \"remote-curl: fall back to basic auth if Negotiate fails\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-19T14:14:28Z","receivedAt":"2021-05-19T14:15:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 19, 2021 at 09:58:50AM -0400, Derrick Stolee wrote:\n\n> > (Note that this isn't a pure revert; the previous commit added a test\n> > showing the regression, so we can now flip it to expect_success).\n> \n> Keeping the test is excellent, because it gives us a way to confirm\n> that a second attempt at a fix is at least as good as the first.\n> \n> The only thing that could improve this situation is to add a test\n> that checks the bug that the previous version introduced, so that\n> the next round doesn't repeat the mistake. That can be deferred\n> because it is more important that we get this fix in time for the\n> next release candidate.\n\nRe-reading what I wrote, I think \"the previous commit\" may be ambiguous.\nThe original commit which introduced the bug (and which we're reverting\nhere) didn't include a test at all. In patch 1/2 of this series (what\nI'm calling \"the previous commit\"), I provided a test which shows the\nregression. And now this revert shows that we fixed it (by flipping from\nexpect_failure to expect_success).\n\nSo I think I've already done what you're asking (if I understand it\ncorrectly).\n\nIt probably would be a little easier to follow by reverting first, and\nthen adding in the expect_success test on top to future-proof us. But by\ndoing it in the other order, it was easy to see the test demonstrate the\nbehavior before and after the revert.\n\n-Peff\n"},{"id":"424965","messageId":"f9a1a761-7536-c9b6-4837-d95e2e56a471@gmail.com","threadId":"55715","inReplyTo":"YKUdRNH+uLBxAi+m@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] Revert \"remote-curl: fall back to basic auth if Negotiate fails\"","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-19T14:53:47Z","receivedAt":"2021-05-19T14:53:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/19/2021 10:14 AM, Jeff King wrote:\n> On Wed, May 19, 2021 at 09:58:50AM -0400, Derrick Stolee wrote:\n> \n>>> (Note that this isn't a pure revert; the previous commit added a test\n>>> showing the regression, so we can now flip it to expect_success).\n>>\n>> Keeping the test is excellent, because it gives us a way to confirm\n>> that a second attempt at a fix is at least as good as the first.\n>>\n>> The only thing that could improve this situation is to add a test\n>> that checks the bug that the previous version introduced, so that\n>> the next round doesn't repeat the mistake. That can be deferred\n>> because it is more important that we get this fix in time for the\n>> next release candidate.\n> \n> Re-reading what I wrote, I think \"the previous commit\" may be ambiguous.\n> The original commit which introduced the bug (and which we're reverting\n> here) didn't include a test at all. In patch 1/2 of this series (what\n> I'm calling \"the previous commit\"), I provided a test which shows the\n> regression. And now this revert shows that we fixed it (by flipping from\n> expect_failure to expect_success).\n> \n> So I think I've already done what you're asking (if I understand it\n> correctly).\n\nAh. For some reason my email client didn't thread your messages\ntogether, so I saw this as a one-off patch (ignoring the 2/2 part\nof the message, of course).\n\nThanks,\n-Stolee\n"}]}