From 845499cbb62b95e07e790fbbde47d33449e78181 Mon Sep 17 00:00:00 2001 From: kenneth topp Date: Wed, 21 Sep 2022 22:38:51 -0400 Subject: [PATCH 1/5] Handle SSLZeroReturnError exceptions Python 3.8 introduced a different exception for zero byte tcp connections. These connections are generated by cherrypy on startup, and so the exception is not displayed as it is expected. Without this fix an exception is displayed although it is harmless. This treats the new exception just like it was treated under the errno exception. --- cheroot/ssl/builtin.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cheroot/ssl/builtin.py b/cheroot/ssl/builtin.py index b22d4ae611..0e1d4424f6 100644 --- a/cheroot/ssl/builtin.py +++ b/cheroot/ssl/builtin.py @@ -270,6 +270,11 @@ def wrap(self, sock): s = self.context.wrap_socket( sock, do_handshake_on_connect=True, server_side=True, ) + except ssl.SSLZeroReturnError: + # This is almost certainly due to the cherrypy engine + # 'pinging' the socket to assert it's connectable; + # the 'ping' isn't SSL. + return EMPTY_RESULT except ssl.SSLError as ex: if ex.errno == ssl.SSL_ERROR_EOF: # This is almost certainly due to the cherrypy engine From 931ebd60b9fc111152970fd67c6e8548227302e4 Mon Sep 17 00:00:00 2001 From: kenneth topp Date: Sat, 24 Sep 2022 17:03:44 -0400 Subject: [PATCH 2/5] Simplify SSL socket exceptions in builtin Currently, sockets with SSL exceptions are discarded, except for http-over-https where we send back an http error response. For a subset of those exceptions we silient discard them, and for others we will print out the stack trace. This patch updates the code to silient discard all exceptions unless it is the http-over-https case. --- cheroot/ssl/builtin.py | 60 +++++------------------------------------- stubtest_allowlist.txt | 1 - 2 files changed, 6 insertions(+), 55 deletions(-) diff --git a/cheroot/ssl/builtin.py b/cheroot/ssl/builtin.py index 0e1d4424f6..3e15e02d99 100644 --- a/cheroot/ssl/builtin.py +++ b/cheroot/ssl/builtin.py @@ -27,7 +27,6 @@ from . import Adapter from .. import errors -from .._compat import IS_ABOVE_OPENSSL10 from ..makefile import StreamReader, StreamWriter from ..server import HTTPServer @@ -270,63 +269,16 @@ def wrap(self, sock): s = self.context.wrap_socket( sock, do_handshake_on_connect=True, server_side=True, ) - except ssl.SSLZeroReturnError: - # This is almost certainly due to the cherrypy engine - # 'pinging' the socket to assert it's connectable; - # the 'ping' isn't SSL. - return EMPTY_RESULT except ssl.SSLError as ex: - if ex.errno == ssl.SSL_ERROR_EOF: - # This is almost certainly due to the cherrypy engine - # 'pinging' the socket to assert it's connectable; - # the 'ping' isn't SSL. - return EMPTY_RESULT - elif ex.errno == ssl.SSL_ERROR_SSL: + if ex.errno == ssl.SSL_ERROR_SSL: if _assert_ssl_exc_contains(ex, 'http request'): # The client is speaking HTTP to an HTTPS server. raise errors.NoSSLError - - # Check if it's one of the known errors - # Errors that are caught by PyOpenSSL, but thrown by - # built-in ssl - _block_errors = ( - 'unknown protocol', 'unknown ca', 'unknown_ca', - 'unknown error', - 'https proxy request', 'inappropriate fallback', - 'wrong version number', - 'no shared cipher', 'certificate unknown', - 'ccs received early', - 'certificate verify failed', # client cert w/o trusted CA - 'version too low', # caused by SSL3 connections - 'unsupported protocol', # caused by TLS1 connections - ) - if _assert_ssl_exc_contains(ex, *_block_errors): - # Accepted error, let's pass - return EMPTY_RESULT - elif _assert_ssl_exc_contains(ex, 'handshake operation timed out'): - # This error is thrown by builtin SSL after a timeout - # when client is speaking HTTP to an HTTPS server. - # The connection can safely be dropped. - return EMPTY_RESULT - raise - except generic_socket_error as exc: - """It is unclear why exactly this happens. - - It's reproducible only with openssl>1.0 and stdlib - :py:mod:`ssl` wrapper. - In CherryPy it's triggered by Checker plugin, which connects - to the app listening to the socket port in TLS mode via plain - HTTP during startup (from the same process). - - - Ref: https://github.com/cherrypy/cherrypy/issues/1618 - """ - is_error0 = exc.args == (0, 'Error') - - if is_error0 and IS_ABOVE_OPENSSL10: - return EMPTY_RESULT - raise - return s, self.get_environ(s) + except generic_socket_error: + pass + else: + return s, self.get_environ(s) + return EMPTY_RESULT def get_environ(self, sock): """Create WSGI environ entries to be merged into each request.""" diff --git a/stubtest_allowlist.txt b/stubtest_allowlist.txt index ddc832bf44..89ca49bde0 100644 --- a/stubtest_allowlist.txt +++ b/stubtest_allowlist.txt @@ -37,7 +37,6 @@ cheroot.ssl.pyopenssl.SSLConnection.write # false positives (https://github.com/python/mypy/issues/11843) cheroot.connections.IS_WINDOWS -cheroot.ssl.builtin.IS_ABOVE_OPENSSL10 # suppress is both a function and class cheroot._compat.suppress From 6a82c1d9cae8817d102d1feb0c2fc51ffce0c895 Mon Sep 17 00:00:00 2001 From: Sviatoslav Sydorenko Date: Tue, 4 Jul 2023 20:40:19 +0200 Subject: [PATCH 3/5] =?UTF-8?q?=F0=9F=90=9B=20Turn=20`SSLEOFError`=20into?= =?UTF-8?q?=20`FatalSSLAlert`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch turns a new `ssl.SSLEOFError` into an internally ignored `FatalSSLAlert` allowing it not to leak into the outer abstraction layers in its raw form. The exception is new since Python 3.8 and it's fine to use it unconditionally since we no longer support Python 3.7. This patch also handles `SSLZeroReturnError` same as `SSLEOFError` as it's semantically equivalent per [[1]]. [1]: https://github.com/cherrypy/cheroot/pull/518#issuecomment-1631774708 --- cheroot/ssl/builtin.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/cheroot/ssl/builtin.py b/cheroot/ssl/builtin.py index 3e15e02d99..b128c858ef 100644 --- a/cheroot/ssl/builtin.py +++ b/cheroot/ssl/builtin.py @@ -269,6 +269,13 @@ def wrap(self, sock): s = self.context.wrap_socket( sock, do_handshake_on_connect=True, server_side=True, ) + except ( + ssl.SSLEOFError, + ssl.SSLZeroReturnError, + ) as tls_connection_drop_error: + raise errors.FatalSSLAlert( + *tls_connection_drop_error.args, + ) from tls_connection_drop_error except ssl.SSLError as ex: if ex.errno == ssl.SSL_ERROR_SSL: if _assert_ssl_exc_contains(ex, 'http request'): From eb92823498642b90e8060baf48ab21ac7714c2da Mon Sep 17 00:00:00 2001 From: Sviatoslav Sydorenko Date: Wed, 24 Jan 2024 04:23:01 +0100 Subject: [PATCH 4/5] Relay TLS connection drop through `FatalSSLAlert` Specifically, this patch adds an exception interception code to the place where the socket is being first wrapped. In case of the `builtin` TLS adapter, this is also a place where a handshake attempt happens. This switches the method of relaying an unrecoverable connection error from a sentinel of a tuple with two falsy values to raising a unified exception consistently. --- cheroot/connections.py | 9 +++++++-- cheroot/ssl/builtin.py | 32 +++++++++++++++++++------------- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/cheroot/connections.py b/cheroot/connections.py index 9346bc6aea..9ea6c829f2 100644 --- a/cheroot/connections.py +++ b/cheroot/connections.py @@ -292,6 +292,13 @@ def _from_server_socket(self, server_socket): # noqa: C901 # FIXME if self.server.ssl_adapter is not None: try: s, ssl_env = self.server.ssl_adapter.wrap(s) + except errors.FatalSSLAlert as tls_connection_drop_error: + self.server.error_log( + f'Client {addr !s} lost — peer dropped the TLS ' + 'connection suddenly, during handshake: ' + f'{tls_connection_drop_error !s}', + ) + return except errors.NoSSLError: msg = ( 'The client sent a plain HTTP request, but ' @@ -311,8 +318,6 @@ def _from_server_socket(self, server_socket): # noqa: C901 # FIXME if ex.args[0] not in errors.socket_errors_to_ignore: raise return - if not s: - return mf = self.server.ssl_adapter.makefile # Re-apply our timeout since we may have a new socket object if hasattr(s, 'settimeout'): diff --git a/cheroot/ssl/builtin.py b/cheroot/ssl/builtin.py index b128c858ef..e28e5df188 100644 --- a/cheroot/ssl/builtin.py +++ b/cheroot/ssl/builtin.py @@ -30,8 +30,6 @@ from ..makefile import StreamReader, StreamWriter from ..server import HTTPServer -generic_socket_error = OSError - def _assert_ssl_exc_contains(exc, *msgs): """Check whether SSL exception contains either of messages provided.""" @@ -264,7 +262,6 @@ def bind(self, sock): def wrap(self, sock): """Wrap and return the given socket, plus WSGI environ entries.""" - EMPTY_RESULT = None, {} try: s = self.context.wrap_socket( sock, do_handshake_on_connect=True, server_side=True, @@ -276,16 +273,25 @@ def wrap(self, sock): raise errors.FatalSSLAlert( *tls_connection_drop_error.args, ) from tls_connection_drop_error - except ssl.SSLError as ex: - if ex.errno == ssl.SSL_ERROR_SSL: - if _assert_ssl_exc_contains(ex, 'http request'): - # The client is speaking HTTP to an HTTPS server. - raise errors.NoSSLError - except generic_socket_error: - pass - else: - return s, self.get_environ(s) - return EMPTY_RESULT + except ssl.SSLError as generic_tls_error: + peer_speaks_plain_http_over_https = ( + generic_tls_error.errno == ssl.SSL_ERROR_SSL and + _assert_ssl_exc_contains(generic_tls_error, 'http request') + ) + if peer_speaks_plain_http_over_https: + reraised_connection_drop_exc_cls = errors.NoSSLError + else: + reraised_connection_drop_exc_cls = errors.FatalSSLAlert + + raise reraised_connection_drop_exc_cls( + *generic_tls_error.args, + ) from generic_tls_error + except OSError as tcp_connection_drop_error: + raise errors.FatalSSLAlert( + *tcp_connection_drop_error.args, + ) from tcp_connection_drop_error + + return s, self.get_environ(s) def get_environ(self, sock): """Create WSGI environ entries to be merged into each request.""" From 93d8379ff34312300ad1542309c5584a6aadb6f2 Mon Sep 17 00:00:00 2001 From: Sviatoslav Sydorenko Date: Wed, 24 Jan 2024 05:23:49 +0100 Subject: [PATCH 5/5] Log clients speaking HTTP on the HTTPS port This patch extends the processing of a case when a client attempts sending plain HTTP into an HTTPS port by emitting a log message. --- cheroot/connections.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/cheroot/connections.py b/cheroot/connections.py index 9ea6c829f2..df70e6ea02 100644 --- a/cheroot/connections.py +++ b/cheroot/connections.py @@ -299,7 +299,13 @@ def _from_server_socket(self, server_socket): # noqa: C901 # FIXME f'{tls_connection_drop_error !s}', ) return - except errors.NoSSLError: + except errors.NoSSLError as http_over_https_err: + self.server.error_log( + f'Client {addr !s} attempted to speak plain HTTP into ' + 'a TCP connection configured for TLS-only traffic — ' + 'trying to send back a plain HTTP error response: ' + f'{http_over_https_err !s}', + ) msg = ( 'The client sent a plain HTTP request, but ' 'this server only speaks HTTPS on this port.'