fix(userspace/engine): supporting enabled-only overwritten rules

[jasondellaluce]

Signed-off-by: Jason Dellaluce [email protected]
Co-authored-by: Leonardo Grasso [email protected]

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area engine

What this PR does / why we need it:
This enables overwriting rules by specifying their name and the enabled field only, so that users can easily enable/disable already-defined rules. The expected behavior is to be able to overwrite rules like the following (example from @leogr):

- rule: A rule enabled by default
  enabled: false

The following issues currently prevent this from being supported:

• Every rule is forced to either specify a condition or an exception field. However, this conceptually applies only to rules having append set as true.
• Overwritten rules are entirely substituted to their already-defined counterpart. This is correct, however it does not support changing the enabled flag only, which is a feature demanded by the community.

Proof of this ambiguity can be seen in the current version of falco-website, which reports a non-working example: https://github.com/falcosecurity/falco-website/blob/4a7b5eb82027da5dadd50dc688c7ebd55c24bd9e/content/en/docs/rules/_index.md?plain=1#L263

Which issue(s) this PR fixes:
Fixes #1537
Fixes #1510

Special notes for your reviewer:
See #1537 for additional context.

Does this PR introduce a user-facing change?:

fix(userspace/engine): supporting enabled-only overwritten rules

[jasondellaluce]

The test invalid_append_rule_without_condition asserted that rules with append set as true contained a condition field. However, according to our documentation, this is not a requirement. This is explicitly stated here 👇🏼
https://falco.org/docs/rules/#via-custom-rule-definition

Accordingly, this PR removes the invalid_append_rule_without_condition test as it fails with the latest updates. Also, rules can now specify omit the condition and exception fields if they have append as true.

@mstemm @leogr

[mstemm]

About exceptions, the docs may not state it, but you're not supposed to be able to append exceptions at the moment, only conditions. @sai-arigeli is working on a separate PR to change that: #1768.

And about enabled, I'm not sure we want disable a rule by appending to it with a different enabled value. Is there a compelling use case for this? Why wouldn't redefining the original rule work?

[leogr]

Hey @mstemm

And about enabled, I'm not sure we want disable a rule by appending to it with a different enabled value. Is there a compelling use case for this? Why wouldn't redefining the original rule work?

Currently, it works neither 👇

Redefine

Wed Nov 10 08:26:22 2021: Runtime error: Could not load rules file /etc/falco/falco_rules.local.yaml: 1 errors:                                                                                           
Rule must have exceptions or condition property                                                                                                                                                           
---                                                                                                                                                                                                       
- rule: Unexpected inbound connection source                                                                                                                                                              
  enabled: false                                                                                                                                                                                          
---                                            
. Exiting.

Append

Wed Nov 10 08:27:23 2021: Runtime error: Could not load rules file /etc/falco/falco_rules.local.yaml: 1 errors:
Rule must have exceptions or condition property
---
- rule: Unexpected inbound connection source
  append: true
  enabled: false
---
. Exiting.

I guess the reason why @jasondellaluce tried to fix the "append" case is that the official documentation advice it: https://falco.org/docs/rules/#via-custom-rule-definition
(that's currently broken; indeed, we had to add a note with an alternative)

I would correct both cases. In my opinion, enabled: false should work both in redefining and appending cases since they are semantically equivalent.

In general, enabled needs to be fixed because enabled is currently not really usable.

[jasondellaluce]

I don't think this strictly overlaps with #1768, as this is not correlated to rule exceptions. In that perspective, I only weakened the requirement of needing a condition or an exception in appended rules, in order to allow users to append rules defining only the enabled field:

- rule: Unexpected inbound connection source
  append: true
  enabled: false

Users raised many issues about this (such as #1510 and #1537), since we indicate this as feasible in our doc (as @leogr said).

I would correct both cases. In my opinion, enabled: false should work both in redefining and appending cases since they are semantically equivalent.

I agree with this. Some additional work is required for this, so I'm gonna wait until we are on the same page on this discussion before pushing some additional commit.

[leogr]

After having discussed this privately, I'm ok with fixing only the redefinition case.

Basically, what this PR should do is:

1. Allow to disable a rule by redefining it:

- rule: A rule enabled by default
  enabled: false

2. Allow to enable a rule by redefining it:

- rule: A rule disabled by default
  enabled: true

The append: true behavior will continue to be as-is now.
Consequently, we will have to update this piece of documentation too.

@mstemm do you agree?

[mstemm]

Currently the rule loading code doesn't handle cases where append is false, and the only thing in the second rule definition is an enabled flag. Currently, if a rule/macro has append false, everything in the prior rule is discarded and only the properties in the new rule are used instead.

So this would introduce a special case where a rule with append false and only an enabled flag changes the enabled value from the original rule, but otherwise everything in the original rule is used instead?

I guess this is okay, but to me this seems like a problem where the docs are wrong, but we're changing the code to match the docs rather than fixing the docs.

[leogr]

I guess this is okay, but to me this seems like a problem where the docs are wrong, but we're changing the code to match the docs rather than fixing the docs.

@mstemm The real point here is that users are asking to disable/enable a rule without modifying the default ruleset file (that is a very common use case) and we should provide a way to do that. The docs might be wrong in the how, but they are perfectly correct w.r.t. the need. For these reasons, my last proposal suggests fixing both the code and the docs.

[jasondellaluce]

I just refactored the PR trying to implement @leogr's proposal. For clarity, I also added two more integration tests that assert the expected behavior wit enabled-only rules. Also, the invalid_append_rule_without_condition test has been restored and adapted to the new changes. Let me know what you think.

[leogr]

LGTM now

/approve

[poiana]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jasondellaluce, leogr

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 [leogr]

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

[poiana]

LGTM label has been added.

Git tree hash: 4bb5471f374227d89c325b33e328897c58214440

[leogr]

/milestone 0.31.0

[mstemm]

Ok, this seems like a reasonable way to handle this use case. Approved!