From 771a61a75734d1fe3c39dd56fbfbe5896c8429b4 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Mon, 31 Aug 2026 22:36:54 -0400 Subject: [PATCH 1/2] 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. What is spooled is decided by what the fd actually is, not by which argument it arrived through. A file argument and a shell redirect from a regular file both seek, so both reach the SDK unwrapped; only a genuine stream is buffered. The SDK measures whatever it can seek, and under ~2 GiB that keeps the archive in the compact ZIP32 layout; an unmeasurable payload 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` and `encrypt < file.txt` are 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. Measurability is worth a second thing the archive layout does not show: the SDK fails a measured payload whose reader runs dry early, rather than returning a TDF that is silently short. A pipe has no declared length to check against. handlers.Handler's Encrypt is now the one place that states both, including that sdk.WithInputSize could restore the check for a caller who knows the length and that otdfctl does not expose it. The same distinction reaches decrypt and inspect, which spool because a TDF's manifest lives at the end of the archive. A redirect from a regular file is seekable already, so they read it in place rather than copying the whole TDF into TMPDIR to get a seekable view of it. 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 input itself, restored to the offset it was handed over at, when it can be; the sniffed prefix pushed back with io.MultiReader when it cannot. A megabyte in memory at most, and only when --mime-type was not given. Restoring the saved offset rather than seeking to zero matters once a redirect stays seekable: stdin arrives part-consumed whenever a wrapper reads a header before exec'ing us, and the payload is what remains, not the whole file. Rewindability is decided by attempting the seek, not by asserting io.Seeker. An *os.File on a FIFO, a process substitution, or /dev/stdin on the end of a pipe satisfies the interface and then returns ESPIPE, and treating that as fatal made `encrypt <(echo hi)` fail while `encrypt --mime-type text/plain <(echo hi)` succeeded -- a manifest-shaping flag deciding whether the command ran at all. A failed lseek leaves the offset untouched, so replaying the prefix is always correct. resolveInputSize in sdk/tdf.go already drew the same line. Input resolution moves into resolveEncryptInput so the ZIP32 invariant can be asserted without a running platform. That invariant lives in encryptRun, not in detectMimeType: anything wrapping the file between there and CreateTDF would flip every file encrypt to ZIP64 with the unit suite still green. encryptRun probes it the same way the SDK does, so the "this will be ZIP64" breadcrumb is logged once and is right whether or not detection ran -- --mime-type skips detectMimeType entirely, and that was the path on which an input that satisfies io.Seeker and then refuses reached the SDK unremarked. Testing: TestDetectMimeTypePreservesThePayload runs each case over all three reader shapes -- rewindable, Seek hidden, Seek refused with ESPIPE -- asserting both that the payload arrives whole and that only the first comes back measurable. Cases added for a payload of exactly the sniff window, the shortest input that fills the buffer and the boundary at which replaying the prefix could duplicate or drop a byte; for a reader handed over mid-payload, which must be restored to where it was rather than to zero; and for a read that fails partway, which must abort rather than encrypt a truncated payload. TestDetectMimeTypeKeepsSeekability guards the ZIP32 layout directly, since nothing else would fail if a file came back wrapped. streamio gains regular-file stdin coverage it never had: the file comes back unwrapped with its offset untouched, an empty or fully-consumed one still reports absent, and OpenSeekable hands it over without creating a spool. 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 -- now covering the redirect alongside the file and the pipe. A process-substitution case covers the ESPIPE path end to end, with and without --mime-type, which unit tests can only reach through a hand-written fake. The multi-segment case uses a real pipe rather than a redirect, since a redirect is now measured; 3 MiB clears the SDK's 2 MiB segment size, so the archive spans several segments whose hashes have to be combined on the way out and verified on the way back in. Decrypt from stdin is split into a piped case, which still spools and must remove the spool, and a redirect case, which no longer spools at all. extra_field_len fails rather than returning a number when it cannot read a full header: empty od output makes bash evaluate the arithmetic to 0, which is exactly the value the ZIP32 assertion passes on, so a missing archive would have looked like a pass. Signed-off-by: Dave Mihalcik --- otdfctl/cmd/tdf/decrypt.go | 4 +- otdfctl/cmd/tdf/encrypt.go | 113 ++++++++++------ otdfctl/cmd/tdf/encrypt_test.go | 155 ++++++++++++++++++++-- otdfctl/cmd/tdf/inspect.go | 3 +- otdfctl/e2e/streaming.bats | 135 ++++++++++++++++++- otdfctl/pkg/handlers/tdf.go | 15 ++- otdfctl/pkg/streamio/input.go | 120 ++++++++++++++--- otdfctl/pkg/streamio/input_test.go | 199 ++++++++++++++++++++++++++++- 8 files changed, 659 insertions(+), 85 deletions(-) diff --git a/otdfctl/cmd/tdf/decrypt.go b/otdfctl/cmd/tdf/decrypt.go index 05096d7ec0..5da5328cdf 100644 --- a/otdfctl/cmd/tdf/decrypt.go +++ b/otdfctl/cmd/tdf/decrypt.go @@ -61,7 +61,9 @@ func decryptRun(cmd *cobra.Command, args []string) { defer closeIn() // cli.ExitWithError calls os.Exit, which skips deferred functions, so both - // the spooled input and the partial output have to be discarded first. + // the input and the partial output have to be discarded first. closeIn is + // what removes the spool, when there is one -- a piped TDF is spooled to get + // a seekable view of it, while a file argument or a redirect is not. // Declared before the destination exists so every exit below can use it. var outFile *streamio.OutputFile fail := func(msg string, err error) { diff --git a/otdfctl/cmd/tdf/encrypt.go b/otdfctl/cmd/tdf/encrypt.go index d4c09aa805..1ccafdaf20 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" @@ -29,26 +30,34 @@ 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. An input +// that can be rewound is handed back unchanged, keeping it measurable; anything +// else gets the sniffed prefix pushed back in front of it, a megabyte in memory +// at most. +// +// On error the returned reader is nil, and the payload has been partly consumed. +func detectMimeType(in io.Reader, fileExt string) (string, io.Reader, error) { mimetype.SetLimit(Size1MB) // limit to 1MB + // The rewind below restores where the payload started rather than seeking to + // zero: stdin can arrive part-consumed — `{ read -r header; otdfctl encrypt; } + // < payload.txt` leaves it mid-file — and the payload is what remains. + seeker, start, _ := streamio.Seekable(in) + 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 - } - if _, err := in.Seek(0, io.SeekStart); err != nil { - return "", err + return "", nil, err } + head = head[:n] // 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 @@ -59,7 +68,18 @@ func detectMimeType(in io.ReadSeeker, fileExt string) (string, error) { detected = byExt } } - return detected, nil + + // Rewinding keeps the payload measurable; replaying the prefix is always + // correct, since a failed lseek(2) leaves the offset untouched. So a refused + // seek is a fallback, not an error — the SDK draws the same line when it + // sizes the payload. + if seeker != nil { + if _, err := seeker.Seek(start, io.SeekStart); err == nil { + return detected, in, nil + } + slog.Debug("payload rewind failed after its position probe succeeded; replaying the sniffed prefix instead") + } + return detected, io.MultiReader(bytes.NewReader(head), in), nil } func encryptRun(cmd *cobra.Command, args []string) { @@ -117,32 +137,41 @@ 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() + inputName := "stdin" 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) + inputName = filePath + } + + // Whichever source it is, it goes to the SDK as-is rather than through a + // temporary file, since CreateTDF takes a plain io.Reader. + in, cleanup := piped, func() {} + if filePath != "" { + in, cleanup, err = streamio.OpenFile(filePath) if err != nil { - cli.ExitWithError("Failed to read stdin:", err) + cli.ExitWithError("Failed to read "+inputName+":", 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() instead of relying on this. defer cleanup() + // fail does not return -- cli.ExitWithError ends in os.Exit -- but call sites + // return immediately afterwards so encryptRun reads top to bottom. With -o it + // discards the partial TDF, the no-partial-output guarantee streamio.OutputFile + // exists to provide; a stdout destination cannot be given one, since the bytes + // are already gone. Declared before tdfFile exists, which is why the nil guard. + var tdfFile *streamio.OutputFile + fail := func(msg string, err error) { + if tdfFile != nil { + tdfFile.Cleanup() + } + cleanup() + cli.ExitWithError(msg, err) + } + // Resolve the destination before encrypting, so the payload streams straight // to it rather than accumulating in memory first. var dest io.Writer - var tdfFile *streamio.OutputFile if out != "" { // make sure output ends in .tdf extension if !strings.HasSuffix(out, ".tdf") { @@ -150,8 +179,8 @@ func encryptRun(cmd *cobra.Command, args []string) { } tdfFile, err = streamio.NewOutputFile(out, encryptedOutputFileMode) if err != nil { - cleanup() - cli.ExitWithError("Failed to write encrypted file "+out, err) + fail("Failed to write encrypted file "+out, err) + return } defer tdfFile.Cleanup() dest = tdfFile @@ -159,23 +188,26 @@ func encryptRun(cmd *cobra.Command, args []string) { dest = os.Stdout } - fail := func(msg string, err error) { - if tdfFile != nil { - tdfFile.Cleanup() - } - cleanup() - cli.ExitWithError(msg, err) - } - // auto-detect mime type if not provided if fileMimeType == "" { slog.Debug("detecting mime type of file") - fileMimeType, err = detectMimeType(in, fileExt) + // Assigned through temporaries rather than straight into fileMimeType and + // in: on error detectMimeType returns a nil reader, and writing that into + // in would arm a nil dereference for anyone who later drops the return. + mimeType, rest, err := detectMimeType(in, fileExt) if err != nil { - fail("Failed to read file:", err) + fail("Failed to read "+inputName+" to detect its type (pass --mime-type to skip detection):", err) + return } + fileMimeType, in = mimeType, rest } slog.Debug("encrypting file", slog.String("mime_type", fileMimeType)) + // Worth a breadcrumb: the same bytes encrypt to a slightly larger TDF this + // way, and nothing else tells the user why. Asked here rather than inside + // detectMimeType so --mime-type, which skips detection entirely, still logs. + if !streamio.Measurable(in) { + slog.Debug("payload length is not knowable up front; writing a ZIP64 archive") + } // Do the encryption err = h.Encrypt(c.Context(), dest, in, handlers.EncryptOptions{ @@ -188,12 +220,17 @@ func encryptRun(cmd *cobra.Command, args []string) { TargetMode: targetMode, }) if err != nil { + // The return matters: the Commit below would otherwise rename a + // partially-written TDF into place, which is the exact outcome + // streamio.OutputFile exists to prevent. fail("Failed to encrypt", err) + return } if tdfFile != nil { if err := tdfFile.Commit(); err != nil { fail("Failed to write encrypted file "+out, err) + return } } } diff --git a/otdfctl/cmd/tdf/encrypt_test.go b/otdfctl/cmd/tdf/encrypt_test.go index 8c22bea7ed..8e398f7d36 100644 --- a/otdfctl/cmd/tdf/encrypt_test.go +++ b/otdfctl/cmd/tdf/encrypt_test.go @@ -2,15 +2,50 @@ package tdf import ( "bytes" + "crypto/sha256" + "errors" "io" + "os" "strings" + "syscall" "testing" + "github.com/opentdf/platform/otdfctl/pkg/streamio" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestDetectMimeTypeRewindsForTheEncoder(t *testing.T) { +// 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) } + +// espipeReader implements io.Seeker and refuses every seek, the way an *os.File +// on a FIFO, a process substitution, or /dev/stdin on the end of a pipe does. +// Neither of the other two shapes covers that: strings.Reader always seeks, +// pipeReader hides Seek entirely. +type espipeReader struct{ inner io.Reader } + +func (r espipeReader) Read(p []byte) (int, error) { return r.inner.Read(p) } + +func (r espipeReader) Seek(int64, int) (int64, error) { + return 0, &os.PathError{Op: "seek", Path: "/dev/fd/63", Err: syscall.ESPIPE} +} + +// readerKinds are the three input shapes detectMimeType has to cope with. Only +// the first can be rewound, so only the first may come back measurable. +var readerKinds = []struct { + name string + wrap func(io.Reader) io.Reader + rewindable bool +}{ + {name: "seekable", wrap: func(r io.Reader) io.Reader { return r }, rewindable: true}, + {name: "pipe", wrap: func(r io.Reader) io.Reader { return pipeReader{r} }}, + {name: "refuses to seek", wrap: func(r io.Reader) io.Reader { return espipeReader{r} }}, +} + +func TestDetectMimeTypePreservesThePayload(t *testing.T) { for _, tc := range []struct { name string content string @@ -18,31 +53,125 @@ 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"}, + // Exactly the sniff window: the shortest input that fills the buffer, so + // the shortest for which io.ReadFull returns a nil error rather than + // io.ErrUnexpectedEOF, and the boundary at which replaying the prefix + // could duplicate or drop a byte. + {name: "exactly the sniff window", content: strings.Repeat("a", Size1MB), want: "text/plain; charset=utf-8"}, // 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 _, kind := range readerKinds { + t.Run(kind.name, func(t *testing.T) { + in := kind.wrap(strings.NewReader(tc.content)) + + got, rest, err := detectMimeType(in, "") + require.NoError(t, err) + assert.Equal(t, tc.want, got) - got, err := detectMimeType(in, "") - require.NoError(t, err) - assert.Equal(t, tc.want, got) + // A stream that came back looking seekable would claim a + // length the SDK then could not read. + assert.Equal(t, kind.rewindable, streamio.Measurable(rest), + "measurability must match rewindability") - // The whole payload must still reach the encoder. - all, err := io.ReadAll(in) - require.NoError(t, err) - assert.Len(t, all, len(tc.content)) + all, err := io.ReadAll(rest) + require.NoError(t, err) + assertSamePayload(t, tc.content, string(all)) + }) + } }) } } +// assertSamePayload compares payloads without rendering a megabyte-wide diff for +// the cases deliberately larger than the sniff window. +func assertSamePayload(t *testing.T, want, got string) { + t.Helper() + + const diffLimit = 4096 + if len(want) <= diffLimit && len(got) <= diffLimit { + assert.Equal(t, want, got) + return + } + // Length first, since truncation is what these cases exist to catch, then + // content by digest -- assert.Equal on a megabyte renders a useless diff. + assert.Len(t, got, len(want), "payload length") + assert.Equal(t, sha256.Sum256([]byte(want)), sha256.Sum256([]byte(got)), "payload digest") +} + +// A sniffed input that came back wrapped in io.MultiReader would silently cost +// every file encrypt its declared length, and nothing else would fail. +func TestDetectMimeTypeKeepsSeekability(t *testing.T) { + in := strings.NewReader("hello, world\n") + + _, rest, err := detectMimeType(in, "") + require.NoError(t, err) + + // Same, not merely "is an io.Seeker": the documented contract is that a + // rewindable input is handed back unchanged. + assert.Same(t, in, rest, "a rewindable input must be handed back unchanged") +} + +// A reader handed over mid-payload must be rewound to where it was, not to zero: +// `{ read -r header; otdfctl encrypt; } < payload.txt` leaves stdin part-consumed, +// and seeking to the start would encrypt the header the wrapper already took. +func TestDetectMimeTypeRestoresANonZeroOffset(t *testing.T) { + const header, payload = "header line\n", "the actual payload\n" + + in := strings.NewReader(header + payload) + _, err := in.Seek(int64(len(header)), io.SeekStart) + require.NoError(t, err) + + _, rest, err := detectMimeType(in, "") + require.NoError(t, err) + + got, err := io.ReadAll(rest) + require.NoError(t, err) + assert.Equal(t, payload, string(got), "the payload is what remains, not the whole file") +} + +// errAfterNReader yields n bytes and then fails with something that is neither +// EOF nor ErrUnexpectedEOF: a failing device, or a socket reset mid-read. Not a +// dying shell producer -- `failing-cmd | otdfctl encrypt` closes the write end, +// which reads as a clean EOF that nothing here can tell from a whole payload. +type errAfterNReader struct { + remaining int + err error +} + +func (r *errAfterNReader) Read(p []byte) (int, error) { + if r.remaining == 0 { + return 0, r.err + } + n := min(len(p), r.remaining) + for i := range n { + p[i] = 'a' + } + r.remaining -= n + return n, nil +} + +// Detection is the first thing that touches the payload, so swallowing a read +// error here would encrypt a truncated payload into a TDF that looks complete. +func TestDetectMimeTypeReportsAReadFailure(t *testing.T) { + want := errors.New("the device gave up") + + _, rest, err := detectMimeType(&errAfterNReader{remaining: 16, err: want}, "") + require.ErrorIs(t, err, want) + + // The nil is contractual: encryptRun assigns the returned reader through a + // temporary so a failed detection cannot install it as the payload. + assert.Nil(t, rest, "a failed detection must not hand back a reader") +} + 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 +182,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 +190,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/cmd/tdf/inspect.go b/otdfctl/cmd/tdf/inspect.go index e28e7980e8..f2ded24f80 100644 --- a/otdfctl/cmd/tdf/inspect.go +++ b/otdfctl/cmd/tdf/inspect.go @@ -57,7 +57,8 @@ func inspectRun(cmd *cobra.Command, args []string) { } // cli.ExitWithError calls os.Exit, which does not run deferred functions, so // cleanup is also invoked explicitly before every exit below — including the - // successful one, since piped input is spooled to a temporary file. + // successful one, since a piped TDF is spooled to a temporary file. A file + // argument or a redirect from one is seekable already and is not spooled. defer cleanup() result, errs := h.InspectTDF(in) diff --git a/otdfctl/e2e/streaming.bats b/otdfctl/e2e/streaming.bats index 9bf1c69a28..c4077dad78 100755 --- a/otdfctl/e2e/streaming.bats +++ b/otdfctl/e2e/streaming.bats @@ -78,12 +78,26 @@ assert_no_leftovers() { assert_output --partial "$SECRET_TEXT" } -# A TDF's manifest lives at the end of the archive, so decrypt spools a pipe to -# disk to get a seekable view. Verify it round-trips and removes the spool. -@test "roundtrip TDF3, decrypt reading the TDF from stdin" { +# A TDF's manifest lives at the end of the archive, so decrypt needs a seekable +# view of it. A pipe cannot give one, so it is spooled to disk. Verify it +# round-trips and removes the spool. +@test "roundtrip TDF3, decrypt reading the TDF from a pipe" { ./otdfctl encrypt -o "$TDF_OUT" $COMMON "$PLAIN" # Scope TMPDIR to this test so the leftover check cannot see another test's # spool, and cannot be fooled by one either. + run bash -c "TMPDIR='$BATS_TEST_TMPDIR' cat '$TDF_OUT' | ./otdfctl decrypt $COMMON >'$RESULT'" + assert_success + diff "$PLAIN" "$RESULT" + + assert_no_leftovers "$BATS_TEST_TMPDIR/otdfctl-spool-*" +} + +# A redirect from a regular file seeks already, so there is nothing to spool. +# What this pins is the round-trip; that no spool is created at all is not +# observable from out here, and TestOpenSeekableDoesNotSpoolARegularFileStdin +# covers it. +@test "roundtrip TDF3, decrypt reading the TDF from a redirect" { + ./otdfctl encrypt -o "$TDF_OUT" $COMMON "$PLAIN" TMPDIR="$BATS_TEST_TMPDIR" ./otdfctl decrypt $COMMON <"$TDF_OUT" >"$RESULT" diff "$PLAIN" "$RESULT" @@ -163,6 +177,121 @@ assert_no_leftovers() { [ -c /dev/null ] } +# assert_payload_first checks the premise extra_field_len rests on: the archive +# opens with the payload's local file header. If the SDK ever reorders entries, +# this fails here rather than letting extra_field_len read two arbitrary bytes. +assert_payload_first() { + [ -s "$1" ] || fail "$1 is missing or empty" + # 'PK\3\4' as decimal bytes; xargs normalizes od's column padding, which BSD + # and GNU space differently. + run bash -c "od -An -tu1 -N4 '$1' | xargs" + assert_output "80 75 3 4" + # The filename follows the 30-byte header. Always '0.payload', so 9 bytes. + run bash -c "dd if='$1' bs=1 skip=30 count=9 status=none" + assert_output "0.payload" +} + +# extra_field_len reads the extra field length at offset 28 of the payload's +# local file header: non-zero for ZIP64, which carries the extended information +# extra field there, zero for ZIP32. od -tu1 rather than -tu2 because only GNU od +# can be told the endianness. +# +# It fails rather than returning a number when it cannot read a full header -- +# empty output would evaluate to 0, which is what the ZIP32 assertions pass on. +extra_field_len() { + local lo hi + [ -s "$1" ] || { echo "extra_field_len: $1 is missing or empty" >&2; return 1; } + read -r lo hi <<<"$(od -An -tu1 -j28 -N2 "$1")" + if [ -z "$lo" ] || [ -z "$hi" ]; then + echo "extra_field_len: $1 is too short to hold a local file header" >&2 + return 1 + fi + echo $((lo + hi * 256)) +} + +# Encrypting from a pipe no longer spools, so the payload cannot be measured and +# the archive has to be ZIP64; anything measurable stays ZIP32. The layout is what +# to assert on, since both forms round-trip and 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" + assert_payload_first "$TDF_OUT" + [ "$(extra_field_len "$TDF_OUT")" -eq 0 ] + + # --mime-type skips detection entirely, which is the one thing that could have + # wrapped the file and cost it the ZIP32 layout. It must not change the layout. + local typed_tdf="$BATS_TEST_TMPDIR/typed.tdf" + ./otdfctl encrypt -o "$typed_tdf" $COMMON --mime-type text/plain "$PLAIN" + assert_payload_first "$typed_tdf" + [ "$(extra_field_len "$typed_tdf")" -eq 0 ] + + # A redirect arrives on stdin but seeks, so it stays ZIP32. Dropping the spool + # must not turn the whole of stdin into a stream. + local redirected_tdf="$BATS_TEST_TMPDIR/redirected.tdf" + run bash -c "./otdfctl encrypt $COMMON <'$PLAIN' >'$redirected_tdf'" + assert_success + assert_payload_first "$redirected_tdf" + [ "$(extra_field_len "$redirected_tdf")" -eq 0 ] + + local piped_tdf="$BATS_TEST_TMPDIR/piped.tdf" + run bash -c "echo '$SECRET_TEXT' | ./otdfctl encrypt $COMMON >'$piped_tdf'" + assert_success + assert_payload_first "$piped_tdf" + [ "$(extra_field_len "$piped_tdf")" -gt 0 ] + + ./otdfctl decrypt -o "$RESULT" $COMMON "$piped_tdf" + run cat "$RESULT" + assert_output "$SECRET_TEXT" +} + +# Process substitution is the shape a type assertion gets wrong: bash hands us +# /dev/fd/63, which opens as an *os.File and so satisfies io.Seeker, and then +# every seek returns ESPIPE. Only unit tests with a hand-written fake model that, +# so it is worth an end-to-end case. +@test "encrypt streams a process substitution" { + local subst_tdf="$BATS_TEST_TMPDIR/subst.tdf" + run bash -c "./otdfctl encrypt $COMMON <(cat '$PLAIN') >'$subst_tdf'" + assert_success + assert_payload_first "$subst_tdf" + # Unmeasurable, so ZIP64, exactly like a pipe. + [ "$(extra_field_len "$subst_tdf")" -gt 0 ] + + ./otdfctl decrypt -o "$RESULT" $COMMON "$subst_tdf" + diff "$PLAIN" "$RESULT" + + # --mime-type skips detection, so the ESPIPE is hit later, by the SDK's own + # sizing attempt rather than by detectMimeType's. + local typed_subst_tdf="$BATS_TEST_TMPDIR/subst-typed.tdf" + run bash -c "./otdfctl encrypt $COMMON --mime-type text/plain <(cat '$PLAIN') >'$typed_subst_tdf'" + assert_success + assert_payload_first "$typed_subst_tdf" + [ "$(extra_field_len "$typed_subst_tdf")" -gt 0 ] + + ./otdfctl decrypt -o "$RESULT" $COMMON "$typed_subst_tdf" + diff "$PLAIN" "$RESULT" +} + +# 3 MiB clears the SDK's 2 MiB default segment size, so the archive spans several +# segments whose hashes are combined on the way out and verified on the way back +# in. A pipe also feeds the encoder short reads at segment boundaries; a file +# does not. +@test "encrypt streams a multi-segment pipe" { + local big="$BATS_TEST_TMPDIR/multi.bin" + local bigtdf="$BATS_TEST_TMPDIR/multi.tdf" + local bigout="$BATS_TEST_TMPDIR/multi.out" + dd if=/dev/urandom of="$big" bs=1048576 count=3 status=none + + # A real pipe, not a redirect: a redirect from a regular file seeks, and would + # be measured and written as ZIP32. + run bash -c "cat '$big' | ./otdfctl encrypt $COMMON >'$bigtdf'" + assert_success + assert_payload_first "$bigtdf" + [ "$(extra_field_len "$bigtdf")" -gt 0 ] + + ./otdfctl decrypt -o "$bigout" $COMMON "$bigtdf" + cmp "$big" "$bigout" +} + # 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 0a2bb794c5..bd91fbd81b 100644 --- a/otdfctl/pkg/handlers/tdf.go +++ b/otdfctl/pkg/handlers/tdf.go @@ -52,10 +52,17 @@ 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 one is measurable, and the SDK gives a +// measured payload two things an unmeasurable one cannot have. It gets the +// compact ZIP32 layout below ~2 GiB, where an unmeasurable payload pays a few +// dozen bytes for ZIP64 — a choice fixed before the first segment goes out. And +// it gets a length check, so a reader that runs dry early fails instead of +// yielding a complete-looking TDF holding a truncated payload. +// +// Neither is worth spooling a stream to disk for: needing a writable temp +// directory to encrypt a pipe costs more than it buys. This is what the rest of +// otdfctl means by "keeping the payload measurable"; see streamio.Measurable. +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/otdfctl/pkg/streamio/input.go b/otdfctl/pkg/streamio/input.go index dc7c29a874..a09f88d95c 100644 --- a/otdfctl/pkg/streamio/input.go +++ b/otdfctl/pkg/streamio/input.go @@ -23,16 +23,71 @@ const PipeBufferSize = 1024 * 1024 // callers report differently. var ErrNoInput = errors.New("no input provided") -// PipeReader reports whether in is a pipe or redirect carrying at least one -// byte, and returns a reader over it. +// Seekable reports r's current position, and whether it could be asked for one. // -// Presence is established with a one-byte Peek rather than a read, so the -// payload still reaches the caller intact and nothing is buffered beyond the -// reader's window. A terminal, or an empty redirect such as -// `otdfctl encrypt < /dev/null`, reports false — matching the behavior of a -// buffered implementation, which decides the same question by checking whether -// a full read came back empty. -func PipeReader(in *os.File) (*bufio.Reader, bool, error) { +// It probes rather than asserting io.Seeker, because an *os.File on a FIFO, a +// process substitution, or /dev/stdin on the end of a pipe all satisfy the +// interface and then return ESPIPE. Seeking to the current position never moves +// it, so this is safe on a payload about to be read. +func Seekable(r io.Reader) (io.Seeker, int64, bool) { + seeker, ok := r.(io.Seeker) + if !ok { + return nil, 0, false + } + off, err := seeker.Seek(0, io.SeekCurrent) + if err != nil { + return nil, 0, false + } + return seeker, off, true +} + +// Measurable reports whether the SDK can size r without reading it, and so +// whether the archive gets the compact layout. See handlers.Handler's Encrypt. +func Measurable(r io.Reader) bool { + _, _, ok := Seekable(r) + return ok +} + +// unmeasurable hides a reader's Seek method, so anything probing for one sizes +// the payload by reading it instead. +type unmeasurable struct{ io.Reader } + +// OpenFile opens path for reading, ready to hand to the SDK as-is. +// +// The file stays measurable, except when its stat cannot be trusted: a procfs or +// sysfs file is a regular file that reports zero bytes and then reads out +// content. Measuring one declares an empty payload, and the SDK limits its reads +// to the length it was given, so the encrypt would succeed with nothing in it. +// Those come back with the Seeker hidden — an archive a few dozen bytes larger +// beats an archive missing the payload. +// +// cleanup is always non-nil, including on the error return. +func OpenFile(path string) (io.Reader, func(), error) { + f, err := os.Open(path) + if err != nil { + return nil, func() {}, err + } + cleanup := func() { f.Close() } + stat, err := f.Stat() + if err != nil { + cleanup() + return nil, func() {}, err + } + if stat.Mode().IsRegular() && stat.Size() == 0 { + return unmeasurable{f}, cleanup, nil + } + return f, cleanup, nil +} + +// PipeReader reports whether in carries at least one byte of payload, and +// returns a reader over it. A terminal, or an empty redirect such as +// `otdfctl encrypt < /dev/null`, reports false. +// +// A redirect from a regular file — `otdfctl encrypt < payload.txt` — is handed +// back as the *os.File itself, so the payload stays measurable. Wrapping it +// would throw that away for nothing: it is the same fd either way. Everything +// else is a stream, and gets a buffered reader. +func PipeReader(in *os.File) (io.Reader, bool, error) { stat, err := in.Stat() if err != nil { return nil, false, err @@ -41,6 +96,21 @@ func PipeReader(in *os.File) (*bufio.Reader, bool, error) { return nil, false, nil } + // Presence comes from the stat rather than a Peek, which would advance the + // fd the caller is about to be handed. Measured from the current offset, not + // zero: `{ read -r header; otdfctl encrypt; } < payload.txt` leaves stdin + // mid-file, and what remains is the payload. + // + // Only a stat that counts bytes ahead of the offset gets to decide, since a + // zero-byte procfs file reads out content anyway (see OpenFile). Everything + // else falls through to the Peek, which asks the file instead of the stat and + // still answers absent for a genuinely empty or already-consumed one. + if stat.Mode().IsRegular() { + if _, off, ok := Seekable(in); ok && stat.Size() > off { + return in, true, nil + } + } + r := bufio.NewReaderSize(in, PipeBufferSize) if _, err := r.Peek(1); err != nil { if errors.Is(err, io.EOF) { @@ -82,25 +152,29 @@ func Spool(r io.Reader) (*os.File, func(), error) { } // OpenSeekable resolves a command's input to something seekable: the named file -// when one is given, otherwise piped stdin spooled to disk. It returns -// ErrNoInput for the same "nothing to read" condition whether the file argument -// was absent or the pipe was empty. +// when one is given, otherwise stdin. It returns ErrNoInput for the same +// "nothing to read" condition whether the file argument was absent or the pipe +// was empty. +// +// Only what cannot seek is spooled to disk. A named file or a redirect from one +// already seeks, and copying it would need a TMPDIR with room for the whole TDF +// to buy nothing. // // The returned cleanup must run on every path, per Spool. func OpenSeekable(path string) (*os.File, func(), error) { if path != "" { - f, err := os.Open(path) + in, cleanup, err := OpenFile(path) if err != nil { return nil, func() {}, err } - if _, err := f.Seek(0, io.SeekCurrent); err == nil { - return f, func() { f.Close() }, nil + if f, ok := in.(*os.File); ok && Measurable(f) { + return f, cleanup, nil } - // Not seekable (a FIFO or /dev/fd/N): spool it like piped stdin so - // callers still get something they can seek. - spooled, cleanup, err := Spool(f) - f.Close() - return spooled, cleanup, err + // A FIFO, /dev/fd/N, or a file OpenFile would not vouch for: spool it + // like piped stdin so callers still get something they can seek. + spooled, spoolCleanup, err := Spool(in) + cleanup() + return spooled, spoolCleanup, err } piped, ok, err := PipeReader(os.Stdin) @@ -110,5 +184,11 @@ func OpenSeekable(path string) (*os.File, func(), error) { if !ok { return nil, func() {}, ErrNoInput } + if f, isFile := piped.(*os.File); isFile { + // PipeReader only hands back the file for a regular-file redirect, which + // is seekable already. The cleanup stays empty on purpose: this is + // os.Stdin, and closing it out from under the process is not ours to do. + return f, func() {}, nil + } return Spool(piped) } diff --git a/otdfctl/pkg/streamio/input_test.go b/otdfctl/pkg/streamio/input_test.go index 2c8f8ad3d5..fdf78a27d7 100644 --- a/otdfctl/pkg/streamio/input_test.go +++ b/otdfctl/pkg/streamio/input_test.go @@ -81,6 +81,171 @@ func TestPipeReaderOnTerminalReportsAbsent(t *testing.T) { assert.False(t, ok) } +// openRegularStdin stands in for `otdfctl encrypt < payload.txt`: a redirect +// from a regular file is not a pipe, and PipeReader has to notice. +func openRegularStdin(t *testing.T, content string) *os.File { + t.Helper() + + path := filepath.Join(t.TempDir(), "payload.txt") + require.NoError(t, os.WriteFile(path, []byte(content), 0o600)) + f, err := os.Open(path) + require.NoError(t, err) + t.Cleanup(func() { f.Close() }) + return f +} + +// A regular-file redirect must come back as the file itself. Wrapping it would +// cost the payload its length for nothing -- it is the same fd either way. +func TestPipeReaderRegularFileStaysSeekable(t *testing.T) { + const content = "hello from a redirect\n" + f := openRegularStdin(t, content) + + got, ok, err := PipeReader(f) + require.NoError(t, err) + require.True(t, ok) + + assert.Same(t, f, got, "a regular file must be handed back unwrapped") + + // Presence came from the stat, so nothing was read on the way through. + off, err := f.Seek(0, io.SeekCurrent) + require.NoError(t, err) + assert.Zero(t, off, "detecting presence must not consume the payload") + + all, err := io.ReadAll(got) + require.NoError(t, err) + assert.Equal(t, content, string(all)) +} + +// An empty redirect is "no input", not "a zero-byte payload" -- the same answer +// the Peek gives for an empty pipe. +func TestPipeReaderEmptyRegularFileReportsAbsent(t *testing.T) { + got, ok, err := PipeReader(openRegularStdin(t, "")) + require.NoError(t, err) + assert.False(t, ok) + assert.Nil(t, got) +} + +// Presence is measured from the current offset, not from zero: +// `{ read -r hdr; otdfctl encrypt; } < f` leaves stdin mid-file, and what +// remains is the payload. +func TestPipeReaderRegularFileAtNonZeroOffset(t *testing.T) { + const header, payload = "header line\n", "the actual payload\n" + f := openRegularStdin(t, header+payload) + _, err := f.Seek(int64(len(header)), io.SeekStart) + require.NoError(t, err) + + got, ok, err := PipeReader(f) + require.NoError(t, err) + require.True(t, ok) + + all, err := io.ReadAll(got) + require.NoError(t, err) + assert.Equal(t, payload, string(all), "the payload is what remains, not the whole file") +} + +// A file consumed to its end has nothing left to encrypt, so it reports absent +// even though the file itself is not empty. +func TestPipeReaderRegularFileAtEOFReportsAbsent(t *testing.T) { + const content = "already read\n" + f := openRegularStdin(t, content) + _, err := f.Seek(int64(len(content)), io.SeekStart) + require.NoError(t, err) + + _, ok, err := PipeReader(f) + require.NoError(t, err) + assert.False(t, ok) +} + +// A procfs file is a readable regular file that stats as zero bytes, so the +// stat cannot be the one to say whether a payload is there. Deciding on it alone +// turned `otdfctl encrypt < /proc/cpuinfo` into "no input". +func TestPipeReaderZeroSizedRegularFileWithContent(t *testing.T) { + f, err := os.Open(zeroSizedFileWithContent(t)) + require.NoError(t, err) + defer f.Close() + + got, ok, err := PipeReader(f) + require.NoError(t, err) + require.True(t, ok, "a file that stats as empty may still have content") + + all, err := io.ReadAll(got) + require.NoError(t, err) + assert.NotEmpty(t, all, "the payload has to survive the presence check") +} + +// zeroSizedFileWithContent names a file that stats as zero bytes and reads out +// content anyway. Only procfs and its kin do that, and nothing in a TempDir can +// be made to, so this skips everywhere else. +func zeroSizedFileWithContent(t *testing.T) string { + t.Helper() + + if runtime.GOOS != "linux" { + t.Skip("a readable regular file that stats as zero bytes needs procfs") + } + const path = "/proc/self/status" + stat, err := os.Stat(path) + require.NoError(t, err) + require.True(t, stat.Mode().IsRegular(), "the premise of this test") + require.Zero(t, stat.Size(), "the premise of this test") + return path +} + +// Nothing between here and sdk.CreateTDF would fail if a wrapper hid the Seeker; +// every file encrypt would just quietly stop declaring its length. +func TestOpenFileStaysMeasurable(t *testing.T) { + const content = "hello, world\n" + path := filepath.Join(t.TempDir(), "payload.txt") + require.NoError(t, os.WriteFile(path, []byte(content), 0o600)) + + in, cleanup, err := OpenFile(path) + require.NoError(t, err) + defer cleanup() + + assert.True(t, Measurable(in), "a file argument must reach the SDK measurable") + + got, err := io.ReadAll(in) + require.NoError(t, err) + assert.Equal(t, content, string(got)) +} + +// A stat of zero is not a measurement. The SDK limits its reads to the length it +// was given, so undercounting is worse than not counting: the encrypt succeeds +// and the TDF holds nothing. +func TestOpenFileDoesNotMeasureAZeroSizedFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "empty.txt") + require.NoError(t, os.WriteFile(path, nil, 0o600)) + + in, cleanup, err := OpenFile(path) + require.NoError(t, err) + defer cleanup() + + assert.False(t, Measurable(in)) +} + +// The case that branch exists for: procfs reports zero and then reads out +// content. Both halves matter -- io.ReadAll drains the file either way, and it is +// the SDK's io.LimitReader, sized from the measurement, that would stop at zero. +func TestOpenFileReadsAZeroSizedFileWhole(t *testing.T) { + in, cleanup, err := OpenFile(zeroSizedFileWithContent(t)) + require.NoError(t, err) + defer cleanup() + + assert.False(t, Measurable(in), "the stat undercounts, so it must not be trusted") + + got, err := io.ReadAll(in) + require.NoError(t, err) + assert.NotEmpty(t, got, "the payload has to reach the SDK") +} + +func TestOpenFileReportsAMissingFile(t *testing.T) { + _, cleanup, err := OpenFile(filepath.Join(t.TempDir(), "absent.txt")) + require.ErrorIs(t, err, os.ErrNotExist) + + // cleanup is non-nil even here, so a caller may defer it without a nil guard. + require.NotNil(t, cleanup) + cleanup() +} + func TestSpoolIsSeekableAndComplete(t *testing.T) { // Larger than any plausible internal buffer, so a truncating copy shows up. content := strings.Repeat("xyz", PipeBufferSize) @@ -131,8 +296,7 @@ func TestOpenSeekableReadsNamedFile(t *testing.T) { require.NoError(t, err) assert.Equal(t, "payload", string(got)) - // A named file is opened directly, not copied through a spool. - assert.Equal(t, path, in.Name()) + assert.Equal(t, path, in.Name(), "a named file must not be copied through a spool") } func TestOpenSeekableReportsMissingFile(t *testing.T) { @@ -169,9 +333,7 @@ func TestOpenSeekableSpoolsNonSeekableNamedFile(t *testing.T) { require.NoError(t, err) assert.Equal(t, content, string(got)) - // A FIFO's own bytes were consumed into the spool; the caller's handle is a - // distinct, seekable temp file. - assert.NotEqual(t, path, in.Name()) + assert.NotEqual(t, path, in.Name(), "the FIFO's bytes went into a temp file") _, err = in.Seek(0, io.SeekStart) require.NoError(t, err, "the whole point of spooling is that the result seeks") } @@ -197,6 +359,33 @@ func TestOpenSeekableReadsFromStdinPipe(t *testing.T) { got, err := io.ReadAll(in) require.NoError(t, err) assert.Equal(t, content, string(got)) + + assert.NotSame(t, os.Stdin, in, "a real stream has to be spooled to seek") +} + +// A redirect from a regular file seeks already. Spooling it would copy the whole +// TDF into TMPDIR to produce a handle no better than the one we had. +func TestOpenSeekableDoesNotSpoolARegularFileStdin(t *testing.T) { + const content = "redirected stdin, read in place\n" + + origStdin := os.Stdin + os.Stdin = openRegularStdin(t, content) + defer func() { os.Stdin = origStdin }() + + in, cleanup, err := OpenSeekable("") + require.NoError(t, err) + defer cleanup() + + assert.Same(t, os.Stdin, in, "a regular-file redirect must not be spooled") + + got, err := io.ReadAll(in) + require.NoError(t, err) + assert.Equal(t, content, string(got)) + + // Closing stdin is not ours to do, so cleanup has to leave it usable. + cleanup() + _, err = in.Seek(0, io.SeekStart) + require.NoError(t, err, "cleanup must not close stdin") } func TestOpenSeekableReturnsErrNoInputForEmptyStdinPipe(t *testing.T) { From 4c8190061e75ba9ba8c6e232e064ed0af292df27 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Fri, 25 Sep 2026 11:48:51 -0400 Subject: [PATCH 2/2] refactor(cli): give streamio one input resolver encrypt open-coded the file-argument-or-stdin choice that decrypt and inspect already got from streamio, and PipeReader was named for the case it handles most easily rather than the one that is hard. streamio.Open resolves either source without copying, preferring the file argument; OpenExclusive is the same for a command that refuses to guess, reporting ErrTwoInputs instead of silently dropping one of two payloads. OpenSeekable is now Open plus a spool, which drops its duplicate stdin branch, and encrypt's input block becomes one call and a switch. OpenFile is unexported behind them, and PipeReader is Piped. Behavior is unchanged: encrypt still rejects two inputs, decrypt and inspect still prefer the file argument and never touch a stdin they were not given -- Open leaves it unread, so a command inside a while-read loop cannot eat the loop's own input. --- otdfctl/cmd/tdf/encrypt.go | 36 ++---- otdfctl/pkg/cli/pipe.go | 6 +- otdfctl/pkg/streamio/input.go | 121 +++++++++++------- otdfctl/pkg/streamio/input_test.go | 190 ++++++++++++++++++++++------- 4 files changed, 230 insertions(+), 123 deletions(-) diff --git a/otdfctl/cmd/tdf/encrypt.go b/otdfctl/cmd/tdf/encrypt.go index 1ccafdaf20..49d02b566c 100644 --- a/otdfctl/cmd/tdf/encrypt.go +++ b/otdfctl/cmd/tdf/encrypt.go @@ -115,28 +115,6 @@ func encryptRun(cmd *cobra.Command, args []string) { wrappingKeyAlgorithm = ocrypto.RSA2048Key } - piped, hasPiped, err := streamio.PipeReader(os.Stdin) - if err != nil { - cli.ExitWithError("failed to scan bytes from stdin", err) - } - - inputCount := 0 - if filePath != "" { - inputCount++ - } - if hasPiped { - inputCount++ - } - - cliExit := func(s string) { - cli.ExitWithError("Must provide "+s+" of the following to encrypt: [file argument, stdin input]", nil) - } - if inputCount == 0 { - cliExit("ONE") - } else if inputCount > 1 { - cliExit("ONLY ONE") - } - inputName := "stdin" if filePath != "" { inputName = filePath @@ -144,12 +122,14 @@ func encryptRun(cmd *cobra.Command, args []string) { // Whichever source it is, it goes to the SDK as-is rather than through a // temporary file, since CreateTDF takes a plain io.Reader. - in, cleanup := piped, func() {} - if filePath != "" { - in, cleanup, err = streamio.OpenFile(filePath) - if err != nil { - cli.ExitWithError("Failed to read "+inputName+":", err) - } + in, cleanup, err := streamio.OpenExclusive(filePath) + switch { + case errors.Is(err, streamio.ErrNoInput): + cli.ExitWithError("Must provide ONE of the following to encrypt: [file argument, stdin input]", err) + case errors.Is(err, streamio.ErrTwoInputs): + cli.ExitWithError("Must provide ONLY ONE of the following to encrypt: [file argument, stdin input]", err) + case err != nil: + cli.ExitWithError("Failed to read "+inputName+":", err) } // cli.ExitWithError calls os.Exit, which skips deferred functions, so every // exit below goes through fail() instead of relying on this. diff --git a/otdfctl/pkg/cli/pipe.go b/otdfctl/pkg/cli/pipe.go index 8d0472960d..73697801fa 100644 --- a/otdfctl/pkg/cli/pipe.go +++ b/otdfctl/pkg/cli/pipe.go @@ -30,10 +30,10 @@ func ReadFromArgsOrPipe(args []string, pipe *os.File) []byte { } // Deprecated: reads the entire pipe into memory and terminates the process on -// failure. Use streamio.PipeReader, which reports whether input is present -// without consuming it, paired with io.ReadAll for the equivalent []byte. +// failure. Use streamio.Piped, which reports whether input is present without +// consuming it, paired with io.ReadAll for the equivalent []byte. func ReadFromPipe(in *os.File) []byte { - r, ok, err := streamio.PipeReader(in) + r, ok, err := streamio.Piped(in) if err != nil { ExitWithError("failed to read stat from stdin", err) } diff --git a/otdfctl/pkg/streamio/input.go b/otdfctl/pkg/streamio/input.go index a09f88d95c..59af55e1cd 100644 --- a/otdfctl/pkg/streamio/input.go +++ b/otdfctl/pkg/streamio/input.go @@ -14,14 +14,20 @@ import ( "os" ) -// PipeBufferSize is the window PipeReader buffers over a pipe — generous -// enough that a typical CLI payload is served from a single read. +// PipeBufferSize is the window Piped buffers over a pipe — generous enough that +// a typical CLI payload is served from a single read. const PipeBufferSize = 1024 * 1024 -// ErrNoInput reports that a command was given neither a file argument nor a -// non-empty pipe. It is distinct from a failure to open a named file, which -// callers report differently. -var ErrNoInput = errors.New("no input provided") +var ( + // ErrNoInput reports that a command was given neither a file argument nor a + // non-empty pipe. It is distinct from a failure to open a named file, which + // callers report differently. + ErrNoInput = errors.New("no input provided") + + // ErrTwoInputs reports that a command which takes one payload was given two. + // Only OpenExclusive returns it; Open prefers the file argument instead. + ErrTwoInputs = errors.New("file argument and stdin input both provided") +) // Seekable reports r's current position, and whether it could be asked for one. // @@ -52,7 +58,7 @@ func Measurable(r io.Reader) bool { // the payload by reading it instead. type unmeasurable struct{ io.Reader } -// OpenFile opens path for reading, ready to hand to the SDK as-is. +// openFile opens path for reading, ready to hand to the SDK as-is. // // The file stays measurable, except when its stat cannot be trusted: a procfs or // sysfs file is a regular file that reports zero bytes and then reads out @@ -62,7 +68,7 @@ type unmeasurable struct{ io.Reader } // beats an archive missing the payload. // // cleanup is always non-nil, including on the error return. -func OpenFile(path string) (io.Reader, func(), error) { +func openFile(path string) (io.Reader, func(), error) { f, err := os.Open(path) if err != nil { return nil, func() {}, err @@ -79,15 +85,16 @@ func OpenFile(path string) (io.Reader, func(), error) { return f, cleanup, nil } -// PipeReader reports whether in carries at least one byte of payload, and -// returns a reader over it. A terminal, or an empty redirect such as -// `otdfctl encrypt < /dev/null`, reports false. +// Piped reports whether in carries at least one byte of payload, and returns a +// reader over it. A terminal, or an empty redirect such as +// `otdfctl encrypt < /dev/null`, reports false. It is named for the usual case +// but takes redirects too, which is the harder half. // // A redirect from a regular file — `otdfctl encrypt < payload.txt` — is handed // back as the *os.File itself, so the payload stays measurable. Wrapping it // would throw that away for nothing: it is the same fd either way. Everything // else is a stream, and gets a buffered reader. -func PipeReader(in *os.File) (io.Reader, bool, error) { +func Piped(in *os.File) (io.Reader, bool, error) { stat, err := in.Stat() if err != nil { return nil, false, err @@ -102,7 +109,7 @@ func PipeReader(in *os.File) (io.Reader, bool, error) { // mid-file, and what remains is the payload. // // Only a stat that counts bytes ahead of the offset gets to decide, since a - // zero-byte procfs file reads out content anyway (see OpenFile). Everything + // zero-byte procfs file reads out content anyway (see openFile). Everything // else falls through to the Peek, which asks the file instead of the stat and // still answers absent for a genuinely empty or already-consumed one. if stat.Mode().IsRegular() { @@ -121,6 +128,49 @@ func PipeReader(in *os.File) (io.Reader, bool, error) { return r, true, nil } +// Open resolves a command's payload: the named file when one is given, +// otherwise stdin. It returns ErrNoInput when there is neither. +// +// Nothing is copied, so a measurable source reaches the caller measurable; see +// Measurable. A file argument wins outright and stdin is not even looked at, +// since a command run inside `while read f; do ... done < list` inherits a stdin +// that is the loop's, not a payload — and looking costs a peek that would eat it. +// +// cleanup is always non-nil, including on the error return. For a piped payload +// it does nothing: closing stdin is not ours to do. +func Open(path string) (io.Reader, func(), error) { + if path != "" { + return openFile(path) + } + piped, ok, err := Piped(os.Stdin) + switch { + case err != nil: + return nil, func() {}, err + case !ok: + return nil, func() {}, ErrNoInput + } + return piped, func() {}, nil +} + +// OpenExclusive is Open for a command that refuses to choose: being handed both +// a file argument and a payload on stdin is ErrTwoInputs rather than a silent +// preference. Telling them apart means inspecting stdin even when the file +// argument would have decided it, which for a pipe consumes what it peeked. +func OpenExclusive(path string) (io.Reader, func(), error) { + piped, hasPiped, err := Piped(os.Stdin) + switch { + case err != nil: + return nil, func() {}, err + case path != "" && hasPiped: + return nil, func() {}, ErrTwoInputs + case path != "": + return openFile(path) + case hasPiped: + return piped, func() {}, nil + } + return nil, func() {}, ErrNoInput +} + // Spool copies r into a temporary file and rewinds it, giving a seekable view // of a stream that has none. // @@ -151,44 +201,25 @@ func Spool(r io.Reader) (*os.File, func(), error) { return f, cleanup, nil } -// OpenSeekable resolves a command's input to something seekable: the named file -// when one is given, otherwise stdin. It returns ErrNoInput for the same -// "nothing to read" condition whether the file argument was absent or the pipe -// was empty. +// OpenSeekable is Open for a command that has to seek its input — reading a +// TDF, whose manifest sits at the end of the archive. // -// Only what cannot seek is spooled to disk. A named file or a redirect from one -// already seeks, and copying it would need a TMPDIR with room for the whole TDF -// to buy nothing. +// Only what cannot seek is spooled to disk: a named file or a redirect from one +// seeks already, and copying it would need a TMPDIR with room for the whole TDF +// to hand back a view no better than the one it had. // // The returned cleanup must run on every path, per Spool. func OpenSeekable(path string) (*os.File, func(), error) { - if path != "" { - in, cleanup, err := OpenFile(path) - if err != nil { - return nil, func() {}, err - } - if f, ok := in.(*os.File); ok && Measurable(f) { - return f, cleanup, nil - } - // A FIFO, /dev/fd/N, or a file OpenFile would not vouch for: spool it - // like piped stdin so callers still get something they can seek. - spooled, spoolCleanup, err := Spool(in) - cleanup() - return spooled, spoolCleanup, err - } - - piped, ok, err := PipeReader(os.Stdin) + in, cleanup, err := Open(path) if err != nil { return nil, func() {}, err } - if !ok { - return nil, func() {}, ErrNoInput - } - if f, isFile := piped.(*os.File); isFile { - // PipeReader only hands back the file for a regular-file redirect, which - // is seekable already. The cleanup stays empty on purpose: this is - // os.Stdin, and closing it out from under the process is not ours to do. - return f, func() {}, nil + if f, ok := in.(*os.File); ok && Measurable(f) { + return f, cleanup, nil } - return Spool(piped) + // A pipe, a FIFO, /dev/fd/N, or a file whose stat openFile would not vouch + // for. Spooling closes over the original, so let go of it either way. + spooled, spoolCleanup, err := Spool(in) + cleanup() + return spooled, spoolCleanup, err } diff --git a/otdfctl/pkg/streamio/input_test.go b/otdfctl/pkg/streamio/input_test.go index fdf78a27d7..363920ce8e 100644 --- a/otdfctl/pkg/streamio/input_test.go +++ b/otdfctl/pkg/streamio/input_test.go @@ -13,7 +13,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestPipeReader(t *testing.T) { +func TestPiped(t *testing.T) { for _, tc := range []struct { name string content string @@ -32,7 +32,7 @@ func TestPipeReader(t *testing.T) { _, _ = io.WriteString(w, tc.content) }() - got, ok, err := PipeReader(r) + got, ok, err := Piped(r) require.NoError(t, err) require.Equal(t, tc.wantOK, ok) if !tc.wantOK { @@ -47,7 +47,7 @@ func TestPipeReader(t *testing.T) { } } -func TestPipeReaderPreservesPayloadLargerThanBuffer(t *testing.T) { +func TestPipedPreservesPayloadLargerThanBuffer(t *testing.T) { content := strings.Repeat("a", PipeBufferSize*2+7) r, w, err := os.Pipe() @@ -59,7 +59,7 @@ func TestPipeReaderPreservesPayloadLargerThanBuffer(t *testing.T) { _, _ = io.WriteString(w, content) }() - got, ok, err := PipeReader(r) + got, ok, err := Piped(r) require.NoError(t, err) require.True(t, ok) @@ -68,7 +68,7 @@ func TestPipeReaderPreservesPayloadLargerThanBuffer(t *testing.T) { assert.Len(t, all, len(content)) } -func TestPipeReaderOnTerminalReportsAbsent(t *testing.T) { +func TestPipedOnTerminalReportsAbsent(t *testing.T) { // A regular file is not a char device, so use os.Stdin's actual mode only // when it is one; otherwise this assertion is vacuous and we skip. stat, err := os.Stdin.Stat() @@ -76,31 +76,63 @@ func TestPipeReaderOnTerminalReportsAbsent(t *testing.T) { if (stat.Mode() & os.ModeCharDevice) == 0 { t.Skip("stdin is not a terminal under this test runner") } - _, ok, err := PipeReader(os.Stdin) + _, ok, err := Piped(os.Stdin) require.NoError(t, err) assert.False(t, ok) } -// openRegularStdin stands in for `otdfctl encrypt < payload.txt`: a redirect -// from a regular file is not a pipe, and PipeReader has to notice. -func openRegularStdin(t *testing.T, content string) *os.File { +// tempFile writes content to a file that lasts as long as the test. +func tempFile(t *testing.T, content string) string { t.Helper() path := filepath.Join(t.TempDir(), "payload.txt") require.NoError(t, os.WriteFile(path, []byte(content), 0o600)) - f, err := os.Open(path) + return path +} + +// openRegularStdin stands in for `otdfctl encrypt < payload.txt`: a redirect +// from a regular file is not a pipe, and Piped has to notice. +func openRegularStdin(t *testing.T, content string) *os.File { + t.Helper() + + f, err := os.Open(tempFile(t, content)) require.NoError(t, err) t.Cleanup(func() { f.Close() }) return f } +// useStdin makes f stdin for the duration of the test. +func useStdin(t *testing.T, f *os.File) { + t.Helper() + + orig := os.Stdin + os.Stdin = f + t.Cleanup(func() { os.Stdin = orig }) +} + +// stdinPipe makes a pipe carrying content stdin. An empty content is a producer +// that writes nothing and hangs up, the same "nothing to read" a caller gets +// from `otdfctl encrypt < /dev/null`. +func stdinPipe(t *testing.T, content string) { + t.Helper() + + r, w, err := os.Pipe() + require.NoError(t, err) + t.Cleanup(func() { r.Close() }) + go func() { + defer w.Close() + _, _ = io.WriteString(w, content) + }() + useStdin(t, r) +} + // A regular-file redirect must come back as the file itself. Wrapping it would // cost the payload its length for nothing -- it is the same fd either way. -func TestPipeReaderRegularFileStaysSeekable(t *testing.T) { +func TestPipedRegularFileStaysSeekable(t *testing.T) { const content = "hello from a redirect\n" f := openRegularStdin(t, content) - got, ok, err := PipeReader(f) + got, ok, err := Piped(f) require.NoError(t, err) require.True(t, ok) @@ -118,8 +150,8 @@ func TestPipeReaderRegularFileStaysSeekable(t *testing.T) { // An empty redirect is "no input", not "a zero-byte payload" -- the same answer // the Peek gives for an empty pipe. -func TestPipeReaderEmptyRegularFileReportsAbsent(t *testing.T) { - got, ok, err := PipeReader(openRegularStdin(t, "")) +func TestPipedEmptyRegularFileReportsAbsent(t *testing.T) { + got, ok, err := Piped(openRegularStdin(t, "")) require.NoError(t, err) assert.False(t, ok) assert.Nil(t, got) @@ -128,13 +160,13 @@ func TestPipeReaderEmptyRegularFileReportsAbsent(t *testing.T) { // Presence is measured from the current offset, not from zero: // `{ read -r hdr; otdfctl encrypt; } < f` leaves stdin mid-file, and what // remains is the payload. -func TestPipeReaderRegularFileAtNonZeroOffset(t *testing.T) { +func TestPipedRegularFileAtNonZeroOffset(t *testing.T) { const header, payload = "header line\n", "the actual payload\n" f := openRegularStdin(t, header+payload) _, err := f.Seek(int64(len(header)), io.SeekStart) require.NoError(t, err) - got, ok, err := PipeReader(f) + got, ok, err := Piped(f) require.NoError(t, err) require.True(t, ok) @@ -145,13 +177,13 @@ func TestPipeReaderRegularFileAtNonZeroOffset(t *testing.T) { // A file consumed to its end has nothing left to encrypt, so it reports absent // even though the file itself is not empty. -func TestPipeReaderRegularFileAtEOFReportsAbsent(t *testing.T) { +func TestPipedRegularFileAtEOFReportsAbsent(t *testing.T) { const content = "already read\n" f := openRegularStdin(t, content) _, err := f.Seek(int64(len(content)), io.SeekStart) require.NoError(t, err) - _, ok, err := PipeReader(f) + _, ok, err := Piped(f) require.NoError(t, err) assert.False(t, ok) } @@ -159,12 +191,12 @@ func TestPipeReaderRegularFileAtEOFReportsAbsent(t *testing.T) { // A procfs file is a readable regular file that stats as zero bytes, so the // stat cannot be the one to say whether a payload is there. Deciding on it alone // turned `otdfctl encrypt < /proc/cpuinfo` into "no input". -func TestPipeReaderZeroSizedRegularFileWithContent(t *testing.T) { +func TestPipedZeroSizedRegularFileWithContent(t *testing.T) { f, err := os.Open(zeroSizedFileWithContent(t)) require.NoError(t, err) defer f.Close() - got, ok, err := PipeReader(f) + got, ok, err := Piped(f) require.NoError(t, err) require.True(t, ok, "a file that stats as empty may still have content") @@ -197,7 +229,7 @@ func TestOpenFileStaysMeasurable(t *testing.T) { path := filepath.Join(t.TempDir(), "payload.txt") require.NoError(t, os.WriteFile(path, []byte(content), 0o600)) - in, cleanup, err := OpenFile(path) + in, cleanup, err := openFile(path) require.NoError(t, err) defer cleanup() @@ -215,7 +247,7 @@ func TestOpenFileDoesNotMeasureAZeroSizedFile(t *testing.T) { path := filepath.Join(t.TempDir(), "empty.txt") require.NoError(t, os.WriteFile(path, nil, 0o600)) - in, cleanup, err := OpenFile(path) + in, cleanup, err := openFile(path) require.NoError(t, err) defer cleanup() @@ -226,7 +258,7 @@ func TestOpenFileDoesNotMeasureAZeroSizedFile(t *testing.T) { // content. Both halves matter -- io.ReadAll drains the file either way, and it is // the SDK's io.LimitReader, sized from the measurement, that would stop at zero. func TestOpenFileReadsAZeroSizedFileWhole(t *testing.T) { - in, cleanup, err := OpenFile(zeroSizedFileWithContent(t)) + in, cleanup, err := openFile(zeroSizedFileWithContent(t)) require.NoError(t, err) defer cleanup() @@ -238,7 +270,7 @@ func TestOpenFileReadsAZeroSizedFileWhole(t *testing.T) { } func TestOpenFileReportsAMissingFile(t *testing.T) { - _, cleanup, err := OpenFile(filepath.Join(t.TempDir(), "absent.txt")) + _, cleanup, err := openFile(filepath.Join(t.TempDir(), "absent.txt")) require.ErrorIs(t, err, os.ErrNotExist) // cleanup is non-nil even here, so a caller may defer it without a nil guard. @@ -246,6 +278,88 @@ func TestOpenFileReportsAMissingFile(t *testing.T) { cleanup() } +// Open and OpenExclusive part company only over being handed both sources at +// once, so every other case is asserted of both. +func TestResolveInput(t *testing.T) { + for _, resolver := range []struct { + name string + open func(string) (io.Reader, func(), error) + }{ + {name: "Open", open: Open}, + {name: "OpenExclusive", open: OpenExclusive}, + } { + t.Run(resolver.name, func(t *testing.T) { + for _, tc := range []struct { + name, inFile, onStdin string + wantMeasurable bool + wantErr error + }{ + // A file argument reaches the SDK measurable. A pipe must not: + // encrypting one would then need a TMPDIR to spool it. + {name: "file argument", inFile: "the payload\n", wantMeasurable: true}, + {name: "stdin", onStdin: "piped payload\n"}, + {name: "neither", wantErr: ErrNoInput}, + } { + t.Run(tc.name, func(t *testing.T) { + stdinPipe(t, tc.onStdin) + path := "" + if tc.inFile != "" { + path = tempFile(t, tc.inFile) + } + + in, cleanup, err := resolver.open(path) + // cleanup is non-nil even on the error return, so a caller + // may defer it without a nil guard. + require.NotNil(t, cleanup) + defer cleanup() + + if tc.wantErr != nil { + require.ErrorIs(t, err, tc.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tc.wantMeasurable, Measurable(in)) + + got, err := io.ReadAll(in) + require.NoError(t, err) + assert.Equal(t, tc.inFile+tc.onStdin, string(got)) // exactly one is set + }) + } + }) + } +} + +// Given both, Open takes the file argument and leaves stdin not just unused but +// unread: `while read f; do otdfctl … "$f"; done < list` loses a line for every +// byte peeked out from under the loop. +func TestOpenPrefersTheFileArgument(t *testing.T) { + const onStdin, inFile = "the loop's list\n", "the payload\n" + stdinPipe(t, onStdin) + + in, cleanup, err := Open(tempFile(t, inFile)) + require.NoError(t, err) + defer cleanup() + + got, err := io.ReadAll(in) + require.NoError(t, err) + assert.Equal(t, inFile, string(got)) + + rest, err := io.ReadAll(os.Stdin) + require.NoError(t, err) + assert.Equal(t, onStdin, string(rest), "stdin must survive a call that never wanted it") +} + +// OpenExclusive refuses to pick, rather than silently dropping one of the two +// payloads it was handed. +func TestOpenExclusiveRejectsTwoInputs(t *testing.T) { + stdinPipe(t, "a piped payload\n") + + _, cleanup, err := OpenExclusive(tempFile(t, "a file payload\n")) + require.ErrorIs(t, err, ErrTwoInputs) + require.NotNil(t, cleanup) + cleanup() +} + func TestSpoolIsSeekableAndComplete(t *testing.T) { // Larger than any plausible internal buffer, so a truncating copy shows up. content := strings.Repeat("xyz", PipeBufferSize) @@ -339,18 +453,8 @@ func TestOpenSeekableSpoolsNonSeekableNamedFile(t *testing.T) { } func TestOpenSeekableReadsFromStdinPipe(t *testing.T) { - r, w, err := os.Pipe() - require.NoError(t, err) - - origStdin := os.Stdin - os.Stdin = r - defer func() { os.Stdin = origStdin }() - - content := "piped stdin, spooled to disk" - go func() { - defer w.Close() - _, _ = io.WriteString(w, content) - }() + const content = "piped stdin, spooled to disk" + stdinPipe(t, content) in, cleanup, err := OpenSeekable("") require.NoError(t, err) @@ -368,9 +472,7 @@ func TestOpenSeekableReadsFromStdinPipe(t *testing.T) { func TestOpenSeekableDoesNotSpoolARegularFileStdin(t *testing.T) { const content = "redirected stdin, read in place\n" - origStdin := os.Stdin - os.Stdin = openRegularStdin(t, content) - defer func() { os.Stdin = origStdin }() + useStdin(t, openRegularStdin(t, content)) in, cleanup, err := OpenSeekable("") require.NoError(t, err) @@ -389,14 +491,8 @@ func TestOpenSeekableDoesNotSpoolARegularFileStdin(t *testing.T) { } func TestOpenSeekableReturnsErrNoInputForEmptyStdinPipe(t *testing.T) { - r, w, err := os.Pipe() - require.NoError(t, err) - require.NoError(t, w.Close()) - - origStdin := os.Stdin - os.Stdin = r - defer func() { os.Stdin = origStdin }() + stdinPipe(t, "") - _, _, err = OpenSeekable("") + _, _, err := OpenSeekable("") require.ErrorIs(t, err, ErrNoInput) }