threads / patch / 29839

v3, 4 partshttp: handle proxy proactive authentication

Subject: [PATCH v3 3/4] http: handle proxy proactive authentication

## tl;dr

5 messages between Mar 5, 2012 and Mar 6, 2012. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Nelson Benitez Leon· Mar 5, 2012, 15:19 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.

Signed-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>
---
 http.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
Show changes to http.c +12 −0
diff --git a/http.c b/http.c
index 8932da5..b0b4362 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;
 
@@ -303,6 +304,17 @@ static CURL *get_curl_handle(void)
 		}
 	}
 	if (curl_http_proxy) {
+		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 */
+			struct strbuf pbuf = STRBUF_INIT;
+			credential_fill(&proxy_auth);
+			strbuf_addf(&pbuf, "%s://%s:%s@%s",proxy_auth.protocol,
+			    	proxy_auth.username, proxy_auth.password,
+			    	proxy_auth.host);
+			free ((void *)curl_http_proxy);
+			curl_http_proxy =  strbuf_detach(&pbuf, NULL);
+		}
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
 		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
 	}

-- 
1.7.7.6
Jeff King· Mar 6, 2012, 08:30 UTC · re: Nelson Benitez Leon · lore

Re: [PATCH v3 3/4] http: handle proxy proactive authentication

On Mon, Mar 05, 2012 at 04:19:40PM +0100, Nelson Benitez Leon wrote:
Show 24 quoted lines
> diff --git a/http.c b/http.c
> index 8932da5..b0b4362 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;
>  
> @@ -303,6 +304,17 @@ static CURL *get_curl_handle(void)
>  		}
>  	}
>  	if (curl_http_proxy) {
> +		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 */
> +			struct strbuf pbuf = STRBUF_INIT;
> +			credential_fill(&proxy_auth);
> +			strbuf_addf(&pbuf, "%s://%s:%s@%s",proxy_auth.protocol,
> +			    	proxy_auth.username, proxy_auth.password,
> +			    	proxy_auth.host);

Can we pull this out into a helper function, since the next patch will need to do the exact same thing in the 407 case?

Also, when turning it back into a URL to hand to curl, should we be percent-encoding the items we put in? If my password has an "@" in it, wouldn't we generate a bogus URL? Although looking at how the http auth code handles this, we set CURLOPT_USERPWD directly. Should you be setting CURLOPT_PROXYUSERPWD instead of munging the proxy URL?

> +			free ((void *)curl_http_proxy);

Please don't cast to void. This is C, not C++, and casts to void pointers are implicit. They can never help, and might cover up an actual type error (e.g., casting a non-pointer type).

-Peff
Nelson Benitez Leon· Mar 6, 2012, 10:58 UTC · re: Jeff King · lore

Re: [PATCH v3 3/4] http: handle proxy proactive authentication

On 03/06/2012 09:30 AM, Jeff King wrote:
Show 29 quoted lines
> On Mon, Mar 05, 2012 at 04:19:40PM +0100, Nelson Benitez Leon wrote:
> 
>> diff --git a/http.c b/http.c
>> index 8932da5..b0b4362 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;
>>  
>> @@ -303,6 +304,17 @@ static CURL *get_curl_handle(void)
>>  		}
>>  	}
>>  	if (curl_http_proxy) {
>> +		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 */
>> +			struct strbuf pbuf = STRBUF_INIT;
>> +			credential_fill(&proxy_auth);
>> +			strbuf_addf(&pbuf, "%s://%s:%s@%s",proxy_auth.protocol,
>> +			    	proxy_auth.username, proxy_auth.password,
>> +			    	proxy_auth.host);
> 
> Can we pull this out into a helper function, since the next patch will
> need to do the exact same thing in the 407 case?
Ok.
Show 5 quoted lines
> Also, when turning it back into a URL to hand to curl, should we be
> percent-encoding the items we put in? If my password has an "@" in it,
> wouldn't we generate a bogus URL? Although looking at how the http auth
> code handles this, we set CURLOPT_USERPWD directly. Should you be
> setting CURLOPT_PROXYUSERPWD instead of munging the proxy URL?

Ok, but it seems is CURLOPT_PROXYUSERNAME and CURLOPT_PROXYPASSWORD what we need here as per documentation[1]

[1] http://curl.haxx.se/libcurl/c/curl_easy_setopt.html#CURLOPTPROXYUSERNAME
Show 5 quoted lines
>> +			free ((void *)curl_http_proxy);
> 
> Please don't cast to void. This is C, not C++, and casts to void
> pointers are implicit.  They can never help, and might cover up an
> actual type error (e.g., casting a non-pointer type).

Ok, will remove it, I copy/paste it from the http code and I must admit I didn't understand why this was needed.

Jeff King· Mar 6, 2012, 10:09 UTC · re: Nelson Benitez Leon · lore

Re: [PATCH v3 3/4] http: handle proxy proactive authentication

On Tue, Mar 06, 2012 at 11:58:59AM +0100, Nelson Benitez Leon wrote:
Show 10 quoted lines
> > Also, when turning it back into a URL to hand to curl, should we be
> > percent-encoding the items we put in? If my password has an "@" in it,
> > wouldn't we generate a bogus URL? Although looking at how the http auth
> > code handles this, we set CURLOPT_USERPWD directly. Should you be
> > setting CURLOPT_PROXYUSERPWD instead of munging the proxy URL?
> 
> Ok, but it seems is CURLOPT_PROXYUSERNAME and CURLOPT_PROXYPASSWORD what
> we need here as per documentation[1]
> 
> [1] http://curl.haxx.se/libcurl/c/curl_easy_setopt.html#CURLOPTPROXYUSERNAME

Yes, the split CURLOPT_USERNAME and CURLOPT_PASSWORD interface is much better (it allows the username to contain a colon). But it was not introduced until curl 7.19.3, and we support older versions.

We could do an #if on LIBCURL_VERSION_NUM to use the new form when it's available, but nobody has complained so far.

Show 8 quoted lines
> >> +			free ((void *)curl_http_proxy);
> > 
> > Please don't cast to void. This is C, not C++, and casts to void
> > pointers are implicit.  They can never help, and might cover up an
> > actual type error (e.g., casting a non-pointer type).
> 
> Ok, will remove it, I copy/paste it from the http code and I must admit
> I didn't understand why this was needed.

Ah. I grepped for the spot you copied. The cast is to get rid of the "const" on curl_http_proxy. But if it's a pointer to allocated memory, it should not be declared const in the first place. Unfortunately, fixing this means casting in the call to git_config_string (which for some reason takes a pointer-to-const-pointer, even though the value it puts in will always be allocated by xstrdup). Or fixing git_config_string, but that cascades to require fixing in lots of other places. Ugh.

-Peff
Jeff King· Mar 6, 2012, 10:53 UTC · re: Jeff King · lore

Re: [PATCH v3 3/4] http: handle proxy proactive authentication

On Tue, Mar 06, 2012 at 05:09:47AM -0500, Jeff King wrote:
Show 11 quoted lines
> > Ok, will remove it, I copy/paste it from the http code and I must admit
> > I didn't understand why this was needed.
> 
> Ah.  I grepped for the spot you copied. The cast is to get rid of the
> "const" on curl_http_proxy. But if it's a pointer to allocated memory,
> it should not be declared const in the first place. Unfortunately,
> fixing this means casting in the call to git_config_string (which for
> some reason takes a pointer-to-const-pointer, even though the value it
> puts in will always be allocated by xstrdup). Or fixing
> git_config_string, but that cascades to require fixing in lots of other
> places. Ugh.

I did a little more looking into this. The situation is annoyingly complex, because some callers really do have const strings. They do things like this:

  static const char *prune_expire = "2.weeks.ago";
  [...]
  git_config_string(&prune_expire, var, value);

And that one should be const, because the string literal really is const. Sometimes it is converted into an allocated value, but we effectively treat it as const.

And then you have things like curl_http_proxy, which are never actually const, but always allocated strings (that get freed during cleanup). Those ones should probably not be const.

So I don't think there's an easy solution, and the least evil thing is probably to just keep the cast on free, as you did (although I think if you convert to using CURLOPT_PROXYUSERPWD, you won't be rewriting curl_http_proxy anymore, and thus your free() call will go away).

-Peff

← back to recent threads