Remove secret vault import (closes #139)

`secret vault import` 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, so the
command goes, with its tests and its README entry. The help and the
note of `secret generate mnemonic` named `secret import`, which imports
a file as a secret; they now name `secret init` and
`secret vault create`.

`secret vault import` now prints the help of `secret vault` and exits
0, as cobra does for any subcommand of `secret vault` it does not know.

Judgement call: TestWorkflowMismatch in internal/vault, which replayed
the import step by step, is deleted.

Model: opus-5-5

README.md

@@ -67,10 +67,6 @@ and unlocker set. This allows for complete isolation between different contexts
 
 ## Commands Reference
 
-Every time `secret` prints, in a table or in `--json` output, is in UTC,
-whatever `TZ` or the system's time zone is set to. The date in a version's name
-is the UTC date too.
-
 ### Confirmation Before Removal
 
 `secret rm`, `secret version rm`, `secret vault remove` and
@@ -94,8 +90,7 @@ remove things pass `--force`.
 #### `secret init`
 
 Initializes the secret manager with a default vault. Prompts for a BIP39
-mnemonic phrase and creates the initial directory structure. When the vault
-`default` already exists, it fails before asking for the mnemonic or passphrase.
+mnemonic phrase and creates the initial directory structure.
 
 **Environment Variables:**
 
@@ -110,19 +105,14 @@ Lists all available vaults. The current vault is marked.
 
 #### `secret vault create <name>`
 
-Creates a new vault with the specified name. When a vault of that name already
-exists, it fails before asking for the mnemonic or passphrase.
+Creates a new vault with the specified name.
 
 **Vault Name Format:** only lowercase ASCII letters, digits, `.`, `-` and `_`
 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` ⚠️ 🛑
 
@@ -137,24 +127,9 @@ one.
 
 ### Secret Management
 
-#### Secret Size
-
-While `secret` works on a secret's value it keeps the value in locked memory, so
-the value counts against the locked-memory limit (`ulimit -l`). `secret` raises
-that limit to the hard limit when it starts. `secret add`, `secret import` and
-`secret generate secret` refuse a value that does not fit in the limit with 8
-memory pages to spare (32 KiB with 4 KiB pages), with an error that names the
-limit. Linux's usual limit of 8 MiB allows a value of nearly 8 MiB; a larger one
-needs a higher hard limit, such as `memlock` in `/etc/security/limits.conf`. No
-value may be over 100 MiB, whatever the limit.
-
 #### `secret add <secret-name> [--force]`
 
-Adds a secret to the current vault. Reads the secret value from stdin. When the
-value ends in a newline, that one newline is dropped, so
-`echo value | secret add name` stores `value`; from `printf 'a\n\n'` it stores
-`a` and one newline. To keep a file's final newline, as in a PEM key, use
-`secret import`, which stores the file's bytes unchanged.
+Adds a secret to the current vault. Reads the secret value from stdin.
 
 - `--force, -f`: Overwrite existing secret
 
@@ -167,13 +142,7 @@ contain `//`, or have `..` as a path segment.
 
 #### `secret get <secret-name> [--version <version>]`
 
-Writes the secret's value to stdout exactly as stored, with no newline added
-after it. A failed write of the value, such as to a file on a full disk, prints
-the error and exits 1, with two exceptions. If the program reading from a pipe
-has already exited, as in `secret get s | true`, the broken-pipe signal
-(SIGPIPE) ends `secret` with no message and the shell reports exit status 141.
-If the shell closed stdout before `secret` started (`secret get s >&-`), the
-value is written to `/dev/null` and `secret get` exits 0.
+Retrieves and outputs a secret value to stdout.
 
 - `--version, -v`: Get a specific version (default: current)
 
@@ -181,13 +150,6 @@ value is written to `/dev/null` and `secret get` exits 0.
 
 Lists all secrets in the current vault. Optional filter for substring matching.
 
-The `LAST UPDATED` column, and `updated_at` in the `--json` output, give the
-time the secret's `current` file was last written: every command that adds a
-version replaces it, and so does `secret version promote`. `created_at` gives
-the time the `value.age` of the secret's oldest version was written. Both are
-the files' modification times, so listing asks for no passphrase. Moving a
-secret to another vault keeps both times.
-
 #### `secret remove <secret-name> [--force]` / `secret rm` ⚠️ 🛑
 
 **DANGER**: Permanently removes a secret and ALL its versions. It first asks for
@@ -216,18 +178,10 @@ Moves or renames a secret within the current vault.
 Lists all versions of a secret showing creation time, status, and validity
 period.
 
-`STATUS` is `current` for the secret's current version, `expired` for another
-version whose `NOT_AFTER` has passed, and `inactive` for any other version.
-Adding a version sets `NOT_AFTER` on the version that was current;
-`secret version promote` sets no times. A version that is not current therefore
-shows `inactive` when no version was added while it was current.
-
 #### `secret version promote <secret-name> <version>`
 
 Promotes a specific version to current by rewriting the secret's `current` file
-to name it. The versions' own times are left unchanged, allowing for rollback
-scenarios; `secret list` then gives the time of the promote as the secret's last
-update.
+to name it. Does not modify any timestamps, allowing for rollback scenarios.
 
 #### `secret version remove <secret-name> <version> [--force]` / `secret version rm` ⚠️ 🛑
 
@@ -279,12 +233,6 @@ Creates a new unlocker of the specified type:
 A vault has one passphrase unlocker: adding one replaces the one the vault has,
 which is removed only once the new one is the current unlocker.
 
-`secret unlocker add passphrase` asks on the terminal for the new passphrase
-even when `SB_UNLOCK_PASSPHRASE` is set, and uses the variable to unlock the
-vault. With no controlling terminal, as under cron or in a CI job, the new
-unlocker takes its passphrase from `SB_UNLOCK_PASSPHRASE`, and the command says
-so.
-
 #### `secret unlocker remove <unlocker-id> [--force]` / `secret unlocker rm` ⚠️ 🛑
 
 **DANGER**: Permanently removes an unlocker. It first asks for confirmation,
@@ -318,16 +266,10 @@ Selects an unlocker as the current default for operations.
 #### `secret import <secret-name> --source <filename>`
 
 Imports a secret from a file and stores it in the current vault under the given
-name. The file's bytes are stored unchanged, a final newline included.
+name.
 
 ### 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,
@@ -438,14 +380,6 @@ 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
@@ -582,11 +516,7 @@ secret decrypt encryption/mykey --input document.txt.age --output document.txt
 ### Vault Management
 
 - **Derivation Index**: Each vault uses a unique derivation index from the
-  mnemonic, and thus a unique key pair; creating a vault (`secret init` and
-  `secret vault create`) fails, with an error naming the file, when any vault's
-  `vault-metadata.json` exists but cannot be read or parsed; a vault directory
-  with no `vault-metadata.json` is skipped, so the index it uses can be given to
-  a new vault
+  mnemonic, and thus a unique key pair
 - **Public Key Hash**: Double SHA-256 hash of the vault's public key; the same
   hash of the index-0 public key identifies vaults from the same mnemonic
 - **Automatic Key Derivation**: When creating vaults with a mnemonic, keys are

internal/cli/create_vault_test.go

@@ -144,20 +144,25 @@ func TestVaultCreationLeavesNoSecretInEnvironment(t *testing.T) {
 // TestNoVaultImportCommand is a regression test for
 // https://git.eeqj.de/sneak/secret/issues/139: `secret vault import`
 // refused every vault that `secret vault create` makes, because each has a
-// long-term key. The command is gone, so it fails as any subcommand of
-// `secret vault` that does not exist does.
+// long-term key. The command is gone, so cobra answers it with the help of
+// `secret vault`, as it does any subcommand of `secret vault` it does not
+// know.
 func TestNoVaultImportCommand(t *testing.T) {
 	t.Setenv(secret.EnvStateDir, t.TempDir())
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-	t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
 
-	_, err := cli.ExecuteCommandInProcess(
-		strings.Fields("vault create work"), "", nil)
-	require.NoError(t, err)
+	run := func(args ...string) string {
+		t.Setenv(secret.EnvMnemonic, testMnemonic)
+		t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
 
-	_, err = cli.ExecuteCommandInProcess(
-		strings.Fields("vault import work"), "", nil)
-	require.EqualError(t, err, `unknown command "import" for "secret vault"`)
+		output, err := cli.ExecuteCommandInProcess(args, "", nil)
+		require.NoError(t, err, "%v", args)
+
+		return output
+	}
+
+	run("vault", "create", "work")
+
+	require.Equal(t, run("vault"), run("vault", "import", "work"))
 }
 
 // TestStopAtPassphrasePromptLeavesNothing is a regression test for the
@@ -241,51 +246,12 @@ func TestMnemonicNotReadNamesOnlyMnemonic(t *testing.T) {
 	output, err := cmd.CombinedOutput()
 	require.Error(t, err)
 
-	require.Equal(t, "Error: failed to read mnemonic: there is no terminal to "+
-		"ask on. Please set the SB_SECRET_MNEMONIC environment variable or run "+
+	require.Equal(t, "Initialized secrets manager at: "+stateDir+"\n"+
+		"Error: failed to read mnemonic: there is no terminal to ask on. "+
+		"Please set the SB_SECRET_MNEMONIC environment variable or run "+
 		"interactively\n", string(output))
 }
 
-// TestCreateExistingVaultAsksForNothing is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/151: `secret init` and `secret
-// vault create` asked for the mnemonic and the unlocker passphrase before
-// refusing a vault that already exists, and `secret init` printed
-// "Initialized secrets manager at" first. Each runs here as the built binary,
-// in a new session, which has no terminal, and with neither
-// SB_SECRET_MNEMONIC nor SB_UNLOCK_PASSPHRASE set, so asking for either
-// fails. Each must fail saying only that the vault exists.
-func TestCreateExistingVaultAsksForNothing(t *testing.T) {
-	t.Parallel()
-
-	stateDir := t.TempDir()
-	_, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default", nil, nil)
-	require.NoError(t, err)
-
-	// run runs the built binary with args, which must fail, and returns what
-	// it printed
-	run := func(args ...string) string {
-		//nolint:gosec // G204: test executes the freshly built secret binary
-		cmd := exec.CommandContext(t.Context(), secretBinaryPath(t), args...)
-		cmd.Env = []string{
-			secret.EnvStateDir + "=" + stateDir,
-			"PATH=" + os.Getenv("PATH"),
-			"HOME=" + os.Getenv("HOME"),
-		}
-		cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true}
-
-		output, err := cmd.CombinedOutput()
-		require.Error(t, err, "secret %v", args)
-
-		return string(output)
-	}
-
-	require.Equal(t,
-		"Error: failed to create default vault: vault default already exists\n",
-		run("init"))
-	require.Equal(t, "Error: vault default already exists\n",
-		run("vault", "create", "default"))
-}
-
 // TestStopDuringCreateLeavesWholeVaultOrNone is a regression test for
 // https://git.eeqj.de/sneak/secret/issues/105: `secret init` or `secret vault
 // create` killed after the passphrase prompt but before the unlocker was

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

@@ -29,12 +29,6 @@ func newGenerateCmd() *cobra.Command {
 		Use:   "generate",
 		Short: "Generate random data",
 		Long:  `Generate various types of random data including mnemonics and secrets.`,
-		// Cobra checks Args only on a command that has a RunE; without
-		// one, an unknown subcommand prints this help and exits 0.
-		Args: cobra.NoArgs,
-		RunE: func(cmd *cobra.Command, _ []string) error {
-			return cmd.Help()
-		},
 	}
 
 	cmd.AddCommand(newGenerateMnemonicCmd())
@@ -78,12 +72,6 @@ func newGenerateSecretCmd() *cobra.Command {
 				return fmt.Errorf("failed to initialize CLI: %w", err)
 			}
 
-			// Before readSecretEnv, which makes the first locked allocation
-			err = checkLockedMemoryLimit(0)
-			if err != nil {
-				return err
-			}
-
 			destroySecrets := cli.readSecretEnv()
 			defer destroySecrets()
 
@@ -147,16 +135,10 @@ func (cli *Instance) GenerateSecret(
 		return errLengthTooSmall
 	}
 
-	if length > maxSecretSize {
-		return errSecretTooLarge
-	}
-
-	err := checkLockedMemoryLimit(length)
-	if err != nil {
-		return err
-	}
-
-	var secretValue string
+	var (
+		secretValue string
+		err         error
+	)
 
 	switch secretType {
 	case "base58":
@@ -175,6 +157,12 @@ 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 {
@@ -187,7 +175,7 @@ func (cli *Instance) GenerateSecret(
 	secretBuffer := memguard.NewBufferFromBytes([]byte(secretValue))
 	defer secretBuffer.Destroy()
 
-	err = cli.storeSecret(vlt, secretName, secretBuffer, force)
+	err = vlt.AddSecret(secretName, secretBuffer, force)
 	if err != nil {
 		return err
 	}

internal/cli/info_helper.go

@@ -18,23 +18,24 @@ type vaultStats struct {
 
 // addVersion accumulates size and timestamp info for one version directory
 func (s *vaultStats) addVersion(fs afero.Fs, versionPath string) {
-	// Add the size of every file in the version directory: its value,
-	// metadata and keys. A directory that cannot be read adds nothing and
-	// gets a warning; its timestamp is still tracked below.
-	files, err := afero.ReadDir(fs, versionPath)
-	if err != nil {
-		secret.Warn("Could not read version directory",
-			"directory", versionPath, "error", err)
+	// Add size of encrypted data
+	dataPath := filepath.Join(versionPath, "data.age")
+
+	stat, err := fs.Stat(dataPath)
+	if err == nil {
+		s.totalSize += stat.Size()
 	}
 
-	for _, file := range files {
-		if !file.IsDir() {
-			s.totalSize += file.Size()
-		}
+	// Add size of metadata
+	metaPath := filepath.Join(versionPath, "metadata.age")
+
+	stat, err = fs.Stat(metaPath)
+	if err == nil {
+		s.totalSize += stat.Size()
 	}
 
 	// Track timestamps
-	stat, err := fs.Stat(versionPath)
+	stat, err = fs.Stat(versionPath)
 	if err == nil {
 		modTime := stat.ModTime()
 		if s.oldestTime.IsZero() || modTime.Before(s.oldestTime) {

internal/cli/info_helper_test.go

@@ -1,51 +0,0 @@
-//nolint:testpackage // white-box test of unexported internals
-package cli
-
-import (
-	"io"
-	"os"
-	"path/filepath"
-	"testing"
-
-	"github.com/spf13/afero"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-// TestAddVersionWarnsWhenVersionDirUnreadable asserts that a version
-// directory that cannot be listed, so that its files are left out of the
-// total size, gets a warning naming it and the error, and that its
-// timestamp is still tracked.
-//
-//nolint:paralleltest // replaces os.Stderr, where secret.Warn writes
-func TestAddVersionWarnsWhenVersionDirUnreadable(t *testing.T) {
-	versionDir := filepath.Join(testVaultDir(listTestVaultName),
-		unreadableTestSecretsDirName, unreadableTestSecretName,
-		"versions", "20260809.001")
-
-	base := afero.NewMemMapFs()
-	require.NoError(t, base.MkdirAll(versionDir, listTestDirPerm))
-
-	stderrReader, stderrWriter, err := os.Pipe()
-	require.NoError(t, err)
-
-	defer func() { _ = stderrReader.Close() }()
-
-	realStderr := os.Stderr
-	os.Stderr = stderrWriter
-
-	var stats vaultStats
-	stats.addVersion(&openFailFs{Fs: base, path: versionDir}, versionDir)
-
-	os.Stderr = realStderr
-
-	require.NoError(t, stderrWriter.Close())
-
-	warning, err := io.ReadAll(stderrReader)
-	require.NoError(t, err)
-
-	assert.Contains(t, string(warning), versionDir)
-	assert.Contains(t, string(warning), errOpenFailed.Error())
-	assert.False(t, stats.latestTime.IsZero(),
-		"the timestamp must still be tracked")
-}

internal/cli/info_test.go

@@ -1,64 +0,0 @@
-package cli_test
-
-import (
-	"bytes"
-	"crypto/rand"
-	"encoding/json"
-	"testing"
-
-	"github.com/awnumar/memguard"
-	"github.com/spf13/afero"
-	"github.com/spf13/cobra"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/secret/internal/cli"
-	"sneak.berlin/go/secret/internal/vault"
-)
-
-// TestInfoTotalSizeCountsSecretValues is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/147, where the total size shown by
-// `secret info` left out every secret's value. Adding a secret must make it
-// grow by at least the size of the value.
-func TestInfoTotalSizeCountsSecretValues(t *testing.T) {
-	t.Parallel()
-
-	const valueSize = 10000
-
-	fs := afero.NewMemMapFs()
-
-	vlt, err := vault.CreateVault(fs, testStateDir, "default",
-		testMnemonicBuffer(t), nil)
-	require.NoError(t, err)
-
-	before := infoTotalSize(t, fs)
-
-	valueBytes := make([]byte, valueSize)
-	_, err = rand.Read(valueBytes)
-	require.NoError(t, err)
-
-	value := memguard.NewBufferFromBytes(valueBytes)
-	defer value.Destroy()
-
-	require.NoError(t, vlt.AddSecret("s1", value, false))
-
-	after := infoTotalSize(t, fs)
-	require.GreaterOrEqual(t, after-before, int64(valueSize))
-}
-
-// infoTotalSize returns the total size that `secret info --json` reports for
-// the state directory on fs.
-func infoTotalSize(t *testing.T, fs afero.Fs) int64 {
-	t.Helper()
-
-	var out bytes.Buffer
-
-	cmd := &cobra.Command{}
-	cmd.SetOut(&out)
-
-	err := cli.NewCLIInstanceWithStateDir(fs, testStateDir).Info(cmd, true)
-	require.NoError(t, err)
-
-	var info cli.InfoOutput
-	require.NoError(t, json.Unmarshal(out.Bytes(), &info))
-
-	return info.TotalSize
-}

internal/cli/init.go

@@ -17,9 +17,6 @@ import (
 // errPassphraseMismatch is returned when passphrase confirmation fails
 var errPassphraseMismatch = errors.New("passphrases do not match")
 
-// defaultVaultName is the name of the vault `secret init` creates
-const defaultVaultName = "default"
-
 // NewInitCmd creates the init command
 func NewInitCmd() *cobra.Command {
 	return &cobra.Command{
@@ -67,17 +64,36 @@ func (cli *Instance) promptMnemonic() (*memguard.LockedBuffer, func(), error) {
 	return mnemonicBuffer, mnemonicBuffer.Destroy, nil
 }
 
-// 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.
+// Init initializes the secret manager, holding the state directory 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")
 
-	// Refuse an existing default vault before asking for anything
-	err := vault.CheckNewVaultName(cli.fs, cli.stateDir, defaultVaultName)
+	// 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 {
-		return fmt.Errorf("failed to create default vault: %w", err)
+		secret.Debug("Failed to create state directory", "error", err)
+
+		return fmt.Errorf("failed to create state directory: %w", err)
+	}
+
+	if cmd != nil {
+		cmd.Printf("Initialized secrets manager at: %s\n", stateDir)
 	}
 
 	// Prompt for mnemonic
@@ -114,38 +130,11 @@ func (cli *Instance) Init(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, passphrase)
+	vlt, err := vault.CreateVault(cli.fs, cli.stateDir, "default",
+		mnemonic, passphraseBuffer)
 	if err != nil {
 		secret.Debug("Failed to create default vault", "error", err)
 
@@ -163,7 +152,6 @@ func (cli *Instance) initialize(
 	}
 
 	if cmd != nil {
-		cmd.Printf("Initialized secrets manager at: %s\n", stateDir)
 		cmd.Printf("\nDefault vault created and configured\n")
 		cmd.Printf("Long-term public key: %s\n", ltIdentity.Recipient().String())
 		cmd.Printf("Unlocker ID: %s\n", unlocker.GetID())

internal/cli/list_times_test.go

@@ -1,215 +0,0 @@
-package cli_test
-
-import (
-	"bytes"
-	"encoding/json"
-	"os"
-	"os/exec"
-	"path/filepath"
-	"testing"
-	"time"
-
-	"github.com/awnumar/memguard"
-	"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"
-)
-
-const (
-	// listClockStep is how long TestListShowsWhenSecretWasWritten waits
-	// between its steps, so that a time read in one step cannot be the
-	// time read in the next.
-	listClockStep = 200 * time.Millisecond
-
-	// fileTimeSlack allows for the clock the kernel stamps files with,
-	// which advances only every few milliseconds: a file's modification
-	// time can be slightly earlier than a time.Now() read before the file
-	// was written.
-	fileTimeSlack = 100 * time.Millisecond
-)
-
-// TestListShowsWhenSecretWasWritten is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/142, where `secret list` gave
-// the time of the listing as every secret's creation and last-update time.
-// Two listings with a clock step between must be the same, and give the
-// time of the add; after a forced add, the creation time is unchanged and
-// the last update is the time of that add. It runs on a real filesystem,
-// where writing a file into a directory also changes the directory's time.
-func TestListShowsWhenSecretWasWritten(t *testing.T) {
-	t.Parallel()
-
-	fs := afero.NewOsFs()
-	stateDir := t.TempDir()
-
-	vlt, err := vault.CreateVault(fs, stateDir, "default", testMnemonicBuffer(t), nil)
-	require.NoError(t, err)
-
-	vlt.Mnemonic = testMnemonicBuffer(t)
-	c := cli.NewCLIInstanceWithStateDir(fs, stateDir)
-
-	beforeAdd := time.Now()
-	err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("one")), false)
-	require.NoError(t, err)
-
-	afterAdd := time.Now()
-
-	firstListing := listSecrets(t, c, true)
-
-	time.Sleep(listClockStep)
-	require.Equal(t, firstListing, listSecrets(t, c, true))
-
-	added := listedTimes(t, firstListing)
-	assert.WithinRange(t, added.CreatedAt, beforeAdd.Add(-fileTimeSlack), afterAdd)
-	assert.WithinRange(t, added.UpdatedAt, beforeAdd.Add(-fileTimeSlack), afterAdd)
-
-	time.Sleep(listClockStep)
-
-	beforeUpdate := time.Now()
-	err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("two")), true)
-	require.NoError(t, err)
-
-	afterUpdate := time.Now()
-
-	updated := listedTimes(t, listSecrets(t, c, true))
-	assert.True(t, added.CreatedAt.Equal(updated.CreatedAt),
-		"created %s, then %s", added.CreatedAt, updated.CreatedAt)
-	assert.WithinRange(t, updated.UpdatedAt,
-		beforeUpdate.Add(-fileTimeSlack), afterUpdate)
-
-	// The table's LAST UPDATED column gives the same time, to the minute
-	assert.Contains(t, listSecrets(t, c, false),
-		updated.UpdatedAt.Format("2006-01-02 15:04"))
-}
-
-// TestMovedSecretKeepsItsTimes checks that a secret moved to another vault
-// is listed there with the times it was listed with before the move, and
-// not with the time of the move.
-func TestMovedSecretKeepsItsTimes(t *testing.T) {
-	t.Parallel()
-
-	fs := afero.NewOsFs()
-	stateDir := t.TempDir()
-
-	_, err := vault.CreateVault(fs, stateDir, "other", testMnemonicBuffer(t), nil)
-	require.NoError(t, err)
-
-	// "default" is created last, so it is the current vault
-	vlt, err := vault.CreateVault(fs, stateDir, "default", testMnemonicBuffer(t), nil)
-	require.NoError(t, err)
-
-	vlt.Mnemonic = testMnemonicBuffer(t)
-
-	// Two versions written apart, so that the creation and the last update
-	// are different times
-	err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("one")), false)
-	require.NoError(t, err)
-
-	time.Sleep(listClockStep)
-
-	err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("two")), true)
-	require.NoError(t, err)
-
-	c := cli.NewCLIInstanceWithStateDir(fs, stateDir)
-	c.Mnemonic = testMnemonicBuffer(t)
-	beforeMove := listedTimes(t, listSecrets(t, c, true))
-
-	time.Sleep(listClockStep)
-	require.NoError(t, c.MoveSecret(&cobra.Command{}, "default:x", "other", false))
-	require.NoError(t, vault.SelectVault(fs, stateDir, "other"))
-
-	afterMove := listedTimes(t, listSecrets(t, c, true))
-	assert.True(t, beforeMove.CreatedAt.Equal(afterMove.CreatedAt),
-		"created %s, then %s", beforeMove.CreatedAt, afterMove.CreatedAt)
-	assert.True(t, beforeMove.UpdatedAt.Equal(afterMove.UpdatedAt),
-		"updated %s, then %s", beforeMove.UpdatedAt, afterMove.UpdatedAt)
-}
-
-// TestListedTimesAreUTC is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/169, where times were printed
-// in the local time zone. It runs `secret list` on the built binary with
-// TZ set to New York, where the secret's last update, 03:04:05 UTC on 2
-// January, is 22:04:05 the day before.
-func TestListedTimesAreUTC(t *testing.T) {
-	t.Parallel()
-
-	stateDir := t.TempDir()
-
-	vlt, err := vault.CreateVault(
-		afero.NewOsFs(), stateDir, "default", testMnemonicBuffer(t), nil)
-	require.NoError(t, err)
-
-	vlt.Mnemonic = testMnemonicBuffer(t)
-	err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("one")), false)
-	require.NoError(t, err)
-
-	updated := time.Date(2026, time.January, 2, 3, 4, 5, 0, time.UTC)
-	current := filepath.Join(stateDir, "vaults.d", "default", "secrets.d", "x",
-		"current")
-	require.NoError(t, os.Chtimes(current, updated, updated))
-
-	tests := []struct {
-		flags []string
-		want  string
-	}{
-		{flags: nil, want: "2026-01-02 03:04"},
-		{
-			flags: []string{"--json"},
-			want:  `"updated_at": "2026-01-02T03:04:05Z"`,
-		},
-	}
-
-	for _, tt := range tests {
-		args := append([]string{"list"}, tt.flags...)
-
-		//nolint:gosec // G204: test executes the freshly built secret binary
-		cmd := exec.CommandContext(t.Context(), secretBinaryPath(t), args...)
-		cmd.Env = []string{
-			secret.EnvStateDir + "=" + stateDir,
-			"TZ=America/New_York",
-			"PATH=" + os.Getenv("PATH"),
-		}
-
-		output, err := cmd.CombinedOutput()
-		require.NoError(t, err, "%v: %s", args, output)
-		assert.Contains(t, string(output), tt.want, "%v", args)
-	}
-}
-
-// listSecrets returns the output of `secret list`, or of
-// `secret list --json` when jsonOutput is set.
-func listSecrets(t *testing.T, c *cli.Instance, jsonOutput bool) string {
-	t.Helper()
-
-	var out bytes.Buffer
-
-	cmd := &cobra.Command{}
-	cmd.SetOut(&out)
-	require.NoError(t, c.ListSecrets(cmd, jsonOutput, false, ""))
-
-	return out.String()
-}
-
-// listedTimes returns the times that the output of `secret list --json`
-// gives for the one secret in the vault.
-func listedTimes(t *testing.T, listing string) secret.Metadata {
-	t.Helper()
-
-	var parsed struct {
-		Secrets []map[string]string `json:"secrets"`
-	}
-
-	require.NoError(t, json.Unmarshal([]byte(listing), &parsed))
-	require.Len(t, parsed.Secrets, 1)
-
-	createdAt, err := time.Parse(time.RFC3339Nano, parsed.Secrets[0]["created_at"])
-	require.NoError(t, err)
-
-	updatedAt, err := time.Parse(time.RFC3339Nano, parsed.Secrets[0]["updated_at"])
-	require.NoError(t, err)
-
-	return secret.Metadata{CreatedAt: createdAt, UpdatedAt: updatedAt}
-}

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 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.
+// 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.
 // 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,25 +346,6 @@ 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
@@ -404,7 +385,17 @@ func requireWaitsForLock(
 
 	go func() { done <- run(cli, olderVersion, unlockerID) }()
 
-	requireWaitingForLock(t, done)
+	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):
+		}
+	}
 
 	assert.Equal(t, before, stateDirModTimes(t, fs),
 		"changed the state directory before waiting for the lock")
@@ -481,311 +472,6 @@ 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

@@ -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/passphrase_prompt_test.go

@@ -138,148 +138,6 @@ func TestInitWritesNoBlankLineAfterMnemonic(t *testing.T) {
 		"stderr: %q", stderr.String())
 }
 
-// TestUnlockerAddAsksForNewPassphrase is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/154: with SB_UNLOCK_PASSPHRASE set,
-// `secret unlocker add passphrase` gave the new unlocker the passphrase in
-// the variable without asking. At a terminal it must ask for the new
-// passphrase there, twice, and unlock the vault with the variable, so that
-// `secret get` then opens the vault with the passphrase typed. pty.Open is
-// described above TestRemoveIgnoresTerminalOnStdout.
-func TestUnlockerAddAsksForNewPassphrase(t *testing.T) {
-	t.Parallel()
-
-	const newPassphrase = "new-passphrase"
-
-	stateDir := t.TempDir()
-
-	passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
-	defer passphrase.Destroy()
-
-	vlt, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default",
-		testMnemonicBuffer(t), passphrase)
-	require.NoError(t, err)
-
-	value := memguard.NewBufferFromBytes([]byte("v1"))
-	defer value.Destroy()
-
-	require.NoError(t, vlt.AddSecret("s1", value, false))
-
-	ctx, cancel := context.WithTimeout(t.Context(), commandWait)
-	defer cancel()
-
-	//nolint:gosec // G204: test executes the freshly built secret binary
-	cmd := exec.CommandContext(ctx, secretBinaryPath(t),
-		"unlocker", "add", "passphrase")
-	cmd.Env = []string{
-		secret.EnvStateDir + "=" + stateDir,
-		secret.EnvUnlockPassphrase + "=" + testPassphrase,
-		"PATH=" + os.Getenv("PATH"),
-		"HOME=" + os.Getenv("HOME"),
-	}
-
-	ptmx, tty, err := pty.Open()
-	require.NoError(t, err)
-
-	defer func() { _ = ptmx.Close() }()
-
-	deadline, _ := ctx.Deadline()
-	require.NoError(t, ptmx.SetReadDeadline(deadline))
-
-	cmd.Stderr = tty
-	// Ctty is a descriptor in the new process: 2, its stderr.
-	cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true, Setctty: true, Ctty: 2}
-
-	require.NoError(t, cmd.Start())
-
-	_ = tty.Close()
-
-	var (
-		shown []byte
-		char  byte
-	)
-
-	terminal := bufio.NewReader(ptmx)
-
-	for _, prompt := range []string{
-		"Enter passphrase for unlocker: ", "Confirm passphrase: ",
-	} {
-		for !bytes.HasSuffix(shown, []byte(prompt)) {
-			char, err = terminal.ReadByte()
-			require.NoError(t, err,
-				"secret unlocker add passphrase did not ask on the terminal: %s",
-				shown)
-
-			shown = append(shown, char)
-		}
-
-		_, err = ptmx.WriteString(newPassphrase + "\n")
-		require.NoError(t, err)
-	}
-
-	// The read ends once the command has exited and so closed the terminal.
-	rest, _ := io.ReadAll(terminal)
-
-	require.NoError(t, cmd.Wait(),
-		"secret unlocker add passphrase: %s%s", shown, rest)
-
-	// The binary, not this test process, opens the vault: the new unlocker
-	// has age's full scrypt work factor, which is slow under the race
-	// detector.
-	assert.Equal(t, "v1",
-		runWithoutTerminal(t, stateDir, newPassphrase, "get", "s1"))
-}
-
-// TestUnlockerAddWithoutTerminalSaysPassphraseIsVariable is a regression
-// test for https://git.eeqj.de/sneak/secret/issues/154: with no terminal to
-// ask on, `secret unlocker add passphrase` gives the new unlocker the
-// passphrase in SB_UNLOCK_PASSPHRASE, and its output must say so.
-func TestUnlockerAddWithoutTerminalSaysPassphraseIsVariable(t *testing.T) {
-	t.Parallel()
-
-	stateDir := t.TempDir()
-
-	passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
-	defer passphrase.Destroy()
-
-	_, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default",
-		testMnemonicBuffer(t), passphrase)
-	require.NoError(t, err)
-
-	output := runWithoutTerminal(t, stateDir, testPassphrase,
-		"unlocker", "add", "passphrase")
-
-	assert.Contains(t, output, "The new unlocker takes its passphrase "+
-		"from SB_UNLOCK_PASSPHRASE, since there is no terminal to ask for a "+
-		"new one on\n")
-}
-
-// runWithoutTerminal runs the secret binary with args and SB_UNLOCK_PASSPHRASE
-// set to passphrase, in a new session, which has no terminal, and returns
-// what it printed once it has succeeded.
-func runWithoutTerminal(
-	t *testing.T, stateDir, passphrase string, args ...string,
-) string {
-	t.Helper()
-
-	ctx, cancel := context.WithTimeout(t.Context(), commandWait)
-	defer cancel()
-
-	//nolint:gosec // G204: test executes the freshly built secret binary
-	cmd := exec.CommandContext(ctx, secretBinaryPath(t), args...)
-	cmd.Env = []string{
-		secret.EnvStateDir + "=" + stateDir,
-		secret.EnvUnlockPassphrase + "=" + passphrase,
-		"PATH=" + os.Getenv("PATH"),
-		"HOME=" + os.Getenv("HOME"),
-	}
-	cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true}
-
-	output, err := cmd.CombinedOutput()
-	require.NoError(t, err, "secret %v: %s", args, output)
-
-	return string(output)
-}
-
 // runAtTerminal runs the secret binary with args and input on stdin, in a
 // new session whose controlling terminal is a new terminal, which is also
 // its stderr: a command typed at a terminal with its input piped. pty.Open

internal/cli/root.go

@@ -2,7 +2,6 @@ package cli
 
 import (
 	"os"
-	"time"
 
 	"github.com/awnumar/memguard"
 	"github.com/spf13/cobra"
@@ -15,12 +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
 	// and exits with status 1. The passphrase prompt turns echo off on the
 	// controlling terminal until the read finishes, so a signal there would
@@ -58,19 +51,6 @@ func Entry() int {
 	return 0
 }
 
-// raiseLockedMemoryLimit raises this process's limit on locked memory, which
-// holds every secret value, to the hard limit, the most a process may set
-// without privileges. When that fails the limit stays as it was, and
-// checkLockedMemoryLimit names it in its error.
-func raiseLockedMemoryLimit() {
-	var limit unix.Rlimit
-
-	if unix.Getrlimit(unix.RLIMIT_MEMLOCK, &limit) == nil {
-		limit.Cur = limit.Max
-		_ = unix.Setrlimit(unix.RLIMIT_MEMLOCK, &limit)
-	}
-}
-
 func newRootCmd() *cobra.Command {
 	secret.Debug("newRootCmd starting")
 

internal/cli/secrets.go

@@ -14,7 +14,6 @@ import (
 	"github.com/awnumar/memguard"
 	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
-	"golang.org/x/sys/unix"
 	"sneak.berlin/go/secret/internal/secret"
 	"sneak.berlin/go/secret/internal/vault"
 )
@@ -25,30 +24,26 @@ const (
 	// vaultSecretParts is the number of parts when splitting vault:secret
 	vaultSecretParts = 2
 
+	// initialBufferSize is the starting size for secret read buffers (4KB)
+	initialBufferSize = 4 * 1024
 	// maxSecretSize is the maximum allowed size of a secret (100MB)
 	maxSecretSize = 100 * 1024 * 1024
-
-	// lockedMemoryReservePages is the locked memory, in pages, that `secret
-	// add`, `secret import` and `secret generate secret` may need besides
-	// the value they store, for keys and the read buffer: memguard locks
-	// whole pages, at least one for each buffer. `secret add` was measured
-	// to need 7 with both SB_SECRET_MNEMONIC and SB_UNLOCK_PASSPHRASE set,
-	// and 6 with one; the eighth is to spare.
-	lockedMemoryReservePages = 8
-	// kibibyte is the unit of the locked-memory limit in `ulimit -l`.
-	kibibyte = 1024
 )
 
 // Sentinel errors for secret operations
 var (
-	errSecretTooLarge    = errors.New("secret too large: exceeds 100MB limit")
-	errLockedMemoryLimit = errors.New(
-		"secret too large for the locked-memory limit")
+	errSecretTooLarge              = errors.New("secret too large: exceeds 100MB limit")
 	errCrossVaultSourceUnqualified = errors.New(
 		"source must specify vault (e.g., vault:secret) for cross-vault move")
 	errMoveOntoItself = errors.New("cannot be moved onto itself")
 )
 
+// bufferInfo tracks a protected buffer and the number of bytes used in it
+type bufferInfo struct {
+	buffer *memguard.LockedBuffer
+	used   int
+}
+
 // ParseVaultSecretRef parses a "vault:secret" or just "secret" reference
 // Returns (vaultName, secretName, isQualified)
 // If no vault is specified, returns empty vaultName and isQualified=false
@@ -80,12 +75,6 @@ func newAddCmd() *cobra.Command {
 
 			cli.cmd = cmd // Set the command for stdin access
 
-			// Before readSecretEnv, which makes the first locked allocation
-			err = checkLockedMemoryLimit(0)
-			if err != nil {
-				return err
-			}
-
 			destroySecrets := cli.readSecretEnv()
 			defer destroySecrets()
 
@@ -185,12 +174,6 @@ func newImportCmd() *cobra.Command {
 				return fmt.Errorf("failed to initialize CLI: %w", err)
 			}
 
-			// Before readSecretEnv, which makes the first locked allocation
-			err = checkLockedMemoryLimit(0)
-			if err != nil {
-				return err
-			}
-
 			destroySecrets := cli.readSecretEnv()
 			defer destroySecrets()
 
@@ -287,105 +270,88 @@ The source secret is deleted after successful copy.`,
 	return cmd
 }
 
-// checkLockedMemoryLimit returns an error naming the locked-memory limit
-// (`ulimit -l`) when a value of size bytes does not fit in it with
-// lockedMemoryReservePages. memguard, which holds secret values in locked
-// memory, crashes or hangs the process when it cannot lock more, so a
-// command calls this with size 0 before its first locked allocation, and
-// again with the value's size before a buffer for the value is allocated.
-func checkLockedMemoryLimit(size int) error {
-	var limit unix.Rlimit
+// updateBufferSize updates the buffer size based on usage pattern
+func updateBufferSize(currentSize int, sameSize *int) int {
+	const (
+		doubleAfterBuffers = 2
+		growthFactor       = 2
+	)
 
-	err := unix.Getrlimit(unix.RLIMIT_MEMLOCK, &limit)
-	if err != nil {
-		return fmt.Errorf("failed to read the locked-memory limit: %w", err)
+	*sameSize++
+	if *sameSize >= doubleAfterBuffers {
+		*sameSize = 0
+
+		return currentSize * growthFactor
 	}
 
-	// memguard locks whole pages, so the value is counted in whole pages.
-	pageSize := os.Getpagesize()
-	valuePages := (size + pageSize - 1) / pageSize
-
-	//nolint:gosec // G115: a page count and a page size are never negative
-	needed := uint64((valuePages + lockedMemoryReservePages) * pageSize)
-	if needed > limit.Cur {
-		return fmt.Errorf("%w of %d KiB (ulimit -l)",
-			errLockedMemoryLimit, limit.Cur/kibibyte)
-	}
-
-	return nil
+	return currentSize
 }
 
-// readSecretFromReader reads all of reader into a new locked buffer, which
-// the caller must destroy, leaving out one trailing newline when trimNewline
-// is set. It reads a page at a time into a memguard.Stream, which keeps what
-// it is given encrypted, so that the value is in locked memory only once, in
-// the buffer returned, and both size checks come before that buffer is
-// allocated. It takes the value back out with Stream.Next, not Stream.Read:
-// Read puts back the part of a piece that does not fit, and memguard hangs
-// when it runs out of locked memory doing that.
-func readSecretFromReader(
-	reader io.Reader, trimNewline bool,
-) (*memguard.LockedBuffer, error) {
-	stream := memguard.NewStream()
+// destroyBuffers destroys every buffer in the list
+func destroyBuffers(buffers []bufferInfo) {
+	for _, b := range buffers {
+		b.buffer.Destroy()
+	}
+}
 
-	chunk := memguard.NewBuffer(os.Getpagesize())
-	defer chunk.Destroy()
+// readSecretFromReader reads all data from reader into protected buffers,
+// enforcing the maximum secret size. On failure the accumulated buffers
+// are destroyed; on success the caller must destroy them.
+func readSecretFromReader(reader io.Reader) ([]bufferInfo, int, error) {
+	var buffers []bufferInfo
 
-	size := 0
-	endsWithNewline := false
+	totalSize := 0
+	currentBufferSize := initialBufferSize
+	sameSize := 0
 
 	for {
-		n, err := io.ReadFull(reader, chunk.Bytes())
-		if n > 0 {
-			size += n
-			if size > maxSecretSize {
-				return nil, errSecretTooLarge
+		// Create a new buffer
+		buffer := memguard.NewBuffer(currentBufferSize)
+		n, err := io.ReadFull(reader, buffer.Bytes())
+
+		if n == 0 {
+			// No data read, destroy the unused buffer
+			buffer.Destroy()
+		} else {
+			buffers = append(buffers, bufferInfo{buffer: buffer, used: n})
+			totalSize += n
+
+			if totalSize > maxSecretSize {
+				destroyBuffers(buffers)
+
+				return nil, 0, errSecretTooLarge
 			}
 
-			endsWithNewline = chunk.Bytes()[n-1] == '\n'
-
-			// Write encrypts the bytes and wipes them from chunk.
-			_, _ = stream.Write(chunk.Bytes()[:n])
+			// If we filled the buffer, consider growing for next iteration
+			if n == currentBufferSize {
+				currentBufferSize = updateBufferSize(currentBufferSize, &sameSize)
+			}
 		}
 
-		if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) {
+		if err == io.EOF || errors.Is(err, io.ErrUnexpectedEOF) {
 			break
-		}
+		} else if err != nil {
+			destroyBuffers(buffers)
 
-		if err != nil {
-			return nil, err
+			return nil, 0, err
 		}
 	}
 
-	// Freed before the value's buffer is allocated, so that the two are
-	// never locked at once.
-	chunk.Destroy()
+	return buffers, totalSize, nil
+}
 
-	if trimNewline && endsWithNewline {
-		size--
+// combineBuffers copies the used portions of buffers into a single
+// protected buffer of totalSize bytes
+func combineBuffers(buffers []bufferInfo, totalSize int) *memguard.LockedBuffer {
+	valueBuffer := memguard.NewBuffer(totalSize)
+
+	offset := 0
+	for _, b := range buffers {
+		copy(valueBuffer.Bytes()[offset:], b.buffer.Bytes()[:b.used])
+		offset += b.used
 	}
 
-	err := checkLockedMemoryLimit(size)
-	if err != nil {
-		return nil, err
-	}
-
-	value := memguard.NewBuffer(size)
-
-	// copy stops at the value's size, so a trimmed newline is not copied.
-	for offset := 0; offset < size; {
-		decrypted, err := stream.Next()
-		if err != nil {
-			value.Destroy()
-
-			return nil, fmt.Errorf("failed to decrypt the value read: %w", err)
-		}
-
-		offset += copy(value.Bytes()[offset:], decrypted.Bytes())
-		decrypted.Destroy()
-	}
-
-	return value, nil
+	return valueBuffer
 }
 
 // AddSecret adds a secret to the current vault
@@ -404,110 +370,58 @@ func (cli *Instance) AddSecret(secretName string, force bool) error {
 
 	secret.Debug("Got current vault", "vault_name", vlt.GetName())
 
-	// Read the secret value, less a trailing newline, into locked memory
-	secret.Debug("Reading secret value from stdin into a protected buffer")
+	// Read secret value directly into protected buffers
+	secret.Debug("Reading secret value from stdin into protected buffers")
 
-	valueBuffer, err := readSecretFromReader(cli.cmd.InOrStdin(), true)
+	buffers, totalSize, err := readSecretFromReader(cli.cmd.InOrStdin())
 	if err != nil {
-		if errors.Is(err, errSecretTooLarge) || errors.Is(err, errLockedMemoryLimit) {
+		if errors.Is(err, errSecretTooLarge) {
 			return err
 		}
 
 		return fmt.Errorf("failed to read secret value: %w", err)
 	}
-	defer valueBuffer.Destroy()
+	defer destroyBuffers(buffers)
 
-	secret.Debug("Read secret value from stdin", "value_length", valueBuffer.Size())
+	// Check for trailing newline in the last buffer
+	if len(buffers) > 0 && totalSize > 0 {
+		lastBuffer := &buffers[len(buffers)-1]
+		if lastBuffer.buffer.Bytes()[lastBuffer.used-1] == '\n' {
+			lastBuffer.used--
+			totalSize--
+		}
+	}
+
+	secret.Debug("Read secret value from stdin",
+		"value_length", totalSize, "buffers", len(buffers))
+
+	// Combine all buffers into a single protected buffer
+	valueBuffer := combineBuffers(buffers, totalSize)
+	defer valueBuffer.Destroy()
 
 	// 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.
-	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
+		return err
 	}
 	defer release()
 
-	replacing, err := replacesVersion(vlt, name, force)
+	// 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)
 	if err != nil {
-		return false, err
+		secret.Debug("vault.AddSecret failed", "error", err)
+
+		return err
 	}
 
-	if replacing && vlt.Locked() {
-		return true, nil
-	}
+	secret.Debug("vault.AddSecret completed successfully")
 
-	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
+	return nil
 }
 
 // GetSecret retrieves and prints the current version of a secret
@@ -529,10 +443,7 @@ func (cli *Instance) GetSecret(cmd *cobra.Command, secretName string) error {
 	defer value.Destroy()
 
 	// Write the value straight from locked memory, with no trailing newline
-	_, err = cmd.OutOrStdout().Write(value.Bytes())
-	if err != nil {
-		return fmt.Errorf("failed to write secret value: %w", err)
-	}
+	_, _ = cmd.OutOrStdout().Write(value.Bytes())
 
 	return nil
 }
@@ -567,10 +478,7 @@ func (cli *Instance) GetSecretWithVersion(
 	secret.Debug("Got secret value", "valueLength", value.Size())
 
 	// Write the value straight from locked memory, with no trailing newline
-	_, err = cmd.OutOrStdout().Write(value.Bytes())
-	if err != nil {
-		return fmt.Errorf("failed to write secret value: %w", err)
-	}
+	_, _ = cmd.OutOrStdout().Write(value.Bytes())
 
 	secret.Debug("Printed value to stdout")
 
@@ -633,12 +541,8 @@ func printSecretsJSON(
 			"name": secretName,
 		}
 
-		// A secret whose times cannot be read is listed without them
+		// Try to get metadata using GetSecretObject
 		secretObj, err := vlt.GetSecretObject(secretName)
-		if err == nil {
-			err = secretObj.LoadMetadata()
-		}
-
 		if err == nil {
 			metadata := secretObj.GetMetadata()
 			secretInfo["created_at"] = metadata.CreatedAt
@@ -714,10 +618,6 @@ func printSecretsTable(
 		lastUpdated := "unknown"
 
 		secretObj, err := vlt.GetSecretObject(secretName)
-		if err == nil {
-			err = secretObj.LoadMetadata()
-		}
-
 		if err == nil {
 			metadata := secretObj.GetMetadata()
 			lastUpdated = metadata.UpdatedAt.Format("2006-01-02 15:04")
@@ -753,7 +653,7 @@ func (cli *Instance) ImportSecret(
 
 	vlt.Mnemonic, vlt.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
 
-	// Read secret value from the source file into a protected buffer
+	// Read secret value from the source file into protected buffers
 	file, err := cli.fs.Open(sourceFile)
 	if err != nil {
 		return fmt.Errorf("failed to open file %s: %w", sourceFile, err)
@@ -765,15 +665,26 @@ func (cli *Instance) ImportSecret(
 		}
 	}()
 
-	valueBuffer, err := readSecretFromReader(file, false)
+	buffers, totalSize, err := readSecretFromReader(file)
 	if err != nil {
 		return fmt.Errorf("failed to read secret from file %s: %w", sourceFile, err)
 	}
+	defer destroyBuffers(buffers)
+
+	// Combine all buffers into a single protected buffer
+	valueBuffer := combineBuffers(buffers, totalSize)
 	defer valueBuffer.Destroy()
 
 	// Locked only now that the file has been read, as in AddSecret: the
 	// file may be a pipe written by another secret command.
-	err = cli.storeSecret(vlt, secretName, valueBuffer, force)
+	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)
 	if err != nil {
 		return err
 	}
@@ -875,12 +786,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 +903,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 +914,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 +1000,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 +1067,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 +1080,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

@@ -3,23 +3,17 @@ package cli
 
 import (
 	"bytes"
-	"context"
 	"crypto/rand"
 	"fmt"
 	"io"
-	"os"
-	"os/exec"
 	"path/filepath"
-	"strconv"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/secret/internal/secret"
 	"sneak.berlin/go/secret/internal/vault"
 	"sneak.berlin/go/secret/pkg/agehd"
 )
@@ -36,11 +30,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)
@@ -208,25 +208,23 @@ func TestImportSecretVariousSizes(t *testing.T) {
 	}
 }
 
-// TestAddSecretAroundPageSize tests that secret add, which reads its input
-// one memory page (usually 4 KiB) at a time, stores values just under, at and
-// just over one and two pages unchanged.
+// TestAddSecretBufferGrowth tests that our buffer growth strategy works correctly
 //
 //nolint:paralleltest // in parallel, size tests could exceed the memlock limit
-func TestAddSecretAroundPageSize(t *testing.T) {
-	// Sizes around one and two pages of 4 KiB, then larger ones up to 1 MiB
+func TestAddSecretBufferGrowth(t *testing.T) {
+	// Test various sizes that should trigger buffer growth
 	sizes := []int{
 		1,       // Single byte
 		100,     // Small
-		4095,    // Just under 4KB
+		4095,    // Just under initial 4KB
 		4096,    // Exactly 4KB
 		4097,    // Just over 4KB
-		8191,    // Just under 8KB
+		8191,    // Just under 8KB (first double)
 		8192,    // Exactly 8KB
 		8193,    // Just over 8KB
-		12288,   // 12KB
+		12288,   // 12KB (should trigger second double)
 		16384,   // 16KB
-		32768,   // 32KB
+		32768,   // 32KB (after more doublings)
 		65536,   // 64KB
 		131072,  // 128KB
 		524288,  // 512KB
@@ -317,144 +315,6 @@ func TestAddSecretStreamingBehavior(t *testing.T) {
 		"Retrieved secret should match original")
 }
 
-// newBinaryTestStateDir returns a new state directory holding a vault made
-// from the test mnemonic, for the secret binary to use.
-func newBinaryTestStateDir(t *testing.T) string {
-	t.Helper()
-
-	stateDir := t.TempDir()
-	_, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default",
-		testMnemonicBuffer(t), nil)
-	require.NoError(t, err)
-
-	return stateDir
-}
-
-// runAtLockedMemoryLimit runs the secret binary with args, stdin, the state
-// directory stateDir and the test mnemonic, its locked-memory limit (ulimit
-// -l) lowered to limitKiB, and returns what it printed.
-func runAtLockedMemoryLimit(
-	t *testing.T, stateDir string, limitKiB int, stdin io.Reader,
-	args ...string,
-) (string, error) {
-	t.Helper()
-
-	const commandWait = 10 * time.Second
-
-	secretPath, err := filepath.Abs("../../secret")
-	require.NoError(t, err)
-
-	ctx, cancel := context.WithTimeout(t.Context(), commandWait)
-	defer cancel()
-
-	// The shell lowers the limit for itself and the binary only.
-	script := fmt.Sprintf(`ulimit -l %d && exec "$0" "$@"`, limitKiB)
-
-	//nolint:gosec // G204: runs the freshly built secret binary
-	cmd := exec.CommandContext(ctx, "sh",
-		append([]string{"-c", script, secretPath}, args...)...)
-	cmd.Env = []string{
-		secret.EnvStateDir + "=" + stateDir,
-		secret.EnvMnemonic + "=" + testMnemonic,
-	}
-	cmd.Stdin = stdin
-
-	output, err := cmd.CombinedOutput()
-
-	return string(output), err
-}
-
-// requireRefusedAtLockedMemoryLimit runs the secret binary as
-// runAtLockedMemoryLimit does, and requires it to exit with status 1 and
-// print wantError, with no panic.
-func requireRefusedAtLockedMemoryLimit(
-	t *testing.T, stateDir string, limitKiB int, wantError string,
-	stdin io.Reader, args ...string,
-) {
-	t.Helper()
-
-	output, err := runAtLockedMemoryLimit(t, stateDir, limitKiB, stdin, args...)
-
-	var exitErr *exec.ExitError
-
-	require.ErrorAs(t, err, &exitErr, "%v: %s", args, output)
-	assert.Equal(t, 1, exitErr.ExitCode(), "%v: %s", args, output)
-	assert.Contains(t, output, wantError, args)
-	assert.NotContains(t, output, "panic", args)
-}
-
-// TestSecretsAtLockedMemoryLimit runs the secret binary with the
-// locked-memory limit (ulimit -l) lowered to 1 MiB, where memguard would crash
-// the process with a panic instead of returning an error. add must store the
-// largest value that fits, which takes holding it in locked memory only once.
-// add, import and generate secret must refuse a value one byte larger with
-// exit status 1 and an error saying why.
-func TestSecretsAtLockedMemoryLimit(t *testing.T) {
-	t.Parallel()
-
-	const limitKiB = 1024
-
-	fits := limitKiB*kibibyte - lockedMemoryReservePages*os.Getpagesize()
-	limitError := fmt.Sprintf("locked-memory limit of %d KiB", limitKiB)
-	stateDir := newBinaryTestStateDir(t)
-
-	tooBigFile := filepath.Join(t.TempDir(), "too-big")
-	require.NoError(t, os.WriteFile(tooBigFile, make([]byte, fits+1), 0o600))
-
-	output, err := runAtLockedMemoryLimit(t, stateDir, limitKiB,
-		bytes.NewReader(make([]byte, fits)), "add", "fits")
-	require.NoError(t, err, "the largest value that fits: %s", output)
-
-	requireRefusedAtLockedMemoryLimit(t, stateDir, limitKiB, limitError,
-		bytes.NewReader(make([]byte, fits+1)), "add", "too-big")
-	requireRefusedAtLockedMemoryLimit(t, stateDir, limitKiB, limitError,
-		nil, "import", "too-big", "--source", tooBigFile)
-	requireRefusedAtLockedMemoryLimit(t, stateDir, limitKiB, limitError,
-		nil, "generate", "secret", "too-big", "--length", strconv.Itoa(fits+1))
-}
-
-// TestSecretsUnderSmallLockedMemoryLimits runs add, import and generate
-// secret with a short value under two small locked-memory limits (ulimit
-// -l). Under 20 KiB, too little for the buffers they need besides the value,
-// each must refuse it with exit status 1 and an error naming the limit, where
-// memguard would panic or hang. Under 64 KiB, the default of older Linux
-// kernels, each must store it.
-func TestSecretsUnderSmallLockedMemoryLimits(t *testing.T) {
-	t.Parallel()
-
-	const (
-		tooSmallLimitKiB   = 20
-		oldDefaultLimitKiB = 64
-		shortValue         = "hunter2"
-	)
-
-	tooSmallLimitError := fmt.Sprintf("locked-memory limit of %d KiB",
-		tooSmallLimitKiB)
-	stateDir := newBinaryTestStateDir(t)
-
-	shortFile := filepath.Join(t.TempDir(), "short")
-	require.NoError(t, os.WriteFile(shortFile, []byte(shortValue), 0o600))
-
-	requireRefusedAtLockedMemoryLimit(t, stateDir, tooSmallLimitKiB,
-		tooSmallLimitError, strings.NewReader(shortValue), "add", "added")
-	requireRefusedAtLockedMemoryLimit(t, stateDir, tooSmallLimitKiB,
-		tooSmallLimitError, nil, "import", "imported", "--source", shortFile)
-	requireRefusedAtLockedMemoryLimit(t, stateDir, tooSmallLimitKiB,
-		tooSmallLimitError, nil, "generate", "secret", "generated")
-
-	output, err := runAtLockedMemoryLimit(t, stateDir, oldDefaultLimitKiB,
-		strings.NewReader(shortValue), "add", "added")
-	require.NoError(t, err, "add: %s", output)
-
-	output, err = runAtLockedMemoryLimit(t, stateDir, oldDefaultLimitKiB,
-		nil, "import", "imported", "--source", shortFile)
-	require.NoError(t, err, "import: %s", output)
-
-	output, err = runAtLockedMemoryLimit(t, stateDir, oldDefaultLimitKiB,
-		nil, "generate", "secret", "generated")
-	require.NoError(t, err, "generate secret: %s", output)
-}
-
 // slowReader simulates a reader that returns data in small chunks
 type slowReader struct {
 	data      []byte

internal/cli/trailing_newline_test.go

@@ -1,47 +0,0 @@
-//nolint:testpackage // white-box test of unexported internals
-package cli
-
-import (
-	"bytes"
-	"testing"
-
-	"github.com/spf13/afero"
-	"github.com/spf13/cobra"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-// TestAddDropsOneTrailingNewlineImportKeepsIt pins the rule the README
-// gives: `secret add` drops one trailing newline from stdin, and
-// `secret import` stores the file's bytes unchanged.
-func TestAddDropsOneTrailingNewlineImportKeepsIt(t *testing.T) {
-	t.Parallel()
-
-	fs, vlt := newSizeTestVault(t)
-	value := []byte("a\n\n")
-
-	cmd := &cobra.Command{}
-	cmd.SetIn(bytes.NewReader(value))
-
-	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
-	cli.cmd = cmd
-
-	require.NoError(t, cli.AddSecret("added", false))
-
-	sourceFile := "/test/value"
-	require.NoError(t, afero.WriteFile(fs, sourceFile, value, 0o600))
-	require.NoError(t, cli.ImportSecret(cmd, "imported", sourceFile, false))
-
-	added, err := vlt.GetSecret("added")
-	require.NoError(t, err)
-
-	defer added.Destroy()
-
-	imported, err := vlt.GetSecret("imported")
-	require.NoError(t, err)
-
-	defer imported.Destroy()
-
-	assert.Equal(t, []byte("a\n"), added.Bytes())
-	assert.Equal(t, []byte("a\n\n"), imported.Bytes())
-}

internal/cli/unknown_subcommand_test.go

@@ -1,58 +0,0 @@
-package cli_test
-
-import (
-	"fmt"
-	"os/exec"
-	"strings"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/secret/internal/secret"
-)
-
-// TestUnknownSubcommandFails is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/183: given a subcommand it does not
-// have, `secret vault`, `secret unlocker`, `secret version` and `secret
-// generate` printed their help and exited 0, so a script could not tell a
-// mistyped command from a successful one. Each runs here as the built binary
-// and must exit 1 naming the subcommand, as an unknown command does at the
-// top level, and must still print its help and exit 0 when given nothing.
-func TestUnknownSubcommandFails(t *testing.T) {
-	t.Parallel()
-
-	stateDir := t.TempDir()
-
-	// run runs the built binary with args and returns what it printed and
-	// its exit code
-	run := func(args ...string) (string, int) {
-		//nolint:gosec // G204: test executes the freshly built secret binary
-		cmd := exec.CommandContext(t.Context(), secretBinaryPath(t), args...)
-		cmd.Env = []string{secret.EnvStateDir + "=" + stateDir}
-
-		output, err := cmd.CombinedOutput()
-		if err != nil {
-			var exitErr *exec.ExitError
-			require.ErrorAs(t, err, &exitErr, "secret %v did not run", args)
-		}
-
-		return string(output), cmd.ProcessState.ExitCode()
-	}
-
-	for _, call := range []string{
-		"vault improt", "unlocker foo", "version foo", "generate foo",
-	} {
-		parent, unknown, _ := strings.Cut(call, " ")
-
-		output, exitCode := run(parent, unknown)
-		assert.Equal(t, 1, exitCode, "secret %s should fail:\n%s", call, output)
-		assert.Contains(t, output, fmt.Sprintf(
-			"unknown command %q for \"secret %s\"", unknown, parent))
-
-		output, exitCode = run(parent)
-		assert.Equal(t, 0, exitCode,
-			"secret %s should succeed:\n%s", parent, output)
-		assert.Contains(t, output, usageHeading,
-			"secret %s should print its help", parent)
-	}
-}

internal/cli/unlockers.go

@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"log"
 	"maps"
-	"net/url"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -15,7 +14,6 @@ import (
 	"strings"
 	"time"
 
-	"filippo.io/age"
 	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
 	"sneak.berlin/go/secret/internal/secret"
@@ -40,10 +38,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
@@ -72,13 +66,12 @@ func getDefaultGPGKey() (string, error) {
 
 	output, err := cmd.Output()
 	if err == nil {
-		defaultKey, parseErr := parseDefaultGPGKey(string(output))
-		if parseErr != nil {
-			return "", parseErr
-		}
-
-		if defaultKey != "" {
-			return defaultKey, nil
+		for line := range strings.SplitSeq(string(output), "\n") {
+			fields := strings.Split(line, ":")
+			if len(fields) > 9 && fields[0] == "default-key" && fields[9] != "" {
+				// The default key is in field 10 (index 9)
+				return fields[9], nil
+			}
 		}
 	}
 
@@ -105,38 +98,11 @@ func getDefaultGPGKey() (string, error) {
 	return "", errNoGPGSecretKeys
 }
 
-// parseDefaultGPGKey returns the default-key value in the output of
-// `gpgconf --list-options gpg`, or "" when gpg.conf sets none. gpgconf(1)
-// prints a string value with a leading `"` and percent-escaped.
-func parseDefaultGPGKey(gpgconfOutput string) (string, error) {
-	for line := range strings.SplitSeq(gpgconfOutput, "\n") {
-		fields := strings.Split(line, ":")
-		if len(fields) > 9 && fields[0] == "default-key" && fields[9] != "" {
-			// The default key is in field 10 (index 9)
-			keyID, err := url.PathUnescape(strings.TrimPrefix(fields[9], `"`))
-			if err != nil {
-				return "", fmt.Errorf("failed to decode gpgconf default-key %s: %w",
-					fields[9], err)
-			}
-
-			return keyID, nil
-		}
-	}
-
-	return "", nil
-}
-
 func newUnlockerCmd() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "unlocker",
 		Short: "Manage unlockers",
 		Long:  `Create, list, and remove unlockers for the current vault.`,
-		// Cobra checks Args only on a command that has a RunE; without
-		// one, an unknown subcommand prints this help and exits 0.
-		Args: cobra.NoArgs,
-		RunE: func(cmd *cobra.Command, _ []string) error {
-			return cmd.Help()
-		},
 	}
 
 	cmd.AddCommand(newUnlockerListCmd())
@@ -325,8 +291,7 @@ func (cli *Instance) UnlockersList(jsonOutput bool) error {
 		return err
 	}
 
-	// Not nil, so that a vault with no unlockers prints [] in JSON, not null
-	unlockers := make([]UnlockerInfo, 0, len(unlockerMetadata))
+	var unlockers []UnlockerInfo
 
 	for _, unlockerID := range slices.Sorted(maps.Keys(unlockerMetadata)) {
 		metadata := unlockerMetadata[unlockerID]
@@ -409,6 +374,12 @@ 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)
@@ -422,64 +393,6 @@ 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) {
@@ -499,31 +412,23 @@ 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
-	// set, and the variable unlocks the vault. With no terminal to ask on,
-	// the new unlocker takes the passphrase in the variable.
-	fromVariable := false
-
-	passphraseBuffer, err := readSecurePassphrase("Enter passphrase for unlocker: ")
-
-	switch {
-	case errors.Is(err, secret.ErrNoTerminal) && cli.UnlockPassphrase != nil:
-		passphraseBuffer = cli.UnlockPassphrase
-		fromVariable = true
-	case err != nil:
-		return err
-	default:
+	// The new unlocker gets the passphrase from the environment, which also
+	// unlocks the current passphrase unlocker, else the one entered here
+	passphraseBuffer := cli.UnlockPassphrase
+	if passphraseBuffer == nil {
+		// Use secure passphrase input with confirmation
+		passphraseBuffer, err = readSecurePassphrase("Enter passphrase for unlocker: ")
+		if err != nil {
+			return err
+		}
 		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
@@ -531,12 +436,6 @@ func (cli *Instance) addPassphraseUnlocker(cmd *cobra.Command) error {
 
 	cmd.Printf("Created passphrase unlocker: %s\n", passphraseUnlocker.GetID())
 
-	if fromVariable {
-		cmd.Printf("The new unlocker takes its passphrase from %s, since "+
-			"there is no terminal to ask for a new one on\n",
-			secret.EnvUnlockPassphrase)
-	}
-
 	// CreatePassphraseUnlocker has already made it the current unlocker
 	cmd.Printf("Automatically selected as current unlocker\n")
 
@@ -562,13 +461,12 @@ 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)
@@ -576,26 +474,21 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error {
 		return fmt.Errorf("failed to resolve GPG key fingerprint: %w", err)
 	}
 
-	// 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)
+	// Check if this GPG key is already added
+	exists, err := cli.pgpUnlockerExists(vlt, fingerprint)
 	if err != nil {
-		return err
+		return fmt.Errorf(
+			"could not check whether GPG key %s is already an unlocker: %w",
+			gpgKeyID, err,
+		)
 	}
 
-	release, err := cli.unlockThenLock(vlt)
-	if err != nil {
-		return err
-	}
-	defer release()
-
-	err = cli.checkPGPKeyNotAdded(vlt, gpgKeyID, fingerprint)
-	if err != nil {
-		return err
+	if exists {
+		return fmt.Errorf("GPG key %s %w", gpgKeyID, errGPGKeyAlreadyUnlocker)
 	}
 
-	pgpUnlocker, err := secret.CreatePGPUnlocker(vlt, gpgKeyID, fingerprint)
+	pgpUnlocker, err := secret.CreatePGPUnlocker(cli.fs, cli.stateDir,
+		gpgKeyID, fingerprint, cli.Mnemonic, cli.UnlockPassphrase)
 	if err != nil {
 		return err
 	}
@@ -770,27 +663,6 @@ 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/unlockers_add_test.go

@@ -2,8 +2,6 @@
 package cli
 
 import (
-	"bytes"
-	"os"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -85,77 +83,6 @@ func TestAddPGPUnlocker(t *testing.T) {
 	}
 }
 
-// TestAddPGPUnlockerDefaultKey is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/141: with default-key set in
-// gpg.conf and no --keyid, `secret unlocker add pgp` uses that key.
-// gpg.conf names the key by its user ID, and the printed key ID must be that
-// user ID. The first secret key, which is used when gpg.conf sets no
-// default-key, prints as a hex key ID.
-func TestAddPGPUnlockerDefaultKey(t *testing.T) {
-	newTestGPGKey(t)
-	// The key comes from neither --keyid nor the environment.
-	t.Setenv(secret.EnvGPGKeyID, "")
-
-	gpgConf := filepath.Join(os.Getenv("GNUPGHOME"), "gpg.conf")
-	//nolint:gosec // G703: GNUPGHOME is the directory newTestGPGKey made
-	require.NoError(t, os.WriteFile(gpgConf,
-		[]byte("default-key "+unreadableTestGPGUserID+"\n"), 0o600))
-
-	fs := afero.NewMemMapFs()
-	_, err := vault.CreateVault(fs, listTestStateDir, listTestVaultName,
-		testMnemonicBuffer(t), nil)
-	require.NoError(t, err)
-
-	instance, cmd := newTestInstance(fs)
-	instance.Mnemonic = testMnemonicBuffer(t)
-
-	var output bytes.Buffer
-	cmd.SetOut(&output)
-
-	require.NoError(t, instance.UnlockersAdd(unlockerTypePGP, cmd))
-	assert.Contains(t, output.String(),
-		"Using default GPG key: "+unreadableTestGPGUserID+"\n")
-}
-
-// TestParseDefaultGPGKey is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/141: gpgconf prints the
-// default-key value with a leading `"` and percent-escaped, and the key ID
-// is that value decoded.
-func TestParseDefaultGPGKey(t *testing.T) {
-	t.Parallel()
-
-	// gpgconfLine is the default-key line of `gpgconf --list-options gpg`
-	// up to the value, its last field.
-	const gpgconfLine = "default-key:0:0:use NAME as default secret key:" +
-		"1:1:NAME:::"
-
-	tests := []struct {
-		name  string
-		value string
-		want  string
-	}{
-		{
-			"fingerprint", `"56163D7F3FAFF709608855BEC1A360216D007F41`,
-			"56163D7F3FAFF709608855BEC1A360216D007F41",
-		},
-		{
-			"user ID with a comma", `"Doe%2c Alice <alice@example.com>`,
-			"Doe, Alice <alice@example.com>",
-		},
-		{"no default-key in gpg.conf", "", ""},
-	}
-
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			t.Parallel()
-
-			keyID, err := parseDefaultGPGKey(gpgconfLine + test.value + "\n")
-			require.NoError(t, err)
-			assert.Equal(t, test.want, keyID)
-		})
-	}
-}
-
 // TestAddPGPUnlockerUnknownKey asserts that adding a PGP unlocker for a key
 // the keyring does not hold fails at looking up the key's fingerprint and
 // leaves no new unlocker directory. The error must come from the lookup: a

internal/cli/unlockers_list_test.go

@@ -1,14 +1,13 @@
 // Unlocker List Tests
 //
-// Tests for `secret unlocker list`:
+// Tests for `secret unlocker list` behavior when an unlocker's metadata
+// cannot be read or used:
 //
 //   - TestUnlockersListToleratesCorruptMetadata: one unlocker's corrupt
 //     metadata does not stop the others from being listed.
 //   - TestUnlockersListSkipsUnreadableMetadata: an unlocker whose metadata
 //     file cannot be checked for or read is left out, and the other is
 //     still listed.
-//   - TestUnlockersListEmptyJSON: a vault with no unlockers lists them as
-//     an empty JSON list.
 
 //nolint:testpackage // white-box test of unexported internals
 package cli
@@ -322,36 +321,3 @@ func TestUnlockersListSkipsUnreadableMetadata(t *testing.T) {
 		})
 	}
 }
-
-// TestUnlockersListEmptyJSON is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/148: listing the unlockers of a
-// vault that has none, as after its last unlocker is removed, printed
-// "unlockers": null in JSON form instead of an empty list.
-func TestUnlockersListEmptyJSON(t *testing.T) {
-	t.Parallel()
-
-	fs := afero.NewMemMapFs()
-	vaultDir := filepath.Join(listTestStateDir, "vaults.d", listTestVaultName)
-
-	require.NoError(t, afero.WriteFile(
-		fs, filepath.Join(listTestStateDir, "currentvault"),
-		[]byte(listTestVaultName), listTestFilePerm,
-	))
-	require.NoError(t, fs.MkdirAll(
-		filepath.Join(vaultDir, listTestUnlockersDirName), listTestDirPerm,
-	))
-
-	var buf bytes.Buffer
-
-	cmd := &cobra.Command{}
-	cmd.SetOut(&buf)
-
-	instance := &Instance{fs: fs, stateDir: listTestStateDir, cmd: cmd}
-	require.NoError(t, instance.UnlockersList(true))
-
-	var decoded map[string]any
-
-	require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded))
-	assert.Equal(t, []any{}, decoded["unlockers"],
-		"no unlockers must be printed as [], not %s", buf.String())
-}

internal/cli/unreadable_dir_test.go

@@ -9,12 +9,11 @@
 // "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 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.
+// 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.
 
 //nolint:testpackage // white-box test of unexported internals
 package cli

internal/cli/vault.go

@@ -29,12 +29,6 @@ func newVaultCmd() *cobra.Command {
 		Use:   "vault",
 		Short: "Manage vaults",
 		Long:  `Create, list, and select vaults for organizing secrets.`,
-		// Cobra checks Args only on a command that has a RunE; without
-		// one, an unknown subcommand prints this help and exits 0.
-		Args: cobra.NoArgs,
-		RunE: func(cmd *cobra.Command, _ []string) error {
-			return cmd.Help()
-		},
 	}
 
 	cmd.AddCommand(newVaultListCmd())
@@ -222,17 +216,15 @@ func (cli *Instance) resolvePassphrase() (*memguard.LockedBuffer, func(), error)
 	return passphraseBuffer, passphraseBuffer.Destroy, nil
 }
 
-// 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.
+// CreateVault creates a new vault
 func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
 	secret.Debug("Creating new vault", "name", name, "state_dir", cli.stateDir)
 
-	// Refuse an invalid name or an existing vault before asking for anything
-	err := vault.CheckNewVaultName(cli.fs, cli.stateDir, name)
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
 	if err != nil {
 		return err
 	}
+	defer release()
 
 	mnemonic, cleanupMnemonic, err := cli.promptMnemonic()
 	if err != nil {
@@ -261,14 +253,7 @@ func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
 	}
 	defer cleanupPassphrase()
 
-	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.
+	// Create the vault with its passphrase unlocker
 	vlt, err := vault.CreateVault(cli.fs, cli.stateDir, name,
 		mnemonic, passphraseBuffer)
 	if err != nil {

internal/cli/vault_list_test.go

@@ -1,42 +0,0 @@
-package cli_test
-
-import (
-	"bytes"
-	"encoding/json"
-	"path/filepath"
-	"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"
-)
-
-// TestVaultListEmptyJSON is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/148: listing the vaults when
-// vaults.d holds none printed "vaults": null in JSON form instead of an
-// empty list.
-func TestVaultListEmptyJSON(t *testing.T) {
-	t.Parallel()
-
-	fs := afero.NewMemMapFs()
-	require.NoError(t, fs.MkdirAll(
-		filepath.Join(testStateDir, "vaults.d"), secret.DirPerms,
-	))
-
-	var buf bytes.Buffer
-
-	cmd := &cobra.Command{}
-	cmd.SetOut(&buf)
-
-	c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
-	require.NoError(t, c.ListVaults(cmd, true))
-
-	var decoded map[string]any
-
-	require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded))
-	assert.Equal(t, []any{}, decoded["vaults"],
-		"no vaults must be printed as [], not %s", buf.String())
-}

internal/cli/version.go

@@ -43,12 +43,6 @@ func VersionCommands(cli *Instance) *cobra.Command {
 		Short: "Manage secret versions",
 		Long: "Commands for managing secret versions including listing, " +
 			"promoting, and retrieving specific versions",
-		// Cobra checks Args only on a command that has a RunE; without
-		// one, an unknown subcommand prints this help and exits 0.
-		Args: cobra.NoArgs,
-		RunE: func(cmd *cobra.Command, _ []string) error {
-			return cmd.Help()
-		},
 	}
 
 	// List versions command
@@ -70,10 +64,8 @@ func VersionCommands(cli *Instance) *cobra.Command {
 	promoteCmd := &cobra.Command{
 		Use:   "promote <secret-name> <version>",
 		Short: "Promote a specific version to current",
-		Long: "Rewrites the secret's current file to name the specified " +
-			"version. The versions' own times are left unchanged; secret " +
-			"list then gives the time of the promote as the secret's last " +
-			"update.",
+		Long: "Updates the current symlink to point to the specified " +
+			"version without modifying timestamps",
 		Args: cobra.ExactArgs(2), //nolint:mnd // secret-name and version args
 		ValidArgsFunction: func(
 			cmd *cobra.Command, args []string, toComplete string,
@@ -235,13 +227,9 @@ func printVersionRow(
 	}
 
 	// Determine status
-	status := "inactive"
-
-	switch {
-	case version == currentVersion:
+	status := "expired"
+	if version == currentVersion {
 		status = "current"
-	case sv.Metadata.NotAfter != nil && sv.Metadata.NotAfter.Before(time.Now()):
-		status = "expired"
 	}
 
 	// Format timestamps
@@ -303,7 +291,7 @@ func (cli *Instance) PromoteVersion(
 			version, vault.ErrVersionNotFound, secretName)
 	}
 
-	// Rewrite the secret's current file to name the version
+	// Update the current symlink using the proper function
 	err = secret.SetCurrentVersion(cli.fs, secretDir, version)
 	if err != nil {
 		return fmt.Errorf("failed to update current version: %w", err)

internal/cli/version_test.go

@@ -5,14 +5,9 @@
 // - TestListVersionsCommand: Tests `secret version list` command output
 // - TestListVersionsNonExistentSecret: Tests error handling for missing secrets
 // - TestPromoteVersionCommand: Tests `secret version promote` command
-// - TestListVersionsStatusAfterPromote: Tests each version's status after
-//   promoting an older version
 // - TestPromoteNonExistentVersion: Tests error handling for invalid promotion
-// - TestPromoteHelpSaysCurrentFileIsRewritten: Tests `secret version promote
-//   --help` text
 // - TestGetSecretWithVersion: Tests `secret get --version` flag functionality
 // - TestGetSecretWritesBinaryValue: Tests `secret get` output of binary values
-// - TestGetSecretReturnsWriteError: Tests `secret get` when stdout fails
 // - TestVersionCommandStructure: Tests command structure and help text
 // - TestListVersionsEmptyOutput: Tests edge case with no versions
 //
@@ -25,7 +20,6 @@ package cli
 
 import (
 	"bytes"
-	"os"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -246,66 +240,6 @@ func TestPromoteVersionCommand(t *testing.T) {
 	assert.Equal(t, []byte("version-1"), promoted.Bytes())
 }
 
-func TestListVersionsStatusAfterPromote(t *testing.T) {
-	t.Parallel()
-
-	fs := afero.NewMemMapFs()
-	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
-	cli.Mnemonic = testMnemonicBuffer(t)
-
-	setupTestVault(t, fs)
-
-	vlt, err := vault.GetCurrentVault(fs, testStateDir)
-	require.NoError(t, err)
-
-	vlt.Mnemonic = cli.Mnemonic
-
-	addTestSecret(t, vlt, []byte("version-1"), false)
-	addTestSecret(t, vlt, []byte("version-2"), true)
-	addTestSecret(t, vlt, []byte("version-3"), true)
-
-	vaultDir, err := vlt.GetDirectory()
-	require.NoError(t, err)
-
-	// Newest first
-	versions, err := secret.ListVersions(fs,
-		filepath.Join(vaultDir, "secrets.d", "test%secret"))
-	require.NoError(t, err)
-	require.Len(t, versions, 3)
-
-	newest, middle, oldest := versions[0], versions[1], versions[2]
-
-	cmd := newRootCmd()
-
-	var buf bytes.Buffer
-	cmd.SetOut(&buf)
-	cmd.SetErr(&buf)
-
-	err = cli.PromoteVersion(cmd, "test/secret", oldest)
-	require.NoError(t, err)
-
-	buf.Reset()
-
-	err = cli.ListVersions(cmd, "test/secret")
-	require.NoError(t, err)
-
-	// STATUS is the fourth field of a row, since CREATED is a date and a time
-	statuses := map[string]string{}
-
-	for line := range strings.SplitSeq(buf.String(), "\n") {
-		fields := strings.Fields(line)
-		if len(fields) > 3 {
-			statuses[fields[0]] = fields[3]
-		}
-	}
-
-	assert.Equal(t, "current", statuses[oldest])
-	// Its NOT_AFTER was set when the newest version was added
-	assert.Equal(t, "expired", statuses[middle])
-	// It has no NOT_AFTER
-	assert.Equal(t, "inactive", statuses[newest])
-}
-
 func TestPromoteNonExistentVersion(t *testing.T) {
 	t.Parallel()
 
@@ -334,23 +268,6 @@ func TestPromoteNonExistentVersion(t *testing.T) {
 	require.ErrorIs(t, err, vault.ErrVersionNotFound)
 }
 
-// TestPromoteHelpSaysCurrentFileIsRewritten is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/153: `secret version promote
-// --help` said it updates a symlink, but promote rewrites the secret's
-// `current` file, which is a plain file.
-//
-//nolint:paralleltest // executes the CLI in-process and sets the environment
-func TestPromoteHelpSaysCurrentFileIsRewritten(t *testing.T) {
-	env := map[string]string{secret.EnvStateDir: t.TempDir()}
-
-	output, err := ExecuteCommandInProcess(
-		[]string{"version", "promote", "--help"}, "", env)
-	require.NoError(t, err)
-
-	assert.NotContains(t, output, "symlink")
-	assert.Contains(t, output, "current file")
-}
-
 func TestGetSecretWithVersion(t *testing.T) {
 	t.Parallel()
 
@@ -443,45 +360,6 @@ func TestGetSecretWritesBinaryValue(t *testing.T) {
 	assert.Equal(t, value, buf.Bytes())
 }
 
-// TestGetSecretReturnsWriteError checks that `secret get`, with and without
-// a version, returns the error of a failed write of the value.
-func TestGetSecretReturnsWriteError(t *testing.T) {
-	t.Parallel()
-
-	fs := afero.NewMemMapFs()
-	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
-	cli.Mnemonic = testMnemonicBuffer(t)
-
-	setupTestVault(t, fs)
-
-	vlt, err := vault.GetCurrentVault(fs, testStateDir)
-	require.NoError(t, err)
-
-	addTestSecret(t, vlt, []byte("value"), false)
-
-	vaultDir, err := vlt.GetDirectory()
-	require.NoError(t, err)
-
-	versions, err := secret.ListVersions(fs,
-		filepath.Join(vaultDir, "secrets.d", "test%secret"))
-	require.NoError(t, err)
-	require.Len(t, versions, 1)
-
-	// Stdout is a file closed before the value is written to it
-	out, err := os.Create(filepath.Join(t.TempDir(), "stdout"))
-	require.NoError(t, err)
-	require.NoError(t, out.Close())
-
-	cmd := newRootCmd()
-	cmd.SetOut(out)
-
-	err = cli.GetSecret(cmd, "test/secret")
-	require.ErrorIs(t, err, os.ErrClosed)
-
-	err = cli.GetSecretWithVersion(cmd, "test/secret", versions[0])
-	require.ErrorIs(t, err, os.ErrClosed)
-}
-
 //nolint:paralleltest // reads process environment to determine the state dir
 func TestVersionCommandStructure(t *testing.T) {
 	// Test that version commands are properly structured

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

@@ -15,13 +15,10 @@ import (
 
 var (
 	errNilPassphraseBuffer = errors.New("passphrase buffer is nil")
+	errNoTerminal          = errors.New("there is no terminal to ask on")
 	errNothingEntered      = errors.New("nothing was entered")
 )
 
-// ErrNoTerminal is wrapped in the error of ReadPassphrase and ReadMnemonic
-// when the process has no controlling terminal to ask on.
-var ErrNoTerminal = errors.New("there is no terminal to ask on")
-
 // ErrMnemonicNotRead is wrapped in every error of ReadMnemonic: there is no
 // terminal to read the mnemonic from, reading it failed, or it was empty.
 var ErrMnemonicNotRead = errors.New("failed to read mnemonic")
@@ -206,7 +203,7 @@ func readFromTerminal(
 
 		return nil, fmt.Errorf(
 			"%w: %w. Please set the %s environment variable or run interactively",
-			notRead, ErrNoTerminal, envVar)
+			notRead, errNoTerminal, envVar)
 	}
 	defer func() { _ = terminal.Close() }()
 

internal/secret/pgpunlock_test.go

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

internal/secret/pgpunlocker.go

@@ -192,45 +192,58 @@ func generatePGPUnlockerName() (string, error) {
 	return fmt.Sprintf("%s-pgp-%s", hostname, enrollmentTime), nil
 }
 
-// pgpUnlockerDir returns the directory in vault for a new PGP unlocker,
-// named after the host and the time.
-func pgpUnlockerDir(vault VaultInterface) (string, error) {
+// 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)
+	}
+
 	// Generate the unlocker name based on hostname and time
 	unlockerName, err := generatePGPUnlockerName()
 	if err != nil {
-		return "", fmt.Errorf("failed to generate unlocker name: %w", err)
+		return nil, "", fmt.Errorf("failed to generate unlocker name: %w", err)
 	}
 
 	vaultDir, err := vault.GetDirectory()
 	if err != nil {
-		return "", fmt.Errorf("failed to get vault directory: %w", err)
+		return nil, "", fmt.Errorf("failed to get vault directory: %w", err)
 	}
 
-	return filepath.Join(vaultDir, "unlockers.d", unlockerName), nil
+	return vault, filepath.Join(vaultDir, "unlockers.d", unlockerName), nil
 }
 
-// CreatePGPUnlocker creates a new PGP unlocker and stores it in vault.
+// CreatePGPUnlocker creates a new PGP unlocker and stores it in the 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 vault's
-// GetOrDeriveLongTermKey: the key it holds when it is unlocked, else the
-// key from its mnemonic or its current unlocker.
+// 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.
 func CreatePGPUnlocker(
-	vault VaultInterface, gpgKeyID, fingerprint string,
+	fs afero.Fs, stateDir, gpgKeyID, fingerprint string,
+	mnemonic, passphrase *memguard.LockedBuffer,
 ) (*PGPUnlocker, error) {
 	err := checkGPGAvailable()
 	if err != nil {
 		return nil, err
 	}
 
-	unlockerDir, err := pgpUnlockerDir(vault)
+	vault, unlockerDir, err := pgpUnlockerDir(fs, stateDir)
 	if err != nil {
 		return nil, err
 	}
 
-	fs := vault.GetFilesystem()
+	// The vault's GetOrDeriveLongTermKey, in step 2, uses both
+	vault.SetMnemonic(mnemonic)
+	vault.SetUnlockPassphrase(passphrase)
 
 	// Step 1: Generate a new age keypair for the PGP unlocker
 	ageIdentity, err := age.GenerateX25519Identity()
@@ -282,7 +295,7 @@ func CreatePGPUnlocker(
 func encryptPGPUnlockerKeys(
 	vault VaultInterface, ageIdentity *age.X25519Identity, gpgKeyID string,
 ) ([]byte, []byte, error) {
-	// The key vault holds, or else from its mnemonic or current unlocker
+	// From the mnemonic or the current unlocker, as for a passphrase unlocker
 	ltIdentity, err := vault.GetOrDeriveLongTermKey()
 	if err != nil {
 		return nil, nil, fmt.Errorf("failed to get long-term key: %w", err)
@@ -375,8 +388,7 @@ func ResolveGPGKeyFingerprint(keyID string) (string, error) {
 			return "", fmt.Errorf("%w: %s", ErrGPGKeyNotFound, keyID)
 		}
 
-		return "", fmt.Errorf(
-			"failed to resolve GPG key fingerprint: %w", withGPGStderr(err))
+		return "", fmt.Errorf("failed to resolve GPG key fingerprint: %w", err)
 	}
 
 	// Parse the output to extract the fingerprint
@@ -406,24 +418,6 @@ func checkGPGAvailable() error {
 	return nil
 }
 
-// withGPGStderr appends gpg's stderr, which says why gpg failed, to err
-// from cmd.Output. cmd.Output keeps that stderr in the *exec.ExitError.
-// err is returned unchanged when gpg wrote nothing to stderr.
-func withGPGStderr(err error) error {
-	var exitErr *exec.ExitError
-
-	if !errors.As(err, &exitErr) {
-		return err
-	}
-
-	stderr := strings.TrimSpace(string(exitErr.Stderr))
-	if stderr == "" {
-		return err
-	}
-
-	return fmt.Errorf("%w: %s", err, stderr)
-}
-
 // gpgEncryptDefault is the default implementation of GPG encryption
 func gpgEncryptDefault(data *memguard.LockedBuffer, keyID string) ([]byte, error) {
 	if data == nil {
@@ -443,7 +437,7 @@ func gpgEncryptDefault(data *memguard.LockedBuffer, keyID string) ([]byte, error
 
 	output, err := cmd.Output()
 	if err != nil {
-		return nil, fmt.Errorf("GPG encryption failed: %w", withGPGStderr(err))
+		return nil, fmt.Errorf("GPG encryption failed: %w", err)
 	}
 
 	return output, nil
@@ -456,7 +450,7 @@ func gpgDecryptDefault(encryptedData []byte) (*memguard.LockedBuffer, error) {
 
 	output, err := cmd.Output()
 	if err != nil {
-		return nil, fmt.Errorf("GPG decryption failed: %w", withGPGStderr(err))
+		return nil, fmt.Errorf("GPG decryption failed: %w", err)
 	}
 
 	// Create a secure buffer for the decrypted data

internal/secret/pgpunlocker_test.go

@@ -58,8 +58,7 @@ func TestCreatePGPUnlockerFailureWritesNothing(t *testing.T) {
 	}}
 
 	_, err = secret.CreatePGPUnlocker(
-		vault.NewVault(fs, testVaultStateDir, testVaultName),
-		testGPGKeyID, testGPGFingerprint)
+		fs, testVaultStateDir, testGPGKeyID, testGPGFingerprint, nil, nil)
 	require.Error(t, err)
 
 	vaultDir, err := vlt.GetDirectory()
@@ -88,40 +87,18 @@ func TestPGPUnlockerAddedTwiceKeepsFirst(t *testing.T) {
 
 	fs := afero.NewMemMapFs()
 	mnemonic := testMnemonicBuffer(t)
-	vlt, err := vault.CreateVault(fs, testVaultStateDir, testVaultName, mnemonic, nil)
+	_, err := vault.CreateVault(fs, testVaultStateDir, testVaultName, mnemonic, nil)
 	require.NoError(t, err)
 
-	first, err := secret.CreatePGPUnlocker(vlt, testGPGKeyID, testGPGFingerprint)
+	first, err := secret.CreatePGPUnlocker(
+		fs, testVaultStateDir, testGPGKeyID, testGPGFingerprint, mnemonic, nil)
 	require.NoError(t, err)
 
 	firstFiles := dirFiles(t, fs, first.GetDirectory())
 
-	second, err := secret.CreatePGPUnlocker(vlt, testGPGKeyID, testGPGFingerprint)
+	second, err := secret.CreatePGPUnlocker(
+		fs, testVaultStateDir, testGPGKeyID, testGPGFingerprint, mnemonic, nil)
 	require.NoError(t, err)
 	assert.NotEqual(t, first.GetDirectory(), second.GetDirectory())
 	assert.Equal(t, firstFiles, dirFiles(t, fs, first.GetDirectory()))
 }
-
-// TestGPGDecryptErrorHasGPGReason runs the real gpg on data that is not an
-// OpenPGP message and checks that the error carries the reason gpg writes
-// to stderr.
-func TestGPGDecryptErrorHasGPGReason(t *testing.T) {
-	// The empty GNUPGHOME keeps gpg away from the user's own keyring;
-	// LC_ALL=C keeps gpg's message in English.
-	t.Setenv("GNUPGHOME", t.TempDir())
-	t.Setenv("LC_ALL", "C")
-
-	_, err := secret.GPGDecryptFunc([]byte("not an OpenPGP message"))
-	require.ErrorContains(t, err, "gpg: no valid OpenPGP data found")
-}
-
-// TestGPGDecryptErrorWithoutGPGStderr checks that when gpg fails without
-// writing to stderr, the error ends with the exit status and nothing after it.
-//
-//nolint:paralleltest // installFakeGPG uses t.Setenv
-func TestGPGDecryptErrorWithoutGPGStderr(t *testing.T) {
-	installFakeGPG(t)
-
-	_, err := secret.GPGDecryptFunc([]byte("not an OpenPGP message"))
-	require.EqualError(t, err, "GPG decryption failed: exit status 1")
-}

internal/secret/secret.go

@@ -2,11 +2,10 @@ package secret
 
 import (
 	"errors"
-	"fmt"
 	"log/slog"
-	"os"
 	"path/filepath"
 	"strings"
+	"time"
 
 	"filippo.io/age"
 	"github.com/awnumar/memguard"
@@ -28,6 +27,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)
 }
@@ -62,53 +66,31 @@ func NewSecret(vault VaultInterface, name string) *Secret {
 		Name:      name,
 		Directory: secretDir,
 		vault:     vault,
+		Metadata: Metadata{
+			CreatedAt: time.Now(),
+			UpdatedAt: time.Now(),
+		},
 	}
 }
 
-// LoadMetadata sets the secret's times from the modification times of its
-// files, which can be read without unlocking the vault. UpdatedAt is when
-// the secret's current file was last written, by an add or a promote.
-// CreatedAt is when its oldest version's value.age was written. That file
-// is never rewritten; the version's directory is no use, as its time
-// changes when the next add rewrites the version's metadata.age.
+// LoadMetadata is deprecated - metadata is now per-version and encrypted
 func (s *Secret) LoadMetadata() error {
-	fs := s.vault.GetFilesystem()
-
-	current, err := fs.Stat(filepath.Join(s.Directory, "current"))
-	if err != nil {
-		return fmt.Errorf("failed to read when secret %s was updated: %w",
-			s.Name, err)
-	}
-
-	versions, err := ListVersions(fs, s.Directory)
-	if err != nil {
-		return err
-	}
-
-	if len(versions) == 0 {
-		return fmt.Errorf("secret %s has no versions: %w", s.Name, os.ErrNotExist)
-	}
-
-	// ListVersions lists the newest version first
-	oldest := versions[len(versions)-1]
-
-	value, err := fs.Stat(
-		filepath.Join(s.Directory, "versions", oldest, "value.age"))
-	if err != nil {
-		return fmt.Errorf("failed to read when secret %s was created: %w",
-			s.Name, err)
-	}
-
+	Debug("LoadMetadata called but is deprecated in versioned model",
+		"secret_name", s.Name)
+	// For backward compatibility, we'll populate with basic info
+	now := time.Now()
 	s.Metadata = Metadata{
-		CreatedAt: value.ModTime(),
-		UpdatedAt: current.ModTime(),
+		CreatedAt: now,
+		UpdatedAt: now,
 	}
 
 	return nil
 }
 
-// GetMetadata returns the times LoadMetadata read
+// GetMetadata returns the secret metadata (deprecated)
 func (s *Secret) GetMetadata() Metadata {
+	Debug("GetMetadata called but is deprecated in versioned model", "secret_name", s.Name)
+
 	return s.Metadata
 }
 

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

@@ -32,8 +32,7 @@ type VersionMetadata struct {
 	ID        string     `json:"id"`                  // ULID
 	CreatedAt *time.Time `json:"createdAt,omitempty"` // When version was created
 	NotBefore *time.Time `json:"notBefore,omitempty"` // When this version becomes active
-	// NotAfter is set when another version is added while this one is current
-	NotAfter *time.Time `json:"notAfter,omitempty"`
+	NotAfter  *time.Time `json:"notAfter,omitempty"`  // Expiry (nil = current)
 }
 
 // Version represents a version of a secret

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
@@ -158,9 +140,8 @@ func ListVaults(fs afero.Fs, stateDir string) ([]string, error) {
 		return nil, fmt.Errorf("failed to read vaults directory: %w", err)
 	}
 
-	// Extract vault names. Not nil, so that no vaults prints [] in JSON,
-	// not null.
-	vaults := make([]string, 0, len(entries))
+	// Extract vault names
+	var vaults []string
 
 	for _, entry := range entries {
 		if entry.IsDir() {
@@ -231,40 +212,16 @@ func processMnemonicForVault(
 	return ltIdentity, derivationIndex, publicKeyHash, familyHash, nil
 }
 
-// CheckNewVaultName returns an error when name cannot be the name of a new
-// 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. 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 {
-		return err
-	}
-
-	exists, err := afero.DirExists(fs, filepath.Join(stateDir, "vaults.d", name))
-	if err != nil {
-		return fmt.Errorf("failed to check if vault exists: %w", err)
-	}
-
-	if exists {
-		return fmt.Errorf("vault %s %w", name, ErrVaultExists)
-	}
-
-	return nil
-}
-
 // CreateVault creates a new vault and selects it as the current vault. When
 // mnemonic is not nil, the vault's long-term key is derived from it, and the
 // returned vault has it as its Mnemonic; when it is nil, the vault has no
-// long-term key. When passphrase is not nil, the vault gets a passphrase
-// unlocker protected by it, as its current unlocker; that needs a mnemonic.
-// It refuses an invalid name or a vault that already exists, as
-// CheckNewVaultName does, before writing anything. The commands that call it
-// hold the state directory lock, so no other command can create the vault
-// between the check and the writes.
+// long-term key until one is imported. When passphrase is not nil, the vault
+// gets a passphrase unlocker protected by it, as its current unlocker; that
+// needs a mnemonic. It refuses a vault that already exists before writing
+// anything: creating it again would replace its keys, and its secrets could
+// no longer be decrypted. The commands that call it hold the state directory
+// lock, so no other command can create the vault between the check and the
+// writes.
 //
 // The vault is written whole into a temporary directory, which is renamed
 // into vaults.d only once complete, and only then selected: a crash at any
@@ -276,17 +233,30 @@ func CreateVault(
 ) (*Vault, error) {
 	secret.Debug("Creating new vault", "name", name, "state_dir", stateDir)
 
-	err := CheckNewVaultName(fs, stateDir, name)
+	err := ValidateVaultName(name)
 	if err != nil {
+		secret.Debug("Invalid vault name provided", "vault_name", name)
+
 		return nil, err
 	}
 
+	secret.Debug("Vault name validation passed", "vault_name", name)
+
+	vaultDir := filepath.Join(stateDir, "vaults.d", name)
+
+	exists, err := afero.DirExists(fs, vaultDir)
+	if err != nil {
+		return nil, fmt.Errorf("failed to check if vault exists: %w", err)
+	}
+
+	if exists {
+		return nil, fmt.Errorf("vault %s %w", name, ErrVaultExists)
+	}
+
 	if passphrase != nil && mnemonic == nil {
 		return nil, fmt.Errorf("vault %s %w", name, ErrUnlockerWithoutMnemonic)
 	}
 
-	vaultDir := filepath.Join(stateDir, "vaults.d", name)
-
 	secret.Debug("Creating vault directory structure", "vault_dir", vaultDir)
 
 	err = secret.WriteDir(fs, vaultDir, func(dir string) error {

internal/vault/metadata.go

@@ -4,9 +4,7 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"encoding/json"
-	"errors"
 	"fmt"
-	"os"
 	"path/filepath"
 
 	"github.com/spf13/afero"
@@ -35,10 +33,7 @@ func ComputeDoubleSHA256(data []byte) string {
 }
 
 // GetNextDerivationIndex finds the next available derivation index for a given mnemonic
-// by deriving the public key for index 0 and using its hash to identify related vaults.
-// It fails when a vault's metadata file exists but cannot be read or parsed: the
-// index that vault uses is then unknown, and handing it out again would give a
-// second vault the same long-term key.
+// by deriving the public key for index 0 and using its hash to identify related vaults
 func GetNextDerivationIndex(
 	fs afero.Fs, stateDir string, mnemonic string,
 ) (uint32, error) {
@@ -77,23 +72,21 @@ func GetNextDerivationIndex(
 			continue
 		}
 
+		// Try to read vault metadata
 		metadataPath := filepath.Join(vaultsDir, entry.Name(), "vault-metadata.json")
 
 		metadataBytes, err := afero.ReadFile(fs, metadataPath)
-		if errors.Is(err, os.ErrNotExist) {
-			// Skip directories without metadata
-			continue
-		}
-
 		if err != nil {
-			return 0, fmt.Errorf("failed to read vault metadata %s: %w", metadataPath, err)
+			// Skip vaults without metadata
+			continue
 		}
 
 		var metadata Metadata
 
 		err = json.Unmarshal(metadataBytes, &metadata)
 		if err != nil {
-			return 0, fmt.Errorf("failed to parse vault metadata %s: %w", metadataPath, err)
+			// Skip vaults with invalid metadata
+			continue
 		}
 
 		// Check if this vault uses the same mnemonic by comparing family hashes

internal/vault/metadata_test.go

@@ -1,15 +1,11 @@
 package vault_test
 
 import (
-	"errors"
 	"path/filepath"
 	"strings"
 	"testing"
 
 	"github.com/spf13/afero"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/secret/internal/secret"
 	"sneak.berlin/go/secret/internal/vault"
 	"sneak.berlin/go/secret/pkg/agehd"
 )
@@ -167,88 +163,6 @@ func testGetNextDerivationIndex(t *testing.T, fs afero.Fs) {
 	}
 }
 
-// errMetadataUnreadable is returned by the test filesystem in place of a
-// successful open of one vault's metadata file.
-var errMetadataUnreadable = errors.New("input/output error")
-
-// metadataReadFailFs fails every open of the file at unreadablePath. The
-// file still exists, so checking for it succeeds and only reading it fails.
-type metadataReadFailFs struct {
-	afero.Fs
-
-	unreadablePath string
-}
-
-//nolint:ireturn // afero.File is the interface required by afero.Fs
-func (f *metadataReadFailFs) Open(name string) (afero.File, error) {
-	if name == f.unreadablePath {
-		return nil, errMetadataUnreadable
-	}
-
-	return f.Fs.Open(name)
-}
-
-// TestCreateVaultWithCorruptMetadataFails checks that creating a vault from
-// a mnemonic fails, naming the file and leaving no new vault, when another
-// vault's metadata file exists but cannot be parsed or read. That vault's
-// derivation index is then unknown, so the new vault could get the same one
-// and the same key.
-func TestCreateVaultWithCorruptMetadataFails(t *testing.T) {
-	t.Parallel()
-
-	vaultsDir := filepath.Join(testStateDir, "vaults.d")
-	metadataPath := filepath.Join(vaultsDir, "work", "vault-metadata.json")
-
-	tests := []struct {
-		name string
-		// breakMetadata makes the work vault's metadata file unusable on
-		// base and returns the filesystem to create the second vault on.
-		breakMetadata func(t *testing.T, base afero.Fs) afero.Fs
-	}{
-		{
-			name: "parsing the file fails",
-			breakMetadata: func(t *testing.T, base afero.Fs) afero.Fs {
-				t.Helper()
-
-				err := afero.WriteFile(base, metadataPath, []byte("{"),
-					secret.FilePerms)
-				require.NoError(t, err)
-
-				return base
-			},
-		},
-		{
-			name: "reading the file fails",
-			breakMetadata: func(_ *testing.T, base afero.Fs) afero.Fs {
-				return &metadataReadFailFs{Fs: base, unreadablePath: metadataPath}
-			},
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-
-			base := afero.NewMemMapFs()
-
-			_, err := vault.CreateVault(base, testStateDir, "work",
-				testMnemonicBuffer(t), nil)
-			require.NoError(t, err)
-
-			fs := tt.breakMetadata(t, base)
-
-			_, err = vault.CreateVault(fs, testStateDir, "other",
-				testMnemonicBuffer(t), nil)
-			require.ErrorContains(t, err, metadataPath)
-
-			entries, err := afero.ReadDir(base, vaultsDir)
-			require.NoError(t, err)
-			require.Len(t, entries, 1)
-			assert.Equal(t, "work", entries[0].Name())
-		})
-	}
-}
-
 func testMetadataPersistence(t *testing.T, fs afero.Fs) {
 	t.Helper()
 

internal/vault/secrets.go

@@ -427,8 +427,7 @@ func (v *Vault) UnlockVault() (*age.X25519Identity, error) {
 	return longTermIdentity, nil
 }
 
-// GetSecretObject retrieves a Secret object from this vault. Its times are
-// zero until its LoadMetadata reads them.
+// GetSecretObject retrieves a Secret object with metadata loaded from this vault
 func (v *Vault) GetSecretObject(name string) (*secret.Secret, error) {
 	err := ValidateSecretName(name)
 	if err != nil {
@@ -455,7 +454,16 @@ func (v *Vault) GetSecretObject(name string) (*secret.Secret, error) {
 		return nil, fmt.Errorf("secret %s %w", name, ErrSecretNotFound)
 	}
 
-	return secret.NewSecret(v, name), nil
+	// Create a Secret object
+	secretObj := secret.NewSecret(v, name)
+
+	// Load the metadata from disk
+	err = secretObj.LoadMetadata()
+	if err != nil {
+		return nil, err
+	}
+
+	return secretObj, nil
 }
 
 // CopySecretVersion copies a single version from source into destSecretDir
@@ -573,8 +581,8 @@ func (v *Vault) CopySecretAllVersions(
 	}
 
 	// Copy each version and the current pointer, then move the copy into place
-	err = v.copyVersions(srcVault, srcIdentity, srcSecretName, srcSecretDir,
-		destSecretName, destSecretDir, versions, currentVersion)
+	err = v.copyVersions(srcVault, srcIdentity,
+		srcSecretName, destSecretName, destSecretDir, versions, currentVersion)
 	if err != nil {
 		return err
 	}
@@ -793,13 +801,10 @@ func (v *Vault) createAndSaveVersion(
 // pointer into a temporary directory, then moves that directory to
 // destSecretDir, replacing a secret already there. Nothing in this vault
 // changes until the copy is complete, so an interrupted copy leaves only a
-// temporary directory behind. Each copied value.age and current file gets
-// the modification time of the file it was copied from, so `secret list`,
-// which reads a secret's times from them, lists the copy with the times of
-// the source.
+// temporary directory behind.
 func (v *Vault) copyVersions(
 	srcVault *Vault, srcIdentity *age.X25519Identity,
-	srcSecretName, srcSecretDir, destSecretName, destSecretDir string,
+	srcSecretName, destSecretName, destSecretDir string,
 	versions []string, currentVersion string,
 ) error {
 	buildDir, err := secret.TempDirFor(v.fs, destSecretDir)
@@ -818,14 +823,6 @@ func (v *Vault) copyVersions(
 		if err != nil {
 			return fmt.Errorf("failed to copy version %s: %w", versionName, err)
 		}
-
-		valuePath := filepath.Join("versions", versionName, "value.age")
-
-		err = copyModTime(srcVault.fs, filepath.Join(srcSecretDir, valuePath),
-			v.fs, filepath.Join(buildDir, valuePath))
-		if err != nil {
-			return err
-		}
 	}
 
 	err = secret.SetCurrentVersion(v.fs, buildDir, currentVersion)
@@ -833,12 +830,6 @@ func (v *Vault) copyVersions(
 		return fmt.Errorf("failed to set current version: %w", err)
 	}
 
-	err = copyModTime(srcVault.fs, filepath.Join(srcSecretDir, "current"),
-		v.fs, filepath.Join(buildDir, "current"))
-	if err != nil {
-		return err
-	}
-
 	// With --force, the secret being replaced goes only now that its
 	// replacement is complete
 	exists, err := afero.DirExists(v.fs, destSecretDir)
@@ -863,26 +854,6 @@ func (v *Vault) copyVersions(
 	return nil
 }
 
-// copyModTime gives destPath in destFs the modification time of srcPath in
-// srcFs.
-func copyModTime(
-	srcFs afero.Fs, srcPath string, destFs afero.Fs, destPath string,
-) error {
-	info, err := srcFs.Stat(srcPath)
-	if err != nil {
-		return fmt.Errorf("failed to read modification time of %s: %w",
-			srcPath, err)
-	}
-
-	err = destFs.Chtimes(destPath, info.ModTime(), info.ModTime())
-	if err != nil {
-		return fmt.Errorf("failed to set modification time of %s: %w",
-			destPath, err)
-	}
-
-	return nil
-}
-
 // checkCopyDestination refuses to copy over an existing secret unless force
 // is set. A secret being replaced is removed by copyVersions, once its
 // replacement is complete.

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)