Restricted Pod E2E tests #109946

tallclair · 2022-05-10T17:02:47Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Add a new MixinRestrictedPodSecurity and corresponding MustMixinRestrictedPodSecurity utilities which set the required fields to pass the restricted pod security checks, and verify that the given pod does in fact meet the requirements. These are complementary to the existing GetRestricted{Pod,Container}SecurityContext functions, but in most cases require fewer changes.
Update the test/e2e/common/node/pods.go tests to set the restricted namespace label and use the new utilities.

Does this PR introduce a user-facing change?

NONE

/area test
/assign @liggitt @s-urbaniak

tallclair · 2022-05-10T18:19:12Z

Sorry, looks like I have a little more work to do on this.

liggitt · 2022-05-17T14:22:59Z

test/e2e/common/node/pods.go

@@ -235,12 +235,12 @@ var _ = SIGDescribe("Pods", func() {
 			Spec: v1.PodSpec{
 				Containers: []v1.Container{
 					{
-						Name:  "nginx",
-						Image: imageutils.GetE2EImage(imageutils.Nginx),
+						Name:  "pause",


does changing from nginx to pause change the lifecycle characteristics of the pod (w.r.t. handling shutdown signals)? same question applies in multiple places

Good question. Both the pause container and nginx handle SIGINT and SIGTERM as a fast shutdown, which I think is what containerd uses. nginx additionally handles SIGQUIT, but I don't see any references to SIGQUIT in k/k, containerd, or cri-o.

Maybe @SergeyKanzhelev or @endocrimes have additional context.

This test doesn't cover any corner cases of shutdown so this seems ok to me 👍

liggitt · 2022-05-17T14:26:54Z

test/e2e/framework/pod/utils.go

 // GetRestrictedPodSecurityContext returns a minimal restricted pod security context.
 func GetRestrictedPodSecurityContext() *v1.PodSecurityContext {
 	return &v1.PodSecurityContext{
 		RunAsNonRoot:   pointer.BoolPtr(true),
+		RunAsUser:      pointer.Int64(nonRootUser),


I'm still unsure about this... this isn't really a minimal restricted pod security context, right? shouldn't runAsNonRoot: true be sufficient?

Part of my goal with these methods was that most e2e test authors can just apply MixinRestrictedPodSecurity and not think about the restricted profile at all. If we don't set RunAsUser here (and in the mixin) then for all cases other than the pause pod the test author will need to manually set a RunAsUser. They'll probably pick something random that gets cargo-culted around.

What's your argument against this?

If we don't set RunAsUser here (and in the mixin) then for all cases other than the pause pod the test author will need to manually set a RunAsUser

Would they have to set it, or do you mean because the container image would have to be built to run with a non-root uid, which is uncommon?

What's your argument against this?

It would break images that expect to run with a particular non-zero uid at runtime, right?

I searched through our test image manifests, and the only ones that set a user are:

pause: user doesn't matter

nonroot: all the tests using this override the user anyway

windows containers: don't support Restricted for now (added TODO)

I'd prefer that the tests which require a specific user are explicit about it, rather than having all the tests that don't have an opinion set something arbitrary.

rather than having all the tests that don't have an opinion set something arbitrary

but… isn't this setting something arbitrary (1000) on all tests that don't have an opinion?

Is runAsUser generally needed as boilerplate, though? I thought runAsNonRoot was supposed to be sufficient.

If an e2e wanted to exercise runAsNonRoot functionality with an image that did not run as root by default and didn't want to set a runAsUser, that would mean they couldn't use GetRestrictedPodSecurityContext or MustMixinRestrictedPodSecurity to do so, because those wouldn't actually set the minimal securityContext that is compatible with the restricted PSS.

IMO, if a test is explicitly testing the runAsNonRoot functionality (with or without a runAsUser set), then the test should be explicitly writing & managing the SecurityContext, and not deferring that to a helper.

There should probably be 1-2 E2E tests explicitly checking runAsNonRoot behavior. I'd prefer to optimize these convenience methods for the common path, and force those couple of tests to be write the pod manually.

Ok, fair enough. Make sure the docs don't claim to produce a minimal valid restricted securityContext and doc the uid chosen

This reasoning makes sense to me 👍 - A comment summarizing this would go a long way for future readers of the framework code here tho.

Updated comments.

test/e2e/framework/pod/utils.go

endocrimes · 2022-05-20T23:32:46Z

/assign

endocrimes

/approve

looks good from a node pov, just needs the comment/doc updates

tallclair · 2022-05-24T23:13:31Z

Added comments about the default non-root user. Also squashed commits & rebased.

tallclair · 2022-05-24T23:13:50Z

/hold cancel

liggitt · 2022-05-25T01:21:30Z

/test pull-kubernetes-e2e-capz-conformance

liggitt · 2022-05-25T01:22:51Z

test/e2e/framework/pod/utils.go

 func GetRestrictedPodSecurityContext() *v1.PodSecurityContext {
 	return &v1.PodSecurityContext{
 		RunAsNonRoot:   pointer.BoolPtr(true),
+		RunAsUser:      pointer.Int64(DefaultNonRootUser),


what do windows nodes do if runAsUser is non-nil? do they ignore it or fail?

It is ignored by kubelet:

kubernetes/pkg/kubelet/kuberuntime/security_context_windows.go

Lines 46 to 49 in 68fc207

if effectiveSc.RunAsUser != nil {

klog.InfoS("Windows container does not support SecurityContext.RunAsUser, please use SecurityContext.WindowsOptions",

"pod", klog.KObj(pod), "containerName", container.Name)

}

With PodOS feature and OS field is set (in beta in 1.24) the adminsion controller would reject it:

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

Lines 3286 to 3307 in 976a940

// If the OS field is set to windows, following fields must be unset:

// - spec.hostPID

// - spec.hostIPC

// - spec.securityContext.seLinuxOptions

// - spec.securityContext.seccompProfile

// - spec.securityContext.fsGroup

// - spec.securityContext.fsGroupChangePolicy

// - spec.securityContext.sysctls

// - spec.shareProcessNamespace

// - spec.securityContext.runAsUser

// - spec.securityContext.runAsGroup

// - spec.securityContext.supplementalGroups

// - spec.containers[*].securityContext.seLinuxOptions

// - spec.containers[*].securityContext.seccompProfile

// - spec.containers[*].securityContext.capabilities

// - spec.containers[*].securityContext.readOnlyRootFilesystem

// - spec.containers[*].securityContext.privileged

// - spec.containers[*].securityContext.allowPrivilegeEscalation

// - spec.containers[*].securityContext.procMount

// - spec.containers[*].securityContext.runAsUser

// - spec.containers[*].securityContext.runAsGroup

// +optional

ok, there's a TODO to avoid setting RunAsUser in MustMixinRestrictedPodSecurity for explicitly windows pods as part of the PodOS work, I just wanted to make sure this wasn't going to fail in a windows kubelet for pods that didn't explicitly indicate their OS

So, this did break the tests because of #102849

https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/ci-kubernetes-e2e-capz-master-containerd-windows/1529769915945324544

May 26 10:47:49.396: INFO: At 2022-05-26 10:44:57 +0000 UTC - event for pod-update-d046cc17-42c7-493c-8c0c-fea0e9ac3164: {kubelet capz-conf-qwm24} FailedMount: (combined from similar events): MountVolume.SetUp failed for volume "kube-api-access-9l7hz" : chown c:\var\lib\kubelet\pods\0a6b5b55-fc8f-42b4-b285-19aedc311882\volumes\kubernetes.io~projected\kube-api-access-9l7hz\..2022_05_26_10_46_59.624505313\token: not supported by windows

this isn't setting RunAsUserName, but does the windows kubelet have similar problems with RunAsUser?

if so, that seems like a good reason to drop RunAsUser here

Yes the title is slightly misleading but it states in the issue: In addition if a Windows Pod is created with a Projected Volume and RunAsUser set, the Pod will be stuck at ContainerCreating:

ok. @tallclair, that's a good reason not to set RunAsUser here for pods that could run on Windows. @jsturtevant, do you mind opening a PR to drop the RunAsUser field?

linking here for anyone following the thread #110235

endocrimes · 2022-05-25T16:51:39Z

/triage accepted
/priority backlog

liggitt · 2022-05-25T19:12:56Z

/lgtm
/approve

k8s-ci-robot · 2022-05-25T19:13:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: endocrimes, liggitt, tallclair

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:

~~test/e2e/common/OWNERS~~ [endocrimes,liggitt,tallclair]
~~test/e2e/framework/OWNERS~~ [liggitt]

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

k8s-triage-robot · 2022-05-25T21:29:27Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot assigned liggitt May 10, 2022

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label May 10, 2022

k8s-ci-robot assigned s-urbaniak May 10, 2022

k8s-ci-robot requested review from odinuge and pohly May 10, 2022 17:03

tallclair changed the title ~~Ps e2e~~ Restricted Pod E2E tests May 10, 2022

tallclair marked this pull request as draft May 10, 2022 18:18

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 10, 2022

tallclair changed the title ~~Restricted Pod E2E tests~~ WIP: Restricted Pod E2E tests May 10, 2022

tallclair force-pushed the ps-e2e branch from 7edf41d to 559e04a Compare May 10, 2022 18:32

tallclair marked this pull request as ready for review May 10, 2022 21:45

k8s-ci-robot requested review from andrewsykim and wzshiming May 10, 2022 21:46

tallclair force-pushed the ps-e2e branch 3 times, most recently from 2b0b97f to 02c1c0d Compare May 12, 2022 17:10

tallclair changed the title ~~WIP: Restricted Pod E2E tests~~ Restricted Pod E2E tests May 12, 2022

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 May 12, 2022

liggitt reviewed May 17, 2022

View reviewed changes

tallclair mentioned this pull request May 17, 2022

PodSecurity: OS based updates to restricted standard #105919

Merged

k8s-ci-robot assigned endocrimes May 20, 2022

endocrimes reviewed May 22, 2022

View reviewed changes

endocrimes added this to Triage in SIG Node CI/Test Board May 22, 2022

tallclair added 2 commits May 24, 2022 16:10

Add MixinRestrictedPodSecurity e2e util

ccc69b1

Run common pod E2Es as restricted

702ab97

tallclair force-pushed the ps-e2e branch from c821497 to 702ab97 Compare May 24, 2022 23:10

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 24, 2022

liggitt reviewed May 25, 2022

View reviewed changes

endocrimes moved this from Triage to PRs Waiting on Author in SIG Node CI/Test Board May 25, 2022

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 25, 2022

k8s-ci-robot merged commit 60481c9 into kubernetes:master May 25, 2022

SIG Node CI/Test Board automation moved this from PRs Waiting on Author to Done May 25, 2022

SIG-Auth: PodSecurity automation moved this from In Review to Done (1.25) May 25, 2022

k8s-ci-robot added this to the v1.25 milestone May 25, 2022

jsturtevant mentioned this pull request May 26, 2022

test: RunAsUser causes pods to not start on Windows #110235

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restricted Pod E2E tests #109946

Restricted Pod E2E tests #109946

tallclair commented May 10, 2022

tallclair commented May 10, 2022

liggitt May 17, 2022

tallclair May 17, 2022

endocrimes May 22, 2022

liggitt May 17, 2022

tallclair May 17, 2022

liggitt May 17, 2022

tallclair May 17, 2022

liggitt May 17, 2022

liggitt May 20, 2022

tallclair May 20, 2022

liggitt May 20, 2022

endocrimes May 22, 2022

tallclair May 24, 2022

endocrimes commented May 20, 2022

endocrimes left a comment

tallclair commented May 24, 2022

tallclair commented May 24, 2022

liggitt commented May 25, 2022

liggitt May 25, 2022

jsturtevant May 25, 2022

liggitt May 25, 2022 •

edited

jsturtevant May 26, 2022

liggitt May 26, 2022

jsturtevant May 26, 2022

liggitt May 26, 2022

jsturtevant May 26, 2022

endocrimes commented May 25, 2022

liggitt commented May 25, 2022

k8s-ci-robot commented May 25, 2022

k8s-triage-robot commented May 25, 2022

	if effectiveSc.RunAsUser != nil {
	klog.InfoS("Windows container does not support SecurityContext.RunAsUser, please use SecurityContext.WindowsOptions",
	"pod", klog.KObj(pod), "containerName", container.Name)
	}

	// If the OS field is set to windows, following fields must be unset:
	// - spec.hostPID
	// - spec.hostIPC
	// - spec.securityContext.seLinuxOptions
	// - spec.securityContext.seccompProfile
	// - spec.securityContext.fsGroup
	// - spec.securityContext.fsGroupChangePolicy
	// - spec.securityContext.sysctls
	// - spec.shareProcessNamespace
	// - spec.securityContext.runAsUser
	// - spec.securityContext.runAsGroup
	// - spec.securityContext.supplementalGroups
	// - spec.containers[*].securityContext.seLinuxOptions
	// - spec.containers[*].securityContext.seccompProfile
	// - spec.containers[*].securityContext.capabilities
	// - spec.containers[*].securityContext.readOnlyRootFilesystem
	// - spec.containers[*].securityContext.privileged
	// - spec.containers[*].securityContext.allowPrivilegeEscalation
	// - spec.containers[*].securityContext.procMount
	// - spec.containers[*].securityContext.runAsUser
	// - spec.containers[*].securityContext.runAsGroup
	// +optional

Restricted Pod E2E tests #109946

Restricted Pod E2E tests #109946

Conversation

tallclair commented May 10, 2022

What type of PR is this?

What this PR does / why we need it:

Does this PR introduce a user-facing change?

tallclair commented May 10, 2022

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

endocrimes commented May 20, 2022

endocrimes left a comment

Choose a reason for hiding this comment

tallclair commented May 24, 2022

tallclair commented May 24, 2022

liggitt commented May 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt May 25, 2022 • edited

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

endocrimes commented May 25, 2022

liggitt commented May 25, 2022

k8s-ci-robot commented May 25, 2022

k8s-triage-robot commented May 25, 2022

liggitt May 25, 2022 •

edited