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

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,50 +1084,25 @@ 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; 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 {
-	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 and the destination
+	// vault exist, and that the destination is not the source under another
+	// name
+	_, err := cli.checkCrossVaultMove(
+		srcVault, srcSecretName, destVault, destSecretName)
 	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
 
@@ -1142,6 +1112,39 @@ func (cli *Instance) moveSecretCrossVault(
 		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()
+
+	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)
+	if err != nil {
+		return err
+	}
+
 	// Count versions for user feedback
 	versions, _ := secret.ListVersions(cli.fs, srcSecretDir)
 	versionCount := len(versions)
@@ -1167,6 +1170,57 @@ func (cli *Instance) moveSecretCrossVault(
 	return nil
 }
 
+// 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.
+func (cli *Instance) checkCrossVaultMove(
+	srcVault *vault.Vault, srcSecretName string,
+	destVault *vault.Vault, destSecretName string,
+) (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)
+	}
+
+	_, 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()
+	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)
+	}
+
+	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