[management, proxy, self-hosted] Add CrowdSec AppSec request inspection to the reverse proxy - #6917
Open
lixmal wants to merge 6 commits into
Open
[management, proxy, self-hosted] Add CrowdSec AppSec request inspection to the reverse proxy#6917lixmal wants to merge 6 commits into
lixmal wants to merge 6 commits into
Conversation
Contributor
|
Warning Review limit reachedNext included review available in 3 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (41)
📝 WalkthroughWalkthroughChangesCrowdSec AppSec support is added across provisioning, management capability propagation, service restriction persistence and validation, proxy startup, request mirroring, and middleware enforcement. HTTP services can use enforce, observe, or off modes with body limits, redaction, verdict mapping, and failure handling. CrowdSec AppSec integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ProxyCLI
participant ProxyServer
participant AuthMiddleware
participant AppSecClient
participant CrowdSec
participant Backend
ProxyCLI->>ProxyServer: Provide AppSec URL and limits
ProxyServer->>AppSecClient: Initialize shared client
ProxyServer->>AuthMiddleware: Configure AppSec middleware
AuthMiddleware->>AppSecClient: Inspect inbound HTTP request
AppSecClient->>CrowdSec: Send redacted mirrored request
CrowdSec-->>AppSecClient: Return allow, captcha, or ban action
AppSecClient-->>AuthMiddleware: Return verdict and body release
AuthMiddleware->>Backend: Forward allowed request
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Jul 27, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@shared/management/proto/proxy_service.proto`:
- Around line 214-216: Update the documentation comment for the appsec_mode
field to state that enabled modes ("enforce" and "observe") are supported only
for HTTP services and are invalid for TCP/UDP/TLS services, rather than being
ignored. Preserve the existing mode values and field definition.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f5316be8-28c1-4fb1-a74a-c3f6eb5770a8
⛔ Files ignored due to path filters (1)
shared/management/proto/proxy_service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (38)
infrastructure_files/getting-started.shmanagement/internals/modules/reverseproxy/domain/domain.gomanagement/internals/modules/reverseproxy/domain/manager/api.gomanagement/internals/modules/reverseproxy/domain/manager/manager.gomanagement/internals/modules/reverseproxy/domain/manager/manager_test.gomanagement/internals/modules/reverseproxy/proxy/manager.gomanagement/internals/modules/reverseproxy/proxy/manager/manager.gomanagement/internals/modules/reverseproxy/proxy/manager/manager_test.gomanagement/internals/modules/reverseproxy/proxy/manager_mock.gomanagement/internals/modules/reverseproxy/proxy/proxy.gomanagement/internals/modules/reverseproxy/service/manager/api.gomanagement/internals/modules/reverseproxy/service/manager/manager.gomanagement/internals/modules/reverseproxy/service/service.gomanagement/internals/modules/reverseproxy/service/service_test.gomanagement/internals/shared/grpc/proxy.gomanagement/server/store/sql_store.gomanagement/server/store/sql_store_service_test.gomanagement/server/store/store.gomanagement/server/store/store_mock.goproxy/cmd/proxy/cmd/root.goproxy/internal/appsec/body.goproxy/internal/appsec/client.goproxy/internal/appsec/client_test.goproxy/internal/auth/appsec_test.goproxy/internal/auth/header.goproxy/internal/auth/middleware.goproxy/internal/auth/middleware_test.goproxy/internal/auth/oidc.goproxy/internal/auth/tunnel_lookup_test.goproxy/internal/middleware/bodytap/request.goproxy/internal/netutil/upgrade.goproxy/internal/restrict/restrict.goproxy/lifecycle.goproxy/management_integration_test.goproxy/server.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/proxy_service.proto
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
lixmal
force-pushed
the
reverse-proxy-crowdsec-appsec
branch
from
July 27, 2026 13:09
842b196 to
cbe2836
Compare
lixmal
force-pushed
the
reverse-proxy-crowdsec-appsec
branch
from
July 27, 2026 16:50
cbe2836 to
b936400
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@proxy/internal/appsec/client.go`:
- Around line 453-456: Update the Cookie redaction logic around
redactCookieHeader to iterate over all values returned by dst.Values("Cookie"),
redact each value using req.RedactCookies, and replace the complete Cookie
header set so no later value bypasses redaction. Preserve unchanged values while
ensuring the header is rewritten with the processed values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 10698771-9b54-4f6f-b7f4-d2dbc5b0ed42
⛔ Files ignored due to path filters (1)
shared/management/proto/proxy_service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (38)
infrastructure_files/getting-started.shmanagement/internals/modules/reverseproxy/domain/domain.gomanagement/internals/modules/reverseproxy/domain/manager/api.gomanagement/internals/modules/reverseproxy/domain/manager/manager.gomanagement/internals/modules/reverseproxy/domain/manager/manager_test.gomanagement/internals/modules/reverseproxy/proxy/manager.gomanagement/internals/modules/reverseproxy/proxy/manager/manager.gomanagement/internals/modules/reverseproxy/proxy/manager/manager_test.gomanagement/internals/modules/reverseproxy/proxy/manager_mock.gomanagement/internals/modules/reverseproxy/proxy/proxy.gomanagement/internals/modules/reverseproxy/service/manager/api.gomanagement/internals/modules/reverseproxy/service/manager/manager.gomanagement/internals/modules/reverseproxy/service/service.gomanagement/internals/modules/reverseproxy/service/service_test.gomanagement/internals/shared/grpc/proxy.gomanagement/server/store/sql_store.gomanagement/server/store/sql_store_service_test.gomanagement/server/store/store.gomanagement/server/store/store_mock.goproxy/cmd/proxy/cmd/root.goproxy/internal/appsec/body.goproxy/internal/appsec/client.goproxy/internal/appsec/client_test.goproxy/internal/auth/appsec_test.goproxy/internal/auth/header.goproxy/internal/auth/middleware.goproxy/internal/auth/middleware_test.goproxy/internal/auth/oidc.goproxy/internal/auth/tunnel_lookup_test.goproxy/internal/middleware/bodytap/request.goproxy/internal/netutil/upgrade.goproxy/internal/restrict/restrict.goproxy/lifecycle.goproxy/management_integration_test.goproxy/server.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/proxy_service.proto
🚧 Files skipped from review as they are similar to previous changes (35)
- management/server/store/store_mock.go
- proxy/internal/netutil/upgrade.go
- management/server/store/store.go
- management/internals/modules/reverseproxy/proxy/proxy.go
- management/internals/modules/reverseproxy/service/manager/manager.go
- management/internals/modules/reverseproxy/domain/manager/manager_test.go
- management/internals/modules/reverseproxy/domain/manager/api.go
- proxy/management_integration_test.go
- management/internals/modules/reverseproxy/proxy/manager.go
- proxy/internal/auth/header.go
- shared/management/http/api/types.gen.go
- management/internals/modules/reverseproxy/domain/domain.go
- proxy/internal/auth/oidc.go
- shared/management/http/api/openapi.yml
- management/internals/shared/grpc/proxy.go
- proxy/internal/middleware/bodytap/request.go
- management/internals/modules/reverseproxy/proxy/manager/manager_test.go
- management/internals/modules/reverseproxy/service/manager/api.go
- proxy/internal/restrict/restrict.go
- proxy/internal/auth/tunnel_lookup_test.go
- proxy/lifecycle.go
- management/internals/modules/reverseproxy/proxy/manager/manager.go
- infrastructure_files/getting-started.sh
- proxy/internal/auth/appsec_test.go
- proxy/cmd/proxy/cmd/root.go
- proxy/internal/appsec/body.go
- management/internals/modules/reverseproxy/domain/manager/manager.go
- proxy/server.go
- management/internals/modules/reverseproxy/service/service.go
- management/internals/modules/reverseproxy/service/service_test.go
- shared/management/proto/proxy_service.proto
- proxy/internal/auth/middleware_test.go
- management/internals/modules/reverseproxy/proxy/manager_mock.go
- proxy/internal/auth/middleware.go
- proxy/internal/appsec/client_test.go
lixmal
force-pushed
the
reverse-proxy-crowdsec-appsec
branch
3 times, most recently
from
July 27, 2026 17:15
735ebba to
dcb942a
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
proxy/internal/appsec/client.go (1)
442-458: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCookie redaction still only checks the first
Cookievalue; unfixed from prior review.
dst.Get("Cookie")reads only the first value inhttp.Header["Cookie"]. Per RFC 9113 §8.2.3, HTTP/2 clients (mainstream browsers included) are permitted to split theCookieheader into multiple header fields for HPACK compression efficiency, so a session cookie can legitimately land in a second or laterCookieheader that this code never inspects or redacts. That value would still be forwarded intact to the AppSec engine even when it names a cookie inreq.RedactCookies.Proposed fix
- if cookie := dst.Get("Cookie"); cookie != "" { - if redacted := redactCookieHeader(cookie, req.RedactCookies); redacted != cookie { - dst.Set("Cookie", redacted) - } + cookies := dst.Values("Cookie") + for i, cookie := range cookies { + cookies[i] = redactCookieHeader(cookie, req.RedactCookies) + } + if len(cookies) > 0 { + dst["Cookie"] = cookies }🤖 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 `@proxy/internal/appsec/client.go` around lines 442 - 458, Update redactSecrets to inspect all Cookie header values via dst.Values("Cookie") rather than only dst.Get("Cookie"), redact each value with redactCookieHeader using req.RedactCookies, and replace the header values while preserving unmodified values.
🧹 Nitpick comments (1)
proxy/internal/auth/middleware.go (1)
372-375: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a higher log level for inspection failures.
In enforce mode this path fails closed with a 403, yet the only signal is a
Debugline. An unreachable or misconfigured AppSec engine would silently 403 all opted-in traffic with nothing in default-level logs.Warnf(or a rate-limited warn) would make the outage visible; the per-request verdict metadata already covers the access-log side.♻️ Proposed change
result, err := mw.appsec.Inspect(r.Context(), req) if err != nil { - mw.logger.Debugf("AppSec inspection failed for %s: %v", r.Host, err) + mw.logger.Warnf("AppSec inspection failed for %s: %v", r.Host, err) }🤖 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 `@proxy/internal/auth/middleware.go` around lines 372 - 375, Update the AppSec inspection error handling around mw.appsec.Inspect to log failures at warning level instead of debug level, preserving the existing message, context, and enforcement behavior.
🤖 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.
Duplicate comments:
In `@proxy/internal/appsec/client.go`:
- Around line 442-458: Update redactSecrets to inspect all Cookie header values
via dst.Values("Cookie") rather than only dst.Get("Cookie"), redact each value
with redactCookieHeader using req.RedactCookies, and replace the header values
while preserving unmodified values.
---
Nitpick comments:
In `@proxy/internal/auth/middleware.go`:
- Around line 372-375: Update the AppSec inspection error handling around
mw.appsec.Inspect to log failures at warning level instead of debug level,
preserving the existing message, context, and enforcement behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3aabb737-1204-4303-bb21-06c36c1b6bb6
⛔ Files ignored due to path filters (1)
shared/management/proto/proxy_service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (38)
infrastructure_files/getting-started.shmanagement/internals/modules/reverseproxy/domain/domain.gomanagement/internals/modules/reverseproxy/domain/manager/api.gomanagement/internals/modules/reverseproxy/domain/manager/manager.gomanagement/internals/modules/reverseproxy/domain/manager/manager_test.gomanagement/internals/modules/reverseproxy/proxy/manager.gomanagement/internals/modules/reverseproxy/proxy/manager/manager.gomanagement/internals/modules/reverseproxy/proxy/manager/manager_test.gomanagement/internals/modules/reverseproxy/proxy/manager_mock.gomanagement/internals/modules/reverseproxy/proxy/proxy.gomanagement/internals/modules/reverseproxy/service/manager/api.gomanagement/internals/modules/reverseproxy/service/manager/manager.gomanagement/internals/modules/reverseproxy/service/service.gomanagement/internals/modules/reverseproxy/service/service_test.gomanagement/internals/shared/grpc/proxy.gomanagement/server/store/sql_store.gomanagement/server/store/sql_store_service_test.gomanagement/server/store/store.gomanagement/server/store/store_mock.goproxy/cmd/proxy/cmd/root.goproxy/internal/appsec/body.goproxy/internal/appsec/client.goproxy/internal/appsec/client_test.goproxy/internal/auth/appsec_test.goproxy/internal/auth/header.goproxy/internal/auth/middleware.goproxy/internal/auth/middleware_test.goproxy/internal/auth/oidc.goproxy/internal/auth/tunnel_lookup_test.goproxy/internal/middleware/bodytap/request.goproxy/internal/netutil/upgrade.goproxy/internal/restrict/restrict.goproxy/lifecycle.goproxy/management_integration_test.goproxy/server.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/proxy_service.proto
🚧 Files skipped from review as they are similar to previous changes (36)
- proxy/internal/auth/oidc.go
- proxy/internal/netutil/upgrade.go
- management/internals/modules/reverseproxy/service/manager/api.go
- shared/management/proto/proxy_service.proto
- management/internals/modules/reverseproxy/domain/manager/api.go
- management/server/store/store.go
- management/internals/modules/reverseproxy/proxy/manager/manager_test.go
- management/server/store/sql_store_service_test.go
- management/server/store/store_mock.go
- management/internals/modules/reverseproxy/domain/manager/manager_test.go
- management/internals/modules/reverseproxy/service/manager/manager.go
- proxy/internal/appsec/body.go
- management/internals/shared/grpc/proxy.go
- proxy/cmd/proxy/cmd/root.go
- proxy/internal/auth/header.go
- management/internals/modules/reverseproxy/domain/domain.go
- management/internals/modules/reverseproxy/domain/manager/manager.go
- shared/management/http/api/openapi.yml
- management/internals/modules/reverseproxy/proxy/manager/manager.go
- management/internals/modules/reverseproxy/service/service_test.go
- proxy/internal/middleware/bodytap/request.go
- proxy/internal/auth/tunnel_lookup_test.go
- management/internals/modules/reverseproxy/proxy/proxy.go
- proxy/server.go
- infrastructure_files/getting-started.sh
- shared/management/http/api/types.gen.go
- management/server/store/sql_store.go
- proxy/internal/restrict/restrict.go
- management/internals/modules/reverseproxy/proxy/manager_mock.go
- management/internals/modules/reverseproxy/service/service.go
- proxy/management_integration_test.go
- proxy/internal/auth/appsec_test.go
- proxy/lifecycle.go
- proxy/internal/auth/middleware_test.go
- management/internals/modules/reverseproxy/proxy/manager.go
- proxy/internal/appsec/client_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
proxy/internal/appsec/client.go (1)
373-391: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider reserving the actual body size, not always the full cap, from the shared Budget.
The oversize check above already guarantees
r.ContentLength <= c.maxBodyByteswhen known, yet the reservation always draws downc.maxBodyBytesregardless. Since net/http bounds reads on a Content-Length request body to the declared length, reservingmin(r.ContentLength, c.maxBodyBytes)when the length is known (falling back to the cap only for chunked/unknown-length bodies) would be equally safe and reduce contention on the shared Budget — which now also backs agent-network capture per the PR objectives.Proposed refactor
// A Content-Length over the cap is known to be too large before reading. if r.ContentLength > c.maxBodyBytes { return nil, BypassOversize, noopRelease, nil } - // Reserve the whole cap rather than the eventual length: the reservation - // has to be made before the body is read, and until then the only bound - // known is the cap. Skipping inspection when the pool is drained keeps a - // burst of large bodies from being an out-of-memory lever; the bypass is - // recorded so the gap in coverage is visible. + // Reserve only what the body can actually consume: the declared + // Content-Length when known (net/http bounds reads to it), otherwise the + // cap for chunked/unknown-length bodies. Skipping inspection when the pool + // is drained keeps a burst of large bodies from being an out-of-memory + // lever; the bypass is recorded so the gap in coverage is visible. + reserve := c.maxBodyBytes + if r.ContentLength >= 0 && r.ContentLength < reserve { + reserve = r.ContentLength + } release = noopRelease if c.budget != nil { - if !c.budget.Acquire(c.maxBodyBytes) { + if !c.budget.Acquire(reserve) { c.logger.Debugf("appsec buffer budget exhausted, inspecting headers and URI only") return nil, BypassBudget, noopRelease, nil } var once sync.Once - release = func() { once.Do(func() { c.budget.Release(c.maxBodyBytes) }) } + release = func() { once.Do(func() { c.budget.Release(reserve) }) } }🤖 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 `@proxy/internal/appsec/client.go` around lines 373 - 391, Update the budget reservation logic in the body-reading flow to reserve the known Content-Length when it is nonnegative, using c.maxBodyBytes only for unknown or chunked bodies. Preserve the existing oversize rejection, BypassBudget behavior, and release callback, ensuring the acquired amount and released amount use the same computed reservation.
🤖 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 `@proxy/lifecycle.go`:
- Around line 192-194: Update the server configuration construction in New to
forward cfg.CrowdSecAppSecMaxConcurrent and cfg.MiddlewareCaptureBudgetBytes
into the corresponding serverConfig fields, alongside the existing
CrowdSecAppSec settings. Preserve the explicitly configured concurrency and
capture-budget values instead of leaving them at zero defaults.
---
Nitpick comments:
In `@proxy/internal/appsec/client.go`:
- Around line 373-391: Update the budget reservation logic in the body-reading
flow to reserve the known Content-Length when it is nonnegative, using
c.maxBodyBytes only for unknown or chunked bodies. Preserve the existing
oversize rejection, BypassBudget behavior, and release callback, ensuring the
acquired amount and released amount use the same computed reservation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a877343-4a1f-4815-a5e0-903093c3d541
⛔ Files ignored due to path filters (1)
shared/management/proto/proxy_service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (38)
infrastructure_files/getting-started.shmanagement/internals/modules/reverseproxy/domain/domain.gomanagement/internals/modules/reverseproxy/domain/manager/api.gomanagement/internals/modules/reverseproxy/domain/manager/manager.gomanagement/internals/modules/reverseproxy/domain/manager/manager_test.gomanagement/internals/modules/reverseproxy/proxy/manager.gomanagement/internals/modules/reverseproxy/proxy/manager/manager.gomanagement/internals/modules/reverseproxy/proxy/manager/manager_test.gomanagement/internals/modules/reverseproxy/proxy/manager_mock.gomanagement/internals/modules/reverseproxy/proxy/proxy.gomanagement/internals/modules/reverseproxy/service/manager/api.gomanagement/internals/modules/reverseproxy/service/manager/manager.gomanagement/internals/modules/reverseproxy/service/service.gomanagement/internals/modules/reverseproxy/service/service_test.gomanagement/internals/shared/grpc/proxy.gomanagement/server/store/sql_store.gomanagement/server/store/sql_store_service_test.gomanagement/server/store/store.gomanagement/server/store/store_mock.goproxy/cmd/proxy/cmd/root.goproxy/internal/appsec/body.goproxy/internal/appsec/client.goproxy/internal/appsec/client_test.goproxy/internal/auth/appsec_test.goproxy/internal/auth/header.goproxy/internal/auth/middleware.goproxy/internal/auth/middleware_test.goproxy/internal/auth/oidc.goproxy/internal/auth/tunnel_lookup_test.goproxy/internal/middleware/bodytap/request.goproxy/internal/netutil/upgrade.goproxy/internal/restrict/restrict.goproxy/lifecycle.goproxy/management_integration_test.goproxy/server.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/proxy_service.proto
🚧 Files skipped from review as they are similar to previous changes (34)
- proxy/internal/netutil/upgrade.go
- management/internals/modules/reverseproxy/domain/manager/manager_test.go
- proxy/internal/auth/oidc.go
- management/server/store/store_mock.go
- proxy/internal/auth/header.go
- management/internals/modules/reverseproxy/domain/manager/manager.go
- proxy/internal/middleware/bodytap/request.go
- shared/management/http/api/types.gen.go
- management/internals/modules/reverseproxy/proxy/manager.go
- management/internals/modules/reverseproxy/proxy/manager/manager_test.go
- management/server/store/store.go
- management/internals/modules/reverseproxy/service/manager/api.go
- proxy/internal/auth/tunnel_lookup_test.go
- shared/management/proto/proxy_service.proto
- management/internals/modules/reverseproxy/proxy/proxy.go
- management/internals/modules/reverseproxy/domain/domain.go
- management/server/store/sql_store_service_test.go
- proxy/internal/appsec/body.go
- management/internals/modules/reverseproxy/proxy/manager/manager.go
- management/internals/shared/grpc/proxy.go
- management/internals/modules/reverseproxy/service/service_test.go
- management/internals/modules/reverseproxy/service/manager/manager.go
- proxy/management_integration_test.go
- proxy/internal/auth/appsec_test.go
- proxy/internal/restrict/restrict.go
- infrastructure_files/getting-started.sh
- shared/management/http/api/openapi.yml
- proxy/server.go
- management/internals/modules/reverseproxy/service/service.go
- management/internals/modules/reverseproxy/proxy/manager_mock.go
- management/server/store/sql_store.go
- proxy/internal/appsec/client_test.go
- proxy/internal/auth/middleware_test.go
- proxy/internal/auth/middleware.go
lixmal
force-pushed
the
reverse-proxy-crowdsec-appsec
branch
from
July 27, 2026 17:42
dcb942a to
26f88d2
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
proxy/internal/appsec/client.go (1)
504-520: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCookie redaction still only covers the first
Cookieheader value.This exact pattern was flagged in a prior review round (marked "Addressed in commit dcb942a"), but the code shown here still uses
dst.Get("Cookie"), which only returns the first value when multipleCookieheader entries exist. Any laterCookievalue — potentially the proxy's own session cookie — is mirrored to the AppSec engine unredacted.🔒 Proposed fix to redact every Cookie header value
- if cookie := dst.Get("Cookie"); cookie != "" { - if redacted := redactCookieHeader(cookie, req.RedactCookies); redacted != cookie { - dst.Set("Cookie", redacted) - } + if cookies := dst.Values("Cookie"); len(cookies) > 0 { + for i, cookie := range cookies { + cookies[i] = redactCookieHeader(cookie, req.RedactCookies) + } + dst["Cookie"] = cookies }🤖 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 `@proxy/internal/appsec/client.go` around lines 504 - 520, Update redactSecrets to process every Cookie header value rather than using dst.Get("Cookie") once. Iterate over dst.Values("Cookie"), apply redactCookieHeader to each value with req.RedactCookies, and replace the Cookie header values while preserving unchanged values and redacting any sensitive cookies in later entries.
🧹 Nitpick comments (1)
proxy/internal/appsec/client.go (1)
217-224: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winIdle connection pool doesn't scale with configured concurrency.
MaxIdleConnsPerHostis fixed at 32 whileMaxConcurrentdefaults to 256 and can be configured up to 4096. Once in-flight inspections exceed 32, excess connections are torn down instead of reused, adding a fresh handshake per burst request on this hot path.♻️ Suggested fix: scale the pool with the configured bound
+ idleConnsPerHost := maxConcurrent + if idleConnsPerHost <= 0 || idleConnsPerHost > 256 { + idleConnsPerHost = 256 + } return &Client{ ... http: &http.Client{ Timeout: timeout, Transport: &http.Transport{ MaxIdleConns: 100, - MaxIdleConnsPerHost: 32, + MaxIdleConnsPerHost: idleConnsPerHost, IdleConnTimeout: 90 * time.Second, }, }, }, nil🤖 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 `@proxy/internal/appsec/client.go` around lines 217 - 224, Update the http.Transport configuration in the client constructor to derive MaxIdleConnsPerHost from the configured MaxConcurrent bound instead of keeping it fixed at 32, ensuring the idle pool scales with allowed inspection concurrency while preserving the existing transport settings.
🤖 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.
Duplicate comments:
In `@proxy/internal/appsec/client.go`:
- Around line 504-520: Update redactSecrets to process every Cookie header value
rather than using dst.Get("Cookie") once. Iterate over dst.Values("Cookie"),
apply redactCookieHeader to each value with req.RedactCookies, and replace the
Cookie header values while preserving unchanged values and redacting any
sensitive cookies in later entries.
---
Nitpick comments:
In `@proxy/internal/appsec/client.go`:
- Around line 217-224: Update the http.Transport configuration in the client
constructor to derive MaxIdleConnsPerHost from the configured MaxConcurrent
bound instead of keeping it fixed at 32, ensuring the idle pool scales with
allowed inspection concurrency while preserving the existing transport settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dabc1632-199e-4b60-9219-153105916142
⛔ Files ignored due to path filters (1)
shared/management/proto/proxy_service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (39)
infrastructure_files/getting-started.shmanagement/internals/modules/reverseproxy/domain/domain.gomanagement/internals/modules/reverseproxy/domain/manager/api.gomanagement/internals/modules/reverseproxy/domain/manager/manager.gomanagement/internals/modules/reverseproxy/domain/manager/manager_test.gomanagement/internals/modules/reverseproxy/proxy/manager.gomanagement/internals/modules/reverseproxy/proxy/manager/manager.gomanagement/internals/modules/reverseproxy/proxy/manager/manager_test.gomanagement/internals/modules/reverseproxy/proxy/manager_mock.gomanagement/internals/modules/reverseproxy/proxy/proxy.gomanagement/internals/modules/reverseproxy/service/manager/api.gomanagement/internals/modules/reverseproxy/service/manager/manager.gomanagement/internals/modules/reverseproxy/service/service.gomanagement/internals/modules/reverseproxy/service/service_test.gomanagement/internals/shared/grpc/proxy.gomanagement/server/store/sql_store.gomanagement/server/store/sql_store_service_test.gomanagement/server/store/store.gomanagement/server/store/store_mock.goproxy/cmd/proxy/cmd/root.goproxy/internal/appsec/body.goproxy/internal/appsec/client.goproxy/internal/appsec/client_test.goproxy/internal/auth/appsec_test.goproxy/internal/auth/header.goproxy/internal/auth/middleware.goproxy/internal/auth/middleware_test.goproxy/internal/auth/oidc.goproxy/internal/auth/tunnel_lookup_test.goproxy/internal/middleware/bodytap/request.goproxy/internal/netutil/upgrade.goproxy/internal/restrict/restrict.goproxy/lifecycle.goproxy/lifecycle_test.goproxy/management_integration_test.goproxy/server.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/proxy_service.proto
🚧 Files skipped from review as they are similar to previous changes (32)
- management/internals/modules/reverseproxy/domain/manager/api.go
- proxy/internal/auth/oidc.go
- management/internals/modules/reverseproxy/domain/domain.go
- management/server/store/store_mock.go
- management/internals/modules/reverseproxy/domain/manager/manager_test.go
- management/internals/modules/reverseproxy/proxy/manager.go
- proxy/internal/netutil/upgrade.go
- shared/management/proto/proxy_service.proto
- proxy/internal/restrict/restrict.go
- management/internals/modules/reverseproxy/proxy/proxy.go
- proxy/internal/auth/header.go
- management/internals/modules/reverseproxy/domain/manager/manager.go
- shared/management/http/api/types.gen.go
- management/internals/modules/reverseproxy/service/manager/api.go
- infrastructure_files/getting-started.sh
- management/internals/modules/reverseproxy/proxy/manager/manager.go
- proxy/management_integration_test.go
- proxy/internal/auth/tunnel_lookup_test.go
- proxy/lifecycle.go
- proxy/cmd/proxy/cmd/root.go
- proxy/internal/appsec/body.go
- proxy/internal/middleware/bodytap/request.go
- shared/management/http/api/openapi.yml
- management/internals/modules/reverseproxy/service/service.go
- management/server/store/sql_store.go
- proxy/internal/auth/appsec_test.go
- management/internals/modules/reverseproxy/service/service_test.go
- proxy/server.go
- management/internals/modules/reverseproxy/proxy/manager_mock.go
- proxy/internal/auth/middleware_test.go
- proxy/internal/auth/middleware.go
- proxy/internal/appsec/client_test.go
lixmal
force-pushed
the
reverse-proxy-crowdsec-appsec
branch
3 times, most recently
from
July 28, 2026 08:52
9770039 to
663468e
Compare
lixmal
force-pushed
the
reverse-proxy-crowdsec-appsec
branch
from
August 19, 2026 10:05
af1f94b to
5885fcc
Compare
|
Collaborator
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
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.
Describe your changes
Adds CrowdSec AppSec (WAF) request inspection to the reverse proxy. Where the existing CrowdSec integration asks whether the client IP is known bad, this asks whether the request itself is an attack: each HTTP request on an opted-in service is mirrored to the Security Engine's AppSec endpoint, which answers allow, ban, or captcha. It is off by default and enabled per service.
appsec_mode(off/enforce/observe) to service access restrictions, HTTP services only, alongside the existing CrowdSec modesupports_appseccluster capability so the option only appears where an endpoint is configured, which keeps this opt-in for self-hosted and bring-your-own-proxy deploymentsIssue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
netbirdio/docs#889
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
off,observe, andenforcemodes (enforce can block; observe records verdicts).appsec_modeto access restrictions plus capability reporting (supports_appsec) for proxy clusters and reverse-proxy domains.Bug Fixes