diff --git a/CHANGELOG.md b/CHANGELOG.md index 14e6542a29..62f48d5c2a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,12 @@ ## UNRELEASED BUG FIXES: +* Connect: Use `runAsNonRoot: false` for connect-init's container when tproxy is enabled. [[GH-493](https://github.com/hashicorp/consul-k8s/pull/493)] + +BREAKING CHANGES: +* Connect: Add a security context to the init copy container and the envoy sidecar and ensure they + do not run as root. If a pod container shares the same `runAsUser` (5995) as Envoy an error is returned + on scheduling. [[GH-493](https://github.com/hashicorp/consul-k8s/pull/493)] * CRDs: Fix a bug where the `config` field in `ProxyDefaults` CR was not synced to Consul because `apiextensions.k8s.io/v1` requires CRD spec to have structured schema. [[GH-495](https://github.com/hashicorp/consul-k8s/pull/495)] diff --git a/connect-inject/container_init.go b/connect-inject/container_init.go index 40d014e18b..9204b2a145 100644 --- a/connect-inject/container_init.go +++ b/connect-inject/container_init.go @@ -13,6 +13,8 @@ const ( InjectInitCopyContainerName = "copy-consul-bin" InjectInitContainerName = "consul-connect-inject-init" rootUserAndGroupID = 0 + envoyUserAndGroupID = 5995 + copyContainerUserAndGroupID = 5996 netAdminCapability = "NET_ADMIN" ) @@ -37,6 +39,8 @@ type initContainerCommandData struct { PrometheusScrapePath string // PrometheusBackendPort configures where the listener on Envoy will point to. PrometheusBackendPort string + // EnvoyUID is the Linux user id that will be used when tproxy is enabled. + EnvoyUID int // EnableTransparentProxy configures this init container to run in transparent proxy mode, // i.e. run consul connect redirect-traffic command and add the required privileges to the @@ -60,6 +64,13 @@ func (h *Handler) containerInitCopyContainer() corev1.Container { }, }, Command: []string{"/bin/sh", "-ec", cmd}, + SecurityContext: &corev1.SecurityContext{ + // Set RunAsUser because the default user for the consul container is root and we want to run non-root. + RunAsUser: pointerToInt64(copyContainerUserAndGroupID), + RunAsGroup: pointerToInt64(copyContainerUserAndGroupID), + RunAsNonRoot: pointerToBool(true), + ReadOnlyRootFilesystem: pointerToBool(true), + }, } } @@ -78,6 +89,7 @@ func (h *Handler) containerInit(pod corev1.Pod, k8sNamespace string) (corev1.Con NamespaceMirroringEnabled: h.EnableK8SNSMirroring, ConsulCACert: h.ConsulCACert, EnableTransparentProxy: tproxyEnabled, + EnvoyUID: envoyUserAndGroupID, } if data.AuthMethod != "" { @@ -170,6 +182,8 @@ func (h *Handler) containerInit(pod corev1.Pod, k8sNamespace string) (corev1.Con container.SecurityContext = &corev1.SecurityContext{ RunAsUser: pointerToInt64(rootUserAndGroupID), RunAsGroup: pointerToInt64(rootUserAndGroupID), + // RunAsNonRoot overrides any setting in the Pod so that we can still run as root here as required. + RunAsNonRoot: pointerToBool(false), Capabilities: &corev1.Capabilities{ Add: []corev1.Capability{netAdminCapability}, }, @@ -194,6 +208,11 @@ func pointerToInt64(i int64) *int64 { return &i } +// pointerToBool takes a bool and returns a pointer to it. +func pointerToBool(b bool) *bool { + return &b +} + // initContainerCommandTpl is the template for the command executed by // the init container. const initContainerCommandTpl = ` @@ -254,6 +273,6 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ -namespace="{{ .ConsulNamespace }}" \ {{- end }} -proxy-id="$(cat /consul/connect-inject/proxyid)" \ - -proxy-uid=0 + -proxy-uid={{ .EnvoyUID }} {{- end }} ` diff --git a/connect-inject/container_init_test.go b/connect-inject/container_init_test.go index 04adfcd8bd..6299801fca 100644 --- a/connect-inject/container_init_test.go +++ b/connect-inject/container_init_test.go @@ -192,10 +192,11 @@ func TestHandlerContainerInit_transparentProxy(t *testing.T) { Capabilities: &corev1.Capabilities{ Add: []corev1.Capability{netAdminCapability}, }, + RunAsNonRoot: pointerToBool(false), } expectedCmd := `/consul/connect-inject/consul connect redirect-traffic \ -proxy-id="$(cat /consul/connect-inject/proxyid)" \ - -proxy-uid=0` + -proxy-uid=5995` container, err := h.containerInit(*pod, k8sNamespace) require.NoError(t, err) actualCmd := strings.Join(container.Command, " ") @@ -248,7 +249,6 @@ func TestHandlerContainerInit_namespacesEnabled(t *testing.T) { Pod func(*corev1.Pod) *corev1.Pod Handler Handler Cmd string // Strings.Contains test - CmdNot string // Not contains }{ { "whole template, default namespace", @@ -271,7 +271,6 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ -proxy-id="$(cat /consul/connect-inject/proxyid)" \ -namespace="default" \ -bootstrap > /consul/connect-inject/envoy-bootstrap.yaml`, - "", }, { @@ -295,7 +294,6 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ -proxy-id="$(cat /consul/connect-inject/proxyid)" \ -namespace="non-default" \ -bootstrap > /consul/connect-inject/envoy-bootstrap.yaml`, - "", }, { @@ -325,7 +323,6 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ -token-file="/consul/connect-inject/acl-token" \ -namespace="non-default" \ -bootstrap > /consul/connect-inject/envoy-bootstrap.yaml`, - "", }, { "Whole template, auth method, non-default namespace, mirroring enabled", @@ -355,7 +352,6 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ -token-file="/consul/connect-inject/acl-token" \ -namespace="k8snamespace" \ -bootstrap > /consul/connect-inject/envoy-bootstrap.yaml`, - "", }, { "whole template, default namespace, tproxy enabled", @@ -384,8 +380,7 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ /consul/connect-inject/consul connect redirect-traffic \ -namespace="default" \ -proxy-id="$(cat /consul/connect-inject/proxyid)" \ - -proxy-uid=0`, - "", + -proxy-uid=5995`, }, { @@ -415,8 +410,7 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ /consul/connect-inject/consul connect redirect-traffic \ -namespace="non-default" \ -proxy-id="$(cat /consul/connect-inject/proxyid)" \ - -proxy-uid=0`, - "", + -proxy-uid=5995`, }, { @@ -453,8 +447,7 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ /consul/connect-inject/consul connect redirect-traffic \ -namespace="k8snamespace" \ -proxy-id="$(cat /consul/connect-inject/proxyid)" \ - -proxy-uid=0`, - "", + -proxy-uid=5995`, }, } @@ -463,14 +456,10 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ require := require.New(t) h := tt.Handler - container, err := h.containerInit(*tt.Pod(minimal()), k8sNamespace) require.NoError(err) actual := strings.Join(container.Command, " ") - require.Equal(actual, tt.Cmd) - if tt.CmdNot != "" { - require.NotContains(actual, tt.CmdNot) - } + require.Equal(tt.Cmd, actual) }) } } @@ -598,16 +587,18 @@ func TestHandlerContainerInit_Resources(t *testing.T) { }, container.Resources) } -// Test that the init copy container has the correct command. +// Test that the init copy container has the correct command and SecurityContext. func TestHandlerContainerInitCopyContainer(t *testing.T) { require := require.New(t) h := Handler{} container := h.containerInitCopyContainer() + expectedSecurityContext := &corev1.SecurityContext{ + RunAsUser: pointerToInt64(copyContainerUserAndGroupID), + RunAsGroup: pointerToInt64(copyContainerUserAndGroupID), + RunAsNonRoot: pointerToBool(true), + ReadOnlyRootFilesystem: pointerToBool(true), + } + require.Equal(container.SecurityContext, expectedSecurityContext) actual := strings.Join(container.Command, " ") require.Contains(actual, `cp /bin/consul /consul/connect-inject/consul`) } - -// pointerToBool takes a boolean and returns a pointer to it. -func pointerToBool(b bool) *bool { - return &b -} diff --git a/connect-inject/envoy_sidecar.go b/connect-inject/envoy_sidecar.go index 111f48d40a..7b9495db32 100644 --- a/connect-inject/envoy_sidecar.go +++ b/connect-inject/envoy_sidecar.go @@ -10,7 +10,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" ) -func (h *Handler) envoySidecar(pod corev1.Pod, k8sNamespace string) (corev1.Container, error) { +func (h *Handler) envoySidecar(pod corev1.Pod) (corev1.Container, error) { resources, err := h.envoySidecarResources(pod) if err != nil { return corev1.Container{}, err @@ -21,6 +21,21 @@ func (h *Handler) envoySidecar(pod corev1.Pod, k8sNamespace string) (corev1.Cont return corev1.Container{}, err } + if pod.Spec.SecurityContext != nil { + // User container and Envoy container cannot have the same UID. + if pod.Spec.SecurityContext.RunAsUser != nil && *pod.Spec.SecurityContext.RunAsUser == envoyUserAndGroupID { + return corev1.Container{}, fmt.Errorf("pod security context cannot have the same uid as envoy: %v", envoyUserAndGroupID) + } + } + // Ensure that none of the user's containers have the same UID as Envoy. At this point in injection the handler + // has only injected init containers so all containers defined in pod.Spec.Containers are from the user. + for _, c := range pod.Spec.Containers { + // User container and Envoy container cannot have the same UID. + if c.SecurityContext != nil && c.SecurityContext.RunAsUser != nil && *c.SecurityContext.RunAsUser == envoyUserAndGroupID { + return corev1.Container{}, fmt.Errorf("container %q has runAsUser set to the same uid %q as envoy which is not allowed", c.Name, envoyUserAndGroupID) + } + } + container := corev1.Container{ Name: "envoy-sidecar", Image: h.ImageEnvoy, @@ -40,6 +55,12 @@ func (h *Handler) envoySidecar(pod corev1.Pod, k8sNamespace string) (corev1.Cont }, }, Command: cmd, + SecurityContext: &corev1.SecurityContext{ + RunAsUser: pointerToInt64(envoyUserAndGroupID), + RunAsGroup: pointerToInt64(envoyUserAndGroupID), + RunAsNonRoot: pointerToBool(true), + ReadOnlyRootFilesystem: pointerToBool(true), + }, } return container, nil } diff --git a/connect-inject/envoy_sidecar_test.go b/connect-inject/envoy_sidecar_test.go index 5aacef3ee6..a314cf0b0c 100644 --- a/connect-inject/envoy_sidecar_test.go +++ b/connect-inject/envoy_sidecar_test.go @@ -1,6 +1,7 @@ package connectinject import ( + "fmt" "testing" "github.com/stretchr/testify/require" @@ -27,7 +28,7 @@ func TestHandlerEnvoySidecar(t *testing.T) { }, }, } - container, err := h.envoySidecar(pod, k8sNamespace) + container, err := h.envoySidecar(pod) require.NoError(err) require.Equal(container.Command, []string{ "envoy", @@ -42,6 +43,56 @@ func TestHandlerEnvoySidecar(t *testing.T) { }) } +// Test that if the user specifies a pod security context with the same uid as `envoyUserAndGroupID` that we return +// an error to the handler. +func TestHandlerEnvoySidecar_FailsWithDuplicatePodSecurityContextUID(t *testing.T) { + require := require.New(t) + h := Handler{} + pod := corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "web", + }, + }, + SecurityContext: &corev1.PodSecurityContext{ + RunAsUser: pointerToInt64(envoyUserAndGroupID), + }, + }, + } + _, err := h.envoySidecar(pod) + require.Error(err, fmt.Sprintf("pod security context cannot have the same uid as envoy: %v", envoyUserAndGroupID)) +} + +// Test that if the user specifies a container with security context with the same uid as `envoyUserAndGroupID` +// that we return an error to the handler. +func TestHandlerEnvoySidecar_FailsWithDuplicateContainerSecurityContextUID(t *testing.T) { + require := require.New(t) + h := Handler{} + pod := corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "web", + // Setting RunAsUser: 1 should succeed. + SecurityContext: &corev1.SecurityContext{ + RunAsUser: pointerToInt64(1), + }, + }, + { + Name: "app", + // Setting RunAsUser: 5995 should fail. + SecurityContext: &corev1.SecurityContext{ + RunAsUser: pointerToInt64(envoyUserAndGroupID), + }, + }, + }, + }, + } + _, err := h.envoySidecar(pod) + require.Error(err, fmt.Sprintf("container %q has runAsUser set to the same uid %q as envoy which is not allowed", pod.Spec.Containers[1].Name, envoyUserAndGroupID)) +} + // Test that we can pass extra args to envoy via the extraEnvoyArgs flag // or via pod annotations. When arguments are passed in both ways, the // arguments set via pod annotations are used. @@ -126,7 +177,7 @@ func TestHandlerEnvoySidecar_EnvoyExtraArgs(t *testing.T) { EnvoyExtraArgs: tc.envoyExtraArgs, } - c, err := h.envoySidecar(*tc.pod, k8sNamespace) + c, err := h.envoySidecar(*tc.pod) require.NoError(t, err) require.Equal(t, tc.expectedContainerCommand, c.Command) }) @@ -300,7 +351,7 @@ func TestHandlerEnvoySidecar_Resources(t *testing.T) { }, }, } - container, err := c.handler.envoySidecar(pod, k8sNamespace) + container, err := c.handler.envoySidecar(pod) if c.expErr != "" { require.NotNil(err) require.Contains(err.Error(), c.expErr) diff --git a/connect-inject/handler.go b/connect-inject/handler.go index f6ab87908c..b98da8d4b2 100644 --- a/connect-inject/handler.go +++ b/connect-inject/handler.go @@ -191,8 +191,7 @@ func (h *Handler) Handle(_ context.Context, req admission.Request) admission.Res container.Env = append(container.Env, containerEnvVars...) } - // TODO: rename both of these initcontainers appropriately - // Add the consul-init container + // Add the init container which copies the Consul binary to /consul/connect-inject/. initCopyContainer := h.containerInitCopyContainer() pod.Spec.InitContainers = append(pod.Spec.InitContainers, initCopyContainer) @@ -205,8 +204,8 @@ func (h *Handler) Handle(_ context.Context, req admission.Request) admission.Res } pod.Spec.InitContainers = append(pod.Spec.InitContainers, initContainer) - // Add the Envoy and Consul sidecars. - envoySidecar, err := h.envoySidecar(pod, req.Namespace) + // Add the Envoy sidecar. + envoySidecar, err := h.envoySidecar(pod) if err != nil { h.Log.Error(err, "error configuring injection sidecar container", "request name", req.Name) return admission.Errored(http.StatusInternalServerError, fmt.Errorf("error configuring injection sidecar container: %s", err))