Repository navigation
fix(computeplane): do not grant tenants the cluster admin kubeconfig - #4722
Conversation
The computeplane module deploys the kubernetes chart into the tenant namespace, and that chart's dashboard Role lets the tenant's "use" subjects read <release>-admin-kubeconfig. Withholding the Secret from the tenant secrets view does not help when RBAC grants it directly, so any tenant user at "use" level or above could fetch the cluster-admin credential of a cluster the module promises to keep operator-owned. The kubernetes chart now drops that rule when the internal value withholdAdminKubeconfig is set, and the module sets it in the cluster HelmRelease's inline values. The tenant cannot reach those values: the module reads only nodeGroups from them and splices an allowlisted subset into the pool releases, and Flux applies spec.values over valuesFrom. A regular kubernetes app keeps the rule, so its dashboard can still offer the kubeconfig. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe dashboard Role conditionally includes a rule to read the release’s ChangesDashboard admin kubeconfig access
Priority: ⬆️ High Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Computeplane withholds dashboard access to its admin kubeconfig, while regular Kubernetes apps retain their existing access. No merge-blocking issue was identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change narrows credential access, and the inspected configuration prevents tenant inputs from disabling that restriction. No introduced or worsened security issue was established. Successful rollout and recovery from any earlier credential disclosure remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM.
This closes the tenant's read path to the ComputePlane cluster-admin kubeconfig, and the removal is scoped the way it should be.
What I checked:
- The
withholdAdminKubeconfigguard only drops thesecrets/<release>-admin-kubeconfigrule from the dashboard Role. With the value absent (the default) the rule stays, so a normalkubernetesapp doesn't change at all: rendering the merge base and HEAD for a plain cluster gives byte-identical output. - The invariant itself, not just the guard. A tenant "use" subject has no other way to that Secret. None of the tenant tiers (
cozy:tenant,:view,:use,:admin,:super-admin) grantsecretsat all, by any name, so the per-app dashboard Role was the only RBAC grant and it's now gated. The aggregated-API path is closed independently bycomputeplane-rdwithsecrets.include: [], and a tenant can't squat the name through TenantSecret since there's no write grant ontenantsecrets. - The pin can't be reopened from tenant input.
withholdAdminKubeconfig: trueis a literal in the wrapper'sspec.values, which Flux merges overvaluesFrom(cozystack-values), so nothing injected there wins. - The tests actually hold the behavior. Inverting the guard reddens both assertions in
dashboard_resourcemap_test.yaml(default-on and withheld), and the computeplane test pins that the wrapper passes the flag through. Both touched charts pass against current main.
Keeping the toggle out of values.yaml and the schema, with the reasoning written inline, is the right call.
## What this PR does This adds an e2e suite for the ComputePlane tenant module, which had only render tests so far. The suite lives in `hack/e2e-chainsaw/computeplane/`. It turns the module on in its own tenant, `tenant-cplane`, so the shared `tenant-test` is never toggled. That tenant is a child of `tenant-root`, not of `tenant-test`, because a tenant two levels down cannot reach the etcd it inherits from root (#4726). What it checks on a real cluster: - the module release installs, and the cluster release it renders keeps the pinned addons and the `internal.cozystack.io/computeplane` label; - the pool release gets only the keys from the module's per-pool allowlist (the patch sends a `nameOverride` through the ComputePlane API and checks it is dropped); - Kamaji writes `computeplane-cluster-admin-kubeconfig`, and the tenant can't get at it: the Secret is not labelled `tenantresource=true`, `tenantsecrets` does not list it to the tenant's super-admin group, and `kubectl auth can-i get` answers `no` for that group (the read was possible until #4722); - cert-manager and ingress-nginx come up inside the cluster, and the Proxied ingress Service gets a ready endpoint; - the module and cluster releases have no remediation cycle in `status.history`; - on teardown, the in-cluster addon releases are gone before the KamajiControlPlane starts deleting; - turning the module off removes all of it: releases, in-cluster addons, control plane, Secret, worker VMs and disks, and the LINSTOR pools go back to their pre-suite level. The issue names the Secret `computeplane-admin-kubeconfig`. The module writes `computeplane-cluster-admin-kubeconfig`, so the suite checks that name. The issue also lists fluxcd as a pinned addon. That is out of date. The module runs no in-cluster Flux on purpose (`packages/extra/computeplane/templates/cluster.yaml:108-110`), so the suite checks cert-manager and ingress-nginx only. One part of the issue is not covered here. The `placement: ComputePlane` routing in cozystack-api doesn't exist yet, so there is no catalog app to run on the cluster. The issue ties that to "together with, or right after" this test, so I left it for the routing work, tracked in #4715. Things the review found outside this diff, each filed on its own: the module release's 10m install timeout against a slow cluster bringup (#4714), the module docs not matching the per-pool allowlist (#4716), and the LINSTOR helpers in `_lib` running `kubectl exec` without a timeout (#4111). The suite brings its own pool: `md0`, one worker, same sizing as the kubernetes suites. The default `md0` that the module renders for empty `nodeGroups` asks for the `replicated` StorageClass. The container lane doesn't have it, so on this lane the default pool stays covered by the render tests only. ComputePlane workers can't use the in-sandbox Talos image cache. The pool allowlist drops `talos`, so the worker pulls its disk from the public Image Factory (#4700). If the worker join or an addon release times out, the catch block prints the worker image source and points at that issue, so a red run caused by the factory is easy to tell apart. The suite itself needs no entry in `hack/select-e2e.sh`, because `computeplane-application` maps to it by name. That source depends on `kubernetes-application` and re-sources the kubernetes charts. So PRs that touch the kubernetes charts or ingress-nginx now select `computeplane` too, which I estimate adds 12-15 minutes to a green run (kubernetes-previous takes about 8 minutes with two workers, plus teardown). A red run can take much longer, because the waits sit above the release install timeouts. The suite header has the numbers. It is the third suite that brings up a tenant cluster, and the 215m E2E job cap is sized for the red path of the kubernetes pair only. If worker join breaks everywhere, all three go red and can run past the cap, which cancels the job and loses its log. I left that as an accepted risk. `hack/select-e2e_test.bats` now expects `computeplane` in the ingress-nginx selection. The computeplane source also owns `apps/kubernetes-nodes`. Before this PR a pool chart edit reached no suite and ran everything. With the new suite alone it would have selected only `computeplane`, which never creates `KubernetesNodes` natively. So `kubernetes-nodes-application` now maps to `kubernetes-latest kubernetes-previous computeplane` in `hack/select-e2e.sh`, and two new tests pin that for a pool chart edit and a `kubernetes-nodes-rd` edit. The suite is not in the leak-guard allowlist. Its teardown already fails when a TenantControlPlane sticks on its finalizer (#4489), because the admin kubeconfig Secret it owns stays behind, so an exemption would only hide other leaks. The README suite list now names the suite. JFYI, not a defect claim: the Proxied ingress Service from the kubernetes chart has `internal.cozystack.io/tenantresource: "true"`. So the tenant sees it, even though `computeplane-rd` has `services.include: []`. The module doesn't promise to hide it. How I checked it. `chainsaw lint` passes at v0.2.15, the version the sandbox pins. I also ran the suite against an envtest apiserver, with fake controllers applying the real `helm template` output of the tenant and computeplane charts. The intact run is green. Each of these breakages turns it red at the step meant to catch it: label removed, addon `exposeMethod` changed, a key outside the pool allowlist, the allowlist `pick` replaced by the whole group, stale Ready from an older generation, wrong Secret name, Secret labelled `tenantresource`, Secret listed in `tenantsecrets`, a direct RBAC grant on the Secret, ingress-nginx addon not Ready, no ready ingress endpoint, an `uninstalled` history entry, the control plane deleted before the addon releases, an addon release or worker VM left after teardown. Closes #3369 Closes #4747 ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note test(e2e): add an end-to-end suite for the ComputePlane tenant module ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added end-to-end coverage for enabling and disabling the ComputePlane tenant module, including generated releases, pool value filtering, admin credential access, addon and cluster readiness, and resource cleanup. * Kubernetes nodes changes now select the ComputePlane suite alongside the latest and previous Kubernetes suites. * **Documentation** * Added the ComputePlane suite to the documented end-to-end test suite list. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
A tenant user could read the admin kubeconfig of their own ComputePlane cluster, which the module promises never to hand out. The module deploys the
kubernetesapp into the tenant namespace ascomputeplane-cluster. The secrets allowlist kept the Secret out oftenantsecrets, but the chart's dashboard Role still gave the tenant'susesubjects get/list/watch oncomputeplane-cluster-admin-kubeconfig. So a plainkubectl get secretworked.The fix drops that one rule for the ComputePlane cluster only. The kubernetes chart now skips it when the internal value
withholdAdminKubeconfigis set, and the computeplane module sets that value inline in the cluster HelmRelease. The tenant can't reach those values, because the module reads onlynodeGroupsand passes an allowlisted subset of it to the pool releases. The value is kept out ofvalues.yamland the schema, same asclusterReleaseNamein kubernetes-nodes.A regular Kubernetes app keeps the rule, so the dashboard still offers its kubeconfig. Only the v1.7.0 alphas ship the module. After this lands, the
kubectl auth can-icheck can go back into the e2e suite in #4713.Screenshots
No UI changes.
Downstream repositories
Release note
Summary by CodeRabbit