mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 17:22:07 +00:00
30f9f1e712
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
189 lines
6.3 KiB
Go
189 lines
6.3 KiB
Go
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)
|
|
}
|
|
}
|