From d6db91076a9538db2bccf3a797d201a2098a6219 Mon Sep 17 00:00:00 2001 From: Arlen Vasconcelos Date: Fri, 25 Sep 2026 14:03:18 -0400 Subject: [PATCH 01/16] Let Kubernetes RBAC, not the Cloud role, decide user-run operations 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. --- deploy/helm/radar/README.md | 12 ++ deploy/helm/radar/templates/_helpers.tpl | 14 ++ .../templates/cloud-rbac-cluster-read.yaml | 27 ++- .../cloud-rbac-integration-read.yaml | 8 +- .../radar/templates/cloud-rbac-system.yaml | 64 ++++++++ .../cloud_rbac_integration_read_test.yaml | 10 ++ .../radar/tests/cloud_rbac_system_test.yaml | 155 ++++++++++++++++++ deploy/helm/radar/values.schema.json | 1 + deploy/helm/radar/values.yaml | 8 + docs/authentication.md | 2 + docs/reachability.md | 2 +- internal/helm/handlers.go | 97 +---------- internal/helm/handlers_test.go | 114 ++----------- .../k8s/integration_read_baseline_test.go | 16 +- internal/mcp/tools_diagnose.go | 8 - internal/mcp/tools_helm.go | 32 +--- internal/server/reachability_run.go | 15 -- internal/server/reachability_run_test.go | 18 +- .../components/resources/ResourcesView.tsx | 4 + .../components/ui/RestrictedState.test.tsx | 17 ++ .../src/components/ui/RestrictedState.tsx | 39 ++++- pkg/auth/cloud_role.go | 10 +- scripts/test-chart.sh | 10 +- web/src/api/client.ts | 29 +--- web/src/components/helm/HelmCompareRoute.tsx | 8 +- web/src/components/helm/HelmReleaseDrawer.tsx | 26 +-- web/src/components/helm/RoleGatedPanel.tsx | 47 ------ .../components/resources/ResourcesView.tsx | 8 +- 28 files changed, 424 insertions(+), 377 deletions(-) create mode 100644 deploy/helm/radar/templates/cloud-rbac-system.yaml create mode 100644 deploy/helm/radar/tests/cloud_rbac_system_test.yaml create mode 100644 packages/k8s-ui/src/components/ui/RestrictedState.test.tsx delete mode 100644 web/src/components/helm/RoleGatedPanel.tsx diff --git a/deploy/helm/radar/README.md b/deploy/helm/radar/README.md index ae33cfec78..24bb4f927e 100644 --- a/deploy/helm/radar/README.md +++ b/deploy/helm/radar/README.md @@ -161,6 +161,18 @@ Radar binary (including Radar Cloud self-upgrade) does not update RBAC. Missing on an older `--reuse-values` installation default to enabled; set explicit false before upgrading if the added visibility is unwanted. +### Radar Cloud background identities (`radar:system`) + +The hub's alerts worker and timeline puller call Radar as the `radar:system` +group, not as a user. `cloud.systemRbac` (default `true`) binds that group to a +read-only set: `view`, the cluster-read and integration-read add-ons above, and +`get/list/watch` on Secrets, which Helm release alerts need because Helm stores +each release as a Secret. It is independent of `cloud.defaultRbac`, so turning +the role bindings off (`cloud.defaultRbac.create=false`, for example when IdP +groups decide cluster access) leaves alerts and the hub timeline working. +Setting `cloud.systemRbac=false` stops both unless you bind `radar:system` +yourself. + ### Connecting to Argo CD (GitOps deep diff) Radar's GitOps pages show a Git-rendered desired-vs-live diff when connected to diff --git a/deploy/helm/radar/templates/_helpers.tpl b/deploy/helm/radar/templates/_helpers.tpl index 25fe5a36ba..52d8a6e6f8 100644 --- a/deploy/helm/radar/templates/_helpers.tpl +++ b/deploy/helm/radar/templates/_helpers.tpl @@ -71,3 +71,17 @@ Create the name of the service account to use {{- end -}} {{- $settings | toPrettyJson -}} {{- end -}} + +{{/* +Whether the always-on read-only binding for Radar Cloud's own background +identities (radar:system) renders. An absent value means on: `--reuse-values` +upgrades render with the previous release's tree, which predates the key, and +the hub's alerts and timeline depend on the binding existing. +*/}} +{{- define "radar.cloudSystemRbac" -}} +{{- if and .Values.cloud.enabled (ne (toString .Values.cloud.systemRbac) "false") -}} +true +{{- else -}} +false +{{- end -}} +{{- end -}} diff --git a/deploy/helm/radar/templates/cloud-rbac-cluster-read.yaml b/deploy/helm/radar/templates/cloud-rbac-cluster-read.yaml index af53e994e0..da65fd9384 100644 --- a/deploy/helm/radar/templates/cloud-rbac-cluster-read.yaml +++ b/deploy/helm/radar/templates/cloud-rbac-cluster-read.yaml @@ -47,8 +47,9 @@ from being truthy. {{- $_ := set $csrOn $tier (ne (toString (index $csr $tier)) "false") }} {{- end }} {{- $rbacDefaults := .Values.cloud.defaultRbac }} -{{- $anyTier := or (and $rbacDefaults.viewer (get $csrOn "viewer")) (and $rbacDefaults.member (get $csrOn "member")) (and $rbacDefaults.owner (get $csrOn "owner")) }} -{{- if and .Values.cloud.enabled .Values.cloud.defaultRbac.create .Values.rbac.create $anyTier -}} +{{- $anyTier := and $rbacDefaults.create (or (and $rbacDefaults.viewer (get $csrOn "viewer")) (and $rbacDefaults.member (get $csrOn "member")) (and $rbacDefaults.owner (get $csrOn "owner"))) }} +{{- $systemOn := include "radar.cloudSystemRbac" . | eq "true" }} +{{- if and .Values.cloud.enabled .Values.rbac.create (or $anyTier $systemOn) -}} apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: @@ -151,7 +152,7 @@ rules: {{- $labels := include "radar.labels" . }} {{- $rbac := .Values.cloud.defaultRbac }} {{- range $tier := list "viewer" "member" "owner" }} -{{- if and (index $rbac $tier) (get $csrOn $tier) }} +{{- if and $rbac.create (index $rbac $tier) (get $csrOn $tier) }} --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding @@ -173,4 +174,24 @@ subjects: apiGroup: rbac.authorization.k8s.io {{- end }} {{- end }} +{{- if $systemOn }} +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: {{ $fullname }}-cloud-system-cluster-read + labels: + {{- $labels | nindent 4 }} +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: {{ $fullname }}-cluster-read +subjects: + - kind: Group + name: radar:system + apiGroup: rbac.authorization.k8s.io + - kind: Group + name: cloud:system + apiGroup: rbac.authorization.k8s.io +{{- end }} {{- end -}} diff --git a/deploy/helm/radar/templates/cloud-rbac-integration-read.yaml b/deploy/helm/radar/templates/cloud-rbac-integration-read.yaml index 6c5648c47a..f53fb94fe1 100644 --- a/deploy/helm/radar/templates/cloud-rbac-integration-read.yaml +++ b/deploy/helm/radar/templates/cloud-rbac-integration-read.yaml @@ -1,17 +1,21 @@ {{- $rbac := .Values.cloud.defaultRbac -}} {{- $integration := $rbac.integrationRead | default dict -}} {{- $cluster := $rbac.clusterScopedRead | default dict -}} -{{- if and .Values.cloud.enabled .Values.rbac.create $rbac.create -}} +{{- $systemOn := include "radar.cloudSystemRbac" . | eq "true" -}} +{{- if and .Values.cloud.enabled .Values.rbac.create (or $rbac.create $systemOn) -}} {{- $root := . -}} {{- $baseline := .Files.Get "files/integration-read-baseline.yaml" | fromYaml -}} {{- range $scope := list "Namespaced" "Cluster" -}} {{- $tiers := list -}} {{- range $tier := list "viewer" "member" "owner" -}} {{- /* Missing keys in --reuse-values upgrades follow default-on; explicit false survives. */ -}} -{{- if and (index $rbac $tier) (ne (toString (index $integration $tier)) "false") (or (eq $scope "Namespaced") (ne (toString (index $cluster $tier)) "false")) -}} +{{- if and $rbac.create (index $rbac $tier) (ne (toString (index $integration $tier)) "false") (or (eq $scope "Namespaced") (ne (toString (index $cluster $tier)) "false")) -}} {{- $tiers = append $tiers $tier -}} {{- end -}} {{- end -}} +{{- if $systemOn -}} +{{- $tiers = append $tiers "system" -}} +{{- end -}} {{- $rules := list -}} {{- range $entry := $baseline.entries -}} {{- if and (eq $entry.decision "grant") (eq $entry.scope $scope) (or $root.Values.rbac.crdGroups.all (index $root.Values.rbac.crdGroups $entry.collection)) -}} diff --git a/deploy/helm/radar/templates/cloud-rbac-system.yaml b/deploy/helm/radar/templates/cloud-rbac-system.yaml new file mode 100644 index 0000000000..ffc2d893b7 --- /dev/null +++ b/deploy/helm/radar/templates/cloud-rbac-system.yaml @@ -0,0 +1,64 @@ +{{- /* +Read-only access for Radar Cloud's own background identities. + +The hub's alerts worker and timeline puller have no user or IdP groups; they +assert the radar:system group. Without a binding of their own they would ride +on radar:owner, which disappears when an org turns the role bindings off +(cloud.defaultRbac.create=false) to let IdP groups decide cluster access — +silently stopping alerts and the hub timeline. + +The grant is the viewer set (view here, plus the cluster-read and +integration-read add-ons, which bind radar:system in their own templates) and +Secret read. Secret read is what Helm release alerts need: Radar lists Helm +releases as the caller, and Helm stores each release as a Secret. Kubernetes +RBAC cannot narrow that to Helm's Secrets, so it is all Secrets, read-only. +No writes. +*/}} +{{- if and (include "radar.cloudSystemRbac" . | eq "true") .Values.rbac.create -}} +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: {{ include "radar.fullname" . }}-cloud-system-view + labels: + {{- include "radar.labels" . | nindent 4 }} +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: view +subjects: + - kind: Group + name: radar:system + apiGroup: rbac.authorization.k8s.io + - kind: Group + name: cloud:system + apiGroup: rbac.authorization.k8s.io +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: {{ include "radar.fullname" . }}-system-secrets-read + labels: + {{- include "radar.labels" . | nindent 4 }} +rules: + - apiGroups: [""] + resources: ["secrets"] + verbs: ["get", "list", "watch"] +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: {{ include "radar.fullname" . }}-cloud-system-secrets-read + labels: + {{- include "radar.labels" . | nindent 4 }} +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: {{ include "radar.fullname" . }}-system-secrets-read +subjects: + - kind: Group + name: radar:system + apiGroup: rbac.authorization.k8s.io + - kind: Group + name: cloud:system + apiGroup: rbac.authorization.k8s.io +{{- end -}} diff --git a/deploy/helm/radar/tests/cloud_rbac_integration_read_test.yaml b/deploy/helm/radar/tests/cloud_rbac_integration_read_test.yaml index 8d0e6fdc5a..dc611a9c77 100644 --- a/deploy/helm/radar/tests/cloud_rbac_integration_read_test.yaml +++ b/deploy/helm/radar/tests/cloud_rbac_integration_read_test.yaml @@ -6,7 +6,17 @@ set: cloud.token: dummy cloud.url: wss://cloud.example cloud.clusterName: test + # Tier behavior only; radar:system's own binding is covered below and in + # cloud_rbac_system_test.yaml. + cloud.systemRbac: false tests: + - it: keeps both add-ons for radar:system when the tiers are off + set: + cloud.systemRbac: true + cloud.defaultRbac.create: false + asserts: + - hasDocuments: + count: 4 - it: binds both scopes to all three default tiers asserts: - hasDocuments: diff --git a/deploy/helm/radar/tests/cloud_rbac_system_test.yaml b/deploy/helm/radar/tests/cloud_rbac_system_test.yaml new file mode 100644 index 0000000000..3d83cb8c78 --- /dev/null +++ b/deploy/helm/radar/tests/cloud_rbac_system_test.yaml @@ -0,0 +1,155 @@ +suite: Cloud radar:system RBAC +templates: + - cloud-rbac-system.yaml + - cloud-rbac-cluster-read.yaml + - cloud-rbac-integration-read.yaml +tests: + - it: keeps radar:system's view binding when the role bindings are off + set: + cloud: + enabled: true + token: dummy + url: "wss://cloud.example" + clusterName: test-cluster + defaultRbac: + create: false + template: cloud-rbac-system.yaml + documentSelector: + path: metadata.name + value: RELEASE-NAME-radar-cloud-system-view + asserts: + - isKind: + of: ClusterRoleBinding + + - it: keeps the cluster-read role for radar:system alone when the role bindings are off + set: + cloud: + enabled: true + token: dummy + url: "wss://cloud.example" + clusterName: test-cluster + defaultRbac: + create: false + template: cloud-rbac-cluster-read.yaml + asserts: + - hasDocuments: + count: 2 + - isKind: + of: ClusterRole + documentIndex: 0 + - equal: + path: metadata.name + value: RELEASE-NAME-radar-cloud-system-cluster-read + documentIndex: 1 + - equal: + path: subjects[0].name + value: radar:system + documentIndex: 1 + + - it: grants Secret read only, no writes + set: + cloud: + enabled: true + token: dummy + url: "wss://cloud.example" + clusterName: test-cluster + template: cloud-rbac-system.yaml + documentSelector: + path: metadata.name + value: RELEASE-NAME-radar-system-secrets-read + asserts: + - equal: + path: rules + value: + - apiGroups: [""] + resources: ["secrets"] + verbs: ["get", "list", "watch"] + + - it: names both radar:system and the legacy cloud:system group + set: + cloud: + enabled: true + token: dummy + url: "wss://cloud.example" + clusterName: test-cluster + template: cloud-rbac-system.yaml + documentSelector: + path: metadata.name + value: RELEASE-NAME-radar-cloud-system-view + asserts: + - equal: + path: roleRef.name + value: view + - equal: + path: subjects + value: + - kind: Group + name: radar:system + apiGroup: rbac.authorization.k8s.io + - kind: Group + name: cloud:system + apiGroup: rbac.authorization.k8s.io + + - it: stays on when systemRbac is absent from reused values + set: + cloud: + enabled: true + token: dummy + url: "wss://cloud.example" + clusterName: test-cluster + systemRbac: null + template: cloud-rbac-system.yaml + asserts: + - hasDocuments: + count: 3 + + - it: renders nothing when systemRbac is false + set: + cloud: + enabled: true + token: dummy + url: "wss://cloud.example" + clusterName: test-cluster + systemRbac: false + template: cloud-rbac-system.yaml + asserts: + - hasDocuments: + count: 0 + + - it: renders nothing outside cloud mode + template: cloud-rbac-system.yaml + asserts: + - hasDocuments: + count: 0 + + - it: renders nothing when the operator owns all RBAC + set: + rbac: + create: false + cloud: + enabled: true + token: dummy + url: "wss://cloud.example" + clusterName: test-cluster + template: cloud-rbac-system.yaml + asserts: + - hasDocuments: + count: 0 + + - it: gives radar:system the integration-read add-on without the tiers + set: + cloud: + enabled: true + token: dummy + url: "wss://cloud.example" + clusterName: test-cluster + defaultRbac: + create: false + template: cloud-rbac-integration-read.yaml + documentSelector: + path: metadata.name + value: RELEASE-NAME-radar-cloud-system-integration-read-namespaced + asserts: + - equal: + path: subjects[0].name + value: radar:system diff --git a/deploy/helm/radar/values.schema.json b/deploy/helm/radar/values.schema.json index b6e9eb1190..5547b119fd 100644 --- a/deploy/helm/radar/values.schema.json +++ b/deploy/helm/radar/values.schema.json @@ -324,6 +324,7 @@ "nodeOps": { "type": "boolean" } } }, + "systemRbac": { "type": "boolean" }, "insecureSkipVerify": { "type": "boolean" } } }, diff --git a/deploy/helm/radar/values.yaml b/deploy/helm/radar/values.yaml index 3d0bb0bb62..086dd9bb52 100644 --- a/deploy/helm/radar/values.yaml +++ b/deploy/helm/radar/values.yaml @@ -517,6 +517,14 @@ cloud: # while owner=false this renders nothing, but it stays latent — re-enabling # owner later activates it. See templates/cloud-rbac-node-ops.yaml. nodeOps: false + # Read-only binding for Radar Cloud's own background identities (the hub's + # alerts worker and timeline puller), which assert the radar:system group. + # Independent of defaultRbac: it stays on when the role bindings above are + # off, so alerts and the hub timeline keep working when IdP groups decide + # cluster access. Grants the viewer set (view + cluster-read + + # integration-read) plus Secret read, which Helm release alerts need (Helm + # stores releases as Secrets). No writes. See templates/cloud-rbac-system.yaml. + systemRbac: true # MCP (Model Context Protocol) server for AI tools mcp: diff --git a/docs/authentication.md b/docs/authentication.md index 46c52301d6..999794509d 100644 --- a/docs/authentication.md +++ b/docs/authentication.md @@ -232,6 +232,8 @@ Under cloud-mode (`RADAR_CLOUD_MODE=true`, set automatically by the chart when ` - Forces `--auth-mode=proxy` with pinned `X-Forwarded-User` / `X-Forwarded-Groups` headers. Radar accepts those headers only on requests marked in-process by its authenticated Cloud tunnel; the ordinary pod TCP listener cannot assert Cloud identity. - Ships three default ClusterRoleBindings mapping Cloud's `radar:owner` / `radar:member` / `radar:viewer` groups to the standard K8s `admin` / `edit` / `view` ClusterRoles. Configurable via `cloud.defaultRbac.*` in `values.yaml`. - Adds a **cluster-read add-on** (`cloud.defaultRbac.clusterScopedRead.{viewer,member,owner}`, each default on) granting `get/list/watch` on infrastructure the built-in `view`/`edit`/`admin` roles exclude — Nodes, PersistentVolumes, StorageClasses, IngressClasses, PriorityClasses, RuntimeClasses, ClusterTrustBundles, CRDs, and admission webhook configurations — plus list-only Upgrade impact evidence for CSIDrivers, CSIStorageCapacities, legacy PodSecurityPolicies, and API flow-control configuration. When the matching collection is enabled, it also grants APIServices, PrometheusRules, Karpenter kinds, and node metrics. It does not grant Secrets, RBAC objects, API-server metrics, or kubelet proxy access. It's an independent axis per tier: set a tier `false` to make it namespaced-only (e.g. `clusterScopedRead.viewer: false`). Owner node cordon/drain is a separate cluster-scoped *write*, off by default (`cloud.defaultRbac.nodeOps`). +- Binds `radar:system` — the group the hub's alerts worker and timeline puller assert — to a read-only set: `view`, the cluster-read and integration-read add-ons, and Secret read (Helm stores releases as Secrets, and Helm release alerts list them as the caller). Controlled by `cloud.systemRbac` (default on) and independent of `cloud.defaultRbac`, so orgs that turn the role bindings off to let IdP groups (`radar:idp:`) decide cluster access keep alerts and the hub timeline. +- Uses the Cloud role (`radar:owner` / `radar:member` / `radar:viewer`) only to gate Radar's own configuration (settings, integrations, audit settings, self-upgrade), which runs as Radar's ServiceAccount. Operations that run as the impersonated user — Helm reads and writes, in-cluster probe pods — are decided by Kubernetes RBAC alone. - Does **not** grant `PodCertificateRequest` access through the default Cloud tier bindings. Radar surfaces the API when Kubernetes 1.37 serves it, but a Cloud user can browse requests only when their Kubernetes RBAC independently grants `get/list` access. To opt in, bind a Role or ClusterRole granting `get/list/watch` on `certificates.k8s.io/podcertificaterequests` to the intended `radar:*` groups. This stays separate because certificate-request metadata is more sensitive than ordinary workload inventory. - Restricts the ordinary pod/ClusterIP TCP listener to `/api/health`; the full handler is served only over yamux streams from the outbound Cloud tunnel. Cloud mode also omits `/debug/pprof/*` and narrows auth exemptions to health only. diff --git a/docs/reachability.md b/docs/reachability.md index 59c75b3bad..b968789a6f 100644 --- a/docs/reachability.md +++ b/docs/reachability.md @@ -112,7 +112,7 @@ Relayed and Job-based probes are bounded by Kubernetes RBAC, but they do not alw With [authentication](authentication.md) enabled, Radar uses the signed-in user's identity. Without authentication, Kubernetes-authorized probes use Radar's own client identity: your kubeconfig identity locally, or Radar's ServiceAccount when it runs in-cluster. The API server enforces relay permissions at request time; Radar preflights all three Job permissions before creating anything. If impersonation is unavailable, the relay is skipped rather than falling back to Radar's ServiceAccount. A proxy denial can mark that route *unreachable via the API server*, but the headline stays *unknown* because the real path was not confirmed. -The UI asks before the first in-cluster run for a cluster and names the requests it will send (unless that consent was previously remembered). MCP has no dialog: callers must explicitly pass `in_cluster: true`, and the same RBAC preflight applies. Radar Cloud also requires org role Member or higher. +The UI asks before the first in-cluster run for a cluster and names the requests it will send (unless that consent was previously remembered). MCP has no dialog: callers must explicitly pass `in_cluster: true`, and the same RBAC preflight applies. The active Kubernetes identity authorizes Job creation, but network and mesh policy see the probe pod, not that identity. The Job runs in the diagnosed resource's namespace; its pod uses that namespace's default ServiceAccount identity but mounts no ServiceAccount token. Each run creates at most five Jobs; their containers run non-root with a read-only filesystem, all capabilities dropped, a 25-second deadline, no retries, and a 60-second TTL backstop. diff --git a/internal/helm/handlers.go b/internal/helm/handlers.go index 9793095580..d1c9ee7595 100644 --- a/internal/helm/handlers.go +++ b/internal/helm/handlers.go @@ -199,14 +199,10 @@ func (h *Handlers) handleGetRelease(w http.ResponseWriter, r *http.Request) { writeJSON(w, release) } -// handleGetManifest returns the rendered manifest for a release. -// Member+ only — manifests can inline literal Secret resources with -// base64-encoded data, which K8s 'view' (the default radar:viewer -// binding) excludes. +// handleGetManifest returns the rendered manifest for a release. Read as +// the caller: Helm stores releases as Secrets, so Kubernetes RBAC on Secrets +// decides who can see it. func (h *Handlers) handleGetManifest(w http.ResponseWriter, r *http.Request) { - if !requireCloudRole(w, r, auth.RoleMember, "view Helm release manifests") { - return - } client := GetClient() if client == nil { writeError(w, http.StatusServiceUnavailable, "Helm client not initialized") @@ -236,12 +232,8 @@ func (h *Handlers) handleGetManifest(w http.ResponseWriter, r *http.Request) { w.Write([]byte(manifest)) } -// handleGetValues returns the values for a release. Member+ only — -// values may contain credentials set via --set or values.yaml. +// handleGetValues returns the values for a release, read as the caller. func (h *Handlers) handleGetValues(w http.ResponseWriter, r *http.Request) { - if !requireCloudRole(w, r, auth.RoleMember, "view Helm release values") { - return - } client := GetClient() if client == nil { writeError(w, http.StatusServiceUnavailable, "Helm client not initialized") @@ -272,11 +264,7 @@ func (h *Handlers) handleGetValues(w http.ResponseWriter, r *http.Request) { } // handleGetValuesDiff returns a values diff between two release revisions. -// Member+ only — values often contain credentials. func (h *Handlers) handleGetValuesDiff(w http.ResponseWriter, r *http.Request) { - if !requireCloudRole(w, r, auth.RoleMember, "diff Helm release values") { - return - } client := GetClient() if client == nil { writeError(w, http.StatusServiceUnavailable, "Helm client not initialized") @@ -300,12 +288,8 @@ func (h *Handlers) handleGetValuesDiff(w http.ResponseWriter, r *http.Request) { writeJSON(w, diff) } -// handleGetDiff returns the diff between two revisions. Member+ only -// — same surface as GetManifest (renders both revisions). +// handleGetDiff returns the diff between two revisions. func (h *Handlers) handleGetDiff(w http.ResponseWriter, r *http.Request) { - if !requireCloudRole(w, r, auth.RoleMember, "diff Helm release manifests") { - return - } client := GetClient() if client == nil { writeError(w, http.StatusServiceUnavailable, "Helm client not initialized") @@ -332,9 +316,6 @@ func (h *Handlers) handleGetDiff(w http.ResponseWriter, r *http.Request) { // handleGetNotesDiff returns a release notes diff between two revisions. func (h *Handlers) handleGetNotesDiff(w http.ResponseWriter, r *http.Request) { - if !requireCloudRole(w, r, auth.RoleMember, "diff Helm release notes") { - return - } client := GetClient() if client == nil { writeError(w, http.StatusServiceUnavailable, "Helm client not initialized") @@ -357,9 +338,6 @@ func (h *Handlers) handleGetNotesDiff(w http.ResponseWriter, r *http.Request) { // handleGetHooksDiff returns a release hook metadata diff between two revisions. func (h *Handlers) handleGetHooksDiff(w http.ResponseWriter, r *http.Request) { - if !requireCloudRole(w, r, auth.RoleMember, "diff Helm release hooks") { - return - } client := GetClient() if client == nil { writeError(w, http.StatusServiceUnavailable, "Helm client not initialized") @@ -382,9 +360,6 @@ func (h *Handlers) handleGetHooksDiff(w http.ResponseWriter, r *http.Request) { // handleGetResourceDiff returns added/removed rendered resources between revisions. func (h *Handlers) handleGetResourceDiff(w http.ResponseWriter, r *http.Request) { - if !requireCloudRole(w, r, auth.RoleMember, "diff Helm release resources") { - return - } client := GetClient() if client == nil { writeError(w, http.StatusServiceUnavailable, "Helm client not initialized") @@ -506,9 +481,6 @@ func (h *Handlers) handleBatchUpgradeCheck(w http.ResponseWriter, r *http.Reques // handleRollback rolls back a release to a previous revision func (h *Handlers) handleRollback(w http.ResponseWriter, r *http.Request) { - if !requireCloudRole(w, r, auth.RoleMember, "rollback Helm releases") { - return - } if !requireHelmWrite(w, r) { return } @@ -555,9 +527,6 @@ func (h *Handlers) handleRollback(w http.ResponseWriter, r *http.Request) { // handleRollbackStream rolls back a release with SSE progress streaming func (h *Handlers) handleRollbackStream(w http.ResponseWriter, r *http.Request) { - if !requireCloudRole(w, r, auth.RoleMember, "rollback Helm releases") { - return - } if !requireHelmWrite(w, r) { return } @@ -648,9 +617,6 @@ func (h *Handlers) handleRollbackStream(w http.ResponseWriter, r *http.Request) // handleUninstall removes a release func (h *Handlers) handleUninstall(w http.ResponseWriter, r *http.Request) { - if !requireCloudRole(w, r, auth.RoleMember, "uninstall Helm releases") { - return - } if !requireHelmWrite(w, r) { return } @@ -685,9 +651,6 @@ func (h *Handlers) handleUninstall(w http.ResponseWriter, r *http.Request) { // handleUpgrade upgrades a release to a new version func (h *Handlers) handleUpgrade(w http.ResponseWriter, r *http.Request) { - if !requireCloudRole(w, r, auth.RoleMember, "upgrade Helm releases") { - return - } if !requireHelmWrite(w, r) { return } @@ -729,9 +692,6 @@ func (h *Handlers) handleUpgrade(w http.ResponseWriter, r *http.Request) { // handleUpgradeStream upgrades a release with SSE progress streaming func (h *Handlers) handleUpgradeStream(w http.ResponseWriter, r *http.Request) { - if !requireCloudRole(w, r, auth.RoleMember, "upgrade Helm releases") { - return - } if !requireHelmWrite(w, r) { return } @@ -838,12 +798,7 @@ func (h *Handlers) handleUpgradeStream(w http.ResponseWriter, r *http.Request) { } // handlePreviewValues previews the effect of new values on a release. -// Member+ — renders the chart with proposed values, same surface as -// GetManifest. func (h *Handlers) handlePreviewValues(w http.ResponseWriter, r *http.Request) { - if !requireCloudRole(w, r, auth.RoleMember, "preview Helm release values") { - return - } client := GetClient() if client == nil { writeError(w, http.StatusServiceUnavailable, "Helm client not initialized") @@ -871,9 +826,6 @@ func (h *Handlers) handlePreviewValues(w http.ResponseWriter, r *http.Request) { // handleApplyValues applies new values to a release func (h *Handlers) handleApplyValues(w http.ResponseWriter, r *http.Request) { - if !requireCloudRole(w, r, auth.RoleMember, "apply Helm release values") { - return - } if !requireHelmWrite(w, r) { return } @@ -1085,9 +1037,6 @@ func (h *Handlers) handleGetChartDetailVersion(w http.ResponseWriter, r *http.Re // handleInstall installs a new Helm release (non-streaming version) func (h *Handlers) handleInstall(w http.ResponseWriter, r *http.Request) { - if !requireCloudRole(w, r, auth.RoleMember, "install Helm releases") { - return - } if !requireHelmWrite(w, r) { return } @@ -1141,9 +1090,6 @@ func (h *Handlers) handleInstall(w http.ResponseWriter, r *http.Request) { // handleInstallStream installs a Helm release with SSE progress streaming func (h *Handlers) handleInstallStream(w http.ResponseWriter, r *http.Request) { - if !requireCloudRole(w, r, auth.RoleMember, "install Helm releases") { - return - } if !requireHelmWrite(w, r) { return } @@ -1293,8 +1239,7 @@ func writeError(w http.ResponseWriter, status int, message string) { // writeErrorCode is writeError with a stable machine-readable error_code // in the response body so the frontend + MCP clients can branch on the error -// type without parsing the human message. Used for role-gated 403s and -// any other case where the consumer wants to react differently per code. +// type without parsing the human message. func writeErrorCode(w http.ResponseWriter, status int, code, message string) { if status >= 500 { errorlog.Record("helm", "error", "%s", message) @@ -1307,36 +1252,6 @@ func writeErrorCode(w http.ResponseWriter, status int, code, message string) { }) } -// requireCloudRole gates a handler on the caller's Cloud role tier. -// Returns true if the request should proceed. -// -// When the caller has no Cloud role (OSS deploy, or running outside -// Cloud's tunnel), CloudRole.AtLeast bypasses the gate — radar OSS -// continues to use only K8s RBAC for authorization, no Cloud-specific -// product gating. This is the same behavior as before; the gate is -// strictly additive for Cloud-attributed callers. -// -// When the caller IS Cloud-attributed and their tier is below `min`, -// returns 403 with error_code=cloud_role_insufficient so the frontend can -// render a friendly "your role doesn't allow this" message instead of -// a generic auth failure. -func requireCloudRole(w http.ResponseWriter, r *http.Request, min auth.CloudRole, opName string) bool { - role := auth.CloudRoleFromContext(r.Context()) - if role.AtLeast(min) { - return true - } - username := "unknown" - if u := auth.UserFromContext(r.Context()); u != nil { - username = u.Username - } - // All user-controlled values use %q so log-line injection via CR/LF - // in headers or path is escaped. opName is a compile-time literal. - log.Printf("[helm] Cloud role %q denied %s for user %q (need at least %q): %q", role, opName, username, min, r.URL.Path) - writeErrorCode(w, http.StatusForbidden, auth.ErrCodeCloudRoleInsufficient, - "Your Radar Cloud role ("+role.String()+") cannot "+opName+". Requires "+string(min)+" or higher.") - return false -} - // ============================================================================ // ArtifactHub Handlers // ============================================================================ diff --git a/internal/helm/handlers_test.go b/internal/helm/handlers_test.go index 8a28dca1e3..dd483771ab 100644 --- a/internal/helm/handlers_test.go +++ b/internal/helm/handlers_test.go @@ -1,8 +1,6 @@ package helm import ( - "context" - "encoding/json" "io" "net/http" "net/http/httptest" @@ -13,79 +11,6 @@ import ( "github.com/skyhook-io/radar/internal/auth" ) -// TestRequireCloudRole exercises the role gate without standing up a -// Helm client — the gate runs first, so we never hit the client. This -// guards the gate against regressions ("forgot to wire requireCloudRole -// on a new write endpoint", "swapped the comparison the wrong way"). -func TestRequireCloudRole(t *testing.T) { - cases := []struct { - name string - groups []string - min auth.CloudRole - wantOK bool - wantCode string // expected error_code when wantOK == false - }{ - // No user attached (OSS / non-Cloud): gate bypasses. - {name: "no user — bypass", groups: nil, min: auth.RoleMember, wantOK: true}, - - // Cloud user without role group (header stripped, misconfig): - // CloudRole is RoleNone, AtLeast bypasses. This is permissive - // by design — the assumption is that radar-hub guarantees the - // role group when forwarding. - {name: "user without radar:* — bypass", groups: []string{"developers"}, min: auth.RoleOwner, wantOK: true}, - - // Cloud-attributed users — gate enforces. - {name: "viewer denied member-required op", groups: []string{"radar:viewer"}, min: auth.RoleMember, - wantOK: false, wantCode: auth.ErrCodeCloudRoleInsufficient}, - {name: "viewer denied owner-required op", groups: []string{"radar:viewer"}, min: auth.RoleOwner, - wantOK: false, wantCode: auth.ErrCodeCloudRoleInsufficient}, - {name: "viewer allowed viewer-required op", groups: []string{"radar:viewer"}, min: auth.RoleViewer, wantOK: true}, - {name: "member allowed member-required op", groups: []string{"radar:member"}, min: auth.RoleMember, wantOK: true}, - {name: "member denied owner-required op", groups: []string{"radar:member"}, min: auth.RoleOwner, - wantOK: false, wantCode: auth.ErrCodeCloudRoleInsufficient}, - {name: "owner allowed owner-required op", groups: []string{"radar:owner"}, min: auth.RoleOwner, wantOK: true}, - - // Defensive: a stuffed lower tier alongside owner shouldn't - // downgrade the user. CloudRoleFromGroups picks the highest. - {name: "stuffed viewer + owner — allowed", groups: []string{"radar:viewer", "radar:owner"}, min: auth.RoleOwner, wantOK: true}, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - req := httptest.NewRequest(http.MethodGet, "/test", nil) - ctx := req.Context() - if tc.groups != nil { - ctx = auth.ContextWithUser(ctx, &auth.User{Username: "test-user", Groups: tc.groups}) - } else { - _ = context.Background() - } - req = req.WithContext(ctx) - rec := httptest.NewRecorder() - - ok := requireCloudRole(rec, req, tc.min, "test-op") - if ok != tc.wantOK { - t.Errorf("requireCloudRole = %v, want %v (status=%d, body=%s)", - ok, tc.wantOK, rec.Code, rec.Body.String()) - } - if !tc.wantOK { - if rec.Code != http.StatusForbidden { - t.Errorf("status = %d, want 403", rec.Code) - } - var resp map[string]string - if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { - t.Fatalf("decode body: %v (body=%s)", err, rec.Body.String()) - } - if resp["error_code"] != tc.wantCode { - t.Errorf("error_code = %q, want %q", resp["error_code"], tc.wantCode) - } - if resp["error"] == "" { - t.Error("error message is empty") - } - } - }) - } -} - func TestDecodeOptionalApplyValuesRequest(t *testing.T) { cases := []struct { name string @@ -176,25 +101,15 @@ func TestDecodeApplyValuesRequest(t *testing.T) { }) } -// TestSensitiveHelmHandlers_GateOnViewer asserts that every Helm -// handler we believe is gated actually 403s a Cloud viewer with -// error_code=cloud_role_insufficient. The unit test above -// (TestRequireCloudRole) covers the gate function in isolation; this -// test covers the *application* of the gate to each handler. -// -// Why both: the gate was retrofitted as a one-line `if !requireCloudRole(...) -// { return }` at the top of each handler. A future refactor that drops -// the line (or reorders it after a side-effecting call) would silently -// regress without the unit test failing. This test fails the moment -// the gate disappears from any covered handler. -// -// We don't need a real Helm client because the gate runs first; the -// handler short-circuits before touching `helm.GetClient()`. -func TestSensitiveHelmHandlers_GateOnViewer(t *testing.T) { +// TestHelmHandlers_NotGatedOnCloudRole pins that Helm operations are not +// refused on the caller's Radar Cloud role. They run as the impersonated user, +// so Kubernetes RBAC decides; a role check here would override IdP-group +// bindings that grant the user access (a viewer whose group can edit a +// namespace must be able to roll back there). Other refusals (no Helm client, +// rbac.helm off) are fine here; only a role refusal fails. +func TestHelmHandlers_NotGatedOnCloudRole(t *testing.T) { h := NewHandlers(nil) - // (name, method, handler) for every endpoint we believe is gated. - // Sensitive reads → member; writes → member. cases := []struct { name string method string @@ -202,6 +117,7 @@ func TestSensitiveHelmHandlers_GateOnViewer(t *testing.T) { }{ {"GetManifest", http.MethodGet, h.handleGetManifest}, {"GetValues", http.MethodGet, h.handleGetValues}, + {"GetValuesDiff", http.MethodGet, h.handleGetValuesDiff}, {"GetDiff", http.MethodGet, h.handleGetDiff}, {"GetNotesDiff", http.MethodGet, h.handleGetNotesDiff}, {"GetHooksDiff", http.MethodGet, h.handleGetHooksDiff}, @@ -222,21 +138,15 @@ func TestSensitiveHelmHandlers_GateOnViewer(t *testing.T) { req := httptest.NewRequest(tc.method, "/test", nil) req = req.WithContext(auth.ContextWithUser(req.Context(), &auth.User{ Username: "viewer-test", - Groups: []string{"radar:viewer"}, + Groups: []string{"radar:viewer", "radar:idp:team-a-devs"}, })) rec := httptest.NewRecorder() tc.handler(rec, req) - if rec.Code != http.StatusForbidden { - t.Fatalf("status = %d, want 403 (gate should fire before any helm work)", rec.Code) - } - var resp map[string]string - if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { - t.Fatalf("decode body: %v (body=%s)", err, rec.Body.String()) - } - if resp["error_code"] != auth.ErrCodeCloudRoleInsufficient { - t.Errorf("error_code = %q, want %q", resp["error_code"], auth.ErrCodeCloudRoleInsufficient) + body := rec.Body.String() + if strings.Contains(body, auth.ErrCodeCloudRoleInsufficient) || strings.Contains(body, "Radar Cloud role") { + t.Fatalf("status = %d body = %s; a Cloud viewer must reach Kubernetes RBAC, not a role gate", rec.Code, body) } }) } diff --git a/internal/k8s/integration_read_baseline_test.go b/internal/k8s/integration_read_baseline_test.go index 55d8de3cc9..351863470c 100644 --- a/internal/k8s/integration_read_baseline_test.go +++ b/internal/k8s/integration_read_baseline_test.go @@ -174,11 +174,16 @@ func TestIntegrationReadBindings(t *testing.T) { name, overrides string namespaced, cluster []string removeMap, removeKey bool + // radar:system is bound whenever cloud mode renders RBAC at all, + // independent of the tier settings. + noSystem bool }{ {name: "defaults", namespaced: all, cluster: all}, - {name: "OSS", overrides: "cloud.enabled=false"}, + {name: "OSS", overrides: "cloud.enabled=false", noSystem: true}, {name: "customer managed", overrides: "cloud.defaultRbac.create=false"}, - {name: "RBAC disabled", overrides: "rbac.create=false"}, + {name: "RBAC disabled", overrides: "rbac.create=false", noSystem: true}, + {name: "system off", overrides: "cloud.systemRbac=false", namespaced: all, cluster: all, noSystem: true}, + {name: "system and tiers off", overrides: "cloud.systemRbac=false,cloud.defaultRbac.create=false", noSystem: true}, {name: "viewer disabled", overrides: "cloud.defaultRbac.viewer=false", namespaced: all[1:], cluster: all[1:]}, {name: "member addon off", overrides: "cloud.defaultRbac.integrationRead.member=false", namespaced: []string{"viewer", "owner"}, cluster: []string{"viewer", "owner"}}, {name: "owner addon off", overrides: "cloud.defaultRbac.integrationRead.owner=false", namespaced: all[:2], cluster: all[:2]}, @@ -194,8 +199,13 @@ func TestIntegrationReadBindings(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { _, docs := renderIntegrationReadChart(t, tc.overrides, tc.removeMap, tc.removeKey) + namespaced, cluster := tc.namespaced, tc.cluster + if !tc.noSystem { + namespaced = append(append([]string{}, namespaced...), "system") + cluster = append(append([]string{}, cluster...), "system") + } wantDocs := 0 - for scope, tiers := range map[string][]string{"namespaced": tc.namespaced, "cluster": tc.cluster} { + for scope, tiers := range map[string][]string{"namespaced": namespaced, "cluster": cluster} { if len(tiers) == 0 { continue } diff --git a/internal/mcp/tools_diagnose.go b/internal/mcp/tools_diagnose.go index 165bb4663e..30d74c2c59 100644 --- a/internal/mcp/tools_diagnose.go +++ b/internal/mcp/tools_diagnose.go @@ -23,7 +23,6 @@ import ( "github.com/skyhook-io/radar/internal/reachability" "github.com/skyhook-io/radar/internal/trace" aicontext "github.com/skyhook-io/radar/pkg/ai/context" - pkgauth "github.com/skyhook-io/radar/pkg/auth" "github.com/skyhook-io/radar/pkg/issuesapi" "github.com/skyhook-io/radar/pkg/k8score" "github.com/skyhook-io/radar/pkg/probe" @@ -1120,13 +1119,6 @@ func handleNetworkTraceDiagnose(ctx context.Context, input diagnoseInput, kind s } var inClusterTests []reachability.InClusterTestResult if input.InCluster { - // Creating transient probe pods is a mutating, product-gated action. Mirror - // the REST handler (reachability_run.go requireCloudRole) and the helm - // precedent (tools_helm.go) so a sub-Member Cloud Viewer can't bypass the - // RoleMember gate via MCP. probe=true / static paths stay viewer-accessible. - if role := pkgauth.CloudRoleFromContext(ctx); !role.AtLeast(pkgauth.RoleMember) { - return nil, nil, fmt.Errorf("Radar Cloud role %q cannot run an in-cluster reachability test (requires member or higher)", role.String()) - } var byTarget map[string][]probe.Result inClusterTests, byTarget = reachability.RunInClusterTests(ctx, tr, input.Namespace) // Fold the live results back in: an in-cluster pass upgrades the route to diff --git a/internal/mcp/tools_helm.go b/internal/mcp/tools_helm.go index d15ef9162c..4d964a2444 100644 --- a/internal/mcp/tools_helm.go +++ b/internal/mcp/tools_helm.go @@ -147,26 +147,14 @@ func handleGetHelmRelease(ctx context.Context, req *mcp.CallToolRequest, input g includes := parseIncludes(input.Include) - // Mirror the frontend gate on sensitive Helm reads: viewers cannot pull - // values/manifest/diff. Without this the user would still be blocked - // by K8s RBAC (view ClusterRole excludes secrets), but the error would - // be a confusing K8s "secrets is forbidden" rather than the structured - // cloud_role_insufficient code the frontend emits. - cloudRole := pkgauth.CloudRoleFromContext(ctx) - gatedSensitive := !cloudRole.AtLeast(pkgauth.RoleMember) - if includes["values"] { - if gatedSensitive { - result["valuesError"] = fmt.Sprintf("Radar Cloud role %q cannot view Helm release values (requires member or higher)", cloudRole.String()) + values, err := helmClient.GetValuesAsUser(input.Namespace, input.Name, false, username, groups) + if err != nil { + log.Printf("[mcp] Failed to get values for %s/%s: %v", input.Namespace, input.Name, err) + result["valuesError"] = err.Error() } else { - values, err := helmClient.GetValuesAsUser(input.Namespace, input.Name, false, username, groups) - if err != nil { - log.Printf("[mcp] Failed to get values for %s/%s: %v", input.Namespace, input.Name, err) - result["valuesError"] = err.Error() - } else { - result["values"] = redactedHelmValues(values.UserSupplied) - result["valuesRedacted"] = true - } + result["values"] = redactedHelmValues(values.UserSupplied) + result["valuesRedacted"] = true } } @@ -181,8 +169,6 @@ func handleGetHelmRelease(ctx context.Context, req *mcp.CallToolRequest, input g if errMsg := diffRevisionError(input, detail.Revision, "diff"); errMsg != "" { // Surface the contract gap instead of silently producing no diff. result["diffError"] = errMsg - } else if gatedSensitive { - result["diffError"] = fmt.Sprintf("Radar Cloud role %q cannot view Helm release diffs (requires member or higher)", cloudRole.String()) } else { rev1, rev2 := diffRevisions(input, detail.Revision) diff, err := helmClient.GetManifestDiffAsUser(input.Namespace, input.Name, rev1, rev2, username, groups) @@ -197,8 +183,6 @@ func handleGetHelmRelease(ctx context.Context, req *mcp.CallToolRequest, input g if includes["values_diff"] { if errMsg := diffRevisionError(input, detail.Revision, "values_diff"); errMsg != "" { result["valuesDiffError"] = errMsg - } else if gatedSensitive { - result["valuesDiffError"] = fmt.Sprintf("Radar Cloud role %q cannot view Helm release value diffs (requires member or higher)", cloudRole.String()) } else { rev1, rev2 := diffRevisions(input, detail.Revision) diff, err := helmClient.GetValuesDiffAsUser(input.Namespace, input.Name, rev1, rev2, false, username, groups) @@ -214,8 +198,6 @@ func handleGetHelmRelease(ctx context.Context, req *mcp.CallToolRequest, input g if includes["notes_diff"] { if errMsg := diffRevisionError(input, detail.Revision, "notes_diff"); errMsg != "" { result["notesDiffError"] = errMsg - } else if gatedSensitive { - result["notesDiffError"] = fmt.Sprintf("Radar Cloud role %q cannot view Helm release notes diffs (requires member or higher)", cloudRole.String()) } else { rev1, rev2 := diffRevisions(input, detail.Revision) diff, err := helmClient.GetNotesDiffAsUser(input.Namespace, input.Name, rev1, rev2, username, groups) @@ -230,8 +212,6 @@ func handleGetHelmRelease(ctx context.Context, req *mcp.CallToolRequest, input g if includes["resource_diff"] { if errMsg := diffRevisionError(input, detail.Revision, "resource_diff"); errMsg != "" { result["resourceDiffError"] = errMsg - } else if gatedSensitive { - result["resourceDiffError"] = fmt.Sprintf("Radar Cloud role %q cannot view Helm release resource diffs (requires member or higher)", cloudRole.String()) } else { rev1, rev2 := diffRevisions(input, detail.Revision) diff, err := helmClient.GetResourceDiffAsUser(input.Namespace, input.Name, rev1, rev2, username, groups) diff --git a/internal/server/reachability_run.go b/internal/server/reachability_run.go index da48d94959..4f408db7f5 100644 --- a/internal/server/reachability_run.go +++ b/internal/server/reachability_run.go @@ -69,15 +69,6 @@ func (s *Server) handleProbeInClusterCapability(w http.ResponseWriter, r *http.R s.writeJSON(w, resp) return } - // The POST gate is additive: K8s RBAC AND the Cloud-role tier (Member). Mirror - // the Cloud-role half here (without the 403) so the capability answer matches - // what the POST will actually do - a radar:viewer with the right RBAC must not - // see allowed=true and then get a 403 cloud_role_insufficient on submit. - if !auth.CloudRoleFromContext(r.Context()).AtLeast(auth.RoleMember) { - resp.Reason = "your Radar Cloud role cannot run an in-cluster reachability test" - s.writeJSON(w, resp) - return - } allowed, reason, err := reachability.Capability(r.Context(), client, namespace) switch { case err != nil: @@ -118,12 +109,6 @@ func (s *Server) handleTraceInCluster(w http.ResponseWriter, r *http.Request) { s.writeError(w, http.StatusBadRequest, "trace is only supported for Service, Ingress, HTTPRoute, GRPCRoute, or Gateway") return } - // Creating transient probe pods is the one mutating action here - gate on the - // Cloud-role tier every mutating handler enforces (Member), mirroring the MCP - // diagnose(inCluster) path. - if !s.requireCloudRole(w, r, auth.RoleMember, "run an in-cluster reachability test") { - return - } namespaces := s.traceNamespaceCeiling(r) // Mirror handleTrace: never leak that a resource exists outside the caller's // namespace scope - return an unknown-verdict trace instead. diff --git a/internal/server/reachability_run_test.go b/internal/server/reachability_run_test.go index c6471e561e..8fdda66549 100644 --- a/internal/server/reachability_run_test.go +++ b/internal/server/reachability_run_test.go @@ -128,13 +128,10 @@ func TestStampInClusterProbes_CrossNSResultSkipsSubjectNSTwin(t *testing.T) { } } -// The one mutating action here is creating probe Pods in the customer's -// cluster. The Cloud-role gate (Member+) is the only thing between a Cloud -// Viewer and that - regression means a viewer creates pods. -func TestTraceInClusterRequiresCloudMember(t *testing.T) { - // Only recognized sub-Member tiers are denied: an unrecognized tier group - // maps to RoleNone, which AtLeast() deliberately bypasses so non-Cloud - // deploys aren't gated. +// Probe pods are created with the caller's impersonated client, so Kubernetes +// RBAC decides who may run the test. A Cloud-role refusal here would block a +// viewer whose IdP group grants pod create in the namespace. +func TestTraceInClusterNotGatedOnCloudRole(t *testing.T) { for _, tier := range []string{"cloud:viewer", "radar:viewer"} { t.Run(tier, func(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "/api/trace/Service/prod/web/in-cluster", strings.NewReader(`{}`)) @@ -148,11 +145,8 @@ func TestTraceInClusterRequiresCloudMember(t *testing.T) { (&Server{}).handleTraceInCluster(rec, req) - if rec.Code != http.StatusForbidden { - t.Fatalf("status = %d, want 403; body=%s", rec.Code, rec.Body.String()) - } - if !strings.Contains(rec.Body.String(), auth.ErrCodeCloudRoleInsufficient) { - t.Fatalf("body %q missing %q", rec.Body.String(), auth.ErrCodeCloudRoleInsufficient) + if strings.Contains(rec.Body.String(), auth.ErrCodeCloudRoleInsufficient) { + t.Fatalf("status = %d body = %s; want no Cloud-role refusal", rec.Code, rec.Body.String()) } }) } diff --git a/packages/k8s-ui/src/components/resources/ResourcesView.tsx b/packages/k8s-ui/src/components/resources/ResourcesView.tsx index 658c6022b4..6865967de8 100644 --- a/packages/k8s-ui/src/components/resources/ResourcesView.tsx +++ b/packages/k8s-ui/src/components/resources/ResourcesView.tsx @@ -3376,6 +3376,8 @@ interface ResourcesViewProps { /** Per-kind reason a forbidden kind is hidden ("rbac_denied" | "unavailable"), * keyed by the same count key as resourceForbidden. Drives RestrictedState copy. */ resourceReasons?: Record + /** Groups the caller can be granted access through (see RestrictedState). */ + rbacSubjects?: string[] resourceUnavailable?: string[] // Single query for the currently selected kind's full data selectedKindQuery?: ResourceQueryResult @@ -3632,6 +3634,7 @@ export function ResourcesView({ resourceCounts: resourceCountsProp, resourceForbidden: resourceForbiddenProp, resourceReasons, + rbacSubjects, resourceUnavailable: resourceUnavailableProp, selectedKindQuery: selectedKindQueryProp, connectionState, @@ -6372,6 +6375,7 @@ export function ResourcesView({ group={selectedKind.group} resource={selectedKind.name} reason={resourceReasons?.[selectedKindCountKey]} + subjects={rbacSubjects} /> diff --git a/packages/k8s-ui/src/components/ui/RestrictedState.test.tsx b/packages/k8s-ui/src/components/ui/RestrictedState.test.tsx new file mode 100644 index 0000000000..2c1e61a416 --- /dev/null +++ b/packages/k8s-ui/src/components/ui/RestrictedState.test.tsx @@ -0,0 +1,17 @@ +import { describe, expect, it } from 'vitest' +import { buildRbacRequest } from './RestrictedState' + +describe('buildRbacRequest', () => { + it('keeps the placeholder subject when the caller has no bindable groups', () => { + const yaml = buildRbacRequest('', 'nodes', []) + expect(yaml).toContain('name: ') + }) + + it('binds only the first group and lists the rest as commented alternatives', () => { + const yaml = buildRbacRequest('', 'nodes', ['radar:idp:team-a', 'radar:idp:everyone']) + expect(yaml).toContain(' name: "radar:idp:team-a"') + expect(yaml).toContain(' # or: name: "radar:idp:everyone"') + expect(yaml).not.toMatch(/^ {4}name: "radar:idp:everyone"$/m) + expect(yaml).not.toContain('') + }) +}) diff --git a/packages/k8s-ui/src/components/ui/RestrictedState.tsx b/packages/k8s-ui/src/components/ui/RestrictedState.tsx index 8083c05c39..574f90fe00 100644 --- a/packages/k8s-ui/src/components/ui/RestrictedState.tsx +++ b/packages/k8s-ui/src/components/ui/RestrictedState.tsx @@ -25,12 +25,36 @@ interface Props { * ServiceAccount can't read it at all (not installed / SA RBAC / feature off) * — a user grant won't help, so show a different message and no snippet. */ reason?: 'rbac_denied' | 'unavailable' | string + /** Kubernetes groups the caller carries that an admin can bind, e.g. the + * `radar:idp:` groups Radar Cloud forwards. The first becomes the + * snippet's subject; the rest are listed as commented alternatives. */ + subjects?: string[] /** Tightens spacing for inline/embedded use (topology overlay, etc.). */ compact?: boolean className?: string } -function buildRbacRequest(group: string, resource: string): string { +// Only the first group is bound: a session's groups often include an org-wide +// one ("Everyone"), and granting that by copy-paste would widen access for the +// whole org. The admin picks from the commented alternatives. +function subjectLines(subjects: string[]): string[] { + if (subjects.length === 0) { + return [ + ` - kind: Group # or User / ServiceAccount`, + ` name: `, + ` apiGroup: rbac.authorization.k8s.io`, + ] + } + const [first, ...rest] = subjects + return [ + ` - kind: Group`, + ` name: "${first}"`, + ` apiGroup: rbac.authorization.k8s.io`, + ...rest.map((g) => ` # or: name: "${g}"`), + ] +} + +export function buildRbacRequest(group: string, resource: string, subjects: string[]): string { // resource is the placeholder when we don't have a confident API plural // (e.g. a CRD deep-link before discovery resolves the kind) — use a generic // role name rather than radar-read-. @@ -54,13 +78,11 @@ function buildRbacRequest(group: string, resource: string): string { ` kind: ClusterRole`, ` name: ${roleName}`, `subjects:`, - ` - kind: Group # or User / ServiceAccount`, - ` name: `, - ` apiGroup: rbac.authorization.k8s.io`, + ...subjectLines(subjects), ].join('\n') } -export function RestrictedState({ kindLabel, group = '', resource, reason, compact, className }: Props) { +export function RestrictedState({ kindLabel, group = '', resource, reason, subjects = [], compact, className }: Props) { const [expanded, setExpanded] = useState(false) const accessDisclosure = useDisclosure(expanded) const [copied, setCopied] = useState(false) @@ -72,7 +94,7 @@ export function RestrictedState({ kindLabel, group = '', resource, reason, compa // produce a snippet that doesn't grant access. Only inline the resource when // it looks like a valid resource name; otherwise leave a clear placeholder. const validResource = resource && /^[a-z0-9.-]+$/.test(resource) ? resource : '' - const snippet = buildRbacRequest(group, validResource) + const snippet = buildRbacRequest(group, validResource, subjects) const copy = () => { navigator.clipboard.writeText(snippet).then( @@ -125,8 +147,9 @@ export function RestrictedState({ kindLabel, group = '', resource, reason, compa

- Apply this, or send it to whoever administers your cluster, to grant your identity - read access. + {subjects.length > 0 + ? 'Apply this, or send it to whoever administers your cluster. It grants read access to one of your identity provider groups.' + : 'Apply this, or send it to whoever administers your cluster, to grant your identity read access.'}

diff --git a/pkg/auth/cloud_role.go b/pkg/auth/cloud_role.go index 286f38d6c5..e2f277e928 100644 --- a/pkg/auth/cloud_role.go +++ b/pkg/auth/cloud_role.go @@ -10,11 +10,11 @@ import ( // into X-Forwarded-Groups (plus the deprecated `cloud:*` equivalents for // the transition window); this type extracts that signal. // -// CloudRole is the **product-side** boundary (decides whether the user -// can attempt an operation in the UI / API). The K8s ClusterRoleBindings -// the chart ships are the **structural** boundary (decides whether the -// K8s API server allows the impersonated request). The two stack: -// CloudRole gates "can I attempt it?", K8s RBAC gates "can it succeed?". +// CloudRole gates Radar's own features — settings, integrations, self-upgrade — +// which run as Radar's service account, so nothing else checks them. It must +// not gate operations that run as the impersonated user (Helm, probe pods): +// Kubernetes RBAC decides those, and when IdP groups grant access the role +// says nothing about what the user may do in the cluster. type CloudRole string const ( diff --git a/scripts/test-chart.sh b/scripts/test-chart.sh index 6798f49170..d51cbd6a70 100755 --- a/scripts/test-chart.sh +++ b/scripts/test-chart.sh @@ -261,14 +261,20 @@ assert_contains 'name: radar-cloud-member-cluster-read$' "member binding assert_contains 'name: radar-cloud-owner-cluster-read$' "owner binding kept" echo -render "all clusterScopedRead false: no role, no bindings" $CLOUD --set cloud.defaultRbac.clusterScopedRead.viewer=false --set cloud.defaultRbac.clusterScopedRead.member=false --set cloud.defaultRbac.clusterScopedRead.owner=false +render "all clusterScopedRead false: no role, no bindings" $CLOUD --set cloud.systemRbac=false --set cloud.defaultRbac.clusterScopedRead.viewer=false --set cloud.defaultRbac.clusterScopedRead.member=false --set cloud.defaultRbac.clusterScopedRead.owner=false assert_not_contains 'name: radar-cluster-read$' "cluster-read role suppressed" echo -render "all tiers disabled: no orphan cluster-read role" $CLOUD --set cloud.defaultRbac.viewer=false --set cloud.defaultRbac.member=false --set cloud.defaultRbac.owner=false +render "all tiers disabled: no orphan cluster-read role" $CLOUD --set cloud.systemRbac=false --set cloud.defaultRbac.viewer=false --set cloud.defaultRbac.member=false --set cloud.defaultRbac.owner=false assert_not_contains 'name: radar-cluster-read$' "no unbound ClusterRole when every tier is off" echo +render "role bindings off: cluster-read stays for radar:system only" $CLOUD --set cloud.defaultRbac.create=false +assert_contains 'name: radar-cluster-read$' "cluster-read role kept for radar:system" +assert_contains 'name: radar-cloud-system-cluster-read$' "radar:system bound to cluster-read" +assert_not_contains 'name: radar-cloud-viewer-cluster-read$' "no tier cluster-read bindings" +echo + render "custom viewerClusterRole does NOT drop viewer cluster-read (independent axes)" $CLOUD --set cloud.defaultRbac.viewerClusterRole=my-restricted-view assert_contains 'name: radar-cloud-viewer-cluster-read$' "viewer keeps cluster-read with custom namespaced role" echo diff --git a/web/src/api/client.ts b/web/src/api/client.ts index 5aebe42972..532c2d9f7b 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -2202,34 +2202,13 @@ export function useCloudRole() { } /** - * useCanHelmAct combines the K8s capability gate (rbac.helm=true) and - * the Cloud role gate (member+) into a single answer for any Helm - * write or sensitive-read button. Returns { allowed, reason } so the - * tooltip can explain which gate failed. - * - * Cloud role check runs FIRST so the message is actionable for Cloud - * users — telling them "Helm write permissions required" is wrong if - * the chart is fine and the actual gate is their viewer role. + * useCanHelmAct answers whether Helm write and sensitive-read buttons are + * usable. Only the chart's rbac.helm capability is checked here: the calls run + * as the impersonated user, so Kubernetes RBAC decides the rest. The Cloud role + * says nothing about cluster access when IdP groups grant it. */ export function useCanHelmAct(): { allowed: boolean; reason?: string } { const helmWrite = useCanHelmWrite(); - const { role, canAtLeast, isLoading } = useCloudRole(); - // Fail-closed for action buttons during the auth/me round-trip: - // a Cloud viewer who clicks during loading would otherwise fire a - // real request that gets 403'd. For OSS / kubectl-plugin the - // round-trip is sub-ms so this is imperceptible; for Cloud it - // prevents the click-through window. Distinct from useCloudRole's - // canAtLeast (which is optimistic during loading) because passive - // content gates don't have a click-handler to misfire. - if (isLoading) { - return { allowed: false, reason: "Loading permissions…" }; - } - if (!canAtLeast("member")) { - return { - allowed: false, - reason: `Your Radar Cloud role (${role ?? "unknown"}) cannot run Helm operations. Ask a member or owner.`, - }; - } if (!helmWrite) { return { allowed: false, diff --git a/web/src/components/helm/HelmCompareRoute.tsx b/web/src/components/helm/HelmCompareRoute.tsx index d13fd6c1f2..8ad93baa24 100644 --- a/web/src/components/helm/HelmCompareRoute.tsx +++ b/web/src/components/helm/HelmCompareRoute.tsx @@ -19,7 +19,6 @@ import { } from 'lucide-react' import { PaneLoader, DiffLine, hasDiffBodyChange } from '@skyhook-io/k8s-ui' import { - useCloudRole, useHelmHooksDiff, useHelmManifestDiff, useHelmNotesDiff, @@ -30,7 +29,6 @@ import { import type { HelmHook, HelmRevision, HooksDiff, ResourceDiff } from '../../types' import { getHelmStatusColor, getKindBadgeColor, SEVERITY_BADGE } from '../../utils/badge-colors' import { formatDate } from './helm-utils' -import { RoleGatedPanel } from './RoleGatedPanel' import { Tooltip } from '../ui/Tooltip' import { TRANSITION_MENU, overlayExitMs, overlayTransitionStyle } from '../../utils/animation' import { useAnimatedUnmount } from '../../hooks/useAnimatedUnmount' @@ -53,8 +51,6 @@ export function HelmCompareRoute() { const navigate = useNavigate() const location = useLocation() const [searchParams, setSearchParams] = useSearchParams() - const { canAtLeast } = useCloudRole() - const canViewSensitive = canAtLeast('member') const releaseRef = parseReleaseParam(searchParams.get('release')) const storageNamespace = searchParams.get('releaseStorage') || undefined @@ -74,7 +70,7 @@ export function HelmCompareRoute() { const revision1 = defaultLeftRevision const revision2 = defaultRightRevision const pairReady = Boolean(helmNamespace && releaseName && revision1 > 0 && revision2 > 0 && revision1 !== revision2) - const diffEnabled = canViewSensitive && pairReady + const diffEnabled = pairReady const left = revisions.find((r) => r.revision === revision1) const right = revisions.find((r) => r.revision === revision2) @@ -214,7 +210,6 @@ export function HelmCompareRoute() {
-
{pairReady && ( @@ -327,7 +322,6 @@ export function HelmCompareRoute() {
-
) } diff --git a/web/src/components/helm/HelmReleaseDrawer.tsx b/web/src/components/helm/HelmReleaseDrawer.tsx index a3bfd2ee62..aa95c65954 100644 --- a/web/src/components/helm/HelmReleaseDrawer.tsx +++ b/web/src/components/helm/HelmReleaseDrawer.tsx @@ -18,8 +18,7 @@ import type { SelectedHelmRelease, HelmHook, ChartDependency, HelmOperation, Hel import { apiVersionToGroup, kindToPluralWithGroup, type NavigateToResource } from '../../utils/navigation' import { formatDate } from './helm-utils' import { getHelmStatusColor, getKindBadgeColor, getResourceStatusColor, SEVERITY_BADGE, SEVERITY_TEXT } from '../../utils/badge-colors' -import { useCanHelmAct, useCloudRole } from '../../api/client' -import { RoleGatedPanel } from './RoleGatedPanel' +import { useCanHelmAct } from '../../api/client' import { RevisionHistory } from './RevisionHistory' import { ManifestViewer } from './ManifestViewer' import { ValuesViewer } from './ValuesViewer' @@ -130,12 +129,6 @@ export function HelmReleaseDrawer({ release, onClose, onNavigateToResource, isOp const targetVersionRef = useRef('') const editedUpgradeYamlRef = useRef('') const { allowed: canHelmWrite, reason: helmActReason } = useCanHelmAct() - // Cloud viewers can't view release manifests / values / diffs - // (backend gate at requireCloudRole('member')). Skip the queries - // when the role would 403 — saves a round-trip and avoids a - // transient error state under the role-gated panel. - const { canAtLeast } = useCloudRole() - const canViewSensitive = canAtLeast('member') const helmNamespace = release.storageNamespace || release.namespace const { data: releaseDetail, isLoading, error: releaseError, refetch: refetchRelease } = useHelmRelease( @@ -149,7 +142,6 @@ export function HelmReleaseDrawer({ release, onClose, onNavigateToResource, isOp helmNamespace, release.name, selectedRevision, - canViewSensitive, ) // Fetch values @@ -157,7 +149,7 @@ export function HelmReleaseDrawer({ release, onClose, onNavigateToResource, isOp helmNamespace, release.name, showAllValues, - canViewSensitive, + true, selectedRevision, ) const { @@ -168,7 +160,7 @@ export function HelmReleaseDrawer({ release, onClose, onNavigateToResource, isOp helmNamespace, release.name, false, - canViewSensitive && showUpgradeConfirm && adjustValues, + showUpgradeConfirm && adjustValues, ) // Lazy check for upgrade availability @@ -714,7 +706,7 @@ export function HelmReleaseDrawer({ release, onClose, onNavigateToResource, isOp releaseNamespace={releaseDetail.namespace} managedByFluxHelmRelease={releaseDetail.managedByFluxHelmRelease} hookDiagnostics={releaseDetail.hookDiagnostics} - canCompare={canViewSensitive} + canCompare onCompare={handleCompareRevisions} onNavigateToResource={onNavigateToResource} /> @@ -732,19 +724,16 @@ export function HelmReleaseDrawer({ release, onClose, onNavigateToResource, isOp /> )} {activeTab === 'manifest' && ( - - copyToClipboard(text, 'manifest')} copied={copied === 'manifest'} /> - )} {activeTab === 'values' && ( - - refetch()} /> - )} {activeTab === 'resources' && ( )} - {upgradeProgress.length === 0 && canHelmWrite && canViewSensitive && ( + {upgradeProgress.length === 0 && canHelmWrite && (
+ )} + +
+ +
+ {groups.map((g) => ( + + {g} + + ))} +
+ + +
+

+ Ask a cluster admin to bind one of your groups to a role. Fill in the placeholders + before applying; as written, this grants nothing. +

+
+
+ ClusterRoleBinding + +
+ 
{snippet}
+
+
+ +
+ + + ) +} diff --git a/web/src/components/noClusterAccess.test.ts b/web/src/components/noClusterAccess.test.ts new file mode 100644 index 0000000000..b971d7bd02 --- /dev/null +++ b/web/src/components/noClusterAccess.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it } from 'vitest' +import { bindableGroups, buildNoAccessBinding } from './noClusterAccess' + +describe('bindableGroups', () => { + it('keeps only person-identifying Radar Cloud groups', () => { + expect( + bindableGroups([ + 'radar:viewer', + 'radar:org:o1', + 'radar:user:u1', + 'radar:email:bob@example.com', + 'radar:idp:team-a', + 'cloud:viewer', + ]), + ).toEqual(['radar:user:u1', 'radar:email:bob@example.com', 'radar:idp:team-a']) + }) + + it('keeps every group outside Cloud', () => { + expect(bindableGroups(['oidc:platform', 'devs'])).toEqual(['oidc:platform', 'devs']) + }) +}) + +describe('buildNoAccessBinding', () => { + it('grants nothing as-is and lists the groups as comments', () => { + const yaml = buildNoAccessBinding(['radar:idp:team-a', 'radar:idp:everyone']) + expect(yaml).toContain(' name: ') + expect(yaml).toContain(' name: ') + expect(yaml).toContain('# radar:idp:everyone') + expect(yaml).not.toMatch(/^\s+name: "?radar:idp/m) + }) + + it('omits the group list when there are none', () => { + expect(buildNoAccessBinding([])).not.toContain('# Your groups') + }) +}) diff --git a/web/src/components/noClusterAccess.ts b/web/src/components/noClusterAccess.ts new file mode 100644 index 0000000000..c4b8648248 --- /dev/null +++ b/web/src/components/noClusterAccess.ts @@ -0,0 +1,35 @@ +// Groups worth offering an admin to bind. Radar Cloud forwards its own +// vocabulary: only the IdP, user and email groups identify this person; +// radar: and radar:org: would grant every member of the org, and +// cloud:* are legacy duplicates. Outside Cloud every group is the operator's. +export function bindableGroups(groups: readonly string[]): string[] { + const radar = groups.filter((g) => g.startsWith('radar:') || g.startsWith('cloud:')) + if (radar.length === 0) return [...groups] + return groups.filter( + (g) => g.startsWith('radar:idp:') || g.startsWith('radar:user:') || g.startsWith('radar:email:'), + ) +} + +// Applies as-is without granting anything: the subject and role are +// placeholders. Choosing which group gets which role is the admin's call; a +// pre-filled first group could be an org-wide one. +export function buildNoAccessBinding(groups: readonly string[]): string { + const lines = [ + 'apiVersion: rbac.authorization.k8s.io/v1', + 'kind: ClusterRoleBinding', + 'metadata:', + ' name: ', + 'roleRef:', + ' apiGroup: rbac.authorization.k8s.io', + ' kind: ClusterRole', + ' name: # e.g. view, edit, admin', + 'subjects:', + ' - kind: Group', + ' name: ', + ' apiGroup: rbac.authorization.k8s.io', + ] + if (groups.length > 0) { + lines.push('# Your groups:', ...groups.map((g) => `# ${g}`)) + } + return lines.join('\n') +} From 7da7e0b8c2e807f69a92b094c5cfade70db34a3b Mon Sep 17 00:00:00 2001 From: Arlen Vasconcelos Date: Fri, 25 Sep 2026 14:25:32 -0400 Subject: [PATCH 03/16] Drop the RBAC snippet from the no-access banner 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. --- web/src/components/NoClusterAccessBanner.tsx | 91 +++++--------------- web/src/components/noClusterAccess.test.ts | 16 +--- web/src/components/noClusterAccess.ts | 24 ------ 3 files changed, 22 insertions(+), 109 deletions(-) diff --git a/web/src/components/NoClusterAccessBanner.tsx b/web/src/components/NoClusterAccessBanner.tsx index 9ffcd9c2f5..4b2b8c96ec 100644 --- a/web/src/components/NoClusterAccessBanner.tsx +++ b/web/src/components/NoClusterAccessBanner.tsx @@ -1,16 +1,13 @@ import { useEffect, useState } from 'react' -import { Check, Copy, ShieldAlert } from 'lucide-react' +import { ShieldAlert } from 'lucide-react' import { AlertBanner, Collapse, CollapseChevron, useDisclosure } from '@skyhook-io/k8s-ui' import { useAuthMe } from '../api/client' -import { bindableGroups, buildNoAccessBinding } from './noClusterAccess' +import { bindableGroups } from './noClusterAccess' // Every read is filtered to the user's namespaces, so someone bound to none // sees empty lists in every view and reads it as an empty cluster. Stated in // terms of the user's own access, not how the cluster is meant to be set up: // Radar can't know whether an org intends hub roles or IdP groups here. -const toggleClass = - 'flex items-center gap-1.5 text-xs text-theme-text-secondary hover:text-theme-text-primary transition-colors' - export function NoClusterAccessBanner() { const { data: me, refetch } = useAuthMe() // auth/me is first fetched before the cluster connects, when the server @@ -21,25 +18,10 @@ export function NoClusterAccessBanner() { }, [refetch]) const [groupsOpen, setGroupsOpen] = useState(false) const groupsDisclosure = useDisclosure(groupsOpen) - const [expanded, setExpanded] = useState(false) - const disclosure = useDisclosure(expanded) - const [copied, setCopied] = useState(false) if (!me?.noNamespaceAccess) return null const groups = bindableGroups(me.groups ?? []) - const snippet = buildNoAccessBinding(groups) - - const copy = () => { - navigator.clipboard.writeText(snippet).then( - () => { - setCopied(true) - setTimeout(() => setCopied(false), 2000) - }, - () => {}, - ) - } - return (
} > -
-
- {groups.length > 0 && ( - - )} + {groups.length > 0 && ( +
-
- -
- {groups.map((g) => ( - - {g} - - ))} -
- - -
-

- Ask a cluster admin to bind one of your groups to a role. Fill in the placeholders - before applying; as written, this grants nothing. -

-
-
- ClusterRoleBinding - -
- 
{snippet}
+ {g} + + ))}
-
- -
+ +
+ )}
) diff --git a/web/src/components/noClusterAccess.test.ts b/web/src/components/noClusterAccess.test.ts index b971d7bd02..44d3ee7b36 100644 --- a/web/src/components/noClusterAccess.test.ts +++ b/web/src/components/noClusterAccess.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { bindableGroups, buildNoAccessBinding } from './noClusterAccess' +import { bindableGroups } from './noClusterAccess' describe('bindableGroups', () => { it('keeps only person-identifying Radar Cloud groups', () => { @@ -19,17 +19,3 @@ describe('bindableGroups', () => { expect(bindableGroups(['oidc:platform', 'devs'])).toEqual(['oidc:platform', 'devs']) }) }) - -describe('buildNoAccessBinding', () => { - it('grants nothing as-is and lists the groups as comments', () => { - const yaml = buildNoAccessBinding(['radar:idp:team-a', 'radar:idp:everyone']) - expect(yaml).toContain(' name: ') - expect(yaml).toContain(' name: ') - expect(yaml).toContain('# radar:idp:everyone') - expect(yaml).not.toMatch(/^\s+name: "?radar:idp/m) - }) - - it('omits the group list when there are none', () => { - expect(buildNoAccessBinding([])).not.toContain('# Your groups') - }) -}) diff --git a/web/src/components/noClusterAccess.ts b/web/src/components/noClusterAccess.ts index c4b8648248..75ba1d4be4 100644 --- a/web/src/components/noClusterAccess.ts +++ b/web/src/components/noClusterAccess.ts @@ -9,27 +9,3 @@ export function bindableGroups(groups: readonly string[]): string[] { (g) => g.startsWith('radar:idp:') || g.startsWith('radar:user:') || g.startsWith('radar:email:'), ) } - -// Applies as-is without granting anything: the subject and role are -// placeholders. Choosing which group gets which role is the admin's call; a -// pre-filled first group could be an org-wide one. -export function buildNoAccessBinding(groups: readonly string[]): string { - const lines = [ - 'apiVersion: rbac.authorization.k8s.io/v1', - 'kind: ClusterRoleBinding', - 'metadata:', - ' name: ', - 'roleRef:', - ' apiGroup: rbac.authorization.k8s.io', - ' kind: ClusterRole', - ' name: # e.g. view, edit, admin', - 'subjects:', - ' - kind: Group', - ' name: ', - ' apiGroup: rbac.authorization.k8s.io', - ] - if (groups.length > 0) { - lines.push('# Your groups:', ...groups.map((g) => `# ${g}`)) - } - return lines.join('\n') -} From 00e502d109f6233f9dc147ac30ec946265f74a64 Mon Sep 17 00:00:00 2001 From: Arlen Vasconcelos Date: Sun, 27 Sep 2026 00:13:26 -0400 Subject: [PATCH 04/16] chart: bind only radar:system for hub background identities 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 --- deploy/helm/radar/templates/cloud-rbac-cluster-read.yaml | 3 --- deploy/helm/radar/templates/cloud-rbac-system.yaml | 6 ------ deploy/helm/radar/tests/cloud_rbac_system_test.yaml | 5 +---- 3 files changed, 1 insertion(+), 13 deletions(-) diff --git a/deploy/helm/radar/templates/cloud-rbac-cluster-read.yaml b/deploy/helm/radar/templates/cloud-rbac-cluster-read.yaml index da65fd9384..31fa547761 100644 --- a/deploy/helm/radar/templates/cloud-rbac-cluster-read.yaml +++ b/deploy/helm/radar/templates/cloud-rbac-cluster-read.yaml @@ -190,8 +190,5 @@ subjects: - kind: Group name: radar:system apiGroup: rbac.authorization.k8s.io - - kind: Group - name: cloud:system - apiGroup: rbac.authorization.k8s.io {{- end }} {{- end -}} diff --git a/deploy/helm/radar/templates/cloud-rbac-system.yaml b/deploy/helm/radar/templates/cloud-rbac-system.yaml index ffc2d893b7..137b22621a 100644 --- a/deploy/helm/radar/templates/cloud-rbac-system.yaml +++ b/deploy/helm/radar/templates/cloud-rbac-system.yaml @@ -29,9 +29,6 @@ subjects: - kind: Group name: radar:system apiGroup: rbac.authorization.k8s.io - - kind: Group - name: cloud:system - apiGroup: rbac.authorization.k8s.io --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole @@ -58,7 +55,4 @@ subjects: - kind: Group name: radar:system apiGroup: rbac.authorization.k8s.io - - kind: Group - name: cloud:system - apiGroup: rbac.authorization.k8s.io {{- end -}} diff --git a/deploy/helm/radar/tests/cloud_rbac_system_test.yaml b/deploy/helm/radar/tests/cloud_rbac_system_test.yaml index 3d83cb8c78..2c97aace6e 100644 --- a/deploy/helm/radar/tests/cloud_rbac_system_test.yaml +++ b/deploy/helm/radar/tests/cloud_rbac_system_test.yaml @@ -65,7 +65,7 @@ tests: resources: ["secrets"] verbs: ["get", "list", "watch"] - - it: names both radar:system and the legacy cloud:system group + - it: binds only radar:system set: cloud: enabled: true @@ -86,9 +86,6 @@ tests: - kind: Group name: radar:system apiGroup: rbac.authorization.k8s.io - - kind: Group - name: cloud:system - apiGroup: rbac.authorization.k8s.io - it: stays on when systemRbac is absent from reused values set: From 5b3fea74323de583f30f6d6ec99ba9648d64693a Mon Sep 17 00:00:00 2001 From: Arlen Vasconcelos Date: Sun, 27 Sep 2026 00:27:56 -0400 Subject: [PATCH 05/16] Show every group in the no-access banner 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. --- web/src/components/NoClusterAccessBanner.tsx | 3 +-- web/src/components/noClusterAccess.test.ts | 21 -------------------- web/src/components/noClusterAccess.ts | 11 ---------- 3 files changed, 1 insertion(+), 34 deletions(-) delete mode 100644 web/src/components/noClusterAccess.test.ts delete mode 100644 web/src/components/noClusterAccess.ts diff --git a/web/src/components/NoClusterAccessBanner.tsx b/web/src/components/NoClusterAccessBanner.tsx index 4b2b8c96ec..28912eea0d 100644 --- a/web/src/components/NoClusterAccessBanner.tsx +++ b/web/src/components/NoClusterAccessBanner.tsx @@ -2,7 +2,6 @@ import { useEffect, useState } from 'react' import { ShieldAlert } from 'lucide-react' import { AlertBanner, Collapse, CollapseChevron, useDisclosure } from '@skyhook-io/k8s-ui' import { useAuthMe } from '../api/client' -import { bindableGroups } from './noClusterAccess' // Every read is filtered to the user's namespaces, so someone bound to none // sees empty lists in every view and reads it as an empty cluster. Stated in @@ -21,7 +20,7 @@ export function NoClusterAccessBanner() { if (!me?.noNamespaceAccess) return null - const groups = bindableGroups(me.groups ?? []) + const groups = me.groups ?? [] return (
{ - it('keeps only person-identifying Radar Cloud groups', () => { - expect( - bindableGroups([ - 'radar:viewer', - 'radar:org:o1', - 'radar:user:u1', - 'radar:email:bob@example.com', - 'radar:idp:team-a', - 'cloud:viewer', - ]), - ).toEqual(['radar:user:u1', 'radar:email:bob@example.com', 'radar:idp:team-a']) - }) - - it('keeps every group outside Cloud', () => { - expect(bindableGroups(['oidc:platform', 'devs'])).toEqual(['oidc:platform', 'devs']) - }) -}) diff --git a/web/src/components/noClusterAccess.ts b/web/src/components/noClusterAccess.ts deleted file mode 100644 index 75ba1d4be4..0000000000 --- a/web/src/components/noClusterAccess.ts +++ /dev/null @@ -1,11 +0,0 @@ -// Groups worth offering an admin to bind. Radar Cloud forwards its own -// vocabulary: only the IdP, user and email groups identify this person; -// radar: and radar:org: would grant every member of the org, and -// cloud:* are legacy duplicates. Outside Cloud every group is the operator's. -export function bindableGroups(groups: readonly string[]): string[] { - const radar = groups.filter((g) => g.startsWith('radar:') || g.startsWith('cloud:')) - if (radar.length === 0) return [...groups] - return groups.filter( - (g) => g.startsWith('radar:idp:') || g.startsWith('radar:user:') || g.startsWith('radar:email:'), - ) -} From e3622c53833e6ca78848a5beb0a8085f340a7d80 Mon Sep 17 00:00:00 2001 From: Arlen Vasconcelos Date: Sun, 27 Sep 2026 00:52:19 -0400 Subject: [PATCH 06/16] Leave the RBAC snippet's subject to the admin 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. --- .../components/resources/ResourcesView.tsx | 4 -- .../components/ui/RestrictedState.test.tsx | 17 -------- .../src/components/ui/RestrictedState.tsx | 39 ++++--------------- .../components/resources/ResourcesView.tsx | 8 +--- 4 files changed, 9 insertions(+), 59 deletions(-) delete mode 100644 packages/k8s-ui/src/components/ui/RestrictedState.test.tsx diff --git a/packages/k8s-ui/src/components/resources/ResourcesView.tsx b/packages/k8s-ui/src/components/resources/ResourcesView.tsx index 6865967de8..658c6022b4 100644 --- a/packages/k8s-ui/src/components/resources/ResourcesView.tsx +++ b/packages/k8s-ui/src/components/resources/ResourcesView.tsx @@ -3376,8 +3376,6 @@ interface ResourcesViewProps { /** Per-kind reason a forbidden kind is hidden ("rbac_denied" | "unavailable"), * keyed by the same count key as resourceForbidden. Drives RestrictedState copy. */ resourceReasons?: Record - /** Groups the caller can be granted access through (see RestrictedState). */ - rbacSubjects?: string[] resourceUnavailable?: string[] // Single query for the currently selected kind's full data selectedKindQuery?: ResourceQueryResult @@ -3634,7 +3632,6 @@ export function ResourcesView({ resourceCounts: resourceCountsProp, resourceForbidden: resourceForbiddenProp, resourceReasons, - rbacSubjects, resourceUnavailable: resourceUnavailableProp, selectedKindQuery: selectedKindQueryProp, connectionState, @@ -6375,7 +6372,6 @@ export function ResourcesView({ group={selectedKind.group} resource={selectedKind.name} reason={resourceReasons?.[selectedKindCountKey]} - subjects={rbacSubjects} />
diff --git a/packages/k8s-ui/src/components/ui/RestrictedState.test.tsx b/packages/k8s-ui/src/components/ui/RestrictedState.test.tsx deleted file mode 100644 index 2c1e61a416..0000000000 --- a/packages/k8s-ui/src/components/ui/RestrictedState.test.tsx +++ /dev/null @@ -1,17 +0,0 @@ -import { describe, expect, it } from 'vitest' -import { buildRbacRequest } from './RestrictedState' - -describe('buildRbacRequest', () => { - it('keeps the placeholder subject when the caller has no bindable groups', () => { - const yaml = buildRbacRequest('', 'nodes', []) - expect(yaml).toContain('name: ') - }) - - it('binds only the first group and lists the rest as commented alternatives', () => { - const yaml = buildRbacRequest('', 'nodes', ['radar:idp:team-a', 'radar:idp:everyone']) - expect(yaml).toContain(' name: "radar:idp:team-a"') - expect(yaml).toContain(' # or: name: "radar:idp:everyone"') - expect(yaml).not.toMatch(/^ {4}name: "radar:idp:everyone"$/m) - expect(yaml).not.toContain('') - }) -}) diff --git a/packages/k8s-ui/src/components/ui/RestrictedState.tsx b/packages/k8s-ui/src/components/ui/RestrictedState.tsx index 574f90fe00..8083c05c39 100644 --- a/packages/k8s-ui/src/components/ui/RestrictedState.tsx +++ b/packages/k8s-ui/src/components/ui/RestrictedState.tsx @@ -25,36 +25,12 @@ interface Props { * ServiceAccount can't read it at all (not installed / SA RBAC / feature off) * — a user grant won't help, so show a different message and no snippet. */ reason?: 'rbac_denied' | 'unavailable' | string - /** Kubernetes groups the caller carries that an admin can bind, e.g. the - * `radar:idp:` groups Radar Cloud forwards. The first becomes the - * snippet's subject; the rest are listed as commented alternatives. */ - subjects?: string[] /** Tightens spacing for inline/embedded use (topology overlay, etc.). */ compact?: boolean className?: string } -// Only the first group is bound: a session's groups often include an org-wide -// one ("Everyone"), and granting that by copy-paste would widen access for the -// whole org. The admin picks from the commented alternatives. -function subjectLines(subjects: string[]): string[] { - if (subjects.length === 0) { - return [ - ` - kind: Group # or User / ServiceAccount`, - ` name: `, - ` apiGroup: rbac.authorization.k8s.io`, - ] - } - const [first, ...rest] = subjects - return [ - ` - kind: Group`, - ` name: "${first}"`, - ` apiGroup: rbac.authorization.k8s.io`, - ...rest.map((g) => ` # or: name: "${g}"`), - ] -} - -export function buildRbacRequest(group: string, resource: string, subjects: string[]): string { +function buildRbacRequest(group: string, resource: string): string { // resource is the placeholder when we don't have a confident API plural // (e.g. a CRD deep-link before discovery resolves the kind) — use a generic // role name rather than radar-read-. @@ -78,11 +54,13 @@ export function buildRbacRequest(group: string, resource: string, subjects: stri ` kind: ClusterRole`, ` name: ${roleName}`, `subjects:`, - ...subjectLines(subjects), + ` - kind: Group # or User / ServiceAccount`, + ` name: `, + ` apiGroup: rbac.authorization.k8s.io`, ].join('\n') } -export function RestrictedState({ kindLabel, group = '', resource, reason, subjects = [], compact, className }: Props) { +export function RestrictedState({ kindLabel, group = '', resource, reason, compact, className }: Props) { const [expanded, setExpanded] = useState(false) const accessDisclosure = useDisclosure(expanded) const [copied, setCopied] = useState(false) @@ -94,7 +72,7 @@ export function RestrictedState({ kindLabel, group = '', resource, reason, subje // produce a snippet that doesn't grant access. Only inline the resource when // it looks like a valid resource name; otherwise leave a clear placeholder. const validResource = resource && /^[a-z0-9.-]+$/.test(resource) ? resource : '' - const snippet = buildRbacRequest(group, validResource, subjects) + const snippet = buildRbacRequest(group, validResource) const copy = () => { navigator.clipboard.writeText(snippet).then( @@ -147,9 +125,8 @@ export function RestrictedState({ kindLabel, group = '', resource, reason, subje

- {subjects.length > 0 - ? 'Apply this, or send it to whoever administers your cluster. It grants read access to one of your identity provider groups.' - : 'Apply this, or send it to whoever administers your cluster, to grant your identity read access.'} + Apply this, or send it to whoever administers your cluster, to grant your identity + read access.

diff --git a/web/src/components/resources/ResourcesView.tsx b/web/src/components/resources/ResourcesView.tsx index df20747243..2aa61b915c 100644 --- a/web/src/components/resources/ResourcesView.tsx +++ b/web/src/components/resources/ResourcesView.tsx @@ -1,7 +1,7 @@ import { useState, useMemo, useCallback, useEffect } from 'react' import { useLocation, useNavigate } from 'react-router-dom' import { useQuery } from '@tanstack/react-query' -import { ApiError, debugNamespaceLog, fetchJSON, isForbiddenError, isKindSyncFailed, isStillLoadingError, useCapabilities, useNamespaceCapabilities, useResources, useSecretCertExpiry, useTopPodMetrics, useTopNodeMetrics, useBulkDeleteResources, useBulkRestartWorkloads, useBulkScaleWorkloads, useAudit, useAuthMe } from '../../api/client' +import { ApiError, debugNamespaceLog, fetchJSON, isForbiddenError, isKindSyncFailed, isStillLoadingError, useCapabilities, useNamespaceCapabilities, useResources, useSecretCertExpiry, useTopPodMetrics, useTopNodeMetrics, useBulkDeleteResources, useBulkRestartWorkloads, useBulkScaleWorkloads, useAudit } from '../../api/client' import { isBadgeWorthy } from '../../utils/auditBadges' import type { AuditBadgeMessage } from '@skyhook-io/k8s-ui' import { apiUrl, getAuthHeaders, getCredentialsMode, stripBasename } from '../../api/config' @@ -125,11 +125,6 @@ export function ResourcesView({ namespaces, selectedResource, onResourceClick, o // Lightweight resource counts for sidebar badges (~2KB instead of ~608MB) const namespacesParam = namespaces.join(',') - const { data: authMe } = useAuthMe() - const idpGroups = useMemo( - () => (authMe?.groups ?? []).filter((g) => g.startsWith('radar:idp:')), - [authMe?.groups], - ) const { data: countsData, isError: countsIsError } = useQuery({ queryKey: ['resource-counts', namespacesParam], queryFn: async () => { @@ -438,7 +433,6 @@ export function ResourcesView({ namespaces, selectedResource, onResourceClick, o resourceCounts={countsData?.counts ?? EMPTY_RESOURCE_COUNTS} resourceForbidden={countsData?.forbidden} resourceReasons={countsData?.reasons} - rbacSubjects={idpGroups} resourceUnavailable={countsData?.unavailable} selectedKindQuery={selectedKindQueryResult} printerTable={selectedKindQueryBlocked ? null : selectedKindQuery.data?.printerTable ?? null} From acbdbad4153d72d2637b0275d2adfcfeea04a675 Mon Sep 17 00:00:00 2001 From: Arlen Vasconcelos Date: Mon, 28 Sep 2026 22:10:58 -0400 Subject: [PATCH 07/16] chart: radar:system is one all-or-nothing grant, and Cloud Helm no longer 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 Claude-Session: https://claude.ai/code/session_017J25KHG5W4DzS32bU1J8ou --- deploy/helm/radar/README.md | 29 ++++++--- deploy/helm/radar/templates/_helpers.tpl | 12 ++-- .../cloud-rbac-integration-read.yaml | 2 + .../radar/templates/cloud-rbac-system.yaml | 14 ++-- deploy/helm/radar/templates/clusterrole.yaml | 5 +- .../radar/tests/cloud_rbac_system_test.yaml | 43 +++++++++++- .../radar/tests/helm_write_rbac_test.yaml | 65 +++++++++++++++++++ deploy/helm/radar/values.yaml | 21 ++++-- .../k8s/integration_read_baseline_test.go | 10 +-- 9 files changed, 166 insertions(+), 35 deletions(-) create mode 100644 deploy/helm/radar/tests/helm_write_rbac_test.yaml diff --git a/deploy/helm/radar/README.md b/deploy/helm/radar/README.md index 24bb4f927e..7fab6f4c7d 100644 --- a/deploy/helm/radar/README.md +++ b/deploy/helm/radar/README.md @@ -163,15 +163,24 @@ before upgrading if the added visibility is unwanted. ### Radar Cloud background identities (`radar:system`) -The hub's alerts worker and timeline puller call Radar as the `radar:system` -group, not as a user. `cloud.systemRbac` (default `true`) binds that group to a -read-only set: `view`, the cluster-read and integration-read add-ons above, and -`get/list/watch` on Secrets, which Helm release alerts need because Helm stores -each release as a Secret. It is independent of `cloud.defaultRbac`, so turning -the role bindings off (`cloud.defaultRbac.create=false`, for example when IdP -groups decide cluster access) leaves alerts and the hub timeline working. -Setting `cloud.systemRbac=false` stops both unless you bind `radar:system` -yourself. +The hub's alerts worker, timeline puller and AI diagnose agent call Radar +as the `radar:system` group, not as a user. `cloud.systemRbac` (default `true`) +binds that group to a read-only set: `view`, the cluster-read and +integration-read add-ons above, and `get/list/watch` on Secrets. Secret read is +what Helm release alerts and Secret changes in the hub timeline need, because +Helm stores each release as a Secret. + +It is independent of `cloud.defaultRbac`, so turning the role bindings off +(`cloud.defaultRbac.create=false`, for example when IdP groups decide cluster +access) leaves alerts and the hub timeline working. It is all or nothing: +`cloud.systemRbac=false` removes the whole grant, and alerts, the hub timeline +and AI diagnose then see nothing on clusters without the role bindings unless +you bind `radar:system` yourself. Change it through this value: a binding +edited or deleted with `kubectl` comes back on the next upgrade. + +An upgrade with `--reuse-values` from a release that predates the key leaves it +off, so an existing install never gains Secret read without choosing it. Set +`cloud.systemRbac=true` on those installs. ### Connecting to Argo CD (GitOps deep diff) @@ -359,7 +368,7 @@ Disabled by default for security: | Terminal | `rbac.podExec: true` | Shell access to pods | | Port Forward | `rbac.portForward: true` | Port forwarding to pods. Also the fallback for traffic sources (Hubble/Caretta) — Radar dials the relay/metrics Service directly first, so in-cluster installs only need this when a NetworkPolicy or routing blocks Radar's namespace from reaching the service | | Logs | `rbac.podLogs: true` | View pod logs (**enabled by default**) | -| Helm Write | `rbac.helm: true` | Install/upgrade/rollback/uninstall Helm releases. Under auth or cloud-mode, also emits a split helm add-on ClusterRole — `radar-helm` (member-safe: CRDs, storage, namespaces) and `radar-helm-admin` (owner-only: RBAC, webhooks, ApiServices) | +| Helm Write | `rbac.helm: true` | Install/upgrade/rollback/uninstall Helm releases. No-auth installs: grants Radar's ServiceAccount create/update/patch/delete on all resource types, since Helm runs as it. Under auth or cloud-mode Helm runs as the signed-in user, so the ServiceAccount gets no write grant; instead a split helm add-on ClusterRole is emitted — `radar-helm` (member-safe: CRDs, storage, namespaces) and `radar-helm-admin` (owner-only: RBAC, webhooks, ApiServices) | | RBAC view | `rbac.viewRBAC: true` | Show ClusterRoles, ClusterRoleBindings, Roles, RoleBindings in the resource browser. Off by default — cache-served reads bypass per-user RBAC, so this exposes the cluster's authorization graph to every authenticated Radar user. Auto-enabled under auth or cloud mode (every read is re-checked per user there). | | Webhooks view | `rbac.viewWebhooks: true` | Show MutatingWebhookConfigurations and ValidatingWebhookConfigurations in the resource browser. Off by default — the configurations reveal which admission controls are enforced (Gatekeeper / Kyverno policies, image scanners, DLP) and where the gaps are, which is recon value for a low-trust viewer. Auto-enabled under auth or cloud mode. | | Node runtime evidence | `rbac.viewNodeRuntime: true` | Let upgrade-impact checks inspect kubelet metrics and effective configuration through `nodes/proxy`. Off by default because this exposes node-level runtime and configuration details to anyone who can reach a no-auth Radar install. Under auth, grant `get` on `nodes/proxy` to each Kubernetes identity that should inspect this evidence. | diff --git a/deploy/helm/radar/templates/_helpers.tpl b/deploy/helm/radar/templates/_helpers.tpl index 52d8a6e6f8..9d8f882806 100644 --- a/deploy/helm/radar/templates/_helpers.tpl +++ b/deploy/helm/radar/templates/_helpers.tpl @@ -73,13 +73,15 @@ Create the name of the service account to use {{- end -}} {{/* -Whether the always-on read-only binding for Radar Cloud's own background -identities (radar:system) renders. An absent value means on: `--reuse-values` -upgrades render with the previous release's tree, which predates the key, and -the hub's alerts and timeline depend on the binding existing. +Whether the read-only binding for Radar Cloud's own background identities +(radar:system) renders. It includes cluster-wide Secret read, so an absent +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 +without someone choosing it. Fresh installs, plain upgrades, GitOps and +`--reset-then-reuse-values` get true from values.yaml. */}} {{- define "radar.cloudSystemRbac" -}} -{{- if and .Values.cloud.enabled (ne (toString .Values.cloud.systemRbac) "false") -}} +{{- if and .Values.cloud.enabled (eq (toString .Values.cloud.systemRbac) "true") -}} true {{- else -}} false diff --git a/deploy/helm/radar/templates/cloud-rbac-integration-read.yaml b/deploy/helm/radar/templates/cloud-rbac-integration-read.yaml index f53fb94fe1..78eead75d8 100644 --- a/deploy/helm/radar/templates/cloud-rbac-integration-read.yaml +++ b/deploy/helm/radar/templates/cloud-rbac-integration-read.yaml @@ -49,6 +49,7 @@ subjects: - kind: Group name: radar:{{ $tier }} apiGroup: rbac.authorization.k8s.io +{{- if ne $tier "system" }} - kind: Group name: cloud:{{ $tier }} apiGroup: rbac.authorization.k8s.io @@ -56,3 +57,4 @@ subjects: {{- end }} {{- end }} {{- end }} +{{- end }} diff --git a/deploy/helm/radar/templates/cloud-rbac-system.yaml b/deploy/helm/radar/templates/cloud-rbac-system.yaml index 137b22621a..825c316233 100644 --- a/deploy/helm/radar/templates/cloud-rbac-system.yaml +++ b/deploy/helm/radar/templates/cloud-rbac-system.yaml @@ -1,18 +1,20 @@ {{- /* Read-only access for Radar Cloud's own background identities. -The hub's alerts worker and timeline puller have no user or IdP groups; they -assert the radar:system group. Without a binding of their own they would ride +The hub's background identities have no user or IdP groups; they assert the +radar:system group. That is the alerts worker, the timeline puller and the +AI diagnose agent. Without a binding of their own they would ride on radar:owner, which disappears when an org turns the role bindings off (cloud.defaultRbac.create=false) to let IdP groups decide cluster access — silently stopping alerts and the hub timeline. The grant is the viewer set (view here, plus the cluster-read and integration-read add-ons, which bind radar:system in their own templates) and -Secret read. Secret read is what Helm release alerts need: Radar lists Helm -releases as the caller, and Helm stores each release as a Secret. Kubernetes -RBAC cannot narrow that to Helm's Secrets, so it is all Secrets, read-only. -No writes. +Secret read. Secret read is what Helm release alerts and Secret changes in the +hub timeline need: Radar lists Helm releases as the caller, and Helm stores +each release as a Secret. Kubernetes RBAC cannot narrow that to Helm's +Secrets, so it is all Secrets, read-only. No writes. All or nothing: +cloud.systemRbac=false removes the whole grant. */}} {{- if and (include "radar.cloudSystemRbac" . | eq "true") .Values.rbac.create -}} apiVersion: rbac.authorization.k8s.io/v1 diff --git a/deploy/helm/radar/templates/clusterrole.yaml b/deploy/helm/radar/templates/clusterrole.yaml index 583f784f43..d904e9fd5d 100644 --- a/deploy/helm/radar/templates/clusterrole.yaml +++ b/deploy/helm/radar/templates/clusterrole.yaml @@ -59,10 +59,13 @@ rules: - podsecuritypolicies verbs: ["list"] - {{- if .Values.rbac.helm }} + {{- if and .Values.rbac.helm (eq .Values.auth.mode "none") (not .Values.cloud.enabled) }} # Helm write operations require broad permissions because charts can create # any Kubernetes resource type. Read access for common resources is granted # by other rules in this ClusterRole. Secrets read access is auto-enabled below. + # No-auth installs only: under auth or cloud mode every Helm action runs as the + # impersonated user, so the ServiceAccount never needs it there. The per-tier + # add-ons (clusterrole-helm*.yaml) are what users get instead. - apiGroups: ["*"] resources: ["*"] verbs: ["create", "update", "patch", "delete"] diff --git a/deploy/helm/radar/tests/cloud_rbac_system_test.yaml b/deploy/helm/radar/tests/cloud_rbac_system_test.yaml index 2c97aace6e..b3a25a995e 100644 --- a/deploy/helm/radar/tests/cloud_rbac_system_test.yaml +++ b/deploy/helm/radar/tests/cloud_rbac_system_test.yaml @@ -65,6 +65,25 @@ tests: resources: ["secrets"] verbs: ["get", "list", "watch"] + - it: binds Secret read to radar:system + set: + cloud: + enabled: true + token: dummy + url: "wss://cloud.example" + clusterName: test-cluster + template: cloud-rbac-system.yaml + documentSelector: + path: metadata.name + value: RELEASE-NAME-radar-cloud-system-secrets-read + asserts: + - equal: + path: subjects + value: + - kind: Group + name: radar:system + apiGroup: rbac.authorization.k8s.io + - it: binds only radar:system set: cloud: @@ -87,7 +106,10 @@ tests: name: radar:system apiGroup: rbac.authorization.k8s.io - - it: stays on when systemRbac is absent from reused values + # --reuse-values from a release predating systemRbac renders with the key + # absent: nothing renders, so an existing install never gains Secret read + # without choosing it. + - it: renders nothing when systemRbac is absent from reused values set: cloud: enabled: true @@ -96,6 +118,18 @@ tests: clusterName: test-cluster systemRbac: null template: cloud-rbac-system.yaml + asserts: + - hasDocuments: + count: 0 + + - it: renders the view binding and Secret read by default + set: + cloud: + enabled: true + token: dummy + url: "wss://cloud.example" + clusterName: test-cluster + template: cloud-rbac-system.yaml asserts: - hasDocuments: count: 3 @@ -148,5 +182,8 @@ tests: value: RELEASE-NAME-radar-cloud-system-integration-read-namespaced asserts: - equal: - path: subjects[0].name - value: radar:system + path: subjects + value: + - kind: Group + name: radar:system + apiGroup: rbac.authorization.k8s.io diff --git a/deploy/helm/radar/tests/helm_write_rbac_test.yaml b/deploy/helm/radar/tests/helm_write_rbac_test.yaml new file mode 100644 index 0000000000..e8766819e7 --- /dev/null +++ b/deploy/helm/radar/tests/helm_write_rbac_test.yaml @@ -0,0 +1,65 @@ +suite: Helm write RBAC for the ServiceAccount +templates: + - clusterrole.yaml +tests: + # Helm runs as the ServiceAccount only when nobody is signed in. + - it: grants the ServiceAccount write on every resource in no-auth installs + set: + rbac: + helm: true + auth: + mode: none + asserts: + - contains: + path: rules + content: + apiGroups: ["*"] + resources: ["*"] + verbs: ["create", "update", "patch", "delete"] + + # Under cloud mode Helm runs as the impersonated user; the wizard sets + # rbac.helm=true, which must not hand the ServiceAccount cluster-wide write. + - it: grants the ServiceAccount no write in cloud mode + set: + rbac: + helm: true + cloud: + enabled: true + token: dummy + url: "wss://cloud.example" + clusterName: test-cluster + asserts: + - notContains: + path: rules + content: + apiGroups: ["*"] + resources: ["*"] + verbs: ["create", "update", "patch", "delete"] + + - it: grants the ServiceAccount no write under auth + set: + rbac: + helm: true + auth: + mode: proxy + asserts: + - notContains: + path: rules + content: + apiGroups: ["*"] + resources: ["*"] + verbs: ["create", "update", "patch", "delete"] + + - it: grants the ServiceAccount no write without rbac.helm + set: + rbac: + helm: false + auth: + mode: none + asserts: + - notContains: + path: rules + content: + apiGroups: ["*"] + resources: ["*"] + verbs: ["create", "update", "patch", "delete"] diff --git a/deploy/helm/radar/values.yaml b/deploy/helm/radar/values.yaml index 086dd9bb52..1fb4c9f669 100644 --- a/deploy/helm/radar/values.yaml +++ b/deploy/helm/radar/values.yaml @@ -75,8 +75,11 @@ rbac: # Enable these based on features you need: # Allow Helm write operations (install, upgrade, rollback, uninstall, apply values) - # Requires broad permissions - grants create/update/patch/delete on all resource types, - # since Helm charts can create any kind of Kubernetes resource. + # No-auth installs: grants Radar's ServiceAccount create/update/patch/delete on all + # resource types, since Helm charts can create any kind of Kubernetes resource + # and Helm runs as that ServiceAccount there. + # Auth or cloud mode: Helm runs as the signed-in user, so the ServiceAccount gets no + # write grant; this only adds the Helm add-on roles for the Cloud owner/member tiers. # Automatically enables secrets read access (needed for Helm release storage). helm: false @@ -518,12 +521,18 @@ cloud: # owner later activates it. See templates/cloud-rbac-node-ops.yaml. nodeOps: false # Read-only binding for Radar Cloud's own background identities (the hub's - # alerts worker and timeline puller), which assert the radar:system group. + # alerts worker, timeline puller and AI diagnose agent), which assert the + # radar:system group. Grants the viewer set (view + cluster-read + + # integration-read) and cluster-wide Secret read, which Helm release alerts + # and Secret changes in the hub timeline need (Helm stores each release as a + # Secret). No writes. See templates/cloud-rbac-system.yaml. # Independent of defaultRbac: it stays on when the role bindings above are # off, so alerts and the hub timeline keep working when IdP groups decide - # cluster access. Grants the viewer set (view + cluster-read + - # integration-read) plus Secret read, which Helm release alerts need (Helm - # stores releases as Secrets). No writes. See templates/cloud-rbac-system.yaml. + # cluster access. All or nothing: false removes the whole grant, and then + # alerts, the hub timeline and AI diagnose see nothing on clusters without + # the role bindings unless you bind radar:system yourself. + # An upgrade with --reuse-values from a release that predates this key leaves + # it OFF, so an existing install never gains Secret read without choosing it. systemRbac: true # MCP (Model Context Protocol) server for AI tools diff --git a/internal/k8s/integration_read_baseline_test.go b/internal/k8s/integration_read_baseline_test.go index 351863470c..40802c7b2f 100644 --- a/internal/k8s/integration_read_baseline_test.go +++ b/internal/k8s/integration_read_baseline_test.go @@ -227,10 +227,12 @@ func TestIntegrationReadBindings(t *testing.T) { continue } found = true - if doc.RoleRef != (rbacv1.RoleRef{APIGroup: rbacv1.GroupName, Kind: "ClusterRole", Name: roleName}) || !reflect.DeepEqual(doc.Subjects, []rbacv1.Subject{ - {Kind: "Group", Name: "radar:" + tier, APIGroup: rbacv1.GroupName}, - {Kind: "Group", Name: "cloud:" + tier, APIGroup: rbacv1.GroupName}, - }) { + wantSubjects := []rbacv1.Subject{{Kind: "Group", Name: "radar:" + tier, APIGroup: rbacv1.GroupName}} + // radar:system has no legacy cloud:* twin. + if tier != "system" { + wantSubjects = append(wantSubjects, rbacv1.Subject{Kind: "Group", Name: "cloud:" + tier, APIGroup: rbacv1.GroupName}) + } + if doc.RoleRef != (rbacv1.RoleRef{APIGroup: rbacv1.GroupName, Kind: "ClusterRole", Name: roleName}) || !reflect.DeepEqual(doc.Subjects, wantSubjects) { t.Errorf("incorrect binding: %+v", doc) } } From 55fc3efa39a73df84fad68865733771f35fe2e5f Mon Sep 17 00:00:00 2001 From: Arlen Vasconcelos Date: Mon, 28 Sep 2026 22:11:10 -0400 Subject: [PATCH 08/16] Let Helm writes check the signed-in user, not Radar's ServiceAccount 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 Claude-Session: https://claude.ai/code/session_017J25KHG5W4DzS32bU1J8ou --- internal/helm/client.go | 31 +++-- internal/helm/handlers.go | 118 ++++++++++++------ internal/helm/handlers_test.go | 20 +++ internal/helm/helm_write_gate_test.go | 85 +++++++++++++ internal/helm/rollback_impersonation_test.go | 41 ++++++ internal/k8s/capabilities.go | 8 ++ internal/server/capabilities_test.go | 16 +++ internal/server/server.go | 1 + web/src/api/client.ts | 30 +++-- web/src/components/helm/ChartBrowser.tsx | 13 +- web/src/components/helm/HelmReleaseDrawer.tsx | 10 +- web/src/components/helm/InstallWizard.tsx | 5 +- web/src/components/helm/ValuesViewer.tsx | 2 +- web/src/contexts/CapabilitiesContext.tsx | 7 +- 14 files changed, 303 insertions(+), 84 deletions(-) create mode 100644 internal/helm/helm_write_gate_test.go create mode 100644 internal/helm/rollback_impersonation_test.go diff --git a/internal/helm/client.go b/internal/helm/client.go index 4ddff3a4cc..00cb0e5a95 100644 --- a/internal/helm/client.go +++ b/internal/helm/client.go @@ -2353,22 +2353,25 @@ func (c *Client) Rollback(namespace, name string, revision int) error { // RollbackWithProgress rolls back a release with progress reporting via a channel. // If progressCh is nil, progress messages are silently discarded. func (c *Client) RollbackWithProgress(namespace, name string, revision int, progressCh chan<- InstallProgress) error { - sendProgress := func(phase, message, detail string) { - if progressCh == nil { - return - } - select { - case progressCh <- InstallProgress{Phase: phase, Message: message, Detail: detail}: - default: - } + actionConfig, err := c.getActionConfig(namespace) + if err != nil { + return err } + return c.rollbackWithProgressUsing(actionConfig, name, revision, progressCh) +} - sendProgress("preparing", fmt.Sprintf("Preparing rollback of %s to revision %d...", name, revision), "") - - actionConfig, err := c.getActionConfig(namespace) +// RollbackWithProgressAsUser is RollbackWithProgress with K8s impersonation. +func (c *Client) RollbackWithProgressAsUser(namespace, name string, revision int, username string, groups []string, progressCh chan<- InstallProgress) error { + actionConfig, err := c.getActionConfigForUser(namespace, username, groups) if err != nil { return err } + return c.rollbackWithProgressUsing(actionConfig, name, revision, progressCh) +} + +func (c *Client) rollbackWithProgressUsing(actionConfig *action.Configuration, name string, revision int, progressCh chan<- InstallProgress) error { + sendProgress := progressSender(progressCh) + sendProgress("preparing", fmt.Sprintf("Preparing rollback of %s to revision %d...", name, revision), "") sendProgress("rolling-back", fmt.Sprintf("Rolling back %s to revision %d...", name, revision), "") if err := c.rollbackWith(actionConfig, name, revision); err != nil { return err @@ -2379,11 +2382,7 @@ func (c *Client) RollbackWithProgress(namespace, name string, revision int, prog // RollbackAsUser performs a rollback with K8s impersonation. func (c *Client) RollbackAsUser(namespace, name string, revision int, username string, groups []string) error { - actionConfig, err := c.getActionConfigForUser(namespace, username, groups) - if err != nil { - return err - } - return c.rollbackWith(actionConfig, name, revision) + return c.RollbackWithProgressAsUser(namespace, name, revision, username, groups, nil) } func (c *Client) rollbackWith(actionConfig *action.Configuration, name string, revision int) error { diff --git a/internal/helm/handlers.go b/internal/helm/handlers.go index d1c9ee7595..f4a754f810 100644 --- a/internal/helm/handlers.go +++ b/internal/helm/handlers.go @@ -25,6 +25,17 @@ func IsForbiddenError(err error) bool { return strings.Contains(errLower, "forbidden") || strings.Contains(errLower, "unauthorized") } +// writeReleaseReadError reports a failed read of release content made as the +// caller. Helm stores releases as Secrets, so a denial is the caller's +// Kubernetes RBAC, not a server fault. +func writeReleaseReadError(w http.ResponseWriter, err error) { + if IsForbiddenError(err) { + writeError(w, http.StatusForbidden, "insufficient permissions to read this Helm release: "+err.Error()) + return + } + writeError(w, http.StatusInternalServerError, err.Error()) +} + // userCreds pulls the auth user off the request for *AsUser helpers. // Returns ("", nil) when no user is attached (auth disabled / local binary), // which the *AsUser methods treat as "use the SA identity". @@ -223,7 +234,7 @@ func (h *Handlers) handleGetManifest(w http.ResponseWriter, r *http.Request) { username, groups := userCreds(r) manifest, err := client.GetManifestAsUser(namespace, name, revision, username, groups) if err != nil { - writeError(w, http.StatusInternalServerError, err.Error()) + writeReleaseReadError(w, err) return } @@ -256,7 +267,7 @@ func (h *Handlers) handleGetValues(w http.ResponseWriter, r *http.Request) { username, groups := userCreds(r) values, err := client.GetValuesRevisionAsUser(namespace, name, allValues, revision, username, groups) if err != nil { - writeError(w, http.StatusInternalServerError, err.Error()) + writeReleaseReadError(w, err) return } @@ -282,7 +293,7 @@ func (h *Handlers) handleGetValuesDiff(w http.ResponseWriter, r *http.Request) { username, groups := userCreds(r) diff, err := client.GetValuesDiffAsUser(namespace, name, rev1, rev2, allValues, username, groups) if err != nil { - writeError(w, http.StatusInternalServerError, err.Error()) + writeReleaseReadError(w, err) return } writeJSON(w, diff) @@ -307,7 +318,7 @@ func (h *Handlers) handleGetDiff(w http.ResponseWriter, r *http.Request) { username, groups := userCreds(r) diff, err := client.GetManifestDiffAsUser(namespace, name, rev1, rev2, username, groups) if err != nil { - writeError(w, http.StatusInternalServerError, err.Error()) + writeReleaseReadError(w, err) return } @@ -330,7 +341,7 @@ func (h *Handlers) handleGetNotesDiff(w http.ResponseWriter, r *http.Request) { username, groups := userCreds(r) diff, err := client.GetNotesDiffAsUser(namespace, name, rev1, rev2, username, groups) if err != nil { - writeError(w, http.StatusInternalServerError, err.Error()) + writeReleaseReadError(w, err) return } writeJSON(w, diff) @@ -352,7 +363,7 @@ func (h *Handlers) handleGetHooksDiff(w http.ResponseWriter, r *http.Request) { username, groups := userCreds(r) diff, err := client.GetHooksDiffAsUser(namespace, name, rev1, rev2, username, groups) if err != nil { - writeError(w, http.StatusInternalServerError, err.Error()) + writeReleaseReadError(w, err) return } writeJSON(w, diff) @@ -374,7 +385,7 @@ func (h *Handlers) handleGetResourceDiff(w http.ResponseWriter, r *http.Request) username, groups := userCreds(r) diff, err := client.GetResourceDiffAsUser(namespace, name, rev1, rev2, username, groups) if err != nil { - writeError(w, http.StatusInternalServerError, err.Error()) + writeReleaseReadError(w, err) return } writeJSON(w, diff) @@ -481,7 +492,7 @@ func (h *Handlers) handleBatchUpgradeCheck(w http.ResponseWriter, r *http.Reques // handleRollback rolls back a release to a previous revision func (h *Handlers) handleRollback(w http.ResponseWriter, r *http.Request) { - if !requireHelmWrite(w, r) { + if !requireHelmWrite(w, r, chi.URLParam(r, "namespace")) { return } @@ -527,7 +538,7 @@ func (h *Handlers) handleRollback(w http.ResponseWriter, r *http.Request) { // handleRollbackStream rolls back a release with SSE progress streaming func (h *Handlers) handleRollbackStream(w http.ResponseWriter, r *http.Request) { - if !requireHelmWrite(w, r) { + if !requireHelmWrite(w, r, chi.URLParam(r, "namespace")) { return } @@ -567,8 +578,14 @@ func (h *Handlers) handleRollbackStream(w http.ResponseWriter, r *http.Request) progressCh := make(chan InstallProgress, 10) defer close(progressCh) + auth.AuditLog(r, namespace, name) + user := auth.UserFromContext(r.Context()) resultCh := make(chan error, 1) go func() { + if user != nil { + resultCh <- client.RollbackWithProgressAsUser(namespace, name, revision, user.Username, user.Groups, progressCh) + return + } resultCh <- client.RollbackWithProgress(namespace, name, revision, progressCh) }() @@ -617,7 +634,7 @@ func (h *Handlers) handleRollbackStream(w http.ResponseWriter, r *http.Request) // handleUninstall removes a release func (h *Handlers) handleUninstall(w http.ResponseWriter, r *http.Request) { - if !requireHelmWrite(w, r) { + if !requireHelmWrite(w, r, chi.URLParam(r, "namespace")) { return } @@ -651,7 +668,7 @@ func (h *Handlers) handleUninstall(w http.ResponseWriter, r *http.Request) { // handleUpgrade upgrades a release to a new version func (h *Handlers) handleUpgrade(w http.ResponseWriter, r *http.Request) { - if !requireHelmWrite(w, r) { + if !requireHelmWrite(w, r, chi.URLParam(r, "namespace")) { return } @@ -692,7 +709,7 @@ func (h *Handlers) handleUpgrade(w http.ResponseWriter, r *http.Request) { // handleUpgradeStream upgrades a release with SSE progress streaming func (h *Handlers) handleUpgradeStream(w http.ResponseWriter, r *http.Request) { - if !requireHelmWrite(w, r) { + if !requireHelmWrite(w, r, chi.URLParam(r, "namespace")) { return } @@ -817,7 +834,7 @@ func (h *Handlers) handlePreviewValues(w http.ResponseWriter, r *http.Request) { username, groups := userCreds(r) preview, err := client.PreviewValuesChangeAsUser(namespace, name, req.Values, req.Version, req.Repository, username, groups) if err != nil { - writeError(w, http.StatusInternalServerError, err.Error()) + writeReleaseReadError(w, err) return } @@ -826,7 +843,7 @@ func (h *Handlers) handlePreviewValues(w http.ResponseWriter, r *http.Request) { // handleApplyValues applies new values to a release func (h *Handlers) handleApplyValues(w http.ResponseWriter, r *http.Request) { - if !requireHelmWrite(w, r) { + if !requireHelmWrite(w, r, chi.URLParam(r, "namespace")) { return } @@ -885,18 +902,11 @@ func (h *Handlers) handleListRepositories(w http.ResponseWriter, r *http.Request writeJSON(w, repos) } -// handleUpdateRepository updates the index for a specific repository. -// -// Deliberately NOT gated by requireCloudRole: this fetches chart -// metadata from external repos (artifacthub.io, oci://, etc.) and -// caches it on the radar pod's local filesystem. It mutates pod-local -// state, not cluster state — refresh-the-catalog rather than -// modify-the-cluster. requireHelmWrite still gates it because a future -// install/upgrade depends on a fresh repo cache, but a viewer -// triggering a repo refresh has no security or product cost beyond a -// few HTTP calls to public chart hosts. +// handleUpdateRepository updates the index for a specific repository. It +// mutates only the pod-local chart cache, not cluster state; requireHelmWrite +// gates it because a future install/upgrade depends on a fresh repo cache. func (h *Handlers) handleUpdateRepository(w http.ResponseWriter, r *http.Request) { - if !requireHelmWrite(w, r) { + if !requireHelmWrite(w, r, "") { return } @@ -937,7 +947,7 @@ func (h *Handlers) handleAddOCISource(w http.ResponseWriter, r *http.Request) { if h.ConfigWriteAllowed != nil && !h.ConfigWriteAllowed(w, r) { return } - if !requireHelmWrite(w, r) { + if !requireHelmWrite(w, r, "") { return } var req ociSourceRequest @@ -958,7 +968,7 @@ func (h *Handlers) handleRemoveOCISource(w http.ResponseWriter, r *http.Request) if h.ConfigWriteAllowed != nil && !h.ConfigWriteAllowed(w, r) { return } - if !requireHelmWrite(w, r) { + if !requireHelmWrite(w, r, "") { return } var req ociSourceRequest @@ -1037,10 +1047,6 @@ func (h *Handlers) handleGetChartDetailVersion(w http.ResponseWriter, r *http.Re // handleInstall installs a new Helm release (non-streaming version) func (h *Handlers) handleInstall(w http.ResponseWriter, r *http.Request) { - if !requireHelmWrite(w, r) { - return - } - client := GetClient() if client == nil { writeError(w, http.StatusServiceUnavailable, "Helm client not initialized") @@ -1070,6 +1076,10 @@ func (h *Handlers) handleInstall(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, "repository is required") return } + // Before the chart is fetched: see requireHelmWrite. + if !requireHelmWrite(w, r, req.Namespace) { + return + } auth.AuditLog(r, req.Namespace, req.ReleaseName) var release *HelmRelease @@ -1090,10 +1100,6 @@ func (h *Handlers) handleInstall(w http.ResponseWriter, r *http.Request) { // handleInstallStream installs a Helm release with SSE progress streaming func (h *Handlers) handleInstallStream(w http.ResponseWriter, r *http.Request) { - if !requireHelmWrite(w, r) { - return - } - client := GetClient() if client == nil { writeError(w, http.StatusServiceUnavailable, "Helm client not initialized") @@ -1123,6 +1129,10 @@ func (h *Handlers) handleInstallStream(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, "repository is required") return } + // Before the chart is fetched: see requireHelmWrite. + if !requireHelmWrite(w, r, req.Namespace) { + return + } // Set up SSE headers w.Header().Set("Content-Type", "text/event-stream") @@ -1201,11 +1211,41 @@ type installResult struct { err error } -// requireHelmWrite checks if the service account has Helm write permissions. -// Uses secrets/create as a sentinel check — if the service account can create -// secrets, it likely has the broad RBAC granted by rbac.helm=true. +// requireHelmWrite gates a Helm write whose release lives in namespace. // Returns true if the request should proceed, false if an error was written. -func requireHelmWrite(w http.ResponseWriter, r *http.Request) bool { +// +// With a signed-in caller the write runs as that caller, so their own RBAC +// decides, not Radar's service account: Helm stores the release as a Secret in +// its namespace, so secrets/create there is the check. It runs before any chart +// is fetched, so a caller who could never install can't make Radar reach out to +// an arbitrary repository URL either. An empty namespace is a pod-local change +// (the chart cache, OCI sources) and needs no cluster permission. +// +// Without auth Helm runs as the service account, which needs rbac.helm=true; +// secrets/create is the sentinel for that grant. +func requireHelmWrite(w http.ResponseWriter, r *http.Request, namespace string) bool { + if user := auth.UserFromContext(r.Context()); user != nil { + if namespace == "" { + return true + } + caps, err := k8s.CheckNamespaceCapabilitiesForUser(r.Context(), user.Username, user.Groups, namespace) + if err != nil { + log.Printf("[helm] Failed to check Helm permissions for %s in %q: %v", k8s.SanitizeForLog(user.Username), k8s.SanitizeForLog(namespace), err) + writeError(w, http.StatusInternalServerError, "failed to check permissions: "+err.Error()) + return false + } + if caps == nil || caps.Errors.HelmWrite { + writeError(w, http.StatusServiceUnavailable, "couldn't verify your Kubernetes permissions for Helm; try again") + return false + } + if !caps.HelmWrite { + log.Printf("[helm] Denied %s %s for %s: cannot create secrets in %q", r.Method, r.URL.Path, k8s.SanitizeForLog(user.Username), k8s.SanitizeForLog(namespace)) + writeError(w, http.StatusForbidden, fmt.Sprintf("You don't have permission to manage Helm releases in namespace %q. Helm stores each release as a Secret there, and your Kubernetes RBAC doesn't allow creating Secrets in it.", namespace)) + return false + } + return true + } + caps, err := k8s.CheckCapabilities(r.Context()) if err != nil { log.Printf("[helm] Failed to check capabilities for %s %s: %v", r.Method, r.URL.Path, err) diff --git a/internal/helm/handlers_test.go b/internal/helm/handlers_test.go index dd483771ab..e3c3ed8b89 100644 --- a/internal/helm/handlers_test.go +++ b/internal/helm/handlers_test.go @@ -1,6 +1,7 @@ package helm import ( + "errors" "io" "net/http" "net/http/httptest" @@ -151,3 +152,22 @@ func TestHelmHandlers_NotGatedOnCloudRole(t *testing.T) { }) } } + +// A release read runs as the caller, so a Kubernetes denial is a 403 the UI +// can explain, not a 500 recorded as a server fault. +func TestWriteReleaseReadError(t *testing.T) { + cases := []struct { + err error + want int + }{ + {errors.New(`secrets is forbidden: User "alice" cannot list resource "secrets" in API group "" in the namespace "team-a"`), http.StatusForbidden}, + {errors.New("release: not found"), http.StatusInternalServerError}, + } + for _, tc := range cases { + rec := httptest.NewRecorder() + writeReleaseReadError(rec, tc.err) + if rec.Code != tc.want { + t.Errorf("%q: status = %d, want %d", tc.err, rec.Code, tc.want) + } + } +} diff --git a/internal/helm/helm_write_gate_test.go b/internal/helm/helm_write_gate_test.go new file mode 100644 index 0000000000..63abc7405c --- /dev/null +++ b/internal/helm/helm_write_gate_test.go @@ -0,0 +1,85 @@ +package helm + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "slices" + "strings" + "sync/atomic" + "testing" + + authv1 "k8s.io/api/authorization/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/rest" + + "github.com/skyhook-io/radar/internal/auth" + "github.com/skyhook-io/radar/internal/k8s" +) + +// With a signed-in caller the Helm write gate asks about the caller, in the +// release's namespace, not about Radar's ServiceAccount: an IdP group bound to +// edit in one namespace can manage releases there without rbac.helm=true. +func TestRequireHelmWrite_ChecksCallerInNamespace(t *testing.T) { + var reviews atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost || r.URL.Path != "/apis/authorization.k8s.io/v1/subjectaccessreviews" { + writeK8sStatus(t, w, http.StatusNotFound, "NotFound", "unexpected test request") + return + } + reviews.Add(1) + var review authv1.SubjectAccessReview + if err := json.NewDecoder(r.Body).Decode(&review); err != nil { + t.Fatalf("decode subject access review: %v", err) + } + a := review.Spec.ResourceAttributes + allowed := a != nil && a.Namespace == "payments" && a.Resource == "secrets" && a.Verb == "create" && + slices.Contains(review.Spec.Groups, "radar:idp:platform-eng") + w.Header().Set("Content-Type", "application/json") + writeTestJSON(t, w, authv1.SubjectAccessReview{ + TypeMeta: metav1.TypeMeta{APIVersion: "authorization.k8s.io/v1", Kind: "SubjectAccessReview"}, + Status: authv1.SubjectAccessReviewStatus{Allowed: allowed}, + }) + })) + t.Cleanup(srv.Close) + client, err := kubernetes.NewForConfig(&rest.Config{Host: srv.URL, ContentConfig: rest.ContentConfig{ContentType: "application/json"}}) + if err != nil { + t.Fatalf("create test client: %v", err) + } + prev := k8s.SetTestClient(client) + t.Cleanup(func() { k8s.SetTestClient(prev) }) + prevStatus := k8s.GetConnectionStatus() + k8s.SetConnectionStatus(k8s.ConnectionStatus{State: k8s.StateConnected}) + t.Cleanup(func() { k8s.SetConnectionStatus(prevStatus) }) + + cases := []struct { + name string + user string + groups []string + namespace string + want int + }{ + {"group bound in the namespace", "helm-gate-dana", []string{"radar:viewer", "radar:idp:platform-eng"}, "payments", http.StatusOK}, + {"same group, another namespace", "helm-gate-dana", []string{"radar:viewer", "radar:idp:platform-eng"}, "billing", http.StatusForbidden}, + {"hub role alone grants nothing here", "helm-gate-eli", []string{"radar:owner"}, "payments", http.StatusForbidden}, + {"pod-local change needs no cluster permission", "helm-gate-fay", []string{"radar:viewer"}, "", http.StatusOK}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/api/helm/releases", nil) + req = req.WithContext(auth.ContextWithUser(req.Context(), &auth.User{Username: tc.user, Groups: tc.groups})) + rec := httptest.NewRecorder() + ok := requireHelmWrite(rec, req, tc.namespace) + if got := map[bool]int{true: http.StatusOK, false: rec.Code}[ok]; got != tc.want { + t.Fatalf("status = %d, want %d (body %s)", got, tc.want, rec.Body.String()) + } + if tc.want == http.StatusForbidden && strings.Contains(rec.Body.String(), "rbac.helm") { + t.Errorf("a caller's own RBAC denial must not point at rbac.helm: %s", rec.Body.String()) + } + }) + } + if reviews.Load() == 0 { + t.Fatal("no SubjectAccessReview was made for the caller") + } +} diff --git a/internal/helm/rollback_impersonation_test.go b/internal/helm/rollback_impersonation_test.go new file mode 100644 index 0000000000..ce52942c92 --- /dev/null +++ b/internal/helm/rollback_impersonation_test.go @@ -0,0 +1,41 @@ +package helm + +import ( + "net/http" + "net/http/httptest" + "sync" + "testing" + + "helm.sh/helm/v3/pkg/cli" + "k8s.io/client-go/rest" +) + +// The streaming rollback is a cluster write; when a user is attached it must +// reach the API server as that user, never as Radar's ServiceAccount. +func TestRollbackWithProgressAsUser_Impersonates(t *testing.T) { + var mu sync.Mutex + var seen []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + seen = append(seen, r.Header.Get("Impersonate-User")) + mu.Unlock() + http.Error(w, `{"kind":"Status","apiVersion":"v1","status":"Failure","reason":"Forbidden","code":403}`, http.StatusForbidden) + })) + defer srv.Close() + + c := &Client{settings: cli.New(), restConfig: &rest.Config{Host: srv.URL}} + if err := c.RollbackWithProgressAsUser("team-a", "web", 1, "alice", []string{"radar:idp:team-a"}, nil); err == nil { + t.Fatal("rollback succeeded against a server that forbids everything") + } + + mu.Lock() + defer mu.Unlock() + if len(seen) == 0 { + t.Fatal("rollback made no API request") + } + for i, u := range seen { + if u != "alice" { + t.Errorf("request %d Impersonate-User = %q, want alice", i, u) + } + } +} diff --git a/internal/k8s/capabilities.go b/internal/k8s/capabilities.go index 81ec0f236c..352bf58f85 100644 --- a/internal/k8s/capabilities.go +++ b/internal/k8s/capabilities.go @@ -148,6 +148,7 @@ type NamespaceCapabilityErrors struct { Exec bool Logs bool PortForward bool + HelmWrite bool WorkloadWrites WorkloadWriteCapabilityErrors } @@ -156,6 +157,7 @@ type NamespaceCapabilities struct { Exec bool `json:"exec"` Logs bool `json:"logs"` PortForward bool `json:"portForward"` + HelmWrite bool `json:"helmWrite"` // Helm writes a release as a Secret in its namespace WorkloadWrites WorkloadWritePermissions `json:"workloadWrites"` Errors NamespaceCapabilityErrors `json:"-"` } @@ -436,6 +438,9 @@ func CheckNamespaceCapabilities(ctx context.Context, namespace string) (*Namespa capCheck{group: "apps", resource: "statefulsets", verb: "patch", result: &result.WorkloadWrites.StatefulSets, apiError: &result.Errors.WorkloadWrites.StatefulSets}, capCheck{group: "argoproj.io", resource: "rollouts", verb: "patch", result: &result.WorkloadWrites.Rollouts, apiError: &result.Errors.WorkloadWrites.Rollouts}, ) + if !ForceDisableHelmWrite { + checks = append(checks, capCheck{resource: "secrets", verb: "create", result: &result.HelmWrite, apiError: &result.Errors.HelmWrite}) + } var hadErrors atomic.Bool var wg sync.WaitGroup @@ -630,6 +635,9 @@ func CheckNamespaceCapabilitiesForUser(ctx context.Context, username string, gro capCheck{group: "apps", resource: "statefulsets", verb: "patch", result: &result.WorkloadWrites.StatefulSets, apiError: &result.Errors.WorkloadWrites.StatefulSets}, capCheck{group: "argoproj.io", resource: "rollouts", verb: "patch", result: &result.WorkloadWrites.Rollouts, apiError: &result.Errors.WorkloadWrites.Rollouts}, ) + if !ForceDisableHelmWrite { + checks = append(checks, capCheck{resource: "secrets", verb: "create", result: &result.HelmWrite, apiError: &result.Errors.HelmWrite}) + } var hadErrors atomic.Bool var wg sync.WaitGroup diff --git a/internal/server/capabilities_test.go b/internal/server/capabilities_test.go index cd278a7359..adb30d4dbc 100644 --- a/internal/server/capabilities_test.go +++ b/internal/server/capabilities_test.go @@ -66,3 +66,19 @@ func TestMergeNamespaceCapabilitiesPreservesGlobalOnErroredChecks(t *testing.T) t.Fatalf("clean workload write denies should still override non-errored grants: %+v", caps.WorkloadWrites) } } + +// Helm writes are namespace-scoped like the rest: a user who can create Secrets +// only in their team's namespace gets the Helm buttons there and nowhere else. +func TestMergeNamespaceCapabilitiesHelmWrite(t *testing.T) { + caps := &k8s.Capabilities{HelmWrite: false} + mergeNamespaceCapabilities(caps, &k8s.NamespaceCapabilities{HelmWrite: true}) + if !caps.HelmWrite { + t.Fatal("a clean namespace grant should enable Helm writes there") + } + + caps = &k8s.Capabilities{HelmWrite: true} + mergeNamespaceCapabilities(caps, &k8s.NamespaceCapabilities{}) + if caps.HelmWrite { + t.Fatal("a clean namespace deny should override a global Helm write grant") + } +} diff --git a/internal/server/server.go b/internal/server/server.go index 596eb8bccc..d2985d0caf 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -1396,6 +1396,7 @@ func mergeNamespaceCapabilities(caps *k8s.Capabilities, nsCaps *k8s.NamespaceCap caps.Exec = mergeNamespaceCapability(caps.Exec, nsCaps.Exec, nsCaps.Errors.Exec) caps.Logs = mergeNamespaceCapability(caps.Logs, nsCaps.Logs, nsCaps.Errors.Logs) caps.PortForward = mergeNamespaceCapability(caps.PortForward, nsCaps.PortForward, nsCaps.Errors.PortForward) + caps.HelmWrite = mergeNamespaceCapability(caps.HelmWrite, nsCaps.HelmWrite, nsCaps.Errors.HelmWrite) caps.WorkloadWrites.Deployments = mergeNamespaceCapability(caps.WorkloadWrites.Deployments, nsCaps.WorkloadWrites.Deployments, nsCaps.Errors.WorkloadWrites.Deployments) caps.WorkloadWrites.DaemonSets = mergeNamespaceCapability(caps.WorkloadWrites.DaemonSets, nsCaps.WorkloadWrites.DaemonSets, nsCaps.Errors.WorkloadWrites.DaemonSets) caps.WorkloadWrites.StatefulSets = mergeNamespaceCapability(caps.WorkloadWrites.StatefulSets, nsCaps.WorkloadWrites.StatefulSets, nsCaps.Errors.WorkloadWrites.StatefulSets) diff --git a/web/src/api/client.ts b/web/src/api/client.ts index d67a1f7f80..7a6cd0444b 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -22,7 +22,7 @@ import type { } from '@skyhook-io/k8s-ui' import { useQuery, useMutation, useQueryClient, skipToken } from '@tanstack/react-query' import { showApiError, showApiSuccess } from '../components/ui/Toast' -import { useCanHelmWrite } from '../contexts/CapabilitiesContext' +import { useIsAuthEnabled, useNamespacedCapabilities } from '../contexts/CapabilitiesContext' import type { Topology, ClusterInfo, @@ -2207,21 +2207,29 @@ export function useCloudRole() { } /** - * useCanHelmAct answers whether Helm write and sensitive-read buttons are - * usable. Only the chart's rbac.helm capability is checked here: the calls run - * as the impersonated user, so Kubernetes RBAC decides the rest. The Cloud role - * says nothing about cluster access when IdP groups grant it. + * useCanHelmAct answers whether Helm write buttons are usable for a release in + * `namespace`. With auth on, Helm runs as the signed-in user, so the check is + * the user's own Kubernetes RBAC in that namespace (Helm writes each release as + * a Secret there); the Cloud role says nothing about cluster access when IdP + * groups grant it. Without auth, Helm runs as Radar's ServiceAccount, which + * needs the chart's rbac.helm=true. */ -export function useCanHelmAct(): { allowed: boolean; reason?: string } { - const helmWrite = useCanHelmWrite(); - if (!helmWrite) { +export function useCanHelmAct(namespace?: string): { allowed: boolean; reason?: string } { + const { canHelmWrite } = useNamespacedCapabilities(namespace); + const authEnabled = useIsAuthEnabled(); + if (canHelmWrite) return { allowed: true }; + if (authEnabled) { return { allowed: false, - reason: - "Helm write permissions required. Set rbac.helm=true in the Radar Helm chart values.", + reason: namespace + ? `Your Kubernetes permissions don't allow managing Helm releases in ${namespace} (creating Secrets there).` + : "Your Kubernetes permissions don't allow managing Helm releases.", }; } - return { allowed: true }; + return { + allowed: false, + reason: "Helm write permissions required. Set rbac.helm=true in the Radar Helm chart values.", + }; } // Namespaces diff --git a/web/src/components/helm/ChartBrowser.tsx b/web/src/components/helm/ChartBrowser.tsx index 9cb725b321..fe4472a96b 100644 --- a/web/src/components/helm/ChartBrowser.tsx +++ b/web/src/components/helm/ChartBrowser.tsx @@ -3,7 +3,7 @@ import { Search, RefreshCw, Package, Database, AlertCircle, ExternalLink, Chevro import { PaneLoader, Input } from '@skyhook-io/k8s-ui' import { clsx } from 'clsx' import { useHelmRepositories, useSearchCharts, useUpdateRepository, useUpdateRepositorySilent, useArtifactHubSearch, type ArtifactHubSortOption } from '../../api/client' -import { useCanHelmWrite } from '../../contexts/CapabilitiesContext' +import { useCanHelmWrite, useIsAuthEnabled } from '../../contexts/CapabilitiesContext' import type { ChartInfo, HelmRepository, ArtifactHubChart, ChartSource } from '../../types' import { formatAge } from './helm-utils' import { SEVERITY_BADGE } from '../../utils/badge-colors' @@ -25,12 +25,11 @@ export function ChartBrowser({ onChartSelect }: ChartBrowserProps) { const [showVerifiedOnly, setShowVerifiedOnly] = useState(false) const [artifactHubSort, setArtifactHubSort] = useState('relevance') - // Repo refresh is gated only by `requireHelmWrite` on the backend - // (handleUpdateRepository deliberately skips requireCloudRole — it - // mutates pod-local chart cache, not cluster state). So the frontend gate - // here must NOT include the Cloud role check, or Cloud viewers with - // rbac.helm=true would be blocked from a refresh the backend allows. - const canHelmWrite = useCanHelmWrite() + // Refreshing repositories only touches Radar's own chart cache. With auth on, + // any signed-in user may do it; without auth it follows rbac.helm. + const helmWrite = useCanHelmWrite() + const authEnabled = useIsAuthEnabled() + const canHelmWrite = helmWrite || authEnabled const helmWriteReason = canHelmWrite ? '' : 'Helm write permissions required. Set rbac.helm=true in the Radar Helm chart values.' // Local repo hooks diff --git a/web/src/components/helm/HelmReleaseDrawer.tsx b/web/src/components/helm/HelmReleaseDrawer.tsx index aa95c65954..110edecd5c 100644 --- a/web/src/components/helm/HelmReleaseDrawer.tsx +++ b/web/src/components/helm/HelmReleaseDrawer.tsx @@ -128,8 +128,8 @@ export function HelmReleaseDrawer({ release, onClose, onNavigateToResource, isOp const resizeStartWidth = useRef(DEFAULT_WIDTH) const targetVersionRef = useRef('') const editedUpgradeYamlRef = useRef('') - const { allowed: canHelmWrite, reason: helmActReason } = useCanHelmAct() const helmNamespace = release.storageNamespace || release.namespace + const { allowed: canHelmWrite, reason: helmActReason } = useCanHelmAct(helmNamespace) const { data: releaseDetail, isLoading, error: releaseError, refetch: refetchRelease } = useHelmRelease( helmNamespace, @@ -706,7 +706,6 @@ export function HelmReleaseDrawer({ release, onClose, onNavigateToResource, isOp releaseNamespace={releaseDetail.namespace} managedByFluxHelmRelease={releaseDetail.managedByFluxHelmRelease} hookDiagnostics={releaseDetail.hookDiagnostics} - canCompare onCompare={handleCompareRevisions} onNavigateToResource={onNavigateToResource} /> @@ -1055,7 +1054,6 @@ function HelmOperationBanner({ releaseNamespace, managedByFluxHelmRelease, hookDiagnostics, - canCompare, onCompare, onNavigateToResource, }: { @@ -1064,7 +1062,6 @@ function HelmOperationBanner({ releaseNamespace: string managedByFluxHelmRelease?: string hookDiagnostics?: HookDiagnostic[] - canCompare?: boolean onCompare?: (rev1: number, rev2: number) => void onNavigateToResource?: NavigateToResource }) { @@ -1135,7 +1132,6 @@ function HelmOperationBanner({ @@ -1154,13 +1150,11 @@ function HelmOperationBanner({ function OperationInsightSignals({ insight, releaseNamespace, - canCompare, onCompare, onNavigateToResource, }: { insight?: HelmOperationInsight releaseNamespace: string - canCompare: boolean onCompare?: (rev1: number, rev2: number) => void onNavigateToResource?: NavigateToResource }) { @@ -1169,7 +1163,7 @@ function OperationInsightSignals({ const relatedCount = primaryResource ? Math.max(0, (insight?.signalCount ?? 0) - 1) : insight?.relatedResources?.length ?? 0 - const showCompare = Boolean(compare && canCompare && onCompare) + const showCompare = Boolean(compare && onCompare) if (!primaryResource && !showCompare) { return null } diff --git a/web/src/components/helm/InstallWizard.tsx b/web/src/components/helm/InstallWizard.tsx index 73f8e8c96a..d647004612 100644 --- a/web/src/components/helm/InstallWizard.tsx +++ b/web/src/components/helm/InstallWizard.tsx @@ -66,7 +66,6 @@ export function InstallWizard({ repo, chartName, version, source, repoUrl, defau const progressEndRef = useRef(null) const queryClient = useQueryClient() - const { allowed: canHelmWrite, reason: helmActReason } = useCanHelmAct() // Choose the right data based on source const isLocal = source === 'local' @@ -209,6 +208,10 @@ export function InstallWizard({ repo, chartName, version, source, repoUrl, defau ) const releaseNameError = releaseNameValidation.valid ? null : releaseNameValidation.error const namespaceError = namespaceValidation.valid ? null : namespaceValidation.error + // Checked in the namespace being installed into, once it's a valid name. + const { allowed: canHelmWrite, reason: helmActReason } = useCanHelmAct( + namespaceValidation.valid ? namespace.trim() : undefined, + ) const canProceedFromInfo = releaseNameValidation.valid && namespaceValidation.valid const canInstall = canProceedFromInfo && !yamlError diff --git a/web/src/components/helm/ValuesViewer.tsx b/web/src/components/helm/ValuesViewer.tsx index f00f4cbbf8..487d87a0e3 100644 --- a/web/src/components/helm/ValuesViewer.tsx +++ b/web/src/components/helm/ValuesViewer.tsx @@ -46,7 +46,7 @@ export function ValuesViewer({ const previewMutation = useHelmPreviewValues() const applyMutation = useHelmApplyValues() - const { allowed: canHelmWrite, reason: helmActReason } = useCanHelmAct() + const { allowed: canHelmWrite, reason: helmActReason } = useCanHelmAct(namespace) const isHistoricalRevision = typeof revision === 'number' && typeof currentRevision === 'number' && revision !== currentRevision const canEdit = Boolean(namespace && name) && canHelmWrite && !isHistoricalRevision diff --git a/web/src/contexts/CapabilitiesContext.tsx b/web/src/contexts/CapabilitiesContext.tsx index 6ffadda2dd..6647f25d48 100644 --- a/web/src/contexts/CapabilitiesContext.tsx +++ b/web/src/contexts/CapabilitiesContext.tsx @@ -114,6 +114,10 @@ export function useCanHelmWrite(): boolean { return useContext(CapabilitiesContext).helmWrite } +export function useIsAuthEnabled(): boolean { + return useContext(CapabilitiesContext).authEnabled === true +} + export function useCanNodeWrite(): boolean { return useContext(CapabilitiesContext).nodeWrite } @@ -158,7 +162,8 @@ export function useNamespacedCapabilities(namespace: string | undefined) { canExec: nsCaps?.exec ?? globalCaps.exec, canViewLogs: nsCaps?.logs ?? globalCaps.logs, canPortForward: nsCaps?.portForward ?? globalCaps.portForward, + canHelmWrite: nsCaps?.helmWrite ?? globalCaps.helmWrite, workloadWrites: nsCaps?.workloadWrites ?? globalCaps.workloadWrites, workloadWritesPending: Boolean(namespace && isPending), - }), [globalCaps.exec, globalCaps.logs, globalCaps.portForward, globalCaps.workloadWrites, isPending, namespace, nsCaps]) + }), [globalCaps.exec, globalCaps.logs, globalCaps.portForward, globalCaps.helmWrite, globalCaps.workloadWrites, isPending, namespace, nsCaps]) } From e51efe0993317f5770c73cdfa65d1c49a5fc3694 Mon Sep 17 00:00:00 2001 From: Arlen Vasconcelos Date: Mon, 28 Sep 2026 22:11:10 -0400 Subject: [PATCH 09/16] Answer /auth/me's no-namespace flag from the permission cache only 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 Claude-Session: https://claude.ai/code/session_017J25KHG5W4DzS32bU1J8ou --- internal/server/server.go | 17 ++++++++++++----- internal/server/server_auth_test.go | 11 +++++++++++ web/src/App.tsx | 2 +- web/src/components/NoClusterAccessBanner.tsx | 2 +- 4 files changed, 25 insertions(+), 7 deletions(-) diff --git a/internal/server/server.go b/internal/server/server.go index d2985d0caf..4cbec25ece 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -5147,11 +5147,18 @@ func (s *Server) handleAuthMe(w http.ResponseWriter, r *http.Request) { } // Lets the shell explain an empty cluster: every read is filtered to // the user's namespaces, so a user bound to none sees empty lists that - // look like a cluster with nothing in it. Only once connected: before - // that, namespace discovery fails closed and would report "none" for - // everyone. - if k8s.IsConnected() && noNamespaceAccess(s.getUserNamespaces(r, nil)) { - resp["noNamespaceAccess"] = true + // look like a cluster with nothing in it. Reported only from a + // discovery that succeeded (the cached entry): before the cluster + // connects, or when a SAR errors, discovery fails closed to "none" + // without caching, and that is not a statement about the user's RBAC. + // Read from the cache only, never discovered here: app startup waits on + // this endpoint, and discovery costs a SAR or two per namespace. The + // banner asks again once content has loaded, by which point the first + // resource request has filled the cache. + if k8s.IsConnected() { + if perms := s.permCache.Get(user.Username, user.Groups); perms != nil && noNamespaceAccess(auth.FilterNamespacesForUser(nil, user, perms)) { + resp["noNamespaceAccess"] = true + } } } s.writeJSON(w, resp) diff --git a/internal/server/server_auth_test.go b/internal/server/server_auth_test.go index 93f6ded462..30eae1d669 100644 --- a/internal/server/server_auth_test.go +++ b/internal/server/server_auth_test.go @@ -905,6 +905,17 @@ func TestHandleAuthMe_NoNamespaceAccess(t *testing.T) { } }) + // /auth/me never discovers: with nothing cached (before the first resource + // request, or after a failed discovery, which caches nothing) the field is + // left out and the banner asks again once content has loaded. + t.Run("absent when nothing is cached", func(t *testing.T) { + k8s.SetConnectionStatus(k8s.ConnectionStatus{State: k8s.StateConnected}) + s := newAuthServer(auth.Config{Mode: "proxy"}) + if _, has := authMeBody(t, s, user)["noNamespaceAccess"]; has { + t.Error("noNamespaceAccess present with no cached permissions") + } + }) + // Before the cluster connects, namespace discovery fails closed; reporting // that as "no access" would flash the banner for every user. t.Run("absent while not connected", func(t *testing.T) { diff --git a/web/src/App.tsx b/web/src/App.tsx index 0c90024e40..509ec66d27 100644 --- a/web/src/App.tsx +++ b/web/src/App.tsx @@ -1961,11 +1961,11 @@ function AppInner({ manageDocumentTitle = false, documentTitleSuffix, onClusterL )} + {contentReady && } {/* Main content - only show when connected and authenticated */} {/* inert while a fullscreen detail overlay covers the views — keeps the retained background list out of the focus order + a11y tree (the visual cover already blocks pointer events). */} - {contentReady && } {(contentReady || shellDuringSync) &&
{/* Search included, not just the path: selection inside a view rides in the query (?resource=, ?release=), so a path-only key would still diff --git a/web/src/components/NoClusterAccessBanner.tsx b/web/src/components/NoClusterAccessBanner.tsx index 28912eea0d..e14efb7f1c 100644 --- a/web/src/components/NoClusterAccessBanner.tsx +++ b/web/src/components/NoClusterAccessBanner.tsx @@ -26,7 +26,7 @@ export function NoClusterAccessBanner() { Radar is connected, but your Kubernetes permissions don't allow reading any namespace, so From c40188896098d964357ea5c3e2b6f94b4f7be054 Mon Sep 17 00:00:00 2001 From: Arlen Vasconcelos Date: Mon, 28 Sep 2026 23:14:41 -0400 Subject: [PATCH 10/16] chart: radar:system is for the alerts worker and timeline puller only 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 --- deploy/helm/radar/README.md | 6 +++--- deploy/helm/radar/templates/cloud-rbac-system.yaml | 4 ++-- deploy/helm/radar/values.yaml | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/deploy/helm/radar/README.md b/deploy/helm/radar/README.md index 7fab6f4c7d..66ea168a88 100644 --- a/deploy/helm/radar/README.md +++ b/deploy/helm/radar/README.md @@ -163,7 +163,7 @@ before upgrading if the added visibility is unwanted. ### Radar Cloud background identities (`radar:system`) -The hub's alerts worker, timeline puller and AI diagnose agent call Radar +The hub's alerts worker and timeline puller call Radar as the `radar:system` group, not as a user. `cloud.systemRbac` (default `true`) binds that group to a read-only set: `view`, the cluster-read and integration-read add-ons above, and `get/list/watch` on Secrets. Secret read is @@ -173,8 +173,8 @@ Helm stores each release as a Secret. It is independent of `cloud.defaultRbac`, so turning the role bindings off (`cloud.defaultRbac.create=false`, for example when IdP groups decide cluster access) leaves alerts and the hub timeline working. It is all or nothing: -`cloud.systemRbac=false` removes the whole grant, and alerts, the hub timeline -and AI diagnose then see nothing on clusters without the role bindings unless +`cloud.systemRbac=false` removes the whole grant, and alerts and the hub +timeline then see nothing on clusters without the role bindings unless you bind `radar:system` yourself. Change it through this value: a binding edited or deleted with `kubectl` comes back on the next upgrade. diff --git a/deploy/helm/radar/templates/cloud-rbac-system.yaml b/deploy/helm/radar/templates/cloud-rbac-system.yaml index 825c316233..c55b10c995 100644 --- a/deploy/helm/radar/templates/cloud-rbac-system.yaml +++ b/deploy/helm/radar/templates/cloud-rbac-system.yaml @@ -2,8 +2,8 @@ Read-only access for Radar Cloud's own background identities. The hub's background identities have no user or IdP groups; they assert the -radar:system group. That is the alerts worker, the timeline puller and the -AI diagnose agent. Without a binding of their own they would ride +radar:system group. That is the alerts worker and the timeline puller (AI +diagnose reads as radar:viewer). Without a binding of their own they would ride on radar:owner, which disappears when an org turns the role bindings off (cloud.defaultRbac.create=false) to let IdP groups decide cluster access — silently stopping alerts and the hub timeline. diff --git a/deploy/helm/radar/values.yaml b/deploy/helm/radar/values.yaml index 1fb4c9f669..1da3188e34 100644 --- a/deploy/helm/radar/values.yaml +++ b/deploy/helm/radar/values.yaml @@ -521,7 +521,7 @@ cloud: # owner later activates it. See templates/cloud-rbac-node-ops.yaml. nodeOps: false # Read-only binding for Radar Cloud's own background identities (the hub's - # alerts worker, timeline puller and AI diagnose agent), which assert the + # alerts worker and timeline puller), which assert the # radar:system group. Grants the viewer set (view + cluster-read + # integration-read) and cluster-wide Secret read, which Helm release alerts # and Secret changes in the hub timeline need (Helm stores each release as a @@ -529,7 +529,7 @@ cloud: # Independent of defaultRbac: it stays on when the role bindings above are # off, so alerts and the hub timeline keep working when IdP groups decide # cluster access. All or nothing: false removes the whole grant, and then - # alerts, the hub timeline and AI diagnose see nothing on clusters without + # alerts and the hub timeline see nothing on clusters without # the role bindings unless you bind radar:system yourself. # An upgrade with --reuse-values from a release that predates this key leaves # it OFF, so an existing install never gains Secret read without choosing it. From 6eb752a89a5ca4f7b7ba2732c402a718af6ebe29 Mon Sep 17 00:00:00 2001 From: Arlen Vasconcelos Date: Tue, 29 Sep 2026 11:01:14 -0400 Subject: [PATCH 11/16] Keep asking about namespace access until the server knows /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 Claude-Session: https://claude.ai/code/session_017J25KHG5W4DzS32bU1J8ou --- internal/server/server.go | 8 ++++--- internal/server/server_auth_test.go | 13 ++++++----- web/src/api/client.ts | 5 ++++- .../components/NoClusterAccessBanner.test.ts | 22 +++++++++++++++++++ web/src/components/NoClusterAccessBanner.tsx | 22 ++++++++++++++++++- 5 files changed, 59 insertions(+), 11 deletions(-) create mode 100644 web/src/components/NoClusterAccessBanner.test.ts diff --git a/internal/server/server.go b/internal/server/server.go index 4cbec25ece..3084bdc825 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -5154,10 +5154,12 @@ func (s *Server) handleAuthMe(w http.ResponseWriter, r *http.Request) { // Read from the cache only, never discovered here: app startup waits on // this endpoint, and discovery costs a SAR or two per namespace. The // banner asks again once content has loaded, by which point the first - // resource request has filled the cache. + // resource request has filled the cache. Absent means not known yet, so + // the client keeps asking; false means known and some namespace is + // readable. if k8s.IsConnected() { - if perms := s.permCache.Get(user.Username, user.Groups); perms != nil && noNamespaceAccess(auth.FilterNamespacesForUser(nil, user, perms)) { - resp["noNamespaceAccess"] = true + if perms := s.permCache.Get(user.Username, user.Groups); perms != nil { + resp["noNamespaceAccess"] = noNamespaceAccess(auth.FilterNamespacesForUser(nil, user, perms)) } } } diff --git a/internal/server/server_auth_test.go b/internal/server/server_auth_test.go index 30eae1d669..6df7d3f603 100644 --- a/internal/server/server_auth_test.go +++ b/internal/server/server_auth_test.go @@ -887,21 +887,22 @@ func TestHandleAuthMe_NoNamespaceAccess(t *testing.T) { } }) - t.Run("absent when some namespace is readable", func(t *testing.T) { + // false, not absent: absent means "not known yet" and keeps the client polling. + t.Run("false when some namespace is readable", func(t *testing.T) { k8s.SetConnectionStatus(k8s.ConnectionStatus{State: k8s.StateConnected}) s := newAuthServer(auth.Config{Mode: "proxy"}) s.permCache.Set(user.Username, user.Groups, &auth.UserPermissions{AllowedNamespaces: []string{"team-a"}}) - if _, has := authMeBody(t, s, user)["noNamespaceAccess"]; has { - t.Error("noNamespaceAccess present for a user who can read team-a") + if got, has := authMeBody(t, s, user)["noNamespaceAccess"]; !has || got != false { + t.Errorf("noNamespaceAccess = %v (present %v), want false for a user who can read team-a", got, has) } }) - t.Run("absent for cluster-wide access", func(t *testing.T) { + t.Run("false for cluster-wide access", func(t *testing.T) { k8s.SetConnectionStatus(k8s.ConnectionStatus{State: k8s.StateConnected}) s := newAuthServer(auth.Config{Mode: "proxy"}) s.permCache.Set(user.Username, user.Groups, &auth.UserPermissions{AllowedNamespaces: nil}) - if _, has := authMeBody(t, s, user)["noNamespaceAccess"]; has { - t.Error("noNamespaceAccess present for a user with cluster-wide access") + if got, has := authMeBody(t, s, user)["noNamespaceAccess"]; !has || got != false { + t.Errorf("noNamespaceAccess = %v (present %v), want false for a user with cluster-wide access", got, has) } }) diff --git a/web/src/api/client.ts b/web/src/api/client.ts index 7a6cd0444b..ee17c89c92 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -2150,10 +2150,13 @@ export interface AuthMe { * When false, logout clears Radar's cookie but the proxy may re-auth * the same user on the next request. */ proxyLogoutConfigured?: boolean; - /** Connected, and the user's RBAC allows reading no namespace at all. */ + /** Connected, and the user's RBAC allows reading no namespace at all. + * Absent until the server knows (its first namespace discovery hasn't + * finished yet), and always absent from servers that predate it. */ noNamespaceAccess?: boolean; } + export function useAuthMe() { return useQuery({ queryKey: ["auth-me"], diff --git a/web/src/components/NoClusterAccessBanner.test.ts b/web/src/components/NoClusterAccessBanner.test.ts new file mode 100644 index 0000000000..8b82c3b30a --- /dev/null +++ b/web/src/components/NoClusterAccessBanner.test.ts @@ -0,0 +1,22 @@ +import { describe, expect, it } from 'vitest' +import { ACCESS_CHECK_RETRIES, shouldRetryAccessCheck } from './NoClusterAccessBanner' + +// /auth/me can answer before the first namespace discovery finishes; the +// banner must not stay hidden for the whole session because of that race. +describe('shouldRetryAccessCheck', () => { + it('retries while access is not known yet, a bounded number of times', () => { + expect(shouldRetryAccessCheck({ authEnabled: true }, 0)).toBe(true) + expect(shouldRetryAccessCheck({ authEnabled: true }, ACCESS_CHECK_RETRIES - 1)).toBe(true) + expect(shouldRetryAccessCheck({ authEnabled: true }, ACCESS_CHECK_RETRIES)).toBe(false) + }) + + it('stops once access is known either way', () => { + expect(shouldRetryAccessCheck({ authEnabled: true, noNamespaceAccess: false }, 0)).toBe(false) + expect(shouldRetryAccessCheck({ authEnabled: true, noNamespaceAccess: true }, 0)).toBe(false) + }) + + it('never retries without auth', () => { + expect(shouldRetryAccessCheck({ authEnabled: false }, 0)).toBe(false) + expect(shouldRetryAccessCheck(undefined, 0)).toBe(false) + }) +}) diff --git a/web/src/components/NoClusterAccessBanner.tsx b/web/src/components/NoClusterAccessBanner.tsx index e14efb7f1c..ea834ef0a6 100644 --- a/web/src/components/NoClusterAccessBanner.tsx +++ b/web/src/components/NoClusterAccessBanner.tsx @@ -1,7 +1,17 @@ import { useEffect, useState } from 'react' import { ShieldAlert } from 'lucide-react' import { AlertBanner, Collapse, CollapseChevron, useDisclosure } from '@skyhook-io/k8s-ui' -import { useAuthMe } from '../api/client' +import { useAuthMe, type AuthMe } from '../api/client' + +// Unknown (field absent) means the server hadn't finished its first namespace +// discovery when it answered: the banner's own refetch can beat the first +// resource load. Ask again shortly, a bounded number of times, since a server +// that predates the field never answers. +export const ACCESS_CHECK_RETRY_MS = 5000 +export const ACCESS_CHECK_RETRIES = 12 +export function shouldRetryAccessCheck(me: AuthMe | undefined, attempts: number): boolean { + return !!me?.authEnabled && me.noNamespaceAccess === undefined && attempts < ACCESS_CHECK_RETRIES +} // Every read is filtered to the user's namespaces, so someone bound to none // sees empty lists in every view and reads it as an empty cluster. Stated in @@ -15,6 +25,16 @@ export function NoClusterAccessBanner() { useEffect(() => { void refetch() }, [refetch]) + const [attempts, setAttempts] = useState(0) + const retry = shouldRetryAccessCheck(me, attempts) + useEffect(() => { + if (!retry) return + const id = setTimeout(() => { + setAttempts((n) => n + 1) + void refetch() + }, ACCESS_CHECK_RETRY_MS) + return () => clearTimeout(id) + }, [retry, attempts, refetch]) const [groupsOpen, setGroupsOpen] = useState(false) const groupsDisclosure = useDisclosure(groupsOpen) From af4afc7c2af8644e5b35543415ad07825f612c37 Mon Sep 17 00:00:00 2001 From: Arlen Vasconcelos Date: Tue, 29 Sep 2026 13:19:23 -0400 Subject: [PATCH 12/16] Keep the no-access banner's group IDs out of session replay 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 Claude-Session: https://claude.ai/code/session_017J25KHG5W4DzS32bU1J8ou --- web/src/components/NoClusterAccessBanner.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/web/src/components/NoClusterAccessBanner.tsx b/web/src/components/NoClusterAccessBanner.tsx index ea834ef0a6..b8b58be622 100644 --- a/web/src/components/NoClusterAccessBanner.tsx +++ b/web/src/components/NoClusterAccessBanner.tsx @@ -55,7 +55,10 @@ export function NoClusterAccessBanner() { } > {groups.length > 0 && ( -
+ // ph-no-capture: group IDs name the customer's directory structure. + // Radar Cloud embeds this view with session replay on, and its text + // mask only catches credential-shaped values. A no-op without PostHog. +
-
-
- {pairReady && ( - - )} +
+
+ {pairReady && ( + + )} -
- {!pairReady ? ( -
- -
-
Pick two different revisions to compare
-
Use the revision selectors above to choose a source and target revision.
-
+
+ {!pairReady ? ( +
+ +
+
Pick two different revisions to compare
+
Use the revision selectors above to choose a source and target revision.
- ) : ( - <> -
- -
- - - - + ) : ( + <> +
+ +
+ + + + + + + + + + - - - - - - - - )} -
-
+ + )} +
+
) } diff --git a/web/src/components/helm/HelmReleaseDrawer.tsx b/web/src/components/helm/HelmReleaseDrawer.tsx index 110edecd5c..91c9c5f6b6 100644 --- a/web/src/components/helm/HelmReleaseDrawer.tsx +++ b/web/src/components/helm/HelmReleaseDrawer.tsx @@ -724,27 +724,27 @@ export function HelmReleaseDrawer({ release, onClose, onNavigateToResource, isOp )} {activeTab === 'manifest' && ( copyToClipboard(text, 'manifest')} - copied={copied === 'manifest'} - /> + manifest={manifest || ''} + isLoading={manifestLoading} + revision={selectedRevision} + onCopy={(text) => copyToClipboard(text, 'manifest')} + copied={copied === 'manifest'} + /> )} {activeTab === 'values' && ( copyToClipboard(text, 'values')} - copied={copied === 'values'} - namespace={helmNamespace} - name={release.name} - revision={selectedRevision} - currentRevision={releaseDetail.revision} - onApplySuccess={() => refetch()} - /> + values={values} + isLoading={valuesLoading} + showAllValues={showAllValues} + onToggleAllValues={setShowAllValues} + onCopy={(text) => copyToClipboard(text, 'values')} + copied={copied === 'values'} + namespace={helmNamespace} + name={release.name} + revision={selectedRevision} + currentRevision={releaseDetail.revision} + onApplySuccess={() => refetch()} + /> )} {activeTab === 'resources' && (