Re: [PATCH v5 2/5] http: handle proxy proactive authentication

Jeff King [off-list ref] writes:

On Thu, Apr 12, 2012 at 02:25:12PM -0700, Junio C Hamano wrote:

quoted

Outside git, these actually come from things like these:

	http_proxy=127.0.0.1:1080
	HTTPS_PROXY=127.0.0.1:1080

And http.proxy configuration variable we have is a substitute for
http_proxy.  So if we want to keep the credentials for destination servers
and the credentials for proxies, "http.proxy" codepath should be asking
you with "http".  If we are looking at HTTPS_PROXY, you should get "https".
The patch that broke the unauthenticated proxy access does neither.

Hmm. Does the distinction between http and https actually matter to
curl? My reading of the code and documentation is that only "http" is
meaningful (actually, anything besides socks*:// gets converted to
http).

So as far as I can tell, these are equivalent:

  http_proxy=http://127.0.0.1:1080
  http_proxy=https://127.0.0.1:1080
  http_proxy=foobar://127.0.0.1:1080

Yes, that is exactly what I was trying to say.  The foobar:// part does
not matter; "http" in "http_proxy" is what matters, as it is how you can
specify two separate proxies depending on what destination you are going
via what protocol.

Not splitting "http" and "http-proxy" does have a slight confusion, as
the default proxy port is "1080". So a proxy of "http://127.0.0.1" would
mean "http://127.0.0.1:1080", whereas a regular request would mean
"http://127.0.0.1:80". The credential code includes the port as part of
the unique hostname, but since the default-port magic happens inside
curl, we have no access to it (short of re-implementing it ourselves).

Ok, so how about this as a replacement patch for what I have had for the
past few days?

 credential.c |   44 +++++++++++++++++++++++++++++++-------------
 credential.h |    1 +
 http.c       |   10 +++++++---
 3 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/credential.c b/credential.c
index 62d1c56..5803645 100644
--- a/credential.c
+++ b/credential.c

@@ -313,22 +313,17 @@ void credential_reject(struct credential *c)
 	c->approved = 0;
 }
 
-void credential_from_url(struct credential *c, const char *url)
+static void credential_for_dest(struct credential *c, const char *dest)
 {
-	const char *at, *colon, *cp, *slash, *host, *proto_end;
-
-	credential_clear(c);
+	const char *at, *colon, *cp, *slash, *host;
 
 	/*
 	 * Match one of:
-	 *   (1) proto://<host>/...
-	 *   (2) proto://<user>@<host>/...
-	 *   (3) proto://<user>:<pass>@<host>/...
+	 *   (1) <host>/...
+	 *   (2) <user>@<host>/...
+	 *   (3) <user>:<pass>@<host>/...
 	 */
-	proto_end = strstr(url, "://");
-	if (!proto_end)
-		return;
-	cp = proto_end + 3;
+	cp = dest;
 	at = strchr(cp, '@');
 	colon = strchr(cp, ':');
 	slash = strchrnul(cp, '/');

@@ -348,10 +343,9 @@ void credential_from_url(struct credential *c, const char *url)
 		host = at + 1;
 	}
 
-	if (proto_end - url > 0)
-		c->protocol = xmemdupz(url, proto_end - url);
 	if (slash - host > 0)
 		c->host = url_decode_mem(host, slash - host);
+
 	/* Trim leading and trailing slashes from path */
 	while (*slash == '/')
 		slash++;

@@ -363,3 +357,27 @@ void credential_from_url(struct credential *c, const char *url)
 			*p-- = '\0';
 	}
 }
+
+void credential_for_destination(struct credential *c, const char *dest, const char *proto)
+{
+	credential_clear(c);
+	c->protocol = xstrdup(proto);
+	credential_for_dest(c, dest);
+}
+
+void credential_from_url(struct credential *c, const char *url)
+{
+	const char *proto_end;
+
+	credential_clear(c);
+
+	/*
+	 * Strip "proto://" part and let credential_for_dest()
+	 * parse the remainder.
+	 */
+	proto_end = strstr(url, "://");
+	if (!proto_end)
+		return;
+	c->protocol = xmemdupz(url, proto_end - url);
+	credential_for_dest(c, proto_end + 3);
+}

diff --git a/credential.h b/credential.h
index 96ea41b..4b1c320 100644
--- a/credential.h
+++ b/credential.h

@@ -27,6 +27,7 @@ void credential_reject(struct credential *);
 
 int credential_read(struct credential *, FILE *);
 void credential_from_url(struct credential *, const char *url);
+void credential_for_destination(struct credential *, const char *dest, const char *proto);
 int credential_match(const struct credential *have,
 		     const struct credential *want);
 

diff --git a/http.c b/http.c
index 1c71edb..752b6ea 100644
--- a/http.c
+++ b/http.c

@@ -336,14 +336,18 @@ static CURL *get_curl_handle(const char *url)
 	if (curl_http_proxy) {
 		struct strbuf proxyhost = STRBUF_INIT;
 
-		if (!proxy_auth.host) /* check to parse only once */
-			credential_from_url(&proxy_auth, curl_http_proxy);
+		if (!proxy_auth.host) {
+			const char *cp;
+			cp = strstr(curl_http_proxy, "://");
+			cp = cp ? (cp + 3) : curl_http_proxy;
+			credential_for_destination(&proxy_auth, cp, "http-proxy");
+		}
 
 		if (http_proactive_auth && proxy_auth.username && !proxy_auth.password)
 			/* proxy string has username but no password, ask for password */
 			credential_fill(&proxy_auth);
 
-		strbuf_addf(&proxyhost, "%s://%s", proxy_auth.protocol, proxy_auth.host);
+		strbuf_addstr(&proxyhost, proxy_auth.host);
 		curl_easy_setopt(result, CURLOPT_PROXY, strbuf_detach(&proxyhost, NULL));
 		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
 		set_proxy_auth(result);

On Thu, Apr 12, 2012 at 03:18:01PM -0700, Junio C Hamano wrote:

quoted

So as far as I can tell, these are equivalent:

  http_proxy=http://127.0.0.1:1080
  http_proxy=https://127.0.0.1:1080
  http_proxy=foobar://127.0.0.1:1080

Yes, that is exactly what I was trying to say.  The foobar:// part does
not matter; "http" in "http_proxy" is what matters, as it is how you can
specify two separate proxies depending on what destination you are going
via what protocol.

But you snipped the later part of my message, which is that the "http"
in "http_proxy" does _not_ matter. It is about which destinations to
apply the proxy to, not how you talk to the proxy (and the latter is what
should matter for the credentials).

quoted

Not splitting "http" and "http-proxy" does have a slight confusion, as
the default proxy port is "1080". So a proxy of "http://127.0.0.1" would
mean "http://127.0.0.1:1080", whereas a regular request would mean
"http://127.0.0.1:80". The credential code includes the port as part of
the unique hostname, but since the default-port magic happens inside
curl, we have no access to it (short of re-implementing it ourselves).

Ok, so how about this as a replacement patch for what I have had for the
past few days?

My other message argued "the http-proxy distinction might be important,
but probably isn't". But I didn't talk about "the http-proxy distinction
might break helpers". The stock helpers will be fine; they are totally
clueless about what the protocol means, and just treat it as a string to
be matched. But for something like osxkeychain, where it is converting
the protocol string into some OS-specific magic value, it does matter,
and http-proxy would cause it to exit in confusion.

It looks like OS X defines a SOCKS type and an HTTPProxy type for its
keychain API. So in either case, it should probably be updated to handle
these new types. And I guess that argues for making the distinction,
since at least one helper does want to care about it.

-Peff