Remove wildcard permissions from knative-serving-core ClusterRole#16601
Remove wildcard permissions from knative-serving-core ClusterRole#16601wiz-abhi wants to merge 1 commit into
Conversation
|
|
|
Welcome @wiz-abhi! It looks like this is your first PR to knative/serving 🎉 |
|
Hi @wiz-abhi. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wiz-abhi The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
7c9763a to
b320fb2
Compare
| - apiGroups: ["*"] | ||
| resources: ["*/scale"] | ||
| - apiGroups: ["apps"] | ||
| resources: ["deployments/scale"] |
There was a problem hiding this comment.
We recently allowed scaling other resources besides deployments that expose the scale API, see #16540
Considering this and that the proposed change above only changes permissions for knative's own CRDs I personally don't see a benefit in this change and don't consider this a security improvement.
Will need to wait for feedback from a maintainer though.
There was a problem hiding this comment.
Thanks for the review — good catch on #16540. You're absolutely right that changing apiGroups: [""] / resources: ["/scale"] to deployments/scale would regress the multi-type workload scaling support added there. I'll revert that part.
For the Knative-owned API groups, my intent was least-privilege hardening (avoiding * to prevent automatic access to future CRDs/subresources in those groups), but I agree the immediate practical security gain may be limited.
I'd like maintainer guidance on policy here:
- Keep wildcard for Knative API groups (lower maintenance), or
- Use explicit resource lists for stricter RBAC posture (with ongoing maintenance cost).
I'm happy to align either way and update/close this PR accordingly.
b320fb2 to
7e80dfa
Compare
Replace '*' wildcards in resources for Knative-owned API groups with explicit resource lists to follow the principle of least privilege. Changes: - Expand resources for serving.knative.dev, autoscaling.internal.knative.dev, and networking.internal.knative.dev apiGroups to list all CRDs explicitly including their /status and /finalizers subresources. - Preserve apiGroups: ['*'] / resources: ['*/scale'] to maintain multi-type workload scaling support added in knative#16540. Fixes knative#16599
7e80dfa to
0642739
Compare
Fixes #16599
Proposed Changes
*wildcards inknative-serving-coreClusterRole with explicit resource lists for Knative-owned API groupsserving.knative.dev,autoscaling.internal.knative.dev, andnetworking.internal.knative.devapiGroups to include all CRDs and their/statusand/finalizerssubresourcesapiGroups: ["*"] / resources: ["*/scale"]rule to maintain multi-type workload scaling support added in use the /scale subresource to when updating replica count #16540Release Note