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

["Quentin Bouget" <ypsah@devyard.org>]

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