threads / patch / 30408

patch, 6 partshttp: handle proxy proactive authentication

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

## tl;dr

6 messages between May 3, 2012 and May 4, 2012. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Nelson Benitez Leon· May 3, 2012, 16:39 UTC · lore

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(-)
Show changes to http.c +27 −1
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
Jeff King· May 4, 2012, 07:16 UTC · re: Nelson Benitez Leon · lore

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

On Thu, May 03, 2012 at 06:39:54PM +0200, Nelson Benitez Leon wrote:
Show 10 quoted lines
> 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:

Show 16 quoted lines
> @@ -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
Nelson Benitez Leon· May 4, 2012, 11:10 UTC · re: Jeff King · lore

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

On 05/04/2012 09:16 AM, Jeff King wrote:
Show 37 quoted lines
> 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.

Jeff King· May 4, 2012, 10:51 UTC · re: Nelson Benitez Leon · lore

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

On Fri, May 04, 2012 at 01:10:38PM +0200, Nelson Benitez Leon wrote:
Show 8 quoted lines
> > 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
Nelson Benitez Leon· May 4, 2012, 13:55 UTC · re: Jeff King · lore

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

On 05/04/2012 09:16 AM, Jeff King wrote:
Show 15 quoted lines
> 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.

Jeff King· May 4, 2012, 13:55 UTC · re: Nelson Benitez Leon · lore

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

On Fri, May 04, 2012 at 03:55:47PM +0200, Nelson Benitez Leon wrote:
Show 13 quoted lines
> >> 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.

Show 8 quoted lines
> 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

← back to recent threads