diff --git a/.coverage-baseline b/.coverage-baseline index add29667..641c13bc 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -75.4 +75.5 diff --git a/README.md b/README.md index df96aa94..4cab3268 100644 --- a/README.md +++ b/README.md @@ -233,6 +233,7 @@ Start with the stable docs surface: - [`docs/configuration.md`](docs/configuration.md) - [`docs/attribution-setup-guide.md`](docs/attribution-setup-guide.md) - [`docs/security-model.md`](docs/security-model.md) +- [`docs/rbac.md`](docs/rbac.md): the two ClusterRoles, and how to stop the reverser enumerating Secrets - [`docs/commit-signing.md`](docs/commit-signing.md) - [`docs/github-setup-guide.md`](docs/github-setup-guide.md) - [`docs/sops-age-guide.md`](docs/sops-age-guide.md) diff --git a/charts/gitops-reverser/README.md b/charts/gitops-reverser/README.md index a52c8191..293cea85 100644 --- a/charts/gitops-reverser/README.md +++ b/charts/gitops-reverser/README.md @@ -162,6 +162,9 @@ nodeSelector: |-----------|-------------|---------| | `replicaCount` | Number of controller replicas (can't be higher than 1 for now, sorry) | `1` | | `image.repository` | Container image repository | `ghcr.io/configbutler/gitops-reverser` | +| `env` | Extra container env vars, as Kubernetes `EnvVar` entries | `[]` | +| `volumes` / `volumeMounts` | Extra pod volumes and their mounts, appended as-is | `[]` | +| `servers.enableHTTP2` | Serve HTTP/2 on the TLS servers. Off by default: disabling it mitigates the HTTP/2 Rapid-Reset CVE class | `false` | | `servers.audit.bindAddress` | host:port the audit ingress server binds to (`--audit-bind-address`) | `0.0.0.0:9444` | | `servers.audit.port` | Audit container/Service port; must match the port in `bindAddress` | `9444` | | `servers.audit.tls.enabled` | Serve audit ingress with TLS | `true` | @@ -185,11 +188,16 @@ nodeSelector: | `queue.redis.auth.existingSecret` | Name of a pre-created Secret holding the Redis password (only used when `queue.redis.addr` is set) | `""` | | `queue.redis.auth.existingSecretKey` | Key within the Secret that holds the password | `password` | | `queue.redis.auth.username` | Optional Redis ACL username | `""` | +| `queue.redis.db` | Redis logical database index. Redis offers 16; use `queue.redis.keyPrefix` to go past that | `0` | +| `queue.redis.keyPrefix` | Root of every key this release writes (watch cursors, attribution facts, command author records). Give each reverser its own prefix to share one Redis/Valkey between more reversers than `db` can separate. Changing it orphans the previous prefix's keys: cursors cold-replay once, which is safe. Allowed: `[A-Za-z0-9]`, `-`, `_`, `.`, `:` | `gitops-reverser` | | `queue.redis.tls.enabled` | Enable TLS for Redis connection | `false` | | `attribution.enabled` | Run audit ingress and name mirrored-resource commit authors from matching kube-apiserver audit facts | `false` | | `attribution.ttl` | How long an attribution fact is retained waiting for the matching watch event to join it | `10m` | | `attribution.grace` | Bounded per-event wait for a matching audit fact before a watch event ships as the committer | `3s` | | `servers.admission.enabled` | Install the validate-operator-types admission webhook that captures CommitRequest authors (a form of author attribution). Enabled by default; a no-op until `queue.redis.addr` is set | `true` | +| `rbac.create` | Create the manager ClusterRole and its binding | `true` | +| `rbac.watchTypes.mode` | Which types a `WatchRule` may read. `any` grants cluster-wide read on everything — convenient, but the reverser can then read every Secret in the cluster. `selected` grants read on `rbac.watchTypes.selected` only, so the reverser cannot list or watch Secrets (it keeps `get` on named Secrets it is pointed at). See [`docs/rbac.md`](../../docs/rbac.md) | `any` | +| `rbac.watchTypes.selected` | Types to grant when `mode: selected`, as `{apiGroups, resources}` entries (verbs are always `get,list,watch`). Required and non-empty in that mode; `namespaces`, `customresourcedefinitions` and `apiservices` come from the manager role and must not be restated | `[]` | | `servers.metrics.bindAddress` | Metrics listener bind address | `:8080` | | `servers.metrics.tls.enabled` | Serve metrics with TLS | `false` | | `servers.metrics.tls.certPath` | Metrics TLS certificate mount path | `/tmp/k8s-metrics-server/metrics-server-certs` | diff --git a/charts/gitops-reverser/templates/deployment.yaml b/charts/gitops-reverser/templates/deployment.yaml index 66a9a755..35d0fa07 100644 --- a/charts/gitops-reverser/templates/deployment.yaml +++ b/charts/gitops-reverser/templates/deployment.yaml @@ -42,6 +42,9 @@ spec: imagePullPolicy: {{ .Values.image.pullPolicy }} args: - --health-probe-bind-address={{ .Values.servers.healthProbe.bindAddress }} + {{- if .Values.servers.enableHTTP2 }} + - --enable-http2 + {{- end }} - --metrics-bind-address={{ .Values.servers.metrics.bindAddress }} {{- if not .Values.servers.metrics.tls.enabled }} - --metrics-insecure diff --git a/charts/gitops-reverser/templates/rbac.yaml b/charts/gitops-reverser/templates/rbac.yaml index 786f1d73..a433ddea 100644 --- a/charts/gitops-reverser/templates/rbac.yaml +++ b/charts/gitops-reverser/templates/rbac.yaml @@ -25,6 +25,63 @@ roleRef: kind: ClusterRole name: {{ include "gitops-reverser.fullname" . }}-manager-role subjects: +- kind: ServiceAccount + name: {{ include "gitops-reverser.serviceAccountName" . }} + namespace: {{ .Release.Namespace }} +{{/* + mode, the shape of each `selected` entry, and "selected must not be empty" are all enforced + by values.schema.json, which helm checks on template, lint, install and upgrade. Keeping the + rules there rather than as `fail` calls means one statement of the contract, and errors that + name the offending path. +*/}} +{{- $watchTypes := .Values.rbac.watchTypes | default dict }} +{{- $mode := $watchTypes.mode | default "any" }} +{{- $selected := $watchTypes.selected | default list }} +--- +# The read access a WatchRule needs. Separate from the manager role because RBAC is additive: +# folded in, a wildcard here would grant cluster-wide Secret list/watch however narrow the +# Secret rule beside it is. Verbs are always get/list/watch — the reverser never writes to a +# watched type. See docs/rbac.md. +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: {{ include "gitops-reverser.fullname" . }}-watch-{{ $mode }} + labels: + {{- include "gitops-reverser.labels" . | nindent 4 }} +rules: +{{- if eq $mode "any" }} +- apiGroups: + - '*' + resources: + - '*' + verbs: + - get + - list + - watch +{{- else }} +{{- range $rule := $selected }} +- apiGroups: + {{- toYaml $rule.apiGroups | nindent 4 }} + resources: + {{- toYaml $rule.resources | nindent 4 }} + verbs: + - get + - list + - watch +{{- end }} +{{- end }} +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: {{ include "gitops-reverser.fullname" . }}-watch-{{ $mode }} + labels: + {{- include "gitops-reverser.labels" . | nindent 4 }} +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: {{ include "gitops-reverser.fullname" . }}-watch-{{ $mode }} +subjects: - kind: ServiceAccount name: {{ include "gitops-reverser.serviceAccountName" . }} namespace: {{ .Release.Namespace }} diff --git a/charts/gitops-reverser/values.schema.json b/charts/gitops-reverser/values.schema.json new file mode 100644 index 00000000..f085fa48 --- /dev/null +++ b/charts/gitops-reverser/values.schema.json @@ -0,0 +1,532 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "title": "gitops-reverser values", + "description": "Helm validates these on template, lint, install and upgrade. Objects the chart owns are closed (additionalProperties: false) so a typo fails the install instead of silently doing nothing; Kubernetes pass-through objects (securityContext, affinity, resources, volumes, ...) stay open, because their shape belongs to Kubernetes and not to this chart.", + "type": "object", + "additionalProperties": false, + "$defs": { + "duration": { + "type": "string", + "pattern": "^([0-9]+(\\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$", + "description": "Go duration, e.g. 30s, 10m, 87600h." + }, + "port": { + "type": "integer", + "minimum": 1, + "maximum": 65535 + }, + "secretNameOverride": { + "type": "string", + "description": "Leave empty to use the chart-generated Secret name." + }, + "intOrString": { + "type": ["integer", "string"] + }, + "stringMap": { + "type": "object", + "additionalProperties": { "type": "string" } + }, + "objectList": { + "type": "array", + "items": { "type": "object" } + } + }, + "properties": { + "global": { + "type": "object", + "description": "Helm's cross-chart values. Free-form: this chart reads none of it, but a parent chart may set it." + }, + + "replicaCount": { "type": "integer", "minimum": 0 }, + + "deploymentStrategy": { + "type": "object", + "additionalProperties": false, + "properties": { + "type": { "enum": ["RollingUpdate", "Recreate"] }, + "rollingUpdate": { + "type": "object", + "additionalProperties": false, + "properties": { + "maxSurge": { "$ref": "#/$defs/intOrString" }, + "maxUnavailable": { "$ref": "#/$defs/intOrString" } + } + } + } + }, + + "image": { + "type": "object", + "additionalProperties": false, + "properties": { + "repository": { "type": "string", "minLength": 1 }, + "pullPolicy": { "enum": ["Always", "IfNotPresent", "Never"] }, + "tag": { "type": "string" } + } + }, + + "imagePullSecrets": { "$ref": "#/$defs/objectList" }, + "nameOverride": { "type": "string" }, + "fullnameOverride": { "type": "string" }, + "createNamespace": { "type": "boolean" }, + + "serviceAccount": { + "type": "object", + "additionalProperties": false, + "properties": { + "create": { "type": "boolean" }, + "annotations": { "$ref": "#/$defs/stringMap" }, + "name": { "type": "string" } + } + }, + + "podAnnotations": { "$ref": "#/$defs/stringMap" }, + "podSecurityContext": { "type": "object" }, + "securityContext": { "type": "object" }, + + "env": { + "$ref": "#/$defs/objectList", + "description": "Extra container env vars, as Kubernetes EnvVar entries." + }, + "volumes": { "$ref": "#/$defs/objectList" }, + "volumeMounts": { "$ref": "#/$defs/objectList" }, + + "controllerManager": { + "type": "object", + "additionalProperties": false, + "properties": { + "additionalSensitiveResources": { + "type": "array", + "items": { "type": "string" }, + "description": "Resource or group/resource, e.g. core.cozystack.io/tenantsecrets." + } + } + }, + + "certManager": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "issuer": { + "type": "object", + "additionalProperties": false, + "properties": { + "name": { "type": "string", "minLength": 1 }, + "kind": { "enum": ["Issuer", "ClusterIssuer"] }, + "create": { "type": "boolean" } + } + } + } + }, + + "servers": { + "type": "object", + "additionalProperties": false, + "properties": { + "enableHTTP2": { + "type": "boolean", + "description": "Serve HTTP/2 on the TLS servers. Off by default: it mitigates the HTTP/2 Rapid-Reset CVE class." + }, + "healthProbe": { + "type": "object", + "additionalProperties": false, + "properties": { + "bindAddress": { "type": "string" }, + "port": { "$ref": "#/$defs/port" } + } + }, + "metrics": { + "type": "object", + "additionalProperties": false, + "properties": { + "bindAddress": { "type": "string" }, + "port": { "$ref": "#/$defs/port" }, + "tls": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "certManager": { "type": "boolean" }, + "certPath": { "type": "string" }, + "certName": { "type": "string" }, + "certKey": { "type": "string" }, + "secretNameOverride": { "$ref": "#/$defs/secretNameOverride" } + } + } + } + }, + "admission": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "bindAddress": { "type": "string" }, + "port": { "$ref": "#/$defs/port" }, + "timeoutSeconds": { + "type": "integer", + "minimum": 1, + "maximum": 30, + "description": "The apiserver rejects a webhook timeout outside 1-30s." + }, + "tls": { + "type": "object", + "additionalProperties": false, + "properties": { + "certManager": { "type": "boolean" }, + "certPath": { "type": "string" }, + "certName": { "type": "string" }, + "certKey": { "type": "string" }, + "secretNameOverride": { "$ref": "#/$defs/secretNameOverride" } + } + } + } + }, + "audit": { + "type": "object", + "additionalProperties": false, + "properties": { + "bindAddress": { "type": "string" }, + "port": { "$ref": "#/$defs/port" }, + "timeouts": { + "type": "object", + "additionalProperties": false, + "properties": { + "read": { "$ref": "#/$defs/duration" }, + "write": { "$ref": "#/$defs/duration" }, + "idle": { "$ref": "#/$defs/duration" } + } + }, + "maxRequestBodyBytes": { "type": "integer", "minimum": 1 }, + "tls": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "certManager": { "type": "boolean" }, + "certPath": { "type": "string" }, + "certName": { "type": "string" }, + "certKey": { "type": "string" }, + "clientCAPath": { "type": "string" }, + "clientCAName": { "type": "string" }, + "secretNameOverride": { "$ref": "#/$defs/secretNameOverride" }, + "rootCA": { + "type": "object", + "additionalProperties": false, + "properties": { + "secretNameOverride": { "$ref": "#/$defs/secretNameOverride" } + } + }, + "client": { + "type": "object", + "additionalProperties": false, + "properties": { + "duration": { "$ref": "#/$defs/duration" }, + "renewBefore": { "$ref": "#/$defs/duration" }, + "secretNameOverride": { "$ref": "#/$defs/secretNameOverride" } + } + } + } + } + } + } + } + }, + + "queue": { + "type": "object", + "additionalProperties": false, + "properties": { + "redis": { + "type": "object", + "additionalProperties": false, + "properties": { + "addr": { "type": "string", "description": "host:port. Empty runs without Redis." }, + "auth": { + "type": "object", + "additionalProperties": false, + "properties": { + "existingSecret": { "type": "string" }, + "existingSecretKey": { "type": "string" }, + "username": { "type": "string" } + } + }, + "db": { "type": "integer", "minimum": 0 }, + "keyPrefix": { + "type": "string", + "maxLength": 128, + "pattern": "^[A-Za-z0-9._:-]*$", + "description": "Root of every key this release writes. The binary validates it too, and rejects a prefix that is empty once trailing ':' is trimmed." + }, + "tls": { + "type": "object", + "additionalProperties": false, + "properties": { "enabled": { "type": "boolean" } } + } + } + } + } + }, + + "attribution": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "ttl": { "$ref": "#/$defs/duration" }, + "grace": { "$ref": "#/$defs/duration" } + } + }, + + "rbac": { + "type": "object", + "additionalProperties": false, + "properties": { + "create": { + "type": "boolean", + "description": "Create the manager ClusterRole and its binding." + }, + "watchTypes": { + "type": "object", + "additionalProperties": false, + "description": "Which types a WatchRule may read. The reverser never writes to a watched type.", + "properties": { + "mode": { + "enum": ["any", "selected"], + "description": "any: cluster-wide read on everything, including every Secret. selected: read only the types listed below." + }, + "selected": { + "type": "array", + "description": "Types to grant when mode is selected. Verbs are always get/list/watch.", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["apiGroups", "resources"], + "properties": { + "apiGroups": { + "type": "array", + "minItems": 1, + "items": { "type": "string" }, + "description": "API groups; use [\"\"] for the core group." + }, + "resources": { + "type": "array", + "minItems": 1, + "items": { "type": "string" } + } + } + } + } + }, + "if": { + "properties": { "mode": { "const": "selected" } }, + "required": ["mode"] + }, + "then": { + "required": ["selected"], + "properties": { + "selected": { + "minItems": 1, + "description": "mode=selected with no entries would leave the reverser able to watch nothing." + } + } + } + } + } + }, + + "resources": { "type": "object" }, + "nodeSelector": { "$ref": "#/$defs/stringMap" }, + "tolerations": { "$ref": "#/$defs/objectList" }, + "affinity": { "type": "object" }, + + "podDisruptionBudget": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "minAvailable": { "$ref": "#/$defs/intOrString" }, + "maxUnavailable": { "$ref": "#/$defs/intOrString" } + } + }, + + "monitoring": { + "type": "object", + "additionalProperties": false, + "properties": { + "serviceMonitor": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "namespace": { "type": "string" }, + "labels": { "$ref": "#/$defs/stringMap" }, + "interval": { "$ref": "#/$defs/duration" }, + "scrapeTimeout": { "$ref": "#/$defs/duration" }, + "path": { "type": "string" }, + "port": { "type": "string" }, + "scheme": { + "enum": ["http", "https"], + "description": "Must match servers.metrics.tls.enabled." + } + } + } + } + }, + + "service": { + "type": "object", + "additionalProperties": false, + "properties": { + "clusterIP": { "type": "string" }, + "ports": { + "type": "object", + "additionalProperties": false, + "properties": { + "audit": { "$ref": "#/$defs/port" }, + "metrics": { "$ref": "#/$defs/port" } + } + } + } + }, + + "auditService": { + "type": "object", + "additionalProperties": false, + "properties": { + "type": { "enum": ["ClusterIP", "NodePort", "LoadBalancer"] }, + "nodePort": { "type": "integer", "minimum": 30000, "maximum": 32767 }, + "clusterIP": { "type": "string" } + } + }, + + "auditKubeconfig": { + "type": "object", + "additionalProperties": false, + "properties": { "insecureSkipTLSVerify": { "type": "boolean" } } + }, + + "quickstart": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "namespace": { "type": "string" }, + "createNamespace": { "type": "boolean" }, + "gitProvider": { + "type": "object", + "additionalProperties": false, + "properties": { + "name": { "type": "string" }, + "url": { "type": "string" }, + "allowedBranches": { "type": "array", "items": { "type": "string" } }, + "secretRef": { + "type": "object", + "additionalProperties": false, + "properties": { "name": { "type": "string" } } + }, + "push": { + "type": "object", + "additionalProperties": false, + "properties": { "commitWindow": { "$ref": "#/$defs/duration" } } + } + } + }, + "gitTarget": { + "type": "object", + "additionalProperties": false, + "properties": { + "name": { "type": "string" }, + "branch": { "type": "string" }, + "path": { "type": "string", "minLength": 1 }, + "encryption": { + "type": "object", + "additionalProperties": false, + "properties": { + "provider": { "type": "string" }, + "age": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "generateWhenMissing": { "type": "boolean" } + } + }, + "secretRef": { + "type": "object", + "additionalProperties": false, + "properties": { "name": { "type": "string" } } + } + } + } + } + }, + "watchRule": { + "type": "object", + "additionalProperties": false, + "properties": { + "name": { "type": "string" }, + "rules": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "properties": { + "operations": { + "type": "array", + "minItems": 1, + "items": { "enum": ["CREATE", "UPDATE", "DELETE"] } + }, + "apiGroups": { "type": "array", "items": { "type": "string" } }, + "apiVersions": { "type": "array", "items": { "type": "string" } }, + "resources": { "type": "array", "items": { "type": "string" } } + } + } + } + } + } + }, + "if": { + "properties": { "enabled": { "const": true } }, + "required": ["enabled"] + }, + "then": { + "properties": { + "gitProvider": { + "properties": { "url": { "minLength": 1 } }, + "required": ["url"] + } + }, + "required": ["gitProvider"] + } + }, + + "logging": { + "type": "object", + "additionalProperties": false, + "properties": { + "level": { + "enum": ["debug", "info", "error", "panic"], + "description": "zap levels. `warn` is NOT one of them: the binary rejects it and the pod crash-loops." + }, + "development": { "type": "boolean" }, + "encoder": { "enum": ["json", "console"] }, + "stacktraceLevel": { + "enum": ["info", "error", "panic"], + "description": "zap stacktrace levels; `debug` is not accepted here." + } + } + }, + + "labels": { + "type": "object", + "additionalProperties": false, + "properties": { + "managedBy": { + "type": "string", + "description": "Set to kubectl only for the generated install.yaml; leave as Helm otherwise." + } + } + } + } +} diff --git a/charts/gitops-reverser/values.yaml b/charts/gitops-reverser/values.yaml index 36be158c..142063b2 100644 --- a/charts/gitops-reverser/values.yaml +++ b/charts/gitops-reverser/values.yaml @@ -33,6 +33,12 @@ serviceAccount: podAnnotations: {} +# Extension points the Deployment appends as-is. Declared here because the template already +# reads them; without an entry they are invisible to anyone reading values.yaml. +env: [] # Kubernetes EnvVar entries, e.g. [{name: REDIS_PASSWORD, valueFrom: {...}}] +volumes: [] +volumeMounts: [] + podSecurityContext: runAsNonRoot: true runAsUser: 65532 @@ -211,6 +217,24 @@ attribution: # RBAC configuration rbac: create: true + # Which types the reverser may read. It never writes to a watched type, so verbs are always + # get/list/watch. Shape is enforced by values.schema.json; see docs/rbac.md. + # + # any cluster-wide read on everything, so a WatchRule can name any type — including + # one installed later. It also means the reverser can list and read every Secret + # in the cluster, credentials that have nothing to do with it included. + # selected read only the types below, so the reverser cannot list or watch Secrets at all. + # (The manager role keeps `get` on Secrets: it still reads the git-creds and + # age-key Secrets a GitProvider/GitTarget names.) A WatchRule naming a type you + # did not grant will fail to watch it. namespaces, CRDs and apiservices come from + # the manager role — do not restate them. + watchTypes: + mode: any # any | selected + selected: [] + # - apiGroups: [""] # core group + # resources: ["configmaps"] + # - apiGroups: ["apps"] + # resources: ["deployments"] # Resource limits and requests resources: diff --git a/cmd/main.go b/cmd/main.go index 6a8e4511..4ccc8ef9 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -423,8 +423,9 @@ func parseFlagsWithArgs(fs *flag.FlagSet, args []string) (appConfig, error) { fs.StringVar(&cfg.redisAddr, "redis-addr", "valkey:6379", "Redis/Valkey address (host:port). Holds each GitTarget's watch resume cursors (state continuity) "+ "and, when author attribution is enabled, the attribution facts. Leave empty to run without "+ - "Redis: watches cold-replay on restart instead of resuming. Incompatible with "+ - "--author-attribution=true or --admission-webhook.") + "Redis: watches cold-replay on restart instead of resuming. Required by "+ + "--author-attribution=true. --admission-webhook still runs without it, but command-author "+ + "capture is a no-op: CommitRequests commit as the configured committer.") fs.BoolVar(&cfg.authorAttribution, "author-attribution", true, "Name the real actor (human or service account) who caused each change as the Git commit author, "+ "resolved from matching audit facts; this runs the audit webhook ingress (default true). When "+ diff --git a/cmd/main_chart_args_test.go b/cmd/main_chart_args_test.go index a4fc8867..e435bc5f 100644 --- a/cmd/main_chart_args_test.go +++ b/cmd/main_chart_args_test.go @@ -60,15 +60,25 @@ func renderManagerArgs(t *testing.T, setValues ...string) []string { // lints and packages cleanly. Parse what the chart actually emits. func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { tests := map[string]struct { - setValues []string - wantKeyPrefix string - wantRedisAddr string - wantRedisDBSet int - wantUsername string + setValues []string + wantKeyPrefix string + wantRedisAddr string + wantRedisDBSet int + wantUsername string + wantAttribution bool + wantAuditTLSCert string + wantHTTP2 bool }{ "chart defaults": { wantKeyPrefix: "gitops-reverser", }, + // servers.enableHTTP2 was a value no template read: setting it changed nothing, while + // its comment promised an HTTP/2 Rapid-Reset mitigation. Assert it reaches the binary. + "http2 enabled": { + setValues: []string{"servers.enableHTTP2=true"}, + wantKeyPrefix: "gitops-reverser", + wantHTTP2: true, + }, "nested tenant prefix": { setValues: []string{"queue.redis.keyPrefix=cell-a:tenant-7"}, wantKeyPrefix: "cell-a:tenant-7", @@ -85,6 +95,29 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { wantRedisDBSet: 3, wantUsername: "reverser", }, + // Attribution renders a whole block of audit-server flags the default path never emits, + // and it is the mode that validateAuditConfig scrutinises. Render it and parse it too. + "attribution enabled with audit TLS": { + setValues: []string{ + "attribution.enabled=true", + "queue.redis.addr=redis.example.com:6379", + }, + wantKeyPrefix: "gitops-reverser", + wantRedisAddr: "redis.example.com:6379", + wantAttribution: true, + wantAuditTLSCert: "/tmp/k8s-audit-server/audit-server-certs", + }, + // Attribution without audit TLS takes the --audit-insecure branch instead. + "attribution enabled without audit TLS": { + setValues: []string{ + "attribution.enabled=true", + "queue.redis.addr=redis.example.com:6379", + "servers.audit.tls.enabled=false", + }, + wantKeyPrefix: "gitops-reverser", + wantRedisAddr: "redis.example.com:6379", + wantAttribution: true, + }, } for name, tc := range tests { @@ -103,6 +136,12 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { // A quoted-value render would reach Redis as `"reverser"` and fail ACL auth. require.Equal(t, tc.wantUsername, cfg.redisUsername) } + require.Equal(t, tc.wantAttribution, cfg.authorAttribution) + if tc.wantAuditTLSCert != "" { + require.Equal(t, tc.wantAuditTLSCert, cfg.auditCertPath) + } + require.Equal(t, tc.wantHTTP2, cfg.enableHTTP2, + "servers.enableHTTP2 must reach the binary, and default to off") }) } } diff --git a/cmd/main_chart_rbac_test.go b/cmd/main_chart_rbac_test.go new file mode 100644 index 00000000..f43b907c --- /dev/null +++ b/cmd/main_chart_rbac_test.go @@ -0,0 +1,262 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "os" + "os/exec" + "strings" + "testing" + + "github.com/stretchr/testify/require" + rbacv1 "k8s.io/api/rbac/v1" + "sigs.k8s.io/yaml" +) + +// rbacChartInputs are read in-process so `go test` treats them as cache inputs; helm reads +// the chart in a subprocess the test log cannot see. +var rbacChartInputs = []string{ + "../charts/gitops-reverser/values.yaml", + "../charts/gitops-reverser/values.schema.json", + "../charts/gitops-reverser/templates/rbac.yaml", + "../charts/gitops-reverser/config/role.yaml", +} + +// selectedConfigMapsAndDeployments is the least-privilege example the chart ships commented +// out: the core group's configmaps plus apps/deployments, and nothing else. +var selectedConfigMapsAndDeployments = []string{ + "rbac.watchTypes.mode=selected", + "rbac.watchTypes.selected[0].apiGroups[0]=", // "" is the core group + "rbac.watchTypes.selected[0].resources[0]=configmaps", + "rbac.watchTypes.selected[1].apiGroups[0]=apps", + "rbac.watchTypes.selected[1].resources[0]=deployments", +} + +func helmTemplateRBAC(setValues ...string) ([]byte, error) { + args := []string{ + "template", "gitops-reverser", "../charts/gitops-reverser", + "--show-only", "templates/rbac.yaml", + } + for _, sv := range setValues { + args = append(args, "--set", sv) + } + return exec.Command("helm", args...).CombinedOutput() +} + +// renderClusterRoles renders the chart's RBAC and returns every ClusterRole it produces. +func renderClusterRoles(t *testing.T, setValues ...string) []rbacv1.ClusterRole { + t.Helper() + + for _, path := range rbacChartInputs { + _, err := os.ReadFile(path) + require.NoErrorf(t, err, "%s missing; `task helm-sync` generates it", path) + } + + out, err := helmTemplateRBAC(setValues...) + require.NoErrorf(t, err, "helm template failed: %s", out) + + var roles []rbacv1.ClusterRole + for _, doc := range strings.Split(string(out), "\n---") { + if strings.TrimSpace(doc) == "" { + continue + } + var obj rbacv1.ClusterRole + require.NoError(t, yaml.Unmarshal([]byte(doc), &obj)) + if obj.Kind == "ClusterRole" { + roles = append(roles, obj) + } + } + require.NotEmpty(t, roles, "chart rendered no ClusterRole") + return roles +} + +func covers(values []string, want string) bool { + for _, v := range values { + if v == want || v == "*" { + return true + } + } + return false +} + +// grantsSecretListOrWatch reports the rules that let the holder enumerate Secrets cluster-wide. +// `get` is fine: the operator reads only the Secrets a GitProvider or GitTarget names. +func grantsSecretListOrWatch(role rbacv1.ClusterRole) []rbacv1.PolicyRule { + var bad []rbacv1.PolicyRule + for _, rule := range role.Rules { + if !covers(rule.APIGroups, "") || !covers(rule.Resources, "secrets") { + continue + } + if covers(rule.Verbs, "list") || covers(rule.Verbs, "watch") { + bad = append(bad, rule) + } + } + return bad +} + +func roleNamed(t *testing.T, roles []rbacv1.ClusterRole, suffix string) (rbacv1.ClusterRole, bool) { + t.Helper() + for _, r := range roles { + if strings.HasSuffix(r.Name, suffix) { + return r, true + } + } + return rbacv1.ClusterRole{}, false +} + +// The whole point of splitting the wildcard out: `selected` mode must leave a role set that +// cannot enumerate Secrets. A reverser mirroring two types on a management cluster would +// otherwise hold read access to every credential in it. +func TestChartRBAC_SelectedModeCannotEnumerateSecrets(t *testing.T) { + roles := renderClusterRoles(t, selectedConfigMapsAndDeployments...) + + _, found := roleNamed(t, roles, "-watch-any") + require.False(t, found, "the wildcard ClusterRole must not render in selected mode") + + for _, role := range roles { + require.Emptyf(t, grantsSecretListOrWatch(role), + "ClusterRole %q still grants Secret list/watch", role.Name) + } +} + +// selected grants exactly the listed types, read-only. Anything more is a privilege the user +// did not ask for; anything less and the WatchRule cannot do its job. +func TestChartRBAC_SelectedModeGrantsOnlyTheListedTypes(t *testing.T) { + roles := renderClusterRoles(t, selectedConfigMapsAndDeployments...) + + selected, found := roleNamed(t, roles, "-watch-selected") + require.True(t, found, "selected mode must render its ClusterRole") + require.Len(t, selected.Rules, 2) + + require.Equal(t, []string{""}, selected.Rules[0].APIGroups) + require.Equal(t, []string{"configmaps"}, selected.Rules[0].Resources) + require.Equal(t, []string{"apps"}, selected.Rules[1].APIGroups) + require.Equal(t, []string{"deployments"}, selected.Rules[1].Resources) + + for _, rule := range selected.Rules { + require.ElementsMatch(t, []string{"get", "list", "watch"}, rule.Verbs, + "a watched type is only ever read") + require.NotContains(t, rule.Resources, "*") + require.NotContains(t, rule.APIGroups, "*") + } +} + +// The manager role is generated from the kubebuilder markers. It must never carry the +// wildcard itself: RBAC is additive, so a wildcard there would re-grant Secret list/watch +// and no chart value could take it away. +func TestChartRBAC_ManagerRoleNeverCarriesTheWildcard(t *testing.T) { + modes := map[string][]string{ + "any": {"rbac.watchTypes.mode=any"}, + "selected": selectedConfigMapsAndDeployments, + } + for name, values := range modes { + t.Run(name, func(t *testing.T) { + roles := renderClusterRoles(t, values...) + manager, found := roleNamed(t, roles, "-manager-role") + require.True(t, found, "manager ClusterRole must always render") + + for _, rule := range manager.Rules { + require.NotContains(t, rule.Resources, "*", + "manager role must not grant wildcard resources (%v)", rule.APIGroups) + } + require.Empty(t, grantsSecretListOrWatch(manager), + "the manager role must never enumerate Secrets; it reads the ones it is pointed at") + }) + } +} + +// The API-resource catalog and its trigger informers read these three. Without them a +// least-privilege install 403s on every reflector retry, which is why a `selected` user must +// not have to restate them. +func TestChartRBAC_ManagerRoleGrantsTheAPISurfaceReads(t *testing.T) { + roles := renderClusterRoles(t, selectedConfigMapsAndDeployments...) + manager, found := roleNamed(t, roles, "-manager-role") + require.True(t, found) + + for _, want := range []struct{ group, resource string }{ + {"apiextensions.k8s.io", "customresourcedefinitions"}, + {"apiregistration.k8s.io", "apiservices"}, + {"", "namespaces"}, + } { + granted := false + for _, rule := range manager.Rules { + if covers(rule.APIGroups, want.group) && covers(rule.Resources, want.resource) && + covers(rule.Verbs, "list") && covers(rule.Verbs, "watch") { + granted = true + break + } + } + require.Truef(t, granted, "manager role must grant list+watch on %s/%s", want.group, want.resource) + } +} + +// Default installs keep working: a WatchRule may name any type, so `any` is the default. +func TestChartRBAC_WildcardRoleShipsByDefault(t *testing.T) { + roles := renderClusterRoles(t) + + wildcard, found := roleNamed(t, roles, "-watch-any") + require.True(t, found, "the wildcard ClusterRole must render by default") + require.Len(t, wildcard.Rules, 1) + require.Equal(t, []string{"*"}, wildcard.Rules[0].APIGroups) + require.Equal(t, []string{"*"}, wildcard.Rules[0].Resources) + require.ElementsMatch(t, []string{"get", "list", "watch"}, wildcard.Rules[0].Verbs) +} + +// A mis-set watchTypes must fail the render, not silently grant the wrong thing. Granting +// nothing is as bad as granting everything: one breaks the operator, the other the cluster. +// +// values.schema.json is what rejects these, and helm applies it to template, lint, install +// and upgrade alike. The template carries no equivalent `fail` guards, so these cases are the +// only thing standing between a typo and a ClusterRole nobody meant to create. +func TestChartRBAC_MisconfiguredWatchTypesFailsTheRender(t *testing.T) { + tests := map[string]struct { + setValues []string + wantErr string + }{ + "unknown mode": { + setValues: []string{"rbac.watchTypes.mode=readonly"}, + wantErr: "at '/rbac/watchTypes/mode': value must be one of 'any', 'selected'", + }, + "selected with no entries": { + setValues: []string{"rbac.watchTypes.mode=selected"}, + wantErr: "at '/rbac/watchTypes/selected': minItems: got 0, want 1", + }, + "entry without resources": { + setValues: []string{ + "rbac.watchTypes.mode=selected", + "rbac.watchTypes.selected[0].apiGroups[0]=apps", + }, + wantErr: "at '/rbac/watchTypes/selected/0': missing property 'resources'", + }, + "entry without apiGroups": { + setValues: []string{ + "rbac.watchTypes.mode=selected", + "rbac.watchTypes.selected[0].resources[0]=deployments", + }, + wantErr: "at '/rbac/watchTypes/selected/0': missing property 'apiGroups'", + }, + // Verbs are not the user's to choose: the reverser mirrors a type, it never writes one. + "entry setting verbs": { + setValues: []string{ + "rbac.watchTypes.mode=selected", + "rbac.watchTypes.selected[0].apiGroups[0]=apps", + "rbac.watchTypes.selected[0].resources[0]=deployments", + "rbac.watchTypes.selected[0].verbs[0]=create", + }, + wantErr: "at '/rbac/watchTypes/selected/0': additional properties 'verbs' not allowed", + }, + // A typo in a key name is the likeliest way to think you narrowed access when you did not. + "unknown key under watchTypes": { + setValues: []string{"rbac.watchTypes.modes=selected"}, + wantErr: "additional properties 'modes' not allowed", + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + out, err := helmTemplateRBAC(tc.setValues...) + require.Errorf(t, err, "render should have failed, got:\n%s", out) + require.Contains(t, string(out), tc.wantErr) + }) + } +} diff --git a/cmd/main_redis_flags_test.go b/cmd/main_redis_flags_test.go new file mode 100644 index 00000000..c2487750 --- /dev/null +++ b/cmd/main_redis_flags_test.go @@ -0,0 +1,56 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "flag" + "testing" + + "github.com/stretchr/testify/require" +) + +func parseArgs(t *testing.T, args ...string) (appConfig, error) { + t.Helper() + return parseFlagsWithArgs(flag.NewFlagSet("manager", flag.ContinueOnError), args) +} + +// The --redis-addr help text is a promise about which combinations start. Keep it true. +func TestParseFlags_RedisAddrIsOnlyRequiredByAttribution(t *testing.T) { + tests := map[string]struct { + args []string + wantErr string + }{ + "attribution needs redis": { + args: []string{"--redis-addr=", "--author-attribution=true"}, + wantErr: "redis-addr is required when author-attribution is enabled", + }, + // The webhook is failurePolicy: Ignore by design and the controller is the real gate, + // so running it without Redis is a supported, degraded mode — not a usage error. + // (--admission-webhook-cert-path has no default and is required whenever the webhook + // is on; that requirement is about TLS, not about Redis.) + "admission webhook runs without redis": { + args: []string{ + "--redis-addr=", "--author-attribution=false", + "--admission-webhook", "--admission-webhook-cert-path=/tmp/certs", + }, + }, + "admission webhook still requires its cert path": { + args: []string{"--redis-addr=", "--author-attribution=false", "--admission-webhook"}, + wantErr: "admission-webhook-cert-path is required", + }, + "configured-author runs without redis": { + args: []string{"--redis-addr=", "--author-attribution=false"}, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + _, err := parseArgs(t, tc.args...) + if tc.wantErr != "" { + require.ErrorContains(t, err, tc.wantErr) + return + } + require.NoError(t, err) + }) + } +} diff --git a/config/rbac/kustomization.yaml b/config/rbac/kustomization.yaml index 8f754254..5907796f 100644 --- a/config/rbac/kustomization.yaml +++ b/config/rbac/kustomization.yaml @@ -3,6 +3,10 @@ kind: Kustomization resources: - sa.yaml - role.yaml + # The wildcard read a WatchRule may need, kept out of role.yaml so it can be dropped whole. + # See docs/rbac.md; the Helm chart renders the same pair from `rbac.watchTypes`. + - watch-any-role.yaml + - watch-any-rolebinding.yaml - cluster-role-jane-aggregated.yaml - cluster-rolebinding.yaml - cluster-rolebinding-jane.yaml # E2E-only helper: allows impersonated user jane@acme.com writes in tests. diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 40ca498d..b1f41a80 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -19,14 +19,19 @@ rules: verbs: - create - get - - list - - patch - update +- apiGroups: + - apiextensions.k8s.io + resources: + - customresourcedefinitions + verbs: + - get + - list - watch - apiGroups: - - '*' + - apiregistration.k8s.io resources: - - '*' + - apiservices verbs: - get - list diff --git a/config/rbac/watch-any-role.yaml b/config/rbac/watch-any-role.yaml new file mode 100644 index 00000000..d2ddd24e --- /dev/null +++ b/config/rbac/watch-any-role.yaml @@ -0,0 +1,24 @@ +# The read access the watch manager needs for the types a WatchRule names. +# +# A WatchRule may name any type in the cluster, including types installed after this role, so +# this cannot be derived from the code the way every other rule in config/rbac/role.yaml is. +# It is kept OUT of that role on purpose: RBAC is additive, so a wildcard read here would +# grant cluster-wide Secret list/watch no matter how narrow the Secret rule beside it is. +# +# Drop this role (Helm: `rbac.watchTypes.mode: selected`) when you know which types the +# reverser will mirror, and grant get/list/watch on exactly those instead. The operator +# needs nothing else from it: it holds no long-lived informers on watched types, and it +# never reads a Secret it was not pointed at by name. +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: gitops-reverser-watch-any +rules: + - apiGroups: + - '*' + resources: + - '*' + verbs: + - get + - list + - watch diff --git a/config/rbac/watch-any-rolebinding.yaml b/config/rbac/watch-any-rolebinding.yaml new file mode 100644 index 00000000..26b6cd38 --- /dev/null +++ b/config/rbac/watch-any-rolebinding.yaml @@ -0,0 +1,15 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + labels: + app.kubernetes.io/managed-by: kustomize + app.kubernetes.io/name: gitops-reverser + name: gitops-reverser-watch-any +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: gitops-reverser-watch-any +subjects: +- kind: ServiceAccount + name: gitops-reverser + namespace: sut diff --git a/docs/README.md b/docs/README.md index a7d738b2..76823f3d 100644 --- a/docs/README.md +++ b/docs/README.md @@ -20,6 +20,7 @@ If you only want the supported product docs, start with the files below. - [`sops-age-guide.md`](sops-age-guide.md): Secret encryption with SOPS + age - [`security-model.md`](security-model.md): controller access, trust boundaries, and the Git credentials Secret shape +- [`rbac.md`](rbac.md): the two ClusterRoles, and how to stop the reverser enumerating Secrets - [`bi-directional.md`](bi-directional.md): safe shared-path and handoff patterns - [`alternatives.md`](alternatives.md): nearby tools and when another approach fits better - [`UPGRADING.md`](UPGRADING.md): breaking changes and migration steps, newest first @@ -37,7 +38,6 @@ docs surface: kept for historical context - [`ci/`](ci/): CI/devcontainer rationale and troubleshooting - [`future/`](future/): ideas that are intentionally deferred -- [`serious-bug/`](serious-bug/): worked bug reports against upstream projects - [`audit-setup/`](audit-setup/): cluster-specific audit delivery notes and examples These root-level files are also working notes rather than polished user docs: diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 69a97244..c5445289 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,56 @@ guidance that the changelog's breaking-change entries link to. We are pre-1.0, so breaking changes bump the **minor** version (release-please is configured with `bump-minor-pre-major`) rather than the major. Read the relevant entry before upgrading across it. +## Unreleased — the wildcard cluster read moved to its own, droppable ClusterRole (next minor; behavior change) + +The shipped RBAC now says what the binary actually does. Nothing is taken away from a default +install; what changes is that the parts can be separated. + +**The manager ClusterRole no longer contains `apiGroups: ["*"], resources: ["*"]`.** The types a +`WatchRule` may read now come from a ClusterRole of their own, rendered from the new +`rbac.watchTypes` and bound to the same ServiceAccount. The default (`mode: any`) reproduces the old +wildcard exactly, so the default install keeps the same effective permissions. The split exists +because RBAC is additive: while the wildcard sat in the manager role, no chart value could remove the +cluster-wide Secret read it implied. + +**The manager role's `secrets` rule narrowed from `get,list,watch,create,update,patch` to +`get,create,update`.** The operator has held no Secret informer since `v0.31.0` — Secrets are +excluded from the manager cache, so every read is a direct `get` of a Secret a `GitProvider` or +`GitTarget` names — but the marker was never updated to match. It never used `list`, `watch` or +`patch`. + +**The manager role gained explicit `get,list,watch` on `customresourcedefinitions` and +`apiservices`.** They were previously reachable only through the wildcard. The API-resource catalog +and its trigger informers read both. + +**Migration** + +- Default installs (`rbac.watchTypes` unset): no action. Helm creates `-watch-any` and its + binding; `kubectl apply` of `dist/install.yaml` includes the same pair. +- To run least-privilege, set `rbac.watchTypes.mode: selected` and list the types your `WatchRule`s + name. The chart renders the ClusterRole; verbs are always `get,list,watch`. See [`rbac.md`](rbac.md). + + ```yaml + rbac: + watchTypes: + mode: selected + selected: + - apiGroups: [""] + resources: ["configmaps"] + - apiGroups: ["apps"] + resources: ["deployments"] + ``` + +- If you hand-wrote a role because the shipped one was too broad, you can now drop it along with the + parts that duplicate `-manager-role`. + +**Related behavior change.** A trigger informer (`customresourcedefinitions`, `apiservices`) that +the API server serves but RBAC denies is now **stopped** after the first `403`, logged once, and +re-armed automatically on a later catalog refresh if the permission is granted. Previously the +reflector retried the denial forever. Discovery reports what the server serves, not what the caller +may read, so a narrowed role reached this path — which is exactly the path this release makes easy +to enter. + ## Unreleased — `manifest-analyzer` scan modes renamed, and `--format json` now emits a versioned contract (next minor; breaking) The analyzer's machine-readable output moved to the new public diff --git a/docs/configuration.md b/docs/configuration.md index c06b68cc..0f268b6f 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -695,6 +695,11 @@ queue: existingSecretKey: "password" ``` +Every key this operator writes is rooted at `queue.redis.keyPrefix` (`--redis-key-prefix`, default +`gitops-reverser`). Give each reverser its own prefix when several share one Redis/Valkey: `--redis-db` +separates only 16 logical databases, and one reverser per tenant or per branch environment passes that +long before it reaches any real Redis limit. + When attribution is enabled, these flags tune the join: - `--author-attribution-ttl` (default `10m`): how long an attribution fact is retained waiting for the diff --git a/docs/design/e2e-aggregated-apiserver-test-design.md b/docs/design/e2e-aggregated-apiserver-test-design.md index 3137dc51..e8a47b2b 100644 --- a/docs/design/e2e-aggregated-apiserver-test-design.md +++ b/docs/design/e2e-aggregated-apiserver-test-design.md @@ -280,7 +280,10 @@ are published per Kubernetes release; newer k3s versions may require building fr ### gitops-reverser RBAC: no changes needed -The existing ClusterRole at `config/rbac/role.yaml` already grants wildcard read access: +The default kustomize install still binds a wildcard read, but it no longer sits in +`config/rbac/role.yaml`: it moved to a ClusterRole of its own, +[`config/rbac/watch-any-role.yaml`](../../config/rbac/watch-any-role.yaml) (the Helm equivalent is +`rbac.watchTypes.mode: any`). ```yaml - apiGroups: ['*'] @@ -288,8 +291,9 @@ The existing ClusterRole at `config/rbac/role.yaml` already grants wildcard read verbs: [get, list, watch] ``` -This covers `wardle.example.com` flunders and fischers. No additional RBAC overlay is needed for -the gitops-reverser controller. +This covers `wardle.example.com` flunders and fischers, so no additional RBAC overlay is needed. An +install that narrowed `watchTypes` to `selected` would have to name them explicitly; the e2e setup +does not. ### Integration into `prepare-e2e` diff --git a/docs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.md b/docs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.md index 0bd46d55..fa4d425a 100644 --- a/docs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.md +++ b/docs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.md @@ -257,7 +257,7 @@ followed — the same posture as `ScanDir`, over the whole tree. It reuses `coll A product layer consuming this from Go imports [`pkg/manifestanalyzer`](../../../pkg/manifestanalyzer) — `ScanRepo` and `ScanFolder` — rather than exec'ing the binary. That package is the supported, versioned projection of the reports below; the engine above stays internal and -free to move. See [../multi-tenant/README.md](../multi-tenant/README.md) item 4. +free to move. **Report shape as shipped**, refining the sketch above. Per candidate: `path`, `layout`, `acceptedByOperator`, `refusalReasons[]` (`{code, detail}`), `renderRoot`, `readScope[]`, diff --git a/docs/design/manifest/gitpathaccepted-projection-race-and-external-drift.md b/docs/design/manifest/gitpathaccepted-projection-race-and-external-drift.md index 21683672..6b38bc4b 100644 --- a/docs/design/manifest/gitpathaccepted-projection-race-and-external-drift.md +++ b/docs/design/manifest/gitpathaccepted-projection-race-and-external-drift.md @@ -33,7 +33,7 @@ is and is not the cause, then the design direction. > 5-minute `RequeueSteadyInterval`, so the happy-path GitTarget reconcile requeues after > **5 minutes, not 10** (§1.5, §2). The projection-race analysis and its GitPath-channel fix > are otherwise unaffected. See -> [`../../future/secret-value-retention-plan.md`](../../future/secret-value-retention-plan.md). +> [`../../security-model.md`](../../security-model.md). ### 1.1 The failure diff --git a/docs/design/reconcile-triggering.md b/docs/design/reconcile-triggering.md index 1441c39b..b426b5ee 100644 --- a/docs/design/reconcile-triggering.md +++ b/docs/design/reconcile-triggering.md @@ -36,7 +36,7 @@ Facts first, then the fix, then the gaps, then the toolbox. > Secrets, so out-of-band credential/age-key changes are picked up on this unified steady > cadence instead of via a Secret informer. Where the narrative below says "up to 10 > minutes," read 5 minutes. See -> [`future/secret-value-retention-plan.md`](../future/secret-value-retention-plan.md). +> [`security-model.md`](../security-model.md). ### 1.2 Per-controller triggers @@ -162,7 +162,7 @@ until the 5-minute steady requeue, so `Ready=False (ConnectionFailed)` lingers t > reads credential and age-key Secrets directly by name and relies on the 5-minute > `RequeueSteadyInterval` to pick up rotations; the GitTarget's former encryption-Secret watch > was also removed. Adding a Secret watch would reintroduce cluster-wide Secret retention. See -> [`future/secret-value-retention-plan.md`](../future/secret-value-retention-plan.md). The +> [`security-model.md`](../security-model.md). The > §5 recommendation to "watch the auth Secret" below is retained only as historical context. --- diff --git a/docs/future/least-privilege-remaining-work.md b/docs/future/least-privilege-remaining-work.md new file mode 100644 index 00000000..595084b5 --- /dev/null +++ b/docs/future/least-privilege-remaining-work.md @@ -0,0 +1,68 @@ +# Least privilege: what is left + +> Status: three open items. Everything else from the old Secret-handling/RBAC plans has shipped +> and now lives in [`../rbac.md`](../rbac.md) and [`../security-model.md`](../security-model.md). +> Origin: issue [#205](https://github.com/ConfigButler/gitops-reverser/issues/205). + +## Already shipped (do not re-plan) + +The controller holds no Secret values (cache `DisableFor`), runs no control-plane Secret watch, +refreshes on a 5-minute steady reconcile, and passes only public age recipients to SOPS. The +wildcard cluster read moved out of the manager ClusterRole into `rbac.watchTypes`, so an install +can grant read on named types only, and the manager role's Secret verbs are down to +`get,create,update`. + +**The one constraint that shapes everything below:** Kubernetes RBAC is additive. There is no deny +rule and no "everything except Secrets". While `mode: any` is bound, no namespaced Secret `Role` +reduces anything. + +## 1. Never mirror Secrets + +An install should be able to say: *git credentials are controller input; cluster Secrets are not +mirrored output*. Today a `WatchRule` naming `secrets` is honoured. + +Shape: an exclusion policy (`--exclude-resources=secrets`, or a structured counterpart to +`SensitiveResourcePolicy`) that makes the followability funnel refuse the type, explains the refusal +on `WatchRule`/`ClusterWatchRule` status, and never opens the watch. Generated RBAC can then omit +the grant entirely. + +## 2. Permission-aware followability + +The funnel checks the verbs **discovery advertises**, not the verbs this ServiceAccount **holds** +([`internal/typeset/funnel.go`](../../internal/typeset/funnel.go)). Under `mode: selected` a type +can look followable and then `403` when the watch opens. + +Shape: collect effective permissions (`SelfSubjectRulesReview` in bulk, `SelfSubjectAccessReview` +for ambiguous wildcard/aggregated cases), add permitted verbs to the type observation, and fail the +verbs requirement with a `ReasonNotPermitted` surfaced on rule status. + +This is what makes a narrowed install diagnosable instead of mysterious. The API-surface trigger +informers already do the honest thing on `403` — they stop and log once, and re-arm when the +permission is granted — but watched types still churn. + +## 3. Scope the Secret grant, and an RBAC generator + +`secrets: get,create,update` is cluster-scoped because a `GitProvider` may reference a Secret in any +namespace. A `selected` install therefore cannot *enumerate* Secrets but can still read one it can +name. Closing that needs a namespaced `Role` per referenced namespace, which the chart cannot render +without knowing the manifests. + +Which is the same input an **RBAC generator** needs: read `GitProvider`, `GitTarget`, `WatchRule` +and `ClusterWatchRule` manifests and emit the narrow role set — fixed controller grants, Secret +`get` scoped to referenced namespaces, Secret write grants only where `generateWhenMissing` or +signing-key generation applies, and one `get,list,watch` per selected GVR. Manifest input first: it +is reviewable in CI and needs no cluster access. + +Until it exists, the escape hatch is `rbac.create: false` with a hand-written role set. + +## What not to do + +- Do not claim a metadata-only Secret watch lowers RBAC. Kubernetes has no metadata-only Secret + permission; it still needs `get,list,watch`. +- Do not reintroduce a namespace-scoped Secret **value** cache to satisfy a namespace flag. +- Do not add a Secret watch by default. Direct reads plus the 5-minute reconcile are the baseline; + add `--secret-watch-namespaces` only if measured rotation latency demands it, and never let it + become a value cache. +- Do not use `resourceNames` as the model for Secret watches: `list`/`watch` and rotation make it + brittle. +- Do not remove the `any` default. It is the convenient path, and it is honest about its cost. diff --git a/docs/future/publish-age-recipients-as-metadata.md b/docs/future/publish-age-recipients-as-metadata.md new file mode 100644 index 00000000..71d03d16 --- /dev/null +++ b/docs/future/publish-age-recipients-as-metadata.md @@ -0,0 +1,36 @@ +# Publish age recipients as metadata + +> Status: idea, not started. +> Context: the SOPS write path is already public-recipient only (see +> [`../security-model.md`](../security-model.md)). This removes the last reason to read the +> age-key Secret's `data` on a hot path. + +## The idea + +For a **generated** age key the controller already writes `configbutler.ai/age-recipient` on the +Secret. For a **BYO** age-key Secret it re-reads `data` to derive the recipient each time it needs +it, even though only the public half is ever used. + +Derive the recipient once, publish it as a controller-owned annotation +(`configbutler.ai/age-recipients`), and let the write path read recipients from the `GitTarget` +spec plus that annotation. The private key `data` is then read only at bootstrap and rotation. + +## The security rule that makes or breaks it + +**Never trust a user-written recipient annotation.** If an attacker can set it to their own public +key, every Secret mirrored under that `GitTarget` is encrypted to a key they hold — a silent, +complete compromise of the encrypted-at-rest guarantee, with no error anywhere. + +So exactly one of: + +- the controller derives the annotation from the real key material and **overwrites** it on every + reconcile, treating any other value as untrusted; or +- the `GitTarget` spec carries the public recipient directly, and the annotation is a cache keyed + by the Secret's `resourceVersion`. + +Anything that reads a recipient it did not itself derive is wrong. + +## Why it is not urgent + +Reading `data` to take a public half is not a leak — the value never leaves the process and never +reaches `sops`. This is a hot-path and blast-radius improvement, not a fix. diff --git a/docs/future/scoped-rbac-least-privilege-plan.md b/docs/future/scoped-rbac-least-privilege-plan.md deleted file mode 100644 index 8adcfda0..00000000 --- a/docs/future/scoped-rbac-least-privilege-plan.md +++ /dev/null @@ -1,253 +0,0 @@ -# Scoped RBAC and least-privilege install - -> Status: design plan — **not started** (this is Track B). Its runtime prerequisites -> (stop retaining Secret values, drop control-plane Secret watches, 5-minute reconcile -> fallback) shipped in PR -> [#208](https://github.com/ConfigButler/gitops-reverser/pull/208), so the runtime no longer -> *needs* Secret `list/watch` for controller-owned inputs. What remains is the packaging/RBAC -> work below: the default install still *grants* the broad Secret permission and the -> `*/* get,list,watch` wildcard. -> Date: 2026-07-07 (updated 2026-07-08). -> Roadmap: [Secret handling roadmap](secret-handling-roadmap.md). -> Related issue: [#205](https://github.com/ConfigButler/gitops-reverser/issues/205). - -## Summary - -This plan is about what the operator's ServiceAccount is allowed to read. It is separate -from the Secret cache cleanup in -[secret-value-retention-plan.md](secret-value-retention-plan.md), which reduces what the -process keeps in memory. - -The key constraint is Kubernetes RBAC: permissions are additive. There is no deny rule and -no "all resources except Secrets" rule. The default install currently grants: - -- core `secrets get,list,watch,create,update,patch`; -- core `namespaces get,list,watch`; -- `*/* get,list,watch` for dynamic watching. - -While the wildcard exists, a namespaced Secret Role does not reduce the ServiceAccount's -cluster-wide Secret read permission. Real least privilege means running without the -wildcard and granting only the resources the install needs. - -## What the reporter wants - -The reported use case is narrower than the default product model: - -- use one Git credential Secret in one namespace; -- do not mirror Kubernetes Secrets into Git at all; -- avoid cluster-wide Secret reads; -- keep SOPS encryption one-way and avoid private material on the write path. - -The right answer is not only `--secret-namespaces`. That flag name also implies a scoped -value cache, which is not the target after the retention cleanup. The better plan is: - -1. ✅ stop retaining Secret values in the controller process (PR #208); -2. ✅ drop control-plane Secret watches and use a 5-minute reconcile fallback (PR #208); -3. ⬜ add a mode that refuses to mirror Secrets; -4. ⬜ support BYO-RBAC without the wildcard; -5. ⬜ generate the narrow RBAC needed for a specific install. - -## Current RBAC sources - -The generated ClusterRole is in -[config/rbac/role.yaml](../../config/rbac/role.yaml) and mirrored into the chart at -[charts/gitops-reverser/config/role.yaml](../../charts/gitops-reverser/config/role.yaml). - -Important grants: - -```yaml -- apiGroups: [""] - resources: ["namespaces"] - verbs: ["get", "list", "watch"] -- apiGroups: [""] - resources: ["secrets"] - verbs: ["create", "get", "list", "patch", "update", "watch"] -- apiGroups: ["*"] - resources: ["*"] - verbs: ["get", "list", "watch"] -``` - -Why they exist: - -- The wildcard comes from the dynamic watch manager: - [internal/watch/manager.go](../../internal/watch/manager.go#L33). It lets - `WatchRule` and `ClusterWatchRule` select many current and future resource types without - an RBAC redeploy. -- Secret reads support Git credentials, age encryption configuration, and signing keys. - Secret list/watch is only needed for the current Secret reactivity watch and should go - away in the retention plan. -- Secret writes support generated signing and age-key Secrets. -- namespace reads are needed for namespaced rule resolution. - -## Supported install modes - -### Default mode - -Keep the generated ClusterRole and wildcard. - -Good for convenience and broad discovery. Not least privilege. - -### Hardened static mode - -Operators set: - -```yaml -rbac: - create: false -serviceAccount: - create: false - name: existing-service-account -``` - -They bind that ServiceAccount to a hand-written or generated role set. This already works -at the packaging layer. What is missing is documentation and better runtime behavior when -a rule selects something not granted. - -### Future dynamic mode - -The operator reconciles its own Roles and RoleBindings from active rules. This is not a -near-term target. It requires RBAC write permission, privilege-escalation safeguards, and -an explicit opt-in. - -## Plan - -### 1. Document BYO-RBAC - -Write an operator-facing recipe for hardened static installs: - -- disable chart-created RBAC and ServiceAccount; -- create a ServiceAccount owned by the platform team; -- grant fixed controller permissions; -- grant variable watch permissions based on the install's rules; -- document the trade-off: no wildcard means no automatic access to new resource types. - -Fixed permissions include: - -- GitOps Reverser CRDs, `/status`, and `/finalizers`; -- leader-election leases only if leader election is enabled. It is not enabled today, and - the generated role does not currently grant `coordination.k8s.io/leases`; -- events if emitted; -- namespace reads for namespaced rule resolution; -- scoped Secret `get` for referenced Git credentials and age-key inputs; -- scoped Secret writes for generated signing and age-key material; -- Secret `list/watch` only if optional fast-rotation metadata watches are enabled. - -Variable permissions include: - -- one `get,list,watch` rule per selected GVR; -- Role when every selection is namespace-scoped; -- ClusterRole when the selected resource or selection scope is cluster-wide. - -### 2. Add "never mirror Secrets" - -Add an exclusion policy, for example: - -```text ---exclude-resources=secrets -``` - -or a structured counterpart to `SensitiveResourcePolicy`. - -Behavior: - -- Secrets are refused by the followability funnel; -- `WatchRule` or `ClusterWatchRule` status explains the refusal; -- the dynamic target watch path never opens a Secret watch; -- generated RBAC can omit mirrored-Secret read grants. - -This serves the reporter's strongest requirement: Git credentials remain controller input, -but cluster Secrets are not mirrored output. - -### 3. Make followability permission-aware - -The current funnel checks discovery-advertised verbs, not the ServiceAccount's actual -authorization: -[internal/typeset/funnel.go](../../internal/typeset/funnel.go#L16). - -With narrow RBAC, a type can look followable in discovery and then fail with `403 -Forbidden` when the dynamic watch opens. That should become a clean refusal. - -Implementation shape: - -- collect effective permissions for relevant namespaces and cluster scope; -- prefer `SelfSubjectRulesReview` for bulk rules; -- use `SelfSubjectAccessReview` as a fallback for ambiguous wildcard or aggregated cases; -- add permitted verbs to the type observation; -- fail the verbs requirement with a new reason such as `ReasonNotPermitted`; -- surface the reason on `WatchRule` and `ClusterWatchRule` status. - -This is not required to remove Secret values from the cache, but it is required to make -least-privilege installs pleasant and diagnosable. - -### 4. Add an RBAC generator - -Add a command or task that reads manifests or live cluster state and emits the narrow role -set for: - -- `GitProvider`; -- `GitTarget`; -- `WatchRule`; -- `ClusterWatchRule`. - -Output: - -- fixed controller grants; -- Secret input grants for referenced Git credentials and age-key Secrets; -- generated Secret write grants where `generateWhenMissing` or signing-key generation can - create/update Secrets; -- one watch grant per selected GVR; -- no `*/*` wildcard when `--no-wildcard` is selected. - -Start with manifest input. It is reviewable in CI and does not require cluster access. -Live-cluster input can come later. - -### 5. Use a 5-minute control-plane reconcile fallback - -The hardened default should not need Secret watches for controller-owned inputs. Instead, -control-plane controllers should use a common 5-minute periodic reconcile: - -- credential or age-key rotation is picked up within 5 minutes if no related work happens - sooner; -- branch workers still read credentials directly when work is happening; -- generated-key recovery and recipient annotation refresh can wait for the next GitTarget - reconcile; -- RBAC for controller-owned Secret inputs can drop `list/watch` and keep scoped `get`. - -This does not affect data-plane mirroring watches. Those still need `get,list,watch` for -the resources the user chooses to mirror. - -### 6. Optional: namespace-scope Secret metadata watches - -After [secret-value-retention-plan.md](secret-value-retention-plan.md) switches to direct -reads and a 5-minute reconcile fallback, metadata watches can be added back only for -installs that need faster Secret rotation. Avoid adding a public flag first if a generated -static namespace set is enough. - -```text ---secret-watch-namespaces=ns1,ns2 -``` - -If a flag is needed, prefer `--secret-watch-namespaces` over the issue's -`--secret-namespaces`: it says exactly what is scoped. This should not create a full -Secret value cache. It only scopes rotation reactivity. The matching RBAC still needs -normal Secret `get,list,watch` permissions in those namespaces. - -## What not to do - -- Do not claim metadata-only watch lowers RBAC. It does not. -- Do not reintroduce a namespace-scoped full Secret cache as the primary fix. -- Do not require a Secret metadata watch by default when a 5-minute reconcile is enough. -- Do not claim a namespaced Secret Role helps while `*/* get,list,watch` is still bound. -- Do not replace the default wildcard install for everyone. -- Do not use `resourceNames` as the main model for Secret watches; list/watch and rotation - behavior make that brittle. - -## Done - -This plan is complete when: - -- hardened static install docs exist; -- users can opt out of mirroring Secrets; -- missing watch permissions surface as rule status, not unexplained watch churn; -- an RBAC generator can produce a no-wildcard role set for a concrete install; -- public docs clearly distinguish RBAC blast radius from in-process Secret exposure. diff --git a/docs/future/secret-handling-roadmap.md b/docs/future/secret-handling-roadmap.md deleted file mode 100644 index b27c3afb..00000000 --- a/docs/future/secret-handling-roadmap.md +++ /dev/null @@ -1,120 +0,0 @@ -# Secret handling roadmap - -> Status: roadmap. **Track A (process/crypto exposure) is implemented; Track B (RBAC blast -> radius) is not started.** -> Date: 2026-07-07 (updated 2026-07-08). -> Related issue: [#205](https://github.com/ConfigButler/gitops-reverser/issues/205). -> Detailed plans: -> [Secret value retention](secret-value-retention-plan.md) and -> [Scoped RBAC and least-privilege install](scoped-rbac-least-privilege-plan.md). - -> **Progress.** PR [#208](https://github.com/ConfigButler/gitops-reverser/pull/208) -> delivered Track A: the controller no longer retains Secret values in memory, no longer -> watches Secrets on the control plane, reconciles on a unified 5-minute fallback, and keeps -> the SOPS write path public-recipient-only. This closes the **process-exposure** and -> **rotation-reaction** axes of the table below. The **RBAC blast radius** axis — the literal -> "limit secrets permissions" ask in #205 — is Track B and still open: the default install -> still grants `secrets get;list;watch` plus the dynamic-watch wildcard. - -## Why this exists - -The issue asks for `--secret-namespaces=ns1,ns2` so the operator stops watching every -Secret in the cluster. That request is directionally right, but it mixes three different -problems: - -| Axis | Question | Main fix | -|---|---|---| -| Process exposure | What Secret values does the controller hold in memory? | Direct Secret reads only | -| Rotation reaction | How fast do changed credentials or age keys take effect? | 5-minute reconcile fallback | -| RBAC blast radius | What is the ServiceAccount allowed to read? | Drop the wildcard and generate narrow RBAC | - -Those fixes should not be shipped as one large feature. The first two are small and give -the biggest immediate security win. RBAC narrowing is a separate install mode with a real -trade-off: no wildcard means no automatic access to newly selected resource types. - -The important update: control-plane Secret watches are not required. Mirroring still needs -data-plane watches for selected resources, and controllers should still watch our own CRDs. -But Git credentials and age-key inputs can be read directly and refreshed by a 5-minute -periodic reconcile instead of a Secret informer. - -## Current state - -The operator has three Secret paths today: - -- `GitTarget` registers a full-object Secret watch to react to generated age Secret - changes. With the default controller-runtime cache, that means every Secret value in - every namespace can be held in memory. -- `GitProvider`, branch workers, encryption setup, and signing-key helpers read specific - Secrets through the cached client. -- Mirrored Secrets selected by a `WatchRule` or `ClusterWatchRule` use the dynamic watch - path. That path is about user-selected resources, not the controller's own credential - inputs. - -The default chart also grants `*/* get,list,watch` for dynamic watching. Because -Kubernetes RBAC is additive, a namespaced Secret Role does not reduce the ServiceAccount's -Secret read permission while that wildcard remains. - -## Decision - -Treat this as two tracks. - -**Track A: stop retaining Secret values.** This is the first implementation track. It -keeps the default broad install behavior, but changes the process footprint: - -- delete control-plane Secret watches instead of converting them to metadata; -- bypass the cache for typed Secret reads; -- read referenced Secret values directly by name; -- standardize control-plane periodic reconciles at 5 minutes; -- remove private age identities from the SOPS encrypt path. - -This solves the immediate "the process caches every Secret value forever" problem and -removes the need for Secret list/watch on controller-owned inputs in the default mode. - -**Track B: support narrow installs.** This is the install/RBAC track: - -- document bring-your-own RBAC; -- add a "never mirror Secrets" policy; -- make followability permission-aware so missing grants become status refusals rather than - runtime watch churn; -- add a generator for minimal Roles and ClusterRoles. - -This is what reduces what the ServiceAccount is allowed to read. - -## Recommended order - -Steps 1–3 are **done** (PR -[#208](https://github.com/ConfigButler/gitops-reverser/pull/208)); steps 4–6 are Track B and -**not started**; step 7 is optional and not needed while the 5-minute fallback suffices. - -1. ✅ **Stop value caching and drop Secret dependency watches.** - Implemented the small controller/client changes in - [secret-value-retention-plan.md](secret-value-retention-plan.md). This makes Secret - `list/watch` unnecessary for controller-owned inputs; packaging cleanup follows in the - RBAC track. -2. ✅ **Remove private age identity handling from the encrypt path.** - SOPS encryption is now public-recipient only. Small deletion, strong security payoff. -3. ✅ **Set control-plane reconcile fallback to 5 minutes.** - `RequeueSteadyInterval` is now the common periodic reconcile cadence for control-plane - controllers after removing Secret watches. -4. ⬜ **Add `--exclude-resources=secrets` or equivalent.** - This lets an install say: Git credentials are controller input; Kubernetes Secrets are - not mirrored output. -5. ⬜ **Document BYO-RBAC and add permission-aware followability.** - This makes hand-narrowed RBAC predictable. -6. ⬜ **Add an RBAC generator.** - Generate the narrow role set from `GitProvider`, `GitTarget`, `WatchRule`, and - `ClusterWatchRule` manifests. -7. ⬜ **Optional: add Secret metadata watches for faster rotation.** - Do this only if the 5-minute fallback is not responsive enough. Prefer a namespace set - derived from manifests or generated Helm values before adding a runtime flag. - -## Non-goals - -- Do not replace the default wildcard install for everyone. The default remains convenient. -- Do not reintroduce a full Secret value informer just to support a namespace flag. -- Do not add a Secret metadata watch by default; direct reads plus a 5-minute reconcile are - the simpler baseline. -- Do not add a namespace flag before proving generated or inferred configuration is not - enough. -- Do not add a long-lived in-process Secret value TTL cache unless an install measures API - pressure and explicitly opts in. diff --git a/docs/future/secret-value-retention-plan.md b/docs/future/secret-value-retention-plan.md deleted file mode 100644 index f5db818e..00000000 --- a/docs/future/secret-value-retention-plan.md +++ /dev/null @@ -1,265 +0,0 @@ -# Stop retaining Secret values - -> Status: **implemented** (steps 1–6) in PR -> [#208](https://github.com/ConfigButler/gitops-reverser/pull/208). Step 7 (publish -> recipients as metadata) remains future work. -> Date: 2026-07-07 (implemented 2026-07-08). -> Roadmap: [Secret handling roadmap](secret-handling-roadmap.md). -> Scope: remove full Secret values from the controller-runtime cache, keep direct reads -> fresh, drop control-plane Secret watches, and remove private age identities from the SOPS -> encrypt path. - -> **What shipped.** The manager sets `Client.Cache.DisableFor: corev1.Secret`, so typed -> Secret reads bypass the cache and no informer retains Secret values. The GitTarget -> control-plane Secret watch was removed. The short/medium/long requeue intervals collapsed -> into one 5-minute `RequeueSteadyInterval`. The SOPS write path carries public age -> recipients only — no `SOPS_AGE_KEY_FILE`, no identity temp file, no blanket Secret-data -> env. And (step 5) the branch worker resolves Git credentials once per push cycle instead -> of per commit. RBAC narrowing is deliberately **not** part of this — it stays in -> [scoped-rbac-least-privilege-plan.md](scoped-rbac-least-privilege-plan.md). - -## Summary - -This plan is the first implementation slice of the Secret handling work. It does not try -to solve RBAC. It solves the process-level problem: the controller should not keep every -Secret value in the cluster in memory. - -The target state: - -- no controller-runtime informer stores full `corev1.Secret` objects; -- no controller-runtime Secret informer is started for control-plane dependencies; -- typed Secret reads go directly to the API server; -- external Secret changes are noticed by direct reads and a 5-minute reconcile fallback; -- SOPS encryption uses public age recipients only; -- no private age identity is written to disk or passed to the `sops` process. - -TTL is a fallback load optimization, not the design. - -## Current problem - -The manager is created without cache options: -[cmd/main.go](../../cmd/main.go#L841). Any informer started by the shared cache is -cluster-wide unless configured otherwise. - -`GitTarget.SetupWithManager` registers a full Secret watch: -[gittarget_controller.go](../../internal/controller/gittarget_controller.go#L965). -The map function only needs the Secret name and namespace, but the current watch stores -full Secret objects, including `data`. - -Several code paths also read specific Secrets through the cached client: - -- GitProvider credentials: - [gitprovider_controller.go](../../internal/controller/gitprovider_controller.go#L262) -- branch-worker credentials: - [branch_worker.go](../../internal/git/branch_worker.go#L504) -- age encryption Secret: - [encryption.go](../../internal/git/encryption.go#L173) -- signing keys: - [helpers.go](../../internal/git/helpers.go#L37) - -Result: one reactivity watch can make the process retain cleartext Secret values for the -whole cluster, even though the operator only needs a small number of named Secrets. - -## Target model - -Use one default access mode for controller-owned Secret inputs: - -- **Read values directly** only when code actually needs a specific Secret. -- **Reconcile every 5 minutes** so out-of-band Secret changes are picked up without a - Secret watch. - -```mermaid -flowchart TB - Timer["5-minute reconcile fallback"] - Reconcile["GitProvider / GitTarget / branch worker"] - DirectRead["direct Secret GET
specific name + namespace"] - API["kube-apiserver"] - - Timer --> Reconcile - Reconcile --> DirectRead - DirectRead --> API -``` - -This gives up instant Secret-rotation reaction in exchange for a cleaner default: no -Secret list/watch, no metadata informer, no namespace flag, and no Secret values retained -by the controller cache. Mirroring watches are separate and remain required for selected -resources. - -## Implementation steps - -### 1. Delete the GitTarget Secret watch - -Remove the control-plane Secret watch from `GitTarget.SetupWithManager`: - -```go -// internal/controller/gittarget_controller.go -- Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.encryptionSecretToGitTargets)). -``` - -Do not replace it with `WatchesMetadata` by default. Generated age Secret recovery and -out-of-band age-key updates can wait for the 5-minute GitTarget reconcile. That is a -better least-privilege baseline than keeping a cluster-wide Secret metadata watch. - -### 2. Disable cached reads for typed Secrets - -Add `Client.Cache.DisableFor` in `newManager`: - -```go -mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ - Scheme: scheme, - Metrics: metricsOptions, - HealthProbeBindAddress: probeAddr, - WebhookServer: webhookServer, - Client: client.Options{ - Cache: &client.CacheOptions{ - DisableFor: []client.Object{&corev1.Secret{}}, - }, - }, -}) -``` - -This keeps typed Secret reads live and prevents a typed Secret `Get` from starting a -full-object Secret cache. - -### 3. Keep GitProvider credential rotation pull-based - -Do not add a GitProvider Secret watch by default. `GitProvider` should revalidate on its -own spec changes and on the common 5-minute periodic reconcile. - -Branch workers also read credentials directly when they need them, so writes can pick up -new credentials on the next operation. GitProvider status may lag until the next 5-minute -validation pass, which is acceptable for the default least-privilege mode. - -### 4. Standardize control-plane reconcile fallback at 5 minutes - -After removing Secret watches, use 5 minutes as the common control-plane periodic -reconcile cadence. This replaces the old split where fast paths used 2 minutes, some -Secret/config failures used 5 minutes, and steady validation could wait up to 10 minutes. -In code terms, collapse the control-plane uses of the short, medium, and long requeue -intervals to a 5-minute steady fallback for GitProvider, GitTarget, WatchRule, and -ClusterWatchRule reconcilers. - -The intent is simple to explain: - -- no Secret watch by default; -- external Secret changes become visible within 5 minutes through reconcile; -- direct reads still use the freshest Secret value when work is already happening. - -### 5. Remove redundant credential reads where practical - -The branch worker reads the same credentials Secret several times during a push cycle. -Prefer a structural fix: - -- resolve auth once at the start of a flush or push operation; -- pass the resolved auth through clone, fetch, push, force-push, and retry code; -- keep direct reads as the default. - -Only add a short TTL memo if an install measures API pressure. If added, keep it opt-in -and off by default. A long TTL recreates stale-credential behavior. - -Direct reads are acceptable because the operator reads a small set of named Secrets, not -every object during every reconcile. A direct `GET` is real API/etcd work, so avoid tight -loops; the structural read-once change above is the preferred load fix. - -### 6. Stop passing private age identities to SOPS - -The write path only encrypts. It does not decrypt. - -Evidence in the current code: - -- `Encryptor` only exposes `Encrypt`: - [encryption.go](../../internal/git/encryption.go#L35) -- `SOPSEncryptor.Encrypt` runs `sops --encrypt`: - [sops_encryptor.go](../../internal/git/sops_encryptor.go#L43) -- encrypted-content reuse compares plaintext digest plus Kubernetes identity markers and - never decrypts Git content: - [content_writer.go](../../internal/git/content_writer.go#L87) - -age encryption needs public recipients. The private age identity is for decryption, and -this operator does not decrypt. So remove it from the hot path: - -- drop `AgeIdentities` from `ResolvedEncryptionConfig`; -- stop returning `secretIdentities` from `ResolveTargetEncryption`; -- remove `writeAgeIdentityFile`, `SOPS_AGE_KEY_FILE`, and the age identity temp-file path; -- remove blanket `toSOPSEnvironment(secret.Data)` for the age path. - -After this step, the operator may still read a BYO private age key to derive a recipient, -but it does not write the private key to disk or pass it to `sops`. - -### 7. Later: publish recipients as metadata - -This is useful but larger than the first slice. - -For generated age keys, the controller already writes -`configbutler.ai/age-recipient`. For BYO age-key Secrets, the controller can derive the -recipient once and publish a controller-owned annotation such as -`configbutler.ai/age-recipients`. - -Then the write path can read recipients from spec plus metadata and avoid reading private -age-key `data` except during bootstrap or rotation. - -Security rule: never blindly trust a user-written recipient annotation. If an attacker can -set the annotation to their public key, mirrored Secrets can be encrypted to a key they -control. The controller must derive and overwrite the annotation from the actual key -material, or the GitTarget spec must carry the public recipient directly. - -## RBAC impact - -The default plan no longer needs `secrets list/watch` for controller-owned credential and -age-key inputs. It still needs scoped `secrets get` for referenced input Secrets, plus -scoped write verbs where the operator generates signing or age-key Secrets. - -The runtime change can land before the generated default RBAC is narrowed. The important -contract is that `list/watch` becomes unnecessary for controller-owned Secret inputs, so -the least-privilege packaging can remove those verbs. - -If optional Secret metadata watches are added later, they use the normal Secret endpoint -and require `secrets get,list,watch` in their scope. Kubernetes RBAC does not provide a -metadata-only Secret permission. - -RBAC narrowing is covered by -[scoped-rbac-least-privilege-plan.md](scoped-rbac-least-privilege-plan.md). - -## Optional fast-rotation mode - -Add Secret metadata watches only if 5-minute rotation reaction is not good enough: - -- first preference: no Secret watch; -- second preference: have the RBAC generator or Helm values derive the static namespace set - from `GitProvider` and `GitTarget` manifests; -- last resort: add an explicit `--secret-watch-namespaces=ns1,ns2` flag for metadata - reactivity. - -If the flag exists, use the more precise name above rather than `--secret-namespaces`. -The original issue name sounds like it scopes a value cache. This plan scopes metadata -reactivity only and must not reintroduce a full Secret value cache. - -## Tests - -- Unit: Secret-to-GitProvider and Secret-to-GitTarget map functions enqueue only matching - objects only if optional metadata watch mode is implemented. -- Unit: SOPS environment no longer includes private age identity material. -- Envtest: typed Secret reads see fresh data after a direct Secret update, proving they do - not come from a stale cache. -- Unit or controller test: control-plane steady reconciles use a 5-minute interval. -- E2E: rotating a Git credential Secret is picked up by direct reads or by GitProvider - status within 5 minutes. -- E2E: rotating an age key Secret is picked up by direct reads or by GitTarget reconcile - within 5 minutes. - -## Done - -All of the following are met as of PR -[#208](https://github.com/ConfigButler/gitops-reverser/pull/208): - -- [x] no full-object Secret informer is created by the manager; -- [x] no Secret metadata informer is created by default; -- [x] typed Secret reads bypass the cache; -- [x] control-plane controllers use a 5-minute fallback reconcile for external changes; -- [x] SOPS encryption no longer receives private age identities; -- [x] docs clearly state the default mode needs Secret `get` for inputs, not Secret - `list/watch` (see [../security-model.md](../security-model.md)). - -Note this is a **runtime** milestone: the default install still *grants* `secrets -get;list;watch` — narrowing that grant is tracked separately in -[scoped-rbac-least-privilege-plan.md](scoped-rbac-least-privilege-plan.md). diff --git a/docs/rbac.md b/docs/rbac.md new file mode 100644 index 00000000..e3902f3d --- /dev/null +++ b/docs/rbac.md @@ -0,0 +1,124 @@ +# RBAC: running the reverser without trusting it with the cluster + +The reverser ships able to read every object in the cluster, because a `WatchRule` may name +any type — including a type installed after the operator. That default is honest, not +required. This page shows how to run it with the read access it actually uses, which on a +management cluster is the difference between "mirrors two CRDs" and "can read every +credential we own". + +## The two ClusterRoles + +| ClusterRole | Contents | Where it comes from | +|---|---|---| +| `-manager-role` | Exactly what the binary calls: its own CRs, `namespaces`, `customresourcedefinitions`, `apiservices`, and `secrets` with **`get`, `create`, `update`** | Generated from the kubebuilder markers into [`config/rbac/role.yaml`](../config/rbac/role.yaml). Never edit by hand. | +| `-watch-any` **or** `-watch-selected` | The types a `WatchRule` may read, `get`/`list`/`watch` only | The chart's `rbac.watchTypes`; for kustomize, [`config/rbac/watch-any-role.yaml`](../config/rbac/watch-any-role.yaml). | + +They are separate because **RBAC is additive**. A wildcard read folded into the manager role +would grant cluster-wide Secret `list`/`watch` no matter how narrow the Secret rule beside +it is, and no chart value could take it back. Split out, the wildcard can be replaced. + +## Choosing what the reverser may read + +```yaml +rbac: + create: true + watchTypes: + mode: any # any | selected + selected: [] +``` + +**`mode: any`** (default) grants cluster-wide read on every resource. A `WatchRule` can name +any type, including one installed after the operator, and it will just work. The price is +that the reverser **can read every Secret in the cluster** — every git credential, every +token — even though it never asks for one it was not pointed at. + +**`mode: selected`** grants read on exactly the types you list, and nothing else: + +```yaml +rbac: + watchTypes: + mode: selected + selected: + - apiGroups: [""] # core group + resources: ["configmaps"] + - apiGroups: ["apps"] + resources: ["deployments"] +``` + +Verbs are always `get`, `list`, `watch` — the reverser never writes to a watched type, so a +`verbs:` key is rejected rather than letting you grant one by accident. So is `mode: +selected` with an empty list, which would leave the operator able to watch nothing, and an +unknown key under `rbac`, which is how you think you narrowed access when you did not. + +Those rules live in the chart's +[`values.schema.json`](../charts/gitops-reverser/values.schema.json), which Helm enforces on +`template`, `lint`, `install` and `upgrade`, naming the offending path: + +``` +Error: values don't meet the specifications of the schema(s) in the following chart(s): +- at '/rbac/watchTypes/selected/0': additional properties 'verbs' not allowed +``` + +Do **not** restate `namespaces`, `customresourcedefinitions` or `apiservices`: the manager +role already carries them. A `WatchRule` naming a type you did not grant will fail to watch +it, so the list must cover every type your rules name. + +## The operator does not read Secrets wholesale + +It holds **no Secret informer**: Secrets are excluded from the manager's cache +([`cmd/main.go`](../cmd/main.go), `Cache.DisableFor`), so every read is a direct `get` of a +Secret a `GitProvider` or `GitTarget` names. Nothing in the operator calls `list` or `watch` +on Secrets. It creates and updates only the Secrets it owns (the generated signing key). + +That is why the manager role asks for `get`, `create`, `update` and nothing more. + +## Running least-privilege + +Set `mode: selected` and list your types. The chart renders the ClusterRole and its binding +for you — there is no hand-written role to maintain, and no way to drift from the +ServiceAccount the operator actually runs as. + +Result: `GitProvider` Ready, `ClusterWatchRule` Ready with its streams, commits flowing, and +the reverser **cannot enumerate Secrets** — no `list`, no `watch`, on any namespace. + +### What `selected` does not remove + +Be precise about what is left, because RBAC is additive and the manager role is still bound: + +| Verb on `secrets` | Still granted? | Why | +|---|---|---| +| `list`, `watch` | **No** | The operator never enumerates Secrets. Removed from the manager role, and `selected` adds no wildcard to put them back. | +| `get` | Yes, cluster-scoped | It reads the git-creds and age-key Secrets a `GitProvider`/`GitTarget` names — and those may live in any namespace. | +| `create`, `update` | Yes, cluster-scoped | It generates the signing key and, with `generateWhenMissing`, the age key. | + +So a `selected` install can still **read a Secret whose name and namespace it already +knows**. It cannot discover one. That is a real reduction — enumeration is what turns a +mirroring operator into a credential-harvesting one — but it is not zero Secret access, and +this page will not pretend otherwise. + +Scoping `get`/`create`/`update` down to the namespaces a `GitProvider` actually references +needs a namespaced `Role` the chart does not yet render. Until then the escape hatch is +`rbac.create: false` plus your own role set. See +[`future/least-privilege-remaining-work.md`](future/least-privilege-remaining-work.md). + +## What happens if a read is denied + +Discovery answers what the API **server serves**, not what this ServiceAccount may **read**. +An ordinary apiserver serves `apiregistration.k8s.io` whether or not your ClusterRole names +`apiservices`, so a narrowed role sails past the "is it served?" check and then gets a `403` +on the informer's first `LIST`. + +For the two **API-surface trigger** informers (`customresourcedefinitions`, `apiservices`) +the operator treats a `403` as authoritative: it stops that informer, logs once, and falls +back to refreshing its API-resource catalog on the periodic tick. It does **not** retry the +denial forever, which would bury real errors under benign noise. Grant the permission later +and the next catalog refresh re-arms the informer — no restart. + +These two are conveniences: they only make the catalog notice a new CRD or aggregated API +sooner than the periodic refresh would. Everything else keeps working without them, which is +why failing closed and quiet is the right answer to "you may not read this". + +A denied **watched type** is different: that is the job you asked for, not a convenience, so +the operator keeps retrying it rather than quietly dropping it. Its watch stream errors and +the manager retries with backoff. If you narrow the role, make sure every type your +`WatchRule`s name is granted, and check the operator logs after the change. diff --git a/docs/security-model.md b/docs/security-model.md index 8e9c0b4d..29028061 100644 --- a/docs/security-model.md +++ b/docs/security-model.md @@ -2,30 +2,78 @@ What GitOps Reverser can access, why, and which pieces are sensitive. Read this before installing. -## Why the controller has broad access - -GitOps Reverser writes the live state of watched resource types into Git. To do that it must: - -- **Read watched resources cluster-wide.** WatchRule and ClusterWatchRule decide which types are - followed, but the controller needs read (get/list/watch) access to those types to materialize - them. Broad WatchRules imply broad read access. -- **Read referenced Secrets.** It reads the Git credentials Secret (and, when encryption is - configured, the SOPS/age key Secret) named by a GitProvider/GitTarget. At **runtime** the - control-plane code paths read these controller-owned input Secrets **directly by name** (`get`): - they no longer `list` or `watch` Secrets and never cache Secret values in memory. Out-of-band - credential or age-key rotations are picked up on the direct read the next time work happens, and - at the latest by the 5-minute periodic reconcile. **This is a runtime-behavior change, not (yet) - an RBAC change:** the default install still *grants* `secrets get;list;watch` (and the - dynamic-watch wildcard), so the ServiceAccount's effective Secret permission is unchanged until - the separate RBAC-narrowing track lands. Mirrored Secrets selected by a WatchRule are a separate - concern and still use the watched-resource read path above. See - [`future/secret-value-retention-plan.md`](future/secret-value-retention-plan.md) and - [`future/scoped-rbac-least-privilege-plan.md`](future/scoped-rbac-least-privilege-plan.md). -- **Receive audit events.** The kube-apiserver audit webhook posts events to the controller's - audit ingress. Those events carry object metadata and, for some resources, request/response - bodies. - -The controller does not need write access to watched resources. Its only write target is Git. +## What the controller is allowed to read + +The ServiceAccount is bound to **two** ClusterRoles, and keeping them apart is the whole point: +Kubernetes RBAC is additive, so a wildcard read folded in beside a narrow Secret rule silently +widens it. + +```mermaid +flowchart LR + SA["ServiceAccount
gitops-reverser"] + + subgraph M["manager-role (generated from the code)"] + M1["its own CRDs and status"] + M2["namespaces, CRDs, apiservices
get, list, watch"] + M3["secrets
get, create, update
no list, no watch"] + end + + subgraph W["watch-any or watch-selected (you choose)"] + W1["mode any
every resource: get, list, watch"] + W2["mode selected
only the types you name"] + end + + SA --> M + SA --> W +``` + +To mirror live state into Git the controller must: + +- **Read the watched types.** `WatchRule`/`ClusterWatchRule` pick them; the controller needs + `get,list,watch` on each. `rbac.watchTypes.mode: any` grants that for every type at once — + convenient, and it means the reverser can enumerate **every Secret in the cluster**. `mode: + selected` grants only the types you name. See [`rbac.md`](rbac.md). +- **Read referenced Secrets.** The git-creds Secret, and the SOPS/age key Secret when encryption + is configured, both named by a `GitProvider`/`GitTarget`. This is a `get` by name — never a + `list` or a `watch`. +- **Receive audit events.** The kube-apiserver audit webhook posts events to the audit ingress. + Those carry object metadata and, for some resources, request/response bodies. + +The controller never writes to a watched type. Its only write target is Git, plus the Secrets it +generates itself (the signing key, and the age key under `generateWhenMissing`). + +## The controller does not hold Secret values + +There is **no Secret informer**. The manager is built with `Client.Cache.DisableFor: +corev1.Secret` ([`cmd/main.go`](../cmd/main.go)), so a typed Secret read bypasses the cache and +goes straight to the API server. No control-plane Secret watch exists; the `GitTarget` one was +removed. + +```mermaid +flowchart TB + Timer["5-minute steady reconcile
RequeueSteadyInterval"] + Work["work happens:
a push, a flush, a reconcile"] + Read["direct GET
one Secret, by name and namespace"] + API["kube-apiserver"] + Cache["controller cache
never holds Secret values"] + + Timer --> Work + Work --> Read + Read --> API + Read -.->|bypasses| Cache +``` + +The trade is deliberate: no instant reaction to a rotated credential, in exchange for a process +that cannot leak Secret values it was never asked to hold. A rotated git credential or age key is +picked up by the next direct read when work happens, and at the latest by the 5-minute reconcile. + +This is why the manager role asks for `secrets: get, create, update` and nothing more — the +runtime genuinely does not need `list` or `watch`. What it does **not** do is scope those verbs to +a namespace: a `GitProvider` may reference a Secret anywhere, so the grant is cluster-scoped. A +`selected` install therefore cannot *discover* a Secret, but can still read one whose name and +namespace it already knows. Narrowing that further is +[remaining work](future/least-privilege-remaining-work.md). + ## Sensitive trust boundaries @@ -41,11 +89,17 @@ The controller does not need write access to watched resources. Its only write t Without encryption, a watched `Secret` is committed as-is (its data is plain in the repository). With SOPS + age (`GitTarget.spec.encryption`), Secret values are encrypted before commit using the age -recipients, and the private key never leaves the cluster. Because the write path only encrypts, it -uses **public age recipients only**: the private age identity is never written to disk or passed to -the `sops` process, even when the recipient is derived from a cluster age-key Secret. So: only watch -`Secret` types you intend to commit, and prefer encryption. Secret-shaped custom resource types can opt into the same -encryption path at controller startup. See [`sops-age-guide.md`](sops-age-guide.md). +recipients, and the private key never leaves the cluster. + +The write path **only encrypts** — nothing in the operator decrypts Git content — so it needs +**public age recipients only**. Concretely: no private identity is written to disk, `SOPS_AGE_KEY_FILE` +is never set, and Secret `data` is never handed to the `sops` process as environment. That holds even +when the recipient is derived from a cluster age-key Secret, which the controller reads once to take +the public half. + +So: only watch `Secret` types you intend to commit, and prefer encryption. Secret-shaped custom +resource types can opt into the same encryption path at controller startup. See +[`sops-age-guide.md`](sops-age-guide.md). ## Git credentials Secret shape diff --git a/internal/controller/gitprovider_controller.go b/internal/controller/gitprovider_controller.go index 3ac2146f..cbd19464 100644 --- a/internal/controller/gitprovider_controller.go +++ b/internal/controller/gitprovider_controller.go @@ -54,7 +54,7 @@ type gitProviderLogFirsts struct { // +kubebuilder:rbac:groups=configbutler.ai,resources=gitproviders,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=configbutler.ai,resources=gitproviders/status,verbs=get;update;patch // +kubebuilder:rbac:groups=configbutler.ai,resources=gitproviders/finalizers,verbs=update -// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch +// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;create;update // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. diff --git a/internal/controller/gittarget_controller.go b/internal/controller/gittarget_controller.go index 96a757fd..9e68dda5 100644 --- a/internal/controller/gittarget_controller.go +++ b/internal/controller/gittarget_controller.go @@ -107,7 +107,7 @@ type GitTargetReconciler struct { // +kubebuilder:rbac:groups=configbutler.ai,resources=gittargets,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=configbutler.ai,resources=gittargets/status,verbs=get;update;patch // +kubebuilder:rbac:groups=configbutler.ai,resources=gitproviders,verbs=get;list;watch -// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch +// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;create;update // Reconcile validates GitTarget references and drives startup lifecycle gates. // diff --git a/internal/queue/key_prefix_test.go b/internal/queue/key_prefix_test.go index 44d1ae2f..6294ee95 100644 --- a/internal/queue/key_prefix_test.go +++ b/internal/queue/key_prefix_test.go @@ -108,6 +108,20 @@ func TestRedisStore_DefaultPrefixIsUnchanged(t *testing.T) { require.Equal(t, "gitops-reverser:author:v1:command:cr-uid", store.CommandAuthorStore().key("cr-uid")) } +// A store built without NewRedisStore — a zero-value struct in a test, a future constructor +// that forgets to normalize — must still write under the default root rather than into an +// unprefixed keyspace indistinguishable from another tool's keys. Every key family resolves +// the prefix at use, not only at construction; watch cursors were the one that did not. +func TestRedisStore_ZeroValueStoreStillWritesPrefixedKeys(t *testing.T) { + t.Parallel() + + var store RedisStore // keyPrefix == "" + require.Equal(t, "gitops-reverser:watch:v1:target:gtuid-3:configmaps:cluster:last-rv", + store.watchCursorKey("gtuid-3", coreConfigmapsGVR(), "")) + require.Equal(t, "gitops-reverser:author:v1:command:cr-uid", store.CommandAuthorStore().key("cr-uid")) + require.Equal(t, "gitops-reverser:author:v1:audit:", store.AttributionIndex(0).factKeyBase("")) +} + // Two reversers sharing one Redis/Valkey and one logical database must not read each // other's cursors. Sixteen databases is the only separator Redis offers; a prefix is not // bounded that way. diff --git a/internal/queue/redis_store.go b/internal/queue/redis_store.go index cd369ff7..6a7500d8 100644 --- a/internal/queue/redis_store.go +++ b/internal/queue/redis_store.go @@ -150,6 +150,6 @@ func (s *RedisStore) watchCursorKey( if namespace != "" { scope = "namespace:" + escapeKeyField(namespace) } - return s.keyPrefix + watchCursorKeySuffix + "target:" + escapeKeyField(gitTargetUID) + ":" + + return resolveKeyPrefix(s.keyPrefix) + watchCursorKeySuffix + "target:" + escapeKeyField(gitTargetUID) + ":" + groupResourceKey(gvr.Group, gvr.Resource) + ":" + scope + ":last-rv" } diff --git a/internal/watch/api_surface_triggers_forbidden_test.go b/internal/watch/api_surface_triggers_forbidden_test.go new file mode 100644 index 00000000..bc5cc37a --- /dev/null +++ b/internal/watch/api_surface_triggers_forbidden_test.go @@ -0,0 +1,170 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "context" + "errors" + "sync/atomic" + "testing" + "time" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + kwatch "k8s.io/apimachinery/pkg/watch" + dynamicfake "k8s.io/client-go/dynamic/fake" + clienttesting "k8s.io/client-go/testing" +) + +func triggerListKinds() map[schema.GroupVersionResource]string { + return map[schema.GroupVersionResource]string{ + crdTriggerGVR(): "CustomResourceDefinitionList", + apiServiceTriggerGVR(): "APIServiceList", + } +} + +// forbiddenTriggerClient serves the CRD trigger normally and denies the given resource with a +// 403 on both list and watch — an ordinary apiserver seen by a ServiceAccount whose ClusterRole +// does not name that resource. Flip the returned flag to grant the permission. +// +// The denial is gated on an atomic rather than by rewriting the client's reaction chains: a +// reflector on another resource reads those chains concurrently, so mutating them races. +func forbiddenTriggerClient(denied schema.GroupVersionResource) (*dynamicfake.FakeDynamicClient, *atomic.Bool) { + client := dynamicfake.NewSimpleDynamicClientWithCustomListKinds(runtime.NewScheme(), triggerListKinds()) + + forbidden := apierrors.NewForbidden( + schema.GroupResource{Group: denied.Group, Resource: denied.Resource}, "", + errors.New("RBAC: no rule grants list on this resource")) + + var denying atomic.Bool + denying.Store(true) + + client.PrependReactor("list", denied.Resource, + func(clienttesting.Action) (bool, runtime.Object, error) { + if denying.Load() { + return true, nil, forbidden + } + return false, nil, nil // fall through to the object tracker + }) + client.PrependWatchReactor(denied.Resource, + func(clienttesting.Action) (bool, kwatch.Interface, error) { + if denying.Load() { + return true, nil, forbidden + } + return false, nil, nil + }) + return client, &denying +} + +// managerWithTriggerClient wires a manager whose discovery serves BOTH triggers (so the +// ServesWatchable gate cannot be what stops anything) and whose dynamic client is the fake. +func managerWithTriggerClient(t *testing.T, client *dynamicfake.FakeDynamicClient) *Manager { + t.Helper() + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + catalog := NewAPIResourceCatalog() + _, err := catalog.Refresh(aggregatingDiscovery()) + require.NoError(t, err) + + m := &Manager{Log: logr.Discard()} + m.triggerCtx = ctx + m.triggerClient = client + m.resourceCatalog = catalog + return m +} + +func (m *Manager) triggerIsStarted(gvr schema.GroupVersionResource) bool { + m.triggersMu.Lock() + defer m.triggersMu.Unlock() + _, started := m.triggersStarted[gvr] + return started +} + +func (m *Manager) triggerForbiddenLogged(gvr schema.GroupVersionResource) bool { + m.triggersMu.Lock() + defer m.triggersMu.Unlock() + _, logged := m.triggersForbiddenLogged[gvr] + return logged +} + +// Discovery reports what the SERVER serves, not what the CALLER may read. On an ordinary +// apiserver apiregistration.k8s.io is served, so ServesWatchable says yes and the informer +// starts — then the reflector's LIST is denied and client-go retries it forever. That is the +// same endless, benign noise the unserved-resource gate exists to remove. +// +// The informer must stop, and stop counting as started, so a later refresh can re-arm it. +func TestEnsureAPISurfaceTriggerInformers_StopsForbiddenInformer(t *testing.T) { + denied := apiServiceTriggerGVR() + client, _ := forbiddenTriggerClient(denied) + m := managerWithTriggerClient(t, client) + + m.ensureAPISurfaceTriggerInformers(m.Log) + + // Discovery serves both, so both start; only the 403 may take one down. + require.True(t, m.triggerIsStarted(crdTriggerGVR())) + + require.Eventually(t, func() bool { return !m.triggerIsStarted(denied) }, + 5*time.Second, 10*time.Millisecond, + "a forbidden trigger informer must be stopped and un-started, not retried forever") + + // One denial must not disarm the whole API surface. + require.True(t, m.triggerIsStarted(crdTriggerGVR()), + "the permitted trigger must keep running") +} + +// A denial that never clears must be recorded once, not once per reflector retry. +func TestEnsureAPISurfaceTriggerInformers_ForbiddenLogsOncePerDenial(t *testing.T) { + denied := apiServiceTriggerGVR() + client, _ := forbiddenTriggerClient(denied) + m := managerWithTriggerClient(t, client) + + m.ensureAPISurfaceTriggerInformers(m.Log) + + require.Eventually(t, func() bool { return m.triggerForbiddenLogged(denied) }, + 5*time.Second, 10*time.Millisecond, "the denial must be recorded so it logs once") +} + +// Granting the RBAC later must be picked up on the next catalog refresh, with no restart — +// the same promise the unserved path makes for an aggregation layer installed later. +func TestEnsureAPISurfaceTriggerInformers_ReArmsAfterForbiddenIsGranted(t *testing.T) { + denied := apiServiceTriggerGVR() + client, denying := forbiddenTriggerClient(denied) + m := managerWithTriggerClient(t, client) + + m.ensureAPISurfaceTriggerInformers(m.Log) + require.Eventually(t, func() bool { return !m.triggerIsStarted(denied) }, + 5*time.Second, 10*time.Millisecond, "forbidden informer should have been stopped") + + // Grant the permission. The reactors stay in place and start falling through. + denying.Store(false) + + m.ensureAPISurfaceTriggerInformers(m.Log) + + require.Eventually(t, func() bool { return m.triggerIsStarted(denied) }, + 5*time.Second, 10*time.Millisecond, + "a trigger denied earlier must re-arm once RBAC allows it, without a restart") +} + +// A non-RBAC error is transient — a connection reset, a 500 — and the reflector's own +// backoff owns it. Tearing the informer down there would turn a blip into a lost trigger. +func TestEnsureAPISurfaceTriggerInformers_KeepsInformerOnTransientError(t *testing.T) { + gvr := apiServiceTriggerGVR() + client := dynamicfake.NewSimpleDynamicClientWithCustomListKinds(runtime.NewScheme(), triggerListKinds()) + client.PrependReactor("list", gvr.Resource, + func(clienttesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewInternalError(errors.New("etcd leader changed")) + }) + + m := managerWithTriggerClient(t, client) + m.ensureAPISurfaceTriggerInformers(m.Log) + + require.Never(t, func() bool { return !m.triggerIsStarted(gvr) }, + 500*time.Millisecond, 25*time.Millisecond, + "a 500 is not a permission problem; the reflector must keep retrying it") + require.False(t, m.triggerForbiddenLogged(gvr)) +} diff --git a/internal/watch/api_surface_triggers_test.go b/internal/watch/api_surface_triggers_test.go index 319806bc..f7e8d736 100644 --- a/internal/watch/api_surface_triggers_test.go +++ b/internal/watch/api_surface_triggers_test.go @@ -156,6 +156,6 @@ func TestEnsureAPISurfaceTriggerInformers_NoOpBeforeStart(t *testing.T) { m := &Manager{Log: logr.Discard()} m.ensureAPISurfaceTriggerInformers(m.Log) - require.Nil(t, m.triggerFactory) + require.Nil(t, m.triggerClient) require.Empty(t, m.triggersStarted) } diff --git a/internal/watch/manager.go b/internal/watch/manager.go index 101d9bad..09834f6e 100644 --- a/internal/watch/manager.go +++ b/internal/watch/manager.go @@ -19,7 +19,6 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/dynamic" - "k8s.io/client-go/dynamic/dynamicinformer" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" @@ -30,8 +29,20 @@ import ( "github.com/ConfigButler/gitops-reverser/internal/typeset" ) -// RBAC permissions for dynamic watch manager - read-only access to watch all (also future ones!) resource types -// +kubebuilder:rbac:groups=*,resources=*,verbs=get;list;watch +// The API-resource catalog reads the two resources that describe the API surface itself, so +// it can tell a type that vanished from one the operator was never allowed to see. These are +// the trigger informers' resources too; without them a least-privilege install gets a 403 on +// every reflector retry, which is why the trigger error handler stops on Forbidden. +// +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list;watch +// +kubebuilder:rbac:groups=apiregistration.k8s.io,resources=apiservices,verbs=get;list;watch + +// A WatchRule may name ANY type, so the watch manager's read access cannot be derived from +// code. It is deliberately NOT a marker: controller-gen would fold `groups=*,resources=*` +// into this ClusterRole, and RBAC is additive — a wildcard read grants cluster-wide Secret +// list/watch no matter how narrow the Secret rule beside it is. The wildcard therefore lives +// in a ClusterRole of its own (config/rbac/watch-any-role.yaml, `rbac.watchTypes.mode: any` in +// the chart), so an operator that mirrors two CRDs can be told the exact types it may read and +// be denied every other object in the cluster. See docs/rbac.md. // Manager is a controller-runtime Runnable that keeps the followability registry and the // demand-driven materialization axis fresh and drives the per-type splice reconcile. It @@ -101,16 +112,23 @@ type Manager struct { // (re-)evaluated after every catalog refresh — which controllers drive, not just the // manager's own loop — so this is not a startup-only structure. triggersMu sync.Mutex - // triggerCtx is the manager's lifetime context, the stop channel every trigger informer - // is started with. Set once by Start; nil before then, which defers informer creation. + // triggerCtx is the manager's lifetime context, the parent of every trigger informer's + // own context. Set once by Start; nil before then, which defers informer creation. triggerCtx context.Context - // triggerFactory is the shared dynamic informer factory backing the trigger informers. - triggerFactory dynamicinformer.DynamicSharedInformerFactory + // triggerClient is the dynamic client the trigger informers list and watch through. + // Built once from the REST config, or injected by tests. + triggerClient dynamic.Interface // triggersStarted is the set of trigger resources whose informer is already running. triggersStarted map[schema.GroupVersionResource]struct{} + // triggerStops cancels one trigger informer without touching the others. Each informer + // gets its own context so a single forbidden resource can be stopped and later re-armed. + triggerStops map[schema.GroupVersionResource]context.CancelFunc // triggersSkipLogged records which unserved trigger resources have already been logged, // so a permanently absent aggregation layer produces one line, not one per refresh. triggersSkipLogged map[schema.GroupVersionResource]struct{} + // triggersForbiddenLogged records which trigger resources RBAC has already denied, so a + // permanently unauthorized resource produces one line per denial, not one per retry. + triggersForbiddenLogged map[schema.GroupVersionResource]struct{} // catalogReadyOnce guards the one-time "catalog ready" log line, matching the // firstMessage/firstGroupReady sync.Once pattern used by the audit consumer. catalogReadyOnce sync.Once diff --git a/internal/watch/manager_catalog.go b/internal/watch/manager_catalog.go index 8b5cc2f7..26ee0123 100644 --- a/internal/watch/manager_catalog.go +++ b/internal/watch/manager_catalog.go @@ -13,6 +13,8 @@ import ( "github.com/go-logr/logr" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/discovery" "k8s.io/client-go/dynamic" @@ -388,7 +390,7 @@ func (m *Manager) ensureAPISurfaceTriggerInformers(log logr.Logger) { log.V(1).Info("deferring API surface trigger informers - catalog not ready") return } - if m.triggerFactory == nil { + if m.triggerClient == nil { cfg := m.restConfig() if cfg == nil { log.V(1).Info("skipping API surface trigger informers - no REST config available") @@ -399,9 +401,13 @@ func (m *Manager) ensureAPISurfaceTriggerInformers(log logr.Logger) { log.Error(err, "failed to create API surface trigger client") return } - m.triggerFactory = dynamicinformer.NewDynamicSharedInformerFactory(dynamicClient, 0) + m.triggerClient = dynamicClient + } + if m.triggersStarted == nil { m.triggersStarted = map[schema.GroupVersionResource]struct{}{} m.triggersSkipLogged = map[schema.GroupVersionResource]struct{}{} + m.triggersForbiddenLogged = map[schema.GroupVersionResource]struct{}{} + m.triggerStops = map[schema.GroupVersionResource]context.CancelFunc{} } handler := cache.ResourceEventHandlerFuncs{ @@ -420,25 +426,100 @@ func (m *Manager) ensureAPISurfaceTriggerInformers(log logr.Logger) { "the catalog refreshes on its periodic tick instead", "gvr", gvr.String()) } - var fresh []cache.SharedIndexInformer for _, gvr := range start { - informer := m.triggerFactory.ForResource(gvr).Informer() - if _, addErr := informer.AddEventHandler(handler); addErr != nil { - log.Error(addErr, "failed to add API surface trigger handler", "gvr", gvr.String()) - continue - } - m.triggersStarted[gvr] = struct{}{} - delete(m.triggersSkipLogged, gvr) - fresh = append(fresh, informer) - log.Info("API surface trigger informer started", "gvr", gvr.String()) + m.startAPISurfaceTriggerInformer(ctx, log, gvr, handler) + } +} + +// startAPISurfaceTriggerInformer runs one trigger informer under its own context. Each gets +// a private informer rather than a share of a dynamicSharedInformerFactory: the factory +// records an informer as started forever, so a resource stopped for one reason could never +// be re-armed through it. Own informer, own context, own stop. +func (m *Manager) startAPISurfaceTriggerInformer( + ctx context.Context, + log logr.Logger, + gvr schema.GroupVersionResource, + handler cache.ResourceEventHandlerFuncs, +) { + informer := dynamicinformer.NewFilteredDynamicInformer( + m.triggerClient, gvr, metav1.NamespaceAll, 0, cache.Indexers{}, nil, + ).Informer() + + if _, addErr := informer.AddEventHandler(handler); addErr != nil { + log.Error(addErr, "failed to add API surface trigger handler", "gvr", gvr.String()) + return } - if len(fresh) == 0 { + // Must precede Run. A forbidden LIST reaches this handler too: the reflector routes every + // ListAndWatch error through it, not only watch errors. + if setErr := informer.SetWatchErrorHandlerWithContext(m.triggerWatchErrorHandler(gvr, log)); setErr != nil { + log.Error(setErr, "failed to install API surface trigger error handler", "gvr", gvr.String()) return } - // Start is idempotent per informer: it launches only the ones not yet running. - m.triggerFactory.Start(ctx.Done()) - go waitForAPISurfaceTriggerSync(ctx, log, fresh) + gvrCtx, cancel := context.WithCancel(ctx) + m.triggersStarted[gvr] = struct{}{} + m.triggerStops[gvr] = cancel + delete(m.triggersSkipLogged, gvr) + // A denial that clears and returns must be logged again; the once-only guard covers a + // single denial, not the resource's whole lifetime. + delete(m.triggersForbiddenLogged, gvr) + log.Info("API surface trigger informer started", "gvr", gvr.String()) + + // cancel is also held in triggerStops so a forbidden reflector can stop just this one; + // releasing it here as well keeps the context from outliving the informer it belongs to. + go func() { + defer cancel() + informer.RunWithContext(gvrCtx) + }() + go waitForAPISurfaceTriggerSync(gvrCtx, log, gvr, informer) +} + +// triggerWatchErrorHandler tears down a trigger informer the operator is not authorized to +// read, and leaves every other error to the reflector's own backoff. +// +// Discovery answers what the API server SERVES, which is not what this ServiceAccount may +// READ: a cluster can serve apiregistration.k8s.io while a least-privilege ClusterRole omits +// apiservices. The informer then starts and its LIST is denied on every retry, forever — +// the same benign, endlessly repeated noise the unserved-resource gate exists to remove, +// and exactly how a real error gets missed. These resources are conveniences (they only +// make the catalog refresh sooner than its periodic tick), so failing closed and quiet is +// the honest response to "you may not read this". +// +// A 403 is authoritative and will not resolve by retrying, but it CAN be granted later, so +// the resource is un-started rather than blacklisted: the next catalog refresh re-arms it. +func (m *Manager) triggerWatchErrorHandler( + gvr schema.GroupVersionResource, + log logr.Logger, +) cache.WatchErrorHandlerWithContext { + return func(ctx context.Context, r *cache.Reflector, err error) { + if !apierrors.IsForbidden(err) { + cache.DefaultWatchErrorHandler(ctx, r, err) + return + } + m.stopForbiddenTrigger(gvr, log, err) + } +} + +// stopForbiddenTrigger cancels the informer's context and forgets it was started, so the +// next successful catalog refresh can try again. Called from the reflector's goroutine. +func (m *Manager) stopForbiddenTrigger(gvr schema.GroupVersionResource, log logr.Logger, err error) { + m.triggersMu.Lock() + defer m.triggersMu.Unlock() + + if cancel, ok := m.triggerStops[gvr]; ok { + cancel() + delete(m.triggerStops, gvr) + } + delete(m.triggersStarted, gvr) + + if _, logged := m.triggersForbiddenLogged[gvr]; logged { + // The reflector can report the denial more than once before its context unwinds. + return + } + m.triggersForbiddenLogged[gvr] = struct{}{} + log.Info("not authorized to watch an API surface trigger; informer stopped and the catalog "+ + "refreshes on its periodic tick instead. Grant get/list/watch to re-arm it without a restart", + "gvr", gvr.String(), "reason", err.Error()) } // apiSurfaceTriggerGVRs are the resources whose changes mean the API surface moved: a CRD @@ -479,14 +560,18 @@ func (m *Manager) setTriggerContext(ctx context.Context) { m.triggerCtx = ctx } -func waitForAPISurfaceTriggerSync(ctx context.Context, log logr.Logger, informers []cache.SharedIndexInformer) { - syncFns := make([]cache.InformerSynced, 0, len(informers)) - for _, informer := range informers { - syncFns = append(syncFns, informer.HasSynced) - } - if !cache.WaitForCacheSync(ctx.Done(), syncFns...) { - log.Info("API surface trigger informer sync stopped before completion") +// waitForAPISurfaceTriggerSync watches one informer's initial sync. It takes that informer's +// own context, so a trigger stopped for being forbidden ends this wait instead of leaving a +// goroutine blocked on a cache that will never sync. +func waitForAPISurfaceTriggerSync( + ctx context.Context, + log logr.Logger, + gvr schema.GroupVersionResource, + informer cache.SharedIndexInformer, +) { + if !cache.WaitForCacheSync(ctx.Done(), informer.HasSynced) { + log.V(1).Info("API surface trigger informer sync stopped before completion", "gvr", gvr.String()) return } - log.V(1).Info("API surface trigger informers synced") + log.V(1).Info("API surface trigger informer synced", "gvr", gvr.String()) } diff --git a/internal/watch/target_watch.go b/internal/watch/target_watch.go index 919c5ead..4421c6da 100644 --- a/internal/watch/target_watch.go +++ b/internal/watch/target_watch.go @@ -38,6 +38,19 @@ var ( errTargetWatchExpired = errors.New("target watch resourceVersion expired") ) +// targetWatchClosedErr distinguishes a watch that died under us from one that closed because +// we are shutting down. Teardown closes the result channel and cancels the context, leaving +// BOTH select cases ready — and select picks among ready cases at random, so the shutdown +// path returned a spurious error roughly half the time. The context is the authority. +func targetWatchClosedErr(ctx context.Context) error { + select { + case <-ctx.Done(): + return nil + default: + return errTargetWatchClosed + } +} + type targetWatchSet struct { cancel context.CancelFunc specs map[targetWatchKey]string @@ -346,7 +359,7 @@ func (m *Manager) targetWatchReplayAndStream( return nil case ev, ok := <-w.ResultChan(): if !ok { - return errTargetWatchClosed + return targetWatchClosedErr(ctx) } nextReplaying, err := m.handleTargetWatchSessionEvent( ctx, log, gitDest, key, ops, ev, replaying, &replay, @@ -605,7 +618,7 @@ func (m *Manager) streamLiveTargetWatchEvents( return nil case ev, ok := <-events: if !ok { - return errTargetWatchClosed + return targetWatchClosedErr(ctx) } if targetWatchEventAtOrBeforeFloor(ev, floor) { continue diff --git a/internal/watch/target_watch_shutdown_test.go b/internal/watch/target_watch_shutdown_test.go new file mode 100644 index 00000000..d6c24df3 --- /dev/null +++ b/internal/watch/target_watch_shutdown_test.go @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "context" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/watch" + + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// A watch whose result channel closes because the operator is shutting down has not failed. +// Both the ctx.Done() case and the closed-channel case are ready at once, and Go's select +// picks between ready cases at random — so a clean shutdown reported a spurious +// "target watch result channel closed" about half the time. Nothing retried on it, but it +// surfaced as an error on every teardown, and it is the kind of noise that trains an +// operator to ignore the log. +// +// Repeated because the bug is a coin flip per iteration: one run proves nothing. +func TestStreamLiveTargetWatchEvents_CanceledContextIsNotAnError(t *testing.T) { + t.Parallel() + + m := &Manager{Log: logr.Discard()} + gitDest := types.NewResourceReference("target", "default") + key := targetWatchKey{GVR: configmapsGVR, Namespace: "apps"} + + for i := range 200 { + ctx, cancel := context.WithCancel(context.Background()) + cancel() // shutting down + + events := make(chan watch.Event) + close(events) // ...which is why the watch closed + + err := m.streamLiveTargetWatchEvents(ctx, logr.Discard(), gitDest, key, OperationSet{}, events) + require.NoErrorf(t, err, "iteration %d: a closed watch during shutdown is not an error", i) + } +} + +// The same channel closing while the operator is still running IS an error: the watch died +// under it, and the caller must reopen rather than silently stop mirroring the type. +func TestStreamLiveTargetWatchEvents_ClosedChannelWhileRunningIsAnError(t *testing.T) { + t.Parallel() + + m := &Manager{Log: logr.Discard()} + gitDest := types.NewResourceReference("target", "default") + key := targetWatchKey{GVR: configmapsGVR, Namespace: "apps"} + + events := make(chan watch.Event) + close(events) + + err := m.streamLiveTargetWatchEvents( + context.Background(), logr.Discard(), gitDest, key, OperationSet{}, events) + require.ErrorIs(t, err, errTargetWatchClosed) +}