diff --git a/auth.go b/auth.go index 96359b7..c39ea74 100644 --- a/auth.go +++ b/auth.go @@ -4,6 +4,7 @@ import ( "crypto/sha256" "crypto/subtle" "fmt" + "net" "net/http" "net/url" "path" @@ -187,6 +188,40 @@ type authEntry struct { digest [32]byte } +// authFailureWindow and authFailureLimit bound how many failed identify +// attempts one source IP may make before further attempts from it are +// throttled outright (security audit finding 3, 2026-08-22): identify +// previously had no counter, no backoff, and no lockout at all — an +// unauthenticated caller got unlimited digest-lookup attempts at line +// rate. 20 failures per minute is generous headroom for a legitimate +// client with a stale or mistyped key retried a few times (a human +// pasting the wrong key, a misconfigured script cycling through a couple +// of candidates) while still meaningfully bounding a scripted guesser's +// throughput per source IP. This does not, and cannot, defend the +// underlying key space on its own (a SHA-256 digest lookup already makes +// brute force infeasible) — its real job is capping the synchronous cost +// an unauthenticated caller can impose on the shared Traefik ingress per +// unit time: identify's own digest lookup, and logAuthEvent's stderr +// write (logger.go), both of which this finding's own framing calls out. +const ( + authFailureWindow = time.Minute + authFailureLimit = 20 + // authFailureLogEvery rate-limits logAuthEvent's own failed-auth log + // line (logger.go) the identical way limiter.logStoreError (limits.go) + // already rate-limits store-error lines — sharing that function's own + // storeErrorLogEvery interval keeps this codebase's "how often do we + // log a sustained failure condition" cadence consistent across both + // call sites, rather than picking a new, unrelated number here. + authFailureLogEvery = storeErrorLogEvery +) + +// authFailureKeyPrefix namespaces authStore's failure-tracking keys in its +// own memoryStore instance (failures, below) — a plain string key, not +// windowKey's (kind, id, metric, window) tuple (limits.go), since auth +// failures are tracked per SOURCE IP, a concept the rest of this +// package's counter keying was never built to express. +const authFailureKeyPrefix = "authfail:" + // authStore identifies requests by API key and resolves them to a user and // their group. Raw API keys are never retained after construction — only // their SHA-256 digests. @@ -198,19 +233,35 @@ type authEntry struct { // overwrite each other. buildMu is never held at the same time as mu, so a // rebuild in progress never blocks identify's readers. reloadMu (Task 4) // serializes maybeReload's throttle check and the reload it may trigger. +// +// failures is identify's own per-source-IP failed-attempt counter +// (security audit finding 3, 2026-08-22) — the SAME in-process memoryStore +// type limiter's fallback store already uses (limits.go), a deliberately +// reused type rather than a new dependency, given its own TTL-bucketed +// counter semantics already fit this need exactly. It is authStore's own +// dedicated instance, not limiter.fallback: auth failures are a distinct +// concept (throttling an unauthenticated caller) from LLM traffic limits +// (throttling an authenticated one's usage), and authStore must work +// identically whether or not a Gateway even has a limiter wired yet. +// authFailLogMu/authFailLastLogAt back shouldLogAuthFailure, the identical +// "guard a timestamp with a small mutex, rate-limit a log line" shape +// limiter.logMu/lastLogAt already use. type authStore struct { - groups map[string]*group - inline map[[32]byte]*authEntry // built once by newAuthStore; never mutated afterward - byDigest map[[32]byte]*authEntry // inline entries plus the current file-sourced set; guarded by mu - usersFile *usersFile // nil when Users.File is not configured; maybeReload no-ops - log gatewayLogger // set alongside usersFile; unused when usersFile is nil - nowFn func() time.Time // injected for tests; defaults to time.Now - lastCheck time.Time // guarded by reloadMu - lastModTime time.Time // guarded by reloadMu - fileUserCount int // guarded by mu; size of the current file-sourced user set - mu sync.RWMutex - buildMu sync.Mutex - reloadMu sync.Mutex + groups map[string]*group + inline map[[32]byte]*authEntry // built once by newAuthStore; never mutated afterward + byDigest map[[32]byte]*authEntry // inline entries plus the current file-sourced set; guarded by mu + usersFile *usersFile // nil when Users.File is not configured; maybeReload no-ops + log gatewayLogger // set alongside usersFile; unused when usersFile is nil + nowFn func() time.Time // injected for tests; defaults to time.Now + failures *memoryStore // per-source-IP failed-identify counters; see doc comment above + lastCheck time.Time // guarded by reloadMu + lastModTime time.Time // guarded by reloadMu + authFailLastLogAt time.Time // guarded by authFailLogMu; last time a failed-auth line was logged + fileUserCount int // guarded by mu; size of the current file-sourced user set + mu sync.RWMutex + buildMu sync.Mutex + reloadMu sync.Mutex + authFailLogMu sync.Mutex } // newAuthStore builds an authStore from cfg's groups and inline users. It @@ -223,6 +274,13 @@ func newAuthStore(cfg *Config) (*authStore, error) { byDigest: make(map[[32]byte]*authEntry), nowFn: time.Now, } + a.failures = newMemoryStore() + // Delegate through a.nowFn rather than copying it: a test that + // overrides a.nowFn AFTER construction (the fakeClock convention + // newReloadableAuthStore already uses, users_file_test.go) must still + // control failures' own clock — this closure reads a.nowFn fresh on + // every call, so it always sees whichever func is currently assigned. + a.failures.nowFn = func() time.Time { return a.nowFn() } for name, gc := range cfg.Groups { if gc == nil { return nil, fmt.Errorf("llmgateway: group %q: config must not be nil", name) @@ -363,14 +421,95 @@ func (a *authStore) replaceFileUsers(us []*UserConfig) error { return nil } +// clientIP returns the source address identify's own throttle keys on: +// r.RemoteAddr with any ":port" suffix stripped — net/http's own "address +// as seen by this server's TCP accept", never a client-supplied header +// (security audit finding 3, 2026-08-22). This plugin runs AS a Traefik +// middleware inside the shared ingress process, not behind some second, +// untrusted proxy of its own, so RemoteAddr is the same trust boundary +// logAuthEvent's own RemoteAddr-only logging (logger.go) already assumes, +// and the same one routes_passthrough.go's dangerousClientHeaderPrefixes +// strip (X-Forwarded-*, X-Auth-*) exists to keep a client from spoofing on +// the OUTBOUND side — deliberately NOT read here either, for the same +// reason. A RemoteAddr with no ":port" (net.SplitHostPort's own error +// case — not expected from net/http in practice, but not assumed) is used +// as-is rather than discarded, so a throttle key still exists for +// whatever value RemoteAddr carries. +func clientIP(r *http.Request) string { + host, _, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + return r.RemoteAddr + } + return host +} + +// authThrottled reports whether ip has already reached authFailureLimit +// failed identify attempts within the current authFailureWindow — a +// read-only check, so a throttled IP's later attempts cost nothing beyond +// one in-process map read, never a digest computation or byDigest lookup. +func (a *authStore) authThrottled(ip string) bool { + count, _ := a.failures.get(authFailureKeyPrefix + ip) // memoryStore.get never errors + return count >= authFailureLimit +} + +// recordAuthFailure increments ip's failure count for the current +// authFailureWindow, creating a fresh window if the previous one already +// expired (memoryStore.incrBy's own fixed-window contract, limits.go). +func (a *authStore) recordAuthFailure(ip string) { + _, _ = a.failures.incrBy(authFailureKeyPrefix+ip, 1, authFailureWindow) // memoryStore.incrBy never errors +} + +// shouldLogAuthFailure reports whether the next failed-auth log line +// (logAuthEvent, logger.go) should actually be written, rate-limited to +// once per authFailureLogEvery regardless of how many attempts fail in +// between — the identical technique limiter.logStoreError (limits.go) +// already applies to store errors, applied here so an attacker driving +// unlimited failed-auth attempts can never drive unlimited synchronous +// stderr writes on the shared Traefik ingress (security audit finding 3, +// 2026-08-22) even before authThrottled's own per-IP cap engages (a +// distributed attempt spread across many source IPs never trips any +// single IP's own threshold, but must still not flood the log). +// Successful-auth logging is deliberately left unbounded: it requires an +// already-valid API key, so its volume is bounded by legitimate +// authenticated traffic — itself already governed by checkAndCount's +// requests-per-minute/day limits elsewhere — not by an unauthenticated +// caller's attempt rate, which is exactly what this finding is about +// bounding. +func (a *authStore) shouldLogAuthFailure() bool { + now := a.nowFn() + a.authFailLogMu.Lock() + defer a.authFailLogMu.Unlock() + if now.Sub(a.authFailLastLogAt) < authFailureLogEvery { + return false + } + a.authFailLastLogAt = now + return true +} + // identify resolves r's presented API key to a user and their group. It // reads "Authorization: Bearer " (case-insensitive "bearer" prefix) or // "x-api-key: "; Bearer wins when both are present. The presented key // is looked up by SHA-256 digest, then verified with a constant-time // compare — the key itself is never retained. +// +// Per-source-IP throttling (security audit finding 3, 2026-08-22): a +// request from an IP already over authThrottled's own budget is rejected +// immediately, before any digest work at all. Every OTHER failure path +// below — no key presented, key not found, or a (practically unreachable, +// see subtle.ConstantTimeCompare's own call site) digest mismatch — +// records one failure for the caller's IP via recordAuthFailure. A +// successful identify never records a failure and never resets a prior +// count: this is a rate limit on failed attempts, not a lockout a +// legitimate request can clear early. func (a *authStore) identify(r *http.Request) (*user, *group, bool) { + ip := clientIP(r) + if a.authThrottled(ip) { + return nil, nil, false + } + key, ok := presentedKey(r) if !ok { + a.recordAuthFailure(ip) return nil, nil, false } digest := sha256.Sum256([]byte(key)) @@ -379,9 +518,11 @@ func (a *authStore) identify(r *http.Request) (*user, *group, bool) { entry, ok := a.byDigest[digest] a.mu.RUnlock() if !ok { + a.recordAuthFailure(ip) return nil, nil, false } if subtle.ConstantTimeCompare(digest[:], entry.digest[:]) != 1 { + a.recordAuthFailure(ip) return nil, nil, false } return entry.user, entry.group, true diff --git a/auth_test.go b/auth_test.go index 40cca70..46030e6 100644 --- a/auth_test.go +++ b/auth_test.go @@ -577,3 +577,145 @@ func TestAuthStore_Identify_NotBlockedByBuildMu(t *testing.T) { t.Fatal("identify blocked while buildMu is held — reader lock must not depend on the writer-serialization lock") } } + +// --- security audit finding 3: per-source-IP auth-failure throttling --- + +func TestClientIP_StripsPort(t *testing.T) { + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = "203.0.113.9:54321" + if got := clientIP(r); got != "203.0.113.9" { + t.Errorf("clientIP = %q, want %q", got, "203.0.113.9") + } +} + +func TestClientIP_MalformedRemoteAddr_UsedAsIs(t *testing.T) { + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = "not-a-host-port" + if got := clientIP(r); got != "not-a-host-port" { + t.Errorf("clientIP = %q, want the raw RemoteAddr back unchanged", got) + } +} + +// TestAuthStore_Identify_ThrottlesAfterRepeatedFailures_EvenValidKeyRejected +// is the core regression for finding 3: identify previously had no +// counter, no backoff, and no lockout at all. After authFailureLimit +// failed attempts from one source IP, even a subsequently CORRECT key +// from that same IP must be rejected — proving the throttle actually +// short-circuits real auth once tripped, not merely that repeated wrong +// keys keep failing (which they would anyway). A different source IP must +// be entirely unaffected, proving the throttle is per-IP, not global. +func TestAuthStore_Identify_ThrottlesAfterRepeatedFailures_EvenValidKeyRejected(t *testing.T) { + a, err := newAuthStore(testAuthCfg()) // inline user "a" / sk-secret + if err != nil { + t.Fatal(err) + } + const attackerIP = "203.0.113.9:54321" + + for i := 0; i < authFailureLimit; i++ { + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = attackerIP + r.Header.Set("x-api-key", "wrong-key") + if _, _, ok := a.identify(r); ok { + t.Fatalf("attempt %d: want failure for a wrong key, got success", i) + } + } + + // Throttle should now be engaged for attackerIP — even the CORRECT + // key must be rejected. + valid := httptest.NewRequest("POST", "/v1/chat/completions", nil) + valid.RemoteAddr = attackerIP + valid.Header.Set("Authorization", "Bearer sk-secret") + if _, _, ok := a.identify(valid); ok { + t.Fatal("want a throttled IP rejected even with a valid key") + } + + // A different source IP must be unaffected by attackerIP's throttle. + other := httptest.NewRequest("POST", "/v1/chat/completions", nil) + other.RemoteAddr = "198.51.100.5:1111" + other.Header.Set("Authorization", "Bearer sk-secret") + if _, _, ok := a.identify(other); !ok { + t.Fatal("want a different source IP unaffected by another IP's throttle") + } +} + +// TestAuthStore_Identify_ThrottleResetsAfterWindowExpires proves the +// throttle is a fixed-window rate limit, not a permanent lockout: once +// authFailureWindow has actually elapsed, the same IP's valid key works +// again. +func TestAuthStore_Identify_ThrottleResetsAfterWindowExpires(t *testing.T) { + a, err := newAuthStore(testAuthCfg()) // inline user "a" / sk-secret + if err != nil { + t.Fatal(err) + } + clock := &fakeClock{now: time.Unix(1_700_000_000, 0)} + a.nowFn = clock.Now + const attackerIP = "203.0.113.9:54321" + + for i := 0; i < authFailureLimit; i++ { + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = attackerIP + r.Header.Set("x-api-key", "wrong-key") + if _, _, ok := a.identify(r); ok { + t.Fatalf("attempt %d: want failure for a wrong key, got success", i) + } + } + blocked := httptest.NewRequest("POST", "/v1/chat/completions", nil) + blocked.RemoteAddr = attackerIP + blocked.Header.Set("Authorization", "Bearer sk-secret") + if _, _, ok := a.identify(blocked); ok { + t.Fatal("want throttled before the window elapses") + } + + clock.Advance(authFailureWindow + time.Second) + + recovered := httptest.NewRequest("POST", "/v1/chat/completions", nil) + recovered.RemoteAddr = attackerIP + recovered.Header.Set("Authorization", "Bearer sk-secret") + if _, _, ok := a.identify(recovered); !ok { + t.Fatal("want the same IP's valid key to work again once authFailureWindow has elapsed") + } +} + +// TestAuthStore_Identify_FewFailedAttempts_NeverThrottled is the good-path +// counterpart: a handful of failed attempts (well under authFailureLimit +// — a human retrying a mistyped key a couple of times) must never trip +// the throttle. +func TestAuthStore_Identify_FewFailedAttempts_NeverThrottled(t *testing.T) { + a, err := newAuthStore(testAuthCfg()) + if err != nil { + t.Fatal(err) + } + const ip = "203.0.113.9:54321" + for i := 0; i < 3; i++ { + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = ip + r.Header.Set("x-api-key", "wrong-key") + a.identify(r) + } + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = ip + r.Header.Set("Authorization", "Bearer sk-secret") + if _, _, ok := a.identify(r); !ok { + t.Fatal("want a valid key to succeed after only 3 prior failures (well under authFailureLimit)") + } +} + +// TestAuthStore_Identify_SuccessfulAuthDoesNotRecordFailure proves a +// successful identify never itself counts toward the failure budget — +// otherwise a legitimate, high-traffic user could eventually throttle +// themselves. +func TestAuthStore_Identify_SuccessfulAuthDoesNotRecordFailure(t *testing.T) { + a, err := newAuthStore(testAuthCfg()) + if err != nil { + t.Fatal(err) + } + const ip = "203.0.113.9:54321" + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = ip + r.Header.Set("Authorization", "Bearer sk-secret") + for i := 0; i < authFailureLimit+5; i++ { + if _, _, ok := a.identify(r); !ok { + t.Fatalf("attempt %d: want success (valid key), got failure", i) + } + } +} diff --git a/logger.go b/logger.go index 1af1c0a..4c9097a 100644 --- a/logger.go +++ b/logger.go @@ -25,9 +25,21 @@ func (g *Gateway) errorf(format string, args ...any) { // line ever includes the presented API key itself — identify resolves a // key to a *user and hands this function only that user's name, never the // key it authenticated. +// +// The failure line is rate-limited via g.auth.shouldLogAuthFailure +// (auth.go), the same technique limiter.logStoreError (limits.go) already +// applies to store-error lines (security audit finding 3, 2026-08-22): an +// unauthenticated caller driving unlimited failed attempts must never be +// able to drive unlimited synchronous stderr writes on the shared Traefik +// ingress — a slow log pipe would otherwise stall request handling for +// every tenant, not just the one failing auth. The success line stays +// unbounded — see shouldLogAuthFailure's own doc comment for why that is +// safe. func (g *Gateway) logAuthEvent(ok bool, userName string, r *http.Request) { if !ok { - g.logf("auth failed: %s %q from %s", r.Method, r.URL.Path, r.RemoteAddr) + if g.auth.shouldLogAuthFailure() { + g.logf("auth failed: %s %q from %s", r.Method, r.URL.Path, r.RemoteAddr) + } return } g.logf("auth ok: user %q %s %q", userName, r.Method, r.URL.Path) diff --git a/logger_test.go b/logger_test.go new file mode 100644 index 0000000..cc9bafb --- /dev/null +++ b/logger_test.go @@ -0,0 +1,124 @@ +package traefikllmgateway + +import ( + "bytes" + "context" + "io" + "net/http" + "net/http/httptest" + "os" + "strings" + "testing" + "time" +) + +// newTestGatewayForLogger builds a minimal *Gateway via the real New() +// constructor (the only supported construction path — see this package's +// other test files, none of which build a bare &Gateway{} literal) for +// logAuthEvent's own tests below. +func newTestGatewayForLogger(t *testing.T) *Gateway { + t.Helper() + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}} + h, err := New(context.Background(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + gw, ok := h.(*Gateway) + if !ok { + t.Fatal("handler is not *Gateway") + } + return gw +} + +// captureStderr redirects os.Stderr for the duration of fn (the same +// os.Pipe-based idiom this package's other test files already use for +// logf/errorf, e.g. TestGateway_Logf_WritesInfoPrefix, llmgateway_test.go) +// and returns everything written to it. +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + origStderr := os.Stderr + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + os.Stderr = w + fn() + _ = w.Close() + os.Stderr = origStderr + var buf bytes.Buffer + if _, err := io.Copy(&buf, r); err != nil { + t.Fatalf("io.Copy: %v", err) + } + return buf.String() +} + +// TestLogAuthEvent_Failure_RateLimited is the regression for the missing +// rate limit on logAuthEvent's failure line (security audit finding 3, +// 2026-08-22): an attacker driving many failed-auth attempts in quick +// succession must produce ONE log line per authFailureLogEvery, not one +// per attempt — otherwise the synchronous os.Stderr write itself becomes +// an amplification vector on the shared Traefik ingress. +func TestLogAuthEvent_Failure_RateLimited(t *testing.T) { + gw := newTestGatewayForLogger(t) + clock := &fakeClock{now: time.Unix(1_700_000_000, 0)} + gw.auth.nowFn = clock.Now + + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = "203.0.113.9:1234" + + out := captureStderr(t, func() { + for i := 0; i < 5; i++ { + gw.logAuthEvent(false, "", r) + } + }) + + if n := strings.Count(out, "auth failed:"); n != 1 { + t.Fatalf("auth-failed lines = %d, want exactly 1 (5 attempts within one authFailureLogEvery window), output=%q", n, out) + } +} + +// TestLogAuthEvent_Failure_LogsAgainAfterIntervalElapses proves the rate +// limit is a cadence, not a one-shot silence: once authFailureLogEvery has +// actually elapsed, the next failure is logged again. +func TestLogAuthEvent_Failure_LogsAgainAfterIntervalElapses(t *testing.T) { + gw := newTestGatewayForLogger(t) + clock := &fakeClock{now: time.Unix(1_700_000_000, 0)} + gw.auth.nowFn = clock.Now + + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = "203.0.113.9:1234" + + out := captureStderr(t, func() { + gw.logAuthEvent(false, "", r) + clock.Advance(authFailureLogEvery + time.Second) + gw.logAuthEvent(false, "", r) + }) + + if n := strings.Count(out, "auth failed:"); n != 2 { + t.Fatalf("auth-failed lines = %d, want exactly 2 (interval elapsed between the two attempts), output=%q", n, out) + } +} + +// TestLogAuthEvent_Success_NeverRateLimited proves successful-auth logging +// is deliberately left unbounded (shouldLogAuthFailure's own doc comment, +// auth.go, justifies why): it takes an already-valid key to reach, so its +// volume is bounded by legitimate traffic elsewhere, not by an +// unauthenticated caller's attempt rate. +func TestLogAuthEvent_Success_NeverRateLimited(t *testing.T) { + gw := newTestGatewayForLogger(t) + clock := &fakeClock{now: time.Unix(1_700_000_000, 0)} + gw.auth.nowFn = clock.Now + + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + + out := captureStderr(t, func() { + for i := 0; i < 5; i++ { + gw.logAuthEvent(true, "alice", r) + } + }) + + if n := strings.Count(out, "auth ok:"); n != 5 { + t.Fatalf("auth-ok lines = %d, want 5 (success logging must never be rate-limited), output=%q", n, out) + } +}