Skip to content
3 changes: 3 additions & 0 deletions newsfragments/3413.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Upon a ``SSL.CertificateError``, a TLS alert is now sent to the peer before
raising ``trio.BrokenResourceError`` from the original error, preventing the
Comment on lines +1 to +2
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably both of these could use single backticks to link to the respective documentation.

client from hanging.
19 changes: 18 additions & 1 deletion src/trio/_ssl.py
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,8 @@ def __init__(

self._estimated_receive_size = STARTING_RECEIVE_SIZE

self._ssl_error: _stdlib_ssl.CertificateError | None = None

_forwarded: ClassVar = {
"context",
"server_side",
Expand Down Expand Up @@ -499,7 +501,15 @@ async def _retry(
ret = fn(*args)
except _stdlib_ssl.SSLWantReadError:
want_read = True
except (_stdlib_ssl.SSLError, _stdlib_ssl.CertificateError) as exc:
except _stdlib_ssl.CertificateError as exc:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So my understanding is that we need to do this in _retry rather than in do_handshake because renegotiation?

# We assume to have pending data in MemoryBIO: the TLS alert
# corresponding to the exception. As the alert needs to be
# sent to the peer, we stash the exception for now. We'll
# raise it after the alert is sent below, which will surely
# happen as sending has priority over receiving.
assert self._outgoing.pending
self._ssl_error = exc
except _stdlib_ssl.SSLError as exc:
self._state = _State.BROKEN
raise trio.BrokenResourceError from exc
else:
Expand Down Expand Up @@ -633,6 +643,13 @@ async def _retry(
)
self._incoming.write(data)
self._inner_recv_count += 1
if self._ssl_error:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not convinced we need to store the error in self._ssl_error...

# Raise the stashed SSLError exception.
self._state = _State.BROKEN
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One concern I just had is that maybe it would be better to move this earlier (i.e. within the except), since then we can't get multiple certificate errors. As is, I think if multiple tasks enter _retry on the same tick, that might be a situation where there isn't outgoing data to send? (and just generally unknown behavior)

# Not sure if this is necessary, but let's clean up
# self._ssl_error just in case.
self._ssl_error, ssl_error = None, self._ssl_error
raise trio.BrokenResourceError from ssl_error
Comment on lines +649 to +652
Copy link
Copy Markdown
Contributor

@A5rocks A5rocks May 9, 2026

Choose a reason for hiding this comment

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

I think this doesn't work; the frame still has a reference to ssl_error I think? Maybe add a test to make sure :^) (or just check that from e doesn't erase e from the frame, because that sounds like the sort of trick Python would do)

Anyways, the right way is probably:

Suggested change
# Not sure if this is necessary, but let's clean up
# self._ssl_error just in case.
self._ssl_error, ssl_error = None, self._ssl_error
raise trio.BrokenResourceError from ssl_error
try:
raise trio.BrokenResourceError from self._ssl_error
finally:
del self._ssl_error

if not yielded:
await trio.lowlevel.cancel_shielded_checkpoint()
return ret
Expand Down
158 changes: 141 additions & 17 deletions src/trio/_tests/test_ssl.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import datetime
import os
import socket as stdlib_socket
import ssl
Expand Down Expand Up @@ -77,6 +78,13 @@

TRIO_TEST_1_CERT.configure_cert(SERVER_CTX)

SERVER_CTX_REQ = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH)
if hasattr(ssl, "OP_IGNORE_UNEXPECTED_EOF"):
SERVER_CTX_REQ.options &= ~ssl.OP_IGNORE_UNEXPECTED_EOF
TRIO_TEST_1_CERT.configure_cert(SERVER_CTX_REQ)
SERVER_CTX_REQ.verify_mode = ssl.CERT_REQUIRED
TRIO_TEST_CA.configure_trust(SERVER_CTX_REQ)


# TLS 1.3 has a lot of changes from previous versions. So we want to run tests
# with both TLS 1.3, and TLS 1.2.
Expand Down Expand Up @@ -105,7 +113,10 @@ def client_ctx(request: pytest.FixtureRequest) -> ssl.SSLContext:
def ssl_echo_serve_sync(
sock: stdlib_socket.socket,
*,
expect_fail: bool = False,
# expect_fail = "raise" expects to fail but raises nevertheless, i.e. it is
# like False but does not print; used where the raised exception is checked
# in the main thread.
expect_fail: bool | str = False,
) -> None:
try:
wrapped = SERVER_CTX.wrap_socket(
Expand Down Expand Up @@ -142,7 +153,9 @@ def ssl_echo_serve_sync(
except (ConnectionResetError, ConnectionAbortedError): # pragma: no cover
return
except Exception as exc:
if expect_fail:
if expect_fail == "raise":
raise
elif expect_fail:
print("ssl_echo_serve_sync got error as expected:", exc)
else: # pragma: no cover
print("ssl_echo_serve_sync got unexpected error:", exc)
Expand All @@ -158,7 +171,9 @@ def ssl_echo_serve_sync(
# (running in a thread). Useful for testing making connections with different
# SSLContexts.
@asynccontextmanager
async def ssl_echo_server_raw(expect_fail: bool = False) -> AsyncIterator[SocketStream]:
async def ssl_echo_server_raw(
expect_fail: bool | str = False,
) -> AsyncIterator[SocketStream]:
a, b = stdlib_socket.socketpair()
async with trio.open_nursery() as nursery:
# Exiting the 'with a, b' context manager closes the sockets, which
Expand All @@ -178,7 +193,7 @@ async def ssl_echo_server_raw(expect_fail: bool = False) -> AsyncIterator[Socket
@asynccontextmanager
async def ssl_echo_server(
client_ctx: SSLContext,
expect_fail: bool = False,
expect_fail: bool | str = False,
) -> AsyncIterator[SSLStream[Stream]]:
async with ssl_echo_server_raw(expect_fail=expect_fail) as sock:
yield SSLStream(sock, client_ctx, server_hostname="trio-test-1.example.org")
Expand Down Expand Up @@ -453,21 +468,33 @@ async def test_ssl_client_basics(client_ctx: SSLContext) -> None:
await s.aclose()

# Didn't configure the CA file, should fail
async with ssl_echo_server_raw(expect_fail=True) as sock:
bad_client_ctx = ssl.create_default_context()
s = SSLStream(sock, bad_client_ctx, server_hostname="trio-test-1.example.org")
assert not s.server_side
with pytest.raises(BrokenResourceError) as excinfo:
await s.send_all(b"x")
assert isinstance(excinfo.value.__cause__, ssl.SSLError)
# Check that the server receives TLSV1_ALERT_UNKNOWN_CA
# rather than choking with UNEXPECTED_EOF_WHILE_READING.
with pytest.RaisesGroup(
pytest.RaisesExc(ssl.SSLError, match="TLSV1_ALERT_UNKNOWN_CA")
):
async with ssl_echo_server_raw(expect_fail="raise") as sock:
bad_client_ctx = ssl.create_default_context()
s = SSLStream(
sock, bad_client_ctx, server_hostname="trio-test-1.example.org"
)
assert not s.server_side
with pytest.raises(BrokenResourceError) as excinfo1:
await s.send_all(b"x")
assert isinstance(excinfo1.value.__cause__, ssl.SSLError)

# Trusted CA, but wrong host name
async with ssl_echo_server_raw(expect_fail=True) as sock:
s = SSLStream(sock, client_ctx, server_hostname="trio-test-2.example.org")
assert not s.server_side
with pytest.raises(BrokenResourceError) as excinfo:
await s.send_all(b"x")
assert isinstance(excinfo.value.__cause__, ssl.CertificateError)
# Check that the server receives SSLV3_ALERT_BAD_CERTIFICATE
# rather than choking with UNEXPECTED_EOF_WHILE_READING.
with pytest.RaisesGroup(
pytest.RaisesExc(ssl.SSLError, match="SSLV3_ALERT_BAD_CERTIFICATE")
):
async with ssl_echo_server_raw(expect_fail="raise") as sock:
s = SSLStream(sock, client_ctx, server_hostname="trio-test-2.example.org")
assert not s.server_side
with pytest.raises(BrokenResourceError) as excinfo2:
await s.send_all(b"x")
assert isinstance(excinfo2.value.__cause__, ssl.CertificateError)


async def test_ssl_server_basics(client_ctx: SSLContext) -> None:
Expand Down Expand Up @@ -503,6 +530,103 @@ def client() -> None:
t.join()


async def test_client_certificate(client_ctx: SSLContext) -> None:

# A valid client certificate
client_cert = TRIO_TEST_CA.issue_cert("user@example.org")
client_cert.configure_cert(client_ctx)

a, b = stdlib_socket.socketpair()
with a, b:
server_sock = tsocket.from_stdlib_socket(b)
server_transport = SSLStream(
SocketStream(server_sock),
SERVER_CTX_REQ,
server_side=True,
)
assert server_transport.server_side

def client() -> None:
with client_ctx.wrap_socket(
a,
server_hostname="trio-test-1.example.org",
) as client_sock:
client_sock.sendall(b"x")
assert client_sock.recv(1) == b"y"
client_sock.sendall(b"z")
client_sock.unwrap()

t = threading.Thread(target=client)
t.start()

assert await server_transport.receive_some(1) == b"x"
await server_transport.send_all(b"y")
assert await server_transport.receive_some(1) == b"z"
assert await server_transport.receive_some(1) == b""
await server_transport.aclose()

t.join()

# An expired client certificate: this should fail
client_cert = TRIO_TEST_CA.issue_cert(
"user@example.org", not_after=datetime.datetime.now(datetime.timezone.utc)
)
client_cert.configure_cert(client_ctx)

a, b = stdlib_socket.socketpair()
with a, b:
server_sock = tsocket.from_stdlib_socket(b)
server_transport = SSLStream(
SocketStream(server_sock),
SERVER_CTX_REQ,
server_side=True,
)
assert server_transport.server_side

# Prior to the changes to _ssl.py made in this commit, this test hangs
# without the timeout introduced below. With the new _ssl.py,
# pytest.raises in the client successfully catches the error; the
# thread therefore finishes, setting client_done. With the old
# _ssl.py, the thread hangs and will be abandoned after the timeout,
# leaving client_done unset and thereby triggering the assertion error.
# (The general timeout imposed on tests is not only too long for this,
# but it also doesn't work, because the thread is not abandoned.)
#
# Potential problem: determinism. It is highly unlikely but I guess it
# could happen that the client thread doesn't get from .recv to
# client_done.set in the allotted time, resulting in a false negative.

client_done = trio.Event()

def client() -> None:
# For TLS 1.3, pytest.raises is ok around .sendall below, but it
# needs to be here for TLS 1.2. (Is this because TLS 1.2 verifies
# client-side certificates during the initial handshake?)
with pytest.raises(ssl.SSLError, match="SSLV3_ALERT_CERTIFICATE_EXPIRED"):
with client_ctx.wrap_socket(
a,
server_hostname="trio-test-1.example.org",
) as client_sock:
client_sock.sendall(b"x")
client_sock.recv(1)
trio.from_thread.run_sync(client_done.set)

async with trio.open_nursery() as nursery:
nursery.start_soon(
partial(trio.to_thread.run_sync, client, abandon_on_cancel=True)
)
with pytest.raises(BrokenResourceError) as excinfo:
assert await server_transport.receive_some(1) == b"x"
assert isinstance(excinfo.value.__cause__, ssl.SSLError)
assert excinfo.value.__cause__.reason == "CERTIFICATE_VERIFY_FAILED"
# This timeout shouldn't affect the execution time of the test on
# healthy code.
with trio.move_on_after(0.1):
await client_done.wait()
nursery.cancel_scope.cancel()
assert client_done.is_set(), "The client thread is hanging"


async def test_attributes(client_ctx: SSLContext) -> None:
async with ssl_echo_server_raw(expect_fail=True) as sock:
good_ctx = client_ctx
Expand Down
Loading