hashicorp / vault

A tool for secrets management, encryption as a service, and privileged access management
https://www.vaultproject.io/
Other
30.96k stars 4.18k forks source link

[Kubernetes Secrets Engine] Allow ClusterRoles to be used as reference for RoleBinding #25579

Open refucktor opened 7 months ago

refucktor commented 7 months ago

Is your feature request related to a problem? Please describe. I'm using a workflow where I pre-define the Role/ClusterRole in K8s and in the K8s secrets role I'm providing the kubernetes_role_name. Then I noticed the following behavior:

In summary, having a generic ClusterRole (let's called "ABC") with some permissions and then creating a K8s secrets engine role of type Role using the ClusterRole name "ABC" the final RoleBinding is not able to identify that the reference role is of type ClusterRole and instead creates a reference of type Role

Describe the solution you'd like I would like to have one additional parameter while creating the K8s Secrets Engine Role that allows me to specify if the reference role used for the RoleBinding is of type Role or ClusterRole, to stay backward compatible, I would give it a default value from the value of the parameter kubernetes_role_type

Describe alternatives you've considered All alternatives will end up in a much less pleasant experience:

Explain any additional use-cases The main use case is to have the ability to create generic ClusterRoles to save a lot of scaffold and configuration and then use those to create Role for the target namespaces

tomhjp commented 7 months ago

Thanks for the interest! I want to check that I understand the request properly.

Currently in the Vault role config, you set up a reference to either a Kubernetes Role or ClusterRole. When a Vault client generates the creds, if you chose ClusterRole the client gets the chance to specify if they want to create a RoleBinding or ClusterRoleBinding against that ClusterRole. If you chose a Role, they don't get a choice, they can only generate a RoleBinding against the Role.

It seems like what you want is a single reusable ClusterRole definition that can only be used to generate RoleBindings?

It's perhaps not the most intuitive UX, so I'm open to discussing alternatives, but if you set cluster_role_binding as one of the denied_parameters in clients' policies, would that achieve the same outcome? To be clear, that means you would set the kubernetes_role_type on the role as ClusterRole, so that the plugin correctly finds the referenced k8s resource, but Vault clients wouldn't be able to generate ClusterRoleBindings against it.

refucktor commented 7 months ago

thanks for the comment 👋🏻

  1. yes, you explained the current behavior perfectly!

It seems like what you want is a single reusable ClusterRole definition that can only be used to generate RoleBindings?

  1. More or less, to rephrase it, I would like to have the possibility to create vault roles using kubernetes_role_type=Role and using any existing k8s role name whether or not that role is Role or ClusterRole, and the problem for me is that I have no option to define that last part
  1. your alternative is an excellent workaround that we already considered but it comes with a few drawbacks which IMHO are somehow relevant

I slightly disagree on the possible complexity for UX 😊, we are talking about 1 extra optional parameter for configuring the vault role which, if not set, should use the value from kubernetes_role_type, then on the client side to generate the credentials there is no change at all.

sorry if I made a long comment, I actually enjoyed our exchange 🤓, looking forward to read your thoughts

tomhjp commented 7 months ago

Thanks, that makes sense. By the way, this comment

It's perhaps not the most intuitive UX

was in reference to my own suggestion. I agree it's awkward to grant permissions and then have to subsequently and consistently take some of them away. But it helps me to understand the use-case better to have your reasoning on that idea laid out, thanks!

Given this is only a binary choice anyway, how about a new disallow_cluster_role_binding parameter on the Vault role? I think it still allows the full amount of flexibility you need, but with a more intuitive/localised UX than relying on policies, and is pretty easy to reason about. I think setting it while kuberenetes_role_type is set to Role should be rejected as invalid config, and it can keep backwards compatibility by defaulting to false. But if you set it while kubernetes_role_type is ClusterRole, then clients can only generate RoleBindings against the ClusterRole.

refucktor commented 7 months ago

hey @tomhjp! I have a similar approach to your suggestion in the PR I sent to the kubernetes secrets repo, but in my solution, we just need to specify if the target k8s role reference is of type Role or ClusterRole. This approach is one of the most backwards compatible I could come up with, in that way, the user generating the credentials still see that it's vault role has a type of Role and if she tries to toggle to ClusterRole then the error is as expected.