Repository navigation
fix(dashboard): let dashboard users read backup CRD schemas - #4236
Conversation
Andrey Kolkov (androndo)
added
the
kind/backport
github-actions
Bot
added
area/uncategorized
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe dashboard read-only ClusterRole now permits ChangesBackup CRD access
Priority: ➖ Normal Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change provides the intended narrowly scoped schema access without introducing an identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Out of Scope Changes checkExplanation FAIL. The new permission is in a ClusterRole bound to Resolution Restrict the permission to the required tenant-user authorization path, such as tenant groups or a tenant-scoped aggregate role. Keep the five
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM
This rule never reaches the authorizer. A get on a CRD is a cluster-scoped request; the 403 quoted in #4053 spells that out, "at the cluster scope". cozy:tenant:view is bound only by a RoleBinding, in packages/apps/tenant/templates/tenant.yaml. A RoleBinding pointing at a ClusterRole grants that role inside the binding's own namespace, and cluster-scoped requests are checked against ClusterRoleBindings. I grepped packages/ for cozy:tenant:view, cozy:tenant:use, cozy:tenant:admin and cozy:tenant:super-admin: four RoleBindings in that one file, and no ClusterRoleBinding on any tenant persona anywhere. The label and the aggregation chain are right, and a tenant user still gets the same 403.
A carrier that would work already exists. cozystack-dashboard-readonly in packages/system/dashboard/templates/rbac.yaml is bound to system:authenticated by a ClusterRoleBinding in the same file, and it already holds the cluster-scoped reads the dashboard depends on: storageclasses, applicationdefinitions, backupclasses. The five CRD names there would land in a binding that can authorize a cluster-scoped get. That reaches every authenticated user rather than tenant members only, same as the three rules already in that role.
The rule itself is fine otherwise: apiextensions.k8s.io, customresourcedefinitions, five resourceNames matching the five files under definitions/, verbs stopping at get, nothing wildcarded.
No test with it. A rendering test over this template would pass either way, because the render is correct and the binding is what is wrong. The check that catches it is kubectl auth can-i get customresourcedefinitions.apiextensions.k8s.io/plans.backups.cozystack.io --as-group=<tenant>-super-admin, and that belongs in e2e.
The red E2E is not from this diff. opensearch is the cluster-wide failure whose fix landed after this branch was cut. The four *-backup-roundtrip suites fail on the BackupJob reaching Failed during the S3 upload, which runs under the controller's own ServiceAccount, and tenant view permissions are not on that path.
2591917 to
5f8fd21
Compare
Andrey Kolkov (androndo)
changed the title
|
Aleksei Sviridkin (@lexfrei) you're right — thanks for catching it. A RoleBinding pointing at the aggregated tenant ClusterRole can't authorize a cluster-scoped Moved the grant to Agreed on the red E2E being unrelated (the |
Andrey Kolkov (androndo)
added
area/dashboard
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM
cozystack-dashboard-readonly is the right carrier. Its ClusterRoleBinding names system:authenticated, so a cluster-scoped get on a CRD is now checked against a binding that can authorize it, which is what round one was missing. The backup-controller copy is gone: customresourcedefinitions appears nowhere else under that package.
Five resourceNames, five files in definitions/, names match one for one. get alone is still right, since useCRDSchema does a by-name get and a rule carrying resourceNames can't authorize a list anyway.
No cluster on my side, so that authorization claim is read off the rendered objects rather than a real kubectl auth can-i. The console sends the user's own token (client.ts sets Authorization: Bearer), and every logged-in user carries system:authenticated.
The scope warning from the bot is half right. The wider audience is real. Its suggested fix, granting through a tenant-view aggregation path, is the same shape that never reached the authorizer last round, so don't take it. Widening is the standing tradeoff for this role: storageclasses went in for the same reason in v1.1.4, and applicationdefinitions and backupclasses sit there on the same terms.
Worth correcting the PR body before this merges, since the body lands on main. The verification line names --as-group=<tenant>-super-admin, which reads like a tenant-scoped grant. It isn't: the same check passes for any authenticated user carrying no tenant group at all.
Two follow-ups, neither of them this PR's to fix. The migration forms have the same bug: MigrationSourceCreatePage, MigrationSourceEditPage and MigrationTaskCreatePage call the same hook on vmimportsources.forklift.cozystack.io and vmimporttasks.forklift.cozystack.io, and cozy:migration:view grants forklift.cozystack.io resources only, so those forms still 403 on the schema lookup. Second, a test would discriminate now that the rule sits in this chart: tests/cluster_usage_rbac_test.yaml already pins a cluster-scoped grant together with its binding's subject in this package, and the same assert here would have gone red on the previous head.
0198c14 to
f3207cf
Compare
The dashboard backup build/edit forms build each form from the CRD's OpenAPI schema, which useCRDSchema fetches directly from apiextensions.k8s.io with the logged-in user's token — a cluster-scoped get on the CRD by name. No role a tenant user carries granted customresourcedefinitions, so every tenant user hit a 403 on the schema lookup and the form never rendered, while kubectl kept working through the existing backup CR grants. The tenant personas are bound only by RoleBindings, which cannot authorize a cluster-scoped request, so the grant belongs on cozystack-dashboard-readonly: it is bound to system:authenticated by a ClusterRoleBinding and already carries the dashboard's other cluster-scoped reads. Add a get on the five backups.cozystack.io CRDs, scoped by resourceNames to those schemas only. Fixes: #4053 Assisted-By: LLM Signed-off-by: Andrey Kolkov <[email protected]>
f3207cf to
3a8a06b
Compare
|
Successfully created backport PR for |
2 participants
What this PR does
The dashboard's backup build/edit forms (
BackupPlanCreatePage,BackupJobCreatePage,BackupRestoreJobCreatePage,BackupCreatePage,BackupClassCreatePage,BackupClassEditPage,BackupResourceEditPage) build their form from the CRD's OpenAPI schema, whichuseCRDSchemafetches directly fromapiextensions.k8s.iowith the logged-in user's token — a cluster-scopedgeton the CRD by name.No role a tenant user carries granted
customresourcedefinitions, so every tenant user — tenant super-admin included — hit a 403 on the schema lookup and the form never rendered, even though they already hold write access to the backup CRs throughcozy:backups:admin.kubectlkept working because that only touches the CRs, not their CRDs.The tenant personas (
cozy:tenant:{view,use,admin,super-admin}) are bound only by RoleBindings, which authorize namespaced requests but never a cluster-scoped one, so the grant belongs oncozystack-dashboard-readonlyinpackages/system/dashboard/templates/rbac.yaml: it is bound tosystem:authenticatedby a ClusterRoleBinding and already carries the dashboard's other cluster-scoped reads (storageclasses,applicationdefinitions,backupclasses). This adds ageton the fivebackups.cozystack.ioCRDs, scoped byresourceNamesto those schemas only — exactly the by-namegetuseCRDSchemaperforms, no wildcard and no collection listing.Verification:
helm templaterenderscozystack-dashboard-readonlywith the new rule cleanly; the real contract iskubectl auth can-i get customresourcedefinitions.apiextensions.k8s.io/plans.backups.cozystack.io --as-group=<tenant>-super-admin, which belongs in e2e.Fixes: #4053
Downstream repositories
Release note
Summary by CodeRabbit