From 4ed77902d13dbd5d830fd47d5cc10b49f65d1e7b Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sun, 4 Oct 2026 11:16:23 +0200 Subject: [PATCH] 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 --- TODO.md | 7 +++ internal/cli/cli.go | 5 -- internal/cli/create_vault_test.go | 7 ++- internal/cli/lock_test.go | 3 +- internal/cli/move_test.go | 10 +++- internal/cli/secrets.go | 26 +++-------- internal/cli/secrets_size_test.go | 22 +++++++-- internal/cli/version_test.go | 54 ++++++++++++++++++++-- internal/secret/atomic_test.go | 10 +++- internal/vault/integration_test.go | 6 ++- internal/vault/integration_version_test.go | 44 ++++++++++++++---- internal/vault/secrets.go | 26 ++++------- internal/vault/secrets_version_test.go | 24 +++++++--- internal/vault/vault_test.go | 6 ++- 14 files changed, 177 insertions(+), 73 deletions(-) diff --git a/TODO.md b/TODO.md index 2a8d576..3612f96 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,13 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 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 diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 071d287..90b4f5c 100644 --- a/internal/cli/cli.go +++ b/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...) -} diff --git a/internal/cli/create_vault_test.go b/internal/cli/create_vault_test.go index acc1d92..54e9930 100644 --- a/internal/cli/create_vault_test.go +++ b/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) } } diff --git a/internal/cli/lock_test.go b/internal/cli/lock_test.go index 4fb81a4..99d7c01 100644 --- a/internal/cli/lock_test.go +++ b/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") diff --git a/internal/cli/move_test.go b/internal/cli/move_test.go index 40c43c8..a5f216e 100644 --- a/internal/cli/move_test.go +++ b/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) diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index c8ae0ed..9cef727 100644 --- a/internal/cli/secrets.go +++ b/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 } diff --git a/internal/cli/secrets_size_test.go b/internal/cli/secrets_size_test.go index 72682d5..4dd3a0d 100644 --- a/internal/cli/secrets_size_test.go +++ b/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 diff --git a/internal/cli/version_test.go b/internal/cli/version_test.go index d2296d8..7cf041b 100644 --- a/internal/cli/version_test.go +++ b/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 diff --git a/internal/secret/atomic_test.go b/internal/secret/atomic_test.go index dddff04..739c09b 100644 --- a/internal/secret/atomic_test.go +++ b/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()) }) } } diff --git a/internal/vault/integration_test.go b/internal/vault/integration_test.go index e173254..a4bf842 100644 --- a/internal/vault/integration_test.go +++ b/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()) } } diff --git a/internal/vault/integration_version_test.go b/internal/vault/integration_version_test.go index b20a569..c1fc9cc 100644 --- a/internal/vault/integration_version_test.go +++ b/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 diff --git a/internal/vault/secrets.go b/internal/vault/secrets.go index 72895c1..3faeb47 100644 --- a/internal/vault/secrets.go +++ b/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 diff --git a/internal/vault/secrets_version_test.go b/internal/vault/secrets_version_test.go index 6a0d9d8..31617af 100644 --- a/internal/vault/secrets_version_test.go +++ b/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 diff --git a/internal/vault/vault_test.go b/internal/vault/vault_test.go index 1588bf6..f5ce2da 100644 --- a/internal/vault/vault_test.go +++ b/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()) } }