From 87d3cb0d61d6060b9c9db3991b23d1d2cb06ff79 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Mon, 31 Aug 2026 22:36:54 -0400 Subject: [PATCH] fix(cli): drop the encrypt-side stdin spool DSPX-4499 fixed encrypt's OOM by streaming, but CreateTDF still required an io.ReadSeeker, so piped stdin had to be spooled to a temporary file first. That traded the whole-payload allocation for a disk write and a writable TMPDIR -- better, but not the point. Now that CreateTDF takes an io.Reader, the pipe goes straight to the SDK. A file is still opened seekably, on purpose. The SDK measures a seekable payload and keeps the archive in the compact ZIP32 layout; an unmeasurable one has to be ZIP64, because the choice is baked into the payload's local file header before the first segment goes out. So `encrypt file.txt` is unchanged byte for byte, and `... | encrypt` produces a slightly larger TDF than it did when it was spooled. That is the trade, and it is the right way round: nobody should need a writable temp directory to encrypt a stream. MIME sniffing is what made this more than a deletion. It reads the first megabyte and previously seeked back to zero, which a pipe cannot do. Wrapping the input in a bufio.Reader is not an option either -- that hides the Seeker and would silently flip every file encrypt to ZIP64. detectMimeType now returns a reader alongside the type: the same reader for a seekable input, rewound; the sniffed prefix pushed back with io.MultiReader for anything else. A megabyte in memory at most, and only when --mime-type was not given. Testing: TestDetectMimeTypePreservesThePayload now runs each case twice, once over a reader whose Seek method is hidden, and asserts the payload arrives whole either way. TestDetectMimeTypeKeepsSeekability guards the ZIP32 layout directly, since nothing else would fail if a file came back wrapped. e2e gains "encrypt measures a file and streams a pipe", which reads the local file header's extra field length to tell the two layouts apart -- both forms round-trip, so a quiet return to spooling would show up nowhere else. Signed-off-by: Dave Mihalcik --- otdfctl/cmd/tdf/encrypt.go | 51 +++++++++++++++++------------- otdfctl/cmd/tdf/encrypt_test.go | 56 +++++++++++++++++++++++++-------- otdfctl/e2e/streaming.bats | 29 +++++++++++++++++ otdfctl/pkg/handlers/tdf.go | 9 +++--- spec/DSPX-4499.md | 17 +++++++--- 5 files changed, 118 insertions(+), 44 deletions(-) diff --git a/otdfctl/cmd/tdf/encrypt.go b/otdfctl/cmd/tdf/encrypt.go index 9358338a76..943fc9996a 100644 --- a/otdfctl/cmd/tdf/encrypt.go +++ b/otdfctl/cmd/tdf/encrypt.go @@ -1,6 +1,7 @@ package tdf import ( + "bytes" "errors" "io" "log/slog" @@ -27,26 +28,37 @@ var ( EncryptCmd = &encryptDoc.Command ) -// detectMimeType sniffs the payload's type from its head and rewinds, so the -// whole payload still reaches the encoder. +// detectMimeType sniffs the payload's type from its head and returns a reader +// that still yields the whole payload. // // Detection needs only the first megabyte, which is what mimetype is limited to -// anyway, so this reads a bounded prefix rather than the whole payload. -func detectMimeType(in io.ReadSeeker, fileExt string) (string, error) { +// anyway, so this reads a bounded prefix rather than the whole payload. A +// seekable input is rewound and handed back unchanged, which matters: the SDK +// measures a seekable payload and keeps the archive in the compact ZIP32 +// layout. A pipe cannot be rewound, so the sniffed prefix is pushed back in +// front of it instead — a megabyte held in memory at most. +func detectMimeType(in io.Reader, fileExt string) (string, io.Reader, error) { mimetype.SetLimit(Size1MB) // limit to 1MB head := make([]byte, Size1MB) // A payload shorter than the sniff window is the common case, not an error. n, err := io.ReadFull(in, head) if err != nil && !errors.Is(err, io.EOF) && !errors.Is(err, io.ErrUnexpectedEOF) { - return "", err + return "", nil, err } - if _, err := in.Seek(0, io.SeekStart); err != nil { - return "", err + head = head[:n] + + rest := in + if seeker, ok := in.(io.Seeker); ok { + if _, err := seeker.Seek(0, io.SeekStart); err != nil { + return "", nil, err + } + } else { + rest = io.MultiReader(bytes.NewReader(head), in) } // defaults to application/octet-stream if nothing is recognized - detected := mimetype.Detect(head[:n]).String() + detected := mimetype.Detect(head).String() if detected == "application/octet-stream" && fileExt != "" { // mime.TypeByExtension is the extension lookup. mimetype.Lookup takes a // MIME type string, so passing it a bare extension always returned nil @@ -57,7 +69,7 @@ func detectMimeType(in io.ReadSeeker, fileExt string) (string, error) { detected = byExt } } - return detected, nil + return detected, rest, nil } func encryptRun(cmd *cobra.Command, args []string) { @@ -115,26 +127,21 @@ func encryptRun(cmd *cobra.Command, args []string) { cliExit("ONLY ONE") } - // The SDK seeks to the end of the payload to size it, so the input has to be - // seekable. A file already is; a pipe is spooled to disk, which trades the - // temporary file for the memory a whole-payload read used to cost. - var in io.ReadSeeker - var cleanup func() + // The SDK encrypts straight from a reader, so piped input goes to it as-is + // rather than through a temporary file. A file is still opened seekably: the + // SDK measures a seekable payload and keeps the archive in the compact ZIP32 + // layout, which a pipe has to give up. + var in io.Reader = piped + cleanup := func() {} if filePath != "" { f, err := os.Open(filePath) if err != nil { cli.ExitWithError("Failed to read file:", err) } in, cleanup = f, func() { f.Close() } - } else { - f, spoolCleanup, err := streamio.Spool(piped) - if err != nil { - cli.ExitWithError("Failed to read stdin:", err) - } - in, cleanup = f, spoolCleanup } // cli.ExitWithError calls os.Exit, which skips deferred functions, so every - // exit below goes through fail() to discard the spool and any partial output. + // exit below goes through fail() to discard any partial output. defer cleanup() // Resolve the destination before encrypting, so the payload streams straight @@ -168,7 +175,7 @@ func encryptRun(cmd *cobra.Command, args []string) { // auto-detect mime type if not provided if fileMimeType == "" { slog.Debug("detecting mime type of file") - fileMimeType, err = detectMimeType(in, fileExt) + fileMimeType, in, err = detectMimeType(in, fileExt) if err != nil { fail("Failed to read file:", err) } diff --git a/otdfctl/cmd/tdf/encrypt_test.go b/otdfctl/cmd/tdf/encrypt_test.go index 8c22bea7ed..179ed3ea5b 100644 --- a/otdfctl/cmd/tdf/encrypt_test.go +++ b/otdfctl/cmd/tdf/encrypt_test.go @@ -10,7 +10,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestDetectMimeTypeRewindsForTheEncoder(t *testing.T) { +func TestDetectMimeTypePreservesThePayload(t *testing.T) { for _, tc := range []struct { name string content string @@ -19,30 +19,60 @@ func TestDetectMimeTypeRewindsForTheEncoder(t *testing.T) { {name: "text", content: "hello, world\n", want: "text/plain; charset=utf-8"}, {name: "json", content: `{"a":1}`, want: "application/json"}, // Larger than the sniff window, so a detector that consumed the reader - // instead of rewinding would truncate the payload. + // without handing the prefix back would truncate the payload. {name: "larger than sniff window", content: strings.Repeat("a", Size1MB+7), want: "text/plain; charset=utf-8"}, } { t.Run(tc.name, func(t *testing.T) { - in := strings.NewReader(tc.content) + for _, seekable := range []bool{true, false} { + name := "seekable" + if !seekable { + name = "pipe" + } + t.Run(name, func(t *testing.T) { + var in io.Reader = strings.NewReader(tc.content) + if !seekable { + in = pipeReader{in} + } - got, err := detectMimeType(in, "") - require.NoError(t, err) - assert.Equal(t, tc.want, got) + got, rest, err := detectMimeType(in, "") + require.NoError(t, err) + assert.Equal(t, tc.want, got) - // The whole payload must still reach the encoder. - all, err := io.ReadAll(in) - require.NoError(t, err) - assert.Len(t, all, len(tc.content)) + // The whole payload must still reach the encoder. + all, err := io.ReadAll(rest) + require.NoError(t, err) + assert.Equal(t, tc.content, string(all)) + }) + } }) } } +// pipeReader hides the Seek method of the reader it wraps, standing in for +// stdin on the end of a pipe. +type pipeReader struct{ inner io.Reader } + +func (r pipeReader) Read(p []byte) (int, error) { return r.inner.Read(p) } + +// TestDetectMimeTypeKeepsSeekability guards the ZIP32 layout: the SDK only +// measures a payload it can seek, and a sniffed input that came back wrapped in +// io.MultiReader would silently force every file encrypt to ZIP64. +func TestDetectMimeTypeKeepsSeekability(t *testing.T) { + in := strings.NewReader("hello, world\n") + + _, rest, err := detectMimeType(in, "") + require.NoError(t, err) + + _, ok := rest.(io.Seeker) + assert.True(t, ok, "a seekable input must stay seekable") +} + func TestDetectMimeTypeFallsBackToExtension(t *testing.T) { // Bytes mimetype cannot classify, so the extension decides. ".pdf" is in // Go's builtin table, so this does not depend on the host's mime.types. unrecognized := bytes.Repeat([]byte{0x01, 0x02, 0x03, 0x04}, 8) - got, err := detectMimeType(bytes.NewReader(unrecognized), "pdf") + got, _, err := detectMimeType(bytes.NewReader(unrecognized), "pdf") require.NoError(t, err) assert.Equal(t, "application/pdf", got) } @@ -53,7 +83,7 @@ func TestDetectMimeTypeUnknownExtensionStaysOctetStream(t *testing.T) { // The previous implementation called mimetype.Lookup(fileExt).String(). // Lookup takes a MIME type string, not an extension, so it returned nil for // every extension and this path panicked. - got, err := detectMimeType(bytes.NewReader(unrecognized), "zzzznotathing") + got, _, err := detectMimeType(bytes.NewReader(unrecognized), "zzzznotathing") require.NoError(t, err) assert.Equal(t, "application/octet-stream", got) } @@ -61,7 +91,7 @@ func TestDetectMimeTypeUnknownExtensionStaysOctetStream(t *testing.T) { func TestDetectMimeTypeEmptyPayload(t *testing.T) { in := strings.NewReader("") - got, err := detectMimeType(in, "") + got, _, err := detectMimeType(in, "") require.NoError(t, err) assert.Equal(t, "text/plain", got) } diff --git a/otdfctl/e2e/streaming.bats b/otdfctl/e2e/streaming.bats index 2fe45f7df7..80cb976f46 100755 --- a/otdfctl/e2e/streaming.bats +++ b/otdfctl/e2e/streaming.bats @@ -126,6 +126,35 @@ setup() { assert_output "0" } +# extra_field_len reads the extra field length from the payload's local file +# header, at offset 28. A ZIP64 archive carries the extended information extra +# field there; a ZIP32 one has none. od -tu1 rather than -tu2 because only GNU +# od can be told the endianness. +extra_field_len() { + local lo hi + read -r lo hi <<<"$(od -An -tu1 -j28 -N2 "$1")" + echo $((lo + hi * 256)) +} + +# Encrypting from a pipe no longer spools to disk, so the payload can no longer +# be measured, so the archive has to be ZIP64 -- the choice is fixed before the +# first segment goes out. A file is still measured and stays ZIP32. The layout +# is what to assert on: both forms round-trip, so a quiet return to spooling +# would show up nowhere else. +@test "encrypt measures a file and streams a pipe" { + ./otdfctl encrypt -o "$TDF_OUT" $COMMON "$PLAIN" + [ "$(extra_field_len "$TDF_OUT")" -eq 0 ] + + local piped_tdf="$BATS_TEST_TMPDIR/piped.tdf" + run bash -c "echo '$SECRET_TEXT' | ./otdfctl encrypt $COMMON >'$piped_tdf'" + assert_success + [ "$(extra_field_len "$piped_tdf")" -gt 0 ] + + ./otdfctl decrypt -o "$RESULT" $COMMON "$piped_tdf" + run cat "$RESULT" + assert_output "$SECRET_TEXT" +} + # The point of DSPX-4499: peak RSS is bounded by segment size, not payload size. # Needs GNU time for 'Maximum resident set size'; BSD/shell time cannot report it. @test "encrypt and decrypt peak memory stay bounded on a large payload" { diff --git a/otdfctl/pkg/handlers/tdf.go b/otdfctl/pkg/handlers/tdf.go index b5e5c1a237..cfba223702 100644 --- a/otdfctl/pkg/handlers/tdf.go +++ b/otdfctl/pkg/handlers/tdf.go @@ -52,10 +52,11 @@ type EncryptOptions struct { // bounded by the SDK's segment size rather than by the payload length, so the // payload may be larger than RAM. // -// in must be seekable: the SDK measures the payload by seeking to its end -// before encrypting, and knowing the length up front is what lets it avoid -// defaulting to ZIP64. A caller holding a pipe should spool it first. -func (h Handler) Encrypt(ctx context.Context, out io.Writer, in io.ReadSeeker, o EncryptOptions) error { +// in need not be seekable, but a seekable input produces a smaller TDF: the SDK +// measures it by seeking to the end and can then keep the archive in the +// compact ZIP32 layout. An unmeasurable payload is written as ZIP64, since the +// choice is fixed before the first segment goes out. +func (h Handler) Encrypt(ctx context.Context, out io.Writer, in io.Reader, o EncryptOptions) error { switch o.TDFType { // Encrypt the data as a ZTDF case "", tdf.TypeTDF3, tdf.TypeZTDF: diff --git a/spec/DSPX-4499.md b/spec/DSPX-4499.md index f29fed6c7d..e2464e3dcb 100644 --- a/spec/DSPX-4499.md +++ b/spec/DSPX-4499.md @@ -182,6 +182,11 @@ embedded markdown (`docs/man/*/_index.md`, read via `doc.GetDocFlag`), not in Go `GetTdfType` must seek too. Piped input is therefore spooled to a temp file on both sides, trading the whole-payload allocation for a disk write and a writable `TMPDIR`. + Superseded on the encrypt side once DSPX-2604 landed: `CreateTDF` takes an `io.Reader`, + so piped input goes to it directly. Seekability is now an optimization rather than a + requirement — a measurable payload stays ZIP32, a piped one is written as ZIP64. Decrypt + and inspect still spool, and always will until there is a streaming reader. + - **Mime detection must not consume the input, and must not hide the `Seeker`.** The earlier revision wrapped input in `bufio.NewReaderSize(in, Size1MB+1)` and used `Peek`. That is wrong here: a `*bufio.Reader` is not an `io.Seeker`, so wrapping a file would @@ -232,8 +237,9 @@ embedded markdown (`docs/man/*/_index.md`, read via `doc.GetDocFlag`), not in Go - Building a streaming/chunked **reader** in the SDK. That would let decrypt take a pipe without spooling, but it is a large SDK change well beyond this ticket's CLI-layer scope. -- Relaxing `CreateTDF` to `io.Reader`, which would remove the encrypt-side spool. That is - DSPX-2604 and lands separately; the spool is ~10 lines to delete afterwards. +- Relaxing `CreateTDF` to `io.Reader`, which removes the encrypt-side spool. That is + DSPX-2604 and lands separately; the spool is ~10 lines to delete afterwards. (Done, in + the commit that removed it — see the seekability note above.) - Removing the dead `--tdf-type`/`-t` flag on `decrypt`, documented as deprecated and never read by `decryptRun`. Noted, left alone. - Any change to `nanotdf` — no nanotdf implementation exists in this repo. @@ -277,10 +283,11 @@ forced with an unresolvable attribute FQN and a KAS allowlist that excludes the As of this change, `streaming.bats` is the **only** e2e coverage of `encrypt`, `decrypt` and `inspect` that actually executes in CI. -Ten cases: file→file, file→stdout, and fully-piped round-trips; decrypt-from-stdin +Eleven cases: file→file, file→stdout, and fully-piped round-trips; decrypt-from-stdin (asserting the spool is removed); inspect from both a file and stdin; empty-stdin rejection -for both commands; no-partial-output-on-failure for both commands; and a 1 GiB peak-RSS -bound for both commands, which skips where GNU `time` is unavailable. +for both commands; no-partial-output-on-failure for both commands; a 1 GiB peak-RSS bound +for both commands, which skips where GNU `time` is unavailable; and, once the encrypt-side +spool was removed, a ZIP32-vs-ZIP64 assertion that a file is measured and a pipe is not. ## Side effect worth knowing about