Thanks for your feedback! > -----Original Message----- > From: Junio C Hamano [mailto:jch2355@xxxxxxxxx] On Behalf Of Junio C > Hamano > Sent: Friday, June 26, 2015 15:25 > To: Enrique Tobis > Cc: 'git@xxxxxxxxxxxxxxx'; 'Nelson Benitez Leon' > Subject: Re: [PATCH] http: always use any proxy auth method available > > Enrique Tobis <Enrique.Tobis@xxxxxxxxxxxx> 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 <etobis@xxxxxxxxxxxx> > > --- > > 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html