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.

Sign up for GitHub

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

Jump to bottom

Enforce service account node audience restriction #128077

Merged
merged 2 commits into from
Nov 6, 2024
Merged

Enforce service account node audience restriction #128077

merged 2 commits into from
Nov 6, 2024

Conversation

aramase
Copy link
Member

@aramase aramase commented Oct 15, 2024 •
edited
Loading

Enforce service account node audience restriction

  • Adds ServiceAccountNodeAudienceRestriction feature gate as beta (enabled by default).

Today we don't do any validation on the audience value that the kubelet requested -> we only check that the SA is in use on some pod scheduled to the kubelet. As part of this KEP, we're introducing a new feature gate ServiceAccountNodeAudienceRestriction in KAS to validate the audience value that the kubelet requests is either part of any API spec

/kind feature
/sig auth
/triage accepted
/milestone v1.32
/priority important-soon

NodeRestriction admission now validates the audience value that kubelet is requesting a service account token for is part of the pod spec volume. This change is introduced with a new kube-apiserver featuregate `ServiceAccountNodeAudienceRestriction` that's enabled by default.

[KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-auth/4412-projected-service-account-tokens-for-kubelet-image-credential-providers/README.md

@k8s-ci-robot
Copy link
Contributor

@aramase: You must be a member of the kubernetes/milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Milestone Maintainers Team and have them propose you as an additional delegate for this responsibility.

In response to this:

Enforce service account node audience restriction

  • Adds ServiceAccountNodeAudienceRestriction feature gate as beta (enabled by default).

Today we don't do any validation on the audience value that the kubelet requested -> we only check that the SA is in use on some pod scheduled to the kubelet. As part of this KEP, we're introducing a new feature gate ServiceAccountNodeAudienceRestriction in KAS to validate the audience value that the kubelet requests is either part of any API spec

/kind feature
/sig auth
/triage accepted
/milestone v1.32
/priority important-soon

TODO

[KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-auth/4412-projected-service-account-tokens-for-kubelet-image-credential-providers/README.md

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. sig/auth Categorizes an issue or PR as relevant to SIG Auth. triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 15, 2024
@aramase aramase changed the title Enforce service account node audience restriction [WIP] Enforce service account node audience restriction Oct 15, 2024
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 15, 2024
@aramase aramase force-pushed the aramase/f/kep_4412_sa_node_aud_restriction branch 3 times, most recently from f82556b to 8172295 Compare October 16, 2024 19:24
@aramase aramase changed the title [WIP] Enforce service account node audience restriction Enforce service account node audience restriction Oct 16, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 16, 2024
@aramase
Copy link
Member Author

aramase commented Oct 16, 2024

/assign enj liggitt

enj
enj suggested changes Oct 17, 2024
View reviewed changes
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.

First pass.

plugin/pkg/admission/noderestriction/admission.go Outdated Show resolved Hide resolved
plugin/pkg/admission/noderestriction/admission.go Outdated Show resolved Hide resolved
plugin/pkg/admission/noderestriction/admission.go Outdated Show resolved Hide resolved
plugin/pkg/admission/noderestriction/admission.go Outdated Show resolved Hide resolved
plugin/pkg/admission/noderestriction/admission.go Show resolved Hide resolved
plugin/pkg/admission/noderestriction/admission_test.go Show resolved Hide resolved
plugin/pkg/admission/noderestriction/admission_test.go Outdated Show resolved Hide resolved
plugin/pkg/admission/noderestriction/admission_test.go Outdated Show resolved Hide resolved
plugin/pkg/admission/noderestriction/admission_test.go Outdated Show resolved Hide resolved
plugin/pkg/admission/noderestriction/admission_test.go Outdated Show resolved Hide resolved
@enj
Copy link
Member

enj commented Oct 17, 2024

/milestone v1.32

@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Oct 17, 2024
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 18, 2024
@dipesh-rawat dipesh-rawat mentioned this pull request Oct 21, 2024
Open
9 tasks
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 5f991c042629107ff4c66127fbbd5aa5785ea1f9

@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.

@aramase
Copy link
Member Author

aramase commented Nov 4, 2024

/label api-review

@k8s-ci-robot k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Nov 4, 2024
@liggitt
Copy link
Member

liggitt commented Nov 6, 2024

/remove-label api-review

I don't think this is touches API files

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed api-review Categorizes an issue or PR as actively needing an API review. labels Nov 6, 2024
@liggitt
Copy link
Member

liggitt commented Nov 6, 2024 •
edited
Loading

Logic looks fine, a couple nits about output. Only outstanding question is the impact of the PVC informer... I suspect that's ~fine but wanted to raise the impact explicitly

https://github.com/kubernetes/enhancements/blob/master/keps/sig-auth/4412-projected-service-account-tokens-for-kubelet-image-credential-providers/README.md#will-enabling--using-this-feature-result-in-non-negligible-increase-of-resource-usage-cpu-ram-disk-io--in-any-components specifically might need updating if kube-apiserver will now watch PVCs when it didn't previously

@aramase aramase force-pushed the aramase/f/kep_4412_sa_node_aud_restriction branch from 203e3d7 to f1de7c8 Compare November 6, 2024 17:34
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2024
@k8s-ci-robot k8s-ci-robot requested a review from liggitt November 6, 2024 17:34
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2024
@aramase
Copy link
Member Author

aramase commented Nov 6, 2024

Logic looks fine, a couple nits about output. Only outstanding question is the impact of the PVC informer... I suspect that's ~fine but wanted to raise the impact explicitly

https://github.com/kubernetes/enhancements/blob/master/keps/sig-auth/4412-projected-service-account-tokens-for-kubelet-image-credential-providers/README.md#will-enabling--using-this-feature-result-in-non-negligible-increase-of-resource-usage-cpu-ram-disk-io--in-any-components specifically might need updating if kube-apiserver will now watch PVCs when it didn't previously

thanks for the review, @liggitt! I've updated the PR to resolve the comments and opened kubernetes/enhancements#4953 to update the KEP.

@aramase
Add ServiceAccountNodeAudienceRestriction feature gate
51f7475 
Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
@aramase
Enforce sa token node audience restriction when ServiceAccountNodeAud…
e93d5d5 
…ienceRestriction=true

Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
@aramase aramase force-pushed the aramase/f/kep_4412_sa_node_aud_restriction branch from f1de7c8 to e93d5d5 Compare November 6, 2024 17:51
@liggitt
Copy link
Member

liggitt commented Nov 6, 2024

/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 Nov 6, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 49564479529a548e9c43d1bf2f10cb3f50fd5920

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aramase, enj, 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 Nov 6, 2024
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 6, 2024 •
edited
Loading

@aramase: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-e2e-gce-cos-alpha-features e93d5d5 link false /test pull-kubernetes-e2e-gce-cos-alpha-features

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

@aramase
Copy link
Member Author

aramase commented Nov 6, 2024

/retest-required

@k8s-ci-robot k8s-ci-robot merged commit 28900b8 into kubernetes:master Nov 6, 2024
14 of 16 checks passed
@aramase aramase deleted the aramase/f/kep_4412_sa_node_aud_restriction branch November 6, 2024 23:18
This was referenced Nov 7, 2024
Open
Merged
@liggitt
Copy link
Member

liggitt commented Dec 2, 2024

For visibility, this broke one of our node daemons using kubelet credentials to provision workload identity tokens for a workload identity audience. Before the escape hatch graduates / locks on, we either need a reasonable path to giving scoped service account token creation access to a daemonset (something along the lines of kubernetes/enhancements#4935) or a way to indicate audiences that should be allowed in addition to the API-visible ones (similar to #128678 but possibly more generalized).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@micahhausler micahhausler micahhausler left review comments

@dims dims Awaiting requested review from dims

@tallclair tallclair Awaiting requested review from tallclair

@msau42 msau42 Awaiting requested review from msau42

@saad-ali saad-ali Awaiting requested review from saad-ali

@enj enj Awaiting requested review from enj

@liggitt liggitt Awaiting requested review from liggitt

Assignees

@liggitt liggitt

@enj enj

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/auth Categorizes an issue or PR as relevant to SIG Auth. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
API Reviews
Status: API review completed, 1.32
SIG Auth
Archived in project
[sig-release] Bug Triage
Archived in project
Milestone
v1.32
Development

Successfully merging this pull request may close these issues.
8 participants
@aramase @k8s-ci-robot @enj @wendy-ha18 @k8s-triage-robot @liggitt @micahhausler @msau42