threads / patch / 60844

patch, 2 partshttp: only reject basic auth credentials once they have been tried

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

## tl;dr

15 messages between Feb 4, 2024 and Feb 5, 2024. Diffs are folded; open one to read it.

replies: 14people: 6as markdown or json

Quentin Bouget· Feb 4, 2024, 18:54 UTC · lore

[PATCH 0/2] Fix gitlab's token-based authentication w/ kerberos

Gitlab supports GSSAPI-based authentication with kerberos but when setup on port 443, token-based authentication with credentials provided in the remote's URL no longer works (cf. the note here [1]).

This patch series provides a fix which I tested against such a gitlab setup.

This is my first ever contribution to git, apologies in advance for any "faux pas".

[1] https://docs.gitlab.com/ee/integration/kerberos.html#http-git-access-with-kerberos-token-passwordless-authentication
Quentin Bouget (2):
  http: only reject basic auth credentials once they have been tried
  http: prevent redirect from dropping credentials during reauth
 http.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)
-- 
2.43.0
Quentin Bouget· Feb 4, 2024, 18:54 UTC · re: Quentin Bouget · lore

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(-)
Show changes to http.c +4 −4
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
Junio C Hamano· Feb 4, 2024, 22:47 UTC · re: Quentin Bouget · lore

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

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
Quentin Bouget· Feb 5, 2024, 03:03 UTC · re: Junio C Hamano · lore

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

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
Patrick Steinhardt· Feb 5, 2024, 05:47 UTC · re: Quentin Bouget · lore

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

On Sun, Feb 04, 2024 at 07:54:26PM +0100, Quentin Bouget wrote:
Show 15 quoted lines
> 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
Show 35 quoted lines
> 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
> 
>
Quentin Bouget· Feb 4, 2024, 18:54 UTC · re: Quentin Bouget · lore

[PATCH 2/2] http: prevent redirect from dropping credentials during reauth

During a re-authentication (second attempt at authenticating with a remote, e.g. after a failed GSSAPI attempt), git allows the remote to provide credential overrides in the redirect URL and unconditionnaly drops the current HTTP credentials in favors of those, even when there aren't any.

This commit makes it so HTTP credentials are only overridden when the redirect URL actually contains credentials itself.

Signed-off-by: Quentin Bouget <ypsah@devyard.org>
---
 http.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
Show changes to http.c +18 −0
diff --git a/http.c b/http.c
index ccea19ac47..caba9cac1e 100644
--- a/http.c
+++ b/http.c
@@ -2160,7 +2160,25 @@ static int http_request_reauth(const char *url,
 	if (options && options->effective_url && options->base_url) {
 		if (update_url_from_redirect(options->base_url,
 					     url, options->effective_url)) {
+			char *username = NULL, *password = NULL;
+
+			if (http_auth.username)
+				username = xstrdup(http_auth.username);
+			if (http_auth.password)
+				password = xstrdup(http_auth.password);
+
 			credential_from_url(&http_auth, options->base_url->buf);
+
+			if (http_auth.username)
+				free(username);
+			else if (username)
+				http_auth.username = username;
+
+			if (http_auth.password)
+				free(password);
+			else if (password)
+				http_auth.password = password;
+
 			url = options->effective_url->buf;
 		}
 	}

-- 
2.43.0
brian m. carlson· Feb 4, 2024, 22:36 UTC · re: Quentin Bouget · lore

Re: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth

On 2024-02-04 at 18:54:27, Quentin Bouget wrote:
Show 8 quoted lines
> During a re-authentication (second attempt at authenticating with a
> remote, e.g. after a failed GSSAPI attempt), git allows the remote to
> provide credential overrides in the redirect URL and unconditionnaly
> drops the current HTTP credentials in favors of those, even when there
> aren't any.
> 
> This commit makes it so HTTP credentials are only overridden when the
> redirect URL actually contains credentials itself.

I don't think your proposed change is safe. Credentials are supposed to be tied to a certain site and may even be tied to a specific repository, and if there's a redirect, then we need to re-fetch credentials or we could leak credentials to the wrong site by reusing them. Your change would therefore introduce a security vulnerability.

I should also point out that in general we are trying to make it less easy and less convenient for people to use credentials in the URL because that always necessitates insecure storage. There have in fact been proposals to remove that functionality entirely.

-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
Quentin Bouget· Feb 5, 2024, 03:01 UTC · re: brian m. carlson · lore

Re: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth

On Sun Feb 4, 2024 at 11:36 PM CET, brian m. carlson wrote:
Show 15 quoted lines
> On 2024-02-04 at 18:54:27, Quentin Bouget wrote:
> > During a re-authentication (second attempt at authenticating with a
> > remote, e.g. after a failed GSSAPI attempt), git allows the remote to
> > provide credential overrides in the redirect URL and unconditionnaly
> > drops the current HTTP credentials in favors of those, even when there
> > aren't any.
> > 
> > This commit makes it so HTTP credentials are only overridden when the
> > redirect URL actually contains credentials itself.
>
> I don't think your proposed change is safe.  Credentials are supposed to
> be tied to a certain site and may even be tied to a specific repository,
> and if there's a redirect, then we need to re-fetch credentials or we
> could leak credentials to the wrong site by reusing them.  Your change
> would therefore introduce a security vulnerability.
Good point, I had not considered the security implications.

I can see libcurl only reuses credentials after a redirect if the hostname has not changed: [1]

	By default, libcurl only sends credentials and Authentication
	headers to the initial hostname as given in the original URL, to
	avoid leaking username + password to other sites.

Does it sound OK if I use the credentials provided by the redirect when there are any (out of consistency with the current implementation), and only allow reusing the current credentials when the redirect and the original URLs share the same hostname?

If so, is there a helper to extract the hostname out of a URL in git? I can see credential.c does this itself, otherwise, libcurl provides its own helpers for this (curl_url(), curl_url_set(), curl_url_get()). [2]

> I should also point out that in general we are trying to make it less
> easy and less convenient for people to use credentials in the URL
> because that always necessitates insecure storage.  There have in fact
> been proposals to remove that functionality entirely.

Apologies, I feel like I may have given the impression I wanted to configure credentials in git's configuration files, which is not the case.

My use case is to `git push` a tag from a CI/CD pipeline to trigger a release, similar to how I do it here. [3]

Or is this the kind of use case you are trying to discourage?

[1] https://curl.se/libcurl/c/CURLOPT_UNRESTRICTED_AUTH.html [2] https://curl.se/libcurl/c/curl_url_get.html [3] https://gitlab.com/ypsah/simple-git-versioning/-/blob/main/.gitlab-ci.yml?ref_type=heads#L110

brian m. carlson· Feb 5, 2024, 22:18 UTC · re: Quentin Bouget · lore

Re: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth

On 2024-02-05 at 03:01:17, Quentin Bouget wrote:
Show 13 quoted lines
> Good point, I had not considered the security implications.
> 
> I can see libcurl only reuses credentials after a redirect if the
> hostname has not changed: [1]
> 
> 	By default, libcurl only sends credentials and Authentication
> 	headers to the initial hostname as given in the original URL, to
> 	avoid leaking username + password to other sites. 
> 
> Does it sound OK if I use the credentials provided by the redirect when
> there are any (out of consistency with the current implementation), and
> only allow reusing the current credentials when the redirect and the
> original URLs share the same hostname?

I don't think we can actually rely on that functionality because `credential.usehttppath` could actually have been set, in which case we'd need a different credential. For example, I know some forges issue certain types of tokens that are tied to a specific URL and wouldn't validate for a redirect, even if it were actually the same repo.

If there are credentials in the URL provided by the redirect, I think it should be safe to use them; otherwise, we'd need to rely on filling them with the credential protocol.

Show 8 quoted lines
> Apologies, I feel like I may have given the impression I wanted to
> configure credentials in git's configuration files, which is not the
> case.
> 
> My use case is to `git push` a tag from a CI/CD pipeline to trigger a
> release, similar to how I do it here. [3]
> 
> Or is this the kind of use case you are trying to discourage?

We're trying to discourage all use of credentials in the URL at the command line and in remote names/configuration files. If you want to pass in credentials from the environment, the Git FAQ explains how to do that[0], and that technique can be used in such a situation.

[0] https://git-scm.com/docs/gitfaq#http-credentials-environment
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
rsbecker@nexbridge.com· Feb 5, 2024, 22:52 UTC · re: brian m. carlson · lore

RE: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth

On Monday, February 5, 2024 5:18 PM, brian m. carlson wrote:
Show 40 quoted lines
>On 2024-02-05 at 03:01:17, Quentin Bouget wrote:
>> Good point, I had not considered the security implications.
>>
>> I can see libcurl only reuses credentials after a redirect if the
>> hostname has not changed: [1]
>>
>> 	By default, libcurl only sends credentials and Authentication
>> 	headers to the initial hostname as given in the original URL, to
>> 	avoid leaking username + password to other sites.
>>
>> Does it sound OK if I use the credentials provided by the redirect
>> when there are any (out of consistency with the current
>> implementation), and only allow reusing the current credentials when
>> the redirect and the original URLs share the same hostname?
>
>I don't think we can actually rely on that functionality because
>`credential.usehttppath` could actually have been set, in which case we'd need a
>different credential.  For example, I know some forges issue certain types of tokens
>that are tied to a specific URL and wouldn't validate for a redirect, even if it were
>actually the same repo.
>
>If there are credentials in the URL provided by the redirect, I think it should be safe
>to use them; otherwise, we'd need to rely on filling them with the credential
>protocol.
>
>> Apologies, I feel like I may have given the impression I wanted to
>> configure credentials in git's configuration files, which is not the
>> case.
>>
>> My use case is to `git push` a tag from a CI/CD pipeline to trigger a
>> release, similar to how I do it here. [3]
>>
>> Or is this the kind of use case you are trying to discourage?
>
>We're trying to discourage all use of credentials in the URL at the command line and
>in remote names/configuration files.  If you want to pass in credentials from the
>environment, the Git FAQ explains how to do that[0], and that technique can be
>used in such a situation.
>
>[0] https://git-scm.com/docs/gitfaq#http-credentials-environment
A common side-use case (not directly in git) for this situation is to attempt to use curl (or libcurl) to create a Pull Request via the GitHub (or other enterprise git server) CLI or POST. This is most often done via REST rather than supplying via the URL. It does remove the need to pass some credentials (a.k.a. the API token) via the URL as the API token gets injected into the JSON content - this may have been the original motivation as many of the servers do redirects. However, they do not reprocess or inject different credentials. I am wonder about the specific use case is for this situation and why a redirect injects a credential change, which I cannot see is a good thing.
--Randall
Junio C Hamano· Feb 4, 2024, 22:51 UTC · re: Quentin Bouget · lore

Re: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth

Quentin Bouget <ypsah@devyard.org> writes:
Show 8 quoted lines
> During a re-authentication (second attempt at authenticating with a
> remote, e.g. after a failed GSSAPI attempt), git allows the remote to
> provide credential overrides in the redirect URL and unconditionnaly
> drops the current HTTP credentials in favors of those, even when there
> aren't any.
>
> This commit makes it so HTTP credentials are only overridden when the
> redirect URL actually contains credentials itself.
"This commit makes it so" -> "Make it so"
Show 6 quoted lines
> +			char *username = NULL, *password = NULL;
> +
> +			if (http_auth.username)
> +				username = xstrdup(http_auth.username);
> +			if (http_auth.password)
> +				password = xstrdup(http_auth.password);

Not a huge deal, but we have xstrdup_or_null() helper function exactly for a use case like this.

Show 11 quoted lines
>  			credential_from_url(&http_auth, options->base_url->buf);
> +
> +			if (http_auth.username)
> +				free(username);
> +			else if (username)
> +				http_auth.username = username;
> +
> +			if (http_auth.password)
> +				free(password);
> +			else if (password)
> +				http_auth.password = password;

This is an interesting change. I wonder what breaks if we completely ignored such credential materials forced by the remote via a redirect?

>  			url = options->effective_url->buf;
>  		}
>  	}
Thanks.
Quentin Bouget· Feb 5, 2024, 03:06 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth

On Sun Feb 4, 2024 at 11:51 PM CET, Junio C Hamano wrote:
Show 12 quoted lines
> Quentin Bouget <ypsah@devyard.org> writes:
>
> > During a re-authentication (second attempt at authenticating with a
> > remote, e.g. after a failed GSSAPI attempt), git allows the remote to
> > provide credential overrides in the redirect URL and unconditionnaly
> > drops the current HTTP credentials in favors of those, even when there
> > aren't any.
> >
> > This commit makes it so HTTP credentials are only overridden when the
> > redirect URL actually contains credentials itself.
>
> "This commit makes it so" -> "Make it so"
Will change.
Show 9 quoted lines
> > +			char *username = NULL, *password = NULL;
> > +
> > +			if (http_auth.username)
> > +				username = xstrdup(http_auth.username);
> > +			if (http_auth.password)
> > +				password = xstrdup(http_auth.password);
>
> Not a huge deal, but we have xstrdup_or_null() helper function
> exactly for a use case like this.
Thanks, will change.
Show 15 quoted lines
> >  			credential_from_url(&http_auth, options->base_url->buf);
> > +
> > +			if (http_auth.username)
> > +				free(username);
> > +			else if (username)
> > +				http_auth.username = username;
> > +
> > +			if (http_auth.password)
> > +				free(password);
> > +			else if (password)
> > +				http_auth.password = password;
>
> This is an interesting change.  I wonder what breaks if we
> completely ignored such credential materials forced by the remote
> via a redirect?

Me too. Maybe the original author would know. Is it OK to Cc them in this case?

> >  			url = options->effective_url->buf;
> >  		}
> >  	}

Thanks, Quentin

rsbecker@nexbridge.com· Feb 4, 2024, 23:01 UTC · re: Quentin Bouget · lore

RE: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth

On Sunday, February 4, 2024 1:54 PM, Quentin Bouget wrote:
>During a re-authentication (second attempt at authenticating with a remote,
e.g.
>after a failed GSSAPI attempt), git allows the remote to provide credential
overrides
>in the redirect URL and unconditionnaly drops the current HTTP credentials
in favors
>of those, even when there aren't any.
>
>This commit makes it so HTTP credentials are only overridden when the
redirect URL
Show 23 quoted lines
>actually contains credentials itself.
>
>Signed-off-by: Quentin Bouget <ypsah@devyard.org>
>---
> http.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
>diff --git a/http.c b/http.c
>index ccea19ac47..caba9cac1e 100644
>--- a/http.c
>+++ b/http.c
>@@ -2160,7 +2160,25 @@ static int http_request_reauth(const char *url,
> 	if (options && options->effective_url && options->base_url) {
> 		if (update_url_from_redirect(options->base_url,
> 					     url, options->effective_url)) {
>+			char *username = NULL, *password = NULL;
>+
>+			if (http_auth.username)
>+				username = xstrdup(http_auth.username);
>+			if (http_auth.password)
>+				password = xstrdup(http_auth.password);
>+
> 			credential_from_url(&http_auth,
options->base_url->buf);
Show 14 quoted lines
>+
>+			if (http_auth.username)
>+				free(username);
>+			else if (username)
>+				http_auth.username = username;
>+
>+			if (http_auth.password)
>+				free(password);
>+			else if (password)
>+				http_auth.password = password;
>+
> 			url = options->effective_url->buf;
> 		}
> 	}

I am wondering whether this is a good idea. Having credentials in a redirect seems like it might be a vector for going somewhere other than what you want to do, with credentials you do not necessarily want. Others might no better than I on this, but would potentially lead to a CVE? I would prefer to see credentials in a redirect rejected rather than used. --Randall

Quentin Bouget· Feb 5, 2024, 03:12 UTC · re: rsbecker@nexbridge.com · lore

Re: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth

On Mon Feb 5, 2024 at 12:01 AM CET,  wrote:
Show 50 quoted lines
> On Sunday, February 4, 2024 1:54 PM, Quentin Bouget wrote:
> > During a re-authentication (second attempt at authenticating with a remote, e.g.
> > after a failed GSSAPI attempt), git allows the remote to provide credential overrides
> > in the redirect URL and unconditionnaly drops the current HTTP credentials in favors
> > of those, even when there aren't any.
> >
> > This commit makes it so HTTP credentials are only overridden when the redirect URL
> > actually contains credentials itself.
> >
> > Signed-off-by: Quentin Bouget <ypsah@devyard.org>
> > ---
> > http.c | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/http.c b/http.c
> > index ccea19ac47..caba9cac1e 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -2160,7 +2160,25 @@ static int http_request_reauth(const char *url,
> >  	if (options && options->effective_url && options->base_url) {
> >  		if (update_url_from_redirect(options->base_url,
> >  					     url, options->effective_url)) {
> > +			char *username = NULL, *password = NULL;
> > +
> > +			if (http_auth.username)
> > +				username = xstrdup(http_auth.username);
> > +			if (http_auth.password)
> > +				password = xstrdup(http_auth.password);
> > +
> >  			credential_from_url(&http_auth, options->base_url->buf);
> > +
> > +			if (http_auth.username)
> > +				free(username);
> > +			else if (username)
> > +				http_auth.username = username;
> > +
> > +			if (http_auth.password)
> > +				free(password);
> > +			else if (password)
> > +				http_auth.password = password;
> > +
> >  			url = options->effective_url->buf;
> >  		}
> >  	}
>
> I am wondering whether this is a good idea. Having credentials in a redirect
> seems like it might be a vector for going somewhere other than what you want
> to do, with credentials you do not necessarily want. Others might no better
> than I on this, but would potentially lead to a CVE? I would prefer to see
> credentials in a redirect rejected rather than used.
I guess this can be controlled by setting http.followRedirects to false.

I am not sure there is generally more danger in following a redirect URL with or without the provided credentials. Note that this is also how git currently behaves and while I am not aware of any concrete use case myself, I am also not confident there isn't any.

That being said, Brian M. Carlson pointed out that reusing credentials from the initial URL for the redirect URL is quite dangerous. In my reply, I have suggested to switch to implementing the same behaviour as libcurl when it comes to reusing credentials: if the hostname of the redirect is the same as the original URL, reuse the credentials, otherwise drop them. [1] I would still retain the (seemingly strange) current behaviour of favoring credentials in the redirect URL over anything else.

Thanks, Quentin

[1] https://curl.se/libcurl/c/CURLOPT_UNRESTRICTED_AUTH.html
Robert Coup· Feb 5, 2024, 09:22 UTC · re: Quentin Bouget · lore

Re: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth

Hi Quentin,
>  I have suggested to switch to implementing the same behaviour as libcurl when it comes to reusing credentials: if the hostname of the redirect is the same as the original URL, reuse the credentials, otherwise drop them.

The protocol & port number also need to match, so it doesn't end up the same as CVE-2022-27774 [1]

Rob :)
[1] https://curl.se/docs/CVE-2022-27774.html

← back to recent threads