Integrate TLS-in-TLS support into urllib3 #1923

jalopezsilva · 2020-08-18T00:35:24Z

Integration PR after #1806 is closed.

The PR finally integrates the SSLTransport class to support TLS-in-TLS tunnels through proxies. This will enable urllib3 to use HTTPS proxies with HTTPS destinations. The previous supported mode, forwarding the absolute URI method, is kept under a flag for HTTPS destinations through HTTPS proxies. I added two new parameters for ProxyManagers proxy_ssl_context which can be used to pass an SSLContext to be used with the proxy and use_forwarding_for_https which enables the forwarding absolute URI mode.

I decided to keep the forwarding mode as it has been incredibly useful within corporate environments when used with trusted proxies. (e.g we can audit no undesired information is sent through requests). This mode is by default disabled and I'm hoping that with enough documentation users won't use it unnecessarily.

I also added three environment variables which can be used to configure the default proxy ssl context. PROXY_SSLCERT, PROXY_SSLKEY, PROXY_SSLPASSWD. These environment variables will be super useful to configure the proxy ssl context when urllib3 is used within other libraries (e.g botocore, requests, etc)

Tests have been added. I have not added documentation yet, but I can do so easily.

Closes #1662

test/__init__.py

For connections that will attempt to use an HTTPS proxy with an HTTPS destination, we'll use the TLS in TLS support provided by SSL Transport. HTTPS proxy and HTTP destinations will continue using a single TLS session as expected. We'll still support the use of forwarding for HTTPS destinations with HTTPS proxies as long as the "use_forwarding_for_https" parameter is provided. Signed-off-by: Jorge Lopez Silva <jalopezsilva@gmail.com>

jalopezsilva · 2020-08-19T18:34:53Z

All right rebased after the closure of #1806, this should be good to review! I'll add subsequent commits when addressing feedback to make it easier.

sethmlarson

Great start to this, very exciting :) Left some comments and questions for you.

cc @nateprewitt for when Requests will inevitably be asked about HTTPS proxy support

src/urllib3/poolmanager.py

src/urllib3/util/proxy.py

sethmlarson · 2020-08-19T20:20:29Z

src/urllib3/util/proxy.py

+    proxy_cert, proxy_key, proxy_pass = client_certificate_and_key_from_env()
+
+    if proxy_cert:
+        ssl_context.load_cert_chain(proxy_cert, keyfile=proxy_key, password=proxy_pass)


If proxy_pass isn't given we shouldn't call with the password parameter. The parameter isn't supported on some Python 2.7 if I recall

sethmlarson · 2020-08-19T20:22:41Z

src/urllib3/util/proxy.py

+    Attempts to retrieve a client certificate and key from the environment
+    variables to use with the proxy.
+    """
+    proxy_cert = os.environ.get("PROXY_SSLCERT")


So I'm kinda in -1 territory when it comes to environment variables. Ideally our users would make these sorts of things configurable with their own interfaces. We don't support setting connection TLS config w/ environment variables either.

I agree with you in principle. Ideally all libraries should provide interfaces that allow clients to pass through a proxy ssl context and any necessary parameters to urllib3. I already have prepared upstream patches to do this for botocore and requests. I'll send those out once we release the changes in urllib3.

That said though, there's a ton of libraries out there that would need clients to prepare PRs or changes to allow these parameters to go through. It seems easier to support those through environment variables for the time being.

Are you concerned about these being accidentally set? Could prefixing them by URLLIB3_* address that concern?

Knowing when to use environment variables versus parameters in the program versus OS defaults is a big can of worms as we know from Requests, so I want to mark it out of scope for this PR and release. I can definitely see the value, I don't want to release it as a part of 1.26 though.

I appreciate that you have PRs for boto and requests though, that will be very useful! :)

test/test_ssl.py

sethmlarson · 2020-09-07T14:40:58Z

Another thing I thought of while working through the docs overhaul:

I think that SSLTransport makes sense to move to urllib3.util since it's not a "third-party" module.
We'll also have to add documentation of these features but that can be its own PR :)

- Make the toggle for absolute URI forwarding public and adjust documentation. - Adjust documentation in util/proxy.py for connection_requires_http_tunnel. - Only set the proxy_pass if the pass was provided. - Adjust unsupported ssl.SSLContext check. - Remove 'server_hostname' parameter on platforms without SNI support. - Better handling and passing of destination scheme between connectionpool and connection.

jalopezsilva · 2020-09-08T16:38:59Z

PR has been updated with two additional commits.

Commit #2 addresses the feedback received above. All of the items were addressed, let me know if I missed anything.

Commit #3 moved SSLTransport from contrib to util as requested by @sethmlarson.

It seems there are some merge failures, I'll rebase once you've had a chance to review the two additional commits.

- Mistake in previous commit.

sethmlarson

Some more comments! :)

src/urllib3/poolmanager.py

src/urllib3/util/proxy.py

sethmlarson · 2020-09-12T20:19:48Z

src/urllib3/util/proxy.py

+    Attempts to retrieve a client certificate and key from the environment
+    variables to use with the proxy.
+    """
+    proxy_cert = os.environ.get("PROXY_SSLCERT")


Knowing when to use environment variables versus parameters in the program versus OS defaults is a big can of worms as we know from Requests, so I want to mark it out of scope for this PR and release. I can definitely see the value, I don't want to release it as a part of 1.26 though.

I appreciate that you have PRs for boto and requests though, that will be very useful! :)

src/urllib3/util/ssl_.py

src/urllib3/util/ssltransport.py

src/urllib3/connection.py

test/with_dummyserver/test_proxy_poolmanager.py

codecov · 2020-09-12T23:54:54Z

Codecov Report

Merging #1923 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1923   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        25    +1     
  Lines         2187      2243   +56     
=========================================
+ Hits          2187      2243   +56

Impacted Files	Coverage Δ
src/urllib3/connection.py	`100.00% <100.00%> (ø)`
src/urllib3/connectionpool.py	`100.00% <100.00%> (ø)`
src/urllib3/poolmanager.py	`100.00% <100.00%> (ø)`
src/urllib3/util/proxy.py	`100.00% <100.00%> (ø)`
src/urllib3/util/ssl_.py	`100.00% <100.00%> (ø)`
src/urllib3/util/ssltransport.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d560e21...cabdf90. Read the comment docs.

- Improve documentation syntax. - Use Python 2 instead of py2. - Remove support for configuring through environment variables. - Nit and suggestions on util.ssl_.py - Improvements on validation and error messages for not supported SSLContext.

jalopezsilva · 2020-09-13T01:42:25Z

Great work on improving the CI! It's working much more reliably now and it's also faster? (or is it just my impression?) In any case, I've updated the PR with two additional commits: One performs a merge with current urllib3/master and the second addresses the last set of comments by @sethmlarson.

Let me know if we need more changes. I'll follow up on another PR for adding the missing tests on SSLTransport.

sethmlarson · 2020-09-13T02:05:47Z

@jalopezsilva That's all thanks to @pquentin and @hodbn! There's been so much work put into our CI and it's really paying off 👏

src/urllib3/util/ssl_.py

src/urllib3/util/ssltransport.py

src/urllib3/connectionpool.py

- Move to staticmethod instead of classmethod, also make it private. - Error out if SSLTransport isn't available and we need to do TLS-in-TLS. - Avoid modifying the signature of prepare_proxy.

jalopezsilva · 2020-09-13T21:17:05Z

All right, another commit for the remaining comments.

@sethmlarson, happy to add another test case for AppEngineManager but I'm confused on how it would work. Isn't AppEngineManager a replacement for a PoolManager? Can you even specify proxies through it? Let me know if I'm missing something obvious, I went through https://github.com/urllib3/urllib3/blob/master/src/urllib3/contrib/appengine.py as I assumed that's what you were referring to.

pquentin · 2020-09-14T06:31:08Z

@jalopezsilva On an unrelated note, should we credit your employer for sponsoring your time on this amazing feature? Or was it done on your time?

jalopezsilva · 2020-09-17T03:43:04Z

@pquentin It's been a little bit of both. FB has been pretty supportive of me upstreaming the changes but I have also put personal time to make sure it goes through! No need to do any special announcements/credits, FB and I will just be happy to know the changes were merged in 😃

sethmlarson · 2020-09-17T04:03:56Z

@sethmlarson Disregard the AppEngine comment, for some reason I thought it implemented the HTTPConnection interface. Doh.

- We no longer need to pass the upstream destination.

jalopezsilva changed the title ~~Tls in tls integration~~ Integrating TLS-in-TLS support into urllib3 Aug 18, 2020

jalopezsilva mentioned this pull request Aug 18, 2020

Add support for TLS in TLS for HTTPS proxies. #1806

Merged

jalopezsilva commented Aug 18, 2020

View reviewed changes

test/__init__.py Outdated Show resolved Hide resolved

jalopezsilva force-pushed the tls_in_tls_integration branch from 542b8ff to 4ff1d93 Compare August 19, 2020 16:43

Fix typo and appengine test failures.

8eac255

sethmlarson requested changes Aug 19, 2020

View reviewed changes

sethmlarson added this to the v1.26 milestone Aug 27, 2020

jalopezsilva added 2 commits September 8, 2020 09:26

Move SSLTransport from contrib to utils.

057cea2

jalopezsilva requested a review from sethmlarson September 8, 2020 16:37

Remove unnecessary parameter from urllopen.

e78ee2f

- Mistake in previous commit.

sethmlarson requested changes Sep 12, 2020

View reviewed changes

Merge branch 'master' into tls_in_tls_integration

05c94b9

Address review comments - Round urllib3#2

09e9e47

- Improve documentation syntax. - Use Python 2 instead of py2. - Remove support for configuring through environment variables. - Nit and suggestions on util.ssl_.py - Improvements on validation and error messages for not supported SSLContext.

jalopezsilva requested a review from sethmlarson September 13, 2020 01:40

sethmlarson requested changes Sep 13, 2020

View reviewed changes

src/urllib3/util/ssl_.py Outdated Show resolved Hide resolved

src/urllib3/util/ssltransport.py Outdated Show resolved Hide resolved

src/urllib3/connectionpool.py Outdated Show resolved Hide resolved

Address review comments - Round urllib3#3.

fd2605d

- Move to staticmethod instead of classmethod, also make it private. - Error out if SSLTransport isn't available and we need to do TLS-in-TLS. - Avoid modifying the signature of prepare_proxy.

jalopezsilva requested a review from sethmlarson September 13, 2020 21:13

Remove out of date comment.

7585d8d

- We no longer need to pass the upstream destination.

jalopezsilva deleted the tls_in_tls_integration branch May 13, 2021 02:37

sync-by-unito bot mentioned this pull request Jun 2, 2021

Bump urllib3 from 1.25.9 to 1.26.5 in /rest_api covidcanidoit/covidcanidoit#329

Open

sethmlarson mentioned this pull request Jun 19, 2021

Accessing HTTPS sites through proxy #1520

Closed

Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022

Integrate TLS-in-TLS support into urllib3 (urllib3#1923)

79ec81d

normj mentioned this pull request Feb 21, 2023

HTTPS Proxy support feature requests dotnet/runtime#31113

Closed

tomchristie mentioned this pull request Jun 13, 2023

Add support for HTTPS Proxy encode/httpcore#714

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate TLS-in-TLS support into urllib3 #1923

Integrate TLS-in-TLS support into urllib3 #1923

jalopezsilva commented Aug 18, 2020 •

edited by sethmlarson

jalopezsilva commented Aug 19, 2020

sethmlarson left a comment

sethmlarson Aug 19, 2020

sethmlarson Aug 19, 2020

jalopezsilva Aug 30, 2020

sethmlarson Sep 12, 2020

sethmlarson commented Sep 7, 2020

jalopezsilva commented Sep 8, 2020

sethmlarson left a comment

sethmlarson Sep 12, 2020

codecov bot commented Sep 12, 2020 •

edited

jalopezsilva commented Sep 13, 2020 •

edited

sethmlarson commented Sep 13, 2020

jalopezsilva commented Sep 13, 2020

pquentin commented Sep 14, 2020

jalopezsilva commented Sep 17, 2020

sethmlarson commented Sep 17, 2020

Integrate TLS-in-TLS support into urllib3 #1923

Integrate TLS-in-TLS support into urllib3 #1923

Conversation

jalopezsilva commented Aug 18, 2020 • edited by sethmlarson

jalopezsilva commented Aug 19, 2020

sethmlarson left a comment

Choose a reason for hiding this comment

sethmlarson Aug 19, 2020

Choose a reason for hiding this comment

sethmlarson Aug 19, 2020

Choose a reason for hiding this comment

jalopezsilva Aug 30, 2020

Choose a reason for hiding this comment

sethmlarson Sep 12, 2020

Choose a reason for hiding this comment

sethmlarson commented Sep 7, 2020

jalopezsilva commented Sep 8, 2020

sethmlarson left a comment

Choose a reason for hiding this comment

sethmlarson Sep 12, 2020

Choose a reason for hiding this comment

codecov bot commented Sep 12, 2020 • edited

Codecov Report

jalopezsilva commented Sep 13, 2020 • edited

sethmlarson commented Sep 13, 2020

jalopezsilva commented Sep 13, 2020

pquentin commented Sep 14, 2020

jalopezsilva commented Sep 17, 2020

sethmlarson commented Sep 17, 2020

jalopezsilva commented Aug 18, 2020 •

edited by sethmlarson

codecov bot commented Sep 12, 2020 •

edited

jalopezsilva commented Sep 13, 2020 •

edited