fix windows container root validate #92355

wawa0210 · 2020-06-21T12:19:18Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind bug
/kind feature

What this PR does / why we need it:

From https://github.com/kubernetes/enhancements/blob/master/keps/sig-windows/20190103-windows-node-support.md#v1container

Windows container does not have a root user,but kubelet will still check the user of the windows container when creating the pod.

The current inspection conditions are based on the RunAsNonRoot and RunAsUser of the Linux container, and the results checked based on these conditions are incorrect

Which issue(s) this PR fixes:

Fixes #91482 #91414

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Ignore root user check when windows pod starts

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

/sig windows
/assign @dchen1107

SergeyKanzhelev · 2020-06-23T08:04:20Z

pkg/kubelet/kuberuntime/kuberuntime_container.go

+	// https://github.com/kubernetes/enhancements/blob/master/keps/sig-windows/20190103-windows-node-support.md#v1container
+	// Windows does not have a root user, we cannot judge the root identity of the windows container by the way to judge the root of the linux container
+	// SecurityContext.RunAsUser, and SecurityContext.RunAsNonRoot These are Linux-specific attributes and cannot be used on windows containers
+	// Therefore, the judgment logic here does not apply to windows container


would it be helpful to report an error back that configuration may be incorrect? Not necessarily by failing this verification, but maybe with the log message?

Both windows container and linux will cover this piece of code, but it only makes sense to execute on linux. Therefore, I feel that there is no need to output some confusing logs on windows

why do you think it will be confusing? If these settings were set by user, it may be important to know that settings were ignored. Or the assumption here that those log messages will never be seen by the person who set these configuration settings?

The processing logic of verifyRunAsNonRoot is not suitable for windows containers. Many of the judgment fields are based on Linux containers, such as SecurityContext.RunAsUser, image.uid, SecurityContext.RunAsNonRoot. So I have circumvented this logic~ Maybe Windows container should be judged separately and used for user input check. Is this better? Do you think?

I think it would be cleaner to split it out and judge differently. Could use file suffix for the implementation as is done for applyPlatformSpecificContainerConfig in the windows and linux files. For windows this can be a noop and if later there are privileged containers we would be able to add logic cleanly.

SergeyKanzhelev · 2020-06-23T08:04:40Z

pkg/kubelet/kuberuntime/kuberuntime_container.go

+	// Windows does not have a root user, we cannot judge the root identity of the windows container by the way to judge the root of the linux container
+	// SecurityContext.RunAsUser, and SecurityContext.RunAsNonRoot These are Linux-specific attributes and cannot be used on windows containers
+	// Therefore, the judgment logic here does not apply to windows container
+	if goruntime.GOOS != "windows" {


would it be possible to add a test?

jsturtevant · 2020-06-24T16:17:35Z

/sig windows

SergeyKanzhelev · 2020-06-30T03:35:29Z

pkg/kubelet/kuberuntime/security_context_others.go

+// +build !windows
+
+/*
+Copyright 2016 The Kubernetes Authors.


nit

Suggested change

Copyright 2016 The Kubernetes Authors.

Copyright 2020 The Kubernetes Authors.

SergeyKanzhelev · 2020-06-30T03:35:41Z

pkg/kubelet/kuberuntime/security_context_windows.go

+// +build windows
+
+/*
+Copyright 2016 The Kubernetes Authors.


Suggested change

Copyright 2016 The Kubernetes Authors.

Copyright 2020 The Kubernetes Authors.

SergeyKanzhelev · 2020-06-30T03:43:51Z

pkg/kubelet/kuberuntime/security_context_others.go

@@ -0,0 +1,51 @@
+// +build !windows


same in security_context_test.go?

I didn't get what you meant.

the file security_context_test.go calls into the method you modified. If you compile different version on windows - running these tests on windows would fail. So I suggest to add the same build !windows to the file containing tests

Also worth renaming the test file with the same pattern as the product code. With _other or _non_windows suffix.

SergeyKanzhelev · 2020-06-30T03:46:00Z

pkg/kubelet/kuberuntime/security_context_others.go

@@ -0,0 +1,51 @@
+// +build !windows
+
+/*


the pattern in file naming is security_context_windows.go and security_context.go In couple places I saw _unix or _linux suffices. I'd vote for consistency and drop _other

Here I have a very uncertain thing is that other platforms besides Linux and Windows support this feature, so I chose to use others~~

There are many examples of this in kubernetes,such as
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/apis/config/validation/validation_others.go

thank you for pointing out. Somehow all examples I saw was not using this pattern. I guess it's ok. If not previous examples my suggestion would be security_context_non_windows.go. It may be the most descriptive name

About this, I went to look at some of the existing designs of kubernetes.

If the platform support is very clear, it is generally *_linux.go, *_windows.go, *_unspport.go.

If it is possible to distinguish between windows and others, it is generally *_others.go, *_windows.go.

I think it’s better to follow the existing naming rules, otherwise it will confuse people , what do you think?

sure, lets keep it. It looks consistent with the pattern. Thank you for looking into it

SergeyKanzhelev

I still think it may be valuable to log warning if unsupported setting was set for windows. But this is an additional feature to log this warning and PR is ok.

SergeyKanzhelev · 2020-07-24T17:09:26Z

/retest
/lgtm

SergeyKanzhelev · 2020-08-18T21:49:13Z

/test pull-kubernetes-e2e-kind-ipv6

liggitt · 2020-08-26T15:57:28Z

this will be moved back into the milestone as part of the phased reopen for 1.20 merges

immuzz · 2020-09-10T16:51:01Z

SIG Windows Backlog Meeting

Next Step: Need 1.20 Tag and merging. @marosset to work on that

wawa0210 · 2020-09-11T00:10:45Z

@jsturtevant @michmike @SergeyKanzhelev
Can you take the time to help review this pr again?

SergeyKanzhelev · 2020-09-11T06:05:56Z

/lgtm

dashpole

Should setting WindowsOptions on linux containers also fail more loudly?

dashpole · 2020-09-11T16:58:41Z

pkg/kubelet/kuberuntime/security_context_windows.go

+		klog.Warningf("Windows container does not support SecurityContext.RunAsUser, please use SecurityContext.WindowsOptions")
+	}
+	if effectiveSc.SELinuxOptions != nil {
+		klog.Warningf("Windows container does not support SecurityContext.SELinuxOptions, please use SecurityContext.WindowsOptions")


This is a big deal, right? Someone thinks they are running with security options that we aren't applying. Should this fail more loudly?

If i'm interpreting this correctly, I would suggest emiting an event, or even returning an error here. Same comment for other unsupported options.

I think the argumentation here is that these settings were likely set by some plugin. See #93475 (comment)

This is why warning was good enough from my perspective, but I'd like somebody from windows sig to confirm

Maybe we should add a check to

kubernetes/pkg/apis/core/validation/validation.go

Line 3769 in 3606591

func ValidatePodSecurityContext(securityContext *core.PodSecurityContext, spec *core.PodSpec, specPath, fldPath *field.Path) field.ErrorList {

that verifies only one of SecurityContext.SELinuxOptions or SecurityContext.WindowsOptions are set.

From my perspective we should encourage plugin/automation authors to use node selectors if they are going to blanket apply security policy to pods rather than working around that assumption in the kubelet.
@jsturtevant thoughts?

Because these do not cause cri errors, it is just that the features of linux container on windows container cannot take effect, and vice versa. If kubelet fails more loudly, it will affect existing applications (adding wrong WindowsOptions or LinuxOptions). So I think the warning level log may be a way to make sense (may need to be more detailed)

@marosset

I think this is a very good idea. When apiserver pre-verification, if the user explicitly sets SecurityContext.SELinuxOptions or SecurityContext.WindowsOptions for verification, if the discussion passes, I can try to open another A pr.
But kubelet is really responsible for calling cri to create pod logic. Should it also have its own necessary verification logic?

Maybe we should add a check to

kubernetes/pkg/apis/core/validation/validation.go

Line 3769 in 3606591

func ValidatePodSecurityContext(securityContext *core.PodSecurityContext, spec *core.PodSpec, specPath, fldPath *field.Path) field.ErrorList {

that verifies only one of SecurityContext.SELinuxOptions or SecurityContext.WindowsOptions are set.

Tightening API validation is not generally possible (see #64841). If that combination of settings is never legitimate (is a multi-OS image possible?), we could add that to the list of things we want to warn about (#94626)

Thanks @liggitt, that addresses my concern.

This also addresses my concerns as well and I agree a warming would be more appropriate here.

dashpole · 2020-09-14T16:04:27Z

/approve

k8s-ci-robot · 2020-09-14T16:05:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, wawa0210

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:

~~pkg/kubelet/OWNERS~~ [dashpole]

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

marosset · 2020-09-14T16:37:38Z

/lgtm

marosset · 2020-09-14T16:37:51Z

/milestone v1.20

k8s-ci-robot requested review from dchen1107 and vishh June 21, 2020 12:20

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 21, 2020

SergeyKanzhelev reviewed Jun 23, 2020

View reviewed changes

k8s-ci-robot added the sig/windows Categorizes an issue or PR as relevant to SIG Windows. label Jun 24, 2020

wawa0210 force-pushed the fix-91482 branch from 6d4d71d to d5a99ad Compare June 25, 2020 16:42

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 25, 2020

wawa0210 force-pushed the fix-91482 branch 2 times, most recently from d5574c8 to 5acdd3c Compare June 26, 2020 03:13

marosset added this to In Progress (v1.19) in SIG-Windows Jun 26, 2020

marosset moved this from In Progress (v1.19) to In Review (v1.19) in SIG-Windows Jun 26, 2020

SergeyKanzhelev reviewed Jun 30, 2020

View reviewed changes

wawa0210 force-pushed the fix-91482 branch from 5acdd3c to 6463882 Compare June 30, 2020 03:44

SergeyKanzhelev reviewed Jun 30, 2020

View reviewed changes

wawa0210 force-pushed the fix-91482 branch 2 times, most recently from 425a0b9 to e15325e Compare July 1, 2020 03:15

SergeyKanzhelev approved these changes Jul 1, 2020

View reviewed changes

fix windows container root validate

ccde63b

wawa0210 force-pushed the fix-91482 branch from 190560d to ccde63b Compare July 24, 2020 12:01

k8s-ci-robot assigned SergeyKanzhelev Jul 24, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 24, 2020

jsturtevant mentioned this pull request Jul 28, 2020

Strip unnecessary security contexts on Windows #93475

Merged

jsturtevant mentioned this pull request Aug 5, 2020

REQUEST: New membership for jsturtevant kubernetes/org#2083

Closed

6 tasks

marosset moved this from In Review (v1.19) to In Review (v1.20) in SIG-Windows Aug 13, 2020

liggitt removed this from the v1.20 milestone Aug 26, 2020

dashpole reviewed Sep 11, 2020

View reviewed changes

liggitt mentioned this pull request Sep 14, 2020

Add field-level warnings for deprecated / known bad values #94626

Open

20 tasks

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2020

k8s-ci-robot assigned marosset Sep 14, 2020

k8s-ci-robot added this to the v1.20 milestone Sep 14, 2020

k8s-ci-robot merged commit 73dda0a into kubernetes:master Sep 14, 2020

SIG-Windows automation moved this from In Review (v1.20) to Done (v1.20) Sep 14, 2020

jsturtevant mentioned this pull request Sep 14, 2020

RunAsUserName Policy Constraint for windows pods open-policy-agent/gatekeeper-library#4

Closed

wawa0210 deleted the fix-91482 branch September 15, 2020 03:40

github-actions bot mentioned this pull request Sep 15, 2020

Week Ending September 13, 2020 dev-obs/actus#223

Open

jsturtevant mentioned this pull request Sep 15, 2020

ingore apparmor on windows #93220

Merged

jsturtevant mentioned this pull request Apr 22, 2021

PSP Replacement KEP kubernetes/enhancements#2582

Merged

18 tasks

	Copyright 2016 The Kubernetes Authors.
	Copyright 2020 The Kubernetes Authors.

fix windows container root validate #92355

fix windows container root validate #92355

Conversation

wawa0210 commented Jun 21, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsturtevant commented Jun 24, 2020

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

wawa0210 Jun 30, 2020 • 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

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

SergeyKanzhelev commented Jul 24, 2020

SergeyKanzhelev commented Aug 18, 2020

liggitt commented Aug 26, 2020

immuzz commented Sep 10, 2020

wawa0210 commented Sep 11, 2020

SergeyKanzhelev commented Sep 11, 2020

dashpole left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marosset Sep 11, 2020 • 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

Choose a reason for hiding this comment

marosset Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

dashpole commented Sep 14, 2020

k8s-ci-robot commented Sep 14, 2020

marosset commented Sep 14, 2020

marosset commented Sep 14, 2020

wawa0210 commented Jun 21, 2020 •

edited

Loading

wawa0210 Jun 30, 2020 •

edited

Loading

marosset Sep 11, 2020 •

edited

Loading

marosset Sep 14, 2020 •

edited

Loading