Skip to content

fix(sdk): read the TDF spec version from all three places - #411

Open
pflynn-virtru wants to merge 3 commits into
mainfrom
fix/read-manifest-spec-version
Open

pflynn-virtru wants to merge 3 commits into
mainfrom
fix/read-manifest-spec-version

Conversation

@pflynn-virtru

@pflynn-virtru pflynn-virtru commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

Some TDF files record their spec version as tdf_spec_version instead of schemaVersion, either at the manifest root or under payload. This SDK only reads schemaVersion, so those files are read as legacy (hex digests) and fail to decrypt.

Change: read the version from schemaVersion, then root tdf_spec_version, then payload.tdf_spec_version. Non-string values like null are skipped. The writer still emits schemaVersion only.

Reader-only, backwards compatible. Counterparts: opentdf/platform#4060 (Go), opentdf/web-sdk#1054.

Testing: unit tests for each placement and precedence; end-to-end decrypt with the version under each name. mvn verify passes.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • The SDK now recognizes manifest versions stored in either the root or payload, while preserving an existing schemaVersion when present.
    • Manifests with missing or null segment sizes now use the corresponding integrity-information defaults; explicit values, including zero, are retained.
    • Reading alternate version fields and serializing a manifest retains the version under the canonical schemaVersion field.

pflynn-virtru and others added 2 commits September 30, 2026 15:10
Adds failing tests for a manifest whose spec version is recorded under
the non-aligned tdf_spec_version name (at the root or under payload),
for null and non-string values, for precedence, and for the writer
emitting schemaVersion only. Also covers files whose digest encoding
disagrees with what the version field implies, and that tampering is
still caught in each case.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
…rusting it

Manifest parsing now resolves the spec version in precedence order:
schemaVersion, then tdf_spec_version at the manifest root, then
tdf_spec_version under payload. Only non-empty JSON strings count;
null and other non-string values are skipped without failing the
decode. The writer is unchanged and emits schemaVersion only, so a
round trip normalizes the non-aligned name.

The reader no longer uses the version to choose between raw and hex
integrity digests. It computes the raw digest and accepts the recorded
value if it is base64 of either the raw bytes or their hex, for segment
hashes and the root signature; for assertion signatures both
aggregateHash||hash candidates are built. The version field is
unauthenticated and only tracked the encoding because this SDK's writer
set both from one boolean. Hex is an invertible encoding of the same
HMAC, so accepting both weakens nothing.

Counterpart of opentdf/platform#4060.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 6698c664-fc61-40b0-9251-49155aa94555

📥 Commits

Reviewing files that changed from the base of the PR and between 8e6f8bc and a7fef09.

📒 Files selected for processing (3)
  • sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java
  • sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java
  • sdk/src/test/java/io/opentdf/platform/sdk/TDFRootSignatureTest.java

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Manifest deserialization now falls back to non-empty tdf_spec_version values at the manifest root or under payload when schemaVersion is absent or empty. Serialization continues to emit schemaVersion. Tests cover lookup precedence, round trips, decryption, and tampering.

Changes

Manifest version compatibility

Layer / File(s) Summary
Manifest version lookup and serialization
sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java, sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java
The Gson adapter reads tdf_spec_version from the root first, then from payload, when schemaVersion is absent or empty. Tests cover precedence, ignored values, retained manifest data, and serialization using only schemaVersion.
Decryption and integrity checks
sdk/src/test/java/io/opentdf/platform/sdk/TDFRootSignatureTest.java
Tests cover decryption with alternate version-field locations, legacy 4.2.2 decryption, and rejection of segment-hash, root-signature, and payload tampering.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: dmihalcik-virtru

Merge Risk: ⚪ Minimal · up to a7fef

The change lets the SDK read the spec version from additional manifest locations while still writing the canonical field. No merge-blocking issue was found.

Security Architecture Review

Security architecture risk: 🔵 Low · up to a7fef

The compatibility change preserves existing integrity checks and canonical output. No introduced verification bypass was identified. The risk remains bounded, but this assessment does not establish the security of every supported payload mode or deployment.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed input surface is the manifest of a TDF supplied to the SDK reader. Alternate version values affect the interpretation of that artifact's integrity values, not key-access addresses or policy contents. The reviewed delta introduces no additional authority, tenant selector, or cross-service destination; actual deployment and tenant exposure were not established.

Security Findings and Attack Paths

  • observed — The existing unencrypted-payload branch uses unkeyed SHA-256 comparisons and emits segment bytes without decryption. Its manifest-controlled selector and behavior predate this PR, and neither branch consults the newly recognized version fields. This is an unchanged architecture condition, not an introduced or worsened concern from the fallback.

Trust Boundaries and Controls

  • observed — Alternate version fields remain untrusted manifest metadata. Root-signature comparison precedes Reader construction, and each encrypted segment must pass its integrity comparison and AES-GCM decryption before output. Assertions retain their existing verification policy, including the caller-configured disable option. Recognizing an alternate field does not remove these controls.

Resilience and Maintainability Implications

  • inferred — Repeated parsing creates independent normalized manifests, avoiding cross-artifact state contamination. Failure containment remains per load and per verified segment rather than an atomic whole-output guarantee; callers must not interpret a failed streaming read as successful completion.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: the SDK now reads the TDF spec version from all three supported locations.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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

A rabbit reads the version line,
At root, then payload, neat and fine.
The canonical field returns to view,
While tests check tampering through.
Hop, the manifests pass the test!

Comment @coderabbitai help to get the list of available commands.

… as-is

Drop the change that accepted either digest encoding regardless of the
recorded version. This PR now only widens where the version is read from:
root schemaVersion, then root tdf_spec_version, then
payload.tdf_spec_version. The resolved version still selects hex vs raw
digests, as before.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

X-Test Failure Report

@pflynn-virtru pflynn-virtru changed the title fix(sdk): read the TDF spec version from all three places, and stop trusting it fix(sdk): read the TDF spec version from all three places Sep 30, 2026
@sonarqubecloud

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown
Contributor

@pflynn-virtru

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant