From 90bfbec5cb275a3365e5f663db471fd971450a90 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 14 Oct 2026 17:56:51 +0000 Subject: [PATCH] Write encrypt and decrypt output only once they succeed (closes #143) `secret encrypt` and `secret decrypt` emptied the file named by `--output` before reading any input, so a failure left it empty, and a file given as both `--input` and `--output` was emptied before it was read. They now write into a temporary file of mode 0600 next to it and rename that over it only on success, using the new `secret.WriteFileAtomicFunc`, which `secret.WriteFileAtomic` now calls. A device or pipe given as `--output`, such as `/dev/null`, is written to directly instead of being replaced by a file. Judgement call: the device and pipe case goes beyond the plan. Judgement call: a symbolic link given as `--output` is replaced, not followed. Model: opus-5-5 --- README.md | 7 ++ TODO.md | 9 +++ internal/cli/crypto.go | 90 ++++++++++++------------ internal/cli/crypto_test.go | 135 ++++++++++++++++++++++++++++++++++++ internal/secret/atomic.go | 21 +++++- 5 files changed, 215 insertions(+), 47 deletions(-) create mode 100644 internal/cli/crypto_test.go diff --git a/README.md b/README.md index 0bb262a..112810f 100644 --- a/README.md +++ b/README.md @@ -274,6 +274,13 @@ generates a new Age key. Decrypts data using an Age key stored as a secret. +Both write the result to a new file next to `--output` and rename it over +`--output` only once they have succeeded, so a failed command leaves an existing +`--output` as it was and creates none, and `--input` and `--output` can be the +same file. The file has mode 0600, whatever mode the file it replaced had, and +when `--output` is a symbolic link to a file, the link itself is replaced. A +device or a pipe, such as `/dev/null`, is written to directly. + ## Storage Architecture ### Directory Structure diff --git a/TODO.md b/TODO.md index 54fcc47..5e2d565 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,15 @@ https://git.eeqj.de/sneak/secret/milestone/12 # Completed Steps +- 2026-10-14: A failed `secret encrypt` or `secret decrypt` leaves the file + named by `--output` as it was, and the same file can be given as `--input` and + `--output` (https://git.eeqj.de/sneak/secret/issues/143). Both emptied the + output file before reading any input, so a wrong key left it empty, and a file + given as both was emptied before it was read. They now write the result into a + temporary file next to it and rename that over it once they have succeeded, + with the new `secret.WriteFileAtomicFunc`, which `secret.WriteFileAtomic` now + calls; the output file has mode 0600. A device or a pipe given as `--output`, + such as `/dev/null`, is still written to directly. - 2026-10-12: A mnemonic typed with extra spaces, tabs or line breaks gives the same long-term key as its words separated by single spaces (https://git.eeqj.de/sneak/secret/issues/135). The key was derived from the diff --git a/internal/cli/crypto.go b/internal/cli/crypto.go index b2112ee..6a4732d 100644 --- a/internal/cli/crypto.go +++ b/internal/cli/crypto.go @@ -188,36 +188,24 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error { input = file } - // Set up output writer - output := cli.cmd.OutOrStdout() - - if outputFile != "" { - file, err := cli.fs.Create(outputFile) + return cli.writeOutput(outputFile, func(output io.Writer) error { + encryptor, err := age.Encrypt(output, recipient) if err != nil { - return fmt.Errorf("failed to create output file: %w", err) + return fmt.Errorf("failed to create age encryptor: %w", err) } - defer func() { _ = file.Close() }() - output = file - } + _, err = io.Copy(encryptor, input) + if err != nil { + return fmt.Errorf("failed to encrypt data: %w", err) + } - // Encrypt the data - encryptor, err := age.Encrypt(output, recipient) - if err != nil { - return fmt.Errorf("failed to create age encryptor: %w", err) - } + err = encryptor.Close() + if err != nil { + return fmt.Errorf("failed to finalize encryption: %w", err) + } - _, err = io.Copy(encryptor, input) - if err != nil { - return fmt.Errorf("failed to encrypt data: %w", err) - } - - err = encryptor.Close() - if err != nil { - return fmt.Errorf("failed to finalize encryption: %w", err) - } - - return nil + return nil + }) } // Decrypt decrypts data using an age secret key stored in a secret @@ -278,31 +266,45 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error { input = file } - // Set up output writer - output := cli.cmd.OutOrStdout() - - if outputFile != "" { - file, err := cli.fs.Create(outputFile) + return cli.writeOutput(outputFile, func(output io.Writer) error { + decryptor, err := age.Decrypt(input, identity) if err != nil { - return fmt.Errorf("failed to create output file: %w", err) + return fmt.Errorf("failed to create age decryptor: %w", err) + } + + _, err = io.Copy(output, decryptor) + if err != nil { + return fmt.Errorf("failed to decrypt data: %w", err) + } + + return nil + }) +} + +// writeOutput calls write with stdout when outputFile is empty, and with +// outputFile itself when it is a device or a pipe, such as /dev/null. +// Otherwise write fills a new file of mode 0600 that replaces outputFile +// only once write has succeeded, so a failure leaves outputFile as it was, +// and outputFile may be the input file too. +func (cli *Instance) writeOutput( + outputFile string, write func(output io.Writer) error, +) error { + if outputFile == "" { + return write(cli.cmd.OutOrStdout()) + } + + info, err := cli.fs.Stat(outputFile) + if err == nil && !info.Mode().IsRegular() { + file, err := cli.fs.OpenFile(outputFile, os.O_WRONLY, 0) + if err != nil { + return fmt.Errorf("failed to open output file: %w", err) } defer func() { _ = file.Close() }() - output = file + return write(file) } - // Decrypt the data - decryptor, err := age.Decrypt(input, identity) - if err != nil { - return fmt.Errorf("failed to create age decryptor: %w", err) - } - - _, err = io.Copy(output, decryptor) - if err != nil { - return fmt.Errorf("failed to decrypt data: %w", err) - } - - return nil + return secret.WriteFileAtomicFunc(cli.fs, outputFile, write) } // isValidAgeSecretKey checks if a string is a valid age secret key by diff --git a/internal/cli/crypto_test.go b/internal/cli/crypto_test.go new file mode 100644 index 0000000..b208231 --- /dev/null +++ b/internal/cli/crypto_test.go @@ -0,0 +1,135 @@ +//nolint:testpackage // sets the unexported fields of Instance +package cli + +import ( + "os" + "path/filepath" + "syscall" + "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 failed command must leave an existing file as it was and create none. +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)) + + require.Error(t, tt.run(cli, dir, existing)) + require.Error(t, tt.run(cli, dir, filepath.Join(dir, "new.txt"))) + + content, err := afero.ReadFile(cli.fs, existing) + require.NoError(t, err) + require.Equal(t, "precious", string(content)) + + // No new.txt, and no temporary file left behind + entries, err := afero.ReadDir(cli.fs, dir) + require.NoError(t, err) + + names := make([]string, 0, len(entries)) + for _, entry := range entries { + names = append(names, entry.Name()) + } + + require.Equal(t, []string{"existing.txt", "plain.txt", "plain.txt.age"}, names) + }) + } +} + +// 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)) +} + +// TestDecryptToPipe checks that secret decrypt writes into a named pipe +// given as --output, as it must into a device such as /dev/null, instead +// of replacing it with a file. +func TestDecryptToPipe(t *testing.T) { + t.Parallel() + + cli, dir := newCryptoTest(t) + pipe := filepath.Join(dir, "pipe") + require.NoError(t, syscall.Mkfifo(pipe, 0o600)) + + received := make(chan []byte, 1) + + go func() { + content, _ := afero.ReadFile(cli.fs, pipe) + received <- content + }() + + require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), pipe)) + + info, err := os.Lstat(pipe) + require.NoError(t, err) + require.Equal(t, os.ModeNamedPipe, info.Mode().Type()) + require.Equal(t, plainText, string(<-received)) +} diff --git a/internal/secret/atomic.go b/internal/secret/atomic.go index 5c2f6f9..5fa5b1c 100644 --- a/internal/secret/atomic.go +++ b/internal/secret/atomic.go @@ -3,6 +3,7 @@ package secret import ( "errors" "fmt" + "io" "os" "path/filepath" "strings" @@ -10,8 +11,9 @@ import ( "github.com/spf13/afero" ) -// tempNamePart is in the name of every temporary file WriteFileAtomic makes, -// ".NAME.tmp-123", and every temporary directory TempDirFor makes, ".tmp-123". +// tempNamePart is in the name of every temporary file WriteFileAtomicFunc +// makes, ".NAME.tmp-123", and every temporary directory TempDirFor makes, +// ".tmp-123". const tempNamePart = ".tmp-" // WriteFileAtomic replaces the file at path with data so that a reader, or @@ -21,13 +23,26 @@ const tempNamePart = ".tmp-" // within one filesystem), is synced to disk, and is renamed over path. The // temporary file is removed if any step fails. func WriteFileAtomic(fs afero.Fs, path string, data []byte) error { + return WriteFileAtomicFunc(fs, path, func(w io.Writer) error { + _, err := w.Write(data) + + return err + }) +} + +// WriteFileAtomicFunc is WriteFileAtomic for content that write streams +// into the temporary file instead of holding it in memory. An error from +// write fails it as any other step does. +func WriteFileAtomicFunc( + fs afero.Fs, path string, write func(w io.Writer) error, +) error { tmp, err := afero.TempFile(fs, filepath.Dir(path), "."+filepath.Base(path)+tempNamePart+"*") if err != nil { return fmt.Errorf("failed to create temporary file for %s: %w", path, err) } - _, err = tmp.Write(data) + err = write(tmp) if err == nil { err = tmp.Sync() }