Add block-outcome telemetry for Netty blocking enforcement failures - #12316
Add block-outcome telemetry for Netty blocking enforcement failures#12316jandro996 wants to merge 3 commits into
Conversation
Report failed AppSec block-response commits to WafMetricCollector so appsec.waf.requests carries an accurate block_failure tag, scoped to Netty as the first framework. AppSecContext gains a module-boundary-safe reportBlockFailure() that AppSecRequestContext delegates to the existing setWafRequestBlockFailure() field.
jandro996
added
type: feature
|
@codex review |
![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: 2c99b3ee39
ℹ️ 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".
|
🎯 Code Coverage (details) 🔗 Commit SHA: 4a2bd4d | Docs | View more details | Give us feedback! |
NettyBlockResponseFunction.tryCommitBlockingResponse() now tracks whether a blocking response was already initiated for the current request. HttpPostRequestDecoderInstrumentation's advice can invoke tryBlock() multiple times across separate decoder invocations for the same request (e.g. one per multipart chunk); once an earlier call already committed the block successfully, ServerRequestContext.isPending() returns false for later calls, which previously caused a spurious block_failure to be reported even though the block had actually succeeded.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdd1140d3d
ℹ️ 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".
| () -> | ||
| commitBlockingResponse( | ||
| segment, statusCode, templateType, extraHeaders, securityResponseId)); | ||
| blockingResponseInitiated = true; |
There was a problem hiding this comment.
When this method is called outside the channel's event loop and the queued commitBlockingResponse later returns false—for example, because the request ceased to be pending before the task ran or the required tracing handler is absent—this assignment still marks the block as initiated. The caller therefore receives true, subsequent attempts short-circuit at line 149, and NettyMultipartHelper neither retries nor reports block_failure. Fresh evidence in this revision is the new unconditional assignment after scheduling; update the state and failure telemetry from the queued task based on the actual commit result.
Useful? React with 👍 / 👎.
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
An off-event-loop block task can fail after Netty accepts it. The new flag still marks it as complete, so later block attempts return success without enforcement or failure telemetry.
🤖 Datadog Autotest · Commit cdd1140 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
NettyBlockResponseFunction.tryCommitBlockingResponse() marked blockingResponseInitiated = true immediately after scheduling the off-event-loop commitBlockingResponse() call, without waiting for it to actually run. If that async commit failed once executed, the failure was silently swallowed and every later tryBlock() call for the same request would short-circuit to true, hiding the failure from block_failure telemetry. The flag is now set only inside the scheduled task, after commitBlockingResponse() actually returns true, matching the same-thread branch's existing behavior.
|
Good catch. Fixed in 4a2bd4d: the off-event-loop branch of |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
What Does This Do
void reportBlockFailure();toAppSecContext(internal-api), alongsideisManuallyKept().reportBlockFailure()inAppSecRequestContextby delegating to the existingsetWafRequestBlockFailure()volatile field — no new state added.NettyMultipartHelper#tryBlock(): captures the boolean returned byBlockResponseFunction#tryCommitBlockingResponse(...); onfalse, resolvesAppSecContextviactx.getData(RequestContextSlot.APPSEC)(the canonicalinstanceofpattern already used byLambdaAppSecHandler) and callsreportBlockFailure(). The method still throwsBlockingExceptionunconditionally, unchanged.NettyBlockResponseFunction#tryCommitBlockingResponse()idempotent per request: a single request can trigger multiple blocking evaluations (e.g. onetryBlock()call per multipart chunk inHttpPostRequestDecoderInstrumentation's advice). Once an earlier call has already committed the block successfully,ServerRequestContext.isPending()legitimately turnsfalsefor the request — without this guard, a later call would misread that as a commit failure and report a spuriousblock_failurefor a block that actually succeeded.MaybeBlockResponseHandler#write()'swriteAndFlushlistener flagging the async post-commit write-failure path as a known, currently-unreported gap for a future framework PR to pick up.WAFModuleSpecification.groovyassertingWafMetricCollector.raspRuleMatch(RuleType, boolean blocked)is called with the correctblockedvalue after RASP action processing.NettyMultipartHelperBlockFailureTest.java(new JUnit 5 test) verifyingreportBlockFailure()fires whentryCommitBlockingResponsereturnsfalse, and does NOT fire when it returnstrue.Scope
This is the first PR of a per-framework rollout — only Netty is covered here. Other frameworks (Tomcat, Jetty, Vert.x, Undertow, Grizzly, Play, Akka-http, Ratpack, RESTEasy, Jersey, Liberty, Spring-webmvc, commons-fileupload, okhttp) are explicitly out of scope and will get their own follow-up PRs. The async post-commit write failure in
MaybeBlockResponseHandler(channel write happens after the block decision, listener-driven) is also deliberately deferred — wiring it up requires investigating the ordering between span/request close and the channel listener to avoid introducing a race.Motivation
appsec.waf.requestscurrently has no signal for when an AppSec block action was decided but its commit to the client actually failed (e.g. the response was already committed, or the channel write failed). This closes that gap for the synchronous Netty commit path, so failed block enforcement becomes visible in telemetry instead of silently looking like a successful block.Additional Notes
None.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: APPSEC-62696
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.