-
Notifications
You must be signed in to change notification settings - Fork 377
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-8112] Appsec allow to set user id denylist #2612
Conversation
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.
Approving because the public facing API looks correct, nonetheless I feel like changes to the private methods are in order.
lib/datadog/appsec/processor.rb
Outdated
@@ -94,6 +80,30 @@ def finalize | |||
|
|||
private | |||
|
|||
def update_rules_data_with_static_configured_values |
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.
Thinking we might want that to be more general, something like:
merge_to_rule_data('blocked_ips' => Datadog::AppSec.settings.ip_denylist, 'blocked_users' => Datadog::AppSec.settings.user_id_denylist)
And a companion:
def apply_denylist(settings)
update_rule_data(merge_to_rule_data('blocked_ips' => settings.ip_denylist, 'blocked_users' => settings.user_id_denylist))
end
# usage: apply_denylist(Datadog::AppSec.settings)
WDYT?
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 like the suggestion.
One thing that I do not think is necessary right now is the merge_to_rule_data
method. If we see that the merging logic gets complicated or that we use it in other places of the codebase we could consider creating the method and exposing it as part of the Processor API.
But for now I think this is readable enough:
def apply_denylist_data(settings)
ruledata_setting = []
ruledata_setting << denylist_data('blocked_ips', settings.ip_denylist)
ruledata_setting << denylist_data('blocked_users', settings.user_id_denylist)
update_rule_data(ruledata_setting)
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.
Fair enough
@@ -40,6 +40,10 @@ def ip_denylist=(value) | |||
options[:ip_denylist] = value | |||
end | |||
|
|||
def user_id_denylist=(value) | |||
options[:user_id_denylist] = value |
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.
Wondering if we should rather resolve to a default here:
options[:user_id_denylist] = value | |
options[:user_id_denylist] = value || [] |
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 like that. That way the setting of sane defaults happen at the right layer 😄
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.
…ing through the configuration block
@@ -142,13 +135,13 @@ def load_ruleset | |||
end | |||
end | |||
|
|||
def create_waf_handle | |||
def create_waf_handle(settings) |
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.
Cool. I think this also makes it more testable
Unfify building rule data information into a single method: `denylist_data(id, denylist)` Pass the settings information as an argument to apply_denylist_data, load_ruleset, and create_waf_handle. That way we couold more easily mock the settings value in tests.
2c89745
to
296b62b
Compare
What does this PR do?
Introduce a new appsec configuration option
user_id_denylist
That list would be use to populate the waf rules data at the initialization of a the
AppSec::Processor
.Changes in
AppSec::Processor
:Previously we had a public method called
update_ip_denylist
which populated the waf rule data just for the IP denylist. Now that we have multiple configuration points for deny lists we have to change the code to account for that.I created a new private method
update_rules_data_with_static_configured_values
that creates all the necessary waf rule data for the different deny list configurations and call the waf handleupdate_rule_data
function once with all the data. I made that change to reduce the number of calls to the handle.Also, I remove the public function
update_ip_denylist
from the processor interface. I thought about leaving it public and adding an extra public functionupdate_user_id_denylist
, but it seems that those are not needed. Ultimately in the future once Remote Configure is implemented we would have to compute the rules data separate and simply callprocessor.update_rule_data(data)