feat(sdk): read the off-spec tdf_spec_version manifest field - #4058
Closed
pflynn-virtru wants to merge 3 commits into
Closed
pflynn-virtru wants to merge 3 commits into
pflynn-virtru wants to merge 3 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
schemaVersion is the correct name for the TDF spec version field and is what this SDK writes. tdf_spec_version is not a former spelling that was renamed -- it is an error that leaked into some specification drafts and some older OpenTDF documentation, and into the writers built from them. Add Manifest.UnmarshalJSON so those files stay readable: when schemaVersion is absent, fall back to tdf_spec_version at the manifest root, then to tdf_spec_version under payload, where this SDK's own bundled schemas reproduce the mistake. schemaVersion always wins when both names are present. Nothing is written back under the off-spec name -- the default marshaller still emits schemaVersion only -- so the error is not propagated by anything that re-emits a manifest it read. The field is not put on a deprecation path, because it was never correct. Off-spec value types are tolerated rather than fatal: the lax schema permits a null tdf_spec_version, and reporting malformed manifests is schema validation's job, not the decoder's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
…fest sdk/experimental/tdf/manifest.go is a deliberate duplicate of sdk/manifest.go and must not drift. The experimental package has no manifest read path today, so this changes no behavior; it is here so that when one lands it agrees with the stable SDK on how tdf_spec_version is read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
The reader decided whether integrity digests were hex or raw by testing
whether the manifest carried a spec version:
isLegacyTDF := r.manifest.TDFVersion == ""
That only ever worked because our own writer sets useHex and
excludeVersionFromManifest from one boolean, making "field present" mean "raw
digests" for files we produced. Nothing authenticates the field, and writers
that decoupled the two -- including every writer that emitted the off-spec
tdf_spec_version name -- break the correspondence.
The consequence was that the spelling and value of an unauthenticated metadata
field decided whether a well-formed container could be read at all. Reading
tdf_spec_version, added earlier on this branch, made it worse: a hex-digest
container carrying that field would be verified as raw and rejected.
Replace the flag with digestMatchesRecorded, which compares a recomputed raw
digest against the manifest value in both spellings. This is the approach
taken in opentdf#3597 for the policy binding, where the two encodings are
distinguished by length so no version signal is needed. Accepting both
weakens nothing: hex is an invertible encoding of the same HMAC, so forging
either form still requires the payload key.
The assertion path cannot use length, since the hash is concatenated into a
larger buffer before signing and leaves no distinguishable trace. It builds
both candidates and accepts either, sound on the same grounds.
This also fixes a latent bug on the canonical path: schemaVersion "4.2.0" was
treated as non-legacy despite being below the hex threshold.
The writer is unchanged and still honors WithTargetMode; the bool those
helpers take is renamed useHex to say what it actually selects.
Test_SpecVersionDoesNotAffectVerification covers raw and hex containers
against every spelling of the field, its absence, and a value that
contradicts the digests. Four of its cases fail without this change.
Closes opentdf#4059
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
pflynn-virtru
force-pushed
the
feat/vis-199-manifest-spec-version-alias
branch
from
September 16, 2026 15:22
93be26e to
47f48ee
Compare
Member
Author
|
Superseded by #4060, which is the same work on a branch whose name carries no internal tracker reference. No review had happened here. |
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
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
schemaVersionis the correct name for the TDF spec version field, and is what this SDK writes.tdf_spec_versionis not a former spelling that was later renamed — it is an error that leaked into some specification drafts and some older OpenTDF documentation, and into the writers built from them. This PR makes the Go reader tolerate that error so the affected files stay usable. It does not put the name on a deprecation path, because it was never correct.Manifest.UnmarshalJSON. WhenschemaVersionis absent, fall back totdf_spec_versionat the manifest root, then totdf_spec_versionunderpayload. Both placements are real — writers disagree, and this SDK's own bundled schemas (sdk/schema/manifest*.schema.json) reproduce the mistake underpayloadwhile never declaring the rootschemaVersionit actually emits.schemaVersionalways wins when both names are present; between the two off-spec locations, root wins overpayload. No error on conflict.schemaVersiononly, so the error is never propagated — including by code that re-emits a manifest it just read. A regression test locks this in.tdf_spec_version: null, and reporting malformed manifests is schema validation's job, not the decoder's.sdk/experimental/tdf/manifest.gois a deliberate duplicate ofsdk/manifest.go, so it gets the same decoder. That package has no manifest read path today, so this changes no behavior there — it just keeps the two from drifting.The JSON schemas are deliberately untouched. Neither sets
additionalProperties: false, so both spellings already validate; correcting the schema/spec discrepancy is separate work, not this PR.The spec version no longer affects verification
Reading a second name for the field exposed a deeper problem, so this PR fixes it too.
The reader used to pick the integrity digest encoding from the field's presence:
isLegacyTDF := r.manifest.TDFVersion == ""atsdk/tdf.go:965,:1070,:1478,:1633. That only worked because our writer setsuseHexandexcludeVersionFromManifestfrom one boolean (sdk/tdf_config.go:310), so "field present" meant "raw digests" — for files we wrote. Nothing authenticates the field, and writers that decoupled the two (including every writer that emitted the off-spec name) break the correspondence. Readingtdf_spec_versionmade it worse: a hex-digest container carrying that field would have been verified as raw and rejected.digestMatchesRecordedreplaces the flag. It recomputes the raw digest and compares against the manifest value in both spellings — the same move #3597 makes for the policy binding, where the encodings are distinguished by length "so no version signal is needed". Accepting both weakens nothing: hex is an invertible encoding of the same HMAC, so forging either form still requires the payload key.The assertion path can't use length — the hash is concatenated into a larger buffer before signing, leaving no trace of its encoding — so it builds both candidates and accepts either, sound on the same grounds.
The writer is untouched and still honors
WithTargetMode; the bool those helpers take is renameduseHexto say what it selects.This also fixes a pre-existing bug on the canonical path:
schemaVersion: "4.2.0"was treated as non-legacy despite being below the hex threshold.Closes #4059.
Checklist
Test_OffSpecSpecVersionIsReadabledecrypts real containers throughCreateTDF/LoadTDFagainst the fake KAS backendManifest.UnmarshalJSON, and no public API changedTesting Instructions
Two things could not be verified locally, both pre-existing and unrelated to this change — each reproduced identically on a stashed clean tree:
make fmt/make lintabort before running: the installed golangci-lint 2.9.0 is built with go1.26 while the repo targets go1.27.1 (the Go language version (go1.26) used to build golangci-lint is lower than the targeted Go version (1.27.1)). Formatting was checked withgofmt -landgofumpt -linstead, both clean.make testfails inlib/fixturesand parts ofservice(TestTokenManager_*,service/integration,service/pkg/server,service/rttests) — these need thedocker composeKeycloak/Postgres stack, which was not running (dial tcp [::1]:8888: connect: connection refused).Cross-SDK interop via
opentdf/testsxtest has not been run yet; the java/js readers are the real counterparties here and it is worth a run before this leaves draft.🤖 Generated with Claude Code