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

update(rules): revert exceptions in default ruleset #1602

Merged
merged 6 commits into from
Apr 9, 2021

Conversation

fntlnz
Copy link
Contributor

@fntlnz fntlnz commented Apr 7, 2021

Exceptions have been introduced in commit 64a231b
The feature itself is very useful for more complex environments where
the simple conditions are difficult to handle.
However, many users reported that they find them difficult to understand so
we are doing a rollback of them in the default ruleset in favor of the
syntax without exceptions.

Signed-off-by: Lorenzo Fontana lo@linux.com

What type of PR is this?

/kind cleanup

/kind rule-update

/kind rule-create

Any specific area of the project related to this PR?

/area rules

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

rule(list mysql_mgmt_binaries): removed
rule(list db_mgmt_binaries): removed
rule(macro parent_ansible_running_python): removed
rule(macro parent_bro_running_python): removed
rule(macro parent_python_running_denyhosts): removed
rule(macro parent_linux_image_upgrade_script): removed
rule(macro parent_java_running_echo): removed
rule(macro parent_scripting_running_builds): removed
rule(macro parent_Xvfb_running_xkbcomp): removed
rule(macro parent_nginx_running_serf): removed
rule(macro parent_node_running_npm): removed
rule(macro parent_java_running_sbt): removed
rule(list known_container_shell_spawn_cmdlines): removed
rule(list known_shell_spawn_binaries): removed
rule(macro run_by_puppet): removed
rule(macro user_privileged_containers): removed
rule(list rancher_images): removed
rule(list images_allow_network_outside_subnet): removed
rule(macro parent_python_running_sdchecks): removed
rule(macro trusted_containers): removed
rule(list authorized_server_binaries): removed

Exceptions have been introduced in commit 64a231b
The feature itself is very useful for more complex environments where
the simple conditions are difficult to handle.
However, many users reported that they find them difficult to understand so
we are doing a rollback of them in the default ruleset in favor of the
syntax without exceptions.

Signed-off-by: Lorenzo Fontana <lo@linux.com>
…write

rules(list known_sa_list): list of known sa moved here from user_known_sa_list

Signed-off-by: Lorenzo Fontana <lo@linux.com>
@fntlnz
Copy link
Contributor Author

fntlnz commented Apr 7, 2021

/assign @mstemm

Signed-off-by: Lorenzo Fontana <lo@linux.com>
@fntlnz
Copy link
Contributor Author

fntlnz commented Apr 7, 2021

Been exploring this with @leogr

The only problem in having a ruleset without exceptions is that by default Falco gives a warning here

warnings[#warnings + 1] = "Rule "..v['rule']..": consider adding an exceptions property to define supported exceptions fields"

The warning is a bit annoying now that by default we don't use exceptions

Wed Apr  7 17:09:46 2021: Falco version 0.27.0-65+167c5bc (driver version 5c0b863ddade7a45568c0ac97d037422c9efb750)
Wed Apr  7 17:09:46 2021: Falco initialized with configuration file ../falco.yaml
Wed Apr  7 17:09:46 2021: Loading rules from file ../rules/k8s_audit_rules.yaml:
When reading rules content: 39 warnings:
Rule Disallowed K8s User: consider adding an exceptions property to define supported exceptions fields
Rule Create Disallowed Pod: consider adding an exceptions property to define supported exceptions fields
Rule Create Privileged Pod: consider adding an exceptions property to define supported exceptions fields
Rule Create Sensitive Mount Pod: consider adding an exceptions property to define supported exceptions fields
Rule Create HostNetwork Pod: consider adding an exceptions property to define supported exceptions fields
Rule Create NodePort Service: consider adding an exceptions property to define supported exceptions fields
Rule Create/Modify Configmap With Private Credentials: consider adding an exceptions property to define supported exceptions fields
Rule Anonymous Request Allowed: consider adding an exceptions property to define supported exceptions fields
Rule Attach/Exec Pod: consider adding an exceptions property to define supported exceptions fields
Rule EphemeralContainers Created: consider adding an exceptions property to define supported exceptions fields
Rule Create Disallowed Namespace: consider adding an exceptions property to define supported exceptions fields
Rule Pod Created in Kube Namespace: consider adding an exceptions property to define supported exceptions fields
Rule Service Account Created in Kube Namespace: consider adding an exceptions property to define supported exceptions fields
Rule System ClusterRole Modified/Deleted: consider adding an exceptions property to define supported exceptions fields
Rule Attach to cluster-admin Role: consider adding an exceptions property to define supported exceptions fields
Rule ClusterRole With Wildcard Created: consider adding an exceptions property to define supported exceptions fields
Rule ClusterRole With Write Privileges Created: consider adding an exceptions property to define supported exceptions fields
Rule ClusterRole With Pod Exec Created: consider adding an exceptions property to define supported exceptions fields
Rule K8s Deployment Created: consider adding an exceptions property to define supported exceptions fields
Rule K8s Deployment Deleted: consider adding an exceptions property to define supported exceptions fields
Rule K8s Service Created: consider adding an exceptions property to define supported exceptions fields
Rule K8s Service Deleted: consider adding an exceptions property to define supported exceptions fields
Rule K8s ConfigMap Created: consider adding an exceptions property to define supported exceptions fields
Rule K8s ConfigMap Deleted: consider adding an exceptions property to define supported exceptions fields
Rule K8s Namespace Created: consider adding an exceptions property to define supported exceptions fields
Rule K8s Namespace Deleted: consider adding an exceptions property to define supported exceptions fields
Rule K8s Serviceaccount Created: consider adding an exceptions property to define supported exceptions fields
Rule K8s Serviceaccount Deleted: consider adding an exceptions property to define supported exceptions fields
Rule K8s Role/Clusterrole Created: consider adding an exceptions property to define supported exceptions fields
Rule K8s Role/Clusterrole Deleted: consider adding an exceptions property to define supported exceptions fields
Rule K8s Role/Clusterrolebinding Created: consider adding an exceptions property to define supported exceptions fields
Rule K8s Role/Clusterrolebinding Deleted: consider adding an exceptions property to define supported exceptions fields
Rule K8s Secret Created: consider adding an exceptions property to define supported exceptions fields
Rule K8s Secret Deleted: consider adding an exceptions property to define supported exceptions fields
Rule All K8s Audit Events: consider adding an exceptions property to define supported exceptions fields
Rule Full K8s Administrative Access: consider adding an exceptions property to define supported exceptions fields
Rule Ingress Object without TLS Certificate Created: consider adding an exceptions property to define supported exceptions fields
Rule Untrusted Node Successfully Joined the Cluster: consider adding an exceptions property to define supported exceptions fields
Rule Untrusted Node Unsuccessfully Tried to Join the Cluster: consider adding an exceptions property to define supported exceptions fields

What do people think? I'm for removing the warning since, internally lua will continue to add an empty exception to rules that don't have it (like the default ones) and users can decide whether or not they want to use them.

@fntlnz
Copy link
Contributor Author

fntlnz commented Apr 7, 2021

/milestone 0.28.0

@poiana poiana added this to the 0.28.0 milestone Apr 7, 2021
Kaizhe
Kaizhe previously approved these changes Apr 7, 2021
Copy link
Contributor

@Kaizhe Kaizhe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a change! lgtm!

@poiana
Copy link

poiana commented Apr 7, 2021

LGTM label has been added.

Git tree hash: 7e19acba5c33a6767b35bae9d90ddf79466b276f

We want users to continue using rules without having to use exceptions.
Exceptions are an additional feature for more advanced use-cases, having
a warning in there will mean that everyone now adds an empty exception
to avoid the warning.

Co-Authored-By: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Lorenzo Fontana <lo@linux.com>
leodido and others added 2 commits April 9, 2021 14:31
…w binary dir rule

Co-authored-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
Co-authored-by: Lorenzo Fontana <lo@linux.com>
Co-authored-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@poiana
Copy link

poiana commented Apr 9, 2021

LGTM label has been added.

Git tree hash: 574708057422880d3a51e957b7c5f154ffbe010b

@poiana poiana added the approved label Apr 9, 2021
@poiana
Copy link

poiana commented Apr 9, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leodido, 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:

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 2e97d0e into master Apr 9, 2021
@poiana poiana deleted the revert/exception-rules branch April 9, 2021 16:17
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.

None yet

6 participants