-
Notifications
You must be signed in to change notification settings - Fork 26
[PSQ] Allow using Classic McEliece for PQ KEM #1108
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
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.
Just some early thoughts
libcrux-psq/Cargo.toml
Outdated
"mceliece460896f", | ||
"zeroize", | ||
], optional = true } | ||
]} |
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.
Shouldn't this still be optional?
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.
Good question! I would like it to be, but I built the PQ KEM selection with an enum now, and that would then have to be non_exhaustive
, no?
I recall now we had a similar situation in the original PSQ and then resolved to do this with a commonly implemented trait, instead of an enum. Ideally that would be one of the KEM traits from libcrux-traits
... I'll see if I can implement that for the CMC wrapper here.
libcrux-psq/Cargo.toml
Outdated
[features] | ||
classic-mceliece = ["dep:classic-mceliece-rust", "rand_old"] | ||
legacy = [] | ||
# classic-mceliece = ["dep:classic-mceliece-rust", "rand_old"] |
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.
Should this go back in?
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.
Yes, I temporarily disabled that too work on the new version as if CMC was unconditionally included, but in any case the old protocol needs the feature.
g_xy: &DHSharedSecret::derive(responder_ephemeral_ecdh_sk, initiator_ephemeral_ecdh_pk)?, | ||
}; | ||
|
||
AEADKey::new(&responder_ikm, tx2).map_err(|e| e.into()) |
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.
?
call into
for you. So something like this would work as well.
AEADKey::new(&responder_ikm, tx2).map_err(|e| e.into()) | |
Ok(AEADKey::new(&responder_ikm, tx2)?) |
|
||
impl<'a> PQPublicKey<'a> { | ||
/// Encapsulate a PQ-shared secret towards the given PQ-KEM public key | ||
pub(crate) fn encapsulate(&self, rng: &mut impl CryptoRng) -> (PQCiphertext, PQSharedSecret) { |
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.
You should add lifetimes here to avoid warnings.
#[repr(u8)] | ||
/// A PQ-KEM key encapsulation | ||
pub enum PQCiphertext { | ||
MlKem768(MlKem768Ciphertext), |
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.
This is pretty bad for the CMCE ciphertext (156 vs 1088). But maybe that's fine? Or would it make sense to use references here as well?
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.
Some thoughts
// External setup | ||
let responder_ecdh_keys = DHKeyPair::new(&mut rng); | ||
let responder_pq_keys = PQKeyPair::new(&mut rng); | ||
let responder_pq_keys = libcrux_ml_kem::mlkem768::rand::generate_key_pair(&mut rng); |
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.
Should the into
once happen up here already?
libcrux-psq/src/handshake/builder.rs
Outdated
} | ||
|
||
/// Set the long-term PQ key pair. | ||
pub fn longterm_pq_keys(mut self, longterm_pq_keys: impl Into<PQKeyPair<'a>>) -> Self { |
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.
We should make the API consistent. The DH above gets just a reference. We should pick a style and stick to it for both types of keys.
let responder_ecdh_keys = DHKeyPair::new(&mut rng); | ||
|
||
let responder_pq_keys = PQKeyPair::new(&mut rng); | ||
let responder_pq_keys = libcrux_ml_kem::mlkem768::rand::generate_key_pair(&mut rng); |
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.
Same comment as for the builder below. We should pick a style for both. Here in the call site we just see the difference as well.
I think either could be fine. Either a generic key_gen
with an algorithm identifier, or explicitly generating the keys. But I think I like one function for all a little better.
self | ||
} | ||
|
||
/// Set the inner AAD. |
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.
Somewhat unrelated to this PR but I think the aad setters need more documentation to tell the user what the outer and inner aads are.
I've changed the proposal here to use a trait mechanism for implementing ciphersuites. This makes it easier to pull out Classic McEliece into a non-default feature. The downside is that in the way I've implemented it here, we need to pre-serialize the PQ encapsulation, to make the One more thing that is missing here is a builder for the ciphersuites, so now they are statically constructed in the tests. |
97daa4c
to
8658ea5
Compare
This PR does three things:
v1
module, with the exception of some of the Classic McEliece code.If you want I can separate 1. and 2. into their own PR, so the changes relating to 3. become more clear.
In terms of possible "configurations" for 3. I went the simplest option, i.e. an initiator can have either an ML-KEM or CMC longterm public key of the responder, and the responder can have either an ML-KEM or CMC key pair. If there is a mismatch, i.e. the initiator sends a registration message for which the responder does not have a matching decapsulation key, then the responder will ignore that message and return to its initial state.
TODO