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

Kubernetes response engine #389

Merged
merged 13 commits into from
Jul 12, 2018
Merged

Kubernetes response engine #389

merged 13 commits into from
Jul 12, 2018

Conversation

nestorsalceda
Copy link
Contributor

A response engine for Falco that allows to process security events executing playbooks to respond to security threats.

I also ping @mstemm for being aware of the merge.

Thanks!

@@ -0,0 +1,18 @@
# Kubernetes Response Engine for Sysdig Falco
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this to a lower directory, something like integrations/alert_handlers? @bencer mentioned that he had other additions like a helm chart, so we're looking for a place for that as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I love the integrations subdirectory.

I'm going to place under integrations/kubernetes_response_engine.

Thanks!

@@ -0,0 +1,164 @@
# Playbooks
Copy link
Contributor

Choose a reason for hiding this comment

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

I was initially confused, thinking that this was related to ansible. You might want to ask around to see if others had the same confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are not the unique who pointed us to this misunderstanding.

A few days ago, @KnoxAnderson suggested to use this name because is well known among security teams.

https://docs.microsoft.com/en-us/azure/security-center/security-center-playbooks
https://blogs.cisco.com/security/using-a-playbook-model-to-organize-your-information-security-monitoring-strategy

So we chose the term "Playbook", perhaps a possible alternative would be "Security Playbooks"?

@@ -0,0 +1,102 @@
# File(s) or Directories containing Falco rules, loaded at startup.
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of copying the entire falco config, can you just include a patch or highlight the changes that need to be made from the main falco config to add the additional file output? I'm worried that with a second copy it will become out-of-date with respect to the main config file as we make changes to the main config file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as the new Falco Helm Chart is merged (I hope in a while), we can remove the falco and falco_config directory from deployment. So this change will dissapear.

Thanks!

@@ -0,0 +1,38 @@
####################
# Your custom rules!
####################
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this rules file really related to the integration, or is it for some specific example of suspicious behavior? I wouldn't mix up the two, otherwise people will think the integration only is related to cryptomining.

Copy link
Contributor Author

@nestorsalceda nestorsalceda Jul 11, 2018

Choose a reason for hiding this comment

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

Good catch, is an specific example of suspicious behavior. I've fixed it.

Thanks!

@@ -0,0 +1,84 @@
apiVersion: extensions/v1beta1
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here--does this differ from the existing falco daemonset configs already at examples/k8s-using-daemonset ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same than above, when the new Falco Helm Chart is merged this change will dissapear.

Thanks!

@mstemm
Copy link
Contributor

mstemm commented Jul 11, 2018

Overall my biggest piece of feedback is that it copies lots of other configs/etc (probably so you could have a completely self-contained example). But I think the big contribution here is the python code that expresses actions as modules. I think the PR should only have that, and then have instructions how to combine it with a falco config, nats integration, etc to get a working example.

Néstor Salceda added 3 commits July 11, 2018 17:18
It was just an example for cryptomining.
A top level directory for this integration could led to confussion.
As long as this PR merged, this is not needed:

helm/charts#6600
@nestorsalceda
Copy link
Contributor Author

Hey @mstemm I think I have addressed all your concerns. A quick summary is:

  • I have moved top level directory to integrations/kubernetes-response-engine
  • I have removed all Falco configuration files because they are going to live in the Helm Chart

What do you think? It looks good to you?

Thanks!

@@ -0,0 +1,65 @@
from mamba import description, context, it, before
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay for tests!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I love to write tests 🤘

@nestorsalceda
Copy link
Contributor Author

Cool! Do you need anything else for merging the PR?

Thanks!

@bencer bencer merged commit ccf3555 into falcosecurity:dev Jul 12, 2018
@bencer bencer deleted the kubernetes-response-engine branch July 12, 2018 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants