[PATCH 1/3] http: handle proxy authentication failure (error 407)

[PATCH 1/3] http: handle proxy authentication failure (error 407)

[Nelson Benitez Leon]

Handle http 407 error code by asking for credentials and
retrying request in case credentials were not present, or
marking credentials as rejected if they were already provided.

Signed-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 http.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/http.c b/http.c
index 5cb87f1..624d1a0 100644
--- a/http.c
+++ b/http.c
@@ -43,6 +43,7 @@ static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
 static const char *curl_cookie_file;
 static struct credential http_auth = CREDENTIAL_INIT;
+static struct credential proxy_auth = CREDENTIAL_INIT;
 static int http_proactive_auth;
 static const char *user_agent;
 
@@ -241,6 +242,20 @@ static int has_cert_password(void)
 	return 1;
 }
 
+static void set_proxy_auth(CURL *result)
+{
+	if (proxy_auth.username && proxy_auth.password) {
+#if LIBCURL_VERSION_NUM >= 0x071301
+		curl_easy_setopt(result, CURLOPT_PROXYUSERNAME, proxy_auth.username);
+		curl_easy_setopt(result, CURLOPT_PROXYPASSWORD, proxy_auth.password);
+#else
+		struct strbuf userpwd = STRBUF_INIT;
+		strbuf_addf(&userpwd, "%s:%s", proxy_auth.username, proxy_auth.password);
+		curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, strbuf_detach(&userpwd, NULL));
+#endif
+	}
+}
+
 static CURL *get_curl_handle(void)
 {
 	CURL *result = curl_easy_init();
@@ -804,6 +819,15 @@ static int http_request(const char *url, void *result, int target, int options)
 				init_curl_http_auth(slot->curl);
 				ret = HTTP_REAUTH;
 			}
+		} else if (results.http_code == 407) { /* Proxy authentication failure */
+			if (proxy_auth.username && proxy_auth.password) {
+				credential_reject(&proxy_auth);
+				ret = HTTP_NOAUTH;
+			} else {
+				credential_fill(&proxy_auth);
+				set_proxy_auth(slot->curl);
+				ret = HTTP_REAUTH;
+			}
 		} else {
 			if (!curl_errorstr[0])
 				strlcpy(curl_errorstr,
-- 
1.7.7.6

Re: [PATCH 1/3] http: handle proxy authentication failure (error 407)

[Jeff King]

On Fri, May 11, 2012 at 03:13:40PM +0200, Nelson Benitez Leon wrote:

> @@ -804,6 +819,15 @@ static int http_request(const char *url, void *result, int target, int options)
>  				init_curl_http_auth(slot->curl);
>  				ret = HTTP_REAUTH;
>  			}
> +		} else if (results.http_code == 407) { /* Proxy authentication failure */
> +			if (proxy_auth.username && proxy_auth.password) {
> +				credential_reject(&proxy_auth);
> +				ret = HTTP_NOAUTH;
> +			} else {
> +				credential_fill(&proxy_auth);
> +				set_proxy_auth(slot->curl);
> +				ret = HTTP_REAUTH;
> +			}

This part will fill in the username/password based on the proxy URL. But
we never set the proxy URL ahead of time, so there is no chance for
credential helpers to act, and the prompts will be confusing (they will
just say "Password" instead of "Password for ...", which will make it
unclear that we want the proxy password, not the remote server's
password).

So it's OK to drop the environment-parsing bits, but:

  1. When we _do_ get the proxy via config, should we still parse it? I
     could go either way. It's a nice feature, and I think we don't have
     to care about how the environment parsing or NO_PROXY works. On the
     other hand, we could just wait for the callback-based
     authentication that will come in newer versions of curl, and code
     to that, which will be even simpler. In the meantime, people can
     just accept it.

  2. When we don't know the proxy name beforehand, we should probably
     say something to stderr to indicate that it was a proxy
     authentication failure.

Also, what about the dumb http-push code-paths? They would need us to
handle http_proactive_auth in the same way. Which obviously won't work
for environment-based proxies, but could work for config-based proxies.
I'm not sure if it's worth caring about. In the long run, the
callback-based authentication is the way forward (though of course it
will take time for that feature to get released in curl, and then for
people to start having a curl that uses it, and so on).

If we just punt on (1) and the proactive auth thing, then I think as a
minimum we can get away with squashing this into your patch:

diff --git a/http.c b/http.c
index 0023119..86e68ee 100644
--- a/http.c
+++ b/http.c
@@ -824,6 +824,7 @@ static int http_request(const char *url, void *result, int target, int options)
 				credential_reject(&proxy_auth);
 				ret = HTTP_NOAUTH;
 			} else {
+				warning(_("http proxy did not accept our credentials, retrying"))
 				credential_fill(&proxy_auth);
 				set_proxy_auth(slot->curl);
 				ret = HTTP_AUTH_RETRY;

-Peff