{"thread":{"id":"29839","subject":"[PATCH v3 3/4] http: handle proxy proactive authentication","startedAt":"2012-03-05T15:19:40Z","lastAt":"2012-03-06T10:58:59Z","messageCount":5,"participants":["Nelson Benitez Leon","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"186114","messageId":"4F54D98C.2070909@seap.minhap.es","threadId":"29839","inReplyTo":null,"subject":"[PATCH v3 3/4] http: handle proxy proactive authentication","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-05T15:19:40Z","receivedAt":"2012-03-05T15:19:40Z","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\nSigned-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>\n---\n http.c |   12 ++++++++++++\n 1 files changed, 12 insertions(+), 0 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 8932da5..b0b4362 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@@ -303,6 +304,17 @@ static CURL *get_curl_handle(void)\n \t\t}\n \t}\n \tif (curl_http_proxy) {\n+\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\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\tstruct strbuf pbuf = STRBUF_INIT;\n+\t\t\tcredential_fill(&proxy_auth);\n+\t\t\tstrbuf_addf(&pbuf, \"%s://%s:%s@%s\",proxy_auth.protocol,\n+\t\t\t    \tproxy_auth.username, proxy_auth.password,\n+\t\t\t    \tproxy_auth.host);\n+\t\t\tfree ((void *)curl_http_proxy);\n+\t\t\tcurl_http_proxy =  strbuf_detach(&pbuf, NULL);\n+\t\t}\n \t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n \t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n \t}\n-- \n1.7.7.6\n"},{"id":"186179","messageId":"20120306083052.GD21199@sigill.intra.peff.net","threadId":"29839","inReplyTo":"4F54D98C.2070909@seap.minhap.es","subject":"Re: [PATCH v3 3/4] http: handle proxy proactive authentication","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-06T08:30:52Z","receivedAt":"2012-03-06T08:30:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 05, 2012 at 04:19:40PM +0100, Nelson Benitez Leon wrote:\n\n> diff --git a/http.c b/http.c\n> index 8932da5..b0b4362 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> @@ -303,6 +304,17 @@ static CURL *get_curl_handle(void)\n>  \t\t}\n>  \t}\n>  \tif (curl_http_proxy) {\n> +\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\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\tstruct strbuf pbuf = STRBUF_INIT;\n> +\t\t\tcredential_fill(&proxy_auth);\n> +\t\t\tstrbuf_addf(&pbuf, \"%s://%s:%s@%s\",proxy_auth.protocol,\n> +\t\t\t    \tproxy_auth.username, proxy_auth.password,\n> +\t\t\t    \tproxy_auth.host);\n\nCan we pull this out into a helper function, since the next patch will\nneed to do the exact same thing in the 407 case?\n\nAlso, when turning it back into a URL to hand to curl, should we be\npercent-encoding the items we put in? If my password has an \"@\" in it,\nwouldn't we generate a bogus URL? Although looking at how the http auth\ncode handles this, we set CURLOPT_USERPWD directly. Should you be\nsetting CURLOPT_PROXYUSERPWD instead of munging the proxy URL?\n\n> +\t\t\tfree ((void *)curl_http_proxy);\n\nPlease don't cast to void. This is C, not C++, and casts to void\npointers are implicit.  They can never help, and might cover up an\nactual type error (e.g., casting a non-pointer type).\n\n-Peff\n"},{"id":"186195","messageId":"20120306100947.GA710@sigill.intra.peff.net","threadId":"29839","inReplyTo":"4F55EDF3.2030201@seap.minhap.es","subject":"Re: [PATCH v3 3/4] http: handle proxy proactive authentication","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-06T10:09:47Z","receivedAt":"2012-03-06T10:09:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 06, 2012 at 11:58:59AM +0100, Nelson Benitez Leon wrote:\n\n> > Also, when turning it back into a URL to hand to curl, should we be\n> > percent-encoding the items we put in? If my password has an \"@\" in it,\n> > wouldn't we generate a bogus URL? Although looking at how the http auth\n> > code handles this, we set CURLOPT_USERPWD directly. Should you be\n> > setting CURLOPT_PROXYUSERPWD instead of munging the proxy URL?\n> \n> Ok, but it seems is CURLOPT_PROXYUSERNAME and CURLOPT_PROXYPASSWORD what\n> we need here as per documentation[1]\n> \n> [1] http://curl.haxx.se/libcurl/c/curl_easy_setopt.html#CURLOPTPROXYUSERNAME\n\nYes, the split CURLOPT_USERNAME and CURLOPT_PASSWORD interface is much\nbetter (it allows the username to contain a colon). But it was not\nintroduced until curl 7.19.3, and we support older versions.\n\nWe could do an #if on LIBCURL_VERSION_NUM to use the new form when it's\navailable, but nobody has complained so far.\n\n> >> +\t\t\tfree ((void *)curl_http_proxy);\n> > \n> > Please don't cast to void. This is C, not C++, and casts to void\n> > pointers are implicit.  They can never help, and might cover up an\n> > actual type error (e.g., casting a non-pointer type).\n> \n> Ok, will remove it, I copy/paste it from the http code and I must admit\n> I didn't understand why this was needed.\n\nAh.  I grepped for the spot you copied. The cast is to get rid of the\n\"const\" on curl_http_proxy. But if it's a pointer to allocated memory,\nit should not be declared const in the first place. Unfortunately,\nfixing this means casting in the call to git_config_string (which for\nsome reason takes a pointer-to-const-pointer, even though the value it\nputs in will always be allocated by xstrdup). Or fixing\ngit_config_string, but that cascades to require fixing in lots of other\nplaces. Ugh.\n\n-Peff\n"},{"id":"186197","messageId":"20120306105339.GA5013@sigill.intra.peff.net","threadId":"29839","inReplyTo":"20120306100947.GA710@sigill.intra.peff.net","subject":"Re: [PATCH v3 3/4] http: handle proxy proactive authentication","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-06T10:53:39Z","receivedAt":"2012-03-06T10:53:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 06, 2012 at 05:09:47AM -0500, Jeff King wrote:\n\n> > Ok, will remove it, I copy/paste it from the http code and I must admit\n> > I didn't understand why this was needed.\n> \n> Ah.  I grepped for the spot you copied. The cast is to get rid of the\n> \"const\" on curl_http_proxy. But if it's a pointer to allocated memory,\n> it should not be declared const in the first place. Unfortunately,\n> fixing this means casting in the call to git_config_string (which for\n> some reason takes a pointer-to-const-pointer, even though the value it\n> puts in will always be allocated by xstrdup). Or fixing\n> git_config_string, but that cascades to require fixing in lots of other\n> places. Ugh.\n\nI did a little more looking into this. The situation is annoyingly\ncomplex, because some callers really do have const strings. They do\nthings like this:\n\n  static const char *prune_expire = \"2.weeks.ago\";\n  [...]\n  git_config_string(&prune_expire, var, value);\n\nAnd that one should be const, because the string literal really is\nconst. Sometimes it is converted into an allocated value, but we\neffectively treat it as const.\n\nAnd then you have things like curl_http_proxy, which are never actually\nconst, but always allocated strings (that get freed during cleanup).\nThose ones should probably not be const.\n\nSo I don't think there's an easy solution, and the least evil thing is\nprobably to just keep the cast on free, as you did (although I think if\nyou convert to using CURLOPT_PROXYUSERPWD, you won't be rewriting\ncurl_http_proxy anymore, and thus your free() call will go away).\n\n-Peff\n"},{"id":"186194","messageId":"4F55EDF3.2030201@seap.minhap.es","threadId":"29839","inReplyTo":"20120306083052.GD21199@sigill.intra.peff.net","subject":"Re: [PATCH v3 3/4] http: handle proxy proactive authentication","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-06T10:58:59Z","receivedAt":"2012-03-06T10:58:59Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 03/06/2012 09:30 AM, Jeff King wrote:\n> On Mon, Mar 05, 2012 at 04:19:40PM +0100, Nelson Benitez Leon wrote:\n> \n>> diff --git a/http.c b/http.c\n>> index 8932da5..b0b4362 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>> @@ -303,6 +304,17 @@ static CURL *get_curl_handle(void)\n>>  \t\t}\n>>  \t}\n>>  \tif (curl_http_proxy) {\n>> +\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\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\tstruct strbuf pbuf = STRBUF_INIT;\n>> +\t\t\tcredential_fill(&proxy_auth);\n>> +\t\t\tstrbuf_addf(&pbuf, \"%s://%s:%s@%s\",proxy_auth.protocol,\n>> +\t\t\t    \tproxy_auth.username, proxy_auth.password,\n>> +\t\t\t    \tproxy_auth.host);\n> \n> Can we pull this out into a helper function, since the next patch will\n> need to do the exact same thing in the 407 case?\n\nOk.\n\n> Also, when turning it back into a URL to hand to curl, should we be\n> percent-encoding the items we put in? If my password has an \"@\" in it,\n> wouldn't we generate a bogus URL? Although looking at how the http auth\n> code handles this, we set CURLOPT_USERPWD directly. Should you be\n> setting CURLOPT_PROXYUSERPWD instead of munging the proxy URL?\n\nOk, but it seems is CURLOPT_PROXYUSERNAME and CURLOPT_PROXYPASSWORD what\nwe need here as per documentation[1]\n\n[1] http://curl.haxx.se/libcurl/c/curl_easy_setopt.html#CURLOPTPROXYUSERNAME\n\n>> +\t\t\tfree ((void *)curl_http_proxy);\n> \n> Please don't cast to void. This is C, not C++, and casts to void\n> pointers are implicit.  They can never help, and might cover up an\n> actual type error (e.g., casting a non-pointer type).\n\nOk, will remove it, I copy/paste it from the http code and I must admit\nI didn't understand why this was needed.\n"}]}