From 904aa02102314277d88a063be15e06b4c400ad7c Mon Sep 17 00:00:00 2001 From: Paul Flynn Date: Wed, 30 Sep 2026 15:10:10 -0400 Subject: [PATCH 1/3] test(sdk): cover reading the spec version from all three places 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) Signed-off-by: Paul Flynn --- .../io/opentdf/platform/sdk/ManifestTest.java | 122 +++++++++++ .../platform/sdk/TDFRootSignatureTest.java | 195 ++++++++++++++++++ 2 files changed, 317 insertions(+) diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java index 5e57937d..45de6945 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java @@ -1,12 +1,17 @@ package io.opentdf.platform.sdk; import com.google.gson.Gson; +import com.google.gson.JsonParser; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.List; import java.util.Map; +import java.util.stream.Stream; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -285,4 +290,121 @@ void testReadingManifestWithObjectStatementValue() throws IOException { ) ); } + + /** + * A minimal but valid manifest with extra members spliced into the {@code payload} object + * and into the manifest root. Each extra, when not empty, must begin with a comma. + */ + private static String manifestWithExtras(String payloadExtra, String rootExtra) { + return "{\n" + + " \"encryptionInformation\": {\n" + + " \"integrityInformation\": {\n" + + " \"encryptedSegmentSizeDefault\": " + ENCRYPTED_SEGMENT_SIZE_DEFAULT + ",\n" + + " \"segmentSizeDefault\": " + SEGMENT_SIZE_DEFAULT + ",\n" + + " \"rootSignature\": { \"alg\": \"HS256\", \"sig\": \"c2ln\" },\n" + + " \"segmentHashAlg\": \"GMAC\",\n" + + " \"segments\": [ { \"hash\": \"aGFzaDA=\" } ]\n" + + " },\n" + + " \"keyAccess\": [ { \"protocol\": \"kas\", \"type\": \"wrapped\"," + + " \"url\": \"http://localhost:65432/kas\", \"wrappedKey\": \"a2V5\" } ],\n" + + " \"method\": { \"algorithm\": \"AES-256-GCM\", \"isStreamable\": true, \"iv\": \"aXY=\" },\n" + + " \"policy\": \"cG9saWN5\",\n" + + " \"type\": \"split\"\n" + + " },\n" + + " \"payload\": { \"isEncrypted\": true, \"protocol\": \"zip\"," + + " \"type\": \"reference\", \"url\": \"0.payload\"" + payloadExtra + " }" + + rootExtra + "\n" + + "}"; + } + + static Stream specVersionCases() { + return Stream.of( + Arguments.of("schemaVersion at root", "", ",\"schemaVersion\":\"4.3.0\"", "4.3.0"), + // where the spec prose documents it, and where web-sdk writes it + Arguments.of("tdf_spec_version at root", "", ",\"tdf_spec_version\":\"4.3.0\"", "4.3.0"), + // where revisions of the JSON schema declared it in error + Arguments.of("tdf_spec_version under payload", ",\"tdf_spec_version\":\"4.3.0\"", "", "4.3.0"), + Arguments.of("schemaVersion wins over payload tdf_spec_version", + ",\"tdf_spec_version\":\"4.2.0\"", ",\"schemaVersion\":\"4.3.0\"", "4.3.0"), + Arguments.of("schemaVersion wins over root tdf_spec_version, whatever the key order", + "", ",\"tdf_spec_version\":\"4.2.0\",\"schemaVersion\":\"4.3.0\"", "4.3.0"), + Arguments.of("schemaVersion wins over root tdf_spec_version", + "", ",\"schemaVersion\":\"4.3.0\",\"tdf_spec_version\":\"4.2.0\"", "4.3.0"), + // the root is the placement with the better provenance, so it decides when the + // two copies disagree + Arguments.of("root tdf_spec_version wins over the payload copy", + ",\"tdf_spec_version\":\"4.2.0\"", ",\"tdf_spec_version\":\"4.3.0\"", "4.3.0"), + // a null root copy is not a value, so the payload copy still applies + Arguments.of("null root tdf_spec_version falls through to payload", + ",\"tdf_spec_version\":\"4.3.0\"", ",\"tdf_spec_version\":null", "4.3.0"), + Arguments.of("numeric root tdf_spec_version falls through to payload", + ",\"tdf_spec_version\":\"4.3.0\"", ",\"tdf_spec_version\":430", "4.3.0"), + Arguments.of("empty root tdf_spec_version falls through to payload", + ",\"tdf_spec_version\":\"4.3.0\"", ",\"tdf_spec_version\":\"\"", "4.3.0"), + // an empty schemaVersion is not a value, so the fallback still applies + Arguments.of("empty schemaVersion falls back to tdf_spec_version", + ",\"tdf_spec_version\":\"4.3.0\"", ",\"schemaVersion\":\"\"", "4.3.0"), + Arguments.of("null schemaVersion falls back to tdf_spec_version", + "", ",\"schemaVersion\":null,\"tdf_spec_version\":\"4.3.0\"", "4.3.0"), + Arguments.of("no version at all", "", "", null), + // non-string values are schema validation's problem to report, not the decoder's + // to choke on. the key is known in the wild carrying null + Arguments.of("null tdf_spec_version is ignored", ",\"tdf_spec_version\":null", "", null), + Arguments.of("numeric tdf_spec_version is ignored", ",\"tdf_spec_version\":430", "", null), + Arguments.of("boolean tdf_spec_version is ignored", "", ",\"tdf_spec_version\":true", null), + Arguments.of("object tdf_spec_version is ignored", + ",\"tdf_spec_version\":{\"major\":4}", ",\"tdf_spec_version\":{\"major\":4}", null), + Arguments.of("array tdf_spec_version is ignored", ",\"tdf_spec_version\":[\"4.3.0\"]", "", null)); + } + + /** + * {@code schemaVersion} is the name of the spec-version field. {@code tdf_spec_version} is a + * non-aligned name that entered some specification drafts and some older OpenTDF documentation + * in error; it is read, at the root and then under {@code payload}, only so that files written + * with it stay usable. + */ + @ParameterizedTest(name = "{0}") + @MethodSource("specVersionCases") + void testSpecVersionIsReadFromAllThreePlaces(String name, String payloadExtra, String rootExtra, String want) { + Manifest manifest = Manifest.readManifest(manifestWithExtras(payloadExtra, rootExtra)); + + assertThat(manifest.tdfVersion).isEqualTo(want); + + // the version lookup must not disturb anything else in the document + assertThat(manifest.payload.url).isEqualTo("0.payload"); + assertThat(manifest.payload.isEncrypted).isTrue(); + assertThat(manifest.encryptionInformation.keyAccessType).isEqualTo("split"); + assertThat(manifest.encryptionInformation.policy).isEqualTo("cG9saWN5"); + var integrityInformation = manifest.encryptionInformation.integrityInformation; + assertThat(integrityInformation.segmentHashAlg).isEqualTo("GMAC"); + assertThat(integrityInformation.rootSignature.signature).isEqualTo("c2ln"); + // and the segment-size fixup, which lives in its own adapter, still runs + assertThat(integrityInformation.segments.get(0).encryptedSegmentSize).isEqualTo(ENCRYPTED_SEGMENT_SIZE_DEFAULT); + } + + /** + * The writer names the field {@code schemaVersion} and never the non-aligned + * {@code tdf_spec_version}, at the root or under {@code payload}. Reading a manifest that + * used the non-aligned name and writing it back out therefore normalizes the name rather + * than propagating it. + */ + @ParameterizedTest(name = "{0}") + @MethodSource("nonAlignedPlacements") + void testRoundTripEmitsSchemaVersionOnly(String name, String payloadExtra, String rootExtra) { + Manifest manifest = Manifest.readManifest(manifestWithExtras(payloadExtra, rootExtra)); + assertThat(manifest.tdfVersion).isEqualTo("4.3.0"); + + var written = JsonParser.parseString(Manifest.toJson(manifest)).getAsJsonObject(); + assertThat(written.get("schemaVersion").getAsString()).isEqualTo("4.3.0"); + assertThat(written.has("tdf_spec_version")).isFalse(); + assertThat(written.getAsJsonObject("payload").has("tdf_spec_version")).isFalse(); + + assertEquals(manifest, Manifest.readManifest(written.toString())); + } + + static Stream nonAlignedPlacements() { + return Stream.of( + Arguments.of("root", "", ",\"tdf_spec_version\":\"4.3.0\""), + Arguments.of("payload", ",\"tdf_spec_version\":\"4.3.0\"", "")); + } } diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/TDFRootSignatureTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFRootSignatureTest.java index b6a4c8de..9b79b261 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFRootSignatureTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFRootSignatureTest.java @@ -330,6 +330,157 @@ void legacyGmacRootIsRejected() throws IOException { .isInstanceOf(SDK.RootSignatureValidationException.class); } + // ------------------------------------------------------ spec version and digests + + /* + * The manifest's spec-version field used to decide the digest encoding: no version meant + * hex (pre-4.3.0), any version meant raw. That only held because this SDK's writer sets + * both from one boolean, and the field is unauthenticated. The encoding is now read off the + * file, so none of these files depend on what the version field says, or where. + */ + + @ParameterizedTest + @ValueSource(strings = { "root", "payload" }) + void currentFileWithVersionOnlyUnderTheNonAlignedNameDecrypts(String placement) throws IOException { + var plaintext = fourSegmentPlaintext(); + var rewritten = rewrite(createTdf(plaintext, Config.withAssertionConfig(assertionConfig())), + manifest -> moveVersionToNonAlignedName(manifest, placement), + UnaryOperator.identity()); + + assertThat(Manifest.readManifest(manifestOf(rewritten)).tdfVersion).isEqualTo(TDF.TDF_SPEC_VERSION); + assertThat(decrypt(rewritten)).containsExactly(plaintext); + } + + @Test + void currentFileWithNoVersionAtAllDecrypts() throws IOException { + // raw digests with no version recorded anywhere used to be misread as hex + var plaintext = fourSegmentPlaintext(); + var rewritten = rewrite(createTdf(plaintext, Config.withAssertionConfig(assertionConfig())), + manifest -> manifest.remove("schemaVersion"), + UnaryOperator.identity()); + + assertThat(Manifest.readManifest(manifestOf(rewritten)).tdfVersion).isNull(); + assertThat(decrypt(rewritten)).containsExactly(plaintext); + } + + @ParameterizedTest + @EnumSource(Config.IntegrityAlgorithm.class) + void hexDigestFileThatCarriesAVersionDecrypts(Config.IntegrityAlgorithm segmentAlgorithm) throws IOException { + // hex digests with a version recorded used to be misread as raw + var plaintext = fourSegmentPlaintext(); + var tdfBytes = createTdf(plaintext, + Config.withTargetMode("4.2.2"), + config -> config.renderVersionInfoInManifest = true, + withSegmentAlgorithm(segmentAlgorithm), + Config.withAssertionConfig(assertionConfig())); + + var manifest = JsonParser.parseString(manifestOf(tdfBytes)).getAsJsonObject(); + assertThat(manifest.get("schemaVersion").getAsString()).isEqualTo(TDF.TDF_SPEC_VERSION); + var rootSignature = Base64.getDecoder().decode(rootSignature(manifest).get("sig").getAsString()); + assertThat(new String(rootSignature, StandardCharsets.UTF_8)).matches("[0-9a-f]{64}"); + + assertThat(decrypt(tdfBytes)).containsExactly(plaintext); + } + + @ParameterizedTest + @ValueSource(strings = { "root", "payload" }) + void hexDigestFileWithVersionUnderTheNonAlignedNameDecrypts(String placement) throws IOException { + var plaintext = fourSegmentPlaintext(); + var tdfBytes = createTdf(plaintext, + Config.withTargetMode("4.2.2"), + config -> config.renderVersionInfoInManifest = true, + Config.withAssertionConfig(assertionConfig())); + var rewritten = rewrite(tdfBytes, manifest -> moveVersionToNonAlignedName(manifest, placement), + UnaryOperator.identity()); + + assertThat(decrypt(rewritten)).containsExactly(plaintext); + } + + @Test + void legacyHexDigestFileWithAnAssertionStillDecrypts() throws IOException { + var plaintext = fourSegmentPlaintext(); + var tdfBytes = createTdf(plaintext, + Config.withTargetMode("4.2.2"), + Config.withAssertionConfig(assertionConfig())); + assertThat(JsonParser.parseString(manifestOf(tdfBytes)).getAsJsonObject().has("schemaVersion")).isFalse(); + + assertThat(decrypt(tdfBytes)).containsExactly(plaintext); + } + + @ParameterizedTest + @ValueSource(strings = { "4.2.2", "4.3.0" }) + void respellingSegmentHashesWithoutTheKeyIsCaught(String targetMode) throws IOException { + // accepting either spelling of a segment digest does not make the spelling free to + // change: the root signature is over the recorded bytes, so switching raw to hex (or + // hex back to raw) without the key breaks it + var tampered = rewrite(createTdf(fourSegmentPlaintext(), Config.withTargetMode(targetMode)), manifest -> { + for (var element : segments(manifest)) { + var segment = element.getAsJsonObject(); + var recorded = Base64.getDecoder().decode(segment.get("hash").getAsString()); + var respelled = "4.2.2".equals(targetMode) + ? unhex(new String(recorded, StandardCharsets.UTF_8)) + : hex(recorded).getBytes(StandardCharsets.UTF_8); + segment.addProperty("hash", Base64.getEncoder().encodeToString(respelled)); + } + }, UnaryOperator.identity()); + + assertThatThrownBy(() -> decrypt(tampered)) + .isInstanceOf(SDK.RootSignatureValidationException.class); + } + + @ParameterizedTest + @ValueSource(strings = { "root", "payload" }) + void tamperingIsStillCaughtWhenTheVersionIsUnderTheNonAlignedName(String placement) throws IOException { + var original = createTdf(fourSegmentPlaintext(), withSegmentAlgorithm(Config.IntegrityAlgorithm.HS256)); + + var editedSegmentHash = rewrite(original, manifest -> { + moveVersionToNonAlignedName(manifest, placement); + var first = segments(manifest).get(0).getAsJsonObject(); + var hash = Base64.getDecoder().decode(first.get("hash").getAsString()); + hash[0] ^= 0xFF; + first.addProperty("hash", Base64.getEncoder().encodeToString(hash)); + }, UnaryOperator.identity()); + assertThatThrownBy(() -> decrypt(editedSegmentHash)) + .isInstanceOf(SDK.RootSignatureValidationException.class); + + var editedRootSignature = rewrite(original, manifest -> { + moveVersionToNonAlignedName(manifest, placement); + var sig = Base64.getDecoder().decode(rootSignature(manifest).get("sig").getAsString()); + sig[0] ^= 0xFF; + rootSignature(manifest).addProperty("sig", Base64.getEncoder().encodeToString(sig)); + }, UnaryOperator.identity()); + assertThatThrownBy(() -> decrypt(editedRootSignature)) + .isInstanceOf(SDK.RootSignatureValidationException.class); + + var editedSegmentBody = rewrite(original, + manifest -> moveVersionToNonAlignedName(manifest, placement), + payload -> flipByte(payload, payload.length / 2)); + assertThatThrownBy(() -> decrypt(editedSegmentBody)) + .isInstanceOf(SDK.SegmentSignatureMismatch.class); + } + + @ParameterizedTest + @ValueSource(strings = { "4.2.2", "4.3.0" }) + void tamperingIsStillCaughtInEitherEncodingWhenAVersionIsRecorded(String targetMode) throws IOException { + var original = createTdf(fourSegmentPlaintext(), + Config.withTargetMode(targetMode), + config -> config.renderVersionInfoInManifest = true, + withSegmentAlgorithm(Config.IntegrityAlgorithm.HS256)); + + var editedRootSignature = rewrite(original, manifest -> { + var sig = Base64.getDecoder().decode(rootSignature(manifest).get("sig").getAsString()); + sig[0] ^= 0x01; + rootSignature(manifest).addProperty("sig", Base64.getEncoder().encodeToString(sig)); + }, UnaryOperator.identity()); + assertThatThrownBy(() -> decrypt(editedRootSignature)) + .isInstanceOf(SDK.RootSignatureValidationException.class); + + var editedSegmentBody = rewrite(original, manifest -> { + }, payload -> flipByte(payload, payload.length / 2)); + assertThatThrownBy(() -> decrypt(editedSegmentBody)) + .isInstanceOf(SDK.SegmentSignatureMismatch.class); + } + // ------------------------------------------------------------------ config @Test @@ -464,8 +615,52 @@ private static byte[] decrypt(byte[] tdfBytes) throws IOException { return plaintext.toByteArray(); } + /** An assertion signed with the default HS256 payload key, so reads verify it. */ + private static AssertionConfig assertionConfig() { + var assertionConfig = new AssertionConfig(); + assertionConfig.id = "assertion1"; + assertionConfig.type = AssertionConfig.Type.BaseAssertion; + assertionConfig.scope = AssertionConfig.Scope.TrustedDataObj; + assertionConfig.appliesToState = AssertionConfig.AppliesToState.Unencrypted; + assertionConfig.statement = new AssertionConfig.Statement(); + assertionConfig.statement.format = "base64binary"; + assertionConfig.statement.schema = "text"; + assertionConfig.statement.value = "ICAgIDxlZGoOkVkaD4="; + return assertionConfig; + } + // ------------------------------------------------------- manifest surgery + /** + * Rewrites the spec version the way a writer built from the non-aligned name emits it: + * {@code schemaVersion} removed, the same value recorded as {@code tdf_spec_version} at the + * manifest root or under {@code payload}. The root signature covers the segment hashes, not + * the JSON, so the result is still internally consistent. + */ + private static void moveVersionToNonAlignedName(JsonObject manifest, String placement) { + var version = manifest.remove("schemaVersion"); + assertThat(version).withFailMessage("fixture should have been written with schemaVersion").isNotNull(); + var target = "root".equals(placement) ? manifest : manifest.getAsJsonObject("payload"); + target.add("tdf_spec_version", version); + } + + private static String hex(byte[] bytes) { + var out = new StringBuilder(bytes.length * 2); + for (var b : bytes) { + out.append(String.format("%02x", b)); + } + return out.toString(); + } + + private static byte[] unhex(String hex) { + var out = new byte[hex.length() / 2]; + for (int index = 0; index < out.length; index++) { + out[index] = (byte) Integer.parseInt(hex.substring(2 * index, 2 * index + 2), 16); + } + return out; + } + + private static JsonObject integrityInformation(JsonObject manifest) { return manifest.getAsJsonObject("encryptionInformation").getAsJsonObject("integrityInformation"); } From 080a406417057ca0708ac904c4988f2697cc1e6d Mon Sep 17 00:00:00 2001 From: Paul Flynn Date: Wed, 30 Sep 2026 15:19:28 -0400 Subject: [PATCH 2/3] fix(sdk): read the TDF spec version from all three places, and stop trusting 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) Signed-off-by: Paul Flynn --- .../io/opentdf/platform/sdk/Manifest.java | 95 +++++++++++++++++++ .../java/io/opentdf/platform/sdk/TDF.java | 86 +++++++++++------ 2 files changed, 153 insertions(+), 28 deletions(-) diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java b/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java index 0e8b044c..d4b86c46 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java @@ -64,7 +64,17 @@ public class Manifest { private static final Gson gson = new GsonBuilder() .registerTypeAdapter(AssertionConfig.Statement.class, new AssertionValueAdapter()) .registerTypeAdapterFactory(new IntegrityInformationAdapterFactory()) + .registerTypeAdapterFactory(new SpecVersionAdapterFactory()) .create(); + + /** + * The TDF spec version the manifest records. Written as {@code schemaVersion} only; on read + * it may also come from the non-aligned {@code tdf_spec_version} name, see + * {@link SpecVersionAdapterFactory}. + *

+ * This is metadata. It does not decide whether a container verifies: the integrity digest + * encoding is read off the file rather than off this field (see {@code TDF.digestMatchesRecorded}). + */ @SerializedName(value = "schemaVersion") String tdfVersion; @@ -277,6 +287,91 @@ private static boolean hasValue(JsonObject object, String memberName) { } } + /** + * Reads {@code tdf_spec_version}, a non-aligned name for the spec-version field, when the + * canonical {@code schemaVersion} is absent, so that files written with that name stay + * readable. + *

+ * {@code schemaVersion} is the canonical name. {@code tdf_spec_version} is not a former + * spelling that was renamed -- it entered some specification drafts and some older OpenTDF + * documentation in error, and writers built from those drafts emitted it. We read it so + * those files stay usable; we never write it. + *

+ * Precedence is {@code schemaVersion}, then {@code tdf_spec_version} at the root, then + * {@code tdf_spec_version} under {@code payload}. Nothing is written back under the + * non-aligned name -- serializing a manifest always emits {@code schemaVersion} only, so a + * round trip normalizes the name rather than propagating it. + *

+ * Both placements are probed because both occur in archival files. The root is where the + * spec's own manifest.md has always documented the field, and where web-sdk both wrote it + * and still reads it. Under {@code payload} is where revisions of the JSON schema declared it + * in error, which led at least one writer to emit the key there with a {@code null} value. + *

+ * Only a non-empty JSON string counts. Any other value -- {@code null}, a number, an object + * -- is skipped rather than failing the decode: reporting malformed manifests is schema + * validation's job, not the decoder's. + *

+ * Gson's {@code @SerializedName(alternate = ...)} cannot do this: it cannot reach into + * {@code payload}, and when both names are present it keeps whichever comes last in the + * document instead of preferring {@code schemaVersion}. + */ + private static class SpecVersionAdapterFactory implements TypeAdapterFactory { + private static final String NON_ALIGNED_SPEC_VERSION = "tdf_spec_version"; + private static final String PAYLOAD = "payload"; + + @Override + public TypeAdapter create(Gson gson, TypeToken type) { + if (!Manifest.class.equals(type.getRawType())) { + return null; + } + final TypeAdapter delegate = gson.getDelegateAdapter(this, type); + final TypeAdapter elementAdapter = gson.getAdapter(JsonElement.class); + return new TypeAdapter() { + @Override + public void write(JsonWriter out, T value) throws IOException { + delegate.write(out, value); + } + + @Override + public T read(JsonReader in) throws IOException { + JsonElement tree = elementAdapter.read(in); + T value = delegate.fromJsonTree(tree); + if (value instanceof Manifest && tree != null && tree.isJsonObject()) { + Manifest manifest = (Manifest) value; + if (manifest.tdfVersion == null || manifest.tdfVersion.isEmpty()) { + String nonAligned = nonAlignedSpecVersion(tree.getAsJsonObject()); + if (nonAligned != null) { + manifest.tdfVersion = nonAligned; + } + } + } + return value; + } + }; + } + + /** The first non-empty string under the non-aligned name, root before payload, or null. */ + private static String nonAlignedSpecVersion(JsonObject root) { + String atRoot = nonEmptyString(root.get(NON_ALIGNED_SPEC_VERSION)); + if (atRoot != null) { + return atRoot; + } + JsonElement payload = root.get(PAYLOAD); + if (payload != null && payload.isJsonObject()) { + return nonEmptyString(payload.getAsJsonObject().get(NON_ALIGNED_SPEC_VERSION)); + } + return null; + } + + private static String nonEmptyString(JsonElement element) { + if (element == null || !element.isJsonPrimitive() || !element.getAsJsonPrimitive().isString()) { + return null; + } + String value = element.getAsString(); + return value.isEmpty() ? null : value; + } + } + static public class PolicyBinding { public String alg; public String hash; diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java b/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java index a16e56e9..aefd8927 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java @@ -431,18 +431,12 @@ public void readPayload(OutputStream outputStream) throws SDK.TamperException, I + segment.encryptedSegmentSize + " but got " + bytesRead + ")"); } - var isLegacyTdf = manifest.tdfVersion == null || manifest.tdfVersion.isEmpty(); - if (manifest.payload.isEncrypted) { var sigAlg = segmentIntegrityAlgorithmFromManifest( manifest.encryptionInformation.integrityInformation.segmentHashAlg); var payloadSig = segmentIntegrity(readBuf, payloadKey, sigAlg); - if (isLegacyTdf) { - payloadSig = Hex.encodeHexString(payloadSig).getBytes(StandardCharsets.UTF_8); - } - - if (segment.hash.compareTo(Base64.getEncoder().encodeToString(payloadSig)) != 0) { + if (!digestMatchesRecorded(segment.hash, payloadSig)) { throw new SDK.SegmentSignatureMismatch("segment signature miss match"); } @@ -795,6 +789,39 @@ TDFObject createTDF(InputStream payload, OutputStream outputStream, Config.TDFCo } + /** + * Whether the value a manifest records for an integrity check matches {@code digest}, the raw + * (non-hex) form recomputed from the file's own bytes. + *

+ * The recorded value is base64 over one of two spellings of the same keyed digest: the raw + * bytes (TDF spec 4.3.0 and later) or their hex (before 4.3.0). Which spelling a file used is + * read off the file, never off the manifest's spec-version field. That field is + * unauthenticated, and it only ever tracked the encoding because this SDK's own writer sets + * {@code hexEncodeRootAndSegmentHashes} and {@code renderVersionInfoInManifest} from one + * boolean -- writers that decoupled them, including every writer that produced the + * non-aligned {@code tdf_spec_version} name, break the correspondence and would be misread + * as using the other encoding. + *

+ * Accepting both spellings weakens nothing. Hex is an invertible encoding of the same HMAC, + * so producing either form requires the payload key exactly as much as producing the other; + * there is no forgery that the strict form would have rejected. + */ + static boolean digestMatchesRecorded(String recorded, byte[] digest) { + var recordedBytes = recorded.getBytes(StandardCharsets.UTF_8); + var raw = Base64.getEncoder().encode(digest); + var hex = Base64.getEncoder().encode(Hex.encodeHexString(digest).getBytes(StandardCharsets.UTF_8)); + // non-short-circuiting, so the time taken does not depend on which spelling matched + return MessageDigest.isEqual(recordedBytes, raw) | MessageDigest.isEqual(recordedBytes, hex); + } + + /** Base64 over {@code aggregateHash || assertionHash}, the value an assertion signature covers. */ + private static String assertionSignedOver(byte[] aggregateHash, byte[] assertionHash) { + var signedOver = new byte[aggregateHash.length + assertionHash.length]; + System.arraycopy(aggregateHash, 0, signedOver, 0, aggregateHash.length); + System.arraycopy(assertionHash, 0, signedOver, aggregateHash.length, assertionHash.length); + return Base64.getEncoder().encodeToString(signedOver); + } + Reader loadTDF(SeekableByteChannel tdf, String platformUrl) throws SDKException, IOException { return loadTDF(tdf, Config.newTDFReaderConfig(), platformUrl); } @@ -922,16 +949,15 @@ Reader loadTDF(SeekableByteChannel tdf, Config.TDFReaderConfig tdfReaderConfig) } } - String rootSigValue; - boolean isLegacyTdf = manifest.tdfVersion == null || manifest.tdfVersion.isEmpty(); + // aggregateHash is built from the segment hashes exactly as the manifest records them + // (raw digest bytes, or their hex), so it is already independent of the encoding and + // only the comparison below has to allow for both spellings. + boolean rootSignatureMatches; if (manifest.payload.isEncrypted) { var sigAlg = rootIntegrityAlgorithmFromManifest(rootAlgorithm); var sig = rootIntegrity(aggregateHash.toByteArray(), payloadKey, sigAlg); - if (isLegacyTdf) { - sig = Hex.encodeHexString(sig).getBytes(); - } - rootSigValue = Base64.getEncoder().encodeToString(sig); + rootSignatureMatches = digestMatchesRecorded(rootSignature, sig); } else { // KNOWN GAP, untouched by this change and tracked separately: this branch is a // bare SHA-256, so it authenticates nothing. `payload.isEncrypted` is itself @@ -947,10 +973,11 @@ Reader loadTDF(SeekableByteChannel tdf, Config.TDFReaderConfig tdfReaderConfig) throw new IllegalStateException("error getting instance of SHA-256 digest", e); } - rootSigValue = Base64.getEncoder().encodeToString(digest.digest(aggregateHash.toString().getBytes())); + String rootSigValue = Base64.getEncoder().encodeToString(digest.digest(aggregateHash.toString().getBytes())); + rootSignatureMatches = rootSignature.compareTo(rootSigValue) == 0; } - if (rootSignature.compareTo(rootSigValue) != 0) { + if (!rootSignatureMatches) { throw new SDK.RootSignatureValidationException("root signature validation failed"); } @@ -993,21 +1020,24 @@ Reader loadTDF(SeekableByteChannel tdf, Config.TDFReaderConfig tdfReaderConfig) } byte[] hashOfAssertion; - if (isLegacyTdf) { - hashOfAssertion = hashOfAssertionAsHex.getBytes(StandardCharsets.UTF_8); - } else { - try { - hashOfAssertion = Hex.decodeHex(hashOfAssertionAsHex); - } catch (DecoderException e) { - throw new SDKException("error decoding assertion hash", e); - } + try { + hashOfAssertion = Hex.decodeHex(hashOfAssertionAsHex); + } catch (DecoderException e) { + throw new SDKException("error decoding assertion hash", e); } - var signature = new byte[aggregateHashByteArrayBytes.length + hashOfAssertion.length]; - System.arraycopy(aggregateHashByteArrayBytes, 0, signature, 0, aggregateHashByteArrayBytes.length); - System.arraycopy(hashOfAssertion, 0, signature, aggregateHashByteArrayBytes.length, hashOfAssertion.length); - var encodeSignature = Base64.getEncoder().encodeToString(signature); - if (!Objects.equals(encodeSignature, hashValues.getSignature())) { + // The same two spellings as the segment and root digests (raw before hex), for the + // same reasons given on digestMatchesRecorded; the spec version is not consulted. + // Here the assertion hash is concatenated onto the aggregate hash before signing, so + // the candidates are the two concatenations, and either may match. That is sound on + // the same grounds: both bind the same aggregate hash and the same assertion hash, + // and the recorded value was already verified under assertionKey. + var recordedSignature = hashValues.getSignature(); + if (!Objects.equals(assertionSignedOver(aggregateHashByteArrayBytes, hashOfAssertion), recordedSignature) + && !Objects.equals( + assertionSignedOver(aggregateHashByteArrayBytes, + hashOfAssertionAsHex.getBytes(StandardCharsets.UTF_8)), + recordedSignature)) { throw new SDK.AssertionException("failed integrity check on assertion signature", assertion.id); } } From a7fef09b5990ce8dbb3d293514f01c9c2f52ef3e Mon Sep 17 00:00:00 2001 From: Paul Flynn Date: Wed, 30 Sep 2026 15:26:11 -0400 Subject: [PATCH 3/3] fix(sdk): scope to reading the spec version; keep the digest encoding 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) Signed-off-by: Paul Flynn --- .../io/opentdf/platform/sdk/Manifest.java | 4 +- .../java/io/opentdf/platform/sdk/TDF.java | 86 +++++--------- .../platform/sdk/TDFRootSignatureTest.java | 111 +----------------- 3 files changed, 35 insertions(+), 166 deletions(-) diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java b/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java index d4b86c46..ae03a1d5 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java @@ -72,8 +72,8 @@ public class Manifest { * it may also come from the non-aligned {@code tdf_spec_version} name, see * {@link SpecVersionAdapterFactory}. *

- * This is metadata. It does not decide whether a container verifies: the integrity digest - * encoding is read off the file rather than off this field (see {@code TDF.digestMatchesRecorded}). + * The reader uses it to choose how the integrity digests are encoded: hex when no version is + * recorded (pre-4.3.0), raw bytes otherwise. */ @SerializedName(value = "schemaVersion") String tdfVersion; diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java b/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java index aefd8927..a16e56e9 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java @@ -431,12 +431,18 @@ public void readPayload(OutputStream outputStream) throws SDK.TamperException, I + segment.encryptedSegmentSize + " but got " + bytesRead + ")"); } + var isLegacyTdf = manifest.tdfVersion == null || manifest.tdfVersion.isEmpty(); + if (manifest.payload.isEncrypted) { var sigAlg = segmentIntegrityAlgorithmFromManifest( manifest.encryptionInformation.integrityInformation.segmentHashAlg); var payloadSig = segmentIntegrity(readBuf, payloadKey, sigAlg); - if (!digestMatchesRecorded(segment.hash, payloadSig)) { + if (isLegacyTdf) { + payloadSig = Hex.encodeHexString(payloadSig).getBytes(StandardCharsets.UTF_8); + } + + if (segment.hash.compareTo(Base64.getEncoder().encodeToString(payloadSig)) != 0) { throw new SDK.SegmentSignatureMismatch("segment signature miss match"); } @@ -789,39 +795,6 @@ TDFObject createTDF(InputStream payload, OutputStream outputStream, Config.TDFCo } - /** - * Whether the value a manifest records for an integrity check matches {@code digest}, the raw - * (non-hex) form recomputed from the file's own bytes. - *

- * The recorded value is base64 over one of two spellings of the same keyed digest: the raw - * bytes (TDF spec 4.3.0 and later) or their hex (before 4.3.0). Which spelling a file used is - * read off the file, never off the manifest's spec-version field. That field is - * unauthenticated, and it only ever tracked the encoding because this SDK's own writer sets - * {@code hexEncodeRootAndSegmentHashes} and {@code renderVersionInfoInManifest} from one - * boolean -- writers that decoupled them, including every writer that produced the - * non-aligned {@code tdf_spec_version} name, break the correspondence and would be misread - * as using the other encoding. - *

- * Accepting both spellings weakens nothing. Hex is an invertible encoding of the same HMAC, - * so producing either form requires the payload key exactly as much as producing the other; - * there is no forgery that the strict form would have rejected. - */ - static boolean digestMatchesRecorded(String recorded, byte[] digest) { - var recordedBytes = recorded.getBytes(StandardCharsets.UTF_8); - var raw = Base64.getEncoder().encode(digest); - var hex = Base64.getEncoder().encode(Hex.encodeHexString(digest).getBytes(StandardCharsets.UTF_8)); - // non-short-circuiting, so the time taken does not depend on which spelling matched - return MessageDigest.isEqual(recordedBytes, raw) | MessageDigest.isEqual(recordedBytes, hex); - } - - /** Base64 over {@code aggregateHash || assertionHash}, the value an assertion signature covers. */ - private static String assertionSignedOver(byte[] aggregateHash, byte[] assertionHash) { - var signedOver = new byte[aggregateHash.length + assertionHash.length]; - System.arraycopy(aggregateHash, 0, signedOver, 0, aggregateHash.length); - System.arraycopy(assertionHash, 0, signedOver, aggregateHash.length, assertionHash.length); - return Base64.getEncoder().encodeToString(signedOver); - } - Reader loadTDF(SeekableByteChannel tdf, String platformUrl) throws SDKException, IOException { return loadTDF(tdf, Config.newTDFReaderConfig(), platformUrl); } @@ -949,15 +922,16 @@ Reader loadTDF(SeekableByteChannel tdf, Config.TDFReaderConfig tdfReaderConfig) } } - // aggregateHash is built from the segment hashes exactly as the manifest records them - // (raw digest bytes, or their hex), so it is already independent of the encoding and - // only the comparison below has to allow for both spellings. - boolean rootSignatureMatches; + String rootSigValue; + boolean isLegacyTdf = manifest.tdfVersion == null || manifest.tdfVersion.isEmpty(); if (manifest.payload.isEncrypted) { var sigAlg = rootIntegrityAlgorithmFromManifest(rootAlgorithm); var sig = rootIntegrity(aggregateHash.toByteArray(), payloadKey, sigAlg); - rootSignatureMatches = digestMatchesRecorded(rootSignature, sig); + if (isLegacyTdf) { + sig = Hex.encodeHexString(sig).getBytes(); + } + rootSigValue = Base64.getEncoder().encodeToString(sig); } else { // KNOWN GAP, untouched by this change and tracked separately: this branch is a // bare SHA-256, so it authenticates nothing. `payload.isEncrypted` is itself @@ -973,11 +947,10 @@ Reader loadTDF(SeekableByteChannel tdf, Config.TDFReaderConfig tdfReaderConfig) throw new IllegalStateException("error getting instance of SHA-256 digest", e); } - String rootSigValue = Base64.getEncoder().encodeToString(digest.digest(aggregateHash.toString().getBytes())); - rootSignatureMatches = rootSignature.compareTo(rootSigValue) == 0; + rootSigValue = Base64.getEncoder().encodeToString(digest.digest(aggregateHash.toString().getBytes())); } - if (!rootSignatureMatches) { + if (rootSignature.compareTo(rootSigValue) != 0) { throw new SDK.RootSignatureValidationException("root signature validation failed"); } @@ -1020,24 +993,21 @@ Reader loadTDF(SeekableByteChannel tdf, Config.TDFReaderConfig tdfReaderConfig) } byte[] hashOfAssertion; - try { - hashOfAssertion = Hex.decodeHex(hashOfAssertionAsHex); - } catch (DecoderException e) { - throw new SDKException("error decoding assertion hash", e); + if (isLegacyTdf) { + hashOfAssertion = hashOfAssertionAsHex.getBytes(StandardCharsets.UTF_8); + } else { + try { + hashOfAssertion = Hex.decodeHex(hashOfAssertionAsHex); + } catch (DecoderException e) { + throw new SDKException("error decoding assertion hash", e); + } } + var signature = new byte[aggregateHashByteArrayBytes.length + hashOfAssertion.length]; + System.arraycopy(aggregateHashByteArrayBytes, 0, signature, 0, aggregateHashByteArrayBytes.length); + System.arraycopy(hashOfAssertion, 0, signature, aggregateHashByteArrayBytes.length, hashOfAssertion.length); + var encodeSignature = Base64.getEncoder().encodeToString(signature); - // The same two spellings as the segment and root digests (raw before hex), for the - // same reasons given on digestMatchesRecorded; the spec version is not consulted. - // Here the assertion hash is concatenated onto the aggregate hash before signing, so - // the candidates are the two concatenations, and either may match. That is sound on - // the same grounds: both bind the same aggregate hash and the same assertion hash, - // and the recorded value was already verified under assertionKey. - var recordedSignature = hashValues.getSignature(); - if (!Objects.equals(assertionSignedOver(aggregateHashByteArrayBytes, hashOfAssertion), recordedSignature) - && !Objects.equals( - assertionSignedOver(aggregateHashByteArrayBytes, - hashOfAssertionAsHex.getBytes(StandardCharsets.UTF_8)), - recordedSignature)) { + if (!Objects.equals(encodeSignature, hashValues.getSignature())) { throw new SDK.AssertionException("failed integrity check on assertion signature", assertion.id); } } diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/TDFRootSignatureTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFRootSignatureTest.java index 9b79b261..f8ccc0c3 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFRootSignatureTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFRootSignatureTest.java @@ -330,13 +330,13 @@ void legacyGmacRootIsRejected() throws IOException { .isInstanceOf(SDK.RootSignatureValidationException.class); } - // ------------------------------------------------------ spec version and digests + // ------------------------------------------------------------------ spec version /* - * The manifest's spec-version field used to decide the digest encoding: no version meant - * hex (pre-4.3.0), any version meant raw. That only held because this SDK's writer sets - * both from one boolean, and the field is unauthenticated. The encoding is now read off the - * file, so none of these files depend on what the version field says, or where. + * The manifest's spec-version field decides the digest encoding: no version means hex + * (pre-4.3.0), any version means raw. A current file whose version is recorded only under + * the non-aligned tdf_spec_version name, at the root or under payload, must still be read + * as raw. */ @ParameterizedTest @@ -351,51 +351,6 @@ void currentFileWithVersionOnlyUnderTheNonAlignedNameDecrypts(String placement) assertThat(decrypt(rewritten)).containsExactly(plaintext); } - @Test - void currentFileWithNoVersionAtAllDecrypts() throws IOException { - // raw digests with no version recorded anywhere used to be misread as hex - var plaintext = fourSegmentPlaintext(); - var rewritten = rewrite(createTdf(plaintext, Config.withAssertionConfig(assertionConfig())), - manifest -> manifest.remove("schemaVersion"), - UnaryOperator.identity()); - - assertThat(Manifest.readManifest(manifestOf(rewritten)).tdfVersion).isNull(); - assertThat(decrypt(rewritten)).containsExactly(plaintext); - } - - @ParameterizedTest - @EnumSource(Config.IntegrityAlgorithm.class) - void hexDigestFileThatCarriesAVersionDecrypts(Config.IntegrityAlgorithm segmentAlgorithm) throws IOException { - // hex digests with a version recorded used to be misread as raw - var plaintext = fourSegmentPlaintext(); - var tdfBytes = createTdf(plaintext, - Config.withTargetMode("4.2.2"), - config -> config.renderVersionInfoInManifest = true, - withSegmentAlgorithm(segmentAlgorithm), - Config.withAssertionConfig(assertionConfig())); - - var manifest = JsonParser.parseString(manifestOf(tdfBytes)).getAsJsonObject(); - assertThat(manifest.get("schemaVersion").getAsString()).isEqualTo(TDF.TDF_SPEC_VERSION); - var rootSignature = Base64.getDecoder().decode(rootSignature(manifest).get("sig").getAsString()); - assertThat(new String(rootSignature, StandardCharsets.UTF_8)).matches("[0-9a-f]{64}"); - - assertThat(decrypt(tdfBytes)).containsExactly(plaintext); - } - - @ParameterizedTest - @ValueSource(strings = { "root", "payload" }) - void hexDigestFileWithVersionUnderTheNonAlignedNameDecrypts(String placement) throws IOException { - var plaintext = fourSegmentPlaintext(); - var tdfBytes = createTdf(plaintext, - Config.withTargetMode("4.2.2"), - config -> config.renderVersionInfoInManifest = true, - Config.withAssertionConfig(assertionConfig())); - var rewritten = rewrite(tdfBytes, manifest -> moveVersionToNonAlignedName(manifest, placement), - UnaryOperator.identity()); - - assertThat(decrypt(rewritten)).containsExactly(plaintext); - } - @Test void legacyHexDigestFileWithAnAssertionStillDecrypts() throws IOException { var plaintext = fourSegmentPlaintext(); @@ -407,27 +362,6 @@ void legacyHexDigestFileWithAnAssertionStillDecrypts() throws IOException { assertThat(decrypt(tdfBytes)).containsExactly(plaintext); } - @ParameterizedTest - @ValueSource(strings = { "4.2.2", "4.3.0" }) - void respellingSegmentHashesWithoutTheKeyIsCaught(String targetMode) throws IOException { - // accepting either spelling of a segment digest does not make the spelling free to - // change: the root signature is over the recorded bytes, so switching raw to hex (or - // hex back to raw) without the key breaks it - var tampered = rewrite(createTdf(fourSegmentPlaintext(), Config.withTargetMode(targetMode)), manifest -> { - for (var element : segments(manifest)) { - var segment = element.getAsJsonObject(); - var recorded = Base64.getDecoder().decode(segment.get("hash").getAsString()); - var respelled = "4.2.2".equals(targetMode) - ? unhex(new String(recorded, StandardCharsets.UTF_8)) - : hex(recorded).getBytes(StandardCharsets.UTF_8); - segment.addProperty("hash", Base64.getEncoder().encodeToString(respelled)); - } - }, UnaryOperator.identity()); - - assertThatThrownBy(() -> decrypt(tampered)) - .isInstanceOf(SDK.RootSignatureValidationException.class); - } - @ParameterizedTest @ValueSource(strings = { "root", "payload" }) void tamperingIsStillCaughtWhenTheVersionIsUnderTheNonAlignedName(String placement) throws IOException { @@ -459,27 +393,6 @@ void tamperingIsStillCaughtWhenTheVersionIsUnderTheNonAlignedName(String placeme .isInstanceOf(SDK.SegmentSignatureMismatch.class); } - @ParameterizedTest - @ValueSource(strings = { "4.2.2", "4.3.0" }) - void tamperingIsStillCaughtInEitherEncodingWhenAVersionIsRecorded(String targetMode) throws IOException { - var original = createTdf(fourSegmentPlaintext(), - Config.withTargetMode(targetMode), - config -> config.renderVersionInfoInManifest = true, - withSegmentAlgorithm(Config.IntegrityAlgorithm.HS256)); - - var editedRootSignature = rewrite(original, manifest -> { - var sig = Base64.getDecoder().decode(rootSignature(manifest).get("sig").getAsString()); - sig[0] ^= 0x01; - rootSignature(manifest).addProperty("sig", Base64.getEncoder().encodeToString(sig)); - }, UnaryOperator.identity()); - assertThatThrownBy(() -> decrypt(editedRootSignature)) - .isInstanceOf(SDK.RootSignatureValidationException.class); - - var editedSegmentBody = rewrite(original, manifest -> { - }, payload -> flipByte(payload, payload.length / 2)); - assertThatThrownBy(() -> decrypt(editedSegmentBody)) - .isInstanceOf(SDK.SegmentSignatureMismatch.class); - } // ------------------------------------------------------------------ config @@ -644,21 +557,7 @@ private static void moveVersionToNonAlignedName(JsonObject manifest, String plac target.add("tdf_spec_version", version); } - private static String hex(byte[] bytes) { - var out = new StringBuilder(bytes.length * 2); - for (var b : bytes) { - out.append(String.format("%02x", b)); - } - return out.toString(); - } - private static byte[] unhex(String hex) { - var out = new byte[hex.length() / 2]; - for (int index = 0; index < out.length; index++) { - out[index] = (byte) Integer.parseInt(hex.substring(2 * index, 2 * index + 2), 16); - } - return out; - } private static JsonObject integrityInformation(JsonObject manifest) {