[PATCH v2] http: honnor empty http.proxy option to bypass proxy

[PATCH v2] http: honnor empty http.proxy option to bypass proxy

[Sergey Ryazanov]

Curl distinguish between empty proxy address and NULL proxy address. In
the first case it completly disable proxy usage, but if proxy address
option is NULL then curl attempt to determine proxy address from
http_proxy environment variable.

According to documentation, if http.proxy configured to empty string
then git should bypass proxy and connects to the server directly:

    export http_proxy=http://network-proxy/
    cd ~/foobar-project
    git config remote.origin.proxy ""
    git fetch

Previously, proxy host was configured by one line:

    curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);

Commit 372370f (http: use credential API to handle proxy auth...) parses
proxy option, extracts proxy host address and additionaly updates curl
configuration, which makes previous call a noop:

    credential_from_url(&proxy_auth, curl_http_proxy);
    curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);

But if proxy option is empty then proxy host field become NULL this
force curl to fallback to proxy configuration detection from
environment. This caused empty http.proxy option not working any more.

Fix this issue by explicitly handling empty http.proxy option. This also
makes code a bit more clear and should help us avoid such regressions in
the future.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---

Changes since v1:
 - explicitly handle this case instead of mangling the common code

 http.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/http.c b/http.c
index 96d84bb..8be75b2 100644
--- a/http.c
+++ b/http.c
@@ -836,8 +836,14 @@ static CURL *get_curl_handle(void)
 		}
 	}
 
-	if (curl_http_proxy) {
-		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+	if (curl_http_proxy && curl_http_proxy[0] == '\0') {
+		/*
+		 * Handle case with the empty http.proxy value here to keep
+		 * common code clean.
+		 * NB: empty option disables proxying at all.
+		 */
+		curl_easy_setopt(result, CURLOPT_PROXY, "");
+	} else if (curl_http_proxy) {
 #if LIBCURL_VERSION_NUM >= 0x071800
 		if (starts_with(curl_http_proxy, "socks5h"))
 			curl_easy_setopt(result,
-- 
2.10.2

Re: [PATCH v2] http: honnor empty http.proxy option to bypass proxy

[Ævar Arnfjörð Bjarmason]

Since nobody pointed this out already, some grammar/spelling fixes.
Also CC-ing Knut who wrote the commit you're referencing.

- http: honnor empty http.proxy option to bypass proxy
+ http: honor empty http.proxy option to bypass proxy

On Tue, Apr 11, 2017 at 11:20 AM, Sergey Ryazanov
<ryazanov.s.a@gmail.com> wrote:
> Curl distinguish between empty proxy address and NULL proxy address. In


...distinguishes between an empty proxy address and a NULL proxy address...

> the first case it completly disable proxy usage, but if proxy address

it completely disables ... but if the proxy ....

> option is NULL then curl attempt to determine proxy address from

...attempts to determine the proxy ...

> http_proxy environment variable.

... the http_....


> According to documentation, if http.proxy configured to empty string
> then git should bypass proxy and connects to the server directly:

...to the documentation, if http_proxy is set to an empty string, git
should bypass the proxy and connect to the server directly.


>
>     export http_proxy=http://network-proxy/
>     cd ~/foobar-project
>     git config remote.origin.proxy ""
>     git fetch
>
> Previously, proxy host was configured by one line:
>
>     curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>
> Commit 372370f (http: use credential API to handle proxy auth...) parses
> proxy option, extracts proxy host address and additionaly updates curl
> configuration, which makes previous call a noop:

[Using the format we usually cite commits with, see SubmittingPatches]

Commit 372370f167 ("http: use credential API to handle proxy
authentication", 2016-01-26) parses the proxy option, then extracts
the proxy host address and updates the curl configuration, making the
previous call a noop.

>
>     credential_from_url(&proxy_auth, curl_http_proxy);
>     curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);
>
> But if proxy option is empty then proxy host field become NULL this
> force curl to fallback to proxy configuration detection from
> environment. This caused empty http.proxy option not working any more.

But if the proxy option is empty then the proxy host field becomes
NULL. This forces curl to fall back to detecting the proxy
configuration from the environment, causing the http.rpoxy option to
not work anymore.

>
> Fix this issue by explicitly handling empty http.proxy option. This also
> makes code a bit more clear and should help us avoid such regressions in
> the future.

Fix this issue by explicitly handling http.proxy being set to the
empty string. This also makes the code a ....

>
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> ---
>
> Changes since v1:
>  - explicitly handle this case instead of mangling the common code
>
>  http.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/http.c b/http.c
> index 96d84bb..8be75b2 100644
> --- a/http.c
> +++ b/http.c
> @@ -836,8 +836,14 @@ static CURL *get_curl_handle(void)
>                 }
>         }
>
> -       if (curl_http_proxy) {
> -               curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +       if (curl_http_proxy && curl_http_proxy[0] == '\0') {
> +               /*
> +                * Handle case with the empty http.proxy value here to keep
> +                * common code clean.
> +                * NB: empty option disables proxying at all.
> +                */
> +               curl_easy_setopt(result, CURLOPT_PROXY, "");
> +       } else if (curl_http_proxy) {
>  #if LIBCURL_VERSION_NUM >= 0x071800
>                 if (starts_with(curl_http_proxy, "socks5h"))
>                         curl_easy_setopt(result,
> --
> 2.10.2
>

Re: [PATCH v2] http: honnor empty http.proxy option to bypass proxy

[Sergey Ryazanov]

On Tue, Apr 11, 2017 at 1:09 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Since nobody pointed this out already, some grammar/spelling fixes.
> Also CC-ing Knut who wrote the commit you're referencing.
>
Thanks a lot! Will fix in v3.

-- 
Sergey

Re: [PATCH v2] http: honnor empty http.proxy option to bypass proxy

[Jeff King]

On Tue, Apr 11, 2017 at 12:20:50PM +0300, Sergey Ryazanov wrote:

> diff --git a/http.c b/http.c
> index 96d84bb..8be75b2 100644
> --- a/http.c
> +++ b/http.c
> @@ -836,8 +836,14 @@ static CURL *get_curl_handle(void)
>  		}
>  	}
>  
> -	if (curl_http_proxy) {
> -		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +	if (curl_http_proxy && curl_http_proxy[0] == '\0') {
> +		/*
> +		 * Handle case with the empty http.proxy value here to keep
> +		 * common code clean.
> +		 * NB: empty option disables proxying at all.
> +		 */
> +		curl_easy_setopt(result, CURLOPT_PROXY, "");
> +	} else if (curl_http_proxy) {

This seems pretty reasonable. You could bump this under the existing "if
(curl_http_proxy)" condition, but that would add an extra level of
indentation to the rest of the parsing.

One thing I do wonder, though: can credential_from_url() ever return a
NULL host field for other inputs, and what should we do on those inputs?

For example, if I set the value to just "https://" with no hostname,
then I think we'll end up with a NULL proxy_auth.host field. And we'll
feed that NULL to CURLOPT_PROXY, which will cause it to use the
defaults.

I don't know _what_ "https://" should do. It's clearly bogus. But
telling curl to use the defaults seems funny. In that sense, your
original was much better (we'd feed it to curl, which would be free to
complain).

-Peff

Re: [PATCH v2] http: honnor empty http.proxy option to bypass proxy

[Sergey Ryazanov]

On Tue, Apr 11, 2017 at 4:06 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Apr 11, 2017 at 12:20:50PM +0300, Sergey Ryazanov wrote:
>> diff --git a/http.c b/http.c
>> index 96d84bb..8be75b2 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -836,8 +836,14 @@ static CURL *get_curl_handle(void)
>>               }
>>       }
>>
>> -     if (curl_http_proxy) {
>> -             curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>> +     if (curl_http_proxy && curl_http_proxy[0] == '\0') {
>> +             /*
>> +              * Handle case with the empty http.proxy value here to keep
>> +              * common code clean.
>> +              * NB: empty option disables proxying at all.
>> +              */
>> +             curl_easy_setopt(result, CURLOPT_PROXY, "");
>> +     } else if (curl_http_proxy) {
>
> This seems pretty reasonable. You could bump this under the existing "if
> (curl_http_proxy)" condition, but that would add an extra level of
> indentation to the rest of the parsing.
>
Because of this additional indentation I decide to add new if () block.

> One thing I do wonder, though: can credential_from_url() ever return a
> NULL host field for other inputs, and what should we do on those inputs?
>
> For example, if I set the value to just "https://" with no hostname,
> then I think we'll end up with a NULL proxy_auth.host field. And we'll
> feed that NULL to CURLOPT_PROXY, which will cause it to use the
> defaults.
>
> I don't know _what_ "https://" should do. It's clearly bogus. But
> telling curl to use the defaults seems funny. In that sense, your
> original was much better (we'd feed it to curl, which would be free to
> complain).
>

I thought about this situation too. IMHO the best solution here is to
check host after credential_from_url() call. If host is NULL then we
should show warning message and exit. Then user could fix its
configuration.

Since I in any case will send v3 with grammar fixes then I could add
new patch to the series. This new patch will check host for NULL and
print warning message. Are you Ok with such solution?

-- 
Sergey

Re: [PATCH v2] http: honnor empty http.proxy option to bypass proxy

[Jeff King]

On Tue, Apr 11, 2017 at 05:56:56PM +0300, Sergey Ryazanov wrote:

> > I don't know _what_ "https://" should do. It's clearly bogus. But
> > telling curl to use the defaults seems funny. In that sense, your
> > original was much better (we'd feed it to curl, which would be free to
> > complain).
> >
> 
> I thought about this situation too. IMHO the best solution here is to
> check host after credential_from_url() call. If host is NULL then we
> should show warning message and exit. Then user could fix its
> configuration.
> 
> Since I in any case will send v3 with grammar fixes then I could add
> new patch to the series. This new patch will check host for NULL and
> print warning message. Are you Ok with such solution?

Yeah, I think that would be fine. It pretty clearly seems like an error,
so any behavior that isn't "quietly pretend that there is no proxy" is
probably fine.

-Peff