gh-117784: Only reference PHA functions ifndef SSL_VERIFY_POST_HANDSHAKE #117785
Conversation
arhadthedev
added
type-feature
WillChilds-Klein
force-pushed
the
detect-no-pha
branch
from
4d15119 to
4c8cee2
Compare
WillChilds-Klein
changed the title
|
My apologies on the force-push in this draft. I inadvertently rebased latest from upstream main. No significant commit history was lost; I'll refrain from rebasing main again on this PR. |
|
I'm not a SSL expert, but I'm concerned about relying on an Does AWS-LC still define |
WillChilds-Klein
force-pushed
the
detect-no-pha
branch
from
1ded2dc to
6743dc0
Compare
WillChilds-Klein
changed the title
PR #1526 introduced the `OPENSSL_NO_TLS_PHA` directive mostly for the purposes of AWS-LC's compatibility with CPython, but in [cpython PR #117785](python/cpython#117785) @encukou points out that detecting the absence of OpenSSL's own `SSL_VERIFY_POST_HANDSHAKE` directive is sufficient. This change removes AWS-LC's `OPENSSL_NO_TLS_PHA` directive in favor of detecting absence of `SSL_VERIFY_POST_HANDSHAKE`.
Lib/test/test_ssl.py
Outdated
| @@ -4364,14 +4364,14 @@ def test_session_handling(self): | |||
| def test_psk(self): | |||
| psk = bytes.fromhex('deadbeef') | |||
|
|
|||
| client_context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT) | |||
| client_context, server_context, _ = testing_context() | |||
There was a problem hiding this comment.
Is this change needed?
Not being an OpenSSL expert, I'd rather avoid having to review it :)
There was a problem hiding this comment.
It's required for this test to pass when built against AWS-LC, but I'll remove it to conform to your guidance here:
IMO we could get to the point where you can get CPython to compile with AWS-LC without special-casing it (i.e. no Py_OPENSSL_IS_AWSLC), but relevant test cases still fail.
You’ll still need a patch, but a more focused one.
Modules/_ssl.c
Outdated
| @@ -187,6 +187,11 @@ extern const SSL_METHOD *TLSv1_2_method(void); | |||
| #endif | |||
|
|
|||
|
|
|||
| #if !defined(SSL_VERIFY_POST_HANDSHAKE) || !defined(TLS1_3_VERSION) || defined(OPENSSL_NO_TLS1_3) | |||
| #define PY_SSL_NO_POST_HS_AUTH | |||
There was a problem hiding this comment.
I'd prefer a positive name: it is always used with ! later and double negatives are harder to read.
Perhaps PySSL_HAVE_POST_HS_AUTH
| @@ -3576,7 +3582,7 @@ set_maximum_version(PySSLContext *self, PyObject *arg, void *c) | |||
| return set_min_max_proto_version(self, arg, 1); | |||
| } | |||
|
|
|||
| #ifdef TLS1_3_VERSION | |||
| #if defined(TLS1_3_VERSION) && !defined(OPENSSL_NO_TLS1_3) | |||
There was a problem hiding this comment.
Are tickets used only for post-handshake authentication? From the docs it looks like a separate TLSv1.3 feature.
There was a problem hiding this comment.
No, tickets are used for session resumption and are unrelated to post-handshake authentication. I believe get_num_tickets pertains to a post-handshake message added in TLSv1.3. SSL_CTX_get_num_tickets's docs indicate as much.
This change just updates that guard to honor OpenSSL's OPENSSL_NO_TLS1_3 as other parts of _ssl.c already do.
|
Thank you @encukou! Would it be possible to backport this to 3.13? I see that 3.12 is currently in "bugfix", so I assume this patch isn't applicable to <= 3.12. |
As of [CPython PR #117785][1], CPython can now build against AWS-LC without any source code modifications. The only patches we still require are to configure the build and work around ([expected][2]) test failures. [1]: python/cpython#117785 [2]: https://discuss.python.org/t/support-building-ssl-and-hashlib-modules-against-aws-lc/44505/4
[CPython PR #117785][1], CPython can now build against AWS-LC without any source code modifications. The only patches we still require are to configure the build and work around ([expected][2]) test failures. [1]: python/cpython#117785 [2]: https://discuss.python.org/t/support-building-ssl-and-hashlib-modules-against-aws-lc/44505/4
…HANDSHAKE (pythonGH-117785) With this change, builds with OpenSSL forks that don't have this functionalty (like AWS-LC or BoringSSL) will require less patching.
…HANDSHAKE (pythonGH-117785) With this change, builds with OpenSSL forks that don't have this functionalty (like AWS-LC or BoringSSL) will require less patching.
…HANDSHAKE (pythonGH-117785) With this change, builds with OpenSSL forks that don't have this functionalty (like AWS-LC or BoringSSL) will require less patching.
Notes
AWS-LC recently defined a new configuration macro
OPENSSL_NO_TLS_PHAto signal lack of support for TLSv1.3 post-handshake authentication. Other cryptography libraries may find this useful. We then adjustModules/_ssl.cto detect this macro, guarding usages of PHA-related functions. We also update two PSK tests to generate client/server contexts withtest_ssl.py's conventionaltesting_context()utility.This pull request implements Issue #117784.