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

PSP Replacement KEP #2582

Merged
merged 37 commits into from
May 12, 2021
Merged

PSP Replacement KEP #2582

merged 37 commits into from
May 12, 2021

Conversation

tallclair
Copy link
Member

@tallclair tallclair commented Mar 22, 2021

This KEP proposes a new policy mechanism to replace the use cases covered by PodSecurityPolicy.

This is an adaptation of the initial proposal that's been under discussion by members of sig-auth and sig-security, here: https://docs.google.com/document/d/1dpfDF3Dk4HhbQe74AyCpzUYMjp4ZhiEgGXSMpVWLlqQ/edit?usp=sharing

Most of the content is copied over from that doc, with the following additions:

  • motivation & goals sections (copied from the PSP replacement goals doc)
  • admission configuration section - This is mostly just spec'ing out what we were already discussing
  • risks & mitigations - highlighting some concerns that have been raised about this approach.
  • updates - discuss how pod updates will be handled in more detail, this section needs review
  • test plan
  • monitoring - needs more input

@liggitt recorded a demo of this proposal, which you can find here: https://youtu.be/SRg_apFQaHE

Enhancement issue: #2579


Outstanding unresolved sections:

Alpha blockers:

  • The name of this feature
  • The mode labels (currently allow enforce, warning, audit)
  • SELinux policy (close to resolution)
  • Capabilities policy
  • Monitoring (needs input from @kubernetes/sig-instrumentation-approvers )
  • CSI inline volume restrictions

implementation-time decisions:

  • Timeout & limit on namespace update warnings
  • Metric version label cardinality

Beta blockers:

  • Deprecation / removal policy for old profile versions
  • Ephemeral containers handling
  • Windows pod handling
  • PSP migration workflow & support

Required approvals:

@tallclair tallclair added sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/security Categorizes an issue or PR as relevant to SIG Security. labels Mar 22, 2021
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 22, 2021
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 22, 2021
version._

Note that policies are not guaranteed to be backwards compatible, and a newer restricted policy
could require setting a field that doesn't exist in the current API version.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like an option for a perma-break. However, I don't see a practical way of knowing the version of a cluster. I don't believe we force behavior for /version for conformance purposes.

Copy link
Member

@liggitt liggitt Mar 24, 2021

Choose a reason for hiding this comment

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

discussed this in the 3/24 breakout meeting without arriving at a specific conclusion. thought about various mechanisms for communicating the version of the cluster to the webhook:

  • check /version (unclear that is guaranteed to return the kubernetes version)
  • configure it in the webhook manifest (only works if you remember to keep the webhook in sync, makes skew during upgrade hard, only works if a single API server is talking to the webhook)
  • configure it in the webhook invocation (e.g. webhook path)
  • add server Kubernetes version into admission review

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 thought that we did resolve this? The conclusion I drew from our discussion was that under the webhook implementation, policy versions would be tied to the webhook version, not the cluster version.

Copy link
Member

Choose a reason for hiding this comment

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

That's certainly the easiest, implementation-wise, but makes for tricky ordering on upgrade... if you upgrade the webhook first, then its latest restricted policy can start requiring fields to be set that the calling server might not be capable of setting yet.

I think the webhook implementation is likely secondary, so I'm ok saying that the webhook library version determines the meaning of latest, but we need to clearly document the expected order on upgrades between API server and webhook.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 11, 2021
Copy link
Member

@enj enj left a comment

Choose a reason for hiding this comment

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

Minor comments.

/lgtm


The following audit annotations will be added:

1. `pod-security.kubernetes.io/enforce-policy = <policy_level>:<resolved_version>` Record which policy was evaluated
Copy link
Member

Choose a reason for hiding this comment

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

Curious why level and version a munged into one key instead of two.

Copy link
Member Author

Choose a reason for hiding this comment

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

It cuts down on the size & verbosity of the logs. It's useful to keep them separate on labels because of the limitations of label selectors, but most log processors that I've seen can handle separating these values if need be. Is there a reason you'd want to see them separated?

Copy link
Member

Choose a reason for hiding this comment

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

This just seemed inconsistent with the labels used on namespaces.


_Blocking for Beta._

How long will old profiles be kept for? What is the removal policy?
Copy link
Member

Choose a reason for hiding this comment

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

+1 on keeping forever.

Comment on lines +168 to +173
- Using labels enables various workflows around policy management through kubectl, for example
issuing queries like `kubectl get namespaces -l
pod-security.kubernetes.io/enforce-version!=v1.22` to find namespaces where the enforcing
policy isn't pinned to the most recent version.
- Keeping the options on namespaces allows atomic create-and-set-policy, as opposed to creating a
namespace and then creating a second object inside the namespace.
Copy link
Member

Choose a reason for hiding this comment

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

If we have a separate cluster scoped object called PodSecurity where the name of object matches the name of the namespace, one interesting property is that authorization to write said PodSecurity is distinct from write on namespace.

That being said, I like the flexibility and UX of the label based approach.

So should we define extra authorization (SAR checks with virtual verbs similar to PSP's use) checks needed to set these labels and enforce them in admission?

Copy link
Member Author

Choose a reason for hiding this comment

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

So should we define extra authorization (SAR checks with virtual verbs similar to PSP's use) checks needed to set these labels and enforce them in admission?

I think there is a use case for generic label policy, and I'd be interested in a proposal for it, but IMO we shouldn't implement something special for pod security.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we have a separate cluster scoped object called PodSecurity where the name of object matches the name of the namespace, one interesting property is that authorization to write said PodSecurity is distinct from write on namespace.

I feel like we discussed this before, but I can't remember if there were any concerns aside from losing out on the label-selector UX. If we went this route, we'd probably want to add a custom kubectl command for it, but it would need to be fairly complicated to cover all the use cases we'd get for free with the label selector.

Copy link
Member

Choose a reason for hiding this comment

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

If we have a separate cluster scoped object called PodSecurity where the name of object matches the name of the namespace, one interesting property is that authorization to write said PodSecurity is distinct from write on namespace.

I feel like we discussed this before, but I can't remember if there were any concerns aside from losing out on the label-selector UX. If we went this route, we'd probably want to add a custom kubectl command for it, but it would need to be fairly complicated to cover all the use cases we'd get for free with the label selector.

Note that this mostly a thought exercise of what we lose by using label selectors instead of a distinct object.

Copy link
Member

Choose a reason for hiding this comment

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

So should we define extra authorization (SAR checks with virtual verbs similar to PSP's use) checks needed to set these labels and enforce them in admission?

I think there is a use case for generic label policy, and I'd be interested in a proposal for it, but IMO we shouldn't implement something special for pod security.

I do not know if I buy the "let us wait for the generic label policy" approach, especially since we want the pod security stuff to mostly be static. Having a one-off approach for pod security that encodes some well known permissions for the pod security labels could provide significant value and safety to this feature, especially in environments where users are allowed to provision namespaces.

Comment on lines +743 to +744
We are targeting GA in v1.24 to allow for migration off PodSecurityPolicy before it is removed in
v1.25.
Copy link
Member

Choose a reason for hiding this comment

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

While I understand the rationale, this seems aggressive for exactly the wrong reason. 😞

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. We shouldn't rush it just for the sake of rushing it. If there are red flags, we'll hold it back. PSP is only beta, so I don't think it would be terrible if this was still beta in v1.24 (although I'd really like to get it past alpha).

keps/sig-auth/2579-psp-replacement/README.md Outdated Show resolved Hide resolved
SIG Auth Old automation moved this from In Review (v1.22) to In Progress (v1.22) May 11, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2021
@tabbysable
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2021
@IanColdwater
Copy link
Contributor

/approve 🎉

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, deads2k, enj, IanColdwater, tallclair

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

@sftim
Copy link
Contributor

sftim commented May 12, 2021

Once this is merged, do we want to go back and tweak the blog article about it?

@jsturtevant
Copy link
Contributor

/lgtm

@tallclair
Copy link
Member Author

That's a wrap!

/remove-hold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 12, 2021
@k8s-ci-robot k8s-ci-robot merged commit 7712007 into kubernetes:master May 12, 2021
SIG Auth Old automation moved this from In Progress (v1.22) to Closed / Done May 12, 2021
vadorovsky added a commit to lockc-project/lockc that referenced this pull request Jun 29, 2021
This change adds the runc wrapper which has a purpose of applying
policies set through Kubernetes namespaces. For now, it uses the
namespace labels proposed in the PSP Replacement KEP[0].

The runc wrapper checks whether the container which is about to be
started was scheduled by kubelet (by checking runc annotations set by
kubelet / runtime servers). If some non-default policy is set, that
policy is written for the particilar container to the BPF map, so then
BPF programs can be aware of that policy.

For now, there is no way of proving that those annotations really come
from Kubernetes. Coming up with some sane way of securely proving that
will be something to implement in a follow up work.

[0] kubernetes/enhancements#2582

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
vadorovsky added a commit to lockc-project/lockc that referenced this pull request Jun 29, 2021
This change adds the runc wrapper which has a purpose of applying
policies set through Kubernetes namespaces. For now, it uses the
namespace labels proposed in the PSP Replacement KEP[0].

The runc wrapper checks whether the container which is about to be
started was scheduled by kubelet (by checking runc annotations set by
kubelet / runtime servers). If some non-default policy is set, that
policy is written for the particilar container to the BPF map, so then
BPF programs can be aware of that policy.

For now, there is no way of proving that those annotations really come
from Kubernetes. Coming up with some sane way of securely proving that
will be something to implement in a follow up work.

[0] kubernetes/enhancements#2582

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
vadorovsky added a commit to lockc-project/lockc that referenced this pull request Jun 29, 2021
This change adds the runc wrapper which has a purpose of applying
policies set through Kubernetes namespaces. For now, it uses the
namespace labels proposed in the PSP Replacement KEP[0].

The runc wrapper checks whether the container which is about to be
started was scheduled by kubelet (by checking runc annotations set by
kubelet / runtime servers). If some non-default policy is set, that
policy is written for the particilar container to the BPF map, so then
BPF programs can be aware of that policy.

For now, there is no way of proving that those annotations really come
from Kubernetes. Coming up with some sane way of securely proving that
will be something to implement in a follow up work.

[0] kubernetes/enhancements#2582

Fixes: #3
Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
vadorovsky added a commit to lockc-project/lockc that referenced this pull request Jun 30, 2021
This change adds the runc wrapper which has a purpose of applying
policies set through Kubernetes namespaces. For now, it uses the
namespace labels proposed in the PSP Replacement KEP[0].

The runc wrapper checks whether the container which is about to be
started was scheduled by kubelet (by checking runc annotations set by
kubelet / runtime servers). If some non-default policy is set, that
policy is written for the particilar container to the BPF map, so then
BPF programs can be aware of that policy.

For now, there is no way of proving that those annotations really come
from Kubernetes. Coming up with some sane way of securely proving that
will be something to implement in a follow up work.

[0] kubernetes/enhancements#2582

Fixes: #3
Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
vadorovsky added a commit to lockc-project/lockc that referenced this pull request Jun 30, 2021
This change adds the runc wrapper which has a purpose of applying
policies set through Kubernetes namespaces. For now, it uses the
namespace labels proposed in the PSP Replacement KEP[0].

The runc wrapper checks whether the container which is about to be
started was scheduled by kubelet (by checking runc annotations set by
kubelet / runtime servers). If some non-default policy is set, that
policy is written for the particilar container to the BPF map, so then
BPF programs can be aware of that policy.

For now, there is no way of proving that those annotations really come
from Kubernetes. Coming up with some sane way of securely proving that
will be something to implement in a follow up work.

[0] kubernetes/enhancements#2582

Fixes: #3
Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
vadorovsky added a commit to lockc-project/lockc that referenced this pull request Jul 7, 2021
This change adds the runc wrapper which has a purpose of applying
policies set through Kubernetes namespaces. For now, it uses the
namespace labels proposed in the PSP Replacement KEP[0].

The runc wrapper checks whether the container which is about to be
started was scheduled by kubelet (by checking runc annotations set by
kubelet / runtime servers). If some non-default policy is set, that
policy is written for the particilar container to the BPF map, so then
BPF programs can be aware of that policy.

For now, there is no way of proving that those annotations really come
from Kubernetes. Coming up with some sane way of securely proving that
will be something to implement in a follow up work.

[0] kubernetes/enhancements#2582

Fixes: #3
Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
vadorovsky added a commit to lockc-project/lockc that referenced this pull request Jul 8, 2021
This change adds the runc wrapper which has a purpose of applying
policies set through Kubernetes namespaces. For now, it uses the
namespace labels proposed in the PSP Replacement KEP[0].

The runc wrapper checks whether the container which is about to be
started was scheduled by kubelet (by checking runc annotations set by
kubelet / runtime servers). If some non-default policy is set, that
policy is written for the particilar container to the BPF map, so then
BPF programs can be aware of that policy.

For now, there is no way of proving that those annotations really come
from Kubernetes. Coming up with some sane way of securely proving that
will be something to implement in a follow up work.

[0] kubernetes/enhancements#2582

Fixes: #3
Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
vadorovsky added a commit to lockc-project/lockc that referenced this pull request Jul 8, 2021
This change adds the runc wrapper which has a purpose of applying
policies set through Kubernetes namespaces. For now, it uses the
namespace labels proposed in the PSP Replacement KEP[0].

The runc wrapper checks whether the container which is about to be
started was scheduled by kubelet (by checking runc annotations set by
kubelet / runtime servers). If some non-default policy is set, that
policy is written for the particilar container to the BPF map, so then
BPF programs can be aware of that policy.

For now, there is no way of proving that those annotations really come
from Kubernetes. Coming up with some sane way of securely proving that
will be something to implement in a follow up work.

[0] kubernetes/enhancements#2582

Fixes: #3
Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/security Categorizes an issue or PR as relevant to SIG Security. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
Archived in project
SIG Auth Old
Closed / Done
Development

Successfully merging this pull request may close these issues.

None yet