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

[PATCH v3 3/4] 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.

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

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

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

[Jeff King]

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?

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

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

[Nelson Benitez Leon]

On 03/06/2012 09:30 AM, Jeff King wrote:
> 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.

> 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

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

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

[Jeff King]

On Tue, Mar 06, 2012 at 11:58:59AM +0100, Nelson Benitez Leon wrote:

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

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

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

[Jeff King]

On Tue, Mar 06, 2012 at 05:09:47AM -0500, Jeff King wrote:

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