From 6c52a1f1d5447842f9dcef32bb3923c369577323 Mon Sep 17 00:00:00 2001 From: PleasantMachine9 <65126927+PleasantMachine9@users.noreply.github.com> Date: Sun, 14 Jun 2020 16:20:21 +0200 Subject: [PATCH] add tests for new SSLSessionResumptionPolicy approach Hopefully this covers every case. --- src/urllib3/connection.py | 12 +- test/test_connection.py | 50 ++++++- test/with_dummyserver/test_https.py | 202 ++++++++++++++-------------- 3 files changed, 162 insertions(+), 102 deletions(-) diff --git a/src/urllib3/connection.py b/src/urllib3/connection.py index 52598af61b..2c9d25bd32 100644 --- a/src/urllib3/connection.py +++ b/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 diff --git a/test/test_connection.py b/test/test_connection.py index a3cd29c976..2026190102 100644 --- a/test/test_connection.py +++ b/test/test_connection.py @@ -3,7 +3,13 @@ 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): @@ -11,6 +17,48 @@ 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" diff --git a/test/with_dummyserver/test_https.py b/test/with_dummyserver/test_https.py index bc5acb507d..d6e380c0ab 100644 --- a/test/with_dummyserver/test_https.py +++ b/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,63 +133,46 @@ 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() @@ -197,81 +180,104 @@ def __ssl_resumption_enabled_assertions(self, https_pool): 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