From 5fc06591a2c9a75a868e98d167238dec96ea5c63 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Wed, 7 Jul 2021 23:02:18 -0400 Subject: [PATCH] PodSecurity: runAsNonRoot: cleanup Improve message and details Add unit tests Consolidate integration test fixtures --- .../policy/check_runAsNonRoot.go | 72 +++++++++----- .../policy/check_runAsNonRoot_test.go | 99 +++++++++++++++++++ .../test/fixtures_runAsNonRoot.go | 12 --- 3 files changed, 147 insertions(+), 36 deletions(-) create mode 100644 staging/src/k8s.io/pod-security-admission/policy/check_runAsNonRoot_test.go diff --git a/staging/src/k8s.io/pod-security-admission/policy/check_runAsNonRoot.go b/staging/src/k8s.io/pod-security-admission/policy/check_runAsNonRoot.go index cfffe2cc237..5b9cb77771a 100644 --- a/staging/src/k8s.io/pod-security-admission/policy/check_runAsNonRoot.go +++ b/staging/src/k8s.io/pod-security-admission/policy/check_runAsNonRoot.go @@ -17,6 +17,7 @@ limitations under the License. package policy import ( + "fmt" "strings" corev1 "k8s.io/api/core/v1" @@ -34,7 +35,9 @@ spec.securityContext.runAsNonRoot spec.containers[*].securityContext.runAsNonRoot spec.initContainers[*].securityContext.runAsNonRoot -**Allowed Values:** true +**Allowed Values:** +true +undefined/null at container-level if pod-level is set to true */ func init() { @@ -57,47 +60,68 @@ func CheckRunAsNonRoot() Check { } func runAsNonRoot_1_0(podMetadata *metav1.ObjectMeta, podSpec *corev1.PodSpec) CheckResult { - var forbiddenPaths []string + // things that explicitly set runAsNonRoot=false + var badSetters []string - // TODO: how to check ephemeral containers - - containerCount := 0 - containerRunAsNonRootCount := 0 podRunAsNonRoot := false - - visitContainersWithPath(podSpec, field.NewPath("spec"), func(container *corev1.Container, path *field.Path) { - containerCount++ - if container.SecurityContext != nil && container.SecurityContext.RunAsNonRoot != nil { - if !*container.SecurityContext.RunAsNonRoot { - forbiddenPaths = append(forbiddenPaths, path.Child("securityContext", "runAsNonRoot").String()) - } else { - containerRunAsNonRootCount++ - } - } - }) - if podSpec.SecurityContext != nil && podSpec.SecurityContext.RunAsNonRoot != nil { if !*podSpec.SecurityContext.RunAsNonRoot { - forbiddenPaths = append(forbiddenPaths, field.NewPath("spec").Child("securityContext", "runAsNonRoot").String()) + badSetters = append(badSetters, "pod") } else { podRunAsNonRoot = true } } + // containers that explicitly set runAsNonRoot=false + var explicitlyBadContainers []string + // containers that didn't set runAsNonRoot and aren't caught by a pod-level runAsNonRoot=true + var implicitlyBadContainers []string + + visitContainersWithPath(podSpec, field.NewPath("spec"), func(container *corev1.Container, path *field.Path) { + if container.SecurityContext != nil && container.SecurityContext.RunAsNonRoot != nil { + // container explicitly set runAsNonRoot + if !*container.SecurityContext.RunAsNonRoot { + // container explicitly set runAsNonRoot to a bad value + explicitlyBadContainers = append(explicitlyBadContainers, container.Name) + } + } else { + // container did not explicitly set runAsNonRoot + if !podRunAsNonRoot { + // no pod-level runAsNonRoot=true, so this container implicitly has a bad value + implicitlyBadContainers = append(implicitlyBadContainers, container.Name) + } + } + }) + + if len(explicitlyBadContainers) > 0 { + badSetters = append( + badSetters, + fmt.Sprintf( + "%s %s", + pluralize("container", "containers", len(explicitlyBadContainers)), + joinQuote(explicitlyBadContainers), + ), + ) + } // pod or containers explicitly set runAsNonRoot=false - if len(forbiddenPaths) > 0 { + if len(badSetters) > 0 { return CheckResult{ Allowed: false, - ForbiddenReason: "runAsNonRoot != false", - ForbiddenDetail: strings.Join(forbiddenPaths, ", "), + ForbiddenReason: "runAsNonRoot != true", + ForbiddenDetail: fmt.Sprintf("%s must not set securityContext.runAsNonRoot=false", strings.Join(badSetters, " and ")), } } // pod didn't set runAsNonRoot and not all containers opted into runAsNonRoot - if podRunAsNonRoot == false && containerCount > containerRunAsNonRootCount { + if len(implicitlyBadContainers) > 0 { return CheckResult{ Allowed: false, - ForbiddenReason: "runAsNonRoot != false", + ForbiddenReason: "runAsNonRoot != true", + ForbiddenDetail: fmt.Sprintf( + "pod or %s %s must set securityContext.runAsNonRoot=true", + pluralize("container", "containers", len(implicitlyBadContainers)), + joinQuote(implicitlyBadContainers), + ), } } diff --git a/staging/src/k8s.io/pod-security-admission/policy/check_runAsNonRoot_test.go b/staging/src/k8s.io/pod-security-admission/policy/check_runAsNonRoot_test.go new file mode 100644 index 00000000000..bda7f8db2f3 --- /dev/null +++ b/staging/src/k8s.io/pod-security-admission/policy/check_runAsNonRoot_test.go @@ -0,0 +1,99 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package policy + +import ( + "testing" + + corev1 "k8s.io/api/core/v1" + utilpointer "k8s.io/utils/pointer" +) + +func TestRunAsNonRoot(t *testing.T) { + tests := []struct { + name string + pod *corev1.Pod + expectReason string + expectDetail string + }{ + { + name: "no explicit runAsNonRoot", + pod: &corev1.Pod{Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "a"}, + }, + }}, + expectReason: `runAsNonRoot != true`, + expectDetail: `pod or container "a" must set securityContext.runAsNonRoot=true`, + }, + { + name: "pod runAsNonRoot=false", + pod: &corev1.Pod{Spec: corev1.PodSpec{ + SecurityContext: &corev1.PodSecurityContext{RunAsNonRoot: utilpointer.Bool(false)}, + Containers: []corev1.Container{ + {Name: "a", SecurityContext: nil}, + }, + }}, + expectReason: `runAsNonRoot != true`, + expectDetail: `pod must not set securityContext.runAsNonRoot=false`, + }, + { + name: "containers runAsNonRoot=false", + pod: &corev1.Pod{Spec: corev1.PodSpec{ + SecurityContext: &corev1.PodSecurityContext{RunAsNonRoot: utilpointer.Bool(true)}, + Containers: []corev1.Container{ + {Name: "a", SecurityContext: nil}, + {Name: "b", SecurityContext: &corev1.SecurityContext{}}, + {Name: "c", SecurityContext: &corev1.SecurityContext{RunAsNonRoot: utilpointer.Bool(false)}}, + {Name: "d", SecurityContext: &corev1.SecurityContext{RunAsNonRoot: utilpointer.Bool(false)}}, + {Name: "e", SecurityContext: &corev1.SecurityContext{RunAsNonRoot: utilpointer.Bool(true)}}, + {Name: "f", SecurityContext: &corev1.SecurityContext{RunAsNonRoot: utilpointer.Bool(true)}}, + }, + }}, + expectReason: `runAsNonRoot != true`, + expectDetail: `containers "c", "d" must not set securityContext.runAsNonRoot=false`, + }, + { + name: "pod nil, container fallthrough", + pod: &corev1.Pod{Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "a", SecurityContext: nil}, + {Name: "b", SecurityContext: &corev1.SecurityContext{}}, + {Name: "d", SecurityContext: &corev1.SecurityContext{RunAsNonRoot: utilpointer.Bool(true)}}, + {Name: "e", SecurityContext: &corev1.SecurityContext{RunAsNonRoot: utilpointer.Bool(true)}}, + }, + }}, + expectReason: `runAsNonRoot != true`, + expectDetail: `pod or containers "a", "b" must set securityContext.runAsNonRoot=true`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + result := runAsNonRoot_1_0(&tc.pod.ObjectMeta, &tc.pod.Spec) + if result.Allowed { + t.Fatal("expected disallowed") + } + if e, a := tc.expectReason, result.ForbiddenReason; e != a { + t.Errorf("expected\n%s\ngot\n%s", e, a) + } + if e, a := tc.expectDetail, result.ForbiddenDetail; e != a { + t.Errorf("expected\n%s\ngot\n%s", e, a) + } + }) + } +} diff --git a/staging/src/k8s.io/pod-security-admission/test/fixtures_runAsNonRoot.go b/staging/src/k8s.io/pod-security-admission/test/fixtures_runAsNonRoot.go index ec199b7844e..f013c483510 100644 --- a/staging/src/k8s.io/pod-security-admission/test/fixtures_runAsNonRoot.go +++ b/staging/src/k8s.io/pod-security-admission/test/fixtures_runAsNonRoot.go @@ -52,12 +52,6 @@ func init() { p.Spec.Containers[0].SecurityContext.RunAsNonRoot = pointer.BoolPtr(true) p.Spec.InitContainers[0].SecurityContext.RunAsNonRoot = pointer.BoolPtr(true) }), - // set at pod level and containers - tweak(p, func(p *corev1.Pod) { - p.Spec.SecurityContext.RunAsNonRoot = pointer.BoolPtr(true) - p.Spec.Containers[0].SecurityContext.RunAsNonRoot = pointer.BoolPtr(true) - p.Spec.InitContainers[0].SecurityContext.RunAsNonRoot = pointer.BoolPtr(true) - }), } }, generateFail: func(p *corev1.Pod) []*corev1.Pod { @@ -74,12 +68,6 @@ func init() { // explicit false on containers tweak(p, func(p *corev1.Pod) { p.Spec.Containers[0].SecurityContext.RunAsNonRoot = pointer.BoolPtr(false) }), tweak(p, func(p *corev1.Pod) { p.Spec.InitContainers[0].SecurityContext.RunAsNonRoot = pointer.BoolPtr(false) }), - // nil security contexts - tweak(p, func(p *corev1.Pod) { - p.Spec.SecurityContext = nil - p.Spec.Containers[0].SecurityContext = nil - p.Spec.InitContainers[0].SecurityContext = nil - }), } }, }