-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
PEP 748: tlslib - context & socket #4960
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
539e879
791d34e
785c0c4
3d40e2f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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( | ||
| self, | ||
| address: tuple[str | None, int], | ||
| timeout=GLOBAL_DEFAULT, | ||
| source_address=None, | ||
| *, | ||
| all_errors=False, | ||
|
Comment on lines
+357
to
+360
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs typing information
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, and we need to add the typing information in the original |
||
| ) -> TLSSocket: | ||
| """Creates a TLSSocket that behaves like a socket.socket, and | ||
| contains information about the TLS exchange | ||
| (cipher, negotiated_protocol, negotiated_tls_version, etc.). | ||
|
|
@@ -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, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to set
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I only replicated the |
||
| backlog=None, | ||
| reuse_port=False, | ||
| dualstack_ipv6=False, | ||
|
Comment on lines
+396
to
+399
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs typing information
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, and we need to add the typing information in the original |
||
| ) -> TLSSocket: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
| ~~~~~~ | ||
|
|
@@ -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`` | ||
|
|
||
|
|
@@ -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: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm worried that the current
Also, should we define a specific
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 about the over-specification, your shorter text is better indeed
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The best would indeed to type Maybe we can type if using |
||
| """ | ||
| 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 | ||
|
|
||
There was a problem hiding this comment.
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_hostnameparameter here to support 'connect by IP, verify as DNS name'?