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

[Jeff King <peff@peff.net>]

On Fri, May 04, 2012 at 03:55:47PM +0200, Nelson Benitez Leon wrote:

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

> 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