-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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 ProcMount option #1934
add ProcMount option #1934
Conversation
cc @tallclair @dchen1107 happy to discuss at next sig-node meeting |
cc @rhatdan |
@feiskyer: GitHub didn't allow me to request PR reviews from the following users: rhatdan. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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'm definitely in favor of more fine-grained controls than privileged. How is this handled by non-docker runtimes?
Currently the way docker and most other container runtimes work is by masking | ||
and setting as read-only certain paths in `/proc`. This is to prevent data | ||
from being exposed into a container that should not be. However, there are | ||
certain use-cases where it is necessary to turn this off. |
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.
Are there any other constraints that are lifted by running privileged? Device hiding?
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.
Privileged add all caps and all host devices. So this won’t do either of those :)
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.
Yup --privileged turns off seccomp, adds all caps, turns off SELinux and AppArmor, adds all devices from most, turns off the device cgroup, this allows processes inside of the container to make and interact with any devices. Exposes read/write kernel file systems.
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.
You might want to add something equivalent to --device all where the Container Runtime would expose all devices from the host to the container and turn off device cgroup. As a separate knob. Not sure how many people other then people debugging would ever turn these knobs.
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.
CRI-O Does whatever Kubernetes tells it to do, If K8s supports rawproc/CRI-O will support Rawproc.
podman currently does everything that docker does.
Know idea what kind of controls RKT has.
LXC can probably do anything, since it has knobs for everything.
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.
You might want to add something equivalent to --device all where the Container Runtime would expose all devices from the host to the container and turn off device cgroup.
Yeah, that's what I was getting at. We already expose options for capabiltiise, seccomp, and AppArmor. I think we're missing a knob to bipass SELinux (or whatever the privileged behavior is there), disable the device cgroup, and rawproc. Is there anything else?
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.
Which users would I definitely not give rawProc to? Might be good to link/include a capsule "here's why /proc/sys being writable is / is-not dangerous" description here.
@rhatdan I know this was because we wanted to close off the option, if selinux is enough to protect those subsequently I'm less worried.
I want to run unprivileged nested containers if we have sufficient defense behind /proc now.
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.
Which users would I definitely not give rawProc to?
Someone evil? Who would possibly try to exploit the info in proc with an 0day. Remember the user inside is still unprivileged so doing things to proc would be off limits regardless. So we could make it so rawProc is only allowed if the user !=0 :)
/cc @Random-Liu @mrunalp |
Added a bit more information on usecases hopefully that is useful. |
and setting as read-only certain paths in `/proc`. This is to prevent data | ||
from being exposed into a container that should not be. However, there are | ||
certain use-cases where it is necessary to turn this off. | ||
|
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.
Instead of adding a rawproc
option at CRI level, why don't we just expose maskedPaths
and readonlyPaths
(link) in CRI?
I'm not saying that we should do that, but I want to make sure we've considered that option.
I think it is more generic. If needed, we can leverage that to mask/unmask or set read-only
for other paths in the future, instead of adding another rawXXX
, e.g. rawsys
.
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 agree with @Random-Liu. We could just expose maskedPaths and readonlyPaths.
A higher level abstraction makes more sense when it isn't portable between runtimes.
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 thats for cri, but what do you want for the top level, the same? I will do whatever everyone wants, I really have no fight in the naming, I just need a way to do this.
|
||
It will be false by default. | ||
|
||
This will also add a `AllowRawProc` to the `PodSecurityPolicy` to allow |
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.
My $.02: The proposal should be about adding a feature in kubernetes, and have sections on kubernetes API, and CRI changes, respectively. The title and the way that the content is arranged seem to suggest that this is only a CRI change.
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.
@yujuhong i had missed this section in sig-node today, agree it would be good to start from the user facing API down to the CRI.
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 really don't mind what the CRI api is, i really just need this in the top level, what do you suggest then? making this proposal just for the top level, you all can implement in cri like i did in docker...
moby/moby#36644
I really don't even care about the name, lol, I just need a way to do it.
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.
@jessfraz CRI is only a small part of this proposal. The more important questions are why Kubernetes needs this, what the use cases are, and how this would be represented in the Kubernetes API. We can discuss how to add this option to CRI after those are nailed down.
I am unclear about the use cases. Could you explain why nested containers are needed or point us to related issues and proposals? As for adding this to PodSecurityPolicy, I think there are more people with opinions on that (hence my suggestion on changing the proposal to get reviews from the right folks).
If I'm wrong and this is a pure CRI change to support finer-grained control (e.g., break down of Privileged
), let me know and we can review as it is.
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.
this is why I need it https://blog.jessfraz.com/post/building-container-images-securely-on-kubernetes/
|
||
It will be false by default. | ||
|
||
This will also add a `AllowRawProc` to the `PodSecurityPolicy` to allow |
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.
Similar questions with https://github.com/kubernetes/community/pull/1934/files#r175907931.
At Kubernetes API level, should we expose highest level abstraction privileged
? Or several lower level abstractions rawproc
, rawsys
, rawXXX
? Or expose maskedPaths
and readonlyPaths
directly?
None of them seems ideal to me.
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 think it is confusing for users at the top level to have the options of configuring everything but up to you all.
This proposal suggests adding the following to `bool` `LinuxSandboxSecurityContext`: | ||
|
||
``` | ||
bool raw_proc |
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.
repeated string raw_access
instead of bool?
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.
thats sounds right :)
I am going to redo this as a proposal for the high level k8s api, and you all can figure out naming for cri however you want, how does that sound? |
Updated to be a proposal for the top level api, CRI can do as it pleases :) |
The title probably should be updated now to avoid any confusion. |
@Random-Liu @mrunalp how do you want to do this with cri i am happy to open a PR :) or if you all want a certain way that can be as well |
am i the first in this chicken and egg problem lol |
@jessfraz There are 2 options:
Today it is still container runtime handles the default OCI spec, so option 2 seems align more with today's model. But since the default is commonly agreed on, I do hope we can move the behavior up to kubelet gradually (option 1), instead of having a duplicated implementation in each container runtime, so I personally prefer option 1. Another question is how do we support this in dockershim? I'm fine with not supporting it, but would like to know whether it is possible. :) @yujuhong Do you have any preference? |
I have no preference on the first since that is yours all to maintain :) so I am happy to make the PR any way you want , going to do option 1 as far as docker, i made a PR but let's turn off support until that's in and then i can add support after :) |
opened kubernetes/kubernetes#64283 |
Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. Add a ProcMount option to the SecurityContext & AllowedProcMountTypes to PodSecurityPolicy So there is a bit of a chicken and egg problem here in that the CRI runtimes will need to implement this for there to be any sort of e2e testing. **What this PR does / why we need it**: This PR implements design proposal kubernetes/community#1934. This adds a ProcMount option to the SecurityContext and AllowedProcMountTypes to PodSecurityPolicy Relies on google/cadvisor#1967 **Release note**: ```release-note ProcMount added to SecurityContext and AllowedProcMounts added to PodSecurityPolicy to allow paths in the container's /proc to not be masked. ``` cc @Random-Liu @mrunalp
Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. Add a ProcMount option to the SecurityContext & AllowedProcMountTypes to PodSecurityPolicy So there is a bit of a chicken and egg problem here in that the CRI runtimes will need to implement this for there to be any sort of e2e testing. **What this PR does / why we need it**: This PR implements design proposal kubernetes/community#1934. This adds a ProcMount option to the SecurityContext and AllowedProcMountTypes to PodSecurityPolicy Relies on google/cadvisor#1967 **Release note**: ```release-note ProcMount added to SecurityContext and AllowedProcMounts added to PodSecurityPolicy to allow paths in the container's /proc to not be masked. ``` cc @Random-Liu @mrunalp Kubernetes-commit: 39004e852bb523d0497343705ee2bf42b4e9c3e3
Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. Add a ProcMount option to the SecurityContext & AllowedProcMountTypes to PodSecurityPolicy So there is a bit of a chicken and egg problem here in that the CRI runtimes will need to implement this for there to be any sort of e2e testing. **What this PR does / why we need it**: This PR implements design proposal kubernetes/community#1934. This adds a ProcMount option to the SecurityContext and AllowedProcMountTypes to PodSecurityPolicy Relies on google/cadvisor#1967 **Release note**: ```release-note ProcMount added to SecurityContext and AllowedProcMounts added to PodSecurityPolicy to allow paths in the container's /proc to not be masked. ``` cc @Random-Liu @mrunalp Kubernetes-commit: 39004e852bb523d0497343705ee2bf42b4e9c3e3
Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. Add a ProcMount option to the SecurityContext & AllowedProcMountTypes to PodSecurityPolicy So there is a bit of a chicken and egg problem here in that the CRI runtimes will need to implement this for there to be any sort of e2e testing. **What this PR does / why we need it**: This PR implements design proposal kubernetes/community#1934. This adds a ProcMount option to the SecurityContext and AllowedProcMountTypes to PodSecurityPolicy Relies on google/cadvisor#1967 **Release note**: ```release-note ProcMount added to SecurityContext and AllowedProcMounts added to PodSecurityPolicy to allow paths in the container's /proc to not be masked. ``` cc @Random-Liu @mrunalp Kubernetes-commit: 39004e852bb523d0497343705ee2bf42b4e9c3e3
Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. Add a ProcMount option to the SecurityContext & AllowedProcMountTypes to PodSecurityPolicy So there is a bit of a chicken and egg problem here in that the CRI runtimes will need to implement this for there to be any sort of e2e testing. **What this PR does / why we need it**: This PR implements design proposal kubernetes/community#1934. This adds a ProcMount option to the SecurityContext and AllowedProcMountTypes to PodSecurityPolicy Relies on google/cadvisor#1967 **Release note**: ```release-note ProcMount added to SecurityContext and AllowedProcMounts added to PodSecurityPolicy to allow paths in the container's /proc to not be masked. ``` cc @Random-Liu @mrunalp Kubernetes-commit: 39004e852bb523d0497343705ee2bf42b4e9c3e3
Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. Add a ProcMount option to the SecurityContext & AllowedProcMountTypes to PodSecurityPolicy So there is a bit of a chicken and egg problem here in that the CRI runtimes will need to implement this for there to be any sort of e2e testing. **What this PR does / why we need it**: This PR implements design proposal kubernetes/community#1934. This adds a ProcMount option to the SecurityContext and AllowedProcMountTypes to PodSecurityPolicy Relies on google/cadvisor#1967 **Release note**: ```release-note ProcMount added to SecurityContext and AllowedProcMounts added to PodSecurityPolicy to allow paths in the container's /proc to not be masked. ``` cc @Random-Liu @mrunalp Kubernetes-commit: 39004e852bb523d0497343705ee2bf42b4e9c3e3
Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. Add a ProcMount option to the SecurityContext & AllowedProcMountTypes to PodSecurityPolicy So there is a bit of a chicken and egg problem here in that the CRI runtimes will need to implement this for there to be any sort of e2e testing. **What this PR does / why we need it**: This PR implements design proposal kubernetes/community#1934. This adds a ProcMount option to the SecurityContext and AllowedProcMountTypes to PodSecurityPolicy Relies on google/cadvisor#1967 **Release note**: ```release-note ProcMount added to SecurityContext and AllowedProcMounts added to PodSecurityPolicy to allow paths in the container's /proc to not be masked. ``` cc @Random-Liu @mrunalp Kubernetes-commit: 39004e852bb523d0497343705ee2bf42b4e9c3e3
Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. Add a ProcMount option to the SecurityContext & AllowedProcMountTypes to PodSecurityPolicy So there is a bit of a chicken and egg problem here in that the CRI runtimes will need to implement this for there to be any sort of e2e testing. **What this PR does / why we need it**: This PR implements design proposal kubernetes/community#1934. This adds a ProcMount option to the SecurityContext and AllowedProcMountTypes to PodSecurityPolicy Relies on google/cadvisor#1967 **Release note**: ```release-note ProcMount added to SecurityContext and AllowedProcMounts added to PodSecurityPolicy to allow paths in the container's /proc to not be masked. ``` cc @Random-Liu @mrunalp Kubernetes-commit: 39004e852bb523d0497343705ee2bf42b4e9c3e3
Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. Add a ProcMount option to the SecurityContext & AllowedProcMountTypes to PodSecurityPolicy So there is a bit of a chicken and egg problem here in that the CRI runtimes will need to implement this for there to be any sort of e2e testing. **What this PR does / why we need it**: This PR implements design proposal kubernetes/community#1934. This adds a ProcMount option to the SecurityContext and AllowedProcMountTypes to PodSecurityPolicy Relies on google/cadvisor#1967 **Release note**: ```release-note ProcMount added to SecurityContext and AllowedProcMounts added to PodSecurityPolicy to allow paths in the container's /proc to not be masked. ``` cc @Random-Liu @mrunalp Kubernetes-commit: 39004e852bb523d0497343705ee2bf42b4e9c3e3
Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. Add a ProcMount option to the SecurityContext & AllowedProcMountTypes to PodSecurityPolicy So there is a bit of a chicken and egg problem here in that the CRI runtimes will need to implement this for there to be any sort of e2e testing. **What this PR does / why we need it**: This PR implements design proposal kubernetes/community#1934. This adds a ProcMount option to the SecurityContext and AllowedProcMountTypes to PodSecurityPolicy Relies on google/cadvisor#1967 **Release note**: ```release-note ProcMount added to SecurityContext and AllowedProcMounts added to PodSecurityPolicy to allow paths in the container's /proc to not be masked. ``` cc @Random-Liu @mrunalp Kubernetes-commit: 39004e852bb523d0497343705ee2bf42b4e9c3e3
Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. Add a ProcMount option to the SecurityContext & AllowedProcMountTypes to PodSecurityPolicy So there is a bit of a chicken and egg problem here in that the CRI runtimes will need to implement this for there to be any sort of e2e testing. **What this PR does / why we need it**: This PR implements design proposal kubernetes/community#1934. This adds a ProcMount option to the SecurityContext and AllowedProcMountTypes to PodSecurityPolicy Relies on google/cadvisor#1967 **Release note**: ```release-note ProcMount added to SecurityContext and AllowedProcMounts added to PodSecurityPolicy to allow paths in the container's /proc to not be masked. ``` cc @Random-Liu @mrunalp Kubernetes-commit: 39004e852bb523d0497343705ee2bf42b4e9c3e3
Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. Add a ProcMount option to the SecurityContext & AllowedProcMountTypes to PodSecurityPolicy So there is a bit of a chicken and egg problem here in that the CRI runtimes will need to implement this for there to be any sort of e2e testing. **What this PR does / why we need it**: This PR implements design proposal kubernetes/community#1934. This adds a ProcMount option to the SecurityContext and AllowedProcMountTypes to PodSecurityPolicy Relies on google/cadvisor#1967 **Release note**: ```release-note ProcMount added to SecurityContext and AllowedProcMounts added to PodSecurityPolicy to allow paths in the container's /proc to not be masked. ``` cc @Random-Liu @mrunalp Kubernetes-commit: 39004e852bb523d0497343705ee2bf42b4e9c3e3
Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. Add a ProcMount option to the SecurityContext & AllowedProcMountTypes to PodSecurityPolicy So there is a bit of a chicken and egg problem here in that the CRI runtimes will need to implement this for there to be any sort of e2e testing. **What this PR does / why we need it**: This PR implements design proposal kubernetes/community#1934. This adds a ProcMount option to the SecurityContext and AllowedProcMountTypes to PodSecurityPolicy Relies on google/cadvisor#1967 **Release note**: ```release-note ProcMount added to SecurityContext and AllowedProcMounts added to PodSecurityPolicy to allow paths in the container's /proc to not be masked. ``` cc @Random-Liu @mrunalp Kubernetes-commit: 39004e852bb523d0497343705ee2bf42b4e9c3e3
Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. Add a ProcMount option to the SecurityContext & AllowedProcMountTypes to PodSecurityPolicy So there is a bit of a chicken and egg problem here in that the CRI runtimes will need to implement this for there to be any sort of e2e testing. **What this PR does / why we need it**: This PR implements design proposal kubernetes/community#1934. This adds a ProcMount option to the SecurityContext and AllowedProcMountTypes to PodSecurityPolicy Relies on google/cadvisor#1967 **Release note**: ```release-note ProcMount added to SecurityContext and AllowedProcMounts added to PodSecurityPolicy to allow paths in the container's /proc to not be masked. ``` cc @Random-Liu @mrunalp Kubernetes-commit: 39004e852bb523d0497343705ee2bf42b4e9c3e3
What is the security tradeoff a cluster operator is making by allowing unmasked ProcMounts in their pod security policies? |
Could somebody answer this plz? |
@dimm0 does this help? #1934 (comment) |
Yup, thanks |
add ProcMount option
/cc sig-node-proposals
see also:
moby/moby#36597
opencontainers/runc#1658 (comment)