threads / patch / 47466

patchhttp: support CURLPROXY_HTTPS

Subject: [PATCH] http: support CURLPROXY_HTTPS

## tl;dr

6 messages between Dec 19, 2017 and Dec 20, 2017. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Wei Shuyu· Dec 19, 2017, 17:24 UTC · lore

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

Show changes to http.c +5 −0

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

Jonathan Nieder· Dec 19, 2017, 20:59 UTC · re: Wei Shuyu · lore

Re: [PATCH] http: support CURLPROXY_HTTPS

Hi,

Wei Shuyu wrote:

Show 7 quoted lines

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

Show 10 quoted lines

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

Show 6 quoted lines

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

Junio C Hamano· Dec 19, 2017, 21:38 UTC · re: Jonathan Nieder · lore

Re: [PATCH] http: support CURLPROXY_HTTPS

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 16 quoted lines

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

Show 14 quoted lines

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

Show 9 quoted lines

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

Jonathan Nieder· Dec 19, 2017, 21:50 UTC · re: Junio C Hamano · lore

Re: [PATCH] http: support CURLPROXY_HTTPS

Junio C Hamano wrote:

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

Show 17 quoted lines

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

Wei Shuyu· Dec 20, 2017, 02:30 UTC · lore

Re: [PATCH] http: support CURLPROXY_HTTPS

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/

Jeff King· Dec 20, 2017, 11:41 UTC · lore

Re: [PATCH] http: support CURLPROXY_HTTPS

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

Show 14 quoted lines

> 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