-
Notifications
You must be signed in to change notification settings - Fork 375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[APPSEC-10967] ASM API security. Schema extraction #3131
Conversation
95c40cb
to
3837125
Compare
b82f8e7
to
df79586
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, a few notes but nothing consequential or worth a block.
@@ -16,8 +18,13 @@ def initialize(version1, version2) | |||
end | |||
end | |||
|
|||
DEFAULT_PROCESSORS = JSON.parse(Datadog::AppSec::Assets.waf_processors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call that DEFAULT_WAF_PROCESSORS
to materialize the distinction with AppSec::Processor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also refrain from reading+parsing that file + setting that constant at file load time. I'd rather have that be a memoized method.
@@ -10,6 +10,10 @@ def waf_rules(kind = :recommended) | |||
read("waf_rules/#{kind}.json") | |||
end | |||
|
|||
def waf_processors | |||
read('waf_rules/processors.json') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha our Appsec::Processor
naming is a bit unfortunate. Let's make sure there's no confusion in our code.
@@ -62,7 +62,7 @@ Gem::Specification.new do |spec| | |||
spec.add_dependency 'debase-ruby_core_source', '= 3.2.1' | |||
|
|||
# Used by appsec | |||
spec.add_dependency 'libddwaf', '~> 1.11.0.0.0' | |||
spec.add_dependency 'libddwaf', '~> 1.14.0.0.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure to update libddwaf
signatures in vendor/rbs
result = scope.processor_context.extract_schema | ||
|
||
if result | ||
scope.processor_context.events << { | ||
trace: scope.trace, | ||
span: scope.service_entry_span, | ||
waf_result: result, | ||
} | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by this, the extracted schema is a waf result, and stored in the events list?
waf_result = event[:waf_result] | ||
tags['_dd.appsec.triggers'] ||= [] | ||
tags['_dd.appsec.triggers'] += event[:waf_result].events | ||
tags['_dd.appsec.triggers'] += waf_result.events | ||
|
||
waf_result.derivatives.each do |key, value| | ||
tags[key] = JSON.dump(value) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I think I get it now, but coming back to this after a long time I feel some confusion.
input = { | ||
'waf.context.processor' => { | ||
'extract-schema' => true | ||
} | ||
} | ||
|
||
_code, res = @context.run(input, WAF::LibDDWAF::DDWAF_RUN_TIMEOUT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This libddwaf
API looks very strange, which might be the root cause of my confusion.
append information as part of the top level span
2e003b0
to
b47295e
Compare
What does this PR do?
This PR adds the necessary bits to enable API Security's new feature. Schema Extraction 🎉
To enable that, we need the latest version of libddwaf 1.14.0. At the end request lifecycle, we would call the WAF to extract the schema of the different WAF addresses. Calling schema extraction is gated via specific configuration:
DD_EXPERIMENTAL_API_SECURITY_ENABLED
andDD_API_SECURITY_REQUEST_SAMPLE_RATE
The schema extraction information is appended to the top-level span attributes so the backend can later process it.
At the moment, the processor configuration is not part of the static WAF rules, so we need to make sure we always include it as part of the Handle rules. We store the processor configuration at the same pace as the WAF rule.
To better review the changes, I split the work into separate commits. I recommend reading individual commits to make it easier to understand the changes. Another suggestion is to filter out all the
.lock
file changes. Here is a handy link to filter out those filesMotivation:
Additional Notes:
There are a few missing features of the API Secuirty RFC that would be implemented on the following PRs
server.response.body
How to test the change?
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!