Skip to content
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

Integrate TLS-in-TLS support into urllib3 #1923

Merged
merged 13 commits into from Sep 28, 2020

Conversation

@jalopezsilva
Copy link
Contributor

@jalopezsilva jalopezsilva commented Aug 18, 2020

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

@jalopezsilva jalopezsilva changed the title Tls in tls integration Integrating TLS-in-TLS support into urllib3 Aug 18, 2020
test/__init__.py Outdated Show resolved Hide resolved
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 jalopezsilva force-pushed the tls_in_tls_integration branch from 542b8ff to 4ff1d93 Aug 19, 2020
@jalopezsilva
Copy link
Contributor Author

@jalopezsilva jalopezsilva commented Aug 19, 2020

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.

Copy link
Member

@sethmlarson sethmlarson left a comment

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 Outdated Show resolved Hide resolved
src/urllib3/poolmanager.py Outdated Show resolved Hide resolved
src/urllib3/poolmanager.py Outdated Show resolved Hide resolved
src/urllib3/util/proxy.py Outdated Show resolved Hide resolved
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)
Copy link
Member

@sethmlarson sethmlarson Aug 19, 2020

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

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

@sethmlarson sethmlarson Aug 19, 2020

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.

Copy link
Contributor Author

@jalopezsilva jalopezsilva Aug 30, 2020

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?

Copy link
Member

@sethmlarson sethmlarson Sep 12, 2020

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 Outdated Show resolved Hide resolved
@sethmlarson sethmlarson added this to the v1.26 milestone Aug 27, 2020
@sethmlarson
Copy link
Member

@sethmlarson sethmlarson commented Sep 7, 2020

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

jalopezsilva added 2 commits Sep 8, 2020
- 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 jalopezsilva requested a review from sethmlarson Sep 8, 2020
@jalopezsilva
Copy link
Contributor Author

@jalopezsilva jalopezsilva commented Sep 8, 2020

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.
Copy link
Member

@sethmlarson sethmlarson left a comment

Some more comments! :)

src/urllib3/poolmanager.py Outdated Show resolved Hide resolved
src/urllib3/util/proxy.py Outdated Show resolved Hide resolved
Attempts to retrieve a client certificate and key from the environment
variables to use with the proxy.
"""
proxy_cert = os.environ.get("PROXY_SSLCERT")
Copy link
Member

@sethmlarson sethmlarson Sep 12, 2020

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 Outdated Show resolved Hide resolved
src/urllib3/util/ssl_.py Outdated Show resolved Hide resolved
src/urllib3/util/ssl_.py Outdated Show resolved Hide resolved
src/urllib3/util/ssltransport.py Outdated Show resolved Hide resolved
src/urllib3/util/ssltransport.py Outdated Show resolved Hide resolved
src/urllib3/connection.py Outdated Show resolved Hide resolved
test/with_dummyserver/test_proxy_poolmanager.py Outdated Show resolved Hide resolved
@codecov
Copy link

@codecov codecov bot commented Sep 12, 2020

Codecov Report

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

Impacted file tree graph

@@            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 jalopezsilva requested a review from sethmlarson Sep 13, 2020
@jalopezsilva
Copy link
Contributor Author

@jalopezsilva jalopezsilva commented Sep 13, 2020

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
Copy link
Member

@sethmlarson sethmlarson commented Sep 13, 2020

@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 Outdated Show resolved Hide resolved
src/urllib3/util/ssltransport.py Outdated Show resolved Hide resolved
src/urllib3/connectionpool.py Outdated Show resolved Hide resolved
- 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 jalopezsilva requested a review from sethmlarson Sep 13, 2020
@jalopezsilva
Copy link
Contributor Author

@jalopezsilva jalopezsilva commented Sep 13, 2020

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
Copy link
Member

@pquentin pquentin commented Sep 14, 2020

@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
Copy link
Contributor Author

@jalopezsilva jalopezsilva commented Sep 17, 2020

@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
Copy link
Member

@sethmlarson sethmlarson commented Sep 17, 2020

@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 jalopezsilva deleted the tls_in_tls_integration branch May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants