Add cpython integration test #1359

WillChilds-Klein · 2023-12-15T00:52:51Z

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:

$ time ./tests/ci/integration/run_python_integration.sh
...
== Tests result: FAILURE then SUCCESS ==

404 tests OK.

21 tests skipped:
    test_bz2 test_ctypes test_devpoll test_idle test_ioctl test_kqueue
    test_msilib test_ossaudiodev test_readline test_sqlite
    test_startfile test_tcl test_tix test_tk test_ttk_guionly
    test_ttk_textonly test_turtle test_winconsoleio test_winreg
    test_winsound test_zipfile64

1 re-run test:
    test___all__

However, in CI testing, I found that some tests (unrelated to our ssl module integration) failed on our ubuntu 22.02 docker image:

== Tests result: FAILURE then FAILURE ==

395 tests OK.

9 tests failed:
    test_email test_generators test_multiprocessing_fork
    test_multiprocessing_forkserver test_multiprocessing_spawn
    test_pdb test_regrtest test_signal test_threading

21 tests skipped:
    test_bz2 test_dbm_gnu test_dbm_ndbm test_devpoll test_gdb
    test_idle test_ioctl test_kqueue test_msilib test_ossaudiodev
    test_startfile test_tcl test_tix test_tk test_ttk_guionly
    test_ttk_textonly test_turtle test_winconsoleio test_winreg
    test_winsound test_zipfile64

10 re-run tests:
    test___all__ test_email test_generators test_multiprocessing_fork
    test_multiprocessing_forkserver test_multiprocessing_spawn
    test_pdb test_regrtest test_signal test_threading

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() in SSL_CTX_new and seeing which module tests failed) as well as hashlib, which is the only other module other than ssl to include libcrypto. From the CPython 3.10 source to discover which modules use libcrypto:

$ ag -l 'include.+crypto' Modules/
Modules/_hashopenssl.c
Modules/_ssl.c

Call-outs:

As we add support for further versions, we may want to parallelize the build and test processes.

Testing:

see description section above
CI integration tests

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.

codecov-commenter · 2023-12-15T03:43:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (67d3e50) 76.83% compared to head (49d9b22) 76.82%.

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.
📢 Have feedback on the report? Share it here.

tests/ci/integration/python_patch/3.11/aws-lc-cpython.patch

samuel40791765 · 2024-01-26T01:08:27Z

tests/ci/integration/python_patch/main/aws-lc-cpython.patch

+-# _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


Is this diff needed or was this auto-generated

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.

tests/ci/integration/run_python_integration.sh

andrewhop · 2024-01-26T20:50:54Z

tests/ci/integration/run_python_integration.sh

+#   - Modify the ssl module's backing C code to set |SSL_MODE_AUTO_RETRY| in
+#     all calls to |SSL{_CTX}_set_mode|


Is this on by default in OpenSSL?

As of 1.1.1, yes.

andrewhop · 2024-01-26T20:55:09Z

tests/ci/integration/run_python_integration.sh

+#   - 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


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?

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.

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:

https://github.com/python/cpython/issues?q=is%3Aissue+author%3A%40me+

-> 2 years, no security solutions...

Several projects are impacted, I do not know all, example Slixmpp project has done a recent annnouncement:

https://blog.mathieui.net/en/slixmpp-1.8.5.html

https://codeberg.org/poezio/slixmpp/issues/3498

Thanks a lot in advance.

tests/ci/integration/python_patch/main/aws-lc-cpython.patch

andrewhop · 2024-01-26T21:17:04Z

tests/ci/integration/python_patch/main/aws-lc-cpython.patch

+        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)


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?

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.

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.

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.

WillChilds-Klein force-pushed the integration-test-python branch 2 times, most recently from 08a027e to 301fd89 Compare December 15, 2023 03:22

WillChilds-Klein force-pushed the integration-test-python branch 4 times, most recently from 4e6c584 to 38cda10 Compare December 18, 2023 17:42

WillChilds-Klein closed this Dec 19, 2023

WillChilds-Klein deleted the integration-test-python branch December 19, 2023 15:06

WillChilds-Klein restored the integration-test-python branch December 19, 2023 20:16

WillChilds-Klein reopened this Dec 19, 2023

WillChilds-Klein mentioned this pull request Dec 21, 2023

Implement SSL_MODE_AUTO_RETRY #1333

Merged

WillChilds-Klein force-pushed the integration-test-python branch 4 times, most recently from 4702fb3 to 575e69f Compare January 8, 2024 23:40

WillChilds-Klein mentioned this pull request Jan 8, 2024

Add socat integration test #1387

Merged

WillChilds-Klein force-pushed the integration-test-python branch 4 times, most recently from 97e53fd to fc60492 Compare January 9, 2024 02:26

WillChilds-Klein force-pushed the integration-test-python branch 4 times, most recently from 014b032 to 1c77609 Compare January 22, 2024 16:55

WillChilds-Klein marked this pull request as ready for review January 22, 2024 17:05

WillChilds-Klein requested a review from a team as a code owner January 22, 2024 17:05

WillChilds-Klein enabled auto-merge (squash) January 22, 2024 17:07

WillChilds-Klein force-pushed the integration-test-python branch 2 times, most recently from 6376e29 to 7118481 Compare January 22, 2024 21:18

WillChilds-Klein requested review from andrewhop and samuel40791765 January 22, 2024 21:19

WillChilds-Klein force-pushed the integration-test-python branch 2 times, most recently from 6eb2f20 to 268b4bd Compare January 25, 2024 19:48

samuel40791765 reviewed Jan 26, 2024

View reviewed changes

WillChilds-Klein requested a review from samuel40791765 January 26, 2024 17:11

samuel40791765 previously approved these changes Jan 26, 2024

View reviewed changes

andrewhop reviewed Jan 26, 2024

View reviewed changes

WillChilds-Klein dismissed samuel40791765’s stale review via c3cf7e8 January 29, 2024 20:08

WillChilds-Klein requested a review from andrewhop January 29, 2024 20:14

samuel40791765 previously approved these changes Jan 29, 2024

View reviewed changes

torben-hansen added the reviewers-assigned label Jan 30, 2024

WillChilds-Klein and others added 13 commits January 30, 2024 19:43

Add cpython integration test

4b8cec2

Add patch and test for 3.11

686a416

Add patch and test for 3.12

5e18bf7

Add patch and test for main, rm deprecated test_nntplib

f789c31

Debugging IPv6 issue

a0cc9ec

tmp: fix 3.10 patch, temporarily omit 3.11+ tests

3511821

Fix python 3.11 patch

7cd4e38

Fix python 3.12 patch

ce69776

Fix python main patch

ff64c72

Remove codebiuld CI job

f026908

Summarize patch file contents in test comment

aed219e

Remove accidental comment from patch files

192900e

Adjust sk_SSL_CIPHER_find return value check

49d9b22

WillChilds-Klein dismissed samuel40791765’s stale review via 49d9b22 January 30, 2024 19:45

WillChilds-Klein force-pushed the integration-test-python branch from c3cf7e8 to 49d9b22 Compare January 30, 2024 19:45

samuel40791765 approved these changes Jan 31, 2024

View reviewed changes

andrewhop approved these changes Jan 31, 2024

View reviewed changes

WillChilds-Klein merged commit b49958a into aws:main Jan 31, 2024
38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cpython integration test #1359

Add cpython integration test #1359

WillChilds-Klein commented Dec 15, 2023 •

edited

Loading

codecov-commenter commented Dec 15, 2023 •

edited

Loading

samuel40791765 Jan 26, 2024

WillChilds-Klein Jan 26, 2024 •

edited

Loading

andrewhop Jan 26, 2024

WillChilds-Klein Jan 29, 2024

andrewhop Jan 26, 2024

WillChilds-Klein Jan 29, 2024

Neustradamus Feb 9, 2024

andrewhop Jan 26, 2024

WillChilds-Klein Jan 29, 2024

andrewhop Jan 29, 2024

WillChilds-Klein Jan 29, 2024 •

edited

Loading

		# - Modify the ssl module's backing C code to set \|SSL_MODE_AUTO_RETRY\| in
		# all calls to \|SSL{_CTX}_set_mode\|

Add cpython integration test #1359

Add cpython integration test #1359

Conversation

WillChilds-Klein commented Dec 15, 2023 • edited Loading

Issues:

Description of changes:

Call-outs:

Testing:

codecov-commenter commented Dec 15, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

WillChilds-Klein Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillChilds-Klein Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

WillChilds-Klein commented Dec 15, 2023 •

edited

Loading

codecov-commenter commented Dec 15, 2023 •

edited

Loading

WillChilds-Klein Jan 26, 2024 •

edited

Loading

WillChilds-Klein Jan 29, 2024 •

edited

Loading