feat(aap): In App WAF support for lambda#7783
Conversation
Overall package sizeSelf size: 6.26 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.0.2 | 85.93 kB | 825.11 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BenchmarksBenchmark execution time: 2026-06-16 21:02:20 Comparing candidate commit a8d1734 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1937 metrics, 13 unstable metrics.
|
🎉 All green!🧪 All tests passed 🔗 Commit SHA: a8d1734 | Docs | Datadog PR Page | Give us feedback! |
| function reportAttack ({ events: attackData, actions }, req, rootSpan) { | ||
| if (!req) { | ||
| req = storage('legacy').getStore()?.req | ||
| } |
There was a problem hiding this comment.
i'm worried that in this function, if we don't pass the empty object fake "req", it will try to get it from the store, which would make this req variable here undefined, and crash later down the function because it's trying to access props in req (req.socket, req.body, etc).
The safety mecanism that was in place before was the rootSpan check. If req was missing, then rootSpan would also be missing, and thus the if(!rootSpan) return would protect us from undefined req.
But now that we can pass the rootSpan separately from req, this safety check is inoperant. Does that make sense ?
Can you prove to me that req can never be empty here ?
There was a problem hiding this comment.
Yep, your concern is legit: decoupling rootSpan from req breaks the implicit safety mechanism. But in the lambda path, req is never null or undefined in reportAttack (it's an invocationKey ({}), a truthy object flowing from lambda.js). The only place where req is null is in finishRequest.
I've added a test to check the WAF path with a strict req object that throws on any unaudited property access, making it fail if someone adds req.something without a guard in this path.
|
Overall this PR is a bit scary because it invalidates assumptions that were made during the entire evolution of our codebase. We should really be careful about these changes, a strong test suite, and proactive monitoring. For now I feel like the test suite is a bit light no ? |
CarlesDD
force-pushed
the
ccapell/APPSEC-60752/in-app-waf-port
branch
from
f4de885 to
e0b39e1
Compare
Agree this needs careful handling. I've pushed additional changes to strengthen the safety net:
|
CarlesDD
force-pushed
the
ccapell/APPSEC-60752/in-app-waf-port
branch
from
c83559f to
590897a
Compare
![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: 590897a5f9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Co-authored-by: simon-id <simon.id@datadoghq.com>
| const persistent = {} | ||
|
|
||
| if (path) { | ||
| persistent[addresses.HTTP_INCOMING_URL] = path |
There was a problem hiding this comment.
HTTP_INCOMING_URL is expected to be an URL not only the path. There is no way to get the whole URL in lambda?
There was a problem hiding this comment.
HTTP_INCOMING_URL maps to server.request.uri.raw, which is expected to be relative URI (i.e starting at path component, without scheme nor authority components)
| } | ||
|
|
||
| const invocationKey = {} | ||
| span._lambdaAppsecKey = invocationKey |
There was a problem hiding this comment.
I don't think we should store extra data in span, we usually create a WeakMap<span,data>.
And I think that you even could use the span itself as "invocationKey"
|
|
||
| waf.disposeContext(invocationKey) | ||
|
|
||
| Reporter.finishRequest(null, null, {}, undefined, span) |
There was a problem hiding this comment.
shouldn't we send invocationKey as req here to be able to report metrics correctly?
| } | ||
|
|
||
| if (clientIp) { | ||
| persistent[addresses.HTTP_CLIENT_IP] = clientIp |
There was a problem hiding this comment.
add span.setTag(HTTP_CLIENT_IP, clientIp) here and remove first block
| persistent[addresses.HTTP_INCOMING_COOKIES] = cookies | ||
| } | ||
|
|
||
| waf.run({ persistent }, invocationKey, undefined, span) |
There was a problem hiding this comment.
invocationKey gonna be always {} no?
| persistent[addresses.HTTP_INCOMING_RESPONSE_HEADERS] = filteredHeaders | ||
| } | ||
|
|
||
| if (Object.keys(persistent).length > 0) { |
There was a problem hiding this comment.
let's add a flag and make it true when we have something to add to persistent and then avoid Object.keys(persistent).length > 0.
We should fix this in all our appsec code to avoid the overhead of creating unnecessary objects
| if (!rootSpan) { | ||
| rootSpan = web.root(req) | ||
| } | ||
| if (!rootSpan) return |
There was a problem hiding this comment.
| if (!rootSpan) return | |
| if (!req || !rootSpan) return |
| : '{"triggers":' + attackDataStr + '}' | ||
|
|
||
| if (req.socket) { | ||
| if (req?.socket) { |
There was a problem hiding this comment.
No need to change it if we check !req before
| if (req?.socket) { | |
| if (req.socket) { |
|
|
||
| rootSpan.addTags(newTags) | ||
|
|
||
| if (!req) return |
There was a problem hiding this comment.
No need for this it if we check !req before
| rootSpan = web.root(req) | ||
| } | ||
|
|
||
| if (!rootSpan) return |
There was a problem hiding this comment.
reportAttributes doesn't use req after getting rootSpan, so the !req guard isn't needed here (unlike reportAttack, which accesses req.socket, web.getContext(req), and req.body after the guard.
There is also an existing test that calls reportAttributes without a req and expects addTags to be called, relying on web.root(undefined) resolving to a span. Adding !req would break that path.
4 participants
What does this PR do?
Adds AppSec support for AWS lambda to dd-trace-js by introducing DC handlers that allow the
datadog-lambda-jslayer to delegate WAF execution to the tracer.Changes:
datadog:lambda:start-invocationanddatadog:lambda:end-invocationdiagnostic channels with its subscribers (appsec/lambdja.js), which maps extracted HTTP data to WAF addresses, run the WAF and reports the results.reportMetrics,reportAttack,reportAttributes, andfinishRequestnow accept an optionalrootSpanparameter, falling back toweb.root(req)when not provided. This allows the Lambda handler to pass the span directly since there is noreqobject in LambdarootSpanthreading:waf/index.jsandwaf_context_wrapper.jsthreadrootSpanthrough the call chain so thatkeepTraceand reporter calls work correctly for LambdaMotivation
Porting the In-App WAF security product to AWS Lambda for the Node.js runtime. The Lambda layer extracts HTTP data from the event and dispatches it to the tracer's AppSec domain via DC.
This approach keeps all security logic (WAF execution, attack reporting, trace keep decisions) inside dd-trace-js, and the Lambda layer is only responsible for extracting raw HTTP data and publishing it.
Additional Notes
datadog-lambda-jspublishes to them.DD_APPSEC_ENABLEDenv var controls everything.APPSEC-60752