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, and a forced add fails if another
command created the secret meanwhile, rather than ask under the lock.
secret.CreatePGPUnlocker now takes the vault, already unlocked.

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

Model: opus-5-5

README.md

@@ -118,11 +118,7 @@ are allowed, and a name must not be empty, `.` or `..`.
 
 #### `secret vault select <name>`
 
-Switches to the specified vault for subsequent operations. When `currentvault`
-is empty or names a vault that does not exist, every command that uses the
-current vault fails with an error that names the file and says to run
-`secret vault select <name>`. `secret vault list` still lists the vaults, and
-`secret info` still runs; both show no vault as current.
+Switches to the specified vault for subsequent operations.
 
 #### `secret vault remove <name> [--force]` / `secret vault rm` ⚠️ 🛑
 
@@ -322,12 +318,6 @@ name. The file's bytes are stored unchanged, a final newline included.
 
 ### Encryption Operations
 
-With `--output`, `secret encrypt` and `secret decrypt` hold the result in memory
-and write it to `--output` only once encryption or decryption has succeeded, so
-a failure to encrypt or decrypt leaves `--output` untouched. A failure while
-writing `--output`, such as a full disk, can leave it empty or partly written.
-The command can need up to about four times as much memory as the result.
-
 #### `secret encrypt <secret-name> [--input=file] [--output=file]`
 
 Encrypts data using an Age key stored as a secret. If the secret doesn't exist,
@@ -440,11 +430,12 @@ in a CI job, the command fails and names the variable to set.
 
 A command that changes anything locks the state directory, through its file
 `lock`, and another such command waits until the lock is released.
-`secret init`, `secret vault create`, `secret unlocker add`, `secret mv` from
-one vault to another, and `secret add`, `secret import` and
-`secret generate secret` with `--force`, ask for the mnemonic or passphrase
-before they take the lock, so a prompt of theirs left open in one terminal holds
-up no command in another.
+`secret init`, `secret vault create`, `secret unlocker add`, and `secret add`,
+`secret import` and `secret generate secret` with `--force`, ask for the
+mnemonic or passphrase before they take the lock, so a prompt left open in one
+terminal holds up no command in another. A forced add of a secret that does not
+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
 

TODO.md

@@ -8,7 +8,8 @@
 
 # Status
 
-pre-1.0. Open work is tracked on the issue tracker, which is authoritative.
+pre-1.0. No git tags. Open work is tracked on the issue tracker, which is
+authoritative.
 
 # Next Step
 
@@ -17,183 +18,6 @@ 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,16 +3,10 @@ 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/crypto.go

@@ -1,7 +1,6 @@
 package cli
 
 import (
-	"bytes"
 	"errors"
 	"fmt"
 	"io"
@@ -108,8 +107,8 @@ func (cli *Instance) storeNewEncryptionKey(
 // does not exist. The caller must destroy the returned buffer. Only storing
 // a new key takes the state directory lock, so that reading an existing key
 // works on a read-only state directory and keeps no other command waiting
-// at the passphrase prompt, and Encrypt reads its input and writes its
-// output without the lock.
+// at the passphrase prompt, and Encrypt streams its input and output
+// unlocked.
 func (cli *Instance) resolveEncryptionKey(
 	vlt *vault.Vault, secretName string,
 ) (*memguard.LockedBuffer, error) {
@@ -189,17 +188,17 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
 		input = file
 	}
 
-	// Set up output writer. With --output the result is held in memory
-	// and written only once encryption has succeeded, so a failure to
-	// encrypt leaves --output untouched and --input can be the same file.
-	// A failure while writing --output, such as a full disk, can leave it
-	// empty or partly written.
+	// Set up output writer
 	output := cli.cmd.OutOrStdout()
 
-	var result bytes.Buffer
-
 	if outputFile != "" {
-		output = &result
+		file, err := cli.fs.Create(outputFile)
+		if err != nil {
+			return fmt.Errorf("failed to create output file: %w", err)
+		}
+		defer func() { _ = file.Close() }()
+
+		output = file
 	}
 
 	// Encrypt the data
@@ -218,11 +217,7 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
 		return fmt.Errorf("failed to finalize encryption: %w", err)
 	}
 
-	if outputFile == "" {
-		return nil
-	}
-
-	return cli.writeOutputFile(outputFile, result.Bytes())
+	return nil
 }
 
 // Decrypt decrypts data using an age secret key stored in a secret
@@ -283,17 +278,17 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
 		input = file
 	}
 
-	// Set up output writer. With --output the result is held in memory
-	// and written only once decryption has succeeded, so a failure to
-	// decrypt leaves --output untouched and --input can be the same file.
-	// A failure while writing --output, such as a full disk, can leave it
-	// empty or partly written.
+	// Set up output writer
 	output := cli.cmd.OutOrStdout()
 
-	var result bytes.Buffer
-
 	if outputFile != "" {
-		output = &result
+		file, err := cli.fs.Create(outputFile)
+		if err != nil {
+			return fmt.Errorf("failed to create output file: %w", err)
+		}
+		defer func() { _ = file.Close() }()
+
+		output = file
 	}
 
 	// Decrypt the data
@@ -307,32 +302,6 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
 		return fmt.Errorf("failed to decrypt data: %w", err)
 	}
 
-	if outputFile == "" {
-		return nil
-	}
-
-	return cli.writeOutputFile(outputFile, result.Bytes())
-}
-
-// writeOutputFile creates outputFile, or empties it if it exists, and
-// writes result into it.
-func (cli *Instance) writeOutputFile(outputFile string, result []byte) error {
-	file, err := cli.fs.Create(outputFile)
-	if err != nil {
-		return fmt.Errorf("failed to create output file: %w", err)
-	}
-
-	_, err = file.Write(result)
-
-	closeErr := file.Close()
-	if err == nil {
-		err = closeErr
-	}
-
-	if err != nil {
-		return fmt.Errorf("failed to write output file: %w", err)
-	}
-
 	return nil
 }
 

internal/cli/crypto_test.go

@@ -1,185 +0,0 @@
-//nolint:testpackage // sets the unexported fields of Instance
-package cli
-
-import (
-	"errors"
-	"fmt"
-	"io"
-	"os"
-	"path/filepath"
-	"testing"
-
-	"github.com/spf13/afero"
-	"github.com/spf13/cobra"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/secret/internal/vault"
-)
-
-// plainText is the content of plain.txt in the directory newCryptoTest makes.
-const plainText = "document"
-
-// newCryptoTest returns an Instance whose current vault, in a new state
-// directory on the real filesystem, holds the age key secret "key", and a
-// new directory holding only plain.txt and plain.txt.age, its encryption
-// with that key.
-func newCryptoTest(t *testing.T) (*Instance, string) {
-	t.Helper()
-
-	fs := afero.NewOsFs()
-	stateDir := t.TempDir()
-
-	_, err := vault.CreateVault(fs, stateDir, "default", testMnemonicBuffer(t), nil)
-	require.NoError(t, err)
-
-	cli := NewCLIInstanceWithStateDir(fs, stateDir)
-	cli.Mnemonic = testMnemonicBuffer(t)
-	cli.cmd = &cobra.Command{}
-
-	dir := t.TempDir()
-	plain := filepath.Join(dir, "plain.txt")
-	require.NoError(t, afero.WriteFile(fs, plain, []byte(plainText), 0o600))
-	require.NoError(t, cli.Encrypt("key", plain, plain+".age"))
-
-	return cli, dir
-}
-
-// TestFailedCryptoLeavesOutputUnchanged is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/143, where secret encrypt and
-// secret decrypt emptied the file named by --output before they failed.
-// A failure to encrypt or decrypt must leave an existing file as it was and
-// create none, and give the same error as without --output.
-func TestFailedCryptoLeavesOutputUnchanged(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		name string
-		run  func(cli *Instance, dir, output string) error
-	}{
-		// Reading a directory fails once encryption has begun
-		{"encrypt", func(cli *Instance, dir, output string) error {
-			return cli.Encrypt("key", dir, output)
-		}},
-		// plain.txt is not an age file
-		{"decrypt", func(cli *Instance, dir, output string) error {
-			return cli.Decrypt("key", filepath.Join(dir, "plain.txt"), output)
-		}},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-
-			cli, dir := newCryptoTest(t)
-			existing := filepath.Join(dir, "existing.txt")
-			require.NoError(t, afero.WriteFile(cli.fs, existing, []byte("precious"), 0o600))
-
-			cli.cmd.SetOut(io.Discard)
-			toStdout := tt.run(cli, dir, "")
-			require.Error(t, toStdout)
-
-			require.EqualError(t, tt.run(cli, dir, existing), toStdout.Error())
-			require.EqualError(t, tt.run(cli, dir, filepath.Join(dir, "new.txt")),
-				toStdout.Error())
-
-			content, err := afero.ReadFile(cli.fs, existing)
-			require.NoError(t, err)
-			require.Equal(t, "precious", string(content))
-
-			exists, err := afero.Exists(cli.fs, filepath.Join(dir, "new.txt"))
-			require.NoError(t, err)
-			require.False(t, exists, "a failed %s created --output", tt.name)
-		})
-	}
-}
-
-// TestCryptoInPlace checks that secret encrypt and secret decrypt given
-// one file as both --input and --output replace it with the right result.
-// Before https://git.eeqj.de/sneak/secret/issues/143 they emptied the file
-// before reading it.
-func TestCryptoInPlace(t *testing.T) {
-	t.Parallel()
-
-	cli, dir := newCryptoTest(t)
-	file := filepath.Join(dir, "plain.txt")
-
-	require.NoError(t, cli.Encrypt("key", file, file))
-	require.NoError(t, cli.Decrypt("key", file, file))
-
-	content, err := afero.ReadFile(cli.fs, file)
-	require.NoError(t, err)
-	require.Equal(t, plainText, string(content))
-}
-
-// errCloseFailed is returned by the files closeFailFs creates in place of
-// a successful Close, as NFS reports a full disk only when the file is
-// closed.
-var errCloseFailed = errors.New("no space left on device")
-
-// closeFailFs creates files whose Close fails with errCloseFailed.
-type closeFailFs struct {
-	afero.Fs
-}
-
-// closeFailFile is a file created through closeFailFs.
-type closeFailFile struct {
-	afero.File
-}
-
-func (f closeFailFile) Close() error {
-	_ = f.File.Close()
-
-	return errCloseFailed
-}
-
-//nolint:ireturn // afero.File is the interface required by afero.Fs
-func (f closeFailFs) Create(name string) (afero.File, error) {
-	file, err := f.Fs.Create(name)
-	if err != nil {
-		return nil, err
-	}
-
-	return closeFailFile{File: file}, nil
-}
-
-// TestDecryptFailsWhenClosingOutputFails checks that secret decrypt fails
-// when closing --output fails, since some filesystems report a failed
-// write only then.
-func TestDecryptFailsWhenClosingOutputFails(t *testing.T) {
-	t.Parallel()
-
-	cli, dir := newCryptoTest(t)
-	cli.fs = closeFailFs{Fs: cli.fs}
-
-	err := cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"),
-		filepath.Join(dir, "out.txt"))
-	require.ErrorIs(t, err, errCloseFailed)
-}
-
-// TestDecryptToDescriptorOfDeletedFile checks that secret decrypt given
-// /dev/fd/N as --output writes through that descriptor. A program may give
-// the command a temporary file it has already deleted as stdout, pass
-// /dev/stdout as --output and read the result back from that file.
-func TestDecryptToDescriptorOfDeletedFile(t *testing.T) {
-	t.Parallel()
-
-	cli, dir := newCryptoTest(t)
-
-	file, err := os.CreateTemp(t.TempDir(), "out")
-	require.NoError(t, err)
-
-	defer func() { _ = file.Close() }()
-
-	require.NoError(t, os.Remove(file.Name()))
-
-	output := fmt.Sprintf("/dev/fd/%d", file.Fd())
-	require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), output))
-
-	// On macOS the command's open of /dev/fd/N shares this descriptor's
-	// position, which its write leaves at the end.
-	_, err = file.Seek(0, io.SeekStart)
-	require.NoError(t, err)
-
-	content, err := io.ReadAll(file)
-	require.NoError(t, err)
-	require.Equal(t, plainText, string(content))
-}

internal/cli/current_vault_test.go

@@ -1,60 +0,0 @@
-package cli_test
-
-import (
-	"bytes"
-	"path/filepath"
-	"strconv"
-	"testing"
-
-	"github.com/spf13/afero"
-	"github.com/spf13/cobra"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/secret/internal/cli"
-	"sneak.berlin/go/secret/internal/secret"
-	"sneak.berlin/go/secret/internal/vault"
-)
-
-// TestListFailsWhenCurrentVaultDoesNotExist is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/156, where a currentvault file
-// that was empty, held an invalid vault name, or named a vault that does
-// not exist, made `secret list` report an empty vault. Listing must fail
-// with an error that names the file and says to run `secret vault select`,
-// `secret vault list` must still list the vaults, and selecting a vault
-// must make listing work again. The name "." would make vaults.d itself the
-// vault, and "../vaults.d/work" would reach the vault work by a path.
-func TestListFailsWhenCurrentVaultDoesNotExist(t *testing.T) {
-	t.Parallel()
-
-	for _, name := range []string{"", ".", "../vaults.d/work", "gone"} {
-		t.Run(strconv.Quote(name), func(t *testing.T) {
-			t.Parallel()
-
-			fs := newTwoVaultFs(t)
-			currentVaultPath := filepath.Join(testStateDir, "currentvault")
-			require.NoError(t, afero.WriteFile(fs, currentVaultPath,
-				[]byte(name), secret.FilePerms))
-
-			c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
-
-			var out bytes.Buffer
-
-			cmd := &cobra.Command{}
-			cmd.SetOut(&out)
-
-			err := c.ListSecrets(cmd, false, true, "")
-			require.ErrorIs(t, err, vault.ErrVaultNotFound)
-			require.ErrorContains(t, err, currentVaultPath)
-			require.ErrorContains(t, err, "secret vault select")
-			assert.Empty(t, out.String())
-
-			require.NoError(t, c.ListVaults(cmd, false))
-			assert.Equal(t, "Available vaults:\n  default\n  work\n", out.String())
-
-			require.NoError(t, c.SelectVault(cmd, "default"))
-			out.Reset()
-			require.NoError(t, c.ListSecrets(cmd, false, true, ""))
-			assert.Equal(t, "x\n", out.String())
-		})
-	}
-}

internal/cli/entry_test.go

@@ -3,7 +3,6 @@ package cli_test
 import (
 	"bufio"
 	"context"
-	"fmt"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -17,67 +16,29 @@ import (
 	"sneak.berlin/go/secret/internal/secret"
 )
 
-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"
-
-	// 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.
+// 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) {
-	t.Parallel()
+	savedArgs := os.Args
+
+	t.Cleanup(func() { os.Args = savedArgs })
 
 	tests := []struct {
 		args     []string
 		exitCode int
 	}{
-		{args: []string{"--help"}, exitCode: 0},
-		{args: []string{"no-such-command"}, exitCode: 1},
+		{args: []string{"secret", "--help"}, exitCode: 0},
+		{args: []string{"secret", "no-such-command"}, exitCode: 1},
 	}
 
 	for _, tt := range tests {
-		//nolint:gosec // G204: test runs its own test binary
-		cmd := exec.CommandContext(t.Context(), os.Args[0], tt.args...)
+		buf := memguard.NewBufferFromBytes([]byte("key material"))
+		os.Args = 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)
+		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)
 	}
 }
 

internal/cli/lock_test.go

@@ -627,10 +627,6 @@ func TestCommandsCheckAgainOnceLocked(t *testing.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)
 
@@ -662,26 +658,6 @@ func TestCommandsCheckAgainOnceLocked(t *testing.T) {
 			},
 			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 {
@@ -698,6 +674,22 @@ 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)
@@ -743,49 +735,6 @@ 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/move_cross_vault_test.go

@@ -1,315 +0,0 @@
-//nolint:testpackage // white-box test of unexported internals
-package cli
-
-import (
-	"errors"
-	"path/filepath"
-	"testing"
-	"time"
-
-	"github.com/awnumar/memguard"
-	"github.com/spf13/afero"
-	"github.com/spf13/cobra"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/secret/internal/secret"
-	"sneak.berlin/go/secret/internal/vault"
-)
-
-// errPermissionDenied is returned by renameFailFs and removeAllFailFs in
-// place of a rename or removal that succeeds.
-var errPermissionDenied = errors.New("permission denied")
-
-// renameFailFs fails every Rename of one path, as a read-only parent
-// directory does.
-type renameFailFs struct {
-	afero.Fs
-
-	path string
-}
-
-func (f *renameFailFs) Rename(oldname, newname string) error {
-	if oldname == f.path {
-		return errPermissionDenied
-	}
-
-	return f.Fs.Rename(oldname, newname)
-}
-
-// removeAllFailFs fails every RemoveAll of an entry of the directory dir,
-// as a file in it that cannot be deleted does, and records that entry's
-// path in failed.
-type removeAllFailFs struct {
-	afero.Fs
-
-	dir    string
-	failed string
-}
-
-func (f *removeAllFailFs) RemoveAll(path string) error {
-	if filepath.Dir(path) == f.dir {
-		f.failed = path
-
-		return errPermissionDenied
-	}
-
-	return f.Fs.RemoveAll(path)
-}
-
-// createCrossVaults creates in fs the vaults "work" and "default", where
-// "default" holds the secret "x" with the value "value".
-func createCrossVaults(t *testing.T, fs afero.Fs) {
-	t.Helper()
-
-	_, err := vault.CreateVault(fs, testStateDir, "work",
-		testMnemonicBuffer(t), nil)
-	require.NoError(t, err)
-
-	source, err := vault.CreateVault(fs, testStateDir, "default",
-		testMnemonicBuffer(t), nil)
-	require.NoError(t, err)
-
-	err = source.AddSecret("x",
-		memguard.NewBufferFromBytes([]byte("value")), false)
-	require.NoError(t, err)
-}
-
-// requireValue requires that the vault vaultName in fs holds secretName
-// with the value createCrossVaults stored.
-func requireValue(t *testing.T, fs afero.Fs, vaultName, secretName string) {
-	t.Helper()
-
-	vlt := vault.NewVault(fs, testStateDir, vaultName)
-	vlt.Mnemonic = testMnemonicBuffer(t)
-
-	value, err := vlt.GetSecret(secretName)
-	require.NoError(t, err, vaultName+":"+secretName)
-
-	defer value.Destroy()
-
-	require.Equal(t, []byte("value"), value.Bytes())
-}
-
-// TestCrossVaultMoveFailsWhenSourceRemains is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/159, where a move between two
-// vaults whose source could not be removed reported the secret moved and
-// succeeded. It must fail, and the copy and the source must both be there.
-// The state directory lock refuses the wrapped filesystem, so the test
-// calls copySecretAndRemoveSource, which a move between vaults runs once it
-// holds the lock.
-func TestCrossVaultMoveFailsWhenSourceRemains(t *testing.T) {
-	t.Parallel()
-
-	base := afero.NewMemMapFs()
-	createCrossVaults(t, base)
-
-	instance := NewCLIInstanceWithStateDir(&renameFailFs{
-		Fs:   base,
-		path: testStateDir + "/vaults.d/default/secrets.d/x",
-	}, testStateDir)
-
-	source := vault.NewVault(instance.fs, testStateDir, "default")
-	source.Mnemonic = testMnemonicBuffer(t)
-
-	err := instance.copySecretAndRemoveSource(&cobra.Command{}, source, "x",
-		vault.NewVault(instance.fs, testStateDir, "work"), "y", false)
-	require.ErrorIs(t, err, errPermissionDenied)
-	require.ErrorContains(t, err, "secret 'default:x' is copied with all "+
-		"1 version(s) to 'work:y', but the source remains")
-
-	requireValue(t, base, "default", "x")
-	requireValue(t, base, "work", "y")
-}
-
-// TestCrossVaultMoveFailsWhenOldFilesRemain makes the removal of the source
-// fail after the source has been moved out of its vault, when only the
-// temporary directory it was moved into cannot be deleted. The move must
-// fail, say that the secret is moved, and name that directory.
-func TestCrossVaultMoveFailsWhenOldFilesRemain(t *testing.T) {
-	t.Parallel()
-
-	base := afero.NewMemMapFs()
-	createCrossVaults(t, base)
-
-	sourceVaultDir := testStateDir + "/vaults.d/default"
-	fs := &removeAllFailFs{Fs: base, dir: sourceVaultDir}
-
-	instance := NewCLIInstanceWithStateDir(fs, testStateDir)
-
-	source := vault.NewVault(fs, testStateDir, "default")
-	source.Mnemonic = testMnemonicBuffer(t)
-
-	err := instance.copySecretAndRemoveSource(&cobra.Command{}, source, "x",
-		vault.NewVault(fs, testStateDir, "work"), "y", false)
-	require.ErrorIs(t, err, errPermissionDenied)
-	require.ErrorContains(t, err, "secret 'default:x' is moved with all "+
-		"1 version(s) to 'work:y', but its old files could not all be deleted")
-
-	// The error names the directory that holds the old files
-	require.NotEmpty(t, fs.failed)
-	require.ErrorContains(t, err, fs.failed)
-
-	oldFiles, err := afero.DirExists(base, filepath.Join(fs.failed, "x"))
-	require.NoError(t, err)
-	require.True(t, oldFiles)
-
-	// The source vault no longer holds the secret
-	remains, err := afero.DirExists(base, sourceVaultDir+"/secrets.d/x")
-	require.NoError(t, err)
-	require.False(t, remains)
-
-	requireValue(t, base, "work", "y")
-}
-
-// TestCrossVaultMoveUnlocksBeforeLocking is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/189: a move between two vaults
-// unlocked them while holding the state directory lock, and unlocking a
-// vault can ask for its passphrase, so every other command that changes
-// anything waited while the user answered. Here each vault has a passphrase
-// unlocker whose passphrase is the vault's name, and the move is given the
-// passphrase of only one of them. Run while the test holds the lock, it
-// must fail to unlock the other vault without waiting for the lock.
-//
-//nolint:paralleltest // times commands against the in-memory lock all tests share
-func TestCrossVaultMoveUnlocksBeforeLocking(t *testing.T) {
-	fs := afero.NewMemMapFs()
-	createCrossVaults(t, fs)
-
-	vaults, err := vault.ListVaults(fs, testStateDir)
-	require.NoError(t, err)
-
-	for _, name := range vaults {
-		vlt := vault.NewVault(fs, testStateDir, name)
-		vlt.Mnemonic = testMnemonicBuffer(t)
-
-		passphrase := memguard.NewBufferFromBytes([]byte(name))
-		_, err = vlt.CreatePassphraseUnlocker(passphrase)
-		passphrase.Destroy()
-		require.NoError(t, err)
-	}
-
-	// The move is from "default" to "work"
-	for _, tc := range []struct {
-		// failing is the vault that the move cannot unlock, given the
-		// passphrase of the vault named given
-		failing, given string
-	}{
-		{"source", "work"},
-		{"destination", "default"},
-	} {
-		t.Run(tc.failing, func(t *testing.T) {
-			passphrase := memguard.NewBufferFromBytes([]byte(tc.given))
-			defer passphrase.Destroy()
-
-			instance := NewCLIInstanceWithStateDir(fs, testStateDir)
-			instance.UnlockPassphrase = passphrase
-
-			release, err := vault.LockStateDir(fs, testStateDir)
-			require.NoError(t, err)
-			// Also frees a waiting move if the test fails, so that it
-			// releases the lock the other tests use
-			defer release()
-
-			done := make(chan error, 1)
-
-			go func() {
-				done <- instance.MoveSecret(&cobra.Command{},
-					"default:x", "work:y", false)
-			}()
-
-			select {
-			case err := <-done:
-				require.ErrorIs(t, err, secret.ErrWrongPassphrase)
-				require.ErrorContains(t, err,
-					"failed to unlock "+tc.failing+" vault")
-			case <-time.After(lockWait):
-				t.Fatal("waited for the lock before unlocking the " +
-					tc.failing + " vault")
-			}
-		})
-	}
-}
-
-// 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) {
-	mnemonic := testMnemonicBuffer(t)
-
-	otherMnemonic := memguard.NewBufferFromBytes([]byte("legal winner thank " +
-		"year wave sausage worth useful legal winner thank yellow"))
-	t.Cleanup(otherMnemonic.Destroy)
-
-	move := func(cli *Instance) error {
-		cli.Mnemonic = mnemonic
-
-		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,6 +2,7 @@ package cli
 
 import (
 	"os"
+	"time"
 
 	"github.com/awnumar/memguard"
 	"github.com/spf13/cobra"
@@ -14,6 +15,10 @@ 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

@@ -47,6 +47,10 @@ 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
@@ -430,9 +434,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 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.
+// user answers. If, once the lock is held, the add replaces a version after
+// all, because another command added the secret in the meantime, it fails
+// rather than ask with the lock held.
 func (cli *Instance) storeSecret(
 	vlt *vault.Vault, name string, value *memguard.LockedBuffer, force bool,
 ) error {
@@ -453,46 +457,24 @@ 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 false, err
+		return err
 	}
 	defer release()
 
-	replacing, err := replacesVersion(vlt, name, force)
-	if err != nil {
-		return false, err
+	if !replacing {
+		replacing, err = replacesVersion(vlt, name, force)
+		if err != nil {
+			return err
+		}
+
+		if replacing {
+			return fmt.Errorf("secret %s %w", name, errAddedMeanwhile)
+		}
 	}
 
-	if replacing && vlt.Locked() {
-		return true, nil
-	}
-
-	return false, vlt.AddSecret(name, value, force)
+	return vlt.AddSecret(name, value, force)
 }
 
 // replacesVersion reports whether adding the secret name to vlt replaces
@@ -875,12 +857,24 @@ func (cli *Instance) findSecretToRemove(
 	}, nil
 }
 
-// MoveSecret moves or renames a secret (within or across vaults). It
-// leaves taking the state directory lock to moveSecretWithinVault and
-// moveSecretCrossVault, since a move between vaults unlocks both vaults
-// before it takes the lock.
+// MoveSecret moves or renames a secret (within or across vaults), holding
+// the state directory lock.
 func (cli *Instance) MoveSecret(
 	cmd *cobra.Command, source, dest string, force bool,
+) error {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	return cli.moveSecret(cmd, source, dest, force)
+}
+
+// moveSecret does the work of MoveSecret. The caller holds the state
+// directory lock.
+func (cli *Instance) moveSecret(
+	cmd *cobra.Command, source, dest string, force bool,
 ) error {
 	// Parse source and destination
 	srcVaultName, srcSecretName, srcQualified := ParseVaultSecretRef(source)
@@ -980,9 +974,8 @@ func (cli *Instance) existingVault(name string) (*vault.Vault, error) {
 	return vault.NewVault(cli.fs, cli.stateDir, name), nil
 }
 
-// moveSecretWithinVault renames a secret within the vault vlt, holding the
-// state directory lock. Its caller, MoveSecret, has already checked both
-// secret names.
+// moveSecretWithinVault renames a secret within the vault vlt. Its caller,
+// MoveSecret, has already checked both secret names.
 func (cli *Instance) moveSecretWithinVault(
 	cmd *cobra.Command, vlt *vault.Vault, source, dest string, force bool,
 ) error {
@@ -992,12 +985,6 @@ func (cli *Instance) moveSecretWithinVault(
 		return fmt.Errorf("secret '%s' %w", source, errMoveOntoItself)
 	}
 
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	vaultDir, err := vlt.GetDirectory()
 	if err != nil {
 		return err
@@ -1084,75 +1071,59 @@ func (cli *Instance) sameDirectory(dir, other string) (bool, error) {
 
 // moveSecretCrossVault handles moving between two different vaults. Its
 // caller, MoveSecret, has already checked both secret names and that both
-// vaults exist. The copy needs the source vault's long-term key and is
-// encrypted to the destination vault's long-term public key. The
-// destination vault is unlocked to check that the copy will open with what
-// the user gave, and, once the state directory lock is held, that it still
-// has the key unlocked. Either unlock can ask for a passphrase, so both
-// happen before the lock is taken, and no other command waits while the
-// user answers.
+// vaults exist.
 func (cli *Instance) moveSecretCrossVault(
 	cmd *cobra.Command,
 	srcVault *vault.Vault, srcSecretName string,
 	destVault *vault.Vault, destSecretName string,
 	force bool,
 ) error {
-	// Before unlocking, check that the source secret exists, that the
-	// destination is not the source under another name, and, without
-	// --force, that the destination secret does not exist
-	_, err := cli.checkCrossVaultMove(
-		srcVault, srcSecretName, destVault, destSecretName, force)
+	srcVaultDir, err := srcVault.GetDirectory()
+	if err != nil {
+		return fmt.Errorf("failed to get source vault directory: %w", err)
+	}
+
+	// Verify source secret exists
+	srcStorageName := strings.ReplaceAll(srcSecretName, "/", "%")
+	srcSecretDir := filepath.Join(srcVaultDir, "secrets.d", srcStorageName)
+
+	exists, err := afero.DirExists(cli.fs, srcSecretDir)
+	if err != nil || !exists {
+		return fmt.Errorf("secret '%s' %w in vault '%s'",
+			srcSecretName, vault.ErrSecretNotFound, srcVault.Name)
+	}
+
+	// The source is removed after the copy, so a destination that is the
+	// source under another name would be lost with it.
+	destVaultDir, err := destVault.GetDirectory()
+	if err != nil {
+		return fmt.Errorf("failed to get destination vault directory: %w", err)
+	}
+
+	destStorageName := strings.ReplaceAll(destSecretName, "/", "%")
+	destSecretDir := filepath.Join(destVaultDir, "secrets.d", destStorageName)
+
+	same, err := cli.sameDirectory(srcSecretDir, destSecretDir)
 	if err != nil {
 		return err
 	}
 
+	if same {
+		return fmt.Errorf("secret '%s:%s' %w: '%s:%s' is the same secret on "+
+			"this filesystem", srcVault.Name, srcSecretName, errMoveOntoItself,
+			destVault.Name, destSecretName)
+	}
+
+	// Copying needs the long-term keys of both vaults
 	srcVault.Mnemonic, srcVault.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
 	destVault.Mnemonic, destVault.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
 
 	// Unlock destination vault (will fail if neither mnemonic nor unlocker available)
-	destIdentity, err := destVault.GetOrDeriveLongTermKey()
+	_, err = destVault.GetOrDeriveLongTermKey()
 	if err != nil {
 		return fmt.Errorf("failed to unlock destination vault '%s': %w", destVault.Name, err)
 	}
 
-	_, err = srcVault.GetOrDeriveLongTermKey()
-	if err != nil {
-		return fmt.Errorf("failed to unlock source vault '%s': %w",
-			srcVault.Name, err)
-	}
-
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
-	err = cli.checkLongTermKey(destVault, destIdentity)
-	if err != nil {
-		return err
-	}
-
-	return cli.copySecretAndRemoveSource(
-		cmd, srcVault, srcSecretName, destVault, destSecretName, force)
-}
-
-// copySecretAndRemoveSource copies the secret srcSecretName of srcVault,
-// with all its versions, to destSecretName in destVault, and removes the
-// source. moveSecretCrossVault calls it holding the state directory lock.
-// Another command may have changed either vault since moveSecretCrossVault
-// checked them, so it checks them again first.
-func (cli *Instance) copySecretAndRemoveSource(
-	cmd *cobra.Command,
-	srcVault *vault.Vault, srcSecretName string,
-	destVault *vault.Vault, destSecretName string,
-	force bool,
-) error {
-	srcSecretDir, err := cli.checkCrossVaultMove(
-		srcVault, srcSecretName, destVault, destSecretName, force)
-	if err != nil {
-		return err
-	}
-
 	// Count versions for user feedback
 	versions, _ := secret.ListVersions(cli.fs, srcSecretDir)
 	versionCount := len(versions)
@@ -1167,9 +1138,12 @@ func (cli *Instance) copySecretAndRemoveSource(
 	// Delete source secret
 	err = secret.RemoveDirAtomic(cli.fs, srcSecretDir)
 	if err != nil {
-		return cli.sourceRemovalError(err, srcSecretDir,
-			srcVault.Name+":"+srcSecretName, destVault.Name+":"+destSecretName,
-			versionCount)
+		// Copy succeeded but delete failed - warn but don't fail
+		cmd.Printf("Warning: copied secret but failed to remove source: %v\n", err)
+		cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",
+			srcVault.Name, srcSecretName, destVault.Name, destSecretName, versionCount)
+
+		return nil
 	}
 
 	cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",
@@ -1177,90 +1151,3 @@ func (cli *Instance) copySecretAndRemoveSource(
 
 	return nil
 }
-
-// checkCrossVaultMove checks that the secret srcSecretName exists in
-// srcVault, that destSecretName in destVault is not the source under
-// another name, and, unless force is set, that destSecretName does not
-// exist in destVault. It returns the source's directory.
-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 {
-		return "", fmt.Errorf("failed to get source vault directory: %w", err)
-	}
-
-	// Verify source secret exists
-	srcStorageName := strings.ReplaceAll(srcSecretName, "/", "%")
-	srcSecretDir := filepath.Join(srcVaultDir, "secrets.d", srcStorageName)
-
-	exists, err := afero.DirExists(cli.fs, srcSecretDir)
-	if err != nil || !exists {
-		return "", fmt.Errorf("secret '%s' %w in vault '%s'",
-			srcSecretName, vault.ErrSecretNotFound, srcVault.Name)
-	}
-
-	// The source is removed after the copy, so a destination that is the
-	// source under another name would be lost with it.
-	destVaultDir, err := destVault.GetDirectory()
-	if err != nil {
-		return "", fmt.Errorf("failed to get destination vault directory: %w", err)
-	}
-
-	destStorageName := strings.ReplaceAll(destSecretName, "/", "%")
-	destSecretDir := filepath.Join(destVaultDir, "secrets.d", destStorageName)
-
-	same, err := cli.sameDirectory(srcSecretDir, destSecretDir)
-	if err != nil {
-		return "", err
-	}
-
-	if same {
-		return "", fmt.Errorf("secret '%s:%s' %w: '%s:%s' is the same secret "+
-			"on this filesystem", srcVault.Name, srcSecretName, errMoveOntoItself,
-			destVault.Name, destSecretName)
-	}
-
-	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
-}
-
-// sourceRemovalError returns the error of a move between vaults that
-// copied all versionCount versions of the secret srcName to destName, and
-// then failed with removeErr to remove srcSecretDir, the directory of
-// srcName. The removal can fail after the source has left its vault, when
-// only the temporary directory it was moved into cannot be deleted, so the
-// error says the source remains only if srcSecretDir is still there.
-func (cli *Instance) sourceRemovalError(
-	removeErr error, srcSecretDir, srcName, destName string, versionCount int,
-) error {
-	remains, err := afero.DirExists(cli.fs, srcSecretDir)
-
-	switch {
-	case err != nil:
-		return fmt.Errorf("secret '%s' is copied with all %d version(s) "+
-			"to '%s', but removing the source failed and whether it "+
-			"remains is unknown: %w",
-			srcName, versionCount, destName, errors.Join(removeErr, err))
-	case remains:
-		return fmt.Errorf("secret '%s' is copied with all %d version(s) "+
-			"to '%s', but the source remains: %w",
-			srcName, versionCount, destName, removeErr)
-	default:
-		return fmt.Errorf("secret '%s' is moved with all %d version(s) "+
-			"to '%s', but its old files could not all be deleted: %w",
-			srcName, versionCount, destName, removeErr)
-	}
-}

internal/cli/secrets_size_test.go

@@ -36,11 +36,17 @@ func newSizeTestVault(t *testing.T) (afero.Fs, *vault.Vault) {
 
 	fs := afero.NewMemMapFs()
 
-	// Create vault, which selects it as the current vault
+	// Create vault
 	_, err := vault.CreateVault(fs, testStateDir, testVaultName,
 		testMnemonicBuffer(t), nil)
 	require.NoError(t, err)
 
+	// Set current vault
+	currentVaultPath := filepath.Join(testStateDir, "currentvault")
+	vaultPath := filepath.Join(testStateDir, "vaults.d", testVaultName)
+	err = afero.WriteFile(fs, currentVaultPath, []byte(vaultPath), 0o600)
+	require.NoError(t, err)
+
 	// Get vault and set up long-term key
 	vlt, err := vault.GetCurrentVault(fs, testStateDir)
 	require.NoError(t, err)

internal/cli/unlockers.go

@@ -15,7 +15,6 @@ import (
 	"strings"
 	"time"
 
-	"filippo.io/age"
 	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
 	"sneak.berlin/go/secret/internal/secret"
@@ -40,10 +39,6 @@ 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
@@ -429,11 +424,8 @@ func (cli *Instance) UnlockersAdd(unlockerType string, cmd *cobra.Command) error
 // 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()
+	_, err := vlt.GetOrDeriveLongTermKey()
 	if err != nil {
 		return nil, fmt.Errorf("failed to get long-term key: %w", err)
 	}
@@ -443,7 +435,7 @@ func (cli *Instance) unlockThenLock(vlt *vault.Vault) (func(), error) {
 		return nil, err
 	}
 
-	err = cli.checkLongTermKey(vlt, ltIdentity)
+	_, err = cli.existingVault(vlt.GetName())
 	if err != nil {
 		release()
 
@@ -453,33 +445,6 @@ func (cli *Instance) unlockThenLock(vlt *vault.Vault) (func(), error) {
 	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) {

internal/secret/atomic.go

@@ -149,9 +149,7 @@ func WriteDir(fs afero.Fs, dir string, write func(dir string) error) error {
 
 // RemoveDirAtomic deletes the directory dir so that it disappears in one
 // rename: dir is moved into a new directory from TempDirFor, which is then
-// deleted. A crash part-way leaves only that temporary directory behind. If
-// only deleting the temporary directory fails, dir is already gone, and the
-// error names the temporary directory, which holds what is left of it.
+// deleted. A crash part-way leaves only that temporary directory behind.
 func RemoveDirAtomic(fs afero.Fs, dir string) error {
 	tmp, err := TempDirFor(fs, dir)
 	if err != nil {
@@ -167,8 +165,7 @@ func RemoveDirAtomic(fs afero.Fs, dir string) error {
 
 	err = fs.RemoveAll(tmp)
 	if err != nil {
-		return fmt.Errorf("failed to remove %s after moving %s into it: %w",
-			tmp, dir, err)
+		return fmt.Errorf("failed to remove %s: %w", dir, err)
 	}
 
 	return nil

internal/secret/secret.go

@@ -28,6 +28,11 @@ 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,6 +118,12 @@ 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,6 +90,10 @@ 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.Mnemonic = mnemonic
+	vlt.SetMnemonic(mnemonic)
 
 	_, err := vlt.GetSecret(testSecretName)
 	require.ErrorIs(t, err, vault.ErrMnemonicMismatch)

internal/vault/management.go

@@ -55,10 +55,7 @@ func ValidateVaultName(name string) error {
 }
 
 // ResolveVaultSymlink reads the currentvault file to get the path to the current vault
-// The file contains just the vault name (e.g., "default"). An empty or
-// invalid name, or the name of a vault that does not exist, is an error
-// wrapping ErrVaultNotFound that names the file and says to run
-// 'secret vault select <name>'.
+// The file contains just the vault name (e.g., "default")
 func ResolveVaultSymlink(fs afero.Fs, currentVaultPath string) (string, error) {
 	secret.Debug("resolveVaultSymlink starting", "path", currentVaultPath)
 
@@ -77,21 +74,6 @@ func ResolveVaultSymlink(fs afero.Fs, currentVaultPath string) (string, error) {
 	stateDir := filepath.Dir(currentVaultPath)
 	absolutePath := filepath.Join(stateDir, "vaults.d", vaultName)
 
-	// An empty name would make vaults.d itself the vault's directory
-	exists := false
-	if isValidVaultName(vaultName) {
-		exists, err = afero.DirExists(fs, absolutePath)
-		if err != nil {
-			return "", fmt.Errorf("failed to check if vault exists: %w", err)
-		}
-	}
-
-	if !exists {
-		return "", fmt.Errorf("vault '%s' named in %s %w; run "+
-			"'secret vault select <name>' to select a vault",
-			vaultName, currentVaultPath, ErrVaultNotFound)
-	}
-
 	secret.Debug("Resolved to absolute path", "absolute_path", absolutePath)
 
 	return absolutePath, 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.UnlockPassphrase = passphrase
+	vault.SetUnlockPassphrase(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,6 +64,18 @@ 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.UnlockPassphrase = wrong
+	vlt.SetUnlockPassphrase(wrong)
 
 	secretDir := filepath.Join(testStateDir, "vaults.d", testVaultName,
 		"secrets.d", testSecretName)