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

api: add seccomp adjustment #123

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tych0
Copy link

@tych0 tych0 commented Nov 22, 2024

This adds an adjustment for seccomp policies. The intent is that people can wholesale replace policies, or parse them, make some changes, and then send them back. Sending them to NRI via containerd requires some containerd patches as well, those are here: https://github.com/tych0/containerd/commits/nri-seccomp/

Specifically, we are interested in making the listenerPath of the policy dynamic based on a k8s pod spec, so we can't use the Localhost custom policy (well, we can use most of it, except for listenerPath, which we have an NRI plugin to change based on this code).

This patch is a lot of boilerplate, which is unfortunate. There is a much smaller but similar patch:
tych0@a70547a but it involves directly serializing a runtime-spec string

Finally, note the comment in generate.go: the runtime-tools generate code does not have complete coverage for seccomp stuff, so I opted to not use any of it, vs. adding more stuff to runtime-tools. The fact that there are human and computer names is also confusing, it seems like we should stick to the computer names for this particular interface.

Copy link
Member

@dcantah dcantah left a comment

Choose a reason for hiding this comment

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

Nit comments on some things, will do another round later today for the actual implementation

pkg/api/seccomp.go Outdated Show resolved Hide resolved
pkg/api/seccomp.go Outdated Show resolved Hide resolved
pkg/api/seccomp.go Outdated Show resolved Hide resolved
pkg/api/seccomp.go Outdated Show resolved Hide resolved
pkg/api/api.proto Outdated Show resolved Hide resolved
pkg/runtime-tools/generate/generate.go Outdated Show resolved Hide resolved
@tych0
Copy link
Author

tych0 commented Nov 25, 2024


  + '[' -z https://api.github.com/repos/containerd/nri/pulls/123/commits ']'
  ++ curl https://api.github.com/repos/containerd/nri/pulls/123/commits
  ++ jq -r '.[0].parents[0].sha + "..HEAD"'
    % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                   Dload  Upload   Total   Spent    Left  Speed
  
    0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  100   278  100   278    0     0   2986      0 --:--:-- --:--:-- --:--:--  3021
  jq: error (at <stdin>:1): Cannot index object with number

seems like maybe curl needs a --fail there...

pkg/api/seccomp.go Outdated Show resolved Hide resolved
pkg/runtime-tools/generate/generate.go Outdated Show resolved Hide resolved
This adds an adjustment for seccomp policies. The intent is that people can
wholesale replace policies, or parse them, make some changes, and then send
them back. Sending them *to* NRI via containerd requires some containerd
patches as well, those are here: https://github.com/tych0/containerd/commits/nri-seccomp/

Specifically, we are interested in making the listenerPath of the policy
dynamic based on a k8s pod spec, so we can't use the Localhost custom
policy (well, we can use most of it, except for listenerPath, which we have
an NRI plugin to change based on this code).

This patch is a lot of boilerplate, which is unfortunate. There is a much
smaller but similar patch:
a70547a
but it involves directly serializing a runtime-spec string

Finally, note the comment in generate.go: the runtime-tools generate code
does not have complete coverage for seccomp stuff, so I opted to not use
any of it, vs. adding more stuff to runtime-tools. The fact that there are
human and computer names is also confusing, it seems like we should stick
to the computer names for this particular interface.

Signed-off-by: Tycho Andersen <[email protected]>
Copy link
Member

@klihub klihub left a comment

Choose a reason for hiding this comment

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

LGTM.
I think this is the first addition of something primarily/solely security-related to NRI (well, apart from #124, but I think that's in a way the flip-side of the same coin as I guess you guys need both of them). @mikebrow was already before these of the opinion that some of the features in NRI should be possible to lock down administratively, so I'd like him to chime in, too. I'd expect that discussion to be revived.

@mikebrow
Copy link
Member

mikebrow commented Dec 2, 2024

LGTM. I think this is the first addition of something primarily/solely security-related to NRI (well, apart from #124, but I think that's in a way the flip-side of the same coin as I guess you guys need both of them). @mikebrow was already before these of the opinion that some of the features in NRI should be possible to lock down administratively, so I'd like him to chime in, too. I'd expect that discussion to be revived.

Nod, fields currently managed by k8s over the cri api, esp. those configurable in kubelet and/or on the pod spec itself, these need some deep discussion around the cri contract. Had a good talk with Sam on these at kubecon. In a nutshell, I think we should consider admin config switches for controlling our "default" container runtime security profiles. And I believe we need larger discussions with the other side of the cri (the clients) when we are administratively adjusting the kubernetes security profile (the client's security profile). In the case where we are increasing the security profile I could see the client being ok with that, but if there is a case where we are reducing the security profile, increasing the attack surface, then I think we'd need some way to configure that as a limited / possibly client managed option.

@tych0
Copy link
Author

tych0 commented Dec 2, 2024

In the case where we are increasing the security profile I could see the client being ok with that, but if there is a case where we are reducing the security profile, increasing the attack surface, then I think we'd need some way to configure that as a limited / possibly client managed option.

I appreciate the paranoia. Can you elaborate on the threat model here?

IIUC, NRI plugins speak to the containerd socket, which effectively makes people root (they can ask for a privileged container, mknod the blockdev the host rootfs is on, etc. etc.). The ownership for the containerd socket today is root:root (though it may be relaxed in e.g. containerd/containerd#10454), so you have to be root to do anything here.

If that's the case, the NRI plugin already has root on the box, what is protected against by introducing access control at this level?

@mikebrow
Copy link
Member

mikebrow commented Dec 2, 2024

The plugin is an extension of the containerd/crio daemon, let's leave out the rootless conversation as I'm not talking about a concern for what the container runtime or plugin has access to. Same with the socket, which we can and may at some point secure to known/verified client/server components having predefined certs.

So the threat I'm concerned with is relaxing the requested security context/profiles of a pod/container. I see you have not hit adjustments for the podsandbox, yet.

The pods/containers, or course, are not an extension of the container runtime... they are merely invoked and managed by the runtime via the shim/conmon and possibly runc/crun engines down one more level. When the client, kubelet, requests a container that is not privileged it is expecting that to be the case. Same with the other kubernetes security context/profile options. These pods/containers may be "unknown" to the host/root admin, installed by users updated by vendors of the containers. Sure in some on metal enterprise/standard user environments they may want to take risks and allow all edits by their plugins. Still in other cloud like environments running multi-tenant or just single tenant clusters the containers may not be trusted fully.

https://kubernetes.io/docs/tasks/configure-pod-container/security-context/

The pod sandbox security context via CRI with these security profiles:
https://github.com/containerd/containerd/blob/main/vendor/k8s.io/cri-api/pkg/apis/runtime/v1/api.pb.go#L1040-L1081
The container security context:
https://github.com/containerd/containerd/blob/main/vendor/k8s.io/cri-api/pkg/apis/runtime/v1/api.pb.go#L4020-L4098

In the case of this seccomp adjustment there are certain profiles that we should be able to adjust out of hand, like "runtime/default" Here the plugin being an extension of the container runtime it may, IMO set the default for all or on a per container basis even. However if "unconfined" "" or "localhost/" there is the expectation by the kubelet that we will abide.

@mikebrow
Copy link
Member

mikebrow commented Dec 2, 2024

So in summary, would like to talk to the team on this and possibly allow adjustments to the seccomp profile if we are configured for allowing SecurityContextAdjustments = {"restricted", "allowed", "limited" // to adjust only runtime/defaults}

@tych0
Copy link
Author

tych0 commented Dec 2, 2024

Sure in some on metal enterprise/standard user environments they may want to take risks and allow all edits by their plugins.

But I am confused here: plugins require root on the host already, so plugins can necessarily mess with configuration (e.g. just overwriting a localhost/default.json profile with a new one they like better).

Still in other cloud like environments running multi-tenant or just single tenant clusters the containers may not be trusted fully.

I agree that we do not want to trust the containers, and any such modification done by an NRI plugin should be done with care. But this is about trusting the NRI plugins themselves, who are already root, IIUC.

When the client, kubelet, requests a container that is not privileged it is expecting that to be the case. Same with the other kubernetes security context/profile options. These pods/containers may be "unknown" to the host/root admin, installed by users updated by vendors of the containers.

Agreed: the host admin needs to be very careful what NRI plugins it allows to be installed in light of the fact that they're running untrusted code on their systems, and these NRI plugins effectively have root access and can grant it to other applications. But NRI plugins should surely have the capability to change these things if they want.

@mikebrow
Copy link
Member

mikebrow commented Dec 2, 2024

fyi.. https://github.com/containerd/containerd/tree/main/contrib/seccomp not sure if you saw this code.. note the per platform each having their own defaults.

@mikebrow
Copy link
Member

mikebrow commented Dec 2, 2024

But I am confused here: plugins require root on the host already, so plugins can necessarily mess with configuration (e.g. just overwriting a localhost/default.json profile with a new one they like better).

Yes some aspects of the security context of the pods/containers are modifiable via web hook controllers, install scripts, .... And gradually over time the editable files will be encrypted, such as secrets, maybe also security profiles, ...

Just because you "could" hack the system files with root access doesn't mean we should put a service api together that makes it even easier, and seemingly approved.

I agree that we do not want to trust the containers, and any such modification done by an NRI plugin should be done with care. But this is about trusting the NRI plugins themselves, who are already root, IIUC.

Negative on this is about trusting the nri plugins, I'm not talking about trusting the plugins or not, I presume only trusted plugins will be installed. I'm talking about whether the plugins/container runtime should be "modifying" the confined security context requested by the pod spec, and if it is modified, modified first by a pod spec mutating controller or kubelet itself. Things will get confusing if plugins are modifying a predefined, expected/required, security context, particularly if the modification is allowing additional sys calls.

Agreed: the host admin needs to be very careful what NRI plugins it allows to be installed in light of the fact that they're running untrusted code on their systems, and these NRI plugins effectively have root access and can grant it to other applications. But NRI plugins should surely have the capability to change these things if they want.

Admins should only install trusted plugins. I agree 100% that trusted plugins should be able to change these security context profiles, so long as the changes are allowed as a part of the security design for pods/containers.

FYI the e2e testing code and kubelet client should be able to verify adherence to requested profiles for the created pods/containers.

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.

5 participants