From c673f073c6d06e9b4f00a4421c79ee83bcd529ff Mon Sep 17 00:00:00 2001 From: Morgan Kleene Date: Tue, 10 Nov 2026 18:33:51 -0500 Subject: [PATCH 1/6] refactor(sdk,cmdline): move HTTP range TDF loading into cmdline (DSPX-5113) Reworks #420 so the HTTP range source lives entirely in cmdline and the SDK is left as it was. All of #420's sdk/ changes are reverted, preserving the later #418 zip hardening and #388 ML-KEM work that had built on top of them. HttpRangeChannel moves to cmdline as a plain SeekableByteChannel handed to the already-public SDK.loadTDF(SeekableByteChannel, TDFReaderConfig), so no SDK API is needed. It builds its own OkHttp client from the public SDK.getTrustManager(), which keeps --insecure working while sending no platform credentials to the TDF host, and owns and shuts that client down on close(). A fresh client is simpler than deriving one from the SDK's: there is no auth interceptor to drop and no --plaintext h2c prior-knowledge mode to undo, so the default HTTP/2 + HTTP/1.1 protocols are already right. decrypt's -f now takes a String and dispatches through openTdf(): an http(s):// URL opens a range channel, anything else a FileChannel. Everything downstream is unchanged, so the decrypt body returns to its pre-#420 shape. supports reports http_range_source. Partial decrypt (DSPX-5115) is dropped rather than moved: decrypting only the segments covering a range needs the payload key, which TDF.Reader keeps private, and the trimmed-manifest workaround fails because load-time root signature verification hashes the full segment list. A cmdline-only version could discard the prefix from a full read, but it would still download everything up to the range, which is the cost the feature exists to avoid. So --offset, --length, -o/--output and the partial_decrypt feature flag are all gone, along with the ZipReader.skip() override that only served them. Public API notes: removes SDK.loadTDF(URI, TDFReaderConfig), SDK.RangeNotSupportedException and TDF.Reader.readPayload(OutputStream, long, long), all added in #420 and not present in any release. Build: okhttp.version moves from sdk/pom.xml to the root properties, shared by sdk and cmdline; cmdline declares okhttp (compile) and mockwebserver (test) with the same exclusions sdk uses, rather than relying on okhttp arriving transitively. Test plan: - mvn -pl cmdline -am test: sdk 419 pass / 9 skipped, sdk-pqc-bc 54 pass, cmdline 24 pass. New cmdline cases cover bytes actually sent over the wire and reading a STORED zip over range requests through the SDK's ZipReader, which replaces the TDFTest HTTP coverage that lived on the SDK side. - mvn -pl cmdline -am enforcer:enforce: all rules pass, including dependencyConvergence, requireUpperBoundDeps and banDuplicateClasses. - cmdline.jar smoke test: supports lists http_range_source, --json matches, decrypt --help shows the new -f description. - Not run end to end against a live platform. Co-Authored-By: Claude Opus 5 Signed-off-by: Morgan Kleene --- cmdline/pom.xml | 33 +++++ .../java/io/opentdf/platform/Command.java | 140 +++++++----------- .../opentdf/platform}/HttpRangeChannel.java | 90 +++++++++-- .../java/io/opentdf/platform/CommandTest.java | 29 +--- .../platform}/HttpRangeChannelTest.java | 99 +++++++++++-- pom.xml | 2 + sdk/pom.xml | 1 - .../java/io/opentdf/platform/sdk/SDK.java | 37 ----- .../io/opentdf/platform/sdk/SDKBuilder.java | 9 +- .../java/io/opentdf/platform/sdk/TDF.java | 120 ++++----------- .../io/opentdf/platform/sdk/TDFReader.java | 26 +--- .../io/opentdf/platform/sdk/ZipReader.java | 9 -- .../opentdf/platform/sdk/TDFReaderTest.java | 29 +--- .../java/io/opentdf/platform/sdk/TDFTest.java | 91 ------------ 14 files changed, 300 insertions(+), 415 deletions(-) rename {sdk/src/main/java/io/opentdf/platform/sdk => cmdline/src/main/java/io/opentdf/platform}/HttpRangeChannel.java (54%) rename {sdk/src/test/java/io/opentdf/platform/sdk => cmdline/src/test/java/io/opentdf/platform}/HttpRangeChannelTest.java (55%) diff --git a/cmdline/pom.xml b/cmdline/pom.xml index 77bf6b3a..640cf8a5 100644 --- a/cmdline/pom.xml +++ b/cmdline/pom.xml @@ -77,7 +77,40 @@ sdk ${project.version} + + + com.squareup.okhttp3 + okhttp + ${okhttp.version} + + + com.squareup.okio + okio-jvm + + + org.jetbrains.kotlin + kotlin-stdlib-jdk8 + + + + + com.squareup.okhttp3 + mockwebserver + ${okhttp.version} + test + + + com.squareup.okio + okio-jvm + + + org.jetbrains.kotlin + kotlin-stdlib-jdk8 + + + org.junit.jupiter junit-jupiter diff --git a/cmdline/src/main/java/io/opentdf/platform/Command.java b/cmdline/src/main/java/io/opentdf/platform/Command.java index c4953843..ebf8d61a 100644 --- a/cmdline/src/main/java/io/opentdf/platform/Command.java +++ b/cmdline/src/main/java/io/opentdf/platform/Command.java @@ -15,16 +15,14 @@ import java.io.File; import java.io.FileInputStream; import java.io.IOException; -import java.io.OutputStream; import java.io.PrintWriter; import java.net.URI; import java.nio.channels.FileChannel; +import java.nio.channels.SeekableByteChannel; import java.nio.charset.StandardCharsets; -import java.nio.file.AtomicMoveNotSupportedException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; -import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; import java.security.KeyFactory; import java.security.NoSuchAlgorithmException; @@ -85,7 +83,7 @@ static class Supports implements Callable { // Always-on capabilities of this build. There is no "known but // unsupported" feature here: anything not in this list is simply unrecognized. private static final List STATIC_FEATURES = List.of("dpop", "dpop_nonce_challenge", - "partial_decrypt", "http_range_source"); + "http_range_source"); // session-key-mlkem additionally requires a KemProvider that can generate // ML-KEM-768 and ML-KEM-1024 session keys (e.g. sdk-pqc-bc), which the @@ -388,10 +386,8 @@ private SDK buildSDK() { @CommandLine.Command(name = "decrypt") void decrypt( - @Option(names = { "-f", "--file" }, required = true, description = "TDF file path, or an http(s):// URL read with range requests") String tdfFile, - @Option(names = { "-o", "--output" }, defaultValue = Option.NULL_VALUE, description = "Write plaintext here, only on success (default: stdout)") Optional output, - @Option(names = { "--offset" }, defaultValue = "0", description = "Plaintext byte offset to start decrypting from") long offset, - @Option(names = { "--length" }, defaultValue = "-1", description = "Plaintext bytes to decrypt from --offset; negative decrypts to the end") long length, + @Option(names = { "-f", + "--file" }, required = true, description = "TDF file path, or an http(s):// URL read with range requests") String tdfFile, @Option(names = { "--rewrap-key-type" }, defaultValue = Option.NULL_VALUE, description = "Preferred rewrap algorithm, one of ${COMPLETION-CANDIDATES}") Optional rewrapKeyType, @Option(names = { @@ -404,95 +400,69 @@ void decrypt( throws Exception { try (var sdk = buildSDK()) { var opts = new ArrayList>(); - if (assertionVerification.isPresent()) { - var assertionVerificationInput = assertionVerification.get(); - Gson gson = buildGson(); - - Config.AssertionVerificationKeys assertionVerificationKeys; - try { - assertionVerificationKeys = gson.fromJson(assertionVerificationInput, - Config.AssertionVerificationKeys.class); - } catch (JsonSyntaxException e) { - // try it as a file path - try { - String fileJson = Files.readString(Paths.get(assertionVerificationInput)); - assertionVerificationKeys = gson.fromJson(fileJson, - Config.AssertionVerificationKeys.class); - } catch (JsonSyntaxException e2) { - throw new RuntimeException("Failed to parse assertion verification keys from file", e2); - } catch (Exception e3) { - throw new RuntimeException( - "Could not parse assertion verification keys as json string or path to file", - e3); + try (var in = openTdf(sdk, tdfFile)) { + try (var stdout = new BufferedOutputStream(System.out)) { + if (assertionVerification.isPresent()) { + var assertionVerificationInput = assertionVerification.get(); + Gson gson = buildGson(); + + Config.AssertionVerificationKeys assertionVerificationKeys; + try { + assertionVerificationKeys = gson.fromJson(assertionVerificationInput, + Config.AssertionVerificationKeys.class); + } catch (JsonSyntaxException e) { + // try it as a file path + try { + String fileJson = Files.readString(Paths.get(assertionVerificationInput)); + assertionVerificationKeys = gson.fromJson(fileJson, + Config.AssertionVerificationKeys.class); + } catch (JsonSyntaxException e2) { + throw new RuntimeException("Failed to parse assertion verification keys from file", e2); + } catch (Exception e3) { + throw new RuntimeException( + "Could not parse assertion verification keys as json string or path to file", + e3); + } + } + + for (Map.Entry entry : assertionVerificationKeys.keys + .entrySet()) { + try { + Object correctedKey = correctKeyType(entry.getValue().alg, entry.getValue().key, true); + entry.setValue(new AssertionConfig.AssertionKey(entry.getValue().alg, correctedKey)); + } catch (Exception e) { + throw new RuntimeException("Error with assertion verification key: " + e.getMessage(), + e); + } + } + opts.add(Config.withAssertionVerificationKeys(assertionVerificationKeys)); } - } - for (Map.Entry entry : assertionVerificationKeys.keys - .entrySet()) { - try { - Object correctedKey = correctKeyType(entry.getValue().alg, entry.getValue().key, true); - entry.setValue(new AssertionConfig.AssertionKey(entry.getValue().alg, correctedKey)); - } catch (Exception e) { - throw new RuntimeException("Error with assertion verification key: " + e.getMessage(), - e); + if (disableAssertionVerification) { + opts.add(Config.withDisableAssertionVerification(true)); } - } - opts.add(Config.withAssertionVerificationKeys(assertionVerificationKeys)); - } - - if (disableAssertionVerification) { - opts.add(Config.withDisableAssertionVerification(true)); - } - rewrapKeyType.map(Config::WithSessionKeyType).ifPresent(opts::add); + rewrapKeyType.map(Config::WithSessionKeyType).ifPresent(opts::add); - ignoreAllowlist.ifPresent(aBoolean -> opts.add(Config.WithIgnoreKasAllowlist(aBoolean))); - kasAllowlistStr.ifPresent(s -> opts.add(Config.WithKasAllowlist(s.split(",")))); + ignoreAllowlist.ifPresent(aBoolean -> opts.add(Config.WithIgnoreKasAllowlist(aBoolean))); + kasAllowlistStr.ifPresent(s -> opts.add(Config.WithKasAllowlist(s.split(",")))); - var readerConfig = Config.newTDFReaderConfig(opts.toArray(new Consumer[0])); - - if (tdfFile.startsWith("http://") || tdfFile.startsWith("https://")) { - var reader = sdk.loadTDF(URI.create(tdfFile), readerConfig); - writePlaintext(out -> reader.readPayload(out, offset, length), output); - } else { - try (var in = FileChannel.open(Path.of(tdfFile), StandardOpenOption.READ)) { + var readerConfig = Config.newTDFReaderConfig(opts.toArray(new Consumer[0])); var reader = sdk.loadTDF(in, readerConfig); - writePlaintext(out -> reader.readPayload(out, offset, length), output); + reader.readPayload(stdout); } } } } - // With an output file, write to a sibling temp file and move it into place only on - // success, so a failed decrypt (e.g. an offset past the end) leaves no file behind. - private interface PlaintextWriter { - void writeTo(OutputStream out) throws Exception; - } - - private static void writePlaintext(PlaintextWriter plaintext, Optional output) throws Exception { - if (output.isEmpty()) { - try (var stdout = new BufferedOutputStream(System.out)) { - plaintext.writeTo(stdout); - } - return; - } - Path target = output.get().toAbsolutePath(); - Path tmp = Files.createTempFile(target.getParent(), target.getFileName() + ".", ".tmp"); - try { - try (OutputStream out = new BufferedOutputStream(Files.newOutputStream(tmp))) { - plaintext.writeTo(out); - } - moveIntoPlace(tmp, target); - } finally { - Files.deleteIfExists(tmp); - } - } - - static void moveIntoPlace(Path source, Path target) throws IOException { - try { - Files.move(source, target, StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE); - } catch (AtomicMoveNotSupportedException e) { - Files.move(source, target, StandardCopyOption.REPLACE_EXISTING); + /** + * Opens the TDF named by {@code -f}: an {@code http(s)://} URL, read in place with range + * requests, or anything else as a local file path. + */ + private static SeekableByteChannel openTdf(SDK sdk, String tdfFile) throws IOException { + if (tdfFile.startsWith("http://") || tdfFile.startsWith("https://")) { + return HttpRangeChannel.open(sdk, URI.create(tdfFile)); } + return FileChannel.open(Path.of(tdfFile), StandardOpenOption.READ); } @CommandLine.Command(name = "metadata") diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/HttpRangeChannel.java b/cmdline/src/main/java/io/opentdf/platform/HttpRangeChannel.java similarity index 54% rename from sdk/src/main/java/io/opentdf/platform/sdk/HttpRangeChannel.java rename to cmdline/src/main/java/io/opentdf/platform/HttpRangeChannel.java index 26152259..03aeceb2 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/HttpRangeChannel.java +++ b/cmdline/src/main/java/io/opentdf/platform/HttpRangeChannel.java @@ -1,39 +1,91 @@ -package io.opentdf.platform.sdk; +package io.opentdf.platform; +import io.opentdf.platform.sdk.SDK; import okhttp3.OkHttpClient; import okhttp3.Request; +import javax.net.ssl.SSLContext; +import javax.net.ssl.TrustManager; +import javax.net.ssl.X509TrustManager; import java.io.IOException; import java.net.URI; import java.nio.ByteBuffer; import java.nio.channels.ClosedChannelException; import java.nio.channels.NonWritableChannelException; import java.nio.channels.SeekableByteChannel; +import java.security.GeneralSecurityException; import java.util.regex.Pattern; /** * A read-only {@link SeekableByteChannel} over a remote resource, where every read is one - * HTTP range request. The size is learned by probing {@code bytes=0-0}, and every later - * response must report the same total size, so a resource that changes underneath the - * reader is rejected rather than spliced together. + * HTTP range request. Handed to {@link SDK#loadTDF(SeekableByteChannel, io.opentdf.platform.sdk.Config.TDFReaderConfig)} + * it lets the SDK read a TDF in place, downloading only the bytes it actually touches. *

- * Redirects are not followed: the client handed in must have redirects disabled, and a 3xx - * is reported as an error so the caller can use the final URL. + * The size is learned by probing {@code bytes=0-0}, and every later response must report the + * same total size, so a resource that changes underneath the reader is rejected rather than + * spliced together. + *

+ * Redirects are not followed, and a 3xx is reported as an error so the caller can use the + * final URL. */ // TODO: Add a read-ahead window if small ZIP header reads become a network bottleneck. class HttpRangeChannel implements SeekableByteChannel { private static final Pattern CONTENT_RANGE = Pattern.compile("bytes (\\d+)-(\\d+)/(\\d+)"); private final OkHttpClient client; + // Set only when this channel built the client, and so must shut it down on close. + private final boolean ownsClient; private final URI url; private final long size; private long position; private boolean open = true; + /** + * Opens {@code url} with a client that is unauthenticated -- platform credentials never go + * to the TDF host -- but that keeps the TLS settings the SDK was built with, so {@code + * --insecure} applies here too. + */ + static HttpRangeChannel open(SDK sdk, URI url) throws IOException { + String scheme = url.getScheme(); + if (!"http".equalsIgnoreCase(scheme) && !"https".equalsIgnoreCase(scheme)) { + throw new IllegalArgumentException("unsupported URL scheme: " + scheme); + } + return new HttpRangeChannel(rangeClient(sdk), true, url); + } + HttpRangeChannel(OkHttpClient client, URI url) throws IOException { + this(client, false, url); + } + + private HttpRangeChannel(OkHttpClient client, boolean ownsClient, URI url) throws IOException { this.client = client; + this.ownsClient = ownsClient; this.url = url; - this.size = fetch(ByteBuffer.allocate(1), 0); + try { + this.size = fetch(ByteBuffer.allocate(1), 0); + } catch (IOException | RuntimeException e) { + shutdown(); + throw e; + } + } + + private static OkHttpClient rangeClient(SDK sdk) { + // A plain client rather than one derived from the SDK's: the SDK's carries the auth + // interceptor, and under --plaintext it is pinned to h2c prior knowledge, while a TDF + // host is an ordinary HTTP server. Default protocols (HTTP/2, HTTP/1.1) are right here. + var builder = new OkHttpClient.Builder().followRedirects(false).followSslRedirects(false); + var trustManager = sdk.getTrustManager().orElse(null); + if (trustManager instanceof X509TrustManager) { + var x509 = (X509TrustManager) trustManager; + try { + var context = SSLContext.getInstance("TLS"); + context.init(null, new TrustManager[] { x509 }, null); + builder.sslSocketFactory(context.getSocketFactory(), x509); + } catch (GeneralSecurityException e) { + throw new IllegalStateException("could not build a TLS context for range requests", e); + } + } + return builder.build(); } @Override @@ -70,7 +122,7 @@ private long fetch(ByteBuffer dst, long offset) throws IOException { try (var response = client.newCall(request).execute()) { int status = response.code(); if (status == 200) { - throw new SDK.RangeNotSupportedException( + throw new RangeNotSupportedException( "status 200 for bytes " + offset + "-" + end); } if (status >= 300 && status < 400) { @@ -83,12 +135,12 @@ private long fetch(ByteBuffer dst, long offset) throws IOException { String contentRange = response.header("Content-Range"); var m = contentRange == null ? null : CONTENT_RANGE.matcher(contentRange); if (m == null || !m.matches()) { - throw new SDK.RangeNotSupportedException("invalid Content-Range " + contentRange); + throw new RangeNotSupportedException("invalid Content-Range " + contentRange); } long gotStart = Long.parseLong(m.group(1)); long gotEnd = Long.parseLong(m.group(2)); if (gotStart != offset || gotEnd != end) { - throw new SDK.RangeNotSupportedException("requested bytes " + offset + "-" + end + throw new RangeNotSupportedException("requested bytes " + offset + "-" + end + ", got " + gotStart + "-" + gotEnd); } var source = response.body().source(); @@ -141,6 +193,14 @@ public boolean isOpen() { @Override public void close() { open = false; + shutdown(); + } + + private void shutdown() { + if (ownsClient) { + client.dispatcher().executorService().shutdown(); + client.connectionPool().evictAll(); + } } private void ensureOpen() throws ClosedChannelException { @@ -148,4 +208,14 @@ private void ensureOpen() throws ClosedChannelException { throw new ClosedChannelException(); } } + + /** + * The server holding a TDF did not honor an HTTP range request, so the TDF cannot be + * read in place; download it and decrypt it from a file instead. + */ + static class RangeNotSupportedException extends IOException { + RangeNotSupportedException(String errorMessage) { + super("server did not honor the HTTP range request: " + errorMessage); + } + } } diff --git a/cmdline/src/test/java/io/opentdf/platform/CommandTest.java b/cmdline/src/test/java/io/opentdf/platform/CommandTest.java index 8820bf32..ceeff75b 100644 --- a/cmdline/src/test/java/io/opentdf/platform/CommandTest.java +++ b/cmdline/src/test/java/io/opentdf/platform/CommandTest.java @@ -2,21 +2,15 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.io.TempDir; import picocli.CommandLine; import java.io.ByteArrayOutputStream; import java.io.PrintStream; import java.io.PrintWriter; import java.io.StringWriter; -import java.net.URI; import java.nio.charset.StandardCharsets; -import java.nio.file.FileSystems; -import java.nio.file.Files; -import java.nio.file.Path; import java.util.ArrayList; import java.util.List; -import java.util.Map; import static org.assertj.core.api.Assertions.assertThat; @@ -60,7 +54,7 @@ private static boolean isClassPresent(String className) { private static List expectedFeatures() { List features = new ArrayList<>( - List.of("dpop", "dpop_nonce_challenge", "partial_decrypt", "http_range_source")); + List.of("dpop", "dpop_nonce_challenge", "http_range_source")); if (MLKEM_PROVIDER_ON_CLASSPATH) { features.add("session-key-mlkem"); } @@ -80,8 +74,7 @@ void supports_dpop_nonce_challenge_exits_0() { } @Test - void supports_partial_decrypt_and_http_range_source_exit_0() { - assertThat(new CommandLine(new Command()).execute("supports", "partial_decrypt")).isEqualTo(0); + void supports_http_range_source_exit_0() { assertThat(new CommandLine(new Command()).execute("supports", "http_range_source")).isEqualTo(0); } @@ -152,7 +145,7 @@ void supports_noArgs_json_listsFeatures() { assertThat(code[0]).isEqualTo(0); String expected = "{\"dpop\":true,\"dpop_nonce_challenge\":true," - + "\"partial_decrypt\":true,\"http_range_source\":true" + + "\"http_range_source\":true" + (MLKEM_PROVIDER_ON_CLASSPATH ? ",\"session-key-mlkem\":true" : "") + "}"; assertThat(out.trim()).isEqualTo(expected); @@ -188,20 +181,4 @@ void supports_unknownFeature_json_false() { assertThat(out.trim()).isEqualTo("{\"unknown_feature\":false}"); } - @Test - void moveIntoPlaceFallsBackWhenAtomicMoveIsUnsupported(@TempDir Path tempDir) throws Exception { - Path source = Files.writeString(tempDir.resolve("plaintext.tmp"), "plaintext"); - Path archive = tempDir.resolve("output.zip"); - - try (var zip = FileSystems.newFileSystem( - URI.create("jar:" + archive.toUri()), Map.of("create", "true"))) { - Path target = zip.getPath("/plaintext.txt"); - - Command.moveIntoPlace(source, target); - - assertThat(Files.readString(target)).isEqualTo("plaintext"); - assertThat(source).doesNotExist(); - } - } - } diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/HttpRangeChannelTest.java b/cmdline/src/test/java/io/opentdf/platform/HttpRangeChannelTest.java similarity index 55% rename from sdk/src/test/java/io/opentdf/platform/sdk/HttpRangeChannelTest.java rename to cmdline/src/test/java/io/opentdf/platform/HttpRangeChannelTest.java index 3985088a..6fe9e10b 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/HttpRangeChannelTest.java +++ b/cmdline/src/test/java/io/opentdf/platform/HttpRangeChannelTest.java @@ -1,20 +1,26 @@ -package io.opentdf.platform.sdk; +package io.opentdf.platform; import okhttp3.OkHttpClient; import okhttp3.mockwebserver.Dispatcher; import okhttp3.mockwebserver.MockResponse; import okhttp3.mockwebserver.MockWebServer; import okhttp3.mockwebserver.RecordedRequest; +import io.opentdf.platform.sdk.ZipReader; import okio.Buffer; import org.junit.jupiter.api.Test; +import java.io.ByteArrayOutputStream; import java.io.IOException; import java.net.URI; import java.nio.ByteBuffer; +import java.nio.channels.ClosedChannelException; import java.nio.charset.StandardCharsets; import java.util.concurrent.atomic.AtomicLong; import java.util.function.Supplier; import java.util.regex.Pattern; +import java.util.zip.CRC32; +import java.util.zip.ZipEntry; +import java.util.zip.ZipOutputStream; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -71,12 +77,41 @@ void readsAndSeeks() throws Exception { } } + /** Only the bytes asked for cross the wire, which is the point of reading a TDF in place. */ + @Test + void readsOnlyTheRequestedBytes() throws Exception { + byte[] data = new byte[100_000]; + var served = new AtomicLong(); + try (var server = new MockWebServer()) { + server.setDispatcher(rangeDispatcher(() -> data, served)); + try (var ch = new HttpRangeChannel(CLIENT, url(server))) { + served.set(0); // discount the one-byte size probe + assertThat(ch.position(50_000).read(ByteBuffer.allocate(1_000))).isEqualTo(1_000); + } + assertThat(served).hasValue(1_000); + } + } + + @Test + void usesAfterCloseAreRejected() throws Exception { + byte[] data = "0123456789".getBytes(StandardCharsets.UTF_8); + try (var server = new MockWebServer()) { + server.setDispatcher(rangeDispatcher(() -> data, new AtomicLong())); + var ch = new HttpRangeChannel(CLIENT, url(server)); + ch.close(); + + assertThat(ch.isOpen()).isFalse(); + assertThatThrownBy(() -> ch.read(ByteBuffer.allocate(1))) + .isInstanceOf(ClosedChannelException.class); + } + } + @Test void rangeNotSupported() throws Exception { try (var server = new MockWebServer()) { server.enqueue(new MockResponse().setResponseCode(200).setBody("not a partial response")); assertThatThrownBy(() -> new HttpRangeChannel(CLIENT, url(server))) - .isInstanceOf(SDK.RangeNotSupportedException.class); + .isInstanceOf(HttpRangeChannel.RangeNotSupportedException.class); } } @@ -86,7 +121,7 @@ void mismatchedContentRangeIsRangeNotSupported() throws Exception { server.enqueue(new MockResponse().setResponseCode(206) .setHeader("Content-Range", "bytes 0-9/10").setBody("0123456789")); assertThatThrownBy(() -> new HttpRangeChannel(CLIENT, url(server))) - .isInstanceOf(SDK.RangeNotSupportedException.class); + .isInstanceOf(HttpRangeChannel.RangeNotSupportedException.class); } } @@ -97,7 +132,7 @@ void errorStatusesReportTheirStatus() throws Exception { server.enqueue(new MockResponse().setResponseCode(status).setHeader("Location", "/elsewhere")); assertThatThrownBy(() -> new HttpRangeChannel(CLIENT, url(server))) .isInstanceOf(IOException.class) - .isNotInstanceOf(SDK.RangeNotSupportedException.class) + .isNotInstanceOf(HttpRangeChannel.RangeNotSupportedException.class) .hasMessageContaining(String.valueOf(status)) .satisfies(e -> { if (status == 302) { @@ -121,13 +156,57 @@ void sizeChanged() throws Exception { } } + /** + * The SDK reads a TDF as a zip over whatever channel it is handed, so the channel has to + * satisfy ZipReader's seek-heavy access pattern -- not just the reads a test drives by hand. + */ @Test - void rejectsNonHttpSchemes() { - var sdk = new SDK(new FakeServicesBuilder().build(), null, null, null, null, null); - var uri = URI.create("file:///etc/passwd"); - var config = Config.newTDFReaderConfig(); + void readsAZipArchiveThroughZipReader() throws Exception { + var zip = new ByteArrayOutputStream(); + byte[] payload = new byte[64_000]; + new java.util.Random(1).nextBytes(payload); + // STORED, like TDFWriter writes: ZipReader hands back entry bytes as they sit in the + // archive and does not inflate. + try (var out = new ZipOutputStream(zip)) { + out.setMethod(ZipOutputStream.STORED); + out.putNextEntry(stored("0.manifest.json", "{\"hello\":\"world\"}".getBytes(StandardCharsets.UTF_8))); + out.write("{\"hello\":\"world\"}".getBytes(StandardCharsets.UTF_8)); + out.putNextEntry(stored("0.payload", payload)); + out.write(payload); + } + byte[] archive = zip.toByteArray(); + + try (var server = new MockWebServer()) { + server.setDispatcher(rangeDispatcher(() -> archive, new AtomicLong())); + try (var ch = new HttpRangeChannel(CLIENT, url(server))) { + var entries = new ZipReader(ch).getEntries(); + + assertThat(entries).extracting(ZipReader.Entry::getName) + .containsExactlyInAnyOrder("0.manifest.json", "0.payload"); + var found = entries.stream().filter(e -> e.getName().equals("0.payload")).findFirst().orElseThrow(); + try (var data = found.getData()) { + assertThat(data.readAllBytes()).isEqualTo(payload); + } + } + } + } - assertThatThrownBy(() -> sdk.loadTDF(uri, config)) - .isInstanceOf(IllegalArgumentException.class); + private static ZipEntry stored(String name, byte[] data) { + var crc = new CRC32(); + crc.update(data); + var entry = new ZipEntry(name); + entry.setMethod(ZipEntry.STORED); + entry.setSize(data.length); + entry.setCompressedSize(data.length); + entry.setCrc(crc.getValue()); + return entry; + } + + /** The scheme is checked before the SDK is touched, so a null SDK is enough here. */ + @Test + void rejectsNonHttpSchemes() { + assertThatThrownBy(() -> HttpRangeChannel.open(null, URI.create("file:///etc/passwd"))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("file"); } } diff --git a/pom.xml b/pom.xml index 485744f7..9da9bb3e 100644 --- a/pom.xml +++ b/pom.xml @@ -16,6 +16,8 @@ 2.20.0 1.78.0 4.36.2 + + 4.12.0 1.85 2.1.3 diff --git a/sdk/pom.xml b/sdk/pom.xml index b1fbf506..1850241b 100644 --- a/sdk/pom.xml +++ b/sdk/pom.xml @@ -15,7 +15,6 @@ https://github.com/CodeIntelligenceTesting/jazzer/releases/download/v${jazzer.version} 2.1.0 0.7.2 - 4.12.0 protocol/go/v0.39.0 diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java b/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java index b26f1f9e..633f1040 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java @@ -27,7 +27,6 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; -import java.net.URI; import java.nio.channels.SeekableByteChannel; import java.util.ArrayList; import java.util.Collections; @@ -36,8 +35,6 @@ import java.util.Optional; import java.util.stream.Collectors; -import okhttp3.OkHttpClient; - /** * The SDK class represents a software development kit for interacting with the * opentdf platform. It @@ -60,8 +57,6 @@ public class SDK implements AutoCloseable { private final String platformUrl; private final ProtocolClient platformServicesClient; private final SrtSigner srtSigner; - // Fetches TDFs by URL. Unauthenticated: platform credentials never go to the TDF host. - private final OkHttpClient rangeClient; /** * Closes the SDK, including its associated services. @@ -71,8 +66,6 @@ public class SDK implements AutoCloseable { @Override public void close() throws Exception { services.close(); - rangeClient.dispatcher().executorService().shutdown(); - rangeClient.connectionPool().evictAll(); } /** @@ -121,12 +114,6 @@ public Optional getAuthInterceptor() { } SDK(Services services, TrustManager trustManager, Interceptor authInterceptor, ProtocolClient platformServicesClient, String platformUrl, SrtSigner srtSigner) { - this(services, trustManager, authInterceptor, platformServicesClient, platformUrl, srtSigner, - new OkHttpClient.Builder().followRedirects(false).followSslRedirects(false).build()); - } - - SDK(Services services, TrustManager trustManager, Interceptor authInterceptor, ProtocolClient platformServicesClient, String platformUrl, SrtSigner srtSigner, OkHttpClient rangeClient) { - this.rangeClient = rangeClient; this.platformUrl = platformUrl; this.services = services; this.trustManager = trustManager; @@ -153,20 +140,6 @@ public TDF.Reader loadTDF(SeekableByteChannel channel, Config.TDFReaderConfig co return tdf.loadTDF(channel, config, platformUrl); } - /** - * Loads a TDF served over HTTP(S), reading it with range requests so only the zip - * directory, the manifest, and the payload segments actually read are downloaded. - * The server must answer range requests with 206; redirects are not followed. - * - * @throws RangeNotSupportedException if the server ignores range requests - */ - public TDF.Reader loadTDF(URI url, Config.TDFReaderConfig config) throws SDKException, IOException { - if (!"http".equalsIgnoreCase(url.getScheme()) && !"https".equalsIgnoreCase(url.getScheme())) { - throw new IllegalArgumentException("unsupported URL scheme: " + url.getScheme()); - } - return loadTDF(new HttpRangeChannel(rangeClient, url), config); - } - public Manifest createTDF(InputStream payload, OutputStream outputStream, Config.TDFConfig config) throws SDKException, IOException { var tdf = new TDF(services); return tdf.createTDF(payload, outputStream, config).getManifest(); @@ -412,16 +385,6 @@ public boolean attributeValueExists(String valueFqn) { return resp.getFqnAttributeValuesMap().containsKey(valueFqn); } - /** - * The server holding a TDF did not honor an HTTP range request, so the TDF cannot be - * read in place; download it and load it from a file instead. - */ - public static class RangeNotSupportedException extends IOException { - public RangeNotSupportedException(String errorMessage) { - super("server did not honor the HTTP range request: " + errorMessage); - } - } - /** * Indicates that the TDF is malformed in some way */ diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/SDKBuilder.java b/sdk/src/main/java/io/opentdf/platform/sdk/SDKBuilder.java index cac363d0..dda13258 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/SDKBuilder.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/SDKBuilder.java @@ -483,14 +483,7 @@ private KASClient getKASClient(SrtSigner srtSigner, AuthInterceptor interceptor, public SDK build() { var services = buildServices(); - return new SDK(services.services, services.trustManager, services.interceptor, services.protocolClient, platformEndpoint, services.srtSigner, - // getHttpClient() carries the TLS settings but no credentials. Drop the h2c prior - // knowledge plaintext mode forces, since TDF hosts are ordinary HTTP servers. - getHttpClient().newBuilder() - .protocols(List.of(Protocol.HTTP_2, Protocol.HTTP_1_1)) - .followRedirects(false) - .followSslRedirects(false) - .build()); + return new SDK(services.services, services.trustManager, services.interceptor, services.protocolClient, platformEndpoint, services.srtSigner); } private ProtocolClient getUnauthenticatedProtocolClient(String endpoint, OkHttpClient httpClient) { 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 dd000a8d..4ad24298 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java @@ -397,41 +397,6 @@ public Manifest getManifest() { } public void readPayload(OutputStream outputStream) throws SDK.TamperException, IOException { - readPayload(outputStream, 0, -1); - } - - /** - * Writes the plaintext bytes {@code [offset, offset + length)} to {@code outputStream}. - * Only the segments covering the range are read, verified and decrypted, so over - * {@link SDK#loadTDF(java.net.URI, Config.TDFReaderConfig)} only those are downloaded. - * - * @param offset plaintext offset to start at; equal to the payload size writes nothing - * @param length number of bytes to write; negative means to the end of the payload, and - * a range running past the end is clamped to it - * @throws IllegalArgumentException if {@code offset} is negative or beyond the end of the - * payload, before anything is written - */ - public void readPayload(OutputStream outputStream, long offset, long length) - throws SDK.TamperException, IOException { - if (offset < 0) { - throw new IllegalArgumentException("offset must not be negative: " + offset); - } - - validateSegmentSizes(); - - List segments = manifest.encryptionInformation.integrityInformation.segments; - long total = 0; - for (int i = 0; i < segments.size(); i++) { - total += plaintextSize(Manifest.segmentAt(segments, i)); - } - if (offset > total) { - throw new IllegalArgumentException( - "offset " + offset + " is beyond the end of the payload (" + total + " bytes)"); - } - long end = length < 0 || length > total - offset ? total : offset + length; - if (offset == end) { - return; - } MessageDigest digest = null; try { @@ -440,70 +405,45 @@ public void readPayload(OutputStream outputStream, long offset, long length) throw new IllegalStateException("error getting instance of SHA-256", e); } - try (var payload = tdfReader.payload()) { - long plainStart = 0; - for (int i = 0; i < segments.size(); i++) { - if (plainStart >= end) { - break; - } - Manifest.Segment segment = Manifest.segmentAt(segments, i); - long plainEnd = plainStart + plaintextSize(segment); - // <=, not <: a segment ending exactly at offset holds none of the range - if (plainEnd <= offset) { - if (payload.skip(segment.encryptedSegmentSize) != segment.encryptedSegmentSize) { - throw new IllegalStateException("unable to skip segment of " - + segment.encryptedSegmentSize + " bytes"); - } - } else { - byte[] plain = readSegment(payload, segment, digest); - int from = (int) Math.max(0, offset - plainStart); - int to = (int) Math.min(plain.length, end - plainStart); - outputStream.write(plain, from, to - from); - } - plainStart = plainEnd; - } - } - } + validateSegmentSizes(); - // Derived from the ciphertext length rather than trusting the manifest's segmentSize, - // so a range lands on the bytes decryption actually produces. - private long plaintextSize(Manifest.Segment segment) { - return segment.encryptedSegmentSize - - (manifest.payload.isEncrypted ? kGcmIvSize + AesGcm.GCM_TAG_LENGTH : 0); - } + List segments = manifest.encryptionInformation.integrityInformation.segments; + for (int i = 0; i < segments.size(); i++) { + Manifest.Segment segment = Manifest.segmentAt(segments, i); + byte[] readBuf = new byte[(int) segment.encryptedSegmentSize]; + int bytesRead = tdfReader.readPayloadBytes(readBuf); - private byte[] readSegment(InputStream payload, Manifest.Segment segment, MessageDigest digest) { - byte[] readBuf = new byte[(int) segment.encryptedSegmentSize]; - int bytesRead = TDFReader.readPayloadBytes(payload, readBuf); + if (readBuf.length != bytesRead) { + throw new IllegalStateException("unable to read bytes for segment (wanted " + + segment.encryptedSegmentSize + " but got " + bytesRead + ")"); + } - if (readBuf.length != bytesRead) { - throw new IllegalStateException("unable to read bytes for segment (wanted " - + segment.encryptedSegmentSize + " but got " + bytesRead + ")"); - } + var isLegacyTdf = manifest.tdfVersion == null || manifest.tdfVersion.isEmpty(); - var isLegacyTdf = manifest.tdfVersion == null || manifest.tdfVersion.isEmpty(); + if (manifest.payload.isEncrypted) { + var sigAlg = segmentIntegrityAlgorithmFromManifest( + manifest.encryptionInformation.integrityInformation.segmentHashAlg); - if (manifest.payload.isEncrypted) { - var sigAlg = segmentIntegrityAlgorithmFromManifest( - manifest.encryptionInformation.integrityInformation.segmentHashAlg); + var payloadSig = calculateSignature(readBuf, payloadKey, sigAlg); + if (isLegacyTdf) { + payloadSig = Hex.encodeHexString(payloadSig).getBytes(StandardCharsets.UTF_8); + } - var payloadSig = calculateSignature(readBuf, payloadKey, sigAlg); - 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"); + } - if (segment.hash.compareTo(Base64.getEncoder().encodeToString(payloadSig)) != 0) { - throw new SDK.SegmentSignatureMismatch("segment signature miss match"); - } + byte[] writeBuf = aesGcm.decrypt(new AesGcm.Encrypted(readBuf)); + outputStream.write(writeBuf); - return aesGcm.decrypt(new AesGcm.Encrypted(readBuf)); - } else { - String segmentSig = Hex.encodeHexString(digest.digest(readBuf)); - if (segment.hash.compareTo(segmentSig) != 0) { - throw new SDK.SegmentSignatureMismatch("segment signature miss match"); - } + } else { + String segmentSig = Hex.encodeHexString(digest.digest(readBuf)); + if (segment.hash.compareTo(segmentSig) != 0) { + throw new SDK.SegmentSignatureMismatch("segment signature miss match"); + } - return readBuf; + outputStream.write(readBuf); + } } } diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java index bb92fe38..0f669962 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java @@ -26,8 +26,7 @@ public class TDFReader { private static final int MANIFEST_BUFFER_SIZE = 1 << 16; private final ZipReader.Entry manifestEntry; - private final ZipReader.Entry payloadEntry; - private InputStream payload; + private final InputStream payload; public TDFReader(SeekableByteChannel tdf) throws SDKException, IOException { // A zip may legally list the same name twice, and readers disagree about which copy @@ -53,7 +52,7 @@ public TDFReader(SeekableByteChannel tdf) throws SDKException, IOException { } manifestEntry = manifest; - payloadEntry = entries.get(TDF_PAYLOAD_FILE_NAME); + payload = entries.get(TDF_PAYLOAD_FILE_NAME).getData(); } /** Like the constructor, but reports a malformed container as {@link SDK.MalformedTDFException}. */ @@ -105,28 +104,7 @@ Reader manifest() { } } - /** A fresh stream over the payload, positioned at its start. */ - InputStream payload() throws IOException { - try { - return payloadEntry.getData(); - } catch (InvalidZipException e) { - // the payload's local header puts its data past the end of the archive - throw malformed("error reading from payload in TDF", e); - } - } - int readPayloadBytes(byte[] buf) { - if (payload == null) { - try { - payload = payload(); - } catch (IOException e) { - throw new SDKException("error reading from payload in TDF", e); - } - } - return readPayloadBytes(payload, buf); - } - - static int readPayloadBytes(InputStream payload, byte[] buf) { int totalRead = 0; int nread; try { diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java index 634af2b5..c1d24e39 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java @@ -451,15 +451,6 @@ private boolean doneReading() { return offset >= fileSize; } - // Moves the cursor without reading: the default skip reads and discards, which - // over HTTP would download everything skipped. - @Override - public long skip(long n) { - long skipped = Math.max(0, Math.min(n, fileSize - offset)); - offset += skipped; - return skipped; - } - /** * The archive ended before this entry's data did. Reporting that as an ordinary * end of stream would hand the caller a short entry with no sign anything is wrong. diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java index 494283a6..42635294 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java @@ -16,9 +16,6 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.verify; /** * The OpenTDF spec names the manifest entry {@code manifest.json}; this SDK writes @@ -216,20 +213,6 @@ void readsThePayloadAlongsideASpecNamedManifest() throws IOException { } } - @Test - void opensThePayloadOnlyWhenItIsRead() throws IOException { - try (var tdf = spy(archiveOf( - TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD, - "manifest.json", MANIFEST))) { - var reader = new TDFReader(tdf); - - verify(tdf, never()).position(0); - - reader.readPayloadBytes(new byte[PAYLOAD.length()]); - verify(tdf).position(0); - } - } - /** An oversized payload is only detected on read, and still surfaces as a malformed TDF. */ @Test void reportsATruncatedPayloadAsMalformed() throws IOException { @@ -245,7 +228,10 @@ void reportsATruncatedPayloadAsMalformed() throws IOException { } } - /** The check on the local header runs when the payload is opened, before any byte is read. */ + /** + * The check on the local header runs when the payload stream is opened, which the + * constructor does, so the whole TDF is rejected before any byte is read. + */ @Test void reportsAPayloadWhoseDataStartsPastTheEndAsMalformed() throws IOException { var payload = new HostileZips.Entry(TDFWriter.TDF_PAYLOAD_FILE_NAME, new byte[0]); @@ -254,12 +240,7 @@ void reportsAPayloadWhoseDataStartsPastTheEndAsMalformed() throws IOException { // the filename length of the first local file header, which starts at offset 0 ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN).putShort(26, (short) 0xFFFF); try (var tdf = new SeekableInMemoryByteChannel(archive)) { - var reader = new TDFReader(tdf); - - assertThatThrownBy(reader::payload) - .isInstanceOf(SDK.MalformedTDFException.class) - .hasCauseInstanceOf(InvalidZipException.class); - assertThatThrownBy(() -> reader.readPayloadBytes(new byte[1])) + assertThatThrownBy(() -> TDFReader.open(tdf)) .isInstanceOf(SDK.MalformedTDFException.class) .hasCauseInstanceOf(InvalidZipException.class); } diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java index cbc497de..0ddb0b9d 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java @@ -13,12 +13,10 @@ import io.opentdf.platform.policy.kasregistry.ListKeyAccessServersRequest; import io.opentdf.platform.policy.kasregistry.ListKeyAccessServersResponse; import io.opentdf.platform.sdk.TDF.Reader; -import okhttp3.mockwebserver.MockWebServer; import org.apache.commons.compress.utils.SeekableInMemoryByteChannel; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.ValueSource; import javax.annotation.Nonnull; @@ -41,7 +39,6 @@ import java.util.Random; import java.util.concurrent.Executors; import java.util.concurrent.Future; -import java.util.concurrent.atomic.AtomicLong; import java.util.function.Consumer; import java.util.function.Predicate; import java.util.regex.Pattern; @@ -739,94 +736,6 @@ public void testCreatingTDFWithMultipleSegments() throws Exception { assertThat(reader.getMetadata()).isEqualTo(metadata); } - private static final int SEG = Config.MIN_SEGMENT_SIZE; - - /** A TDF of {@code 2.5 * SEG} random bytes, so it has two full segments and a partial one. */ - private static byte[][] partialReadFixture() throws Exception { - var data = new byte[SEG * 5 / 2]; - new Random().nextBytes(data); - var tdfOutputStream = new ByteArrayOutputStream(); - var tdf = new TDF(new FakeServicesBuilder().setKas(kas) - .setKeyAccessServerRegistryService(kasRegistryService).build()); - tdf.createTDF(new ByteArrayInputStream(data), tdfOutputStream, Config.newTDFConfig( - Config.withAutoconfigure(false), - Config.withKasInformation(getSingleRSAKASInfo()), - Config.withSegmentSize(SEG))); - return new byte[][] { data, tdfOutputStream.toByteArray() }; - } - - private static Reader loadFixture(byte[] tdfBytes) throws Exception { - var tdf = new TDF(new FakeServicesBuilder().setKas(kas) - .setKeyAccessServerRegistryService(kasRegistryService).build()); - return tdf.loadTDF(new SeekableInMemoryByteChannel(tdfBytes), platformUrl); - } - - @ParameterizedTest - @CsvSource({ - "0, -1", // whole payload - "16334, 100", // crosses the first segment boundary - "16384, 100", // starts exactly where a segment ends - "16384, 16384", // exactly the second segment - "100, 0", // empty - "32768, -1", // last segment to the end - "40950, 1000", // runs past the end: clamped - "40960, -1", // offset == size: empty - }) - void readPayloadRange(long offset, long length) throws Exception { - var fixture = partialReadFixture(); - byte[] data = fixture[0]; - var out = new ByteArrayOutputStream(); - loadFixture(fixture[1]).readPayload(out, offset, length); - - int end = length < 0 ? data.length : (int) Math.min(data.length, offset + length); - assertThat(out.toByteArray()).containsExactly(Arrays.copyOfRange(data, (int) offset, end)); - } - - @Test - void readPayloadRangeCanBeRepeatedOnOneReader() throws Exception { - var fixture = partialReadFixture(); - var reader = loadFixture(fixture[1]); - for (int i = 0; i < 2; i++) { - var out = new ByteArrayOutputStream(); - reader.readPayload(out, SEG, 10); - assertThat(out.toByteArray()).containsExactly(Arrays.copyOfRange(fixture[0], SEG, SEG + 10)); - } - } - - @Test - void readPayloadRangeRejectsBadOffsetsBeforeWriting() throws Exception { - var fixture = partialReadFixture(); - var reader = loadFixture(fixture[1]); - var out = new ByteArrayOutputStream(); - assertThatThrownBy(() -> reader.readPayload(out, fixture[0].length + 1L, -1)) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("beyond the end"); - assertThatThrownBy(() -> reader.readPayload(out, -1, -1)) - .isInstanceOf(IllegalArgumentException.class); - assertThat(out.size()).isZero(); - } - - @Test - void readPayloadRangeOverHttpFetchesOnlyCoveringSegments() throws Exception { - var fixture = partialReadFixture(); - var served = new AtomicLong(); - try (var server = new MockWebServer()) { - server.setDispatcher(HttpRangeChannelTest.rangeDispatcher(() -> fixture[1], served)); - var tdf = new TDF(new FakeServicesBuilder().setKas(kas) - .setKeyAccessServerRegistryService(kasRegistryService).build()); - var reader = tdf.loadTDF(new HttpRangeChannel(HttpRangeChannelTest.CLIENT, server.url("/x.tdf").uri()), - Config.newTDFReaderConfig(), platformUrl); - - served.set(0); - var out = new ByteArrayOutputStream(); - reader.readPayload(out, 2L * SEG - 50, 100); - assertThat(out.toByteArray()).containsExactly(Arrays.copyOfRange(fixture[0], 2 * SEG - 50, 2 * SEG + 50)); - // the second (full) and third (half) segments, plus the payload entry's local header - long encryptedSegments = (SEG + 28) + (SEG / 2 + 28); - assertThat(served.get()).isBetween(encryptedSegments, encryptedSegments + 1024); - } - } - /** * A segment size small enough that the test payloads below span several segments. Matches * web-sdk's {@code DEFAULT_SEGMENT_SIZE}, though nothing here depends on the exact value. From 7c92d2546604827d4be0ec8eacd3c63c47364781 Mon Sep 17 00:00:00 2001 From: Morgan Kleene Date: Wed, 11 Nov 2026 09:28:18 -0500 Subject: [PATCH 2/6] feat(sdk,cmdline): restore ranged readPayload and cmdline partial decrypt (DSPX-5115) Puts partial decrypt back, with the minimum SDK surface that makes it mean what xtest expects. The HTTP range source stays entirely in cmdline. Restores TDF.Reader.readPayload(out, offset, length), the lazy payload stream in TDFReader that lets one reader serve several reads, and the ZipReader.skip() override. Segments before the range are stepped over rather than read, so reading a slice costs O(length) instead of O(offset + length) -- over a channel that fetches on demand, the difference between pulling a slice and downloading the whole payload. This cannot be done from cmdline alone: skipping segments needs the payload key, which TDF.Reader keeps private, and trimming the manifest's segment list instead fails because load-time root signature verification hashes the full list under that same key. So this is the one SDK change the feature needs; SDK.loadTDF(URI, ...), RangeNotSupportedException and the SDKBuilder range client stay out of the SDK and remain cmdline's. ZipReader.skip() also gains a check #420 did not have. Reading is what normally discovers that an archive ends early, so a skip that only moves the cursor stops noticing: a truncated entry skipped to its end used to report a clean EOF and never reach truncated(). The bytes stepped over are now checked against the channel, which costs nothing over HTTP since the size is already known. cmdline regains --offset, --length and -o/--output, with -o writing to a sibling temp file moved into place only on success, and supports reports partial_decrypt again. Test plan: - mvn -pl cmdline -am test: sdk 433 pass / 9 skipped (was 419; +12 ranged-read cases and +2 for skip), sdk-pqc-bc 54 pass, cmdline 25 pass. - The SDK's covering-segments test no longer needs MockWebServer: it counts bytes pulled through a SeekableByteChannel, which is the invariant that matters and keeps HTTP out of the SDK tests. cmdline still asserts the bytes-on-the-wire property against a real server. - mvn -pl cmdline -am enforcer:enforce: all rules pass. - cmdline.jar: supports --json reports partial_decrypt, decrypt usage shows --offset/--length/-o. - Not run end to end against a live platform. Co-Authored-By: Claude Opus 5 Signed-off-by: Morgan Kleene --- .../java/io/opentdf/platform/Command.java | 120 ++++++++++++------ .../java/io/opentdf/platform/CommandTest.java | 30 ++++- .../java/io/opentdf/platform/sdk/TDF.java | 120 +++++++++++++----- .../io/opentdf/platform/sdk/TDFReader.java | 26 +++- .../io/opentdf/platform/sdk/ZipReader.java | 21 +++ .../opentdf/platform/sdk/TDFReaderTest.java | 29 ++++- .../java/io/opentdf/platform/sdk/TDFTest.java | 110 ++++++++++++++++ .../opentdf/platform/sdk/ZipReaderTest.java | 35 +++++ 8 files changed, 411 insertions(+), 80 deletions(-) diff --git a/cmdline/src/main/java/io/opentdf/platform/Command.java b/cmdline/src/main/java/io/opentdf/platform/Command.java index ebf8d61a..aaf68b87 100644 --- a/cmdline/src/main/java/io/opentdf/platform/Command.java +++ b/cmdline/src/main/java/io/opentdf/platform/Command.java @@ -15,14 +15,17 @@ import java.io.File; import java.io.FileInputStream; import java.io.IOException; +import java.io.OutputStream; import java.io.PrintWriter; import java.net.URI; import java.nio.channels.FileChannel; import java.nio.channels.SeekableByteChannel; import java.nio.charset.StandardCharsets; +import java.nio.file.AtomicMoveNotSupportedException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; import java.security.KeyFactory; import java.security.NoSuchAlgorithmException; @@ -83,7 +86,7 @@ static class Supports implements Callable { // Always-on capabilities of this build. There is no "known but // unsupported" feature here: anything not in this list is simply unrecognized. private static final List STATIC_FEATURES = List.of("dpop", "dpop_nonce_challenge", - "http_range_source"); + "partial_decrypt", "http_range_source"); // session-key-mlkem additionally requires a KemProvider that can generate // ML-KEM-768 and ML-KEM-1024 session keys (e.g. sdk-pqc-bc), which the @@ -388,6 +391,12 @@ private SDK buildSDK() { void decrypt( @Option(names = { "-f", "--file" }, required = true, description = "TDF file path, or an http(s):// URL read with range requests") String tdfFile, + @Option(names = { "-o", + "--output" }, defaultValue = Option.NULL_VALUE, description = "Write plaintext here, only on success (default: stdout)") Optional output, + @Option(names = { + "--offset" }, defaultValue = "0", description = "Plaintext byte offset to start decrypting from") long offset, + @Option(names = { + "--length" }, defaultValue = "-1", description = "Plaintext bytes to decrypt from --offset; negative decrypts to the end") long length, @Option(names = { "--rewrap-key-type" }, defaultValue = Option.NULL_VALUE, description = "Preferred rewrap algorithm, one of ${COMPLETION-CANDIDATES}") Optional rewrapKeyType, @Option(names = { @@ -401,59 +410,90 @@ void decrypt( try (var sdk = buildSDK()) { var opts = new ArrayList>(); try (var in = openTdf(sdk, tdfFile)) { - try (var stdout = new BufferedOutputStream(System.out)) { - if (assertionVerification.isPresent()) { - var assertionVerificationInput = assertionVerification.get(); - Gson gson = buildGson(); + if (assertionVerification.isPresent()) { + var assertionVerificationInput = assertionVerification.get(); + Gson gson = buildGson(); - Config.AssertionVerificationKeys assertionVerificationKeys; + Config.AssertionVerificationKeys assertionVerificationKeys; + try { + assertionVerificationKeys = gson.fromJson(assertionVerificationInput, + Config.AssertionVerificationKeys.class); + } catch (JsonSyntaxException e) { + // try it as a file path try { - assertionVerificationKeys = gson.fromJson(assertionVerificationInput, + String fileJson = Files.readString(Paths.get(assertionVerificationInput)); + assertionVerificationKeys = gson.fromJson(fileJson, Config.AssertionVerificationKeys.class); - } catch (JsonSyntaxException e) { - // try it as a file path - try { - String fileJson = Files.readString(Paths.get(assertionVerificationInput)); - assertionVerificationKeys = gson.fromJson(fileJson, - Config.AssertionVerificationKeys.class); - } catch (JsonSyntaxException e2) { - throw new RuntimeException("Failed to parse assertion verification keys from file", e2); - } catch (Exception e3) { - throw new RuntimeException( - "Could not parse assertion verification keys as json string or path to file", - e3); - } + } catch (JsonSyntaxException e2) { + throw new RuntimeException("Failed to parse assertion verification keys from file", e2); + } catch (Exception e3) { + throw new RuntimeException( + "Could not parse assertion verification keys as json string or path to file", + e3); } + } - for (Map.Entry entry : assertionVerificationKeys.keys - .entrySet()) { - try { - Object correctedKey = correctKeyType(entry.getValue().alg, entry.getValue().key, true); - entry.setValue(new AssertionConfig.AssertionKey(entry.getValue().alg, correctedKey)); - } catch (Exception e) { - throw new RuntimeException("Error with assertion verification key: " + e.getMessage(), - e); - } + for (Map.Entry entry : assertionVerificationKeys.keys + .entrySet()) { + try { + Object correctedKey = correctKeyType(entry.getValue().alg, entry.getValue().key, true); + entry.setValue(new AssertionConfig.AssertionKey(entry.getValue().alg, correctedKey)); + } catch (Exception e) { + throw new RuntimeException("Error with assertion verification key: " + e.getMessage(), + e); } - opts.add(Config.withAssertionVerificationKeys(assertionVerificationKeys)); } + opts.add(Config.withAssertionVerificationKeys(assertionVerificationKeys)); + } - if (disableAssertionVerification) { - opts.add(Config.withDisableAssertionVerification(true)); - } - rewrapKeyType.map(Config::WithSessionKeyType).ifPresent(opts::add); + if (disableAssertionVerification) { + opts.add(Config.withDisableAssertionVerification(true)); + } + rewrapKeyType.map(Config::WithSessionKeyType).ifPresent(opts::add); - ignoreAllowlist.ifPresent(aBoolean -> opts.add(Config.WithIgnoreKasAllowlist(aBoolean))); - kasAllowlistStr.ifPresent(s -> opts.add(Config.WithKasAllowlist(s.split(",")))); + ignoreAllowlist.ifPresent(aBoolean -> opts.add(Config.WithIgnoreKasAllowlist(aBoolean))); + kasAllowlistStr.ifPresent(s -> opts.add(Config.WithKasAllowlist(s.split(",")))); - var readerConfig = Config.newTDFReaderConfig(opts.toArray(new Consumer[0])); - var reader = sdk.loadTDF(in, readerConfig); - reader.readPayload(stdout); - } + var readerConfig = Config.newTDFReaderConfig(opts.toArray(new Consumer[0])); + var reader = sdk.loadTDF(in, readerConfig); + writePlaintext(out -> reader.readPayload(out, offset, length), output); } } } + // With an output file, write to a sibling temp file and move it into place only on + // success, so a failed decrypt (e.g. an offset past the end) leaves no file behind. + private interface PlaintextWriter { + void writeTo(OutputStream out) throws Exception; + } + + private static void writePlaintext(PlaintextWriter plaintext, Optional output) throws Exception { + if (output.isEmpty()) { + try (var stdout = new BufferedOutputStream(System.out)) { + plaintext.writeTo(stdout); + } + return; + } + Path target = output.get().toAbsolutePath(); + Path tmp = Files.createTempFile(target.getParent(), target.getFileName() + ".", ".tmp"); + try { + try (OutputStream out = new BufferedOutputStream(Files.newOutputStream(tmp))) { + plaintext.writeTo(out); + } + moveIntoPlace(tmp, target); + } finally { + Files.deleteIfExists(tmp); + } + } + + static void moveIntoPlace(Path source, Path target) throws IOException { + try { + Files.move(source, target, StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE); + } catch (AtomicMoveNotSupportedException e) { + Files.move(source, target, StandardCopyOption.REPLACE_EXISTING); + } + } + /** * Opens the TDF named by {@code -f}: an {@code http(s)://} URL, read in place with range * requests, or anything else as a local file path. diff --git a/cmdline/src/test/java/io/opentdf/platform/CommandTest.java b/cmdline/src/test/java/io/opentdf/platform/CommandTest.java index ceeff75b..eb791afd 100644 --- a/cmdline/src/test/java/io/opentdf/platform/CommandTest.java +++ b/cmdline/src/test/java/io/opentdf/platform/CommandTest.java @@ -2,15 +2,21 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import picocli.CommandLine; import java.io.ByteArrayOutputStream; import java.io.PrintStream; import java.io.PrintWriter; import java.io.StringWriter; +import java.net.URI; import java.nio.charset.StandardCharsets; +import java.nio.file.FileSystems; +import java.nio.file.Files; +import java.nio.file.Path; import java.util.ArrayList; import java.util.List; +import java.util.Map; import static org.assertj.core.api.Assertions.assertThat; @@ -54,7 +60,7 @@ private static boolean isClassPresent(String className) { private static List expectedFeatures() { List features = new ArrayList<>( - List.of("dpop", "dpop_nonce_challenge", "http_range_source")); + List.of("dpop", "dpop_nonce_challenge", "partial_decrypt", "http_range_source")); if (MLKEM_PROVIDER_ON_CLASSPATH) { features.add("session-key-mlkem"); } @@ -74,7 +80,8 @@ void supports_dpop_nonce_challenge_exits_0() { } @Test - void supports_http_range_source_exit_0() { + void supports_partial_decrypt_and_http_range_source_exit_0() { + assertThat(new CommandLine(new Command()).execute("supports", "partial_decrypt")).isEqualTo(0); assertThat(new CommandLine(new Command()).execute("supports", "http_range_source")).isEqualTo(0); } @@ -145,7 +152,7 @@ void supports_noArgs_json_listsFeatures() { assertThat(code[0]).isEqualTo(0); String expected = "{\"dpop\":true,\"dpop_nonce_challenge\":true," - + "\"http_range_source\":true" + + "\"partial_decrypt\":true,\"http_range_source\":true" + (MLKEM_PROVIDER_ON_CLASSPATH ? ",\"session-key-mlkem\":true" : "") + "}"; assertThat(out.trim()).isEqualTo(expected); @@ -181,4 +188,21 @@ void supports_unknownFeature_json_false() { assertThat(out.trim()).isEqualTo("{\"unknown_feature\":false}"); } + + @Test + void moveIntoPlaceFallsBackWhenAtomicMoveIsUnsupported(@TempDir Path tempDir) throws Exception { + Path source = Files.writeString(tempDir.resolve("plaintext.tmp"), "plaintext"); + Path archive = tempDir.resolve("output.zip"); + + try (var zip = FileSystems.newFileSystem( + URI.create("jar:" + archive.toUri()), Map.of("create", "true"))) { + Path target = zip.getPath("/plaintext.txt"); + + Command.moveIntoPlace(source, target); + + assertThat(Files.readString(target)).isEqualTo("plaintext"); + assertThat(source).doesNotExist(); + } + } + } 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 4ad24298..5bb88009 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java @@ -397,53 +397,113 @@ public Manifest getManifest() { } public void readPayload(OutputStream outputStream) throws SDK.TamperException, IOException { + readPayload(outputStream, 0, -1); + } - MessageDigest digest = null; - try { - digest = MessageDigest.getInstance("SHA-256"); - } catch (NoSuchAlgorithmException e) { - throw new IllegalStateException("error getting instance of SHA-256", e); + /** + * Writes the plaintext bytes {@code [offset, offset + length)} to {@code outputStream}. + * Only the segments covering the range are read, verified and decrypted, so over a + * channel that fetches on demand only those bytes are pulled from the source. + * + * @param offset plaintext offset to start at; equal to the payload size writes nothing + * @param length number of bytes to write; negative means to the end of the payload, and + * a range running past the end is clamped to it + * @throws IllegalArgumentException if {@code offset} is negative or beyond the end of the + * payload, before anything is written + */ + public void readPayload(OutputStream outputStream, long offset, long length) + throws SDK.TamperException, IOException { + if (offset < 0) { + throw new IllegalArgumentException("offset must not be negative: " + offset); } validateSegmentSizes(); List segments = manifest.encryptionInformation.integrityInformation.segments; + long total = 0; for (int i = 0; i < segments.size(); i++) { - Manifest.Segment segment = Manifest.segmentAt(segments, i); - byte[] readBuf = new byte[(int) segment.encryptedSegmentSize]; - int bytesRead = tdfReader.readPayloadBytes(readBuf); + total += plaintextSize(Manifest.segmentAt(segments, i)); + } + if (offset > total) { + throw new IllegalArgumentException( + "offset " + offset + " is beyond the end of the payload (" + total + " bytes)"); + } + long end = length < 0 || length > total - offset ? total : offset + length; + if (offset == end) { + return; + } + + MessageDigest digest = null; + try { + digest = MessageDigest.getInstance("SHA-256"); + } catch (NoSuchAlgorithmException e) { + throw new IllegalStateException("error getting instance of SHA-256", e); + } - if (readBuf.length != bytesRead) { - throw new IllegalStateException("unable to read bytes for segment (wanted " - + segment.encryptedSegmentSize + " but got " + bytesRead + ")"); + try (var payload = tdfReader.payload()) { + long plainStart = 0; + for (int i = 0; i < segments.size(); i++) { + if (plainStart >= end) { + break; + } + Manifest.Segment segment = Manifest.segmentAt(segments, i); + long plainEnd = plainStart + plaintextSize(segment); + // <=, not <: a segment ending exactly at offset holds none of the range + if (plainEnd <= offset) { + if (payload.skip(segment.encryptedSegmentSize) != segment.encryptedSegmentSize) { + throw new IllegalStateException("unable to skip segment of " + + segment.encryptedSegmentSize + " bytes"); + } + } else { + byte[] plain = readSegment(payload, segment, digest); + int from = (int) Math.max(0, offset - plainStart); + int to = (int) Math.min(plain.length, end - plainStart); + outputStream.write(plain, from, to - from); + } + plainStart = plainEnd; } + } + } - var isLegacyTdf = manifest.tdfVersion == null || manifest.tdfVersion.isEmpty(); + // Derived from the ciphertext length rather than trusting the manifest's segmentSize, + // so a range lands on the bytes decryption actually produces. + private long plaintextSize(Manifest.Segment segment) { + return segment.encryptedSegmentSize + - (manifest.payload.isEncrypted ? kGcmIvSize + AesGcm.GCM_TAG_LENGTH : 0); + } - if (manifest.payload.isEncrypted) { - var sigAlg = segmentIntegrityAlgorithmFromManifest( - manifest.encryptionInformation.integrityInformation.segmentHashAlg); + private byte[] readSegment(InputStream payload, Manifest.Segment segment, MessageDigest digest) { + byte[] readBuf = new byte[(int) segment.encryptedSegmentSize]; + int bytesRead = TDFReader.readPayloadBytes(payload, readBuf); - var payloadSig = calculateSignature(readBuf, payloadKey, sigAlg); - if (isLegacyTdf) { - payloadSig = Hex.encodeHexString(payloadSig).getBytes(StandardCharsets.UTF_8); - } + if (readBuf.length != bytesRead) { + throw new IllegalStateException("unable to read bytes for segment (wanted " + + segment.encryptedSegmentSize + " but got " + bytesRead + ")"); + } - if (segment.hash.compareTo(Base64.getEncoder().encodeToString(payloadSig)) != 0) { - throw new SDK.SegmentSignatureMismatch("segment signature miss match"); - } + var isLegacyTdf = manifest.tdfVersion == null || manifest.tdfVersion.isEmpty(); - byte[] writeBuf = aesGcm.decrypt(new AesGcm.Encrypted(readBuf)); - outputStream.write(writeBuf); + if (manifest.payload.isEncrypted) { + var sigAlg = segmentIntegrityAlgorithmFromManifest( + manifest.encryptionInformation.integrityInformation.segmentHashAlg); - } else { - String segmentSig = Hex.encodeHexString(digest.digest(readBuf)); - if (segment.hash.compareTo(segmentSig) != 0) { - throw new SDK.SegmentSignatureMismatch("segment signature miss match"); - } + var payloadSig = calculateSignature(readBuf, payloadKey, sigAlg); + if (isLegacyTdf) { + payloadSig = Hex.encodeHexString(payloadSig).getBytes(StandardCharsets.UTF_8); + } - outputStream.write(readBuf); + if (segment.hash.compareTo(Base64.getEncoder().encodeToString(payloadSig)) != 0) { + throw new SDK.SegmentSignatureMismatch("segment signature miss match"); } + + return aesGcm.decrypt(new AesGcm.Encrypted(readBuf)); + } else { + String segmentSig = Hex.encodeHexString(digest.digest(readBuf)); + if (segment.hash.compareTo(segmentSig) != 0) { + throw new SDK.SegmentSignatureMismatch("segment signature miss match"); + } + + return readBuf; } } diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java index 0f669962..bb92fe38 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java @@ -26,7 +26,8 @@ public class TDFReader { private static final int MANIFEST_BUFFER_SIZE = 1 << 16; private final ZipReader.Entry manifestEntry; - private final InputStream payload; + private final ZipReader.Entry payloadEntry; + private InputStream payload; public TDFReader(SeekableByteChannel tdf) throws SDKException, IOException { // A zip may legally list the same name twice, and readers disagree about which copy @@ -52,7 +53,7 @@ public TDFReader(SeekableByteChannel tdf) throws SDKException, IOException { } manifestEntry = manifest; - payload = entries.get(TDF_PAYLOAD_FILE_NAME).getData(); + payloadEntry = entries.get(TDF_PAYLOAD_FILE_NAME); } /** Like the constructor, but reports a malformed container as {@link SDK.MalformedTDFException}. */ @@ -104,7 +105,28 @@ Reader manifest() { } } + /** A fresh stream over the payload, positioned at its start. */ + InputStream payload() throws IOException { + try { + return payloadEntry.getData(); + } catch (InvalidZipException e) { + // the payload's local header puts its data past the end of the archive + throw malformed("error reading from payload in TDF", e); + } + } + int readPayloadBytes(byte[] buf) { + if (payload == null) { + try { + payload = payload(); + } catch (IOException e) { + throw new SDKException("error reading from payload in TDF", e); + } + } + return readPayloadBytes(payload, buf); + } + + static int readPayloadBytes(InputStream payload, byte[] buf) { int totalRead = 0; int nread; try { diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java index c1d24e39..2d143cb0 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java @@ -451,6 +451,27 @@ private boolean doneReading() { return offset >= fileSize; } + /** + * Moves the cursor without reading: the default skip reads and discards, which + * over a channel that fetches on demand would download everything skipped. + *

+ * Reading is what normally discovers that the archive ends early, so the bytes + * being stepped over are checked against the channel here. Without that, a + * truncated entry skipped to its end would report a clean EOF and never reach + * {@link #truncated()}. + */ + @Override + public long skip(long n) throws IOException { + long skipped = Math.max(0, Math.min(n, fileSize - offset)); + long available = zipChannel.size() - (startPosition + offset); + if (skipped > available) { + offset += Math.max(0, available); + throw truncated(); + } + offset += skipped; + return skipped; + } + /** * The archive ended before this entry's data did. Reporting that as an ordinary * end of stream would hand the caller a short entry with no sign anything is wrong. diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java index 42635294..494283a6 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java @@ -16,6 +16,9 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; /** * The OpenTDF spec names the manifest entry {@code manifest.json}; this SDK writes @@ -213,6 +216,20 @@ void readsThePayloadAlongsideASpecNamedManifest() throws IOException { } } + @Test + void opensThePayloadOnlyWhenItIsRead() throws IOException { + try (var tdf = spy(archiveOf( + TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD, + "manifest.json", MANIFEST))) { + var reader = new TDFReader(tdf); + + verify(tdf, never()).position(0); + + reader.readPayloadBytes(new byte[PAYLOAD.length()]); + verify(tdf).position(0); + } + } + /** An oversized payload is only detected on read, and still surfaces as a malformed TDF. */ @Test void reportsATruncatedPayloadAsMalformed() throws IOException { @@ -228,10 +245,7 @@ void reportsATruncatedPayloadAsMalformed() throws IOException { } } - /** - * The check on the local header runs when the payload stream is opened, which the - * constructor does, so the whole TDF is rejected before any byte is read. - */ + /** The check on the local header runs when the payload is opened, before any byte is read. */ @Test void reportsAPayloadWhoseDataStartsPastTheEndAsMalformed() throws IOException { var payload = new HostileZips.Entry(TDFWriter.TDF_PAYLOAD_FILE_NAME, new byte[0]); @@ -240,7 +254,12 @@ void reportsAPayloadWhoseDataStartsPastTheEndAsMalformed() throws IOException { // the filename length of the first local file header, which starts at offset 0 ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN).putShort(26, (short) 0xFFFF); try (var tdf = new SeekableInMemoryByteChannel(archive)) { - assertThatThrownBy(() -> TDFReader.open(tdf)) + var reader = new TDFReader(tdf); + + assertThatThrownBy(reader::payload) + .isInstanceOf(SDK.MalformedTDFException.class) + .hasCauseInstanceOf(InvalidZipException.class); + assertThatThrownBy(() -> reader.readPayloadBytes(new byte[1])) .isInstanceOf(SDK.MalformedTDFException.class) .hasCauseInstanceOf(InvalidZipException.class); } diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java index 0ddb0b9d..0ef6b0f6 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java @@ -17,6 +17,7 @@ import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.ValueSource; import javax.annotation.Nonnull; @@ -39,6 +40,7 @@ import java.util.Random; import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.concurrent.atomic.AtomicLong; import java.util.function.Consumer; import java.util.function.Predicate; import java.util.regex.Pattern; @@ -736,6 +738,114 @@ public void testCreatingTDFWithMultipleSegments() throws Exception { assertThat(reader.getMetadata()).isEqualTo(metadata); } + private static final int SEG = Config.MIN_SEGMENT_SIZE; + + /** A TDF of {@code 2.5 * SEG} random bytes, so it has two full segments and a partial one. */ + private static byte[][] partialReadFixture() throws Exception { + var data = new byte[SEG * 5 / 2]; + new Random().nextBytes(data); + var tdfOutputStream = new ByteArrayOutputStream(); + var tdf = new TDF(new FakeServicesBuilder().setKas(kas) + .setKeyAccessServerRegistryService(kasRegistryService).build()); + tdf.createTDF(new ByteArrayInputStream(data), tdfOutputStream, Config.newTDFConfig( + Config.withAutoconfigure(false), + Config.withKasInformation(getSingleRSAKASInfo()), + Config.withSegmentSize(SEG))); + return new byte[][] { data, tdfOutputStream.toByteArray() }; + } + + private static Reader loadFixture(byte[] tdfBytes) throws Exception { + var tdf = new TDF(new FakeServicesBuilder().setKas(kas) + .setKeyAccessServerRegistryService(kasRegistryService).build()); + return tdf.loadTDF(new SeekableInMemoryByteChannel(tdfBytes), platformUrl); + } + + @ParameterizedTest + @CsvSource({ + "0, -1", // whole payload + "16334, 100", // crosses the first segment boundary + "16384, 100", // starts exactly where a segment ends + "16384, 16384", // exactly the second segment + "100, 0", // empty + "32768, -1", // last segment to the end + "40950, 1000", // runs past the end: clamped + "40960, -1", // offset == size: empty + }) + void readPayloadRange(long offset, long length) throws Exception { + var fixture = partialReadFixture(); + byte[] data = fixture[0]; + var out = new ByteArrayOutputStream(); + loadFixture(fixture[1]).readPayload(out, offset, length); + + int end = length < 0 ? data.length : (int) Math.min(data.length, offset + length); + assertThat(out.toByteArray()).containsExactly(Arrays.copyOfRange(data, (int) offset, end)); + } + + @Test + void readPayloadRangeCanBeRepeatedOnOneReader() throws Exception { + var fixture = partialReadFixture(); + var reader = loadFixture(fixture[1]); + for (int i = 0; i < 2; i++) { + var out = new ByteArrayOutputStream(); + reader.readPayload(out, SEG, 10); + assertThat(out.toByteArray()).containsExactly(Arrays.copyOfRange(fixture[0], SEG, SEG + 10)); + } + } + + @Test + void readPayloadRangeRejectsBadOffsetsBeforeWriting() throws Exception { + var fixture = partialReadFixture(); + var reader = loadFixture(fixture[1]); + var out = new ByteArrayOutputStream(); + assertThatThrownBy(() -> reader.readPayload(out, fixture[0].length + 1L, -1)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("beyond the end"); + assertThatThrownBy(() -> reader.readPayload(out, -1, -1)) + .isInstanceOf(IllegalArgumentException.class); + assertThat(out.size()).isZero(); + } + + /** Counts the bytes actually pulled from the archive, whatever the channel is backed by. */ + private static class CountingChannel extends SeekableInMemoryByteChannel { + final AtomicLong read = new AtomicLong(); + + CountingChannel(byte[] data) { + super(data); + } + + @Override + public int read(java.nio.ByteBuffer buf) throws IOException { + int n = super.read(buf); + if (n > 0) { + read.addAndGet(n); + } + return n; + } + } + + /** + * The point of the ranged read: segments before the range are stepped over rather than + * pulled in. Over a channel that fetches on demand this is the difference between reading + * a slice and downloading the whole payload. + */ + @Test + void readPayloadRangeReadsOnlyCoveringSegments() throws Exception { + var fixture = partialReadFixture(); + var channel = new CountingChannel(fixture[1]); + var tdf = new TDF(new FakeServicesBuilder().setKas(kas) + .setKeyAccessServerRegistryService(kasRegistryService).build()); + var reader = tdf.loadTDF(channel, Config.newTDFReaderConfig(), platformUrl); + + channel.read.set(0); + var out = new ByteArrayOutputStream(); + reader.readPayload(out, 2L * SEG - 50, 100); + + assertThat(out.toByteArray()).containsExactly(Arrays.copyOfRange(fixture[0], 2 * SEG - 50, 2 * SEG + 50)); + // the second (full) and third (half) segments, plus the payload entry's local header + long encryptedSegments = (SEG + 28) + (SEG / 2 + 28); + assertThat(channel.read.get()).isBetween(encryptedSegments, encryptedSegments + 1024); + } + /** * A segment size small enough that the test payloads below span several segments. Matches * web-sdk's {@code DEFAULT_SEGMENT_SIZE}, though nothing here depends on the exact value. diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java index 7cc188b5..02450b0d 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java @@ -1007,4 +1007,39 @@ private static String readEntry(ZipFile zip, String name) throws IOException { zip.getInputStream(zip.getEntry(name)).transferTo(data); return data.toString(StandardCharsets.UTF_8); } + + /** + * skip() moves the cursor instead of reading, so it has to check the bytes it steps over + * are really there. Otherwise a truncated entry skipped to its end reports a clean EOF. + */ + @Test + public void skipPastTheEndOfATruncatedEntryIsReportedAsInvalid() throws Exception { + var entry = new HostileZips.Entry("0.payload", "0123456789".getBytes(StandardCharsets.UTF_8)); + entry.uncompressedSize = 1_000_000L; + try (var channel = new SeekableInMemoryByteChannel(HostileZips.build(List.of(entry)))) { + var data = new ZipReader(channel).getEntries().get(0).getData(); + + assertThatThrownBy(() -> data.skip(1_000_000L)) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("1000000 byte entry"); + } + } + + /** A skip wholly inside the entry still just moves the cursor, and lands on the right bytes. */ + @Test + public void skipWithinAnIntactEntryMovesTheCursor() throws Exception { + var payload = new byte[5000]; + new Random(7).nextBytes(payload); + var archive = HostileZips.build(List.of(new HostileZips.Entry("0.payload", payload))); + try (var channel = new SeekableInMemoryByteChannel(archive)) { + var data = new ZipReader(channel).getEntries().get(0).getData(); + + assertThat(data.skip(4000)).isEqualTo(4000); + assertThat(data.readAllBytes()).isEqualTo(Arrays.copyOfRange(payload, 4000, 5000)); + // clamped to what is left, and a negative request is a no-op + assertThat(data.skip(10)).isZero(); + assertThat(data.skip(-5)).isZero(); + } + } + } From bc2d6e4951955744a576c3c0da96474501620ad4 Mon Sep 17 00:00:00 2001 From: Morgan Kleene Date: Wed, 11 Nov 2026 09:37:02 -0500 Subject: [PATCH 3/6] Update Command.java --- .../src/main/java/io/opentdf/platform/Command.java | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/cmdline/src/main/java/io/opentdf/platform/Command.java b/cmdline/src/main/java/io/opentdf/platform/Command.java index aaf68b87..89a181c5 100644 --- a/cmdline/src/main/java/io/opentdf/platform/Command.java +++ b/cmdline/src/main/java/io/opentdf/platform/Command.java @@ -389,14 +389,10 @@ private SDK buildSDK() { @CommandLine.Command(name = "decrypt") void decrypt( - @Option(names = { "-f", - "--file" }, required = true, description = "TDF file path, or an http(s):// URL read with range requests") String tdfFile, - @Option(names = { "-o", - "--output" }, defaultValue = Option.NULL_VALUE, description = "Write plaintext here, only on success (default: stdout)") Optional output, - @Option(names = { - "--offset" }, defaultValue = "0", description = "Plaintext byte offset to start decrypting from") long offset, - @Option(names = { - "--length" }, defaultValue = "-1", description = "Plaintext bytes to decrypt from --offset; negative decrypts to the end") long length, + @Option(names = { "-f", "--file" }, required = true, description = "TDF file path, or an http(s):// URL read with range requests") String tdfFile, + @Option(names = { "-o", "--output" }, defaultValue = Option.NULL_VALUE, description = "Write plaintext here, only on success (default: stdout)") Optional output, + @Option(names = { "--offset" }, defaultValue = "0", description = "Plaintext byte offset to start decrypting from") long offset, + @Option(names = { "--length" }, defaultValue = "-1", description = "Plaintext bytes to decrypt from --offset; negative decrypts to the end") long length, @Option(names = { "--rewrap-key-type" }, defaultValue = Option.NULL_VALUE, description = "Preferred rewrap algorithm, one of ${COMPLETION-CANDIDATES}") Optional rewrapKeyType, @Option(names = { From 4994aadd0b4ff91fdaf0ae8b05b2afe1b1438002 Mon Sep 17 00:00:00 2001 From: Morgan Kleene Date: Wed, 11 Nov 2026 09:40:04 -0500 Subject: [PATCH 4/6] Remove unnecessary blank line in CommandTest.java --- cmdline/src/test/java/io/opentdf/platform/CommandTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/cmdline/src/test/java/io/opentdf/platform/CommandTest.java b/cmdline/src/test/java/io/opentdf/platform/CommandTest.java index eb791afd..8820bf32 100644 --- a/cmdline/src/test/java/io/opentdf/platform/CommandTest.java +++ b/cmdline/src/test/java/io/opentdf/platform/CommandTest.java @@ -188,7 +188,6 @@ void supports_unknownFeature_json_false() { assertThat(out.trim()).isEqualTo("{\"unknown_feature\":false}"); } - @Test void moveIntoPlaceFallsBackWhenAtomicMoveIsUnsupported(@TempDir Path tempDir) throws Exception { Path source = Files.writeString(tempDir.resolve("plaintext.tmp"), "plaintext"); From b7f6df1cae90c94c92725c71cf6e27d4606cebcd Mon Sep 17 00:00:00 2001 From: Morgan Kleene Date: Wed, 11 Nov 2026 10:21:58 -0500 Subject: [PATCH 5/6] refactor(cmdline): open the TDF after building the reader config openTdf() was called before the assertion-verification keys were parsed, so a malformed --with-assertion-verification-keys failed after the TDF had already been opened -- over an http(s):// URL, after a range probe had gone out. main builds the reader config first, so restore that order and open the channel last, immediately before loadTDF. No functional change against main. It also drops the reindentation the earlier structure forced on the whole decrypt body, leaving Command.java's diff as just the openTdf() helper and its call site. Test plan: mvn -pl cmdline test -- 25 pass. Co-Authored-By: Claude Opus 5 Signed-off-by: Morgan Kleene --- .../java/io/opentdf/platform/Command.java | 75 ++++++++++--------- 1 file changed, 39 insertions(+), 36 deletions(-) diff --git a/cmdline/src/main/java/io/opentdf/platform/Command.java b/cmdline/src/main/java/io/opentdf/platform/Command.java index 89a181c5..810355f5 100644 --- a/cmdline/src/main/java/io/opentdf/platform/Command.java +++ b/cmdline/src/main/java/io/opentdf/platform/Command.java @@ -405,52 +405,55 @@ void decrypt( throws Exception { try (var sdk = buildSDK()) { var opts = new ArrayList>(); - try (var in = openTdf(sdk, tdfFile)) { - if (assertionVerification.isPresent()) { - var assertionVerificationInput = assertionVerification.get(); - Gson gson = buildGson(); + if (assertionVerification.isPresent()) { + var assertionVerificationInput = assertionVerification.get(); + Gson gson = buildGson(); - Config.AssertionVerificationKeys assertionVerificationKeys; + Config.AssertionVerificationKeys assertionVerificationKeys; + try { + assertionVerificationKeys = gson.fromJson(assertionVerificationInput, + Config.AssertionVerificationKeys.class); + } catch (JsonSyntaxException e) { + // try it as a file path try { - assertionVerificationKeys = gson.fromJson(assertionVerificationInput, + String fileJson = Files.readString(Paths.get(assertionVerificationInput)); + assertionVerificationKeys = gson.fromJson(fileJson, Config.AssertionVerificationKeys.class); - } catch (JsonSyntaxException e) { - // try it as a file path - try { - String fileJson = Files.readString(Paths.get(assertionVerificationInput)); - assertionVerificationKeys = gson.fromJson(fileJson, - Config.AssertionVerificationKeys.class); - } catch (JsonSyntaxException e2) { - throw new RuntimeException("Failed to parse assertion verification keys from file", e2); - } catch (Exception e3) { - throw new RuntimeException( - "Could not parse assertion verification keys as json string or path to file", - e3); - } + } catch (JsonSyntaxException e2) { + throw new RuntimeException("Failed to parse assertion verification keys from file", e2); + } catch (Exception e3) { + throw new RuntimeException( + "Could not parse assertion verification keys as json string or path to file", + e3); } + } - for (Map.Entry entry : assertionVerificationKeys.keys - .entrySet()) { - try { - Object correctedKey = correctKeyType(entry.getValue().alg, entry.getValue().key, true); - entry.setValue(new AssertionConfig.AssertionKey(entry.getValue().alg, correctedKey)); - } catch (Exception e) { - throw new RuntimeException("Error with assertion verification key: " + e.getMessage(), - e); - } + for (Map.Entry entry : assertionVerificationKeys.keys + .entrySet()) { + try { + Object correctedKey = correctKeyType(entry.getValue().alg, entry.getValue().key, true); + entry.setValue(new AssertionConfig.AssertionKey(entry.getValue().alg, correctedKey)); + } catch (Exception e) { + throw new RuntimeException("Error with assertion verification key: " + e.getMessage(), + e); } - opts.add(Config.withAssertionVerificationKeys(assertionVerificationKeys)); } + opts.add(Config.withAssertionVerificationKeys(assertionVerificationKeys)); + } - if (disableAssertionVerification) { - opts.add(Config.withDisableAssertionVerification(true)); - } - rewrapKeyType.map(Config::WithSessionKeyType).ifPresent(opts::add); + if (disableAssertionVerification) { + opts.add(Config.withDisableAssertionVerification(true)); + } + rewrapKeyType.map(Config::WithSessionKeyType).ifPresent(opts::add); - ignoreAllowlist.ifPresent(aBoolean -> opts.add(Config.WithIgnoreKasAllowlist(aBoolean))); - kasAllowlistStr.ifPresent(s -> opts.add(Config.WithKasAllowlist(s.split(",")))); + ignoreAllowlist.ifPresent(aBoolean -> opts.add(Config.WithIgnoreKasAllowlist(aBoolean))); + kasAllowlistStr.ifPresent(s -> opts.add(Config.WithKasAllowlist(s.split(",")))); - var readerConfig = Config.newTDFReaderConfig(opts.toArray(new Consumer[0])); + var readerConfig = Config.newTDFReaderConfig(opts.toArray(new Consumer[0])); + + // Opened last: a bad --with-assertion-verification-keys should fail before the + // TDF is touched, which over a URL means before any request goes out. + try (var in = openTdf(sdk, tdfFile)) { var reader = sdk.loadTDF(in, readerConfig); writePlaintext(out -> reader.readPayload(out, offset, length), output); } From 0f51205049c64588ac7c0790a294f2368f160edc Mon Sep 17 00:00:00 2001 From: Morgan Kleene Date: Wed, 11 Nov 2026 10:27:54 -0500 Subject: [PATCH 6/6] fix(sdk): bound a zip entry's extent by the end of entry data, not the archive Review follow-up on the ZipReader.skip() override. The check it added used the archive size, which is the wrong bound: the archive also holds the central directory and the records after it, so an entry declaring more bytes than it holds can overrun by a few and still fit inside the file. An entry with 10 bytes of data whose directory claims 11 passed that check, and skip(11) then stepped over a central-directory byte and reported a clean EOF. The bound is where entry data ends, which is the start of the central directory. ZipReader now keeps that offset and findStartOfData validates the whole declared extent against it, written as a subtraction so a huge declared size cannot overflow. Checking at open time rather than in skip() also covers read(), which would otherwise have handed back the same directory byte as entry data, and rejects the entry before any byte is read. skip() goes back to a plain clamp: with the extent validated up front it cannot step outside the entry's real data. A hostile-manifest case in TDFTest now fails at this earlier check, so it asserts the new message; it is the same InvalidZipException inside the same MalformedTDFException, just raised before the manifest is parsed. Test plan: mvn -pl sdk -am test -- 435 pass / 9 skipped; mvn -pl cmdline test -- 25 pass. New ZipReaderTest cases cover the one byte overrun that fits inside the archive (asserting the excess really is inside the file, so the test fails against the weaker bound), a size past the end of the archive, and an honest size as a control. Co-Authored-By: Claude Opus 5 Signed-off-by: Morgan Kleene --- .../io/opentdf/platform/sdk/ZipReader.java | 37 ++++++----- .../java/io/opentdf/platform/sdk/TDFTest.java | 8 ++- .../opentdf/platform/sdk/ZipReaderTest.java | 62 ++++++++++++++++--- 3 files changed, 79 insertions(+), 28 deletions(-) diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java index 2d143cb0..60577e09 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java @@ -416,13 +416,17 @@ private long findStartOfData() throws IOException { int extrafieldLength = readUnsignedShort(); long startOfData = zipChannel.position() + filenameLength + extrafieldLength; - // the central directory can't see these two lengths, so this is the first place a - // start past the end shows. an empty entry is never read, so unchecked it would come - // back empty rather than fail as truncated - if (startOfData > zipChannel.size()) { + // The central directory can't see these two lengths, so this is the first place a + // bad extent shows. The whole extent is checked, not just where it starts: an entry + // declaring more bytes than it holds would otherwise read, or skip over, whatever + // record follows it. Bounded by the end of entry data rather than by the archive -- + // the archive also holds the central directory, so a small overrun still fits + // inside the file. Written as a subtraction so a huge declared size cannot overflow. + if (startOfData > endOfEntryData - fileSize) { throw new InvalidZipException("The local file header of entry [" + fileName + "] at offset " - + offsetToLocalHeader + " puts its data at offset " + startOfData - + ", past the end of this " + zipChannel.size() + " byte archive"); + + offsetToLocalHeader + " puts its " + fileSize + " bytes of data at offset " + + startOfData + ", running past the end of this archive's entry data at " + + endOfEntryData); } return startOfData; } @@ -455,19 +459,13 @@ private boolean doneReading() { * Moves the cursor without reading: the default skip reads and discards, which * over a channel that fetches on demand would download everything skipped. *

- * Reading is what normally discovers that the archive ends early, so the bytes - * being stepped over are checked against the channel here. Without that, a - * truncated entry skipped to its end would report a clean EOF and never reach - * {@link #truncated()}. + * Safe because {@link #findStartOfData()} has already bounded this entry's + * extent to the region before the central directory, so clamping to what the + * entry declares cannot step over bytes belonging to another record. */ @Override - public long skip(long n) throws IOException { + public long skip(long n) { long skipped = Math.max(0, Math.min(n, fileSize - offset)); - long available = zipChannel.size() - (startPosition + offset); - if (skipped > available) { - offset += Math.max(0, available); - throw truncated(); - } offset += skipped; return skipped; } @@ -646,6 +644,7 @@ private Entry readCentralDirectoryFileHeader(long centralDirectoryEnd) throws IO public ZipReader(SeekableByteChannel channel) throws IOException { zipChannel = channel; var centralDirectoryRecord = readEndOfCentralDirectory(); + endOfEntryData = centralDirectoryRecord.offsetToStart; seekWithinArchive("the central directory", centralDirectoryRecord.offsetToStart); long centralDirectoryEnd = centralDirectoryRecord.offsetToStart + centralDirectoryRecord.size; // a corrupt zip64 count can be negative, which would read as an empty archive; an @@ -662,6 +661,12 @@ public ZipReader(SeekableByteChannel channel) throws IOException { } final SeekableByteChannel zipChannel; + /** + * Where entry data ends and the central directory begins. Every entry's data lies before + * it, which makes it the real bound on an entry's extent; {@link #zipChannel}'s size is a + * weaker one, since the archive also holds the directory and the records after it. + */ + private long endOfEntryData; final ArrayList entries = new ArrayList<>(); public List getEntries() { diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java index 0ef6b0f6..b2c14341 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java @@ -1627,13 +1627,15 @@ void loadTDFReportsHostileArchivesAsMalformed() { */ @Test void loadTDFReportsAManifestRunningPastTheArchiveAsMalformed() { - // the manifest opens a JSON comment, so the parser reads on to the end of the archive - // looking for its close, rather than failing at the first byte past the entry + // Rejected when the entry is opened, before a byte is read: the declared extent runs + // past where this archive's entry data ends. The manifest opens a JSON comment, so a + // reader that got as far as parsing would run to the end of the archive looking for + // its close rather than stop at the first byte past the entry. var uncompressedPastTheEnd = HostileZips.tdfWith("/*", e -> e.uncompressedSize = 0x7FFFFFFFL); assertThatThrownBy(() -> loadTDFFrom(uncompressedPastTheEnd)) .isInstanceOf(SDK.MalformedTDFException.class) .hasCauseInstanceOf(InvalidZipException.class) - .hasMessageContaining("Archive ended"); + .hasMessageContaining("running past the end of this archive's entry data"); var storedPastTheEnd = HostileZips.tdfWith("{}", e -> e.compressedSize = 0x7FFFFFFFL); assertThatThrownBy(() -> loadTDFFrom(storedPastTheEnd)) diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java index 02450b0d..dcecbd2c 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java @@ -1008,20 +1008,64 @@ private static String readEntry(ZipFile zip, String name) throws IOException { return data.toString(StandardCharsets.UTF_8); } + private static byte[] payloadArchive(byte[] data, Long declaredSize) { + var entry = new HostileZips.Entry("0.payload", data); + entry.uncompressedSize = declaredSize; + return HostileZips.build(List.of(entry)); + } + + /** Where {@code data} sits in {@code archive}, so a test can talk about absolute offsets. */ + private static int offsetOf(byte[] archive, byte[] data) { + for (int i = 0; i + data.length <= archive.length; i++) { + if (Arrays.equals(Arrays.copyOfRange(archive, i, i + data.length), data)) { + return i; + } + } + throw new AssertionError("entry data not found in the archive"); + } + /** - * skip() moves the cursor instead of reading, so it has to check the bytes it steps over - * are really there. Otherwise a truncated entry skipped to its end reports a clean EOF. + * The bound on an entry's extent is where entry data ends, not the archive. The central + * directory follows the data, so an entry declaring one byte more than it holds still + * fits inside the file -- and an archive-sized bound would hand that directory byte back + * as entry data, or let skip() step over it and report a clean EOF. */ @Test - public void skipPastTheEndOfATruncatedEntryIsReportedAsInvalid() throws Exception { - var entry = new HostileZips.Entry("0.payload", "0123456789".getBytes(StandardCharsets.UTF_8)); - entry.uncompressedSize = 1_000_000L; - try (var channel = new SeekableInMemoryByteChannel(HostileZips.build(List.of(entry)))) { - var data = new ZipReader(channel).getEntries().get(0).getData(); + public void aDeclaredSizeOverrunningIntoTheCentralDirectoryIsInvalid() throws Exception { + var data = "0123456789".getBytes(StandardCharsets.UTF_8); + var archive = payloadArchive(data, data.length + 1L); - assertThatThrownBy(() -> data.skip(1_000_000L)) + // the excess byte is comfortably inside the file: this is the case the weaker bound misses + assertThat(offsetOf(archive, data) + data.length + 1).isLessThan(archive.length); + + try (var channel = new SeekableInMemoryByteChannel(archive)) { + var entry = new ZipReader(channel).getEntries().get(0); + + assertThatThrownBy(entry::getData) .isInstanceOf(InvalidZipException.class) - .hasMessageContaining("1000000 byte entry"); + .hasMessageContaining("running past the end of this archive's entry data"); + } + } + + /** The same check covers a size past the end of the whole archive. */ + @Test + public void aDeclaredSizePastTheEndOfTheArchiveIsInvalid() throws Exception { + var archive = payloadArchive("0123456789".getBytes(StandardCharsets.UTF_8), 1_000_000L); + try (var channel = new SeekableInMemoryByteChannel(archive)) { + var entry = new ZipReader(channel).getEntries().get(0); + + assertThatThrownBy(entry::getData).isInstanceOf(InvalidZipException.class); + } + } + + /** The control: the same archive with an honest size reads its bytes back. */ + @Test + public void aDeclaredSizeMatchingTheEntryDataReads() throws Exception { + var data = "0123456789".getBytes(StandardCharsets.UTF_8); + try (var channel = new SeekableInMemoryByteChannel(payloadArchive(data, null))) { + var entry = new ZipReader(channel).getEntries().get(0); + + assertThat(entry.getData().readAllBytes()).isEqualTo(data); } }