A secret mv between vaults unlocks both vaults before taking the lock (closes #189)

A move between two vaults unlocked them while holding the state directory
lock, and unlocking can ask for a passphrase, so every other changing
command waited at that prompt. moveSecretCrossVault now checks the move,
refusing an existing destination secret without --force, unlocks both
vaults, and only then takes the lock. Under the lock, it checks that the
destination vault still has the long-term key it unlocked, and
copySecretAndRemoveSource checks the move again before it copies. A move
within a vault takes the lock in moveSecretWithinVault, so MoveSecret
takes none itself. The error for a vault created again with another key,
shared with secret unlocker add, now ends "nothing was changed".

Model: opus-5-5

README.md

@@ -118,7 +118,11 @@ are allowed, and a name must not be empty, `.` or `..`.
 
 #### `secret vault select <name>`
 
-Switches to the specified vault for subsequent operations.
+Switches to the specified vault for subsequent operations. When `currentvault`
+is empty or names a vault that does not exist, every command that uses the
+current vault fails with an error that names the file and says to run
+`secret vault select <name>`. `secret vault list` still lists the vaults, and
+`secret info` still runs; both show no vault as current.
 
 #### `secret vault remove <name> [--force]` / `secret vault rm` ⚠️ 🛑
 
@@ -318,6 +322,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 can need up to about four times as much memory as the result.
+
 #### `secret encrypt <secret-name> [--input=file] [--output=file]`
 
 Encrypts data using an Age key stored as a secret. If the secret doesn't exist,
@@ -430,12 +440,11 @@ in a CI job, the command fails and names the variable to set.
 
 A command that changes anything locks the state directory, through its file
 `lock`, and another such command waits until the lock is released.
-`secret init`, `secret vault create`, `secret unlocker add`, and `secret add`,
-`secret import` and `secret generate secret` with `--force`, ask for the
-mnemonic or passphrase before they take the lock, so a prompt left open in one
-terminal holds up no command in another. A forced add of a secret that does not
-exist yet fails, and changes nothing, if another command creates the secret
-while it waits for the lock; run it again to replace the secret.
+`secret init`, `secret vault create`, `secret unlocker add`, `secret mv` from
+one vault to another, and `secret add`, `secret import` and
+`secret generate secret` with `--force`, ask for the mnemonic or passphrase
+before they take the lock, so a prompt of theirs left open in one terminal holds
+up no command in another.
 
 ## Security Features
 

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

internal/cli/current_vault_test.go

@@ -0,0 +1,60 @@
+package cli_test
+
+import (
+	"bytes"
+	"path/filepath"
+	"strconv"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/cli"
+	"sneak.berlin/go/secret/internal/secret"
+	"sneak.berlin/go/secret/internal/vault"
+)
+
+// TestListFailsWhenCurrentVaultDoesNotExist is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/156, where a currentvault file
+// that was empty, held an invalid vault name, or named a vault that does
+// not exist, made `secret list` report an empty vault. Listing must fail
+// with an error that names the file and says to run `secret vault select`,
+// `secret vault list` must still list the vaults, and selecting a vault
+// must make listing work again. The name "." would make vaults.d itself the
+// vault, and "../vaults.d/work" would reach the vault work by a path.
+func TestListFailsWhenCurrentVaultDoesNotExist(t *testing.T) {
+	t.Parallel()
+
+	for _, name := range []string{"", ".", "../vaults.d/work", "gone"} {
+		t.Run(strconv.Quote(name), func(t *testing.T) {
+			t.Parallel()
+
+			fs := newTwoVaultFs(t)
+			currentVaultPath := filepath.Join(testStateDir, "currentvault")
+			require.NoError(t, afero.WriteFile(fs, currentVaultPath,
+				[]byte(name), secret.FilePerms))
+
+			c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
+
+			var out bytes.Buffer
+
+			cmd := &cobra.Command{}
+			cmd.SetOut(&out)
+
+			err := c.ListSecrets(cmd, false, true, "")
+			require.ErrorIs(t, err, vault.ErrVaultNotFound)
+			require.ErrorContains(t, err, currentVaultPath)
+			require.ErrorContains(t, err, "secret vault select")
+			assert.Empty(t, out.String())
+
+			require.NoError(t, c.ListVaults(cmd, false))
+			assert.Equal(t, "Available vaults:\n  default\n  work\n", out.String())
+
+			require.NoError(t, c.SelectVault(cmd, "default"))
+			out.Reset()
+			require.NoError(t, c.ListSecrets(cmd, false, true, ""))
+			assert.Equal(t, "x\n", out.String())
+		})
+	}
+}

internal/cli/lock_test.go

@@ -627,6 +627,10 @@ func TestCommandsCheckAgainOnceLocked(t *testing.T) {
 
 	mnemonic := testMnemonicBuffer(t)
 
+	otherMnemonic := memguard.NewBufferFromBytes([]byte("legal winner thank " +
+		"year wave sausage worth useful legal winner thank yellow"))
+	t.Cleanup(otherMnemonic.Destroy)
+
 	passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
 	t.Cleanup(passphrase.Destroy)
 
@@ -658,6 +662,26 @@ func TestCommandsCheckAgainOnceLocked(t *testing.T) {
 			},
 			vault.ErrVaultNotFound,
 		},
+		{
+			"unlocker add, vault created again from another mnemonic",
+			func(cli *Instance) error {
+				cli.Mnemonic, cli.UnlockPassphrase = mnemonic, passphrase
+
+				return cli.UnlockersAdd("passphrase", cli.cmd)
+			},
+			func(fs afero.Fs) error {
+				err := fs.RemoveAll(testStateDir + "/vaults.d/work")
+				if err != nil {
+					return err
+				}
+
+				_, err = vault.CreateVault(fs, testStateDir, "work",
+					otherMnemonic, passphrase)
+
+				return err
+			},
+			errVaultCreatedAgain,
+		},
 		{
 			"unlocker add pgp, key added",
 			func(cli *Instance) error {
@@ -674,22 +698,6 @@ func TestCommandsCheckAgainOnceLocked(t *testing.T) {
 			},
 			errGPGKeyAlreadyUnlocker,
 		},
-		{
-			// Without the mnemonic or a passphrase, replacing the secret
-			// would ask for the passphrase; it does not exist yet, so the
-			// add asks for nothing
-			"add --force, secret added",
-			func(cli *Instance) error {
-				return cli.AddSecret("added", true)
-			},
-			func(fs afero.Fs) error {
-				value := memguard.NewBufferFromBytes([]byte("other"))
-				defer value.Destroy()
-
-				return work(fs).AddSecret("added", value, false)
-			},
-			errAddedMeanwhile,
-		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			requireChecksAgainOnceLocked(t, tc.run, tc.change, tc.want)
@@ -735,6 +743,49 @@ func requireChecksAgainOnceLocked(
 	assert.Equal(t, before, after, "changed the state directory")
 }
 
+// TestForcedAddReplacesSecretAddedMeanwhile runs a forced add of a secret
+// that does not exist yet while the test holds the state directory lock.
+// Once it waits for the lock, the test adds the secret, as another command
+// could have, and releases the lock. The forced add must replace that
+// secret, as if it had run after the other command.
+//
+//nolint:paralleltest // waitingForLock sees any test's command waiting for the lock
+func TestForcedAddReplacesSecretAddedMeanwhile(t *testing.T) {
+	mnemonic := testMnemonicBuffer(t)
+
+	fs := afero.NewMemMapFs()
+	release, done := startWhileLocked(t, fs, func(cli *Instance) error {
+		cli.Mnemonic = mnemonic
+
+		return cli.AddSecret("added", true)
+	})
+
+	requireWaitingForLock(t, done)
+
+	work := vault.NewVault(fs, testStateDir, "work")
+	work.Mnemonic = mnemonic
+
+	other := memguard.NewBufferFromBytes([]byte("other"))
+	t.Cleanup(other.Destroy)
+
+	require.NoError(t, work.AddSecret("added", other, false))
+
+	release()
+
+	select {
+	case err := <-done:
+		require.NoError(t, err)
+	case <-time.After(lockWait):
+		t.Fatal("did not finish once the lock was released")
+	}
+
+	value, err := work.GetSecret("added")
+	require.NoError(t, err)
+	t.Cleanup(value.Destroy)
+
+	assert.Equal(t, "value", string(value.Bytes()))
+}
+
 // TestEncryptWithExistingKeyTakesNoLock checks that secret encrypt with a
 // key that already exists, which only reads the state directory, finishes
 // while another command holds the state directory lock.

internal/cli/move_cross_vault_test.go

@@ -0,0 +1,315 @@
+//nolint:testpackage // white-box test of unexported internals
+package cli
+
+import (
+	"errors"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/awnumar/memguard"
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/secret"
+	"sneak.berlin/go/secret/internal/vault"
+)
+
+// errPermissionDenied is returned by renameFailFs and removeAllFailFs in
+// place of a rename or removal that succeeds.
+var errPermissionDenied = 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 errPermissionDenied
+	}
+
+	return f.Fs.Rename(oldname, newname)
+}
+
+// removeAllFailFs fails every RemoveAll of an entry of the directory dir,
+// as a file in it that cannot be deleted does, and records that entry's
+// path in failed.
+type removeAllFailFs struct {
+	afero.Fs
+
+	dir    string
+	failed string
+}
+
+func (f *removeAllFailFs) RemoveAll(path string) error {
+	if filepath.Dir(path) == f.dir {
+		f.failed = path
+
+		return errPermissionDenied
+	}
+
+	return f.Fs.RemoveAll(path)
+}
+
+// createCrossVaults creates in fs the vaults "work" and "default", where
+// "default" holds the secret "x" with the value "value".
+func createCrossVaults(t *testing.T, fs afero.Fs) {
+	t.Helper()
+
+	_, err := vault.CreateVault(fs, testStateDir, "work",
+		testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	source, err := vault.CreateVault(fs, testStateDir, "default",
+		testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	err = source.AddSecret("x",
+		memguard.NewBufferFromBytes([]byte("value")), false)
+	require.NoError(t, err)
+}
+
+// requireValue requires that the vault vaultName in fs holds secretName
+// with the value createCrossVaults stored.
+func requireValue(t *testing.T, fs afero.Fs, vaultName, secretName string) {
+	t.Helper()
+
+	vlt := vault.NewVault(fs, 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())
+}
+
+// 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 copySecretAndRemoveSource, which a move between vaults runs once it
+// holds the lock.
+func TestCrossVaultMoveFailsWhenSourceRemains(t *testing.T) {
+	t.Parallel()
+
+	base := afero.NewMemMapFs()
+	createCrossVaults(t, base)
+
+	instance := NewCLIInstanceWithStateDir(&renameFailFs{
+		Fs:   base,
+		path: testStateDir + "/vaults.d/default/secrets.d/x",
+	}, testStateDir)
+
+	source := vault.NewVault(instance.fs, testStateDir, "default")
+	source.Mnemonic = testMnemonicBuffer(t)
+
+	err := instance.copySecretAndRemoveSource(&cobra.Command{}, source, "x",
+		vault.NewVault(instance.fs, testStateDir, "work"), "y", false)
+	require.ErrorIs(t, err, errPermissionDenied)
+	require.ErrorContains(t, err, "secret 'default:x' is copied with all "+
+		"1 version(s) to 'work:y', but the source remains")
+
+	requireValue(t, base, "default", "x")
+	requireValue(t, base, "work", "y")
+}
+
+// TestCrossVaultMoveFailsWhenOldFilesRemain makes the removal of the source
+// fail after the source has been moved out of its vault, when only the
+// temporary directory it was moved into cannot be deleted. The move must
+// fail, say that the secret is moved, and name that directory.
+func TestCrossVaultMoveFailsWhenOldFilesRemain(t *testing.T) {
+	t.Parallel()
+
+	base := afero.NewMemMapFs()
+	createCrossVaults(t, base)
+
+	sourceVaultDir := testStateDir + "/vaults.d/default"
+	fs := &removeAllFailFs{Fs: base, dir: sourceVaultDir}
+
+	instance := NewCLIInstanceWithStateDir(fs, testStateDir)
+
+	source := vault.NewVault(fs, testStateDir, "default")
+	source.Mnemonic = testMnemonicBuffer(t)
+
+	err := instance.copySecretAndRemoveSource(&cobra.Command{}, source, "x",
+		vault.NewVault(fs, testStateDir, "work"), "y", false)
+	require.ErrorIs(t, err, errPermissionDenied)
+	require.ErrorContains(t, err, "secret 'default:x' is moved with all "+
+		"1 version(s) to 'work:y', but its old files could not all be deleted")
+
+	// The error names the directory that holds the old files
+	require.NotEmpty(t, fs.failed)
+	require.ErrorContains(t, err, fs.failed)
+
+	oldFiles, err := afero.DirExists(base, filepath.Join(fs.failed, "x"))
+	require.NoError(t, err)
+	require.True(t, oldFiles)
+
+	// The source vault no longer holds the secret
+	remains, err := afero.DirExists(base, sourceVaultDir+"/secrets.d/x")
+	require.NoError(t, err)
+	require.False(t, remains)
+
+	requireValue(t, base, "work", "y")
+}
+
+// TestCrossVaultMoveUnlocksBeforeLocking is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/189: a move between two vaults
+// unlocked them while holding the state directory lock, and unlocking a
+// vault can ask for its passphrase, so every other command that changes
+// anything waited while the user answered. Here each vault has a passphrase
+// unlocker whose passphrase is the vault's name, and the move is given the
+// passphrase of only one of them. Run while the test holds the lock, it
+// must fail to unlock the other vault without waiting for the lock.
+//
+//nolint:paralleltest // times commands against the in-memory lock all tests share
+func TestCrossVaultMoveUnlocksBeforeLocking(t *testing.T) {
+	fs := afero.NewMemMapFs()
+	createCrossVaults(t, fs)
+
+	vaults, err := vault.ListVaults(fs, testStateDir)
+	require.NoError(t, err)
+
+	for _, name := range vaults {
+		vlt := vault.NewVault(fs, testStateDir, name)
+		vlt.Mnemonic = testMnemonicBuffer(t)
+
+		passphrase := memguard.NewBufferFromBytes([]byte(name))
+		_, err = vlt.CreatePassphraseUnlocker(passphrase)
+		passphrase.Destroy()
+		require.NoError(t, err)
+	}
+
+	// The move is from "default" to "work"
+	for _, tc := range []struct {
+		// failing is the vault that the move cannot unlock, given the
+		// passphrase of the vault named given
+		failing, given string
+	}{
+		{"source", "work"},
+		{"destination", "default"},
+	} {
+		t.Run(tc.failing, func(t *testing.T) {
+			passphrase := memguard.NewBufferFromBytes([]byte(tc.given))
+			defer passphrase.Destroy()
+
+			instance := NewCLIInstanceWithStateDir(fs, testStateDir)
+			instance.UnlockPassphrase = passphrase
+
+			release, err := vault.LockStateDir(fs, testStateDir)
+			require.NoError(t, err)
+			// Also frees a waiting move if the test fails, so that it
+			// releases the lock the other tests use
+			defer release()
+
+			done := make(chan error, 1)
+
+			go func() {
+				done <- instance.MoveSecret(&cobra.Command{},
+					"default:x", "work:y", false)
+			}()
+
+			select {
+			case err := <-done:
+				require.ErrorIs(t, err, secret.ErrWrongPassphrase)
+				require.ErrorContains(t, err,
+					"failed to unlock "+tc.failing+" vault")
+			case <-time.After(lockWait):
+				t.Fatal("waited for the lock before unlocking the " +
+					tc.failing + " vault")
+			}
+		})
+	}
+}
+
+// TestCrossVaultMoveChecksAgainOnceLocked runs a move between vaults, given
+// the mnemonic, as TestCommandsCheckAgainOnceLocked runs the commands in its
+// table: once the move waits for the state directory lock, the test removes
+// the source secret or the destination vault, or creates the destination
+// vault again from another mnemonic, and the move must then fail and change
+// nothing.
+//
+//nolint:paralleltest // waitingForLock sees any test's command waiting for the lock
+func TestCrossVaultMoveChecksAgainOnceLocked(t *testing.T) {
+	mnemonic := testMnemonicBuffer(t)
+
+	otherMnemonic := memguard.NewBufferFromBytes([]byte("legal winner thank " +
+		"year wave sausage worth useful legal winner thank yellow"))
+	t.Cleanup(otherMnemonic.Destroy)
+
+	move := func(cli *Instance) error {
+		cli.Mnemonic = mnemonic
+
+		return cli.MoveSecret(cli.cmd, "work:test/secret", "other:moved", false)
+	}
+
+	t.Run("source removed", func(t *testing.T) {
+		requireChecksAgainOnceLocked(t, move, func(fs afero.Fs) error {
+			return fs.RemoveAll(testStateDir + "/vaults.d/work/secrets.d/test%secret")
+		}, vault.ErrSecretNotFound)
+	})
+
+	t.Run("destination vault removed", func(t *testing.T) {
+		requireChecksAgainOnceLocked(t, move, func(fs afero.Fs) error {
+			return fs.RemoveAll(testStateDir + "/vaults.d/other")
+		}, vault.ErrVaultNotFound)
+	})
+
+	t.Run("destination vault created again from another mnemonic", func(t *testing.T) {
+		// Set by the move, which has finished once
+		// requireChecksAgainOnceLocked returns
+		var moveErr error
+
+		requireChecksAgainOnceLocked(t, func(cli *Instance) error {
+			moveErr = move(cli)
+
+			return moveErr
+		}, func(fs afero.Fs) error {
+			err := fs.RemoveAll(testStateDir + "/vaults.d/other")
+			if err != nil {
+				return err
+			}
+
+			_, err = vault.CreateVault(fs, testStateDir, "other", otherMnemonic, nil)
+
+			return err
+		}, errVaultCreatedAgain)
+
+		// secret unlocker add fails with the same error, so its message
+		// must fit a move too
+		require.EqualError(t, moveErr, "vault 'other' was removed and "+
+			"created again with another key while it was being unlocked; "+
+			"nothing was changed")
+	})
+}
+
+// TestCrossVaultMoveRefusesExistingSecretBeforeUnlocking checks that a move
+// between vaults onto a secret that exists, without --force, is refused
+// before either vault is unlocked, since unlocking can ask for a
+// passphrase. The vaults have no unlockers and the move is given no
+// mnemonic, so unlocking either vault fails.
+func TestCrossVaultMoveRefusesExistingSecretBeforeUnlocking(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	createCrossVaults(t, fs)
+
+	work := vault.NewVault(fs, testStateDir, "work")
+	work.Mnemonic = testMnemonicBuffer(t)
+
+	err := work.AddSecret("y",
+		memguard.NewBufferFromBytes([]byte("other")), false)
+	require.NoError(t, err)
+
+	instance := NewCLIInstanceWithStateDir(fs, testStateDir)
+
+	err = instance.MoveSecret(&cobra.Command{}, "default:x", "work:y", false)
+	require.ErrorIs(t, err, vault.ErrSecretExists)
+}

internal/cli/secrets.go

@@ -47,10 +47,6 @@ var (
 	errCrossVaultSourceUnqualified = errors.New(
 		"source must specify vault (e.g., vault:secret) for cross-vault move")
 	errMoveOntoItself = errors.New("cannot be moved onto itself")
-	// errAddedMeanwhile carries only the message tail; storeSecret composes
-	// "secret <name> was added by another command ...".
-	errAddedMeanwhile = errors.New("was added by another command in the " +
-		"meantime, so nothing was changed; run the command again to replace it")
 )
 
 // ParseVaultSecretRef parses a "vault:secret" or just "secret" reference
@@ -434,9 +430,9 @@ func (cli *Instance) AddSecret(secretName string, force bool) error {
 // an existing secret replaces its current version, which needs the vault's
 // long-term key. Getting the key can ask for the passphrase, so storeSecret
 // gets it before it takes the lock, and no other command waits while the
-// user answers. If, once the lock is held, the add replaces a version after
-// all, because another command added the secret in the meantime, it fails
-// rather than ask with the lock held.
+// user answers. If another command adds the secret while this one waits for
+// the lock, storeSecret lets go of the lock, gets the key, and takes the
+// lock again.
 func (cli *Instance) storeSecret(
 	vlt *vault.Vault, name string, value *memguard.LockedBuffer, force bool,
 ) error {
@@ -457,24 +453,46 @@ func (cli *Instance) storeSecret(
 		}
 	}
 
+	needsKey, err := cli.addWithoutAsking(vlt, name, value, force)
+	if !needsKey {
+		return err
+	}
+
+	_, err = vlt.GetOrDeriveLongTermKey()
+	if err != nil {
+		return fmt.Errorf("failed to get long-term key: %w", err)
+	}
+
+	// With the key in memory, this adds the secret
+	_, err = cli.addWithoutAsking(vlt, name, value, force)
+
+	return err
+}
+
+// addWithoutAsking takes the state directory lock and adds the secret name
+// with value to vlt, as vault.AddSecret does. If that would replace the
+// secret's current version while the vault's long-term key is not in
+// memory, getting the key could ask for the passphrase with the lock held:
+// addWithoutAsking then adds nothing and returns true.
+func (cli *Instance) addWithoutAsking(
+	vlt *vault.Vault, name string, value *memguard.LockedBuffer, force bool,
+) (bool, error) {
 	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
 	if err != nil {
-		return err
+		return false, err
 	}
 	defer release()
 
-	if !replacing {
-		replacing, err = replacesVersion(vlt, name, force)
-		if err != nil {
-			return err
-		}
-
-		if replacing {
-			return fmt.Errorf("secret %s %w", name, errAddedMeanwhile)
-		}
+	replacing, err := replacesVersion(vlt, name, force)
+	if err != nil {
+		return false, err
 	}
 
-	return vlt.AddSecret(name, value, force)
+	if replacing && vlt.Locked() {
+		return true, nil
+	}
+
+	return false, vlt.AddSecret(name, value, force)
 }
 
 // replacesVersion reports whether adding the secret name to vlt replaces
@@ -857,24 +875,12 @@ func (cli *Instance) findSecretToRemove(
 	}, nil
 }
 
-// MoveSecret moves or renames a secret (within or across vaults), holding
-// the state directory lock.
+// MoveSecret moves or renames a secret (within or across vaults). It
+// leaves taking the state directory lock to moveSecretWithinVault and
+// moveSecretCrossVault, since a move between vaults unlocks both vaults
+// before it takes the lock.
 func (cli *Instance) MoveSecret(
 	cmd *cobra.Command, source, dest string, force bool,
-) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
-	return cli.moveSecret(cmd, source, dest, force)
-}
-
-// moveSecret does the work of MoveSecret. The caller holds the state
-// directory lock.
-func (cli *Instance) moveSecret(
-	cmd *cobra.Command, source, dest string, force bool,
 ) error {
 	// Parse source and destination
 	srcVaultName, srcSecretName, srcQualified := ParseVaultSecretRef(source)
@@ -974,8 +980,9 @@ func (cli *Instance) existingVault(name string) (*vault.Vault, error) {
 	return vault.NewVault(cli.fs, cli.stateDir, name), nil
 }
 
-// moveSecretWithinVault renames a secret within the vault vlt. Its caller,
-// MoveSecret, has already checked both secret names.
+// moveSecretWithinVault renames a secret within the vault vlt, holding the
+// state directory lock. Its caller, MoveSecret, has already checked both
+// secret names.
 func (cli *Instance) moveSecretWithinVault(
 	cmd *cobra.Command, vlt *vault.Vault, source, dest string, force bool,
 ) error {
@@ -985,6 +992,12 @@ func (cli *Instance) moveSecretWithinVault(
 		return fmt.Errorf("secret '%s' %w", source, errMoveOntoItself)
 	}
 
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	vaultDir, err := vlt.GetDirectory()
 	if err != nil {
 		return err
@@ -1071,59 +1084,75 @@ func (cli *Instance) sameDirectory(dir, other string) (bool, error) {
 
 // moveSecretCrossVault handles moving between two different vaults. Its
 // caller, MoveSecret, has already checked both secret names and that both
-// vaults exist.
+// vaults exist. The copy needs the source vault's long-term key and is
+// encrypted to the destination vault's long-term public key. The
+// destination vault is unlocked to check that the copy will open with what
+// the user gave, and, once the state directory lock is held, that it still
+// has the key unlocked. Either unlock can ask for a passphrase, so both
+// happen before the lock is taken, and no other command waits while the
+// user answers.
 func (cli *Instance) moveSecretCrossVault(
 	cmd *cobra.Command,
 	srcVault *vault.Vault, srcSecretName string,
 	destVault *vault.Vault, destSecretName string,
 	force bool,
 ) error {
-	srcVaultDir, err := srcVault.GetDirectory()
-	if err != nil {
-		return fmt.Errorf("failed to get source vault directory: %w", err)
-	}
-
-	// Verify source secret exists
-	srcStorageName := strings.ReplaceAll(srcSecretName, "/", "%")
-	srcSecretDir := filepath.Join(srcVaultDir, "secrets.d", srcStorageName)
-
-	exists, err := afero.DirExists(cli.fs, srcSecretDir)
-	if err != nil || !exists {
-		return fmt.Errorf("secret '%s' %w in vault '%s'",
-			srcSecretName, vault.ErrSecretNotFound, srcVault.Name)
-	}
-
-	// The source is removed after the copy, so a destination that is the
-	// source under another name would be lost with it.
-	destVaultDir, err := destVault.GetDirectory()
-	if err != nil {
-		return fmt.Errorf("failed to get destination vault directory: %w", err)
-	}
-
-	destStorageName := strings.ReplaceAll(destSecretName, "/", "%")
-	destSecretDir := filepath.Join(destVaultDir, "secrets.d", destStorageName)
-
-	same, err := cli.sameDirectory(srcSecretDir, destSecretDir)
+	// Before unlocking, check that the source secret exists, that the
+	// destination is not the source under another name, and, without
+	// --force, that the destination secret does not exist
+	_, err := cli.checkCrossVaultMove(
+		srcVault, srcSecretName, destVault, destSecretName, force)
 	if err != nil {
 		return err
 	}
 
-	if same {
-		return fmt.Errorf("secret '%s:%s' %w: '%s:%s' is the same secret on "+
-			"this filesystem", srcVault.Name, srcSecretName, errMoveOntoItself,
-			destVault.Name, destSecretName)
-	}
-
-	// Copying needs the long-term keys of both vaults
 	srcVault.Mnemonic, srcVault.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
 	destVault.Mnemonic, destVault.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
 
 	// Unlock destination vault (will fail if neither mnemonic nor unlocker available)
-	_, err = destVault.GetOrDeriveLongTermKey()
+	destIdentity, err := destVault.GetOrDeriveLongTermKey()
 	if err != nil {
 		return fmt.Errorf("failed to unlock destination vault '%s': %w", destVault.Name, err)
 	}
 
+	_, err = srcVault.GetOrDeriveLongTermKey()
+	if err != nil {
+		return fmt.Errorf("failed to unlock source vault '%s': %w",
+			srcVault.Name, err)
+	}
+
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	err = cli.checkLongTermKey(destVault, destIdentity)
+	if err != nil {
+		return err
+	}
+
+	return cli.copySecretAndRemoveSource(
+		cmd, srcVault, srcSecretName, destVault, destSecretName, force)
+}
+
+// copySecretAndRemoveSource copies the secret srcSecretName of srcVault,
+// with all its versions, to destSecretName in destVault, and removes the
+// source. moveSecretCrossVault calls it holding the state directory lock.
+// Another command may have changed either vault since moveSecretCrossVault
+// checked them, so it checks them again first.
+func (cli *Instance) copySecretAndRemoveSource(
+	cmd *cobra.Command,
+	srcVault *vault.Vault, srcSecretName string,
+	destVault *vault.Vault, destSecretName string,
+	force bool,
+) error {
+	srcSecretDir, err := cli.checkCrossVaultMove(
+		srcVault, srcSecretName, destVault, destSecretName, force)
+	if err != nil {
+		return err
+	}
+
 	// Count versions for user feedback
 	versions, _ := secret.ListVersions(cli.fs, srcSecretDir)
 	versionCount := len(versions)
@@ -1138,12 +1167,9 @@ 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 cli.sourceRemovalError(err, srcSecretDir,
+			srcVault.Name+":"+srcSecretName, destVault.Name+":"+destSecretName,
+			versionCount)
 	}
 
 	cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",
@@ -1151,3 +1177,90 @@ func (cli *Instance) moveSecretCrossVault(
 
 	return nil
 }
+
+// checkCrossVaultMove checks that the secret srcSecretName exists in
+// srcVault, that destSecretName in destVault is not the source under
+// another name, and, unless force is set, that destSecretName does not
+// exist in destVault. It returns the source's directory.
+func (cli *Instance) checkCrossVaultMove(
+	srcVault *vault.Vault, srcSecretName string,
+	destVault *vault.Vault, destSecretName string,
+	force bool,
+) (string, error) {
+	srcVaultDir, err := srcVault.GetDirectory()
+	if err != nil {
+		return "", fmt.Errorf("failed to get source vault directory: %w", err)
+	}
+
+	// Verify source secret exists
+	srcStorageName := strings.ReplaceAll(srcSecretName, "/", "%")
+	srcSecretDir := filepath.Join(srcVaultDir, "secrets.d", srcStorageName)
+
+	exists, err := afero.DirExists(cli.fs, srcSecretDir)
+	if err != nil || !exists {
+		return "", fmt.Errorf("secret '%s' %w in vault '%s'",
+			srcSecretName, vault.ErrSecretNotFound, srcVault.Name)
+	}
+
+	// The source is removed after the copy, so a destination that is the
+	// source under another name would be lost with it.
+	destVaultDir, err := destVault.GetDirectory()
+	if err != nil {
+		return "", fmt.Errorf("failed to get destination vault directory: %w", err)
+	}
+
+	destStorageName := strings.ReplaceAll(destSecretName, "/", "%")
+	destSecretDir := filepath.Join(destVaultDir, "secrets.d", destStorageName)
+
+	same, err := cli.sameDirectory(srcSecretDir, destSecretDir)
+	if err != nil {
+		return "", err
+	}
+
+	if same {
+		return "", fmt.Errorf("secret '%s:%s' %w: '%s:%s' is the same secret "+
+			"on this filesystem", srcVault.Name, srcSecretName, errMoveOntoItself,
+			destVault.Name, destSecretName)
+	}
+
+	exists, err = afero.DirExists(cli.fs, destSecretDir)
+	if err != nil {
+		return "", fmt.Errorf("failed to check if destination secret exists: %w", err)
+	}
+
+	if exists && !force {
+		return "", fmt.Errorf(
+			"secret '%s' %w in vault '%s' (use --force to overwrite)",
+			destSecretName, vault.ErrSecretExists, destVault.Name)
+	}
+
+	return srcSecretDir, nil
+}
+
+// sourceRemovalError returns the error of a move between vaults that
+// copied all versionCount versions of the secret srcName to destName, and
+// then failed with removeErr to remove srcSecretDir, the directory of
+// srcName. The removal can fail after the source has left its vault, when
+// only the temporary directory it was moved into cannot be deleted, so the
+// error says the source remains only if srcSecretDir is still there.
+func (cli *Instance) sourceRemovalError(
+	removeErr error, srcSecretDir, srcName, destName string, versionCount int,
+) error {
+	remains, err := afero.DirExists(cli.fs, srcSecretDir)
+
+	switch {
+	case err != nil:
+		return fmt.Errorf("secret '%s' is copied with all %d version(s) "+
+			"to '%s', but removing the source failed and whether it "+
+			"remains is unknown: %w",
+			srcName, versionCount, destName, errors.Join(removeErr, err))
+	case remains:
+		return fmt.Errorf("secret '%s' is copied with all %d version(s) "+
+			"to '%s', but the source remains: %w",
+			srcName, versionCount, destName, removeErr)
+	default:
+		return fmt.Errorf("secret '%s' is moved with all %d version(s) "+
+			"to '%s', but its old files could not all be deleted: %w",
+			srcName, versionCount, destName, removeErr)
+	}
+}

internal/cli/secrets_size_test.go

@@ -36,17 +36,11 @@ func newSizeTestVault(t *testing.T) (afero.Fs, *vault.Vault) {
 
 	fs := afero.NewMemMapFs()
 
-	// Create vault
+	// Create vault, which selects it as the current vault
 	_, err := vault.CreateVault(fs, testStateDir, testVaultName,
 		testMnemonicBuffer(t), nil)
 	require.NoError(t, err)
 
-	// Set current vault
-	currentVaultPath := filepath.Join(testStateDir, "currentvault")
-	vaultPath := filepath.Join(testStateDir, "vaults.d", testVaultName)
-	err = afero.WriteFile(fs, currentVaultPath, []byte(vaultPath), 0o600)
-	require.NoError(t, err)
-
 	// Get vault and set up long-term key
 	vlt, err := vault.GetCurrentVault(fs, testStateDir)
 	require.NoError(t, err)

internal/cli/unlockers.go

@@ -15,6 +15,7 @@ import (
 	"strings"
 	"time"
 
+	"filippo.io/age"
 	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
 	"sneak.berlin/go/secret/internal/secret"
@@ -39,6 +40,10 @@ var (
 	// composes "GPG key <id> is already added as an unlocker".
 	errGPGKeyAlreadyUnlocker = errors.New(
 		"is already added as an unlocker")
+	// errVaultCreatedAgain carries only the message tail; the caller
+	// composes "vault '<name>' was removed and created again ...".
+	errVaultCreatedAgain = errors.New("was removed and created again " +
+		"with another key while it was being unlocked; nothing was changed")
 )
 
 // UnlockerInfo represents unlocker information for display
@@ -424,8 +429,11 @@ func (cli *Instance) UnlockersAdd(unlockerType string, cmd *cobra.Command) error
 // command waits while the user answers. Once the lock is held, it fails if
 // another command has removed the vault in the meantime, since writing the
 // unlocker would create the vault's directory again, holding nothing else.
+// It also fails if the vault was removed and created again with another
+// long-term key, which the unlocker would then hold instead of the new
+// vault's own.
 func (cli *Instance) unlockThenLock(vlt *vault.Vault) (func(), error) {
-	_, err := vlt.GetOrDeriveLongTermKey()
+	ltIdentity, err := vlt.GetOrDeriveLongTermKey()
 	if err != nil {
 		return nil, fmt.Errorf("failed to get long-term key: %w", err)
 	}
@@ -435,7 +443,7 @@ func (cli *Instance) unlockThenLock(vlt *vault.Vault) (func(), error) {
 		return nil, err
 	}
 
-	_, err = cli.existingVault(vlt.GetName())
+	err = cli.checkLongTermKey(vlt, ltIdentity)
 	if err != nil {
 		release()
 
@@ -445,6 +453,33 @@ func (cli *Instance) unlockThenLock(vlt *vault.Vault) (func(), error) {
 	return release, nil
 }
 
+// checkLongTermKey checks, for the vault vlt unlocked as ltIdentity before
+// the state directory lock was taken, that vlt still exists and that its
+// long-term public key is still that of ltIdentity. Another command may have
+// removed the vault in the meantime, or removed it and created it again with
+// another long-term key. The caller holds the lock.
+func (cli *Instance) checkLongTermKey(
+	vlt *vault.Vault, ltIdentity *age.X25519Identity,
+) error {
+	_, err := cli.existingVault(vlt.GetName())
+	if err != nil {
+		return err
+	}
+
+	vaultDir, _ := vlt.GetDirectory()
+
+	ltPubKey, err := afero.ReadFile(cli.fs, filepath.Join(vaultDir, "pub.age"))
+	if err != nil {
+		return fmt.Errorf("failed to read long-term public key: %w", err)
+	}
+
+	if string(ltPubKey) != ltIdentity.Recipient().String() {
+		return fmt.Errorf("vault '%s' %w", vlt.GetName(), errVaultCreatedAgain)
+	}
+
+	return nil
+}
+
 // autoSelectUnlocker selects the newly created unlocker as current,
 // printing a warning if selection fails
 func autoSelectUnlocker(cmd *cobra.Command, vlt *vault.Vault, unlockerID string) {

internal/secret/atomic.go

@@ -149,7 +149,9 @@ func WriteDir(fs afero.Fs, dir string, write func(dir string) error) error {
 
 // RemoveDirAtomic deletes the directory dir so that it disappears in one
 // rename: dir is moved into a new directory from TempDirFor, which is then
-// deleted. A crash part-way leaves only that temporary directory behind.
+// deleted. A crash part-way leaves only that temporary directory behind. If
+// only deleting the temporary directory fails, dir is already gone, and the
+// error names the temporary directory, which holds what is left of it.
 func RemoveDirAtomic(fs afero.Fs, dir string) error {
 	tmp, err := TempDirFor(fs, dir)
 	if err != nil {
@@ -165,7 +167,8 @@ func RemoveDirAtomic(fs afero.Fs, dir string) error {
 
 	err = fs.RemoveAll(tmp)
 	if err != nil {
-		return fmt.Errorf("failed to remove %s: %w", dir, err)
+		return fmt.Errorf("failed to remove %s after moving %s into it: %w",
+			tmp, dir, err)
 	}
 
 	return nil

internal/secret/secret.go

@@ -28,11 +28,6 @@ type VaultInterface interface {
 	GetFilesystem() afero.Fs
 	GetCurrentUnlocker() (Unlocker, error)
 	GetOrDeriveLongTermKey() (*age.X25519Identity, error)
-	// SetMnemonic and SetUnlockPassphrase give GetOrDeriveLongTermKey the
-	// mnemonic to derive the long-term key from, and the passphrase for a
-	// current passphrase unlocker; nil for none.
-	SetMnemonic(mnemonic *memguard.LockedBuffer)
-	SetUnlockPassphrase(passphrase *memguard.LockedBuffer)
 	CreatePassphraseUnlocker(
 		passphrase *memguard.LockedBuffer) (*PassphraseUnlocker, error)
 }

internal/secret/secret_test.go

@@ -118,12 +118,6 @@ func (m *MockVault) GetOrDeriveLongTermKey() (*age.X25519Identity, error) {
 	return nil, errNotImplementedInMock
 }
 
-func (m *MockVault) SetMnemonic(mnemonic *memguard.LockedBuffer) {
-	m.mnemonic = mnemonic
-}
-
-func (m *MockVault) SetUnlockPassphrase(_ *memguard.LockedBuffer) {}
-
 func (m *MockVault) CreatePassphraseUnlocker(
 	_ *memguard.LockedBuffer,
 ) (*PassphraseUnlocker, error) {

internal/secret/version_test.go

@@ -90,10 +90,6 @@ func (m *MockVersionVault) GetOrDeriveLongTermKey() (*age.X25519Identity, error)
 	return nil, errNotImplementedInMock
 }
 
-func (m *MockVersionVault) SetMnemonic(_ *memguard.LockedBuffer) {}
-
-func (m *MockVersionVault) SetUnlockPassphrase(_ *memguard.LockedBuffer) {}
-
 func (m *MockVersionVault) CreatePassphraseUnlocker(
 	_ *memguard.LockedBuffer,
 ) (*secret.PassphraseUnlocker, error) {

internal/vault/errors_test.go

@@ -132,7 +132,7 @@ func TestGetSecretWithWrongMnemonic(t *testing.T) {
 	t.Cleanup(mnemonic.Destroy)
 
 	vlt := vault.NewVault(created.GetFilesystem(), testStateDir, testVaultName)
-	vlt.SetMnemonic(mnemonic)
+	vlt.Mnemonic = mnemonic
 
 	_, err := vlt.GetSecret(testSecretName)
 	require.ErrorIs(t, err, vault.ErrMnemonicMismatch)

internal/vault/management.go

@@ -55,7 +55,10 @@ func ValidateVaultName(name string) error {
 }
 
 // ResolveVaultSymlink reads the currentvault file to get the path to the current vault
-// The file contains just the vault name (e.g., "default")
+// The file contains just the vault name (e.g., "default"). An empty or
+// invalid name, or the name of a vault that does not exist, is an error
+// wrapping ErrVaultNotFound that names the file and says to run
+// 'secret vault select <name>'.
 func ResolveVaultSymlink(fs afero.Fs, currentVaultPath string) (string, error) {
 	secret.Debug("resolveVaultSymlink starting", "path", currentVaultPath)
 
@@ -74,6 +77,21 @@ func ResolveVaultSymlink(fs afero.Fs, currentVaultPath string) (string, error) {
 	stateDir := filepath.Dir(currentVaultPath)
 	absolutePath := filepath.Join(stateDir, "vaults.d", vaultName)
 
+	// An empty name would make vaults.d itself the vault's directory
+	exists := false
+	if isValidVaultName(vaultName) {
+		exists, err = afero.DirExists(fs, absolutePath)
+		if err != nil {
+			return "", fmt.Errorf("failed to check if vault exists: %w", err)
+		}
+	}
+
+	if !exists {
+		return "", fmt.Errorf("vault '%s' named in %s %w; run "+
+			"'secret vault select <name>' to select a vault",
+			vaultName, currentVaultPath, ErrVaultNotFound)
+	}
+
 	secret.Debug("Resolved to absolute path", "absolute_path", absolutePath)
 
 	return absolutePath, nil

internal/vault/secrets_version_test.go

@@ -336,7 +336,7 @@ func TestVaultVersionTimestampsAfterUnlock(t *testing.T) {
 
 	// A new instance is locked; AddSecret unlocks it with the passphrase
 	vault := NewVault(fs, testStateDir, "test")
-	vault.SetUnlockPassphrase(passphrase)
+	vault.UnlockPassphrase = passphrase
 
 	addTestSecretToVault(t, vault, testSecretPath, []byte("version-2"), true)
 	require.False(t, fs.readAt.IsZero(), "AddSecret did not unlock the vault")

internal/vault/vault.go

@@ -64,18 +64,6 @@ func (v *Vault) ClearLongTermKey() {
 	v.longTermKey = nil
 }
 
-// SetMnemonic sets v.Mnemonic, for code that has v only as a
-// secret.VaultInterface.
-func (v *Vault) SetMnemonic(mnemonic *memguard.LockedBuffer) {
-	v.Mnemonic = mnemonic
-}
-
-// SetUnlockPassphrase sets v.UnlockPassphrase, for code that has v only as
-// a secret.VaultInterface.
-func (v *Vault) SetUnlockPassphrase(passphrase *memguard.LockedBuffer) {
-	v.UnlockPassphrase = passphrase
-}
-
 // GetOrDeriveLongTermKey gets the long-term key from memory or derives it
 // from available sources
 func (v *Vault) GetOrDeriveLongTermKey() (*age.X25519Identity, error) {

internal/vault/vault_error_test.go

@@ -128,7 +128,7 @@ func TestAddSecretForceWithWrongPassphraseChangesNothing(t *testing.T) {
 	t.Cleanup(wrong.Destroy)
 
 	vlt := vault.NewVault(fs, testStateDir, testVaultName)
-	vlt.SetUnlockPassphrase(wrong)
+	vlt.UnlockPassphrase = wrong
 
 	secretDir := filepath.Join(testStateDir, "vaults.d", testVaultName,
 		"secrets.d", testSecretName)