diff mbox series

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

Message ID 20240204185427.39664-2-ypsah@devyard.org (mailing list archive)
State New, archived
Headers show
Series Fix gitlab's token-based authentication w/ kerberos | expand

Commit Message

Quentin Bouget Feb. 4, 2024, 6:54 p.m. UTC 
When CURLAUTH_GSSNEGOTIATE is enabled, it is currently assumed that
the provided username/password relate to a GSSAPI auth attempt.
In practice, forges such as gitlab can be deployed with HTTP basic auth
and GSSAPI auth both listening on the same port, meaning just because
the server supports GSSAPI and failed an authentication attempt using
the provided credentials, it does not mean the credentials are not valid
HTTP basic auth credentials.

This is documented as a long running bug here [1] and breaks token-based
authentication when the token is provided in the remote's URL itself.

This commit makes it so credentials are only dropped once they have been
tried both as GSSAPI credentials and HTTP basic auth credentials.

[1] https://gitlab.com/gitlab-org/gitlab/-/blob/b0e0d25646d1992fefda863febdcba8d4c7a1bbf/doc/integration/kerberos.md#L250

Signed-off-by: Quentin Bouget <ypsah@devyard.org>
---
 http.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Junio C Hamano Feb. 4, 2024, 10:47 p.m. UTC | #1 
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.

 * 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
Quentin Bouget Feb. 5, 2024, 3:03 a.m. UTC | #2 
On Sun Feb 4, 2024 at 11:47 PM CET, Junio C Hamano wrote:
> 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

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

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

>    - 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
Patrick Steinhardt Feb. 5, 2024, 5:47 a.m. UTC | #3 
On Sun, Feb 04, 2024 at 07:54:26PM +0100, Quentin Bouget wrote:
> When CURLAUTH_GSSNEGOTIATE is enabled, it is currently assumed that
> the provided username/password relate to a GSSAPI auth attempt.
> In practice, forges such as gitlab can be deployed with HTTP basic auth
> and GSSAPI auth both listening on the same port, meaning just because
> the server supports GSSAPI and failed an authentication attempt using
> the provided credentials, it does not mean the credentials are not valid
> HTTP basic auth credentials.
> 
> This is documented as a long running bug here [1] and breaks token-based
> authentication when the token is provided in the remote's URL itself.
> 
> This commit makes it so credentials are only dropped once they have been
> tried both as GSSAPI credentials and HTTP basic auth credentials.
> 
> [1] https://gitlab.com/gitlab-org/gitlab/-/blob/b0e0d25646d1992fefda863febdcba8d4c7a1bbf/doc/integration/kerberos.md#L250

Do you think it's feasible to add a test for this? We already have a
bunch of tests for authentication with Apache's httpd in t5563, so if we
could extend t/lib-httpd.sh to set up `mod_auth_gssapi` that would be
great.

I didn't try though, and it could just as well be that this would
require a full-fledged Kerberos setup, which would be a deal breaker I
guess. I ain't got enough familiarity with `mod_auth_gssapi` to tell.

Patrick

> Signed-off-by: Quentin Bouget <ypsah@devyard.org>
> ---
>  http.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/http.c b/http.c
> index e73b136e58..ccea19ac47 100644
> --- a/http.c
> +++ b/http.c
> @@ -1758,10 +1758,7 @@ static int handle_curl_result(struct slot_results *results)
>  	} else if (missing_target(results))
>  		return HTTP_MISSING_TARGET;
>  	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;
> @@ -1769,6 +1766,9 @@ static int handle_curl_result(struct slot_results *results)
>  			}
>  			return HTTP_REAUTH;
>  		}
> +		if (http_auth.username && http_auth.password)
> +			credential_reject(&http_auth);
> +		return HTTP_NOAUTH;
>  	} else {
>  		if (results->http_connectcode == 407)
>  			credential_reject(&proxy_auth);
> -- 
> 2.43.0
> 
>
diff mbox series

Patch

diff --git a/http.c b/http.c
index e73b136e58..ccea19ac47 100644
--- a/http.c
+++ b/http.c
@@ -1758,10 +1758,7 @@  static int handle_curl_result(struct slot_results *results)
 	} else if (missing_target(results))
 		return HTTP_MISSING_TARGET;
 	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;
@@ -1769,6 +1766,9 @@  static int handle_curl_result(struct slot_results *results)
 			}
 			return HTTP_REAUTH;
 		}
+		if (http_auth.username && http_auth.password)
+			credential_reject(&http_auth);
+		return HTTP_NOAUTH;
 	} else {
 		if (results->http_connectcode == 407)
 			credential_reject(&proxy_auth);