fix(sdk): allow the same KAS to wrap the same split (DSPX-3379)#967
fix(sdk): allow the same KAS to wrap the same split (DSPX-3379)#967dmihalcik-virtru wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChanges ChangesDuplicate KAO support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant TDF as tdf.ts unwrapKey
participant KAS
Client->>TDF: unwrapKey(manifest)
TDF->>TDF: splitLookupTableFactory builds splitId -> KAO[]
loop for each split id
TDF->>TDF: iterate potentials array, key by url#kid#index
TDF->>KAS: rewrap request per alternative
KAS-->>TDF: rewrap response
end
TDF-->>Client: unwrapped key
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request resolves DSPX-3379 by allowing the same Key Access Server (KAS) to wrap the same split multiple times. It refactors splitLookupTableFactory to return a list of alternative Key Access Objects (KAOs) instead of throwing on duplicates, and updates unwrapKey to handle these duplicates as distinct alternatives. Feedback on the changes highlights a bug in splitLookupTableFactory where accessibleSplits does not normalize undefined sid values to empty strings, which can bypass safety checks. Additionally, a formatting improvement for a stack trace in the markdown specification file was suggested.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ): Record<string, Record<string, KeyAccessObject>> { | ||
| ): Record<string, KeyAccessObject[]> { | ||
| const allowed = (k: KeyAccessObject) => allowedKases.allows(k.url); | ||
| const splitIds = new Set(keyAccess.map(({ sid }) => sid ?? '')); |
There was a problem hiding this comment.
There is a subtle bug/inconsistency in how accessibleSplits (on line 713) is computed compared to splitIds (on line 711).
splitIds normalizes sid to '' if it is undefined:
const splitIds = new Set(keyAccess.map(({ sid }) => sid ?? ''));However, accessibleSplits does not normalize sid:
const accessibleSplits = new Set(keyAccess.filter(allowed).map(({ sid }) => sid));This means accessibleSplits can contain undefined instead of ''. If keyAccess contains both an allowed KAO with sid: undefined and an allowed KAO with sid: '', accessibleSplits will have size 2 (containing both undefined and ''), while splitIds will have size 1 (containing only '').
If there is also a disallowed split (e.g., sid: 'split1'), splitIds will have size 2 ('' and 'split1'), and accessibleSplits will also have size 2 (undefined and ''). This causes the check splitIds.size > accessibleSplits.size to evaluate to false (2 > 2), bypassing the early safety check in splitLookupTableFactory and throwing a less descriptive error later in unwrapKey.
To fix this, accessibleSplits should also normalize sid using sid ?? '':
const accessibleSplits = new Set(keyAccess.filter(allowed).map(({ sid }) => sid ?? ''));| We should allow the same KAS to wrap the same split. This could indicate a mistake during creation (the same key used multiple times) or an intended behavior (different keys on the same KAS indicating different security profiles of life cycles). | ||
| Behavior: | ||
| When a KAO array has two elements with the same split id and the same kas uri, it fails with the error: | ||
| TODO: Fallback to no split ids. Repetition found for [https://virtru.gbr.dev.internal] on split [71ce30e4-cd90-4269-adf7-c06db931338f]TdfError: TODO: Fallback to no split ids. Repetition found for [https://virtru.gbr.dev.internal] on split [71ce30e4-cd90-4269-adf7-c06db931338f] at splitLookupTableFactory (https://virtru.gbr.dev.internal/secureviewer/assets/index-BySOd6k_.js:29440:13) at unwrapKey (https://virtru.gbr.dev.internal/secureviewer/assets/index-BySOd6k_.js:29453:27) at decryptStreamFrom (https://virtru.gbr.dev.internal/secureviewer/assets/index-BySOd6k_.js:29691:75) at ZTDFReader.decrypt (https://virtru.gbr.dev.internal/secureviewer/assets/index-BySOd6k_.js:30874:29)Expected behavior: |
There was a problem hiding this comment.
The stack trace on line 23 is currently formatted as a single long line, which makes it hard to read. It would be much cleaner to format it as a blockquote or a markdown code block.
For example:
Behavior:
When a KAO array has two elements with the same split id and the same kas uri, it fails with the error:
TdfError: TODO: Fallback to no split ids. Repetition found for [https://virtru.gbr.dev.internal] on split [71ce30e4-cd90-4269-adf7-c06db931338f]
at splitLookupTableFactory (https://virtru.gbr.dev.internal/secureviewer/assets/index-BySOd6k_.js:29440:13)
at unwrapKey (https://virtru.gbr.dev.internal/secureviewer/assets/index-BySOd6k_.js:29453:27)
at decryptStreamFrom (https://virtru.gbr.dev.internal/secureviewer/assets/index-BySOd6k_.js:29691:75)
at ZTDFReader.decrypt (https://virtru.gbr.dev.internal/secureviewer/assets/index-BySOd6k_.js:30874:29)
Expected behavior:
dmihalcik-virtru
force-pushed
the
DSPX-3379-multisplit-allowed
branch
from
0cc6e33 to
d370256
Compare
dmihalcik-virtru
changed the title
multisplit allowed Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
splitLookupTableFactory now returns split id -> KeyAccessObject[] (a disjunction of alternatives) instead of a url-keyed map, and no longer throws when a KAS repeats within a split. unwrapKey feeds each alternative to anyPool with a unique key so duplicate KAS entries are each tried until one succeeds. Matches go-sdk/java-sdk behavior; validated by xtest test_tdf_with_duplicate_kao_same_kas (opentdf/tests#555). Adds unit tests for duplicate + same-KAS-different-KID, an in-process decrypt-path test, and fills spec/DSPX-3379.md. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-3379-multisplit-allowed
branch
from
d370256 to
8fc59c3
Compare
|
X-Test Failure Report |
1 participant
What
When a TDF's key-access-object (KAO) array has two entries with the same split
id and the same KAS URI, the web-sdk refused to decrypt, throwing
Unable to decrypt: Multiple keys detected for Key Access Server [...].This treats the KAOs for a split as a disjunction of alternatives (any one
succeeding unwraps the split) instead of a URL-keyed map that can hold at most
one KAO per KAS — matching how the split is already consumed downstream
(
anyPool) and how go-sdk / java-sdk already behave.Changes (
lib/tdf3/src/tdf.ts)splitLookupTableFactorynow returnsRecord<splitId, KeyAccessObject[]>andno longer throws on a repeated
(splitId, kasUrl); every allowed KAO is keptas an alternative. Disallowed-KAS accounting (
UnsafeUrlError) is unchanged.unwrapKeyfeeds each alternative toanyPoolwith a uniqueurl#kid#indexkey so duplicate KAS entries are each tried until one succeeds.
Why (DSPX-3379)
The same KAS wrapping the same split is legitimate — an authoring mistake (same
key used twice) or intentional multiple keys on one KAS. go/java read these
files fine; the web-sdk (e.g. secure viewer) failed. Expected: decrypt (or
permission-denied) exactly as if the copies lived on distinct URIs.
Tests
splitLookupTableFactorykeeps duplicate(sid, kas)andsame-KAS-different-KID entries as alternatives.
splitPlanwith a repeatedkas+sidproduces aduplicate-KAO TDF that decrypts via the full
unwrap/anyPoolpath.test_tdf_with_duplicate_kao_same_kas(chore(xtest): cross-SDK test for duplicate KAO on same KAS+split (DSPX-3379) tests#555) — CI isgreen for go, java, and this branch's js.
Spec:
spec/DSPX-3379.md.Depends on / validated by: opentdf/tests#555
Summary by CodeRabbit
New Features
Bug Fixes
Tests