Skip to content

feat(sdk): DSPX-3309 add hybrid post-quantum key wrapping for KAS (X-Wing, ECDH+ML-KEM)#368

Open
sujankota wants to merge 9 commits into
mainfrom
DSPX-3309-hybrid-pq-key-wrapping
Open

feat(sdk): DSPX-3309 add hybrid post-quantum key wrapping for KAS (X-Wing, ECDH+ML-KEM)#368
sujankota wants to merge 9 commits into
mainfrom
DSPX-3309-hybrid-pq-key-wrapping

Conversation

@sujankota

@sujankota sujankota commented May 18, 2026

Copy link
Copy Markdown
Contributor

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:

  • 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.upsertAndGetNewKeyAccess routes through HybridCrypto.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 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 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 KEM API is JDK21+; ML-KEM in stdlib is 24+).

Everything else is stdlib JCA or an existing SDK helper:

  • ASN.1 envelope — minimal hand-rolled DER codec for SEQUENCE { [0] IMPLICIT OCTET STRING, [1] IMPLICIT OCTET STRING } with multi-byte length support. No org.bouncycastle.asn1.* imports. - HKDF — delegates to the existing ECKeyPair.calculateHKDF(salt, secret) (RFC 5869, empty info, L=32 — what all three algorithms need).
  • EC keygen / ECDH / curve parameters — stdlib
    KeyPairGenerator.getInstance("EC"),
    KeyAgreement.getInstance("ECDH"),
    AlgorithmParameters.getInstance("EC") with ECGenParameterSpec. No
    BouncyCastleProvider registration; consumers control providers via
    java.security as the ADR intends.

bcprov-jdk18on is declared at compile/runtime scope in the default
non-fips Maven profile, version pinned via the existing parent
dep-management entry.

Out of scope (follow-ups)

  • fips profile support for hybrid PQC — needs verification of which
    bc-fips version ships ML-KEM and X-Wing and how it registers them.

Review Change Stack

Summary by CodeRabbit

Release Notes

  • New Features

    • Added support for hybrid post-quantum cryptography with three new key types: X-Wing, secp256r1+ML-KEM-768, and secp384r1+ML-KEM-1024.
    • Introduced hybrid key wrapping mechanism for TDF encryption, enabling classical and post-quantum algorithm combinations.
  • Tests

    • Added end-to-end testing suite for hybrid post-quantum key wrapping workflows.
  • Documentation

    • Updated test documentation with hybrid PQC integration workflow and troubleshooting guidance.

Review Change Stack

@sujankota sujankota requested review from a team as code owners May 18, 2026 19:21
@coderabbitai

coderabbitai Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This 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.

Changes

Hybrid Post-Quantum Cryptography

Layer / File(s) Summary
KeyType and KemProvider SPI contracts
sdk/src/main/java/io/opentdf/platform/sdk/KeyType.java, sdk/src/main/java/io/opentdf/platform/sdk/spi/KemProvider.java, sdk/src/main/java/io/opentdf/platform/sdk/spi/KemProviders.java
KeyType adds HybridXWingKey, HybridSecp256r1MLKEM768Key, HybridSecp384r1MLKEM1024Key constants and isHybrid() predicate. New KemProvider SPI defines supportedKeyTypes(), wrapDEK(), and unwrapDEK() contract. KemProviders registry discovers and caches providers via ServiceLoader with thread-safe lazy initialization.
HybridCrypto utilities and envelope codec
sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridCrypto.java
Package-private HybridCrypto provides wrapDEK() dispatcher routing by KeyType, DER/ASN.1 envelope marshal/unmarshal (two implicit context-tagged OCTET STRINGs) with strict validation, HKDF-SHA256 wrap-key derivation using default salt SHA-256("TDF"), and strict PEM encode/decode with size checks.
XWingKeyPair – X25519 + ML-KEM-768
sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/XWingKeyPair.java
Stores encoded X25519 and ML-KEM-768 key bytes. generate() uses Bouncy Castle's XWingKeyPairGenerator. PEM serialization/deserialization with size validation. wrapDEK() encapsulates shared secret, derives wrap key, encrypts DEK via AES-GCM, and marshals envelope. unwrapDEK() reverses the process with strict ciphertext size validation.
HybridNISTKeyPair – NIST ECDH + ML-KEM
sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridNISTKeyPair.java
Combines NIST elliptic-curve ECDH (P-256/P-384) with ML-KEM (768/1024) in two preset configurations. generate() produces EC scalar + uncompressed point concatenated with ML-KEM public bytes and seed, validated for expected sizes. wrapDEK() generates ephemeral EC keypair, computes ECDH secret, performs ML-KEM encapsulation, derives wrap key via HKDF-SHA256, encrypts DEK via AES-GCM. unwrapDEK() reconstructs secrets, derives wrap key, and decrypts. Includes EC parameter resolution, point encoding/decoding, and fixed-length big-endian conversion.
BouncyCastleKemProvider and service registration
sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/BouncyCastleKemProvider.java, sdk-pqc-bc/src/main/resources/META-INF/services/io.opentdf.platform.sdk.spi.KemProvider
BouncyCastleKemProvider implements KemProvider, dispatches wrap/unwrapDEK() by KeyType to XWingKeyPair or HybridNISTKeyPair, throws SDKException for unsupported types. Public no-arg constructor supports ServiceLoader instantiation. Service registration enables runtime discovery.
TDF manifest – hybrid key access creation
sdk/src/main/java/io/opentdf/platform/sdk/TDF.java
TDF.createKeyAccess() imports KemProviders and adds kHybridWrapped constant. When keyType.isHybrid() is true, calls KemProviders.get(keyType).wrapDEK(...), Base64-encodes wrapped bytes into keyAccess.wrappedKey, sets keyAccess.keyType to kHybridWrapped, and leaves ephemeralPublicKey unset for hybrid keys.
HybridCryptoTest – unit tests
sdk-pqc-bc/src/test/java/io/opentdf/platform/sdk/pqc/bc/HybridCryptoTest.java
JUnit tests validate full round-trips for XWing, P‑256+ML-KEM-768, and P‑384+ML-KEM-1024 with PEM encode/decode, wrap/unwrap, and DEK equality. Verify wrapped ciphertext structure (ASN.1 SEQUENCE byte), randomized output, wrong-scheme rejection, malformed PEM rejection, truncated envelope rejection, and dispatcher routing for supported/unsupported key types.
TDFHybridTest – TDF integration tests
sdk-pqc-bc/src/test/java/io/opentdf/platform/sdk/TDFHybridTest.java
Integration tests generate XWing and NIST hybrid keypairs, create TDF manifests, assert keyAccess[0].type == "hybrid-wrapped", ephemeralPublicKey == null, and wrappedKey non-empty. Base64-decode and unwrap each wrappedKey with matching private key; verify recovered DEK is 32 bytes. Mock KAS provides key material. Helper builds TDF config and returns first KeyAccess.

Maven Build Infrastructure

Layer / File(s) Summary
New sdk-pqc-bc Maven module
sdk-pqc-bc/pom.xml
Introduces sdk-pqc-bc module inheriting from sdk-pom (Java 11). Runtime dependency on core SDK and BouncyCastle bcprov-jdk18on. Test dependencies include SDK test-jar, JUnit 5, AssertJ, Mockito, Log4j API/core, SLF4J bridge.
Maven build wiring and profiles
pom.xml, cmdline/pom.xml, sdk/pom.xml
Root pom adds sdk-pqc-bc to develop profile modules. cmdline adds runtime dependency on sdk-pqc-bc (BouncyCastle KEM provider discovered at runtime via ServiceLoader; FIPS deployments should omit). sdk attaches test-jar artifact via maven-jar-plugin and adds comments clarifying BC exclusion from compile path and runtime PQC discovery.

Integration Test Script and Documentation

Layer / File(s) Summary
Integration test script: test-hybrid-pqc.sh
scripts/test-hybrid-pqc.sh
Bash script performs end-to-end round-trips for hybrid PQC. Builds cmdline.jar (or reuses via --skip-build). Optionally queries KAS via grpcurl for hybrid PEM validation. For each algorithm: encrypts plaintext, validates manifest keyAccess[0] (type == "hybrid-wrapped", ephemeralPublicKey empty, wrappedKey starts with ASN.1 SEQUENCE 0x30), decrypts, diffs plaintext. Aggregates failures, exits 0 on all pass or 1 on any failure.
README: hybrid PQC test documentation
scripts/README.md
Documents test workflow, prerequisites (JDK 17, Maven, Buf token, sdk-pqc-bc on classpath, local platform with PQC/hpqt support and registered hybrid KAS keys), CLI tooling. Run examples with flags: --skip-build, --algorithms, --skip-kas-check. Configuration table (platform/KAS URLs, OIDC credentials, encrypt attribute, algorithm subset, skip/build flags). Output format and exit codes. Troubleshooting table (Buf auth, JDK version, missing/non-hybrid PEM, keyType null, KAS rewrap unsupported). Known gap: KeyType.fromAlgorithm/fromPublicKeyAlgorithm don't map hybrid protobuf enums; script bypasses with explicit --encap-key-type.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Suggested reviewers

  • marythought
  • jentfoo

Poem

🐰 Hybrid algorithms hop through time,
X‑Wing and ML-KEM now entwine,
Wrapped with ECDH's finest grace,
Post-quantum secrets in their place!
AES-GCM seals the envelope tight—
The rabbit cheers: the future's right!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.88% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly and specifically describes the main change: adding hybrid post-quantum key wrapping support for KAS with X-Wing and ECDH+ML-KEM variants, which aligns with the substantial changeset across multiple files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch DSPX-3309-hybrid-pq-key-wrapping

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread sdk/src/main/java/io/opentdf/platform/sdk/HybridNISTKeyPair.java Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
sdk/src/main/java/io/opentdf/platform/sdk/XWingKeyPair.java (1)

12-13: ⚡ Quick win

Clear the derived secrets after wrap/unwrap.

sharedSecret and wrapKey stay live until GC on both paths. In the wrapping primitive itself, this is worth clearing in a finally block 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 win

Zero the hybrid shared secrets on both paths.

ecdhSecret, mlSecret, combinedSecret, and wrapKey all remain in heap memory after the DEK is processed. Given this sits at the core crypto boundary, clear them in finally blocks.

💡 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9991b07 and 38943bc.

📒 Files selected for processing (7)
  • 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

Comment thread sdk/src/main/java/io/opentdf/platform/sdk/HybridCrypto.java Outdated
Comment thread sdk/src/main/java/io/opentdf/platform/sdk/KeyType.java
@github-actions

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 38943bc and 12f0b08.

📒 Files selected for processing (2)
  • scripts/README.md
  • scripts/test-hybrid-pqc.sh

Comment thread scripts/README.md Outdated
Comment thread scripts/test-hybrid-pqc.sh
Comment thread scripts/test-hybrid-pqc.sh
@github-actions

Copy link
Copy Markdown
Contributor

@github-actions

Copy link
Copy Markdown
Contributor

X-Test Failure Report

@sujankota sujankota force-pushed the DSPX-3309-hybrid-pq-key-wrapping branch from 6e08ea4 to d666c07 Compare May 21, 2026 23:44
@sujankota sujankota requested a review from a team as a code owner May 21, 2026 23:44

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (3)
scripts/README.md (1)

80-80: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add 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.md at line 80, The fenced expected-output block around the
test output is missing a language tag and triggers MD040; update the opening to 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 opening to 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 -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.
  • 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 -->

Comment thread sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridCrypto.java Outdated
@github-actions

Copy link
Copy Markdown
Contributor

jentfoo
jentfoo previously approved these changes May 22, 2026

@jentfoo jentfoo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues spotted, just unfortunate having to bring BC back after just removing it.

@mkleene mkleene left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@github-actions

Copy link
Copy Markdown
Contributor

@github-actions

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Integer overflow in length bounds check allows unbounded allocation (DoS).

readLength can return up to 0x7FFFFFFF, so c.pos + len overflows to a negative value and the guard is bypassed, after which new byte[len] attempts a ~2 GB allocation. This is reachable on untrusted input during unwrapDEK: an 8-byte envelope 30 06 80 84 7F FF FF FF passes the SEQUENCE checks and reaches this allocation before any ciphertext-size validation, crashing with OutOfMemoryError.

Use overflow-safe subtraction (the same pattern should be applied to seqEnd at 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 win

Pin 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 < 0x80 and 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

📥 Commits

Reviewing files that changed from the base of the PR and between d666c07 and 1f221a3.

📒 Files selected for processing (15)
  • cmdline/pom.xml
  • pom.xml
  • scripts/README.md
  • sdk-pqc-bc/pom.xml
  • sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/BouncyCastleKemProvider.java
  • sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridCrypto.java
  • sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridNISTKeyPair.java
  • sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/XWingKeyPair.java
  • sdk-pqc-bc/src/main/resources/META-INF/services/io.opentdf.platform.sdk.spi.KemProvider
  • sdk-pqc-bc/src/test/java/io/opentdf/platform/sdk/TDFHybridTest.java
  • sdk-pqc-bc/src/test/java/io/opentdf/platform/sdk/pqc/bc/HybridCryptoTest.java
  • sdk/pom.xml
  • sdk/src/main/java/io/opentdf/platform/sdk/TDF.java
  • sdk/src/main/java/io/opentdf/platform/sdk/spi/KemProvider.java
  • sdk/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

Comment thread cmdline/pom.xml Outdated
Comment thread pom.xml Outdated
Comment thread sdk-pqc-bc/pom.xml
Comment thread sdk/src/main/java/io/opentdf/platform/sdk/spi/KemProviders.java
@github-actions

Copy link
Copy Markdown
Contributor

@mkleene mkleene left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think this will work well. Keeps the FIPS story pretty simple and should be simple to consume for customers.

Comment thread scripts/test-hybrid-pqc.sh
Comment thread sdk/src/main/java/io/opentdf/platform/sdk/TDF.java

@marythought marythought left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. HybridNISTKeyPair dual role. P256_MLKEM768 is a static template with publicKey = 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 suggests P256_MLKEM768 itself holds a keypair. Consider separating the parameters object from the keypair, or making the distinction visible in the type name.

  2. Duplicated dispatch switch. HybridCrypto.wrapDEK and BouncyCastleKemProvider.wrapDEK both switch over the same three KeyTypes dispatching to the same calls. A fourth algorithm means two switch statements to update. Consider having the provider delegate to HybridCrypto, or removing the dispatcher from HybridCrypto entirely.

  3. 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 current default case throws a bare IllegalArgumentException with 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 making fromPublicKeyAlgorithm() handle unknown values gracefully — either skip with a log warning, or throw an error that says what's happening. Either is better than a raw IllegalArgumentException.

Minor

  • concat() is defined in three places (HybridCrypto, HybridNISTKeyPair, XWingKeyPair).
  • HybridCrypto.defaultTDFSalt() recomputes SHA-256("TDF") on every call — TDF.java already has GLOBAL_KEY_SALT doing 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.xml hardcodes <maven.compiler.source>11</maven.compiler.source> — intentional divergence from parent?
  • KemProvider.supportedKeyTypes() has no contract about immutability/thread-safety of the returned Set — matters since this is a public SPI.

  Mirrors opentdf/platform PR #3563 which moves the three hybrid PQ/T KEMs
  into interop with draft-ietf-lamps-pq-composite-kem-14 and
  draft-connolly-cfrg-xwing-kem-10.

  Wire format changes:
  - All three algorithms now use standard SPKI/PKCS#8 PEM envelopes;
    custom XWING/SECP256R1-MLKEM768/SECP384R1-MLKEM1024 block names gone.
    AlgorithmIdentifier OIDs select the scheme (X-Wing 1.3.6.1.4.1.62253.25722,
    P256+ML-KEM-768 1.3.6.1.5.5.7.6.59, P384+ML-KEM-1024 1.3.6.1.5.5.7.6.63).
  - NIST hybrid public-key concat order: mlkemPK || ecPoint
    (previously ecPoint || mlkemPK).
  - NIST hybrid private-key encoding: mlkemSeed(64) || RFC 5915
    ECPrivateKey DER (previously raw padded scalar || mlkemSeed).
  - NIST hybrid ciphertext concat order: mlkemCT || ephemeralECPoint
    (previously ephemeralECPoint || mlkemCT).
  - NIST hybrid combiner:
    SHA3-256(mlkemSS || tradSS || tradCT || tradPK || Label) per
    draft-14 §4.3, with Label "MLKEM768-P256" / "MLKEM1024-P384". 32-byte
    output used directly as AES-256 key; no HKDF wrap step.
  - X-Wing wire bytes, combiner, and TDF DEK envelope unchanged.

  New file: HybridSpki.java — encode/parse SPKI + PKCS#8 (raw bytes go
  directly into OCTET STRING / BIT STRING, matching Go's layout) and
  RFC 5915 ECPrivateKey via BC's ASN.1 helpers.

  Build verification: mvn test (default) — 162 sdk + 14 sdk-pqc-bc =
  176 pass, 0 fail. mvn -P fips,!non-fips clean install -DskipTests —
  clean build.
@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

mkleene
mkleene previously approved these changes Jun 5, 2026

@mkleene mkleene left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine to merge. It would be nice if we had a platform integration test but it looks like the tests repo should handle e2e tests.

Looks good!

Comment thread pom.xml
@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

@marythought marythought left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: Mike's concern about bringing BC back after #367 just removed it — the SPI + ServiceLoader architecture addresses this correctly. The core sdk jar stays BC-free at compile time; sdk-pqc-bc is an optional module gated behind the non-fips profile. The FIPS namespace collision Mike flagged is avoided because sdk-pqc-bc is excluded from the reactor entirely under -P fips,!non-fips.

One remaining gap: FIPS users who attempt hybrid PQC get a generic "no KemProvider registered — add sdk-pqc-bc" error, which is misleading because adding sdk-pqc-bc under FIPS would create the namespace collision Mike described. A more targeted message when running under the FIPS profile (or at least a note that hybrid PQC is not available in FIPS mode) would prevent someone from following the error message into a worse state.

Comment thread sdk-pqc-bc/src/main/java/io/opentdf/platform/sdk/pqc/bc/HybridNISTAlgorithm.java Outdated
Comment thread sdk/src/main/java/io/opentdf/platform/sdk/KeyType.java
@marythought

Copy link
Copy Markdown
Contributor

Companion docs PR: opentdf/docs#343 — updates feature matrix, encrypt options, KASInfo/KeyAccess type reference, and KAS PublicKey docs for hybrid PQC.

marythought
marythought previously approved these changes Jun 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
D Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@github-actions

Copy link
Copy Markdown
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants