Ask for the mnemonic and passphrase before taking the state directory lock (closes #152)

init, vault create, unlocker add, and add, import and generate secret
with --force asked for the mnemonic or passphrase, or unlocked the
vault, while holding the state directory lock, so every other command
that changes anything waited at the prompt. They now ask and unlock
first and lock after. What they checked before asking is checked again
under the lock: unlocker add fails if the vault was removed meanwhile
and refuses a GPG key added meanwhile. A forced add of a secret that
another command created meanwhile lets go of the lock, gets the key,
and takes the lock again. secret.CreatePGPUnlocker now takes the vault,
already unlocked; SetMnemonic and SetUnlockPassphrase, used only by it,
are gone.

Not done: secret move between vaults still unlocks both under the lock;
the issue does not name it.

Model: opus-5-5

README.md

@@ -443,9 +443,7 @@ A command that changes anything locks the state directory, through its file
 `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 left open in one
-terminal holds up no command in another. A forced add of a secret that does not
+terminal holds up no command in another.
-exist yet fails, and changes nothing, if another command creates the secret
-while it waits for the lock; run it again to replace the secret.
 
 ## Security Features
 

internal/cli/lock_test.go

@@ -674,22 +674,6 @@ func TestCommandsCheckAgainOnceLocked(t *testing.T) {
 			},
 			errGPGKeyAlreadyUnlocker,
 		},
-		{
-			// Without the mnemonic or a passphrase, replacing the secret
-			// would ask for the passphrase; it does not exist yet, so the
-			// add asks for nothing
-			"add --force, secret added",
-			func(cli *Instance) error {
-				return cli.AddSecret("added", true)
-			},
-			func(fs afero.Fs) error {
-				value := memguard.NewBufferFromBytes([]byte("other"))
-				defer value.Destroy()
-
-				return work(fs).AddSecret("added", value, false)
-			},
-			errAddedMeanwhile,
-		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			requireChecksAgainOnceLocked(t, tc.run, tc.change, tc.want)
@@ -735,6 +719,49 @@ func requireChecksAgainOnceLocked(
 	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

@@ -47,10 +47,6 @@ var (
 	errCrossVaultSourceUnqualified = errors.New(
 		"source must specify vault (e.g., vault:secret) for cross-vault move")
 	errMoveOntoItself = errors.New("cannot be moved onto itself")
-	// errAddedMeanwhile carries only the message tail; storeSecret composes
-	// "secret <name> was added by another command ...".
-	errAddedMeanwhile = errors.New("was added by another command in the " +
-		"meantime, so nothing was changed; run the command again to replace it")
 )
 
 // ParseVaultSecretRef parses a "vault:secret" or just "secret" reference
@@ -434,9 +430,9 @@ func (cli *Instance) AddSecret(secretName string, force bool) error {
 // 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, once the lock is held, the add replaces a version after
+// user answers. If another command adds the secret while this one waits for
-// all, because another command added the secret in the meantime, it fails
+// the lock, storeSecret lets go of the lock, gets the key, and takes the
-// rather than ask with the lock held.
+// lock again.
 func (cli *Instance) storeSecret(
 	vlt *vault.Vault, name string, value *memguard.LockedBuffer, force bool,
 ) error {
@@ -457,24 +453,46 @@ func (cli *Instance) storeSecret(
 		}
 	}
 
+	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 err
+		return false, err
 	}
 	defer release()
 
-	if !replacing {
+	replacing, err := replacesVersion(vlt, name, force)
-		replacing, err = replacesVersion(vlt, name, force)
+	if err != nil {
-		if err != nil {
+		return false, err
-			return err
-		}
-
-		if replacing {
-			return fmt.Errorf("secret %s %w", name, errAddedMeanwhile)
-		}
 	}
 
-	return vlt.AddSecret(name, value, force)
+	if replacing && vlt.Locked() {
+		return true, nil
+	}
+
+	return false, vlt.AddSecret(name, value, force)
 }
 
 // replacesVersion reports whether adding the secret name to vlt replaces

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/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)