Skip to content

fix(rbac): enforce per-kind RBAC on all timeline change/diff/drop surfaces - #1300

Merged
hisco merged 2 commits into
mainfrom
radar-timeline-sse-rbac-leak
Jul 30, 2026
Merged

fix(rbac): enforce per-kind RBAC on all timeline change/diff/drop surfaces#1300
hisco merged 2 commits into
mainfrom
radar-timeline-sse-rbac-leak

Conversation

@hisco

@hisco hisco commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Problem

Change / timeline / diff data was filtered by namespace and, on some paths, by cluster-scoped kind — but never by whether the caller can read the specific kind within a namespace they can otherwise see. A user with workload-only access in namespace X could receive change events (with field-level diffs — including Secret data key names, since diffSecret emits keys) for Secrets, Roles, ConfigMaps and other kinds they cannot read.

This was systemic: the same gap existed (unfiltered or namespace-only) across REST, the SSE live stream, MCP tools, and debug endpoints — each re-implemented (or omitted) the filter.

Auth is only active for multi-user deployments. With no user on the request (local kubeconfig), the authorizers pass through, so single-user / OSS-local behavior is unchanged.

Approach

One shared authorization decision — k8s.ChangeReadAllowed(kind, apiVersion, namespace, authorize):

  • resolves the change's GVR (apiVersion disambiguates CRD-vs-builtin Kind collisions),
  • authorizes cluster-scoped kinds at namespace "" (a K8s Event about a Node stores the Event's own namespace, not the object's),
  • fails closed on an unresolved kind.

Every surface that emits change/diff/drop data routes through it, each supplying its own authorizer (REST: canReadUser; MCP: canReadInNamespace). The decision is defined once so it can't drift or be forgotten.

Surfaces gated

  • REST: /api/changes, /api/timeline/events (delta + window), /api/changes/children, the SSE k8s_event stream, /api/debug/events + /api/debug/events/diagnose, /api/issues recent_changes, dashboard recent changes, /api/diagnostics drops.
  • MCP: get_changes, dashboard, issues recent_changes, per-issue correlation (including a workload's consumed ConfigMaps), diagnose, get_resource.

Supporting changes

  • Static namespaced-builtin GVR catalogue so builtins resolve without discovery — removes a cold-start window where authenticated users would briefly see fewer rows, and keeps the gate unit-testable.
  • ClassifyKindScope now honors the group hint against the builtin catalogue, closing a CRD-vs-builtin Kind-collision authorization bypass (a CRD Kind=ClusterRole in another group was authorized against builtin clusterroles). This also hardens the ~20 existing canReadClusterScopedKind callers; all pass the correct group.
  • issuesapi.RecentChange gains APIVersion (set from the source timeline event) for the same collision disambiguation.
  • GetDiagnosis is scoped to the active cluster context — it was the only timeline query missing ClusterContext, so it could return a previously-connected cluster's rows from the persistent store.

Helm-package-manager rows (Source == helm) retain their existing namespace/helm authorization (unchanged from the prior filter).

Tests

  • internal/k8s/change_rbac_test.go — GVR resolution, cluster-scoped namespace forcing, CRD-collision fail-closed, ChangeReadAllowed.
  • internal/mcp/changes_rbac_test.go — per-kind gap + cluster-scoped + Helm passthrough.
  • internal/server/sse_rbac_test.go — authorizer path, and a superset check proving the SSE gate drops the same sensitive kinds a narrower interim would.
  • internal/k8s/kinds_test.go — collision-guard behavior.
  • go build, go vet, go test ./..., and -race on the concurrency-sensitive paths all green (main + pkg modules).

Relationship to #1299

This supersedes the interim SSE sensitive-kinds gate in #1299. It covers the same seven kinds (Secret, Role, RoleBinding, ClusterRole, ClusterRoleBinding, Mutating/ValidatingWebhookConfiguration) — verified by a dedicated test — plus every other kind, with per-(kind, namespace) precision instead of a cluster-wide approximation, and across all surfaces (not just SSE). A user holding only a per-namespace grant is no longer over-denied; a user who can't read the kind still gets nothing. #1299 is left open for the author to close.

Known, deliberately-scoped residues

  • Drop records (DropRecord) carry no apiVersion, so they resolve group-less: a dropped event whose Kind collides with a cluster-scoped builtin the caller can read could surface (drop metadata only — kind/namespace/name/reason, no diffs). Threading apiVersion would change the shared k8score.OnDrop contract; not worth it for this low-value surface.
  • Cross-cluster drop provenance: drop metrics are process-global and not reset on context switch, so debug/diagnostics drop surfaces could show a prior cluster's resource names after a switch (for a kind the caller can read now). Pre-existing metrics-lifecycle gap, orthogonal to per-kind RBAC — flagged for a separate change.

https://claude.ai/code/session_01XNhMe6Zdqwj5EoBazNDnmW


Note

High Risk
Touches authentication/authorization across many user-facing data paths; incorrect SAR or GVR resolution could hide legitimate data or leak sensitive change metadata (including secret key names in diffs).

Overview
Closes a namespace-only RBAC gap on timeline and change data: callers who can see a namespace but not a specific kind (e.g. pods but not secrets) no longer receive those rows’ diffs, names, or summaries through REST, SSE, MCP, or debug paths.

Central gate: k8s.ChangeReadAllowed resolves GVR from kind + apiVersion, authorizes cluster-scoped kinds at namespace "", and fails closed when resolution fails. REST and MCP each wire their own SAR-backed authorize callback so the rule stays in one place.

Surfaces updated: /api/changes, timeline streams, change children, SSE k8s_event frames (per-client authorizer at subscribe), issues recent_changes, dashboard changes, MCP get_changes / diagnose / get_resource / issue correlation, and debug drops/diagnose — replacing the older cluster-scoped-only filters.

Supporting fixes: static namespaced-builtin GVR catalogue (no discovery cold-start), ClassifyKindScope group-hint collision guard for CRD vs builtin kinds, RecentChange.APIVersion and dynamic-cache Group/Resource on changes, canReadUser for SSE without a request, and GetDiagnosis scoped to active cluster context with RBAC before recommendations.

Reviewed by Cursor Bugbot for commit 1bb4a13. Bugbot is set up for automated code reviews on this repo. Configure here.

…faces

Change/timeline/diff data was filtered by namespace and, on some paths, by
cluster-scoped kind — but not by whether the caller can read the specific kind
within a namespace they can otherwise see. A user with workload-only access in a
namespace could receive change events (with field-level diffs, including Secret
data key names) for Secrets, Roles, ConfigMaps and other kinds they cannot read,
across REST, SSE, MCP and debug surfaces.

Introduce one shared authorization decision (k8s.ChangeReadAllowed) that resolves
a change's GVR (apiVersion disambiguates CRD kind collisions), authorizes
cluster-scoped kinds at namespace "", and fails closed on unresolved kinds. Every
surface that emits change/diff/drop data routes through it, each supplying its
own authorizer (REST: canReadUser; MCP: canReadInNamespace).

Also:
- Add a static namespaced-builtin GVR catalogue so builtins resolve without
  discovery (removes a cold-start window and keeps the gate unit-testable).
- Fix ClassifyKindScope to honor the group hint against the builtin catalogue,
  closing a CRD-vs-builtin Kind-collision authorization bypass.
- Add APIVersion to issuesapi.RecentChange for collision disambiguation.
- Scope GetDiagnosis to the active cluster context (was returning a
  previously-connected cluster's rows from the persistent store).

Auth is only active for multi-user deployments; with no user on the request
(local kubeconfig) the authorizers pass through, so single-user behavior is
unchanged. Helm-package-manager rows retain their existing namespace/helm
authorization.

Claude-Session: https://claude.ai/code/session_01XNhMe6Zdqwj5EoBazNDnmW
@hisco
hisco requested a review from nadaverell as a code owner July 29, 2026 16:50

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit a3a33eb. Configure here.

Comment thread internal/server/server.go
@hisco

hisco commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

e2e verification (real cluster, real RBAC)

Verified against a dedicated kind cluster with real SubjectAccessReview, driving Radar in --auth-mode proxy as two users in the same namespace team-a:

  • viewer — RoleBinding granting list/get/watch on pods, deployments, replicasets, configmaps, events (not secrets)
  • admin — cluster-admin

Sanity: kubectl auth can-i list secrets -n team-a --as viewerno, --as adminyes.

After generating Secret/Deployment/ConfigMap changes:

GET /api/changes?namespaces=team-a — kinds returned per user

Kind viewer admin
Deployment / Pod / ReplicaSet / ConfigMap
Secret ❌ dropped
ServiceAccount ❌ dropped

SSE GET /api/events/stream — a live Secret change triggered mid-capture: viewer's stream carried Deployment/Pod/Event frames and zero Secret frames; admin's did.

Same namespace, divergence exactly on the kinds viewer cannot read — confirming the per-kind gate on both the REST query and the live stream, end-to-end through real informers + per-user SAR (not mocks).

handleDebugEventsDiagnose filtered GetDiagnosis's timeline events and drop
history by RBAC after the fact and only neutralized the recommendations when
both filtered lists were empty. GetDiagnosis builds the recommendations from the
unfiltered lists, so when RBAC removed events but a drop survived, tips derived
from the filtered-out events were still returned.

Move the per-kind gate into GetDiagnosis via an `allow(kind, apiVersion,
namespace)` callback that runs before the recommendations are derived, so tips
can only describe rows the caller may read. The handler passes the callback
(nil when auth is off); the ad-hoc post-filter + empty-list neutralization are
removed.

Claude-Session: https://claude.ai/code/session_01XNhMe6Zdqwj5EoBazNDnmW
@hisco
hisco merged commit 4dc7ebf into main Jul 30, 2026
9 checks passed
@nadaverell

Copy link
Copy Markdown
Contributor

Adversarial review (self + codex cross-model) of the full branch delta. One fix needed before merge; everything else vetted clean.

Fix before merge: SSE authorization degrades to an uncached serial SAR loop after cache expiry

canReadUser (internal/server/server.go) only memoizes the SAR result when the parent permission-cache entry exists:

if perms != nil {
    perms.SetCanI(verb, group, resource, namespace, allowed)
}

REST paths are fine — canRead/parseNamespacesForUser re-prime the entry on every request. But the SSE path primes once at subscribe (handleSSE), and the entry TTLs out after ~2 minutes. From then on, for that client, every qualifying change frame triggers a fresh SAR that is never stored — and those SARs run serially inside the single watchResourceChanges broadcast goroutine (internal/server/sse.go), with no bounded timeout (the closure holds the connection-lived SSE ctx). One slow/hung SAR stalls change broadcasts for all connected clients; sustained change traffic multiplies into apiserver SAR load per client × distinct (kind, namespace) tuple.

This hits exactly the deployment mode the PR targets (auth-enabled, multi-user, long-lived streams). Suggested shape: memoize SAR results independently of the parent cache entry (create the entry when missing, or a standalone per-user tuple memo with TTL), and put a bounded timeout on the SAR call in the broadcast path. A test across cache expiry would pin it.

Vetted green (checked, no action needed)

  • Slice ownership: the in-place [:0] filters are safe everywhere — MemoryStore.Query materializes a fresh slice per call, GetSnapshot deep-copies RecentDrops, SQLite scans into fresh slices. No shared-backing-array corruption.
  • SSE broadcast locking: snapshot-under-RLock then authorize+send outside the lock is correct; a SAR round-trip can't stall registrations/other broadcasts on b.mu.
  • Collision guard: ClassifyKindScope group-hint check closes the CRD-vs-builtin authorization bypass; fail-closed with no discovery; pinned by kinds_test.go.
  • Fail-closed posture: unresolved kinds are dropped for authenticated callers on every surface (ChangeReadAllowed, clientCanSeeChange with empty resource, filterDropsByRBAC).
  • Diagnose recommendations: the Bugbot follow-up (1bb4a13) correctly moves the RBAC gate inside GetDiagnosis before tips are derived; TestGetDiagnosis_RecommendationsRespectRBAC pins it.
  • Bounded SAR fan-out on REST: filterEventsByRBAC dedupes tuples and caps concurrency at 16 — no SAR stampede on broad timeline loads.
  • Auth-off / single-user: nil user short-circuits everywhere; OSS-local behavior unchanged.

Triaged, not blocking

  • Group-less drop records can mis-authorize a CRD Kind that shadows a builtin (codex flagged as medium): already documented as a residue in the PR description; metadata-only (kind/ns/name/reason) on debug surfaces; fixing requires changing the shared k8score.OnDrop contract. Reasonable deferral.
  • StaleSecretEnv issues bypass Secret RBAC (codex flagged as high on this branch in isolation): that is exactly fix(rbac): gate cross-kind evidence in the issues pipeline #1302 — the three PRs must land as a set.

Merge order

This PR must land before #1301: the diagnose endpoint's store-query cluster scoping (store.Query(..., ClusterContext)) lives here, and #1301 alone leaves that leak open. Whoever rebases #1301 afterwards: handleDiagnostics/handleDebugEvents conflict on the RecentDrops line — the resolution must compose both filters (s.filterDropsByRBAC(r, timeline.DropsForCluster(...))), and GetDiagnosis needs the union of both signatures (store-query scoping + allow from here, stamped DropRecord.ClusterContext filter from #1301).

nadaverell added a commit that referenced this pull request Aug 3, 2026
…ence (#1322)

## Problem

Found while auditing the #1300/#1302 RBAC work for drop-vs-degrade
behavior. Per-issue change correlation
(`internal/mcp/issue_correlation.go`) filters a workload issue's
correlated changes by per-kind RBAC — a workload subject's candidates
include its consumed ConfigMaps, which the caller may not be able to
read (#1300). But when that filter **empties** the list, the code fell
through to stamping the issue with an affirmative `no_recent_changes`
marker.

The marker's own contract says otherwise — fetch errors and saturated
fetches deliberately omit it ("marker omitted = unknown, never a false
'no changes'"). RBAC-hidden is the same epistemic state: evidence
exists; this caller can't see it. The failure mode is concrete: a user
with workload access but no ConfigMap access gets their crashing
Deployment certified "no recent changes in the window — chronic issue"
when its consumed ConfigMap rotated two minutes ago, and MCP consumers
(including AI diagnosis) explicitly weigh that marker.

## Fix

`applyCorrelationVisibilityFilters` applies the category filter first,
then the RBAC filter, and reports whether RBAC removed a **relevant**
row. When the visible set is empty, `rbacHidden` joins `saturated` in
omitting the marker — unknown, not "no changes". Ordering matters: a
status-churn row dropped by the category filter must not suppress the
marker for a genuinely quiet subject.

`CorrelatedChanges` behavior is unchanged (still only rows the caller
may read); auth-off is unchanged (no user → RBAC filter is a passthrough
→ `rbacHidden` is always false).

## Tests

- `TestApplyCorrelationVisibilityFilters_RBACHiddenIsNotNoChanges`:
unreadable ConfigMap as the only relevant change → empty + hidden
(marker suppressed); readable Deployment + status-churn row → visible +
not hidden (quiet subjects still earn the marker); auth-off passthrough.
- `go build`, `go vet`, full `go test ./...` green.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Changes triage marker semantics for MCP/issue consumers (including
diagnosis), but scope is limited to the correlation visibility path and
aligns with existing “omit = unknown” behavior for errors and
saturation.
> 
> **Overview**
> Fixes a false **no_recent_changes** stamp when per-kind RBAC removes
correlated evidence (e.g. a consumed ConfigMap the caller cannot list)
but the issue subject is still readable.
> 
> Correlation now runs **`applyCorrelationVisibilityFilters`**, which
applies spec/lifecycle filtering first, then RBAC, and sets
**`rbacHidden`** when relevant rows were dropped for permissions. If the
visible set is empty, **`rbacHidden`** is treated like a saturated
fetch: the marker is **omitted** (unknown), not an affirmative “nothing
changed.” Status-only churn still does not count as hidden, so genuinely
quiet subjects still get the marker.
> 
> Adds
**`TestApplyCorrelationVisibilityFilters_RBACHiddenIsNotNoChanges`** for
hidden ConfigMap, readable Deployment + status churn, and auth-off
passthrough.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
cd705f9. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants