From fb4f82655f37c950428cb8dcb9c4f9adb1fb086e Mon Sep 17 00:00:00 2001 From: julianz- <6255571+julianz-@users.noreply.github.com> Date: Sun, 7 Dec 2025 19:00:23 -0800 Subject: [PATCH] Deprecated `Makefile` in favor of `StreamReader` and `StreamWriter` We replace the `MakeFile` factory function with direct class instantiation. The `MakeFile` function now emits a `DeprecationWarning` and will be removed in a future release. `makefile()` methods are also deprecated on the adapters and its base class. --- .flake8 | 1 + cheroot/connections.py | 6 +- cheroot/makefile.py | 13 +- cheroot/server.py | 55 ++- cheroot/ssl/__init__.py | 26 +- cheroot/ssl/builtin.py | 50 ++- cheroot/ssl/builtin.pyi | 2 - cheroot/ssl/pyopenssl.py | 254 +++++++++--- cheroot/ssl/pyopenssl.pyi | 33 +- cheroot/test/ssl/test_ssl_pyopenssl.py | 365 ++++++++++++++++++ cheroot/test/test_makefile.py | 17 +- cheroot/test/test_server.py | 21 +- cheroot/test/test_ssl.py | 139 ++++++- .../changelog-fragments.d/805.deprecation.rst | 12 + docs/spelling_wordlist.txt | 1 + 15 files changed, 899 insertions(+), 96 deletions(-) create mode 100644 cheroot/test/ssl/test_ssl_pyopenssl.py create mode 100644 docs/changelog-fragments.d/805.deprecation.rst diff --git a/.flake8 b/.flake8 index 6ab1219ead..6ff496bc1a 100644 --- a/.flake8 +++ b/.flake8 @@ -137,6 +137,7 @@ per-file-ignores = cheroot/ssl/pyopenssl.py: C815, DAR101, DAR201, DAR401, I001, I003, I005, N801, N804, RST304, WPS100, WPS110, WPS111, WPS117, WPS120, WPS121, WPS130, WPS210, WPS220, WPS221, WPS225, WPS229, WPS231, WPS238, WPS301, WPS335, WPS338, WPS420, WPS422, WPS430, WPS432, WPS501, WPS504, WPS505, WPS601, WPS608, WPS615 cheroot/test/conftest.py: DAR101, DAR201, DAR301, I001, I003, I005, WPS100, WPS130, WPS325, WPS354, WPS420, WPS422, WPS430, WPS457 cheroot/test/helper.py: DAR101, DAR201, DAR401, I001, I003, I004, N802, WPS110, WPS111, WPS121, WPS201, WPS220, WPS231, WPS301, WPS414, WPS421, WPS422, WPS505 + cheroot/test/ssl/test_ssl_pyopenssl.py: DAR101, DAR201, WPS202, WPS118, WPS204, WPS226, WPS441 cheroot/test/test_cli.py: DAR101, DAR201, I001, I005, N802, S101, S108, WPS110, WPS421, WPS431, WPS473 cheroot/test/test_makefile.py: DAR101, DAR201, I004, RST304, S101, WPS110, WPS122 cheroot/test/test_wsgi.py: DAR101, DAR301, I001, I004, S101, WPS110, WPS111, WPS117, WPS118, WPS121, WPS210, WPS421, WPS430, WPS432, WPS441, WPS509 diff --git a/cheroot/connections.py b/cheroot/connections.py index 98f17e42f5..e9e2af80e5 100644 --- a/cheroot/connections.py +++ b/cheroot/connections.py @@ -11,7 +11,6 @@ from . import errors from ._compat import IS_WINDOWS -from .makefile import MakeFile try: @@ -304,7 +303,6 @@ def _from_server_socket(self, server_socket): # noqa: C901 # FIXME if hasattr(s, 'settimeout'): s.settimeout(self.server.timeout) - mf = MakeFile 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: @@ -327,12 +325,12 @@ def _from_server_socket(self, server_socket): # noqa: C901 # FIXME ) 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 if hasattr(s, 'settimeout'): s.settimeout(self.server.timeout) - conn = self.server.ConnectionClass(self.server, s, mf) + conn = self.server.ConnectionClass(self.server, s) if not isinstance(self.server.bind_addr, (str, bytes)): # optional values diff --git a/cheroot/makefile.py b/cheroot/makefile.py index f5780a1ede..5cf4c787fc 100644 --- a/cheroot/makefile.py +++ b/cheroot/makefile.py @@ -3,6 +3,7 @@ # prefer slower Python-based io module import _pyio as io import socket +from warnings import warn as _warn # Write only 16K at a time to sockets @@ -70,6 +71,16 @@ def write(self, val, *args, **kwargs): def MakeFile(sock, mode='r', bufsize=io.DEFAULT_BUFFER_SIZE): - """File object attached to a socket object.""" + """ + File object attached to a socket object. + + This function is now deprecated: Use + StreamReader or StreamWriter directly. + """ + _warn( + 'MakeFile is deprecated. Use StreamReader or StreamWriter directly.', + DeprecationWarning, + stacklevel=2, + ) cls = StreamReader if 'r' in mode else StreamWriter return cls(sock, mode, bufsize) diff --git a/cheroot/server.py b/cheroot/server.py index 27e9173b15..7343bd93bf 100644 --- a/cheroot/server.py +++ b/cheroot/server.py @@ -84,10 +84,16 @@ from . import __version__, connections, errors from ._compat import IS_PPC, bton -from .makefile import MakeFile, StreamWriter +from .makefile import StreamReader, StreamWriter from .workers import threadpool +try: + from OpenSSL import SSL +except ImportError: + SSL = None # OpenSSL not available + + __all__ = ( 'ChunkedRFile', 'DropUnderscoreHeaderReader', @@ -1276,19 +1282,31 @@ class HTTPConnection: # Fields set by ConnectionManager. last_used = None - def __init__(self, server, sock, makefile=MakeFile): + def __init__(self, server, sock, makefile=None): """Initialize HTTPConnection instance. Args: server (HTTPServer): web server object receiving this request sock (socket._socketobject): the raw socket object (usually TCP) for this connection - makefile (file): a fileobject class for reading from the socket + makefile (file): Now deprecated. + Used to be a fileobject class for reading from the socket. """ self.server = server self.socket = sock - self.rfile = makefile(sock, 'rb', self.rbufsize) - self.wfile = makefile(sock, 'wb', self.wbufsize) + + if makefile is not None: + _warn( + 'The `makefile` parameter in creating an `HTTPConnection` ' + 'is deprecated and will be removed in a future version. ' + 'The connection socket should now be fully wrapped by the ' + 'adapter before being passed to this constructor.', + DeprecationWarning, + stacklevel=2, + ) + + self.rfile = StreamReader(sock, 'rb', self.rbufsize) + self.wfile = StreamWriter(sock, 'wb', self.wbufsize) self.requests_seen = 0 self.peercreds_enabled = self.server.peercreds_enabled @@ -1358,12 +1376,8 @@ def communicate(self): # noqa: C901 # FIXME def _handle_no_ssl(self, req): if not req or req.sent_headers: return - # Unwrap wfile - try: - resp_sock = self.socket._sock - except AttributeError: - # self.socket is of OpenSSL.SSL.Connection type - resp_sock = self.socket._socket + # Unwrap to get raw TCP socket + resp_sock = self.socket._sock self.wfile = StreamWriter(resp_sock, 'wb', self.wbufsize) msg = ( 'The client sent a plain HTTP request, but ' @@ -1507,20 +1521,23 @@ def peer_group(self): def _close_kernel_socket(self): """Terminate the connection at the transport level.""" - # Honor ``sock_shutdown`` for PyOpenSSL connections. - shutdown = getattr( - self.socket, - 'sock_shutdown', - self.socket.shutdown, - ) - try: - shutdown(socket.SHUT_RDWR) # actually send a TCP FIN + self.socket.shutdown(socket.SHUT_RDWR) except errors.acceptable_sock_shutdown_exceptions: pass except socket.error as e: if e.errno not in errors.acceptable_sock_shutdown_error_codes: raise + except Exception as exc: + # translate SSL exceptions to FatalSSLAlert + if SSL is not None and isinstance( + exc, + (SSL.Error, SSL.SysCallError), + ): + raise errors.FatalSSLAlert( + 'TLS/SSL connection failure', + ) from exc + raise # re-raise everything else class HTTPServer: diff --git a/cheroot/ssl/__init__.py b/cheroot/ssl/__init__.py index 458bdcc3d6..a9e369c23c 100644 --- a/cheroot/ssl/__init__.py +++ b/cheroot/ssl/__init__.py @@ -61,8 +61,21 @@ class Adapter(ABC): Required methods: * ``wrap(sock) -> (wrapped socket, ssl environ dict)`` - * ``makefile(sock, mode='r', bufsize=DEFAULT_BUFFER_SIZE) -> - socket file object`` + * ``get_environ() -> (ssl environ dict)`` + + Optional methods: + + * ``makefile(sock, mode='r', bufsize=-1) -> socket file object`` + + This method is deprecated and will be removed in a future release. + + Historically, the ``PyOpenSSL`` adapter used ``makefile()`` to + wrap the underlying socket in an ``OpenSSL``-aware file object + so that Cheroot's HTTP request parser (which expects file-like I/O + such as ``readline()``) could read from TLS connections. The + adapter now fully wraps the socket in a TLSSocket object that + provides the necessary socket and file-like + methods directly, so ``makefile()`` is no longer needed. """ @abstractmethod @@ -110,9 +123,14 @@ def get_environ(self): """Return WSGI environ entries to be merged into each request.""" raise NotImplementedError # pragma: no cover - @abstractmethod def makefile(self, sock, mode='r', bufsize=-1): - """Return socket file object.""" + """ + Return socket file object. + + This method is now deprecated. It will be removed in a future version. + + :raises NotImplementedError: Must be overridden by subclasses. + """ raise NotImplementedError # pragma: no cover def _prompt_for_tls_password(self) -> str: diff --git a/cheroot/ssl/builtin.py b/cheroot/ssl/builtin.py index b7fed72b21..3e1b32662d 100644 --- a/cheroot/ssl/builtin.py +++ b/cheroot/ssl/builtin.py @@ -11,6 +11,7 @@ import sys import threading from contextlib import suppress +from warnings import warn as _warn try: @@ -18,20 +19,31 @@ except ImportError: ssl = None -try: - from _pyio import DEFAULT_BUFFER_SIZE -except ImportError: - try: - from io import DEFAULT_BUFFER_SIZE - except ImportError: - DEFAULT_BUFFER_SIZE = -1 - from .. import errors -from ..makefile import StreamReader, StreamWriter from ..server import HTTPServer from . import Adapter +# WPS413 is to suppress linter error: +# bad magic module function: __getattr__ +# DEFAULT_BUFFER_SIZE is deprecated so this module method will be removed +# in a future release +def __getattr__(name): # noqa: WPS413 + if name == 'DEFAULT_BUFFER_SIZE': + _warn( + ( + '`DEFAULT_BUFFER_SIZE` is deprecated and ' + 'will be removed in a future release.' + ), + DeprecationWarning, + stacklevel=2, + ) + from io import DEFAULT_BUFFER_SIZE + + return DEFAULT_BUFFER_SIZE + raise AttributeError(f'module {__name__!r} has no attribute {name!r}') + + def _assert_ssl_exc_contains(exc, *msgs): """Check whether SSL exception contains either of messages provided.""" if len(msgs) < 1: @@ -496,7 +508,19 @@ def _make_env_dn_dict(self, env_prefix, cert_value): env['%s_%s_%i' % (env_prefix, attr_code, i)] = val return env - def makefile(self, sock, mode='r', bufsize=DEFAULT_BUFFER_SIZE): - """Return socket file object.""" - cls = StreamReader if 'r' in mode else StreamWriter - return cls(sock, mode, bufsize) + def makefile(self, sock, mode='r', bufsize=-1): + """ + Return socket file object. + + ``makefile`` is now deprecated and will be removed in a future + version. + """ + _warn( + 'The `makefile` method is deprecated and will be removed in a future version. ' + 'The connection socket should be fully wrapped by the adapter ' + 'before being passed to the HTTPConnection constructor.', + DeprecationWarning, + stacklevel=2, + ) + + return sock.makefile(mode, bufsize) diff --git a/cheroot/ssl/builtin.pyi b/cheroot/ssl/builtin.pyi index 4032a326e6..126a1790a2 100644 --- a/cheroot/ssl/builtin.pyi +++ b/cheroot/ssl/builtin.pyi @@ -3,8 +3,6 @@ import typing as _t from . import Adapter -DEFAULT_BUFFER_SIZE: int - class BuiltinSSLAdapter(Adapter): CERT_KEY_TO_ENV: _t.Any CERT_KEY_TO_LDAP_CODE: _t.Any diff --git a/cheroot/ssl/pyopenssl.py b/cheroot/ssl/pyopenssl.py index 6cf700341a..b4513401c0 100644 --- a/cheroot/ssl/pyopenssl.py +++ b/cheroot/ssl/pyopenssl.py @@ -59,7 +59,10 @@ try: import OpenSSL.version - from OpenSSL import SSL, crypto + from OpenSSL import ( + SSL, + crypto, + ) try: ssl_conn_type = SSL.Connection @@ -67,6 +70,7 @@ ssl_conn_type = SSL.ConnectionType except ImportError: SSL = None + ssl_conn_type = type(None) import contextlib @@ -106,15 +110,15 @@ def _safe_call(self, is_reader, call, *args, **kwargs): # noqa: C901 time.sleep(self.ssl_retry) except SSL.SysCallError as e: if is_reader and e.args == (-1, 'Unexpected EOF'): - return b'' + return 0 errnum = e.args[0] if is_reader and errnum in errors.socket_errors_to_ignore: - return b'' + return 0 raise socket.error(errnum) except SSL.Error as e: if is_reader and e.args == (-1, 'Unexpected EOF'): - return b'' + return 0 thirdarg = None with contextlib.suppress(IndexError): @@ -129,26 +133,6 @@ def _safe_call(self, is_reader, call, *args, **kwargs): # noqa: C901 if time.time() - start > self.ssl_timeout: raise socket.timeout('timed out') - def recv(self, size): - """Receive message of a size from the socket.""" - return self._safe_call( - True, - super(SSLFileobjectMixin, self).recv, - size, - ) - - def readline(self, size=-1): - """Receive message of a size from the socket. - - Matches the following interface: - https://docs.python.org/3/library/io.html#io.IOBase.readline - """ - return self._safe_call( - True, - super(SSLFileobjectMixin, self).readline, - size, - ) - def sendall(self, *args, **kwargs): """Send whole message to the socket.""" return self._safe_call( @@ -169,11 +153,43 @@ def send(self, *args, **kwargs): class SSLFileobjectStreamReader(SSLFileobjectMixin, StreamReader): - """SSL file object attached to a socket object.""" + """ + SSL file object attached to a socket object. + + .. deprecated::11.2 + This class is deprecated and will be removed in a future release. + """ + + def __init__(self, *args, **kwargs): + """Initialize SSLFileobjectStreamReader.""" + _warn( + '`SSLFileobjectStreamReader` and `SSLFileobjectStreamWriter` ' + 'are deprecated. The `pyOpenSSL` adapter now returns `TLSSocket` ' + 'which works directly with StreamReader/StreamWriter.', + DeprecationWarning, + stacklevel=2, + ) + super().__init__(*args, **kwargs) class SSLFileobjectStreamWriter(SSLFileobjectMixin, StreamWriter): - """SSL file object attached to a socket object.""" + """ + SSL file object attached to a socket object. + + .. deprecated::11.2 + This class is deprecated and will be removed in a future release. + """ + + def __init__(self, *args, **kwargs): + """Initialize SSLFileobjectStreamWriter.""" + _warn( + '`SSLFileobjectStreamReader` and `SSLFileobjectStreamWriter` ' + 'are deprecated. The `pyOpenSSL` adapter now returns `TLSSocket` ' + 'which works directly with StreamReader/StreamWriter.', + DeprecationWarning, + stacklevel=2, + ) + super().__init__(*args, **kwargs) class SSLConnectionProxyMeta: @@ -268,7 +284,7 @@ class SSLConnection(metaclass=SSLConnectionProxyMeta): """ def __init__(self, *args): - """Initialize SSLConnection instance.""" + """Initialize ``SSLConnection`` instance.""" self._ssl_conn = SSL.Connection(*args) self._lock = threading.RLock() @@ -319,7 +335,7 @@ def __init__( *, private_key_password=None, ): - """Initialize OpenSSL Adapter instance.""" + """Initialize ``pyOpenSSLAdapter`` instance.""" if SSL is None: raise ImportError('You must install pyOpenSSL to use HTTPS.') @@ -345,9 +361,11 @@ def wrap(self, sock): ) conn = SSLConnection(self.context, sock) - conn.set_accept_state() # Tell OpenSSL this is a server connection - return conn, self._environ.copy() + + # Wrap the SSLConnection to provide standard socket interface + tls_socket = _TLSSocket(underlying_socket=sock, tls_connection=conn) + return tls_socket, self._environ.copy() def _password_callback( self, @@ -461,17 +479,167 @@ def get_environ(self): return ssl_environ def makefile(self, sock, mode='r', bufsize=-1): - """Return socket file object.""" - cls = ( - SSLFileobjectStreamReader - if 'r' in mode - else SSLFileobjectStreamWriter + """ + Return socket file object. + + ``makefile`` is now deprecated and will be removed in a future + version. + """ + _warn( + 'The `makefile` method is deprecated and will be removed in a future version. ' + 'The connection socket should be fully wrapped by the adapter ' + 'before being passed to the HTTPConnection constructor.', + DeprecationWarning, + stacklevel=2, ) - # sock is an pyopenSSL.SSLConnection instance here - if SSL and isinstance(sock, SSLConnection): - wrapped_socket = cls(sock, mode, bufsize) - wrapped_socket.ssl_timeout = sock.gettimeout() - return wrapped_socket - # This is from past: - # TODO: figure out what it's meant for - return cheroot_server.CP_fileobject(sock, mode, bufsize) + + return sock.makefile(mode, bufsize) + + +class _TLSSocket(SSLFileobjectMixin): + """ + Wrap :py:class:`SSL.Connection `. + + Wrapping with ``_TLSSocket`` makes it possible for an ``SSL.Connection`` + to work with :py:class:`~cheroot.makefile.StreamReader`/\ + :py:class:`~cheroot.makefile.StreamWriter`. + + ``_TLSSocket`` handles OpenSSL-specific errors by either + suppressing them if they if they are acceptable during cleanup + (e.g., "shutdown while in init", "uninitialized") or converting them to + standard socket exceptions or Cheroot-specific errors (\ + :py:exc:`~cheroot.errors.FatalSSLAlert`, + :py:exc:`~cheroot.errors.NoSSLError`) \ + for error handling by the calling I/O classes. + """ + + def __init__( + self, + underlying_socket, + tls_connection, + ssl_timeout=None, + ssl_retry=0.01, + ): + """Initialize with an ``SSL.Connection`` object.""" + self._ssl_conn = tls_connection + self._sock = underlying_socket # Store reference to raw TCP socket + self._lock = threading.RLock() + self.ssl_timeout = ssl_timeout or 3.0 + self.ssl_retry = ssl_retry + + # Socket I/O + # _safe_call is delegated to _TLSSockettMixin + + def recv_into(self, buffer, nbytes=None): + """Receive data into a buffer.""" + with self._lock: + return self._safe_call( + True, + self._ssl_conn.recv_into, + buffer, + nbytes, + ) + + def send(self, data): + """Send data.""" + with self._lock: + return self._safe_call( + False, # is_reader=False + self._ssl_conn.send, + data, + ) + + def fileno(self): + """Return the file descriptor.""" + return self._ssl_conn.fileno() + + def _decref_socketios(self): + """Decrement reference count for socket I/O streams. + + No-op for ``_TLSSocket`` since we don't track reference counts from + :py:meth:`socket.socket.makefile() `. + The method is needed for compatibility with ``socket.SocketIO``, + which is used by :py:class:`~cheroot.makefile.StreamReader` and + :py:class:`~cheroot.makefile.StreamWriter`. + """ + + def shutdown(self, how): + """Shutdown the connection. + + This is a no-op because actually for TLS sockets, + true shutdown is handled by ``close()`` to ensure + proper ordering (SSL shutdown before TCP shutdown). + This method is kept for interface compatibility. + """ + + # C901 close is too complex + def close(self): # noqa: C901 + """Close the TLS socket and underlying connection.""" + exceptions = [] + + # SSL errors that are acceptable during shutdown + ACCEPTABLE_SSL_SHUTDOWN_ERRORS = { + # Shutdown before handshake completed + 'shutdown while in init', + 'uninitialized', + } + + acceptable_codes = errors.acceptable_sock_shutdown_error_codes + + # 1. Attempt an SSL-level shutdown + try: + self._ssl_conn.shutdown() + except SSL.Error as e: + # Many SSL shutdown errors expected when peer has already closed + # SSL.ZeroReturnError means clean shutdown + if isinstance(e, SSL.ZeroReturnError): + pass # Clean shutdown, not an error + elif isinstance(e, SSL.SysCallError): + # Check if it's a syscall error with an acceptable errno + if e.args: + errno_code = e.args[0] + if errno_code not in acceptable_codes: + exceptions.append(e) + else: + # Check the OpenSSL error reason code + error_reason = None + if hasattr(e, '_reason_code'): + error_reason = e._reason_code + elif e.args: + # PyOpenSSL Error format: ([('SSL routines', '', 'reason_string')],) + # Note: e.args[0] is a LIST of tuples, not a tuple itself + error_list = e.args[0] if e.args else [] + for err_tuple in error_list: + if ( + isinstance(err_tuple, tuple) + and len(err_tuple) >= 3 + ): + error_reason = err_tuple[2] + break + + if error_reason not in ACCEPTABLE_SSL_SHUTDOWN_ERRORS: + exceptions.append(e) + except OSError as e: + if e.errno not in acceptable_codes: + exceptions.append(e) + + # 2. Close the raw TCP socket + try: + self._sock.close() + except OSError as e: + if e.errno not in acceptable_codes: + exceptions.append(e) + + # Re-raise collected exceptions as Cheroot-compatible errors + if exceptions: + if len(exceptions) == 1: + exc = exceptions[0] + raise errors.FatalSSLAlert( + f'Error during TLS socket close: {type(exc).__name__}: {exc}', + ) from exc + # Multiple errors - combine into single message + error_msgs = [f'{type(e).__name__}: {e}' for e in exceptions] + combined_errors = '; '.join(error_msgs) + raise errors.FatalSSLAlert( + f'Multiple errors during close: {combined_errors}', + ) from exceptions[0] diff --git a/cheroot/ssl/pyopenssl.pyi b/cheroot/ssl/pyopenssl.pyi index 6ca8d09eca..565eca0444 100644 --- a/cheroot/ssl/pyopenssl.pyi +++ b/cheroot/ssl/pyopenssl.pyi @@ -1,4 +1,6 @@ import collections.abc as _c +import io +import threading import typing as _t from OpenSSL import SSL @@ -9,10 +11,8 @@ from . import Adapter ssl_conn_type: _t.Type[SSL.Connection] class SSLFileobjectMixin: - ssl_timeout: int + ssl_timeout: float ssl_retry: float - def recv(self, size): ... - def readline(self, size: int = ...): ... def sendall(self, *args, **kwargs): ... def send(self, *args, **kwargs): ... @@ -50,5 +50,30 @@ class pyOpenSSLAdapter(Adapter): /, ) -> bytes: ... def get_environ(self): ... - def makefile(self, sock, mode: str = ..., bufsize: int = ...): ... def get_context(self) -> SSL.Context: ... + def makefile(self, sock, mode: str = ..., bufsize: int = ...): ... + +class _TLSSocket(SSLFileobjectMixin, io.RawIOBase): + _ssl_conn: _t.Any + _sock: _t.Any + _lock: threading.RLock + ssl_timeout: float + ssl_retry: float + + def __init__( + self, + underlying_socket: _t.Any, + tls_connection: _t.Any, + ssl_timeout: float | None = None, + ssl_retry: float | None = 0.01, + ) -> None: ... + def recv_into( + self, + buffer: bytes, + nbytes: float | None = None, + ) -> int: ... + def send(self, data: bytes) -> int: ... + def fileno(self) -> int: ... + def _decref_socketios(self) -> _t.Any: ... + def shutdown(self, how: int) -> None: ... + def close(self) -> None: ... diff --git a/cheroot/test/ssl/test_ssl_pyopenssl.py b/cheroot/test/ssl/test_ssl_pyopenssl.py new file mode 100644 index 0000000000..21e8ab05d2 --- /dev/null +++ b/cheroot/test/ssl/test_ssl_pyopenssl.py @@ -0,0 +1,365 @@ +"""Tests for `_TLSSocket`.""" + +import errno +import socket + +import pytest + +from OpenSSL import SSL + +from cheroot import errors +from cheroot.ssl.pyopenssl import _TLSSocket + + +@pytest.fixture +def acceptable_codes(): + """Mock acceptable error codes.""" + return {errno.ECONNRESET, errno.EPIPE, errno.ENOTCONN} + + +@pytest.fixture +def mock_tls_socket(mocker, acceptable_codes): + """Create a ``_TLSSocket`` with mocked dependencies.""" + # Mock the acceptable_sock_shutdown_error_codes + mocker.patch.object( + errors, + 'acceptable_sock_shutdown_error_codes', + acceptable_codes, + ) + + socket = _TLSSocket.__new__(_TLSSocket) + socket._ssl_conn = mocker.create_autospec(SSL.Connection, instance=True) + socket._sock = mocker.Mock() + return socket + + +# ===== SUCCESSFUL SHUTDOWN TESTS ===== + + +def test_close_while_in_init(): + """Test that 'close while in init' errors are handled gracefully.""" + # Create a connection that will error on shutdown + context = SSL.Context(SSL.TLSv1_2_METHOD) + + # Create an unconnected SSL connection + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + ssl_conn = SSL.Connection(context, sock) + + ssl_adapter = _TLSSocket(sock, ssl_conn) + + # close() should NOT raise an exception for 'uninitialized' + # or 'shutdown while in init' + ssl_adapter.close() + + +def test_close_clean_shutdown(mock_tls_socket): + """Test successful close with no errors.""" + mock_tls_socket._ssl_conn.shutdown.return_value = None + mock_tls_socket._sock.close.return_value = None + + # Should not raise any exceptions + mock_tls_socket.close() + + mock_tls_socket._ssl_conn.shutdown.assert_called_once() + mock_tls_socket._sock.close.assert_called_once() + + +def test_close_with_zero_return_error(mock_tls_socket): + """Test close handles ``SSL.ZeroReturnError`` (clean shutdown).""" + mock_tls_socket._ssl_conn.shutdown.side_effect = SSL.ZeroReturnError() + mock_tls_socket._sock.close.return_value = None + + # Should not raise - ZeroReturnError is acceptable + mock_tls_socket.close() + + mock_tls_socket._sock.close.assert_called_once() + + +# ===== ACCEPTABLE SSL SHUTDOWN ERRORS ===== + + +@pytest.mark.parametrize( + 'error_reason', + ( + 'shutdown while in init', + 'uninitialized', + ), +) +def test_close_with_acceptable_ssl_errors(mock_tls_socket, error_reason): + """Test close handles acceptable SSL shutdown errors.""" + error = SSL.Error() + error.args = [[('SSL routines', '', error_reason)]] + mock_tls_socket._ssl_conn.shutdown.side_effect = error + mock_tls_socket._sock.close.return_value = None + + # Should not raise - acceptable error + mock_tls_socket.close() + + mock_tls_socket._sock.close.assert_called_once() + + +def test_close_with_reason_code_attribute(mock_tls_socket): + """Test SSL error with _reason_code attribute.""" + error = SSL.Error() + error._reason_code = 'shutdown while in init' + mock_tls_socket._ssl_conn.shutdown.side_effect = error + mock_tls_socket._sock.close.return_value = None + + # Should not raise + mock_tls_socket.close() + + mock_tls_socket._sock.close.assert_called_once() + + +# ===== SYSCALL ERROR TESTS ===== + + +@pytest.mark.parametrize( + 'errno_code', + ( + errno.ECONNRESET, + errno.EPIPE, + errno.ENOTCONN, + ), +) +def test_close_with_acceptable_syscall_errors(mock_tls_socket, errno_code): + """Test close handles ``SysCallError`` with acceptable errno.""" + error = SSL.SysCallError(errno_code, f'Error {errno_code}') + mock_tls_socket._ssl_conn.shutdown.side_effect = error + mock_tls_socket._sock.close.return_value = None + + # Should not raise - errno is acceptable + mock_tls_socket.close() + + mock_tls_socket._sock.close.assert_called_once() + + +def test_close_with_unacceptable_syscall_error(mock_tls_socket): + """Test close raises on ``SysCallError`` with unacceptable errno.""" + error = SSL.SysCallError(errno.EBADF, 'Bad file descriptor') + mock_tls_socket._ssl_conn.shutdown.side_effect = error + mock_tls_socket._sock.close.return_value = None + + with pytest.raises(errors.FatalSSLAlert) as exc_info: + mock_tls_socket.close() + + assert 'SysCallError' in str(exc_info.value) + assert 'Bad file descriptor' in str(exc_info.value) + + +def test_close_with_syscall_error_no_args(mock_tls_socket): + """Test ``SysCallError`` with no args.""" + error = SSL.SysCallError(-1, 'Error with errno -1') + error.args = [-1] + mock_tls_socket._ssl_conn.shutdown.side_effect = error + mock_tls_socket._sock.close.return_value = None + + # No errno to check, should raise + with pytest.raises(errors.FatalSSLAlert): + mock_tls_socket.close() + + +# ===== UNACCEPTABLE SSL ERRORS ===== + + +def test_close_with_unacceptable_ssl_error(mock_tls_socket): + """Test close raises on unacceptable SSL error.""" + error = SSL.Error() + error.args = [[('SSL routines', '', 'some other error')]] + mock_tls_socket._ssl_conn.shutdown.side_effect = error + mock_tls_socket._sock.close.return_value = None + + with pytest.raises(errors.FatalSSLAlert) as exc_info: + mock_tls_socket.close() + + assert 'Error during TLS socket close' in str(exc_info.value) + + +def test_close_with_ssl_error_empty_args(mock_tls_socket): + """Test ``SSL.Error`` with empty args.""" + error = SSL.Error() + error.args = [] + mock_tls_socket._ssl_conn.shutdown.side_effect = error + mock_tls_socket._sock.close.return_value = None + + # No error reason, should raise + with pytest.raises(errors.FatalSSLAlert): + mock_tls_socket.close() + + +def test_close_with_ssl_error_malformed_args(mock_tls_socket): + """Test ``SSL.Error`` with malformed args (not tuple or wrong length).""" + error = SSL.Error() + error.args = [['not', 'a', 'tuple']] + mock_tls_socket._ssl_conn.shutdown.side_effect = error + mock_tls_socket._sock.close.return_value = None + + # Malformed args, should raise + with pytest.raises(errors.FatalSSLAlert): + mock_tls_socket.close() + + +def test_close_with_ssl_error_short_tuple(mock_tls_socket): + """Test ``SSL.Error`` with tuple that's too short.""" + error = SSL.Error() + error.args = [[('SSL routines', 'only two')]] + mock_tls_socket._ssl_conn.shutdown.side_effect = error + mock_tls_socket._sock.close.return_value = None + + # Tuple too short, should raise + with pytest.raises(errors.FatalSSLAlert): + mock_tls_socket.close() + + +# ===== SOCKET CLOSE ERRORS ===== + + +@pytest.mark.parametrize( + 'errno_code', + ( + errno.ECONNRESET, + errno.EPIPE, + ), +) +def test_close_with_acceptable_socket_errors(mock_tls_socket, errno_code): + """Test close handles socket ``OSError`` with acceptable errno.""" + mock_tls_socket._ssl_conn.shutdown.return_value = None + socket_error = OSError(errno_code, f'Socket error {errno_code}') + mock_tls_socket._sock.close.side_effect = socket_error + + # Should not raise + mock_tls_socket.close() + + +def test_close_with_unacceptable_socket_error(mock_tls_socket): + """Test close raises on socket ``OSError`` with unacceptable errno.""" + mock_tls_socket._ssl_conn.shutdown.return_value = None + socket_error = OSError(errno.EBADF, 'Bad file descriptor') + mock_tls_socket._sock.close.side_effect = socket_error + + with pytest.raises(errors.FatalSSLAlert) as exc_info: + mock_tls_socket.close() + + assert 'OSError' in str(exc_info.value) + + +def test_close_with_ssl_shutdown_oserror_unacceptable(mock_tls_socket): + """Test close handles unacceptable ``OSError`` during SSL shutdown.""" + error = OSError(errno.EBADF, 'Bad file descriptor') + mock_tls_socket._ssl_conn.shutdown.side_effect = error + mock_tls_socket._sock.close.return_value = None + + with pytest.raises(errors.FatalSSLAlert) as exc_info: + mock_tls_socket.close() + + assert 'OSError' in str(exc_info.value) + + +def test_close_with_ssl_shutdown_oserror_acceptable(mock_tls_socket): + """Test close handles acceptable ``OSError`` during SSL shutdown.""" + error = OSError(errno.ECONNRESET, 'Connection reset') + mock_tls_socket._ssl_conn.shutdown.side_effect = error + mock_tls_socket._sock.close.return_value = None + + # Should not raise - acceptable error + mock_tls_socket.close() + + mock_tls_socket._sock.close.assert_called_once() + + +# ===== MULTIPLE ERRORS ===== + + +def test_close_with_multiple_errors(mock_tls_socket): + """Test close with errors in both SSL shutdown and socket close.""" + ssl_error = SSL.Error() + ssl_error.args = [[('SSL routines', '', 'bad error')]] + mock_tls_socket._ssl_conn.shutdown.side_effect = ssl_error + + socket_error = OSError(errno.EBADF, 'Bad file descriptor') + mock_tls_socket._sock.close.side_effect = socket_error + + with pytest.raises(errors.FatalSSLAlert) as exc_info: + mock_tls_socket.close() + + error_msg = str(exc_info.value) + assert 'Multiple errors during close' in error_msg + assert 'Error' in error_msg # SSL.Error + assert 'OSError' in error_msg + + +def test_close_ssl_error_then_acceptable_socket_error(mock_tls_socket): + """Test SSL error followed by acceptable socket error.""" + ssl_error = SSL.Error() + ssl_error.args = [[('SSL routines', '', 'bad error')]] + mock_tls_socket._ssl_conn.shutdown.side_effect = ssl_error + + socket_error = OSError(errno.ECONNRESET, 'Connection reset') + mock_tls_socket._sock.close.side_effect = socket_error + + # Only SSL error should be raised (socket error is acceptable) + with pytest.raises(errors.FatalSSLAlert) as exc_info: + mock_tls_socket.close() + + error_msg = str(exc_info.value) + assert 'Multiple errors' not in error_msg + assert 'Error during TLS socket close' in error_msg + + +# ===== EDGE CASES ===== + + +def test_close_with_multiple_error_tuples(mock_tls_socket): + """Test ``SSL.Error`` with multiple error tuples (uses first one).""" + error = SSL.Error() + error.args = [ + [ + ('invalid', 'tuple'), + ('SSL routines', '', 'shutdown while in init'), + ('SSL routines', '', 'another error'), + ], + ] + mock_tls_socket._ssl_conn.shutdown.side_effect = error + mock_tls_socket._sock.close.return_value = None + + # Should use first valid error reason + mock_tls_socket.close() + + mock_tls_socket._sock.close.assert_called_once() + + +def test_close_ssl_error_none_errno(mock_tls_socket): + """Test ``SysCallError`` with None as errno.""" + error = SSL.SysCallError(None, 'No errno') + mock_tls_socket._ssl_conn.shutdown.side_effect = error + mock_tls_socket._sock.close.return_value = None + + # None not in acceptable codes, should raise + with pytest.raises(errors.FatalSSLAlert): + mock_tls_socket.close() + + +def test_close_ensures_socket_close_even_on_ssl_error(mock_tls_socket): + """Test that socket close is attempted even if SSL shutdown fails.""" + ssl_error = SSL.Error() + ssl_error.args = [[('SSL routines', '', 'bad error')]] + mock_tls_socket._ssl_conn.shutdown.side_effect = ssl_error + mock_tls_socket._sock.close.return_value = None + + with pytest.raises(errors.FatalSSLAlert): + mock_tls_socket.close() + + # Verify socket close was still attempted + mock_tls_socket._sock.close.assert_called_once() + + +def test_close_with_both_acceptable_errors(mock_tls_socket): + """Test close with acceptable errors in both SSL and socket.""" + ssl_error = SSL.SysCallError(errno.ECONNRESET, 'Connection reset') + mock_tls_socket._ssl_conn.shutdown.side_effect = ssl_error + + socket_error = OSError(errno.EPIPE, 'Broken pipe') + mock_tls_socket._sock.close.side_effect = socket_error + + # Should not raise - both errors are acceptable + mock_tls_socket.close() diff --git a/cheroot/test/test_makefile.py b/cheroot/test/test_makefile.py index d65d4ea268..1dda012bda 100644 --- a/cheroot/test/test_makefile.py +++ b/cheroot/test/test_makefile.py @@ -1,5 +1,7 @@ """Tests for :py:mod:`cheroot.makefile`.""" +import pytest + from cheroot import makefile @@ -39,7 +41,7 @@ def test_bytes_read(): """Reader should capture bytes read.""" sock = MockSocket() sock.messages.append(b'foo') - rfile = makefile.MakeFile(sock, 'r') + rfile = makefile.StreamReader(sock, 'r') rfile.read() assert rfile.bytes_read == 3 @@ -48,6 +50,17 @@ def test_bytes_written(): """Writer should capture bytes written.""" sock = MockSocket() sock.messages.append(b'foo') - wfile = makefile.MakeFile(sock, 'w') + wfile = makefile.StreamWriter(sock, 'w') wfile.write(b'bar') assert wfile.bytes_written == 3 + + +def test_makefile_deprecated(): + """MakeFile function should emit a deprecation warning.""" + sock = MockSocket() + sock.messages.append(b'foo') + + expected_message = r'MakeFile.*deprecated' + + with pytest.deprecated_call(match=expected_message): + makefile.MakeFile(sock, 'r') diff --git a/cheroot/test/test_server.py b/cheroot/test/test_server.py index ae6e390a44..b0dd5ca12a 100644 --- a/cheroot/test/test_server.py +++ b/cheroot/test/test_server.py @@ -17,10 +17,12 @@ import requests import requests_unixsocket +from OpenSSL import SSL from pypytools.gc.custom import DefaultGc from .._compat import IS_LINUX, IS_MACOS, IS_WINDOWS, SYS_PLATFORM, bton, ntob -from ..server import IS_UID_GID_RESOLVABLE, Gateway, HTTPServer +from ..errors import FatalSSLAlert +from ..server import IS_UID_GID_RESOLVABLE, Gateway, HTTPConnection, HTTPServer from ..testing import ( ANY_INTERFACE_IPV4, ANY_INTERFACE_IPV6, @@ -626,3 +628,20 @@ def test_overload_thread_does_not_leak(): # We use special exit code to indicate success, rather than normal zero, so # the test doesn't acidentally pass: assert process.returncode == SUCCESSFUL_SUBPROCESS_EXIT + + +def test_close_kernel_socket_ssl_error_raises_fatal_alert(mocker): + """Test ``HTTPConnection`` handles ``FatalSSLAlert``.""" + # Create an HTTPConnection instance + conn = HTTPConnection(server=mocker.Mock(), sock=mocker.Mock()) + + # Mock the socket's shutdown to raise SSL.Error + ssl_error = SSL.Error() + ssl_error.args = [[('SSL routines', '', 'some error')]] + conn.socket.shutdown.side_effect = ssl_error + + with pytest.raises(FatalSSLAlert) as exc_info: + conn._close_kernel_socket() + + assert 'TLS/SSL connection failure' in str(exc_info.value) + assert exc_info.value.__cause__ is ssl_error diff --git a/cheroot/test/test_ssl.py b/cheroot/test/test_ssl.py index 1859e6bc28..64c5fac04b 100644 --- a/cheroot/test/test_ssl.py +++ b/cheroot/test/test_ssl.py @@ -4,6 +4,7 @@ import errno import functools import http.client +import io import json import os import socket @@ -13,6 +14,7 @@ import threading import time import traceback +from contextlib import suppress as _suppress import pytest @@ -27,8 +29,18 @@ load_pem_private_key, ) +import cheroot.errors as _errors from cheroot.connections import ConnectionManager -from cheroot.ssl import Adapter, _ensure_peer_speaks_https +from cheroot.server import HTTPConnection +from cheroot.ssl import ( + Adapter, + _ensure_peer_speaks_https, + builtin as builtin_adapter, +) +from cheroot.ssl.pyopenssl import ( + SSLFileobjectStreamReader, + SSLFileobjectStreamWriter, +) from .._compat import ( IS_ABOVE_OPENSSL10, @@ -130,6 +142,24 @@ def respond(self): return super(HelloWorldGateway, self).respond() +def _stop_server_safely(httpserver): + """ + Stop the HTTP server safely during test tear-down. + + This function is only needed in tests because some test clients intentionally + misbehave — for example, sending plain HTTP to an HTTPS port or disconnecting + before the TLS handshake completes. In these cases, Cheroot raises + FatalSSLAlert during shutdown. + + In production, we don't swallow these errors, as they indicate + protocol violations which we want to monitor. This function ensures + that tests do not fail due to expected pre-handshake shutdown errors + while leaving production behavior unchanged. + """ + with _suppress(_errors.FatalSSLAlert): + httpserver.stop() + + def make_tls_http_server(bind_addr, ssl_adapter, request): """Create and start an HTTP server bound to ``bind_addr``.""" httpserver = HTTPServer( @@ -144,7 +174,7 @@ def make_tls_http_server(bind_addr, ssl_adapter, request): while not httpserver.ready: time.sleep(0.1) - request.addfinalizer(httpserver.stop) + request.addfinalizer(lambda: _stop_server_safely(httpserver)) return httpserver @@ -1127,7 +1157,7 @@ def test_prepare_socket_emits_deprecation_warning( """ Test ``prepare_socket()`` deprecated argument triggers a warning. - ``ssl_adapter`` has been deprecated in ``prepare_socket()``. + ``ssl_adapter`` has been deprecated in ``HTTPServer.prepare_socket()``. """ # Required parameters for prepare_socket (standard IPv4 TCP config) bind_addr = ('127.0.0.1', 8080) @@ -1154,3 +1184,106 @@ def test_prepare_socket_emits_deprecation_warning( assert sock.fileno() > 0 sock.close() + + +def test_httpconnection_makefile_deprecation(mocker): + """ + Test ``makefile`` argument on ``HTTPConnection`` triggers a warning. + + ``makefile`` is now deprecated. + """ + dummy_server = mocker.create_autospec(HTTPServer, instance=True) + dummy_sock = mocker.create_autospec(socket.socket, instance=True) + + # Value for the deprecated 'makefile' parameter + dummy_makefile_value = object() + + expected_message = r'makefile.*deprecated' + + with pytest.deprecated_call(match=expected_message): + # Instantiate HTTPConnection, passing the deprecated 'makefile' + conn = HTTPConnection( + server=dummy_server, + sock=dummy_sock, + makefile=dummy_makefile_value, # This triggers the warning + ) + + assert conn.server is dummy_server + assert conn.socket is dummy_sock + + +@pytest.mark.parametrize( + 'adapter_type', + ( + 'builtin', + 'pyopenssl', + ), +) +def test_adapter_makefile_deprecation( + mocker, + adapter_type, + tls_certificate_chain_pem_path, + tls_certificate_private_key_pem_path, +): + """Test the adapter's makefile() method emits a deprecation warning.""" + 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, + ) + + # Create a mock socket with a makefile method + dummy_sock = mocker.Mock() + mock_file_stream = mocker.Mock(spec=io.FileIO) + dummy_sock.makefile.return_value = mock_file_stream + + expected_message = r'makefile.*deprecated' + + with pytest.deprecated_call(match=expected_message): + result = tls_adapter.makefile(dummy_sock, mode='r', bufsize=8192) + + dummy_sock.makefile.assert_called_once_with('r', 8192) + + assert result is mock_file_stream + + +def test_default_buffer_size_deprecated(): + """Test accessing ``DEFAULT_BUFFER_SIZE`` emits warning.""" + with pytest.deprecated_call( + match='`DEFAULT_BUFFER_SIZE` is deprecated', + ): + val = builtin_adapter.DEFAULT_BUFFER_SIZE + + # Check that the returned value is correct + assert val == io.DEFAULT_BUFFER_SIZE + + +def test_SSLFileobjectStreamReader_deprecated(mocker): + """Test creating SSLFileobjectStreamReader emits warning.""" + expected_message = r'SSLFileobjectStreamReader.*deprecated' + dummy_sock = mocker.create_autospec(socket.socket, instance=True) + dummy_sock.fileno.return_value = 1 # Mock file descriptor + + with pytest.deprecated_call( + match=expected_message, + ): + SSLFileobjectStreamReader( + dummy_sock, + mode='rb', + bufsize=io.DEFAULT_BUFFER_SIZE, + ) + + +def test_SSLFileobjectStreamWriter_deprecated(mocker): + """Test creating SSLFileobjectStreamWriter emits warning.""" + expected_message = r'SSLFileobjectStreamWriter.*deprecated' + dummy_sock = mocker.create_autospec(socket.socket, instance=True) + + with pytest.deprecated_call( + match=expected_message, + ): + SSLFileobjectStreamWriter( + dummy_sock, + mode='wb', + bufsize=io.DEFAULT_BUFFER_SIZE, + ) diff --git a/docs/changelog-fragments.d/805.deprecation.rst b/docs/changelog-fragments.d/805.deprecation.rst new file mode 100644 index 0000000000..27271d7676 --- /dev/null +++ b/docs/changelog-fragments.d/805.deprecation.rst @@ -0,0 +1,12 @@ +Deprecated :py:func:`~cheroot.makefile.MakeFile` in favor of :py:class:`~cheroot.makefile.StreamReader` +and :py:class:`~cheroot.makefile.StreamWriter`. + +The :py:func:`!MakeFile` factory function is replaced with direct class instantiation +and now emits a :py:exc:`DeprecationWarning`. The function will be removed in +a future release. + +Adapter-specific ``makefile()`` methods are also deprecated in :py:mod:`~cheroot.ssl.builtin.BuiltinSSLAdapter`, +:py:mod:`~cheroot.ssl.pyopenssl.pyOpenSSLAdapter` and the :py:mod:`~cheroot.ssl.Adapter` interface. +All will be removed in a future release. + +-- by :user:`julianz-` diff --git a/docs/spelling_wordlist.txt b/docs/spelling_wordlist.txt index 6ab6243e0c..cfcea0357c 100644 --- a/docs/spelling_wordlist.txt +++ b/docs/spelling_wordlist.txt @@ -1,4 +1,5 @@ AppVeyor +args Args backend backports