fix(sdk): DSPX-4590 default per-segment sizes when a writer omits them - #3979
fix(sdk): DSPX-4590 default per-segment sizes when a writer omits them#3979dmihalcik-virtru wants to merge 2 commits 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: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
7be0388 to
407835f
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
407835f to
fad425f
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
fad425f to
128c23a
Compare
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:
|
d06d10d to
1d28ae5
Compare
445b9d5 to
77d87d8
Compare
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:
|
1d28ae5 to
9967bdc
Compare
77d87d8 to
2c686d3
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:
|
segmentSize and encryptedSegmentSize are optional per-segment overrides: manifest.schema.json requires only the integrityInformation defaults, and web-sdk omits the per-segment keys whenever they equal the default, so every web-sdk container over one segment failed to decrypt in go-sdk. Fall back to the manifest defaults in the payload-size computation, WriteTo and ReadAt. Rebased onto #3933 (map ReadAt plaintext offsets from cumulative segment sizes), which rewrote ReadAt's segment lookup from a uniform DefaultSegmentSize stride to a walk over each segment's actual plaintext/ciphertext size -- necessary for the non-uniform segments sdk/experimental/tdf can emit. Reconciling the two surfaced a further bug: resolveSegmentSizes treated a per-segment size of 0 as "omitted, use the default" independently for Size and EncryptedSize, but JSON can't distinguish an omitted key from an explicit 0, and go-sdk's own CreateTDF already writes segmentSize: 0 for the sole segment of an empty-payload TDF (no omitempty on the field). That made an empty TDF round-trip to the wrong payloadSize. resolveSegmentSizes now resolves EncryptedSize first -- its zero value is never ambiguous, since ciphertext can never legitimately be zero bytes -- and disambiguates a zero Size by comparing the resolved EncryptedSize against DefaultEncryptedSegSize rather than assuming Size and EncryptedSize are only ever omitted together. Checking go-sdk, java-sdk and web-sdk's actual manifest-writing source confirmed go-sdk and java-sdk always set both fields together (so a joint-zero assumption happened to hold for them), but web-sdk's lib/tdf3/src/tdf.ts decides whether to omit segmentSize and encryptedSegmentSize with two independent equals-the-default comparisons, not one joint check -- so a joint-zero- only version would have mis-resolved a segment where only one of the two happened to be omitted. The corrected comparison needs no assumption about the cipher's per-segment overhead (nonce/tag size stays out of manifest.go entirely): the overhead is constant across every segment in one manifest, so if the resolved EncryptedSize equals its default, the plaintext size must too, regardless of what that overhead number actually is. Also gives calculateSignature's too-short-ciphertext-for-GMAC error (previously a bare, unclassified error) a proper ErrTampered-wrapped sentinel, consistent with the rest of this file's integrity failures. Verified against opentdf/tests' DSPX-4592-java-underflow branch (adds test_tdfs.py::test_chunky_roundtrip, a 5 MiB round-trip that forces a full-default-sized segment): with platform-ref and otdfctl-ref both pointed at this branch and XT_FORCE_SUPPORTS=chunky, js-encrypt -> go-decrypt passes (js omits per-segment sizes on the full-sized segment; go now defaults them back). The one remaining failure in that run, js-encrypt -> java-decrypt, is java-sdk's own pre-existing GMAC-on-empty- segment bug (DSPX-4589), unrelated to this change. Note on #3933 standalone: without this fix, #3933's cumulative-walk ReadAt uses seg.Size directly, so an omitted (0) per-segment size stalls the plaintext cursor and desyncs the ciphertext offset for every segment after it. Reading a web-sdk multi-segment file then fails with a misleading "tamper detected: failed integrity check on segment hash" instead of main's current (also broken, but at least consistent) "fail to create gmac signature". #3933 should not be merged or relied on standalone for real multi-segment interop until this lands on top of it. The zip64/ZIP64-conformance findings originally bundled with this change (findings 1-6 of the DSPX-4590 investigation) now live in a separate PR stacked on top of this one, since they are independent of the segment- size defaulting fixed here. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
9967bdc to
be2cc62
Compare
2c686d3 to
70e1d63
Compare
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:
|
|
Jira: https://virtru.atlassian.net/browse/DSPX-4590
Part 2 of a 3-PR stack: #3933 (base) <- this PR <- #3981 (zip64 conformance, stacked on top).
Based on #3933 (fix(sdk): map ReadAt plaintext offsets from cumulative
segment sizes) rather than
main, since both PRs rewroteReader.ReadAtfor independent reasons and needed to be reconciled rather than merged
separately.
Relationship to #3933
#3933 replaced
ReadAt's uniform-stride segment lookup(
offset / DefaultSegmentSize) with a cumulative walk over each segment'sactual declared size, so non-uniform segments (which
sdk/experimental/tdfcan emit) map to the correct plaintext offsets.
This PR fixes finding 7 of the DSPX-4590 investigation on top of that:
manifest.schema.jsonmakes per-segmentsegmentSize/encryptedSegmentSizeoptional (a writer may omit them whenever they equal the manifest-level
default), and web-sdk does exactly that. go-sdk's reader didn't default
them back, so every web-sdk container over one segment failed to decrypt.
IntegrityInformation.resolveSegmentSizesnow substitutes the manifestdefaults, wired into
LoadTDF'spayloadSizecomputation,WriteTo, andReadAt(using #3933's cumulative-walk structure, callingresolveSegmentSizesper segment instead of trustingseg.Size/seg.EncryptedSizedirectly).The empty/omitted per-segment size ambiguity
JSON can't distinguish an omitted key from an explicit
0. go-sdk's ownCreateTDFwritessegmentSize: 0(noomitempty) for the sole segment ofan empty-payload TDF, so an early version of this fix that treated any
0as "omitted" mis-resolved that case. Checking go-sdk, java-sdk and web-sdk's
actual manifest-writing source found:
Size/EncryptedSizetogether (neverindependently zero).
lib/tdf3/src/tdf.ts) decides whether to omit each field withtwo independent equals-the-default comparisons, not one joint check.
resolveSegmentSizesnow resolvesEncryptedSizefirst -- its zero valueis never ambiguous, since ciphertext can never legitimately be zero bytes --
and disambiguates a zero
Sizeby comparing the resolvedEncryptedSizeagainst
DefaultEncryptedSegSize, rather than assuming the two fields areonly ever omitted together. This needs no assumption about the cipher's
per-segment overhead (nonce/tag size stays out of
manifest.goentirely):the overhead is constant across every segment in one manifest, so if the
resolved
EncryptedSizeequals its default, the plaintext size must too,regardless of what that overhead number actually is.
GMAC failure classification
calculateSignature's too-short-ciphertext-for-GMAC path returned a bare,unclassified
errors.New(...), unlike every other integrity failure in thisfile. It now returns a new
ErrGMACSignatureFailed, wrapped inErrTamperedlike
ErrSegSizeMismatch/ErrSegSigValidation.A note on #3933 standalone
Without this fix, #3933's cumulative-walk
ReadAtusesseg.Sizedirectly,so an omitted (
0) per-segment size stalls the plaintext cursor and desyncsthe ciphertext offset for every segment after it. Reading a web-sdk
multi-segment file then fails with a misleading
tamper detected: failed integrity check on segment hashinstead ofmain's current (also broken,but at least consistent)
fail to create gmac signature. #3933 should notbe merged or relied on standalone for real multi-segment interop until
this lands on top of it.
Testing
cd sdk && go test ./... -racemake fmt,make lint(0 new issues)cd sdk && go test -run TestREADMECodeBlocksopentdf/tests'DSPX-4592-java-underflowbranch(
test_tdfs.py::test_chunky_roundtrip, a 5 MiB round-trip that forces afull-default-sized segment): with
platform-ref/otdfctl-refpointed atthis branch and
XT_FORCE_SUPPORTS=chunky, js-encrypt -> go-decryptpasses. The one remaining failure in that run, js-encrypt -> java-decrypt,
is java-sdk's own pre-existing GMAC-on-empty-segment bug (DSPX-4589),
unrelated to this change.
Supersedes #3967, which is left open, unmodified, for reference.