Rust wrapper: add mlkem module#9833
Conversation
|
retest this please (ERROR: wolfSSL » PRB-140-3-tests #8576 was deleted) |
| /// | ||
| /// # Parameters | ||
| /// | ||
| /// * `key_type`: Key type. One of [`MlKem::TYPE_512`], [`MlKem::TYPE_768`], |
There was a problem hiding this comment.
here, just like for the dilithium.rs, I think there should be a check for key_type.
either here or in generate_with_random_ex, making sure that is either 512, 768 or 1024.
There was a problem hiding this comment.
It looks like the C side already does check this parameter in wc_MlKemKey_Init(). That check is probably better anyway since it takes into account different build options the C library can be built with.
| if ct.len() != expected_ct_size { | ||
| return Err(sys::wolfCrypt_ErrorCodes_BUFFER_E); | ||
| } | ||
| if ss.len() != Self::SHARED_SECRET_SIZE { |
There was a problem hiding this comment.
here maybe, for consistency, could they both get validated against a runtime query (like it's happening for ct.len()) or both against a rust constant.
so adding a shared_secret_size() for the ss check too or just a brief comment explaining why the constant is used directly.
There was a problem hiding this comment.
I added some comments; let me know if they seem sufficient to you. The cipher text length is dependent upon the parameter set used while the shared secret length is always the same regardless of the parameter set in use.
| self.ws_key, | ||
| ss.as_mut_ptr(), | ||
| ct.as_ptr(), | ||
| ct.len() as u32, |
There was a problem hiding this comment.
maybe here (and other places too) I would consider a more idiomatic way of casting something like:
u32::try_from(ct.len()).map_err(|_| ...)? (I think this right way of typing this).
in practice this is just to prevent in case ct.len() is > than u32::MAAX, the cipher text for ml-kem I don't think it can be that big, but just for better practice a better casting would be better I think.
There was a problem hiding this comment.
Hmm, yeah I think a whole lot of things would break if we were trying to use buffer sizes > 4GB :)
I don't really consider that a concern worth doing anything about personally. But I can change if we think it is worth the tradeoff of longer runtime and more ROM usage (there are a few places where the same thing is done).
There was a problem hiding this comment.
I wonder if the compiler would just elide the entire check on a 32-bit target.
|
retest this please (build removed) |
3 participants
Description
Rust wrapper: add mlkem module
Testing
Unit tests
Checklist