✨ Add SyntheticPermissions support and migrate to ServiceAccount impersonation #2351
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes the authentication mechanism for ClusterExtension management by replacing token-based ServiceAccount authentication with Kubernetes impersonation. This change significantly improves the security posture by eliminating the need for actual ServiceAccount resources to exist in the cluster, reducing the risk of credential misuse.
Key changes:
- Replaced TokenGetter/TokenInjectingRoundTripper with ServiceAccount impersonation using
transport.NewImpersonatingRoundTripper - Removed the SyntheticPermissions feature gate and related synthetic authentication code
- Simplified operator-controller RBAC to always use cluster-admin permissions (previously conditional on BoxcutterRuntime feature gate)
- Updated all documentation and samples to reflect that ServiceAccount resources should NOT be created
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| manifests/standard.yaml | Updated CRD documentation to remove "must exist" requirements for ServiceAccounts; changed ClusterRole to grant full cluster-admin permissions with wildcards |
| manifests/standard-e2e.yaml | Same CRD and ClusterRole updates as standard.yaml for e2e test environment |
| manifests/experimental.yaml | Updated CRD documentation and ClusterRole permissions; changed ClusterRoleBinding to bind to operator-controller-manager-role instead of cluster-admin |
| manifests/experimental-e2e.yaml | Same updates as experimental.yaml for e2e test environment |
| internal/operator-controller/features/features.go | Removed SyntheticPermissions feature gate definition |
| internal/operator-controller/controllers/clusterextension_controller.go | Removed special error handling for ServiceAccountNotFoundError |
| internal/operator-controller/authentication/tripper.go | Deleted entire file - token injection round tripper no longer needed |
| internal/operator-controller/authentication/tokengetter_test.go | Deleted test file for removed TokenGetter functionality |
| internal/operator-controller/authentication/tokengetter.go | Deleted TokenGetter implementation and ServiceAccountNotFoundError |
| internal/operator-controller/authentication/synthetic_test.go | Deleted test for removed synthetic user permissions |
| internal/operator-controller/authentication/synthetic.go | Deleted synthetic user authentication implementation |
| internal/operator-controller/authentication/service_account_impersonation.go | Added new function to create impersonation config for ServiceAccounts using proper system:serviceaccount username and groups |
| internal/operator-controller/action/restconfig_test.go | Updated tests to verify impersonation headers instead of token injection; removed synthetic user tests |
| internal/operator-controller/action/restconfig.go | Replaced token-based mapper with impersonation-based mapper; removed synthetic user logic |
| helm/olmv1/templates/rbac/clusterrolebinding-operator-controller-manager-rolebinding.yml | Simplified to always use operator-controller-manager-role binding; removed BoxcutterRuntime feature gate conditional |
| helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml | Changed from conditional with limited permissions to always-enabled with full cluster-admin wildcards; removed BoxcutterRuntime feature gate check |
| helm/olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml | Updated field documentation to remove "must exist" language for ServiceAccounts |
| helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml | Same CRD documentation updates as standard variant |
| hack/demo/synthetic-user-cluster-admin-demo-script.sh | Deleted demo script for removed synthetic user feature |
| docs/tutorials/install-extension.md | Added prominent security warning explaining impersonation and why ServiceAccount resources should NOT be created |
| docs/howto/derive-service-account.md | Updated title and content to focus on RBAC instead of ServiceAccount resources; added warnings against creating ServiceAccount resources |
| docs/draft/howto/use-synthetic-permissions.md | Deleted entire documentation file for removed feature |
| docs/api-reference/olmv1-api-reference.md | Updated API documentation to remove "must exist" requirements for ServiceAccounts |
| config/samples/olm_v1_clusterextension.yaml | Replaced ServiceAccount resource with detailed comment explaining impersonation approach and security rationale |
| cmd/operator-controller/main.go | Removed TokenGetter instantiation and SyntheticPermissions feature gate check; simplified to always use ServiceAccountRestConfigMapper |
| api/v1/clusterextension_types.go | Updated Go documentation to remove "must exist" language and clarify that only RBAC configuration is needed |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
trgeiger
reviewed
Nov 20, 2025
openshift-ci
bot
added
the
do-not-merge/work-in-progress
joelanford
force-pushed
the
sa-impersonation
branch
from
6bab0fc to
947c60e
Compare
openshift-merge-robot
added
the
needs-rebase
Copilot finished reviewing on behalf of
joelanford
November 21, 2025 06:13
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml
Outdated
Show resolved
Hide resolved
joelanford
force-pushed
the
sa-impersonation
branch
from
947c60e to
4a9dcce
Compare
joelanford
changed the title
joelanford
force-pushed
the
sa-impersonation
branch
from
4a9dcce to
841aedf
Compare
openshift-merge-robot
removed
the
needs-rebase
joelanford
force-pushed
the
sa-impersonation
branch
from
841aedf to
2c8372c
Compare
Copilot finished reviewing on behalf of
joelanford
November 21, 2025 16:00
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml
Show resolved
Hide resolved
|
|
||
| // ServiceAccountImpersonationConfig returns an ImpersonationConfig for impersonating a ServiceAccount. | ||
| // This allows authentication as a ServiceAccount without requiring an actual token. | ||
| func ServiceAccountImpersonationConfig(ext ocv1.ClusterExtension) transport.ImpersonationConfig { |
There was a problem hiding this comment.
ServiceAccountImpersonationConfig does not handle the case where ext.Spec.ServiceAccount.Name is empty. In the experimental channel, the serviceAccount field is now optional, but this function will produce an invalid username format system:serviceaccount:namespace: (with a trailing colon and no name) when the service account name is empty.
This function should either:
- Check if the name is empty and return an error, or
- Be guarded by callers to ensure it's only called when the service account name is non-empty
Looking at the code in cmd/operator-controller/main.go lines 674-683, the caller does check for empty name before calling this function in the userInfoMapper, but ServiceAccountRestConfigMapper() in internal/operator-controller/action/restconfig.go (lines 38-50) calls this function without checking if the service account name is empty. This could lead to invalid impersonation attempts.
Suggested change
| func ServiceAccountImpersonationConfig(ext ocv1.ClusterExtension) transport.ImpersonationConfig { | |
| func ServiceAccountImpersonationConfig(ext ocv1.ClusterExtension) transport.ImpersonationConfig { | |
| if ext.Spec.ServiceAccount.Name == "" { | |
| panic("ServiceAccountImpersonationConfig: ServiceAccount name is empty, cannot impersonate an empty service account") | |
| } |
Member
Author
There was a problem hiding this comment.
As long as the SyntheticPermissions feature gate is enabled alongside and the optionality of the ServiceAccount field, this should be all good.
However, I am not opposed to an explicit panic to make it easier for us to detect and avoid this issue. Thoughts from other maintainers?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2351 +/- ##
==========================================
- Coverage 74.23% 74.03% -0.20%
==========================================
Files 91 90 -1
Lines 7239 7199 -40
==========================================
- Hits 5374 5330 -44
- Misses 1433 1436 +3
- Partials 432 433 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joelanford
force-pushed
the
sa-impersonation
branch
from
2c8372c to
1c6b0f4
Compare
…ions This commit enhances the CRD generator with the following capabilities: - Support for <opcon:validation:Required> and <opcon:validation:Optional> tags to control field requirements in the generated CRDs - Support for <opcon:standard:description> tags to provide channel-specific descriptions (complementing experimental descriptions) - Refactored opconTweaks functions to accept parent schema, enabling modification of the required fields list These changes provide more flexibility in defining CRD schemas with channel-specific behavior and field requirements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
joelanford
force-pushed
the
sa-impersonation
branch
from
1c6b0f4 to
4cc2d52
Compare
Copilot finished reviewing on behalf of
joelanford
November 21, 2025 20:06
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| If serviceAccount is specified, OLM will authenticate as that service account. | ||
| Otherwise, operator-controller will authenticate as: | ||
| - User: "olm:clusterextensions:<clusterExtensionName>" |
There was a problem hiding this comment.
The synthetic user name format is incorrect. According to the implementation in internal/operator-controller/authentication/synthetic.go, the user name should be olm:clusterextension:<clusterExtensionName> (singular) not olm:clusterextensions:<clusterExtensionName> (plural). The group name correctly uses the plural form olm:clusterextensions.
Suggested change
| - User: "olm:clusterextensions:<clusterExtensionName>" | |
| - User: "olm:clusterextension:<clusterExtensionName>" |
manifests/experimental.yaml
Outdated
| If serviceAccount is specified, OLM will authenticate as that service account. | ||
| Otherwise, operator-controller will authenticate as: | ||
| - User: "olm:clusterextensions:<clusterExtensionName>" |
There was a problem hiding this comment.
The synthetic user name format is incorrect. According to the implementation in internal/operator-controller/authentication/synthetic.go, the user name should be olm:clusterextension:<clusterExtensionName> (singular) not olm:clusterextensions:<clusterExtensionName> (plural). The group name correctly uses the plural form olm:clusterextensions.
Suggested change
| - User: "olm:clusterextensions:<clusterExtensionName>" | |
| - User: "olm:clusterextension:<clusterExtensionName>" |
manifests/experimental-e2e.yaml
Outdated
| If serviceAccount is specified, OLM will authenticate as that service account. | ||
| Otherwise, operator-controller will authenticate as: | ||
| - User: "olm:clusterextensions:<clusterExtensionName>" |
There was a problem hiding this comment.
The synthetic user name format is incorrect. According to the implementation in internal/operator-controller/authentication/synthetic.go, the user name should be olm:clusterextension:<clusterExtensionName> (singular) not olm:clusterextensions:<clusterExtensionName> (plural). The group name correctly uses the plural form olm:clusterextensions.
Suggested change
| - User: "olm:clusterextensions:<clusterExtensionName>" | |
| - User: "olm:clusterextension:<clusterExtensionName>" |
| | `namespace` _string_ | namespace is a reference to a Kubernetes namespace.<br />This is the namespace in which the provided ServiceAccount must exist.<br />It also designates the default namespace where namespace-scoped resources<br />for the extension are applied to the cluster.<br />Some extensions may contain namespace-scoped resources to be applied in other namespaces.<br />This namespace must exist.<br />namespace is required, immutable, and follows the DNS label standard<br />as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-),<br />start and end with an alphanumeric character, and be no longer than 63 characters<br />[RFC 1123]: https://tools.ietf.org/html/rfc1123 | | MaxLength: 63 <br />Required: \{\} <br /> | | ||
| | `serviceAccount` _[ServiceAccountReference](#serviceaccountreference)_ | serviceAccount is a reference to a ServiceAccount used to perform all interactions<br />with the cluster that are required to manage the extension.<br />The ServiceAccount must be configured with the necessary permissions to perform these interactions.<br />The ServiceAccount must exist in the namespace referenced in the spec.<br />serviceAccount is required. | | Required: \{\} <br /> | | ||
| | `namespace` _string_ | namespace is a reference to a Kubernetes namespace.<br />This designates the default namespace where namespace-scoped resources<br />for the extension are applied to the cluster.<br />Some extensions may contain namespace-scoped resources to be applied in other namespaces.<br />This namespace must exist.<br /><opcon:standard:description><br />This is also the namespace of the referenced service account.<br /></opcon:standard:description><br /><opcon:experimental:description><br />This is also the namespace of the referenced service account, if specified.<br /></opcon:experimental:description><br />namespace is required, immutable, and follows the DNS label standard<br />as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-),<br />start and end with an alphanumeric character, and be no longer than 63 characters<br />[RFC 1123]: https://tools.ietf.org/html/rfc1123 | | MaxLength: 63 <br />Required: \{\} <br /> | | ||
| | `serviceAccount` _[ServiceAccountReference](#serviceaccountreference)_ | serviceAccount is a reference to a ServiceAccount used to perform all interactions<br />with the cluster that are required to manage the extension.<br /><opcon:standard:description><br />The ServiceAccount must be configured with the necessary permissions to perform these interactions.<br />serviceAccount is required.<br /></opcon:standard:description><br /><opcon:experimental:description><br />If serviceAccount is specified, OLM will authenticate as that service account.<br />Otherwise, operator-controller will authenticate as:<br /> - User: "olm:clusterextensions:<clusterExtensionName>"<br /> - Group: "olm:clusterextensions"<br />The authenticated user must be configured with the necessary permissions to perform these interactions.<br /></opcon:experimental:description><br /><opcon:standard:validation:Required><br /><opcon:experimental:validation:Optional> | | | |
There was a problem hiding this comment.
The synthetic user name format in the documentation is incorrect. The User should be olm:clusterextension:<clusterExtensionName> (singular) not olm:clusterextensions:<clusterExtensionName> (plural), according to the implementation in internal/operator-controller/authentication/synthetic.go. The group name correctly uses the plural form.
Suggested change
| | `serviceAccount` _[ServiceAccountReference](#serviceaccountreference)_ | serviceAccount is a reference to a ServiceAccount used to perform all interactions<br />with the cluster that are required to manage the extension.<br /><opcon:standard:description><br />The ServiceAccount must be configured with the necessary permissions to perform these interactions.<br />serviceAccount is required.<br /></opcon:standard:description><br /><opcon:experimental:description><br />If serviceAccount is specified, OLM will authenticate as that service account.<br />Otherwise, operator-controller will authenticate as:<br /> - User: "olm:clusterextensions:<clusterExtensionName>"<br /> - Group: "olm:clusterextensions"<br />The authenticated user must be configured with the necessary permissions to perform these interactions.<br /></opcon:experimental:description><br /><opcon:standard:validation:Required><br /><opcon:experimental:validation:Optional> | | | | |
| | `serviceAccount` _[ServiceAccountReference](#serviceaccountreference)_ | serviceAccount is a reference to a ServiceAccount used to perform all interactions<br />with the cluster that are required to manage the extension.<br /><opcon:standard:description><br />The ServiceAccount must be configured with the necessary permissions to perform these interactions.<br />serviceAccount is required.<br /></opcon:standard:description><br /><opcon:experimental:description><br />If serviceAccount is specified, OLM will authenticate as that service account.<br />Otherwise, operator-controller will authenticate as:<br /> - User: "olm:clusterextension:<clusterExtensionName>"<br /> - Group: "olm:clusterextensions"<br />The authenticated user must be configured with the necessary permissions to perform these interactions.<br /></opcon:experimental:description><br /><opcon:standard:validation:Required><br /><opcon:experimental:validation:Optional> | | | |
This commit updates the ClusterExtension API to make the serviceAccount
field optional in the experimental channel while keeping it required in
the standard channel.
Changes to ClusterExtension API:
- Added <opcon:standard:validation:Required> and
<opcon:experimental:validation:Optional> tags to ServiceAccount field
- Added channel-specific descriptions for serviceAccount behavior:
- Standard: Service account is required
- Experimental: If omitted, authenticates as synthetic user
"olmv1:clusterextensions:<clusterExtensionName>" with group
"olmv1:clusterextensions"
- Added channel-specific descriptions for namespace field explaining
the relationship with serviceAccount
- Updated ServiceAccountReference documentation to remove namespace
requirement text
- Changed JSON tags to use omitzero for optional behavior
Generated artifacts updated:
- CRDs (standard and experimental channels)
- API reference documentation
- All manifest files
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
This commit enables the SyntheticPermissions feature to allow ClusterExtensions to operate without specifying a ServiceAccount by authenticating as synthetic users. Feature gate changes: - Enabled SyntheticPermissions in experimental channel (helm) - Enabled SyntheticPermissions in tilt configuration RBAC changes: - Added conditional RBAC rules for impersonating users and the "olm:clusterextensions" group when SyntheticPermissions is enabled - Kept existing serviceaccounts/token permissions for token-based auth Logic changes: - Updated SyntheticUserRestConfigMapper to check for empty SA name instead of synthetic-user sentinel value - Modified NewRBACPreAuthorizer to accept a userInfoMapper function - Implemented userInfoMapper in main.go to return synthetic user info when SA name is empty and feature gate is enabled, otherwise returns standard service account user info - Updated authorization tests to work with new userInfoMapper pattern With these changes: - When ServiceAccount.Name is empty and SyntheticPermissions is enabled: authenticates as "olm:clusterextension:<name>" with group "olm:clusterextensions" using impersonation - When ServiceAccount.Name is specified: uses existing token-based authentication (unchanged) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
This commit replaces token-based authentication with ServiceAccount impersonation for better security and simpler token management. Changes: Authentication layer: - Added ServiceAccountImpersonationConfig() function that returns an ImpersonationConfig for a ServiceAccount user - Updated ServiceAccountRestConfigMapper() to use impersonation via NewImpersonatingRoundTripper instead of TokenInjectingRoundTripper - Removed TokenGetter parameter from ServiceAccountRestConfigMapper - Deleted tokengetter.go, tokengetter_test.go, and tripper.go as they are no longer needed RBAC: - Changed from serviceaccounts/token create to serviceaccounts impersonate permission Controller: - Removed ServiceAccountNotFoundError handling since impersonation doesn't require the ServiceAccount to exist beforehand - Removed authentication package import from controller Main setup: - Removed TokenGetter initialization - Updated userInfoMapper to use ServiceAccountImpersonationConfig Tests: - Updated tests to verify impersonation headers instead of token injection - Added tests for ServiceAccountImpersonationConfig - Updated controller tests that referenced TokenGetter Benefits of impersonation over tokens: - No need to manage token lifecycle or expiration - No need to check if ServiceAccount exists before use - Simpler code with fewer moving parts - More secure as no tokens are created or cached - More secure as it is no longer required to create highly privileged ServiceAccounts that could be used by workloads in the install namespace. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
joelanford
force-pushed
the
sa-impersonation
branch
from
4cc2d52 to
324a970
Compare
|
PR needs rebase. 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. |
openshift-merge-robot
added
the
needs-rebase
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces support for SyntheticPermissions (experimental feature) and migrates OLM v1 from token-based authentication to ServiceAccount impersonation for improved security and simplified authentication management.
What Changed
1. CRD Generator Enhancements (Commit c08e679)
Extended the CRD generator to support channel-specific field requirements and descriptions:
These enhancements enable the following API changes.
2. Optional ServiceAccount in Experimental Channel (Commit 1094bc9)
Made the
serviceAccountfield optional in the experimental channel while keeping it required in the standard channel:ClusterExtension API changes:
serviceAccountfield uses new validation tags to be optional (experimental) vs required (standard)Generated artifacts updated:
3. SyntheticPermissions Feature Gate (Commit 189a05a)
Enabled the SyntheticPermissions feature gate and implemented the logic for synthetic user authentication. When
serviceAccount.Nameis empty in experimental:olm:clusterextension:<clusterExtensionName>olm:clusterextensionsFeature gate:
helm/experimental.yaml)helm/tilt.yaml)RBAC permissions:
olm:clusterextension:<clusterExtensionName>)olm:clusterextensionsgroupImplementation:
SyntheticUserRestConfigMapperto check for empty SA name (instead of sentinel value)NewRBACPreAuthorizerto accept auserInfoMapperfunctionuserInfoMapperinmain.gothat:Authentication behavior:
ServiceAccount.Nameis empty + SyntheticPermissions enabled: synthetic user impersonationServiceAccount.Nameis specified: service account impersonation4. ServiceAccount Impersonation (Commit 324a970)
Replaced token-based authentication with ServiceAccount impersonation for all authentication in the standard and experimental channels:
Authentication layer changes:
ServiceAccountImpersonationConfig()function returning anImpersonationConfigServiceAccountRestConfigMapper()to useNewImpersonatingRoundTrippertokengetter.go- Token lifecycle managementtokengetter_test.go- Token getter teststripper.go- Token injection round tripperRBAC changes:
serviceaccounts/token createpermissionserviceaccounts impersonatepermissionController simplifications:
ServiceAccountNotFoundErrorhandling (impersonation doesn't require SA to exist beforehand)Tests updated:
ServiceAccountImpersonationConfigBenefits:
Testing
Reviewer Checklist