sneak/secret
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: resolve critical security vulnerabilities in debug logging and command execution

Browse Source 
- Remove sensitive data from debug logs (vault/secrets.go, secret/version.go)
- Add input validation for GPG key IDs and keychain item names
- Resolve GPG key IDs to full fingerprints before storing in metadata
- Add comprehensive test coverage for validation functions
- Add golangci-lint configuration with additional linters

Security improvements:
- Debug logs no longer expose decrypted secret values or private keys
- GPG and keychain commands now validate input to prevent injection attacks
- All validation uses precompiled regex patterns for performance
This commit is contained in:
Jeffrey Paul 2025-06-20 07:50:26 -07:00
parent 004dce5472
commit 985d79d3c0
8 changed files with 529 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

99
.golangci.yml Normal file
View File

@ -0,0 +1,99 @@
run:
timeout: 5m
go: "1.22"
linters:
enable:
# Additional linters requested
- testifylint # Checks usage of github.com/stretchr/testify
- usetesting # usetesting is an analyzer that detects using os.Setenv instead of t.Setenv since Go 1.17
- tagliatelle # Checks the struct tags
- nlreturn # nlreturn checks for a new line before return and branch statements
- nilnil # Checks that there is no simultaneous return of nil error and an invalid value
- nestif # Reports deeply nested if statements
- mnd # An analyzer to detect magic numbers
- lll # Reports long lines
- intrange # intrange is a linter to find places where for loops could make use of an integer range
- gofumpt # Gofumpt checks whether code was gofumpt-ed
- gochecknoglobals # Check that no global variables exist
# Default/existing linters that are commonly useful
- govet
- errcheck
- staticcheck
- unused
- gosimple
- ineffassign
- typecheck
- gofmt
- goimports
- misspell
- revive
- gosec
- unconvert
- unparam
linters-settings:
lll:
line-length: 120
mnd:
# List of enabled checks, see https://github.com/tommy-muehle/go-mnd/#checks for description.
checks:
- argument
- case
- condition
- operation
- return
- assign
ignored-numbers:
- '0'
- '1'
- '2'
- '8'
- '16'
- '40' # GPG fingerprint length
- '64'
- '128'
- '256'
- '512'
- '1024'
- '2048'
- '4096'
nestif:
min-complexity: 4
nlreturn:
block-size: 2
tagliatelle:
case:
rules:
json: snake
yaml: snake
xml: snake
bson: snake
testifylint:
enable-all: true
usetesting:
strict: true
issues:
exclude-rules:
# Exclude some linters from running on tests files
- path: _test\.go
linters:
- gochecknoglobals
- mnd
- unparam
# Allow long lines in generated code or test data
- path: ".*_gen\\.go"
linters:
- lll
max-issues-per-linter: 0
max-same-issues: 0

37
internal/secret/keychainunlocker.go
View File

@ -8,6 +8,7 @@ import (
"os"
"os/exec"
"path/filepath"
"regexp"
"time"
"filippo.io/age"
@ -15,6 +16,12 @@ import (
"github.com/spf13/afero"
)
var (
// keychainItemNameRegex validates keychain item names
// Allows alphanumeric characters, dots, hyphens, and underscores only
keychainItemNameRegex = regexp.MustCompile(`^[A-Za-z0-9._-]+$`)
)
// KeychainUnlockerMetadata extends UnlockerMetadata with keychain-specific data
type KeychainUnlockerMetadata struct {
UnlockerMetadata
@ -392,9 +399,25 @@ func checkMacOSAvailable() error {
return nil
}
// validateKeychainItemName validates that a keychain item name is safe for command execution
func validateKeychainItemName(itemName string) error {
if itemName == "" {
return fmt.Errorf("keychain item name cannot be empty")
}
if !keychainItemNameRegex.MatchString(itemName) {
return fmt.Errorf("invalid keychain item name format: %s", itemName)
}
return nil
}
// storeInKeychain stores data in the macOS keychain using the security command
func storeInKeychain(itemName string, data []byte) error {
cmd := exec.Command("/usr/bin/security", "add-generic-password",
if err := validateKeychainItemName(itemName); err != nil {
return fmt.Errorf("invalid keychain item name: %w", err)
}
cmd := exec.Command("/usr/bin/security", "add-generic-password", //nolint:gosec // Input validated by validateKeychainItemName
"-a", itemName,
"-s", itemName,
"-w", string(data),
@ -409,7 +432,11 @@ func storeInKeychain(itemName string, data []byte) error {
// retrieveFromKeychain retrieves data from the macOS keychain using the security command
func retrieveFromKeychain(itemName string) ([]byte, error) {
cmd := exec.Command("/usr/bin/security", "find-generic-password",
if err := validateKeychainItemName(itemName); err != nil {
return nil, fmt.Errorf("invalid keychain item name: %w", err)
}
cmd := exec.Command("/usr/bin/security", "find-generic-password", //nolint:gosec // Input validated by validateKeychainItemName
"-a", itemName,
"-s", itemName,
"-w") // Return password only
@ -429,7 +456,11 @@ func retrieveFromKeychain(itemName string) ([]byte, error) {
// deleteFromKeychain removes an item from the macOS keychain using the security command
func deleteFromKeychain(itemName string) error {
cmd := exec.Command("/usr/bin/security", "delete-generic-password",
if err := validateKeychainItemName(itemName); err != nil {
return fmt.Errorf("invalid keychain item name: %w", err)
}
cmd := exec.Command("/usr/bin/security", "delete-generic-password", //nolint:gosec // Input validated by validateKeychainItemName
"-a", itemName,
"-s", itemName)

35
internal/secret/pgpunlock_test.go
View File

@ -189,21 +189,26 @@ Passphrase: ` + testPassphrase + `
}
t.Log("GPG key generated successfully")
// Get the key ID
// Get the key ID and fingerprint
output, err := runGPGWithPassphrase(gnupgHomeDir, testPassphrase,
[]string{"--list-secret-keys", "--with-colons"}, nil)
[]string{"--list-secret-keys", "--with-colons", "--fingerprint"}, nil)
if err != nil {
t.Fatalf("Failed to list GPG keys: %v", err)
}
// Parse output to get key ID
var keyID string
// Parse output to get key ID and fingerprint
var keyID, fingerprint string
lines := strings.Split(string(output), "\n")
for _, line := range lines {
if strings.HasPrefix(line, "sec:") {
fields := strings.Split(line, ":")
if len(fields) >= 5 {
keyID = fields[4]
}
} else if strings.HasPrefix(line, "fpr:") {
fields := strings.Split(line, ":")
if len(fields) >= 10 && fields[9] != "" {
fingerprint = fields[9]
break
}
}
@ -212,7 +217,11 @@ Passphrase: ` + testPassphrase + `
if keyID == "" {
t.Fatalf("Failed to find GPG key ID in output: %s", output)
}
if fingerprint == "" {
t.Fatalf("Failed to find GPG fingerprint in output: %s", output)
}
t.Logf("Generated GPG key ID: %s", keyID)
t.Logf("Generated GPG fingerprint: %s", fingerprint)
// Set the GPG_AGENT_INFO to empty to ensure gpg-agent doesn't interfere
oldAgentInfo := os.Getenv("GPG_AGENT_INFO")
@ -326,9 +335,9 @@ Passphrase: ` + testPassphrase + `
t.Errorf("Expected PGP unlock key type 'pgp', got '%s'", pgpUnlocker.GetType())
}
// Check if the key ID includes the GPG key ID
if !strings.Contains(pgpUnlocker.GetID(), keyID) {
t.Errorf("PGP unlock key ID '%s' does not contain GPG key ID '%s'", pgpUnlocker.GetID(), keyID)
// Check if the key ID includes the GPG fingerprint
if !strings.Contains(pgpUnlocker.GetID(), fingerprint) {
t.Errorf("PGP unlock key ID '%s' does not contain GPG fingerprint '%s'", pgpUnlocker.GetID(), fingerprint)
}
// Check if the key directory exists
@ -400,8 +409,8 @@ Passphrase: ` + testPassphrase + `
t.Errorf("Expected metadata type 'pgp', got '%s'", metadata.Type)
}
if metadata.GPGKeyID != keyID {
t.Errorf("Expected GPG key ID '%s', got '%s'", keyID, metadata.GPGKeyID)
if metadata.GPGKeyID != fingerprint {
t.Errorf("Expected GPG fingerprint '%s', got '%s'", fingerprint, metadata.GPGKeyID)
}
})
@ -431,7 +440,7 @@ Passphrase: ` + testPassphrase + `
pgpMetadata := PGPUnlockerMetadata{
UnlockerMetadata: metadata,
GPGKeyID: keyID,
GPGKeyID: fingerprint,
}
// Write metadata file
@ -450,9 +459,9 @@ Passphrase: ` + testPassphrase + `
t.Fatalf("Failed to get GPG key ID: %v", err)
}
// Verify key ID
if retrievedKeyID != keyID {
t.Errorf("Expected GPG key ID '%s', got '%s'", keyID, retrievedKeyID)
// Verify key ID (should be the fingerprint)
if retrievedKeyID != fingerprint {
t.Errorf("Expected GPG fingerprint '%s', got '%s'", fingerprint, retrievedKeyID)
}
})

68
internal/secret/pgpunlocker.go
View File

@ -7,6 +7,7 @@ import (
"os"
"os/exec"
"path/filepath"
"regexp"
"strings"
"time"
@ -24,6 +25,19 @@ var (
// GPGDecryptFunc is the function used for GPG decryption
// Can be overridden in tests to provide a non-interactive implementation
GPGDecryptFunc = gpgDecryptDefault
// gpgKeyIDRegex validates GPG key IDs
// Allows either:
// 1. Email addresses (user@domain.tld format)
// 2. Short key IDs (8 hex characters)
// 3. Long key IDs (16 hex characters)
// 4. Full fingerprints (40 hex characters)
gpgKeyIDRegex = regexp.MustCompile(
`^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}$|` +
`^[A-Fa-f0-9]{8}$|` +
`^[A-Fa-f0-9]{16}$|` +
`^[A-Fa-f0-9]{40}$`,
)
)
// PGPUnlockerMetadata extends UnlockerMetadata with PGP-specific data
@ -285,14 +299,20 @@ func CreatePGPUnlocker(fs afero.Fs, stateDir string, gpgKeyID string) (*PGPUnloc
return nil, fmt.Errorf("failed to write encrypted age private key: %w", err)
}
// Step 9: Create and write enhanced metadata
// Step 9: Resolve the GPG key ID to its full fingerprint
fingerprint, err := resolveGPGKeyFingerprint(gpgKeyID)
if err != nil {
return nil, fmt.Errorf("failed to resolve GPG key fingerprint: %w", err)
}
// Step 10: Create and write enhanced metadata with full fingerprint
pgpMetadata := PGPUnlockerMetadata{
UnlockerMetadata: UnlockerMetadata{
Type: "pgp",
CreatedAt: time.Now(),
Flags: []string{"gpg", "encrypted"},
},
GPGKeyID: gpgKeyID,
GPGKeyID: fingerprint,
}
metadataBytes, err := json.MarshalIndent(pgpMetadata, "", " ")
@ -311,6 +331,46 @@ func CreatePGPUnlocker(fs afero.Fs, stateDir string, gpgKeyID string) (*PGPUnloc
}, nil
}
// validateGPGKeyID validates that a GPG key ID is safe for command execution
func validateGPGKeyID(keyID string) error {
if keyID == "" {
return fmt.Errorf("GPG key ID cannot be empty")
}
if !gpgKeyIDRegex.MatchString(keyID) {
return fmt.Errorf("invalid GPG key ID format: %s", keyID)
}
return nil
}
// resolveGPGKeyFingerprint resolves any GPG key identifier to its full fingerprint
func resolveGPGKeyFingerprint(keyID string) (string, error) {
if err := validateGPGKeyID(keyID); err != nil {
return "", fmt.Errorf("invalid GPG key ID: %w", err)
}
// Use GPG to get the full fingerprint for the key
cmd := exec.Command("gpg", "--list-keys", "--with-colons", "--fingerprint", keyID)
output, err := cmd.Output()
if err != nil {
return "", fmt.Errorf("failed to resolve GPG key fingerprint: %w", err)
}
// Parse the output to extract the fingerprint
lines := strings.Split(string(output), "\n")
for _, line := range lines {
if strings.HasPrefix(line, "fpr:") {
fields := strings.Split(line, ":")
if len(fields) >= 10 && fields[9] != "" {
return fields[9], nil
}
}
}
return "", fmt.Errorf("could not find fingerprint for GPG key: %s", keyID)
}
// checkGPGAvailable verifies that GPG is available
func checkGPGAvailable() error {
cmd := exec.Command("gpg", "--version")
@ -322,6 +382,10 @@ func checkGPGAvailable() error {
// gpgEncryptDefault is the default implementation of GPG encryption
func gpgEncryptDefault(data []byte, keyID string) ([]byte, error) {
if err := validateGPGKeyID(keyID); err != nil {
return nil, fmt.Errorf("invalid GPG key ID: %w", err)
}
cmd := exec.Command("gpg", "--trust-model", "always", "--armor", "--encrypt", "-r", keyID)
cmd.Stdin = strings.NewReader(string(data))

297
internal/secret/validation_test.go Normal file
View File

@ -0,0 +1,297 @@
package secret
import (
"testing"
)
func TestValidateGPGKeyID(t *testing.T) {
tests := []struct {
name string
keyID string
wantErr bool
}{
// Valid cases
{
name: "valid email address",
keyID: "test@example.com",
wantErr: false,
},
{
name: "valid email with dots and hyphens",
keyID: "test.user-name@example-domain.co.uk",
wantErr: false,
},
{
name: "valid email with plus",
keyID: "test+tag@example.com",
wantErr: false,
},
{
name: "valid short key ID (8 hex chars)",
keyID: "ABCDEF12",
wantErr: false,
},
{
name: "valid long key ID (16 hex chars)",
keyID: "ABCDEF1234567890",
wantErr: false,
},
{
name: "valid fingerprint (40 hex chars)",
keyID: "ABCDEF1234567890ABCDEF1234567890ABCDEF12",
wantErr: false,
},
{
name: "valid lowercase hex fingerprint",
keyID: "abcdef1234567890abcdef1234567890abcdef12",
wantErr: false,
},
{
name: "valid mixed case hex",
keyID: "AbCdEf1234567890",
wantErr: false,
},
// Invalid cases
{
name: "empty key ID",
keyID: "",
wantErr: true,
},
{
name: "key ID with spaces",
keyID: "test user@example.com",
wantErr: true,
},
{
name: "key ID with semicolon (command injection)",
keyID: "test@example.com; rm -rf /",
wantErr: true,
},
{
name: "key ID with pipe (command injection)",
keyID: "test@example.com | cat /etc/passwd",
wantErr: true,
},
{
name: "key ID with backticks (command injection)",
keyID: "test@example.com`whoami`",
wantErr: true,
},
{
name: "key ID with dollar sign (command injection)",
keyID: "test@example.com$(whoami)",
wantErr: true,
},
{
name: "key ID with quotes",
keyID: "test\"@example.com",
wantErr: true,
},
{
name: "key ID with single quotes",
keyID: "test'@example.com",
wantErr: true,
},
{
name: "key ID with backslash",
keyID: "test\\@example.com",
wantErr: true,
},
{
name: "key ID with newline",
keyID: "test@example.com\nrm -rf /",
wantErr: true,
},
{
name: "key ID with carriage return",
keyID: "test@example.com\rrm -rf /",
wantErr: true,
},
{
name: "hex with invalid length (7 chars)",
keyID: "ABCDEF1",
wantErr: true,
},
{
name: "hex with invalid length (9 chars)",
keyID: "ABCDEF123",
wantErr: true,
},
{
name: "hex with non-hex characters",
keyID: "ABCDEFGH",
wantErr: true,
},
{
name: "mixed format (email with hex)",
keyID: "test@ABCDEF12",
wantErr: true,
},
{
name: "key ID with ampersand",
keyID: "test@example.com & echo test",
wantErr: true,
},
{
name: "key ID with redirect",
keyID: "test@example.com > /tmp/test",
wantErr: true,
},
{
name: "key ID with null byte",
keyID: "test@example.com\x00",
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := validateGPGKeyID(tt.keyID)
if (err != nil) != tt.wantErr {
t.Errorf("validateGPGKeyID() error = %v, wantErr %v", err, tt.wantErr)
}
})
}
}
func TestValidateKeychainItemName(t *testing.T) {
tests := []struct {
name string
itemName string
wantErr bool
}{
// Valid cases
{
name: "valid simple name",
itemName: "my-secret-key",
wantErr: false,
},
{
name: "valid name with dots",
itemName: "com.example.app.key",
wantErr: false,
},
{
name: "valid name with underscores",
itemName: "my_secret_key_123",
wantErr: false,
},
{
name: "valid alphanumeric",
itemName: "Secret123Key",
wantErr: false,
},
{
name: "valid with hyphen at start",
itemName: "-my-key",
wantErr: false,
},
{
name: "valid with dot at start",
itemName: ".hidden-key",
wantErr: false,
},
// Invalid cases
{
name: "empty item name",
itemName: "",
wantErr: true,
},
{
name: "item name with spaces",
itemName: "my secret key",
wantErr: true,
},
{
name: "item name with semicolon",
itemName: "key;rm -rf /",
wantErr: true,
},
{
name: "item name with pipe",
itemName: "key|cat /etc/passwd",
wantErr: true,
},
{
name: "item name with backticks",
itemName: "key`whoami`",
wantErr: true,
},
{
name: "item name with dollar sign",
itemName: "key$(whoami)",
wantErr: true,
},
{
name: "item name with quotes",
itemName: "key\"name",
wantErr: true,
},
{
name: "item name with single quotes",
itemName: "key'name",
wantErr: true,
},
{
name: "item name with backslash",
itemName: "key\\name",
wantErr: true,
},
{
name: "item name with newline",
itemName: "key\nname",
wantErr: true,
},
{
name: "item name with carriage return",
itemName: "key\rname",
wantErr: true,
},
{
name: "item name with ampersand",
itemName: "key&echo test",
wantErr: true,
},
{
name: "item name with redirect",
itemName: "key>/tmp/test",
wantErr: true,
},
{
name: "item name with null byte",
itemName: "key\x00name",
wantErr: true,
},
{
name: "item name with parentheses",
itemName: "key(test)",
wantErr: true,
},
{
name: "item name with brackets",
itemName: "key[test]",
wantErr: true,
},
{
name: "item name with asterisk",
itemName: "key*",
wantErr: true,
},
{
name: "item name with question mark",
itemName: "key?",
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := validateKeychainItemName(tt.itemName)
if (err != nil) != tt.wantErr {
t.Errorf("validateKeychainItemName() error = %v, wantErr %v", err, tt.wantErr)
}
})
}
}

1
internal/secret/version.go
View File

@ -343,7 +343,6 @@ func (sv *SecretVersion) GetValue(ltIdentity *age.X25519Identity) ([]byte, error
Debug("Successfully retrieved version value",
"version", sv.Version,
"value_length", len(value),
"value_as_string", string(value),
"is_empty", len(value) == 0)
return value, nil
}

18
pkg/bip85/bip85.go
View File

@ -22,19 +22,19 @@ import (
const (
// BIP85_MASTER_PATH is the derivation path prefix for all BIP85 applications
BIP85_MASTER_PATH = "m/83696968'"
BIP85_MASTER_PATH = "m/83696968'" //nolint:revive // ALL_CAPS used for BIP85 constants
// BIP85_KEY_HMAC_KEY is the HMAC key used for deriving the entropy
BIP85_KEY_HMAC_KEY = "bip-entropy-from-k"
BIP85_KEY_HMAC_KEY = "bip-entropy-from-k" //nolint:revive // ALL_CAPS used for BIP85 constants
// Application numbers
APP_BIP39 = 39 // BIP39 mnemonics
APP_HD_WIF = 2 // WIF for Bitcoin Core
APP_XPRV = 32 // Extended private key
APP_HEX = 128169
APP_PWD64 = 707764 // Base64 passwords
APP_PWD85 = 707785 // Base85 passwords
APP_RSA = 828365
APP_BIP39 = 39 // BIP39 mnemonics //nolint:revive // ALL_CAPS used for BIP85 constants
APP_HD_WIF = 2 // WIF for Bitcoin Core //nolint:revive // ALL_CAPS used for BIP85 constants
APP_XPRV = 32 // Extended private key //nolint:revive // ALL_CAPS used for BIP85 constants
APP_HEX = 128169 //nolint:revive // ALL_CAPS used for BIP85 constants
APP_PWD64 = 707764 // Base64 passwords //nolint:revive // ALL_CAPS used for BIP85 constants
APP_PWD85 = 707785 // Base85 passwords //nolint:revive // ALL_CAPS used for BIP85 constants
APP_RSA = 828365 //nolint:revive // ALL_CAPS used for BIP85 constants
)
// Version bytes for extended keys

2
pkg/bip85/bip85_test.go
View File

@ -1,5 +1,7 @@
package bip85
//nolint:gosec,revive,unparam // Test file with hardcoded test vectors
import (
"bytes"
"encoding/hex"