{"thread":{"id":"29800","subject":"[PATCH v2 3/3] http: when proxy url has username but no password, ask for password","startedAt":"2012-03-01T17:49:16Z","lastAt":"2012-03-02T14:05:17Z","messageCount":8,"participants":["Nelson Benitez Leon","Sam Vilain","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"185835","messageId":"4F4FB69C.7000708@vilain.net","threadId":"29800","inReplyTo":"4F4FBE6C.5050507@seap.minhap.es","subject":"Re: [PATCH v2 3/3] http: when proxy url has username but no password, ask for password","fromName":"Sam Vilain","fromEmail":"sam@vilain.net","sentAt":"2012-03-01T17:49:16Z","receivedAt":"2012-03-01T17:49:16Z","isPatch":true,"sender":{"key":"sam@vilain.net","avatar":"https://gravatar.com/avatar/8fc840ca854dbf6f7065b4335e3b934951c1dca3b11db688e95e471901f8f4a8?d=mp&s=160"},"body":"On 3/1/12 10:22 AM, Nelson Benitez Leon wrote:\n> Support proxy urls with username but without a password, in which\n> case we interactively ask for the password (using credential api).\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> Signed-off-by: Nelson Benitez Leon\n> ---\n> http.c | 16 +++++++++++++++-\n> 1 files changed, 15 insertions(+), 1 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index 8932da5..5916194 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,7 +304,20 @@ static CURL *get_curl_handle(void)\n> \t\t}\n> \t}\n> \tif (curl_http_proxy) {\n> -\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n> +\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n> +\t\tif (proxy_auth.username != NULL&& proxy_auth.password == NULL) {\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\nWouldn't it be better to wait until the proxy returns a 403 before \nassuming that the proxy setting is incorrect/missing a password? What \nif the administrator expects the user to fill in both the username and \npassword? That is the behaviour of a web browser.\n\nAlso, I think you should wait until that 403 to detect whether the proxy \nsetting came from the environment, and only load it explicitly then.\n\nSam\n\n> +\t\t\tstrbuf_addf(&pbuf, \"%s://%s:%s@%s\",proxy_auth.protocol,\n> +\t\t\t\t proxy_auth.username, proxy_auth.password,\n> +\t\t\t\t proxy_auth.host);\n> +\t\t\tfree ((void *)curl_http_proxy);\n> +\t\t\tcurl_http_proxy = strbuf_detach(&pbuf, NULL);\n> +\t\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n> +\t\t} else {\n> +\t\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n> +\t\t}\n> \t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n> \t}\n>\n"},{"id":"185833","messageId":"4F4FBE6C.5050507@seap.minhap.es","threadId":"29800","inReplyTo":null,"subject":"[PATCH v2 3/3] http: when proxy url has username but no password, ask for password","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-01T18:22:36Z","receivedAt":"2012-03-01T18:22:36Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"Support proxy urls with username but without a password, in which\ncase we interactively ask for the password (using credential api).\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 \n---\n http.c | 16 +++++++++++++++-\n 1 files changed, 15 insertions(+), 1 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 8932da5..5916194 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,7 +304,20 @@ static CURL *get_curl_handle(void)\n \t\t}\n \t}\n \tif (curl_http_proxy) {\n-\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n+\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n+\t\tif (proxy_auth.username != NULL && proxy_auth.password == NULL) {\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\t proxy_auth.username, proxy_auth.password,\n+\t\t\t\t proxy_auth.host);\n+\t\t\tfree ((void *)curl_http_proxy);\n+\t\t\tcurl_http_proxy = strbuf_detach(&pbuf, NULL);\n+\t\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n+\t\t} else {\n+\t\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n+\t\t}\n \t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n \t}\n \n-- \n1.7.7.6\n"},{"id":"185842","messageId":"7vty28m8sd.fsf@alter.siamese.dyndns.org","threadId":"29800","inReplyTo":"4F4FBE6C.5050507@seap.minhap.es","subject":"Re: [PATCH v2 3/3] http: when proxy url has username but no password, ask for password","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-01T19:16:18Z","receivedAt":"2012-03-01T19:16:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nelson Benitez Leon writes:\n\n> Support proxy urls with username but without a password, in which\n> case we interactively ask for the password (using credential api).\n> This makes possible to not have the password written down in\n> http_proxy env var or in http.proxy config option.\n\nHow do other people's applications that use http_proxy environment\nvariable handle this situation?\n\nWith this patch and the previous 2/3, we are allowing people to set\n\"http_proxy=http://me@over.there/\", but an environment variable is global\nto the user's environment, so if other applications do not grok the \"name\nonly\" proxy URL the same way as this patch does, adding this code only to\nGit does not make users' lives any better.\n\nOf course the above does not apply to http.proxy configuration, which is\nspecific to Git.\n\n>\n> Signed-off-by: Nelson Benitez Leon \n> ---\n> http.c | 16 +++++++++++++++-\n> 1 files changed, 15 insertions(+), 1 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index 8932da5..5916194 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,7 +304,20 @@ static CURL *get_curl_handle(void)\n> \t\t}\n> \t}\n> \tif (curl_http_proxy) {\n> -\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n> +\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n> +\t\tif (proxy_auth.username != NULL && proxy_auth.password == NULL) {\n\nJust a style, but \n\n\t\tif (proxy_auth.username && !proxy_auth.password) {\n\nis much more preferred.\n\n> +\t\t\tfree ((void *)curl_http_proxy);\n\nI think somebody already pointed out interaction of this with 2/3.\n"},{"id":"185861","messageId":"20120301215812.GG17631@sigill.intra.peff.net","threadId":"29800","inReplyTo":"4F4FB69C.7000708@vilain.net","subject":"Re: [PATCH v2 3/3] http: when proxy url has username but no password, ask for password","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-01T21:58:12Z","receivedAt":"2012-03-01T21:58:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 01, 2012 at 09:49:16AM -0800, Sam Vilain wrote:\n\n> > \tif (curl_http_proxy) {\n> >-\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n> >+\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n> >+\t\tif (proxy_auth.username != NULL&& proxy_auth.password == NULL) {\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> \n> Wouldn't it be better to wait until the proxy returns a 403 before\n> assuming that the proxy setting is incorrect/missing a password?\n> What if the administrator expects the user to fill in both the\n> username and password? That is the behaviour of a web browser.\n> \n> Also, I think you should wait until that 403 to detect whether the\n> proxy setting came from the environment, and only load it explicitly\n> then.\n\nIt's worth looking at what the http auth code does here.\n\nIn the beginning (2005), git saw that there was a username in the URL\nand prompted for a password unconditionally before making a request. If\nyou didn't have a username, you didn't do auth, period.\n\nLater, 42653c0 (Prompt for a username when an HTTP request 401s,\n2010-04-01) taught git to handle 401s, for when the URL does not contain\na username. We kept the unconditional pre-prompt, though; it has the\nnice side effect of avoiding a round-trip to the server.\n\nThen, in 986bbc0 (http: don't always prompt for password, 2011-11-04),\nthe unconditional pre-prompt was taken away. While avoiding the\nround-trip is nice, it circumvented curl's reading of the .netrc file,\nwhich means git would prompt unnecessarily, even when curl could\neventually read out of the netrc.\n\nHowever, the code for dumb http push-over-dav doesn't handle the 401\nproperly. As a work-around, a4ddbc3 (http-push: enable \"proactive auth\",\n2011-12-13) re-enabled the pre-prompt, but only for the code paths that\nneed it (and those code paths are now broken for .netrc, as everything\nwas before 986bbc0). This is a hack, and in the long run it would be\nnice to have everything handle 401s properly, but the dav code is\nsomewhat obsolete these days, and I suspect nobody really wants to\noverhaul it.\n\nComplicating all of this is the fact that I think Nelson's original\npatch was based on an older, pre-986bbc0 version of git, which is why he\nfollowed the pre-prompt route, copying the style of regular http auth.\n\nSo there's the history lesson. What should proxy auth do?\n\n 1. Definitely respond to HTTP 407 by prompting on the fly; this code\n should go along-side the HTTP 401 code in http.c.\n\n 2. Definitely do the pre-prompt thing when http_proactive_auth is set\n (which is used only by http-push). Unless somebody really feels\n like re-writing http-push to handle retries for authentication.\n\n 3. Consider doing the pre-prompt thing when http_proactive_auth is not\n set. This can save a round-trip, but we should not do it if there\n is a good reason not to. The two possible reasons I can think of\n are:\n\n a. Like http auth, if curl will read the proxy credentials from\n .netrc, then we should not do it for the same reasons\n mentioned in 986bbc0.\n\n b. If people realistically have proxy URLs with usernames but do\n _not_ want to ask for a password, then the prompt will be\n annoying. I'm not sure that anybody expects that.\n\nI consider (3) to be a \"meh, if you are really interested in looking\ninto this\" step, as it is really just a possible optimization (and I\nsuspect curl _does_ use netrc for proxy credentials, but I haven't\nchecked). But we definitely want to get (1) and (2) right.\n\n-Peff\n"},{"id":"185914","messageId":"20120302124538.GA10637@sigill.intra.peff.net","threadId":"29800","inReplyTo":"4F50CC41.5020307@seap.minhap.es","subject":"Re: [PATCH v2 3/3] http: when proxy url has username but no password, ask for password","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-02T12:45:39Z","receivedAt":"2012-03-02T12:45:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 02, 2012 at 02:33:53PM +0100, Nelson Benitez Leon wrote:\n\n> > So there's the history lesson. What should proxy auth do?\n> > \n> > 1. Definitely respond to HTTP 407 by prompting on the fly; this code\n> > should go along-side the HTTP 401 code in http.c.\n> > \n> > 2. Definitely do the pre-prompt thing when http_proactive_auth is set\n> > (which is used only by http-push). Unless somebody really feels\n> > like re-writing http-push to handle retries for authentication.\n> > \n> > 3. Consider doing the pre-prompt thing when http_proactive_auth is not\n> > set. This can save a round-trip, but we should not do it if there\n> > is a good reason not to. The two possible reasons I can think of\n> > are:\n> > \n> > a. Like http auth, if curl will read the proxy credentials from\n> > .netrc, then we should not do it for the same reasons\n> > mentioned in 986bbc0.\n> > \n> > b. If people realistically have proxy URLs with usernames but do\n> > _not_ want to ask for a password, then the prompt will be\n> > annoying. I'm not sure that anybody expects that.\n> \n> So, trying to sum up, I will try to redo patch-set as follows:\n> - Ignore PATCH 2/3 , that is, we won't read any env var.\n> - Let cURL try to connect and if that fails with 407 , then do a credential_fill\n> and try to reconnect.\n> \n> Is that ok? or do I need to do something more?\n\nI think you'll still need to read the env var, because you'll need to\nknow the proxy URL when getting the password (to ask credential helpers\nproperly, and to prompt the user).\n\nAlso, I think you'll need to call credential_fill() when\nhttp_proactive_auth is set. Otherwise http-push will not be able to do\nproxy auth.\n\n-Peff\n"},{"id":"185911","messageId":"4F50CC41.5020307@seap.minhap.es","threadId":"29800","inReplyTo":"20120301215812.GG17631@sigill.intra.peff.net","subject":"Re: [PATCH v2 3/3] http: when proxy url has username but no password, ask for password","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-02T13:33:53Z","receivedAt":"2012-03-02T13:33:53Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 03/01/2012 10:58 PM, Jeff King wrote:\n> On Thu, Mar 01, 2012 at 09:49:16AM -0800, Sam Vilain wrote:\n> \n>>> \tif (curl_http_proxy) {\n>>> -\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n>>> +\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n>>> +\t\tif (proxy_auth.username != NULL&& proxy_auth.password == NULL) {\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>>\n>> Wouldn't it be better to wait until the proxy returns a 403 before\n>> assuming that the proxy setting is incorrect/missing a password?\n>> What if the administrator expects the user to fill in both the\n>> username and password? That is the behaviour of a web browser.\n>>\n>> Also, I think you should wait until that 403 to detect whether the\n>> proxy setting came from the environment, and only load it explicitly\n>> then.\n> \n> It's worth looking at what the http auth code does here.\n> \n[snip]\n> overhaul it.\n> \n> Complicating all of this is the fact that I think Nelson's original\n> patch was based on an older, pre-986bbc0 version of git, which is why he\n> followed the pre-prompt route, copying the style of regular http auth.\n> \n> So there's the history lesson. What should proxy auth do?\n> \n> 1. Definitely respond to HTTP 407 by prompting on the fly; this code\n> should go along-side the HTTP 401 code in http.c.\n> \n> 2. Definitely do the pre-prompt thing when http_proactive_auth is set\n> (which is used only by http-push). Unless somebody really feels\n> like re-writing http-push to handle retries for authentication.\n> \n> 3. Consider doing the pre-prompt thing when http_proactive_auth is not\n> set. This can save a round-trip, but we should not do it if there\n> is a good reason not to. The two possible reasons I can think of\n> are:\n> \n> a. Like http auth, if curl will read the proxy credentials from\n> .netrc, then we should not do it for the same reasons\n> mentioned in 986bbc0.\n> \n> b. If people realistically have proxy URLs with usernames but do\n> _not_ want to ask for a password, then the prompt will be\n> annoying. I'm not sure that anybody expects that.\n\nSo, trying to sum up, I will try to redo patch-set as follows:\n- Ignore PATCH 2/3 , that is, we won't read any env var.\n- Let cURL try to connect and if that fails with 407 , then do a credential_fill\nand try to reconnect.\n\nIs that ok? or do I need to do something more?\n"},{"id":"185921","messageId":"20120302135237.GB23846@sigill.intra.peff.net","threadId":"29800","inReplyTo":"4F50D39D.5040806@seap.minhap.es","subject":"Re: [PATCH v2 3/3] http: when proxy url has username but no password, ask for password","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-02T13:52:37Z","receivedAt":"2012-03-02T13:52:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 02, 2012 at 03:05:17PM +0100, Nelson Benitez Leon wrote:\n\n> > I think you'll still need to read the env var, because you'll need to\n> > know the proxy URL when getting the password (to ask credential helpers\n> > properly, and to prompt the user).\n> \n> Ok, but I can read it after receiving the 407 (and in case we were not\n> using http.proxy) so discarding PATCH 2/3 still applies, ok? or we need\n> to read it first-hand for the http_proactive_auth you mention below?\n\nYou will need it for proactive_auth.\n\n> > Also, I think you'll need to call credential_fill() when\n> > http_proactive_auth is set. Otherwise http-push will not be able to do\n> > proxy auth.\n> \n> I still don't get what proactive_auth is about, will ask you when I get\n> to that part of the patch.\n\nIt is a flag that, when true, instructs the http code to do auth if we\nhave a non-NULL username, even before we get an http 401. It is only set\nfor http-push, because the http-push-over-dav code does not properly\ndetect and retry on a 401 (and I don't expect it will be easy to\nproperly detect and retry on a 407, either). Whereas the smart-http code\nand the dumb http fetch code properly detect the 401.\n\n-Peff\n"},{"id":"185917","messageId":"4F50D39D.5040806@seap.minhap.es","threadId":"29800","inReplyTo":"20120302124538.GA10637@sigill.intra.peff.net","subject":"Re: [PATCH v2 3/3] http: when proxy url has username but no password, ask for password","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-02T14:05:17Z","receivedAt":"2012-03-02T14:05:17Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 03/02/2012 01:45 PM, Jeff King wrote:\n> On Fri, Mar 02, 2012 at 02:33:53PM +0100, Nelson Benitez Leon wrote:\n> \n>>> So there's the history lesson. What should proxy auth do?\n>>>\n>>> 1. Definitely respond to HTTP 407 by prompting on the fly; this code\n>>> should go along-side the HTTP 401 code in http.c.\n>>>\n>>> 2. Definitely do the pre-prompt thing when http_proactive_auth is set\n>>> (which is used only by http-push). Unless somebody really feels\n>>> like re-writing http-push to handle retries for authentication.\n>>>\n>>> 3. Consider doing the pre-prompt thing when http_proactive_auth is not\n>>> set. This can save a round-trip, but we should not do it if there\n>>> is a good reason not to. The two possible reasons I can think of\n>>> are:\n>>>\n>>> a. Like http auth, if curl will read the proxy credentials from\n>>> .netrc, then we should not do it for the same reasons\n>>> mentioned in 986bbc0.\n>>>\n>>> b. If people realistically have proxy URLs with usernames but do\n>>> _not_ want to ask for a password, then the prompt will be\n>>> annoying. I'm not sure that anybody expects that.\n>>\n>> So, trying to sum up, I will try to redo patch-set as follows:\n>> - Ignore PATCH 2/3 , that is, we won't read any env var.\n>> - Let cURL try to connect and if that fails with 407 , then do a credential_fill\n>> and try to reconnect.\n>>\n>> Is that ok? or do I need to do something more?\n> \n> I think you'll still need to read the env var, because you'll need to\n> know the proxy URL when getting the password (to ask credential helpers\n> properly, and to prompt the user).\n\nOk, but I can read it after receiving the 407 (and in case we were not\nusing http.proxy) so discarding PATCH 2/3 still applies, ok? or we need\nto read it first-hand for the http_proactive_auth you mention below?\n\n> \n> Also, I think you'll need to call credential_fill() when\n> http_proactive_auth is set. Otherwise http-push will not be able to do\n> proxy auth.\n\nI still don't get what proactive_auth is about, will ask you when I get\nto that part of the patch.\n\nThank you,\n\n\n> -Peff\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"}]}