From 07a67c252cacdb64da6dd951fcb0357aa1a10f75 Mon Sep 17 00:00:00 2001
From: Yu-Ju Hong <yjhong@google.com>
Date: Mon, 5 Jun 2017 17:36:53 -0700
Subject: [PATCH] kuberuntime: check the value of RunAsNonRoot when verifying

The verification function is fixed to check the value of RunAsNonRoot,
not just the existence of it. Also adds unit tests to verify the correct
behavior.
---
 .../kuberuntime/kuberuntime_container_test.go |  4 +-
 pkg/kubelet/kuberuntime/security_context.go   |  3 +-
 .../kuberuntime/security_context_test.go      | 95 +++++++++----------
 3 files changed, 50 insertions(+), 52 deletions(-)

diff --git a/pkg/kubelet/kuberuntime/kuberuntime_container_test.go b/pkg/kubelet/kuberuntime/kuberuntime_container_test.go
index 0f02ddc2ec8..64e9c4ca5e7 100644
--- a/pkg/kubelet/kuberuntime/kuberuntime_container_test.go
+++ b/pkg/kubelet/kuberuntime/kuberuntime_container_test.go
@@ -228,7 +228,7 @@ func TestGenerateContainerConfig(t *testing.T) {
 	assert.Equal(t, expectedConfig, containerConfig, "generate container config for kubelet runtime v1.")
 
 	runAsUser := types.UnixUserID(0)
-	RunAsNonRoot := false
+	runAsNonRootTrue := true
 	podWithContainerSecurityContext := &v1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			UID:       "12345678",
@@ -244,7 +244,7 @@ func TestGenerateContainerConfig(t *testing.T) {
 					Command:         []string{"testCommand"},
 					WorkingDir:      "testWorkingDir",
 					SecurityContext: &v1.SecurityContext{
-						RunAsNonRoot: &RunAsNonRoot,
+						RunAsNonRoot: &runAsNonRootTrue,
 						RunAsUser:    &runAsUser,
 					},
 				},
diff --git a/pkg/kubelet/kuberuntime/security_context.go b/pkg/kubelet/kuberuntime/security_context.go
index c63bd212700..6c76db9639d 100644
--- a/pkg/kubelet/kuberuntime/security_context.go
+++ b/pkg/kubelet/kuberuntime/security_context.go
@@ -72,7 +72,8 @@ func (m *kubeGenericRuntimeManager) determineEffectiveSecurityContext(pod *v1.Po
 // verifyRunAsNonRoot verifies RunAsNonRoot.
 func verifyRunAsNonRoot(pod *v1.Pod, container *v1.Container, uid int64) error {
 	effectiveSc := securitycontext.DetermineEffectiveSecurityContext(pod, container)
-	if effectiveSc == nil || effectiveSc.RunAsNonRoot == nil {
+	// If the option is not set, or if running as root is allowed, return nil.
+	if effectiveSc == nil || effectiveSc.RunAsNonRoot == nil || !*effectiveSc.RunAsNonRoot {
 		return nil
 	}
 
diff --git a/pkg/kubelet/kuberuntime/security_context_test.go b/pkg/kubelet/kuberuntime/security_context_test.go
index 414d2e711d1..9408cf90be9 100644
--- a/pkg/kubelet/kuberuntime/security_context_test.go
+++ b/pkg/kubelet/kuberuntime/security_context_test.go
@@ -45,60 +45,57 @@ func TestVerifyRunAsNonRoot(t *testing.T) {
 		},
 	}
 
-	err := verifyRunAsNonRoot(pod, &pod.Spec.Containers[0], int64(0))
-	assert.NoError(t, err)
+	rootUser := types.UnixUserID(0)
+	runAsNonRootTrue := true
+	runAsNonRootFalse := false
 
-	runAsUser := types.UnixUserID(0)
-	RunAsNonRoot := false
-	podWithContainerSecurityContext := &v1.Pod{
-		ObjectMeta: metav1.ObjectMeta{
-			UID:       "12345678",
-			Name:      "bar",
-			Namespace: "new",
+	for _, test := range []struct {
+		desc   string
+		sc     *v1.SecurityContext
+		errStr string
+	}{
+		{
+			desc:   "Pass if SecurityContext is not set",
+			sc:     nil,
+			errStr: "",
 		},
-		Spec: v1.PodSpec{
-			Containers: []v1.Container{
-				{
-					Name:            "foo",
-					Image:           "busybox",
-					ImagePullPolicy: v1.PullIfNotPresent,
-					Command:         []string{"testCommand"},
-					WorkingDir:      "testWorkingDir",
-					SecurityContext: &v1.SecurityContext{
-						RunAsNonRoot: &RunAsNonRoot,
-						RunAsUser:    &runAsUser,
-					},
-				},
+		{
+			desc: "Pass if RunAsNonRoot is not set",
+			sc: &v1.SecurityContext{
+				RunAsUser: &rootUser,
 			},
+			errStr: "",
 		},
-	}
-
-	err2 := verifyRunAsNonRoot(podWithContainerSecurityContext, &podWithContainerSecurityContext.Spec.Containers[0], int64(0))
-	assert.EqualError(t, err2, "container's runAsUser breaks non-root policy")
-
-	RunAsNonRoot = false
-	podWithContainerSecurityContext = &v1.Pod{
-		ObjectMeta: metav1.ObjectMeta{
-			UID:       "12345678",
-			Name:      "bar",
-			Namespace: "new",
-		},
-		Spec: v1.PodSpec{
-			Containers: []v1.Container{
-				{
-					Name:            "foo",
-					Image:           "busybox",
-					ImagePullPolicy: v1.PullIfNotPresent,
-					Command:         []string{"testCommand"},
-					WorkingDir:      "testWorkingDir",
-					SecurityContext: &v1.SecurityContext{
-						RunAsNonRoot: &RunAsNonRoot,
-					},
-				},
+		{
+			desc: "Pass if RunAsNonRoot is false",
+			sc: &v1.SecurityContext{
+				RunAsNonRoot: &runAsNonRootFalse,
+				RunAsUser:    &rootUser,
 			},
+			errStr: "",
 		},
+		{
+			desc: "Fail if container's RunAsUser is root and RunAsNonRoot is true",
+			sc: &v1.SecurityContext{
+				RunAsNonRoot: &runAsNonRootTrue,
+				RunAsUser:    &rootUser,
+			},
+			errStr: "container's runAsUser breaks non-root policy",
+		},
+		{
+			desc: "Fail if image's user is root and RunAsNonRoot is true",
+			sc: &v1.SecurityContext{
+				RunAsNonRoot: &runAsNonRootTrue,
+			},
+			errStr: "container has runAsNonRoot and image will run as root",
+		},
+	} {
+		pod.Spec.Containers[0].SecurityContext = test.sc
+		err := verifyRunAsNonRoot(pod, &pod.Spec.Containers[0], int64(0))
+		if len(test.errStr) == 0 {
+			assert.NoError(t, err, test.desc)
+		} else {
+			assert.EqualError(t, err, test.errStr, test.desc)
+		}
 	}
-
-	err3 := verifyRunAsNonRoot(podWithContainerSecurityContext, &podWithContainerSecurityContext.Spec.Containers[0], int64(0))
-	assert.EqualError(t, err3, "container has runAsNonRoot and image will run as root")
 }