-
-
Notifications
You must be signed in to change notification settings - Fork 29.3k
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
bpo-31399: Let OpenSSL verify hostname and IP address #3462
Conversation
a3fa2cf
to
75d33a3
Compare
75d33a3
to
35176ba
Compare
35176ba
to
65faab4
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.
Mostly looks good, from a first skim
Modules/_ssl.c
Outdated
@@ -310,6 +310,10 @@ typedef struct { | |||
PyObject *set_hostname; | |||
#endif | |||
int check_hostname; | |||
/* OpenSSL does not provide X509_VERIFY_PARAM_get_hostflags(), hostflags | |||
* are initialized as 0. |
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 don't understand this comment
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.
OpenSSL has no API to get hostflags from X509_VERIFY_PARAM. We have to maintain our own copy. I'll clarify the comment in my next push.
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.
Ahhh. Now I understand. I think "OpenSSL does not have an API for getting the hostflags from an X509_VERIFY_PARAM" would be clearer.
Also is there a bug/PR on OpenSSL for this we can link?
Modules/_ssl.c
Outdated
#endif | ||
self->hostflags = X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS; |
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.
Do we need X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS
as well?
65faab4
to
c9f4c6f
Compare
Thanks @alex I have updated the comment for |
e499eca
to
89a28a5
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.
asyncio test change is correct (but please update assertion as suggested)
Lib/test/test_asyncio/test_events.py
Outdated
self.loop.run_until_complete(f_c) | ||
|
||
# close connection | ||
proto.transport.close() | ||
# transport is None because TLS ALERT aborted the handshake | ||
self.assertEqual(proto.transport, None) |
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.
Please use self.assertIsNone(proto.transport)
c400837
to
d1dd4f3
Compare
@asvetlov Thanks, I changed the assert as requested I also forgot to remove the call to match_hostname. How should I handle the call in asyncio? If I remove it, the code will no worker be secure on 3.6. |
This change is for 3.7 only, right? If so just remove the call in the |
d1dd4f3
to
d07134d
Compare
Yes, it's 3.7 only. |
d07134d
to
7b5edd5
Compare
This branch is now based on PR #5242. Please review and ACK the other PR first. |
7c4fc70
to
197d51f
Compare
Are there stoppers for the issue left? |
Good to know. |
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.
Code looks fine, I have some API design concerns though.
Doc/library/ssl.rst
Outdated
@@ -430,9 +431,14 @@ Certificate handling | |||
of the certificate, is now supported. | |||
|
|||
.. versionchanged:: 3.7 | |||
The function is no longer used. Hostname matching is now performed |
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.
"no longer used by TLS connections" or something?
Doc/library/ssl.rst
Outdated
@@ -586,6 +592,42 @@ Constants | |||
be passed, either to :meth:`SSLContext.load_verify_locations` or as a | |||
value of the ``ca_certs`` parameter to :func:`wrap_socket`. | |||
|
|||
.. class:: HostFlags |
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.
Do all of these need to be a public API?
I don't see a need to support things like partial wildcards or multi-label wildcards.
The previous implementation didn't support them, and I don't think expanding the public API like this is a good idea.
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.
Partial wildcard matching is disabled by default. I have to expose the flag value so the getter can report the flag correctly:
>>> import ssl
>>> ssl.create_default_context().host_flags
<HostFlags.HOSTFLAG_NO_PARTIAL_WILDCARDS: 4>
I also like to add NEVER_CHECK_SUBJECT
for urllib3, too.
I don't care about the other flags. I'm happy to remove them.
Doc/library/ssl.rst
Outdated
@@ -1730,6 +1778,14 @@ to speed up repeated connections from the same clients. | |||
The protocol version chosen when constructing the context. This attribute | |||
is read-only. | |||
|
|||
.. attribute:: SSLContext.host_flags |
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.
Does this need to be a public API? I feel like we can set reasonable defaults that are applicable to everything.
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 exposed the flag primary for urllib3/urllib3#497. urllib3 wants to disable CN support and require certificates with SAN fields.
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.
What if we just expose a single check_subject_hostname
or something like that, instead of all these flags?
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'm ok with that, but I'm not 100% happy with the name.
check_subject_cn
check_common_name
check_subject_common_name
use_common_name
check_only_san
- ...
I'm going to rename host_flags
to _host_flags
, keep all flags in _ssl
and implement the flag in pure Python. That way we don't expose the API, but still have it available to implement other flags.
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.
Sounds good -- my desire is for us not to commit to a really broad API when we really want a tiny thing.
hostname_checks_common_name
perhaps?
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.
That's explicit, almost German. I like it :)
The feature is only available with OpenSSL 1.1.0+. How should I handle OpenSSL 1.0.2? Don't define the property at all or only implement readonly property + ssl.HAS_NEVER_CHECK_HOSTNAME
?
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.
readonly + a constant makes sense to me
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.
Done
I'm currently at a conference and will take care of proper documentation next week.
Lib/asyncio/sslproto.py
Outdated
if (self._server_hostname and | ||
self._sslcontext.verify_mode != ssl.CERT_NONE): | ||
ssl.match_hostname(peercert, self._server_hostname) | ||
# Since 3.7, the hostname is matched by OpenSSL during handshake. |
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.
Not sure this comment is needed here, since it's not acually referring to any code which exists
2199b99
to
e819fbd
Compare
The ssl module now uses OpenSSL's X509_VERIFY_PARAM_set1_host() and X509_VERIFY_PARAM_set1_ip() API to verify hostname and IP addresses. Signed-off-by: Christian Heimes <christian@python.org>
Signed-off-by: Christian Heimes <christian@python.org>
Signed-off-by: Christian Heimes <christian@python.org>
libssl must provide X509_VERIFY_PARAM_set1_host() Signed-off-by: Christian Heimes <christian@python.org>
Signed-off-by: Christian Heimes <christian@python.org>
Remove all hostflags except for NO_PARTIAL_WILDCARDS and NEVER_CHECK_SUBJECT. The other flags aren't that useful at the moment. Don't support OpenSSL special mode with a leading dot, e.g. ".example.org" matches "www.example.org". It's not standard conform. Signed-off-by: Christian Heimes <christian@python.org>
Host flags are now in internal API. Public API is a new attribute hostname_checks_common_name. Signed-off-by: Christian Heimes <christian@python.org>
e819fbd
to
845c149
Compare
@tiran: Please replace |
Running into this patch on travis-ci as ubuntu trusty ships with openssl 1.0.1 -- this patch causes python3.7 to build without
I agree that the patch is the right move going forward 👍, figured I'd post this here for visibility though. |
Replace Python's custom ssl.match_hostname() with OpenSSL 1.0.2 APIs. The hostname or IP address are now verified by OpenSSL during the TLS handshake. A failure to verify the name results in a proper handshake abort with TLS Alert
bad cert
.Signed-off-by: Christian Heimes christian@python.org
https://bugs.python.org/issue31399