diff --git a/cheroot/connections.py b/cheroot/connections.py index 04df0d42a3..98f17e42f5 100644 --- a/cheroot/connections.py +++ b/cheroot/connections.py @@ -1,12 +1,13 @@ """Utilities to manage open connections.""" -import io +import contextlib as _cm import os import selectors import socket import threading import time -from contextlib import suppress +from http import HTTPStatus as _HTTPStatus +from http.client import responses as _http_responses from . import errors from ._compat import IS_WINDOWS @@ -112,6 +113,16 @@ def close(self): self._selector.close() +@_cm.contextmanager +def _suppress_socket_io_errors(socket, /): + """Suppress known socket I/O errors.""" + try: + yield + except OSError as ex: + if ex.args[0] not in errors.socket_errors_to_ignore: + raise + + class ConnectionManager: """Class which manages HTTPConnection objects. @@ -281,7 +292,7 @@ def _remove_invalid_sockets(self): # One of the reason on why a socket could cause an error # is that the socket is already closed, ignore the # socket error if we try to close it at this point. - with suppress(OSError): + with _cm.suppress(OSError): conn.close() def _from_server_socket(self, server_socket): # noqa: C901 # FIXME @@ -297,7 +308,8 @@ def _from_server_socket(self, server_socket): # noqa: C901 # FIXME ssl_env = {} # if ssl cert and key are set, we try to be a secure HTTP server if self.server.ssl_adapter is not None: - try: + # FIXME: WPS505 -- too many nested blocks + try: # noqa: WPS505 s, ssl_env = self.server.ssl_adapter.wrap(s) except errors.FatalSSLAlert as tls_connection_drop_error: self.server.error_log( @@ -313,23 +325,7 @@ def _from_server_socket(self, server_socket): # noqa: C901 # FIXME '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.' - ) - buf = [ - '%s 400 Bad Request\r\n' % self.server.protocol, - 'Content-Length: %s\r\n' % len(msg), - 'Content-Type: text/plain\r\n\r\n', - msg, - ] - - wfile = mf(s, 'wb', io.DEFAULT_BUFFER_SIZE) - try: - wfile.write(''.join(buf).encode('ISO-8859-1')) - except OSError as ex: - if ex.args[0] not in errors.socket_errors_to_ignore: - raise + self._send_bad_request_plain_http_error(s) return None mf = self.server.ssl_adapter.makefile # Re-apply our timeout since we may have a new socket object @@ -403,3 +399,31 @@ def can_add_keepalive_connection(self): """Flag whether it is allowed to add a new keep-alive connection.""" ka_limit = self.server.keep_alive_conn_limit return ka_limit is None or self._num_connections < ka_limit + + def _send_bad_request_plain_http_error(self, raw_sock, /): + """Send Bad Request 400 response, and close the socket.""" + msg = ( + 'The client sent a plain HTTP request, but this server ' + 'only speaks HTTPS on this port.' + ) + + http_response_status_line = ' '.join( + ( + self.server.protocol, + str(_HTTPStatus.BAD_REQUEST.value), + _http_responses[_HTTPStatus.BAD_REQUEST], + ), + ) + response_parts = [ + f'{http_response_status_line}\r\n', + 'Content-Type: text/plain\r\n', + f'Content-Length: {len(msg)}\r\n', + 'Connection: close\r\n', + '\r\n', + msg, + ] + response_bytes = ''.join(response_parts).encode('ISO-8859-1') + + with _suppress_socket_io_errors(raw_sock), _cm.closing(raw_sock): + raw_sock.sendall(response_bytes) + raw_sock.shutdown(socket.SHUT_WR) diff --git a/cheroot/ssl/__init__.py b/cheroot/ssl/__init__.py index 94c6792c26..23c1e85675 100644 --- a/cheroot/ssl/__init__.py +++ b/cheroot/ssl/__init__.py @@ -1,8 +1,58 @@ """Implementation of the SSL adapter base interface.""" +import socket as _socket from abc import ABC, abstractmethod from warnings import warn as _warn +from .. import errors as _errors + + +def _ensure_peer_speaks_https(raw_socket, /) -> None: + """ + Raise exception if the client sent plain HTTP. + + This method probes the TCP stream for signs of the peer having + sent us plaintext HTTP on the HTTPS port by peeking at the + first bytes. If there's no data yet, the method considers the + guess inconclusive and does not error out. This allows the server + to continue until the SSL handshake is attempted, at which point + an error will be caught by the SSL layer if the client + is not speaking TLS. + + :raises NoSSLError: When plaintext HTTP is detected on an HTTPS socket + """ + PEEK_BYTES = 16 + PEEK_TIMEOUT = 0.5 + + original_timeout = raw_socket.gettimeout() + raw_socket.settimeout(PEEK_TIMEOUT) + + try: + first_bytes = raw_socket.recv(PEEK_BYTES, _socket.MSG_PEEK) + except (OSError, _socket.timeout): + return + finally: + raw_socket.settimeout(original_timeout) + + if not first_bytes: + return + + http_methods = ( + b'GET ', + b'POST ', + b'PUT ', + b'DELETE ', + b'HEAD ', + b'OPTIONS ', + b'PATCH ', + b'CONNECT ', + b'TRACE ', + ) + if first_bytes.startswith(http_methods): + raise _errors.NoSSLError( + 'Expected HTTPS on the socket but got plain HTTP', + ) from None + class Adapter(ABC): """Base class for SSL driver library adapters. @@ -51,7 +101,8 @@ def bind(self, sock): @abstractmethod def wrap(self, sock): """Wrap the given socket and return WSGI environ entries.""" - raise NotImplementedError # pragma: no cover + _ensure_peer_speaks_https(sock) + return sock, {} @abstractmethod def get_environ(self): diff --git a/cheroot/ssl/builtin.py b/cheroot/ssl/builtin.py index 881e09db68..fcde32f93e 100644 --- a/cheroot/ssl/builtin.py +++ b/cheroot/ssl/builtin.py @@ -305,6 +305,10 @@ def context(self, context): def wrap(self, sock): """Wrap and return the given socket, plus WSGI environ entries.""" + sock, _env = super().wrap( # checks for plaintext http on https port + sock, + ) + try: s = self.context.wrap_socket( sock, diff --git a/cheroot/ssl/pyopenssl.py b/cheroot/ssl/pyopenssl.py index 194fd223fd..37fb136d74 100644 --- a/cheroot/ssl/pyopenssl.py +++ b/cheroot/ssl/pyopenssl.py @@ -339,6 +339,11 @@ def wrap(self, sock): # pyOpenSSL doesn't perform the handshake until the first read/write # forcing the handshake to complete tends to result in the connection # closing so we can't reliably access protocol/client cert for the env + + sock, _env = super().wrap( # checks for plaintext http on https port + sock, + ) + conn = SSLConnection(self.context, sock) conn.set_accept_state() # Tell OpenSSL this is a server connection diff --git a/cheroot/test/test_ssl.py b/cheroot/test/test_ssl.py index 881b5ae990..77fb0b96c4 100644 --- a/cheroot/test/test_ssl.py +++ b/cheroot/test/test_ssl.py @@ -1,6 +1,7 @@ """Tests for TLS support.""" import contextlib +import errno import functools import http.client import json @@ -26,7 +27,8 @@ load_pem_private_key, ) -from cheroot.ssl import Adapter +from cheroot.connections import ConnectionManager +from cheroot.ssl import Adapter, _ensure_peer_speaks_https from .._compat import ( IS_ABOVE_OPENSSL10, @@ -35,9 +37,7 @@ IS_LINUX, IS_MACOS, IS_PYPY, - IS_SOLARIS, IS_WINDOWS, - SYS_PLATFORM, bton, ntob, ntou, @@ -699,6 +699,42 @@ def test_https_over_http_error(http_server, ip_addr): assert expected_substring in ssl_err.value.args[-1] +def test_http_over_https_no_data(mocker): + """Test ``_ensure_peer_speaks_https()`` handles empty peek correctly.""" + mock_socket = mocker.Mock(spec=socket.socket) + mock_socket.recv.return_value = b'' # Empty peek + mock_socket.gettimeout.return_value = None + + # Should not raise - empty peek means we can't detect plain HTTP + _ensure_peer_speaks_https(mock_socket) + + mock_socket.recv.assert_called_once_with(16, socket.MSG_PEEK) + + +@pytest.mark.parametrize( + 'exception', + ( + socket.timeout('Timed out'), + ConnectionResetError(errno.ECONNRESET, 'Connection reset by peer'), + ), + ids=('timeout', 'connection_reset'), +) +def test_http_over_https_check_socket_errors( + exception, + mocker, +): + """Test ``_ensure_peer_speaks_https()`` handles socket errors gracefully.""" + mock_socket = mocker.Mock(spec=socket.socket) + mock_socket.gettimeout.return_value = None + mock_socket.recv.side_effect = exception + mocked_sock_recv_spy = mocker.spy(mock_socket, 'recv') + + # socket errors should be suppressed + _ensure_peer_speaks_https(mock_socket) + assert mocked_sock_recv_spy.spy_exception is exception + mocked_sock_recv_spy.assert_called_once() + + @pytest.mark.parametrize( 'adapter_type', ( @@ -713,7 +749,6 @@ def test_https_over_http_error(http_server, ip_addr): pytest.param(ANY_INTERFACE_IPV6, marks=missing_ipv6), ), ) -@pytest.mark.flaky(reruns=3, reruns_delay=2) # pylint: disable-next=too-many-positional-arguments def test_http_over_https_error( http_request_timeout, @@ -726,36 +761,6 @@ def test_http_over_https_error( tls_certificate_private_key_pem_path, ): """Ensure that connecting over HTTP to HTTPS port is handled.""" - # disable some flaky tests - # https://github.com/cherrypy/cheroot/issues/225 - issue_225 = IS_MACOS and adapter_type == 'builtin' - if issue_225: - pytest.xfail('Test fails in Travis-CI') - - if IS_LINUX: - expected_error_code, expected_error_text = ( - 104, - 'Connection reset by peer', - ) - elif IS_MACOS: - expected_error_code, expected_error_text = ( - 54, - 'Connection reset by peer', - ) - elif IS_SOLARIS: - expected_error_code, expected_error_text = ( - None, - 'Remote end closed connection without response', - ) - elif IS_WINDOWS: - expected_error_code, expected_error_text = ( - 10054, - 'An existing connection was forcibly closed by the remote host', - ) - else: - expected_error_code, expected_error_text = None, None - pytest.skip(f'{SYS_PLATFORM} is unsupported') # pragma: no cover - tls_adapter_cls = get_ssl_adapter_class(name=adapter_type) tls_adapter = tls_adapter_cls( tls_certificate_chain_pem_path, @@ -775,31 +780,117 @@ def test_http_over_https_error( if ip_addr is ANY_INTERFACE_IPV6: fqdn = '[{fqdn}]'.format(**locals()) - expect_fallback_response_over_plain_http = adapter_type == 'pyopenssl' - if expect_fallback_response_over_plain_http: - resp = requests.get( - f'http://{fqdn!s}:{port!s}/', - timeout=http_request_timeout, - ) - assert resp.status_code == 400 - assert resp.text == ( - 'The client sent a plain HTTP request, ' - 'but this server only speaks HTTPS on this port.' - ) - return + resp = requests.get( + f'http://{fqdn!s}:{port!s}/', + timeout=http_request_timeout, + ) + assert resp.status_code == 400 + assert resp.text == ( + 'The client sent a plain HTTP request, ' + 'but this server only speaks HTTPS on this port.' + ) - with pytest.raises(requests.exceptions.ConnectionError) as ssl_err: - requests.get( # FIXME: make stdlib ssl behave like PyOpenSSL - f'http://{fqdn!s}:{port!s}/', - timeout=http_request_timeout, + +@pytest.mark.parametrize( + ('error', 'raising_expectation'), + ( + ( + BrokenPipeError(errno.EPIPE, 'Broken pipe'), + contextlib.nullcontext(), + ), + ( + OSError(9999, 'An error to reckon with'), + pytest.raises(OSError, match=r'An error to reckon with'), + ), + ), + ids=('error-suppressed', 'error-propagates'), +) +def test_send_bad_request_socket_errors( + mocker, + error, + raising_expectation, +): + """Test socket error handling when sending 400 Bad Request.""" + # Mock the selector in ConnectionManager initialization + mocker.patch('cheroot.connections._ThreadsafeSelector', autospec=True) + + mock_server = mocker.Mock(spec=HTTPServer) + mock_server.protocol = 'HTTP/1.1' + mock_server.socket = mocker.Mock(spec=socket.socket) + conn_manager = ConnectionManager(mock_server) + + mock_raw_socket = mocker.Mock(spec=socket.socket) + mock_raw_socket.sendall.side_effect = error + + with raising_expectation as exc_info: + conn_manager._send_bad_request_plain_http_error( + mock_raw_socket, ) - underlying_error = ssl_err.value.args[0].args[-1] - err_text = str(underlying_error) - assert underlying_error.errno == expected_error_code, ( - 'The underlying error is {underlying_error!r}'.format(**locals()) + # If we expect an error, check it's the correct one + should_propagate = not isinstance( + raising_expectation, + contextlib.nullcontext, ) - assert expected_error_text in err_text + if should_propagate: + assert exc_info.value is error + + mock_raw_socket.sendall.assert_called_once() + mock_raw_socket.close.assert_called_once() + + +@pytest.mark.parametrize('adapter_type', ('builtin', 'pyopenssl')) +# pylint: disable-next=too-many-positional-arguments +def test_http_over_https_ssl_handshake( + mocker, + tls_http_server, + adapter_type, + tls_certificate, + tls_certificate_chain_pem_path, + tls_certificate_private_key_pem_path, +): + """ + Test NoSSLError raised when SSL handshake catches HTTP. + + Normally the early probe ``_ensure_peer_speaks_https()`` + will detect a client attempting to speak HTTP on a TLS + port but if this times out or fails for some reason, SSL + should raise an error at the time of the handshake. Here + we test the error is caught and triggers the emission of + a ``400 Bad Request``. + """ + interface, _host, port = _get_conn_data(ANY_INTERFACE_IPV4) + + tls_adapter_cls = get_ssl_adapter_class(name=adapter_type) + tls_adapter = tls_adapter_cls( + tls_certificate_chain_pem_path, + tls_certificate_private_key_pem_path, + ) + + tls_certificate.configure_cert(tls_adapter.context) + + # Mock the early probe to not detect the HTTP request + mocker.patch('cheroot.ssl._ensure_peer_speaks_https', autospec=True) + + tlshttpserver = tls_http_server((interface, port), tls_adapter) + + interface, _host, port = _get_conn_data(tlshttpserver.bind_addr) + + # Send plain HTTP + with socket.create_connection((interface, port)) as sock: + BUFFER_SIZE = 256 + sock.sendall(b'GET / HTTP/1.1\r\nHost: localhost\r\n\r\n') + + if adapter_type == 'pyopenssl': + # pyopenssl can send the 400 response + response = sock.recv(BUFFER_SIZE) + assert b'400 Bad Request' in response + else: + # builtin adapter: connection closed before 400 can be sent + with pytest.raises( + ConnectionError, + ): + sock.recv(1) @pytest.mark.parametrize('adapter_type', ('builtin', 'pyopenssl')) diff --git a/docs/changelog-fragments.d/800.bugfix.1.rst b/docs/changelog-fragments.d/800.bugfix.1.rst new file mode 100644 index 0000000000..8d9683c25c --- /dev/null +++ b/docs/changelog-fragments.d/800.bugfix.1.rst @@ -0,0 +1,5 @@ +Plaintext HTTP error responses on HTTPS ports now reliably send complete +messages to clients using :meth:`~socket.socket.sendall` instead of buffered +writes that may not flush before the connection closes. + +-- by :user:`julianz-` diff --git a/docs/changelog-fragments.d/800.bugfix.2.rst b/docs/changelog-fragments.d/800.bugfix.2.rst new file mode 100644 index 0000000000..496a32be4a --- /dev/null +++ b/docs/changelog-fragments.d/800.bugfix.2.rst @@ -0,0 +1,7 @@ +The :py:mod:`cheroot.ssl.builtin` SSL adapter now responds with a +``400 Bad Request`` error to plaintext HTTP requests on HTTPS ports by detecting +them before the SSL handshake, matching the behavior of +:py:mod:`cheroot.ssl.pyopenssl`. Previously, such requests would fail during +the SSL handshake without sending an HTTP response. + +-- by :user:`julianz-` diff --git a/docs/spelling_wordlist.txt b/docs/spelling_wordlist.txt index 1e1022c82f..6ab6243e0c 100644 --- a/docs/spelling_wordlist.txt +++ b/docs/spelling_wordlist.txt @@ -40,6 +40,7 @@ netloc noqa PIL pipelining +plaintext positionally pre preconfigure