Re: [RFC PATCH v2] http: support CURLPROXY_HTTPS

[Junio C Hamano <gitster@pobox.com>]

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