Skip to content

gh-135401: Test AWS-LC as a cryptography library in CI #135402

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

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

WillChilds-Klein
Copy link
Contributor

@WillChilds-Klein WillChilds-Klein commented Jun 11, 2025

Notes

Currently in draft status to test the new workflow. Building AWS-LC, linking the interpreter to it dynamically, and running the ssl tests all appear to work fine. I'm still working on remaining issues in test_hmac.

TODO

  • The proposal to remove NID_blake2b512 from ./configure's hashlib feature probe may be controversial, so we need to make sure this is adequately discussed. I believe that this is safe to do because CPython now guards BLAKE2 usages based on whether or not the underlying cryptography library supports it.
  • Engage with @picnixz to discuss 8f4a0eb, which modifies a test he recently wrote.
  • @WillChilds-Klein to fix test_hmac failures

@AA-Turner AA-Turner changed the title gh-135401 Add AWS-LC-backed ssl module CI job gh-135401: Test AWS-LC SSL in CI Jun 11, 2025
@@ -1,12 +1,12 @@
#!./python
"""Run Python tests against multiple installations of OpenSSL and LibreSSL
"""Run Python tests against multiple installations of crypto libraries
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""Run Python tests against multiple installations of crypto libraries
"""Run Python tests against multiple installations of cryptographic libraries.
Currently tested are:
* OpenSSL
* LibreSSL
* AWS-LC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that "crypto" is unfortunate shorthand. Not to get too deep in the semantic weeds, but I wonder if "cryptography" might be more appropriate than "cryptographic". The former noun-based nomenclature is more idiomatic when referring to libraries ("compression library", "serialization library", etc.) while the latter's adjective seems a bit off.

I don't have a strong preference, what do you think @AA-Turner?

by the way, thank you for the quick review :)

Copy link
Member

Choose a reason for hiding this comment

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

libcrypto is the OpenSSL cryptographic library, and the other libs (BoringSSL, LibreSSL) are forks. So I'd prefer keeping crypto here or use libcrypto

@@ -70,7 +74,7 @@
parser = argparse.ArgumentParser(
prog='multissl',
description=(
"Run CPython tests with multiple OpenSSL and LibreSSL "
"Run CPython tests with multiple crypto libraries"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Run CPython tests with multiple crypto libraries"
"Run CPython tests with multiple cryptographic libraries"

with:
path: ./multissl/aws-lc/${{ matrix.awslc_ver }}
key: ${{ matrix.os }}-multissl-aws-lc-${{ matrix.awslc_ver }}
# TODO [childw] can we use env.* instead of env vars here?
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest for the initial version, keep as similar to the OpenSSL job/workflow, and then perhaps update both at once afterwards?

Copy link
Contributor Author

@WillChilds-Klein WillChilds-Klein Jun 11, 2025

Choose a reason for hiding this comment

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

Fair enough. I'll remove the TODOs. Perhaps we can leave this comment unresolved as a reminder for me to clean up both (if tenable) if/after this PR has been merged.

@WillChilds-Klein WillChilds-Klein changed the title gh-135401: Test AWS-LC SSL in CI gh-135401: Test AWS-LC as a cryptography library in CI Jun 11, 2025
@@ -513,6 +554,14 @@ def main():
build.install()
builds.append(build)

for version in args.awslc:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rewrite the three loops as:

for build_class, versions in [
    (BuildOpenSSL, args.openssl),
    (BuildLibreSSL, args.libressl),
    (BuildAWSLC, args.awslc),
]:
    for version in versions:
        build = build_class(version, args)
        build.install()
        builds.append(build)

@@ -7545,7 +7545,6 @@ WITH_SAVE_ENV([
OBJ_nid2sn(NID_md5);
OBJ_nid2sn(NID_sha1);
OBJ_nid2sn(NID_sha3_512);
OBJ_nid2sn(NID_blake2b512);
Copy link
Member

Choose a reason for hiding this comment

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

Can you update this check to actually check for sha2 as well? I doubt someone won't have SHA2 but has SHA3 but it will be consistent with our expectations. Also, for posterity, we don't use OpenSSL BLAKE-2 because it has limited support, so it doesn't make sense to enable it here (but we can use it in HMAC)

@picnixz
Copy link
Member

picnixz commented Jun 13, 2025

Can you cherry-pick 8f4a0eb and make a separate PR please? TiA.

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.

3 participants