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@xxxxxxxxxx | 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
