diff --git a/cmd/smoketest/redis-rate-limiter/main.go b/cmd/smoketest/redis-rate-limiter/main.go new file mode 100644 index 0000000..21c1d22 --- /dev/null +++ b/cmd/smoketest/redis-rate-limiter/main.go @@ -0,0 +1,778 @@ +// Command redis-rate-limiter is a standalone smoke test for the +// Redis-backed rate limiter: the limit itself, that a window is fixed +// rather than pushed out by the traffic it is denying, that keys are +// namespaced so they cannot land on the host app's own data, that two +// engine instances pointed at one Redis share a window (the entire +// reason this implementation exists), that a counter left without an +// expiry heals instead of locking its key out forever, and what happens +// to a login when Redis is unreachable. +// +// Run with: +// +// go run ./cmd/smoketest/redis-rate-limiter +// +// That needs no server: it runs against an in-process stand-in that +// implements the INCR/PTTL/PEXPIRE semantics the limiter's Lua is +// written against. The stand-in cannot prove the Lua is valid Lua, so +// when a real server is at hand, point this at it — the same checks +// then run against the actual script, and the ones that need to freeze +// time or break the connection announce themselves as skipped: +// +// REDIS_ADDR=127.0.0.1:6379 go run ./cmd/smoketest/redis-rate-limiter +package main + +import ( + "context" + "errors" + "fmt" + "os" + "time" + + "github.com/crydensync/cryden/v2" + "github.com/crydensync/cryden/v2/auth" + "github.com/crydensync/cryden/v2/security" + "github.com/crydensync/cryden/v2/store/memory" + "github.com/redis/go-redis/v9" +) + +const ( + email = "raymondproguy@dev.com" + password = "Tr0ubl3-Fr33!2026" + + callerIP = "1.2.3.4" +) + +var failures int + +// server is the Redis a scenario runs against: the Scripter the limiter +// needs, plus the few things only a test asks of a server — reading a +// counter's TTL, planting a counter that has none, moving past a window, +// and (stand-in only) breaking every command or counting round trips. +type server interface { + redis.Scripter + + // exists reports whether a key is present, which is how the key a + // limiter built is checked from outside it. + exists(ctx context.Context, key string) (bool, error) + // ttl is the counter's remaining window, negative when it has none. + ttl(ctx context.Context, key string) (time.Duration, error) + // seedWithoutTTL plants a counter with no expiry — the state the + // script's PTTL branch exists to recover from. + seedWithoutTTL(ctx context.Context, key string, hits int64) error + // waitOut moves past a window of d: instantly for the stand-in, + // by actually waiting for a real server. + waitOut(d time.Duration) + // breakable reports whether this server can be made to fail on + // command. A real one cannot be, not from in here. + breakable() bool + // breakWith makes every command fail with err until cleared. + breakWith(err error) + // roundTrips is how many EVALSHA and EVAL calls have been made, or + // (0, 0, false) when the server does not count them. + roundTrips() (evalSha, eval int, counted bool) + cleanup(ctx context.Context) +} + +// standIn is Redis as far as this limiter is concerned: INCR, PTTL, +// PEXPIRE, lazy expiry of a key whose window has closed, and the +// EVALSHA-NOSCRIPT-EVAL handshake go-redis performs against a server +// whose script cache is cold. Single-goroutine by construction, like the +// rest of this program, so nothing here locks. +type standIn struct { + counts map[string]int64 + expiry map[string]time.Time + now time.Time + cached bool + err error + + evalSha, eval int +} + +func newStandIn() *standIn { + return &standIn{ + counts: map[string]int64{}, + expiry: map[string]time.Time{}, + now: time.Date(2026, 9, 4, 12, 0, 0, 0, time.UTC), + } +} + +// script applies security's redisFixedWindowLimit, kept line-for-line +// with the Lua it stands in for. +func (s *standIn) script(keys []string, args []any) (int64, error) { + key := keys[0] + windowMS, err := argInt64(args[0]) + if err != nil { + return 0, err + } + limit, err := argInt64(args[1]) + if err != nil { + return 0, err + } + + // A real server drops an expired key before any command reads it. + if exp, ok := s.expiry[key]; ok && !s.now.Before(exp) { + delete(s.counts, key) + delete(s.expiry, key) + } + + s.counts[key]++ + hits := s.counts[key] + if hits == 1 || s.pttl(key) < 0 { + s.expiry[key] = s.now.Add(time.Duration(windowMS) * time.Millisecond) + } + if hits > limit { + return 0, nil + } + return 1, nil +} + +// pttl is PTTL for a key already known to exist: what is left of its +// window, or -1 when no expiry was ever set on it. +func (s *standIn) pttl(key string) int64 { + exp, ok := s.expiry[key] + if !ok { + return -1 + } + return exp.Sub(s.now).Milliseconds() +} + +// argInt64 is also an assertion: Redis takes arguments as strings and +// Lua's tonumber would accept a float, so a window arriving as a +// time.Duration rather than a count of milliseconds has to be rejected +// here instead of quietly becoming a window a million times too long. +func argInt64(arg any) (int64, error) { + switch v := arg.(type) { + case int64: + return v, nil + case int: + return int64(v), nil + default: + return 0, fmt.Errorf("script argument %#v has unexpected type %T", arg, arg) + } +} + +func (s *standIn) Eval(_ context.Context, _ string, keys []string, args ...any) *redis.Cmd { + s.eval++ + if s.err != nil { + // A failing EVAL never reached the script cache, so it must not + // leave the stand-in claiming the body is loaded. + return redis.NewCmdResult(nil, s.err) + } + s.cached = true + return redis.NewCmdResult(s.script(keys, args)) +} + +func (s *standIn) EvalSha(_ context.Context, _ string, keys []string, args ...any) *redis.Cmd { + s.evalSha++ + if s.err != nil { + return redis.NewCmdResult(nil, s.err) + } + if !s.cached { + return redis.NewCmdResult(nil, redis.ErrNoScript) + } + return redis.NewCmdResult(s.script(keys, args)) +} + +func (s *standIn) EvalRO(ctx context.Context, script string, keys []string, args ...any) *redis.Cmd { + return s.Eval(ctx, script, keys, args...) +} + +func (s *standIn) EvalShaRO(ctx context.Context, sha1 string, keys []string, args ...any) *redis.Cmd { + return s.EvalSha(ctx, sha1, keys, args...) +} + +func (s *standIn) ScriptExists(_ context.Context, _ ...string) *redis.BoolSliceCmd { + return redis.NewBoolSliceResult([]bool{s.cached}, s.err) +} + +func (s *standIn) ScriptLoad(_ context.Context, _ string) *redis.StringCmd { + if s.err != nil { + return redis.NewStringResult("", s.err) + } + s.cached = true + return redis.NewStringResult("stand-in-sha", nil) +} + +func (s *standIn) exists(_ context.Context, key string) (bool, error) { + if exp, ok := s.expiry[key]; ok && !s.now.Before(exp) { + return false, nil + } + _, ok := s.counts[key] + return ok, nil +} + +func (s *standIn) ttl(_ context.Context, key string) (time.Duration, error) { + if _, ok := s.counts[key]; !ok { + return -2, nil + } + return time.Duration(s.pttl(key)) * time.Millisecond, nil +} + +func (s *standIn) seedWithoutTTL(_ context.Context, key string, hits int64) error { + s.counts[key] = hits + delete(s.expiry, key) + return nil +} + +func (s *standIn) waitOut(d time.Duration) { s.now = s.now.Add(d) } +func (s *standIn) breakable() bool { return true } +func (s *standIn) breakWith(err error) { s.err = err } +func (s *standIn) roundTrips() (int, int, bool) { return s.evalSha, s.eval, true } +func (s *standIn) cleanup(_ context.Context) {} + +var _ server = (*standIn)(nil) + +// realServer is a live Redis, used when REDIS_ADDR is set: the checks +// below then run against the actual Lua instead of a model of it. Every +// key the limiter touches is recorded, so cleanup can delete exactly +// those and leave anything else on that server alone. +type realServer struct { + client *redis.Client + touched map[string]bool +} + +func dialRealServer(ctx context.Context, addr string) (*realServer, error) { + client := redis.NewClient(&redis.Options{Addr: addr}) + if err := client.Ping(ctx).Err(); err != nil { + _ = client.Close() + return nil, err + } + return &realServer{client: client, touched: map[string]bool{}}, nil +} + +func (s *realServer) note(keys []string) { + for _, k := range keys { + s.touched[k] = true + } +} + +func (s *realServer) Eval(ctx context.Context, script string, keys []string, args ...any) *redis.Cmd { + s.note(keys) + return s.client.Eval(ctx, script, keys, args...) +} + +func (s *realServer) EvalSha(ctx context.Context, sha1 string, keys []string, args ...any) *redis.Cmd { + s.note(keys) + return s.client.EvalSha(ctx, sha1, keys, args...) +} + +func (s *realServer) EvalRO(ctx context.Context, script string, keys []string, args ...any) *redis.Cmd { + s.note(keys) + return s.client.EvalRO(ctx, script, keys, args...) +} + +func (s *realServer) EvalShaRO(ctx context.Context, sha1 string, keys []string, args ...any) *redis.Cmd { + s.note(keys) + return s.client.EvalShaRO(ctx, sha1, keys, args...) +} + +func (s *realServer) ScriptExists(ctx context.Context, hashes ...string) *redis.BoolSliceCmd { + return s.client.ScriptExists(ctx, hashes...) +} + +func (s *realServer) ScriptLoad(ctx context.Context, script string) *redis.StringCmd { + return s.client.ScriptLoad(ctx, script) +} + +func (s *realServer) exists(ctx context.Context, key string) (bool, error) { + n, err := s.client.Exists(ctx, key).Result() + return n == 1, err +} + +func (s *realServer) ttl(ctx context.Context, key string) (time.Duration, error) { + return s.client.PTTL(ctx, key).Result() +} + +func (s *realServer) seedWithoutTTL(ctx context.Context, key string, hits int64) error { + s.note([]string{key}) + return s.client.Set(ctx, key, hits, 0).Err() +} + +// waitOut really waits: a live server's expiry is the server's own +// clock, and there is nothing here to fast-forward it with. +func (s *realServer) waitOut(d time.Duration) { time.Sleep(d) } + +func (s *realServer) breakable() bool { return false } +func (s *realServer) breakWith(error) {} +func (s *realServer) roundTrips() (int, int, bool) { return 0, 0, false } + +func (s *realServer) cleanup(ctx context.Context) { + keys := make([]string, 0, len(s.touched)) + for k := range s.touched { + keys = append(keys, k) + } + if len(keys) > 0 { + if err := s.client.Del(ctx, keys...).Err(); err != nil { + fail(fmt.Sprintf("cleaning up %d key(s): %v", len(keys), err)) + } + } + if err := s.client.Close(); err != nil { + fail(fmt.Sprintf("closing the redis client: %v", err)) + } +} + +var _ server = (*realServer)(nil) + +// runID keeps one run's counters clear of the last one's, which matters +// against a live server where a key outlives the process that made it. +var runID = fmt.Sprintf("run-%d", time.Now().UnixNano()) + +// callerKey is a key of the shape auth builds — the limiter treats it as +// opaque, so the only thing that matters is that each check gets its own. +func callerKey(scenario string) string { + return scenario + ":" + callerIP + ":" + runID +} + +func mustLimiter(srv server, limit int, window time.Duration) *security.RedisRateLimiter { + rl, err := security.NewRedisRateLimiter(srv, limit, window) + if err != nil { + fail(fmt.Sprintf("constructing a limiter (%d per %v): %v", limit, window, err)) + return nil + } + return rl +} + +func mustPrefixedLimiter(srv server, limit int, window time.Duration, prefix string) *security.RedisRateLimiter { + rl, err := security.NewRedisRateLimiterWithPrefix(srv, limit, window, prefix) + if err != nil { + fail(fmt.Sprintf("constructing a limiter under %q: %v", prefix, err)) + return nil + } + return rl +} + +// engineOver is a whole engine wired to one limiter, with its own +// in-memory stores — a replica, as far as the limiter can tell. Note +// what is NOT set: RateLimitAttempts and RateLimitWindow, which a +// supplied limiter makes irrelevant. +func engineOver(limiter security.RateLimiter) (*cryden.Engine, error) { + return cryden.New(cryden.Config{ + JWTSecret: "smoketest-jwt-secret", + Users: memory.NewUserStore(), + Sessions: memory.NewSessionStore(), + Audit: memory.NewAuditStore(), + RateLimiter: limiter, + }) +} + +func main() { + ctx := context.Background() + + srv, mode := chooseServer(ctx) + fmt.Printf("running against %s\n", mode) + + constructorRejectsNonsense(ctx, srv) + theLimitItself(ctx, srv) + theWindowIsFixed(ctx, srv) + keysAreNamespaced(ctx, srv) + replicasShareOneWindow(ctx, srv) + aCounterWithNoExpiryHeals(ctx, srv) + throughTheEngine(ctx, srv) + twoEnginesShareOneLimit(ctx, srv) + whenRedisIsUnreachable(ctx, srv) + theScriptIsCachedAfterFirstUse(ctx, srv) + + srv.cleanup(ctx) + + fmt.Println() + if failures == 0 { + fmt.Println("ALL CHECKS PASSED") + return + } + fmt.Printf("%d CHECK(S) FAILED\n", failures) + os.Exit(1) +} + +// chooseServer returns the live server at REDIS_ADDR when that is set, +// and the in-process stand-in otherwise. An unreachable REDIS_ADDR is a +// hard stop rather than a silent fall back to the stand-in: someone who +// set it wants the real script checked, and quietly not doing that is +// the one outcome worse than failing. +func chooseServer(ctx context.Context) (server, string) { + addr := os.Getenv("REDIS_ADDR") + if addr == "" { + return newStandIn(), "the in-process stand-in (set REDIS_ADDR to check the Lua against a real server)" + } + srv, err := dialRealServer(ctx, addr) + if err != nil { + fmt.Printf("✗ REDIS_ADDR=%s is set but unreachable: %v\n", addr, err) + os.Exit(1) + } + return srv, "the real Redis at " + addr +} + +func constructorRejectsNonsense(_ context.Context, srv server) { + section("a limiter the host builds itself validates its own arguments") + + _, err := security.NewRedisRateLimiter(nil, 10, time.Minute) + expectSentinel("no client at all is rejected", err, security.ErrNilRedisClient) + + for _, limit := range []int{0, -1} { + _, err := security.NewRedisRateLimiter(srv, limit, time.Minute) + expectSentinel(fmt.Sprintf("a limit of %d is rejected", limit), err, security.ErrInvalidRateLimit) + } + + for _, window := range []time.Duration{0, 500 * time.Microsecond} { + _, err := security.NewRedisRateLimiter(srv, 10, window) + expectSentinel(fmt.Sprintf("a %v window is rejected: PEXPIRE cannot express it", window), err, security.ErrInvalidRateWindow) + } + + if _, err := security.NewRedisRateLimiter(srv, 10, time.Millisecond); err != nil { + fail(fmt.Sprintf("one millisecond is the smallest window PEXPIRE can express, so it should be accepted: %v", err)) + } else { + pass("one millisecond, the smallest window PEXPIRE can express, is accepted") + } +} + +func theLimitItself(ctx context.Context, srv server) { + section("the limit itself, and one key's traffic not touching another's") + + rl := mustLimiter(srv, 3, time.Minute) + key := callerKey("login") + + for i := 1; i <= 3; i++ { + expectAllow(ctx, fmt.Sprintf("call %d of 3 is allowed", i), rl, key, true) + } + expectAllow(ctx, "the 4th call in the same window is denied", rl, key, false) + expectAllow(ctx, "and so is the 5th — denial is not a one-off", rl, key, false) + expectAllow(ctx, "a different key is untouched by any of that", rl, callerKey("login-elsewhere"), true) +} + +func theWindowIsFixed(ctx context.Context, srv server) { + section("the window is fixed, not pushed out by the traffic it is denying") + + const window = 400 * time.Millisecond + rl := mustLimiter(srv, 1, window) + key := callerKey("magic-link") + counter := security.DefaultRedisKeyPrefix + key + + expectAllow(ctx, "the first call is allowed", rl, key, true) + expectWindowArmed(ctx, "and arms a window on the counter it created", srv, counter, window) + for i := 2; i <= 5; i++ { + expectAllow(ctx, fmt.Sprintf("call %d inside that window is denied", i), rl, key, false) + } + + srv.waitOut(window + 100*time.Millisecond) + + expectAllow(ctx, "and the window expired on its original schedule regardless", rl, key, true) +} + +func keysAreNamespaced(ctx context.Context, srv server) { + section("keys are namespaced, so a counter cannot land on the host app's own data") + + rl := mustLimiter(srv, 5, time.Minute) + key := callerKey("signup") + expectAllow(ctx, "a call under the default prefix is allowed", rl, key, true) + expectKey(ctx, "its counter is at cryden:ratelimit:", srv, security.DefaultRedisKeyPrefix+key, true) + expectKey(ctx, "and nothing at all was written at the bare key", srv, key, false) + + bare := mustPrefixedLimiter(srv, 5, time.Minute, "") + bareKey := callerKey("signup-unprefixed") + expectAllow(ctx, "an empty prefix is accepted and means exactly that", bare, bareKey, true) + expectKey(ctx, "the counter is at the caller's key, verbatim", srv, bareKey, true) + + staging := mustPrefixedLimiter(srv, 1, time.Minute, "staging:"+runID+":") + production := mustPrefixedLimiter(srv, 1, time.Minute, "production:"+runID+":") + shared := callerKey("login-two-deployments") + expectAllow(ctx, "staging's one call is allowed", staging, shared, true) + expectAllow(ctx, "production's is too — a prefix is a separate window", production, shared, true) + expectAllow(ctx, "and staging is out on its own count, not the shared one", staging, shared, false) +} + +func replicasShareOneWindow(ctx context.Context, srv server) { + section("two limiters over one Redis share a window — the reason this exists") + + replicaA := mustLimiter(srv, 2, time.Minute) + replicaB := mustLimiter(srv, 2, time.Minute) + key := callerKey("login-shared-window") + + expectAllow(ctx, "replica A takes the window's first call", replicaA, key, true) + expectAllow(ctx, "replica B takes its second", replicaB, key, true) + expectAllow(ctx, "replica A is denied the third: the limit is 2 in total, not 2 each", replicaA, key, false) + expectAllow(ctx, "and replica B is denied too", replicaB, key, false) +} + +func aCounterWithNoExpiryHeals(ctx context.Context, srv server) { + section("a counter left without an expiry heals, instead of locking its key out forever") + + const window = 400 * time.Millisecond + rl := mustLimiter(srv, 5, window) + key := callerKey("login-stuck") + counter := security.DefaultRedisKeyPrefix + key + + check("planting a counter past the limit with no TTL on it", srv.seedWithoutTTL(ctx, counter, 99)) + expectNoWindow(ctx, "the planted counter has no window to expire on", srv, counter) + expectAllow(ctx, "the next call is denied, as a counter past its limit should be", rl, key, false) + expectWindowArmed(ctx, "but that same call armed the window it was missing", srv, counter, window) + + srv.waitOut(window + 100*time.Millisecond) + + expectAllow(ctx, "so one window later the key is usable again", rl, key, true) +} + +func throughTheEngine(ctx context.Context, srv server) { + section("through a real engine: the limiter auth actually consults") + + limiter := mustPrefixedLimiter(srv, 1, time.Minute, "engine:"+runID+":") + if limiter == nil { + return + } + engine, err := engineOver(limiter) + check("an engine wired with Config.RateLimiter and no attempt/window knobs", err) + if engine == nil { + return + } + + _, err = cryden.SignUp(ctx, engine, email, password, callerIP) + check("the first signup from this address goes through", err) + + _, err = cryden.SignUp(ctx, engine, "someone-else@dev.com", password, callerIP) + expectSentinel("the second is rate limited before it reaches the store at all", err, auth.ErrRateLimited) + + _, err = cryden.Login(ctx, engine, email, password, callerIP, "curl/8.6.0") + check("logging in still works: signup and login are counted separately", err) + + _, err = cryden.Login(ctx, engine, email, password, callerIP, "curl/8.6.0") + expectSentinel("a second login for the same account from the same address is limited", err, auth.ErrRateLimited) +} + +func twoEnginesShareOneLimit(ctx context.Context, srv server) { + section("two engines, separate stores, one shared limit") + + prefix := "replicas:" + runID + ":" + replicaA, errA := engineOver(mustPrefixedLimiter(srv, 1, time.Minute, prefix)) + replicaB, errB := engineOver(mustPrefixedLimiter(srv, 1, time.Minute, prefix)) + check("wiring replica A", errA) + check("wiring replica B", errB) + if replicaA == nil || replicaB == nil { + return + } + + _, err := cryden.SignUp(ctx, replicaA, email, password, callerIP) + check("a signup on replica A goes through", err) + + _, err = cryden.SignUp(ctx, replicaB, email, password, callerIP) + expectSentinel("the same address is now limited on replica B, which never saw that request", err, auth.ErrRateLimited) + + // The same shape with the default limiter, to show what is being + // fixed rather than just asserting the fix: a limit of 1 that two + // separate processes each apply to themselves lets 2 through. + inProcessA, errA := engineWithInProcessLimit(1) + inProcessB, errB := engineWithInProcessLimit(1) + check("wiring two engines on the in-process default instead", errors.Join(errA, errB)) + if inProcessA == nil || inProcessB == nil { + return + } + _, err = cryden.SignUp(ctx, inProcessA, email, password, callerIP) + check("their first signup goes through as well", err) + _, err = cryden.SignUp(ctx, inProcessB, email, password, callerIP) + check("and so does the second — each keeps its own count, so a limit of 1 let 2 through", err) +} + +// engineWithInProcessLimit is the default wiring: no Config.RateLimiter, +// so New builds an in-memory limiter from the tuning knobs. +func engineWithInProcessLimit(attempts int) (*cryden.Engine, error) { + return cryden.New(cryden.Config{ + JWTSecret: "smoketest-jwt-secret", + Users: memory.NewUserStore(), + Sessions: memory.NewSessionStore(), + Audit: memory.NewAuditStore(), + RateLimitAttempts: attempts, + RateLimitWindow: time.Minute, + }) +} + +func whenRedisIsUnreachable(ctx context.Context, srv server) { + section("when Redis is unreachable, the engine fails closed") + + if !srv.breakable() { + skip("a login fails closed while Redis is down", + "a live server cannot be taken down from in here — stop it and re-run to see this") + return + } + + limiter := mustPrefixedLimiter(srv, 10, time.Minute, "outage:"+runID+":") + if limiter == nil { + return + } + engine, err := engineOver(limiter) + check("an engine over a limiter that is about to lose its Redis", err) + if engine == nil { + return + } + + _, err = cryden.SignUp(ctx, engine, email, password, callerIP) + check("a signup while Redis is healthy", err) + + down := errors.New("dial tcp 127.0.0.1:6379: connect: connection refused") + srv.breakWith(down) + + allowed, err := limiter.Allow(ctx, callerKey("login-during-outage")) + expectBool("the limiter denies rather than waving the call through", allowed, false) + expectWrapped("and reports the underlying failure to its caller", err, down) + + _, err = cryden.Login(ctx, engine, email, password, callerIP, "curl/8.6.0") + expectFailedButNot("the login fails, and not as an ordinary rate limit", err, auth.ErrRateLimited) + + srv.breakWith(nil) + + _, err = cryden.Login(ctx, engine, email, password, callerIP, "curl/8.6.0") + check("and everything recovers the moment Redis is back", err) +} + +func theScriptIsCachedAfterFirstUse(ctx context.Context, srv server) { + section("the script body is sent once, not on every login") + + if _, _, counted := srv.roundTrips(); !counted { + skip("EVALSHA carries every call after the first", + "only the stand-in counts commands; against a real server use MONITOR or INFO commandstats") + return + } + + // A server that has never seen this script, which is what a freshly + // started Redis looks like. + cold := newStandIn() + rl := mustLimiter(cold, 10, time.Minute) + if rl == nil { + return + } + + expectAllow(ctx, "the first call is allowed", rl, callerKey("login-cold-cache"), true) + evalSha, eval, _ := cold.roundTrips() + expectCounts("it took one EVALSHA, refused with NOSCRIPT, then one EVAL", evalSha, 1, eval, 1) + + expectAllow(ctx, "the second call is allowed too", rl, callerKey("login-cold-cache"), true) + evalSha, eval, _ = cold.roundTrips() + expectCounts("and took only an EVALSHA: the body was not sent again", evalSha, 2, eval, 1) +} + +func section(name string) { + fmt.Printf("\n— %s\n", name) +} + +func expectAllow(ctx context.Context, step string, rl *security.RedisRateLimiter, key string, want bool) { + if rl == nil { + fail(step + ": no limiter to call") + return + } + allowed, err := rl.Allow(ctx, key) + if err != nil { + fail(fmt.Sprintf("%s: unexpected error: %v", step, err)) + return + } + expectBool(step, allowed, want) +} + +func expectBool(step string, got, want bool) { + if got != want { + fail(fmt.Sprintf("%s: got allowed=%t, want %t", step, got, want)) + return + } + pass(step) +} + +func expectKey(ctx context.Context, step string, srv server, key string, want bool) { + got, err := srv.exists(ctx, key) + if err != nil { + fail(fmt.Sprintf("%s: reading key %q: %v", step, key, err)) + return + } + if got != want { + fail(fmt.Sprintf("%s: key %q exists=%t, want %t", step, key, got, want)) + return + } + pass(step) +} + +// expectWindowArmed accepts anything from a hair under the full window +// down to half of it: a live server's PTTL counts down from the moment +// the counter was created, and this is checked a round trip or two later. +func expectWindowArmed(ctx context.Context, step string, srv server, key string, window time.Duration) { + got, err := srv.ttl(ctx, key) + if err != nil { + fail(fmt.Sprintf("%s: reading the TTL of %q: %v", step, key, err)) + return + } + if got <= window/2 || got > window { + fail(fmt.Sprintf("%s: TTL of %q is %v, want something just under %v", step, key, got, window)) + return + } + pass(step) +} + +func expectNoWindow(ctx context.Context, step string, srv server, key string) { + got, err := srv.ttl(ctx, key) + if err != nil { + fail(fmt.Sprintf("%s: reading the TTL of %q: %v", step, key, err)) + return + } + if got >= 0 { + fail(fmt.Sprintf("%s: expected no TTL on %q, got %v", step, key, got)) + return + } + pass(step) +} + +func expectCounts(step string, gotFirst, wantFirst, gotSecond, wantSecond int) { + if gotFirst != wantFirst || gotSecond != wantSecond { + fail(fmt.Sprintf("%s: got %d and %d, want %d and %d", step, gotFirst, gotSecond, wantFirst, wantSecond)) + return + } + pass(step) +} + +func expectSentinel(step string, got, want error) { + if !errors.Is(got, want) { + fail(fmt.Sprintf("%s: got %v, want %v", step, got, want)) + return + } + pass(step) +} + +func expectWrapped(step string, got, want error) { + if got == nil { + fail(step + ": expected an error, got none") + return + } + if !errors.Is(got, want) { + fail(fmt.Sprintf("%s: %v does not unwrap to %v", step, got, want)) + return + } + pass(step) +} + +func expectFailedButNot(step string, got, notWant error) { + if got == nil { + fail(step + ": expected an error, got none") + return + } + if errors.Is(got, notWant) { + fail(fmt.Sprintf("%s: got %v, which is exactly what this must not be", step, got)) + return + } + pass(step) +} + +func check(step string, err error) { + if err != nil { + fail(fmt.Sprintf("%s: unexpected error: %v", step, err)) + return + } + pass(step) +} + +func pass(step string) { + fmt.Println("✓", step) +} + +func fail(msg string) { + failures++ + fmt.Println("✗", msg) +} + +// skip is neither a pass nor a failure: the check could not run in this +// mode at all, and saying so is more honest than a ✓ that checked +// nothing. +func skip(step, why string) { + fmt.Printf("· %s — skipped: %s\n", step, why) +} diff --git a/config.go b/config.go index 0f3faa5..62fe9ee 100644 --- a/config.go +++ b/config.go @@ -108,6 +108,24 @@ type Config struct { LockoutThreshold int // default: 5 failed attempts LockoutDuration time.Duration // default: 15 minutes Logger logger.Logger // default: ConsoleJSONLogger + + // RateLimiter replaces the default limiter, which is an in-process + // security.InMemoryRateLimiter built from RateLimitAttempts and + // RateLimitWindow above. That default is correct for exactly one + // process: run three replicas behind a load balancer and each keeps + // its own counters, so the effective limit is three times what was + // configured. Set this to a shared implementation — + // security.NewRedisRateLimiter(client, attempts, window) — and every + // replica counts against one window. + // + // Injected already constructed, the same as every store, so the + // engine never dials Redis itself or owns its lifecycle. When set, + // RateLimitAttempts and RateLimitWindow are ignored entirely: they + // are the in-memory limiter's constructor arguments, and a limiter + // the host built already carries its own bounds. + // + // Left nil, nothing changes from previous versions. + RateLimiter security.RateLimiter } func (c *Config) validate() error { diff --git a/config_test.go b/config_test.go index 4af1b82..fbe3606 100644 --- a/config_test.go +++ b/config_test.go @@ -2,9 +2,11 @@ package cryden import ( "testing" + "time" "github.com/crydensync/cryden/v2/security" "github.com/crydensync/cryden/v2/store/memory" + "github.com/redis/go-redis/v9" ) func validConfig() Config { diff --git a/docs/development/CURRENT-STATE.md b/docs/development/CURRENT-STATE.md index bcc4d84..0fbb317 100644 --- a/docs/development/CURRENT-STATE.md +++ b/docs/development/CURRENT-STATE.md @@ -1,7 +1,7 @@ # cryden — current state -Last updated: 2026-09-04 (by the session that built -named/fingerprinted sessions). Update this file's date and content every time a session +Last updated: 2026-09-05 (by the session that built the Redis-backed +rate limiter). Update this file's date and content every time a session finishes an item — see `CLAUDE.md`'s end-of-session checklist. ## Tagged releases @@ -24,7 +24,7 @@ If you find a real bug in it while working on something else, fix it on its own small branch and note it in `PROGRESS.md` — don't treat finding it as license to re-audit the rest. -## Tier 2 — Security & Monitoring: IN PROGRESS (3 of 4 done) +## Tier 2 — Security & Monitoring: DONE (4 of 4) ### Item 8 — anomaly detection: DONE, branch `feat/anomaly-detection` @@ -141,11 +141,51 @@ subtests), `security/geolocation_test.go` (2), `session/named_test.go` test: `cmd/smoketest/named-sessions` (42 checks). Manual guide: `docs/testing/named-sessions.md`. -### Item 11: NOT STARTED - -Detailed specs in `NEXT.md`. The design decision recorded for item 8 -below is kept for reference — it is what the shipped code implements. -**Do not re-ask or re-derive it**: +### Item 11 — Redis-backed rate limiter: DONE, branch `feat/redis-rate-limiter` + +A **second real implementation** of the existing `security.RateLimiter`, +not a new interface: the in-memory one keeps its counters in a Go map, +which is correct for exactly one process — three replicas keep three +maps, so a configured limit of 10 lets 30 through. + +Shipped as: `security/redisratelimiter.go` (`RedisRateLimiter`, +`NewRedisRateLimiter`, `NewRedisRateLimiterWithPrefix`, +`DefaultRedisKeyPrefix = "cryden:ratelimit:"`), three new sentinels in +`security/errors.go`, and `Config.RateLimiter` — injected already +constructed, like every store, so the engine never dials Redis nor owns +its lifecycle. `engine.go` falls back to the in-process default only +when that field is nil. Nothing in `auth/` changed or can tell which +implementation it holds. + +Decisions worth not re-deriving (full reasoning in `PROGRESS.md`): +`github.com/redis/go-redis/v9`, injected as its own `redis.Scripter` +interface so Client/ClusterClient/Ring/UniversalClient all work and +`redis.NewScript`'s EVALSHA→EVAL fallback is reused rather than +reimplemented; one Lua script per `Allow` because INCR and PEXPIRE +apart lets two replicas each arm their own window; `PEXPIRE` only when +`INCR` returns 1 (or `PTTL` reports none) so a denied client's own +retries cannot push its window out; exactly one key per call, so +Cluster needs no special case; windows under 1ms rejected rather than +rounded, the single place the two implementations are not +interchangeable. Fail-closed is unchanged and now load-bearing — all +three call sites already propagate a limiter error, so Redis becomes a +hard dependency of SignUp/Login/RequestMagicLink; documented, with a +fail-open wrapper left to the host. + +Tests: `security/redisratelimiter_test.go` (14 funcs over a fake that +models the script), plus 3 in `config_test.go` and 1 in +`new_facade_test.go`. Smoke test: `cmd/smoketest/redis-rate-limiter` +(58 checks over ten scenarios) — runs against an in-process stand-in by +default, and against a real server with `REDIS_ADDR` set, which is the +mode that actually executes the Lua. **No Redis server was reachable in +the build environment**, so the Lua itself is so far verified only +against that stand-in; one `docker run` closes the gap. Manual guide: +`docs/testing/redis-rate-limiter.md`. + +#### Item 8's recorded decisions, kept for reference + +What the shipped anomaly-detection code implements. **Do not re-ask or +re-derive it**: - **Signals to evaluate**: new IP/device (vs. recent successful logins), failed-attempt velocity (per-user and per-IP), and @@ -173,9 +213,6 @@ below is kept for reference — it is what the shipped code implements. `login_attempts` table with three partial indexes — plus `CountTargetsForIP`, added by item 9 above against the same table. -Item 11 (Redis-backed rate limiter) has no prior design decisions -recorded — see `NEXT.md` for the level of detail available, make -reasonable calls on anything unspecified, note them in `PROGRESS.md`. ## Tier 3 — Infrastructure & Extensibility: NOT STARTED @@ -212,6 +249,14 @@ project brief. `config.go`/`engine.go` additions sit directly above theirs, so lifting it onto `main` alone means resolving that adjacency by hand. Unmerged and unpushed. +- `feat/redis-rate-limiter` — item 11, complete, 6 commits, branched + from `feat/named-sessions` at `345b2d7`, the tip of the chain, so this + branch carries items 8, 9, 10 and 11. Item 11 has no functional + dependency on any of them, but it adds a `config.go`/`engine.go` field + in the same region they did, so the same by-hand adjacency applies if + it is lifted onto `main` alone. It is also the only item so far that + adds a **direct third-party dependency** (`go-redis`) to `go.mod`. + Unmerged and unpushed. Nothing else in flight. Each new session picks the top item off `NEXT.md`, creates its own branch, and this section should be updated to diff --git a/docs/development/NEXT.md b/docs/development/NEXT.md index 0665ad6..187bc82 100644 --- a/docs/development/NEXT.md +++ b/docs/development/NEXT.md @@ -12,28 +12,9 @@ patterns and note the assumption in `PROGRESS.md` — don't block on it. --- -## Tier 2 — Security & Monitoring - -### 1. Redis-backed rate limiter (item 11) - -`security.RateLimiter` already exists with one implementation -(in-memory, documented as not safe across multiple instances). This is -a **second real implementation**, not an interface-only integration — -Redis is configured infrastructure the host app wires in explicitly -(a connection string/client), the same category as Postgres, not an -arbitrary third-party internet service like HIBP. Ship a real -`security.RedisRateLimiter` (or wherever you decide it should live — -probably `security/`, matching where the in-memory one lives) using a -real, well-established Go Redis client library. `Config` gets a new -way to select/configure it (follow how `Users`/`Sessions`/etc. stores -are injected as already-constructed instances, not built internally -from a connection string — match that pattern here too). - ---- - ## Tier 3 — Infrastructure & Extensibility -### 2. Argon2id as an additional trusted hasher (item 12) +### 1. Argon2id as an additional trusted hasher (item 12) Second implementation of `security.Hasher`, not a replacement for bcrypt. Real design question: how does the engine know which @@ -44,7 +25,7 @@ dispatching `Compare`, while `Hash` always uses whichever algorithm is currently configured. Build it this way unless you find a strong reason not to; note the reasoning either way. -### 3. Additional storage backend beyond Postgres (item 13) +### 2. Additional storage backend beyond Postgres (item 13) Every `store.X` interface already exists — implement all of them against a second backend (SQLite is the most likely candidate per @@ -55,7 +36,7 @@ specific assumptions baked into existing interface docs/behavior `store/postgres/` implementations lean on these and a different backend will need different real solutions, not just syntax swaps. -### 4. Cloud logger integrations (item 14) +### 3. Cloud logger integrations (item 14) `logger.Logger` already exists with one implementation (console JSON). Decide interface-only-vs-shipped-implementation the same way as @@ -68,7 +49,7 @@ console-JSON-to-stdout is already the universal integration point there's a specific strong reason a direct integration adds real value over "the host app already captures stdout." -### 5. Extensible JWT claims (item 15) +### 4. Extensible JWT claims (item 15) Let host apps attach their own data to access tokens. Read `token/jwt.go`'s current claims struct and `JWTIssuer.Issue` before @@ -79,7 +60,7 @@ signing-method check). Likely shape: `Issue` gains an optional `ClaimsProvider` hook — pick whichever fits the existing `Issue` call sites with the least disruption. -### 6. API keys / machine-to-machine auth (item 16) +### 5. API keys / machine-to-machine auth (item 16) New concept, not a variant of an existing one — no human to prompt, so this sits outside the second-factor system entirely (confirm this @@ -91,7 +72,7 @@ values, not human passwords), and its own facade functions (`GenerateAPIKey`, `RevokeAPIKey`, and something that validates a presented key and returns which user/scope it belongs to). -### 7. Webhooks (item 17) +### 6. Webhooks (item 17) Notify the host app on key events. Same question as everything else that reaches outward: interface-only, zero shipped implementations @@ -103,7 +84,7 @@ subset, not all of them) and wire it in wherever `audit.Record` is already called for those events — don't build a second parallel event bus. -### 8. Custom email templates (item 18) +### 7. Custom email templates (item 18) Check `notify.EmailSender`/`notify.MagicLinkSender` as they exist today first — there's a real chance this needs **no engine change at @@ -121,19 +102,19 @@ than building something speculative to have built something. automatic action — no auto-lock, no auto-config-change, nothing. Every one of these produces information for a human to act on. -### 9. Weekly digest (item 19) +### 8. Weekly digest (item 19) Reads `AuditStore`, summarizes in plain English, returns text. Nothing else. -### 10. Support-ticket assistant (item 20) +### 9. Support-ticket assistant (item 20) Read-only diagnosis ("why can't user X log in") — queries `AuditStore`/`UserStore`/session state, produces an explanation, never touches anything. -### 11. Config tuning advisor (item 21) +### 10. Config tuning advisor (item 21) Produces a report of suggested config changes. Never applies them. -### 12. Ask-AI widget (item 22) +### 11. Ask-AI widget (item 22) The most complex of the four. Needs its own full design pass before any code — at minimum: an LLM provider interface (zero shipped implementations, host brings their own key/provider, same pattern as diff --git a/docs/development/PROGRESS.md b/docs/development/PROGRESS.md index 502af93..68cf540 100644 --- a/docs/development/PROGRESS.md +++ b/docs/development/PROGRESS.md @@ -252,3 +252,78 @@ items 8 and 9 did not recur this session. Still unfixed, still worth its own small branch. Next in queue: item 11, the Redis-backed rate limiter. + +## 2026-09-05 — Redis-backed rate limiter (item 11) + +Branch: `feat/redis-rate-limiter` (6 commits, unmerged, unpushed, +branched from `feat/named-sessions` at `345b2d7` — the tip of the chain, +so this branch carries items 8, 9, 10 and 11). + +Built: `security/RedisRateLimiter`, a second real implementation of the +existing `security.RateLimiter`, so counters live in Redis instead of a +per-process Go map. `security/redisratelimiter.go` holds the type, two +constructors and the Lua; `security/errors.go` gains three sentinels; +`Config.RateLimiter` accepts an already-constructed limiter and +`engine.go` falls back to the in-process default only when it is nil. +Nothing in `auth/` changed — every call site already held the interface. + +Assumptions and calls made, none of which `NEXT.md` specified: + +- **`github.com/redis/go-redis/v9`**, and the injected type is that + library's own `redis.Scripter` rather than `*redis.Client` or a bespoke + narrow interface. Client, ClusterClient, Ring and UniversalClient all + satisfy it, `redis.NewScript`'s EVALSHA→EVAL fallback comes along for + free instead of being reimplemented, and a fake stays writable via + `redis.NewCmdResult`/`redis.ErrNoScript`. This is the engine's first + direct third-party dependency of this kind — justified on `NEXT.md`'s + own terms: Redis is configured infrastructure, the same category as + Postgres and `lib/pq`, not an internet service like HIBP. +- **One Lua script per `Allow`.** INCR and PEXPIRE as two round trips + lets two replicas each arm their own window over one key, which is + precisely the bug this item exists to fix. +- **`PEXPIRE` only when `INCR` returns 1** (or when `PTTL` reports no + expiry, which self-heals a counter left without one). Arming it on + every call is the more obvious idiom and is wrong: it turns a blocked + client's own retries into a permanent block, and it would also break + parity with the in-memory limiter's fixed window. +- **Fixed-window parity is deliberate.** Same allow/deny arithmetic as + `InMemoryRateLimiter` (calls 1..limit pass, limit+1 denied, window + never extended) so the two are interchangeable. A sliding window would + be a different feature with a different cost, not an improvement + smuggled into this one. +- **Two positional constructors** (`NewRedisRateLimiter` and + `...WithPrefix`) over functional options, matching every other + constructor in the repo. Default prefix `cryden:ratelimit:` so a + counter can never collide with a host app's own keys; an empty prefix + is legal and means raw keys. +- **Windows under 1ms are rejected, not rounded.** `PEXPIRE` cannot + express them. This is the one place the two implementations are not + interchangeable, and it is documented as such rather than papered over. +- **Fail-closed left as it was.** All three call sites already propagate + a limiter error, so wiring Redis makes it a hard dependency of SignUp, + Login and RequestMagicLink. Changing caller behaviour was out of scope + for this item; instead the trade-off is documented, along with the + fail-open wrapper a host can write against the interface. The error is + wrapped, never `ErrRateLimited`, so callers can still tell "limit hit" + from "limiter broken". +- **Exactly one key per call**, so Redis Cluster needs no special + handling and the script never spans hash slots. +- Replaced a stale comment in `go.mod` that claimed the module proxy was + unreachable; it is, and go-redis is now a direct require. + +Verification: `gofmt -l .` clean, `go build ./...`, `go vet ./...` and +`go test ./...` all clean, `go test -race` clean, and the smoke test +passes all 58 checks over ten scenarios. + +**The one real gap: no Redis server was reachable here** (no daemon, +`docker info` unavailable), so the Lua was executed only against a +stand-in that models its semantics, never by Redis itself. The Go side — +allow/deny arithmetic, the fixed window, prefixing, the EVALSHA→EVAL +fallback, fail-closed propagation through the engine — is genuinely +tested; the script's own behaviour on a real server is not. Rather than +claim otherwise, the smoke test takes `REDIS_ADDR` and runs every +scenario against a live server, namespacing and cleaning up its own +keys, and `docs/testing/redis-rate-limiter.md` opens with the two +commands that close the gap. Worth doing before this branch is merged. + +Next in queue: item 12, Argon2id as an additional trusted hasher. diff --git a/docs/testing/redis-rate-limiter.md b/docs/testing/redis-rate-limiter.md new file mode 100644 index 0000000..1c8608b --- /dev/null +++ b/docs/testing/redis-rate-limiter.md @@ -0,0 +1,255 @@ +# Manual test guide — Redis-backed rate limiter + +The rate limiter that ships by default keeps its counters in a Go map. +That is correct for exactly one process. Run three replicas behind a +load balancer and each keeps its own map, so a configured limit of 10 +lets 30 through — and an attacker doesn't have to know that to benefit +from it, the load balancer spreads their attempts for them. + +`security.RedisRateLimiter` moves the counters to Redis so every replica +counts against one window. It is a **real second implementation of the +same `security.RateLimiter` interface**, not a new hook: `Allow` is the +whole surface, and nothing in `auth/` can tell which one it holds. + +The fastest full check is the smoke test: + +``` +go run ./cmd/smoketest/redis-rate-limiter +``` + +58 checks over ten scenarios, no Redis server and no database required — +it runs against an in-process stand-in that models what Redis does with +the script. + +That stand-in is a model, so it proves the Go side and **not Redis's own +execution of the Lua**. To close that gap, point the same smoke test at +a real server: + +``` +docker run --rm -d -p 6379:6379 --name cryden-redis redis:7-alpine +REDIS_ADDR=127.0.0.1:6379 go run ./cmd/smoketest/redis-rate-limiter +docker rm -f cryden-redis +``` + +Identical output, except the two scenarios that need to break Redis or +count round trips from the inside, which report as skipped. An +unreachable `REDIS_ADDR` is a hard stop, never a silent fallback to the +stand-in — a green run is always a green run of the mode you asked for. +Keys are namespaced per run and deleted afterwards, so it is safe +against a Redis you also use for something else. + +## Setup + +The client is injected already constructed, the same as every store — +the engine never dials Redis, never reconnects it and never closes it: + +```go +rdb := redis.NewClient(&redis.Options{ + Addr: os.Getenv("REDIS_ADDR"), + Password: os.Getenv("REDIS_PASSWORD"), +}) +defer rdb.Close() + +limiter, err := security.NewRedisRateLimiter(rdb, 10, time.Minute) +if err != nil { + log.Fatal(err) +} + +engine, err := cryden.New(cryden.Config{ + JWTSecret: os.Getenv("CRYDEN_JWT_SECRET"), + Users: users, + Sessions: sessions, + Audit: audit, + RateLimiter: limiter, +}) +``` + +`RateLimitAttempts` and `RateLimitWindow` are ignored once +`RateLimiter` is set — they are the in-memory limiter's constructor +arguments, and a limiter you built already carries its own bounds. Leave +`RateLimiter` nil and nothing changes from previous versions. + +`*redis.Client`, `*redis.ClusterClient`, `*redis.Ring` and +`redis.UniversalClient` are all accepted: the parameter is +`redis.Scripter`, go-redis's own interface, so a fake is easy to write +and Cluster needs no special case (one `Allow` touches exactly one key, +so the script never spans slots). + +Constructor arguments are validated, because there is no +`Config.applyDefaults` between you and it: + +| Argument | Rejected with | +|---|---| +| `nil` client | `security.ErrNilRedisClient` | +| `limit <= 0` | `security.ErrInvalidRateLimit` | +| `window < time.Millisecond` | `security.ErrInvalidRateWindow` | + +The millisecond floor is the **one** place the two implementations are +not interchangeable: `PEXPIRE` cannot express a shorter window, and +rounding a 500µs window up to 1ms silently would be worse than saying +so. + +## 1. The limit itself + +With a limit of 3 and a one-minute window, call `Allow` five times with +the same key: + +``` +true true true false false +``` + +Calls 1–3 pass, 4 and 5 are denied. A different key is unaffected — +counters are per key, and keys are opaque strings the caller builds. + +## 2. The window is fixed, not sliding + +Take the limit, then keep hammering. When the window ends, the counter +resets **on its original schedule** — the denied calls do not push it +out. + +This is worth checking deliberately, because the obvious +implementation gets it wrong. Arming the expiry on every call (rather +than only on the call that creates the counter) turns a blocked +client's own retries into a permanently blocked key: every retry +extends the window it is waiting on. The Lua arms `PEXPIRE` when +`INCR` returns 1, and otherwise only when `PTTL` reports no expiry at +all. + +## 3. Two replicas share one window + +The reason the feature exists. Build **two** limiters over the same +Redis, limit 2, and alternate: + +``` +A.Allow(key) -> true +B.Allow(key) -> true +A.Allow(key) -> false +B.Allow(key) -> false +``` + +Two in total, not two each. Do the same with two engines on the +in-process default and a limit of 1, and both let a signup through — +that contrast is the bug this fixes. + +## 4. Through the engine + +Nothing about the call sites changed. With a limit of 1: + +```go +cryden.SignUp(ctx, engine, "raymondproguy@dev.com", "Tr0ubl3-Fr33!2026", "1.2.3.4") // ok +cryden.SignUp(ctx, engine, "raymondproguy@dev.com", "Tr0ubl3-Fr33!2026", "1.2.3.4") // auth.ErrRateLimited +``` + +The second returns `auth.ErrRateLimited` **before touching the user +store**. Logging in still works right after, because the three call +sites use different keys: + +| Call | Key | +|---|---| +| `SignUp` | `signup:` | +| `Login` | `login::` | +| `RequestMagicLink` | `magic-link::` | + +Those keys get the prefix `cryden:ratelimit:` in Redis, so +`cryden:ratelimit:signup:1.2.3.4`. Check with `redis-cli --scan --pattern +'cryden:ratelimit:*'` while attempts are in flight, and `PTTL` on one of +them to watch the window count down. + +## 5. Keys are namespaced + +The prefix exists so a counter can never land on a key your app already +owns. `security.NewRedisRateLimiterWithPrefix` overrides it — useful to +separate staging from production on one Redis, since a different prefix +is a different window: + +```go +security.NewRedisRateLimiterWithPrefix(rdb, 10, time.Minute, "myapp:staging:rl:") +``` + +An empty prefix is legal and means raw, unprefixed keys. Verify that +what you expect is what exists: with the default prefix, `EXISTS +signup:1.2.3.4` must be 0 and `EXISTS cryden:ratelimit:signup:1.2.3.4` +must be 1. + +## 6. Redis going down blocks logins — on purpose + +**The important negative case, and the one thing to decide before +deploying this.** Stop Redis and try to log in: + +``` +docker stop cryden-redis +``` + +The login fails with the wrapped Redis error — not +`auth.ErrRateLimited`, so a caller branching on that sentinel still +tells the two apart: + +``` +security: redis rate limiter: dial tcp 127.0.0.1:6379: connect: connection refused +``` + +`Allow` returns `(false, err)`, and all three call sites propagate it. +So wiring Redis in makes it a **hard dependency of SignUp, Login and +RequestMagicLink**. That is the deliberate choice — failing open on a +limiter outage means an unlimited credential-stuffing window that +starts exactly when your monitoring is already busy — but it is a +choice, and the availability cost is real. + +To trade it the other way, wrap the limiter in your own type. The +engine holds an interface, so this needs no engine change: + +```go +type failOpen struct{ inner security.RateLimiter } + +func (f failOpen) Allow(ctx context.Context, key string) (bool, error) { + allowed, err := f.inner.Allow(ctx, key) + if err != nil { + // Prefer serving traffic to enforcing the limit. Alert on this. + return true, nil + } + return allowed, nil +} +``` + +Start Redis again and confirm logins recover immediately — the limiter +holds no broken state, it only asks. + +## 7. The script body is sent once + +`redis.NewScript` + `Script.Run` means EVALSHA first, EVAL only on +`NOSCRIPT`. So the ~200-byte body crosses the wire once per Redis script +cache, not once per login. Check with `redis-cli INFO commandstats` +after a few hundred attempts: `cmdstat_evalsha` should climb while +`cmdstat_eval` stays at 1. A `SCRIPT FLUSH` mid-traffic must not cause +errors — the next call reloads and retries. + +## Postgres + +Nothing to run. This feature adds **no table, no column and no +migration** — the counters live in Redis and nowhere else. + +## Known limits + +- **Fixed window, not sliding.** A client can spend its whole budget at + the end of one window and again at the start of the next, so a burst + of up to 2×limit is reachable across a window boundary. This matches + the in-memory limiter exactly, which is the point: the two are + interchangeable, and a sliding window would be a different feature + with a different cost. +- **One round trip per attempt.** Every `SignUp`/`Login`/ + `RequestMagicLink` now waits on Redis. Keep it close to the app; a + cross-region Redis puts its latency in front of every login. +- **Counters are not durable and should not be.** A Redis restart + forgets every window. That is acceptable for rate limiting and is why + no persistence is configured or required here — do not reach for + `AOF` on this account. +- **Redis becomes a dependency of the login path.** See §6. Decide + fail-closed versus fail-open deliberately rather than discovering the + default during an incident. +- **The engine never closes the client.** It didn't open it. Lifecycle, + pooling, TLS, auth and retries are all yours, configured on the + client you pass in. +- **No per-key overrides.** One limit and one window apply to every key + a limiter serves. Different budgets for signup and login means two + limiters and, since the engine holds one, a small dispatching + `RateLimiter` of your own that picks between them on the key's prefix. diff --git a/engine.go b/engine.go index 5e3113d..d59beaf 100644 --- a/engine.go +++ b/engine.go @@ -99,6 +99,16 @@ func New(cfg Config) (*Engine, error) { } } + // A host-supplied limiter wins outright; the in-process default is + // built from the two tuning knobs only when none was given. Which + // one is in play is not a detail the rest of the engine can see — + // every call site holds the security.RateLimiter interface, so + // swapping the counter's home never reaches auth/. + rateLimiter := cfg.RateLimiter + if rateLimiter == nil { + rateLimiter = security.NewInMemoryRateLimiter(cfg.RateLimitAttempts, cfg.RateLimitWindow) + } + return &Engine{ users: cfg.Users, sessions: cfg.Sessions, @@ -115,7 +125,7 @@ func New(cfg Config) (*Engine, error) { passwordPolicy: cfg.PasswordPolicy, hasher: hasher, ids: security.NewUUIDv7Generator(), - rateLimiter: security.NewInMemoryRateLimiter(cfg.RateLimitAttempts, cfg.RateLimitWindow), + rateLimiter: rateLimiter, refreshGen: refreshGen, jwtIssuer: jwtIssuer, pendingIssuer: pendingIssuer, diff --git a/go.mod b/go.mod index 55a84b2..c9fed24 100644 --- a/go.mod +++ b/go.mod @@ -9,10 +9,12 @@ require ( github.com/google/uuid v1.6.0 github.com/lib/pq v1.12.3 github.com/pquerna/otp v1.5.0 + github.com/redis/go-redis/v9 v9.22.0 golang.org/x/crypto v0.55.0 ) require ( + github.com/cespare/xxhash/v2 v2.3.0 // indirect github.com/fxamacker/cbor/v2 v2.9.3 // indirect github.com/go-viper/mapstructure/v2 v2.5.0 // indirect github.com/go-webauthn/x v0.3.0 // indirect @@ -20,6 +22,7 @@ require ( github.com/philhofer/fwd v1.2.0 // indirect github.com/tinylib/msgp v1.6.4 // indirect github.com/x448/float16 v0.8.4 // indirect + go.uber.org/atomic v1.11.0 // indirect golang.org/x/sys v0.47.0 // indirect ) @@ -30,11 +33,10 @@ require ( // build the otp package itself. require github.com/boombuler/barcode v1.0.1-0.20190219062509-6c824513bacc // indirect -// github.com/go-webauthn/webauthn's own transitive dependencies -// (fxamacker/cbor, go-webauthn/x, tinylib/msgp, etc.) are not listed -// here individually — run 'go mod tidy' after pulling this branch to -// resolve and pin them; no network access to the Go module proxy was -// available while authoring this change to do it here. +// github.com/redis/go-redis/v9 backs security.RedisRateLimiter, the +// distributed RateLimiter implementation. It is a direct dependency of +// the engine itself rather than of tests only: Redis is infrastructure +// the host app operates, in the same category as Postgres and lib/pq. // // github.com/descope/virtualwebauthn is a real dependency, but only // ever imported from _test.go files and cmd/smoketest/webauthn-passkeys diff --git a/go.sum b/go.sum index 0224791..3628090 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,11 @@ github.com/boombuler/barcode v1.0.1-0.20190219062509-6c824513bacc h1:biVzkmvwrH8WK8raXaxBx6fRVTlJILwEwQGL1I/ByEI= github.com/boombuler/barcode v1.0.1-0.20190219062509-6c824513bacc/go.mod h1:paBWMcWSl3LHKBqUq+rly7CNSldXjb2rDl3JlRe0mD8= +github.com/bsm/ginkgo/v2 v2.12.0 h1:Ny8MWAHyOepLGlLKYmXG4IEkioBysk6GpaRTLC8zwWs= +github.com/bsm/ginkgo/v2 v2.12.0/go.mod h1:SwYbGRRDovPVboqFv0tPTcG1sN61LM1Z4ARdbAV9g4c= +github.com/bsm/gomega v1.27.10 h1:yeMWxP2pV2fG3FgAODIY8EiRE3dy0aeFYt4l7wh6yKA= +github.com/bsm/gomega v1.27.10/go.mod h1:JyEr/xRbxbtgWNi8tIEVPUYZ5Dzef52k01W3YH0H+O0= +github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= +github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/descope/virtualwebauthn v1.0.5 h1:fMXji5UMepJC51Ge6d4v5IAjiJQRKmXE9hlo/B9SczQ= github.com/descope/virtualwebauthn v1.0.5/go.mod h1:lLCfN+DpCM3iisM4bCILZlFEWkC1Zo7ZgsxC45CUapI= @@ -19,6 +25,8 @@ github.com/google/go-tpm-tools v0.3.13-0.20230620182252-4639ecce2aba h1:qJEJcuLz github.com/google/go-tpm-tools v0.3.13-0.20230620182252-4639ecce2aba/go.mod h1:EFYHy8/1y2KfgTAsx7Luu7NGhoxtuVHnNo8jE7FikKc= github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= +github.com/klauspost/cpuid/v2 v2.2.10 h1:tBs3QSyvjDyFTq3uoc/9xFpCuOsJQFNPiAhYdw2skhE= +github.com/klauspost/cpuid/v2 v2.2.10/go.mod h1:hqwkgyIinND0mEev00jJYCxPNVRVXFQeu1XKlok6oO0= github.com/lib/pq v1.12.3 h1:tTWxr2YLKwIvK90ZXEw8GP7UFHtcbTtty8zsI+YjrfQ= github.com/lib/pq v1.12.3/go.mod h1:/p+8NSbOcwzAEI7wiMXFlgydTwcgTr3OSKMsD2BitpA= github.com/philhofer/fwd v1.2.0 h1:e6DnBTl7vGY+Gz322/ASL4Gyp1FspeMvx1RNDoToZuM= @@ -26,6 +34,8 @@ github.com/philhofer/fwd v1.2.0/go.mod h1:RqIHx9QI14HlwKwm98g9Re5prTQ6LdeRQn+gXJ github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/pquerna/otp v1.5.0 h1:NMMR+WrmaqXU4EzdGJEE1aUUI0AMRzsp96fFFWNPwxs= github.com/pquerna/otp v1.5.0/go.mod h1:dkJfzwRKNiegxyNb54X/3fLwhCynbMspSyWKnvi1AEg= +github.com/redis/go-redis/v9 v9.22.0 h1:laDvpYXTJtZLloinw1fA5Kqd6HAEH2XKxOkG/PDq2F0= +github.com/redis/go-redis/v9 v9.22.0/go.mod h1:y2g0Wj8rQvuK0ELM+oxSudcLtC09JScs98I/X9gRWY4= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.12.1 h1:EuwCh5fleGS7H32xRwO3wRGT7DxrDhLAT6FF8MpWDWE= @@ -34,6 +44,10 @@ github.com/tinylib/msgp v1.6.4 h1:mOwYbyYDLPj35mkA2BjjYejgJk9BuHxDdvRnb6v2ZcQ= github.com/tinylib/msgp v1.6.4/go.mod h1:RSp0LW9oSxFut3KzESt5Voq4GVWyS+PSulT77roAqEA= github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM= github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg= +github.com/zeebo/xxh3 v1.1.0 h1:s7DLGDK45Dyfg7++yxI0khrfwq9661w9EN78eP/UZVs= +github.com/zeebo/xxh3 v1.1.0/go.mod h1:IisAie1LELR4xhVinxWS5+zf1lA4p0MW4T+w+W07F5s= +go.uber.org/atomic v1.11.0 h1:ZvwS0R+56ePWxUNi+Atn9dWONBPp/AUETXlHW0DxSjE= +go.uber.org/atomic v1.11.0/go.mod h1:LUxbIzbOniOlMKjJjyPfpl4v+PKK2cNJn91OQbhoJI0= go.uber.org/mock v0.6.0 h1:hyF9dfmbgIX5EfOdasqLsWD6xqpNZlXblLB/Dbnwv3Y= go.uber.org/mock v0.6.0/go.mod h1:KiVJ4BqZJaMj4svdfmHM0AUx4NJYO8ZNpPnZn1Z+BBU= go.yaml.in/yaml/v3 v3.0.5 h1:N6y/pJk8buWs9NY5ERU2HSMfm+IuD/OtfdAnq6kESPw= diff --git a/new_facade_test.go b/new_facade_test.go index ead0f0e..5b015e9 100644 --- a/new_facade_test.go +++ b/new_facade_test.go @@ -5,6 +5,7 @@ import ( "errors" "testing" + "github.com/crydensync/cryden/v2/auth" "github.com/crydensync/cryden/v2/security" "github.com/crydensync/cryden/v2/store" "github.com/crydensync/cryden/v2/store/memory" @@ -234,3 +235,40 @@ func TestListNamedSessions_SurvivesAFailingGeolocator(t *testing.T) { t.Errorf("expected a device-only label, got %+v", list) } } + +// stubRateLimiter is the shape a host app supplies when it replaces the +// default limiter: something the engine only ever reaches through the +// security.RateLimiter interface, with no idea where the count lives. +type stubRateLimiter struct { + allow bool + keys []string +} + +func (s *stubRateLimiter) Allow(_ context.Context, key string) (bool, error) { + s.keys = append(s.keys, key) + return s.allow, nil +} + +var _ security.RateLimiter = (*stubRateLimiter)(nil) + +// Wiring a limiter into Config has to reach the real call path, not just +// the Engine struct — this asserts the injected limiter is the one +// SignUp consults, and that it is handed the key auth builds rather than +// something the facade invented. +func TestSignUp_UsesTheConfiguredRateLimiter(t *testing.T) { + limiter := &stubRateLimiter{allow: false} + cfg := validConfig() + cfg.RateLimiter = limiter + engine, err := New(cfg) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + _, err = SignUp(context.Background(), engine, "raymondproguy@dev.com", "Tr0ubl3-Fr33!2026", "1.2.3.4") + if err != auth.ErrRateLimited { + t.Fatalf("expected auth.ErrRateLimited from a limiter that denies, got %v", err) + } + if len(limiter.keys) != 1 || limiter.keys[0] != "signup:1.2.3.4" { + t.Errorf("expected one call keyed \"signup:1.2.3.4\", got %v", limiter.keys) + } +} diff --git a/security/errors.go b/security/errors.go index b96eb90..484709d 100644 --- a/security/errors.go +++ b/security/errors.go @@ -4,4 +4,12 @@ import "errors" var ( ErrInvalidBcryptCost = errors.New("security: bcrypt cost out of valid range") + + // Reported by NewRedisRateLimiter / NewRedisRateLimiterWithPrefix, + // which validate their arguments because a host constructs that + // limiter directly — there is no Config.applyDefaults between the + // caller and it to turn a zero value into something sane. + ErrNilRedisClient = errors.New("security: redis rate limiter requires a redis client") + ErrInvalidRateLimit = errors.New("security: rate limit must allow at least one call per window") + ErrInvalidRateWindow = errors.New("security: rate limit window must be at least one millisecond") ) diff --git a/security/redisratelimiter.go b/security/redisratelimiter.go new file mode 100644 index 0000000..0b4d6d7 --- /dev/null +++ b/security/redisratelimiter.go @@ -0,0 +1,138 @@ +package security + +import ( + "context" + "fmt" + "time" + + "github.com/redis/go-redis/v9" +) + +// DefaultRedisKeyPrefix namespaces every key RedisRateLimiter touches. +// Redis is normally shared with whatever else the host app keeps there, +// and the keys this limiter is handed are opaque caller-built strings +// (auth passes "login:"+ip+":"+email, "signup:"+ip, and similar) — with +// no prefix, a counter and an unrelated application key could collide +// on a name neither side chose deliberately. +const DefaultRedisKeyPrefix = "cryden:ratelimit:" + +// redisFixedWindowLimit is the entire limiter: one INCR that creates the +// counter, one PEXPIRE that closes the window it belongs to, and one +// comparison against the limit. It is a script because those steps have +// to be a single atomic operation — the whole point of counting in Redis +// is that several engine instances share one counter, and a plain +// INCR-then-PEXPIRE issued from Go lets two of them interleave: both +// INCR to 1, both arm an expiry, and one window quietly becomes two. +// +// The PTTL branch covers a counter that exists with no expiry at all — +// a key set by hand, or one left behind by a process that died between +// the INCR and the PEXPIRE of some older non-atomic implementation. +// Without it that key would count up forever and deny its caller +// permanently; with it, the next call re-arms the window and the +// counter heals itself. +var redisFixedWindowLimit = redis.NewScript(` +local hits = redis.call("INCR", KEYS[1]) +if hits == 1 or redis.call("PTTL", KEYS[1]) < 0 then + redis.call("PEXPIRE", KEYS[1], ARGV[1]) +end +if hits > tonumber(ARGV[2]) then + return 0 +end +return 1 +`) + +// RedisRateLimiter is the distributed RateLimiter implementation: the +// same fixed-window-counter-per-key policy InMemoryRateLimiter applies, +// with the counter kept in Redis so every engine instance pointed at +// that Redis shares one window. Swapping one for the other changes where +// the count lives, not what counts as over the limit. +// +// Unlike BreachedPasswordChecker or IPGeolocator — interfaces the engine +// deliberately ships no implementation of, because using them means +// calling somebody else's internet service — Redis is infrastructure the +// host app configures and operates, the same category as Postgres. So +// this is a real implementation, and like every store it takes an +// already-constructed client rather than a connection string it would +// dial itself. +// +// Operationally, wiring this makes Redis a hard dependency of the three +// entry points that rate-limit: SignUp, Login and RequestMagicLink each +// propagate a limiter error to their caller rather than guessing, so +// while Redis is unreachable those calls fail instead of running +// unlimited. Failing closed is the safe direction and it is deliberate, +// but it is a real availability trade-off, and a host that would rather +// stay open can wrap this in its own RateLimiter that swallows the error +// and returns true — that is a decision only the host can make. +// +// Works with a *redis.Client, *redis.ClusterClient or *redis.Ring: every +// Allow touches exactly one key, so there is no multi-slot script for +// Cluster to reject. The client stays owned by the host app; this type +// never closes it. +type RedisRateLimiter struct { + client redis.Scripter + limit int + window time.Duration + keyPrefix string +} + +var _ RateLimiter = (*RedisRateLimiter)(nil) + +// NewRedisRateLimiter constructs a limiter allowing `limit` calls per +// `window` per key, counted in the Redis that `client` talks to, under +// DefaultRedisKeyPrefix. Like NewInMemoryRateLimiter, both bounds are +// the caller's to set — there are no hidden defaults here — but unlike +// it, they are validated, because a limiter a host constructs itself has +// no Config.applyDefaults upstream of it to fill in a zero value. +func NewRedisRateLimiter(client redis.Scripter, limit int, window time.Duration) (*RedisRateLimiter, error) { + return NewRedisRateLimiterWithPrefix(client, limit, window, DefaultRedisKeyPrefix) +} + +// NewRedisRateLimiterWithPrefix is NewRedisRateLimiter with the key +// namespace chosen explicitly, for two deployments that share one Redis +// database and must not share counters — staging alongside production, +// or one namespace per tenant. Passing "" is allowed and means exactly +// what it says: raw caller-supplied keys, no prefix, appropriate when +// the database belongs to this engine alone. +func NewRedisRateLimiterWithPrefix(client redis.Scripter, limit int, window time.Duration, keyPrefix string) (*RedisRateLimiter, error) { + if client == nil { + return nil, ErrNilRedisClient + } + if limit <= 0 { + return nil, ErrInvalidRateLimit + } + // PEXPIRE's unit is the floor: a window that rounds down to zero + // milliseconds makes Redis reject the expiry outright rather than + // approximate it, so the counter would never expire. This is the one + // place the two implementations are not interchangeable — + // InMemoryRateLimiter compares deadlines and has no such floor. + if window < time.Millisecond { + return nil, ErrInvalidRateWindow + } + return &RedisRateLimiter{ + client: client, + limit: limit, + window: window, + keyPrefix: keyPrefix, + }, nil +} + +// Allow counts this call against `key`'s current window and reports +// whether it is within the limit. One round trip (EVALSHA), plus one +// extra the first time this process meets a Redis that has not cached +// the script yet, which go-redis retries as a full EVAL by itself. +func (r *RedisRateLimiter) Allow(ctx context.Context, key string) (bool, error) { + verdict, err := redisFixedWindowLimit.Run( + ctx, + r.client, + []string{r.keyPrefix + key}, + r.window.Milliseconds(), + r.limit, + ).Int64() + if err != nil { + // Deny on error, never "allowed, but also here is an error": a + // caller that read the bool and ignored err would otherwise run + // completely unlimited for as long as Redis stayed down. + return false, fmt.Errorf("security: redis rate limiter: %w", err) + } + return verdict == 1, nil +} diff --git a/security/redisratelimiter_test.go b/security/redisratelimiter_test.go new file mode 100644 index 0000000..53debd7 --- /dev/null +++ b/security/redisratelimiter_test.go @@ -0,0 +1,465 @@ +package security + +import ( + "context" + "errors" + "sync" + "testing" + "time" + + "github.com/redis/go-redis/v9" +) + +// fakeRedis stands in for a Redis server: the four things +// redisFixedWindowLimit actually depends on (INCR, PTTL, PEXPIRE and +// lazy expiry of a key whose TTL has passed), plus the +// EVALSHA-NOSCRIPT-EVAL handshake go-redis performs against a server +// that has not cached the script yet. +// +// What this proves and what it cannot: it exercises every decision made +// on the Go side — key namespacing, argument order and units, the limit +// comparison, error handling, the script-cache fallback, and that two +// limiters over one server share a counter — against the semantics the +// Lua is written against. It does not run the Lua, so it cannot prove +// the script is valid Lua or that a real server's INCR/PTTL/PEXPIRE +// behave as modelled below. docs/testing/redis-rate-limiter.md covers +// that half, against a real server. +type fakeRedis struct { + mu sync.Mutex + counts map[string]int64 + expiry map[string]time.Time + // now is a frozen clock, so a window can roll over without the test + // sleeping through it. + now time.Time + // cached is whether the script has been loaded, i.e. whether EVALSHA + // works yet. A real server starts out with an empty script cache. + cached bool + // err, when set, fails every command — a Redis that is unreachable, + // out of memory, or refusing writes. + err error + + evalSha, eval int + lastKeys []string + + // t reports an argument the limiter should never have sent; see + // argInt64. + t *testing.T +} + +func newFakeRedis(t *testing.T) *fakeRedis { + return &fakeRedis{ + t: t, + counts: map[string]int64{}, + expiry: map[string]time.Time{}, + now: time.Date(2026, 9, 4, 12, 0, 0, 0, time.UTC), + } +} + +func (f *fakeRedis) advance(d time.Duration) { + f.mu.Lock() + defer f.mu.Unlock() + f.now = f.now.Add(d) +} + +// script applies redisFixedWindowLimit's logic. Kept deliberately +// line-for-line with the Lua so a change to one is obvious in review of +// the other. +func (f *fakeRedis) script(keys []string, args []any) int64 { + f.t.Helper() + f.mu.Lock() + defer f.mu.Unlock() + + key := keys[0] + f.lastKeys = keys + windowMS := f.argInt64(args[0]) + limit := f.argInt64(args[1]) + + // A real server drops an expired key before any command reads it. + if exp, ok := f.expiry[key]; ok && !f.now.Before(exp) { + delete(f.counts, key) + delete(f.expiry, key) + } + + f.counts[key]++ + hits := f.counts[key] + if hits == 1 || f.pttl(key) < 0 { + f.expiry[key] = f.now.Add(time.Duration(windowMS) * time.Millisecond) + } + if hits > limit { + return 0 + } + return 1 +} + +// pttl is PTTL for a key already known to exist: milliseconds left, or +// -1 when no expiry was ever set on it. +func (f *fakeRedis) pttl(key string) int64 { + exp, ok := f.expiry[key] + if !ok { + return -1 + } + return exp.Sub(f.now).Milliseconds() +} + +// argInt64 doubles as an assertion about what the limiter sends: Redis +// takes its arguments as strings and Lua's tonumber would quietly +// accept a float, so a window arriving as a time.Duration (nanoseconds) +// rather than a count of milliseconds has to fail here, loudly, instead +// of producing a window a billion times too long. +func (f *fakeRedis) argInt64(arg any) int64 { + f.t.Helper() + switch v := arg.(type) { + case int64: + return v + case int: + return int64(v) + default: + f.t.Fatalf("script argument %#v has unexpected type %T", arg, arg) + return 0 + } +} + +func (f *fakeRedis) Eval(_ context.Context, _ string, keys []string, args ...any) *redis.Cmd { + f.mu.Lock() + f.eval++ + err := f.err + if err == nil { + // A failing EVAL never reached the script cache, so it must not + // leave the fake claiming the body is loaded. + f.cached = true + } + f.mu.Unlock() + if err != nil { + return redis.NewCmdResult(nil, err) + } + return redis.NewCmdResult(f.script(keys, args), nil) +} + +func (f *fakeRedis) EvalSha(_ context.Context, _ string, keys []string, args ...any) *redis.Cmd { + f.mu.Lock() + f.evalSha++ + err, cached := f.err, f.cached + f.mu.Unlock() + if err != nil { + return redis.NewCmdResult(nil, err) + } + if !cached { + return redis.NewCmdResult(nil, redis.ErrNoScript) + } + return redis.NewCmdResult(f.script(keys, args), nil) +} + +func (f *fakeRedis) EvalRO(ctx context.Context, script string, keys []string, args ...any) *redis.Cmd { + return f.Eval(ctx, script, keys, args...) +} + +func (f *fakeRedis) EvalShaRO(ctx context.Context, sha1 string, keys []string, args ...any) *redis.Cmd { + return f.EvalSha(ctx, sha1, keys, args...) +} + +func (f *fakeRedis) ScriptExists(_ context.Context, _ ...string) *redis.BoolSliceCmd { + f.mu.Lock() + defer f.mu.Unlock() + return redis.NewBoolSliceResult([]bool{f.cached}, f.err) +} + +func (f *fakeRedis) ScriptLoad(_ context.Context, _ string) *redis.StringCmd { + f.mu.Lock() + defer f.mu.Unlock() + if f.err != nil { + return redis.NewStringResult("", f.err) + } + f.cached = true + return redis.NewStringResult("fake-sha", nil) +} + +var _ redis.Scripter = (*fakeRedis)(nil) + +func (f *fakeRedis) ttlOf(key string) time.Duration { + f.mu.Lock() + defer f.mu.Unlock() + exp, ok := f.expiry[key] + if !ok { + return -1 + } + return exp.Sub(f.now) +} + +// seed plants a counter with no expiry on it, the state the script's +// PTTL branch exists to recover from. +func (f *fakeRedis) seed(key string, hits int64) { + f.mu.Lock() + defer f.mu.Unlock() + f.counts[key] = hits +} + +func (f *fakeRedis) lastKey() string { + f.mu.Lock() + defer f.mu.Unlock() + if len(f.lastKeys) == 0 { + return "" + } + return f.lastKeys[0] +} + +func mustLimiter(t *testing.T, client redis.Scripter, limit int, window time.Duration) *RedisRateLimiter { + t.Helper() + rl, err := NewRedisRateLimiter(client, limit, window) + if err != nil { + t.Fatalf("unexpected constructor error: %v", err) + } + return rl +} + +func allow(t *testing.T, rl *RedisRateLimiter, key string) bool { + t.Helper() + allowed, err := rl.Allow(context.Background(), key) + if err != nil { + t.Fatalf("unexpected error from Allow(%q): %v", key, err) + } + return allowed +} + +func TestNewRedisRateLimiter_RejectsANilClient(t *testing.T) { + if _, err := NewRedisRateLimiter(nil, 10, time.Minute); err != ErrNilRedisClient { + t.Errorf("expected ErrNilRedisClient, got %v", err) + } +} + +func TestNewRedisRateLimiter_RejectsANonPositiveLimit(t *testing.T) { + for _, limit := range []int{0, -1} { + if _, err := NewRedisRateLimiter(newFakeRedis(t), limit, time.Minute); err != ErrInvalidRateLimit { + t.Errorf("limit %d: expected ErrInvalidRateLimit, got %v", limit, err) + } + } +} + +// A window under a millisecond has no honest representation in PEXPIRE, +// so it is rejected rather than rounded to zero (which Redis refuses) or +// to one (which would silently not be what was asked for). +func TestNewRedisRateLimiter_RejectsAWindowUnderOneMillisecond(t *testing.T) { + for _, window := range []time.Duration{0, 500 * time.Microsecond} { + if _, err := NewRedisRateLimiter(newFakeRedis(t), 10, window); err != ErrInvalidRateWindow { + t.Errorf("window %v: expected ErrInvalidRateWindow, got %v", window, err) + } + } + if _, err := NewRedisRateLimiter(newFakeRedis(t), 10, time.Millisecond); err != nil { + t.Errorf("a one-millisecond window is the smallest valid one, got %v", err) + } +} + +// The same expectation TestInMemoryRateLimiter_AllowsUpToLimit sets for +// the in-process limiter, asserted here so the two stay interchangeable. +func TestRedisRateLimiter_AllowsUpToTheLimitThenDenies(t *testing.T) { + rl := mustLimiter(t, newFakeRedis(t), 3, time.Minute) + + for i := 0; i < 3; i++ { + if !allow(t, rl, "key1") { + t.Fatalf("expected call %d to be allowed", i+1) + } + } + if allow(t, rl, "key1") { + t.Error("expected the 4th call within the window to be denied") + } +} + +func TestRedisRateLimiter_KeysAreIndependent(t *testing.T) { + rl := mustLimiter(t, newFakeRedis(t), 1, time.Minute) + + if !allow(t, rl, "a") || !allow(t, rl, "b") { + t.Fatal("expected the first call for each independent key to be allowed") + } + if allow(t, rl, "a") { + t.Error("expected the second call for key 'a' to be denied") + } +} + +func TestRedisRateLimiter_ResetsAfterTheWindow(t *testing.T) { + server := newFakeRedis(t) + rl := mustLimiter(t, server, 1, time.Minute) + + if !allow(t, rl, "key1") { + t.Fatal("expected the first call to be allowed") + } + if allow(t, rl, "key1") { + t.Fatal("expected the second call inside the window to be denied") + } + + server.advance(time.Minute + time.Second) + + if !allow(t, rl, "key1") { + t.Error("expected a call after the window expired to be allowed again") + } +} + +// Fixed window, not sliding: the expiry is armed by the call that +// created the counter and never pushed out, so hammering a key while +// denied cannot postpone its own reset. Worth pinning down because the +// obvious Redis idiom — PEXPIRE on every call — would quietly turn a +// blocked attacker's own traffic into a permanent block. +func TestRedisRateLimiter_DenialDoesNotExtendTheWindow(t *testing.T) { + server := newFakeRedis(t) + rl := mustLimiter(t, server, 1, time.Minute) + + allow(t, rl, "key1") + server.advance(30 * time.Second) + for i := 0; i < 5; i++ { + if allow(t, rl, "key1") { + t.Fatalf("expected denial %d inside the window", i+1) + } + } + + server.advance(31 * time.Second) + + if !allow(t, rl, "key1") { + t.Error("expected the window to still expire on its original schedule") + } +} + +// The caller's key is opaque and arbitrary ("login:1.2.3.4:someone@ +// example.com"); the prefix is what keeps it from landing on top of +// something the host app keeps in the same Redis. +func TestRedisRateLimiter_NamespacesKeys(t *testing.T) { + server := newFakeRedis(t) + rl := mustLimiter(t, server, 5, time.Minute) + allow(t, rl, "login:1.2.3.4:raymondproguy@dev.com") + if got, want := server.lastKey(), DefaultRedisKeyPrefix+"login:1.2.3.4:raymondproguy@dev.com"; got != want { + t.Errorf("default prefix: server saw key %q, want %q", got, want) + } + + custom, err := NewRedisRateLimiterWithPrefix(server, 5, time.Minute, "staging:rl:") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + allow(t, custom, "signup:1.2.3.4") + if got, want := server.lastKey(), "staging:rl:signup:1.2.3.4"; got != want { + t.Errorf("custom prefix: server saw key %q, want %q", got, want) + } + + bare, err := NewRedisRateLimiterWithPrefix(server, 5, time.Minute, "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + allow(t, bare, "signup:1.2.3.4") + if got, want := server.lastKey(), "signup:1.2.3.4"; got != want { + t.Errorf("empty prefix: server saw key %q, want %q", got, want) + } +} + +// Two prefixes over one server are two separate windows — the point of +// NewRedisRateLimiterWithPrefix. +func TestRedisRateLimiter_PrefixesDoNotShareCounters(t *testing.T) { + server := newFakeRedis(t) + staging, err := NewRedisRateLimiterWithPrefix(server, 1, time.Minute, "staging:") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + prod, err := NewRedisRateLimiterWithPrefix(server, 1, time.Minute, "prod:") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if !allow(t, staging, "login:same-key") { + t.Fatal("expected staging's first call to be allowed") + } + if !allow(t, prod, "login:same-key") { + t.Error("expected production's first call to be unaffected by staging's") + } +} + +// The reason this implementation exists: N replicas, one window. Two +// limiters over one server is what that looks like from inside a test. +func TestRedisRateLimiter_TwoLimitersShareOneWindow(t *testing.T) { + server := newFakeRedis(t) + replicaA := mustLimiter(t, server, 2, time.Minute) + replicaB := mustLimiter(t, server, 2, time.Minute) + + if !allow(t, replicaA, "login:1.2.3.4") { + t.Fatal("expected replica A's first call to be allowed") + } + if !allow(t, replicaB, "login:1.2.3.4") { + t.Fatal("expected replica B's call to be allowed as the window's second") + } + if allow(t, replicaA, "login:1.2.3.4") { + t.Error("expected the third call to be denied: the limit is shared, not per replica") + } +} + +// PEXPIRE takes milliseconds, and the argument reaching it has to be a +// count of them — a time.Duration would arrive as nanoseconds and set a +// window a million times too long. +func TestRedisRateLimiter_SendsTheWindowInMilliseconds(t *testing.T) { + server := newFakeRedis(t) + rl := mustLimiter(t, server, 5, 2*time.Second) + + allow(t, rl, "k") + + if got := server.ttlOf(DefaultRedisKeyPrefix + "k"); got != 2*time.Second { + t.Errorf("counter TTL is %v, want %v", got, 2*time.Second) + } +} + +// A counter with no TTL — set by hand, or left behind by a process that +// died between an INCR and a PEXPIRE — would otherwise count up forever +// and lock its key out permanently. The script re-arms the window +// instead, so the counter recovers on its own one window later. +func TestRedisRateLimiter_RearmsACounterThatHasNoExpiry(t *testing.T) { + server := newFakeRedis(t) + rl := mustLimiter(t, server, 5, time.Minute) + key := DefaultRedisKeyPrefix + "login:stuck" + server.seed(key, 99) + + if allow(t, rl, "login:stuck") { + t.Fatal("expected a counter already past the limit to deny") + } + if got := server.ttlOf(key); got != time.Minute { + t.Fatalf("expected the call to arm a %v window, got TTL %v", time.Minute, got) + } + + server.advance(time.Minute + time.Second) + + if !allow(t, rl, "login:stuck") { + t.Error("expected the counter to have expired and the key to recover") + } +} + +// Redis being down must not read as "allowed". The engine's callers +// (SignUp, Login, RequestMagicLink) propagate this error rather than +// deciding for the host, so a limiter that returned true here would take +// the rate limit off entirely for as long as the outage lasted. +func TestRedisRateLimiter_DeniesAndReportsWhenRedisFails(t *testing.T) { + server := newFakeRedis(t) + down := errors.New("dial tcp 127.0.0.1:6379: connect: connection refused") + server.err = down + rl := mustLimiter(t, server, 10, time.Minute) + + allowed, err := rl.Allow(context.Background(), "login:1.2.3.4") + if allowed { + t.Error("expected a Redis failure to deny, not to allow") + } + if err == nil { + t.Fatal("expected an error") + } + if !errors.Is(err, down) { + t.Errorf("expected the underlying error to stay unwrappable, got %v", err) + } +} + +// go-redis sends EVALSHA first and only falls back to a full EVAL when +// the server reports NOSCRIPT, which is what a cold script cache looks +// like. Asserted because it is the difference between shipping the +// script body on every single login and shipping it once. +func TestRedisRateLimiter_FallsBackToEvalUntilTheScriptIsCached(t *testing.T) { + server := newFakeRedis(t) + rl := mustLimiter(t, server, 10, time.Minute) + + allow(t, rl, "k") + if server.evalSha != 1 || server.eval != 1 { + t.Fatalf("first call: got %d EVALSHA and %d EVAL, want 1 and 1", server.evalSha, server.eval) + } + + allow(t, rl, "k") + if server.evalSha != 2 || server.eval != 1 { + t.Errorf("second call: got %d EVALSHA and %d EVAL, want 2 and 1 — the script should be cached now", server.evalSha, server.eval) + } +} diff --git a/store/interfaces.go b/store/interfaces.go index 0742ae2..901466f 100644 --- a/store/interfaces.go +++ b/store/interfaces.go @@ -131,6 +131,21 @@ const ( EventRecoveryCodeUsed AuditEventType = "recovery_code_used" EventRecoveryCodeFailed AuditEventType = "recovery_code_failed" EventPasswordBreachRejected AuditEventType = "password_breach_rejected" + + // EventAnomalyDetected records that a login attempt tripped one or + // more anomaly signals (see security.AnomalySignal). Metadata + // carries a "signals" key listing which ones fired, plus the counts + // behind them. Recorded on an otherwise SUCCESSFUL primary + // authentication — it annotates a login that was allowed to + // proceed, it is never a rejection, and there is deliberately no + // matching sentinel error for callers to branch on. + EventAnomalyDetected AuditEventType = "anomaly_detected" + + // EventCredentialStuffingDetected records that one IP's recent failed + // attempts were spread across enough different target accounts to + // look like credential stuffing rather than a forgotten password. + EventCredentialStuffingDetected AuditEventType = "credential_stuffing_detected" + ) // AuditEvent is a single security-relevant, queryable record. @@ -323,3 +338,32 @@ type RecoveryCodeStore interface { // hygiene, if a host app wants to clean up explicitly. DeleteAll(ctx context.Context, userID string) error } + +type LoginAttemptOutcome string + +const ( + OutcomeSuccess LoginAttemptOutcome = "success" + OutcomeFailure LoginAttemptOutcome = "failure" +) + +type LoginAttempt struct { + ID string + UserID string + IP string + UserAgent string + Outcome LoginAttemptOutcome + CreatedAt time.Time +} + +type AnomalyStore interface { + RecordAttempt(ctx context.Context, attempt LoginAttempt) error + ListRecentSuccesses(ctx context.Context, userID string, limit int) ([]LoginAttempt, error) + CountFailuresForUser(ctx context.Context, userID string, since time.Time) (int, error) + CountFailuresForIP(ctx context.Context, ip string, since time.Time) (int, error) + CountTargetsForIP(ctx context.Context, ip string, since time.Time) (IPTargetCounts, error) +} + +type IPTargetCounts struct { + DistinctAccounts int + UnknownTargetFailures int +}