Add TLSSocket abstraction for uniform SSL/TLS handling
#799
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| def _create_pyopenssl_connection(self, raw_socket): | ||
| """Create PyOpenSSL connection object.""" | ||
| try: | ||
| ssl_object = ssl_conn_type(self.context, raw_socket) |
Check failure
Code scanning / CodeQL
Use of insecure SSL/TLS version High
| (bind_host, actual_port), | ||
| timeout=_CONNECTION_TIMEOUT_SECONDS, | ||
| ) | ||
| client_sock = context.wrap_socket(sock, server_hostname=bind_host) |
Check failure
Code scanning / CodeQL
Use of insecure SSL/TLS version High test
| ) | ||
|
|
||
| # 2. Wrapping with SSL and completing handshake | ||
| client_sock = context.wrap_socket(sock, server_hostname=bind_host) |
Check failure
Code scanning / CodeQL
Use of insecure SSL/TLS version High test
| environ['SSL_COMPRESS_METHOD'] = compression | ||
| except AttributeError: | ||
| # TLSSocket might not have compression method | ||
| ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
| if server_hostname: | ||
| environ['SSL_TLS_SNI'] = server_hostname | ||
| except AttributeError: | ||
| ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
Codecov Report❌ Patch coverage is 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 |
julianz-
force-pushed
the
tls_socket_refactor
branch
from
83a6da2 to
7447077
Compare
julianz-
force-pushed
the
tls_socket_refactor
branch
from
7447077 to
2c8e1a1
Compare
webknjaz
left a comment
webknjaz
left a comment
There was a problem hiding this comment.
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.
cheroot/connections.py
Outdated
| if hasattr( | ||
| self.server.ssl_adapter, | ||
| '_check_for_plain_http', | ||
| ) and self.server.ssl_adapter._check_for_plain_http(s): |
There was a problem hiding this comment.
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.
cheroot/connections.py
Outdated
| response_bytes = ''.join(response_parts).encode('ISO-8859-1') | ||
|
|
||
| try: | ||
| sock.sendall(response_bytes) |
There was a problem hiding this comment.
Do we need a modern enough PyOpenSSL for sendall() to work well?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
cheroot/connections.py
Outdated
| s.settimeout(self.server.timeout) | ||
| except errors.NoSSLError: | ||
| # this only arises for PyOpenSSL as | ||
| # we handle http over https pre-emptivley |
There was a problem hiding this comment.
| # 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)
| 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() | ||
|
|
There was a problem hiding this comment.
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.
cheroot/makefile.py
Outdated
| import _pyio as io | ||
| import socket | ||
|
|
||
| from .ssl.tls_socket import TLSSocket |
There was a problem hiding this comment.
I'm not sure we should be bringing TLS internals into the generic makefile. I'll have to think more about this..
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
With the @classmethod suggestion (#799 (comment)), this will be redundant.
There was a problem hiding this comment.
With the @classmethod suggestion (#799 (comment)), this will be redundant.
| f'Fatal SSL error during handshake: {error}', | ||
| ) from error | ||
|
|
||
| @property |
There was a problem hiding this comment.
This could probably be a cached properly.
| with open(self.certificate, 'rb') as cert_file: | ||
| cert_data = cert_file.read() |
There was a problem hiding this comment.
We should just use pathlib for these.
|
|
||
| env = { | ||
| env_prefix: ','.join(dn), | ||
| env_prefix: '/%s' % (dn_string,), |
There was a problem hiding this comment.
Use f-strings instead of old-style interpolation.
| def bind(self, sock): ... | ||
| def wrap(self, sock): ... | ||
| def get_environ(self, sock): ... | ||
| def get_environ(self, conn) -> dict: ... |
There was a problem hiding this comment.
This public API change might be dangerous for CherryPy. Will have to check if it uses this.
| 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 |
There was a problem hiding this comment.
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.
julianz-
force-pushed
the
tls_socket_refactor
branch
16 times, most recently
from
d0753ec to
ae8afd4
Compare
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.
julianz-
force-pushed
the
tls_socket_refactor
branch
from
ae8afd4 to
21b753e
Compare
|
|
||
|
|
||
| @pytest.fixture | ||
| def tls_certificate_passwd_private_key_pem_path( |
There was a problem hiding this comment.
I think this is only used in one test module. Unless it's used in more places, it should remain in that module.
There was a problem hiding this comment.
Let's put moving these into a separate PR.
There was a problem hiding this comment.
Rename to cheroot/test/ssl/test_builtin.py
There was a problem hiding this comment.
Rename to cheroot/test/ssl/test_ssl_pyopenssl.py
| IS_LINUX, | ||
| IS_MACOS, | ||
| IS_PYPY, | ||
| IS_SOLARIS, |
There was a problem hiding this comment.
If this var isn't used elsewhere, we might want to drop it from compat.
| @@ -1,5 +1,5 @@ | |||
| [mypy] | |||
| python_version = 3.8 | |||
| python_version = 3.9 | |||
There was a problem hiding this comment.
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.
|
|
||
| # prefer slower Python-based io module | ||
| import _pyio as io | ||
| import io as stdlib_io |
| # Wrap raw socket with SocketIO | ||
| raw_io = socket.SocketIO(sock, 'rb') | ||
|
|
||
| super().__init__(raw_io, bufsize) |
There was a problem hiding this comment.
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.
| 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): |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
Don't use napoleon that is not integrated. Use Sphinx-native param lists.
| 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.
|
|
||
| def _parse_dn_components(components, key_prefix, dn_type): | ||
| """ | ||
| Parse Distinguished Name components into environ dict. |
There was a problem hiding this comment.
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:
| Parse Distinguished Name components into environ dict. | |
| Transform Distinguished Name components list into a dictionary. |
| # Fields set by ConnectionManager. | ||
| last_used = None | ||
|
|
||
| def __init__(self, server, sock, makefile=MakeFile): |
There was a problem hiding this comment.
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.
| try: | ||
| resp_sock = self.socket._sock | ||
| except AttributeError: | ||
| # self.socket is of OpenSSL.SSL.Connection type |
There was a problem hiding this comment.
Oh, look — another abstraction leak to fight..
|
@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. |
|
@webknjaz Agreed. Let's do things in smaller chunks. |
webknjaz
changed the title
TLSSocket abstraction for uniform SSL/TLS handling
| def _parse_dn_components(components, key_prefix, dn_type): | ||
| """ | ||
| Parse Distinguished Name components into environ dict. | ||
| 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 | ||
| """ | ||
| env = {} | ||
| dn_parts = [] | ||
|
|
||
| for key, attr_value in components: | ||
| dn_parts.append(f'{key}={attr_value}') | ||
| if key in {'CN', 'commonName'}: | ||
| env[f'{key_prefix}_{dn_type}_DN_CN'] = attr_value | ||
|
|
||
| if dn_parts: | ||
| dn_string = DN_SEPARATOR.join(dn_parts) | ||
| env[f'{key_prefix}_{dn_type}_DN'] = f'{DN_SEPARATOR}{dn_string}' | ||
|
|
||
| return env | ||
|
|
||
|
|
||
| def parse_pyopenssl_cert_to_environ(cert, key_prefix): | ||
| """Parse a pyOpenSSL X509 certificate into WSGI environ dict.""" | ||
| env = {} | ||
| if not cert: | ||
| return env | ||
|
|
||
| # Subject | ||
| subject = cert.get_subject() | ||
| if subject: | ||
| components = [ | ||
| (key.decode(UTF8_ENCODING), attr_value.decode(UTF8_ENCODING)) | ||
| for key, attr_value in subject.get_components() | ||
| ] | ||
| env.update(_parse_dn_components(components, key_prefix, 'S')) | ||
|
|
||
| # Issuer | ||
| issuer = cert.get_issuer() | ||
| if issuer: | ||
| components = [ | ||
| (key.decode(UTF8_ENCODING), attr_value.decode(UTF8_ENCODING)) | ||
| for key, attr_value in issuer.get_components() | ||
| ] | ||
| env.update(_parse_dn_components(components, key_prefix, 'I')) | ||
|
|
||
| # Version and Serial | ||
| env[f'{key_prefix}_M_VERSION'] = str(cert.get_version()) | ||
| env[f'{key_prefix}_M_SERIAL'] = str(cert.get_serial_number()) | ||
|
|
||
| return env | ||
|
|
||
|
|
||
| def parse_x509_cert_to_environ(cert, key_prefix): | ||
| """Parse a cryptography x509 certificate into environ dict.""" | ||
| env = {} | ||
|
|
||
| # Subject | ||
| with suppress(Exception): | ||
| subject = cert.subject | ||
| components = [(attr.oid._name, attr.value) for attr in subject] | ||
| env.update(_parse_dn_components(components, key_prefix, 'S')) | ||
|
|
||
| # Issuer | ||
| with suppress(Exception): | ||
| issuer = cert.issuer | ||
| components = [(attr.oid._name, attr.value) for attr in issuer] | ||
| env.update(_parse_dn_components(components, key_prefix, 'I')) | ||
|
|
||
| # Version and Serial | ||
| with suppress(Exception): | ||
| env[f'{key_prefix}_M_VERSION'] = str(cert.version.value) | ||
| env[f'{key_prefix}_M_SERIAL'] = str(cert.serial_number) | ||
|
|
||
| return env |
There was a problem hiding this comment.
These helpers look like they could be a separate refactoring PR. Also, I'm not sure if it's a good idea to put adapter-specific things into a shared module.
|
@julianz- let's proceed here by splitting out the makefile function removal into a separate PR. And another PR for the certificate parsing helpers. This should help use make this one as small as possible. Plus cleaning up unrelated config changes will be helpful too. |
|
After checking, removing the makefile really depends on using a unified class to represent the adpaters - i.e. the one I added here: |
I was imagining you'd replace things like
Go for it! Once done, this PR can be rebased and it'll be easier to see what to do with the rest. It's also a good idea to drop any unrelated linting / tooling changes. |
|
Ah okay. I have done something more minimal that removes the |
|
Having looked into it, I see that it's not as simple to get rid of Once that's done, we you can rebase this and we'd strategize further. Sounds good? |
2 participants
Introduces a new
TLSSocketclass to act as a unified wrapper for SSL/TLS connections, regardless of the underlying adapter (builtin,pyOpenSSL).This refactoring aims to: