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

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

[Nelson Benitez Leon]

If http_proactive_auth flag is set and there is a username
but no password in the proxy url, then interactively ask for
the password.

This makes possible to not have the password written down in
http_proxy env var or in http.proxy config option.

Also take care that CURLOPT_PROXY don't include username or
password, as we now set them in the new set_proxy_auth() function
where we use their specific cURL options.

Signed-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>
---
 http.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/http.c b/http.c
index be88acb..e7410f8 100644
--- a/http.c
+++ b/http.c
@@ -44,6 +44,7 @@ static const char *curl_http_proxy;
 static const char *curl_cookie_file;
 static struct credential cre_url = CREDENTIAL_INIT;
 static struct credential http_auth = CREDENTIAL_INIT;
+static struct credential proxy_auth = CREDENTIAL_INIT;
 static int http_proactive_auth;
 static const char *user_agent;
 
@@ -233,6 +234,20 @@ static int has_cert_password(void)
 	return 1;
 }
 
+static void set_proxy_auth(CURL *result)
+{
+	if (proxy_auth.username && proxy_auth.password) {
+#if LIBCURL_VERSION_NUM >= 0x071901
+		curl_easy_setopt(result, CURLOPT_PROXYUSERNAME, proxy_auth.username);
+		curl_easy_setopt(result, CURLOPT_PROXYPASSWORD, proxy_auth.password);
+#else
+		struct strbuf userpwd = STRBUF_INIT;
+		strbuf_addf(&userpwd, "%s:%s", proxy_auth.username, proxy_auth.password);
+		curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, strbuf_detach(&userpwd, NULL));
+#endif
+	}
+}
+
 static CURL *get_curl_handle(const char *url)
 {
 	CURL *result = curl_easy_init();
@@ -317,8 +332,18 @@ static CURL *get_curl_handle(const char *url)
 		free(env_proxy_var);
 	}
 	if (curl_http_proxy) {
-		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+		if (!proxy_auth.host) /* check to parse only once */
+			credential_from_url(&proxy_auth, curl_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);
+
+		struct strbuf proxyhost = STRBUF_INIT;
+		strbuf_addf(&proxyhost, "%s://%s", proxy_auth.protocol, 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);
 	}
 
 	return result;
-- 
1.7.7.6

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

[Junio C Hamano]

Nelson Benitez Leon <nelsonjesus.benitez@seap.minhap.es> writes:

>  	if (curl_http_proxy) {
> -		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +		if (!proxy_auth.host) /* check to parse only once */
> +			credential_from_url(&proxy_auth, curl_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);
> +
> +		struct strbuf proxyhost = STRBUF_INIT;
> +		strbuf_addf(&proxyhost, "%s://%s", proxy_auth.protocol, proxy_auth.host);
> +		curl_easy_setopt(result, CURLOPT_PROXY, strbuf_detach(&proxyhost, NULL));

How well has this code been tested?  The documentation for CURLOPT_PROXY
says this:

   CURLOPT_PROXY

   Set HTTP proxy to use. The parameter should be a char * to a zero
   terminated string holding the host name or dotted IP address. To
   specify port number in this string, append :[port] to the end of the
   host name. The proxy string may be prefixed with [protocol]:// since
   any such prefix will be ignored. The proxy's port number may optionally
   be specified with the separate option. If not specified, libcurl will
   default to using port 1080 for proxies. CURLOPT_PROXYPORT.

If the user has been happily using "127.0.0.1:4321" in curl_http_proxy
(i.e. without the meaningless <proto>:// part), the original code would
have called curl_easy_setopt with that string, and that would have been
how everything used to work.

If you haven't figured out proxy_auth.host at this point in the codepath,
you call credential_from_url() but the function only knows how to parse
the value for

	"<proto>://[<user>[:<pass>]@]<host>[:<port>]/..."

Specifically, it will punt with anything without "://" in it.

And then you use proxy_auth.protocol and proxy_auth.host to build
proxyhost.buf that presumably mimick the original curl_http_proxy (but
without the credential part).

I haven't formed an opinion on what the proper solution should be, but
either the credential_from_url() function needs to be updated to accept
the scp style [user@]<host>:<port> argument, or this specific caller
should take the responsibility to do special case the syntax.

>  		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +		set_proxy_auth(result);
>  	}
>  
>  	return result;

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I haven't formed an opinion on what the proper solution should be, but
> either the credential_from_url() function needs to be updated to accept
> the scp style [user@]<host>:<port> argument, or this specific caller
> should take the responsibility to do special case the syntax.

Well, calling the above "scp" style is a mistake (it is not scp style at
all), but the patch to teach the credentail_from_url() to handle the proxy
specification may look like this:

 credential.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/credential.c b/credential.c
index 62d1c56..482ae88 100644
--- a/credential.c
+++ b/credential.c
@@ -324,11 +324,13 @@ void credential_from_url(struct credential *c, const char *url)
 	 *   (1) proto://<host>/...
 	 *   (2) proto://<user>@<host>/...
 	 *   (3) proto://<user>:<pass>@<host>/...
+	 * or "proto://"-less variants of the above for *_proxy variables.
 	 */
 	proto_end = strstr(url, "://");
-	if (!proto_end)
-		return;
-	cp = proto_end + 3;
+	if (proto_end)
+		cp = proto_end + 3;
+	else
+		cp = url;
 	at = strchr(cp, '@');
 	colon = strchr(cp, ':');
 	slash = strchrnul(cp, '/');
@@ -348,7 +350,7 @@ void credential_from_url(struct credential *c, const char *url)
 		host = at + 1;
 	}
 
-	if (proto_end - url > 0)
+	if (proto_end && proto_end != url)
 		c->protocol = xmemdupz(url, proto_end - url);
 	if (slash - host > 0)
 		c->host = url_decode_mem(host, slash - host);

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> I haven't formed an opinion on what the proper solution should be, but
>> either the credential_from_url() function needs to be updated to accept
>> the scp style [user@]<host>:<port> argument, or this specific caller
>> should take the responsibility to do special case the syntax.
>
> Well, calling the above "scp" style is a mistake (it is not scp style at
> all), but the patch to teach the credentail_from_url() to handle the proxy
> specification may look like this:

Jeff, do you have an opinion on this?  I briefly wondered if we also want
to teach the traditional [user@]host:/path/to/repo to this function (it is
not a URL in RFC1738 sense, but it is in the remote.$name.url sense), but
because SSH does its own thing interacting with agents, perhaps it may not
help to teach our credential layer to store and supply cached passphrases
(or passwords, if the authentication is done by merely sending password
over the encrypted channel).

A safer approach might be to keep externally visible API to this function
as before, but add another function only for the use of http_proxy and
friends (whose kosher format is "host:address" without the "<scheme>://"
part), and call it from the codepath broken by the patch.

>  credential.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/credential.c b/credential.c
> index 62d1c56..482ae88 100644
> --- a/credential.c
> +++ b/credential.c
> @@ -324,11 +324,13 @@ void credential_from_url(struct credential *c, const char *url)
>  	 *   (1) proto://<host>/...
>  	 *   (2) proto://<user>@<host>/...
>  	 *   (3) proto://<user>:<pass>@<host>/...
> +	 * or "proto://"-less variants of the above for *_proxy variables.
>  	 */
>  	proto_end = strstr(url, "://");
> -	if (!proto_end)
> -		return;
> -	cp = proto_end + 3;
> +	if (proto_end)
> +		cp = proto_end + 3;
> +	else
> +		cp = url;
>  	at = strchr(cp, '@');
>  	colon = strchr(cp, ':');
>  	slash = strchrnul(cp, '/');
> @@ -348,7 +350,7 @@ void credential_from_url(struct credential *c, const char *url)
>  		host = at + 1;
>  	}
>  
> -	if (proto_end - url > 0)
> +	if (proto_end && proto_end != url)
>  		c->protocol = xmemdupz(url, proto_end - url);
>  	if (slash - host > 0)
>  		c->host = url_decode_mem(host, slash - host);

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

[Jeff King]

On Thu, Apr 12, 2012 at 08:54:22AM -0700, Junio C Hamano wrote:

> >> I haven't formed an opinion on what the proper solution should be, but
> >> either the credential_from_url() function needs to be updated to accept
> >> the scp style [user@]<host>:<port> argument, or this specific caller
> >> should take the responsibility to do special case the syntax.
> >
> > Well, calling the above "scp" style is a mistake (it is not scp style at
> > all), but the patch to teach the credentail_from_url() to handle the proxy
> > specification may look like this:
> 
> Jeff, do you have an opinion on this?  I briefly wondered if we also want
> to teach the traditional [user@]host:/path/to/repo to this function (it is
> not a URL in RFC1738 sense, but it is in the remote.$name.url sense), but
> because SSH does its own thing interacting with agents, perhaps it may not
> help to teach our credential layer to store and supply cached passphrases
> (or passwords, if the authentication is done by merely sending password
> over the encrypted channel).

My first instinct was "that is not a URL, and should be handled outside
this function". In particular, it has no protocol field, and that is an
important part of the credential-matching process. It would be up to the
caller to supply something sane in the protocol portion. In this case,
it would probably be "http" (unless we want to distinguish http proxies
from http end-points in the credential store, but I doubt that is
useful). But for an ssh-style URL, it would be "ssh". So already the
abstraction is a little bit leaky.

As far as parsing ssh goes, I'm not sure that is unambiguous with the
proxy syntax. If you see "127.0.0.1:1234", is that short for
"http://127.0.0.1:1234/" (what the proxy code wants), or for
"ssh://127.0.0.1/1234" (what ssh code would want)? The former seems less
odd to me, as it really is just a URL missing some components. The
latter is a true alternative syntax.

Like you said, I don't think we will ever want to handle ssh
credentials, though. There is already a solution for people who don't
want to input ssh passwords, and it is much more advanced and
well-supported within the community than what we would provide.

> A safer approach might be to keep externally visible API to this function
> as before, but add another function only for the use of http_proxy and
> friends (whose kosher format is "host:address" without the "<scheme>://"
> part), and call it from the codepath broken by the patch.

I think that would be cleaner conceptually, but it also means
reimplementing the user/password-parsing logic. And given that I don't
think we want to handle ssh, and that the semantics in your patch are
the only sane ones to me, it is not so bad. The caller just needs to be
aware of filling in the "protocol" field. Perhaps we could have an
alternate version that supplies a "default protocol" parameter. The
presence of that parameter would activate this code-path and
automatically fill in the protocol field.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> My first instinct was "that is not a URL, and should be handled outside
> this function". In particular, it has no protocol field, and that is an
> important part of the credential-matching process. It would be up to the
> caller to supply something sane in the protocol portion. In this case,
> it would probably be "http"...

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.

> ... (unless we want to distinguish http proxies
> from http end-points in the credential store, but I doubt that is
> useful).

That is something we may want to think carefully about.

If it is better to separate them, then we can easily invent "http-proxy",
"https-proxy" etc. for them, e.g.

	HTTPS_PROXY=http://127.0.0.1:1080
	git fetch https://over.there.xz/repo/sito/ry.git

would ask you for a credential to access 127.0.0.1:1080 in "https-proxy"
domain, and another to access over.there.xz in "https" domain.

In either case, the last example will not use "http" anywhere, even though
the value of the proxy has noiseword "http://" in front of it, which is
ignored.  So in that sense, even if we ignored the breakage for the proxy
specification without noiseword which Shawn noticed, the patch is broken,
as it asks credential for http://127.0.0.1:1080 and you parse it for "http"
protocol.

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

[Jeff King]

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

> 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

And furthermore, the decision to use http_proxy versus https_proxy is
about what the _target_ connection wants to do. So if you see this:

  HTTPS_PROXY=127.0.0.1:1080

it is still an http proxy; it is just that it is used for requests going
to https:// servers, and it will ask to tunnel via CONNECT instead of
GET. But in either case, the conversation with the proxy is over
straight http.

So the value should always be "http" in that case. This is a credential
we are handing to the proxy, not to the target server, and it is done
over http, not https.

I can't see that there is a way to tell curl to speak SSL to the proxy
itself.  Maybe I am missing it, but I couldn't find anything in the
code, nor make it work with "curl -x" to an "openssl s_server" instance.

> That is something we may want to think carefully about.
> 
> If it is better to separate them, then we can easily invent "http-proxy",
> "https-proxy" etc. for them, e.g.
> 
> 	HTTPS_PROXY=http://127.0.0.1:1080
> 	git fetch https://over.there.xz/repo/sito/ry.git
> 
> would ask you for a credential to access 127.0.0.1:1080 in "https-proxy"
> domain, and another to access over.there.xz in "https" domain.

No, it should ask for the credential for 127.0.0.1:1080 in the "http"
domain, per the above discussion.

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

In practice, I doubt it matters much; do people really have different
credentials for proxies and regular servers on the same host? And if so,
there is already a workaround by using the port number in the proxy
specification.

I really wish curl's credential-handling was implemented as a callback;
this would be much simpler if could let curl decipher the request and
come to us with the complete request (protocol, host, port, path, etc).
But even if we got such a feature in curl, we are stuck supporting the
old way for a while anyway.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Apr 12, 2012 at 02:25:12PM -0700, Junio C Hamano wrote:
>
>> 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);

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

[Jeff King]

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

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

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Oh, yes, I am in violent agreement. The language the http clients
(browsers etc) talk to the proxy may be part of HTTP specification, but it
is definitely different from the "http" talked with the origin servers.

>> > Not splitting "http" and "http-proxy" does have a slight confusion,...
>> 
>> 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.

OK.  Sounds like we are in agreement.

Nelson, care to re-roll the series, with fixes discussed in this thread
rolled into the second patch?

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

[Jeff King]

On Fri, Apr 13, 2012 at 12:35:17PM -0700, Junio C Hamano wrote:

> > 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.
> 
> OK.  Sounds like we are in agreement.
> 
> Nelson, care to re-roll the series, with fixes discussed in this thread
> rolled into the second patch?

I think there is a bug in the patch you posted, though:

> 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");
> +		}

What happens if the URL starts with "socks://"? We would want to
preserve that, and have our protocol end as "socks://".

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

Same here. If we get "socks://127.0.0.1", we will feed "127.0.0.1" to
curl, which will then assume that it's http.

BTW, do we actually need to strip the username out of the URL? We do not
do so with regular URLs, and curl takes the auth we give it over what is
in the URL. Can this be simplified to just:

  curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);

?

At any rate, I think the rule we want for parsing the url is to actually
parse the protocol from the url, and if it's not there, assume it's
http. And then convert all "http" to "http-proxy", because this is a
weird alternate use of "http". So we can just take your old patch to
relax the credential_from_url parsing, and then fix it up on the calling
side.  Like this:

diff --git a/credential.c b/credential.c
index 62d1c56..813e3cf 100644
--- a/credential.c
+++ b/credential.c
@@ -324,11 +324,15 @@ void credential_from_url(struct credential *c, const char *url)
 	 *   (1) proto://<host>/...
 	 *   (2) proto://<user>@<host>/...
 	 *   (3) proto://<user>:<pass>@<host>/...
+	 * or "proto://"-less variants of the above. They are not technically
+	 * URLs, but the caller may have some context-specific knowledge about
+	 * what protocol is in use.
 	 */
 	proto_end = strstr(url, "://");
-	if (!proto_end)
-		return;
-	cp = proto_end + 3;
+	if (proto_end)
+		cp = proto_end + 3;
+	else
+		cp = url;
 	at = strchr(cp, '@');
 	colon = strchr(cp, ':');
 	slash = strchrnul(cp, '/');
@@ -348,7 +352,7 @@ void credential_from_url(struct credential *c, const char *url)
 		host = at + 1;
 	}
 
-	if (proto_end - url > 0)
+	if (proto_end && proto_end != url)
 		c->protocol = xmemdupz(url, proto_end - url);
 	if (slash - host > 0)
 		c->host = url_decode_mem(host, slash - host);
diff --git a/http.c b/http.c
index 1c71edb..a164f79 100644
--- a/http.c
+++ b/http.c
@@ -334,17 +334,20 @@ static CURL *get_curl_handle(const char *url)
 		free(env_proxy_var);
 	}
 	if (curl_http_proxy) {
-		struct strbuf proxyhost = STRBUF_INIT;
-
-		if (!proxy_auth.host) /* check to parse only once */
+		if (!proxy_auth.host) {
 			credential_from_url(&proxy_auth, curl_http_proxy);
+			if (!proxy_auth.protocol ||
+			    !strcmp(proxy_auth.protocol, "http")) {
+				free(proxy_auth.protocol);
+				proxy_auth.protocol = xstrdup("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);
-		curl_easy_setopt(result, CURLOPT_PROXY, strbuf_detach(&proxyhost, NULL));
+		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
 		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
 		set_proxy_auth(result);
 	}

Note that curl will treat "foobar://" (or any protocol it does not
understand) as an http proxy. I didn't want to get into white-listing
"socks:// is ok, but foobar:// is really just http in disguise" based on
curl's internal rules. So you can use "foobar://" if you want, but it's
not going to share credentials with "http://" (even though curl will use
them in exactly the same way).

-Peff

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

[Jeff King]

On Tue, Mar 13, 2012 at 03:03:54PM +0100, Nelson Benitez Leon wrote:

> Also take care that CURLOPT_PROXY don't include username or
> password, as we now set them in the new set_proxy_auth() function
> where we use their specific cURL options.

Did you test that this is necessary? We don't do it for the regular URL
case, and it makes the code much simpler if we can avoid munging what we
hand to curl.

> +static void set_proxy_auth(CURL *result)
> +{
> +	if (proxy_auth.username && proxy_auth.password) {
> +#if LIBCURL_VERSION_NUM >= 0x071901
> +		curl_easy_setopt(result, CURLOPT_PROXYUSERNAME, proxy_auth.username);
> +		curl_easy_setopt(result, CURLOPT_PROXYPASSWORD, proxy_auth.password);
> +#else

Is that version check right? You are giving a hexadecimal number, so
7.19.1 would be 071301.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Mar 13, 2012 at 03:03:54PM +0100, Nelson Benitez Leon wrote:
>
>> Also take care that CURLOPT_PROXY don't include username or
>> password, as we now set them in the new set_proxy_auth() function
>> where we use their specific cURL options.
>
> Did you test that this is necessary? We don't do it for the regular URL
> case, and it makes the code much simpler if we can avoid munging what we
> hand to curl.
>
>> +static void set_proxy_auth(CURL *result)
>> +{
>> +	if (proxy_auth.username && proxy_auth.password) {
>> +#if LIBCURL_VERSION_NUM >= 0x071901
>> +		curl_easy_setopt(result, CURLOPT_PROXYUSERNAME, proxy_auth.username);
>> +		curl_easy_setopt(result, CURLOPT_PROXYPASSWORD, proxy_auth.password);
>> +#else
>
> Is that version check right? You are giving a hexadecimal number, so
> 7.19.1 would be 071301.

I notice that I missed this comment, and I think the version queued in
'pu' still has this incorrect.  

CURLOPT_PROXYUSERNAME is marked as Introduced at 7.19.1 in

   https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions

so I agree that the above would need to be 0x071301.