Skip to content

fix(rbac): gate cross-kind evidence in the issues pipeline - #1302

Open
hisco wants to merge 1 commit into
mainfrom
radar-issues-rbac
Open

fix(rbac): gate cross-kind evidence in the issues pipeline#1302
hisco wants to merge 1 commit into
mainfrom
radar-issues-rbac

Conversation

@hisco

@hisco hisco commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Problem

Issues are computed by detectors running as the cache ServiceAccount (which sees everything) and served filtered by namespace only — never by whether the caller can read the specific kind an issue's evidence was derived from. Two HIGH cross-kind leaks (from the timeline-RBAC audit):

  • StaleSecretEnv — entirely derived from a referenced Secret's change history: the Secret's existence, its data key names (for envFrom consumers — not in the pod spec), and the rotation timing. A user who can read a Pod but not Secrets in its namespace still received them.
  • Unschedulable — embedded cluster-scoped Node label values: the scheduling explainer enumerated what the fleet's nodes carry for the offending label (zones, instance types, tenancy/internal labels), leaking node topology to a namespaced pod-reader who can't list nodes.

Auth is only active for multi-user deployments; with no user on the request the predicates pass through, so single-user behavior is unchanged.

Fix

StaleSecretEnv — a new Filters.CanRead(group, resource, namespace) (namespaced per-kind SAR) + applyCrossKindEvidenceAccess, which drops StaleSecretEnv rows when the caller can't read Secrets in the issue's namespace. Because issue serving fans out across many compose paths, the predicate is threaded through every one so the gate can't be bypassed:

  • /api/issues (grouped + flat) and MCP issues
  • RelatedIssues — per-resource issue summaries: REST /api/issues/resource, AI + MCP get_resource/diagnose, mutation-verification
  • BuildIssueIndex — issue counts in search / list_resources (REST + MCP summary-context)
  • GitOps insights ResourceProblems

Unschedulable node labels — the scheduling explainer (explainMissingLabel/explainMissingExpr) now reports only the pod's own unsatisfiable requirement (no node has disktype=ssd) and drops the fleet-value enumeration (— N node(s) carry disktype: [hdd, nvme]) for all callers. The node values are low-value diagnostic color and cluster-scoped Node content, so removing them (rather than per-user redaction) is the simplest complete fix.

Tests

  • internal/issues/cross_kind_access_test.go — StaleSecretEnv dropped when secrets unreadable in the namespace; unrelated issues + authorized callers + nil predicate untouched.
  • internal/k8s/detect_scheduling*_test.go — updated to assert the redacted message and that the fleet value is now absent (regression pins).
  • e2e (kind + proxy auth): a namespaced viewer with no node RBAC gets their pod's Unschedulable issue as no node has disktype=ssd — the fleet value hdd is not present. Passes.
  • go build, go vet, go test ./... green (main + pkg).

Scope

The two HIGH findings from the audit. The audit's lower-severity items (namespace-wide quota text for Job/SS/DS, Service port lists, OOM→owning-RS limit, deploymentChangeContext rollout, and trivial count leaks) are tracked as a follow-up.

https://claude.ai/code/session_01XNhMe6Zdqwj5EoBazNDnmW


Note

High Risk
Changes authorization and data exposure across the issues pipeline, REST, MCP, and GitOps surfaces—security-sensitive RBAC and information-leak fixes that must stay consistent on every compose path.

Overview
Closes RBAC gaps where issues were namespace-filtered but still exposed evidence from kinds the caller cannot read, and where Unschedulable text leaked cluster node label values to namespaced readers.

StaleSecretEnv — Adds Filters.CanRead (SAR-backed per-kind read) and applyCrossKindEvidenceAccess, which drops StaleSecretEnv rows when the user cannot list secrets in the issue namespace. The predicate is threaded through compose, RelatedIssues, issue indexes (BuildIssueIndex), REST /api/issues, MCP issues / get_resource / diagnose / list/search summary context, GitOps insights ResourceProblems, and mutation verification so the gate cannot be bypassed on a side path. nil CanRead preserves auth-off and tests.

Unschedulable messagingexplainMissingLabel / explainMissingExpr now state only the pod’s own unsatisfiable selector/affinity (e.g. no node has kubernetes.io/arch=arm64) and no longer enumerate what nodes in the fleet actually carry (zones, arch values, etc.).

Tests cover secret gating and scheduling message redaction; existing call sites pass SAR canRead from server/MCP handlers.

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

@nadaverell

Copy link
Copy Markdown
Contributor

Adversarial review (self + codex cross-model) of the full branch delta. Green light.

Vetted green (checked, no action needed)

  • Predicate threading is complete: Filters.CanRead verified wired through every fan-out path that serves issues — Compose (applyCrossKindEvidenceAccess runs on flat rows, so grouped output and member fan-out inherit it), RelatedIssues (REST /api/issues/resource, AI/MCP get_resource/diagnose, mutation verification), BuildIssueIndex (search + list_resources counts — so the count can't side-channel a gated issue), REST /api/issues (grouped + flat), MCP issues, and GitOps insights ResourceProblems.
  • Both hosts wire real per-user SARs: REST uses s.canRead (request user), MCP uses canReadInNamespace (ctx user, memoized) — no path passes the cache SA's identity.
  • Auth-off unchanged: nil CanRead is a strict passthrough; single-user/OSS behavior identical.
  • Unschedulable fix is the right shape: deleting the fleet-value enumeration for all callers is simpler and more complete than per-user redaction; tests assert the fleet value is absent, and the e2e (kind + proxy auth) confirms it end-to-end.

Triaged, not blocking

  • Other cross-kind evidence still flows (codex, high): missing-reference issues reveal existence of referenced Secrets/ConfigMaps/PVCs, Service-derived issues carry port lists, etc. These are exactly the lower-severity audit items this PR's Scope section defers — a conscious cut, not an oversight (and referenced names are already visible in the readable pod spec; only existence/derived detail is incremental).
  • Residual scheduling oracle (codex, medium): the message still distinguishes "no node carries label X" from "no node has X=v" — one existence bit, probeable only by someone with pod-create rights, who already gets similar-granularity signals from kube-scheduler's own Unschedulable events. Below the actionable line.

One ask for the follow-up

applyCrossKindEvidenceAccess is a blocklist keyed on Reason == "StaleSecretEnv" — it fails open for the next detector that embeds cross-kind evidence, the opposite posture of #1300's fail-closed gate. When the deferred audit items get fixed, prefer stamping issues with their evidence-source GVRs and filtering generically over adding more reason-string matches; at two or three reason checks the scattered policy becomes a liability.

Issues are computed by detectors running as the cache ServiceAccount (which
sees everything) and are served filtered by namespace only — not by whether the
caller can read the specific kind an issue's evidence was derived from. Two HIGH
cross-kind leaks resulted:

- StaleSecretEnv issues are entirely derived from a referenced Secret's change
  history (the Secret's existence, its data key names for envFrom consumers, and
  the rotation timing — none of which is in the Pod's own readable spec). A user
  who can read a Pod but not Secrets in its namespace still received them.

- Unschedulable issues embedded cluster-scoped Node label VALUES: the scheduling
  explainer enumerated the values the fleet's nodes carry for the offending label
  (zones, instance types, tenancy/internal labels), leaking node topology to a
  namespaced pod-reader who cannot list nodes.

StaleSecretEnv: add Filters.CanRead (namespaced per-kind SAR) and
applyCrossKindEvidenceAccess, which drops StaleSecretEnv rows when the caller
can't read Secrets in the issue's namespace. The predicate is threaded through
EVERY issue-serving path — /api/issues + MCP issues, RelatedIssues (per-resource
summaries: REST + AI + MCP get_resource/diagnose + mutation-verification),
BuildIssueIndex (issue counts in search/list), and GitOps insights
ResourceProblems — so the gate can't be bypassed. nil predicate (auth off) is a
no-op, preserving single-user behavior.

Unschedulable node labels: the scheduling explainer now reports only the pod's
OWN unsatisfiable requirement, dropping the fleet-value enumeration for all
callers (the node values are low-value diagnostic color and cluster-scoped Node
content).

Claude-Session: https://claude.ai/code/session_01XNhMe6Zdqwj5EoBazNDnmW
@hisco
hisco force-pushed the radar-issues-rbac branch from 770cc05 to ef7d642 Compare July 31, 2026 09:38

@nadaverell nadaverell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Revising my earlier green light on one point after looking at this through the product lens: the StaleSecretEnv gate needs graceful degradation, not row removal. Requesting changes so this lands before merge.

The problem with drop-the-row

applyCrossKindEvidenceAccess deletes the entire issue for callers without secret-read in the namespace. But the caller it deletes it for — pod/workload access, no Secret access — is the standard RBAC split for app developers, i.e. this issue's primary audience. A dev whose pod is silently running stale DB credentials now gets nothing: no issue, no "something is stale," no restart recommendation. We've protected the evidence by removing the diagnosis from exactly the people it exists for.

What to do instead: degrade the row

Serve unauthorized callers a redacted row rather than no row. The field-level analysis makes this clean, because most of the message is already visible to a pod-reader via the pod spec:

Field Spec-visible to pod-reader? Redacted row
Secret name Yes (secretKeyRef / envFrom refs) Keep
Key name (secretKeyRef env) Yes (.key in spec) Keep (or drop for a simpler uniform rule)
Key names (envFrom consumers) No — the audit's key-name leak Strip
Rotation timestamp (SecretChangedAt) No — the timing leak Strip (the "after this container started" framing carries the operational meaning)
Container start time, restart guidance Yes / derived Keep

The generic message tier already exists — the Ready-path detection message names neither secret nor key:

"Pod/web is Ready, but a running container loaded a Secret-backed environment value before Radar observed its consumed Secret key change; it may still hold the pre-change value."

So the shape is roughly: when CanRead("", "secrets", ns) is false, keep the issue but serve the generic message tier and strip the per-check evidence fields (key names, SecretChangedAt) instead of dropping the row in the compose filter. Simplest safe rule if per-consumer-type distinction is annoying: no key names, no timestamps for ungated callers, secret name OK.

Tests to pin: unauthorized caller gets the redacted row, not absence (and no key/timestamp anywhere in message or evidence); authorized caller unchanged; nil CanRead (auth off) unchanged.

Same principle, other direction: Unschedulable fleet values

The scheduling fix deletes the fleet-value hint (— node(s) carry disktype: [hdd, nvme]) for everyone, including callers who can read every Node. The unauthorized path degrades gracefully there (the core no node has disktype=ssd survives), but the authorized path lost the actionable half — what values actually exist. Preferred end state: gate the enrichment on node-read (CanRead at cluster scope) instead of deleting it. Fine as an immediate follow-up rather than in this PR if you'd rather keep the diff focused — but the StaleSecretEnv degradation should land here.

General rule for both (and for the deferred audit items): degrade, don't drop; gate enrichment, don't delete it.

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