A cross-vault secret mv that cannot remove the source fails (closes #159)

When `secret mv` copied a secret into another vault but could not remove
the source, it printed a warning and the usual "Moved" line and exited 0,
so a script could not tell that the secret was now in both vaults. It now
prints no "Moved" line and fails with an error saying that the copy holds
all versions and the source remains.

The test is in package cli and calls moveSecret, because the state
directory lock refuses the wrapped filesystem that makes the removal fail.

Judgement call: if the source is moved aside but the temporary directory
holding it cannot then be deleted, the error still says the source remains.

Model: opus-5-5

README.md

@@ -313,11 +313,6 @@ 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/crypto.go

@@ -1,7 +1,6 @@
 package cli
 
 import (
-	"bytes"
 	"errors"
 	"fmt"
 	"io"
@@ -108,8 +107,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 reads its input and writes its
-// output without the lock.
+// at the passphrase prompt, and Encrypt streams its input and output
+// unlocked.
 func (cli *Instance) resolveEncryptionKey(
 	vlt *vault.Vault, secretName string,
 ) (*memguard.LockedBuffer, error) {
@@ -189,17 +188,17 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
 		input = file
 	}
 
-	// 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.
+	// Set up output writer
 	output := cli.cmd.OutOrStdout()
 
-	var result bytes.Buffer
-
 	if outputFile != "" {
-		output = &result
+		file, err := cli.fs.Create(outputFile)
+		if err != nil {
+			return fmt.Errorf("failed to create output file: %w", err)
+		}
+		defer func() { _ = file.Close() }()
+
+		output = file
 	}
 
 	// Encrypt the data
@@ -218,11 +217,7 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
 		return fmt.Errorf("failed to finalize encryption: %w", err)
 	}
 
-	if outputFile == "" {
-		return nil
-	}
-
-	return cli.writeOutputFile(outputFile, result.Bytes())
+	return nil
 }
 
 // Decrypt decrypts data using an age secret key stored in a secret
@@ -283,17 +278,17 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
 		input = file
 	}
 
-	// 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.
+	// Set up output writer
 	output := cli.cmd.OutOrStdout()
 
-	var result bytes.Buffer
-
 	if outputFile != "" {
-		output = &result
+		file, err := cli.fs.Create(outputFile)
+		if err != nil {
+			return fmt.Errorf("failed to create output file: %w", err)
+		}
+		defer func() { _ = file.Close() }()
+
+		output = file
 	}
 
 	// Decrypt the data
@@ -307,32 +302,6 @@ 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

@@ -1,185 +0,0 @@
-//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/move_cross_vault_test.go

@@ -0,0 +1,85 @@
+//nolint:testpackage // white-box test of unexported internals
+package cli
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/awnumar/memguard"
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/vault"
+)
+
+// errRenameFailed is returned by renameFailFs in place of a successful
+// rename.
+var errRenameFailed = errors.New("permission denied")
+
+// renameFailFs fails every Rename of one path, as a read-only parent
+// directory does.
+type renameFailFs struct {
+	afero.Fs
+
+	path string
+}
+
+func (f *renameFailFs) Rename(oldname, newname string) error {
+	if oldname == f.path {
+		return errRenameFailed
+	}
+
+	return f.Fs.Rename(oldname, newname)
+}
+
+// TestCrossVaultMoveFailsWhenSourceRemains is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/159, where a move between two
+// vaults whose source could not be removed reported the secret moved and
+// succeeded. It must fail, and the copy and the source must both be there.
+// The state directory lock refuses the wrapped filesystem, so the test
+// calls moveSecret, which MoveSecret runs once it holds the lock.
+func TestCrossVaultMoveFailsWhenSourceRemains(t *testing.T) {
+	t.Parallel()
+
+	base := afero.NewMemMapFs()
+
+	_, err := vault.CreateVault(base, testStateDir, "work",
+		testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	source, err := vault.CreateVault(base, testStateDir, "default",
+		testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	err = source.AddSecret("x",
+		memguard.NewBufferFromBytes([]byte("value")), false)
+	require.NoError(t, err)
+
+	instance := NewCLIInstanceWithStateDir(&renameFailFs{
+		Fs:   base,
+		path: testStateDir + "/vaults.d/default/secrets.d/x",
+	}, testStateDir)
+	instance.Mnemonic = testMnemonicBuffer(t)
+
+	err = instance.moveSecret(&cobra.Command{}, "default:x", "work:y", false)
+	require.ErrorIs(t, err, errRenameFailed)
+	require.ErrorContains(t, err, "secret 'default:x' is copied with all "+
+		"1 version(s) to 'work:y', but the source remains")
+
+	// requireValue requires that the vault vaultName holds secretName with
+	// the value stored above.
+	requireValue := func(vaultName, secretName string) {
+		vlt := vault.NewVault(base, testStateDir, vaultName)
+		vlt.Mnemonic = testMnemonicBuffer(t)
+
+		value, err := vlt.GetSecret(secretName)
+		require.NoError(t, err, vaultName+":"+secretName)
+
+		defer value.Destroy()
+
+		require.Equal(t, []byte("value"), value.Bytes())
+	}
+
+	requireValue("default", "x")
+	requireValue("work", "y")
+}

internal/cli/secrets.go

@@ -1097,12 +1097,10 @@ func (cli *Instance) moveSecretCrossVault(
 	// Delete source secret
 	err = secret.RemoveDirAtomic(cli.fs, srcSecretDir)
 	if err != nil {
-		// Copy succeeded but delete failed - warn but don't fail
-		cmd.Printf("Warning: copied secret but failed to remove source: %v\n", err)
-		cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",
-			srcVault.Name, srcSecretName, destVault.Name, destSecretName, versionCount)
-
-		return nil
+		return fmt.Errorf("secret '%s:%s' is copied with all %d version(s) "+
+			"to '%s:%s', but the source remains: %w",
+			srcVault.Name, srcSecretName, versionCount,
+			destVault.Name, destSecretName, err)
 	}
 
 	cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",