refactor(sdk): de-duplicate ChunkedWriter onto existing sdk helpers by dmihalcik-virtru · Pull Request #3923 · opentdf/platform

dmihalcik-virtru · 2026-08-27T13:38:22Z

Purpose

Intermediate PR in the stack main ← #3782 ← **this** ← #3865 ← #3921.

#3782 introduced ChunkedWriter with its own private copies of crypto helpers that already
existed in sdk/tdf.go (same package) and in sdk/experimental/tdf/. This PR collapses that
three-way duplication before #3865 rewrites CreateTDF on top of ChunkedWriter — so that
rewrite has one implementation to build on instead of two to reconcile.

Duplication was located with dupl (AST/token-based, so it sees through the renames
wrapKeyWithEC → chunkedWrapKeyWithEC). jscpd reported only 1.05% exact clones; that gap
is itself the signal that this was a paraphrased copy rather than a literal one, and it's why
CI never flagged it (dupl is not in .golangci.yaml's enabled linters).

Commits

refactor(sdk): reuse tdf.go crypto helpers in ChunkedWriter
Deletes chunkedEncryptMetadata, chunkedWrapKeyWith{EC,KEM,RSA,PublicKey}, and
chunkedCreatePolicyBinding, routing key access construction through the existing
createKeyAccess / encryptMetadata / createPolicyBinding. Also hoists the policy binding
and metadata encryption out of the per-KAS loop, which had been re-encrypting identical
metadata once per URL in an OR-group.
refactor(sdk): point experimental/tdf manifest and assertion types at sdk
Replaces the manifest and assertion types in sdk/experimental/tdf with aliases onto their
sdk counterparts. Source-compatible — experimental/tdf is exported public API with
likely downstream consumers, so nothing is deleted from its surface. IntegrityAlgorithm
keeps its own defined type because sdk's is = int and cannot carry the String() method.
fix(sdk): support legacy hex signatures in ChunkedWriter — see below.

Bug fixes

EC key access was unusable. Copied verbatim from experimental/tdf/key_access.go,
ChunkedWriter emitted key type "eccWrapped" (the KAS only accepts "ec-wrapped",
rewrap.go:713) and XOR-wrapped the DEK with the HKDF output where the unwrap path expects
AES-GCM. EC-KAS TDFs from ChunkedWriter could not be rewrapped. It went unnoticed because
experimental/tdf/reader.go is a stub and chunked_test.go's fake KAS is RSA-only. Fixed in
both packages; TestChunkedECKeyAccess covers it and fails on the pre-fix code.

WithChunkedExcludeVersion produced unreadable TDFs. The option omitted schemaVersion —
which is exactly how a reader detects a pre-4.3.0 TDF and switches to expecting hex-then-base64
signatures — while both signature sites hardcoded isLegacyTDF: false. Every such TDF failed
root signature verification.

The two settings have to travel together, and useHex is consumed during WriteSegment, long
before a Finalize option is seen. So this adds WithChunkedTargetMode(mode string) at
construction, mirroring mainline WithTargetMode and reusing the package's existing
isLessThanSemver / hexSemverThreshold. WithChunkedExcludeVersion on its own now fails with
ErrChunkedVersionHexMismatch rather than emitting a TDF no reader can verify.

Legacy hex readers are still deployed, so the doubly-encoded form remains fully supported —
TestChunkedLegacyTargetMode round-trips a 4.2.2-mode TDF and asserts the root and every
segment signature decode to 64 bytes.

Testing

make lint — clean apart from the pre-existing SA1019 at sdk/kas_client_test.go:188
make test — passes across sdk, otdfctl, service
New: TestChunkedECKeyAccess, TestChunkedKAOShape, TestChunkedLegacyTargetMode,
TestChunkedCurrentTargetMode, TestChunkedExcludeVersionRequiresLegacyMode,
TestChunkedTargetModeInvalid
ChunkedWriter is only reachable end-to-end through refactor(sdk): rewrite CreateTDF on top of ChunkedWriter #3865 + fix(cli): stream encrypt and decrypt instead of buffering whole payload #3921, so cross-SDK xtest is run
against the tip of the stack.

Follow-ups (not in scope here)

Enable dupl for sdk/ in .golangci.yaml — it would have blocked the original PR.
SplitResult.KASPublicKeys could be map[string]KASInfo, deleting the exported
KASPublicKey entirely. That's a public-contract change to the new KeySplitter interface
and belongs in its own PR.
Invert the relationship fully: mark sdk/experimental/tdf deprecated in its godoc pointing at
the sdk equivalents.

ChunkedWriter shipped with its own copies of the key-wrapping, metadata-encryption and policy-binding helpers that sdk/tdf.go already provides in the same package. Delete the copies and delegate to createKeyAccess, which also picks up two fixes the copies were missing: - EC key access objects were emitted with type "eccWrapped", but the KAS dispatches on "ec-wrapped" (service/kas/access/rewrap.go), so those TDFs could never be rewrapped. - The EC path XOR'd the DEK with the HKDF output instead of AES-GCM encrypting it, which is what the KAS unwrap path expects. Neither was caught before because experimental/tdf/reader.go is a stub and the chunked tests' fake KAS is RSA-only. To make the delegation possible, extract createPolicyBinding and reuse integrityAlgorithmString in tdf.go so both paths share one implementation, and add KASPublicKey.toKASInfo to bridge the splitter's key descriptor to createKeyAccess. Also hoist the policy binding and metadata encryption out of the per-KAS loop: both key on the split share rather than the KAS, so an OR-group was re-encrypting identical metadata once per URL for nothing. buildChunkedPolicy is left alone on purpose; sharing createPolicyObject would flip zero-attribute policies from [] to null and the policy UUID from v4 to v1. Adds TestChunkedKAOShape and TestChunkedECKeyAccess, the latter unwrapping the EC-wrapped DEK exactly as the KAS does. Both fail before this change. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>

… sdk sdk/experimental/tdf carried its own copies of the manifest and assertion types, substantively identical to the sdk ones. Replace them with type aliases so the two packages are interchangeable at compile time, the implementation lives in one place, and downstream importers keep the exact same API. examples/cmd/benchmark_experimental.go, the only in-repo consumer, builds unchanged. Fix the EC bugs at their origin here too, so nothing is left behind for a later search to find: wrapKeyWithEC emitted "eccWrapped" rather than the "ec-wrapped" the KAS dispatches on, and XOR'd the DEK with the HKDF output instead of AES-GCM encrypting it. The existing EC tests asserted the wrong key type; they now assert the right one and additionally unwrap the DEK the way service/kas/access/rewrap.go does, which is what would have caught the envelope bug. Also replace the remaining wrap-scheme and protocol string literals with named constants. Not aliased, deliberately: - IntegrityAlgorithm: sdk's is itself an alias for int, so it cannot carry the String() method this package exports. - Policy/PolicyBody/PolicyAttribute: sdk's PolicyObject declares Body as an anonymous struct over an unexported element type, so there is nothing to alias to until those are named and exported in sdk. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>

WithChunkedExcludeVersion omitted schemaVersion from the manifest, but the segment and root signatures were computed with the hex flag hardcoded to false. Readers derive isLegacyTDF from the absence of schemaVersion alone (tdf.go:988), so the option produced a manifest that claimed to predate 4.3.0 while carrying 4.3.0-style singly-encoded signatures -- unverifiable by legacy readers and by this SDK's own reader. Add WithChunkedTargetMode, which reuses the existing isLessThanSemver / hexSemverThreshold pair from tdf.go and sets useHex and excludeVersion together, mirroring the mainline WithTargetMode. The flag lives on the writer rather than the finalize config because segment signatures are written during WriteSegment, before Finalize options are seen. Finalize now rejects the inconsistent combination with ErrChunkedVersionHexMismatch instead of emitting a TDF no reader can verify.

coderabbitai · 2026-08-27T13:38:33Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3897485c-8d8b-4368-b95f-e79a8405ed2c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

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

github-actions · 2026-08-27T13:44:22Z

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric	Value
Approved Decision Requests	1000
Denied Decision Requests	0
Total Time	261.583144ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric	Value
Approved Decision Requests	1000
Denied Decision Requests	0
Total Time	131.33333ms

Benchmark Statistics

Name	№ Requests	Avg Duration	Min Duration	Max Duration

Bulk Benchmark Results

Metric	Value
Total Decrypts	100
Successful Decrypts	100
Failed Decrypts	0
Total Time	446.623128ms
Throughput	223.90 requests/second

TDF3 Benchmark Results:

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	44.030254023s
Average Latency	439.704192ms
Throughput	113.56 requests/second

github-actions · 2026-08-27T13:46:12Z

⚠️ Govulncheck found vulnerabilities ⚠️

The following modules have known vulnerabilities:

examples
otdfctl
sdk
service
lib/ocrypto
lib/fixtures
tests-bdd

See the workflow run for details.

github-actions · 2026-08-27T13:53:07Z

dmihalcik-virtru · 2026-08-27T13:57:56Z

xtest cross-SDK run against the tip of the stack (DSPX-4499-streaming-codec, which includes #3782 → #3923 → #3865 → #3921): all green — https://github.com/opentdf/tests/actions/runs/33077938349

Per the plan, I grepped the job logs rather than trusting the check mark, since both fixes here are only reachable end-to-end through the stack.

EC key access (eccWrapped → ec-wrapped, XOR → AES-GCM) — ztdf-ecwrap ran and passed, not skipped, for all three decrypt SDKs:

PASSED test_tdfs.py::test_tdf_roundtrip[...-go@BR-ztdf-ecwrap]
PASSED test_tdfs.py::test_tdf_roundtrip[...-java@main-ztdf-ecwrap]
PASSED test_tdfs.py::test_tdf_roundtrip[...-js@main-ztdf-ecwrap]

plus the test_policytypes.py or/and/hierarchy cases under ztdf-ecwrap. These assert kao.type == "ec-wrapped" and round-trip through a real rewrap, so they exercise exactly the path that was broken.

Legacy hex / doubly-encoded signatures — test_tdf_spec_target_422 passed for all three pairs:

PASSED test_tdfs.py::test_tdf_spec_target_422[...-go@BR-...]
PASSED test_tdfs.py::test_tdf_spec_target_422[...-java@main-...]
PASSED test_tdfs.py::test_tdf_spec_target_422[...-js@main-...]

That test writes a 4.2.2-mode TDF from the go SDK and reads it back with java/js, which is the deployed-legacy-reader compatibility this PR is meant to preserve.

Worth noting for reviewers: the looks_like_422 branch inside test_tdf_roundtrip did not run, because select_target_version returns 4.3.0 whenever both SDKs report hexless (semver ≥ 0.4.39) — which every current java/js release does. So the manifest-shape assertions on legacy output come from test_tdf_spec_target_422 and from the new unit test TestChunkedLegacyTargetMode, not from the roundtrip matrix.

16 skips, none related: 12 DPoP (Keycloak realm not configured for DPoP; js SDK lacks dpop; platform 0.25.0 lacks dpop_nonce_challenge) and 4 obligations (java SDK gap). Totals: 88 + 68 + 7 passed across the three steps.

ec_tdf_enabled is confirmed on — otdf-local sets it for key-management KAS instances (otdf_local/services/kas.py:79), and the km1/km2 KAS instances started in this run.

dmihalcik-virtru added 3 commits August 27, 2026 09:25

dmihalcik-virtru requested review from a team as code owners August 27, 2026 13:38

github-actions Bot added the comp:sdk A software development kit, including library, for client applications and inter-service communicati label Aug 27, 2026

github-actions Bot added the size/l label Aug 27, 2026

dmihalcik-virtru requested review from ronelliott and strantalis August 27, 2026 14:04

ronelliott approved these changes Aug 27, 2026

View reviewed changes

dmihalcik-virtru merged commit f50c9fe into feat/DSPX-2604 Aug 27, 2026
44 checks passed

dmihalcik-virtru deleted the feat/DSPX-2604-dedupe branch August 27, 2026 19:11

Navigation Menu

Uh oh!

refactor(sdk): de-duplicate ChunkedWriter onto existing sdk helpers - #3923

refactor(sdk): de-duplicate ChunkedWriter onto existing sdk helpers#3923
dmihalcik-virtru merged 3 commits into
feat/DSPX-2604from
feat/DSPX-2604-dedupe

dmihalcik-virtru commented Aug 27, 2026

Uh oh!

coderabbitai Bot commented Aug 27, 2026

Review skipped

Uh oh!

github-actions Bot commented Aug 27, 2026

Benchmark authorization.GetDecisions Results:

Benchmark authorization.v2.GetMultiResourceDecision Results:

Benchmark Statistics

Bulk Benchmark Results

TDF3 Benchmark Results:

Uh oh!

github-actions Bot commented Aug 27, 2026

Uh oh!

github-actions Bot commented Aug 27, 2026

Uh oh!

dmihalcik-virtru commented Aug 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

dmihalcik-virtru commented Aug 27, 2026

Purpose

Commits

Bug fixes

Testing

Follow-ups (not in scope here)

Uh oh!

coderabbitai Bot commented Aug 27, 2026

Review skipped

Uh oh!

github-actions Bot commented Aug 27, 2026

Benchmark authorization.GetDecisions Results:

Benchmark authorization.v2.GetMultiResourceDecision Results:

Benchmark Statistics

Bulk Benchmark Results

TDF3 Benchmark Results:

Uh oh!

github-actions Bot commented Aug 27, 2026

⚠️ Govulncheck found vulnerabilities ⚠️

Uh oh!

github-actions Bot commented Aug 27, 2026

X-Test Results

Uh oh!

dmihalcik-virtru commented Aug 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants