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

policy exceptions #33

Merged
merged 7 commits into from
Dec 22, 2022
Merged

policy exceptions #33

merged 7 commits into from
Dec 22, 2022

Conversation

Eileen-Yu
Copy link
Contributor

@Eileen-Yu Eileen-Yu commented Sep 16, 2022

@Eileen-Yu Eileen-Yu changed the title docs: propose policy exception [WIP] docs: propose policy exception Sep 16, 2022
@chipzoller
Copy link
Member

Hi @Eileen-Yu , please complete the DCO sign-off for this and all future commits as required by CNCF.

proposals/allow_policy_exception.md Outdated Show resolved Hide resolved
proposals/allow_policy_exception.md Outdated Show resolved Hide resolved
proposals/allow_policy_exception.md Outdated Show resolved Hide resolved
proposals/allow_policy_exception.md Outdated Show resolved Hide resolved
proposals/allow_policy_exception.md Outdated Show resolved Hide resolved
proposals/allow_policy_exception.md Show resolved Hide resolved
proposals/allow_policy_exception.md Outdated Show resolved Hide resolved
@JimBugwadia JimBugwadia changed the title [WIP] docs: propose policy exception [WIP] policy exceptions Sep 20, 2022
@JimBugwadia JimBugwadia marked this pull request as draft September 20, 2022 22:46
@Eileen-Yu Eileen-Yu changed the title [WIP] policy exceptions policy exceptions Sep 26, 2022
@Eileen-Yu
Copy link
Contributor Author

Eileen-Yu commented Sep 27, 2022

Since the KDP has been updated, mark all outdated comments as resolved.

Copy link
Member

@chipzoller chipzoller left a comment

Choose a reason for hiding this comment

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

Looks pretty good.

Is it also possible to record for the admin to see which user/ServiceAccount/group created the exception request?

proposals/allow_policy_exception.md Outdated Show resolved Hide resolved
proposals/allow_policy_exception.md Show resolved Hide resolved
proposals/allow_policy_exception.md Outdated Show resolved Hide resolved
proposals/allow_policy_exception.md Outdated Show resolved Hide resolved
@eddycharly
Copy link
Member

@JimBugwadia @Eileen-Yu it looks to me that this KDP introduces two different concepts:

  1. adds a concept to kyverno to bypass policies based on exceptions
  2. adds a concept outside of kyverno to request/approve policy exceptions

The first concept does not need to know about the second, to make it work we just a new CRD (PolicyException ?) and implement the exception mechanism in kyverno.

The second concept is more an organisational concept, like different populations with different responsibilities (devs can create policy exception requests, ops can review the requests and approve/reject them).
For this second concept I don't think every organisation will have the same requirements and we could easily imagine different workflows for that.

In my opinion, the first concept needs to be in core kyverno while the second concept should come on top of it, it could even be totally separated from the kyverno repository (different controller, different CRDs, different chart, etc...).

If an organisation wants to manage the creation of policy exception workflows entirely differently he should be able to do it.

As we are talking about GitOps here, a very simple approach to manage the request/approval would be to use a separate git repo to policy exceptions and only ops can validate pull requests (this has the benefits of keeping history in git and so on).

WDYT ?

@JimBugwadia
Copy link
Member

@eddycharly we had started with a scheme like that but it seemed to lead to synchronous workflows.

It would be nice to simply have a PolicyException object and defer the approval lifecycle to external tools.

However this requires that the PolicyException resource be secured and handled with care. Which implies that developers cannot create asynchronously, as part of their workloads.

One idea was to require signed exceptions as a protection mechanism - but that creates coupling to the YAML signing capabilities.

Perhaps we can capture this as an alternate design and discuss further.

@eddycharly
Copy link
Member

eddycharly commented Sep 30, 2022

However this requires that the PolicyException resource be secured and handled with care. Which implies that developers cannot create asynchronously, as part of their workloads.

Kyverno should only read PolicyExceptions and we could argue that the only one service account that should be able to create them is the controller responsible for approving PolicyExceptionRequests.

I mean, not every organisation is going to need to this request/approval mechanism, this makes sense to me that it is not coupled with core kyverno.

Most of the time developers have limited permissions on a cluster, and if Argocd is used for example that's very easy to enforce permissions at argocd level too. I find PolicyExceptions useful but if I had to use it in a GitOps workflow I feel like it could be a pain. I would rather expect that PolicyExceptions are created from a specific repository where developers can open PRs and let Argo sync them automatically, GitOps means we don't need to connect to the cluster at all.

With this design it requires an Admin to create the approval when a request is made which is not what all users will want/need. It makes sense for some not for all, we should design that can adapt to different use cases while not restricting others ones IMHO.

cc @JimBugwadia @Eileen-Yu

@JimBugwadia
Copy link
Member

JimBugwadia commented Sep 30, 2022

@eddycharly - in this alternate approach, are you proposing 2 CRs for Kyverno (a request and a response) or one (an externally approved exception)?

@eddycharly
Copy link
Member

@JimBugwadia from the kyverno POV only one PolicyException is enough.

The request/approval mechanism is separate and eventually, when a request is approved this will create the corresponding PolicyException. But for kyverno itself only PolicyExceptions matter.

@JimBugwadia
Copy link
Member

@eddycharly - this would certainly simplify the design.

However, this may not fully meet the initial request kyverno/kyverno#2627 - or at least seems to convert part of the requirements to something the user needs to solve outside of Kyverno.

Perhaps a way to address would be to:

  1. Start with the approach where Kyverno simply uses a PolicyException (we can provide configuration options for which namespaces, etc. to use for exceptions)
  2. Ability to require signed exceptions (i.e. a YAML signature) to "approve" exceptions and prevent tampering. This could simply be a matter of configuring a policy.
  3. Then if needed revisit whether some in-cluster approvals are required.

@stone-z
Copy link

stone-z commented Nov 24, 2022

Following up from a Slack thread, just a brief comment about use cases for this feature. This is intentionally short, happy to expand or join a call to elaborate if needed.

I think this concept is great, and the right direction to be able to accommodate complex automation alongside Kyverno. There are some use cases, however, which I anticipate will be desired but not met by this design, or at least I haven't worked out exactly what the intended workflow would be. Things like:

  • Use case: Admin wants to exclude a list of trusted images from PSS policies
    • Problem 1: exclude blocks can't reference the image used in a resource (only name/namespace and selectors)
    • Problem 2: exclude blocks can't use contexts, so no possibility to reference e.g. a list in a ConfigMap
    • Workaround: Admin creates individual exclusions (or static list) for Deployments instead of images
  • Use case: Admin wants to create individual exclusions for Deployments from PSS policies ^
    • Problem 1: PSS policies are written at the Pod level, so:
      • Admin can't exclude them based on Deployment name/selectors
      • If excluding by Pod name (using a wildcard, since Pod names usually aren't known at admission time), autogen is disabled for that policy
    • Workaround: Admin must exclude Pod by name using a wildcard, leading to:
    • Problem 2: Users who can create Pods can now bypass PSS policies by creating a specially-named Pod

Suggestions:

  • Support exclusions based on parent resource names/selectors (e.g. exclude Pod from PSS policy based on Deployment name)
  • Support or somehow enable contexts+preconditions set via PERs

I might have missed something as I'm very much still trying to wrap my head around exceptions in a post-PSP, out-of-tree world. Happy to chat and expand more if needed. Thanks for such a great project!

@JimBugwadia
Copy link
Member

Hi @stone-z - thanks for the review and feedback! Some thoughts and questions below:

Use case: Admin wants to exclude a list of trusted images from PSS policies

Are you using the new validate.podSecurity rule or the individual pod security policies?

When excluding images from PSS controls, it seems like its best to be fine grained and exclude an image for a specific control. The validate.podSecurity rule allows that.

What would be the benefit of managing the image & control pairs externally vs directly in the policy?

Use case: Admin wants to create individual exclusions for Deployments from PSS policies

This should be possible. @Eileen-Yu - can you please share the sample for excluding a Deployment and the pods it controls?

Eileen-Yu and others added 6 commits December 21, 2022 18:10
Signed-off-by: Eileen <eileenylj@gmail.com>
Signed-off-by: Jim Bugwadia <jim@nirmata.com>
Signed-off-by: Eileen <eileenylj@gmail.com>
Signed-off-by: Jim Bugwadia <jim@nirmata.com>
Signed-off-by: Eileen <eileenylj@gmail.com>
Signed-off-by: Eileen <eileenylj@gmail.com>
Signed-off-by: Eileen <eileenylj@gmail.com>
@Eileen-Yu
Copy link
Contributor Author

Hi @stone-z Thx for your feedback!

For the second use case you mentioned,

  • Use case: Admin wants to create individual exclusions for Deployments from PSS policies ^

Considering on the case of autogen, we would introduce some best practices for the users to specify exceptions.
As for the exclusions for Deployments, you can:

---
apiVersion: kyverno.io/v2alpha1
kind: PolicyException
metadata:
  name: bypass-test-deployment
  namespace: kyverno
spec:
  exceptions:
    - policyName: require-labels
      ruleNames:
        - check-for-labels
        - autogen-check-for-labels
  match:
    any:
      - resources:
          kinds:
            - Deployment
          names:
            - nginx
          namespaces:
            - default
      - resources:
          kinds:
            - Pod
          namespaces:
            - default
          selector:
            matchLabels:
              app: nginx

@JimBugwadia JimBugwadia self-requested a review December 22, 2022 19:27
@chipzoller chipzoller merged commit b1ae124 into kyverno:main Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants