chore(sdk): Lets CreateTDF use ChunkedWriter - #3946
Merged
Merged
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 21 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughTDF creation now uses the chunked writer for segment encryption and manifest finalization. Key-access resolution builds policy strings and key access objects from DEK shares. The writer supports a preset DEK and configurable segment sizing. ChangesTDF creation flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SDK as SDK.CreateTDF
participant Resolver as resolveKeyAccess
participant Writer as chunkedWriter
participant Output as TDF output
SDK->>Resolver: Resolve DEK shares and key access
SDK->>Writer: WriteSegment for each payload segment
Writer->>Output: Return encrypted segment data
SDK->>Writer: Finalize with TDF options
Writer->>Output: Return manifest and final data
Suggested reviewers: Merge Risk: ⚪ Minimal · up to TDF creation now reuses the chunked writer for encryption and manifest building. No concrete regression was found in policy serialization, key access, segment sizing, or integrity settings, and the change appears ready to merge after normal CI. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. A rabbit checks the keys in rows Comment |
github-actions
Bot
added
comp:sdk
This was referenced Sep 1, 2026
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
dmihalcik-virtru
force-pushed
the
dspx-2604-17-createtdf-delegates
branch
from
September 1, 2026 03:30
02e414b to
f6a3cf2
Compare
dmihalcik-virtru
force-pushed
the
dspx-2604-base-17
branch
from
September 1, 2026 03:30
9693e0a to
74608c8
Compare
Contributor
X-Test Failure Report |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
dmihalcik-virtru
force-pushed
the
dspx-2604-base-17
branch
from
September 23, 2026 19:29
b5cb847 to
c4bf431
Compare
dmihalcik-virtru
force-pushed
the
dspx-2604-17-createtdf-delegates
branch
from
September 23, 2026 19:29
3d949d6 to
79e745d
Compare
Contributor
X-Test Failure Report |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
dmihalcik-virtru
force-pushed
the
dspx-2604-17-createtdf-delegates
branch
from
September 24, 2026 14:50
79e745d to
a5d2340
Compare
dmihalcik-virtru
changed the base branch from
dspx-2604-base-17
to
dspx-2604-15-delegate-writer
September 24, 2026 14:51
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
CreateTDFContext and the chunked writer had grown two full implementations of
the same thing: build a policy, split the DEK, wrap each share to its KAS,
encrypt segments, accumulate an aggregate hash, sign assertions, emit a
manifest. Two copies of TDF construction is one too many — every spec change
has to land twice, and the second copy is the one that gets forgotten.
CreateTDF now delegates. It keeps the parts that are genuinely its own — the
KAO template, autoconfigure, the input-size resolution and read loop from the
previous commit — and hands each segment to the chunked writer, which owns
manifest assembly for both paths from here on.
The two paths differ in when key access is resolved, so that is what the
writer is now parameterized on. The chunked writer defers to a KeySplitter at
Finalize, because a caller may still be adding attributes while segments are
in flight. CreateTDF cannot: it knows its attributes up front and wants an
unreachable KAS to fail the call before a single payload byte reaches the
output writer. Both are expressed as a keyAccessResolver, with the DEK now
injectable so CreateTDF can wrap it ahead of time and hand the writer a
staticKeyAccess.
Everything downstream of that split is shared: resolvePolicyAndKeyAccess and
buildKeyAccessObjects replace prepareManifest's inline loop and the chunked
writer's buildChunkedPolicy/buildChunkedKeyAccessObjects, so both paths now
emit byte-identical policy and key access for the same attributes. The
splitter-contract checks move with them into splitterKeyAccess.resolve, which
is now the single place a third-party KeySplitter's output is validated and
verified to XOR back to the DEK before anything is wrapped.
Two things fall out of the unification, both moving the chunked writer onto
the shipped classic behavior:
- With zero attributes the policy body's "dataAttributes" and "dissem" are
now null rather than []. createPolicyObjectFromFQNs initializes them
inside the attribute loop; the deleted buildChunkedPolicy did so
unconditionally. The classic path has always emitted null here.
- A KAS named by a split but missing a public key is still rejected rather
than skipped (the check moved into buildKeyAccessObjects), but the error
now names the missing PEM rather than the absent map entry — the two cases
were indistinguishable in practice and only the outcome matters.
The writer also gained an explicit segment size. It previously reported the
first segment's actual length as defaultSegmentSize, which is only correct
when every segment is full; a single-segment TDF would advertise a short
default. CreateTDF knows the configured size and now says so.
TDFObject loses aesGcm and payloadKey, which only ever existed to carry state
between prepareManifest and the encrypt loop.
Deliberately not in this commit: removing enableEncryption, tdfFormat, and
readActionName, which are dead but unrelated; they are a separate cleanup.
Testing: the existing TDFSuite round trips pin byte-level output across
integrity algorithms, segment sizes, target modes, and multi-KAS splits, and
pass unchanged. Also verified against the streaming-input and input-size
coverage added in the previous commit, the experimental chunked writer suite,
and cross-module builds of examples, otdfctl, service, and tests-bdd.
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
wip: resolve conflict
dmihalcik-virtru
force-pushed
the
dspx-2604-17-createtdf-delegates
branch
from
September 24, 2026 15:10
a5d2340 to
9318a77
Compare
dmihalcik-virtru
changed the base branch from
dspx-2604-15-delegate-writer
to
main
September 24, 2026 15:10
sujankota
previously approved these changes
Sep 24, 2026
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
dmihalcik-virtru
enabled auto-merge
September 24, 2026 15:25
dmihalcik-virtru
changed the title
Contributor
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
sujankota
approved these changes
Sep 24, 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.
Proposed Changes
CreateTDFContext and the chunked writer had grown two full implementations of
the same thing: build a policy, split the DEK, wrap each share to its KAS,
encrypt segments, accumulate an aggregate hash, sign assertions, emit a
manifest. Two copies of TDF construction is one too many — every spec change
has to land twice, and the second copy is the one that gets forgotten.
CreateTDF now delegates. It keeps the parts that are genuinely its own — the
KAO template, autoconfigure, the input-size resolution and read loop from the
previous commit — and hands each segment to the chunked writer, which owns
manifest assembly for both paths from here on.
The two paths differ in when key access is resolved, so that is what the
writer is now parameterized on. The chunked writer defers to a KeySplitter at
Finalize, because a caller may still be adding attributes while segments are
in flight. CreateTDF cannot: it knows its attributes up front and wants an
unreachable KAS to fail the call before a single payload byte reaches the
output writer. Both are expressed as a keyAccessResolver, with the DEK now
injectable so CreateTDF can wrap it ahead of time and hand the writer a
staticKeyAccess.
Everything downstream of that split is shared: resolvePolicyAndKeyAccess and
buildKeyAccessObjects replace prepareManifest's inline loop and the chunked
writer's buildChunkedPolicy/buildChunkedKeyAccessObjects, so both paths now
emit byte-identical policy and key access for the same attributes.
Two things fall out of the unification, both moving the chunked writer onto
the shipped classic behavior:
With zero attributes the policy body's "dataAttributes" and "dissem" are
now null rather than []. createPolicyObjectFromFQNs initializes them
inside the attribute loop; the deleted buildChunkedPolicy did so
unconditionally. The classic path has always emitted null here.
A KAS named by a split but missing a public key is still rejected rather
than skipped (the check moved into buildKeyAccessObjects), but the error
now names the missing PEM rather than the absent map entry — the two cases
were indistinguishable in practice and only the outcome matters.
The writer also gained an explicit segment size. It previously reported the
first segment's actual length as defaultSegmentSize, which is only correct
when every segment is full; a single-segment TDF would advertise a short
default. CreateTDF knows the configured size and now says so.
TDFObject loses aesGcm and payloadKey, which only ever existed to carry state
between prepareManifest and the encrypt loop.
Since #3940 (revised) fixed the chunked writer's root to HS256 and its
segments to GMAC, there is nothing to plumb through here: CreateTDF stops
passing TDFConfig.rootIntegrityAlg and segmentIntegrityAlg to the writer.
Those were already the only values the defaults could hold and no exported
option set either, so the manifest is unchanged.
Deliberately not in this commit: removing enableEncryption, tdfFormat,
readActionName, and the two now-unread integrity fields, which are dead but
unrelated; they are a separate cleanup (#3947).
Testing: the existing TDFSuite round trips pin byte-level output across
segment sizes, target modes, and multi-KAS splits, and pass unchanged. Also
verified against the streaming-input and input-size
coverage added in the previous commit, the experimental chunked writer suite,
and cross-module builds of examples, otdfctl, service, and tests-bdd.
Checklist
Testing Instructions
The existing
TDFSuiteround trips pin byte-level output across segmentsizes, target modes and multi-KAS splits, and pass unchanged — that is the
main assurance here.
This touches key access construction on the shipped path, so it wants a
cross-SDK run before merge. This is the branch to pin it to for the whole
writer-delegation half of the stack: xtest drives the Go side through
otdfctl→SDK.CreateTDF, and this is the first commit where that callreaches the chunked writer at all.
otdfctl-refmust name the branch, notmain— otherwise the CLI is builtagainst main's
sdk/and the run passes without executing any of this. Thejob label should read
go@dspx-2604-17-createtdf-delegates.The full DSPX-2604 stack — 20 PRs
mainmainmainmainmainmainmaindspx-2604-base-11= #3932 + #3934 + #3935dspx-2604-base-17= #3944 + #3945dspx-2604-base-19= #3947 + #3939Reviewable in parallel right now, since they sit directly on
mainand depend onnothing else: 01, 02, 04, 05, 06, 07, 08.
Why three PRs have a
dspx-2604-base-*base. A GitHub PR takes one base branch,but 11, 17 and 19 each build on more than one parent. The
base-*branches are emptymerge commits that exist only to join those parents so the PR diff shows exactly its
own change and nothing else. They contain no code, have no PR of their own, and go
away once their parents land — retarget the child onto
mainat that point.Wants a cross-SDK xtest run before merge: 15, 17 (and therefore 20). They touch
the KAS wire format. Dispatch it against 17 or 20, never 15 on its own: xtest drives
the Go side through
otdfctl->SDK.CreateTDF, and 17 is the first commit wherethat call reaches the rewritten writer. Set
otdfctl-refto the same branch asplatform-ref-- it defaults tomain, which builds the CLI against main'ssdk/and makes the run vacuous.
Red checks you may see are network flakes, not this stack. Four distinct ones hit
this batch and all clear on re-run:
golangci-lint config verifytiming out onhttps://golangci-lint.run/.../golangci.v2.8.jsonschema.json(fails the wholego (<module>)job and fail-fast cancels its siblings), the bats installer getting a 403,Docker Hub timing out on
keycloak/keycloak:26.4, andbufreporting "the serverhosted at that remote is unavailable" while the Java SDK generates sources. The
govulncheckstep also emits##[error]annotations against the go1.25.11 stdlib, butit is
continue-on-error: trueand never fails a job — 01 bumps the toolchain andclears those annotations.
Summary by CodeRabbit