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..0a2bb794c5 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,86 @@ 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. +// +// 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 + 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.WithIgnoreAllowlist(o.IgnoreAllowlist), + sdk.WithTDFFulfillableObligationFQNs(o.FulfillableObligations), } - if kasAllowList != nil { - opts = append(opts, sdk.WithKasAllowlist(kasAllowList)) + // 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)) } 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..575b427e01 --- /dev/null +++ b/otdfctl/pkg/handlers/tdf_test.go @@ -0,0 +1,108 @@ +package handlers + +import ( + "bytes" + "os" + "path/filepath" + "testing" + + "github.com/opentdf/platform/sdk" + "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") + }) + } +} + +// 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"))) + + 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) +}