Stop vault safety checks from reading unreadable state as empty (closes #51)

Adding a PGP unlocker checked unlockers.d for a duplicate and, when the
directory could not be read, reported no duplicate and went on. The
check now returns an error naming the directory and cause, and the add
stops; a duplicate found is still reported as one.

The same flaw guarded removing the last unlocker and removing a vault
(an unreadable secrets directory counted as no secrets) and vault
import (an unreadable pub.age counted as no long-term key). Those now
stop with an error too.

`unlocker list` keeps skipping entries it cannot read; a comment at the
duplicate check says why the two differ.

Model: opus-5-5

TODO.md

@@ -25,11 +25,13 @@ Bring the repo into policy compliance in one commit:
 
 # Completed Steps
 
-- 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-03: The checks run before changing a vault now stop with an
+  error naming the path and cause when they cannot read what they
+  inspect, instead of reading the failure as "nothing there": the
+  duplicate check before `unlocker add pgp` (an unreadable
+  `unlockers.d`), the secret count that guards removing the last
+  unlocker and removing a vault, and the existing long-term key check
+  before `vault import`.
 - 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
@@ -93,11 +95,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.
     - 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

internal/cli/unlockers.go

@@ -49,7 +49,6 @@ var (
 		"is already added as an unlocker")
 	errUnsupportedUnlockerType = errors.New("unsupported unlocker type")
 	errLastUnlocker            = errors.New("refusing to remove last unlocker")
-	errUnlockerExists          = errors.New("unlocker already exists")
 )
 
 // UnlockerInfo represents unlocker information for display
@@ -691,8 +690,15 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error {
 	// Check if this GPG key is already added
 	expectedID := "pgp-" + fingerprint
 
-	err = cli.checkUnlockerExists(vlt, expectedID)
+	exists, err := cli.checkUnlockerExists(vlt, expectedID)
 	if err != nil {
+		return fmt.Errorf(
+			"could not check whether GPG key %s is already an unlocker: %w",
+			gpgKeyID, err,
+		)
+	}
+
+	if exists {
 		return fmt.Errorf("GPG key %s %w", gpgKeyID, errGPGKeyAlreadyUnlocker)
 	}
 
@@ -772,44 +778,38 @@ func (cli *Instance) UnlockerSelect(unlockerID string) error {
 	return vlt.SelectUnlocker(unlockerID)
 }
 
-// checkUnlockerExists checks if an unlocker with the given ID exists
-func (cli *Instance) checkUnlockerExists(vlt *vault.Vault, unlockerID string) error {
-	// Get the list of unlockers and check if any match the ID
-	unlockers, err := vlt.ListUnlockers()
-	if err != nil {
-		secret.Warn("Could not list unlockers during duplicate check", "error", err)
-
-		return nil // If we can't list unlockers, assume it doesn't exist
-	}
-
-	// Get vault directory to construct unlocker instances
+// checkUnlockerExists reports whether the vault already has an unlocker
+// with the given ID. It returns an error, and no answer, when unlockers.d
+// cannot be read; the caller must then not create the unlocker.
+func (cli *Instance) checkUnlockerExists(
+	vlt *vault.Vault, unlockerID string,
+) (bool, error) {
 	vaultDir, err := vlt.GetDirectory()
 	if err != nil {
-		secret.Warn("Could not get vault directory during duplicate check",
-			"error", err)
-
-		return nil
+		return false, fmt.Errorf("failed to get vault directory: %w", err)
 	}
 
-	// Check each unlocker's ID
 	unlockersDir := filepath.Join(vaultDir, "unlockers.d")
 
+	unlockers, err := vlt.ListUnlockers()
+	if err != nil {
+		return false, fmt.Errorf(
+			"failed to list unlockers in %s: %w", unlockersDir, err,
+		)
+	}
+
 	for _, metadata := range unlockers {
 		// Construct the unlocker matching this metadata to get its ID
 		id, err := findUnlockerIDByMetadata(cli.fs, unlockersDir, metadata, true)
 		if err != nil {
-			secret.Warn(
-				"Could not read unlockers directory during duplicate check, "+
-					"skipping unlocker",
-				"unlockers_dir", unlockersDir, "error", err)
-
-			continue
+			// Unlike `unlocker list`, never skip here: a skipped entry may be the duplicate.
+			return false, err
 		}
 
 		if id != "" && id == unlockerID {
-			return errUnlockerExists
+			return true, nil
 		}
 	}
 
-	return nil
+	return false, nil
 }

internal/cli/unreadable_dir_test.go

@@ -0,0 +1,262 @@
+// Unreadable Directory Tests
+//
+// The checks that guard adding a PGP unlocker (is this key already an
+// unlocker?), removing the last unlocker and removing a vault (does the
+// vault hold secrets?), and importing a mnemonic (does the vault already
+// have a long-term key?) each look at the vault on disk before acting.
+// When that look fails they must refuse to act, not read the failure as
+// "nothing there" and go ahead.
+
+//nolint:testpackage // white-box test of unexported internals
+package cli
+
+import (
+	"context"
+	"errors"
+	"io"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"git.eeqj.de/sneak/secret/internal/secret"
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+const (
+	// unreadableTestGPGUserID is the user ID of the throwaway GPG key the
+	// PGP unlocker tests generate, and the --keyid they pass.
+	unreadableTestGPGUserID = "unlocker-test@example.com"
+
+	// unreadableTestSecretName is the secret stored in the vaults the
+	// removal tests remove from.
+	unreadableTestSecretName = "api-key"
+
+	// unreadableTestOtherVault is a second vault for the vault removal
+	// test, since the last vault can never be removed.
+	unreadableTestOtherVault = "work"
+
+	// unreadableTestSecretsDirName is the directory holding a vault's
+	// secrets, and unreadableTestCurrentFileName the per-secret file
+	// naming its current version.
+	unreadableTestSecretsDirName  = "secrets.d"
+	unreadableTestCurrentFileName = "current"
+)
+
+// errStatFailed is returned by statFailFs in place of a successful stat.
+var errStatFailed = errors.New("input/output error")
+
+// statFailFs fails every Stat of one path, as an I/O or permission error
+// on that path would.
+type statFailFs struct {
+	afero.Fs
+
+	path string
+}
+
+func (f *statFailFs) Stat(name string) (os.FileInfo, error) {
+	if name == f.path {
+		return nil, errStatFailed
+	}
+
+	return f.Fs.Stat(name)
+}
+
+// testVaultDir returns the directory of the named vault in the synthetic
+// state directory built by newListTestVault.
+func testVaultDir(vaultName string) string {
+	return filepath.Join(listTestStateDir, "vaults.d", vaultName)
+}
+
+// newTestInstance returns a CLI instance on fs whose output is discarded.
+func newTestInstance(fs afero.Fs) (*Instance, *cobra.Command) {
+	cmd := &cobra.Command{}
+	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
+
+	return &Instance{fs: fs, stateDir: listTestStateDir, cmd: cmd}, cmd
+}
+
+// assertDirEntries asserts that dir holds exactly the named entries.
+func assertDirEntries(t *testing.T, fs afero.Fs, dir string, want ...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())
+	}
+
+	assert.ElementsMatch(t, want, names)
+}
+
+// newTestGPGKey points GNUPGHOME at a fresh directory, generates a GPG key
+// without a passphrase there, and returns the key's fingerprint.
+func newTestGPGKey(t *testing.T) string {
+	t.Helper()
+
+	t.Setenv("GNUPGHOME", t.TempDir())
+
+	t.Cleanup(func() {
+		// Stop the gpg-agent that key generation starts. t.Context is
+		// already canceled when cleanup runs.
+		ctx := context.WithoutCancel(t.Context())
+		_ = exec.CommandContext(ctx, "gpgconf", "--kill", "gpg-agent").Run()
+	})
+
+	output, err := exec.CommandContext(t.Context(), "gpg", "--batch",
+		"--pinentry-mode", "loopback", "--passphrase", "",
+		"--quick-gen-key", unreadableTestGPGUserID, "ed25519", "sign", "never",
+	).CombinedOutput()
+	require.NoError(t, err, "generating the test GPG key: %s", output)
+
+	fingerprint, err := secret.ResolveGPGKeyFingerprint(unreadableTestGPGUserID)
+	require.NoError(t, err)
+
+	return fingerprint
+}
+
+// addTestPGPUnlocker runs `secret unlocker add pgp` for the test key
+// against fs.
+func addTestPGPUnlocker(fs afero.Fs) error {
+	instance, cmd := newTestInstance(fs)
+	cmd.Flags().String("keyid", unreadableTestGPGUserID, "")
+
+	return instance.addPGPUnlocker(cmd)
+}
+
+// TestAddPGPUnlockerDuplicateCheck asserts that adding a PGP unlocker
+// fails, and creates no unlocker directory, when unlockers.d cannot be
+// read for the duplicate check; and, as the control case, that a readable
+// unlockers.d holding the same key is still refused as a duplicate.
+//
+//nolint:paralleltest // t.Setenv (GNUPGHOME) forbids parallel tests
+func TestAddPGPUnlockerDuplicateCheck(t *testing.T) {
+	fingerprint := newTestGPGKey(t)
+	unlockersDir := filepath.Join(
+		testVaultDir(listTestVaultName), listTestUnlockersDirName)
+
+	tests := []struct {
+		name       string
+		openBudget int
+	}{
+		// The vault's own enumeration of unlockers.d fails.
+		{name: "listing fails", openBudget: 0},
+		// The enumeration succeeds; the rescan that resolves IDs fails.
+		{name: "rescan fails", openBudget: 1},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			base := newListTestVault(t, 1)
+			fs := &unlockersDirFailFs{Fs: base, openBudget: tt.openBudget}
+
+			err := addTestPGPUnlocker(fs)
+
+			require.ErrorIs(t, err, errUnlockersDirUnreadable)
+			require.NotErrorIs(t, err, errGPGKeyAlreadyUnlocker)
+			assert.Contains(t, err.Error(), unlockersDir,
+				"the error must name the directory it could not read")
+			assertDirEntries(t, base, unlockersDir, listTestUnlockerDirOne)
+		})
+	}
+
+	t.Run("duplicate refused", func(t *testing.T) {
+		base := newListTestVault(t, 1)
+		writePGPUnlocker(t, base, unlockersDir, listTestUnlockerDirTwo,
+			time.Date(2026, time.August, 10, 12, 30, 0, 0, time.UTC),
+			fingerprint)
+
+		err := addTestPGPUnlocker(base)
+
+		require.ErrorIs(t, err, errGPGKeyAlreadyUnlocker)
+		assertDirEntries(t, base, unlockersDir,
+			listTestUnlockerDirOne, listTestUnlockerDirTwo)
+	})
+}
+
+// writeTestSecret stores a secret with a current-version pointer, which is
+// what makes it count as a secret, in the given vault directory.
+func writeTestSecret(t *testing.T, fs afero.Fs, vaultDir string) {
+	t.Helper()
+
+	secretDir := filepath.Join(
+		vaultDir, unreadableTestSecretsDirName, unreadableTestSecretName)
+	require.NoError(t, fs.MkdirAll(secretDir, listTestDirPerm))
+	require.NoError(t, afero.WriteFile(fs,
+		filepath.Join(secretDir, unreadableTestCurrentFileName),
+		[]byte("20260809.001"), listTestFilePerm))
+}
+
+// TestRemoveLastUnlockerAbortsWhenSecretsUnreadable asserts that the last
+// unlocker is kept when the secrets it protects cannot be counted.
+func TestRemoveLastUnlockerAbortsWhenSecretsUnreadable(t *testing.T) {
+	t.Parallel()
+
+	vaultDir := testVaultDir(listTestVaultName)
+	unlockersDir := filepath.Join(vaultDir, listTestUnlockersDirName)
+	secretsDir := filepath.Join(vaultDir, unreadableTestSecretsDirName)
+
+	for _, path := range []string{
+		secretsDir,
+		filepath.Join(secretsDir, unreadableTestSecretName,
+			unreadableTestCurrentFileName),
+	} {
+		t.Run(filepath.Base(path), func(t *testing.T) {
+			t.Parallel()
+
+			base := newListTestVault(t, 1)
+			writeTestSecret(t, base, vaultDir)
+			instance, cmd := newTestInstance(&statFailFs{Fs: base, path: path})
+
+			err := instance.UnlockersRemove(
+				"pgp-"+listTestGPGKeyID+"A", false, cmd)
+
+			require.ErrorIs(t, err, errStatFailed)
+			assertDirEntries(t, base, unlockersDir, listTestUnlockerDirOne)
+		})
+	}
+}
+
+// TestRemoveVaultAbortsWhenSecretsDirUnreadable asserts that a vault is
+// kept when whether it holds secrets cannot be determined.
+func TestRemoveVaultAbortsWhenSecretsDirUnreadable(t *testing.T) {
+	t.Parallel()
+
+	base := newListTestVault(t, 1)
+	vaultDir := testVaultDir(unreadableTestOtherVault)
+	writeTestSecret(t, base, vaultDir)
+	instance, cmd := newTestInstance(&statFailFs{
+		Fs: base, path: filepath.Join(vaultDir, unreadableTestSecretsDirName),
+	})
+
+	err := instance.RemoveVault(cmd, unreadableTestOtherVault, false)
+
+	require.ErrorIs(t, err, errStatFailed)
+
+	exists, err := afero.DirExists(base, vaultDir)
+	require.NoError(t, err)
+	assert.True(t, exists, "the vault must not be removed")
+}
+
+// TestVaultImportAbortsWhenPubKeyUnreadable asserts that a mnemonic import
+// stops when whether the vault already has a long-term key cannot be
+// determined.
+func TestVaultImportAbortsWhenPubKeyUnreadable(t *testing.T) {
+	t.Parallel()
+
+	base := newListTestVault(t, 1)
+	instance, cmd := newTestInstance(&statFailFs{
+		Fs: base, path: filepath.Join(testVaultDir(listTestVaultName), "pub.age"),
+	})
+
+	err := instance.VaultImport(cmd, listTestVaultName)
+
+	require.ErrorIs(t, err, errStatFailed)
+}

internal/cli/vault.go

@@ -388,8 +388,12 @@ func (cli *Instance) vaultImportPreflight(
 	// Check if vault already has a public key
 	pubKeyPath := vaultDir + "/pub.age"
 
-	_, err = cli.fs.Stat(pubKeyPath)
-	if err == nil {
+	exists, err = afero.Exists(cli.fs, pubKeyPath)
+	if err != nil {
+		return "", "", "", fmt.Errorf("failed to check %s: %w", pubKeyPath, err)
+	}
+
+	if exists {
 		return "", "", "", fmt.Errorf("vault '%s' %w",
 			vaultName, errVaultHasLongTermKey)
 	}
@@ -536,17 +540,26 @@ func (cli *Instance) VaultImport(cmd *cobra.Command, vaultName string) error {
 }
 
 // vaultHasSecrets reports whether the vault directory contains any secrets
-func (cli *Instance) vaultHasSecrets(vaultDir string) bool {
+func (cli *Instance) vaultHasSecrets(vaultDir string) (bool, error) {
 	secretsDir := filepath.Join(vaultDir, "secrets.d")
 
-	exists, _ := afero.DirExists(cli.fs, secretsDir)
+	exists, err := afero.DirExists(cli.fs, secretsDir)
+	if err != nil {
+		return false, fmt.Errorf("failed to check secrets directory %s: %w",
+			secretsDir, err)
+	}
+
 	if !exists {
-		return false
+		return false, nil
 	}
 
 	entries, err := afero.ReadDir(cli.fs, secretsDir)
+	if err != nil {
+		return false, fmt.Errorf("failed to read secrets directory %s: %w",
+			secretsDir, err)
+	}
 
-	return err == nil && len(entries) > 0
+	return len(entries) > 0, nil
 }
 
 // switchAwayFromVault selects another vault as current before removal
@@ -610,7 +623,10 @@ func (cli *Instance) RemoveVault(cmd *cobra.Command, name string, force bool) er
 	}
 
 	// Check if vault has secrets
-	hasSecrets := cli.vaultHasSecrets(vaultDir)
+	hasSecrets, err := cli.vaultHasSecrets(vaultDir)
+	if err != nil {
+		return err
+	}
 
 	// Require --force if vault has secrets
 	if hasSecrets && !force {

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")
+}

internal/vault/vault.go

@@ -138,7 +138,12 @@ func (v *Vault) NumSecrets() (int, error) {
 
 	secretsDir := filepath.Join(vaultDir, "secrets.d")
 
-	exists, _ := afero.DirExists(v.fs, secretsDir)
+	exists, err := afero.DirExists(v.fs, secretsDir)
+	if err != nil {
+		return 0, fmt.Errorf("failed to check secrets directory %s: %w",
+			secretsDir, err)
+	}
+
 	if !exists {
 		return 0, nil
 	}
@@ -162,7 +167,7 @@ func (v *Vault) NumSecrets() (int, error) {
 
 		exists, err := afero.Exists(v.fs, currentFile)
 		if err != nil {
-			continue // Skip directories we can't read
+			return 0, fmt.Errorf("failed to check %s: %w", currentFile, err)
 		}
 
 		if exists {