spidernet-io / spiderpool

Underlay and RDMA network solution of the Kubernetes, for bare metal, VM and any public cloud
https://spidernet-io.github.io/spiderpool/
Apache License 2.0
507 stars 73 forks source link

fix(charts): Remove unnecessary sensitive permissions for DaemonSet agent and Pod init #3522

Closed kaaass closed 3 weeks ago

kaaass commented 1 month ago

What type of PR is this?

What this PR does / why we need it:

This PR removes unnecessary RBAC permissions for the DaemonSet agent and Pod init to avoid potential security risks. Technically, this PR does:

  1. Create a separate ClusterRole for each workload
  2. Remove some unnecessary permissions applied by the DaemonSet agent and Pod init

This PR minimizes the deletion of permissions to avoid affecting the normal function of the APP. However, there should still be some unnecessary RBAC permissions (such as for the Deployment controller).

Also, this PR DOES NOT include changing the kubebuilder annotation in: pkg/k8s/apis/spiderpool.spidernet.io/v2beta1/rbac.go

Which issue(s) this PR fixes:

Fixes #3420 Fixes #3361

Special notes for your reviewer:

The removed permission from the permission rules are as follows:

I am more aggressive in deleting the permissions of DaemonSet agent, mainly because DaemonSet will be deployed to any worker node in the cluster, so attackers have more opportunities to obtain the ServiceAccount token of DaemonSet agent.

ty-dc commented 1 month ago

After the fix, some issues occurred, it seems that there is insufficient permissions.

  Warning  FailedCreatePodSandBox  118s                kubelet            Failed to create pod sandbox: rpc error: code = Unknown desc = failed to setup network for sandbox "91390322e1fcc5b6f8011c1f9292f3eaaba9db0b6704d79a8315413f16f05b04": plugin type="multus" name="multus-cni-network" failed (add): [default/test-pod-57c88b845c-prtfb/35ed07d0-ee08-4a64-963b-75dfeff0eb2c:macvlan-vlan0]: error adding container to network "macvlan-vlan0": failed to GetCoordinatorConfig: [GET /coordinator/config][500] getCoordinatorConfigFailure  spidercoordinator: default no ready
kaaass commented 1 month ago

Thanks @ty-dc. Seems like the removal in my latest commit is too aggressive. I'll revert them.

cyclinder commented 1 month ago

Hi @kaaass, codes look good now, Could you squash the comments? now we have 6 comments on the PR. it would be nice if we can squash to one comment.

kaaass commented 1 month ago

@cyclinder Sure! I've pushed them

weizhoublue commented 1 month ago

actually, I expect upgrading CI to test this PR @ty-dc

ty-dc commented 1 month ago

actually, I expect upgrading CI to test this PR @ty-dc

ok