Gate the RBAC management UI on a per-cluster ConfigMap - #5114
Gate the RBAC management UI on a per-cluster ConfigMap#5114dimitri-nicolo wants to merge 8 commits into
Conversation
397e263 to
4bfcc19
Compare
dimitri-nicolo
added
release-note-not-required
and removed
release-note-required
labels
1ada653 to
b72d4e6
Compare
pasanw
left a comment
pasanw
left a comment
There was a problem hiding this comment.
The TL:DR of most of my comments: we've moved the enable knob outside of the operator API, but the operator should still consume it
| @@ -707,6 +704,14 @@ func (c *kubeControllersComponent) rbacSyncIDPGroupsRole() []client.Object { | |||
| ResourceNames: []string{"tigera-idp-groups"}, | |||
There was a problem hiding this comment.
Should this permission be gated on whether or not the config map is present and has the feature enabled?
There was a problem hiding this comment.
Added in "Gate the rbacsync controller on the feature gate, and skip multi-tenant".
rbacSyncEnabled(cfg) requires Enterprise, RBACManagementEnabled, and !Tenant.MultiTenant(). It gates this Role and the controller's entry in ENABLED_CONTROLLERS.
| // Create-only and ownerless, so an admin's toggle survives both reconciles and | ||
| // Installation deletion. Deleting the ConfigMap is fail-closed: consumers read a missing | ||
| // one as disabled and this re-seeds it disabled, since the previous value is not tracked. | ||
| func (r *ReconcileInstallation) seedRBACManagementConfigMap(ctx context.Context, log logr.Logger) error { |
There was a problem hiding this comment.
I would lean towards the operator having a read-only relationship with this config map, leaving the user fully in control of whether this config map exists. Any components that need to read from the config map can assume the feature is disabled entirely if the configmap doesn't exist
There was a problem hiding this comment.
Added in "Leave the RBAC management UI gate entirely to the admin".
The seeding is dropped: the installation controller no longer creates the ConfigMap, and render.RBACManagementConfigMap is removed. The operator's only contact with the gate is the three controllers reading it. A missing ConfigMap already read as disabled, so the default is unchanged; deleting it now stays deleted instead of reappearing on the next reconcile.
There was a problem hiding this comment.
Claude says that the RBACManagementEnabled check provided protection against the controller and role being enabled in multi-tenant environments. If so we'd need a tenancy check instead?
There was a problem hiding this comment.
Added in "Gate the rbacsync controller on the feature gate, and skip multi-tenant".
The old protection was incidental: the installation controller looked the Manager CR up by a cluster-scoped key, which never resolves on a multi-tenant management cluster, so the feature always read as disabled there. Reading a ConfigMap removed that side effect, so rbacSyncEnabled now checks !cfg.Tenant.MultiTenant() explicitly.
| // against the tigera-idp-groups ConfigMap. It reads this cluster's | ||
| // rbac-ui-config and reconciles nothing while the feature is off there. | ||
| enabledControllers = append(enabledControllers, "rbacsync") | ||
| kubeControllerRolePolicyRules = append(kubeControllerRolePolicyRules, rbacSyncControllerRules()...) |
There was a problem hiding this comment.
Should this permission be gated on whether or not the config map is present and has the feature enabled?
There was a problem hiding this comment.
Done, same rbacSyncEnabled gate as the Role above.
| ) | ||
| objsToCreate = append(objsToCreate, c.managedClustersUpdateRBAC()...) | ||
| if c.cfg.Manager.RBACManagementEnabled() && !c.cfg.Tenant.MultiTenant() { | ||
| if !c.cfg.Tenant.MultiTenant() { |
There was a problem hiding this comment.
Should this permission be gated on whether or not the config map is present and has the feature enabled?
There was a problem hiding this comment.
Done in "Gate the manager RBAC management UI access on the feature gate".
rbacManagementUIActive() (RBACManagementEnabled && !Tenant.MultiTenant()) gates the calico-manager cluster rules, the namespaced Role scoping ui-apis to the IdP resources, and the LDAP/AD egress policy. ui-apis keeps read access to the gate ConfigMap whenever the Role is rendered, so it can observe the feature being switched off.
| }, | ||
| ) | ||
| } | ||
| rules = append(rules, |
There was a problem hiding this comment.
Should this permission be gated on whether or not the config map is present and has the feature enabled?
There was a problem hiding this comment.
Done in "Gate the tigera-network-admin RBAC management rules on the feature gate".
The clusterrolebindings/rolebindings create/update/delete rule and the clusterroles/roles read are now behind the gate.
The RBAC management UI is gated per cluster by the rbac-ui-config ConfigMap in calico-system, keyed rbac-ui-enabled. The operator seeds it disabled and an admin with access to that cluster edits the value to toggle the feature there. ui-apis and rbacsync each read it live for the cluster they are acting on, so a toggle takes effect without restarting or rolling anything. Keep the constants in sync with ui-apis rbacmanagement/gate. The installation controller owns the seed: it runs on management, managed and standalone clusters alike, needs no Manager CR (a managed cluster has none, yet rbacsync still reads that cluster's copy over the tunnel), and is a single reconciler per cluster, so there is no second writer to race. The seed runs right after the namespace it lives in and is gated on the Enterprise variant. The admin's value is protected two ways. The seed is create-only, so an existing ConfigMap is never written and a concurrent create cannot lose the toggle. And it carries no OwnerReference, so deleting the Installation does not garbage-collect the switch and a reinstall does not silently turn the feature off. Deletion is fail-closed: both consumers read a missing ConfigMap as disabled, and the next reconcile re-seeds it disabled rather than restoring a value the operator does not track. The seed is logged, since the feature going quiet is otherwise invisible. Since activation is decided at runtime, the access the feature needs is rendered on every non-multi-tenant Enterprise cluster: the manager cluster and namespaced roles, the rbacsync controller and its roles, the tigera-network-admin rules, and the LDAP egress (gated on LDAP being configured on Authentication). ui-apis and rbacsync get read-only access to the gate, the value being the admin's to set. Multi-tenant management clusters render none of it, matching the force-disable on the ui-apis side. Manager.spec.rbacUI, the RBACManagementEnabled helper, and the Manager CR reads and watches the installation and apiserver controllers held for it are removed, since nothing derives the feature's state from a CR. EV-6816
Adds RBACManagementEnabled, parsed with strconv.ParseBool so True or 1 also work; a missing ConfigMap, missing key or unparsable value reads as disabled. The installation, apiserver and manager controllers each read the ConfigMap and watch it, so a toggle re-runs the reconcile. Nothing is gated on the value yet. EV-6816
tigera-network-admin carries create/update/delete on clusterrolebindings and rolebindings for the RBAC management UI. Rendering that on every Enterprise cluster grants escalation-capable permissions to clusters that never switch the feature on, so add the rules only while rbac-ui-config enables it. ui-apis still writes these impersonating the caller, so the apiserver enforces escalation against the user's own permissions. EV-6816
The calico-manager cluster rules, the namespaced Role scoping ui-apis to the IdP resources, and the LDAP/AD egress all exist to serve the RBAC management UI, so render them only while rbac-ui-config enables the feature. rbacManagementUIActive folds the gate together with the multi-tenant exclusion so the cluster rules and the namespaced grant cannot drift apart. ui-apis keeps read access to the gate itself, which is how it observes the feature being switched off. EV-6816
The rbacsync controller and its namespaced Role are rendered only while rbac-ui-config enables the RBAC management UI. Also restores the multi-tenant exclusion. That was previously an accident of how the value was read: the installation controller looked the Manager CR up by a cluster-scoped key, which never resolves on a multi-tenant management cluster, so the feature always read as disabled there. Reading the ConfigMap removed that side effect, so rbacSyncEnabled now checks tenancy explicitly. ENABLED_CONTROLLERS is a container env var, so toggling the gate restarts calico-kube-controllers. EV-6816
Review feedback: the operator should have a read-only relationship with rbac-ui-config, leaving the user in control of whether it exists. Drops the seeding. The installation controller no longer creates the ConfigMap and render.RBACManagementConfigMap goes with it, so the operator's only contact with the gate is the three controllers reading it. The read path was already fail-closed on a missing ConfigMap, so the default is unchanged. What changes is that deleting it now stays deleted rather than reappearing disabled on the next reconcile. EV-6816
The manager and apiserver reads had no test: the render tests set RBACManagementEnabled by hand, so nothing asserted either controller plumbs the ConfigMap through to its render config. Adds specs per controller that reconcile and assert on the rendered output -- the gated tigera-network-admin rules, and the namespaced IdP Role -- for the ConfigMap being absent, enabled and explicitly false. Also covers an unreadable ConfigMap degrading rather than rendering as disabled, and the gate being read on a managed cluster. EV-6816
Nothing granted write access to rbac-ui-config, so enabling the RBAC management UI needed cluster-admin. Since the operator no longer creates the ConfigMap, an admin with only tigera-network-admin could not switch the feature on at all. Adds the write access to tigera-network-admin, ungated: a rule rendered only while the feature is on could never be used to turn it on. create cannot be restricted by resource name, so this grant admits creating any ConfigMap in the namespace. Narrowing it would need a namespaced Role, which is not available here -- tigera-network-admin is a ClusterRole the customer binds themselves, so an operator-rendered Role would have no subject to bind to. EV-6816
b72d4e6 to
570c7e4
Compare
3 participants
Description
Replaces the
Manager.spec.rbacUI.stategate for the RBAC management UI with a per-clusterrbac-ui-configConfigMap incalico-system, keyedrbac-ui-enabled. The admin owns the ConfigMap outright; the operator, ui-apis and rbacsync only read it, so a toggle needs no restart or rollout.The consuming side is on calico-private branch https://github.com/tigera/calico-private/pull/12954 — that PR will be merged alongside this one.
Changes
Manager.spec.rbacUIremoved, CRD regenerated, along with the Manager CR reads and watches in the installation and apiserver controllers.tigera-network-adminrolebinding rules, thecalico-managercluster rules, the namespaced Role scoping ui-apis to the IdP resources, the LDAP/AD egress, and the rbacsync controller plus its Role. Multi-tenant renders none of it.tigera-network-admingets write access torbac-ui-config.Notes
createon ConfigMaps is namespace-wide — RBAC cannot restrictcreateby resource name, so the write grant admits creating any ConfigMap in the namespace. A namespaced Role would narrow it buttigera-network-adminis a ClusterRole the customer binds themselves.Release Note
For PR author
make gen-filesmake gen-versionsFor PR reviewers
A note for code reviewers - all pull requests must have the following:
kind/bugif this is a bugfix.kind/enhancementif this is a a new feature.enterpriseif this PR applies to Calico Enterprise only.