feat(sdk): add ML-KEM key access objects (DSPX-3229)#933
Open
dmihalcik-virtru wants to merge 5 commits into
Open
feat(sdk): add ML-KEM key access objects (DSPX-3229)#933dmihalcik-virtru wants to merge 5 commits into
dmihalcik-virtru wants to merge 5 commits into
Conversation
📝 WalkthroughWalkthroughThe SDK adds ML-KEM-768 and ML-KEM-1024 support across cryptography, TDF key access, KAS rewrap handling, roundtrip workflows, and the web application. New DER envelopes, PEM/SPKI handling, crypto-service APIs, tests, configuration, and manifest inspection are included. ChangesML-KEM integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WebApp
participant TDFClient
participant CryptoService
participant KAS
WebApp->>TDFClient: createZTDF(wrappingKeyAlgorithm)
TDFClient->>CryptoService: generateMlKemKeyPair(level)
TDFClient->>KAS: send ML-KEM public key for rewrap
KAS->>CryptoService: mlKemEncapsulate(client public key)
KAS-->>TDFClient: return DER kemEnvelope
TDFClient->>CryptoService: mlKemDecapsulate(private key, ciphertext)
TDFClient-->>WebApp: decrypt TDF and expose manifest metadata
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
If these changes look good, signoff on them with: If they aren't any good, please remove them with: |
![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 NIST FIPS 203 ML-KEM (Module-Lattice-Based Key-Encapsulation Mechanism) as a post-quantum cryptographic option for TDF Key Access Objects. It adds the @noble/post-quantum dependency and implements key generation, encapsulation, and decapsulation for ML-KEM-512, 768, and 1024. Key changes include updates to the CryptoService, the addition of an MlKemWrapped key access class, and support in the TDF client and web application for selecting these algorithms. Feedback was provided regarding the need for input length validation when processing wrapped keys to prevent errors with malformed data.
|
If these changes look good, signoff on them with: If they aren't any good, please remove them with: |
dmihalcik-virtru
force-pushed
the
DSPX-3229-claude-version
branch
from
May 15, 2026 02:50
e613ab9 to
819fe70
Compare
dmihalcik-virtru
force-pushed
the
DSPX-3229-claude-version
branch
from
July 7, 2026 22:12
9a51a79 to
2f2cd63
Compare
This was referenced Jul 10, 2026
dmihalcik-virtru
force-pushed
the
DSPX-3229-claude-version
branch
from
July 10, 2026 17:23
2e0cd71 to
e9caab7
Compare
1 similar comment
dmihalcik-virtru
force-pushed
the
DSPX-3229-claude-version
branch
from
July 15, 2026 15:47
0062064 to
f207817
Compare
X-Test Failure Report |
dmihalcik-virtru
marked this pull request as ready for review
July 15, 2026 16:22
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web-app/src/App.tsx (1)
131-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake optional fields explicit in the metadata type.
In TDF Key Access Objects, properties like
urlandprotocolare typically only present for remote KAOs and may be undefined for wrapped KAOs. To prevent potential strict-mode TypeScript errors and ensure the type accurately reflects the runtime data, consider making these fields optional.💡 Proposed refactor
type KaoMetadata = { kid: string; - type: string; - url: string; - protocol: string; + type?: string; + url?: string; + protocol?: string; wrappedKeyBytes: number; };🤖 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 `@web-app/src/App.tsx` around lines 131 - 138, Update the KaoMetadata type so metadata fields that may be absent for wrapped KAOs, particularly url and protocol, are explicitly optional while preserving required fields for all KAOs.lib/tdf3/src/crypto/declarations.ts (1)
166-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRefactor union type to use existing type alias.
The union type defined for
algorithmduplicates theKeyAlgorithmtype alias defined earlier in the file. ReusingKeyAlgorithmimproves maintainability and reduces duplication.♻️ Proposed refactor
export type PublicKeyInfo = { /** Detected algorithm of the key. */ - algorithm: - | 'rsa:2048' - | 'rsa:4096' - | 'ec:secp256r1' - | 'ec:secp384r1' - | 'ec:secp521r1' - | 'mlkem:512' - | 'mlkem:768' - | 'mlkem:1024'; + algorithm: KeyAlgorithm; /** Normalized PEM string (or raw base64 for ML-KEM keys). */🤖 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 `@lib/tdf3/src/crypto/declarations.ts` around lines 166 - 175, Update the algorithm property type in the relevant declaration to use the existing KeyAlgorithm alias instead of repeating the string union, leaving the allowed algorithm values and surrounding declarations unchanged.Source: Linters/SAST tools
🤖 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 `@lib/tdf3/src/crypto/core/mlkem-asn1.ts`:
- Around line 130-142: Validate that the BIT STRING content declared by
bs.length fits within der before slicing in the ML-KEM SPKI decoder. In the
parsing flow around rawKeyLen and the returned rawKey, reject when pos +
rawKeyLen exceeds der.length, preserving the existing expected-size validation
and only returning a complete key.
---
Nitpick comments:
In `@lib/tdf3/src/crypto/declarations.ts`:
- Around line 166-175: Update the algorithm property type in the relevant
declaration to use the existing KeyAlgorithm alias instead of repeating the
string union, leaving the allowed algorithm values and surrounding declarations
unchanged.
In `@web-app/src/App.tsx`:
- Around line 131-138: Update the KaoMetadata type so metadata fields that may
be absent for wrapped KAOs, particularly url and protocol, are explicitly
optional while preserving required fields for all KAOs.
🪄 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: 9bec631b-01e0-412d-84e2-b635191a7857
⛔ Files ignored due to path filters (1)
lib/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
.github/workflows/roundtrip/gen-mlkem-keys.go.github/workflows/roundtrip/init-temp-keys.sh.github/workflows/roundtrip/opentdf.yamlCLAUDE.mdcli/src/cli.tslib/package.jsonlib/src/access.tslib/src/crypto/pemPublicToCrypto.tslib/src/opentdf.tslib/tdf3/src/client/index.tslib/tdf3/src/crypto/core/key-format.tslib/tdf3/src/crypto/core/keys.tslib/tdf3/src/crypto/core/mlkem-asn1.tslib/tdf3/src/crypto/core/mlkem.tslib/tdf3/src/crypto/declarations.tslib/tdf3/src/crypto/index.tslib/tdf3/src/models/key-access.tslib/tdf3/src/tdf.tslib/tests/mocha/encrypt-decrypt.spec.tslib/tests/mocha/unit/crypto-di.spec.tslib/tests/mocha/unit/crypto/mlkem.spec.tslib/tests/mocha/unit/tdf.spec.tslib/tests/server.tsspec/DSPX-3229.mdweb-app/src/App.tsxweb-app/tests/tests/roundtrip.spec.ts
dmihalcik-virtru
force-pushed
the
DSPX-3229-claude-version
branch
from
July 15, 2026 17:46
304e656 to
9e7cb9c
Compare
dmihalcik-virtru
force-pushed
the
DSPX-3229-claude-version
branch
from
July 15, 2026 18:59
9e7cb9c to
9e2a25e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@spec/DSPX-3229.md`:
- Around line 49-52: Add a language identifier such as text to the fenced
Markdown block containing the kemEnvelope DER example in DSPX-3229.md, while
preserving the example content unchanged.
- Around line 17-24: Update the ML-KEM specification consistently to support
mlkem:512, mlkem:768, and mlkem:1024, and use type: "wrapped" for ML-KEM KAOs
instead of "mlkem-wrapped". Revise the algorithm unions, runtime validation
guards, KAO examples, and acceptance criteria at all referenced sections while
preserving the existing variant-specific algorithm behavior.
- Around line 74-75: Resolve the ML-KEM public-key wire-format contradiction in
the specification: choose either raw encapsulation-key bytes encoded as base64
or PEM-wrapped SPKI for both fetch and rewrap requests. Update the objective
text around clientPublicKey and the acceptance criteria at the sections covering
fetch and rewrap so all references consistently require the same representation.
- Around line 57-63: Resolve the conflicting ML-KEM key-derivation requirements
by establishing one authoritative scheme across the specification and
implementation. Align the contract, relevant ML-KEM handling, and test vectors
so they consistently either use the raw 32-byte shared secret or the required
HKDF derivation, without retaining contradictory “no HKDF” and HKDF-based
behavior.
🪄 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: 27a1661e-6fc3-4c41-898a-59672f210482
📒 Files selected for processing (19)
.github/workflows/roundtrip/init-temp-keys.shlib/src/access.tslib/src/crypto/pemPublicToCrypto.tslib/tdf3/src/client/index.tslib/tdf3/src/crypto/core/key-format.tslib/tdf3/src/crypto/core/keys.tslib/tdf3/src/crypto/core/mlkem-asn1.tslib/tdf3/src/crypto/core/mlkem.tslib/tdf3/src/crypto/declarations.tslib/tdf3/src/models/key-access.tslib/tdf3/src/tdf.tslib/tests/mocha/encrypt-decrypt.spec.tslib/tests/mocha/unit/crypto-di.spec.tslib/tests/mocha/unit/crypto/mlkem.spec.tslib/tests/mocha/unit/mlkem-key-access.spec.tslib/tests/mocha/unit/tdf.spec.tslib/tests/server.tsspec/DSPX-3229.mdweb-app/tests/tests/roundtrip.spec.ts
💤 Files with no reviewable changes (2)
- lib/tests/mocha/encrypt-decrypt.spec.ts
- lib/src/access.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- web-app/tests/tests/roundtrip.spec.ts
- lib/tests/mocha/unit/crypto-di.spec.ts
- lib/tests/mocha/unit/tdf.spec.ts
- lib/tdf3/src/crypto/core/keys.ts
- lib/tdf3/src/client/index.ts
- lib/tdf3/src/crypto/core/mlkem.ts
- lib/tdf3/src/models/key-access.ts
- lib/tdf3/src/tdf.ts
dmihalcik-virtru
force-pushed
the
DSPX-3229-claude-version
branch
from
July 15, 2026 19:58
9e2a25e to
c5f2221
Compare
X-Test Failure Report✅ java@main-v0.9.0 |
dmihalcik-virtru
force-pushed
the
DSPX-3229-claude-version
branch
from
July 15, 2026 20:27
a5b9001 to
8ec7511
Compare
dmihalcik-virtru
force-pushed
the
DSPX-3229-claude-version
branch
from
July 16, 2026 15:21
8ec7511 to
cdd9d89
Compare
dmihalcik-virtru
force-pushed
the
DSPX-3229-claude-version
branch
from
July 16, 2026 16:11
cdd9d89 to
67c6ad0
Compare
dmihalcik-virtru
force-pushed
the
DSPX-3229-claude-version
branch
from
July 17, 2026 19:06
67c6ad0 to
5970bff
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/tdf3/src/tdf.ts (1)
740-756: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve distinct key-access alternatives hosted by the same KAS.
Indexing solely by URL rejects otherwise distinct KAOs, such as entries with different
kidvalues during key rotation. A previously readable manifest can therefore become unreconstructable. Use an index or a URL-plus-KID key for the promise map instead of throwing.🤖 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 `@lib/tdf3/src/tdf.ts` around lines 740 - 756, Update the key-access grouping logic around splitPotentials so distinct KAOs from the same KAS are preserved by indexing entries with a URL-plus-KID identity (or equivalent unique key), rather than treating matching kao.url values as duplicates. Remove the unconditional duplicate-URL InvalidFileError for different KIDs while retaining duplicate detection for truly identical key-access entries.lib/tdf3/src/crypto/core/keys.ts (1)
16-27: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winExclude ML-KEM from the generic WebCrypto wrappers.
KeyAlgorithmnow includesmlkem:*, so these signatures permit constructing an ML-KEM-branded key backed by aCryptoKeyand missingmlKemLevel. Such a key later reaches ML-KEM code as an invalid runtime representation.Proposed type constraint
+type WebCryptoKeyAlgorithm = EcKeyAlgorithm | RsaKeyAlgorithm; + -export function wrapPublicKey(key: CryptoKey, algorithm: KeyAlgorithm): PublicKey { +export function wrapPublicKey(key: CryptoKey, algorithm: WebCryptoKeyAlgorithm): PublicKey { @@ -export function wrapPrivateKey(key: CryptoKey, algorithm: KeyAlgorithm): PrivateKey { +export function wrapPrivateKey(key: CryptoKey, algorithm: WebCryptoKeyAlgorithm): PrivateKey {Also applies to: 34-45
🤖 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 `@lib/tdf3/src/crypto/core/keys.ts` around lines 16 - 27, Restrict wrapPublicKey and the corresponding private-key wrapper to non-ML-KEM KeyAlgorithm values so mlkem:* algorithms cannot produce generic CryptoKey-backed wrappers missing mlKemLevel. Preserve the existing RSA and EC metadata handling, and leave ML-KEM construction to its dedicated representation.
🤖 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 `@lib/tdf3/src/client/index.ts`:
- Around line 733-737: Update the wrapping-key algorithm validation around
wrappingKeyAlgorithm and algorithm so an explicitly requested mismatch is
rejected rather than merely logged. When the fetched PEM algorithm differs from
the requested wrappingKeyAlgorithm, stop the encryption flow by throwing or
returning the established validation error, while preserving behavior when no
algorithm was explicitly requested or the algorithms match.
In `@lib/tdf3/src/crypto/declarations.ts`:
- Around line 499-506: Update the hkdfDerive documentation in the declarations
interface to remove the claim that it derives an AES-256-GCM key from an ML-KEM
shared secret. Keep the API description focused on generic HKDF derivation and
its parameters without documenting HKDF as part of the ML-KEM wrapping flow.
---
Outside diff comments:
In `@lib/tdf3/src/crypto/core/keys.ts`:
- Around line 16-27: Restrict wrapPublicKey and the corresponding private-key
wrapper to non-ML-KEM KeyAlgorithm values so mlkem:* algorithms cannot produce
generic CryptoKey-backed wrappers missing mlKemLevel. Preserve the existing RSA
and EC metadata handling, and leave ML-KEM construction to its dedicated
representation.
In `@lib/tdf3/src/tdf.ts`:
- Around line 740-756: Update the key-access grouping logic around
splitPotentials so distinct KAOs from the same KAS are preserved by indexing
entries with a URL-plus-KID identity (or equivalent unique key), rather than
treating matching kao.url values as duplicates. Remove the unconditional
duplicate-URL InvalidFileError for different KIDs while retaining duplicate
detection for truly identical key-access entries.
🪄 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: f5b19322-dfcd-4ba2-be5d-f395b63f68e3
⛔ Files ignored due to path filters (3)
cli/package-lock.jsonis excluded by!**/package-lock.jsonlib/package-lock.jsonis excluded by!**/package-lock.jsonweb-app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.github/workflows/roundtrip/init-temp-keys.sh.github/workflows/roundtrip/opentdf.yamlCLAUDE.mdcli/src/cli.tslib/package.jsonlib/src/access.tslib/src/auth/dpop.tslib/src/crypto/pemPublicToCrypto.tslib/src/opentdf.tslib/tdf3/src/client/index.tslib/tdf3/src/crypto/core/key-format.tslib/tdf3/src/crypto/core/keys.tslib/tdf3/src/crypto/core/mlkem-asn1.tslib/tdf3/src/crypto/core/mlkem.tslib/tdf3/src/crypto/declarations.tslib/tdf3/src/crypto/index.tslib/tdf3/src/models/key-access.tslib/tdf3/src/tdf.tslib/tests/mocha/encrypt-decrypt.spec.tslib/tests/mocha/unit/crypto-di.spec.tslib/tests/mocha/unit/crypto/mlkem.spec.tslib/tests/mocha/unit/mlkem-key-access.spec.tslib/tests/mocha/unit/tdf.spec.tslib/tests/server.tsspec/DSPX-3229.mdweb-app/src/App.tsxweb-app/tests/tests/roundtrip.spec.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- CLAUDE.md
- lib/src/opentdf.ts
- lib/src/crypto/pemPublicToCrypto.ts
- .github/workflows/roundtrip/opentdf.yaml
- lib/tdf3/src/crypto/index.ts
- .github/workflows/roundtrip/init-temp-keys.sh
- spec/DSPX-3229.md
- lib/tests/mocha/unit/mlkem-key-access.spec.ts
- cli/src/cli.ts
- lib/tdf3/src/crypto/core/mlkem.ts
- lib/src/access.ts
- lib/tests/mocha/unit/crypto-di.spec.ts
- lib/tests/mocha/unit/crypto/mlkem.spec.ts
- lib/tdf3/src/models/key-access.ts
- web-app/tests/tests/roundtrip.spec.ts
- lib/tests/server.ts
- lib/tdf3/src/crypto/core/key-format.ts
- lib/tdf3/src/crypto/core/mlkem-asn1.ts
Comment on lines
+499
to
+506
| /** | ||
| * Derive a symmetric key from input key material using HKDF. | ||
| * Used to derive an AES-256-GCM key from an ML-KEM shared secret. | ||
| * @param ikm - Input key material (e.g., ML-KEM shared secret) | ||
| * @param params - HKDF parameters (hash, salt, optional info/keyLength) | ||
| * @returns Opaque 256-bit AES symmetric key | ||
| */ | ||
| hkdfDerive: (ikm: SymmetricKey, params: HkdfParams) => Promise<SymmetricKey>; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not document HKDF as part of the ML-KEM wrapping flow.
The TDF implementation uses the raw 32-byte ML-KEM shared secret directly as the AES-256 key. This comment describes a different, incompatible derivation contract.
Proposed documentation fix
/**
* Derive a symmetric key from input key material using HKDF.
- * Used to derive an AES-256-GCM key from an ML-KEM shared secret.
+ * ML-KEM key wrapping does not use this method; it uses the raw
+ * 32-byte shared secret directly as the AES-256-GCM key.📝 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.
Suggested change
| /** | |
| * Derive a symmetric key from input key material using HKDF. | |
| * Used to derive an AES-256-GCM key from an ML-KEM shared secret. | |
| * @param ikm - Input key material (e.g., ML-KEM shared secret) | |
| * @param params - HKDF parameters (hash, salt, optional info/keyLength) | |
| * @returns Opaque 256-bit AES symmetric key | |
| */ | |
| hkdfDerive: (ikm: SymmetricKey, params: HkdfParams) => Promise<SymmetricKey>; | |
| /** | |
| * Derive a symmetric key from input key material using HKDF. | |
| * ML-KEM key wrapping does not use this method; it uses the raw | |
| * 32-byte shared secret directly as the AES-256-GCM key. | |
| * `@param` ikm - Input key material (e.g., ML-KEM shared secret) | |
| * `@param` params - HKDF parameters (hash, salt, optional info/keyLength) | |
| * `@returns` Opaque 256-bit AES symmetric key | |
| */ | |
| hkdfDerive: (ikm: SymmetricKey, params: HkdfParams) => Promise<SymmetricKey>; |
🤖 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 `@lib/tdf3/src/crypto/declarations.ts` around lines 499 - 506, Update the
hkdfDerive documentation in the declarations interface to remove the claim that
it derives an AES-256-GCM key from an ML-KEM shared secret. Keep the API
description focused on generic HKDF derivation and its parameters without
documenting HKDF as part of the ML-KEM wrapping flow.
…3229) Adds ML-KEM (Kyber, NIST FIPS 203) key access objects and rewrap support on top of the key-algorithm refactor: - PEM-ify ML-KEM keys and surface JWA names (ML-KEM-768+A192KW, ML-KEM-1024+A256KW) - require kid for ML-KEM key access - platform-interoperable key wrap; ASN.1/SPKI structure and length validation - drop ML-KEM-512 support (mlkem:768 / mlkem:1024 only) - roundtrip CI: provision ML-KEM keys via platform keygen
dmihalcik-virtru
force-pushed
the
DSPX-3229-claude-version
branch
from
July 17, 2026 19:13
5970bff to
19f18bd
Compare
X-Test Failure Report |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/tdf3/src/crypto/core/keys.ts (1)
22-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPrevent the CryptoKey wrappers from accepting ML-KEM algorithms.
After expanding
KeyAlgorithm, these functions acceptmlkem:*but produce keys withoutmlKemLeveland with aCryptoKeyinstead of raw ML-KEM bytes. Restrict their parameters to the RSA/EC subtypes.Proposed type-safe fix
import { ecAlgorithmToCurve, isEcKeyAlgorithm, isRsaKeyAlgorithm, - type KeyAlgorithm, + type EcKeyAlgorithm, type PrivateKey, type PublicKey, + type RsaKeyAlgorithm, rsaAlgorithmToModulusBits, } from '../declarations'; +type WebCryptoKeyAlgorithm = EcKeyAlgorithm | RsaKeyAlgorithm; + -export function wrapPublicKey(key: CryptoKey, algorithm: KeyAlgorithm): PublicKey { +export function wrapPublicKey(key: CryptoKey, algorithm: WebCryptoKeyAlgorithm): PublicKey { ... -export function wrapPrivateKey(key: CryptoKey, algorithm: KeyAlgorithm): PrivateKey { +export function wrapPrivateKey(key: CryptoKey, algorithm: WebCryptoKeyAlgorithm): PrivateKey {Also applies to: 40-43
🤖 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 `@lib/tdf3/src/crypto/core/keys.ts` around lines 22 - 25, Restrict the parameters of the CryptoKey wrapper functions containing the RSA/EC algorithm branching to the RSA/EC key-algorithm subtypes, excluding ML-KEM algorithms. Preserve the existing modulusBits and curve handling, and ensure ML-KEM inputs cannot produce CryptoKey wrappers missing mlKemLevel.
🤖 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 `@lib/tests/server.ts`:
- Around line 40-42: Update BASE_KEY_ALG configuration handling and the related
key-selection logic to explicitly recognize every supported algorithm, including
mlkem:768 and the EC default, while rejecting rsa:2048, typos, and other
unsupported values with a clear failure. Do not fall back to EC for invalid
configuration; ensure the failure occurs before roundtrip tests run.
---
Outside diff comments:
In `@lib/tdf3/src/crypto/core/keys.ts`:
- Around line 22-25: Restrict the parameters of the CryptoKey wrapper functions
containing the RSA/EC algorithm branching to the RSA/EC key-algorithm subtypes,
excluding ML-KEM algorithms. Preserve the existing modulusBits and curve
handling, and ensure ML-KEM inputs cannot produce CryptoKey wrappers missing
mlKemLevel.
🪄 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: 5a3a0936-3aa6-4c3b-bf30-862a69cacbf8
⛔ Files ignored due to path filters (3)
cli/package-lock.jsonis excluded by!**/package-lock.jsonlib/package-lock.jsonis excluded by!**/package-lock.jsonweb-app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
.github/workflows/roundtrip/init-temp-keys.sh.github/workflows/roundtrip/opentdf.yamlCLAUDE.mdlib/package.jsonlib/src/access.tslib/src/crypto/pemPublicToCrypto.tslib/tdf3/src/client/index.tslib/tdf3/src/crypto/core/key-format.tslib/tdf3/src/crypto/core/keys.tslib/tdf3/src/crypto/core/mlkem-asn1.tslib/tdf3/src/crypto/core/mlkem.tslib/tdf3/src/crypto/declarations.tslib/tdf3/src/crypto/index.tslib/tdf3/src/models/key-access.tslib/tdf3/src/tdf.tslib/tests/mocha/encrypt-decrypt.spec.tslib/tests/mocha/unit/crypto-di.spec.tslib/tests/mocha/unit/crypto/mlkem.spec.tslib/tests/mocha/unit/mlkem-key-access.spec.tslib/tests/mocha/unit/tdf.spec.tslib/tests/server.tsspec/DSPX-3229.mdweb-app/src/App.tsxweb-app/tests/tests/roundtrip.spec.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- CLAUDE.md
- .github/workflows/roundtrip/init-temp-keys.sh
- lib/src/crypto/pemPublicToCrypto.ts
- .github/workflows/roundtrip/opentdf.yaml
- spec/DSPX-3229.md
- lib/tdf3/src/client/index.ts
- lib/tests/mocha/unit/mlkem-key-access.spec.ts
- web-app/tests/tests/roundtrip.spec.ts
- lib/tdf3/src/crypto/index.ts
- web-app/src/App.tsx
- lib/tests/mocha/unit/crypto/mlkem.spec.ts
- lib/tests/mocha/unit/tdf.spec.ts
- lib/tdf3/src/crypto/core/mlkem.ts
- lib/tests/mocha/encrypt-decrypt.spec.ts
- lib/tdf3/src/tdf.ts
- lib/src/access.ts
- lib/tdf3/src/crypto/core/key-format.ts
- lib/tdf3/src/models/key-access.ts
Comment on lines
+40
to
+42
| // Allow tests to flip the WellKnown base key by setting BASE_KEY_ALG=mlkem:768 | ||
| // (or any other supported algorithm) before importing this module. Default stays EC. | ||
| const BASE_KEY_ALG = process.env.BASE_KEY_ALG || 'ec:secp256r1'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject unsupported BASE_KEY_ALG values instead of silently selecting EC.
BASE_KEY_ALG=rsa:2048 or a typo currently advertises an EC base key. Handle every supported algorithm explicitly and fail fast on invalid configuration so roundtrip tests cannot exercise the wrong key-wrap family.
Also applies to: 572-582
🤖 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 `@lib/tests/server.ts` around lines 40 - 42, Update BASE_KEY_ALG configuration
handling and the related key-selection logic to explicitly recognize every
supported algorithm, including mlkem:768 and the EC default, while rejecting
rsa:2048, typos, and other unsupported values with a clear failure. Do not fall
back to EC for invalid configuration; ensure the failure occurs before roundtrip
tests run.
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Reverts 908d9b1; WellKnown base-key selection is advisory. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/tdf3/src/tdf.ts (1)
933-946: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAvoid using
.bufferdirectly on slicedUint8Arrays.If
entityWrappedKeyis a Node.jsBufferor ifslice()returns a view instead of a copy,iv.bufferandwrappedKey.bufferwill return the underlyingArrayBufferfor the entire envelope. Passing this directly toBinary.fromArrayBuffer()will include the other fields, causing decryption to fail.To be perfectly safe against
TypedArray.bufferview hazards, slice the underlyingArrayBufferexplicitly (similar to howexportPublicKeyPemhandles it), or use a helper ifBinarysupportsUint8Arraydirectly.♻️ Proposed refactor
- const iv = encryptedDek.slice(0, 12); - const wrappedKey = encryptedDek.slice(12); + const ivBytes = encryptedDek.slice(0, 12); + const wrappedKeyBytes = encryptedDek.slice(12); const sharedSecret = await cryptoService.mlKemDecapsulate( ephemeralEncryptionKeys.privateKey, kemCiphertext ); const decryptResult = await cryptoService.decrypt( - Binary.fromArrayBuffer(wrappedKey.buffer), + Binary.fromArrayBuffer( + wrappedKeyBytes.buffer.slice( + wrappedKeyBytes.byteOffset, + wrappedKeyBytes.byteOffset + wrappedKeyBytes.byteLength + ) + ), sharedSecret, - Binary.fromArrayBuffer(iv.buffer), + Binary.fromArrayBuffer( + ivBytes.buffer.slice(ivBytes.byteOffset, ivBytes.byteOffset + ivBytes.byteLength) + ), Algorithms.AES_256_GCM );🤖 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 `@lib/tdf3/src/tdf.ts` around lines 933 - 946, Update the decrypt call in the ML-KEM decapsulation flow to avoid passing iv.buffer or wrappedKey.buffer directly from sliced Uint8Array/Buffer views. Create exact ArrayBuffer slices using each view’s byteOffset and byteLength, then pass those bounded buffers to Binary.fromArrayBuffer while preserving the existing IV and wrapped-key contents.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/tdf3/src/tdf.ts`:
- Around line 933-946: Update the decrypt call in the ML-KEM decapsulation flow
to avoid passing iv.buffer or wrappedKey.buffer directly from sliced
Uint8Array/Buffer views. Create exact ArrayBuffer slices using each view’s
byteOffset and byteLength, then pass those bounded buffers to
Binary.fromArrayBuffer while preserving the existing IV and wrapped-key
contents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47718359-5e6d-4be0-bdd9-668402a3f477
⛔ Files ignored due to path filters (3)
cli/package-lock.jsonis excluded by!**/package-lock.jsonlib/package-lock.jsonis excluded by!**/package-lock.jsonweb-app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
.github/workflows/roundtrip/init-temp-keys.sh.github/workflows/roundtrip/opentdf.yamlCLAUDE.mdlib/package.jsonlib/src/access.tslib/src/crypto/pemPublicToCrypto.tslib/tdf3/src/client/index.tslib/tdf3/src/crypto/core/key-format.tslib/tdf3/src/crypto/core/keys.tslib/tdf3/src/crypto/core/mlkem-asn1.tslib/tdf3/src/crypto/core/mlkem.tslib/tdf3/src/crypto/declarations.tslib/tdf3/src/crypto/index.tslib/tdf3/src/models/key-access.tslib/tdf3/src/tdf.tslib/tests/mocha/encrypt-decrypt.spec.tslib/tests/mocha/unit/crypto-di.spec.tslib/tests/mocha/unit/crypto/mlkem.spec.tslib/tests/mocha/unit/mlkem-key-access.spec.tslib/tests/mocha/unit/tdf.spec.tslib/tests/server.tsspec/DSPX-3229.mdweb-app/src/App.tsxweb-app/tests/tests/roundtrip.spec.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- CLAUDE.md
- lib/package.json
- spec/DSPX-3229.md
- lib/tests/mocha/unit/crypto/mlkem.spec.ts
- web-app/tests/tests/roundtrip.spec.ts
- lib/tests/mocha/unit/mlkem-key-access.spec.ts
- .github/workflows/roundtrip/opentdf.yaml
- lib/tests/mocha/unit/crypto-di.spec.ts
- web-app/src/App.tsx
- lib/src/access.ts
- lib/tdf3/src/crypto/index.ts
- .github/workflows/roundtrip/init-temp-keys.sh
- lib/tdf3/src/crypto/core/mlkem.ts
- lib/tests/mocha/encrypt-decrypt.spec.ts
- lib/tdf3/src/client/index.ts
- lib/tdf3/src/models/key-access.ts
- lib/tdf3/src/crypto/core/mlkem-asn1.ts
- lib/tests/server.ts
- lib/tdf3/src/crypto/declarations.ts
| 'try encrypting some of your own files' | ||
| ); | ||
|
|
||
| // Manifest inspector should display the expected ML-KEM kid (mlkem512/768/1024) |
Member
There was a problem hiding this comment.
nit: no 512 level in this implementation
Member
There was a problem hiding this comment.
Add at least one FIPS 203 KAT for encaps/decaps
pflynn-virtru
approved these changes
Jul 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@noble/post-quantum.MlKemWrappedKAO class, crypto core (encapsulate/decapsulate), encrypt + rewrap dispatch, PEM SPKI handling for ML-KEM public keys, and a mocha test server with ML-KEM re-encapsulation.Format
wrappedKey = base64(DER(kemEnvelope { [0] kemCiphertext, [1] encryptedDek }))whereencryptedDek = nonce(12) || aes-256-gcm ct || tag(16).lib/ocrypto): the raw 32-byte ML-KEM shared secret is used directly as the AES-256-GCM key — no HKDF, nogetZtdfSalt.id-alg-ml-kem-{768,1024}; RSA/EC unchanged.type: "mlkem-wrapped"(distinct from RSA's"wrapped"and EC's"ec-wrapped"; the platform Go KAS and java-sdk discriminate ML-KEM rewrap on this value),alg: mlkem:{768,1024},kidrequired, noephemeralPublicKeyfield.rsa:2048until KAS rollout.Test plan
cd lib && npm test— unit tests (incl.mlkem-key-access.spec.ts) + encrypt/decrypt matrix (2 ML-KEM levels × {ec, rsa, mlkem})cd lib && npm run build— type-check passesmake lint/make license-check—@noble/post-quantumis MITcd web-app && npm run build— sample app still builds with the new dropdown and manifest inspectorcd web-app/tests && npm test— Playwright roundtrips formlkem:768/1024, asserting KAOtypeismlkem-wrappedandkidmatches the requested variant (requires KAS-side ML-KEM support)mlkem:768selected; verify the manifestwrappedKeydecodes viadecodeKemEnvelopeDerto a 1088-byte KEM ciphertext + a 60-byteencryptedDek; decrypt round-trips.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation