Set the time zone in main and run Entry in its own test process (closes #192)

cli.Entry() set time.Local to UTC and ended with memguard.Purge(), and
TestEntryWipesBuffersAndReturnsExitCode ran it in the same process as
the other tests. A timer left by an earlier test could read time.Local
as it changed, which the race detector reported on some runs, and the
purge could destroy a buffer still in use, the likeliest cause of a
crash seen on another run.

main() in cmd/secret now sets time.Local before it calls cli.Entry();
the test that runs the built binary with TZ=America/New_York still
covers it. The Entry test runs cli.Entry() in a copy of the test binary
started with SECRET_TEST_RUN_ENTRY set, which init picks up so that
TestMain does not run there.

Model: opus-5-5

README.md

@@ -438,6 +438,14 @@ 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`, `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
 
 ### Encryption

TODO.md

@@ -8,8 +8,7 @@
 
 # Status
 
-pre-1.0. No git tags. Open work is tracked on the issue tracker, which is
-authoritative.
+pre-1.0. Open work is tracked on the issue tracker, which is authoritative.
 
 # Next Step
 
@@ -18,6 +17,183 @@ https://git.eeqj.de/sneak/secret/milestone/12
 
 # Completed Steps
 
+- 2026-10-15: `make test` no longer fails on some runs in `internal/cli`
+  (https://git.eeqj.de/sneak/secret/issues/192). `cli.Entry()` set `time.Local`
+  to UTC and ended by wiping every memguard buffer, and a test ran it in the
+  same process as the other tests. A timer left by an earlier test could read
+  `time.Local` as it changed, which the race detector reported, and the wipe
+  could destroy a buffer still in use, the likeliest cause of a crash of the
+  test process on another run. `main()` in `cmd/secret` now sets `time.Local`
+  before it calls `cli.Entry()`, and that test runs `cli.Entry()` in a process
+  of its own.
+- 2026-10-15: `secret mv` from one vault to another unlocks both vaults before
+  it takes the state directory lock
+  (https://git.eeqj.de/sneak/secret/issues/189). It unlocked them while holding
+  the lock, and unlocking can ask for a passphrase, so every other command that
+  changes anything 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, which
+  `secret unlocker add` shares, now ends "nothing was changed".
+- 2026-10-15: `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, or unlock the vault, before they take the
+  state directory lock (https://git.eeqj.de/sneak/secret/issues/152). They did
+  so while holding the lock, so every other command that changes anything waited
+  at the prompt. Under the lock, `secret unlocker add` fails if the vault was
+  removed, or created again with another key, and refuses a GPG key added
+  meanwhile; a forced add of a secret created meanwhile lets go of the lock,
+  gets the key and takes the lock again. `secret.CreatePGPUnlocker` takes the
+  unlocked vault; `SetMnemonic` and `SetUnlockPassphrase` are gone.
+- 2026-10-15: A `secret mv` to another vault that copies the secret but cannot
+  remove the source fails (https://git.eeqj.de/sneak/secret/issues/159). It
+  printed a warning and the usual "Moved" line and exited 0. If the source
+  secret's directory is still in place, the error says the copy holds all
+  versions and the source remains. If the source left its vault and only the
+  temporary directory it was moved into could not be deleted, the error says the
+  secret is moved and names that directory, which the error of `RemoveDirAtomic`
+  now does.
+- 2026-10-15: `secret encrypt` and `secret decrypt` with `--output` hold the
+  result in memory and open the output file only once encryption or decryption
+  has succeeded (https://git.eeqj.de/sneak/secret/issues/143). They emptied that
+  file before reading any input, so a failed command left it empty, and
+  encrypting a file onto itself gave the encryption of nothing. A failure to
+  encrypt or decrypt now leaves `--output` untouched, and `--input` can name the
+  same file. A failure while writing `--output`, such as a full disk, can still
+  leave it empty or partly written; an error from closing it is reported.
+  Without `--output` the result still goes to stdout as it is produced.
+- 2026-10-15: A command that uses the current vault fails when `currentvault` is
+  empty or names no vault (https://git.eeqj.de/sneak/secret/issues/156).
+  `secret list` reported an empty vault with exit status 0, and `secret add`
+  failed on a missing directory. `ResolveVaultSymlink` now refuses an empty or
+  invalid name, or one with no directory under `vaults.d`, with an error that
+  wraps `ErrVaultNotFound`, names the file and says to run
+  `secret vault select <name>`. `secret vault list` and `secret info` still run.
+- 2026-10-15: `secret` prints every time in UTC, whatever the machine's time
+  zone (https://git.eeqj.de/sneak/secret/issues/169). `secret list`,
+  `secret version list`, `secret unlocker list` and `secret info` printed times
+  in the local time zone without naming it, so one vault showed different times
+  on two machines. `Entry()` now sets `time.Local` to UTC before anything else,
+  as policy 6 in `AGENTS.md` asks, so every time the program prints or writes is
+  in UTC. `--json` times end in `Z`, and a new version's name takes the UTC
+  date. The README says so.
+- 2026-10-15: `secret vault`, `secret unlocker`, `secret version` and
+  `secret generate` fail when given a subcommand they do not have
+  (https://git.eeqj.de/sneak/secret/issues/183). They printed their help and
+  exited 0, so a mistyped command looked like one that worked. Each now exits 1
+  with `unknown command "<name>" for "secret <parent>"`, and with no arguments
+  still prints its help. Cobra checks a command's `Args` only when it has a
+  `RunE`, so each gets `Args: cobra.NoArgs` and a `RunE` that prints its help;
+  their help now shows a `secret <parent> [flags]` usage line.
+  `secret vault help` fails as an unknown subcommand, while `secret help vault`
+  and `secret vault --help` still work.
+- 2026-10-15: With `SB_UNLOCK_PASSPHRASE` set, `secret unlocker add passphrase`
+  asks on the terminal for the new unlocker's passphrase and uses the variable
+  to unlock the vault (https://git.eeqj.de/sneak/secret/issues/154). It gave the
+  new unlocker the passphrase in the variable without asking. With no
+  controlling terminal, the new unlocker takes the variable's passphrase and the
+  output says so. `secret.ErrNoTerminal` is exported so that the command can
+  tell a missing terminal from other failures to read, such as nothing typed or
+  a confirmation that does not match, which still fail the command.
+- 2026-10-15: `secret init` and `secret vault create` stop with an error naming
+  the file when a vault's `vault-metadata.json` cannot be read or parsed, and
+  leave the state directory as it was
+  (https://git.eeqj.de/sneak/secret/issues/140). `GetNextDerivationIndex` passed
+  over such a vault, so the next vault made from the same mnemonic could get its
+  derivation index and so the same long-term key. A vault directory with no
+  metadata file is still skipped.
+- 2026-10-15: The README says that `secret add` drops one trailing newline from
+  stdin, that `secret import` stores a file's bytes unchanged, and that
+  `secret get` writes the value with no newline added
+  (https://git.eeqj.de/sneak/secret/issues/158). It said none of this, so a PEM
+  key piped into `secret add` came back one byte short with no hint why. A new
+  test stores `a\n\n` through `add` and through `import` and checks that `add`
+  kept two bytes and `import` three. No program code changes.
+- 2026-10-15: When gpg fails to decrypt, to encrypt or to look up a key's
+  fingerprint, the error ends with what gpg wrote to stderr, trimmed
+  (https://git.eeqj.de/sneak/secret/issues/146). It gave only gpg's exit status,
+  so the user could not tell why. `cmd.Output` keeps gpg's stderr in the
+  `*exec.ExitError` it returns, and the three errors now use it. When gpg wrote
+  nothing to stderr the error is as before, and when gpg wrote several lines the
+  error has them all.
+- 2026-10-15: `secret version list` no longer shows a version without
+  `NOT_AFTER` as expired (https://git.eeqj.de/sneak/secret/issues/150). STATUS
+  was `expired` for every version other than the current one, so after a promote
+  the newest version, which has no `NOT_AFTER`, read `expired`. STATUS is now
+  `current` for the current version, `expired` for another version whose
+  `NOT_AFTER` has passed, and `inactive` for any other version. The README
+  describes the three.
+- 2026-10-15: `secret unlocker list --json` for a vault with no unlockers, and
+  `secret vault list --json` when `vaults.d` holds no vaults, print the empty
+  list as `[]` instead of `null` (https://git.eeqj.de/sneak/secret/issues/148).
+  Both lists were nil slices, which `encoding/json` writes as `null`; each is
+  now made as an empty slice before it is filled. `vault.ListVaults` therefore
+  returns an empty slice instead of nil for an empty `vaults.d`.
+- 2026-10-15: `secret add`, `secret import` and `secret generate secret` fail
+  with an error, not memguard's panic, when a secret is too large for the
+  locked-memory limit (https://git.eeqj.de/sneak/secret/issues/145). The value
+  was held twice, so half the usual 8 MiB limit was enough to crash them.
+  `secret` now raises the limit to the hard limit at startup, and each of the
+  three commands refuses, before its first locked allocation, a limit that
+  leaves fewer than 8 pages for its keys and buffers. The value is read a page
+  at a time into a `memguard.Stream`, which keeps it encrypted, and copied once
+  into locked memory after its size is checked against 100 MiB and the limit. It
+  leaves the stream through `Next`, not `Read`, because `Read` re-encrypts a
+  leftover piece and memguard hangs if that runs out of locked memory.
+- 2026-10-15: Total Size in `secret info` counts every file of each version: the
+  value, the metadata and the version's two keys
+  (https://git.eeqj.de/sneak/secret/issues/147). It added up each version's
+  `data.age` and `metadata.age`, but a version keeps its value in `value.age`,
+  so no secret's value was counted. A version directory that cannot be read adds
+  nothing and gets a warning naming it.
+- 2026-10-15: `secret vault import` is gone, with its tests and its README entry
+  (https://git.eeqj.de/sneak/secret/issues/139). It took only a vault without a
+  long-term key, and `secret init` and `secret vault create` give every vault
+  one, so it always failed; a mnemonic enters through those two commands. The
+  help and the note of `secret generate mnemonic` named `secret import`, which
+  imports a file as a secret, and now name `secret init` and
+  `secret vault create`.
+- 2026-10-15: `secret version promote --help` says promote rewrites the
+  `current` file, not a "current symlink"
+  (https://git.eeqj.de/sneak/secret/issues/153). `current` is a plain file
+  holding the version name, as the README already said, and the comment in
+  `PromoteVersion` now says so too.
+- 2026-10-14: `secret init` and `secret vault create` refuse a vault that
+  already exists before they ask for the mnemonic or the unlocker passphrase
+  (https://git.eeqj.de/sneak/secret/issues/151). They asked first, and
+  `secret init` printed "Initialized secrets manager at" before refusing. The
+  check `vault.CreateVault` made before writing is now
+  `vault.CheckNewVaultName`, which both commands call before the first prompt
+  and `CreateVault` still calls. It validates the name too, so
+  `secret vault create` with an invalid name also fails before the prompts.
+  `secret init` prints the "Initialized" line only once the vault is created.
+- 2026-10-14: `secret get` fails when it cannot write the value
+  (https://git.eeqj.de/sneak/secret/issues/155). `GetSecret` and
+  `GetSecretWithVersion` discarded the result of writing it to stdout, so a full
+  disk or a failing stdout left an empty or cut-short file and exit status 0.
+  The write error is now returned, so the command prints it and exits 1. A
+  stdout the shell closed before `secret` started still exits 0, because the Go
+  runtime opens `/dev/null` in its place and the write there succeeds.
+- 2026-10-14: `secret list` and `secret list --json` give each secret's own
+  creation and last-update times (https://git.eeqj.de/sneak/secret/issues/142).
+  They gave the time of the listing for every secret, because
+  `Secret.LoadMetadata` set both to the current time. It now reads file
+  modification times, so listing still needs no passphrase. The last update is
+  when the secret's `current` file was written. The creation is when its oldest
+  version's `value.age` was written, not that version's directory, whose time
+  changes when the next add rewrites its `metadata.age`. A move to another vault
+  gives the copied files the times of the originals. `GetSecretObject` no longer
+  loads the times, so `secret get` keeps its error for a missing `current` file.
+- 2026-10-14: `secret unlocker add pgp` without `--keyid` uses the `default-key`
+  in `gpg.conf` (https://git.eeqj.de/sneak/secret/issues/141).
+  `getDefaultGPGKey` took that value from `gpgconf --list-options gpg` as
+  printed, with a leading `"` and percent-escaped, so gpg rejected the key ID.
+  The new `parseDefaultGPGKey` drops the `"` and percent-decodes the rest with
+  `url.PathUnescape`, which leaves `+` alone. A value that does not decode is an
+  error, not a fallback to the first secret key.
 - 2026-10-14: A wrong passphrase, from `SB_UNLOCK_PASSPHRASE` or typed at the
   prompt, gives the error `wrong passphrase for unlocker <id>`
   (https://git.eeqj.de/sneak/secret/issues/157). It used to be age's "no

cmd/secret/main.go

@@ -3,10 +3,16 @@ package main
 
 import (
 	"os"
+	"time"
 
 	"sneak.berlin/go/secret/internal/cli"
 )
 
 func main() {
+	// Every time secret prints or writes is in UTC, whatever TZ or the
+	// system's time zone is set to. It is set here, before cli.Entry starts
+	// any goroutine that could read time.Local while it changes.
+	time.Local = time.UTC //nolint:gosmopolitan // this makes local time UTC
+
 	os.Exit(cli.Entry())
 }

internal/cli/entry_test.go

@@ -3,6 +3,7 @@ package cli_test
 import (
 	"bufio"
 	"context"
+	"fmt"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -16,29 +17,67 @@ import (
 	"sneak.berlin/go/secret/internal/secret"
 )
 
-// Entry must return its exit code rather than exit, so that its deferred
-// memguard purge runs on the success and the error path alike.
-//
-//nolint:paralleltest // sets os.Args, and Entry wipes every buffer in the process
-func TestEntryWipesBuffersAndReturnsExitCode(t *testing.T) {
-	savedArgs := os.Args
+const (
+	// envRunEntry, when set, makes this test binary run cli.Entry with its
+	// own arguments and exit, instead of running the tests.
+	envRunEntry = "SECRET_TEST_RUN_ENTRY"
 
-	t.Cleanup(func() { os.Args = savedArgs })
+	// bufferWiped is what the test binary prints, run with envRunEntry set,
+	// when cli.Entry wiped the buffer made before it ran.
+	bufferWiped = "Entry wiped the buffer made before it ran"
+)
+
+// With envRunEntry set, the test binary makes a buffer, runs cli.Entry,
+// says whether the buffer is wiped, and exits with Entry's exit code. This
+// is done in init, not in a test, so that TestMain does not run in that
+// process: it would build the secret binary again, and remove it at the end,
+// while the other tests run it.
+//
+//nolint:gochecknoinits // must run before TestMain, as above
+func init() {
+	if os.Getenv(envRunEntry) == "" {
+		return
+	}
+
+	buf := memguard.NewBufferFromBytes([]byte("key material"))
+	code := cli.Entry()
+
+	if !buf.IsAlive() {
+		fmt.Fprintln(os.Stderr, bufferWiped)
+	}
+
+	os.Exit(code)
+}
+
+// Entry must return its exit code rather than exit, so that its deferred
+// memguard purge runs on the success and the error path alike. Entry wipes
+// every memguard buffer in the process, including those other tests are
+// using, so it runs in a process of its own: this test binary, run again
+// with envRunEntry set.
+func TestEntryWipesBuffersAndReturnsExitCode(t *testing.T) {
+	t.Parallel()
 
 	tests := []struct {
 		args     []string
 		exitCode int
 	}{
-		{args: []string{"secret", "--help"}, exitCode: 0},
-		{args: []string{"secret", "no-such-command"}, exitCode: 1},
+		{args: []string{"--help"}, exitCode: 0},
+		{args: []string{"no-such-command"}, exitCode: 1},
 	}
 
 	for _, tt := range tests {
-		buf := memguard.NewBufferFromBytes([]byte("key material"))
-		os.Args = tt.args
+		//nolint:gosec // G204: test runs its own test binary
+		cmd := exec.CommandContext(t.Context(), os.Args[0], tt.args...)
 
-		assert.Equal(t, tt.exitCode, cli.Entry(), "exit code for %v", tt.args)
-		assert.False(t, buf.IsAlive(), "Entry left a buffer unwiped for %v", tt.args)
+		cmd.Env = append(os.Environ(), envRunEntry+"=1")
+
+		output, err := cmd.CombinedOutput()
+		require.NotNil(t, cmd.ProcessState, "%v did not run: %v", tt.args, err)
+
+		assert.Equal(t, tt.exitCode, cmd.ProcessState.ExitCode(),
+			"exit code for %v: %s", tt.args, output)
+		assert.Contains(t, string(output), bufferWiped,
+			"Entry left a buffer unwiped for %v", tt.args)
 	}
 }
 

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/move_cross_vault_test.go

@@ -4,7 +4,6 @@ package cli
 import (
 	"errors"
 	"path/filepath"
-	"sync"
 	"testing"
 	"time"
 
@@ -230,68 +229,87 @@ func TestCrossVaultMoveUnlocksBeforeLocking(t *testing.T) {
 	}
 }
 
-// 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.
+// 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) {
-	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)
+	mnemonic := testMnemonicBuffer(t)
 
-			release, err := vault.LockStateDir(fs, testStateDir)
-			require.NoError(t, err)
+	otherMnemonic := memguard.NewBufferFromBytes([]byte("legal winner thank " +
+		"year wave sausage worth useful legal winner thank yellow"))
+	t.Cleanup(otherMnemonic.Destroy)
 
-			// 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()
+	move := func(cli *Instance) error {
+		cli.Mnemonic = mnemonic
 
-			instance := NewCLIInstanceWithStateDir(fs, testStateDir)
-			instance.Mnemonic = testMnemonicBuffer(t)
-
-			done := make(chan error, 1)
-
-			go func() {
-				done <- instance.MoveSecret(&cobra.Command{},
-					"default:x", "work:y", false)
-			}()
-
-			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):
-				}
-			}
-
-			require.NoError(t, fs.RemoveAll(testStateDir+tc.removed))
-			release()
-
-			select {
-			case err := <-done:
-				require.ErrorIs(t, err, tc.want)
-			case <-time.After(lockWait):
-				t.Fatal("did not finish once the lock was released")
-			}
-		})
+		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/root.go

@@ -2,7 +2,6 @@ package cli
 
 import (
 	"os"
-	"time"
 
 	"github.com/awnumar/memguard"
 	"github.com/spf13/cobra"
@@ -15,10 +14,6 @@ import (
 // every memguard buffer before it returns, so the caller must do nothing
 // but exit with the code.
 func Entry() int {
-	// Every time secret prints or writes is in UTC, whatever TZ or the
-	// system's time zone is set to.
-	time.Local = time.UTC //nolint:gosmopolitan // this makes local time UTC
-
 	raiseLockedMemoryLimit()
 
 	// On SIGINT or SIGTERM memguard runs this function, wipes every buffer

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
 	}
@@ -1025,18 +1084,24 @@ 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. Copying needs the long-term keys of both vaults, and getting
-// one can ask for a passphrase, so it gets them before it takes the state
-// directory lock, and no other command waits while the user answers.
+// 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 {
-	// Refuse a move that cannot be made before asking for anything
+	// 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)
+		srcVault, srcSecretName, destVault, destSecretName, force)
 	if err != nil {
 		return err
 	}
@@ -1045,7 +1110,7 @@ func (cli *Instance) moveSecretCrossVault(
 	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)
 	}
@@ -1062,6 +1127,11 @@ 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)
 }
@@ -1078,7 +1148,7 @@ func (cli *Instance) copySecretAndRemoveSource(
 	force bool,
 ) error {
 	srcSecretDir, err := cli.checkCrossVaultMove(
-		srcVault, srcSecretName, destVault, destSecretName)
+		srcVault, srcSecretName, destVault, destSecretName, force)
 	if err != nil {
 		return err
 	}
@@ -1109,11 +1179,13 @@ func (cli *Instance) copySecretAndRemoveSource(
 }
 
 // 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.
+// 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 {
@@ -1130,11 +1202,6 @@ 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()
@@ -1156,6 +1223,17 @@ 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,6 +15,7 @@ import (
 	"strings"
 	"time"
 
+	"filippo.io/age"
 	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
 	"sneak.berlin/go/secret/internal/secret"
@@ -39,6 +40,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 changed")
 )
 
 // UnlockerInfo represents unlocker information for display
@@ -404,12 +409,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 +422,64 @@ 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.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 {
+		return 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)
+	}
+
+	return 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 +499,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 +518,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 +562,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 +576,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 +770,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)