Improve the API description of `PodSecurityContext.SupplementalGroups` to clarify its unfamiliar behavior #113047

everpeace · 2022-10-13T14:39:50Z

What type of PR is this?

/kind documentation
/sig node

What this PR does / why we need it:

As reported #112879, the API description of PodSecurityContext.SupplementalGroups is ambiguous regarding how group membership defined in the container image is handled for the first process in the containers.

Assume a pod spec be like the below:

spec:
  securityContext: { runAsUser:1000, runAsGroup:1000, supplementalGroups:[60000] }
  containers:
  - image: some-image # uid=1000 is defined to belong to gid=50000 in this image

Then, the first process of the container can belong to groups=1000(runAsGroup), 50000(defined in the image), 60000(supplementalGroups) in popular CRI implementations (it was confirmed in containerd and cri-o). here is reproduction code.

I think this behavior would be unfamiliar and confusing to most cluster administrators and users. Moreover, when a cluster enforces some security policy that protects the value of supplementalGroups field, the effect of its enforcement is limited, i.e. user can easily bypass the enforcement just by using a custom image. And, the behavior(unexpected group membership) will cause unexpected file access permission. It would be a security concern in some use cases when using hostPath volumes because uid/gid matters in accessing contents in hostPath volumes.

Thus, this PR improved the API description to clarify this unfamiliar current behavior and added a warning about unexpected group membership for many cluster administrators.

Note: as reported #112879, we once reported this behavior to hackerone.com(#1688374) by following Kubernetes Security and Disclosure Information. Kubernetes Security Response Committee responded that this behavior "works as intended."

Which issue(s) this PR fixes:

#112879 (fixed partially)

Special notes for your reviewer:

This is my first step to resolve #112879 described in #112879 (comment). As a next step, I will write a KEP(kubernetes/enhancements#3619) introducing a new API to customize the behavior (how group membership will be handled in the first process in the container) as proposed in #112879.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-triage-robot · 2022-10-13T15:34:33Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

leilajal · 2022-10-13T16:31:47Z

/remove-sig api-machinery

everpeace · 2022-10-14T04:03:20Z

/assign @liggitt

liggitt · 2022-10-14T13:50:44Z

staging/src/k8s.io/api/core/v1/types.go

+	// any container. Please remember that the group membership defined in the
+	// container image is kept. That means, if


is this part of the CRI spec or is it container-runtime implementation dependent?

@liggitt Thanks for a great question. However, as far as I know, I think this part is also still ambiguous in the CRI spec(v0.25.3): LinuxContainerSecurityContext.supplemental_groups, LinuxSandboxSecurityContext.supplemental_groups

message LinuxContainerSecurityContext { ... // List of groups applied to the first process run in the container, in // addition to the container's primary GID. repeated int64 supplemental_groups = 8; ... } message LinuxSandboxSecurityContext { ... // List of groups applied to the first process run in the sandbox, in // addition to the sandbox's primary GID. repeated int64 supplemental_groups = 5; ... }

At least, I confirmed containerd and cri-o implemented this behavior. here is reproduction code.

Then, would it be better to update the CRI spec too?? I believe https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/cri-api is what we update. Is this correct?

Then, would it be better to update the CRI spec too?? I believe https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/cri-api is what we update. Is this correct?

I updated cri-api at a913eaa

is this part of the CRI spec or is it container-runtime implementation dependent?

I will ask this at sig-node and come back again.

is this part of the CRI spec or is it container-runtime implementation dependent?

I will ask this at sig-node and come back again.

I think right now, it's technically implementation dependent, because there's no declaration in the cri spec nor a test in critest to validate it. It seems this PR is aiming to update it to be part of the CRI spec (would not hurt to add a follow-up to test this behavior in github.com/kubernetes-sigs/cri-tools)

@haircommander Thank you for your comment!

I would like to clarify the behavior in the spec because most popular cri (containerd and crio) does.

Then, I will open a PR to add critest in github.com/kubernetes-sigs/cri-tools.

I opened a PR in cri-tools repo that adds a test case to critest: kubernetes-sigs/cri-tools#1005

I also added an e2e test case in k/k to test the updated API description: 9e88694

staging/src/k8s.io/api/core/v1/types.go

everpeace · 2022-11-03T14:27:35Z

rebased to resolve conflict.

…luding cri-api) so that it explicitly describe group information defined in the container image will be kept. This also adds e2e test case of SupplementalGroups with pre-defined groups in the container image to make the behaivier clearer.

everpeace · 2022-11-06T01:07:41Z

Squashed all the previous commits into ac1d5fd.

mrunalp · 2022-11-07T22:12:13Z

/lgtm

liggitt · 2022-11-07T22:22:22Z

/approve
for API doc change

k8s-ci-robot · 2022-11-07T22:22:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everpeace, liggitt, mrunalp

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:

~~api/OWNERS~~ [liggitt]
~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/generated/openapi/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~staging/src/k8s.io/cri-api/pkg/OWNERS~~ [mrunalp]
~~test/e2e/node/OWNERS~~ [liggitt,mrunalp]

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

fedebongio · 2022-11-08T21:07:55Z

/triage accepted

k8s-ci-robot requested review from justinsb and liggitt October 13, 2022 14:41

everpeace mentioned this pull request Oct 13, 2022

Can bypass PodSecurityContext.SupplementalGroups by custom container image although PSP(or other policy engines) enforces the field #112879

Closed

everpeace changed the title ~~Improve the API description of 'PodSecurityContext.SupplementalGroups' to clarify its unfamiliar behavior~~ Improve the API description of PodSecurityContext.SupplementalGroups to clarify its unfamiliar behavior Oct 13, 2022

k8s-ci-robot added area/code-generation sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Oct 13, 2022

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 13, 2022

everpeace force-pushed the improve-supplemental-groups-description branch from aa41353 to a7b8c68 Compare October 14, 2022 00:12

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 14, 2022

everpeace force-pushed the improve-supplemental-groups-description branch from a7b8c68 to 97cfd4d Compare October 14, 2022 02:25

This comment was marked as off-topic.

Sign in to view

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 14, 2022

k8s-ci-robot assigned liggitt Oct 14, 2022

liggitt reviewed Oct 14, 2022

View reviewed changes

staging/src/k8s.io/api/core/v1/types.go Outdated Show resolved Hide resolved

liggitt reviewed Oct 14, 2022

View reviewed changes

staging/src/k8s.io/api/core/v1/types.go Outdated Show resolved Hide resolved

everpeace force-pushed the improve-supplemental-groups-description branch from 0bb5973 to f7b0788 Compare November 3, 2022 14:27

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 3, 2022

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Nov 3, 2022

This comment was marked as off-topic.

Sign in to view

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Nov 3, 2022

This comment was marked as off-topic.

Sign in to view

everpeace force-pushed the improve-supplemental-groups-description branch from f7b0788 to ac1d5fd Compare November 6, 2022 01:06

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Nov 6, 2022

This comment was marked as off-topic.

Sign in to view

mrunalp approved these changes Nov 7, 2022

View reviewed changes

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

k8s-ci-robot merged commit 68875cf into kubernetes:master Nov 8, 2022

k8s-ci-robot added this to the v1.26 milestone Nov 8, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 8, 2022

everpeace mentioned this pull request Jan 30, 2023

KEP-3169: Fine-grained SupplementalGroups control kubernetes/enhancements#3620

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the API description of `PodSecurityContext.SupplementalGroups` to clarify its unfamiliar behavior #113047

Improve the API description of `PodSecurityContext.SupplementalGroups` to clarify its unfamiliar behavior #113047

everpeace commented Oct 13, 2022 •

edited by liggitt

Loading

k8s-triage-robot commented Oct 13, 2022

leilajal commented Oct 13, 2022

This comment was marked as off-topic.

everpeace commented Oct 14, 2022

liggitt Oct 14, 2022

everpeace Oct 14, 2022 •

edited

Loading

everpeace Oct 17, 2022

everpeace Oct 17, 2022

haircommander Oct 17, 2022

everpeace Oct 17, 2022 •

edited

Loading

everpeace Oct 18, 2022

everpeace Oct 19, 2022 •

edited

Loading

everpeace commented Nov 3, 2022

This comment was marked as off-topic.

This comment was marked as off-topic.

everpeace commented Nov 6, 2022 •

edited

Loading

This comment was marked as off-topic.

mrunalp commented Nov 7, 2022

liggitt commented Nov 7, 2022

k8s-ci-robot commented Nov 7, 2022

fedebongio commented Nov 8, 2022

		// any container. Please remember that the group membership defined in the
		// container image is kept. That means, if

Improve the API description of PodSecurityContext.SupplementalGroups to clarify its unfamiliar behavior #113047

Improve the API description of PodSecurityContext.SupplementalGroups to clarify its unfamiliar behavior #113047

Conversation

everpeace commented Oct 13, 2022 • edited by liggitt Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-triage-robot commented Oct 13, 2022

leilajal commented Oct 13, 2022

This comment was marked as off-topic.

everpeace commented Oct 14, 2022

liggitt Oct 14, 2022

Choose a reason for hiding this comment

everpeace Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

everpeace Oct 17, 2022

Choose a reason for hiding this comment

everpeace Oct 17, 2022

Choose a reason for hiding this comment

haircommander Oct 17, 2022

Choose a reason for hiding this comment

everpeace Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

everpeace Oct 18, 2022

Choose a reason for hiding this comment

everpeace Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

everpeace commented Nov 3, 2022

This comment was marked as off-topic.

This comment was marked as off-topic.

everpeace commented Nov 6, 2022 • edited Loading

This comment was marked as off-topic.

mrunalp commented Nov 7, 2022

liggitt commented Nov 7, 2022

k8s-ci-robot commented Nov 7, 2022

fedebongio commented Nov 8, 2022

Improve the API description of `PodSecurityContext.SupplementalGroups` to clarify its unfamiliar behavior #113047

Improve the API description of `PodSecurityContext.SupplementalGroups` to clarify its unfamiliar behavior #113047

everpeace commented Oct 13, 2022 •

edited by liggitt

Loading

everpeace Oct 14, 2022 •

edited

Loading

everpeace Oct 17, 2022 •

edited

Loading

everpeace Oct 19, 2022 •

edited

Loading

everpeace commented Nov 6, 2022 •

edited

Loading