bpo-31399: Let OpenSSL verify hostname and IP address by tiran · Pull Request #3462

bpo-31399: Let OpenSSL verify hostname and IP address by tiran · Pull Request #3462 · python/cpython

Member

alex left a 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.

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

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?

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

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.

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.

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

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.

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.

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?

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.

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?

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?

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

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.

		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.

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