{"thread":{"id":"60844","subject":"[PATCH 1/2] http: only reject basic auth credentials once they have been tried","startedAt":"2024-02-04T19:10:42Z","lastAt":"2024-02-05T22:52:32Z","messageCount":15,"participants":["Quentin Bouget","brian m. carlson","Junio C Hamano","rsbecker@nexbridge.com","Patrick Steinhardt","Robert Coup"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"487898","messageId":"20240204185427.39664-2-ypsah@devyard.org","threadId":"60844","inReplyTo":"20240204185427.39664-1-ypsah@devyard.org","subject":"[PATCH 1/2] http: only reject basic auth credentials once they have been tried","fromName":"Quentin Bouget","fromEmail":"ypsah@devyard.org","sentAt":"2024-02-04T18:54:26Z","receivedAt":"2024-02-04T19:10:42Z","isPatch":true,"sender":{"key":"ypsah@devyard.org","avatar":null},"body":"When CURLAUTH_GSSNEGOTIATE is enabled, it is currently assumed that\nthe provided username/password relate to a GSSAPI auth attempt.\nIn practice, forges such as gitlab can be deployed with HTTP basic auth\nand GSSAPI auth both listening on the same port, meaning just because\nthe server supports GSSAPI and failed an authentication attempt using\nthe provided credentials, it does not mean the credentials are not valid\nHTTP basic auth credentials.\n\nThis is documented as a long running bug here [1] and breaks token-based\nauthentication when the token is provided in the remote's URL itself.\n\nThis commit makes it so credentials are only dropped once they have been\ntried both as GSSAPI credentials and HTTP basic auth credentials.\n\n[1] https://gitlab.com/gitlab-org/gitlab/-/blob/b0e0d25646d1992fefda863febdcba8d4c7a1bbf/doc/integration/kerberos.md#L250\n\nSigned-off-by: Quentin Bouget <ypsah@devyard.org>\n---\n http.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex e73b136e58..ccea19ac47 100644\n--- a/http.c\n+++ b/http.c\n@@ -1758,10 +1758,7 @@ static int handle_curl_result(struct slot_results *results)\n \t} else if (missing_target(results))\n \t\treturn HTTP_MISSING_TARGET;\n \telse if (results->http_code == 401) {\n-\t\tif (http_auth.username && http_auth.password) {\n-\t\t\tcredential_reject(&http_auth);\n-\t\t\treturn HTTP_NOAUTH;\n-\t\t} else {\n+\t\tif ((http_auth_methods & CURLAUTH_GSSNEGOTIATE) == CURLAUTH_GSSNEGOTIATE) {\n \t\t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n \t\t\tif (results->auth_avail) {\n \t\t\t\thttp_auth_methods &= results->auth_avail;\n@@ -1769,6 +1766,9 @@ static int handle_curl_result(struct slot_results *results)\n \t\t\t}\n \t\t\treturn HTTP_REAUTH;\n \t\t}\n+\t\tif (http_auth.username && http_auth.password)\n+\t\t\tcredential_reject(&http_auth);\n+\t\treturn HTTP_NOAUTH;\n \t} else {\n \t\tif (results->http_connectcode == 407)\n \t\t\tcredential_reject(&proxy_auth);\n-- \n2.43.0\n\n"},{"id":"487899","messageId":"20240204185427.39664-1-ypsah@devyard.org","threadId":"60844","inReplyTo":null,"subject":"[PATCH 0/2] Fix gitlab's token-based authentication w/ kerberos","fromName":"Quentin Bouget","fromEmail":"ypsah@devyard.org","sentAt":"2024-02-04T18:54:25Z","receivedAt":"2024-02-04T20:10:50Z","isPatch":true,"sender":{"key":"ypsah@devyard.org","avatar":null},"body":"Gitlab supports GSSAPI-based authentication with kerberos but when setup\non port 443, token-based authentication with credentials provided in the\nremote's URL no longer works (cf. the note here [1]).\n\nThis patch series provides a fix which I tested against such a gitlab\nsetup.\n\nThis is my first ever contribution to git, apologies in advance for any\n\"faux pas\".\n\n[1] https://docs.gitlab.com/ee/integration/kerberos.html#http-git-access-with-kerberos-token-passwordless-authentication\n\nQuentin Bouget (2):\n  http: only reject basic auth credentials once they have been tried\n  http: prevent redirect from dropping credentials during reauth\n\n http.c | 26 ++++++++++++++++++++++----\n 1 file changed, 22 insertions(+), 4 deletions(-)\n\n-- \n2.43.0\n\n"},{"id":"487901","messageId":"20240204185427.39664-3-ypsah@devyard.org","threadId":"60844","inReplyTo":"20240204185427.39664-1-ypsah@devyard.org","subject":"[PATCH 2/2] http: prevent redirect from dropping credentials during reauth","fromName":"Quentin Bouget","fromEmail":"ypsah@devyard.org","sentAt":"2024-02-04T18:54:27Z","receivedAt":"2024-02-04T21:59:20Z","isPatch":true,"sender":{"key":"ypsah@devyard.org","avatar":null},"body":"During a re-authentication (second attempt at authenticating with a\nremote, e.g. after a failed GSSAPI attempt), git allows the remote to\nprovide credential overrides in the redirect URL and unconditionnaly\ndrops the current HTTP credentials in favors of those, even when there\naren't any.\n\nThis commit makes it so HTTP credentials are only overridden when the\nredirect URL actually contains credentials itself.\n\nSigned-off-by: Quentin Bouget <ypsah@devyard.org>\n---\n http.c | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex ccea19ac47..caba9cac1e 100644\n--- a/http.c\n+++ b/http.c\n@@ -2160,7 +2160,25 @@ static int http_request_reauth(const char *url,\n \tif (options && options->effective_url && options->base_url) {\n \t\tif (update_url_from_redirect(options->base_url,\n \t\t\t\t\t     url, options->effective_url)) {\n+\t\t\tchar *username = NULL, *password = NULL;\n+\n+\t\t\tif (http_auth.username)\n+\t\t\t\tusername = xstrdup(http_auth.username);\n+\t\t\tif (http_auth.password)\n+\t\t\t\tpassword = xstrdup(http_auth.password);\n+\n \t\t\tcredential_from_url(&http_auth, options->base_url->buf);\n+\n+\t\t\tif (http_auth.username)\n+\t\t\t\tfree(username);\n+\t\t\telse if (username)\n+\t\t\t\thttp_auth.username = username;\n+\n+\t\t\tif (http_auth.password)\n+\t\t\t\tfree(password);\n+\t\t\telse if (password)\n+\t\t\t\thttp_auth.password = password;\n+\n \t\t\turl = options->effective_url->buf;\n \t\t}\n \t}\n-- \n2.43.0\n\n"},{"id":"487903","messageId":"ZcARb4YNCD4NLJku@tapette.crustytoothpaste.net","threadId":"60844","inReplyTo":"20240204185427.39664-3-ypsah@devyard.org","subject":"Re: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-02-04T22:36:31Z","receivedAt":"2024-02-04T22:36:39Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-02-04 at 18:54:27, Quentin Bouget wrote:\n> During a re-authentication (second attempt at authenticating with a\n> remote, e.g. after a failed GSSAPI attempt), git allows the remote to\n> provide credential overrides in the redirect URL and unconditionnaly\n> drops the current HTTP credentials in favors of those, even when there\n> aren't any.\n> \n> This commit makes it so HTTP credentials are only overridden when the\n> redirect URL actually contains credentials itself.\n\nI don't think your proposed change is safe.  Credentials are supposed to\nbe tied to a certain site and may even be tied to a specific repository,\nand if there's a redirect, then we need to re-fetch credentials or we\ncould leak credentials to the wrong site by reusing them.  Your change\nwould therefore introduce a security vulnerability.\n\nI should also point out that in general we are trying to make it less\neasy and less convenient for people to use credentials in the URL\nbecause that always necessitates insecure storage.  There have in fact\nbeen proposals to remove that functionality entirely.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"487904","messageId":"xmqqfry7onhf.fsf@gitster.g","threadId":"60844","inReplyTo":"20240204185427.39664-2-ypsah@devyard.org","subject":"Re: [PATCH 1/2] http: only reject basic auth credentials once they have been tried","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-04T22:47:56Z","receivedAt":"2024-02-04T22:48:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Quentin Bouget <ypsah@devyard.org> writes:\n\n>  \telse if (results->http_code == 401) {\n> -\t\tif (http_auth.username && http_auth.password) {\n> -\t\t\tcredential_reject(&http_auth);\n> -\t\t\treturn HTTP_NOAUTH;\n> -\t\t} else {\n> +\t\tif ((http_auth_methods & CURLAUTH_GSSNEGOTIATE) == CURLAUTH_GSSNEGOTIATE) {\n>  \t\t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n>  \t\t\tif (results->auth_avail) {\n>  \t\t\t\thttp_auth_methods &= results->auth_avail;\n>\t\t\t\thttp_auth_methods_restricted = 1;\n>\t\t\t}\n>\t\t\treturn HTTP_REAUTH;\n>\t\t}\n> +\t\tif (http_auth.username && http_auth.password)\n> +\t\t\tcredential_reject(&http_auth);\n> +\t\treturn HTTP_NOAUTH;\n\nA few comments and questions.\n\n * GSSNEGOTIATE is a synonym for NEGOTIATE since cURL 7.38.0\n   (released in Sep 2014); currently the earliest version we claim\n   to support is 7.19.5 (released May 2009) without imap-send, and\n   we require 7.34.0 (released Dec 2013) with imap-send, so for now,\n   it is prudent that this patch uses GSSNEGOTIATE.\n\n * Is it something that the client code of libcURL can rely on that\n   these CURLAUTH_FOO macros are bitmasks [*]?  If so, wouldn't\n\n\tif ((http_auth_methods & CURLAUTH_GSSNEGOTIATE))\n\n   be clear enough (and less risk of making typo)?\n\n * When we see 401, the first thing we do in the new code is to see\n   if GSS is enabled in auth_methods, and if so we drop it from\n   auth_methods (to prevent us from trying it again) and say REAUTH.\n\n   - What assures us that the presense of GSS bit in auth_methods\n     mean we tried GSS to get this 401?  Could it be that we tried\n     basic and seeing 401 from that, but we haven't tried GSS and we\n     could retry with GSS now?  Is it commonly known that GSS is\n     always tried first before Basic/Digest when both are availble,\n     or something like that?\n\n   - When auth_avail was given by the cURL library, we further limit\n     the auth_methods (after dropping GSS) and say REAUTH.  This is\n     not a new to the updated code, but can it happen that the\n     resulting restricted auth_methods bitmap becomes empty (i.e.\n     REAUTH would be useless)?\n\n\nThanks.\n\n[References]\n\n * https://github.com/curl/curl/blob/b8c003832d730bb2f4b9de4204675ca5d9f7a903/include/curl/curl.h#L787C4-L787C64\n"},{"id":"487905","messageId":"xmqq8r3zonat.fsf@gitster.g","threadId":"60844","inReplyTo":"20240204185427.39664-3-ypsah@devyard.org","subject":"Re: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-04T22:51:54Z","receivedAt":"2024-02-04T22:51:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Quentin Bouget <ypsah@devyard.org> writes:\n\n> During a re-authentication (second attempt at authenticating with a\n> remote, e.g. after a failed GSSAPI attempt), git allows the remote to\n> provide credential overrides in the redirect URL and unconditionnaly\n> drops the current HTTP credentials in favors of those, even when there\n> aren't any.\n>\n> This commit makes it so HTTP credentials are only overridden when the\n> redirect URL actually contains credentials itself.\n\n\"This commit makes it so\" -> \"Make it so\"\n\n> +\t\t\tchar *username = NULL, *password = NULL;\n> +\n> +\t\t\tif (http_auth.username)\n> +\t\t\t\tusername = xstrdup(http_auth.username);\n> +\t\t\tif (http_auth.password)\n> +\t\t\t\tpassword = xstrdup(http_auth.password);\n\nNot a huge deal, but we have xstrdup_or_null() helper function\nexactly for a use case like this.\n\n>  \t\t\tcredential_from_url(&http_auth, options->base_url->buf);\n> +\n> +\t\t\tif (http_auth.username)\n> +\t\t\t\tfree(username);\n> +\t\t\telse if (username)\n> +\t\t\t\thttp_auth.username = username;\n> +\n> +\t\t\tif (http_auth.password)\n> +\t\t\t\tfree(password);\n> +\t\t\telse if (password)\n> +\t\t\t\thttp_auth.password = password;\n\nThis is an interesting change.  I wonder what breaks if we\ncompletely ignored such credential materials forced by the remote\nvia a redirect?\n\n>  \t\t\turl = options->effective_url->buf;\n>  \t\t}\n>  \t}\n\nThanks.\n\n"},{"id":"487906","messageId":"017a01da57be$28748aa0$795d9fe0$@nexbridge.com","threadId":"60844","inReplyTo":"20240204185427.39664-3-ypsah@devyard.org","subject":"RE: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-02-04T23:01:53Z","receivedAt":"2024-02-04T23:02:14Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Sunday, February 4, 2024 1:54 PM, Quentin Bouget wrote:\n>During a re-authentication (second attempt at authenticating with a remote,\ne.g.\n>after a failed GSSAPI attempt), git allows the remote to provide credential\noverrides\n>in the redirect URL and unconditionnaly drops the current HTTP credentials\nin favors\n>of those, even when there aren't any.\n>\n>This commit makes it so HTTP credentials are only overridden when the\nredirect URL\n>actually contains credentials itself.\n>\n>Signed-off-by: Quentin Bouget <ypsah@devyard.org>\n>---\n> http.c | 18 ++++++++++++++++++\n> 1 file changed, 18 insertions(+)\n>\n>diff --git a/http.c b/http.c\n>index ccea19ac47..caba9cac1e 100644\n>--- a/http.c\n>+++ b/http.c\n>@@ -2160,7 +2160,25 @@ static int http_request_reauth(const char *url,\n> \tif (options && options->effective_url && options->base_url) {\n> \t\tif (update_url_from_redirect(options->base_url,\n> \t\t\t\t\t     url, options->effective_url)) {\n>+\t\t\tchar *username = NULL, *password = NULL;\n>+\n>+\t\t\tif (http_auth.username)\n>+\t\t\t\tusername = xstrdup(http_auth.username);\n>+\t\t\tif (http_auth.password)\n>+\t\t\t\tpassword = xstrdup(http_auth.password);\n>+\n> \t\t\tcredential_from_url(&http_auth,\noptions->base_url->buf);\n>+\n>+\t\t\tif (http_auth.username)\n>+\t\t\t\tfree(username);\n>+\t\t\telse if (username)\n>+\t\t\t\thttp_auth.username = username;\n>+\n>+\t\t\tif (http_auth.password)\n>+\t\t\t\tfree(password);\n>+\t\t\telse if (password)\n>+\t\t\t\thttp_auth.password = password;\n>+\n> \t\t\turl = options->effective_url->buf;\n> \t\t}\n> \t}\n\nI am wondering whether this is a good idea. Having credentials in a redirect\nseems like it might be a vector for going somewhere other than what you want\nto do, with credentials you do not necessarily want. Others might no better\nthan I on this, but would potentially lead to a CVE? I would prefer to see\ncredentials in a redirect rejected rather than used.\n--Randall\n\n"},{"id":"487911","messageId":"CYWT6JWQCTFG.106OCJTV3NQDU@devyard.org","threadId":"60844","inReplyTo":"ZcARb4YNCD4NLJku@tapette.crustytoothpaste.net","subject":"Re: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth","fromName":"Quentin Bouget","fromEmail":"ypsah@devyard.org","sentAt":"2024-02-05T03:01:17Z","receivedAt":"2024-02-05T03:01:37Z","isPatch":true,"sender":{"key":"ypsah@devyard.org","avatar":null},"body":"On Sun Feb 4, 2024 at 11:36 PM CET, brian m. carlson wrote:\n> On 2024-02-04 at 18:54:27, Quentin Bouget wrote:\n> > During a re-authentication (second attempt at authenticating with a\n> > remote, e.g. after a failed GSSAPI attempt), git allows the remote to\n> > provide credential overrides in the redirect URL and unconditionnaly\n> > drops the current HTTP credentials in favors of those, even when there\n> > aren't any.\n> > \n> > This commit makes it so HTTP credentials are only overridden when the\n> > redirect URL actually contains credentials itself.\n>\n> I don't think your proposed change is safe.  Credentials are supposed to\n> be tied to a certain site and may even be tied to a specific repository,\n> and if there's a redirect, then we need to re-fetch credentials or we\n> could leak credentials to the wrong site by reusing them.  Your change\n> would therefore introduce a security vulnerability.\n\nGood point, I had not considered the security implications.\n\nI can see libcurl only reuses credentials after a redirect if the\nhostname has not changed: [1]\n\n\tBy default, libcurl only sends credentials and Authentication\n\theaders to the initial hostname as given in the original URL, to\n\tavoid leaking username + password to other sites. \n\nDoes it sound OK if I use the credentials provided by the redirect when\nthere are any (out of consistency with the current implementation), and\nonly allow reusing the current credentials when the redirect and the\noriginal URLs share the same hostname?\n\nIf so, is there a helper to extract the hostname out of a URL in git?\nI can see credential.c does this itself, otherwise, libcurl provides its\nown helpers for this (curl_url(), curl_url_set(), curl_url_get()). [2]\n\n> I should also point out that in general we are trying to make it less\n> easy and less convenient for people to use credentials in the URL\n> because that always necessitates insecure storage.  There have in fact\n> been proposals to remove that functionality entirely.\n\nApologies, I feel like I may have given the impression I wanted to\nconfigure credentials in git's configuration files, which is not the\ncase.\n\nMy use case is to `git push` a tag from a CI/CD pipeline to trigger a\nrelease, similar to how I do it here. [3]\n\nOr is this the kind of use case you are trying to discourage?\n\n[1] https://curl.se/libcurl/c/CURLOPT_UNRESTRICTED_AUTH.html\n[2] https://curl.se/libcurl/c/curl_url_get.html\n[3] https://gitlab.com/ypsah/simple-git-versioning/-/blob/main/.gitlab-ci.yml?ref_type=heads#L110\n"},{"id":"487912","messageId":"CYWTAIFR6W5W.1GH1O6KKP0U5U@devyard.org","threadId":"60844","inReplyTo":"xmqq8r3zonat.fsf@gitster.g","subject":"Re: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth","fromName":"Quentin Bouget","fromEmail":"ypsah@devyard.org","sentAt":"2024-02-05T03:06:27Z","receivedAt":"2024-02-05T03:06:48Z","isPatch":true,"sender":{"key":"ypsah@devyard.org","avatar":null},"body":"On Sun Feb 4, 2024 at 11:51 PM CET, Junio C Hamano wrote:\n> Quentin Bouget <ypsah@devyard.org> writes:\n>\n> > During a re-authentication (second attempt at authenticating with a\n> > remote, e.g. after a failed GSSAPI attempt), git allows the remote to\n> > provide credential overrides in the redirect URL and unconditionnaly\n> > drops the current HTTP credentials in favors of those, even when there\n> > aren't any.\n> >\n> > This commit makes it so HTTP credentials are only overridden when the\n> > redirect URL actually contains credentials itself.\n>\n> \"This commit makes it so\" -> \"Make it so\"\n\nWill change.\n\n> > +\t\t\tchar *username = NULL, *password = NULL;\n> > +\n> > +\t\t\tif (http_auth.username)\n> > +\t\t\t\tusername = xstrdup(http_auth.username);\n> > +\t\t\tif (http_auth.password)\n> > +\t\t\t\tpassword = xstrdup(http_auth.password);\n>\n> Not a huge deal, but we have xstrdup_or_null() helper function\n> exactly for a use case like this.\n\nThanks, will change.\n\n> >  \t\t\tcredential_from_url(&http_auth, options->base_url->buf);\n> > +\n> > +\t\t\tif (http_auth.username)\n> > +\t\t\t\tfree(username);\n> > +\t\t\telse if (username)\n> > +\t\t\t\thttp_auth.username = username;\n> > +\n> > +\t\t\tif (http_auth.password)\n> > +\t\t\t\tfree(password);\n> > +\t\t\telse if (password)\n> > +\t\t\t\thttp_auth.password = password;\n>\n> This is an interesting change.  I wonder what breaks if we\n> completely ignored such credential materials forced by the remote\n> via a redirect?\n\nMe too. Maybe the original author would know. Is it OK to Cc them in\nthis case?\n\n> >  \t\t\turl = options->effective_url->buf;\n> >  \t\t}\n> >  \t}\n\nThanks,\nQuentin\n"},{"id":"487913","messageId":"CYWTFH7Z8IY4.313MNLG552V8M@devyard.org","threadId":"60844","inReplyTo":"017a01da57be$28748aa0$795d9fe0$@nexbridge.com","subject":"Re: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth","fromName":"Quentin Bouget","fromEmail":"ypsah@devyard.org","sentAt":"2024-02-05T03:12:56Z","receivedAt":"2024-02-05T03:13:05Z","isPatch":true,"sender":{"key":"ypsah@devyard.org","avatar":null},"body":"On Mon Feb 5, 2024 at 12:01 AM CET,  wrote:\n> On Sunday, February 4, 2024 1:54 PM, Quentin Bouget wrote:\n> > During a re-authentication (second attempt at authenticating with a remote, e.g.\n> > after a failed GSSAPI attempt), git allows the remote to provide credential overrides\n> > in the redirect URL and unconditionnaly drops the current HTTP credentials in favors\n> > of those, even when there aren't any.\n> >\n> > This commit makes it so HTTP credentials are only overridden when the redirect URL\n> > actually contains credentials itself.\n> >\n> > Signed-off-by: Quentin Bouget <ypsah@devyard.org>\n> > ---\n> > http.c | 18 ++++++++++++++++++\n> > 1 file changed, 18 insertions(+)\n> >\n> > diff --git a/http.c b/http.c\n> > index ccea19ac47..caba9cac1e 100644\n> > --- a/http.c\n> > +++ b/http.c\n> > @@ -2160,7 +2160,25 @@ static int http_request_reauth(const char *url,\n> >  \tif (options && options->effective_url && options->base_url) {\n> >  \t\tif (update_url_from_redirect(options->base_url,\n> >  \t\t\t\t\t     url, options->effective_url)) {\n> > +\t\t\tchar *username = NULL, *password = NULL;\n> > +\n> > +\t\t\tif (http_auth.username)\n> > +\t\t\t\tusername = xstrdup(http_auth.username);\n> > +\t\t\tif (http_auth.password)\n> > +\t\t\t\tpassword = xstrdup(http_auth.password);\n> > +\n> >  \t\t\tcredential_from_url(&http_auth, options->base_url->buf);\n> > +\n> > +\t\t\tif (http_auth.username)\n> > +\t\t\t\tfree(username);\n> > +\t\t\telse if (username)\n> > +\t\t\t\thttp_auth.username = username;\n> > +\n> > +\t\t\tif (http_auth.password)\n> > +\t\t\t\tfree(password);\n> > +\t\t\telse if (password)\n> > +\t\t\t\thttp_auth.password = password;\n> > +\n> >  \t\t\turl = options->effective_url->buf;\n> >  \t\t}\n> >  \t}\n>\n> I am wondering whether this is a good idea. Having credentials in a redirect\n> seems like it might be a vector for going somewhere other than what you want\n> to do, with credentials you do not necessarily want. Others might no better\n> than I on this, but would potentially lead to a CVE? I would prefer to see\n> credentials in a redirect rejected rather than used.\n\nI guess this can be controlled by setting http.followRedirects to false.\n\nI am not sure there is generally more danger in following a redirect URL\nwith or without the provided credentials. Note that this is also how git\ncurrently behaves and while I am not aware of any concrete use case\nmyself, I am also not confident there isn't any.\n\nThat being said, Brian M. Carlson pointed out that reusing credentials\nfrom the initial URL for the redirect URL is quite dangerous.\nIn my reply, I have suggested to switch to implementing the same\nbehaviour as libcurl when it comes to reusing credentials: if the\nhostname of the redirect is the same as the original URL, reuse the\ncredentials, otherwise drop them. [1]\nI would still retain the (seemingly strange) current behaviour of\nfavoring credentials in the redirect URL over anything else.\n\nThanks,\nQuentin\n\n[1] https://curl.se/libcurl/c/CURLOPT_UNRESTRICTED_AUTH.html\n"},{"id":"487914","messageId":"CYWT85QZ5IBD.3CWNWYQTUMTDJ@devyard.org","threadId":"60844","inReplyTo":"xmqqfry7onhf.fsf@gitster.g","subject":"Re: [PATCH 1/2] http: only reject basic auth credentials once they have been tried","fromName":"Quentin Bouget","fromEmail":"ypsah@devyard.org","sentAt":"2024-02-05T03:03:23Z","receivedAt":"2024-02-05T04:22:51Z","isPatch":true,"sender":{"key":"ypsah@devyard.org","avatar":null},"body":"On Sun Feb 4, 2024 at 11:47 PM CET, Junio C Hamano wrote:\n> Quentin Bouget <ypsah@devyard.org> writes:\n>\n> >  \telse if (results->http_code == 401) {\n> > -\t\tif (http_auth.username && http_auth.password) {\n> > -\t\t\tcredential_reject(&http_auth);\n> > -\t\t\treturn HTTP_NOAUTH;\n> > -\t\t} else {\n> > +\t\tif ((http_auth_methods & CURLAUTH_GSSNEGOTIATE) == CURLAUTH_GSSNEGOTIATE) {\n> >  \t\t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n> >  \t\t\tif (results->auth_avail) {\n> >  \t\t\t\thttp_auth_methods &= results->auth_avail;\n> >\t\t\t\thttp_auth_methods_restricted = 1;\n> >\t\t\t}\n> >\t\t\treturn HTTP_REAUTH;\n> >\t\t}\n> > +\t\tif (http_auth.username && http_auth.password)\n> > +\t\t\tcredential_reject(&http_auth);\n> > +\t\treturn HTTP_NOAUTH;\n>\n> A few comments and questions.\n>\n>  * GSSNEGOTIATE is a synonym for NEGOTIATE since cURL 7.38.0\n>    (released in Sep 2014); currently the earliest version we claim\n>    to support is 7.19.5 (released May 2009) without imap-send, and\n>    we require 7.34.0 (released Dec 2013) with imap-send, so for now,\n>    it is prudent that this patch uses GSSNEGOTIATE.\n\nAgreed\n\n>  * Is it something that the client code of libcURL can rely on that\n>    these CURLAUTH_FOO macros are bitmasks [*]?  If so, wouldn't\n>\n> \tif ((http_auth_methods & CURLAUTH_GSSNEGOTIATE))\n>\n>    be clear enough (and less risk of making typo)?\n>\n> [References]\n>\n>  * https://github.com/curl/curl/blob/b8c003832d730bb2f4b9de4204675ca5d9f7a903/include/curl/curl.h#L787C4-L787C64\n\nI had not considered the risk of typos, fair enough. Will change.\n\n>  * When we see 401, the first thing we do in the new code is to see\n>    if GSS is enabled in auth_methods, and if so we drop it from\n>    auth_methods (to prevent us from trying it again) and say REAUTH.\n>\n>    - What assures us that the presense of GSS bit in auth_methods\n>      mean we tried GSS to get this 401?  Could it be that we tried\n>      basic and seeing 401 from that, but we haven't tried GSS and we\n>      could retry with GSS now?  Is it commonly known that GSS is\n>      always tried first before Basic/Digest when both are availble,\n>      or something like that?\n\nlibcurl's documentation on CURLOPT_HTTPAUTH says:\n\n\tIf more than one bit is set, libcurl first queries the host to\n\tsee which authentication methods it supports and then picks the\n\tbest one you allow it to use.\n\nAnd then:\n\n\tCURLAUTH_NEGOTIATE\n\n\t\tHTTP Negotiate (SPNEGO) authentication. Negotiate\n\t\tauthentication is defined in RFC 4559 and is the most\n\t\tsecure way to perform authentication over HTTP. \n\nWhich hints at CURLAUTH_NEGOTIATE (aka. GSSNEGOTIATE as you pointed out)\nbeing the prefered auth method.\n\nThe current implementation confirms this. [1]\n\n>    - When auth_avail was given by the cURL library, we further limit\n>      the auth_methods (after dropping GSS) and say REAUTH.  This is\n>      not a new to the updated code, but can it happen that the\n>      resulting restricted auth_methods bitmap becomes empty (i.e.\n>      REAUTH would be useless)?\n\nI am not familiar enough with SPNEGO to say. Seems plausible.\nShould I instead do:\n\n\treturn (http_auth_methods & CURLAUTH_ANY) ? HTTP_REAUTH : HTTP_NOAUTH;\n\nThanks,\nQuentin\n\n[1] https://github.com/curl/curl/blob/b8c003832d730bb2f4b9de4204675ca5d9f7a903/lib/http.c#L373\n"},{"id":"487915","messageId":"ZcB2hHBoyiBdOw81@tanuki","threadId":"60844","inReplyTo":"20240204185427.39664-2-ypsah@devyard.org","subject":"Re: [PATCH 1/2] http: only reject basic auth credentials once they have been tried","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-05T05:47:48Z","receivedAt":"2024-02-05T05:47:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Feb 04, 2024 at 07:54:26PM +0100, Quentin Bouget wrote:\n> When CURLAUTH_GSSNEGOTIATE is enabled, it is currently assumed that\n> the provided username/password relate to a GSSAPI auth attempt.\n> In practice, forges such as gitlab can be deployed with HTTP basic auth\n> and GSSAPI auth both listening on the same port, meaning just because\n> the server supports GSSAPI and failed an authentication attempt using\n> the provided credentials, it does not mean the credentials are not valid\n> HTTP basic auth credentials.\n> \n> This is documented as a long running bug here [1] and breaks token-based\n> authentication when the token is provided in the remote's URL itself.\n> \n> This commit makes it so credentials are only dropped once they have been\n> tried both as GSSAPI credentials and HTTP basic auth credentials.\n> \n> [1] https://gitlab.com/gitlab-org/gitlab/-/blob/b0e0d25646d1992fefda863febdcba8d4c7a1bbf/doc/integration/kerberos.md#L250\n\nDo you think it's feasible to add a test for this? We already have a\nbunch of tests for authentication with Apache's httpd in t5563, so if we\ncould extend t/lib-httpd.sh to set up `mod_auth_gssapi` that would be\ngreat.\n\nI didn't try though, and it could just as well be that this would\nrequire a full-fledged Kerberos setup, which would be a deal breaker I\nguess. I ain't got enough familiarity with `mod_auth_gssapi` to tell.\n\nPatrick\n\n> Signed-off-by: Quentin Bouget <ypsah@devyard.org>\n> ---\n>  http.c | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/http.c b/http.c\n> index e73b136e58..ccea19ac47 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1758,10 +1758,7 @@ static int handle_curl_result(struct slot_results *results)\n>  \t} else if (missing_target(results))\n>  \t\treturn HTTP_MISSING_TARGET;\n>  \telse if (results->http_code == 401) {\n> -\t\tif (http_auth.username && http_auth.password) {\n> -\t\t\tcredential_reject(&http_auth);\n> -\t\t\treturn HTTP_NOAUTH;\n> -\t\t} else {\n> +\t\tif ((http_auth_methods & CURLAUTH_GSSNEGOTIATE) == CURLAUTH_GSSNEGOTIATE) {\n>  \t\t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n>  \t\t\tif (results->auth_avail) {\n>  \t\t\t\thttp_auth_methods &= results->auth_avail;\n> @@ -1769,6 +1766,9 @@ static int handle_curl_result(struct slot_results *results)\n>  \t\t\t}\n>  \t\t\treturn HTTP_REAUTH;\n>  \t\t}\n> +\t\tif (http_auth.username && http_auth.password)\n> +\t\t\tcredential_reject(&http_auth);\n> +\t\treturn HTTP_NOAUTH;\n>  \t} else {\n>  \t\tif (results->http_connectcode == 407)\n>  \t\t\tcredential_reject(&proxy_auth);\n> -- \n> 2.43.0\n> \n> \n"},{"id":"487922","messageId":"CAFLLRpJn6PJndfHCR8H6uSdCSVfzHPBBQ5o5=Z1Lj_2a3XtkyA@mail.gmail.com","threadId":"60844","inReplyTo":"CYWTFH7Z8IY4.313MNLG552V8M@devyard.org","subject":"Re: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth","fromName":"Robert Coup","fromEmail":"robert.coup@koordinates.com","sentAt":"2024-02-05T09:22:05Z","receivedAt":"2024-02-05T09:22:23Z","isPatch":true,"sender":{"key":"robert.coup@koordinates.com","avatar":"https://gravatar.com/avatar/d1a87d63ffb562b791992d8a119ebbdd742e703109d23333ca3fca51306ee95c?d=mp&s=160"},"body":"Hi Quentin,\n\n>  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.\n\nThe protocol & port number also need to match, so it doesn't end up\nthe same as CVE-2022-27774 [1]\n\nRob :)\n\n[1] https://curl.se/docs/CVE-2022-27774.html\n"},{"id":"487957","messageId":"ZcFeoyFXqLhMXVRh@tapette.crustytoothpaste.net","threadId":"60844","inReplyTo":"CYWT6JWQCTFG.106OCJTV3NQDU@devyard.org","subject":"Re: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-02-05T22:18:11Z","receivedAt":"2024-02-05T22:18:14Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-02-05 at 03:01:17, Quentin Bouget wrote:\n> Good point, I had not considered the security implications.\n> \n> I can see libcurl only reuses credentials after a redirect if the\n> hostname has not changed: [1]\n> \n> \tBy default, libcurl only sends credentials and Authentication\n> \theaders to the initial hostname as given in the original URL, to\n> \tavoid leaking username + password to other sites. \n> \n> Does it sound OK if I use the credentials provided by the redirect when\n> there are any (out of consistency with the current implementation), and\n> only allow reusing the current credentials when the redirect and the\n> original URLs share the same hostname?\n\nI don't think we can actually rely on that functionality because\n`credential.usehttppath` could actually have been set, in which case\nwe'd need a different credential.  For example, I know some forges issue\ncertain types of tokens that are tied to a specific URL and wouldn't\nvalidate for a redirect, even if it were actually the same repo.\n\nIf there are credentials in the URL provided by the redirect, I think it\nshould be safe to use them; otherwise, we'd need to rely on filling them\nwith the credential protocol.\n\n> Apologies, I feel like I may have given the impression I wanted to\n> configure credentials in git's configuration files, which is not the\n> case.\n> \n> My use case is to `git push` a tag from a CI/CD pipeline to trigger a\n> release, similar to how I do it here. [3]\n> \n> Or is this the kind of use case you are trying to discourage?\n\nWe're trying to discourage all use of credentials in the URL at the\ncommand line and in remote names/configuration files.  If you want to\npass in credentials from the environment, the Git FAQ explains how to do\nthat[0], and that technique can be used in such a situation.\n\n[0] https://git-scm.com/docs/gitfaq#http-credentials-environment\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"487966","messageId":"024001da5885$f85e1160$e91a3420$@nexbridge.com","threadId":"60844","inReplyTo":"ZcFeoyFXqLhMXVRh@tapette.crustytoothpaste.net","subject":"RE: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-02-05T22:52:12Z","receivedAt":"2024-02-05T22:52:32Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Monday, February 5, 2024 5:18 PM, brian m. carlson wrote:\n>On 2024-02-05 at 03:01:17, Quentin Bouget wrote:\n>> Good point, I had not considered the security implications.\n>>\n>> I can see libcurl only reuses credentials after a redirect if the\n>> hostname has not changed: [1]\n>>\n>> \tBy default, libcurl only sends credentials and Authentication\n>> \theaders to the initial hostname as given in the original URL, to\n>> \tavoid leaking username + password to other sites.\n>>\n>> Does it sound OK if I use the credentials provided by the redirect\n>> when there are any (out of consistency with the current\n>> implementation), and only allow reusing the current credentials when\n>> the redirect and the original URLs share the same hostname?\n>\n>I don't think we can actually rely on that functionality because\n>`credential.usehttppath` could actually have been set, in which case we'd need a\n>different credential.  For example, I know some forges issue certain types of tokens\n>that are tied to a specific URL and wouldn't validate for a redirect, even if it were\n>actually the same repo.\n>\n>If there are credentials in the URL provided by the redirect, I think it should be safe\n>to use them; otherwise, we'd need to rely on filling them with the credential\n>protocol.\n>\n>> Apologies, I feel like I may have given the impression I wanted to\n>> configure credentials in git's configuration files, which is not the\n>> case.\n>>\n>> My use case is to `git push` a tag from a CI/CD pipeline to trigger a\n>> release, similar to how I do it here. [3]\n>>\n>> Or is this the kind of use case you are trying to discourage?\n>\n>We're trying to discourage all use of credentials in the URL at the command line and\n>in remote names/configuration files.  If you want to pass in credentials from the\n>environment, the Git FAQ explains how to do that[0], and that technique can be\n>used in such a situation.\n>\n>[0] https://git-scm.com/docs/gitfaq#http-credentials-environment\n\nA 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.\n\n--Randall\n\n"}]}