Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 82 additions & 17 deletions peps/pep-0748.rst
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,15 @@ The ``ClientContext`` protocol class has the following class definition:
...

@abstractmethod
def connect(self, address: tuple[str | None, int]) -> TLSSocket:
def create_connection(
Comment thread
Julien00859 marked this conversation as resolved.
self,
address: tuple[str | None, bytes | str | int | None],
timeout: float | None = ...,
source_address: _Address | None = None,
*,
all_errors: bool = False,
sever_hostname: str | None = None,
) -> TLSSocket:
"""Creates a TLSSocket that behaves like a socket.socket, and
contains information about the TLS exchange
(cipher, negotiated_protocol, negotiated_tls_version, etc.).
Expand All @@ -365,7 +373,45 @@ The ``ClientContext`` protocol class has the following class definition:
(cipher, negotiated_protocol, negotiated_tls_version, etc.)."""
...

The ``ServerContext`` is similar, taking a ``TLSServerConfiguration`` instead.
The ``ServerContext`` protocol class has the following class definition:

.. code-block:: python

class ServerContext(Protocol):
@abstractmethod
def __init__(self, configuration: TLSServerConfiguration) -> None:
"""Create a new server context object from a given TLS server configuration."""
...

@property
@abstractmethod
def configuration(self) -> TLSServerConfiguration:
"""Returns the TLS server configuration that was used to create the server context."""
...

@abstractmethod
def create_server(
self,
address: _Address,
*,
family: int = socket.AF_INET,
backlog: int | None = None,
reuse_port: bool = False,
dualstack_ipv6: bool = False,
) -> TLSSocket:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want stronger typing for the TLS sockets? Both 'listening sockets' and 'connected sockets' are still the same type.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to implement them using stronger types in siotls, but IMO we need not to enforce it in the spec.

"""Creates a listening socket with an accept() function that returns
a TLSSocket that behaves like a socket.socket, and contains
information about the TLS exchange
(cipher, negotiated_protocol, negotiated_tls_version, etc.).
"""
...

@abstractmethod
def create_buffer(self) -> TLSBuffer:
"""Creates a TLSBuffer that acts as an in-memory channel,
and contains information about the TLS exchange
(cipher, negotiated_protocol, negotiated_tls_version, etc.)."""
...

Socket
~~~~~~
Expand All @@ -375,8 +421,8 @@ specification of the ``TLSSocket`` protocol class. Specifically, implementations
need to implement the following:

* ``recv`` and ``send``
* ``listen`` and ``accept``
* ``close``
* ``accept`` (server-side)
* ``shutdown`` and ``close``
* ``getsockname``
* ``getpeername``

Expand Down Expand Up @@ -418,22 +464,41 @@ The following code describes these functions in more detail:
...

@abstractmethod
def close(self, force: bool = False) -> None:
"""Shuts down the connection and mark the socket closed.
If force is True, this method should send the close_notify alert and shut down
the socket without waiting for the other side.
If force is False, this method should send the close_notify alert and raise
the WantReadError exception until a corresponding close_notify alert has been
received from the other side.
In either case, this method should return WantWriteError if sending the
close_notify alert currently fails."""
def shutdown(self, how: Literal[0, 1, 2]) -> None:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried that the current shutdown / close descriptions may be overspecified for the PEP. Should we focus on describing the observable behavior?

  • shutdown(SHUT_WR) gracefully closes the TLS sending side
  • shutdown(SHUT_RD) and shutdown(SHUT_RDWR) may discard unread peer data and are unsafe unless truncation is not an issue
  • close() should only fully close the transport when safe, otherwise it should raise WantReadError/WantWriteError as appropriate

Also, should we define a specific enum for how? Using literals might be brittle.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 about the over-specification, your shorter text is better indeed

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The best would indeed to type how using an enum, but I'm afraid such an enum doesn't exist. The right place for such an enum would be socket, but in socket there already are the three global integers socket.SHUT_RD/WR/RDWR and deprecating those in favor of an enum doesn't seem right to me.

Maybe we can type if using Literal[socket.SHUT_RD, socket.SHUT_WR, socket.SHUT_RDWR] instead of [0, 1, 2]. Not sure how it would render in a sphinx/pdoc documentation though :(

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can type if using Literal[socket.SHUT_RD, socket.SHUT_WR, socket.SHUT_RDWR] instead of [0, 1, 2]. Not sure how it would render in a sphinx/pdoc documentation though :(

Doesn't work :(

$ uv run ty check src/siotls/socket.py
error[invalid-type-form]: Type arguments for `Literal` must be `None`, a literal value (int, bool, str, or bytes), or an enum member
   --> src/siotls/socket.py:178:44
    |
176 |             logger.debug('%s/%s bytes sent', out_data_sent, len(out_data))
177 |
178 |     def shutdown(self, how: typing.Literal[socket.SHUT_RDWR]) -> None:
    |                                            ^^^^^^^^^^^^^^^^
179 |         """
180 |         Shutdown TLS and the underlying socket.
    |
info: rule `invalid-type-form` is enabled by default

Found 1 diagnostic

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should the enum be in socket? I think it's actually better to define something like a TLSShutdownMode enum here, because the first step is to close the TLS connection and the second is to close the socket. Each backend is then free to determine how to close the socket.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't help but think we are duplicating the information, would you agree to wait for a third opinion?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a shorter docstring for shutdown().

I decided to keep the docstring of close() mostly as it was. That close() raises exceptions in TLSSocket when it does not with regular sockets is gonna be a big surprise to users. I think it warrant a lengthier docstring, and the current one is IMO a good fit.

The commit is in a fixup! because I spent too many hours coming with the original one, that's my way of not loosing my work 🥲. It'll will eventually be squashed don't worry.

"""
Shutdown TLS and the underlying socket.

* ``socket.SHUT_RD`` (``0``) unilateraly close the receiving-side:
discard present and future unread messages.
* ``socket.SHUT_WR`` (``1``) gracefully close the sending-side:
send a closing alert and prevent sending more messages.
* ``socket.SHUT_RDWR`` (``2``): both ``SHUT_WR`` and ``SHUT_RD``.

.. danger::

Both ``shutdown(SHUT_RD)`` and ``shutdown(SHUT_RDWR)`` pose a
risk of data loss: they are unsafe unless the connection is
otherwise known to be over or when truncation is not an issue.

In TLS 1.2, the same risk applies also to ``shutdown(SHUT_WR)``.
"""
...

@abstractmethod
def listen(self, backlog: int) -> None:
"""Enable a server to accept connections. If backlog is specified, it
specifies the number of unaccepted connections that the system will allow
before refusing new connections."""
def close(self) -> None:
"""
Close the underlying socket, but only when it is safe to do so.

When the sending-side of the connection is still open, it gracefully
closes the TLS sending-side before closing the socket, raising
``WantWriteError`` when it fails.

When the receiving-side of the connection is still open, it always
raises ``WantReadError`` as there's a risk of data loss / truncation
attack. The user must first either: (safe) ``recv`` until the peer
closes its sending-side of the connection, or (unsafe) take the risk
and unilateraly ``shutdown`` the receiving-side of the connection.
"""
...

@abstractmethod
Expand Down
Loading