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

gh-128035: Add ssl.HAS_PHA to detect libssl PHA support #128036

Merged
merged 9 commits into from
Dec 24, 2024

Conversation

WillChilds-Klein
Copy link
Contributor

@WillChilds-Klein WillChilds-Klein commented Dec 17, 2024

Notes

Please see #128035's description.

Testing

  • CPython CI
  • confirmed that PHA-related tests no longer fail when CPython is built against AWS-LC

📚 Documentation preview 📚: https://cpython-previews--128036.org.readthedocs.build/

Copy link
Member

@tomasr8 tomasr8 left a comment

Choose a reason for hiding this comment

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

This will also need a news entry :)

Doc/library/ssl.rst Outdated Show resolved Hide resolved
Doc/library/ssl.rst Outdated Show resolved Hide resolved
Lib/test/test_httplib.py Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
TLSv1.3 post-handshake client authentication (PHA), often referred to as "mutual TLS" or "mTLS", allows TLS servers to authenticate client identities using digital certificates. This commit exposes a boolean property ``ssl.HAS_PHA`` to indicate whether the crypto library CPython is built against supports PHA, allowing python's test suite and consuming modules to branch accordingly.
Copy link
Contributor

Choose a reason for hiding this comment

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

A shorter NEWS should be written. The NEWS is a message that users will see (it's in the changelog). Some suggestion:

Indicate through :data:`ssl.HAS_PHA` whether the :mod:`ssl` module supports TLSv1.3
post-handshake client authentication (PHA). Patch by YOURNAME.

In addition, you should add a What's New entry in Doc/whatsnew/3.14.rst indicating the additional constant. Usually, the same message as for the NEWS entry can be reused (check the other entries for the formatting).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the guidance. I've updated the news and whatsnew files accordingly.

@WillChilds-Klein WillChilds-Klein marked this pull request as ready for review December 17, 2024 21:22
Comment on lines 73 to 75
* Indicate through :data:`ssl.HAS_PHA` whether the :mod:`ssl` module supports TLSv1.3
post-handshake client authentication (PHA). (Contributed by Will Childs-Klein in
:gh:`128036`.)
Copy link
Member

Choose a reason for hiding this comment

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

This should go under Improved modules. You can make a new section for ssl:

ssl
---

* Indicate through :data:`ssl.HAS_PHA` whether the :mod:`ssl` module supports TLSv1.3
  post-handshake client authentication (PHA). (Contributed by Will Childs-Klein in
  :gh:`128036`.)

Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Considering the precedent on HAS_PSK and conditional blake2 detection for OpenSSL-like implementations that do not support it, this an ok change.

Some questions:

  • is there an equivalent of OPENSSL_NO_PHA constant or not? (AFAIK, no, but are there some implementations that offer TLSv1.3 without PHA and would define OPENSSL_NO_PHA?)
  • is there a better constant than SSL_VERIFY_POST_HANDSHAKE to check if PHA is supported or not? namely, is there an implementation which supports PHA (whether by default or not) but uses another constant?

cc @gpshead for a second look as well

Comment on lines +586 to +587
* Indicate through :data:`ssl.HAS_PHA` whether the :mod:`ssl` module supports
TLSv1.3 post-handshake client authentication (PHA).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Indicate through :data:`ssl.HAS_PHA` whether the :mod:`ssl` module supports
TLSv1.3 post-handshake client authentication (PHA).
* Indicate through :data:`ssl.HAS_PHA` whether the :mod:`ssl` module
supports TLSv1.3 post-handshake client authentication (PHA).

(just for reducing the line length and make it look more uniform, but this is entirely optional and a matter of taste; my previous suggestion may even have been incorrect in that regard).

Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

This makes sense to me. If there are better ways to determine this in the future, we can adapt to those. As with a lot of things libssl related... I feel like it can be configured via the environment in such ways that regardless of compile time checks capabilities can be changed at runtime via its configs. This constant only covers if the library could support it, but isn't able to express if it happens to be enabled or not within the current process. That is fine and is the expected simple thing to do.

@gpshead gpshead self-assigned this Dec 24, 2024
@gpshead gpshead added type-feature A feature request or enhancement topic-SSL labels Dec 24, 2024
@gpshead gpshead enabled auto-merge (squash) December 24, 2024 18:29
@gpshead gpshead merged commit 418114c into python:main Dec 24, 2024
49 checks passed
@WillChilds-Klein WillChilds-Klein deleted the ssl-add-has-pha-property branch December 27, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-SSL type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants