habitat-sh / habitat-operator

A Kubernetes operator for Habitat services
Apache License 2.0
61 stars 17 forks source link

[RBAC] Harden RBAC policies for the operator #346

Closed surajssd closed 6 years ago

surajssd commented 6 years ago

Right now we ask user to create ClusterRole and ClusterRoleBinding. Which grants the operator cluster wide permissions, which is too much of permissions.

So this PR tries to limit those permissions by assigning only Role and RoleBinding, which are namespace limited.

Fixes https://github.com/habitat-sh/habitat-operator/issues/338

surajssd commented 6 years ago

Few questions though:

1) Do we want to permanently limit this operator to work only in single namespace? 2) Or do we want it to switch between ClusterScope or Namespaced? 3) If 2 is true then right now I am checking that using flag namespace if value is given then the code scopes it to single namespace. But then it is expected from user to create appropriate Roles, RoleBindings and CRD definition.

surajssd commented 6 years ago

If you want to test this and create CRD manually then the definition looks like this:

apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
  name: habitats.habitat.sh
spec:
  group: habitat.sh
  names:
    kind: Habitat
    listKind: HabitatList
    plural: habitats
    shortNames:
    - hab
    singular: habitat
  scope: Namespaced
  version: v1beta1
krnowak commented 6 years ago

Do we want to permanently limit this operator to work only in single namespace?

No. I would like to have something like this:

Or do we want it to switch between ClusterScope or Namespaced?

More like that. What I'm really not sure is the process of registering a CRD. That's because even checking if the CRD was registered requires a permission to get a CRD, which is a cluster-wide resource, thus requiring use of the ClusterRole instead of a Role. Maybe we could add a flag like, I dunno, --assume-crd-registered which would skip the checks altogether and thus drop the need for the permission. So at this point it would be a responsibility of the cluster admin to ensure that the CRD is registered prior to running the operator.

If 2 is true then right now I am checking that using flag namespace if value is given then the code scopes it to single namespace. But then it is expected from user to create appropriate Roles, RoleBindings and CRD definition.

Yes. The RBAC rules we have are just examples. A user can use those and they should work fine, but if the user wants some restricted rules, then the user can write them using our example rules as a base. What we can do is to add example RBAC rules, which restrict the operator to a single namespace, so the user can copy them and modify the namespace name to user's own needs.

One sticking point is our helm package where we use the RBAC rules with ClusterRoles, so I don't know what to do with that, yet.

With all that above, user could feed the k8s cluster with RBAC rules with only a Role in it, restricting the operator to some namespace, create a CRD explicitly and then run something like ./habitat-operator --namespace=<SOME_NAMESPACE> --assume-crd-registered.

Might also be a good idea to have the CRD yaml file somewhere in the repo, so the user does not need to write their own based on the CreateCRD somewhere in the operator's codebase.

surajssd commented 6 years ago

@krnowak added those flags, as you mentioned. Now about testing this I have some questions.

IIUC, the test setup code starts the operator once and then test runs against it. Now deploy multiple operator should the code base of setting up operator change? WDYT?

surajssd commented 6 years ago

Fixing tests right now.