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,
copySecretAndRemoveSource checks again before it copies. A move within a
vault takes the lock in moveSecretWithinVault, so MoveSecret takes none
itself.

The two tests of a source that cannot be removed now call
copySecretAndRemoveSource, since the lock refuses their wrapped
filesystems.

Model: opus-5-5

README.md

@@ -440,10 +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 of theirs left
-open in one terminal holds up no command in another.
+`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/move_cross_vault_test.go

@@ -4,7 +4,6 @@ package cli
 import (
 	"errors"
 	"path/filepath"
-	"sync"
 	"testing"
 	"time"
 
@@ -230,68 +229,55 @@ func TestCrossVaultMoveUnlocksBeforeLocking(t *testing.T) {
 	}
 }
 
-// TestCrossVaultMoveChecksAgainOnceLocked starts a move between two vaults,
-// given the mnemonic, while the test holds the state directory lock. Once
-// the move waits for the lock, the test removes what another command could
-// have removed in the meantime, then releases the lock. The move must fail
-// with the error that says what is missing.
+// 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, 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) {
-	for _, tc := range []struct {
-		name string
-		// removed is the directory, under the state directory, that the
-		// test removes
-		removed string
-		want    error
-	}{
-		{"source", "/vaults.d/default/secrets.d/x", vault.ErrSecretNotFound},
-		{"destination vault", "/vaults.d/work", vault.ErrVaultNotFound},
-	} {
-		t.Run(tc.name, func(t *testing.T) {
-			fs := afero.NewMemMapFs()
-			createCrossVaults(t, fs)
+	mnemonic := testMnemonicBuffer(t)
 
-			release, err := vault.LockStateDir(fs, testStateDir)
-			require.NoError(t, err)
+	move := func(cli *Instance) error {
+		cli.Mnemonic = mnemonic
 
-			// Released at most once, and also if the test fails while
-			// holding it, so that later tests can take it
-			release = sync.OnceFunc(release)
-			defer release()
-
-			instance := NewCLIInstanceWithStateDir(fs, testStateDir)
-			instance.Mnemonic = testMnemonicBuffer(t)
-
-			done := make(chan error, 1)
-
-			go func() {
-				done <- instance.MoveSecret(&cobra.Command{},
-					"default:x", "work:y", false)
-			}()
-
-			timeout := time.After(lockWait)
-
-			for !waitingForLock() {
-				select {
-				case err := <-done:
-					t.Fatalf("finished while the lock was held, with error %v",
-						err)
-				case <-timeout:
-					t.Fatal("never waited for the lock")
-				case <-time.After(time.Millisecond):
-				}
-			}
-
-			require.NoError(t, fs.RemoveAll(testStateDir+tc.removed))
-			release()
-
-			select {
-			case err := <-done:
-				require.ErrorIs(t, err, tc.want)
-			case <-time.After(lockWait):
-				t.Fatal("did not finish once the lock was released")
-			}
-		})
+		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)
+	})
+}
+
+// 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

@@ -1095,10 +1095,11 @@ func (cli *Instance) moveSecretCrossVault(
 	force bool,
 ) error {
 	// Before unlocking, check that the source secret and the destination
-	// vault exist, and that the destination is not the source under another
-	// name
+	// vault exist, 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)
+		srcVault, srcSecretName, destVault, destSecretName, force)
 	if err != nil {
 		return err
 	}
@@ -1140,7 +1141,7 @@ func (cli *Instance) copySecretAndRemoveSource(
 	force bool,
 ) error {
 	srcSecretDir, err := cli.checkCrossVaultMove(
-		srcVault, srcSecretName, destVault, destSecretName)
+		srcVault, srcSecretName, destVault, destSecretName, force)
 	if err != nil {
 		return err
 	}
@@ -1171,11 +1172,14 @@ func (cli *Instance) copySecretAndRemoveSource(
 }
 
 // checkCrossVaultMove checks that the secret srcSecretName exists in
-// srcVault, that destVault exists, and that destSecretName in destVault is
-// not the source under another name. It returns the source's directory.
+// srcVault, that destVault exists, 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 {
@@ -1218,6 +1222,17 @@ func (cli *Instance) checkCrossVaultMove(
 			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
 }