Remove PQ TLS 1.2 Support #5022

alexw91 · 2025-01-10T00:54:04Z

Release Summary:

Removes draft support for post-quantum TLS 1.2 (draft-campagna-tls-bike-sike-hybrid) from s2n-tls. Support for post-quantum TLS 1.3 (draft-ietf-tls-hybrid-design) is kept.

Resolved issues:

None.

Description of changes:

Removes draft support for post-quantum TLS 1.2 (draft-campagna-tls-bike-sike-hybrid) from s2n-tls. Support for post-quantum TLS 1.3 (draft-ietf-tls-hybrid-design) is kept.

Call-outs:

Removes PQ TLS 1.2 support and the TLS_ECDHE_KYBER_RSA_WITH_AES_256_GCM_SHA384 cipher entirely from s2n-tls.
Older PQ TLS policies that should no longer be used have been deprecated.
s2n_ecdhe_kyber_rsa_with_aes_256_gcm_sha384 was removed from all cipher preference lists. If this removal made the cipher preference list identical to an already existing cipher preference list, then the entire cipher preference list was deleted and security policies migrated to use the other already existing cipher preference list. Otherwise, if no equivalent cipher preference list existed then s2n_ecdhe_kyber_rsa_with_aes_256_gcm_sha384 was removed in-place (eg for cipher_suites_pq_tls_1_0_2021_05_24).
All PQ TLS 1.2 tests have either been deleted or modified to support TLS 1.3.
The function s2n_connection_get_kem_name() is kept, but now hardcoded to always return NONE in all circumstances so as to not break customers.

Testing:

New unit tests were added to ensure that all PQ enabled Security Policies support TLS 1.3
New unit tests were added to ensure that all deprecated security policies return S2N_ERR_DEPRECATED_SECURITY_POLICY when requested.
All PQ TLS 1.2 tests have either been deleted or modified to support TLS 1.3.

Remember:

Any change to the library source code should at least include unit tests.
Any change to the core stuffer or blob methods should include CBMC proofs.
Any change to the CI or tests should:
1. prove that the test succeeds for good input
2. prove that the test fails for bad input (eg, a test for memory leaks fails when a memory leak is committed)

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

alexw91 · 2025-01-10T00:56:02Z

tests/unit/s2n_security_policies_test.c

+            /* Ensure that PQ enabled Policies support TLS 1.3 since TLS 1.3 is now required for PQ support. */
+            EXPECT_TRUE(has_tls_13_cipher);


Moved "TLS 1.3 + PQ checks" further down in the function so that this block has access to the has_tls_13_cipher variable. This move allows adding this check that requires TLS 1.3 support for all PQ policies.

lrstewart

The diff here is still very large, so I'm still worried. If there's any possible way to keep breaking this change down into smaller chunks, that would probably make this review move faster and make the change safer.

lrstewart · 2025-01-17T20:06:40Z

tls/s2n_security_policies.c

 const char *deprecated_security_policies[] = {
+    "KMS-PQ-TLS-1-0-2019-06",
+    "KMS-PQ-TLS-1-0-2020-02",
+    "KMS-PQ-TLS-1-0-2020-07",


Your changes to the policy list are very hard / impossible to visually verify. Could you maybe run a manual test to confirm that s2n_array_len(security_policy_selection) + s2n_array_len(deprecated_security_policies) is the same before and after your change? Like maybe add some logic to a test or other binary that can report that number, and then run that test with and without your change.

I'm not sure we could add a permanent, committed test without requiring that it be updated every time someone adds a new policy :/

It might be even better if you can confirm no changes to the names. Maybe the test/binary could write the names to a file instead of just reporting a number, and then you can diff the files with and without your change.

lrstewart · 2025-01-17T20:09:55Z

tls/s2n_security_policies.c

-    .cipher_preferences = &cipher_preferences_pq_tls_1_0_2021_05_26,
+    .cipher_preferences = &cipher_preferences_20210825_gcm,


Why does this not change the security policy? Why is this safe? (Same for the other policies)

lrstewart · 2025-01-17T20:10:58Z

tls/s2n_kex.c

-    return query == kex->hybrid[0] || query == kex->hybrid[1];
+    return false;


You don't need the previous kex/query check if you're just going to return false anyway

lrstewart · 2025-01-17T20:12:21Z

tls/s2n_kem_preferences.c

-    .kem_count = s2n_array_len(pq_kems_r3_2021_05),
-    .kems = pq_kems_r3_2021_05,


Why are these changes safe / why do they not change policies in active use?

lrstewart · 2025-01-17T20:15:16Z

tls/s2n_cipher_preferences.c

    S2N_TLS13_CIPHER_SUITES_20190801,
-    &s2n_ecdhe_kyber_rsa_with_aes_256_gcm_sha384,


Why is this change safe?

lrstewart · 2025-01-17T20:17:51Z

tests/unit/s2n_tls13_pq_handshake_test.c

-                .client_policy = &security_policy_pq_tls_1_1_2021_05_21,
+                .client_policy = &security_policy_pq_tls_1_0_2021_05_24,


For your tests: how did you verify that you have the right replacement policy, and aren't changing the behavior of the test?

This is a more general comment then just this line: this PR is really too large for a reviewer to personally verify all your test policy substitutions. How do you know they're all correct?

lrstewart · 2025-01-17T20:22:58Z

tests/unit/s2n_cipher_suite_match_test.c

Please don't delete all the newlines-- it makes it harder to read.

alexw91 commented Jan 10, 2025

View reviewed changes

alexw91 requested review from lrstewart and jmayclin January 10, 2025 18:53

alexw91 force-pushed the remove-pq-tls12 branch 2 times, most recently from 87b1a7d to a2b0656 Compare January 15, 2025 23:13

Remove PQ TLS 1.2 Support

fcfb279

alexw91 force-pushed the remove-pq-tls12 branch from a2b0656 to fcfb279 Compare January 17, 2025 01:17

lrstewart mentioned this pull request Jan 17, 2025

Openssl-3.0-fips does not support signing raw digest bytes #5047

Open

lrstewart reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove PQ TLS 1.2 Support #5022

Remove PQ TLS 1.2 Support #5022

alexw91 commented Jan 10, 2025

alexw91 Jan 10, 2025 •

edited

Loading

lrstewart left a comment

lrstewart Jan 17, 2025

lrstewart Jan 17, 2025

lrstewart Jan 17, 2025

lrstewart Jan 17, 2025

lrstewart Jan 17, 2025

lrstewart Jan 17, 2025 •

edited

Loading

lrstewart Jan 17, 2025

		/* Ensure that PQ enabled Policies support TLS 1.3 since TLS 1.3 is now required for PQ support. */
		EXPECT_TRUE(has_tls_13_cipher);

		.cipher_preferences = &cipher_preferences_pq_tls_1_0_2021_05_26,
		.cipher_preferences = &cipher_preferences_20210825_gcm,

		return query == kex->hybrid[0] \|\| query == kex->hybrid[1];
		return false;

		.kem_count = s2n_array_len(pq_kems_r3_2021_05),
		.kems = pq_kems_r3_2021_05,

		S2N_TLS13_CIPHER_SUITES_20190801,
		&s2n_ecdhe_kyber_rsa_with_aes_256_gcm_sha384,

		.client_policy = &security_policy_pq_tls_1_1_2021_05_21,
		.client_policy = &security_policy_pq_tls_1_0_2021_05_24,

Remove PQ TLS 1.2 Support #5022

Are you sure you want to change the base?

Remove PQ TLS 1.2 Support #5022

Conversation

alexw91 commented Jan 10, 2025

Release Summary:

Resolved issues:

Description of changes:

Call-outs:

Testing:

alexw91 Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

lrstewart left a comment

Choose a reason for hiding this comment

lrstewart Jan 17, 2025

Choose a reason for hiding this comment

lrstewart Jan 17, 2025

Choose a reason for hiding this comment

lrstewart Jan 17, 2025

Choose a reason for hiding this comment

lrstewart Jan 17, 2025

Choose a reason for hiding this comment

lrstewart Jan 17, 2025

Choose a reason for hiding this comment

lrstewart Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

lrstewart Jan 17, 2025

Choose a reason for hiding this comment

alexw91 Jan 10, 2025 •

edited

Loading

lrstewart Jan 17, 2025 •

edited

Loading