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

@@ -313,6 +313,11 @@ generates a new Age key.
 
 Decrypts data using an Age key stored as a secret.
 
+With `--output`, both 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.
+
 ## Storage Architecture
 
 ### Directory Structure

internal/cli/create_vault_test.go

@@ -144,20 +144,25 @@ func TestVaultCreationLeavesNoSecretInEnvironment(t *testing.T) {
 // TestNoVaultImportCommand is a regression test for
 // https://git.eeqj.de/sneak/secret/issues/139: `secret vault import`
 // refused every vault that `secret vault create` makes, because each has a
-// long-term key. The command is gone, so it fails as any subcommand of
-// `secret vault` that does not exist does.
+// long-term key. The command is gone, so cobra answers it with the help of
+// `secret vault`, as it does any subcommand of `secret vault` it does not
+// know.
 func TestNoVaultImportCommand(t *testing.T) {
 	t.Setenv(secret.EnvStateDir, t.TempDir())
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-	t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
 
-	_, err := cli.ExecuteCommandInProcess(
-		strings.Fields("vault create work"), "", nil)
-	require.NoError(t, err)
+	run := func(args ...string) string {
+		t.Setenv(secret.EnvMnemonic, testMnemonic)
+		t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
 
-	_, err = cli.ExecuteCommandInProcess(
-		strings.Fields("vault import work"), "", nil)
-	require.EqualError(t, err, `unknown command "import" for "secret vault"`)
+		output, err := cli.ExecuteCommandInProcess(args, "", nil)
+		require.NoError(t, err, "%v", args)
+
+		return output
+	}
+
+	run("vault", "create", "work")
+
+	require.Equal(t, run("vault"), run("vault", "import", "work"))
 }
 
 // TestStopAtPassphrasePromptLeavesNothing is a regression test for the

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
 }
 

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 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))
+
+			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))
+}

internal/cli/generate.go

@@ -29,12 +29,6 @@ func newGenerateCmd() *cobra.Command {
 		Use:   "generate",
 		Short: "Generate random data",
 		Long:  `Generate various types of random data including mnemonics and secrets.`,
-		// Cobra checks Args only on a command that has a RunE; without
-		// one, an unknown subcommand prints this help and exits 0.
-		Args: cobra.NoArgs,
-		RunE: func(cmd *cobra.Command, _ []string) error {
-			return cmd.Help()
-		},
 	}
 
 	cmd.AddCommand(newGenerateMnemonicCmd())

internal/cli/unknown_subcommand_test.go

@@ -1,58 +0,0 @@
-package cli_test
-
-import (
-	"fmt"
-	"os/exec"
-	"strings"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/secret/internal/secret"
-)
-
-// TestUnknownSubcommandFails is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/183: given a subcommand it does not
-// have, `secret vault`, `secret unlocker`, `secret version` and `secret
-// generate` printed their help and exited 0, so a script could not tell a
-// mistyped command from a successful one. Each runs here as the built binary
-// and must exit 1 naming the subcommand, as an unknown command does at the
-// top level, and must still print its help and exit 0 when given nothing.
-func TestUnknownSubcommandFails(t *testing.T) {
-	t.Parallel()
-
-	stateDir := t.TempDir()
-
-	// run runs the built binary with args and returns what it printed and
-	// its exit code
-	run := func(args ...string) (string, int) {
-		//nolint:gosec // G204: test executes the freshly built secret binary
-		cmd := exec.CommandContext(t.Context(), secretBinaryPath(t), args...)
-		cmd.Env = []string{secret.EnvStateDir + "=" + stateDir}
-
-		output, err := cmd.CombinedOutput()
-		if err != nil {
-			var exitErr *exec.ExitError
-			require.ErrorAs(t, err, &exitErr, "secret %v did not run", args)
-		}
-
-		return string(output), cmd.ProcessState.ExitCode()
-	}
-
-	for _, call := range []string{
-		"vault improt", "unlocker foo", "version foo", "generate foo",
-	} {
-		parent, unknown, _ := strings.Cut(call, " ")
-
-		output, exitCode := run(parent, unknown)
-		assert.Equal(t, 1, exitCode, "secret %s should fail:\n%s", call, output)
-		assert.Contains(t, output, fmt.Sprintf(
-			"unknown command %q for \"secret %s\"", unknown, parent))
-
-		output, exitCode = run(parent)
-		assert.Equal(t, 0, exitCode,
-			"secret %s should succeed:\n%s", parent, output)
-		assert.Contains(t, output, usageHeading,
-			"secret %s should print its help", parent)
-	}
-}

internal/cli/unlockers.go

@@ -126,12 +126,6 @@ func newUnlockerCmd() *cobra.Command {
 		Use:   "unlocker",
 		Short: "Manage unlockers",
 		Long:  `Create, list, and remove unlockers for the current vault.`,
-		// Cobra checks Args only on a command that has a RunE; without
-		// one, an unknown subcommand prints this help and exits 0.
-		Args: cobra.NoArgs,
-		RunE: func(cmd *cobra.Command, _ []string) error {
-			return cmd.Help()
-		},
 	}
 
 	cmd.AddCommand(newUnlockerListCmd())

internal/cli/vault.go

@@ -29,12 +29,6 @@ func newVaultCmd() *cobra.Command {
 		Use:   "vault",
 		Short: "Manage vaults",
 		Long:  `Create, list, and select vaults for organizing secrets.`,
-		// Cobra checks Args only on a command that has a RunE; without
-		// one, an unknown subcommand prints this help and exits 0.
-		Args: cobra.NoArgs,
-		RunE: func(cmd *cobra.Command, _ []string) error {
-			return cmd.Help()
-		},
 	}
 
 	cmd.AddCommand(newVaultListCmd())

internal/cli/version.go

@@ -43,12 +43,6 @@ func VersionCommands(cli *Instance) *cobra.Command {
 		Short: "Manage secret versions",
 		Long: "Commands for managing secret versions including listing, " +
 			"promoting, and retrieving specific versions",
-		// Cobra checks Args only on a command that has a RunE; without
-		// one, an unknown subcommand prints this help and exits 0.
-		Args: cobra.NoArgs,
-		RunE: func(cmd *cobra.Command, _ []string) error {
-			return cmd.Help()
-		},
 	}
 
 	// List versions command