Skip corrupt unlocker metadata in unlocker select and remove (closes #72)

findUnlockerByID failed on the first unlocker directory whose metadata
could not be checked, read or parsed, so `secret unlocker select` and
`remove` failed when one sorted before the unlocker asked for. It now
skips such a directory with the warning ListUnlockers gives, through
the code both now share. A skipped directory is removed by its
directory name, with RemoveDirAtomic, and cannot be selected. Removing
one whose metadata file is missing or corrupt never counts as removing
the last unlocker; removing one whose metadata file cannot be checked
for or read does, since it may be the only working unlocker.

Model: opus-5-5

TODO.md

@@ -31,7 +31,26 @@ Bring the repo into policy compliance in one commit:
   failing when it sorts before the unlocker asked for. Such a directory,
   or one without a metadata file, is removed by its directory name, the
   name the warning gives; only the directory is removed, since its type
-  is unknown. Removing it never counts as removing the last unlocker.
+  is unknown. Removing one whose metadata file is missing or corrupt
+  never counts as removing the last unlocker. Removing one whose metadata
+  file cannot be checked for or read always does, since it may be the
+  only working unlocker, so in a vault with secrets it needs `--force`.
+- 2026-10-04: A failed command prints its error once, without the usage
+  text after it (https://git.eeqj.de/sneak/secret/issues/41). Usage is
+  still printed for a command called wrongly: wrong number of arguments,
+  unknown flag, bad flag value, missing required flag, or flags that
+  break a flag group (mutually exclusive, required together, one
+  required). The root command's `PersistentPreRunE` turns usage off.
+  Cobra checks arguments and flag values before that hook but required
+  flags and flag groups only after it, so the hook checks those two
+  first. Root `SilenceUsage` would have hidden usage for all of these.
+- 2026-10-04: `secret get` keeps the secret in locked memory until it
+  writes it out (https://git.eeqj.de/sneak/secret/issues/37):
+  `Vault.GetSecret` and `Vault.GetSecretVersion` return a
+  `*memguard.LockedBuffer`, which every caller destroys, and `secret get`
+  writes its bytes straight to stdout, still with no trailing newline.
+  Before, the value was copied into ordinary memory that nothing wiped,
+  and `get --version` also wrote it to the debug log.
 - 2026-10-04: The `Makefile` no longer sets `DOCKER_HOST`, so its docker
   targets use the local docker daemon, or whatever `DOCKER_HOST` the
   environment sets. `make build` calls the new `script/build`, which
@@ -224,8 +243,6 @@ Bring the repo into policy compliance in one commit:
       209-216); non-constant-time public key compare (vault.go:95-100).
 - High priority:
     - Secure temporary file handling and cleanup.
-    - Print cobra usage only for argument errors, not internal
-      failures.
     - Initialize a default unlock key at vault creation.
     - Confirmation prompts for destructive operations (keys rm, vault
       deletion).

internal/cli/cli.go

@@ -68,8 +68,3 @@ func (cli *Instance) SetStateDir(stateDir string) {
 func (cli *Instance) GetStateDir() string {
 	return cli.stateDir
 }
-
-// Print outputs to the command's configured output writer
-func (cli *Instance) Print(a ...any) (int, error) {
-	return fmt.Fprint(cli.cmd.OutOrStdout(), a...)
-}

internal/cli/create_vault_test.go

@@ -1,6 +1,7 @@
 package cli_test
 
 import (
+	"bytes"
 	"testing"
 
 	"git.eeqj.de/sneak/secret/internal/cli"
@@ -89,7 +90,11 @@ func TestCreateExistingVaultChangesNothing(t *testing.T) {
 	for _, name := range vaults {
 		value, err := vault.NewVault(fs, testStateDir, name).GetSecret("x")
 		require.NoError(t, err)
-		require.Equal(t, "value", string(value))
+
+		unchanged := bytes.Equal([]byte("value"), value.Bytes())
+		value.Destroy()
+
+		require.True(t, unchanged, "vault %q kept its secret", name)
 	}
 }
 

internal/cli/lock_test.go

@@ -147,7 +147,8 @@ func TestConcurrentAddsKeepEveryVersion(t *testing.T) {
 				value, err := vlt.GetSecretVersion("shared", version)
 				require.NoError(t, err)
 
-				values[string(value)] = true
+				values[string(value.Bytes())] = true
+				value.Destroy()
 			}
 
 			assert.Len(t, values, adds+1, "every add stored its own value")

internal/cli/move_test.go

@@ -178,7 +178,10 @@ func TestMoveOntoSameSecretUnderAnotherNameIsRejected(t *testing.T) {
 
 			value, err := vlt.GetSecret("x")
 			require.NoError(t, err)
-			require.Equal(t, "value", string(value))
+
+			defer value.Destroy()
+
+			require.Equal(t, []byte("value"), value.Bytes())
 
 			target, err := os.Readlink(link)
 			require.NoError(t, err)
@@ -222,7 +225,10 @@ func TestForcedCaseOnlyMoveOnCaseSensitiveFilesystem(t *testing.T) {
 
 	value, err := vlt.GetSecret("foo")
 	require.NoError(t, err)
-	require.Equal(t, "upper", string(value))
+
+	defer value.Destroy()
+
+	require.Equal(t, []byte("upper"), value.Bytes())
 
 	_, err = vlt.GetSecret("Foo")
 	require.ErrorIs(t, err, vault.ErrSecretNotFound)

internal/cli/root.go

@@ -46,9 +46,30 @@ func newRootCmd() *cobra.Command {
 		Short: "A simple secrets manager",
 		Long: `A simple secrets manager to store and retrieve sensitive ` +
 			`information securely.`,
-		// Ensure usage is shown after errors
-		SilenceUsage:  false,
+		// Cobra prints the error a command returns; Entry does not.
 		SilenceErrors: false,
+		// Usage belongs only to a command called wrongly. Cobra has
+		// checked its arguments and flag values before this runs, but
+		// checks required flags (ValidateRequiredFlags) and flag groups
+		// (ValidateFlagGroups) only after it, so both are checked here
+		// to keep usage for them. An error after that comes from running
+		// the command, and usage would only bury it. A subcommand that
+		// sets its own PersistentPreRun replaces this one.
+		PersistentPreRunE: func(cmd *cobra.Command, _ []string) error {
+			err := cmd.ValidateRequiredFlags()
+			if err != nil {
+				return err
+			}
+
+			err = cmd.ValidateFlagGroups()
+			if err != nil {
+				return err
+			}
+
+			cmd.SilenceUsage = true
+
+			return nil
+		},
 	}
 
 	secret.Debug("Adding subcommands to root command")

internal/cli/secrets.go

@@ -414,9 +414,6 @@ func (cli *Instance) AddSecret(secretName string, force bool) error {
 func (cli *Instance) GetSecret(cmd *cobra.Command, secretName string) error {
 	secret.Debug("GetSecret called", "secretName", secretName)
 
-	// Store the command for output
-	cli.cmd = cmd
-
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -427,9 +424,10 @@ func (cli *Instance) GetSecret(cmd *cobra.Command, secretName string) error {
 	if err != nil {
 		return err
 	}
+	defer value.Destroy()
 
-	// Print the secret value to stdout
-	_, _ = cli.Print(string(value))
+	// Write the value straight from locked memory, with no trailing newline
+	_, _ = cmd.OutOrStdout().Write(value.Bytes())
 
 	return nil
 }
@@ -442,9 +440,6 @@ func (cli *Instance) GetSecretWithVersion(
 	secret.Debug("GetSecretWithVersion called",
 		"secretName", secretName, "version", version)
 
-	// Store the command for output
-	cli.cmd = cmd
-
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -460,22 +455,15 @@ func (cli *Instance) GetSecretWithVersion(
 
 		return err
 	}
+	defer value.Destroy()
 
-	secret.Debug("Got secret value", "valueLength", len(value))
+	secret.Debug("Got secret value", "valueLength", value.Size())
 
-	// Print the secret value to stdout
-	_, _ = cli.Print(string(value))
+	// Write the value straight from locked memory, with no trailing newline
+	_, _ = cmd.OutOrStdout().Write(value.Bytes())
 
 	secret.Debug("Printed value to stdout")
 
-	// Debug: Log what we're actually printing
-	secret.Debug("Secret retrieval debug info",
-		"secretName", secretName,
-		"version", version,
-		"valueLength", len(value),
-		"valueAsString", string(value),
-		"isEmpty", len(value) == 0)
-
 	return nil
 }
 

internal/cli/secrets_size_test.go

@@ -143,7 +143,10 @@ func runAddSecretSizeCase(t *testing.T, size int, wantErr bool, errMsg string) {
 	// Verify the secret was stored correctly
 	retrievedValue, err := vlt.GetSecret(secretName)
 	require.NoError(t, err)
-	assert.Equal(t, testData, retrievedValue,
+
+	defer retrievedValue.Destroy()
+
+	assert.Equal(t, testData, retrievedValue.Bytes(),
 		"Retrieved secret should match original (without newline)")
 }
 
@@ -193,7 +196,11 @@ func runImportSecretSizeCase(t *testing.T, size int, wantErr bool, errMsg string
 	// Verify the secret was stored correctly
 	retrievedValue, err := vlt.GetSecret(secretName)
 	require.NoError(t, err)
-	assert.Equal(t, testData, retrievedValue, "Retrieved secret should match original")
+
+	defer retrievedValue.Destroy()
+
+	assert.Equal(t, testData, retrievedValue.Bytes(),
+		"Retrieved secret should match original")
 }
 
 // TestAddSecretVariousSizes tests adding secrets of various sizes through stdin
@@ -375,7 +382,10 @@ func TestAddSecretBufferGrowth(t *testing.T) {
 			// Verify the secret was stored correctly
 			retrievedValue, err := vlt.GetSecret(secretName)
 			require.NoError(t, err)
-			assert.Equal(t, testData, retrievedValue,
+
+			defer retrievedValue.Destroy()
+
+			assert.Equal(t, testData, retrievedValue.Bytes(),
 				"Retrieved secret should match original exactly")
 		})
 	}
@@ -416,7 +426,11 @@ func TestAddSecretStreamingBehavior(t *testing.T) {
 	// Verify the secret was stored correctly
 	retrievedValue, err := vlt.GetSecret("streaming-test")
 	require.NoError(t, err)
-	assert.Equal(t, testData, retrievedValue, "Retrieved secret should match original")
+
+	defer retrievedValue.Destroy()
+
+	assert.Equal(t, testData, retrievedValue.Bytes(),
+		"Retrieved secret should match original")
 }
 
 // slowReader simulates a reader that returns data in small chunks

internal/cli/unlockers.go

@@ -744,25 +744,26 @@ func (cli *Instance) removeUnlocker(
 		return err
 	}
 
-	// Get list of unlockers. It leaves out a directory whose metadata
-	// cannot be read or parsed: that unlocker does not work, so removing it
-	// by its directory name never removes the last unlocker.
+	// Get list of unlockers. It leaves out a directory whose metadata file
+	// is missing or cannot be checked for, read or parsed.
 	unlockers, err := vlt.ListUnlockers()
 	if err != nil {
 		return fmt.Errorf("failed to list unlockers: %w", err)
 	}
 
+	vaultDir, err := vlt.GetDirectory()
+	if err != nil {
+		return fmt.Errorf("failed to get vault directory: %w", err)
+	}
+
+	unlockersDir := filepath.Join(vaultDir, "unlockers.d")
+
 	// Check if we're removing the last unlocker
 	removingLast := false
 
 	if len(unlockers) == 1 {
-		vaultDir, err := vlt.GetDirectory()
-		if err != nil {
-			return fmt.Errorf("failed to get vault directory: %w", err)
-		}
-
-		lastID, err := findUnlockerIDByMetadata(cli.fs,
-			filepath.Join(vaultDir, "unlockers.d"), unlockers[0], true)
+		lastID, err := findUnlockerIDByMetadata(
+			cli.fs, unlockersDir, unlockers[0], true)
 		if err != nil {
 			return err
 		}
@@ -770,6 +771,15 @@ func (cli *Instance) removeUnlocker(
 		removingLast = lastID == unlockerID
 	}
 
+	// unlockerID may instead name a directory left out of the list. If its
+	// metadata file is missing or corrupt it is not a working unlocker, so
+	// removing it never removes the last one. If the file cannot be checked
+	// for or read, the unlocker may be the only working one, so removing it
+	// counts as removing the last unlocker.
+	if metadataUnreadable(cli.fs, filepath.Join(unlockersDir, unlockerID)) {
+		removingLast = true
+	}
+
 	if removingLast {
 		// Check if vault has secrets
 		numSecrets, err := vlt.NumSecrets()
@@ -804,6 +814,20 @@ func (cli *Instance) removeUnlocker(
 	return nil
 }
 
+// metadataUnreadable reports whether checking for or reading the metadata
+// file in the unlocker directory unlockerDir fails. A missing file is not
+// a failure.
+func metadataUnreadable(fs afero.Fs, unlockerDir string) bool {
+	metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json")
+
+	exists, err := afero.Exists(fs, metadataPath)
+	if err == nil && exists {
+		_, err = afero.ReadFile(fs, metadataPath)
+	}
+
+	return err != nil
+}
+
 // UnlockerSelect selects an unlocker as current
 func (cli *Instance) UnlockerSelect(unlockerID string) error {
 	release, err := vault.LockStateDir(cli.fs, cli.stateDir)

internal/cli/unlockers_corrupt_test.go

@@ -4,7 +4,9 @@
 // by its ID. These tests give the first unlocker, which sorts before the
 // one the commands act on, metadata that is not JSON, and check that the
 // commands step past it, and that it can itself be removed by its
-// directory name, which `secret unlocker list` names in its warning.
+// directory name, which `secret unlocker list` names in its warning. A
+// last test checks that an unlocker whose metadata file cannot be read is
+// removed by its directory name only as the last unlocker is.
 
 //nolint:testpackage // white-box test of unexported internals
 package cli
@@ -110,3 +112,56 @@ func TestUnlockerRemoveWithCorruptUnlocker(t *testing.T) {
 		})
 	}
 }
+
+// TestUnlockerRemoveWithUnreadableMetadata asserts that removing the only
+// unlocker of a vault with secrets by its directory name, when its
+// metadata file cannot be checked for or read, is refused without --force:
+// listing leaves it out, but it may still be the vault's only working
+// unlocker. With --force it is removed. The state directory lock refuses
+// the failing filesystem, so the test calls removeUnlocker, which
+// UnlockersRemove runs once it holds the lock.
+func TestUnlockerRemoveWithUnreadableMetadata(t *testing.T) {
+	t.Parallel()
+
+	vaultDir := testVaultDir(listTestVaultName)
+	unlockersDir := filepath.Join(vaultDir, listTestUnlockersDirName)
+	failingPath := filepath.Join(unlockersDir, listTestUnlockerDirOne,
+		listTestMetadataFileName)
+
+	tests := []struct {
+		name string
+		wrap func(base afero.Fs) afero.Fs
+	}{
+		{
+			name: "checking for the file fails",
+			wrap: func(base afero.Fs) afero.Fs {
+				return &metadataStatFailFs{Fs: base, uncheckablePath: failingPath}
+			},
+		},
+		{
+			name: "reading the file fails",
+			wrap: func(base afero.Fs) afero.Fs {
+				return &metadataReadFailFs{Fs: base, unreadablePath: failingPath}
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			base := newListTestVault(t, 1)
+			writeTestSecret(t, base, vaultDir)
+
+			instance, cmd := newTestInstance(tt.wrap(base))
+
+			err := instance.removeUnlocker(listTestUnlockerDirOne, false, cmd)
+			require.ErrorIs(t, err, errLastUnlocker)
+			assertDirEntries(t, base, unlockersDir, listTestUnlockerDirOne)
+
+			require.NoError(t,
+				instance.removeUnlocker(listTestUnlockerDirOne, true, cmd))
+			assertDirEntries(t, base, unlockersDir)
+		})
+	}
+}

internal/cli/usage_test.go

@@ -0,0 +1,46 @@
+package cli_test
+
+import (
+	"strings"
+	"testing"
+
+	"git.eeqj.de/sneak/secret/internal/cli"
+	"git.eeqj.de/sneak/secret/internal/secret"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// usageHeading starts the usage text cobra prints after an error.
+const usageHeading = "Usage:"
+
+// A command called wrongly gets usage after its error; a command that
+// fails while running gets its error alone. Either way the command fails
+// and its error is shown exactly once.
+//
+//nolint:paralleltest // executes the CLI in-process and sets the environment
+func TestUsageOnlyForCallErrors(t *testing.T) {
+	// No vault in the state directory, so `get x` fails while running.
+	env := map[string]string{secret.EnvStateDir: t.TempDir()}
+
+	tests := []struct {
+		call      string
+		wantUsage bool
+	}{
+		{call: "get", wantUsage: true},
+		{call: "get x y", wantUsage: true},
+		{call: "get --no-such-flag x", wantUsage: true},
+		{call: "generate secret x --length abc", wantUsage: true},
+		{call: "import x", wantUsage: true},
+		{call: "get x", wantUsage: false},
+	}
+
+	for _, tt := range tests {
+		output, err := cli.ExecuteCommandInProcess(strings.Fields(tt.call), "", env)
+		require.Error(t, err, "%q should fail", tt.call)
+
+		assert.Equal(t, 1, strings.Count(output, err.Error()),
+			"%q should show its error once:\n%s", tt.call, output)
+		assert.Equal(t, tt.wantUsage, strings.Contains(output, usageHeading),
+			"usage shown for %q:\n%s", tt.call, output)
+	}
+}

internal/cli/version_test.go

@@ -7,6 +7,7 @@
 // - TestPromoteVersionCommand: Tests `secret version promote` command
 // - TestPromoteNonExistentVersion: Tests error handling for invalid promotion
 // - TestGetSecretWithVersion: Tests `secret get --version` flag functionality
+// - TestGetSecretWritesBinaryValue: Tests `secret get` output of binary values
 // - TestVersionCommandStructure: Tests command structure and help text
 // - TestListVersionsEmptyOutput: Tests edge case with no versions
 //
@@ -23,6 +24,7 @@ import (
 	"strings"
 	"testing"
 	"time"
+	"unicode/utf8"
 
 	"git.eeqj.de/sneak/secret/internal/secret"
 	"git.eeqj.de/sneak/secret/internal/vault"
@@ -188,7 +190,10 @@ func TestPromoteVersionCommand(t *testing.T) {
 	// Current should be version-2
 	value, err := vlt.GetSecret("test/secret")
 	require.NoError(t, err)
-	assert.Equal(t, []byte("version-2"), value)
+
+	defer value.Destroy()
+
+	assert.Equal(t, []byte("version-2"), value.Bytes())
 
 	// Promote first version
 	firstVersion := versions[1] // Older version
@@ -211,9 +216,12 @@ func TestPromoteVersionCommand(t *testing.T) {
 	assert.Contains(t, outputStr, firstVersion)
 
 	// Verify current is now version-1
-	value, err = vlt.GetSecret("test/secret")
+	promoted, err := vlt.GetSecret("test/secret")
 	require.NoError(t, err)
-	assert.Equal(t, []byte("version-1"), value)
+
+	defer promoted.Destroy()
+
+	assert.Equal(t, []byte("version-1"), promoted.Bytes())
 }
 
 //nolint:paralleltest // uses t.Setenv via setupTestVault
@@ -290,6 +298,46 @@ func TestGetSecretWithVersion(t *testing.T) {
 	assert.Equal(t, "version-1", buf.String())
 }
 
+//nolint:paralleltest // uses t.Setenv via setupTestVault
+func TestGetSecretWritesBinaryValue(t *testing.T) {
+	fs := afero.NewMemMapFs()
+	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
+
+	setupTestVault(t, fs)
+
+	vlt, err := vault.GetCurrentVault(fs, testStateDir)
+	require.NoError(t, err)
+
+	value := []byte{0x00, 'a', 0xff, 0xfe, 0x00, 0xc3, 0x28, 'z', 0x00}
+	require.False(t, utf8.Valid(value))
+	// A copy, since storing a value wipes the slice it came from
+	addTestSecret(t, vlt, bytes.Clone(value), false)
+
+	vaultDir, err := vlt.GetDirectory()
+	require.NoError(t, err)
+
+	versions, err := secret.ListVersions(fs,
+		filepath.Join(vaultDir, "secrets.d", "test%secret"))
+	require.NoError(t, err)
+	require.Len(t, versions, 1)
+
+	cmd := newRootCmd()
+
+	var buf bytes.Buffer
+	cmd.SetOut(&buf)
+
+	// Each writes exactly the stored bytes, with no trailing newline
+	err = cli.GetSecret(cmd, "test/secret")
+	require.NoError(t, err)
+	assert.Equal(t, value, buf.Bytes())
+
+	buf.Reset()
+
+	err = cli.GetSecretWithVersion(cmd, "test/secret", versions[0])
+	require.NoError(t, err)
+	assert.Equal(t, value, buf.Bytes())
+}
+
 //nolint:paralleltest // reads process environment to determine the state dir
 func TestVersionCommandStructure(t *testing.T) {
 	// Test that version commands are properly structured

internal/secret/atomic_test.go

@@ -342,7 +342,10 @@ func TestLongestNames(t *testing.T) {
 
 	got, err := vlt.GetSecret(name)
 	require.NoError(t, err)
-	assert.Equal(t, "long", string(got))
+
+	defer got.Destroy()
+
+	assert.Equal(t, []byte("long"), got.Bytes())
 
 	vaultDir, err := vlt.GetDirectory()
 	require.NoError(t, err)
@@ -380,7 +383,10 @@ func TestForcedCopyKeepsDestinationUntilReplaced(t *testing.T) {
 
 			value, err := dest.GetSecret("shared")
 			require.NoError(t, err)
-			assert.Equal(t, "old", string(value))
+
+			defer value.Destroy()
+
+			assert.Equal(t, []byte("old"), value.Bytes())
 		})
 	}
 }

internal/vault/integration_test.go

@@ -1,6 +1,7 @@
 package vault_test
 
 import (
+	"bytes"
 	"os"
 	"path/filepath"
 	"slices"
@@ -197,10 +198,11 @@ func testDeepPathSecrets(t *testing.T, fs afero.Fs, tempDir string) {
 	if err != nil {
 		t.Fatalf("Failed to retrieve deep path secret: %v", err)
 	}
+	defer retrievedValue.Destroy()
 
-	if string(retrievedValue) != string(expectedValue) {
+	if !bytes.Equal(retrievedValue.Bytes(), expectedValue) {
 		t.Errorf("Retrieved value doesn't match. Expected %q, got %q",
-			string(expectedValue), string(retrievedValue))
+			expectedValue, retrievedValue.Bytes())
 	}
 }
 

internal/vault/integration_version_test.go

@@ -119,7 +119,10 @@ func testCreateInitialVersion(
 	// Verify secret can be retrieved
 	value, err := vault.GetSecret(secretName)
 	require.NoError(t, err)
-	assert.Equal(t, []byte("version-1-data"), value)
+
+	defer value.Destroy()
+
+	assert.Equal(t, []byte("version-1-data"), value.Bytes())
 
 	// Verify version directory structure
 	secretDir := filepath.Join(vaultDir, "secrets.d", "integration%test")
@@ -166,7 +169,10 @@ func testCreateSecondVersion(
 	// Verify new value is current
 	value, err := vault.GetSecret(secretName)
 	require.NoError(t, err)
-	assert.Equal(t, []byte("version-2-data"), value)
+
+	defer value.Destroy()
+
+	assert.Equal(t, []byte("version-2-data"), value.Bytes())
 
 	// Verify we now have two versions
 	versions, err = secret.ListVersions(fs, secretDir)
@@ -209,7 +215,10 @@ func testCreateThirdVersion(
 	// Current should be version-3
 	value, err := vault.GetSecret(secretName)
 	require.NoError(t, err)
-	assert.Equal(t, []byte("version-3-data"), value)
+
+	defer value.Destroy()
+
+	assert.Equal(t, []byte("version-3-data"), value.Bytes())
 }
 
 func testRetrieveSpecificVersions(
@@ -225,15 +234,24 @@ func testRetrieveSpecificVersions(
 	// Get each version by its name
 	value1, err := vault.GetSecretVersion(secretName, versions[2]) // oldest
 	require.NoError(t, err)
-	assert.Equal(t, []byte("version-1-data"), value1)
+
+	defer value1.Destroy()
+
+	assert.Equal(t, []byte("version-1-data"), value1.Bytes())
 
 	value2, err := vault.GetSecretVersion(secretName, versions[1]) // middle
 	require.NoError(t, err)
-	assert.Equal(t, []byte("version-2-data"), value2)
+
+	defer value2.Destroy()
+
+	assert.Equal(t, []byte("version-2-data"), value2.Bytes())
 
 	value3, err := vault.GetSecretVersion(secretName, versions[0]) // newest
 	require.NoError(t, err)
-	assert.Equal(t, []byte("version-3-data"), value3)
+
+	defer value3.Destroy()
+
+	assert.Equal(t, []byte("version-3-data"), value3.Bytes())
 
 	// An empty version is not one of the versions; GetSecret gets the
 	// current one
@@ -259,7 +277,10 @@ func testPromoteOldVersion(
 	// Verify current now returns the old version's value
 	value, err := vault.GetSecret(secretName)
 	require.NoError(t, err)
-	assert.Equal(t, []byte("version-1-data"), value)
+
+	defer value.Destroy()
+
+	assert.Equal(t, []byte("version-1-data"), value.Bytes())
 
 	// Verify the version metadata hasn't changed
 	// (promoting shouldn't modify timestamps)
@@ -353,8 +374,13 @@ func TestVersionConcurrency(t *testing.T) {
 				value, err := vault.GetSecret(secretName)
 				if err != nil {
 					errCh <- err
-				} else if string(value) != "initial" {
-					errCh <- fmt.Errorf("%w: %s", errUnexpectedValue, value)
+				} else {
+					if value.String() != "initial" {
+						errCh <- fmt.Errorf("%w: %s",
+							errUnexpectedValue, value.Bytes())
+					}
+
+					value.Destroy()
 				}
 
 				done <- true

internal/vault/secrets.go

@@ -301,8 +301,9 @@ func updateVersionMetadata(
 	return nil
 }
 
-// GetSecret retrieves the current version of a secret from this vault
-func (v *Vault) GetSecret(name string) ([]byte, error) {
+// GetSecret retrieves the current version of a secret from this vault.
+// The caller must destroy the returned buffer.
+func (v *Vault) GetSecret(name string) (*memguard.LockedBuffer, error) {
 	secret.DebugWith("Getting secret from vault",
 		slog.String("vault_name", v.Name),
 		slog.String("secret_name", name),
@@ -326,7 +327,10 @@ func (v *Vault) GetSecret(name string) ([]byte, error) {
 
 // GetSecretVersion retrieves a specific version of a secret. The version
 // must be one of the secret's versions; GetSecret gets the current one.
-func (v *Vault) GetSecretVersion(name string, version string) ([]byte, error) {
+// The caller must destroy the returned buffer.
+func (v *Vault) GetSecretVersion(
+	name string, version string,
+) (*memguard.LockedBuffer, error) {
 	secret.DebugWith("Getting secret version from vault",
 		slog.String("vault_name", v.Name),
 		slog.String("secret_name", name),
@@ -372,26 +376,14 @@ func (v *Vault) GetSecretVersion(name string, version string) ([]byte, error) {
 		return nil, fmt.Errorf("failed to decrypt version: %w", err)
 	}
 
-	// Create a copy to return since the buffer will be destroyed
-	result := make([]byte, decryptedValue.Size())
-	copy(result, decryptedValue.Bytes())
-	decryptedValue.Destroy()
-
 	secret.DebugWith("Successfully decrypted secret version",
 		slog.String("secret_name", name),
 		slog.String("version", version),
 		slog.String("vault_name", v.Name),
-		slog.Int("decrypted_length", len(result)),
+		slog.Int("decrypted_length", decryptedValue.Size()),
 	)
 
-	// Debug: Log metadata about the decrypted value without exposing the actual secret
-	secret.Debug("Vault secret decryption debug info",
-		"secret_name", name,
-		"version", version,
-		"decrypted_value_length", len(result),
-		"is_empty", len(result) == 0)
-
-	return result, nil
+	return decryptedValue, nil
 }
 
 // UnlockVault unlocks the vault and returns the long-term private key

internal/vault/secrets_version_test.go

@@ -131,7 +131,10 @@ func TestVaultAddSecretCreatesVersion(t *testing.T) {
 	// Get the secret value
 	retrievedValue, err := vault.GetSecret(testSecretPath)
 	require.NoError(t, err)
-	assert.Equal(t, expectedValue, retrievedValue)
+
+	defer retrievedValue.Destroy()
+
+	assert.Equal(t, expectedValue, retrievedValue.Bytes())
 }
 
 //nolint:paralleltest // createTestVaultWithKey uses t.Setenv
@@ -165,7 +168,10 @@ func TestVaultAddSecretMultipleVersions(t *testing.T) {
 	// Current value should be version-2
 	value, err := vault.GetSecret(testSecretPath)
 	require.NoError(t, err)
-	assert.Equal(t, []byte("version-2"), value)
+
+	defer value.Destroy()
+
+	assert.Equal(t, []byte("version-2"), value.Bytes())
 }
 
 //nolint:paralleltest // createTestVaultWithKey uses t.Setenv
@@ -192,15 +198,21 @@ func TestVaultGetSecretVersion(t *testing.T) {
 
 	// Get specific version (first one)
 	firstVersion := versions[1] // Last in list is first created
-	value, err := vault.GetSecretVersion(testSecretPath, firstVersion)
+	first, err := vault.GetSecretVersion(testSecretPath, firstVersion)
 	require.NoError(t, err)
-	assert.Equal(t, []byte("version-1"), value)
+
+	defer first.Destroy()
+
+	assert.Equal(t, []byte("version-1"), first.Bytes())
 
 	// Get specific version (second one)
 	secondVersion := versions[0] // First in list is most recent
-	value, err = vault.GetSecretVersion(testSecretPath, secondVersion)
+	second, err := vault.GetSecretVersion(testSecretPath, secondVersion)
 	require.NoError(t, err)
-	assert.Equal(t, []byte("version-2"), value)
+
+	defer second.Destroy()
+
+	assert.Equal(t, []byte("version-2"), second.Bytes())
 
 	// An empty version is not one of the versions; GetSecret gets the
 	// current one

internal/vault/vault_test.go

@@ -1,6 +1,7 @@
 package vault_test
 
 import (
+	"bytes"
 	"path/filepath"
 	"slices"
 	"testing"
@@ -184,10 +185,11 @@ func testSecretOperations(t *testing.T, fs afero.Fs) {
 	if err != nil {
 		t.Fatalf("Failed to get secret: %v", err)
 	}
+	defer retrievedValue.Destroy()
 
-	if string(retrievedValue) != string(expectedValue) {
+	if !bytes.Equal(retrievedValue.Bytes(), expectedValue) {
 		t.Errorf("Expected secret value '%s', got '%s'",
-			string(expectedValue), string(retrievedValue))
+			expectedValue, retrievedValue.Bytes())
 	}
 }