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
121 changes: 104 additions & 17 deletions peps/pep-0748.rst
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,14 @@ The ``ClientContext`` protocol class has the following class definition:
...

@abstractmethod
def connect(self, address: tuple[str | None, int]) -> TLSSocket:
def create_connection(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should we add a server_hostname parameter here to support 'connect by IP, verify as DNS name'?

self,
address: tuple[str | None, int],
timeout=GLOBAL_DEFAULT,
source_address=None,
*,
all_errors=False,
Comment on lines +357 to +360
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Needs typing information

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.

Agreed, and we need to add the typing information in the original socket.create_connection as well.

) -> 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 +372,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: tuple[str | None, int],
*,
family=AF_INET,
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 to set AF_INET as the default here? We could have None that lets the backend choose based on the address?

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 only replicated the socket.create_server API which uses family=AF_INET by default. To me it is either we stick precisely to the same signature as socket.create_server, or we use another function name.

backlog=None,
reuse_port=False,
dualstack_ipv6=False,
Comment on lines +396 to +399
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Needs typing information

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.

Agreed, and we need to add the typing information in the original socket.create_server as well.

) -> 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 +420,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 +463,64 @@ 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 :(

"""
Shutdown TLS and the underlying socket.

Proper TLS applications ought to signal their peers when they're
done sending data by sending a closing alert.

* ``socket.SHUT_WR`` (``1``) sends the closing alert to the peer and
then prevents sending any further message. Proper TLS application
**MUST** call this method with this parameter to gracefully close
the TLS connection. *Safe* in TLS 1.3. Actually acts like
``SHUT_RDWD`` in TLS 1.2 and is as *unsafe* as ``SHUT_RD``.

* ``socket.SHUT_RD`` (``0``) simulates receiving the closing alert
from the peer and then ignores all further received messages.
*Unsafe* (risk of data loss) unless the connection is otherwise
known to be over thanks to the application-layer protocol (e.g.
HTTP Content-Length).

* ``socket.SHUT_RDWR`` (``2``) does both ``SHUT_RD`` and ``SHUT_WR``.
As *unsafe* as ``SHUT_RD``.

In TLS 1.2, the closing alert is synchronous, receiving it triggers
an immediate closing alert response, and both connections are
immediately shut. It means that ``SHUT_WR`` acts like ``SHUT_RDWR``
and is unsafe unless all the data have been received.

In TLS 1.3, the closing alert is asynchronous, sending the closing
alert only closes the sender's sending-end and receiver's
receiving-end. The peer can keep on sending data until he
independently decides to close its own sending-end of the
connection. There is not risk of data truncation with ``SHUT_WR``.

.. danger::

Both ``socket.SHUT_RD`` (``0``) and ``socket.SHUT_RDWR`` (``2``)
pose a risk of data loss, only use them when facing a bad actor
or when the connection is otherwise known to be over.

In TLS 1.2 the same risk applies also to ``socket.SHUT_WR`` (``1``).
"""
...

@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-end of the connection is still open, it sends a
closing alert before closing the socket, raising ``WantWriteError``
when it fails.

When the receiving-end 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-end of the connection, or (unsafe) take the risk
and unilateraly ``shutdown`` the reading-end of the connection.
"""
...

@abstractmethod
Expand Down
Loading