mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-10 05:58:52 +00:00
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
This commit is contained in:
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user