Workaround OpenSSL 1.1.1 session-ticket issue

newsfragments/819.bugfix.rst

@@ -0,0 +1,6 @@
+OpenSSL has a bug in its handling of TLS 1.3 session tickets that can
+cause deadlocks or data loss in some rare edge cases. These edge cases
+most frequently happen during tests. (OpenSSL bugs: `#7948
+<https://github.com/openssl/openssl/issues/7948>`__, `#7967
+<https://github.com/openssl/openssl/issues/7967>`__.) `trio.SSLStream`
+now works around this issue, so you don't have to worry about it.

trio/_ssl.py

@@ -338,6 +338,7 @@ def __init__(
             )
         self._https_compatible = https_compatible
         self._outgoing = _stdlib_ssl.MemoryBIO()
+        self._delayed_outgoing = None
         self._incoming = _stdlib_ssl.MemoryBIO()
         self._ssl_object = ssl_context.wrap_bio(
             self._incoming,
@@ -428,7 +429,9 @@ def _check_status(self):
     # comments, though, just make sure to think carefully if you ever have to
     # touch it. The big comment at the top of this file will help explain
     # too.
-    async def _retry(self, fn, *args, ignore_want_read=False):
+    async def _retry(
+        self, fn, *args, ignore_want_read=False, is_handshake=False
+    ):
         await trio.hazmat.checkpoint_if_cancelled()
         yielded = False
         finished = False
@@ -473,6 +476,32 @@ async def _retry(self, fn, *args, ignore_want_read=False):
                 finished = True
             to_send = self._outgoing.read()
 
+            # Some versions of SSL_do_handshake have a bug in how they handle
+            # the TLS 1.3 handshake on the server side: after the handshake
+            # finishes, they automatically send session tickets, even though
+            # the client may not be expecting data to arrive at this point and
+            # sending it could cause a deadlock or lost data. This applies at
+            # least to OpenSSL 1.1.1c and earlier, and the OpenSSL devs
+            # currently have no plans to fix it:
+            #
+            #   https://github.com/openssl/openssl/issues/7948
+            #   https://github.com/openssl/openssl/issues/7967
+            #
+            # The correct behavior is to wait to send session tickets on the
+            # first call to SSL_write. (This is what BoringSSL does.) So, we
+            # use a heuristic to detect when OpenSSL has tried to send session
+            # tickets, and we manually delay sending them until the
+            # appropriate moment. For more discussion see:
+            #
+            #   https://github.com/python-trio/trio/issues/819#issuecomment-517529763
+            if (
+                is_handshake and not want_read and self._ssl_object.server_side
+                and self._ssl_object.version() == "TLSv1.3"
+            ):
+                assert self._delayed_outgoing is None
+                self._delayed_outgoing = to_send
+                to_send = b""
+
             # Outputs from the above code block are:
             #
             # - to_send: bytestring; if non-empty then we need to send
@@ -535,6 +564,9 @@ async def _retry(self, fn, *args, ignore_want_read=False):
                 async with self._inner_send_lock:
                     yielded = True
                     try:
+                        if self._delayed_outgoing is not None:
+                            to_send = self._delayed_outgoing + to_send
+                            self._delayed_outgoing = None
                         await self.transport_stream.send_all(to_send)
                     except:
                         # Some unknown amount of our data got sent, and we
@@ -571,7 +603,7 @@ async def _retry(self, fn, *args, ignore_want_read=False):
 
     async def _do_handshake(self):
         try:
-            await self._retry(self._ssl_object.do_handshake)
+            await self._retry(self._ssl_object.do_handshake, is_handshake=True)
         except:
             self._state = _State.BROKEN
             raise

trio/tests/test_highlevel_ssl_helpers.py

@@ -7,7 +7,7 @@
 import trio
 from trio.socket import AF_INET, SOCK_STREAM, IPPROTO_TCP
 import trio.testing
-from .test_ssl import CLIENT_CTX, SERVER_CTX
+from .test_ssl import client_ctx, SERVER_CTX
 
 from .._highlevel_ssl_helpers import (
     open_ssl_over_tcp_stream, open_ssl_over_tcp_listeners, serve_ssl_over_tcp
@@ -40,7 +40,10 @@ async def getnameinfo(self, *args):  # pragma: no cover
 
 
 # This uses serve_ssl_over_tcp, which uses open_ssl_over_tcp_listeners...
-async def test_open_ssl_over_tcp_stream_and_everything_else():
+# noqa is needed because flake8 doesn't understand how pytest fixtures work.
+async def test_open_ssl_over_tcp_stream_and_everything_else(
+    client_ctx,  # noqa: F811
+):
     async with trio.open_nursery() as nursery:
         (listener,) = await nursery.start(
             partial(
@@ -66,7 +69,7 @@ async def test_open_ssl_over_tcp_stream_and_everything_else():
         stream = await open_ssl_over_tcp_stream(
             "xyzzy.example.org",
             80,
-            ssl_context=CLIENT_CTX,
+            ssl_context=client_ctx,
         )
         with pytest.raises(trio.BrokenResourceError):
             await stream.do_handshake()
@@ -75,7 +78,7 @@ async def test_open_ssl_over_tcp_stream_and_everything_else():
         stream = await open_ssl_over_tcp_stream(
             "trio-test-1.example.org",
             80,
-            ssl_context=CLIENT_CTX,
+            ssl_context=client_ctx,
         )
         assert isinstance(stream, trio.SSLStream)
         assert stream.server_hostname == "trio-test-1.example.org"
@@ -88,7 +91,7 @@ async def test_open_ssl_over_tcp_stream_and_everything_else():
         stream = await open_ssl_over_tcp_stream(
             "trio-test-1.example.org",
             80,
-            ssl_context=CLIENT_CTX,
+            ssl_context=client_ctx,
             https_compatible=True,
             # also, smoke test happy_eyeballs_delay
             happy_eyeballs_delay=1,