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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 9, 2012, 21:39 UTC
Message-ID: <7v398cvb30.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4F5F53CA.7090003@seap.minhap.es>

Nelson Benitez Leon <nelsonjesus.benitez@seap.minhap.es> writes:

Show 12 quoted lines

>  	if (curl_http_proxy) {
> -		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +		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);
> +
> +		struct strbuf proxyhost = STRBUF_INIT;
> +		strbuf_addf(&proxyhost, "%s://%s", proxy_auth.protocol, proxy_auth.host);
> +		curl_easy_setopt(result, CURLOPT_PROXY, strbuf_detach(&proxyhost, NULL));

How well has this code been tested? The documentation for CURLOPT_PROXY says this:

   CURLOPT_PROXY

   Set HTTP proxy to use. The parameter should be a char * to a zero
   terminated string holding the host name or dotted IP address. To
   specify port number in this string, append :[port] to the end of the
   host name. The proxy string may be prefixed with [protocol]:// since
   any such prefix will be ignored. The proxy's port number may optionally
   be specified with the separate option. If not specified, libcurl will
   default to using port 1080 for proxies. CURLOPT_PROXYPORT.

If the user has been happily using "127.0.0.1:4321" in curl_http_proxy (i.e. without the meaningless <proto>:// part), the original code would have called curl_easy_setopt with that string, and that would have been how everything used to work.

If you haven't figured out proxy_auth.host at this point in the codepath, you call credential_from_url() but the function only knows how to parse the value for

	"<proto>://[<user>[:<pass>]@]<host>[:<port>]/..."

Specifically, it will punt with anything without "://" in it.

And then you use proxy_auth.protocol and proxy_auth.host to build proxyhost.buf that presumably mimick the original curl_http_proxy (but without the credential part).

I haven't formed an opinion on what the proper solution should be, but either the credential_from_url() function needs to be updated to accept the scp style [user@]<host>:<port> argument, or this specific caller should take the responsibility to do special case the syntax.

Show 5 quoted lines

>  		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +		set_proxy_auth(result);
>  	}
>  
>  	return result;