Repository navigation
Let Kubernetes RBAC, not the Cloud role, decide user-run operations - #1899
nadaverell merged 16 commits into
Conversation
Radar Cloud orgs whose IdP groups decide cluster access turn the chart's hub-role bindings off. Two things broke in that setup: - The hub's alerts worker and timeline puller only had cluster access through radar:owner. The chart now always binds radar:system, which they assert, to a read-only set: view, the cluster-read and integration-read add-ons, and Secret read (Helm release alerts list releases as the caller, and Helm stores them as Secrets). It is controlled by cloud.systemRbac, independent of cloud.defaultRbac. - Helm reads and writes, and in-cluster probe pods, were refused on the Cloud role before Kubernetes was asked. They run as the impersonated user, so a viewer whose IdP group grants access was wrongly denied. Those role checks are removed from the REST handlers, the MCP tools and the UI; the role still gates Radar's own configuration. The restricted-access panel now fills its RBAC snippet with the user's radar:idp:* groups, binding only the first.
Every read is filtered to the user's namespaces, so a user bound to none saw empty lists in every view with nothing saying why. /api/auth/me now reports noNamespaceAccess once connected, and the shell shows a banner above every view: who is signed in, their bindable groups in a collapsed list, and a ClusterRoleBinding with placeholders that grants nothing until an admin fills it in. The banner is based on the user's own access, not on how the cluster is meant to be set up, so it holds in OSS proxy and OIDC modes and on Cloud clusters whether they use hub roles or IdP groups. It re-checks every minute while shown, so it clears once a binding lands.
Knowing why every view is empty is the part the user needs; binding a group is the cluster admin's job, done in their own tooling. The banner keeps the explanation and the collapsed list of the user's groups.
No chart ever bound cloud:system and every running hub sends radar:system with it, so the legacy subject matched nothing. The hub stops sending it too (skyhook-dev/radar-hub#272). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The filter picked which group was safe to suggest binding, which only mattered while the banner showed an RBAC snippet. The list is now a diagnostic of who Kubernetes sees the user as, so hiding tier and org groups made it wrong.
The restricted-access snippet filled its subject from the caller's radar:idp:* groups. That only applied in Radar Cloud, bound whichever IdP group came first (possibly an org-wide one), and hard-coded the hub's group vocabulary in the frontend. The snippet keeps the exact group, resource and verbs for the denied kind; the subject goes back to a placeholder the admin fills in.
…nger needs a write-everything SA cloud.systemRbac binds radar:system (the hub's alerts worker, timeline puller and AI diagnose agent) to view, the cluster-read and integration-read add-ons, and Secret read, which Helm release alerts and Secret changes in the hub timeline need. One flag: false removes the whole grant. An absent value now means off, like defaultRbac.create, so a --reuse-values upgrade from a release that predates the key never gains cluster-wide Secret read without someone choosing it. radar:system has no legacy cloud:* twin in the integration-read bindings. rbac.helm's */* create/update/patch/delete rule for the ServiceAccount now renders only in no-auth, non-Cloud installs. Under auth or Cloud every Helm action runs as the signed-in user, and no ServiceAccount write path depends on the grant; rbac.helm there only adds the per-tier Helm add-on roles. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017J25KHG5W4DzS32bU1J8ou
With a user attached, every Helm action already runs as that user, yet the write gate asked whether Radar's ServiceAccount could create Secrets, i.e. whether rbac.helm=true had granted it write on everything. An IdP group bound to edit in one namespace could not use Helm there without that grant. requireHelmWrite now checks the caller's secrets/create in the release's namespace (Helm stores each release as a Secret there), before any chart is fetched, so a caller who could never install cannot make Radar reach out to an arbitrary repository URL. Chart-cache and OCI-source changes are pod-local and need no cluster permission. Without auth the ServiceAccount check stays. /api/capabilities?namespace= reports helmWrite for that namespace, and the release drawer, values editor and install wizard ask about the release's own namespace. With auth on, a denial names the user's RBAC rather than rbac.helm. Also: the streaming rollback runs as the user like the plain one, and a forbidden read of release content returns 403 instead of 500. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017J25KHG5W4DzS32bU1J8ou
The flag behind the no-access banner ran namespace discovery when nothing was cached: one or two SubjectAccessReviews per namespace on the request app startup waits for, racing the same discovery in the first resource load. It now reads the cache and leaves the flag out on a miss; the banner asks again once content has loaded, when the cache is warm. Only a cached (successful) discovery is reported, never the fail-closed result of an error. The banner title says what is true: the user can't read any namespace, while cluster-scoped kinds may still be readable. It renders once content is ready. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017J25KHG5W4DzS32bU1J8ou
AI diagnose keeps reading as radar:viewer, as on main, so it never gets the Secret read radar:system carries. How diagnose reads clusters whose hub-role bindings are off is RAD-657. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
/auth/me answers from the permission cache only, so the banner's one refetch could beat the first namespace discovery and get no answer. Polling ran only while the answer was "no access", so the banner then stayed hidden and the user saw an unexplained empty cluster. /auth/me now sends noNamespaceAccess false once access is known, and leaves it out only while it isn't. The banner re-asks every 5s while it's unknown, at most 12 times, so it still stops against a server that predates the field. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017J25KHG5W4DzS32bU1J8ou
The banner lists the user's groups, IdP group IDs included, which name the customer's directory structure. Radar Cloud embeds this view with replay on, and its text mask only catches credential-shaped values, so the list is ph-no-capture. It does nothing without PostHog. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017J25KHG5W4DzS32bU1J8ou
This reverts commit 6eb752a. The unknown state and its retries only covered the first visit, where /auth/me can answer before namespace discovery fills the cache. A reload shows the banner then (the cache is warm), and people reload an empty screen anyway, so the extra state isn't worth keeping. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017J25KHG5W4DzS32bU1J8ou
PR Summary by QodoLet Kubernetes RBAC authorize user-run Helm and probe operations
AI Description
Diagram
High-Level Assessment
Files changed (38)
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c0f51b9. Configure here.
Code Review by Qodo
1. Helm comments retain upgrade history
|
| value means OFF: a `--reuse-values` upgrade from a release that predates the | ||
| key renders with the previous release's tree and never gains Secret read |
There was a problem hiding this comment.
1. Helm comments retain upgrade history 📘 Rule violation ⚙ Maintainability
The new radar.cloudSystemRbac template comment describes a --reuse-values upgrade from a release that predates the setting. The same release-history explanation appears in the values file and chart test, putting migration history in code comments rather than only in release documentation.
Agent Prompt
## Issue description
Added Helm comments describe releases that predate `systemRbac`, contrary to the code-comment rule.
## Fix Focus Areas
- deploy/helm/radar/templates/_helpers.tpl[75-82]
- deploy/helm/radar/values.yaml[534-535]
- deploy/helm/radar/tests/cloud_rbac_system_test.yaml[109-111]
## Recommended Fix
Keep the security rationale and absent-value behavior in code comments, but move release-specific upgrade history to release documentation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Keeping it. The comment explains why an absent value means off, which is the logic, not history.
Preview reads the release as the caller and can fetch a chart version from a repository. It lost its Cloud-role gate without gaining the write check, so a caller who could read a release but not change it could still make Radar fetch a repository through it. It now runs requireHelmWrite first, as install does.
Group names are neutral data fragments, which is the structural tone. The chips keep their look and now follow any retune of that tone.
The install wizard waited on every keystroke's /capabilities call; it now checks the namespace after typing pauses. useHelmManifest lost its unused enabled param, the compare and drawer views lose the indentation the removed RoleGatedPanel left behind, and comments that still described the role gate for Helm are corrected.
…, no-access banner (#1971) Follow-up to #1899: closes the gaps from its review. RAD-578. ## Must-fix - **Registered OCI chart sources are owner-only in Radar Cloud.** Upgrade discovery picks the highest version across all registered prefixes, so any Cloud user, even one with no cluster RBAC, could add a prefix that feeds someone else's upgrade. Add and remove now take the owner gate, like the rest of Radar's own config, and the Track chart source dialog hides the controls from non-owners. - **Under Cloud, a request with no role group is refused by owner-gated endpoints.** "No role" skips the gate so OSS keeps its own config, but under Cloud it meant the Hub's `radar:system` identity passed owner-only settings. OSS behaviour is unchanged. - **Per-user capability caches are keyed on username and groups.** They now decide Helm writes. Keyed by username alone, a user who lost an IdP group kept the old answer for up to a minute. ## Smaller fixes - Release reads map only a Kubernetes denial to 403. A chart repository's own 401/403 is no longer reported as the caller's RBAC. - When Kubernetes refuses the Helm release listing for issues, it is logged once per identity, so missing Helm alerts leave a trace. - The no-access banner asks `/api/auth/me?check=namespaces`, which runs namespace discovery, so it can't vanish when the permission cache expires. It keeps its last definite answer through a check that couldn't decide, and polls until access is confirmed. - Helm actions stay disabled until the per-namespace permission check succeeds, including when it fails. ## Tests - New: a user losing a group (end to end through `requireHelmWrite`); the Cloud no-role deny; the owner gate on chart sources; repository 401/403 vs RBAC denial; `Impersonate-Group` on streaming rollback; the banner's discovery path failing closed. - `go test` (both modules), `tsc`, and web vitest pass. ## Verified on EKS (radar-test-nonprod), proxy auth with simulated users - Banner still shown 150 s in, past the permission cache expiry; it cleared by itself 64 s after a binding was added. - A Hub `viewer` whose IdP group grants `edit` in one namespace: Helm rollback and uninstall work there and are refused elsewhere. Dropping the group takes effect on the next request. - A user who can list pods but not Secrets: the Helm-denied log line appears once across repeated calls. - Not live-testable locally: the Cloud no-role deny and the owner gate under real Cloud mode, which only accepts requests over the Hub tunnel. Unit tests cover both. Docs: skyhook-dev/radar-docs#129 (owner-only chart sources). <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes authorization for Cloud config (OCI sources, no-tier denial) and Helm write caching keyed on groups; incorrect behavior would block legitimate admins or briefly allow stale permissions, but scope is bounded and covered by new tests. > > **Overview** > Follow-up to #1899 that tightens **Radar Cloud config auth**, **Helm/RBAC correctness**, and the **no-namespace-access** UX. > > **Cloud config:** Registered OCI chart sources are now **owner-only** in Cloud (`requireHelmSourceConfigWrite`), with matching UI in the track-chart-source dialog. In Cloud mode, callers **without** a `radar:owner|member|viewer` tier (e.g. Hub `radar:system`) are **denied** on tier-gated Radar config instead of bypassing the gate like OSS. > > **RBAC caching & Helm:** Per-user capability caches (including namespace Helm-write SARs) are keyed by **username + groups** via `IdentityCacheKey`, so revoking an IdP group stops reusing stale “can write” for up to the TTL. Release read errors map to **403 only for Kubernetes RBAC** (`isReleaseReadForbidden`), not chart-repo 401/403. Helm issues omit logging is **once per identity** when Secret list is forbidden. > > **No-access banner:** `/api/auth/me?check=namespaces` runs namespace discovery; the banner polls that path, keeps the last **boolean** `noNamespaceAccess`, and Helm actions stay disabled while per-namespace write capability is **unknown** (`helmWriteUnknown`). > > Docs note owner-only chart sources and no-role Cloud denial on config. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 7c39b11. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
… for background Diagnose (#1949) Linear: RAD-657 ## Why Radar Cloud's AI Diagnose is blind on clusters whose role bindings are off (`cloud.defaultRbac.create=false`, IdP groups decide access), because it reads as `radar:viewer`, which is unbound there. After this release: - **Manual** Diagnose runs read as the person who started them (skyhook-dev/radar-hub#292), so the AI never exceeds that person's access. A person may be able to read Secrets, so Helm diffs handed to the model must not carry Secret values. - **Background** (alert-triggered) runs read as one view-only identity, the `radar:ai` group (skyhook-dev/radar-hub#288), which this chart binds. MCP clients are not affected: they keep reading with the user's own identity. ## What changes **1. Secret redaction in `get_helm_release` (every MCP caller)** - `include=diff` (the manifest diff) and `include=notes_diff` were returned unredacted; only `values_diff` went through `RedactSecrets`. - Each `kind: Secret` document now has its `data` / `stringData` values replaced by `[REDACTED]` before diffing. The keys stay, so added and removed keys still show. Secrets inside `kind: List` items are covered, the `last-applied-configuration` annotation (a full copy of the object) is dropped, and a document that doesn't parse is withheld whole. Other documents are left byte-for-byte untouched. - `RedactSecrets` then runs over both diffs as a second layer. - `include=values` and `include=values_diff` now use key-aware redaction built for Helm values (`aicontext.RedactHelmValues`). Chart authors name credentials freely (`dbPassword`, `auth.postgresPassword`), so any key ending in `password`, `token`, `apiKey`, `secretKey`, `clientSecret`, `credentials` and similar is masked, numbers included. Secret *references* (`existingSecret`, `*SecretName`, `*Ref`) keep their values. The values diff masks both revisions before diffing. CRD specs keep the existing exact-key matching. - The REST/UI Helm diff is unchanged: it is the user's own view. **2. `radar:ai` chart binding** - New `templates/cloud-rbac-ai.yaml`: binds `radar:ai` to `view`. No Secret role, no writes. - `radar:ai` is added to the cluster-read and integration-read add-ons. - New value `cloud.aiRbac`, default `true`, independent of `cloud.defaultRbac`. As with `cloud.systemRbac`, an absent value means off, so a `--reuse-values` upgrade never gains the binding silently. - Docs: the chart README and `docs/authentication.md`. The binding does nothing until Radar Cloud sends the group. Background runs send `radar:ai` with no tier group, so the AI's access is exactly this binding: `cloud.aiRbac=false` really turns it off, and a widened viewer role doesn't widen the AI. Clusters on older charts get no background Diagnose until they upgrade. Radar Cloud refuses manual runs on a Radar older than this release, rather than reading with less protection. ## Tests - Go unit tests for Helm values redaction: nested keys, numeric credentials, references kept under `credentials`, both revisions of a values diff. - Go unit tests for the manifest-diff redaction: Secret values gone, keys and non-Secret changes kept, last-applied copies, Secrets inside Lists, and unparseable documents. Plus a check that `resource_diff` masks Secret values. - `TestIntegrationReadBindings` expects the `radar:ai` bindings alongside `radar:system`, with cases for `aiRbac=false`. - New `tests/cloud_rbac_ai_test.yaml`: renders with `defaultRbac.create=false`, absent or `false` means off, bound to `view` only, add-ons present whatever the tier settings. - `scripts/test-chart.sh`: new render cases; the "no orphan role" cases now also set `cloud.aiRbac=false`. - `helm unittest`: the `service_test.yaml` failures also fail on `main` locally; CI's Helm chart job passes. ### Live test on EKS (proxy auth, simulated Diagnose identities) - **Background identity** (`radar:ai,radar:org:<org>`) with the chart's `radar:ai` binding: pods in every namespace, no Secrets. - **Manual run as a contractor** whose IdP group has `view` in one namespace: pods in that namespace only; listing all namespaces returns only that one. - **Helm release with a Secret, a token in its notes, and `dbPassword` / `apiKey` values**, read through `get_helm_release` by a user who can read Secrets: no sentinel value in `values`, `diff`, `notes_diff` or `values_diff`. The REST diff (the user's own view in the UI) still shows them, as intended. ### End to end on kind - **Role bindings off, `aiRbac=true`:** the AI identity reads pods, events and nodes; Secrets are forbidden. - **`aiRbac=false`:** it reads nothing. - **`get_helm_release include=diff,notes_diff`, called by an owner who can read Secrets:** no Secret values in the output. The #1899 binary returns the plaintext values and a `ghp_` token from the notes. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes cluster RBAC defaults and how credentials appear in MCP Helm diffs; misconfiguration could block background Diagnose or leak secrets if redaction regresses, but grants stay read-only and UI paths are unchanged. > > **Overview** > Adds **default Kubernetes RBAC for automatic Radar Cloud Diagnose** via new `cloud.aiRbac` (default on): chart-owned aggregated **view** role for `radar:ai`, plus cluster-read and integration-read add-ons—**no Secret access**. Refactors `radar:system` the same way (owned aggregated view role instead of binding directly to `view`), with Secret read unchanged for alerts/timeline. Docs and Helm tests cover independence from `cloud.defaultRbac`, `--reuse-values` absent-key behavior, and installer `escalate` requirements. > > **Hardens Helm MCP output for AI callers** while leaving REST/UI diffs as the user’s own view: manifest diffs strip Secret `data`/`stringData` (and unparseable Secret docs) before diffing; values diffs use new **`RedactHelmValues`** (suffix/key-aware credentials, preserved secret *references*); notes diffs get pattern redaction. `get_helm_release` `include=diff`, `values_diff`, and `notes_diff` route through these paths. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 16be4f7. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Nadav Erell <nadaverell@gmail.com>

Why
Part of RAD-578: in Radar Cloud, hub roles (
owner/member/viewer) should cover Radar's own features, and for orgs with an IdP, IdP groups (radar:idp:<id>) should decide cluster access. Those orgs turn the chart's hub-role bindings off (cloud.defaultRbac.create=false). Two things broke in that setup:radar:owner→admin, so alerts and the hub timeline went silent. The same thing happens today to anyone who setsdefaultRbac.owner=false.viewerwhose IdP group grantseditin a namespace couldn't roll back there.What
Chart
cloud-rbac-system.yaml: bindsradar:system(the hub's alerts worker and timeline puller) toviewplus Secretget/list/watch. The cluster-read and integration-read add-ons also bind it, and now still render whendefaultRbac.create=false. AI diagnose does not use it; it stays onradar:viewer.adminthroughradar:owner.cloud.systemRbac(defaulttrue), all or nothing. An absent value counts as off, likedefaultRbac.create, so a--reuse-valuesupgrade from an older release never gains cluster-wide Secret read without someone choosing it. Schema, chart README anddocs/authentication.mdupdated.Backend
requireCloudRolefrom all 16 Helm handlers and the MCPget_helm_releasegate.diagnose). Probe pods are created with the user's impersonated client, andreachability.Capabilityalready checks Kubernetes permissions.UI
RoleGatedPanelis removed from the Helm Manifest, Values and Compare tabs.useCanHelmAct(namespace)checks the user's own Helm write permission in the release's namespace (see below).RestrictedStatekeeps the exact group, resource and verbs for the denied kind in its RBAC snippet, and leaves the subject as a placeholder the admin fills in.OSS without a Hub is unaffected: the cloud templates need
cloud.enabled, and the role gates already did nothing without a Cloud role.Test plan
helm unittest(newcloud_rbac_system_test.yaml, updated integration-read suite) andscripts/test-chart.shgo test ./...andpkgmodule tests. The chart-renderingTestIntegrationReadBindingsnow expectsradar:system.TestHelmHandlers_NotGatedOnCloudRoleandTestTraceInClusterNotGatedOnCloudRole, both red before the changemake tsc; web and k8s-ui vitest, plus a newRestrictedState.test.tsxviewerwith admin-level RBAC sees Manifest, Values, Rollback and Compare;viewerwith onlyviewgets Kubernetes' "Access Restricted" instead of a role message;TestHandleAuthMe_NoNamespaceAccess(reported only when connected with zero namespaces) andnoClusterAccess.test.tsNo-access banner
/api/auth/menow reportsnoNamespaceAccess: truefor such a user, and only once connected: before that, namespace discovery fails closed.Release order
Release this chart before the Hub web wizard can default new installs to IdP-only access. The Hub PRs keep sending
radar:ownerfor one release so older charts keep working.Related: skyhook-dev/radar-hub#272, skyhook-dev/radar-hub-web#326, skyhook-io/helm-charts#51.
Cross-review follow-up (2026-09-29)
55fc3efa). With a user attached,requireHelmWritechecks the caller'ssecrets/createin the release's namespace, before any chart is fetched (so a caller who could never install can't make Radar fetch an arbitrary repo URL). Chart-cache and OCI-source changes need no cluster permission. No-auth installs keep the ServiceAccount check./api/capabilities?namespace=reportshelmWriteper namespace; with auth on, a denial names the user's RBAC, notrbac.helm. Also: streaming rollback runs as the user, and a forbidden release read returns 403.rbac.helmin Cloud/auth mode (acbdbad4). The*/*create/update/patch/delete rule renders only for no-auth, non-Cloud installs; every Helm action there runs as the user, and no ServiceAccount write path depends on it (audited).radar:systemis one all-or-nothing grant; absent means off (acbdbad4,c4018889)./auth/mereads the permission cache only (e51efe09), so app startup no longer runs a SubjectAccessReview per namespace. The banner title now reads "You can't read any namespace in this cluster"./auth/mecheck can arrive before namespace discovery fills the cache, so the banner may need one reload to appear. A retry loop for this (6eb752a8) was reverted (c0f51b96) as not worth the extra state.ph-no-capture(af4afc7c): group IDs name the customer's directory structure, and Radar Cloud records this view in session replay.go testforinternal/helm,internal/k8s,internal/serverpass (the oneinternal/serverfailure,TestNoEffortNotesReferencedFromSource, comes from other local worktrees under.claude/).helm unittest: 70/72, the 2 failures (service_test.yaml) also fail on HEAD. Web vitest for contexts/helm/api pass.🤖 Generated with Claude Code
Note
High Risk
Changes cluster RBAC (including optional cluster-wide Secret read for
radar:system) and broadens who can attempt Helm and probe operations based on Kubernetes permissions instead of Cloud role, which affects security boundaries for IdP-only orgs.Overview
Radar Cloud orgs that turn off default hub-role bindings (
cloud.defaultRbac.create=false) can keep alerts and the hub timeline working via a newcloud.systemRbacpath that bindsradar:systemto read-only access (view, cluster-read/integration-read add-ons, and cluster-wide Secret read). That grant is separate from tier bindings;--reuse-valuesupgrades without the key stay off until explicitly enabled.Authorization shifts from Cloud tier to Kubernetes RBAC for user-impersonated work: Helm reads/writes, previews, rollbacks (including streaming), in-cluster reachability probes, and MCP Helm/diagnose paths no longer call
requireCloudRole. Writes are gated by the caller’ssecrets/createin the release namespace (before chart fetch); release read failures surface as 403 when RBAC denies Secret access. The chart stops granting cluster-wide*/*write on Radar’s ServiceAccount when auth or cloud mode is on—only no-auth installs keep that for Helm-as-SA.The UI drops member-only Helm panels and uses namespace-scoped
useCanHelmAct, aligned with per-namespacehelmWritecapabilities./api/auth/meaddsnoNamespaceAccessand a warning banner when a connected user can read no namespaces.Reviewed by Cursor Bugbot for commit 812e48f. Bugbot is set up for automated code reviews on this repo. Configure here.