{"thread":{"id":"39733","subject":"[PATCH] http: always use any proxy auth method available","startedAt":"2015-06-26T18:19:04Z","lastAt":"2015-06-29T17:00:23Z","messageCount":4,"participants":["Enrique Tobis","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"264983","messageId":"FCAB894186380D42A07AFFFA5A1282B8F1EC65FD@EXMBNJE2.ad.twosigma.com","threadId":"39733","inReplyTo":null,"subject":"[PATCH] http: always use any proxy auth method available","fromName":"Enrique Tobis","fromEmail":"enrique.tobis@twosigma.com","sentAt":"2015-06-26T18:19:04Z","receivedAt":"2015-06-26T18:19:04Z","isPatch":true,"sender":{"key":"enrique.tobis@twosigma.com","avatar":"https://avatars.githubusercontent.com/u/940401?v=4"},"body":"By default, libcurl honors some environment variables that specify a\nproxy (e.g. http_proxy, https_proxy). Also by default, libcurl will\nonly try to authenticate with a proxy using the Basic method. This\nchange makes libcurl always try the most secure proxy authentication\nmethod available. As a consequence, you can use environment variables\nto instruct git to use a proxy that uses an authentication method\ndifferent from Basic (e.g. Negotiate).\n\nSigned-off-by: Enrique A. Tobis \n---\n http.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex f0c5bbc..e9c6fdd 100644\n--- a/http.c\n+++ b/http.c\n@@ -416,10 +416,10 @@ static CURL *get_curl_handle(void)\n \n \tif (curl_http_proxy) {\n \t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n+\t}\n #if LIBCURL_VERSION_NUM >= 0x070a07\n-\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n+\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n #endif\n-\t}\n \n \tset_curl_keepalive(result);\n \n-- \n1.7.3\n"},{"id":"264987","messageId":"xmqqfv5etgqt.fsf@gitster.dls.corp.google.com","threadId":"39733","inReplyTo":"FCAB894186380D42A07AFFFA5A1282B8F1EC65FD@EXMBNJE2.ad.twosigma.com","subject":"Re: [PATCH] http: always use any proxy auth method available","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-26T19:24:42Z","receivedAt":"2015-06-26T19:24:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Enrique Tobis writes:\n\nThanks. I wonder why this was addressed me directly (i.e. I am not\nan area expert, and I haven't seen this patch discussed here and\nreviewed by other people), but anyway...\n\n> By default, libcurl honors some environment variables that specify a\n> proxy (e.g. http_proxy, https_proxy). Also by default, libcurl will\n> only try to authenticate with a proxy using the Basic method. \n\nOK, that is a statement of two facts.\n\nWhat's missing here is what they relate to this change. Are these\ntwo good things that we want to keep? Are these bad things we need\nto tweak out by changing our software? Or some combination? Some\nthird key information that is left untold?\n\n> This\n> change makes libcurl always try the most secure proxy authentication\n> method available. As a consequence, you can use environment variables\n> to instruct git to use a proxy that uses an authentication method\n> different from Basic (e.g. Negotiate).\n\nThat is a worthy goal, but the description of the current problem\nseems lacking. Perhaps you meant something like this:\n\n\tWe use CURLOPT_PROXYAUTH to ask for the most secure\n authentication method with proxy only when we have\n curl_http_proxy set, by http.proxy or remote.*.proxy\n configuration variables. However, libcurl also allows users\n to use http proxies by setting some environment variables,\n and by default the authentication with the proxy uses Basic\n auth (unless specified with CURLOPT_PROXYAUTH, that is).\n\n\tBy always using CURLOPT_PROXYAUTH to ask for the most secure\n\tauthentication method, even when we are not aware that we\n\tare using proxy (because there is no configuration that\n\ttells us so), we can allow users to tell libcurl to use\n\ta proxy with more secure method without setting http.proxy\n or remote.*.proxy configuration variables.\n\nBut I am just guessing; as I said, I am not an expert in this area\nof the code.\n\n> Signed-off-by: Enrique A. Tobis \n> ---\n> http.c | 4 ++--\n> 1 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index f0c5bbc..e9c6fdd 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -416,10 +416,10 @@ static CURL *get_curl_handle(void)\n> \n> \tif (curl_http_proxy) {\n> \t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n> +\t}\n> #if LIBCURL_VERSION_NUM >= 0x070a07\n\nThe authoritative source of truth:\n\n https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions\n\nmatches this version number, so there is nothing wrong per-se on\nthis line, but it makes me wonder why we didn't do\n\n\t#ifdef CURLOPT_PROXYAUTH\n\ninstead. That's not something that should be changed with this\nchange, though.\n\n> -\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n> +\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n\n> #endif\n> -\t}\n> \n> \tset_curl_keepalive(result);\n\nAssuming that I guessed your justification for this change corretly\nin the earlier part of this message, I think the change makes sense.\n\nThanks.\n"},{"id":"265006","messageId":"FCAB894186380D42A07AFFFA5A1282B8F1EC7AB5@EXMBNJE2.ad.twosigma.com","threadId":"39733","inReplyTo":"xmqqfv5etgqt.fsf@gitster.dls.corp.google.com","subject":"RE: [PATCH] http: always use any proxy auth method available","fromName":"Enrique Tobis","fromEmail":"enrique.tobis@twosigma.com","sentAt":"2015-06-26T21:53:24Z","receivedAt":"2015-06-26T21:53:24Z","isPatch":true,"sender":{"key":"enrique.tobis@twosigma.com","avatar":"https://avatars.githubusercontent.com/u/940401?v=4"},"body":"Thanks for your feedback!\n\n> -----Original Message-----\n> From: Junio C Hamano [mailto:jch2355@gmail.com] On Behalf Of Junio C\n> Hamano\n> Sent: Friday, June 26, 2015 15:25\n> To: Enrique Tobis\n> Cc: 'git@vger.kernel.org'; 'Nelson Benitez Leon'\n> Subject: Re: [PATCH] http: always use any proxy auth method available\n> \n> Enrique Tobis writes:\n> \n> Thanks. I wonder why this was addressed me directly (i.e. I am not an area\n> expert, and I haven't seen this patch discussed here and reviewed by other\n> people), but anyway...\n\nI apologize for that. I misunderstood part of the instructions for submitting patches.\n\n> > By default, libcurl honors some environment variables that specify a\n> > proxy (e.g. http_proxy, https_proxy). Also by default, libcurl will\n> > only try to authenticate with a proxy using the Basic method.\n> \n> OK, that is a statement of two facts.\n> \n> What's missing here is what they relate to this change. Are these two good\n> things that we want to keep? Are these bad things we need to tweak out by\n> changing our software? Or some combination? Some third key information\n> that is left untold?\n> \n> > This\n> > change makes libcurl always try the most secure proxy authentication\n> > method available. As a consequence, you can use environment variables\n> > to instruct git to use a proxy that uses an authentication method\n> > different from Basic (e.g. Negotiate).\n> \n> That is a worthy goal, but the description of the current problem seems\n> lacking. Perhaps you meant something like this:\n> \n> \tWe use CURLOPT_PROXYAUTH to ask for the most secure\n> authentication method with proxy only when we have\n> curl_http_proxy set, by http.proxy or remote.*.proxy\n> configuration variables. However, libcurl also allows users\n> to use http proxies by setting some environment variables,\n> and by default the authentication with the proxy uses Basic\n> auth (unless specified with CURLOPT_PROXYAUTH, that is).\n> \n> \tBy always using CURLOPT_PROXYAUTH to ask for the most secure\n> \tauthentication method, even when we are not aware that we\n> \tare using proxy (because there is no configuration that\n> \ttells us so), we can allow users to tell libcurl to use\n> \ta proxy with more secure method without setting http.proxy\n> or remote.*.proxy configuration variables.\n> \n> But I am just guessing; as I said, I am not an expert in this area of the code.\n\nHow about this?\n\n\"\nWe 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.\n\nThis 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.\n\"\n\n> > Signed-off-by: Enrique A. Tobis \n> > ---\n> > http.c | 4 ++--\n> > 1 files changed, 2 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/http.c b/http.c\n> > index f0c5bbc..e9c6fdd 100644\n> > --- a/http.c\n> > +++ b/http.c\n> > @@ -416,10 +416,10 @@ static CURL *get_curl_handle(void)\n> >\n> > \tif (curl_http_proxy) {\n> > \t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n> > +\t}\n> > #if LIBCURL_VERSION_NUM >= 0x070a07\n> \n> The authoritative source of truth:\n> \n> https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-\n> versions\n> \n> matches this version number, so there is nothing wrong per-se on this line,\n> but it makes me wonder why we didn't do\n> \n> \t#ifdef CURLOPT_PROXYAUTH\n> \n> instead. That's not something that should be changed with this change,\n> though.\n> \n> > -\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH,\n> CURLAUTH_ANY);\n> > +\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n> \n> > #endif\n> > -\t}\n> >\n> > \tset_curl_keepalive(result);\n> \n> Assuming that I guessed your justification for this change corretly in the\n> earlier part of this message, I think the change makes sense.\n\nYou 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.\n \n> Thanks.\n\nThank you!\n\nEnrique\n"},{"id":"265144","messageId":"xmqqzj3ibgbc.fsf@gitster.dls.corp.google.com","threadId":"39733","inReplyTo":"FCAB894186380D42A07AFFFA5A1282B8F1EC7AB5@EXMBNJE2.ad.twosigma.com","subject":"Re: [PATCH] http: always use any proxy auth method available","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-29T17:00:23Z","receivedAt":"2015-06-29T17:00:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Enrique Tobis writes:\n\n> You did guess correctly. As you said you are not an expert in this\n> area, should I wait until someone else chimes in or is this enough to\n> resubmit for inclusion? Assuming my revised explanation is acceptable,\n> of course.\n\nI'll queue the patch as-is with your updated log message to explain\nit. Maybe somebody else has more comment on it, but I think the\nchange makes sense.\n\nThanks.\n"}]}