use rootless containers where possible #493

kschoche · 2021-04-20T20:39:44Z

Changes proposed in this PR:

Add a security context to the init copy container and the envoy sidecar and ensure they run as non-root.
Add RunAsNonRoot: false to the connt-init container so that it can run as root even if the Pod has this disabled. This should address [0.32.0-beta1] container's runAsUser breaks non-root policy consul-helm#918.
Remove an unused param in the envoySidecar() function.

How I've tested this PR:
unit tests.
I've deployed this into a test cluster with a sample app that has:
(1) no pod security contexts
and
(2) pod security context :

  securityContext:
    runAsUser: 1000
    runAsGroup: 3000
    fsGroup: 2000
    runAsNonRoot: true

and confirmed that the pod deploys succesfully with the changes to the init container and fails without them.

How I expect reviewers to test this PR:
unit tests / run it.

Checklist:

Tests added
CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

connect-inject/envoy_sidecar.go

ishustava

Looks great!! The only thing that's blocking I think is failing when security context of the application container (not only pod) uses the same uid as Envoy. Everything else looks great!

ishustava · 2021-04-20T22:01:40Z

CHANGELOG.md

+FEATURES:
+* Connect: Add a security context to the init copy container and the envoy sidecar and ensure they do not run as root. Also use `RunAsNonRoot: false` for connect-init's container when tproxy is enabled. [[GH-493](https://github.com/hashicorp/consul-k8s/pull/493)]
+


Also use RunAsNonRoot: false for connect-init's container when tproxy is enabled.

should this be recorded as a bug?

Sure! good idea!

connect-inject/container_init.go

connect-inject/envoy_sidecar.go

ishustava · 2021-04-20T22:10:51Z

connect-inject/handler.go

@@ -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.


lkysow

Looks good. Mainly wondering about the uid for the copy container.

lkysow · 2021-04-21T15:18:18Z

CHANGELOG.md

+* 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)]


Suggested change

* Connect: Use `RunAsNonRoot: false` for connect-init's container when tproxy is enabled. [[GH-493](https://github.com/hashicorp/consul-k8s/pull/493)]

* Connect: Use `runAsNonRoot: false` for connect-init's container when tproxy is enabled. [[GH-493](https://github.com/hashicorp/consul-k8s/pull/493)]

lkysow · 2021-04-21T15:19:05Z

connect-inject/container_init.go

@@ -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.


I found this comment confusing because it says it's envoy's UID but then it says it's for the command consul connect redirect-traffic.

Upon further reading I see that this is what gets passed into that command's -proxy-uid flag so that makes sense now but I'm thinking this comment should either just say "this is envoy's linux user id" without talking about the redirect-traffic command or it should say "this is envoy's linux user id that will be passed to the -proxy-uid flag of consul connect redirect-traffic".

lkysow · 2021-04-21T15:19:13Z

connect-inject/container_init.go

@@ -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.


Suggested change

// EnvoyUID is the UID that `consul connect redirect-traffic` will use when tproxy is enabled.

// EnvoyUID is the Linux user ID that `consul connect redirect-traffic` will use when tproxy is enabled.

For folks that don't know what UID stands for.

lkysow · 2021-04-21T15:23:44Z

connect-inject/container_init.go

+			RunAsUser:              pointerToInt64(envoyUserAndGroupID),
+			RunAsGroup:             pointerToInt64(envoyUserAndGroupID),


why does the copy container need to run as the envoy user?

Without setting a UID it will run as root because that is the default of the consul container.

Ahh interesting. So we can pick any random uid here as long at it's not 0 and we're good?

Can you add a comment explaining that here 🙏

Yup, exactly! And I just picked the same UID as envoy so I didn't have to make another constant. I'll definitely add a comment to clarify thanks for pointing this out!!

$ kubectl exec -it counting -c envoy-sidecar -- sh / $ ls -lah /consul/connect-inject/consul -rwxr-xr-x 1 5995 5995 111.7M Apr 20 20:44 /consul/connect-inject/consul

Maybe to avoid confusion, we should use a different user? In the RFC, we proposed 5996.

lkysow · 2021-04-21T15:26:18Z

connect-inject/container_init.go

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


I think we can pass it in as an int since the template will render it corrctly.

lkysow · 2021-04-21T15:28:39Z

connect-inject/container_init_test.go

 			if tt.CmdNot != "" {
-				require.NotContains(actual, tt.CmdNot)
+				require.NotContains(tt.CmdNot, actual)


Looks like none of the test cases set CmdNot so you can remove this field altogether.

nice catch!

lkysow · 2021-04-21T15:28:50Z

connect-inject/container_init_test.go

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


lkysow · 2021-04-21T15:38:45Z

connect-inject/envoy_sidecar.go

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


Suggested change

return corev1.Container{}, fmt.Errorf("user containers cannot have the same uid as envoy: %v", 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)

This error will be shown during kubectl describe so I think it should make sense in that context and be as helpful as possible. A user won't think of their containers as "user containers", they'll just think of them as their pod's containers. Nice to include the specific container's name too. This is just a suggestion for a possible error that meets that criteria.

lkysow · 2021-04-21T15:39:48Z

connect-inject/envoy_sidecar_test.go

+	_, err := h.envoySidecar(pod)
+	require.Error(err, fmt.Sprintf("user containers cannot have the same uid as envoy: %v", envoyUserAndGroupID))
+}
+


👍 good test coverage.

ishustava · 2021-04-21T15:40:48Z

CHANGELOG.md

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


I think we should also call out that we will error out if you have the same user as envoy as a breaking change. Unless we want to error out only when tproxy is enabled.

Yeah good point. But now I have a FEATURE + BREAKING CHANGE + BUG FIX entry in the changelog against the same PR, can we consolidate this somehow?

Haha, yes that's true. Maybe change the non-bug fix to a breaking change and add a note about what's breaking.

@ishustava how does this look?

ishustava · 2021-04-21T15:57:37Z

connect-inject/container_init.go

+			RunAsUser:              pointerToInt64(envoyUserAndGroupID),
+			RunAsGroup:             pointerToInt64(envoyUserAndGroupID),


Maybe to avoid confusion, we should use a different user? In the RFC, we proposed 5996.

kschoche · 2021-04-21T16:29:48Z

connect-inject/container_init_test.go

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


Unused in this test.

use rootless containers where possible

7d24e63

kschoche added the theme/tproxy Items related to transparent proxy label Apr 20, 2021

kschoche requested a review from a team April 20, 2021 20:39

kschoche self-assigned this Apr 20, 2021

kschoche requested review from lkysow and ishustava and removed request for a team April 20, 2021 20:39

update changelog

a0075cd

thisisnotashwin changed the base branch from master to adding-to-ep-controller April 20, 2021 20:48

thisisnotashwin changed the base branch from adding-to-ep-controller to master April 20, 2021 20:49

kschoche commented Apr 20, 2021

View reviewed changes

connect-inject/envoy_sidecar.go Show resolved Hide resolved

kschoche closed this Apr 20, 2021

kschoche reopened this Apr 20, 2021

ishustava reviewed Apr 20, 2021

View reviewed changes

review comments

d3041fa

kschoche requested a review from ishustava April 20, 2021 22:52

lkysow reviewed Apr 21, 2021

View reviewed changes

ishustava approved these changes Apr 21, 2021

View reviewed changes

review comments

fb6c7fd

kschoche commented Apr 21, 2021

View reviewed changes

kschoche requested a review from lkysow April 21, 2021 16:30

lkysow approved these changes Apr 21, 2021

View reviewed changes

Merge branch 'master' into tproxy-rootless-containers

2feb3dd

kschoche merged commit 345419e into master Apr 21, 2021

kschoche deleted the tproxy-rootless-containers branch April 21, 2021 19:21

ishustava mentioned this pull request Apr 21, 2021

[0.32.0-beta1] container's runAsUser breaks non-root policy hashicorp/consul-helm#918

Closed

david-yu mentioned this pull request May 26, 2021

[SECURITY] sidecar security context config #347

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use rootless containers where possible #493

use rootless containers where possible #493

kschoche commented Apr 20, 2021 •

edited

Loading

ishustava left a comment

ishustava Apr 20, 2021

kschoche Apr 20, 2021

ishustava Apr 20, 2021

lkysow left a comment

lkysow Apr 21, 2021

lkysow Apr 21, 2021

lkysow Apr 21, 2021

lkysow Apr 21, 2021

kschoche Apr 21, 2021

lkysow Apr 21, 2021

kschoche Apr 21, 2021

ishustava Apr 21, 2021

lkysow Apr 21, 2021

lkysow Apr 21, 2021

kschoche Apr 21, 2021

lkysow Apr 21, 2021

lkysow Apr 21, 2021

lkysow Apr 21, 2021

ishustava Apr 21, 2021

kschoche Apr 21, 2021

ishustava Apr 21, 2021

kschoche Apr 21, 2021

ishustava Apr 21, 2021

ishustava Apr 21, 2021

kschoche Apr 21, 2021

		FEATURES:
		* Connect: Add a security context to the init copy container and the envoy sidecar and ensure they do not run as root. Also use `RunAsNonRoot: false` for connect-init's container when tproxy is enabled. [[GH-493](https://github.com/hashicorp/consul-k8s/pull/493)]

	* Connect: Use `RunAsNonRoot: false` for connect-init's container when tproxy is enabled. [[GH-493](https://github.com/hashicorp/consul-k8s/pull/493)]
	* Connect: Use `runAsNonRoot: false` for connect-init's container when tproxy is enabled. [[GH-493](https://github.com/hashicorp/consul-k8s/pull/493)]

	// EnvoyUID is the UID that `consul connect redirect-traffic` will use when tproxy is enabled.
	// EnvoyUID is the Linux user ID that `consul connect redirect-traffic` will use when tproxy is enabled.

		RunAsUser: pointerToInt64(envoyUserAndGroupID),
		RunAsGroup: pointerToInt64(envoyUserAndGroupID),

	return corev1.Container{}, fmt.Errorf("user containers cannot have the same uid as envoy: %v", 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)


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

use rootless containers where possible #493

use rootless containers where possible #493

Conversation

kschoche commented Apr 20, 2021 • edited Loading

ishustava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkysow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kschoche commented Apr 20, 2021 •

edited

Loading