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

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

[Quentin Bouget]

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

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

[Quentin Bouget]

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

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

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

[Junio C Hamano]

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

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

[Quentin Bouget]

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

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2579 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Quentin Bouget]

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;
 		}
 	}
-- 
2.43.0

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Quentin Bouget]

On Sun Feb 4, 2024 at 11:36 PM CET, brian m. carlson wrote:
> 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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1967 bytes --]

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
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[rsbecker]

On Monday, February 5, 2024 5:18 PM, brian m. carlson wrote:
>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

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

[Junio C Hamano]

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"

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

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

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

[Quentin Bouget]

On Sun Feb 4, 2024 at 11:51 PM CET, Junio C Hamano wrote:
> 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.

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

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

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

[rsbecker]

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

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

[Quentin Bouget]

On Mon Feb 5, 2024 at 12:01 AM CET,  wrote:
> 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

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

[Robert Coup]

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