Fix SSLProtocol to correctly propagate errors and abort connections

Add functional tests

Author: 1st1

Lib/asyncio/sslproto.py

@@ -7,6 +7,7 @@
 
 from . import base_events
 from . import constants
+from . import futures
 from . import protocols
 from . import transports
 from .log import logger
@@ -490,6 +491,12 @@ def connection_lost(self, exc):
         if self._session_established:
             self._session_established = False
             self._loop.call_soon(self._app_protocol.connection_lost, exc)
+        else:
+            # Most likely an exception occurred while in SSL handshake.
+            # Just mark the app transport as closed so that its __del__
+            # doesn't complain.
+            if self._app_transport is not None:
+                self._app_transport._closed = True
         self._transport = None
         self._app_transport = None
         self._wakeup_waiter(exc)
@@ -605,10 +612,12 @@ def _start_handshake(self):
 
     def _check_handshake_timeout(self):
         if self._in_handshake is True:
-            logger.warning(
-                "SSL handshake for %r is taking longer than %r seconds: "
-                "aborting the connection", self, self._ssl_handshake_timeout)
-            self._abort()
+            msg = (
+                f"SSL handshake for {self} is taking longer than "
+                f"{self._ssl_handshake_timeout} seconds: "
+                f"aborting the connection"
+            )
+            self._fatal_error(ConnectionAbortedError(msg))
 
     def _on_handshake_complete(self, handshake_exc):
         self._in_handshake = False
@@ -620,21 +629,16 @@ def _on_handshake_complete(self, handshake_exc):
                 raise handshake_exc
 
             peercert = sslobj.getpeercert()
-        except BaseException as exc:
-            if self._loop.get_debug():
-                if isinstance(exc, ssl.CertificateError):
-                    logger.warning("%r: SSL handshake failed "
-                                   "on verifying the certificate",
-                                   self, exc_info=True)
-                else:
-                    logger.warning("%r: SSL handshake failed",
-                                   self, exc_info=True)
-            self._transport.close()
-            if isinstance(exc, Exception):
-                self._wakeup_waiter(exc)
-                return
+        except Exception as exc:
+            if isinstance(exc, ssl.CertificateError):
+                msg = (
+                    f'{self}: SSL handshake failed on verifying '
+                    f'the certificate'
+                )
             else:
-                raise
+                msg = f'{self}: SSL handshake failed'
+            self._fatal_error(exc, msg)
+            return
 
         if self._loop.get_debug():
             dt = self._loop.time() - self._handshake_start_time
@@ -702,19 +706,19 @@ def _process_write_backlog(self):
                 raise
 
     def _fatal_error(self, exc, message='Fatal error on transport'):
-        # Should be called from exception handler only.
+        if self._transport:
+            self._transport._force_close(exc)
+
         if isinstance(exc, base_events._FATAL_ERROR_IGNORE):
             if self._loop.get_debug():
                 logger.debug("%r: %s", self, message, exc_info=True)
-        else:
+        elif not isinstance(exc, futures.CancelledError):
             self._loop.call_exception_handler({
                 'message': message,
                 'exception': exc,
                 'transport': self._transport,
                 'protocol': self,
             })
-        if self._transport:
-            self._transport._force_close(exc)
 
     def _finalize(self):
         self._sslpipe = None

Lib/test/test_asyncio/test_sslproto.py

@@ -49,35 +49,6 @@ def mock_handshake(callback):
             ssl_proto.connection_made(transport)
         return transport
 
-    def test_cancel_handshake(self):
-        # Python issue #23197: cancelling a handshake must not raise an
-        # exception or log an error, even if the handshake failed
-        waiter = asyncio.Future(loop=self.loop)
-        ssl_proto = self.ssl_protocol(waiter=waiter)
-        handshake_fut = asyncio.Future(loop=self.loop)
-
-        def do_handshake(callback):
-            exc = Exception()
-            callback(exc)
-            handshake_fut.set_result(None)
-            return []
-
-        waiter.cancel()
-        self.connection_made(ssl_proto, do_handshake=do_handshake)
-
-        with test_utils.disable_logger():
-            self.loop.run_until_complete(handshake_fut)
-
-    def test_handshake_timeout(self):
-        # bpo-29970: Check that a connection is aborted if handshake is not
-        # completed in timeout period, instead of remaining open indefinitely
-        ssl_proto = self.ssl_protocol()
-        transport = self.connection_made(ssl_proto)
-
-        with test_utils.disable_logger():
-            self.loop.run_until_complete(tasks.sleep(0.2, loop=self.loop))
-        self.assertTrue(transport.abort.called)
-
     def test_handshake_timeout_zero(self):
         sslcontext = test_utils.dummy_ssl_context()
         app_proto = mock.Mock()
@@ -477,6 +448,116 @@ async def main():
 
         self.loop.run_until_complete(main())
 
+    def test_handshake_timeout(self):
+        # bpo-29970: Check that a connection is aborted if handshake is not
+        # completed in timeout period, instead of remaining open indefinitely
+        client_sslctx = test_utils.simple_client_sslcontext()
+
+        # silence error logger
+        messages = []
+        self.loop.set_exception_handler(lambda loop, ctx: messages.append(ctx))
+
+        server_side_aborted = False
+
+        def server(sock):
+            nonlocal server_side_aborted
+            try:
+                sock.recv_all(1024 * 1024)
+            except ConnectionAbortedError:
+                server_side_aborted = True
+            finally:
+                sock.close()
+
+        async def client(addr):
+            await asyncio.wait_for(
+                self.loop.create_connection(
+                    asyncio.Protocol,
+                    *addr,
+                    ssl=client_sslctx,
+                    server_hostname='',
+                    ssl_handshake_timeout=10.0),
+                0.5,
+                loop=self.loop)
+
+        with self.tcp_server(server,
+                             max_clients=1,
+                             backlog=1) as srv:
+
+            with self.assertRaises(asyncio.TimeoutError):
+                self.loop.run_until_complete(client(srv.addr))
+
+        self.assertTrue(server_side_aborted)
+
+        # Python issue #23197: cancelling a handshake must not raise an
+        # exception or log an error, even if the handshake failed
+        self.assertEqual(messages, [])
+
+    def test_create_connection_ssl_slow_handshake(self):
+        client_sslctx = test_utils.simple_client_sslcontext()
+
+        # silence error logger
+        self.loop.set_exception_handler(lambda *args: None)
+
+        def server(sock):
+            try:
+                sock.recv_all(1024 * 1024)
+            except ConnectionAbortedError:
+                pass
+            finally:
+                sock.close()
+
+        async def client(addr):
+            reader, writer = await asyncio.open_connection(
+                *addr,
+                ssl=client_sslctx,
+                server_hostname='',
+                loop=self.loop,
+                ssl_handshake_timeout=1.0)
+
+        with self.tcp_server(server,
+                             max_clients=1,
+                             backlog=1) as srv:
+
+            with self.assertRaisesRegex(
+                    ConnectionAbortedError,
+                    r'SSL handshake.*is taking longer'):
+
+                self.loop.run_until_complete(client(srv.addr))
+
+    def test_create_connection_ssl_failed_certificate(self):
+        # silence error logger
+        self.loop.set_exception_handler(lambda *args: None)
+
+        sslctx = test_utils.simple_server_sslcontext()
+        client_sslctx = test_utils.simple_client_sslcontext(
+            disable_verify=False)
+
+        def server(sock):
+            try:
+                sock.start_tls(
+                    sslctx,
+                    server_side=True)
+                sock.connect()
+            except ssl.SSLError:
+                pass
+            finally:
+                sock.close()
+
+        async def client(addr):
+            reader, writer = await asyncio.open_connection(
+                *addr,
+                ssl=client_sslctx,
+                server_hostname='',
+                loop=self.loop,
+                ssl_handshake_timeout=1.0)
+
+        with self.tcp_server(server,
+                             max_clients=1,
+                             backlog=1) as srv:
+
+            with self.assertRaises(ssl.SSLCertVerificationError):
+                self.loop.run_until_complete(client(srv.addr))
+
 
 @unittest.skipIf(ssl is None, 'No ssl module')
 class SelectorStartTLSTests(BaseStartTLS, unittest.TestCase):

Lib/test/test_asyncio/utils.py

@@ -77,10 +77,11 @@ def simple_server_sslcontext():
     return server_context
 
 
-def simple_client_sslcontext():
+def simple_client_sslcontext(*, disable_verify=True):
     client_context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
     client_context.check_hostname = False
-    client_context.verify_mode = ssl.CERT_NONE
+    if disable_verify:
+        client_context.verify_mode = ssl.CERT_NONE
     return client_context