-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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
remove secret-based sa token client builder #99291
Conversation
/sig auth |
082f57d
to
4ebe5cf
Compare
AuthenticationClient: c.Client.AuthenticationV1(), | ||
Namespace: metav1.NamespaceSystem, | ||
} | ||
c.ClientBuilder = clientbuilder.NewDynamicClientBuilder( |
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 means the user running the cloud controller manager needs serviceaccount/token create permissions. This is included in the system:kube-controller-manager
role... is that what the cloud controller manager process is expected to be bound to?
cc @jpbetz
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 believe the KCM has these permission because its running the ServiceAccountController. The CCM is not running the ServiceAccountController, so ideally it should not have or need these elevated permissions. Ideally I think we should find a way to separate the controllers which need these elevated permissions from the KCM to minimize the surface area of privilege escalation attack.
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.
SAController is not the reason that KCM needs serviceaccount/token create permission. some volumes related controller requires that permission, but more direct,
UseServiceAccountCredentials bool |
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.
The CCM is not running the ServiceAccountController, so ideally it should not have or need these elevated permissions
It makes sense for CCM to be able to request tokens specifically for the service accounts for the control loops in the CCM. That is possible to grant in a narrow way (grant "serviceaccounts/tokens" on just the resourceNames for the expected service accounts), but I couldn't find where the CCM role is defined.
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 was (understandably) unable to convince anyone to put the CCM SA as part of the standard bootstrap as its not a core component of K8s. So its up to each cloud provider to set this up. So for example the GCP setup can be found at https://github.com/kubernetes/cloud-provider-gcp/blob/0e9c8d0151f46b5a6c0beea7778c034c6984d5d0/cluster/addons/cloud-controller-manager/cloud-node-controller-binding.yaml#L13.
/lgtm update the release note with info about the cloud-controller-manager once you get feedback from @jpbetz |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, zshihang 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 |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
since
TokenRequest
is GAed, we don't need this client builder anymoreDoes this PR introduce a user-facing change?