From eef05ee42903847533e3ddddb08e5fe13543f853 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 14 Oct 2026 23:38:46 +0000 Subject: [PATCH] Hold the encrypt and decrypt result in memory until it is written (closes #143) With --output, secret encrypt and secret decrypt emptied that file before reading any input, so a failed command left it empty, and encrypting a file onto itself gave the encryption of nothing. With --output they now hold the result in memory and open --output, with the same call as before, only once encryption or decryption has succeeded. A failure to encrypt or decrypt leaves --output untouched, and --input can be the same file. A failure while writing --output, such as a full disk, can leave it empty or partly written; an error from closing it is reported. Without --output the result still goes to stdout as it is produced. Model: opus-5-5 --- README.md | 6 ++ internal/cli/crypto.go | 69 ++++++++++---- internal/cli/crypto_test.go | 185 ++++++++++++++++++++++++++++++++++++ 3 files changed, 241 insertions(+), 19 deletions(-) create mode 100644 internal/cli/crypto_test.go diff --git a/README.md b/README.md index a62d1db..0167115 100644 --- a/README.md +++ b/README.md @@ -314,6 +314,12 @@ name. The file's bytes are stored unchanged, a final newline included. ### Encryption Operations +With `--output`, `secret encrypt` and `secret decrypt` hold the result in memory +and write it to `--output` only once encryption or decryption has succeeded, so +a failure to encrypt or decrypt leaves `--output` untouched. A failure while +writing `--output`, such as a full disk, can leave it empty or partly written. +The command needs about as much memory as the result. + #### `secret encrypt [--input=file] [--output=file]` Encrypts data using an Age key stored as a secret. If the secret doesn't exist, diff --git a/internal/cli/crypto.go b/internal/cli/crypto.go index b2112ee..11e6be9 100644 --- a/internal/cli/crypto.go +++ b/internal/cli/crypto.go @@ -1,6 +1,7 @@ package cli import ( + "bytes" "errors" "fmt" "io" @@ -107,8 +108,8 @@ func (cli *Instance) storeNewEncryptionKey( // does not exist. The caller must destroy the returned buffer. Only storing // a new key takes the state directory lock, so that reading an existing key // works on a read-only state directory and keeps no other command waiting -// at the passphrase prompt, and Encrypt streams its input and output -// unlocked. +// at the passphrase prompt, and Encrypt reads its input and writes its +// output without the lock. func (cli *Instance) resolveEncryptionKey( vlt *vault.Vault, secretName string, ) (*memguard.LockedBuffer, error) { @@ -188,17 +189,17 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error { input = file } - // Set up output writer + // Set up output writer. With --output the result is held in memory + // and written only once encryption has succeeded, so a failure to + // encrypt leaves --output untouched and --input can be the same file. + // A failure while writing --output, such as a full disk, can leave it + // empty or partly written. output := cli.cmd.OutOrStdout() - if outputFile != "" { - file, err := cli.fs.Create(outputFile) - if err != nil { - return fmt.Errorf("failed to create output file: %w", err) - } - defer func() { _ = file.Close() }() + var result bytes.Buffer - output = file + if outputFile != "" { + output = &result } // Encrypt the data @@ -217,7 +218,11 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error { return fmt.Errorf("failed to finalize encryption: %w", err) } - return nil + if outputFile == "" { + return nil + } + + return cli.writeOutputFile(outputFile, result.Bytes()) } // Decrypt decrypts data using an age secret key stored in a secret @@ -278,17 +283,17 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error { input = file } - // Set up output writer + // Set up output writer. With --output the result is held in memory + // and written only once decryption has succeeded, so a failure to + // decrypt leaves --output untouched and --input can be the same file. + // A failure while writing --output, such as a full disk, can leave it + // empty or partly written. output := cli.cmd.OutOrStdout() - if outputFile != "" { - file, err := cli.fs.Create(outputFile) - if err != nil { - return fmt.Errorf("failed to create output file: %w", err) - } - defer func() { _ = file.Close() }() + var result bytes.Buffer - output = file + if outputFile != "" { + output = &result } // Decrypt the data @@ -302,6 +307,32 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error { return fmt.Errorf("failed to decrypt data: %w", err) } + if outputFile == "" { + return nil + } + + return cli.writeOutputFile(outputFile, result.Bytes()) +} + +// writeOutputFile creates outputFile, or empties it if it exists, and +// writes result into it. +func (cli *Instance) writeOutputFile(outputFile string, result []byte) error { + file, err := cli.fs.Create(outputFile) + if err != nil { + return fmt.Errorf("failed to create output file: %w", err) + } + + _, err = file.Write(result) + + closeErr := file.Close() + if err == nil { + err = closeErr + } + + if err != nil { + return fmt.Errorf("failed to write output file: %w", err) + } + return nil } diff --git a/internal/cli/crypto_test.go b/internal/cli/crypto_test.go new file mode 100644 index 0000000..6cbb98b --- /dev/null +++ b/internal/cli/crypto_test.go @@ -0,0 +1,185 @@ +//nolint:testpackage // sets the unexported fields of Instance +package cli + +import ( + "errors" + "fmt" + "io" + "os" + "path/filepath" + "testing" + + "github.com/spf13/afero" + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" + "sneak.berlin/go/secret/internal/vault" +) + +// plainText is the content of plain.txt in the directory newCryptoTest makes. +const plainText = "document" + +// newCryptoTest returns an Instance whose current vault, in a new state +// directory on the real filesystem, holds the age key secret "key", and a +// new directory holding only plain.txt and plain.txt.age, its encryption +// with that key. +func newCryptoTest(t *testing.T) (*Instance, string) { + t.Helper() + + fs := afero.NewOsFs() + stateDir := t.TempDir() + + _, err := vault.CreateVault(fs, stateDir, "default", testMnemonicBuffer(t), nil) + require.NoError(t, err) + + cli := NewCLIInstanceWithStateDir(fs, stateDir) + cli.Mnemonic = testMnemonicBuffer(t) + cli.cmd = &cobra.Command{} + + dir := t.TempDir() + plain := filepath.Join(dir, "plain.txt") + require.NoError(t, afero.WriteFile(fs, plain, []byte(plainText), 0o600)) + require.NoError(t, cli.Encrypt("key", plain, plain+".age")) + + return cli, dir +} + +// TestFailedCryptoLeavesOutputUnchanged is a regression test for +// https://git.eeqj.de/sneak/secret/issues/143, where secret encrypt and +// secret decrypt emptied the file named by --output before they failed. +// A failure to encrypt or decrypt must leave an existing file as it was and +// create none, and give the same error as without --output. +func TestFailedCryptoLeavesOutputUnchanged(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + run func(cli *Instance, dir, output string) error + }{ + // Reading a directory fails once encryption has begun + {"encrypt", func(cli *Instance, dir, output string) error { + return cli.Encrypt("key", dir, output) + }}, + // plain.txt is not an age file + {"decrypt", func(cli *Instance, dir, output string) error { + return cli.Decrypt("key", filepath.Join(dir, "plain.txt"), output) + }}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + cli, dir := newCryptoTest(t) + existing := filepath.Join(dir, "existing.txt") + require.NoError(t, afero.WriteFile(cli.fs, existing, []byte("precious"), 0o600)) + + cli.cmd.SetOut(io.Discard) + toStdout := tt.run(cli, dir, "") + require.Error(t, toStdout) + + require.EqualError(t, tt.run(cli, dir, existing), toStdout.Error()) + require.EqualError(t, tt.run(cli, dir, filepath.Join(dir, "new.txt")), + toStdout.Error()) + + content, err := afero.ReadFile(cli.fs, existing) + require.NoError(t, err) + require.Equal(t, "precious", string(content)) + + exists, err := afero.Exists(cli.fs, filepath.Join(dir, "new.txt")) + require.NoError(t, err) + require.False(t, exists, "a failed %s created --output", tt.name) + }) + } +} + +// TestCryptoInPlace checks that secret encrypt and secret decrypt given +// one file as both --input and --output replace it with the right result. +// Before https://git.eeqj.de/sneak/secret/issues/143 they emptied the file +// before reading it. +func TestCryptoInPlace(t *testing.T) { + t.Parallel() + + cli, dir := newCryptoTest(t) + file := filepath.Join(dir, "plain.txt") + + require.NoError(t, cli.Encrypt("key", file, file)) + require.NoError(t, cli.Decrypt("key", file, file)) + + content, err := afero.ReadFile(cli.fs, file) + require.NoError(t, err) + require.Equal(t, plainText, string(content)) +} + +// errCloseFailed is returned by the files closeFailFs creates in place of +// a successful Close, as NFS reports a full disk only when the file is +// closed. +var errCloseFailed = errors.New("no space left on device") + +// closeFailFs creates files whose Close fails with errCloseFailed. +type closeFailFs struct { + afero.Fs +} + +// closeFailFile is a file created through closeFailFs. +type closeFailFile struct { + afero.File +} + +func (f closeFailFile) Close() error { + _ = f.File.Close() + + return errCloseFailed +} + +//nolint:ireturn // afero.File is the interface required by afero.Fs +func (f closeFailFs) Create(name string) (afero.File, error) { + file, err := f.Fs.Create(name) + if err != nil { + return nil, err + } + + return closeFailFile{File: file}, nil +} + +// TestDecryptFailsWhenClosingOutputFails checks that secret decrypt fails +// when closing --output fails, since some filesystems report a failed +// write only then. +func TestDecryptFailsWhenClosingOutputFails(t *testing.T) { + t.Parallel() + + cli, dir := newCryptoTest(t) + cli.fs = closeFailFs{Fs: cli.fs} + + err := cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), + filepath.Join(dir, "out.txt")) + require.ErrorIs(t, err, errCloseFailed) +} + +// TestDecryptToDescriptorOfDeletedFile checks that secret decrypt given +// /dev/fd/N as --output writes through that descriptor. A program may give +// the command a temporary file it has already deleted as stdout, pass +// /dev/stdout as --output and read the result back from that file. +func TestDecryptToDescriptorOfDeletedFile(t *testing.T) { + t.Parallel() + + cli, dir := newCryptoTest(t) + + file, err := os.CreateTemp(t.TempDir(), "out") + require.NoError(t, err) + + defer func() { _ = file.Close() }() + + require.NoError(t, os.Remove(file.Name())) + + output := fmt.Sprintf("/dev/fd/%d", file.Fd()) + require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), output)) + + // On macOS the command's open of /dev/fd/N shares this descriptor's + // position, which its write leaves at the end. + _, err = file.Seek(0, io.SeekStart) + require.NoError(t, err) + + content, err := io.ReadAll(file) + require.NoError(t, err) + require.Equal(t, plainText, string(content)) +}