From 87abfe205847386305a180ad528670879a9d2ace Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 27 Jul 2026 23:45:55 +0200 Subject: [PATCH] Stop trimming binary HMAC secrets before use loadHMACSecrets ran bytes.TrimSpace on raw HMAC secret file content, silently stripping any leading/trailing byte that fell in the ASCII whitespace set. Secrets are documented as raw random bytes, so this corrupted ~4.6% of random 32-byte secrets and crashed embedded auth-server pods with a misleading "must be at least 32 bytes" error. Use file bytes verbatim and fail loudly with the file path and byte count when a secret is too short. Also removes an unused, identically buggy duplicate loader in server/crypto/keys.go. --- pkg/authserver/runner/embeddedauthserver.go | 14 +- .../runner/embeddedauthserver_test.go | 87 ++++++++++- pkg/authserver/server/crypto/keys.go | 60 -------- pkg/authserver/server/crypto/keys_test.go | 139 ------------------ 4 files changed, 95 insertions(+), 205 deletions(-) diff --git a/pkg/authserver/runner/embeddedauthserver.go b/pkg/authserver/runner/embeddedauthserver.go index 4431ad0796..680fbde42c 100644 --- a/pkg/authserver/runner/embeddedauthserver.go +++ b/pkg/authserver/runner/embeddedauthserver.go @@ -378,8 +378,12 @@ func loadHMACSecrets(files []string) (*servercrypto.HMACSecrets, error) { return nil, fmt.Errorf("failed to read HMAC secret from %s: %w", files[0], err) } - // Trim whitespace (Kubernetes Secret mounts may include trailing newlines) - current = bytes.TrimSpace(current) + // HMAC secrets are raw binary (see MCPExternalAuthConfig.HMACSecretRefs doc) — do not + // trim; any byte, including whitespace-valued ones, may be genuine key material. + if len(current) < servercrypto.MinSecretLength { + return nil, fmt.Errorf("HMAC secret in %s is %d bytes, but must be at least %d bytes", + files[0], len(current), servercrypto.MinSecretLength) + } secrets := &servercrypto.HMACSecrets{ Current: current, @@ -395,7 +399,11 @@ func loadHMACSecrets(files []string) (*servercrypto.HMACSecrets, error) { if err != nil { return nil, fmt.Errorf("failed to read rotated HMAC secret from %s: %w", file, err) } - secrets.Rotated = append(secrets.Rotated, bytes.TrimSpace(secret)) + if len(secret) < servercrypto.MinSecretLength { + return nil, fmt.Errorf("rotated HMAC secret in %s is %d bytes, but must be at least %d bytes", + file, len(secret), servercrypto.MinSecretLength) + } + secrets.Rotated = append(secrets.Rotated, secret) } return secrets, nil diff --git a/pkg/authserver/runner/embeddedauthserver_test.go b/pkg/authserver/runner/embeddedauthserver_test.go index f4aa34064e..9b453505ea 100644 --- a/pkg/authserver/runner/embeddedauthserver_test.go +++ b/pkg/authserver/runner/embeddedauthserver_test.go @@ -167,12 +167,12 @@ func TestLoadHMACSecrets(t *testing.T) { assert.Equal(t, []byte(rotatedSecret), secrets.Rotated[0]) }) - t.Run("trims whitespace from secrets", func(t *testing.T) { + t.Run("preserves leading and trailing whitespace bytes in secrets", func(t *testing.T) { t.Parallel() tmpDir := t.TempDir() secretFile := filepath.Join(tmpDir, "hmac-secret") - secretValue := " secret-with-whitespace \n" + secretValue := " secret-with-whitespace-that-is-at-least-32-bytes \n" err := os.WriteFile(secretFile, []byte(secretValue), 0600) require.NoError(t, err) @@ -181,7 +181,88 @@ func TestLoadHMACSecrets(t *testing.T) { require.NoError(t, err) require.NotNil(t, secrets) - assert.Equal(t, []byte("secret-with-whitespace"), secrets.Current) + assert.Equal(t, []byte(secretValue), secrets.Current) + }) + + t.Run("does not corrupt binary secrets with whitespace-valued edge bytes", func(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + secretFile := filepath.Join(tmpDir, "hmac-secret") + + original := make([]byte, 32) + for i := range original { + original[i] = 0xAB + } + original[0] = 0x20 + original[31] = 0x0A + + require.NoError(t, os.WriteFile(secretFile, original, 0600)) + + secrets, err := loadHMACSecrets([]string{secretFile}) + require.NoError(t, err) + require.NotNil(t, secrets) + + require.Len(t, secrets.Current, 32) + assert.Equal(t, original, secrets.Current) + }) + + t.Run("does not corrupt rotated binary secrets with whitespace-valued edge bytes", func(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + currentFile := filepath.Join(tmpDir, "hmac-current") + rotatedFile := filepath.Join(tmpDir, "hmac-rotated") + + require.NoError(t, os.WriteFile(currentFile, []byte("current-secret-32-bytes-minimum!"), 0600)) + + original := make([]byte, 32) + for i := range original { + original[i] = 0xAB + } + original[0] = 0x20 + original[31] = 0x0A + + require.NoError(t, os.WriteFile(rotatedFile, original, 0600)) + + secrets, err := loadHMACSecrets([]string{currentFile, rotatedFile}) + require.NoError(t, err) + require.NotNil(t, secrets) + + require.Len(t, secrets.Rotated, 1) + assert.Equal(t, original, secrets.Rotated[0]) + }) + + t.Run("short current secret returns error naming file and byte counts", func(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + secretFile := filepath.Join(tmpDir, "hmac-secret") + + require.NoError(t, os.WriteFile(secretFile, make([]byte, 31), 0600)) + + _, err := loadHMACSecrets([]string{secretFile}) + require.Error(t, err) + assert.Contains(t, err.Error(), secretFile) + assert.Contains(t, err.Error(), "31") + assert.Contains(t, err.Error(), "32") + }) + + t.Run("short rotated secret returns error naming file and byte counts", func(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + currentFile := filepath.Join(tmpDir, "hmac-current") + rotatedFile := filepath.Join(tmpDir, "hmac-rotated") + + require.NoError(t, os.WriteFile(currentFile, []byte("current-secret-32-bytes-minimum!"), 0600)) + require.NoError(t, os.WriteFile(rotatedFile, make([]byte, 31), 0600)) + + _, err := loadHMACSecrets([]string{currentFile, rotatedFile}) + require.Error(t, err) + assert.Contains(t, err.Error(), rotatedFile) + assert.Contains(t, err.Error(), "31") + assert.Contains(t, err.Error(), "32") }) t.Run("skips empty paths in rotated files", func(t *testing.T) { diff --git a/pkg/authserver/server/crypto/keys.go b/pkg/authserver/server/crypto/keys.go index 111a2678ef..0493b48d62 100644 --- a/pkg/authserver/server/crypto/keys.go +++ b/pkg/authserver/server/crypto/keys.go @@ -26,7 +26,6 @@ import ( "encoding/pem" "fmt" "os" - "strings" "github.com/go-jose/go-jose/v4" ) @@ -227,62 +226,3 @@ func DeriveSigningKeyParams(key crypto.Signer, keyID, algorithm string) (*Signin return params, nil } - -// loadHMACSecretFile loads an HMAC secret from a file (internal helper). -// Returns nil if path is empty (triggers random generation in toInternalConfig). -// The secret must be at least 32 bytes after trimming whitespace. -func loadHMACSecretFile(secretPath string) ([]byte, error) { - if secretPath == "" { - return nil, nil - } - - data, err := os.ReadFile(secretPath) // #nosec G304 - secretPath is provided by user via CLI flag or config - if err != nil { - return nil, fmt.Errorf("failed to read HMAC secret file: %w", err) - } - - // Trim whitespace (common in Kubernetes Secret mounts which often add trailing newlines) - secret := []byte(strings.TrimSpace(string(data))) - - if len(secret) < MinSecretLength { - return nil, fmt.Errorf("HMAC secret must be at least %d bytes", MinSecretLength) - } - - return secret, nil -} - -// LoadHMACSecrets loads HMAC secrets from file paths for rotation support. -// paths[0] is the current (signing) secret; paths[1:] are rotated (verification) secrets. -// Returns nil if paths is empty (caller should generate random secret). -func LoadHMACSecrets(paths []string) (*HMACSecrets, error) { - if len(paths) == 0 { - return nil, nil - } - - // Load current secret (required, cannot be empty path) - if paths[0] == "" { - return nil, fmt.Errorf("current HMAC secret path cannot be empty") - } - current, err := loadHMACSecretFile(paths[0]) - if err != nil { - return nil, fmt.Errorf("failed to load current HMAC secret: %w", err) - } - - // Load rotated secrets (optional, skip empty paths) - var rotated [][]byte - for i, path := range paths[1:] { - if path == "" { - continue - } - secret, err := loadHMACSecretFile(path) - if err != nil { - return nil, fmt.Errorf("failed to load rotated HMAC secret [%d]: %w", i+1, err) - } - rotated = append(rotated, secret) - } - - return &HMACSecrets{ - Current: current, - Rotated: rotated, - }, nil -} diff --git a/pkg/authserver/server/crypto/keys_test.go b/pkg/authserver/server/crypto/keys_test.go index 358fd82774..59949d6722 100644 --- a/pkg/authserver/server/crypto/keys_test.go +++ b/pkg/authserver/server/crypto/keys_test.go @@ -25,7 +25,6 @@ import ( "encoding/pem" "os" "path/filepath" - "strings" "testing" "github.com/stretchr/testify/assert" @@ -286,137 +285,6 @@ func TestDeriveKeyID(t *testing.T) { assert.NotEqual(t, id1, id3, "different keys should produce different IDs") } -func TestLoadHMACSecrets(t *testing.T) { - t.Parallel() - - validSecret := strings.Repeat("a", 32) - validSecret2 := strings.Repeat("b", 32) - tooShortSecret := strings.Repeat("a", 31) - - tests := []struct { - name string - setup func(t *testing.T, dir string) []string - wantCurrent []byte - wantRotated [][]byte - wantErr string - }{ - { - name: "empty paths", - setup: func(_ *testing.T, _ string) []string { return []string{} }, - wantCurrent: nil, - wantRotated: nil, - }, - { - name: "single secret", - setup: func(_ *testing.T, dir string) []string { - return []string{writeFileNamed(t, dir, "current", validSecret)} - }, - wantCurrent: []byte(validSecret), - wantRotated: nil, - }, - { - name: "with rotated secrets", - setup: func(_ *testing.T, dir string) []string { - return []string{ - writeFileNamed(t, dir, "current", validSecret), - writeFileNamed(t, dir, "rotated1", validSecret2), - } - }, - wantCurrent: []byte(validSecret), - wantRotated: [][]byte{[]byte(validSecret2)}, - }, - { - name: "empty current path", - setup: func(_ *testing.T, _ string) []string { - return []string{""} - }, - wantErr: "current HMAC secret path cannot be empty", - }, - { - name: "invalid current secret file", - setup: func(_ *testing.T, _ string) []string { - return []string{"/nonexistent/secret"} - }, - wantErr: "failed to load current", - }, - { - name: "invalid rotated secret", - setup: func(_ *testing.T, dir string) []string { - return []string{ - writeFileNamed(t, dir, "current", validSecret), - "/nonexistent/rotated", - } - }, - wantErr: "failed to load rotated HMAC secret [1]", - }, - { - name: "skip empty rotated paths", - setup: func(_ *testing.T, dir string) []string { - return []string{ - writeFileNamed(t, dir, "current", validSecret), - "", - writeFileNamed(t, dir, "rotated2", validSecret2), - } - }, - wantCurrent: []byte(validSecret), - wantRotated: [][]byte{[]byte(validSecret2)}, - }, - { - name: "whitespace trimmed", - setup: func(_ *testing.T, dir string) []string { - return []string{ - writeFileNamed(t, dir, "current", " "+validSecret+" \n\n"), - writeFileNamed(t, dir, "rotated", "\t"+validSecret2+"\n"), - } - }, - wantCurrent: []byte(validSecret), - wantRotated: [][]byte{[]byte(validSecret2)}, - }, - { - name: "current too short", - setup: func(_ *testing.T, dir string) []string { - return []string{writeFileNamed(t, dir, "current", tooShortSecret)} - }, - wantErr: "HMAC secret must be at least", - }, - { - name: "rotated too short", - setup: func(_ *testing.T, dir string) []string { - return []string{ - writeFileNamed(t, dir, "current", validSecret), - writeFileNamed(t, dir, "rotated", tooShortSecret), - } - }, - wantErr: "failed to load rotated HMAC secret [1]", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - dir := t.TempDir() - paths := tt.setup(t, dir) - - secrets, err := LoadHMACSecrets(paths) - - if tt.wantErr != "" { - require.Error(t, err) - assert.Contains(t, err.Error(), tt.wantErr) - assert.Nil(t, secrets) - } else { - require.NoError(t, err) - if tt.wantCurrent == nil { - assert.Nil(t, secrets) - } else { - require.NotNil(t, secrets) - assert.Equal(t, tt.wantCurrent, secrets.Current) - assert.Equal(t, tt.wantRotated, secrets.Rotated) - } - } - }) - } -} - // Helpers func writePEM(t *testing.T, dir, pemType string, der []byte) string { @@ -426,10 +294,3 @@ func writePEM(t *testing.T, dir, pemType string, der []byte) string { require.NoError(t, os.WriteFile(path, data, 0600)) return path } - -func writeFileNamed(t *testing.T, dir, name, content string) string { - t.Helper() - path := filepath.Join(dir, name) - require.NoError(t, os.WriteFile(path, []byte(content), 0600)) - return path -}