Hot-swappable protection config, sustained rate limits, and penalty box

[kriszyp]

Closes #11, closes #4, closes #1.

What this does

Three protection features that build on each other, reviewed and folded into one PR:

Hot-swappable protection config (#11)

Protection config (per-IP rate limits, concurrency caps, CIDR allow/blocklists, JA3 blocklist, handshake timeout, SNI enforcement) previously required a listener restart to change — the biggest operational gap for incident response, and a prerequisite for the planned Harper-WAF → Symphony blocklist feed.

• updateConfig({ protection: [{ port, protection }] }) — per-listener updates, applied atomically (validate-all → parse-all → store-all; errors name every offending port, nothing tears on a malformed CIDR).
• CIDR lists moved inside the existing ArcSwap<ProtectionConfig>, so one atomic pointer store swaps the whole snapshot; check() already loads the config per admission, so swaps reach live listeners with zero hot-path cost.
• Standalone server: protection presence is part of listenerSig (none↔some transitions force a seamless recreate so the listener gets the right Option<ProtectionState>); protection contents are excluded so contents-only changes hot-apply without recreating.
• README hot-swap contradiction fixed; first Rust unit tests for protection.rs.

Burst + sustained rate limits (#4)

A second, independent per-IP token bucket over a per-minute window (sustained: { connectionsPerMinute, burst? }) beside the per-second bucket. Both are checked on admission; exhausting either blocks. Same lock-free ×1000 fixed-point AtomicU32 CAS idiom.

Penalty box (#1)

On any rate-limit exhaustion, the IP is boxed for penaltyBox.durationMs (default 10 min). Attempts while boxed are blocked outright but still debit the buckets, so continued excess — actual bucket exhaustion, not mere attempts — extends the penalty (reset to full duration from now). Expires naturally once the source quiets. Boxed IPs surface in blockedIps().penaltyBoxed.

Also fixed along the way

• ProtectionState::evict() was dead code — nothing ever spawned it, so the per-IP DashMap grew unboundedly per unique source IP (exactly under IP-diverse attack). Now a periodic task (60s, spawn_blocking so the O(N) retain never stalls the accept path), with lazy bucket projection so an attacker can't reset their sustained window by pausing until eviction.
• All protection timing moved from wall-clock to a monotonic timebase — an NTP step can no longer release boxed IPs early or freeze refills.
• Penalty deadlines use saturating math (a "ban forever" durationMs can't wrap into the past).
• Active-connection counting is now RAII through the held Arc<IpState> (Decision::Allow carries it), structurally eliminating an eviction/decrement race.
• blockedIps(): HashSet dedup, elapsed-refill projection (no stale over-reporting), penaltyBoxed gated on current config, and concurrency-limited IPs now reported even when no rate limit is configured (pre-existing bug).
• Hot-path micro-perf: float-derived bucket constants precomputed at config parse instead of per connection.

Review

Cross-model reviewed (Gemini diff-only leg + Opus domain pass, two rounds; all blockers/significant findings fixed in 4abb297 and 0d3571a). Caveat: the Codex leg was unavailable both attempts (workspace spend cap), so there is no second outside-model code-trace — extra reviewer attention on the atomics in src/protection.rs is welcome.

Testing

• cargo test: 31/32 — sole failure is sni::tests::test_is_grease, pre-existing on main, fixed in Fix CI: check out core submodule, drop retired macos-13 runner #18.
• npm test: 46/47 — sole failure is the pre-existing hardcoded '0.4.0' version assertion, fixed in Fix CI: check out core submodule, drop retired macos-13 runner #18.
• cargo clippy --all-targets: no new items vs main (main's 10 pre-existing errors are tracked separately).
• New coverage: 20+ Rust unit tests (token buckets across config swaps, penalty entry/extend/expiry, eviction retention invariants, overflow), Node specs for updateConfig protection hot-swap, sustained limits, penalty box + hot-swap, and server-level none↔some config-file transition tests.

— Drafted and driven by KrAIs (Claude Opus 4.8 / Fable 5) on Kris's behalf; adversarially reviewed as described above.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request enhances the proxy's protection layer by introducing sustained (per-minute) rate limits, a penalty box mechanism to temporarily block offending IPs, and hot-swappable per-listener protection configurations. It also switches to a monotonic clock for timing, implements lazy bucket projection for IP state eviction, and refactors connection tracking to prevent active-counter decrement races. A critical concurrency issue was identified in the refill_and_consume function, where multiple threads could concurrently calculate the same elapsed time and double-refill the token bucket before the refill timestamp is updated. It is recommended to update the refill timestamp first using compare_exchange to claim the elapsed time window.

[gemini-code-assist]

There is a race condition in refill_and_consume under high concurrency. If multiple threads concurrently call refill_and_consume before the winning thread updates last_refill_ns, they will all see the old last_refill_ns timestamp, calculate the same elapsed time, and repeatedly add the refill tokens to the bucket. This leads to a double-refill (or multi-refill) bug, allowing clients to bypass the rate limit during concurrent floods.

To fix this, we should update last_refill_ns first using compare_exchange to "claim" the elapsed time before updating the token count. This ensures only one thread can successfully apply the refill for any given elapsed time window.

	// Refill phase — caps at burst_fp to handle burst decreases without underflow.
	loop {
		let last = last_refill_ns.load(Ordering::Relaxed);
		let elapsed = now.saturating_sub(last);
		let refill = ((elapsed as f64) * rate_per_ns * 1000.0) as u32;
		if refill == 0 {
			break;
		}
		// Advance the refill timestamp first to claim the elapsed time and prevent double-refill under concurrency.
		if last_refill_ns
			.compare_exchange(last, now, Ordering::Relaxed, Ordering::Relaxed)
			.is_ok()
		{
			loop {
				let old_tokens = tokens.load(Ordering::Relaxed);
				let new_tokens = old_tokens.saturating_add(refill).min(burst_fp);
				if tokens
					.compare_exchange(old_tokens, new_tokens, Ordering::Relaxed, Ordering::Relaxed)
					.is_ok()
				{
					break;
				}
			}
			break;
		}
	}

[kriszyp]

Codex backfill review completed (the Codex leg was quota-blocked when this PR opened; the description's review caveat is now resolved). Outcome, so reviewers don't re-derive it:

Both Codex "blockers" were refuted on code-trace:

• Concurrent refill double-counting — the refill CAS caps at burst_fp on every path and exactly one thread wins the last_refill_ns CAS, so over-admission is bounded by burst and self-corrects; this is the documented approximate-bucket design (CLAUDE.md), not a defect.
• Eviction removing IpState mid-check — only reachable in concurrency-only configs, a nanosecond window against a 60s eviction tick, non-amplifiable, and the Arc-carried release keeps counters correct.

One real finding, fixed in 68a2512 (plus bundled hardening):

• updateConfig previously swapped routes before validating protection — a combined update with invalid protection left new routes live while protection stayed stale. Routes are now built-and-held until protection validates+parses; both apply all-or-nothing (regression test included).
• Eviction's penalty retention is now config-gated, so disabling penaltyBox and re-enabling it can't resurrect a pre-disable deadline.
• NaN/negative rate values now error at parse instead of silently disabling the limit.
• Penalty deadline writes use fetch_max (monotonic under concurrent writers).

— KrAIs (Claude Opus 4.8 / Fable 5), on Kris's behalf

🤖 Generated with Claude Code

[Devin-Holland]

Deep adversarial review — no blockers

Went at this with two independent skeptical code-traces (a manual pass plus a second agent whose only job was to break the atomics). Neither could break refill_and_consume, the penalty-box state machine, the ArcSwap hot-swap, or the monotonic timebase — the token-bucket approximations are all correctly bounded by .min(burst_fp) and documented as such. Really nice work; the two-phase validate→store atomicity, the held-Arc release (Fix 6), and the lazy-projection eviction invariant are all sound.

Verified empirically off the PR head in a worktree:

• cargo test --lib → 78/78 pass
• cargo clippy --all-targets → only the 10 pre-existing dead-code warnings, none in the new code

Everything inline below is hardening / nits — nothing that should gate the merge. The one I'd most like your read on is the ip_table ↔ penalty-box tension (evict_at), since it's a place where two features in this same PR pull against each other and it's really a design call for you.

One stale note for the description: sni::tests::test_is_grease passes on this branch now (the JA4 fixtures commit brought the fix in), so the "31/32, sole failure pre-existing" line is out of date.

Reviewed by devain (Claude Opus 4.8), on Devin's behalf.

[Devin-Holland]

[Medium] The penalty box re-opens the unbounded-ip_table growth that this PR's eviction fix is built to close.

evict_at retains any entry while now < deadline, so a boxed IP is pinned for the full durationMs (default 10 min; an absurd/Infinity value saturates to u64::MAX → effectively permanent). Under exactly the IP-diverse flood the eviction fix targets, every attacker that trips a limit gets pinned for the whole penalty window, and there's no absolute cap on ip_table — retention is purely time-based. So the headline eviction fix ("DashMap grew unboundedly per unique source IP") is partly undone by the headline penalty-box feature: entries that would otherwise evict once buckets refill (often < 1 min) now live for the penalty duration, and a ban-forever durationMs makes them immortal.

TCP (no source spoofing) plus the pre-check handshake bound the real rate, so ~10^5 unique attackers is ~10–15 MB — not catastrophic — but the relationship entries ≈ unique-attackers × penalty-window deserves an explicit decision. Worth documenting, and maybe a hard ip_table size cap with pressure-eviction (drop soonest-expiring / oldest) so the protection layer can't itself become the memory-exhaustion vector. Non-blocking — mostly want your call on it.

[Devin-Holland]

[Low, but a real gap] Eviction-vs-admission race can transiently split per-IP state → concurrency cap (and rate bucket) exceeded.

get_or_create_state inserts and releases the shard lock here; the active increment happens later (fetch_update/fetch_add around L356–364) with no lock held and no await between. The 60s eviction task can fire in that window. A freshly-created entry has active == 0 and a full bucket, so evict_at's retain predicate returns false and removes it before the connection increments it:

1. Conn A: get_or_create_state inserts E1 (active=0), returns Arc(E1)
2. Eviction retain: E1 looks idle → evicts E1
3. Conn A: increments E1.active 0→1 → Allow(E1) (now orphaned)
4. Conn B (same IP): map empty → inserts fresh E2, increments → Allow(E2)

→ two admitted conns for one IP with maxConcurrentPerIp: 1; the same mechanism hands B a fresh full bucket (rate limit momentarily reset). Fix 6's held-Arc keeps the decrement correct but doesn't close the admission window. Needs the 60s tick to land inside a few-instruction span, so exploitability is very low — flagging because active_guard_arc_release_survives_eviction evicts after the increment (active=1), so this exact window has no coverage. If you want it airtight: do the concurrency fetch_update while still holding the DashMap Ref, before cloning the Arc out.

[Devin-Holland]

[Low] penaltyBox.durationMs skips the finite/≤0 validation its sibling rate fields got in Fix 3.

parse_protection_config now rejects non-finite / ≤0 connectionsPerSecond/connectionsPerMinute and negative burst, but durationMs is just pb.duration_ms.unwrap_or(600_000.0) as u64. Since NaN as u64 == 0, a durationMs: NaN silently disables the penalty box the operator asked for; a negative value → 0 (also disabled); Infinity → ban-forever. Inconsistent with the sibling fields and the failure mode is silent — suggest the same is_finite() / > 0 guard returning an error.

[Devin-Holland]

[Low] spawn_blocking moves the eviction work off the reactor, but doesn't stop retain from blocking an admission on the shard it's holding.

The comment reads as "O(N) retain no longer stalls the accept path." It keeps the work off a reactor thread, but DashMap::retain takes a per-shard write lock while check()'s get() needs a read lock on the same shard — so an admission whose IP hashes to the shard currently being retained blocks a reactor thread until that shard's float-math loop finishes. Under the large table this targets, one shard can hold many entries, so there's real residual latency. Consider chunked eviction (or at least softening the comment's claim).

[Devin-Holland]

[Nit] These integration assertions are too loose to catch a regression.

• L362 penaltyReasons.includes('penalty_boxed') || penaltyReasons.includes('rate_limited') passes even if penalty_boxed never fires.
• L376 info.penaltyBoxed.includes('127.0.0.1') || info.rateLimited.includes('127.0.0.1') — same OR-escape.
• L381 readmits IP after penalty expires only checks the TCP connect succeeds, but symphony blocks after accept, so that succeeds regardless of penalty state (the test comment concedes it).

The precise behavior is well covered by the Rust unit tests, and the server.spec.ts none↔some recreate tests are genuinely solid (real TLS round-trip, polls to blocked/unblocked) — so this is cosmetic. Just tighten these three to assert the exact reason / exact IP / an actual proxied response so they'd fail if the feature regressed.