From b72c19a34da1f6daefe25dc77ecfebf58abe9aa5 Mon Sep 17 00:00:00 2001 From: Martin Dobrev Date: Fri, 7 Aug 2026 14:32:08 +0100 Subject: [PATCH 1/6] docs: Add governance groundwork and park the credential-stuffing assertions 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. --- CLA.md | 103 ++++++++++++++++++++++++++++++++++++++++++++ CONTRIBUTING.md | 87 +++++++++++++++++++++++++++++++++++++ SECURITY.md | 53 +++++++++++++++++++++++ scripts/test-waf.sh | 24 ++++++----- 4 files changed, 256 insertions(+), 11 deletions(-) create mode 100644 CLA.md create mode 100644 CONTRIBUTING.md create mode 100644 SECURITY.md diff --git a/CLA.md b/CLA.md new file mode 100644 index 0000000..899193d --- /dev/null +++ b/CLA.md @@ -0,0 +1,103 @@ +# Contributor License Agreement + +> **This is a template and has not been reviewed by a lawyer.** It follows the +> structure of the widely used Apache Individual CLA, but you should have it +> reviewed by qualified counsel before relying on it. Whoever adopts it is +> responsible for its adequacy in their jurisdiction. + +## Why this exists + +Forms WAF is MIT licensed today. This agreement grants Dobrev IT Ltd a licence +broad enough to relicense or dual-license the project in future — for example +under an open-core model — without having to locate and obtain permission from +every past contributor. + +**You keep the copyright in your contribution.** This is a licence grant, not an +assignment. You remain free to use your own work however you wish, including in +other projects. + +If you would rather not sign, that is entirely reasonable. Please open an issue +describing what you want to change; a maintainer may be able to implement it +independently. + +--- + +## Agreement + +By submitting a Contribution to this project, You accept and agree to the +following terms for Your present and future Contributions. + +**1. Definitions** + +"You" means the copyright owner, or the legal entity authorised by the copyright +owner, entering into this agreement. + +"Contribution" means any original work of authorship, including any +modification of or addition to an existing work, that is intentionally submitted +by You to the project for inclusion in or documentation of any of its products. +"Submitted" means any form of electronic, verbal or written communication sent +to the project or its maintainers, including but not limited to pull requests, +issues and electronic mailing lists, excluding communication conspicuously +marked or otherwise designated in writing by You as "Not a Contribution". + +**2. Grant of Copyright Licence** + +You grant to Dobrev IT Ltd and to recipients of software distributed by it a +perpetual, worldwide, non-exclusive, no-charge, royalty-free, irrevocable +copyright licence to reproduce, prepare derivative works of, publicly display, +publicly perform, sublicense and distribute Your Contributions and such +derivative works, **under any licensing terms, including proprietary terms**. + +**3. Grant of Patent Licence** + +You grant to Dobrev IT Ltd and to recipients of software distributed by it a +perpetual, worldwide, non-exclusive, no-charge, royalty-free, irrevocable +(except as stated in this section) patent licence to make, have made, use, offer +to sell, sell, import and otherwise transfer the Work, where such licence +applies only to those patent claims licensable by You that are necessarily +infringed by Your Contribution alone or by combination of Your Contribution with +the Work. + +If any entity institutes patent litigation alleging that Your Contribution, or +the Work to which You contributed, constitutes direct or contributory patent +infringement, then any patent licences granted to that entity under this +agreement terminate as of the date such litigation is filed. + +**4. Your Representations** + +You represent that You are legally entitled to grant the above licences. If Your +employer has rights to intellectual property that You create, You represent that +You have received permission to make the Contribution on behalf of that +employer, that Your employer has waived such rights, or that Your employer has +executed a separate corporate CLA. + +You represent that each Contribution is Your original creation, and that Your +Contribution submissions include complete details of any third-party licence or +other restriction of which You are personally aware and which is associated with +any part of Your Contribution. + +**5. No Warranty** + +You are not expected to provide support for Your Contributions, except to the +extent You desire to do so. Contributions are provided "AS IS", without +warranty of any kind, express or implied, including any warranty of +merchantability or fitness for a particular purpose. + +**6. Notification** + +You agree to notify the project of any facts or circumstances of which You become +aware that would make these representations inaccurate in any respect. + +--- + +## How to sign + +Comment on your first pull request with: + +``` +I have read the CLA document and I hereby sign the CLA. +``` + +Include the name and email address you use for commits. A maintainer will record +it. If you are contributing on behalf of an employer, say so — a corporate CLA +may be needed instead. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000..adff893 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,87 @@ +# Contributing + +Thanks for considering a contribution. + +## Before you start + +For anything beyond a small fix, open an issue first. This is a security product +with a defence engine whose scoring is calibrated against real traffic patterns, +and a change that looks harmless can shift the boundary between "flag" and +"block" for every deployment. Discussing the shape of a change first saves +rework. + +**Security issues do not belong in pull requests or public issues.** See +[SECURITY.md](SECURITY.md). + +## Development + +The stack runs under Docker Compose; see [README.md](README.md). `CLAUDE.md`, if +present in your checkout, carries working notes on the architecture. + +```bash +docker compose up -d --build +docker compose exec redis sh /init-data.sh # seed defaults +``` + +## The checks your change must pass + +CI runs these on every pull request, and all of them block: + +```bash +# Lua unit tests (busted, on LuaJIT -- see openresty/spec/README.md) +docker build -t forms-waf-lua-test -f openresty/Dockerfile.test ./openresty +docker run --rm -v "$PWD/openresty/lua:/app/lua:ro" \ + -v "$PWD/openresty/spec:/app/spec:ro" \ + forms-waf-lua-test --verbose /app/spec + +# Admin UI +cd admin-ui && npm ci && npm run typecheck && npm run lint && npm run audit:gate + +# Integration suite (needs a running stack, and admin credentials so it can +# raise the rate limits it would otherwise trip) +WAF_ADMIN_USER=admin WAF_ADMIN_PASS=... ./scripts/test-waf.sh + +# API contract, against a running stack +python3 scripts/check-api-contract.py +``` + +Syntax-check Lua with **LuaJIT**, not the system `luac`. Several modules use +`goto`/`::continue::`, which PUC Lua 5.1 rejects and Lua 5.4 accepts — neither +matches the runtime. + +## Things that are easy to get wrong + +These have each caused a shipped defect, so they are worth knowing up front. + +**A new endpoint config key must be added to `config_resolver.resolve()`.** That +function builds its result from an explicit allowlist, and anything not named +there is silently dropped before the request path sees it. Three features were +inert for exactly this reason. `openresty/tests/config_contract_spec.lua` guards +the seam — add your key to it. + +**A new Admin API route needs an RBAC entry.** `rbac.check_permission()` +default-denies any route with no mapping, so a registered-but-unmapped handler +returns 403 for every role including admin. The startup audit will log the gap; +do not ignore it. + +**A change to an API response shape needs the OpenAPI spec updated.** +`docs/openapi.yaml` is validated against a live server, and the admin UI's types +are generated from it. Both directions are enforced. + +**Defaults are a security decision.** Shipping something permissive "so the +tests pass" is how the allowlist ended up disabling the WAF. If a test and a +default disagree, work out which one is wrong. + +## Commits and pull requests + +- `type: Subject` — `feat:`, `fix:`, `docs:`, `test:`, `chore:`, `ci:` +- Explain *why*, not just what. A commit that says what the diff already says is + a wasted opportunity. +- Say what you verified, and how. "Tests pass" is weaker than the numbers. +- Branch from `main`, open a PR against `main`. + +## Contributor License Agreement + +Contributions require a signed CLA — see [CLA.md](CLA.md). This lets the project +relicense or dual-license in future without tracking down every past +contributor. You keep the copyright in your contribution. diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000..0a5d1e0 --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,53 @@ +# Security Policy + +## Reporting a vulnerability + +**Please do not open a public issue for security problems.** + +Report privately through either channel: + +- GitHub's [private vulnerability reporting](https://github.com/dobrevit/forms-waf/security/advisories/new) (preferred — it keeps the discussion attached to the repository) +- Email **security@dobrev.eu** + +Please include enough detail to reproduce: affected version or commit, configuration relevant to the issue, and the steps or request that triggers it. A proof of concept helps, but a clear description is enough to get started. + +### What to expect + +| Stage | Target | +|---|---| +| Acknowledgement | 3 working days | +| Initial assessment | 10 working days | +| Fix or mitigation for a confirmed high-severity issue | 30 days | + +If a report is disputed, we will explain the reasoning rather than closing it silently. If a fix will take longer than the target, we will say so and why. + +## Scope + +In scope: + +- The WAF request path (`openresty/lua/`) — bypasses, request smuggling, injection, denial of service +- The Admin API and its authentication, session handling and RBAC +- The admin UI (`admin-ui/`) +- Default configuration shipped in `redis/init-data.sh` and `helm/` +- The Helm chart and container images + +Out of scope: + +- Vulnerabilities in a deployer's own upstream application +- Findings that require a misconfiguration explicitly warned against in the documentation +- Automated scanner output with no demonstrated impact +- Denial of service through sheer traffic volume against an under-provisioned deployment + +## A note on defaults + +This is a WAF, so its shipped defaults are part of its security posture. Reports +about defaults that weaken protection are in scope and welcome — a previous +release seeded an IP allowlist covering every RFC1918 range, which disabled +inspection entirely in the topology the product targets. That class of issue +matters as much as a code-level bug. + +## Disclosure + +We aim to publish an advisory once a fix is available, crediting the reporter +unless anonymity is requested. If you intend to publish independently, please +give us a reasonable window to ship a fix first. diff --git a/scripts/test-waf.sh b/scripts/test-waf.sh index c5fa200..fd248b9 100755 --- a/scripts/test-waf.sh +++ b/scripts/test-waf.sh @@ -549,17 +549,19 @@ if echo "$WP_RESPONSE" | grep -q '"endpoint":"wp-login"' || [ "$WP_PROBE_STATUS" sleep 1 - # Test 6: Common username "admin" should be blocked (from builtin_credential_stuffing signature) - test_request "WP Login - Credential stuffing (admin)" "403" "POST" "/wp-login.php" \ - -A "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36" \ - -d "log=admin&pwd=somepassword" - - sleep 1 - - # Test 7: Common password should be blocked - test_request "WP Login - Credential stuffing (password123)" "403" "POST" "/wp-login.php" \ - -A "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36" \ - -d "log=someuser&pwd=password123" + # KNOWN GAP: these asserted that a single common-credential POST returns 403, + # and they passed -- but for the wrong reason. Bare curl sends a Chrome + # User-Agent with none of Chrome's headers, so the fake-modern-browser check + # scored them, not credential-stuffing detection. Given a realistic browser + # client both requests are allowed. + # + # Credential stuffing is by definition automated and repetitive, so the honest + # assertion is that a burst of common-credential attempts from one source is + # blocked while a single attempt from a real browser is not. That needs a + # product decision on where the line sits, so the assertions are parked rather + # than rewritten to match whatever the code happens to do today. + log_known_gap "WP Login - Credential stuffing (admin) - assertion passed for the wrong reason; needs a burst-based test" + log_known_gap "WP Login - Credential stuffing (password123) - assertion passed for the wrong reason; needs a burst-based test" else log_info "WP Login endpoint not configured - skipping defense line tests" From 1b90fb342caf17c5e730ecdd8cca1fdffa2e8fdd Mon Sep 17 00:00:00 2001 From: Martin Dobrev Date: Fri, 7 Aug 2026 14:32:39 +0100 Subject: [PATCH 2/6] test: Stop attributing every known gap to R-27 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. --- scripts/test-waf.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/test-waf.sh b/scripts/test-waf.sh index fd248b9..5981663 100755 --- a/scripts/test-waf.sh +++ b/scripts/test-waf.sh @@ -595,7 +595,7 @@ echo "========================================" echo "Test Summary" echo "========================================" echo -e "Passed: ${GREEN}$PASS${NC}" -echo -e "Known gaps: ${YELLOW}${KNOWN_GAPS}${NC} (see R-27)" +echo -e "Known gaps: ${YELLOW}${KNOWN_GAPS}${NC} (assertions parked deliberately - see the [KNOWN GAP] lines above)" echo -e "Failed: ${RED}$FAIL${NC}" echo "" From 96322fce0974759d641ae7d0c6f2bba8ca147af7 Mon Sep 17 00:00:00 2001 From: Martin Dobrev Date: Fri, 7 Aug 2026 14:39:38 +0100 Subject: [PATCH 3/6] feat: Record shadow-mode would-block decisions with attribution (#C1) 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. --- openresty/conf/nginx.conf | 1 + openresty/lua/redis_sync.lua | 13 ++ openresty/lua/shadow_recorder.lua | 186 ++++++++++++++++++++++++ openresty/lua/waf_handler.lua | 29 ++++ openresty/spec/shadow_recorder_spec.lua | 132 +++++++++++++++++ 5 files changed, 361 insertions(+) create mode 100644 openresty/lua/shadow_recorder.lua create mode 100644 openresty/spec/shadow_recorder_spec.lua diff --git a/openresty/conf/nginx.conf b/openresty/conf/nginx.conf index f804f9e..45892ec 100644 --- a/openresty/conf/nginx.conf +++ b/openresty/conf/nginx.conf @@ -135,6 +135,7 @@ http { lua_shared_dict waf_metrics 10m; # WAF metrics counters (per vhost/endpoint) lua_shared_dict waf_timing 1m; # Timing token encryption keys lua_shared_dict coordinator_cache 1m; # Instance coordinator state + lua_shared_dict shadow_cache 10m; # Shadow-mode would-block buffer (flushed to Redis by a timer) # Initialize modules on worker start init_worker_by_lua_block { diff --git a/openresty/lua/redis_sync.lua b/openresty/lua/redis_sync.lua index d28df1a..d0ee1e6 100644 --- a/openresty/lua/redis_sync.lua +++ b/openresty/lua/redis_sync.lua @@ -1155,6 +1155,19 @@ local function do_sync() -- Sync Slack notification configuration sync_slack(red) + -- Drain the shadow-mode buffer here rather than on its own timer: this + -- function already holds an open connection and runs on a predictable + -- interval. + local ok_shadow, shadow_recorder = pcall(require, "shadow_recorder") + if ok_shadow and shadow_recorder then + local ok_flush, flushed = pcall(shadow_recorder.flush, red) + if not ok_flush then + ngx.log(ngx.ERR, "shadow recorder flush failed: ", tostring(flushed)) + elseif flushed > 0 then + ngx.log(ngx.INFO, "shadow recorder: flushed ", flushed, " would-block decisions") + end + end + close_redis(red) ngx.log(ngx.INFO, "Redis sync completed") diff --git a/openresty/lua/shadow_recorder.lua b/openresty/lua/shadow_recorder.lua new file mode 100644 index 0000000..93c6dca --- /dev/null +++ b/openresty/lua/shadow_recorder.lua @@ -0,0 +1,186 @@ +--[[ + Shadow mode recorder + ==================== + Monitoring mode already decides what it *would* have blocked -- waf_handler + computes is_monitoring_would_block, writes a line to the error log and bumps a + counter. Then it throws the detail away, so the only way to answer "what + happens if I turn blocking on?" is to grep logs. + + This keeps that decision, with attribution, so the answer becomes a query. + + Two constraints shape the design, both learned from defects in this codebase: + + * Nothing in the request path may read or write Redis (R-04). Recording + therefore writes to a shared dict and a timer flushes it. + * One timer per request is how you exhaust lua_max_running_timers and + starve redis_sync (R-03). A single periodic flush is used instead, the + same shape field_learner already uses for its batching. + + Storage is bounded on purpose. A WAF in monitoring mode on a busy site sees + everything, and an unbounded recorder becomes its own outage. +]] + +local cjson = require "cjson.safe" + +local _M = {} + +local shadow_cache = ngx.shared.shadow_cache + +-- Redis keys +local KEYS = { + decisions = "waf:shadow:decisions", -- capped list of recent records + rules = "waf:shadow:rules", -- HASH rule -> count + endpoints = "waf:shadow:endpoints", -- HASH vhost|endpoint -> count + stats = "waf:shadow:stats", -- HASH totals +} + +-- Bounds. These are deliberate ceilings, not tuning knobs to raise casually: +-- every one of them is what stops a busy site's monitoring mode from becoming an +-- incident of its own. +local MAX_BUFFERED = 500 -- records held in the shared dict between flushes +local MAX_DECISIONS = 2000 -- records retained in Redis for the detail view +local DECISION_TTL = 7 * 24 * 3600 +local MAX_FLAG_LENGTH = 120 +local MAX_PATH_LENGTH = 256 + +local BUFFER_HEAD = "shadow:head" -- next write slot +local BUFFER_TAIL = "shadow:tail" -- next slot to flush +local DROPPED = "shadow:dropped" + +--- Trim a value that originates from the request, so a long or hostile field +--- cannot bloat storage. +local function clip(value, limit) + if value == nil then return nil end + value = tostring(value) + if #value > limit then + return value:sub(1, limit) .. "..." + end + return value +end + +--- Record one would-block decision. Called from the request path, so this must +--- stay allocation-light and must never touch Redis. +-- @param decision table vhost_id, endpoint_id, client_ip, host, path, method, +-- score, blocked_by (array), flags (array) +function _M.record(decision) + if not shadow_cache or type(decision) ~= "table" then + return false + end + + local head = shadow_cache:incr(BUFFER_HEAD, 1, 0) + if not head then + return false + end + + local tail = shadow_cache:get(BUFFER_TAIL) or 0 + if head - tail > MAX_BUFFERED then + -- Buffer is full: drop this record rather than evicting an older one, and + -- count the loss so the UI can say "showing a sample" instead of implying + -- completeness. + shadow_cache:incr(DROPPED, 1, 0) + return false, "buffer full" + end + + local record = cjson.encode({ + ts = ngx.time(), + vhost_id = decision.vhost_id or "unknown", + endpoint_id = decision.endpoint_id or "global", + client_ip = decision.client_ip, + host = clip(decision.host, MAX_PATH_LENGTH), + path = clip(decision.path, MAX_PATH_LENGTH), + method = decision.method, + score = decision.score or 0, + blocked_by = decision.blocked_by, + flags = decision.flags, + }) + if not record then + return false + end + + -- Slots expire well after a flush would have consumed them, so a stalled + -- flush cannot pin memory indefinitely. + shadow_cache:set("shadow:rec:" .. head, record, 600) + return true +end + +--- Drain the buffer into Redis. Timer context only. +-- @param red an open Redis connection +-- @return number of records flushed +function _M.flush(red) + if not shadow_cache or not red then + return 0 + end + + local head = shadow_cache:get(BUFFER_HEAD) or 0 + local tail = shadow_cache:get(BUFFER_TAIL) or 0 + if head <= tail then + return 0 + end + + -- redis_sync's timer runs on EVERY worker, so several of them reach this at + -- the same moment. Reading the tail, draining, then writing the tail back + -- lets each worker drain the same slots -- three workers turned one + -- would-block decision into three recorded copies, which would have + -- overstated the impact of promoting an endpoint by 3x. + -- + -- incr is atomic on a shared dict, so claim the range first and let each + -- worker own a disjoint slice. Whoever claims nothing does nothing. + 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 + + local flushed = 0 + for slot = claimed_start, claimed_end do + local key = "shadow:rec:" .. slot + local record = shadow_cache:get(key) + if record then + local decoded = cjson.decode(record) + if decoded then + red:lpush(KEYS.decisions, record) + + local scope = (decoded.vhost_id or "unknown") .. "|" .. (decoded.endpoint_id or "global") + red:hincrby(KEYS.endpoints, scope, 1) + red:hincrby(KEYS.stats, "would_block_total", 1) + + for _, rule in ipairs(decoded.blocked_by or {}) do + red:hincrby(KEYS.rules, clip(rule, MAX_FLAG_LENGTH), 1) + end + flushed = flushed + 1 + end + shadow_cache:delete(key) + end + end + + if flushed > 0 then + red:ltrim(KEYS.decisions, 0, MAX_DECISIONS - 1) + red:expire(KEYS.decisions, DECISION_TTL) + red:expire(KEYS.rules, DECISION_TTL) + red:expire(KEYS.endpoints, DECISION_TTL) + red:expire(KEYS.stats, DECISION_TTL) + end + + 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 +end + +function _M.get_keys() + return KEYS +end + +--- Exposed for tests and for the API's "is anything buffered" check. +function _M.buffer_depth() + if not shadow_cache then return 0 end + local head = shadow_cache:get(BUFFER_HEAD) or 0 + local tail = shadow_cache:get(BUFFER_TAIL) or 0 + return math.max(0, head - tail) +end + +return _M diff --git a/openresty/lua/waf_handler.lua b/openresty/lua/waf_handler.lua index 4df5d52..912e9f1 100644 --- a/openresty/lua/waf_handler.lua +++ b/openresty/lua/waf_handler.lua @@ -47,6 +47,33 @@ end -- F08: HMAC key for log integrity (optional) local LOG_HMAC_KEY = os.getenv("WAF_LOG_HMAC_KEY") +-- Keep a would-block decision so "what happens if I switch this to blocking?" +-- becomes a query rather than a log grep. +-- +-- There are two would-block branches in process_request and both must record. +-- Capturing only one would silently understate the impact of promoting an +-- endpoint, which is the one thing this feature exists to get right. +-- +-- 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 + -- Structured audit logging for security events -- Outputs JSON formatted log entries for easy parsing by log aggregation tools -- F08: Optionally adds HMAC signature for log integrity verification @@ -732,6 +759,7 @@ function _M.process_request() profile_result.flags and table.concat(profile_result.flags, ",") or "none" )) metrics.record_request(summary.vhost_id, summary.endpoint_id, "monitored", profile_result.score or 0) + record_shadow_decision(summary, client_ip, host, path, method, profile_result) -- Return to continue to proxy_pass - HAProxy will use the request headers we set return end @@ -826,6 +854,7 @@ function _M.process_request() profile_result.score or 0 )) metrics.record_request(summary.vhost_id, summary.endpoint_id, "monitored", profile_result.score or 0) + record_shadow_decision(summary, client_ip, host, path, method, profile_result) else metrics.record_request(summary.vhost_id, summary.endpoint_id, "allowed", profile_result.score or 0) end diff --git a/openresty/spec/shadow_recorder_spec.lua b/openresty/spec/shadow_recorder_spec.lua new file mode 100644 index 0000000..34e9f13 --- /dev/null +++ b/openresty/spec/shadow_recorder_spec.lua @@ -0,0 +1,132 @@ +--[[ + The recorder buffers would-block decisions in a shared dict and a timer + drains them. Two properties matter more than the rest: + + * The drain is claimed atomically. redis_sync's timer runs on every + worker, and this box has 24 of them. Reading the tail, draining, then + writing it back let every worker drain the same slots -- one decision + became one record per worker, which would have overstated the impact of + promoting an endpoint by up to 24x. That was a real defect, caught by + counting records rather than trusting the code. + + * The buffer is bounded and says so. A WAF in monitoring mode on a busy + site sees everything; silently dropping records would make the diff view + claim completeness it does not have. +]] +local helper = require "spec_helper" + +describe("shadow_recorder", function() + local recorder + + -- Minimal Redis double that records what would have been written. + local function fake_redis() + local calls = { lpush = {}, hincrby = {}, ltrim = 0, expire = 0 } + return { + calls = calls, + lpush = function(_, _, v) table.insert(calls.lpush, v); return 1 end, + hincrby = function(_, key, field, n) + calls.hincrby[key .. "/" .. field] = (calls.hincrby[key .. "/" .. field] or 0) + n + return 1 + end, + ltrim = function() calls.ltrim = calls.ltrim + 1; return true end, + expire = function() calls.expire = calls.expire + 1; return true end, + } + end + + local function a_decision(overrides) + local d = { + vhost_id = "example-com", endpoint_id = "contact", + client_ip = "203.0.113.9", host = "example.com", path = "/contact", + method = "POST", score = 85, + blocked_by = { "keyword_filter" }, flags = { "kw:viagra" }, + } + for k, v in pairs(overrides or {}) do d[k] = v end + return d + end + + before_each(function() + helper.install_ngx() + helper.stub_external_modules() + package.loaded["shadow_recorder"] = nil + recorder = require "shadow_recorder" + end) + + it("buffers a decision without touching Redis", function() + assert.is_true(recorder.record(a_decision())) + assert.equals(1, recorder.buffer_depth()) + end) + + it("ignores anything that is not a decision table", function() + assert.is_false(recorder.record(nil)) + assert.is_false(recorder.record("not a table")) + assert.equals(0, recorder.buffer_depth()) + end) + + it("flushes buffered decisions to Redis and empties the buffer", function() + for i = 1, 3 do recorder.record(a_decision({ score = 80 + i })) end + local red = fake_redis() + assert.equals(3, recorder.flush(red)) + assert.equals(3, #red.calls.lpush) + assert.equals(0, recorder.buffer_depth()) + end) + + it("does not re-flush what a previous drain already claimed", function() + -- The defect this guards: a second drain seeing the same slots. + for i = 1, 4 do recorder.record(a_decision()) end + local first, second = fake_redis(), fake_redis() + assert.equals(4, recorder.flush(first)) + assert.equals(0, recorder.flush(second)) + assert.equals(4, #first.calls.lpush) + assert.equals(0, #second.calls.lpush) + end) + + it("claims the range before draining, so a concurrent drain finds nothing", function() + for i = 1, 5 do recorder.record(a_decision()) end + local depth_before = recorder.buffer_depth() + assert.equals(5, depth_before) + -- A worker that claims first leaves nothing for the next to claim. + recorder.flush(fake_redis()) + assert.equals(0, recorder.buffer_depth(), + "the claim must advance the tail so another worker drains nothing") + end) + + it("flushing an empty buffer is a no-op", function() + local red = fake_redis() + assert.equals(0, recorder.flush(red)) + assert.equals(0, #red.calls.lpush) + assert.equals(0, red.calls.ltrim) + end) + + it("counts each rule that contributed to the decision", function() + recorder.record(a_decision({ blocked_by = { "keyword_filter", "honeypot" } })) + local red = fake_redis() + recorder.flush(red) + assert.equals(1, red.calls.hincrby["waf:shadow:rules/keyword_filter"]) + assert.equals(1, red.calls.hincrby["waf:shadow:rules/honeypot"]) + end) + + it("aggregates by vhost and endpoint", function() + recorder.record(a_decision()) + recorder.record(a_decision({ endpoint_id = "signup" })) + local red = fake_redis() + recorder.flush(red) + assert.equals(1, red.calls.hincrby["waf:shadow:endpoints/example-com|contact"]) + assert.equals(1, red.calls.hincrby["waf:shadow:endpoints/example-com|signup"]) + end) + + it("clips attacker-controlled values so one request cannot bloat storage", function() + recorder.record(a_decision({ path = string.rep("A", 5000) })) + local red = fake_redis() + recorder.flush(red) + assert.is_true(#red.calls.lpush[1] < 2000, + "a 5000-character path must not be stored whole") + end) + + it("drops rather than growing without bound, and counts what it dropped", function() + for _ = 1, 600 do recorder.record(a_decision()) end -- cap is 500 + local red = fake_redis() + recorder.flush(red) + assert.is_true(red.calls.hincrby["waf:shadow:stats/dropped_total"] > 0, + "dropped records must be counted so the UI can say it is showing a sample") + end) +end) From 6e76aac46d9274d7f36bff3c78f530112ca618cd Mon Sep 17 00:00:00 2001 From: Martin Dobrev Date: Fri, 7 Aug 2026 14:52:20 +0100 Subject: [PATCH 4/6] feat: Add shadow mode Admin API and promote flow 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. --- docs/API_HANDLERS.md | 19 ++ docs/openapi.yaml | 115 ++++++++++ openresty/lua/admin_api.lua | 2 + openresty/lua/api_handlers/shadow.lua | 275 ++++++++++++++++++++++++ openresty/lua/rbac.lua | 10 + openresty/lua/shadow_recorder.lua | 12 +- openresty/spec/shadow_recorder_spec.lua | 21 ++ scripts/check-api-contract.py | 6 + 8 files changed, 459 insertions(+), 1 deletion(-) create mode 100644 openresty/lua/api_handlers/shadow.lua diff --git a/docs/API_HANDLERS.md b/docs/API_HANDLERS.md index f53e2c9..4fc48be 100644 --- a/docs/API_HANDLERS.md +++ b/docs/API_HANDLERS.md @@ -335,6 +335,24 @@ local ok, err = utils.validate_required(data, {"field1", "field2"}) --- +### Shadow Mode (`api_handlers/shadow.lua`) + +What monitoring mode *would* have blocked, so a rule set can be proven before it +starts rejecting traffic. `shadow_recorder.lua` buffers decisions in a shared +dict; `redis_sync` drains them. + +| Method | Endpoint | Description | +|--------|----------|-------------| +| GET | /shadow/summary | Would-block totals, top rules, top flags, per-scope counts | +| GET | /shadow/decisions | Individual decisions, newest first (`limit`, `vhost_id`, `endpoint_id`) | +| GET | /shadow/impact | Pre-flight impact of promoting one scope (`vhost_id` required) | +| POST | /shadow/promote | Switch a vhost from monitoring to blocking | +| DELETE | /shadow/decisions | Discard the recorded sample | + +`dropped_total` is non-zero when the recorder's buffer overflowed; the sample is +then incomplete and every count understates reality. `sample_incomplete` on the +impact response carries the same warning. + ## RBAC Permission Mapping Each endpoint requires specific permissions. See [RBAC Guide](RBAC.md) for full details. @@ -353,6 +371,7 @@ Each endpoint requires specific permissions. See [RBAC Guide](RBAC.md) for full | captcha | read, update | | security | read, update | | slack | read, update, test | +| shadow | read, promote, delete | --- diff --git a/docs/openapi.yaml b/docs/openapi.yaml index 3309cb4..ad98122 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -113,6 +113,73 @@ components: high_event_count: { type: integer } high_event_rate: { type: integer } + ShadowDecision: + type: object + required: [ts, vhost_id, endpoint_id, score] + properties: + ts: { type: integer, description: Unix time the decision was made } + vhost_id: { type: string } + endpoint_id: { type: string } + client_ip: { type: string } + host: { type: string } + path: { type: string } + method: { type: string } + score: { type: integer } + blocked_by: { type: array, items: { type: string } } + flags: { type: array, items: { type: string } } + + ShadowCount: + type: object + required: [name, count] + properties: + name: { type: string } + count: { type: integer } + + ShadowSummary: + type: object + required: [would_block_total, dropped_total, retained_decisions, top_rules, top_flags, scopes] + properties: + would_block_total: { type: integer } + dropped_total: + type: integer + description: > + Records lost to the recorder's buffer cap. Non-zero means the sample + is incomplete and the counts understate reality. + retained_decisions: { type: integer } + top_rules: { type: array, items: { $ref: "#/components/schemas/ShadowCount" } } + top_flags: + type: array + description: Detection-level attribution; this is what identifies a rule to suppress. + items: { $ref: "#/components/schemas/ShadowCount" } + scopes: + type: array + items: + type: object + required: [vhost_id, endpoint_id, would_block_count] + properties: + vhost_id: { type: string } + endpoint_id: { type: string } + would_block_count: { type: integer } + + ShadowImpact: + type: object + required: [vhost_id, would_block_count, unique_client_ips, average_score, sample_incomplete] + properties: + vhost_id: { type: string } + endpoint_id: { type: string } + would_block_count: { type: integer } + unique_client_ips: { type: integer } + average_score: { type: integer } + window_start: { type: integer } + window_end: { type: integer } + sample_incomplete: + type: boolean + description: True when records were dropped, so this understates the impact. + dropped_total: { type: integer } + top_rules: { type: array, items: { $ref: "#/components/schemas/ShadowCount" } } + top_flags: { type: array, items: { $ref: "#/components/schemas/ShadowCount" } } + affected_endpoints: { type: array, items: { $ref: "#/components/schemas/ShadowCount" } } + WafStatus: type: object required: [redis_connected, blocked_hashes_count, whitelisted_ips_count, endpoints_count, vhosts_count] @@ -197,6 +264,54 @@ paths: emitted_events: { type: array, items: { type: string } } unavailable_events: { type: array, items: { type: string } } + /shadow/summary: + get: + summary: What monitoring mode would have blocked + responses: + "200": + description: Aggregate view over the retained window + content: + application/json: + schema: { $ref: "#/components/schemas/ShadowSummary" } + + /shadow/decisions: + get: + summary: Individual would-block decisions + responses: + "200": + description: Recent decisions, newest first + content: + application/json: + schema: + type: object + required: [decisions, count] + properties: + decisions: + type: array + items: { $ref: "#/components/schemas/ShadowDecision" } + count: { type: integer } + limit: { type: integer } + + /shadow/impact: + get: + summary: Pre-flight impact of promoting a scope to blocking + x-contract-example-query: "vhost_id=_default" + parameters: + - name: vhost_id + in: query + required: true + schema: { type: string } + - name: endpoint_id + in: query + required: false + schema: { type: string } + responses: + "200": + description: What promoting this scope would have blocked + content: + application/json: + schema: { $ref: "#/components/schemas/ShadowImpact" } + /keywords/blocked: get: summary: Blocked keywords diff --git a/openresty/lua/admin_api.lua b/openresty/lua/admin_api.lua index 5fc19ae..5aa4e31 100644 --- a/openresty/lua/admin_api.lua +++ b/openresty/lua/admin_api.lua @@ -19,6 +19,7 @@ local keywords_handler = require "api_handlers.keywords" local config_handler = require "api_handlers.config" local webhooks_handler = require "api_handlers.webhooks" local slack_handler = require "api_handlers.slack" +local shadow_handler = require "api_handlers.shadow" local geoip_handler = require "api_handlers.geoip" local reputation_handler = require "api_handlers.reputation" local bulk_handler = require "api_handlers.bulk" @@ -68,6 +69,7 @@ register_handlers(keywords_handler) register_handlers(config_handler) register_handlers(webhooks_handler) register_handlers(slack_handler) +register_handlers(shadow_handler) register_handlers(geoip_handler) register_handlers(reputation_handler) register_handlers(bulk_handler) diff --git a/openresty/lua/api_handlers/shadow.lua b/openresty/lua/api_handlers/shadow.lua new file mode 100644 index 0000000..654d8d0 --- /dev/null +++ b/openresty/lua/api_handlers/shadow.lua @@ -0,0 +1,275 @@ +-- api_handlers/shadow.lua +-- Shadow mode: what monitoring mode would have blocked, and promoting a scope +-- to blocking once you can see it. + +local _M = {} + +local utils = require "api_handlers.utils" +local cjson = require "cjson.safe" +local redis_sync = require "redis_sync" + +local KEYS = { + decisions = "waf:shadow:decisions", + rules = "waf:shadow:rules", + flags = "waf:shadow:flags", + endpoints = "waf:shadow:endpoints", + stats = "waf:shadow:stats", +} + +local DEFAULT_LIMIT = 100 +local MAX_LIMIT = 500 + +-- An empty Lua table encodes as a JSON object. The UI treats these as arrays and +-- calls .map on them, so force array encoding (same defect as the Slack handler). +local function as_array(t) + if type(t) ~= "table" then + return setmetatable({}, cjson.array_mt) + end + return setmetatable(t, cjson.array_mt) +end + +local function redis_hash_to_table(raw) + local out = {} + if type(raw) ~= "table" then return out end + for i = 1, #raw, 2 do + out[raw[i]] = tonumber(raw[i + 1]) or 0 + end + return out +end + +--- Sort a name->count map into a descending array, capped. +local function top_n(counts, limit) + local list = {} + for name, count in pairs(counts) do + table.insert(list, { name = name, count = count }) + end + table.sort(list, function(a, b) + if a.count == b.count then return a.name < b.name end + return a.count > b.count + end) + while #list > (limit or 10) do table.remove(list) end + return as_array(list) +end + +local function query_arg(name, default) + local args = ngx.req.get_uri_args() + local value = args[name] + if type(value) == "table" then value = value[1] end + if value == nil or value == "" then return default end + return value +end + +--- Decode the stored decision list, optionally narrowed to one scope. +local function read_decisions(red, vhost_id, endpoint_id, limit) + local raw = red:lrange(KEYS.decisions, 0, MAX_LIMIT - 1) + local out = {} + if type(raw) ~= "table" then return out end + + for _, entry in ipairs(raw) do + local decision = cjson.decode(entry) + if decision then + local matches = true + if vhost_id and decision.vhost_id ~= vhost_id then matches = false end + if endpoint_id and decision.endpoint_id ~= endpoint_id then matches = false end + if matches then + table.insert(out, decision) + if #out >= limit then break end + end + end + end + return out +end + +_M.handlers = {} + +-- GET /shadow/summary - what monitoring mode has withheld so far +_M.handlers["GET:/shadow/summary"] = function() + local red, err = utils.get_redis() + if not red then + return utils.error_response("Redis connection failed: " .. (err or "unknown")) + end + + local stats = redis_hash_to_table(red:hgetall(KEYS.stats)) + local rules = redis_hash_to_table(red:hgetall(KEYS.rules)) + local flags = redis_hash_to_table(red:hgetall(KEYS.flags)) + local endpoints = redis_hash_to_table(red:hgetall(KEYS.endpoints)) + local retained = red:llen(KEYS.decisions) + utils.close_redis(red) + + -- Split the "vhost|endpoint" aggregate key back into its parts so the UI does + -- not have to know the encoding. + local scopes = {} + for scope, count in pairs(endpoints) do + local vhost_id, endpoint_id = scope:match("^(.-)|(.*)$") + table.insert(scopes, { + vhost_id = vhost_id or scope, + endpoint_id = endpoint_id or "global", + would_block_count = count, + }) + end + table.sort(scopes, function(a, b) return a.would_block_count > b.would_block_count end) + + return utils.json_response({ + would_block_total = stats.would_block_total or 0, + -- Non-zero means the buffer overflowed and the sample is incomplete. The + -- UI must say so rather than presenting the counts as exhaustive. + dropped_total = stats.dropped_total or 0, + retained_decisions = tonumber(retained) or 0, + top_rules = top_n(rules, 10), + -- The actionable breakdown: which detections drove the would-blocks. + top_flags = top_n(flags, 15), + scopes = as_array(scopes), + }) +end + +-- GET /shadow/decisions - the individual records behind the summary +_M.handlers["GET:/shadow/decisions"] = function() + 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") + + local red, err = utils.get_redis() + if not red then + return utils.error_response("Redis connection failed: " .. (err or "unknown")) + end + local decisions = read_decisions(red, vhost_id, endpoint_id, limit) + utils.close_redis(red) + + return utils.json_response({ + decisions = as_array(decisions), + count = #decisions, + limit = limit, + }) +end + +-- GET /shadow/impact - the pre-flight answer to "what happens if I promote this?" +_M.handlers["GET:/shadow/impact"] = function() + local vhost_id = query_arg("vhost_id") + if not vhost_id then + return utils.error_response("vhost_id is required") + end + local endpoint_id = query_arg("endpoint_id") + + local red, err = utils.get_redis() + if not red then + return utils.error_response("Redis connection failed: " .. (err or "unknown")) + end + + local decisions = read_decisions(red, vhost_id, endpoint_id, MAX_LIMIT) + local stats = redis_hash_to_table(red:hgetall(KEYS.stats)) + utils.close_redis(red) + + local rules, flags, ips, endpoints_hit = {}, {}, {}, {} + local score_total, oldest, newest = 0, nil, nil + for _, d in ipairs(decisions) do + for _, rule in ipairs(d.blocked_by or {}) do + rules[rule] = (rules[rule] or 0) + 1 + end + for _, flag in ipairs(d.flags or {}) do + flags[flag] = (flags[flag] or 0) + 1 + end + if d.client_ip then ips[d.client_ip] = true end + local ep = d.endpoint_id or "global" + endpoints_hit[ep] = (endpoints_hit[ep] or 0) + 1 + score_total = score_total + (d.score or 0) + if not oldest or (d.ts or 0) < oldest then oldest = d.ts end + if not newest or (d.ts or 0) > newest then newest = d.ts end + end + + local unique_ips = 0 + for _ in pairs(ips) do unique_ips = unique_ips + 1 end + + return utils.json_response({ + vhost_id = vhost_id, + endpoint_id = endpoint_id, + -- What promoting this scope would have blocked, over the retained window. + would_block_count = #decisions, + unique_client_ips = unique_ips, + average_score = (#decisions > 0) and math.floor(score_total / #decisions) or 0, + window_start = oldest, + window_end = newest, + top_rules = top_n(rules, 10), + top_flags = top_n(flags, 15), + affected_endpoints = top_n(endpoints_hit, 20), + -- Carried through so the caller cannot mistake a truncated sample for the + -- full picture when deciding to promote. + sample_incomplete = (stats.dropped_total or 0) > 0, + dropped_total = stats.dropped_total or 0, + }) +end + +-- POST /shadow/promote - switch a vhost from monitoring to blocking +_M.handlers["POST:/shadow/promote"] = function() + local data, err = utils.get_json_body() + if not data then + return utils.error_response(err or "Invalid JSON") + end + local ok, verr = utils.validate_required(data, { "vhost_id" }) + if not ok then + return utils.error_response(verr) + end + + local red, rerr = utils.get_redis() + if not red then + return utils.error_response("Redis connection failed: " .. (rerr or "unknown")) + end + + local key = "waf:vhosts:config:" .. data.vhost_id + local raw = red:get(key) + if not raw or raw == ngx.null then + utils.close_redis(red) + return utils.error_response("Virtual host not found: " .. data.vhost_id, 404) + end + + local config = cjson.decode(raw) + if not config then + utils.close_redis(red) + return utils.error_response("Stored vhost configuration is not valid JSON", 500) + end + + config.waf = config.waf or {} + local previous_mode = config.waf.mode + if previous_mode == "blocking" then + utils.close_redis(red) + return utils.json_response({ + promoted = false, + vhost_id = data.vhost_id, + mode = "blocking", + message = "Already in blocking mode; nothing to do", + }) + end + + config.waf.mode = "blocking" + local saved, serr = red:set(key, cjson.encode(config)) + utils.close_redis(red) + if not saved then + return utils.error_response("Failed to save configuration: " .. (serr or "unknown")) + end + + redis_sync.sync_now() + + ngx.log(ngx.WARN, string.format( + "SHADOW PROMOTE: vhost=%s mode %s -> blocking by user=%s", + data.vhost_id, tostring(previous_mode), + (ngx.ctx.admin_user and ngx.ctx.admin_user.username) or "unknown")) + + return utils.json_response({ + promoted = true, + vhost_id = data.vhost_id, + previous_mode = previous_mode, + mode = "blocking", + }) +end + +-- DELETE /shadow/decisions - reset the observation window +_M.handlers["DELETE:/shadow/decisions"] = function() + local red, err = utils.get_redis() + if not red then + return utils.error_response("Redis connection failed: " .. (err or "unknown")) + end + red:del(KEYS.decisions, KEYS.rules, KEYS.flags, KEYS.endpoints, KEYS.stats) + utils.close_redis(red) + return utils.json_response({ cleared = true }) +end + +return _M diff --git a/openresty/lua/rbac.lua b/openresty/lua/rbac.lua index 0794158..e1475dd 100644 --- a/openresty/lua/rbac.lua +++ b/openresty/lua/rbac.lua @@ -37,6 +37,7 @@ local DEFAULT_ROLES = { captcha = {"create", "read", "update", "delete", "enable", "disable", "test"}, webhooks = {"read", "update", "test"}, slack = {"read", "update", "test", "reset"}, + shadow = {"read", "promote", "delete"}, geoip = {"read", "update", "reload"}, reputation = {"read", "update"}, timing = {"read", "update"}, @@ -68,6 +69,7 @@ local DEFAULT_ROLES = { captcha = {"read"}, webhooks = {"read"}, slack = {"read"}, + shadow = {"read"}, geoip = {"read"}, reputation = {"read"}, timing = {"read"}, @@ -96,6 +98,7 @@ local DEFAULT_ROLES = { captcha = {"read"}, webhooks = {"read"}, slack = {"read"}, + shadow = {"read"}, geoip = {"read"}, reputation = {"read"}, timing = {"read"}, @@ -183,6 +186,13 @@ local ENDPOINT_PERMISSIONS = { -- Timing (per-vhost listing) ["GET:/timing/vhosts"] = {resource = "timing", action = "read"}, + -- Shadow mode (what monitoring would have blocked, and promoting a scope) + ["GET:/shadow/summary"] = {resource = "shadow", action = "read"}, + ["GET:/shadow/decisions"] = {resource = "shadow", action = "read"}, + ["GET:/shadow/impact"] = {resource = "shadow", action = "read"}, + ["POST:/shadow/promote"] = {resource = "shadow", action = "promote"}, + ["DELETE:/shadow/decisions"] = {resource = "shadow", action = "delete"}, + -- Slack notifications ["GET:/slack/config"] = {resource = "slack", action = "read"}, ["PUT:/slack/config"] = {resource = "slack", action = "update"}, diff --git a/openresty/lua/shadow_recorder.lua b/openresty/lua/shadow_recorder.lua index 93c6dca..01ca40e 100644 --- a/openresty/lua/shadow_recorder.lua +++ b/openresty/lua/shadow_recorder.lua @@ -29,7 +29,8 @@ local shadow_cache = ngx.shared.shadow_cache -- Redis keys local KEYS = { decisions = "waf:shadow:decisions", -- capped list of recent records - rules = "waf:shadow:rules", -- HASH rule -> count + rules = "waf:shadow:rules", -- HASH profile/rule -> count + flags = "waf:shadow:flags", -- HASH detection flag -> count endpoints = "waf:shadow:endpoints", -- HASH vhost|endpoint -> count stats = "waf:shadow:stats", -- HASH totals } @@ -148,6 +149,14 @@ function _M.flush(red) for _, rule in ipairs(decoded.blocked_by or {}) do red:hincrby(KEYS.rules, clip(rule, MAX_FLAG_LENGTH), 1) end + + -- blocked_by names the profile ("legacy"), which tells an + -- operator nothing about what to suppress. The flags carry the + -- actual reason -- kw:viagra, fp_flag:suspicious-bot -- so count + -- those too; they are what the diff view is for. + for _, flag in ipairs(decoded.flags or {}) do + red:hincrby(KEYS.flags, clip(flag, MAX_FLAG_LENGTH), 1) + end flushed = flushed + 1 end shadow_cache:delete(key) @@ -158,6 +167,7 @@ function _M.flush(red) red:ltrim(KEYS.decisions, 0, MAX_DECISIONS - 1) red:expire(KEYS.decisions, DECISION_TTL) red:expire(KEYS.rules, DECISION_TTL) + red:expire(KEYS.flags, DECISION_TTL) red:expire(KEYS.endpoints, DECISION_TTL) red:expire(KEYS.stats, DECISION_TTL) end diff --git a/openresty/spec/shadow_recorder_spec.lua b/openresty/spec/shadow_recorder_spec.lua index 34e9f13..d40a2ea 100644 --- a/openresty/spec/shadow_recorder_spec.lua +++ b/openresty/spec/shadow_recorder_spec.lua @@ -114,6 +114,27 @@ describe("shadow_recorder", function() assert.equals(1, red.calls.hincrby["waf:shadow:endpoints/example-com|signup"]) end) + it("counts detection flags, which are what name the rule to suppress", function() + recorder.record(a_decision({ flags = { "legacy:kw:viagra", "legacy:fp_flag:bot" } })) + recorder.record(a_decision({ flags = { "legacy:kw:viagra" } })) + local red = fake_redis() + recorder.flush(red) + -- "legacy" as a rule name tells an operator nothing; the flag does. + assert.equals(2, red.calls.hincrby["waf:shadow:flags/legacy:kw:viagra"]) + assert.equals(1, red.calls.hincrby["waf:shadow:flags/legacy:fp_flag:bot"]) + end) + + it("clips a flag, since part of it comes from the request", function() + recorder.record(a_decision({ flags = { "legacy:kw:" .. string.rep("A", 900) } })) + local red = fake_redis() + recorder.flush(red) + for field in pairs(red.calls.hincrby) do + if field:find("^waf:shadow:flags/") then + assert.is_true(#field < 300, "an unclipped flag would let one request bloat the hash") + end + end + end) + it("clips attacker-controlled values so one request cannot bloat storage", function() recorder.record(a_decision({ path = string.rep("A", 5000) })) local red = fake_redis() diff --git a/scripts/check-api-contract.py b/scripts/check-api-contract.py index 42107d5..25e6e7e 100755 --- a/scripts/check-api-contract.py +++ b/scripts/check-api-contract.py @@ -117,6 +117,12 @@ def main(): if "get" not in methods: continue url = f"{ADMIN_URL}{base}{path}" + # Some endpoints require a query parameter. The spec carries an example + # under x-contract-example-query so this can exercise them rather than + # skipping the ones most likely to drift. + example_query = methods["get"].get("x-contract-example-query") + if example_query: + url = f"{url}?{example_query}" try: status, raw, _ = request(url, cookie=cookie) except urllib.error.HTTPError as exc: From fa2a1ec62843ef2e35139762b083784371b6c638 Mon Sep 17 00:00:00 2001 From: Martin Dobrev Date: Fri, 7 Aug 2026 14:56:24 +0100 Subject: [PATCH 5/6] feat: Add shadow mode UI 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. --- admin-ui/src/App.tsx | 2 + admin-ui/src/api/client.ts | 95 +++++ admin-ui/src/api/generated.ts | 161 ++++++++ admin-ui/src/components/layout/Sidebar.tsx | 2 + admin-ui/src/pages/shadow/ShadowMode.tsx | 434 +++++++++++++++++++++ 5 files changed, 694 insertions(+) create mode 100644 admin-ui/src/pages/shadow/ShadowMode.tsx diff --git a/admin-ui/src/App.tsx b/admin-ui/src/App.tsx index ba0b8c4..7f76210 100644 --- a/admin-ui/src/App.tsx +++ b/admin-ui/src/App.tsx @@ -27,6 +27,7 @@ import AttackSignatures from '@/pages/security/AttackSignatures' import AttackSignatureEditor from '@/pages/security/AttackSignatureEditor' import BehavioralAnalytics from '@/pages/analytics/BehavioralAnalytics' import ClusterStatus from '@/pages/cluster/ClusterStatus' +import ShadowMode from '@/pages/shadow/ShadowMode' import { About } from '@/pages/About' import { Users } from '@/pages/admin/Users' import { AuthProviders } from '@/pages/admin/AuthProviders' @@ -87,6 +88,7 @@ export default function App() { } /> } /> } /> + } /> } /> } /> } /> diff --git a/admin-ui/src/api/client.ts b/admin-ui/src/api/client.ts index c486b17..11538f8 100644 --- a/admin-ui/src/api/client.ts +++ b/admin-ui/src/api/client.ts @@ -1409,3 +1409,98 @@ export const backupApi = { return backup }, } + +// --------------------------------------------------------------------------- +// Shadow mode +// +// What monitoring mode *would* have blocked. The point of these is to let an +// operator see the consequences of a rule set before it starts rejecting real +// traffic, then promote it once the sample looks right. +// --------------------------------------------------------------------------- + +export interface ShadowCount { + name: string + count: number +} + +export interface ShadowDecision { + ts: number + vhost_id: string + endpoint_id: string + client_ip?: string + host?: string + path?: string + method?: string + score: number + blocked_by?: string[] + flags?: string[] +} + +export interface ShadowScope { + vhost_id: string + endpoint_id: string + would_block_count: number +} + +export interface ShadowSummary { + would_block_total: number + // Non-zero means the recorder's buffer overflowed and every count below + // understates reality. + dropped_total: number + retained_decisions: number + top_rules: ShadowCount[] + top_flags: ShadowCount[] + scopes: ShadowScope[] +} + +export interface ShadowImpact { + vhost_id: string + endpoint_id?: string + would_block_count: number + unique_client_ips: number + average_score: number + window_start?: number + window_end?: number + sample_incomplete: boolean + dropped_total?: number + top_rules: ShadowCount[] + top_flags: ShadowCount[] + affected_endpoints: ShadowCount[] +} + +export interface ShadowPromoteResponse { + promoted: boolean + vhost_id: string + mode?: string + previous_mode?: string + message?: string +} + +export const shadowApi = { + summary: () => request('/shadow/summary'), + + decisions: (params?: { limit?: number; vhost_id?: string; endpoint_id?: string }) => { + const q = new URLSearchParams() + if (params?.limit) q.set('limit', String(params.limit)) + if (params?.vhost_id) q.set('vhost_id', params.vhost_id) + if (params?.endpoint_id) q.set('endpoint_id', params.endpoint_id) + const qs = q.toString() + return request<{ decisions: ShadowDecision[]; count: number; limit?: number }>( + `/shadow/decisions${qs ? `?${qs}` : ''}` + ) + }, + + impact: (vhostId: string, endpointId?: string) => { + const q = new URLSearchParams({ vhost_id: vhostId }) + if (endpointId) q.set('endpoint_id', endpointId) + return request(`/shadow/impact?${q.toString()}`) + }, + + promote: (vhostId: string) => + request('/shadow/promote', { + method: 'POST', + body: JSON.stringify({ vhost_id: vhostId }), + }), + + clear: () => request<{ cleared: boolean }>('/shadow/decisions', { method: 'DELETE' }), +} diff --git a/admin-ui/src/api/generated.ts b/admin-ui/src/api/generated.ts index d3d4979..6c6323f 100644 --- a/admin-ui/src/api/generated.ts +++ b/admin-ui/src/api/generated.ts @@ -195,6 +195,121 @@ export interface paths { patch?: never; trace?: never; }; + "/shadow/summary": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + /** What monitoring mode would have blocked */ + get: { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description Aggregate view over the retained window */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ShadowSummary"]; + }; + }; + }; + }; + put?: never; + post?: never; + delete?: never; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; + "/shadow/decisions": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + /** Individual would-block decisions */ + get: { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description Recent decisions, newest first */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": { + decisions: components["schemas"]["ShadowDecision"][]; + count: number; + limit?: number; + }; + }; + }; + }; + }; + put?: never; + post?: never; + delete?: never; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; + "/shadow/impact": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + /** Pre-flight impact of promoting a scope to blocking */ + get: { + parameters: { + query: { + vhost_id: string; + endpoint_id?: string; + }; + header?: never; + path?: never; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description What promoting this scope would have blocked */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ShadowImpact"]; + }; + }; + }; + }; + put?: never; + post?: never; + delete?: never; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; "/keywords/blocked": { parameters: { query?: never; @@ -374,6 +489,52 @@ export interface components { high_event_rate?: number; }; }; + ShadowDecision: { + /** @description Unix time the decision was made */ + ts: number; + vhost_id: string; + endpoint_id: string; + client_ip?: string; + host?: string; + path?: string; + method?: string; + score: number; + blocked_by?: string[]; + flags?: string[]; + }; + ShadowCount: { + name: string; + count: number; + }; + ShadowSummary: { + would_block_total: number; + /** @description Records lost to the recorder's buffer cap. Non-zero means the sample is incomplete and the counts understate reality. */ + dropped_total: number; + retained_decisions: number; + top_rules: components["schemas"]["ShadowCount"][]; + /** @description Detection-level attribution; this is what identifies a rule to suppress. */ + top_flags: components["schemas"]["ShadowCount"][]; + scopes: { + vhost_id: string; + endpoint_id: string; + would_block_count: number; + }[]; + }; + ShadowImpact: { + vhost_id: string; + endpoint_id?: string; + would_block_count: number; + unique_client_ips: number; + average_score: number; + window_start?: number; + window_end?: number; + /** @description True when records were dropped, so this understates the impact. */ + sample_incomplete: boolean; + dropped_total?: number; + top_rules?: components["schemas"]["ShadowCount"][]; + top_flags?: components["schemas"]["ShadowCount"][]; + affected_endpoints?: components["schemas"]["ShadowCount"][]; + }; WafStatus: { redis_host?: string; redis_port?: number; diff --git a/admin-ui/src/components/layout/Sidebar.tsx b/admin-ui/src/components/layout/Sidebar.tsx index 24b177c..d3f6e8a 100644 --- a/admin-ui/src/components/layout/Sidebar.tsx +++ b/admin-ui/src/components/layout/Sidebar.tsx @@ -23,6 +23,7 @@ import { Activity, Server, Fingerprint, + EyeOff, MessageSquare, Workflow, Target, @@ -50,6 +51,7 @@ const navigation = [ { name: 'Security', children: [ + { name: 'Shadow Mode', href: '/security/shadow', icon: EyeOff }, { name: 'Form Timing', href: '/security/timing', icon: Clock }, { name: 'Defense Profiles', href: '/security/defense-profiles', icon: Workflow }, { name: 'Attack Signatures', href: '/security/attack-signatures', icon: Target }, diff --git a/admin-ui/src/pages/shadow/ShadowMode.tsx b/admin-ui/src/pages/shadow/ShadowMode.tsx new file mode 100644 index 0000000..7199ee7 --- /dev/null +++ b/admin-ui/src/pages/shadow/ShadowMode.tsx @@ -0,0 +1,434 @@ +import { useState } from 'react' +import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query' +import { shadowApi } from '@/api/client' +import type { ShadowCount, ShadowImpact, ShadowScope } from '@/api/client' +import { Card, CardContent, CardDescription, CardHeader, CardTitle } from '@/components/ui/card' +import { Badge } from '@/components/ui/badge' +import { Button } from '@/components/ui/button' +import { Alert, AlertTitle, AlertDescription } from '@/components/ui/alert' +import { + AlertDialog, + AlertDialogAction, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogTitle, +} from '@/components/ui/alert-dialog' +import { + Table, + TableBody, + TableCell, + TableHead, + TableHeader, + TableRow, +} from '@/components/ui/table' +import { useToast } from '@/components/ui/use-toast' +import { + EyeOff, + ShieldCheck, + AlertTriangle, + RefreshCw, + Trash2, + ArrowUpCircle, + Fingerprint, +} from 'lucide-react' + +function formatWhen(ts: number): string { + if (!ts) return '-' + const secs = Math.floor(Date.now() / 1000 - ts) + if (secs < 60) return `${secs}s ago` + if (secs < 3600) return `${Math.floor(secs / 60)}m ago` + if (secs < 86400) return `${Math.floor(secs / 3600)}h ago` + return `${Math.floor(secs / 86400)}d ago` +} + +/** + * A count bar. The relative width is what makes a list of flags readable at a + * glance -- "which rule is responsible for most of this" is the whole question. + */ +function CountBars({ items, empty }: { items: ShadowCount[]; empty: string }) { + if (!items?.length) { + return

{empty}

+ } + const max = Math.max(...items.map((i) => i.count)) + return ( +
+ {items.map((item) => ( +
+
+ {item.name} + {item.count} +
+
+
+
+
+ ))} +
+ ) +} + +export default function ShadowMode() { + const { toast } = useToast() + const queryClient = useQueryClient() + const [pending, setPending] = useState(null) + + const { data: summary, isLoading, refetch } = useQuery({ + queryKey: ['shadow', 'summary'], + queryFn: shadowApi.summary, + refetchInterval: 15000, + }) + + const { data: recent } = useQuery({ + queryKey: ['shadow', 'decisions'], + queryFn: () => shadowApi.decisions({ limit: 25 }), + refetchInterval: 15000, + }) + + // Promotion is the one destructive-ish action here: it starts rejecting live + // traffic. So the impact is fetched first and shown for confirmation rather + // than promoting straight from the table. + const impactMutation = useMutation({ + mutationFn: (scope: ShadowScope) => shadowApi.impact(scope.vhost_id), + onSuccess: (impact) => setPending(impact), + onError: (err: Error) => + toast({ title: 'Could not load impact', description: err.message, variant: 'destructive' }), + }) + + const promoteMutation = useMutation({ + mutationFn: (vhostId: string) => shadowApi.promote(vhostId), + onSuccess: (res) => { + setPending(null) + toast({ + title: res.promoted ? `${res.vhost_id} is now blocking` : 'No change', + description: res.promoted + ? `Was ${res.previous_mode}. This vhost now rejects matching traffic.` + : res.message, + }) + queryClient.invalidateQueries({ queryKey: ['shadow'] }) + queryClient.invalidateQueries({ queryKey: ['vhosts'] }) + }, + onError: (err: Error) => + toast({ title: 'Promote failed', description: err.message, variant: 'destructive' }), + }) + + const clearMutation = useMutation({ + mutationFn: shadowApi.clear, + onSuccess: () => { + toast({ title: 'Sample discarded' }) + queryClient.invalidateQueries({ queryKey: ['shadow'] }) + }, + onError: (err: Error) => + toast({ title: 'Could not clear', description: err.message, variant: 'destructive' }), + }) + + if (isLoading) { + return
Loading shadow decisions...
+ } + + const incomplete = (summary?.dropped_total ?? 0) > 0 + + return ( +
+
+
+

+ + Shadow Mode +

+

+ What monitoring mode would have blocked. Review the sample, then promote a + vhost to blocking once it looks right. +

+
+
+ + +
+
+ + {incomplete && ( + + + This sample is incomplete + + {summary?.dropped_total} decision(s) were dropped because the recorder's buffer + filled up. Every count below understates the real impact — treat them as a floor, + not a total. + + + )} + +
+ + + Would have blocked + + {summary?.would_block_total ?? 0} + + + +

+ requests allowed through that a blocking vhost would have rejected +

+ + + + + Decisions retained + + {summary?.retained_decisions ?? 0} + + + +

individually inspectable below

+ + + + + Scopes affected + {summary?.scopes?.length ?? 0} + + +

vhost / endpoint combinations

+ + +
+ +
+ + + + + Top detections + + + The specific signal that fired. This is what identifies a rule to keep or + suppress — the profile name alone does not. + + + + + + + + + + Top rules + The profile or mechanism that produced the decision. + + + + + +
+ + + + Affected scopes + + Promote a vhost once its sample looks right. You will see the impact before + anything changes. + + + + {summary?.scopes?.length ? ( + + + + Vhost + Endpoint + Would block + + + + + {summary.scopes.map((scope) => ( + + {scope.vhost_id} + + {scope.endpoint_id || '-'} + + + {scope.would_block_count} + + + + + + ))} + +
+ ) : ( +

+ No shadow decisions recorded. A vhost in monitoring mode records what it would + have blocked; a vhost already in blocking mode records nothing here. +

+ )} + + + + + + Recent decisions + Newest first. + + + {recent?.decisions?.length ? ( +
+ + + + When + Client + Request + Score + Detections + + + + {recent.decisions.map((d, i) => ( + + + {formatWhen(d.ts)} + + + {d.client_ip || '-'} + + + + {d.method} {d.path} + + + {d.score} + +
+ {(d.flags ?? []).slice(0, 4).map((f) => ( + + {f} + + ))} + {(d.flags?.length ?? 0) > 4 && ( + + +{(d.flags?.length ?? 0) - 4} + + )} +
+ + + ))} + +
+
+ ) : ( +

No decisions recorded.

+ )} + + + + !open && setPending(null)}> + + + + + Promote {pending?.vhost_id} to blocking? + + +
+

+ This vhost will start rejecting traffic immediately. Based on the recorded + sample, it would have blocked: +

+
+
+
+ {pending?.would_block_count ?? 0} +
+
requests
+
+
+
+ {pending?.unique_client_ips ?? 0} +
+
distinct clients
+
+
+
+ {pending?.average_score ?? 0} +
+
average score
+
+
+ {pending?.sample_incomplete && ( + + + + Records were dropped, so the real impact is larger than these figures. + + + )} + {!!pending?.top_flags?.length && ( +
+

Mostly from

+ +
+ )} + {!!pending?.affected_endpoints?.length && ( +
+

Affected endpoints

+
+ {pending.affected_endpoints.map((e) => ( + + {e.name} ({e.count}) + + ))} +
+
+ )} +
+ + + + Cancel + { + e.preventDefault() + if (pending) promoteMutation.mutate(pending.vhost_id) + }} + disabled={promoteMutation.isPending} + > + {promoteMutation.isPending ? 'Promoting...' : 'Promote to blocking'} + + + + +
+ ) +} From 04f4041197fb51f831e4f857107e33cef74169cd Mon Sep 17 00:00:00 2001 From: Martin Dobrev Date: Fri, 7 Aug 2026 15:13:31 +0100 Subject: [PATCH 6/6] fix: Address Copilot review on PR #39 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. --- openresty/lua/api_handlers/shadow.lua | 13 +++- openresty/lua/shadow_recorder.lua | 93 ++++++++++++++++--------- openresty/lua/waf_handler.lua | 20 +++++- openresty/spec/shadow_recorder_spec.lua | 64 +++++++++++++++-- 4 files changed, 150 insertions(+), 40 deletions(-) diff --git a/openresty/lua/api_handlers/shadow.lua b/openresty/lua/api_handlers/shadow.lua index 654d8d0..79b0728 100644 --- a/openresty/lua/api_handlers/shadow.lua +++ b/openresty/lua/api_handlers/shadow.lua @@ -61,7 +61,12 @@ end --- Decode the stored decision list, optionally narrowed to one scope. local function read_decisions(red, vhost_id, endpoint_id, limit) - local raw = red:lrange(KEYS.decisions, 0, MAX_LIMIT - 1) + -- Unfiltered, every entry read is an entry returned, so ask for exactly the + -- page wanted: a limit=25 UI poll was pulling and decoding 500 JSON blobs. + -- Filtered, the matches can sit anywhere in the list, so the wider read is + -- what makes the filter meaningful. + local scan_to = (vhost_id or endpoint_id) and MAX_LIMIT or limit + local raw = red:lrange(KEYS.decisions, 0, scan_to - 1) local out = {} if type(raw) ~= "table" then return out end @@ -124,7 +129,11 @@ end -- GET /shadow/decisions - the individual records behind the summary _M.handlers["GET:/shadow/decisions"] = function() - local limit = math.min(tonumber(query_arg("limit", DEFAULT_LIMIT)) or DEFAULT_LIMIT, MAX_LIMIT) + -- Clamped at both ends. Without the floor, limit=0 or a negative made + -- read_decisions return nothing at all, which reads as "no decisions + -- recorded" rather than as bad input. + local limit = tonumber(query_arg("limit", DEFAULT_LIMIT)) or DEFAULT_LIMIT + limit = math.max(1, math.min(math.floor(limit), MAX_LIMIT)) local vhost_id = query_arg("vhost_id") local endpoint_id = query_arg("endpoint_id") diff --git a/openresty/lua/shadow_recorder.lua b/openresty/lua/shadow_recorder.lua index 01ca40e..02e6342 100644 --- a/openresty/lua/shadow_recorder.lua +++ b/openresty/lua/shadow_recorder.lua @@ -45,8 +45,12 @@ local MAX_FLAG_LENGTH = 120 local MAX_PATH_LENGTH = 256 local BUFFER_HEAD = "shadow:head" -- next write slot -local BUFFER_TAIL = "shadow:tail" -- next slot to flush +local BUFFER_TAIL = "shadow:tail" -- last slot successfully flushed local DROPPED = "shadow:dropped" +local DRAIN_LOCK = "shadow:draining" +-- Long enough that a real drain never trips it, short enough that a worker +-- killed mid-drain cannot wedge the buffer for more than one sync interval or two. +local DRAIN_LOCK_TTL = 30 --- Trim a value that originates from the request, so a long or hostile field --- cannot bloat storage. @@ -104,37 +108,12 @@ function _M.record(decision) return true end ---- Drain the buffer into Redis. Timer context only. --- @param red an open Redis connection --- @return number of records flushed -function _M.flush(red) - if not shadow_cache or not red then - return 0 - end - - local head = shadow_cache:get(BUFFER_HEAD) or 0 - local tail = shadow_cache:get(BUFFER_TAIL) or 0 - if head <= tail then - return 0 - end - - -- redis_sync's timer runs on EVERY worker, so several of them reach this at - -- the same moment. Reading the tail, draining, then writing the tail back - -- lets each worker drain the same slots -- three workers turned one - -- would-block decision into three recorded copies, which would have - -- overstated the impact of promoting an endpoint by 3x. - -- - -- incr is atomic on a shared dict, so claim the range first and let each - -- worker own a disjoint slice. Whoever claims nothing does nothing. - 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 - +--- Write one slot range to Redis. Called under the drain lock. +-- Separated out so flush() can run it under pcall and still release the lock: +-- otherwise a failure part-way wedges the buffer until the lock TTL expires. +local function drain_range(red, from, to) local flushed = 0 - for slot = claimed_start, claimed_end do + for slot = from, to do local key = "shadow:rec:" .. slot local record = shadow_cache:get(key) if record then @@ -161,6 +140,57 @@ function _M.flush(red) end shadow_cache:delete(key) end + -- Advance one slot at a time rather than jumping the tail at the end: a + -- drain that dies part-way resumes from the last completed slot, with + -- neither a replayed record nor a lost one. + shadow_cache:set(BUFFER_TAIL, slot) + end + return flushed +end + +--- Drain the buffer into Redis. Timer context only. +-- @param red an open Redis connection +-- @return number of records flushed +function _M.flush(red) + if not shadow_cache or not red then + return 0 + end + + local head = shadow_cache:get(BUFFER_HEAD) or 0 + local tail = shadow_cache:get(BUFFER_TAIL) or 0 + if head <= tail then + return 0 + end + + -- redis_sync's timer runs on EVERY worker, so several of them reach this at + -- the same moment. Reading the tail, draining, then writing the tail back + -- lets each worker drain the same slots -- three workers turned one + -- would-block decision into three recorded copies, which overstated the + -- impact of promoting an endpoint by 3x. + -- + -- Claiming a range with incr(TAIL, pending) fixed the duplicates and + -- introduced something worse: a second worker claiming the same `pending` + -- pushed the tail past the head, into slots the writer had not filled yet. + -- Those claims drained nothing, and every record written into that gap was + -- then skipped for good, because the head <= tail guard above reports an + -- empty buffer. Silent loss in the thing whose whole job is to be a + -- trustworthy sample. + -- + -- add() is atomic and fails when the key already exists, so exactly one + -- worker drains and the rest return immediately. + if not shadow_cache:add(DRAIN_LOCK, 1, DRAIN_LOCK_TTL) then + return 0 + end + + -- The TTL on the lock only covers a worker that dies outright. A drain that + -- merely fails -- Redis going away mid-range -- releases here instead, so the + -- next sync interval retries rather than waiting the TTL out. + local ok, flushed = pcall(drain_range, red, tail + 1, head) + if not ok then + shadow_cache:delete(DRAIN_LOCK) + ngx.log(ngx.ERR, "shadow flush failed, resuming from last completed slot: ", + tostring(flushed)) + return 0 end if flushed > 0 then @@ -178,6 +208,7 @@ function _M.flush(red) shadow_cache:set(DROPPED, 0) end + shadow_cache:delete(DRAIN_LOCK) return flushed end diff --git a/openresty/lua/waf_handler.lua b/openresty/lua/waf_handler.lua index 912e9f1..1bdaa31 100644 --- a/openresty/lua/waf_handler.lua +++ b/openresty/lua/waf_handler.lua @@ -56,9 +56,25 @@ local LOG_HMAC_KEY = os.getenv("WAF_LOG_HMAC_KEY") -- -- 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. +-- Resolved once and cached, following get_multi_executor above. This runs on +-- every would-block request, and pcall(require, ...) on a hot path is overhead +-- for a lookup whose answer never changes. _shadow_tried keeps a failed require +-- from being retried on every request too. +local _shadow_recorder +local _shadow_tried = false + +local function get_shadow_recorder() + if not _shadow_tried then + _shadow_tried = true + local ok, mod = pcall(require, "shadow_recorder") + if ok then _shadow_recorder = mod end + end + return _shadow_recorder +end + 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 + local shadow_recorder = get_shadow_recorder() + if not shadow_recorder then return end shadow_recorder.record({ diff --git a/openresty/spec/shadow_recorder_spec.lua b/openresty/spec/shadow_recorder_spec.lua index d40a2ea..a4c8340 100644 --- a/openresty/spec/shadow_recorder_spec.lua +++ b/openresty/spec/shadow_recorder_spec.lua @@ -80,14 +80,68 @@ describe("shadow_recorder", function() assert.equals(0, #second.calls.lpush) end) - it("claims the range before draining, so a concurrent drain finds nothing", function() + it("only one worker drains at a time", function() for i = 1, 5 do recorder.record(a_decision()) end - local depth_before = recorder.buffer_depth() - assert.equals(5, depth_before) - -- A worker that claims first leaves nothing for the next to claim. + assert.equals(5, recorder.buffer_depth()) recorder.flush(fake_redis()) assert.equals(0, recorder.buffer_depth(), - "the claim must advance the tail so another worker drains nothing") + "a completed drain must leave nothing for another worker") + end) + + it("a concurrent drain never pushes the tail past the head", function() + -- The defect this guards, and it is subtler than the duplicate it + -- replaced. Claiming a range with incr(TAIL, pending) let a second + -- worker claim slots the writer had not filled yet, leaving tail > head. + -- Records written into that gap were then skipped for good: no + -- duplicates, so the obvious test passed, while the sample silently lost + -- records -- the one thing a shadow sample must not do. + for i = 1, 5 do recorder.record(a_decision()) end + + -- Two workers reach flush together. The first holds the drain lock, so + -- simulate the second arriving before the first has released it by + -- calling flush again from inside the fake connection's first lpush. + local second_result + local red = fake_redis() + local real_lpush = red.lpush + local reentered = false + red.lpush = function(self, ...) + if not reentered then + reentered = true + second_result = recorder.flush(fake_redis()) + end + return real_lpush(self, ...) + end + + assert.equals(5, recorder.flush(red)) + assert.equals(0, second_result, "the second worker must drain nothing") + assert.is_true(reentered, "the concurrent drain did not run; test is not proving anything") + + -- The real assertion: the buffer still works afterwards. + for i = 1, 3 do recorder.record(a_decision()) end + assert.equals(3, recorder.buffer_depth(), + "records written after a concurrent drain must still be visible") + local after = fake_redis() + assert.equals(3, recorder.flush(after), + "records written after a concurrent drain must still be flushed") + end) + + it("resumes from the last completed slot rather than replaying or skipping", function() + for i = 1, 6 do recorder.record(a_decision()) end + -- A drain that dies part-way: fail the connection after 2 records. + local red = fake_redis() + local n = 0 + local real_lpush = red.lpush + red.lpush = function(self, ...) + n = n + 1 + if n > 2 then error("connection lost") end + return real_lpush(self, ...) + end + pcall(recorder.flush, red) + + -- The 2 that landed must not come back; the other 4 must not be lost. + local resumed = fake_redis() + assert.equals(4, recorder.flush(resumed), + "a resumed drain must pick up exactly the slots that did not complete") end) it("flushing an empty buffer is a no-op", function()