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

README.md

@@ -113,9 +113,7 @@ automatically switch to another vault if removing the current one.
 Adds a secret to the current vault. Reads the secret value from stdin.
 - `--force, -f`: Overwrite existing secret
 
-**Secret Name Format:** only ASCII letters, digits, `.`, `-`, `_` and `/`
-are allowed, and a name must not be empty, start with `.` or `/`, end with
-`/`, contain `//`, or have `..` as a path segment.
+**Secret Name Format:** `[a-z0-9\.\-\_\/]+`
 - Forward slashes (`/`) are converted to percent signs (`%`) for storage
 - Examples: `database/password`, `api.key`, `ssh_private_key`
 

TODO.md

@@ -25,18 +25,13 @@ Bring the repo into policy compliance in one commit:
 
 # Completed Steps
 
-- 2026-10-03: Every command that builds a path from a secret name
-  checks the name first with `vault.ValidateSecretName` and touches
-  nothing when it is invalid: `rm`, `mv` (both names, within a vault
-  and between vaults, before switching the current vault), `import`,
-  `version list`/`promote`/`rm`, `encrypt` and `decrypt`. The error
-  and `README.md` state the naming rule. Before, `secret rm ..`
-  deleted the whole vault and `secret rm .` every secret in it.
-- 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: Key material is wiped on every exit: `Entry()` returns
+  the exit code after its deferred `memguard.Purge()` has run, and only
+  `main` calls `os.Exit`. SIGINT and SIGTERM go through memguard's
+  handler, which wipes every buffer before exiting; when the process is
+  in the terminal's foreground process group it first restores the
+  terminal settings from startup, so an interrupted passphrase prompt no
+  longer leaves echo off.
 - 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
@@ -100,14 +95,16 @@ 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: no maximum secret size (DoS).
+    - 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:
       209-216); non-constant-time public key compare (vault.go:95-100).
 - High priority:

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

@@ -122,11 +122,6 @@ func (cli *Instance) resolveEncryptionKey(
 
 // Encrypt encrypts data using an age secret key stored in a secret
 func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
-	err := vault.ValidateSecretName(secretName)
-	if err != nil {
-		return err
-	}
-
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -196,11 +191,6 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
 
 // Decrypt decrypts data using an age secret key stored in a secret
 func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
-	err := vault.ValidateSecretName(secretName)
-	if err != nil {
-		return err
-	}
-
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {

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

@@ -1,267 +0,0 @@
-package cli_test
-
-import (
-	"maps"
-	"os"
-	"slices"
-	"strings"
-	"testing"
-
-	"git.eeqj.de/sneak/secret/internal/cli"
-	"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/require"
-)
-
-const (
-	// testStateDir is the in-memory state directory of the test vaults.
-	testStateDir = "/test/state"
-
-	// testPassphrase protects the passphrase unlocker of each test vault.
-	testPassphrase = "test-passphrase"
-
-	// testVersion is a version name in the format the vault uses.
-	testVersion = "20260101.001"
-
-	// missingFile is an import source that does not exist, so an import
-	// that opened it before checking the name would fail with another error.
-	missingFile = "/no/such/file"
-)
-
-// newTwoVaultFs returns an in-memory filesystem holding the vaults "work"
-// and "default", the current one. Each holds the secret "x" and a
-// passphrase unlocker, so both secrets.d and unlockers.d have contents.
-//
-//nolint:ireturn // afero.Fs is the filesystem abstraction used throughout
-func newTwoVaultFs(t *testing.T) afero.Fs {
-	t.Helper()
-
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-
-	fs := afero.NewMemMapFs()
-
-	for _, name := range []string{"work", "default"} {
-		vlt, err := vault.CreateVault(fs, testStateDir, name)
-		require.NoError(t, err)
-
-		err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("value")), false)
-		require.NoError(t, err)
-
-		_, err = vlt.CreatePassphraseUnlocker(
-			memguard.NewBufferFromBytes([]byte(testPassphrase)))
-		require.NoError(t, err)
-	}
-
-	return fs
-}
-
-// snapshotStateDir maps every file under the state directory to its
-// contents, and every directory, written with a trailing "/", to "". Two
-// snapshots are equal only if nothing in it was added, removed or changed.
-func snapshotStateDir(t *testing.T, fs afero.Fs) map[string]string {
-	t.Helper()
-
-	tree := map[string]string{}
-
-	err := afero.Walk(fs, testStateDir, func(
-		path string, info os.FileInfo, err error,
-	) error {
-		if err != nil {
-			return err
-		}
-
-		if info.IsDir() {
-			tree[path+"/"] = ""
-
-			return nil
-		}
-
-		content, err := afero.ReadFile(fs, path)
-		if err != nil {
-			return err
-		}
-
-		tree[path] = string(content)
-
-		return nil
-	})
-	require.NoError(t, err)
-
-	return tree
-}
-
-// newFsFromSnapshot returns a new in-memory filesystem holding exactly the
-// directories and files recorded by snapshotStateDir.
-//
-//nolint:ireturn // afero.Fs is the filesystem abstraction used throughout
-func newFsFromSnapshot(t *testing.T, tree map[string]string) afero.Fs {
-	t.Helper()
-
-	fs := afero.NewMemMapFs()
-
-	// In sorted order every directory comes before its contents.
-	for _, path := range slices.Sorted(maps.Keys(tree)) {
-		dir, isDir := strings.CutSuffix(path, "/")
-		if isDir {
-			require.NoError(t, fs.MkdirAll(dir, secret.DirPerms))
-
-			continue
-		}
-
-		err := afero.WriteFile(fs, path, []byte(tree[path]), secret.FilePerms)
-		require.NoError(t, err)
-	}
-
-	return fs
-}
-
-// requireRejectedAndUnchanged runs a command on a copy of the state
-// directory recorded in before. It requires exactly the error
-// vault.ValidateSecretName gives for the rejected name, so that a later
-// check rejecting the name does not count, and everything under the state
-// directory as it was: the error alone proves nothing, since it could come
-// after the vault had already been deleted.
-func requireRejectedAndUnchanged(
-	t *testing.T, before map[string]string, rejected string,
-	run func(c *cli.Instance) error,
-) {
-	t.Helper()
-
-	fs := newFsFromSnapshot(t, before)
-
-	err := run(cli.NewCLIInstanceWithStateDir(fs, testStateDir))
-
-	require.Equal(t, before, snapshotStateDir(t, fs))
-	require.ErrorIs(t, err, vault.ErrInvalidSecretName)
-	require.EqualError(t, err, vault.ValidateSecretName(rejected).Error())
-}
-
-// TestInvalidSecretNameLeavesVaultsUnchanged is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/33, where `secret rm ..` deleted
-// the whole vault, and `secret rm .` or `secret rm ""` every secret in it.
-// Moves and imports use --force, so that only the name check stands in
-// the way.
-//
-//nolint:paralleltest // newTwoVaultFs uses t.Setenv
-func TestInvalidSecretNameLeavesVaultsUnchanged(t *testing.T) {
-	// Creating a passphrase unlocker is slow by design, so the vaults are
-	// created once and each case runs on its own copy of them.
-	before := snapshotStateDir(t, newTwoVaultFs(t))
-
-	vaultDir := testStateDir + "/vaults.d/default"
-	require.Contains(t, before, vaultDir+"/secrets.d/x/")
-	require.Contains(t, before, vaultDir+"/unlockers.d/passphrase/")
-	require.Equal(t, "default", before[testStateDir+"/currentvault"])
-
-	cmd := &cobra.Command{}
-
-	tests := []struct {
-		command  string
-		rejected string // the secret name the command must reject
-		run      func(c *cli.Instance) error
-	}{
-		{"rm ..", "..", func(c *cli.Instance) error {
-			return c.RemoveSecret(cmd, "..", false)
-		}},
-		{"rm .", ".", func(c *cli.Instance) error {
-			return c.RemoveSecret(cmd, ".", false)
-		}},
-		{`rm ""`, "", func(c *cli.Instance) error {
-			return c.RemoveSecret(cmd, "", false)
-		}},
-		{"rm ../../etc", "../../etc", func(c *cli.Instance) error {
-			return c.RemoveSecret(cmd, "../../etc", false)
-		}},
-		{"mv --force .. x", "..", func(c *cli.Instance) error {
-			return c.MoveSecret(cmd, "..", "x", true)
-		}},
-		{"mv --force x ..", "..", func(c *cli.Instance) error {
-			return c.MoveSecret(cmd, "x", "..", true)
-		}},
-		{`mv --force x ""`, "", func(c *cli.Instance) error {
-			return c.MoveSecret(cmd, "x", "", true)
-		}},
-		// "work" is not the current vault: a move within it must not
-		// select it when a name is rejected.
-		{"mv --force work:.. work:x", "..", func(c *cli.Instance) error {
-			return c.MoveSecret(cmd, "work:..", "work:x", true)
-		}},
-		{"mv --force work:x work:..", "..", func(c *cli.Instance) error {
-			return c.MoveSecret(cmd, "work:x", "work:..", true)
-		}},
-		{"mv --force default:.. work", "..", func(c *cli.Instance) error {
-			return c.MoveSecret(cmd, "default:..", "work", true)
-		}},
-		{"mv --force default:.. work:y", "..", func(c *cli.Instance) error {
-			return c.MoveSecret(cmd, "default:..", "work:y", true)
-		}},
-		{"mv --force default:x work:..", "..", func(c *cli.Instance) error {
-			return c.MoveSecret(cmd, "default:x", "work:..", true)
-		}},
-		{"import --force ..", "..", func(c *cli.Instance) error {
-			return c.ImportSecret(cmd, "..", missingFile, true)
-		}},
-		{"import --force .", ".", func(c *cli.Instance) error {
-			return c.ImportSecret(cmd, ".", missingFile, true)
-		}},
-		{"import --force ../../etc", "../../etc", func(c *cli.Instance) error {
-			return c.ImportSecret(cmd, "../../etc", missingFile, true)
-		}},
-		{"version list ..", "..", func(c *cli.Instance) error {
-			return c.ListVersions(cmd, "..")
-		}},
-		{"version promote ..", "..", func(c *cli.Instance) error {
-			return c.PromoteVersion(cmd, "..", testVersion)
-		}},
-		{"version rm ..", "..", func(c *cli.Instance) error {
-			return c.RemoveVersion(cmd, "..", testVersion)
-		}},
-		{"encrypt ..", "..", func(c *cli.Instance) error {
-			return c.Encrypt("..", "", "")
-		}},
-		{"decrypt ..", "..", func(c *cli.Instance) error {
-			return c.Decrypt("..", "", "")
-		}},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.command, func(t *testing.T) {
-			requireRejectedAndUnchanged(t, before, tt.rejected, tt.run)
-		})
-	}
-}
-
-// TestMoveToVaultNameRenamesInCurrentVault checks that `secret mv x work`,
-// where "work" is also the name of a vault, renames the secret "x" to "work"
-// in the current vault and changes nothing else.
-//
-//nolint:paralleltest // newTwoVaultFs uses t.Setenv
-func TestMoveToVaultNameRenamesInCurrentVault(t *testing.T) {
-	before := snapshotStateDir(t, newTwoVaultFs(t))
-	fs := newFsFromSnapshot(t, before)
-
-	c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
-	err := c.MoveSecret(&cobra.Command{}, "x", "work", false)
-	require.NoError(t, err)
-
-	// Expected: the state as before, with everything under the current
-	// vault's secrets.d/x/ now under secrets.d/work/.
-	oldDir := testStateDir + "/vaults.d/default/secrets.d/x/"
-	newDir := testStateDir + "/vaults.d/default/secrets.d/work/"
-	want := map[string]string{}
-
-	for path, content := range before {
-		rest, found := strings.CutPrefix(path, oldDir)
-		if found {
-			path = newDir + rest
-		}
-
-		want[path] = content
-	}
-
-	require.Contains(t, want, newDir)
-	require.Equal(t, want, snapshotStateDir(t, fs))
-}

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
-func Entry() {
-	cmd := newRootCmd()
+// Entry runs the secret CLI and returns the process exit code. It wipes
+// every memguard buffer before it returns, so the caller must do nothing
+// 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()
-	if err != nil {
-		os.Exit(1)
+	memguard.CatchSignal(func(os.Signal) {
+		foreground, err := unix.IoctlGetInt(unix.Stdin, unix.TIOCGPGRP)
+		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

@@ -603,11 +603,6 @@ func printSecretsTable(
 func (cli *Instance) ImportSecret(
 	cmd *cobra.Command, secretName, sourceFile string, force bool,
 ) error {
-	err := vault.ValidateSecretName(secretName)
-	if err != nil {
-		return err
-	}
-
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -654,11 +649,6 @@ func (cli *Instance) ImportSecret(
 
 // RemoveSecret removes a secret from the vault
 func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool) error {
-	err := vault.ValidateSecretName(secretName)
-	if err != nil {
-		return err
-	}
-
 	// Get current vault
 	currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -712,8 +702,13 @@ func (cli *Instance) MoveSecret(
 	srcVaultName, srcSecretName, srcQualified := ParseVaultSecretRef(source)
 	destVaultName, destSecretName, destQualified := ParseVaultSecretRef(dest)
 
+	// If neither is qualified, this is a simple within-vault rename
+	if !srcQualified && !destQualified {
+		return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force)
+	}
+
 	// Cross-vault move requires source to be qualified
-	if !srcQualified && destQualified {
+	if !srcQualified {
 		return errCrossVaultSourceUnqualified
 	}
 
@@ -721,46 +716,31 @@ func (cli *Instance) MoveSecret(
 	// Format: "work:secret default" means move to vault "default"
 	// Format: "work:secret default:newname" means move to vault "default"
 	// with a new name
-	if srcQualified && !destQualified {
+	if !destQualified {
 		// Check if dest is actually a vault name
 		vaults, err := vault.ListVaults(cli.fs, cli.stateDir)
 		if err == nil && slices.Contains(vaults, dest) {
 			// dest is a vault name, use source secret name
 			destVaultName = dest
 			destSecretName = srcSecretName
-		} else {
-			// dest is a secret name in source vault
+		}
+
+		// If destVaultName is still empty, dest is a secret name in source vault
+		if destVaultName == "" {
 			destVaultName = srcVaultName
+			destSecretName = dest
 		}
 	}
 
-	// If destination secret name is empty, use source secret name. A plain
-	// rename keeps it empty, so that the check below rejects it.
-	if srcQualified && destSecretName == "" {
+	// If destination secret name is empty, use source secret name
+	if destSecretName == "" {
 		destSecretName = srcSecretName
 	}
 
-	// Check both names, for every form of the move, before selecting a vault
-	// below, so that a rejected move leaves the current vault as it was.
-	err := vault.ValidateSecretName(srcSecretName)
-	if err != nil {
-		return err
-	}
-
-	err = vault.ValidateSecretName(destSecretName)
-	if err != nil {
-		return err
-	}
-
-	// If neither is qualified, this is a simple within-vault rename
-	if !srcQualified && !destQualified {
-		return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force)
-	}
-
 	// Same vault? Use simple rename if possible (optimization)
 	if srcVaultName == destVaultName {
 		// Select the vault and do a simple move
-		err = vault.SelectVault(cli.fs, cli.stateDir, srcVaultName)
+		err := vault.SelectVault(cli.fs, cli.stateDir, srcVaultName)
 		if err != nil {
 			return fmt.Errorf("failed to select vault '%s': %w", srcVaultName, err)
 		}
@@ -773,8 +753,7 @@ func (cli *Instance) MoveSecret(
 		cmd, srcVaultName, srcSecretName, destVaultName, destSecretName, force)
 }
 
-// moveSecretWithinVault handles rename within the current vault. Its caller,
-// MoveSecret, has already checked both secret names.
+// moveSecretWithinVault handles rename within the current vault
 func (cli *Instance) moveSecretWithinVault(
 	cmd *cobra.Command, source, dest string, force bool,
 ) error {
@@ -829,8 +808,7 @@ func (cli *Instance) moveSecretWithinVault(
 	return nil
 }
 
-// moveSecretCrossVault handles moving between different vaults. Its caller,
-// MoveSecret, has already checked both secret names.
+// moveSecretCrossVault handles moving between different vaults
 func (cli *Instance) moveSecretCrossVault(
 	cmd *cobra.Command,
 	srcVaultName, srcSecretName,

internal/cli/version.go

@@ -112,11 +112,6 @@ func VersionCommands(cli *Instance) *cobra.Command {
 func (cli *Instance) ListVersions(cmd *cobra.Command, secretName string) error {
 	secret.Debug("ListVersions called", "secret_name", secretName)
 
-	err := vault.ValidateSecretName(secretName)
-	if err != nil {
-		return err
-	}
-
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -244,11 +239,6 @@ func formatVersionTime(t *time.Time) string {
 func (cli *Instance) PromoteVersion(
 	cmd *cobra.Command, secretName string, version string,
 ) error {
-	err := vault.ValidateSecretName(secretName)
-	if err != nil {
-		return err
-	}
-
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -292,11 +282,6 @@ func (cli *Instance) PromoteVersion(
 func (cli *Instance) RemoveVersion(
 	cmd *cobra.Command, secretName string, version string,
 ) error {
-	err := vault.ValidateSecretName(secretName)
-	if err != nil {
-		return err
-	}
-
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {

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/secret/secret_test.go

@@ -310,6 +310,64 @@ func TestPerSecretKeyFunctionality(t *testing.T) {
 	})
 }
 
+// For testing purposes only
+func isValidSecretName(name string) bool {
+	if name == "" {
+		return false
+	}
+	// Valid characters for secret names: letters, numbers, dash, dot, underscore, slash
+	for _, char := range name {
+		if (char < 'a' || char > 'z') && // lowercase letters
+			(char < 'A' || char > 'Z') && // uppercase letters
+			(char < '0' || char > '9') && // numbers
+			char != '-' && // dash
+			char != '.' && // dot
+			char != '_' && // underscore
+			char != '/' { // slash
+			return false
+		}
+	}
+
+	return true
+}
+
+func TestSecretNameValidation(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name  string
+		valid bool
+	}{
+		{"valid-name", true},
+		{"valid.name", true},
+		{"valid_name", true},
+		{"valid/path/name", true},
+		{"123valid", true},
+		{"", false},
+		{"Valid-Upper-Name", true}, // uppercase allowed
+		{"2025-11-21-ber1app1-vaultik-test-bucket-AKI", true}, // real-world uppercase key ID
+		{"MixedCase/Path/Name", true},                         // mixed case with path
+		{"invalid name", false},                               // space not allowed
+		{"invalid@name", false},                               // @ not allowed
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			result := isValidSecretName(test.name)
+			if result != test.valid {
+				t.Errorf(
+					"isValidSecretName(%q) = %v, want %v",
+					test.name,
+					result,
+					test.valid,
+				)
+			}
+		})
+	}
+}
+
 func TestSecretGetValueWithEnvMnemonicUsesVaultDerivationIndex(t *testing.T) {
 	// This test demonstrates the bug where GetValue uses hardcoded index 0
 	// instead of the vault's actual derivation index when using environment mnemonic

internal/vault/errors.go

@@ -29,11 +29,10 @@ var (
 	// ErrNilValueBuffer indicates a nil value buffer was supplied.
 	ErrNilValueBuffer = errors.New("value buffer is nil")
 
-	// ErrInvalidSecretName indicates a secret name that breaks the naming
-	// rule: only ASCII letters, digits, '.', '-', '_' and '/'; not empty;
-	// no leading '.' or '/', no trailing '/', no '//', no '..' path segment.
-	// Composed by ValidateSecretName as
-	// "invalid secret name '<name>': <the rule>".
+	// ErrInvalidSecretName indicates a secret name that does not match
+	// the allowed pattern [a-z0-9.\-_/]+. Composed as
+	// "invalid secret name '<name>': must match pattern [a-z0-9.\-_/]+",
+	// or as "invalid secret name: <name>" by GetSecretObject.
 	ErrInvalidSecretName = errors.New("invalid secret name")
 
 	// ErrSecretExists indicates the secret already exists and --force

internal/vault/secrets.go

@@ -79,7 +79,6 @@ func (v *Vault) ListSecrets() ([]string, error) {
 // - No leading or trailing slashes
 // - No double slashes
 // - No names starting with dots
-// - No ".." path segments
 func isValidSecretName(name string) bool {
 	if name == "" {
 		return false
@@ -111,22 +110,6 @@ func isValidSecretName(name string) bool {
 	return matched
 }
 
-// ValidateSecretName returns an error wrapping ErrInvalidSecretName when
-// name is not a valid secret name. Call it on the name exactly as the user
-// gave it, before building any path from it.
-func ValidateSecretName(name string) error {
-	if !isValidSecretName(name) {
-		return fmt.Errorf(
-			"%w '%s': only ASCII letters, digits, '.', '-', '_' and '/' are allowed, "+
-				"and a name must not be empty, start with '.' or '/', end with '/', "+
-				"contain '//', or have '..' as a path segment",
-			ErrInvalidSecretName, name,
-		)
-	}
-
-	return nil
-}
-
 // AddSecret adds a secret to this vault
 func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool) error {
 	if value == nil {
@@ -141,11 +124,13 @@ func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool)
 	)
 
 	// Validate secret name
-	err := ValidateSecretName(name)
-	if err != nil {
+	if !isValidSecretName(name) {
 		secret.Debug("Invalid secret name provided", "secret_name", name)
 
-		return err
+		return fmt.Errorf(
+			"%w '%s': must match pattern [a-z0-9.\\-_/]+",
+			ErrInvalidSecretName, name,
+		)
 	}
 
 	secret.Debug("Secret name validation passed", "secret_name", name)
@@ -373,9 +358,8 @@ func (v *Vault) UnlockVault() (*age.X25519Identity, error) {
 
 // GetSecretObject retrieves a Secret object with metadata loaded from this vault
 func (v *Vault) GetSecretObject(name string) (*secret.Secret, error) {
-	err := ValidateSecretName(name)
-	if err != nil {
-		return nil, err
+	if !isValidSecretName(name) {
+		return nil, fmt.Errorf("%w: %s", ErrInvalidSecretName, name)
 	}
 
 	// First check if the secret exists by checking for the metadata file
@@ -644,11 +628,13 @@ func (v *Vault) updatePreviousVersion(
 // version exist, and resolves an empty version to the current one.
 func (v *Vault) resolveSecretVersion(name, version string) (string, error) {
 	// Validate secret name to prevent path traversal
-	err := ValidateSecretName(name)
-	if err != nil {
+	if !isValidSecretName(name) {
 		secret.Debug("Invalid secret name provided", "secret_name", name)
 
-		return "", err
+		return "", fmt.Errorf(
+			"%w '%s': must match pattern [a-z0-9.\\-_/]+",
+			ErrInvalidSecretName, name,
+		)
 	}
 
 	// Get vault directory