use rootless containers where possible

CHANGELOG.md

@@ -1,5 +1,11 @@
 ## UNRELEASED
 
+FEATURES:
+* Connect: Add a security context to the init copy container and the envoy sidecar and ensure they do not run as root. [[GH-493](https://github.com/hashicorp/consul-k8s/pull/493)]
+
+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)]
+
 ## 0.26.0-beta1 (April 16, 2021)
 
 BREAKING CHANGES:

connect-inject/container_init.go

@@ -13,6 +13,7 @@ const (
 	InjectInitCopyContainerName = "copy-consul-bin"
 	InjectInitContainerName     = "consul-connect-inject-init"
 	rootUserAndGroupID          = 0
+	envoyUserAndGroupID         = 5995
 	netAdminCapability          = "NET_ADMIN"
 )
 
@@ -37,6 +38,8 @@ type initContainerCommandData struct {
 	PrometheusScrapePath string
 	// PrometheusBackendPort configures where the listener on Envoy will point to.
 	PrometheusBackendPort string
+	// EnvoyUID is the UID that `consul connect redirect-traffic` will use when tproxy is enabled.
+	EnvoyUID string
 
 	// 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 +63,12 @@ func (h *Handler) containerInitCopyContainer() corev1.Container {
 			},
 		},
 		Command: []string{"/bin/sh", "-ec", cmd},
+		SecurityContext: &corev1.SecurityContext{
+			RunAsUser:              pointerToInt64(envoyUserAndGroupID),
+			RunAsGroup:             pointerToInt64(envoyUserAndGroupID),
+			RunAsNonRoot:           pointerToBool(true),
+			ReadOnlyRootFilesystem: pointerToBool(true),
+		},
 	}
 }
 
@@ -78,6 +87,7 @@ func (h *Handler) containerInit(pod corev1.Pod, k8sNamespace string) (corev1.Con
 		NamespaceMirroringEnabled: h.EnableK8SNSMirroring,
 		ConsulCACert:              h.ConsulCACert,
 		EnableTransparentProxy:    tproxyEnabled,
+		EnvoyUID:                  strconv.Itoa(envoyUserAndGroupID),
 	}
 
 	if data.AuthMethod != "" {
@@ -170,6 +180,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 +206,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 +271,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 }}
 `

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, " ")
@@ -384,7 +385,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,7 +416,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,7 +454,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`,
 			"",
 		},
 	}
@@ -467,9 +468,9 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \
 			container, err := h.containerInit(*tt.Pod(minimal()), k8sNamespace)
 			require.NoError(err)
 			actual := strings.Join(container.Command, " ")
-			require.Equal(actual, tt.Cmd)
+			require.Equal(tt.Cmd, actual)
 			if tt.CmdNot != "" {
-				require.NotContains(actual, tt.CmdNot)
+				require.NotContains(tt.CmdNot, actual)
 			}
 		})
 	}
@@ -598,16 +599,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(envoyUserAndGroupID),
+		RunAsGroup:             pointerToInt64(envoyUserAndGroupID),
+		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
-}

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("user containers cannot have the same uid as envoy: %v", 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
 }

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,51 @@ 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",
+				},
+				{
+					Name: "app",
+					SecurityContext: &corev1.SecurityContext{
+						RunAsUser: pointerToInt64(envoyUserAndGroupID),
+					},
+				},
+			},
+		},
+	}
+	_, err := h.envoySidecar(pod)
+	require.Error(err, fmt.Sprintf("user containers cannot have the same uid as envoy: %v", 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 +172,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 +346,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)

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))