-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Lean on SSLContext to verify hostnames when possible #2178
Conversation
It won't make a difference in practice, and avoids useless differences.
* 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.
Codecov Report
@@ Coverage Diff @@
## main #2178 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 25 25
Lines 2250 2250
=========================================
Hits 2250 2250
Continue to review full report at Codecov.
|
Ubuntu build timed out, retrying |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good, a few comments for you:
src/urllib3/util/ssl_.py
Outdated
# We ask for verification here but it may be disabled in HTTPSConnection.connect | ||
context.check_hostname = cert_reqs == ssl.CERT_REQUIRED | ||
if hasattr(context, "hostname_checks_common_name"): | ||
context.hostname_checks_common_name = False # Python 3.7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the # Python ...
comment be on the if
line to cover the whole branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the intent here is to remind us to remove this when we drop support for Python 3.6 while still making sure it's covered.
Now that I think about it more, the best thing to do is to rely on codecov telling us it's no longer covered when we drop Python 3.6.
@@ -362,6 +362,18 @@ def connect(self): | |||
ssl_version=resolve_ssl_version(self.ssl_version), | |||
cert_reqs=resolve_cert_reqs(self.cert_reqs), | |||
) | |||
# In some cases, we want to verify hostnames ourselves | |||
if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love how this codifies all the situations where we disable check_hostname
so nicely 🤩
We already have way too much public functions
ba4d13f
to
edb7841
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all review comments are covered here, merge when ready! 🎉
Thanks! Merged |
Closes #517
There are four situations where we can't lean on SSLContext to verify certs for us:
ssl
So, given the preparatory work done in #2176 and #2177, this PR only adds one commit where we just ask the SSL backend to verify the hostname (SecureTransport was already doing it anyway.)
I made sure that invalid hostnames are still rejected with all three SSL backends.