Thread (10 messages) flat view 10 messages, 4 authors, 2021-05-19

Re: Git 2.23.0-rc0 HTTP authentication failure - error message change

From: brian m. carlson <hidden>
Date: 2021-05-19 00:12:59
Subsystem: the rest · Maintainer: Linus Torvalds

On 2021-05-18 at 05:50:26, Jeff King wrote:
quoted hunk ↗ jump to 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:
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

Attachments

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help