#146 Fixing Container Security Context Logic (#149)

* Fixing Container Security Context Logic

Kubernetes rationalizes Container Security Context in conjunction with the
Pod Spec Security Context. In this scenario you can 'leave out' certain
security context settings and rely on the pod spec definition to still
set these settings for you. The RunAsNonRoot setting originally only checked
to see if the value was set at the container level, vs also checking if it
was enabled at the pod level.

I have attached the container's parent pod spec to the container validate
struct in case any other things like this arise in the future.

I have also refactored the logic for validating bool pointers, since these
can be tricky, if you want to avoid dereferences pointer issues.

Changes:
- Added parent pod spec of container to validate certain settings which affect container spec
- Refactored the logic statements for validating bool pointers (used helpers)
- Added tests for this pod.container.securityContext condition

CHANGELOG.md

@@ -1,3 +1,6 @@
+# x.x.x (Next Release)
+* BUG: Fixed logic on RunAsNonRoot check to incorporate settings in podSpec
+
 # 0.2.0
 * Added `--output-format` flag for better CI/CD support
 * Added `--display-name` flag

pkg/validator/container.go

@@ -29,16 +29,34 @@ type ContainerValidation struct {
 	*ResourceValidation
 	Container       *corev1.Container
 	IsInitContainer bool
+	parentPodSpec   corev1.PodSpec
 }
 
 // ValidateContainer validates that each pod conforms to the Polaris config, returns a ResourceResult.
-func ValidateContainer(cnConf *conf.Configuration, container *corev1.Container, isInit bool) ContainerResult {
+// FIXME When validating a container, there are some things in a container spec
+//       that can be affected by the podSpec. This means we need a copy of the
+//       relevant podSpec in order to check certain aspects of a containerSpec.
+//       Perhaps there is a more ideal solution instead of attaching a parent
+//       podSpec to every container Validation struct...
+func ValidateContainer(container *corev1.Container, parentPodResult *PodResult, cnConf *conf.Configuration, isInit bool) ContainerResult {
 	cv := ContainerValidation{
 		Container:          container,
 		ResourceValidation: &ResourceValidation{},
 		IsInitContainer:    isInit,
 	}
 
+	// Support initializing
+	// FIXME This is a product of pulling in the podSpec, ideally we'd never
+	//       expect this be nil but our tests have conditions in which the
+	//       parent podResult isn't initialized in this ContainerValidation
+	//       struct.
+	if parentPodResult == nil {
+		// initialize a blank pod spec
+		cv.parentPodSpec = corev1.PodSpec{}
+	} else {
+		cv.parentPodSpec = parentPodResult.podSpec
+	}
+
 	cv.validateResources(&cnConf.Resources)
 	cv.validateHealthChecks(&cnConf.HealthChecks)
 	cv.validateImage(&cnConf.Images)
@@ -176,39 +194,58 @@ func (cv *ContainerValidation) validateNetworking(networkConf *conf.Networking)
 func (cv *ContainerValidation) validateSecurity(securityConf *conf.Security) {
 	category := messages.CategorySecurity
 	securityContext := cv.Container.SecurityContext
+	podSecurityContext := cv.parentPodSpec.SecurityContext
+
+	// Support an empty container security context
 	if securityContext == nil {
 		securityContext = &corev1.SecurityContext{}
 	}
 
+	// Support an empty pod security context
+	if podSecurityContext == nil {
+		podSecurityContext = &corev1.PodSecurityContext{}
+	}
+
 	if securityConf.RunAsRootAllowed.IsActionable() {
-		if securityContext.RunAsNonRoot == (*bool)(nil) || !*securityContext.RunAsNonRoot {
-			cv.addFailure(messages.RunAsRootFailure, securityConf.RunAsRootAllowed, category)
-		} else {
+		if getBoolValue(securityContext.RunAsNonRoot) {
+			// Check if the container is explicitly set to True (pass)
 			cv.addSuccess(messages.RunAsRootSuccess, category)
+		} else if securityContext.RunAsNonRoot == nil {
+			// Check if the value in the container spec if nil (thus defaulting to the podspec)
+			// Check if the container value is not set
+			if getBoolValue(podSecurityContext.RunAsNonRoot) {
+				// if the pod spec default for containers is true, then pass
+				cv.addSuccess(messages.RunAsRootSuccess, category)
+			} else {
+				// else fail as RunAsNonRoot defaults to false
+				cv.addFailure(messages.RunAsRootFailure, securityConf.RunAsRootAllowed, category)
+			}
+		} else {
+			cv.addFailure(messages.RunAsRootFailure, securityConf.RunAsRootAllowed, category)
 		}
 	}
 
 	if securityConf.RunAsPrivileged.IsActionable() {
-		if securityContext.Privileged == (*bool)(nil) || !*securityContext.Privileged {
-			cv.addSuccess(messages.RunAsPrivilegedSuccess, category)
-		} else {
+		if getBoolValue(securityContext.Privileged) {
 			cv.addFailure(messages.RunAsPrivilegedFailure, securityConf.RunAsPrivileged, category)
+		} else {
+			cv.addSuccess(messages.RunAsPrivilegedSuccess, category)
 		}
 	}
 
 	if securityConf.NotReadOnlyRootFileSystem.IsActionable() {
-		if securityContext.ReadOnlyRootFilesystem == (*bool)(nil) || !*securityContext.ReadOnlyRootFilesystem {
-			cv.addFailure(messages.ReadOnlyFilesystemFailure, securityConf.NotReadOnlyRootFileSystem, category)
-		} else {
+		if getBoolValue(securityContext.ReadOnlyRootFilesystem) {
 			cv.addSuccess(messages.ReadOnlyFilesystemSuccess, category)
+		} else {
+			cv.addFailure(messages.ReadOnlyFilesystemFailure, securityConf.NotReadOnlyRootFileSystem, category)
 		}
 	}
 
 	if securityConf.PrivilegeEscalationAllowed.IsActionable() {
-		if securityContext.AllowPrivilegeEscalation == (*bool)(nil) || !*securityContext.AllowPrivilegeEscalation {
-			cv.addSuccess(messages.PrivilegeEscalationSuccess, category)
-		} else {
+		if getBoolValue(securityContext.AllowPrivilegeEscalation) {
 			cv.addFailure(messages.PrivilegeEscalationFailure, securityConf.PrivilegeEscalationAllowed, category)
+		} else {
+			cv.addSuccess(messages.PrivilegeEscalationSuccess, category)
 		}
 	}
 
@@ -323,3 +360,12 @@ func capContains(list []corev1.Capability, val corev1.Capability) bool {
 
 	return false
 }
+
+// getBoolValue returns false if nil or returns the value of the bool pointer
+func getBoolValue(val *bool) bool {
+	if val == nil {
+		return false
+	}
+
+	return *val
+}

pkg/validator/container_test.go

@@ -567,6 +567,42 @@ func TestValidateSecurity(t *testing.T) {
 		ResourceValidation: &ResourceValidation{},
 	}
 
+	badCVWithGoodPodSpec := ContainerValidation{
+		Container: &corev1.Container{Name: "", SecurityContext: &corev1.SecurityContext{
+			RunAsNonRoot:             &falseVar,
+			ReadOnlyRootFilesystem:   &falseVar,
+			Privileged:               &trueVar,
+			AllowPrivilegeEscalation: &trueVar,
+			Capabilities: &corev1.Capabilities{
+				Add: []corev1.Capability{"AUDIT_CONTROL", "SYS_ADMIN", "NET_ADMIN"},
+			},
+		}},
+		ResourceValidation: &ResourceValidation{},
+		parentPodSpec: corev1.PodSpec{
+			SecurityContext: &corev1.PodSecurityContext{
+				RunAsNonRoot: &trueVar,
+			},
+		},
+	}
+
+	badCVWithBadPodSpec := ContainerValidation{
+		Container: &corev1.Container{Name: "", SecurityContext: &corev1.SecurityContext{
+			RunAsNonRoot:             nil, // this will use the default from the podspec
+			ReadOnlyRootFilesystem:   &falseVar,
+			Privileged:               &trueVar,
+			AllowPrivilegeEscalation: &trueVar,
+			Capabilities: &corev1.Capabilities{
+				Add: []corev1.Capability{"AUDIT_CONTROL", "SYS_ADMIN", "NET_ADMIN"},
+			},
+		}},
+		ResourceValidation: &ResourceValidation{},
+		parentPodSpec: corev1.PodSpec{
+			SecurityContext: &corev1.PodSecurityContext{
+				RunAsNonRoot: &falseVar,
+			},
+		},
+	}
+
 	goodCV := ContainerValidation{
 		Container: &corev1.Container{Name: "", SecurityContext: &corev1.SecurityContext{
 			RunAsNonRoot:             &trueVar,
@@ -593,6 +629,42 @@ func TestValidateSecurity(t *testing.T) {
 		ResourceValidation: &ResourceValidation{},
 	}
 
+	strongCVWithPodSpecSecurityContext := ContainerValidation{
+		Container: &corev1.Container{Name: "", SecurityContext: &corev1.SecurityContext{
+			RunAsNonRoot:             nil, // not set but overridden via podSpec
+			ReadOnlyRootFilesystem:   &trueVar,
+			Privileged:               &falseVar,
+			AllowPrivilegeEscalation: &falseVar,
+			Capabilities: &corev1.Capabilities{
+				Drop: []corev1.Capability{"ALL"},
+			},
+		}},
+		ResourceValidation: &ResourceValidation{},
+		parentPodSpec: corev1.PodSpec{
+			SecurityContext: &corev1.PodSecurityContext{
+				RunAsNonRoot: &trueVar,
+			},
+		},
+	}
+
+	strongCVWithBadPodSpecSecurityContext := ContainerValidation{
+		Container: &corev1.Container{Name: "", SecurityContext: &corev1.SecurityContext{
+			RunAsNonRoot:             &trueVar, // will override the bad setting in PodSpec
+			ReadOnlyRootFilesystem:   &trueVar,
+			Privileged:               &falseVar,
+			AllowPrivilegeEscalation: &falseVar,
+			Capabilities: &corev1.Capabilities{
+				Drop: []corev1.Capability{"ALL"},
+			},
+		}},
+		ResourceValidation: &ResourceValidation{},
+		parentPodSpec: corev1.PodSpec{
+			SecurityContext: &corev1.PodSecurityContext{
+				RunAsNonRoot: &falseVar, // is overridden at container level with RunAsNonRoot:true
+			},
+		},
+	}
+
 	var testCases = []struct {
 		name             string
 		securityConf     conf.Security
@@ -661,6 +733,66 @@ func TestValidateSecurity(t *testing.T) {
 				Category: "Security",
 			}},
 		},
+		{
+			name:         "bad security context + standard validation config with good settings in podspec",
+			securityConf: standardConf,
+			cv:           badCVWithGoodPodSpec,
+			expectedMessages: []*ResultMessage{{
+				Message:  "The following security capabilities should not be added: SYS_ADMIN, NET_ADMIN",
+				Type:     "error",
+				Category: "Security",
+			}, {
+				Message:  "Privilege escalation should not be allowed",
+				Type:     "error",
+				Category: "Security",
+			}, {
+				Message:  "Should not be running as privileged",
+				Type:     "error",
+				Category: "Security",
+			}, {
+				Message:  "The following security capabilities should not be added: AUDIT_CONTROL, SYS_ADMIN, NET_ADMIN",
+				Type:     "warning",
+				Category: "Security",
+			}, {
+				Message:  "Should not be allowed to run as root",
+				Type:     "warning",
+				Category: "Security",
+			}, {
+				Message:  "Filesystem should be read only",
+				Type:     "warning",
+				Category: "Security",
+			}},
+		},
+		{
+			name:         "bad security context + standard validation config from default set in podspec",
+			securityConf: standardConf,
+			cv:           badCVWithBadPodSpec,
+			expectedMessages: []*ResultMessage{{
+				Message:  "The following security capabilities should not be added: SYS_ADMIN, NET_ADMIN",
+				Type:     "error",
+				Category: "Security",
+			}, {
+				Message:  "Privilege escalation should not be allowed",
+				Type:     "error",
+				Category: "Security",
+			}, {
+				Message:  "Should not be running as privileged",
+				Type:     "error",
+				Category: "Security",
+			}, {
+				Message:  "The following security capabilities should not be added: AUDIT_CONTROL, SYS_ADMIN, NET_ADMIN",
+				Type:     "warning",
+				Category: "Security",
+			}, {
+				Message:  "Should not be allowed to run as root",
+				Type:     "warning",
+				Category: "Security",
+			}, {
+				Message:  "Filesystem should be read only",
+				Type:     "warning",
+				Category: "Security",
+			}},
+		},
 		{
 			name:         "good security context + standard validation config",
 			securityConf: standardConf,
@@ -739,6 +871,58 @@ func TestValidateSecurity(t *testing.T) {
 				Category: "Security",
 			}},
 		},
+		{
+			name:         "strong security context + strong validation config via podspec default",
+			securityConf: strongConf,
+			cv:           strongCVWithPodSpecSecurityContext,
+			expectedMessages: []*ResultMessage{{
+				Message:  "Is not allowed to run as root",
+				Type:     "success",
+				Category: "Security",
+			}, {
+				Message:  "Filesystem is read only",
+				Type:     "success",
+				Category: "Security",
+			}, {
+				Message:  "Not running as privileged",
+				Type:     "success",
+				Category: "Security",
+			}, {
+				Message:  "Privilege escalation not allowed",
+				Type:     "success",
+				Category: "Security",
+			}, {
+				Message:  "Security capabilities are within the configured limits",
+				Type:     "success",
+				Category: "Security",
+			}},
+		},
+		{
+			name:         "strong security context + strong validation config with bad setting in podspec default",
+			securityConf: strongConf,
+			cv:           strongCVWithBadPodSpecSecurityContext,
+			expectedMessages: []*ResultMessage{{
+				Message:  "Is not allowed to run as root",
+				Type:     "success",
+				Category: "Security",
+			}, {
+				Message:  "Filesystem is read only",
+				Type:     "success",
+				Category: "Security",
+			}, {
+				Message:  "Not running as privileged",
+				Type:     "success",
+				Category: "Security",
+			}, {
+				Message:  "Privilege escalation not allowed",
+				Type:     "success",
+				Category: "Security",
+			}, {
+				Message:  "Security capabilities are within the configured limits",
+				Type:     "success",
+				Category: "Security",
+			}},
+		},
 	}
 
 	for _, tt := range testCases {

pkg/validator/pod.go

@@ -40,6 +40,7 @@ func ValidatePod(podConf conf.Configuration, pod *corev1.PodSpec) PodResult {
 		Messages:         pv.messages(),
 		ContainerResults: []ContainerResult{},
 		Summary:          pv.summary(),
+		podSpec:          *pod,
 	}
 
 	pv.validateContainers(pod.InitContainers, &pRes, &podConf, true)
@@ -54,7 +55,7 @@ func ValidatePod(podConf conf.Configuration, pod *corev1.PodSpec) PodResult {
 
 func (pv *PodValidation) validateContainers(containers []corev1.Container, pRes *PodResult, podConf *conf.Configuration, isInit bool) {
 	for _, container := range containers {
-		cRes := ValidateContainer(podConf, &container, isInit)
+		cRes := ValidateContainer(&container, pRes, podConf, isInit)
 		pRes.ContainerResults = append(pRes.ContainerResults, cRes)
 	}
 }

pkg/validator/types.go

@@ -14,6 +14,8 @@
 
 package validator
 
+import corev1 "k8s.io/api/core/v1"
+
 // MessageType represents the type of Message
 type MessageType string
 
@@ -116,6 +118,7 @@ type PodResult struct {
 	Summary          *ResultSummary
 	Messages         []*ResultMessage
 	ContainerResults []ContainerResult
+	podSpec          corev1.PodSpec
 }
 
 // ResultMessage contains a message and a type indicator (success, warning, or error).