Add tags #206
Add tags #206
Conversation
|
I know you guys aren't as familiar with the falco code, but since there are corresponding changes on the sysdig side I thought I'd have you take a look here as well. |
|
The TravisCI failure is because it's building against mainline sysdig, which doesn't have the changes in draios/sysdig#746 yet. |
|
I'm going to make some additional changes to support globbing on rule names in addition to tag matches. Let me make those changes before you take a look. |
mstemm
force-pushed
the
add-tags
branch
5 times, most recently
from
6db6975 to
3c6277d
Compare
|
Ok, I've done the additional changes. Any feedback welcome! |
- in lua, look for a tags attribute to each rule. This is passed up in add_filter as a tags argument (as a lua table). If not present, an empty table is used. The tags table is iterated to populate a set of tags as strings, which is passed to add_filter(). - A new method falco_engine::enable_rule_by_tag is similar to enable_rule(), but is given a set of tag strings. Any rules containing one of the tags is enabled/disabled. - The list of event types has been changed to a set to more accurately reflect its purpose. - New argument to falco -T allows disabling all rules matching a given tag, via enable_rule_by_tag(). It can be provided multiple times. - New argument to falco -t allows running those rules matching a given tag. If provided all rules are first disabled. It can be provided multiple times, but can not be combined with -T or -D (disable rules by name) - falco_enging supports the notion of a ruleset. The idea is that you can choose a set of rules that are enabled/disabled by using enable_rule()/enable_rule_by_tag() in combination with a ruleset. Later, in process_event() you include that ruleset and the rules you had previously enabled will be run. - rulsets are provided as strings in enable_rule()/enable_rule_by_tag() and as numbers in process_event()--this avoids the overhead of string lookups per-event. Ruleset ids are created on the fly as needed. A utility method find_ruleset_id() looks up the ruleset id for a given name. The default ruleset is NULL string/0 numeric if not provided. - Although the ruleset is a useful falco engine feature, it isn't that important to the falco standalone program, so it's not documented. However, you can change the ruleset by providing FALCO_RULESET in the environment.
Add automated tests that verify the ability to tag sets of rules, disable them with -T, and run them with -t, works: - New test option disable_tags adds -T <tag> arguments to the falco command line, and run_tags adds -t <tag> arguments to the falco command line. - A new trace file open-multiple-files.scap opens 13 different files, and a new rules file has 13 different rules with all combinations of the tags a, b, c (both forward and backward), a rule with an empty list of tags, a rule with no tags field, and a rule with a completely different tag d. Using the above, add tests for: - Both disabling all combations of a, b, c using disable_tags as well as run all combinations of a, b, c, using run_tags. - Specifying both disabled (-T/-D) and enabled (-t) rules. Not allowed. - Specifying a ruleset while having tagged rules enabled, rules based on a name disabled, and no particular rules enabled or disabled.
Tag the existing ruleset to group tags in a meaningful way. The added tags are: - filesystem: the rule relates to reading/writing files - sofware_mgmt: the rule relates to any software/package management tool like rpm, dpkg, etc. - process: the rule relates to starting a new process or changing the state of a current process. - database: the rule relates to databases - host: the rule *only* works outside of containers - shell: the rule specifically relates to starting shells - container: the rule *only* works inside containers - cis: the rule is related to the CIS Docker benchmark. - users: the rule relates to management of users or changing the identity of a running process. - network: the rule relates to network activity Rules can have multiple tags if they relate to multiple of the above. Rules do not have to have tags, although all the current rules do.
userspace/engine/falco_engine.h
Outdated
| // | ||
| void enable_rule(std::string &pattern, bool enabled); | ||
| void enable_rule(std::string &pattern, bool enabled, std::string *ruleset = NULL); |
There was a problem hiding this comment.
Same as the sysdig review, const pattern (and tags in enable_rule_by_tags)
userspace/engine/falco_engine.cpp
Outdated
|
|
||
| uint16_t falco_engine::find_ruleset_id(std::string &ruleset) | ||
| { | ||
| auto it = m_known_rulesets.find(ruleset); |
There was a problem hiding this comment.
Similar comments to the sysdig review: lower_bound, insert with a hint, and no second find to cut this down from 3 map walks to just 1
userspace/engine/falco_engine.h
Outdated
| // to enable_rule/enable_rule_by_tag(), you should look up the | ||
| // ruleset id and pass it to process_event(). | ||
| // | ||
| uint16_t find_ruleset_id(std::string &ruleset); |
userspace/engine/falco_engine.h
Outdated
| // | ||
| // Enable/Disable any rules with any of the provided tags (set, exact matches only) | ||
| // | ||
| void enable_rule_by_tag(std::set<std::string> &tags, bool enabled, std::string *ruleset = NULL); |
| @@ -120,6 +144,8 @@ class falco_engine : public falco_common | |||
| inline bool should_drop_evt(); | |||
|
|
|||
| falco_rules *m_rules; | |||
| uint16_t m_next_ruleset_id; | |||
| std::map<string, uint16_t> m_known_rulesets; | |||
There was a problem hiding this comment.
Is it possible to have an empty string as the key in a map of strings? It feels weird, but I'm pretty sure it works. If you populate m_known_rulesets[""] = 0 in the constructor, this simplifies a lot of the code here and in falco.cpp. No more passing naked pointers, checking if ruleset is not NULL, etc.
There was a problem hiding this comment.
I thought about that, but the main reason I went with a string pointer is that it allows the current falco integration in the agent, which doesn't care about rulesets, to remain unchanged. Maybe I should make new methods that provide a ruleset string and have the current methods with no argument call the new methods.
There was a problem hiding this comment.
Could be a wrapper in falco, but agent changes are also fine. I don't think there are many calls. Either way works. Now's the time to change the API if it makes implementation much easier.
- Instead of having a possibly null string pointer as the argument to enable_* and process_event, have wrapper versions that assume a default falco ruleset. The default ruleset name is a static member of the falco_engine class, and the default ruleset id is created/found in the constructor. - This makes the whole mechanism simple enough that it doesn't require seprarate testing, so remove the capability within falco to read a ruleset from the environment and remove automated tests that specify a ruleset. - Make pattern/tags/ruleset arguments to enable_* functions const. (I'll squash this down before I commit)
|
The sysdig changes were merged and the tests are passing now. I've also addressed all feedback. Wanna take a look again? |
Changes to add tags to falco rules and enable/disable sets of rules based on tags. See commits for details.
This, along with the corresponding changes in draios/sysdig#746, fixes #59 and #60.