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

KEP-3488: Implement secondary authz for ValidatingAdmissionPolicy #116054

Merged
merged 3 commits into from
Mar 6, 2023

Conversation

jpbetz
Copy link
Contributor

@jpbetz jpbetz commented Feb 25, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implements the Secondary Authz section of CEL for Admission Control.

Example CEL expressions:

authorizer.group('').resource('pods').namespace('default').check('create').allowed()
authorizer.path('/healthz').check('GET').allowed()
authorizer.requestResource.check('my-custom-verb').allowed()

Special notes for your reviewer:

This accounts for a few things missed in the KEP, namely:

  • Authz uses resources, not kinds
  • A group must be specified for resource authorization, so the builder functions have been adjusted to accomodate
  • Authz decisions are tri-state: "allow", "no opinion" and "deny". I've tried to document this. Do we also need a "noOpinion()" function along side "allowed()" and "denied()" ?
  • Added a "reason()" function to get the decision reason, which may be useful in messages

Does this PR introduce a user-facing change?

Added authorization check support to the CEL expressions of ValidatingAdmissionPolicy via a `authorizer`
variable with expressions. The new variable provides a builder that allows expressions such `authorizer.group('').resource('pods').check('create').allowed()`.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


/sig api-machinery
/priority important-soon

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 25, 2023
@k8s-ci-robot k8s-ci-robot added area/apiserver area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Feb 25, 2023
@jpbetz
Copy link
Contributor Author

jpbetz commented Feb 25, 2023

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 25, 2023
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/code-generation kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 25, 2023
@jpbetz
Copy link
Contributor Author

jpbetz commented Feb 25, 2023

/retest

@jpbetz jpbetz changed the title Implement secondary authz for ValidatingAdmissionPolicy KEP-3488: Implement secondary authz for ValidatingAdmissionPolicy Feb 25, 2023
@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@sftim
Copy link
Contributor

sftim commented Feb 27, 2023

Changelog suggestion:

Added authorization check support to the CEL expressions of ValidatingAdmissionPolicy via a `authorizer`
variable with expressions. The new variable provides a builder that allows expressions such as `authorizer.group('').resource('pods').check('create').allowed()`.

@sftim
Copy link
Contributor

sftim commented Feb 27, 2023

It's not obvious to me how this works. Does this have any interaction with SubjectAccessReview - perhaps via check()?
Alternatively, is this exposing the internals of a particular implementation of the Kubernetes API?

How many HTTP requests to the authz backend does authorizer.group('').resource('pods').check('create').allowed() provoke if webhook authz is enabled? How about for:

authorizer.resource('signers', 'certificates.k8s.io', '*').name(oldObject.spec.signerName).check('approve').allowed() ||
authorizer.resource('signers', 'certificates.k8s.io', '*').name([oldObject.spec.signerName.split('/')[0],'*'].join('/')).check('approve').allowed()

?

You might like to sketch out the docs for this before we merge the code change @jpbetz, to make sure that we know what we need to explain.

Copy link
Contributor

@deads2k deads2k left a comment

Choose a reason for hiding this comment

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

This fell out very nicely. I think the last big question is whether we need to offer a means to specify the username to check the authorizer against. We've used serviceaccounts in the past and doing so using a method .forServiceAccount(namespace, name string) or some-such that restricts specifying any username could encourage patterns that have worked out well in the past without opening up for behavior we want to discourage.

@jpbetz
Copy link
Contributor Author

jpbetz commented Feb 27, 2023

You might like to sketch out the docs for this before we merge the code change @jpbetz, to make sure that we know what we need to explain.

Good call @sftim , I've added docs for the Kubernetes CEL libraries, including this one, to kubernetes/website#39642. I've attempted to answer the questions you asked there.

@jpbetz
Copy link
Contributor Author

jpbetz commented Feb 28, 2023

@deads2k Feedback addressed and this is ready for another pass when you are ready. Most importantly, service accounts are supported. Thanks for the review!

@jpbetz jpbetz force-pushed the secondary-authz branch from a3f9a96 to acd2443 Compare March 1, 2023 03:02
@jpbetz
Copy link
Contributor Author

jpbetz commented Mar 1, 2023

/label api-review

API changes: Adds an authorization variable and CEL functions available to ValidationAdmissionPolicy. Updates api docs to highlight the availability. No API types or fields are changed.

@jpbetz
Copy link
Contributor Author

jpbetz commented Mar 3, 2023

/retest

@jpbetz jpbetz force-pushed the secondary-authz branch 2 times, most recently from a370e49 to bdfff33 Compare March 3, 2023 18:29
@jpbetz
Copy link
Contributor Author

jpbetz commented Mar 3, 2023

Rebased and API review feedback applied

@liggitt
Copy link
Member

liggitt commented Mar 3, 2023

API bits lgtm, just a few questions on wiring / interfaces / exposed surface area

@liggitt
Copy link
Member

liggitt commented Mar 6, 2023

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 5036f589367fbbe8f1f1e9803645263ee475e6ca

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, liggitt

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2023
@liggitt
Copy link
Member

liggitt commented Mar 6, 2023

/hold might make sense to squash in review commits?

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 6, 2023
@jpbetz jpbetz force-pushed the secondary-authz branch from 304961a to 4d30c43 Compare March 6, 2023 17:09
@liggitt
Copy link
Member

liggitt commented Mar 6, 2023

/hold cancel
/lgtm

@jpbetz
Copy link
Contributor Author

jpbetz commented Mar 6, 2023

Rebased to a reasonable minimum set of commits (keeping codegen separate)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/code-generation area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. 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. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Status: API review completed, 1.27
Development

Successfully merging this pull request may close these issues.

8 participants