Enable "Trusted IPs", a.k.a passlist with optional monitoring #3229
Conversation
lloeki
force-pushed
the
enable-trusted-ips
branch
from
435fd90
to
bc2b8da
Compare
lloeki
force-pushed
the
enable-trusted-ips
branch
from
bc2b8da
to
92c2866
Compare
Codecov Report
@@ Coverage Diff @@
## master #3229 +/- ##
=========================================
Coverage ? 98.22%
=========================================
Files ? 1253
Lines ? 72360
Branches ? 3361
=========================================
Hits ? 71075
Misses ? 1285
Partials ? 0
📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
| exclusions = [] | ||
|
|
||
| exclusions << { | ||
| 'conditions' => [ |
There was a problem hiding this comment.
Instead of manually building up hashes like this, does it make sense to have some sort of exclusion object like a struct or class to ensure the hash is well formed? Or is that overkill in this situation?
There was a problem hiding this comment.
I would agree if it were pure Ruby as a consumer, but these will be passed to libddwaf's C land by converting them to ddwaf C objects via libddwaf-rb's ruby_to_object, which only understands Ruby core types.
That said, I could add an arbitrary object conversion protocol, as such:
- non-core types such as a custom
Structwould have ato_libddwafmethod, to transform it to whatever libddwaf expects as a structure. - libddwaf-rb's
ruby_to_objectwould check forto_libddwaf, e.g callingruby_to_object(v.to_libddwaf).
| exclusions << { | ||
| 'conditions' => [ | ||
| { | ||
| 'operator' => 'ip_match', | ||
| 'parameters' => { | ||
| 'inputs' => [ | ||
| { | ||
| 'address' => 'http.client_ip' | ||
| } | ||
| ], | ||
| 'list' => pass | ||
| } | ||
| } | ||
| ], | ||
| 'id' => SecureRandom.uuid, | ||
| } | ||
|
|
||
| exclusions << { | ||
| 'conditions' => [ | ||
| { | ||
| 'operator' => 'ip_match', | ||
| 'parameters' => { | ||
| 'inputs' => [ | ||
| { | ||
| 'address' => 'http.client_ip' | ||
| } | ||
| ], | ||
| 'list' => monitor | ||
| } | ||
| } | ||
| ], | ||
| 'id' => SecureRandom.uuid, | ||
| 'on_match' => 'monitor' | ||
| } |
There was a problem hiding this comment.
In line with @ekump's suggestion, maybe we can extract these into private methods, just to clean up the repetition a bit:
| exclusions << { | |
| 'conditions' => [ | |
| { | |
| 'operator' => 'ip_match', | |
| 'parameters' => { | |
| 'inputs' => [ | |
| { | |
| 'address' => 'http.client_ip' | |
| } | |
| ], | |
| 'list' => pass | |
| } | |
| } | |
| ], | |
| 'id' => SecureRandom.uuid, | |
| } | |
| exclusions << { | |
| 'conditions' => [ | |
| { | |
| 'operator' => 'ip_match', | |
| 'parameters' => { | |
| 'inputs' => [ | |
| { | |
| 'address' => 'http.client_ip' | |
| } | |
| ], | |
| 'list' => monitor | |
| } | |
| } | |
| ], | |
| 'id' => SecureRandom.uuid, | |
| 'on_match' => 'monitor' | |
| } | |
| exclusions << build_exclusion(pass) | |
| exclusions << build_exclusion(monitor).merge('on_match' => 'monitor') |
It's fine to keep it as is too.
There was a problem hiding this comment.
I didn't want to go too far and crystallise an inherently incomplete design so I must admit I punted: indeed since load_exclusions/passlist_exclusions and load_data/denylist_data got added, it feels that RuleLoader should be split and/or refactored.
I mean, using this API - however internal - feels a bit awkward to me:
def create_processor(settings)
rules = AppSec::Processor::RuleLoader.load_rules(ruleset: settings.appsec.ruleset)
return nil unless rules
actions = rules['actions']
AppSec::Processor::Actions.merge(actions) if actions
data = AppSec::Processor::RuleLoader.load_data(
ip_denylist: settings.appsec.ip_denylist,
user_id_denylist: settings.appsec.user_id_denylist,
)
exclusions = AppSec::Processor::RuleLoader.load_exclusions(ip_passlist: settings.appsec.ip_passlist)
ruleset = AppSec::Processor::RuleMerger.merge(
rules: [rules],
data: data,
exclusions: exclusions,
)
processor = Processor.new(ruleset: ruleset)
return nil unless processor.ready?
processor
end
|
Thanks for the reviews! I've tracked your suggestions internally as they fit into wider scoped changes. |
What does this PR do?
Enable "Trusted IPs", a.k.a passlist with optional monitoring
Motivation:
Observe events for passlisted IPs but without blocking for those.
Additional Notes:
None
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!