-
Notifications
You must be signed in to change notification settings - Fork 759
Delete all code that references Kyber #5705
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
base: main
Are you sure you want to change the base?
Conversation
7b38a03 to
7820197
Compare
dougch
left a comment
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.
Nice! it might also be worth calling out dropping OQS...
| } else { | ||
| RESULT_GUARD(s2n_fips_validate_kem(hybrid_group->kem, valid)); | ||
|
|
||
| if ((hybrid_group->curve != &s2n_ecc_curve_none) && !hybrid_group->send_kem_first) { |
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.
I keep staring at this and I have no idea why this change in necessary. Please explain 😢.
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.
- With the removal of Kyber,
kem_preferences_allis now FIPS compliant. (Because every KEM preference now contains MLKEM, which is FIPS approved). - Since
kem_preferences_allis now FIPS compliant, I thought that the TLS Policytest_all_fipsshould be updated to usekem_preferences_all. test_all_fipshas the FIPS rule check enforced on it.- The existing FIPS rule checks didn't properly handle pure MLKEM1024, so I updated this file
s2n_fips_rules.cto correctly handle pure MLKEM1024
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.
I'm interested in this logic about the send_kem_first field. Previously, if the kem was first, we validated it. Otherwise we validated the ecc curve. Now we are always validating the kem, and only validating the ecc curve if it exists and comes first. Is that correct? I get that in pure MLKEM, the ecc curve may not exist, but the ordering of groups logic is now really confusing.
Goal
Removes any reference that Kyber ever existed from s2n-tls's codebase.
Why
How
grep -ni kyber -r .in the s2n-tls directory and deleted everything I could find.Callouts
20240730which was previously pointed to bydefault_pq. I don't think that will cause any issues, but wanted to call that out for others to confirm.test_all_fipswas updated to add all MLKEM KeyShares sincekem_preferences_allis now FIPS compliant.crypto/s2n_fips_rules.cso that pure ML-KEM-1024 would be detected as FIPS.Testing
Unit Testing on M4 Mac.
Related
Related to:
release summary: Delete all code that references Kyber
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.