Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow proxy-init container to run as non-root #7162

Merged
merged 1 commit into from
Nov 5, 2021

Conversation

chrischdi
Copy link
Contributor

Linkerd proxy-init container is currently enforced to run as root.

Removes hardcoding runAsNonRoot: false and runAsUser: 0. This way
the container inherits the user ID from the proxy-init image instead which
may allow to run as non-root.

Validation should happen during the usual tests.

Fixes #5505

Signed-off-by: Schlotter, Christian christian.schlotter@daimler.com


Requires linkerd/linkerd2-proxy-init#49 for having the correct capabilities set for the iptables binaries.

It would be also possible to keep the option for users to run as root by adding something like:

  {{- if .Values.proxyInit.runAsRoot }}
  runAsNonRoot: false
  runAsUser: 0
  {{- end }}

Christian Schlotter christian.schlotter@daimler.com, Daimler AG on behalf of Daimler TSS GmbH

Provider Information

@@ -44,8 +44,6 @@ securityContext:
privileged: false
{{- end }}
readOnlyRootFilesystem: true
runAsNonRoot: false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to k8s reference docs, the default for runAsNonRoot is already false. So I think we should have here runAsNonRoot: true instead, for the Kubelet to check that the container is not triggered under user 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-added runAsNonRoot and set it to true. This will enforce the proxy-init container that it needs to run as non-root.

This then will require the changes from linkerd/linkerd2-proxy-init#49 .

Indicates that the container must run as a non-root user. If true, the
Kubelet will validate the image at runtime to ensure that it does not run
as UID 0 (root) and fail to start the container if it does. If unset or
false, no such validation will be performed. May also be set in
PodSecurityContext. If set in both SecurityContext and PodSecurityContext,
the value specified in SecurityContext takes precedence.
(from kubectl explain pod.spec.initContainers.securityContext.runAsNonRoot

@chrischdi
Copy link
Contributor Author

As mentioned I re-added runAsNonRoot and set it to true. This will enforce the proxy-init container that it needs to run as non-root.

I also adjusted the tests to match the new chart.

@alpeb
Copy link
Member

alpeb commented Oct 28, 2021

Thanks @chrischdi, we'll wait for linkerd/linkerd2-proxy-init#49 to get one more review to get it merged, then I'll create a new release v1.5.0. Then you can update this current PR to pick up that new proxy-init version 👍

@chrischdi
Copy link
Contributor Author

chrischdi commented Oct 29, 2021

As mentioned in linkerd/linkerd2-proxy-init#49 (comment) :

We may have to switch to the following because of the configurable proxyInit.closeWaitTimeoutSecs which would require root permissions due to the execution of sysctl.

{{- if .Values.proxyInit.closeWaitTimeoutSecs }}
privileged: true
runAsNonRoot: false
runAsUser: 0
{{- else }}
privileged: false
runAsNonRoot: true
{{- end }}

@chrischdi
Copy link
Contributor Author

requires rebase after #7203 was done :-)

@chrischdi
Copy link
Contributor Author

Rebased changes to main branch because #7203 got merged :-)

@chrischdi
Copy link
Contributor Author

As mentioned in linkerd/linkerd2-proxy-init#49 (comment) :

We may have to switch to the following because of the configurable proxyInit.closeWaitTimeoutSecs which would require root permissions due to the execution of sysctl.

{{- if .Values.proxyInit.closeWaitTimeoutSecs }}
privileged: true
runAsNonRoot: false
runAsUser: 0
{{- else }}
privileged: false
runAsNonRoot: true
{{- end }}

Still requires this change, I'll update the PR

Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. So the idea is to have proxy-init run as non-root, unless closeWaitTimeoutSecs is used. Any objections to that @mateiidavid ?
Also could you please take care of the DCO?

test/integration/inject/inject_test.go Show resolved Hide resolved
@chrischdi
Copy link
Contributor Author

Looking good. So the idea is to have proxy-init run as non-root, unless closeWaitTimeoutSecs is used. Any objections to that @mateiidavid ? Also could you please take care of the DCO?

Thanks for the review.

Looks like I missed the zero and signing the commits again :-) should be ok now 👍

Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chrischdi !

@mateiidavid
Copy link
Member

Looking good. So the idea is to have proxy-init run as non-root, unless closeWaitTimeoutSecs is used. Any objections to that @mateiidavid ?
Also could you please take care of the DCO?

Not at all! Think everything looks great.

Copy link
Member

@mateiidavid mateiidavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might have to do yet another rebase, sorry about that. We bumped up proxy-init to v1.5.1, the base image was modified. Ready to 🚢 from my side after that.

Linkerd proxy-init container is currently enforced to run as root.

Removes hardcoding `runAsNonRoot: false` and `runAsUser: 0`. This way
the container inherits the user ID from the proxy-init image instead which
may allow to run as non-root.

Validation should happen during the usual tests.

Fixes linkerd#5505

Signed-off-by: Schlotter, Christian <christian.schlotter@daimler.com>
@chrischdi
Copy link
Contributor Author

I think you might have to do yet another rebase, sorry about that. We bumped up proxy-init to v1.5.1, the base image was modified. Ready to ship from my side after that.

No worries :-) done, I squashed the commits again, I hope that's okay (the second and third only have been fixups).

@alpeb alpeb merged commit 9853353 into linkerd:main Nov 5, 2021
@part-time-githubber
Copy link

part-time-githubber commented Nov 15, 2021

@alpeb So how do we track down which stable release this is is going to make into, and when?

@kleimkuhler
Copy link
Contributor

Additive changes like this typically make it into the next stable release which will 2.12 and likely sometime early 2022.

If there is a stable 2.11.2 we could maybe consider bringing this in especially if 2.12 is still far enough out, but it's not very likely. We'll also want to make sure a change like this has a few edge releases to be tested out in before including it in a stable release.

@part-time-githubber
Copy link

@kleimkuhler many thanks for the prompt response on this!

olix0r pushed a commit that referenced this pull request Apr 12, 2022
Linkerd proxy-init container is currently enforced to run as root.

Removes hardcoding `runAsNonRoot: false` and `runAsUser: 0`. This way
the container inherits the user ID from the proxy-init image instead which
may allow to run as non-root.

Fixes #5505

Signed-off-by: Schlotter, Christian <christian.schlotter@daimler.com>
(cherry picked from commit 9853353)
Signed-off-by: Oliver Gould <ver@buoyant.io>
olix0r pushed a commit that referenced this pull request Apr 13, 2022
Linkerd proxy-init container is currently enforced to run as root.

Removes hardcoding `runAsNonRoot: false` and `runAsUser: 0`. This way
the container inherits the user ID from the proxy-init image instead which
may allow to run as non-root.

Fixes #5505

Signed-off-by: Schlotter, Christian <christian.schlotter@daimler.com>
(cherry picked from commit 9853353)
Signed-off-by: Oliver Gould <ver@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linkerd without CNI - run as non-root
6 participants