WIP: 📝 Add SubjectMapping enhancement proposal - #174
Conversation
This enhancement proposes a SubjectMapping API for transparent authentication of hub subjects accessing managed clusters. The design eliminates token storage on the hub, simplifies RBAC, and requires zero code changes to applications. Key features: - Transparent authentication via cluster-proxy - On-demand token generation on managed clusters - Support for ServiceAccount subjects (Alpha/Beta) - Future support for User subjects - TokenReview-based hub token validation - Conflict detection without validation webhooks Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zhujian7 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 |
- Add CEL validation rules to ensure correct subject fields are populated based on kind - Clarify PlacementRef.Namespace as required field for cluster-scoped SubjectMapping - Add validation to enforce placementRef usage based on subject type - Fix all instances of 'ServiceAccountMapping' to 'SubjectMapping' for consistency - Update metadata.yaml with reviewers and approvers Signed-off-by: zhujian <jiazhu@redhat.com>
Rename 233-serviceaccount-mapping to 233-subject-mapping to: - Match the actual API name (SubjectMapping, not ServiceAccountMapping) - Reflect broader scope supporting ServiceAccount and User subjects - Improve clarity and avoid confusion about feature capabilities Signed-off-by: zhujian <jiazhu@redhat.com>
This commit fixes the ManagedServiceAccount namespace model and makes
the design more concise:
- Fix ManagedServiceAccount creation to use cluster namespace instead of
subject namespace (lines 180-205)
- Add lifecycle management section explaining finalizers and labels
approach since owner references cannot cross namespace boundaries
- Clarify namespace model in examples and architecture diagram
- Make cache invalidation conditions explicit (which fields trigger
invalidation vs which don't)
- Condense design by ~100 lines:
* Reduce Subject Matching section from detailed Go code to brief
descriptions (~70 lines)
* Simplify Test Plan to key categories (~40 lines)
* Shorten Alternatives section to summary (~30 lines)
The proposal is now more concise (932 lines vs 1029) while retaining
all critical technical details.
Signed-off-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: zhujian <jiazhu@redhat.com>
Clarify what happens when clusters are added or removed from ManagedClusterSet or Placement: - Controller watches ManagedClusterSetBinding, ManagedCluster labels, and PlacementDecision resources - Cluster added: Creates ManagedServiceAccount in new cluster namespace - Cluster removed: Deletes corresponding ManagedServiceAccount - Orphaned resources cleaned up during reconciliation This addresses a critical gap in the lifecycle management section. Signed-off-by: zhujian <jiazhu@redhat.com>
Fix confusing explanation on line 638. The namespace field is required because multiple Placements with the same name can exist in different namespaces, not simply because SubjectMapping is cluster-scoped. The new explanation directly states the reason for requiring the namespace field. Signed-off-by: zhujian <jiazhu@redhat.com>
Restructure the document to improve readability and follow logical progression from high-level to detailed design: Changes: - Add 'Overview' section with high-level description and architecture flow - Move architecture flow diagram from buried section to Overview (first thing readers see) - Rename 'API Design' subsections for clarity: - 'Architecture Components' → 'Component Overview' - Group detailed flows under 'Implementation Details' - Add forward reference link to API Specification - Remove duplicate High-Level Architecture diagram - Promote 'Architecture and Components' to top-level subsection - Promote 'Implementation Details' to top-level subsection New flow: User Stories → Overview (what/why) → API Design (examples) → Architecture (components) → Implementation (how) → Security → Risks This matches the recommendation: show WHAT before HOW, with progressive detail from overview to implementation specifics. Signed-off-by: zhujian <jiazhu@redhat.com>
Improve the architecture flow diagram and YAML examples to avoid
confusion about namespaces and ServiceAccount names:
Changes:
1. Architecture diagram step 1: Use clearer format
- Before: 'namespace: argocd' (ambiguous - SubjectMapping or SA?)
- After: 'argocd/argocd-hub-sa → argocd-sa-for-hub' (shows mapping)
2. Update ServiceAccount naming convention:
- Before: hub-argocd-sa, hub-admin-user
- After: argocd-sa-for-hub, admin-sa-for-hub
- Rationale: '-for-hub' suffix is self-documenting and clearer
3. Show correct namespace where managed SA is created:
- ServiceAccount created in 'open-cluster-management-agent-addon'
namespace on managed clusters (where addon agent runs)
- Updated TokenRequest API call to show full path
4. Update step 6 to reference correct token name
5. Clarify ManagedServiceAccount creation bullets to show both
the MSA resource location (cluster namespace) and the actual
ServiceAccount location (addon namespace)
This makes the flow easier to understand and aligns with how
managed-serviceaccount addon actually works.
Signed-off-by: zhujian <jiazhu@redhat.com>
c371133 to
a88d116
Compare
…entation Eliminate duplication between "Architecture and Components" and "Implementation Details" sections by clearly separating concerns: - Architecture section: lightweight overview of WHAT components exist and WHERE they are located (controller, authentication module, repository structure) - Implementation Details: comprehensive HOW covering ManagedServiceAccount creation, lifecycle management, cluster discovery, binding/unbinding, authentication flow, and token caching Removed duplicate Cluster Discovery section that appeared in both sections. This improves readability and follows standard OCM enhancement proposal patterns. Signed-off-by: zhujian <jiazhu@redhat.com>
ee88a94 to
561799b
Compare
Replace placementRef with a unified clusters field supporting two types: - InferFromServiceAccount: Auto-discovers via ManagedClusterSetBindings in ServiceAccount's namespace (ServiceAccount subjects, default) - ClusterList: Explicit cluster names (User subjects, required) This simplification removes the Placement API dependency and provides a cleaner, more consistent design for Alpha. Additional cluster discovery types (ManagedClusterSet, ClusterSelector, Placement) can be added in future releases. Key changes: - Add clusters field with type discriminator to SubjectMappingSpec - Remove placementRef field and PlacementRef type - Update validation rules for subject-specific cluster requirements - Simplify API examples and cluster discovery documentation - Move advanced discovery types to Future Enhancements section Signed-off-by: Jia Zhu <jiazhu@redhat.com> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com>
561799b to
3b42414
Compare
Question: ServiceAccount-only vs. ServiceAccount + User subjects?Should we support User subjects in addition to ServiceAccount subjects, or keep it ServiceAccount-only? Option 1: ServiceAccount + User (current design)
Option 2: ServiceAccount-only (simplified)
Simplified API example: apiVersion: authentication.open-cluster-management.io/v1alpha1
kind: ServiceAccountMapping # Renamed from SubjectMapping
metadata:
name: argocd-hub-sa # Must match ServiceAccount name
namespace: argocd # Implicit: maps argocd/argocd-hub-sa
spec:
managedServiceAccount:
name: argocd-sa-for-hub
tokenExpirationSeconds: 3600
# No hubSubject - implicit from metadata
# No clusters - always uses ManagedClusterSetBindings in namespaceBenefits:
@qiujian16 @xuezhaojun Do you see compelling use cases for User subjects? Or should we consider ServiceAccount-only (potentially as ServiceAccountMapping API)? |
|
|
||
| 2. **RBAC Complexity**: Hub components need Secret read permissions across multiple cluster namespaces. For example, ArgoCD accessing 100 managed clusters requires Secret read permissions in 100 namespaces, creating operational overhead and potential security gaps. | ||
|
|
||
| These challenges become more pronounced as cluster count scales and security requirements tighten. |
There was a problem hiding this comment.
can you also add clusterprofile part, it mitigate rbac issue for controller but introduce scalabaility concerns.
| │ │ - Finds ManagedClusterSetBinding in argocd namespace │ │ | ||
| │ │ - Discovers bound clusters (cluster1...cluster100) │ │ | ||
| │ │ - Creates ManagedServiceAccount in each cluster NS: │ │ | ||
| │ │ * cluster1/argocd-sa-for-hub (MSA in cluster1 NS) │ │ |
There was a problem hiding this comment.
should note this is not the current managedserviceaccout, it does not need to create secret.
|
|
||
| ### API Design | ||
|
|
||
| A new cluster-scoped custom resource that specifies the mapping configuration: |
| apiVersion: authentication.open-cluster-management.io/v1alpha1 | ||
| kind: SubjectMapping | ||
| metadata: | ||
| name: argocd-cluster-access |
There was a problem hiding this comment.
I think it should be namespace scoped .
| hubSubject: | ||
| kind: ServiceAccount | ||
| serviceAccount: | ||
| namespace: argocd |
There was a problem hiding this comment.
if it is sa, namespace is not needed.
| tokenExpirationSeconds: 3600 | ||
|
|
||
| # User subjects must specify clusters explicitly | ||
| clusters: |
There was a problem hiding this comment.
should not need a cluster list, it could be clusterset that bound to this namespace or placement.
| ```go | ||
| const ( | ||
| // Annotation to control token sync behavior | ||
| AnnotationTokenProjection = "authentication.open-cluster-management.io/token-projection" |
There was a problem hiding this comment.
could we have a spec field telling the agent that you do not need to map token to secret, just create the sa on the spoke.
There was a problem hiding this comment.
Currently, we use an annotation, do you prefer a new spec field?
Major changes: - Change API scope from cluster-scoped to namespace-scoped - Remove User subject support, focus on ServiceAccount mapping only - Add CEL validation: metadata.name must match hubSubject.serviceAccount.name - Simplify API from ~150 lines to ~60 lines (removed conflict resolution) - Store hub ServiceAccount identity in ManagedServiceAccount annotations - proxy-agent uses Role (not ClusterRole) for minimal RBAC in cluster namespace - Add User Access Workaround section for users needing access - Update authentication flow to use annotation-based lookup Security improvements: - proxy-agent only needs access to ManagedServiceAccounts in its own cluster namespace - Reduced RBAC scope from cluster-wide to single namespace (Role vs ClusterRole) - Eliminates need for broad permissions across all hub namespaces Documentation improvements: - Fixed inconsistencies (cluster-scoped vs namespace-scoped references) - Updated architecture diagram to reflect annotation-based lookup - Removed duplicate content about duplicate prevention - Clarified that proxy-agent watches ManagedServiceAccount (not SubjectMapping) The namespace-scoped design with CEL validation ensures only one SubjectMapping per ServiceAccount, eliminating conflict resolution complexity. The annotation-based approach for storing hub identity on ManagedServiceAccount allows proxy-agent to discover mappings with minimal permissions. Signed-off-by: zhujian <jiazhu@redhat.com>
Changes: - Add ClusterProfile plugin limitations to Motivation section (addresses PR open-cluster-management-io#174 review comment) - Explains that ClusterProfile plugin exacerbates token storage risk by copying secrets - Notes applications still need Secret read permissions and exec plugin configuration - Provides context for why SubjectMapping is needed - Fix RBAC section terminology for clarity - Change "Hub proxy-agent" to "proxy-agent on hub" - Change "Managed cluster proxy-agent" to "proxy-agent on managed cluster" - Add comments explaining what each permission is for - Clarifies that proxy-agent needs hub permissions for TokenReview and ManagedServiceAccount lookup This addresses the final review comment from PR open-cluster-management-io#174 (line 23) about adding ClusterProfile context to explain the motivation behind SubjectMapping. Signed-off-by: zhujian <jiazhu@redhat.com>
|
|
||
| ## Alternatives | ||
|
|
||
| ### ManagedServiceAccountTokenRequest API |
There was a problem hiding this comment.
addin spiffe as another alternative
|
|
||
| 1. **Increased cluster-proxy Complexity**: Adds authentication logic to proxy-agent, increasing maintenance burden | ||
| 2. **Kubernetes Version Dependency**: Requires Kubernetes 1.20+ for stable TokenRequest API | ||
| 3. **cluster-proxy Dependency**: Only works with clusters using cluster-proxy for hub communication |
There was a problem hiding this comment.
this might not be a drawback considering security.
|
we should also add a section on how cluster profile plugin should be updated accordingly. |
Major changes: - Add ClusterProfile plugin limitations to Motivation section - Support multiple SubjectMappings referencing same managed SA name - Store hub identities in ManagedServiceAccount annotation as JSON array - Per-identity token expiration settings - Virtual subresource for access control (webhook implementation TBD) - Update lifecycle management for shared ManagedServiceAccounts - Add section on granting permissions to managed SAs - Remove per-cluster status (not needed with shared SA support) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com>
- Add ClusterProfile Integration section explaining how SubjectMapping eliminates the credentials plugin's token storage issues - Clarify that ClusterProfile discovery remains useful while credentials plugin becomes unnecessary - Remove specific "360 days" token lifetime reference as it is configurable - Describe transparent authentication workflow for ClusterProfile users Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com>
|
|
||
| ### Goals | ||
|
|
||
| - Eliminate token storage on the hub cluster by generating tokens on-demand |
There was a problem hiding this comment.
Instead of individual cluster tokens leaking there is only the hub-sa token that needs to be secured, which at least lowers the blast radius to just a single secret to secure.
| │ │ - Watches ManagedServiceAccount in cluster namespace │ │ | ||
| │ │ - Receives request via tunnel │ │ | ||
| │ │ - Detects caller: argocd-hub-sa (from request auth) │ │ | ||
| │ │ - Lists ManagedServiceAccounts with annotations: │ │ |
There was a problem hiding this comment.
Will we include a CEL to try to enforce who can apply what annotations? Or is it consider allowed, if you have access to create a managedService account, you can reference an managedServiceAccountMappings?
There was a problem hiding this comment.
Thanks @jnpacker very important catch!
Yes! We've implemented ValidatingAdmissionPolicy with CEL to prevent bypass attacks. Users cannot bypass SubjectMapping RBAC by directly creating/modifying ManagedServiceAccount resources with the hub-identities annotation.
How it works:
The policy validates that users have 'use' permission on the virtual subresource subjectmappings/managedserviceaccounts for:
- Resource name: The managed ServiceAccount name (being created)
- Namespace: The hub ServiceAccount's namespace (from the annotation)
Example: To create a ManagedServiceAccount mapping argocd:hub-sa → my-managed-sa, you need:
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: allow-my-managed-sa
namespace: argocd # Hub SA namespace
rules:
- apiGroups: ["authentication.open-cluster-management.io"]
resources: ["subjectmappings/managedserviceaccounts"]
verbs: ["use"]
resourceNames: ["my-managed-sa"] # Managed SA name
Implementation:
apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingAdmissionPolicy
metadata:
name: managedserviceaccount-hub-identities-protection
spec:
failurePolicy: Fail
matchConstraints:
resourceRules:
- apiGroups: ["authentication.open-cluster-management.io"]
apiVersions: ["v1beta1"]
operations: ["CREATE", "UPDATE"]
resources: ["managedserviceaccounts"]
validations:
- expression: |
(has(request.userInfo.username) &&
request.userInfo.username == 'system:serviceaccount:open-cluster-management:subjectmapping-controller') ||
!has(object.metadata.annotations) ||
!('authentication.open-cluster-management.io/hub-identities' in object.metadata.annotations) ||
object.metadata.annotations['authentication.open-cluster-management.io/hub-identities'] == '' ||
object.metadata.annotations['authentication.open-cluster-management.io/hub-identities']
.split('\n')
.all(line, line == '' || (
line.split(':').size() == 3 &&
authorizer.group('authentication.open-cluster-management.io')
.resource('subjectmappings/managedserviceaccounts')
.namespace(line.split(':')[0])
.name(object.metadata.name)
.check('use').allowed()
))| - Replace ManagedServiceAccount API (both will coexist) | ||
| - Support authentication methods other than service account tokens on managed clusters | ||
| - Support managed clusters not using cluster-proxy | ||
| - Support mapping to Users or Groups on managed clusters (only ServiceAccounts as targets) |
There was a problem hiding this comment.
Something interesting we should think about specifically because you used and ARGOCD example.
ArgoCD has the ability to allow for signed commits, you can post your public key into argocd to guarantee you are allowed to commit and push to the target server....
Maybe something that lends itself to this flow. If you send a signed commit to argocd on the hub and your public key is published to the argocd, we use that as the means to initiate a subject mapping.
In this way, there is actually NO credential at all stored on the hub or the spoke.
This is what I expect the user/group scenario will be more like down the road.
There was a problem hiding this comment.
Thanks @jnpacker for this interesting idea! This provides a very meaningful scenario for mapping User subjects.
Current scope: ServiceAccount-only on both hub (source) and managed cluster (target) sides.
Future direction: The API is extensible for User subjects on the hub side (HubSubject.kind field). I think your GPG signing idea fits perfectly - verified user identity from signature → SubjectMapping → on-demand token generation (zero credentials stored).
Managed cluster targets remain ServiceAccounts because Kubernetes TokenRequest API requires them for token generation.
…s protection Changed hub-identities annotation from JSON to newline-separated format (namespace:name:tokenExpiration) because ValidatingAdmissionPolicy's CEL environment does not support json.decode(). This enables proper RBAC validation to prevent users from bypassing SubjectMapping access control by directly creating/modifying ManagedServiceAccount resources. Key changes: - Add annotation format explanation and rationale - Implement parseHubIdentities() and formatHubIdentities() helpers - Update controller and proxy-agent code to use string parsing - Add ValidatingAdmissionPolicy that validates 'use' permission on virtual subresource subjectmappings/managedserviceaccounts - Update all examples to use newline-separated format - Add test plan for annotation validation Signed-off-by: zhujian <jiazhu@redhat.com>
|
I believe one ManagedServiceAccount is required per cluster, but regarding apiVersion: authentication.open-cluster-management.io/v1beta1
kind: ManagedServiceAccount
metadata:
name: my-sample
namespace: spoke-1
---
apiVersion: authentication.open-cluster-management.io/v1beta1
kind: ManagedServiceAccount
metadata:
name: my-sample
namespace: spoke-2 |
each cluster will have one |
Summary
This enhancement proposes a SubjectMapping API that enables transparent authentication for hub subjects accessing managed clusters through cluster-proxy.
Implementation Phases
Alpha/Beta: ServiceAccount subject mapping (primary implementation)
Future: User subject mapping (designed but deferred)
Related Issue(s)
Enhancement #233
Key Design Decisions