BUG: Unnecessary RBAC permissions

[echoounlimited]

Description

The bug is that the Deployment volcano-admission and volcano-controllers in the charts have too much RBAC permissions than they need. The service account of volcano-admission is bound to a clusterrole (admission.yaml#L44) with the following permissions:

• get verb of the secrets resource (ClusterRole)

The service account of volcano-controllers is bound to a clusterrole (controllers.yaml#L16) with the following permissions:

• update verb of the pods resource (ClusterRole)
• get/list verb of the secrets resource (ClusterRole)

After reading the source code of volcano, I didn't find any Kubernetes API usages using these permissions. Besides, some of these unused permissions may have potential risks. For example, if malicious users gain control of a Kubernetes node running a volcano-controllers pod, they can list all the names of the secrets, and with the name, they can get the details of all the secrets objects (since this is declared in a ClusterRole).

Therefore, for security reasons, I suggest checking these permissions to determine if they are truly unnecessary. If they are, the issue should be fixed by removing the unnecessary permission or other feasible methods.

To Reproduce

Use charts with default values.

[Monokaix]

1、volcano admission need generate secrets in a init job, and I will fire a pr to shrink permissions. 2、volcano controller's permissions of list/watch secrets have been removed in latest version. 3、update permission is necessary for volcano controller because it need update pod's podgroup, etc.

[kaaass]

@Monokaix I tried to find all update/patch relate operation to pods. Maybe we only need the patch verb instead of update verb?

https://github.com/volcano-sh/volcano/blob/fa0548f1e7ab7930e6ba91b97394106d56eb7278/pkg/scheduler/api/devices/nvidia/vgpu/utils.go#L437-L438

https://github.com/volcano-sh/volcano/blob/fa0548f1e7ab7930e6ba91b97394106d56eb7278/pkg/controllers/podgroup/pg_controller_handler.go#L105

https://github.com/volcano-sh/volcano/blob/fa0548f1e7ab7930e6ba91b97394106d56eb7278/pkg/scheduler/api/devices/nvidia/gpushare/device_info.go#L136

https://github.com/volcano-sh/volcano/blob/fa0548f1e7ab7930e6ba91b97394106d56eb7278/pkg/scheduler/api/devices/nvidia/gpushare/device_info.go#L195

https://github.com/volcano-sh/volcano/blob/fa0548f1e7ab7930e6ba91b97394106d56eb7278/pkg/scheduler/api/devices/nvidia/gpushare/device_info.go#L212

[lowang-bh]

similar issues : A potential risk in volcano that could lead to takeover of the cluster #3446

[Monokaix]

@Monokaix I tried to find all update/patch relate operation to pods. Maybe we only need the patch verb instead of update verb?

https://github.com/volcano-sh/volcano/blob/fa0548f1e7ab7930e6ba91b97394106d56eb7278/pkg/scheduler/api/devices/nvidia/vgpu/utils.go#L437-L438

https://github.com/volcano-sh/volcano/blob/fa0548f1e7ab7930e6ba91b97394106d56eb7278/pkg/controllers/podgroup/pg_controller_handler.go#L105

https://github.com/volcano-sh/volcano/blob/fa0548f1e7ab7930e6ba91b97394106d56eb7278/pkg/scheduler/api/devices/nvidia/gpushare/device_info.go#L136

https://github.com/volcano-sh/volcano/blob/fa0548f1e7ab7930e6ba91b97394106d56eb7278/pkg/scheduler/api/devices/nvidia/gpushare/device_info.go#L195

https://github.com/volcano-sh/volcano/blob/fa0548f1e7ab7930e6ba91b97394106d56eb7278/pkg/scheduler/api/devices/nvidia/gpushare/device_info.go#L212

After a deep insight, volcano scheduler called UpdateStatus method, which needs update verb role: )

[Monokaix]

And volcano admission related permissions has been reduced in pr https://github.com/volcano-sh/volcano/pull/3504

[Monokaix]

@kaaass I think we can remove update verb in volcano controller, you can do that if you're available: )

[kaaass]

After a deep insight, volcano scheduler called UpdateStatus method, which needs update verb role: )

@Monokaix Thank you for reply : )

UpdateStatus only requires permission to subresource pods/status (client-go source code). Subresource uses a separate permission grants (document).

@kaaass I think we can remove update verb in volcano controller, you can do that if you're available: )

I'm happy to do that! Sadly I'm a little busy at the time. I'll give it a try if it is still unsolved maybe later this week : )

[Monokaix]

After a deep insight, volcano scheduler called UpdateStatus method, which needs update verb role: )

@Monokaix Thank you for reply : )

UpdateStatus only requires permission to subresource pods/status (client-go source code). Subresource uses a separate permission grants (document).

@kaaass I think we can remove update verb in volcano controller, you can do that if you're available: )

I'm happy to do that! Sadly I'm a little busy at the time. I'll give it a try if it is still unsolved maybe later this week : )

That's ok.

[Monokaix]

/close

[volcano-sh-bot]

@Monokaix: Closing this issue.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.