update(rule_loader): deprecate the `append` flag in falco rules #2992

Andreagit97 · 2024-01-03T15:03:05Z

What type of PR is this?

/kind cleanup

/kind design

Any specific area of the project related to this PR?

/area engine

/area tests

What this PR does / why we need it:

In this PR I propose to deprecate the append flag in the Falco rules. With deprecate I mean that we should log a warning when the append key is used suggesting the new override key. The flag shouldn't be removed until Falco 1.0.0 since it could cause a big breaking change between users (I'm open to other ideas, maybe we can wait for some release cycle). In this PR there are also some cleanups.

Side note: I'm not fully convinced by the fact that warnings in the rule loading are suppressed by default...if we introduce a warning I expect that users will see it by default since probably they need to take some actions... To be more concrete, in this PR i added a warning that in my opinion should be shown by default because users should know how to update the rules with the most recent features to avoid breaking changes in the future. Example:

Wed Jan  3 15:46:27 2024: Falco version: 0.37.0-231+ad964c0 (x86_64)
Wed Jan  3 15:46:27 2024: Falco initialized with configuration file: ../falco.yaml
Wed Jan  3 15:46:27 2024: System info: Linux version 6.2.0-39-generic (buildd@lcy02-amd64-045) (x86_64-linux-gnu-gcc-11 (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0, GNU ld (GNU Binutils for Ubuntu) 2.38) #40~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Thu Nov 16 10:53:04 UTC 2
Wed Jan  3 15:46:27 2024: Loading rules from file ../rules/falco-deprecated_rules.yaml
Wed Jan  3 15:46:27 2024: Loading rules from file ../rules/falco-incubating_rules.yaml
../rules/falco-incubating_rules.yaml: Ok, with warnings
1 Warnings:
In rules content: (../rules/falco-incubating_rules.yaml:0:0)
    rule 'Non sudo setuid': (../rules/falco-incubating_rules.yaml:1275:2)
------
- rule: Non sudo setuid
  ^
------
LOAD_DEPRECATED_ITEM (Used deprecated item): 'append' key is deprecated. Add an append entry (e.g. 'condition: append') under 'override' instead.

Wed Jan  3 15:46:27 2024: Loading rules from file ../rules/falco-sandbox_rules.yaml
Wed Jan  3 15:46:28 2024: Loading rules from file ../rules/falco_rules.yaml
Wed Jan  3 15:46:28 2024: The chosen syscall buffer dimension is: 8388608 bytes (8 MBs)
Wed Jan  3 15:46:28 2024: Starting health webserver with threadiness 8, listening on 0.0.0.0:8765
Wed Jan  3 15:46:28 2024: Loaded event sources: syscall
Wed Jan  3 15:46:28 2024: Enabled event sources: syscall
Wed Jan  3 15:46:28 2024: Opening 'syscall' source with modern BPF probe.
Wed Jan  3 15:46:28 2024: One ring buffer every '2' CPUs.

Today to obtain the above result users should run Falco with the -v which IMO is not ideal, WDYT @falcosecurity/falco-maintainers ?

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

update(rule_loader): deprecate the `append` flag in Falco rules

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

github-actions · 2024-01-03T15:03:31Z

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

FedeDP · 2024-01-03T15:10:35Z

I fully agree with the proposal, and with keeping the deprecated append until Falco 1.0.0 too.

Today to obtain the above result users should run Falco with the -v which IMO is not ideal, WDYT @falcosecurity/falco-maintainers ?

I think we should split real warnings from verbose messages (if any); in your example, the warning message should always be visible; also, given that Falco is a "fire and forget" tool, i think that having a more verbose startup by default is not that impactful after all.

incertum · 2024-01-03T17:39:26Z

I like this a lot @Andreagit97, also ok to keep this warning until Falco 1.0.0
What about the special snowflake enabled override?

jasondellaluce · 2024-01-03T17:53:16Z

What about the special snowflake enabled override?

+1 for this.

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

Andreagit97 · 2024-01-04T17:17:57Z

Update:

This PR deprecates 2 things now (with relative tests):

append usage without override
enabled usage without override
~~required_engine_version not SemVer compatible~~

Summary for Falco 0.37.0 here: #2763

I have enabled all rules warnings by default, let me know if it is OK for you. This is a possible output example:

Thu Jan  4 18:10:03 2024: Falco version: 0.37.0-231+ad964c0 (x86_64)
Thu Jan  4 18:10:03 2024: Falco initialized with configuration file: ../falco.yaml
Thu Jan  4 18:10:03 2024: System info: Linux version 6.2.0-39-generic (buildd@lcy02-amd64-045) (x86_64-linux-gnu-gcc-11 (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0, GNU ld (GNU Binutils for Ubuntu) 2.38) #40~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Thu Nov 16 10:53:04 UTC 2
Thu Jan  4 18:10:03 2024: Loading rules from file ../rules/falco-incubating_rules.yaml
Thu Jan  4 18:10:03 2024: Loading rules from file ../rules/falco-sandbox_rules.yaml
Thu Jan  4 18:10:03 2024: Loading rules from file ../rules/falco_rules.yaml
Thu Jan  4 18:10:03 2024: ../rules/falco_rules.yaml: Ok, with warnings
1 Warnings:
In rules content: (../rules/falco_rules.yaml:0:0)
    rule 'Fileless execution via memfd_create': (../rules/falco_rules.yaml:1252:2)
------
- rule: Fileless execution via memfd_create
  ^
------
LOAD_DEPRECATED_ITEM (Used deprecated item): 'append' key is deprecated. Add an 'append' entry (e.g. 'condition: append') under 'override' instead.
Thu Jan  4 18:10:03 2024: The chosen syscall buffer dimension is: 8388608 bytes (8 MBs)
Thu Jan  4 18:10:03 2024: Starting health webserver with threadiness 8, listening on 0.0.0.0:8765
Thu Jan  4 18:10:03 2024: Loaded event sources: syscall
Thu Jan  4 18:10:03 2024: Enabled event sources: syscall
Thu Jan  4 18:10:03 2024: Opening 'syscall' source with modern BPF probe.
Thu Jan  4 18:10:03 2024: One ring buffer every '2' CPUs.

I don't love the multi-line log but at the moment this is what we have... @jasondellaluce there was a particular reason to use fprintf(stderr,...) instead of falco::log?

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

Andreagit97 · 2024-01-10T16:21:29Z

Now this should be ready

FedeDP

/approve

poiana · 2024-01-11T08:18:22Z

LGTM label has been added.

Git tree hash: 0d42da1acd26344d1de3091beac0fa0d750eb7b3

userspace/engine/rule_loader_reader.cpp

LucaGuerra · 2024-01-11T09:48:01Z

userspace/engine/rule_loader_collector.cpp

@@ -244,10 +240,7 @@ void rule_loader::collector::append(configuration& cfg, rule_update_info& info)
 {
 	auto prev = m_rule_infos.at(info.name);

-	THROW(!prev,
-	       // "Rule has 'append' key or an append override but no rule by that name already exists", // TODO replace with this and update testing


Thank you for updating these errors <3

unit_tests/engine/test_rule_loader.cpp

LucaGuerra · 2024-01-11T09:59:18Z

Great job Andrea! 🎉

Just a couple minor comments and I can approve :)

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com> Co-authored-by: Luca Guerra <luca.guerra@sysdig.com>

LucaGuerra

LGTM

poiana · 2024-01-11T14:34:05Z

LGTM label has been added.

Git tree hash: 4534eddb97a798dc1d15551b691fd293e31bdd1b

FedeDP

/approve

poiana · 2024-01-11T14:36:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, 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~~ [Andreagit97,FedeDP,LucaGuerra]

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

FedeDP · 2024-01-11T14:37:27Z

/unhold

Andreagit97 added 3 commits January 3, 2024 12:40

cleanup(rule_loader): add a common log message

4e52242

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

cleanup(rule_loader): remove useless include

ad964c0

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

update(rule_loader): deprecate append key and add a warning

55b1c02

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

poiana added release-note do-not-merge/work-in-progress kind/cleanup dco-signoff: yes kind/design area/engine area/tests labels Jan 3, 2024

Andreagit97 requested a review from LucaGuerra January 3, 2024 15:03

poiana requested review from jasondellaluce and Kaizhe January 3, 2024 15:03

poiana added approved size/L labels Jan 3, 2024

poiana added the do-not-merge/hold label Jan 3, 2024

Andreagit97 changed the title ~~[WIP] update(rule_loader): deprecate the append flag in falco rules~~ [WIP] PROPOSAL: update(rule_loader): deprecate the append flag in falco rules Jan 3, 2024

Andreagit97 mentioned this pull request Jan 4, 2024

[TRACKING] Breaking changes in Falco 0.37.0 #2763

Closed

6 tasks

Andreagit97 added 5 commits January 4, 2024 16:35

update(rule_loader): deprecate enabled usage

7484bde

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

cleanup(tests): use new check_error_message helper

66cea67

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

update(rule_loader): deprecate all non-SemVer compatible values

4a1fe2f

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

cleanup(falco_engine): remove unused methods

e930f40

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

update(falco): always enable rules warnings

468897b

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

poiana added size/XL and removed size/L labels Jan 4, 2024

Andreagit97 changed the title ~~[WIP] PROPOSAL: update(rule_loader): deprecate the append flag in falco rules~~ update(rule_loader): deprecate the append flag in falco rules Jan 10, 2024

poiana removed the do-not-merge/work-in-progress label Jan 10, 2024

Andreagit97 mentioned this pull request Jan 10, 2024

cleanup(falco): remove some legacy tests falcosecurity/testing#39

Merged

chore: bump testing submodule manually to fix e2e tests

9db9ecb

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

FedeDP previously approved these changes Jan 11, 2024

View reviewed changes

poiana assigned FedeDP Jan 11, 2024

poiana added the lgtm label Jan 11, 2024

Andreagit97 mentioned this pull request Jan 11, 2024

build(deps): Bump submodules/falcosecurity-testing from 9b9630e to ae3950a #3005

Closed

LucaGuerra reviewed Jan 11, 2024

View reviewed changes

userspace/engine/rule_loader_reader.cpp Outdated Show resolved Hide resolved

LucaGuerra reviewed Jan 11, 2024

View reviewed changes

userspace/engine/rule_loader_reader.cpp Outdated Show resolved Hide resolved

LucaGuerra reviewed Jan 11, 2024

View reviewed changes

unit_tests/engine/test_rule_loader.cpp Show resolved Hide resolved

cleanup: restore the name of a variable

0dcf972

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com> Co-authored-by: Luca Guerra <luca.guerra@sysdig.com>

Andreagit97 dismissed FedeDP’s stale review via 0dcf972 January 11, 2024 14:03

poiana removed the lgtm label Jan 11, 2024

poiana requested a review from FedeDP January 11, 2024 14:03

LucaGuerra approved these changes Jan 11, 2024

View reviewed changes

poiana assigned LucaGuerra Jan 11, 2024

poiana added the lgtm label Jan 11, 2024

FedeDP approved these changes Jan 11, 2024

View reviewed changes

poiana removed the do-not-merge/hold label Jan 11, 2024

poiana merged commit a6a1a97 into falcosecurity:master Jan 11, 2024
30 checks passed

Andreagit97 mentioned this pull request Jan 15, 2024

Need documentation for Selective Override (Falco PR# 2981) falcosecurity/falco-website#1226

Closed

Andreagit97 mentioned this pull request Jan 29, 2024

[TRACKING] Breaking changes in Falco 1.0.0 #3038

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update(rule_loader): deprecate the `append` flag in falco rules #2992

update(rule_loader): deprecate the `append` flag in falco rules #2992

Andreagit97 commented Jan 3, 2024

github-actions bot commented Jan 3, 2024

FedeDP commented Jan 3, 2024

incertum commented Jan 3, 2024

jasondellaluce commented Jan 3, 2024

Andreagit97 commented Jan 4, 2024 •

edited

Loading

Andreagit97 commented Jan 10, 2024

FedeDP left a comment

poiana commented Jan 11, 2024

LucaGuerra Jan 11, 2024

LucaGuerra commented Jan 11, 2024

LucaGuerra left a comment

poiana commented Jan 11, 2024

FedeDP left a comment

poiana commented Jan 11, 2024

FedeDP commented Jan 11, 2024

update(rule_loader): deprecate the append flag in falco rules #2992

update(rule_loader): deprecate the append flag in falco rules #2992

Conversation

Andreagit97 commented Jan 3, 2024

github-actions bot commented Jan 3, 2024

FedeDP commented Jan 3, 2024

incertum commented Jan 3, 2024

jasondellaluce commented Jan 3, 2024

Andreagit97 commented Jan 4, 2024 • edited Loading

Andreagit97 commented Jan 10, 2024

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Jan 11, 2024

LucaGuerra Jan 11, 2024

Choose a reason for hiding this comment

LucaGuerra commented Jan 11, 2024

LucaGuerra left a comment

Choose a reason for hiding this comment

poiana commented Jan 11, 2024

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Jan 11, 2024

FedeDP commented Jan 11, 2024

update(rule_loader): deprecate the `append` flag in falco rules #2992

update(rule_loader): deprecate the `append` flag in falco rules #2992

Andreagit97 commented Jan 4, 2024 •

edited

Loading