[closed in favor of a new follow up PR] cleanup/new/feat(load-rules): expand rules append support and feature partial overrides

[incertum]

What type of PR is this?

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

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

/kind release

Any specific area of the project related to this PR?

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

/area build

/area engine

/area tests

/area proposals

/area CI

What this PR does / why we need it:

Expand rules append support and feature partial overrides / merges

Which issue(s) this PR fixes:

Consolidated issue #1340.

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

cleanup/new/feat(load-rules): expand rules append support and feature partial overrides / merges

[poiana]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: incertum

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

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

[incertum]

/milestone 0.36.0

[incertum]

@jasondellaluce and @leogr these features have been requested by 3+ adopters and I am also supportive especially now that we introduce the rules maturity framework and we will disable all non stable rules by default.

Hence we urgently need easier customization options for folks who prefer not to create a new rules file.

Less concerned about the actual code implementation than the capability, hence happy to make any changes.

@jasondellaluce we could either also have you do the fixes for the tags + enable filters here or in a follow up, whichever you prefer.

Lastly, would you have more test ideas?

[leogr]

@jasondellaluce and @leogr these features have been requested by 3+ adopters and I am also supportive especially now that we introduce the rules maturity framework and we will disable all non stable rules by default.

Hence we urgently need easier customization options for folks who prefer not to create a new rules file.

Less concerned about the actual code implementation than the capability, hence happy to make any changes.

Hey @incertum

I understand the importance of this but I'm not totally sure I understand what this feature does. Could you summarize it, please? Also, why do we need PRIORITY_INVALID? Is it an implementation detail or part of the feature?

Finally, two tests are currently not passing, PTAL

🙏

[github-actions]

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

[incertum]

@leogr

Let's say

- rule: Dummy Rule 6
  desc: My test desc 6
  condition: evt.type in (ptrace)
  enabled: true
  output: '%evt.type %proc.cmdline'
  priority: INFORMATIONAL
  tags: [maturity_sandbox, host]

Adopter wants to change the priority and add an output field, but Falco currently does not allow it in Falco 0.35.1 :/

Instead adopter needs to copy and paste the entire rule in their custom rules file like this, defying the purposes of supporting append feature etc ...

- rule: Dummy Rule 6
  desc: My test desc 6
  condition: evt.type in (ptrace)
  enabled: true
  output: '%evt.type %proc.cmdline %proc.name'
  priority: CRITICAL
  tags: [maturity_sandbox, host]

To make matters more confusing append was restricted to only condition in Falco 0.35.1 and below and partial override / rules merging was only allowed for enabled.

e.g.:

- rule: Dummy Rule 6
  enabled: false

- rule: Dummy Rule 6
  append: true
  condition: and not proc.name=cat

was possible but nothing else was.

---

Adopters asked to support all possible scenarios of appending to and/or overriding a previous key when providing a new possibly incomplete rules definition while ensuring we still have one complete Falco rule at the end.

I hope the falco_rules_test1.yaml and the new unit test assertions help clarify the new support spectrum. In summary:

• We now can also append to tags and output when using the append mode
• We can also conveniently keep using the append feature for "non-appendable" fields like enabled and priority. In that case instead of appending it overrides the previous field. This was requesting for better UX, because many adopters now will want to append to let's say output and also adjust priority at the same time. They now can.
• For even more convenience it was also requested to be able to override enabled and priority when using append mode without providing an "appendable" key (a small extension of the previous bullet point)
• Then it was also requested to just support partial overrides (talking about non-append mode now). This means when re-defining a rule you do not need a complete rule anymore you can just override any of the fields. If one key is not re-defined in the new rules definition we just use the old one from the previous rules definition. As a result we always have one complete Falco rule at the end. This is also the reason why I introduced invalid priority as default value for priority to account for cases where priority was not re-defined, in that case I use the old one. Note there are checks to make sure we always have one complete rule at any given point in time.

[incertum]

re test failure idk what the failure is? Where do I retrieve the report or better test it locally? Most likely once I know it's a matter of updating some other old tests with the new bevaior.

[jasondellaluce]

Where do I retrieve the report or better test it locally?

The report is available in the PR's workflow summary: https://github.com/falcosecurity/falco/actions/runs/5764238799?pr=2671. For testing it locally, you follow the instructions on https://github.com/falcosecurity/testing, and use the -falco-binary option for using your new Falco binary instead of the one provided by the installed package.

[jasondellaluce]

I'll leave my thoughts here.

From my understanding, this PR is bringing different kinds of contributions:

1. Extenting support to append for rules only, from condition and exceptions to also tags and output
2. Allow partial definitions of rules only, merging them with definitions from before

I'm onboard with point (1), it's a nice and long-requested feature. I would honestly also append to desc at this point

Point (2) is instead confusing to me in this form, and also a breaking change. Consider this case

- rule: Rule 1
  condition: evt.type=open
  desc: Desc of Rule 1
  output: Output of Rule 1
  priority: WARNING
  exceptions: [...]

- rule: Rule 1
  condition: evt.type=openat
  desc: Desc of Rule 1
  output: Output of Rule 1
  priority: CRITICAL

Up until now, this was an acceptable input, and Rule 1 would end up having no exceptions, as being re-defined without them. This is not a case for just exceptions, but for all the non-mandatory fields (e.g. tags, warn_evttypes, skip-if-unknown-filter, exceptions), which ended up having the default value for all those fields if re-defined without them being explicit. This is a change in behavior.

Also, merging while also append-ing seems confusing to me. What gets appended and what doesn't? And what's the logic for that? If both are defined, who takes priority?

My proposal is instead to make merging explicit and mutually-explusive with appending. In this way, the current behavior will always be preserved, but you could have something like:

- rule: Rule 1
  condition: evt.type=open
  desc: Desc of Rule 1
  output: Output of Rule 1
  priority: WARNING
  exceptions: [...]

- rule: Rule 1
  condition: evt.type=openat
  merge: true

- rule: Rule 1
  priority: CRITICAL
  merge: true

- rule: Rule 1
  condition: and proc.name=cat
  append: true

This way:

• All previous behavior will be left unchanged
• append will not work together with merge, and will only support condition, exceptions, output, and tags
• When enabled is defined only, it will be interpreted as having merge: true implicitly for backward-compatibility reasons (but it should raise a warning IMO)
• merge will always require a previous definitions to be present, just like append, and will not cause unpredictable behavior when changing loading order of rules files

This is much more clear to read, implement, understand for newcomers. The only downside that I see is that you'll need two distinct definitions for merging and appending if you need to do both, which I think is an acceptable tradeoff.

[incertum]

Let's gather more feedback from the adopters who need a very convenient solution @tspearconquest and @jonny-wg2? Thanks in advance!

I do understand that the separation is cleaner in terms of software engineering, however while some improvement it may still not be the best UX. Meaning if (as you pointed out) I need to re-define one append field and one partial override field for one upstream rule I need to re-define it twice in my custom files. With the current changes I can do it in one swing.

Re the exceptions case, hmmm upstream rules have no exceptions at the moment right? Why would someone write a custom rule with an exception / aka a non required field to then re-define it again. Is it an edge case we want to sign up to support in general? Could there be another way to handle the exception cases to support a one swing re-definition of previous rules?

By the way as a side note: I am not sure if exceptions are helping adoption, it appears just having append to condition is all you would need to effectively use Falco in a more clean way?

In summary, if we could customize an upstream rule with just one re-definition, I think even I would consider giving it a try, else I likely stick with a custom approach of just re-writing the entire rules files.

[incertum]

@jasondellaluce maybe add a third option append_merge: true? @jonny-wg2 your use case of using append for just overriding priority for example would be reverted again (acceptable I think).

[jonny-wg2]

append_merge: true would be ok for me. I also agree with your previous comment of the complexity of updating rules vs compared to just an append. But from my need, if there is just a simple way of changing the prio without rewriting the rule then it would already be a big improvement.

[leogr]

After reading the conversation and finally understanding the situation, I tend to agree with @jasondellaluce .

In particular, this feature must not introduce any backward incompatible change. To avoid disrupting our adopters, supporting the previous behavior is a must-have, IMO. (If in the future we want to review the whole syntax because we are not satisfied with it, it would ok but we will also have to think about proper versioning and a deprecation plan; but this should be a separate discussion).

Recap

• All previous behavior will be left unchanged

👍

• append will not work together with merge, and will only support condition, exceptions, output, and tags

👍
Please, note we also have lists and macros. The append behavior with them must be preserved.

• When enabled is defined only, it will be interpreted as having merge: true implicitly for backward-compatibility reasons (but it should raise a warning IMO)

👍
For this special case (enabled without merge), I recommend (a) explicitly documenting as a shortcut OR (b) raising the warning. Assuming we want to preserve the previous behavior, I'd prefer (a), but I'm fine with either option.

• merge will always require a previous definitions to be present, just like append, and will not cause unpredictable behavior when changing loading order of rules files

👍
Again, we also have lists and macros. Using merge with them should no behavior change and should be equivalent to a replacement (because lists and macros have just one key).

For example, assuming macro: a_macro was already defined, the two following syntax must produce the same effect:

- macro: a_macro
  condition: ...
  
- macro: a_macro
  merge: true
  condition: ...

However, if macro: a_macro was not already defined, merge should error (likewise append).

Other considerations

-Do not forget the source field. Although replacing source in rules usually makes little sense, in the case of merge, all fields must work the same way, just for consistency.

• Also, I'm not convinced about introducing append_merge. It can be confusing, and I don't see any compelling benefit.

For example, I think this is easy to read and understand:

- rule: Rule 1
  condition: evt.type=openat
  priority: CRITICAL
  merge: true

- rule: Rule 1
  condition: and proc.name=cat
  append: true

Rather than this other:

- rule: Rule 1
  condition: and proc.name=cat
  priority: CRITICAL
  append_merge: true

• And a thought regarding the implementation.

Less concerned about the actual code implementation than the capability, hence happy to make any changes.

Stating that I agree with introducing merge since it covers real-world use cases; however, In this specific case, I'm more concerned by the implementation than the feature itself. In my experience, dealing with rule syntax problems has always been a pain. So, I would trade some velocity for solidity, ensuring the implementation is solid enough before incorporating it.

[Andreagit97]

Looking at the example here: #2671 (review)

- rule: Rule 1
  condition: evt.type=open
  desc: Desc of Rule 1
  output: Output of Rule 1
  priority: WARNING
  exceptions: [...]

- rule: Rule 1
  condition: evt.type=openat
  merge: true

- rule: Rule 1
  priority: CRITICAL
  merge: true

- rule: Rule 1
  condition: and proc.name=cat
  append: true

I've not fully understood this merge option... more in detail:

- rule: Rule 1
  condition: evt.type=openat
  merge: true # <--- What does this `merge` do on the rule?

using this partial rule should I expect the original evt.type=open to be replace by evt.type=openat?

• if yes, IMO merge is not a good name, from a merge i would expect evt.type=openat + evt.type=open
• if no, what is the difference between append and merge?

BTW not sure why we use a boolean for merge and append maybe it would be nice to have a string/enum like
modify: -> modify: merge/modify: append, in this way we don't have to add a boolean for every new operation and moreover it becomes clear if we can use them together or no. Right now as a user it's seems i can use both together since they are 2 different boolean not correleted between them...

Final question about always the same yaml snippet. I imagine that we could put together these 2 block in just one, right? 👇

- rule: Rule 1
  condition: evt.type=openat
  merge: true

- rule: Rule 1
  priority: CRITICAL
  merge: true

- rule: Rule 1
  condition: evt.type=openat
  priority: CRITICAL
  merge: true

[jasondellaluce]

using this partial rule should I expect the original evt.type=open to be replace by evt.type=openat?

Yes. I'm totally open to better names, but you got the idea.

modify: -> modify: merge/modify: append

I like this too! However, we'd still need to support append for backward compatibility, maybe with a warning?

Final question about always the same yaml snippet. I imagine that we could put together these 2 block in just one, right? 👇

Yes.

[Andreagit97]

using this partial rule should I expect the original evt.type=open to be replace by evt.type=openat?

Yes. I'm totally open to better names, but you got the idea.

Thank you, yes i would go with something like replace so it would be clearer that the whole value of a certain key will be "replaced" by the new value

modify: -> modify: merge/modify: append

I like this too! However, we'd still need to support append for backward compatibility, maybe with a warning?

Yeah maybe we could depracte the actual append and log a warning

Final question about always the same yaml snippet. I imagine that we could put together these 2 block in just one, right? point_down

Yes.

ty!

[incertum]

Top Level Requirement: Adopters can customize a rule by re-defining it just once, not twice. This would be best UX from my perspective (being an adopter myself), because it is a lot of overhead to maintain Falco rules.

---

Can we overload append as bool and list type in a transition process?

After reading all new feedback, sharing my new favorite I would recommend. It may be the most powerful one covering all possible use cases in a non ambiguous way for once and it would only require a one time re-definition of a previously defined rule.

It would also more naturally align with database concepts where you always define the column alongside the operation, kind of what we do in Falco rules anyways. That way the documentation and communication will also be easier as it's not a hidden contract anymore.

A mix match example:

append: [condition, output]
replace: [enabled, tags, priority]

Alternatively instead of overloading append we can use a new key merge.

append eligible fields: {condition|output|tags|desc}
replace eligible fields: {condition|output|tags|desc|enabled|priority|exceptions}

---

Other new suggestions to accomplish all goals with just one re-definition?

[incertum]

@jasondellaluce @leogr @Andreagit97 I am out at a conference until end of week.
After that Jason is out and I in turn will be out for most of September. I tried to help here, but I won't be able to continue developing this feature. Hence I am closing the PR, please feel free to recover some of the new tests when you work on it in a new PR.

[Andreagit97]

solve:

• [FEATURE] Support selective rule overrides / merging #1340