threads / patch / 45669

v4, 2 partshttp: few fixes for the proxy configuration handling

Subject: [PATCH v4 0/2] http: few fixes for the proxy configuration handling

## tl;dr

4 messages between Apr 11, 2017 and Apr 14, 2017. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Sergey Ryazanov· Apr 11, 2017, 20:22 UTC · lore
Hello,
this is few patches, which fixes regressions in the proxy handling.
Both patches are based on the maint branch.
Changes since v3:
 - fix grammar (thanks to Jeff)
Changes since v2:
 - fix grammar (thanks to Ævar)
 - add new patch which fixes the silent ignoring of proxy missconfiguration
Sergey Ryazanov (2):
  http: honor empty http.proxy option to bypass proxy
  http: fix the silent ignoring of proxy misconfiguraion
 http.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
-- 
2.10.2
Sergey Ryazanov· Apr 11, 2017, 20:22 UTC · re: Sergey Ryazanov · lore

[PATCH v4 1/2] http: honor empty http.proxy option to bypass proxy

Curl distinguishes between an empty proxy address and a NULL proxy address. In the first case it completely disables proxy usage, but if the proxy address option is NULL then curl attempts to determine the proxy address from the http_proxy environment variable.

According to the documentation, if the http.proxy option is set to an empty string, git should bypass 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 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 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.proxy option to not work anymore.

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

Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
Changes since v1:
 - explicitly handle this case instead of mangling the common code
Changes since v2:
 - fix grammar (thanks to Ævar)
Changes since v3:
 - no changes
 http.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
Show changes to http.c +8 −2
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
Sergey Ryazanov· Apr 11, 2017, 20:22 UTC · re: Sergey Ryazanov · lore

[PATCH v4 2/2] http: fix the silent ignoring of proxy misconfiguraion

Earlier, the whole http.proxy option string was passed to curl without any preprocessing so curl could complain about the invalid proxy configuration.

After the commit 372370f167 ("http: use credential API to handle proxy authentication", 2016-01-26), if the user specified an invalid HTTP proxy option in the configuration, then the option parsing silently fails and NULL will be passed to curl as a proxy. This forces curl to fall back to detecting the proxy configuration from the environment, causing the http.proxy option ignoring.

Fix this issue by checking the proxy option parsing result. If parsing failed then print an error message and die. Such behaviour allows the user to quickly figure the proxy misconfiguration and correct it.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
Changes since v2:
  - new patch
Changes since v3:
  - fix grammar (thanks to Jeff)
 http.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to http.c +3 −0
diff --git a/http.c b/http.c
index 8be75b2..82664dd 100644
--- a/http.c
+++ b/http.c
@@ -867,6 +867,9 @@ static CURL *get_curl_handle(void)
 			strbuf_release(&url);
 		}
 
+		if (!proxy_auth.host)
+			die("Invalid proxy URL '%s'", curl_http_proxy);
+
 		curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);
 #if LIBCURL_VERSION_NUM >= 0x071304
 		var_override(&curl_no_proxy, getenv("NO_PROXY"));

-- 
2.10.2
Jeff King· Apr 14, 2017, 21:23 UTC · re: Sergey Ryazanov · lore

Re: [PATCH v4 0/2] http: few fixes for the proxy configuration handling

On Tue, Apr 11, 2017 at 11:22:17PM +0300, Sergey Ryazanov wrote:
Show 17 quoted lines
> this is few patches, which fixes regressions in the proxy handling.
> 
> Both patches are based on the maint branch.
> 
> Changes since v3:
>  - fix grammar (thanks to Jeff)
> 
> Changes since v2:
>  - fix grammar (thanks to Ævar)
>  - add new patch which fixes the silent ignoring of proxy missconfiguration
> 
> Sergey Ryazanov (2):
>   http: honor empty http.proxy option to bypass proxy
>   http: fix the silent ignoring of proxy misconfiguraion
> 
>  http.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
This whole series looks good to me.
-Peff

← back to recent threads