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

Add /etc mount for falco container #475

Merged
merged 1 commit into from
Jan 24, 2019
Merged

Add /etc mount for falco container #475

merged 1 commit into from
Jan 24, 2019

Conversation

mfdii
Copy link
Member

@mfdii mfdii commented Nov 26, 2018

Fixes #473.

This is required to support COS and Minikube as the probe loader script looks for the OS version by reading files in /etc.

I also added the environment variables to enable the eBPF probe, but left them commented out.

@mstemm
Copy link
Contributor

mstemm commented Nov 28, 2018

Do you think it would be more secure to explicitly mount /etc/os-release and /etc/VERSION instead of all of /etc/ ? You did create the mount read-only, which is good, but someone who got into the falco daemonset pod could read stuff like /etc/passwd (and maybe even shadow, depending on how it's running) from the host.

@mfdii
Copy link
Member Author

mfdii commented Dec 4, 2018

Ok, did as you suggested and I am only mounting the files Falco needs.

There's a bug in the probe loader script in the logic used to detect COS vs Minikube for eBPF support. I need to submit a PR to sysdig to fix that before we should merge this.

@mfdii
Copy link
Member Author

mfdii commented Dec 4, 2018

Required probe loader script fix in draios/sysdig/1273.

@gianlucaborello
Copy link
Contributor

gianlucaborello commented Dec 4, 2018

(Commenting on this since I saw it referenced from draios/sysdig#1273)

Are we sure we want to introduce that much granularity in the mount paths, which need to be propagated to sysdig, agent as well as traditional command lines, and kept in sync every time we add a new supported distribution by peeking into a new file in /etc?

What you seem to be wanting to prevent is already possible in a very easy way: just go into a Falco container and run cat /host/proc/1/root/etc/shadow, you'll easily get access to the host file system even if you didn't mount /etc.

And this is without even counting that we are a privileged container, have full access to the Docker socket, and have the ability to compile and inject custom code into the kernel, we are basically already root++ on the host, so mounting /etc basically doesn't make a difference.

From my point of view, the only reason why we are not mounting the entire host file system, thus greatly simplifying the command line invocation, is just because we don't want to keep those mount points busy when containers churn, otherwise I'd consider it reasonable, and more practical, to do that as well (other projects, such as Google's cadvisor, do exactly that and mount the entire host's /).

Feel free to ignore this, just wanted to provide my perspective since I value CLI usability a lot, and this seems a step backwards.

@mstemm mstemm merged commit ec07f7c into dev Jan 24, 2019
@mstemm mstemm deleted the mfdii/fix_k8sds branch January 24, 2019 03:13
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