Wipe memguard buffers on every exit and restore echo (closes #35)

Entry() now returns the exit code and only main calls os.Exit, so the
deferred memguard.Purge() in Entry() runs on success and on error;
before, os.Exit(1) skipped every deferred Destroy().

SIGINT and SIGTERM go through memguard's handler, which wipes every
buffer and exits with status 1. The passphrase prompt turns terminal
echo off until its read returns, and the handler exits before that, so
the handler first restores the terminal settings saved at startup, but
only when this process is in the terminal's foreground process group: a
background process that changes the terminal is stopped instead of
exiting.

Model: opus-5-5

TODO.md

@@ -25,39 +25,13 @@ Bring the repo into policy compliance in one commit:
 
 # Completed Steps
 
-- 2026-10-03: Commands that change the state directory hold one lock
+- 2026-10-03: Key material is wiped on every exit: `Entry()` returns
-  (`flock` on `lock` in the state directory; a mutex on the in-memory
+  the exit code after its deferred `memguard.Purge()` has run, and only
-  test filesystem), so concurrent commands no longer lose versions or
+  `main` calls `os.Exit`. SIGINT and SIGTERM go through memguard's
-  race on the current pointers. Every file is written through
+  handler, which wipes every buffer before exiting; when the process is
-  `secret.WriteFileAtomic` (temporary file, sync, rename), so no file
+  in the terminal's foreground process group it first restores the
-  is ever half-written and `current`, `currentvault` and
+  terminal settings from startup, so an interrupted passphrase prompt no
-  `current-unlocker` never go missing. New versions, new secrets and
+  longer leaves echo off.
-  cross-vault copies are built in a temporary directory and renamed
-  into place, and removals rename out of the way first, so a version
-  or secret is never half-added and never half-removed. An
-  interrupted command can still leave:
-    - a broken unlocker, when it was replacing one: an unlocker added
-      under the directory name of an existing one is rewritten file by
-      file. That happens to a passphrase unlocker added to a vault that
-      has one, and to a PGP, keychain or Secure Enclave unlocker added
-      on the same host and day as another of its type
-      (https://git.eeqj.de/sneak/secret/issues/71);
-    - from `vault create` stopped at the passphrase prompt, a new vault
-      with no unlocker that is already the current vault; from `init`
-      stopped there, the default vault with no unlocker;
-    - from an unlocker add stopped before its metadata is written, a
-      directory that `unlocker list` warns about and `unlocker rm`
-      cannot remove;
-    - data under a `.tmp-` name in the state directory: a secret or
-      version being added, or the secret, version, unlocker or vault
-      being removed, encrypted keys included. Nothing deletes it; it
-      must be deleted by hand
-      (https://git.eeqj.de/sneak/secret/issues/75).
-- 2026-10-03: The keychain unlocker's age key passphrase stays in
-  locked memory: it is generated into a locked buffer, and the
-  keychain JSON is written and read by `KeychainData` code in
-  `internal/secret/keychaindata.go` (tested on Linux) without
-  `encoding/json` holding it; the JSON field names are unchanged.
 - 2026-10-02: A plain `docker build .` builds again: the size tests
   skip a case that needs more locked memory than the process can
   lock, and run every case under `script/cibuild`. The image stamps the
@@ -121,11 +95,14 @@ Bring the repo into policy compliance in one commit:
     - Command injection: GPG key IDs passed unescaped to exec.Command
       (pgpunlocker.go:323-327); data.String() passed unescaped to the
       security command (keychainunlocker.go:472-476).
-    - Memory security: age identity .String() creates unprotected
+    - Memory security: KeychainData stores AgePrivKeyPassphrase as a
-      copies (keychainunlocker.go:356, pgpunlocker.go:256,
+      plain string (keychainunlocker.go:342,393-396); age identity
-      version.go:155); age secret key held in a plain string in
+      .String() creates unprotected copies (keychainunlocker.go:356,
-      cli/crypto.go:86,91,113; private keys exposed via buffer.Bytes()
+      pgpunlocker.go:256, version.go:155); age secret key held in a
-      to GPGEncryptFunc and EncryptWithPassphrase.
+      plain string in cli/crypto.go:86,91,113; private keys exposed via
+      buffer.Bytes() to GPGEncryptFunc and EncryptWithPassphrase.
+    - Race conditions: no file locking in vault/secrets.go:142-176;
+      non-atomic writes can leave the vault inconsistent.
     - Input validation: dots in secret names risk path traversal
       (vault/secrets.go:75-99); no maximum secret size (DoS).
     - Timing attacks: bytes.Equal passphrase compare (cli/init.go:

cmd/secret/main.go

@@ -1,8 +1,12 @@
 // Package main is the entry point for the secret CLI application.
 package main
 
-import "git.eeqj.de/sneak/secret/internal/cli"
+import (
+	"os"
+
+	"git.eeqj.de/sneak/secret/internal/cli"
+)
 
 func main() {
-	cli.Entry()
+	os.Exit(cli.Entry())
 }

internal/cli/crypto.go

@@ -72,18 +72,10 @@ func newDecryptCmd() *cobra.Command {
 
 // resolveEncryptionKey returns a secure buffer holding the age secret key
 // for the named secret, generating and storing a new key if the secret
-// does not exist. The caller must destroy the returned buffer. It holds the
+// does not exist. The caller must destroy the returned buffer.
-// state directory lock itself, so that Encrypt streams its input and output
-// unlocked and cannot block a secret command at the other end of a pipe.
 func (cli *Instance) resolveEncryptionKey(
 	vlt *vault.Vault, secretName string,
 ) (*memguard.LockedBuffer, error) {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return nil, err
-	}
-	defer release()
-
 	// Check if secret exists
 	secretObj := secret.NewSecret(vlt, secretName)
 

internal/cli/entry_test.go

@@ -0,0 +1,108 @@
+package cli_test
+
+import (
+	"bufio"
+	"context"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+
+	"git.eeqj.de/sneak/secret/internal/cli"
+	"git.eeqj.de/sneak/secret/internal/secret"
+	"github.com/awnumar/memguard"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Entry must return its exit code rather than exit, so that its deferred
+// memguard purge runs on the success and the error path alike.
+//
+//nolint:paralleltest // sets os.Args, and Entry wipes every buffer in the process
+func TestEntryWipesBuffersAndReturnsExitCode(t *testing.T) {
+	savedArgs := os.Args
+
+	t.Cleanup(func() { os.Args = savedArgs })
+
+	tests := []struct {
+		args     []string
+		exitCode int
+	}{
+		{args: []string{"secret", "--help"}, exitCode: 0},
+		{args: []string{"secret", "no-such-command"}, exitCode: 1},
+	}
+
+	for _, tt := range tests {
+		buf := memguard.NewBufferFromBytes([]byte("key material"))
+		os.Args = tt.args
+
+		assert.Equal(t, tt.exitCode, cli.Entry(), "exit code for %v", tt.args)
+		assert.False(t, buf.IsAlive(), "Entry left a buffer unwiped for %v", tt.args)
+	}
+}
+
+// Ctrl-C while `secret add` waits for the value on stdin must end the
+// process through memguard's signal handler, which wipes every buffer and
+// exits with status 1, not through Go's default handling, which kills the
+// process with the buffers intact.
+func TestInterruptExitsThroughMemguard(t *testing.T) {
+	t.Parallel()
+
+	const waitingForValue = "Reading secret value from stdin"
+
+	ctx, cancel := context.WithTimeout(t.Context(), time.Minute)
+	defer cancel()
+
+	wd, err := filepath.Abs("../..")
+	require.NoError(t, err)
+
+	secretPath := filepath.Join(wd, "secret")
+	env := []string{
+		secret.EnvStateDir + "=" + t.TempDir(),
+		secret.EnvMnemonic + "=" + testMnemonic,
+		secret.EnvUnlockPassphrase + "=test-passphrase",
+		"PATH=/usr/bin:/bin",
+		// The debug log on stderr shows when add starts waiting for the value.
+		"GODEBUG=berlin.sneak.pkg.secret",
+	}
+
+	//nolint:gosec // G204: test executes the freshly built secret binary
+	initCmd := exec.CommandContext(ctx, secretPath, "init")
+	initCmd.Env = env
+
+	output, err := initCmd.CombinedOutput()
+	require.NoError(t, err, "init should succeed: %s", output)
+
+	//nolint:gosec // G204: test executes the freshly built secret binary
+	addCmd := exec.CommandContext(ctx, secretPath, "add", "test/secret")
+	addCmd.Env = env
+
+	// Held open and never written, so add keeps waiting for the value.
+	stdin, err := addCmd.StdinPipe()
+	require.NoError(t, err)
+
+	defer func() { _ = stdin.Close() }()
+
+	stderr, err := addCmd.StderrPipe()
+	require.NoError(t, err)
+	require.NoError(t, addCmd.Start())
+
+	waiting := false
+
+	scanner := bufio.NewScanner(stderr)
+	for !waiting && scanner.Scan() {
+		waiting = strings.Contains(scanner.Text(), waitingForValue)
+	}
+
+	require.True(t, waiting, "add never logged %q", waitingForValue)
+	require.NoError(t, addCmd.Process.Signal(os.Interrupt))
+
+	err = addCmd.Wait()
+
+	var exitErr *exec.ExitError
+
+	require.ErrorAs(t, err, &exitErr)
+	assert.Equal(t, 1, exitErr.ExitCode(), "add ended with %v", err)
+}

internal/cli/generate.go

@@ -155,12 +155,6 @@ func (cli *Instance) GenerateSecret(
 		return fmt.Errorf("failed to generate random secret: %w", err)
 	}
 
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Store the secret in the vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {

internal/cli/init.go

@@ -103,21 +103,8 @@ func (cli *Instance) setupDefaultVault(
 	return vlt, ltIdentity, nil
 }
 
-// Init initializes the secret manager, holding the state directory lock
+// Init initializes the secret manager
-// while initialize runs
 func (cli *Instance) Init(cmd *cobra.Command) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
-	return cli.initialize(cmd)
-}
-
-// initialize creates the state directory, the default vault and its first
-// unlocker
-func (cli *Instance) initialize(cmd *cobra.Command) error {
 	secret.Debug("Starting secret manager initialization")
 
 	// Create state directory

internal/cli/lock_test.go

@@ -1,493 +0,0 @@
-//nolint:testpackage // sets the unexported fields of Instance
-package cli
-
-import (
-	"io"
-	"os"
-	"path/filepath"
-	"strconv"
-	"strings"
-	"sync"
-	"testing"
-	"time"
-
-	"git.eeqj.de/sneak/secret/internal/secret"
-	"git.eeqj.de/sneak/secret/internal/vault"
-	"github.com/awnumar/memguard"
-	"github.com/spf13/afero"
-	"github.com/spf13/cobra"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-const (
-	// lockWait is how long a test waits for something that must happen
-	// once the lock is free.
-	lockWait = 10 * time.Second
-
-	// heldWait is how long a test watches a command that must wait for the
-	// lock. A command that takes no lock changes the state directory well
-	// within it.
-	heldWait = 100 * time.Millisecond
-
-	// testPassphrase protects the passphrase unlockers the tests create.
-	testPassphrase = "test-passphrase"
-
-	// testInput is a file outside the state directory that commands read.
-	testInput = "/input"
-)
-
-// lockInBackground starts taking the state directory lock and returns a
-// channel that delivers the function releasing it once it has been taken.
-func lockInBackground(t *testing.T, fs afero.Fs) <-chan func() {
-	t.Helper()
-
-	taken := make(chan func(), 1)
-
-	go func() {
-		release, err := vault.LockStateDir(fs, testStateDir)
-		if assert.NoError(t, err) {
-			taken <- release
-		}
-	}()
-
-	return taken
-}
-
-// addAtOnce runs one add of the secret name per value, all at once, and
-// returns their errors.
-func addAtOnce(
-	fs afero.Fs, stateDir, name string, force bool, values []string,
-) []error {
-	errs := make(chan error, len(values))
-
-	for _, value := range values {
-		go func() {
-			cli := NewCLIInstanceWithStateDir(fs, stateDir)
-			cli.cmd = &cobra.Command{}
-			cli.cmd.SetIn(strings.NewReader(value))
-
-			errs <- cli.AddSecret(name, force)
-		}()
-	}
-
-	results := make([]error, 0, len(values))
-	for range values {
-		results = append(results, <-errs)
-	}
-
-	return results
-}
-
-// numbered returns count distinct values starting with prefix.
-func numbered(prefix string, count int) []string {
-	values := make([]string, 0, count)
-	for i := range count {
-		values = append(values, prefix+"-"+strconv.Itoa(i))
-	}
-
-	return values
-}
-
-// TestConcurrentAddsKeepEveryVersion runs adds of one secret at once, on
-// the in-memory and on the real filesystem. Without the state directory
-// lock, adds of a new secret all find it absent and replace each other, and
-// forced adds read the same highest version number and overwrite each
-// other's version. With it they behave as if run one after another.
-//
-//nolint:paralleltest // t.Setenv forbids parallel subtests
-func TestConcurrentAddsKeepEveryVersion(t *testing.T) {
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-
-	const adds = 8
-
-	for _, tc := range []struct {
-		name     string
-		fs       afero.Fs
-		stateDir string
-	}{
-		{"memory", afero.NewMemMapFs(), testStateDir},
-		{"real", afero.NewOsFs(), t.TempDir()},
-	} {
-		t.Run(tc.name, func(t *testing.T) {
-			_, err := vault.CreateVault(tc.fs, tc.stateDir, "default")
-			require.NoError(t, err)
-
-			// One add creates the secret; the others find that it exists
-			created := 0
-
-			for _, err := range addAtOnce(tc.fs, tc.stateDir, "shared", false,
-				numbered("create", adds)) {
-				if err == nil {
-					created++
-				} else {
-					require.ErrorIs(t, err, vault.ErrSecretExists)
-				}
-			}
-
-			require.Equal(t, 1, created, "exactly one add creates the secret")
-
-			// Every forced add stores a version of its own
-			for _, err := range addAtOnce(tc.fs, tc.stateDir, "shared", true,
-				numbered("force", adds)) {
-				require.NoError(t, err)
-			}
-
-			vlt, err := vault.GetCurrentVault(tc.fs, tc.stateDir)
-			require.NoError(t, err)
-
-			vaultDir, err := vlt.GetDirectory()
-			require.NoError(t, err)
-
-			versions, err := secret.ListVersions(tc.fs,
-				filepath.Join(vaultDir, "secrets.d", "shared"))
-			require.NoError(t, err)
-			require.Len(t, versions, adds+1, "one version per successful add")
-
-			values := make(map[string]bool, len(versions))
-
-			for _, version := range versions {
-				value, err := vlt.GetSecretVersion("shared", version)
-				require.NoError(t, err)
-
-				values[string(value)] = true
-			}
-
-			assert.Len(t, values, adds+1, "every add stored its own value")
-		})
-	}
-}
-
-// readNotifier passes reads through to Reader and closes reading at the
-// first one.
-type readNotifier struct {
-	io.Reader
-
-	reading chan struct{}
-	once    sync.Once
-}
-
-func (r *readNotifier) Read(p []byte) (int, error) {
-	r.once.Do(func() { close(r.reading) })
-
-	return r.Reader.Read(p)
-}
-
-// TestEncryptPipedIntoAdd runs `secret encrypt key | secret add name` in
-// one process, starting encrypt once add is reading its input. Had add
-// taken the state directory lock before reading, it would hold the lock
-// while waiting for encrypt's output, and encrypt would wait for the lock
-// to store its key: neither would finish.
-func TestEncryptPipedIntoAdd(t *testing.T) {
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-
-	fs := afero.NewMemMapFs()
-	_, err := vault.CreateVault(fs, testStateDir, "default")
-	require.NoError(t, err)
-	require.NoError(t, afero.WriteFile(fs, testInput, []byte("piped"), 0o600))
-
-	pipeReader, pipeWriter := io.Pipe()
-	// If the test gives up, this makes add's read fail, so that both
-	// commands return and release the lock the other tests use
-	t.Cleanup(func() { _ = pipeReader.Close() })
-
-	const commands = 2
-
-	input := &readNotifier{Reader: pipeReader, reading: make(chan struct{})}
-	results := make(chan error, commands)
-
-	go func() {
-		add := NewCLIInstanceWithStateDir(fs, testStateDir)
-		add.cmd = &cobra.Command{}
-		add.cmd.SetIn(input)
-
-		results <- add.AddSecret("encrypted", false)
-	}()
-
-	go func() {
-		<-input.reading
-
-		encrypt := NewCLIInstanceWithStateDir(fs, testStateDir)
-		encrypt.cmd = &cobra.Command{}
-		encrypt.cmd.SetOut(pipeWriter)
-
-		err := encrypt.Encrypt("key", testInput, "")
-		// Ends add's input, as the end of the pipe does
-		_ = pipeWriter.CloseWithError(err)
-
-		results <- err
-	}()
-
-	timeout := time.After(lockWait)
-
-	for range commands {
-		select {
-		case err := <-results:
-			require.NoError(t, err)
-		case <-timeout:
-			t.Fatal("secret encrypt piped into secret add never finished")
-		}
-	}
-}
-
-// TestFailedCommandReleasesLock checks that a command failing after it
-// took the state directory lock leaves the lock free for the next command.
-func TestFailedCommandReleasesLock(t *testing.T) {
-	t.Parallel()
-
-	fs := afero.NewMemMapFs()
-	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
-
-	// Fails once it holds the lock: there is no current vault
-	err := cli.RemoveSecret(&cobra.Command{}, "missing", false)
-	require.Error(t, err)
-
-	select {
-	case release := <-lockInBackground(t, fs):
-		release()
-	case <-time.After(lockWait):
-		t.Fatal("the failed command left the state directory locked")
-	}
-}
-
-// stateDirModTimes returns the modification time of every file and
-// directory under the test state directory. Any change a command makes, even
-// rewriting a file with the same content, changes it.
-func stateDirModTimes(t *testing.T, fs afero.Fs) map[string]int64 {
-	t.Helper()
-
-	modTimes := make(map[string]int64)
-
-	err := afero.Walk(fs, testStateDir,
-		func(path string, info os.FileInfo, err error) error {
-			if err != nil {
-				return err
-			}
-
-			modTimes[path] = info.ModTime().UnixNano()
-
-			return nil
-		})
-	require.NoError(t, err)
-
-	return modTimes
-}
-
-// setupEveryCommand makes what each command in
-// TestChangingCommandsWaitForLock needs: the current vault "default" with
-// two versions of "test/secret", the vault "other" without a long-term key,
-// for vault import, and the file testInput. If withUnlocker is set, it also
-// gives "default" a passphrase unlocker, which is slow. It returns the older
-// version and the unlocker's ID.
-func setupEveryCommand(
-	t *testing.T, fs afero.Fs, withUnlocker bool,
-) (string, string) {
-	t.Helper()
-
-	other, err := vault.CreateVault(fs, testStateDir, "other")
-	require.NoError(t, err)
-
-	otherDir, err := other.GetDirectory()
-	require.NoError(t, err)
-	require.NoError(t, fs.Remove(filepath.Join(otherDir, "pub.age")))
-
-	vlt, err := vault.CreateVault(fs, testStateDir, "default")
-	require.NoError(t, err)
-
-	addTestSecret(t, vlt, []byte("older"), false)
-	addTestSecret(t, vlt, []byte("newer"), true)
-
-	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, 2)
-
-	unlockerID := ""
-
-	if withUnlocker {
-		passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
-		defer passphrase.Destroy()
-
-		unlocker, err := vlt.CreatePassphraseUnlocker(passphrase)
-		require.NoError(t, err)
-
-		unlockerID = unlocker.GetID()
-	}
-
-	require.NoError(t, afero.WriteFile(fs, testInput, []byte("input"), 0o600))
-
-	// Newest first
-	return versions[1], unlockerID
-}
-
-// requireWaitsForLock runs a command, given what setupEveryCommand made,
-// while holding the state directory lock. The command must neither finish
-// nor change anything while the lock is held, and must succeed once it is
-// released.
-func requireWaitsForLock(
-	t *testing.T,
-	withUnlocker bool,
-	run func(cli *Instance, olderVersion, unlockerID string) error,
-) {
-	t.Helper()
-
-	fs := afero.NewMemMapFs()
-	olderVersion, unlockerID := setupEveryCommand(t, fs, withUnlocker)
-	before := stateDirModTimes(t, fs)
-
-	release, err := vault.LockStateDir(fs, testStateDir)
-	require.NoError(t, err)
-
-	// Released at most once, and also if the test fails while holding it,
-	// so that later tests can take it
-	release = sync.OnceFunc(release)
-	defer release()
-
-	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, olderVersion, unlockerID) }()
-
-	select {
-	case err := <-done:
-		t.Fatalf("finished while the lock was held, with error %v", err)
-	case <-time.After(heldWait):
-	}
-
-	assert.Equal(t, before, stateDirModTimes(t, fs),
-		"changed the state directory while the lock was held")
-
-	release()
-
-	select {
-	case err := <-done:
-		require.NoError(t, err)
-	case <-time.After(lockWait):
-		t.Fatal("did not finish once the lock was released")
-	}
-}
-
-// TestChangingCommandsWaitForLock checks that each command that changes the
-// state directory waits for its lock.
-//
-//nolint:paralleltest // t.Setenv forbids parallel subtests
-func TestChangingCommandsWaitForLock(t *testing.T) {
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-	t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
-
-	for _, tc := range []struct {
-		name         string
-		withUnlocker bool
-		run          func(cli *Instance, olderVersion, unlockerID string) error
-	}{
-		{"add", false, func(cli *Instance, _, _ string) error {
-			return cli.AddSecret("added", false)
-		}},
-		{"import", false, func(cli *Instance, _, _ string) error {
-			return cli.ImportSecret(cli.cmd, "imported", testInput, false)
-		}},
-		{"generate secret", false, func(cli *Instance, _, _ string) error {
-			return cli.GenerateSecret(cli.cmd, "generated", 16, "base58", false)
-		}},
-		{"encrypt", false, func(cli *Instance, _, _ string) error {
-			return cli.Encrypt("key", testInput, "")
-		}},
-		{"rm", false, func(cli *Instance, _, _ string) error {
-			return cli.RemoveSecret(cli.cmd, "test/secret", false)
-		}},
-		{"move", false, func(cli *Instance, _, _ string) error {
-			return cli.MoveSecret(cli.cmd, "test/secret", "moved", false)
-		}},
-		{"version promote", false, func(cli *Instance, olderVersion, _ string) error {
-			return cli.PromoteVersion(cli.cmd, "test/secret", olderVersion)
-		}},
-		{"version rm", false, func(cli *Instance, olderVersion, _ string) error {
-			return cli.RemoveVersion(cli.cmd, "test/secret", olderVersion)
-		}},
-		{"vault create", false, func(cli *Instance, _, _ string) error {
-			return cli.CreateVault(cli.cmd, "created")
-		}},
-		{"vault select", false, func(cli *Instance, _, _ string) error {
-			return cli.SelectVault(cli.cmd, "other")
-		}},
-		{"vault import", false, func(cli *Instance, _, _ string) error {
-			return cli.VaultImport(cli.cmd, "other")
-		}},
-		{"vault rm", false, func(cli *Instance, _, _ string) error {
-			return cli.RemoveVault(cli.cmd, "other", false)
-		}},
-		{"unlocker add", false, func(cli *Instance, _, _ string) error {
-			return cli.UnlockersAdd("passphrase", cli.cmd)
-		}},
-		{"unlocker rm", true, func(cli *Instance, _, unlockerID string) error {
-			return cli.UnlockersRemove(unlockerID, true, cli.cmd)
-		}},
-		{"unlocker select", true, func(cli *Instance, _, unlockerID string) error {
-			return cli.UnlockerSelect(unlockerID)
-		}},
-		{"init", false, func(cli *Instance, _, _ string) error {
-			return cli.Init(cli.cmd)
-		}},
-	} {
-		t.Run(tc.name, func(t *testing.T) {
-			requireWaitsForLock(t, tc.withUnlocker, tc.run)
-		})
-	}
-}
-
-// TestEncryptStreamsUnlocked checks that secret encrypt has released the
-// state directory lock by the time it writes its output. Holding it while
-// streaming would stall every other changing command for as long as the
-// stream lasts, and forever when the other end of the pipe is one of them.
-func TestEncryptStreamsUnlocked(t *testing.T) {
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-
-	fs := afero.NewMemMapFs()
-	_, err := vault.CreateVault(fs, testStateDir, "default")
-	require.NoError(t, err)
-	require.NoError(t, afero.WriteFile(fs, testInput, []byte("streamed"), 0o600))
-
-	outputReader, outputWriter := io.Pipe()
-	done := make(chan error, 1)
-
-	go func() {
-		encrypt := NewCLIInstanceWithStateDir(fs, testStateDir)
-		encrypt.cmd = &cobra.Command{}
-		encrypt.cmd.SetOut(outputWriter)
-
-		err := encrypt.Encrypt("key", testInput, "")
-		_ = outputWriter.CloseWithError(err)
-
-		done <- err
-	}()
-
-	// The first byte of output: encrypt is streaming now, and blocked
-	// writing until it is read
-	_, err = io.ReadFull(outputReader, make([]byte, 1))
-	require.NoError(t, err)
-
-	taken := lockInBackground(t, fs)
-
-	select {
-	case release := <-taken:
-		release()
-	case <-time.After(lockWait):
-		// Let encrypt finish, so that it releases the lock, then free it
-		// again for the tests that follow
-		_, _ = io.Copy(io.Discard, outputReader)
-
-		(<-taken)()
-		t.Fatal("secret encrypt held the lock while streaming")
-	}
-
-	_, err = io.Copy(io.Discard, outputReader)
-	require.NoError(t, err)
-	require.NoError(t, <-done)
-}

internal/cli/root.go

@@ -4,17 +4,38 @@ import (
 	"os"
 
 	"git.eeqj.de/sneak/secret/internal/secret"
+	"github.com/awnumar/memguard"
 	"github.com/spf13/cobra"
+	"golang.org/x/sys/unix"
+	"golang.org/x/term"
 )
 
-// Entry is the entry point for the secret CLI application
+// Entry runs the secret CLI and returns the process exit code. It wipes
-func Entry() {
+// every memguard buffer before it returns, so the caller must do nothing
-	cmd := newRootCmd()
+// but exit with the code.
+func Entry() int {
+	// On SIGINT or SIGTERM memguard runs this function, wipes every buffer
+	// and exits with status 1. The passphrase prompt turns terminal echo
+	// off until the read finishes, so a signal there would leave echo off.
+	// Only a process in the terminal's foreground process group may reset
+	// it: one in the background that tries is stopped instead of exiting.
+	terminalState, terminalErr := term.GetState(unix.Stdin)
 
-	err := cmd.Execute()
+	memguard.CatchSignal(func(os.Signal) {
-	if err != nil {
+		foreground, err := unix.IoctlGetInt(unix.Stdin, unix.TIOCGPGRP)
-		os.Exit(1)
+		if terminalErr == nil && err == nil && foreground == unix.Getpgrp() {
+			_ = term.Restore(unix.Stdin, terminalState)
 		}
+	}, os.Interrupt, unix.SIGTERM)
+
+	defer memguard.Purge()
+
+	err := newRootCmd().Execute()
+	if err != nil {
+		return 1
+	}
+
+	return 0
 }
 
 func newRootCmd() *cobra.Command {

internal/cli/secrets.go

@@ -377,15 +377,6 @@ func (cli *Instance) AddSecret(secretName string, force bool) error {
 	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.
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Add the secret to the vault
 	secret.Debug("Calling vault.AddSecret", "secret_name", secretName,
 		"value_length", valueBuffer.Size(), "force", force)
@@ -644,14 +635,6 @@ func (cli *Instance) ImportSecret(
 	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.
-	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 {
@@ -666,12 +649,6 @@ func (cli *Instance) ImportSecret(
 
 // RemoveSecret removes a secret from the vault
 func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Get current vault
 	currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -706,7 +683,7 @@ func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool)
 	}
 
 	// Remove the secret directory
-	err = secret.RemoveDirAtomic(cli.fs, secretDir)
+	err = cli.fs.RemoveAll(secretDir)
 	if err != nil {
 		return fmt.Errorf("failed to remove secret: %w", err)
 	}
@@ -721,12 +698,6 @@ func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool)
 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()
-
 	// Parse source and destination
 	srcVaultName, srcSecretName, srcQualified := ParseVaultSecretRef(source)
 	destVaultName, destSecretName, destQualified := ParseVaultSecretRef(dest)
@@ -821,7 +792,7 @@ func (cli *Instance) moveSecretWithinVault(
 			return fmt.Errorf("secret '%s' %w", dest, errSecretExistsNoForce)
 		}
 
-		err = secret.RemoveDirAtomic(cli.fs, destDir)
+		err = cli.fs.RemoveAll(destDir)
 		if err != nil {
 			return fmt.Errorf("failed to remove existing destination: %w", err)
 		}
@@ -901,7 +872,7 @@ func (cli *Instance) moveSecretCrossVault(
 	}
 
 	// Delete source secret
-	err = secret.RemoveDirAtomic(cli.fs, srcSecretDir)
+	err = cli.fs.RemoveAll(srcSecretDir)
 	if err != nil {
 		// Copy succeeded but delete failed - warn but don't fail
 		cmd.Printf("Warning: copied secret but failed to remove source: %v\n", err)

internal/cli/unlockers.go

@@ -534,12 +534,6 @@ func (cli *Instance) printUnlockersTable(unlockers []UnlockerInfo) error {
 
 // UnlockersAdd adds a new unlocker
 func (cli *Instance) UnlockersAdd(unlockerType string, cmd *cobra.Command) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	switch unlockerType {
 	case unlockerTypePassphrase:
 		return cli.addPassphraseUnlocker(cmd)
@@ -720,12 +714,6 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error {
 func (cli *Instance) UnlockersRemove(
 	unlockerID string, force bool, cmd *cobra.Command,
 ) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -775,12 +763,6 @@ func (cli *Instance) UnlockersRemove(
 
 // UnlockerSelect selects an unlocker as current
 func (cli *Instance) UnlockerSelect(unlockerID string) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {

internal/cli/vault.go

@@ -267,12 +267,6 @@ func resolvePassphrase() (*memguard.LockedBuffer, error) {
 func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
 	secret.Debug("Creating new vault", "name", name, "state_dir", cli.stateDir)
 
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Get or prompt for mnemonic
 	var mnemonicStr string
 
@@ -360,13 +354,7 @@ func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
 
 // SelectVault selects a vault as the current one
 func (cli *Instance) SelectVault(cmd *cobra.Command, name string) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	err := vault.SelectVault(cli.fs, cli.stateDir, name)
-	if err != nil {
-		return err
-	}
-	defer release()
-
-	err = vault.SelectVault(cli.fs, cli.stateDir, name)
 	if err != nil {
 		return err
 	}
@@ -454,21 +442,8 @@ func updateVaultImportMetadata(
 	return nil
 }
 
-// VaultImport imports a mnemonic into a specific vault, holding the state
+// VaultImport imports a mnemonic into a specific vault
-// directory lock while importMnemonic runs
 func (cli *Instance) VaultImport(cmd *cobra.Command, vaultName string) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
-	return cli.importMnemonic(cmd, vaultName)
-}
-
-// importMnemonic gives the vault a long-term key derived from the mnemonic
-// and a passphrase unlocker
-func (cli *Instance) importMnemonic(cmd *cobra.Command, vaultName string) error {
 	secret.Debug("Importing mnemonic into vault",
 		"vault_name", vaultName, "state_dir", cli.stateDir)
 
@@ -503,7 +478,7 @@ func (cli *Instance) importMnemonic(cmd *cobra.Command, vaultName string) error
 	secret.Debug("Storing long-term public key",
 		"pubkey", ltPublicKey, "vault_dir", vaultDir)
 
-	err = secret.WriteFileAtomic(cli.fs, pubKeyPath, []byte(ltPublicKey))
+	err = afero.WriteFile(cli.fs, pubKeyPath, []byte(ltPublicKey), secret.FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to store long-term public key: %w", err)
 	}
@@ -602,12 +577,6 @@ func (cli *Instance) switchAwayFromVault(
 
 // RemoveVault removes a vault with safety checks
 func (cli *Instance) RemoveVault(cmd *cobra.Command, name string, force bool) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Get list of all vaults
 	vaults, err := vault.ListVaults(cli.fs, cli.stateDir)
 	if err != nil {
@@ -657,7 +626,7 @@ func (cli *Instance) RemoveVault(cmd *cobra.Command, name string, force bool) er
 	}
 
 	// Remove the vault directory
-	err = secret.RemoveDirAtomic(cli.fs, vaultDir)
+	err = cli.fs.RemoveAll(vaultDir)
 	if err != nil {
 		return fmt.Errorf("failed to remove vault directory: %w", err)
 	}

internal/cli/version.go

@@ -239,12 +239,6 @@ func formatVersionTime(t *time.Time) string {
 func (cli *Instance) PromoteVersion(
 	cmd *cobra.Command, secretName string, version string,
 ) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -288,12 +282,6 @@ func (cli *Instance) PromoteVersion(
 func (cli *Instance) RemoveVersion(
 	cmd *cobra.Command, secretName string, version string,
 ) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -345,7 +333,7 @@ func (cli *Instance) RemoveVersion(
 	}
 
 	// Remove the version directory
-	err = secret.RemoveDirAtomic(cli.fs, versionDir)
+	err = cli.fs.RemoveAll(versionDir)
 	if err != nil {
 		return fmt.Errorf("failed to remove version: %w", err)
 	}

internal/secret/atomic.go

@@ -1,86 +0,0 @@
-package secret
-
-import (
-	"fmt"
-	"path/filepath"
-
-	"github.com/spf13/afero"
-)
-
-// WriteFileAtomic replaces the file at path with data so that a reader, or
-// a crash at any moment, finds either the old content or the new, never a
-// partial file. The data goes into a temporary file that afero.TempFile
-// creates with mode 0600 in the same directory (a rename is only atomic
-// within one filesystem), is synced to disk, and is renamed over path. The
-// temporary file is removed if any step fails.
-func WriteFileAtomic(fs afero.Fs, path string, data []byte) error {
-	tmp, err := afero.TempFile(fs, filepath.Dir(path),
-		"."+filepath.Base(path)+".tmp-*")
-	if err != nil {
-		return fmt.Errorf("failed to create temporary file for %s: %w", path, err)
-	}
-
-	_, err = tmp.Write(data)
-	if err == nil {
-		err = tmp.Sync()
-	}
-
-	closeErr := tmp.Close()
-	if err == nil {
-		err = closeErr
-	}
-
-	if err == nil {
-		err = fs.Rename(tmp.Name(), path)
-	}
-
-	if err != nil {
-		_ = fs.Remove(tmp.Name())
-
-		return fmt.Errorf("failed to write %s: %w", path, err)
-	}
-
-	return nil
-}
-
-// TempDirFor creates an empty temporary directory in which to build the
-// directory target before renaming it into place, or into which to move
-// target before deleting it. It is made in target's grandparent: on the
-// same filesystem, so the rename is atomic, and outside target's parent,
-// the directory that is listed to find vaults, secrets, versions and
-// unlockers, so one left behind by a crash is never taken for one of them.
-// Its name leaves out target's, which may already be as long as a file name
-// can be.
-func TempDirFor(fs afero.Fs, target string) (string, error) {
-	dir, err := afero.TempDir(fs, filepath.Dir(filepath.Dir(target)), ".tmp-")
-	if err != nil {
-		return "", fmt.Errorf(
-			"failed to create temporary directory for %s: %w", target, err)
-	}
-
-	return dir, nil
-}
-
-// 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.
-func RemoveDirAtomic(fs afero.Fs, dir string) error {
-	tmp, err := TempDirFor(fs, dir)
-	if err != nil {
-		return err
-	}
-
-	err = fs.Rename(dir, filepath.Join(tmp, filepath.Base(dir)))
-	if err != nil {
-		_ = fs.Remove(tmp)
-
-		return fmt.Errorf("failed to remove %s: %w", dir, err)
-	}
-
-	err = fs.RemoveAll(tmp)
-	if err != nil {
-		return fmt.Errorf("failed to remove %s: %w", dir, err)
-	}
-
-	return nil
-}

internal/secret/atomic_test.go

@@ -1,672 +0,0 @@
-package secret_test
-
-import (
-	"errors"
-	"os"
-	"path/filepath"
-	"strings"
-	"testing"
-
-	"filippo.io/age"
-	"git.eeqj.de/sneak/secret/internal/secret"
-	"git.eeqj.de/sneak/secret/internal/vault"
-	"github.com/awnumar/memguard"
-	"github.com/spf13/afero"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-var errInjected = errors.New("injected failure")
-
-// The kinds of change hookFs passes to before.
-const (
-	opCreate = "create"
-	opOpen   = "open"
-	opSync   = "sync"
-	opMkdir  = "mkdir"
-	opRemove = "remove"
-	opRename = "rename"
-)
-
-// currentFile is the file in a secret's directory that names its current
-// version.
-const currentFile = "current"
-
-// unlockerMetadataFile is the file a new unlocker writes last.
-const unlockerMetadataFile = "unlocker-metadata.json"
-
-// unlockerPassphrase protects the passphrase unlockers the tests create.
-//
-//nolint:gosec // G101: test data, not a real credential
-const unlockerPassphrase = "unlocker passphrase"
-
-// hookFs passes every call through to Fs, but first calls before for each
-// call that changes the filesystem, and for each Sync of a file opened
-// through it, with the path it changes (the new path, for Rename). A test
-// uses before to inspect the tree at every point where a crash could stop
-// the code under test, or returns an error from it to make that call fail.
-// If opened is set, OpenFile also tells it the mode it opens each file with.
-type hookFs struct {
-	afero.Fs
-
-	before func(op, path string) error
-	opened func(path string, perm os.FileMode)
-}
-
-// hookFile is a file opened through hookFs.
-type hookFile struct {
-	afero.File
-
-	before func(op, path string) error
-}
-
-func (f hookFile) Sync() error {
-	err := f.before(opSync, f.Name())
-	if err != nil {
-		return err
-	}
-
-	return f.File.Sync()
-}
-
-//nolint:ireturn // implements afero.Fs
-func (h hookFs) Create(name string) (afero.File, error) {
-	err := h.before(opCreate, name)
-	if err != nil {
-		return nil, err
-	}
-
-	file, err := h.Fs.Create(name)
-	if err != nil {
-		return nil, err
-	}
-
-	return hookFile{File: file, before: h.before}, nil
-}
-
-//nolint:ireturn // implements afero.Fs
-func (h hookFs) OpenFile(
-	name string, flag int, perm os.FileMode,
-) (afero.File, error) {
-	err := h.before(opOpen, name)
-	if err != nil {
-		return nil, err
-	}
-
-	if h.opened != nil {
-		h.opened(name, perm)
-	}
-
-	file, err := h.Fs.OpenFile(name, flag, perm)
-	if err != nil {
-		return nil, err
-	}
-
-	return hookFile{File: file, before: h.before}, nil
-}
-
-func (h hookFs) Mkdir(name string, perm os.FileMode) error {
-	err := h.before(opMkdir, name)
-	if err != nil {
-		return err
-	}
-
-	return h.Fs.Mkdir(name, perm)
-}
-
-func (h hookFs) MkdirAll(path string, perm os.FileMode) error {
-	err := h.before(opMkdir, path)
-	if err != nil {
-		return err
-	}
-
-	return h.Fs.MkdirAll(path, perm)
-}
-
-func (h hookFs) Remove(name string) error {
-	err := h.before(opRemove, name)
-	if err != nil {
-		return err
-	}
-
-	return h.Fs.Remove(name)
-}
-
-func (h hookFs) RemoveAll(path string) error {
-	err := h.before(opRemove, path)
-	if err != nil {
-		return err
-	}
-
-	return h.Fs.RemoveAll(path)
-}
-
-func (h hookFs) Rename(oldname, newname string) error {
-	err := h.before(opRename, newname)
-	if err != nil {
-		return err
-	}
-
-	return h.Fs.Rename(oldname, newname)
-}
-
-// testFilesystem is a filesystem to run a test on, with a directory in it
-// to work in.
-type testFilesystem struct {
-	name string
-	open func(t *testing.T) (afero.Fs, string)
-}
-
-// testFilesystems are the in-memory filesystem that most tests use and the
-// real one: every rename-based guarantee is checked on both.
-//
-//nolint:gochecknoglobals // read-only table shared by the tests below
-var testFilesystems = []testFilesystem{
-	{"memory", func(*testing.T) (afero.Fs, string) {
-		return afero.NewMemMapFs(), "/test"
-	}},
-	{"real", func(t *testing.T) (afero.Fs, string) {
-		t.Helper()
-
-		return afero.NewOsFs(), t.TempDir()
-	}},
-}
-
-// dirNames lists the names in dir.
-func dirNames(t *testing.T, fs afero.Fs, dir string) []string {
-	t.Helper()
-
-	entries, err := afero.ReadDir(fs, dir)
-	require.NoError(t, err)
-
-	names := make([]string, 0, len(entries))
-	for _, entry := range entries {
-		names = append(names, entry.Name())
-	}
-
-	return names
-}
-
-// writeLongTermKey gives the test vault under stateDir a new long-term key
-// and returns it.
-func writeLongTermKey(
-	t *testing.T, fs afero.Fs, stateDir string,
-) *age.X25519Identity {
-	t.Helper()
-
-	vault := &MockVersionVault{Name: testVaultName, fs: fs, stateDir: stateDir}
-
-	vaultDir, err := vault.GetDirectory()
-	require.NoError(t, err)
-	require.NoError(t, fs.MkdirAll(vaultDir, 0o700))
-
-	ltIdentity, err := age.GenerateX25519Identity()
-	require.NoError(t, err)
-	require.NoError(t, afero.WriteFile(fs, filepath.Join(vaultDir, "pub.age"),
-		[]byte(ltIdentity.Recipient().String()), 0o600))
-
-	return ltIdentity
-}
-
-// newVaultWithSecret creates the vault name under stateDir from the test
-// mnemonic, with a secret "shared" in it that holds value.
-func newVaultWithSecret(
-	t *testing.T, fs afero.Fs, stateDir, name, value string,
-) *vault.Vault {
-	t.Helper()
-
-	vlt, err := vault.CreateVault(fs, stateDir, name)
-	require.NoError(t, err)
-
-	buffer := memguard.NewBufferFromBytes([]byte(value))
-	defer buffer.Destroy()
-
-	require.NoError(t, vlt.AddSecret("shared", buffer, false))
-
-	return vlt
-}
-
-func TestWriteFileAtomicReplacesFile(t *testing.T) {
-	t.Parallel()
-
-	for _, tfs := range testFilesystems {
-		t.Run(tfs.name, func(t *testing.T) {
-			t.Parallel()
-
-			fs, dir := tfs.open(t)
-			path := filepath.Join(dir, currentFile)
-
-			require.NoError(t, secret.WriteFileAtomic(fs, path, []byte("old")))
-			require.NoError(t, secret.WriteFileAtomic(fs, path, []byte("new")))
-
-			data, err := afero.ReadFile(fs, path)
-			require.NoError(t, err)
-			assert.Equal(t, "new", string(data))
-
-			info, err := fs.Stat(path)
-			require.NoError(t, err)
-			assert.Equal(t, secret.FilePerms, info.Mode().Perm())
-
-			// No temporary file is left next to it
-			assert.Equal(t, []string{currentFile}, dirNames(t, fs, dir))
-		})
-	}
-}
-
-func TestWriteFileAtomicFailureKeepsOldFile(t *testing.T) {
-	t.Parallel()
-
-	for _, tfs := range testFilesystems {
-		t.Run(tfs.name, func(t *testing.T) {
-			t.Parallel()
-
-			base, dir := tfs.open(t)
-			path := filepath.Join(dir, currentFile)
-			require.NoError(t, secret.WriteFileAtomic(base, path, []byte("old")))
-
-			fs := hookFs{Fs: base, before: func(op, _ string) error {
-				if op == opRename {
-					return errInjected
-				}
-
-				return nil
-			}}
-
-			err := secret.WriteFileAtomic(fs, path, []byte("new"))
-			require.ErrorIs(t, err, errInjected)
-
-			data, err := afero.ReadFile(base, path)
-			require.NoError(t, err)
-			assert.Equal(t, "old", string(data))
-
-			// The temporary file is removed again
-			assert.Equal(t, []string{currentFile}, dirNames(t, base, dir))
-		})
-	}
-}
-
-// TestRemoveDirAtomic checks that RemoveDirAtomic deletes nothing where the
-// directory stands, which a crash could stop half-way, and that it leaves
-// nothing behind.
-func TestRemoveDirAtomic(t *testing.T) {
-	t.Parallel()
-
-	for _, tfs := range testFilesystems {
-		t.Run(tfs.name, func(t *testing.T) {
-			t.Parallel()
-
-			base, dir := tfs.open(t)
-			listed := filepath.Join(dir, "secrets.d")
-			target := filepath.Join(listed, "doomed")
-
-			require.NoError(t, base.MkdirAll(filepath.Join(target, "versions"), 0o700))
-			require.NoError(t, secret.WriteFileAtomic(base,
-				filepath.Join(target, currentFile), []byte("20231216.001")))
-
-			fs := hookFs{Fs: base, before: func(op, path string) error {
-				if op == opRemove && strings.HasPrefix(path, target) {
-					t.Errorf("deleted %s where it stands", path)
-				}
-
-				return nil
-			}}
-
-			require.NoError(t, secret.RemoveDirAtomic(fs, target))
-
-			// Gone, and no temporary directory is left in the directory
-			// that is listed or in the one above it
-			assert.Empty(t, dirNames(t, base, listed))
-			assert.Equal(t, []string{"secrets.d"}, dirNames(t, base, dir))
-		})
-	}
-}
-
-// TestLongestNames adds a secret to a vault and removes the vault, both
-// named with 255 bytes, the most a file name may have, on the real
-// filesystem: the temporary directories they use must fit that limit too.
-func TestLongestNames(t *testing.T) {
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-
-	const longestName = 255
-
-	fs := afero.NewOsFs()
-	name := strings.Repeat("a", longestName)
-
-	vlt, err := vault.CreateVault(fs, t.TempDir(), name)
-	require.NoError(t, err)
-
-	value := memguard.NewBufferFromBytes([]byte("long"))
-	defer value.Destroy()
-
-	require.NoError(t, vlt.AddSecret(name, value, false))
-
-	got, err := vlt.GetSecret(name)
-	require.NoError(t, err)
-	assert.Equal(t, "long", string(got))
-
-	vaultDir, err := vlt.GetDirectory()
-	require.NoError(t, err)
-	require.NoError(t, secret.RemoveDirAtomic(fs, vaultDir))
-	assert.NoDirExists(t, vaultDir)
-}
-
-// TestForcedCopyKeepsDestinationUntilReplaced copies a secret over one in
-// another vault, as a forced move between vaults does, and makes the last
-// step that completes the copy fail. The secret it was to replace must
-// still be there unchanged: it may go only once its replacement is whole.
-//
-//nolint:paralleltest // t.Setenv forbids t.Parallel
-func TestForcedCopyKeepsDestinationUntilReplaced(t *testing.T) {
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-
-	for _, tfs := range testFilesystems {
-		t.Run(tfs.name, func(t *testing.T) {
-			base, stateDir := tfs.open(t)
-			src := newVaultWithSecret(t, base, stateDir, "source", "new")
-			dest := newVaultWithSecret(t, base, stateDir, "dest", "old")
-
-			// The copy is complete once its current file is written
-			fs := hookFs{Fs: base, before: func(op, path string) error {
-				if op == opRename && filepath.Base(path) == currentFile {
-					return errInjected
-				}
-
-				return nil
-			}}
-
-			err := vault.NewVault(fs, stateDir, "dest").
-				CopySecretAllVersions(src, "shared", "shared", true)
-			require.ErrorIs(t, err, errInjected)
-
-			value, err := dest.GetSecret("shared")
-			require.NoError(t, err)
-			assert.Equal(t, "old", string(value))
-		})
-	}
-}
-
-// TestTempDirsStayOutOfListings adds a version, adds a secret, copies a
-// secret over another and removes one, and checks that none of them makes a
-// directory directly in secrets.d or in a versions directory. Those are
-// listed to find secrets and versions, so a temporary directory made there
-// would be listed while half-built, and one left by a crash would stay.
-//
-//nolint:paralleltest // t.Setenv forbids t.Parallel
-func TestTempDirsStayOutOfListings(t *testing.T) {
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-
-	for _, tfs := range testFilesystems {
-		t.Run(tfs.name, func(t *testing.T) {
-			base, stateDir := tfs.open(t)
-			newVaultWithSecret(t, base, stateDir, "default", "first")
-
-			fs := hookFs{Fs: base, before: func(op, path string) error {
-				parent := filepath.Base(filepath.Dir(path))
-				if op == opMkdir && (parent == "secrets.d" || parent == "versions") {
-					t.Errorf("made %s where it is listed", path)
-				}
-
-				return nil
-			}}
-			vlt := vault.NewVault(fs, stateDir, "default")
-
-			value := memguard.NewBufferFromBytes([]byte("second"))
-			defer value.Destroy()
-
-			require.NoError(t, vlt.AddSecret("shared", value, true))
-			require.NoError(t, vlt.AddSecret("other", value, false))
-			require.NoError(t, vlt.CopySecretAllVersions(vlt, "shared", "other", true))
-
-			vaultDir, err := vlt.GetDirectory()
-			require.NoError(t, err)
-			require.NoError(t, secret.RemoveDirAtomic(fs,
-				filepath.Join(vaultDir, "secrets.d", "shared")))
-		})
-	}
-}
-
-// TestVersionSaveIsWholeOrAbsent checks, before every change Save makes and
-// once after it returns, that the version directory either does not exist
-// or holds all of its files: a crash at any point leaves no version that
-// cannot be decrypted.
-func TestVersionSaveIsWholeOrAbsent(t *testing.T) {
-	t.Parallel()
-
-	for _, tfs := range testFilesystems {
-		t.Run(tfs.name, func(t *testing.T) {
-			t.Parallel()
-
-			base, stateDir := tfs.open(t)
-			ltIdentity := writeLongTermKey(t, base, stateDir)
-
-			var versionDir string
-
-			checkVersionDir := func(string, string) error {
-				exists, err := afero.DirExists(base, versionDir)
-				require.NoError(t, err)
-
-				if exists {
-					assert.ElementsMatch(t,
-						[]string{"pub.age", "value.age", "priv.age", "metadata.age"},
-						dirNames(t, base, versionDir),
-						"version directory visible before it was complete")
-				}
-
-				return nil
-			}
-
-			fs := hookFs{Fs: base, before: checkVersionDir}
-			vault := &MockVersionVault{Name: testVaultName, fs: fs, stateDir: stateDir}
-			sv := secret.NewVersion(vault, "test/secret", "20231215.001")
-			versionDir = sv.Directory
-
-			value := memguard.NewBufferFromBytes([]byte("whole or nothing"))
-			defer value.Destroy()
-
-			require.NoError(t, sv.Save(value))
-			require.NoError(t, checkVersionDir("", ""))
-
-			got, err := sv.GetValue(ltIdentity)
-			require.NoError(t, err)
-
-			defer got.Destroy()
-
-			assert.Equal(t, "whole or nothing", got.String())
-		})
-	}
-}
-
-// TestVersionSaveFailureLeavesNothing makes the write of the encrypted
-// private key fail, after the value has been written, and checks that
-// neither the version nor its temporary directory is left behind.
-func TestVersionSaveFailureLeavesNothing(t *testing.T) {
-	t.Parallel()
-
-	for _, tfs := range testFilesystems {
-		t.Run(tfs.name, func(t *testing.T) {
-			t.Parallel()
-
-			base, stateDir := tfs.open(t)
-			writeLongTermKey(t, base, stateDir)
-
-			fs := hookFs{Fs: base, before: func(op, path string) error {
-				if op == opRename && filepath.Base(path) == "priv.age" {
-					return errInjected
-				}
-
-				return nil
-			}}
-			vault := &MockVersionVault{Name: testVaultName, fs: fs, stateDir: stateDir}
-			sv := secret.NewVersion(vault, "test/secret", "20231215.001")
-
-			value := memguard.NewBufferFromBytes([]byte("never stored"))
-			defer value.Destroy()
-
-			require.ErrorIs(t, sv.Save(value), errInjected)
-
-			// The secret directory holds only the empty versions directory
-			versionsDir := filepath.Dir(sv.Directory)
-			assert.Equal(t, []string{"versions"},
-				dirNames(t, base, filepath.Dir(versionsDir)))
-			assert.Empty(t, dirNames(t, base, versionsDir))
-		})
-	}
-}
-
-// TestCurrentFilesNeverMissing selects the current version, vault and
-// unlocker again and checks, before each change this makes, that the file
-// naming the current one exists: a reader or a crash never finds it
-// missing.
-//
-//nolint:paralleltest // t.Setenv forbids t.Parallel
-func TestCurrentFilesNeverMissing(t *testing.T) {
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-
-	for _, tfs := range testFilesystems {
-		t.Run(tfs.name, func(t *testing.T) {
-			base, stateDir := tfs.open(t)
-			vlt := newVaultWithSecret(t, base, stateDir, testVaultName, "value")
-
-			passphrase := memguard.NewBufferFromBytes([]byte(unlockerPassphrase))
-			defer passphrase.Destroy()
-
-			// Created as the current unlocker
-			unlocker, err := vlt.CreatePassphraseUnlocker(passphrase)
-			require.NoError(t, err)
-
-			vaultDir, err := vlt.GetDirectory()
-			require.NoError(t, err)
-
-			secretDir := filepath.Join(vaultDir, "secrets.d", "shared")
-			version, err := secret.GetCurrentVersion(base, secretDir)
-			require.NoError(t, err)
-
-			for _, tc := range []struct {
-				path     string
-				reselect func(fs afero.Fs) error
-			}{
-				{filepath.Join(secretDir, currentFile), func(fs afero.Fs) error {
-					return secret.SetCurrentVersion(fs, secretDir, version)
-				}},
-				{filepath.Join(stateDir, "currentvault"), func(fs afero.Fs) error {
-					return vault.SelectVault(fs, stateDir, testVaultName)
-				}},
-				{filepath.Join(vaultDir, "current-unlocker"), func(fs afero.Fs) error {
-					return vault.NewVault(fs, stateDir, testVaultName).
-						SelectUnlocker(unlocker.GetID())
-				}},
-			} {
-				fs := hookFs{Fs: base, before: func(string, string) error {
-					exists, err := afero.Exists(base, tc.path)
-					require.NoError(t, err)
-					assert.True(t, exists, "%s is missing", filepath.Base(tc.path))
-
-					return nil
-				}}
-
-				require.NoError(t, tc.reselect(fs))
-			}
-		})
-	}
-}
-
-// TestWriteFileAtomicTempFile checks that WriteFileAtomic creates its
-// temporary file with mode 0600, rather than wider and narrowed later, so
-// that no other user can ever read it, and syncs it before renaming it into
-// place, so that a crash cannot leave the file named but its data lost.
-func TestWriteFileAtomicTempFile(t *testing.T) {
-	t.Parallel()
-
-	for _, tfs := range testFilesystems {
-		t.Run(tfs.name, func(t *testing.T) {
-			t.Parallel()
-
-			base, dir := tfs.open(t)
-
-			var modes []os.FileMode
-
-			synced := false
-			fs := hookFs{
-				Fs: base,
-				before: func(op, _ string) error {
-					switch op {
-					case opSync:
-						synced = true
-					case opRename:
-						assert.True(t, synced, "renamed before syncing")
-					}
-
-					return nil
-				},
-				opened: func(_ string, perm os.FileMode) {
-					modes = append(modes, perm)
-				},
-			}
-
-			require.NoError(t, secret.WriteFileAtomic(fs,
-				filepath.Join(dir, currentFile), []byte("new")))
-			assert.Equal(t, []os.FileMode{secret.FilePerms}, modes)
-		})
-	}
-}
-
-// TestPassphraseUnlockerGetsKeyFirst creates a passphrase unlocker in a
-// vault whose long-term key cannot be had: it must fail without writing
-// anything, so that it never leaves a partial unlocker, nor breaks the one
-// it would replace.
-func TestPassphraseUnlockerGetsKeyFirst(t *testing.T) {
-	// No mnemonic, and no current unlocker to get the key from
-	t.Setenv(secret.EnvMnemonic, "")
-
-	base := afero.NewMemMapFs()
-	_, err := vault.CreateVault(base, testVaultStateDir, testVaultName)
-	require.NoError(t, err)
-
-	fs := hookFs{Fs: base, before: func(_, path string) error {
-		t.Errorf("changed %s before getting the long-term key", path)
-
-		return nil
-	}}
-
-	passphrase := memguard.NewBufferFromBytes([]byte(unlockerPassphrase))
-	defer passphrase.Destroy()
-
-	_, err = vault.NewVault(fs, testVaultStateDir, testVaultName).
-		CreatePassphraseUnlocker(passphrase)
-	require.Error(t, err)
-}
-
-// TestPassphraseUnlockerWritesMetadataLast checks that the last file a new
-// passphrase unlocker writes in its directory is its metadata: an unlocker
-// directory without metadata is never used, so one interrupted earlier
-// cannot be.
-func TestPassphraseUnlockerWritesMetadataLast(t *testing.T) {
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-
-	base := afero.NewMemMapFs()
-	vlt, err := vault.CreateVault(base, testVaultStateDir, testVaultName)
-	require.NoError(t, err)
-
-	vaultDir, err := vlt.GetDirectory()
-	require.NoError(t, err)
-
-	unlockerDir := filepath.Join(vaultDir, "unlockers.d", "passphrase")
-
-	var last string
-
-	fs := hookFs{Fs: base, before: func(_, path string) error {
-		if filepath.Dir(path) == unlockerDir {
-			last = filepath.Base(path)
-		}
-
-		return nil
-	}}
-
-	passphrase := memguard.NewBufferFromBytes([]byte(unlockerPassphrase))
-	defer passphrase.Destroy()
-
-	_, err = vault.NewVault(fs, testVaultStateDir, testVaultName).
-		CreatePassphraseUnlocker(passphrase)
-	require.NoError(t, err)
-	assert.Equal(t, unlockerMetadataFile, last)
-}

internal/secret/helpers_darwin.go

@@ -0,0 +1,29 @@
+//go:build darwin
+
+package secret
+
+import (
+	"crypto/rand"
+	"fmt"
+	"math/big"
+)
+
+// generateRandomString generates a random string of the specified length using the given character set
+func generateRandomString(length int, charset string) (string, error) {
+	if length <= 0 {
+		return "", fmt.Errorf("length must be positive")
+	}
+
+	result := make([]byte, length)
+	charsetLen := big.NewInt(int64(len(charset)))
+
+	for i := range length {
+		randomIndex, err := rand.Int(rand.Reader, charsetLen)
+		if err != nil {
+			return "", fmt.Errorf("failed to generate random number: %w", err)
+		}
+		result[i] = charset[randomIndex.Int64()]
+	}
+
+	return string(result), nil
+}

internal/secret/keychaindata.go

@@ -1,142 +0,0 @@
-package secret
-
-import (
-	"bytes"
-	"encoding/hex"
-	"encoding/json"
-	"errors"
-	"fmt"
-	"strings"
-
-	"github.com/awnumar/memguard"
-)
-
-var (
-	errPassphraseLength = errors.New(
-		"passphrase length must be a positive even number")
-	errPassphraseNotHex = errors.New(
-		"keychain passphrase must be lowercase hex")
-	errNoKeychainPassphrase = errors.New(
-		"keychain data has no agePrivKeyPassphrase string")
-)
-
-// KeychainData is what a keychain unlocker stores in the macOS keychain.
-// It is stored as JSON, but encode and decodeKeychainData keep the
-// passphrase out of encoding/json, which would leave copies of it in
-// ordinary memory.
-type KeychainData struct {
-	AgePublicKey         string
-	AgePrivKeyPassphrase *memguard.LockedBuffer
-	EncryptedLongtermKey string
-}
-
-// generateRandomPassphrase returns length random lowercase hex characters
-// in a locked buffer. The caller must destroy it.
-func generateRandomPassphrase(length int) (*memguard.LockedBuffer, error) {
-	// Each random byte becomes two hex characters.
-	randomBytes := hex.DecodedLen(length)
-	if length <= 0 || hex.EncodedLen(randomBytes) != length {
-		return nil, errPassphraseLength
-	}
-
-	random := memguard.NewBufferRandom(randomBytes)
-	defer random.Destroy()
-
-	passphrase := memguard.NewBuffer(length)
-	hex.Encode(passphrase.Bytes(), random.Bytes())
-	passphrase.Freeze()
-
-	return passphrase, nil
-}
-
-// encode returns d as JSON in a locked buffer:
-// {"agePublicKey":"...","agePrivKeyPassphrase":"...","encryptedLongtermKey":"..."}.
-// The passphrase is copied straight into the buffer, so it must be hex,
-// which JSON does not escape. The caller must destroy the returned buffer.
-func (d *KeychainData) encode() (*memguard.LockedBuffer, error) {
-	if d.AgePrivKeyPassphrase == nil {
-		return nil, errNilPassphraseBuffer
-	}
-
-	if d.AgePrivKeyPassphrase.Size() == 0 {
-		return nil, errEmptyPassphrase
-	}
-
-	for _, c := range d.AgePrivKeyPassphrase.Bytes() {
-		if strings.IndexByte("0123456789abcdef", c) < 0 {
-			return nil, errPassphraseNotHex
-		}
-	}
-
-	publicKey, err := json.Marshal(d.AgePublicKey)
-	if err != nil {
-		return nil, fmt.Errorf("failed to encode age public key: %w", err)
-	}
-
-	longtermKey, err := json.Marshal(d.EncryptedLongtermKey)
-	if err != nil {
-		return nil, fmt.Errorf("failed to encode long-term key: %w", err)
-	}
-
-	parts := [][]byte{
-		[]byte(`{"agePublicKey":`), publicKey,
-		[]byte(`,"agePrivKeyPassphrase":"`), d.AgePrivKeyPassphrase.Bytes(),
-		[]byte(`","encryptedLongtermKey":`), longtermKey,
-		[]byte(`}`),
-	}
-
-	size := 0
-	for _, part := range parts {
-		size += len(part)
-	}
-
-	encoded := memguard.NewBuffer(size)
-
-	written := 0
-	for _, part := range parts {
-		written += copy(encoded.Bytes()[written:], part)
-	}
-
-	encoded.Freeze()
-
-	return encoded, nil
-}
-
-// decodeKeychainData parses keychain data written by encode. The caller
-// must destroy the returned AgePrivKeyPassphrase.
-func decodeKeychainData(data *memguard.LockedBuffer) (*KeychainData, error) {
-	if data == nil {
-		return nil, errNilDataBuffer
-	}
-
-	// json.Unmarshal gives a json.RawMessage field the field's JSON text
-	// unchanged, in the one copy RawMessage makes; it is wiped on return.
-	var fields struct {
-		AgePublicKey         string          `json:"agePublicKey"`
-		AgePrivKeyPassphrase json.RawMessage `json:"agePrivKeyPassphrase"`
-		EncryptedLongtermKey string          `json:"encryptedLongtermKey"`
-	}
-
-	defer func() { memguard.WipeBytes(fields.AgePrivKeyPassphrase) }()
-
-	err := json.Unmarshal(data.Bytes(), &fields)
-	if err != nil {
-		return nil, fmt.Errorf("failed to parse keychain data: %w", err)
-	}
-
-	// json.Unmarshal accepted the JSON, so text that starts with a quote is
-	// a whole string. The passphrase is hex, so it is the text between the
-	// quotes.
-	quoted := fields.AgePrivKeyPassphrase
-	if !bytes.HasPrefix(quoted, []byte(`"`)) {
-		return nil, errNoKeychainPassphrase
-	}
-
-	return &KeychainData{
-		AgePublicKey: fields.AgePublicKey,
-		// NewBufferFromBytes wipes the bytes it copies.
-		AgePrivKeyPassphrase: memguard.NewBufferFromBytes(
-			quoted[1 : len(quoted)-1]),
-		EncryptedLongtermKey: fields.EncryptedLongtermKey,
-	}, nil
-}

internal/secret/keychaindata_test.go

@@ -1,118 +0,0 @@
-//nolint:testpackage // white-box test of unexported internals
-package secret
-
-import (
-	"encoding/json"
-	"testing"
-
-	"github.com/awnumar/memguard"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-func TestGenerateRandomPassphrase(t *testing.T) {
-	t.Parallel()
-
-	first, err := generateRandomPassphrase(64)
-	require.NoError(t, err)
-
-	defer first.Destroy()
-
-	second, err := generateRandomPassphrase(64)
-	require.NoError(t, err)
-
-	defer second.Destroy()
-
-	assert.Regexp(t, `^[0-9a-f]{64}$`, first.String())
-	assert.NotEqual(t, first.String(), second.String())
-	assert.False(t, first.IsMutable())
-
-	for _, length := range []int{0, -2, 63} {
-		_, err := generateRandomPassphrase(length)
-		require.ErrorIs(t, err, errPassphraseLength, "length %d", length)
-	}
-}
-
-func TestKeychainDataEncodeDecode(t *testing.T) {
-	t.Parallel()
-
-	passphrase := memguard.NewBufferFromBytes([]byte("0a1b2c3d"))
-	defer passphrase.Destroy()
-
-	data := KeychainData{
-		AgePublicKey:         "age1example",
-		AgePrivKeyPassphrase: passphrase,
-		EncryptedLongtermKey: "beef",
-	}
-
-	encoded, err := data.encode()
-	require.NoError(t, err)
-
-	defer encoded.Destroy()
-
-	assert.JSONEq(t,
-		`{"agePublicKey":"age1example",`+
-			`"agePrivKeyPassphrase":"0a1b2c3d",`+
-			`"encryptedLongtermKey":"beef"}`,
-		encoded.String())
-	assert.False(t, encoded.IsMutable())
-
-	decoded, err := decodeKeychainData(encoded)
-	require.NoError(t, err)
-
-	defer decoded.AgePrivKeyPassphrase.Destroy()
-
-	assert.Equal(t, "age1example", decoded.AgePublicKey)
-	assert.Equal(t, "0a1b2c3d", decoded.AgePrivKeyPassphrase.String())
-	assert.Equal(t, "beef", decoded.EncryptedLongtermKey)
-}
-
-func TestKeychainDataEncodeRejectsBadPassphrase(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		name       string
-		passphrase *memguard.LockedBuffer
-		wantErr    error
-	}{
-		{"nil", nil, errNilPassphraseBuffer},
-		{"empty", memguard.NewBuffer(0), errEmptyPassphrase},
-		{
-			"not hex",
-			memguard.NewBufferFromBytes([]byte(`abc"def`)),
-			errPassphraseNotHex,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-
-			data := KeychainData{AgePrivKeyPassphrase: tt.passphrase}
-			_, err := data.encode()
-			require.ErrorIs(t, err, tt.wantErr)
-		})
-	}
-}
-
-func TestDecodeKeychainDataRejectsBadData(t *testing.T) {
-	t.Parallel()
-
-	for _, text := range []string{
-		`{"agePublicKey":"age1example"}`,
-		`{"agePrivKeyPassphrase":42}`,
-	} {
-		data := memguard.NewBufferFromBytes([]byte(text))
-		_, err := decodeKeychainData(data)
-		data.Destroy()
-		require.ErrorIs(t, err, errNoKeychainPassphrase, text)
-	}
-
-	notJSON := memguard.NewBufferFromBytes([]byte(`{"agePrivKeyPassphrase":`))
-	defer notJSON.Destroy()
-
-	_, err := decodeKeychainData(notJSON)
-
-	var syntaxError *json.SyntaxError
-	require.ErrorAs(t, err, &syntaxError)
-}

internal/secret/keychainunlocker.go

@@ -45,6 +45,13 @@ type KeychainUnlocker struct {
 	fs        afero.Fs
 }
 
+// KeychainData represents the data stored in the macOS keychain
+type KeychainData struct {
+	AgePublicKey         string `json:"agePublicKey"`
+	AgePrivKeyPassphrase string `json:"agePrivKeyPassphrase"`
+	EncryptedLongtermKey string `json:"encryptedLongtermKey"`
+}
+
 // GetIdentity implements Unlocker interface for Keychain-based unlockers
 func (k *KeychainUnlocker) GetIdentity() (*age.X25519Identity, error) {
 	DebugWith("Getting keychain unlocker identity",
@@ -74,18 +81,13 @@ func (k *KeychainUnlocker) GetIdentity() (*age.X25519Identity, error) {
 		slog.Int("data_length", len(keychainDataBytes)),
 	)
 
-	// Move the keychain data into locked memory; this wipes keychainDataBytes
-	keychainDataBuffer := memguard.NewBufferFromBytes(keychainDataBytes)
-	defer keychainDataBuffer.Destroy()
-
 	// Step 3: Parse keychain data
-	keychainData, err := decodeKeychainData(keychainDataBuffer)
+	var keychainData KeychainData
-	if err != nil {
+	if err := json.Unmarshal(keychainDataBytes, &keychainData); err != nil {
 		Debug("Failed to parse keychain data", "error", err, "unlocker_id", k.GetID())
 
 		return nil, fmt.Errorf("failed to parse keychain data: %w", err)
 	}
-	defer keychainData.AgePrivKeyPassphrase.Destroy()
 
 	Debug("Parsed keychain data successfully", "unlocker_id", k.GetID())
 
@@ -107,7 +109,11 @@ func (k *KeychainUnlocker) GetIdentity() (*age.X25519Identity, error) {
 
 	// Step 5: Decrypt the age private key using the passphrase from keychain
 	Debug("Decrypting age private key with keychain passphrase", "unlocker_id", k.GetID())
-	agePrivKeyBuffer, err := DecryptWithPassphrase(encryptedAgePrivKeyData, keychainData.AgePrivKeyPassphrase)
+	// Create secure buffer for the keychain passphrase
+	passphraseBuffer := memguard.NewBufferFromBytes([]byte(keychainData.AgePrivKeyPassphrase))
+	defer passphraseBuffer.Destroy()
+
+	agePrivKeyBuffer, err := DecryptWithPassphrase(encryptedAgePrivKeyData, passphraseBuffer)
 	if err != nil {
 		Debug("Failed to decrypt age private key with keychain passphrase", "error", err, "unlocker_id", k.GetID())
 
@@ -189,7 +195,7 @@ func (k *KeychainUnlocker) Remove() error {
 
 	// Step 3: Remove directory
 	Debug("Removing keychain unlocker directory", "directory", k.Directory)
-	if err := RemoveDirAtomic(k.fs, k.Directory); err != nil {
+	if err := k.fs.RemoveAll(k.Directory); err != nil {
 		Debug("Failed to remove keychain unlocker directory", "error", err, "directory", k.Directory)
 
 		return fmt.Errorf("failed to remove keychain unlocker directory: %w", err)
@@ -363,28 +369,30 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 	if err != nil {
 		return nil, fmt.Errorf("failed to generate age private key passphrase: %w", err)
 	}
-	defer agePrivKeyPassphrase.Destroy()
 
 	// Step 3: Store age recipient as plaintext
 	ageRecipient := ageIdentity.Recipient().String()
 	recipientPath := filepath.Join(unlockerDir, "pub.txt")
-	if err := WriteFileAtomic(fs, recipientPath, []byte(ageRecipient)); err != nil {
+	if err := afero.WriteFile(fs, recipientPath, []byte(ageRecipient), FilePerms); err != nil {
 		return nil, fmt.Errorf("failed to write age recipient: %w", err)
 	}
 
 	// Step 4: Encrypt age private key with the generated passphrase and store on disk
-	// Create a secure buffer for the private key
+	// Create secure buffers for both the private key and passphrase
 	agePrivKeyStr := ageIdentity.String()
 	agePrivKeyBuffer := memguard.NewBufferFromBytes([]byte(agePrivKeyStr))
 	defer agePrivKeyBuffer.Destroy()
 
-	encryptedAgePrivKey, err := EncryptWithPassphrase(agePrivKeyBuffer, agePrivKeyPassphrase)
+	passphraseBuffer := memguard.NewBufferFromBytes([]byte(agePrivKeyPassphrase))
+	defer passphraseBuffer.Destroy()
+
+	encryptedAgePrivKey, err := EncryptWithPassphrase(agePrivKeyBuffer, passphraseBuffer)
 	if err != nil {
 		return nil, fmt.Errorf("failed to encrypt age private key with passphrase: %w", err)
 	}
 
 	agePrivKeyPath := filepath.Join(unlockerDir, "priv.age")
-	if err := WriteFileAtomic(fs, agePrivKeyPath, encryptedAgePrivKey); err != nil {
+	if err := afero.WriteFile(fs, agePrivKeyPath, encryptedAgePrivKey, FilePerms); err != nil {
 		return nil, fmt.Errorf("failed to write encrypted age private key: %w", err)
 	}
 
@@ -403,7 +411,7 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 
 	// Write encrypted long-term private key
 	ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age")
-	if err := WriteFileAtomic(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge); err != nil {
+	if err := afero.WriteFile(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge, FilePerms); err != nil {
 		return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err)
 	}
 
@@ -414,10 +422,13 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 		EncryptedLongtermKey: hex.EncodeToString(encryptedLtPrivKeyToAge),
 	}
 
-	keychainDataBuffer, err := keychainData.encode()
+	keychainDataBytes, err := json.Marshal(keychainData)
 	if err != nil {
-		return nil, fmt.Errorf("failed to encode keychain data: %w", err)
+		return nil, fmt.Errorf("failed to marshal keychain data: %w", err)
 	}
+
+	// Create a secure buffer for keychain data
+	keychainDataBuffer := memguard.NewBufferFromBytes(keychainDataBytes)
 	defer keychainDataBuffer.Destroy()
 
 	// Step 8: Store data in keychain
@@ -440,9 +451,9 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 		return nil, fmt.Errorf("failed to marshal unlocker metadata: %w", err)
 	}
 
-	if err := WriteFileAtomic(fs,
+	if err := afero.WriteFile(fs,
 		filepath.Join(unlockerDir, "unlocker-metadata.json"),
-		metadataBytes); err != nil {
+		metadataBytes, FilePerms); err != nil {
 		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
 	}
 
@@ -490,7 +501,7 @@ func storeInKeychain(itemName string, data *memguard.LockedBuffer) error {
 	item.SetAccount(itemName)
 	item.SetLabel(fmt.Sprintf("%s - %s", KEYCHAIN_APP_IDENTIFIER, itemName))
 	item.SetDescription("Secret vault keychain data")
-	item.SetData(data.Bytes())
+	item.SetData([]byte(data.String()))
 	item.SetSynchronizable(keychain.SynchronizableNo)
 	// Use AccessibleWhenUnlockedThisDeviceOnly for better security and to trigger auth
 	item.SetAccessible(keychain.AccessibleWhenUnlockedThisDeviceOnly)
@@ -565,3 +576,8 @@ func deleteFromKeychain(itemName string) error {
 
 	return nil
 }
+
+// generateRandomPassphrase generates a random passphrase for encrypting the age private key
+func generateRandomPassphrase(length int) (string, error) {
+	return generateRandomString(length, "0123456789abcdef")
+}

internal/secret/passphraseunlocker.go

@@ -127,7 +127,7 @@ func (p *PassphraseUnlocker) Remove() error {
 
 	// For passphrase unlockers, we just need to remove the directory
 	// No external resources (like keychain items) to clean up
-	err := RemoveDirAtomic(p.fs, p.Directory)
+	err := p.fs.RemoveAll(p.Directory)
 	if err != nil {
 		return fmt.Errorf("failed to remove passphrase unlocker directory: %w", err)
 	}

internal/secret/pgpunlocker.go

@@ -172,7 +172,7 @@ func (p *PGPUnlocker) GetID() string {
 func (p *PGPUnlocker) Remove() error {
 	// For PGP unlockers, we just need to remove the directory
 	// No external resources (like keychain items) to clean up
-	err := RemoveDirAtomic(p.fs, p.Directory)
+	err := p.fs.RemoveAll(p.Directory)
 	if err != nil {
 		return fmt.Errorf("failed to remove PGP unlocker directory: %w", err)
 	}
@@ -275,7 +275,7 @@ func CreatePGPUnlocker(
 	ageRecipient := ageIdentity.Recipient().String()
 	recipientPath := filepath.Join(unlockerDir, "pub.txt")
 
-	err = WriteFileAtomic(fs, recipientPath, []byte(ageRecipient))
+	err = afero.WriteFile(fs, recipientPath, []byte(ageRecipient), FilePerms)
 	if err != nil {
 		return nil, fmt.Errorf("failed to write age recipient: %w", err)
 	}
@@ -298,7 +298,7 @@ func CreatePGPUnlocker(
 	// Write encrypted long-term private key
 	ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age")
 
-	err = WriteFileAtomic(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge)
+	err = afero.WriteFile(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge, FilePerms)
 	if err != nil {
 		return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err)
 	}
@@ -315,7 +315,7 @@ func CreatePGPUnlocker(
 
 	agePrivKeyPath := filepath.Join(unlockerDir, "priv.age.gpg")
 
-	err = WriteFileAtomic(fs, agePrivKeyPath, encryptedAgePrivKey)
+	err = afero.WriteFile(fs, agePrivKeyPath, encryptedAgePrivKey, FilePerms)
 	if err != nil {
 		return nil, fmt.Errorf("failed to write encrypted age private key: %w", err)
 	}
@@ -357,8 +357,9 @@ func writePGPUnlockerMetadata(
 		return nil, fmt.Errorf("failed to marshal unlocker metadata: %w", err)
 	}
 
-	err = WriteFileAtomic(fs,
+	err = afero.WriteFile(fs,
-		filepath.Join(unlockerDir, "unlocker-metadata.json"), metadataBytes)
+		filepath.Join(unlockerDir, "unlocker-metadata.json"),
+		metadataBytes, FilePerms)
 	if err != nil {
 		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
 	}

internal/secret/seunlocker_darwin.go

@@ -148,7 +148,7 @@ func (s *SecureEnclaveUnlocker) Remove() error {
 	}
 
 	Debug("Removing SE unlocker directory", "directory", s.Directory)
-	if err := RemoveDirAtomic(s.fs, s.Directory); err != nil {
+	if err := s.fs.RemoveAll(s.Directory); err != nil {
 		return fmt.Errorf("failed to remove SE unlocker directory: %w", err)
 	}
 
@@ -271,7 +271,7 @@ func CreateSecureEnclaveUnlocker(
 
 	// Write SE-encrypted long-term key
 	ltKeyPath := filepath.Join(unlockerDir, seLongtermFilename)
-	if err := WriteFileAtomic(fs, ltKeyPath, encryptedLtKey); err != nil {
+	if err := afero.WriteFile(fs, ltKeyPath, encryptedLtKey, FilePerms); err != nil {
 		return nil, fmt.Errorf(
 			"failed to write SE-encrypted long-term key: %w",
 			err,
@@ -295,7 +295,7 @@ func CreateSecureEnclaveUnlocker(
 	}
 
 	metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json")
-	if err := WriteFileAtomic(fs, metadataPath, metadataBytes); err != nil {
+	if err := afero.WriteFile(fs, metadataPath, metadataBytes, FilePerms); err != nil {
 		return nil, fmt.Errorf("failed to write metadata: %w", err)
 	}
 

internal/secret/version.go

@@ -131,10 +131,7 @@ func GenerateVersionName(fs afero.Fs, secretDir string) (string, error) {
 	return fmt.Sprintf("%s.%03d", today, newSerial), nil
 }
 
-// Save saves the version metadata and value. The files are written into a
+// Save saves the version metadata and value
-// temporary directory that is renamed to sv.Directory once all of them are
-// complete, so the version directory is either whole or absent, even if the
-// process dies part-way.
 func (sv *Version) Save(value *memguard.LockedBuffer) error {
 	if value == nil {
 		return errNilValueBuffer
@@ -148,22 +145,14 @@ func (sv *Version) Save(value *memguard.LockedBuffer) error {
 
 	fs := sv.vault.GetFilesystem()
 
-	// Create the versions directory the finished version is renamed into
+	// Create version directory
-	err := fs.MkdirAll(filepath.Dir(sv.Directory), DirPerms)
+	err := fs.MkdirAll(sv.Directory, DirPerms)
 	if err != nil {
-		Debug("Failed to create versions directory", "error", err, "dir", sv.Directory)
+		Debug("Failed to create version directory", "error", err, "dir", sv.Directory)
 
-		return fmt.Errorf("failed to create versions directory: %w", err)
+		return fmt.Errorf("failed to create version directory: %w", err)
 	}
 
-	tmpDir, err := TempDirFor(fs, sv.Directory)
-	if err != nil {
-		return err
-	}
-
-	// Once the rename below has moved it into place, this finds nothing.
-	defer func() { _ = fs.RemoveAll(tmpDir) }()
-
 	// Generate a new keypair for this version
 	Debug("Generating version-specific keypair", "version", sv.Version)
 
@@ -184,28 +173,21 @@ func (sv *Version) Save(value *memguard.LockedBuffer) error {
 		slog.String("public_key", versionIdentity.Recipient().String()),
 	)
 
-	err = sv.writePublicKeyAndValue(fs, tmpDir, versionIdentity, value)
+	err = sv.writePublicKeyAndValue(fs, versionIdentity, value)
 	if err != nil {
 		return err
 	}
 
-	err = sv.writeEncryptedPrivateKey(fs, tmpDir, versionPrivateKeyBuffer)
+	err = sv.writeEncryptedPrivateKey(fs, versionPrivateKeyBuffer)
 	if err != nil {
 		return err
 	}
 
-	err = sv.writeEncryptedMetadata(fs, tmpDir, versionIdentity)
+	err = sv.writeEncryptedMetadata(fs, versionIdentity)
 	if err != nil {
 		return err
 	}
 
-	err = fs.Rename(tmpDir, sv.Directory)
-	if err != nil {
-		Debug("Failed to move version into place", "error", err, "dir", sv.Directory)
-
-		return fmt.Errorf("failed to move version into place: %w", err)
-	}
-
 	Debug("Successfully saved secret version",
 		"version", sv.Version, "secret_name", sv.SecretName)
 
@@ -376,18 +358,17 @@ func (sv *Version) GetValue(
 }
 
 // writePublicKeyAndValue stores the version's public key and the value
-// encrypted to it in dir.
+// encrypted to it.
 func (sv *Version) writePublicKeyAndValue(
 	fs afero.Fs,
-	dir string,
 	versionIdentity *age.X25519Identity,
 	value *memguard.LockedBuffer,
 ) error {
 	versionPublicKey := versionIdentity.Recipient().String()
-	pubKeyPath := filepath.Join(dir, "pub.age")
+	pubKeyPath := filepath.Join(sv.Directory, "pub.age")
 	Debug("Writing version public key", "path", pubKeyPath)
 
-	err := WriteFileAtomic(fs, pubKeyPath, []byte(versionPublicKey))
+	err := afero.WriteFile(fs, pubKeyPath, []byte(versionPublicKey), FilePerms)
 	if err != nil {
 		Debug("Failed to write version public key", "error", err, "path", pubKeyPath)
 
@@ -404,10 +385,10 @@ func (sv *Version) writePublicKeyAndValue(
 		return fmt.Errorf("failed to encrypt version value: %w", err)
 	}
 
-	valuePath := filepath.Join(dir, "value.age")
+	valuePath := filepath.Join(sv.Directory, "value.age")
 	Debug("Writing encrypted version value", "path", valuePath)
 
-	err = WriteFileAtomic(fs, valuePath, encryptedValue)
+	err = afero.WriteFile(fs, valuePath, encryptedValue, FilePerms)
 	if err != nil {
 		Debug("Failed to write encrypted version value", "error", err, "path", valuePath)
 
@@ -418,10 +399,9 @@ func (sv *Version) writePublicKeyAndValue(
 }
 
 // writeEncryptedPrivateKey encrypts the version's private key to the
-// vault's long-term public key and stores it in dir.
+// vault's long-term public key and stores it.
 func (sv *Version) writeEncryptedPrivateKey(
 	fs afero.Fs,
-	dir string,
 	versionPrivateKeyBuffer *memguard.LockedBuffer,
 ) error {
 	vaultDir, _ := sv.vault.GetDirectory()
@@ -455,10 +435,10 @@ func (sv *Version) writeEncryptedPrivateKey(
 		return fmt.Errorf("failed to encrypt version private key: %w", err)
 	}
 
-	privKeyPath := filepath.Join(dir, "priv.age")
+	privKeyPath := filepath.Join(sv.Directory, "priv.age")
 	Debug("Writing encrypted version private key", "path", privKeyPath)
 
-	err = WriteFileAtomic(fs, privKeyPath, encryptedPrivKey)
+	err = afero.WriteFile(fs, privKeyPath, encryptedPrivKey, FilePerms)
 	if err != nil {
 		Debug("Failed to write encrypted version private key",
 			"error", err, "path", privKeyPath)
@@ -470,10 +450,9 @@ func (sv *Version) writeEncryptedPrivateKey(
 }
 
 // writeEncryptedMetadata encrypts the version metadata to the version's
-// public key and stores it in dir.
+// public key and stores it.
 func (sv *Version) writeEncryptedMetadata(
 	fs afero.Fs,
-	dir string,
 	versionIdentity *age.X25519Identity,
 ) error {
 	Debug("Encrypting version metadata", "version", sv.Version)
@@ -497,10 +476,10 @@ func (sv *Version) writeEncryptedMetadata(
 		return fmt.Errorf("failed to encrypt version metadata: %w", err)
 	}
 
-	metadataPath := filepath.Join(dir, "metadata.age")
+	metadataPath := filepath.Join(sv.Directory, "metadata.age")
 	Debug("Writing encrypted version metadata", "path", metadataPath)
 
-	err = WriteFileAtomic(fs, metadataPath, encryptedMetadata)
+	err = afero.WriteFile(fs, metadataPath, encryptedMetadata, FilePerms)
 	if err != nil {
 		Debug("Failed to write encrypted version metadata",
 			"error", err, "path", metadataPath)
@@ -561,12 +540,15 @@ func GetCurrentVersion(fs afero.Fs, secretDir string) (string, error) {
 }
 
 // SetCurrentVersion updates the "current" file to point to a specific version
-// The file contains just the version name (e.g., "20231215.001"). It is
+// The file contains just the version name (e.g., "20231215.001")
-// replaced in one rename, so once written it always exists.
 func SetCurrentVersion(fs afero.Fs, secretDir string, version string) error {
 	currentPath := filepath.Join(secretDir, "current")
 
-	err := WriteFileAtomic(fs, currentPath, []byte(version))
+	// Remove existing file if it exists
+	_ = fs.Remove(currentPath)
+
+	// Write just the version name to the file
+	err := afero.WriteFile(fs, currentPath, []byte(version), FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to create current version file: %w", err)
 	}

internal/vault/errors.go

@@ -62,10 +62,4 @@ var (
 	// ErrUnlockerNotFound indicates no unlocker with the given ID exists.
 	// Composed as "unlocker with ID <id> not found".
 	ErrUnlockerNotFound = errors.New("not found")
-
-	// ErrNoLockForFilesystem indicates LockStateDir was given a filesystem
-	// it cannot lock. Composed as "cannot lock the state directory on
-	// filesystem <type>".
-	ErrNoLockForFilesystem = errors.New(
-		"cannot lock the state directory on filesystem")
 )

internal/vault/lock.go

@@ -1,73 +0,0 @@
-package vault
-
-import (
-	"fmt"
-	"os"
-	"path/filepath"
-	"sync"
-	"syscall"
-
-	"git.eeqj.de/sneak/secret/internal/secret"
-	"github.com/spf13/afero"
-)
-
-// lockFileName is the file in the state directory that LockStateDir locks.
-const lockFileName = "lock"
-
-// memFsLock stands in for the lock file on the in-memory filesystem, which
-// has no file locks. Every in-memory filesystem in the process shares it.
-//
-//nolint:gochecknoglobals // must outlive the call that takes it
-var memFsLock sync.Mutex
-
-// LockStateDir takes the lock that a command changing anything under
-// stateDir holds until it returns, and returns the function that releases
-// it. While one command holds it, the next one waits here. Reads take no
-// lock: each file or directory a command changes is replaced in a single
-// rename, so a reader finds it as it was before or after, never half-made.
-//
-// On the real filesystem the lock is flock(2) on the file "lock" in
-// stateDir, which the kernel releases when the process dies, so a killed
-// command never leaves the tool locked. The in-memory filesystem the tests
-// use has no file locks, so a process-wide mutex stands in for flock there.
-// Any other filesystem is refused rather than left unlocked.
-func LockStateDir(fs afero.Fs, stateDir string) (func(), error) {
-	switch fs.(type) {
-	case *afero.OsFs:
-		return flockStateDir(stateDir)
-	case *afero.MemMapFs:
-		memFsLock.Lock()
-
-		return memFsLock.Unlock, nil
-	default:
-		return nil, fmt.Errorf("%w %T", ErrNoLockForFilesystem, fs)
-	}
-}
-
-// flockStateDir takes flock(2) on the lock file in stateDir, creating the
-// directory and the file if needed. Go opens files close-on-exec, so
-// programs the command runs, such as gpg, do not inherit the lock.
-func flockStateDir(stateDir string) (func(), error) {
-	err := os.MkdirAll(stateDir, secret.DirPerms)
-	if err != nil {
-		return nil, fmt.Errorf("failed to create state directory: %w", err)
-	}
-
-	lockPath := filepath.Join(stateDir, lockFileName)
-
-	//nolint:gosec // G304: the path is the lock file in the state directory
-	file, err := os.OpenFile(lockPath, os.O_RDWR|os.O_CREATE, secret.FilePerms)
-	if err != nil {
-		return nil, fmt.Errorf("failed to open lock file: %w", err)
-	}
-
-	err = syscall.Flock(int(file.Fd()), syscall.LOCK_EX)
-	if err != nil {
-		_ = file.Close()
-
-		return nil, fmt.Errorf("failed to lock %s: %w", lockPath, err)
-	}
-
-	// Closing the file releases the lock.
-	return func() { _ = file.Close() }, nil
-}

internal/vault/lock_test.go

@@ -1,134 +0,0 @@
-package vault_test
-
-import (
-	"testing"
-	"time"
-
-	"git.eeqj.de/sneak/secret/internal/vault"
-	"github.com/spf13/afero"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-const (
-	// lockWait is how long a test waits for the lock before deciding it
-	// will never come free.
-	lockWait = 10 * time.Second
-
-	// heldWait is how long a test watches a second holder fail to take a
-	// lock that is held. Broken exclusion lets it in at once.
-	heldWait = 100 * time.Millisecond
-)
-
-// lockFilesystem is a filesystem LockStateDir can lock, with a state
-// directory on it.
-type lockFilesystem struct {
-	name     string
-	fs       afero.Fs
-	stateDir string
-}
-
-// lockFilesystems returns the real filesystem, locked with flock, and the
-// in-memory one, locked with a mutex.
-func lockFilesystems(t *testing.T) []lockFilesystem {
-	t.Helper()
-
-	return []lockFilesystem{
-		{"memory", afero.NewMemMapFs(), testStateDir},
-		{"real", afero.NewOsFs(), t.TempDir()},
-	}
-}
-
-// lockInBackground starts taking the lock and returns a channel that
-// delivers the function releasing it once it has been taken.
-func lockInBackground(
-	t *testing.T, fs afero.Fs, stateDir string,
-) <-chan func() {
-	t.Helper()
-
-	taken := make(chan func(), 1)
-
-	go func() {
-		release, err := vault.LockStateDir(fs, stateDir)
-		if assert.NoError(t, err) {
-			taken <- release
-		}
-	}()
-
-	return taken
-}
-
-// TestLockStateDirExcludes checks that while the lock is held a second
-// holder, with its own open lock file on the real filesystem, waits, and
-// that it gets the lock once the first releases it.
-func TestLockStateDirExcludes(t *testing.T) {
-	t.Parallel()
-
-	for _, lfs := range lockFilesystems(t) {
-		t.Run(lfs.name, func(t *testing.T) {
-			t.Parallel()
-
-			release, err := vault.LockStateDir(lfs.fs, lfs.stateDir)
-			require.NoError(t, err)
-
-			taken := lockInBackground(t, lfs.fs, lfs.stateDir)
-
-			select {
-			case second := <-taken:
-				second()
-				release()
-				t.Fatal("a second holder took the lock while it was held")
-			case <-time.After(heldWait):
-			}
-
-			release()
-
-			select {
-			case second := <-taken:
-				second()
-			case <-time.After(lockWait):
-				t.Fatal("the second holder never got the lock")
-			}
-		})
-	}
-}
-
-// TestLockStateDirFreeAfterPanic checks that a holder that panics, and
-// releases the lock with defer as every command does, leaves it free.
-func TestLockStateDirFreeAfterPanic(t *testing.T) {
-	t.Parallel()
-
-	for _, lfs := range lockFilesystems(t) {
-		t.Run(lfs.name, func(t *testing.T) {
-			t.Parallel()
-
-			assert.Panics(t, func() {
-				release, err := vault.LockStateDir(lfs.fs, lfs.stateDir)
-				require.NoError(t, err)
-
-				defer release()
-
-				panic("the command failed")
-			})
-
-			select {
-			case release := <-lockInBackground(t, lfs.fs, lfs.stateDir):
-				release()
-			case <-time.After(lockWait):
-				t.Fatal("the lock was still held after its holder panicked")
-			}
-		})
-	}
-}
-
-// TestLockStateDirRefusesOtherFilesystems checks that a filesystem with no
-// lock implementation is refused instead of being used unlocked.
-func TestLockStateDirRefusesOtherFilesystems(t *testing.T) {
-	t.Parallel()
-
-	fs := afero.NewReadOnlyFs(afero.NewMemMapFs())
-
-	release, err := vault.LockStateDir(fs, testStateDir)
-	require.ErrorIs(t, err, vault.ErrNoLockForFilesystem)
-	assert.Nil(t, release)
-}

internal/vault/management.go

@@ -169,7 +169,7 @@ func processMnemonicForVault(
 
 	ltPubKeyPath := filepath.Join(vaultDir, "pub.age")
 
-	err = secret.WriteFileAtomic(fs, ltPubKeyPath, []byte(ltPubKey))
+	err = afero.WriteFile(fs, ltPubKeyPath, []byte(ltPubKey), secret.FilePerms)
 	if err != nil {
 		return 0, "", "", fmt.Errorf("failed to write long-term public key: %w", err)
 	}
@@ -295,13 +295,21 @@ func SelectVault(fs afero.Fs, stateDir string, name string) error {
 		return fmt.Errorf("vault %s %w", name, ErrVaultNotFound)
 	}
 
-	// Create or replace the currentvault file with just the vault name. It
+	// Create or update the currentvault file with just the vault name
-	// is replaced in one rename, so it never goes missing.
 	currentVaultPath := filepath.Join(stateDir, "currentvault")
 
+	// Remove existing file if it exists
+	_, err = fs.Stat(currentVaultPath)
+	if err == nil {
+		secret.Debug("Removing existing currentvault file", "path", currentVaultPath)
+
+		_ = fs.Remove(currentVaultPath)
+	}
+
+	// Write just the vault name to the file
 	secret.Debug("Writing currentvault file", "vault_name", name)
 
-	err = secret.WriteFileAtomic(fs, currentVaultPath, []byte(name))
+	err = afero.WriteFile(fs, currentVaultPath, []byte(name), secret.FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to select vault: %w", err)
 	}

internal/vault/metadata.go

@@ -113,7 +113,7 @@ func SaveVaultMetadata(fs afero.Fs, vaultDir string, metadata *Metadata) error {
 		return fmt.Errorf("failed to marshal vault metadata: %w", err)
 	}
 
-	err = secret.WriteFileAtomic(fs, metadataPath, metadataBytes)
+	err = afero.WriteFile(fs, metadataPath, metadataBytes, secret.FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to write vault metadata: %w", err)
 	}

internal/vault/secrets.go

@@ -156,59 +156,17 @@ func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool)
 		slog.String("secret_dir", secretDir),
 	)
 
-	// Check for an existing secret and the version the new one supersedes
+	// Check for an existing secret and prepare its directory
-	exists, previousVersion, err := v.checkExistingSecret(name, secretDir, force)
+	exists, previousVersion, err := v.prepareSecretDir(name, secretDir, force)
 	if err != nil {
 		return err
 	}
 
-	if exists {
-		return v.addVersion(name, secretDir, value, previousVersion)
-	}
-
-	return v.addNewSecret(name, secretDir, value)
-}
-
-// addNewSecret creates a secret by assembling its first version and current
-// pointer in a temporary directory, then renaming that directory to
-// secretDir, so an interrupted add leaves no half-made secret behind.
-func (v *Vault) addNewSecret(
-	name, secretDir string, value *memguard.LockedBuffer,
-) error {
-	buildDir, err := secret.TempDirFor(v.fs, secretDir)
-	if err != nil {
-		return err
-	}
-
-	// Once the rename below has moved it into place, this finds nothing.
-	defer func() { _ = v.fs.RemoveAll(buildDir) }()
-
-	err = v.addVersion(name, buildDir, value, nil)
-	if err != nil {
-		return err
-	}
-
-	err = v.fs.Rename(buildDir, secretDir)
-	if err != nil {
-		return fmt.Errorf("failed to move new secret into place: %w", err)
-	}
-
-	return nil
-}
-
-// addVersion saves value as a new version under secretDir, sets the
-// notAfter timestamp of the version it supersedes, if any, and then points
-// current at the new version. Until that last step, current still names the
-// previous version, which stays readable.
-func (v *Vault) addVersion(
-	name, secretDir string, value *memguard.LockedBuffer,
-	previousVersion *secret.Version,
-) error {
 	now := time.Now()
 
 	// Create the new version and save the encrypted value
 	versionName, err := v.createAndSaveVersion(
-		name, secretDir, value, previousVersion, &now)
+		name, secretDir, value, previousVersion, &now, exists)
 	if err != nil {
 		return err
 	}
@@ -278,7 +236,7 @@ func updateVersionMetadata(
 	// Write encrypted metadata
 	metadataPath := filepath.Join(version.Directory, "metadata.age")
 
-	err = secret.WriteFileAtomic(fs, metadataPath, encryptedMetadata)
+	err = afero.WriteFile(fs, metadataPath, encryptedMetadata, secret.FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to write encrypted version metadata: %w", err)
 	}
@@ -436,14 +394,12 @@ func (v *Vault) GetSecretObject(name string) (*secret.Secret, error) {
 	return secretObj, nil
 }
 
-// CopySecretVersion copies a single version from source into destSecretDir
+// CopySecretVersion copies a single version from source to this vault
-// in this vault. It decrypts the value using srcIdentity and re-encrypts
+// It decrypts the value using srcIdentity and re-encrypts for this vault
-// for this vault.
 func (v *Vault) CopySecretVersion(
 	srcVersion *secret.Version,
 	srcIdentity *age.X25519Identity,
 	destSecretName string,
-	destSecretDir string,
 	destVersionName string,
 ) error {
 	secret.DebugWith("Copying secret version to vault",
@@ -469,7 +425,6 @@ func (v *Vault) CopySecretVersion(
 
 	// Create destination version with same name
 	destVersion := secret.NewVersion(v, destSecretName, destVersionName)
-	destVersion.Directory = filepath.Join(destSecretDir, "versions", destVersionName)
 
 	// Copy metadata (preserve original timestamps)
 	destVersion.Metadata = srcVersion.Metadata
@@ -510,11 +465,11 @@ func (v *Vault) CopySecretAllVersions(
 		return fmt.Errorf("failed to get destination vault directory: %w", err)
 	}
 
-	// Refuse to replace an existing destination secret unless forced
+	// Check if destination secret already exists and clear it if forced
 	destStorageName := strings.ReplaceAll(destSecretName, "/", "%")
 	destSecretDir := filepath.Join(destVaultDir, "secrets.d", destStorageName)
 
-	err = v.checkCopyDestination(destSecretDir, destSecretName, force)
+	err = v.prepareCopyDestination(destSecretDir, destSecretName, force)
 	if err != nil {
 		return err
 	}
@@ -550,8 +505,14 @@ func (v *Vault) CopySecretAllVersions(
 		return fmt.Errorf("failed to get current version: %w", err)
 	}
 
-	// Copy each version and the current pointer, then move the copy into place
+	// Create destination secret directory
-	err = v.copyVersions(srcVault, srcIdentity,
+	err = v.fs.MkdirAll(destSecretDir, secret.DirPerms)
+	if err != nil {
+		return fmt.Errorf("failed to create destination secret directory: %w", err)
+	}
+
+	// Copy each version and set the current pointer, rolling back on error
+	err = v.copyVersionsWithRollback(srcVault, srcIdentity,
 		srcSecretName, destSecretName, destSecretDir, versions, currentVersion)
 	if err != nil {
 		return err
@@ -566,10 +527,10 @@ func (v *Vault) CopySecretAllVersions(
 	return nil
 }
 
-// checkExistingSecret reports whether the secret already exists, refuses to
+// prepareSecretDir checks for an existing secret directory and prepares it
-// overwrite it unless force is set, and returns its current version, which
+// for a new version. It returns whether the secret already existed and the
-// the new version supersedes, if any.
+// current version to be superseded, if any.
-func (v *Vault) checkExistingSecret(
+func (v *Vault) prepareSecretDir(
 	name, secretDir string, force bool,
 ) (bool, *secret.Version, error) {
 	// Check if secret already exists
@@ -586,6 +547,19 @@ func (v *Vault) checkExistingSecret(
 	secret.Debug("Secret existence check complete", "exists", exists)
 
 	if !exists {
+		// Create secret directory for new secret
+		secret.Debug("Creating secret directory", "secret_dir", secretDir)
+
+		err = v.fs.MkdirAll(secretDir, secret.DirPerms)
+		if err != nil {
+			secret.Debug("Failed to create secret directory",
+				"error", err, "secret_dir", secretDir)
+
+			return false, nil, fmt.Errorf("failed to create secret directory: %w", err)
+		}
+
+		secret.Debug("Created secret directory successfully")
+
 		return false, nil, nil
 	}
 
@@ -727,11 +701,11 @@ func (v *Vault) resolveSecretVersion(name, version string) (string, error) {
 }
 
 // createAndSaveVersion generates a new version name, sets the version
-// timestamps, and saves the encrypted value under secretDir, which is a
+// timestamps, and saves the encrypted value. When saving fails for a newly
-// temporary directory while a new secret is being assembled.
+// created secret, the secret directory is removed again.
 func (v *Vault) createAndSaveVersion(
 	name, secretDir string, value *memguard.LockedBuffer,
-	previousVersion *secret.Version, now *time.Time,
+	previousVersion *secret.Version, now *time.Time, exists bool,
 ) (string, error) {
 	// Generate new version name
 	versionName, err := secret.GenerateVersionName(v.fs, secretDir)
@@ -745,7 +719,6 @@ func (v *Vault) createAndSaveVersion(
 
 	// Create new version
 	newVersion := secret.NewVersion(v, name, versionName)
-	newVersion.Directory = filepath.Join(secretDir, "versions", versionName)
 
 	// Set version timestamps
 	if previousVersion == nil {
@@ -765,73 +738,57 @@ func (v *Vault) createAndSaveVersion(
 	if err != nil {
 		secret.Debug("Failed to save new version", "error", err, "version", versionName)
 
+		// Clean up the secret directory if this was a new secret
+		if !exists {
+			secret.Debug("Cleaning up secret directory due to save failure",
+				"secret_dir", secretDir)
+
+			_ = v.fs.RemoveAll(secretDir)
+		}
+
 		return "", fmt.Errorf("failed to save version: %w", err)
 	}
 
 	return versionName, nil
 }
 
-// copyVersions copies each version of the source secret and its current
+// copyVersionsWithRollback copies each version of the source secret into the
-// pointer into a temporary directory, then moves that directory to
+// destination directory and sets the current version pointer, removing the
-// destSecretDir, replacing a secret already there. Nothing in this vault
+// partial copy when any step fails.
-// changes until the copy is complete, so an interrupted copy leaves only a
+func (v *Vault) copyVersionsWithRollback(
-// temporary directory behind.
-func (v *Vault) copyVersions(
 	srcVault *Vault, srcIdentity *age.X25519Identity,
 	srcSecretName, destSecretName, destSecretDir string,
 	versions []string, currentVersion string,
 ) error {
-	buildDir, err := secret.TempDirFor(v.fs, destSecretDir)
+	// Copy each version
-	if err != nil {
-		return err
-	}
-
-	// Once the rename below has moved it into place, this finds nothing.
-	defer func() { _ = v.fs.RemoveAll(buildDir) }()
-
 	for _, versionName := range versions {
 		srcVersion := secret.NewVersion(srcVault, srcSecretName, versionName)
 
-		err = v.CopySecretVersion(
+		err := v.CopySecretVersion(srcVersion, srcIdentity, destSecretName, versionName)
-			srcVersion, srcIdentity, destSecretName, buildDir, versionName)
 		if err != nil {
+			// Rollback: remove partial copy
+			secret.Debug("Rolling back partial copy due to error", "error", err)
+
+			_ = v.fs.RemoveAll(destSecretDir)
+
 			return fmt.Errorf("failed to copy version %s: %w", versionName, err)
 		}
 	}
 
-	err = secret.SetCurrentVersion(v.fs, buildDir, currentVersion)
+	// Set current version
+	err := secret.SetCurrentVersion(v.fs, destSecretDir, currentVersion)
 	if err != nil {
+		_ = v.fs.RemoveAll(destSecretDir)
+
 		return fmt.Errorf("failed to set current version: %w", err)
 	}
 
-	// With --force, the secret being replaced goes only now that its
-	// replacement is complete
-	exists, err := afero.DirExists(v.fs, destSecretDir)
-	if err != nil {
-		return fmt.Errorf("failed to check destination: %w", err)
-	}
-
-	if exists {
-		secret.Debug("Removing existing destination secret", "path", destSecretDir)
-
-		err = secret.RemoveDirAtomic(v.fs, destSecretDir)
-		if err != nil {
-			return fmt.Errorf("failed to remove existing destination secret: %w", err)
-		}
-	}
-
-	err = v.fs.Rename(buildDir, destSecretDir)
-	if err != nil {
-		return fmt.Errorf("failed to move copied secret into place: %w", err)
-	}
-
 	return nil
 }
 
-// checkCopyDestination refuses to copy over an existing secret unless force
+// prepareCopyDestination ensures the destination secret directory can be
-// is set. A secret being replaced is removed by copyVersions, once its
+// created, removing an existing secret when force is set.
-// replacement is complete.
+func (v *Vault) prepareCopyDestination(
-func (v *Vault) checkCopyDestination(
 	destSecretDir, destSecretName string, force bool,
 ) error {
 	exists, err := afero.DirExists(v.fs, destSecretDir)
@@ -846,5 +803,15 @@ func (v *Vault) checkCopyDestination(
 		)
 	}
 
+	if exists && force {
+		// Remove existing secret
+		secret.Debug("Removing existing destination secret", "path", destSecretDir)
+
+		err = v.fs.RemoveAll(destSecretDir)
+		if err != nil {
+			return fmt.Errorf("failed to remove existing destination secret: %w", err)
+		}
+	}
+
 	return nil
 }

internal/vault/unlockers.go

@@ -310,16 +310,30 @@ func (v *Vault) SelectUnlocker(unlockerID string) error {
 		return fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound)
 	}
 
-	// Create or replace the current-unlocker file with just the unlocker
+	// Create/update current-unlocker file with just the unlocker name
-	// name. It is replaced in one rename, so it never goes missing.
 	currentUnlockerPath := filepath.Join(vaultDir, "current-unlocker")
 
+	// Remove existing file if it exists
+	exists, err := afero.Exists(v.fs, currentUnlockerPath)
+	if err != nil {
+		return fmt.Errorf("failed to check if current-unlocker file exists: %w", err)
+	}
+
+	if exists {
+		err = v.fs.Remove(currentUnlockerPath)
+		if err != nil {
+			return fmt.Errorf("failed to remove existing current-unlocker file: %w", err)
+		}
+	}
+
 	// Get just the unlocker name (basename of the directory)
 	unlockerName := filepath.Base(targetUnlockerDir)
 
+	// Write just the unlocker name to the file
 	secret.Debug("Writing current-unlocker file", "unlocker_name", unlockerName)
 
-	err = secret.WriteFileAtomic(v.fs, currentUnlockerPath, []byte(unlockerName))
+	err = afero.WriteFile(v.fs, currentUnlockerPath, []byte(unlockerName),
+		secret.FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to create current-unlocker file: %w", err)
 	}
@@ -337,14 +351,6 @@ func (v *Vault) CreatePassphraseUnlocker(
 		return nil, fmt.Errorf("failed to get vault directory: %w", err)
 	}
 
-	// We need to get the long-term key (either from memory if unlocked, or
-	// derive it). Getting it before anything is written means failing to
-	// get it changes nothing, even when replacing the current unlocker.
-	ltIdentity, err := v.GetOrDeriveLongTermKey()
-	if err != nil {
-		return nil, fmt.Errorf("failed to get long-term key: %w", err)
-	}
-
 	// Create unlocker directory
 	unlockerDir := filepath.Join(vaultDir, "unlockers.d", unlockerTypePassphrase)
 
@@ -365,7 +371,33 @@ func (v *Vault) CreatePassphraseUnlocker(
 		return nil, err
 	}
 
+	// Create metadata
+	metadata := UnlockerMetadata{
+		Type:      unlockerTypePassphrase,
+		CreatedAt: time.Now(),
+		Flags:     []string{},
+	}
+
+	// Write metadata
+	metadataBytes, err := json.MarshalIndent(metadata, "", "  ")
+	if err != nil {
+		return nil, fmt.Errorf("failed to marshal metadata: %w", err)
+	}
+
+	metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json")
+
+	err = afero.WriteFile(v.fs, metadataPath, metadataBytes, secret.FilePerms)
+	if err != nil {
+		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
+	}
+
 	// Encrypt long-term private key to this unlocker
+	// We need to get the long-term key (either from memory if unlocked, or derive it)
+	ltIdentity, err := v.GetOrDeriveLongTermKey()
+	if err != nil {
+		return nil, fmt.Errorf("failed to get long-term key: %w", err)
+	}
+
 	ltPrivKeyBuffer := memguard.NewBufferFromBytes([]byte(ltIdentity.String()))
 	defer ltPrivKeyBuffer.Destroy()
 
@@ -377,31 +409,11 @@ func (v *Vault) CreatePassphraseUnlocker(
 
 	ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age")
 
-	err = secret.WriteFileAtomic(v.fs, ltPrivKeyPath, encryptedLtPrivKey)
+	err = afero.WriteFile(v.fs, ltPrivKeyPath, encryptedLtPrivKey, secret.FilePerms)
 	if err != nil {
 		return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err)
 	}
 
-	// Write the metadata last: readers skip an unlocker directory without
-	// it, so an unlocker interrupted before this point is never used.
-	metadata := UnlockerMetadata{
-		Type:      unlockerTypePassphrase,
-		CreatedAt: time.Now(),
-		Flags:     []string{},
-	}
-
-	metadataBytes, err := json.MarshalIndent(metadata, "", "  ")
-	if err != nil {
-		return nil, fmt.Errorf("failed to marshal metadata: %w", err)
-	}
-
-	metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json")
-
-	err = secret.WriteFileAtomic(v.fs, metadataPath, metadataBytes)
-	if err != nil {
-		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
-	}
-
 	// Create the unlocker instance
 	unlocker := secret.NewPassphraseUnlocker(v.fs, unlockerDir, metadata)
 
@@ -455,8 +467,9 @@ func (v *Vault) writeUnlockerKeypair(
 	// Write public key
 	pubKeyPath := filepath.Join(unlockerDir, "pub.age")
 
-	err := secret.WriteFileAtomic(v.fs, pubKeyPath,
+	err := afero.WriteFile(v.fs, pubKeyPath,
-		[]byte(unlockerIdentity.Recipient().String()))
+		[]byte(unlockerIdentity.Recipient().String()),
+		secret.FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to write unlocker public key: %w", err)
 	}
@@ -475,7 +488,7 @@ func (v *Vault) writeUnlockerKeypair(
 	// Write encrypted private key
 	privKeyPath := filepath.Join(unlockerDir, "priv.age")
 
-	err = secret.WriteFileAtomic(v.fs, privKeyPath, encryptedPrivKey)
+	err = afero.WriteFile(v.fs, privKeyPath, encryptedPrivKey, secret.FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to write encrypted unlocker private key: %w", err)
 	}

internal/vault/vault_error_test.go

@@ -90,10 +90,4 @@ func TestAddSecretCleansUpOnFailure(t *testing.T) {
 	secretDir := filepath.Join(vaultDir, "secrets.d", testSecretName)
 	exists, _ := afero.DirExists(fs, secretDir)
 	assert.False(t, exists, "Secret directory should not exist after failed AddSecret")
-
-	// Nor is the temporary directory the secret was assembled in left behind
-	entries, err := afero.ReadDir(fs, vaultDir)
-	require.NoError(t, err)
-	require.Len(t, entries, 1)
-	assert.Equal(t, "pub.age", entries[0].Name())
 }