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:
signature.asc
Description: PGP signature
