Repository navigation
fix(contrib/go-chi): set the AppSec route and path params after chi routing - #5847
Draft
RomainMuller wants to merge 23 commits into
Draft
RomainMuller wants to merge 23 commits into
RomainMuller wants to merge 23 commits into
Conversation
…r chi routing With r.Use(Middleware()) before routing, AppSec read the route and the path parameters before chi routed the request. Look the route up before the handler, run the WAF again with the final route and parameters after it, and track when the response is sent.
github-actions
Bot
added
the
apm:ecosystem
…hi routing The same change as for chi.v5: look the route up before the handler, run the WAF with the final route and parameters after it, and track when the response is sent.
Contributor
Config Audit |
…or each router setup
Contributor
|
…the same-type rules
…eal server, add benchmarks
…the writer contract
…tatus of a flushed response Address the review of the first version: - the early lookup adds to the path parameters that chi already knows when the tracer runs, and never replaces them (except the "*" of a mount), so a block that works without the lookup still works when a middleware rewrites the route path before a tracer in a mount with a parameter; - Status() keeps the status of the first successful flush, also when the handler calls WriteHeader after a flush that bypassed chi's writer; - go generate fails when the generator fails; - godoc: failed flush, ordinary mounts and parameters that AppSec cannot remove; - tests: same-type rules for chi v1, panic without late run, raw and empty path, concurrent requests, bounded cycle, HTTP/2 check, stale parameters.
…utes chi matched The merge of the known parameters (previous commit) had edge cases with duplicate names and catch-all routes. Use a simpler rule: a prediction that does not extend the route patterns that chi already matched contradicts chi (a parent mount was routed with a changed route path), so keep what chi matched, as before the lookup existed. Also add the same-type rule tests to chi v1.
…h the lookup Same route patterns do not mean the same parameters: a parent that rewrites the route path can route a mount with another value of its parameter. Drop the prediction when the parameters of the patterns that chi matched differ. Document the handling of the mount wildcard.
…y matched Round 1 of the review found requests that AppSec blocked before the lookup existed and that were not blocked anymore: - a name used at several mount levels: the prediction (last value) replaced the value that chi matched; chi.v5 also changed its early read from the first to the last value; - r.With(Middleware()).Mount: the lookup reset the "*" of the mount before the rule could check it. The early parameters are now the ones that chi matched (first value for chi.v5, as before). The prediction only adds names; if it has another value for a known name, it is dropped. The known "*" stays. The final report keeps the last value. Add the preservation tests, the missing assertions of the review (missing_route after a panic, route and server log of the hidden-parent rows, the methods of the 24 tracker types) and a benchmark with 4 nested tracers and a long path.
…th the known ones
… chi matched Round 2 of the review found 2 cases where a rule that does not block on main blocked, or a rule that blocks on main did not: - the late run replaced a parameter that chi matched before the handler (a name used at several mount levels), so a rule with a condition on the response header first lost its match, and the WAF mixed the 2 values across the runs; - the early check kept the known '*' of a mount and added the names of the lookup, a set that chi never has. The late run now adds the final parameters to the known ones, and reports only the route when they contradict them. The known '*' is a contradiction too. Add the compatibility tests, the Push call check and the godoc.
…ers of a request An outer tracer reported its final parameters after the inner tracer returned and replaced the value that the inner tracer kept (the same value as on main), so a response rule lost its match. All the tracers of a request now share the path parameters that AppSec gave to the WAF as baseline input: a late run or an early prediction that contradicts a value in it reports only the route; a tracer still adds new names. A tracker that cannot reach the shared input of the outer tracers (the writer chain reaches an outer tracker but the request context does not) gives no new parameters. Add the nested tests and the shared input tests.
…acer cannot share it A tracer that finds an outer tracer through the writer chain but not through the request context cannot share the baseline input of the outer tracers, but it still gives its known parameters to their WAF context. The outer late runs did not know it and could replace the value that it kept. Such a tracer now marks the shared input of the outer tracers that it finds (through the writer chain and the request context) as uncertain; they report only their route. The tracer itself keeps giving its known parameters. Document that baseline input is replaced at tracer entry and that predictions are not recorded, and that a rule can still match a predicted and a final value across 2 runs. Add the restored-context test and TestPredictionPhaseCombination.
The guard protects the baseline input that is already known when a late update happens. Document that it cannot undo an earlier WAF match (a later tracer that replaces the baseline input or sets the flag), that a tracer hidden through both the writer chain and the request context cannot be flagged, and that other AppSec integrations are not covered. Add the tests that document the first two limits.
… tests The gzip bytes of a small body depend on the Go version (go1.27 can store the body, go1.26 compresses it), so a test that looks for plain text in the body failed on CI. A client does not decompress a body that was flushed before the Content-Encoding header. Decompress the body when it is a gzip stream, and check the documented behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes the AppSec route and path parameters of
contrib/go-chi/chi.v5andcontrib/go-chi/chiwhenMiddleware()is added withUsebefore routing:http.routevalue) to AppSec; it was empty.Routes.Match) and gives its path parameters to the WAF.httpsec.RouteMatchedwith the final route and parameters, guarded by an input shared by all tracers of the request.Motivation
APPSEC-69059.
api_security.missing_routecounted almost every 2xx chi request, and all endpoints shared 1 API Security sampling key per method and status.Behavior changes
block_failure:true; a redirect is not a block.Status()returns 200 afterFlush()withoutWriteHeader(chi reports 0); the span status does not change.FlushError()method only if chi's writer has noFlush()but a writer under it can flush.Known limits
main200, this PR 403).main403, this PR 200).Details and the writer contract:
Middlewaregodoc.Notes for the API Security team
_dd.appsec.s.req.params.DD_APM_TRACING_ENABLED=false, at most 1 kept trace per (method, route, status) every 30 s per process.api_security.missing_routefor chi to fall to almost 0.waf.requestswithrequest_blocked:true(early blocks) andblock_failure:true.How to review
chi.v5/chi.go:reportedRouteand theMiddlewaregodoc.*/appsec.go: early lookup, shared input, lateRouteMatched.*/writer_appsec.go,make_writer_appsec.go: the commit tracker (generated types).*_appsec_test.go: route per setup, real server and WAF, preservation of existing blocks.Tests and benchmarks
65148c8b2, except the guards for behavior that must not change and the documented limits.go test ./...in both modules;go generateleaves no diff.Allocations per request,
mainvs this PR (benchstat, 10 runs):Times are noisy on the machine used; allocations are the reliable numbers.
Reviewer's Checklist
make lintlocally.make testlocally.make generatelocally.make fix-moduleslocally.