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,
unlocks both vaults, and only then takes the lock. Under the lock,
copySecretAndRemoveSource checks again that the source and the destination
vault still exist, then 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,11 +440,10 @@ 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`, `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.
+`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.
 
 ## Security Features
 

internal/cli/move_cross_vault_test.go

@@ -4,6 +4,7 @@ package cli
 import (
 	"errors"
 	"path/filepath"
+	"sync"
 	"testing"
 	"time"
 
@@ -229,55 +230,68 @@ func TestCrossVaultMoveUnlocksBeforeLocking(t *testing.T) {
 	}
 }
 
-// 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.
+// 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.
 //
 //nolint:paralleltest // waitingForLock sees any test's command waiting for the lock
 func TestCrossVaultMoveChecksAgainOnceLocked(t *testing.T) {
-	mnemonic := testMnemonicBuffer(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)
 
-	move := func(cli *Instance) error {
-		cli.Mnemonic = mnemonic
+			release, err := vault.LockStateDir(fs, testStateDir)
+			require.NoError(t, err)
 
-		return cli.MoveSecret(cli.cmd, "work:test/secret", "other:moved", false)
+			// 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")
+			}
+		})
 	}
-
-	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,11 +1095,10 @@ func (cli *Instance) moveSecretCrossVault(
 	force bool,
 ) error {
 	// Before unlocking, check that the source secret and the destination
-	// vault exist, that the destination is not the source under another
-	// name, and, without --force, that the destination secret does not
-	// exist
+	// vault exist, and that the destination is not the source under another
+	// name
 	_, err := cli.checkCrossVaultMove(
-		srcVault, srcSecretName, destVault, destSecretName, force)
+		srcVault, srcSecretName, destVault, destSecretName)
 	if err != nil {
 		return err
 	}
@@ -1141,7 +1140,7 @@ func (cli *Instance) copySecretAndRemoveSource(
 	force bool,
 ) error {
 	srcSecretDir, err := cli.checkCrossVaultMove(
-		srcVault, srcSecretName, destVault, destSecretName, force)
+		srcVault, srcSecretName, destVault, destSecretName)
 	if err != nil {
 		return err
 	}
@@ -1172,14 +1171,11 @@ func (cli *Instance) copySecretAndRemoveSource(
 }
 
 // checkCrossVaultMove checks that the secret srcSecretName exists in
-// 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.
+// srcVault, that destVault exists, and that destSecretName in destVault is
+// not the source under another name. 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 {
@@ -1222,17 +1218,6 @@ 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
 }