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

Replace deprecated PodSecurityPolicy with PodSecurityAdmission enforcement #5536

Merged
merged 1 commit into from
Sep 29, 2022

Conversation

JeromeJu
Copy link
Member

@JeromeJu JeromeJu commented Sep 21, 2022

Changes

This commit removes the PodSeucrityPolicy, which is deprecated in
Kubernetes v1.21 and removed from v1.25. This adds to the
PodSecurityAdmission(PSA) restricted label with respective policies
enforced by PSP but not covered by the restricted standard of PSA.

Loosen permissions in PSP

Note that with the restricted standard of PSA, the pods in the namespace
with the PSA annotations would now have the following permissions other
than the enforcement in PSP:

  • runAsNonRoot in PSA only restricts UID but not GroupID
  • volumes in the restricted mode would look for
    • configMap
    • csi
    • downwardAPI
    • emptyDir
    • ephemeral
    • persistentVolumeClaim
    • projected
    • secret

Policies in PSP not covered by PSA

The following policies are not covered by PSA but are in PSP, they
are specified in both controller.yaml and webhook.yaml:

| previously in PSP | currently in PSA |
| seLinux | PodTemplate.securityContext.seLinux |
| fsGroup | PodTemplate.securityContext.runAsNonRoot |
| runAsGroup | PodTemplate.securityContext.runAsNonRoot |
| supplementalGroups | PodTemplate.securityContext.runAsNonRoot |

  • the seLinux specification is not enforced by PSA, so it has been set in the securityContext field for both controller.yaml and webhook.yaml.
  • fsGroup, runAsGroup and supplementalGroups are enforced to use a range of id other than 0 with mustRunAs rule in PSP but not in PSA, so it is specified as runAsNonRoot in controller and webhook to forbid root access

Fixes: #4112

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • [n/a] Has Docs included if any changes are user facing
  • [n/a] Has Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including
    functionality, content, code)
  • Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
  • Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
  • Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

action required: To allow PodSecurityAdmission to take effect, please set PodSecurity flag as `Beta` in 1.23-1.24. See https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/#feature-gates-for-graduated-or-deprecated-features for more information.

@tekton-robot tekton-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 21, 2022
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 21, 2022
@JeromeJu JeromeJu marked this pull request as draft September 21, 2022 20:38
@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 21, 2022
@JeromeJu JeromeJu changed the title [Deprecated] PodSecurityPolicy PSA for PSP test Sep 21, 2022
@tekton-robot
Copy link
Collaborator

@JeromeJu: The label(s) kind/test cannot be applied, because the repository doesn't have them.

In response to this:

/kind test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot tekton-robot added kind/bug Categorizes issue or PR as related to a bug. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. labels Sep 21, 2022
@JeromeJu JeromeJu marked this pull request as ready for review September 22, 2022 00:45
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 22, 2022
config/controller.yaml Outdated Show resolved Hide resolved
config/101-podsecuritypolicy.yaml Show resolved Hide resolved
@lbernick lbernick self-assigned this Sep 22, 2022
@JeromeJu JeromeJu force-pushed the test-PSA branch 2 times, most recently from 040f93e to eafd07b Compare September 23, 2022 13:05
@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 23, 2022
This commit removes the deprecated PodSeucrityPolicy and add
PodSecurityAdmission(PSA) restricted label with respective policies
enforced by PSP but not covered by the restricted standard of PSA.
@JeromeJu JeromeJu changed the title PSA for PSP test [Deprecated] PodSecurityPolicy Sep 23, 2022
@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 23, 2022
Copy link
Member

@lbernick lbernick left a comment

Choose a reason for hiding this comment

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

This looks basically right to me but I think @wlynch or @skaegi might be better reviewers-- could either of you take a look?

@JeromeJu it might be helpful to make a list (either in your doc or the commit message) of the pod security policy configuration options we're using and what replaces them. (For most, it'll probably be replaced by the pod security admission label you've added.)

Lastly, the release note should indicate changes relevant to users, and I don't think replacing our podsecuritypolicy should be relevant to them since you added the pod security admission. However, I think users might need to set a feature gate to enable pod security admission for k8s 1.22 to 1.24 (https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/#feature-gates-for-graduated-or-deprecated-features), so please include "action required" in the release note with instructions on how to do that.

I'd also change the commit title to "Replace deprecated PodSecurityPolicy with Pod Security Admission enforcement".

Comment on lines -23 to -26
seccomp.security.alpha.kubernetes.io/allowedProfileNames: 'docker/default,runtime/default'
seccomp.security.alpha.kubernetes.io/defaultProfileName: 'runtime/default'
apparmor.security.beta.kubernetes.io/allowedProfileNames: 'runtime/default'
apparmor.security.beta.kubernetes.io/defaultProfileName: 'runtime/default'
Copy link
Member

Choose a reason for hiding this comment

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

Do these annotations need to be replaced with any changes to the deployment spec?

Copy link
Member

Choose a reason for hiding this comment

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

I think I'm okay with this - I don't think we were ever requiring a strict set of apparmor / seccomp policies - we just wanted to make sure that there was some policy set. The restricted policy still enforces this, though loosens it a bit in that it's easier to load in other policies present on the node.

We may want to double check that this is working on a few common k8s setups (since if a policy isn't defined this will fail), but otherwise this seems okay.

config/webhook.yaml Show resolved Hide resolved
@JeromeJu JeromeJu changed the title [Deprecated] PodSecurityPolicy Replace deprecated PodSecurityPolicy with Pod Security Admission enforcement Sep 23, 2022
@JeromeJu JeromeJu changed the title Replace deprecated PodSecurityPolicy with Pod Security Admission enforcement Replace deprecated PodSecurityPolicy with PodSecurityAdmission enforcement Sep 23, 2022
Copy link
Member

@wlynch wlynch left a comment

Choose a reason for hiding this comment

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

/lgtm

Some minor comments, but nothing blocking.

config/webhook.yaml Show resolved Hide resolved
Comment on lines -23 to -26
seccomp.security.alpha.kubernetes.io/allowedProfileNames: 'docker/default,runtime/default'
seccomp.security.alpha.kubernetes.io/defaultProfileName: 'runtime/default'
apparmor.security.beta.kubernetes.io/allowedProfileNames: 'runtime/default'
apparmor.security.beta.kubernetes.io/defaultProfileName: 'runtime/default'
Copy link
Member

Choose a reason for hiding this comment

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

I think I'm okay with this - I don't think we were ever requiring a strict set of apparmor / seccomp policies - we just wanted to make sure that there was some policy set. The restricted policy still enforces this, though loosens it a bit in that it's easier to load in other policies present on the node.

We may want to double check that this is working on a few common k8s setups (since if a policy isn't defined this will fail), but otherwise this seems okay.

Comment on lines -32 to -35
volumes:
- 'emptyDir'
- 'configMap'
- 'secret'
Copy link
Member

Choose a reason for hiding this comment

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

The values covered in PSS is technically larger than we we previously set, but I think I'm okay with the increase in scope. This is the list PSS looks for:

  • configMap
  • csi
  • downwardAPI
  • emptyDir
  • ephemeral
  • persistentVolumeClaim
  • projected
  • secret

max: 65535
seLinux:
rule: 'RunAsAny'
supplementalGroups:
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, it doesn't look like this is included in PSS so it's something we'll need to be mindful about (I'm not sure if you can add root as a supplemental group, though I hope not).

As is I think this is okay, since we're not setting any supplemental groups and we're dropping capabilities like SETUID/SETGID that could be possible escalation points.

Copy link
Member Author

@JeromeJu JeromeJu Sep 23, 2022

Choose a reason for hiding this comment

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

Thanks Billy, I thought the point of supplementalGroups in PSP is to not allow root access. So can I ask will runAsNonRoot to be set as true here cover this?

Copy link
Member

Choose a reason for hiding this comment

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

Reading https://pkg.go.dev/k8s.io/api/core/v1#SecurityContext + the PSS docs it looks like runAsNonRoot doesn't restrict GID, just UID. That said, we're not granting root GID in any of our yamls, so it's probably fine (just something to keep in mind since PSS won't stop us from setting this).

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2022
@jerop jerop added this to the Pipelines v0.41 milestone Sep 27, 2022
@tekton-robot tekton-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Sep 27, 2022
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick, wlynch

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 29, 2022
@JeromeJu
Copy link
Member Author

/retest

@tekton-robot tekton-robot merged commit b506b77 into tektoncd:main Sep 29, 2022
idoru added a commit to vmware-tanzu/cartographer that referenced this pull request Oct 6, 2022
Currently blocked on a release from Tekton:

tektoncd/pipeline#5536
is expected to be released in 0.41 which is scheduled for oct 13, 2022:
https://github.com/tektoncd/pipeline/milestone/60
idoru added a commit to vmware-tanzu/cartographer that referenced this pull request Oct 31, 2022
Currently blocked on a release from Tekton:

tektoncd/pipeline#5536
is expected to be released in 0.41 which is scheduled for oct 13, 2022:
https://github.com/tektoncd/pipeline/milestone/60
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PodSecurityPolicy deprecated in Kubernetes 1.21
5 participants