KEP-3619: Fine-grained SupplementalGroups control #1438

everpeace · 2024-05-30T14:04:36Z

For:

Fine-grained SupplementalGroups control kubernetes/enhancements#3619

Depends on:

k/k
- KEP-3619: Fine-grained SupplementalGroups control kubernetes/kubernetes#117842
- KEP-3619: Add NodeStatus.Features.SupplementalGroupsPolicy API and e2e kubernetes/kubernetes#125470
containerd (need to be merged to main and released)
- Fine-grained SupplementalGroups control containerd/containerd#9737
- Implement RuntimeStatus.features.supplemental_groups_policy from KEP-3619 containerd/containerd#10410
CRI-O (need to be merged to main):
- server/*: add fine-grained SupplementalGroups control for enhanced security cri-o/cri-o#8268
- KEP-3619: implement RuntimeStatus.features.supplemental_groups_policy cri-o/cri-o#8386
~~Make cri-tools independent from k/k #1437 (v1.31 introduces k8s.io/cri-client isolated from kubelet)~~

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implements validation tests for KEP-3619: Fine-grained SupplementalGroups control:

Fine-grained SupplementalGroups control kubernetes/enhancements#3619

, which skips itself when the container runtime does not support SupplementalGroupsPolicy detected via StatusResponse.features.supplemental_groups_policy.

The current SupplementalGroupsPolicy test status:

CRI	OS	Version	Test Stautus
containerd	Linux	master	PASSED
containerd	Linux	1.6	SKIPPED
containerd	Linux	1.7	SKIPPED
containerd	Windows	*	SKIPPED
CRI-O	Linux	master	PASSED
CRI-O	Windows	*	SKIPPED

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Support Fine-grained SupplementalGroups control (KEP-3619)

…Policy).

everpeace

@kwilczynski @mrunalp Could you kindly review this PR?? 🙇 Thanks, in advance.

kwilczynski · 2024-07-23T12:44:18Z

pkg/validate/security_context_linux.go

+						"Linux": PointTo(MatchFields(IgnoreExtras, Fields{
+							"Uid": Equal(imagePredefinedGroupUID),
+							"Gid": Equal(imagePredefinedGroupUID),
+							// we can not assume the order of gids


Would sorting the list make it a little more deterministic?

Sure, fixed in 695b675. Sorting removes gstruct usage, too👍.

kwilczynski · 2024-07-23T12:46:38Z

pkg/validate/security_context_linux.go

+							// SupplementalGroupsPolicy_Merge is default(0)
+							// SupplementalGroupsPolicy: runtimeapi.SupplementalGroupsPolicy_Merge,


Do you want to keep this comment here?

not really. fixed in c45cf82

everpeace

@kwilczynski I addressed your comments. PTAL.

saschagrunert

Thanks!

/hold
for @kwilczynski final thoughts.

kwilczynski · 2024-07-23T13:52:25Z

pkg/validate/security_context_linux.go

+				verifyLogContents(podConfig, logPath, expectedOutput, stdoutType)
+
+				By("verify groups for 'exec'-ed process of container")
+				command := []string{"id", "-G"}


Would the GroupIds() from os/user work here in lieu of fork-out to the id command? Nothing against doing this, just curious...

As far as I know GroupIds() always parse /etc/groups. So, I think GroupIds() wouldn't work.

Got it. Thank you!

kwilczynski · 2024-07-23T13:54:10Z

pkg/validate/security_context_linux.go

+				containerConfig := &runtimeapi.ContainerConfig{
+					Metadata: framework.BuildContainerMetadata(containerName, framework.DefaultAttempt),
+					Image:    &runtimeapi.ImageSpec{Image: testImagePreDefinedGroup},
+					Command:  []string{"sh", "-c", "id -G; while :; do sleep 1; done"},


Would this work in place of the loop?

Suggested change

Command: []string{"sh", "-c", "id -G; while :; do sleep 1; done"},

Command: []string{"sh", "-c", "id -G; sleep infinity"},

I realise this is a code that existed prior... hence I apologise for asking about it. However, perhaps we can make things bit better. 😄

never mind. fixed in c9e3de6

kwilczynski · 2024-07-23T14:31:31Z

/approve
/lgtm

kwilczynski · 2024-07-23T14:31:36Z

/unhold

k8s-ci-robot · 2024-07-23T14:31:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everpeace, kwilczynski, saschagrunert

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:

~~OWNERS~~ [saschagrunert]

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

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 30, 2024

k8s-ci-robot requested review from klueska and Random-Liu May 30, 2024 14:04

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 30, 2024

everpeace force-pushed the kep-3619-SupplementalGroupsPolicy branch 2 times, most recently from 5aac837 to 11b8584 Compare May 30, 2024 14:59

everpeace changed the title ~~KEP-3619: Fine-grained SupplementalGroups control (SupplementalGroupsPolicy)~~ KEP-3619: Fine-grained SupplementalGroups control May 30, 2024

This was referenced May 30, 2024

Fine-grained SupplementalGroups control kubernetes/enhancements#3619

Open

Fine-grained SupplementalGroups control containerd/containerd#9737

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 1, 2024

everpeace force-pushed the kep-3619-SupplementalGroupsPolicy branch from 11b8584 to 5994ce7 Compare June 3, 2024 09:24

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2024

everpeace mentioned this pull request Jun 3, 2024

Add support for Supplemental Groups Policy cri-o/cri-o#8230

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2024

everpeace force-pushed the kep-3619-SupplementalGroupsPolicy branch from 5994ce7 to 67169ed Compare June 14, 2024 00:37

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2024

everpeace changed the title ~~KEP-3619: Fine-grained SupplementalGroups control~~ [WIP] KEP-3619: Fine-grained SupplementalGroups control Jun 26, 2024

everpeace marked this pull request as ready for review June 26, 2024 20:43

k8s-ci-robot requested review from feiskyer and mrunalp June 26, 2024 20:43

everpeace marked this pull request as draft June 26, 2024 20:45

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2024

everpeace force-pushed the kep-3619-SupplementalGroupsPolicy branch from 67169ed to f55a3d5 Compare July 4, 2024 05:47

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 4, 2024

everpeace mentioned this pull request Jul 4, 2024

Implement RuntimeStatus.features.supplemental_groups_policy from KEP-3619 containerd/containerd#10410

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 4, 2024

k8s-ci-robot requested a review from derekwaynecarr July 23, 2024 04:17

everpeace changed the title ~~[WIP] KEP-3619: Fine-grained SupplementalGroups control~~ KEP-3619: Fine-grained SupplementalGroups control Jul 23, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 23, 2024

everpeace added 2 commits July 23, 2024 21:08

KEP-3619: update cri-api dependency

29b04c4

KEP-3619: Fine-grained SupplementalGroups control (SupplementalGroups…

64a7db2

…Policy).

everpeace force-pushed the kep-3619-SupplementalGroupsPolicy branch from 68415b7 to 64a7db2 Compare July 23, 2024 12:09

everpeace commented Jul 23, 2024

View reviewed changes

kwilczynski reviewed Jul 23, 2024

View reviewed changes

everpeace added 2 commits July 23, 2024 21:55

KEP-3619: delete comments

c45cf82

KEP-3619: sort SupplementalGroups field to make the assertion stable

695b675

everpeace force-pushed the kep-3619-SupplementalGroupsPolicy branch from c9678f4 to 695b675 Compare July 23, 2024 13:02

everpeace commented Jul 23, 2024

View reviewed changes

saschagrunert approved these changes Jul 23, 2024

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2024

k8s-ci-robot assigned saschagrunert Jul 23, 2024

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 23, 2024

kwilczynski reviewed Jul 23, 2024

View reviewed changes

KEP-3619: use sleep infinity instead of while loop

c9e3de6

kwilczynski reviewed Jul 23, 2024

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 23, 2024

k8s-ci-robot assigned kwilczynski Jul 23, 2024

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 23, 2024

k8s-ci-robot merged commit ee58080 into kubernetes-sigs:master Jul 23, 2024
23 checks passed

everpeace deleted the kep-3619-SupplementalGroupsPolicy branch July 24, 2024 00:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-3619: Fine-grained SupplementalGroups control #1438

KEP-3619: Fine-grained SupplementalGroups control #1438

everpeace commented May 30, 2024 •

edited

Loading

everpeace left a comment

kwilczynski Jul 23, 2024

everpeace Jul 23, 2024 •

edited

Loading

kwilczynski Jul 23, 2024

everpeace Jul 23, 2024 •

edited

Loading

everpeace left a comment

saschagrunert left a comment

kwilczynski Jul 23, 2024

everpeace Jul 23, 2024

kwilczynski Jul 23, 2024

kwilczynski Jul 23, 2024

kwilczynski Jul 23, 2024 •

edited

Loading

everpeace Jul 23, 2024

kwilczynski commented Jul 23, 2024

kwilczynski commented Jul 23, 2024

k8s-ci-robot commented Jul 23, 2024

		// SupplementalGroupsPolicy_Merge is default(0)
		// SupplementalGroupsPolicy: runtimeapi.SupplementalGroupsPolicy_Merge,

	Command: []string{"sh", "-c", "id -G; while :; do sleep 1; done"},
	Command: []string{"sh", "-c", "id -G; sleep infinity"},

KEP-3619: Fine-grained SupplementalGroups control #1438

KEP-3619: Fine-grained SupplementalGroups control #1438

Conversation

everpeace commented May 30, 2024 • edited 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?

everpeace left a comment

Choose a reason for hiding this comment

kwilczynski Jul 23, 2024

Choose a reason for hiding this comment

everpeace Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

kwilczynski Jul 23, 2024

Choose a reason for hiding this comment

everpeace Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

everpeace left a comment

Choose a reason for hiding this comment

saschagrunert left a comment

Choose a reason for hiding this comment

kwilczynski Jul 23, 2024

Choose a reason for hiding this comment

everpeace Jul 23, 2024

Choose a reason for hiding this comment

kwilczynski Jul 23, 2024

Choose a reason for hiding this comment

kwilczynski Jul 23, 2024

Choose a reason for hiding this comment

kwilczynski Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

everpeace Jul 23, 2024

Choose a reason for hiding this comment

kwilczynski commented Jul 23, 2024

kwilczynski commented Jul 23, 2024

k8s-ci-robot commented Jul 23, 2024

everpeace commented May 30, 2024 •

edited

Loading

everpeace Jul 23, 2024 •

edited

Loading

everpeace Jul 23, 2024 •

edited

Loading

kwilczynski Jul 23, 2024 •

edited

Loading