feat(sdk): DSPX-3383 add pure ML-KEM-768 and ML-KEM-1024 key wrapping#373
feat(sdk): DSPX-3383 add pure ML-KEM-768 and ML-KEM-1024 key wrapping#373sujankota wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds pure ML-KEM-768 and ML-KEM-1024 wrapping support in the SDK and PQC provider, updates TDF key access generation to emit ChangesPure ML-KEM Post-Quantum Cryptography Support
Sequence Diagram(s)sequenceDiagram
participant Script as test-mlkem.sh
participant KAS as KAS PublicKey API
participant CLI as Java cmdline
participant TDF as TDF manifest
Script->>KAS: Request ML-KEM public key
KAS-->>Script: SPKI PEM
Script->>CLI: Encrypt payload with MLKEM key type
CLI-->>Script: .tdf artifact
Script->>TDF: Read keyAccess[0]
TDF-->>Script: mlkem-wrapped metadata
Script->>CLI: Decrypt artifact
CLI-->>Script: Plaintext output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 adds pure ML-KEM (FIPS 203) post-quantum key wrapping support for mlkem:768 and mlkem:1024 to the Java SDK, integrating it into the TDF creation flow and adding end-to-end testing scripts. The review feedback recommends optimizing performance by reusing a single static SecureRandom instance, adding defensive null checks in MLKEMKeyPair to prevent potential NullPointerExceptions, and robustly handling trailing slashes in the test script's platform endpoint parsing.
| MLKEMKeyPair generate() { | ||
| SecureRandom random = new SecureRandom(); | ||
| MLKEMKeyPairGenerator gen = new MLKEMKeyPairGenerator(); | ||
| gen.init(new MLKEMKeyGenerationParameters(random, mlkemParams)); |
There was a problem hiding this comment.
Reuse the shared SECURE_RANDOM instance instead of instantiating a new one on every key generation.
| MLKEMKeyPair generate() { | |
| SecureRandom random = new SecureRandom(); | |
| MLKEMKeyPairGenerator gen = new MLKEMKeyPairGenerator(); | |
| gen.init(new MLKEMKeyGenerationParameters(random, mlkemParams)); | |
| MLKEMKeyPair generate() { | |
| MLKEMKeyPairGenerator gen = new MLKEMKeyPairGenerator(); | |
| gen.init(new MLKEMKeyGenerationParameters(SECURE_RANDOM, mlkemParams)); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
sdk/src/test/java/io/opentdf/platform/sdk/TDFMLKEMTest.java (1)
114-116: ⚡ Quick winAssert the recovered DEK actually decrypts the generated TDF.
MLKEMKeyPairTestalready proveswrapDEK/unwrapDEKcan round-trip a DEK, so this integration test should verify the TDF wiring. On Line 116, checking only.hasSize(32)would still pass ifcreateTDF(...)wrapped the wrong 32-byte buffer. Load the generated TDF with a local unwrap stub and assert the plaintext matches the original payload instead.As per coding guidelines, "sdk/src/test/java/**/*.java: Prefer focused unit tests over integration tests; keep network calls mocked (e.g., using MockWebServer)".
🤖 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 `@sdk/src/test/java/io/opentdf/platform/sdk/TDFMLKEMTest.java` around lines 114 - 116, The test currently only asserts the unwrapped DEK byte length; instead, after obtaining symKey from alg.unwrapDEK(privSeed, wrappedBytes) in TDFMLKEMTest, use the same local TDF load/decryption path (i.e., call createTDF(...) output loader or a local unwrap/decrypt helper) to decrypt the generated TDF payload with symKey and assert the resulting plaintext equals the original payload bytes; reference the createTDF(...) call that produced the TDF, the alg.unwrapDEK(...) call that yields symKey, and the wrappedBytes/privSeed values to wire the decryption and final equality assertion so the test verifies end-to-end TDF decryption rather than just key size.
🤖 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.
Inline comments:
In `@scripts/test-mlkem.sh`:
- Around line 121-127: The grpcurl pre-flight is using PLATFORM_ENDPOINT to
build host but the script exposes --kas-url; change the host extraction to use
KAS_URL instead of PLATFORM_ENDPOINT (i.e., replace
host="${PLATFORM_ENDPOINT#http://}"; host="${host#https://"} with host derived
from KAS_URL), so the grpcurl probe that runs in the loop (the resp=$(grpcurl
... kas.AccessService/PublicKey ...) invocation) targets the actual KAS service;
no other logic changes needed (keep the http/https stripping and the
alg_to_string usage).
In `@sdk/src/main/java/io/opentdf/platform/sdk/MLKEMKeyPair.java`:
- Around line 194-207: The method decodeSizedPemBlock currently dereferences pem
without checking for null; add an explicit null-check at the start of
decodeSizedPemBlock(String pem, String expectedType, int expectedSize) and throw
an SDKException (consistent with other parse failures) when pem is null (include
the expectedType in the message for context). This prevents a
NullPointerException and keeps error handling uniform before continuing with
header/footer parsing and base64 decoding.
In `@sdk/src/test/java/io/opentdf/platform/sdk/MLKEMKeyPairTest.java`:
- Around line 27-28: The DEK test vector uses String.getBytes() which depends on
the JVM default charset; update the DEK constant in MLKEMKeyPairTest to call
getBytes with an explicit Charset (e.g.,
"0123456789abcdef0123456789abcdef".getBytes(StandardCharsets.US_ASCII) or
StandardCharsets.UTF_8) and add the necessary import for StandardCharsets so the
test becomes portable across JVMs.
- Around line 90-95: The test tamperedCiphertextFailsAesGcmTag currently asserts
any Exception which is too broad; change it to assert the specific AES-GCM auth
failure by asserting a RuntimeException is thrown from alg.unwrapDEK(...) and
then assert that that RuntimeException's cause is a
javax.crypto.BadPaddingException (or the specific cause type produced by
AesGcm.decrypt). Locate the test method tamperedCiphertextFailsAesGcmTag in
MLKEMKeyPairTest, replace assertThrows(Exception.class, ...) with capturing the
RuntimeException via assertThrows(RuntimeException.class, ...) and add an
assertion that the thrown.getCause() is an instance of
javax.crypto.BadPaddingException. Ensure the assertions reference
alg.unwrapDEK(kp.getPrivateKey(), wrapped) and the AesGcm decrypt failure cause.
---
Nitpick comments:
In `@sdk/src/test/java/io/opentdf/platform/sdk/TDFMLKEMTest.java`:
- Around line 114-116: The test currently only asserts the unwrapped DEK byte
length; instead, after obtaining symKey from alg.unwrapDEK(privSeed,
wrappedBytes) in TDFMLKEMTest, use the same local TDF load/decryption path
(i.e., call createTDF(...) output loader or a local unwrap/decrypt helper) to
decrypt the generated TDF payload with symKey and assert the resulting plaintext
equals the original payload bytes; reference the createTDF(...) call that
produced the TDF, the alg.unwrapDEK(...) call that yields symKey, and the
wrappedBytes/privSeed values to wire the decryption and final equality assertion
so the test verifies end-to-end TDF decryption rather than just key size.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bdd21dc4-96e3-458d-adce-22fda003f3eb
📒 Files selected for processing (8)
scripts/README.mdscripts/test-mlkem.shsdk/pom.xmlsdk/src/main/java/io/opentdf/platform/sdk/KeyType.javasdk/src/main/java/io/opentdf/platform/sdk/MLKEMKeyPair.javasdk/src/main/java/io/opentdf/platform/sdk/TDF.javasdk/src/test/java/io/opentdf/platform/sdk/MLKEMKeyPairTest.javasdk/src/test/java/io/opentdf/platform/sdk/TDFMLKEMTest.java
Pure ML-KEM (FIPS 203) over the same KemProvider SPI introduced by PR #368 for hybrid PQC. The BC-backed implementation lives in sdk-pqc-bc; the core sdk jar gets two enum values and an isMLKEM() predicate — no BC compile-time references, so the FIPS profile still builds cleanly without sdk-pqc-bc. Wire format (matches platform PR #3491): wrappedKey = base64(mlkem_ct || AES-GCM(IV(12)||DEK||tag(16))) wrap key = HKDF-SHA256(salt=SHA-256("TDF"), ikm=mlkem_ss, L=32) keyAccess.type = "wrapped" (reuses RSA slot; KAS disambiguates by alg) ephemeralPublicKey absent PEM is standard SPKI/PKCS#8 with the NIST FIPS 203 OIDs (2.16.840.1.101.3.4.4.{2,3}) inside the AlgorithmIdentifier. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sujankota
force-pushed
the
DSPX-3383-pure-mlkem
branch
from
232a805 to
e2e5581
Compare
X-Test Failure Report |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
sdk-pqc-bc/src/test/java/io/opentdf/platform/sdk/pqc/bc/MLKEMKeyPairTest.java (1)
106-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow tamper-path assertion to the unwrap contract.
Line 106 uses
assertThrows(Exception.class), which can pass on unrelated failures. Assert the concrete unwrap failure type (SDKException) so this test only validates auth-failure behavior.Proposed change
- assertThrows(Exception.class, + assertThrows(SDKException.class, () -> algo.unwrapDEK(kp.getPrivateKey(), wrapped), "tampered AES-GCM tag must not unwrap");🤖 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 `@sdk-pqc-bc/src/test/java/io/opentdf/platform/sdk/pqc/bc/MLKEMKeyPairTest.java` around lines 106 - 108, The test assertion in the unwrapDEK test case uses the overly broad Exception.class type in assertThrows, which allows the test to pass even if unrelated failures occur instead of the expected authentication failure. Change the assertThrows call to specify the concrete exception type SDKException instead of Exception.class. This ensures the test validates the specific unwrap contract failure for tampered AES-GCM authentication tags rather than catching any exception that might be thrown.
🤖 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.
Inline comments:
In
`@sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/BouncyCastleKemProvider.java`:
- Around line 27-42: The supportedKeyTypes() method in BouncyCastleKemProvider
returns the static SUPPORTED EnumSet directly, allowing callers to mutate it and
affect provider capabilities for all future lookups. Modify the return statement
to wrap SUPPORTED in Collections.unmodifiableSet() or use Set.copyOf() to return
an immutable copy instead of exposing the mutable EnumSet directly.
In `@sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridCrypto.java`:
- Around line 119-124: The addition of c.pos and seqLen can cause integer
overflow before the buffer bounds comparison is performed, allowing malformed
ASN.1 envelopes with large lengths to bypass validation. Check for overflow
before calculating seqEnd by verifying that c.pos does not exceed
Integer.MAX_VALUE minus seqLen. Apply the same overflow-safe validation pattern
to the similar calculation at line 142-146 where c.pos and len are combined.
This ensures that integer wraparound cannot be exploited to bypass buffer bounds
checks.
In `@sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridSpki.java`:
- Around line 105-116: The catch block in the decodeSpkiPem method only catches
IOException, but BouncyCastle's getInstance() methods can also throw unchecked
IllegalArgumentException when the ASN.1 structure is malformed. Modify the catch
clause to catch both IOException and IllegalArgumentException together (e.g.,
catch (IOException | IllegalArgumentException e)), ensuring both exceptions are
wrapped and re-thrown as SDKException with appropriate error context. Apply this
same fix to the two other similar decode methods at lines 123-134 and 154-160.
In `@sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/MLKEMAlgorithm.java`:
- Around line 148-160: The ML-KEM encapsulation handling in MLKEMAlgorithm
leaves sensitive secret material in memory because SecretWithEncapsulation is
never destroyed. Update the encrypt/marshal flow around the generateEncapsulated
call to use try/finally, ensure enc.destroy() is always invoked, and zeroize the
local wrapKey buffer with Arrays.fill after it has been used by AesGcm. Keep the
existing ciphertext and size checks in place, but make sure cleanup happens
regardless of success or failure.
---
Nitpick comments:
In
`@sdk-pqc-bc/src/test/java/io/opentdf/platform/sdk/pqc/bc/MLKEMKeyPairTest.java`:
- Around line 106-108: The test assertion in the unwrapDEK test case uses the
overly broad Exception.class type in assertThrows, which allows the test to pass
even if unrelated failures occur instead of the expected authentication failure.
Change the assertThrows call to specify the concrete exception type SDKException
instead of Exception.class. This ensures the test validates the specific unwrap
contract failure for tampered AES-GCM authentication tags rather than catching
any exception that might be thrown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7ae0bef-93b9-4c00-a448-4ccf1dfccfb9
📒 Files selected for processing (11)
scripts/README.mdscripts/test-mlkem.shsdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/BouncyCastleKemProvider.javasdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridCrypto.javasdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridSpki.javasdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/MLKEMAlgorithm.javasdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/MLKEMKeyPair.javasdk-pqc-bc/src/test/java/io/opentdf/platform/sdk/TDFMLKEMTest.javasdk-pqc-bc/src/test/java/io/opentdf/platform/sdk/pqc/bc/MLKEMKeyPairTest.javasdk/src/main/java/io/opentdf/platform/sdk/KeyType.javasdk/src/main/java/io/opentdf/platform/sdk/TDF.java
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/src/main/java/io/opentdf/platform/sdk/KeyType.java
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🧹 Nitpick comments (1)
sdk-pqc-bc/src/test/java/io/opentdf/platform/sdk/pqc/bc/MLKEMKeyPairTest.java (1)
106-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow tamper-path assertion to the unwrap contract.
Line 106 uses
assertThrows(Exception.class), which can pass on unrelated failures. Assert the concrete unwrap failure type (SDKException) so this test only validates auth-failure behavior.Proposed change
- assertThrows(Exception.class, + assertThrows(SDKException.class, () -> algo.unwrapDEK(kp.getPrivateKey(), wrapped), "tampered AES-GCM tag must not unwrap");🤖 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 `@sdk-pqc-bc/src/test/java/io/opentdf/platform/sdk/pqc/bc/MLKEMKeyPairTest.java` around lines 106 - 108, The test assertion in the unwrapDEK test case uses the overly broad Exception.class type in assertThrows, which allows the test to pass even if unrelated failures occur instead of the expected authentication failure. Change the assertThrows call to specify the concrete exception type SDKException instead of Exception.class. This ensures the test validates the specific unwrap contract failure for tampered AES-GCM authentication tags rather than catching any exception that might be thrown.
🤖 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.
Inline comments:
In
`@sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/BouncyCastleKemProvider.java`:
- Around line 27-42: The supportedKeyTypes() method in BouncyCastleKemProvider
returns the static SUPPORTED EnumSet directly, allowing callers to mutate it and
affect provider capabilities for all future lookups. Modify the return statement
to wrap SUPPORTED in Collections.unmodifiableSet() or use Set.copyOf() to return
an immutable copy instead of exposing the mutable EnumSet directly.
In `@sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridCrypto.java`:
- Around line 119-124: The addition of c.pos and seqLen can cause integer
overflow before the buffer bounds comparison is performed, allowing malformed
ASN.1 envelopes with large lengths to bypass validation. Check for overflow
before calculating seqEnd by verifying that c.pos does not exceed
Integer.MAX_VALUE minus seqLen. Apply the same overflow-safe validation pattern
to the similar calculation at line 142-146 where c.pos and len are combined.
This ensures that integer wraparound cannot be exploited to bypass buffer bounds
checks.
In `@sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridSpki.java`:
- Around line 105-116: The catch block in the decodeSpkiPem method only catches
IOException, but BouncyCastle's getInstance() methods can also throw unchecked
IllegalArgumentException when the ASN.1 structure is malformed. Modify the catch
clause to catch both IOException and IllegalArgumentException together (e.g.,
catch (IOException | IllegalArgumentException e)), ensuring both exceptions are
wrapped and re-thrown as SDKException with appropriate error context. Apply this
same fix to the two other similar decode methods at lines 123-134 and 154-160.
In `@sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/MLKEMAlgorithm.java`:
- Around line 148-160: The ML-KEM encapsulation handling in MLKEMAlgorithm
leaves sensitive secret material in memory because SecretWithEncapsulation is
never destroyed. Update the encrypt/marshal flow around the generateEncapsulated
call to use try/finally, ensure enc.destroy() is always invoked, and zeroize the
local wrapKey buffer with Arrays.fill after it has been used by AesGcm. Keep the
existing ciphertext and size checks in place, but make sure cleanup happens
regardless of success or failure.
---
Nitpick comments:
In
`@sdk-pqc-bc/src/test/java/io/opentdf/platform/sdk/pqc/bc/MLKEMKeyPairTest.java`:
- Around line 106-108: The test assertion in the unwrapDEK test case uses the
overly broad Exception.class type in assertThrows, which allows the test to pass
even if unrelated failures occur instead of the expected authentication failure.
Change the assertThrows call to specify the concrete exception type SDKException
instead of Exception.class. This ensures the test validates the specific unwrap
contract failure for tampered AES-GCM authentication tags rather than catching
any exception that might be thrown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7ae0bef-93b9-4c00-a448-4ccf1dfccfb9
📒 Files selected for processing (11)
scripts/README.mdscripts/test-mlkem.shsdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/BouncyCastleKemProvider.javasdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridCrypto.javasdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridSpki.javasdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/MLKEMAlgorithm.javasdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/MLKEMKeyPair.javasdk-pqc-bc/src/test/java/io/opentdf/platform/sdk/TDFMLKEMTest.javasdk-pqc-bc/src/test/java/io/opentdf/platform/sdk/pqc/bc/MLKEMKeyPairTest.javasdk/src/main/java/io/opentdf/platform/sdk/KeyType.javasdk/src/main/java/io/opentdf/platform/sdk/TDF.java
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/src/main/java/io/opentdf/platform/sdk/KeyType.java
🛑 Comments failed to post (4)
sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/BouncyCastleKemProvider.java (1)
27-42: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not expose the mutable
EnumSet.
supportedKeyTypes()returns the staticEnumSetdirectly, so any caller can mutate provider capabilities for all future lookups. Return an immutable set or a defensive copy.Proposed fix
+import java.util.Collections; import java.util.EnumSet; import java.util.Set; @@ - private static final Set<KeyType> SUPPORTED = EnumSet.of( + private static final Set<KeyType> SUPPORTED = Collections.unmodifiableSet(EnumSet.of( KeyType.HybridXWingKey, KeyType.HybridSecp256r1MLKEM768Key, KeyType.HybridSecp384r1MLKEM1024Key, KeyType.MLKEM768Key, - KeyType.MLKEM1024Key); + KeyType.MLKEM1024Key));🤖 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 `@sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/BouncyCastleKemProvider.java` around lines 27 - 42, The supportedKeyTypes() method in BouncyCastleKemProvider returns the static SUPPORTED EnumSet directly, allowing callers to mutate it and affect provider capabilities for all future lookups. Modify the return statement to wrap SUPPORTED in Collections.unmodifiableSet() or use Set.copyOf() to return an immutable copy instead of exposing the mutable EnumSet directly.sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridCrypto.java (1)
119-124: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use overflow-safe ASN.1 length bounds checks.
c.pos + seqLenandc.pos + lencan overflow before the buffer comparison. For malformed long-form lengths, the element path can then attempt a huge allocation instead of rejecting the envelope.Proposed fix
} int seqLen = readLength(c); - int seqEnd = c.pos + seqLen; - if (seqEnd > der.length) { + if (seqLen > der.length - c.pos) { throw new SDKException("hybrid wrapped key envelope length exceeds buffer"); } + int seqEnd = c.pos + seqLen; if (seqEnd != der.length) { throw new SDKException("hybrid wrapped key envelope has trailing bytes"); } @@ } int len = readLength(c); - if (c.pos + len > c.buf.length) { + if (len > c.buf.length - c.pos) { throw new SDKException("context-tagged element length exceeds buffer"); }Also applies to: 142-146
🤖 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 `@sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridCrypto.java` around lines 119 - 124, The addition of c.pos and seqLen can cause integer overflow before the buffer bounds comparison is performed, allowing malformed ASN.1 envelopes with large lengths to bypass validation. Check for overflow before calculating seqEnd by verifying that c.pos does not exceed Integer.MAX_VALUE minus seqLen. Apply the same overflow-safe validation pattern to the similar calculation at line 142-146 where c.pos and len are combined. This ensures that integer wraparound cannot be exploited to bypass buffer bounds checks.sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridSpki.java (1)
105-116: 🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
fd -t f "HybridSpki.java" --absoluteRepository: opentdf/java-sdk
Length of output: 378
🏁 Script executed:
git ls-files "sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridSpki.java"Repository: opentdf/java-sdk
Length of output: 226
🏁 Script executed:
cat -n sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridSpki.java | sed -n '100,165p'Repository: opentdf/java-sdk
Length of output: 3538
🌐 Web query:
BouncyCastle ASN1Primitive.fromByteArray IllegalArgumentException exception handling💡 Result:
The method org.bouncycastle.asn1.ASN1Primitive.fromByteArray(byte[] data) is designed to parse well-formed ASN.1 DER data and is explicitly documented to throw a java.io.IOException when it encounters parsing issues [1][2][3]. When an IllegalArgumentException occurs in conjunction with this method, it typically originates from deeper validation logic within the BouncyCastle ASN.1 classes (such as ASN1Integer, ASN1Boolean, or ASN1ObjectIdentifier) during the reconstruction of primitive objects [4][5][6]. Handling this Exception 1. Verify Data Integrity: The most common cause is malformed or non-compliant ASN.1 data (e.g., integers not encoded in the smallest possible octets, invalid OID encodings, or incorrect boolean lengths) [4][5][6]. Ideally, the source of the data should be corrected to adhere to ASN.1/DER encoding rules [4]. 2. Use System Properties for Workarounds: For cases where you must process non-compliant data that cannot be fixed at the source, BouncyCastle provides system properties to relax validation rules for specific known issues [4][6]. These properties can be set to "true" to bypass certain strict checks: - org.bouncycastle.asn1.allow_unsafe_integer: Allows non-minimal integer encodings [4]. - org.bouncycastle.asn1.allow_wrong_oid_enc: Allows specific invalid OID encodings [6]. If possible, these properties should be applied locally using org.bouncycastle.util.Properties.setThreadOverride(property, value) to minimize security risks and avoid affecting the entire application [4]. 3. Proper Exception Catching: Because ASN1Primitive.fromByteArray declares that it throws IOException, you should ensure your application correctly handles IOException [1][2]. If your code also encounters IllegalArgumentException (which is a RuntimeException) during this process, it often indicates an underlying violation of DER encoding rules that BouncyCastle's strict internal validation cannot parse [4][5][6]. Catching IllegalArgumentException directly is sometimes necessary when dealing with unpredictable or legacy third-party data, but it is best practice to investigate the specific cause (e.g., corrupted stream, invalid object type) to ensure it is safe to proceed [4][7][8]. Important Note: Always use consistent versions of BouncyCastle provider jars to avoid classpath conflicts, which can also trigger unexpected IllegalArgumentExceptions during object instantiation [7].
Citations:
- 1: https://downloads.bouncycastle.org/lts-java/docs/bcprov-lts8on-2.73.5-javadoc/org/bouncycastle/asn1/ASN1Primitive.html
- 2: https://github.com/bcgit/bc-java/blob/master/core/src/main/java/org/bouncycastle/asn1/ASN1Primitive.java
- 3: https://downloads.bouncycastle.org/java/docs/bcprov-jdk15to18-javadoc/org/bouncycastle/asn1/ASN1Primitive.html
- 4: bcgit/bc-java#577
- 5: bcgit/bc-java#1614
- 6: bcgit/bc-java#1758
- 7: bcgit/bc-java#213
- 8: bcgit/bc-java#732
🌐 Web query:
BouncyCastle SubjectPublicKeyInfo.getInstance() throws exceptions IllegalArgumentException💡 Result:
The java.lang.IllegalArgumentException thrown by Bouncy Castle's SubjectPublicKeyInfo.getInstance typically indicates that the provided input object is not a valid ASN.1 encoded structure, or it does not conform to the expected X.509 SubjectPublicKeyInfo sequence [1]. Common causes include: 1. Invalid Input Data: The input byte array may be corrupted, incomplete, or formatted in a non-ASN.1 format (e.g., raw key material, SSH public key format, or X/Y coordinates without the required OID and metadata headers) [2][3]. SubjectPublicKeyInfo expects an ASN.1 encoded sequence containing both the AlgorithmIdentifier and the subjectPublicKey (bit string) [1]. 2. Incorrect Data Type: Passing an object that is not an ASN.1 sequence (e.g., just the raw key bits or a different ASN.1 structure) will trigger a failure during decoding [3][4]. 3. Missing Metadata: If you are attempting to create a SubjectPublicKeyInfo from raw public key bytes (like EC coordinates), you must manually wrap the raw bytes with the appropriate AlgorithmIdentifier (OID) and structure rather than passing the raw bytes directly to getInstance [5][3]. To resolve this, ensure that: - The byte array passed to getInstance is a DER/BER encoded SubjectPublicKeyInfo structure, such as that returned by PublicKey.getEncoded from standard Java JCA objects [2][4]. - If you have raw key material, use the appropriate Bouncy Castle factory methods (e.g., PublicKeyFactory.createKey) or explicitly construct a new SubjectPublicKeyInfo instance using the correct AlgorithmIdentifier and key data [5][3]. You can inspect the contents of your byte array to verify it starts with an ASN.1 Sequence tag (0x30) before attempting to parse it [1].
Citations:
- 1: https://github.com/bcgit/bc-java/blob/master/core/src/main/java/org/bouncycastle/asn1/x509/SubjectPublicKeyInfo.java
- 2: https://stackoverflow.com/questions/78782369/how-to-convert-ssh-public-key-to-publickey-in-java
- 3: bcgit/bc-java#1244
- 4: bcgit/bc-java#1683
- 5: https://stackoverflow.com/questions/4828818/obtaining-public-key-from-subjectpublickeyinfo
Catch
IllegalArgumentExceptionalongsideIOExceptionin ASN.1 decode methods.BouncyCastle's
getInstance()methods (SubjectPublicKeyInfo, PrivateKeyInfo, ECPrivateKey) throw uncheckedIllegalArgumentExceptionwhen the input is not a valid ASN.1 encoded structure or doesn't conform to the expected format. The current exception handlers only catchIOException, allowing these unchecked exceptions to escape and violate the SDK's error contract. AddIllegalArgumentExceptionto all three catch blocks to normalize malformed DER errors toSDKException.Proposed fix
- } catch (IOException e) { + } catch (IOException | IllegalArgumentException e) { throw new SDKException("failed to parse SPKI", e); } @@ - } catch (IOException e) { + } catch (IOException | IllegalArgumentException e) { throw new SDKException("failed to parse PKCS#8", e); } @@ - } catch (IOException e) { + } catch (IOException | IllegalArgumentException e) { throw new SDKException("failed to parse RFC 5915 ECPrivateKey", e); }Applies to lines 105–116, 123–134, and 154–160.
🤖 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 `@sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridSpki.java` around lines 105 - 116, The catch block in the decodeSpkiPem method only catches IOException, but BouncyCastle's getInstance() methods can also throw unchecked IllegalArgumentException when the ASN.1 structure is malformed. Modify the catch clause to catch both IOException and IllegalArgumentException together (e.g., catch (IOException | IllegalArgumentException e)), ensuring both exceptions are wrapped and re-thrown as SDKException with appropriate error context. Apply this same fix to the two other similar decode methods at lines 123-134 and 154-160.sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/MLKEMAlgorithm.java (1)
148-160: 🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "MLKEMAlgorithm.java" | head -5Repository: opentdf/java-sdk
Length of output: 232
🏁 Script executed:
# Once we find the file, check its content around lines 148-160 if [ -f "sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/MLKEMAlgorithm.java" ]; then echo "=== File exists, checking line range 140-170 ===" sed -n '140,170p' "sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/MLKEMAlgorithm.java" fiRepository: opentdf/java-sdk
Length of output: 2024
🏁 Script executed:
# Check the full structure of the file to understand imports and class definition ast-grep outline "sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/MLKEMAlgorithm.java" --view expanded | head -100Repository: opentdf/java-sdk
Length of output: 1111
🌐 Web query:
BouncyCastle SecretWithEncapsulation Destroyable interface lifecycle management💡 Result:
In the Bouncy Castle cryptography library, the SecretWithEncapsulation interface is used to handle secret keys generated during encapsulation processes, such as in Key Encapsulation Mechanisms (KEM) [1][2]. This interface extends the javax.security.auth.Destroyable interface [1][3], which provides the standard Java mechanism for lifecycle management of sensitive objects [1][4]. Lifecycle Management via Destroyable: The Destroyable interface allows implementing classes to explicitly release sensitive data to mitigate security risks associated with keeping secrets in memory [1][5]. 1. destroy: Calling this method signals the object to destroy its sensitive contents (typically by zeroing out the underlying byte arrays) [5][6]. Once destroyed, attempts to use the object may result in an IllegalStateException or equivalent [5]. 2. isDestroyed: This method allows callers to check whether the object has already been destroyed, which is crucial for safe memory management and preventing use-after-destroy errors [5]. Implementation Details: Classes implementing SecretWithEncapsulation, such as SecretWithEncapsulationImpl, are responsible for overriding the destroy method to ensure that both the secret key and any associated sensitive data are cleared from memory [5]. When managing the lifecycle, developers should treat these objects as temporary, calling destroy as soon as the secret is no longer needed (e.g., after it has been used to derive a session key or perform further cryptographic operations). Because SecretWithEncapsulation extends Destroyable, it aligns with broader Bouncy Castle practices, where various sensitive key types (such as private keys) also implement Destroyable to support proactive memory zeroization [7][8]. In specific FIPS-compliant contexts, Bouncy Castle may also offer specialized interfaces like ZeroizableSecretKey for more explicit, manual zeroization control [6].
Citations:
- 1: https://downloads.bouncycastle.org/java/docs/bcprov-jdk15to18-javadoc/org/bouncycastle/crypto/SecretWithEncapsulation.html
- 2: https://downloads.bouncycastle.org/lts-java/docs/bcprov-lts8on-2.73.11-javadoc/org/bouncycastle/crypto/SecretWithEncapsulation.html
- 3: https://downloads.bouncycastle.org/java/docs/bcprov-jdk18on-javadoc/org/bouncycastle/crypto/SecretWithEncapsulation.html
- 4: https://downloads.bouncycastle.org/lts-java/docs/bcprov-lts8on-2.73.5-javadoc/org/bouncycastle/crypto/SecretWithEncapsulation.html
- 5: https://downloads.bouncycastle.org/java/docs/bcprov-jdk15to18-javadoc/org/bouncycastle/pqc/crypto/util/SecretWithEncapsulationImpl.html
- 6: https://downloads.bouncycastle.org/fips-java/docs/bc-fips-2.1.0-javadoc/org/bouncycastle/jcajce/ZeroizableSecretKey.html
- 7: https://downloads.bouncycastle.org/lts-java/docs/bcprov-lts8on-2.73.5-javadoc/org/bouncycastle/jce/interfaces/ECPrivateKey.html
- 8: https://downloads.bouncycastle.org/lts-java/docs/bcprov-lts8on-2.73.5-javadoc/org/bouncycastle/pqc/jcajce/provider/lms/BCLMSPrivateKey.html
Destroy the encapsulation secret after use.
SecretWithEncapsulationextendsDestroyableper BouncyCastle's API contract. The current code extracts the secret and encapsulation but never callsdestroy()onenc, leaving sensitive material in memory until garbage collection. Wrap the encryption/marshal logic intry/finally, callenc.destroy(), and zeroize the localwrapKeycopy withArrays.fill()after use.Proposed fix
MLKEMPublicKeyParameters pub = new MLKEMPublicKeyParameters(mlkemParams, rawPub); SecretWithEncapsulation enc = new MLKEMGenerator(SECURE_RANDOM).generateEncapsulated(pub); - byte[] wrapKey = enc.getSecret(); // 32-byte AES key, used directly (no HKDF) - byte[] ciphertext = enc.getEncapsulation(); - if (ciphertext.length != ciphertextSize) { - throw new SDKException("ML-KEM ciphertext size " + ciphertext.length + " != expected " + ciphertextSize); - } - if (wrapKey.length != HybridCrypto.WRAP_KEY_SIZE) { - throw new SDKException("ML-KEM shared secret size " + wrapKey.length - + " != expected " + HybridCrypto.WRAP_KEY_SIZE); - } - byte[] encryptedDek = new AesGcm(wrapKey).encrypt(dek).asBytes(); - return HybridCrypto.marshalEnvelope(ciphertext, encryptedDek); + byte[] wrapKey = enc.getSecret(); // 32-byte AES key, used directly (no HKDF) + try { + byte[] ciphertext = enc.getEncapsulation(); + if (ciphertext.length != ciphertextSize) { + throw new SDKException("ML-KEM ciphertext size " + ciphertext.length + " != expected " + ciphertextSize); + } + if (wrapKey.length != HybridCrypto.WRAP_KEY_SIZE) { + throw new SDKException("ML-KEM shared secret size " + wrapKey.length + + " != expected " + HybridCrypto.WRAP_KEY_SIZE); + } + byte[] encryptedDek = new AesGcm(wrapKey).encrypt(dek).asBytes(); + return HybridCrypto.marshalEnvelope(ciphertext, encryptedDek); + } finally { + java.util.Arrays.fill(wrapKey, (byte) 0); + enc.destroy(); + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.MLKEMPublicKeyParameters pub = new MLKEMPublicKeyParameters(mlkemParams, rawPub); SecretWithEncapsulation enc = new MLKEMGenerator(SECURE_RANDOM).generateEncapsulated(pub); byte[] wrapKey = enc.getSecret(); // 32-byte AES key, used directly (no HKDF) try { byte[] ciphertext = enc.getEncapsulation(); if (ciphertext.length != ciphertextSize) { throw new SDKException("ML-KEM ciphertext size " + ciphertext.length + " != expected " + ciphertextSize); } if (wrapKey.length != HybridCrypto.WRAP_KEY_SIZE) { throw new SDKException("ML-KEM shared secret size " + wrapKey.length " != expected " + HybridCrypto.WRAP_KEY_SIZE); } byte[] encryptedDek = new AesGcm(wrapKey).encrypt(dek).asBytes(); return HybridCrypto.marshalEnvelope(ciphertext, encryptedDek); } finally { java.util.Arrays.fill(wrapKey, (byte) 0); enc.destroy(); }🤖 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 `@sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/MLKEMAlgorithm.java` around lines 148 - 160, The ML-KEM encapsulation handling in MLKEMAlgorithm leaves sensitive secret material in memory because SecretWithEncapsulation is never destroyed. Update the encrypt/marshal flow around the generateEncapsulated call to use try/finally, ensure enc.destroy() is always invoked, and zeroize the local wrapKey buffer with Arrays.fill after it has been used by AesGcm. Keep the existing ciphertext and size checks in place, but make sure cleanup happens regardless of success or failure.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
2 participants
Overview
Adds pure ML-KEM-768 (
mlkem:768) and ML-KEM-1024 (mlkem:1024) post-quantum key wrapping, on top of theKemProviderSPI introduced by PR #368.The BC-backed implementation lives in
sdk-pqc-bc(already on the classpath for hybrid PQC); the coresdkjar only gets twoKeyTypeconstants and anisMLKEM()predicate. No new BC compile-time references — the FIPS Maven profile still builds cleanly withoutsdk-pqc-bc.Conforms to open platform PR opentdf/platform#3562 (replacement for closed #3491).
Wire format
wrappedKey = base64( ASN.1 SEQUENCE { [0] mlkem_ct, [1] AES-GCM(IV(12)||DEK||tag(16)) } )
AES wrap key = mlkem_decaps_shared_secret ← used DIRECTLY, no HKDF
keyAccess.type = "mlkem-wrapped" ← its own KAO scheme
ephemeralPublicKey = absent
Envelope is byte-identical to the hybrid path. Only the wrap-key derivation differs: hybrid still uses HKDF (it's the combiner over two shared-secret
halves); pure ML-KEM uses the 32-byte FIPS 203 Decaps output directly. Rationale: HSM-backed KAS (Thales Luna 7.15.1, strict-FIPS) emits Decaps output only as a
non-extractable
CKK_AESobject — any KDF in the unwrap chain blocks HSM rewrap. FIPS 203's K is already uniformly random; the"mlkem-wrapped"wire typeitself is the domain-separation tag. See platform ADR
2026-06-16-mlkem-direct-key-wrap.md.PEM: standard SPKI / PKCS#8 with NIST FIPS 203 OIDs
2.16.840.1.101.3.4.4.{2,3}.Changes
sdk/.../KeyType.javaMLKEM768Key,MLKEM1024Key,isMLKEM()sdk/.../TDF.javaisMLKEM()dispatch branch →KemProviders.get(kt).wrapDEK(...),kMlkemWrappedconstantsdk-pqc-bc/.../MLKEMAlgorithm.java(new)sdk-pqc-bc/.../MLKEMKeyPair.java(new)sdk-pqc-bc/.../HybridSpki.javasdk-pqc-bc/.../HybridCrypto.javasdk-pqc-bc/.../BouncyCastleKemProvider.javaSUPPORTEDscripts/test-mlkem.sh(new),scripts/README.mdPlus
MLKEMKeyPairTest(10 tests) andTDFMLKEMTest(2 tests).Verification
mvn clean install -DskipTests(default)mvn -P fips,!non-fips clean install -DskipTestsmvn -pl sdk,sdk-pqc-bc -am testKnown gap
Platform proto v0.34.0 doesn't yet define
ALGORITHM_MLKEM_768/_1024, soKeyType.fromAlgorithmcan't auto-map them. Workaround:--encap-key-type=MLKEM*Keyexplicit. Two switch cases when the proto bump lands.
Dependency
Platform PR #3562 is open, not merged — KAS rewrap requires its server-side changes. If reviewers change #3562 before merge, this PR may need a small
follow-up.
Summary by CodeRabbit
New Features
Documentation
Tests