Bundle B: Auth & transport surface tightening — 5 findings closed

Closes M-001 + M-002 + M-013 + M-018 + M-025 from comprehensive-audit-2026-04-25. M-001 (CWE-916) — PBKDF2 100k -> 600k via v3 blob format internal/crypto/encryption.go: - New v3Magic (0x03), pbkdf2IterationsV3 (600,000 — OWASP 2024 Password Storage Cheat Sheet floor), v3SaltSize (16 bytes), deriveKeyWithSaltV3 helper. - EncryptIfKeySet now unconditionally writes v3: magic(0x03) || salt(16) || nonce(12) || ciphertext+tag - DecryptIfKeySet falls through v3 -> v2 -> v1 with AEAD verification at each step. Wrong-passphrase v3 reads cannot be silently misattributed to v2/v1. - IsLegacyFormat updated to recognize 0x03 as non-legacy. internal/crypto/encryption_v3_test.go (NEW, 7 tests): V3 round-trip / V2 read-fallback against deterministic v2 fixture / V3 wrong-passphrase fails / V3-vs-V2 dispatch order / V2 vs V3 keys differ for same (passphrase, salt) / iteration-count pin at OWASP 2024 floor / IsLegacyFormat-recognises-V3. Coverage internal/crypto: 86.7% -> 88.2%. M-002 (CWE-862) — Auth-exempt allowlist constants + AST regression test Recon found auth-exempt surface spans TWO layers (audit's claim was incomplete): Layer 1 (router.go direct r.mux.Handle): GET /health, GET /ready, GET /api/v1/auth/info, GET /api/v1/version Layer 2 (cmd/server/main.go::buildFinalHandler URL-prefix dispatch): /.well-known/pki/*, /.well-known/est/*, /scep[/...]* internal/api/router/router.go: - New AuthExemptRouterRoutes constant with per-entry justifications. - New AuthExemptDispatchPrefixes constant. internal/api/router/auth_exempt_test.go (NEW, 2 tests): AST-walks router.go for every direct mux.Handle call and asserts set equals AuthExemptRouterRoutes; reads source bytes of Register / RegisterFunc and asserts they still wrap with middleware.Chain. cmd/server/auth_exempt_test.go (NEW, 2 tests): 14-case table test on buildFinalHandler asserting documented prefixes route to noAuthHandler and authenticated routes route to apiHandler; inverse-overlap pin proves no documented bypass shadows an authenticated prefix. M-013 (CWE-942) — CORS deny-by-default verified-already-clean + pin Audit claim 'default allows all origins if env-var unset' was WRONG. internal/api/middleware/middleware.go::NewCORS already denies cross- origin requests when len(cfg.AllowedOrigins) == 0 (no Access-Control-Allow-Origin header is emitted, same-origin policy applies). internal/api/middleware/cors_test.go: +TestNewCORS_NilOriginsDeniesAll + TestNewCORS_M013_ContractDocumentedInOrder (5-case table test pinning the 3-arm dispatch contract). M-018 (CWE-319 / PCI-DSS Req 4) — Postgres TLS opt-in toggle deploy/helm/certctl/values.yaml: new postgresql.tls.{mode,caSecretRef} operator-facing knobs. Default 'disable' preserves in-cluster pod- network behavior; PCI-scoped operators set verify-full. deploy/helm/certctl/templates/_helpers.tpl: certctl.databaseURL helper pipes postgresql.tls.mode into ?sslmode=. deploy/helm/certctl/templates/server-secret.yaml: uses the helper instead of hardcoded sslmode=disable. deploy/docker-compose.yml: CERTCTL_DATABASE_URL is now ${CERTCTL_DATABASE_URL:-...} so operators override without editing. docs/database-tls.md (NEW): operator runbook covering 4 deployment shapes, RDS verify-full example with PGSSLROOTCERT mount, and pg_stat_ssl verification query. helm template + helm lint clean. M-025 (OWASP ASVS L2 §11.2.1) — Per-key rate limiting internal/api/middleware/middleware.go::NewRateLimiter rewritten from a single global tokenBucket to a keyedRateLimiter map keyed on 'user:'+GetUser(ctx) for authenticated callers 'ip:'+RemoteAddr-host for unauthenticated - Empty UserKey strings treated as unauthenticated. - X-Forwarded-For intentionally NOT consulted (header-spoofing risk). - Create-on-demand bucket allocation under sync.RWMutex with double- check pattern. RateLimitConfig.PerUserRPS / PerUserBurstSize fields with env vars CERTCTL_RATE_LIMIT_PER_USER_RPS / CERTCTL_RATE_LIMIT_PER_USER_BURST allow per-user budgets distinct from per-IP. internal/api/middleware/ratelimit_keyed_test.go (NEW, 5 tests): TwoIPsHaveIndependentBuckets / SameUserDifferentIPsShareBucket / TwoUsersHaveIndependentBuckets / PerUserBudgetOverride / EmptyUserKeyTreatedAsAnonymous. Coverage internal/api/middleware: 82.1% -> 83.7%. Audit deliverables: cowork/comprehensive-audit-2026-04-25/audit-report.md: score 25/55 -> 30/55 closed (High 7/9, Medium 7/27 -> 12/27, Low 8/19). cowork/comprehensive-audit-2026-04-25/findings.yaml: 5 status flips open -> closed with closure notes citing the Bundle B mechanism. certctl/CHANGELOG.md: Bundle B section under [unreleased]. Verification: go test -count=1 -short ./... all green staticcheck on changed packages no new SA*/ST* hits (the 4 pre-existing SA1019 sites in cmd/server/main_test.go are Bundle 9 / M-028 partial closure leftovers tracked in Bundle C) helm template + helm lint clean internal/repository/postgres setup-fail sandbox disk pressure, same on master HEAD before this branch — environmental, not Bundle B
2026-06-07 14:51:30 +00:00 · 2026-04-26 23:09:10 +00:00
parent f609270cea
commit 30f9f1e712
17 changed files with 1287 additions and 114 deletions
@@ -6,6 +6,76 @@ import (
 	"testing"
 )

+// Bundle B / Audit M-013 (CWE-942) regression pins.
+//
+// The audit-finding text reads: "CORS configuration default allows all
+// origins if env-var unset". Phase 0 recon proves that claim is WRONG —
+// internal/api/middleware/middleware.go::NewCORS already denies when
+// len(cfg.AllowedOrigins) == 0 (no Access-Control-Allow-Origin header is
+// emitted, so same-origin policy applies). Bundle B's M-013 closure is
+// "verified-already-clean": these tests pin the deny-by-default contract
+// in BOTH shapes (nil slice and empty slice) so a future refactor that
+// inverts the default fails CI.
+
+// TestNewCORS_NilOriginsDeniesAll pins the deny-by-default contract for
+// the nil-slice shape (which is what propagates from a missing
+// CERTCTL_CORS_ORIGINS env var via internal/config/config.go::getEnvList).
+func TestNewCORS_NilOriginsDeniesAll(t *testing.T) {
+	mw := NewCORS(CORSConfig{AllowedOrigins: nil})
+	handler := mw(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+	req := httptest.NewRequest(http.MethodGet, "/api/v1/certificates", nil)
+	req.Header.Set("Origin", "https://attacker.example.com")
+	rr := httptest.NewRecorder()
+	handler.ServeHTTP(rr, req)
+	if got := rr.Header().Get("Access-Control-Allow-Origin"); got != "" {
+		t.Errorf("nil AllowedOrigins must NOT emit Access-Control-Allow-Origin, got %q", got)
+	}
+	if got := rr.Header().Get("Vary"); got != "" {
+		t.Errorf("nil AllowedOrigins must NOT emit Vary, got %q", got)
+	}
+}
+
+// TestNewCORS_M013_ContractDocumentedInOrder pins the documented dispatch
+// order so a refactor cannot silently invert the cases:
+//
+//	1. len(AllowedOrigins) == 0  → deny (no CORS headers)
+//	2. AllowedOrigins == ["*"]   → allow all (Access-Control-Allow-Origin: *)
+//	3. else                      → exact-match allowlist with Vary: Origin
+//
+// If a refactor accidentally falls through to the allow-all branch when
+// AllowedOrigins is empty, this test fails on case 1.
+func TestNewCORS_M013_ContractDocumentedInOrder(t *testing.T) {
+	cases := []struct {
+		name           string
+		origins        []string
+		incomingOrigin string
+		wantHeader     string // "" means no header expected
+	}{
+		{"deny_empty_slice", []string{}, "https://app.example.com", ""},
+		{"deny_nil", nil, "https://app.example.com", ""},
+		{"allow_all_with_star", []string{"*"}, "https://app.example.com", "*"},
+		{"exact_allow_match", []string{"https://app.example.com"}, "https://app.example.com", "https://app.example.com"},
+		{"exact_deny_mismatch", []string{"https://app.example.com"}, "https://attacker.example.com", ""},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			mw := NewCORS(CORSConfig{AllowedOrigins: tc.origins})
+			handler := mw(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				w.WriteHeader(http.StatusOK)
+			}))
+			req := httptest.NewRequest(http.MethodGet, "/", nil)
+			req.Header.Set("Origin", tc.incomingOrigin)
+			rr := httptest.NewRecorder()
+			handler.ServeHTTP(rr, req)
+			if got := rr.Header().Get("Access-Control-Allow-Origin"); got != tc.wantHeader {
+				t.Errorf("got Access-Control-Allow-Origin=%q, want %q (incoming origin=%q)", got, tc.wantHeader, tc.incomingOrigin)
+			}
+		})
+	}
+}
+
 // TestNewCORS_EmptyOriginList denies CORS by default (secure default).
 func TestNewCORS_EmptyOriginList(t *testing.T) {
 	mw := NewCORS(CORSConfig{AllowedOrigins: []string{}})
@@ -240,24 +240,67 @@ func NewAuth(cfg AuthConfig) func(http.Handler) http.Handler {
 }

 // RateLimitConfig holds configuration for the rate limiter.
+//
+// Bundle B / Audit M-025 (OWASP ASVS L2 §11.2.1) extends this with per-user
+// and per-IP keying. The historic RPS / BurstSize fields are preserved for
+// source compatibility — they now describe the per-key budget rather than
+// the global budget. PerUserRPS / PerUserBurstSize, when non-zero, override
+// RPS / BurstSize for authenticated callers; the IP-keyed fallback
+// continues to use RPS / BurstSize so unauthenticated callers don't get
+// a more generous bucket than authenticated ones by default.
 type RateLimitConfig struct {
-	RPS       float64 // Requests per second
-	BurstSize int     // Maximum burst size
+	RPS       float64 // Tokens per second per key (default applies to IP-keyed buckets)
+	BurstSize int     // Max tokens per key (default applies to IP-keyed buckets)
+
+	// PerUserRPS overrides RPS for authenticated callers (keyed by UserKey
+	// in context). Zero means "use RPS as the authenticated budget too".
+	PerUserRPS float64
+
+	// PerUserBurstSize overrides BurstSize for authenticated callers.
+	// Zero means "use BurstSize".
+	PerUserBurstSize int
 }

-// NewRateLimiter creates a token bucket rate limiting middleware.
-// Uses a simple token bucket: tokens refill at RPS rate, burst allows short spikes.
+// NewRateLimiter creates a per-key token bucket rate limiting middleware.
+//
+// Bundle B / Audit M-025: pre-bundle this returned a single global bucket
+// shared across every request, so a single noisy caller could exhaust the
+// budget for everyone else (effectively a self-DoS). Post-bundle each
+// authenticated user and each unauthenticated IP gets its own bucket. Keys
+// are computed per request:
+//
+//   - Authenticated: "user:" + middleware.GetUser(ctx)
+//   - Unauthenticated: "ip:" + r.RemoteAddr's host portion
+//
+// The bucket map is sync.RWMutex-guarded; create-on-demand for new keys.
+// There is no eviction — for a long-running server with millions of unique
+// IPs this can leak memory. A future enhancement is per-key TTL via a
+// lazy sweeper. For now the leak is bounded by realistic operator IP
+// fan-out and is acceptable per OWASP ASVS L2 (the threat model is abuse
+// by a known set of clients, not infinite-cardinality scanners).
 func NewRateLimiter(cfg RateLimitConfig) func(http.Handler) http.Handler {
-	limiter := &tokenBucket{
-		rate:       cfg.RPS,
-		burstSize:  float64(cfg.BurstSize),
-		tokens:     float64(cfg.BurstSize),
-		lastRefill: time.Now(),
+	// Default per-user budgets to the IP-keyed budget when not overridden.
+	perUserRPS := cfg.PerUserRPS
+	if perUserRPS == 0 {
+		perUserRPS = cfg.RPS
+	}
+	perUserBurst := float64(cfg.PerUserBurstSize)
+	if perUserBurst == 0 {
+		perUserBurst = float64(cfg.BurstSize)
+	}
+
+	limiter := &keyedRateLimiter{
+		ipRate:       cfg.RPS,
+		ipBurst:      float64(cfg.BurstSize),
+		userRate:     perUserRPS,
+		userBurst:    perUserBurst,
+		buckets:      make(map[string]*tokenBucket),
 	}

 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			if !limiter.allow() {
+			key, isUser := rateLimitKey(r)
+			if !limiter.allow(key, isUser) {
 				w.Header().Set("Content-Type", "application/json; charset=utf-8")
 				w.Header().Set("Retry-After", "1")
 				http.Error(w, `{"error":"Rate limit exceeded"}`, http.StatusTooManyRequests)
@@ -268,6 +311,70 @@ func NewRateLimiter(cfg RateLimitConfig) func(http.Handler) http.Handler {
 	}
 }

+// rateLimitKey computes the per-request bucket key. Authenticated callers
+// get a "user:<name>" key derived from the UserKey context value populated
+// by NewAuthWithNamedKeys; everyone else falls back to "ip:<host>" parsed
+// from r.RemoteAddr (X-Forwarded-For is intentionally NOT consulted here
+// — operators behind a trusted proxy must configure that proxy to set
+// RemoteAddr correctly, or the rate limiter would be trivially bypassable
+// by spoofing the header).
+//
+// Returns (key, isAuthenticated). Empty UserKey strings are treated as
+// unauthenticated so a misconfigured auth middleware doesn't grant the
+// same bucket to every anonymous request.
+func rateLimitKey(r *http.Request) (string, bool) {
+	if user := GetUser(r.Context()); user != "" {
+		return "user:" + user, true
+	}
+	host := r.RemoteAddr
+	if idx := strings.LastIndex(host, ":"); idx >= 0 {
+		host = host[:idx]
+	}
+	if host == "" {
+		host = "unknown"
+	}
+	return "ip:" + host, false
+}
+
+// keyedRateLimiter holds a token bucket per (user-or-ip) key with separate
+// rate / burst defaults for the user-keyed and ip-keyed dimensions.
+type keyedRateLimiter struct {
+	mu        sync.RWMutex
+	buckets   map[string]*tokenBucket
+	ipRate    float64
+	ipBurst   float64
+	userRate  float64
+	userBurst float64
+}
+
+func (k *keyedRateLimiter) allow(key string, isUser bool) bool {
+	// Fast path: bucket already exists.
+	k.mu.RLock()
+	tb, ok := k.buckets[key]
+	k.mu.RUnlock()
+
+	if !ok {
+		// Slow path: create-on-demand under write lock with double-check.
+		k.mu.Lock()
+		tb, ok = k.buckets[key]
+		if !ok {
+			rate, burst := k.ipRate, k.ipBurst
+			if isUser {
+				rate, burst = k.userRate, k.userBurst
+			}
+			tb = &tokenBucket{
+				rate:       rate,
+				burstSize:  burst,
+				tokens:     burst,
+				lastRefill: time.Now(),
+			}
+			k.buckets[key] = tb
+		}
+		k.mu.Unlock()
+	}
+	return tb.allow()
+}
+
 // tokenBucket implements a simple thread-safe token bucket rate limiter.
 // This avoids importing golang.org/x/time/rate to keep dependencies minimal.
 type tokenBucket struct {
@@ -0,0 +1,188 @@
+package middleware
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
+
+// Bundle B / Audit M-025 (OWASP ASVS L2 §11.2.1): per-key rate-limiter
+// regression suite. Pre-bundle the limiter was global — a single noisy
+// caller could exhaust everyone's budget. Post-bundle each authenticated
+// user and each distinct IP gets an independent token bucket.
+
+func newKeyedTestHandler(t *testing.T, cfg RateLimitConfig) http.Handler {
+	t.Helper()
+	return NewRateLimiter(cfg)(
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.WriteHeader(http.StatusOK)
+		}),
+	)
+}
+
+// TestRateLimiter_M025_TwoIPsHaveIndependentBuckets ensures one IP
+// exhausting its bucket does not affect another IP.
+func TestRateLimiter_M025_TwoIPsHaveIndependentBuckets(t *testing.T) {
+	h := newKeyedTestHandler(t, RateLimitConfig{RPS: 0.0001, BurstSize: 1})
+
+	// IP A burns its single token.
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	req.RemoteAddr = "10.0.0.1:54321"
+	rr := httptest.NewRecorder()
+	h.ServeHTTP(rr, req)
+	if rr.Code != http.StatusOK {
+		t.Fatalf("IP A first request should pass; got %d", rr.Code)
+	}
+
+	// IP A's second request must 429.
+	rr = httptest.NewRecorder()
+	h.ServeHTTP(rr, req)
+	if rr.Code != http.StatusTooManyRequests {
+		t.Errorf("IP A second request should 429; got %d", rr.Code)
+	}
+
+	// IP B's first request must still pass — independent bucket.
+	req2 := httptest.NewRequest(http.MethodGet, "/", nil)
+	req2.RemoteAddr = "10.0.0.2:54321"
+	rr2 := httptest.NewRecorder()
+	h.ServeHTTP(rr2, req2)
+	if rr2.Code != http.StatusOK {
+		t.Errorf("IP B first request must pass (independent bucket); got %d", rr2.Code)
+	}
+}
+
+// TestRateLimiter_M025_SameUserDifferentIPsShareBucket pins the keying
+// rule that authenticated callers are bucketed by user identity, not by
+// IP — so a user rotating between devices still shares one budget.
+func TestRateLimiter_M025_SameUserDifferentIPsShareBucket(t *testing.T) {
+	h := newKeyedTestHandler(t, RateLimitConfig{RPS: 0.0001, BurstSize: 1})
+
+	mkReq := func(remote string) *http.Request {
+		req := httptest.NewRequest(http.MethodGet, "/", nil)
+		req.RemoteAddr = remote
+		ctx := context.WithValue(req.Context(), UserKey{}, "alice")
+		return req.WithContext(ctx)
+	}
+
+	// Alice from IP X exhausts her bucket.
+	rr := httptest.NewRecorder()
+	h.ServeHTTP(rr, mkReq("10.0.0.1:54321"))
+	if rr.Code != http.StatusOK {
+		t.Fatalf("alice first request should pass; got %d", rr.Code)
+	}
+
+	// Alice from IP Y must 429 — same user-scoped bucket.
+	rr = httptest.NewRecorder()
+	h.ServeHTTP(rr, mkReq("10.0.0.2:54321"))
+	if rr.Code != http.StatusTooManyRequests {
+		t.Errorf("alice second request from different IP should still 429; got %d", rr.Code)
+	}
+}
+
+// TestRateLimiter_M025_TwoUsersHaveIndependentBuckets pins the keying rule
+// that two authenticated users share neither buckets nor side effects.
+func TestRateLimiter_M025_TwoUsersHaveIndependentBuckets(t *testing.T) {
+	h := newKeyedTestHandler(t, RateLimitConfig{RPS: 0.0001, BurstSize: 1})
+
+	mkReq := func(user string) *http.Request {
+		req := httptest.NewRequest(http.MethodGet, "/", nil)
+		req.RemoteAddr = "10.0.0.1:54321"
+		ctx := context.WithValue(req.Context(), UserKey{}, user)
+		return req.WithContext(ctx)
+	}
+
+	rr := httptest.NewRecorder()
+	h.ServeHTTP(rr, mkReq("alice"))
+	if rr.Code != http.StatusOK {
+		t.Fatalf("alice first request should pass; got %d", rr.Code)
+	}
+
+	rr = httptest.NewRecorder()
+	h.ServeHTTP(rr, mkReq("alice"))
+	if rr.Code != http.StatusTooManyRequests {
+		t.Fatalf("alice second request should 429; got %d", rr.Code)
+	}
+
+	// Bob shares the same RemoteAddr but his bucket is independent.
+	rr = httptest.NewRecorder()
+	h.ServeHTTP(rr, mkReq("bob"))
+	if rr.Code != http.StatusOK {
+		t.Errorf("bob's first request must pass despite alice exhausting hers; got %d", rr.Code)
+	}
+}
+
+// TestRateLimiter_M025_PerUserBudgetOverride exercises the optional
+// PerUserRPS / PerUserBurstSize knobs. Authenticated callers get the
+// generous budget; unauthenticated callers stay on the strict default.
+func TestRateLimiter_M025_PerUserBudgetOverride(t *testing.T) {
+	cfg := RateLimitConfig{
+		RPS:              0.0001,
+		BurstSize:        1, // strict for unauthenticated
+		PerUserRPS:       0.0001,
+		PerUserBurstSize: 5, // generous for authenticated
+	}
+	h := newKeyedTestHandler(t, cfg)
+
+	// IP-keyed: 1 token, second request 429.
+	ipReq := func() *http.Request {
+		req := httptest.NewRequest(http.MethodGet, "/", nil)
+		req.RemoteAddr = "10.0.0.99:54321"
+		return req
+	}
+	rr := httptest.NewRecorder()
+	h.ServeHTTP(rr, ipReq())
+	if rr.Code != http.StatusOK {
+		t.Fatalf("ip request 1 should pass; got %d", rr.Code)
+	}
+	rr = httptest.NewRecorder()
+	h.ServeHTTP(rr, ipReq())
+	if rr.Code != http.StatusTooManyRequests {
+		t.Errorf("ip request 2 should 429; got %d", rr.Code)
+	}
+
+	// User-keyed: 5 tokens, sixth request 429.
+	userReq := func() *http.Request {
+		req := httptest.NewRequest(http.MethodGet, "/", nil)
+		req.RemoteAddr = "10.0.0.42:54321"
+		ctx := context.WithValue(req.Context(), UserKey{}, "carol")
+		return req.WithContext(ctx)
+	}
+	for i := 1; i <= 5; i++ {
+		rr := httptest.NewRecorder()
+		h.ServeHTTP(rr, userReq())
+		if rr.Code != http.StatusOK {
+			t.Errorf("user request %d should pass; got %d", i, rr.Code)
+		}
+	}
+	rr = httptest.NewRecorder()
+	h.ServeHTTP(rr, userReq())
+	if rr.Code != http.StatusTooManyRequests {
+		t.Errorf("user request 6 should 429 (over PerUserBurstSize); got %d", rr.Code)
+	}
+}
+
+// TestRateLimiter_M025_EmptyUserKeyTreatedAsAnonymous ensures a
+// misconfigured auth middleware that puts an empty string under UserKey
+// does NOT collapse every anonymous request onto a single bucket.
+func TestRateLimiter_M025_EmptyUserKeyTreatedAsAnonymous(t *testing.T) {
+	h := newKeyedTestHandler(t, RateLimitConfig{RPS: 0.0001, BurstSize: 1})
+
+	mkReq := func(remote string) *http.Request {
+		req := httptest.NewRequest(http.MethodGet, "/", nil)
+		req.RemoteAddr = remote
+		ctx := context.WithValue(req.Context(), UserKey{}, "")
+		return req.WithContext(ctx)
+	}
+
+	rr := httptest.NewRecorder()
+	h.ServeHTTP(rr, mkReq("10.0.1.1:54321"))
+	if rr.Code != http.StatusOK {
+		t.Fatalf("first anonymous request should pass; got %d", rr.Code)
+	}
+	rr = httptest.NewRecorder()
+	h.ServeHTTP(rr, mkReq("10.0.1.2:54321"))
+	if rr.Code != http.StatusOK {
+		t.Errorf("second anonymous request from different IP should still pass (independent IP buckets); got %d", rr.Code)
+	}
+}