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

refactor!: deprecate PSP regression tests and warn for unsafe usage of <NA> in k8s audit filters #1976

Merged
merged 5 commits into from
Apr 21, 2022

Conversation

jasondellaluce
Copy link
Contributor

@jasondellaluce jasondellaluce commented Apr 21, 2022

What type of PR is this?

/kind cleanup

/kind design

/kind feature

Any specific area of the project related to this PR?

/area engine

/area tests

What this PR does / why we need it:

This follows up the discussion in https://kubernetes.slack.com/archives/CMWH3EH32/p1650475403605479, the upcoming deprecation of PSPs in K8S (see https://kubernetes.io/blog/2021/04/06/podsecuritypolicy-deprecation-past-present-and-future/), and the recent dropping of the official Falco documentation for PSPs (see: falcosecurity/falco-website#572).

This PR starts the deprecation process of PSP from Falco by removing the legacy regression tests that were present in our test suite. This is also an enabler for the project of porting the K8S Audit support to a plugin implementation (see: #1952), since the PSP regression tests used unsafe rule filters based on top of notorious <NA>-related bugs supported by the current K8S Audit log Falco implementation.

Accordingly, this PR also adds a warning to the rule loader named unsafe-na-check, that advises the users when a rule relies on unsafe comparisons with <NA>. For instance, cases such as ka.req.pod.volumes.volume_type in (flexVolume,"<NA>") (taken right from the removed PSP test cases), would produce the warning below:

Rule <rule_name>: warning (unsafe-na-check):
    comparing a field value with <NA> is unsafe 
    and can lead to unpredictable behavior of the 
    rule. Consider using the 'exists' operator instead. 

The condition above can safely be rewritten as: (not ka.req.pod.volumes.volume_type exists) or ka.req.pod.volumes.volume_type in (flexVolume).

The warning is non-blocking and is only applied to K8S Audit-specific fields (ka.* and jevt.*), so that there is no effective breaking change. It's also worth mentioning that no rules like to one above is present in the current officially supported k8s audit ruleset. We may considering extending the warning to every field and-or making it blocking in the future, once we reach proper coherence with <NA> values in sinsp filterchecks too.

Breaking changes will be eventually introduced in the the plugin-based K8S Audit implementation proposed in #1952, that will fix the way <NA>s are extracted from ka.* fields and evaluated at runtime.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

refactor!: deprecate PSP regression tests and warn for unsafe usage of <NA> in k8s audit filters

…ter AST

The first warnings we support involve the unsafe comparisons with <NA>, which were present
in the legacy regression tests for PSPs.

Signed-off-by: Jason Dellaluce <[email protected]>
@poiana poiana requested review from krisnova and mstemm April 21, 2022 11:28
@jasondellaluce jasondellaluce changed the title refactor!: deprecate PSP regression tests and print warnings for bad usage of <NA> refactor!: deprecate PSP regression tests and warn for unsafe usage of <NA> Apr 21, 2022
@jasondellaluce jasondellaluce changed the title refactor!: deprecate PSP regression tests and warn for unsafe usage of <NA> refactor!: deprecate PSP regression tests and warn for unsafe usage of <NA> in k8s audit filters Apr 21, 2022
@jasondellaluce
Copy link
Contributor Author

/milestone 0.32.0

@poiana poiana added this to the 0.32.0 milestone Apr 21, 2022
@poiana
Copy link
Contributor

poiana commented Apr 21, 2022

LGTM label has been added.

Git tree hash: 5e952beca89b4881c3f85ce23ffa889c6a337a19

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.

💥

👏

/approve

@poiana
Copy link
Contributor

poiana commented Apr 21, 2022

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

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

@poiana poiana merged commit 0bf53f0 into master Apr 21, 2022
@poiana poiana deleted the refactor/deprecate-psp-tests branch April 21, 2022 16:51
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