diff --git a/docs/source/reference-io.rst b/docs/source/reference-io.rst index 6ed76b0c42..06ee790069 100644 --- a/docs/source/reference-io.rst +++ b/docs/source/reference-io.rst @@ -243,6 +243,11 @@ And if you're implementing a server, you can use :class:`SSLListener`: :show-inheritance: :members: +Some methods on :class:`SSLStream` raise :exc:`NeedHandshakeError` if +you call them before the handshake completes: + +.. autoexception:: NeedHandshakeError + .. module:: trio.socket diff --git a/newsfragments/735.misc.rst b/newsfragments/735.misc.rst new file mode 100644 index 0000000000..6b1833760a --- /dev/null +++ b/newsfragments/735.misc.rst @@ -0,0 +1,7 @@ +There are a number of methods on :class:`trio.ssl.SSLStream` that +report information about the negotiated TLS connection, like +``selected_alpn_protocol``, and thus cannot succeed until after the +handshake has been performed. Previously, we returned None from these +methods, like the stdlib :mod:`ssl` module does, but this is +confusing, because that can also be a valid return value. Now we raise +:exc:`trio.ssl.NeedHandshakeError` instead. diff --git a/newsfragments/743.misc.rst b/newsfragments/743.misc.rst deleted file mode 100644 index d3606f5cb9..0000000000 --- a/newsfragments/743.misc.rst +++ /dev/null @@ -1,15 +0,0 @@ -Override the following methods in :class:`~ssl.SSLStream` which return `None` in case the handshake is not established: - -- :meth:`~ssl.SSLSocket.get_channel_binding`: https://docs.python.org/3/library/ssl.html#ssl.SSLSocket.get_channel_binding -- :meth:`~ssl.SSLSocket.selected_alpn_protocol`: https://docs.python.org/3/library/ssl.html#ssl.SSLSocket.selected_alpn_protocol -- :meth:`~ssl.SSLSocket.selected_npn_protocol`: https://docs.python.org/3/library/ssl.html#ssl.SSLSocket.selected_npn_protocol - -These methods raise now a `NoHandshakeError` if the handshake wasn't established. Note that they still return `None` in the cases when: - -- the other party does not support ALPN/NPN. -- the `SSLContext.set_alpn_protocols()` or `SSLContext.set_npn_protocols()` was not called. - -and in the case of :meth:`~ssl.SSLSocket.get_channel_binding` in the case when there is no connection. - -This feature is mostly concerned about the polish and user experience of the library, and references gripes like the one in -https://github.com/python-trio/trio/issues/735. \ No newline at end of file diff --git a/trio/__init__.py b/trio/__init__.py index c7478c831b..2cab1e5818 100644 --- a/trio/__init__.py +++ b/trio/__init__.py @@ -19,7 +19,7 @@ TrioInternalError, RunFinishedError, WouldBlock, Cancelled, BusyResourceError, ClosedResourceError, MultiError, run, open_nursery, open_cancel_scope, current_effective_deadline, TASK_STATUS_IGNORED, - current_time, BrokenResourceError, EndOfChannel, NoHandshakeError + current_time, BrokenResourceError, EndOfChannel ) from ._timeouts import ( diff --git a/trio/_core/__init__.py b/trio/_core/__init__.py index 5c7e46c941..95681622c1 100644 --- a/trio/_core/__init__.py +++ b/trio/_core/__init__.py @@ -16,8 +16,7 @@ def _public(fn): from ._exceptions import ( TrioInternalError, RunFinishedError, WouldBlock, Cancelled, - BusyResourceError, ClosedResourceError, BrokenResourceError, EndOfChannel, - NoHandshakeError + BusyResourceError, ClosedResourceError, BrokenResourceError, EndOfChannel ) from ._multierror import MultiError diff --git a/trio/_core/_exceptions.py b/trio/_core/_exceptions.py index 482c6e2c30..fd20406ae0 100644 --- a/trio/_core/_exceptions.py +++ b/trio/_core/_exceptions.py @@ -125,34 +125,6 @@ class BrokenResourceError(Exception): """ -class NoHandshakeError(Exception): - """Raised when a method like ``select_alpn_protocol`` is called - before the handshake is established. - - Some methods defined in the :class:`ssl.SSLSocket` from the stdlib - return ``None`` if the handshake hasn't happened yet. - - These are: - - - ``get_channel_binding``: https://docs.python.org/3/library/ssl.html#ssl.SSLSocket.get_channel_binding - - ``selected_alpn_protocol``: https://docs.python.org/3/library/ssl.html#ssl.SSLSocket.selected_alpn_protocol - - ``selected_npn_protocol``: https://docs.python.org/3/library/ssl.html#ssl.SSLSocket.selected_npn_protocol - - Note that these methods might also return ``None```in other cases. - - In case of calling `selected_alpn_protocol`` and ``selected_npn_protocol`` - other cases of returning ``None`` are: - - - If the other party does not support ALPN/NPN. - - If ``SSLContext.set_alpn_protocols()`` or ``SSLContext.set_npn_protocols()`` was not called. - - and in the case of ``get_channel_binding``: - - - If not connected. - - """ - - class EndOfChannel(Exception): """Raised when trying to receive from a :class:`trio.abc.ReceiveChannel` that has no more data to receive. diff --git a/trio/_ssl.py b/trio/_ssl.py index 2fedc03354..a8cdcb1983 100644 --- a/trio/_ssl.py +++ b/trio/_ssl.py @@ -164,6 +164,14 @@ ################################################################ +class NeedHandshakeError(Exception): + """Some :class:`SSLStream` methods can't return any meaningful data until + after the handshake. If you call them before the handshake, they raise + this error. + + """ + + class _Once: def __init__(self, afn, *args): self._afn = afn @@ -207,15 +215,6 @@ class SSLStream(Stream): documentation on SSL/TLS as well. SSL/TLS is subtle and quick to anger. Really. I'm not kidding. - To illustrate the point with an example, some of the methods of the - :class:`~ssl.SSLContext` return ``None`` when no handshake is established. - To make it behave more explicitly, we decided to raise `trio.core.NoHandshakeError` - in the :mod:`ssl` methods defined in ``_after_handshake``, - in case no handshake is established. - - Note that these methods still return ``None`` in other cases, as detailed - in ``trio.core.NoHandshakeError``. - Args: transport_stream (~trio.abc.Stream): The stream used to transport encrypted data. Required. @@ -290,6 +289,13 @@ class SSLStream(Stream): Internally, this class is implemented using an instance of :class:`ssl.SSLObject`, and all of :class:`~ssl.SSLObject`'s methods and attributes are re-exported as methods and attributes on this class. + However, there is one difference: :class:`~ssl.SSLObject` has several + methods that return information about the encrypted connection, like + :meth:`~ssl.SSLSocket.cipher` or + :meth:`~ssl.SSLSocket.selected_alpn_protocol`. If you call them before the + handshake, when they can't possibly return useful data, then + :class:`ssl.SSLObject` returns None, but :class:`trio.ssl.SSLStream` + raises :exc:`NeedHandshakeError`. This also means that if you register a SNI callback using :obj:`~ssl.SSLContext.sni_callback`, then the first argument your callback @@ -357,15 +363,23 @@ def __init__( } _after_handshake = { - "get_channel_binding", + "session_reused", + "getpeercert", "selected_npn_protocol", + "cipher", + "shared_ciphers", + "compression", + "get_channel_binding", "selected_alpn_protocol", + "version", } def __getattr__(self, name): if name in self._forwarded: if name in self._after_handshake and not self._handshook.done: - raise _core.NoHandshakeError + raise NeedHandshakeError( + "call do_handshake() before calling {!r}".format(name) + ) return getattr(self._ssl_object, name) else: diff --git a/trio/ssl.py b/trio/ssl.py index 8c8a07c045..80f26b5813 100644 --- a/trio/ssl.py +++ b/trio/ssl.py @@ -6,7 +6,7 @@ # here. # Trio-specific symbols: -from ._ssl import (SSLStream, SSLListener) +from ._ssl import SSLStream, SSLListener, NeedHandshakeError # Symbols re-exported from the stdlib ssl module: diff --git a/trio/tests/test_ssl.py b/trio/tests/test_ssl.py index 1c7a510110..e193691510 100644 --- a/trio/tests/test_ssl.py +++ b/trio/tests/test_ssl.py @@ -14,7 +14,7 @@ from .. import _core from .._highlevel_socket import SocketStream, SocketListener from .._highlevel_generic import aclose_forcefully -from .._core import ClosedResourceError, BrokenResourceError, NoHandshakeError +from .._core import ClosedResourceError, BrokenResourceError from .._highlevel_open_tcp_stream import open_tcp_stream from .. import ssl as tssl from .. import socket as tsocket @@ -1112,10 +1112,10 @@ async def client_side(cancel_scope): async def test_selected_alpn_protocol_before_handshake(): client, server = ssl_memory_stream_pair() - with pytest.raises(NoHandshakeError): + with pytest.raises(tssl.NeedHandshakeError): client.selected_alpn_protocol() - with pytest.raises(NoHandshakeError): + with pytest.raises(tssl.NeedHandshakeError): server.selected_alpn_protocol() @@ -1138,10 +1138,10 @@ async def test_selected_alpn_protocol_when_not_set(): async def test_selected_npn_protocol_before_handshake(): client, server = ssl_memory_stream_pair() - with pytest.raises(NoHandshakeError): + with pytest.raises(tssl.NeedHandshakeError): client.selected_npn_protocol() - with pytest.raises(NoHandshakeError): + with pytest.raises(tssl.NeedHandshakeError): server.selected_npn_protocol() @@ -1164,10 +1164,10 @@ async def test_selected_npn_protocol_when_not_set(): async def test_get_channel_binding_before_handshake(): client, server = ssl_memory_stream_pair() - with pytest.raises(NoHandshakeError): + with pytest.raises(tssl.NeedHandshakeError): client.get_channel_binding() - with pytest.raises(NoHandshakeError): + with pytest.raises(tssl.NeedHandshakeError): server.get_channel_binding()