{"thread":{"id":"30408","subject":"[PATCH 2/6] http: handle proxy proactive authentication","startedAt":"2012-05-03T16:39:54Z","lastAt":"2012-05-04T13:55:47Z","messageCount":6,"participants":["Nelson Benitez Leon","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"190646","messageId":"4FA2B4DA.60908@seap.minhap.es","threadId":"30408","inReplyTo":null,"subject":"[PATCH 2/6] http: handle proxy proactive authentication","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-05-03T16:39:54Z","receivedAt":"2012-05-03T16:39:54Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"If http_proactive_auth flag is set and there is a username\nbut no password in the proxy url, then interactively ask for\nthe password.\n\nThis makes possible to not have the password written down in\nhttp_proxy env var or in http.proxy config option.\n\nAlso take care that CURLOPT_PROXY don't include username or\npassword, as we now set them in the new set_proxy_auth() function\nwhere we use their specific cURL options.\n\nSigned-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n http.c |   28 +++++++++++++++++++++++++++-\n 1 files changed, 27 insertions(+), 1 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 64df7b1..02f9fcd 100644\n--- a/http.c\n+++ b/http.c\n@@ -43,6 +43,7 @@ static int curl_ftp_no_epsv;\n static const char *curl_http_proxy;\n static const char *curl_cookie_file;\n static struct credential http_auth = CREDENTIAL_INIT;\n+static struct credential proxy_auth = CREDENTIAL_INIT;\n static int http_proactive_auth;\n static const char *user_agent;\n \n@@ -272,6 +273,20 @@ static int has_cert_password(void)\n \treturn 1;\n }\n \n+static void set_proxy_auth(CURL *result)\n+{\n+\tif (proxy_auth.username && proxy_auth.password) {\n+#if LIBCURL_VERSION_NUM >= 0x071301\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERNAME, proxy_auth.username);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYPASSWORD, proxy_auth.password);\n+#else\n+\t\tstruct strbuf userpwd = STRBUF_INIT;\n+\t\tstrbuf_addf(&userpwd, \"%s:%s\", proxy_auth.username, proxy_auth.password);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERPWD, strbuf_detach(&userpwd, NULL));\n+#endif\n+\t}\n+}\n+\n static CURL *get_curl_handle(const char *url)\n {\n \tCURL *result = curl_easy_init();\n@@ -351,8 +366,19 @@ static CURL *get_curl_handle(const char *url)\n \t}\n \t\n \tif (curl_http_proxy) {\n-\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n+\t\tstruct strbuf proxyhost = STRBUF_INIT;\n+\n+\t\tif (!proxy_auth.host) /* check to parse only once */\n+\t\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n+\n+\t\tif (http_proactive_auth && proxy_auth.username && !proxy_auth.password)\n+\t\t\t/* proxy string has username but no password, ask for password */\n+\t\t\tcredential_fill(&proxy_auth);\n+\n+\t\tstrbuf_addf(&proxyhost, \"%s://%s\", proxy_auth.protocol, proxy_auth.host);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXY, strbuf_detach(&proxyhost, NULL));\n \t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n+\t\tset_proxy_auth(result);\n \t}\n \n \treturn result;\n-- \n1.7.7.6\n"},{"id":"190727","messageId":"20120504071632.GB21895@sigill.intra.peff.net","threadId":"30408","inReplyTo":"4FA2B4DA.60908@seap.minhap.es","subject":"Re: [PATCH 2/6] http: handle proxy proactive authentication","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-04T07:16:32Z","receivedAt":"2012-05-04T07:16:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 03, 2012 at 06:39:54PM +0200, Nelson Benitez Leon wrote:\n\n> If http_proactive_auth flag is set and there is a username\n> but no password in the proxy url, then interactively ask for\n> the password.\n> \n> This makes possible to not have the password written down in\n> http_proxy env var or in http.proxy config option.\n> \n> Also take care that CURLOPT_PROXY don't include username or\n> password, as we now set them in the new set_proxy_auth() function\n> where we use their specific cURL options.\n\nDo we actually need to do that? If we set CURLOPT_PROXYUSERNAME, will\ncurl ignore it in favor of what's in the URL? I ask, because there is a\nbug here:\n\n> @@ -351,8 +366,19 @@ static CURL *get_curl_handle(const char *url)\n>  \t}\n>  \t\n>  \tif (curl_http_proxy) {\n> -\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n> +\t\tstruct strbuf proxyhost = STRBUF_INIT;\n> +\n> +\t\tif (!proxy_auth.host) /* check to parse only once */\n> +\t\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n> +\n> +\t\tif (http_proactive_auth && proxy_auth.username && !proxy_auth.password)\n> +\t\t\t/* proxy string has username but no password, ask for password */\n> +\t\t\tcredential_fill(&proxy_auth);\n> +\n> +\t\tstrbuf_addf(&proxyhost, \"%s://%s\", proxy_auth.protocol, proxy_auth.host);\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROXY, strbuf_detach(&proxyhost, NULL));\n\nWhen you parse the URL via credential_from_url, the components you get\nwill have any URL-encoding removed. So when you regenerate the URL in\nthe proxyhost variable, you would need to re-encode.\n\nBut if we can stop doing this regeneration at all, then the problem goes\naway.\n\nAlso, newer versions of curl will copy the string instead of taking\nownership of the pointer. Unfortunately we have to deal with both old\nand new, but you can get around it by using a static strbuf (so we leak,\nbut we only leak once per program, not once per get_curl_handle call).\nThis issue would also go away if we stop regenerating the URL.\n\n-Peff\n"},{"id":"190745","messageId":"20120504105106.GA24933@sigill.intra.peff.net","threadId":"30408","inReplyTo":"4FA3B92E.3000200@seap.minhap.es","subject":"Re: [PATCH 2/6] http: handle proxy proactive authentication","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-04T10:51:07Z","receivedAt":"2012-05-04T10:51:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 04, 2012 at 01:10:38PM +0200, Nelson Benitez Leon wrote:\n\n> > When you parse the URL via credential_from_url, the components you get\n> > will have any URL-encoding removed. So when you regenerate the URL in\n> > the proxyhost variable, you would need to re-encode.\n> \n> Can a hostname has url-encoded parts? I thought that was only for the\n> request uri (/somedir/somefile.php) or the query string ('?var1=val'),\n> I'm only using the hostname here as a proxy server never has more than\n> that, apart from the port number.\n\nHmm. It can have URL-encoded parts (so we must decode when parsing), but\nthe more important question is whether the decoded version can have\nparts that _need_ to be URL-encoded.  And I think the answer is no,\nafter double-checking the RFCs (i.e., hostnames cannot contain any of the URL\nreserved characters). So quoting would be a no-op, and we can skip it.\n\nAnyway, your later patch ends up removing this chunk of code, so I think\nwe can forget the issue entirely.\n\n-Peff\n"},{"id":"190744","messageId":"4FA3B92E.3000200@seap.minhap.es","threadId":"30408","inReplyTo":"20120504071632.GB21895@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] http: handle proxy proactive authentication","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-05-04T11:10:38Z","receivedAt":"2012-05-04T11:10:38Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 05/04/2012 09:16 AM, Jeff King wrote:\n> On Thu, May 03, 2012 at 06:39:54PM +0200, Nelson Benitez Leon wrote:\n> \n>> If http_proactive_auth flag is set and there is a username\n>> but no password in the proxy url, then interactively ask for\n>> the password.\n>>\n>> This makes possible to not have the password written down in\n>> http_proxy env var or in http.proxy config option.\n>>\n>> Also take care that CURLOPT_PROXY don't include username or\n>> password, as we now set them in the new set_proxy_auth() function\n>> where we use their specific cURL options.\n> \n> Do we actually need to do that? If we set CURLOPT_PROXYUSERNAME, will\n> curl ignore it in favor of what's in the URL? I ask, because there is a\n> bug here:\n> \n>> @@ -351,8 +366,19 @@ static CURL *get_curl_handle(const char *url)\n>>  \t}\n>>  \t\n>>  \tif (curl_http_proxy) {\n>> -\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n>> +\t\tstruct strbuf proxyhost = STRBUF_INIT;\n>> +\n>> +\t\tif (!proxy_auth.host) /* check to parse only once */\n>> +\t\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n>> +\n>> +\t\tif (http_proactive_auth && proxy_auth.username && !proxy_auth.password)\n>> +\t\t\t/* proxy string has username but no password, ask for password */\n>> +\t\t\tcredential_fill(&proxy_auth);\n>> +\n>> +\t\tstrbuf_addf(&proxyhost, \"%s://%s\", proxy_auth.protocol, proxy_auth.host);\n>> +\t\tcurl_easy_setopt(result, CURLOPT_PROXY, strbuf_detach(&proxyhost, NULL));\n> \n> When you parse the URL via credential_from_url, the components you get\n> will have any URL-encoding removed. So when you regenerate the URL in\n> the proxyhost variable, you would need to re-encode.\n\nCan a hostname has url-encoded parts? I thought that was only for the\nrequest uri (/somedir/somefile.php) or the query string ('?var1=val'),\nI'm only using the hostname here as a proxy server never has more than\nthat, apart from the port number.\n"},{"id":"190752","messageId":"20120504135514.GA29590@sigill.intra.peff.net","threadId":"30408","inReplyTo":"4FA3DFE3.5050702@seap.minhap.es","subject":"Re: [PATCH 2/6] http: handle proxy proactive authentication","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-04T13:55:15Z","receivedAt":"2012-05-04T13:55:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 04, 2012 at 03:55:47PM +0200, Nelson Benitez Leon wrote:\n\n> >> Also take care that CURLOPT_PROXY don't include username or\n> >> password, as we now set them in the new set_proxy_auth() function\n> >> where we use their specific cURL options.\n> > \n> > Do we actually need to do that? If we set CURLOPT_PROXYUSERNAME, will\n> > curl ignore it in favor of what's in the URL? \n> \n> I explicitly remove username/pass from CURLOPT_PROXY to not having to worry\n> about that question, to not provide cURL with two different sets of proxy auth\n> info, common sense dictates cURL specific proxy options should take precedence\n> over embedded in url by I haven't seen that mentioned by any cURL docs so we \n> should look at the source to know the truth but even then that could change in\n> the future so I think is safer to only provide one path for auth info.\n\nYes, I would expect the specific proxy options to take over. And that is\nwhat happens for the regular URL case, where we do not do any munging at\nall. I phrased my question as I did because that was the only set of\ncircumstances I could see where not munging the URL would _hurt_ us. In\nother words, I do not find it likely that it will hurt us to leave it\nintact.\n\nBut it may hurt us to munge it.  My concern is that we are adding a\nbunch of code to replicate how curl behaves (with respect to pulling the\nproxy information from the environment). If we leave the proxy URL\nuntouched, then if we fail to do the same thing as curl, the worst case\nis that we don't get the credential properly (if one is even necessary).\nBut if we do rewrite the proxy, then we are potentially screwing up what\ncurl would do, whether a credential would have been necessary or not.\n\nSo to me it is the lower-risk path to let curl do its regular thing\n(pulling the proxy from the environment), and just let us handle the\ncredential acquisition side of things. And it also is less code for us.\n\n> Having username/password on the CURLOPT_PROXY option gives us no special gain at\n> the cost of not permitting usernames with reserved characters like '@' or ':' which\n> are not unusual at all. So I'm inclined to preserve current set_proxy_auth() \n> function and re-introduce the code that sets CURLOPT_PROXY with only $prot://$host.\n> \n> Are you ok with this? or do you prefer I change set_proxy_auth() to a set_curl_proxy()\n> function where I embedded user/pass in CURLOPT_PROXY ? that is the remaining thing I need\n> to know to send a new re-roll.\n\nNo, I think you should leave CURLOPT_PROXY unset, unless you are giving\ncurl the verbatim URL given to us via git-config. Let our parsing be\nonly for credentials, and let curl handle everything else.\n\n-Peff\n"},{"id":"190747","messageId":"4FA3DFE3.5050702@seap.minhap.es","threadId":"30408","inReplyTo":"20120504071632.GB21895@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] http: handle proxy proactive authentication","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-05-04T13:55:47Z","receivedAt":"2012-05-04T13:55:47Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 05/04/2012 09:16 AM, Jeff King wrote:\n> On Thu, May 03, 2012 at 06:39:54PM +0200, Nelson Benitez Leon wrote:\n> \n>> If http_proactive_auth flag is set and there is a username\n>> but no password in the proxy url, then interactively ask for\n>> the password.\n>>\n>> This makes possible to not have the password written down in\n>> http_proxy env var or in http.proxy config option.\n>>\n>> Also take care that CURLOPT_PROXY don't include username or\n>> password, as we now set them in the new set_proxy_auth() function\n>> where we use their specific cURL options.\n> \n> Do we actually need to do that? If we set CURLOPT_PROXYUSERNAME, will\n> curl ignore it in favor of what's in the URL? \n\nI explicitly remove username/pass from CURLOPT_PROXY to not having to worry\nabout that question, to not provide cURL with two different sets of proxy auth\ninfo, common sense dictates cURL specific proxy options should take precedence\nover embedded in url by I haven't seen that mentioned by any cURL docs so we \nshould look at the source to know the truth but even then that could change in\nthe future so I think is safer to only provide one path for auth info.\n\nHaving username/password on the CURLOPT_PROXY option gives us no special gain at\nthe cost of not permitting usernames with reserved characters like '@' or ':' which\nare not unusual at all. So I'm inclined to preserve current set_proxy_auth() \nfunction and re-introduce the code that sets CURLOPT_PROXY with only $prot://$host.\n\nAre you ok with this? or do you prefer I change set_proxy_auth() to a set_curl_proxy()\nfunction where I embedded user/pass in CURLOPT_PROXY ? that is the remaining thing I need\nto know to send a new re-roll.\n"}]}