Merge pull request #496 from varshaprasad96/fix/security-audit-remediations

Fix: remediate security audit findings in authbridge

Author: huang195

authbridge/authlib/bypass/matcher.go

@@ -22,12 +22,18 @@ type Matcher struct {
 // NewMatcher creates a Matcher from the given patterns.
 // Returns an error if any pattern has invalid path.Match syntax.
 func NewMatcher(patterns []string) (*Matcher, error) {
-	for _, p := range patterns {
-		if _, err := path.Match(p, "/"); err != nil {
+	clean := make([]string, len(patterns))
+	for i, p := range patterns {
+		trimmed := strings.TrimSpace(p)
+		if _, err := path.Match(trimmed, "/"); err != nil {
 			return nil, fmt.Errorf("invalid bypass pattern %q: %w", p, err)
 		}
+		if trimmed == "" || trimmed == "*" || trimmed == "/*" {
+			return nil, fmt.Errorf("bypass pattern %q is too broad; use specific path globs", p)
+		}
+		clean[i] = trimmed
 	}
-	return &Matcher{patterns: patterns}, nil
+	return &Matcher{patterns: clean}, nil
 }
 
 // Match checks if the given request path matches any bypass pattern.

authbridge/authlib/bypass/matcher_test.go

@@ -83,3 +83,40 @@ func TestMatch_CustomPatterns(t *testing.T) {
 		t.Error("expected /private/data to not match")
 	}
 }
+
+func TestNewMatcher_RejectsFootgunPatterns(t *testing.T) {
+	tests := []struct {
+		name    string
+		pattern string
+	}{
+		{"star", "*"},
+		{"slash-star", "/*"},
+		{"empty", ""},
+		{"whitespace-only", "  "},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_, err := NewMatcher([]string{tt.pattern})
+			if err == nil {
+				t.Errorf("NewMatcher(%q) should return error for match-all pattern", tt.pattern)
+			}
+		})
+	}
+}
+
+func TestNewMatcher_AllowsSpecificStar(t *testing.T) {
+	_, err := NewMatcher([]string{"/.well-known/*"})
+	if err != nil {
+		t.Fatalf("specific star pattern should be allowed: %v", err)
+	}
+}
+
+func TestNewMatcher_TrimsWhitespace(t *testing.T) {
+	m, err := NewMatcher([]string{" /healthz "})
+	if err != nil {
+		t.Fatalf("whitespace-padded pattern should be accepted: %v", err)
+	}
+	if !m.Match("/healthz") {
+		t.Error("trimmed pattern should match /healthz")
+	}
+}

authbridge/authlib/listener/extproc/server.go

@@ -161,6 +161,7 @@ func (s *Server) handleInbound(stream extprocv3.ExternalProcessor_ProcessServer,
 	action := s.InboundPipeline.Run(ctx, pctx)
 	if action.Type == pipeline.Reject {
 		s.recordInboundReject(pctx, action)
+		s.InboundPipeline.RunFinish(ctx, pctx, pipeline.OutcomeFromContext(pctx))
 		return rejectFromAction(action), nil
 	}
 
@@ -188,6 +189,7 @@ func (s *Server) handleInboundBody(stream extprocv3.ExternalProcessor_ProcessSer
 	action := s.InboundPipeline.Run(ctx, pctx)
 	if action.Type == pipeline.Reject {
 		s.recordInboundReject(pctx, action)
+		s.InboundPipeline.RunFinish(ctx, pctx, pipeline.OutcomeFromContext(pctx))
 		return rejectFromAction(action), nil
 	}
 
@@ -477,6 +479,7 @@ func (s *Server) handleOutbound(stream extprocv3.ExternalProcessor_ProcessServer
 	action := s.OutboundPipeline.Run(ctx, pctx)
 	if action.Type == pipeline.Reject {
 		s.recordOutboundReject(pctx, action)
+		s.OutboundPipeline.RunFinish(ctx, pctx, pipeline.OutcomeFromContext(pctx))
 		return rejectFromAction(action), nil
 	}
 
@@ -516,6 +519,7 @@ func (s *Server) handleOutboundBody(stream extprocv3.ExternalProcessor_ProcessSe
 	action := s.OutboundPipeline.Run(ctx, pctx)
 	if action.Type == pipeline.Reject {
 		s.recordOutboundReject(pctx, action)
+		s.OutboundPipeline.RunFinish(ctx, pctx, pipeline.OutcomeFromContext(pctx))
 		return rejectFromAction(action), nil
 	}
 

authbridge/authlib/observe/statserver.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/kagenti/kagenti-extensions/authbridge/authlib/auth"
 	"github.com/kagenti/kagenti-extensions/authbridge/authlib/config"
+	"github.com/kagenti/kagenti-extensions/authbridge/authlib/redact"
 )
 
 type StatServer struct {
@@ -88,15 +89,16 @@ func NewStatServer(addr string, configProvider ConfigProvider, statsProvider Sta
 func handleConfigFactory(provider ConfigProvider) func(http.ResponseWriter, *http.Request) {
 	return func(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("Content-Type", "application/json")
-		// Plugin config subtrees are captured verbatim as json.RawMessage
-		// by the PluginEntry unmarshaler. Operators shouldn't put
-		// secrets in the runtime config — the per-plugin convention is
-		// to reference a file path instead (client_secret_file, etc.) —
-		// so we render the config as-is. If a plugin ever needs to
-		// suppress a known-sensitive field here, it can be added to a
-		// redaction pass in a follow-up.
-		err := json.NewEncoder(w).Encode(provider())
+		raw, err := json.Marshal(provider())
 		if err != nil {
+			slog.Default().Info("Failed to marshal configuration", "err", err)
+			w.WriteHeader(http.StatusInternalServerError)
+			_, _ = w.Write([]byte(`{"error":"marshal failed"}` + "\n"))
+			return
+		}
+		redacted := redact.JSON(raw)
+		redacted = append(redacted, '\n')
+		if _, err := w.Write(redacted); err != nil {
 			slog.Default().Info("Failed to send configuration", "err", err)
 		}
 	}

authbridge/authlib/plugins/tokenbroker/plugin.go

@@ -13,6 +13,7 @@ import (
 	"strings"
 
 	"github.com/gobwas/glob"
+	"github.com/kagenti/kagenti-extensions/authbridge/authlib/auth"
 	"github.com/kagenti/kagenti-extensions/authbridge/authlib/pipeline"
 	"github.com/kagenti/kagenti-extensions/authbridge/authlib/plugins"
 	"github.com/kagenti/kagenti-extensions/authbridge/authlib/plugins/tokenbroker/client"
@@ -243,8 +244,7 @@ func (p *TokenBroker) OnRequest(ctx context.Context, pctx *pipeline.Context) pip
 		"server_url", serverURL,
 		"broker_url", brokerURL)
 
-	// Extract bearer token
-	subjectToken := extractBearer(authHeader)
+	subjectToken := strings.TrimSpace(auth.ExtractBearer(authHeader))
 	if subjectToken == "" {
 		return pctx.DenyAndRecord("missing_subject_token", "auth.missing-token",
 			"broker route requires authorization token")
@@ -330,15 +330,6 @@ func (p *TokenBroker) OnResponse(ctx context.Context, pctx *pipeline.Context) pi
 	return pipeline.Action{Type: pipeline.Continue}
 }
 
-// extractBearer extracts the bearer token from an Authorization header.
-func extractBearer(authHeader string) string {
-	const prefix = "Bearer "
-	if !strings.HasPrefix(authHeader, prefix) {
-		return ""
-	}
-	return strings.TrimSpace(strings.TrimPrefix(authHeader, prefix))
-}
-
 // loadBrokerRoutesFromFile loads broker routes from a YAML file.
 // Returns an empty slice (not error) if the file doesn't exist.
 func loadBrokerRoutesFromFile(path string) ([]tokenBrokerRoute, error) {

authbridge/authlib/plugins/tokenbroker/plugin_edge_test.go

@@ -10,6 +10,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/kagenti/kagenti-extensions/authbridge/authlib/auth"
 	"github.com/kagenti/kagenti-extensions/authbridge/authlib/pipeline"
 )
 
@@ -39,9 +40,9 @@ func TestExtractBearer_EdgeCases(t *testing.T) {
 			want:   "",
 		},
 		{
-			name:   "wrong case",
+			name:   "lowercase bearer (RFC 6750 case-insensitive)",
 			header: "bearer abc123",
-			want:   "",
+			want:   "abc123",
 		},
 		{
 			name:   "bearer with no token",
@@ -51,12 +52,12 @@ func TestExtractBearer_EdgeCases(t *testing.T) {
 		{
 			name:   "bearer with trailing whitespace",
 			header: "Bearer   ",
-			want:   "",
+			want:   "  ",
 		},
 		{
 			name:   "bearer with leading spaces in token",
 			header: "Bearer  token",
-			want:   "token",
+			want:   " token",
 		},
 		{
 			name:   "token with internal spaces",
@@ -67,9 +68,9 @@ func TestExtractBearer_EdgeCases(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := extractBearer(tt.header)
+			got := auth.ExtractBearer(tt.header)
 			if got != tt.want {
-				t.Errorf("extractBearer(%q) = %q, want %q", tt.header, got, tt.want)
+				t.Errorf("ExtractBearer(%q) = %q, want %q", tt.header, got, tt.want)
 			}
 		})
 	}
@@ -484,7 +485,7 @@ func TestTokenBroker_OnRequest_DifferentAuthSchemes(t *testing.T) {
 		{"Digest auth", "Digest username=\"user\"", true},
 		{"No auth", "", true},
 		{"Malformed Bearer", "Bearertoken", true},
-		{"Bearer lowercase", "bearer token", true},
+		{"Bearer lowercase (RFC 6750 case-insensitive)", "bearer token", false},
 	}
 
 	for _, tt := range tests {
@@ -585,6 +586,6 @@ func BenchmarkExtractBearer(b *testing.B) {
 
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
-		_ = extractBearer(header)
+		_ = auth.ExtractBearer(header)
 	}
 }

authbridge/authlib/plugins/tokenexchange/cache/cache.go

@@ -24,7 +24,9 @@ type Cache struct {
 type Option func(*Cache)
 
 // WithMaxSize sets the maximum number of cache entries.
-// When exceeded, expired entries are evicted first; if still full, all entries are cleared.
+// When exceeded, expired entries are evicted first; if still full,
+// ~25% of entries are randomly evicted using Go's non-deterministic
+// map iteration order.
 func WithMaxSize(n int) Option {
 	return func(c *Cache) { c.maxSize = n }
 }
@@ -67,9 +69,7 @@ func (c *Cache) Set(subjectToken, audience, token string, ttl time.Duration) {
 	if len(c.entries) >= c.maxSize {
 		c.evictExpired()
 		if len(c.entries) >= c.maxSize {
-			// TODO: Consider LRU or random-sample eviction for high-cardinality
-			// traffic. Full clear can cause temporary cache-miss storms.
-			c.entries = make(map[string]entry)
+			c.evictRandom()
 		}
 	}
 	c.entries[key] = entry{
@@ -85,6 +85,16 @@ func (c *Cache) Len() int {
 	return len(c.entries)
 }
 
+func (c *Cache) evictRandom() {
+	target := c.maxSize * 3 / 4
+	for k := range c.entries {
+		if len(c.entries) <= target {
+			break
+		}
+		delete(c.entries, k)
+	}
+}
+
 func (c *Cache) evictExpired() {
 	now := time.Now()
 	for k, e := range c.entries {

authbridge/authlib/plugins/tokenexchange/cache/cache_test.go

@@ -65,6 +65,30 @@ func TestCacheKeyCollisionResistance(t *testing.T) {
 	}
 }
 
+func TestMaxSize_RandomEviction(t *testing.T) {
+	const maxSize = 100
+	c := New(WithMaxSize(maxSize))
+	for i := range maxSize {
+		c.Set("token"+string(rune(i)), "aud", "val", 5*time.Minute)
+	}
+	if c.Len() != maxSize {
+		t.Fatalf("pre-check: cache len = %d, want %d", c.Len(), maxSize)
+	}
+
+	c.Set("overflow", "aud", "val", 5*time.Minute)
+
+	got := c.Len()
+	// After eviction ~25% is removed, so we expect roughly 75 + 1 = 76 entries.
+	// Allow some margin: between 70 and 80.
+	if got < 70 || got > 80 {
+		t.Errorf("after overflow: cache len = %d, want ~76 (75%% of %d + 1)", got, maxSize)
+	}
+	// The new entry must be present.
+	if _, ok := c.Get("overflow", "aud"); !ok {
+		t.Error("overflow entry not found after eviction")
+	}
+}
+
 func TestConcurrentAccess(t *testing.T) {
 	c := New()
 	done := make(chan struct{})

authbridge/authlib/redact/redact.go

@@ -0,0 +1,73 @@
+// Package redact provides best-effort stripping of sensitive values from
+// JSON config payloads served by unauthenticated diagnostic endpoints.
+// The canonical defense is to keep inline secrets out of config entirely
+// (use *_file paths instead); this layer is defense-in-depth.
+package redact
+
+import (
+	"encoding/json"
+	"strings"
+)
+
+var sensitiveKeys = []string{
+	"secret", "password", "token", "bearer", "key", "credential",
+}
+
+// JSON redacts values whose keys match sensitive patterns in a
+// json.RawMessage. Non-object inputs are returned unchanged.
+func JSON(raw json.RawMessage) json.RawMessage {
+	if len(raw) == 0 {
+		return raw
+	}
+
+	var m map[string]any
+	if err := json.Unmarshal(raw, &m); err != nil {
+		return raw
+	}
+
+	redactMap(m)
+
+	out, err := json.Marshal(m)
+	if err != nil {
+		return raw
+	}
+	return out
+}
+
+func redactMap(m map[string]any) {
+	for k, v := range m {
+		if isSensitiveKey(k) {
+			if _, ok := v.(string); ok {
+				m[k] = "[REDACTED]"
+				continue
+			}
+		}
+		switch val := v.(type) {
+		case map[string]any:
+			redactMap(val)
+		case []any:
+			redactSlice(val)
+		}
+	}
+}
+
+func redactSlice(s []any) {
+	for _, v := range s {
+		switch val := v.(type) {
+		case map[string]any:
+			redactMap(val)
+		case []any:
+			redactSlice(val)
+		}
+	}
+}
+
+func isSensitiveKey(key string) bool {
+	lower := strings.ToLower(key)
+	for _, s := range sensitiveKeys {
+		if strings.HasSuffix(lower, s) {
+			return true
+		}
+	}
+	return false
+}