README.md

@@ -438,6 +438,13 @@ asks when stdin is a pipe, as in `echo value | secret add name --force` or
 `cat file | secret encrypt key`. With no controlling terminal, as under cron or
 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.
+
 ## Security Features
 
 ### Encryption

internal/cli/generate.go

@@ -175,12 +175,6 @@ func (cli *Instance) GenerateSecret(
 		return fmt.Errorf("failed to generate random secret: %w", err)
 	}
 
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Store the secret in the vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -193,7 +187,7 @@ func (cli *Instance) GenerateSecret(
 	secretBuffer := memguard.NewBufferFromBytes([]byte(secretValue))
 	defer secretBuffer.Destroy()
 
-	err = vlt.AddSecret(secretName, secretBuffer, force)
+	err = cli.storeSecret(vlt, secretName, secretBuffer, force)
 	if err != nil {
 		return err
 	}

internal/cli/init.go

@@ -67,36 +67,15 @@ func (cli *Instance) promptMnemonic() (*memguard.LockedBuffer, func(), error) {
 	return mnemonicBuffer, mnemonicBuffer.Destroy, nil
 }
 
-// Init initializes the secret manager, holding the state directory lock
-// while initialize runs
+// Init initializes the secret manager. It asks for the mnemonic and the
+// passphrase before it takes the state directory lock, so that no other
+// command waits while the user answers, and holds the lock while
+// initialize runs.
 func (cli *Instance) Init(cmd *cobra.Command) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
-	return cli.initialize(cmd)
-}
-
-// initialize creates the state directory, the default vault and its first
-// unlocker
-func (cli *Instance) initialize(cmd *cobra.Command) error {
 	secret.Debug("Starting secret manager initialization")
 
-	// Create state directory
-	stateDir := cli.GetStateDir()
-	secret.DebugWith("Creating state directory", slog.String("path", stateDir))
-
-	err := cli.fs.MkdirAll(stateDir, secret.DirPerms)
-	if err != nil {
-		secret.Debug("Failed to create state directory", "error", err)
-
-		return fmt.Errorf("failed to create state directory: %w", err)
-	}
-
 	// Refuse an existing default vault before asking for anything
-	err = vault.CheckNewVaultName(cli.fs, cli.stateDir, defaultVaultName)
+	err := vault.CheckNewVaultName(cli.fs, cli.stateDir, defaultVaultName)
 	if err != nil {
 		return fmt.Errorf("failed to create default vault: %w", err)
 	}
@@ -135,11 +114,38 @@ func (cli *Instance) initialize(cmd *cobra.Command) error {
 	}
 	defer cleanupPassphrase()
 
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	return cli.initialize(cmd, mnemonic, passphraseBuffer)
+}
+
+// initialize creates the state directory, and the default vault from
+// mnemonic with a passphrase unlocker protected by passphrase. Another
+// command may have created the vault since Init checked: vault.CreateVault
+// checks again.
+func (cli *Instance) initialize(
+	cmd *cobra.Command, mnemonic, passphrase *memguard.LockedBuffer,
+) error {
+	// Create state directory
+	stateDir := cli.GetStateDir()
+	secret.DebugWith("Creating state directory", slog.String("path", stateDir))
+
+	err := cli.fs.MkdirAll(stateDir, secret.DirPerms)
+	if err != nil {
+		secret.Debug("Failed to create state directory", "error", err)
+
+		return fmt.Errorf("failed to create state directory: %w", err)
+	}
+
 	// Create the default vault with its passphrase unlocker
 	secret.Debug("Creating default vault")
 
 	vlt, err := vault.CreateVault(cli.fs, cli.stateDir, defaultVaultName,
-		mnemonic, passphraseBuffer)
+		mnemonic, passphrase)
 	if err != nil {
 		secret.Debug("Failed to create default vault", "error", err)
 

internal/cli/lock_test.go

@@ -280,9 +280,9 @@ func stateDirModTimes(t *testing.T, fs afero.Fs) map[string]int64 {
 	return modTimes
 }
 
-// setupEveryCommand makes what each command in
-// TestChangingCommandsWaitForLock needs: the current vault "work" with two
-// versions of "test/secret", the vault "other", and the file testInput.
+// setupEveryCommand makes what the commands that the lock tests below run
+// need: the current vault "work" with two versions of "test/secret", the
+// vault "other", and the file testInput.
 // There is no vault "default", which init creates. If withUnlocker is set,
 // it also gives "work" a passphrase unlocker, which is slow. It returns the
 // older version and the unlocker's ID.
@@ -346,6 +346,25 @@ func waitingForLock() bool {
 	return false
 }
 
+// requireWaitingForLock returns once a command waits for the state
+// directory lock. It fails the test if the command finishes first, sending
+// its error on done, or never waits.
+func requireWaitingForLock(t *testing.T, done <-chan error) {
+	t.Helper()
+
+	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):
+		}
+	}
+}
+
 // requireWaitsForLock runs a command, given what setupEveryCommand made,
 // while holding the state directory lock. The command must neither finish
 // nor change anything before it waits for the lock, and must succeed once
@@ -385,17 +404,7 @@ func requireWaitsForLock(
 
 	go func() { done <- run(cli, olderVersion, unlockerID) }()
 
-	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):
-		}
-	}
+	requireWaitingForLock(t, done)
 
 	assert.Equal(t, before, stateDirModTimes(t, fs),
 		"changed the state directory before waiting for the lock")
@@ -472,6 +481,311 @@ func TestChangingCommandsWaitForLock(t *testing.T) {
 	}
 }
 
+// TestCommandsAskBeforeLocking is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/152: these commands took the
+// state directory lock before asking for the mnemonic or a passphrase, so
+// while one waited at its prompt, every other command that changes anything
+// waited for it. Each runs here while the test holds the lock, given what
+// takes it to the last step at which it can ask: the passphrase prompt of
+// init and vault create, and the unlock of the vault, which asks for the
+// passphrase of its passphrase unlocker, for the others. The tests have no
+// terminal, so a prompt fails at once. Each command must get there, and
+// fail, without waiting for the lock.
+//
+//nolint:paralleltest // waitingForLock sees any test's command waiting for the lock
+func TestCommandsAskBeforeLocking(t *testing.T) {
+	newTestGPGKey(t)
+
+	mnemonic := testMnemonicBuffer(t)
+
+	wrongPassphrase := memguard.NewBufferFromBytes([]byte("wrong-passphrase"))
+	t.Cleanup(wrongPassphrase.Destroy)
+
+	for _, tc := range []struct {
+		name string
+		run  func(cli *Instance) error
+		// want is wrapped in the error the command fails with
+		want error
+	}{
+		{"init", func(cli *Instance) error {
+			cli.Mnemonic = mnemonic
+
+			return cli.Init(cli.cmd)
+		}, secret.ErrNoTerminal},
+		{"vault create", func(cli *Instance) error {
+			cli.Mnemonic = mnemonic
+
+			return cli.CreateVault(cli.cmd, "created")
+		}, secret.ErrNoTerminal},
+		// With no terminal to ask for the new passphrase on, the new
+		// unlocker takes the one in SB_UNLOCK_PASSPHRASE, which then fails
+		// to unlock the vault
+		{"unlocker add passphrase", func(cli *Instance) error {
+			cli.UnlockPassphrase = wrongPassphrase
+
+			return cli.UnlockersAdd("passphrase", cli.cmd)
+		}, secret.ErrWrongPassphrase},
+		{"unlocker add pgp", func(cli *Instance) error {
+			cli.cmd.Flags().String("keyid", unreadableTestGPGUserID, "")
+
+			return cli.UnlockersAdd("pgp", cli.cmd)
+		}, secret.ErrNoTerminal},
+		{"add --force", func(cli *Instance) error {
+			return cli.AddSecret("test/secret", true)
+		}, secret.ErrNoTerminal},
+		{"import --force", func(cli *Instance) error {
+			return cli.ImportSecret(cli.cmd, "test/secret", testInput, true)
+		}, secret.ErrNoTerminal},
+		{"generate secret --force", func(cli *Instance) error {
+			return cli.GenerateSecret(cli.cmd, "test/secret", 16, "base58", true)
+		}, secret.ErrNoTerminal},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			requireAsksBeforeLocking(t, tc.run, tc.want)
+		})
+	}
+}
+
+// startWhileLocked makes what setupEveryCommand makes, with the passphrase
+// unlocker, on fs, an empty in-memory filesystem, takes the state directory
+// lock there, and starts run, given a CLI instance on fs with "value" on
+// stdin. It returns the function that releases the lock, which does so only
+// the first time it is called, and the channel that run's error comes on.
+// The lock is also released when the test ends, so that later tests can
+// take it.
+func startWhileLocked(
+	t *testing.T, fs afero.Fs, run func(cli *Instance) error,
+) (func(), <-chan error) {
+	t.Helper()
+
+	setupEveryCommand(t, fs, true)
+
+	release, err := vault.LockStateDir(fs, testStateDir)
+	require.NoError(t, err)
+
+	release = sync.OnceFunc(release)
+	t.Cleanup(release)
+
+	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
+	cli.cmd = &cobra.Command{}
+	cli.cmd.SetIn(strings.NewReader("value"))
+	cli.cmd.SetOut(io.Discard)
+
+	done := make(chan error, 1)
+
+	go func() { done <- run(cli) }()
+
+	return release, done
+}
+
+// requireAsksBeforeLocking starts run while holding the state directory
+// lock, as startWhileLocked does. run must fail, with an error wrapping
+// want, without waiting for the lock.
+func requireAsksBeforeLocking(
+	t *testing.T, run func(cli *Instance) error, want error,
+) {
+	t.Helper()
+
+	release, done := startWhileLocked(t, afero.NewMemMapFs(), run)
+
+	timeout := time.After(lockWait)
+
+	for !waitingForLock() {
+		select {
+		case err := <-done:
+			require.ErrorIs(t, err, want)
+
+			return
+		case <-timeout:
+			t.Fatal("neither failed nor waited for the lock")
+		case <-time.After(time.Millisecond):
+		}
+	}
+
+	// Let the command go on, so that it releases the lock
+	release()
+
+	select {
+	case <-done:
+	case <-time.After(lockWait):
+	}
+
+	t.Fatal("waited for the lock before asking")
+}
+
+// TestCommandsCheckAgainOnceLocked checks that commands which check what
+// they change before they ask for anything, and so before they take the
+// state directory lock, check again once they hold it: another command may
+// have changed it in the meantime. Each runs while the test holds the lock,
+// given what it needs to ask for nothing. Once it waits for the lock, the
+// test makes the change another command could have made, then releases the
+// lock. The command must fail, and change nothing.
+//
+//nolint:paralleltest // waitingForLock sees any test's command waiting for the lock
+func TestCommandsCheckAgainOnceLocked(t *testing.T) {
+	fingerprint := newTestGPGKey(t)
+
+	mnemonic := testMnemonicBuffer(t)
+
+	otherMnemonic := memguard.NewBufferFromBytes([]byte("legal winner thank " +
+		"year wave sausage worth useful legal winner thank yellow"))
+	t.Cleanup(otherMnemonic.Destroy)
+
+	passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
+	t.Cleanup(passphrase.Destroy)
+
+	// work returns the vault "work" on fs, opened with the mnemonic
+	work := func(fs afero.Fs) *vault.Vault {
+		vlt := vault.NewVault(fs, testStateDir, "work")
+		vlt.Mnemonic = mnemonic
+
+		return vlt
+	}
+
+	for _, tc := range []struct {
+		name string
+		run  func(cli *Instance) error
+		// change is what another command changes while run waits
+		change func(fs afero.Fs) error
+		// want is wrapped in the error the command fails with
+		want error
+	}{
+		{
+			"unlocker add, vault removed",
+			func(cli *Instance) error {
+				cli.Mnemonic, cli.UnlockPassphrase = mnemonic, passphrase
+
+				return cli.UnlockersAdd("passphrase", cli.cmd)
+			},
+			func(fs afero.Fs) error {
+				return fs.RemoveAll(testStateDir + "/vaults.d/work")
+			},
+			vault.ErrVaultNotFound,
+		},
+		{
+			"unlocker add, vault created again from another mnemonic",
+			func(cli *Instance) error {
+				cli.Mnemonic, cli.UnlockPassphrase = mnemonic, passphrase
+
+				return cli.UnlockersAdd("passphrase", cli.cmd)
+			},
+			func(fs afero.Fs) error {
+				err := fs.RemoveAll(testStateDir + "/vaults.d/work")
+				if err != nil {
+					return err
+				}
+
+				_, err = vault.CreateVault(fs, testStateDir, "work",
+					otherMnemonic, passphrase)
+
+				return err
+			},
+			errVaultCreatedAgain,
+		},
+		{
+			"unlocker add pgp, key added",
+			func(cli *Instance) error {
+				cli.Mnemonic = mnemonic
+				cli.cmd.Flags().String("keyid", unreadableTestGPGUserID, "")
+
+				return cli.UnlockersAdd("pgp", cli.cmd)
+			},
+			func(fs afero.Fs) error {
+				_, err := secret.CreatePGPUnlocker(work(fs),
+					unreadableTestGPGUserID, fingerprint)
+
+				return err
+			},
+			errGPGKeyAlreadyUnlocker,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			requireChecksAgainOnceLocked(t, tc.run, tc.change, tc.want)
+		})
+	}
+}
+
+// requireChecksAgainOnceLocked starts run while holding the state directory
+// lock, as startWhileLocked does. Once run waits for the lock, change makes
+// the change that another command could have made in the meantime, and the
+// lock is released. run must then fail, with an error wrapping want, and
+// change nothing.
+func requireChecksAgainOnceLocked(
+	t *testing.T, run func(cli *Instance) error,
+	change func(fs afero.Fs) error, want error,
+) {
+	t.Helper()
+
+	fs := afero.NewMemMapFs()
+	release, done := startWhileLocked(t, fs, run)
+
+	requireWaitingForLock(t, done)
+	require.NoError(t, change(fs))
+
+	// Left out: taking and releasing the lock writes the lock file
+	lockFile := testStateDir + "/lock"
+
+	before := stateDirModTimes(t, fs)
+	delete(before, lockFile)
+
+	release()
+
+	select {
+	case err := <-done:
+		require.ErrorIs(t, err, want)
+	case <-time.After(lockWait):
+		t.Fatal("did not finish once the lock was released")
+	}
+
+	after := stateDirModTimes(t, fs)
+	delete(after, lockFile)
+
+	assert.Equal(t, before, after, "changed the state directory")
+}
+
+// TestForcedAddReplacesSecretAddedMeanwhile runs a forced add of a secret
+// that does not exist yet while the test holds the state directory lock.
+// Once it waits for the lock, the test adds the secret, as another command
+// could have, and releases the lock. The forced add must replace that
+// secret, as if it had run after the other command.
+//
+//nolint:paralleltest // waitingForLock sees any test's command waiting for the lock
+func TestForcedAddReplacesSecretAddedMeanwhile(t *testing.T) {
+	mnemonic := testMnemonicBuffer(t)
+
+	fs := afero.NewMemMapFs()
+	release, done := startWhileLocked(t, fs, func(cli *Instance) error {
+		cli.Mnemonic = mnemonic
+
+		return cli.AddSecret("added", true)
+	})
+
+	requireWaitingForLock(t, done)
+
+	work := vault.NewVault(fs, testStateDir, "work")
+	work.Mnemonic = mnemonic
+
+	other := memguard.NewBufferFromBytes([]byte("other"))
+	t.Cleanup(other.Destroy)
+
+	require.NoError(t, work.AddSecret("added", other, false))
+
+	release()
+
+	select {
+	case err := <-done:
+		require.NoError(t, err)
+	case <-time.After(lockWait):
+		t.Fatal("did not finish once the lock was released")
+	}
+
+	value, err := work.GetSecret("added")
+	require.NoError(t, err)
+	t.Cleanup(value.Destroy)
+
+	assert.Equal(t, "value", string(value.Bytes()))
+}
+
 // TestEncryptWithExistingKeyTakesNoLock checks that secret encrypt with a
 // key that already exists, which only reads the state directory, finishes
 // while another command holds the state directory lock.

internal/cli/secrets.go

@@ -422,26 +422,92 @@ func (cli *Instance) AddSecret(secretName string, force bool) error {
 	// Locked only now that stdin has been read: in `secret encrypt key |
 	// secret add name`, holding the lock while reading would leave each
 	// command waiting for the other.
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	return cli.storeSecret(vlt, secretName, valueBuffer, force)
+}
+
+// storeSecret adds the secret name with value to vlt, as vault.AddSecret
+// does, holding the state directory lock while it does. With force, adding
+// an existing secret replaces its current version, which needs the vault's
+// long-term key. Getting the key can ask for the passphrase, so storeSecret
+// gets it before it takes the lock, and no other command waits while the
+// user answers. If another command adds the secret while this one waits for
+// the lock, storeSecret lets go of the lock, gets the key, and takes the
+// lock again.
+func (cli *Instance) storeSecret(
+	vlt *vault.Vault, name string, value *memguard.LockedBuffer, force bool,
+) error {
+	err := vault.ValidateSecretName(name)
 	if err != nil {
 		return err
 	}
+
+	replacing, err := replacesVersion(vlt, name, force)
+	if err != nil {
+		return err
+	}
+
+	if replacing {
+		_, err = vlt.GetOrDeriveLongTermKey()
+		if err != nil {
+			return fmt.Errorf("failed to get long-term key: %w", err)
+		}
+	}
+
+	needsKey, err := cli.addWithoutAsking(vlt, name, value, force)
+	if !needsKey {
+		return err
+	}
+
+	_, err = vlt.GetOrDeriveLongTermKey()
+	if err != nil {
+		return fmt.Errorf("failed to get long-term key: %w", err)
+	}
+
+	// With the key in memory, this adds the secret
+	_, err = cli.addWithoutAsking(vlt, name, value, force)
+
+	return err
+}
+
+// addWithoutAsking takes the state directory lock and adds the secret name
+// with value to vlt, as vault.AddSecret does. If that would replace the
+// secret's current version while the vault's long-term key is not in
+// memory, getting the key could ask for the passphrase with the lock held:
+// addWithoutAsking then adds nothing and returns true.
+func (cli *Instance) addWithoutAsking(
+	vlt *vault.Vault, name string, value *memguard.LockedBuffer, force bool,
+) (bool, error) {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return false, err
+	}
 	defer release()
 
-	// Add the secret to the vault
-	secret.Debug("Calling vault.AddSecret", "secret_name", secretName,
-		"value_length", valueBuffer.Size(), "force", force)
-
-	err = vlt.AddSecret(secretName, valueBuffer, force)
+	replacing, err := replacesVersion(vlt, name, force)
 	if err != nil {
-		secret.Debug("vault.AddSecret failed", "error", err)
-
-		return err
+		return false, err
 	}
 
-	secret.Debug("vault.AddSecret completed successfully")
+	if replacing && vlt.Locked() {
+		return true, nil
+	}
 
-	return nil
+	return false, vlt.AddSecret(name, value, force)
+}
+
+// replacesVersion reports whether adding the secret name to vlt replaces
+// the secret's current version: whether force is set and the secret has one.
+func replacesVersion(vlt *vault.Vault, name string, force bool) (bool, error) {
+	if !force {
+		return false, nil
+	}
+
+	exists, err := secret.NewSecret(vlt, name).Exists()
+	if err != nil {
+		return false, fmt.Errorf("failed to check if secret exists: %w", err)
+	}
+
+	return exists, nil
 }
 
 // GetSecret retrieves and prints the current version of a secret
@@ -707,14 +773,7 @@ func (cli *Instance) ImportSecret(
 
 	// Locked only now that the file has been read, as in AddSecret: the
 	// file may be a pipe written by another secret command.
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
-	// Store the secret in the vault
-	err = vlt.AddSecret(secretName, valueBuffer, force)
+	err = cli.storeSecret(vlt, secretName, valueBuffer, force)
 	if err != nil {
 		return err
 	}

internal/cli/unlockers.go

@@ -39,6 +39,10 @@ var (
 	// composes "GPG key <id> is already added as an unlocker".
 	errGPGKeyAlreadyUnlocker = errors.New(
 		"is already added as an unlocker")
+	// 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")
 )
 
 // UnlockerInfo represents unlocker information for display
@@ -404,12 +408,6 @@ func (cli *Instance) printUnlockersTable(unlockers []UnlockerInfo) error {
 
 // UnlockersAdd adds a new unlocker
 func (cli *Instance) UnlockersAdd(unlockerType string, cmd *cobra.Command) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	switch unlockerType {
 	case unlockerTypePassphrase:
 		return cli.addPassphraseUnlocker(cmd)
@@ -423,6 +421,52 @@ func (cli *Instance) UnlockersAdd(unlockerType string, cmd *cobra.Command) error
 	}
 }
 
+// unlockThenLock gets the long-term key of vlt, the vault an unlocker is
+// added to, then takes the state directory lock and returns the function
+// that releases it. Getting the key can ask for a passphrase, so it comes
+// before the lock, as the question of askThenLock does, and no other
+// command waits while the user answers. Once the lock is held, it fails if
+// another command has removed the vault in the meantime, since writing the
+// unlocker would create the vault's directory again, holding nothing else.
+// It also fails if the vault was removed and created again with another
+// long-term key, which the unlocker would then hold instead of the new
+// vault's own.
+func (cli *Instance) unlockThenLock(vlt *vault.Vault) (func(), error) {
+	ltIdentity, err := vlt.GetOrDeriveLongTermKey()
+	if err != nil {
+		return nil, fmt.Errorf("failed to get long-term key: %w", err)
+	}
+
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return nil, err
+	}
+
+	_, err = cli.existingVault(vlt.GetName())
+	if err != nil {
+		release()
+
+		return nil, 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)
+	}
+
+	if string(ltPubKey) != ltIdentity.Recipient().String() {
+		release()
+
+		return nil, fmt.Errorf("vault '%s' %w", vlt.GetName(), errVaultCreatedAgain)
+	}
+
+	return release, nil
+}
+
 // autoSelectUnlocker selects the newly created unlocker as current,
 // printing a warning if selection fails
 func autoSelectUnlocker(cmd *cobra.Command, vlt *vault.Vault, unlockerID string) {
@@ -442,9 +486,6 @@ func (cli *Instance) addPassphraseUnlocker(cmd *cobra.Command) error {
 		return fmt.Errorf("failed to get current vault: %w", err)
 	}
 
-	// For passphrase unlockers, we don't need the vault to be unlocked
-	// The CreatePassphraseUnlocker method will handle getting the
-	// long-term key
 	vlt.Mnemonic, vlt.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
 
 	// The new passphrase is asked for even when SB_UNLOCK_PASSPHRASE is
@@ -464,6 +505,12 @@ func (cli *Instance) addPassphraseUnlocker(cmd *cobra.Command) error {
 		defer passphraseBuffer.Destroy()
 	}
 
+	release, err := cli.unlockThenLock(vlt)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	passphraseUnlocker, err := vlt.CreatePassphraseUnlocker(passphraseBuffer)
 	if err != nil {
 		return err
@@ -502,12 +549,13 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error {
 		cmd.Printf("Using default GPG key: %s\n", gpgKeyID)
 	}
 
-	// Check if this key is already added as an unlocker
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
 		return fmt.Errorf("failed to get current vault: %w", err)
 	}
 
+	vlt.Mnemonic, vlt.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
+
 	// Resolve the GPG key ID to its fingerprint, once: the duplicate check
 	// and the new unlocker's metadata both use this result
 	fingerprint, err := secret.ResolveGPGKeyFingerprint(gpgKeyID)
@@ -515,21 +563,26 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error {
 		return fmt.Errorf("failed to resolve GPG key fingerprint: %w", err)
 	}
 
-	// Check if this GPG key is already added
-	exists, err := cli.pgpUnlockerExists(vlt, fingerprint)
+	// Refuse a key that is already an unlocker before asking for anything,
+	// and again once the lock is held, since another command may have added
+	// it in the meantime
+	err = cli.checkPGPKeyNotAdded(vlt, gpgKeyID, fingerprint)
 	if err != nil {
-		return fmt.Errorf(
-			"could not check whether GPG key %s is already an unlocker: %w",
-			gpgKeyID, err,
-		)
+		return err
 	}
 
-	if exists {
-		return fmt.Errorf("GPG key %s %w", gpgKeyID, errGPGKeyAlreadyUnlocker)
+	release, err := cli.unlockThenLock(vlt)
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	err = cli.checkPGPKeyNotAdded(vlt, gpgKeyID, fingerprint)
+	if err != nil {
+		return err
 	}
 
-	pgpUnlocker, err := secret.CreatePGPUnlocker(cli.fs, cli.stateDir,
-		gpgKeyID, fingerprint, cli.Mnemonic, cli.UnlockPassphrase)
+	pgpUnlocker, err := secret.CreatePGPUnlocker(vlt, gpgKeyID, fingerprint)
 	if err != nil {
 		return err
 	}
@@ -704,6 +757,27 @@ func (cli *Instance) UnlockerSelect(unlockerID string) error {
 	return vlt.SelectUnlocker(unlockerID)
 }
 
+// checkPGPKeyNotAdded fails when vlt already has a PGP unlocker for the GPG
+// key gpgKeyID, whose fingerprint is fingerprint, or when that cannot be
+// checked.
+func (cli *Instance) checkPGPKeyNotAdded(
+	vlt *vault.Vault, gpgKeyID, fingerprint string,
+) error {
+	exists, err := cli.pgpUnlockerExists(vlt, fingerprint)
+	if err != nil {
+		return fmt.Errorf(
+			"could not check whether GPG key %s is already an unlocker: %w",
+			gpgKeyID, err,
+		)
+	}
+
+	if exists {
+		return fmt.Errorf("GPG key %s %w", gpgKeyID, errGPGKeyAlreadyUnlocker)
+	}
+
+	return nil
+}
+
 // pgpUnlockerExists reports whether the vault already has a PGP unlocker
 // for the GPG key with the given fingerprint. It returns an error, and no
 // answer, when unlockers.d or an unlocker's metadata file cannot be read;

internal/cli/unreadable_dir_test.go

@@ -9,11 +9,12 @@
 // "nothing there" and go ahead.
 //
 // The tests make the look fail with a wrapper around the in-memory
-// filesystem, which the state directory lock refuses. So they call the
-// function each command runs once it holds the lock, such as addPGPUnlocker
-// for UnlockersAdd, or, for a removal, the function that makes its checks,
-// such as findVaultToRemove for RemoveVault, which runs again under the
-// lock before anything is removed, with --force or without.
+// filesystem, which the state directory lock refuses. So they call a
+// function that makes the look before any lock is taken: addPGPUnlocker for
+// UnlockersAdd, which looks again once it holds the lock, or, for a removal,
+// the function that makes its checks, such as findVaultToRemove for
+// RemoveVault, which runs again under the lock before anything is removed,
+// with --force or without.
 
 //nolint:testpackage // white-box test of unexported internals
 package cli

internal/cli/vault.go

@@ -222,18 +222,14 @@ func (cli *Instance) resolvePassphrase() (*memguard.LockedBuffer, func(), error)
 	return passphraseBuffer, passphraseBuffer.Destroy, nil
 }
 
-// CreateVault creates a new vault
+// CreateVault creates a new vault. It asks for the mnemonic and the
+// passphrase before it takes the state directory lock, so that no other
+// command waits while the user answers.
 func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
 	secret.Debug("Creating new vault", "name", name, "state_dir", cli.stateDir)
 
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Refuse an invalid name or an existing vault before asking for anything
-	err = vault.CheckNewVaultName(cli.fs, cli.stateDir, name)
+	err := vault.CheckNewVaultName(cli.fs, cli.stateDir, name)
 	if err != nil {
 		return err
 	}
@@ -265,7 +261,14 @@ func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
 	}
 	defer cleanupPassphrase()
 
-	// Create the vault with its passphrase unlocker
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	// Create the vault with its passphrase unlocker. Another command may
+	// have created it since the check above: vault.CreateVault checks again.
 	vlt, err := vault.CreateVault(cli.fs, cli.stateDir, name,
 		mnemonic, passphraseBuffer)
 	if err != nil {

internal/secret/pgpunlock_test.go

@@ -375,8 +375,7 @@ func testCreatePGPUnlocker(
 	}
 
 	// Now create a PGP unlock key (this will use our custom GPGEncryptFunc)
-	pgpUnlocker, err := secret.CreatePGPUnlocker(
-		fs, stateDir, keyID, fingerprint, mnemonic, nil)
+	pgpUnlocker, err := secret.CreatePGPUnlocker(vlt, keyID, fingerprint)
 	if err != nil {
 		t.Fatalf("Failed to create PGP unlock key: %v", err)
 	}

internal/secret/pgpunlocker.go

@@ -192,58 +192,45 @@ func generatePGPUnlockerName() (string, error) {
 	return fmt.Sprintf("%s-pgp-%s", hostname, enrollmentTime), nil
 }
 
-// pgpUnlockerDir returns the current vault and the directory in it for a
-// new PGP unlocker, named after the host and the time.
-//
-//nolint:ireturn // the vault is only available behind VaultInterface
-func pgpUnlockerDir(
-	fs afero.Fs, stateDir string,
-) (VaultInterface, string, error) {
-	// Get current vault
-	vault, err := GetCurrentVault(fs, stateDir)
-	if err != nil {
-		return nil, "", fmt.Errorf("failed to get current vault: %w", err)
-	}
-
+// pgpUnlockerDir returns the directory in vault for a new PGP unlocker,
+// named after the host and the time.
+func pgpUnlockerDir(vault VaultInterface) (string, error) {
 	// Generate the unlocker name based on hostname and time
 	unlockerName, err := generatePGPUnlockerName()
 	if err != nil {
-		return nil, "", fmt.Errorf("failed to generate unlocker name: %w", err)
+		return "", fmt.Errorf("failed to generate unlocker name: %w", err)
 	}
 
 	vaultDir, err := vault.GetDirectory()
 	if err != nil {
-		return nil, "", fmt.Errorf("failed to get vault directory: %w", err)
+		return "", fmt.Errorf("failed to get vault directory: %w", err)
 	}
 
-	return vault, filepath.Join(vaultDir, "unlockers.d", unlockerName), nil
+	return filepath.Join(vaultDir, "unlockers.d", unlockerName), nil
 }
 
-// CreatePGPUnlocker creates a new PGP unlocker and stores it in the vault.
+// CreatePGPUnlocker creates a new PGP unlocker and stores it in vault.
 // It encrypts to the GPG key gpgKeyID and records fingerprint, that key's
 // fingerprint as ResolveGPGKeyFingerprint returns it, in the metadata.
 // Everything that can fail short of writing a file is done before anything
 // is written, and the files are written through WriteDir, so a failure
-// leaves no partial unlocker. The long-term key comes from mnemonic when it
-// is not nil, else from the current unlocker, which is given passphrase when
-// it is a passphrase unlocker.
+// leaves no partial unlocker. The long-term key comes from vault's
+// GetOrDeriveLongTermKey: the key it holds when it is unlocked, else the
+// key from its mnemonic or its current unlocker.
 func CreatePGPUnlocker(
-	fs afero.Fs, stateDir, gpgKeyID, fingerprint string,
-	mnemonic, passphrase *memguard.LockedBuffer,
+	vault VaultInterface, gpgKeyID, fingerprint string,
 ) (*PGPUnlocker, error) {
 	err := checkGPGAvailable()
 	if err != nil {
 		return nil, err
 	}
 
-	vault, unlockerDir, err := pgpUnlockerDir(fs, stateDir)
+	unlockerDir, err := pgpUnlockerDir(vault)
 	if err != nil {
 		return nil, err
 	}
 
-	// The vault's GetOrDeriveLongTermKey, in step 2, uses both
-	vault.SetMnemonic(mnemonic)
-	vault.SetUnlockPassphrase(passphrase)
+	fs := vault.GetFilesystem()
 
 	// Step 1: Generate a new age keypair for the PGP unlocker
 	ageIdentity, err := age.GenerateX25519Identity()
@@ -295,7 +282,7 @@ func CreatePGPUnlocker(
 func encryptPGPUnlockerKeys(
 	vault VaultInterface, ageIdentity *age.X25519Identity, gpgKeyID string,
 ) ([]byte, []byte, error) {
-	// From the mnemonic or the current unlocker, as for a passphrase unlocker
+	// The key vault holds, or else from its mnemonic or current unlocker
 	ltIdentity, err := vault.GetOrDeriveLongTermKey()
 	if err != nil {
 		return nil, nil, fmt.Errorf("failed to get long-term key: %w", err)

internal/secret/pgpunlocker_test.go

@@ -58,7 +58,8 @@ func TestCreatePGPUnlockerFailureWritesNothing(t *testing.T) {
 	}}
 
 	_, err = secret.CreatePGPUnlocker(
-		fs, testVaultStateDir, testGPGKeyID, testGPGFingerprint, nil, nil)
+		vault.NewVault(fs, testVaultStateDir, testVaultName),
+		testGPGKeyID, testGPGFingerprint)
 	require.Error(t, err)
 
 	vaultDir, err := vlt.GetDirectory()
@@ -87,17 +88,15 @@ func TestPGPUnlockerAddedTwiceKeepsFirst(t *testing.T) {
 
 	fs := afero.NewMemMapFs()
 	mnemonic := testMnemonicBuffer(t)
-	_, err := vault.CreateVault(fs, testVaultStateDir, testVaultName, mnemonic, nil)
+	vlt, err := vault.CreateVault(fs, testVaultStateDir, testVaultName, mnemonic, nil)
 	require.NoError(t, err)
 
-	first, err := secret.CreatePGPUnlocker(
-		fs, testVaultStateDir, testGPGKeyID, testGPGFingerprint, mnemonic, nil)
+	first, err := secret.CreatePGPUnlocker(vlt, testGPGKeyID, testGPGFingerprint)
 	require.NoError(t, err)
 
 	firstFiles := dirFiles(t, fs, first.GetDirectory())
 
-	second, err := secret.CreatePGPUnlocker(
-		fs, testVaultStateDir, testGPGKeyID, testGPGFingerprint, mnemonic, nil)
+	second, err := secret.CreatePGPUnlocker(vlt, testGPGKeyID, testGPGFingerprint)
 	require.NoError(t, err)
 	assert.NotEqual(t, first.GetDirectory(), second.GetDirectory())
 	assert.Equal(t, firstFiles, dirFiles(t, fs, first.GetDirectory()))

internal/secret/secret.go

@@ -28,11 +28,6 @@ type VaultInterface interface {
 	GetFilesystem() afero.Fs
 	GetCurrentUnlocker() (Unlocker, error)
 	GetOrDeriveLongTermKey() (*age.X25519Identity, error)
-	// SetMnemonic and SetUnlockPassphrase give GetOrDeriveLongTermKey the
-	// mnemonic to derive the long-term key from, and the passphrase for a
-	// current passphrase unlocker; nil for none.
-	SetMnemonic(mnemonic *memguard.LockedBuffer)
-	SetUnlockPassphrase(passphrase *memguard.LockedBuffer)
 	CreatePassphraseUnlocker(
 		passphrase *memguard.LockedBuffer) (*PassphraseUnlocker, error)
 }

internal/secret/secret_test.go

@@ -118,12 +118,6 @@ func (m *MockVault) GetOrDeriveLongTermKey() (*age.X25519Identity, error) {
 	return nil, errNotImplementedInMock
 }
 
-func (m *MockVault) SetMnemonic(mnemonic *memguard.LockedBuffer) {
-	m.mnemonic = mnemonic
-}
-
-func (m *MockVault) SetUnlockPassphrase(_ *memguard.LockedBuffer) {}
-
 func (m *MockVault) CreatePassphraseUnlocker(
 	_ *memguard.LockedBuffer,
 ) (*PassphraseUnlocker, error) {

internal/secret/version_test.go

@@ -90,10 +90,6 @@ func (m *MockVersionVault) GetOrDeriveLongTermKey() (*age.X25519Identity, error)
 	return nil, errNotImplementedInMock
 }
 
-func (m *MockVersionVault) SetMnemonic(_ *memguard.LockedBuffer) {}
-
-func (m *MockVersionVault) SetUnlockPassphrase(_ *memguard.LockedBuffer) {}
-
 func (m *MockVersionVault) CreatePassphraseUnlocker(
 	_ *memguard.LockedBuffer,
 ) (*secret.PassphraseUnlocker, error) {

internal/vault/errors_test.go

@@ -132,7 +132,7 @@ func TestGetSecretWithWrongMnemonic(t *testing.T) {
 	t.Cleanup(mnemonic.Destroy)
 
 	vlt := vault.NewVault(created.GetFilesystem(), testStateDir, testVaultName)
-	vlt.SetMnemonic(mnemonic)
+	vlt.Mnemonic = mnemonic
 
 	_, err := vlt.GetSecret(testSecretName)
 	require.ErrorIs(t, err, vault.ErrMnemonicMismatch)

internal/vault/management.go

@@ -235,9 +235,9 @@ func processMnemonicForVault(
 // vault: one wrapping ErrInvalidVaultName when it is not a valid vault name,
 // and one wrapping ErrVaultExists when that vault already exists. Creating an
 // existing vault again would replace its keys, and its secrets could no
-// longer be decrypted. CreateVault checks this before writing anything; the
-// commands that create a vault also check it before they ask for the
-// mnemonic or passphrase, holding the state directory lock.
+// longer be decrypted. The commands that create a vault check it before they
+// ask for the mnemonic or passphrase, and CreateVault checks it again, under
+// the state directory lock, before writing anything.
 func CheckNewVaultName(fs afero.Fs, stateDir string, name string) error {
 	err := ValidateVaultName(name)
 	if err != nil {

internal/vault/secrets_version_test.go

@@ -336,7 +336,7 @@ func TestVaultVersionTimestampsAfterUnlock(t *testing.T) {
 
 	// A new instance is locked; AddSecret unlocks it with the passphrase
 	vault := NewVault(fs, testStateDir, "test")
-	vault.SetUnlockPassphrase(passphrase)
+	vault.UnlockPassphrase = passphrase
 
 	addTestSecretToVault(t, vault, testSecretPath, []byte("version-2"), true)
 	require.False(t, fs.readAt.IsZero(), "AddSecret did not unlock the vault")

internal/vault/vault.go

@@ -64,18 +64,6 @@ func (v *Vault) ClearLongTermKey() {
 	v.longTermKey = nil
 }
 
-// SetMnemonic sets v.Mnemonic, for code that has v only as a
-// secret.VaultInterface.
-func (v *Vault) SetMnemonic(mnemonic *memguard.LockedBuffer) {
-	v.Mnemonic = mnemonic
-}
-
-// SetUnlockPassphrase sets v.UnlockPassphrase, for code that has v only as
-// a secret.VaultInterface.
-func (v *Vault) SetUnlockPassphrase(passphrase *memguard.LockedBuffer) {
-	v.UnlockPassphrase = passphrase
-}
-
 // GetOrDeriveLongTermKey gets the long-term key from memory or derives it
 // from available sources
 func (v *Vault) GetOrDeriveLongTermKey() (*age.X25519Identity, error) {

internal/vault/vault_error_test.go

@@ -128,7 +128,7 @@ func TestAddSecretForceWithWrongPassphraseChangesNothing(t *testing.T) {
 	t.Cleanup(wrong.Destroy)
 
 	vlt := vault.NewVault(fs, testStateDir, testVaultName)
-	vlt.SetUnlockPassphrase(wrong)
+	vlt.UnlockPassphrase = wrong
 
 	secretDir := filepath.Join(testStateDir, "vaults.d", testVaultName,
 		"secrets.d", testSecretName)