-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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-2862: Fine-grained Kubelet API Authorization #4760
base: master
Are you sure you want to change the base?
Conversation
vinayakankugoyal
commented
Jul 12, 2024
- One-line PR description: Adding a new KEP
- Issue link: Fine grained Kubelet API authorization #2862
- Other comments:
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vinayakankugoyal The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cc @tallclair |
cb4f550
to
8c66ffa
Compare
8c66ffa
to
b23608f
Compare
b23608f
to
97cef1b
Compare
97cef1b
to
e96fad8
Compare
ee772b8
to
a29ca76
Compare
a29ca76
to
2a92135
Compare
We propose a change to the Kubelet API authorization to provide more | ||
fine-grained control. Currently, the API uses a coarse authorization scheme, | ||
where actions like reading health status and the ability to exec into a pod | ||
require the same permission. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AIUI, this is a primarily change to the Node API. Does that sound right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a change to the kubelet API. So agents (think logging and monitoring) that interact with the kubelet directly.
- Introduce new subresources for `/healthz` and `/pods/` kubelet endpoints, | ||
allowing for more granular authorization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this right?
- Introduce new subresources for `/healthz` and `/pods/` kubelet endpoints, | |
allowing for more granular authorization. | |
- Introduce new subresources for a Node, `healthz` and `pods`, that are proxied to the kubelet endpoints | |
`/healthz` and `/pods/` respectively. Making this change allowsfor more granular authorization. |
its user is bound to the `system:kubelet-api-admin` role. When the feature-gate | ||
is enabled we will update the `ClusterRole` as follow:- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RBAC is an optional authz mode and we typically can't assume it's active in a cluster. Of course, nearly all clusters do enable RBAC. Anyway:
its user is bound to the `system:kubelet-api-admin` role. When the feature-gate | |
is enabled we will update the `ClusterRole` as follow:- | |
in a cluster with RBAC enabled, then its user is mapped to the `system:kubelet-api-admin` | |
ClusterRole. | |
For a cluster with the `KubeletFineGrainedAuthz` feature gate enabled, the | |
`system:kubelet-api-admin` ClusterRole could be changed to look like: |
- apiGroups: | ||
- "" | ||
resources: | ||
- nodes | ||
verbs: | ||
- proxy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be allowing full access to proxy to all nodes. Is that what we want to demonstrate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This role already exists and has full access to proxy to all nodes. I just want to demonstrate the additional permissions we will add to this role.
<!-- | ||
Describe them, providing: | ||
- API call type (e.g. PATCH pods) | ||
- estimated throughput | ||
- originating component(s) (e.g. Kubelet, Feature-X-controller) | ||
Focusing mostly on: | ||
- components listing and/or watching resources they didn't before | ||
- API calls that may be triggered by changes of some Kubernetes resources | ||
(e.g. update of object X triggers new updates of object Y) | ||
- periodic API calls to reconcile state (e.g. periodic fetching state, | ||
heartbeats, leader election, etc.) | ||
--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(optionally)
<!-- | |
Describe them, providing: | |
- API call type (e.g. PATCH pods) | |
- estimated throughput | |
- originating component(s) (e.g. Kubelet, Feature-X-controller) | |
Focusing mostly on: | |
- components listing and/or watching resources they didn't before | |
- API calls that may be triggered by changes of some Kubernetes resources | |
(e.g. update of object X triggers new updates of object Y) | |
- periodic API calls to reconcile state (e.g. periodic fetching state, | |
heartbeats, leader election, etc.) | |
--> | |
For some requests, the API server performs an additional SubjectAccessReview (only when a check | |
for the `proxy` subresource wasn't authorized). |
e5283d2
to
9207765
Compare
Signed-off-by: Vinayak Goyal <vinaygo@google.com>
9207765
to
608bac3
Compare
/assign @liggitt |
### Notes | ||
|
||
If this change were to be implemented as proposed, the `proxy` `subresource` | ||
would still cover `/attach/`, `/exec/`, `/run/`, `/configz/`, `/debug/`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the motivation to keep /configz/
under proxy
? Reading config maybe useful for monitoring tools
| /pods/ | proxy | proxy, pods | | ||
| /runningpods/ | proxy | proxy, pods | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we documented those endpoints or have any stability guarantees. Introducing special permissions for those make those endpoints official.
We need to discuss in SIG Node forum whether we want to document those endpoints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are the other endpoints documented? Can you point me to those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to update this thread: we will have a discussion on SIG Node meeting next week
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc/ @tallclair
Thank you @vinayakankugoyal for this proposal. LGTM. |
/assign |
/cc |