Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Controller RBAC is too permissive #82

Closed
erikgb opened this issue Sep 23, 2023 · 10 comments · Fixed by #89
Closed

Controller RBAC is too permissive #82

erikgb opened this issue Sep 23, 2023 · 10 comments · Fixed by #89
Assignees
@ymmt2005
Labels
bug Something isn't working

Comments

@erikgb
Copy link
Contributor

erikgb commented Sep 23, 2023 •
edited
Loading

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. 😄
@erikgb erikgb added the bug Something isn't working label Sep 23, 2023
@ymmt2005
Copy link
Member

ymmt2005 commented Sep 24, 2023 •
edited
Loading

@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
Copy link
Contributor Author

erikgb commented Sep 24, 2023 •
edited
Loading

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
Copy link
Member

ymmt2005 commented Sep 25, 2023 •
edited
Loading

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

@ymmt2005
Copy link
Member

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
Copy link
Contributor Author

erikgb commented Sep 25, 2023 •
edited
Loading

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
Copy link
Member

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
Copy link
Contributor Author

erikgb commented Sep 25, 2023

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
Copy link
Member

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

@erikgb
Copy link
Contributor Author

erikgb commented Sep 25, 2023

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
Copy link
Member

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

Agreed.

@ymmt2005 ymmt2005 self-assigned this Sep 27, 2023
ymmt2005 added a commit that referenced this issue Sep 27, 2023
@ymmt2005
Make admin privileges configurable
10fc5b9 
Fix #82.

With this change, we stop granting the below permission to the
accurate controller.

```yaml
  - apiGroups:
      - '*'
    resources:
      - '*'
    verbs:
      - get
      - list
      - watch
```

Also, we make the ClusterRole admin optional.
The Helm chart now takes optional ClusterRoles to be granted.
@ymmt2005 ymmt2005 mentioned this issue Sep 27, 2023
Merged
ymmt2005 added a commit that referenced this issue Sep 27, 2023
@ymmt2005
Make admin privileges configurable
47512fd 
Fix #82.

With this change, we stop granting the below permission to the
accurate controller.

```yaml
  - apiGroups:
      - '*'
    resources:
      - '*'
    verbs:
      - get
      - list
      - watch
```

Also, we make the ClusterRole admin optional.
The Helm chart now takes optional ClusterRoles to be granted.
ymmt2005 added a commit that referenced this issue Sep 27, 2023
@ymmt2005
Make admin privileges configurable
f6f5086 
Fix #82.

With this change, we stop granting the below permission to the
accurate controller.

```yaml
  - apiGroups:
      - '*'
    resources:
      - '*'
    verbs:
      - get
      - list
      - watch
```

Also, we make the ClusterRole admin optional.
The Helm chart now takes optional ClusterRoles to be granted.
ymmt2005 added a commit that referenced this issue Sep 27, 2023
@ymmt2005
Make admin privileges configurable
9fa15d7 
Fix #82.

With this change, we stop granting the below permission to the
accurate controller.

```yaml
  - apiGroups:
      - '*'
    resources:
      - '*'
    verbs:
      - get
      - list
      - watch
```

Also, we make the ClusterRole admin optional.
The Helm chart now takes optional ClusterRoles to be granted.
ymmt2005 added a commit that referenced this issue Sep 27, 2023
@ymmt2005
Make admin privileges configurable
9c19f30 
Fix #82.

With this change, we stop granting the below permission to the
accurate controller.

```yaml
  - apiGroups:
      - '*'
    resources:
      - '*'
    verbs:
      - get
      - list
      - watch
```

Also, we make the ClusterRole admin optional.
The Helm chart now takes optional ClusterRoles to be granted.
ymmt2005 added a commit that referenced this issue Sep 27, 2023
@ymmt2005
Make admin privileges configurable
2b50aa1 
Fix #82.

With this change, we stop granting the below permission to the
accurate controller.

```yaml
  - apiGroups:
      - '*'
    resources:
      - '*'
    verbs:
      - get
      - list
      - watch
```

Also, we make the ClusterRole admin optional.
The Helm chart now takes optional ClusterRoles to be granted.
ymmt2005 added a commit that referenced this issue Sep 27, 2023
@ymmt2005
Make admin privileges configurable
f8bff61 
Fix #82.

With this change, we stop granting the below permission to the
accurate controller.

```yaml
  - apiGroups:
      - '*'
    resources:
      - '*'
    verbs:
      - get
      - list
      - watch
```

Also, we make the ClusterRole admin optional.
The Helm chart now takes optional ClusterRoles to be granted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ymmt2005 ymmt2005

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make admin privileges configurable cybozu-go/accurate
2 participants
@erikgb @ymmt2005