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.

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

new(falco): add rule selection configuration in falco.yaml #3178

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LucaGuerra
Copy link
Contributor

@LucaGuerra LucaGuerra commented Apr 23, 2024

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area engine

What this PR does / why we need it:

This is a rather straightforward implementation of #3174 (comment) . Essentially, it introduces two new ways of enabling/disabling rules without changing the rule files. Following the example:

rules:
  - disable:
      rule: *
  - enable:
      tag: network
  - enable:
      rule: Directory traversal monitored file
  - enable:
      rule: k8s_*
  - disable:
      rule: k8s_noisy_rule

This means: disable everything, enable all rules tagged networking, also enable the rule called Directory traversal monitored file, then enable any rule matching the wildcard pattern k8s_* and disable k8s_noisy_rule.

You can achieve the same via the CLI

falco -o "rules[].disable.rule=*" -o "rules[].enable.tag=network" -o "rules[].enable.rule=Directory traversal monitored file
" -o "rules[].enable.rule=k8s_*" -o "rules[].disable.rule=k8s_noisy_rule"

The new syntax [] allows to append a new element at the end of sequences, which is how the CLI works in this case.

At this point, rule names support wildcard while tag names do not. I am a bit unsure about what to do with tag names. On one side, what you want to do is enable and disable them one by one so wildcards seem a bit too much there. On the other hand we currently allow to "intersect" the tags we want, such as only networking AND exec, which is not supported here. Perhaps we could add a tags option for that?

Which issue(s) this PR fixes:

Fixes #3174

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(falco): allow selecting which rules to load from the configuration file or command line

Copy link

This PR may bring feature or behavior changes in the Falco engine and may require the engine version to be bumped.

Please double check userspace/engine/falco_engine_version.h file. See versioning for FALCO_ENGINE_VERSION.

/hold

@leogr
Copy link
Member

leogr commented Apr 23, 2024

This PR may bring feature or behavior changes in the Falco engine and may require the engine version to be bumped.

Please double check userspace/engine/falco_engine_version.h file. See versioning for FALCO_ENGINE_VERSION.

/hold

I guess this is a false positive.

@LucaGuerra LucaGuerra changed the title new(falco): implement rule selection configuration in falco.yaml new(falco): add rule selection configuration in falco.yaml Apr 23, 2024
Signed-off-by: Luca Guerra <luca@guerra.sh>
Signed-off-by: Luca Guerra <luca@guerra.sh>
Copy link
Contributor

@incertum incertum left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link

poiana commented Apr 24, 2024

LGTM label has been added.

Git tree hash: b3c5a8310099e8d4525e0d575604bb9213817734

@poiana
Copy link

poiana commented Apr 24, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: incertum, LucaGuerra

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [LucaGuerra,incertum]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Andreagit97 Andreagit97 added this to the 0.38.0 milestone Apr 26, 2024
@leogr
Copy link
Member

leogr commented Apr 30, 2024

/assign

Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

SGTM

cc @jasondellaluce @FedeDP for a second look

@@ -177,6 +177,39 @@ rules_files:
- /etc/falco/falco_rules.local.yaml
- /etc/falco/rules.d

# [Experimental] `rules`
Copy link
Member

@leogr leogr Apr 30, 2024

Choose a reason for hiding this comment

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

Suggested change
# [Experimental] `rules`
# [Incubating] `rules`

As per the upcoming adoption policy, this should be labeled as "Incubating".

This is not a blocker for this PR (since we can change it later), but maybe it is worth starting using new labels.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree

@FedeDP
Copy link
Contributor

FedeDP commented May 2, 2024

As suggested by Andrea, i'd deprecate -t,-T,-D options so that we can remove them in Falco 0.39: #3174 (comment)

@@ -90,6 +90,13 @@ void falco_configuration::init(const std::vector<std::string>& cmdline_options)
load_yaml("default");
}

void falco_configuration::init_from_content(const std::string& config_content, const std::vector<std::string>& cmdline_options)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather add the config_content="" optional argument to the old init method (the one above) instead of adding a completely new method.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think they are both just used by tests anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

Proposal/discussion: More flexible ways to enable/disable rules from configuration
6 participants