git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "brian m. carlson" <sandals@crustytoothpaste.net>
To: Jeff King <peff@peff.net>
Cc: Ben Humphreys <behumphreys@atlassian.com>,
	Junio C Hamano <gitster@pobox.com>,
	Christopher Schenk <christopher@cschenk.net>,
	git@vger.kernel.org
Subject: Re: Git 2.23.0-rc0 HTTP authentication failure - error message change
Date: Wed, 19 May 2021 00:12:52 +0000	[thread overview]
Message-ID: <YKRYBLeIlgILfHFj@camp.crustytoothpaste.net> (raw)
In-Reply-To: <YKNVop80H8xSTCjz@coredump.intra.peff.net>

[-- 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 --]

  parent reply	other threads:[~2021-05-19  0:12 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-18  3:07 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   ` brian m. carlson [this message]
2021-05-19 11:49     ` Git 2.23.0-rc0 HTTP authentication failure - error message change Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YKRYBLeIlgILfHFj@camp.crustytoothpaste.net \
    --to=sandals@crustytoothpaste.net \
    --cc=behumphreys@atlassian.com \
    --cc=christopher@cschenk.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --subject='Re: Git 2.23.0-rc0 HTTP authentication failure - error message change' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).