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

fix: Replace wildcards in RBAC objects with explicit resources and verbs #6129

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

legal90
Copy link

@legal90 legal90 commented Sep 3, 2024

According to Kubernetes documentation and various k8s security guidelines, wildcards in resource and verb entries should be avoided:

Warning

Using wildcards in resource and verb entries could result in overly permissive access being granted to sensitive resources. For instance, if a new resource type is added, or a new subresource is added, or a new custom verb is checked, the wildcard entry automatically grants access, which may be undesirable. The principle of least privilege should be employed, using specific resources and verbs to ensure only the permissions required for the workload to function correctly are applied.

Refs:

This PR could be seen as a continuation of a previous work for hardening the RBAC: kedacore/charts#625
It replaces * with explicit verbs and resources, according to KEDA needs.

Checklist

Relates to kedacore/charts#682

@legal90 legal90 requested a review from a team as a code owner September 3, 2024 18:22
config/rbac/role.yaml Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

JorTurFer commented Sep 11, 2024

/run-e2e
Update: You can check the progress here

@legal90 legal90 force-pushed the rbac-no-wildcards branch 2 times, most recently from 0d56c96 to da55007 Compare September 13, 2024 08:40
@zroubalik
Copy link
Member

zroubalik commented Sep 18, 2024

/run-e2e
Update: You can check the progress here

@zroubalik
Copy link
Member

zroubalik commented Sep 24, 2024

/run-e2e
Update: You can check the progress here

@JorTurFer
Copy link
Member

JorTurFer commented Oct 16, 2024

/run-e2e
Update: You can check the progress here

@JorTurFer
Copy link
Member

JorTurFer commented Oct 16, 2024

/run-e2e internal
Update: You can check the progress here

Signed-off-by: Mikhail Zholobov <legal90@gmail.com>
Signed-off-by: Mikhail Zholobov <legal90@gmail.com>
Signed-off-by: Mikhail Zholobov <legal90@gmail.com>
@JorTurFer
Copy link
Member

JorTurFer commented Oct 30, 2024

/run-e2e internal
Update: You can check the progress here

@JorTurFer
Copy link
Member

JorTurFer commented Oct 31, 2024

/run-e2e rabbit
Update: You can check the progress here

// +kubebuilder:rbac:groups="*",resources="*/scale",verbs=get;list;watch;update;patch
// +kubebuilder:rbac:groups="apps",resources=deployments/scale;statefulsets/scale,verbs=get;list;watch;update;patch
Copy link
Member

Choose a reason for hiding this comment

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

We have to revert this change because KEDA must allow scaling any resource as default using raw manifests

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the review, @JorTurFer !
I rolled back this line and re-generated role.yaml (by make manifests).
Could you please check again?

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 good, only 1 comment inline
PTAL @wozniakjan

@JorTurFer
Copy link
Member

JorTurFer commented Oct 31, 2024

/run-e2e subresource_scale_test
Update: You can check the progress here

According to the PR review comment.
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.

lgtm, assuming e2e tests pass

@@ -18,7 +18,8 @@ rules:
resources:
- events
verbs:
- '*'
- create
- patch
Copy link
Member

Choose a reason for hiding this comment

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

I wasn't sure about this being sufficient but it should be fine. For any other reviewers, here is the docs
https://book.kubebuilder.io/reference/raising-events#granting-the-required-permissions

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.

4 participants