From 0461c496d83100a12656c371410eba76542b2e4a Mon Sep 17 00:00:00 2001 From: PleasantMachine9 <65126927+PleasantMachine9@users.noreply.github.com> Date: Mon, 29 Jun 2020 23:28:18 +0200 Subject: [PATCH 1/3] Add support for TLS session resumption in HTTPSConnection Indicate with a simple flag kwarg whether this should be enabled. To test it in the wild, CPython 3.6+ and an SSL ticket/id issuing server is needed. This cannot be tested with current libraries in the repo, as tornado (which is the mock https server) does not seem to work properly with SSL tickets/ids. Below is a way to test it in the real world: import logging from urllib3.connectionpool import connection_from_url logging.basicConfig(level=logging.DEBUG) # example sites that support SSL resumption, at time of writing: urls = [ 'https://www.reddit.com/', 'https://www.twitch.tv/', 'https://twitter.com/', ] for url in urls: conn = connection_from_url(url) # by default, connection is kept alive, # for testing this, it's easier if it isn't: conn.request('GET', '/', headers={'Connection':'close'}) conn.request('GET', '/', headers={'Connection':'close'}) # should see: # DEBUG:urllib3.connection:For <...>: session_reused=True --- src/urllib3/connection.py | 38 +++++++++- src/urllib3/connectionpool.py | 28 ++++++++ src/urllib3/util/ssl_.py | 32 +++++++-- test/test_retry.py | 6 +- test/with_dummyserver/test_https.py | 105 +++++++++++++++++++++++++++- 5 files changed, 197 insertions(+), 12 deletions(-) diff --git a/src/urllib3/connection.py b/src/urllib3/connection.py index 51016825c1..7e1e185623 100644 --- a/src/urllib3/connection.py +++ b/src/urllib3/connection.py @@ -71,6 +71,9 @@ class BrokenPipeError(Exception): _CONTAINS_CONTROL_CHAR_RE = re.compile(r"[^-!#$%&'*+.^_`|~0-9a-zA-Z]") +# OP_NO_TICKET is only available in Python 3.6+ +SSL_OP_NO_TICKET = getattr(ssl, "OP_NO_TICKET", None) + class HTTPConnection(_HTTPConnection, object): """ @@ -262,6 +265,7 @@ class HTTPSConnection(HTTPConnection): """ default_port = port_by_scheme["https"] + default_resume_ssl_sessions = True cert_reqs = None ca_certs = None @@ -283,7 +287,10 @@ def __init__( server_hostname=None, **kw ): - + # do this before calling base class: + self.resume_ssl_sessions = kw.pop( + "resume_ssl_sessions", HTTPSConnection.default_resume_ssl_sessions + ) HTTPConnection.__init__(self, host, port, strict=strict, timeout=timeout, **kw) self.key_file = key_file @@ -292,6 +299,8 @@ def __init__( self.ssl_context = ssl_context self.server_hostname = server_hostname + # This may get a value after the first call to .connect(): + self._ssl_session = None # Required property for Google AppEngine 1.9.0 which otherwise causes # HTTPS requests to go out as HTTP. (See Issue #356) self._protocol = "https" @@ -369,6 +378,18 @@ def connect(self): ssl_version=resolve_ssl_version(self.ssl_version), cert_reqs=resolve_cert_reqs(self.cert_reqs), ) + if SSL_OP_NO_TICKET is not None: + # Unset this sslctx flag, so that we express + # to the server we support tickets on TLSv1.2 and below. + # On TLSv1.3, this flag does nothing, and is ignored + # by openssl, as the TLS client no longer states + # it supports tickets to the server explicitly: + if self.resume_ssl_sessions: + self.ssl_context.options &= ~SSL_OP_NO_TICKET + # In other cases, set the flag, expressing + # to the server we don't want a ticket: + else: + self.ssl_context.options |= SSL_OP_NO_TICKET context = self.ssl_context context.verify_mode = resolve_cert_reqs(self.cert_reqs) @@ -384,6 +405,9 @@ def connect(self): ): context.load_default_certs() + # If this is None, it will be the same as if it were ommitted in kwargs: + prev_ssl_session = self._ssl_session + self.sock = ssl_wrap_socket( sock=conn, keyfile=self.key_file, @@ -394,6 +418,7 @@ def connect(self): ca_cert_data=self.ca_cert_data, server_hostname=server_hostname, ssl_context=context, + ssl_session=prev_ssl_session, ) if self.assert_fingerprint: @@ -426,6 +451,17 @@ def connect(self): or self.assert_fingerprint is not None ) + if prev_ssl_session is not None: + log.debug( + "For %s: new socket created, socket.session_reused=%s", + self, + getattr(self.sock, "session_reused", None), + ) + + curr_ssl_session = getattr(self.sock, "session", None) + if curr_ssl_session is not None and self.resume_ssl_sessions: + self._ssl_session = curr_ssl_session + def _match_hostname(cert, asserted_hostname): try: diff --git a/src/urllib3/connectionpool.py b/src/urllib3/connectionpool.py index f1d61116f2..bfbf4ad293 100644 --- a/src/urllib3/connectionpool.py +++ b/src/urllib3/connectionpool.py @@ -868,6 +868,32 @@ class HTTPSConnectionPool(HTTPConnectionPool): ``ca_cert_dir``, ``ssl_version``, ``key_password`` are only used if :mod:`ssl` is available and are fed into :meth:`urllib3.util.ssl_wrap_socket` to upgrade the connection socket into an SSL socket. + + When running on CPython >=3.6 compiled with :mod:`ssl` (openssl binding), + it is possible to resume TLS sessions in case the TCP socket level was dropped/closed. + This feature is enabled by default when supported by the underlying openssl bindings. + To disable it for all connections in this pool, set the kwarg ``resume_ssl_sessions`` + to ``False``. + + >> from urllib3 import HTTPSConnectionPool + >> pool_with_resumption_enabled = HTTPSConnectionPool('localhost', 443) + >> pool_with_resumption_disabled = HTTPSConnectionPool('localhost', 443, resume_ssl_sessions=False) + + To globally change the default behavior in your application + without this explicit kwarg, it is also possible to the following: + + >> from urllib3 import HTTPSConnectionPool + >> from urllib3.connection import HTTPSConnection + >> HTTPSConnection.default_resume_ssl_sessions = False + >> pool_with_resumption_disabled = HTTPSConnectionPool('localhost', 443) + + .. note:: + In most cases the ``Connection: keep-alive`` directive of the HTTP layer solves + largely the same issue and does it better. + However, there are some cases where ``keep-alive`` does not work, + or a middlebox does not agree with too long-lived TCP connections. + In such cases reconnection may be accelerated by TLS resumption, + which will save some CPU work and network traffic for both sides. """ scheme = "https" @@ -1027,6 +1053,8 @@ def connection_from_url(url, **kw): Passes additional parameters to the constructor of the appropriate :class:`.ConnectionPool`. Useful for specifying things like timeout, maxsize, headers, etc. + For the keyword args available, see :class:`.HTTPConnectionPool` + or :class:`.HTTPSConnectionPool`, depending on your url. Example:: diff --git a/src/urllib3/util/ssl_.py b/src/urllib3/util/ssl_.py index 5d84409118..3631ad4607 100644 --- a/src/urllib3/util/ssl_.py +++ b/src/urllib3/util/ssl_.py @@ -16,6 +16,8 @@ HAS_SNI = False IS_PYOPENSSL = False IS_SECURETRANSPORT = False +# SSLSocket.session -> version 3.6 +IS_SSL_SESSION_SUPPORTED = sys.version_info >= (3, 6) ALPN_PROTOCOLS = ["http/1.1"] # Maps the length of a digest to a possible hash function producing this digest @@ -103,6 +105,7 @@ def _const_compare_digest_backport(a, b): try: from ssl import SSLContext # Modern SSL? except ImportError: + IS_SSL_SESSION_SUPPORTED = False class SSLContext(object): # Platform-specific: Python 2 def __init__(self, protocol_version): @@ -316,9 +319,10 @@ def ssl_wrap_socket( ca_cert_dir=None, key_password=None, ca_cert_data=None, + ssl_session=None, ): """ - All arguments except for server_hostname, ssl_context, and ca_cert_dir have + All arguments except for server_hostname, ssl_context, ssl_session, and ca_cert_dir have the same meaning as they do when using :func:`ssl.wrap_socket`. :param server_hostname: @@ -337,6 +341,18 @@ def ssl_wrap_socket( :param ca_cert_data: Optional string containing CA certificates in PEM format suitable for passing as the cadata parameter to SSLContext.load_verify_locations() + :param ssl_session: + If not ``None``, will be passed to :func:`ssl.wrap_socket` + as the `session` keyword argument. + An `_ssl.Session` object is obtained from a previously created + ``SSLSocket`` object, that had a completed handshake. + If provided, the ssl layer may attempt session resumption + depending on the ssl bindings in use and whether a ticket/id + to use for resumption was sent by the SSL server, but there is + no guarantee that a resumption will be attempted, nor that it will succeed. + In case the resumption fails or does not happen, the underlying ssl + layer will fall back to a full handshake - ie. same as when establishing + a new connection, so this failure is transparent to a caller. """ context = ssl_context if context is None: @@ -393,11 +409,17 @@ def ssl_wrap_socket( SNIMissingWarning, ) + # Depending on the underlying wrap_socket, + # it may not accept the `session` kwarg even if it is provided by caller + send_session = IS_SSL_SESSION_SUPPORTED and ssl_session is not None + + wrap_kwargs = {} if send_sni: - ssl_sock = context.wrap_socket(sock, server_hostname=server_hostname) - else: - ssl_sock = context.wrap_socket(sock) - return ssl_sock + wrap_kwargs["server_hostname"] = server_hostname + if send_session: + wrap_kwargs["session"] = ssl_session + + return context.wrap_socket(sock, **wrap_kwargs) def is_ipaddress(hostname): diff --git a/test/test_retry.py b/test/test_retry.py index a29b03e2cd..a470ecb6c1 100644 --- a/test/test_retry.py +++ b/test/test_retry.py @@ -334,11 +334,7 @@ def test_respect_retry_after_header_propagated(self, respect_retry_after_header) ) @pytest.mark.parametrize( "stub_timezone", - [ - "UTC", - "Asia/Jerusalem", - None, - ], + ["UTC", "Asia/Jerusalem", None], indirect=True, ) @pytest.mark.usefixtures("stub_timezone") diff --git a/test/with_dummyserver/test_https.py b/test/with_dummyserver/test_https.py index 5eb241de6b..80843907e6 100644 --- a/test/with_dummyserver/test_https.py +++ b/test/with_dummyserver/test_https.py @@ -36,7 +36,12 @@ resolvesLocalhostFQDN, ) from urllib3 import HTTPSConnectionPool -from urllib3.connection import VerifiedHTTPSConnection, RECENT_DATE +from urllib3.connection import ( + HTTPSConnection, + VerifiedHTTPSConnection, + RECENT_DATE, + SSL_OP_NO_TICKET, +) from urllib3.exceptions import ( SSLError, ConnectTimeoutError, @@ -134,6 +139,104 @@ def test_simple(self): r = https_pool.request("GET", "/") assert r.status == 200, r.data + def __ssl_resumption_assertions(self, https_pool): + def is_sock_ssl_session_capable(sock): + return isinstance(sock, ssl.SSLSocket) and hasattr(sock, "session") + + def https_conn_assertions(https_conn, resume_ssl_sessions): + if SSL_OP_NO_TICKET is not None: + if resume_ssl_sessions: + assert (SSL_OP_NO_TICKET & https_conn.ssl_context.options) == 0 + else: + assert (SSL_OP_NO_TICKET & https_conn.ssl_context.options) != 0 + + if resume_ssl_sessions and is_sock_ssl_session_capable(https_conn.sock): + assert https_conn._ssl_session is not None + else: + assert https_conn._ssl_session is None + + # in case it's none, get the default that it will be + resume_ssl_sessions = https_pool.conn_kw.get( + "resume_ssl_sessions", HTTPSConnection.default_resume_ssl_sessions + ) + 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 + + https_conn_assertions(https_conn, resume_ssl_sessions) + + # 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 + + https_conn_assertions(https_conn, resume_ssl_sessions) + # 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 ( + # not is_sock_ssl_session_capable(https_conn.sock) + # or https_conn.sock.session_reused == resume_ssl_sessions + # ) + + def test_resume_ssl_sessions_enabled(self): + """ + Test the cases where HTTPSConnection objects are implicitly + or explitictly told to try to reuse their previous ssl_sessions if possible. + 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"): + # implicitly: + with HTTPSConnectionPool( + host, + self.port, + ca_certs=DEFAULT_CA, + cert_reqs=ssl.CERT_NONE, + # resume_ssl_sessions=True, + ) as https_pool: + self.__ssl_resumption_assertions(https_pool) + + # explicitly: + with HTTPSConnectionPool( + host, + self.port, + ca_certs=DEFAULT_CA, + cert_reqs=ssl.CERT_NONE, + resume_ssl_sessions=True, + ) as https_pool: + self.__ssl_resumption_assertions(https_pool) + + def test_resume_ssl_sessions_disabled(self): + """ + Test the cases where HTTPSConnection objects are explicitly + told not to reuse their previous ssl_sessions. + In this case, OP_NO_TICKET should be set in the default sslctx of + the connection objects as well. + 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"): + with HTTPSConnectionPool( + host, + self.port, + ca_certs=DEFAULT_CA, + cert_reqs=ssl.CERT_NONE, + resume_ssl_sessions=False, + ) as https_pool: + self.__ssl_resumption_assertions(https_pool) + @resolvesLocalhostFQDN def test_dotted_fqdn(self): with HTTPSConnectionPool( From 93199c04d72bd2dcafe98663ae3bac5f9af54b9f Mon Sep 17 00:00:00 2001 From: PleasantMachine9 <65126927+PleasantMachine9@users.noreply.github.com> Date: Sat, 3 Oct 2020 01:06:37 +0200 Subject: [PATCH 2/3] clean up merge conflicts with tls_in_tls --- src/urllib3/connection.py | 1 + src/urllib3/util/ssl_.py | 41 ++++++++++++++--------------- src/urllib3/util/ssltransport.py | 26 ++++++++++++++++-- test/with_dummyserver/test_https.py | 4 +-- 4 files changed, 47 insertions(+), 25 deletions(-) diff --git a/src/urllib3/connection.py b/src/urllib3/connection.py index a3a059d057..afc8d787e7 100644 --- a/src/urllib3/connection.py +++ b/src/urllib3/connection.py @@ -510,6 +510,7 @@ def _connect_tls_proxy(self, hostname, conn): ssl_context=ssl_context, ) + def _match_hostname(cert, asserted_hostname): try: match_hostname(cert, asserted_hostname) diff --git a/src/urllib3/util/ssl_.py b/src/urllib3/util/ssl_.py index cea99a4766..d2a049ca25 100644 --- a/src/urllib3/util/ssl_.py +++ b/src/urllib3/util/ssl_.py @@ -430,17 +430,30 @@ def ssl_wrap_socket( SNIMissingWarning, ) + wrap_sock_kwargs = {} + # do not pass it if not sending SNI + if send_sni: + wrap_sock_kwargs["server_hostname"] = server_hostname # Depending on the underlying wrap_socket, # it may not accept the `session` kwarg even if it is provided by caller - send_session = IS_SSL_SESSION_SUPPORTED and ssl_session is not None + if IS_SSL_SESSION_SUPPORTED and ssl_session is not None: + wrap_sock_kwargs["session"] = ssl_session - wrap_kwargs = {} - if send_sni: - wrap_kwargs["server_hostname"] = server_hostname - if send_session: - wrap_kwargs["session"] = ssl_session + return _ssl_wrap_socket_impl(sock, context, tls_in_tls, wrap_sock_kwargs) + + +def _ssl_wrap_socket_impl(sock, ssl_context, tls_in_tls, wrap_sock_kwargs): + if tls_in_tls: + if not SSLTransport: + # Import error, ssl is not available. + raise ProxySchemeUnsupported( + "TLS in TLS requires support for the 'ssl' module" + ) + + SSLTransport._validate_ssl_context_for_tls_in_tls(ssl_context) + return SSLTransport(sock, ssl_context, **wrap_sock_kwargs) - return _ssl_wrap_socket_impl(sock, context, tls_in_tls, **wrap_kwargs) + return ssl_context.wrap_socket(sock, **wrap_sock_kwargs) def is_ipaddress(hostname): @@ -465,17 +478,3 @@ def _is_key_file_encrypted(key_file): return True return False - - -def _ssl_wrap_socket_impl(sock, ssl_context, tls_in_tls, **kwargs): - if tls_in_tls: - if not SSLTransport: - # Import error, ssl is not available. - raise ProxySchemeUnsupported( - "TLS in TLS requires support for the 'ssl' module" - ) - - SSLTransport._validate_ssl_context_for_tls_in_tls(ssl_context) - return SSLTransport(sock, ssl_context, server_hostname=kwargs.get("server_hostname")) - - return ssl_context.wrap_socket(sock, **kwargs) diff --git a/src/urllib3/util/ssltransport.py b/src/urllib3/util/ssltransport.py index d23e518de7..5fd6405808 100644 --- a/src/urllib3/util/ssltransport.py +++ b/src/urllib3/util/ssltransport.py @@ -42,10 +42,29 @@ def _validate_ssl_context_for_tls_in_tls(ssl_context): ) def __init__( - self, socket, ssl_context, suppress_ragged_eofs=True, server_hostname=None + self, + socket, + ssl_context, + suppress_ragged_eofs=True, + server_hostname=None, + session=None, ): """ Create an SSLTransport around socket using the provided ssl_context. + + :param socket: + The socket object to wrap. + :param ssl_context: + :class:`ssl.SSLContext` to use call :func:`ssl.SSLContext.wrap_bio` on. + :param suppress_ragged_eofs: + Suppress ``ssl.SSL_ERROR_EOF`` read errors. + :param server_hostname: + If not ``None``, will be passed to :func:`ssl.SSLContext.wrap_bio` + :param session: + If not ``None``, will be passed to :func:`ssl.SSLContext.wrap_bio`. + Should be an :class:`ssl.SSLSession` object from a previously established + :class:`ssl.SSLSocket`, that was created with the same + :class:`ssl.SSLContext` as the one being passed. """ self.incoming = ssl.MemoryBIO() self.outgoing = ssl.MemoryBIO() @@ -54,7 +73,10 @@ def __init__( self.socket = socket self.sslobj = ssl_context.wrap_bio( - self.incoming, self.outgoing, server_hostname=server_hostname + self.incoming, + self.outgoing, + server_hostname=server_hostname, + session=session, ) # Perform initial handshake. diff --git a/test/with_dummyserver/test_https.py b/test/with_dummyserver/test_https.py index d1fae8fcf4..c21fdfb888 100644 --- a/test/with_dummyserver/test_https.py +++ b/test/with_dummyserver/test_https.py @@ -37,10 +37,10 @@ from dummyserver.testcase import HTTPSDummyServerTestCase from urllib3 import HTTPSConnectionPool from urllib3.connection import ( - HTTPSConnection, - VerifiedHTTPSConnection, RECENT_DATE, SSL_OP_NO_TICKET, + HTTPSConnection, + VerifiedHTTPSConnection, ) from urllib3.exceptions import ( ConnectTimeoutError, From a1fd6bc5c196b8cbc10c67ddd64643492ac09205 Mon Sep 17 00:00:00 2001 From: PleasantMachine9 <65126927+PleasantMachine9@users.noreply.github.com> Date: Sat, 3 Oct 2020 01:13:46 +0200 Subject: [PATCH 3/3] fix unknown session kwarg issue on python < 3.6 Explicitly passing the kwarg and it being None is not the same as passing a kwarg dict where it's not added when it is None. --- src/urllib3/util/ssl_.py | 2 +- src/urllib3/util/ssltransport.py | 21 ++++----------------- 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/src/urllib3/util/ssl_.py b/src/urllib3/util/ssl_.py index d2a049ca25..b8ed2a64e9 100644 --- a/src/urllib3/util/ssl_.py +++ b/src/urllib3/util/ssl_.py @@ -373,7 +373,7 @@ def ssl_wrap_socket( layer will fall back to a full handshake - ie. same as when establishing a new connection, so this failure is transparent to a caller. :param tls_in_tls: - Use SSLTransport to wrap the existing socket. + Use :class:`SSLTransport` to wrap the existing socket if available. """ context = ssl_context if context is None: diff --git a/src/urllib3/util/ssltransport.py b/src/urllib3/util/ssltransport.py index 5fd6405808..24731a96d5 100644 --- a/src/urllib3/util/ssltransport.py +++ b/src/urllib3/util/ssltransport.py @@ -42,12 +42,7 @@ def _validate_ssl_context_for_tls_in_tls(ssl_context): ) def __init__( - self, - socket, - ssl_context, - suppress_ragged_eofs=True, - server_hostname=None, - session=None, + self, socket, ssl_context, suppress_ragged_eofs=True, **wrap_bio_kwargs ): """ Create an SSLTransport around socket using the provided ssl_context. @@ -58,13 +53,8 @@ def __init__( :class:`ssl.SSLContext` to use call :func:`ssl.SSLContext.wrap_bio` on. :param suppress_ragged_eofs: Suppress ``ssl.SSL_ERROR_EOF`` read errors. - :param server_hostname: - If not ``None``, will be passed to :func:`ssl.SSLContext.wrap_bio` - :param session: - If not ``None``, will be passed to :func:`ssl.SSLContext.wrap_bio`. - Should be an :class:`ssl.SSLSession` object from a previously established - :class:`ssl.SSLSocket`, that was created with the same - :class:`ssl.SSLContext` as the one being passed. + :param wrap_bio_kwargs: + These args will be passed to :func:`ssl.SSLContext.wrap_bio` """ self.incoming = ssl.MemoryBIO() self.outgoing = ssl.MemoryBIO() @@ -73,10 +63,7 @@ def __init__( self.socket = socket self.sslobj = ssl_context.wrap_bio( - self.incoming, - self.outgoing, - server_hostname=server_hostname, - session=session, + self.incoming, self.outgoing, **wrap_bio_kwargs ) # Perform initial handshake.