Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Integrate TLS-in-TLS support into urllib3#1923

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
pquentin merged 13 commits intourllib3:masterfromjalopezsilva:tls_in_tls_integration
Sep 28, 2020

Conversation

jalopezsilva
Copy link
Contributor

@jalopezsilvajalopezsilva commentedAug 18, 2020•
edited by sethmlarson
Loading

Integration PR after#1806 is closed.

The PR finally integrates theSSLTransport 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 ProxyManagersproxy_ssl_context which can be used to pass anSSLContext to be used with the proxy anduse_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

@jalopezsilvajalopezsilva changed the titleTls in tls integrationIntegrating TLS-in-TLS support into urllib3Aug 18, 2020
For connections that will attempt to use an HTTPS proxy with an HTTPSdestination, we'll use the TLS in TLS support provided by SSL Transport.HTTPS proxy and HTTP destinations will continue using a single TLSsession as expected.We'll still support the use of forwarding for HTTPS destinations withHTTPS proxies as long as the "use_forwarding_for_https" parameter isprovided.Signed-off-by: Jorge Lopez Silva <jalopezsilva@gmail.com>
@jalopezsilva
Copy link
ContributorAuthor

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

@sethmlarsonsethmlarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ifproxy_pass isn't given we shouldn't call with thepassword parameter. The parameter isn't supported on some Python 2.7 if I recall

jalopezsilva reacted with thumbs up emoji
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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 byURLLIB3_* address that concern?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

jalopezsilva reacted with thumbs up emoji
@sethmlarsonsethmlarson added this to thev1.26 milestoneAug 27, 2020
@sethmlarson
Copy link
Member

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

I think thatSSLTransport makes sense to move tourllib3.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 reacted with thumbs up emoji

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

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.

Copy link
Member

@sethmlarsonsethmlarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Some more comments! :)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

jalopezsilva reacted with thumbs up emoji
@codecov
Copy link

codecovbot commentedSep 12, 2020•
edited
Loading

Codecov Report

Merging#1923 intomaster willnot change coverage.
The diff coverage is100.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 FilesCoverage Δ
src/urllib3/connection.py100.00% <100.00%> (ø)
src/urllib3/connectionpool.py100.00% <100.00%> (ø)
src/urllib3/poolmanager.py100.00% <100.00%> (ø)
src/urllib3/util/proxy.py100.00% <100.00%> (ø)
src/urllib3/util/ssl_.py100.00% <100.00%> (ø)
src/urllib3/util/ssltransport.py100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updated560e21...cabdf90. Read thecomment 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
Copy link
ContributorAuthor

jalopezsilva commentedSep 13, 2020•
edited
Loading

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 currenturllib3/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 onSSLTransport.

@sethmlarson
Copy link
Member

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

jalopezsilva reacted with rocket emoji

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

All right, another commit for the remaining comments.

@sethmlarson, happy to add another test case forAppEngineManager but I'm confused on how it would work. Isn'tAppEngineManager a replacement for a PoolManager? Can you even specify proxies through it? Let me know if I'm missing something obvious, I went throughhttps://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

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

@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 reacted with heart emoji

@sethmlarson
Copy link
Member

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

jalopezsilva reacted with thumbs up emoji

- We no longer need to pass the upstream destination.
This was referencedMar 16, 2021
@jalopezsilvajalopezsilva deleted the tls_in_tls_integration branchMay 13, 2021 02:37
Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull requestMar 16, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@pquentinpquentinpquentin approved these changes

@sethmlarsonsethmlarsonsethmlarson approved these changes

@hodbnhodbnAwaiting requested review from hodbn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.

urllib3 does not allow to use HTTPS proxy
4 participants
@jalopezsilva@sethmlarson@pquentin@mirkokg
[8]ページ先頭
©2009-2025 Movatter.jp