[PATCH] http: support CURLPROXY_HTTPS

[PATCH] http: support CURLPROXY_HTTPS

[Wei Shuyu]

HTTP proxy over SSL is supported by curl since 7.52.0.
This is very useful for networks with protocol whitelist.

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

diff --git a/http.c b/http.c
index 215bebef1..32d33261c 100644
--- a/http.c
+++ b/http.c
@@ -865,6 +865,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: [PATCH] http: support CURLPROXY_HTTPS

[Jonathan Nieder]

Hi,

Wei Shuyu wrote:

> HTTP proxy over SSL is supported by curl since 7.52.0.
> This is very useful for networks with protocol whitelist.
>
> Signed-off-by: Wei Shuyu <wsy@dogben.com>
> ---
>  http.c | 5 +++++
>  1 file changed, 5 insertions(+)

Thanks for writing this.  Can you give an example of how I'd use it
(ideally in the form of a test in t/ so we avoid this functionality
regressing, but if that's not straightforward then an example for the
commit message is fine as well)?

> diff --git a/http.c b/http.c
> index 215bebef1..32d33261c 100644
> --- a/http.c
> +++ b/http.c
> @@ -865,6 +865,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

Can this use #ifdef CURLPROXY_HTTPS instead?  That way, if someone's
copy of curl has backported support then they get the benefit of this
change as well.

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

Thanks and hope that helps,
Jonathan

Re: [PATCH] http: support CURLPROXY_HTTPS

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Hi,
>
> Wei Shuyu wrote:
>
>> HTTP proxy over SSL is supported by curl since 7.52.0.
>> This is very useful for networks with protocol whitelist.
>>
>> Signed-off-by: Wei Shuyu <wsy@dogben.com>
>> ---
>>  http.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>
> Thanks for writing this.  Can you give an example of how I'd use it
> (ideally in the form of a test in t/ so we avoid this functionality
> regressing, but if that's not straightforward then an example for the
> commit message is fine as well)?

Just FYI, here is an entry I added to the What's cooking report
(which will be used as the log message for a merge commit that pulls
this topic in, and will become an entry in the release notes if this
topic ever becomes a part of a release).

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

There are multiple ways other than http.proxy configuration variable
that a user can use to tell Git to use a proxy; I do not think the
log message of this change is a place to enumerate all of them, but
showing one of them to the readers would be good to remind them what
we are talking about, I would guess.

>> diff --git a/http.c b/http.c
>> index 215bebef1..32d33261c 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -865,6 +865,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
>
> Can this use #ifdef CURLPROXY_HTTPS instead?  That way, if someone's
> copy of curl has backported support then they get the benefit of this
> change as well.

It sounds like a worthwhile thing to do (assuming that these are
always implemented as preprocessor macros).

>> +		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);
>
> Thanks and hope that helps,
> Jonathan

Re: [PATCH] http: support CURLPROXY_HTTPS

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Wei Shuyu wrote:

>>> diff --git a/http.c b/http.c
>>> index 215bebef1..32d33261c 100644
>>> --- a/http.c
>>> +++ b/http.c
>>> @@ -865,6 +865,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
>>
>> Can this use #ifdef CURLPROXY_HTTPS instead?  That way, if someone's
>> copy of curl has backported support then they get the benefit of this
>> change as well.
>
> It sounds like a worthwhile thing to do (assuming that these are
> always implemented as preprocessor macros).

Oh, good point!  It's an enumerator, not a preprocessor macro.  But
there is a preprocessor macro CURL_VERSION_HTTPS_PROXY.

Anyway, using LIBCURL_VERSION_NUM is consistent with the surrounding
code.

Thanks,
Jonathan

Re: [PATCH] http: support CURLPROXY_HTTPS

[Wei Shuyu]

On 2017-12-20 10:22, Wei Shuyu wrote:

> CURLPROXY_HTTPS is intended for run-time detection. I don't think it's 
> a
> good idea to use it with #ifdef.

s/CURLPROXY_HTTPS/CURL_VERSION_HTTPS_PROXY/

Re: [PATCH] http: support CURLPROXY_HTTPS

[Jeff King]

On Wed, Dec 20, 2017 at 10:22:06AM +0800, Wei Shuyu wrote:

> On 2017-12-20 04:59, Jonathan Nieder wrote:
> 
> > Thanks for writing this.  Can you give an example of how I'd use it
> > (ideally in the form of a test in t/ so we avoid this functionality
> > regressing, but if that's not straightforward then an example for the
> > commit message is fine as well)?
> 
> Hi Jonathan,
> Its usage is the same as other protocols. Just set http.proxy or
> http_proxy/https_proxy
> environment to https://url.
> 
> To use apache server as a proxy, just add `ProxyRequests On` to an https
> site.

Unfortunately I don't think we have any proxy tests at all in our test
suite right now. The sticking point is that we need an actual proxy to
test against. :)

If it really is as simple as "ProxyRequests On", then we might be able
to convince the existing apache process we run to proxy requests to
itself (perhaps on a secondary port?).

-Peff