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

cleanup(rules): cleanup rules disabled by default - 3 #2168

Merged
merged 2 commits into from
Aug 26, 2022

Conversation

incertum
Copy link
Contributor

@incertum incertum commented Aug 18, 2022

Signed-off-by: Melissa Kilby melissa.kilby.oss@gmail.com

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

If contributing rules or changes to rules, please make sure to also uncomment one of the following line:

/kind rule-update

/kind rule-create

Any specific area of the project related to this PR?

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

/area build

/area engine

/area rules

/area tests

/area proposals

/area CI

What this PR does / why we need it:

Still missed a whole bunch of (never_true) macros at the beginning of the condition or when and (never_true) was split across lines. #2166

Question:

What about these always_true macros that don't seem to add much value in those 3 rules below? Keep or also clean up?

macro: consider_userfaultfd_activities
rule: Unprivileged Delegation of Page Faults Handling to a Userspace Process

macro: consider_all_chmods
rule: Set Setuid or Setgid bit
rule: Container Drift Detected (chmod)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

rule(Unexpected outbound connection destination)!: disabled by default
rule(Unexpected inbound connection source)!: disabled by default
rule(Read Shell Configuration File)!: disabled by default
rule(Schedule Cron Jobs)!: disabled by default
rule(Launch Suspicious Network Tool on Host)!: disabled by default
rule(Create Hidden Files or Directories)!: disabled by default
rule(Outbound or Inbound Traffic not to Authorized Server Process and Port)!: disabled by default
rule(Network Connection outside Local Subnet)!: disabled by default
rule(macro: consider_all_outbound_conns)!: remove unused macro
rule(macro: consider_all_inbound_conns)!: remove unused macro
rule(macro: consider_shell_config_reads)!: remove unused macro
rule(macro: consider_all_cron_jobs)!: remove unused macro
rule(macro: consider_all_inbound_conns)!: remove unused macro
rule(macro: consider_hidden_file_creation)!: remove unused macro
rule(macro: allowed_port)!: remove unused macro
rule(macro: enabled_rule_network_only_subnet)!: remove unused macro
rule(macro: consider_userfaultfd_activities)!: remove unused macro
rule(macro: consider_all_chmods)!: remove unused macro
rule(Set Setuid or Setgid bit)!: removed consider_all_chmods macro
rule(Container Drift Detected (chmod))!: removed consider_all_chmods macro
rule(Unprivileged Delegation of Page Faults Handling to a Userspace Process)!: removed consider_userfaultfd_activities macro

@jasondellaluce
Copy link
Contributor

@darryk10 what do you think? I like these changes, they would positively affect runtime performance.

@jasondellaluce
Copy link
Contributor

/milestone 0.33.0

@poiana poiana added this to the 0.33.0 milestone Aug 22, 2022
@jasondellaluce
Copy link
Contributor

I spent this morning investigating why the CI jobs are currently failing. The bug propagation path is really twisted, but it turns out the some regex-based parsing function of our rule conditions language parser are a bit flaky. This is due to the regex support in C++ being very ambiguous specially in our setup.

Since this has been discussed already in other pull requests, this led me to the conclusion of adopting a portable regex library for regex parsing in libsinsp, which can be used in Falco as well later if needed. 👉 falcosecurity/libs#556

If we agree on this, that would require merging that PR and bumping the libs version on Falco.

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>
Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>
@poiana
Copy link
Contributor

poiana commented Aug 25, 2022

LGTM label has been added.

Git tree hash: aa1e351ddefe597d3fb55cbc8a44ad6d66f87161

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.

😍

Copy link
Contributor

@jasondellaluce jasondellaluce 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
Contributor

poiana commented Aug 26, 2022

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants