Bugfix: Send the TLS alert to the peer upon a ssl.CertificateError.

This commit (partially) addresses python-trio/trio-websocket#199, which I
traced to a bug in Trio itself.  A corresponding bugfix relying on the present
bugfix will be submitted to the Trio Websocket repo.

From the user's perspective, the issue was that a TLS client hanged after
submitting an invalid (e.g. expired) client certificate.

Before this fix, when `SSLStream._retry` caught a `ssl.CertificateError`, the
error was immediately re-raised (wrapped in a `trio.BrokenResourceError`).  The
TLS alert prepared by the `ssl` library and waiting in MemoryBIO was therefore
never sent to the peer.

Now, upon catching a `ssl.CertificateError`, we first check whether we have any
pending outgoing data (`self._outgoing.pending`).  If so, the exception is
stashed away and only raised (again, wrapped in a `trio.BrokenResourceError`)
after sending out the pending data (which should contain the alert).

I had first tried an alternative implementation, where the `CertificateError`
was not stashed away.  Rather, the error was raised immediately, but only if
there was no pending outgoing data.  The idea was to rely on the loop in
`SSLStream._retry`.  Upon seeing `CertificateError` for the first time, there
would be pending data, so the exception would not be reraised and the pending
data would be sent out, while upon seeing the `CertificateError` for the second
time, there would be no pending data, so the exception would be re-raised.
However, it turned out that the loop did not always continue (I'm not sure
why), so there was no second time in some situations.

TESTS in `test_ssl.py`

`test_ssl_client_basics` (modified)

Here we test whether the TLS alert sent out by the client reaches
the (blocking) server.

The second (no CA file) and the third (wrong host name) subtest of this test
were modified to check that the server encounters the correct SSL error.  In
the old code, the server encountered `UNEXPECTED_EOF_WHILE_READING` (protocol
error) in both subtests.  After the fix, it correctly receives
`TLSV1_ALERT_UNKNOWN_CA` and `SSLV3_ALERT_BAD_CERTIFICATE`, respectively.

To facilitate the modified test, function `ssl_echo_serve_sync` (called by
`ssl_echo_server_raw` called by this test) now allows for a special value of
keyword argument `expect_fail`: `"raise"`.  When given this value, the error is
expected but raised nevertheless, the idea being that it should be caught and
inspected in the test code.

`test_client_certificate` (new)

Here we test whether the TLS alert sent out by the server reaches
the (blocking) client.  The test is modeled on `test_ssl_server_basics`.

The server is configured to require client authentication (`SERVER_CTX_REQ`,
defined at the top of the file).  In the first subtest, the client submits a
valid certificate; in the second subtest, an expired one.

There is a complication with the second subtest.  If the client does not send
out the TLS alert (like before the bugfix), the server hangs, but we don't want
the test to hang.  I could think of no other way to test whether the server
hangs than imposing an arbitrary (small) timeout, and there is a (very) small
chance that even the correct code will not finish within the allotted time,
resulting in a false negative.

Author: sasozivanovic

newsfragments/NNNN.bugfix.rst

@@ -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
+client from hanging.

src/trio/_ssl.py

@@ -394,6 +394,8 @@ def __init__(
 
         self._estimated_receive_size = STARTING_RECEIVE_SIZE
 
+        self._ssl_error = None
+
     _forwarded: ClassVar = {
         "context",
         "server_side",
@@ -499,7 +501,19 @@ 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:
+                if self._outgoing.pending:
+                    # We have pending data in MemoryBIO.  The assumption is
+                    # that this is 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.
+                    self._ssl_error = exc
+                else:
+                    self._state = _State.BROKEN
+                    raise trio.BrokenResourceError from exc
+            except _stdlib_ssl.SSLError as exc:
                 self._state = _State.BROKEN
                 raise trio.BrokenResourceError from exc
             else:
@@ -633,6 +647,13 @@ async def _retry(
                             )
                             self._incoming.write(data)
                         self._inner_recv_count += 1
+            if self._ssl_error:
+                # Raise the stashed SSLError exception.
+                self._state = _State.BROKEN
+                # 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
         if not yielded:
             await trio.lowlevel.cancel_shielded_checkpoint()
         return ret

src/trio/_tests/test_ssl.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import datetime
 import os
 import socket as stdlib_socket
 import ssl
@@ -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.
@@ -105,6 +113,9 @@ def client_ctx(request: pytest.FixtureRequest) -> ssl.SSLContext:
 def ssl_echo_serve_sync(
     sock: stdlib_socket.socket,
     *,
+    # 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 = False,
 ) -> None:
     try:
@@ -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)
@@ -453,21 +466,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")
+    ) as excinfo:
+        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 excinfo:
+                await s.send_all(b"x")
+            assert isinstance(excinfo.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")
+    ) as excinfo:
+        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 excinfo:
+                await s.send_all(b"x")
+            assert isinstance(excinfo.value.__cause__, ssl.CertificateError)
 
 
 async def test_ssl_server_basics(client_ctx: SSLContext) -> None:
@@ -503,6 +528,101 @@ 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.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.
+        #
+        # 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