diff --git a/otdfctl/cmd/tdf/decrypt.go b/otdfctl/cmd/tdf/decrypt.go index f6cd7beded..397d5999e6 100644 --- a/otdfctl/cmd/tdf/decrypt.go +++ b/otdfctl/cmd/tdf/decrypt.go @@ -2,14 +2,15 @@ package tdf import ( "errors" - "fmt" + "io" "os" "github.com/opentdf/platform/lib/ocrypto" "github.com/opentdf/platform/otdfctl/cmd/common" "github.com/opentdf/platform/otdfctl/pkg/cli" + "github.com/opentdf/platform/otdfctl/pkg/handlers" "github.com/opentdf/platform/otdfctl/pkg/man" - "github.com/opentdf/platform/otdfctl/pkg/utils" + "github.com/opentdf/platform/otdfctl/pkg/streamio" "github.com/spf13/cobra" ) @@ -43,55 +44,61 @@ func decryptRun(cmd *cobra.Command, args []string) { sessionKeyAlgorithm = ocrypto.RSA2048Key } - // check for piped input - piped := readPipedStdin() - - // Prefer file argument over piped input over default filename - bytesToDecrypt := piped + // Prefer the file argument over piped input. var tdfFile string - var err error if len(args) > 0 { tdfFile = args[0] - bytesToDecrypt, err = utils.ReadBytesFromFile(tdfFile, MaxFileSize) + } + in, closeIn, err := streamio.OpenSeekable(tdfFile) + switch { + case errors.Is(err, streamio.ErrNoInput): + cli.ExitWithError("Must provide ONE of the following to decrypt: [file argument, stdin input]", err) + case err != nil: + cli.ExitWithError("Failed to read file:", err) + } + defer closeIn() + + // Resolve the destination before decrypting, so the plaintext streams + // straight to it rather than accumulating in memory first. + var dest io.Writer = os.Stdout + var outFile *streamio.OutputFile + if output != "" { + outFile, err = streamio.NewOutputFile(output) if err != nil { - cli.ExitWithError("Failed to read file:", err) + closeIn() + cli.ExitWithError("Failed to write decrypted data to file", err) } + defer outFile.Cleanup() + dest = outFile } - if len(bytesToDecrypt) == 0 { - cli.ExitWithError("Must provide ONE of the following to decrypt: [file argument, stdin input]", errors.New("no input provided")) + // cli.ExitWithError calls os.Exit, which skips deferred functions, so both + // the spooled input and the partial output have to be discarded first. + fail := func(msg string, err error) { + closeIn() + if outFile != nil { + outFile.Cleanup() + } + cli.ExitWithError(msg, err) } ignoreAllowlist := len(kasAllowList) == 1 && kasAllowList[0] == "*" - decrypted, err := h.DecryptBytes( - c.Context(), - bytesToDecrypt, - assertionVerification, - disableAssertionVerification, - sessionKeyAlgorithm, - kasAllowList, - ignoreAllowlist, - nil, - ) + err = h.Decrypt(c.Context(), dest, in, handlers.DecryptOptions{ + AssertionVerificationKeysFile: assertionVerification, + DisableAssertionCheck: disableAssertionVerification, + SessionKeyAlgorithm: sessionKeyAlgorithm, + KASAllowList: kasAllowList, + IgnoreAllowlist: ignoreAllowlist, + }) if err != nil { - cli.ExitWithError("Failed to decrypt file", err) + fail("Failed to decrypt file", err) } - if output == "" { - //nolint:forbidigo // printing decrypted content to stdout - fmt.Print(decrypted.String()) - return - } - // Here 'output' is the filename given with -o - f, err := os.Create(output) - if err != nil { - cli.ExitWithError("Failed to write decrypted data to file", err) - } - defer f.Close() - _, err = f.Write(decrypted.Bytes()) - if err != nil { - cli.ExitWithError("Failed to write decrypted data to file", err) + if outFile != nil { + if err := outFile.Commit(); err != nil { + fail("Failed to write decrypted data to file", err) + } } } diff --git a/otdfctl/cmd/tdf/encrypt.go b/otdfctl/cmd/tdf/encrypt.go index 3935825f28..9358338a76 100644 --- a/otdfctl/cmd/tdf/encrypt.go +++ b/otdfctl/cmd/tdf/encrypt.go @@ -1,8 +1,10 @@ package tdf import ( + "errors" "io" "log/slog" + "mime" "os" "path/filepath" "strings" @@ -11,8 +13,9 @@ import ( "github.com/opentdf/platform/lib/ocrypto" "github.com/opentdf/platform/otdfctl/cmd/common" "github.com/opentdf/platform/otdfctl/pkg/cli" + "github.com/opentdf/platform/otdfctl/pkg/handlers" "github.com/opentdf/platform/otdfctl/pkg/man" - "github.com/opentdf/platform/otdfctl/pkg/utils" + "github.com/opentdf/platform/otdfctl/pkg/streamio" "github.com/spf13/cobra" ) @@ -24,6 +27,39 @@ var ( EncryptCmd = &encryptDoc.Command ) +// detectMimeType sniffs the payload's type from its head and rewinds, so the +// whole payload still reaches the encoder. +// +// 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) { + 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 + } + if _, err := in.Seek(0, io.SeekStart); err != nil { + return "", err + } + + // defaults to application/octet-stream if nothing is recognized + detected := mimetype.Detect(head[:n]).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 + // and dereferencing that panicked — which is what happened for any file + // whose contents were unrecognizable and whose name had an extension. + // An extension with no known type leaves octet-stream in place. + if byExt := mime.TypeByExtension("." + fileExt); byExt != "" { + detected = byExt + } + } + return detected, nil +} + func encryptRun(cmd *cobra.Command, args []string) { c := cli.New(cmd, args, cli.WithPrintJSON()) h := common.NewHandler(c) @@ -57,13 +93,16 @@ func encryptRun(cmd *cobra.Command, args []string) { wrappingKeyAlgorithm = ocrypto.RSA2048Key } - piped := readPipedStdin() + 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 len(piped) > 0 { + if hasPiped { inputCount++ } @@ -76,71 +115,84 @@ func encryptRun(cmd *cobra.Command, args []string) { cliExit("ONLY ONE") } - // prefer filepath argument over stdin input - bytesSlice := piped - var err error + // 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() if filePath != "" { - bytesSlice, err = utils.ReadBytesFromFile(filePath, MaxFileSize) + f, err := os.Open(filePath) if err != nil { cli.ExitWithError("Failed to read file:", err) } - } - - // auto-detect mime type if not provided - if fileMimeType == "" { - slog.Debug("detecting mime type of file") - // get the mime type of the file - mimetype.SetLimit(Size1MB) // limit to 1MB - m := mimetype.Detect(bytesSlice) - // default to application/octet-stream if no mime type is detected - fileMimeType = m.String() - - if fileMimeType == "application/octet-stream" { - if fileExt != "" { - fileMimeType = mimetype.Lookup(fileExt).String() - } + 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 } - slog.Debug("encrypting file", - slog.Int("file_len", len(bytesSlice)), - slog.String("mime_type", fileMimeType), - ) - - // Do the encryption - encrypted, err := h.EncryptBytes( - tdfType, - bytesSlice, - attrValues, - fileMimeType, - kasURLPath, - assertions, - wrappingKeyAlgorithm, - targetMode, - ) - if err != nil { - cli.ExitWithError("Failed to encrypt", err) - } - - // Find the destination as the output flag filename or stdout - var dest *os.File + // cli.ExitWithError calls os.Exit, which skips deferred functions, so every + // exit below goes through fail() to discard the spool and any partial output. + defer cleanup() + + // 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") { out += ".tdf" } - tdfFile, err := os.Create(out) + tdfFile, err = streamio.NewOutputFile(out) if err != nil { + cleanup() cli.ExitWithError("Failed to write encrypted file "+out, err) } - defer tdfFile.Close() + defer tdfFile.Cleanup() dest = tdfFile } else { dest = os.Stdout } - _, e := io.Copy(dest, encrypted) - if e != nil { - cli.ExitWithError("Failed to write encrypted data to stdout", e) + 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) + if err != nil { + fail("Failed to read file:", err) + } + } + slog.Debug("encrypting file", slog.String("mime_type", fileMimeType)) + + // Do the encryption + err = h.Encrypt(c.Context(), dest, in, handlers.EncryptOptions{ + TDFType: tdfType, + Attributes: attrValues, + MimeType: fileMimeType, + KASURLPath: kasURLPath, + Assertions: assertions, + WrappingKeyAlgorithm: wrappingKeyAlgorithm, + TargetMode: targetMode, + }) + if err != nil { + fail("Failed to encrypt", err) + } + + if tdfFile != nil { + if err := tdfFile.Commit(); err != nil { + fail("Failed to write encrypted file "+out, err) + } } } diff --git a/otdfctl/cmd/tdf/encrypt_test.go b/otdfctl/cmd/tdf/encrypt_test.go new file mode 100644 index 0000000000..8c22bea7ed --- /dev/null +++ b/otdfctl/cmd/tdf/encrypt_test.go @@ -0,0 +1,67 @@ +package tdf + +import ( + "bytes" + "io" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDetectMimeTypeRewindsForTheEncoder(t *testing.T) { + for _, tc := range []struct { + name string + content string + want string + }{ + {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. + {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) + + got, 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)) + }) + } +} + +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") + require.NoError(t, err) + assert.Equal(t, "application/pdf", got) +} + +func TestDetectMimeTypeUnknownExtensionStaysOctetStream(t *testing.T) { + unrecognized := bytes.Repeat([]byte{0x01, 0x02, 0x03, 0x04}, 8) + + // 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") + require.NoError(t, err) + assert.Equal(t, "application/octet-stream", got) +} + +func TestDetectMimeTypeEmptyPayload(t *testing.T) { + in := strings.NewReader("") + + 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 a35a3e0d04..7b8fd351de 100644 --- a/otdfctl/cmd/tdf/inspect.go +++ b/otdfctl/cmd/tdf/inspect.go @@ -2,11 +2,13 @@ package tdf import ( "errors" + "log/slog" "github.com/opentdf/platform/otdfctl/cmd/common" "github.com/opentdf/platform/otdfctl/pkg/cli" "github.com/opentdf/platform/otdfctl/pkg/handlers" "github.com/opentdf/platform/otdfctl/pkg/man" + "github.com/opentdf/platform/otdfctl/pkg/streamio" "github.com/opentdf/platform/sdk" "github.com/spf13/cobra" ) @@ -42,17 +44,36 @@ func inspectRun(cmd *cobra.Command, args []string) { h := common.NewHandler(c) defer h.Close() - data := cli.ReadFromArgsOrPipe(args, nil) - if len(data) == 0 { - c.ExitWithError("must provide ONE of the following: [file argument, stdin input]", errors.New("no input provided")) + var path string + if len(args) > 0 { + path = args[0] } + in, cleanup, err := streamio.OpenSeekable(path) + if err != nil { + if errors.Is(err, streamio.ErrNoInput) { + c.ExitWithError("must provide ONE of the following: [file argument, stdin input]", err) + } + c.ExitWithError("failed to read input", err) + } + // 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. + defer cleanup() - result, errs := h.InspectTDF(data) + result, errs := h.InspectTDF(in) for _, err := range errs { - if errors.Is(err, handlers.ErrTDFInspectFailNotValidTDF) { + switch { + case errors.Is(err, handlers.ErrTDFInspectFailNotValidTDF): + cleanup() c.ExitWithError("not a valid TDF", err) - } else if errors.Is(err, handlers.ErrTDFInspectFailNotInspectable) { + case errors.Is(err, handlers.ErrTDFInspectFailNotInspectable): + cleanup() c.ExitWithError("failed to inspect TDF", err) + default: + // Attribute/metadata reads (e.g. a denied or unreachable KAS rewrap) + // are non-fatal by design so the manifest still prints, but must not + // vanish silently. + slog.Warn("inspect: partial result", slog.Any("error", err)) } } @@ -76,8 +97,10 @@ func inspectRun(cmd *cobra.Command, args []string) { Attributes: result.Attributes, } + cleanup() c.ExitWithJSON(m, cli.ExitCodeSuccess) } + cleanup() c.ExitWithError("failed to inspect TDF", nil) } diff --git a/otdfctl/cmd/tdf/tdf.go b/otdfctl/cmd/tdf/tdf.go index 7f727af92b..be6a89336e 100644 --- a/otdfctl/cmd/tdf/tdf.go +++ b/otdfctl/cmd/tdf/tdf.go @@ -1,31 +1,8 @@ package tdf -import ( - "io" - "os" - - "github.com/opentdf/platform/otdfctl/pkg/cli" -) - const ( - Size1MB = 1024 * 1024 - MaxFileSize = int64(10 * 1024 * 1024 * 1024) // 10 GB - TDF = "TDF" + Size1MB = 1024 * 1024 + TDF = "TDF" // GroupID is the group ID for TDF commands GroupID = TDF ) - -func readPipedStdin() []byte { - stat, err := os.Stdin.Stat() - if err != nil { - cli.ExitWithError("Failed to read stat from stdin", err) - } - if (stat.Mode() & os.ModeCharDevice) == 0 { - buf, err := io.ReadAll(os.Stdin) - if err != nil { - cli.ExitWithError("failed to scan bytes from stdin", err) - } - return buf - } - return nil -} diff --git a/otdfctl/e2e/action.yaml b/otdfctl/e2e/action.yaml index b95a21027c..efda6a561a 100644 --- a/otdfctl/e2e/action.yaml +++ b/otdfctl/e2e/action.yaml @@ -54,12 +54,22 @@ runs: # suite while other files still create unnamespaced policy fixtures. bats --tap e2e --filter-tags namespaced_policy_migration | tee e2e/bats-results.tap + # Then the streaming suite, also on its own. It round-trips with no + # attributes, which falls back to the platform base key, and + # key-base.bats sets one pointing at a KAS that does not resolve and + # cannot unset it afterwards -- a base key can be replaced but not + # cleared. Anything unattributed scheduled after that file produces an + # undecryptable TDF, so this has to run first rather than race for a + # slot. Running alone also keeps the 1 GiB peak-RSS case from measuring + # itself against three neighbours competing for the same memory. + bats --tap e2e --filter-tags payload_streaming | tee -a e2e/bats-results.tap + if command -v parallel >/dev/null 2>&1; then echo "GNU parallel found, running remaining tests in parallel" - bats --tap e2e --filter-tags '!namespaced_policy_migration' --jobs 4 --no-parallelize-within-files --no-tempdir-cleanup | tee -a e2e/bats-results.tap + bats --tap e2e --filter-tags '!namespaced_policy_migration,!payload_streaming' --jobs 4 --no-parallelize-within-files --no-tempdir-cleanup | tee -a e2e/bats-results.tap else echo "GNU parallel not found, running remaining tests sequentially" - bats --tap e2e --filter-tags '!namespaced_policy_migration' | tee -a e2e/bats-results.tap + bats --tap e2e --filter-tags '!namespaced_policy_migration,!payload_streaming' | tee -a e2e/bats-results.tap fi env: # Define 'bats' install location in ubuntu diff --git a/otdfctl/e2e/streaming.bats b/otdfctl/e2e/streaming.bats new file mode 100755 index 0000000000..2fe45f7df7 --- /dev/null +++ b/otdfctl/e2e/streaming.bats @@ -0,0 +1,160 @@ +#!/usr/bin/env bats + +# bats file_tags=payload_streaming + +# Streaming encrypt/decrypt/inspect (DSPX-4499). +# +# These live outside encrypt-decrypt.bats deliberately. That file carries a +# file-level skip pending the namespaced-subject-mappings migration, so anything +# added to it would not run. None of the cases here need an entitlement: the +# round-trips encrypt with no attributes, and the two failure cases are forced +# with an unresolvable attribute FQN and a KAS allowlist that excludes the +# platform, neither of which requires policy fixtures. +# +# The payload_streaming tag exists so action.yaml can run this file before the +# parallel batch, and it is load-bearing. Encrypting with no attributes falls +# back to the platform base key, and key-base.bats sets one pointing at +# https://test-kas-for-base-keys.com, which does not resolve. It cannot put +# things back: a base key can be replaced but not cleared, so its teardown +# leaves the platform unable to decrypt anything unattributed for the rest of +# the run. encrypt-decrypt.bats would hit the same wall today if it were not +# skipped. Running before key-base.bats is a workaround, not a fix -- the leak +# is worth closing on its own. + +setup_file() { + export HOST=http://localhost:8080 + export CREDSFILE=creds.json + echo -n '{"clientId":"opentdf","clientSecret":"secret"}' >"$CREDSFILE" + export WITH_CREDS="--with-client-creds-file $CREDSFILE" + export COMMON="--host $HOST --tls-no-verify $WITH_CREDS" + + export SECRET_TEXT="my special streaming secret" +} + +setup() { + bats_load_library bats-support + bats_load_library bats-assert + + PLAIN="$BATS_TEST_TMPDIR/payload.txt" + TDF_OUT="$BATS_TEST_TMPDIR/payload.txt.tdf" + RESULT="$BATS_TEST_TMPDIR/payload.out" + printf '%s\n' "$SECRET_TEXT" >"$PLAIN" +} + +# Baseline: both ends are seekable files, so nothing is spooled. +@test "roundtrip TDF3, no attributes, file to file" { + ./otdfctl encrypt -o "$TDF_OUT" $COMMON "$PLAIN" + ./otdfctl decrypt -o "$RESULT" $COMMON "$TDF_OUT" + diff "$PLAIN" "$RESULT" +} + +@test "roundtrip TDF3, no attributes, file to stdout" { + ./otdfctl encrypt $COMMON "$PLAIN" >"$TDF_OUT" + ./otdfctl decrypt -o "$RESULT" $COMMON "$TDF_OUT" + diff "$PLAIN" "$RESULT" +} + +# The fully piped form is the one documented in docs/man/encrypt/_index.md, and +# the one with no seekable input on either end. +@test "roundtrip TDF3, stdin to stdout, fully piped" { + run bash -c "echo '$SECRET_TEXT' | ./otdfctl encrypt $COMMON | ./otdfctl decrypt $COMMON" + assert_success + 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" { + ./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. + TMPDIR="$BATS_TEST_TMPDIR" ./otdfctl decrypt $COMMON <"$TDF_OUT" >"$RESULT" + diff "$PLAIN" "$RESULT" + + run bash -c "ls $BATS_TEST_TMPDIR/otdfctl-spool-* 2>/dev/null | wc -l" + assert_output "0" +} + +@test "inspect reads a TDF from a file and from stdin" { + ./otdfctl encrypt -o "$TDF_OUT" $COMMON "$PLAIN" + + run bash -c "./otdfctl inspect $COMMON '$TDF_OUT' | jq -r '.manifest.protocol'" + assert_success + assert_output "zip" + + run bash -c "TMPDIR='$BATS_TEST_TMPDIR' ./otdfctl inspect $COMMON < '$TDF_OUT' | jq -r '.manifest.protocol'" + assert_success + assert_output "zip" + + # inspect spools piped input too, and exits via os.Exit on the success path. + run bash -c "ls $BATS_TEST_TMPDIR/otdfctl-spool-* 2>/dev/null | wc -l" + assert_output "0" +} + +# An empty redirect is 'no input', not 'a zero-byte payload'. Presence is +# detected with a peek rather than a read, so this must stay an error. +@test "encrypt rejects empty stdin" { + run bash -c "./otdfctl encrypt $COMMON < /dev/null" + assert_failure +} + +@test "decrypt rejects empty stdin" { + run bash -c "./otdfctl decrypt $COMMON < /dev/null" + assert_failure +} + +# Output goes to a temp sibling and is renamed only on success, so a failed run +# must leave neither a partial .tdf nor the temp file behind. +@test "encrypt leaves no output behind when it fails" { + run bash -c "echo '$SECRET_TEXT' | ./otdfctl encrypt -o '$TDF_OUT' $COMMON -a 'https://streaming-does-not-exist.io/attr/nope/value/nope'" + assert_failure + [ ! -f "$TDF_OUT" ] + + run bash -c "ls $BATS_TEST_TMPDIR/.payload.txt.tdf.tmp-* 2>/dev/null | wc -l" + assert_output "0" +} + +@test "decrypt leaves no output behind when it fails" { + ./otdfctl encrypt -o "$TDF_OUT" $COMMON "$PLAIN" + + # An allowlist with no entry for the platform KAS fails the rewrap. + run ./otdfctl decrypt -o "$RESULT" $COMMON --kas-allowlist "https://nowhere.example.com" "$TDF_OUT" + assert_failure + [ ! -f "$RESULT" ] + + run bash -c "ls $BATS_TEST_TMPDIR/.payload.out.tmp-* 2>/dev/null | wc -l" + assert_output "0" +} + +# 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" { + GNU_TIME=$(command -v gtime || command -v /usr/bin/time) + if [ -z "$GNU_TIME" ] || ! $GNU_TIME -v true 2>&1 | grep -q "Maximum resident set size"; then + skip "GNU time not available" + fi + + local big="$BATS_TEST_TMPDIR/big.bin" + local bigtdf="$BATS_TEST_TMPDIR/big.bin.tdf" + local bigout="$BATS_TEST_TMPDIR/big.out" + local enclog="$BATS_TEST_TMPDIR/enc.log" + local declog="$BATS_TEST_TMPDIR/dec.log" + + # 1 GiB. The buffered implementation peaked around 3.6x this for both commands. + dd if=/dev/zero of="$big" bs=1048576 count=1024 status=none + + $GNU_TIME -v -o "$enclog" ./otdfctl encrypt -o "$bigtdf" $COMMON "$big" + $GNU_TIME -v -o "$declog" ./otdfctl decrypt -o "$bigout" $COMMON "$bigtdf" + cmp "$big" "$bigout" + + local enc_kb dec_kb + enc_kb=$(grep "Maximum resident set size" "$enclog" | grep -o '[0-9]*') + dec_kb=$(grep "Maximum resident set size" "$declog" | grep -o '[0-9]*') + rm -f "$big" "$bigtdf" "$bigout" + + echo "peak RSS: encrypt ${enc_kb} KB, decrypt ${dec_kb} KB" + # 512 MiB leaves generous headroom over the ~66 MiB a 1 MiB payload used, while + # still failing loudly on any return to whole-payload buffering. + [ "$enc_kb" -lt 524288 ] + [ "$dec_kb" -lt 524288 ] +} diff --git a/otdfctl/pkg/cli/pipe.go b/otdfctl/pkg/cli/pipe.go index 561c8ff19f..8d0472960d 100644 --- a/otdfctl/pkg/cli/pipe.go +++ b/otdfctl/pkg/cli/pipe.go @@ -3,8 +3,22 @@ package cli import ( "io" "os" + + "github.com/opentdf/platform/otdfctl/pkg/streamio" ) +// These wrappers read their whole input into memory and call ExitWithError — +// which calls os.Exit — from inside the read, so they cannot be used anywhere +// that wants to handle the failure itself. They delegate to pkg/streamio so +// there is one implementation of "find the file argument or piped stdin" to +// maintain. +// +// New code should call pkg/streamio directly, which streams and returns +// errors instead of buffering and exiting. + +// Deprecated: reads the entire input into memory and terminates the process on +// failure. Use streamio.OpenSeekable, which resolves the same "file argument or +// piped stdin" choice without buffering and returns an error. func ReadFromArgsOrPipe(args []string, pipe *os.File) []byte { if len(args) > 0 { return ReadFromFile(args[0]) @@ -15,31 +29,38 @@ func ReadFromArgsOrPipe(args []string, pipe *os.File) []byte { return ReadFromPipe(pipe) } +// 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. func ReadFromPipe(in *os.File) []byte { - stat, err := in.Stat() + r, ok, err := streamio.PipeReader(in) if err != nil { ExitWithError("failed to read stat from stdin", err) } - if (stat.Mode() & os.ModeCharDevice) == 0 { - buf, err := io.ReadAll(in) - if err != nil { - ExitWithError("failed to scan bytes from stdin", err) - } - return buf + if !ok { + return nil + } + buf, err := io.ReadAll(r) + if err != nil { + ExitWithError("failed to scan bytes from stdin", err) } - return nil + return buf } +// Deprecated: reads the entire file into memory with no size cap at all — not +// even the 10 GB one the tdf commands apply — and terminates the process on +// failure. Open the file and stream from it, or use utils.ReadBytesFromFile if +// a bounded in-memory read is genuinely wanted. func ReadFromFile(filePath string) []byte { - fileToEncrypt, err := os.Open(filePath) + f, err := os.Open(filePath) if err != nil { - ExitWithError("Failed to git open file at path: "+filePath, err) + ExitWithError("Failed to open file at path: "+filePath, err) } - defer fileToEncrypt.Close() + defer f.Close() - bytes, err := io.ReadAll(fileToEncrypt) + buf, err := io.ReadAll(f) if err != nil { ExitWithError("Failed to read bytes from file at path: "+filePath, err) } - return bytes + return buf } diff --git a/otdfctl/pkg/handlers/tdf.go b/otdfctl/pkg/handlers/tdf.go index d8a5b0d69f..b5e5c1a237 100644 --- a/otdfctl/pkg/handlers/tdf.go +++ b/otdfctl/pkg/handlers/tdf.go @@ -1,7 +1,6 @@ package handlers import ( - "bytes" "context" "crypto/rsa" "crypto/x509" @@ -38,51 +37,57 @@ type TDFInspect struct { UnencryptedMetadata []byte } -func (h Handler) EncryptBytes( - tdfType string, - unencrypted []byte, - attrValues []string, - mimeType string, - kasURLPath string, - assertions string, - wrappingKeyAlgorithm ocrypto.KeyType, - targetMode string, -) (*bytes.Buffer, error) { - var encrypted []byte - enc := bytes.NewBuffer(encrypted) +// EncryptOptions carries the non-stream inputs to Encrypt. +type EncryptOptions struct { + TDFType string + Attributes []string + MimeType string + KASURLPath string + Assertions string + WrappingKeyAlgorithm ocrypto.KeyType + TargetMode string +} - switch tdfType { +// Encrypt streams the plaintext from in to a TDF written to out. Memory use is +// 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 { + switch o.TDFType { // Encrypt the data as a ZTDF case "", tdf.TypeTDF3, tdf.TypeZTDF: opts := []sdk.TDFOption{ - sdk.WithDataAttributes(attrValues...), + sdk.WithDataAttributes(o.Attributes...), sdk.WithKasInformation(sdk.KASInfo{ - URL: h.platformEndpoint + kasURLPath, + URL: h.platformEndpoint + o.KASURLPath, }), - sdk.WithMimeType(mimeType), - sdk.WithWrappingKeyAlg(wrappingKeyAlgorithm), //nolint:staticcheck // SDK option is deprecated but no replacement is available in this SDK version. + sdk.WithMimeType(o.MimeType), + sdk.WithWrappingKeyAlg(o.WrappingKeyAlgorithm), //nolint:staticcheck // SDK option is deprecated but no replacement is available in this SDK version. } var assertionConfigs []sdk.AssertionConfig //nolint:nestif // nested its mainly for error catching and handling case of string vs file - if assertions != "" { - err := json.Unmarshal([]byte(assertions), &assertionConfigs) + if o.Assertions != "" { + err := json.Unmarshal([]byte(o.Assertions), &assertionConfigs) if err != nil { // if unable to marshal to json, interpret as file string and try to read from file - assertionBytes, err := utils.ReadBytesFromFile(assertions, MaxAssertionsFileSize) + assertionBytes, err := utils.ReadBytesFromFile(o.Assertions, MaxAssertionsFileSize) if err != nil { - return nil, fmt.Errorf("unable to read assertions file: %w", err) + return fmt.Errorf("unable to read assertions file: %w", err) } err = json.Unmarshal(assertionBytes, &assertionConfigs) if err != nil { - return nil, fmt.Errorf("unable to unmarshal assertions json: %w", err) + return fmt.Errorf("unable to unmarshal assertions json: %w", err) } } for i, config := range assertionConfigs { if !config.SigningKey.IsEmpty() { correctedKey, err := correctKeyType(config.SigningKey, false) if err != nil { - return nil, fmt.Errorf("error with assertion signing key: %w", err) + return fmt.Errorf("error with assertion signing key: %w", err) } assertionConfigs[i].SigningKey.Key = correctedKey } @@ -90,85 +95,102 @@ func (h Handler) EncryptBytes( opts = append(opts, sdk.WithAssertions(assertionConfigs...)) } - if targetMode != "" { - opts = append(opts, sdk.WithTargetMode(targetMode)) + if o.TargetMode != "" { + opts = append(opts, sdk.WithTargetMode(o.TargetMode)) } - _, err := h.sdk.CreateTDF(enc, bytes.NewReader(unencrypted), opts...) - return enc, err + _, err := h.sdk.CreateTDFContext(ctx, out, in, opts...) + return err default: - return nil, errors.New("unknown TDF type") + return errors.New("unknown TDF type") } } -func (h Handler) DecryptBytes( - ctx context.Context, - toDecrypt []byte, - assertionVerificationKeysFile string, - disableAssertionCheck bool, - sessionKeyAlgorithm ocrypto.KeyType, - kasAllowList []string, - ignoreAllowlist bool, - fulfillableObligations []string, -) (*bytes.Buffer, error) { - out := &bytes.Buffer{} - pt := io.Writer(out) - ec := bytes.NewReader(toDecrypt) - switch sdk.GetTdfType(ec) { +// DecryptOptions carries the non-stream inputs to Decrypt. +type DecryptOptions struct { + AssertionVerificationKeysFile string + DisableAssertionCheck bool + SessionKeyAlgorithm ocrypto.KeyType + KASAllowList []string + IgnoreAllowlist bool + FulfillableObligations []string +} + +// streamingCopy asserts that the SDK reader is copied one segment at a time. +// +// io.Copy prefers WriteTo when the source implements it, and sdk.Reader's +// WriteTo decrypts segment by segment. Without it, io.Copy would fall back to +// Read, which serves bytes from an internal buffer grown by ReadAt — putting +// the whole payload back in memory and silently undoing this change with no +// test failure to show for it. +var _ io.WriterTo = (*sdk.Reader)(nil) + +// Decrypt streams the plaintext of the TDF in in to out. Memory use is bounded +// by the SDK's segment size rather than by the payload length. +// +// in must be seekable because the TDF's manifest lives at the end of the +// archive; callers with a pipe need to spool it first. +func (h Handler) Decrypt(ctx context.Context, out io.Writer, in io.ReadSeeker, o DecryptOptions) error { + switch sdk.GetTdfType(in) { case sdk.Standard: opts := []sdk.TDFReaderOption{ - sdk.WithDisableAssertionVerification(disableAssertionCheck), - sdk.WithSessionKeyType(sessionKeyAlgorithm), - sdk.WithIgnoreAllowlist(ignoreAllowlist), - sdk.WithTDFFulfillableObligationFQNs(fulfillableObligations), + sdk.WithDisableAssertionVerification(o.DisableAssertionCheck), + sdk.WithSessionKeyType(o.SessionKeyAlgorithm), + sdk.WithIgnoreAllowlist(o.IgnoreAllowlist), + sdk.WithTDFFulfillableObligationFQNs(o.FulfillableObligations), } - if kasAllowList != nil { - opts = append(opts, sdk.WithKasAllowlist(kasAllowList)) + if o.KASAllowList != nil { + opts = append(opts, sdk.WithKasAllowlist(o.KASAllowList)) } var assertionVerificationKeys sdk.AssertionVerificationKeys - if assertionVerificationKeysFile != "" { + if o.AssertionVerificationKeysFile != "" { // read the file - assertionVerificationBytes, err := utils.ReadBytesFromFile(assertionVerificationKeysFile, MaxAssertionsFileSize) + assertionVerificationBytes, err := utils.ReadBytesFromFile(o.AssertionVerificationKeysFile, MaxAssertionsFileSize) if err != nil { - return nil, fmt.Errorf("unable to read assertions verification keys file: %w", err) + return fmt.Errorf("unable to read assertions verification keys file: %w", err) } err = json.Unmarshal(assertionVerificationBytes, &assertionVerificationKeys) if err != nil { - return nil, fmt.Errorf("unable to unmarshal assertion verification keys json: %w", err) + return fmt.Errorf("unable to unmarshal assertion verification keys json: %w", err) } for assertionName, key := range assertionVerificationKeys.Keys { correctedKey, err := correctKeyType(key, true) if err != nil { - return nil, fmt.Errorf("error with assertion signing key: %w", err) + return fmt.Errorf("error with assertion signing key: %w", err) } assertionVerificationKeys.Keys[assertionName] = sdk.AssertionKey{Alg: key.Alg, Key: correctedKey} } opts = append(opts, sdk.WithAssertionVerificationKeys(assertionVerificationKeys)) } - r, err := h.sdk.LoadTDF(ec, opts...) + r, err := h.sdk.LoadTDF(in, opts...) if err != nil { - return nil, err + return err } //nolint:errorlint // callers intended to test error equality directly - if _, err = io.Copy(pt, r); err != nil && err != io.EOF { - return nil, formatDecryptError(ctx, r.Obligations, err) + if _, err = io.Copy(out, r); err != nil && err != io.EOF { + return formatDecryptError(ctx, r.Obligations, err) } case sdk.Invalid: - return nil, errors.New("invalid TDF") + return errors.New("invalid TDF") default: - return nil, errors.New("unknown TDF type") + return errors.New("unknown TDF type") } - return out, nil + return nil } -func (h Handler) InspectTDF(toInspect []byte) (TDFInspect, []error) { - b := bytes.NewReader(toInspect) - switch sdk.GetTdfType(b) { +// InspectTDF reads the manifest and attributes of a TDF. +// +// It takes an io.ReadSeeker rather than a byte slice because only the manifest +// at the end of the archive is needed; buffering the whole payload to reach it +// costs memory proportional to the file. GetTdfType rewinds to the start, so +// the reader is positioned for LoadTDF. +func (h Handler) InspectTDF(toInspect io.ReadSeeker) (TDFInspect, []error) { + switch sdk.GetTdfType(toInspect) { case sdk.Standard: // grouping errors so we don't impact the piping of the data errs := []error{} - tdfreader, err := h.sdk.LoadTDF(bytes.NewReader(toInspect)) + tdfreader, err := h.sdk.LoadTDF(toInspect) if err != nil { if strings.Contains(err.Error(), "zip: not a valid zip file") { return TDFInspect{}, []error{ErrTDFInspectFailNotInspectable} diff --git a/otdfctl/pkg/streamio/input.go b/otdfctl/pkg/streamio/input.go new file mode 100644 index 0000000000..dc7c29a874 --- /dev/null +++ b/otdfctl/pkg/streamio/input.go @@ -0,0 +1,114 @@ +// Package streamio provides the input and output plumbing shared by commands +// that move payloads too large to hold in memory. +// +// Everything here returns errors rather than terminating the process, so the +// decision to exit stays with the command layer. That is the difference from +// the older helpers in pkg/cli, which call cli.ExitWithError from inside the +// read and are therefore unusable from anywhere that wants to recover. +package streamio + +import ( + "bufio" + "errors" + "io" + "os" +) + +// PipeBufferSize is the window PipeReader 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") + +// PipeReader reports whether in is a pipe or redirect carrying at least one +// byte, and returns a reader over it. +// +// 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) { + stat, err := in.Stat() + if err != nil { + return nil, false, err + } + if (stat.Mode() & os.ModeCharDevice) != 0 { + return nil, false, nil + } + + r := bufio.NewReaderSize(in, PipeBufferSize) + if _, err := r.Peek(1); err != nil { + if errors.Is(err, io.EOF) { + return nil, false, nil + } + return nil, false, err + } + return r, true, nil +} + +// Spool copies r into a temporary file and rewinds it, giving a seekable view +// of a stream that has none. +// +// A TDF's manifest sits at the end of the archive, so any command that needs +// to seek it cannot consume a pipe directly. Spooling trades disk for the +// memory a whole-payload read would use, and needs a TMPDIR with room for the +// full TDF — it fails loudly if there isn't one. +// +// The returned cleanup must run on every path. cli.ExitWithError calls os.Exit +// and skips deferred functions, so deferring it alone is not enough. +func Spool(r io.Reader) (*os.File, func(), error) { + f, err := os.CreateTemp("", "otdfctl-spool-*.tdf") + if err != nil { + return nil, func() {}, err + } + cleanup := func() { + f.Close() + os.Remove(f.Name()) + } + if _, err := io.Copy(f, r); err != nil { + cleanup() + return nil, func() {}, err + } + if _, err := f.Seek(0, io.SeekStart); err != nil { + cleanup() + return nil, func() {}, err + } + return f, cleanup, nil +} + +// 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. +// +// 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) + if err != nil { + return nil, func() {}, err + } + if _, err := f.Seek(0, io.SeekCurrent); err == nil { + return f, func() { f.Close() }, 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 + } + + piped, ok, err := PipeReader(os.Stdin) + if err != nil { + return nil, func() {}, err + } + if !ok { + return nil, func() {}, ErrNoInput + } + return Spool(piped) +} diff --git a/otdfctl/pkg/streamio/input_test.go b/otdfctl/pkg/streamio/input_test.go new file mode 100644 index 0000000000..2c8f8ad3d5 --- /dev/null +++ b/otdfctl/pkg/streamio/input_test.go @@ -0,0 +1,213 @@ +package streamio + +import ( + "io" + "os" + "path/filepath" + "runtime" + "strings" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPipeReader(t *testing.T) { + for _, tc := range []struct { + name string + content string + wantOK bool + }{ + {name: "with data", content: "hello world", wantOK: true}, + {name: "empty pipe reports absent", content: "", wantOK: false}, + } { + t.Run(tc.name, func(t *testing.T) { + r, w, err := os.Pipe() + require.NoError(t, err) + defer r.Close() + + go func() { + defer w.Close() + _, _ = io.WriteString(w, tc.content) + }() + + got, ok, err := PipeReader(r) + require.NoError(t, err) + require.Equal(t, tc.wantOK, ok) + if !tc.wantOK { + return + } + + // Peek must not consume: the whole payload is still readable. + all, err := io.ReadAll(got) + require.NoError(t, err) + assert.Equal(t, tc.content, string(all)) + }) + } +} + +func TestPipeReaderPreservesPayloadLargerThanBuffer(t *testing.T) { + content := strings.Repeat("a", PipeBufferSize*2+7) + + r, w, err := os.Pipe() + require.NoError(t, err) + defer r.Close() + + go func() { + defer w.Close() + _, _ = io.WriteString(w, content) + }() + + got, ok, err := PipeReader(r) + require.NoError(t, err) + require.True(t, ok) + + all, err := io.ReadAll(got) + require.NoError(t, err) + assert.Len(t, all, len(content)) +} + +func TestPipeReaderOnTerminalReportsAbsent(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() + require.NoError(t, err) + if (stat.Mode() & os.ModeCharDevice) == 0 { + t.Skip("stdin is not a terminal under this test runner") + } + _, ok, err := PipeReader(os.Stdin) + require.NoError(t, err) + assert.False(t, ok) +} + +func TestSpoolIsSeekableAndComplete(t *testing.T) { + // Larger than any plausible internal buffer, so a truncating copy shows up. + content := strings.Repeat("xyz", PipeBufferSize) + + f, cleanup, err := Spool(strings.NewReader(content)) + require.NoError(t, err) + defer cleanup() + + // The spool must be positioned at the head, not at the end of the copy. + pos, err := f.Seek(0, io.SeekCurrent) + require.NoError(t, err) + assert.Equal(t, int64(0), pos, "spool must be rewound for the caller") + + got, err := io.ReadAll(f) + require.NoError(t, err) + assert.Len(t, got, len(content)) + + // Seeking backwards is the whole reason for spooling: a TDF's manifest is at + // the end of the archive, so the reader has to be able to go back. + _, err = f.Seek(0, io.SeekStart) + require.NoError(t, err) + head := make([]byte, 3) + _, err = io.ReadFull(f, head) + require.NoError(t, err) + assert.Equal(t, "xyz", string(head)) +} + +func TestSpoolCleanupRemovesTheFile(t *testing.T) { + f, cleanup, err := Spool(strings.NewReader("payload")) + require.NoError(t, err) + + name := f.Name() + require.FileExists(t, name) + + cleanup() + assert.NoFileExists(t, name, "the spool must not outlive the command") +} + +func TestOpenSeekableReadsNamedFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "in.tdf") + require.NoError(t, os.WriteFile(path, []byte("payload"), 0o600)) + + in, cleanup, err := OpenSeekable(path) + require.NoError(t, err) + defer cleanup() + + got, err := io.ReadAll(in) + 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()) +} + +func TestOpenSeekableReportsMissingFile(t *testing.T) { + _, _, err := OpenSeekable(filepath.Join(t.TempDir(), "absent.tdf")) + require.Error(t, err) + // Distinguishable from "you gave me nothing", which callers report differently. + require.NotErrorIs(t, err, ErrNoInput) + require.ErrorIs(t, err, os.ErrNotExist) +} + +func TestOpenSeekableSpoolsNonSeekableNamedFile(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("FIFOs are not portable to windows") + } + + path := filepath.Join(t.TempDir(), "in.fifo") + require.NoError(t, syscall.Mkfifo(path, 0o600)) + + content := "a named pipe cannot seek, so this has to be spooled" + go func() { + w, err := os.OpenFile(path, os.O_WRONLY, 0) + if err != nil { + return + } + defer w.Close() + _, _ = io.WriteString(w, content) + }() + + in, cleanup, err := OpenSeekable(path) + require.NoError(t, err) + defer cleanup() + + got, err := io.ReadAll(in) + 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()) + _, err = in.Seek(0, io.SeekStart) + require.NoError(t, err, "the whole point of spooling is that the result seeks") +} + +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) + }() + + in, cleanup, err := OpenSeekable("") + require.NoError(t, err) + defer cleanup() + + got, err := io.ReadAll(in) + require.NoError(t, err) + assert.Equal(t, content, string(got)) +} + +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 }() + + _, _, err = OpenSeekable("") + require.ErrorIs(t, err, ErrNoInput) +} diff --git a/otdfctl/pkg/streamio/output.go b/otdfctl/pkg/streamio/output.go new file mode 100644 index 0000000000..7d3bb35d8b --- /dev/null +++ b/otdfctl/pkg/streamio/output.go @@ -0,0 +1,86 @@ +package streamio + +import ( + "errors" + "os" + "path/filepath" +) + +// ErrOutputFileFinished reports that Commit or Cleanup was called on an +// OutputFile that had already been committed or discarded. +var ErrOutputFileFinished = errors.New("streamio: output file already committed or discarded") + +// outputFileMode is the permission Commit applies to the destination. +// os.CreateTemp always creates the temp file with 0600; without an explicit +// Chmod that would leak onto the destination regardless of the caller's +// umask, so this matches the common default a plain os.Create would produce. +const outputFileMode = 0o644 + +// OutputFile writes to a temporary file alongside the destination and renames +// it into place only once the write has succeeded, so an interrupted or failed +// run leaves no partial output where a complete file is expected. +// +// Note that cli.ExitWithError calls os.Exit, which does not run deferred +// functions. Cleanup must therefore be called explicitly on every error path, +// not only via defer. +type OutputFile struct { + f *os.File + path string + finished bool +} + +// NewOutputFile creates the temporary file in the destination's own directory. +// A rename is only atomic within a single filesystem, so the temp file must +// live beside the destination rather than in a shared temp directory — +// Commit's os.Rename fails outright (EXDEV) if that invariant is broken. +func NewOutputFile(path string) (*OutputFile, error) { + f, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".tmp-*") + if err != nil { + return nil, err + } + return &OutputFile{f: f, path: path}, nil +} + +func (o *OutputFile) Write(p []byte) (int, error) { return o.f.Write(p) } + +// Name reports the path of the temporary file currently being written, which is +// not the destination until Commit succeeds. +func (o *OutputFile) Name() string { return o.f.Name() } + +// Commit closes the temporary file and moves it onto the destination path. +// +// It returns ErrOutputFileFinished if called more than once, or after Cleanup — +// otherwise a second call would re-enter the close/rename-failure path against +// an already-closed or already-moved file and report a spurious error. +func (o *OutputFile) Commit() error { + if o.finished { + return ErrOutputFileFinished + } + o.finished = true + + if err := o.f.Chmod(outputFileMode); err != nil { + o.f.Close() + os.Remove(o.f.Name()) + return err + } + if err := o.f.Close(); err != nil { + os.Remove(o.f.Name()) + return err + } + if err := os.Rename(o.f.Name(), o.path); err != nil { + os.Remove(o.f.Name()) + return err + } + return nil +} + +// Cleanup discards the temporary file. It is a no-op after a successful Commit +// (or a prior Cleanup), so it is safe to both defer it and call it directly. +func (o *OutputFile) Cleanup() { + if o.finished { + return + } + o.finished = true + o.f.Close() + os.Remove(o.f.Name()) +} diff --git a/otdfctl/pkg/streamio/output_test.go b/otdfctl/pkg/streamio/output_test.go new file mode 100644 index 0000000000..e7e73cb643 --- /dev/null +++ b/otdfctl/pkg/streamio/output_test.go @@ -0,0 +1,148 @@ +package streamio + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestOutputFileCommitRenamesIntoPlace(t *testing.T) { + dir := t.TempDir() + dest := filepath.Join(dir, "out.tdf") + + o, err := NewOutputFile(dest) + require.NoError(t, err) + + _, err = o.Write([]byte("payload")) + require.NoError(t, err) + + // Nothing is visible at the destination until Commit. + _, err = os.Stat(dest) + require.ErrorIs(t, err, os.ErrNotExist, "destination must not exist before Commit") + + require.NoError(t, o.Commit()) + + got, err := os.ReadFile(dest) + require.NoError(t, err) + assert.Equal(t, "payload", string(got)) + assert.Empty(t, tempSiblings(t, dir, "out.tdf"), "temp file should be gone after Commit") +} + +func TestOutputFileCleanupLeavesNoPartialOutput(t *testing.T) { + dir := t.TempDir() + dest := filepath.Join(dir, "out.tdf") + + o, err := NewOutputFile(dest) + require.NoError(t, err) + _, err = o.Write([]byte("partial")) + require.NoError(t, err) + + // Simulates the failure path: encryption died after some bytes were written. + o.Cleanup() + + _, err = os.Stat(dest) + require.ErrorIs(t, err, os.ErrNotExist, "a failed run must not leave a partial file") + assert.Empty(t, tempSiblings(t, dir, "out.tdf"), "a failed run must not leave a temp file") +} + +func TestOutputFileCleanupAfterCommitIsNoop(t *testing.T) { + dir := t.TempDir() + dest := filepath.Join(dir, "out.tdf") + + o, err := NewOutputFile(dest) + require.NoError(t, err) + _, err = o.Write([]byte("payload")) + require.NoError(t, err) + require.NoError(t, o.Commit()) + + // Both deferred and explicit cleanup run on the success path. + o.Cleanup() + o.Cleanup() + + got, err := os.ReadFile(dest) + require.NoError(t, err) + assert.Equal(t, "payload", string(got), "Cleanup after Commit must not delete the output") +} + +func TestOutputFileTempIsSiblingOfDestination(t *testing.T) { + dir := t.TempDir() + dest := filepath.Join(dir, "out.tdf") + + o, err := NewOutputFile(dest) + require.NoError(t, err) + defer o.Cleanup() + + // A rename is only atomic within one filesystem — a shared temp dir on a + // different filesystem would make Commit's os.Rename fail outright — so the + // temp file must live beside the destination. + assert.Equal(t, dir, filepath.Dir(o.Name())) +} + +func TestOutputFileCommitSetsReadableMode(t *testing.T) { + dir := t.TempDir() + dest := filepath.Join(dir, "out.tdf") + + o, err := NewOutputFile(dest) + require.NoError(t, err) + _, err = o.Write([]byte("payload")) + require.NoError(t, err) + require.NoError(t, o.Commit()) + + info, err := os.Stat(dest) + require.NoError(t, err) + assert.Equal(t, os.FileMode(outputFileMode), info.Mode().Perm(), + "os.CreateTemp defaults to 0600; Commit must not leak that onto the destination") +} + +func TestOutputFileCommitAfterCommitReturnsError(t *testing.T) { + dir := t.TempDir() + dest := filepath.Join(dir, "out.tdf") + + o, err := NewOutputFile(dest) + require.NoError(t, err) + _, err = o.Write([]byte("payload")) + require.NoError(t, err) + require.NoError(t, o.Commit()) + + require.ErrorIs(t, o.Commit(), ErrOutputFileFinished) + + got, err := os.ReadFile(dest) + require.NoError(t, err) + assert.Equal(t, "payload", string(got), "a redundant Commit must not disturb the already-committed output") +} + +func TestOutputFileCommitAfterCleanupReturnsError(t *testing.T) { + dir := t.TempDir() + dest := filepath.Join(dir, "out.tdf") + + o, err := NewOutputFile(dest) + require.NoError(t, err) + _, err = o.Write([]byte("payload")) + require.NoError(t, err) + + o.Cleanup() + require.ErrorIs(t, o.Commit(), ErrOutputFileFinished) + + _, err = os.Stat(dest) + require.ErrorIs(t, err, os.ErrNotExist, "Commit after Cleanup must not create the destination") +} + +// tempSiblings returns any leftover temp files NewOutputFile would have created +// for dest in dir. +func tempSiblings(t *testing.T, dir, dest string) []string { + t.Helper() + entries, err := os.ReadDir(dir) + require.NoError(t, err) + + var found []string + for _, e := range entries { + if strings.HasPrefix(e.Name(), "."+dest+".tmp-") { + found = append(found, e.Name()) + } + } + return found +} diff --git a/spec/DSPX-4499.md b/spec/DSPX-4499.md new file mode 100644 index 0000000000..f29fed6c7d --- /dev/null +++ b/spec/DSPX-4499.md @@ -0,0 +1,294 @@ +--- +ticket: DSPX-4499 +title: otdfctl streaming encrypt/decrypt +status: draft +authors: [dmihalcik@virtru.com] +branches: + [ + opentdf/platform:dspx-2604-08-streamio, + opentdf/platform:dspx-2604-09-stream-encrypt, + opentdf/platform:dspx-2604-10-stream-decrypt, + ] +prs: [] +depends-on: [] +created: 2026-08-25 +updated: 2026-08-31 +--- + +# otdfctl: stream encrypt/decrypt instead of buffering the whole payload in memory + +## Summary + +`otdfctl encrypt` and `otdfctl decrypt` hold the entire plaintext _and_ the entire +ciphertext in memory at once. Peak RSS scales at roughly 3.6x the payload, so a 1 GiB file +costs ~3.7 GiB of RAM and a large enough file simply OOMs on a machine that has plenty of +disk for it. The SDK already supports streaming; this is purely a CLI-layer choice. + +> The original branch was named `DSPX-4499-streaming-codec`, but no codec abstraction is +> involved and none is being introduced. The name was vestigial and has been dropped. + +## Problem / Motivation + +Measured on a GitHub `ubuntu-latest` runner by the xtest paired benchmark harness +([tests run 32507318075](https://github.com/opentdf/tests/actions/runs/32507318075), go +SDK, `--bench-payloads 1GiB`), reading `ru_maxrss` from `os.wait4` on the forked CLI +process: + +| cell | median peak RSS | max | +| -------------- | --------------- | -------- | +| encrypt 1 MiB | 66 MiB | 79 MiB | +| encrypt 1 GiB | 3754 MiB | 5119 MiB | +| decrypt 1 GiB | 3808 MiB | 5025 MiB | + +Growth is essentially all payload-proportional: 66 MiB → 3754 MiB for 1023 MiB more input. + +### Root cause + +**Encrypt** + +1. `pkg/cli/pipe.go` — `ReadFromFile` does `io.ReadAll(fileToEncrypt)`, slurping the whole + file into a `[]byte`, with no cap at all. `cmd/tdf/encrypt.go` uses + `utils.ReadBytesFromFile`, the same pattern with a 10 GB cap. +2. `pkg/handlers/tdf.go` — `EncryptBytes` creates `enc := bytes.NewBuffer(encrypted)` and + calls `h.sdk.CreateTDF(enc, bytes.NewReader(unencrypted), opts...)`. The entire TDF + accumulates in a second in-memory buffer. +3. `cmd/tdf/encrypt.go` — only then does `io.Copy(dest, encrypted)` touch the disk. + +**Decrypt** + +1. `cmd/tdf/decrypt.go` — the TDF is slurped to `bytesToDecrypt`. +2. `pkg/handlers/tdf.go` — `out := &bytes.Buffer{}`, `ec := bytes.NewReader(toDecrypt)`; + `io.Copy(pt, r)` fills `out` with the whole plaintext. +3. `cmd/tdf/decrypt.go` — for stdout, `fmt.Print(decrypted.String())` makes a _third_ full + copy, since `Buffer.String()` allocates a new string. The `-o` path uses + `decrypted.Bytes()` and avoids that one. + +So the live set at peak is ~2 GiB minimum for a 1 GiB encrypt (two whole-payload copies), +plus the transient while each buffer's incremental growth copies old to new, plus Go's +default `GOGC=100` letting the heap reach ~2x live before collecting. RSS is a high-water +mark and Go returns pages lazily. ~3.7 GiB is what that arithmetic predicts, and it is what +we measure. + +### Why the SDK is not the blocker + +The relevant SDK signatures on `main` already take streams: + +```go +func (s SDK) CreateTDF(writer io.Writer, reader io.ReadSeeker, opts ...TDFOption) (*TDFObject, error) +func (s SDK) LoadTDF(reader io.ReadSeeker, opts ...TDFReaderOption) (*Reader, error) +func GetTdfType(reader io.ReadSeeker) TdfType +``` + +An `*os.File` satisfies `io.ReadSeeker`, so file-to-file streaming at roughly constant +memory is available today. `otdfctl` hands these APIs a `bytes.Reader` and a `bytes.Buffer` +instead. **No SDK change is required for this work**, and none is made. + +## Proposed Solution + +Pass the open `*os.File` straight through, and write to the destination file rather than to +a `bytes.Buffer`. `EncryptBytes` / `DecryptBytes` are replaced by stream-shaped handlers +taking an `io.ReadSeeker` and an `io.Writer`. + +### Shipped as three PRs + +| PR | Contents | +| --------------------------------- | ------------------------------------------------------------------------------------------------ | +| `dspx-2604-08-streamio` | New `otdfctl/pkg/streamio` package; deprecate `pkg/cli/pipe.go`; `InspectTDF` takes a `ReadSeeker` | +| `dspx-2604-09-stream-encrypt` | `handlers.Encrypt`; `cmd/tdf/encrypt.go` streams; MIME sniffing rewritten | +| `dspx-2604-10-stream-decrypt` | `handlers.Decrypt`; `cmd/tdf/decrypt.go` streams; `MaxFileSize` removed; e2e cases | + +All three sit directly on `main`. An earlier revision of this work was stacked on +[#3782](https://github.com/opentdf/platform/pull/3782) and +[#3865](https://github.com/opentdf/platform/pull/3865) so that encrypt could take a +non-seekable `io.Reader`. That coupling bought one thing — no encrypt-side spool — at the +cost of parking a user-visible OOM fix behind two large unreviewed SDK PRs. It is not worth +it. Encrypt spools piped stdin instead (see below), and the spool can be deleted in a +follow-up once `CreateTDF` accepts an `io.Reader`. + +### Why `pkg/streamio` and not `pkg/cli` + +Per review feedback on the earlier revision, the plumbing belongs in a package rather than +in `cmd/`. It is a new package rather than an addition to `pkg/cli` because everything in +it returns errors, where `pkg/cli`'s helpers call `cli.ExitWithError` — that is, +`os.Exit` — from inside the read. That makes them unusable anywhere that needs to recover, +and untestable without a subprocess. + +`pkg/cli/pipe.go` is **deprecated, not deleted**: it is an exported package and external +consumers may import it. The deprecation notice on `ReadFromFile` names the uncapped +`io.ReadAll` explicitly. + +## Inputs / Outputs / Contracts + +New handler surface in `otdfctl/pkg/handlers/tdf.go`. Options structs, because the existing +parameter lists were already eight positional arguments long: + +```go +type EncryptOptions struct { + TDFType string + Attributes []string + MimeType string + KASURLPath string + Assertions string + WrappingKeyAlgorithm ocrypto.KeyType + TargetMode string +} + +// Encrypt streams plaintext from in to a TDF on out. Memory is bounded by the SDK's +// segment size, not by payload length. +func (h Handler) Encrypt(ctx context.Context, out io.Writer, in io.ReadSeeker, o EncryptOptions) error + +type DecryptOptions struct { + AssertionVerificationKeysFile string + DisableAssertionCheck bool + SessionKeyAlgorithm ocrypto.KeyType + KASAllowList []string + IgnoreAllowlist bool + FulfillableObligations []string +} + +func (h Handler) Decrypt(ctx context.Context, out io.Writer, in io.ReadSeeker, o DecryptOptions) error + +func (h Handler) InspectTDF(in io.ReadSeeker) (TDFInspect, []error) +``` + +These replace `EncryptBytes`, `DecryptBytes`, and the `[]byte` form of `InspectTDF`. The +only callers are `cmd/tdf/{encrypt,decrypt,inspect}.go`; there are no test callers. + +And in `otdfctl/pkg/streamio`: + +```go +// PipeReader reports whether stdin has piped data waiting, without consuming it. +func PipeReader(in *os.File) (*bufio.Reader, bool, error) + +// Spool copies a non-seekable reader to a temp file and rewinds it. +func Spool(r io.Reader) (*os.File, func(), error) + +// OpenSeekable resolves a path argument or piped stdin to a seekable reader. +func OpenSeekable(path string) (*os.File, func(), error) + +// OutputFile writes to a temp sibling and renames into place only on Commit. +type OutputFile struct{ /* ... */ } +``` + +**No CLI flags change.** User-visible behavior is unchanged except as noted under Edge +Cases. Worth knowing for future work: otdfctl flags are declared in YAML frontmatter of +embedded markdown (`docs/man/*/_index.md`, read via `doc.GetDocFlag`), not in Go literals. + +## Edge Cases & Constraints + +- **Both commands need a seekable input, for different reasons.** `CreateTDF` seeks to the + end of the payload to size it, and knowing the size up front is what lets it avoid + defaulting to ZIP64. A TDF's manifest lives at the _end_ of the archive, so `LoadTDF` and + `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`. + +- **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 + silently flip file encrypts to ZIP64 and change the output bytes. `detectMimeType` + instead reads a bounded 1 MiB prefix — the limit `mimetype.SetLimit` already imposes — + and seeks back to 0, so the encoder still sees the whole payload from the start. + +- **A latent panic in the extension fallback.** `cmd/tdf/encrypt.go` on `main` calls + `mimetype.Lookup(fileExt).String()` when content sniffing yields + `application/octet-stream`. `mimetype.Lookup` takes a _MIME type string_ and runs + `mime.ParseMediaType` on it, which fails on a bare extension, so it returns `nil` for + every extension — and `(*MIME).String()` has no nil guard. Any file whose contents cannot + be classified _and_ whose name has an extension crashes the CLI. Verified empirically. + Fixed by using `mime.TypeByExtension("."+fileExt)`, which is the actual extension lookup, + and pinned by `TestDetectMimeTypeUnknownExtensionStaysOctetStream`. + +- **`io.Copy` must select `WriteTo`.** The SDK `Reader` implements `WriteTo` + (`sdk/tdf.go:973`), the O(one-segment) streaming decrypt path. Its `Read` delegates to + `ReadAt`, which accumulates every segment into a `bytes.Buffer`. If a future refactor + drops `io.WriterTo`, decrypt silently re-regresses to buffering with no test failure. + Guarded with `var _ io.WriterTo = (*sdk.Reader)(nil)`. + +- **Output file on error.** Writing directly to the destination means a failed encrypt + would leave a partial `.tdf` behind where today it leaves nothing. `streamio.OutputFile` + writes to a temp file in the destination's own directory and renames on success — + same-directory keeps the rename atomic — and removes it on failure. For stdout there is + no rename and a partial stream is unavoidable, as with any streaming CLI. + +- **`cli.ExitWithError` calls `os.Exit`, which does not run deferred functions.** Every + exit path must therefore discard the spool and the partial output explicitly, not only + via `defer`. This includes `inspect`'s _success_ path, which exits through + `c.ExitWithJSON`. + +- **Ordering.** The `.tdf` extension fixup and destination creation happened _after_ + encryption; they move ahead of it. + +- **Empty-vs-absent stdin.** The current exactly-one-input check keys off + `len(piped) > 0`, which requires having read stdin. Replacing it with an + `os.ModeCharDevice` presence test alone would change behavior for + `otdfctl encrypt < /dev/null` (piped but empty). `streamio.PipeReader` uses the presence + test _plus_ a `bufio.Peek(1)` to preserve today's semantics without an unbounded read. + +- **File size cap.** `MaxFileSize` (`cmd/tdf/tdf.go`, 10 GB) existed to bound RAM. With + streaming it is an arbitrary cliff below the SDK's real 64 GiB `maxFileSizeSupported` + (`sdk/tdf.go:34`), which now enforces the limit. Removed. + +## Out of Scope + +- 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. +- 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. +- Flag or man-page changes; none are required. + +## Acceptance Criteria + +- [ ] Peak RSS for `encrypt` and `decrypt` of a 1 GiB file is bounded by a small multiple of + the segment size, not of the payload size — target well under 500 MiB, versus ~3.7 GiB + today. Asserted by the e2e case below; not yet run against this branch. +- [ ] stdin/stdout paths keep working, including the documented + `echo "hello world" | otdfctl encrypt | otdfctl decrypt | cat` + (`docs/man/encrypt/_index.md:72`). +- [ ] A failed encrypt or decrypt with `-o` leaves neither a partial output file nor a + stray temp file. +- [x] `make lint` passes with 0 new issues; `make test` passes. +- [x] Unit coverage for the MIME rewind, the extension fallback, and the panic it replaces. + +### Prior measurement + +The stacked revision of this work, whose file-input path is byte-for-byte the same code, +measured on macOS with `/usr/bin/time -l`: + +| payload | encrypt | decrypt | +| ------- | ------- | ------- | +| 1 GiB | 73 MiB | 76 MiB | +| 4 GiB | 78 MiB | 74 MiB | + +Flat in payload size, roughly 50x below the ~3.7 GiB the buffered implementation used at +1 GiB, and both round-trips byte-identical. These numbers are carried over as an +expectation, not as a measurement of this branch. + +## Testing + +`otdfctl/e2e/streaming.bats` is a **new file**, not an addition to `encrypt-decrypt.bats`. +That file carries a pre-existing file-level `skip` pending the namespaced-subject-mappings +migration, so anything added to it would silently not run. Nothing here needs an +entitlement: the round-trips encrypt with no attributes, and the two failure cases are +forced with an unresolvable attribute FQN and a KAS allowlist that excludes the platform. + +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 +(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. + +## Side effect worth knowing about + +The xtest benchmark harness gates on `rss`. At 1 GiB it currently reports ratio 1.00004 +with CI [0.9996, 1.0038] — the tightest, most confident-looking PASS in the run — because +both arms allocate the same two whole-payload buffers and that high-water mark swamps +everything else. Any per-segment allocation change (for example the ~2 MiB-per-segment +saving claimed by [#3865](https://github.com/opentdf/platform/pull/3865)) is invisible +under it. This is the mirror image of the RSS floor-censoring the harness already guards +against: a ceiling rather than a floor. Fixing the CLI makes the harness's RSS metric able +to see SDK allocation changes at all.