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

Add support for Pod Security Admission #9719

Merged
merged 10 commits into from
Dec 19, 2022
Merged

Add support for Pod Security Admission #9719

merged 10 commits into from
Dec 19, 2022

Conversation

alpeb
Copy link
Member

@alpeb alpeb commented Oct 27, 2022

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

# 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 with a patch to run the multicluster integration test with CNI in k8s 1.25.

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.
@alpeb alpeb requested a review from a team as a code owner October 27, 2022 20:09
@olix0r olix0r added this to the stable-2.13.0 milestone Nov 3, 2022
@alpeb
Copy link
Member Author

alpeb commented Nov 3, 2022

Also closes #9445

Copy link
Member

@adleong adleong left a 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?

@alpeb
Copy link
Member Author

alpeb commented Nov 4, 2022

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.

Yep, that's a good suggestion. We're already using the CM so should be no problem to leverage that.

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?

Correct, we don't use the namespace template in the Helm case. But we trigger the namespace-metadata post-install hook which adds the linkerd.io/extension: viz label to the existing namespace. We could expand that to also add the pod-security.kubernetes.io/enforce label, whose value would depend on the .Values.cniEnabled value. There's also the possibility to leverage Helm's lookup template function for it to read into the CM in order to retrieve the value automatically, but I'm not sure if the extra complexity is granted (we do stuff like that in the operator chart btw).

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.

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?

@alpeb
Copy link
Member Author

alpeb commented Nov 10, 2022

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 namespace-metadata-rbac.yaml which is no biggie. OTOH the lookup function runs under the user running the helm command, so that's full privs and no special RBAC required. I'll experiment with both options and see how it goes.

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.
@alpeb
Copy link
Member Author

alpeb commented Nov 15, 2022

Ok 0ba4df3 and 06edf42 add the ability to detect the cniEnabled value during the extensions installation, both for the CLI and Helm. For the latter, a new role has been added into the linkerd namespace enabling reading the linkerd-config CM. I ended up doing this from the namespace-metadata hook instead of using the lookup helm function so this still works when using helm template instead of helm install.

Copy link
Contributor

@kleimkuhler kleimkuhler left a 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.

@@ -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 {
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member

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.

@alpeb alpeb requested a review from kleimkuhler November 17, 2022 14:29
pkg/healthcheck/healthcheck.go Outdated Show resolved Hide resolved
pkg/healthcheck/healthcheck.go Outdated Show resolved Hide resolved
@@ -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
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

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.

Copy link
Member Author

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 😉

pkg/healthcheck/healthcheck.go Outdated Show resolved Hide resolved
# 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)
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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 */}}
Copy link
Member

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?

Copy link
Member Author

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.

@alpeb alpeb merged commit faf0ff6 into main Dec 19, 2022
@alpeb alpeb deleted the alpeb/psa branch December 19, 2022 15:23
sgrzemski pushed a commit to sgrzemski/linkerd2 that referenced this pull request Jan 9, 2023
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>
alpeb added a commit that referenced this pull request Jan 26, 2023
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 alpeb mentioned this pull request Jan 26, 2023
adleong pushed a commit that referenced this pull request Jan 27, 2023
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!
@channaneq
Copy link

@alpeb Could I ask a question on this, is this statement correct?

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.

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!

@alpeb
Copy link
Member Author

alpeb commented Apr 5, 2023

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.

I thought that this would place the proxy-init container under the privileged policy as the restricted policy is much tighter

You're right, that was a misnomer on my part. I've corrected the PR's description.

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.

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.

@joebowbeer
Copy link
Contributor

joebowbeer commented Apr 5, 2023

@channaneq writes:

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.

As I understand it, the privileged policy is essentially no enforcement at all:

Unrestricted policy, providing the widest possible level of permissions. This policy allows for known privilege escalations.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Pod Security Admission
7 participants