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

Move "no SNI if IP address" responsibilty to TLS backends #2177

Closed
wants to merge 3 commits into from

Conversation

pquentin
Copy link
Member

  • The ssl module already avoids SNI when the host is an IP address
  • pyOpenSSL now does that too
  • We were already using SNI on IP addresses with SecureTransport anyway

In other words, this does not change anything, but is cleaner
(we no longer test for SecureTransport in ssl.py) and will allow us to
lean on ssl.SSLContext to match hostnames.

It won't make a difference in practice, and avoids useless differences.
@codecov
Copy link

codecov bot commented Mar 12, 2021

Codecov Report

Merging #2177 (2377b87) into main (11fecfd) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main     #2177   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         2250      2246    -4     
=========================================
- Hits          2250      2246    -4     
Impacted Files Coverage Δ
src/urllib3/response.py 100.00% <100.00%> (ø)
src/urllib3/util/retry.py 100.00% <100.00%> (ø)
src/urllib3/util/ssl_.py 100.00% <100.00%> (ø)
src/urllib3/util/url.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 11fecfd...4fbec8d. Read the comment docs.

 * The ssl module already avoids SNI when the host is an IP address
 * pyOpenSSL now does that too
 * We were already using SNI on IP addresses with SecureTransport anyway

In other words, this does not change anything, but is cleaner (we no
longer test for SecureTransport in ssl.py) and will allow us to lean on
ssl.SSLContext to match hostnames.
sethmlarson
sethmlarson previously approved these changes Mar 13, 2021
Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

The change itself looks good, great to simplify things! One thought:

src/urllib3/util/__init__.py Outdated Show resolved Hide resolved
We already have way too much public functions
@pquentin
Copy link
Member Author

The codecov check is red, but the coverage is still 100%: https://app.codecov.io/gh/urllib3/urllib3/compare/2177/commits

Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

Thanks for the change, this looks great now 🎉

@pquentin
Copy link
Member Author

Was merged as part of #2178 by mistake, closing

@pquentin pquentin closed this Mar 16, 2021
@pquentin pquentin deleted the sni-in-ssl-backends branch September 24, 2021 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants