From 00256d6bc90e80db31fedb5cadc235fe2a321308 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Mon, 31 Aug 2026 21:36:57 -0400 Subject: [PATCH 1/2] fix(cli): stream decrypt and inspect instead of buffering `otdfctl decrypt` read the whole TDF into memory, handed the slice to DecryptBytes, which accumulated the whole plaintext in a bytes.Buffer, and then -- for stdout -- called Buffer.String(), allocating a third full copy. Peak RSS was roughly 3.6x the payload; a 1 GiB file cost ~3.7 GiB of RAM and a large enough file simply OOMed on a machine with plenty of disk for it. The plaintext now streams from the SDK reader to the destination. Handler.Decrypt takes an io.ReadSeeker and an io.Writer, with DecryptOptions replacing the positional parameter list, and inspect reaches the manifest through the same seekable reader rather than buffering the archive to get at its tail. Measured on a 1 GiB round-trip: encrypt peaks at 74 MiB and decrypt at 67 MiB, against ~3754 MiB and ~3808 MiB before. The round-trip is byte-identical. io.Copy is what does the streaming, and it does so only because sdk.Reader implements WriteTo, which decrypts one segment at a time. Its Read delegates to ReadAt, which grows an internal bytes.Buffer holding every segment decrypted so far -- so dropping WriteTo would silently restore the old memory profile with no test failure to show for it. A compile-time assertion pins the interface. Removes MaxFileSize. The 10 GB cap existed to bound RAM; the real limit is the SDK maxFileSizeSupported at 64 GiB, which enforces itself. Output to a file is atomic, as on the encrypt side: the plaintext goes to a temporary sibling and is renamed into place only on success. Since cli.ExitWithError calls os.Exit and skips deferred functions, the spooled input and the partial output are discarded explicitly on every exit path -- including inspect's success path, which exits through ExitWithJSON. A destination a rename cannot stand in for -- /dev/null, a fifo, a symlink the caller means to write through -- is opened and written directly instead. decrypt's -o was a plain os.Create before this change, and `-o /dev/null` is a routine way to time a decrypt or check one succeeds without keeping the plaintext; the atomic path alone would have regressed both. The output file mode is deliberately left as it is. #4037 turns it into a per-caller parameter and #4046 applies it through the umask, which is a better answer for the hardcoded 0644 inherited here than anything this PR could do in passing. e2e coverage lands in a new otdfctl/e2e/streaming.bats rather than in encrypt-decrypt.bats, keeping the streaming concerns -- spooling, temp output, peak memory -- apart from that file's entitlement fixtures. Nothing in the new file needs an entitlement, so it needs no policy fixtures: the round-trips use no attributes, and the failure cases are forced with an unresolvable attribute FQN and a KAS allowlist that excludes the platform. Both that file and encrypt-decrypt.bats are tagged unattributed_encrypt, and action.yaml gives the tag its own pass ahead of the parallel batch. That ordering is load-bearing, not tidiness. An encrypt 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 afterwards: a base key can be replaced but never cleared, so every unattributed encrypt scheduled after that file yields a TDF nothing can decrypt. Under --jobs 4 the file order is nondeterministic, so overlapping the two made this suite flaky rather than merely broken -- which is how it presented, a different subset of round-trips failing per run. Running alone also keeps the 1 GiB peak-RSS case from measuring itself against three neighbours competing for the same memory. encrypt-decrypt.bats is tagged for the same reason. #4042 lifted its file-level skip, and its very first case is an unattributed round-trip, so it now races key-base.bats for a slot in the parallel batch and fails whenever it loses. That it passes today is an accident of bats scheduling files alphabetically. The underlying leak is still worth closing in key-base.bats. action.yaml also installs the 'time' package, and the peak-RSS case now fails rather than skips when CI lacks GNU time. It is the only test that demonstrates the fix, so a silent skip would let a return to whole-payload buffering through. Signed-off-by: Dave Mihalcik --- otdfctl/cmd/tdf/decrypt.go | 88 +++++----- otdfctl/cmd/tdf/tdf.go | 35 +--- otdfctl/e2e/action.yaml | 21 ++- otdfctl/e2e/encrypt-decrypt.bats | 9 + otdfctl/e2e/streaming.bats | 203 +++++++++++++++++++++++ otdfctl/pkg/handlers/tdf.go | 78 +++++---- otdfctl/pkg/handlers/tdf_test.go | 90 ++++++++++ otdfctl/pkg/streamio/output.go | 76 +++++++-- otdfctl/pkg/streamio/output_unix_test.go | 103 ++++++++++++ 9 files changed, 581 insertions(+), 122 deletions(-) create mode 100755 otdfctl/e2e/streaming.bats create mode 100644 otdfctl/pkg/handlers/tdf_test.go create mode 100644 otdfctl/pkg/streamio/output_unix_test.go diff --git a/otdfctl/cmd/tdf/decrypt.go b/otdfctl/cmd/tdf/decrypt.go index d627ecefcd..05096d7ec0 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/streamio" - "github.com/opentdf/platform/otdfctl/pkg/utils" "github.com/spf13/cobra" ) @@ -45,60 +46,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) - if err != nil { - cli.ExitWithError("Failed to read file:", err) + } + 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() + + // cli.ExitWithError calls os.Exit, which skips deferred functions, so both + // the spooled input and the partial output have to be discarded first. + // Declared before the destination exists so every exit below can use it. + var outFile *streamio.OutputFile + fail := func(msg string, err error) { + if outFile != nil { + outFile.Cleanup() } + closeIn() + cli.ExitWithError(msg, err) } - if len(bytesToDecrypt) == 0 { - cli.ExitWithError("Must provide ONE of the following to decrypt: [file argument, stdin input]", errors.New("no input provided")) + // Resolve the destination before decrypting, so the plaintext streams + // straight to it rather than accumulating in memory first. + var dest io.Writer = os.Stdout + if output != "" { + outFile, err = streamio.NewOutputFile(output, decryptedOutputFileMode) + if err != nil { + fail("Failed to write decrypted data to file", err) + } + defer outFile.Cleanup() + dest = outFile } 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 := streamio.NewOutputFile(output, decryptedOutputFileMode) - if err != nil { - cli.ExitWithError("Failed to write decrypted data to file", err) - } - defer f.Cleanup() - _, err = f.Write(decrypted.Bytes()) - if err != nil { - f.Cleanup() - cli.ExitWithError("Failed to write decrypted data to file", err) - } - if err := f.Commit(); err != nil { - f.Cleanup() - 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/tdf.go b/otdfctl/cmd/tdf/tdf.go index 9f4395a782..be6a89336e 100644 --- a/otdfctl/cmd/tdf/tdf.go +++ b/otdfctl/cmd/tdf/tdf.go @@ -1,39 +1,8 @@ package tdf -import ( - "io" - "os" - - "github.com/opentdf/platform/otdfctl/pkg/cli" - "github.com/opentdf/platform/otdfctl/pkg/streamio" -) - 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 ) - -// readPipedStdin returns the whole of piped stdin, or nil when stdin is a -// terminal or an empty redirect. -// -// Detection is delegated to streamio.PipeReader so there is a single answer to -// "is there piped input?" across the CLI. The read itself is still unbounded; -// callers that must not hold the payload in memory should use -// streamio.OpenSeekable instead. -func readPipedStdin() []byte { - r, ok, err := streamio.PipeReader(os.Stdin) - if err != nil { - cli.ExitWithError("failed to scan bytes from stdin", err) - } - if !ok { - return nil - } - buf, err := io.ReadAll(r) - if err != nil { - cli.ExitWithError("failed to scan bytes from stdin", err) - } - return buf -} diff --git a/otdfctl/e2e/action.yaml b/otdfctl/e2e/action.yaml index b95a21027c..b3b3acff0f 100644 --- a/otdfctl/e2e/action.yaml +++ b/otdfctl/e2e/action.yaml @@ -34,7 +34,11 @@ runs: shell: bash run: | sudo apt-get update - sudo apt-get install -y gnome-keyring + # 'time' is GNU time, which reports 'Maximum resident set size'. The + # streaming suite's peak-RSS case needs it and would otherwise skip + # itself silently -- and that case is the only proof that encrypt and + # decrypt have not gone back to buffering the whole payload. + sudo apt-get install -y gnome-keyring time working-directory: otdfctl - name: Setup Bats and bats libs uses: bats-core/bats-action@2.0.0 @@ -54,12 +58,23 @@ runs: # suite while other files still create unnamespaced policy fixtures. bats --tap e2e --filter-tags namespaced_policy_migration | tee e2e/bats-results.tap + # Then every file that encrypts without attributes, also on its own. + # Unattributed encrypts fall 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 these have to run first rather than race for a + # slot. Running alone also keeps streaming.bats' 1 GiB peak-RSS case + # from measuring itself against three neighbours competing for the same + # memory. + bats --tap e2e --filter-tags unattributed_encrypt | 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,!unattributed_encrypt' --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,!unattributed_encrypt' | tee -a e2e/bats-results.tap fi env: # Define 'bats' install location in ubuntu diff --git a/otdfctl/e2e/encrypt-decrypt.bats b/otdfctl/e2e/encrypt-decrypt.bats index 5c4e79ac18..464d1e9f98 100755 --- a/otdfctl/e2e/encrypt-decrypt.bats +++ b/otdfctl/e2e/encrypt-decrypt.bats @@ -1,6 +1,15 @@ #!/usr/bin/env bats +# bats file_tags=unattributed_encrypt + # Tests for encrypt decrypt +# +# Tagged so action.yaml runs this file before the parallel batch. Several cases +# here encrypt 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. Scheduled +# into the parallel batch this file would eventually land after key-base.bats +# and fail on an undecryptable TDF. See the header of streaming.bats. setup_file() { export CREDSFILE=creds.json diff --git a/otdfctl/e2e/streaming.bats b/otdfctl/e2e/streaming.bats new file mode 100755 index 0000000000..9bf1c69a28 --- /dev/null +++ b/otdfctl/e2e/streaming.bats @@ -0,0 +1,203 @@ +#!/usr/bin/env bats + +# bats file_tags=unattributed_encrypt + +# Streaming encrypt/decrypt/inspect (DSPX-4499). +# +# These live outside encrypt-decrypt.bats to keep the streaming concerns -- +# spooling, temp output, peak memory -- separate from that file's entitlement +# fixtures. None of the cases here need an entitlement: the round-trips encrypt +# with no attributes, and the failure cases are forced with an unresolvable +# attribute FQN and a KAS allowlist that excludes the platform, neither of which +# requires policy fixtures. +# +# The unattributed_encrypt tag is load-bearing, and is shared with +# encrypt-decrypt.bats. 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. The tag lets action.yaml run every file that encrypts without +# attributes ahead of the parallel batch rather than racing key-base.bats for a +# slot. That 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" +} + +teardown() { + # The large-payload case below leaves ~3 GiB in BATS_TEST_TMPDIR. An assertion + # failure aborts the test body, so cleanup placed inline would be skipped + # exactly when the files are largest. Harmless for every other test here. + rm -f "$BATS_TEST_TMPDIR/big.bin" "$BATS_TEST_TMPDIR/big.bin.tdf" "$BATS_TEST_TMPDIR/big.out" +} + +# assert_no_leftovers fails if anything matches the given glob, naming what it +# found. Listing the paths rather than counting them keeps this portable: BSD +# `wc -l` pads its output to a fixed width, so counting passes on CI's coreutils +# and fails on a developer's macOS for no real reason. +assert_no_leftovers() { + run bash -c "ls -d $1 2>/dev/null" + assert_output "" "expected no files matching $1" +} + +# 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" + + assert_no_leftovers "$BATS_TEST_TMPDIR/otdfctl-spool-*" +} + +@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. + assert_no_leftovers "$BATS_TEST_TMPDIR/otdfctl-spool-*" +} + +# 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" ] + + assert_no_leftovers "$BATS_TEST_TMPDIR/.payload.txt.tdf.tmp-*" +} + +@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" ] + + assert_no_leftovers "$BATS_TEST_TMPDIR/.payload.out.tmp-*" +} + +# The payoff of writing to a temp sibling: a failed decrypt over an existing +# file leaves the old contents intact, where writing straight to the destination +# would have truncated it before discovering the failure. +@test "decrypt leaves an existing output file untouched when it fails" { + ./otdfctl encrypt -o "$TDF_OUT" $COMMON "$PLAIN" + printf 'do not clobber me\n' >"$RESULT" + + run ./otdfctl decrypt -o "$RESULT" $COMMON --kas-allowlist "https://nowhere.example.com" "$TDF_OUT" + assert_failure + + run cat "$RESULT" + assert_output "do not clobber me" +} + +# -o at a path a rename cannot stand in for is written through directly, the way +# os.Create did. /dev/null is the case people actually use, to time a decrypt or +# to check one succeeds without keeping the plaintext. +@test "decrypt to /dev/null succeeds and leaves the device node alone" { + ./otdfctl encrypt -o "$TDF_OUT" $COMMON "$PLAIN" + + run ./otdfctl decrypt -o /dev/null $COMMON "$TDF_OUT" + assert_success + [ -c /dev/null ] +} + +# 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" { + # '|| true' so that finding neither binary reaches the skip below: bats runs + # tests under errexit, and a bare failing assignment would abort the test with + # no message rather than skipping it. + GNU_TIME=$(command -v gtime || command -v /usr/bin/time || true) + if [ -z "$GNU_TIME" ] || ! $GNU_TIME -v true 2>&1 | grep -q "Maximum resident set size"; then + # In CI this must not skip. It is the only test that demonstrates the fix, + # and a silent skip would let a return to whole-payload buffering through. + # action.yaml installs the 'time' package for exactly this reason. + [ -z "$CI" ] || fail "GNU time is required in CI: install the 'time' package" + 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]*') + + 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/handlers/tdf.go b/otdfctl/pkg/handlers/tdf.go index f4bfb25e27..9786b09be9 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" @@ -107,64 +106,77 @@ func (h Handler) Encrypt(ctx context.Context, out io.Writer, in io.ReadSeeker, o } } -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 +} + +// The assertion below is what keeps Decrypt's io.Copy streaming 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 } // InspectTDF reads the manifest and attributes of a TDF. diff --git a/otdfctl/pkg/handlers/tdf_test.go b/otdfctl/pkg/handlers/tdf_test.go new file mode 100644 index 0000000000..49eb9ba551 --- /dev/null +++ b/otdfctl/pkg/handlers/tdf_test.go @@ -0,0 +1,90 @@ +package handlers + +import ( + "bytes" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// zipPrefix is the signature sdk.GetTdfType looks for. It is enough to route +// input down the Standard branch, which is where the option plumbing lives; the +// cases below all fail before anything reaches the SDK, so no platform +// connection and no real TDF is needed. +var zipPrefix = []byte{0x50, 0x4B, 0x03, 0x04} + +func TestDecryptRejectsNonTDFInput(t *testing.T) { + var out bytes.Buffer + + err := Handler{}.Decrypt(t.Context(), &out, bytes.NewReader([]byte("not a tdf at all")), DecryptOptions{}) + + require.EqualError(t, err, "invalid TDF") + assert.Empty(t, out.Bytes(), "a rejected input must not produce output") +} + +// GetTdfType cannot read four bytes from an empty input, so it reports Invalid +// rather than a read error. Worth pinning: an empty spool is a plausible way to +// get here, and "invalid TDF" is the message the user sees for it. +func TestDecryptRejectsEmptyInput(t *testing.T) { + var out bytes.Buffer + + err := Handler{}.Decrypt(t.Context(), &out, bytes.NewReader(nil), DecryptOptions{}) + + require.EqualError(t, err, "invalid TDF") + assert.Empty(t, out.Bytes()) +} + +func TestDecryptAssertionVerificationKeyErrors(t *testing.T) { + dir := t.TempDir() + + malformed := filepath.Join(dir, "malformed.json") + require.NoError(t, os.WriteFile(malformed, []byte("{not json"), 0o600)) + + badKey := filepath.Join(dir, "bad-key.json") + require.NoError(t, os.WriteFile(badKey, + []byte(`{"keys":{"assertion1":{"alg":"RS256","key":"not a pem block"}}}`), 0o600)) + + for _, tc := range []struct { + name string + file string + wantMsg string + }{ + { + name: "missing file", + file: filepath.Join(dir, "does-not-exist.json"), + wantMsg: "unable to read assertions verification keys file", + }, + { + name: "malformed json", + file: malformed, + wantMsg: "unable to unmarshal assertion verification keys json", + }, + { + name: "unusable key", + file: badKey, + wantMsg: "error with assertion signing key", + }, + } { + t.Run(tc.name, func(t *testing.T) { + var out bytes.Buffer + + err := Handler{}.Decrypt(t.Context(), &out, bytes.NewReader(zipPrefix), DecryptOptions{ + AssertionVerificationKeysFile: tc.file, + }) + + require.ErrorContains(t, err, tc.wantMsg) + assert.Empty(t, out.Bytes(), "an option failure must not produce output") + }) + } +} + +func TestInspectTDFRejectsNonTDFInput(t *testing.T) { + result, errs := Handler{}.InspectTDF(bytes.NewReader([]byte("not a tdf at all"))) + + require.Len(t, errs, 1) + require.ErrorIs(t, errs[0], ErrTDFInspectFailNotValidTDF) + assert.Nil(t, result.ZTDFManifest) +} diff --git a/otdfctl/pkg/streamio/output.go b/otdfctl/pkg/streamio/output.go index aaa461a3e7..04dc602dde 100644 --- a/otdfctl/pkg/streamio/output.go +++ b/otdfctl/pkg/streamio/output.go @@ -21,23 +21,48 @@ const tempFileAttempts = 1000 // 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. // +// A destination that a rename cannot stand in for — /dev/null, /dev/stdout, a +// fifo, a symlink the caller means to write through — is opened and written +// directly instead, matching what os.Create did before. Those destinations give +// up the no-partial-output guarantee, which is inherent: there is nothing to +// rename into place. +// // 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 + f *os.File + path string + direct bool + 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. +// NewOutputFile opens the destination for writing. +// +// For an ordinary destination it 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. // // The temp file is created with mode, and a rename carries that mode onto the -// destination after the process umask has been applied. +// destination after the process umask has been applied. A destination written +// through directly is opened with mode, which the umask likewise applies to, +// and which has no effect at all on a destination that already exists. func NewOutputFile(path string, mode os.FileMode) (*OutputFile, error) { + direct, err := isDirectDestination(path) + if err != nil { + return nil, err + } + if direct { + f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, mode) + if err != nil { + return nil, err + } + return &OutputFile{f: f, path: path, direct: true}, nil + } + f, err := createTemp(filepath.Dir(path), "."+filepath.Base(path)+".tmp-", mode) if err != nil { return nil, err @@ -67,10 +92,30 @@ func createTemp(dir, prefix string, mode os.FileMode) (*os.File, error) { return nil, fmt.Errorf("streamio: no unused temporary name for %s after %d attempts", filepath.Join(dir, prefix), tempFileAttempts) } +// isDirectDestination reports whether path names something that must be written +// through rather than replaced by a rename. +// +// os.Lstat rather than os.Stat, so a symlink is recognized as a symlink: with +// os.Stat a link to a regular file looks regular, and the rename would replace +// the link itself instead of updating what it points at. +// +// A path that does not exist yet is the common case and takes the atomic route. +func isDirectDestination(path string) (bool, error) { + fi, err := os.Lstat(path) + if errors.Is(err, os.ErrNotExist) { + return false, nil + } + if err != nil { + return false, err + } + return !fi.Mode().IsRegular(), 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. +// Name reports the path currently being written: the temporary file for an +// ordinary destination, which is not the destination until Commit succeeds, or +// the destination itself for one being written through directly. func (o *OutputFile) Name() string { return o.f.Name() } // Commit closes the temporary file and moves it onto the destination path. @@ -83,6 +128,11 @@ func (o *OutputFile) Commit() error { return ErrOutputFileFinished } o.finished = true + + if o.direct { + return o.f.Close() + } + if err := o.f.Close(); err != nil { os.Remove(o.f.Name()) return err @@ -96,11 +146,17 @@ func (o *OutputFile) Commit() error { // 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. +// +// A destination being written through directly is only closed, never removed — +// the file is the caller's, and for /dev/null and friends removing it would do +// real damage. func (o *OutputFile) Cleanup() { if o.finished { return } o.finished = true o.f.Close() - os.Remove(o.f.Name()) + if !o.direct { + os.Remove(o.f.Name()) + } } diff --git a/otdfctl/pkg/streamio/output_unix_test.go b/otdfctl/pkg/streamio/output_unix_test.go new file mode 100644 index 0000000000..cf3a8a5a6b --- /dev/null +++ b/otdfctl/pkg/streamio/output_unix_test.go @@ -0,0 +1,103 @@ +//go:build !windows + +package streamio + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A rename cannot stand in for /dev/null, and replacing it would destroy the +// device node. `-o /dev/null` is a routine way to benchmark or smoke-test a +// decrypt, and worked before the switch to atomic output. +func TestOutputFileWritesThroughDevNull(t *testing.T) { + o, err := NewOutputFile(os.DevNull, testOutputFileMode) + require.NoError(t, err) + + _, err = o.Write([]byte("payload")) + require.NoError(t, err) + require.NoError(t, o.Commit()) + + fi, err := os.Stat(os.DevNull) + require.NoError(t, err) + assert.NotZero(t, fi.Mode()&os.ModeDevice, "%s must still be a device node", os.DevNull) +} + +// Cleanup removes the temp file it created; it must not remove a destination it +// was only writing through, which is not its to delete. +func TestOutputFileCleanupLeavesDirectDestination(t *testing.T) { + o, err := NewOutputFile(os.DevNull, testOutputFileMode) + require.NoError(t, err) + + _, err = o.Write([]byte("partial")) + require.NoError(t, err) + o.Cleanup() + + fi, err := os.Stat(os.DevNull) + require.NoError(t, err) + assert.NotZero(t, fi.Mode()&os.ModeDevice, "Cleanup must not remove a direct destination") +} + +// os.Stat follows symlinks, so a link to a regular file looks regular and the +// rename would replace the link rather than update its target. os.Lstat is what +// makes this land on the target, as os.Create did. +func TestOutputFileWritesThroughSymlink(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target.txt") + link := filepath.Join(dir, "link.txt") + + require.NoError(t, os.WriteFile(target, []byte("stale"), 0o600)) + require.NoError(t, os.Symlink(target, link)) + + o, err := NewOutputFile(link, testOutputFileMode) + require.NoError(t, err) + _, err = o.Write([]byte("payload")) + require.NoError(t, err) + require.NoError(t, o.Commit()) + + fi, err := os.Lstat(link) + require.NoError(t, err) + assert.NotZero(t, fi.Mode()&os.ModeSymlink, "the symlink must survive, not be replaced by a regular file") + + got, err := os.ReadFile(target) + require.NoError(t, err) + assert.Equal(t, "payload", string(got), "the write must land on the symlink's target") +} + +// A fifo has no seekable identity to rename over either, and opening one for +// writing blocks until a reader arrives — so this only checks the routing +// decision, not a full write. +func TestDirectDestinationDetection(t *testing.T) { + dir := t.TempDir() + + regular := filepath.Join(dir, "regular.txt") + require.NoError(t, os.WriteFile(regular, []byte("x"), 0o600)) + + for _, tc := range []struct { + name string + path string + want bool + }{ + {"missing path takes the atomic route", filepath.Join(dir, "nope.txt"), false}, + {"existing regular file takes the atomic route", regular, false}, + {"device node is written through", os.DevNull, true}, + {"directory is written through, and fails to open", dir, true}, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := isDirectDestination(tc.path) + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} + +// A directory reaches the direct path and then fails to open, which is a better +// outcome than os.CreateTemp succeeding inside it and the rename failing later. +func TestOutputFileRejectsDirectoryDestination(t *testing.T) { + _, err := NewOutputFile(t.TempDir(), testOutputFileMode) + require.Error(t, err) +} From 1e855a175cf9f9f9ffe9e8c6f6a59bc64d49bd88 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Mon, 14 Sep 2026 15:42:56 -0400 Subject: [PATCH 2/2] fix(cli): default the decrypt session key algorithm DecryptOptions replaced Decrypt's positional parameter list, which made the zero value reachable for the first time: every caller of DecryptBytes had been forced to pass a session key algorithm, but a struct literal can now omit one. Decrypt forwarded the field unconditionally, so an omitted algorithm reached sdk.WithSessionKeyType as the empty string, and ocrypto.NewKeyPair rejects it -- "newTDFReaderConfig failed: failed to create RSA key pair: unsupported key type:", raised while building the config, before the TDF is read at all. The option is now only appended when the field is set. The SDK already defaults kasSessionKey to RSA-2048 when the option is absent, which is the algorithm decryptRun asks for anyway, so the CLI path is unchanged; what changes is that DecryptOptions{} means "SDK default" rather than "empty algorithm", matching the KASAllowList field beside it. The CLI fills the field in on every path, so this is latent today -- it is the cost of the struct: a zero value that no positional signature could express is now constructible, and each field has to say what its zero value means. The same trap exists on the encrypt side, where an unset WrappingKeyAlgorithm fails with "key type missing"; that field predates this PR and is left alone here. Signed-off-by: Dave Mihalcik --- otdfctl/pkg/handlers/tdf.go | 11 ++++++++++- otdfctl/pkg/handlers/tdf_test.go | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/otdfctl/pkg/handlers/tdf.go b/otdfctl/pkg/handlers/tdf.go index 9786b09be9..0a2bb794c5 100644 --- a/otdfctl/pkg/handlers/tdf.go +++ b/otdfctl/pkg/handlers/tdf.go @@ -107,6 +107,10 @@ func (h Handler) Encrypt(ctx context.Context, out io.Writer, in io.ReadSeeker, o } // DecryptOptions carries the non-stream inputs to Decrypt. +// +// Every field is optional; the zero value means "use the SDK default". That +// holds for SessionKeyAlgorithm in particular: the empty string is not a valid +// ocrypto.KeyType, so it is left unset rather than forwarded. type DecryptOptions struct { AssertionVerificationKeysFile string DisableAssertionCheck bool @@ -136,10 +140,15 @@ func (h Handler) Decrypt(ctx context.Context, out io.Writer, in io.ReadSeeker, o case sdk.Standard: opts := []sdk.TDFReaderOption{ sdk.WithDisableAssertionVerification(o.DisableAssertionCheck), - sdk.WithSessionKeyType(o.SessionKeyAlgorithm), sdk.WithIgnoreAllowlist(o.IgnoreAllowlist), sdk.WithTDFFulfillableObligationFQNs(o.FulfillableObligations), } + // Forwarding an unset algorithm would fail in ocrypto.NewKeyPair before + // the TDF is read at all. Omitting the option instead lets the SDK apply + // its own RSA-2048 default, which is the algorithm the CLI asks for. + if o.SessionKeyAlgorithm != "" { + opts = append(opts, sdk.WithSessionKeyType(o.SessionKeyAlgorithm)) + } if o.KASAllowList != nil { opts = append(opts, sdk.WithKasAllowlist(o.KASAllowList)) } diff --git a/otdfctl/pkg/handlers/tdf_test.go b/otdfctl/pkg/handlers/tdf_test.go index 49eb9ba551..575b427e01 100644 --- a/otdfctl/pkg/handlers/tdf_test.go +++ b/otdfctl/pkg/handlers/tdf_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "testing" + "github.com/opentdf/platform/sdk" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -81,6 +82,23 @@ func TestDecryptAssertionVerificationKeyErrors(t *testing.T) { } } +// An unset SessionKeyAlgorithm must mean "let the SDK choose", not "use the +// empty algorithm". The empty string is not a valid ocrypto.KeyType, so passing +// it through to WithSessionKeyType fails config construction before the TDF is +// ever read — a trap the CLI only avoids because it happens to fill the field +// in. Reaching the zip reader (rather than a key-type error) is what proves the +// option was skipped. +func TestDecryptDefaultsSessionKeyAlgorithm(t *testing.T) { + var out bytes.Buffer + + h := Handler{sdk: &sdk.SDK{}} + err := h.Decrypt(t.Context(), &out, bytes.NewReader(zipPrefix), DecryptOptions{}) + + require.Error(t, err) + assert.NotContains(t, err.Error(), "unsupported key type", + "an unset session key algorithm must fall through to the SDK default") +} + func TestInspectTDFRejectsNonTDFInput(t *testing.T) { result, errs := Handler{}.InspectTDF(bytes.NewReader([]byte("not a tdf at all")))