bpo-33734: asyncio/ssl: a bunch of bugfixes (GH-7321) (GH-7396)
* Fix AttributeError (not all SSL exceptions have 'errno' attribute)
* Increase default handshake timeout from 10 to 60 seconds
* Make sure start_tls can be cancelled correctly
* Make sure any error in SSLProtocol gets propagated (instead of just being logged)
(cherry picked from commit 9602643120a509858d0bee4215d7f150e6125468)
Co-authored-by: Yury Selivanov <yury@magic.io>
diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py
index 61938e9..34cc625 100644
--- a/Lib/asyncio/base_events.py
+++ b/Lib/asyncio/base_events.py
@@ -1114,7 +1114,12 @@
self.call_soon(ssl_protocol.connection_made, transport)
self.call_soon(transport.resume_reading)
- await waiter
+ try:
+ await waiter
+ except Exception:
+ transport.close()
+ raise
+
return ssl_protocol._app_transport
async def create_datagram_endpoint(self, protocol_factory,
diff --git a/Lib/asyncio/constants.py b/Lib/asyncio/constants.py
index d7ba496..33feed6 100644
--- a/Lib/asyncio/constants.py
+++ b/Lib/asyncio/constants.py
@@ -12,7 +12,8 @@
DEBUG_STACK_DEPTH = 10
# Number of seconds to wait for SSL handshake to complete
-SSL_HANDSHAKE_TIMEOUT = 10.0
+# The default timeout matches that of Nginx.
+SSL_HANDSHAKE_TIMEOUT = 60.0
# Used in sendfile fallback code. We use fallback for platforms
# that don't support sendfile, or for TLS connections.
diff --git a/Lib/asyncio/events.py b/Lib/asyncio/events.py
index 40946bb..e4e6322 100644
--- a/Lib/asyncio/events.py
+++ b/Lib/asyncio/events.py
@@ -352,8 +352,7 @@
ssl_handshake_timeout is the time in seconds that an SSL server
will wait for completion of the SSL handshake before aborting the
- connection. Default is 10s, longer timeouts may increase vulnerability
- to DoS attacks (see https://support.f5.com/csp/article/K13834)
+ connection. Default is 60s.
start_serving set to True (default) causes the created server
to start accepting connections immediately. When set to False,
@@ -411,7 +410,7 @@
accepted connections.
ssl_handshake_timeout is the time in seconds that an SSL server
- will wait for the SSL handshake to complete (defaults to 10s).
+ will wait for the SSL handshake to complete (defaults to 60s).
start_serving set to True (default) causes the created server
to start accepting connections immediately. When set to False,
diff --git a/Lib/asyncio/sslproto.py b/Lib/asyncio/sslproto.py
index a6d382e..8515ec5 100644
--- a/Lib/asyncio/sslproto.py
+++ b/Lib/asyncio/sslproto.py
@@ -214,13 +214,14 @@
# Drain possible plaintext data after close_notify.
appdata.append(self._incoming.read())
except (ssl.SSLError, ssl.CertificateError) as exc:
- if getattr(exc, 'errno', None) not in (
+ exc_errno = getattr(exc, 'errno', None)
+ if exc_errno not in (
ssl.SSL_ERROR_WANT_READ, ssl.SSL_ERROR_WANT_WRITE,
ssl.SSL_ERROR_SYSCALL):
if self._state == _DO_HANDSHAKE and self._handshake_cb:
self._handshake_cb(exc)
raise
- self._need_ssldata = (exc.errno == ssl.SSL_ERROR_WANT_READ)
+ self._need_ssldata = (exc_errno == ssl.SSL_ERROR_WANT_READ)
# Check for record level data that needs to be sent back.
# Happens for the initial handshake and renegotiations.
@@ -263,13 +264,14 @@
# It is not allowed to call write() after unwrap() until the
# close_notify is acknowledged. We return the condition to the
# caller as a short write.
+ exc_errno = getattr(exc, 'errno', None)
if exc.reason == 'PROTOCOL_IS_SHUTDOWN':
- exc.errno = ssl.SSL_ERROR_WANT_READ
- if exc.errno not in (ssl.SSL_ERROR_WANT_READ,
+ exc_errno = exc.errno = ssl.SSL_ERROR_WANT_READ
+ if exc_errno not in (ssl.SSL_ERROR_WANT_READ,
ssl.SSL_ERROR_WANT_WRITE,
ssl.SSL_ERROR_SYSCALL):
raise
- self._need_ssldata = (exc.errno == ssl.SSL_ERROR_WANT_READ)
+ self._need_ssldata = (exc_errno == ssl.SSL_ERROR_WANT_READ)
# See if there's any record level data back for us.
if self._outgoing.pending:
@@ -488,6 +490,12 @@
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)
@@ -515,11 +523,8 @@
try:
ssldata, appdata = self._sslpipe.feed_ssldata(data)
- except ssl.SSLError as e:
- if self._loop.get_debug():
- logger.warning('%r: SSL error %s (reason %s)',
- self, e.errno, e.reason)
- self._abort()
+ except Exception as e:
+ self._fatal_error(e, 'SSL error in data received')
return
for chunk in ssldata:
@@ -602,8 +607,12 @@
def _check_handshake_timeout(self):
if self._in_handshake is True:
- logger.warning("%r stalled during handshake", self)
- self._abort()
+ msg = (
+ f"SSL handshake 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
@@ -615,21 +624,13 @@
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 = 'SSL handshake failed on verifying the certificate'
else:
- raise
+ msg = 'SSL handshake failed'
+ self._fatal_error(exc, msg)
+ return
if self._loop.get_debug():
dt = self._loop.time() - self._handshake_start_time
@@ -686,18 +687,14 @@
# delete it and reduce the outstanding buffer size.
del self._write_backlog[0]
self._write_buffer_size -= len(data)
- except BaseException as exc:
+ except Exception as exc:
if self._in_handshake:
- # BaseExceptions will be re-raised in _on_handshake_complete.
+ # Exceptions will be re-raised in _on_handshake_complete.
self._on_handshake_complete(exc)
else:
self._fatal_error(exc, 'Fatal error on SSL transport')
- if not isinstance(exc, Exception):
- # BaseException
- raise
def _fatal_error(self, exc, message='Fatal error on transport'):
- # Should be called from exception handler only.
if isinstance(exc, base_events._FATAL_ERROR_IGNORE):
if self._loop.get_debug():
logger.debug("%r: %s", self, message, exc_info=True)