Keep secret get values in locked memory (closes #37)

Vault.GetSecret and Vault.GetSecretVersion return the decrypted value
as a *memguard.LockedBuffer instead of copying it into an ordinary
[]byte that nothing wiped. Every caller destroys the buffer, and
`secret get` writes its bytes straight to stdout, still with no
trailing newline. Instance.Print, which formatted through fmt and had
no other callers, is removed, and so is a debug log line in
`get --version` that held the plaintext value.

Model: opus-5-5

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/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/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())
 	}
 }