Improve security context handling (#3482)

Author: anbraten

pipeline/backend/kubernetes/pod.go

@@ -122,7 +122,7 @@ func podSpec(step *types.Step, config *config, options BackendOptions) (v1.PodSp
 		HostAliases:        hostAliases(step.ExtraHosts),
 		NodeSelector:       nodeSelector(options.NodeSelector, step.Environment["CI_SYSTEM_PLATFORM"]),
 		Tolerations:        tolerations(options.Tolerations),
-		SecurityContext:    podSecurityContext(options.SecurityContext, config.SecurityContext),
+		SecurityContext:    podSecurityContext(options.SecurityContext, config.SecurityContext, step.Privileged),
 	}
 	spec.Volumes, err = volumes(step.Volumes)
 	if err != nil {
@@ -339,7 +339,7 @@ func toleration(backendToleration Toleration) v1.Toleration {
 	}
 }
 
-func podSecurityContext(sc *SecurityContext, secCtxConf SecurityContextConfig) *v1.PodSecurityContext {
+func podSecurityContext(sc *SecurityContext, secCtxConf SecurityContextConfig, stepPrivileged bool) *v1.PodSecurityContext {
 	var (
 		nonRoot *bool
 		user    *int64
@@ -348,21 +348,31 @@ func podSecurityContext(sc *SecurityContext, secCtxConf SecurityContextConfig) *
 		seccomp *v1.SeccompProfile
 	)
 
-	if sc != nil && sc.RunAsNonRoot != nil {
-		if *sc.RunAsNonRoot {
-			nonRoot = sc.RunAsNonRoot // true
-		}
-	} else if secCtxConf.RunAsNonRoot {
-		nonRoot = &secCtxConf.RunAsNonRoot // true
+	if secCtxConf.RunAsNonRoot {
+		nonRoot = newBool(true)
 	}
 
 	if sc != nil {
-		user = sc.RunAsUser
-		group = sc.RunAsGroup
-		fsGroup = sc.FSGroup
-	}
+		// only allow to set user if its not root or step is privileged
+		if sc.RunAsUser != nil && (*sc.RunAsUser != 0 || stepPrivileged) {
+			user = sc.RunAsUser
+		}
+
+		// only allow to set group if its not root or step is privileged
+		if sc.RunAsGroup != nil && (*sc.RunAsGroup != 0 || stepPrivileged) {
+			group = sc.RunAsGroup
+		}
+
+		// only allow to set fsGroup if its not root or step is privileged
+		if sc.FSGroup != nil && (*sc.FSGroup != 0 || stepPrivileged) {
+			fsGroup = sc.FSGroup
+		}
+
+		// only allow to set nonRoot if it's not set globally already
+		if nonRoot == nil && sc.RunAsNonRoot != nil {
+			nonRoot = sc.RunAsNonRoot
+		}
 
-	if sc != nil {
 		seccomp = seccompProfile(sc.SeccompProfile)
 	}
 
@@ -398,23 +408,19 @@ func seccompProfile(scp *SecProfile) *v1.SeccompProfile {
 }
 
 func containerSecurityContext(sc *SecurityContext, stepPrivileged bool) *v1.SecurityContext {
-	var privileged *bool
-
-	if sc != nil && sc.Privileged != nil && *sc.Privileged {
-		privileged = sc.Privileged // true
-	} else if stepPrivileged {
-		privileged = &stepPrivileged // true
-	}
-
-	if privileged == nil {
+	if !stepPrivileged {
 		return nil
 	}
 
-	securityContext := &v1.SecurityContext{
-		Privileged: privileged,
+	if sc != nil && sc.Privileged != nil && *sc.Privileged {
+		securityContext := &v1.SecurityContext{
+			Privileged: newBool(true),
+		}
+		log.Trace().Msgf("container security context that will be used: %v", securityContext)
+		return securityContext
 	}
-	log.Trace().Msgf("container security context that will be used: %v", securityContext)
-	return securityContext
+
+	return nil
 }
 
 func apparmorAnnotation(containerName string, scp *SecProfile) (*string, *string) {

pipeline/backend/kubernetes/pod_test.go

@@ -20,6 +20,7 @@ import (
 
 	"github.com/kinbiko/jsonassert"
 	"github.com/stretchr/testify/assert"
+	v1 "k8s.io/api/core/v1"
 
 	"go.woodpecker-ci.org/woodpecker/v2/pipeline/backend/types"
 )
@@ -348,3 +349,66 @@ func TestFullPod(t *testing.T) {
 	ja := jsonassert.New(t)
 	ja.Assertf(string(podJSON), expected)
 }
+
+func TestPodPrivilege(t *testing.T) {
+	createTestPod := func(stepPrivileged, globalRunAsRoot bool, secCtx SecurityContext) (*v1.Pod, error) {
+		return mkPod(&types.Step{
+			Name:       "go-test",
+			Image:      "golang:1.16",
+			Privileged: stepPrivileged,
+		}, &config{
+			Namespace:       "woodpecker",
+			SecurityContext: SecurityContextConfig{RunAsNonRoot: globalRunAsRoot},
+		}, "wp-01he8bebctabr3kgk0qj36d2me-0", "linux/amd64", BackendOptions{
+			SecurityContext: &secCtx,
+		})
+	}
+
+	// securty context is requesting user and group 101 (non-root)
+	secCtx := SecurityContext{
+		RunAsUser:  newInt64(101),
+		RunAsGroup: newInt64(101),
+		FSGroup:    newInt64(101),
+	}
+	pod, err := createTestPod(false, false, secCtx)
+	assert.NoError(t, err)
+	assert.Equal(t, int64(101), *pod.Spec.SecurityContext.RunAsUser)
+	assert.Equal(t, int64(101), *pod.Spec.SecurityContext.RunAsGroup)
+	assert.Equal(t, int64(101), *pod.Spec.SecurityContext.FSGroup)
+
+	// securty context is requesting root, but step is not privileged
+	secCtx = SecurityContext{
+		RunAsUser:  newInt64(0),
+		RunAsGroup: newInt64(0),
+		FSGroup:    newInt64(0),
+	}
+	pod, err = createTestPod(false, false, secCtx)
+	assert.NoError(t, err)
+	assert.Nil(t, pod.Spec.SecurityContext)
+	assert.Nil(t, pod.Spec.Containers[0].SecurityContext)
+
+	// step is not privileged, but security context is requesting privileged
+	secCtx = SecurityContext{
+		Privileged: newBool(true),
+	}
+	pod, err = createTestPod(false, false, secCtx)
+	assert.NoError(t, err)
+	assert.Nil(t, pod.Spec.SecurityContext)
+	assert.Nil(t, pod.Spec.Containers[0].SecurityContext)
+
+	// step is privileged and security context is requesting privileged
+	secCtx = SecurityContext{
+		Privileged: newBool(true),
+	}
+	pod, err = createTestPod(true, false, secCtx)
+	assert.NoError(t, err)
+	assert.Equal(t, true, *pod.Spec.Containers[0].SecurityContext.Privileged)
+
+	// global runAsNonRoot is true and override is requested value by security context
+	secCtx = SecurityContext{
+		RunAsNonRoot: newBool(false),
+	}
+	pod, err = createTestPod(false, true, secCtx)
+	assert.NoError(t, err)
+	assert.Equal(t, true, *pod.Spec.SecurityContext.RunAsNonRoot)
+}