Rust wrapper: add mlkem module#9833
Conversation
|
retest this please (ERROR: wolfSSL » PRB-140-3-tests #8576 was deleted) |
|
retest this please (build removed) |
There was a problem hiding this comment.
Pull request overview
Adds an ML-KEM (post-quantum KEM) Rust wrapper module to the wolfCrypt Rust bindings, along with CI enablement and unit tests.
Changes:
- Introduces
mlkemRust module wrapping wolfCrypt ML-KEM key lifecycle and operations (generate/encap/decap/encode/decode). - Adds ML-KEM unit tests covering constants, size queries, deterministic APIs, and encap/decap flows.
- Extends bindings/CI to expose and build ML-KEM (
headers.h,build.rscfg scan, GitHub workflow).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| wrapper/rust/wolfssl-wolfcrypt/src/mlkem.rs | New ML-KEM wrapper API (key management + KEM operations) |
| wrapper/rust/wolfssl-wolfcrypt/tests/test_mlkem.rs | New unit tests for ML-KEM wrapper behavior |
| wrapper/rust/wolfssl-wolfcrypt/src/lib.rs | Exposes the new mlkem module from the crate |
| wrapper/rust/wolfssl-wolfcrypt/headers.h | Adds wolfcrypt/mlkem.h to bindings header set |
| wrapper/rust/wolfssl-wolfcrypt/build.rs | Enables cfg(mlkem) detection in build-time symbol scan |
| .github/workflows/rust-wrapper.yml | Adds a CI matrix entry building wolfSSL with --enable-mlkem |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| impl Drop for MlKem { | ||
| /// Safely free the underlying wolfSSL ML-KEM key context. | ||
| /// | ||
| /// This calls `wc_MlKemKey_Delete()`. The Rust Drop trait guarantees this | ||
| /// is called when the `MlKem` struct goes out of scope. | ||
| fn drop(&mut self) { | ||
| unsafe { | ||
| sys::wc_MlKemKey_Delete(self.ws_key, core::ptr::null_mut()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
new_ex() accepts a heap hint and passes it into wc_MlKemKey_New(...), but Drop always calls wc_MlKemKey_Delete(..., null_mut()). If the underlying C API expects the same heap hint for deallocation, this can lead to mismatched allocation/free behavior. Store the heap pointer in MlKem and pass it to wc_MlKemKey_Delete, or remove the heap parameter from the Rust API if it’s not actually needed/safe to expose.
There was a problem hiding this comment.
This is completely incorrect. The second parameter to wc_MlKemKey_Delete is not the heap hint.
| pub fn encode_public_key(&self, out: &mut [u8]) -> Result<usize, i32> { | ||
| let rc = unsafe { | ||
| sys::wc_MlKemKey_EncodePublicKey(self.ws_key, out.as_mut_ptr(), out.len() as u32) | ||
| }; | ||
| if rc != 0 { | ||
| return Err(rc); | ||
| } | ||
| Ok(out.len()) | ||
| } | ||
|
|
||
| /// Encode (export) the private key to a byte buffer. | ||
| /// | ||
| /// The `out` buffer must be exactly `private_key_size()` bytes. | ||
| /// | ||
| /// # Parameters | ||
| /// | ||
| /// * `out`: Output buffer to receive the encoded private key. | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// Returns either Ok(size) containing the number of bytes written or Err(e) | ||
| /// containing the wolfSSL library error code value. | ||
| /// | ||
| /// # Example | ||
| /// | ||
| /// ```rust |
There was a problem hiding this comment.
Both encode_public_key and encode_private_key return Ok(out.len()) even though the function contract says the buffer must be exactly the key size. If a caller passes a larger buffer and the C API succeeds, returning out.len() overstates the number of bytes actually written. Consider validating out.len() against public_key_size() / private_key_size() up front and returning the expected size (or changing the API to return () to avoid implying an exact written length).
There was a problem hiding this comment.
Ignoring this. The C function already checks that the buffer size is exactly the required size.
| check_cfg(&binding, "DILITHIUM_RND_SZ", "dilithium_rnd_sz"); | ||
|
|
||
| /* mlkem / ML-KEM */ | ||
| check_cfg(&binding, "wc_MlKemKey_Init", "mlkem"); |
There was a problem hiding this comment.
The mlkem feature detection is keyed off wc_MlKemKey_Init, but the Rust wrapper uses wc_MlKemKey_New, wc_MlKemKey_Delete, wc_MlKemKey_MakeKey, etc. If wc_MlKemKey_Init isn’t present in a given wolfSSL build (or differs by version), cfg(mlkem) may not be set even though the functions actually used by the wrapper are available. Use a symbol that the wrapper directly relies on (e.g., wc_MlKemKey_New or wc_MlKemKey_MakeKey) for the check_cfg probe.
| check_cfg(&binding, "wc_MlKemKey_Init", "mlkem"); | |
| check_cfg(&binding, "wc_MlKemKey_New", "mlkem"); |
There was a problem hiding this comment.
Ignoring this. If wc_MlKemKey_Init() is defined then _New(), etc... are defined as well.
| // Verify the shared secret length is as expected. | ||
| if ss.len() != Self::SHARED_SECRET_SIZE { |
There was a problem hiding this comment.
The docstring for encapsulate() says ss must be exactly shared_secret_size() bytes, but the implementation validates against Self::SHARED_SECRET_SIZE. If shared_secret_size() is intended to be authoritative (and keeps the implementation consistent with docs), consider validating with self.shared_secret_size()? instead of the constant.
| // Verify the shared secret length is as expected. | |
| if ss.len() != Self::SHARED_SECRET_SIZE { | |
| // Verify the shared secret length is as expected based on the parameter | |
| // set (key type) in use. | |
| let expected_ss_size = self.shared_secret_size()?; | |
| if ss.len() != expected_ss_size { |
There was a problem hiding this comment.
I updated the comment to say the expected size is SHARED_SECRET_SIZE instead of shared_secret_size().
| pub fn decapsulate(&mut self, ss: &mut [u8], ct: &[u8]) -> Result<(), i32> { | ||
| // Verify the shared secret length is as expected. | ||
| if ss.len() != Self::SHARED_SECRET_SIZE { | ||
| return Err(sys::wolfCrypt_ErrorCodes_BUFFER_E); | ||
| } | ||
| let rc = unsafe { | ||
| sys::wc_MlKemKey_Decapsulate( | ||
| self.ws_key, | ||
| ss.as_mut_ptr(), | ||
| ct.as_ptr(), | ||
| ct.len() as u32, | ||
| ) | ||
| }; |
There was a problem hiding this comment.
encapsulate() performs an explicit cipher text size check before calling into C, but decapsulate() doesn’t. For consistency (and clearer/earlier Rust-side errors), consider validating ct.len() against self.cipher_text_size()? and returning BUFFER_E when it doesn’t match.
There was a problem hiding this comment.
Ignoring this. The length is checked in encapsulate() because it is not passed to and checked in the C function. In contrast, for decapsulate(), it is passed in and check by the C function.
Description
Rust wrapper: add mlkem module
Testing
Unit tests
Checklist