-
Notifications
You must be signed in to change notification settings - Fork 724
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
Add PQ integration tests between s2n and AWS-LC's libssl #4267
Conversation
592be0f
to
5617bc1
Compare
5617bc1
to
faa069d
Compare
faa069d
to
e26e8e9
Compare
e00d2b6
to
6408468
Compare
df4e39b
to
9cba320
Compare
9cba320
to
65a3fb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Add HybridKeyShare support for SecP256r1Kyber768Draft00 and X25519Kyber768Draft00 aws-lc#1201 is merged into AWS-LC
- AWS-LC cuts a new release that has PR 1201 in it
- The install_awslc.sh script is updated to use this new release version.
- s2n's CI Docker images that contain cached test dependency artifacts may need to be regenerated to contain the latest version of aws-lc in the ./test-deps directory.
These all appear complete, the current codebuild container was built with v1.17.4
Kicked off a retry of the failed general job.
6cbb021
to
6075144
Compare
port = next(available_ports) | ||
|
||
awslc_env_vars = dict() | ||
awslc_env_vars["PATH"] = os.path.abspath("../../test-deps/awslc/bin") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a bit odd to me that we have to manually specify these. We currently have AWS-LC integ tests, and as far as I can tell we don't have to specify these for the other tests?
Ideally we'd launch aws-lc here the same way that we do in other tests. If that's not possible, could you add a comment explaning why the extra env variables are needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as I can tell we don't have to specify these for the other tests?
We do still set up environment variables when testing s2n with libOQS's fork of Openssl.
Removing all awslc_env_vars
resulted in CI failure. From trial and error, it seems that setting awslc_env_vars["PATH"] = os.path.abspath("../../test-deps/awslc/bin")
is the only required value for this to pass in CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we wouldn't be using any CI specific paths in our integration tests ("test-deps") since it makes these tests really hard to run from a local dev machine. however I've found that our integration tests are already really difficult to run from a local dev machine, so it does feel a little rich to hold you to that standard 😉.
c4fe9d1
to
617f645
Compare
617f645
to
4df9654
Compare
port = next(available_ports) | ||
|
||
awslc_env_vars = dict() | ||
awslc_env_vars["PATH"] = os.path.abspath("../../test-deps/awslc/bin") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we wouldn't be using any CI specific paths in our integration tests ("test-deps") since it makes these tests really hard to run from a local dev machine. however I've found that our integration tests are already really difficult to run from a local dev machine, so it does feel a little rich to hold you to that standard 😉.
Resolved issues:
N/A
Description of changes:
Adds hybrid post-quantum TLS integration tests between s2n and AWS-LC that is currently in a PR here: aws/aws-lc#1201
Call-outs:
This new integration test will fail in s2n's current GitHub CI until the following are completed:
install_awslc.sh
script is updated to use this new release version../test-deps
directory.Testing:
Locally removed all other tests in
test_pq_handshake.py
file and confirmed that these tests pass locally on my Ubuntu EC2 instance:By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.