From bde833193cc6babd99cda5104ea3beb361669af4 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, or next to the file it is a symbolic link to, and rename that over it only on success, with the new `secret.WriteFileAtomicFunc`. An output file that cannot be written still fails. Ctrl-C, SIGTERM or a hangup removes the temporary file. A device or pipe, such as `/dev/null`, is written to directly. Judgement calls: the device and pipe case; a command started with `nohup` still ignores a hangup. Model: opus-5-5 --- README.md | 13 ++ internal/cli/crypto.go | 155 +++++++++++++---- internal/cli/crypto_test.go | 304 +++++++++++++++++++++++++++++++++ internal/cli/entry_test.go | 114 +++++++++++++ internal/cli/leftovers_test.go | 8 +- internal/cli/root.go | 29 +++- internal/secret/atomic.go | 35 +++- 7 files changed, 604 insertions(+), 54 deletions(-) create mode 100644 internal/cli/crypto_test.go diff --git a/README.md b/README.md index dceb58b..02c8163 100644 --- a/README.md +++ b/README.md @@ -292,6 +292,19 @@ 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. When `--output` is a symbolic link, the file it points to is +replaced, or created if it does not exist yet, and the link kept; the new file +is then made next to that file. The directory the new file is made in must be +writable. The new file has mode 0600 and belongs to the user running the +command, whatever mode and owner the file it replaces had, so a hard link to the +old file keeps the old content. Ctrl-C, SIGTERM or a hangup removes the new +file; a command killed any other way, such as with SIGKILL, leaves it there, +named `.tmp-` and a number. A device or a pipe, such as `/dev/null`, is written +to directly. + ## Storage Architecture ### Directory Structure diff --git a/internal/cli/crypto.go b/internal/cli/crypto.go index b2112ee..617e3db 100644 --- a/internal/cli/crypto.go +++ b/internal/cli/crypto.go @@ -5,9 +5,12 @@ import ( "fmt" "io" "os" + "path/filepath" + "sync/atomic" "filippo.io/age" "github.com/awnumar/memguard" + "github.com/spf13/afero" "github.com/spf13/cobra" "sneak.berlin/go/secret/internal/secret" "sneak.berlin/go/secret/internal/vault" @@ -188,36 +191,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 +269,123 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error { input = file } - // Set up output writer - output := cli.cmd.OutOrStdout() + return cli.writeOutput(outputFile, func(output io.Writer) error { + decryptor, err := age.Decrypt(input, identity) + if err != nil { + return fmt.Errorf("failed to create age decryptor: %w", err) + } - if outputFile != "" { - file, err := cli.fs.Create(outputFile) + _, err = io.Copy(output, decryptor) + if err != nil { + return fmt.Errorf("failed to decrypt data: %w", err) + } + + return nil + }) +} + +// temporaryOutput is the name of the temporary file writeOutput is filling, +// until it is renamed into place. The signal handler in Entry removes it, +// since for decrypt it holds part of the secret. +// +//nolint:gochecknoglobals // read by the signal handler, which has no Instance +var temporaryOutput atomic.Pointer[string] + +// 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, +// or the file it is a symbolic link to, only once write has succeeded, so a +// failure leaves that file as it was, and it may be the input file too. +// An existing outputFile that cannot be written fails before write is +// called. +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 create output file: %w", err) } defer func() { _ = file.Close() }() - output = file + return write(file) } - // Decrypt the data - decryptor, err := age.Decrypt(input, identity) + // An existing file that cannot be written fails here, as writing into it + // directly would. Opened without O_TRUNC, which would empty it. + file, err := cli.fs.OpenFile(outputFile, os.O_WRONLY, 0) + if err == nil { + err = file.Close() + } + + if err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("failed to create output file: %w", err) + } + + path, err := outputPath(outputFile) if err != nil { - return fmt.Errorf("failed to create age decryptor: %w", err) + return fmt.Errorf("failed to create output file: %w", err) + } + + err = secret.WriteFileAtomicFunc(cli.fs, path, func(file afero.File) error { + name := file.Name() + temporaryOutput.Store(&name) + + return write(file) + }) + + temporaryOutput.Store(nil) + + return err +} + +// outputPath returns the path of the file that writing to outputFile +// replaces, with no symbolic link in it, so that filepath.Dir of it is the +// directory that file is in. When outputFile is a symbolic link, as +// /dev/stdout is when stdout goes to a file, that is the path the link +// points to, whether or not a file is there yet, so the link is kept, as +// writing through it would keep it. A link that cannot be followed, such as +// one that points to itself, is an error. +func outputPath(outputFile string) (string, error) { + path, err := filepath.EvalSymlinks(outputFile) + if !errors.Is(err, os.ErrNotExist) { + return path, err + } + + // No file is at outputFile, or at the end of its links, yet + target, err := os.Readlink(outputFile) + if errors.Is(err, os.ErrNotExist) { + // Nothing is at outputFile itself. Its directory part is resolved + // with filepath.EvalSymlinks, since filepath.Dir would remove a link + // to a directory followed by ".." as text and give another directory. + dir, name := filepath.Split(outputFile) + + dir, err = filepath.EvalSymlinks(dir) + if err != nil { + return "", err + } + + return filepath.Join(dir, name), nil } - _, err = io.Copy(output, decryptor) if err != nil { - return fmt.Errorf("failed to decrypt data: %w", err) + return "", err } - return nil + if !filepath.IsAbs(target) { + // Not filepath.Join, which would remove a ".." in target together + // with the name before it, even when that name is a link to a + // directory + dir, _ := filepath.Split(outputFile) + target = dir + target + } + + return outputPath(target) } // 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..d519f37 --- /dev/null +++ b/internal/cli/crypto_test.go @@ -0,0 +1,304 @@ +//nolint:testpackage // sets the unexported fields of Instance +package cli + +import ( + "io" + "os" + "path/filepath" + "strings" + "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, +// 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)) + + // 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) + }) + } +} + +// writeFailFs fails every open of one path for writing, as a file of mode +// 0444 does for anyone but root, who runs these tests in Docker. +type writeFailFs struct { + afero.Fs + + path string +} + +//nolint:ireturn // afero.File is the interface required by afero.Fs +func (f *writeFailFs) OpenFile( + name string, flag int, perm os.FileMode, +) (afero.File, error) { + if name == f.path && flag&(os.O_WRONLY|os.O_RDWR) != 0 { + return nil, &os.PathError{Op: "open", Path: name, Err: syscall.EACCES} + } + + return f.Fs.OpenFile(name, flag, perm) +} + +// TestDecryptToUnwritableOutput checks that secret decrypt fails when +// --output is a file that cannot be written, as writing into it directly +// does, and leaves it as it was, although its directory can be written. +func TestDecryptToUnwritableOutput(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.fs = &writeFailFs{Fs: cli.fs, path: existing} + + err := cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), existing) + require.ErrorIs(t, err, os.ErrPermission) + + content, err := afero.ReadFile(cli.fs, existing) + require.NoError(t, err) + require.Equal(t, "precious", string(content)) +} + +// 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)) +} + +// TestDecryptThroughLink checks that secret decrypt given a symbolic link +// as --output replaces the file the link points to, in another directory, +// and keeps the link, as writing through the link would. +func TestDecryptThroughLink(t *testing.T) { + t.Parallel() + + cli, dir := newCryptoTest(t) + targetDir := t.TempDir() + target := filepath.Join(targetDir, "target.txt") + require.NoError(t, afero.WriteFile(cli.fs, target, []byte("old"), 0o600)) + + link := filepath.Join(dir, "link") + require.NoError(t, os.Symlink(target, link)) + + require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), link)) + + info, err := os.Lstat(link) + require.NoError(t, err) + require.Equal(t, os.ModeSymlink, info.Mode().Type()) + + content, err := afero.ReadFile(cli.fs, target) + require.NoError(t, err) + require.Equal(t, plainText, string(content)) + + // No temporary file left beside the target + entries, err := afero.ReadDir(cli.fs, targetDir) + require.NoError(t, err) + require.Len(t, entries, 1) +} + +// TestDecryptThroughLinkToMissingFile checks that secret decrypt given a +// symbolic link to a file that does not exist yet as --output creates that +// file and keeps the link, as writing through the link would. Here --output +// is a relative link to a second link, which points into another directory. +func TestDecryptThroughLinkToMissingFile(t *testing.T) { + t.Parallel() + + cli, dir := newCryptoTest(t) + targetDir := t.TempDir() + target := filepath.Join(targetDir, "target.txt") + + link := filepath.Join(dir, "link") + require.NoError(t, os.Symlink("second-link", link)) + require.NoError(t, os.Symlink(target, filepath.Join(dir, "second-link"))) + + require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), link)) + + for _, name := range []string{"link", "second-link"} { + info, err := os.Lstat(filepath.Join(dir, name)) + require.NoError(t, err) + require.Equal(t, os.ModeSymlink, info.Mode().Type(), name) + } + + content, err := afero.ReadFile(cli.fs, target) + require.NoError(t, err) + require.Equal(t, plainText, string(content)) + + // No temporary file left beside the target + entries, err := afero.ReadDir(cli.fs, targetDir) + require.NoError(t, err) + require.Len(t, entries, 1) +} + +// TestDecryptThroughLinkAndParent checks that secret decrypt makes the file +// --output names in the directory its path leads to when the path holds a +// symbolic link to a directory followed by "..": dir/link/../sub/new.txt is +// targetDir/sub/new.txt, and dir/sub, where removing "link/.." as text +// leads, does not exist. +func TestDecryptThroughLinkAndParent(t *testing.T) { + t.Parallel() + + cli, dir := newCryptoTest(t) + targetDir := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(targetDir, "linked"), 0o700)) + require.NoError(t, os.Mkdir(filepath.Join(targetDir, "sub"), 0o700)) + require.NoError(t, os.Symlink(filepath.Join(targetDir, "linked"), + filepath.Join(dir, "link"))) + + // Not filepath.Join, which would remove "link/.." too + output := dir + "/link/../sub/new.txt" + + require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), output)) + + content, err := afero.ReadFile(cli.fs, filepath.Join(targetDir, "sub", "new.txt")) + require.NoError(t, err) + require.Equal(t, plainText, string(content)) +} + +// TestDecryptThroughLoopingLink checks that secret decrypt given a symbolic +// link that points to itself as --output fails, as writing through it +// would, and leaves the link as it was. +func TestDecryptThroughLoopingLink(t *testing.T) { + t.Parallel() + + cli, dir := newCryptoTest(t) + link := filepath.Join(dir, "link") + require.NoError(t, os.Symlink("link", link)) + + require.Error(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), link)) + + target, err := os.Readlink(link) + require.NoError(t, err) + require.Equal(t, "link", target) +} + +// TestDecryptToLongestName checks that secret decrypt writes an --output +// whose name is as long as a file name can be, 255 bytes on Linux and +// macOS: the temporary file's name must not be longer. +func TestDecryptToLongestName(t *testing.T) { + t.Parallel() + + const longestName = 255 + + cli, dir := newCryptoTest(t) + output := filepath.Join(dir, strings.Repeat("a", longestName)) + + require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), output)) + + content, err := afero.ReadFile(cli.fs, output) + require.NoError(t, err) + require.Equal(t, plainText, string(content)) +} diff --git a/internal/cli/entry_test.go b/internal/cli/entry_test.go index 3889936..1097502 100644 --- a/internal/cli/entry_test.go +++ b/internal/cli/entry_test.go @@ -7,7 +7,9 @@ import ( "os/exec" "path/filepath" "strings" + "syscall" "testing" + "time" "github.com/awnumar/memguard" "github.com/stretchr/testify/assert" @@ -105,3 +107,115 @@ func TestInterruptExitsThroughMemguard(t *testing.T) { require.ErrorAs(t, err, &exitErr) assert.Equal(t, 1, exitErr.ExitCode(), "add ended with %v", err) } + +// Ctrl-C or a hangup while encrypt or decrypt waits for input must remove +// the temporary file it is writing next to --output, which for decrypt +// holds part of the secret. +func TestInterruptRemovesTemporaryOutput(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(t.Context(), commandWait) + defer cancel() + + secretPath := secretBinaryPath(t) + env := []string{ + secret.EnvStateDir + "=" + t.TempDir(), + secret.EnvMnemonic + "=" + testMnemonic, + secret.EnvUnlockPassphrase + "=test-passphrase", + "PATH=" + os.Getenv("PATH"), + } + + //nolint:gosec // G204: test executes the freshly built secret binary + initCmd := exec.CommandContext(ctx, secretPath, "init") + initCmd.Env = env + + output, err := initCmd.CombinedOutput() + require.NoError(t, err, "init should succeed: %s", output) + + // encrypt stores the new key before it starts writing, so decrypt has it + for _, command := range []string{"encrypt", "decrypt"} { + for _, sig := range []os.Signal{os.Interrupt, syscall.SIGHUP} { + outputDir := t.TempDir() + + //nolint:gosec // G204: test executes the freshly built secret binary + cmd := exec.CommandContext(ctx, secretPath, command, "key", + "--output", filepath.Join(outputDir, "out")) + cmd.Env = env + + // Held open and never written, so the command keeps waiting for + // input. Wait closes it. + _, err := cmd.StdinPipe() + require.NoError(t, err) + require.NoError(t, cmd.Start()) + + require.Eventually(t, func() bool { + entries, err := os.ReadDir(outputDir) + + return err == nil && len(entries) > 0 + }, commandWait, 10*time.Millisecond, "%s made no temporary file", command) + require.NoError(t, cmd.Process.Signal(sig)) + + var exitErr *exec.ExitError + + require.ErrorAs(t, cmd.Wait(), &exitErr) + + entries, err := os.ReadDir(outputDir) + require.NoError(t, err) + assert.Empty(t, entries, "%s left its temporary file on %v", command, sig) + } + } +} + +// encrypt started with nohup must go on ignoring a hangup, which it catches +// otherwise, and finish writing --output. +func TestNohupIgnoresHangup(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(t.Context(), commandWait) + defer cancel() + + secretPath := secretBinaryPath(t) + env := []string{ + secret.EnvStateDir + "=" + t.TempDir(), + secret.EnvMnemonic + "=" + testMnemonic, + secret.EnvUnlockPassphrase + "=test-passphrase", + "PATH=" + os.Getenv("PATH"), + } + + //nolint:gosec // G204: test executes the freshly built secret binary + initCmd := exec.CommandContext(ctx, secretPath, "init") + initCmd.Env = env + + output, err := initCmd.CombinedOutput() + require.NoError(t, err, "init should succeed: %s", output) + + outputDir := t.TempDir() + outputFile := filepath.Join(outputDir, "out") + + //nolint:gosec // G204: test executes the freshly built secret binary + cmd := exec.CommandContext(ctx, "nohup", secretPath, "encrypt", "key", + "--output", outputFile) + cmd.Env = env + + stdin, err := cmd.StdinPipe() + require.NoError(t, err) + require.NoError(t, cmd.Start()) + + require.Eventually(t, func() bool { + entries, err := os.ReadDir(outputDir) + + return err == nil && len(entries) > 0 + }, commandWait, 10*time.Millisecond, "encrypt made no temporary file") + require.NoError(t, cmd.Process.Signal(syscall.SIGHUP)) + + // Time for a hangup that is caught to end encrypt before it has its input + time.Sleep(500 * time.Millisecond) + + _, err = stdin.Write([]byte("document")) + require.NoError(t, err) + require.NoError(t, stdin.Close()) + require.NoError(t, cmd.Wait()) + + _, err = os.Stat(outputFile) + require.NoError(t, err) +} diff --git a/internal/cli/leftovers_test.go b/internal/cli/leftovers_test.go index e035b6b..15e985d 100644 --- a/internal/cli/leftovers_test.go +++ b/internal/cli/leftovers_test.go @@ -52,10 +52,10 @@ func TestLeftoversRemovedByNextChangingCommand(t *testing.T) { } for _, file := range []string{ - testStateDir + "/.currentvault.tmp-4", - vaultDir + "/.current-unlocker.tmp-5", - secretDir + "/.current.tmp-6", - secretDir + "/versions/" + versions[0] + "/.metadata.age.tmp-7", + testStateDir + "/.tmp-4", + vaultDir + "/.tmp-5", + secretDir + "/.tmp-6", + secretDir + "/versions/" + versions[0] + "/.tmp-7", } { require.NoError(t, afero.WriteFile(fs, file, []byte("partial"), secret.FilePerms)) diff --git a/internal/cli/root.go b/internal/cli/root.go index 2921c8c..67c56d3 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -2,6 +2,7 @@ package cli import ( "os" + "os/signal" "github.com/awnumar/memguard" "github.com/spf13/cobra" @@ -14,12 +15,13 @@ import ( // every memguard buffer before it returns, so the caller must do nothing // but exit with the code. func Entry() int { - // On SIGINT or SIGTERM memguard runs this function, wipes every buffer - // and exits with status 1. The passphrase prompt turns echo off on the - // controlling terminal until the read finishes, so a signal there would - // leave echo off. Only a process in the terminal's foreground process - // group may reset it: one in the background that tries is stopped - // instead of exiting. + // On SIGINT, SIGTERM or SIGHUP memguard runs this function, wipes every + // buffer and exits with status 1. The function removes the temporary + // file that encrypt or decrypt is writing. The passphrase prompt turns + // echo off on the controlling terminal until the read finishes, so a + // signal there would leave echo off. Only a process in the terminal's + // foreground process group may reset it: one in the background that + // tries is stopped instead of exiting. terminal, terminalErr := os.Open(secret.TerminalPath) var terminalState *term.State @@ -30,7 +32,20 @@ func Entry() int { terminalState, terminalErr = term.GetState(int(terminal.Fd())) } + signals := []os.Signal{os.Interrupt, unix.SIGTERM} + + // A command started with nohup must go on ignoring SIGHUP, which + // catching it would undo + if !signal.Ignored(unix.SIGHUP) { + signals = append(signals, unix.SIGHUP) + } + memguard.CatchSignal(func(os.Signal) { + name := temporaryOutput.Load() + if name != nil { + _ = os.Remove(*name) + } + if terminalErr != nil { return } @@ -39,7 +54,7 @@ func Entry() int { if err == nil && foreground == unix.Getpgrp() { _ = term.Restore(int(terminal.Fd()), terminalState) } - }, os.Interrupt, unix.SIGTERM) + }, signals...) defer memguard.Purge() diff --git a/internal/secret/atomic.go b/internal/secret/atomic.go index 5c2f6f9..93bd27a 100644 --- a/internal/secret/atomic.go +++ b/internal/secret/atomic.go @@ -10,8 +10,8 @@ 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 starts the name of every temporary file WriteFileAtomicFunc +// makes 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,17 +21,38 @@ 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 { - tmp, err := afero.TempFile(fs, filepath.Dir(path), - "."+filepath.Base(path)+tempNamePart+"*") + return WriteFileAtomicFunc(fs, path, func(file afero.File) error { + _, err := file.Write(data) + if err != nil { + return fmt.Errorf("failed to write %s: %w", path, err) + } + + return nil + }) +} + +// WriteFileAtomicFunc is WriteFileAtomic for content that write streams +// into the temporary file, which it is given, instead of holding it in +// memory. An error from write is returned as it is. The temporary file's +// name leaves out path's, which may already be as long as a name can be. +func WriteFileAtomicFunc( + fs afero.Fs, path string, write func(file afero.File) error, +) error { + tmp, err := afero.TempFile(fs, filepath.Dir(path), tempNamePart) if err != nil { return fmt.Errorf("failed to create temporary file for %s: %w", path, err) } - _, err = tmp.Write(data) - if err == nil { - err = tmp.Sync() + err = write(tmp) + if err != nil { + _ = tmp.Close() + _ = fs.Remove(tmp.Name()) + + return err } + err = tmp.Sync() + closeErr := tmp.Close() if err == nil { err = closeErr