Add cpython integration test#1359
Conversation
08a027e to
301fd89
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1359 +/- ##
==========================================
- Coverage 76.83% 76.82% -0.01%
==========================================
Files 425 425
Lines 71527 71527
==========================================
- Hits 54959 54952 -7
- Misses 16568 16575 +7 ☔ View full report in Codecov by Sentry. |
4e6c584 to
38cda10
Compare
4702fb3 to
575e69f
Compare
97e53fd to
fc60492
Compare
014b032 to
1c77609
Compare
6376e29 to
7118481
Compare
6eb2f20 to
268b4bd
Compare
| -# _ssl _ssl.c $(OPENSSL_INCLUDES) $(OPENSSL_LDFLAGS) \ | ||
| -# -l:libssl.a -Wl,--exclude-libs,libssl.a \ | ||
| -# -l:libcrypto.a -Wl,--exclude-libs,libcrypto.a | ||
| -# _hashlib _hashopenssl.c $(OPENSSL_INCLUDES) $(OPENSSL_LDFLAGS) \ | ||
| -# -l:libcrypto.a -Wl,--exclude-libs,libcrypto.a | ||
| +_ssl _ssl.c $(OPENSSL_INCLUDES) $(OPENSSL_LDFLAGS) \ | ||
| + -l:libssl.a -Wl,--exclude-libs,libssl.a \ | ||
| + -l:libcrypto.a -Wl,--exclude-libs,libcrypto.a | ||
| +_hashlib _hashopenssl.c $(OPENSSL_INCLUDES) $(OPENSSL_LDFLAGS) \ | ||
| + -l:libcrypto.a -Wl,--exclude-libs,libcrypto.a |
There was a problem hiding this comment.
Is this diff needed or was this auto-generated
There was a problem hiding this comment.
it's needed. the diff for each branch is slightly different. each branch's changeset was constructed manually on the relevant branch of my cpython fork, tested, and then git diffed into these patch files.
| # - Modify the ssl module's backing C code to set |SSL_MODE_AUTO_RETRY| in | ||
| # all calls to |SSL{_CTX}_set_mode| |
There was a problem hiding this comment.
Is this on by default in OpenSSL?
| # - In |test_bio|handshake|, check whether protocol is TLSv1.3 before testing | ||
| # tls-unique channel binding behavior, as channel bindings are not defined | ||
| # on that protocol |
There was a problem hiding this comment.
Is this a difference in support of channel binding between AWS-LC and OpenSSL, or how OpenSSL and AWS-LC report a failure in attempting to enable channel binding?
There was a problem hiding this comment.
I believe it's a difference in channel binding "support" between AWS-LC and OpenSSL. I re-built my python fork against OSSL 3.0, reverted changes to this test, and dumped the return value of get_channel_binding(). On a TLSv1.3 connection OSSL will actually return a byte array, which is surprising given that channel bindings aren't defined for TLSv1.3. The CPython team has been made aware of this oddity.
There was a problem hiding this comment.
Dear @andrewhop, @WillChilds-Klein, I am happy to see your comments.
It is possible to talk on the CPython specified tickets, the problems with missing "tls-exporter" RFC9266 support, and "tls-server-end-point" support too?
I have done several tickets here without security solutions:
-> 2 years, no security solutions...
Several projects are impacted, I do not know all, example Slixmpp project has done a recent annnouncement:
Thanks a lot in advance.
| + size_t unused_idx; | ||
| + if (sk_SSL_CIPHER_find(client_ciphers, &unused_idx, cipher) < 0) | ||
| +#else | ||
| if (sk_SSL_CIPHER_find(client_ciphers, cipher) < 0) |
There was a problem hiding this comment.
Why not add a find that doesn't take an out index that wraps the existing sk_find for better compatibility? Also can our version of find ever return a negative number?
There was a problem hiding this comment.
Why not add a find that doesn't take an out index that wraps the existing sk_find for better compatibility?
I don't think we can do this, as (to my knowledge) C doesn't support function overloading. Or are you proposing that we create a new funciton/symbol altogether that wraps our existing sk_SSL_CIPHER_find implementation and use that here?
I suppose we could also just change the signature to match OpenSSL's, but that might risk breaking any consumers that already use that function.
Also can our version of find ever return a negative number?
Not according to our documentation, no. Good catch! I'll update this.
There was a problem hiding this comment.
I was thinking we could have sk_SSL_CIPHER_find_internal which is our current implementation and sk_SSL_CIPHER_find which matches OpenSSL. I don't have a sense for how common OpenSSL stack usage outside of libcrypto/libssl is.
There was a problem hiding this comment.
I don't have a sense for how common OpenSSL stack usage outside of libcrypto/libssl is.
It looks like it's non-zero. What percentage of those projects depend on OSSL's signature vs. BSSL's signature? I don't know, but I would guess it would skew towards OSSL given its ubiquity. If we change the signature of sk_SSL_CIPHER_find, though, it would necessarily become a point of build incompatibility with BSSL. If our ultimate goal is OSSL compatibility, that's probably a trade worth making.
c3cf7e8 to
49d9b22
Compare
Issues:
Addresses i/CryptoAlg-2136
Description of changes:
This change adds an integration test for CPython versions 3.10 through tip-of-main. Running all 3.10 python module tests on my AL2 dev machine takes ~5m:
However, in CI testing, I found that some tests (unrelated to our
sslmodule integration) failed on our ubuntu 22.02 docker image:As a result, I've narrowed down our python test suite to only the modules that actually use our libssl (this list was discovered by calling
abort()inSSL_CTX_newand seeing which module tests failed) as well ashashlib, which is the only other module other thansslto include libcrypto. From the CPython 3.10 source to discover which modules use libcrypto:Call-outs:
As we add support for further versions, we may want to parallelize the build and test processes.
Testing:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.