[PATCH 1/6] http: try http_proxy env var when http.proxy config option is not set

[PATCH 1/6] http: try http_proxy env var when http.proxy config option is not set

[Nelson Benitez Leon]

cURL already reads it, but if $http_proxy has username but no password
cURL will not ask you for the password and so failed to authenticate
returning a 407 error code. So we read it ourselves to detect that and
ask for the password. Also we read it prior to connection to be able to
make a proactive authentication in case the flag http_proactive_auth is
set.

We also take care to read env proxy var according to protocol being
used in the destination url, e.g.  when the url to retrieve is a https
one, then the proxy env var we look at is https_proxy. We also look at
the uppercase version of these if the lowercase is not found, with the
exception of HTTP_PROXY because cURL ignores it. To make this possible
we now passed destination url parameter to get_active_slot() and
get_curl_handle() functions.

We also read no_proxy env var so to ignore aforementioned proxy env var
if no_proxy contains an asterisk ('*') or contains the host used in url
destination.

Signed-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 http-push.c   |   24 +++++++++++-----------
 http-walker.c |    2 +-
 http.c        |   60 ++++++++++++++++++++++++++++++++++++++++++++++++++------
 http.h        |    2 +-
 remote-curl.c |    4 +-
 5 files changed, 69 insertions(+), 23 deletions(-)

diff --git a/http-push.c b/http-push.c
index 1df7ab5..4e23d00 100644
--- a/http-push.c
+++ b/http-push.c
@@ -297,7 +297,7 @@ static void start_mkcol(struct transfer_request *request)
 
 	request->url = get_remote_object_url(repo->url, hex, 1);
 
-	slot = get_active_slot();
+	slot = get_active_slot(request->url);
 	slot->callback_func = process_response;
 	slot->callback_data = request;
 	curl_setup_http_get(slot->curl, request->url, DAV_MKCOL);
@@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
 	strbuf_add(&buf, request->lock->tmpfile_suffix, 41);
 	request->url = strbuf_detach(&buf, NULL);
 
-	slot = get_active_slot();
+	slot = get_active_slot(request->url);
 	slot->callback_func = process_response;
 	slot->callback_data = request;
 	curl_setup_http(slot->curl, request->url, DAV_PUT,
@@ -438,7 +438,7 @@ static void start_move(struct transfer_request *request)
 	struct active_request_slot *slot;
 	struct curl_slist *dav_headers = NULL;
 
-	slot = get_active_slot();
+	slot = get_active_slot(request->url);
 	slot->callback_func = process_response;
 	slot->callback_data = request;
 	curl_setup_http_get(slot->curl, request->url, DAV_MOVE);
@@ -467,7 +467,7 @@ static int refresh_lock(struct remote_lock *lock)
 
 	dav_headers = get_dav_token_headers(lock, DAV_HEADER_IF | DAV_HEADER_TIMEOUT);
 
-	slot = get_active_slot();
+	slot = get_active_slot(lock->url);
 	slot->results = &results;
 	curl_setup_http_get(slot->curl, lock->url, DAV_LOCK);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);
@@ -882,7 +882,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 	while (ep) {
 		char saved_character = ep[1];
 		ep[1] = '\0';
-		slot = get_active_slot();
+		slot = get_active_slot(url);
 		slot->results = &results;
 		curl_setup_http_get(slot->curl, url, DAV_MKCOL);
 		if (start_active_slot(slot)) {
@@ -912,7 +912,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 	dav_headers = curl_slist_append(dav_headers, timeout_header);
 	dav_headers = curl_slist_append(dav_headers, "Content-Type: text/xml");
 
-	slot = get_active_slot();
+	slot = get_active_slot(url);
 	slot->results = &results;
 	curl_setup_http(slot->curl, url, DAV_LOCK, &out_buffer, fwrite_buffer);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);
@@ -980,7 +980,7 @@ static int unlock_remote(struct remote_lock *lock)
 
 	dav_headers = get_dav_token_headers(lock, DAV_HEADER_LOCK);
 
-	slot = get_active_slot();
+	slot = get_active_slot(lock->url);
 	slot->results = &results;
 	curl_setup_http_get(slot->curl, lock->url, DAV_UNLOCK);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);
@@ -1158,7 +1158,7 @@ static void remote_ls(const char *path, int flags,
 	dav_headers = curl_slist_append(dav_headers, "Depth: 1");
 	dav_headers = curl_slist_append(dav_headers, "Content-Type: text/xml");
 
-	slot = get_active_slot();
+	slot = get_active_slot(url);
 	slot->results = &results;
 	curl_setup_http(slot->curl, url, DAV_PROPFIND,
 			&out_buffer, fwrite_buffer);
@@ -1232,7 +1232,7 @@ static int locking_available(void)
 	dav_headers = curl_slist_append(dav_headers, "Depth: 0");
 	dav_headers = curl_slist_append(dav_headers, "Content-Type: text/xml");
 
-	slot = get_active_slot();
+	slot = get_active_slot(repo->url);
 	slot->results = &results;
 	curl_setup_http(slot->curl, repo->url, DAV_PROPFIND,
 			&out_buffer, fwrite_buffer);
@@ -1409,7 +1409,7 @@ static int update_remote(unsigned char *sha1, struct remote_lock *lock)
 
 	strbuf_addf(&out_buffer.buf, "%s\n", sha1_to_hex(sha1));
 
-	slot = get_active_slot();
+	slot = get_active_slot(lock->url);
 	slot->results = &results;
 	curl_setup_http(slot->curl, lock->url, DAV_PUT,
 			&out_buffer, fwrite_null);
@@ -1535,7 +1535,7 @@ static void update_remote_info_refs(struct remote_lock *lock)
 	if (!aborted) {
 		dav_headers = get_dav_token_headers(lock, DAV_HEADER_IF);
 
-		slot = get_active_slot();
+		slot = get_active_slot(lock->url);
 		slot->results = &results;
 		curl_setup_http(slot->curl, lock->url, DAV_PUT,
 				&buffer, fwrite_null);
@@ -1695,7 +1695,7 @@ static int delete_remote_branch(const char *pattern, int force)
 		return 0;
 	url = xmalloc(strlen(repo->url) + strlen(remote_ref->name) + 1);
 	sprintf(url, "%s%s", repo->url, remote_ref->name);
-	slot = get_active_slot();
+	slot = get_active_slot(url);
 	slot->results = &results;
 	curl_setup_http_get(slot->curl, url, DAV_DELETE);
 	if (start_active_slot(slot)) {
diff --git a/http-walker.c b/http-walker.c
index 51a906e..5d5ae34 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -348,7 +348,7 @@ static void fetch_alternates(struct walker *walker, const char *base)
 	 * Use a callback to process the result, since another request
 	 * may fail and need to have alternates loaded before continuing
 	 */
-	slot = get_active_slot();
+	slot = get_active_slot(url);
 	slot->callback_func = process_alternates_response;
 	alt_req.walker = walker;
 	slot->callback_data = &alt_req;
diff --git a/http.c b/http.c
index 5cb87f1..64df7b1 100644
--- a/http.c
+++ b/http.c
@@ -229,6 +229,37 @@ static void init_curl_http_auth(CURL *result)
 #endif
 }
 
+static const char *read_prot_proxy_env(const char *protocol)
+{
+	const char *env_proxy;
+	struct strbuf var = STRBUF_INIT;
+
+	strbuf_addf(&var, "%s_proxy", protocol);
+	env_proxy = getenv(var.buf);
+	if (!env_proxy && strcmp("http_proxy", var.buf)) {
+		char *p;
+		for (p = var.buf; *p; p++)
+			*p = toupper(*p);
+		env_proxy = getenv(var.buf);
+	}
+	strbuf_release(&var);
+	
+	return env_proxy;
+}
+
+static int host_allowed_by_noproxy_env (const char *host)
+{
+	const char *no_proxy = getenv("no_proxy");
+	if (!no_proxy)
+		no_proxy = getenv("NO_PROXY");
+	if (!no_proxy ||
+	    (strcmp("*", no_proxy) &&
+	     !strstr(no_proxy, host)))
+		return 1;
+	
+	return 0;
+}
+
 static int has_cert_password(void)
 {
 	if (ssl_cert == NULL || ssl_cert_password_required != 1)
@@ -241,7 +272,7 @@ static int has_cert_password(void)
 	return 1;
 }
 
-static CURL *get_curl_handle(void)
+static CURL *get_curl_handle(const char *url)
 {
 	CURL *result = curl_easy_init();
 
@@ -304,6 +335,21 @@ static CURL *get_curl_handle(void)
 	if (curl_ftp_no_epsv)
 		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
 
+	if (!curl_http_proxy) {
+		static struct credential parsed_url = CREDENTIAL_INIT;
+		credential_from_url(&parsed_url, url);
+
+		if (parsed_url.protocol) {
+			const char *env_proxy;
+			env_proxy = read_prot_proxy_env(parsed_url.protocol);
+
+			if (env_proxy && host_allowed_by_noproxy_env(parsed_url.host))
+				curl_http_proxy = xstrdup(env_proxy);
+		}
+
+		credential_clear(&parsed_url);
+	}
+	
 	if (curl_http_proxy) {
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
 		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
@@ -394,7 +440,7 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 	}
 
 #ifndef NO_CURL_EASY_DUPHANDLE
-	curl_default = get_curl_handle();
+	curl_default = get_curl_handle(url);
 #endif
 }
 
@@ -443,7 +489,7 @@ void http_cleanup(void)
 	ssl_cert_password_required = 0;
 }
 
-struct active_request_slot *get_active_slot(void)
+struct active_request_slot *get_active_slot(const char *url)
 {
 	struct active_request_slot *slot = active_queue_head;
 	struct active_request_slot *newslot;
@@ -481,7 +527,7 @@ struct active_request_slot *get_active_slot(void)
 
 	if (slot->curl == NULL) {
 #ifdef NO_CURL_EASY_DUPHANDLE
-		slot->curl = get_curl_handle();
+		slot->curl = get_curl_handle(url);
 #else
 		slot->curl = curl_easy_duphandle(curl_default);
 #endif
@@ -756,7 +802,7 @@ static int http_request(const char *url, void *result, int target, int options)
 	struct strbuf buf = STRBUF_INIT;
 	int ret;
 
-	slot = get_active_slot();
+	slot = get_active_slot(url);
 	slot->results = &results;
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
 
@@ -1101,7 +1147,7 @@ struct http_pack_request *new_http_pack_request(
 		goto abort;
 	}
 
-	preq->slot = get_active_slot();
+	preq->slot = get_active_slot(preq->url);
 	curl_easy_setopt(preq->slot->curl, CURLOPT_FILE, preq->packfile);
 	curl_easy_setopt(preq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite);
 	curl_easy_setopt(preq->slot->curl, CURLOPT_URL, preq->url);
@@ -1261,7 +1307,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
 		}
 	}
 
-	freq->slot = get_active_slot();
+	freq->slot = get_active_slot(freq->url);
 
 	curl_easy_setopt(freq->slot->curl, CURLOPT_FILE, freq);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);
diff --git a/http.h b/http.h
index 915c286..483e3ed 100644
--- a/http.h
+++ b/http.h
@@ -73,7 +73,7 @@ extern curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp);
 #endif
 
 /* Slot lifecycle functions */
-extern struct active_request_slot *get_active_slot(void);
+extern struct active_request_slot *get_active_slot(const char *url);
 extern int start_active_slot(struct active_request_slot *slot);
 extern void run_active_slot(struct active_request_slot *slot);
 extern void finish_active_slot(struct active_request_slot *slot);
diff --git a/remote-curl.c b/remote-curl.c
index 0896221..6ceba7a 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -384,7 +384,7 @@ static int probe_rpc(struct rpc_state *rpc)
 	struct strbuf buf = STRBUF_INIT;
 	int err;
 
-	slot = get_active_slot();
+	slot = get_active_slot(rpc->service_url);
 
 	headers = curl_slist_append(headers, rpc->hdr_content_type);
 	headers = curl_slist_append(headers, rpc->hdr_accept);
@@ -441,7 +441,7 @@ static int post_rpc(struct rpc_state *rpc)
 			return err;
 	}
 
-	slot = get_active_slot();
+	slot = get_active_slot(rpc->service_url);
 
 	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
 	curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
-- 
1.7.7.6

Re: [PATCH 1/6] http: try http_proxy env var when http.proxy config option is not set

[Junio C Hamano]

Nelson Benitez Leon <nelsonjesus.benitez@seap.minhap.es> writes:

> cURL already reads it, but if $http_proxy has username but no password
> cURL will not ask you for the password and so failed to authenticate
> returning a 407 error code. So we read it ourselves to detect that and
> ask for the password. Also we read it prior to connection to be able to
> make a proactive authentication in case the flag http_proactive_auth is
> set.
>
> We also take care to read env proxy var according to protocol being
> used in the destination url, e.g.  when the url to retrieve is a https
> one, then the proxy env var we look at is https_proxy. We also look at
> the uppercase version of these if the lowercase is not found, with the
> exception of HTTP_PROXY because cURL ignores it. To make this possible
> we now passed destination url parameter to get_active_slot() and
> get_curl_handle() functions.
>
> We also read no_proxy env var so to ignore aforementioned proxy env var
> if no_proxy contains an asterisk ('*') or contains the host used in url
> destination.
>
> Signed-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

I'll trust Peff to point out anything I missed, but from a cursory look,
the result looks much cleaner than the previous round.

> diff --git a/http.c b/http.c
> index 5cb87f1..64df7b1 100644
> --- a/http.c
> +++ b/http.c
> @@ -229,6 +229,37 @@ static void init_curl_http_auth(CURL *result)
> ...
> +static int host_allowed_by_noproxy_env (const char *host)
> +{

I'll queue the updated series with s/_env (/_env(/; here, and also add a
missing explanation on the bulk of "noise" in the patch at the end of the
log message:

    In order to be able to determine what proxy settings is needed from
    the very beginning of a request, get_active_slot() learns to take the
    destination URL, as it needs to pass it to get_curl_handle() that
    implements the logic to pick proxies based on the protocol used.


Thanks.

Re: [PATCH 1/6] http: try http_proxy env var when http.proxy config option is not set

[Jeff King]

On Thu, May 03, 2012 at 06:39:47PM +0200, Nelson Benitez Leon wrote:

> +static const char *read_prot_proxy_env(const char *protocol)
> +{
> +	const char *env_proxy;
> +	struct strbuf var = STRBUF_INIT;
> +
> +	strbuf_addf(&var, "%s_proxy", protocol);
> +	env_proxy = getenv(var.buf);
> +	if (!env_proxy && strcmp("http_proxy", var.buf)) {
> +		char *p;
> +		for (p = var.buf; *p; p++)
> +			*p = toupper(*p);
> +		env_proxy = getenv(var.buf);
> +	}
> +	strbuf_release(&var);
> +	
> +	return env_proxy;
> +}

Thanks, this is way more readable than the previous iteration.

> +static int host_allowed_by_noproxy_env (const char *host)
> +{
> +	const char *no_proxy = getenv("no_proxy");
> +	if (!no_proxy)
> +		no_proxy = getenv("NO_PROXY");
> +	if (!no_proxy ||
> +	    (strcmp("*", no_proxy) &&
> +	     !strstr(no_proxy, host)))
> +		return 1;
> +	
> +	return 0;
> +}

This simplified parsing misses a lot of corner cases. Three I can see
right off the bat:

  1. If your NO_PROXY is "no-proxy.com", and your host is
     "proxy.com", your code will consider that a match, but curl does
     not.

  2. If your NO_PROXY contains "no-proxy.com", but your host is
     "www.no-proxy.com", curl will consider that a match, but your code
     does not.

  3. If your NO_PROXY contains "no-proxy.com", but your host is
     "no-proxy.com:80", curl will consider that a match, but your code
     does not.

I don't see any way around it besides implementing curl's full
tokenizing and matching algorithm, which is about a page of code. I'd
really prefer not to re-implement bits of curl (especially because they
may change later), but AFAIK there is no way to ask curl "is there a
proxy configured, and if so, what is it?".

The rest of this patch looks OK to me, though.

-Peff

Re: [PATCH 1/6] http: try http_proxy env var when http.proxy config option is not set

[Daniel Stenberg]

On Fri, 4 May 2012, Jeff King wrote:

> I don't see any way around it besides implementing curl's full tokenizing 
> and matching algorithm, which is about a page of code. I'd really prefer not 
> to re-implement bits of curl (especially because they may change later), but 
> AFAIK there is no way to ask curl "is there a proxy configured, and if so, 
> what is it?".

Sorry for being thick, but I lost track on this thread. Why does it need this 
info again?

Or perhaps put another way: if there was an ideal way to get this done or 
provide this to libcurl other than the current way, how would you suggest it 
would be done from a git internal point of view?

We're currently discussing new ways of providing authentication info in 
libcurl and I want to make sure I can get useful bits from this exercise into 
that talk to possibly offer something smoother in the future.

-- 

  / daniel.haxx.se

Re: [PATCH 1/6] http: try http_proxy env var when http.proxy config option is not set

[Jeff King]

On Fri, May 04, 2012 at 09:27:16AM +0200, Daniel Stenberg wrote:

> On Fri, 4 May 2012, Jeff King wrote:
> 
> >I don't see any way around it besides implementing curl's full
> >tokenizing and matching algorithm, which is about a page of code.
> >I'd really prefer not to re-implement bits of curl (especially
> >because they may change later), but AFAIK there is no way to ask
> >curl "is there a proxy configured, and if so, what is it?".
> 
> Sorry for being thick, but I lost track on this thread. Why does it
> need this info again?

We need to know information about the proxy in order to look up the
username and password in our credential database. Before the request is
made in some cases, and in others, after we see a 407. If we fed the
proxy to curl via CURLOPT_PROXY, it's easy. But if the proxy came from
the environment, we have to replicate curl's lookup rules.

> Or perhaps put another way: if there was an ideal way to get this
> done or provide this to libcurl other than the current way, how would
> you suggest it would be done from a git internal point of view?

The absolute simplest way for us would be to stop using
CURLOPT_PROXYUSERNAME/PASSWORD to set it ahead of time, and instead
provide a callback that curl would call on a 407. That callback would
just need the URL of the proxy, and would return the username/password
(or even just set them on the curl object via
CURLOPT_PROXYUSERNAME/PASSWORD).

For that matter, it would simplify our code to do the same for regular
http auth, too. And though we usually know our URL in that case, we
might not if we got a 302 with FOLLOWLOCATION set.

-Peff

Re: [PATCH 1/6] http: try http_proxy env var when http.proxy config option is not set

[Daniel Stenberg]

On Fri, 4 May 2012, Jeff King wrote:

> The absolute simplest way for us would be to stop using
> CURLOPT_PROXYUSERNAME/PASSWORD to set it ahead of time, and instead
> provide a callback that curl would call on a 407. That callback would
> just need the URL of the proxy, and would return the username/password
> (or even just set them on the curl object via
> CURLOPT_PROXYUSERNAME/PASSWORD).
>
> For that matter, it would simplify our code to do the same for regular http 
> auth, too. And though we usually know our URL in that case, we might not if 
> we got a 302 with FOLLOWLOCATION set.

Thanks a lot. That is in fact almost exactly the solution we're discussing 
right now.

-- 

  / daniel.haxx.se

Re: [PATCH 1/6] http: try http_proxy env var when http.proxy config option is not set

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, May 04, 2012 at 09:27:16AM +0200, Daniel Stenberg wrote:
>
>> On Fri, 4 May 2012, Jeff King wrote:
>> 
>> >I don't see any way around it besides implementing curl's full
>> >tokenizing and matching algorithm, which is about a page of code.
>> >I'd really prefer not to re-implement bits of curl (especially
>> >because they may change later), but AFAIK there is no way to ask
>> >curl "is there a proxy configured, and if so, what is it?".
>> 
>> Sorry for being thick, but I lost track on this thread. Why does it
>> need this info again?
>
> We need to know information about the proxy in order to look up the
> username and password in our credential database. Before the request is
> made in some cases, and in others, after we see a 407. If we fed the
> proxy to curl via CURLOPT_PROXY, it's easy. But if the proxy came from
> the environment, we have to replicate curl's lookup rules.
>
>> Or perhaps put another way: if there was an ideal way to get this
>> done or provide this to libcurl other than the current way, how would
>> you suggest it would be done from a git internal point of view?
>
> The absolute simplest way for us would be to stop using
> CURLOPT_PROXYUSERNAME/PASSWORD to set it ahead of time, and instead
> provide a callback that curl would call on a 407. That callback would
> just need the URL of the proxy, and would return the username/password
> (or even just set them on the curl object via
> CURLOPT_PROXYUSERNAME/PASSWORD).
>
> For that matter, it would simplify our code to do the same for regular
> http auth, too. And though we usually know our URL in that case, we
> might not if we got a 302 with FOLLOWLOCATION set.
>
> -Peff

Thanks for a nice summary, and I agree with your list of what we wish we
had from the cURL library.  With such a change, it becomes irrelevant how
the user fed cURL provisional (partial) authentication information, either
in http.proxy (which we turn into CURLOPT_PROXY), or from the environment
(without Git having to know anything about it), and a lot of complexity
that led to bugs in this series will become unnecessary.

I am tempted to suggest that the current series should be rerolled without
all the guessing and preauth tricks until such an update to the cURL
library materializes.

Re: [PATCH 1/6] http: try http_proxy env var when http.proxy config option is not set

[Jeff King]

On Fri, May 04, 2012 at 08:36:13AM -0700, Junio C Hamano wrote:

> Thanks for a nice summary, and I agree with your list of what we wish we
> had from the cURL library.  With such a change, it becomes irrelevant how
> the user fed cURL provisional (partial) authentication information, either
> in http.proxy (which we turn into CURLOPT_PROXY), or from the environment
> (without Git having to know anything about it), and a lot of complexity
> that led to bugs in this series will become unnecessary.
> 
> I am tempted to suggest that the current series should be rerolled without
> all the guessing and preauth tricks until such an update to the cURL
> library materializes.

I am very tempted by that, too. In the meantime (and even once that curl
version comes out and we write the new code, people will still be on the
older version of curl), we have a fallback: they can use http.proxy if
they want auth support. It's not as nice as supporting auth on the
environment variables, but I think it will end up being a lot cleaner.

-Peff

Re: [PATCH 1/6] http: try http_proxy env var when http.proxy config option is not set

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I am very tempted by that, too. In the meantime (and even once that curl
> version comes out and we write the new code, people will still be on the
> older version of curl), we have a fallback: they can use http.proxy if
> they want auth support. It's not as nice as supporting auth on the
> environment variables, but I think it will end up being a lot cleaner.

Sounds good.