Added TLSSocket abstraction for uniform SSL/TLS handling #799

julianz- · 2025-11-20T22:17:30Z

Introduces a new TLSSocket class to act as a unified wrapper for SSL/TLS connections, regardless of the underlying adapter (builtin, pyOpenSSL).

This refactoring aims to:

Simplify adapter logic by centralizing common TLS socket properties and methods.
Improve consistency when populating WSGI environment variables.
Centralize error handling for the adapters.

cheroot/ssl/builtin.py

cheroot/ssl/pyopenssl.py

+    def _create_pyopenssl_connection(self, raw_socket):
+        """Create PyOpenSSL connection object."""
+        try:
+            ssl_object = ssl_conn_type(self.context, raw_socket)


cheroot/test/ssl/test_ssl_builtin.py

+            (bind_host, actual_port),
+            timeout=_CONNECTION_TIMEOUT_SECONDS,
+        )
+        client_sock = context.wrap_socket(sock, server_hostname=bind_host)


cheroot/test/ssl/test_ssl_pyopenssl.py

+        )
+
+        # 2. Wrapping with SSL and completing handshake
+        client_sock = context.wrap_socket(sock, server_hostname=bind_host)


cheroot/ssl/builtin.py

+                environ['SSL_COMPRESS_METHOD'] = compression
+        except AttributeError:
+            # TLSSocket might not have compression method
+            ...


cheroot/ssl/builtin.py

+            if server_hostname:
+                environ['SSL_TLS_SNI'] = server_hostname
+        except AttributeError:
+            ...


cheroot/test/ssl/test_ssl_builtin.py

cheroot/test/ssl/test_ssl_pyopenssl.py

codecov · 2025-11-20T22:20:57Z

Codecov Report

❌ Patch coverage is 13.60000% with 216 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.58%. Comparing base (a475500) to head (21b753e).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #799       +/-   ##
===========================================
- Coverage   77.69%   38.58%   -39.12%     
===========================================
  Files          41       13       -28     
  Lines        4672      635     -4037     
  Branches      544        1      -543     
===========================================
- Hits         3630      245     -3385     
+ Misses        900      390      -510     
+ Partials      142        0      -142

webknjaz

These are a few superficial observations. We'll have to work towards moving as much as possible into multiple small PRs so that this one will be reviewable. But one thing I'd want to request already is avoiding intercepting broad exception class and sticking to what's known and expected. I marked some things that are mostly ready for extraction out of this patch.

webknjaz · 2025-11-21T01:00:31Z

cheroot/connections.py

+                if hasattr(
+                    self.server.ssl_adapter,
+                    '_check_for_plain_http',
+                ) and self.server.ssl_adapter._check_for_plain_http(s):


Better move this into the wrap method of the adapter and raise a NoSSLError so that it's handled in the same except block below for both adapters.
This way, the adapter internals won't leak into this layer.

Much better yes!

webknjaz · 2025-11-21T01:04:31Z

cheroot/connections.py

+        response_bytes = ''.join(response_parts).encode('ISO-8859-1')
+
+        try:
+            sock.sendall(response_bytes)


Do we need a modern enough PyOpenSSL for sendall() to work well?

I changed to:

if hasattr(sock, 'sendall'): sock.sendall(response_bytes) else: # Fallback for older PyOpenSSL or SSL objects sock.send(response_bytes)

Is that safe enough?

I don't know. The fallback might need a loop or something. I was just mostly asking because you worked with that lib recently and I didn't. If it needs a newer PyOpenSSL, I'd maybe think of starting to provide a pyopenssl extra in the core packaging metadata.

webknjaz · 2025-11-21T01:07:26Z

cheroot/connections.py

-                    s.settimeout(self.server.timeout)
+                except errors.NoSSLError:
+                    # this only arises for PyOpenSSL as
+                    # we handle http over https pre-emptivley


Suggested change

# we handle http over https pre-emptivley

# we handle http over https pre-emptively

(although, with the suggestion above we could just drop the entire comment)

Yes gone now

webknjaz · 2025-11-21T01:08:50Z

cheroot/connections.py

+    def _send_bad_request_plain_http_error(self, sock, addr):
+        """Send Bad Request 400 response, and close the socket."""
+        self.server.error_log(
+            f'Client {addr!s} attempted to speak plain HTTP into '
+            'a TCP connection configured for TLS-only traffic — '
+            'Sending 400 Bad Request.',
+        )
+
+        msg = (
+            'The client sent a plain HTTP request, but this server '
+            'only speaks HTTPS on this port.'
+        )
+
+        response_parts = [
+            f'{self.server.protocol} 400 Bad Request\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')
+
+        try:
+            sock.sendall(response_bytes)
+            sock.shutdown(socket.SHUT_WR)
+        except OSError as ex:
+            if ex.args[0] not in errors.socket_errors_to_ignore:
+                raise
+
+        sock.close()
+


This method (or a simplified/standalone variant) could go into a separate PR so that we'd accept it earlier and have a smaller patch here.

Sure. Will do.

webknjaz · 2025-11-21T01:11:39Z

cheroot/makefile.py

 import _pyio as io
 import socket

+from .ssl.tls_socket import TLSSocket


I'm not sure we should be bringing TLS internals into the generic makefile. I'll have to think more about this..

Good point. Maybe this is better?

# Check for wrapped socket if hasattr(sock, 'read') and hasattr(sock, 'write'): raw_io = sock # Already wrapped (could be TLSSocket) else: # Standard socket - wrap with SocketIO raw_io = socket.SocketIO(sock, mode)

With the @classmethod suggestion (#799 (comment)), this will be redundant.

webknjaz · 2025-11-21T01:36:14Z

cheroot/ssl/builtin.py

+            f'Fatal SSL error during handshake: {error}',
+        ) from error

    @property


This could probably be a cached properly.

webknjaz · 2025-11-21T01:37:10Z

cheroot/ssl/builtin.py

+            with open(self.certificate, 'rb') as cert_file:
+                cert_data = cert_file.read()


We should just use pathlib for these.

webknjaz · 2025-11-21T01:38:26Z

cheroot/ssl/builtin.py

+
        env = {
-            env_prefix: ','.join(dn),
+            env_prefix: '/%s' % (dn_string,),


Use f-strings instead of old-style interpolation.

webknjaz · 2025-11-21T01:39:59Z

cheroot/ssl/builtin.pyi

    def bind(self, sock): ...
    def wrap(self, sock): ...
-    def get_environ(self, sock): ...
+    def get_environ(self, conn) -> dict: ...


This public API change might be dangerous for CherryPy. Will have to check if it uses this.

webknjaz · 2025-11-21T01:43:39Z

cheroot/ssl/tls_socket.py

+
+This module provides a TLSSocket class that abstracts over
+different SSL/TLS implementations, such as Python's built-in ssl module
+and pyOpenSSL. It offers a consistent interface for the rest of


This module should only have a generic base class and sockets for different libs would be subclasses. Trying to put multiple corner cases from multiple libs is going to be difficult to maintain or extend.

Introduces a new `TLSSocket` class to act as a unified wrapper for SSL/TLS connections, regardless of the underlying adapter (`builtin`, `pyOpenSSL`). This refactoring aims to: 1. Simplify adapter logic by centralizing common TLS socket properties and methods (e.g., cipher details, certificate paths). 2. Improve consistency when populating WSGI environment variables. 3. Centralize error handling in the adapters.

webknjaz · 2025-11-23T00:05:21Z

cheroot/test/conftest.py

+
+
+@pytest.fixture
+def tls_certificate_passwd_private_key_pem_path(


I think this is only used in one test module. Unless it's used in more places, it should remain in that module.

webknjaz · 2025-11-23T00:06:21Z

cheroot/test/conftest.py

Let's put moving these into a separate PR.

webknjaz · 2025-11-23T00:06:52Z

cheroot/test/ssl/test_ssl_builtin.py

Rename to cheroot/test/ssl/test_builtin.py

webknjaz · 2025-11-23T00:07:07Z

cheroot/test/ssl/test_ssl_pyopenssl.py

Rename to cheroot/test/ssl/test_ssl_pyopenssl.py

webknjaz · 2025-11-23T00:08:44Z

cheroot/test/test_ssl.py

    IS_LINUX,
    IS_MACOS,
    IS_PYPY,
-    IS_SOLARIS,


If this var isn't used elsewhere, we might want to drop it from compat.

webknjaz · 2025-11-23T00:28:16Z

.mypy.ini

@@ -1,5 +1,5 @@
 [mypy]
-python_version = 3.8
+python_version = 3.9


This would have to go into a separate PR and be coupled with changes to the core packaging metadata, linting configuration, dev env and CI setup.

webknjaz · 2025-11-23T00:29:16Z

cheroot/makefile.py


 # prefer slower Python-based io module
 import _pyio as io
+import io as stdlib_io


Why is this needed?

webknjaz · 2025-11-23T00:31:35Z

cheroot/makefile.py

+            # Wrap raw socket with SocketIO
+            raw_io = socket.SocketIO(sock, 'rb')
+
+        super().__init__(raw_io, bufsize)


This is still checking obscure properties of objects to infer TLS relation. We shouldn't be relying on whether something is TLS or not in here. This is an abstraction leak no matter the method of checking.
The callers should decide if something needs wrapping and this is best done through providing alternative constructors.

webknjaz · 2025-11-23T00:33:34Z

cheroot/makefile.py

-    def __init__(self, sock, mode='w', bufsize=io.DEFAULT_BUFFER_SIZE):
-        """Initialize socket stream writer."""
-        super().__init__(socket.SocketIO(sock, mode), bufsize)
+    def __init__(self, sock, bufsize=io.DEFAULT_BUFFER_SIZE):


Removing arguments or changing their order is a backwards incompatible change. We should avoid those and at least have a deprecation period to allow for downstream adoption. Deprecations would have to be dedicated multi-stage coordinated processes.

webknjaz · 2025-11-23T00:39:03Z

cheroot/ssl/__init__.py

+    Args:
+        components: Iterable of (key, value) tuples
+        key_prefix: 'SSL_CLIENT' or 'SSL_SERVER'
+        dn_type: 'S' for subject or 'I' for issuer
+
+    Returns:
+        dict: ``DN`` and ``CN`` environment variables


Don't use napoleon that is not integrated. Use Sphinx-native param lists.

Suggested change

Args:

components: Iterable of (key, value) tuples

key_prefix: 'SSL_CLIENT' or 'SSL_SERVER'

dn_type: 'S' for subject or 'I' for issuer

Returns:

dict: ``DN`` and ``CN`` environment variables

:param components: Iterable of (key, value) tuples

:param key_prefix: 'SSL_CLIENT' or 'SSL_SERVER'

:param dn_type: 'S' for subject or 'I' for issuer

:returns: ``DN`` and ``CN`` environment variables

:rtype: dict

Currently, it's not rendered well and Sphinx objects aren't recorded for params: https://cheroot--799.org.readthedocs.build/en/799/pkg/cheroot.ssl/#cheroot.ssl._parse_dn_components.

When done correctly, the params are clearly marked and well-formatted. As well as return values and raised exceptions. Example: https://cheroot--799.org.readthedocs.build/en/799/pkg/cheroot._compat/#cheroot._compat.extract_bytes.

webknjaz · 2025-11-23T00:44:22Z

cheroot/ssl/__init__.py

+
+def _parse_dn_components(components, key_prefix, dn_type):
+    """
+    Parse Distinguished Name components into environ dict.


Use full words in docstrings. Not "environ" or "dict", because docstrings are user-facing. Additionally, it's possible to link objects Sphinx knows of, like :class:`dict`.

Within the scope of this function, it doesn't really matter how the caller will use the dict. So claiming that it's an "environ dict" doesn't make sense. It's just a dict and that's it. The caller might put it into a context.

Perhaps, this would be better:

Suggested change

Parse Distinguished Name components into environ dict.

Transform Distinguished Name components list into a dictionary.

webknjaz · 2025-11-23T00:47:51Z

cheroot/server.py

    # Fields set by ConnectionManager.
    last_used = None

-    def __init__(self, server, sock, makefile=MakeFile):


We'll probably have to keep this for a while (with a sentinel default). And issue a warning when the value isn't that sentinel.

webknjaz · 2025-11-23T00:48:54Z

cheroot/server.py

        try:
            resp_sock = self.socket._sock
        except AttributeError:
            # self.socket is of OpenSSL.SSL.Connection type


Oh, look — another abstraction leak to fight..

webknjaz · 2025-11-23T01:03:26Z

@julianz- the PR is so big that it can't be reviewed efficiently. I just can't keep up. Let's focus on finding all the things that can be merged as standalone PRs first. I'm trying to spot some of those but maybe, you'll see a few opportunities too.

github-advanced-security bot found potential problems Nov 20, 2025

View reviewed changes

julianz- force-pushed the tls_socket_refactor branch from 83a6da2 to 7447077 Compare November 20, 2025 22:54

psf-chronographer bot added the bot:chronographer:provided label Nov 20, 2025

julianz- mentioned this pull request Nov 20, 2025

Drop locks from the cheroot.ssl.pyopenssl.SSLConnection method #787

Open

julianz- force-pushed the tls_socket_refactor branch from 7447077 to 2c8e1a1 Compare November 21, 2025 01:46

webknjaz reviewed Nov 21, 2025

View reviewed changes

julianz- force-pushed the tls_socket_refactor branch 16 times, most recently from d0753ec to ae8afd4 Compare November 22, 2025 20:06

julianz- force-pushed the tls_socket_refactor branch from ae8afd4 to 21b753e Compare November 22, 2025 20:09