KIC 2.0 CTRLMGR RBAC Roundup

[shaneutt]

The purpose of this issue is to ensure we review and restrict our RBAC permissions provided to the controller manager prior to the first KIC 2.0 alpha release as the kubebuilder defaults are too permissive.

Acceptance Criteria:

• review and reduce the RBAC configuration for the controller manager down to exactly what is needed
• compare the new RBAC rules with the previous KIC 1.x RBAC rules and document/justify any diffs

[rainest]

Spike time

Status quo

Kubebuilder doesn't appear to have clear defaults on resources--it looks like everything is derived from markers we write. We currently impose defaults (or rather, only allow a single configuration throughout) through the templates in railgun/hack/generators/controllers/networking/main.go. All controllers we generate from that receive the same set of templated permissions, which allow all verbs:

//+kubebuilder:rbac:groups={{.URL}},resources={{.Plural}},verbs=get;list;watch;create;update;patch;delete
//+kubebuilder:rbac:groups={{.URL}},resources={{.Plural}}/status,verbs=get;update;patch
//+kubebuilder:rbac:groups={{.URL}},resources={{.Plural}}/finalizers,verbs=update

and dumps them into a single ClusterRole, which we name in our Makefile target:

manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects.
    $(CONTROLLER_GEN) $(CRD_OPTIONS) rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases

https://book.kubebuilder.io/reference/markers/rbac.html covers the //+kubebuilder:rbac options.
https://book.kubebuilder.io/reference/controller-gen.html#output-rules covers the generator options.

Namespaced role generation

Part of what we'd like to do is provide two variants of the RBAC rule set, one appropriate for the permissive default (cluster-wide) configuration and one appropriate for a restricted, per namespace configuration. It looks like Kubebuilder can get us partway there by adding namespaced copy of the existing tags:

//+kubebuilder:rbac:groups="",namespace=foons,resources=services,verbs=get;list;watch;create;update;patch;delete
//+kubebuilder:rbac:groups="",namespace=foons,resources=services/status,verbs=get;update;patch
//+kubebuilder:rbac:groups="",namespace=foons,resources=services/finalizers,verbs=update

There are some caveats:

• The Role uses the same name as the ClusterRole, which isn't really an issue, but is maybe worth noting.
• Generation always creates a single Role and ClusterRole. There's no way to separate permissions into discrete Roles and ClusterRoles.
• The namespace is defined in the tag and propagates to the Role definition, i.e. you do not get a generic Role whose namespace you can specify at creation time. Roles generated here won't be immediately appropriate for a given cluster (and really can't be--we have no way of knowing a user's namespaces ahead of time, so at best we can prescribe a single standard namespace). We'd need to use these as some sort of template for kustomize and/or Helm.
• The output roles provide no way to filter the generated roles, and we'll always generate both sets barring some separation of the generated controller file (which I don't think we need or want). The contents of config/bases aren't directly delivered to end users, however, so this should be fine--we instead handle separation via different kustomization.yamls and default the all-in-ones to the cluster version.

Kustomization strategic merge example for end users will presumably look like:

apiVersion: v1
kind: Role
metadata:
  name: manager-role
  namespace: FILL-IN-YOUR-NAMESPACE

Helm will use a for loop to iterate over a set of namespaces and generate a role for each.

Restricting permission verbs by resource

If we do not wish to apply the same set of permissions to all resources, as we do now, we need a new typeNeeded field for the generator. The most granular approach there is to provide the entire tag strings, but this goes against what we're currently doing (where some portion of the tag string is generated) and requires you to update the same value (e.g. the plural name) in multiple places within typesNeeded, which is a usability pitfall.

Providing the verb list for the main resource thus seems like the next step. Unsure if we want to be able to toggle status/finalizer permissions off, but I'd expect their verbs are consistent if they're present.

Differences from existing v1 permissions

Stock v1 permissions

The existing v1 permissions are more limited, and unfortunately a bit hard to compare because of the way K8S RBAC groups things. Nevertheless:

• Most resources lack create. The only things that do have create are ConfigMaps (AFAIK for the leader election map, though we don't restrict it at all) and Events (not actually sure how we use these).
• Some resources (e.g. endpoints, secrets) lack get, and only have list/watch. Not sure why, as list includes content. Is there a distinction between list,get and list alone in what you can access?
• Only status subresources (on Ingress and TCPIngress only) and ConfigMaps have update.
• We include a get nodes permission to list which nodes the controller is running on, though I forget exactly why/how we present this status info.
• We create permissions for some things (e.g. events) that we don't create controllers for (nodes, events). Presumably we just stuff hand-written Kubebuilder tags for these somewhere.

Chart variant

The permissions we install with the chart actually differ further, though it looks like the main change is the addition of a Role that further locks down the leader ConfigMap to the controller namespace and limits updates to its name. Not really sure why we also have a namespace/pod/secret get permission (secret for the admission cert, I guess?).

Additional permissions needed by v2

It's not clear that v2 needs its additional verbs. I'm fairly certain we never create or update resources other than ConfigMaps and Events. We may not need all the new status permissions (e.g. Services have a use case if we can implement a map to the associated Kong resources, which we currently lack, whereas Endpoints we almost certainly don't need to update status for). I suppose we need the finalizer permissions, though I don't know enough detail about how we use those to say for sure.

The generic approach to verbs appears to have just been a permissive kitchen sink approach to avoid the RBAC question initially, and we can probably revert to the set of permissions we had in v1 with some minor additions once we have an implementation that can discriminate properly.

Other notes

• It doesn't look like the (Cluster)RoleBinding resources are generated, so we need to write a new one for the namespaced Roles. These are simple and don't ever really change though.
• It's not clear how (if at all) we're using the various roles that are apparently generated for our CRDs, e.g. kongingress_viewer_role.yaml and kongingress_editor_role.yaml. Are these intended for person accounts rather than the controller service account?

[shaneutt]

Great writeup. Sounds like we need to drop the extra verbs at the very least... but perhaps we should be considering making upstream improvements to Kubebuilder to get the improvements for separate roles and bindings, and greater specificity we desire.

As a course of action, I would be in favor of us taking this approach:

1. make the tweaks we can to reduce permissions for KIC 2.0 down to something closer to 1.x for now
2. start some work in upstream kubebuilder to add more features to generate the rules we want
3. (try) to get our patches in prior to KIC 2.0 GA

[rainest]

I don't think there's anything we actually need new from Kubebuilder--some of those points are just notes. We don't need multiple roles of the same type, and we already use kustomize to apply additional post-processing to the generated roles. We'd need to add new kustomize stuff, but that's entirely doable with what we have now.

[rainest]

Alright, well, one annoyance: the Role and ClusterRole end up in the same file, and we won't normally apply the Role. We can use the second workflow from kubernetes-sigs/kustomize#1593 (comment) to separate them.

[shaneutt]

resolved by #1457