New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support HTTPS proxy and SOCKS+HTTP(s) #1127

Closed
wants to merge 50 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@rousskov
Contributor

rousskov commented Nov 16, 2016

This is another attempt to get the HTTPS proxy support into Curl. Since the previous attempts (see pull requests #268, #305, and #515), the code has been synced with master ea80a2d dated 2016-11-10. All backends should build, but we did not test the compilation of Windows-specific ones. OpenSSL, GnuTLS, and NSS backends support HTTPS proxy features in our tests, although the latter was only tested in --insecure mode.

The first sections below describe the two major parts of the new code (in a format suitable for a commit message) followed by usage examples and a to-do list. You have seen all of this information except the list of supported backends in pull request #305.

1. HTTPS proxies:

An HTTPS proxy receives all transactions over an SSL/TLS connection.
Once a secure connection with the proxy is established, the user agent
uses the proxy as usual, including sending CONNECT requests to instruct
the proxy to establish a [usually secure] TCP tunnel with an origin
server. HTTPS proxies protect nearly all aspects of user-proxy
communications as opposed to HTTP proxies that receive all requests
(including CONNECT requests) in vulnerable clear text.

With HTTPS proxies, it is possible to have two concurrent _nested_
SSL/TLS sessions: the "outer" one between the user agent and the proxy
and the "inner" one between the user agent and the origin server
(through the proxy). This change adds supports for such nested sessions
as well.

A secure connection with a proxy requires its own set of the usual SSL
options (their actual descriptions differ and need polishing, see TODO):

  --proxy-cacert FILE        CA certificate to verify peer against
  --proxy-capath DIR         CA directory to verify peer against
  --proxy-cert CERT[:PASSWD] Client certificate file and password
  --proxy-cert-type TYPE     Certificate file type (DER/PEM/ENG)
  --proxy-ciphers LIST       SSL ciphers to use
  --proxy-crlfile FILE       Get a CRL list in PEM format from the file
  --proxy-insecure           Allow connections to proxies with bad certs
  --proxy-key KEY            Private key file name
  --proxy-key-type TYPE      Private key file type (DER/PEM/ENG)
  --proxy-pass PASS          Pass phrase for the private key
  --proxy-ssl-allow-beast    Allow security flaw to improve interop
  --proxy-sslv2              Use SSLv2
  --proxy-sslv3              Use SSLv3
  --proxy-tlsv1              Use TLSv1
  --proxy-tlsuser USER       TLS username
  --proxy-tlspassword STRING TLS password
  --proxy-tlsauthtype STRING TLS authentication type (default SRP)

All --proxy-foo options are independent from their --foo counterparts,
except --proxy-crlfile which defaults to --crlfile and --proxy-capath
which defaults to --capath.

Curl now also supports %{proxy_ssl_verify_result} --write-out variable,
similar to the existing %{ssl_verify_result} variable.

Supported backends: OpenSSL, GnuTLS, and NSS.

2. SOCKS proxy + HTTP/HTTPS proxy combination:

If both --socks* and --proxy options are given, Curl first connects to
the SOCKS proxy and then connects (through SOCKS) to the HTTP or HTTPS
proxy.

3. Copyright statement:

Copyright (C) 2015 The Measurement Factory, Inc.
Distributed under the terms of the Curl project itself.
Authors: Dmitry Kurochkin, Vasy Okhin, and Alex Rousskov.

4. Usage examples:

proxy='--proxy https://localhost:3129/'
ca='--proxy-cacert ssl_cert/myCA.pem'
curl $ca $proxy http://www.example.com
curl $ca $proxy https://www.example.com
curl $ca $proxy --proxytunnel https://www.example.com

socks='--socks5 127.0.0.1:1080'
curl $ca $proxy --proxytunnel $socks http://www.example.com
curl $ca $proxy --proxytunnel $socks https://www.example.com

curl --insecure --proxytunnel $ca $socks $proxy \
    ftps://user:pass@127.0.0.1/example.txt

curl --insecure --proxytunnel $ca $socks $proxy \
    --ftp-ssl ftp://user:pass@127.0.0.1/example.txt

format='OR=%{ssl_verify_result},PR=%{proxy_ssl_verify_result}\n\n';
curl --proxy-insecure -w $format $proxy https://www.example.com

5. TODO:

  1. Update documentation for the new APIs and --proxy-* options. Look for Added in 7.XXX marks.
  2. Replace TBD comment with the actual Curl version in include/curl/curl.h.
  3. Consider adding --proxy-foo options to mimic --tlsv1.0, --tlsv1.1, and --tlsv1.2 added to Curl v7.42, after the active work on this feature has stopped.

The code passes make check tests in our environment except for the following test cases when building with the NSS backend: 310, 311, 312, 313, 2034, 2035, 2037, 2038, 2041, and 2042. The same test cases fail prior to our changes.

Please note that SSL-inside-SSL does not work with OpenSSL versions ranging from 0.9.7a to 1.0.0-beta2, inclusively, due to an OpenSSL bug.

The code has seen more than a year of light use in a few different environments.

We will do our best to address your change requests, but we do not consider ourselves Curl experts, and are looking forward to finally leaving this code in more capable hands :-). Thank you for Curl!

rousskov and others added some commits Jun 18, 2015

proxy: Support HTTPS proxy and SOCKS+HTTP(s)
HTTPS proxies:

An HTTPS proxy receives all transactions over an SSL/TLS connection. Once a
secure connection with the proxy is established, the user agent uses the proxy
as usual, including sending CONNECT requests to instruct the proxy to establish
a [usually secure] TCP tunnel with an origin server. HTTPS proxies protect
nearly all aspects of user-proxy communications as opposed to HTTP proxies that
receive all requests (including CONNECT requests) in vulnerable clear text.

With HTTPS proxies, it is possible to have two concurrent _nested_ SSL/TLS
sessions: the "outer" one between the user agent and the proxy and the "inner"
one between the user agent and the origin server (through the proxy). This
change adds supports for such nested sessions as well.

The secure connection with the proxy requires its own set of the usual
SSL/TLS-related options (their descriptions need polishing):

  --proxy-cacert FILE        CA certificate to verify peer against
  --proxy-capath DIR         CA directory to verify peer against
  --proxy-cert CERT[:PASSWD] Client certificate file and password
  --proxy-cert-type TYPE     Certificate file type (DER/PEM/ENG)
  --proxy-ciphers LIST       SSL ciphers to use
  --proxy-crlfile FILE       Get a CRL list in PEM format from the given file
  --proxy-insecure           Allow connections to SSL sites without certs
  --proxy-key KEY            Private key file name
  --proxy-key-type TYPE      Private key file type (DER/PEM/ENG)
  --proxy-pass PASS          Pass phrase for the private key
  --proxy-ssl-allow-beast    Allow security flaw to improve interop
  --proxy-sslv2              Use SSLv2
  --proxy-sslv3              Use SSLv3
  --proxy-tlsv1              Use TLSv1
  --proxy-tlsuser USER       TLS username
  --proxy-tlspassword STRING TLS password
  --proxy-tlsauthtype STRING TLS authentication type (default SRP)

All --proxy-foo options are independent from their --foo counterparts, except
--proxy-crlfile defaults to --crlfile and --proxy-capath defaults to --capath.

Curl now also supports %{proxy_ssl_verify_result} --write-out variable,
similar to the existing %{ssl_verify_result} variable.

SOCKS proxy + HTTP/HTTPS proxy combination:

If both --socks* and --proxy options are given, Curl first connects to the
SOCKS proxy and then connects (through SOCKS) to the HTTP or HTTPS proxy.
openssl: fixup the proxy/primary access macros
IS_PROXY_SSL() no longer depend on connect state

IS_PROXY_SSL renamed to SSL_IS_PROXY

SSL_OPTION renamed to SSL_SET_OPTION

SSL_OPTION_PRIM renamed to SSL_CONN_CONFIG
openssl: remove unused arguments
... that popped up when SSL_IS_PROXY() was modified.
multi: Move http2 push function declarations to header end
This change necessary for binary compatibility.

Prior to this change test 1135 failed due to the order of functions.
NSS: Full backend support for HTTPS proxies
Also adjusts SSL_IS_PROXY() to work correctly without requiring
sockindex.
PolarSSL: Fixed build with backend
PolarSSL does not support HTTPS proxies yet
(ssl_connect_init_proxy returns CURLE_NOT_BUILT_IN).
cyassl: Fixed build
CyaSSL does not support HTTPS proxies yet
(ssl_connect_init_proxy returns CURLE_NOT_BUILT_IN).
axtls: Fixed build
axTLS does not support HTTPS proxies yet
(ssl_connect_init_proxy returns CURLE_NOT_BUILT_IN).
curl_gssapi: remove 'const' to fix compiler warnings
initialization discards 'const' qualifier from pointer target type
Patrick Monnerat
nss: fix crash when closing HTTPS conn over HTTPS proxy
Truncated valgrind output capturing the crash follows:

Invalid read of size 8
   at 0x628B4A0: PR_Close (priometh.c:104)
   by 0x43C088: nss_close (nss.c:1356)
   by 0x43C17F: Curl_nss_close (nss.c:1377)
   by 0x438F42: Curl_ssl_close (vtls.c:618)
   by 0x45460B: Curl_disconnect (url.c:3000)
   by 0x42EACA: close_all_connections (multi.c:1887)
   by 0x42EB37: curl_multi_cleanup (multi.c:1907)
   by 0x44B4CA: Curl_close (url.c:427)
   by 0x4282C2: curl_easy_cleanup (easy.c:859)
   by 0x41341C: main_free (tool_main.c:206)
   by 0x4134F4: main (tool_main.c:260)
 Address 0x99afa10 is 0 bytes inside a block of size 48 free'd
   at 0x4C29D6A: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x62A2C33: pt_Close (ptio.c:1277)
   by 0x43BACD: nspr_io_close (nss.c:1141)
   by 0x54D6E25: ssl_DefClose (ssldef.c:203)
   by 0x43BACD: nspr_io_close (nss.c:1141)
   by 0x54D6E25: ssl_DefClose (ssldef.c:203)
   by 0x43C088: nss_close (nss.c:1356)
   by 0x43C173: Curl_nss_close (nss.c:1376)
   by 0x438F42: Curl_ssl_close (vtls.c:618)
   by 0x45460B: Curl_disconnect (url.c:3000)
   by 0x42EACA: close_all_connections (multi.c:1887)
   by 0x42EB37: curl_multi_cleanup (multi.c:1907)
 Block was alloc'd at
   at 0x4C28C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x6289F71: _PR_Getfd (prfdcach.c:103)
   by 0x62A4816: pt_SetMethods.isra.13 (ptio.c:3303)
   by 0x62A56E5: PR_ImportTCPSocket (ptio.c:4577)
   by 0x43D4D0: nss_setup_connect (nss.c:1733)
   by 0x43DDCA: nss_connect_common (nss.c:1929)
   by 0x43DF1E: Curl_nss_connect_nonblocking (nss.c:1985)
   by 0x4386CE: Curl_ssl_connect_nonblocking (vtls.c:364)
   by 0x47F02C: https_proxy_connect (http_proxy.c:58)
   by 0x47F086: Curl_proxy_connect (http_proxy.c:74)
   by 0x445891: Curl_http_connect (http.c:1348)
   by 0x42D613: multi_runsingle (multi.c:1233)
Patrick Monnerat
gskit: Implement SSL over SSL
This is done via a TCP socket pair monitored at each negotiate/send/receive
attempt for explicit pipelining.
See gskit.c comments for more information.
Patrick Monnerat
gskit: protect inetsocketpair() against foreign connection.
Avoid blocking read.
Drain SSL over SSL output upon close.
Fixed handling of --proxy-cert options.
Some of the options were not cloned from the the SSL config
stored in the connection structure.
Merge branch 'master' into https-proxy
It is building which openssl, nss, GnuTLS and polarssl. Need restore run tests.
fixed tests
fixed tests: 81 169 183 184 185 239 243 275 407 540 547 548 555 590 1001
1002 1077 1078 1087 1088 1098 1215 1218 1228 1331 1421

is not fixed test 1139 - need add documentation for:
--proxy-crlfile
--proxy-pass
--proxy-ssl-allow-beast
--proxy-cert
--proxy-tlsuser
--proxy-key-type
--proxy-ciphers
--proxy-sslv3
--proxy-tlsauthtype
--proxy-cacert
--proxy-tlspassword
--proxy-sslv2
--proxy-tlsv1
--proxy-insecure
--proxy-key
--proxy-cert-type
--proxy-capath
@rousskov

This comment has been minimized.

Contributor

rousskov commented Nov 18, 2016

@sithglan, please test whether SNI-to-proxy works for you now. It works in our tests but @OkhinVI was not sure that the recent changes are sufficient.

@rousskov

This comment has been minimized.

Contributor

rousskov commented Nov 18, 2016

@MarcelRaad, we now pretend to use sockindex in CURL_DISABLE_PROXY builds which should address your build warnings. @bagder, I agree that the commented out code in Curl_connected_proxy() should have been simply removed (or refactored) but I do not know why it was not. It is gone now.

@sithglan

This comment has been minimized.

Contributor

sithglan commented Nov 18, 2016

@rousskov, I was able to test SNI-to-proxy and it works, however, I now have two new problems: When I enable certificate pinning (CURLOPT_PINNEDPUBLICKEY) for the inner SSL connection, it also applies for the outer proxy ssl connection. With the version I have build, I still have problems issuing GET statements through two tunneled connections. I get the following error message:

(infra) [~/work/vlconnect] local/linux/bin/curl --proxy-cacert /etc/ssl/certs/ca-certificates.crt --insecure --proxy https://tg:password@proxy.glanzmann.de:443/ https://thomas.glanzmann.de/
curl: (4) A requested feature, protocol or option was not found built-in in this libcurl due to a build-time decision.

If you need access to my proxy, drop me an email. With the same curl version without a proxy I can access my homepage. Curl access to a http site through https proxy works.

@bagder

This comment has been minimized.

Member

bagder commented Nov 18, 2016

  1. I'd like to have the HTTPS-proxy branch rebased on top of the master until we can merge it, to make it easier to review for everyone. You dropping in 46 commits in your branch suddenly makes that downright impossible to handle nicely. Maybe you can squash some of that and send as pull requests against that branch instead?
  2. The next version is going to be called 7.52.0, as mentioned in the current RELEASE-NOTES and curlver.h

And a detail on the process: I think we need to focus on the existing functionality pre-merge so that we lessen the problems this causes to existing users when we merge. Flaws and issues in the new functionality can be handled after merge too, as those problems won't hurt existing users. Having this focus help us land this sooner rather than later and we will all benefit from that.

@bagder

This comment has been minimized.

Member

bagder commented Nov 18, 2016

Alternatively, you rebase your branch and squash some commits.

@nickzman

This comment has been minimized.

Collaborator

nickzman commented on lib/vtls/darwinssl.c in dba3ef5 Nov 19, 2016

Why was the failf() removed here? If we have to resort to SSLSetProtocolVersionEnabled() to set the TLS version, then the user is using a version of macOS that is too old to support TLS 1.1 or later.

This comment has been minimized.

Member

bagder replied Nov 19, 2016

I don't think it was removed, it was just the diff there that got mighty confused. I didn't intend to remove anything, I just modified some symbols to make it build and I re-indented a bunch of lines.

The failf() for the CURL_SSLVERSION_TLSv1_1 case above is still present like this: https://github.com/curl/curl/blob/HTTPS-proxy/lib/vtls/darwinssl.c#L1204

@rousskov

This comment has been minimized.

Contributor

rousskov commented Nov 19, 2016

@bagder, we can do whatever you want with the https-proxy-take3 branch, including deleting/abandoning it if it helps. If I did not use the right method to bring the https-proxy-take3 branch in sync with your HTTPS-proxy branch changes and force-pushes, I apologize. Again, whatever you want us to do with our branch, we will do. You just need to tell us exactly what you want (and wait).

Needless to say, if the last merge was a mistake, we can create a new branch (https-proxy-take4) and a new pull request that does not have that last merge. Naturally, that code will not have the latest SNI fixes and is likely to suffer the same fate in a few days as both teams keep modifying their respective branches!

I showed your comments to somebody who knows more about git/github than I do, but they could not figure out what we should do to make and keep you happy. Rebase against which branch? Squash which commits? Etc. Please detail your suggestion(s), and we will do our best to follow them.

@bagder

This comment has been minimized.

Member

bagder commented Nov 19, 2016

OK, sorry for being vague. Let me explain how I see it and would like us to get this landed:

In order to keep the HTTS-proxy work with the latest curl master branch, I want to keep it rebased on top of master. I pulled your branch the other day into HTTPS-proxy, and I pulled @sithglan's mbedtls fix and I fixed darwinssl myself into there.

So, if we just keep fixing things in that branch and allow it to rebase occasionally (when we merge things into master) we keep the HTTPS-proxy work as simple as possible to review and understand as an additional change onto master. Of course it doesn't have to be that particular branch but we can also opt to create another one that works in a similar way. A major point being that I want the new branch be rebased on master.

My hope/plan/intention is that we manage to fix problems in the branch right now and perhaps within a week we can have it in a state where we believe it doesn't break existing functionality (much) and we merge it - and then we continue cleaning it up, improving it and going forward in the master branch like we normally work in the project. And we typically don't do "merge commits" but we rebase and fast-forward merge. I think rebasing the branch on top of master is doable for a short period of time and I hope that's all we're going to need.

@rousskov

This comment has been minimized.

Contributor

rousskov commented Nov 19, 2016

@bagder, FWIW, I believe your plan to use HTTPS-proxy branch as the focus branch for this work is the best way forward. AFAICT, this implies that we abandon our https-proxy-take3 branch and submit any future pull requests against your HTTPS-proxy branch (as we have done in the past). Please correct me if I am wrong.

What do you need us to fix in your HTTPS-proxy? SNI-to-proxy?

@bagder

This comment has been minimized.

Member

bagder commented Nov 20, 2016

Yes please submit PRs against that branch and I'll merge asap.

What do you need us to fix in your HTTPS-proxy? SNI-to-proxy?

The HTTPS-proxy only has 4 commits on top of master right now:

$ git log --oneline master..
dba3ef5 darwinssl: adopted to the HTTPS proxy changes
435d789 gtls: fix indent to silence compiler warning
4d67222 mbedtls: Fix compile errors
e3e1eef proxy: Support HTTPS proxy and SOCKS+HTTP(s)

So everything beyond that. Yes, that would include SNI-to-proxy and whatever else you fixed since the first commit you submitted in this pull request.

@bagder

This comment has been minimized.

Member

bagder commented Nov 21, 2016

OK, I rebased it again but I'll hold off further rebases until you've got a chance to do pull requests on it. I might create a new branch to keep rebased instead.

@bagder

This comment has been minimized.

Member

bagder commented Nov 21, 2016

I successfully built with the schannel/winssl TLS in this branch on Windows just now. 👍

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Nov 21, 2016

I tested proxy connections with schannel. I had problems with Digest authentication, but they seem to be unrelated (I get the same behavior in 7.51.0). I haven't found any other problems.

@bagder

This comment has been minimized.

Member

bagder commented Nov 23, 2016

@rousskov and friends, we need this in shape before Monday to get merged for the pending release, otherwise we miss the window and we'll have to wait until the next release and that will give more rebase work and handling of merge conflicts. So I'd appreciate getting your additional fixes sooner rather than later.

@bagder

This comment has been minimized.

Member

bagder commented Nov 23, 2016

Since the HTTPS-proxy feature is only provided by a subset of TLS backends now and that is a situation that is likely to remain for a long time into the future (possibly forever), I propose that we create a new feature-bit for applications to check if the provided libcurl supports this feature.

CURL_VERSION_HTTPS_PROXY perhaps?

@bagder

This comment has been minimized.

Member

bagder commented Nov 23, 2016

I merged a new patch from the HTTPS-proxy team and rebased the branch again.

@rousskov

This comment has been minimized.

Contributor

rousskov commented Nov 23, 2016

@bagder

This comment has been minimized.

Member

bagder commented Nov 23, 2016

@rousskov others have pitched in already as you should be aware, both as the patches in the branch show and as the discussions on the mailing list goes. I hope you're on the curl-library list as well to help us sort of some of the remaining issues and planning of this feature set.

@OkhinVI

This comment has been minimized.

Contributor

OkhinVI commented Nov 24, 2016

I added CURL_VERSION_HTTPS_PROXY bit in the patch:
0001-add-bit-CURL_VERSION_HTTPS_PROXY.txt
But I do not know how the tests will be runing for the GSKit.

@bagder

This comment has been minimized.

Member

bagder commented Nov 24, 2016

I added CURL_VERSION_HTTPS_PROXY bit in the patch

I think we should let the vtls/*.h headers define if they support HTTPS proxy or not so that it works fine with all build systems and as it makes the decision up to each backend rather than configure.

@bagder

This comment has been minimized.

Member

bagder commented Nov 24, 2016

This branch is now merged into master as of 74ffa04. Thanks for all your work so far. Now let's fix the outstanding issues and TODOs!

@sithglan

This comment has been minimized.

Contributor

sithglan commented Nov 25, 2016

Hello,
I posted two patches to the curl mailing list in order to address some the CERTIFICATE PINNING and https proxy default port issue. Please review and apply.

Cheers,
Thomas

@sithglan

This comment has been minimized.

Contributor

sithglan commented Nov 25, 2016

@rousskov could you please elaborate why mbedtls does not support SSL-inside-SSL? Is that a limitation of mbedtls or is it simply work to be done? I'm asking because for me it appears that it would be technically possible with mbedtls:

https://tls.mbed.org/discussions/generic/sharing-ssl-data-between-threads-and-multiple-connections

@OkhinVI

This comment has been minimized.

Contributor

OkhinVI commented Nov 25, 2016

I think we should let the vtls/*.h headers define if they support HTTPS proxy or not so that it works fine with all build systems and as it makes the decision up to each backend rather than configure.

I corrected the patch and make a pull request.

@rousskov

This comment has been minimized.

Contributor

rousskov commented Nov 25, 2016

@bagder

This comment has been minimized.

Member

bagder commented Nov 26, 2016

This branch and this pull request have been merged to master. Closing this now, please file issues and further pull requests like usual.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment