Fix [Dev] ML-KEM (with Archival) support for CRMFPopClient (and/or cli)#5364
Fix [Dev] ML-KEM (with Archival) support for CRMFPopClient (and/or cli)#5364jmagne wants to merge 1 commit into
Conversation
Add code in KRA to extract the ML-KEM session key if encapsulated. Added a new user profile caMLKEMUserCert.cfg Fixed UserKeyDefault.java to recognize ML-KEM, it already checks for ML-DSA.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for ML-KEM (Module-Lattice-Based Key-Encapsulation Mechanism) by adding a new certificate profile, implementing encapsulation and decapsulation logic in CryptoUtil, and updating the KRA to support ML-KEM transport keys. Feedback from the review highlights a critical need to use the returned token-resident key after calling importPublicKey to ensure correct encapsulation. Additionally, the reviewer suggested using more specific exceptions like NoSuchAlgorithmException, removing redundant log messages, and correcting minor whitespace issues.
| token.importPublicKey(wrappingKey, false); | ||
|
|
||
| // Do ML-KEM encapsulate to derive shared secret + ciphertext | ||
| logger.debug("CryptoUtil: Using ML-KEM encapsulation for key archival"); | ||
| KEMEncapsulation keyResult = encapsulateMLKEM(wrappingKey, params.getPayloadEncryptionAlgorithm()); |
There was a problem hiding this comment.
The return value of token.importPublicKey(wrappingKey, false) is ignored. As noted in the comment on line 2360, ML-KEM requires the public key to be imported as a PKCS#11 object on the token for encapsulation. The importPublicKey method returns a PK11PubKey (as a PublicKey) which should be used in the subsequent encapsulateMLKEM call to ensure the operation uses the token-resident key. Additionally, there is a double space before params on line 2367.
| token.importPublicKey(wrappingKey, false); | |
| // Do ML-KEM encapsulate to derive shared secret + ciphertext | |
| logger.debug("CryptoUtil: Using ML-KEM encapsulation for key archival"); | |
| KEMEncapsulation keyResult = encapsulateMLKEM(wrappingKey, params.getPayloadEncryptionAlgorithm()); | |
| PublicKey tokenKey = token.importPublicKey(wrappingKey, false); | |
| // Do ML-KEM encapsulate to derive shared secret + ciphertext | |
| logger.debug("CryptoUtil: Using ML-KEM encapsulation for key archival"); | |
| KEMEncapsulation keyResult = encapsulateMLKEM(tokenKey, params.getPayloadEncryptionAlgorithm()); |
| sk = CryptoUtil.decapsulateMLKEM(wrappingKey, | ||
| encSymmKey, | ||
| params.getPayloadEncryptionAlgorithm()); | ||
| logger.debug("TransportKeyUnit.unwrap: Using ML-KEM decapsulation"); |
There was a problem hiding this comment.
This debug log message is identical to the one on line 434. It should be updated to indicate that the decapsulation step has completed successfully or removed to avoid redundancy.
| logger.debug("TransportKeyUnit.unwrap: Using ML-KEM decapsulation"); | |
| logger.debug("TransportKeyUnit.unwrap: ML-KEM decapsulation successful"); |
| byte[] kemCipherText = null; | ||
|
|
||
| if (wrappingAlg == null) { | ||
| throw new Exception("Unable to determine transport key algorithm"); |
There was a problem hiding this comment.
Using a generic Exception is discouraged. Since this is a cryptographic operation where an algorithm cannot be determined, NoSuchAlgorithmException would be more appropriate and idiomatic.
| throw new Exception("Unable to determine transport key algorithm"); | |
| throw new NoSuchAlgorithmException("Unable to determine transport key algorithm"); |
| if (isAlgorithmMLKEM(wrappingAlg)) { | ||
| session_data = kemCipherText; | ||
| } else { | ||
| session_data = wrapUsingPublicKey( |
There was a problem hiding this comment.
There are unnecessary extra spaces before the method call.
| session_data = wrapUsingPublicKey( | |
| session_data = wrapUsingPublicKey( |
|
1 participant
Add code in KRA to extract the ML-KEM session key if encapsulated.
Added a new user profile caMLKEMUserCert.cfg
Fixed UserKeyDefault.java to recognize ML-KEM, it already checks for ML-DSA.
Assisted-by: Claude Sonnet 4.5