{"thread":{"id":"29840","subject":"[PATCH v3 4/4] http: handle proxy authentication failure (error 407)","startedAt":"2012-03-05T15:21:28Z","lastAt":"2012-03-06T11:10:12Z","messageCount":3,"participants":["Nelson Benitez Leon","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"186116","messageId":"4F54D9F8.2010401@seap.minhap.es","threadId":"29840","inReplyTo":null,"subject":"[PATCH v3 4/4] http: handle proxy authentication failure (error 407)","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-05T15:21:28Z","receivedAt":"2012-03-05T15:21:28Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"Handle http 407 error code by asking for credentials and\nretrying request in case credentials were not present, or\nmarking credentials as rejected if they were already provided.\n\nSigned-off-by: Nelson Benitez Leon \n---\n http.c | 16 ++++++++++++++++\n 1 files changed, 16 insertions(+), 0 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex b0b4362..5fffa47 100644\n--- a/http.c\n+++ b/http.c\n@@ -812,6 +812,22 @@ static int http_request(const char *url, void *result, int target, int options)\n \t\t\t\tinit_curl_http_auth(slot->curl);\n \t\t\t\tret = HTTP_REAUTH;\n \t\t\t}\n+\t\t} else if (results.http_code == 407) { /* Proxy authentication failure */\n+\t\t\tif (proxy_auth.username && proxy_auth.password) {\n+\t\t\t\tcredential_reject(&proxy_auth);\n+\t\t\t\tret = HTTP_NOAUTH;\n+\t\t\t} else {\n+\t\t\t\tstruct strbuf pbuf = STRBUF_INIT;\n+\t\t\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n+\t\t\t\tcredential_fill(&proxy_auth);\n+\t\t\t\tstrbuf_addf(&pbuf, \"%s://%s:%s@%s\",proxy_auth.protocol,\n+\t\t\t \t\t\tproxy_auth.username, proxy_auth.password,\n+\t\t\t \t\t\tproxy_auth.host);\n+\t\t\t\tfree ((void *)curl_http_proxy);\n+\t\t\t\tcurl_http_proxy = strbuf_detach(&pbuf, NULL);\n+\t\t\t\tcurl_easy_setopt(slot->curl, CURLOPT_PROXY, curl_http_proxy);\n+\t\t\t\tret = HTTP_REAUTH;\n+\t\t\t}\n \t\t} else {\n \t\t\tif (!curl_errorstr[0])\n \t\t\t\tstrlcpy(curl_errorstr,\n-- \n1.7.7.6\n"},{"id":"186183","messageId":"20120306085445.GE21199@sigill.intra.peff.net","threadId":"29840","inReplyTo":"4F54D9F8.2010401@seap.minhap.es","subject":"Re: [PATCH v3 4/4] http: handle proxy authentication failure (error 407)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-06T08:54:45Z","receivedAt":"2012-03-06T08:54:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 05, 2012 at 04:21:28PM +0100, Nelson Benitez Leon wrote:\n\n> Handle http 407 error code by asking for credentials and\n> retrying request in case credentials were not present, or\n> marking credentials as rejected if they were already provided.\n> \n> Signed-off-by: Nelson Benitez Leon \n> ---\n> http.c | 16 ++++++++++++++++\n> 1 files changed, 16 insertions(+), 0 deletions(-)\n\nNo tests. I wonder how hard it would be to set up an apache proxy for\nautomated testing, just as we set one up as a git server in\nt/lib-httpd.sh. It should be basically the same process, but with a\ndifferent config file, I would think.\n\n> diff --git a/http.c b/http.c\n> index b0b4362..5fffa47 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -812,6 +812,22 @@ static int http_request(const char *url, void *result, int target, int options)\n> \t\t\t\tinit_curl_http_auth(slot->curl);\n> \t\t\t\tret = HTTP_REAUTH;\n> \t\t\t}\n> +\t\t} else if (results.http_code == 407) { /* Proxy authentication failure */\n> +\t\t\tif (proxy_auth.username && proxy_auth.password) {\n> +\t\t\t\tcredential_reject(&proxy_auth);\n> +\t\t\t\tret = HTTP_NOAUTH;\n> +\t\t\t} else {\n> +\t\t\t\tstruct strbuf pbuf = STRBUF_INIT;\n> +\t\t\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n> +\t\t\t\tcredential_fill(&proxy_auth);\n> +\t\t\t\tstrbuf_addf(&pbuf, \"%s://%s:%s@%s\",proxy_auth.protocol,\n> +\t\t\t \t\t\tproxy_auth.username, proxy_auth.password,\n> +\t\t\t \t\t\tproxy_auth.host);\n> +\t\t\t\tfree ((void *)curl_http_proxy);\n> +\t\t\t\tcurl_http_proxy = strbuf_detach(&pbuf, NULL);\n> +\t\t\t\tcurl_easy_setopt(slot->curl, CURLOPT_PROXY, curl_http_proxy);\n> +\t\t\t\tret = HTTP_REAUTH;\n> +\t\t\t}\n\nHmm. So HTTP_REAUTH used to mean \"try again, we got new http\ncredentials\". And in http_request_reauth, we recognized the REAUTH flag,\nand tried again. Now it also means \"try again, we got new proxy\ncredentials\". But what happens if you need to retry twice? That is, the\nfollowing sequence:\n\n 1. We make a request; proxy returns 407, because we didn't give it a\n password. We ask for the password and return REAUTH.\n\n 2. We make another request; the proxy passes it to the actual server,\n who returns 401, because we didn't give an http password. We ask\n for the password and return REAUTH.\n\n 3. We make a third request, but this time everybody is happy.\n\nI don't think step (3) actually happens with your code. We don't\nactually look for REAUTH on the second step.\n\nWe need to actually loop, retrying if we get reauth (and arguably REAUTH\nshould simply be called RETRY). Like this:\n\ndiff --git a/http.c b/http.c\nindex e4afbe5..c325b00 100644\n--- a/http.c\n+++ b/http.c\n@@ -810,10 +810,13 @@ static int http_request(const char *url, curl_write_callback cb, void *result,\n static int http_request_reauth(const char *url, curl_write_callback cb,\n \t\t\t void *result, unsigned long offset, int options)\n {\n-\tint ret = http_request(url, cb, result, offset, options);\n-\tif (ret != HTTP_REAUTH)\n-\t\treturn ret;\n-\treturn http_request(url, cb, result, offset, options);\n+\tint ret;\n+\n+\tdo {\n+\t\tret = http_request(url, cb, result, offset, options);\n+\t} while (ret == HTTP_REAUTH);\n+\n+\treturn ret;\n }\n \n int http_get_strbuf(const char *url, struct strbuf *result, int options)\n\n\nI think the whole thing would be a bit easier to read if\nhttp_request_reauth actually handled the 401 and 407 itself, but\nunfortunately we need to access the curl handle. Arguably http_request\nshould just be setting the auth from the http_auth struct itself each\ntime it is called (the http_auth struct is what caches the password, so\nit's not like we would prompt the user again). But that refactoring is\nunrelated to what you're doing, I think, so we can leave it.\n\n-Peff\n"},{"id":"186196","messageId":"4F55F094.6010606@seap.minhap.es","threadId":"29840","inReplyTo":"20120306085445.GE21199@sigill.intra.peff.net","subject":"Re: [PATCH v3 4/4] http: handle proxy authentication failure (error 407)","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-06T11:10:12Z","receivedAt":"2012-03-06T11:10:12Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 03/06/2012 09:54 AM, Jeff King wrote:\n> On Mon, Mar 05, 2012 at 04:21:28PM +0100, Nelson Benitez Leon wrote:\n> \n>> Handle http 407 error code by asking for credentials and\n>> retrying request in case credentials were not present, or\n>> marking credentials as rejected if they were already provided.\n>>\n>> Signed-off-by: Nelson Benitez Leon \n>> ---\n>> http.c | 16 ++++++++++++++++\n>> 1 files changed, 16 insertions(+), 0 deletions(-)\n> \n> No tests. I wonder how hard it would be to set up an apache proxy for\n> automated testing, just as we set one up as a git server in\n> t/lib-httpd.sh. It should be basically the same process, but with a\n> different config file, I would think.\n\nI cannot help with this, I didn't even know you could setup a proxy\nwith solely apache, I thought you needed special software like Squid..\nanyway I'm testing all these patches with my employers proxy which uses\nNTLM autentication and they're performing well..\n\n> [snip]\n> \n> We need to actually loop, retrying if we get reauth (and arguably REAUTH\n> should simply be called RETRY). Like this:\n> \n> diff --git a/http.c b/http.c\n> index e4afbe5..c325b00 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -810,10 +810,13 @@ static int http_request(const char *url, curl_write_callback cb, void *result,\n> static int http_request_reauth(const char *url, curl_write_callback cb,\n> \t\t\t void *result, unsigned long offset, int options)\n> {\n> -\tint ret = http_request(url, cb, result, offset, options);\n> -\tif (ret != HTTP_REAUTH)\n> -\t\treturn ret;\n> -\treturn http_request(url, cb, result, offset, options);\n> +\tint ret;\n> +\n> +\tdo {\n> +\t\tret = http_request(url, cb, result, offset, options);\n> +\t} while (ret == HTTP_REAUTH);\n> +\n> +\treturn ret;\n> }\n> \n> int http_get_strbuf(const char *url, struct strbuf *result, int options)\n\nYes, giving the case you mention (proxy auth + http auth) your patch does\nthe right thing.\n"}]}