Skip to content
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

PEP 748: A Unified TLS API for Python #3853

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

woodruffw
Copy link
Contributor

@woodruffw woodruffw commented Jun 27, 2024

Current pre-PEP discussion: https://discuss.python.org/t/pre-pep-discussion-revival-of-pep-543/51263

I'm leaving this in draft while @jvdprng and I clean it up a bit -- in particular, we need to copy the latest code examples over from https://github.com/trailofbits/tlslib.py and also make sure that we didn't miss anything while converting this over from our internal doc 🙂


📚 Documentation preview 📚: https://pep-previews--3853.org.readthedocs.build/


These bindings can be used by tools that expect the interface provided by the
Python standard library, with the goal of reducing the dependence of the Python
ecosystem on OpenSSL.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CC @jvdprng: this doesn't include your rendered diagrams, yet. We'll need to add those.

Signed-off-by: William Woodruff <[email protected]>
@hugovk
Copy link
Member

hugovk commented Jun 27, 2024

You can use PEP number 748.

Signed-off-by: William Woodruff <[email protected]>
peps/pep-9999.rst Outdated Show resolved Hide resolved
@woodruffw woodruffw changed the title pep-9999: A Unified TLS API for Python PEP 748: A Unified TLS API for Python Jun 27, 2024
This is getting a little out of control, though.

Signed-off-by: William Woodruff <[email protected]>
peps/pep-0748.rst Outdated Show resolved Hide resolved
Author: Joop van de Pol <[email protected]>,
William Woodruff <[email protected]>
Sponsor: Alyssa Coghlan <[email protected]>
Discussions-To: https://discuss.python.org/t/pre-pep-discussion-revival-of-pep-543/51263
Copy link
Member

Choose a reason for hiding this comment

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

When this ready, please open a new discussion and list it here.

You can either open the discussion just before merge and add it here, or remove this line for merge, then open the discussion, and open a new PR to list it here (and in the Post-History).

peps/pep-0748.rst Show resolved Hide resolved
peps/pep-0748.rst Outdated Show resolved Hide resolved
woodruffw and others added 2 commits June 27, 2024 16:54
Co-authored-by: Hugo van Kemenade <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
peps/pep-0748.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, more specific comments in line.

When the Python code is added, it may be worth moving down to a dedicated Implementation section if having it inline makes the prose too hard to read.

peps/pep-0748.rst Outdated Show resolved Hide resolved
implement the following:

* ``recv`` and ``send``
* ``listen`` and ``accept``
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the expected behaviour of these two APIs if the socket was created from a client context?

Copy link

Choose a reason for hiding this comment

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

I think it should probably raise a generic error, as I'm not aware of situations where a TLS client would be waiting for a TLS server to make the connection. The current MVP does not handle this properly, so I should fix that.

peps/pep-0748.rst Outdated Show resolved Hide resolved
peps/pep-0748.rst Outdated Show resolved Hide resolved
defined for TLS v1.3, and for TLS v1.2, it only contains the cipher suites that
correspond to the TLS v1.3 cipher suites, with ECDHE key exchange (for perfect
forward secrecy) and ECDSA or RSA signatures, which are an additional ten cipher
suites.
Copy link
Contributor

Choose a reason for hiding this comment

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

Likely worth explicitly stating here that libraries and applications that need interoperability with legacy ciphersuites will need to continue to use the existing OpenSSL based TLS APIs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or add a sentence referring to the acceptance of raw integers covered in more detail later

peps/pep-0748.rst Outdated Show resolved Hide resolved
peps/pep-0748.rst Outdated Show resolved Hide resolved
peps/pep-0748.rst Outdated Show resolved Hide resolved
peps/pep-0748.rst Outdated Show resolved Hide resolved
Major future TLS features may require revisions of these protocol classes. These
revisions should be made cautiously: many backends may not be able to move
forward swiftly, and will be invalidated by changes in these protocol classes.
This is acceptable, but wherever possible features that are specific to
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is odd. This new module would be expected to follow the same compatibility rules as the rest of the standard library (the existing SSL module exception for updating to more secure defaults over time would apply)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed; I think we took this verbatim from PEP 543, so this entire "future" section probably needs to be reworked.

woodruffw and others added 4 commits June 28, 2024 10:42
Signed-off-by: William Woodruff <[email protected]>
Copy link

@glyph glyph left a comment

Choose a reason for hiding this comment

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

Network Framework presents some unique challenges here and may require a higher-level standardization, something that hooks into asyncio and asks the operating system for something more like "please establish a connection to a peer with these connection parameters" if we want it to fit into a truly platform-integrated-but-cross-platform system for TLS

Comment on lines +771 to +773
Network Framework is the macOS (10.15+) system TLS library. This library is
substantially more restricted than OpenSSL in many ways, as it has a much more
restricted class of users. One of these substantial restrictions is in
Copy link

Choose a reason for hiding this comment

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

It seems like it would be worth mentioning that Network Framework precludes the implementation of either TLSBuffer or TLSSocket, since there is no way to parse bytes into or out of a bytes buffer, nor any way to cause send and recv to block, as the Network Framework versions of those functions take completion handlers and require a CoreFoundation runloop to already be running.

It's a little misleading to even call Network Framework a "system TLS library", which is why it seems "restricted". Personally I find its architecture very annoying, and badly done in a lot of ways, but in its model it considers TLS to be a the same sort of layer for applications as TCP. In the same way that you can't get raw sockets and then ask the operating system's ethernet driver to assemble connection data from segments in frames with BSD sockets, you cannot ask the operating system's TLS driver to assemble connection data from TCP streams into TLS records for you.

peps/pep-0748.rst Show resolved Hide resolved
peps/pep-0748.rst Show resolved Hide resolved
woodruffw and others added 2 commits July 22, 2024 12:38
Signed-off-by: William Woodruff <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
.github/CODEOWNERS Outdated Show resolved Hide resolved
peps/pep-0748.rst Show resolved Hide resolved
Co-authored-by: Hugo van Kemenade <[email protected]>
@JelleZijlstra
Copy link
Member

What's blocking this from getting merged?

@woodruffw
Copy link
Contributor Author

What's blocking this from getting merged?

I have a few outstanding changes to make, that I haven't had time to make. But I think it could be merged as-is to get a draft version up, and I could start a follow-up PR with the remaining changes.

@woodruffw woodruffw marked this pull request as ready for review September 24, 2024 17:38
@woodruffw woodruffw requested review from tiran and a team as code owners September 24, 2024 17:38
peps/pep-0748.rst Outdated Show resolved Hide resolved
peps/pep-0748.rst Outdated Show resolved Hide resolved
peps/pep-0748.rst Outdated Show resolved Hide resolved
peps/pep-0748.rst Outdated Show resolved Hide resolved
peps/pep-0748.rst Outdated Show resolved Hide resolved
implementations do not support all of these constructors, and they can
communicate this to users as described in the “Runtime” section below.
Certificates from arbitrary identifiers, in particular, are expected to be
useful primarily to users seeking to build integrations on top of HSMs, TPMs,
Copy link
Member

Choose a reason for hiding this comment

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

These abbreviations are never defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

The "expand the first use" style should be sufficient for these two (hardware security modules (HSMs), trusted platform modules (TPMs), if these are actually the first use)

peps/pep-0748.rst Outdated Show resolved Hide resolved
"""
Creates a Certificate object from a path, buffer, or ID.

If none of these is given, an exception is raised.
Copy link
Member

Choose a reason for hiding this comment

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

What does it mean if more than one is given? Is that meaningful?

An alternative design could be a single base class with three concrete child classes corresponding to the three variants.

peps/pep-0748.rst Outdated Show resolved Hide resolved
:mod:`ssl` module exposes too many OpenSSL-specific function calls and features
to easily map to an alternative TLS implementation.

Proposal
Copy link
Member

Choose a reason for hiding this comment

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

The PEP never says where all the proposed new classes will live. Are you adding a new standard library module? If so, what is it called?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. The proof-of-concept PyPI package is tlslib (https://pypi.org/project/tlslib/), while Alex Gaynor and Tom Prince also reserved tls a decade ago (to prevent it from being used for malicious purposes). That means both of those are available as viable names for the stdlib interface.

My recommendation would be to use tls, so tlslib can remain on PyPI as a backport module for any future enhancements to the interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants