feat(sdk): DSPX-3309 add hybrid post-quantum key wrapping for KAS (X-Wing, ECDH+ML-KEM)#368
feat(sdk): DSPX-3309 add hybrid post-quantum key wrapping for KAS (X-Wing, ECDH+ML-KEM)#368sujankota wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements hybrid post-quantum cryptography (X‑Wing and NIST ECDH+ML‑KEM hybrids) for TDF, including envelope marshaling, wrap-key derivation via HKDF-SHA256 and AES-GCM encryption, TDF manifest integration, comprehensive unit and integration tests, Maven build wiring, and a developer-facing integration test script with documentation. ChangesHybrid Post-Quantum Cryptography
Maven Build Infrastructure
Integration Test Script and Documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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 introduces support for hybrid post-quantum key wrapping, specifically X-Wing (X25519 + ML-KEM-768) and NIST hybrid schemes (P-256/P-384 + ML-KEM). It adds new classes for cryptographic operations and ASN.1 envelope management, updates the KeyType enum, and integrates these capabilities into the TDF creation process. Feedback recommends specifying the UTF-8 character set when converting strings to bytes and suggests explicitly referencing the BouncyCastle provider in cryptographic calls to ensure platform consistency and avoid provider ambiguity.
| static byte[] defaultTDFSalt() { | ||
| try { | ||
| MessageDigest d = MessageDigest.getInstance("SHA-256"); | ||
| d.update("TDF".getBytes()); |
There was a problem hiding this comment.
It is recommended to specify a character set when calling getBytes() to ensure consistent behavior across different platforms. Using StandardCharsets.UTF_8 is a best practice.
| d.update("TDF".getBytes()); | |
| d.update("TDF".getBytes(java.nio.charset.StandardCharsets.UTF_8)); |
| PublicKey peerPub = kf.generatePublic(peerSpec); | ||
| PrivateKey myPriv = kf.generatePrivate(mySpec); | ||
|
|
||
| KeyAgreement ka = KeyAgreement.getInstance("ECDH", BouncyCastleProvider.PROVIDER_NAME); |
There was a problem hiding this comment.
When using KeyAgreement.getInstance, it is generally safer to use the provider instance directly if it has already been loaded, or ensure that the provider name is correctly referenced. While "BC" is standard for BouncyCastle, consider if the provider should be explicitly passed to avoid ambiguity in environments with multiple providers.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
sdk/src/main/java/io/opentdf/platform/sdk/XWingKeyPair.java (1)
12-13: ⚡ Quick winClear the derived secrets after wrap/unwrap.
sharedSecretandwrapKeystay live until GC on both paths. In the wrapping primitive itself, this is worth clearing in afinallyblock once AES-GCM completes.💡 Suggested fix
import java.security.SecureRandom; +import java.util.Arrays; @@ - SecretWithEncapsulation enc = new XWingKEMGenerator(new SecureRandom()).generateEncapsulated(pub); - byte[] sharedSecret = enc.getSecret(); - byte[] ciphertext = enc.getEncapsulation(); - - byte[] wrapKey = HybridCrypto.deriveWrapKey(sharedSecret, null, null); - byte[] encryptedDek = new AesGcm(wrapKey).encrypt(dek).asBytes(); - return HybridCrypto.marshalEnvelope(ciphertext, encryptedDek); + SecretWithEncapsulation enc = new XWingKEMGenerator(new SecureRandom()).generateEncapsulated(pub); + byte[] sharedSecret = enc.getSecret(); + byte[] ciphertext = enc.getEncapsulation(); + byte[] wrapKey = null; + try { + wrapKey = HybridCrypto.deriveWrapKey(sharedSecret, null, null); + byte[] encryptedDek = new AesGcm(wrapKey).encrypt(dek).asBytes(); + return HybridCrypto.marshalEnvelope(ciphertext, encryptedDek); + } finally { + Arrays.fill(sharedSecret, (byte) 0); + if (wrapKey != null) { + Arrays.fill(wrapKey, (byte) 0); + } + } @@ - byte[] sharedSecret = new XWingKEMExtractor(priv).extractSecret(ciphertext); - byte[] wrapKey = HybridCrypto.deriveWrapKey(sharedSecret, null, null); - return new AesGcm(wrapKey).decrypt(new AesGcm.Encrypted(encryptedDek)); + byte[] sharedSecret = new XWingKEMExtractor(priv).extractSecret(ciphertext); + byte[] wrapKey = null; + try { + wrapKey = HybridCrypto.deriveWrapKey(sharedSecret, null, null); + return new AesGcm(wrapKey).decrypt(new AesGcm.Encrypted(encryptedDek)); + } finally { + Arrays.fill(sharedSecret, (byte) 0); + if (wrapKey != null) { + Arrays.fill(wrapKey, (byte) 0); + } + }Also applies to: 67-73, 87-90
🤖 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/main/java/io/opentdf/platform/sdk/XWingKeyPair.java` around lines 12 - 13, In XWingKeyPair.java ensure derived secrets are explicitly cleared after use: locate the methods that perform wrapping/unwrapping (e.g., the wrap/unwrap primitives around sharedSecret and wrapKey) and add try { ... } finally { Arrays.fill(sharedSecret, (byte)0); Arrays.fill(wrapKey, (byte)0); sharedSecret = null; wrapKey = null; } (or equivalent) so both success and exception paths wipe and null out the byte[] secrets; apply the same pattern to the other occurrences noted around the blocks at the other wrap/unwrap usages (lines referenced 67-73 and 87-90).sdk/src/main/java/io/opentdf/platform/sdk/HybridNISTKeyPair.java (1)
203-207: ⚡ Quick winZero the hybrid shared secrets on both paths.
ecdhSecret,mlSecret,combinedSecret, andwrapKeyall remain in heap memory after the DEK is processed. Given this sits at the core crypto boundary, clear them infinallyblocks.💡 Suggested fix
- byte[] combinedSecret = concat(ecdhSecret, mlSecret); - byte[] hybridCt = concat(ephemeralEcPub, mlCiphertext); - byte[] wrapKey = HybridCrypto.deriveWrapKey(combinedSecret, null, null); - byte[] encryptedDek = new AesGcm(wrapKey).encrypt(dek).asBytes(); - return HybridCrypto.marshalEnvelope(hybridCt, encryptedDek); + byte[] combinedSecret = null; + byte[] wrapKey = null; + try { + combinedSecret = concat(ecdhSecret, mlSecret); + byte[] hybridCt = concat(ephemeralEcPub, mlCiphertext); + wrapKey = HybridCrypto.deriveWrapKey(combinedSecret, null, null); + byte[] encryptedDek = new AesGcm(wrapKey).encrypt(dek).asBytes(); + return HybridCrypto.marshalEnvelope(hybridCt, encryptedDek); + } finally { + Arrays.fill(ecdhSecret, (byte) 0); + Arrays.fill(mlSecret, (byte) 0); + if (combinedSecret != null) { + Arrays.fill(combinedSecret, (byte) 0); + } + if (wrapKey != null) { + Arrays.fill(wrapKey, (byte) 0); + } + } @@ - byte[] combinedSecret = concat(ecdhSecret, mlSecret); - byte[] wrapKey = HybridCrypto.deriveWrapKey(combinedSecret, null, null); - return new AesGcm(wrapKey).decrypt(new AesGcm.Encrypted(encryptedDek)); + byte[] combinedSecret = null; + byte[] wrapKey = null; + try { + combinedSecret = concat(ecdhSecret, mlSecret); + wrapKey = HybridCrypto.deriveWrapKey(combinedSecret, null, null); + return new AesGcm(wrapKey).decrypt(new AesGcm.Encrypted(encryptedDek)); + } finally { + Arrays.fill(ecdhSecret, (byte) 0); + Arrays.fill(mlSecret, (byte) 0); + if (combinedSecret != null) { + Arrays.fill(combinedSecret, (byte) 0); + } + if (wrapKey != null) { + Arrays.fill(wrapKey, (byte) 0); + } + }Also applies to: 231-235
🤖 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/main/java/io/opentdf/platform/sdk/HybridNISTKeyPair.java` around lines 203 - 207, The shared-secret bytes (ecdhSecret, mlSecret, combinedSecret, wrapKey) in HybridNISTKeyPair must be zeroed after use; wrap the encryption and the corresponding decryption path (the block around deriveWrapKey/encrypt and the block referenced at 231-235) in try/finally so that in each finally you overwrite each secret byte[] (e.g., Arrays.fill(..., (byte)0) or equivalent) and null out references to avoid lingering heap data; ensure you zero ecdhSecret, mlSecret, combinedSecret, and wrapKey regardless of success or exception and do so in the same methods that call HybridCrypto.deriveWrapKey, AesGcm.encrypt, and HybridCrypto.unmarshal/unwrap to guarantee cleanup.
🤖 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/src/main/java/io/opentdf/platform/sdk/HybridCrypto.java`:
- Around line 74-100: The ASN.1 parsing can throw IllegalArgumentException and
IllegalStateException (e.g., ASN1ParsingException) which currently escape
normalization; update the code to catch and wrap those in SDKException.
Specifically, in unmarshalEnvelope around the
ASN1InputStream/ASN1Sequence.getInstance calls add handlers for
IllegalArgumentException and IllegalStateException (or a multi-catch alongside
IOException) and rethrow new SDKException(..., e). Also update
readImplicitOctetString to guard the ASN1TaggedObject.getInstance and
ASN1OctetString.getInstance calls with a try/catch for
IllegalArgumentException/IllegalStateException and throw an SDKException with a
clear message; refer to the methods unmarshalEnvelope and
readImplicitOctetString and the BouncyCastle calls ASN1Sequence.getInstance,
ASN1TaggedObject.getInstance, and ASN1OctetString.getInstance when locating
changes.
In `@sdk/src/main/java/io/opentdf/platform/sdk/KeyType.java`:
- Around line 18-20: Add the three new hybrid enum constants to both factory
switch statements so fromAlgorithm(...) and fromPublicKeyAlgorithm(...) return
the correct KeyType for HybridXWingKey, HybridSecp256r1MLKEM768Key, and
HybridSecp384r1MLKEM1024Key; locate the switch in the KeyType enum's
fromAlgorithm(...) method and the switch in fromPublicKeyAlgorithm(...) and add
matching case entries that map the corresponding protobuf algorithm enum values
to these KeyType constants to avoid IllegalArgumentException when those protobuf
values are encountered.
---
Nitpick comments:
In `@sdk/src/main/java/io/opentdf/platform/sdk/HybridNISTKeyPair.java`:
- Around line 203-207: The shared-secret bytes (ecdhSecret, mlSecret,
combinedSecret, wrapKey) in HybridNISTKeyPair must be zeroed after use; wrap the
encryption and the corresponding decryption path (the block around
deriveWrapKey/encrypt and the block referenced at 231-235) in try/finally so
that in each finally you overwrite each secret byte[] (e.g., Arrays.fill(...,
(byte)0) or equivalent) and null out references to avoid lingering heap data;
ensure you zero ecdhSecret, mlSecret, combinedSecret, and wrapKey regardless of
success or exception and do so in the same methods that call
HybridCrypto.deriveWrapKey, AesGcm.encrypt, and HybridCrypto.unmarshal/unwrap to
guarantee cleanup.
In `@sdk/src/main/java/io/opentdf/platform/sdk/XWingKeyPair.java`:
- Around line 12-13: In XWingKeyPair.java ensure derived secrets are explicitly
cleared after use: locate the methods that perform wrapping/unwrapping (e.g.,
the wrap/unwrap primitives around sharedSecret and wrapKey) and add try { ... }
finally { Arrays.fill(sharedSecret, (byte)0); Arrays.fill(wrapKey, (byte)0);
sharedSecret = null; wrapKey = null; } (or equivalent) so both success and
exception paths wipe and null out the byte[] secrets; apply the same pattern to
the other occurrences noted around the blocks at the other wrap/unwrap usages
(lines referenced 67-73 and 87-90).
🪄 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: d2649e03-5d69-4cad-bbe5-02e23e6fc142
📒 Files selected for processing (7)
sdk/src/main/java/io/opentdf/platform/sdk/HybridCrypto.javasdk/src/main/java/io/opentdf/platform/sdk/HybridNISTKeyPair.javasdk/src/main/java/io/opentdf/platform/sdk/KeyType.javasdk/src/main/java/io/opentdf/platform/sdk/TDF.javasdk/src/main/java/io/opentdf/platform/sdk/XWingKeyPair.javasdk/src/test/java/io/opentdf/platform/sdk/HybridCryptoTest.javasdk/src/test/java/io/opentdf/platform/sdk/TDFHybridTest.java
| static byte[][] unmarshalEnvelope(byte[] der) { | ||
| try (ASN1InputStream in = new ASN1InputStream(new ByteArrayInputStream(der))) { | ||
| ASN1Primitive prim = in.readObject(); | ||
| if (prim == null) { | ||
| throw new SDKException("hybrid wrapped key envelope is empty"); | ||
| } | ||
| if (in.readObject() != null) { | ||
| throw new SDKException("hybrid wrapped key envelope has trailing bytes"); | ||
| } | ||
| ASN1Sequence seq = ASN1Sequence.getInstance(prim); | ||
| if (seq.size() != 2) { | ||
| throw new SDKException("hybrid wrapped key envelope must have 2 elements, got " + seq.size()); | ||
| } | ||
| byte[] hybridCt = readImplicitOctetString(seq.getObjectAt(0), 0); | ||
| byte[] encDek = readImplicitOctetString(seq.getObjectAt(1), 1); | ||
| return new byte[][] { hybridCt, encDek }; | ||
| } catch (IOException e) { | ||
| throw new SDKException("failed to decode hybrid wrapped key envelope", e); | ||
| } | ||
| } | ||
|
|
||
| private static byte[] readImplicitOctetString(org.bouncycastle.asn1.ASN1Encodable enc, int expectedTag) { | ||
| ASN1TaggedObject tagged = ASN1TaggedObject.getInstance(enc); | ||
| if (tagged.getTagNo() != expectedTag) { | ||
| throw new SDKException("expected context tag " + expectedTag + " but got " + tagged.getTagNo()); | ||
| } | ||
| return org.bouncycastle.asn1.ASN1OctetString.getInstance(tagged, false).getOctets(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Bouncy Castle ASN.1, what exceptions can ASN1Sequence.getInstance(Object)andASN1OctetString.getInstance(ASN1TaggedObject, boolean) throw when the ASN.1 input is the wrong type?
💡 Result:
In the Bouncy Castle ASN.1 library, both ASN1Sequence.getInstance(Object) and ASN1OctetString.getInstance(ASN1TaggedObject, boolean) are designed to throw a java.lang.IllegalArgumentException when the provided input cannot be converted to the expected type [1][2]. When these methods are called with an object that does not match the required ASN.1 structure (for example, passing a non-sequence object to ASN1Sequence.getInstance), the library explicitly throws an IllegalArgumentException [1][2]. This exception often includes a descriptive message, such as "unknown object in getInstance" or similar, indicating that the input type is incompatible with the requested ASN.1 class [3][4]. While IllegalArgumentException is the standard exception for type mismatches in these factory methods, other ASN.1-related exceptions may occur during broader parsing operations: 1. ASN1ParsingException: This is a runtime exception (extending IllegalStateException) thrown when the ASN.1 data is correctly encoded but contains unexpected data while building an object [5][6]. 2. ASN1Exception: This is an IOException thrown when the library encounters corrupted or unexpected data within an ASN.1 stream [7][6]. In summary, for the specific getInstance methods mentioned, you should expect and handle java.lang.IllegalArgumentException when the input type is incorrect [1][2].
Citations:
- 1: http://docs.glngn.com/latest/api/org.bouncycastle.bcprov-jdk15on/org/bouncycastle/asn1/ASN1Sequence.html
- 2: https://javadoc.io/static/org.bouncycastle/bcprov-ext-jdk15on/1.49/org/bouncycastle/asn1/ASN1OctetString.html
- 3: IllegalArgumentException: unknown object in getInstance: org.bouncycastle.asn1.DERTaggedObject bcgit/bc-java#213
- 4: https://www.javatips.net/api/org.bouncycastle.asn1.asn1taggedobject
- 5: https://downloads.bouncycastle.org/java/docs/bcprov-jdk14-javadoc/org/bouncycastle/asn1/ASN1ParsingException.html
- 6: https://downloads.bouncycastle.org/java/docs/bcprov-jdk14-javadoc/org/bouncycastle/asn1/package-summary.html
- 7: https://downloads.bouncycastle.org/lts-java/docs/bccore-lts8on-2.73.5-javadoc/org/bouncycastle/asn1/ASN1Exception.html
Normalize all ASN.1 shape failures to SDKException.
The catch block only handles IOException, but Bouncy Castle's ASN.1 getInstance(...) methods throw IllegalArgumentException when the input type is incorrect, and ASN1ParsingException (extends IllegalStateException) when the ASN.1 is validly encoded but structurally unexpected. These exceptions will propagate uncaught from the parser, bypassing SDK exception normalization on untrusted envelope bytes.
Suggested fix
- } catch (IOException e) {
+ } catch (IOException | IllegalArgumentException | IllegalStateException e) {
throw new SDKException("failed to decode hybrid wrapped key envelope", e);
}🤖 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/main/java/io/opentdf/platform/sdk/HybridCrypto.java` around lines 74
- 100, The ASN.1 parsing can throw IllegalArgumentException and
IllegalStateException (e.g., ASN1ParsingException) which currently escape
normalization; update the code to catch and wrap those in SDKException.
Specifically, in unmarshalEnvelope around the
ASN1InputStream/ASN1Sequence.getInstance calls add handlers for
IllegalArgumentException and IllegalStateException (or a multi-catch alongside
IOException) and rethrow new SDKException(..., e). Also update
readImplicitOctetString to guard the ASN1TaggedObject.getInstance and
ASN1OctetString.getInstance calls with a try/catch for
IllegalArgumentException/IllegalStateException and throw an SDKException with a
clear message; refer to the methods unmarshalEnvelope and
readImplicitOctetString and the BouncyCastle calls ASN1Sequence.getInstance,
ASN1TaggedObject.getInstance, and ASN1OctetString.getInstance when locating
changes.
| HybridXWingKey("hpqt:xwing"), | ||
| HybridSecp256r1MLKEM768Key("hpqt:secp256r1-mlkem768"), | ||
| HybridSecp384r1MLKEM1024Key("hpqt:secp384r1-mlkem1024"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Hybrid algorithm enum values =="
rg -n --type java 'ALGORITHM_.*(XWING|MLKEM)|KAS_PUBLIC_KEY_ALG_ENUM_.*(XWING|MLKEM)'
echo
echo "== Call sites of KeyType.fromAlgorithm / fromPublicKeyAlgorithm =="
rg -n --type java '\bKeyType\.(fromAlgorithm|fromPublicKeyAlgorithm)\s*\('Repository: opentdf/java-sdk
Length of output: 95
🏁 Script executed:
cat -n sdk/src/main/java/io/opentdf/platform/sdk/KeyType.javaRepository: opentdf/java-sdk
Length of output: 4297
🏁 Script executed:
fd -e proto | head -20Repository: opentdf/java-sdk
Length of output: 42
🏁 Script executed:
rg -l "enum Algorithm|class Algorithm" --type java | head -10Repository: opentdf/java-sdk
Length of output: 42
🏁 Script executed:
rg "import io.opentdf.platform.policy.Algorithm|import io.opentdf.platform.policy.KasPublicKeyAlgEnum" --type java -l | head -10Repository: opentdf/java-sdk
Length of output: 410
🏁 Script executed:
rg "fromAlgorithm|fromPublicKeyAlgorithm" --type java -B 2 -A 2Repository: opentdf/java-sdk
Length of output: 5904
🏁 Script executed:
find . -name "*.proto" | xargs cat | grep -A 30 "enum Algorithm\|enum KasPublicKeyAlgEnum" | head -80Repository: opentdf/java-sdk
Length of output: 42
🏁 Script executed:
rg "enum ALGORITHM_|enum KAS_PUBLIC_KEY_ALG_ENUM_" --type java -A 5 | head -50Repository: opentdf/java-sdk
Length of output: 42
🏁 Script executed:
rg "ALGORITHM_|KAS_PUBLIC_KEY_ALG_ENUM_" --type java | grep -E "(case|=)" | head -30Repository: opentdf/java-sdk
Length of output: 1511
🏁 Script executed:
find . -path ./build -prune -o -type f -name "*.java" -exec grep -l "ALGORITHM_.*XWING\|ALGORITHM_.*MLKEM\|KAS_PUBLIC_KEY_ALG_ENUM.*XWING\|KAS_PUBLIC_KEY_ALG_ENUM.*MLKEM" {} \;Repository: opentdf/java-sdk
Length of output: 42
Add hybrid key type support to fromAlgorithm() and fromPublicKeyAlgorithm() factory methods to maintain API completeness.
The three new hybrid enum constants (HybridXWingKey, HybridSecp256r1MLKEM768Key, HybridSecp384r1MLKEM1024Key) are missing from both factory method switches. These methods are actively called in Config.java and Autoconfigure.java to map from protobuf enums to KeyType. When the corresponding protobuf enum values are added, any code path using those typed conversions will fail with IllegalArgumentException. Add matching cases to both switches to ensure complete coverage per the coding guideline to "keep public SDK APIs stable and additive where possible."
🤖 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/main/java/io/opentdf/platform/sdk/KeyType.java` around lines 18 - 20,
Add the three new hybrid enum constants to both factory switch statements so
fromAlgorithm(...) and fromPublicKeyAlgorithm(...) return the correct KeyType
for HybridXWingKey, HybridSecp256r1MLKEM768Key, and HybridSecp384r1MLKEM1024Key;
locate the switch in the KeyType enum's fromAlgorithm(...) method and the switch
in fromPublicKeyAlgorithm(...) and add matching case entries that map the
corresponding protobuf algorithm enum values to these KeyType constants to avoid
IllegalArgumentException when those protobuf values are encountered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/README.md`:
- Line 79: The fenced code block missing a language tag should be updated to use
a language identifier (e.g., add "text") so markdownlint MD040 is satisfied;
change the opening fence from ``` to ```text for the block that contains the
"[OK] hpqt:..." lines and ensure the closing fence remains ``` so rendering
and linting are correct.
In `@scripts/test-hybrid-pqc.sh`:
- Around line 38-50: The option parsing loop currently reads values for flags
like --algorithms, --platform-endpoint, --kas-url, --attr, --client-id, and
--client-secret without verifying a following argument, which under set -u can
cause a shell error; update the case branches that assign to ALGORITHMS,
PLATFORM_ENDPOINT, KAS_URL, DATA_ATTR, CLIENT_ID, and CLIENT_SECRET to first
validate that "$2" exists and is not another option (e.g., [[ -n "${2-}" &&
"${2:0:1}" != "-" ]]) and if the check fails print the usage/help and exit with
the same misuse exit code (2), leaving the boolean flags (--skip-build,
--skip-kas-check) unchanged.
- Around line 197-198: The envelope-check fails on macOS/BSD because the script
calls `base64 -d` and `xxd`; update the decoding/byte-extraction to be portable
by trying `base64 -d` and falling back to `base64 -D` (or vice versa) when
decoding the `wrapped` variable, and replace the `xxd -p -l 1` usage with an
`od` invocation (e.g. `od -An -tx1 -N1`) to reliably produce the first byte in
hex; modify the assignment around `first_byte=$(... )` and any place referencing
`xxd`/`base64` so it uses this portable approach while preserving the existing
`wrapped` variable and the subsequent `if [[ "$first_byte" != "30" ]]` check.
🪄 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: 9a4a0dd5-861a-469c-9eb2-0033e49a9b50
📒 Files selected for processing (2)
scripts/README.mdscripts/test-hybrid-pqc.sh
|
|
||
| ### Expected output | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced code block.
This avoids markdownlint MD040 and improves rendering consistency.
Proposed fix
-```
+```text
[OK] hpqt:xwing: KAS returns hybrid PEM (-----BEGIN XWING PUBLIC KEY-----)
[OK] hpqt:secp256r1-mlkem768: KAS returns hybrid PEM (-----BEGIN SECP256R1 MLKEM768 PUBLIC KEY-----)
[OK] hpqt:secp384r1-mlkem1024: KAS returns hybrid PEM (-----BEGIN SECP384R1 MLKEM1024 PUBLIC KEY-----)
...
[OK] HybridXWingKey: manifest OK (hybrid-wrapped, ASN.1 envelope, no ephemeralPublicKey)
[OK] HybridXWingKey: round-trip OK
...
All 3 hybrid algorithm(s) passed round-trip.</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 79-79: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@scripts/README.md` at line 79, The fenced code block missing a language tag
should be updated to use a language identifier (e.g., add "text") so
markdownlint MD040 is satisfied; change the opening fence from ``` to ```text
for the block that contains the "[OK] hpqt:..." lines and ensure the closing
fence remains ``` so rendering and linting are correct.
| while [[ $# -gt 0 ]]; do | ||
| case "$1" in | ||
| --skip-build) SKIP_BUILD=1; shift ;; | ||
| --skip-kas-check) SKIP_KAS_CHECK=1; shift ;; | ||
| --algorithms) IFS=, read -r -a ALGORITHMS <<< "$2"; shift 2 ;; | ||
| --platform-endpoint) PLATFORM_ENDPOINT="$2"; shift 2 ;; | ||
| --kas-url) KAS_URL="$2"; shift 2 ;; | ||
| --attr) DATA_ATTR="$2"; shift 2 ;; | ||
| --client-id) CLIENT_ID="$2"; shift 2 ;; | ||
| --client-secret) CLIENT_SECRET="$2"; shift 2 ;; | ||
| -h|--help) sed -n '2,/^$/p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;; | ||
| *) echo "unknown option: $1" >&2; exit 2 ;; | ||
| esac |
There was a problem hiding this comment.
Guard flags that require a value.
With set -u, missing values for options like --algorithms or --kas-url can crash with a shell error instead of returning the documented misuse exit path.
Proposed fix
+require_opt_value() {
+ local opt="$1"
+ local val="${2-}"
+ if [[ -z "$val" || "$val" == --* ]]; then
+ echo "missing value for $opt" >&2
+ exit 2
+ fi
+}
+
while [[ $# -gt 0 ]]; do
case "$1" in
--skip-build) SKIP_BUILD=1; shift ;;
--skip-kas-check) SKIP_KAS_CHECK=1; shift ;;
- --algorithms) IFS=, read -r -a ALGORITHMS <<< "$2"; shift 2 ;;
- --platform-endpoint) PLATFORM_ENDPOINT="$2"; shift 2 ;;
- --kas-url) KAS_URL="$2"; shift 2 ;;
- --attr) DATA_ATTR="$2"; shift 2 ;;
- --client-id) CLIENT_ID="$2"; shift 2 ;;
- --client-secret) CLIENT_SECRET="$2"; shift 2 ;;
+ --algorithms) require_opt_value "$1" "${2-}"; IFS=, read -r -a ALGORITHMS <<< "$2"; shift 2 ;;
+ --platform-endpoint) require_opt_value "$1" "${2-}"; PLATFORM_ENDPOINT="$2"; shift 2 ;;
+ --kas-url) require_opt_value "$1" "${2-}"; KAS_URL="$2"; shift 2 ;;
+ --attr) require_opt_value "$1" "${2-}"; DATA_ATTR="$2"; shift 2 ;;
+ --client-id) require_opt_value "$1" "${2-}"; CLIENT_ID="$2"; shift 2 ;;
+ --client-secret) require_opt_value "$1" "${2-}"; CLIENT_SECRET="$2"; shift 2 ;;
-h|--help) sed -n '2,/^$/p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;;
*) echo "unknown option: $1" >&2; exit 2 ;;
esac
done📝 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.
| while [[ $# -gt 0 ]]; do | |
| case "$1" in | |
| --skip-build) SKIP_BUILD=1; shift ;; | |
| --skip-kas-check) SKIP_KAS_CHECK=1; shift ;; | |
| --algorithms) IFS=, read -r -a ALGORITHMS <<< "$2"; shift 2 ;; | |
| --platform-endpoint) PLATFORM_ENDPOINT="$2"; shift 2 ;; | |
| --kas-url) KAS_URL="$2"; shift 2 ;; | |
| --attr) DATA_ATTR="$2"; shift 2 ;; | |
| --client-id) CLIENT_ID="$2"; shift 2 ;; | |
| --client-secret) CLIENT_SECRET="$2"; shift 2 ;; | |
| -h|--help) sed -n '2,/^$/p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;; | |
| *) echo "unknown option: $1" >&2; exit 2 ;; | |
| esac | |
| require_opt_value() { | |
| local opt="$1" | |
| local val="${2-}" | |
| if [[ -z "$val" || "$val" == --* ]]; then | |
| echo "missing value for $opt" >&2 | |
| exit 2 | |
| fi | |
| } | |
| while [[ $# -gt 0 ]]; do | |
| case "$1" in | |
| --skip-build) SKIP_BUILD=1; shift ;; | |
| --skip-kas-check) SKIP_KAS_CHECK=1; shift ;; | |
| --algorithms) require_opt_value "$1" "${2-}"; IFS=, read -r -a ALGORITHMS <<< "$2"; shift 2 ;; | |
| --platform-endpoint) require_opt_value "$1" "${2-}"; PLATFORM_ENDPOINT="$2"; shift 2 ;; | |
| --kas-url) require_opt_value "$1" "${2-}"; KAS_URL="$2"; shift 2 ;; | |
| --attr) require_opt_value "$1" "${2-}"; DATA_ATTR="$2"; shift 2 ;; | |
| --client-id) require_opt_value "$1" "${2-}"; CLIENT_ID="$2"; shift 2 ;; | |
| --client-secret) require_opt_value "$1" "${2-}"; CLIENT_SECRET="$2"; shift 2 ;; | |
| -h|--help) sed -n '2,/^$/p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;; | |
| *) echo "unknown option: $1" >&2; exit 2 ;; | |
| esac | |
| done |
🤖 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 `@scripts/test-hybrid-pqc.sh` around lines 38 - 50, The option parsing loop
currently reads values for flags like --algorithms, --platform-endpoint,
--kas-url, --attr, --client-id, and --client-secret without verifying a
following argument, which under set -u can cause a shell error; update the case
branches that assign to ALGORITHMS, PLATFORM_ENDPOINT, KAS_URL, DATA_ATTR,
CLIENT_ID, and CLIENT_SECRET to first validate that "$2" exists and is not
another option (e.g., [[ -n "${2-}" && "${2:0:1}" != "-" ]]) and if the check
fails print the usage/help and exit with the same misuse exit code (2), leaving
the boolean flags (--skip-build, --skip-kas-check) unchanged.
| first_byte=$(base64 -d <<<"$wrapped" 2>/dev/null | xxd -p -l 1 || true) | ||
| if [[ "$first_byte" != "30" ]]; then |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'MA==\n' | base64 -d >/dev/null 2>&1 && echo "base64 -d: supported" || echo "base64 -d: unsupported"
printf 'MA==\n' | base64 -D >/dev/null 2>&1 && echo "base64 -D: supported" || echo "base64 -D: unsupported"Repository: opentdf/java-sdk
Length of output: 104
🏁 Script executed:
fd -t f "test-hybrid-pqc.sh"Repository: opentdf/java-sdk
Length of output: 87
🏁 Script executed:
git ls-files | grep -i "test-hybrid-pqc"Repository: opentdf/java-sdk
Length of output: 87
🏁 Script executed:
cat -n scripts/test-hybrid-pqc.sh | sed -n '190,205p'Repository: opentdf/java-sdk
Length of output: 916
🏁 Script executed:
head -30 scripts/test-hybrid-pqc.shRepository: opentdf/java-sdk
Length of output: 1340
🌐 Web query:
base64 -d vs -D GNU BSD macOS portability differences
💡 Result:
On GNU/Linux (coreutils), base64 -d and base64 --decode are the decode flags; there is no separate -D option in the GNU coreutils interface. [1][2] On macOS/BSD, base64 -D (or --decode) is the decode flag; base64’s BSD/GNU option set differs (e.g., BSD uses -D for decode in its man page). [3][4] So the portability difference is simply that you can’t use the same single-letter flag across GNU vs BSD/macOS: use --decode/-d on GNU, and --decode/-D on macOS/BSD. [1][2][3][4] Practical portable approach: prefer the long option --decode when available in both implementations (GNU coreutils documents base64 --decode; BSD/macOS man page documents --decode too). [1][3] If you must write a strictly portable shell snippet that works with both and long options ever differ, detect which variant is present (GNU coreutils vs BSD/macOS), but the evidence here supports --decode as the shared, portable spelling. [1][3]
Citations:
- 1: https://www.gnu.org/software/coreutils/manual/html_node/base64-invocation.html
- 2: https://www.gnu.org/software/coreutils/base64
- 3: https://www.unix.com/man-page/osx/1/base64?os=osx§ion=1&query=base64
- 4: https://www.unix.com/man_page/osx/1/base64/
🌐 Web query:
xxd vs od hex dump portability macOS BSD Linux
💡 Result:
xxd vs od for hex dumps portability across macOS (BSD/Darwin) and BSD/Linux: 1) Portability: prefer od for “works everywhere” shell scripting - od is in coreutils on Linux and is also present in base userlands on macOS and BSD, with a consistent POSIX-style interface. [1][2] - BSD/GNU variants of od/hexdump options differ; for true portability you should use od options that exist across implementations (notably -A for offset radix, -t for hex element format, -v to avoid line compression). [2][3] 2) xxd: great ergonomics, but availability/behavior can vary - xxd produces hex dumps and can also reverse a hex dump back to binary (“-r”), and supports a variety of formatting styles and options. [4] - However, xxd is not “POSIX core”: on some minimal environments it may be missing (e.g., busybox). [1] 3) Practical guidance (portable one-liners) A. Most portable “byte-per-line hex (two hex chars)” using od The following approach uses only POSIX utilities around od, so it adapts to flag differences across od implementations: [5] - Example technique (from a cross-platform discussion): use od -An -tx1 -v to emit hex bytes, then post-process to get one byte per line, since BSD od may differ in flags like -w. [5] B. If you can tolerate different output styles, od -t is the key OpenBSD’s od documents -t TYPE strings where x selects hex output and you can add size/type suffixes like 1, 2, 4, etc. [2] So, for single-byte hex you can use forms like -t x1 with -A x (hex addresses) and -v (no collapsing). [2] 4) Recommendations by use case - For portable scripts across macOS + Linux + BSD: use od with -A/-t/-v, or use the “od + standard text filters” pattern. [1][2][5] - For interactive “nice-looking” dumps and reversible workflows (hexdump -> patch/binary): use xxd when available; it’s feature-rich (including reverse/patching). [4] Bottom line: - od is the most portable choice for hex dumping across macOS/BSD/Linux because it’s broadly available and supports hex output via -t. [1][2] - xxd is best when you want its richer UX and reverse/patch capabilities, but don’t assume it exists in every Unix-like environment. [4][1]
Citations:
- 1:
hexdumpis a linux only tool dehydrated-io/dehydrated#910 - 2: https://man.openbsd.org/man1/od.1
- 3: https://manpages.org/od
- 4: https://commandlinux.com/man-page/man1/xxd/
- 5: https://unix.stackexchange.com/questions/312584/portable-one-byte-per-line-hex-dump
Fix base64 portability for macOS/BSD environments.
The script uses base64 -d, which is unsupported on macOS/BSD (they require base64 -D). This causes false envelope-check failures on those platforms. Additionally, replace xxd with od for better POSIX portability.
Proposed fix
+b64decode() {
+ if printf 'MA==\n' | base64 -d >/dev/null 2>&1; then
+ base64 -d
+ else
+ base64 -D
+ fi
+}
+
- first_byte=$(base64 -d <<<"$wrapped" 2>/dev/null | xxd -p -l 1 || true)
+ first_byte=$(b64decode <<<"$wrapped" 2>/dev/null | od -An -tx1 -N1 | tr -d ' \n' || true)🤖 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 `@scripts/test-hybrid-pqc.sh` around lines 197 - 198, The envelope-check fails
on macOS/BSD because the script calls `base64 -d` and `xxd`; update the
decoding/byte-extraction to be portable by trying `base64 -d` and falling back
to `base64 -D` (or vice versa) when decoding the `wrapped` variable, and replace
the `xxd -p -l 1` usage with an `od` invocation (e.g. `od -An -tx1 -N1`) to
reliably produce the first byte in hex; modify the assignment around
`first_byte=$(... )` and any place referencing `xxd`/`base64` so it uses this
portable approach while preserving the existing `wrapped` variable and the
subsequent `if [[ "$first_byte" != "30" ]]` check.
X-Test Failure Report |
sujankota
force-pushed
the
DSPX-3309-hybrid-pq-key-wrapping
branch
from
6e08ea4 to
d666c07
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
scripts/README.md (1)
80-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language tag to the expected-output code fence.
Line 80 still uses an untyped fenced block and will keep triggering MD040.
Proposed fix
-``` +```text [OK] hpqt:xwing: KAS returns hybrid PEM (-----BEGIN XWING PUBLIC KEY-----) [OK] hpqt:secp256r1-mlkem768: KAS returns hybrid PEM (-----BEGIN SECP256R1 MLKEM768 PUBLIC KEY-----) [OK] hpqt:secp384r1-mlkem1024: KAS returns hybrid PEM (-----BEGIN SECP384R1 MLKEM1024 PUBLIC KEY-----) ... [OK] HybridXWingKey: manifest OK (hybrid-wrapped, ASN.1 envelope, no ephemeralPublicKey) [OK] HybridXWingKey: round-trip OK ... All 3 hybrid algorithm(s) passed round-trip.</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@scripts/README.mdat line 80, The fenced expected-output block around the
test output is missing a language tag and triggers MD040; update the openingto include a language (e.g., change the opening fence totext) so the block
is typed and the linter stops flagging it—target the expected-output fenced
block shown in the diff (the multi-line sample starting with "[OK]
hpqt:xwing...") and add the language tag to its opening fence.</details> </blockquote></details> <details> <summary>scripts/test-hybrid-pqc.sh (2)</summary><blockquote> `197-197`: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Make wrappedKey decode check portable across GNU/BSD tools.** Line 197 uses GNU-specific `base64 -d` behavior and `xxd`, which can fail on macOS/BSD or minimal environments. <details> <summary>Proposed fix</summary> ```diff +b64decode() { + if printf 'MA==\n' | base64 -d >/dev/null 2>&1; then + base64 -d + else + base64 -D + fi +} + - first_byte=$(base64 -d <<<"$wrapped" 2>/dev/null | xxd -p -l 1 || true) + first_byte=$(b64decode <<<"$wrapped" 2>/dev/null | od -An -tx1 -N1 | tr -d ' \n' || true) ``` </details> ```shell #!/bin/bash set -euo pipefail # Inspect current implementation around the envelope check cat -n scripts/test-hybrid-pqc.sh | sed -n '190,205p' # Show local base64 flag support (illustrates GNU/BSD divergence risk) printf 'supports base64 -d: ' printf 'MA==\n' | base64 -d >/dev/null 2>&1 && echo yes || echo no printf 'supports base64 -D: ' printf 'MA==\n' | base64 -D >/dev/null 2>&1 && echo yes || echo no # Show whether xxd is present (currently an undeclared dependency) if command -v xxd >/dev/null 2>&1; then echo 'xxd: present' else echo 'xxd: missing' fi ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test-hybrid-pqc.sh` at line 197, The current line that sets first_byte using `base64 -d` and `xxd` is not portable; change the decode+hex extraction to use a portable fallback: try `base64 --decode` (or `base64 -D`) and if that fails try `base64 -d`, and replace `xxd -p -l 1` with a portable tool like `od -An -tx1 -N1` (or `hexdump -v -n 1 -e '1/1 "%02x"'`) to extract the first byte; update the assignment to `first_byte=$(printf '%s' "$wrapped" | base64 --decode 2>/dev/null || printf '%s' "$wrapped" | base64 -D 2>/dev/null || printf '%s' "$wrapped" | base64 -d 2>/dev/null | od -An -tx1 -N1 | tr -d ' \t\n')` (or equivalent fallback sequence) so `first_byte` production works on GNU/BSD/macOS and when `xxd` is absent in scripts/test-hybrid-pqc.sh. ``` </details> --- `42-47`: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Guard value-taking flags before reading `$2`.** Line 42–47 can crash under `set -u` when a flag is missing its value, bypassing the documented misuse path (`exit 2`). <details> <summary>Proposed fix</summary> ```diff +require_opt_value() { + local opt="$1" + local val="${2-}" + if [[ -z "$val" || "$val" == --* ]]; then + echo "missing value for $opt" >&2 + exit 2 + fi +} + while [[ $# -gt 0 ]]; do case "$1" in --skip-build) SKIP_BUILD=1; shift ;; --skip-kas-check) SKIP_KAS_CHECK=1; shift ;; - --algorithms) IFS=, read -r -a ALGORITHMS <<< "$2"; shift 2 ;; - --platform-endpoint) PLATFORM_ENDPOINT="$2"; shift 2 ;; - --kas-url) KAS_URL="$2"; shift 2 ;; - --attr) DATA_ATTR="$2"; shift 2 ;; - --client-id) CLIENT_ID="$2"; shift 2 ;; - --client-secret) CLIENT_SECRET="$2"; shift 2 ;; + --algorithms) require_opt_value "$1" "${2-}"; IFS=, read -r -a ALGORITHMS <<< "$2"; shift 2 ;; + --platform-endpoint) require_opt_value "$1" "${2-}"; PLATFORM_ENDPOINT="$2"; shift 2 ;; + --kas-url) require_opt_value "$1" "${2-}"; KAS_URL="$2"; shift 2 ;; + --attr) require_opt_value "$1" "${2-}"; DATA_ATTR="$2"; shift 2 ;; + --client-id) require_opt_value "$1" "${2-}"; CLIENT_ID="$2"; shift 2 ;; + --client-secret) require_opt_value "$1" "${2-}"; CLIENT_SECRET="$2"; shift 2 ;; -h|--help) sed -n '2,/^$/p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;; *) echo "unknown option: $1" >&2; exit 2 ;; esac done ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test-hybrid-pqc.sh` around lines 42 - 47, The parsing for value-taking flags (--algorithms, --platform-endpoint, --kas-url, --attr, --client-id, --client-secret) reads "$2" unguarded which can cause a crash under set -u when a value is missing; update the argument parsing to first validate that a next positional exists and is not another flag (e.g., check that $# -ge 2 and that "$2" does not start with --) before assigning to ALGORITHMS, PLATFORM_ENDPOINT, KAS_URL, DATA_ATTR, CLIENT_ID, or CLIENT_SECRET, and if the check fails print the misuse message and exit 2 to preserve the documented behavior. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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/src/main/java/io/opentdf/platform/sdk/HybridCrypto.java:
- Around line 129-147: readLength() currently accepts non-canonical DER length
encodings (long-form for values < 0x80 and leading-zero length bytes); fix it by
reading the long-form length bytes into a temporary byte[] (instead of
accumulating immediately), reject if bytes[0] == 0 (leading zero), compute the
decoded len from that array, then compute the minimal number of bytes required
for that len and throw an SDKException if numBytes != minimalBytes (or if len <
0x80 when long-form was used); keep other checks (numBytes==0 or >4 and
overflow) and throw SDKException on violations so unmarshalEnvelope() enforces
strict DER canonical lengths.- Around line 3-5: defaultTDFSalt() currently calls "TDF".getBytes() which
depends on the JVM default charset; change it to use an explicit charset (e.g.,
StandardCharsets.UTF_8) so the HKDF salt is deterministic across platforms;
update the method (defaultTDFSalt) to call
"TDF".getBytes(StandardCharsets.UTF_8) and add the necessary import for
java.nio.charset.StandardCharsets.In
@sdk/src/main/java/io/opentdf/platform/sdk/XWingKeyPair.java:
- Around line 3-10: The XWingKeyPair and HybridNISTKeyPair classes import
non-FIPS BouncyCastle PQC APIs (e.g., XWingKEMGenerator, XWingKEMExtractor,
XWingKeyPairGenerator and ML-KEM equivalents) which break the fips profile;
modify these classes to avoid static references to org.bouncycastle.pqc.* by
either (a) moving PQC-specific code behind a separate optional module or factory
loaded via reflection, or (b) replacing the direct imports with
provider-agnostic interfaces and runtime lookups so the code compiles under the
fips profile when bcprov-jdk18on is absent; update XWingKeyPair and
HybridNISTKeyPair to use the new indirection (factory/reflection/provider
lookup) for generators/extractors/keypair creation (e.g., XWingKEMGenerator,
XWingKEMExtractor, XWingKeyPairGenerator and MLKEM equivalents) so the fips
build no longer requires non-fips BC classes at compile time.
Duplicate comments:
In@scripts/README.md:
- Line 80: The fenced expected-output block around the test output is missing a
language tag and triggers MD040; update the openingto include a language (e.g., change the opening fence totext) so the block is typed and the linter
stops flagging it—target the expected-output fenced block shown in the diff (the
multi-line sample starting with "[OK] hpqt:xwing...") and add the language tag
to its opening fence.In
@scripts/test-hybrid-pqc.sh:
- Line 197: The current line that sets first_byte using
base64 -dandxxdis
not portable; change the decode+hex extraction to use a portable fallback: try
base64 --decode(orbase64 -D) and if that fails trybase64 -d, and
replacexxd -p -l 1with a portable tool likeod -An -tx1 -N1(orhexdump -v -n 1 -e '1/1 "%02x"') to extract the first byte; update the assignment to
first_byte=$(printf '%s' "$wrapped" | base64 --decode 2>/dev/null || printf '%s' "$wrapped" | base64 -D 2>/dev/null || printf '%s' "$wrapped" | base64 -d 2>/dev/null | od -An -tx1 -N1 | tr -d ' \t\n')(or equivalent fallback
sequence) sofirst_byteproduction works on GNU/BSD/macOS and whenxxdis
absent in scripts/test-hybrid-pqc.sh.- Around line 42-47: The parsing for value-taking flags (--algorithms,
--platform-endpoint, --kas-url, --attr, --client-id, --client-secret) reads "$2"
unguarded which can cause a crash under set -u when a value is missing; update
the argument parsing to first validate that a next positional exists and is not
another flag (e.g., check that $# -ge 2 and that "$2" does not start with --)
before assigning to ALGORITHMS, PLATFORM_ENDPOINT, KAS_URL, DATA_ATTR,
CLIENT_ID, or CLIENT_SECRET, and if the check fails print the misuse message and
exit 2 to preserve the documented behavior.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `709afa34-584d-46f7-bfb4-4b12dcd4c629` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 6e08ea4e829176fd15bccd61ee5e48be3e859ec9 and d666c0788da28cec69d26662e78c68e67c1d1443. </details> <details> <summary>📒 Files selected for processing (10)</summary> * `scripts/README.md` * `scripts/test-hybrid-pqc.sh` * `sdk/pom.xml` * `sdk/src/main/java/io/opentdf/platform/sdk/HybridCrypto.java` * `sdk/src/main/java/io/opentdf/platform/sdk/HybridNISTKeyPair.java` * `sdk/src/main/java/io/opentdf/platform/sdk/KeyType.java` * `sdk/src/main/java/io/opentdf/platform/sdk/TDF.java` * `sdk/src/main/java/io/opentdf/platform/sdk/XWingKeyPair.java` * `sdk/src/test/java/io/opentdf/platform/sdk/HybridCryptoTest.java` * `sdk/src/test/java/io/opentdf/platform/sdk/TDFHybridTest.java` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| import java.security.MessageDigest; | ||
| import java.security.NoSuchAlgorithmException; | ||
| import java.util.Base64; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="sdk/src/main/java/io/opentdf/platform/sdk/HybridCrypto.java"
echo "== File existence =="
ls -l "$FILE" || (echo "Missing $FILE" && exit 1)
echo
echo "== Imports (lines 1-60) =="
nl -ba "$FILE" | sed -n '1,60p'
echo
echo "== HKDF salt/byte conversion usages (lines 150-240) =="
nl -ba "$FILE" | sed -n '150,240p' | sed -n '/TDF/Ip'
echo
echo "== Full context around first TDF getBytes =="
# show around any 'TDF'.getBytes occurrences
for ln in $(rg -n --fixed-string '"TDF"' "$FILE" | cut -d: -f1 | head -n 20); do
echo "--- around line $ln ---"
start=$((ln-15)); end=$((ln+15))
nl -ba "$FILE" | sed -n "${start},${end}p"
done
echo
echo "== Search for getBytes() without charset in this file =="
rg -n --regexp 'getBytes\s*\(\s*\)' "$FILE" || true
echo
echo "== Search for TDF.getBytes specifically =="
rg -n --fixed-string '"TDF".getBytes' "$FILE" || trueRepository: opentdf/java-sdk
Length of output: 280
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="sdk/src/main/java/io/opentdf/platform/sdk/HybridCrypto.java"
echo "== Imports (top of file) =="
awk 'NR<=80{printf "%4d:%s\n", NR, $0}' "$FILE" | sed -n '1,80p'
echo
echo "== Lines 1-40 (imports area) =="
awk 'NR<=40{printf "%4d:%s\n", NR, $0}' "$FILE" | sed -n '1,40p'
echo
echo "== Search: \"TDF\".getBytes in file =="
rg -n --fixed-string '"TDF".getBytes' "$FILE" || true
echo
echo "== Search: getBytes() without charset in file =="
rg -n --regexp 'getBytes\s*\(\s*\)' "$FILE" || true
echo
echo "== Context around any \"TDF\" occurrences =="
# show up to 10 occurrences
mapfile -t lines < <(rg -n --fixed-string '"TDF"' "$FILE" | cut -d: -f1 | head -n 10)
for ln in "${lines[@]}"; do
start=$((ln-15)); end=$((ln+15));
echo "--- around line $ln ---"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e{printf "%4d:%s\n", NR, $0}' "$FILE"
done
echo
echo "== Context around ~181-185 =="
awk 'NR>=160 && NR<=210{printf "%4d:%s\n", NR, $0}' "$FILE" | sed -n '1,200p'Repository: opentdf/java-sdk
Length of output: 6551
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="sdk/src/main/java/io/opentdf/platform/sdk/HybridCrypto.java"
echo "== Lines 160-210 =="
awk 'NR>=160 && NR<=210{printf "%4d:%s\n", NR, $0}' "$FILE"
echo
echo "== Search: literal \"TDF\" =="
rg -nF '"TDF"' "$FILE" || true
echo
echo '== Search: literal "TDF".getBytes ='
rg -nF '"TDF".getBytes' "$FILE" || true
echo
echo "== Search: any getBytes() without charset in this file =="
rg -n --regexp 'getBytes\s*\(\s*\)' "$FILE" || trueRepository: opentdf/java-sdk
Length of output: 2859
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="sdk/src/main/java/io/opentdf/platform/sdk/HybridCrypto.java"
rg -n "StandardCharsets" "$FILE" || true
rg -n "import java\.nio\.charset" "$FILE" || trueRepository: opentdf/java-sdk
Length of output: 42
Pin HKDF salt input ("TDF") to an explicit charset
defaultTDFSalt() uses d.update("TDF".getBytes()), which depends on the JVM default charset; this can change the derived wrap key and break interoperability.
💡 Suggested fix
+import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.util.Base64;
@@
- d.update("TDF".getBytes());
+ d.update("TDF".getBytes(StandardCharsets.US_ASCII));
return d.digest();📝 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.
| import java.security.MessageDigest; | |
| import java.security.NoSuchAlgorithmException; | |
| import java.util.Base64; | |
| import java.nio.charset.StandardCharsets; | |
| import java.security.MessageDigest; | |
| import java.security.NoSuchAlgorithmException; | |
| import java.util.Base64; |
| import java.security.MessageDigest; | |
| import java.security.NoSuchAlgorithmException; | |
| import java.util.Base64; | |
| d.update("TDF".getBytes(StandardCharsets.US_ASCII)); | |
| return d.digest(); |
🤖 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/main/java/io/opentdf/platform/sdk/HybridCrypto.java` around lines 3 -
5, defaultTDFSalt() currently calls "TDF".getBytes() which depends on the JVM
default charset; change it to use an explicit charset (e.g.,
StandardCharsets.UTF_8) so the HKDF salt is deterministic across platforms;
update the method (defaultTDFSalt) to call
"TDF".getBytes(StandardCharsets.UTF_8) and add the necessary import for
java.nio.charset.StandardCharsets.
| private static int readLength(Cursor c) { | ||
| int first = c.readByte(); | ||
| if ((first & 0x80) == 0) { | ||
| return first; | ||
| } | ||
| int numBytes = first & 0x7F; | ||
| if (numBytes == 0 || numBytes > 4) { | ||
| // indefinite-length (numBytes == 0) is BER-only; DER rejects it. | ||
| // > 4 would overflow a positive 32-bit int and is implausible for our envelope. | ||
| throw new SDKException("invalid ASN.1 length encoding: numBytes=" + numBytes); | ||
| } | ||
| int len = 0; | ||
| for (int i = 0; i < numBytes; i++) { | ||
| len = (len << 8) | c.readByte(); | ||
| } | ||
| if (len < 0) { | ||
| throw new SDKException("ASN.1 length overflowed signed int"); | ||
| } | ||
| return len; |
There was a problem hiding this comment.
Reject non-canonical length encodings in the DER parser.
readLength() still accepts long-form encodings for values below 0x80 and leading-zero length bytes, so unmarshalEnvelope() is strict about truncation but not strict DER. That leaves multiple encodings for the same envelope on untrusted input.
💡 Suggested fix
private static int readLength(Cursor c) {
int first = c.readByte();
if ((first & 0x80) == 0) {
return first;
}
int numBytes = first & 0x7F;
if (numBytes == 0 || numBytes > 4) {
// indefinite-length (numBytes == 0) is BER-only; DER rejects it.
// > 4 would overflow a positive 32-bit int and is implausible for our envelope.
throw new SDKException("invalid ASN.1 length encoding: numBytes=" + numBytes);
}
int len = 0;
+ int firstLenByte = -1;
for (int i = 0; i < numBytes; i++) {
- len = (len << 8) | c.readByte();
+ int b = c.readByte();
+ if (i == 0) {
+ firstLenByte = b;
+ }
+ len = (len << 8) | b;
+ }
+ if (firstLenByte == 0 || len < 0x80) {
+ throw new SDKException("non-canonical ASN.1 length encoding");
}
if (len < 0) {
throw new SDKException("ASN.1 length overflowed signed int");
}
return len;🤖 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/main/java/io/opentdf/platform/sdk/HybridCrypto.java` around lines 129
- 147, readLength() currently accepts non-canonical DER length encodings
(long-form for values < 0x80 and leading-zero length bytes); fix it by reading
the long-form length bytes into a temporary byte[] (instead of accumulating
immediately), reject if bytes[0] == 0 (leading zero), compute the decoded len
from that array, then compute the minimal number of bytes required for that len
and throw an SDKException if numBytes != minimalBytes (or if len < 0x80 when
long-form was used); keep other checks (numBytes==0 or >4 and overflow) and
throw SDKException on violations so unmarshalEnvelope() enforces strict DER
canonical lengths.
| import org.bouncycastle.crypto.AsymmetricCipherKeyPair; | ||
| import org.bouncycastle.crypto.SecretWithEncapsulation; | ||
| import org.bouncycastle.pqc.crypto.xwing.XWingKEMExtractor; | ||
| import org.bouncycastle.pqc.crypto.xwing.XWingKEMGenerator; | ||
| import org.bouncycastle.pqc.crypto.xwing.XWingKeyGenerationParameters; | ||
| import org.bouncycastle.pqc.crypto.xwing.XWingKeyPairGenerator; | ||
| import org.bouncycastle.pqc.crypto.xwing.XWingPrivateKeyParameters; | ||
| import org.bouncycastle.pqc.crypto.xwing.XWingPublicKeyParameters; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== bcprov declarations in sdk/pom.xml profiles =="
sed -n '/<profiles>/,/<\/profiles>/p' sdk/pom.xml | rg -n '(<id>|artifactId>|groupId>|bcprov|bcfips|bctls)'
echo
echo "== main-source Bouncy Castle references =="
rg -n --type java 'org\.bouncycastle\.(pqc|crypto)\.' sdk/src/main/java/io/opentdf/platform/sdkRepository: opentdf/java-sdk
Length of output: 3630
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sdk/pom.xml profiles: show dependency blocks in non-fips and fips =="
# Print a focused range around profiles for readability
nl -ba sdk/pom.xml | sed -n '1,220p'
echo
echo "== Search sdk/pom.xml for hybrid/xwing filtering/excludes =="
rg -n --hidden --no-ignore-vcs -S "(XWing|HybridNIST|xwing|mlkem|fips|non-fips|maven-(compiler|resources)-plugin|<excludes>|<includes>|failOnError|skipTests)" sdk/pom.xmlRepository: opentdf/java-sdk
Length of output: 193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sdk/pom.xml (first 260 lines with line numbers via awk) =="
awk '{printf "%6d:%s\n", NR, $0}' sdk/pom.xml | sed -n '1,260p'
echo
echo "== Search sdk/pom.xml for relevant profile/build filtering (full match) =="
rg -n --hidden --no-ignore-vcs -S '(non-fips|fips|XWing|HybridNIST|xwing|mlkem|maven-compiler-plugin|maven-resources-plugin|<excludes>|<includes>|failOnError|skip|source|testSource|compileSource|maven-dependency-plugin|dependencyManagement|bcprov-jdk18on|bc-fips|bctls-fips|bcpkix-jdk18on|bcpkix-fips)' sdk/pom.xmlRepository: opentdf/java-sdk
Length of output: 14614
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Show sdk/pom.xml around profile definitions (lines ~430-560) =="
awk '{printf "%6d:%s\n", NR, $0}' sdk/pom.xml | sed -n '430,560p'
echo
echo "== Find parent pom (sdk-pom) and inspect for fips/non-fips source filtering =="
# find any sdk-pom.xml in repo
fd -t f "sdk-pom.xml" . | head -n 20
# if found, show around any profile/build filtering keywords
PARENT="$(fd -t f "sdk-pom.xml" . | head -n 1 || true)"
if [ -n "${PARENT}" ]; then
echo
echo "== Using parent file: $PARENT =="
rg -n --hidden --no-ignore-vcs -S '(non-fips|fips|profile|maven-compiler-plugin|<excludes>|<includes>|xwing|XWing|HybridNIST|sourceDirectory|testSourceDirectory)' "$PARENT" || true
echo
awk '{printf "%6d:%s\n", NR, $0}' "$PARENT" | sed -n '1,220p'
fiRepository: opentdf/java-sdk
Length of output: 7078
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate sdk-pom.xml in repo =="
git ls-files | rg -n "sdk-pom\.xml$" || true
echo
echo "== If found, show its top section with profiles/build =="
PARENT="$(git ls-files | rg "sdk-pom\.xml$" | head -n 1 || true)"
if [ -n "${PARENT}" ]; then
echo "Parent: $PARENT"
awk '{printf "%6d:%s\n", NR, $0}' "$PARENT" | sed -n '1,260p'
echo
rg -n --hidden --no-ignore-vcs -S '(<profiles>|<id>fips|<id>non-fips|<excludes>|<includes>|maven-compiler-plugin|sourceDirectory|testSource|maven-resources-plugin)' "$PARENT" || true
else
echo "sdk-pom.xml not found via git ls-files"
fiRepository: opentdf/java-sdk
Length of output: 190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Files named *pom.xml* (top-level hints) =="
fd -t f -e xml -a pom.xml . | head -n 50
echo
echo "== Search for parent artifactId sdk-pom =="
rg -n --hidden --no-ignore-vcs "<artifactId>sdk-pom</artifactId>" . || true
echo
echo "== Search for fips/non-fips profile config outside sdk/pom.xml =="
rg -n --hidden --no-ignore-vcs -S '(<id>fips</id>|<id>non-fips</id>|fips\.test|java\.security\.properties\.fips|bc-fips|bcprov-jdk18on|bctls-fips|bcpkix-fips)' . || trueRepository: opentdf/java-sdk
Length of output: 2309
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== scripts/README.md: locate hybrid PQC note =="
rg -n "hybrid PQC|non-fips|fips profile" scripts/README.md
echo
echo "== Root pom.xml: show bcprov/bc-fips dependencyManagement section (around bc-fips/bcprov) =="
rg -n --hidden --no-ignore-vcs "bcprov-jdk18on|bc-fips|bctls-fips|bcpkix-fips|dependencyManagement|<dependencies>" pom.xml | head -n 50
# show a narrow region around the dependencyManagement bcprov/bc-fips entries
# (no line numbers tool; use awk to print by range)
echo
echo "== pom.xml lines 100-170 =="
awk '{printf "%6d:%s\n", NR, $0}' pom.xml | sed -n '100,180p'
echo
echo "== sdk/pom.xml: search for any FIPS/non-fips source filtering =="
rg -n --hidden --no-ignore-vcs -S "(<excludes>|<includes>|maven-compiler-plugin|maven-resources-plugin|XWingKeyPair|HybridNISTKeyPair|xwing|mlkem)" sdk/pom.xml
echo
echo "== sdk/pom.xml: confirm no other bcprov dependency under fips profile =="
rg -n --hidden --no-ignore-vcs -S "<profile>|<id>fips</id>|bcprov-jdk18on|bc-fips|bctls-fips|bcpkix-fips" sdk/pom.xmlRepository: opentdf/java-sdk
Length of output: 5921
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Root pom.xml: search for fips/non-fips profiles and bcprov usage =="
rg -n --hidden --no-ignore-vcs -S '(<profiles>|<id>fips</id>|<id>non-fips</id>|bcprov-jdk18on|bcpkix-jdk18on)' pom.xml
echo
echo "== Root pom.xml: show profiles section (rough range 150-260) if present =="
awk '{printf "%6d:%s\n", NR, $0}' pom.xml | sed -n '1,260p'
echo
echo "== Root pom.xml: show any dependencies that mention bcprov/bcpkix (outside dependencyManagement) =="
# crude but effective: print lines around any bcprov/bcpkix occurrences
rg -n --hidden --no-ignore-vcs -S 'bcprov-jdk18on|bcpkix-jdk18on' pom.xml -n --context 3Repository: opentdf/java-sdk
Length of output: 13901
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sdk/pom.xml: all bcprov/bcpkix declarations =="
rg -n --hidden --no-ignore-vcs 'bcprov-jdk18on|bcpkix-jdk18on|bc-fips|bctls-fips|bcpkix-fips' sdk/pom.xml
echo
echo "== sdk/pom.xml: any compiler excludes/includes for hybrid sources =="
rg -n --hidden --no-ignore-vcs -S 'maven-compiler-plugin|<excludes>|<includes>|XWingKeyPair|HybridNISTKeyPair|xwing|mlkem' sdk/pom.xml
echo
echo "== sdk/pom.xml: show any maven-compiler-plugin configuration blocks =="
rg -n --hidden --no-ignore-vcs 'maven-compiler-plugin' sdk/pom.xmlRepository: opentdf/java-sdk
Length of output: 743
Fix FIPS build compatibility for hybrid PQC keypair sources (XWingKeyPair / HybridNISTKeyPair)
sdk/src/main/java/io/opentdf/platform/sdk/XWingKeyPair.java(imports at lines 3-10) requiresorg.bouncycastle.crypto.*andorg.bouncycastle.pqc.crypto.xwing.*, but thefipsprofile insdk/pom.xmldoes not includeorg.bouncycastle:bcprov-jdk18on(it only addsbc-fips/bctls-fipsatruntimescope andbcpkix-fipsattestscope).- The same applies to
sdk/src/main/java/io/opentdf/platform/sdk/HybridNISTKeyPair.java(ML-KEM imports). - Matches
scripts/README.mdnoting thefipsprofile “does not yet support hybrid PQC”.
🤖 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/main/java/io/opentdf/platform/sdk/XWingKeyPair.java` around lines 3 -
10, The XWingKeyPair and HybridNISTKeyPair classes import non-FIPS BouncyCastle
PQC APIs (e.g., XWingKEMGenerator, XWingKEMExtractor, XWingKeyPairGenerator and
ML-KEM equivalents) which break the fips profile; modify these classes to avoid
static references to org.bouncycastle.pqc.* by either (a) moving PQC-specific
code behind a separate optional module or factory loaded via reflection, or (b)
replacing the direct imports with provider-agnostic interfaces and runtime
lookups so the code compiles under the fips profile when bcprov-jdk18on is
absent; update XWingKeyPair and HybridNISTKeyPair to use the new indirection
(factory/reflection/provider lookup) for generators/extractors/keypair creation
(e.g., XWingKEMGenerator, XWingKEMExtractor, XWingKeyPairGenerator and MLKEM
equivalents) so the fips build no longer requires non-fips BC classes at compile
time.
jentfoo
left a comment
jentfoo
left a comment
There was a problem hiding this comment.
No issues spotted, just unfortunate having to bring BC back after just removing it.
There was a problem hiding this comment.
I think that this breaks FIPS, at least with BouncyCastle. I'm not sure which other providers are out there but it doesn't seem like there are many.
Since these classes aren't present in the FIPS jar we'd need to include the regular BC jar in order to guarantee that we don't have link errors. The spec says
This specification allows an implementation flexibility as to when linking activities (and, because of recursion, loading) take place, provided that all of the following properties are maintained:
A class or interface is completely loaded before it is linked. A class or interface is completely verified and prepared before it is initialized. Errors detected during linkage are thrown at a point in the program where some action is taken by the program that might, directly or indirectly, require linkage to the class or interface involved in the error. A symbolic reference to a dynamically-computed constant is not resolved until either (i) an ldc, ldc_w, or ldc2_w instruction that refers to it is executed, or (ii) a bootstrap method that refers to it as a static argument is invoked. A symbolic reference to a dynamically-computed call site is not resolved until a bootstrap method that refers to it as a static argument is invoked.
which makes me think that a class loader would be justified in throwing a ClassNotFoundException at startup if the regular BC provider is not on the class path. But if the regular provider is on the classpath then the FIPS jar will not work properly because of collisions in the standard and FIPS class names.
If we really want this to work we could put it behind a separate jar and the java SPI maybe?
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridCrypto.java (1)
94-97:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInteger overflow in length bounds check allows unbounded allocation (DoS).
readLengthcan return up to0x7FFFFFFF, soc.pos + lenoverflows to a negative value and the guard is bypassed, after whichnew byte[len]attempts a ~2 GB allocation. This is reachable on untrusted input duringunwrapDEK: an 8-byte envelope30 06 80 84 7F FF FF FFpasses the SEQUENCE checks and reaches this allocation before any ciphertext-size validation, crashing withOutOfMemoryError.Use overflow-safe subtraction (the same pattern should be applied to
seqEndat line 72, though the strict equality at line 76 currently masks it there).🛡️ Proposed fix
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"); }🤖 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 94 - 97, The bounds check on the length read from readLength is vulnerable to integer overflow (c.pos + len), allowing len to wrap negative and bypass the guard; change the check to an overflow-safe form such as verifying len is non-negative and len <= c.buf.length - c.pos before allocating (and throw SDKException if not), and apply the same subtraction-based safety for calculating/validating seqEnd in the same method (e.g., ensure seqEnd is within 0..c.buf.length using seqEnd <= c.buf.length and computed via subtraction to avoid overflow).
♻️ Duplicate comments (2)
sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridCrypto.java (2)
185-193:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPin the HKDF salt input to an explicit charset.
defaultTDFSalt()still uses"TDF".getBytes(), which depends on the JVM default charset and can change the derived wrap key across environments, breaking interoperability.💡 Suggested fix
- d.update("TDF".getBytes()); + d.update("TDF".getBytes(java.nio.charset.StandardCharsets.US_ASCII));🤖 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 185 - 193, The defaultTDFSalt() function uses "TDF".getBytes() which relies on the platform default charset; change it to use an explicit charset (e.g., StandardCharsets.UTF_8) when converting the literal to bytes so the MessageDigest d = MessageDigest.getInstance("SHA-256") always digests the same input across environments; update imports if necessary to include java.nio.charset.StandardCharsets and keep the same exception handling in defaultTDFSalt().
133-152:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
readLength()still accepts non-canonical DER length encodings.Long-form encodings for values
< 0x80and leading-zero length bytes are accepted, so multiple encodings map to the same envelope on untrusted input. Reject non-minimal lengths to enforce strict DER (per the earlier suggestion).🤖 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 133 - 152, readLength currently accepts non-canonical DER encodings (long-form for values < 0x80 and leading-zero length bytes). Update readLength(Cursor c) to enforce strict DER: after computing numBytes and reading the bytes into len, throw an SDKException if len < 0x80 (because values < 0x80 must use short-form) and also throw if the most-significant length byte is 0 (leading-zero) when numBytes > 1; you can detect the MSB by inspecting (len >> ((numBytes - 1) * 8)) & 0xFF after the read. Keep existing SDKException usage for errors.
🤖 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 `@cmdline/pom.xml`:
- Around line 84-89: The unconditional runtime dependency on artifactId
sdk-pqc-bc causes it to be pulled into FIPS builds; remove the dependency block
from the main dependencies section and place it inside a Maven profile (e.g.,
id="pqc" or id="non-fips") so it is only included when that profile is
explicitly activated, keeping the same groupId/artifactId/version
(${project.version}) and runtime scope; ensure the FIPS build does not activate
that profile (or make the profile opt-in) so omission is enforced by build
configuration rather than documentation.
In `@pom.xml`:
- Line 289: The root pom.xml now lists a fourth module "sdk-pqc-bc" which
diverges from the repo policy expecting three root modules; either remove
"sdk-pqc-bc" from the <module> entries in the root POM to restore the
three-module layout (matching modules sdk/, cmdline/, examples/), or update the
repository build policy/docs to include and describe the new root module
contract and add "sdk-pqc-bc" to that documentation; locate the
<module>sdk-pqc-bc</module> entry in the root pom.xml and apply the chosen fix
(remove the module entry or update the policy/docs accordingly).
In `@sdk-pqc-bc/pom.xml`:
- Around line 34-37: The project uses org.bouncycastle:bcprov-jdk18on without an
explicit version in this module; update the central bouncycastle.version
property from 1.82 to 1.84 in the root dependencyManagement (change the
<bouncycastle.version> value) so this module inherits the patched release, and
audit any LDAP-related code paths that may use BouncyCastle helpers (e.g.,
LDAPStoreHelper or any LDAP filter construction) to ensure metacharacter
neutralization is applied per the 1.84 patch notes.
In `@sdk/src/main/java/io/opentdf/platform/sdk/spi/KemProviders.java`:
- Around line 74-86: The load() method currently returns a mutable HashMap which
violates the class Javadoc and exposes the static cache to external mutation via
KemProviders.registered(); change load() to return an unmodifiable map (e.g.,
wrap the populated map with Collections.unmodifiableMap(...) or use
Map.copyOf(...)) so the static cache and the registered() keySet/view are
immutable; update the return in KemProviders.load() accordingly so the static
cache remains safe from external clear/remove calls and races.
---
Outside diff comments:
In `@sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridCrypto.java`:
- Around line 94-97: The bounds check on the length read from readLength is
vulnerable to integer overflow (c.pos + len), allowing len to wrap negative and
bypass the guard; change the check to an overflow-safe form such as verifying
len is non-negative and len <= c.buf.length - c.pos before allocating (and throw
SDKException if not), and apply the same subtraction-based safety for
calculating/validating seqEnd in the same method (e.g., ensure seqEnd is within
0..c.buf.length using seqEnd <= c.buf.length and computed via subtraction to
avoid overflow).
---
Duplicate comments:
In `@sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridCrypto.java`:
- Around line 185-193: The defaultTDFSalt() function uses "TDF".getBytes() which
relies on the platform default charset; change it to use an explicit charset
(e.g., StandardCharsets.UTF_8) when converting the literal to bytes so the
MessageDigest d = MessageDigest.getInstance("SHA-256") always digests the same
input across environments; update imports if necessary to include
java.nio.charset.StandardCharsets and keep the same exception handling in
defaultTDFSalt().
- Around line 133-152: readLength currently accepts non-canonical DER encodings
(long-form for values < 0x80 and leading-zero length bytes). Update
readLength(Cursor c) to enforce strict DER: after computing numBytes and reading
the bytes into len, throw an SDKException if len < 0x80 (because values < 0x80
must use short-form) and also throw if the most-significant length byte is 0
(leading-zero) when numBytes > 1; you can detect the MSB by inspecting (len >>
((numBytes - 1) * 8)) & 0xFF after the read. Keep existing SDKException usage
for errors.
🪄 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: 7f819728-2b79-47da-871c-2c8e2296b396
📒 Files selected for processing (15)
cmdline/pom.xmlpom.xmlscripts/README.mdsdk-pqc-bc/pom.xmlsdk-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/HybridNISTKeyPair.javasdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/XWingKeyPair.javasdk-pqc-bc/src/main/resources/META-INF/services/io.opentdf.platform.sdk.spi.KemProvidersdk-pqc-bc/src/test/java/io/opentdf/platform/sdk/TDFHybridTest.javasdk-pqc-bc/src/test/java/io/opentdf/platform/sdk/pqc/bc/HybridCryptoTest.javasdk/pom.xmlsdk/src/main/java/io/opentdf/platform/sdk/TDF.javasdk/src/main/java/io/opentdf/platform/sdk/spi/KemProvider.javasdk/src/main/java/io/opentdf/platform/sdk/spi/KemProviders.java
✅ Files skipped from review due to trivial changes (2)
- sdk-pqc-bc/src/main/resources/META-INF/services/io.opentdf.platform.sdk.spi.KemProvider
- sdk/src/main/java/io/opentdf/platform/sdk/spi/KemProvider.java
🚧 Files skipped from review as they are similar to previous changes (2)
- sdk/src/main/java/io/opentdf/platform/sdk/TDF.java
- scripts/README.md
| <dependency> | ||
| <groupId>io.opentdf.platform</groupId> | ||
| <artifactId>sdk-pqc-bc</artifactId> | ||
| <version>${project.version}</version> | ||
| <scope>runtime</scope> | ||
| </dependency> |
There was a problem hiding this comment.
sdk-pqc-bc is always included despite the “FIPS should omit” contract.
Right now this runtime dependency is unconditional, so FIPS-oriented cmdline builds still pull it in. Gate it behind a non-FIPS profile (or an opt-in PQC profile) so omission is enforceable by build config, not documentation.
🤖 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 `@cmdline/pom.xml` around lines 84 - 89, The unconditional runtime dependency
on artifactId sdk-pqc-bc causes it to be pulled into FIPS builds; remove the
dependency block from the main dependencies section and place it inside a Maven
profile (e.g., id="pqc" or id="non-fips") so it is only included when that
profile is explicitly activated, keeping the same groupId/artifactId/version
(${project.version}) and runtime scope; ensure the FIPS build does not activate
that profile (or make the profile opt-in) so omission is enforced by build
configuration rather than documentation.
| <id>develop</id> | ||
| <modules> | ||
| <module>sdk</module> | ||
| <module>sdk-pqc-bc</module> |
There was a problem hiding this comment.
Root module list now diverges from the documented repository structure policy.
Adding sdk-pqc-bc under develop introduces a fourth root module; either update the repository build policy/docs to reflect this new module contract, or keep the root module set aligned with policy.
As per coding guidelines, root pom.xml is expected to maintain a three-module structure (sdk/, cmdline/, examples/).
🤖 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 `@pom.xml` at line 289, The root pom.xml now lists a fourth module "sdk-pqc-bc"
which diverges from the repo policy expecting three root modules; either remove
"sdk-pqc-bc" from the <module> entries in the root POM to restore the
three-module layout (matching modules sdk/, cmdline/, examples/), or update the
repository build policy/docs to include and describe the new root module
contract and add "sdk-pqc-bc" to that documentation; locate the
<module>sdk-pqc-bc</module> entry in the root pom.xml and apply the chosen fix
(remove the module entry or update the policy/docs accordingly).
| <dependency> | ||
| <groupId>org.bouncycastle</groupId> | ||
| <artifactId>bcprov-jdk18on</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the first patched version of org.bouncycastle:bcprov-jdk18on for GHSA-c3fc-8qff-9hwx and GHSA-p93r-85wp-75v3, and are there any migration notes?
💡 Result:
The first patched version for both GHSA-c3fc-8qff-9hwx and GHSA-p93r-85wp-75v3 is 1.84 [1][2][3][4]. For users unable to upgrade immediately to 1.84, the Bouncy Castle maintainers released backported patch versions 1.80.2 and 1.81.1, which include fixes for these vulnerabilities [5][6]. Regarding migration notes, the primary guidance is to upgrade to version 1.84 or later [3]. If you are utilizing the LDAPStoreHelper class (specifically related to GHSA-c3fc-8qff-9hwx/CVE-2026-0636), ensure your implementation is reviewed for proper escaping of LDAP filter metacharacters, as the fix involves introducing this neutralization [3]. No major breaking API changes have been highlighted as part of these specific security patches, but it is standard practice to rebuild dependent artifacts after upgrading the dependency [3]. For specific configuration adjustments if you cannot upgrade, manually pre-sanitizing inputs (encoding , *, (,), and NUL per RFC 4515) before calling the helper is recommended as a temporary mitigation [3].
Citations:
- 1: https://osv.dev/vulnerability/GHSA-p93r-85wp-75v3
- 2: https://test.osv.dev/vulnerability/GHSA-c3fc-8qff-9hwx
- 3: https://www.sentinelone.com/vulnerability-database/cve-2026-0636/
- 4: https://app.opencve.io/cve/?vendor=bouncycastle
- 5: spring-cloud-commons usage of bcprov-jdk18on 1.81 with CVE's bcgit/bc-java#2293
- 6: https://nvd.nist.gov/vuln/detail/CVE-2026-5588
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print relevant version/property definitions
echo "=== sdk-pqc-bc/pom.xml (lines around dependency) ==="
nl -ba sdk-pqc-bc/pom.xml | sed -n '1,120p' | sed -n '1,120p' >/dev/null
nl -ba sdk-pqc-bc/pom.xml | sed -n '25,60p'
echo "=== Root pom.xml bouncycastle.version property ==="
ROOT_POM="pom.xml"
if [ -f "$ROOT_POM" ]; then
nl -ba "$ROOT_POM" | rg -n "bouncycastle\.version" -n || true
nl -ba "$ROOT_POM" | sed -n '1,120p' | rg -n "bouncycastle\.version" || true
# Print some surrounding lines for the property
nl -ba "$ROOT_POM" | rg -n "bouncycastle\.version" | cut -d: -f1 | head -n1 | while read -r ln; do
start=$((ln-10)); end=$((ln+10));
nl -ba "$ROOT_POM" | sed -n "${start},${end}p"
done
else
echo "Root pom.xml not found at repository root."
fi
echo "=== Search for bcprov-jdk18on usage and version management ==="
rg -n --hidden --glob 'pom.xml' "bcprov-jdk18on" .
rg -n --hidden --glob 'pom.xml' "<bouncycastle\.version>" .Repository: opentdf/java-sdk
Length of output: 174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== sdk-pqc-bc/pom.xml dependency section ==="
nl_cmd() { awk '{printf "%6d %s\n", NR, $0}'; }
if [ -f "sdk-pqc-bc/pom.xml" ]; then
nl_cmd < sdk-pqc-bc/pom.xml | sed -n '1,120p' | sed -n '25,80p' || true
rg -n "bcprov-jdk18on|bouncycastle\.version|<version>" -S sdk-pqc-bc/pom.xml || true
# Print a tight window around bcprov-jdk18on
line=$(rg -n "bcprov-jdk18on" sdk-pqc-bc/pom.xml | head -n1 | cut -d: -f1 || true)
if [ -n "${line}" ]; then
start=$((line-8)); end=$((line+8))
echo "--- Window ${start}-${end} around bcprov-jdk18on ---"
nl_cmd < sdk-pqc-bc/pom.xml | awk -v s="$start" -v e="$end" '$1>=s && $1<=e{print $0}'
fi
else
echo "sdk-pqc-bc/pom.xml not found"
fi
echo "=== Root pom.xml property ${bouncycastle.version} ==="
ROOT_POM="pom.xml"
if [ -f "$ROOT_POM" ]; then
rg -n "<bouncycastle\.version>" "$ROOT_POM" || true
line=$(rg -n "<bouncycastle\.version>" "$ROOT_POM" | head -n1 | cut -d: -f1 || true)
if [ -n "${line}" ]; then
start=$((line-10)); end=$((line+10))
echo "--- Window ${start}-${end} around <bouncycastle.version> ---"
nl_cmd < "$ROOT_POM" | awk -v s="$start" -v e="$end" '$1>=s && $1<=e{print $0}'
fi
echo "=== Search for version management / dependencyManagement usage ==="
rg -n "<dependencyManagement>|<dependencies>|bcprov-jdk18on|bouncycastle\.version" "$ROOT_POM" || true
else
echo "pom.xml not found"
fi
echo "=== Search for any overrides of bcprov-jdk18on within sdk-pqc-bc/pom.xml ==="
rg -n "bcprov-jdk18on" sdk-pqc-bc/pom.xml -n || trueRepository: opentdf/java-sdk
Length of output: 3623
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for bouncycastle.version / bcprov-jdk18on version management ==="
rg -n "bouncycastle\.version|bcprov-jdk18on" -S pom.xml sdk-pqc-bc/pom.xml || true
echo "=== Root pom.xml: window around <bouncycastle.version> ==="
ROOT_POM="pom.xml"
if [ -f "$ROOT_POM" ]; then
if rg -n "<bouncycastle\.version>" "$ROOT_POM" >/dev/null; then
line=$(rg -n "<bouncycastle\.version>" "$ROOT_POM" | head -n1 | cut -d: -f1)
start=$((line-10)); end=$((line+10))
sed -n "${start},${end}p" "$ROOT_POM" | nl -ba -w3 -s':'
else
echo "No <bouncycastle.version> property found in root pom.xml."
fi
echo "=== Root pom.xml: window around dependencyManagement entry for bcprov-jdk18on (if any) ==="
if rg -n "bcprov-jdk18on" "$ROOT_POM" >/dev/null; then
line=$(rg -n "bcprov-jdk18on" "$ROOT_POM" | head -n1 | cut -d: -f1)
start=$((line-15)); end=$((line+15))
sed -n "${start},${end}p" "$ROOT_POM" | nl -ba -w3 -s':'
else
echo "No bcprov-jdk18on usage found in root pom.xml."
fi
else
echo "pom.xml not found at repository root."
fi
echo "=== Any bcprov-jdk18on overrides elsewhere ==="
rg -n --glob '**/pom.xml' "bcprov-jdk18on" . || trueRepository: opentdf/java-sdk
Length of output: 681
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Root pom.xml: lines around <bouncycastle.version> ==="
line=$(rg -n "<bouncycastle\.version>" pom.xml | head -n1 | cut -d: -f1)
start=$((line-10)); end=$((line+10))
sed -n "${start},${end}p" pom.xml | cat -n
echo "=== Root pom.xml: dependencyManagement entry(s) for bcprov-jdk18on ==="
rg -n "bcprov-jdk18on" pom.xml | head -n20 | cat
for l in $(rg -n "bcprov-jdk18on" pom.xml | cut -d: -f1 | sort -u); do
start=$((l-12)); end=$((l+12))
echo "--- Window around match at line $l ($start-$end) ---"
sed -n "${start},${end}p" pom.xml | cat -n
done
echo "=== sdk-pqc-bc/pom.xml: confirm bcprov-jdk18on has no local version ==="
rg -n "bcprov-jdk18on" sdk-pqc-bc/pom.xml
sed -n '28,45p' sdk-pqc-bc/pom.xml | cat -nRepository: opentdf/java-sdk
Length of output: 3651
Upgrade Bouncy Castle bcprov-jdk18on to a patched release (1.82 → 1.84).
sdk-pqc-bc/pom.xml declares org.bouncycastle:bcprov-jdk18on without a version; the root pom.xml supplies it via <bouncycastle.version>1.82</bouncycastle.version> in dependencyManagement. For GHSA-c3fc-8qff-9hwx and GHSA-p93r-85wp-75v3, the first patched version is 1.84 (backports: 1.81.1 / 1.80.2). Review any affected LDAP-filter construction paths (patch includes LDAP metacharacter neutralization, e.g., around LDAPStoreHelper) if used.
🤖 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/pom.xml` around lines 34 - 37, The project uses
org.bouncycastle:bcprov-jdk18on without an explicit version in this module;
update the central bouncycastle.version property from 1.82 to 1.84 in the root
dependencyManagement (change the <bouncycastle.version> value) so this module
inherits the patched release, and audit any LDAP-related code paths that may use
BouncyCastle helpers (e.g., LDAPStoreHelper or any LDAP filter construction) to
ensure metacharacter neutralization is applied per the 1.84 patch notes.
| private static Map<KeyType, KemProvider> load() { | ||
| Map<KeyType, KemProvider> map = new HashMap<>(); | ||
| for (KemProvider provider : ServiceLoader.load(KemProvider.class)) { | ||
| for (KeyType kt : provider.supportedKeyTypes()) { | ||
| KemProvider existing = map.putIfAbsent(kt, provider); | ||
| if (existing != null && existing != provider) { | ||
| logger.warn("multiple KemProviders claim {}: keeping {}, ignoring {}", | ||
| kt, existing.getClass().getName(), provider.getClass().getName()); | ||
| } | ||
| } | ||
| } | ||
| return map; | ||
| } |
There was a problem hiding this comment.
Return an unmodifiable map to match the documented contract.
load() returns a plain HashMap, but the class Javadoc states it "builds an unmodifiable KeyType → KemProvider map." Because the same instance is published via the static cache, registered() (lines 56-58) hands callers a live, mutable keySet() view: KemProviders.registered().clear()/remove(...) would corrupt the shared cache for the whole JVM and race with readers.
🛡️ Proposed fix
- return map;
+ return java.util.Collections.unmodifiableMap(map);📝 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.
| private static Map<KeyType, KemProvider> load() { | |
| Map<KeyType, KemProvider> map = new HashMap<>(); | |
| for (KemProvider provider : ServiceLoader.load(KemProvider.class)) { | |
| for (KeyType kt : provider.supportedKeyTypes()) { | |
| KemProvider existing = map.putIfAbsent(kt, provider); | |
| if (existing != null && existing != provider) { | |
| logger.warn("multiple KemProviders claim {}: keeping {}, ignoring {}", | |
| kt, existing.getClass().getName(), provider.getClass().getName()); | |
| } | |
| } | |
| } | |
| return map; | |
| } | |
| private static Map<KeyType, KemProvider> load() { | |
| Map<KeyType, KemProvider> map = new HashMap<>(); | |
| for (KemProvider provider : ServiceLoader.load(KemProvider.class)) { | |
| for (KeyType kt : provider.supportedKeyTypes()) { | |
| KemProvider existing = map.putIfAbsent(kt, provider); | |
| if (existing != null && existing != provider) { | |
| logger.warn("multiple KemProviders claim {}: keeping {}, ignoring {}", | |
| kt, existing.getClass().getName(), provider.getClass().getName()); | |
| } | |
| } | |
| } | |
| return java.util.Collections.unmodifiableMap(map); | |
| } |
🤖 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/main/java/io/opentdf/platform/sdk/spi/KemProviders.java` around lines
74 - 86, The load() method currently returns a mutable HashMap which violates
the class Javadoc and exposes the static cache to external mutation via
KemProviders.registered(); change load() to return an unmodifiable map (e.g.,
wrap the populated map with Collections.unmodifiableMap(...) or use
Map.copyOf(...)) so the static cache and the registered() keySet/view are
immutable; update the return in KemProviders.load() accordingly so the static
cache remains safe from external clear/remove calls and races.
mkleene
left a comment
mkleene
left a comment
There was a problem hiding this comment.
Overall I think this will work well. Keeps the FIPS story pretty simple and should be simple to consume for customers.
There was a problem hiding this comment.
can we turn this into a test that runs in CI like this?
marythought
left a comment
marythought
left a comment
There was a problem hiding this comment.
DX Review
Strong PR — the SPI + ServiceLoader architecture is clean, the test script is genuinely helpful, and the wire format is well-documented for cross-SDK interop. A few things worth considering:
Design feedback
-
HybridNISTKeyPairdual role.P256_MLKEM768is a static template withpublicKey = null, and.generate()returns a new instance with keys populated — but both are the same type. Nothing prevents calling instance methods on the template, and the API shape suggestsP256_MLKEM768itself holds a keypair. Consider separating the parameters object from the keypair, or making the distinction visible in the type name. -
Duplicated dispatch switch.
HybridCrypto.wrapDEKandBouncyCastleKemProvider.wrapDEKboth switch over the same threeKeyTypes dispatching to the same calls. A fourth algorithm means two switch statements to update. Consider having the provider delegate toHybridCrypto, or removing the dispatcher fromHybridCryptoentirely. -
Auto-discovery gap deserves a defensive error now. The README documents that
fromPublicKeyAlgorithm()doesn't map hybrid protobuf enums. When unrecognized enum values do arrive, the currentdefaultcase throws a bareIllegalArgumentExceptionwith no mention of PQC or what to do. Since the enum mapping can't be added until the protobuf values exist, the fix available now is makingfromPublicKeyAlgorithm()handle unknown values gracefully — either skip with a log warning, or throw an error that says what's happening. Either is better than a rawIllegalArgumentException.
Minor
concat()is defined in three places (HybridCrypto,HybridNISTKeyPair,XWingKeyPair).HybridCrypto.defaultTDFSalt()recomputesSHA-256("TDF")on every call —TDF.javaalready hasGLOBAL_KEY_SALTdoing the same thing.XWingKeyPair.PRIVATE_KEY_SIZE = 32— worth a note that this is a seed, not the full private key.sdk-pqc-bc/pom.xmlhardcodes<maven.compiler.source>11</maven.compiler.source>— intentional divergence from parent?KemProvider.supportedKeyTypes()has no contract about immutability/thread-safety of the returnedSet— matters since this is a public SPI.
4 participants
Add hybrid post-quantum key wrapping to the Java SDK so TDFs can be protected against "harvest now, decrypt later" attacks while preserving classical security guarantees during the PQC transition.
Introduces three new
KeyTypevalues backed by hybrid KEMs:HybridXWingKey(hpqt:xwing) — X-Wing (X25519 + ML-KEM-768)HybridSecp256r1MLKEM768Key(hpqt:secp256r1-mlkem768)HybridSecp384r1MLKEM1024Key(hpqt:secp384r1-mlkem1024)When a KAS advertises one of these algorithms,
TDF.upsertAndGetNewKeyAccessroutes throughHybridCrypto.wrapDEK, which performs both a classical ECDH/X25519 key agreement and an ML-KEM encapsulation, combines the two shared secrets (HKDF-SHA256 with the standard TDF salt), and uses the result to wrap the DEK with AES-256-GCM. A newhybrid-wrappedkey-access type is emitted; the ephemeral classical public key and ML-KEM ciphertext are packaged together inside an ASN.1 envelope stored inwrappedKey(rather than the separateephemeralPublicKeyfield used for EC-wrapped keys).New supporting classes:
HybridCrypto,HybridNISTKeyPair,XWingKeyPair, plus unit tests and a full-manifest TDF round-trip test.Provider-agnostic implementation
In line with #367's removal of BouncyCastle as a compile dependency, this PR limits BC usage to the only primitives JDK 11 stdlib cannot supply — ML-KEM keygen/encap/decap and X-Wing keygen/encap/decap (the JCA
KEMAPI is JDK21+; ML-KEM in stdlib is 24+).Everything else is stdlib JCA or an existing SDK helper:
SEQUENCE { [0] IMPLICIT OCTET STRING, [1] IMPLICIT OCTET STRING }with multi-byte length support. Noorg.bouncycastle.asn1.*imports. - HKDF — delegates to the existingECKeyPair.calculateHKDF(salt, secret)(RFC 5869, empty info, L=32 — what all three algorithms need).KeyPairGenerator.getInstance("EC"),KeyAgreement.getInstance("ECDH"),AlgorithmParameters.getInstance("EC")withECGenParameterSpec. NoBouncyCastleProviderregistration; consumers control providers viajava.securityas the ADR intends.bcprov-jdk18onis declared at compile/runtime scope in the defaultnon-fipsMaven profile, version pinned via the existing parentdep-management entry.
Out of scope (follow-ups)
fipsprofile support for hybrid PQC — needs verification of whichbc-fipsversion ships ML-KEM and X-Wing and how it registers them.Summary by CodeRabbit
Release Notes
New Features
Tests
Documentation