Thanks for your feedback!
> -----Original Message-----
> From: Junio C Hamano [mailto:jch2...@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.to...@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...
I apologize for that. I misunderstood part of the instructions for submitting
patches.
> > 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.
How about this?
"
We set CURLOPT_PROXYAUTH to use the most secure authentication method available
only when the user has set configuration variables to specify a proxy. However,
libcurl also supports specifying a proxy through environment variables. In that
case libcurl defaults to only using the Basic proxy authentication method.
This change sets CURLOPT_PROXYAUTH to always use the most secure authentication
method available, even when there is no git configuration telling us to use a
proxy. This allows the user to use environment variables to configure a proxy
that requires an authentication method different from Basic.
"
> > Signed-off-by: Enrique A. Tobis <eto...@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.
You did guess correctly. As you said you are not an expert in this area, should
I wait until someone else chimes in or is this enough to resubmit for
inclusion? Assuming my revised explanation is acceptable, of course.
> Thanks.
Thank you!
Enrique
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
