Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[APPSEC-8112] Appsec allow to set user id denylist #2612

Merged
merged 5 commits into from
Feb 13, 2023
Merged

[APPSEC-8112] Appsec allow to set user id denylist #2612

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a7f13b4
add new user_id_denylist configuration to appsec
GustavoCaso Feb 9, 2023
65eabdb
load static rules data from configuration when initializing Datadog::…
GustavoCaso Feb 9, 2023
ecf17ad
set default value for denylist configurations at the the time os sett…
GustavoCaso Feb 13, 2023
36aaf27
Change the method signature for loading rule data.
GustavoCaso Feb 13, 2023
296b62b
make sure the default value for client_ip_denylist and user_id_denyli…
GustavoCaso Feb 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
load static rules data from configuration when initializing Datadog::… 
…AppSec::Processor
  • Loading branch information
@GustavoCaso
GustavoCaso committed Feb 9, 2023
commit 65eabdbccace4e1de67aaf74032d902bd42dacf6
40 changes: 25 additions & 15 deletions lib/datadog/appsec/processor.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ def initialize
return
end

update_ip_denylist
update_rules_data_with_static_configured_values
end

def ready?
Expand All @@ -70,20 +70,6 @@ def toggle_rules(map)
@handle.toggle_rules(map)
end

def update_ip_denylist(denylist = Datadog::AppSec.settings.ip_denylist, id: 'blocked_ips')
denylist ||= []

ruledata_setting = [
{
'id' => id,
'type' => 'data_with_expiration',
'data' => denylist.map { |ip| { 'value' => ip.to_s, 'expiration' => 2**63 } }
}
]

update_rule_data(ruledata_setting)
end

def finalize
@handle.finalize
end
Expand All @@ -94,6 +80,30 @@ def finalize

private

def update_rules_data_with_static_configured_values
Copy link
Contributor

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?

Copy link
Member Author

@GustavoCaso GustavoCaso Feb 13, 2023 •
edited
Loading

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough

ruledata_setting = []
ruledata_setting << ip_denylist_data(Datadog::AppSec.settings.ip_denylist || [])
ruledata_setting << user_id_denylist_data(Datadog::AppSec.settings.user_id_denylist || [])

update_rule_data(ruledata_setting)
end

def ip_denylist_data(denylist, id: 'blocked_ips')
{
'id' => id,
'type' => 'data_with_expiration',
'data' => denylist.map { |ip| { 'value' => ip.to_s, 'expiration' => 2**63 } }
}
end

def user_id_denylist_data(denylist, id: 'blocked_users')
{
'id' => id,
GustavoCaso marked this conversation as resolved.
Show resolved Hide resolved
'type' => 'data_with_expiration',
'data' => denylist.map { |ip| { 'value' => ip.to_s, 'expiration' => 2**63 } }
GustavoCaso marked this conversation as resolved.
Show resolved Hide resolved
GustavoCaso marked this conversation as resolved.
Show resolved Hide resolved
}
end

def load_libddwaf
Processor.require_libddwaf && Processor.libddwaf_provides_waf?
end
Expand Down
4 changes: 3 additions & 1 deletion sig/datadog/appsec/processor.rbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ module Datadog
def new_context: () -> Context
def update_rule_data: (untyped data) -> untyped
def toggle_rules: (untyped map) -> untyped
def update_ip_denylist: (?untyped denylist, ?id: ::String) -> untyped
def update_rules_data_with_static_configured_values: () -> untyped
def ip_denylist_data: (::Array[String?] denylist, ?id: ::String) -> ::Hash[::String, untyped]
def user_id_denylist_data: (::Array[String?] denylist, ?id: ::String) -> ::Hash[::String, untyped]
def finalize: () -> void

attr_reader handle: untyped
Expand Down
30 changes: 30 additions & 0 deletions spec/datadog/appsec/processor_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,36 @@
it { is_expected.to_not be_ready }
end

context 'when loading static data rule configuration' do
before do
allow(Datadog::AppSec.settings).to receive(:ip_denylist).and_return(['192.192.1.1'])
allow(Datadog::AppSec.settings).to receive(:user_id_denylist).and_return(['user3'])
end

it 'calls #update_rule_data with the right value' do
expect_any_instance_of(described_class).to receive(:update_rule_data) do |_, args|
expect(args.size).to eq(2)

blocked_ips = args.find { |hash| hash['id'] == 'blocked_ips' }
blocked_users = args.find { |hash| hash['id'] == 'blocked_users' }

expect(blocked_ips).to_not be_nil
expect(blocked_users).to_not be_nil
expect(blocked_ips['type']).to eq('data_with_expiration')
expect(blocked_users['type']).to eq('data_with_expiration')

blocked_ips_data = blocked_ips['data']
blocked_user_data = blocked_users['data']
expect(blocked_ips_data.size).to eq(1)
expect(blocked_user_data.size).to eq(1)
expect(blocked_ips_data[0]['value']).to eq('192.192.1.1')
expect(blocked_user_data[0]['value']).to eq('user3')
end

described_class.new
end
end

context 'when things are OK' do
before do
expect(Datadog::AppSec::Assets).to receive(:waf_rules).with(:recommended).and_call_original
Expand Down