From 8ec3fc877dba7644b9a1b2255c9eb8e2b4834706 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 15 Jul 2025 08:59:23 +0200 Subject: [PATCH] Fix GetValue methods to return LockedBuffer internally - Changed Secret.GetValue and Version.GetValue to return *memguard.LockedBuffer - Updated all internal callers to handle LockedBuffer properly - For backward compatibility, vault.GetSecret still returns []byte but makes a copy - This ensures secret values are protected in memory during decryption - Updated tests to handle LockedBuffer returns - Fixed CLI getSecretValue to use LockedBuffer throughout --- TODO.md | 4 ++-- internal/cli/crypto.go | 36 +++++++++------------------------ internal/secret/secret.go | 2 +- internal/secret/version.go | 11 ++++++---- internal/secret/version_test.go | 5 +++-- internal/vault/secrets.go | 13 ++++++++---- 6 files changed, 32 insertions(+), 39 deletions(-) diff --git a/TODO.md b/TODO.md index 928ac12..44d9b46 100644 --- a/TODO.md +++ b/TODO.md @@ -10,10 +10,10 @@ prioritized from most critical (top) to least critical (bottom). - [x] **1. Secret.Save accepts unprotected data**: `internal/secret/secret.go:67` - `Save(value []byte, force bool)` - ✓ REMOVED - deprecated function deleted - [x] **2. EncryptWithPassphrase accepts unprotected data**: `internal/secret/crypto.go:73` - `EncryptWithPassphrase(data []byte, passphrase *memguard.LockedBuffer)` - ✓ FIXED - now accepts LockedBuffer for data - [x] **3. storeInKeychain accepts unprotected data**: `internal/secret/keychainunlocker.go:469` - `storeInKeychain(itemName string, data []byte)` - ✓ FIXED - now accepts LockedBuffer for data -- [ ] **4. gpgEncryptDefault accepts unprotected data**: `internal/secret/pgpunlocker.go:351` - `gpgEncryptDefault(data []byte, keyID string)` - encrypts unprotected data +- [x] **4. gpgEncryptDefault accepts unprotected data**: `internal/secret/pgpunlocker.go:351` - `gpgEncryptDefault(data []byte, keyID string)` - ✓ FIXED - now accepts LockedBuffer for data ### Functions returning unprotected secrets -- [ ] **5. GetValue returns unprotected secret**: `internal/secret/secret.go:93` - `GetValue(unlocker Unlocker) ([]byte, error)` - returns decrypted secret as bare []byte +- [x] **5. GetValue returns unprotected secret**: `internal/secret/secret.go:93` - `GetValue(unlocker Unlocker) ([]byte, error)` - ✓ FIXED - now returns LockedBuffer internally - [ ] **6. DecryptWithIdentity returns unprotected data**: `internal/secret/crypto.go:57` - `DecryptWithIdentity(data []byte, identity age.Identity) ([]byte, error)` - returns decrypted data unprotected - [ ] **7. DecryptWithPassphrase returns unprotected data**: `internal/secret/crypto.go:94` - `DecryptWithPassphrase(encryptedData []byte, passphrase *memguard.LockedBuffer) ([]byte, error)` - returns decrypted data unprotected - [ ] **8. gpgDecryptDefault returns unprotected data**: `internal/secret/pgpunlocker.go:368` - `gpgDecryptDefault(encryptedData []byte) ([]byte, error)` - returns decrypted data unprotected diff --git a/internal/cli/crypto.go b/internal/cli/crypto.go index db337a9..f58c8c5 100644 --- a/internal/cli/crypto.go +++ b/internal/cli/crypto.go @@ -96,21 +96,13 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error { } } else { // Secret exists, get the age secret key from it - secretValue, err := cli.getSecretValue(vlt, secretObj) + secretBuffer, err := cli.getSecretValue(vlt, secretObj) if err != nil { return fmt.Errorf("failed to get secret value: %w", err) } + defer secretBuffer.Destroy() - // Create secure buffer for the secret value - secureBuffer := memguard.NewBufferFromBytes(secretValue) - defer secureBuffer.Destroy() - - // Clear the original secret value - for i := range secretValue { - secretValue[i] = 0 - } - - ageSecretKey = secureBuffer.String() + ageSecretKey = secretBuffer.String() // Validate that it's a valid age secret key if !isValidAgeSecretKey(ageSecretKey) { @@ -189,36 +181,28 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error { } // Get the age secret key from the secret - var secretValue []byte + var secretBuffer *memguard.LockedBuffer if os.Getenv(secret.EnvMnemonic) != "" { - secretValue, err = secretObj.GetValue(nil) + secretBuffer, err = secretObj.GetValue(nil) } else { unlocker, unlockErr := vlt.GetCurrentUnlocker() if unlockErr != nil { return fmt.Errorf("failed to get current unlocker: %w", unlockErr) } - secretValue, err = secretObj.GetValue(unlocker) + secretBuffer, err = secretObj.GetValue(unlocker) } if err != nil { return fmt.Errorf("failed to get secret value: %w", err) } - - // Create secure buffer for the secret value - secureBuffer := memguard.NewBufferFromBytes(secretValue) - defer secureBuffer.Destroy() - - // Clear the original secret value - for i := range secretValue { - secretValue[i] = 0 - } + defer secretBuffer.Destroy() // Validate that it's a valid age secret key - if !isValidAgeSecretKey(secureBuffer.String()) { + if !isValidAgeSecretKey(secretBuffer.String()) { return fmt.Errorf("secret '%s' does not contain a valid age secret key", secretName) } // Parse the age secret key to get the identity - identity, err := age.ParseX25519Identity(secureBuffer.String()) + identity, err := age.ParseX25519Identity(secretBuffer.String()) if err != nil { return fmt.Errorf("failed to parse age secret key: %w", err) } @@ -266,7 +250,7 @@ func isValidAgeSecretKey(key string) bool { } // getSecretValue retrieves the value of a secret using the appropriate unlocker -func (cli *Instance) getSecretValue(vlt *vault.Vault, secretObj *secret.Secret) ([]byte, error) { +func (cli *Instance) getSecretValue(vlt *vault.Vault, secretObj *secret.Secret) (*memguard.LockedBuffer, error) { if os.Getenv(secret.EnvMnemonic) != "" { return secretObj.GetValue(nil) } diff --git a/internal/secret/secret.go b/internal/secret/secret.go index c706af4..287ffc8 100644 --- a/internal/secret/secret.go +++ b/internal/secret/secret.go @@ -63,7 +63,7 @@ func NewSecret(vault VaultInterface, name string) *Secret { } // GetValue retrieves and decrypts the current version's value using the provided unlocker -func (s *Secret) GetValue(unlocker Unlocker) ([]byte, error) { +func (s *Secret) GetValue(unlocker Unlocker) (*memguard.LockedBuffer, error) { DebugWith("Getting secret value", slog.String("secret_name", s.Name), slog.String("vault_name", s.vault.GetName()), diff --git a/internal/secret/version.go b/internal/secret/version.go index 9036b2d..1b2ffed 100644 --- a/internal/secret/version.go +++ b/internal/secret/version.go @@ -324,7 +324,7 @@ func (sv *Version) LoadMetadata(ltIdentity *age.X25519Identity) error { } // GetValue retrieves and decrypts the version value -func (sv *Version) GetValue(ltIdentity *age.X25519Identity) ([]byte, error) { +func (sv *Version) GetValue(ltIdentity *age.X25519Identity) (*memguard.LockedBuffer, error) { DebugWith("Getting version value", slog.String("secret_name", sv.SecretName), slog.String("version", sv.Version), @@ -388,12 +388,15 @@ func (sv *Version) GetValue(ltIdentity *age.X25519Identity) ([]byte, error) { return nil, fmt.Errorf("failed to decrypt version value: %w", err) } + // Create a secure buffer for the decrypted value + valueBuffer := memguard.NewBufferFromBytes(value) + Debug("Successfully retrieved version value", "version", sv.Version, - "value_length", len(value), - "is_empty", len(value) == 0) + "value_length", valueBuffer.Size(), + "is_empty", valueBuffer.Size() == 0) - return value, nil + return valueBuffer, nil } // ListVersions lists all versions of a secret diff --git a/internal/secret/version_test.go b/internal/secret/version_test.go index 3ca95d3..6a250f1 100644 --- a/internal/secret/version_test.go +++ b/internal/secret/version_test.go @@ -255,10 +255,11 @@ func TestSecretVersionGetValue(t *testing.T) { require.NoError(t, err) // Retrieve the value - retrievedValue, err := sv.GetValue(ltIdentity) + retrievedBuffer, err := sv.GetValue(ltIdentity) require.NoError(t, err) + defer retrievedBuffer.Destroy() - assert.Equal(t, expectedValue, retrievedValue) + assert.Equal(t, expectedValue, retrievedBuffer.Bytes()) } func TestListVersions(t *testing.T) { diff --git a/internal/vault/secrets.go b/internal/vault/secrets.go index 8bc2805..5d3296c 100644 --- a/internal/vault/secrets.go +++ b/internal/vault/secrets.go @@ -393,21 +393,26 @@ 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(decryptedValue)), + slog.Int("decrypted_length", len(result)), ) // 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(decryptedValue), - "is_empty", len(decryptedValue) == 0) + "decrypted_value_length", len(result), + "is_empty", len(result) == 0) - return decryptedValue, nil + return result, nil } // UnlockVault unlocks the vault and returns the long-term private key