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".

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

@@ -5,11 +5,13 @@ 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"
 )
 
@@ -92,7 +94,8 @@ func requireValue(t *testing.T, fs afero.Fs, vaultName, secretName string) {
 // 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.
+// calls copySecretAndRemoveSource, which a move between vaults runs once it
+// holds the lock.
 func TestCrossVaultMoveFailsWhenSourceRemains(t *testing.T) {
 	t.Parallel()
 
@@ -103,9 +106,12 @@ func TestCrossVaultMoveFailsWhenSourceRemains(t *testing.T) {
 		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)
+	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")
@@ -128,9 +134,12 @@ func TestCrossVaultMoveFailsWhenOldFilesRemain(t *testing.T) {
 	fs := &removeAllFailFs{Fs: base, dir: sourceVaultDir}
 
 	instance := NewCLIInstanceWithStateDir(fs, testStateDir)
-	instance.Mnemonic = testMnemonicBuffer(t)
 
-	err := instance.moveSecret(&cobra.Command{}, "default:x", "work:y", false)
+	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")
@@ -150,3 +159,157 @@ func TestCrossVaultMoveFailsWhenOldFilesRemain(t *testing.T) {
 
 	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

@@ -875,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)
@@ -992,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 {
@@ -1003,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
@@ -1089,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)
@@ -1167,6 +1178,65 @@ 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

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"
@@ -42,7 +43,7 @@ var (
 	// 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 added")
+		"with another key while it was being unlocked; nothing was changed")
 )
 
 // UnlockerInfo represents unlocker information for display
@@ -442,29 +443,41 @@ 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()
 
 		return nil, err
 	}
 
+	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 {
-		release()
-
-		return nil, fmt.Errorf("failed to read long-term public key: %w", err)
+		return fmt.Errorf("failed to read long-term public key: %w", err)
 	}
 
 	if string(ltPubKey) != ltIdentity.Recipient().String() {
-		release()
-
-		return nil, fmt.Errorf("vault '%s' %w", vlt.GetName(), errVaultCreatedAgain)
+		return fmt.Errorf("vault '%s' %w", vlt.GetName(), errVaultCreatedAgain)
 	}
 
-	return release, nil
+	return nil
 }
 
 // autoSelectUnlocker selects the newly created unlocker as current,