From 884fbf3b17abfd18c7b01984c445da8dffb72018 Mon Sep 17 00:00:00 2001 From: Abdelrahman Ahmed Date: Sun, 18 Oct 2026 12:28:53 +0300 Subject: [PATCH 1/2] Make least-privilege the chart's default RBAC mode The chart now defaults to rbac.mode=least-privilege, so a default install no longer gets list/watch on every resource kind. rbac.mode=full stays available for custom resources and kinds added later. Also fix the values.yaml comment: the license instance Secret grant is get/update, not get/create/update. --- README.md | 7 ++++--- deploy/chart/README.md | 2 +- deploy/chart/templates/NOTES.txt | 2 +- deploy/chart/templates/rbac.yaml | 2 +- deploy/chart/test_helm.sh | 10 +++++++--- deploy/chart/values.schema.json | 4 ++-- deploy/chart/values.yaml | 29 ++++++++++++++++------------- docs/configuration.md | 4 ++-- docs/contributing-source.md | 4 ++-- docs/production-operations.md | 5 ++++- docs/upgrading-to-1.0.md | 11 +++++++---- 11 files changed, 47 insertions(+), 33 deletions(-) diff --git a/README.md b/README.md index 8defc953a..092217eae 100644 --- a/README.md +++ b/README.md @@ -337,9 +337,10 @@ state file, and SelfSubjectAccessReviews for its permission audit (not persisted). It never gets `pods/exec` or `nodes/proxy`. **By default its ServiceAccount can read every Secret in the cluster** (kwatch keeps only hashes, never values). Set `watch.secrets: false` (Helm: `--set -watch.secrets=false`) to remove all Secret access; `rbac.mode: -least-privilege` narrows the other kinds but still lists Secrets unless -`watch.secrets` is false. Helm prints this warning on install and kwatch logs +watch.secrets=false`) to remove all Secret access. The chart's default +`rbac.mode: least-privilege` lists only the kinds kwatch understands but +still lists Secrets unless `watch.secrets` is false; set `rbac.mode: full` +to also watch custom resources and kinds added later. Helm prints this warning on install and kwatch logs it once at startup. diff --git a/deploy/chart/README.md b/deploy/chart/README.md index c5a7db6ee..ddc758383 100644 --- a/deploy/chart/README.md +++ b/deploy/chart/README.md @@ -176,7 +176,7 @@ kubectl delete namespace kwatch | `networkPolicy.healthIngressFrom` | Extra peers (`namespaceSelector`, `podSelector` or `ipBlock`) that may reach the health port (`/health`, `/metrics`, `/status`), such as your Prometheus namespace. Without it only pods in the release namespace (and `kubeletCIDRs`) can | `[]` | | `image.repository` / `image.tag` / `image.pullPolicy` | Container image; an empty tag uses the chart's `appVersion` | `ghcr.io/abahmed/kwatch` / `""` / `IfNotPresent` | | `crd.install` | Install the shared `KwatchConfig` CRD with this release; skipped automatically when another release owns it. `config.crd.enabled` is separate: it turns the live overlay on | `true` | -| `rbac.mode` | `full` grants read-only `list`/`watch` on every resource (and `get` only on namespaces, nodes, `nodes/stats`, `nodes/metrics` and `pods/log`); `least-privilege` grants an explicit `list`/`watch` set, and kinds outside it show as `permission_denied` in `/health` coverage | `full` | +| `rbac.mode` | `least-privilege` grants an explicit read-only `list`/`watch` set, and kinds outside it (custom resources included) show as `permission_denied` in `/health` coverage; `full` grants `list`/`watch` on every resource. Both grant `get` only on namespaces, nodes, `nodes/stats`, `nodes/metrics` and `pods/log`. Set `full` to watch custom resources | `least-privilege` | | `persistence.size` | Size of the state volume (PVC). The state file is capped at 512 MiB of logical data (evidence 128 MiB), is rewritten at startup when it grows more than a quarter past that, and no backups are kept; the file and the temporary rewrite copy share the volume | `2Gi` | | `persistence.existingClaim` | Reuse an existing PVC for the state file | `""` | | `persistence.emptyDir` | Keep state in an `emptyDir` instead of a PVC. All state is lost when the Pod is rescheduled, so use it only for evaluation; installing with it prints a warning | `false` | diff --git a/deploy/chart/templates/NOTES.txt b/deploy/chart/templates/NOTES.txt index f042abf6b..8ca7a5548 100644 --- a/deploy/chart/templates/NOTES.txt +++ b/deploy/chart/templates/NOTES.txt @@ -1,7 +1,7 @@ {{- if eq (include "kwatch.watchSecrets" .) "true" }} WARNING: watch.secrets=true (the default). kwatch's ServiceAccount can read -every Secret in the cluster{{ if eq (.Values.rbac.mode | default "full") "full" }}, and rbac.mode=full grants list and watch on every resource kind{{ end }}. +every Secret in the cluster{{ if eq (.Values.rbac.mode | default "least-privilege") "full" }}, and rbac.mode=full grants list and watch on every resource kind{{ end }}. kwatch keeps only hashes of Secret data, never the values. To remove all Secret access set watch.secrets=false (--set watch.secrets=false); checks that need Secrets then report that they cannot verify. rbac.mode=least-privilege diff --git a/deploy/chart/templates/rbac.yaml b/deploy/chart/templates/rbac.yaml index ec4d4ee1c..476f90656 100644 --- a/deploy/chart/templates/rbac.yaml +++ b/deploy/chart/templates/rbac.yaml @@ -6,7 +6,7 @@ metadata: {{- include "kwatch.labels" . | nindent 4 }} rules: {{- $secrets := eq (include "kwatch.watchSecrets" .) "true" }} -{{- if and (eq (.Values.rbac.mode | default "full") "full") $secrets }} +{{- if and (eq (.Values.rbac.mode | default "least-privilege") "full") $secrets }} # rbac.mode=full: list and watch on every resource so the dynamic source # can watch any API kind. Secret values are hashed in the informer # transform and never stored. The only write is the audit's diff --git a/deploy/chart/test_helm.sh b/deploy/chart/test_helm.sh index 53f606358..966fd3c87 100755 --- a/deploy/chart/test_helm.sh +++ b/deploy/chart/test_helm.sh @@ -254,8 +254,12 @@ grep -Fq -- "- {}" <<<"$OUT_NP_ALL" || { echo "PASS: optional network policy" echo "=== rbac modes ===" -grep -Fq 'resources: ["*"]' <<<"$OUT1" || { - echo "FAIL: default rbac.mode must be full"; exit 1; +if grep -Fq 'resources: ["*"]' <<<"$OUT1"; then + echo "FAIL: default rbac.mode must be least-privilege"; exit 1; +fi +OUT_FULL=$(helm template test-full . --set rbac.mode=full 2>&1) +grep -Fq 'resources: ["*"]' <<<"$OUT_FULL" || { + echo "FAIL: rbac.mode=full must use wildcard list access"; exit 1; } OUT_LP=$(helm template test-lp . --set rbac.mode=least-privilege 2>&1) if grep -Fq 'resources: ["*"]' <<<"$OUT_LP"; then @@ -269,7 +273,7 @@ done if helm template test-bad . --set rbac.mode=bogus >/dev/null 2>&1; then echo "FAIL: invalid rbac.mode was accepted"; exit 1; fi -for out in "$OUT1" "$OUT_LP"; do +for out in "$OUT1" "$OUT_FULL" "$OUT_LP"; do grep -Fq '"nodes/stats", "nodes/metrics"' <<<"$out" || { echo "FAIL: kubelet stats RBAC is missing"; exit 1; } diff --git a/deploy/chart/values.schema.json b/deploy/chart/values.schema.json index 76282ca5a..16a939c58 100644 --- a/deploy/chart/values.schema.json +++ b/deploy/chart/values.schema.json @@ -25,8 +25,8 @@ "full", "least-privilege" ], - "default": "full", - "description": "full grants read-only access to every resource; least-privilege grants an explicit list/watch set" + "default": "least-privilege", + "description": "least-privilege (default) grants an explicit list/watch set; full grants read-only list/watch on every resource" } }, "additionalProperties": false diff --git a/deploy/chart/values.yaml b/deploy/chart/values.yaml index a474023ba..d6a3cbb14 100644 --- a/deploy/chart/values.yaml +++ b/deploy/chart/values.yaml @@ -29,19 +29,22 @@ podSecurityContext: fsGroup: 1000 rbac: - # -- Cluster read access. "full" grants list/watch on every resource so - # any API kind can be watched, and get only on namespaces, nodes, - # nodes/stats and nodes/metrics (kubelet stats, read from each kubelet - # directly) and pods/log (crash output); a wildcard get is avoided - # because it would also match pods/exec and similar subresources. - # nodes/proxy is never granted. "least-privilege" grants an explicit - # list/watch set matching the built-in sources and skips other kinds. - # Both modes also grant get on pods in the release namespace only, to - # explain why the previous kwatch Pod restarted, and get/update only on - # kwatch's own state lock Lease. The license instance key is the one - # Secret exception: get/create/update on the name kwatch-license-instance - # in the release namespace, in both modes and even with watch.secrets=false. - mode: full + # -- Cluster read access. "least-privilege" (the default) grants an + # explicit list/watch set matching the built-in sources; kinds outside + # it, custom resources included, show as permission_denied in /health + # coverage. "full" grants list/watch on every resource so any API kind, + # including CRDs installed later, can be watched. Both modes grant get + # only on namespaces, nodes, nodes/stats and nodes/metrics (kubelet + # stats, read from each kubelet directly) and pods/log (crash output); + # a wildcard get is avoided because it would also match pods/exec and + # similar subresources. nodes/proxy is never granted. Both modes also + # grant get on pods in the release namespace only, to explain why the + # previous kwatch Pod restarted, and get/update only on kwatch's own + # state lock Lease. The license instance key is the one Secret + # exception: get/update on the name kwatch-license-instance in the + # release namespace, in both modes and even with watch.secrets=false. + # Opt back into the wildcard with --set rbac.mode=full. + mode: least-privilege watch: # -- Watch Secrets. They feed checks such as missing Secret references diff --git a/docs/configuration.md b/docs/configuration.md index 167c650db..6c31e2b14 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -443,8 +443,8 @@ kwatch never needs write access to monitored workloads. The chart value | Mode | What it grants | |:---|---| -| `full` (default, and the raw `deploy/deploy.yaml`) | `list` and `watch` on every resource in every API group, so any built-in kind and any CRD, including ones installed later, can be watched | -| `least-privilege` | An explicit `list`/`watch` set for the kinds kwatch understands. Kinds outside it are reported as `permission_denied` in `/health` coverage, detectors that need them stay silent, and custom resources are not covered | +| `full` (the raw `deploy/deploy.yaml`; opt in with `--set rbac.mode=full`) | `list` and `watch` on every resource in every API group, so any built-in kind and any CRD, including ones installed later, can be watched | +| `least-privilege` (chart default) | An explicit `list`/`watch` set for the kinds kwatch understands. Kinds outside it are reported as `permission_denied` in `/health` coverage, detectors that need them stay silent, and custom resources are not covered | In both modes: diff --git a/docs/contributing-source.md b/docs/contributing-source.md index e634ec65a..6c2128912 100644 --- a/docs/contributing-source.md +++ b/docs/contributing-source.md @@ -22,8 +22,8 @@ else reads the model. Real example: `LimitRangeSchema` in watches it in `status` or `metadata` mode automatically; change `metadataResources` or the coverage entry in `coverage_catalog.go` to choose the mode. -4. **Check RBAC and docs.** The default ClusterRole already allows `list` and - `watch` on every resource; the least-privilege role in +4. **Check RBAC and docs.** The `full` ClusterRole already allows `list` and + `watch` on every resource; the least-privilege role (the chart default) in `deploy/chart/templates/rbac.yaml` lists kinds by name, so add the new one there. Then regenerate the coverage page: `go run ./cmd/coveragedocs`. diff --git a/docs/production-operations.md b/docs/production-operations.md index 79e881d06..e9383ce39 100644 --- a/docs/production-operations.md +++ b/docs/production-operations.md @@ -553,7 +553,10 @@ delivery failures, and dead letters. The chart value `rbac.mode` selects the cluster read access: -- `full` (default, and the raw `deploy/deploy.yaml` manifest) grants +- `least-privilege` is the chart default; see below. Set + `rbac.mode=full` to watch custom resources and kinds added later. +- `full` (the raw `deploy/deploy.yaml` manifest, and the chart with + `--set rbac.mode=full`) grants `list` and `watch` on every resource in every API group, so the dynamic source can watch any built-in or custom kind, including CRDs installed later. `get` is granted only on namespaces, nodes, `nodes/stats`, diff --git a/docs/upgrading-to-1.0.md b/docs/upgrading-to-1.0.md index 440b4b130..65e9770b4 100644 --- a/docs/upgrading-to-1.0.md +++ b/docs/upgrading-to-1.0.md @@ -140,10 +140,13 @@ list of metrics is in the [configuration reference](./configuration.md). The chart and `deploy/deploy.yaml` replace the old ClusterRole. -- `rbac.mode` is `full` by default: `list` and `watch` on every resource in - every API group, so any built-in or custom kind can be understood. - `least-privilege` grants an explicit set; kinds outside it show as - `permission_denied` in `/health`. +- The chart's `rbac.mode` is `least-privilege` by default: an explicit + `list`/`watch` set for the kinds kwatch understands. Kinds outside it, + custom resources included, show as `permission_denied` in `/health` and + their detectors stay silent. To watch custom resources and kinds added + later, opt back into the wildcard with `--set rbac.mode=full` (`list` + and `watch` on every resource in every API group). The raw + `deploy/deploy.yaml` manifest keeps the `full` role. - `get` is granted only on `namespaces`, `nodes`, `nodes/stats`, `nodes/metrics` and `pods/log`. `nodes/proxy` is no longer granted. - Secrets are watched by default, which lets the ServiceAccount read every From 3d9d574aaaa1a3e1126346ef0020463b91abde51 Mon Sep 17 00:00:00 2001 From: Abdelrahman Ahmed Date: Sun, 18 Oct 2026 13:31:46 +0300 Subject: [PATCH 2/2] Render the full-RBAC test with rbac.mode=full; fix a flaky saver test TestHelmFullRBACIsReadOnly rendered the chart defaults, which are now least-privilege; it now sets rbac.mode=full, and a new test checks that the default is least-privilege. TestThreadSaverFlushTimeoutDoesNotWriteAfterClose could hang: with a 1ms flush timeout, Save saw the expired context before calling the snapshot, so the test waited forever for it. A first save now holds the lock inside the snapshot, so the Flush always times out whatever the scheduling. --- internal/app/thread_state_test.go | 9 ++++++++- internal/architecture/rbac_manifest_test.go | 8 ++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/internal/app/thread_state_test.go b/internal/app/thread_state_test.go index b4848b405..9ded61e44 100644 --- a/internal/app/thread_state_test.go +++ b/internal/app/thread_state_test.go @@ -205,6 +205,10 @@ func (b *blockingThreads) RestoreThreads(map[string]map[string]string) {} // A Flush that times out leaves its save running. Closing the saver must // stop that save from writing, and a concurrent save must not race it. +// +// A first save is held inside the snapshot while it owns the lock, so the +// Flush's own save always waits on the lock and the Flush always times +// out. The test does not depend on when the Flush goroutine is scheduled. func TestThreadSaverFlushTimeoutDoesNotWriteAfterClose(t *testing.T) { disk := threadDisk(t) source := &blockingThreads{ @@ -213,13 +217,16 @@ func TestThreadSaverFlushTimeoutDoesNotWriteAfterClose(t *testing.T) { saver := newThreadSaver(source, disk) saver.flushTimeout = time.Millisecond - require.False(t, saver.Flush(context.Background())) + held := make(chan error, 1) + go func() { held <- saver.Save(context.Background()) }() <-source.entered + require.False(t, saver.Flush(context.Background())) saver.Close() concurrent := make(chan error, 1) go func() { concurrent <- saver.Save(context.Background()) }() close(source.release) + require.ErrorIs(t, <-held, errThreadSaverClosed) require.ErrorIs(t, <-concurrent, errThreadSaverClosed) var rec threadRecord found, err := disk.getThreads(&rec) diff --git a/internal/architecture/rbac_manifest_test.go b/internal/architecture/rbac_manifest_test.go index 9a3c8e8fc..26642204e 100644 --- a/internal/architecture/rbac_manifest_test.go +++ b/internal/architecture/rbac_manifest_test.go @@ -222,6 +222,14 @@ func TestRawManifestRBACIsReadOnly(t *testing.T) { func TestHelmFullRBACIsReadOnly(t *testing.T) { assertFullRBACIsReadOnly(t, + renderHelm(t, "../../deploy/chart", "--namespace", "kwatch", + "--set", "rbac.mode=full"), + "kwatch", helmLease) +} + +// The chart's default install is least-privilege, never the wildcard. +func TestHelmDefaultRBACIsLeastPrivilege(t *testing.T) { + assertLeastPrivilegeRBAC(t, renderHelm(t, "../../deploy/chart", "--namespace", "kwatch"), "kwatch", helmLease) }