-
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
Add support for Pod Security Admission #9719
Conversation
Closes #9676 This adds the `pod-security.kubernetes.io/enforce` label as described in [Pod Security Admission labels for namespaces](https://kubernetes.io/docs/concepts/security/pod-security-admission/#pod-security-admission-labels-for-namespaces). PSA gives us three different possible values (policies or modes): [privileged, baseline and restricted](https://kubernetes.io/docs/concepts/security/pod-security-standards/). For non-CNI mode, the proxy-init container relies on granting the NET_RAW and NET_ADMIN capabilities, which places those pods under the `restricted` policy. OTOH for CNI mode we can enforce the `restricted` policy, by setting some defaults on the containers' `securityContext` as done in this PR. Also note this change also adds the `cniEnabled` entry in the `values.yaml` file for all the extension charts, which determines what policy to use. Final note: this includes the fix from #9717, otherwise an empty gateway UID prevents the pod to be created under the `restricted` policy. ## How to test As this is only enforced as of k8s 1.25, here are the instructions to run 1.25 with k3d using Calico as CNI: ```bash # launch k3d with k8s v1.25, with no flannel CI $ k3d cluster create --image='+v1.25' --k3s-arg '--disable=local-storage,metrics-server@server:0' --no-lb --k3s-arg --write-kubeconfig-mode=644 --k3s-arg --flannel-backend=none --k3s-arg --cluster-cidr=192.168.0.0/16 --k3s-arg '--disable=servicelb,traefik@server:0' # install Calico $ k apply -f https://k3d.io/v5.1.0/usage/advanced/calico.yaml # load all the images $ bin/image-load --k3d proxy controller policy-controller web metrics-api tap cni-plugin jaeger-webhook # install linkerd-cni $ bin/go-run cli install-cni|k apply -f - # install linkerd-crds $ bin/go-run cli install --crds|k apply -f - # install linkerd-control-plane in CNI mode $ bin/go-run cli install --linkerd-cni-enabled|k apply -f - # Pods should come up without issues. You can also try the viz and jaeger extensions. # Try removing one of the securityContext entries added in this PR, and the Pod # won't come up. You should be able to see the PodSecurity error in the associated # ReplicaSet. ``` To test the multicluster extension using CNI, check this [gist](https://gist.github.com/alpeb/4cbbd5ad87538b9e0d39a29b4e3f02eb) with a patch to run the multicluster integration test with CNI in k8s 1.25.
Also closes #9445 |
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.
Would it be worth having the linkerd extension install commands read the cniEnabled value from the main linkerd configmap and set that in the values used to install the extension? I worry about people doing something like linkerd viz install
without correctly setting the cniEnabled value to match what's in the main control plane.
As for Helm installs of the extensions, it's a moot point because we never use the namespace templates in the Helm path, right? Which means if Helm users want this label on their namespace, they have to add it themselves?
Yep, that's a good suggestion. We're already using the CM so should be no problem to leverage that.
Correct, we don't use the namespace template in the Helm case. But we trigger the |
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. Alex's comments make sense. Good idea to use the CM for the extensions. For the helm hook, I assume that by:
We could expand that to also add the pod-security.kubernetes.io/enforce label, whose value would depend on the .Values.cniEnabled value
we mean that cniEnabled would still be needed as a value. The hook can't read it from the CM, correct? Also, do we need any special RBAC to read the CM if we go with the lookup
function?
Yes, we'd still need that value. Actually the hook would be able to read it from the CM if we add the right privs into |
Have `CheckPublicAPIClientOrRetryOrExit` return the `HealthChecker` instance, which now the extensions CLI install subcommands use to figure the value of `cniEnabled`.
The `namespace-metadata` Jobs for each extension now read the `linkerd-config` CM to retrieve the value for `cniEnabled`, and patch their namespace with the appropriate `pod-security.kubernetes.io/enforce` label. Added `config-rbac.yaml` with role used in extensions to read the `linkerd-config` CM.
Ok 0ba4df3 and 06edf42 add the ability to detect the |
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.
Overall this looks good. I left a comment about a change to checking the public API.
In terms of what this PR is actually addressing, all that looks good.
edit: Okay my comment didn't submit with this review; it's what I get for trying to do this through VS Code's review tool. It's submitted separately below.
pkg/public/api.go
Outdated
@@ -11,7 +11,7 @@ import ( | |||
// plane. If the checks fail, then CLI will print an error and exit. If the | |||
// hcOptions.retryDeadline param is specified, then the CLI will print a | |||
// message to stderr and retry. | |||
func CheckPublicAPIClientOrRetryOrExit(hcOptions healthcheck.Options) { | |||
func CheckPublicAPIClientOrRetryOrExit(hcOptions healthcheck.Options) *healthcheck.HealthChecker { |
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.
Could we keep these separate? Maybe we introduce func NewPublicAPIClient
and then it gets initialized by a call to func (hc *healthcheck.HealthChecker) CheckPublicAPIClient
. Afterwards we should still be able to check hc.cniEnabled
.
It adds a little code, but here when we only care about checking certain things we don't really need to return anything.
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.
Agreed adding a return there felt a little weird. I've pushed e3a5450 which separates these things, just for the the viz install as an example for now. Those new functions would live directly inside the healthcheck
package, without having to go through the intermediary public
package (pkg/public/api.go
) which we can remove after applying this to all the extensions.
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.
Yea I like this. So this would then allow us to remove func CheckPublicAPIClientOrRetryOrExit
and func exitOnError
.
Once this is used in the other related changes in this PR, it should look good to me.
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.
Have to say, it looks really nice this way.
viz/cmd/install.go
Outdated
@@ -62,10 +61,11 @@ func newCmdInstall() *cobra.Command { | |||
The installation can be configured by using the --set, --values, --set-string and --set-file flags. | |||
A full list of configurable values can be found at https://www.github.com/linkerd/linkerd2/tree/main/viz/charts/linkerd-viz/README.md | |||
`, | |||
RunE: func(cmd *cobra.Command, args []string) error { | |||
RunE: func(_ *cobra.Command, _ []string) error { | |||
cniEnabled := false |
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.
We could also declare this at the top where we have ha
, skipChecks
and ignoreCluster
instead of instantiating here. In a sense, we already do this with ha
so we could probably be consistent with that pattern.
We can still pass it as an argument to the function, I think that makes it more readable.
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'll move it to the top as you suggest.
We can still pass it as an argument to the function, I think that makes it more readable.
Can you please clarify?
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.
Yeah, ofc! Could have been a bit more clearer.
In places like cli/cmd/install.go
we actually declare these values (e.g ignoreCluster
) in the outer scope, not in the enclosing function's scope (the command builder function, newCmdInstall
). If we have global state, we can access it (afaik) from other functions without having to pass it in as an argument.
Lines 70 to 72 in 26dbd4e
ignoreCluster bool | |
) |
I like the approach here more because we don't rely on global state. Passing the values in as an argument to the function install(w io.Writer, options values.Options, ha, cniEnabled bool)
also reads better to me.
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.
@mateiidavid ok cool, so I think my last commit addresses all your feedback 😉
# discard value in the last-applied-configuration annotation | ||
cniEnabled=$(curl -kfv -H "Authorization: Bearer $token" \ | ||
"https://kubernetes.default.svc/api/v1/namespaces/{{.Values.linkerdNamespace}}/configmaps/linkerd-config" | \ | ||
sed -r -n 's/.*cniEnabled: (\w+).*/\1/gp' | tail -1) |
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.
is it worth pulling in something like yq here to parse this properly and make it a bit more robust?
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.
We'd have to use a different image than curlImages/curl
to include anything besides curl
. And given the privileges this has we can't install anything at runtime either.
Ideally we could build a binary that did all this and include it in say the proxy image, but it'd require some extra maintenance.
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've raised #9985 to track this effort separately.
{{- end }} | ||
annotations: | ||
{{ include "partials.annotations.created-by" . }} | ||
{{- /* cross-namespace RBAC doesn't play well as a hook for some reason */}} |
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.
can you clarify this? why do we need the helm hook annnotations on the RoleBinding? why are they commented out? what is the consequence of cross namespace RBAC not working for hooks?
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 for pointing this out. The problem was apparently related to ordering, i.e. the script in the namespace job running before it acquired the permissions to access the ConfigMap. I uncommented these lines, and set helm.sh/hook-weight
to 1 in the namespace jobs so they run after these RoleBindings are persisted.
Closes linkerd#9676 This adds the `pod-security.kubernetes.io/enforce` label as described in [Pod Security Admission labels for namespaces](https://kubernetes.io/docs/concepts/security/pod-security-admission/#pod-security-admission-labels-for-namespaces). PSA gives us three different possible values (policies or modes): [privileged, baseline and restricted](https://kubernetes.io/docs/concepts/security/pod-security-standards/). For non-CNI mode, the proxy-init container relies on granting the NET_RAW and NET_ADMIN capabilities, which places those pods under the `restricted` policy. OTOH for CNI mode we can enforce the `restricted` policy, by setting some defaults on the containers' `securityContext` as done in this PR. Also note this change also adds the `cniEnabled` entry in the `values.yaml` file for all the extension charts, which determines what policy to use. Final note: this includes the fix from linkerd#9717, otherwise an empty gateway UID prevents the pod to be created under the `restricted` policy. ## How to test As this is only enforced as of k8s 1.25, here are the instructions to run 1.25 with k3d using Calico as CNI: ```bash # launch k3d with k8s v1.25, with no flannel CI $ k3d cluster create --image='+v1.25' --k3s-arg '--disable=local-storage,metrics-server@server:0' --no-lb --k3s-arg --write-kubeconfig-mode=644 --k3s-arg --flannel-backend=none --k3s-arg --cluster-cidr=192.168.0.0/16 --k3s-arg '--disable=servicelb,traefik@server:0' # install Calico $ k apply -f https://k3d.io/v5.1.0/usage/advanced/calico.yaml # load all the images $ bin/image-load --k3d proxy controller policy-controller web metrics-api tap cni-plugin jaeger-webhook # install linkerd-cni $ bin/go-run cli install-cni|k apply -f - # install linkerd-crds $ bin/go-run cli install --crds|k apply -f - # install linkerd-control-plane in CNI mode $ bin/go-run cli install --linkerd-cni-enabled|k apply -f - # Pods should come up without issues. You can also try the viz and jaeger extensions. # Try removing one of the securityContext entries added in this PR, and the Pod # won't come up. You should be able to see the PodSecurity error in the associated # ReplicaSet. ``` To test the multicluster extension using CNI, check this [gist](https://gist.github.com/alpeb/4cbbd5ad87538b9e0d39a29b4e3f02eb) with a patch to run the multicluster integration test with CNI in k8s 1.25. Signed-off-by: Szymon Grzemski <sz.grzemski@gmail.com>
Fixes #10150 When we added PodSecurityAdmission in #9719 (and included in edge-23.1.1), we added the entry `seccompProfile.type=RuntimeDefault` to the containers SecurityContext. For PSP to accept that we require to add the annotation `seccomp.security.alpha.kubernetes.io/allowedProfileNames: "runtime/default"` into the PSP resource, which also implies we require to add the entry `seccompProfile.type=RuntimeDefault` to the pod's SecurityContext as well, not just the container's. It also turns out the `namespace-metadata` Jobs used by extensions for the helm installation method didn't have their ServiceAccount properly bound to the PSP resource. This resulted in the `helm install` command failing, and although the extensions resources did get deployed, they were not being discoverable by `linkerd check`. This change fixes that as well, that has been broken since 2.12.0!
Fixes #10150 When we added PodSecurityAdmission in #9719 (and included in edge-23.1.1), we added the entry `seccompProfile.type=RuntimeDefault` to the containers SecurityContext. For PSP to accept that we require to add the annotation `seccomp.security.alpha.kubernetes.io/allowedProfileNames: "runtime/default"` into the PSP resource, which also implies we require to add the entry `seccompProfile.type=RuntimeDefault` to the pod's SecurityContext as well, not just the container's. It also turns out the `namespace-metadata` Jobs used by extensions for the helm installation method didn't have their ServiceAccount properly bound to the PSP resource. This resulted in the `helm install` command failing, and although the extensions resources did get deployed, they were not being discoverable by `linkerd check`. This change fixes that as well, that has been broken since 2.12.0!
@alpeb Could I ask a question on this, is this statement correct?
I thought that this would place the proxy-init container under the privileged policy as the restricted policy is much tighter Secondly, we are looking to move away from PSP's to PSA's, in order to do this, do we have to enforce the privileged policy in all namespaces where linkerd injected pods exist? This approach is potentially questionable from a security perspective but I would be keen to hear your thoughts on this as I may be missing something. Thanks! |
You're right, that was a misnomer on my part. I've corrected the PR's description.
Unfortunately PSA is a very coarse mechanism and we're forced to fall back into the most open-ended standard in the non-CNI case, basically opting out of the mechanism. My recommendation is to consider a more advanced alternative such as OPA Gatekeeper. |
@channaneq writes:
As I understand it, the privileged policy is essentially no enforcement at all:
In other words, PSA must be disabled in namespaces where linkerd is injected in non-CNI mode. Furthermore, until stable-2.13.0 is released, PSA restricted level cannot be enforced in CNI mode because of the noop pause initContainer that runs as root (#9671), leaving baseline (minimal) as the only level of enforcement even in CNI mode. |
Closes #9676
This adds the
pod-security.kubernetes.io/enforce
label as described in Pod Security Admission labels for namespaces.PSA gives us three different possible values (policies or modes): privileged, baseline and restricted.
For non-CNI mode, the proxy-init container relies on granting the NET_RAW and NET_ADMIN capabilities, which places those pods under the
restricted
privileged
policy. OTOH for CNI mode we can enforce therestricted
policy, by setting some defaults on the containers'securityContext
as done in this PR.Also note this change also adds the
cniEnabled
entry in thevalues.yaml
file for all the extension charts, which determines what policy to use.Final note: this includes the fix from #9717, otherwise an empty gateway UID prevents the pod to be created under the
restricted
policy.How to test
As this is only enforced as of k8s 1.25, here are the instructions to run 1.25 with k3d using Calico as CNI:
To test the multicluster extension using CNI, check this gist with a patch to run the multicluster integration test with CNI in k8s 1.25.