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

@@ -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

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

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()),

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

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) {

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