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.

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

More secure rbac #625

Merged
merged 14 commits into from
Apr 15, 2024
Merged

More secure rbac #625

merged 14 commits into from
Apr 15, 2024

Conversation

jkremser
Copy link
Contributor

@jkremser jkremser commented Apr 8, 2024

Issue #624

the changes rely on WATCH_NAMESPACE variable. So if it is empty, we basically install almost the same rbac as before (ClusterRole with ClusterRoleBinding so that it can watch for events in all the namespaces).

If it contains one or multiple namespaces (so it assumes this change is merged), it will use normal RoleBinding together with the cluster role so that these rules are namespaced for each listed namespace in that env var.

Note: Normal RoleBinding + ClusterRole works the same way as RoleBinding with normal Role, but this way we can reuse the same resource and lower the complexity of helm chart a bit.

It also contains a way to restrict the set of secrets, the operator should be able to look into. By default it's the same behavior as before, (well, if WATCH_NAMESPACE is not empty, it will be restricted only to secrets in that ns), but one can also specify: --set permissions.operator.restrict.namesAllowList="{foo,bar}" so that the operator can read only secrets in that namespace called "foo" and "bar"

The other RBAC-related change is the whitelisting of CRDs that can be scaled by the operator. Again, by default it's the same behavior as before, but if requested, one can restrict the set of CRDs that can be referenced by scaled object in advance during the helm installation by:

rbac:
  enabledCustomScaledRefKinds: true
  scaledRefKinds:
   - apiGroup: argoproj.io
     kind: Rollout
   - apiGroup: cluster.x-k8s.io
     kind: MachineDeployment
   - apiGroup: cluster.x-k8s.io
     kind: MachinePool

The PR also splits the RBAC for all three individual components so that webhooks or metric server don't have the same rights as the operator itself(that actually had a right to read ("get") every single resource in the cluster). Now each component has its own service account with a taylored roles attached to it.

Since the PR is quite complex to review, I've prepared some deployment scenarios and captured the resulting RBAC using the rbac-tool kubectl plugin.

  • Scenario 1 - no restrictions, just separate RBACs for each KEDA component (which is common to each scenario)

    values.yaml
      crds:
        install: true
      image:
        keda:
          repository: docker.io/jkremser/keda # this contains the unreleased change for watching multiple explicitly mentioned namespaces
          tag: latest
        webhooks:
          repository: docker.io/jkremser/keda-admission-webhooks
          tag: latest
      
      logging:
        operator:
          level: debug
          format: json
          stackTracesEnabled: true

    final rbac:

  • Scenario 2 - some CRDs are whitelisted, watching default ns + 2 whitelisted secrets

    values.yaml
        # the values.yaml from Scenario 1 + the following:
        watchNamespace: default
        rbac:
          enabledCustomScaledRefKinds: true
          scaledRefKinds:
           - apiGroup: argoproj.io
             kind: Rollout
           - apiGroup: cluster.x-k8s.io
             kind: MachineDeployment
           - apiGroup: cluster.x-k8s.io
             kind: MachinePool
        
        permissions:
          operator:
            restrict:
              namesAllowList:
               - secretPassword
               - someApiKey
    

    final rbac:

  • Scenario 3 - all CRDs are allowed + some namespaces are being watched + some secrets are whitelisted

    values.yaml
        # the values.yaml from Scenario 1 + the following:
        rbac:
          enabledCustomScaledRefKinds: true
        watchNamespace: default,keda,production
        permissions:
          operator:
            restrict:
              namesAllowList:
               - foo
               - bar
               - baz

    final rbac:

  • Scenario 4 - all namespaces are watched, only secrets called "bar" can be read

    values.yaml
        # the values.yaml from Scenario 1 + the following:
        watchNamespace: "" # all namespaces -> cluster-wide rights
        enabledCustomScaledRefKinds: false
        permissions:
          operator:
            restrict:
              namesAllowList:
               - bar

    final rbac:

note: for all four scenarios, I've created a Deployment, ScaledObject, checked that hpa was created and deleted it and was checking if there were any rbac related issues

Signed-off-by: Jirka Kremser <[email protected]>
@jkremser jkremser requested a review from a team as a code owner April 8, 2024 14:14
@jkremser jkremser changed the title [wip] More secure rbac More secure rbac Apr 10, 2024
Signed-off-by: Jirka Kremser <[email protected]>
@JorTurFer
Copy link
Member

WOW, this PR is a great idea! I'm checking it in depth because you said, it's complex and RBAC is risky, but I'll post something asap

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking nice! @zroubalik @tomkerkhove , we should test this really well as it's an important change related with RBAC.
After merging this, we will have to be careful when we apply RBAC changes from core to the chart

keda/templates/metrics-server/serviceaccount.yaml Outdated Show resolved Hide resolved
keda/templates/metrics-server/serviceaccount.yaml Outdated Show resolved Hide resolved
keda/templates/webhooks/serviceaccount.yaml Outdated Show resolved Hide resolved
keda/templates/webhooks/serviceaccount.yaml Show resolved Hide resolved
keda/templates/manager/clusterrole.yaml Show resolved Hide resolved
keda/templates/manager/clusterrole.yaml Outdated Show resolved Hide resolved
keda/templates/manager/clusterrole.yaml Outdated Show resolved Hide resolved
keda/templates/manager/clusterrole.yaml Show resolved Hide resolved
keda/templates/manager/minimal-rbac.yaml Outdated Show resolved Hide resolved
Signed-off-by: Jirka Kremser <[email protected]>
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, I really like it!

@wozniakjan PTAL, extra pair of eyes are always better :)

keda/templates/manager/clusterrole.yaml Show resolved Hide resolved
keda/templates/metrics-server/clusterrolebinding.yaml Outdated Show resolved Hide resolved
keda/templates/manager/serviceaccount.yaml Show resolved Hide resolved
Copy link
Member

@wozniakjan wozniakjan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks really good, here are couple of minor formatting nits

keda/README.md Outdated Show resolved Hide resolved
keda/README.md Outdated Show resolved Hide resolved
keda/values.yaml Outdated Show resolved Hide resolved
keda/values.yaml Outdated Show resolved Hide resolved
jkremser and others added 4 commits April 12, 2024 16:44
Co-authored-by: Jan Wozniak <[email protected]>
Signed-off-by: Jirka Kremser <[email protected]>
Co-authored-by: Jan Wozniak <[email protected]>
Signed-off-by: Jirka Kremser <[email protected]>
Co-authored-by: Jan Wozniak <[email protected]>
Signed-off-by: Jirka Kremser <[email protected]>
Co-authored-by: Jan Wozniak <[email protected]>
Signed-off-by: Jirka Kremser <[email protected]>
@zroubalik zroubalik merged commit 207bab7 into kedacore:main Apr 15, 2024
37 checks passed
This was referenced Apr 28, 2024
QuentinBisson pushed a commit to giantswarm/keda-upstream that referenced this pull request Jul 1, 2024
Signed-off-by: Jirka Kremser <[email protected]>
Signed-off-by: Jirka Kremser <[email protected]>
Co-authored-by: Jan Wozniak <[email protected]>
@joebowbeer
Copy link
Contributor

@jkremser Should the instructions for restricting access to secrets be updated?

https://keda.sh/docs/2.15/operate/cluster/#restrict-secret-access

Also see kedacore/keda-docs#1307

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants