search

cybozu-go / accurate

Kubernetes controller for multi-tenancy. It propagates resources between namespaces accurately and allows tenant users to create/delete sub-namespaces.
https://cybozu-go.github.io/accurate/
Apache License 2.0
38 stars 5 forks source link

Controller RBAC is too permissive #82

Closed erikgb closed 11 months ago

erikgb commented 11 months ago

Describe the bug

The cluster role bound to the controller is too permissive by default, and I think this is a bug: https://github.com/cybozu-go/accurate/blob/main/charts/accurate/templates/generated/generated.yaml#L77-L84

If this permissive RBAC is required for the controller to operate, I think why should be documented.

We are evaluating this project as an alternative to HNC and might file a few issues/PRs for minor fixes. I hope there is a maintainer team with some bandwidth and interest in "external" contributions. 😄

ymmt2005 commented 11 months ago

@erikgb Thank you for your interest in this project. That permissive RBAC is not required. Actually, we should use controller.additionalRBAC.rule parameter to give necessary permissions.

erikgb commented 11 months ago

Thanks @ymmt2005! Will you create a PR to fix this? Or can I just create one - suggesting just to remove the permissive RBAC? Or will that require other additional changes?

ymmt2005 commented 11 months ago

This permission was added in #20 to allow the accurate controller to check annotations. I need further investigation into what the problem was.

ymmt2005 commented 11 months ago

So, the problem was we got an error from the accurate controller when it tried to access a parent resource of a Secret, and the parent was a cluster-scoped CRD.

As the parent can be anything, we allowed the accurate controller to read any kind of resources to suppress that error.

The relevant feature is this. https://cybozu-go.github.io/accurate/propagation.html#annotating-a-resource-to-propagate-resources-created-from-it

We thought it'd be pretty difficult for normal users to identify the error and fix it by giving additional permissions.

erikgb commented 11 months ago

a parent resource of a Secret, and the parent was a cluster-scoped CRD

Does this make sense? How can a cluster-scoped resource be a parent of a namespace-scoped resource?

BTW the example in the docs of this feature is obsolete, as cert-manager now supports secret templates. 😉 https://cert-manager.io/docs/reference/api-docs/#cert-manager.io/v1.CertificateSecretTemplate

ymmt2005 commented 11 months ago

I was wrong. The resource was CephCluster from Rook, which is namespace-scoped. https://rook.io/docs/rook/v1.12/CRDs/Cluster/ceph-cluster-crd/

The problem was, although we granted admin ClusterRole to the accurate controller, Rook did not aggregate permissions for CephCluster into admin, therefore, the accurate controller could not get them. https://github.com/cybozu-go/accurate/blob/main/charts/accurate/templates/generated/generated.yaml#L188-L204

erikgb commented 11 months ago

The problem was, although we granted admin ClusterRole to the accurate controller, Rook did not aggregate permissions for CephCluster into admin, therefore, the accurate controller could not get them.

I think it should be the user's responsibility to configure RBAC. Even granting (namespace) admin cluster-wide RBAC is questionable IMO.

ymmt2005 commented 11 months ago

yes, we can put these as a default/recommended setting in Helm values.yaml.

erikgb commented 11 months ago

yes, we can put these as a default/recommended setting in Helm values.yaml

I suppose you are addressing the cluster-wide admin permission now? Read access to ALL resources is not a good default IMO.

ymmt2005 commented 11 months ago

Read access to ALL resources is not a good default IMO.

Agreed.