PodSecurity: OS based updates to restricted standard #105919

ravisantoshgudimetla · 2021-10-26T20:06:50Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR ensures that the restricted standard is updated based on the newly added OS field to the pod spec.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

As of v1.25, the PodSecurity `restricted` level no longer requires pods that set .spec.os.name="windows" to also set Linux-specific securityContext fields. If a 1.25+ cluster has unsupported [out-of-skew](https://kubernetes.io/releases/version-skew-policy/#kubelet) nodes prior to v1.23 and wants to ensure namespaces enforcing the `restricted` policy continue to require Linux-specific securityContext fields on all pods, ensure a version of the `restricted` prior to v1.25 is selected by labeling the namespace (for example, `pod-security.kubernetes.io/enforce-version: v1.24`)

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

[KEP]:  https://github.com/kubernetes/enhancements/pull/3303
[Other doc]: https://kubernetes.io/docs/concepts/security/pod-security-standards/

liggitt · 2021-10-26T20:10:14Z

staging/src/k8s.io/pod-security-admission/policy/check_seccompProfile_restricted.go

@@ -66,6 +66,11 @@ func seccompProfileRestricted_1_19(podMetadata *metav1.ObjectMeta, podSpec *core

 	podSeccompSet := false

+	// Pod API validation would have failed if podOS == Windows and if secCompProfile has been set.


suggest putting this at the top of the function

staging/src/k8s.io/pod-security-admission/policy/check_capabilities_restricted.go

liggitt · 2021-10-26T20:17:59Z

The podsecurity commit looks about like what I expected

One interesting thing is that we don't want to relax PodSecurity until the oldest supported kubelet honors the OS field (and will refuse to run a pod with a mismatched OS field). Otherwise, 1.24 (assuming pod OS is beta) you could submit a pod with a linux container image to a restricted namespace without setting seccompProfile or dropping capabilities by setting the pod OS field to "windows", but it could get scheduled to a 1.22 or 1.23 linux node which would happily run the pod.

k8s-triage-robot · 2021-10-26T20:49:34Z

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.

fedebongio · 2021-10-28T20:05:27Z

This seems to me mostly sig Auth. Removing API Machinery for the moment.
/cc @deads2k
/triage accepted

liggitt · 2021-10-28T20:51:24Z

also exclude the allowPrivilegeEscalation restricted check for windows

the pod security integration tests should be extended to exercise the minimal valid os-specific pods (linux and windows) ... that would have caught the conflicting requirement that allowPrivilegeEscalation be specified and that allowPrivilegeEscalation is disallowed in validation for windows pods

enj · 2021-11-08T17:14:35Z

@ravisantoshgudimetla code freeze for v1.23 is in a week. Please address Jordan's comments ASAP if you are targeting this for the next release.

tallclair · 2022-07-13T22:21:08Z

staging/src/k8s.io/pod-security-admission/policy/check_allowPrivilegeEscalation.go

@@ -57,6 +57,12 @@ func CheckAllowPrivilegeEscalation() Check {
 }

 func allowPrivilegeEscalation_1_8(podMetadata *metav1.ObjectMeta, podSpec *corev1.PodSpec) CheckResult {
+	// Pod API validation would have failed if podOS == Windows and if privilegeEscalation has been set.
+	// We can admit the Windows pod even if privilegeEscalation has not been set.
+	if podSpec.OS != nil && podSpec.OS.Name == corev1.Windows {


Although nodes that don't understand pod os are technically out of the version skew window in my experience there is often a long tail of clusters running old nodes. I'm wondering if we should make these relaxed constraints a version bump. I.e. only the v1.25 version of the AllowPrivilegeEscalation check would have this? Same for the capabilities & seccomp checks.

@liggitt WDYT?

Are you thinking that clusters in that state would have pinned their Pod Security policies to 1.24 and earlier versions? If not, I don't really see what the version bump would accomplish

Are you concerned that pod security admission can run as a webhook?

Yeah, then this could have a release note like

If you have nodes older than v1.23, namespaces using the restricted pod security level should version-pin to a version before v1.25

Fair enough - I updated the release note. PTAL

If we're going with that release note, then the policies also need to be updated to have a different implementation. Basically:

Add a allowPrivilegeEscalation_1_25(...) function, with this check (extract the common logic to a helper)

Update CheckAllowPrivilegeEscalation() to add a new version with the new function

Same for Capabilities & seccomp checks.

staging/src/k8s.io/pod-security-admission/test/testdata/README.md

staging/src/k8s.io/pod-security-admission/test/testdata/baseline/v1.0/pass/base_linux.yaml

liggitt · 2022-07-13T14:23:32Z

test/integration/auth/podsecurity_test.go

@@ -55,6 +55,8 @@ func TestPodSecurity(t *testing.T) {
 	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ProcMountType, true)()
 	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.WindowsHostProcessContainers, true)()
 	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AppArmor, true)()
+	// Ensure IdentifyPodOS field in enabled.
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IdentifyPodOS, true)()


I guess we'll immediately remove these when the feature gate gets promoted to GA?

Yes, I'll open a PR once this merges.

liggitt · 2022-07-13T22:23:28Z

staging/src/k8s.io/pod-security-admission/policy/check_allowPrivilegeEscalation.go

@@ -57,6 +57,12 @@ func CheckAllowPrivilegeEscalation() Check {
 }

 func allowPrivilegeEscalation_1_8(podMetadata *metav1.ObjectMeta, podSpec *corev1.PodSpec) CheckResult {
+	// Pod API validation would have failed if podOS == Windows and if privilegeEscalation has been set.
+	// We can admit the Windows pod even if privilegeEscalation has not been set.
+	if podSpec.OS != nil && podSpec.OS.Name == corev1.Windows {


Are you thinking that clusters in that state would have pinned their Pod Security policies to 1.24 and earlier versions? If not, I don't really see what the version bump would accomplish

ravisantoshgudimetla · 2022-07-18T12:22:14Z

/retest

tallclair

lgtm from a policy perspective, but I'll let @liggitt have the final say.

staging/src/k8s.io/pod-security-admission/policy/check_allowPrivilegeEscalation.go

staging/src/k8s.io/pod-security-admission/policy/check_allowPrivilegeEscalation_test.go

staging/src/k8s.io/pod-security-admission/test/fixtures.go

staging/src/k8s.io/pod-security-admission/test/fixtures_test.go

staging/src/k8s.io/pod-security-admission/test/run.go

staging/src/k8s.io/pod-security-admission/test/fixtures_test.go

liggitt · 2022-07-22T15:06:33Z

An interesting consequence of making the OS exemption only apply to PodSecurity 1.25+ versions is that lower versions will require securityContext fields to be set even with the OS is explicitly windows, but API validation will forbid those securityContext fields to be set when the OS is explicitly windows.

That means that setting OS=windows on a pod will not be possible with a restricted PodSecurity version pinned below 1.25. I think I'm ok with that, especially since a notable reason to pin that way is if you have older nodes who don't understand pod OS and wouldn't enforce it.

staging/src/k8s.io/pod-security-admission/test/fixtures.go

liggitt · 2022-07-25T14:38:42Z

staging/src/k8s.io/pod-security-admission/test/fixtures.go

+	minimalValidLinuxPods[api.LevelRestricted][api.MajorMinorVersion(1, 22)] = addLinux(restricted_1_22)
+	minimalValidWindowsPods[api.LevelRestricted][api.MajorMinorVersion(1, 22)] = addWindows(restricted_1_22)
+
+	// 1.25+: OS specific changes


I'd suggest capturing addWindows(restricted_1_0) in a variable with a comment, and so we have it as a starting point if any future versions add restricted requirements for windows pods

// none of the restricted requirements added between 1.0 and 1.25 apply to pods that are explicitly Windows restricted_1_25_windows := addWindows(restricted_1_0) ... minimalValidWindowsPods[api.LevelRestricted][api.MajorMinorVersion(1, 25)] = restricted_1_25_windows

this is still outstanding

staging/src/k8s.io/pod-security-admission/test/fixtures.go

staging/src/k8s.io/pod-security-admission/test/run.go

k8s-ci-robot · 2022-07-26T23:57:36Z

@ravisantoshgudimetla: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-gce-alpha-features	60f2b2382734eb8c355f9389e55da15fe0d4ef34	link	false	`/test pull-kubernetes-e2e-gce-alpha-features`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

ravisantoshgudimetla · 2022-07-27T13:30:11Z

/retest

liggitt · 2022-07-27T14:56:32Z

/lgtm
/approve

k8s-ci-robot · 2022-07-27T14:57:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, ravisantoshgudimetla

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:

~~staging/src/k8s.io/pod-security-admission/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-ci-robot requested review from caesarxuchao and dchen1107 October 26, 2021 20:07

liggitt reviewed Oct 26, 2021

View reviewed changes

ravisantoshgudimetla force-pushed the ps-restricted-updates branch from b5b5e92 to 67525f0 Compare October 26, 2021 20:17

k8s-ci-robot requested a review from deads2k October 28, 2021 20:05

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 28, 2021

ravisantoshgudimetla mentioned this pull request Nov 8, 2021

Pod os field kubelet #105292

Merged

liggitt self-assigned this Nov 9, 2021

ravisantoshgudimetla force-pushed the ps-restricted-updates branch from 67525f0 to 60f2b23 Compare November 10, 2021 14:45

k8s-ci-robot added the area/test label Nov 10, 2021

tallclair reviewed Jul 13, 2022

View reviewed changes

liggitt reviewed Jul 13, 2022

View reviewed changes

ravisantoshgudimetla force-pushed the ps-restricted-updates branch 6 times, most recently from a98aeaf to a6f3258 Compare July 15, 2022 23:33

tallclair reviewed Jul 18, 2022

View reviewed changes

staging/src/k8s.io/pod-security-admission/policy/check_allowPrivilegeEscalation.go Outdated Show resolved Hide resolved

liggitt reviewed Jul 22, 2022

View reviewed changes

ravisantoshgudimetla force-pushed the ps-restricted-updates branch 2 times, most recently from 89556b2 to 3e371d4 Compare July 25, 2022 13:02

liggitt reviewed Jul 25, 2022

View reviewed changes

[podsecurity] OS based updates to restricted standard

b3a2e6a

ravisantoshgudimetla force-pushed the ps-restricted-updates branch 2 times, most recently from f49228f to 0eff212 Compare July 26, 2022 14:12

liggitt reviewed Jul 26, 2022

View reviewed changes

staging/src/k8s.io/pod-security-admission/test/run.go Outdated Show resolved Hide resolved

liggitt reviewed Jul 26, 2022

View reviewed changes

staging/src/k8s.io/pod-security-admission/test/run.go Show resolved Hide resolved

Update test fixtures

96950f5

ravisantoshgudimetla force-pushed the ps-restricted-updates branch from 0eff212 to 96950f5 Compare July 26, 2022 23:47

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

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

k8s-ci-robot merged commit ec905a4 into kubernetes:master Jul 27, 2022

tallclair mentioned this pull request Aug 1, 2022

Promote PodOS field to GA kubernetes/website#35590

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PodSecurity: OS based updates to restricted standard #105919

PodSecurity: OS based updates to restricted standard #105919

ravisantoshgudimetla commented Oct 26, 2021 •

edited by liggitt

Loading

liggitt Oct 26, 2021

liggitt commented Oct 26, 2021

k8s-triage-robot commented Oct 26, 2021

fedebongio commented Oct 28, 2021

liggitt commented Oct 28, 2021

enj commented Nov 8, 2021

tallclair Jul 13, 2022

liggitt Jul 13, 2022

ravisantoshgudimetla Jul 14, 2022

tallclair Jul 14, 2022

ravisantoshgudimetla Jul 14, 2022

tallclair Jul 14, 2022

liggitt Jul 13, 2022

ravisantoshgudimetla Jul 14, 2022

liggitt Jul 13, 2022

ravisantoshgudimetla commented Jul 18, 2022

tallclair left a comment

liggitt commented Jul 22, 2022

liggitt Jul 25, 2022

liggitt Jul 26, 2022

k8s-ci-robot commented Jul 26, 2022 •

edited

Loading

ravisantoshgudimetla commented Jul 27, 2022

liggitt commented Jul 27, 2022

k8s-ci-robot commented Jul 27, 2022

		@@ -66,6 +66,11 @@ func seccompProfileRestricted_1_19(podMetadata metav1.ObjectMeta, podSpec core

		podSeccompSet := false

		// Pod API validation would have failed if podOS == Windows and if secCompProfile has been set.

PodSecurity: OS based updates to restricted standard #105919

PodSecurity: OS based updates to restricted standard #105919

Conversation

ravisantoshgudimetla commented Oct 26, 2021 • 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.:

Choose a reason for hiding this comment

liggitt commented Oct 26, 2021

k8s-triage-robot commented Oct 26, 2021

fedebongio commented Oct 28, 2021

liggitt commented Oct 28, 2021

enj commented Nov 8, 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

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

ravisantoshgudimetla commented Jul 18, 2022

tallclair left a comment

Choose a reason for hiding this comment

liggitt commented Jul 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 26, 2022 • edited Loading

ravisantoshgudimetla commented Jul 27, 2022

liggitt commented Jul 27, 2022

k8s-ci-robot commented Jul 27, 2022

ravisantoshgudimetla commented Oct 26, 2021 •

edited by liggitt

Loading

k8s-ci-robot commented Jul 26, 2022 •

edited

Loading