Skip to content

Commit

Permalink
fix for RunAsNonRoot check (#2557)
Browse files Browse the repository at this point in the history 
* fix for RunAsNonRoot check

* addressing comments from Brandon

* Addressing comments from Gonzalo
  • Loading branch information
@edcdavid
edcdavid authored Nov 14, 2024
1 parent b115e81 commit 71b39e4
Show file tree
Hide file tree
Showing 6 changed files with 246 additions and 46 deletions.
10 changes: 7 additions & 3 deletions pkg/provider/containers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/go-logr/logr"
"github.com/go-logr/stdr"
"github.com/redhat-best-practices-for-k8s/certsuite/internal/log"
"github.com/redhat-best-practices-for-k8s/certsuite/pkg/stringhelper"
corev1 "k8s.io/api/core/v1"

"github.com/redhat-openshift-ecosystem/openshift-preflight/artifacts"
Expand Down Expand Up @@ -194,9 +195,12 @@ func (c *Container) IsReadOnlyRootFilesystem(logger *log.Logger) bool {
return *c.Container.SecurityContext.ReadOnlyRootFilesystem
}

func (c *Container) IsContainerRunAsNonRoot() bool {
func (c *Container) IsContainerRunAsNonRoot(podRunAsNonRoot *bool) (isContainerRunAsNonRoot bool, reason string) {
if c.SecurityContext != nil && c.SecurityContext.RunAsNonRoot != nil {
return *c.SecurityContext.RunAsNonRoot
return *c.SecurityContext.RunAsNonRoot, fmt.Sprintf("RunAsNonRoot is set to %t at the container level, overriding a %v value defined at pod level.", *c.SecurityContext.RunAsNonRoot, stringhelper.BoolToString(podRunAsNonRoot))
}
return false
if podRunAsNonRoot != nil {
return *podRunAsNonRoot, fmt.Sprintf("RunAsNonRoot is set to nil at container level and inheriting a %t value from the pod level RunAsNonRoot setting.", *podRunAsNonRoot)
}
return false, "RunAsNonRoot set to nil at pod and container level"
}
47 changes: 41 additions & 6 deletions pkg/provider/containers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,9 +249,11 @@ func TestIsContainerRunAsNonRoot(t *testing.T) {
trueVal := true
falseVal := false
tests := []struct {
name string
container Container
expected bool
name string
container Container
podDefault *bool
expected bool
expectedReason string
}{
{
name: "Container set to run as non-root",
Expand All @@ -262,7 +264,9 @@ func TestIsContainerRunAsNonRoot(t *testing.T) {
},
},
},
expected: true,
podDefault: &falseVal,
expected: true,
expectedReason: "RunAsNonRoot is set to true at the container level, overriding a false value defined at pod level.",
},
{
name: "Container set to not run as non-root",
Expand All @@ -273,16 +277,47 @@ func TestIsContainerRunAsNonRoot(t *testing.T) {
},
},
},
expected: false,
podDefault: &trueVal,
expected: false,
expectedReason: "RunAsNonRoot is set to false at the container level, overriding a true value defined at pod level.",
},
{
name: "Container set to not run as non-root",
container: Container{
Container: &corev1.Container{
SecurityContext: &corev1.SecurityContext{
RunAsNonRoot: nil,
},
},
},
podDefault: &falseVal,
expected: false,
expectedReason: "RunAsNonRoot is set to nil at container level and inheriting a false value from the pod level RunAsNonRoot setting.",
},
{
name: "nil at pod and true at container",
container: Container{
Container: &corev1.Container{
SecurityContext: &corev1.SecurityContext{
RunAsNonRoot: &trueVal,
},
},
},
podDefault: nil,
expected: true,
expectedReason: "RunAsNonRoot is set to true at the container level, overriding a nil value defined at pod level.",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := tt.container.IsContainerRunAsNonRoot()
result, reason := tt.container.IsContainerRunAsNonRoot(tt.podDefault)
if result != tt.expected {
t.Errorf("expected %v, got %v", tt.expected, result)
}
if reason != tt.expectedReason {
t.Errorf("expectedReason %v, got %v", tt.expectedReason, reason)
}
})
}
}
29 changes: 23 additions & 6 deletions pkg/provider/pods.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -502,14 +502,31 @@ func (p *Pod) IsRunAsUserID(uid int64) bool {
return *p.Pod.Spec.SecurityContext.RunAsUser == uid
}

func (p *Pod) IsRunAsNonRoot() bool {
// Check pod-level security context
// Returns the list of containers that have the RunAsNonRoot SCC parameter set to false
// The RunAsNonRoot parameter is checked first at the pod level and acts as a default value
// for the container configuration, if it is not present.
// The RunAsNonRoot parameter is checked next at the container level.
// See: https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-the-security-context-for-a-container
func (p *Pod) GetRunAsNonRootFalseContainers(knownContainersToSkip map[string]bool) (nonCompliantContainers []*Container, nonComplianceReason []string) {
// Check pod-level security context this will be set by default for containers
// If not already configured at the container level
var podRunAsNonRoot *bool
if p.Pod.Spec.SecurityContext != nil && p.Pod.Spec.SecurityContext.RunAsNonRoot != nil {
return *p.Pod.Spec.SecurityContext.RunAsNonRoot
podRunAsNonRoot = p.Pod.Spec.SecurityContext.RunAsNonRoot
}

// If neither container-level nor pod-level security context is set, fail
return false
// Check each container for the RunAsNonRoot parameter.
// If it is not present, the pod value applies
for _, cut := range p.Containers {
if knownContainersToSkip[cut.Name] {
continue
}
if isRunAsNonRoot, reason := cut.IsContainerRunAsNonRoot(podRunAsNonRoot); !isRunAsNonRoot {
// found a container with RunAsNonRoot set to false
nonCompliantContainers = append(nonCompliantContainers, cut)
nonComplianceReason = append(nonComplianceReason, reason)
}
}
return nonCompliantContainers, nonComplianceReason
}

// Get the list of top owners of pods
Expand Down
165 changes: 157 additions & 8 deletions pkg/provider/pods_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package provider

import (
"errors"
"reflect"
"strings"
"testing"

Expand Down Expand Up @@ -520,12 +521,86 @@ func TestIsRunAsUserID(t *testing.T) {

func TestIsRunAsNonRoot(t *testing.T) {
tests := []struct {
name string
pod *Pod
expected bool
name string
pod *Pod
wantNonCompliantContainers []*Container
wantNonComplianceReason []string
}{
{
name: "All containers and pod set to run as non-root",
pod: &Pod{
Pod: &corev1.Pod{
Spec: corev1.PodSpec{
SecurityContext: &corev1.PodSecurityContext{
RunAsNonRoot: boolPtr(true),
},
},
},
Containers: []*Container{
{
Container: &corev1.Container{
SecurityContext: &corev1.SecurityContext{
RunAsNonRoot: boolPtr(true),
},
},
},
},
},
},
{
name: "One container with RunAsNonRoot set to false",
pod: &Pod{
Pod: &corev1.Pod{
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
SecurityContext: &corev1.SecurityContext{
RunAsNonRoot: boolPtr(true),
},
},
{
SecurityContext: &corev1.SecurityContext{
RunAsNonRoot: boolPtr(false),
},
},
},
SecurityContext: &corev1.PodSecurityContext{
RunAsNonRoot: boolPtr(true),
},
},
},
Containers: []*Container{
{
Container: &corev1.Container{
SecurityContext: &corev1.SecurityContext{
RunAsNonRoot: boolPtr(true),
},
},
},
{
Container: &corev1.Container{
SecurityContext: &corev1.SecurityContext{
RunAsNonRoot: boolPtr(false),
},
},
},
},
},
wantNonCompliantContainers: []*Container{
{
Container: &corev1.Container{
SecurityContext: &corev1.SecurityContext{
RunAsNonRoot: boolPtr(false),
},
},
},
},
wantNonComplianceReason: []string{
"RunAsNonRoot is set to false at the container level, overriding a true value defined at pod level.",
},
},
{
name: "One container with RunAsNonRoot set to false, nil in pod",
pod: &Pod{
Pod: &corev1.Pod{
Spec: corev1.PodSpec{
Expand All @@ -535,14 +610,86 @@ func TestIsRunAsNonRoot(t *testing.T) {
RunAsNonRoot: boolPtr(true),
},
},
{
SecurityContext: &corev1.SecurityContext{
RunAsNonRoot: boolPtr(false),
},
},
},
SecurityContext: &corev1.PodSecurityContext{
RunAsNonRoot: nil,
},
},
},
Containers: []*Container{
{
Container: &corev1.Container{
SecurityContext: &corev1.SecurityContext{
RunAsNonRoot: boolPtr(true),
},
},
},
{
Container: &corev1.Container{
SecurityContext: &corev1.SecurityContext{
RunAsNonRoot: boolPtr(false),
},
},
},
},
},
wantNonCompliantContainers: []*Container{
{
Container: &corev1.Container{
SecurityContext: &corev1.SecurityContext{
RunAsNonRoot: boolPtr(false),
},
},
},
},
wantNonComplianceReason: []string{
"RunAsNonRoot is set to false at the container level, overriding a nil value defined at pod level.",
},
},
{
name: "One container with RunAsNonRoot non set (nil)",
pod: &Pod{
Pod: &corev1.Pod{
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
SecurityContext: &corev1.SecurityContext{
RunAsNonRoot: boolPtr(true),
},
},
{
SecurityContext: &corev1.SecurityContext{
RunAsNonRoot: nil,
},
},
},
SecurityContext: &corev1.PodSecurityContext{
RunAsNonRoot: boolPtr(true),
},
},
},
Containers: []*Container{
{
Container: &corev1.Container{
SecurityContext: &corev1.SecurityContext{
RunAsNonRoot: boolPtr(true),
},
},
},
{
Container: &corev1.Container{
SecurityContext: &corev1.SecurityContext{
RunAsNonRoot: nil,
},
},
},
},
},
expected: true,
},
{
name: "No containers, pod set to run as non-root",
Expand All @@ -555,15 +702,17 @@ func TestIsRunAsNonRoot(t *testing.T) {
},
},
},
expected: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := tt.pod.IsRunAsNonRoot()
if result != tt.expected {
t.Errorf("expected %v, got %v", tt.expected, result)
gotNonCompliantContainers, gotNonComplianceReason := tt.pod.GetRunAsNonRootFalseContainers(map[string]bool{})
if !reflect.DeepEqual(gotNonCompliantContainers, tt.wantNonCompliantContainers) {
t.Errorf("Pod.GetRunAsNonRootFalseContainers() gotNonCompliantContainers = %v, want %v", gotNonCompliantContainers, tt.wantNonCompliantContainers)
}
if !reflect.DeepEqual(gotNonComplianceReason, tt.wantNonComplianceReason) {
t.Errorf("Pod.GetRunAsNonRootFalseContainers() gotNonComplianceReason = %v, want %v", gotNonComplianceReason, tt.wantNonComplianceReason)
}
})
}
Expand Down
8 changes: 8 additions & 0 deletions pkg/stringhelper/stringhelper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package stringhelper

import (
"fmt"
"strings"
)

Expand Down Expand Up @@ -65,3 +66,10 @@ func RemoveEmptyStrings(s []string) []string {
}
return r
}

func BoolToString(b *bool) string {
if b == nil {
return "nil"
}
return fmt.Sprintf("%t", *b)
}
Loading

0 comments on commit 71b39e4

Please sign in to comment.