{"thread":{"id":"47490","subject":"[RFC PATCH v2] http: support CURLPROXY_HTTPS","startedAt":"2017-12-23T15:02:53Z","lastAt":"2017-12-27T18:45:04Z","messageCount":4,"participants":["Wei Shuyu","Ævar Arnfjörð Bjarmason","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"335227","messageId":"20171223150215.8615-1-wsy@dogben.com","threadId":"47490","inReplyTo":null,"subject":"[RFC PATCH v2] http: support CURLPROXY_HTTPS","fromName":"Wei Shuyu","fromEmail":"wsy@dogben.com","sentAt":"2017-12-23T15:02:15Z","receivedAt":"2017-12-23T15:02:53Z","isPatch":true,"sender":{"key":"wsy@dogben.com","avatar":"https://avatars.githubusercontent.com/u/7259503?v=4"},"body":"Git has been taught to support an https:// used for http.proxy when\nusing recent versions of libcurl.\n\nTo use https proxy with self-signed certs, we need a way to\nunset CURLOPT_PROXY_SSL_VERIFYPEER and CURLOPT_PROXY_SSL_VERIFYHOST\njust like direct SSL connection. This is required if we want\nuse t/lib-httpd to test proxy.\n\nIn this patch I reuse http.sslverify to do this, do we need an\nindependent option like http.sslproxyverify?\n\nTo fully support https proxy, we also need ways to set more options\nsuch as CURLOPT_PROXY_SSLCERT. However, I'm not sure if we need to\nsupport them.\n\nSigned-off-by: Wei Shuyu <wsy@dogben.com>\n---\n http.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 215bebef1..d8a5e48f0 100644\n--- a/http.c\n+++ b/http.c\n@@ -708,6 +708,10 @@ static CURL *get_curl_handle(void)\n \tif (!curl_ssl_verify) {\n \t\tcurl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 0);\n \t\tcurl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 0);\n+#if LIBCURL_VERSION_NUM >= 0x073400\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXY_SSL_VERIFYPEER, 0);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXY_SSL_VERIFYHOST, 0);\n+#endif\n \t} else {\n \t\t/* Verify authenticity of the peer's certificate */\n \t\tcurl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 1);\n@@ -865,6 +869,11 @@ static CURL *get_curl_handle(void)\n \t\telse if (starts_with(curl_http_proxy, \"socks\"))\n \t\t\tcurl_easy_setopt(result,\n \t\t\t\tCURLOPT_PROXYTYPE, CURLPROXY_SOCKS4);\n+#endif\n+#if LIBCURL_VERSION_NUM >= 0x073400\n+\t\telse if (starts_with(curl_http_proxy, \"https\"))\n+\t\t\tcurl_easy_setopt(result,\n+\t\t\t\tCURLOPT_PROXYTYPE, CURLPROXY_HTTPS);\n #endif\n \t\tif (strstr(curl_http_proxy, \"://\"))\n \t\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n-- \n2.15.1\n\n"},{"id":"335229","messageId":"878tdts5c0.fsf@evledraar.gmail.com","threadId":"47490","inReplyTo":"20171223150215.8615-1-wsy@dogben.com","subject":"Re: [RFC PATCH v2] http: support CURLPROXY_HTTPS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-23T17:48:47Z","receivedAt":"2017-12-23T17:48:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Dec 23 2017, Wei Shuyu jotted:\n\n> Git has been taught to support an https:// used for http.proxy when\n> using recent versions of libcurl.\n>\n> To use https proxy with self-signed certs, we need a way to\n> unset CURLOPT_PROXY_SSL_VERIFYPEER and CURLOPT_PROXY_SSL_VERIFYHOST\n> just like direct SSL connection. This is required if we want\n> use t/lib-httpd to test proxy.\n>\n> In this patch I reuse http.sslverify to do this, do we need an\n> independent option like http.sslproxyverify?\n>\n> To fully support https proxy, we also need ways to set more options\n> such as CURLOPT_PROXY_SSLCERT. However, I'm not sure if we need to\n> support them.\n\nIt would be good to add a link to\nhttps://daniel.haxx.se/blog/2016/11/26/https-proxy-with-curl/ to the\ncommit message, since it explains in great detail what this is for and\nhow it compares to what we were doing before.\n\n> Signed-off-by: Wei Shuyu <wsy@dogben.com>\n> ---\n>  http.c | 9 +++++++++\n>  1 file changed, 9 insertions(+)\n>\n> diff --git a/http.c b/http.c\n> index 215bebef1..d8a5e48f0 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -708,6 +708,10 @@ static CURL *get_curl_handle(void)\n>  \tif (!curl_ssl_verify) {\n>  \t\tcurl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 0);\n>  \t\tcurl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 0);\n> +#if LIBCURL_VERSION_NUM >= 0x073400\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROXY_SSL_VERIFYPEER, 0);\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROXY_SSL_VERIFYHOST, 0);\n> +#endif\n>  \t} else {\n>  \t\t/* Verify authenticity of the peer's certificate */\n>  \t\tcurl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 1);\n> @@ -865,6 +869,11 @@ static CURL *get_curl_handle(void)\n>  \t\telse if (starts_with(curl_http_proxy, \"socks\"))\n>  \t\t\tcurl_easy_setopt(result,\n>  \t\t\t\tCURLOPT_PROXYTYPE, CURLPROXY_SOCKS4);\n> +#endif\n> +#if LIBCURL_VERSION_NUM >= 0x073400\n> +\t\telse if (starts_with(curl_http_proxy, \"https\"))\n> +\t\t\tcurl_easy_setopt(result,\n> +\t\t\t\tCURLOPT_PROXYTYPE, CURLPROXY_HTTPS);\n>  #endif\n>  \t\tif (strstr(curl_http_proxy, \"://\"))\n>  \t\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n"},{"id":"335373","messageId":"20171227182931.GD149622@aiede.mtv.corp.google.com","threadId":"47490","inReplyTo":"20171223150215.8615-1-wsy@dogben.com","subject":"Re: [RFC PATCH v2] http: support CURLPROXY_HTTPS","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-12-27T18:29:31Z","receivedAt":"2017-12-27T18:29:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Wei Shuyu wrote:\n\n> Git has been taught to support an https:// used for http.proxy when\n> using recent versions of libcurl.\n\nnit: commit messages use the imperative mood, as though commanding the\ncode base to do something:\n\n\tSupport https:// for http.proxy when using recent versions of\n\tcurl.\n\n[...]\n> To use https proxy with self-signed certs, we need a way to\n> unset CURLOPT_PROXY_SSL_VERIFYPEER and CURLOPT_PROXY_SSL_VERIFYHOST\n> just like direct SSL connection. This is required if we want\n> use t/lib-httpd to test proxy.\n\nInteresting.  Sounds promising.\n\n> In this patch I reuse http.sslverify to do this, do we need an\n> independent option like http.sslproxyverify?\n\nI think a separate option is a good idea.  This way, I can use\n\n\t[http \"https://example.com\"]\n\t\tsslVerify = false\n\nto fetch from a misconfigured server or\n\n\t[http \"https://example.com\"]\n\t\tproxy = https://127.0.0.1\n\t\tsslVerifyProxy = false\n\nto proxy through a misconfigured proxy.  Alternatively, is there a\nstraightforward way to make\n\n\t[http \"https://example.com\"]\n\t\tproxy = https://127.0.0.1\n\t[http \"https://127.0.0.1\"]\n\t\tsslVerify = false\n\ndo that?  Something like\n\n\tstruct urlmatch_config proxy_config = { STRING_LIST_INIT_DUP };\n\tconfig.section = \"http\";\n\tconfig.key = NULL;\n\tconfig.collect_fn = proxy_options;\n\tconfig.cascade_fn = git_default_config;\n\tconfig.cb = NULL;\n\tconfig.url = normalized_proxy_url;\n\tgit_config(urlmatch_config_entry, &config);\n\nHow does this interact with the GIT_SSL_NO_VERIFY environment\nvariable?\n\n> To fully support https proxy, we also need ways to set more options\n> such as CURLOPT_PROXY_SSLCERT. However, I'm not sure if we need to\n> support them.\n\nThat's for client certs, right?  Would it work to make that\nconfigurable as\n\n\t[http \"https://127.0.0.1\"]\n\t\tsslCert = ...\n\n?\n\nThat said, I agree with you that it's not a prerequisite for this\ninitial patch.\n\nThanks,\nJonathan\n"},{"id":"335376","messageId":"xmqq4locnh7b.fsf@gitster.mtv.corp.google.com","threadId":"47490","inReplyTo":"878tdts5c0.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH v2] http: support CURLPROXY_HTTPS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-27T18:44:56Z","receivedAt":"2017-12-27T18:45:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> On Sat, Dec 23 2017, Wei Shuyu jotted:\n>\n>> Git has been taught to support an https:// used for http.proxy when\n>> using recent versions of libcurl.\n>>\n>> To use https proxy with self-signed certs, we need a way to\n>> unset CURLOPT_PROXY_SSL_VERIFYPEER and CURLOPT_PROXY_SSL_VERIFYHOST\n>> just like direct SSL connection. This is required if we want\n>> use t/lib-httpd to test proxy.\n>>\n>> In this patch I reuse http.sslverify to do this, do we need an\n>> independent option like http.sslproxyverify?\n>>\n>> To fully support https proxy, we also need ways to set more options\n>> such as CURLOPT_PROXY_SSLCERT. However, I'm not sure if we need to\n>> support them.\n>\n> It would be good to add a link to\n> https://daniel.haxx.se/blog/2016/11/26/https-proxy-with-curl/ to the\n> commit message, since it explains in great detail what this is for and\n> how it compares to what we were doing before.\n\nYeah, I found it a quite readable explanation.\n\nIt appears to me that the configuration for the proxy would have to\nbe different from the configuration for the hosts in the outside\nworld.  You may want to be a lot more strongly suspicious to the\nlatter while trusting the former a bit more.  I also suspect that\nwe'd need options like PROXY_SSLCERT that are parallel to the main\nSSL support in the final form, but it may be OK to leave it to a\nfollow-up series.\n\nAnd in order to start small and grow later, I think it is not a good\nidea to repurpose http.sslverify to imply http.sslproxyverify; you\nare declaring \"when you disable peer verification, you also disable\nproxy verification\", and need to worry about backward compatibility\nwhen you later introduce http.sslproxyverify that can control these\nindependently.  It probably is less bad than usual b/c issues in\nthat it probably is much more common to disable verification for\nproxy than target hosts, though.\n\nIn any case, thanks for working on this.\n\n>\n>> Signed-off-by: Wei Shuyu <wsy@dogben.com>\n>> ---\n>>  http.c | 9 +++++++++\n>>  1 file changed, 9 insertions(+)\n>>\n>> diff --git a/http.c b/http.c\n>> index 215bebef1..d8a5e48f0 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -708,6 +708,10 @@ static CURL *get_curl_handle(void)\n>>  \tif (!curl_ssl_verify) {\n>>  \t\tcurl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 0);\n>>  \t\tcurl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 0);\n>> +#if LIBCURL_VERSION_NUM >= 0x073400\n>> +\t\tcurl_easy_setopt(result, CURLOPT_PROXY_SSL_VERIFYPEER, 0);\n>> +\t\tcurl_easy_setopt(result, CURLOPT_PROXY_SSL_VERIFYHOST, 0);\n>> +#endif\n>>  \t} else {\n>>  \t\t/* Verify authenticity of the peer's certificate */\n>>  \t\tcurl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 1);\n>> @@ -865,6 +869,11 @@ static CURL *get_curl_handle(void)\n>>  \t\telse if (starts_with(curl_http_proxy, \"socks\"))\n>>  \t\t\tcurl_easy_setopt(result,\n>>  \t\t\t\tCURLOPT_PROXYTYPE, CURLPROXY_SOCKS4);\n>> +#endif\n>> +#if LIBCURL_VERSION_NUM >= 0x073400\n>> +\t\telse if (starts_with(curl_http_proxy, \"https\"))\n>> +\t\t\tcurl_easy_setopt(result,\n>> +\t\t\t\tCURLOPT_PROXYTYPE, CURLPROXY_HTTPS);\n>>  #endif\n>>  \t\tif (strstr(curl_http_proxy, \"://\"))\n>>  \t\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n"}]}