feat(appsec): upgrade to go-libddwaf v5 with subcontext-based ephemeral runs#4938
Conversation
Mechanical import-path rewrite from go-libddwaf/v4 to /v5 across the root module (incl. /timer, /waferrors subpackages). API breakages introduced by v5 are intentionally left for subsequent commits in this series.
…waf v5
The flat {key_regex,value_regex} shape under obfuscator/config did not apply
redaction in v5; the C WAF expects the config nested under an "obfuscator"
key. Verified by the end-to-end TestWAF/obfuscation redaction assertion.
- wafmanager: wrap WAF build error with telemetrylog.NewSafeError (gocritic) - metrics: simplify composite literals in subcontext duration map (gofmt) - roundtripper subcontext test: justified //nolint:bodyclose (false positive)
github-actions
Bot
added
the
apm:ecosystem
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57af60e62d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if metrics := sub.contextOp.GetMetricsInstance(); metrics != nil { | ||
| metrics.AddSubcontextStats(sub.scope, sub.subcontext.Truncations(), sub.subcontext.Timer.Stats()) |
There was a problem hiding this comment.
When a shared subcontext is used for more than one run, this records only externalTimerStats[sub.scope] for the last run's scope. In the real HTTP round-tripper path, a request-side SSRF block now calls op.Finish(RoundTripOperationRes{}), which fires the response finish listener with empty WAF-scope data before Close; that overwrites the request run's RASP scope, so the accumulated RASP subcontext duration is not added to rasp.duration_ext and may be reported under WAF or dropped. Aggregate all scopes present in subcontext.Timer.Stats() instead of using a single mutable sub.scope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The new file is unnecessary, merge it into another test file
| if metrics == nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
This one is very problematic
BenchmarksBenchmark execution time: 2026-06-24 13:36:47 Comparing candidate commit 5c54ac7 in PR branch Found 1 performance improvements and 5 performance regressions! Performance is the same for 291 metrics, 2 unstable metrics, 1 flaky benchmarks without significant changes.
|
…dressData Replace the per-run Ephemeral boolean dispatch with explicit entry points: RunSimple (persistent -> request Context) and RunSimpleSubcontext (ephemeral -> short-lived Subcontext). gRPC message monitors now call RunSimpleSubcontext; HTTP body monitors stay on RunSimple. addresses.RunAddressData becomes a type alias of libddwaf.RunAddressData (Data + TimerKey), dropping the Ephemeral field, the hasPersistent/hasEphemeral bookkeeping, the Build() mix panic, and ToLibddwaf(). RASP rule-type detection keys off TimerKey==RASPScope. skipRASPRuleAfterRequest now lives only on the subcontext path, since RASP always runs in subcontexts. Addresses PR review feedback (remove Ephemeral; embed/alias RunAddressData; RunSimpleSubcontext; relocate RASP skip). Tests updated to the new API and guarded with libddwaf.Usable(); round-trip subcontext test merged into roundtripper_test.go.
runWAF returned early on a nil metrics instance before sending action events, absorbing derivatives, applying Keep, and emitting the security event - so a nil metrics instance silently dropped blocking. Reorder so action/blocking/ derivative handling always runs and only the metrics calls are guarded.
A shared subcontext (SSRF round trip) recorded only the last run's scope via a single mutable field, so a WAF-scope response finish on the block path dropped the RASP external duration. AddSubcontextStats now iterates the known scopes and attributes each from the subcontext timer stats. Adds a regression test plus nil-context and idempotent-Close coverage.
…RUnlock Guarantee the shared round-trip subcontext is closed even if FinishOperation panics, and defer the read lock release in WAFManager.NewHandle.
The v2fix dev tool transitively requires go-libddwaf/v4; commit the indirect require so 'make fix-modules' is a no-op and the check-modules CI gate passes. The tracer modules themselves remain v5-only.
Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>
… Runner rc.2 folds a subcontext's per-scope timer durations and truncations into the parent Context on Close, and exports libddwaf.Runner. Drop the manual accumulator (AddSubcontextStats, subcontextExternalDurations, MergedTruncations, ExternalDuration and the Submit subcontext loop) and the local wafRunner interface; onFinish now reads ctx.Timer.Stats()/ctx.Truncations() which already include subcontext contributions. Tests assert via the rolled-up Context.
make fix-modules + orchestrion _integration direct bump to rc.2. Indirect version/go.sum updates across all modules and go.work.sum.
…liottness/go-libdwaf-v5
| github.com/DataDog/datadog-agent/pkg/trace/traceutil v0.79.0 // indirect | ||
| github.com/DataDog/datadog-go/v5 v5.8.3 // indirect | ||
| github.com/DataDog/go-libddwaf/v4 v4.10.0 // indirect | ||
| github.com/DataDog/go-libddwaf/v5 v5.0.0-rc.1 // indirect |
There was a problem hiding this comment.
I guess before merging it might be nice to release the v5.0.0 tag
There was a problem hiding this comment.
I want to dogfood the PR before doing the final release. I will tag go-libddwaf@v5.0.0 after dd-trace-go@v2.10.0-rc.1 is published and dogfooded
Benchmark analysisI investigated every benchmark flagged by the benchmarking platform (candidate
1.
|
| function | instr count (v4 / v5) | result |
|---|---|---|
proto.(*MarshalOptions).Size |
32 / 32 | identical stream |
impl.(*MessageInfo).sizePointer |
276 / 276 | identical stream |
impl.(*MessageInfo).sizePointerSlow |
224 / 224 | identical stream |
impl.sizeMessageInfo |
32 / 32 | identical stream |
The instruction + source-line streams are identical; the only differences are pure relocations (e.g. proto.Size moved 0x10046d610→0x10046d790; an ADRP data reference and the CALL runtime.morestack displacement shift accordingly). Same code — just placed a few hundred bytes later.
Does not reproduce off-amd64. Interleaved A/B (n=12, benchstat) on an Apple M4 Max (darwin/arm64) shows no regression — the candidate is marginally faster:
OTLPProtoSize/1span-16 234.2n ± 1% 234.2n ± 2% ~ (p=0.702)
OTLPProtoSize/10spans-16 1.903µ ± 1% 1.872µ ± 2% -1.63% (p=0.037)
OTLPProtoSize/100spans-16 18.45µ ± 1% 18.27µ ± 2% ~ (p=0.410)
OTLPProtoSize/1000spans-16 182.9µ ± 2% 181.8µ ± 1% ~ (p=0.114)
geomean 6.227µ 6.178µ -0.79%
This is textbook layout-induced measurement bias (Mytkowicz et al., "Producing Wrong Data Without Doing Anything Obviously Wrong!", ASPLOS 2009): relinking unrelated code shifts the alignment of a tiny zero-alloc loop, which amd64 front-ends (uop cache / 16–32B fetch windows) are sensitive to. Corroborating signs: BenchmarkOTLPProtoMarshal (same package, calls Size internally) was not flagged, and the relative effect shrinks as the workload grows (8.4% @ 1span → 2.6% @ 1000spans), i.e. a fixed front-door perturbation diluted by real work. It carries no production cost and will drift/vanish on the next unrelated commit.
2. BenchmarkSampleWAFContext — the +6 allocations and the −14% are the same change (net win)
This benchmark drives the go-libddwaf v5 API directly (NewContext + 2× Run + Close). Both reported items come from one design change: v5's per-scope timer model. The candidate adds timer.WithComponents(addresses.Scopes[:]...) (2 scopes: rasp, waf) and the new RunAddressData{Data, Scope} model, replacing v4's single budget timer + Persistent/Ephemeral channel. That per-scope timing is exactly what makes the run faster — so the handful of extra allocations is the cost of a ~14–19% speedup. Reproduced on arm64:
SampleWAFContext-16 sec/op: 66.12µ → 53.42µ -19.20% (p=0.000)
SampleWAFContext-16 B/op: 9.425Ki → 9.230Ki -2.07% (p=0.000) ← bytes go DOWN
SampleWAFContext-16 allocs: 162 → 167 +3.09% (p=0.000) ← +5 (report: +6)
Faster, fewer bytes, +5/6 small allocations — inherent to the go-libddwaf v5 API and a favorable trade.
Bottom line
Nothing here needs fixing. The WAF result is a net improvement, and the OTLP items are alignment/measurement bias (proto.Size is provably the same machine code, doesn't reproduce off-amd64). If the OTLP noise on future reports is bothersome, the realistic lever is marking BenchmarkOTLPProtoSize flaky rather than chasing a non-existent code regression.
Methodology: clean-worktree A/B of the exact report commits, go test -c binaries run interleaved with benchstat (n=12); machine-code comparison via go tool nm / go tool objdump. Repro host is darwin/arm64 (Apple M4 Max); the platform runs linux/amd64, which is the crux of the OTLP finding.
Config Audit |
### What does this PR do? Merges WAF attribute derivatives with **first-write-wins** in `AbsorbDerivatives` so a uniform-status redirect chain reports the **first** hop's `appsec.api.redirection.*` value instead of the last. It also enables the `APPSEC_RASP_NON_BLOCKING` scenario in the system-tests CI matrix (across all weblog variants, matching `APPSEC_RASP`) so this stays covered. ### Motivation #4938 moved per-hop downstream request/response WAF evaluation from the persistent request context to per-roundtrip **ephemeral subcontexts**. The persistent context deduplicates attribute generation within a request, so a redirection attribute rule matching the same status (e.g. 302) on every hop produced its derivative only once (the first hop). Ephemeral subcontexts don't share that dedup, so the rule re-fires on every hop and `AbsorbDerivatives`' last-write-wins kept the **final** hop. This regressed system-tests `Test_API10_redirect_status::test_api10_redirect` on `golang@2.10.0-dev` (bisected to `40280d189`): `appsec.api.redirection.move_target` became `/mirror/200` (last hop) instead of `/redirect?totalRedirects=2` (first hop). The fix restores the pre-#4938 "first discovery wins" invariant at the derivative merge boundary while preserving #4938's per-hop ephemeral evaluation (RASP SSRF still fires on every hop). The existing blocked-response-schema skip is kept ahead of the new guard. ### Testing - New `TestAppsecHTTP30XRedirectChainKeepsFirstHop` (uniform-302 chain) — verified it FAILS without the fix and PASSES with it. The existing `TestAppsecHTTP30X` used distinct 308/307 per hop, so it never collided on a single key. - New `AbsorbDerivatives` unit tests (first-write-wins + blocked-response-schema skip preserved). - Full `contrib/net/http` appsec suite and `internal/appsec/emitter/waf` package pass; `go vet` clean. ### Reviewer's Checklist - [x] Changed code has unit tests for its functionality at or near 100% coverage. - [x] [System-Tests](https://github.com/DataDog/system-tests/) covering this feature have been enabled (`APPSEC_RASP_NON_BLOCKING` added to the CI matrix). - [ ] There is a benchmark for any new code, or changes to existing code. - [ ] If this interacts with the agent in a new way, a system test has been added. - [x] New code is free of linting errors. - [x] New code doesn't break existing tests. - [ ] Add an appropriate team label so this PR gets put in the right place for the release notes. - [x] All generated files are up to date. - [ ] Non-trivial go.mod changes reviewed by @DataDog/dd-trace-go-guild (no go.mod changes). Co-authored-by: eliott.bouhana <eliott.bouhana@datadoghq.com>
### What does this PR do? Merges WAF attribute derivatives with **first-write-wins** in `AbsorbDerivatives` so a uniform-status redirect chain reports the **first** hop's `appsec.api.redirection.*` value instead of the last. It also enables the `APPSEC_RASP_NON_BLOCKING` scenario in the system-tests CI matrix (across all weblog variants, matching `APPSEC_RASP`) so this stays covered. ### Motivation #4938 moved per-hop downstream request/response WAF evaluation from the persistent request context to per-roundtrip **ephemeral subcontexts**. The persistent context deduplicates attribute generation within a request, so a redirection attribute rule matching the same status (e.g. 302) on every hop produced its derivative only once (the first hop). Ephemeral subcontexts don't share that dedup, so the rule re-fires on every hop and `AbsorbDerivatives`' last-write-wins kept the **final** hop. This regressed system-tests `Test_API10_redirect_status::test_api10_redirect` on `golang@2.10.0-dev` (bisected to `40280d189`): `appsec.api.redirection.move_target` became `/mirror/200` (last hop) instead of `/redirect?totalRedirects=2` (first hop). The fix restores the pre-#4938 "first discovery wins" invariant at the derivative merge boundary while preserving #4938's per-hop ephemeral evaluation (RASP SSRF still fires on every hop). The existing blocked-response-schema skip is kept ahead of the new guard. ### Testing - New `TestAppsecHTTP30XRedirectChainKeepsFirstHop` (uniform-302 chain) — verified it FAILS without the fix and PASSES with it. The existing `TestAppsecHTTP30X` used distinct 308/307 per hop, so it never collided on a single key. - New `AbsorbDerivatives` unit tests (first-write-wins + blocked-response-schema skip preserved). - Full `contrib/net/http` appsec suite and `internal/appsec/emitter/waf` package pass; `go vet` clean. ### Reviewer's Checklist - [x] Changed code has unit tests for its functionality at or near 100% coverage. - [x] [System-Tests](https://github.com/DataDog/system-tests/) covering this feature have been enabled (`APPSEC_RASP_NON_BLOCKING` added to the CI matrix). - [ ] There is a benchmark for any new code, or changes to existing code. - [ ] If this interacts with the agent in a new way, a system test has been added. - [x] New code is free of linting errors. - [x] New code doesn't break existing tests. - [ ] Add an appropriate team label so this PR gets put in the right place for the release notes. - [x] All generated files are up to date. - [ ] Non-trivial go.mod changes reviewed by @DataDog/dd-trace-go-guild (no go.mod changes). Co-authored-by: eliott.bouhana <eliott.bouhana@datadoghq.com>
4 participants
What & why
Upgrades dd-trace-go from
go-libddwaf/v4 v4.10.0togo-libddwaf/v5 v5.0.0-rc.2across the root module and all nested modules. v5 removed the per-runPersistent/Ephemeraldata channel, so previously-ephemeral WAF evaluations (RASP LFI/SQLi/SSRF/CMDi, gRPC request/response messages, GraphQL resolvers) now run on v5 subcontexts. Persistent runs stay on the mainContext.Design
SubcontextOperation(internal/appsec/emitter/waf/subcontext.go) wraps*libddwaf.Subcontext. Constructed viaContextOperation.NewSubcontextOp();Run/Closeare nil-safe no-ops when the WAF context is closed.Ephemeralflag. Dispatch is explicit at the call site:RunSimple(persistent →Context) vsRunSimpleSubcontext(ephemeral →Subcontext).addresses.RunAddressDatais now a type alias oflibddwaf.RunAddressData(Data+TimerKey).Subcontext.Close()/Context.Close()fold per-scope timer durations + truncations into the parentContext, soContextMetricsno longer keeps a manual subcontext accumulator —onFinishreadsctx.Timer.Stats()/ctx.Truncations()which already include subcontext data.libddwaf.Runneris used for the shared run helper (no local interface).SubcontextOperation.Run, not a new dispatch path.Decisions
SSRFRequestrule on the response run; encoded as a permanent regression test).context.Background()throughout (no request-context threading into the WAF).SubcontextOperationcoverage (ephemeral routing, close/no-leak, SSRF freshness, unsupported-address filtering, nil-context safety, idempotent Close).Notable fixes (from review)
deferthe round-trip subcontext Close;deferthe read-lock release inWAFManager.NewHandle.obfuscatorso v5 redaction actually applies (end-to-end redaction test).tools/v2fixgo.mod tidied (go-libddwaf v4 indirect) somake fix-modulesis a no-op.Verification
go build ./...(root +internal/orchestrion/_integration) clean;make fix-modulesno-op;make generateclean.make test/appsec+go test ./internal/appsec/...green, including the new SubcontextOperation coverage.golangci-lintclean on the changed appsec packages.go-libddwaf/v4(outside the v2fix dev tool) and zerorc.1references.🤖 Generated with assistance; please review.