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 🎉 |
knative-prow
Bot
added
size/XS
|
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 |
wiz-abhi
force-pushed
the
fix-clusterrole-wildcards
branch
from
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.
wiz-abhi
force-pushed
the
fix-clusterrole-wildcards
branch
from
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
wiz-abhi
force-pushed
the
fix-clusterrole-wildcards
branch
from
7e80dfa to
0642739
Compare
|
/ok-to-test |
knative-prow
Bot
added
ok-to-test
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16601 +/- ##
==========================================
- Coverage 80.25% 80.24% -0.02%
==========================================
Files 217 217
Lines 13568 13568
==========================================
- Hits 10889 10887 -2
- Misses 2315 2317 +2
Partials 364 364 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| verbs: ["get", "list", "create", "update", "delete", "patch", "watch"] | ||
| - apiGroups: ["serving.knative.dev", "autoscaling.internal.knative.dev", "networking.internal.knative.dev"] | ||
| resources: ["*", "*/status", "*/finalizers"] | ||
| resources: ["configurations", "configurations/status", "configurations/finalizers", "revisions", "revisions/status", "revisions/finalizers", "routes", "routes/status", "routes/finalizers", "services", "services/status", "services/finalizers", "domainmappings", "domainmappings/status", "domainmappings/finalizers", "metrics", "metrics/status", "metrics/finalizers", "podautoscalers", "podautoscalers/status", "podautoscalers/finalizers", "certificates", "certificates/status", "certificates/finalizers", "ingresses", "ingresses/status", "ingresses/finalizers", "serverlessservices", "serverlessservices/status", "serverlessservices/finalizers", "clusterdomainclaims", "clusterdomainclaims/status", "clusterdomainclaims/finalizers"] |
There was a problem hiding this comment.
If the listing is not driven by generator, but rather maintained by hand or LLM tool. We should keep wildcard for our own custom resources.
There was a problem hiding this comment.
3 participants
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