search

volcano-sh / volcano

A Cloud Native Batch System (Project under CNCF)
https://volcano.sh
Apache License 2.0
4.17k stars 962 forks source link

BUG: Unnecessary RBAC permissions #3505

Closed Yseona closed 3 months ago

Yseona commented 4 months ago

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:

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

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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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

Monokaix commented 4 months ago

@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 commented 4 months ago

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

Monokaix commented 4 months ago

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

kaaass commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

/close

volcano-sh-bot commented 3 months ago

@Monokaix: Closing this issue.

In response to [this](https://github.com/volcano-sh/volcano/issues/3505#issuecomment-2216036497): >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.