add tests for new SSLSessionResumptionPolicy approach

Hopefully this covers every case.

src/urllib3/connection.py

@@ -413,7 +413,8 @@ def connect(self):
                     == SSLSessionResumptionPolicy.ALWAYS
                 ):
                     # Double negatives are confusing. Unset this ssl flag,
-                    # so that we *do* request a ticket on TLSv1.2 and below.
+                    # so that we *do* request a ticket on TLSv1.2 and below,
+                    # in case the policy is ALWAYS
                     self.ssl_context.options &= ~SSL_OP_NO_TICKET
                 else:
                     # In other cases, set the flag, so we don't
@@ -496,12 +497,17 @@ def _save_ssl_session_if_needed(self):
         """
         curr_ssl_session = getattr(self.sock, "session", None)
         if curr_ssl_session is None:
-            log.debug("For %s: not saving ssl session as as curr_ssl_session is None")
+            log.debug(
+                "For %s: not saving ssl session as as curr_ssl_session is %s",
+                self,
+                curr_ssl_session,
+            )
             return
 
         if self.ssl_session_resumption_policy == SSLSessionResumptionPolicy.NEVER:
             log.debug(
-                "For %s: not saving ssl session as SSLSessionResumptionPolicy is NEVER"
+                "For %s: not saving ssl session as SSLSessionResumptionPolicy is NEVER",
+                self,
             )
             return
 

test/test_connection.py

@@ -3,14 +3,62 @@
 
 import pytest
 
-from urllib3.connection import CertificateError, _match_hostname, RECENT_DATE
+from urllib3.connection import (
+    CertificateError,
+    _match_hostname,
+    RECENT_DATE,
+    SSLSessionResumptionPolicy,
+    HTTPSConnection,
+)
 
 
 class TestConnection(object):
     """
     Tests in this suite should not make any network requests or connections.
     """
 
+    @mock.patch('urllib3.connection.HTTPSConnection.connect', return_value=None)
+    def test_httpsconn_save_ssl_session_if_policy_balanced(self, mock_connect):
+        """
+        This is an edge-case which may not be covered in test_https, because the
+        ``session.has_ticket`` should really never be ``True`` pre-TLSv1.3,
+        since we set OP_NO_TICKET, which means the server should not be sending a ticket.
+        Just in case, this code path should be checked too for 100% coverage.
+        """
+        # __init__ will not call connect(), so no socket connection
+        # is established in this test:
+        https_conn = HTTPSConnection(
+            "localhost",
+            ssl_session_resumption_policy=SSLSessionResumptionPolicy.BALANCED,
+        )
+
+        sock = mock.Mock()
+        session = mock.Mock()
+        sock.session = session
+        https_conn.sock = sock
+
+        attr_not_set = object()
+        for ver in (attr_not_set, None, "TLSv1", "TLSv1.1", "TLSv1.2"):
+            if ver is not attr_not_set:
+                sock.version = lambda: ver
+            for has_ticket in (True, False):
+                https_conn.ssl_session = None
+                session.has_ticket = has_ticket
+                https_conn._save_ssl_session_if_needed()
+                assert (
+                    not has_ticket or https_conn.ssl_session is None
+                ), "For TLS version %s has_ticket: %s" % (ver, has_ticket)
+
+        # finally check that in case of TLSv1.3, it's saved:
+        sock.version = lambda: "TLSv1.3"
+        for has_ticket in (True, False):
+            https_conn.ssl_session = None
+            session.has_ticket = has_ticket
+            https_conn._save_ssl_session_if_needed()
+            assert https_conn.ssl_session is session
+
+        mock_connect.assert_not_called()
+
     def test_match_hostname_no_cert(self):
         cert = None
         asserted_hostname = "foo"

test/with_dummyserver/test_https.py

@@ -35,7 +35,7 @@
     LONG_TIMEOUT,
     resolvesLocalhostFQDN,
 )
-from urllib3 import HTTPSConnectionPool
+from urllib3 import HTTPSConnectionPool, SSLSessionResumptionPolicy
 from urllib3.connection import VerifiedHTTPSConnection, RECENT_DATE, SSL_OP_NO_TICKET
 from urllib3.exceptions import (
     SSLError,
@@ -133,145 +133,151 @@ def test_simple(self):
             r = https_pool.request("GET", "/")
             assert r.status == 200, r.data
 
-    def __is_sock_tls_1_3(self, sock):
-        return "TLSv1.3" in (
-            self.tls_protocol_name,
-            # .version() is Python 3.5+
-            getattr(sock, "version", lambda *args: None)(),
-        )
+    def __ssl_resumption_assertions(self, https_pool):
+        def is_sock_tls_1_3(sock):
+            return "TLSv1.3" in (
+                self.tls_protocol_name,
+                # .version() is Python 3.5+
+                sock.version() if hasattr(sock, "version") else None,
+            )
 
-    def __is_ssl_session_ticket_capable(self, sock):
-        return isinstance(sock, ssl.SSLSocket) and hasattr(sock, "session")
+        def is_sock_ssl_session_capable(sock):
+            return isinstance(sock, ssl.SSLSocket) and hasattr(sock, "session")
 
-    def __ssl_resumption_disabled_assertions(self, https_pool):
-        r = https_pool.request("GET", "/")
-        assert r.status == 200, r.data
-        https_conn = https_pool.pool.get_nowait()
-        assert https_conn is not None
-        https_pool.pool.put(
-            https_conn
-        )  # put https_conn back so next request can use it
-        first_req_sock = https_conn.sock
-        # We should have this flag set in case ssl_session_reuse==False
-        if SSL_OP_NO_TICKET is not None:
-            assert (SSL_OP_NO_TICKET & https_conn.ssl_context.options) != 0
-
-        assert https_conn.ssl_session_reuse is False
-        assert https_conn.ssl_session is None
-
-        if self.__is_ssl_session_ticket_capable(first_req_sock):
-            assert https_conn.sock.session_reused is False
-            assert https_conn.sock.session is not None
-            # for some reason on 1.3 the SSL_OP_NO_TICKET is ignored
-            if not (
-                SSL_OP_NO_TICKET is None or self.__is_sock_tls_1_3(https_conn.sock)
-            ):
-                assert https_conn.sock.session.has_ticket is False
-
-        https_conn.sock.close()  # force next call to reopen TCP layer by closing it
-        https_conn.sock = None
-        r = https_pool.request("GET", "/")
-        assert r.status == 200, r.data
-        assert first_req_sock is not https_conn.sock
-        # make sure that the session was not reused:
-        assert https_conn.ssl_session is None
-
-        # We should have this flag set in case ssl_session_reuse==False
-        if SSL_OP_NO_TICKET is not None:
-            assert (SSL_OP_NO_TICKET & https_conn.ssl_context.options) != 0
-
-        if self.__is_ssl_session_ticket_capable(first_req_sock):
-            assert https_conn.sock.session_reused is False
-            assert https_conn.sock.session is not None
-            # for some reason on 1.3 the SSL_OP_NO_TICKET is ignored
-            if not (
-                SSL_OP_NO_TICKET is None or self.__is_sock_tls_1_3(https_conn.sock)
-            ):
-                assert https_conn.sock.session.has_ticket is False
-
-    def __ssl_resumption_enabled_assertions(self, https_pool):
+        def https_conn_assertions(https_conn, policy):
+            if SSL_OP_NO_TICKET is not None:
+                # OP_NO_TICKET should only be disabled if policy is ALWAYS
+                if policy == SSLSessionResumptionPolicy.ALWAYS:
+                    assert (SSL_OP_NO_TICKET & https_conn.ssl_context.options) == 0
+                else:
+                    assert (SSL_OP_NO_TICKET & https_conn.ssl_context.options) != 0
+
+            if policy == SSLSessionResumptionPolicy.NEVER:
+                assert https_conn.ssl_session is None
+            elif policy == SSLSessionResumptionPolicy.ALWAYS:
+                if is_sock_ssl_session_capable(https_conn.sock):
+                    assert https_conn.ssl_session is not None
+                else:
+                    assert https_conn.ssl_session is None
+            elif policy == SSLSessionResumptionPolicy.BALANCED:
+                if is_sock_ssl_session_capable(https_conn.sock):
+                    assert https_conn.ssl_session is not None
+                    # in BALANCED mode, we should not be requesting tickets before TLSv1.3:
+                    if not is_sock_tls_1_3(https_conn.sock):
+                        assert https_conn.ssl_session.has_ticket is False
+                else:
+                    assert https_conn.ssl_session is None
+            else:
+                assert False, "Unknown policy %s" % policy
+
+        policy = https_pool.conn_kw.get(
+            "ssl_session_resumption_policy", SSLSessionResumptionPolicy.DEFAULT
+        )
         r = https_pool.request("GET", "/")
         assert r.status == 200, r.data
         https_conn = https_pool.pool.get_nowait()
         assert https_conn is not None
         https_pool.pool.put(
             https_conn
         )  # put https_conn back so next request can use it
-        first_req_sock = https_conn.sock
 
-        assert https_conn.ssl_session_reuse is True
-        # We should not have this flag set in case ssl_session_reuse==True
-        if SSL_OP_NO_TICKET is not None:
-            assert (SSL_OP_NO_TICKET & https_conn.ssl_context.options) == 0
+        https_conn_assertions(https_conn, policy)
 
-        if self.__is_ssl_session_ticket_capable(first_req_sock):
-            assert https_conn.ssl_session is not None
-            assert https_conn.sock.session is not None
-            assert https_conn.sock.session.has_ticket is True
-            assert https_conn.sock.session_reused is False
-
-        https_conn.sock.close()  # force next call to reopen TCP layer by closing it
+        # force next call to reopen TCP layer by closing it
+        https_conn.sock.close()
+        first_req_sock = https_conn.sock
         https_conn.sock = None
+
         r = https_pool.request("GET", "/")
         assert r.status == 200, r.data
         assert first_req_sock is not https_conn.sock
 
-        # We should not have this flag set in case ssl_session_reuse==True
-        if SSL_OP_NO_TICKET is not None:
-            assert (SSL_OP_NO_TICKET & https_conn.ssl_context.options) == 0
-
-        if self.__is_ssl_session_ticket_capable(first_req_sock):
-            assert https_conn.sock.session is not None
-            #    FIXME: For some reason the Tornado HTTPS server ignores the
-            #    session ticket that itself sent in `first_req_sock.session`,
-            #    even though with pcap tools it can be observed that
-            #    the 2nd ClientHello contains the session key as expected.
-            #    Is Tornado messing up, or ssl.wrap_socket/do_handshake?
-            #    Uncomment this when the HTTPS server handles session tickets properly:
-            # assert https_conn.sock.session_reused is True
-            assert https_conn.ssl_session is not None
-
-    def test_ssl_session_resumption_enabled(self):
-        """Test the cases where we tell HTTPSConnectionPool explicitly or implicitly
-        to attempt session reuse, if the underlying socket supports it.
-        Test both the SNI and IP address requests to cover both branches in ssl_.py."""
+        https_conn_assertions(https_conn, policy)
+        #    FIXME: For some reason the Tornado HTTPS server ignores the
+        #    session ticket that itself sent in `first_req_sock.session`,
+        #    even though with pcap tools it can be observed that
+        #    the 2nd ClientHello contains the session key as expected.
+        #    Is Tornado messing up, or ssl.wrap_socket/do_handshake?
+        #    Uncomment this when the HTTPS server handles session tickets properly:
+        # assert https_conn.sock.session_reused == (
+        #     policy == SSLSessionResumptionPolicy.ALWAYS
+        #     or (policy == SSLSessionResumptionPolicy.BALANCED
+        #         and is_sock_tls_1_3(first_req_sock)
+        # )
+
+    def test_ssl_session_resumption_policy_balanced(self):
+        """
+        Test the cases where we tell HTTPSConnectionPool explicitly or implicitly
+        to use the ``BALANCED`` session resumption policy, if the underlying socket supports it.
+        This means that we should set OP_NO_TICKET on TLSv1.2 and earlier
+        Test both the SNI and IP address requests to cover both branches in ssl_.py.
+        """
         for host in ("127.0.0.1", self.host):
             with mock.patch("warnings.warn") as warn:
 
-                # ssl_session_reuse=True implicitly:
+                # ssl_session_reuse=BALANCED explicitly:
+                with HTTPSConnectionPool(
+                    host,
+                    self.port,
+                    ca_certs=DEFAULT_CA,
+                    cert_reqs=ssl.CERT_NONE,
+                    ssl_session_resumption_policy=SSLSessionResumptionPolicy.BALANCED,
+                ) as https_pool:
+                    self.__ssl_resumption_assertions(https_pool)
+
+                # ssl_session_reuse=DEFAULT(=BALANCED) implicitly:
                 with HTTPSConnectionPool(
                     host, self.port, ca_certs=DEFAULT_CA, cert_reqs=ssl.CERT_NONE,
                 ) as https_pool:
-                    self.__ssl_resumption_enabled_assertions(https_pool)
+                    self.__ssl_resumption_assertions(https_pool)
+
+            if warn.called:
+                calls = warn.call_args_list
+                assert all(InsecureRequestWarning == x[0][1] for x in calls)
 
-                # ssl_session_reuse=True explicitly:
+    def test_ssl_session_resumption_policy_always(self):
+        """
+        Test the cases where we tell HTTPSConnectionPool explicitly
+        to use the ``ALWAYS`` session resumption policy, if the underlying socket supports it.
+        This means that we should set OP_NO_TICKET on TLSv1.2 and earlier
+        Test both the SNI and IP address requests to cover both branches in ssl_.py.
+        """
+        for host in ("127.0.0.1", self.host):
+            with mock.patch("warnings.warn") as warn:
+
+                # ssl_session_reuse=ALWAYS explicitly:
                 with HTTPSConnectionPool(
                     host,
                     self.port,
                     ca_certs=DEFAULT_CA,
                     cert_reqs=ssl.CERT_NONE,
-                    ssl_session_reuse=True,
+                    ssl_session_resumption_policy=SSLSessionResumptionPolicy.ALWAYS,
                 ) as https_pool:
-                    self.__ssl_resumption_enabled_assertions(https_pool)
+                    self.__ssl_resumption_assertions(https_pool)
 
             if warn.called:
                 calls = warn.call_args_list
                 assert all(InsecureRequestWarning == x[0][1] for x in calls)
 
-    def test_ssl_session_resumption_disabled_explicitly(self):
-        """Test the cases where we tell HTTPSConnectionPool explicitly
-        not to attempt session reuse, even if the underlying socket supports it.
-        Test both the SNI and IP address requests to cover both branches in ssl_.py."""
+    def test_ssl_session_resumption_policy_never(self):
+        """
+        Test the cases where we tell HTTPSConnectionPool explicitly
+        to use the ``NEVER`` session resumption policy, if the underlying socket supports it.
+        This means that we should set OP_NO_TICKET on TLSv1.2 and earlier
+        Test both the SNI and IP address requests to cover both branches in ssl_.py.
+        """
         for host in ("127.0.0.1", self.host):
             with mock.patch("warnings.warn") as warn:
+
+                # ssl_session_reuse=NEVER explicitly:
                 with HTTPSConnectionPool(
                     host,
                     self.port,
                     ca_certs=DEFAULT_CA,
                     cert_reqs=ssl.CERT_NONE,
-                    ssl_session_reuse=False,
+                    ssl_session_resumption_policy=SSLSessionResumptionPolicy.NEVER,
                 ) as https_pool:
-                    self.__ssl_resumption_disabled_assertions(https_pool)
+                    self.__ssl_resumption_assertions(https_pool)
 
             if warn.called:
                 calls = warn.call_args_list