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

From: Quentin Bouget <ypsah@devyard.org>
Date: Feb 5, 2024, 03:03 UTC
Message-ID: <CYWT85QZ5IBD.3CWNWYQTUMTDJ@devyard.org>
In-Reply-To: <xmqqfry7onhf.fsf@gitster.g>

On Sun Feb 4, 2024 at 11:47 PM CET, Junio C Hamano wrote:

Show 26 quoted lines

> Quentin Bouget <ypsah@devyard.org> writes:
>
> >  	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.

Agreed

Show 10 quoted lines

>  * 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)?
>
> [References]
>
>  * https://github.com/curl/curl/blob/b8c003832d730bb2f4b9de4204675ca5d9f7a903/include/curl/curl.h#L787C4-L787C64

I had not considered the risk of typos, fair enough. Will change.

Show 10 quoted lines

>  * 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?

libcurl's documentation on CURLOPT_HTTPAUTH says:

	If more than one bit is set, libcurl first queries the host to
	see which authentication methods it supports and then picks the
	best one you allow it to use.

And then:

	CURLAUTH_NEGOTIATE

		HTTP Negotiate (SPNEGO) authentication. Negotiate
		authentication is defined in RFC 4559 and is the most
		secure way to perform authentication over HTTP.

Which hints at CURLAUTH_NEGOTIATE (aka. GSSNEGOTIATE as you pointed out) being the prefered auth method.

The current implementation confirms this. [1]

Show 5 quoted lines

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

I am not familiar enough with SPNEGO to say. Seems plausible. Should I instead do:

	return (http_auth_methods & CURLAUTH_ANY) ? HTTP_REAUTH : HTTP_NOAUTH;

Thanks, Quentin

[1] https://github.com/curl/curl/blob/b8c003832d730bb2f4b9de4204675ca5d9f7a903/lib/http.c#L373