From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 05 Feb 2024 05:47:48 GMT
Subject: Re: [PATCH 1/2] http: only reject basic auth credentials once they have been tried
Message-ID: <ZcB2hHBoyiBdOw81@tanuki>
In-Reply-To: <20240204185427.39664-2-ypsah@devyard.org>

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