[OPRUN-4144] OLMv1: Make ServiceAccount Field Optional in the ClusterExtension API #1897
Conversation
Signed-off-by: Rashmi Gottipati <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@rashmigottipati: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
Spoken with Jordan sync on this, when reviewing this I don't necessarily have the most up to date "why" and so some comments may be based on an older understanding. Please point me in the right direction if I've missed the "why" here
|
|
||
| One of the core design principles of OLMv1 is Secure By Default. This enhancement aims at upholding and also improving upon that principle while addressing a usability issue. | ||
|
|
||
| The proposal is to make the `spec.serviceAccount` field in the ClusterExtension API optional in Tech Preview. For extensions without a ServiceAccount, OLMv1 uses a synthetic identity with zero permissions and relies on Kubernetes impersonation, allowing administrators to explicitly grant the necessary privileges. For extensions with a ServiceAccount, OLMv1 continues to use token based authentication, preserving backward compatibility. |
There was a problem hiding this comment.
For extensions with a ServiceAccount, OLMv1 continues to use token based authentication, preserving backward compatibility.
Why can you not switch the existing to impersonation? From a permissions perspective OLM would need to be able to impersonate service accounts. Does it not have that permission today?
There was a problem hiding this comment.
OLM already has a mechanism to impersonate ServiceAccounts, and this is implemented behind the SyntheticPermissions feature gate. The proposal retained the existing token based auth for backwards compatibility, but from a permissions perspective it's possible to fully switch to impersonation and remove all token based usage.
For now, we will be switching to impersonation only in TP to ensure it works well and is viable before fully switching to impersonation and eliminating the token based route.
|
|
||
| Currently, the `spec.serviceAccount` field is required on every `ClusterExtension`, which creates usability and security challenges. | ||
|
|
||
| From a usability standpoint, users must understand the exact permissions their extension requires, create a corresponding ServiceAccount, and configure ClusterRoleBindings appropriately. Ensuring that the service account has the correct permissions is a manual and often complex process, leading to failed installations and a frustrating experience, particularly for new users. User feedback indicates a strong preference to avoid configuring RBAC for each ClusterExtension, with some users resorting to granting cluster admin privileges just to satisfy the requirement. |
There was a problem hiding this comment.
How could you make this process less frustrating for users so that the least-privilege approach is not frustrating?
I've reviewed the docs that explain how this is done. It involves many steps including extracting manifests and merging different rules to create the appropriate Roles and ClusterRoles required.
It looks like it could be scripted. Why is a CLI utility (Extension of existing OLM CLI) not on the cards to automate this process and spit out the required least-privilege RBAC for the user?
There was a problem hiding this comment.
You're right that the existing process can be complex and frustrating for users trying to follow a least privilege approach. One way to make this process less frustrating is by making the spec.serviceAccount field optional and have two sets of extensions:
- not requiring permissions can run with synthetic identity with zero permissions, removing the need to create SA and configure RBAC
- requiring permissions, where the cluster admin can grant via RoleBindings and ClusterRoleBindings, or even synthetic user bindings keeping the least privilege design intact
Regarding scripting, I think in the future it would be possible to provide a CLI utility to generate the Roles/ClusterRoles and necessary bindings. It would require redesigning of the existing permission model. For now, it's outside of the scope of what this EP is trying to solve, as the main focus is to remove the mandatory SA requirement and ensure a safe default.
There was a problem hiding this comment.
not requiring permissions can run with synthetic identity with zero permissions, removing the need to create SA and configure RBAC
How can a CE require zero permissions? IIUC the synthetic identity is used to create all of the resources for the CE is it not? So it will need to be able to have permissions for deployments and other resources within the CE bundle?
Regarding scripting, I think in the future it would be possible to provide a CLI utility to generate the Roles/ClusterRoles and necessary bindings.
If you did this first, do you still believe you'd need this EP?
I think in discussion with Joe I've understood better what we are trying to achieve in this EP since I reviewed it yesterday. But AFAICT this EP currently uses the argument of "RBAC is hard to configure, so folks are doing highly privileged things" as a motivation to implement the synthetic groups.
If you instead were using an argument of "even if RBAC was easy to set up, some folks would still want to apply blanket permissions to a group and not care about individual CEs", I think it would create a stronger argument for why this feature is important
|
|
||
| Before arriving at this design, we evaluated simpler alternatives such as removing the field entirely and implicitly using a cluster-admin service account, or making the field optional with cluster-admin as the default. While these approaches improve ease of use, they conflict with the principle of least privilege. | ||
|
|
||
| By making the spec.serviceAccount field optional and introducing synthetic identities with zero permissions by default, this enhancement provides a safer and simpler installation experience. It preserves backward compatibility and still allows the use of custom ServiceAccounts when fine-grained control is needed, aligning usability with the principle of least privilege. |
There was a problem hiding this comment.
Up to this point I'm not seeing how this changes the complexity for an end user.
In the old world:
- They create a service account
- They have to create Roles/ClusterRoles and the appropriate bindings to add permissions to the service account
- They provide the service account to the ClusterExtension to use
The second bullet is the complex part
In the new world:
- They have to create Roles/ClusterRoles and the appropriate bindings to add permissions to the synthetic service account
The complex part still exists?
There was a problem hiding this comment.
I completely agree with your statement that the complex part is creating Roles/ClusterRoles and the corresponding bindings. The complex part still exists today, but only for extensions that require permissions, and this proposal does not eliminate that.
Whats changing is that, not all extensions would be forced to go through that complexity.
So in the old world:
Every extension must:
- create a service account
- create Roles/ClusterRoles
- bind them
- provide the SA to the ClusterExtension
In the new world:
Only extensions that need permissions require RBAC
So if an extension does not need any permissions, users dont have to do the below:
- create service account
- create any RBAC
- create the necessary bindings
- provide SA to the extension
There was a problem hiding this comment.
Only extensions that need permissions require RBAC
Can you provide an example of an extension that does not require any permissions? I'm struggling to think what that would look like
|
|
||
| ### User Stories | ||
|
|
||
| - As a cluster admin, I want extensions to run with zero privileges by default, so that the cluster remains secure unless I explicitly grant permissions |
There was a problem hiding this comment.
In the current world, I create a service account and bind nothing to it to achieve this, right?
|
|
||
| - As a cluster admin, I want extensions to run with zero privileges by default, so that the cluster remains secure unless I explicitly grant permissions | ||
| - As an extension author, I want to retain the ability to use custom ServiceAccounts for fine grained RBAC, allowing my extensions to operate with the privileges they need | ||
| - As a cluster admin, I want to grant permissions to multiple extensions using group bindings to apply the same permissions to all extensions without a ServiceAccount |
There was a problem hiding this comment.
This user story I'm not quite following. The Why part of the user story is missing, can you expand?
As a <persona>, I would like <something>, so that I can <achieve some goal>
(Nit, none of your user stories follow this format currently)
| #### Deletion Behavior | ||
|
|
||
| - **Extensions with a ServiceAccount:** | ||
| - Existing behavior applies; all resources owned by the CE are deleted, including associated RBAC |
There was a problem hiding this comment.
Why is the service account and RBAC associated to the service account deleted? They are not owned by the ClusterExtension, they are owned by the cluster admin
| - **Extensions with a ServiceAccount:** | ||
| - Existing behavior applies; all resources owned by the CE are deleted, including associated RBAC | ||
| - **Extensions without a ServiceAccount (synthetic identity):** | ||
| - No actual ServiceAccount exists, so OLM does not delete any RBAC bindings created by the admin, so admins are responsible for removing any ClusterRoleBindings or group bindings associated with synthetic identities if desired. |
There was a problem hiding this comment.
This is how it should be for service accounts too, I'm surprised it isn't
| - Document when to use group vs per extension bindings | ||
|
|
||
| ### Benefits | ||
| This approach satisfies both security and usability: zero-permission default when unset, explicit privilege delegation, simplified installation for new users (one manual step of RBAC binding instead of ServiceAccount + RBAC), and preserves backward compatibility. |
There was a problem hiding this comment.
- Service accounts have zero permissions by default
- They require explicit privilege delegation
- The act of creating a service account and binding to that is not particularly cumbersome when put in scope of the RBAC creation - we appear to solving the easy part not the complex part?
| - Installation requires extra manual step of creating RBAC | ||
| - Learning curve for synthetic identities | ||
|
|
||
| Choosing this approach despite these drawbacks as there are more advantages: secure by default, better UX than ServiceAccount+RBAC, simplified implementation. |
There was a problem hiding this comment.
I don't think the first two are real benefits given earlier comments.
On simplified implementation, I think we can improve the implementation of the service account token/impersonation story without having to make any breaking changes.
| 3. Keep SA field required, only add impersonation | ||
| - This doesn't solve usability problem | ||
| 4. Always use impersonation (even when SA is set) | ||
| - Breaks backward compatibility, requires more extensive migration and changes existing behavior |
There was a problem hiding this comment.
These are basically the same right?
Does OLM have the ability (RBAC) to impersonate already? Or is that something that would need to be added?
If it needs to be added, is that something that would require cluster admin intervention, or does CVO handle OLMs RBAC?
Do you have data on how many people are using OLMv1 in the field? If we required a one time RBAC change (create a binding allowing impersonation of a certain service account), how many people would actually be affected?
There was a problem hiding this comment.
Yes OLM has the ability to impersonate already. It's implemented behind the SyntheticPermissions feature gate.
I don't have the exact stats on how many people are using olmv1. But I will try to find out more about this.
From what I know so far, the adoption appears to be relatively low.
There was a problem hiding this comment.
Understanding those statistics might help us make informed decisions about what we can and cannot change at this point. If only 0.1% of clusters are affected then we may argue we can make broader/more disruptive changes and help that 0.1% with the change for example
2 participants
No description provided.