threads / rfc / 47490

RFC v2http: support CURLPROXY_HTTPS

Subject: [RFC PATCH v2] http: support CURLPROXY_HTTPS

## tl;dr

4 messages between Dec 23, 2017 and Dec 27, 2017. Diffs are folded; open one to read it.

replies: 3people: 4as markdown or json

Wei Shuyu· Dec 23, 2017, 15:02 UTC · lore

Git has been taught to support an https:// used for http.proxy when using recent versions of libcurl.

To use https proxy with self-signed certs, we need a way to unset CURLOPT_PROXY_SSL_VERIFYPEER and CURLOPT_PROXY_SSL_VERIFYHOST just like direct SSL connection. This is required if we want use t/lib-httpd to test proxy.

In this patch I reuse http.sslverify to do this, do we need an independent option like http.sslproxyverify?

To fully support https proxy, we also need ways to set more options such as CURLOPT_PROXY_SSLCERT. However, I'm not sure if we need to support them.

Signed-off-by: Wei Shuyu <wsy@dogben.com>
---
 http.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Show changes to http.c +9 −0

diff --git a/http.c b/http.c
index 215bebef1..d8a5e48f0 100644
--- a/http.c
+++ b/http.c
@@ -708,6 +708,10 @@ static CURL *get_curl_handle(void)
 	if (!curl_ssl_verify) {
 		curl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 0);
 		curl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 0);
+#if LIBCURL_VERSION_NUM >= 0x073400
+		curl_easy_setopt(result, CURLOPT_PROXY_SSL_VERIFYPEER, 0);
+		curl_easy_setopt(result, CURLOPT_PROXY_SSL_VERIFYHOST, 0);
+#endif
 	} else {
 		/* Verify authenticity of the peer's certificate */
 		curl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 1);
@@ -865,6 +869,11 @@ static CURL *get_curl_handle(void)
 		else if (starts_with(curl_http_proxy, "socks"))
 			curl_easy_setopt(result,
 				CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4);
+#endif
+#if LIBCURL_VERSION_NUM >= 0x073400
+		else if (starts_with(curl_http_proxy, "https"))
+			curl_easy_setopt(result,
+				CURLOPT_PROXYTYPE, CURLPROXY_HTTPS);
 #endif
 		if (strstr(curl_http_proxy, "://"))
 			credential_from_url(&proxy_auth, curl_http_proxy);

-- 
2.15.1

Ævar Arnfjörð Bjarmason· Dec 23, 2017, 17:48 UTC · re: Wei Shuyu · lore

Re: [RFC PATCH v2] http: support CURLPROXY_HTTPS

On Sat, Dec 23 2017, Wei Shuyu jotted:

Show 14 quoted lines

> Git has been taught to support an https:// used for http.proxy when
> using recent versions of libcurl.
>
> To use https proxy with self-signed certs, we need a way to
> unset CURLOPT_PROXY_SSL_VERIFYPEER and CURLOPT_PROXY_SSL_VERIFYHOST
> just like direct SSL connection. This is required if we want
> use t/lib-httpd to test proxy.
>
> In this patch I reuse http.sslverify to do this, do we need an
> independent option like http.sslproxyverify?
>
> To fully support https proxy, we also need ways to set more options
> such as CURLOPT_PROXY_SSLCERT. However, I'm not sure if we need to
> support them.

It would be good to add a link to https://daniel.haxx.se/blog/2016/11/26/https-proxy-with-curl/ to the commit message, since it explains in great detail what this is for and how it compares to what we were doing before.

Show 32 quoted lines

> Signed-off-by: Wei Shuyu <wsy@dogben.com>
> ---
>  http.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/http.c b/http.c
> index 215bebef1..d8a5e48f0 100644
> --- a/http.c
> +++ b/http.c
> @@ -708,6 +708,10 @@ static CURL *get_curl_handle(void)
>  	if (!curl_ssl_verify) {
>  		curl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 0);
>  		curl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 0);
> +#if LIBCURL_VERSION_NUM >= 0x073400
> +		curl_easy_setopt(result, CURLOPT_PROXY_SSL_VERIFYPEER, 0);
> +		curl_easy_setopt(result, CURLOPT_PROXY_SSL_VERIFYHOST, 0);
> +#endif
>  	} else {
>  		/* Verify authenticity of the peer's certificate */
>  		curl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 1);
> @@ -865,6 +869,11 @@ static CURL *get_curl_handle(void)
>  		else if (starts_with(curl_http_proxy, "socks"))
>  			curl_easy_setopt(result,
>  				CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4);
> +#endif
> +#if LIBCURL_VERSION_NUM >= 0x073400
> +		else if (starts_with(curl_http_proxy, "https"))
> +			curl_easy_setopt(result,
> +				CURLOPT_PROXYTYPE, CURLPROXY_HTTPS);
>  #endif
>  		if (strstr(curl_http_proxy, "://"))
>  			credential_from_url(&proxy_auth, curl_http_proxy);

Junio C Hamano· Dec 27, 2017, 18:44 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC PATCH v2] http: support CURLPROXY_HTTPS

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 21 quoted lines

> On Sat, Dec 23 2017, Wei Shuyu jotted:
>
>> Git has been taught to support an https:// used for http.proxy when
>> using recent versions of libcurl.
>>
>> To use https proxy with self-signed certs, we need a way to
>> unset CURLOPT_PROXY_SSL_VERIFYPEER and CURLOPT_PROXY_SSL_VERIFYHOST
>> just like direct SSL connection. This is required if we want
>> use t/lib-httpd to test proxy.
>>
>> In this patch I reuse http.sslverify to do this, do we need an
>> independent option like http.sslproxyverify?
>>
>> To fully support https proxy, we also need ways to set more options
>> such as CURLOPT_PROXY_SSLCERT. However, I'm not sure if we need to
>> support them.
>
> It would be good to add a link to
> https://daniel.haxx.se/blog/2016/11/26/https-proxy-with-curl/ to the
> commit message, since it explains in great detail what this is for and
> how it compares to what we were doing before.

Yeah, I found it a quite readable explanation.

It appears to me that the configuration for the proxy would have to be different from the configuration for the hosts in the outside world. You may want to be a lot more strongly suspicious to the latter while trusting the former a bit more. I also suspect that we'd need options like PROXY_SSLCERT that are parallel to the main SSL support in the final form, but it may be OK to leave it to a follow-up series.

And in order to start small and grow later, I think it is not a good idea to repurpose http.sslverify to imply http.sslproxyverify; you are declaring "when you disable peer verification, you also disable proxy verification", and need to worry about backward compatibility when you later introduce http.sslproxyverify that can control these independently. It probably is less bad than usual b/c issues in that it probably is much more common to disable verification for proxy than target hosts, though.

In any case, thanks for working on this.

Show 33 quoted lines

>
>> Signed-off-by: Wei Shuyu <wsy@dogben.com>
>> ---
>>  http.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/http.c b/http.c
>> index 215bebef1..d8a5e48f0 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -708,6 +708,10 @@ static CURL *get_curl_handle(void)
>>  	if (!curl_ssl_verify) {
>>  		curl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 0);
>>  		curl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 0);
>> +#if LIBCURL_VERSION_NUM >= 0x073400
>> +		curl_easy_setopt(result, CURLOPT_PROXY_SSL_VERIFYPEER, 0);
>> +		curl_easy_setopt(result, CURLOPT_PROXY_SSL_VERIFYHOST, 0);
>> +#endif
>>  	} else {
>>  		/* Verify authenticity of the peer's certificate */
>>  		curl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 1);
>> @@ -865,6 +869,11 @@ static CURL *get_curl_handle(void)
>>  		else if (starts_with(curl_http_proxy, "socks"))
>>  			curl_easy_setopt(result,
>>  				CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4);
>> +#endif
>> +#if LIBCURL_VERSION_NUM >= 0x073400
>> +		else if (starts_with(curl_http_proxy, "https"))
>> +			curl_easy_setopt(result,
>> +				CURLOPT_PROXYTYPE, CURLPROXY_HTTPS);
>>  #endif
>>  		if (strstr(curl_http_proxy, "://"))
>>  			credential_from_url(&proxy_auth, curl_http_proxy);

Jonathan Nieder· Dec 27, 2017, 18:29 UTC · re: Wei Shuyu · lore

Re: [RFC PATCH v2] http: support CURLPROXY_HTTPS

Wei Shuyu wrote:

> Git has been taught to support an https:// used for http.proxy when
> using recent versions of libcurl.

nit: commit messages use the imperative mood, as though commanding the code base to do something:

	Support https:// for http.proxy when using recent versions of
	curl.

[...]

> To use https proxy with self-signed certs, we need a way to
> unset CURLOPT_PROXY_SSL_VERIFYPEER and CURLOPT_PROXY_SSL_VERIFYHOST
> just like direct SSL connection. This is required if we want
> use t/lib-httpd to test proxy.

Interesting.  Sounds promising.

> In this patch I reuse http.sslverify to do this, do we need an
> independent option like http.sslproxyverify?

I think a separate option is a good idea.  This way, I can use

	[http "https://example.com"]
		sslVerify = false

to fetch from a misconfigured server or

	[http "https://example.com"]
		proxy = https://127.0.0.1
		sslVerifyProxy = false

to proxy through a misconfigured proxy. Alternatively, is there a straightforward way to make

	[http "https://example.com"]
		proxy = https://127.0.0.1
	[http "https://127.0.0.1"]
		sslVerify = false

do that?  Something like

	struct urlmatch_config proxy_config = { STRING_LIST_INIT_DUP };
	config.section = "http";
	config.key = NULL;
	config.collect_fn = proxy_options;
	config.cascade_fn = git_default_config;
	config.cb = NULL;
	config.url = normalized_proxy_url;
	git_config(urlmatch_config_entry, &config);

How does this interact with the GIT_SSL_NO_VERIFY environment variable?

> To fully support https proxy, we also need ways to set more options
> such as CURLOPT_PROXY_SSLCERT. However, I'm not sure if we need to
> support them.

That's for client certs, right? Would it work to make that configurable as

	[http "https://127.0.0.1"]
		sslCert = ...

That said, I agree with you that it's not a prerequisite for this initial patch.

Thanks, Jonathan