Re: [PATCH 1/2] http: only reject basic auth credentials once they have been tried

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 4, 2024, 22:47 UTC
Message-ID: <xmqqfry7onhf.fsf@gitster.g>
In-Reply-To: <20240204185427.39664-2-ypsah@devyard.org>

Quentin Bouget <ypsah@devyard.org> writes:

Show 16 quoted lines

>  	else if (results->http_code == 401) {
> -		if (http_auth.username && http_auth.password) {
> -			credential_reject(&http_auth);
> -			return HTTP_NOAUTH;
> -		} else {
> +		if ((http_auth_methods & CURLAUTH_GSSNEGOTIATE) == CURLAUTH_GSSNEGOTIATE) {
>  			http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
>  			if (results->auth_avail) {
>  				http_auth_methods &= results->auth_avail;
>				http_auth_methods_restricted = 1;
>			}
>			return HTTP_REAUTH;
>		}
> +		if (http_auth.username && http_auth.password)
> +			credential_reject(&http_auth);
> +		return HTTP_NOAUTH;

A few comments and questions.

 * GSSNEGOTIATE is a synonym for NEGOTIATE since cURL 7.38.0
   (released in Sep 2014); currently the earliest version we claim
   to support is 7.19.5 (released May 2009) without imap-send, and
   we require 7.34.0 (released Dec 2013) with imap-send, so for now,
   it is prudent that this patch uses GSSNEGOTIATE.

 * Is it something that the client code of libcURL can rely on that
   these CURLAUTH_FOO macros are bitmasks [*]?  If so, wouldn't

	if ((http_auth_methods & CURLAUTH_GSSNEGOTIATE))

   be clear enough (and less risk of making typo)?

 * When we see 401, the first thing we do in the new code is to see
   if GSS is enabled in auth_methods, and if so we drop it from
   auth_methods (to prevent us from trying it again) and say REAUTH.

   - What assures us that the presense of GSS bit in auth_methods
     mean we tried GSS to get this 401?  Could it be that we tried
     basic and seeing 401 from that, but we haven't tried GSS and we
     could retry with GSS now?  Is it commonly known that GSS is
     always tried first before Basic/Digest when both are availble,
     or something like that?

   - When auth_avail was given by the cURL library, we further limit
     the auth_methods (after dropping GSS) and say REAUTH.  This is
     not a new to the updated code, but can it happen that the
     resulting restricted auth_methods bitmap becomes empty (i.e.
     REAUTH would be useless)?

Thanks.

[References]

 * https://github.com/curl/curl/blob/b8c003832d730bb2f4b9de4204675ca5d9f7a903/include/curl/curl.h#L787C4-L787C64