Module-based rule evaluation precedence #353
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #353 +/- ##
==========================================
+ Coverage 84.68% 84.95% +0.26%
==========================================
Files 153 157 +4
Lines 7889 7994 +105
Branches 3520 3551 +31
==========================================
+ Hits 6681 6791 +110
+ Misses 460 459 -1
+ Partials 748 744 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| return std::hash<std::string_view>{}(address); | ||
| } | ||
|
|
||
| struct target_address { |
There was a problem hiding this comment.
These are not currently used, but I plan to start using them soon.
| @@ -22,33 +23,22 @@ | |||
| namespace ddwaf { | |||
|
|
|||
| struct ruleset { | |||
| void insert_rule(const std::shared_ptr<rule> &rule) | |||
| // OLINTNEXTLINE(bugprone-easily-swappable-parameters) | |||
| void insert_rules(const std::vector<std::shared_ptr<core_rule>> &base, | |||
There was a problem hiding this comment.
Do you need shared ownership so that ruleset_builder can reuse the rules in updates that don't replace the rules? Did you ensure that rules that are mutated in an update (through toggle/set_actions/set_verdict) are not incorrectly shared between rulesets?
There was a problem hiding this comment.
Do you need shared ownership so that ruleset_builder can reuse the rules in updates that don't replace the rules?
Yes, and between live handles / contexts during or after update.
Did you ensure that rules that are mutated in an update (through toggle/set_actions/set_verdict) are not incorrectly shared between rulesets?
Currently rules are not mutated, they are regenerated when overrides are applied.
There was a problem hiding this comment.
I see. Does it mean the current code is buggy, given that you changed the definition of base_rule_update? I find this strategy on the dangerous side, because the correctness depends on distinct parts of the codebase to act in concert not to do unsafe writes.
Perhaps a better alternative would be to make core_rule immutable; the mutation methods would instead return a new instance. You could then perhaps even avoid recreating the full set of rules; the ruleset_builder would need to store the original rules (before overrides).
There was a problem hiding this comment.
Does it mean the current code is buggy, given that you changed the definition of base_rule_update?
Not really, the only difference is that now an update to the actions will result in regenerating rules.
I find this strategy on the dangerous side, because the correctness depends on distinct parts of the codebase to act in concert not to do unsafe writes.
Perhaps a better alternative would be to make core_rule immutable; the mutation methods would instead return a new instance. You could then perhaps even avoid recreating the full set of rules; the ruleset_builder would need to store the original rules (before overrides).
I have to refactor everything for the multi-configuration builder, I am making rules safely mutable to avoid the re-computation costs the current strategy results in, or at least this is the direction I'm currently going on, I might change my mind.
This PR introduces the new concept of
modulesto better organise rules based on their precedence. Modules are introduced as a generic mechanism which contains a set of rules in the required order (user / base, blocking, etc). Individual modules can have optional grouping, which is introduced specifically for the purpose of having independent collections within thewafmodule, as the short-circuit evaluation follows a different criteria due to rules being grouped by type.The modules introduced are the following:
network-acl,authentication-acl,custom-acl,configuration,business-logic,rasp,waf. The network and authentication modules do not follow the provided timeout, and the ordering of each module changes a little bit based on whether user (Custom) or base (DD) rules should take precedence.Related Jira: APPSEC-55598