{"thread":{"id":"29798","subject":"[PATCH v2 1/3]http: authenticate on NTLM proxies and others suppported by CuRL","startedAt":"2012-03-01T18:19:38Z","lastAt":"2012-03-05T15:33:16Z","messageCount":5,"participants":["Nelson Benitez Leon","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"185831","messageId":"4F4FBDBA.8040609@seap.minhap.es","threadId":"29798","inReplyTo":null,"subject":"[PATCH v2 1/3]http: authenticate on NTLM proxies and others suppported by CuRL","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-01T18:19:38Z","receivedAt":"2012-03-01T18:19:38Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"CURLAUTH_ANY option automatically chooses the best auth method\nfrom among those the server supports, that means curl\nwill ask the proxy and use the appropiate, and it will only do that if\nyou are using a proxy (i.e. you've set CURLOPT_PROXY or you have http_proxy\nenv var), also curl will not try to authenticate if you've not provided\nusername or password in the proxy string, as told here[1]..\n\nso, setting CURLOPT_PROXYAUTH = CURLAUTH_ANY will not affect the speed of\nnormal curl use, only if 1) you are using a proxy and 2) your proxy requires\nauthentication, only then curl will just make two or three roundtrips to find out\nthe auth methods the proxy is using, that is a tiny cost compared to having the\nuser find out the proxy auth type and set manually a specific config option to\nenable that type.\n\nSo as CURLAUTH_ANY provide us out-of-the-box proxy support without affecting speed,\nwe don't want it activated manually from a config option, instead we added it\nautomatically when a proxy is being used.\n\n[1] https://bugzilla.redhat.com/show_bug.cgi?id=769254#c6\n\nSigned-off-by: Nelson Benitez Leon \n---\n http.c | 4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 0ffd79c..8ac8eb6 100644\n--- a/http.c\n+++ b/http.c\n@@ -295,8 +295,10 @@ static CURL *get_curl_handle(void)\n \tif (curl_ftp_no_epsv)\n \t\tcurl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);\n \n-\tif (curl_http_proxy)\n+\tif (curl_http_proxy) {\n \t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n+\t}\n \n \treturn result;\n }\n-- \n1.7.7.6\n"},{"id":"185840","messageId":"7v399snnrs.fsf@alter.siamese.dyndns.org","threadId":"29798","inReplyTo":"4F4FBDBA.8040609@seap.minhap.es","subject":"Re: [PATCH v2 1/3]http: authenticate on NTLM proxies and others suppported by CuRL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-01T19:07:19Z","receivedAt":"2012-03-01T19:07:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nelson Benitez Leon writes:\n\nThanks; doesn't a missing space before http: above look ugly to you, by\nthe way?\n\n> CURLAUTH_ANY option automatically chooses the best auth method from\n> among those the server supports, that means curl will ask the proxy and\n> use the appropiate, and it will only do that if you are using a proxy\n> (i.e. you've set CURLOPT_PROXY or you have http_proxy env var), also\n> curl will not try to authenticate if you've not provided username or\n> password in the proxy string, as told here[1]..\n\nThe above may justify why you used CURLAUTH_ANY as opposed to\nCURLAUTH_BASIC or other types, but without any description of the problem\nyou are trying to solve before that paragraph, it does not justify why you\nare adding a code to use CURLOPT_PROXYAUTH in the first place.\n\nThis is my *guess* of the problem you are trying to solve.\n\n\tWhen the proxy server specified by the http.proxy configuration or\n\tthe http_proxy environment variable requires authentication, git\n\tfailed to connect to the proxy, because we did not configure the\n\tcURL handle with CURLOPT_PROXYAUTH.\n\nIt may or may not match the reality, but either case, the explanation\nshould have something like that at the beginning of the log.\n\nInstead of \"as told here[1]..\", I would have preferred for _you_ to say \"I\nran with this patch against a proxy that requires authentication and\nanother that does not, and made sure that there was no prompt or any extra\nnetwork traffic when no username or password is in in the proxy URL to\ntrigger the authentication\", or something like that. The item you refer\nto with your \"as told here[1]..\" only has this to say on this issue:\n\n I don't think PROXYAUTH=HTTP_ANY must be conditional. curl parses the\n proxy string, and if there's no user/pass, I'm quite sure it will ignore\n PROXYAUTH anyway.\n\nwhich is *not* convincing enough to be worth even referring to. It is\njust expressing one person's educated guess.\n\n> so, setting CURLOPT_PROXYAUTH = CURLAUTH_ANY will not affect the speed of\n> normal curl use, only if 1) you are using a proxy and 2) your proxy requires\n> authentication, only then curl will just make two or three roundtrips to find out\n> the auth methods the proxy is using, that is a tiny cost compared to having the\n> user find out the proxy auth type and set manually a specific config option to\n> enable that type.\n>\n> So as CURLAUTH_ANY provide us out-of-the-box proxy support without affecting speed,\n> we don't want it activated manually from a config option, instead we added it\n> automatically when a proxy is being used.\n\nI think you are discussing the latency (not the speed) here. Also the\nfirst few sentences are very hard to parse. Perhaps you meant to say\nsomething like this?\n\n When a proxy is in use, and you tell git that the proxy requires\n\tauthentication by having username in the http.proxy configuration,\n\tan extra request needs to be made to the proxy to find out what\n\tauthentication method it supports, as this patch uses CURLAUTH_ANY\n\tto let the library pick the most secure method supported by the\n\tproxy server.\n\n The extra round-trip adds extra latency, but relieves the user\n\tfrom the burden to configure a specific authentication method. If\n\tit becomes problem, a later patch could add a configuration option\n\tto specify what method to use, but let's start simple for the time\n\tbeing.\n"},{"id":"185916","messageId":"4F50D16D.3090800@seap.minhap.es","threadId":"29798","inReplyTo":"7v399snnrs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/3]http: authenticate on NTLM proxies and others suppported by CuRL","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-02T13:55:57Z","receivedAt":"2012-03-02T13:55:57Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 03/01/2012 08:07 PM, Junio C Hamano wrote:\n> Nelson Benitez Leon writes:\n> \n> Thanks; doesn't a missing space before http: above look ugly to you, by\n> the way?\n> \n>> CURLAUTH_ANY option automatically chooses the best auth method from\n>> among those the server supports, that means curl will ask the proxy and\n>> use the appropiate, and it will only do that if you are using a proxy\n>> (i.e. you've set CURLOPT_PROXY or you have http_proxy env var), also\n>> curl will not try to authenticate if you've not provided username or\n>> password in the proxy string, as told here[1]..\n> \n> The above may justify why you used CURLAUTH_ANY as opposed to\n> CURLAUTH_BASIC or other types, but without any description of the problem\n> you are trying to solve before that paragraph, it does not justify why you\n> are adding a code to use CURLOPT_PROXYAUTH in the first place.\n> \n> This is my *guess* of the problem you are trying to solve.\n\nI've ammended the commit message with your wording, text as follows:\n\nWhen the proxy server specified by the http.proxy configuration or\nthe http_proxy environment variable requires authentication, git\nfailed to connect to the proxy, because we did not configure the\ncURL handle with CURLOPT_PROXYAUTH.\n\nWhen a proxy is in use, and you tell git that the proxy requires\nauthentication by having username in the http.proxy configuration,\nan extra request needs to be made to the proxy to find out what\nauthentication method it supports, as this patch uses CURLAUTH_ANY\nto let the library pick the most secure method supported by the\nproxy server.\n\nThe extra round-trip adds extra latency, but relieves the user\nfrom the burden to configure a specific authentication method. If\nit becomes problem, a later patch could add a configuration option\nto specify what method to use, but let's start simple for the time\nbeing.\n\nSo as CURLAUTH_ANY provide us out-of-the-box proxy support, we don't\nwant it activated manually from a config option, instead we added it\nautomatically when a proxy is being used.\n\nSigned-off-by: Nelson Benitez Leon \n---\n http.c | 4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 0ffd79c..8ac8eb6 100644\n--- a/http.c\n+++ b/http.c\n@@ -295,8 +295,10 @@ static CURL *get_curl_handle(void)\n \tif (curl_ftp_no_epsv)\n \t\tcurl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);\n \n-\tif (curl_http_proxy)\n+\tif (curl_http_proxy) {\n \t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n+\t}\n \n \treturn result;\n }\n-- \n1.7.7.6\n"},{"id":"185939","messageId":"7v4nu6et1x.fsf@alter.siamese.dyndns.org","threadId":"29798","inReplyTo":"4F50D16D.3090800@seap.minhap.es","subject":"Re: [PATCH v2 1/3]http: authenticate on NTLM proxies and others suppported by CuRL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-02T18:50:18Z","receivedAt":"2012-03-02T18:50:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nelson Benitez Leon writes:\n\nIs there anything specific about NTLM in this patch? Let's retitle it to\nsomething like\n\n\thttp: support proxies that needs authentication\n\n> ...\n> So as CURLAUTH_ANY provide us out-of-the-box proxy support, we don't\n> want it activated manually from a config option, instead we added it\n> automatically when a proxy is being used.\n\nI can sort of parse this paragraph but cannot follow the logic.\n\n - Because we use CURLAUTH_ANY, a proxy that requires authentication is\n supported out-of-the-box.\n\n - We do not want having to manually enable it.\n\n - Instead of adding a manual configuration, we do so automatically.\n\nIt sounds like it is saying the same thing three times. Do we even need\nthat paragraph? Shouldn't we drop it instead?\n\nOtherwise it all looks good, so you can either\n\n (1) explain why the above suggestions are wrong and why I should apply\n the message I am responding to as-is;\n\n (2) send another re-roll; or\n\n (3) say \"yeah, your suggestions all sound good to me\", in which case I'll\n just locally amend and apply.\n\nThanks.\n\n>\n> Signed-off-by: Nelson Benitez Leon \n> ---\n> http.c | 4 +++-\n> 1 files changed, 3 insertions(+), 1 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index 0ffd79c..8ac8eb6 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -295,8 +295,10 @@ static CURL *get_curl_handle(void)\n> \tif (curl_ftp_no_epsv)\n> \t\tcurl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);\n> \n> -\tif (curl_http_proxy)\n> +\tif (curl_http_proxy) {\n> \t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n> +\t}\n> \n> \treturn result;\n> }\n"},{"id":"186118","messageId":"4F54DCBC.6080307@seap.minhap.es","threadId":"29798","inReplyTo":"7v4nu6et1x.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/3]http: authenticate on NTLM proxies and others suppported by CuRL","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-05T15:33:16Z","receivedAt":"2012-03-05T15:33:16Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 03/02/2012 07:50 PM, Junio C Hamano wrote:\n> Nelson Benitez Leon writes:\n> \n> Is there anything specific about NTLM in this patch? Let's retitle it to\n> something like\n> \n> \thttp: support proxies that needs authentication\n> \n>> ...\n>> So as CURLAUTH_ANY provide us out-of-the-box proxy support, we don't\n>> want it activated manually from a config option, instead we added it\n>> automatically when a proxy is being used.\n> \n> I can sort of parse this paragraph but cannot follow the logic.\n> \n> - Because we use CURLAUTH_ANY, a proxy that requires authentication is\n> supported out-of-the-box.\n> \n> - We do not want having to manually enable it.\n> \n> - Instead of adding a manual configuration, we do so automatically.\n> \n> It sounds like it is saying the same thing three times. Do we even need\n> that paragraph? Shouldn't we drop it instead?\n\nI've dropped it in v3 patchset.\n\n> \n> Otherwise it all looks good, so you can either\n> \n> (1) explain why the above suggestions are wrong and why I should apply\n> the message I am responding to as-is;\n> \n> (2) send another re-roll; or\n> \n> (3) say \"yeah, your suggestions all sound good to me\", in which case I'll\n> just locally amend and apply.\n> \n\nI've re-rolled a new patchset and included your suggestions about wording,\nyou're welcome to better fine-tune the messages and commit if you find them\nsuitable.\n\nThanks!\n"}]}