-
Notifications
You must be signed in to change notification settings - Fork 373
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-9341] Enable configuring blocking response via Remote Configuration #3099
Conversation
40bee94
to
d1bf510
Compare
3f803a9
to
0d6c696
Compare
Codecov Report
@@ Coverage Diff @@
## master #3099 +/- ##
========================================
Coverage 98.16% 98.16%
========================================
Files 1323 1325 +2
Lines 75237 75457 +220
Branches 3430 3455 +25
========================================
+ Hits 73855 74072 +217
- Misses 1382 1385 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
bddb865
to
d498af6
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, just a Q.
I think for those catch/throw protocols we should (but not in this PR) move to instances instead of hashes and arrays, it should make things easier to follow (and properly type-checked)
end | ||
|
||
throw(Datadog::AppSec::Ext::INTERRUPT, [nil, [:block, event]]) if block | ||
throw(Datadog::AppSec::Ext::INTERRUPT, [nil, [[:block, event]]]) if block |
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.
Curious about the double array here: was it missing before or is that an intended API change?
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.
In was missing 😨
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.
While missing the functionality still worked because we did not use the values
[:block, {}].any? {|action, _event| action == :block }
# => true
We now care about the returned value with the code changes on the request middleware. We use find
rather than any?
blocked_event = request_response.find { |action, _options| action == :block }
Old code
blocked_event = [:block, {}].find {|action, _event| action == :block }
#=> :block
New Code
blocked_event = [[:block, {}]].find {|action, _event| action == :block }
# => [:block, {}]
Absolutely 😄 I mentioned in the PR description
|
What does this PR do?
Update ASM to be able to configure the blocking response via Remote Configuration.
Motivation:
Allow customers to change the blocking response via the DD UI
The PR includes multiple changes:
AppSec::Processor::Actions
apart from storing in memory, we need to merge existing actions with the ones coming from the Remote Configuration and merging.The logic for merging replaces existing actions with new ones coming from remote configuration and keeps the ones not updated.
AppSec::Response.negotiate
method to account for the WAF result actions and the configured informationAdditional Notes:
I've removed unused returned values from the methods
Reactive::<Class>.subscribe
, andReactive::<Class>.publish
The returned values from the reactive engine are plain arrays ex:
[:block, event]
or[:monitor, event]
those could be refactored into their own classesAppSec::Processor::Actions::Block
andAppSec::Processor::Actions::Monitor
. I left that exercise to a future PR.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!