Lock the state directory and write vault files atomically (closes #34)

Each command that changes the state directory holds one lock: flock(2)
on `lock` in the state directory, dropped by the kernel if the process
dies, or a process-wide mutex on the in-memory test filesystem. It
covers the state directory, not each vault, because `currentvault`,
`vault create` and cross-vault moves span vaults, and a lock file in a
vault would be deleted by `vault remove` under a waiting command.

Files go through `secret.WriteFileAtomic`; versions, new secrets and
cross-vault copies are built in a temporary directory and renamed into
place; removals rename out of the way first. An unlocker added under
an existing unlocker's directory name is still rewritten file by file:
#71.

Model: opus-5-5

TODO.md

@@ -29,35 +29,17 @@ Bring the repo into policy compliance in one commit:
   (`flock` on `lock` in the state directory; a mutex on the in-memory
   test filesystem), so concurrent commands no longer lose versions or
   race on the current pointers. Every file is written through
-  `secret.WriteFileAtomic` (temporary file, sync, rename), so no file
-  is ever half-written and `current`, `currentvault` and
-  `current-unlocker` never go missing. New versions, new secrets and
-  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.
+  `secret.WriteFileAtomic` (temporary file, sync, rename); new
+  versions, new secrets and cross-vault copies are built in a
+  temporary directory and renamed into place, and removals rename out
+  of the way first, so an interrupted command leaves nothing
+  half-written, with one exception: an unlocker added under the
+  directory name of an existing one is rewritten in place, file by
+  file, and a crash part-way leaves it unable to open the vault. 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).
 - 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 +103,12 @@ 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
-      copies (keychainunlocker.go:356, pgpunlocker.go:256,
-      version.go:155); age secret key held in a plain string in
-      cli/crypto.go:86,91,113; private keys exposed via buffer.Bytes()
-      to GPGEncryptFunc and EncryptWithPassphrase.
+    - Memory security: KeychainData stores AgePrivKeyPassphrase as a
+      plain string (keychainunlocker.go:342,393-396); age identity
+      .String() creates unprotected copies (keychainunlocker.go:356,
+      pgpunlocker.go:256, version.go:155); age secret key held in a
+      plain string in cli/crypto.go:86,91,113; private keys exposed via
+      buffer.Bytes() to GPGEncryptFunc and EncryptWithPassphrase.
     - 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:

internal/cli/lock_test.go

@@ -3,7 +3,6 @@ package cli
 
 import (
 	"io"
-	"os"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -13,47 +12,12 @@ import (
 
 	"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(
@@ -184,7 +148,7 @@ func TestEncryptPipedIntoAdd(t *testing.T) {
 	fs := afero.NewMemMapFs()
 	_, err := vault.CreateVault(fs, testStateDir, "default")
 	require.NoError(t, err)
-	require.NoError(t, afero.WriteFile(fs, testInput, []byte("piped"), 0o600))
+	require.NoError(t, afero.WriteFile(fs, "/plaintext", []byte("piped"), 0o600))
 
 	pipeReader, pipeWriter := io.Pipe()
 	// If the test gives up, this makes add's read fail, so that both
@@ -211,14 +175,14 @@ func TestEncryptPipedIntoAdd(t *testing.T) {
 		encrypt.cmd = &cobra.Command{}
 		encrypt.cmd.SetOut(pipeWriter)
 
-		err := encrypt.Encrypt("key", testInput, "")
+		err := encrypt.Encrypt("key", "/plaintext", "")
 		// Ends add's input, as the end of the pipe does
 		_ = pipeWriter.CloseWithError(err)
 
 		results <- err
 	}()
 
-	timeout := time.After(lockWait)
+	timeout := time.After(10 * time.Second)
 
 	for range commands {
 		select {
@@ -242,253 +206,19 @@ func TestFailedCommandReleasesLock(t *testing.T) {
 	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.
-//
-//nolint:paralleltest // t.Setenv forbids t.Parallel
-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)
+	taken := make(chan func(), 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
+		release, err := vault.LockStateDir(fs, testStateDir)
+		if assert.NoError(t, err) {
+			taken <- release
+		}
 	}()
 
-	// 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")
+	case <-time.After(10 * time.Second):
+		t.Fatal("the failed command left the state directory locked")
 	}
-
-	_, err = io.Copy(io.Discard, outputReader)
-	require.NoError(t, err)
-	require.NoError(t, <-done)
 }

internal/secret/atomic_test.go

@@ -22,7 +22,6 @@ var errInjected = errors.New("injected failure")
 const (
 	opCreate = "create"
 	opOpen   = "open"
-	opSync   = "sync"
 	opMkdir  = "mkdir"
 	opRemove = "remove"
 	opRename = "rename"
@@ -32,39 +31,15 @@ const (
 // 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.
-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.
+// call that changes the filesystem, 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.
 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
@@ -74,12 +49,7 @@ func (h hookFs) Create(name string) (afero.File, error) {
 		return nil, err
 	}
 
-	file, err := h.Fs.Create(name)
-	if err != nil {
-		return nil, err
-	}
-
-	return hookFile{File: file, before: h.before}, nil
+	return h.Fs.Create(name)
 }
 
 //nolint:ireturn // implements afero.Fs
@@ -91,16 +61,7 @@ func (h hookFs) OpenFile(
 		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
+	return h.Fs.OpenFile(name, flag, perm)
 }
 
 func (h hookFs) Mkdir(name string, perm os.FileMode) error {
@@ -511,68 +472,10 @@ func TestVersionSaveFailureLeavesNothing(t *testing.T) {
 	}
 }
 
-// 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) {
+// TestSetCurrentVersionNeverMissing checks, before every change
+// SetCurrentVersion makes, that the current file exists: a reader or a crash
+// never finds the secret without a current version.
+func TestSetCurrentVersionNeverMissing(t *testing.T) {
 	t.Parallel()
 
 	for _, tfs := range testFilesystems {
@@ -580,95 +483,24 @@ func TestWriteFileAtomicTempFile(t *testing.T) {
 			t.Parallel()
 
 			base, dir := tfs.open(t)
+			secretDir := filepath.Join(dir, "secret")
+			require.NoError(t, base.MkdirAll(secretDir, 0o700))
+			require.NoError(t, secret.SetCurrentVersion(base, secretDir, "20231216.001"))
 
-			var modes []os.FileMode
+			currentPath := filepath.Join(secretDir, currentFile)
+			fs := hookFs{Fs: base, before: func(string, string) error {
+				exists, err := afero.Exists(base, currentPath)
+				require.NoError(t, err)
+				assert.True(t, exists, "current is missing")
 
-			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
+			}}
 
-					return nil
-				},
-				opened: func(_ string, perm os.FileMode) {
-					modes = append(modes, perm)
-				},
-			}
+			require.NoError(t, secret.SetCurrentVersion(fs, secretDir, "20231216.002"))
 
-			require.NoError(t, secret.WriteFileAtomic(fs,
-				filepath.Join(dir, currentFile), []byte("new")))
-			assert.Equal(t, []os.FileMode{secret.FilePerms}, modes)
+			version, err := secret.GetCurrentVersion(base, secretDir)
+			require.NoError(t, err)
+			assert.Equal(t, "20231216.002", version)
 		})
 	}
 }
-
-// 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.
-//
-//nolint:paralleltest // t.Setenv forbids t.Parallel
-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.
-//
-//nolint:paralleltest // t.Setenv forbids t.Parallel
-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)
-	if err != nil {
+	var keychainData KeychainData
+	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())
 
@@ -363,7 +369,6 @@ 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()
@@ -373,12 +378,15 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 	}
 
 	// 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)
 	}
@@ -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
@@ -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")
+}