fix(sdk): emit spec-compliant key access in experimental/tdf and delegate Writer - #3944
dmihalcik-virtru wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe experimental TDF writer now wraps ChangesExperimental TDF writer
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Merge Risk: 🔵 Low · up to The code is broadly mergeable, but the tests and public examples should more clearly protect and explain complete TDF assembly and sparse segment selection. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 hops through chunks of data Comment |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
7971d6c to
daa07d2
Compare
0c00bb2 to
a3447fc
Compare
X-Test Failure Report |
X-Test Failure Report |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
898ee17 to
4294327
Compare
2243a07 to
67cab04
Compare
X-Test Failure Report |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
67cab04 to
9de9059
Compare
4294327 to
525cf63
Compare
X-Test Failure Report |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
525cf63 to
b005207
Compare
9de9059 to
8127c71
Compare
X-Test Failure Report |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
…gate Writer
The experimental writer built its own key access objects, and for EC KAS
keys it built them wrong in three ways at once. It set keyType
`"eccWrapped"`, but `service/kas/access/rewrap.go` dispatches on the exact
string `"ec-wrapped"` and has no case for the other spelling. It derived
the wrapping key with HKDF and then XORed the DEK, where the spec and every
KAS expect AES-GCM under that derived key. And it omitted `schemaVersion`
from the KAO entirely. Any TDF this package produced against an EC KAS was
undecryptable, and nothing in the repo caught it because the package tested
its own output against its own expectations.
The fix is not a patch to that code but its deletion. `key_access.go` (-266)
goes away and `Writer` delegates to `sdk.NewChunkedWriter`, so key access
objects come from `sdk.createKeyAccess` -- the same code path
`SDK.CreateTDF` has always used and that the cross-SDK tests exercise. RSA,
EC, ML-KEM and hybrid wrapping now have exactly one implementation.
`key_access_test.go` (-652) goes with it; equivalent coverage against the
sdk functions landed earlier in this stack, so nothing is lost.
`writer.go` drops from 680 lines to ~292: `Writer` becomes its config plus
an inner `sdk.ChunkedWriter` and a `finalized` flag. Manifest assembly,
segment encryption, integrity hashing and assertion signing all move to the
one implementation. `manifest.go` sheds the `calculateSignature` copy and
the three constants that only its callers needed.
`keysplit_adapter.go` (+60) is why this is a delegation rather than a
rename. `sdk.DefaultKeySplitter` is single-KAS and ignores attributes;
`keysplit.XORSplitter` evaluates the full ABAC boolean expression and
XOR-splits the DEK across every KAS the resulting clauses require. The two
result shapes are field-identical, so the adapter is a straight copy. The
one structural mismatch is where the default KAS enters -- sdk passes it per
`Split` call, keysplit takes it at construction -- so the splitter is built
inside `Split`.
API changes callers will notice
`Finalize` now returns a single `*FinalizeResult` instead of
`(finalBytes, manifest, error)`. Error values are aliases of their sdk
counterparts rather than copies, so `errors.Is` matches under either name.
`WithSegments` no longer requires a contiguous prefix starting at 0.
Indices may be sparse -- a caller mapping fixed index blocks onto S3
multipart uploads writes gaps by construction -- but must still name written
segments in ascending order and may only drop from the end, because that is
the order the payload is laid out in.
`WithExcludeVersionFromManifest` is deprecated. It was always a no-op: the
manifest builder never read the flag. Omitting `schemaVersion` is how a
reader is told the TDF predates 4.3.0, and such a reader then expects
hex-then-base64 signatures, which are decided per segment at write time,
long before Finalize sees the option. `WithTargetMode` sets both together
and is the replacement.
This package's `"application/octet-stream"` MIME default is preserved
independently of the sdk default. `examples/cmd/benchmark_experimental.go`,
the only non-test consumer in the repo, compiles unchanged.
Because this changes the KAS wire format for EC keys, it wants a cross-SDK
run before merge:
gh workflow run xtest.yml --repo opentdf/tests --ref main \
-f platform-ref=<branch> -f otdfctl-ref=main -f java-ref=main -f js-ref=main
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
b005207 to
5b0fa96
Compare
8127c71 to
1abae3e
Compare
X-Test Failure Report |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
Proposed Changes
The experimental writer built its own key access objects, and for EC KAS
keys it built them wrong in three ways at once. It set keyType
"eccWrapped", butservice/kas/access/rewrap.godispatches on the exactstring
"ec-wrapped"and has no case for the other spelling. It derivedthe wrapping key with HKDF and then XORed the DEK, where the spec and every
KAS expect AES-GCM under that derived key. And it omitted
schemaVersionfrom the KAO entirely. Any TDF this package produced against an EC KAS was
undecryptable, and nothing in the repo caught it because the package tested
its own output against its own expectations.
The fix is not a patch to that code but its deletion.
key_access.go(-266)goes away and
Writerdelegates tosdk.NewChunkedWriter, so key accessobjects come from
sdk.createKeyAccess-- the same code pathSDK.CreateTDFhas always used and that the cross-SDK tests exercise. RSA,EC, ML-KEM and hybrid wrapping now have exactly one implementation.
key_access_test.go(-652) goes with it; equivalent coverage against thesdk functions landed earlier in this stack, so nothing is lost.
writer.godrops from 680 lines to ~292:Writerbecomes its config plusan inner
sdk.ChunkedWriterand afinalizedflag. Manifest assembly,segment encryption, integrity hashing and assertion signing all move to the
one implementation.
manifest.gosheds thecalculateSignaturecopy andthe three constants that only its callers needed.
keysplit_adapter.go(+60) is why this is a delegation rather than arename.
sdk.DefaultKeySplitteris single-KAS and ignores attributes;keysplit.XORSplitterevaluates the full ABAC boolean expression andXOR-splits the DEK across every KAS the resulting clauses require. The two
result shapes are field-identical, so the adapter is a straight copy. The
one structural mismatch is where the default KAS enters -- sdk passes it per
Splitcall, keysplit takes it at construction -- so the splitter is builtinside
Split.API changes callers will notice
Finalizenow returns a single*FinalizeResultinstead of(finalBytes, manifest, error). Error values are aliases of their sdkcounterparts rather than copies, so
errors.Ismatches under either name.WithSegmentsno longer requires a contiguous prefix starting at 0.Indices may be sparse -- a caller mapping fixed index blocks onto S3
multipart uploads writes gaps by construction -- but must still name written
segments in ascending order and may only drop from the end, because that is
the order the payload is laid out in.
WithExcludeVersionFromManifestis deprecated. It was always a no-op: themanifest builder never read the flag. Omitting
schemaVersionis how areader is told the TDF predates 4.3.0, and such a reader then expects
hex-then-base64 signatures, which are decided per segment at write time,
long before Finalize sees the option.
WithTargetModesets both togetherand is the replacement.
WithIntegrityAlgorithmandWithSegmentIntegrityAlgorithmno longer takeeffect, and asking for anything but the default is now an error from
NewWriterrather than a silent substitution. #3940 (revised) removed thecorresponding knobs from
sdk.ChunkedWriter, which emits an HS256 root andGMAC segments unconditionally; an
Optionreturns no error, soNewWriteris the only place a request the delegate cannot satisfy can be caught.
WithIntegrityAlgorithm(RootHS256)andWithSegmentIntegrityAlgorithm(SegmentGMAC)still succeed and are no-ops.That moves this package's segment default from
SegmentHS256toSegmentGMAC. The old default was never right here: HS256 segment hashesonly make sense over bytes that are not AEAD output, and every segment this
writer produces is AES-GCM ciphertext with a tag already computed over
exactly those bytes. A caller that took the default was paying for a second
MAC over the same data and writing a manifest that named it.
NewWriternow refuses
SegmentHS256outright withsdk.ErrUnsupportedSegmentIntegrityAlgorithm, andRootIntegrityAlg(GMAC)-- reachable by conversion even though the type names no such constant --
with
sdk.ErrUnsupportedRootIntegrityAlgorithm. See #4030 for why a GMACroot is unsound: the aggregate hash never passes through the AEAD, so the
"tag" is a copy of the last segment hash and forgeable with no key.
This package's
"application/octet-stream"MIME default is preservedindependently of the sdk default.
examples/cmd/benchmark_experimental.go,the only non-test consumer in the repo, compiles unchanged.
Because this changes the KAS wire format for EC keys, it wants a cross-SDK
run before merge — but not against this branch. xtest drives the Go side
through
otdfctl, which only reachesSDK.CreateTDF; nothing in the CLIimports
sdk/experimental/tdf, andCreateTDFdoes not delegate to thechunked writer until #3946. A run pinned here exercises none of this. Run it
against #3946 (or the stack tip), and set
otdfctl-refto the same branch —left at
mainthe CLI is built against main'ssdk/and the run passesvacuously:
gh workflow run xtest.yml --repo opentdf/tests --ref main
-f platform-ref=dspx-2604-17-createtdf-delegates
-f otdfctl-ref=dspx-2604-17-createtdf-delegates
-f java-ref=main -f js-ref=main
Checklist
Testing Instructions
This changes the KAS wire format for EC keys, so it wants a cross-SDK run
before merge. Pin it to #3946, not to this branch — that is the first point
where
CreateTDFgoes through the changed code, andotdfctlis the onlyGo consumer xtest drives:
otdfctl-refmust name the branch too: it defaults tomain, which buildsthe CLI against main's
sdk/and makes the run vacuous. Check the job labelreads
go@<branch>rather thango@main.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.