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

From: Jeff King <peff@peff.net>
Date: May 4, 2012, 07:16 UTC
Message-ID: <20120504071632.GB21895@sigill.intra.peff.net>
In-Reply-To: <4FA2B4DA.60908@seap.minhap.es>

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