KEP 2763: Ambient capabilities in Kubernetes #2757

vinayakankugoyal · 2021-05-19T19:08:26Z

Hello! thank you for taking the time to review this KEP.

related issue kubernetes/kubernetes#56374

tracking issue in containerd: containerd/containerd#5644

KEP issue: #2763

k8s-ci-robot · 2021-05-19T19:08:33Z

Hi @vinayakankugoyal. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

vinayakankugoyal · 2021-05-19T21:15:34Z

/cc @tallclair

tallclair

Thanks for picking this up! I'd really like to see this feature.

tallclair · 2021-05-20T16:23:02Z

keps/sig-security/NNNN-ambient-capabilities/README.md

+[documentation style guide]: https://github.com/kubernetes/community/blob/master/contributors/guide/style-guide.md
+-->
+
+This KEP proposes that kubernetes provide a way to set ambient capabilities for containers through the Pod manifest. It also proposes changes that must be made to containerd to enable ambient capabilities end-to-end.


I assume there's a separate process for proposing features for containerd. It probably makes senes to come to an understanding of what we want in k8s first, but I'd like to see the containerd (and ideally cri-o) changes merge before this KEP is marked implementable.

Are there runtimes other than containerd we should care about? How do we usually expand CRI?

Currently this KEP focusses on containerd. I think we should expand this to other runtimes in the future. Coordinating this across multiple runtimes at once would be extremely challenging.

@tallclair

but I'd like to see the containerd (and ideally cri-o) changes merge before this KEP is marked implementable

but wouldn't we have to first update the CRI API for them to start reading the AddAmbientCapabilities field?
I think that the order of operation should be:

Update CRI API

Update containerd and CRI-O

Update k8s core APIs

WDYT?

Yeah, that makes sense. Can we just make sure a containerd & cri-o maintainer approves this KEP? I recommend @mrunalp for cri-O, I'm not sure who's active on containerd these days.

Yup we have volunteers for reviewing/approving the KEP from
CRI API - @SergeyKanzhelev
containerd - @mikebrow
cri-O - @mrunalp

keps/sig-security/NNNN-ambient-capabilities/README.md

tallclair · 2021-05-20T16:28:33Z

keps/sig-security/NNNN-ambient-capabilities/README.md

+	Capabilities *Capabilities `json:"capabilities,omitempty" protobuf:"bytes,1,opt,name=capabilities"`
+     // The ambient capabilities to add when running containers as non-root.
+     // +optional
+     AmbientCapabilities []Capability `json:"ambientCapabilities,omitempty" protobuf:"bytes,1,rep,name=ambientCapabilities,casttype=[]Capability"`


If we go with a separate field, I suggest putting it in the Capabilities struct. E.g. Ambient works like add, but also makes it ambient.

Good call! I agree we should do this as well.

tallclair · 2021-05-20T16:29:27Z

keps/sig-security/NNNN-ambient-capabilities/README.md

+
+### Changes to kubernetes API (https://pkg.go.dev/k8s.io/api/core/v1)
+
+Reusing the existing capabilities field in the securityContext might cause confusion and we propose adding a new field in the SecurityContext called ambientCapabilities. 


I would be open to reusing the add field, as long as the default set is still not included in the ambient set.

I think I agree. We inherited the API shape from docker here, and it's not ideal. If we are working in the confines of today's shape, it seems plausible that add: CAP_* on a non-root pod actually means the spec author wanted the process to have CAP_*.

IIUC what you are suggesting is that add will add the explicitly added capability to the ambient set and the default ones would still be added like before? What I envisioned was that if you are adding a capability using addAmbient then under the hood we drop all the default capabilities and only add the explicitly added capability to the ambient set.

Yeah. IIRC, the way Add works today is it adds it to the bounding & effective sets, the same for the default capabilities. What I'm suggesting is that the default capabilities are still only added to bounding & effective (as is proposed), and Add adds to those sets, but also to Ambient. Does that make sense?

Default and explicitly added capabilities are added to inherited, permitted, bounding and effective sets. See this for the code location where this is done.

What in think you are suggesting:

when someone adds a capability explicitly - that capability should be added to all sets i.e. inherited, permitted, bounding, effective and ambient.

the default capabilities should only be added to inherited. permitted,bounding and effective sets.

The advantage of your approach is that:

we would not require change to kubernetes API, we would still require changes to the CRI API so that we can distinguish which ones are added to the ambient set vs which ones are not.

The disadvantage of your approach is:

we are changing the behavior of an existing field. (not saying this is bad, I think this shouldn't break anyone)

I think kubernetes/kubernetes#56374 (comment) talks about why we would want to add a new field.
Adding a new field is a simple add and would make it easier to implement the feature. (We wouldn't have to worry about breaking something as the old behavior would remain untouched)

keps/sig-security/NNNN-ambient-capabilities/README.md

tallclair · 2021-05-20T16:33:17Z

keps/sig-security/NNNN-ambient-capabilities/README.md

+```
+
+
+Notes about how ambientCapabilities would work:


I wonder if we should forbid adding certain capabilities if the container sets runAsNonRoot? For example, runAsNonRoot is probably not adding much protection if you add CAP_SYS_ADMIN (or even CAP_DAC_OVERRIDE, in the default set) to the ambient set.

Yeah I was thinking the same! Great call out!

tallclair · 2021-05-20T16:36:29Z

keps/sig-security/NNNN-ambient-capabilities/README.md

+	DropCapabilities     []string `protobuf:"bytes,2,rep,name=drop_capabilities,json=dropCapabilities,proto3" json:"drop_capabilities,omitempty"`
+	XXX_NoUnkeyedLiteral struct{} `json:"-"`
+	XXX_sizecache        int32    `json:"-"`
+          // List of ambient capabilities to add.


Should AddAmbientCapabilities also add them to the inherited & bound sets? I think so?

I think we would need them to be added to inheritable and permitted set. See https://lwn.net/Articles/636533/.
TLDR;
AmbientCapabilities obey the invariant that no bit can ever be set in ambient set if it is
not set in both permitted and inheritable.

tallclair · 2021-05-20T16:37:12Z

keps/sig-security/NNNN-ambient-capabilities/README.md

+	XXX_NoUnkeyedLiteral struct{} `json:"-"`
+	XXX_sizecache        int32    `json:"-"`
+          // List of ambient capabilities to add.
+          AddAmbientCapabilities []string


Note: Even if we end up reusing the existing Add capabilities field in the k8s API, I agree that it should be kept separate at the CRI level.

tallclair · 2021-05-20T16:42:49Z

/cc @mrunalp

keps/sig-security/2763-ambient-capabilities/README.md

mrunalp · 2021-05-21T04:14:16Z

keps/sig-security/2763-ambient-capabilities/README.md

+	Drop []Capability `json:"drop,omitempty" protobuf:"bytes,2,rep,name=drop,casttype=Capability"`
+  // Ambient capabilities to add
+	// +optional
+	Ambient []Capability `json:"ambient,omitempty" protobuf:"bytes,2,rep,name=ambient,casttype=Capability"`


An alternative could be a boolean for ambient that uses the default capabilties list.

bool does not work because containerd/docker add some capabilities by default and they will all become ambient for a non-root container, essentially making it root.

haircommander · 2021-05-25T15:15:54Z

keps/sig-security/2763-ambient-capabilities/README.md

+
+### Changes to kubernetes API (https://pkg.go.dev/k8s.io/api/core/v1)
+
+<<[UNRESOLVED should we reuse add instead of adding new field]>>


based on this note kubernetes/kubernetes#56374 (comment)
I don't think we should reuse add. I think having an additional slice in the Capabilities struct that is add-only is a clean addition

Here is my two cents about the k8s API design. (Talked with Vinayak offline already)

In one end of the spectrum, k8s API can expose the exact interfaces of the layer below it. In this case, it is Linux capabilities API. It means k8s API can just allow user directly interact with permitted, inherited, effective, bounding and ambient sets. This way gives user 100% control and flexibility as what Linux allows. This also means k8s API need to adapt with Linux capabilities API. For example, a new thing in Linux capabilities results in a new thing in k8s API.

In the other end, k8s API defines and abstracts its own way of interacting with capabilities. So k8s API can be simpler, at the cost of flexibility. And it should hide details of the underlying OS. The original design of securityContext.capabilities.add and drop are pretty concise. But it is unclear if it supports both root and non-root. It seems to support but from this k8s doc, it doesn't support non root.

Linux Capabilities: Give a process some privileges, but not all the privileges of the root user.

At this moment, we need to be backward-compatible, which is the only reason that I agree we need a new field. With the new field, I propose to clarify the spec, based on [kubernetes/kubernetes#56374 (comment)].

add: if root user, add capabilities; if non-root, behavior is unspecified

drop: if root user, drop capabilities; if non-root, behavior is unspecified

setForNonRoot (open to suggestion, I name it based on runAsNonRoot): if non-root user, set capabilities; if root, no ops

My goals are:

clarify the root vs non-root cases

clarify how the new field work with existing ones

hide linux details

Some New thoughts after talked with Vinayak today. I prefer (1) > (3) > (2)

Option 1: Reuse add and drop

The capabilities will be added to ambient set, besides the current capability sets. Thus it will work for non-root user case without file capability.

It will not break existing scenarios.

Option 2: Add new field - ambient

Users need to understand all those Linux capability concept: https://lwn.net/Articles/636533/

We need to enhance the add and drop spec by saying permitted, inheritable, bounding and effective set.

Option 3: Add new field - addForNonRoot

add is for any process(parent or child) with root user

drop is for any process(parent or child) with root user

addForNonRoot is for any process(parent or child) with non-root user

dchen1107 · 2021-06-01T16:29:35Z

cc/ @SergeyKanzhelev @qiutongs

vinayakankugoyal · 2021-06-01T16:47:19Z

/cc @SergeyKanzhelev @qiutongs

tabbysable · 2021-10-02T08:18:11Z

/approve

tabbysable · 2021-10-02T08:19:56Z

/label tide/merge-method-squash

k8s-ci-robot · 2021-10-04T02:02:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tabbysable, vinayakankugoyal

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:

~~keps/sig-security/OWNERS~~ [tabbysable]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tabbysable · 2021-10-06T22:59:33Z

/lgtm

vinayakankugoyal · 2021-10-07T01:26:14Z

@tabbysable could you please LGTM again (sorry for the inconvenience). There was a typo in the kep metadata that was causing the verify-kep-metadata.sh test to fail. Those are the only changes since last LGTM. Thank you!

tabbysable · 2021-10-07T02:12:53Z

we should probably bump the milestones to 1.24, since this is certainly not going into 1.23, just to avoid confusion

tabbysable · 2021-10-07T02:20:45Z

alright everyone, buckle up:
/lgtm

vinayakankugoyal · 2021-10-07T02:21:15Z

/retest

* KEP 2763: Ambient capabilities in Kubernetes * KEP 2763: Add use cases and polish Signed-off-by: Alexey Perevalov <alexey.perevalov@huawei.com> * update prod-readiness yaml. Change beta to alpha. * Minor formatting fixes. * Delete 2763.yaml because merging provisionally. * Update kep.yaml * Update kep.yaml Co-authored-by: Alexey Perevalov <alexey.perevalov@huawei.com>

giuseppe · 2022-04-26T09:49:19Z

keps/sig-security/2763-ambient-capabilities/README.md

+There are 2 options here:-
+- **Option 1:** Reuse Add field in [Capabilities](https://pkg.go.dev/k8s.io/api/core/v1#Capabilities)
+
+  When a capability gets added explicitly to a non-root container it also gets added to the ambient set in addition to getting added to inheritable, permitted, bounding and effective sets. The default capabilities are not added to the ambient set.


having capabilities being added to the inheritable set turned out to be a CVE affecting multiple runtimes (Moby, containerd, CRI-O, Podman...), more details in the Moby advisory: GHSA-2mm7-x5h6-5pvq

Having the capabilities added directly to the ambient set requires them to be present in the inheritable set as well, thus getting back the old behavior.

I think this option should not be considered.

Jc2k · 2022-04-26T10:42:41Z

keps/sig-security/2763-ambient-capabilities/README.md

+
+#### Restricted ambient capabilities.
+
+<<[UNRESOLVED what is the set of capabilities that we should allow to be made ambient]>>


Previously when this was discussed PSP's were deprecated and there wasn't alternative. But we now have a PodSecurity admission controller in beta and enabled by default as of 1.23.

So I think if there are any restrictions it should be done in the PodSecurity admission controller. Most should probably be under the 'Privileged' category.

@Jc2k are you thinking of a change to the pod security standards? These are what you can enforce with Pod Security admission.

It depends.

For context, let me stress again that when this section of the KEP were first discussed, the admission controller wasn't in beta (if it existed at all) so it wasn't considered a valid solution to this UNRESOLVED part of the KEP.

In the case where we re-use securityContext.capabilities.add, the existing standards look like they are valid for root and non-root containers. Neither would be allowed to use CAP_SYS_ADMIN or CAP_DAC_OVERRIDE etc unless they were using the privileged policy level. This is good.

If we are not reusing securityContext.capabilities.add, then I think the standards should be updated anyway. They currently cover all the ways you can add capabilities to containers. If there is a new way to add a capabitility to a container, it should be covered by the standards.

Jc2k · 2022-04-26T11:08:57Z

I'm aware of at least one popular CNI that now has a non-root mode. It does this through suid binaries. I'd much prefer a DaemonSet with an explicit and limited capability allow list to a container image using suid binaries, even if both ultimately have powerful permission sets.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 19, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 19, 2021

k8s-ci-robot requested review from IanColdwater and tabbysable May 19, 2021 19:08

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/security Categorizes an issue or PR as relevant to SIG Security. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 19, 2021

vinayakankugoyal force-pushed the implentable branch from bfe446f to cb42032 Compare May 19, 2021 20:43

k8s-ci-robot requested a review from tallclair May 19, 2021 21:15

vinayakankugoyal force-pushed the implentable branch 3 times, most recently from 62ea37c to 10fa18e Compare May 20, 2021 15:27

tallclair reviewed May 20, 2021

View reviewed changes

k8s-ci-robot requested a review from mrunalp May 20, 2021 16:42

vinayakankugoyal mentioned this pull request May 21, 2021

Support for ambient capabilities in kubernetes. #2763

Open

6 tasks

vinayakankugoyal force-pushed the implentable branch from 10fa18e to effbb42 Compare May 21, 2021 01:40

vinayakankugoyal changed the title ~~Initial KEP for ambient capabilities in kubernetes.~~ KEP for ambient capabilities in kubernetes. May 21, 2021

vinayakankugoyal force-pushed the implentable branch from effbb42 to d2457f1 Compare May 21, 2021 03:34

mrunalp reviewed May 21, 2021

View reviewed changes

keps/sig-security/2763-ambient-capabilities/README.md Outdated Show resolved Hide resolved

mrunalp reviewed May 21, 2021

View reviewed changes

vinayakankugoyal force-pushed the implentable branch 2 times, most recently from 5ada203 to 57720d4 Compare May 22, 2021 01:11

haircommander reviewed May 25, 2021

View reviewed changes

k8s-ci-robot requested review from qiutongs and SergeyKanzhelev June 1, 2021 16:47

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 2, 2021

Delete 2763.yaml because merging provisionally.

846575e

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2021

k8s-ci-robot assigned tabbysable Oct 6, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 6, 2021

Update kep.yaml

3b84e9c

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 7, 2021

Update kep.yaml

0cfb55c

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 7, 2021

k8s-ci-robot merged commit 3133297 into kubernetes:master Oct 7, 2021

k8s-ci-robot added this to the v1.23 milestone Oct 7, 2021

AlexeyPerevalov mentioned this pull request Oct 25, 2021

Ambient capability containerd/containerd#6153

Closed

qiutongs mentioned this pull request Nov 2, 2021

CRI: Drop effective and permitted cap for non-root user. containerd/containerd#4669

Closed

cpuguy83 mentioned this pull request Nov 17, 2021

CRI: Support enable_unprivileged_icmp and enable_unprivileged_ports options containerd/containerd#6170

Merged

giuseppe reviewed Apr 26, 2022

View reviewed changes

Jc2k reviewed Apr 26, 2022

View reviewed changes

palnabarun mentioned this pull request Jun 12, 2022

canonical kep number json field palnabarun/enhancements#1

Closed

frezbo mentioned this pull request Jul 17, 2022

feat: follow KEP-2568 non-root enhancements siderolabs/talos#5917

Merged

7 tasks

palnabarun mentioned this pull request Jul 26, 2022

KEP Manifest Generator palnabarun/enhancements#2

Closed


		### Changes to kubernetes API (https://pkg.go.dev/k8s.io/api/core/v1)

		Reusing the existing capabilities field in the securityContext might cause confusion and we propose adding a new field in the SecurityContext called ambientCapabilities.


		### Changes to kubernetes API (https://pkg.go.dev/k8s.io/api/core/v1)

		<<[UNRESOLVED should we reuse add instead of adding new field]>>


		#### Restricted ambient capabilities.

		<<[UNRESOLVED what is the set of capabilities that we should allow to be made ambient]>>

KEP 2763: Ambient capabilities in Kubernetes #2757

KEP 2763: Ambient capabilities in Kubernetes #2757

Conversation

vinayakankugoyal commented May 19, 2021 • edited by ehashman Loading

k8s-ci-robot commented May 19, 2021

vinayakankugoyal commented May 19, 2021

tallclair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinayakankugoyal Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

vinayakankugoyal Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinayakankugoyal Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tallclair commented May 20, 2021

Choose a reason for hiding this comment

vinayakankugoyal Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiutongs Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

dchen1107 commented Jun 1, 2021

vinayakankugoyal commented Jun 1, 2021

tabbysable commented Oct 2, 2021

tabbysable commented Oct 2, 2021

k8s-ci-robot commented Oct 4, 2021

tabbysable commented Oct 6, 2021

vinayakankugoyal commented Oct 7, 2021

tabbysable commented Oct 7, 2021

tabbysable commented Oct 7, 2021

vinayakankugoyal commented Oct 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jc2k commented Apr 26, 2022

vinayakankugoyal commented May 19, 2021 •

edited by ehashman

Loading

vinayakankugoyal Jun 23, 2021 •

edited

Loading

vinayakankugoyal Jun 23, 2021 •

edited

Loading

vinayakankugoyal Jun 24, 2021 •

edited

Loading

vinayakankugoyal Jun 23, 2021 •

edited

Loading

qiutongs Jun 25, 2021 •

edited

Loading