-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Introduce value to run proxy-init as privileged #9873
Conversation
This change aims to solve two distinct issues that have cropped up in the proxy-init configuration. First, it decouples `allowPrivilegeEscalation` from running proxy-init as root. At the moment, whenever the container is run as root, privilege escalation is also allowed. In more restrictive environments, this will prevent the pod from coming up (e.g security policies may complain about `allowPrivilegeEscalation=true`). Worth noting that privilege escalation is not necessary in many scenarios since the capabilities are passed to the iptables child process at build time. Second, it introduces a new `privileged` value that will allow users to run the proxy-init container without any restrictions (meaning all capabilities are inherited). This is essentially the same as mapping root on host to root in the container. This value may solve issues in distributions that run security enhanced linux, since iptables will be able to load kernel modules that it may otherwise not be able to load (privileged mode allows the container nearly the same privileges as processes running outside of a container on a host, this further allows the container to set configurations in AppArmor or SELinux). Privileged mode is independent from running the container as root. This gives users more control over the security context in proxy-init. The value may still be used with `runAsRoot: false`. Fixes #9718 Signed-off-by: Matei David <matei@buoyant.io>
@@ -41,7 +41,7 @@ imagePullPolicy: {{.Values.proxyInit.image.pullPolicy | default .Values.imagePul | |||
name: linkerd-init | |||
{{ include "partials.resources" .Values.proxyInit.resources }} | |||
securityContext: | |||
{{- if or .Values.proxyInit.closeWaitTimeoutSecs .Values.proxyInit.runAsRoot }} | |||
{{- if or .Values.proxyInit.closeWaitTimeoutSecs .Values.proxyInit.privileged }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious: what is the relationship between .Values.proxyInit.closeWaitTimeoutSecs
and this feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
closeWaitTimeoutSecs
is there to address long-running requests (predates me, #4276 should have info about it). The feature can't be used without running in privileged mode, which is why it acts as a conditional in our templates. When the change to run the init container as non-root was made, we've chosen to tackle the issue through templates (linkerd/linkerd2-proxy-init#49 (comment)) and this is since tied to anything that has to do with init container privileges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic sounds good to me, just some nits 👇
@@ -907,6 +909,7 @@ spec: | |||
- mountPath: /var/run/secrets/tokens | |||
name: linkerd-identity-token | |||
initContainers: | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea what is causing these empty lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They don't seem to be introduced by this PR 🤔 I've looked around and I have a bit of an idea about what's going on.
On latest main if I build the CNI and print out the install manifests:
:; git branch -v
dependabot/cargo/clap-4.0.19 c430080e9 build(deps): bump clap from 4.0.18 to 4.0.19
eliza/issue-9671 9b777750b update goldenfiles
* main 4ea8ab21d edge-22.11.3 change notes (#9884)
matei/add-init-priv-escalation 3c0635b7f PR feedback
:; bin/linkerd install --ignore-cluster| rg 'initContainer' -A 2
initContainers:
- args:
--
initContainers:
- args:
--
initContainers:
- args:
- --incoming-proxy-port
Culprit seems to be this line:
{{ if .Values.cniEnabled -}} |
(and the equivalent line for
identity
, injector doesn't seem to suffer from this).
To fix, I've added -
at the start of the if for destination
and identity
.
Result:
:; git branch
dependabot/cargo/clap-4.0.19
eliza/issue-9671
main
* matei/add-init-priv-escalation
:; bin/linkerd install --ignore-cluster| rg 'initContainer' -A 2
initContainers:
- args:
- --incoming-proxy-port
--
initContainers:
- args:
- --incoming-proxy-port
--
initContainers:
- args:
- --incoming-proxy-port
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The templates don't seem to want to update themselves through go test ./... -update
though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good after Alejandro's comments.
Signed-off-by: Matei David <matei@buoyant.io>
Signed-off-by: Matei David <matei@buoyant.io>
Thanks for the feedback, addressed all of it. Will wait for #9889 (review) to merge first to have the emptyline fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mateiidavid , it seems there's still is some funkiness with the empty lines even after having merged #9889, but that's unrelated to this PR.
…t-priv-escalation
This change aims to solve two distinct issues that have cropped up in the proxy-init configuration. First, it decouples `allowPrivilegeEscalation` from running proxy-init as root. At the moment, whenever the container is run as root, privilege escalation is also allowed. In more restrictive environments, this will prevent the pod from coming up (e.g security policies may complain about `allowPrivilegeEscalation=true`). Worth noting that privilege escalation is not necessary in many scenarios since the capabilities are passed to the iptables child process at build time. Second, it introduces a new `privileged` value that will allow users to run the proxy-init container without any restrictions (meaning all capabilities are inherited). This is essentially the same as mapping root on host to root in the container. This value may solve issues in distributions that run security enhanced linux, since iptables will be able to load kernel modules that it may otherwise not be able to load (privileged mode allows the container nearly the same privileges as processes running outside of a container on a host, this further allows the container to set configurations in AppArmor or SELinux). Privileged mode is independent from running the container as root. This gives users more control over the security context in proxy-init. The value may still be used with `runAsRoot: false`. Fixes #9718 Signed-off-by: Matei David <matei@buoyant.io>
This change aims to solve two distinct issues that have cropped up in the proxy-init configuration. First, it decouples `allowPrivilegeEscalation` from running proxy-init as root. At the moment, whenever the container is run as root, privilege escalation is also allowed. In more restrictive environments, this will prevent the pod from coming up (e.g security policies may complain about `allowPrivilegeEscalation=true`). Worth noting that privilege escalation is not necessary in many scenarios since the capabilities are passed to the iptables child process at build time. Second, it introduces a new `privileged` value that will allow users to run the proxy-init container without any restrictions (meaning all capabilities are inherited). This is essentially the same as mapping root on host to root in the container. This value may solve issues in distributions that run security enhanced linux, since iptables will be able to load kernel modules that it may otherwise not be able to load (privileged mode allows the container nearly the same privileges as processes running outside of a container on a host, this further allows the container to set configurations in AppArmor or SELinux). Privileged mode is independent from running the container as root. This gives users more control over the security context in proxy-init. The value may still be used with `runAsRoot: false`. Fixes linkerd#9718 Signed-off-by: Matei David <matei@buoyant.io> Signed-off-by: Szymon Grzemski <sz.grzemski@gmail.com>
This change aims to solve two distinct issues that have cropped up in the proxy-init configuration.
First, it decouples
allowPrivilegeEscalation
from running proxy-init as root. At the moment, whenever the container is run as root, privilege escalation is also allowed. In more restrictive environments, this will prevent the pod from coming up (e.g security policies may complain aboutallowPrivilegeEscalation=true
). Worth noting that privilege escalation is not necessary in many scenarios since the capabilities are passed to the iptables child process at build time.Second, it introduces a new
privileged
value that will allow users to run the proxy-init container without any restrictions (meaning all capabilities are inherited). This is essentially the same as mapping root on host to root in the container. This value may solve issues in distributions that run security enhanced linux, since iptables will be able to load kernel modules that it may otherwise not be able to load (privileged mode allows the container nearly the same privileges as processes running outside of a container on a host, this further allows the container to set configurations in AppArmor or SELinux).Privileged mode is independent from running the container as root. This gives users more control over the security context in proxy-init. The value may still be used with
runAsRoot: false
.Fixes #9718
Signed-off-by: Matei David matei@buoyant.io
Relevant literature:
privileged
modeMy hunch is that coupled with SELinux options at a Pod level, this should also fix #7767
Tests
I tested only with the control plane which makes use of the
proxy-init
partial. I tested four distinct scenarios:privileged=false
,runAsRoot=false
)privileged=true
)privileged=true
,runAsRoot=true
)privileged=false
,runAsRoot=true
)I tested that the pods are running and we see what we are supposed to in the security context. All yaml output is taken from the destination pod in all 4 cases.