diff options
| -rw-r--r-- | src/urllib3/connection.py | 17 | ||||
| -rw-r--r-- | src/urllib3/contrib/pyopenssl.py | 9 | ||||
| -rw-r--r-- | src/urllib3/contrib/securetransport.py | 2 | ||||
| -rw-r--r-- | src/urllib3/util/ssl_.py | 23 | ||||
| -rw-r--r-- | test/test_ssl.py | 24 | ||||
| -rw-r--r-- | test/test_util.py | 5 | ||||
| -rw-r--r-- | test/with_dummyserver/test_https.py | 2 | ||||
| -rw-r--r-- | test/with_dummyserver/test_proxy_poolmanager.py | 4 | ||||
| -rw-r--r-- | test/with_dummyserver/test_socketlevel.py | 7 |
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() |