{"thread":{"id":"29838","subject":"[PATCH v3 2/4] http: try http_proxy env var when http.proxy config option is not set","startedAt":"2012-03-05T15:17:48Z","lastAt":"2012-03-06T15:58:44Z","messageCount":6,"participants":["Nelson Benitez Leon","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"186113","messageId":"4F54D91C.6080905@seap.minhap.es","threadId":"29838","inReplyTo":null,"subject":"[PATCH v3 2/4] http: try http_proxy env var when http.proxy config option is not set","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-05T15:17:48Z","receivedAt":"2012-03-05T15:17:48Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"cURL already reads it, but if $http_proxy has username but no password\ncURL will not ask you for the password and so failed to authenticate\nreturning a 407 error code. So we read it ourselves to detect that and\nask for the password. Also we read it prior to connection to be able to\nmake a proactive authentication in case the flag http_proactive_auth is\nset.\n\nSigned-off-by: Nelson Benitez Leon \n---\n http.c | 7 +++++++\n 1 files changed, 7 insertions(+), 0 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 8ac8eb6..8932da5 100644\n--- a/http.c\n+++ b/http.c\n@@ -295,6 +295,13 @@ 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+\t\tconst char *env_proxy;\n+\t\tenv_proxy = getenv(\"http_proxy\");\n+\t\tif (env_proxy) {\n+\t\t\tcurl_http_proxy = xstrdup(env_proxy);\n+\t\t}\n+\t}\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-- \n1.7.7.6\n"},{"id":"186125","messageId":"7v4nu32bwp.fsf@alter.siamese.dyndns.org","threadId":"29838","inReplyTo":"4F54D91C.6080905@seap.minhap.es","subject":"Re: [PATCH v3 2/4] http: try http_proxy env var when http.proxy config option is not set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-05T17:30:30Z","receivedAt":"2012-03-05T17:30:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nelson Benitez Leon writes:\n\n> cURL already reads it, but if $http_proxy has username but no password\n> cURL will not ask you for the password and so failed to authenticate\n> returning a 407 error code. So we read it ourselves to detect that and\n> ask for the password. Also we read it prior to connection to be able to\n> make a proactive authentication in case the flag http_proactive_auth is\n> set.\n>\n> Signed-off-by: Nelson Benitez Leon \n> ---\n\nThe above does not address my earlier worries about compatibility\nacross curl applications (it does not even say \"it does not matter;\nwe do not care about other's application\"), so let's spell it out\nagain. When the user has\n\n\thttp_proxy=http://me@proxy.example.com\n\texport http_proxy\n\nwith your patch, git might do whatever we desire to do, and the end\nresult may be better, but how would the user experience be for the\nuser when using other curl based programs on the same system?\n\nAlso I thought the conclusion from the other thread was that even if\nwe were to do this, we should apply the http_proxy environment only\nwhen we are talking to http:// and for https:// we would instead\nread HTTPS_PROXY or something?\n\n> http.c | 7 +++++++\n> 1 files changed, 7 insertions(+), 0 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index 8ac8eb6..8932da5 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -295,6 +295,13 @@ 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> +\t\tconst char *env_proxy;\n> +\t\tenv_proxy = getenv(\"http_proxy\");\n> +\t\tif (env_proxy) {\n> +\t\t\tcurl_http_proxy = xstrdup(env_proxy);\n> +\t\t}\n> +\t}\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"},{"id":"186201","messageId":"20120306112745.GA6733@sigill.intra.peff.net","threadId":"29838","inReplyTo":"4F560196.8070500@seap.minhap.es","subject":"Re: [PATCH v3 2/4] http: try http_proxy env var when http.proxy config option is not set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-06T11:27:45Z","receivedAt":"2012-03-06T11:27:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 06, 2012 at 01:22:46PM +0100, Nelson Benitez Leon wrote:\n\n> > Also I thought the conclusion from the other thread was that even if\n> > we were to do this, we should apply the http_proxy environment only\n> > when we are talking to http:// and for https:// we would instead\n> > read HTTPS_PROXY or something?\n> \n> Ok I completely miss this, can this be added in a later patch?\n\nHmm. Your current series munges the curl_http_proxy variable in order to\nput the username and password in, and therefore needs to know what is in\nthe proxy variable.\n\nBut if you switch patch 4/4 to set CURLOPT_PROXYUSERPWD, then we won't\nneed to care what's in curl_http_proxy, no? We will get a 407 from curl\nbecause curl detected the proxy (either from the environment, or because\nwe actually told it via curl_http_proxy), and then we will fill in the\nusername and password without touching the actual proxy URL.\n\nSo this patch can just be dropped at that point, right?\n\n-Peff\n"},{"id":"186200","messageId":"4F560196.8070500@seap.minhap.es","threadId":"29838","inReplyTo":"7v4nu32bwp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/4] http: try http_proxy env var when http.proxy config option is not set","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-06T12:22:46Z","receivedAt":"2012-03-06T12:22:46Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 03/05/2012 06:30 PM, Junio C Hamano wrote:\n> Nelson Benitez Leon writes:\n> \n>> cURL already reads it, but if $http_proxy has username but no password\n>> cURL will not ask you for the password and so failed to authenticate\n>> returning a 407 error code. So we read it ourselves to detect that and\n>> ask for the password. Also we read it prior to connection to be able to\n>> make a proactive authentication in case the flag http_proactive_auth is\n>> set.\n>>\n>> Signed-off-by: Nelson Benitez Leon \n>> ---\n> \n> The above does not address my earlier worries about compatibility\n> across curl applications (it does not even say \"it does not matter;\n> we do not care about other's application\"), so let's spell it out\n> again. When the user has\n> \n> \thttp_proxy=http://me@proxy.example.com\n> \texport http_proxy\n> \n> with your patch, git might do whatever we desire to do, and the end\n> result may be better, but how would the user experience be for the\n> user when using other curl based programs on the same system?\n\nAs I said in the commit message, in that cases a 407 error will be\nreturned from those applications, because curl will not ask you for\nthe password, I tested that myself, maybe my message is not clear\nenough, you can suggest me a better wording or feel free to amend it\nyourself :-). \n\nAn $http_proxy without password means it needs support from the application,\nI expect users who put a proxy url without password to be confident that \nthe application they're using will ask them for the password (git in this\ncase), if they're not sure they have to go with the standard approach of\nsetting both username and password in $http_proxy to obtain full\ncompatibility amongst any programs (at the cost of having the password\nwritten down somewhere).\n\n> \n> Also I thought the conclusion from the other thread was that even if\n> we were to do this, we should apply the http_proxy environment only\n> when we are talking to http:// and for https:// we would instead\n> read HTTPS_PROXY or something?\n\nOk I completely miss this, can this be added in a later patch?\n\n> \n>> http.c | 7 +++++++\n>> 1 files changed, 7 insertions(+), 0 deletions(-)\n>>\n>> diff --git a/http.c b/http.c\n>> index 8ac8eb6..8932da5 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -295,6 +295,13 @@ 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>> +\t\tconst char *env_proxy;\n>> +\t\tenv_proxy = getenv(\"http_proxy\");\n>> +\t\tif (env_proxy) {\n>> +\t\t\tcurl_http_proxy = xstrdup(env_proxy);\n>> +\t\t}\n>> +\t}\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> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"186207","messageId":"4F561A6F.2040206@seap.minhap.es","threadId":"29838","inReplyTo":"20120306112745.GA6733@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/4] http: try http_proxy env var when http.proxy config option is not set","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-06T14:08:47Z","receivedAt":"2012-03-06T14:08:47Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 03/06/2012 12:27 PM, Jeff King wrote:\n> On Tue, Mar 06, 2012 at 01:22:46PM +0100, Nelson Benitez Leon wrote:\n> \n>>> Also I thought the conclusion from the other thread was that even if\n>>> we were to do this, we should apply the http_proxy environment only\n>>> when we are talking to http:// and for https:// we would instead\n>>> read HTTPS_PROXY or something?\n>>\n>> Ok I completely miss this, can this be added in a later patch?\n> \n> Hmm. Your current series munges the curl_http_proxy variable in order to\n> put the username and password in, and therefore needs to know what is in\n> the proxy variable.\n> \n> But if you switch patch 4/4 to set CURLOPT_PROXYUSERPWD, then we won't\n> need to care what's in curl_http_proxy, no? We will get a 407 from curl\n> because curl detected the proxy (either from the environment, or because\n> we actually told it via curl_http_proxy), and then we will fill in the\n> username and password without touching the actual proxy URL.\n> \n> So this patch can just be dropped at that point, right?\n\nI think so, I will try 4/4 using proxyuserpwd.\n"},{"id":"186222","messageId":"7vboo9vhzf.fsf@alter.siamese.dyndns.org","threadId":"29838","inReplyTo":"4F560196.8070500@seap.minhap.es","subject":"Re: [PATCH v3 2/4] http: try http_proxy env var when http.proxy config option is not set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-06T15:58:44Z","receivedAt":"2012-03-06T15:58:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nelson Benitez Leon writes:\n\n> As I said in the commit message, in that cases a 407 error will be\n> returned from those applications, because curl will not ask you for\n> the password,...\n\nIf that is the case, people cannot write their username in the\nhttp_proxy environment variable---otherwise they will not be able to\nuse these applications. If people do not have username in that\nenvironment, this patch will not help us.\n\nI think I saw you and Peff discussing down-thread that this patch\nmay not be needed, so if that is the case it is good---we won't have\nto worry about it.\n\nThanks.\n"}]}