[RFC PATCH v2] http: support CURLPROXY_HTTPS

[RFC PATCH v2] http: support CURLPROXY_HTTPS

[Wei Shuyu]

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(+)

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

Re: [RFC PATCH v2] http: support CURLPROXY_HTTPS

[Ævar Arnfjörð Bjarmason]

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.

> 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);

Re: [RFC PATCH v2] http: support CURLPROXY_HTTPS

[Jonathan Nieder]

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

Re: [RFC PATCH v2] http: support CURLPROXY_HTTPS

[Junio C Hamano]

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

> 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.

>
>> 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);