Feat: Shadow mode - #39
Merged
Merged
Conversation
…rtions SECURITY.md, CONTRIBUTING.md and CLA.md. A security product with no disclosure address is itself something researchers notice, and all three are needed before outside contributions can be accepted under any licensing model. SECURITY.md states scope and response targets, and explicitly puts shipped defaults in scope. That is not boilerplate: a previous release seeded an IP allowlist covering every RFC1918 range, which disabled inspection entirely in the topology this product targets. A permissive default deserves the same treatment as a code-level bug. CONTRIBUTING.md documents the gates and, more usefully, the four things that have each already caused a shipped defect: a new endpoint config key must be added to config_resolver.resolve() or it is silently dropped; a new Admin API route needs an RBAC entry or it is 403 for everyone including admin; a changed response shape needs the OpenAPI spec updated; and defaults are a security decision, not a way to make tests pass. CLA.md follows the Apache ICLA structure -- a broad licence grant rather than copyright assignment, so contributors keep their copyright while the project retains the option to relicense later. It is clearly marked as a template needing review by counsel before it is relied upon. I am not qualified to give legal advice and have not pretended otherwise. Also parks the two credential-stuffing assertions as known gaps. They asserted that a single common-credential POST returns 403 and they passed -- but because bare curl trips the fake-modern-browser check, not because credential stuffing was detected. Given a realistic browser client both requests are allowed. Credential stuffing is automated and repetitive, so the honest assertion is that a burst is blocked while a single attempt from a real browser is not. Where that line sits is a product decision, so they are parked visibly rather than rewritten to match whatever the code happens to do today.
R-27 (pattern scanning) was closed and its two assertions restored, but the summary line still pointed all known gaps at it. The two gaps now reported are the credential-stuffing assertions, which are unrelated. The summary now refers the reader to the [KNOWN GAP] lines, which carry their own reasons.
Monitoring mode already decides what it would have blocked -- waf_handler
computes is_monitoring_would_block, writes a line to the error log, bumps a
counter, and then discards the detail. So the only way to answer "what breaks if
I turn blocking on?" is to grep logs, which is why promoting an endpoint is
currently a blind leap.
shadow_recorder keeps those decisions with their attribution: vhost, endpoint,
score, which rules fired, and the flags behind them.
Two constraints shaped the design, both from defects already fixed here:
* The request path must not touch Redis (R-04), so record() writes to a shared
dict and redis_sync's existing timer drains it -- that function already holds
a connection and runs on a predictable interval.
* One timer per request is how lua_max_running_timers gets exhausted and
redis_sync starved (R-03), so there is no per-request timer at all.
Storage is bounded deliberately: 500 buffered records, 2000 retained, values
clipped, everything on a TTL. A WAF in monitoring mode on a busy site sees every
request, and an unbounded recorder becomes its own incident. Dropped records are
counted so the UI can say it is showing a sample rather than implying
completeness.
Both would-block branches record. There are two in process_request and they log
differently ("MONITORING (profile would block)" and "WOULD_BLOCK (profile)");
wiring only the one I found first would have silently understated impact, which
is the single thing this feature must get right.
One defect found and fixed during verification, in this commit's own code. The
first end-to-end test recorded three copies of one request. redis_sync's timer
runs on every worker and this machine has 24, so every worker read the same
buffer tail, drained the same slots and pushed duplicates -- inflating the
impact estimate by up to the worker count. The drain now claims its range with
an atomic incr, so workers take disjoint slices. Same non-atomic
read-modify-write class as R-09, this time mine. Verified: one request produces
exactly one record, twenty produce exactly twenty.
Verification: 53 unit specs pass (10 new, covering the claim invariant, the
bound and the clipping), config contract holds, integration 42 passed / 2 known
gaps / 0 failed. An intermediate integration run showed 6 failures; that was my
own test setup -- I had switched the _default vhost to monitoring mode to
exercise the recorder and had not put it back. The seed file is unchanged at
"blocking" and a fresh volume is unaffected, confirmed by restoring the running
config and re-running.
Exposes what monitoring mode would have blocked, so a rule set can be
proven against real traffic before it starts rejecting it.
GET /shadow/summary totals, top rules, top flags, per-scope counts
GET /shadow/decisions individual decisions, newest first
GET /shadow/impact pre-flight impact of promoting one scope
POST /shadow/promote switch a vhost monitoring -> blocking
DELETE /shadow/decisions discard the sample
Aggregates by detection flag, not just by rule. The rule name is the
profile ("legacy x9"), which tells an operator nothing about what to
suppress; the flag is the actual reason -- kw:viagra, fp_flag:suspicious-bot.
Both the summary and the impact response carry top_flags. Flags are clipped
before storage since part of a flag comes from the request.
dropped_total surfaces on both, and the impact response sets
sample_incomplete when the recorder's buffer overflowed, so a promote
decision is never made against a silently truncated sample.
Promote is idempotent -- already-blocking returns promoted:false rather
than an error -- and reports previous_mode so the change is auditable.
RBAC: admin gets read/promote/delete, operator and viewer read only.
Verified against the live stack: viewer 200 on reads, 403 on promote and
delete; admin promote flipped _default to blocking and the WAF then
returned 403 for a payload that had been passing.
check-api-contract.py now honours x-contract-example-query, so an endpoint
with a required query parameter is validated rather than skipped --
/shadow/impact needs vhost_id.
Gates: 55 unit specs, config contracts hold, contract check 11 endpoints,
integration 42 passed / 2 known gaps / 0 failed.
A page under Security that answers "what happens if I turn this on" before
anything starts rejecting traffic.
Top detections leads, because that is the actionable list -- kw:viagra,
fp_flag:suspicious-bot -- while top rules ("legacy x9") only names the
profile. Counts render as relative bars so the dominant signal is obvious
at a glance rather than requiring the numbers to be compared by eye.
Promote never fires straight from the table: it fetches the impact first
and shows requests, distinct clients, average score, the flags responsible
and the endpoints affected, so the decision is made against evidence.
When the recorder dropped records, both the page and the confirmation
dialog say the sample understates reality rather than presenting a
truncated count as a total.
The empty state explains why it might be empty -- a vhost already in
blocking mode records nothing here.
Verified against the live stack: 3 monitored requests produced exactly 3
decisions (one per request, confirming the drain claim holds across
workers), and the impact response matched what the dialog consumes.
Gates: typecheck 0 errors, lint 0 errors / 59 warnings (baseline),
generated.ts regenerated from the spec, Docker build succeeds.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces “Shadow mode”: recording what monitoring mode would have blocked, surfacing it via new Admin API endpoints and a new Admin UI page, plus wiring/permissions/docs to support promoting a vhost to blocking once the observed sample looks acceptable.
Changes:
- Add a buffered shadow-decision recorder (shared dict → Redis via
redis_sync) and record would-block decisions from the request path. - Add Admin API endpoints (
/shadow/*) with RBAC permissions and OpenAPI/docs updates, plus a new Admin UI “Shadow Mode” page. - Add project governance docs (
SECURITY.md,CONTRIBUTING.md,CLA.md) and adjust test script messaging for known gaps.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| SECURITY.md | Adds security policy and private reporting instructions. |
| scripts/test-waf.sh | Replaces two misleading assertions with explicit “known gap” logging. |
| scripts/check-api-contract.py | Allows GET contract checks to include example query parameters from spec. |
| openresty/spec/shadow_recorder_spec.lua | Adds unit tests for the new shadow recorder behavior and bounds. |
| openresty/lua/waf_handler.lua | Records would-block decisions during monitoring-mode request handling. |
| openresty/lua/shadow_recorder.lua | Implements shared-dict buffering and Redis flush/aggregation for shadow decisions. |
| openresty/lua/redis_sync.lua | Flushes the shadow recorder buffer as part of the existing sync cycle. |
| openresty/lua/rbac.lua | Adds RBAC resource/actions and route mappings for shadow endpoints. |
| openresty/lua/api_handlers/shadow.lua | Implements the /shadow/* Admin API endpoints (summary, decisions, impact, promote, clear). |
| openresty/lua/admin_api.lua | Registers the new shadow API handler module. |
| openresty/conf/nginx.conf | Adds lua_shared_dict shadow_cache for buffering. |
| docs/openapi.yaml | Adds schemas and paths for shadow endpoints, incl. contract example query. |
| docs/API_HANDLERS.md | Documents the new shadow handler and permission mapping. |
| CONTRIBUTING.md | Adds contributor guidance and CI/check expectations. |
| CLA.md | Adds a contributor license agreement and signing instructions. |
| admin-ui/src/pages/shadow/ShadowMode.tsx | New UI page to view summary/decisions and promote vhosts. |
| admin-ui/src/components/layout/Sidebar.tsx | Adds “Shadow Mode” navigation link under Security. |
| admin-ui/src/App.tsx | Registers the new /security/shadow route. |
| admin-ui/src/api/generated.ts | Updates generated API types to include shadow endpoints/schemas. |
| admin-ui/src/api/client.ts | Adds client methods and TS interfaces for shadow endpoints. |
Suppressed comments (1)
openresty/lua/shadow_recorder.lua:181
- If
flush()is updated to take a shared-dict lock (as suggested above), the lock should be released at the end of the drain. Relying only on the TTL unnecessarily blocks other workers from flushing again even when the drain finishes quickly.
local dropped = shadow_cache:get(DROPPED)
if dropped and dropped > 0 then
red:hincrby(KEYS.stats, "dropped_total", dropped)
shadow_cache:set(DROPPED, 0)
end
return flushed
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+129
to
+134
| local pending = head - tail | ||
| local claimed_end = shadow_cache:incr(BUFFER_TAIL, pending, 0) | ||
| if not claimed_end then | ||
| return 0 | ||
| end | ||
| local claimed_start = claimed_end - pending + 1 |
Comment on lines
+127
to
+130
| local limit = math.min(tonumber(query_arg("limit", DEFAULT_LIMIT)) or DEFAULT_LIMIT, MAX_LIMIT) | ||
| local vhost_id = query_arg("vhost_id") | ||
| local endpoint_id = query_arg("endpoint_id") | ||
|
|
Comment on lines
+63
to
+65
| local function read_decisions(red, vhost_id, endpoint_id, limit) | ||
| local raw = red:lrange(KEYS.decisions, 0, MAX_LIMIT - 1) | ||
| local out = {} |
Comment on lines
+57
to
+75
| -- The recorder buffers in a shared dict and a timer drains it: the request path | ||
| -- must not touch Redis, and a timer per request would exhaust the timer pool. | ||
| local function record_shadow_decision(summary, client_ip, host, path, method, profile_result) | ||
| local ok, shadow_recorder = pcall(require, "shadow_recorder") | ||
| if not ok or not shadow_recorder then | ||
| return | ||
| end | ||
| shadow_recorder.record({ | ||
| vhost_id = summary.vhost_id, | ||
| endpoint_id = summary.endpoint_id, | ||
| client_ip = client_ip, | ||
| host = host, | ||
| path = path, | ||
| method = method, | ||
| score = profile_result.score or 0, | ||
| blocked_by = profile_result.blocked_by, | ||
| flags = profile_result.flags, | ||
| }) | ||
| end |
Four findings, all valid. The drain claim (shadow_recorder.lua). The incr(TAIL, pending) range claim fixed the duplicate records it was written for and introduced a worse failure: two workers reading the same pending count both claimed it, so the second claimed slots the writer had not filled yet and pushed the tail past the head. Every record written into that gap was then skipped for good, because the head <= tail guard reports an empty buffer. The verification that passed it only checked for duplicates -- and no duplicates is exactly what overshoot looks like. The loss appears in the *next* batch. Replaced with a single-drainer lock: add() is atomic and fails when the key exists, so one worker drains and the rest return. The tail now advances per completed slot instead of in one jump, so a drain that dies part-way resumes where it stopped, with neither a replayed record nor a lost one. The drain runs under pcall and releases the lock on failure, so the lock TTL only has to cover a worker that dies outright. Two specs guard it now: one drives a concurrent drain and asserts records written afterwards are still flushed, one kills a drain mid-range and asserts the resumed drain picks up exactly the incomplete slots. Live: three consecutive batches across 24 workers gave 5 -> 10 -> 14, exact, where the old code lost everything after the first flush. limit clamping (api_handlers/shadow.lua). limit=0 or negative returned an empty list, which reads as "nothing recorded" rather than as bad input. Now clamped at both ends; limit=0 and limit=-5 return 1. Over-reading (api_handlers/shadow.lua). An unfiltered limit=25 poll from the UI pulled and decoded 500 JSON blobs. Unfiltered, every entry read is an entry returned, so it now asks for exactly the page. Filtered reads keep the wide scan, which is what makes the filter meaningful. Hot-path require (waf_handler.lua). pcall(require, "shadow_recorder") ran on every would-block request. Cached in an upvalue following get_multi_executor, with a tried flag so a failed require is not retried per request. Gates: 57 unit specs, config contracts hold, contract check 11 endpoints, integration 42 passed / 2 known gaps / 0 failed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.