Quality hardening (adversarial review) + BIG-IP/NGINX docs & screenshots - #57
Merged
Conversation
…rial review
A multi-agent bug-hunt of the new code surfaced real impacting defects; fixed with
regression tests (+17):
SECURITY
- Path traversal via finding_id: it lands in on-box file paths and reaches the
transport from the console /api/apply-nginx body, so a value like
'x/../../../etc/nginx/nginx' could sudo-write/delete arbitrary root files. Now
validated (^[A-Za-z0-9_.][A-Za-z0-9_.-]*$) at apply/retire + backstopped by a
'..'-segment guard in the Nginx client; remove_dir also refuses a non-vpcopilot dir.
- Config injection via origin/location: interpolated raw into the vhost, a value
with ';{}' could inject a second server block. Now validated (host:port / path).
- DOM XSS: esc() didn't escape quotes; a crafted finding id in a value="..." attr
could inject a handler in the operator console. esc() now escapes " and '.
- SSH host-key verification was hardcoded off (MITM on the policy-push/root channel
could forge a 'blocked' verdict). Now TOFU (accept-new) by default, NGINX_SSH_STRICT
overridable, /dev/null only in explicit 'no' mode.
SAFETY / CORRECTNESS
- An exception after the real apply (a step-3 nginx -t the operator's config now
rejects, a transient validation/reload error) left an enforcing, un-ledgered
band-aid (retire/reconcile could never find it) or a config-poisoning include.
The real-apply body now detaches + reloads before re-raising.
- nginx_lab.remove ran `rm -f` on the managed DIRECTORY (rc=1) → reload+audit never
ran and the vhost stayed loaded. Uses a new rm -rf remove_dir.
- create dry-run overwrote-then-DELETED an existing live vhost; now preserves/restores
it (new read_file).
- The vhost hardcoded /etc/nginx/conf.d, so band-aids on a custom NGINX_INCLUDE_DIR
silently never enforced. The include path now uses the real include_dir.
- get_config swallowed a failed `nginx -T` as an empty config; now raises.
- _await_enforcement re-hammered a failing login for the whole window; now short-
circuits on auth_failed/no_probe/unobservable-leak. A corrupt spec no longer aborts
a valid apply. status() computes version() inside the try. default_server is opt-in.
Full suite 1178 pass, ruff clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…NGINX.md, TRY_IT.md The same declarative band-aid now emits + applies for three enforcement points (XC, BIG-IP Advanced WAF, F5 WAF for NGINX / App Protect). Docs updated to match: - README: a "one finding, every enforcement point" section (all three forms proven on live BIG-IP + NGINX+App-Protect); the ④ Mitigate note now names the "Apply on your own BIG-IP / NGINX" panels; docs/NGINX.md added to the table. - BIGIP.md: "what it can patch" de-staled — service_policy + waf_data_guard + api_schema all live-proven (waf honestly declined for signature staging); the console ④ Mitigate panel is shipped, not "coming next". - NGINX.md (new): the NGINX-admin guide mirroring BIGIP.md — SSH connect, nginx-lab create, apply/retire, the guard, troubleshooting, the three forms. - TRY_IT.md: an "Or: mitigate on your own NGINX (App Protect)" section. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rt, live Captured against both live appliances (BIG-IP AS3 3.56.0, NGINX Plus R37 + App Protect) via the console on the demo dataset: - 4-mitigate.png (updated): the ④ Mitigate step now shows the XC table PLUS the "Apply on your own BIG-IP" and "Apply on your own NGINX" panels, both reporting ready, with emitter-driven finding dropdowns and the honest declines. - apply-your-own-waf.png (new): a focused shot of both apply panels — used in the README hybrid-fabric section and both admin guides. - report-bigip-nginx.png (new): the report's BIG-IP Advanced WAF + F5 WAF for NGINX sections — same finding, form emitted, honest declines. README / BIGIP.md / NGINX.md reference them; the demo report is regenerated with the NGINX section. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Three things you asked for: a quality deep-dive on the new virtual-patch code, refreshed docs for the BIG-IP + NGINX functionality, and product screenshots taken against the live appliances.
1. Quality deep-dive (adversarial review → fixes)
Ran a multi-agent bug-hunt over the NGINX App Protect variant + the Task A report/console generalization; verified the findings against the code and fixed the real ones, each with a regression test (+17 tests):
Security
finding_id— it lands in on-box file paths and reaches the transport from the console/api/apply-nginxbody, sox/../../../etc/nginx/nginxcouldsudo-write/delete arbitrary root files. Now validated at apply/retire + a..-guard in the client;remove_dirrefuses a non-vpcopilot-dir.origin/location— interpolated raw into the vhost; a value with;{}could inject a secondserverblock. Now validated.esc()didn't escape quotes; a crafted finding id in avalue="…"attribute could inject a handler. Fixed.accept-new) by default,NGINX_SSH_STRICToverridable.Safety / correctness
nginx_lab.removeranrm -fon a directory (rc=1 → reload/audit skipped) — uses a newrm -rf.createdry-run deleted an existing live vhost — now preserves it./etc/nginx/conf.d, so a customNGINX_INCLUDE_DIRsilently never enforced — now uses the real dir.get_configmasked a failednginx -Tas empty;_await_enforcementre-hammered a failing login; a corrupt spec aborted a valid apply — all fixed.Every fix re-verified live on the box (status + apply-nginx still block, with the hardened transport). Full suite 1178 pass, ruff clean.
2. Docs — the hybrid fabric
The same band-aid, three enforcement points (XC · BIG-IP Advanced WAF · F5 WAF for NGINX). README reframed;
docs/BIGIP.mdde-staled (3 forms live, console panel shipped); newdocs/NGINX.md(NGINX-admin guide mirroring BIGIP.md);docs/TRY_IT.mdgains a NGINX section.3. Screenshots (live)
Captured against both live appliances (BIG-IP AS3 3.56.0, NGINX Plus R37 + App Protect): the ④ Mitigate panels reporting ready with emitter-driven dropdowns + honest declines, and the report's BIG-IP + NGINX sections. Wired into the README and admin guides.
🤖 Generated with Claude Code