Skip to content

sec(chart): drop the unused watch verb from the resource-monitor ClusterRole - #780

Merged
saadqbal merged 1 commit into
developfrom
sec/950-drop-unused-watch-resource-monitor
Aug 21, 2026
Merged

sec(chart): drop the unused watch verb from the resource-monitor ClusterRole#780
saadqbal merged 1 commit into
developfrom
sec/950-drop-unused-watch-resource-monitor

Conversation

@saadqbal

@saadqbalsaadqbal commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

The last code item on backend#950. Refs backend#950.

Why

#731 narrowed this rule to the resources the image actually reads, but left watch on both rules. Nothing watches.

Every Kubernetes call in the resource-monitor image is a point read or a one-shot list:

apiGroupcallsverbs needed
""read_namespaced_pod, read_node, read_node_status, list_pod_for_all_namespacesget, list
metrics.k8s.ioget_cluster_custom_object, list_namespaced_custom_object ×2get, list

No Watch(), no watch.stream, no watch=True anywhere in Node-deploy/, and the image shells out to nothing — its CMD is python /app/resource_monitor.py and no kubectl is installed. The template's own comment already enumerated exactly this surface and never mentioned watch.

Worth removing rather than shrugging at: this is a cluster-scoped role, so watch on pods is a live streaming feed of every pod object in the cluster — other tenants' training pods included — held by an agent that only samples its own node's metrics. Same argument that removed pods/log under #950, one verb over.

Why it survived, and the fix for that

resource_monitor_test.yaml pinned resources and left verbs unasserted. The rule got narrowed on one axis while the other kept an unused grant — which is exactly how you end up doing a security trim twice.

Both axes are asserted now. The new test was checked against the unfixed template: re-adding watch fails it with - watch / +- watch on both rules, so it isn't vacuous.

$ helm unittest ./client -f 'tests/resource_monitor_test.yaml' # with watch restored
FAIL Resource-monitor DaemonSet
- ClusterRole grants get and list only — never watch
Tests: 1 failed, 12 passed, 13 total

Verification

  • helm unittest ./client492 passed, 33 suites (was 488)
  • helm lint ./client — clean
  • scripts/chart-version-guard.sh — bump present ✓
  • Rendered output confirms both rules are ["get", "list"]

Chart 1.9.50 → 1.9.51.

Note on closing backend#950

This clears the last code item. What remains there is operational and not mine to do: rotating the exposed resource-monitor credential (the finding-1 token was in Datadog-scraped logs, so the fix stops future exposure but doesn't un-expose it), flipping POD_TOKEN_REQUIRE_BOUND_CLAIMS=1 once pre-#950 tokens have drained, and FR on staging. The chart-rollout bullet there is stale — develop is on 1.9.50, well past the 1.9.45 it names.

🤖 Generated with Claude Code


Note

Medium Risk
Touches cluster-scoped RBAC for a node telemetry ServiceAccount. The change only drops unused watch (least privilege), so blast radius is a failed rollout if the image actually watched, which tests and comments say it does not.

Overview
Tightens the resource-monitor ClusterRole so both core and metrics.k8s.io rules grant only get/list. Unused cluster-wide watch on pods/nodes is dropped (backend#950 follow-up): the agent only does point reads and one-shot lists, and a live watch would stream every pod object in the cluster.

Helm unittest now asserts verbs on both rules (get/list only, no watch) so an unused grant cannot return silently. Chart version 1.9.50 → 1.9.51.

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

…terRole
The last code item on backend#950. #731 narrowed this rule to the resources the
image actually reads but left `watch` on both rules, and nothing watches.
Every Kubernetes call in the resource-monitor image is a point read or a
one-shot list — read_namespaced_pod, read_node, read_node_status,
list_pod_for_all_namespaces, get_cluster_custom_object,
list_namespaced_custom_object. There is no Watch(), no watch.stream, no
watch=True anywhere in Node-deploy/, and the image shells out to nothing (CMD is
`python /app/resource_monitor.py`; no kubectl is installed). The template's own
comment already enumerated exactly this surface and never mentioned watch.
Worth removing rather than shrugging at: this is a CLUSTER-scoped role, so
`watch` on pods is a live streaming feed of every pod object in the cluster —
other tenants' training pods included — held by an agent that only samples its
own node. Same argument that removed `pods/log` under #950, one verb over.
WHY IT SURVIVED, and the fix for that: resource_monitor_test.yaml pinned
`resources` and left `verbs` unasserted, so the rule was narrowed on one axis
while the other kept an unused grant. Both axes are asserted now, and the new
test was checked against the unfixed template — re-adding `watch` fails it, so
it is not vacuous.
helm unittest 492 passed (was 488), helm lint clean.
Refs backend#950
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@saqlainsyed007saqlainsyed007 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.

Approving. Verified against the code:

  • The trim is correct and worth doing. Every Kubernetes call in the resource-monitor image is a point read or a one-shot list (read_namespaced_pod, read_node, read_node_status, list_pod_for_all_namespaces, get_cluster_custom_object, list_namespaced_custom_object) — no Watch(), no watch.stream, no watch=True, and no kubectl in the image. A cluster-scoped watch on pods is a live feed of every pod object across all tenants, held by an agent that only samples its own node — dropping it is a real least-privilege win, the same argument that removed pods/log under #950.
  • The test is mutation-proof, which is the whole point.watch survived the #950 trim precisely because the suite pinned resources and left verbs open. Now both rules assert the exact verb set andnotContains: watch — restoring watch reddens it on both rules, so this axis can't silently regrant. Chart bump present for the drift gate.

Least-privilege on a cluster-scoped role, closed on both axes so it stays closed. LGTM.

@saadqbal
saadqbal merged commit 38365cf into developAug 21, 2026
24 checks passed
@saadqbal
saadqbal deleted the sec/950-drop-unused-watch-resource-monitor branch August 21, 2026 09:29
@LukasWodka

Copy link
Copy Markdown
Contributor

/fr-pass

Sign up for freeto 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.

3 participants

@saadqbal@LukasWodka@saqlainsyed007