Lean on SSLContext to verify hostnames when possible (#2178)

* Use exact same SNI logic in pyOpenSSL and SecureTransport

It won't make a difference in practice, and avoids useless differences.

* Move "no SNI if IP address" responsibilty to TLS backends

 * 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.

* Stop exposing is_ipaddress as a public

We already have way too much public functions

* Lean on SSLContext to verify hostnames when possible

* Address review comments

9 files changed, 37 insertions, 56 deletions

diff --git a/src/urllib3/connection.py b/src/urllib3/connection.py
index 51de132f..cd708b77 100644
--- a/src/urllib3/connection.py
+++ b/src/urllib3/connection.py
@@ -30,7 +30,7 @@ from .exceptions import (
     SystemTimeWarning,
 )
 from .packages.ssl_match_hostname import CertificateError, match_hostname
-from .util import SKIP_HEADER, SKIPPABLE_HEADERS, connection
+from .util import SKIP_HEADER, SKIPPABLE_HEADERS, connection, ssl_
 from .util.ssl_ import (
     assert_fingerprint,
     create_urllib3_context,
@@ -362,6 +362,18 @@ class HTTPSConnection(HTTPConnection):
                 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 (
+                # `ssl` can't verify fingerprints or alternate hostnames
+                self.assert_fingerprint
+                or self.assert_hostname
+                # We still support OpenSSL 1.0.2, which prevents us from verifying
+                # hostnames easily: https://github.com/pyca/pyopenssl/pull/933
+                or ssl_.IS_PYOPENSSL
+                # Python 3.6 can't disable commonName checks
+                or not hasattr(self.ssl_context, "hostname_checks_common_name")
+            ):
+                self.ssl_context.check_hostname = False
 
         context = self.ssl_context
         context.verify_mode = resolve_cert_reqs(self.cert_reqs)
@@ -416,9 +428,6 @@ class HTTPSConnection(HTTPConnection):
             and not context.check_hostname
             and self.assert_hostname is not False
         ):
-            # While urllib3 attempts to always turn off hostname matching from
-            # the TLS library, this cannot always be done. So we check whether
-            # the TLS Library still thinks it's matching hostnames.
             cert = self.sock.getpeercert()
             _match_hostname(cert, self.assert_hostname or server_hostname)
 
diff --git a/src/urllib3/contrib/pyopenssl.py b/src/urllib3/contrib/pyopenssl.py
index cea32472..e2ffbf1b 100644
--- a/src/urllib3/contrib/pyopenssl.py
+++ b/src/urllib3/contrib/pyopenssl.py
@@ -66,6 +66,7 @@ from socket import socket as socket_cls
 from socket import timeout
 
 from .. import util
+from ..util.ssl_ import is_ipaddress
 
 __all__ = ["inject_into_urllib3", "extract_from_urllib3"]
 
@@ -456,10 +457,10 @@ class PyOpenSSLContext:
     ):
         cnx = OpenSSL.SSL.Connection(self._ctx, sock)
 
-        if isinstance(server_hostname, str):
-            server_hostname = server_hostname.encode("utf-8")
-
-        if server_hostname is not None:
+        # If server_hostname is an IP, don't use it for SNI, per RFC6066 Section 3
+        if server_hostname and not is_ipaddress(server_hostname):
+            if isinstance(server_hostname, str):
+                server_hostname = server_hostname.encode("utf-8")
             cnx.set_tlsext_host_name(server_hostname)
 
         cnx.set_connect_state()
diff --git a/src/urllib3/contrib/securetransport.py b/src/urllib3/contrib/securetransport.py
index 190d32f2..0b6f640c 100644
--- a/src/urllib3/contrib/securetransport.py
+++ b/src/urllib3/contrib/securetransport.py
@@ -451,6 +451,8 @@ class WrappedSocket:
         _assert_no_error(result)
 
         # If we have a server hostname, we should set that too.
+        # RFC6066 Section 3 tells us not to use SNI when the host is an IP, but we have
+        # to do it anyway to match server_hostname against the server certificate
         if server_hostname:
             if not isinstance(server_hostname, bytes):
                 server_hostname = server_hostname.encode("utf-8")
diff --git a/src/urllib3/util/ssl_.py b/src/urllib3/util/ssl_.py
index a2c8dbd4..5c2bd663 100644
--- a/src/urllib3/util/ssl_.py
+++ b/src/urllib3/util/ssl_.py
@@ -241,9 +241,10 @@ def create_urllib3_context(
         context.post_handshake_auth = True
 
     context.verify_mode = cert_reqs
-    # We do our own verification, including fingerprints and alternative
-    # hostnames. So disable it here
-    context.check_hostname = False
+    # 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
 
     # Enable logging of TLS session keys via defacto standard environment variable
     # 'SSLKEYLOGFILE', if the feature is available (Python 3.8+). Skip empty values.
@@ -295,9 +296,8 @@ def ssl_wrap_socket(
     """
     context = ssl_context
     if context is None:
-        # Note: This branch of code and all the variables in it are no longer
-        # used by urllib3 itself. We should consider deprecating and removing
-        # this code.
+        # Note: This branch of code and all the variables in it are only used in tests.
+        # We should consider deprecating and removing this code.
         context = create_urllib3_context(ssl_version, cert_reqs, ciphers=ciphers)
 
     if ca_certs or ca_cert_dir or ca_cert_data:
@@ -328,15 +328,7 @@ def ssl_wrap_socket(
     except NotImplementedError:  # Defensive: in CI, we always have set_alpn_protocols
         pass
 
-    # If we detect server_hostname is an IP address then the SNI
-    # extension should not be used according to RFC3546 Section 3.1
-    use_sni_hostname = server_hostname and not is_ipaddress(server_hostname)
-    # SecureTransport uses server_hostname in certificate verification.
-    send_sni = (use_sni_hostname and HAS_SNI) or (
-        IS_SECURETRANSPORT and server_hostname
-    )
-    # Do not warn the user if server_hostname is an invalid SNI hostname.
-    if not HAS_SNI and use_sni_hostname:
+    if not HAS_SNI and server_hostname and not is_ipaddress(server_hostname):
         warnings.warn(
             "An HTTPS request has been made, but the SNI (Server Name "
             "Indication) extension to TLS is not available on this platform. "
@@ -348,7 +340,6 @@ def ssl_wrap_socket(
             SNIMissingWarning,
         )
 
-    server_hostname = server_hostname if send_sni else None
     ssl_sock = _ssl_wrap_socket_impl(sock, context, tls_in_tls, server_hostname)
     return ssl_sock
 
diff --git a/test/test_ssl.py b/test/test_ssl.py
index f2203282..b36d8d92 100644
--- a/test/test_ssl.py
+++ b/test/test_ssl.py
@@ -41,30 +41,6 @@ class TestSSL:
         assert not ssl_.is_ipaddress(addr)
 
     @pytest.mark.parametrize(
-        ["has_sni", "server_hostname", "uses_sni"],
-        [
-            (True, "127.0.0.1", False),
-            (False, "www.python.org", False),
-            (False, "0.0.0.0", False),
-            (True, "www.google.com", True),
-            (True, None, False),
-            (False, None, False),
-        ],
-    )
-    def test_context_sni_with_ip_address(
-        self, monkeypatch, has_sni, server_hostname, uses_sni
-    ):
-        monkeypatch.setattr(ssl_, "HAS_SNI", has_sni)
-
-        sock = mock.Mock()
-        context = mock.create_autospec(ssl_.SSLContext)
-
-        ssl_.ssl_wrap_socket(sock, server_hostname=server_hostname, ssl_context=context)
-
-        expected_hostname = server_hostname if uses_sni else None
-        context.wrap_socket.assert_called_with(sock, server_hostname=expected_hostname)
-
-    @pytest.mark.parametrize(
         ["has_sni", "server_hostname", "should_warn"],
         [
             (True, "www.google.com", False),
diff --git a/test/test_util.py b/test/test_util.py
index 27c5f027..ccbc4d06 100644
--- a/test/test_util.py
+++ b/test/test_util.py
@@ -880,10 +880,7 @@ class TestUtilSSL:
         """Test that a warning is not made if server_hostname is an IP address."""
         sock = object()
         context, warn = self._wrap_socket_and_mock_warn(sock, "8.8.8.8")
-        expected_hostname = "8.8.8.8" if util.IS_SECURETRANSPORT else None
-        context.wrap_socket.assert_called_once_with(
-            sock, server_hostname=expected_hostname
-        )
+        context.wrap_socket.assert_called_once_with(sock, server_hostname="8.8.8.8")
         warn.assert_not_called()
 
     def test_ssl_wrap_socket_sni_none_no_warn(self):
diff --git a/test/with_dummyserver/test_https.py b/test/with_dummyserver/test_https.py
index 9252c745..bf36b986 100644
--- a/test/with_dummyserver/test_https.py
+++ b/test/with_dummyserver/test_https.py
@@ -266,7 +266,7 @@ class TestHTTPS(HTTPSDummyServerTestCase):
             "127.0.0.1", self.port, cert_reqs="CERT_REQUIRED", ca_certs=DEFAULT_CA
         ) as https_pool:
             with pytest.raises(MaxRetryError) as e:
-                https_pool.request("GET", "/")
+                https_pool.request("GET", "/", retries=0)
             assert isinstance(e.value.reason, SSLError)
             assert "doesn't match" in str(
                 e.value.reason
diff --git a/test/with_dummyserver/test_proxy_poolmanager.py b/test/with_dummyserver/test_proxy_poolmanager.py
index 037f3fc6..76094daa 100644
--- a/test/with_dummyserver/test_proxy_poolmanager.py
+++ b/test/with_dummyserver/test_proxy_poolmanager.py
@@ -197,7 +197,9 @@ class TestHTTPProxyManager(HTTPDummyProxyTestCase):
             )
             https_fail_pool = http._new_pool("https", "127.0.0.1", self.https_port)
 
-            with pytest.raises(MaxRetryError, match="doesn't match") as e:
+            with pytest.raises(
+                MaxRetryError, match="doesn't match|IP address mismatch"
+            ) as e:
                 https_fail_pool.request("GET", "/", retries=0)
             assert isinstance(e.value.reason, SSLError)
 
diff --git a/test/with_dummyserver/test_socketlevel.py b/test/with_dummyserver/test_socketlevel.py
index 97788bf7..a4a175ce 100644
--- a/test/with_dummyserver/test_socketlevel.py
+++ b/test/with_dummyserver/test_socketlevel.py
@@ -1351,7 +1351,10 @@ class TestSSL(SocketDummyServerTestCase):
         with mock.patch("urllib3.util.ssl_.SSLContext", lambda *_, **__: context):
             self._start_server(socket_handler)
             with HTTPSConnectionPool(self.host, self.port) as pool:
-                with pytest.raises(MaxRetryError):
+                # Without a proper `SSLContext`, this request will fail in some
+                # arbitrary way, but we only want to know if load_default_certs() was
+                # called, which is why we accept any `Exception` here.
+                with pytest.raises(Exception):
                     pool.request("GET", "/", timeout=SHORT_TIMEOUT)
                 context.load_default_certs.assert_called_with()
 
@@ -1395,7 +1398,7 @@ class TestSSL(SocketDummyServerTestCase):
                 self._start_server(socket_handler)
 
                 with HTTPSConnectionPool(self.host, self.port, **kwargs) as pool:
-                    with pytest.raises(MaxRetryError):
+                    with pytest.raises(Exception):
                         pool.request("GET", "/", timeout=SHORT_TIMEOUT)
                     context.load_default_certs.assert_not_called()