Add WAF support to HTTP ingress routes#786
Conversation
Integrate Coraza WAF with OWASP Core Rule Set into the HTTP ingress layer. Routes gain a waf_level field (0=disabled, 1-4=CRS paranoia level) that enables request-level security filtering before traffic reaches user apps. - Add waf_level field to http_route entity schema - Create pkg/waf engine with per-paranoia-level caching - Add WAF middleware to httpingress (WAF → OIDC → serve chain) - Refactor duplicated OIDC middleware into unified composition - Add CLI commands: route waf / route unwaf - Show WAF level in route show and route list - Use Patch for waf_level=0 to handle entity zero-value encoding
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-route Web Application Firewall (WAF) support. The HTTP route schema gains a Comment |
Consolidate WAF enable/disable into a single command (route waf) with a --disable flag instead of a separate route unwaf subcommand. Add WAF documentation page covering paranoia levels, what gets blocked, and how the middleware fits into the request pipeline. Regenerate CLI command docs to include the updated route waf command.
MustRun asserts success internally, so expected-failure cases for invalid WAF levels need to use Run instead.
Replace deprecated reflect.Ptr with reflect.Pointer and convert WriteString(fmt.Sprintf(...)) to fmt.Fprintf(...) across the codebase.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/ingress/client.go`:
- Around line 432-441: The exported function SetRouteWAFLevelOnRoute currently
dereferences route (route.WafLevel, route.ID) without checking for nil; add an
early nil guard at the start of SetRouteWAFLevelOnRoute (e.g., if route == nil {
return nil, fmt.Errorf("route is nil") }) to return a typed error instead of
panicking, then proceed with the existing validation, assignment to
route.WafLevel and the c.ec.Patch call using route.ID; ensure the error message
is descriptive and consistent with other API errors.
In `@docs/docs/waf.md`:
- Line 113: The docs incorrectly state oversized bodies "skip body inspection
but still have their headers and URL checked"; update the text to reflect the
actual behavior controlled by SecRequestBodyLimitAction in pkg/waf/waf.go: when
SecRequestBodyLimitAction is set to Reject, requests with bodies larger than 10
MB are rejected with HTTP 413 (Request Entity Too Large) rather than being
partially inspected or allowed through; change the sentence to explicitly
mention the 413 rejection and reference SecRequestBodyLimitAction Reject as the
governing configuration.
In `@pkg/waf/waf.go`:
- Around line 78-84: The WAF match callback currently logs the full request URI
via mr.URI(), which can contain sensitive query parameters; update the
WithErrorCallback handler to avoid logging full URIs by extracting and logging
only the path (or a redacted version) from mr.URI() before passing it to
log.Warn — locate the WithErrorCallback anonymous function handling
types.MatchedRule (the mr variable) and replace mr.URI() with a safe value
derived from parsing mr.URI() (e.g., use the path component or a redaction
function) so that log.Warn("WAF rule matched", "id", mr.Rule().ID(), "msg",
mr.Message(), "severity", mr.Rule().Severity().String(), "uri",
<redacted-or-path>) is used instead of the raw mr.URI().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ddae6a93-6c4f-43fc-9894-7357732982e4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
api/ingress/client.goapi/ingress/client_test.goapi/ingress/ingress_v1alpha/schema.gen.goapi/ingress/schema.ymlblackbox/route_waf_test.gocli/commands/commands.gocli/commands/route_list.gocli/commands/route_show.gocli/commands/route_waf.godocs/command-sidebar.jsondocs/docs/command/route-waf.mddocs/docs/command/route.mddocs/docs/commands.mddocs/docs/waf.mddocs/sidebars.tsgo.modpkg/waf/waf.gopkg/waf/waf_test.goservers/httpingress/httpingress.goservers/httpingress/waf.goservers/httpingress/waf_test.go
Add unreachable returns after t.Fatal to satisfy SA5011 nil-pointer analysis, and remove always-true nil checks on runUndo which never returns nil (SA4023).
Add unreachable returns after t.Fatal nil checks so staticcheck recognizes the subsequent dereferences are safe.
phinze
left a comment
phinze
left a comment
There was a problem hiding this comment.
Claude and Paul here. The big-picture architectural conversation lives on the 787 review where we walked through both PRs together with notes from yesterday's product standup. Short version: the route schema is growing asymmetric across our three protections, and we're suggesting consolidating the auth side into a single auth_provider kind there.
Here's how we're thinking WAF should look: waf_profile is its own kind (initially with just paranoia_level, leaving headroom for custom rules, allowlists, mode toggles later). The route holds a waf_profile ref. The user-facing CLI m route waf <host> --level N works as it does today by auto-creating or looking up default profiles (default-l1, default-l2, etc.) under the covers. Power users get m waf profile add/list/show/remove to name and share profiles across routes.
The migration cost asymmetry is what makes doing this now cheap. waf_level int to waf_profile ref is a route-table backfill on every protected route. waf_profile ref to a future middleware ref is a much smaller move on far fewer rows. Doing the entity reshape now sets up the right shape and saves the bigger migration later.
Marking REQUEST_CHANGES on the waf_profile reshape specifically. Once that pivot lands we'll come back through for the rest of the review properly.
Replace the waf_level int field on http_route with a waf_profile ref to a new waf_profile entity kind. Profiles are identified by entity ID (e.g. waf-l1 through waf-l4) and contain paranoia_level. The middleware resolves profiles via a 30s TTL cache to avoid per-request entity lookups.
- Add nil guards to SetRouteWAFLevelOnRoute and DetachWAFProfileFromRoute - Fix docs: oversized request bodies are rejected (413), not passed through - Log only path (not query string) in WAF match callbacks to avoid leaking sensitive query parameters
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cli/commands/route_list.go (1)
29-38: ⚡ Quick winMemoize WAF profile lookups inside
route list.This helper does an RPC per route even though the same few profile IDs repeat across the whole listing. On larger route sets that turns
route listinto an unnecessary N+1 lookup pattern.Proposed fix
- resolveWAFLevel := func(route *ingress_v1alpha.HttpRoute) int { + wafLevels := map[entity.Id]int{} + resolveWAFLevel := func(route *ingress_v1alpha.HttpRoute) int { if entity.Empty(route.WafProfile) { return 0 } + if level, ok := wafLevels[route.WafProfile]; ok { + return level + } profile, err := ic.GetWAFProfileByID(ctx, route.WafProfile) if err != nil || profile == nil { + wafLevels[route.WafProfile] = 0 return 0 } - return int(profile.ParanoiaLevel) + level := int(profile.ParanoiaLevel) + wafLevels[route.WafProfile] = level + return level }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/commands/route_list.go` around lines 29 - 38, The resolveWAFLevel helper in route list performs an RPC per route via ic.GetWAFProfileByID causing N+1 calls; fix by adding a local memoization map (e.g., map[string]*WAFProfile or map[string]*ingress_v1alpha.WafProfile and map[string]bool for missing) in the enclosing scope of resolveWAFLevel, check the map before calling GetWAFProfileByID, store fetched profiles (and negative results) in the map, and have resolveWAFLevel return int(profile.ParanoiaLevel) using the cached entry to avoid repeated RPCs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/commands/route_waf.go`:
- Around line 62-75: When opts.Disable is true the handler uses ctx.Printf to
print human-readable messages even when the user requested JSON; update the
disable branch (the block referencing opts.Disable,
entity.Empty(route.WafProfile), ic.DetachWAFProfileFromRoute and ctx.Printf) to
honor the CLI output format: detect the JSON format flag and emit a proper JSON
response (e.g., an object indicating route label and waf_enabled: false or an
error) instead of plain text both when the route is already unwafed and after a
successful detach, and fall back to the existing human-readable ctx.Printf only
for non-JSON output.
In `@servers/httpingress/waf.go`:
- Around line 50-69: The current lookup in GetWAFProfileByID trusts
profile.ParanoiaLevel and only caches successful profiles; update the logic
around s.ingressClient.GetWAFProfileByID, the profile variable, and the
s.wafProfileCache/wafProfileEntry so that missing or malformed profiles are
normalized and cached for the TTL: if profile is nil or profile.ParanoiaLevel is
out of range, set a fallback paranoia level clamped to the range 1..4, store
that value in s.wafProfileCache[profileID] (using the existing wafProfileEntry
with fetchedAt = time.Now()) under s.wafProfileMu lock, and return the
normalized level; if a real profile exists, clamp profile.ParanoiaLevel to 1..4
before caching and returning it so Handler/next won’t silently disable
inspection.
---
Nitpick comments:
In `@cli/commands/route_list.go`:
- Around line 29-38: The resolveWAFLevel helper in route list performs an RPC
per route via ic.GetWAFProfileByID causing N+1 calls; fix by adding a local
memoization map (e.g., map[string]*WAFProfile or
map[string]*ingress_v1alpha.WafProfile and map[string]bool for missing) in the
enclosing scope of resolveWAFLevel, check the map before calling
GetWAFProfileByID, store fetched profiles (and negative results) in the map, and
have resolveWAFLevel return int(profile.ParanoiaLevel) using the cached entry to
avoid repeated RPCs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3afe4aa-3ab3-4b72-ac8f-dd227925ff21
📒 Files selected for processing (12)
api/ingress/client.goapi/ingress/client_test.goapi/ingress/ingress_v1alpha/schema.gen.goapi/ingress/schema.ymlcli/commands/route_list.gocli/commands/route_show.gocli/commands/route_waf.godocs/docs/waf.mdpkg/waf/waf.goservers/httpingress/httpingress.goservers/httpingress/waf.goservers/httpingress/waf_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/docs/waf.md
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/commands/route_show.go
- pkg/waf/waf.go
- servers/httpingress/httpingress.go
Skip entity existence check for empty ref values during validation. An empty ref means "no reference" and should not trigger a lookup against the entity store. This fixes WAF profile detach which uses Patch to set the waf_profile ref to empty.
phinze
left a comment
phinze
left a comment
There was a problem hiding this comment.
Claude and Paul here. Round two — the waf_profile reshape we asked for landed cleanly, and the inline CR findings from last time (nil guards, URI redaction, docs accuracy) are addressed too. Took it for a spin in dev: enabled, blocked SQL injection and XSS, switched levels, disabled, and watched the shared waf-l1/waf-l2/waf-l3 profiles get lazily created as singletons in the entity store. The whole shape feels right and leaves room for the future m waf profile ... power-user CLI we floated.
One thing the spin surfaced beyond the inlines: we tried this against the real-world "my logs are full of WordPress scans" complaint and the WAF doesn't help with that — CRS catches payloads (SQL injection, XSS, command injection), not URL fingerprinting. Worth saying so explicitly in waf.md since users will reach for this expecting it to quiet down bot noise and be surprised when it doesn't. Inline comment with details.
|
|
||
| import CliCommand from '@site/src/components/CliCommand'; | ||
|
|
||
| # Web Application Firewall (WAF) |
There was a problem hiding this comment.
Could we add a "what this doesn't do" callout near the top? CRS catches attack payloads (SQL injection, XSS, command injection, path traversal) — not URL pattern scanning. Users hitting this from a "my Rails app logs are full of /wp-admin/ and /xmlrpc.php" angle will turn it on, see the same noise, and be confused.
A line like "the WAF inspects request content for attack patterns; it doesn't fingerprint suspicious URLs, rate-limit, or block bot reconnaissance — for that, consider an upstream proxy like Cloudflare" sets expectations and saves a support thread.
- Honor --format json in route waf --disable and already-disabled paths - Add callout to WAF docs clarifying CRS scope (no rate-limiting or bot fingerprinting) - Fix second empty-ref validation path in validateToType to match the ValidateAttribute fix
2 participants
Summary
waf_levelfield (0=disabled, 1-4=CRS paranoia level) enabling request-level security filtering (SQL injection, XSS, path traversal, etc.) before traffic reaches user appsmiren route waf <host> --level Nandmiren route unwaf <host>Test plan
pkg/wafunit tests — SQL injection, XSS, POST injection blocked; clean requests pass; paranoia levels 1-4; engine cachingservers/httpingressWAF middleware tests — disabled when level 0, blocks attacks, allows clean, respects levels, handles invalid/negative levelsapi/ingress/clienttests — set/lookup WAF level, disable (zero-value via Patch), invalid levels, non-existent routemake lintpasses