From 445393fdcefa6d0354b7ce32a2304a7765fbd305 Mon Sep 17 00:00:00 2001
From: Slava Semushin <vsemushi@redhat.com>
Date: Tue, 28 Nov 2017 17:28:58 +0100
Subject: [PATCH] kubelet: MustRunAsNonRoot should reject a pod if it has
 non-numeric USER.

---
 pkg/kubelet/kuberuntime/kuberuntime_container.go   | 11 ++++-------
 .../kuberuntime/kuberuntime_container_test.go      | 14 +++++++++++++-
 pkg/kubelet/kuberuntime/security_context.go        | 11 +++++++----
 pkg/kubelet/kuberuntime/security_context_test.go   |  3 ++-
 4 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/pkg/kubelet/kuberuntime/kuberuntime_container.go b/pkg/kubelet/kuberuntime/kuberuntime_container.go
index 1b622b2ca66d..de0b6fdd7f38 100644
--- a/pkg/kubelet/kuberuntime/kuberuntime_container.go
+++ b/pkg/kubelet/kuberuntime/kuberuntime_container.go
@@ -183,13 +183,10 @@ func (m *kubeGenericRuntimeManager) generateContainerConfig(container *v1.Contai
 	if err != nil {
 		return nil, err
 	}
-	if uid != nil {
-		// Verify RunAsNonRoot. Non-root verification only supports numeric user.
-		if err := verifyRunAsNonRoot(pod, container, *uid); err != nil {
-			return nil, err
-		}
-	} else if username != "" {
-		glog.Warningf("Non-root verification doesn't support non-numeric user (%s)", username)
+
+	// Verify RunAsNonRoot. Non-root verification only supports numeric user.
+	if err := verifyRunAsNonRoot(pod, container, uid, username); err != nil {
+		return nil, err
 	}
 
 	command, args := kubecontainer.ExpandContainerCommandAndArgs(container, opts.Envs)
diff --git a/pkg/kubelet/kuberuntime/kuberuntime_container_test.go b/pkg/kubelet/kuberuntime/kuberuntime_container_test.go
index 86f153ca40fe..c91a3a82e3f4 100644
--- a/pkg/kubelet/kuberuntime/kuberuntime_container_test.go
+++ b/pkg/kubelet/kuberuntime/kuberuntime_container_test.go
@@ -236,7 +236,7 @@ func makeExpectedConfig(m *kubeGenericRuntimeManager, pod *v1.Pod, containerInde
 }
 
 func TestGenerateContainerConfig(t *testing.T) {
-	_, _, m, err := createTestRuntimeManager()
+	_, imageService, m, err := createTestRuntimeManager()
 	assert.NoError(t, err)
 
 	pod := &v1.Pod{
@@ -290,6 +290,18 @@ func TestGenerateContainerConfig(t *testing.T) {
 
 	_, err = m.generateContainerConfig(&podWithContainerSecurityContext.Spec.Containers[0], podWithContainerSecurityContext, 0, "", podWithContainerSecurityContext.Spec.Containers[0].Image)
 	assert.Error(t, err)
+
+	imageId, _ := imageService.PullImage(&runtimeapi.ImageSpec{Image: "busybox"}, nil)
+	image, _ := imageService.ImageStatus(&runtimeapi.ImageSpec{Image: imageId})
+
+	image.Uid = nil
+	image.Username = "test"
+
+	podWithContainerSecurityContext.Spec.Containers[0].SecurityContext.RunAsUser = nil
+	podWithContainerSecurityContext.Spec.Containers[0].SecurityContext.RunAsNonRoot = &runAsNonRootTrue
+
+	_, err = m.generateContainerConfig(&podWithContainerSecurityContext.Spec.Containers[0], podWithContainerSecurityContext, 0, "", podWithContainerSecurityContext.Spec.Containers[0].Image)
+	assert.Error(t, err, "RunAsNonRoot should fail for non-numeric username")
 }
 
 func TestLifeCycleHook(t *testing.T) {
diff --git a/pkg/kubelet/kuberuntime/security_context.go b/pkg/kubelet/kuberuntime/security_context.go
index c0d4ce7347a1..3d48d372ac44 100644
--- a/pkg/kubelet/kuberuntime/security_context.go
+++ b/pkg/kubelet/kuberuntime/security_context.go
@@ -75,7 +75,7 @@ func (m *kubeGenericRuntimeManager) determineEffectiveSecurityContext(pod *v1.Po
 }
 
 // verifyRunAsNonRoot verifies RunAsNonRoot.
-func verifyRunAsNonRoot(pod *v1.Pod, container *v1.Container, uid int64) error {
+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.
 	if effectiveSc == nil || effectiveSc.RunAsNonRoot == nil || !*effectiveSc.RunAsNonRoot {
@@ -89,11 +89,14 @@ func verifyRunAsNonRoot(pod *v1.Pod, container *v1.Container, uid int64) error {
 		return nil
 	}
 
-	if uid == 0 {
+	switch {
+	case uid != nil && *uid == 0:
 		return fmt.Errorf("container has runAsNonRoot and image will run as root")
+	case uid == nil && len(username) > 0:
+		return fmt.Errorf("container has runAsNonRoot and image has non-numeric user (%s), cannot verify user is non-root", username)
+	default:
+		return nil
 	}
-
-	return nil
 }
 
 // convertToRuntimeSecurityContext converts v1.SecurityContext to runtimeapi.SecurityContext.
diff --git a/pkg/kubelet/kuberuntime/security_context_test.go b/pkg/kubelet/kuberuntime/security_context_test.go
index ae724ab314aa..d3261b51f5b1 100644
--- a/pkg/kubelet/kuberuntime/security_context_test.go
+++ b/pkg/kubelet/kuberuntime/security_context_test.go
@@ -105,7 +105,8 @@ func TestVerifyRunAsNonRoot(t *testing.T) {
 		},
 	} {
 		pod.Spec.Containers[0].SecurityContext = test.sc
-		err := verifyRunAsNonRoot(pod, &pod.Spec.Containers[0], int64(0))
+		uid := int64(0)
+		err := verifyRunAsNonRoot(pod, &pod.Spec.Containers[0], &uid, "")
 		if test.fail {
 			assert.Error(t, err, test.desc)
 		} else {