Repository navigation
Commit 2a4ffb1
Redact Secret values from Helm diffs for AI callers and bind radar:ai for background Diagnose (#1949)
Linear: RAD-657
## Why
Radar Cloud's AI Diagnose is blind on clusters whose role bindings are
off (`cloud.defaultRbac.create=false`, IdP groups decide access),
because it reads as `radar:viewer`, which is unbound there. After this
release:
- **Manual** Diagnose runs read as the person who started them
(skyhook-dev/radar-hub#292), so the AI never exceeds that person's
access. A person may be able to read Secrets, so Helm diffs handed to
the model must not carry Secret values.
- **Background** (alert-triggered) runs read as one view-only identity,
the `radar:ai` group (skyhook-dev/radar-hub#288), which this chart
binds.
MCP clients are not affected: they keep reading with the user's own
identity.
## What changes
**1. Secret redaction in `get_helm_release` (every MCP caller)**
- `include=diff` (the manifest diff) and `include=notes_diff` were
returned unredacted; only `values_diff` went through `RedactSecrets`.
- Each `kind: Secret` document now has its `data` / `stringData` values
replaced by `[REDACTED]` before diffing. The keys stay, so added and
removed keys still show. Secrets inside `kind: List` items are covered,
the `last-applied-configuration` annotation (a full copy of the object)
is dropped, and a document that doesn't parse is withheld whole. Other
documents are left byte-for-byte untouched.
- `RedactSecrets` then runs over both diffs as a second layer.
- `include=values` and `include=values_diff` now use key-aware redaction
built for Helm values (`aicontext.RedactHelmValues`). Chart authors name
credentials freely (`dbPassword`, `auth.postgresPassword`), so any key
ending in `password`, `token`, `apiKey`, `secretKey`, `clientSecret`,
`credentials` and similar is masked, numbers included. Secret
*references* (`existingSecret`, `*SecretName`, `*Ref`) keep their
values. The values diff masks both revisions before diffing. CRD specs
keep the existing exact-key matching.
- The REST/UI Helm diff is unchanged: it is the user's own view.
**2. `radar:ai` chart binding**
- New `templates/cloud-rbac-ai.yaml`: binds `radar:ai` to `view`. No
Secret role, no writes.
- `radar:ai` is added to the cluster-read and integration-read add-ons.
- New value `cloud.aiRbac`, default `true`, independent of
`cloud.defaultRbac`. As with `cloud.systemRbac`, an absent value means
off, so a `--reuse-values` upgrade never gains the binding silently.
- Docs: the chart README and `docs/authentication.md`.
The binding does nothing until Radar Cloud sends the group. Background
runs send `radar:ai` with no tier group, so the AI's access is exactly
this binding: `cloud.aiRbac=false` really turns it off, and a widened
viewer role doesn't widen the AI. Clusters on older charts get no
background Diagnose until they upgrade. Radar Cloud refuses manual runs
on a Radar older than this release, rather than reading with less
protection.
## Tests
- Go unit tests for Helm values redaction: nested keys, numeric
credentials, references kept under `credentials`, both revisions of a
values diff.
- Go unit tests for the manifest-diff redaction: Secret values gone,
keys and non-Secret changes kept, last-applied copies, Secrets inside
Lists, and unparseable documents. Plus a check that `resource_diff`
masks Secret values.
- `TestIntegrationReadBindings` expects the `radar:ai` bindings
alongside `radar:system`, with cases for `aiRbac=false`.
- New `tests/cloud_rbac_ai_test.yaml`: renders with
`defaultRbac.create=false`, absent or `false` means off, bound to `view`
only, add-ons present whatever the tier settings.
- `scripts/test-chart.sh`: new render cases; the "no orphan role" cases
now also set `cloud.aiRbac=false`.
- `helm unittest`: the `service_test.yaml` failures also fail on `main`
locally; CI's Helm chart job passes.
### Live test on EKS (proxy auth, simulated Diagnose identities)
- **Background identity** (`radar:ai,radar:org:<org>`) with the chart's
`radar:ai` binding: pods in every namespace, no Secrets.
- **Manual run as a contractor** whose IdP group has `view` in one
namespace: pods in that namespace only; listing all namespaces returns
only that one.
- **Helm release with a Secret, a token in its notes, and `dbPassword` /
`apiKey` values**, read through `get_helm_release` by a user who can
read Secrets: no sentinel value in `values`, `diff`, `notes_diff` or
`values_diff`. The REST diff (the user's own view in the UI) still shows
them, as intended.
### End to end on kind
- **Role bindings off, `aiRbac=true`:** the AI identity reads pods,
events and nodes; Secrets are forbidden.
- **`aiRbac=false`:** it reads nothing.
- **`get_helm_release include=diff,notes_diff`, called by an owner who
can read Secrets:** no Secret values in the output. The #1899 binary
returns the plaintext values and a `ghp_` token from the notes.
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Medium Risk**
> Changes cluster RBAC defaults and how credentials appear in MCP Helm
diffs; misconfiguration could block background Diagnose or leak secrets
if redaction regresses, but grants stay read-only and UI paths are
unchanged.
>
> **Overview**
> Adds **default Kubernetes RBAC for automatic Radar Cloud Diagnose**
via new `cloud.aiRbac` (default on): chart-owned aggregated **view**
role for `radar:ai`, plus cluster-read and integration-read add-ons—**no
Secret access**. Refactors `radar:system` the same way (owned aggregated
view role instead of binding directly to `view`), with Secret read
unchanged for alerts/timeline. Docs and Helm tests cover independence
from `cloud.defaultRbac`, `--reuse-values` absent-key behavior, and
installer `escalate` requirements.
>
> **Hardens Helm MCP output for AI callers** while leaving REST/UI diffs
as the user’s own view: manifest diffs strip Secret `data`/`stringData`
(and unparseable Secret docs) before diffing; values diffs use new
**`RedactHelmValues`** (suffix/key-aware credentials, preserved secret
*references*); notes diffs get pattern redaction. `get_helm_release`
`include=diff`, `values_diff`, and `notes_diff` route through these
paths.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
16be4f7. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
---------
Co-authored-by: Nadav Erell <nadaverell@gmail.com>1 parent 228c7c3 commit 2a4ffb1
22 files changed
Lines changed: 1293 additions & 100 deletions
File tree
- deploy/helm/radar
- templates
- tests
- docs
- internal
- helm
- k8s
- mcp
- pkg/ai/context
- scripts
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
161 | 161 | | |
162 | 162 | | |
163 | 163 | | |
164 | | - | |
165 | | - | |
166 | | - | |
167 | | - | |
168 | | - | |
169 | | - | |
170 | | - | |
171 | | - | |
172 | | - | |
173 | | - | |
174 | | - | |
175 | | - | |
176 | | - | |
177 | | - | |
178 | | - | |
179 | | - | |
180 | | - | |
181 | | - | |
182 | | - | |
183 | | - | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
184 | 205 | | |
185 | 206 | | |
186 | 207 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
73 | 73 | | |
74 | 74 | | |
75 | 75 | | |
76 | | - | |
| 76 | + | |
77 | 77 | | |
78 | 78 | | |
79 | 79 | | |
| |||
87 | 87 | | |
88 | 88 | | |
89 | 89 | | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
49 | 49 | | |
50 | 50 | | |
51 | 51 | | |
52 | | - | |
| 52 | + | |
| 53 | + | |
53 | 54 | | |
54 | 55 | | |
55 | 56 | | |
| |||
191 | 192 | | |
192 | 193 | | |
193 | 194 | | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
194 | 212 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
5 | | - | |
| 5 | + | |
| 6 | + | |
6 | 7 | | |
7 | 8 | | |
8 | 9 | | |
| |||
16 | 17 | | |
17 | 18 | | |
18 | 19 | | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
19 | 23 | | |
20 | 24 | | |
21 | 25 | | |
| |||
49 | 53 | | |
50 | 54 | | |
51 | 55 | | |
52 | | - | |
| 56 | + | |
53 | 57 | | |
54 | 58 | | |
55 | 59 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | | - | |
| 2 | + | |
3 | 3 | | |
4 | | - | |
5 | | - | |
6 | | - | |
7 | | - | |
8 | | - | |
9 | | - | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
10 | 8 | | |
11 | | - | |
12 | | - | |
13 | | - | |
14 | | - | |
15 | | - | |
16 | | - | |
17 | | - | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
18 | 15 | | |
19 | 16 | | |
20 | 17 | | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
21 | 30 | | |
22 | 31 | | |
23 | | - | |
| 32 | + | |
24 | 33 | | |
25 | 34 | | |
26 | 35 | | |
27 | 36 | | |
28 | 37 | | |
29 | | - | |
| 38 | + | |
30 | 39 | | |
31 | 40 | | |
32 | 41 | | |
| |||
0 commit comments