Skip to content

Add TLSSocket abstraction for uniform SSL/TLS handling #799

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
julianz- wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add TLSSocket abstraction for uniform SSL/TLS handling #799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions .flake8
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,16 +124,17 @@ per-file-ignores =
cheroot/__main__.py: WPS130
cheroot/_compat.py: DAR101, DAR201, DAR301, DAR401, I003, RST304, WPS100, WPS111, WPS123, WPS226, WPS229, WPS420, WPS422, WPS432, WPS504, WPS505
cheroot/cli.py: DAR101, DAR201, DAR401, I001, I004, I005, WPS100, WPS110, WPS120, WPS130, WPS202, WPS226, WPS229, WPS338, WPS420, WPS421
cheroot/connections.py: DAR101, DAR201, DAR301, DAR401, I001, I003, I004, I005, RST304, S104, WPS100, WPS110, WPS111, WPS121, WPS122, WPS130, WPS201, WPS204, WPS210, WPS212, WPS214, WPS220, WPS229, WPS231, WPS301, WPS324, WPS338, WPS420, WPS421, WPS422, WPS432, WPS501, WPS504, WPS505
cheroot/connections.py: DAR101, DAR201, DAR301, DAR401, I001, I003, I004, I005, RST304, S104, WPS100, WPS110, WPS111, WPS121, WPS122, WPS130, WPS201, WPS204, WPS210, WPS212, WPS214, WPS220, WPS229, WPS231, WPS237, WPS301, WPS324, WPS338, WPS420, WPS421, WPS422, WPS432, WPS501, WPS504, WPS505
cheroot/errors.py: DAR101, DAR201, I003, RST304, WPS111, WPS121, WPS422
cheroot/makefile.py: DAR101, DAR201, DAR401, E800, I003, I004, N801, N802, S101, WPS100, WPS110, WPS111, WPS117, WPS120, WPS121, WPS122, WPS123, WPS130, WPS204, WPS210, WPS212, WPS213, WPS220, WPS229, WPS231, WPS232, WPS338, WPS420, WPS422, WPS429, WPS431, WPS504, WPS604, WPS606
cheroot/makefile.py: DAR101, DAR201, DAR401, E800, I003, I004, N801, N802, S101, WPS100, WPS110, WPS111, WPS117, WPS120, WPS121, WPS122, WPS123, WPS130, WPS204, WPS210, WPS212, WPS213, WPS226, WPS220, WPS229, WPS231, WPS232, WPS338, WPS420, WPS422, WPS429, WPS431, WPS504, WPS604, WPS606
cheroot/server.py: DAR003, DAR101, DAR201, DAR202, DAR301, DAR401, E800, I001, I003, I004, I005, N806, RST201, RST301, RST303, RST304, WPS100, WPS110, WPS111, WPS115, WPS120, WPS121, WPS122, WPS130, WPS132, WPS201, WPS202, WPS204, WPS210, WPS211, WPS212, WPS213, WPS214, WPS220, WPS221, WPS225, WPS226, WPS229, WPS230, WPS231, WPS236, WPS237, WPS238, WPS301, WPS338, WPS342, WPS410, WPS420, WPS421, WPS422, WPS429, WPS432, WPS504, WPS505, WPS601, WPS602, WPS608, WPS617
cheroot/ssl/builtin.py: DAR101, DAR201, DAR401, I001, I003, N806, RST304, WPS110, WPS111, WPS115, WPS117, WPS120, WPS121, WPS122, WPS130, WPS201, WPS210, WPS214, WPS229, WPS231, WPS338, WPS422, WPS501, WPS505, WPS529, WPS608, WPS612
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/ssl/builtin.py: DAR101, DAR201, DAR401, I001, I003, N806, RST304, WPS110, WPS111, WPS115, WPS117, WPS120, WPS121, WPS122, WPS130, WPS201, WPS204, WPS210, WPS220, WPS214, WPS226, WPS229, WPS231, WPS338, WPS421, WPS422, WPS501, WPS505, WPS529, WPS608, WPS612
cheroot/ssl/pyopenssl.py: C815, DAR101, DAR201, DAR401, I001, I003, I005, N801, N804, RST304, WPS100, WPS110, WPS111, WPS117, WPS120, WPS121, WPS122, WPS130, WPS210, WPS220, WPS221, WPS225, WPS229, WPS231, WPS238, WPS301, WPS335, WPS338, WPS420, WPS422, WPS430, WPS432, WPS501, WPS504, WPS505, WPS601, WPS608, WPS615
cheroot/ssl/tls_socket.py: DAR101, DAR201, DAR401, WPS110, WPS122, WPS210, WPS212, WPS214, WPS220, WPS225, WPS226, WPS229, WPS231, WPS238, WPS338, WPS362, WPS407
cheroot/test/conftest.py: DAR101, DAR201, DAR301, I001, I003, I005, WPS100, WPS130, WPS202, 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/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_makefile.py: DAR101, DAR201, I004, RST304, S101, WPS110, WPS122, WPS362
cheroot/test/test_wsgi.py: DAR101, DAR301, I001, I004, S101, WPS110, WPS111, WPS117, WPS118, WPS121, WPS210, WPS421, WPS430, WPS432, WPS441, WPS509
cheroot/test/test_core.py: C815, DAR101, DAR201, DAR401, I003, I004, N805, N806, S101, WPS110, WPS111, WPS114, WPS121, WPS202, WPS204, WPS226, WPS229, WPS324, WPS421, WPS422, WPS432, WPS602
cheroot/test/test_dispatch.py: DAR101, DAR201, S101, WPS111, WPS121, WPS422, WPS430
Expand All @@ -144,7 +145,9 @@ per-file-ignores =
cheroot/testing.py: C815, DAR101, DAR201, DAR301, I001, I003, S104, WPS100, WPS202, WPS211, WPS229, WPS301, WPS414, WPS420, WPS422, WPS430
cheroot/workers/threadpool.py: DAR101, DAR201, E800, I001, I003, I004, RST201, RST203, RST301, WPS100, WPS110, WPS111, WPS121, WPS122, WPS210, WPS211, WPS214, WPS220, WPS229, WPS230, WPS231, WPS335, WPS338, WPS362, WPS363, WPS410, WPS414, WPS420, WPS422, WPS432, WPS501, WPS505, WPS601, WPS602, WPS617
cheroot/wsgi.py: DAR101, DAR201, DAR401, I001, I003, I005, N801, RST201, RST301, WPS100, WPS110, WPS111, WPS114, WPS121, WPS122, WPS130, WPS210, WPS211, WPS226, WPS229, WPS231, WPS338, WPS420, WPS421, WPS422, WPS430, WPS501, WPS504, WPS602, WPS608
cheroot/ssl/__init__.py: DAR101, DAR201, I003, WPS412, WPS422
cheroot/ssl/__init__.py: DAR101, DAR201, I003, WPS210, WPS412, WPS422
cheroot/test/ssl/test_ssl_builtin.py: DAR101, DAR201, I003, WPS118, WPS201, WPS202, WPS210, WPS213, WPS218, WPS211, WPS226, WPS229, WPS231, WPS243, WPS412, WPS420, WPS422, WPS430, WPS505
cheroot/test/ssl/test_ssl_pyopenssl.py: DAR101, DAR201, I003, WPS118, WPS201, WPS202, WPS204, WPS210, WPS220, WPS213, WPS218, WPS211, WPS226, WPS229, WPS231, WPS243, WPS412, WPS420, WPS422, WPS430, WPS432, WPS435, WPS505
cheroot/test/_pytest_plugin.py: DAR101, I003, I004, WPS422
cheroot/test/test__compat.py: DAR101, I001, I003, I005, WPS116, WPS226, WPS422, S101
cheroot/test/test_errors.py: DAR101, WPS509, S101
Expand Down
2 changes: 1 addition & 1 deletion .mypy.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[mypy]
python_version = 3.8
python_version = 3.9
Copy link
Copy Markdown
Member

@webknjaz webknjaz Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

color_output = true
error_summary = true
files =
Expand Down
81 changes: 44 additions & 37 deletions cheroot/connections.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
"""Utilities to manage open connections."""

import io
import os
import selectors
import socket
Expand All @@ -10,7 +9,6 @@

from . import errors
from ._compat import IS_WINDOWS
from .makefile import MakeFile


try:
Expand Down Expand Up @@ -293,50 +291,22 @@ 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:
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 None
except errors.NoSSLError as http_over_https_err:
except errors.FatalSSLAlert as tls_connection_error:
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}',
f'Failed to establish SSL connection with {addr!s}: '
f'{tls_connection_error!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
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)
except errors.NoSSLError:
return self._send_bad_request_plain_http_error(s, addr)

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
Expand Down Expand Up @@ -381,6 +351,43 @@ def _from_server_socket(self, server_socket): # noqa: C901 # FIXME
return None
raise

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:
# Handle both raw sockets and SSL connections
if hasattr(sock, 'sendall'):
sock.sendall(response_bytes)
else:
# Fallback for older PyOpenSSL or SSL objects
sock.send(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()

Comment on lines +354 to +390
Copy link
Copy Markdown
Member

@webknjaz webknjaz Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@julianz- julianz- Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Will do.

def close(self):
"""Close all monitored connections."""
for _, conn in self._selector.connections:
Expand Down
33 changes: 21 additions & 12 deletions cheroot/makefile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

# prefer slower Python-based io module
import _pyio as io
import io as stdlib_io
Copy link
Copy Markdown
Member

@webknjaz webknjaz Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

import socket


Expand Down Expand Up @@ -38,9 +39,16 @@ def _flush_unlocked(self):
class StreamReader(io.BufferedReader):
"""Socket stream reader."""

def __init__(self, sock, mode='r', bufsize=io.DEFAULT_BUFFER_SIZE):
"""Initialize socket stream reader."""
super().__init__(socket.SocketIO(sock, mode), bufsize)
def __init__(self, sock, bufsize=io.DEFAULT_BUFFER_SIZE):
"""Initialize with socket or raw IO object."""
# If already a RawIOBase (like TLSSocket), use directly
if isinstance(sock, (io.RawIOBase, stdlib_io.RawIOBase)):
raw_io = sock
else:
# Wrap raw socket with SocketIO
raw_io = socket.SocketIO(sock, 'rb')

super().__init__(raw_io, bufsize)
Copy link
Copy Markdown
Member

@webknjaz webknjaz Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.bytes_read = 0

def read(self, *args, **kwargs):
Expand All @@ -57,19 +65,20 @@ def has_data(self):
class StreamWriter(BufferedWriter):
"""Socket stream writer."""

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):
Copy link
Copy Markdown
Member

@webknjaz webknjaz Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""Initialize with socket or raw IO object."""
# If already a RawIOBase (like TLSSocket), use directly
if isinstance(sock, (io.RawIOBase, stdlib_io.RawIOBase)):
raw_io = sock
else:
# Wrap raw socket with SocketIO
raw_io = socket.SocketIO(sock, 'wb')

super().__init__(raw_io, bufsize)
self.bytes_written = 0

def write(self, val, *args, **kwargs):
"""Capture bytes written."""
res = super().write(val, *args, **kwargs)
self.bytes_written += len(val)
return res


def MakeFile(sock, mode='r', bufsize=io.DEFAULT_BUFFER_SIZE):
"""File object attached to a socket object."""
cls = StreamReader if 'r' in mode else StreamWriter
return cls(sock, mode, bufsize)
6 changes: 2 additions & 4 deletions cheroot/makefile.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,11 @@ class BufferedWriter(io.BufferedWriter):

class StreamReader(io.BufferedReader):
bytes_read: int
def __init__(self, sock, mode: str = ..., bufsize=...) -> None: ...
def __init__(self, sock, bufsize=...) -> None: ...
def read(self, *args, **kwargs): ...
def has_data(self): ...

class StreamWriter(BufferedWriter):
bytes_written: int
def __init__(self, sock, mode: str = ..., bufsize=...) -> None: ...
def __init__(self, sock, bufsize=...) -> None: ...
def write(self, val, *args, **kwargs): ...

def MakeFile(sock, mode: str = ..., bufsize=...): ...
11 changes: 5 additions & 6 deletions cheroot/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@

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


Expand Down Expand Up @@ -1275,19 +1275,18 @@ class HTTPConnection:
# Fields set by ConnectionManager.
last_used = None

def __init__(self, server, sock, makefile=MakeFile):
Copy link
Copy Markdown
Member

@webknjaz webknjaz Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

def __init__(self, server, sock):
"""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
"""
self.server = server
self.socket = sock
self.rfile = makefile(sock, 'rb', self.rbufsize)
self.wfile = makefile(sock, 'wb', self.wbufsize)
self.rfile = StreamReader(sock, self.rbufsize)
self.wfile = StreamWriter(sock, self.wbufsize)
self.requests_seen = 0

self.peercreds_enabled = self.server.peercreds_enabled
Expand Down Expand Up @@ -1363,7 +1362,7 @@ def _handle_no_ssl(self, req):
except AttributeError:
# self.socket is of OpenSSL.SSL.Connection type
Copy link
Copy Markdown
Member

@webknjaz webknjaz Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, look — another abstraction leak to fight..

resp_sock = self.socket._socket
self.wfile = StreamWriter(resp_sock, 'wb', self.wbufsize)
self.wfile = StreamWriter(resp_sock, self.wbufsize)
msg = (
'The client sent a plain HTTP request, but '
'this server only speaks HTTPS on this port.'
Expand Down
2 changes: 1 addition & 1 deletion cheroot/server.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ class HTTPConnection:
rfile: Any
wfile: Any
requests_seen: int
def __init__(self, server, sock, makefile=...) -> None: ...
def __init__(self, server, sock) -> None: ...
def communicate(self): ...
linger: bool
def close(self) -> None: ...
Expand Down
Loading
Loading