-
Notifications
You must be signed in to change notification settings - Fork 897
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
Proposal for 2 new Falco rules for syscall source, and 4 for Kubernetes audit source. #1122
Conversation
…nd detect traffic that is not to authorized server process and port Signed-off-by: Vicente Herrera <[email protected]>
…des successfully joining the cluster, nodes unsuccessfully attempt to join, creation ingress without TLS certificate Signed-off-by: Vicente Herrera <[email protected]>
/assign @Kaizhe |
rules/k8s_audit_rules.yaml
Outdated
|
||
|
||
- list: full_admin_k8s_users | ||
items: ["admin", "kubernetes-admin", "kubernetes-admin@kubernetes", "default", "[email protected]", "minikube-user"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why default
is admin user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
K3s admin user is called "default", when /etc/rancher/k3s/k3s.yaml configuration file is created.
The best direct link I can find to share about this is: k3s-io/k3s#140 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know whether falco supports k3s ? And also whether the audit events from k3s is compatible with k8s. If you can provide more information here, that would be great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe they are not, and we overstepped here. k3s have an audit webhook. Let me make some test but it will take some time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it may be good to remove default
in this PR, once you confirm falco support k3s and k3s audit is event is compatible with k8s, then we can add it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the default
user, and clarified comments on the rule to make clear that we are detecting only usernames and not roles.
# # Execute any kubectl command connected using default cluster user, as: | ||
# kubectl create namespace rule-test | ||
|
||
- rule: Full K8s Administrative Access |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full k8s administrative access should be based on the roles granted to the user, not the user name. I would put this rule on hold or disabled by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As Falco supported fields indicate, role information is only available in the Kubernetes audit event "when the request object refers to a cluster role binding" or "when the request object refers to a role/cluster role". https://falco.org/docs/rules/supported-fields/
So for any general event, we can only rely on user name to check this.
Also the rule is disabled by default, including a condition "and not allowed_full_admin_users" to a macro that is defined as "k8s_audit_always_true".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rules works only for k8s audit events. Relying on username is not the right thing to do. And as I suggest, this is something I want to put it on hold or disable by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a enabled
field in the rule you can specify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user name that executed the Kubernetes command is part of the k8s event in ka.user.name (but not the role). Another existing rule "Disallowed K8s User" also list allowed user names as a way to get a detection if a different one executes a command (this new rule is more or less the opposite).
I like more the clear semantics of the enabled field, but right now the only way to change a "enabled: false" to true would be to edit the original k8s_audit_rules.yaml. That would lead to problems when rules get updated. Macros, on the other hand, can be overwritten in falco_rules.loca.yaml or other files to enable rules more easily. I should post a feature request for a better way to enable existing disabled rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the intention for enabled: false
and it is up to the user to enable it. In OSS community, we can worry less about the commercial product impact regarding patching rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OSS is my concern here. I don't think it should be encouraged to make people modify falco_rules.yaml or k8s_audit_rules.yaml directly for rules activation or customization, as it is going to make it harder for them to keep up with updates. I believe that's why falco_rules.local.yaml and macros with (never_true) and similar values exist on all other rules. enabled:false
as it is defined now is not used in any other rule, they all rely on macros for activation.
Just an opinion, of course, tell me if you still think it's necessary.
- macro: allow_all_k8s_nodes | ||
condition: (k8s_audit_always_true) | ||
|
||
- list: allowed_k8s_nodes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most likely the decision shouldn't be made by name. Let's create a list of trusted node labels and only nodes contains any trusted labels can join the cluster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but let me try to discuss this a little bit
So, you can add labels to a node at the same time that it joins the cluster. But we feel that if someone has the kind of access level to your cluster to add a node to it, he may well replicate the labels on the existing nodes to the new one, in an effort to make it look similar to the other ones.
The node name is a more unique way to represent trust in nodes. Someone may see other labels in existing nodes, but it's more difficult that he is aware of the specific list of allowed node names before attempting to join. And when that happens, Falco is going to alert you.
Tell me what you think about this. ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Managing by node name will be painful. As this is the first version, I will let it in and we can come back to improve it.
Signed-off-by: Vicente Herrera <[email protected]>
… rule is disabled by default Signed-off-by: Vicente Herrera <[email protected]>
/lgtm |
LGTM label has been added. Git tree hash: ad8c71b6c9873d2686798bd9eb3e476977683f80
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing these Vicente! 🎉
I need some time to look deeper at them. In the meantime could you please adjust the release-note block according to our CONTRIBUTING guidelines?
Since this PR regards rules its release-note block should use the guidelines here
/hold need to adjust release-note block |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WOW @vicenteherrera - this is great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great Job @vicenteherrera, thanks!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fntlnz, Kaizhe, leodido 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 |
This is in! /milestone 0.22.0 /hold cancel |
What type of PR is this?
/kind feature
/kind rule-create
Any specific area of the project related to this PR?
/area rules
What this PR does / why we need it:
These proposed rules detect network and Kubernetes activity that, outside some whitelists, could indicate malicious activity. No other rules perform similar detections, and they are generic so everybody will benefit for having them in their Falco installation. See the proposed release notes for specific details.
Which issue(s) this PR fixes:
Fixes #1121
Special notes for your reviewer:
The Issue is just a description of the proposal of the rules, I don't know if it would have been better to just create de pull request only.
Does this PR introduce a user-facing change?:
Yes, new rules available in falco.yaml and k8s_audit_rules.yaml, one of the enabled by default: "Ingress Object without TLS Certificate Created"