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

PQ KEMs behind a feature flag #221

Merged
merged 24 commits into from
Jan 14, 2025
Merged

PQ KEMs behind a feature flag #221

merged 24 commits into from
Jan 14, 2025

Conversation

mulmarta
Copy link
Contributor

Implements 2 PQ replacements of DH-KEM

Call outs

DH-related functions were only copied

Safety

I did run mls-rs-crypto-aws-lc tests with address sanitizer and stared at the code very long. Unfortunately aws-lc-rs does not currently support

  • SHAKE
  • storing a decapsulation key as bytes
  • deterministic key generation which is experimental in AWS-LC

Testing

I would love to run some tests from the XWing RFC but unfortunately they seem to be incompatible with ML-KEM from AWS-LC and they don't contain intermediate values I could use to only test the combiner. We could generate test vectors based on the XWing implementation in the future.

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

@mulmarta mulmarta requested a review from a team as a code owner November 28, 2024 11:28
@codecov-commenter
Copy link

codecov-commenter commented Nov 28, 2024

Codecov Report

Attention: Patch coverage is 85.73388% with 104 lines in your changes missing coverage. Please review.

Project coverage is 90.16%. Comparing base (9a20dc9) to head (17aa5b9).

Files with missing lines Patch % Lines
mls-rs-crypto-awslc/src/lib.rs 74.35% 40 Missing ⚠️
mls-rs-crypto-hpke/src/kem_combiner.rs 92.97% 26 Missing ⚠️
mls-rs-crypto-awslc/src/kem/ecdh.rs 84.95% 17 Missing ⚠️
mls-rs-crypto-hpke/src/dhkem.rs 69.69% 10 Missing ⚠️
mls-rs-crypto-traits/src/kem.rs 0.00% 4 Missing ⚠️
mls-rs-crypto-openssl/src/ecdh.rs 40.00% 3 Missing ⚠️
mls-rs-crypto-rustcrypto/src/ecdh.rs 40.00% 3 Missing ⚠️
mls-rs-crypto-awslc/src/kdf.rs 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #221      +/-   ##
==========================================
- Coverage   90.19%   90.16%   -0.04%     
==========================================
  Files         177      180       +3     
  Lines       31808    32317     +509     
==========================================
+ Hits        28690    29139     +449     
- Misses       3118     3178      +60     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tomleavy
Copy link
Contributor

tomleavy commented Jan 7, 2025

Looks like the entire kem combiner is uncovered code, lets fix that

@mulmarta
Copy link
Contributor Author

mulmarta commented Jan 7, 2025

Looks like the entire kem combiner is uncovered code, lets fix that

Looking. It's weird, I'm sure I tested it...

tomleavy
tomleavy previously approved these changes Jan 9, 2025
@tomleavy tomleavy requested a review from stefunctional January 9, 2025 22:09
Copy link
Contributor

@stefunctional stefunctional left a comment

Choose a reason for hiding this comment

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

Just a couple of questions.

mls-rs-crypto-awslc/src/kem/ml_kem.rs Show resolved Hide resolved
mls-rs-crypto-rustcrypto/src/ecdh.rs Outdated Show resolved Hide resolved
mls-rs-crypto-webcrypto/src/ec/ecdh.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@stefunctional stefunctional left a comment

Choose a reason for hiding this comment

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

Thank you!

@mulmarta mulmarta merged commit 9aee7e6 into main Jan 14, 2025
32 checks passed
@mulmarta mulmarta deleted the kyber-rebased branch January 14, 2025 13:05
mulmarta added a commit that referenced this pull request Jan 15, 2025
* wip

* Replace round3 submission by ml-kem

* Fixup

* Rename kyber to ml-kem

* Rename kyber to ml-kem

* Fixup

* Fixup

* Fix?

* Bump versions

* wip

* wip

* Update aws-lc version. Support Kyber with FIPS

* Add tests for combiner

* Fixup

* Fixup

* Fixup

* Fixup

* Fix warnings, not build FIPS and non-FIPS at the same time

* Fixup

* Fixup

* Fixup :(

* Fix key size

* Apply suggestions from code review

Co-authored-by: Stephane Raux <[email protected]>

---------

Co-authored-by: Marta Mularczyk <[email protected]>
Co-authored-by: Stephane Raux <[email protected]>
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.

4 participants