[PATCH 2/6] http: handle proxy proactive authentication

[PATCH 2/6] 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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 http.c |   28 +++++++++++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/http.c b/http.c
index 64df7b1..02f9fcd 100644
--- a/http.c
+++ b/http.c
@@ -43,6 +43,7 @@ static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
 static const char *curl_cookie_file;
 static struct credential http_auth = CREDENTIAL_INIT;
+static struct credential proxy_auth = CREDENTIAL_INIT;
 static int http_proactive_auth;
 static const char *user_agent;
 
@@ -272,6 +273,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 >= 0x071301
+		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();
@@ -351,8 +366,19 @@ static CURL *get_curl_handle(const char *url)
 	}
 	
 	if (curl_http_proxy) {
-		curl_easy_setopt(result, CURLOPT_PROXY, 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 (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_PROXYAUTH, CURLAUTH_ANY);
+		set_proxy_auth(result);
 	}
 
 	return result;
-- 
1.7.7.6

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

[Jeff King]

On Thu, May 03, 2012 at 06:39:54PM +0200, Nelson Benitez Leon wrote:

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

Do we actually need to do that? If we set CURLOPT_PROXYUSERNAME, will
curl ignore it in favor of what's in the URL? I ask, because there is a
bug here:

> @@ -351,8 +366,19 @@ static CURL *get_curl_handle(const char *url)
>  	}
>  	
>  	if (curl_http_proxy) {
> -		curl_easy_setopt(result, CURLOPT_PROXY, 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 (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));

When you parse the URL via credential_from_url, the components you get
will have any URL-encoding removed. So when you regenerate the URL in
the proxyhost variable, you would need to re-encode.

But if we can stop doing this regeneration at all, then the problem goes
away.

Also, newer versions of curl will copy the string instead of taking
ownership of the pointer. Unfortunately we have to deal with both old
and new, but you can get around it by using a static strbuf (so we leak,
but we only leak once per program, not once per get_curl_handle call).
This issue would also go away if we stop regenerating the URL.

-Peff

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

[Nelson Benitez Leon]

On 05/04/2012 09:16 AM, Jeff King wrote:
> On Thu, May 03, 2012 at 06:39:54PM +0200, Nelson Benitez Leon wrote:
> 
>> 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.
> 
> Do we actually need to do that? If we set CURLOPT_PROXYUSERNAME, will
> curl ignore it in favor of what's in the URL? I ask, because there is a
> bug here:
> 
>> @@ -351,8 +366,19 @@ static CURL *get_curl_handle(const char *url)
>>  	}
>>  	
>>  	if (curl_http_proxy) {
>> -		curl_easy_setopt(result, CURLOPT_PROXY, 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 (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));
> 
> When you parse the URL via credential_from_url, the components you get
> will have any URL-encoding removed. So when you regenerate the URL in
> the proxyhost variable, you would need to re-encode.

Can a hostname has url-encoded parts? I thought that was only for the
request uri (/somedir/somefile.php) or the query string ('?var1=val'),
I'm only using the hostname here as a proxy server never has more than
that, apart from the port number.

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

[Jeff King]

On Fri, May 04, 2012 at 01:10:38PM +0200, Nelson Benitez Leon wrote:

> > When you parse the URL via credential_from_url, the components you get
> > will have any URL-encoding removed. So when you regenerate the URL in
> > the proxyhost variable, you would need to re-encode.
> 
> Can a hostname has url-encoded parts? I thought that was only for the
> request uri (/somedir/somefile.php) or the query string ('?var1=val'),
> I'm only using the hostname here as a proxy server never has more than
> that, apart from the port number.

Hmm. It can have URL-encoded parts (so we must decode when parsing), but
the more important question is whether the decoded version can have
parts that _need_ to be URL-encoded.  And I think the answer is no,
after double-checking the RFCs (i.e., hostnames cannot contain any of the URL
reserved characters). So quoting would be a no-op, and we can skip it.

Anyway, your later patch ends up removing this chunk of code, so I think
we can forget the issue entirely.

-Peff

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

[Nelson Benitez Leon]

On 05/04/2012 09:16 AM, Jeff King wrote:
> On Thu, May 03, 2012 at 06:39:54PM +0200, Nelson Benitez Leon wrote:
> 
>> 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.
> 
> Do we actually need to do that? If we set CURLOPT_PROXYUSERNAME, will
> curl ignore it in favor of what's in the URL? 

I explicitly remove username/pass from CURLOPT_PROXY to not having to worry
about that question, to not provide cURL with two different sets of proxy auth
info, common sense dictates cURL specific proxy options should take precedence
over embedded in url by I haven't seen that mentioned by any cURL docs so we 
should look at the source to know the truth but even then that could change in
the future so I think is safer to only provide one path for auth info.

Having username/password on the CURLOPT_PROXY option gives us no special gain at
the cost of not permitting usernames with reserved characters like '@' or ':' which
are not unusual at all. So I'm inclined to preserve current set_proxy_auth() 
function and re-introduce the code that sets CURLOPT_PROXY with only $prot://$host.

Are you ok with this? or do you prefer I change set_proxy_auth() to a set_curl_proxy()
function where I embedded user/pass in CURLOPT_PROXY ? that is the remaining thing I need
to know to send a new re-roll.

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

[Jeff King]

On Fri, May 04, 2012 at 03:55:47PM +0200, 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.
> > 
> > Do we actually need to do that? If we set CURLOPT_PROXYUSERNAME, will
> > curl ignore it in favor of what's in the URL? 
> 
> I explicitly remove username/pass from CURLOPT_PROXY to not having to worry
> about that question, to not provide cURL with two different sets of proxy auth
> info, common sense dictates cURL specific proxy options should take precedence
> over embedded in url by I haven't seen that mentioned by any cURL docs so we 
> should look at the source to know the truth but even then that could change in
> the future so I think is safer to only provide one path for auth info.

Yes, I would expect the specific proxy options to take over. And that is
what happens for the regular URL case, where we do not do any munging at
all. I phrased my question as I did because that was the only set of
circumstances I could see where not munging the URL would _hurt_ us. In
other words, I do not find it likely that it will hurt us to leave it
intact.

But it may hurt us to munge it.  My concern is that we are adding a
bunch of code to replicate how curl behaves (with respect to pulling the
proxy information from the environment). If we leave the proxy URL
untouched, then if we fail to do the same thing as curl, the worst case
is that we don't get the credential properly (if one is even necessary).
But if we do rewrite the proxy, then we are potentially screwing up what
curl would do, whether a credential would have been necessary or not.

So to me it is the lower-risk path to let curl do its regular thing
(pulling the proxy from the environment), and just let us handle the
credential acquisition side of things. And it also is less code for us.

> Having username/password on the CURLOPT_PROXY option gives us no special gain at
> the cost of not permitting usernames with reserved characters like '@' or ':' which
> are not unusual at all. So I'm inclined to preserve current set_proxy_auth() 
> function and re-introduce the code that sets CURLOPT_PROXY with only $prot://$host.
> 
> Are you ok with this? or do you prefer I change set_proxy_auth() to a set_curl_proxy()
> function where I embedded user/pass in CURLOPT_PROXY ? that is the remaining thing I need
> to know to send a new re-roll.

No, I think you should leave CURLOPT_PROXY unset, unless you are giving
curl the verbatim URL given to us via git-config. Let our parsing be
only for credentials, and let curl handle everything else.

-Peff