Add ML-KEM support to KRA key archival#5358
Conversation
|
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:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 (Post-Quantum Cryptography) within the KRA enrollment and storage workflows. Key changes include updating EnrollmentService to extract ML-KEM key strength and modifying StorageKeyUnit to implement KEM encapsulation for shared secret generation. Feedback identifies a unit mismatch where bits were passed instead of bytes for the KEM key size and a potential NullPointerException when checking the key algorithm string.
| CryptoUtil.KEMEncapsulation kemResult = CryptoUtil.encapsulateMLKEM( | ||
| storagePubKey, | ||
| params.getSkLength()); |
There was a problem hiding this comment.
The params.getSkLength() method returns the key length in bits (e.g., 128 or 256), but CryptoUtil.encapsulateMLKEM expects the keySize parameter in bytes. Passing bits instead of bytes will cause the KEM encapsulation to request an incorrectly sized shared secret (e.g., 256 bytes instead of 32 bytes), which will lead to a failure or security issues.
| CryptoUtil.KEMEncapsulation kemResult = CryptoUtil.encapsulateMLKEM( | |
| storagePubKey, | |
| params.getSkLength()); | |
| CryptoUtil.KEMEncapsulation kemResult = CryptoUtil.encapsulateMLKEM( | |
| storagePubKey, | |
| params.getSkLength() / 8); |
| rec.set(KeyRecord.ATTR_META_INFO, metaInfo); | ||
| // key size does not apply to EC | ||
| rec.setKeySize(-1); | ||
| } else if (keyAlg.toUpperCase().startsWith("ML-KEM")) { |
There was a problem hiding this comment.
The check keyAlg.toUpperCase().startsWith("ML-KEM") will throw a NullPointerException if keyAlg is null. While keyAlg is unlikely to be null for a valid key, the preceding checks for "RSA" and "EC" use equals(), which safely handles null values. For consistency and safety, a null check should be added.
| } else if (keyAlg.toUpperCase().startsWith("ML-KEM")) { | |
| } else if (keyAlg != null && keyAlg.toUpperCase().startsWith("ML-KEM")) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
base/kra/src/main/java/com/netscape/kra/EnrollmentService.java (1)
412-417: ⚡ Quick winConsider enforcing the
allowEncDecrypt_archivalconstraint for ML-KEM storage.The comment documents that
allowEncDecrypt_archivalis not supported for ML-KEM storage, but there's no runtime enforcement. IfallowEncDecrypt_archival=trueis configured with an ML-KEM storage certificate,mStorageUnit.encryptInternalPrivate()(line 430) would be called, which lacks ML-KEM support and may fail unexpectedly.Consider adding a check to either:
- Detect ML-KEM storage and skip the encrypt path, or
- Throw a clear error if the unsupported combination is detected
Example enforcement approach
// Before the encrypt/wrap decision, check storage cert algorithm String storageAlg = mStorageUnit.getPublicKey().getAlgorithm(); if (allowEncDecrypt_archival && storageAlg != null && storageAlg.toUpperCase().startsWith("ML-KEM")) { logger.warn("EnrollmentService: allowEncDecrypt_archival not supported with ML-KEM storage, using wrap instead"); allowEncDecrypt_archival = false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/kra/src/main/java/com/netscape/kra/EnrollmentService.java` around lines 412 - 417, The code documents that allowEncDecrypt_archival isn't supported for ML-KEM storage but doesn't enforce it at runtime; before the branch that calls mStorageUnit.encryptInternalPrivate() (the encrypt vs wrap decision in EnrollmentService), query mStorageUnit.getPublicKey().getAlgorithm() and if allowEncDecrypt_archival is true and the algorithm indicates ML-KEM (e.g., startsWith("ML-KEM") case-insensitive) either set allowEncDecrypt_archival=false and emit a clear logger.warn mentioning EnrollmentService and ML-KEM, or throw a focused IllegalArgumentException with a message that allowEncDecrypt_archival is not supported for ML-KEM storage so the unsupported path is never taken.base/kra/src/main/java/com/netscape/kra/StorageKeyUnit.java (1)
1185-1223: API verification confirmed; recovery gap is expected per PR scope.The
CryptoUtil.encapsulateMLKEM(PublicKey, int)API exists and correctly returnsKEMEncapsulationwithsharedSecret(SymmetricKey) andciphertext(byte[]) fields. The usage in StorageKeyUnit.wrap() is correct.The recovery gap is acknowledged:
unwrap_session_key()and related unwrap methods lack ML-KEM decapsulation support. Since this is deferred to a follow-up PR per the scope, optionally add a debug log message during wrap to document that recovery is unavailable until the decapsulation methods are implemented.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/kra/src/main/java/com/netscape/kra/StorageKeyUnit.java` around lines 1185 - 1223, The ML-KEM branch in StorageKeyUnit.wrap() correctly uses CryptoUtil.encapsulateMLKEM(storagePubKey, params.getSkLength()) to produce kemResult.sharedSecret and kemResult.ciphertext, but recovery/unwrap support is not yet implemented; add a clear debug log inside the ML-KEM branch (in StorageKeyUnit.wrap()) after encapsulation to state that ML-KEM decapsulation/unwrap_session_key() support is not available yet and recovery is deferred to a follow-up PR (reference CryptoUtil.encapsulateMLKEM, StorageKeyUnit.wrap(), and unwrap_session_key for context).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@base/kra/src/main/java/com/netscape/kra/EnrollmentService.java`:
- Around line 412-417: The code documents that allowEncDecrypt_archival isn't
supported for ML-KEM storage but doesn't enforce it at runtime; before the
branch that calls mStorageUnit.encryptInternalPrivate() (the encrypt vs wrap
decision in EnrollmentService), query mStorageUnit.getPublicKey().getAlgorithm()
and if allowEncDecrypt_archival is true and the algorithm indicates ML-KEM
(e.g., startsWith("ML-KEM") case-insensitive) either set
allowEncDecrypt_archival=false and emit a clear logger.warn mentioning
EnrollmentService and ML-KEM, or throw a focused IllegalArgumentException with a
message that allowEncDecrypt_archival is not supported for ML-KEM storage so the
unsupported path is never taken.
In `@base/kra/src/main/java/com/netscape/kra/StorageKeyUnit.java`:
- Around line 1185-1223: The ML-KEM branch in StorageKeyUnit.wrap() correctly
uses CryptoUtil.encapsulateMLKEM(storagePubKey, params.getSkLength()) to produce
kemResult.sharedSecret and kemResult.ciphertext, but recovery/unwrap support is
not yet implemented; add a clear debug log inside the ML-KEM branch (in
StorageKeyUnit.wrap()) after encapsulation to state that ML-KEM
decapsulation/unwrap_session_key() support is not available yet and recovery is
deferred to a follow-up PR (reference CryptoUtil.encapsulateMLKEM,
StorageKeyUnit.wrap(), and unwrap_session_key for context).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72978e54-9452-44d2-b6ac-fd450a8d7d27
📒 Files selected for processing (2)
base/kra/src/main/java/com/netscape/kra/EnrollmentService.javabase/kra/src/main/java/com/netscape/kra/StorageKeyUnit.java
ladycfu
force-pushed
the
pqc-kra-archival
branch
3 times, most recently
from
f6bfdbc to
6a07d1e
Compare
This change adds ML-KEM key archival support for storage key wrapping:
**EnrollmentService.java:**
- Add missing imports for CryptoUtil and NoSuchAlgorithmException
- Update privateKeyData comment to clarify RSA/EC vs ML-KEM storage structure
- Add note that allowEncDecrypt_archival is not supported for ML-KEM at this time
- Existing ML-KEM metadata handling uses CryptoUtil.getMLKEMStrength()
- Calls StorageKeyUnit.wrap() at line 431 (no changes needed)
**StorageKeyUnit.java:**
- Detect ML-KEM storage certificate by checking algorithm name
- ML-KEM path: Use CryptoUtil.encapsulateMLKEM() to generate shared secret
and KEM ciphertext
- RSA/EC path: Existing session key generation flow (unchanged)
- Both paths: Wrap user private key with session key/shared secret using
AES-KWP
- Return SEQUENCE { session/ciphertext, wrappedPrivateKey } - same structure
for backward compatibility
- Update comments to clarify dual usage
The privateKeyData structure remains compatible:
- For RSA/EC storage: SEQUENCE { wrappedSessionKey, wrappedPrivateKey }
- For ML-KEM storage: SEQUENCE { kemCiphertext, wrappedPrivateKey }
Recovery (unwrap) support will be added in a separate PR.
IDM-5472
Assisted-by: Claude
|
1 participant
This change adds ML-KEM key archival support for storage key wrapping:
Depends on #5363 (CryptoUtil ML-KEM bits fix)
EnrollmentService.java:
StorageKeyUnit.java:
The privateKeyData structure remains compatible:
Recovery (unwrap) support will be added in a separate PR.
IDM-5472
Assisted-by: Claude
Summary by CodeRabbit
New Features