Skip to content
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

fix(integration): Update PQ integration test expectations #5082

Merged
merged 3 commits into from
Feb 5, 2025

Conversation

goatgoose
Copy link
Contributor

@goatgoose goatgoose commented Feb 4, 2025

Description of changes:

The KMS PQ network integration test recently started failing in CI because KMS started negotiating a different KEM group:

assertion `left == right` failed
  left: Some("SecP256r1Kyber768Draft00")
 right: Some("x25519_kyber-512-r3")

We're currently expecting x25519_kyber-512-r3 to be negotiated, but KMS is now negotiating SecP256r1Kyber768Draft00.

I don't believe this to be indicative of any regression on our side, since this failure wasn't caused by any specific PR. Also, SecP256r1Kyber768Draft00 is actually a more preferred KEM group in the test security policy (PQ-TLS-1-2-2023-10-09):

const struct s2n_kem_group *pq_kem_groups_r3_2023_06[] = {
&s2n_secp256r1_kyber_768_r3,
&s2n_x25519_kyber_768_r3,

This PR updates the expectations of this test.

Call-outs:

The purpose of this test is simply to verify that we can successfully perform a PQ handshake. The specific KEM group seems less important. So I updated this test to just check that any kyber KEM group was negotiated. We could instead update this test to check for the new KEM group specifically, if this seems preferable.

Testing:

The PQ integration test should now succeed.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Feb 4, 2025
@goatgoose goatgoose marked this pull request as ready for review February 4, 2025 22:08
@goatgoose goatgoose requested review from lrstewart and dougch February 4, 2025 22:08
@@ -53,7 +53,7 @@ mod kms_pq {
tls.as_ref().cipher_suite()?,
"TLS_AES_256_GCM_SHA384"
);
assert_eq!(tls.as_ref().kem_group_name(), Some("x25519_kyber-512-r3"));
assert!(tls.as_ref().kem_group_name().unwrap().to_lowercase().contains("kyber"));
Copy link
Contributor

Choose a reason for hiding this comment

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

From the comment above

    // Note: In the future KMS will deprecate kyber_r3 in favor of ML-KEM.
    // At that point this test should be updated with a security policy that
    // supports ML-KEM.

So I'd vote for switching the security policy and then asserting on one of the standardized IANA groups like SecP256r1MLKEM768.

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 tried this but it only sometimes worked. I think some of the KMS hosts maybe still only support the draft KEM groups. For now I updated the test with a security policy that includes both ML-KEM and kyber groups, and assert that any KEM group was negotiated. I opened an issue to update this test to assert ML-KEM when we're able to: #5086

@goatgoose goatgoose requested a review from jmayclin February 5, 2025 18:27
@goatgoose goatgoose enabled auto-merge February 5, 2025 19:14
@goatgoose goatgoose added this pull request to the merge queue Feb 5, 2025
Merged via the queue into aws:main with commit 2952ede Feb 5, 2025
44 checks passed
@goatgoose goatgoose deleted the fix-kms-pq-test branch February 5, 2025 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants