-
Notifications
You must be signed in to change notification settings - Fork 342
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
Replace Kyber with ML-KEM (FIPS 203) for quantum-resistant tunnel secrets #6915
Replace Kyber with ML-KEM (FIPS 203) for quantum-resistant tunnel secrets #6915
Conversation
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.
Reviewable status: 0 of 12 files reviewed, 2 unresolved discussions
talpid-tunnel-config-client/src/ml_kem.rs
line 58 at r1 (raw file):
log::debug!( "ML-KEM decapsulation took {} ms", start.elapsed().as_millis()
I intend to remove this before merging. It's just here to allow me to more easily verify in logs that I run an app with ML-KEM when testing this.
talpid-wireguard/src/config.rs
line 127 at r1 (raw file):
let mut wg_conf = WgConfigBuffer::new(); wg_conf .add::<&[u8]>("private_key", self.tunnel.private_key.to_bytes().as_ref())
This is completely unrelated. I have not yet dug into why these changes are needed. But Rustc is angry at me if I don't make them.
e274862
to
71c9c09
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.
Reviewed 4 of 12 files at r1, 8 of 8 files at r2, 3 of 3 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)
talpid-tunnel-config-client/src/ml_kem.rs
line 58 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
I intend to remove this before merging. It's just here to allow me to more easily verify in logs that I run an app with ML-KEM when testing this.
I guess the line you originally referred to has been cleaned up already?
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)
talpid-tunnel-config-client/src/ml_kem.rs
line 58 at r1 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
I guess the line you originally referred to has been cleaned up already?
Indeed! It was removed during some rebase and squash after I had verified this code worked on a few platforms.
27e2ea8
to
6893ad6
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.
Reviewed 8 of 8 files at r9, 3 of 3 files at r10, 1 of 1 files at r11, 1 of 1 files at r12, 1 of 1 files at r13, 1 of 1 files at r14, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
9a1ae1f
to
a420974
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.
Reviewable status: 6 of 15 files reviewed, 1 unresolved discussion (waiting on @faern and @MarkusPettersson98)
ios/CHANGELOG.md
line 32 at r20 (raw file):
### Changed - Replace the draft key encapsulation mechanism Kyber (round 3) with the standardized ML-KEM (FIPS 203) dito in the handshake for Quantum-resistant tunnels.
Should be under ## Unreleased
.
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.
Reviewed 1 of 3 files at r18.
Reviewable status: 7 of 15 files reviewed, 1 unresolved discussion (waiting on @faern and @MarkusPettersson98)
a420974
to
d30d6cf
Compare
Upgrading one of the key encapsulation mechanism algorithms we use for quantum-resistant tunnels from Kyber (draft) to ML-KEM (standardized FIPS 203)
The dependency with this CVE is no longer in our dependency tree
Required to make it Rust >=1.81
d30d6cf
to
6e64e75
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.
Reviewed 1 of 3 files at r21.
Reviewable status: 6 of 15 files reviewed, all discussions resolved (waiting on @albin-mullvad and @MarkusPettersson98)
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.
Reviewable status: 6 of 15 files reviewed, all discussions resolved (waiting on @albin-mullvad and @MarkusPettersson98)
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.
Reviewable status: 6 of 15 files reviewed, all discussions resolved (waiting on @MarkusPettersson98 and @pinkisemils)
🚨 End to end tests failed. Please check the failed workflow run. |
When adding support for Quantum-resistant tunnels, there were no standardized algorithms for post-quantum secure key encapsulation machanisms (KEM). Mullvad then implemented a scheme where we could mix multiple KEMs in such a way that all of them had to be broken in order to break the tunnel. We then put our bets on two promising algorithms: Classic McEliece and CRYSTALS-Kyber.
NIST recently finished their PQ standardization effort they have been running for quite a while. One of the new standards, ML-KEM is based off of CRYSTALS-Kyber. It is basically the same thing, but renamed and slightly revised.
It is important to point out that we have no reason to believe Kyber is insecure in any way in our current implementation of quantum-resistant tunnels. But it is nice to upgrade to the standardized KEM. This also means we can upgrade from an unmaintained Kyber dependency, to a hopefully better maintained ML-KEM dependency.
This change requires Rust 1.81 due to
hybrid-array
being very bleeding edge. We currently use Rust 1.80 everywhere. So this does not currently pass CI, nor can be built in our build containers. If we agree we want to use these dependencies, I'm going to make sure we bump everything to Rust 1.81 before this PR is ready for merge.This is a draft PR since our servers still don't support ML-KEM. I'm still opening the PR, since that will be rolled out to servers shortly.
This change is