From 9db9c7116fc7f85042c103a29f1883a34cce3c69 Mon Sep 17 00:00:00 2001 From: Anbraten <6918444+anbraten@users.noreply.github.com> Date: Wed, 13 Mar 2024 22:41:13 +0100 Subject: [PATCH] Improve security context handling (#3482) --- pipeline/backend/kubernetes/pod.go | 60 ++++++++++++----------- pipeline/backend/kubernetes/pod_test.go | 64 +++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 27 deletions(-) diff --git a/pipeline/backend/kubernetes/pod.go b/pipeline/backend/kubernetes/pod.go index 70413951f..7a70aa5b0 100644 --- a/pipeline/backend/kubernetes/pod.go +++ b/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 + if secCtxConf.RunAsNonRoot { + nonRoot = newBool(true) + } + + if sc != nil { + // only allow to set user if its not root or step is privileged + if sc.RunAsUser != nil && (*sc.RunAsUser != 0 || stepPrivileged) { + user = sc.RunAsUser } - } else if secCtxConf.RunAsNonRoot { - nonRoot = &secCtxConf.RunAsNonRoot // true - } - if sc != nil { - user = sc.RunAsUser - group = sc.RunAsGroup - fsGroup = sc.FSGroup - } + // 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) { diff --git a/pipeline/backend/kubernetes/pod_test.go b/pipeline/backend/kubernetes/pod_test.go index 167ba849e..bf0972743 100644 --- a/pipeline/backend/kubernetes/pod_test.go +++ b/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) +}