From 29746f500fb69c502bab92bd64ef02e92cbddab7 Mon Sep 17 00:00:00 2001 From: Lauren Leach Date: Fri, 21 Aug 2026 10:46:52 -0700 Subject: [PATCH 1/3] add shared identity source (baton-id) support --- README.md | 32 +++ config_schema.json | 19 +- docs/connector.mdx | 26 +++ pkg/config/conf.gen.go | 2 + pkg/config/config.go | 45 ++++ pkg/config/config_test.go | 2 + pkg/connector/clusterrole.go | 19 +- pkg/connector/config_wiring_test.go | 48 +++- pkg/connector/connector.go | 38 ++- pkg/connector/external_match.go | 208 +++++++++++++++++ pkg/connector/external_match_test.go | 324 ++++++++++++++++++++++++++ pkg/connector/helper.go | 66 ++++-- pkg/connector/resource_types.go | 12 +- pkg/connector/role.go | 10 +- pkg/connector/role_assignment.go | 15 +- pkg/connector/role_assignment_test.go | 10 +- pkg/connector/role_test.go | 12 +- 17 files changed, 835 insertions(+), 53 deletions(-) create mode 100644 pkg/connector/external_match.go create mode 100644 pkg/connector/external_match_test.go diff --git a/README.md b/README.md index fdaa90bb..2a882113 100644 --- a/README.md +++ b/README.md @@ -298,6 +298,36 @@ The following authentication methods are **not supported** for group membership RBAC bindings to groups are fully visible. If `ClusterRole:admin` is bound to group `developers`, that grant is synced. However, **the list of users in `developers` is only complete if those users authenticate via x509 client certificates stored as kubeconfig Secrets in the cluster**. Users authenticating via OIDC or webhook will appear as grant targets on Roles and ClusterRoles (if they have direct bindings) but not as members of their groups. +To resolve those memberships, attach an identity source instead — see below. + +## External Identity Matching + +Because a cluster authorizes identities it does not store, a `User` or `Group` subject in an RBAC binding is only a string some authenticator asserted: an OIDC claim, an x509 `CN=`/`O=` field, a Microsoft Entra object ID, an AWS IAM ARN. The directory that knows who that principal is belongs to a different ConductorOne app. + +Attaching that app as an **identity source** lets ConductorOne resolve the two. For every `User` and `Group` subject the connector emits an additional *carrier* grant annotated with what it knows about the subject, and the Baton SDK rewrites each carrier onto the matching principal from the identity source. A matched group additionally expands through the directory's own membership entitlement, so `Group developers → ClusterRole admin` becomes visible per person — the membership Kubernetes itself cannot supply. + +Two match strategies ride on every carrier and both are attempted, since which one fits is a property of the directory rather than of Kubernetes: + +- the external resource's **ID**, for directories whose IDs Kubernetes uses verbatim (Entra group object IDs, IAM role ARNs) +- a **profile field**, for the OIDC case where the subject is a human-readable name or address + +The defaults suit an OIDC-federated cluster and need no configuration. Override them when the cluster federates against something else: + +| Flag | Default | Set it to | +| --- | --- | --- | +| `--external-user-match-key` | `email` — also matches a user's email addresses | `userPrincipalName` for Microsoft Entra | +| `--external-group-match-key` | `display_name` — where Entra publishes a group's name | `sAMAccountName` to match Active Directory directly rather than through Entra | + +A matched group expands through the identity source's own membership entitlement. That is not configurable: it has to be an entitlement the source actually emitted, and the connector targets Entra's (`members`). Federating against a directory that names it `member` instead — Okta, Google Workspace, Active Directory — requires a code change, not a flag. + +Locally, point the connector at another connector's `.c1z` to do the same resolution offline: + +``` +baton-kubernetes --external-resource-c1z ./entra.c1z +``` + +**Group access stays reviewable with no identity source at all.** The carrier is emitted alongside the ordinary `kube_user` / `kube_group` grant, never instead of it — a carrier is consumed and discarded during matching, so cluster-level evidence would be lost if it were the only record. With no identity source configured, or when a group matches nothing in the directory, the group remains a first-class, attestable grantee exactly as before. + # Contributing, Support and Issues We started Baton because we were tired of taking screenshots and manually @@ -386,9 +416,11 @@ Flags: --cluster string The name of the kubeconfig cluster to use ($BATON_CLUSTER) --context string The name of the kubeconfig context to use ($BATON_CONTEXT) --disable-compression If true, opt-out of response compression for all requests to the server ($BATON_DISABLE_COMPRESSION) + --external-group-match-key string Profile field on the external identity source to match a Kubernetes Group subject against. Defaults to "display_name", which is where Microsoft Entra publishes a group's name. Group subjects are additionally always matched against the external group's ID, which is what AKS clusters use as the group name (an Entra object GUID). ($BATON_EXTERNAL_GROUP_MATCH_KEY) --external-resource-c1z string The path to the c1z file to sync external baton resources with ($BATON_EXTERNAL_RESOURCE_C1Z) --external-resource-entitlement-id-filter string The entitlement that external users, groups must have access to sync external baton resources ($BATON_EXTERNAL_RESOURCE_ENTITLEMENT_ID_FILTER) --external-resource-traits strings Resource type traits (e.g. "user", "group", "app") to sync and match from the external resource c1z. When unset the matcher falls back to user and group; passing this flag replaces the full set rather than adding to it. ($BATON_EXTERNAL_RESOURCE_TRAITS) + --external-user-match-key string Profile field on the external identity source to match a Kubernetes User subject against. Defaults to "email", which also matches a user's email addresses. Use "userPrincipalName" for clusters federated against Microsoft Entra. ($BATON_EXTERNAL_USER_MATCH_KEY) -f, --file string The path to the c1z file to sync with ($BATON_FILE) (default "sync.c1z") --health-check Enable the HTTP health check endpoint ($BATON_HEALTH_CHECK) --health-check-port int Port for the HTTP health check endpoint ($BATON_HEALTH_CHECK_PORT) (default 8081) diff --git a/config_schema.json b/config_schema.json index a74542e2..98a53040 100644 --- a/config_schema.json +++ b/config_schema.json @@ -210,6 +210,22 @@ "displayName": "Include control-plane permissions on objects", "description": "If true, also report permissions held by system: cluster roles on individual objects. These are the Kubernetes control plane's own controllers and they reach every object, so they are excluded by default. What they permit is reported on API resources regardless.", "boolField": {} + }, + { + "name": "external-user-match-key", + "displayName": "External user match key", + "description": "Profile field on the external identity source to match a Kubernetes User subject against. Defaults to \"email\", which also matches a user's email addresses. Use \"userPrincipalName\" for clusters federated against Microsoft Entra.", + "stringField": { + "rules": {} + } + }, + { + "name": "external-group-match-key", + "displayName": "External group match key", + "description": "Profile field on the external identity source to match a Kubernetes Group subject against. Defaults to \"display_name\", which is where Microsoft Entra publishes a group's name. Group subjects are additionally always matched against the external group's ID, which is what AKS clusters use as the group name (an Entra object GUID).", + "stringField": { + "rules": {} + } } ], "constraints": [ @@ -255,5 +271,6 @@ "client-key" ] } - ] + ], + "supportsExternalResources": true } \ No newline at end of file diff --git a/docs/connector.mdx b/docs/connector.mdx index b5c327e6..8735a7b1 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -191,6 +191,32 @@ The following authentication methods are **not supported** for group membership **What this means for access reviews:** RBAC bindings to groups are fully visible. If `ClusterRole:admin` is bound to group `developers`, that grant is synced. However, the list of users in `developers` is only complete if those users authenticate via x509 client certificates whose kubeconfigs are stored as Secrets in the cluster. Users authenticating via OIDC or webhook will appear as grant targets on Roles and ClusterRoles if they have direct bindings, but not as members of their groups. +To resolve those memberships from the directory that does hold them, attach an identity source. + +## Matching cluster identities to a directory + +A cluster authorizes identities it does not store, so a `User` or `Group` subject in an RBAC binding is only a string the authenticator asserted: an OIDC claim, an x509 `CN=`/`O=` field, a Microsoft Entra object ID, an AWS IAM ARN. The directory that knows who that principal is belongs to a different app in C1. + +Selecting that app as this connector's **identity source** lets C1 resolve the two. Each `User` and `Group` subject the connector reports is matched against the identity source's principals, and a matched group is expanded through the directory's own membership, so `Group developers → ClusterRole admin` becomes reviewable person by person — the membership Kubernetes itself cannot supply. + +Matching is attempted two ways at once, because which one fits depends on the directory rather than on Kubernetes: + +- against the external resource's **ID**, for directories whose identifiers the cluster uses verbatim, such as Microsoft Entra group object IDs or AWS IAM role ARNs +- against a **profile field**, for the OIDC case where the subject is a human-readable name or email address + +The defaults suit a cluster federated through an OIDC issuer and need no configuration. Change them when the cluster federates against something else: + +| Setting | Default | Change it to | +| :--- | :--- | :--- | +| External user match key | `email`, which also matches a user's email addresses | `userPrincipalName` for Microsoft Entra | +| External group match key | `display_name`, where Microsoft Entra publishes a group's name | `sAMAccountName` to match Active Directory directly rather than through Entra | + +Resolving a matched group to the accounts inside it needs no setting: the connector targets the membership entitlement Microsoft Entra publishes, which is the identity source it is built to federate against. + + +**Group access stays reviewable whether or not an identity source is attached.** Matching adds a resolved view of each group; it never replaces the group itself. A group that matches nothing in the directory — or a connector with no identity source selected at all — still reports the group as a first-class grantee you can attest in a campaign. + + ## Understanding how the connector selects a cluster The connector resolves its target cluster in this order: diff --git a/pkg/config/conf.gen.go b/pkg/config/conf.gen.go index 037136b5..1dd4efe8 100644 --- a/pkg/config/conf.gen.go +++ b/pkg/config/conf.gen.go @@ -22,6 +22,8 @@ type Kubernetes struct { DisableCompression bool `mapstructure:"disable-compression"` UseRoleAssignments bool `mapstructure:"use-role-assignments"` IncludeSystemObjectPermissions bool `mapstructure:"include-system-object-permissions"` + ExternalUserMatchKey string `mapstructure:"external-user-match-key"` + ExternalGroupMatchKey string `mapstructure:"external-group-match-key"` } func (c *Kubernetes) findFieldByTag(tagValue string) (any, bool) { diff --git a/pkg/config/config.go b/pkg/config/config.go index 4110f3cc..d45855c5 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -33,6 +33,12 @@ const ( // FlagIncludeSystemObjectPermissions is this connector's own flag, not one // of cli-runtime's. FlagIncludeSystemObjectPermissions = "include-system-object-permissions" + // External identity matching flags, also this connector's own. They name the + // directory-side fields a Kubernetes User or Group subject is matched on when + // an identity source is attached to the app. See + // pkg/connector/external_match.go. + FlagExternalUserMatchKey = "external-user-match-key" + FlagExternalGroupMatchKey = "external-group-match-key" ) var ( @@ -148,6 +154,32 @@ var ( " so they are excluded by default. What they permit is reported on API resources regardless."), field.WithDefaultValue(false), ) + // The external-match fields tune, rather than enable, identity matching: + // the connector always emits carrier grants, and they stay inert until an + // identity source is attached to the app in C1. What varies per deployment + // is which directory field the cluster's subject names correspond to, and + // only a deployment federated against something other than an OIDC issuer + // needs to say. Defaults live in pkg/connector, the single place that knows + // what a carrier claims; an empty value here means "use them". + externalUserMatchKeyField = field.StringField( + FlagExternalUserMatchKey, + field.WithDisplayName("External user match key"), + field.WithDescription( + "Profile field on the external identity source to match a Kubernetes User subject against."+ + " Defaults to \"email\", which also matches a user's email addresses."+ + " Use \"userPrincipalName\" for clusters federated against Microsoft Entra."), + field.WithRequired(false), + ) + externalGroupMatchKeyField = field.StringField( + FlagExternalGroupMatchKey, + field.WithDisplayName("External group match key"), + field.WithDescription( + "Profile field on the external identity source to match a Kubernetes Group subject against."+ + " Defaults to \"display_name\", which is where Microsoft Entra publishes a group's name."+ + " Group subjects are additionally always matched against the external group's ID,"+ + " which is what AKS clusters use as the group name (an Entra object GUID)."), + field.WithRequired(false), + ) ) // ConfigurationFields lists all connector-specific schema fields. @@ -170,6 +202,8 @@ var ConfigurationFields = []field.SchemaField{ disableCompressionField, useRoleAssignmentsField, includeSystemObjectPermissionsField, + externalUserMatchKeyField, + externalGroupMatchKeyField, } // ConfigRelations lists mutual-exclusivity and required-together constraints. @@ -194,7 +228,18 @@ var ConfigRelations = []field.SchemaFieldRelationship{ } // Configuration is the full connector schema passed to DefineConfiguration. +// +// SupportsExternalResources declares that this connector resolves grants against +// another app's synced principals, which is what makes C1 offer the +// identity-source picker for the app. Kubernetes needs it because it authorizes +// identities it does not store — see pkg/connector/external_match.go. +// +// It is not what enables the feature locally: --external-resource-c1z and its +// siblings are part of the SDK's default field set and registered for every +// connector regardless. Declaring it here is what carries that capability into +// the platform, where the identity source is actually attached. var Configuration = field.NewConfiguration( ConfigurationFields, field.WithConstraints(ConfigRelations...), + field.WithSupportsExternalResources(true), ) diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 022e66f7..767a5702 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -27,6 +27,8 @@ func TestConfigurationFields(t *testing.T) { config.FlagDisableCompression, config.FlagUseRoleAssignments, config.FlagIncludeSystemObjectPermissions, + config.FlagExternalUserMatchKey, + config.FlagExternalGroupMatchKey, } got := make(map[string]bool) diff --git a/pkg/connector/clusterrole.go b/pkg/connector/clusterrole.go index 65a6ed17..2eb67113 100644 --- a/pkg/connector/clusterrole.go +++ b/pkg/connector/clusterrole.go @@ -28,6 +28,9 @@ type clusterRoleBuilder struct { // because the role_assignment type is expressing the same access. The two // models are mutually exclusive; emitting both would double-count it. useRoleAssignments bool + // matchCfg names the directory-side fields that external-match carrier + // grants claim to match on. See external_match.go. + matchCfg ExternalMatchConfig // Cached namespaces cachedNamespaces []string nsMutex sync.Mutex @@ -224,12 +227,12 @@ func (c *clusterRoleBuilder) Grants(ctx context.Context, resource *v2.Resource, for _, binding := range matchingClusterBindings { // Process each subject in the binding for _, subject := range binding.Subjects { - subjectGrant, err := GrantRoleToSubject(subject, resource, clusterScopedMember) + subjectGrants, err := GrantRoleToSubject(subject, resource, clusterScopedMember, c.matchCfg) if err != nil { l.Debug("subject type not supported", zap.String("subject kind", subject.Kind)) continue } - rv = append(rv, subjectGrant) + rv = append(rv, subjectGrants...) } } @@ -244,12 +247,12 @@ func (c *clusterRoleBuilder) Grants(ctx context.Context, resource *v2.Resource, subject.Namespace = binding.Namespace } entName := fmt.Sprintf("%s:%s", namespace, "member") - subjectGrant, err := GrantRoleToSubject(subject, resource, entName) + subjectGrants, err := GrantRoleToSubject(subject, resource, entName, c.matchCfg) if err != nil { l.Debug("subject kind not supported", zap.String("subject kind", subject.Kind)) continue } - rv = append(rv, subjectGrant) + rv = append(rv, subjectGrants...) } } @@ -293,10 +296,16 @@ func (c *clusterRoleBuilder) cacheNamespaces(ctx context.Context) error { } // newClusterRoleBuilder creates a new cluster role builder. -func newClusterRoleBuilder(client kubernetes.Interface, bindingProvider ClusterRoleBindingProvider, useRoleAssignments bool) *clusterRoleBuilder { +func newClusterRoleBuilder( + client kubernetes.Interface, + bindingProvider ClusterRoleBindingProvider, + useRoleAssignments bool, + matchCfg ExternalMatchConfig, +) *clusterRoleBuilder { return &clusterRoleBuilder{ client: client, bindingProvider: bindingProvider, useRoleAssignments: useRoleAssignments, + matchCfg: matchCfg, } } diff --git a/pkg/connector/config_wiring_test.go b/pkg/connector/config_wiring_test.go index 7d00c567..bdc4a4b0 100644 --- a/pkg/connector/config_wiring_test.go +++ b/pkg/connector/config_wiring_test.go @@ -108,7 +108,7 @@ func TestRoleAssignmentsEmitNothingWhenDisabled(t *testing.T) { crbFor("view-everywhere", "view", userSubject("alice")), ) - assignments, _, err := newRoleAssignmentBuilder(client, &Kubernetes{client: client}, false). + assignments, _, err := newRoleAssignmentBuilder(client, &Kubernetes{client: client}, false, ExternalMatchConfig{}). List(ctx, nil, rs.SyncOpAttrs{SyncID: "sync-1"}) require.NoError(t, err) assert.Empty(t, assignments, "role assignments must not be emitted alongside the flat model") @@ -199,3 +199,49 @@ func TestDefaultSyncFilterIsRegistered(t *testing.T) { assert.Less(t, len(defaults), len(AllResourceTypeIDs), "the default must still be narrower than everything, or it is not a default") } + +// TestExternalMatchConfigReachesBuilders closes the last hop in the +// external-matching chain that nothing else covers. +// +// The other tests take an ExternalMatchConfig and check the annotations it +// produces; this one starts where the operator does. A flag that decodes into +// the generated config struct but never reaches a builder leaves the connector +// silently matching on the defaults, which looks like a directory that simply +// does not match rather than like a wiring bug. +func TestExternalMatchConfigReachesBuilders(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("KUBECONFIG", "") + + builder, _, err := NewFromConfig(context.Background(), &pkgconfig.Kubernetes{ + Server: "https://127.0.0.1:65535", + Token: "fake-token", + InsecureSkipTlsVerify: true, + ExternalUserMatchKey: "userPrincipalName", + ExternalGroupMatchKey: "displayName", + }, nil) + require.NoError(t, err) + + want := ExternalMatchConfig{ + UserMatchKey: "userPrincipalName", + GroupMatchKey: "displayName", + } + + // Every builder that emits subject grants has to carry the config; checking + // only one would let a missed call site through, which is how the flat and + // sparse models could disagree about how a subject is matched. + var checked int + for _, s := range builder.ResourceSyncers(context.Background()) { + switch b := s.(type) { + case *roleBuilder: + assert.Equal(t, want, b.matchCfg, "role builder") + checked++ + case *clusterRoleBuilder: + assert.Equal(t, want, b.matchCfg, "cluster role builder") + checked++ + case *roleAssignmentBuilder: + assert.Equal(t, want, b.matchCfg, "role assignment builder") + checked++ + } + } + assert.Equal(t, 3, checked, "every subject-granting builder must receive the match config") +} diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 80c6c4e9..f9545f99 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -51,6 +51,11 @@ type ConnectorOpts struct { // ClusterName labels the cluster resource. Empty falls back to the API // server host. ClusterName string + // ExternalMatch tunes the external-match carrier grants that accompany every + // User and Group subject. Its zero value is usable and selects the defaults; + // carriers are always emitted, so nothing here turns the feature on or off. + // See external_match.go. + ExternalMatch ExternalMatchConfig } // ConnectorOption is a function that configures the connector options. @@ -124,6 +129,23 @@ func WithClusterName(name string) ConnectorOption { } } +// WithExternalMatch names the directory-side fields that external-match carrier +// grants claim to match on. +// +// Kubernetes never stores the identities it authorizes, so a User or Group +// subject is matched against a separate identity source in C1. Which field it +// matches on is a property of that directory: an OIDC-federated cluster's +// usernames are email addresses, an Entra-federated cluster's are UPNs. Unset +// fields fall back to the defaults in external_match.go, which suit the OIDC +// case; downstream connectors (baton-eks, baton-aks, baton-gke) that know their +// provider's directory should pass its keys. +func WithExternalMatch(cfg ExternalMatchConfig) ConnectorOption { + return func(opts *ConnectorOpts) error { + opts.ExternalMatch = cfg + return nil + } +} + // Kubernetes connector struct. type Kubernetes struct { client kubernetes.Interface @@ -293,6 +315,10 @@ func NewFromConfig( WithRoleAssignments(cfg.UseRoleAssignments), WithSystemObjectPermissions(cfg.IncludeSystemObjectPermissions), WithClusterName(clusterNameFromConfig(opt, cfg)), + WithExternalMatch(ExternalMatchConfig{ + UserMatchKey: cfg.ExternalUserMatchKey, + GroupMatchKey: cfg.ExternalGroupMatchKey, + }), ) if err != nil { return nil, nil, err @@ -414,16 +440,16 @@ func (k *Kubernetes) ResourceSyncers(ctx context.Context) []connectorbuilder.Res return newServiceAccountBuilder(k.client, k.permissions()) }, ResourceTypeRole.Id: func(i *kubernetes.Interface, k *Kubernetes) connectorbuilder.ResourceSyncerV2 { - return newRoleBuilder(k.client, k) + return newRoleBuilder(k.client, k, k.opts.ExternalMatch) }, ResourceTypeClusterRole.Id: func(i *kubernetes.Interface, k *Kubernetes) connectorbuilder.ResourceSyncerV2 { - return newClusterRoleBuilder(k.client, k, k.opts.UseRoleAssignments) + return newClusterRoleBuilder(k.client, k, k.opts.UseRoleAssignments, k.opts.ExternalMatch) }, ResourceTypeCluster.Id: func(i *kubernetes.Interface, k *Kubernetes) connectorbuilder.ResourceSyncerV2 { return newClusterBuilder(k.opts.ClusterName, k.config.Host) }, ResourceTypeRoleAssignment.Id: func(i *kubernetes.Interface, k *Kubernetes) connectorbuilder.ResourceSyncerV2 { - return newRoleAssignmentBuilder(k.client, k, k.opts.UseRoleAssignments) + return newRoleAssignmentBuilder(k.client, k, k.opts.UseRoleAssignments, k.opts.ExternalMatch) }, ResourceTypeAPIResource.Id: func(i *kubernetes.Interface, k *Kubernetes) connectorbuilder.ResourceSyncerV2 { return newAPIResourceBuilder(k) @@ -560,10 +586,10 @@ func (d *defaultCapabilitiesBuilder) ResourceSyncers(_ context.Context) []connec return []connectorbuilder.ResourceSyncerV2{ newNamespaceBuilder(nil, nil), newServiceAccountBuilder(nil, nil), - newRoleBuilder(nil, nil), - newClusterRoleBuilder(nil, nil, false), + newRoleBuilder(nil, nil, ExternalMatchConfig{}), + newClusterRoleBuilder(nil, nil, false, ExternalMatchConfig{}), newClusterBuilder("", ""), - newRoleAssignmentBuilder(nil, nil, true), + newRoleAssignmentBuilder(nil, nil, true, ExternalMatchConfig{}), newAPIResourceBuilder(nil), newKubeUserBuilder(nil), newKubeGroupBuilder(nil), diff --git a/pkg/connector/external_match.go b/pkg/connector/external_match.go new file mode 100644 index 00000000..a18d43c5 --- /dev/null +++ b/pkg/connector/external_match.go @@ -0,0 +1,208 @@ +package connector + +import ( + "fmt" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/bid" + "github.com/conductorone/baton-sdk/pkg/types/entitlement" + "github.com/conductorone/baton-sdk/pkg/types/grant" +) + +// External identity matching ("baton-id"). +// +// A Kubernetes cluster authorizes principals it does not store. A User or Group +// subject in an RBAC binding is only a string some authenticator asserted — an +// OIDC claim, an x509 CN or O= field, an Entra object ID, an IAM ARN — and the +// directory that knows who that principal actually is belongs to a different C1 +// app. That is why this connector cannot expand a group into people on its own, +// and why review evidence has always needed a second, federated source. +// +// The SDK bridges the two with match annotations. A grant carrying one is a +// *carrier*: during SyncExternalResourcesOp the syncer rewrites it onto every +// matching principal from the configured identity source and then deletes the +// original — see baton-sdk pkg/sync/syncer.go processGrantsWithExternalPrincipals. +// Deletion is unconditional, matched or not, so a carrier is not a safe place to +// keep cluster-level evidence: a group that resolves to nothing would vanish +// from the review entirely. This connector therefore emits a carrier *alongside* +// the durable kube_user / kube_group grant rather than instead of it. The durable +// grant carries no annotation, so it always survives and stays attestable at the +// group level; the carrier is what reaches the directory. +// +// Carriers ride on the placeholder user and group resource types, which no +// syncer ever lists. That is deliberate and the SDK sanctions it: a grant whose +// principal type was never synced is normally dropped at ingest, and the +// exemption is precisely a match annotation ("External match annotations own +// placeholder principals" — pkg/sync/ingest_filter.go). Keeping carriers off +// kube_user / kube_group also keeps the two grants distinct, since a grant's +// identity is (principal, entitlement) and reusing the principal would collapse +// them into one. +// +// Every carrier declares both available strategies, because which one fits is a +// property of the directory rather than of Kubernetes, and one connector build +// serves clusters federated against different ones: +// +// - ExternalResourceMatchID matches the external resource's own ID, for +// directories whose IDs Kubernetes uses verbatim (Entra group object IDs, +// IAM role ARNs). +// - ExternalResourceMatch matches a profile key, for the OIDC case where the +// subject is a human-readable name or address. +// +// The SDK attempts both: its ID and key/value branches are sequential, not +// exclusive, so an unmatched strategy costs nothing and the pair of them covers +// both federation styles without per-cluster configuration. Only one annotation +// of each type is honored, since the SDK reads them with annotations.Pick, which +// returns the first of a given type — hence one configurable key per subject +// kind rather than a list. The group membership entitlement a match expands +// through is not configurable either, for a different reason: it is a property +// of the identity source, and this connector federates against one. +// +// ServiceAccounts are never carriers. A ServiceAccount is a real object in the +// cluster, synced as its own resource, and has no directory counterpart to +// match against. + +// Default profile keys for the key/value match strategy. +const ( + // DefaultExternalUserMatchKey is "email" because Kubernetes usernames from + // an OIDC issuer are conventionally email addresses, and because the SDK + // special-cases this key: it matches a user's trait email addresses as well + // as a profile field of that name, so it resolves against directories that + // expose the address either way. Entra-federated clusters, whose usernames + // are UPNs, should set "userPrincipalName" instead. + DefaultExternalUserMatchKey = "email" + + // DefaultExternalGroupMatchKey is "display_name": the profile key Microsoft + // Entra publishes a group's human-readable name under, and Entra is the + // identity source this connector is federated against in practice. + // + // A Kubernetes group subject is a name on every platform except AKS, where + // it is an Entra object GUID — and the GUID is handled by the ID strategy + // instead, so this key only ever has to serve the name case. That includes + // on-premises clusters whose subjects are AD group names: those groups reach + // C1 through Entra as synced groups, under the same display_name, so the + // directory's own connector never has to be the match target. + // + // Set this to whatever key a different identity source uses. Active + // Directory, if matched directly rather than through Entra, carries the name + // in "sAMAccountName" and has no display_name at all. + DefaultExternalGroupMatchKey = "display_name" +) + +// externalGroupMemberEntitlement is the entitlement a matched external group's +// membership expands through. +// +// It has to be the last segment of an entitlement ID the identity source +// actually emitted: the SDK re-mints NewEntitlementID(matchedPrincipal, slug) +// and looks that exact string up in the store, where the external app's +// entitlements were copied verbatim, then drops the expansion on NotFound. +// +// "members" is Microsoft Entra's, which is the identity source this connector +// federates against — the same assumption DefaultExternalGroupMatchKey rests on. +// Note that Entra is also the one connector where this disagrees with its own +// Slug field: it builds the ID by hand as "group::members" while declaring +// Slug "member", and the ID is what the lookup uses. Reading the Slug is how you +// get this wrong. +// +// Directories that construct the entitlement the ordinary way — Okta, Google +// Workspace, Active Directory, JumpCloud, via NewAssignmentEntitlement(r, +// "member") — need "member" instead. Naming both here would cover them, at the +// price of an SDK error log per carrier for whichever one misses; that trade is +// only worth making if this connector stops being Entra-federated. +const externalGroupMemberEntitlement = "members" + +// ExternalMatchConfig names the directory-side fields a Kubernetes subject is +// matched on. Its zero value is usable and means "the defaults above": the +// connector always emits carriers, so no field here switches the feature on or +// off, they only tune what the carriers claim to match. +type ExternalMatchConfig struct { + // UserMatchKey is the profile key an external user is matched on. + UserMatchKey string + // GroupMatchKey is the profile key an external group is matched on. + GroupMatchKey string +} + +// withDefaults fills unset fields, so callers that construct the struct +// partially — or not at all — still produce usable carriers. +func (c ExternalMatchConfig) withDefaults() ExternalMatchConfig { + if c.UserMatchKey == "" { + c.UserMatchKey = DefaultExternalUserMatchKey + } + if c.GroupMatchKey == "" { + c.GroupMatchKey = DefaultExternalGroupMatchKey + } + return c +} + +// userCarrierGrant returns the carrier grant for a User subject, or nil when the +// subject name is empty and there is nothing to match on. +func (c ExternalMatchConfig) userCarrierGrant(resource *v2.Resource, entName string, subjectName string) *v2.Grant { + if subjectName == "" { + return nil + } + cfg := c.withDefaults() + carrier := GenerateResourceForGrant(subjectName, ResourceTypeUser.Id) + return grant.NewGrant( + resource, + entName, + carrier, + grant.WithAnnotation( + &v2.ExternalResourceMatchID{Id: subjectName}, + &v2.ExternalResourceMatch{ + Key: cfg.UserMatchKey, + Value: subjectName, + ResourceType: v2.ResourceType_TRAIT_USER, + }, + ), + ) +} + +// groupCarrierGrant returns the carrier grant for a Group subject, or nil when +// the subject name is empty. +// +// The GrantExpandable annotation is what turns a matched directory group into +// its individual members. Its entitlement ID must name the *carrier* resource: +// the syncer looks the expansion up by the grant principal's bid and then +// re-mints the same slug against whichever external principal matched, so +// pointing it at the carrier is how the remap finds it at all. +func (c ExternalMatchConfig) groupCarrierGrant(resource *v2.Resource, entName string, subjectName string) (*v2.Grant, error) { + if subjectName == "" { + return nil, nil + } + cfg := c.withDefaults() + carrier := GenerateResourceForGrant(subjectName, ResourceTypeGroup.Id) + + memberBID, err := bid.MakeBid(entitlement.NewAssignmentEntitlement(carrier, externalGroupMemberEntitlement)) + if err != nil { + return nil, fmt.Errorf("baton-kubernetes: failed to build %q entitlement bid for group %q: %w", + externalGroupMemberEntitlement, subjectName, err) + } + + return grant.NewGrant( + resource, + entName, + carrier, + grant.WithAnnotation( + &v2.ExternalResourceMatchID{Id: subjectName}, + &v2.ExternalResourceMatch{ + Key: cfg.GroupMatchKey, + Value: subjectName, + ResourceType: v2.ResourceType_TRAIT_GROUP, + }, + // Shallow: the directory's own connector already syncs nested group + // membership, so expanding one level onto that group's member + // entitlement reaches every account without this connector + // re-walking a hierarchy it cannot see. + // + // ResourceTypeIds is deliberately unset, which means unfiltered. + // Narrowing it would require naming the identity source's own + // resource type IDs, and those are that connector's private + // vocabulary — this connector cannot know whether its accounts are + // called "user", "account", or something else, and guessing wrong + // filters the expansion down to nothing. + &v2.GrantExpandable{ + EntitlementIds: []string{memberBID}, + Shallow: true, + }, + ), + ), nil +} diff --git a/pkg/connector/external_match_test.go b/pkg/connector/external_match_test.go new file mode 100644 index 00000000..4909a189 --- /dev/null +++ b/pkg/connector/external_match_test.go @@ -0,0 +1,324 @@ +package connector + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + rbacv1 "k8s.io/api/rbac/v1" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/annotations" + "github.com/conductorone/baton-sdk/pkg/bid" +) + +// testRoleResource is the entitlement-side resource the grants under test point at. +var testRoleResource = &v2.Resource{ + Id: &v2.ResourceId{ResourceType: ResourceTypeRole.Id, Resource: "team-a/pod-reader"}, + DisplayName: "pod-reader", +} + +// isCarrier reports whether a grant is an external-match carrier. Carrying a +// match annotation is what makes it one — that is the property the SDK keys on +// too, so testing for it beats inferring from the principal's resource type. +func isCarrier(g *v2.Grant) bool { + annos := annotations.Annotations(g.GetAnnotations()) + return annos.ContainsAny( + &v2.ExternalResourceMatchAll{}, + &v2.ExternalResourceMatch{}, + &v2.ExternalResourceMatchID{}, + ) +} + +// durableGrants returns only the grants that stand on their own: the ones whose +// principal is a resource this connector actually syncs. Every User and Group +// subject also yields a carrier (see external_match.go), so a test about binding +// scope, subject dedup, or cache lifetime would otherwise be counting carriers +// along with the access it means to measure. +func durableGrants(grants []*v2.Grant) []*v2.Grant { + out := make([]*v2.Grant, 0, len(grants)) + for _, g := range grants { + if !isCarrier(g) { + out = append(out, g) + } + } + return out +} + +// carrierGrants is durableGrants' complement, for the tests that are about +// carriers. +func carrierGrants(grants []*v2.Grant) []*v2.Grant { + out := make([]*v2.Grant, 0, len(grants)) + for _, g := range grants { + if isCarrier(g) { + out = append(out, g) + } + } + return out +} + +// pickMatchID returns the ExternalResourceMatchID on a grant, failing the test +// if it carries none. +func pickMatchID(t *testing.T, g *v2.Grant) *v2.ExternalResourceMatchID { + t.Helper() + annos := annotations.Annotations(g.GetAnnotations()) + got := &v2.ExternalResourceMatchID{} + ok, err := annos.Pick(got) + require.NoError(t, err) + require.True(t, ok, "grant carries no ExternalResourceMatchID") + return got +} + +// pickMatch returns the key/value ExternalResourceMatch on a grant, failing the +// test if it carries none. +func pickMatch(t *testing.T, g *v2.Grant) *v2.ExternalResourceMatch { + t.Helper() + annos := annotations.Annotations(g.GetAnnotations()) + got := &v2.ExternalResourceMatch{} + ok, err := annos.Pick(got) + require.NoError(t, err) + require.True(t, ok, "grant carries no ExternalResourceMatch") + return got +} + +// pickExpandable returns the GrantExpandable on a grant, or nil. +func pickExpandable(t *testing.T, g *v2.Grant) *v2.GrantExpandable { + t.Helper() + annos := annotations.Annotations(g.GetAnnotations()) + got := &v2.GrantExpandable{} + ok, err := annos.Pick(got) + require.NoError(t, err) + if !ok { + return nil + } + return got +} + +// TestUserSubjectEmitsDurableAndCarrier verifies a User subject produces both +// grants: the durable one that survives with no identity source configured, and +// the carrier that reaches the directory. The durable grant must carry no match +// annotation, or the SDK would delete it along with the carrier and the user's +// access would disappear from the review entirely. +func TestUserSubjectEmitsDurableAndCarrier(t *testing.T) { + subject := rbacv1.Subject{Kind: SubjectKindUser, Name: "alice@example.com", APIGroup: RBACAPIGroup} + + grants, err := GrantRoleToSubject(subject, testRoleResource, "member", ExternalMatchConfig{}) + require.NoError(t, err) + require.Len(t, grants, 2) + + durable := durableGrants(grants) + require.Len(t, durable, 1) + assert.Equal(t, ResourceTypeKubeUser.Id, durable[0].GetPrincipal().GetId().GetResourceType()) + assert.Equal(t, "alice@example.com", durable[0].GetPrincipal().GetId().GetResource()) + assert.Empty(t, durable[0].GetAnnotations(), "the durable grant must not be deletable as a carrier") + + carriers := carrierGrants(grants) + require.Len(t, carriers, 1) + carrier := carriers[0] + assert.Equal(t, ResourceTypeUser.Id, carrier.GetPrincipal().GetId().GetResourceType()) + assert.Equal(t, "alice@example.com", carrier.GetPrincipal().GetId().GetResource()) + + assert.Equal(t, "alice@example.com", pickMatchID(t, carrier).GetId()) + + match := pickMatch(t, carrier) + assert.Equal(t, DefaultExternalUserMatchKey, match.GetKey()) + assert.Equal(t, "alice@example.com", match.GetValue()) + assert.Equal(t, v2.ResourceType_TRAIT_USER, match.GetResourceType()) + + assert.Nil(t, pickExpandable(t, carrier), + "a user resolves to one account; there is nothing to expand through") +} + +// TestGroupSubjectEmitsDurableAndCarrier verifies the same for a Group subject, +// plus the expansion annotation that turns a matched directory group into the +// accounts inside it. +func TestGroupSubjectEmitsDurableAndCarrier(t *testing.T) { + subject := rbacv1.Subject{Kind: SubjectKindGroup, Name: "SCRUM-HPC-ADMIN", APIGroup: RBACAPIGroup} + + grants, err := GrantRoleToSubject(subject, testRoleResource, "member", ExternalMatchConfig{}) + require.NoError(t, err) + require.Len(t, grants, 2) + + durable := durableGrants(grants) + require.Len(t, durable, 1) + assert.Equal(t, ResourceTypeKubeGroup.Id, durable[0].GetPrincipal().GetId().GetResourceType()) + assert.Equal(t, "SCRUM-HPC-ADMIN", durable[0].GetPrincipal().GetId().GetResource()) + assert.Empty(t, durable[0].GetAnnotations(), + "group access must stay attestable when the group matches nothing") + + carriers := carrierGrants(grants) + require.Len(t, carriers, 1) + carrier := carriers[0] + assert.Equal(t, ResourceTypeGroup.Id, carrier.GetPrincipal().GetId().GetResourceType()) + + assert.Equal(t, "SCRUM-HPC-ADMIN", pickMatchID(t, carrier).GetId()) + + match := pickMatch(t, carrier) + assert.Equal(t, DefaultExternalGroupMatchKey, match.GetKey()) + assert.Equal(t, "SCRUM-HPC-ADMIN", match.GetValue()) + assert.Equal(t, v2.ResourceType_TRAIT_GROUP, match.GetResourceType()) + + expandable := pickExpandable(t, carrier) + require.NotNil(t, expandable) + assert.True(t, expandable.GetShallow(), + "the directory's own connector resolves nesting; one level is enough") + require.NotEmpty(t, expandable.GetEntitlementIds()) +} + +// TestGroupCarrierExpandableTargetsItsOwnPrincipal pins the invariant the SDK's +// remap depends on: it looks the expansion up by the bid of the grant's +// principal, then re-mints the same slug against whichever external principal +// matched. An expandable entitlement naming any other resource is silently +// ignored, and the group would match but never expand to its members. +func TestGroupCarrierExpandableTargetsItsOwnPrincipal(t *testing.T) { + subject := rbacv1.Subject{Kind: SubjectKindGroup, Name: "prod-developer", APIGroup: RBACAPIGroup} + + grants, err := GrantRoleToSubject(subject, testRoleResource, "member", ExternalMatchConfig{}) + require.NoError(t, err) + carriers := carrierGrants(grants) + require.Len(t, carriers, 1) + carrier := carriers[0] + + expandable := pickExpandable(t, carrier) + require.NotNil(t, expandable) + require.NotEmpty(t, expandable.GetEntitlementIds()) + + wantBID, err := bid.MakeBid(carrier.GetPrincipal()) + require.NoError(t, err) + + for _, entID := range expandable.GetEntitlementIds() { + parsed, err := bid.ParseEntitlementBid(entID) + require.NoError(t, err) + gotBID, err := bid.MakeBid(parsed.GetResource()) + require.NoError(t, err) + assert.Equal(t, wantBID, gotBID, + "every expandable entitlement must name the carrier principal, or the SDK cannot remap it") + } +} + +// TestCarrierAndDurableGrantIDsDiffer verifies the two grants for one subject +// are distinct objects. A grant's identity is (principal, entitlement), so +// putting the carrier on the same principal as the durable grant would collapse +// them into one and the durable grant would be lost. +func TestCarrierAndDurableGrantIDsDiffer(t *testing.T) { + for _, subject := range []rbacv1.Subject{ + {Kind: SubjectKindUser, Name: "alice", APIGroup: RBACAPIGroup}, + {Kind: SubjectKindGroup, Name: "admins", APIGroup: RBACAPIGroup}, + } { + t.Run(subject.Kind, func(t *testing.T) { + grants, err := GrantRoleToSubject(subject, testRoleResource, "member", ExternalMatchConfig{}) + require.NoError(t, err) + require.Len(t, grants, 2) + assert.NotEqual(t, grants[0].GetId(), grants[1].GetId()) + }) + } +} + +// TestServiceAccountEmitsNoCarrier verifies a ServiceAccount stays a single +// grant. It is a real object in the cluster with no directory counterpart, so a +// carrier for it could only ever match the wrong thing. +func TestServiceAccountEmitsNoCarrier(t *testing.T) { + subject := rbacv1.Subject{Kind: SubjectKindServiceAccount, Name: "argo", Namespace: "argocd"} + + grants, err := GrantRoleToSubject(subject, testRoleResource, "member", ExternalMatchConfig{}) + require.NoError(t, err) + require.Len(t, grants, 1) + assert.Equal(t, ResourceTypeServiceAccount.Id, grants[0].GetPrincipal().GetId().GetResourceType()) + assert.Equal(t, "argocd/argo", grants[0].GetPrincipal().GetId().GetResource()) + assert.Empty(t, carrierGrants(grants)) +} + +// TestSystemSubjectsStillSkipped verifies adding carriers did not widen which +// subjects the connector emits at all. Kubernetes' built-in system: principals +// are cluster machinery, not identities any directory knows about. +func TestSystemSubjectsStillSkipped(t *testing.T) { + for _, subject := range []rbacv1.Subject{ + {Kind: SubjectKindGroup, Name: "system:masters", APIGroup: RBACAPIGroup}, + {Kind: SubjectKindUser, Name: "system:kube-controller-manager", APIGroup: RBACAPIGroup}, + } { + t.Run(subject.Name, func(t *testing.T) { + grants, err := GrantRoleToSubject(subject, testRoleResource, "member", ExternalMatchConfig{}) + require.Error(t, err) + assert.Empty(t, grants) + }) + } +} + +// TestNonRBACAPIGroupSubjectsSkipped verifies a User or Group subject from an +// unexpected apiGroup is still rejected rather than turned into a carrier that +// would claim a directory match on an identity Kubernetes did not authenticate +// through RBAC. +func TestNonRBACAPIGroupSubjectsSkipped(t *testing.T) { + subject := rbacv1.Subject{Kind: SubjectKindUser, Name: "alice", APIGroup: "example.com"} + + grants, err := GrantRoleToSubject(subject, testRoleResource, "member", ExternalMatchConfig{}) + require.Error(t, err) + assert.Empty(t, grants) +} + +// TestExternalMatchConfigOverrides verifies the configured keys reach the +// annotations, which is what lets one connector build serve clusters federated +// against different directories. +func TestExternalMatchConfigOverrides(t *testing.T) { + cfg := ExternalMatchConfig{ + UserMatchKey: "userPrincipalName", + GroupMatchKey: "displayName", + } + + userGrants, err := GrantRoleToSubject( + rbacv1.Subject{Kind: SubjectKindUser, Name: "alice@corp.example", APIGroup: RBACAPIGroup}, + testRoleResource, "member", cfg) + require.NoError(t, err) + userCarrier := carrierGrants(userGrants) + require.Len(t, userCarrier, 1) + assert.Equal(t, "userPrincipalName", pickMatch(t, userCarrier[0]).GetKey()) + + groupGrants, err := GrantRoleToSubject( + rbacv1.Subject{Kind: SubjectKindGroup, Name: "eng", APIGroup: RBACAPIGroup}, + testRoleResource, "member", cfg) + require.NoError(t, err) + groupCarrier := carrierGrants(groupGrants) + require.Len(t, groupCarrier, 1) + assert.Equal(t, "displayName", pickMatch(t, groupCarrier[0]).GetKey()) +} + +// TestGroupCarrierUsesEntrasMemberEntitlement pins the slug to the one Microsoft +// Entra actually emits. +// +// The SDK looks up NewEntitlementID(matchedPrincipal, slug) as an exact string +// against entitlements copied verbatim from the identity source, and Entra builds +// its group membership ID by hand as "group::members" while declaring Slug +// "member". Taking the Slug field at face value yields "member", which resolves +// to nothing and silently drops the expansion. +func TestGroupCarrierUsesEntrasMemberEntitlement(t *testing.T) { + grants, err := GrantRoleToSubject( + rbacv1.Subject{Kind: SubjectKindGroup, Name: "eng", APIGroup: RBACAPIGroup}, + testRoleResource, "member", ExternalMatchConfig{}) + require.NoError(t, err) + + carriers := carrierGrants(grants) + require.Len(t, carriers, 1) + expandable := pickExpandable(t, carriers[0]) + require.NotNil(t, expandable) + + var slugs []string + for _, entID := range expandable.GetEntitlementIds() { + parsed, err := bid.ParseEntitlementBid(entID) + require.NoError(t, err) + slugs = append(slugs, parsed.GetSlug()) + } + assert.Equal(t, []string{"members"}, slugs) +} + +// TestExternalMatchConfigDefaults verifies the zero value is usable, since the +// downstream connectors that build this connector as a library may not set it +// and a partial struct must not produce a carrier with an empty match key. +func TestExternalMatchConfigDefaults(t *testing.T) { + got := ExternalMatchConfig{}.withDefaults() + assert.Equal(t, DefaultExternalUserMatchKey, got.UserMatchKey) + assert.Equal(t, DefaultExternalGroupMatchKey, got.GroupMatchKey) + + partial := ExternalMatchConfig{GroupMatchKey: "displayName"}.withDefaults() + assert.Equal(t, DefaultExternalUserMatchKey, partial.UserMatchKey) + assert.Equal(t, "displayName", partial.GroupMatchKey) +} diff --git a/pkg/connector/helper.go b/pkg/connector/helper.go index 419a433a..7932b7ce 100644 --- a/pkg/connector/helper.go +++ b/pkg/connector/helper.go @@ -56,8 +56,24 @@ func GenerateResourceForGrant(rName string, rType string) *v2.Resource { } } -func GrantRoleToSubject(subject rbacv1.Subject, resource *v2.Resource, entName string) (*v2.Grant, error) { - var grantOpts []grant.GrantOption +// GrantRoleToSubject renders one RBAC binding subject as the grants that express +// its access to resource through the entName entitlement. +// +// A ServiceAccount yields exactly one grant: it is a cluster-local object this +// connector already syncs, so there is nothing to federate. A User or Group +// yields two — the durable grant against the synced kube_user / kube_group +// resource, plus an external-match carrier that reaches the identity source. +// See external_match.go for why both are needed and why the carrier cannot +// stand alone. +// +// It returns an error for a subject kind the connector does not model, which +// callers log and skip. +func GrantRoleToSubject( + subject rbacv1.Subject, + resource *v2.Resource, + entName string, + matchCfg ExternalMatchConfig, +) ([]*v2.Grant, error) { if subject.Kind == SubjectKindServiceAccount { saName := fmt.Sprintf("%s/%s", subject.Namespace, subject.Name) // SA are always namespaced, even if they can have cluster roles bind to cluster level. saResource := GenerateResourceForGrant(saName, ResourceTypeServiceAccount.Id) @@ -66,31 +82,39 @@ func GrantRoleToSubject(subject rbacv1.Subject, resource *v2.Resource, entName s entName, saResource, ) - return g, nil + return []*v2.Grant{g}, nil } else if (subject.APIGroup == RBACAPIGroup || subject.APIGroup == RBACAPIGroupV1) && !strings.Contains(subject.Name, "system:") { // Ignore System subjects if subject.Kind == SubjectKindGroup { - // Group grants intentionally carry no GrantExpandable annotation: vanilla - // Kubernetes has no membership source to expand through (membership lives - // in the authenticator — x509 O= fields, OIDC claims, cloud IAM mappers). - // Cloud connectors (EKS/AKS/GKE) add their own expansion annotations paired - // with ExternalResourceMatch in their custom builders. groupResource := GenerateResourceForGrant(subject.Name, ResourceTypeKubeGroup.Id) - g := grant.NewGrant( - resource, - entName, - groupResource, - ) - return g, nil + grants := []*v2.Grant{ + grant.NewGrant( + resource, + entName, + groupResource, + ), + } + carrier, err := matchCfg.groupCarrierGrant(resource, entName, subject.Name) + if err != nil { + return nil, err + } + if carrier != nil { + grants = append(grants, carrier) + } + return grants, nil } if subject.Kind == SubjectKindUser { - g := grant.NewGrant( - resource, - entName, - GenerateResourceForGrant(subject.Name, ResourceTypeKubeUser.Id), - grantOpts..., - ) - return g, nil + grants := []*v2.Grant{ + grant.NewGrant( + resource, + entName, + GenerateResourceForGrant(subject.Name, ResourceTypeKubeUser.Id), + ), + } + if carrier := matchCfg.userCarrierGrant(resource, entName, subject.Name); carrier != nil { + grants = append(grants, carrier) + } + return grants, nil } } return nil, fmt.Errorf("unsupported subject type") diff --git a/pkg/connector/resource_types.go b/pkg/connector/resource_types.go index e4030a91..4ec069d3 100644 --- a/pkg/connector/resource_types.go +++ b/pkg/connector/resource_types.go @@ -96,8 +96,16 @@ var ( Annotations: optInAnnotations(), } ResourceTypeBinding = &v2.ResourceType{Id: "binding", DisplayName: "Binding", Description: "Internal type for processing RBAC bindings"} - ResourceTypeUser = &v2.ResourceType{Id: "user", DisplayName: SubjectTypeUser, Traits: []v2.ResourceType_Trait{v2.ResourceType_TRAIT_USER}} - ResourceTypeGroup = &v2.ResourceType{Id: "group", DisplayName: SubjectTypeGroup, Traits: []v2.ResourceType_Trait{v2.ResourceType_TRAIT_GROUP}} + // ResourceTypeUser and ResourceTypeGroup are the placeholder principal types + // that external-match carrier grants point at. No syncer lists them and they + // are absent from DeclaredResourceTypeIDs on purpose: a carrier's principal + // is a claim about a resource in *another* app, resolved during the SDK's + // external-resource pass, and the carrier is deleted once that pass runs. + // Registering them would instead invite the platform to sync empty types and + // would let a carrier collide with the durable kube_user / kube_group grant + // it is meant to accompany. See external_match.go. + ResourceTypeUser = &v2.ResourceType{Id: "user", DisplayName: SubjectTypeUser, Traits: []v2.ResourceType_Trait{v2.ResourceType_TRAIT_USER}} + ResourceTypeGroup = &v2.ResourceType{Id: "group", DisplayName: SubjectTypeGroup, Traits: []v2.ResourceType_Trait{v2.ResourceType_TRAIT_GROUP}} ) // SparseResourceTypeIDs lists the types belonging to the sparse model, which diff --git a/pkg/connector/role.go b/pkg/connector/role.go index 15af91ff..7eabf3b8 100644 --- a/pkg/connector/role.go +++ b/pkg/connector/role.go @@ -20,6 +20,9 @@ import ( type roleBuilder struct { client kubernetes.Interface bindingProvider RoleBindingProvider + // matchCfg names the directory-side fields that external-match carrier + // grants claim to match on. See external_match.go. + matchCfg ExternalMatchConfig } // ResourceType returns the resource type for Role. @@ -190,12 +193,12 @@ func (r *roleBuilder) Grants(ctx context.Context, resource *v2.Resource, opts rs if subject.Kind == SubjectKindServiceAccount && subject.Namespace == "" { subject.Namespace = binding.Namespace } - subjectGrant, err := GrantRoleToSubject(subject, resource, "member") + subjectGrants, err := GrantRoleToSubject(subject, resource, "member", r.matchCfg) if err != nil { l.Debug("subject kind not supported", zap.String("subject kind", subject.Kind)) continue } - rv = append(rv, subjectGrant) + rv = append(rv, subjectGrants...) } } @@ -203,9 +206,10 @@ func (r *roleBuilder) Grants(ctx context.Context, resource *v2.Resource, opts rs } // newRoleBuilder creates a new role builder. -func newRoleBuilder(client kubernetes.Interface, bindingProvider RoleBindingProvider) *roleBuilder { +func newRoleBuilder(client kubernetes.Interface, bindingProvider RoleBindingProvider, matchCfg ExternalMatchConfig) *roleBuilder { return &roleBuilder{ client: client, bindingProvider: bindingProvider, + matchCfg: matchCfg, } } diff --git a/pkg/connector/role_assignment.go b/pkg/connector/role_assignment.go index 80befa26..76216a7f 100644 --- a/pkg/connector/role_assignment.go +++ b/pkg/connector/role_assignment.go @@ -86,6 +86,9 @@ type roleAssignmentBuilder struct { // entitlements and grants, so emitting assignments too would count the same // access twice. enabled bool + // matchCfg names the directory-side fields that external-match carrier + // grants claim to match on. See external_match.go. + matchCfg ExternalMatchConfig // clusterRoles caches the names of existing cluster roles for one sync, so // paging through assignments does not re-list them per page. @@ -387,12 +390,12 @@ func (b *roleAssignmentBuilder) Grants(ctx context.Context, resource *v2.Resourc } seen[subject] = true - subjectGrant, err := GrantRoleToSubject(subject, resource, assignedEntitlement) + subjectGrants, err := GrantRoleToSubject(subject, resource, assignedEntitlement, b.matchCfg) if err != nil { l.Debug("subject kind not supported", zap.String("subject kind", subject.Kind)) continue } - rv = append(rv, subjectGrant) + rv = append(rv, subjectGrants...) } return rv, nil, nil @@ -472,11 +475,17 @@ func pageLimit(size int) int { return size } -func newRoleAssignmentBuilder(client kubernetes.Interface, k8s *Kubernetes, enabled bool) *roleAssignmentBuilder { +func newRoleAssignmentBuilder( + client kubernetes.Interface, + k8s *Kubernetes, + enabled bool, + matchCfg ExternalMatchConfig, +) *roleAssignmentBuilder { return &roleAssignmentBuilder{ client: client, bindings: k8s, bindingProvider: k8s, enabled: enabled, + matchCfg: matchCfg, } } diff --git a/pkg/connector/role_assignment_test.go b/pkg/connector/role_assignment_test.go index a1dc910f..d4593662 100644 --- a/pkg/connector/role_assignment_test.go +++ b/pkg/connector/role_assignment_test.go @@ -45,7 +45,7 @@ func userSubject(name string) rbacv1.Subject { // Kubernetes connector so the binding cache and lookups behave as in production. func newRoleAssignmentFixture(objects ...runtime.Object) *roleAssignmentBuilder { client := fake.NewSimpleClientset(objects...) - return newRoleAssignmentBuilder(client, &Kubernetes{client: client}, true) + return newRoleAssignmentBuilder(client, &Kubernetes{client: client}, true, ExternalMatchConfig{}) } // listAssignmentIDs drains List and returns the object IDs it emitted. @@ -179,7 +179,7 @@ func TestRoleAssignmentGrantsDedupeSubjects(t *testing.T) { require.NoError(t, err) principals := []string{} - for _, g := range grants { + for _, g := range durableGrants(grants) { principals = append(principals, g.GetPrincipal().GetId().GetResource()) } assert.ElementsMatch(t, []string{"alice", "bob"}, principals, @@ -204,7 +204,7 @@ func TestRoleAssignmentGrantsScopedToNamespace(t *testing.T) { for _, r := range resources { grants, _, err := b.Grants(ctx, r, rs.SyncOpAttrs{SyncID: "sync-1"}) require.NoError(t, err) - for _, g := range grants { + for _, g := range durableGrants(grants) { got[r.GetId().GetResource()] = append(got[r.GetId().GetResource()], g.GetPrincipal().GetId().GetResource()) } } @@ -341,7 +341,7 @@ func TestClusterRoleSuppressedUnderRoleAssignments(t *testing.T) { DisplayName: "view", } - flat := newClusterRoleBuilder(client, k8s, false) + flat := newClusterRoleBuilder(client, k8s, false, ExternalMatchConfig{}) ents, _, err := flat.Entitlements(ctx, resource, rs.SyncOpAttrs{SyncID: "sync-1"}) require.NoError(t, err) assert.NotEmpty(t, ents, "the flat model must still declare cluster role entitlements") @@ -349,7 +349,7 @@ func TestClusterRoleSuppressedUnderRoleAssignments(t *testing.T) { require.NoError(t, err) assert.NotEmpty(t, grants) - sparse := newClusterRoleBuilder(client, k8s, true) + sparse := newClusterRoleBuilder(client, k8s, true, ExternalMatchConfig{}) ents, _, err = sparse.Entitlements(ctx, resource, rs.SyncOpAttrs{SyncID: "sync-2"}) require.NoError(t, err) assert.Empty(t, ents, "role_assignment expresses this access instead") diff --git a/pkg/connector/role_test.go b/pkg/connector/role_test.go index 54be5097..62e4bf8b 100644 --- a/pkg/connector/role_test.go +++ b/pkg/connector/role_test.go @@ -315,18 +315,18 @@ var podReaderResource = &v2.Resource{ // report grants from the first sync's bindings forever. func TestRoleBuilderGrantsAcrossSyncs(t *testing.T) { fakeClient, k8s := bindRoleFixture(t) - builder := newRoleBuilder(fakeClient, k8s) + builder := newRoleBuilder(fakeClient, k8s, ExternalMatchConfig{}) ctx := context.Background() grants, _, err := builder.Grants(ctx, podReaderResource, rs.SyncOpAttrs{SyncID: "sync-1"}) require.NoError(t, err) - require.Len(t, grants, 1) + require.Len(t, durableGrants(grants), 1) bindBob(t, ctx, fakeClient) grants, _, err = builder.Grants(ctx, podReaderResource, rs.SyncOpAttrs{SyncID: "sync-2"}) require.NoError(t, err) - assert.Len(t, grants, 2, "a later sync must reflect bindings added since the first sync") + assert.Len(t, durableGrants(grants), 2, "a later sync must reflect bindings added since the first sync") } // TestBindingCacheHeldWithinSync verifies the cache still does its job: repeated @@ -334,18 +334,18 @@ func TestRoleBuilderGrantsAcrossSyncs(t *testing.T) { // a full cluster-wide binding list. func TestBindingCacheHeldWithinSync(t *testing.T) { fakeClient, k8s := bindRoleFixture(t) - builder := newRoleBuilder(fakeClient, k8s) + builder := newRoleBuilder(fakeClient, k8s, ExternalMatchConfig{}) ctx := context.Background() grants, _, err := builder.Grants(ctx, podReaderResource, rs.SyncOpAttrs{SyncID: "sync-1"}) require.NoError(t, err) - require.Len(t, grants, 1) + require.Len(t, durableGrants(grants), 1) bindBob(t, ctx, fakeClient) grants, _, err = builder.Grants(ctx, podReaderResource, rs.SyncOpAttrs{SyncID: "sync-1"}) require.NoError(t, err) - assert.Len(t, grants, 1, "the same sync must serve its cached snapshot, not re-list") + assert.Len(t, durableGrants(grants), 1, "the same sync must serve its cached snapshot, not re-list") } // TestBindingCacheInvalidationIsIndependentOfList pins the reason the cache is From 264839b735e41b293e09914524fd0b3a3f0c7ebd Mon Sep 17 00:00:00 2001 From: Lauren Leach Date: Fri, 21 Aug 2026 10:52:36 -0700 Subject: [PATCH 2/3] trim external match comments and config field descriptions Co-Authored-By: Claude Opus 5 (1M context) --- config_schema.json | 4 +- pkg/config/config.go | 39 ++----- pkg/connector/clusterrole.go | 3 +- pkg/connector/config_wiring_test.go | 17 +-- pkg/connector/connector.go | 19 +--- pkg/connector/external_match.go | 157 +++++++-------------------- pkg/connector/external_match_test.go | 86 +++++---------- pkg/connector/helper.go | 14 +-- pkg/connector/resource_types.go | 12 +- pkg/connector/role.go | 3 +- pkg/connector/role_assignment.go | 3 +- 11 files changed, 105 insertions(+), 252 deletions(-) diff --git a/config_schema.json b/config_schema.json index 98a53040..27697d3a 100644 --- a/config_schema.json +++ b/config_schema.json @@ -214,7 +214,7 @@ { "name": "external-user-match-key", "displayName": "External user match key", - "description": "Profile field on the external identity source to match a Kubernetes User subject against. Defaults to \"email\", which also matches a user's email addresses. Use \"userPrincipalName\" for clusters federated against Microsoft Entra.", + "description": "Profile field on the identity source to match a Kubernetes User subject against. Defaults to \"email\".", "stringField": { "rules": {} } @@ -222,7 +222,7 @@ { "name": "external-group-match-key", "displayName": "External group match key", - "description": "Profile field on the external identity source to match a Kubernetes Group subject against. Defaults to \"display_name\", which is where Microsoft Entra publishes a group's name. Group subjects are additionally always matched against the external group's ID, which is what AKS clusters use as the group name (an Entra object GUID).", + "description": "Profile field on the identity source to match a Kubernetes Group subject against. Defaults to \"display_name\". Group subjects are also always matched against the identity source's resource ID.", "stringField": { "rules": {} } diff --git a/pkg/config/config.go b/pkg/config/config.go index d45855c5..c990d79a 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -33,10 +33,8 @@ const ( // FlagIncludeSystemObjectPermissions is this connector's own flag, not one // of cli-runtime's. FlagIncludeSystemObjectPermissions = "include-system-object-permissions" - // External identity matching flags, also this connector's own. They name the - // directory-side fields a Kubernetes User or Group subject is matched on when - // an identity source is attached to the app. See - // pkg/connector/external_match.go. + + // External identity matching flags, also this connector's own. FlagExternalUserMatchKey = "external-user-match-key" FlagExternalGroupMatchKey = "external-group-match-key" ) @@ -154,30 +152,23 @@ var ( " so they are excluded by default. What they permit is reported on API resources regardless."), field.WithDefaultValue(false), ) - // The external-match fields tune, rather than enable, identity matching: - // the connector always emits carrier grants, and they stay inert until an - // identity source is attached to the app in C1. What varies per deployment - // is which directory field the cluster's subject names correspond to, and - // only a deployment federated against something other than an OIDC issuer - // needs to say. Defaults live in pkg/connector, the single place that knows - // what a carrier claims; an empty value here means "use them". + // These tune identity matching rather than enable it; empty means use the + // default in pkg/connector/external_match.go. externalUserMatchKeyField = field.StringField( FlagExternalUserMatchKey, field.WithDisplayName("External user match key"), field.WithDescription( - "Profile field on the external identity source to match a Kubernetes User subject against."+ - " Defaults to \"email\", which also matches a user's email addresses."+ - " Use \"userPrincipalName\" for clusters federated against Microsoft Entra."), + "Profile field on the identity source to match a Kubernetes User subject against."+ + " Defaults to \"email\"."), field.WithRequired(false), ) externalGroupMatchKeyField = field.StringField( FlagExternalGroupMatchKey, field.WithDisplayName("External group match key"), field.WithDescription( - "Profile field on the external identity source to match a Kubernetes Group subject against."+ - " Defaults to \"display_name\", which is where Microsoft Entra publishes a group's name."+ - " Group subjects are additionally always matched against the external group's ID,"+ - " which is what AKS clusters use as the group name (an Entra object GUID)."), + "Profile field on the identity source to match a Kubernetes Group subject against."+ + " Defaults to \"display_name\"."+ + " Group subjects are also always matched against the identity source's resource ID."), field.WithRequired(false), ) ) @@ -229,15 +220,9 @@ var ConfigRelations = []field.SchemaFieldRelationship{ // Configuration is the full connector schema passed to DefineConfiguration. // -// SupportsExternalResources declares that this connector resolves grants against -// another app's synced principals, which is what makes C1 offer the -// identity-source picker for the app. Kubernetes needs it because it authorizes -// identities it does not store — see pkg/connector/external_match.go. -// -// It is not what enables the feature locally: --external-resource-c1z and its -// siblings are part of the SDK's default field set and registered for every -// connector regardless. Declaring it here is what carries that capability into -// the platform, where the identity source is actually attached. +// SupportsExternalResources is what makes C1 offer the identity-source picker +// for this app; the --external-resource-* flags are registered by the SDK +// regardless. See pkg/connector/external_match.go. var Configuration = field.NewConfiguration( ConfigurationFields, field.WithConstraints(ConfigRelations...), diff --git a/pkg/connector/clusterrole.go b/pkg/connector/clusterrole.go index 2eb67113..55faab98 100644 --- a/pkg/connector/clusterrole.go +++ b/pkg/connector/clusterrole.go @@ -28,8 +28,7 @@ type clusterRoleBuilder struct { // because the role_assignment type is expressing the same access. The two // models are mutually exclusive; emitting both would double-count it. useRoleAssignments bool - // matchCfg names the directory-side fields that external-match carrier - // grants claim to match on. See external_match.go. + // matchCfg tunes external-match carriers. See external_match.go. matchCfg ExternalMatchConfig // Cached namespaces cachedNamespaces []string diff --git a/pkg/connector/config_wiring_test.go b/pkg/connector/config_wiring_test.go index bdc4a4b0..db7b9cec 100644 --- a/pkg/connector/config_wiring_test.go +++ b/pkg/connector/config_wiring_test.go @@ -200,14 +200,10 @@ func TestDefaultSyncFilterIsRegistered(t *testing.T) { "the default must still be narrower than everything, or it is not a default") } -// TestExternalMatchConfigReachesBuilders closes the last hop in the -// external-matching chain that nothing else covers. -// -// The other tests take an ExternalMatchConfig and check the annotations it -// produces; this one starts where the operator does. A flag that decodes into -// the generated config struct but never reaches a builder leaves the connector -// silently matching on the defaults, which looks like a directory that simply -// does not match rather than like a wiring bug. +// TestExternalMatchConfigReachesBuilders covers the flag -> config -> builder +// hop. A key that decodes but never reaches a builder leaves the connector +// silently matching on defaults, which looks like a directory that just does not +// match rather than a wiring bug. func TestExternalMatchConfigReachesBuilders(t *testing.T) { t.Setenv("HOME", t.TempDir()) t.Setenv("KUBECONFIG", "") @@ -226,9 +222,8 @@ func TestExternalMatchConfigReachesBuilders(t *testing.T) { GroupMatchKey: "displayName", } - // Every builder that emits subject grants has to carry the config; checking - // only one would let a missed call site through, which is how the flat and - // sparse models could disagree about how a subject is matched. + // Check every builder that emits subject grants; one missed call site would + // let the flat and sparse models disagree. var checked int for _, s := range builder.ResourceSyncers(context.Background()) { switch b := s.(type) { diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index f9545f99..3b8d2cac 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -51,10 +51,8 @@ type ConnectorOpts struct { // ClusterName labels the cluster resource. Empty falls back to the API // server host. ClusterName string - // ExternalMatch tunes the external-match carrier grants that accompany every - // User and Group subject. Its zero value is usable and selects the defaults; - // carriers are always emitted, so nothing here turns the feature on or off. - // See external_match.go. + // ExternalMatch tunes the carrier grants emitted for User and Group subjects. + // Zero value is usable. See external_match.go. ExternalMatch ExternalMatchConfig } @@ -129,16 +127,9 @@ func WithClusterName(name string) ConnectorOption { } } -// WithExternalMatch names the directory-side fields that external-match carrier -// grants claim to match on. -// -// Kubernetes never stores the identities it authorizes, so a User or Group -// subject is matched against a separate identity source in C1. Which field it -// matches on is a property of that directory: an OIDC-federated cluster's -// usernames are email addresses, an Entra-federated cluster's are UPNs. Unset -// fields fall back to the defaults in external_match.go, which suit the OIDC -// case; downstream connectors (baton-eks, baton-aks, baton-gke) that know their -// provider's directory should pass its keys. +// WithExternalMatch sets the profile keys external-match carriers claim to match +// on. Unset fields take the defaults in external_match.go; downstream connectors +// that know their identity source should pass its keys. func WithExternalMatch(cfg ExternalMatchConfig) ConnectorOption { return func(opts *ConnectorOpts) error { opts.ExternalMatch = cfg diff --git a/pkg/connector/external_match.go b/pkg/connector/external_match.go index a18d43c5..351441ce 100644 --- a/pkg/connector/external_match.go +++ b/pkg/connector/external_match.go @@ -11,118 +11,51 @@ import ( // External identity matching ("baton-id"). // -// A Kubernetes cluster authorizes principals it does not store. A User or Group -// subject in an RBAC binding is only a string some authenticator asserted — an -// OIDC claim, an x509 CN or O= field, an Entra object ID, an IAM ARN — and the -// directory that knows who that principal actually is belongs to a different C1 -// app. That is why this connector cannot expand a group into people on its own, -// and why review evidence has always needed a second, federated source. +// Kubernetes authorizes identities it does not store: a User or Group subject is +// just a string an authenticator asserted, and the directory behind it is a +// different C1 app. A grant carrying a match annotation is a *carrier* — the SDK +// rewrites it onto the matching external principal, then deletes it. // -// The SDK bridges the two with match annotations. A grant carrying one is a -// *carrier*: during SyncExternalResourcesOp the syncer rewrites it onto every -// matching principal from the configured identity source and then deletes the -// original — see baton-sdk pkg/sync/syncer.go processGrantsWithExternalPrincipals. -// Deletion is unconditional, matched or not, so a carrier is not a safe place to -// keep cluster-level evidence: a group that resolves to nothing would vanish -// from the review entirely. This connector therefore emits a carrier *alongside* -// the durable kube_user / kube_group grant rather than instead of it. The durable -// grant carries no annotation, so it always survives and stays attestable at the -// group level; the carrier is what reaches the directory. +// Deletion is unconditional, matched or not, so carriers are emitted *alongside* +// the durable kube_user / kube_group grant, never instead of it. The durable +// grant is unannotated and always survives, which is what keeps group access +// attestable when a group matches nothing. Carriers ride the placeholder user and +// group resource types, which nothing syncs; the SDK exempts match-annotated +// grants from the usual unsynced-principal drop. // -// Carriers ride on the placeholder user and group resource types, which no -// syncer ever lists. That is deliberate and the SDK sanctions it: a grant whose -// principal type was never synced is normally dropped at ingest, and the -// exemption is precisely a match annotation ("External match annotations own -// placeholder principals" — pkg/sync/ingest_filter.go). Keeping carriers off -// kube_user / kube_group also keeps the two grants distinct, since a grant's -// identity is (principal, entitlement) and reusing the principal would collapse -// them into one. +// Each carrier declares both strategies, since which one fits depends on the +// directory: MatchID for subjects that are already the external resource's ID, +// and a profile-key match for name- or address-shaped subjects. The SDK tries +// both, and honors only the first annotation of each type. // -// Every carrier declares both available strategies, because which one fits is a -// property of the directory rather than of Kubernetes, and one connector build -// serves clusters federated against different ones: -// -// - ExternalResourceMatchID matches the external resource's own ID, for -// directories whose IDs Kubernetes uses verbatim (Entra group object IDs, -// IAM role ARNs). -// - ExternalResourceMatch matches a profile key, for the OIDC case where the -// subject is a human-readable name or address. -// -// The SDK attempts both: its ID and key/value branches are sequential, not -// exclusive, so an unmatched strategy costs nothing and the pair of them covers -// both federation styles without per-cluster configuration. Only one annotation -// of each type is honored, since the SDK reads them with annotations.Pick, which -// returns the first of a given type — hence one configurable key per subject -// kind rather than a list. The group membership entitlement a match expands -// through is not configurable either, for a different reason: it is a property -// of the identity source, and this connector federates against one. -// -// ServiceAccounts are never carriers. A ServiceAccount is a real object in the -// cluster, synced as its own resource, and has no directory counterpart to -// match against. +// ServiceAccounts are never carriers — they are real cluster objects with no +// directory counterpart. -// Default profile keys for the key/value match strategy. +// Default profile keys for the key/value match strategy. Both are overridable +// per deployment; the SDK also resolves "email" against a user's trait email +// addresses, not just a profile field of that name. const ( - // DefaultExternalUserMatchKey is "email" because Kubernetes usernames from - // an OIDC issuer are conventionally email addresses, and because the SDK - // special-cases this key: it matches a user's trait email addresses as well - // as a profile field of that name, so it resolves against directories that - // expose the address either way. Entra-federated clusters, whose usernames - // are UPNs, should set "userPrincipalName" instead. - DefaultExternalUserMatchKey = "email" - - // DefaultExternalGroupMatchKey is "display_name": the profile key Microsoft - // Entra publishes a group's human-readable name under, and Entra is the - // identity source this connector is federated against in practice. - // - // A Kubernetes group subject is a name on every platform except AKS, where - // it is an Entra object GUID — and the GUID is handled by the ID strategy - // instead, so this key only ever has to serve the name case. That includes - // on-premises clusters whose subjects are AD group names: those groups reach - // C1 through Entra as synced groups, under the same display_name, so the - // directory's own connector never has to be the match target. - // - // Set this to whatever key a different identity source uses. Active - // Directory, if matched directly rather than through Entra, carries the name - // in "sAMAccountName" and has no display_name at all. + DefaultExternalUserMatchKey = "email" DefaultExternalGroupMatchKey = "display_name" ) -// externalGroupMemberEntitlement is the entitlement a matched external group's -// membership expands through. -// -// It has to be the last segment of an entitlement ID the identity source -// actually emitted: the SDK re-mints NewEntitlementID(matchedPrincipal, slug) -// and looks that exact string up in the store, where the external app's -// entitlements were copied verbatim, then drops the expansion on NotFound. -// -// "members" is Microsoft Entra's, which is the identity source this connector -// federates against — the same assumption DefaultExternalGroupMatchKey rests on. -// Note that Entra is also the one connector where this disagrees with its own -// Slug field: it builds the ID by hand as "group::members" while declaring -// Slug "member", and the ID is what the lookup uses. Reading the Slug is how you -// get this wrong. -// -// Directories that construct the entitlement the ordinary way — Okta, Google -// Workspace, Active Directory, JumpCloud, via NewAssignmentEntitlement(r, -// "member") — need "member" instead. Naming both here would cover them, at the -// price of an SDK error log per carrier for whichever one misses; that trade is -// only worth making if this connector stops being Entra-federated. +// externalGroupMemberEntitlement is the entitlement a matched group expands +// through. It must be the last segment of an entitlement ID the identity source +// really emitted — the SDK looks up NewEntitlementID(matchedPrincipal, slug) as +// an exact string and drops the expansion on NotFound. Read the source's +// entitlement *ID*, not its Slug field; they can disagree. EntitlementIds is a +// list, so a second slug can be added if a source needs a different one. const externalGroupMemberEntitlement = "members" -// ExternalMatchConfig names the directory-side fields a Kubernetes subject is -// matched on. Its zero value is usable and means "the defaults above": the -// connector always emits carriers, so no field here switches the feature on or -// off, they only tune what the carriers claim to match. +// ExternalMatchConfig names the profile keys a Kubernetes subject is matched on. +// The zero value is usable and takes the defaults; nothing here turns matching +// on or off. type ExternalMatchConfig struct { - // UserMatchKey is the profile key an external user is matched on. - UserMatchKey string - // GroupMatchKey is the profile key an external group is matched on. + UserMatchKey string GroupMatchKey string } -// withDefaults fills unset fields, so callers that construct the struct -// partially — or not at all — still produce usable carriers. +// withDefaults fills unset fields so a partial or zero struct still works. func (c ExternalMatchConfig) withDefaults() ExternalMatchConfig { if c.UserMatchKey == "" { c.UserMatchKey = DefaultExternalUserMatchKey @@ -133,8 +66,7 @@ func (c ExternalMatchConfig) withDefaults() ExternalMatchConfig { return c } -// userCarrierGrant returns the carrier grant for a User subject, or nil when the -// subject name is empty and there is nothing to match on. +// userCarrierGrant returns the carrier for a User subject, or nil if unnamed. func (c ExternalMatchConfig) userCarrierGrant(resource *v2.Resource, entName string, subjectName string) *v2.Grant { if subjectName == "" { return nil @@ -156,14 +88,11 @@ func (c ExternalMatchConfig) userCarrierGrant(resource *v2.Resource, entName str ) } -// groupCarrierGrant returns the carrier grant for a Group subject, or nil when -// the subject name is empty. +// groupCarrierGrant returns the carrier for a Group subject, or nil if unnamed. // -// The GrantExpandable annotation is what turns a matched directory group into -// its individual members. Its entitlement ID must name the *carrier* resource: -// the syncer looks the expansion up by the grant principal's bid and then -// re-mints the same slug against whichever external principal matched, so -// pointing it at the carrier is how the remap finds it at all. +// GrantExpandable is what resolves a matched group to its members. Its +// entitlement must name the *carrier* resource: the SDK finds the expansion by +// the grant principal's bid, then re-mints the slug against whatever matched. func (c ExternalMatchConfig) groupCarrierGrant(resource *v2.Resource, entName string, subjectName string) (*v2.Grant, error) { if subjectName == "" { return nil, nil @@ -188,17 +117,9 @@ func (c ExternalMatchConfig) groupCarrierGrant(resource *v2.Resource, entName st Value: subjectName, ResourceType: v2.ResourceType_TRAIT_GROUP, }, - // Shallow: the directory's own connector already syncs nested group - // membership, so expanding one level onto that group's member - // entitlement reaches every account without this connector - // re-walking a hierarchy it cannot see. - // - // ResourceTypeIds is deliberately unset, which means unfiltered. - // Narrowing it would require naming the identity source's own - // resource type IDs, and those are that connector's private - // vocabulary — this connector cannot know whether its accounts are - // called "user", "account", or something else, and guessing wrong - // filters the expansion down to nothing. + // Shallow: the source's own connector already syncs nested + // membership. ResourceTypeIds is left unset (unfiltered) because + // the source's resource type IDs are not knowable from here. &v2.GrantExpandable{ EntitlementIds: []string{memberBID}, Shallow: true, diff --git a/pkg/connector/external_match_test.go b/pkg/connector/external_match_test.go index 4909a189..c636733a 100644 --- a/pkg/connector/external_match_test.go +++ b/pkg/connector/external_match_test.go @@ -18,9 +18,7 @@ var testRoleResource = &v2.Resource{ DisplayName: "pod-reader", } -// isCarrier reports whether a grant is an external-match carrier. Carrying a -// match annotation is what makes it one — that is the property the SDK keys on -// too, so testing for it beats inferring from the principal's resource type. +// isCarrier reports whether a grant is an external-match carrier. func isCarrier(g *v2.Grant) bool { annos := annotations.Annotations(g.GetAnnotations()) return annos.ContainsAny( @@ -30,11 +28,8 @@ func isCarrier(g *v2.Grant) bool { ) } -// durableGrants returns only the grants that stand on their own: the ones whose -// principal is a resource this connector actually syncs. Every User and Group -// subject also yields a carrier (see external_match.go), so a test about binding -// scope, subject dedup, or cache lifetime would otherwise be counting carriers -// along with the access it means to measure. +// durableGrants returns the non-carrier grants, so tests about binding scope, +// dedup or cache lifetime measure access rather than carriers. func durableGrants(grants []*v2.Grant) []*v2.Grant { out := make([]*v2.Grant, 0, len(grants)) for _, g := range grants { @@ -45,8 +40,7 @@ func durableGrants(grants []*v2.Grant) []*v2.Grant { return out } -// carrierGrants is durableGrants' complement, for the tests that are about -// carriers. +// carrierGrants is durableGrants' complement. func carrierGrants(grants []*v2.Grant) []*v2.Grant { out := make([]*v2.Grant, 0, len(grants)) for _, g := range grants { @@ -57,8 +51,7 @@ func carrierGrants(grants []*v2.Grant) []*v2.Grant { return out } -// pickMatchID returns the ExternalResourceMatchID on a grant, failing the test -// if it carries none. +// pickMatchID returns the grant's ExternalResourceMatchID, or fails. func pickMatchID(t *testing.T, g *v2.Grant) *v2.ExternalResourceMatchID { t.Helper() annos := annotations.Annotations(g.GetAnnotations()) @@ -69,8 +62,7 @@ func pickMatchID(t *testing.T, g *v2.Grant) *v2.ExternalResourceMatchID { return got } -// pickMatch returns the key/value ExternalResourceMatch on a grant, failing the -// test if it carries none. +// pickMatch returns the grant's key/value ExternalResourceMatch, or fails. func pickMatch(t *testing.T, g *v2.Grant) *v2.ExternalResourceMatch { t.Helper() annos := annotations.Annotations(g.GetAnnotations()) @@ -94,11 +86,9 @@ func pickExpandable(t *testing.T, g *v2.Grant) *v2.GrantExpandable { return got } -// TestUserSubjectEmitsDurableAndCarrier verifies a User subject produces both -// grants: the durable one that survives with no identity source configured, and -// the carrier that reaches the directory. The durable grant must carry no match -// annotation, or the SDK would delete it along with the carrier and the user's -// access would disappear from the review entirely. +// TestUserSubjectEmitsDurableAndCarrier verifies a User subject yields both +// grants, and that the durable one carries no annotation — otherwise the SDK +// would delete it along with the carrier. func TestUserSubjectEmitsDurableAndCarrier(t *testing.T) { subject := rbacv1.Subject{Kind: SubjectKindUser, Name: "alice@example.com", APIGroup: RBACAPIGroup} @@ -129,9 +119,8 @@ func TestUserSubjectEmitsDurableAndCarrier(t *testing.T) { "a user resolves to one account; there is nothing to expand through") } -// TestGroupSubjectEmitsDurableAndCarrier verifies the same for a Group subject, -// plus the expansion annotation that turns a matched directory group into the -// accounts inside it. +// TestGroupSubjectEmitsDurableAndCarrier verifies the same for a Group, plus the +// expansion annotation. func TestGroupSubjectEmitsDurableAndCarrier(t *testing.T) { subject := rbacv1.Subject{Kind: SubjectKindGroup, Name: "SCRUM-HPC-ADMIN", APIGroup: RBACAPIGroup} @@ -165,11 +154,9 @@ func TestGroupSubjectEmitsDurableAndCarrier(t *testing.T) { require.NotEmpty(t, expandable.GetEntitlementIds()) } -// TestGroupCarrierExpandableTargetsItsOwnPrincipal pins the invariant the SDK's -// remap depends on: it looks the expansion up by the bid of the grant's -// principal, then re-mints the same slug against whichever external principal -// matched. An expandable entitlement naming any other resource is silently -// ignored, and the group would match but never expand to its members. +// TestGroupCarrierExpandableTargetsItsOwnPrincipal pins what the SDK's remap +// needs: it finds the expansion by the grant principal's bid. An expandable +// naming any other resource is silently ignored and never expands. func TestGroupCarrierExpandableTargetsItsOwnPrincipal(t *testing.T) { subject := rbacv1.Subject{Kind: SubjectKindGroup, Name: "prod-developer", APIGroup: RBACAPIGroup} @@ -196,10 +183,8 @@ func TestGroupCarrierExpandableTargetsItsOwnPrincipal(t *testing.T) { } } -// TestCarrierAndDurableGrantIDsDiffer verifies the two grants for one subject -// are distinct objects. A grant's identity is (principal, entitlement), so -// putting the carrier on the same principal as the durable grant would collapse -// them into one and the durable grant would be lost. +// TestCarrierAndDurableGrantIDsDiffer: grant identity is (principal, +// entitlement), so sharing a principal would collapse the two into one. func TestCarrierAndDurableGrantIDsDiffer(t *testing.T) { for _, subject := range []rbacv1.Subject{ {Kind: SubjectKindUser, Name: "alice", APIGroup: RBACAPIGroup}, @@ -214,9 +199,8 @@ func TestCarrierAndDurableGrantIDsDiffer(t *testing.T) { } } -// TestServiceAccountEmitsNoCarrier verifies a ServiceAccount stays a single -// grant. It is a real object in the cluster with no directory counterpart, so a -// carrier for it could only ever match the wrong thing. +// TestServiceAccountEmitsNoCarrier: a ServiceAccount has no directory +// counterpart, so a carrier could only match the wrong thing. func TestServiceAccountEmitsNoCarrier(t *testing.T) { subject := rbacv1.Subject{Kind: SubjectKindServiceAccount, Name: "argo", Namespace: "argocd"} @@ -228,9 +212,8 @@ func TestServiceAccountEmitsNoCarrier(t *testing.T) { assert.Empty(t, carrierGrants(grants)) } -// TestSystemSubjectsStillSkipped verifies adding carriers did not widen which -// subjects the connector emits at all. Kubernetes' built-in system: principals -// are cluster machinery, not identities any directory knows about. +// TestSystemSubjectsStillSkipped verifies carriers did not widen which subjects +// the connector emits. func TestSystemSubjectsStillSkipped(t *testing.T) { for _, subject := range []rbacv1.Subject{ {Kind: SubjectKindGroup, Name: "system:masters", APIGroup: RBACAPIGroup}, @@ -244,10 +227,8 @@ func TestSystemSubjectsStillSkipped(t *testing.T) { } } -// TestNonRBACAPIGroupSubjectsSkipped verifies a User or Group subject from an -// unexpected apiGroup is still rejected rather than turned into a carrier that -// would claim a directory match on an identity Kubernetes did not authenticate -// through RBAC. +// TestNonRBACAPIGroupSubjectsSkipped: an unexpected apiGroup must be rejected, +// not turned into a carrier claiming a directory match. func TestNonRBACAPIGroupSubjectsSkipped(t *testing.T) { subject := rbacv1.Subject{Kind: SubjectKindUser, Name: "alice", APIGroup: "example.com"} @@ -256,9 +237,7 @@ func TestNonRBACAPIGroupSubjectsSkipped(t *testing.T) { assert.Empty(t, grants) } -// TestExternalMatchConfigOverrides verifies the configured keys reach the -// annotations, which is what lets one connector build serve clusters federated -// against different directories. +// TestExternalMatchConfigOverrides verifies configured keys reach the annotations. func TestExternalMatchConfigOverrides(t *testing.T) { cfg := ExternalMatchConfig{ UserMatchKey: "userPrincipalName", @@ -282,15 +261,11 @@ func TestExternalMatchConfigOverrides(t *testing.T) { assert.Equal(t, "displayName", pickMatch(t, groupCarrier[0]).GetKey()) } -// TestGroupCarrierUsesEntrasMemberEntitlement pins the slug to the one Microsoft -// Entra actually emits. -// -// The SDK looks up NewEntitlementID(matchedPrincipal, slug) as an exact string -// against entitlements copied verbatim from the identity source, and Entra builds -// its group membership ID by hand as "group::members" while declaring Slug -// "member". Taking the Slug field at face value yields "member", which resolves -// to nothing and silently drops the expansion. -func TestGroupCarrierUsesEntrasMemberEntitlement(t *testing.T) { +// TestGroupCarrierMemberEntitlementSlug pins the slug the identity source +// actually emits. The SDK looks up NewEntitlementID(matchedPrincipal, slug) as an +// exact string, and a source's entitlement ID and Slug field can disagree — +// taking the Slug at face value resolves to nothing and drops the expansion. +func TestGroupCarrierMemberEntitlementSlug(t *testing.T) { grants, err := GrantRoleToSubject( rbacv1.Subject{Kind: SubjectKindGroup, Name: "eng", APIGroup: RBACAPIGroup}, testRoleResource, "member", ExternalMatchConfig{}) @@ -310,9 +285,8 @@ func TestGroupCarrierUsesEntrasMemberEntitlement(t *testing.T) { assert.Equal(t, []string{"members"}, slugs) } -// TestExternalMatchConfigDefaults verifies the zero value is usable, since the -// downstream connectors that build this connector as a library may not set it -// and a partial struct must not produce a carrier with an empty match key. +// TestExternalMatchConfigDefaults verifies the zero value is usable, since +// library callers may not set it. func TestExternalMatchConfigDefaults(t *testing.T) { got := ExternalMatchConfig{}.withDefaults() assert.Equal(t, DefaultExternalUserMatchKey, got.UserMatchKey) diff --git a/pkg/connector/helper.go b/pkg/connector/helper.go index 7932b7ce..8ba647de 100644 --- a/pkg/connector/helper.go +++ b/pkg/connector/helper.go @@ -56,17 +56,11 @@ func GenerateResourceForGrant(rName string, rType string) *v2.Resource { } } -// GrantRoleToSubject renders one RBAC binding subject as the grants that express -// its access to resource through the entName entitlement. +// GrantRoleToSubject renders one RBAC binding subject as grants on entName. // -// A ServiceAccount yields exactly one grant: it is a cluster-local object this -// connector already syncs, so there is nothing to federate. A User or Group -// yields two — the durable grant against the synced kube_user / kube_group -// resource, plus an external-match carrier that reaches the identity source. -// See external_match.go for why both are needed and why the carrier cannot -// stand alone. -// -// It returns an error for a subject kind the connector does not model, which +// A ServiceAccount yields one grant. A User or Group yields two: the durable +// grant on kube_user / kube_group, plus an external-match carrier (see +// external_match.go). Returns an error for subject kinds we do not model, which // callers log and skip. func GrantRoleToSubject( subject rbacv1.Subject, diff --git a/pkg/connector/resource_types.go b/pkg/connector/resource_types.go index 4ec069d3..ad8d4c9e 100644 --- a/pkg/connector/resource_types.go +++ b/pkg/connector/resource_types.go @@ -96,14 +96,10 @@ var ( Annotations: optInAnnotations(), } ResourceTypeBinding = &v2.ResourceType{Id: "binding", DisplayName: "Binding", Description: "Internal type for processing RBAC bindings"} - // ResourceTypeUser and ResourceTypeGroup are the placeholder principal types - // that external-match carrier grants point at. No syncer lists them and they - // are absent from DeclaredResourceTypeIDs on purpose: a carrier's principal - // is a claim about a resource in *another* app, resolved during the SDK's - // external-resource pass, and the carrier is deleted once that pass runs. - // Registering them would instead invite the platform to sync empty types and - // would let a carrier collide with the durable kube_user / kube_group grant - // it is meant to accompany. See external_match.go. + // ResourceTypeUser and ResourceTypeGroup are the placeholder principals that + // external-match carriers point at. Deliberately unregistered and absent from + // DeclaredResourceTypeIDs: they stand for resources in another app, and the + // carrier is deleted once matching runs. See external_match.go. ResourceTypeUser = &v2.ResourceType{Id: "user", DisplayName: SubjectTypeUser, Traits: []v2.ResourceType_Trait{v2.ResourceType_TRAIT_USER}} ResourceTypeGroup = &v2.ResourceType{Id: "group", DisplayName: SubjectTypeGroup, Traits: []v2.ResourceType_Trait{v2.ResourceType_TRAIT_GROUP}} ) diff --git a/pkg/connector/role.go b/pkg/connector/role.go index 7eabf3b8..e0513923 100644 --- a/pkg/connector/role.go +++ b/pkg/connector/role.go @@ -20,8 +20,7 @@ import ( type roleBuilder struct { client kubernetes.Interface bindingProvider RoleBindingProvider - // matchCfg names the directory-side fields that external-match carrier - // grants claim to match on. See external_match.go. + // matchCfg tunes external-match carriers. See external_match.go. matchCfg ExternalMatchConfig } diff --git a/pkg/connector/role_assignment.go b/pkg/connector/role_assignment.go index 76216a7f..6fa0c26c 100644 --- a/pkg/connector/role_assignment.go +++ b/pkg/connector/role_assignment.go @@ -86,8 +86,7 @@ type roleAssignmentBuilder struct { // entitlements and grants, so emitting assignments too would count the same // access twice. enabled bool - // matchCfg names the directory-side fields that external-match carrier - // grants claim to match on. See external_match.go. + // matchCfg tunes external-match carriers. See external_match.go. matchCfg ExternalMatchConfig // clusterRoles caches the names of existing cluster roles for one sync, so From f0d78083442120eb373a50898ada811b967aa8a5 Mon Sep 17 00:00:00 2001 From: Lauren Leach Date: Fri, 28 Aug 2026 14:44:57 -0700 Subject: [PATCH 3/3] keep the durable group grant when a carrier fails to build GrantRoleToSubject returned nil plus the error when groupCarrierGrant failed, discarding the kube_group grant it had already built. Every caller reads an error as an unsupported subject kind, logs at Debug and skips the subject, so a carrier failure silently removed real access from the sync. Reserve the error for the one thing callers can act on. A carrier that will not build is now logged at Warn and dropped on its own; the durable grant is the cluster's record that the binding exists and survives. Threading ctx in is what lets the failure be logged where it happens. bid.MakeBid cannot fail for the inputs built here, so the branch is only reachable through the makeCarrierBID seam, which is what the new test uses to prove the durable grant comes back. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/connector/clusterrole.go | 4 +-- pkg/connector/external_match.go | 9 +++++- pkg/connector/external_match_test.go | 45 +++++++++++++++++++++++----- pkg/connector/helper.go | 19 +++++++++++- pkg/connector/role.go | 2 +- pkg/connector/role_assignment.go | 2 +- 6 files changed, 68 insertions(+), 13 deletions(-) diff --git a/pkg/connector/clusterrole.go b/pkg/connector/clusterrole.go index 55faab98..c5881230 100644 --- a/pkg/connector/clusterrole.go +++ b/pkg/connector/clusterrole.go @@ -226,7 +226,7 @@ func (c *clusterRoleBuilder) Grants(ctx context.Context, resource *v2.Resource, for _, binding := range matchingClusterBindings { // Process each subject in the binding for _, subject := range binding.Subjects { - subjectGrants, err := GrantRoleToSubject(subject, resource, clusterScopedMember, c.matchCfg) + subjectGrants, err := GrantRoleToSubject(ctx, subject, resource, clusterScopedMember, c.matchCfg) if err != nil { l.Debug("subject type not supported", zap.String("subject kind", subject.Kind)) continue @@ -246,7 +246,7 @@ func (c *clusterRoleBuilder) Grants(ctx context.Context, resource *v2.Resource, subject.Namespace = binding.Namespace } entName := fmt.Sprintf("%s:%s", namespace, "member") - subjectGrants, err := GrantRoleToSubject(subject, resource, entName, c.matchCfg) + subjectGrants, err := GrantRoleToSubject(ctx, subject, resource, entName, c.matchCfg) if err != nil { l.Debug("subject kind not supported", zap.String("subject kind", subject.Kind)) continue diff --git a/pkg/connector/external_match.go b/pkg/connector/external_match.go index 351441ce..defe83bc 100644 --- a/pkg/connector/external_match.go +++ b/pkg/connector/external_match.go @@ -47,6 +47,13 @@ const ( // list, so a second slug can be added if a source needs a different one. const externalGroupMemberEntitlement = "members" +// makeCarrierBID is a seam. bid.MakeBid cannot fail for the inputs built here — +// the carrier resource always has both a type and a non-empty id — so the +// error branch below is only reachable from tests, which is exactly why it +// needs one: the branch must keep the durable grant, and nothing else proves +// it does. +var makeCarrierBID = bid.MakeBid + // ExternalMatchConfig names the profile keys a Kubernetes subject is matched on. // The zero value is usable and takes the defaults; nothing here turns matching // on or off. @@ -100,7 +107,7 @@ func (c ExternalMatchConfig) groupCarrierGrant(resource *v2.Resource, entName st cfg := c.withDefaults() carrier := GenerateResourceForGrant(subjectName, ResourceTypeGroup.Id) - memberBID, err := bid.MakeBid(entitlement.NewAssignmentEntitlement(carrier, externalGroupMemberEntitlement)) + memberBID, err := makeCarrierBID(entitlement.NewAssignmentEntitlement(carrier, externalGroupMemberEntitlement)) if err != nil { return nil, fmt.Errorf("baton-kubernetes: failed to build %q entitlement bid for group %q: %w", externalGroupMemberEntitlement, subjectName, err) diff --git a/pkg/connector/external_match_test.go b/pkg/connector/external_match_test.go index c636733a..1ca9425a 100644 --- a/pkg/connector/external_match_test.go +++ b/pkg/connector/external_match_test.go @@ -1,6 +1,8 @@ package connector import ( + "context" + "errors" "testing" "github.com/stretchr/testify/assert" @@ -92,7 +94,7 @@ func pickExpandable(t *testing.T, g *v2.Grant) *v2.GrantExpandable { func TestUserSubjectEmitsDurableAndCarrier(t *testing.T) { subject := rbacv1.Subject{Kind: SubjectKindUser, Name: "alice@example.com", APIGroup: RBACAPIGroup} - grants, err := GrantRoleToSubject(subject, testRoleResource, "member", ExternalMatchConfig{}) + grants, err := GrantRoleToSubject(context.Background(), subject, testRoleResource, "member", ExternalMatchConfig{}) require.NoError(t, err) require.Len(t, grants, 2) @@ -124,7 +126,7 @@ func TestUserSubjectEmitsDurableAndCarrier(t *testing.T) { func TestGroupSubjectEmitsDurableAndCarrier(t *testing.T) { subject := rbacv1.Subject{Kind: SubjectKindGroup, Name: "SCRUM-HPC-ADMIN", APIGroup: RBACAPIGroup} - grants, err := GrantRoleToSubject(subject, testRoleResource, "member", ExternalMatchConfig{}) + grants, err := GrantRoleToSubject(context.Background(), subject, testRoleResource, "member", ExternalMatchConfig{}) require.NoError(t, err) require.Len(t, grants, 2) @@ -160,7 +162,7 @@ func TestGroupSubjectEmitsDurableAndCarrier(t *testing.T) { func TestGroupCarrierExpandableTargetsItsOwnPrincipal(t *testing.T) { subject := rbacv1.Subject{Kind: SubjectKindGroup, Name: "prod-developer", APIGroup: RBACAPIGroup} - grants, err := GrantRoleToSubject(subject, testRoleResource, "member", ExternalMatchConfig{}) + grants, err := GrantRoleToSubject(context.Background(), subject, testRoleResource, "member", ExternalMatchConfig{}) require.NoError(t, err) carriers := carrierGrants(grants) require.Len(t, carriers, 1) @@ -191,7 +193,7 @@ func TestCarrierAndDurableGrantIDsDiffer(t *testing.T) { {Kind: SubjectKindGroup, Name: "admins", APIGroup: RBACAPIGroup}, } { t.Run(subject.Kind, func(t *testing.T) { - grants, err := GrantRoleToSubject(subject, testRoleResource, "member", ExternalMatchConfig{}) + grants, err := GrantRoleToSubject(context.Background(), subject, testRoleResource, "member", ExternalMatchConfig{}) require.NoError(t, err) require.Len(t, grants, 2) assert.NotEqual(t, grants[0].GetId(), grants[1].GetId()) @@ -204,7 +206,7 @@ func TestCarrierAndDurableGrantIDsDiffer(t *testing.T) { func TestServiceAccountEmitsNoCarrier(t *testing.T) { subject := rbacv1.Subject{Kind: SubjectKindServiceAccount, Name: "argo", Namespace: "argocd"} - grants, err := GrantRoleToSubject(subject, testRoleResource, "member", ExternalMatchConfig{}) + grants, err := GrantRoleToSubject(context.Background(), subject, testRoleResource, "member", ExternalMatchConfig{}) require.NoError(t, err) require.Len(t, grants, 1) assert.Equal(t, ResourceTypeServiceAccount.Id, grants[0].GetPrincipal().GetId().GetResourceType()) @@ -220,7 +222,7 @@ func TestSystemSubjectsStillSkipped(t *testing.T) { {Kind: SubjectKindUser, Name: "system:kube-controller-manager", APIGroup: RBACAPIGroup}, } { t.Run(subject.Name, func(t *testing.T) { - grants, err := GrantRoleToSubject(subject, testRoleResource, "member", ExternalMatchConfig{}) + grants, err := GrantRoleToSubject(context.Background(), subject, testRoleResource, "member", ExternalMatchConfig{}) require.Error(t, err) assert.Empty(t, grants) }) @@ -232,7 +234,7 @@ func TestSystemSubjectsStillSkipped(t *testing.T) { func TestNonRBACAPIGroupSubjectsSkipped(t *testing.T) { subject := rbacv1.Subject{Kind: SubjectKindUser, Name: "alice", APIGroup: "example.com"} - grants, err := GrantRoleToSubject(subject, testRoleResource, "member", ExternalMatchConfig{}) + grants, err := GrantRoleToSubject(context.Background(), subject, testRoleResource, "member", ExternalMatchConfig{}) require.Error(t, err) assert.Empty(t, grants) } @@ -245,6 +247,7 @@ func TestExternalMatchConfigOverrides(t *testing.T) { } userGrants, err := GrantRoleToSubject( + context.Background(), rbacv1.Subject{Kind: SubjectKindUser, Name: "alice@corp.example", APIGroup: RBACAPIGroup}, testRoleResource, "member", cfg) require.NoError(t, err) @@ -253,6 +256,7 @@ func TestExternalMatchConfigOverrides(t *testing.T) { assert.Equal(t, "userPrincipalName", pickMatch(t, userCarrier[0]).GetKey()) groupGrants, err := GrantRoleToSubject( + context.Background(), rbacv1.Subject{Kind: SubjectKindGroup, Name: "eng", APIGroup: RBACAPIGroup}, testRoleResource, "member", cfg) require.NoError(t, err) @@ -267,6 +271,7 @@ func TestExternalMatchConfigOverrides(t *testing.T) { // taking the Slug at face value resolves to nothing and drops the expansion. func TestGroupCarrierMemberEntitlementSlug(t *testing.T) { grants, err := GrantRoleToSubject( + context.Background(), rbacv1.Subject{Kind: SubjectKindGroup, Name: "eng", APIGroup: RBACAPIGroup}, testRoleResource, "member", ExternalMatchConfig{}) require.NoError(t, err) @@ -296,3 +301,29 @@ func TestExternalMatchConfigDefaults(t *testing.T) { assert.Equal(t, DefaultExternalUserMatchKey, partial.UserMatchKey) assert.Equal(t, "displayName", partial.GroupMatchKey) } + +// TestGroupCarrierFailureKeepsDurableGrant guards the asymmetry between the two +// things GrantRoleToSubject can fail at. +// +// An unsupported subject kind is a real error and callers skip the subject. A +// carrier that will not build is not: the durable kube_group grant is the +// cluster's own record that this binding exists, and it has to survive. Callers +// read any error as "unsupported subject kind" and drop the subject entirely, so +// returning one here would silently delete access data over a failed +// optimization. +func TestGroupCarrierFailureKeepsDurableGrant(t *testing.T) { + orig := makeCarrierBID + t.Cleanup(func() { makeCarrierBID = orig }) + makeCarrierBID = func(bid.BID) (string, error) { + return "", errors.New("synthetic bid failure") + } + + subject := rbacv1.Subject{Kind: SubjectKindGroup, Name: "eng", APIGroup: RBACAPIGroup} + grants, err := GrantRoleToSubject(context.Background(), subject, testRoleResource, "member", ExternalMatchConfig{}) + + require.NoError(t, err, "a carrier failure must not surface as an error: callers read it as an unsupported subject kind and skip the subject") + require.Len(t, grants, 1, "the durable grant must survive on its own") + assert.Empty(t, carrierGrants(grants), "no carrier should be emitted when it cannot be built") + assert.Equal(t, ResourceTypeKubeGroup.Id, grants[0].GetPrincipal().GetId().GetResourceType()) + assert.Equal(t, "eng", grants[0].GetPrincipal().GetId().GetResource()) +} diff --git a/pkg/connector/helper.go b/pkg/connector/helper.go index 8ba647de..af2e5099 100644 --- a/pkg/connector/helper.go +++ b/pkg/connector/helper.go @@ -1,12 +1,15 @@ package connector import ( + "context" "encoding/json" "fmt" "strings" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/types/grant" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" + "go.uber.org/zap" rbacv1 "k8s.io/api/rbac/v1" ) @@ -62,7 +65,12 @@ func GenerateResourceForGrant(rName string, rType string) *v2.Resource { // grant on kube_user / kube_group, plus an external-match carrier (see // external_match.go). Returns an error for subject kinds we do not model, which // callers log and skip. +// +// The error is reserved for that one meaning. A carrier that cannot be built is +// logged and dropped on its own, because the durable grant is the cluster's +// record of the binding and a failed optimization must not erase it. func GrantRoleToSubject( + ctx context.Context, subject rbacv1.Subject, resource *v2.Resource, entName string, @@ -90,7 +98,16 @@ func GrantRoleToSubject( } carrier, err := matchCfg.groupCarrierGrant(resource, entName, subject.Name) if err != nil { - return nil, err + // Skip the carrier, keep the durable grant. Returning the error + // here would lose both: every caller reads an error as an + // unsupported subject kind and drops the subject entirely. + ctxzap.Extract(ctx).Warn( + "baton-kubernetes: failed to build external-match carrier, keeping durable group grant", + zap.String("subject_name", subject.Name), + zap.String("entitlement", entName), + zap.Error(err), + ) + return grants, nil } if carrier != nil { grants = append(grants, carrier) diff --git a/pkg/connector/role.go b/pkg/connector/role.go index e0513923..cf34026e 100644 --- a/pkg/connector/role.go +++ b/pkg/connector/role.go @@ -192,7 +192,7 @@ func (r *roleBuilder) Grants(ctx context.Context, resource *v2.Resource, opts rs if subject.Kind == SubjectKindServiceAccount && subject.Namespace == "" { subject.Namespace = binding.Namespace } - subjectGrants, err := GrantRoleToSubject(subject, resource, "member", r.matchCfg) + subjectGrants, err := GrantRoleToSubject(ctx, subject, resource, "member", r.matchCfg) if err != nil { l.Debug("subject kind not supported", zap.String("subject kind", subject.Kind)) continue diff --git a/pkg/connector/role_assignment.go b/pkg/connector/role_assignment.go index 6fa0c26c..f181279d 100644 --- a/pkg/connector/role_assignment.go +++ b/pkg/connector/role_assignment.go @@ -389,7 +389,7 @@ func (b *roleAssignmentBuilder) Grants(ctx context.Context, resource *v2.Resourc } seen[subject] = true - subjectGrants, err := GrantRoleToSubject(subject, resource, assignedEntitlement, b.matchCfg) + subjectGrants, err := GrantRoleToSubject(ctx, subject, resource, assignedEntitlement, b.matchCfg) if err != nil { l.Debug("subject kind not supported", zap.String("subject kind", subject.Kind)) continue