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,87 +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, or creates the destination
-// vault again from another mnemonic, 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)
 
-	otherMnemonic := memguard.NewBufferFromBytes([]byte("legal winner thank " +
-		"year wave sausage worth useful legal winner thank yellow"))
-	t.Cleanup(otherMnemonic.Destroy)
+			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()
 
-		return cli.MoveSecret(cli.cmd, "work:test/secret", "other:moved", false)
-	}
+			instance := NewCLIInstanceWithStateDir(fs, testStateDir)
+			instance.Mnemonic = testMnemonicBuffer(t)
 
-	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)
-	})
+			done := make(chan error, 1)
 
-	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)
-	})
+			go func() {
+				done <- instance.MoveSecret(&cobra.Command{},
+					"default:x", "work:y", false)
+			}()
 
-	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
+			timeout := time.After(lockWait)
 
-		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
+			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):
+				}
 			}
 
-			_, err = vault.CreateVault(fs, testStateDir, "other", otherMnemonic, nil)
+			require.NoError(t, fs.RemoveAll(testStateDir+tc.removed))
+			release()
 
-			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)
+			select {
+			case err := <-done:
+				require.ErrorIs(t, err, tc.want)
+			case <-time.After(lockWait):
+				t.Fatal("did not finish once the lock was released")
+			}
+		})
+	}
 }

internal/cli/secrets.go

@@ -1084,24 +1084,21 @@ 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. 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.
+// vaults exist. The copy needs the source vault's long-term key; the
+// destination vault is unlocked only to check that it can be. Either can ask
+// for a passphrase, so both happen before it takes the state directory lock,
+// 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 {
-	// 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
+	// Before unlocking, check that the source secret and the destination
+	// 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
 	}
@@ -1110,7 +1107,7 @@ func (cli *Instance) moveSecretCrossVault(
 	destVault.Mnemonic, destVault.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
 
 	// Unlock destination vault (will fail if neither mnemonic nor unlocker available)
-	destIdentity, err := destVault.GetOrDeriveLongTermKey()
+	_, err = destVault.GetOrDeriveLongTermKey()
 	if err != nil {
 		return fmt.Errorf("failed to unlock destination vault '%s': %w", destVault.Name, err)
 	}
@@ -1127,11 +1124,6 @@ func (cli *Instance) moveSecretCrossVault(
 	}
 	defer release()
 
-	err = cli.checkLongTermKey(destVault, destIdentity)
-	if err != nil {
-		return err
-	}
-
 	return cli.copySecretAndRemoveSource(
 		cmd, srcVault, srcSecretName, destVault, destSecretName, force)
 }
@@ -1148,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
 	}
@@ -1179,13 +1171,11 @@ func (cli *Instance) copySecretAndRemoveSource(
 }
 
 // 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.
+// 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 {
@@ -1202,6 +1192,11 @@ func (cli *Instance) checkCrossVaultMove(
 			srcSecretName, vault.ErrSecretNotFound, srcVault.Name)
 	}
 
+	_, err = cli.existingVault(destVault.Name)
+	if err != nil {
+		return "", err
+	}
+
 	// 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()
@@ -1223,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
 }
 

internal/cli/unlockers.go

@@ -15,7 +15,6 @@ import (
 	"strings"
 	"time"
 
-	"filippo.io/age"
 	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
 	"sneak.berlin/go/secret/internal/secret"
@@ -43,7 +42,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 changed")
+		"with another key while it was being unlocked; nothing was added")
 )
 
 // UnlockerInfo represents unlocker information for display
@@ -443,41 +442,29 @@ func (cli *Instance) unlockThenLock(vlt *vault.Vault) (func(), error) {
 		return nil, err
 	}
 
-	err = cli.checkLongTermKey(vlt, ltIdentity)
+	_, err = cli.existingVault(vlt.GetName())
 	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 {
-		return fmt.Errorf("failed to read long-term public key: %w", err)
+		release()
+
+		return nil, 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)
+		release()
+
+		return nil, fmt.Errorf("vault '%s' %w", vlt.GetName(), errVaultCreatedAgain)
 	}
 
-	return nil
+	return release, nil
 }
 
 // autoSelectUnlocker selects the newly created unlocker as current,