From 588ff515bc349b213f5547a2231fb2dd58f8a177 Mon Sep 17 00:00:00 2001 From: Mark Rossetti Date: Thu, 7 Jul 2022 11:24:26 -0700 Subject: [PATCH] Windows: ensure runAsNonRoot does case-insensitive comparison on user name Signed-off-by: Mark Rossetti --- .../kuberuntime/security_context_windows.go | 10 +++-- .../security_context_windows_test.go | 42 +++++++++++++++++++ test/e2e/windows/security_context.go | 36 ++++++++++++++++ 3 files changed, 85 insertions(+), 3 deletions(-) diff --git a/pkg/kubelet/kuberuntime/security_context_windows.go b/pkg/kubelet/kuberuntime/security_context_windows.go index 87b777f5e0a..21946a27602 100644 --- a/pkg/kubelet/kuberuntime/security_context_windows.go +++ b/pkg/kubelet/kuberuntime/security_context_windows.go @@ -25,6 +25,7 @@ import ( "k8s.io/klog/v2" "k8s.io/kubernetes/pkg/kubelet/util/format" "k8s.io/kubernetes/pkg/securitycontext" + "strings" ) var ( @@ -37,6 +38,7 @@ var ( // According to the discussion of sig-windows, at present, we assume that ContainerAdministrator is the windows container root user, // and then optimize this logic according to the best time. // https://docs.google.com/document/d/1Tjxzjjuy4SQsFSUVXZbvqVb64hjNAG5CQX8bK7Yda9w +// note: usernames on Windows are NOT case sensitive! func verifyRunAsNonRoot(pod *v1.Pod, container *v1.Container, uid *int64, username string) error { effectiveSc := securitycontext.DetermineEffectiveSecurityContext(pod, container) // If the option is not set, or if running as root is allowed, return nil. @@ -54,15 +56,17 @@ func verifyRunAsNonRoot(pod *v1.Pod, container *v1.Container, uid *int64, userna if effectiveSc.RunAsGroup != nil { klog.InfoS("Windows container does not support SecurityContext.RunAsGroup", "pod", klog.KObj(pod), "containerName", container.Name) } + // Verify that if runAsUserName is set for the pod and/or container that it is not set to 'ContainerAdministrator' if effectiveSc.WindowsOptions != nil { if effectiveSc.WindowsOptions.RunAsUserName != nil { - if *effectiveSc.WindowsOptions.RunAsUserName == windowsRootUserName { - return fmt.Errorf("container's runAsUser (%s) which will be regarded as root identity and will break non-root policy (pod: %q, container: %s)", username, format.Pod(pod), container.Name) + if strings.EqualFold(*effectiveSc.WindowsOptions.RunAsUserName, windowsRootUserName) { + return fmt.Errorf("container's runAsUserName (%s) which will be regarded as root identity and will break non-root policy (pod: %q, container: %s)", *effectiveSc.WindowsOptions.RunAsUserName, format.Pod(pod), container.Name) } return nil } } - if len(username) > 0 && username == windowsRootUserName { + // Verify that if runAsUserName is NOT set for the pod and/or container that the default user for the container image is not set to 'ContainerAdministrator' + if len(username) > 0 && strings.EqualFold(username, windowsRootUserName) { return fmt.Errorf("container's runAsUser (%s) which will be regarded as root identity and will break non-root policy (pod: %q, container: %s)", username, format.Pod(pod), container.Name) } return nil diff --git a/pkg/kubelet/kuberuntime/security_context_windows_test.go b/pkg/kubelet/kuberuntime/security_context_windows_test.go index 6cca733b5d9..98a63119427 100644 --- a/pkg/kubelet/kuberuntime/security_context_windows_test.go +++ b/pkg/kubelet/kuberuntime/security_context_windows_test.go @@ -47,6 +47,7 @@ func TestVerifyRunAsNonRoot(t *testing.T) { }, } rootUser := "ContainerAdministrator" + rootUserUppercase := "CONTAINERADMINISTRATOR" anyUser := "anyone" runAsNonRootTrue := true runAsNonRootFalse := false @@ -101,6 +102,17 @@ func TestVerifyRunAsNonRoot(t *testing.T) { username: rootUser, fail: true, }, + { + desc: "Fail if container's RunAsUser is root (case-insensitive) and RunAsNonRoot is true", + sc: &v1.SecurityContext{ + RunAsNonRoot: &runAsNonRootTrue, + WindowsOptions: &v1.WindowsSecurityContextOptions{ + RunAsUserName: &rootUserUppercase, + }, + }, + username: anyUser, + fail: true, + }, { desc: "Fail if image's user is root and RunAsNonRoot is true", sc: &v1.SecurityContext{ @@ -109,6 +121,14 @@ func TestVerifyRunAsNonRoot(t *testing.T) { username: rootUser, fail: true, }, + { + desc: "Fail if image's user is root (case-insensitive) and RunAsNonRoot is true", + sc: &v1.SecurityContext{ + RunAsNonRoot: &runAsNonRootTrue, + }, + username: rootUserUppercase, + fail: true, + }, { desc: "Pass if image's user is non-root and RunAsNonRoot is true", sc: &v1.SecurityContext{ @@ -124,6 +144,28 @@ func TestVerifyRunAsNonRoot(t *testing.T) { }, fail: false, }, + { + desc: "Pass if image's user is root, container's RunAsUser is not root and RunAsNonRoot is true", + sc: &v1.SecurityContext{ + RunAsNonRoot: &runAsNonRootTrue, + WindowsOptions: &v1.WindowsSecurityContextOptions{ + RunAsUserName: &anyUser, + }, + }, + username: rootUser, + fail: false, + }, + { + desc: "Pass if image's user is root (case-insensitive), container's RunAsUser is not root and RunAsNonRoot is true", + sc: &v1.SecurityContext{ + RunAsNonRoot: &runAsNonRootTrue, + WindowsOptions: &v1.WindowsSecurityContextOptions{ + RunAsUserName: &anyUser, + }, + }, + username: rootUserUppercase, + fail: false, + }, } { pod.Spec.Containers[0].SecurityContext = test.sc err := verifyRunAsNonRoot(pod, &pod.Spec.Containers[0], test.uid, test.username) diff --git a/test/e2e/windows/security_context.go b/test/e2e/windows/security_context.go index 04188758b6b..4559d79a079 100644 --- a/test/e2e/windows/security_context.go +++ b/test/e2e/windows/security_context.go @@ -153,6 +153,42 @@ var _ = SIGDescribe("[Feature:Windows] SecurityContext", func() { framework.ExpectNoError(e2epod.WaitForPodNameRunningInNamespace(f.ClientSet, windowsPodWithSELinux.Name, f.Namespace.Name), "failed to wait for pod %s to be running", windowsPodWithSELinux.Name) }) + + ginkgo.It("should not be able to create pods with containers running as ContainerAdministrator when runAsNonRoot is true", func() { + ginkgo.By("Creating a pod") + + p := runAsUserNamePod(toPtr("ContainerAdministrator")) + p.Spec.SecurityContext.RunAsNonRoot = &trueVar + + podInvalid, err := f.ClientSet.CoreV1().Pods(f.Namespace.Name).Create(context.TODO(), p, metav1.CreateOptions{}) + framework.ExpectNoError(err, "Error creating pod") + + ginkgo.By("Waiting for pod to finish") + event, err := f.PodClient().WaitForErrorEventOrSuccess(podInvalid) + framework.ExpectNoError(err) + framework.ExpectNotEqual(event, nil, "event should not be empty") + framework.Logf("Got event: %v", event) + expectedEventError := "container's runAsUserName (ContainerAdministrator) which will be regarded as root identity and will break non-root policy" + framework.ExpectEqual(true, strings.Contains(event.Message, expectedEventError), "Event error should indicate non-root policy caused container to not start") + }) + + ginkgo.It("should not be able to create pods with containers running as CONTAINERADMINISTRATOR when runAsNonRoot is true", func() { + ginkgo.By("Creating a pod") + + p := runAsUserNamePod(toPtr("CONTAINERADMINISTRATOR")) + p.Spec.SecurityContext.RunAsNonRoot = &trueVar + + podInvalid, err := f.ClientSet.CoreV1().Pods(f.Namespace.Name).Create(context.TODO(), p, metav1.CreateOptions{}) + framework.ExpectNoError(err, "Error creating pod") + + ginkgo.By("Waiting for pod to finish") + event, err := f.PodClient().WaitForErrorEventOrSuccess(podInvalid) + framework.ExpectNoError(err) + framework.ExpectNotEqual(event, nil, "event should not be empty") + framework.Logf("Got event: %v", event) + expectedEventError := "container's runAsUserName (CONTAINERADMINISTRATOR) which will be regarded as root identity and will break non-root policy" + framework.ExpectEqual(true, strings.Contains(event.Message, expectedEventError), "Event error should indicate non-root policy caused container to not start") + }) }) func runAsUserNamePod(username *string) *v1.Pod {