From 12da03228ca07d392086e0259a96d9cb5fc3d57b Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Tue, 11 Oct 2022 10:34:46 -0400 Subject: [PATCH] Set HostUsers=false for unprivileged build pods + UserNamespaces If the UserNamespacesStatelessPodsSupport feature is enabled, set HostUsers=false for unprivileged build pods, in addition to the non-standardized annotation that older versions of CRI-O could be configured to accept instead. Signed-off-by: Nalin Dahyabhai --- pkg/build/controller/strategy/custom.go | 3 +- pkg/build/controller/strategy/custom_test.go | 3 + pkg/build/controller/strategy/docker.go | 3 +- pkg/build/controller/strategy/docker_test.go | 3 + pkg/build/controller/strategy/sti.go | 3 +- pkg/build/controller/strategy/sti_test.go | 3 + pkg/build/controller/strategy/util.go | 6 +- pkg/build/controller/strategy/util_test.go | 109 ++++++++++++++++--- pkg/cmd/controller/build.go | 12 +- 9 files changed, 123 insertions(+), 22 deletions(-) diff --git a/pkg/build/controller/strategy/custom.go b/pkg/build/controller/strategy/custom.go index c1c6842cf..dc6e174b4 100644 --- a/pkg/build/controller/strategy/custom.go +++ b/pkg/build/controller/strategy/custom.go @@ -30,6 +30,7 @@ func init() { // CustomBuildStrategy creates a build using a custom builder image. type CustomBuildStrategy struct { + ClearHostUsers bool // use "hostUsers: false" to ask to run in a user namespace because we're "stateless" (i.e., have no volumes which will persist beyond the life of this pod) } // CreateBuildPod creates the pod to be used for the Custom build @@ -138,7 +139,7 @@ func (bs *CustomBuildStrategy) CreateBuildPod(build *buildv1.Build, additionalCA setupBuildCAs(build, pod, additionalCAs, internalRegistryHost) setupContainersStorage(pod, &pod.Spec.Containers[0]) if securityContext == nil || securityContext.Privileged == nil || !*securityContext.Privileged { - setupBuilderAutonsUser(build, strategy.Env, pod) + setupBuilderAutonsUser(build, strategy.Env, bs.ClearHostUsers, pod) setupBuilderDeviceFUSE(pod) } return pod, nil diff --git a/pkg/build/controller/strategy/custom_test.go b/pkg/build/controller/strategy/custom_test.go index 86ef2d854..94d3a775e 100644 --- a/pkg/build/controller/strategy/custom_test.go +++ b/pkg/build/controller/strategy/custom_test.go @@ -290,5 +290,8 @@ func TestCustomCreateBuildPodAutonsUser(t *testing.T) { func(build *buildv1.Build, env corev1.EnvVar) { build.Spec.Strategy.CustomStrategy.Env = append(build.Spec.Strategy.CustomStrategy.Env, env) }, + func(clearHostUsersForUserNS bool) { + strategy.ClearHostUsers = clearHostUsersForUserNS + }, ) } diff --git a/pkg/build/controller/strategy/docker.go b/pkg/build/controller/strategy/docker.go index 45b60de40..747994d19 100644 --- a/pkg/build/controller/strategy/docker.go +++ b/pkg/build/controller/strategy/docker.go @@ -28,6 +28,7 @@ func init() { type DockerBuildStrategy struct { Image string BuildCSIVolumesEnabled bool + ClearHostUsers bool // use "hostUsers: false" to ask to run in a user namespace because we're "stateless" (i.e., have no volumes which will persist beyond the life of this pod) } // CreateBuildPod creates the pod to be used for the Docker build @@ -210,7 +211,7 @@ func (bs *DockerBuildStrategy) CreateBuildPod(build *buildv1.Build, additionalCA setupBuildCAs(build, pod, additionalCAs, internalRegistryHost) setupContainersStorage(pod, &pod.Spec.Containers[0]) if securityContext == nil || securityContext.Privileged == nil || !*securityContext.Privileged { - setupBuilderAutonsUser(build, strategy.Env, pod) + setupBuilderAutonsUser(build, strategy.Env, bs.ClearHostUsers, pod) setupBuilderDeviceFUSE(pod) } setupBlobCache(pod) diff --git a/pkg/build/controller/strategy/docker_test.go b/pkg/build/controller/strategy/docker_test.go index bc46423a4..0cfd01b77 100644 --- a/pkg/build/controller/strategy/docker_test.go +++ b/pkg/build/controller/strategy/docker_test.go @@ -251,5 +251,8 @@ func TestDockerCreateBuildPodAutonsUser(t *testing.T) { func(build *buildv1.Build, env corev1.EnvVar) { build.Spec.Strategy.DockerStrategy.Env = append(build.Spec.Strategy.DockerStrategy.Env, env) }, + func(clearHostUsersForUserNS bool) { + strategy.ClearHostUsers = clearHostUsersForUserNS + }, ) } diff --git a/pkg/build/controller/strategy/sti.go b/pkg/build/controller/strategy/sti.go index efcff1d21..7e94965e9 100644 --- a/pkg/build/controller/strategy/sti.go +++ b/pkg/build/controller/strategy/sti.go @@ -21,6 +21,7 @@ type SourceBuildStrategy struct { Image string SecurityClient securityclient.SecurityV1Interface BuildCSIVolumeseEnabled bool + ClearHostUsers bool // use "hostUsers: false" to ask to run in a user namespace because we're "stateless" (i.e., have no volumes which will persist beyond the life of this pod) } // DefaultDropCaps is the list of capabilities to drop if the current user cannot run as root @@ -217,7 +218,7 @@ func (bs *SourceBuildStrategy) CreateBuildPod(build *buildv1.Build, additionalCA setupBuildCAs(build, pod, additionalCAs, internalRegistryHost) setupContainersStorage(pod, &pod.Spec.Containers[0]) if securityContext == nil || securityContext.Privileged == nil || !*securityContext.Privileged { - setupBuilderAutonsUser(build, strategy.Env, pod) + setupBuilderAutonsUser(build, strategy.Env, bs.ClearHostUsers, pod) setupBuilderDeviceFUSE(pod) } setupBlobCache(pod) diff --git a/pkg/build/controller/strategy/sti_test.go b/pkg/build/controller/strategy/sti_test.go index 7d389c999..3c056b927 100644 --- a/pkg/build/controller/strategy/sti_test.go +++ b/pkg/build/controller/strategy/sti_test.go @@ -317,5 +317,8 @@ func TestS2ICreateBuildPodAutonsUser(t *testing.T) { func(build *buildv1.Build, env corev1.EnvVar) { build.Spec.Strategy.SourceStrategy.Env = append(build.Spec.Strategy.SourceStrategy.Env, env) }, + func(clearHostUsersForUserNS bool) { + strategy.ClearHostUsers = clearHostUsersForUserNS + }, ) } diff --git a/pkg/build/controller/strategy/util.go b/pkg/build/controller/strategy/util.go index abb9bfed1..039b2e31d 100644 --- a/pkg/build/controller/strategy/util.go +++ b/pkg/build/controller/strategy/util.go @@ -13,6 +13,7 @@ import ( kvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/klog/v2" "k8s.io/kubernetes/pkg/apis/policy" + "k8s.io/utils/pointer" buildv1 "github.com/openshift/api/build/v1" "github.com/openshift/library-go/pkg/build/naming" @@ -570,9 +571,12 @@ func setupBuilderDeviceFUSE(pod *corev1.Pod) { // Request that the builder be run in a user namespace, mapped from host ID // ranges chosen by the node. -func setupBuilderAutonsUser(build *buildv1.Build, vars []corev1.EnvVar, pod *corev1.Pod) { +func setupBuilderAutonsUser(build *buildv1.Build, vars []corev1.EnvVar, clearHostUsersForUserNS bool, pod *corev1.Pod) { metav1.SetMetaDataAnnotation(&pod.ObjectMeta, "io.openshift.builder", "") metav1.SetMetaDataAnnotation(&pod.ObjectMeta, "io.kubernetes.cri-o.userns-mode", "auto:size=65536") + if clearHostUsersForUserNS { + pod.Spec.HostUsers = pointer.Bool(false) + } } // setupBuildCAs mounts certificate authorities for the build from a predetermined ConfigMap. diff --git a/pkg/build/controller/strategy/util_test.go b/pkg/build/controller/strategy/util_test.go index 68e94993a..27bdb0148 100644 --- a/pkg/build/controller/strategy/util_test.go +++ b/pkg/build/controller/strategy/util_test.go @@ -820,67 +820,135 @@ type buildPodCreator interface { CreateBuildPod(build *buildv1.Build, additionalCAs map[string]string, internalRegistryHost string) (*corev1.Pod, error) } -func testCreateBuildPodAutonsUser(t *testing.T, build *buildv1.Build, strategy buildPodCreator, addEnv func(build *buildv1.Build, env corev1.EnvVar)) { +func testCreateBuildPodAutonsUser(t *testing.T, build *buildv1.Build, strategy buildPodCreator, addEnv func(build *buildv1.Build, env corev1.EnvVar), clearHostUsersForUserNS func(clearHostUsersForUserNS bool)) { for _, testCase := range []struct { - env string - expectError bool - privileged bool - annotations map[string]string - noAnnotations []string + env string + expectError bool + privileged bool + useUserNamespaceUsers bool + annotations map[string]string + noAnnotations []string + expectHostUsers bool }{ { - env: "", - privileged: true, + env: "", + privileged: true, + useUserNamespaceUsers: false, noAnnotations: []string{ "io.openshift.builder", "io.kubernetes.cri-o.Devices", "io.kubernetes.cri-o.userns-mode", }, + expectHostUsers: true, }, { - env: "BUILD_PRIVILEGED=0", - privileged: false, + env: "", + privileged: true, + useUserNamespaceUsers: true, + noAnnotations: []string{ + "io.openshift.builder", + "io.kubernetes.cri-o.Devices", + "io.kubernetes.cri-o.userns-mode", + }, + expectHostUsers: true, + }, + { + env: "BUILD_PRIVILEGED=0", + privileged: false, + useUserNamespaceUsers: false, annotations: map[string]string{ "io.openshift.builder": "", "io.kubernetes.cri-o.Devices": "/dev/fuse:rwm", "io.kubernetes.cri-o.userns-mode": "auto:size=65536", }, + expectHostUsers: true, // won't set HostUsers: false without the UserNamespacesStatelessPodsSupport feature }, { - env: "BUILD_PRIVILEGED=42", - privileged: true, + env: "BUILD_PRIVILEGED=0", + privileged: false, + useUserNamespaceUsers: true, + annotations: map[string]string{ + "io.openshift.builder": "", + "io.kubernetes.cri-o.Devices": "/dev/fuse:rwm", + "io.kubernetes.cri-o.userns-mode": "auto:size=65536", + }, + expectHostUsers: false, + }, + { + env: "BUILD_PRIVILEGED=42", + privileged: true, + useUserNamespaceUsers: false, noAnnotations: []string{ "io.openshift.builder", "io.kubernetes.cri-o.Devices", "io.kubernetes.cri-o.userns-mode", }, + expectHostUsers: true, }, { - env: "BUILD_PRIVILEGED=false", - privileged: false, + env: "BUILD_PRIVILEGED=42", + privileged: true, + useUserNamespaceUsers: true, + noAnnotations: []string{ + "io.openshift.builder", + "io.kubernetes.cri-o.Devices", + "io.kubernetes.cri-o.userns-mode", + }, + expectHostUsers: true, + }, + { + env: "BUILD_PRIVILEGED=false", + privileged: false, + useUserNamespaceUsers: false, + annotations: map[string]string{ + "io.openshift.builder": "", + "io.kubernetes.cri-o.Devices": "/dev/fuse:rwm", + "io.kubernetes.cri-o.userns-mode": "auto:size=65536", + }, + expectHostUsers: true, // won't set HostUsers: false without the UserNamespacesStatelessPodsSupport feature + }, + { + env: "BUILD_PRIVILEGED=false", + privileged: false, + useUserNamespaceUsers: true, annotations: map[string]string{ "io.openshift.builder": "", "io.kubernetes.cri-o.Devices": "/dev/fuse:rwm", "io.kubernetes.cri-o.userns-mode": "auto:size=65536", }, + expectHostUsers: false, }, { - env: "BUILD_PRIVILEGED=true", - privileged: true, + env: "BUILD_PRIVILEGED=true", + privileged: true, + useUserNamespaceUsers: false, noAnnotations: []string{ "io.openshift.builder", "io.kubernetes.cri-o.Devices", "io.kubernetes.cri-o.userns-mode", }, + expectHostUsers: true, + }, + { + env: "BUILD_PRIVILEGED=true", + privileged: true, + useUserNamespaceUsers: true, + noAnnotations: []string{ + "io.openshift.builder", + "io.kubernetes.cri-o.Devices", + "io.kubernetes.cri-o.userns-mode", + }, + expectHostUsers: true, }, } { - t.Run(testCase.env, func(t *testing.T) { + t.Run(testCase.env+fmt.Sprintf(",useUserNamespaceUsers=%t", testCase.useUserNamespaceUsers), func(t *testing.T) { build := build.DeepCopy() for _, envVar := range strings.Split(testCase.env, ":") { if env := strings.SplitN(envVar, "=", 2); len(env) > 1 { addEnv(build, corev1.EnvVar{Name: env[0], Value: env[1]}) } } + clearHostUsersForUserNS(testCase.useUserNamespaceUsers) actual, err := strategy.CreateBuildPod(build, nil, testInternalRegistryHost) if err != nil { t.Errorf("Unexpected error: %v", err) @@ -915,6 +983,13 @@ func testCreateBuildPodAutonsUser(t *testing.T, build *buildv1.Build, strategy b t.Errorf("Expected no %q annotation, but got one", annotation) } } + hostUsers := true + if actual.Spec.HostUsers != nil { + hostUsers = *actual.Spec.HostUsers + } + if hostUsers != testCase.expectHostUsers { + t.Errorf("Expected to see pod HostUsers=%t, got %t (%#v)", testCase.expectHostUsers, hostUsers, actual.Spec.HostUsers) + } }) } } diff --git a/pkg/cmd/controller/build.go b/pkg/cmd/controller/build.go index 07168baf1..970726acf 100644 --- a/pkg/cmd/controller/build.go +++ b/pkg/cmd/controller/build.go @@ -1,10 +1,12 @@ package controller import ( + "fmt" "strings" clientset "k8s.io/client-go/kubernetes" "k8s.io/klog/v2" + "k8s.io/kubernetes/pkg/features" buildclient "github.com/openshift/client-go/build/clientset/versioned" buildcontroller "github.com/openshift/openshift-controller-manager/pkg/build/controller/build" @@ -55,12 +57,16 @@ func RunBuildController(ctx *ControllerContext) (bool, error) { fg := ctx.OpenshiftControllerConfig.FeatureGates csiVolumesEnabled := false + clearHostUsersForUserNS := false if fg != nil { for _, v := range fg { v = strings.TrimSpace(v) if v == "BuildCSIVolumes=true" { csiVolumesEnabled = true } + if v == fmt.Sprintf("%s=true", string(features.UserNamespacesStatelessPodsSupport)) { + clearHostUsersForUserNS = true + } } } @@ -85,13 +91,17 @@ func RunBuildController(ctx *ControllerContext) (bool, error) { DockerBuildStrategy: &buildstrategy.DockerBuildStrategy{ Image: imageTemplate.ExpandOrDie("docker-builder"), BuildCSIVolumesEnabled: csiVolumesEnabled, + ClearHostUsers: clearHostUsersForUserNS, }, SourceBuildStrategy: &buildstrategy.SourceBuildStrategy{ Image: imageTemplate.ExpandOrDie("docker-builder"), SecurityClient: securityClient.SecurityV1(), BuildCSIVolumeseEnabled: csiVolumesEnabled, + ClearHostUsers: clearHostUsersForUserNS, + }, + CustomBuildStrategy: &buildstrategy.CustomBuildStrategy{ + ClearHostUsers: clearHostUsersForUserNS, }, - CustomBuildStrategy: &buildstrategy.CustomBuildStrategy{}, BuildDefaults: builddefaults.BuildDefaults{Config: ctx.OpenshiftControllerConfig.Build.BuildDefaults}, BuildOverrides: buildoverrides.BuildOverrides{Config: ctx.OpenshiftControllerConfig.Build.BuildOverrides}, InternalRegistryHostname: ctx.OpenshiftControllerConfig.DockerPullSecret.InternalRegistryHostname,