feat(aap): In App WAF support for lambda#7783
Conversation
Overall package sizeSelf size: 6.42 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.2.0 | 104.26 kB | 843.44 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7783 +/- ##
=======================================
Coverage 93.72% 93.73%
=======================================
Files 891 892 +1
Lines 51163 51234 +71
Branches 11900 11923 +23
=======================================
+ Hits 47952 48023 +71
Misses 3211 3211
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2026-06-29 22:47:06 Comparing candidate commit 22d6848 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2248 metrics, 38 unstable metrics.
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 22d6848 | 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 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"
There was a problem hiding this comment.
Replaced span._lambdaAppsecKey with a WeakSet to track active invocations.
|
|
||
| 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
There was a problem hiding this comment.
Done. The two separate conditionals are now merged into one.
| 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
There was a problem hiding this comment.
Fixed with a hasPersistentData boolean flag
| 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.
| } catch (err) { | ||
| if (!IS_SERVERLESS) { | ||
| if (IS_SERVERLESS) { | ||
| log.debug('[ASM] Serverless mode: suppressing error log, calling disable()') |
There was a problem hiding this comment.
I think it's helpful to attach the error here for later investigations.
Adds AppSec support for AWS lambda to dd-trace-js by introducing DC handlers that allow the datadog-lambda-js layer to delegate WAF execution to the tracer.
Adds AppSec support for AWS lambda to dd-trace-js by introducing DC handlers that allow the datadog-lambda-js layer to delegate WAF execution to the tracer.
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