threads / patch / 29799

v2, 3 partshttp: try http_proxy env var when http.proxy config option is not set

Subject: [PATCH v2 2/3] http: try http_proxy env var when http.proxy config option is not set

## tl;dr

6 messages between Mar 1, 2012 and Mar 1, 2012. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Nelson Benitez Leon· Mar 1, 2012, 18:21 UTC · lore

CuRL already reads it, but if $http_proxy has username but no password curl will not ask you for the password.. so we read it ourselves to detect that and ask for the password.

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

Show changes to http.c +7 −0

diff --git a/http.c b/http.c
index 8ac8eb6..8932da5 100644
--- a/http.c
+++ b/http.c
@@ -295,6 +295,13 @@ static CURL *get_curl_handle(void)
 	if (curl_ftp_no_epsv)
 		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);

+	if (!curl_http_proxy) {
+		const char *env_proxy;
+		env_proxy = getenv("http_proxy");
+		if (env_proxy) {
+			curl_http_proxy = xstrdup(env_proxy);
+		}
+	}
 	if (curl_http_proxy) {
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
 		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);

-- 
1.7.7.6

Sam Vilain· Mar 1, 2012, 17:45 UTC · re: Nelson Benitez Leon · lore

Re: [PATCH v2 2/3] http: try http_proxy env var when http.proxy config option is not set

On 3/1/12 10:21 AM, Nelson Benitez Leon wrote:

> CuRL already reads it, but if $http_proxy has username but no password
> curl will not ask you for the password.. so we read it ourselves to
> detect that and ask for the password.

That's not what this change does. This change explicitly loads from the environment the 'http_proxy' variable and sets up curl to use it. As Junio said, this is (on its own) a regression.

Sam

Show 23 quoted lines

> Signed-off-by: Nelson Benitez Leon<nbenitezl@gmail.com>
> ---
>   http.c |    7 +++++++
>   1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/http.c b/http.c
> index 8ac8eb6..8932da5 100644
> --- a/http.c
> +++ b/http.c
> @@ -295,6 +295,13 @@ static CURL *get_curl_handle(void)
>   	if (curl_ftp_no_epsv)
>   		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
>
> +	if (!curl_http_proxy) {
> +		const char *env_proxy;
> +		env_proxy = getenv("http_proxy");
> +		if (env_proxy) {
> +			curl_http_proxy = xstrdup(env_proxy);
> +		}
> +	}
>   	if (curl_http_proxy) {
>   		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>   		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);

Junio C Hamano· Mar 1, 2012, 18:33 UTC · re: Sam Vilain · lore

Re: [PATCH v2 2/3] http: try http_proxy env var when http.proxy config option is not set

Sam Vilain <sam@vilain.net> writes:

Show 8 quoted lines

> On 3/1/12 10:21 AM, Nelson Benitez Leon wrote:
>> CuRL already reads it, but if $http_proxy has username but no password
>> curl will not ask you for the password.. so we read it ourselves to
>> detect that and ask for the password.
>
> That's not what this change does.  This change explicitly loads from
> the environment the 'http_proxy' variable and sets up curl to use it.
> As Junio said, this is (on its own) a regression.

Just to make sure there is no understanding down the road, I only expressed a concern that this _might_ be a regression. That Mac OS X behaviour is not something I observed first-hand.

Junio C Hamano· Mar 1, 2012, 19:10 UTC · re: Nelson Benitez Leon · lore

Re: [PATCH v2 2/3] http: try http_proxy env var when http.proxy config option is not set

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.. so we read it ourselves to
> detect that and ask for the password.

Please stop the double-dot. Also your capitalization for cURL is screwed up.

More importantly, please describe what happens after "will not ask". "will not ask you for the password and the connection fails"? "will not ask you for the password and the gives an error message saying 'authentication failure'"?

The logic in the patch, needless to say, seems OK, though.

Thanks.

Show 24 quoted lines

>
> Signed-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>
> ---
>  http.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/http.c b/http.c
> index 8ac8eb6..8932da5 100644
> --- a/http.c
> +++ b/http.c
> @@ -295,6 +295,13 @@ static CURL *get_curl_handle(void)
>  	if (curl_ftp_no_epsv)
>  		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
>
> +	if (!curl_http_proxy) {
> +		const char *env_proxy;
> +		env_proxy = getenv("http_proxy");
> +		if (env_proxy) {
> +			curl_http_proxy = xstrdup(env_proxy);
> +		}
> +	}
>  	if (curl_http_proxy) {
>  		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>  		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);

Jeff King· Mar 1, 2012, 21:01 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 2/3] http: try http_proxy env var when http.proxy config option is not set

On Thu, Mar 01, 2012 at 11:10:38AM -0800, Junio C Hamano wrote:

Show 13 quoted lines

> 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.. so we read it ourselves to
> > detect that and ask for the password.
> 
> Please stop the double-dot.  Also your capitalization for cURL is screwed
> up.
> 
> More importantly, please describe what happens after "will not ask".
> "will not ask you for the password and the connection fails"?
> "will not ask you for the password and the gives an error message saying
> 'authentication failure'"?

When we need to authenticate for the destination webserver, we detect an HTTP 401, _then_ ask for the credentials, and retry the request. I'm curious what the error condition is for the authentication failure, and if we can do the same here (from a brief skim of rfc2616, it looks like it should be a 407, but I do not even have a proxy set up to try).

-Peff

Junio C Hamano· Mar 1, 2012, 21:38 UTC · re: Jeff King · lore

Re: [PATCH v2 2/3] http: try http_proxy env var when http.proxy config option is not set

Jeff King <peff@peff.net> writes:

> When we need to authenticate for the destination webserver, we detect an
> HTTP 401, _then_ ask for the credentials, and retry the request. I'm
> curious what the error condition is for the authentication failure, and
> if we can do the same here.

Yeah, that would be ideal.