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 2 commits into
Conversation
…Wing, ECDH+ML-KEM)
📝 WalkthroughWalkthroughThis PR adds hybrid post-quantum cryptography support to the Java TDF SDK by introducing X-Wing and NIST ECDH+ML-KEM key wrapping schemes. It includes implementations for both key schemes, shared envelope marshaling and key derivation utilities, integration into TDF manifest creation, and comprehensive unit and integration test coverage. ChangesHybrid Post-Quantum Cryptography
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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.
1 participant
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 KeyType values backed by hybrid KEMs:
When a KAS advertises one of these algorithms, TDF.upsertAndGetNewKeyAccess routes through HybridCrypto.wrapDEK, which performs both a classical ECDH/X25519 key
agreement and an ML-KEM encapsulation, combines the two shared secrets, and uses the result to wrap the DEK. A new hybrid-wrapped key-access type is emitted; the
ephemeral classical public key and ML-KEM ciphertext are packaged together inside an ASN.1 envelope stored in wrappedKey (rather than the separate ephemeralPublicKey
field used for EC-wrapped keys).
New supporting classes: HybridCrypto, HybridNISTKeyPair, XWingKeyPair, plus unit and TDF round-trip tests.
Summary by CodeRabbit