RE: [PATCH] http: always use any proxy auth method available

> -----Original Message-----
> From: Junio C Hamano [mailto:jch2355@gmail.com] On Behalf Of Junio C
> Hamano
> Sent: Friday, June 26, 2015 15:25
> To: Enrique Tobis
> Cc: 'git@vger.kernel.org'; 'Nelson Benitez Leon'
> Subject: Re: [PATCH] http: always use any proxy auth method available
> 
> Enrique Tobis <Enrique.Tobis@twosigma.com> writes:
> 
> Thanks.  I wonder why this was addressed me directly (i.e. I am not an area
> expert, and I haven't seen this patch discussed here and reviewed by other
> people), but anyway...

> > By default, libcurl honors some environment variables that specify a
> > proxy (e.g. http_proxy, https_proxy). Also by default, libcurl will
> > only try to authenticate with a proxy using the Basic method.
> 
> OK, that is a statement of two facts.
> 
> What's missing here is what they relate to this change.  Are these two good
> things that we want to keep?  Are these bad things we need to tweak out by
> changing our software?  Or some combination?  Some third key information
> that is left untold?
> 
> > This
> > change makes libcurl always try the most secure proxy authentication
> > method available. As a consequence, you can use environment variables
> > to instruct git to use a proxy that uses an authentication method
> > different from Basic (e.g. Negotiate).
> 
> That is a worthy goal, but the description of the current problem seems
> lacking.  Perhaps you meant something like this:
> 
> 	We use CURLOPT_PROXYAUTH to ask for the most secure
>         authentication method with proxy only when we have
>         curl_http_proxy set, by http.proxy or remote.*.proxy
>         configuration variables.  However, libcurl also allows users
>         to use http proxies by setting some environment variables,
>         and by default the authentication with the proxy uses Basic
>         auth (unless specified with CURLOPT_PROXYAUTH, that is).
> 
> 	By always using CURLOPT_PROXYAUTH to ask for the most secure
> 	authentication method, even when we are not aware that we
> 	are using proxy (because there is no configuration that
> 	tells us so), we can allow users to tell libcurl to use
> 	a proxy with more secure method without setting http.proxy
>         or remote.*.proxy configuration variables.
> 
> But I am just guessing; as I said, I am not an expert in this area of the code.

> > Signed-off-by: Enrique A. Tobis <etobis@twosigma.com>
> > ---
> >  http.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/http.c b/http.c
> > index f0c5bbc..e9c6fdd 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -416,10 +416,10 @@ static CURL *get_curl_handle(void)
> >
> >  	if (curl_http_proxy) {
> >  		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> > +	}
> >  #if LIBCURL_VERSION_NUM >= 0x070a07
> 
> The authoritative source of truth:
> 
>   https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-
> versions
> 
> matches this version number, so there is nothing wrong per-se on this line,
> but it makes me wonder why we didn't do
> 
> 	#ifdef CURLOPT_PROXYAUTH
> 
> instead.  That's not something that should be changed with this change,
> though.
> 
> > -		curl_easy_setopt(result, CURLOPT_PROXYAUTH,
> CURLAUTH_ANY);
> > +	curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> 
> >  #endif
> > -	}
> >
> >  	set_curl_keepalive(result);
> 
> Assuming that I guessed your justification for this change corretly in the
> earlier part of this message, I think the change makes sense.