On Sun, Aug 21, 2022 at 12:32:54AM +0200, Daniel Stenberg wrote:
> I would not mind having a discussion in the curl project to see if we should
> possibly consider adding a middle-ground where we allow sending credentials
> to another port for the same host name, but I am personally NOT sold on the
> idea. I think such redirects should rather be fixed and avoided - since I
> believe users will not understand the security implications of doing them.
I'm not 100% on it either. When it comes to security restrictions,
sometimes simple-and-stupid is the best way. I was literally thinking of
something as basic and restricted as:
if (from_port == 80 && to_port == 443 &&
from_protocol == HTTP && to_protocol == HTTPS)
/* ok, allow it */
just because https upgrade is such a common (and presumably harmless)
redirect. But possibly even that leaves wiggle room for bad things to
happen. I'm happy to defer to you and other curl folks there.
I think the worst thing about the user experience from Git here is that
it says "authentication failed", which is indistinguishable from a bogus
credential. We don't even mention the redirect, because we don't warn
about them unless the request ends up successful!
So I was thinking that curl could tell us "hey, I cleared the auth due
to a redirect" via some curl_easy_getinfo() call. But maybe just
noticing there was a redirect would be sufficient. This seems to work:
diff --git a/http.c b/http.c
index 5d0502f51f..0fe8a906e5 100644
--- a/http.c
+++ b/http.c
@@ -1934,7 +1934,7 @@ static int http_request_reauth(const char *url,
{
int ret = http_request(url, result, target, options);
- if (ret != HTTP_OK && ret != HTTP_REAUTH)
+ if (ret != HTTP_OK && ret != HTTP_REAUTH && ret != HTTP_NOAUTH)
return ret;
if (options && options->effective_url && options->base_url) {
diff --git a/remote-curl.c b/remote-curl.c
index b8758757ec..d462077a97 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -501,6 +501,12 @@ static struct discovery *discover_refs(const char *service, int for_push)
transport_anonymize_url(url.buf));
case HTTP_NOAUTH:
show_http_message(&type, &charset, &buffer);
+ if (LIBCURL_VERSION_NUM >= 0x075300 &&
+ strcmp(refs_url.buf, effective_url.buf)) {
+ char *u = transport_anonymize_url(url.buf);
+ warning(_("request redirected to %s"), u);
+ warning(_("authentication information may have been discarded"));
+ }
die(_("Authentication failed for '%s'"),
transport_anonymize_url(url.buf));
case HTTP_NOMATCHPUBLICKEY:
Though I wonder if there is a cleaner way to determine what happened
than string comparisons.
-Peff
