From 661689da7a5f13d5eb87498714b346e92e661569 Mon Sep 17 00:00:00 2001 From: Vitalii Parfonov Date: Fri, 14 Aug 2026 18:39:51 +0300 Subject: [PATCH 1/5] feat(security): protect collector ServiceAccounts via ValidatingAdmissionPolicy Restrict protected collector ServiceAccounts so only CLO-managed workloads (the operator and the built-in controllers that deploy the collector) may run a Pod under them. This closes the path where a user who can create Pods reuses a collector SA to inherit its logging-scc privileges (e.g. hostPath node access), even if they reproduce the collector's visible Pod metadata. (CVE-2026-10609, LOG-9714/LOG-9441) Two ValidatingAdmissionPolicies (Pods, workloads) key on the non-forgeable request.userInfo.username rather than Pod metadata. Protected SAs and allowed creator identities are fed to CEL via the clo-protected-serviceaccounts param ConfigMap, which the operator rebuilds from the current ClusterLogForwarder list on every CLF event. Bindings use parameterNotFoundAction: Allow to avoid operator self-lockout. Enforced as hard Deny with zero upgrade breakage: the only legitimate creators are stable identities (operator SA + kube controllers) that are allow-listed, so existing CLF users and running collectors are unaffected. CLF-layer controls (forward logs you cannot read; exfiltrate the SA token) are scoped out and documented as follow-ups in docs/design. Coverage: unit (fake client) + envtest (real kube-apiserver, CEL compiled) + e2e. Adds a ValidatingAdmissionPolicy how-to guide for newcomers. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Vitalii Parfonov --- Makefile | 11 + cmd/main.go | 16 + config/rbac/role.yaml | 13 + docs/administration/troubleshooting.md | 38 +- .../protect-collector-serviceaccounts.md | 526 ++++++++++++++++++ .../design/validatingadmissionpolicy-guide.md | 462 +++++++++++++++ docs/features/collection.adoc | 1 + hack/test-protected-sa.sh | 220 ++++++++ internal/admission/policy.go | 85 +++ .../admission/protected-sa-pods-binding.yaml | 13 + internal/admission/protected-sa-pods.yaml | 31 ++ .../protected-sa-workloads-binding.yaml | 13 + .../admission/protected-sa-workloads.yaml | 41 ++ .../admission/protected_sa_envtest_test.go | 197 +++++++ internal/admission/protected_sa_policy.go | 176 ++++++ .../admission/protected_sa_policy_test.go | 99 ++++ internal/admission/suite_test.go | 36 ++ .../admission/protected_sa_controller.go | 35 ++ .../admission/protected_sa_runnable.go | 54 ++ internal/controller/kubebuilder_rbac.go | 1 + .../collection/admission/protected_sa_test.go | 206 +++++++ test/e2e/collection/admission/suite_test.go | 13 + 22 files changed, 2286 insertions(+), 1 deletion(-) create mode 100644 docs/design/protect-collector-serviceaccounts.md create mode 100644 docs/design/validatingadmissionpolicy-guide.md create mode 100755 hack/test-protected-sa.sh create mode 100644 internal/admission/policy.go create mode 100644 internal/admission/protected-sa-pods-binding.yaml create mode 100644 internal/admission/protected-sa-pods.yaml create mode 100644 internal/admission/protected-sa-workloads-binding.yaml create mode 100644 internal/admission/protected-sa-workloads.yaml create mode 100644 internal/admission/protected_sa_envtest_test.go create mode 100644 internal/admission/protected_sa_policy.go create mode 100644 internal/admission/protected_sa_policy_test.go create mode 100644 internal/admission/suite_test.go create mode 100644 internal/controller/admission/protected_sa_controller.go create mode 100644 internal/controller/admission/protected_sa_runnable.go create mode 100644 test/e2e/collection/admission/protected_sa_test.go create mode 100644 test/e2e/collection/admission/suite_test.go diff --git a/Makefile b/Makefile index dd99a0d57c..00617a2cdc 100644 --- a/Makefile +++ b/Makefile @@ -275,6 +275,17 @@ test-unit: test-forwarder-generator test-unit-api test-unit-api: @cd ./api/observability && go test -coverprofile=test.cov ./... +# Validates the protected-SA ValidatingAdmissionPolicies' CEL against a real +# kube-apiserver via envtest (no cluster needed). setup-envtest downloads the +# apiserver/etcd binaries; its version tracks controller-runtime (release-0.23). +# The admission suite skips these specs when KUBEBUILDER_ASSETS is unset, so +# test-unit is unaffected. +ENVTEST_K8S_VERSION ?= 1.31.0 +.PHONY: test-admission-envtest +test-admission-envtest: + KUBEBUILDER_ASSETS="$$(go run sigs.k8s.io/controller-runtime/tools/setup-envtest@release-0.23 use $(ENVTEST_K8S_VERSION) -p path)" \ + go test -count=1 -run TestAdmission ./internal/admission/... + .PHONY: coverage coverage: test-unit go tool cover -html=test.cov -o $${ARTIFACTS_DIR:-.}/coverage.html diff --git a/cmd/main.go b/cmd/main.go index 6f9a3f1f55..9b8c7112eb 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -10,8 +10,10 @@ import ( "strings" "time" + internaladmission "github.com/openshift/cluster-logging-operator/internal/admission" internalcontext "github.com/openshift/cluster-logging-operator/internal/api/context" "github.com/openshift/cluster-logging-operator/internal/collector" + admissioncontroller "github.com/openshift/cluster-logging-operator/internal/controller/admission" internaltls "github.com/openshift/cluster-logging-operator/internal/tls" "sigs.k8s.io/controller-runtime/pkg/metrics/filters" @@ -258,8 +260,22 @@ func main() { os.Exit(1) } + operatorNS := internaladmission.OperatorNamespace() + if err = (&admissioncontroller.ProtectedSAReconciler{ + Client: mgr.GetClient(), + OperatorNS: operatorNS, + }).SetupWithManager(mgr); err != nil { + log.Error(err, "unable to create controller", "controller", "ProtectedServiceAccounts") + os.Exit(1) + } + //+kubebuilder:scaffold:builder + if err := mgr.Add(admissioncontroller.NewProtectedSAAdmissionRunnable(k8sClient, operatorNS)); err != nil { + log.Error(err, "unable to register protected SA admission runnable") + os.Exit(1) + } + if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { log.Error(err, "unable to set up health check") os.Exit(1) diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 0a8f905027..7bd7e10ddd 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -5,6 +5,19 @@ metadata: creationTimestamp: null name: cluster-logging-operator rules: +- apiGroups: + - admissionregistration.k8s.io + resources: + - validatingadmissionpolicies + - validatingadmissionpolicybindings + verbs: + - create + - delete + - get + - list + - patch + - update + - watch - apiGroups: - apps resources: diff --git a/docs/administration/troubleshooting.md b/docs/administration/troubleshooting.md index 3ab2932a34..38277ead2c 100644 --- a/docs/administration/troubleshooting.md +++ b/docs/administration/troubleshooting.md @@ -43,4 +43,40 @@ This could suggest that the application generating the log is not correctly term Does it have a newline character at the end? If not, the application writing the log is not properly terminating its lines. **Check the source application**: Ensure that the application generating the logs is configured to append a newline character -(\n) to every log entry. This is standard practice for most logging libraries and systems. \ No newline at end of file +(\n) to every log entry. This is standard practice for most logging libraries and systems. + +### 3. A Pod or workload is denied: `uses protected ServiceAccount ... which is only allowed for use by authorized ClusterLogForwarders` + +The cluster logging operator installs ValidatingAdmissionPolicies that refuse +a Pod or workload when it references a collector ServiceAccount (one referenced by +a `ClusterLogForwarder`) and the creator is not the operator or its built-in +controllers. This stops a user who can create Pods from inheriting the +collector SA's privileges (e.g. `logging-scc` host mounts). + +**Common causes** + +- A user tried to create a standalone Pod/Deployment/DaemonSet that sets + `serviceAccountName` to a collector ServiceAccount. +- A workload copies the collector's labels/annotations/name — this does not help; + the policy keys on the authenticated creator identity, not on Pod metadata. + +**What to do** + +1. Confirm the policies are present (operator must be running): + + ```sh + oc get validatingadmissionpolicy clo-protected-sa-pods clo-protected-sa-workloads + oc get validatingadmissionpolicybinding clo-protected-sa-pods-binding clo-protected-sa-workloads-binding + ``` + +2. Do not reuse a collector ServiceAccount for non-collector workloads. Use a + ServiceAccount that is not referenced by any `ClusterLogForwarder`, and grant + it only the permissions your workload actually needs. + +3. The set of protected ServiceAccounts is maintained by the operator in the + `clo-protected-serviceaccounts` ConfigMap in the operator namespace; it is + rebuilt from the current `ClusterLogForwarder` list on every change. + +See [`docs/design/validatingadmissionpolicy-guide.md`](../design/validatingadmissionpolicy-guide.md) +for how the policies work and `./hack/test-protected-sa.sh` for a lightweight +verification script. \ No newline at end of file diff --git a/docs/design/protect-collector-serviceaccounts.md b/docs/design/protect-collector-serviceaccounts.md new file mode 100644 index 0000000000..b8f88056c4 --- /dev/null +++ b/docs/design/protect-collector-serviceaccounts.md @@ -0,0 +1,526 @@ +# Protecting collector ServiceAccounts from reuse by arbitrary workloads + +Status: design note / investigation (prototype implemented) +Scope: Cluster Logging Operator (observability.openshift.io/v1 `ClusterLogForwarder`) + +> New to ValidatingAdmissionPolicy? Read `docs/design/validatingadmissionpolicy-guide.md` +> first — it explains VAP mechanics (Policy / Binding / param) and reads our +> manifests line by line. This note covers the *why* (threat model); the guide +> covers the *how*. + +Key files: +`internal/admission/protected_sa_policy.go`, `internal/admission/protected-sa-{pods,workloads}{,-binding}.yaml`, +`internal/controller/admission/protected_sa_controller.go` / `_runnable.go`; +manual test `hack/test-protected-sa.sh`. Validated end-to-end against a live +OpenShift API server via `hack/test-protected-sa.sh`: a restricted user is +denied creating a Pod and a Deployment as a protected collector SA even when +copying the collector's labels/annotations/name, while an unprotected SA passes +through. ConfigMap param keys use the form `sa__` ('/' is not a valid +ConfigMap data key). + +## TL;DR + +A user who can write a `ClusterLogForwarder` (CLF) can name **any** existing +ServiceAccount (SA) in the CLF namespace as the collector SA. That SA is often +bound to `logging-scc` (hostPath mounts, RunAsAny UID, SELinux `spc_t`) and to +cluster-wide read of pods/namespaces/nodes. Nothing today stops the same user +from running an **arbitrary** Pod/Deployment as that SA and inheriting those +privileges. + +**A ValidatingAdmissionPolicy (VAP) can enforce the boundary, but only if it +keys on `request.userInfo` (the authenticated creating identity), which the API +server sets and a requester cannot forge. A VAP keyed on Pod labels / +annotations / names / ownerReferences would be security theater — all of that +metadata is fully reproducible by any user who can create Pods in the +namespace.** + +An alternative was considered and rejected (CVE-2026-10609, LOG-9714/LOG-9441): a +VAP on the **CLF resource** requiring the CLF author to hold the `use` verb on the +referenced SA. It fails the upgrade constraint because `use` on `serviceaccounts` +is a non-standard verb **nobody holds by default**, so every existing CLF author +would be denied on upgrade until an admin provisions new `use` Roles — i.e. it +"requires changes in SA" RBAC, the exact upgrade breakage we must avoid. The +approach below instead moves enforcement to Pod admission and keys on the +operator's identity, so **no user or SA RBAC changes are required**. See §6 for +the full comparison. + +The only pre-existing control is a reconcile-time `SubjectAccessReview` that +checks whether the *collector SA* has the `collect` verb — it never checks whether +the *CLF author* is allowed to use the SA. + +--- + +## 0. Scope of this change (decision) + +The original threat has **one root cause** (a CLF author may name any existing +SA) and **three exploit paths**: + +1. **Reuse the SA in another pod → mount host FS → node access** (via the SCC the + SA is bound to). +2. **Forward logs the author cannot access** — reference the SA in a CLF; the + operator deploys a collector *as* that SA and forwards to an output the author + controls. +3. **Exfiltrate a token** — a token-forwarding output (`BearerToken.From: + serviceAccount`) ships the SA's bearer token to an attacker-controlled URL. + +**What this change ships: Path 1 only** — the protected-SA Pod/workload VAP +(§3). It is enforced (`Deny`) with **zero upgrade breakage**, because the only +legitimate creator of a protected-SA pod is a stable, known identity (the +operator SA / the built-in controllers), which we allow-list. Existing +collectors keep running; only *new, non-operator* workloads that reuse a +protected SA are denied. + +**What is deferred: paths 2 and 3 (the CLF-layer control).** They are real, but +they differ structurally from path 1: the legitimate actor is an **arbitrary +human CLF author** whose permissions cannot be predicted, so there is **no +stable identity to allow-list**. Any admission `Deny` that adds an author→SA +requirement can therefore deny *some* existing author on upgrade. Since +**no-existing-author-breakage is a hard requirement**, a hard `Deny` for +paths 2/3 cannot ship today. + +> Note: the CLF `use`-verb VAP (§6) is the canonical example of the breakage we +> must avoid — `use` on `serviceaccounts` is non-default, so *every* existing +> author would be denied on upgrade until an admin grants a new Role. It is +> **not** being pursued as-is. + +### Recommended future work for paths 2/3 (not implemented here) + +A CLF-admission VAP keyed on `request.userInfo` can close paths 2/3 **without** +breaking existing authors if it is **surgically scoped** so it never touches an +existing CLF: + +- Gate **CREATE** of a CLF, and **UPDATE only when `spec.serviceAccount.name` + changes** (`object.spec.serviceAccount.name != oldObject.spec.serviceAccount.name`). + Existing CLFs and same-SA edits are never re-admitted, so no existing author is + denied on upgrade. +- Test an author→SA relationship the legitimate owner already holds (candidates: + *can manage the SA* — `use`/`update`/`patch`/`delete` on that + `serviceaccounts` object — or *capability parity* — the author can `collect` + the forwarded inputs). "Can manage the SA" closes paths 2 and 3 at their common + root (referencing a foreign privileged SA); "capability parity" targets path 2 + specifically. +- Alternatively, ship the same check as `[Warn, Audit]` first (denies nobody, + surfaces which CLFs *would* be blocked), then flip to `Deny` in a later release + once the fleet is provisioned. + +Token side channels (reading the `-token` Secret, `serviceaccounts/token`, +`pods/exec` into the collector) remain RBAC concerns outside admission — see §4. + +--- + +## 1. Current behavior + +### 1.1 How collector ServiceAccounts are created and used + +- The collector SA is **not** created by CLO in the observability path. The + administrator pre-creates it; the CLF references it **by name** through the + required field `spec.serviceAccount.name` + (`api/observability/v1/clusterlogforwarder_types.go:78-88`). +- `factory.ResourceNames` sets `ServiceAccount = clf.Spec.ServiceAccount.Name` + verbatim (`internal/factory/resource_names.go:45`) and wires it into the pod + spec at `internal/collector/collector.go:158` + (`ServiceAccountName: f.ResourceNames.ServiceAccount`). +- CLO only **Gets** the SA (`internal/controller/observability/collector.go:52`); + it then **binds RBAC/SCC to it**: + - `use` on the `logging-scc` SCC via a namespaced Role+RoleBinding + (`internal/auth/rbac.go:108-142`; SCC defined in + `internal/auth/securitycontextconstraint.go:33-52` — + `AllowHostDirVolumePlugin=true`, `RunAsUser=RunAsAny`, + `SELinuxContext=RunAsAny`). + - `metadata-reader` ClusterRoleBinding → cluster-wide get/list/watch on + pods, namespaces, nodes (`internal/auth/rbac.go:90-106`). + - `system:auth-delegator` ClusterRoleBinding (TokenReview/SAR) + (`internal/auth/rbac.go:73-87`). +- The admin is additionally expected to bind the `collect-{application, + infrastructure,audit}-logs` ClusterRoles (the `collect` verb on `logs`). +- Token exposure: a projected SA token is mounted at + `/var/run/ocp-collector/serviceaccount` (1h expiry, + `internal/collector/collector.go:349-368`). When an output uses + `BearerToken.From: serviceAccount`, a **long-lived** + `kubernetes.io/service-account-token` Secret named `-token` is created + and its token is sent to the output as `Authorization: Bearer` + (`internal/controller/observability/collector.go:49-64`, + `internal/generator/vector/output/common/auth.go:28-34`). + +### 1.2 How collector workloads are identified today + +Collector pods are created from a DaemonSet (or Deployment) named exactly +`clf.Name`, in the **CLF's own namespace** (not a reserved namespace). +Identifying metadata: + +| Attribute | Value | CLO-controlled | Spoofable by a namespace user | +|---|---|---|---| +| `app.kubernetes.io/name` | `vector` (constant) | yes | **yes** | +| `app.kubernetes.io/instance` | `clf.Name` | yes | **yes** (predictable) | +| `app.kubernetes.io/component` | `collector` (constant) | yes | **yes** | +| `app.kubernetes.io/part-of` / `managed-by` | constants | yes | **yes** | +| `vector.dev/exclude` | `true` (constant) | yes | **yes** | +| annotations (`secret-hash`, `configmap-hash`, workload-mgmt) | constants / content hashes | yes | **yes** (any value settable) | +| resource names | `clf.Name[-suffix]`, no random component | yes | **yes** (fully predictable) | +| `serviceAccountName` | `clf.Spec.ServiceAccount.Name` | user-supplied | **yes** | +| ownerReference **on the DaemonSet** | CLF CR, `UID=clf.UID`, controller=true | yes | UID **not** forgeable | +| ownerReference **on the Pod** | set by kube DaemonSet/ReplicaSet controller | no (kube) | user's standalone pod has different/no owner | + +The **only** non-forgeable attribute is the `UID` in the ownerReference of the +**DaemonSet/Deployment**. That is not present on the Pods, and CEL in a VAP +cannot dereference it to a live object. Conclusion: **no Pod-level metadata is a +trustworthy identity signal.** (Sources: +`internal/runtime/runtime.go:114-134`, `internal/collector/collector.go:119-158`, +`internal/factory/resource_names.go:35-51`, +`internal/factory/daemonset.go:13-27`, `internal/utils/utils.go:37-49`.) + +### 1.3 How the existing CLF authorization works + +`internal/validations/observability/validate_permissions.go` runs at **operator +reconcile time** (not admission) and issues SARs asking: *can +`system:serviceaccount::` do `collect` on `logs/`?* Failure sets +the `Authorized=False` status condition and tears the collector down. It checks +the **SA's** permissions, **not** the requesting user's right to use the SA, and +there is **no VAP or webhook** anywhere in the repo (only unrelated test +fixtures match `ValidatingAdmissionPolicy`). + +--- + +## 2. Threat model + +**Attack path (CLF write → SA reuse):** + +1. Attacker has `create/update` on `ClusterLogForwarder` in namespace `N` + (a namespaced, delegable permission). +2. Attacker discovers a privileged SA in `N` — e.g. a collector SA already + bound to `logging-scc` + `metadata-reader`, or any SA they can name. SA names + are predictable (`clf.Name`-derived) and enumerable. +3. **Even without touching a CLF**, the attacker creates their own Pod / + Deployment / DaemonSet in `N` with `serviceAccountName: `. +4. Kubernetes admits it (no control blocks SA reuse). The attacker's pod now + runs **as** that SA. + +**Privileges obtained:** + +- Via `logging-scc`: `hostPath` volumes (mount the node filesystem — read + `/var/log`, and with RunAsAny UID/`spc_t`, broad node-level read access), + RunAsAny UID (including 0), any SELinux context. This is a node-compromise + primitive. +- Via `metadata-reader`: cluster-wide enumeration of pods/namespaces/nodes. +- Via `system:auth-delegator`: mint/validate TokenReviews & SARs. +- If a `-token` Secret exists (token-forwarding outputs): a **long-lived** + bearer token for the SA, usable anywhere. + +The CLF write permission is the entry point, but the actual exploit is **reusing +the SA identity for a non-collector workload** — exactly the boundary we must +enforce. + +--- + +## 3. Proposed design + +Enforce, at Pod admission, the invariant: + +> A Pod that uses a **protected** collector SA may exist only if it is part of a +> workload tree **rooted in an object created by the CLO operator's own +> ServiceAccount.** + +The trust anchor is `request.userInfo.username` — the authenticated identity of +the API caller, set by the API server, **not** spoofable by the requester and +**not** derivable from any object field. This is what makes the boundary real. + +### 3.1 Identifying protected ServiceAccounts + +CEL in a VAP cannot fetch the SA object, so a per-SA label is not readable at +admission. Use an **operator-maintained param object** instead: + +- CLO reconciles a ConfigMap (e.g. `protected-collector-serviceaccounts` in the + operator namespace) whose `data` keys are `"/"` for every + SA referenced by any CLF (`clf.Namespace + "/" + clf.Spec.ServiceAccount.Name`). +- The VAP references this ConfigMap via `paramRef`. CEL tests membership. +- The ConfigMap is **always present** (created empty at startup) so CEL never + errors on a missing param — see §7 bootstrap. + +This is explicit, operator-controlled, multi-CLF / multi-namespace aware, and +strictly stronger than a naming convention. (A naming convention is *also* +spoofable input, so it cannot be the trust basis.) + +### 3.2 Identifying legitimate collector Pods + +Not by metadata — by **who created the object**: + +- The **only** non-controller identity permitted to create a protected-SA + workload is the operator SA + `system:serviceaccount::cluster-logging-operator` + (templated from the operator's own namespace). +- Built-in kube controllers propagate from an admitted root: + `deployment-controller` (Deployment→ReplicaSet), `replicaset-controller` + and `daemon-set-controller` (→Pods), all in `kube-system`. + +Inductive soundness: a controller only creates a child if its parent was +admitted. The parent chain always terminates at a DaemonSet/Deployment, which +only the operator SA may create. So allowing the controller SAs cannot admit a +user-rooted tree — the user's root object is denied before any controller runs. + +### 3.3 The admission rule + +Two policies (kept separate for clarity): + +- **Policy A — Pods:** if `spec.serviceAccountName` is protected, deny unless the + creator is `daemon-set-controller` or `replicaset-controller`. +- **Policy B — workload controllers** (`apps`: daemonsets, deployments, + replicasets, statefulsets; `batch`: jobs, cronjobs; core: + replicationcontrollers): if the pod template references a protected SA, deny + unless the creator is the operator SA or `deployment-controller`. + +| Case | Result | +|---|---| +| protected SA + operator-created DaemonSet/Deployment | allow (B) | +| protected SA + controller-created Pod (from admitted workload) | allow (A) | +| protected SA + user bare Pod (any copied metadata) | deny (A) | +| protected SA + user Deployment/DaemonSet/Job | deny (B) | +| unprotected SA + any Pod | pass through | + +--- + +## 4. Bypass analysis + +| Bypass attempt | Outcome | Why | +|---|---|---| +| Create a bare Pod with the protected SA | **denied** (A) | creator is the user, not a controller SA | +| Copy all collector labels/annotations/name onto the Pod | **denied** (A) | policy ignores metadata; keys on `request.userInfo` | +| Create a Deployment / DaemonSet / StatefulSet with the protected SA | **denied** (B) | creator ≠ operator SA | +| Create a Job/CronJob with the protected SA | **denied** (B) | creator ≠ operator SA; job-controller never runs | +| Forge an ownerReference to the real CLF/DaemonSet | **denied** | policy never trusts ownerRefs; a forged UID is rejected/GC'd by kube anyway | +| Modify an existing Pod to switch to the protected SA | **denied / impossible** | `serviceAccountName` is immutable on Pod UPDATE; UPDATE is also matched | +| Use another controller (custom operator) to create the Pod | **denied** (A) | that controller's SA is not in the allowlist | +| Run a look-alike ReplicaSet directly | **denied** (B) | the *user* is not the operator SA; deployment-controller is allowed but the user is not | + +**Residual paths the Pod VAP does NOT cover** (out of scope for pod admission — +governed by RBAC on the SA, not by running a workload): + +1. **Long-lived `-token` Secret** — a user with `get secret` in the + namespace reads the token directly, no pod needed. +2. **TokenRequest** — a user with `create` on `serviceaccounts/token` for the SA + mints a token directly. +3. **`pods/exec`** into the running collector pod → reach the mounted projected + token. + +These must be addressed separately (restrict `get` on the token Secret / prefer +projected tokens / restrict `serviceaccounts/token` and `pods/exec`). They do +not weaken the primary boundary (no *new workload* can assume the SA identity), +but the design note must state them honestly. + +**Robustness verdict:** because the policy keys on the non-spoofable authenticated +creator identity and never on object metadata, it satisfies the primary success +criterion — a CLF-writer who knows the SA name and reproduces every visible +collector attribute still cannot run an arbitrary workload as the protected SA. + +--- + +## 5. VAP / CEL PoC + +Param (reconciled by CLO; always present): + +```yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: protected-collector-serviceaccounts + namespace: openshift-logging # operator namespace +data: + "app-logging/collector-sa": "" # one key per / +``` + +Policy A — Pods: + +```yaml +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicy +metadata: + name: clo-protected-sa-pods +spec: + failurePolicy: Fail + paramKind: { apiVersion: v1, kind: ConfigMap } + matchConstraints: + resourceRules: + - apiGroups: [""] + apiVersions: ["v1"] + operations: ["CREATE", "UPDATE"] + resources: ["pods"] + variables: + - name: sa + expression: "has(object.spec.serviceAccountName) ? object.spec.serviceAccountName : 'default'" + - name: key + expression: "object.metadata.namespace + '/' + variables.sa" + - name: isProtected + expression: "variables.key in params.data" + validations: + - expression: > + !variables.isProtected || + request.userInfo.username in [ + 'system:serviceaccount:kube-system:daemon-set-controller', + 'system:serviceaccount:kube-system:replicaset-controller' + ] + message: "Pod uses a protected collector ServiceAccount but was not created by a CLO-managed collector controller" +--- +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicyBinding +metadata: + name: clo-protected-sa-pods +spec: + policyName: clo-protected-sa-pods + validationActions: ["Deny"] + paramRef: + name: protected-collector-serviceaccounts + namespace: openshift-logging + parameterNotFoundAction: Deny +``` + +Policy B — workload controllers (extract pod template per kind; CronJob nests +one level deeper): + +```yaml +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicy +metadata: + name: clo-protected-sa-workloads +spec: + failurePolicy: Fail + paramKind: { apiVersion: v1, kind: ConfigMap } + matchConstraints: + resourceRules: + - apiGroups: ["apps"] + apiVersions: ["v1"] + operations: ["CREATE", "UPDATE"] + resources: ["daemonsets", "deployments", "replicasets", "statefulsets"] + - apiGroups: ["batch"] + apiVersions: ["v1"] + operations: ["CREATE", "UPDATE"] + resources: ["jobs", "cronjobs"] + - apiGroups: [""] + apiVersions: ["v1"] + operations: ["CREATE", "UPDATE"] + resources: ["replicationcontrollers"] + variables: + - name: tmplSpec + expression: > + object.kind == 'CronJob' + ? object.spec.jobTemplate.spec.template.spec + : object.spec.template.spec + - name: sa + expression: "has(variables.tmplSpec.serviceAccountName) ? variables.tmplSpec.serviceAccountName : 'default'" + - name: key + expression: "object.metadata.namespace + '/' + variables.sa" + - name: isProtected + expression: "variables.key in params.data" + validations: + - expression: > + !variables.isProtected || + request.userInfo.username == 'system:serviceaccount:openshift-logging:cluster-logging-operator' || + request.userInfo.username == 'system:serviceaccount:kube-system:deployment-controller' + message: "Workload uses a protected collector ServiceAccount but was not created by the Cluster Logging Operator" +``` + +(Bind Policy B the same way. Template `openshift-logging` from the operator's own +namespace at reconcile time.) + +Representative test cases: + +- ALLOW: operator SA creates DaemonSet `app-logging` with `serviceAccountName: + collector-sa` (protected) → B allows; daemon-set-controller creates the Pod → + A allows. +- ALLOW: operator SA creates a Deployment → deployment-controller creates the + ReplicaSet (B allows) → replicaset-controller creates the Pod (A allows). +- DENY: `alice` creates a bare Pod with `collector-sa` and every collector label + copied → A denies. +- DENY: `alice` creates a DaemonSet/Deployment/Job with `collector-sa` → B denies. +- PASS: `alice` creates a Pod with `my-own-sa` (not in param) → not protected, + admitted normally. + +--- + +## 6. Alternative considered: a CLF-author `use`-verb VAP + +An earlier direction was a VAP on the **CLF resource** (CREATE/UPDATE) requiring +the CLF author to hold the `use` verb on the referenced SA: + +``` +!has(object.spec.serviceAccount) || !has(object.spec.serviceAccount.name) || +object.spec.serviceAccount.name == '' || +authorizer.group('').resource('serviceaccounts') + .namespace(object.metadata.namespace).name(object.spec.serviceAccount.name) + .check('use').allowed() +``` + +1. **What it would prevent:** a user referencing, *in a CLF*, an SA they are not + authorized to `use` — including the token-forwarding vector (CLO deploys a + collector as that SA and can forward its token to an output). +2. **What it fails to prevent:** SA reuse **outside** a CLF. It only guards the + `clusterlogforwarders` resource; a user could still create a bare + Pod/Deployment with the protected SA and get the SCC/RBAC privileges. The + broadened threat (arbitrary workload → node access) is **not** covered. +3. **Why it is not shipped:** the `use`-verb requirement forces admins to grant a + new, non-default verb to every CLF author, breaking existing installs on + upgrade (the "changes in SA" problem). This violates the hard upgrade + constraint. The Pod/workload VAP (this design) closes the broader and more + severe vector with **no** user/SA RBAC change, so it is the primary fix. +4. **If the token-forwarding-via-CLF vector must also be closed later,** prefer + doing it **without** a per-user `use` grant — e.g. constrain *which* SAs a CLF + may reference (a namespace-local SA the operator itself provisions/labels), or + gate token-forwarding outputs specifically, rather than requiring users to + obtain `use`. See §0 "Recommended future work for paths 2/3". Treat it as a + separate, explicitly-scoped decision. + +Also **retain** the reconcile-time SAR (`ValidatePermissions`): it verifies the +collector SA can actually `collect` and drives the `Authorized` status. It is +orthogonal to the VAP (validates SA capability, not who may run as the SA). + +Net: **the Pod/workload VAP is the primary mitigation**; the SAR is **retained**; +the CLF-author `use`-verb approach is **not pursued** (its RBAC-grant upgrade cost +is the blocker), and any future CLF-layer control must need no new per-user grants. + +--- + +## 7. Implementation recommendation + +**Components to change (all operator-side; no collector SA/SCC changes):** + +- New reconciler that maintains the param ConfigMap: on CLF add/update/delete, + recompute the set of `/` keys. Likely alongside + `internal/controller/observability/` and `internal/auth/`. +- New reconciler that ensures the two VAPs + bindings exist and are self-healing + (reuse the `internal/reconcile` CreateOrUpdate pattern). Template the operator + namespace into the operator-SA username via the downward API / existing + `OPERATOR_NAME` env. +- Ship the VAP/param as **operator-reconciled objects**, not static OLM bundle + manifests, so the operator-SA username and param stay in sync and self-heal. + +**RBAC:** + +- **No change to collector SAs, their RBAC, or SCC bindings** — satisfies the + "no user migration / no new SA permissions" constraint. +- The **operator's own** ClusterRole needs `admissionregistration.k8s.io` + (`validatingadmissionpolicies`, `validatingadmissionpolicybindings`: + create/update/patch/delete/get/list/watch) plus create/update on the param + ConfigMap. This ships via the CSV and is granted automatically on upgrade — it + is an operator permission, not a user- or collector-facing one. + +**Bootstrap / upgrade behavior:** + +- Fresh install: operator creates the (empty) param first, then the policies. + With the param always present, `failurePolicy: Fail` is safe (CEL never errors + on a missing param). +- Upgrade: operator gains RBAC via the CSV, reconciles param + policies; + existing collectors keep running because the operator SA is allowlisted and it + owns them. +- Reconcile / CLF create-update / rollout: operator-created and + controller-propagated → always allowed. +- Policy temporarily missing (before first reconcile): fail-open window during + which SA reuse is briefly unguarded; closes as soon as the operator reconciles. + Acceptable and self-correcting; the operator is never locked out of its own + workloads. +- Self-lockout guard: keep the param ConfigMap always present, and template the + correct operator-SA username; otherwise a wrong username in Policy B would + block the operator from deploying collectors. + +**Feasibility verdict:** the approach is technically sound and provides a real +security boundary, because it never relies on spoofable Pod metadata. The one +caveat is the token side channels in §4 (token Secret, TokenRequest, +pods/exec), which are RBAC concerns outside pod admission and should be tracked +separately. diff --git a/docs/design/validatingadmissionpolicy-guide.md b/docs/design/validatingadmissionpolicy-guide.md new file mode 100644 index 0000000000..c7ef6c45d0 --- /dev/null +++ b/docs/design/validatingadmissionpolicy-guide.md @@ -0,0 +1,462 @@ +# A practical guide to ValidatingAdmissionPolicy (VAP) — as used to protect collector ServiceAccounts + +Audience: CLO developers new to ValidatingAdmissionPolicy. This explains what a +VAP is, how each piece is configured, and then walks through the *actual* +policies this operator ships to stop a protected collector ServiceAccount (SA) +from being reused by arbitrary workloads. + +Companion docs: +- `docs/design/protect-collector-serviceaccounts.md` — the *why* (threat model, design rationale). +- This file — the *how* (VAP mechanics + a line-by-line reading of our manifests). + +> Scope: this ships the **protected-SA Pod/workload VAP only** (blocks reusing a +> collector SA in an arbitrary pod → node access). The CLF-layer control that +> would also block "forward logs you can't access" / "exfiltrate a token" is a +> deliberate follow-up — see §0 of the design note for why and the recommended +> future design. + +Source of truth (read alongside this doc): +- `internal/admission/protected-sa-pods.yaml` + `-binding.yaml` +- `internal/admission/protected-sa-workloads.yaml` + `-binding.yaml` +- `internal/admission/protected_sa_policy.go` (reconcile + param ConfigMap) +- `internal/controller/admission/protected_sa_controller.go` / `_runnable.go` (when it runs) + +--- + +## 1. What problem does admission control solve? + +Every write to the Kubernetes API (`CREATE`, `UPDATE`, `DELETE`, `CONNECT`) +passes through a pipeline before it is persisted to etcd: + +``` +client → authentication → authorization (RBAC) → admission → etcd + ├── mutating admission + └── validating admission ← VAP runs here +``` + +- **Authorization (RBAC)** answers *"is this identity allowed to create a Pod in + this namespace?"* — a coarse yes/no on the verb+resource. +- **Admission** answers the finer question *"is the **content** of this specific + object acceptable?"* — e.g. "this Pod may create a Pod, but not one that uses + *that particular* ServiceAccount." + +RBAC cannot express our rule, because our rule depends on the *combination* of +**who** is making the request and **which SA the Pod references**. That is +exactly what admission control is for. + +Historically that meant writing a **webhook** (a separate HTTPS service the +API server calls out to). **ValidatingAdmissionPolicy (VAP)** is the newer, +in-process alternative: you declare the rule in **CEL** (Common Expression +Language) inside a normal Kubernetes resource, and the API server evaluates it +itself. No webhook server, no certificates, no extra pod to run or scale. + +> Availability: VAP is GA since Kubernetes 1.30 (OpenShift 4.17+). On older +> clusters the API is absent; our runnable detects that and silently skips +> installation (see `isUnsupportedAdmissionPolicyAPI` in the runnable). + +--- + +## 2. The three objects that make up a VAP + +A working policy is **three** resources. Keeping them separate is the whole +design of the feature — the same policy can be bound multiple times with +different parameters. + +| Object | Kind | Answers | Cluster/namespaced | +|---|---|---|---| +| **Policy** | `ValidatingAdmissionPolicy` | *What is the rule?* (the CEL logic) | cluster-scoped | +| **Binding** | `ValidatingAdmissionPolicyBinding` | *Where does it apply and what happens on failure?* | cluster-scoped | +| **Param** | any resource (here a `ConfigMap`) | *What data does the rule read?* | namespaced (or cluster) | + +Think of it as a function: + +``` +Policy = the function body (CEL) +Param = the arguments passed in (params.*) +Binding = "call the function with these args, and Deny if it returns false" +``` + +A Policy alone does **nothing** until a Binding activates it. This trips people +up: you can `oc get validatingadmissionpolicy` and see it installed, yet nothing +is enforced because no Binding exists (or the Binding's `validationActions` is +not `Deny`). + +--- + +## 3. Anatomy of the Policy object + +Here is `protected-sa-pods.yaml`, annotated field by field. + +```yaml +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicy +metadata: + name: clo-protected-sa-pods +spec: + failurePolicy: Fail # (A) + paramKind: # (B) + apiVersion: v1 + kind: ConfigMap + matchConstraints: # (C) + resourceRules: + - apiGroups: [""] + apiVersions: ["v1"] + operations: ["CREATE", "UPDATE"] + resources: ["pods"] + variables: # (D) + - name: sa + expression: "has(object.spec.serviceAccountName) ? object.spec.serviceAccountName : 'default'" + - name: saKey + expression: "'sa_' + request.namespace + '_' + variables.sa" + - name: isProtected + expression: "has(params.data) && (variables.saKey in params.data)" + - name: allowedCreators + expression: "(has(params.data) && ('podCreators' in params.data)) ? params.data['podCreators'].split(',') : []" + validations: # (E) + - expression: "!variables.isProtected || (request.userInfo.username in variables.allowedCreators)" + messageExpression: "'Pod uses protected collector ServiceAccount \"' + variables.sa + '\" and may only be created by a CLO-managed collector controller'" + reason: Forbidden +``` + +### (A) `failurePolicy: Fail` — what if the rule itself errors? + +If CEL evaluation *errors* (e.g. references a field that doesn't exist, or the +param is malformed), `Fail` means **the request is denied**. The alternative, +`Ignore`, would admit the request. We choose `Fail` (fail-closed) so a broken +policy can never silently let an attacker through. + +> This is subtly different from "param not found" — that is handled by the +> **binding**, see §4. We fail *closed* on evaluation errors but *open* on a +> missing param ConfigMap, so the operator can never lock itself out. This +> split is deliberate. + +### (B) `paramKind` — what type of parameter this policy reads + +Declares that `params` (used in the CEL) is a `ConfigMap`. This only names the +*type*; the specific instance is chosen by the binding's `paramRef` (§4). Omit +`paramKind` entirely if a policy needs no external data. + +### (C) `matchConstraints` — which API requests trigger this policy + +The API server only evaluates the policy for requests matching these rules. +Here: `CREATE` or `UPDATE` of core `v1` `pods`. Everything else (services, +configmaps, pod *deletes*, …) skips this policy entirely. + +Why `UPDATE` too? So an attacker cannot create an innocent Pod and then *edit* +it to point at the protected SA. (In practice `serviceAccountName` is immutable +on Pods, but matching UPDATE is defence-in-depth and is required for the +workload kinds where the template *is* mutable.) + +### (D) `variables` — reusable sub-expressions, evaluated top to bottom + +Variables keep the final rule readable and are referenced as `variables.`. +Each can use the ones above it. The objects available to CEL: + +| CEL binding | What it is | +|---|---| +| `object` | the incoming resource (the Pod being created) | +| `oldObject` | previous version (on UPDATE; null on CREATE) | +| `request` | admission metadata — `request.userInfo`, `request.namespace`, `request.operation`, `request.kind`, … | +| `params` | the bound param object (the ConfigMap) | +| `authorizer` | lets CEL run authorization checks (not used here) | + +Our four variables: + +1. **`sa`** — the Pod's ServiceAccount, defaulting to `"default"` when the field + is absent (Kubernetes does the same). Guarding with `has(...)` avoids a + "no such field" evaluation error. +2. **`saKey`** — builds the lookup key `sa__`. **Note it uses + `request.namespace`, not `object.metadata.namespace`.** `request.namespace` + is set by the API server and always populated; `object.metadata.namespace` + can be empty on the incoming object (the server fills it in later), so it is + the wrong thing to trust. ⚠️ The key format is `_`-separated, not `/`-separated + — see the gotcha in §7. +3. **`isProtected`** — is this SA in the protected set? The set lives in the + ConfigMap's `data` keys. `has(params.data)` guards the case where the + ConfigMap exists but has no `data` at all. +4. **`allowedCreators`** — the allow-list of usernames permitted to create a + protected-SA Pod, read from the ConfigMap's `podCreators` key (a + comma-separated string, split into a list). Empty list if the key is absent. + +### (E) `validations` — the actual rule + +`expression` must evaluate to **`true` to ALLOW**. `false` → the action in the +binding (Deny) fires. Read ours as: + +> Allow **if** the SA is *not* protected, **OR** the caller's username is in the +> allow-list. + +``` +!variables.isProtected || (request.userInfo.username in variables.allowedCreators) +``` + +- Not a protected SA? `!isProtected` is `true` → allowed, short-circuits. Zero + impact on every other workload in the cluster. +- Protected SA? Then it is allowed **only** if + `request.userInfo.username` — the authenticated caller the API server + stamped on the request — is an allowed creator. + +**This is the crux of the whole design: we key on `request.userInfo.username`, +which the requester cannot forge.** We deliberately never look at Pod labels, +annotations, names, or ownerReferences, because a user who can create Pods can +reproduce *all* of those. Identity is the only trustworthy signal. + +`messageExpression` builds the denial message shown to the user (a CEL string +so it can embed the SA name); `reason: Forbidden` maps to HTTP 403. + +--- + +## 4. Anatomy of the Binding object + +```yaml +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicyBinding +metadata: + name: clo-protected-sa-pods-binding +spec: + policyName: clo-protected-sa-pods # which policy to activate + validationActions: [Deny] # what to do when a validation returns false + paramRef: # which concrete param object to pass in + name: clo-protected-serviceaccounts + namespace: openshift-logging # overwritten at reconcile time (see §6) + parameterNotFoundAction: Allow # missing param → admit (fail OPEN) + matchResources: {} # no extra narrowing beyond the policy's matchConstraints +``` + +Key fields: + +- **`policyName`** — links this binding to the policy above. +- **`validationActions`** — what a failing validation does. Options: + - `Deny` — reject the request (what we use). + - `Warn` — admit but return a warning header (great for a dry-run rollout). + - `Audit` — admit but record in the audit log. + You can combine e.g. `[Warn, Audit]` to observe impact before switching to + `[Deny]` — a recommended way to roll a new policy out safely. +- **`paramRef`** — points at the specific ConfigMap instance. `parameterNotFoundAction`: + - `Allow` — if the ConfigMap is missing, **admit** the request (fail open). + - `Deny` — if missing, reject. + We use **`Allow`** on purpose: if the param ConfigMap were ever deleted, we do + *not* want to brick the whole cluster's Pod creation. Combined with + `failurePolicy: Fail` on the policy, the net behaviour is: *fail closed on a + broken rule, fail open on a missing param.* +- **`matchResources`** — an *additional* filter on top of the policy's + `matchConstraints` (e.g. restrict to certain namespaces via labels). Empty + `{}` means "no extra narrowing." + +--- + +## 5. Anatomy of the param ConfigMap + +The operator maintains one ConfigMap, `clo-protected-serviceaccounts`, in its +own namespace. Example contents on a live cluster: + +```yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: clo-protected-serviceaccounts + namespace: openshift-logging +data: + # --- protected SA membership: one key per CLF's SA, value is empty --- + sa_app-logging_collector-sa: "" + sa_team-b_log-collector: "" + + # --- allow-lists of creator identities, comma-separated --- + podCreators: "system:serviceaccount:kube-system:daemon-set-controller,system:serviceaccount:kube-system:replicaset-controller" + workloadCreators: "system:serviceaccount:openshift-logging:cluster-logging-operator,system:serviceaccount:kube-system:deployment-controller" +``` + +Two kinds of entries: + +1. **Protected-SA keys** (`sa__: ""`) — the *set* of SAs to protect. + Only the key matters; the value is empty. CEL tests membership with + `variables.saKey in params.data`. +2. **Creator allow-lists** — `podCreators` and `workloadCreators`, each a + comma-separated username list that CEL `.split(',')`s. Built by + `setCreatorKeys()` in `protected_sa_policy.go`: + - `podCreators` = `daemon-set-controller`, `replicaset-controller` — the + built-in controllers that create *Pods* from an already-admitted workload. + - `workloadCreators` = the **operator SA** + `deployment-controller` — who may + create the top-level *workload* (Deployment→ReplicaSet, DaemonSet). + +### Why this two-level structure is sound + +A built-in controller only creates a child object if its parent was already +admitted. The parent chain always terminates at a Deployment/DaemonSet, which +**only the operator SA** may create (`workloadCreators`). So allowing +`daemon-set-controller`/`replicaset-controller` to create Pods cannot admit a +*user-rooted* tree — the user's root object is denied first, before any +controller runs. A user's *bare* Pod has the user as creator, not a controller, +so it is denied. + +--- + +## 6. How the operator installs and maintains all this + +Nothing above is a static OLM manifest — it is **operator-reconciled**, for two +reasons: (a) the operator-SA username in `workloadCreators` depends on the +install namespace, and (b) the protected-SA set changes as CLFs come and go. + +There are two moving parts: + +### 6a. One-shot install of the policies + bindings (`protected_sa_runnable.go`) + +`NewProtectedSAAdmissionRunnable` runs once after the manager cache starts +(leader-elected, so only one operator pod does it). It calls +`ReconcileProtectedSAPolicies` with exponential backoff, which: + +1. Ensures the param ConfigMap exists with the creator allow-lists populated + (`ensureProtectedSAConfigMap` → `setCreatorKeys`). +2. Does an initial `SyncProtectedServiceAccounts` (best-effort). +3. Creates/updates both Policies and both Bindings from the embedded manifests + (`//go:embed`), **overwriting each binding's `paramRef.namespace` with the + operator's real namespace** (line: `binding.Spec.ParamRef.Namespace = operatorNS`). + +If the cluster's API server doesn't support VAP, it detects the error and skips. + +### 6b. Keeping the protected-SA set current (`protected_sa_controller.go`) + +`ProtectedSAReconciler` is a controller-runtime reconciler watching +`ClusterLogForwarder`. On **every** CLF create/update/delete it calls +`SyncProtectedServiceAccounts`, which: + +- Lists **all** CLFs cluster-wide. +- Rebuilds the set of `sa__` keys from scratch. +- Rewrites the ConfigMap `data` (membership keys + creator allow-lists). + +Rebuilding from the full list (rather than incrementally adding/removing) is why +**deletion needs no finalizer** and the ConfigMap is **self-healing**: whatever +the current CLFs are, the ConfigMap converges to match. + +``` +CLF created/updated/deleted + │ + ▼ +ProtectedSAReconciler.Reconcile + │ + ▼ +SyncProtectedServiceAccounts ──rebuild──► clo-protected-serviceaccounts ConfigMap + │ (paramRef) + ▼ + VAPs read it on every Pod/workload CREATE/UPDATE +``` + +### Operator RBAC required + +The operator's own ClusterRole needs (shipped via the CSV, granted on upgrade — +these are operator permissions, not user- or collector-facing): + +- `admissionregistration.k8s.io`: `validatingadmissionpolicies`, + `validatingadmissionpolicybindings` — create/update/patch/delete/get/list/watch. +- create/update on the param ConfigMap in the operator namespace. + +**No changes to collector SAs, their RBAC, or SCC** — that is the entire point: +the collector keeps working exactly as before, and only the "reuse the SA for +another workload" move is blocked (see the design note). + +--- + +## 7. Gotchas we already hit (learn from these) + +1. **ConfigMap data keys cannot contain `/`.** They must match + `[-._a-zA-Z0-9]+`. The first design used `sa//` and every sync + failed — and because the binding fails *open* (`parameterNotFoundAction: + Allow`), nothing was denied, which looked like a passing test. Fixed to + `sa__`; both namespaces and SA names forbid `_`, so the encoding is + collision-free. **The CEL in the manifest builds the exact same key + (`'sa_' + request.namespace + '_' + variables.sa`) — if you change the format + in Go, change it in both manifests too.** + +2. **The fake client does not validate ConfigMap key charset.** Unit tests with + the fake client passed while the live cluster rejected the key. Validate + VAP/param prototypes against a **real** API server (envtest or a cluster), + not just the fake client. That is why we added the envtest suite. + +3. **A newly created VAP is not enforced instantly.** The API server compiles + and loads policies asynchronously (seconds). Tests must poll (an `Eventually` + that keeps trying a known-bad create until it is denied) before asserting the + deterministic cases. See the "canary" loop in the envtest. + +4. **A Policy without a Binding does nothing**, and a Binding without + `validationActions: [Deny]` only warns/audits. If "nothing is being blocked," + check the Binding first. + +5. **Use `request.namespace`, not `object.metadata.namespace`** for the key — + the latter can be empty on the incoming object. + +6. **Redeploy the right thing.** If you change the CEL manifests, roll the + operator so it re-reconciles. If you change the Go reconcile logic, + rebuild/redeploy the operator image. A stale operator with new manifests + (or vice-versa) produces confusing results. + +--- + +## 8. How to operate and debug it + +Inspect what is installed: + +```bash +oc get validatingadmissionpolicy clo-protected-sa-pods -o yaml +oc get validatingadmissionpolicybinding clo-protected-sa-pods-binding -o yaml +oc get configmap clo-protected-serviceaccounts -n openshift-logging -o yaml +``` + +Confirm a specific SA is protected (key contains a name, so match raw JSON — +jsonpath can't select keys with dots): + +```bash +oc get configmap clo-protected-serviceaccounts -n openshift-logging -o json \ + | grep 'sa__' +``` + +Manually verify enforcement (should be denied): + +```bash +oc create --as system:serviceaccount::some-user -n -f - <<'EOF' +apiVersion: v1 +kind: Pod +metadata: { name: probe } +spec: + serviceAccountName: + containers: [{ name: c, image: registry.redhat.io/ubi9/ubi-minimal:latest }] +EOF +# → Error ... "Pod uses protected collector ServiceAccount ... may only be created by a CLO-managed collector controller" +``` + +Roll out safely on a new policy: set `validationActions: [Warn, Audit]` first, +watch for unexpected warnings/audit entries, then switch to `[Deny]`. + +If enforcement seems off, check in this order: (1) does the Binding exist with +`[Deny]`? (2) does the ConfigMap have the `sa__` key? (3) is the caller +accidentally in `podCreators`/`workloadCreators`? (4) is the cluster new enough +for VAP? + +--- + +## 9. Test coverage (three layers) + +| Layer | File | What it proves | +|---|---|---| +| Unit (fake client) | `internal/admission/protected_sa_policy_test.go` | The generated objects (keys, creator lists, decoded manifests) are shaped correctly. Fast, no cluster. | +| **envtest** (real apiserver) | `internal/admission/protected_sa_envtest_test.go` | The **CEL is actually compiled and enforced**. Catches invalid keys / CEL regressions. Run with `make test-admission-envtest`; skips cleanly when `KUBEBUILDER_ASSETS` is unset. | +| e2e (live cluster) | `test/e2e/collection/admission/` | End-to-end with `oc create --as` impersonation against a real OpenShift cluster. | + +The envtest is the layer that would have caught the `/`-in-key gotcha, because +it runs a genuine kube-apiserver rather than the fake client. + +--- + +## 10. One-paragraph summary + +A **ValidatingAdmissionPolicy** lets the API server itself enforce a CEL rule at +admission time — no webhook. It is three objects: the **Policy** (the CEL rule + +which requests it matches + fail-closed behaviour), the **Binding** (activates +the policy, says `Deny`, and points at the param, failing *open* if the param is +gone), and a **param ConfigMap** the operator keeps in sync. Our rule reads +*"if the Pod uses a protected SA, allow only if the authenticated creator +(`request.userInfo.username`) is on the allow-list."* Because it keys on the +non-forgeable caller identity and never on Pod metadata, copying the collector's +labels/name/annotations does not bypass it — which is exactly the security +boundary we need, achieved without changing any user or ServiceAccount RBAC. + + diff --git a/docs/features/collection.adoc b/docs/features/collection.adoc index 5a97e42a87..987b229057 100644 --- a/docs/features/collection.adoc +++ b/docs/features/collection.adoc @@ -97,6 +97,7 @@ a| |https://issues.redhat.com/browse/LOG-3270[TLS Security Profile Compliance] |Comply with OCP cluster-wide cryptographic profiles for internal communication and allow configuration of outbound connection profiles. See link:./tls_security_profile.adoc[details] |https://issues.redhat.com/browse/LOG-7571[Network Policy]| Network policy in place for the collectors that allows all egress and ingress. +|link:../../docs/design/protect-collector-serviceaccounts.md[Protected collector ServiceAccounts]|ValidatingAdmissionPolicy prevents a collector ServiceAccount from being reused by an arbitrary Pod or workload (CVE-2026-10609) |====== === Tuning diff --git a/hack/test-protected-sa.sh b/hack/test-protected-sa.sh new file mode 100755 index 0000000000..88b9dc2b43 --- /dev/null +++ b/hack/test-protected-sa.sh @@ -0,0 +1,220 @@ +#!/usr/bin/env bash +# +# Manual test for protected collector ServiceAccount admission (LOG-9714 follow-up): +# ValidatingAdmissionPolicies restrict a protected collector ServiceAccount so it +# can only be used by CLO-managed collector workloads. +# +# The trust anchor is request.userInfo (the authenticated creator), NOT Pod +# metadata. This script proves that a user who copies the collector's visible +# labels/annotations/name still cannot run a Pod/Deployment as a protected SA. +# +# This is a lightweight admission test: it verifies deny/allow at CREATE only. +# The CLF may be Not Ready (no collect roles / no LokiStack) — that is expected. +# +# Prerequisites: +# - oc logged in as cluster-admin +# - cluster-logging-operator running with protected-SA VAP reconciliation +# +# Usage: +# ./hack/test-protected-sa.sh +# NS=my-test ./hack/test-protected-sa.sh +# ./hack/test-protected-sa.sh --no-cleanup +# ./hack/test-protected-sa.sh --cleanup-only +# +set -euo pipefail + +OC="${OC:-oc}" +NS="${NS:-protected-sa-manual-test}" +OPERATOR_NS="${OPERATOR_NS:-openshift-logging}" +COLLECTOR_SA="${COLLECTOR_SA:-log-collector}" +UNPROTECTED_SA="${UNPROTECTED_SA:-plain-sa}" +RESTRICTED_SA="${RESTRICTED_SA:-restricted-user}" +CLF_NAME="${CLF_NAME:-protected-sa-test}" +CONFIGMAP="${CONFIGMAP:-clo-protected-serviceaccounts}" +CLEANUP_ONLY=false +NO_CLEANUP=false + +usage() { sed -n '2,20p' "$0" | sed 's/^# \?//'; exit "${1:-0}"; } + +while [[ $# -gt 0 ]]; do + case "$1" in + -h|--help) usage 0 ;; + --cleanup-only) CLEANUP_ONLY=true; shift ;; + --no-cleanup) NO_CLEANUP=true; shift ;; + *) echo "Unknown option: $1" >&2; usage 1 ;; + esac +done + +pass() { echo "[PASS] $*"; } +fail() { echo "[FAIL] $*" >&2; exit 1; } +info() { echo "[INFO] $*"; } + +restricted_user() { echo "system:serviceaccount:${NS}:${RESTRICTED_SA}"; } + +cleanup() { + info "Cleaning up" + "${OC}" delete clusterlogforwarder "${CLF_NAME}" -n "${NS}" --ignore-not-found >/dev/null 2>&1 || true + "${OC}" delete ns "${NS}" --wait=true --timeout=120s --ignore-not-found >/dev/null 2>&1 || true +} + +check_vap() { + info "Checking ValidatingAdmissionPolicy resources" + "${OC}" get validatingadmissionpolicy clo-protected-sa-pods >/dev/null + "${OC}" get validatingadmissionpolicybinding clo-protected-sa-pods-binding >/dev/null + "${OC}" get validatingadmissionpolicy clo-protected-sa-workloads >/dev/null + "${OC}" get validatingadmissionpolicybinding clo-protected-sa-workloads-binding >/dev/null + pass "both VAPs and bindings exist" +} + +setup_namespace() { + info "Setting up namespace ${NS}" + "${OC}" create ns "${NS}" >/dev/null + "${OC}" create sa "${COLLECTOR_SA}" -n "${NS}" >/dev/null + "${OC}" create sa "${UNPROTECTED_SA}" -n "${NS}" >/dev/null + "${OC}" create sa "${RESTRICTED_SA}" -n "${NS}" >/dev/null + # Allow the restricted user to create Pods and Deployments in the namespace. + "${OC}" create role workload-editor -n "${NS}" \ + --verb=create,get,list,delete \ + --resource=pods,deployments.apps \ + --dry-run=client -o yaml | "${OC}" apply -f - >/dev/null + "${OC}" create rolebinding restricted-user-workload-editor -n "${NS}" \ + --role=workload-editor \ + --serviceaccount="${NS}:${RESTRICTED_SA}" \ + --dry-run=client -o yaml | "${OC}" apply -f - >/dev/null +} + +create_clf_marks_sa_protected() { + info "Creating CLF (as admin) so the operator marks ${COLLECTOR_SA} protected" + cat </dev/null +apiVersion: observability.openshift.io/v1 +kind: ClusterLogForwarder +metadata: + name: ${CLF_NAME} +spec: + serviceAccount: + name: ${COLLECTOR_SA} + outputs: + - name: test-output + type: lokiStack + lokiStack: + target: { name: lokistack, namespace: openshift-logging } + authentication: { token: { from: serviceAccount } } + pipelines: + - name: test-pipe + inputRefs: [application] + outputRefs: [test-output] +EOF + info "Waiting for operator to add sa_${NS}_${COLLECTOR_SA} to ${OPERATOR_NS}/${CONFIGMAP}" + # Key format is sa__ (ConfigMap keys can't contain '/'). + local key="sa_${NS}_${COLLECTOR_SA}" deadline=$((SECONDS + 60)) + while true; do + if "${OC}" get configmap "${CONFIGMAP}" -n "${OPERATOR_NS}" -o json 2>/dev/null \ + | grep -q "\"${key}\""; then + pass "operator marked ${COLLECTOR_SA} protected" + return 0 + fi + (( SECONDS >= deadline )) && fail "operator did not mark SA protected within timeout (is CLO running?)" + sleep 2 + done +} + +# A Pod spec that copies the collector's visible metadata to prove spoofing fails. +spoofed_pod_yaml() { + local sa="$1" name="$2" + cat < DENY" + if out="$(spoofed_pod_yaml "${COLLECTOR_SA}" evil-pod | "${OC}" create --as="$(restricted_user)" -f - 2>&1)"; then + fail "Pod create succeeded but was expected to be denied: ${out}" + fi + grep -qi 'protected collector ServiceAccount' <<<"${out}" \ + || fail "Pod denied but message unexpected: ${out}" + pass "bare Pod with protected SA denied despite copied metadata" +} + +test_deployment_denied_with_protected_sa() { + info "Test: restricted user Deployment with protected SA => DENY" + if out="$(deployment_yaml "${COLLECTOR_SA}" evil-deploy | "${OC}" create --as="$(restricted_user)" -f - 2>&1)"; then + fail "Deployment create succeeded but was expected to be denied: ${out}" + fi + grep -qi 'protected collector ServiceAccount' <<<"${out}" \ + || fail "Deployment denied but message unexpected: ${out}" + pass "Deployment with protected SA denied" +} + +test_pod_allowed_with_unprotected_sa() { + info "Test: restricted user Pod with UNprotected SA => ALLOW (pass-through)" + if ! out="$(spoofed_pod_yaml "${UNPROTECTED_SA}" plain-pod | "${OC}" create --as="$(restricted_user)" -f - 2>&1)"; then + fail "Pod with unprotected SA failed but was expected to succeed: ${out}" + fi + pass "Pod with unprotected SA admitted" +} + +main() { + command -v "${OC}" >/dev/null 2>&1 || fail "oc not found (set OC=...)" + "${OC}" whoami >/dev/null 2>&1 || fail "not logged in ($OC whoami failed)" + + if [[ "${CLEANUP_ONLY}" == true ]]; then cleanup; pass "cleanup complete"; exit 0; fi + + check_vap + setup_namespace + create_clf_marks_sa_protected + test_pod_denied_with_protected_sa + test_deployment_denied_with_protected_sa + test_pod_allowed_with_unprotected_sa + + echo + pass "all protected-SA admission tests passed" + info "Allow path for real collector pods is exercised by the operator itself" + + if [[ "${NO_CLEANUP}" == false ]]; then cleanup; info "namespace ${NS} removed"; else info "leaving namespace ${NS} (--no-cleanup)"; fi +} + +main "$@" diff --git a/internal/admission/policy.go b/internal/admission/policy.go new file mode 100644 index 0000000000..5c95e374bc --- /dev/null +++ b/internal/admission/policy.go @@ -0,0 +1,85 @@ +package admission + +import ( + "context" + "errors" + "fmt" + "time" + + log "github.com/ViaQ/logerr/v2/log/static" + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + apiruntime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/discovery" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +var AdmissionReconcileBackoff = wait.Backoff{ + Steps: 5, + Duration: 2 * time.Second, + Factor: 2.0, + Cap: 30 * time.Second, +} + +func reconcileValidatingAdmissionPolicy(ctx context.Context, k8sClient client.Client, desired *admissionregistrationv1.ValidatingAdmissionPolicy) error { + current := &admissionregistrationv1.ValidatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: desired.Name, + }, + } + + op, err := controllerutil.CreateOrUpdate(ctx, k8sClient, current, func() error { + current.Spec = desired.Spec + return nil + }) + if err != nil { + if IsUnsupportedAdmissionPolicyAPI(err) { + log.Info("ValidatingAdmissionPolicy API is unavailable; skipping admission policy", "name", desired.Name) + return nil + } + return fmt.Errorf("reconcile ValidatingAdmissionPolicy %q: %w", desired.Name, err) + } + + log.V(3).Info("reconciled ValidatingAdmissionPolicy", "name", desired.Name, "operation", op) + return nil +} + +func reconcileValidatingAdmissionPolicyBinding(ctx context.Context, k8sClient client.Client, desired *admissionregistrationv1.ValidatingAdmissionPolicyBinding) error { + current := &admissionregistrationv1.ValidatingAdmissionPolicyBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: desired.Name, + }, + } + + op, err := controllerutil.CreateOrUpdate(ctx, k8sClient, current, func() error { + current.Spec = desired.Spec + return nil + }) + if err != nil { + if IsUnsupportedAdmissionPolicyAPI(err) { + log.Info("ValidatingAdmissionPolicyBinding API is unavailable; skipping admission policy binding", "name", desired.Name) + return nil + } + return fmt.Errorf("reconcile ValidatingAdmissionPolicyBinding %q: %w", desired.Name, err) + } + + log.V(3).Info("reconciled ValidatingAdmissionPolicyBinding", "name", desired.Name, "operation", op) + return nil +} + +func IsUnsupportedAdmissionPolicyAPI(err error) bool { + if err == nil { + return false + } + if meta.IsNoMatchError(err) { + return true + } + if apiruntime.IsNotRegisteredError(err) { + return true + } + var groupDiscoveryErr *discovery.ErrGroupDiscoveryFailed + return errors.As(err, &groupDiscoveryErr) +} diff --git a/internal/admission/protected-sa-pods-binding.yaml b/internal/admission/protected-sa-pods-binding.yaml new file mode 100644 index 0000000000..490fbcacf8 --- /dev/null +++ b/internal/admission/protected-sa-pods-binding.yaml @@ -0,0 +1,13 @@ +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicyBinding +metadata: + name: clo-protected-sa-pods-binding +spec: + policyName: clo-protected-sa-pods + validationActions: [Deny] + # namespace is overridden at reconcile time with the operator's own namespace. + paramRef: + name: clo-protected-serviceaccounts + namespace: openshift-logging + parameterNotFoundAction: Allow + matchResources: {} diff --git a/internal/admission/protected-sa-pods.yaml b/internal/admission/protected-sa-pods.yaml new file mode 100644 index 0000000000..16099c225b --- /dev/null +++ b/internal/admission/protected-sa-pods.yaml @@ -0,0 +1,31 @@ +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicy +metadata: + name: clo-protected-sa-pods +spec: + # Fail closed: if the policy cannot be evaluated the request is denied. + # The paramRef in the binding uses parameterNotFoundAction: Allow so a + # missing param ConfigMap does not lock the operator out (see binding). + failurePolicy: Fail + paramKind: + apiVersion: v1 + kind: ConfigMap + matchConstraints: + resourceRules: + - apiGroups: [""] + apiVersions: ["v1"] + operations: ["CREATE", "UPDATE"] + resources: ["pods"] + variables: + - name: sa + expression: "has(object.spec.serviceAccountName) ? object.spec.serviceAccountName : 'default'" + - name: saKey + expression: "'sa_' + request.namespace + '_' + variables.sa" + - name: isProtected + expression: "has(params.data) && (variables.saKey in params.data)" + - name: allowedCreators + expression: "(has(params.data) && ('podCreators' in params.data)) ? params.data['podCreators'].split(',') : []" + validations: + - expression: "!variables.isProtected || (request.userInfo.username in variables.allowedCreators)" + messageExpression: "'Pod uses protected ServiceAccount \"' + request.namespace + '/' + variables.sa + '\" which is only allowed for use by authorized ClusterLogForwarders'" + reason: Forbidden diff --git a/internal/admission/protected-sa-workloads-binding.yaml b/internal/admission/protected-sa-workloads-binding.yaml new file mode 100644 index 0000000000..171d25024c --- /dev/null +++ b/internal/admission/protected-sa-workloads-binding.yaml @@ -0,0 +1,13 @@ +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicyBinding +metadata: + name: clo-protected-sa-workloads-binding +spec: + policyName: clo-protected-sa-workloads + validationActions: [Deny] + # namespace is overridden at reconcile time with the operator's own namespace. + paramRef: + name: clo-protected-serviceaccounts + namespace: openshift-logging + parameterNotFoundAction: Allow + matchResources: {} diff --git a/internal/admission/protected-sa-workloads.yaml b/internal/admission/protected-sa-workloads.yaml new file mode 100644 index 0000000000..4f43f0e5fe --- /dev/null +++ b/internal/admission/protected-sa-workloads.yaml @@ -0,0 +1,41 @@ +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicy +metadata: + name: clo-protected-sa-workloads +spec: + failurePolicy: Fail + paramKind: + apiVersion: v1 + kind: ConfigMap + matchConstraints: + resourceRules: + - apiGroups: ["apps"] + apiVersions: ["v1"] + operations: ["CREATE", "UPDATE"] + resources: ["daemonsets", "deployments", "replicasets", "statefulsets"] + - apiGroups: ["batch"] + apiVersions: ["v1"] + operations: ["CREATE", "UPDATE"] + resources: ["jobs", "cronjobs"] + - apiGroups: [""] + apiVersions: ["v1"] + operations: ["CREATE", "UPDATE"] + resources: ["replicationcontrollers"] + variables: + - name: podSpec + expression: >- + request.kind.kind == 'CronJob' + ? object.spec.jobTemplate.spec.template.spec + : object.spec.template.spec + - name: sa + expression: "has(variables.podSpec.serviceAccountName) ? variables.podSpec.serviceAccountName : 'default'" + - name: saKey + expression: "'sa_' + request.namespace + '_' + variables.sa" + - name: isProtected + expression: "has(params.data) && (variables.saKey in params.data)" + - name: allowedCreators + expression: "(has(params.data) && ('workloadCreators' in params.data)) ? params.data['workloadCreators'].split(',') : []" + validations: + - expression: "!variables.isProtected || (request.userInfo.username in variables.allowedCreators)" + messageExpression: "'Workload uses protected ServiceAccount \"' + request.namespace + '/' + variables.sa + '\" which is only allowed for use by authorized ClusterLogForwarders'" + reason: Forbidden diff --git a/internal/admission/protected_sa_envtest_test.go b/internal/admission/protected_sa_envtest_test.go new file mode 100644 index 0000000000..1dbed9b8e3 --- /dev/null +++ b/internal/admission/protected_sa_envtest_test.go @@ -0,0 +1,197 @@ +package admission + +import ( + "context" + "fmt" + "os" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/openshift/cluster-logging-operator/internal/constants" + internalruntime "github.com/openshift/cluster-logging-operator/internal/runtime" + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + apiruntime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" +) + +var _ = Describe("Protected SA VAP enforcement (envtest)", Ordered, func() { + const ( + podNS = "collector-ns" + operatorNS = "operator-ns" + collectorSA = "log-collector" + unprotectedSA = "plain-sa" + restrictedUser = "system:serviceaccount:collector-ns:restricted-user" + ) + + var ( + testEnv *envtest.Environment + cfg *rest.Config + scheme *apiruntime.Scheme + adminClient client.Client + restricted client.Client + ctx context.Context + ) + + BeforeAll(func() { + if os.Getenv("KUBEBUILDER_ASSETS") == "" { + Skip("KUBEBUILDER_ASSETS not set; run via `make test-admission-envtest`") + } + ctx = context.Background() + + scheme = apiruntime.NewScheme() + Expect(corev1.AddToScheme(scheme)).To(Succeed()) + Expect(appsv1.AddToScheme(scheme)).To(Succeed()) + Expect(rbacv1.AddToScheme(scheme)).To(Succeed()) + Expect(admissionregistrationv1.AddToScheme(scheme)).To(Succeed()) + + testEnv = &envtest.Environment{Scheme: scheme, ControlPlaneStartTimeout: time.Minute} + + var err error + cfg, err = testEnv.Start() + Expect(err).ToNot(HaveOccurred()) + Expect(cfg).ToNot(BeNil()) + + adminClient, err = client.New(cfg, client.Options{Scheme: scheme}) + Expect(err).ToNot(HaveOccurred()) + restricted = clientAsUser(cfg, scheme, restrictedUser) + + for _, ns := range []string{podNS, operatorNS} { + Expect(adminClient.Create(ctx, internalruntime.NewNamespace(ns))).To(Succeed()) + } + + grantWorkloadRBAC(ctx, adminClient, + restrictedUser, + kubeSystemDaemonSetControllerUser, + operatorServiceAccountUser(operatorNS)) + + cm := internalruntime.NewConfigMap(operatorNS, ProtectedSAConfigMapName, nil) + setCreatorKeys(cm.Data, operatorNS) + cm.Data[protectedSAKeyPrefix+podNS+"_"+collectorSA] = "" + Expect(adminClient.Create(ctx, cm)).To(Succeed()) + + installPolicyAndBinding(ctx, adminClient, protectedSAPodsPolicy, protectedSAPodsBinding, operatorNS) + installPolicyAndBinding(ctx, adminClient, protectedSAWorkloadsPolicy, protectedSAWorkloadsBinding, operatorNS) + + Eventually(func(g Gomega) { + err := restricted.Create(ctx, newTestPod(podNS, uniqueName("canary"), collectorSA)) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("protected ServiceAccount")) + }, 90*time.Second, time.Second).Should(Succeed(), "protected-SA Pod policy never became active") + }) + + AfterAll(func() { + if testEnv != nil { + Expect(testEnv.Stop()).To(Succeed()) + } + }) + + It("denies a Pod referencing the protected SA created by a restricted user", func() { + err := restricted.Create(ctx, newTestPod(podNS, uniqueName("evil-pod"), collectorSA)) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("protected ServiceAccount")) + }) + + It("denies a Deployment referencing the protected SA created by a restricted user", func() { + err := restricted.Create(ctx, newTestDeployment(podNS, uniqueName("evil-deploy"), collectorSA)) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("protected ServiceAccount")) + }) + + It("allows a Pod referencing an unprotected SA", func() { + Expect(restricted.Create(ctx, newTestPod(podNS, uniqueName("plain-pod"), unprotectedSA))).To(Succeed()) + }) + + It("allows a Pod referencing the protected SA when created by an allowed controller", func() { + daemonSetController := clientAsUser(cfg, scheme, kubeSystemDaemonSetControllerUser) + Expect(daemonSetController.Create(ctx, newTestPod(podNS, uniqueName("collector-pod"), collectorSA))).To(Succeed()) + }) + + It("allows a Deployment referencing the protected SA when created by the operator", func() { + operator := clientAsUser(cfg, scheme, operatorServiceAccountUser(operatorNS)) + Expect(operator.Create(ctx, newTestDeployment(podNS, uniqueName("collector-deploy"), collectorSA))).To(Succeed()) + }) +}) + +var envtestNameCounter int + +func uniqueName(prefix string) string { + envtestNameCounter++ + return fmt.Sprintf("%s-%d", prefix, envtestNameCounter) +} + +func clientAsUser(cfg *rest.Config, scheme *apiruntime.Scheme, username string) client.Client { + impersonated := rest.CopyConfig(cfg) + impersonated.Impersonate = rest.ImpersonationConfig{UserName: username} + c, err := client.New(impersonated, client.Options{Scheme: scheme}) + Expect(err).ToNot(HaveOccurred()) + return c +} + +func grantWorkloadRBAC(ctx context.Context, c client.Client, users ...string) { + role := internalruntime.NewClusterRole("workload-creator", + internalruntime.NewPolicyRules( + internalruntime.NewPolicyRule( + []string{"", "apps"}, + []string{"pods", "deployments"}, + nil, + []string{"create", "get", "list", "delete"}, + ), + )..., + ) + Expect(c.Create(ctx, role)).To(Succeed()) + + subjects := make([]rbacv1.Subject, 0, len(users)) + for _, u := range users { + subjects = append(subjects, rbacv1.Subject{APIGroup: rbacv1.GroupName, Kind: "User", Name: u}) + } + crb := internalruntime.NewClusterRoleBinding("workload-creator-binding", + rbacv1.RoleRef{APIGroup: rbacv1.GroupName, Kind: "ClusterRole", Name: role.Name}, + subjects..., + ) + Expect(c.Create(ctx, crb)).To(Succeed()) +} + +func installPolicyAndBinding(ctx context.Context, c client.Client, policy *admissionregistrationv1.ValidatingAdmissionPolicy, binding *admissionregistrationv1.ValidatingAdmissionPolicyBinding, operatorNS string) { + Expect(c.Create(ctx, policy.DeepCopy())).To(Succeed()) + b := binding.DeepCopy() + if b.Spec.ParamRef != nil { + b.Spec.ParamRef.Namespace = operatorNS + } + Expect(c.Create(ctx, b)).To(Succeed()) +} + +func newTestPod(namespace, name, sa string) *corev1.Pod { + pod := internalruntime.NewPod(namespace, name, + *internalruntime.NewContainer("c", "registry.redhat.io/ubi9/ubi-minimal:latest", corev1.PullIfNotPresent, nil), + ) + pod.Spec.ServiceAccountName = sa + internalruntime.SetCommonLabels(pod, constants.VectorName, name, constants.CollectorName) + return pod +} + +func newTestDeployment(namespace, name, sa string) *appsv1.Deployment { + labels := map[string]string{"app": name} + replicas := int32(1) + deploy := internalruntime.NewDeployment(namespace, name) + deploy.Spec = appsv1.DeploymentSpec{ + Replicas: &replicas, + Selector: &metav1.LabelSelector{MatchLabels: labels}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: labels}, + Spec: corev1.PodSpec{ + ServiceAccountName: sa, + Containers: []corev1.Container{ + *internalruntime.NewContainer("c", "registry.redhat.io/ubi9/ubi-minimal:latest", corev1.PullIfNotPresent, nil), + }, + }, + }, + } + return deploy +} diff --git a/internal/admission/protected_sa_policy.go b/internal/admission/protected_sa_policy.go new file mode 100644 index 0000000000..09cddcfd46 --- /dev/null +++ b/internal/admission/protected_sa_policy.go @@ -0,0 +1,176 @@ +package admission + +import ( + "context" + _ "embed" + "fmt" + "os" + "sort" + "strings" + + log "github.com/ViaQ/logerr/v2/log/static" + obsv1 "github.com/openshift/cluster-logging-operator/api/observability/v1" + "github.com/openshift/cluster-logging-operator/internal/constants" + internalruntime "github.com/openshift/cluster-logging-operator/internal/runtime" + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +const ( + ProtectedSAConfigMapName = "clo-protected-serviceaccounts" + + ProtectedSAPodsPolicyName = "clo-protected-sa-pods" + ProtectedSAPodsBindingName = "clo-protected-sa-pods-binding" + ProtectedSAWorkloadsPolicyName = "clo-protected-sa-workloads" + ProtectedSAWorkloadsBindingName = "clo-protected-sa-workloads-binding" + + protectedSAKeyPrefix = "sa_" + protectedSAPodCreatorsKey = "podCreators" + protectedSAWorkloadCreatorsKey = "workloadCreators" + kubeSystemDaemonSetControllerUser = "system:serviceaccount:kube-system:daemon-set-controller" + kubeSystemReplicaSetControllerUser = "system:serviceaccount:kube-system:replicaset-controller" + kubeSystemDeploymentControllerUser = "system:serviceaccount:kube-system:deployment-controller" +) + +//go:embed protected-sa-pods.yaml +var protectedSAPodsPolicyYAML string + +//go:embed protected-sa-pods-binding.yaml +var protectedSAPodsBindingYAML string + +//go:embed protected-sa-workloads.yaml +var protectedSAWorkloadsPolicyYAML string + +//go:embed protected-sa-workloads-binding.yaml +var protectedSAWorkloadsBindingYAML string + +var ( + protectedSAPodsPolicy *admissionregistrationv1.ValidatingAdmissionPolicy + protectedSAPodsBinding *admissionregistrationv1.ValidatingAdmissionPolicyBinding + protectedSAWorkloadsPolicy *admissionregistrationv1.ValidatingAdmissionPolicy + protectedSAWorkloadsBinding *admissionregistrationv1.ValidatingAdmissionPolicyBinding +) + +func init() { + protectedSAPodsPolicy = internalruntime.Decode(protectedSAPodsPolicyYAML).(*admissionregistrationv1.ValidatingAdmissionPolicy) + protectedSAPodsBinding = internalruntime.Decode(protectedSAPodsBindingYAML).(*admissionregistrationv1.ValidatingAdmissionPolicyBinding) + protectedSAWorkloadsPolicy = internalruntime.Decode(protectedSAWorkloadsPolicyYAML).(*admissionregistrationv1.ValidatingAdmissionPolicy) + protectedSAWorkloadsBinding = internalruntime.Decode(protectedSAWorkloadsBindingYAML).(*admissionregistrationv1.ValidatingAdmissionPolicyBinding) +} + +// OperatorNamespace returns the namespace the operator runs in. +func OperatorNamespace() string { + if ns := os.Getenv("WATCH_NAMESPACE"); ns != "" { + return strings.Split(ns, ",")[0] + } + return constants.OpenshiftNS +} + +func operatorServiceAccountUser(operatorNS string) string { + return fmt.Sprintf("system:serviceaccount:%s:%s", operatorNS, constants.ClusterLoggingOperator) +} + +// ReconcileProtectedSAPolicies ensures the two ValidatingAdmissionPolicies and +// their bindings exist, and that the param ConfigMap exists with the allowed +// creator identities populated. +func ReconcileProtectedSAPolicies(ctx context.Context, k8sClient client.Client, operatorNS string) error { + if err := ensureProtectedSAConfigMap(ctx, k8sClient, operatorNS); err != nil { + return err + } + if err := SyncProtectedServiceAccounts(ctx, k8sClient, operatorNS); err != nil { + log.V(1).Info("initial protected ServiceAccount sync failed; will resync on CLF events", "error", err) + } + + for _, p := range []struct { + policy *admissionregistrationv1.ValidatingAdmissionPolicy + binding *admissionregistrationv1.ValidatingAdmissionPolicyBinding + }{ + {protectedSAPodsPolicy, protectedSAPodsBinding}, + {protectedSAWorkloadsPolicy, protectedSAWorkloadsBinding}, + } { + if err := reconcileValidatingAdmissionPolicy(ctx, k8sClient, p.policy); err != nil { + return err + } + binding := p.binding.DeepCopy() + if binding.Spec.ParamRef != nil { + binding.Spec.ParamRef.Namespace = operatorNS + } + if err := reconcileValidatingAdmissionPolicyBinding(ctx, k8sClient, binding); err != nil { + return err + } + } + return nil +} + +func ensureProtectedSAConfigMap(ctx context.Context, k8sClient client.Client, operatorNS string) error { + cm := internalruntime.NewConfigMap(operatorNS, ProtectedSAConfigMapName, nil) + internalruntime.SetCommonLabels(cm, constants.ClusterLogging, ProtectedSAConfigMapName, "admission") + _, err := controllerutil.CreateOrUpdate(ctx, k8sClient, cm, func() error { + if cm.Data == nil { + cm.Data = map[string]string{} + } + setCreatorKeys(cm.Data, operatorNS) + return nil + }) + if err != nil { + return fmt.Errorf("ensure protected SA ConfigMap %s/%s: %w", operatorNS, ProtectedSAConfigMapName, err) + } + return nil +} + +func setCreatorKeys(data map[string]string, operatorNS string) { + data[protectedSAPodCreatorsKey] = strings.Join([]string{ + kubeSystemDaemonSetControllerUser, + kubeSystemReplicaSetControllerUser, + }, ",") + data[protectedSAWorkloadCreatorsKey] = strings.Join([]string{ + operatorServiceAccountUser(operatorNS), + kubeSystemDeploymentControllerUser, + }, ",") +} + +// SyncProtectedServiceAccounts rebuilds the param ConfigMap's protected-SA +// membership from the full set of ClusterLogForwarders. +func SyncProtectedServiceAccounts(ctx context.Context, k8sClient client.Client, operatorNS string) error { + clfList := &obsv1.ClusterLogForwarderList{} + if err := k8sClient.List(ctx, clfList); err != nil { + return fmt.Errorf("list ClusterLogForwarders: %w", err) + } + + saKeys := map[string]string{} + for i := range clfList.Items { + clf := &clfList.Items[i] + sa := strings.TrimSpace(clf.Spec.ServiceAccount.Name) + if sa == "" { + continue + } + saKeys[protectedSAKeyPrefix+clf.Namespace+"_"+sa] = "" + } + + cm := internalruntime.NewConfigMap(operatorNS, ProtectedSAConfigMapName, nil) + internalruntime.SetCommonLabels(cm, constants.ClusterLogging, ProtectedSAConfigMapName, "admission") + _, err := controllerutil.CreateOrUpdate(ctx, k8sClient, cm, func() error { + data := map[string]string{} + for k, v := range saKeys { + data[k] = v + } + setCreatorKeys(data, operatorNS) + cm.Data = data + return nil + }) + if err != nil { + return fmt.Errorf("sync protected SA ConfigMap: %w", err) + } + log.V(3).Info("synced protected collector ServiceAccounts", "count", len(saKeys), "serviceAccounts", sortedKeys(saKeys)) + return nil +} + +func sortedKeys(m map[string]string) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, strings.TrimPrefix(k, protectedSAKeyPrefix)) + } + sort.Strings(keys) + return keys +} diff --git a/internal/admission/protected_sa_policy_test.go b/internal/admission/protected_sa_policy_test.go new file mode 100644 index 0000000000..ae13c04233 --- /dev/null +++ b/internal/admission/protected_sa_policy_test.go @@ -0,0 +1,99 @@ +package admission + +import ( + "context" + "strings" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + obsv1 "github.com/openshift/cluster-logging-operator/api/observability/v1" + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + apiruntime "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +var _ = Describe("Protected collector SA ValidatingAdmissionPolicies", func() { + const operatorNS = "openshift-logging" + + var ( + fakeClient client.Client + ctx = context.Background() + ) + + newClient := func(objs ...client.Object) client.Client { + scheme := apiruntime.NewScheme() + Expect(admissionregistrationv1.AddToScheme(scheme)).To(Succeed()) + Expect(corev1.AddToScheme(scheme)).To(Succeed()) + Expect(obsv1.AddToScheme(scheme)).To(Succeed()) + return fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build() + } + + clf := func(name, ns, sa string) *obsv1.ClusterLogForwarder { + return &obsv1.ClusterLogForwarder{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: obsv1.ClusterLogForwarderSpec{ServiceAccount: obsv1.ServiceAccount{Name: sa}}, + } + } + + getCM := func(c client.Client) *corev1.ConfigMap { + cm := &corev1.ConfigMap{} + Expect(c.Get(ctx, client.ObjectKey{Name: ProtectedSAConfigMapName, Namespace: operatorNS}, cm)).To(Succeed()) + return cm + } + + It("reconciles both policies and points the paramRef at the operator namespace", func() { + fakeClient = newClient() + Expect(ReconcileProtectedSAPolicies(ctx, fakeClient, operatorNS)).To(Succeed()) + + for _, name := range []string{ProtectedSAPodsPolicyName, ProtectedSAWorkloadsPolicyName} { + policy := &admissionregistrationv1.ValidatingAdmissionPolicy{} + Expect(fakeClient.Get(ctx, client.ObjectKey{Name: name}, policy)).To(Succeed()) + Expect(*policy.Spec.FailurePolicy).To(Equal(admissionregistrationv1.Fail)) + Expect(policy.Spec.ParamKind.Kind).To(Equal("ConfigMap")) + } + + for _, name := range []string{ProtectedSAPodsBindingName, ProtectedSAWorkloadsBindingName} { + binding := &admissionregistrationv1.ValidatingAdmissionPolicyBinding{} + Expect(fakeClient.Get(ctx, client.ObjectKey{Name: name}, binding)).To(Succeed()) + Expect(binding.Spec.ParamRef).ToNot(BeNil()) + Expect(binding.Spec.ParamRef.Namespace).To(Equal(operatorNS)) + Expect(binding.Spec.ParamRef.Name).To(Equal(ProtectedSAConfigMapName)) + } + }) + + It("populates the param ConfigMap with protected SAs and allowed creators", func() { + fakeClient = newClient( + clf("app-logging", "team-a", "collector-a"), + clf("infra-logging", "team-b", "collector-b"), + clf("no-sa", "team-c", ""), // ignored + ) + Expect(SyncProtectedServiceAccounts(ctx, fakeClient, operatorNS)).To(Succeed()) + + data := getCM(fakeClient).Data + Expect(data).To(HaveKey("sa_team-a_collector-a")) + Expect(data).To(HaveKey("sa_team-b_collector-b")) + Expect(data).ToNot(HaveKey("sa_team-c_")) // empty SA name skipped + + Expect(strings.Split(data[protectedSAPodCreatorsKey], ",")).To(ConsistOf( + kubeSystemDaemonSetControllerUser, + kubeSystemReplicaSetControllerUser, + )) + Expect(strings.Split(data[protectedSAWorkloadCreatorsKey], ",")).To(ConsistOf( + "system:serviceaccount:openshift-logging:cluster-logging-operator", + kubeSystemDeploymentControllerUser, + )) + }) + + It("removes an SA from the ConfigMap when its CLF is deleted (rebuild-from-list)", func() { + fakeClient = newClient(clf("app-logging", "team-a", "collector-a")) + Expect(SyncProtectedServiceAccounts(ctx, fakeClient, operatorNS)).To(Succeed()) + Expect(getCM(fakeClient).Data).To(HaveKey("sa_team-a_collector-a")) + + Expect(fakeClient.Delete(ctx, clf("app-logging", "team-a", "collector-a"))).To(Succeed()) + Expect(SyncProtectedServiceAccounts(ctx, fakeClient, operatorNS)).To(Succeed()) + Expect(getCM(fakeClient).Data).ToNot(HaveKey("sa_team-a_collector-a")) + }) +}) diff --git a/internal/admission/suite_test.go b/internal/admission/suite_test.go new file mode 100644 index 0000000000..4b18a8877f --- /dev/null +++ b/internal/admission/suite_test.go @@ -0,0 +1,36 @@ +package admission + +import ( + "fmt" + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + "k8s.io/apimachinery/pkg/api/meta" + apiruntime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/discovery" +) + +func TestAdmission(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "[internal][admission] Suite") +} + +var _ = Describe("admission policy helpers", func() { + It("recognizes unsupported admission policy API errors", func() { + Expect(IsUnsupportedAdmissionPolicyAPI(&meta.NoKindMatchError{ + GroupKind: schema.GroupKind{Group: admissionregistrationv1.GroupName, Kind: "ValidatingAdmissionPolicy"}, + })).To(BeTrue()) + Expect(IsUnsupportedAdmissionPolicyAPI(apiruntime.NewNotRegisteredErrForKind( + "test", schema.GroupVersionKind{Group: admissionregistrationv1.GroupName, Version: "v1", Kind: "ValidatingAdmissionPolicy"}, + ))).To(BeTrue()) + Expect(IsUnsupportedAdmissionPolicyAPI(&discovery.ErrGroupDiscoveryFailed{ + Groups: map[schema.GroupVersion]error{ + {Group: admissionregistrationv1.GroupName, Version: "v1"}: fmt.Errorf("discovery failed"), + }, + })).To(BeTrue()) + Expect(IsUnsupportedAdmissionPolicyAPI(fmt.Errorf("forbidden"))).To(BeFalse()) + }) +}) diff --git a/internal/controller/admission/protected_sa_controller.go b/internal/controller/admission/protected_sa_controller.go new file mode 100644 index 0000000000..38f5676331 --- /dev/null +++ b/internal/controller/admission/protected_sa_controller.go @@ -0,0 +1,35 @@ +package admission + +import ( + "context" + + log "github.com/ViaQ/logerr/v2/log/static" + obsv1 "github.com/openshift/cluster-logging-operator/api/observability/v1" + internaladmission "github.com/openshift/cluster-logging-operator/internal/admission" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// ProtectedSAReconciler keeps the protected-ServiceAccount param ConfigMap in +// sync with the set of ClusterLogForwarders. It reconciles on every CLF event +// by rebuilding the ConfigMap from the full CLF list, so deletions are handled +// without finalizers and the ConfigMap self-heals. +type ProtectedSAReconciler struct { + Client client.Client + OperatorNS string +} + +func (r *ProtectedSAReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + if err := internaladmission.SyncProtectedServiceAccounts(ctx, r.Client, r.OperatorNS); err != nil { + log.V(1).Error(err, "failed to sync protected collector ServiceAccounts", "trigger", req.NamespacedName) + return ctrl.Result{}, err + } + return ctrl.Result{}, nil +} + +func (r *ProtectedSAReconciler) SetupWithManager(mgr ctrl.Manager) error { + return ctrl.NewControllerManagedBy(mgr). + For(&obsv1.ClusterLogForwarder{}). + Named("protected-sa-configmap"). + Complete(r) +} diff --git a/internal/controller/admission/protected_sa_runnable.go b/internal/controller/admission/protected_sa_runnable.go new file mode 100644 index 0000000000..5090de0c22 --- /dev/null +++ b/internal/controller/admission/protected_sa_runnable.go @@ -0,0 +1,54 @@ +package admission + +import ( + "context" + + log "github.com/ViaQ/logerr/v2/log/static" + internaladmission "github.com/openshift/cluster-logging-operator/internal/admission" + "k8s.io/apimachinery/pkg/util/wait" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +type protectedSAAdmissionRunnable struct { + client client.Client + operatorNS string +} + +// NewProtectedSAAdmissionRunnable reconciles the protected collector +// ServiceAccount ValidatingAdmissionPolicies (and their param ConfigMap) once +// the manager cache has started. +func NewProtectedSAAdmissionRunnable(k8sClient client.Client, operatorNS string) *protectedSAAdmissionRunnable { + return &protectedSAAdmissionRunnable{client: k8sClient, operatorNS: operatorNS} +} + +func (r *protectedSAAdmissionRunnable) Start(ctx context.Context) error { + log.Info("Reconciling protected collector ServiceAccount ValidatingAdmissionPolicies") + + var lastErr error + err := wait.ExponentialBackoffWithContext(ctx, internaladmission.AdmissionReconcileBackoff, func(ctx context.Context) (bool, error) { + lastErr = internaladmission.ReconcileProtectedSAPolicies(ctx, r.client, r.operatorNS) + if lastErr == nil { + return true, nil + } + if internaladmission.IsUnsupportedAdmissionPolicyAPI(lastErr) { + lastErr = nil + return true, nil + } + log.V(1).Info("retrying protected SA ValidatingAdmissionPolicy reconciliation", "error", lastErr) + return false, nil + }) + if err != nil && !wait.Interrupted(err) { + if lastErr != nil { + log.Error(lastErr, "unable to reconcile protected SA ValidatingAdmissionPolicies", "reason", err) + } + return nil + } + if lastErr != nil { + log.Error(lastErr, "unable to reconcile protected SA ValidatingAdmissionPolicies after retries") + } + return nil +} + +func (r *protectedSAAdmissionRunnable) NeedLeaderElection() bool { + return true +} diff --git a/internal/controller/kubebuilder_rbac.go b/internal/controller/kubebuilder_rbac.go index 7cfe5d83cb..761b318ff3 100644 --- a/internal/controller/kubebuilder_rbac.go +++ b/internal/controller/kubebuilder_rbac.go @@ -3,6 +3,7 @@ package controller // This file collects all the "kubebuilder rbac annotations" that the controllers contained // in this operator need to function. +// +kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=validatingadmissionpolicies;validatingadmissionpolicybindings,verbs=create;delete;get;list;patch;update;watch // +kubebuilder:rbac:groups=apps,resources=deployments;daemonsets,verbs=get;list;watch;create;update;delete // +kubebuilder:rbac:groups=authorization.k8s.io,resources=subjectaccessreviews,verbs=create // +kubebuilder:rbac:groups=config.openshift.io,resources=proxies;infrastructures,verbs=get;list;watch diff --git a/test/e2e/collection/admission/protected_sa_test.go b/test/e2e/collection/admission/protected_sa_test.go new file mode 100644 index 0000000000..5b4e2c97dc --- /dev/null +++ b/test/e2e/collection/admission/protected_sa_test.go @@ -0,0 +1,206 @@ +package admission + +import ( + "fmt" + "os/exec" + "strings" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + internaladmission "github.com/openshift/cluster-logging-operator/internal/admission" + "github.com/openshift/cluster-logging-operator/internal/constants" + framework "github.com/openshift/cluster-logging-operator/test/framework/e2e" +) + +var _ = Describe("[collection] Protected collector ServiceAccount admission", Ordered, func() { + + const ( + clfName = "protected-sa-test" + collectorSA = "log-collector" + unprotectedSA = "plain-sa" + ) + + var ( + e2e *framework.E2ETestFramework + deployNS string + user string + ) + + BeforeAll(func() { + e2e = framework.NewE2ETestFramework() + deployNS = e2e.Test.NS.Name + user = fmt.Sprintf("system:serviceaccount:%s:restricted-user", deployNS) + + checkVAPInstalled() + + for _, sa := range []string{collectorSA, unprotectedSA, "restricted-user"} { + _, err := e2e.BuildAuthorizationFor(deployNS, sa).Create() + Expect(err).ToNot(HaveOccurred(), "create ServiceAccount %s", sa) + } + + grantWorkloadEditor(deployNS) + + clf := fmt.Sprintf(` +apiVersion: observability.openshift.io/v1 +kind: ClusterLogForwarder +metadata: + name: %s + namespace: %s +spec: + serviceAccount: + name: %s + outputs: + - name: test-output + type: lokiStack + lokiStack: + target: + name: lokistack + namespace: openshift-logging + authentication: + token: + from: serviceAccount + pipelines: + - name: test-pipe + inputRefs: + - application + outputRefs: + - test-output +`, clfName, deployNS, collectorSA) + + out, err := ocCreate(deployNS, clf) + Expect(err).ToNot(HaveOccurred(), "create CLF: %s", out) + + waitForSAProtected(deployNS, collectorSA) + }) + + AfterAll(func() { + if e2e != nil { + e2e.Cleanup() + } + }) + + It("denies a bare Pod referencing the protected SA even with copied collector metadata", func() { + out, err := ocCreateAs(deployNS, user, spoofedPodYAML(deployNS, collectorSA, "evil-pod", clfName)) + Expect(err).To(HaveOccurred(), "expected Pod creation to be denied, got: %s", out) + Expect(out).To(ContainSubstring("protected ServiceAccount")) + }) + + It("denies a Deployment referencing the protected SA", func() { + out, err := ocCreateAs(deployNS, user, deploymentYAML(deployNS, collectorSA, "evil-deploy")) + Expect(err).To(HaveOccurred(), "expected Deployment creation to be denied, got: %s", out) + Expect(out).To(ContainSubstring("protected ServiceAccount")) + }) + + It("allows a Pod referencing an unprotected SA (no collateral damage)", func() { + out, err := ocCreateAs(deployNS, user, spoofedPodYAML(deployNS, unprotectedSA, "plain-pod", clfName)) + Expect(err).ToNot(HaveOccurred(), "expected Pod with unprotected SA to be admitted, got: %s", out) + }) +}) + +func checkVAPInstalled() { + for _, resource := range []string{ + "validatingadmissionpolicy/" + internaladmission.ProtectedSAPodsPolicyName, + "validatingadmissionpolicybinding/" + internaladmission.ProtectedSAPodsBindingName, + "validatingadmissionpolicy/" + internaladmission.ProtectedSAWorkloadsPolicyName, + "validatingadmissionpolicybinding/" + internaladmission.ProtectedSAWorkloadsBindingName, + } { + out, err := exec.Command("oc", "get", resource).CombinedOutput() + Expect(err).ToNot(HaveOccurred(), "expected %s to exist (operator must reconcile VAP): %s", resource, out) + } +} + +func waitForSAProtected(namespace, sa string) { + key := fmt.Sprintf("sa_%s_%s", namespace, sa) + Eventually(func(g Gomega) { + out, err := exec.Command("oc", "get", "configmap", internaladmission.ProtectedSAConfigMapName, + "-n", constants.OpenshiftNS, "-o", "json").CombinedOutput() + g.Expect(err).ToNot(HaveOccurred(), "get param ConfigMap: %s", out) + g.Expect(string(out)).To(ContainSubstring(key), + "operator did not mark %s/%s protected", namespace, sa) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) +} + +func ocCreate(namespace, yaml string) (string, error) { + cmd := exec.Command("oc", "create", "-n", namespace, "-f", "-") + cmd.Stdin = strings.NewReader(yaml) + out, err := cmd.CombinedOutput() + return string(out), err +} + +func ocCreateAs(namespace, user, yaml string) (string, error) { + cmd := exec.Command("oc", "create", "-n", namespace, "--as", user, "-f", "-") + cmd.Stdin = strings.NewReader(yaml) + out, err := cmd.CombinedOutput() + return string(out), err +} + +func grantWorkloadEditor(namespace string) { + out, err := exec.Command("oc", "create", "role", "workload-editor", + "--verb=create,get,list,delete", + "--resource=pods,deployments.apps", + "-n", namespace).CombinedOutput() + if err != nil { + Fail(fmt.Sprintf("create workload-editor role: %s %v", out, err)) + } + + out, err = exec.Command("oc", "create", "rolebinding", "restricted-user-workload-editor", + "--role=workload-editor", + "--serviceaccount="+namespace+":restricted-user", + "-n", namespace).CombinedOutput() + if err != nil { + Fail(fmt.Sprintf("create workload-editor rolebinding: %s %v", out, err)) + } +} + +func spoofedPodYAML(namespace, sa, name, instanceName string) string { + return fmt.Sprintf(` +apiVersion: v1 +kind: Pod +metadata: + name: %s + namespace: %s + labels: + app.kubernetes.io/name: vector + app.kubernetes.io/instance: %s + app.kubernetes.io/component: collector + app.kubernetes.io/part-of: cluster-logging + app.kubernetes.io/managed-by: cluster-logging-operator + vector.dev/exclude: "true" + annotations: + target.workload.openshift.io/management: '{"effect": "PreferredDuringScheduling"}' +spec: + serviceAccountName: %s + containers: + - name: c + image: registry.redhat.io/ubi9/ubi-minimal:latest + command: ["sleep", "3600"] +`, name, namespace, instanceName, sa) +} + +func deploymentYAML(namespace, sa, name string) string { + return fmt.Sprintf(` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: %s + namespace: %s +spec: + replicas: 1 + selector: + matchLabels: + app: %s + template: + metadata: + labels: + app: %s + app.kubernetes.io/name: vector + app.kubernetes.io/component: collector + spec: + serviceAccountName: %s + containers: + - name: c + image: registry.redhat.io/ubi9/ubi-minimal:latest + command: ["sleep", "3600"] +`, name, namespace, name, name, sa) +} diff --git a/test/e2e/collection/admission/suite_test.go b/test/e2e/collection/admission/suite_test.go new file mode 100644 index 0000000000..063ea69f69 --- /dev/null +++ b/test/e2e/collection/admission/suite_test.go @@ -0,0 +1,13 @@ +package admission + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestSuite(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "[e2e][collection][admission] Suite") +} From 511667f647c25a82c5115792744d80e3b3b374a8 Mon Sep 17 00:00:00 2001 From: Vitalii Parfonov Date: Thu, 27 Aug 2026 14:52:36 +0300 Subject: [PATCH 2/5] fix(security): address review findings for protected-SA VAP - OperatorNamespace reads projected SA namespace file before WATCH_NAMESPACE - Runnable retries indefinitely until context cancellation instead of silently giving up after 5 attempts - Authorize all kube-system controller chains (statefulset, job, cronjob, replication-controller) for every workload type matched by the VAP - VAP guide messageExpression aligned with shipped YAML - collection.adoc: note OCP 4.17+ requirement Signed-off-by: Vitalii Parfonov --- .../protect-collector-serviceaccounts.md | 81 ++++++++++--------- .../design/validatingadmissionpolicy-guide.md | 2 +- docs/features/collection.adoc | 2 +- hack/test-protected-sa.sh | 2 +- internal/admission/protected_sa_policy.go | 25 +++++- .../admission/protected_sa_policy_test.go | 4 + .../admission/protected_sa_runnable.go | 34 ++++---- 7 files changed, 85 insertions(+), 65 deletions(-) diff --git a/docs/design/protect-collector-serviceaccounts.md b/docs/design/protect-collector-serviceaccounts.md index b8f88056c4..c35483bc67 100644 --- a/docs/design/protect-collector-serviceaccounts.md +++ b/docs/design/protect-collector-serviceaccounts.md @@ -229,10 +229,14 @@ the API caller, set by the API server, **not** spoofable by the requester and CEL in a VAP cannot fetch the SA object, so a per-SA label is not readable at admission. Use an **operator-maintained param object** instead: -- CLO reconciles a ConfigMap (e.g. `protected-collector-serviceaccounts` in the - operator namespace) whose `data` keys are `"/"` for every - SA referenced by any CLF (`clf.Namespace + "/" + clf.Spec.ServiceAccount.Name`). -- The VAP references this ConfigMap via `paramRef`. CEL tests membership. +- CLO reconciles a ConfigMap `clo-protected-serviceaccounts` in the operator + namespace whose `data` keys are `"sa__"` for every SA + referenced by any CLF. The `sa_` prefix + `_` separators avoid the `/` + character, which is illegal in ConfigMap data keys (`[-._a-zA-Z0-9]+`); + since namespace and SA names both forbid `_`, the encoding is collision-free. +- The VAP references this ConfigMap via `paramRef`. CEL rebuilds the same key + (`'sa_' + request.namespace + '_' + variables.sa`) and tests membership with + a `has(params.data)` guard so an empty ConfigMap never causes a CEL error. - The ConfigMap is **always present** (created empty at startup) so CEL never errors on a missing param — see §7 bootstrap. @@ -321,13 +325,15 @@ Param (reconciled by CLO; always present): apiVersion: v1 kind: ConfigMap metadata: - name: protected-collector-serviceaccounts + name: clo-protected-serviceaccounts namespace: openshift-logging # operator namespace data: - "app-logging/collector-sa": "" # one key per / + "sa_app-logging_collector-sa": "" # one key per sa__ + podCreators: "system:serviceaccount:kube-system:daemon-set-controller,system:serviceaccount:kube-system:replicaset-controller" + workloadCreators: "system:serviceaccount:openshift-logging:cluster-logging-operator,system:serviceaccount:kube-system:deployment-controller" ``` -Policy A — Pods: +Policy A — Pods (see `internal/admission/protected-sa-pods.yaml`): ```yaml apiVersion: admissionregistration.k8s.io/v1 @@ -346,34 +352,32 @@ spec: variables: - name: sa expression: "has(object.spec.serviceAccountName) ? object.spec.serviceAccountName : 'default'" - - name: key - expression: "object.metadata.namespace + '/' + variables.sa" + - name: saKey + expression: "'sa_' + request.namespace + '_' + variables.sa" - name: isProtected - expression: "variables.key in params.data" + expression: "has(params.data) && (variables.saKey in params.data)" + - name: allowedCreators + expression: "(has(params.data) && ('podCreators' in params.data)) ? params.data['podCreators'].split(',') : []" validations: - - expression: > - !variables.isProtected || - request.userInfo.username in [ - 'system:serviceaccount:kube-system:daemon-set-controller', - 'system:serviceaccount:kube-system:replicaset-controller' - ] - message: "Pod uses a protected collector ServiceAccount but was not created by a CLO-managed collector controller" + - expression: "!variables.isProtected || (request.userInfo.username in variables.allowedCreators)" + messageExpression: "'Pod uses protected ServiceAccount \"' + request.namespace + '/' + variables.sa + '\" which is only allowed for use by authorized ClusterLogForwarders'" + reason: Forbidden --- apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingAdmissionPolicyBinding metadata: - name: clo-protected-sa-pods + name: clo-protected-sa-pods-binding spec: policyName: clo-protected-sa-pods - validationActions: ["Deny"] + validationActions: [Deny] paramRef: - name: protected-collector-serviceaccounts - namespace: openshift-logging - parameterNotFoundAction: Deny + name: clo-protected-serviceaccounts + namespace: openshift-logging # overridden at reconcile time + parameterNotFoundAction: Allow # fail open on missing param to avoid operator self-lockout + matchResources: {} ``` -Policy B — workload controllers (extract pod template per kind; CronJob nests -one level deeper): +Policy B — workload controllers (see `internal/admission/protected-sa-workloads.yaml`): ```yaml apiVersion: admissionregistration.k8s.io/v1 @@ -398,27 +402,28 @@ spec: operations: ["CREATE", "UPDATE"] resources: ["replicationcontrollers"] variables: - - name: tmplSpec - expression: > - object.kind == 'CronJob' + - name: podSpec + expression: >- + request.kind.kind == 'CronJob' ? object.spec.jobTemplate.spec.template.spec : object.spec.template.spec - name: sa - expression: "has(variables.tmplSpec.serviceAccountName) ? variables.tmplSpec.serviceAccountName : 'default'" - - name: key - expression: "object.metadata.namespace + '/' + variables.sa" + expression: "has(variables.podSpec.serviceAccountName) ? variables.podSpec.serviceAccountName : 'default'" + - name: saKey + expression: "'sa_' + request.namespace + '_' + variables.sa" - name: isProtected - expression: "variables.key in params.data" + expression: "has(params.data) && (variables.saKey in params.data)" + - name: allowedCreators + expression: "(has(params.data) && ('workloadCreators' in params.data)) ? params.data['workloadCreators'].split(',') : []" validations: - - expression: > - !variables.isProtected || - request.userInfo.username == 'system:serviceaccount:openshift-logging:cluster-logging-operator' || - request.userInfo.username == 'system:serviceaccount:kube-system:deployment-controller' - message: "Workload uses a protected collector ServiceAccount but was not created by the Cluster Logging Operator" + - expression: "!variables.isProtected || (request.userInfo.username in variables.allowedCreators)" + messageExpression: "'Workload uses protected ServiceAccount \"' + request.namespace + '/' + variables.sa + '\" which is only allowed for use by authorized ClusterLogForwarders'" + reason: Forbidden ``` -(Bind Policy B the same way. Template `openshift-logging` from the operator's own -namespace at reconcile time.) +(Bind Policy B the same way — see `internal/admission/protected-sa-workloads-binding.yaml`. +The operator templates its own namespace into the binding's `paramRef.namespace` +at reconcile time.) Representative test cases: diff --git a/docs/design/validatingadmissionpolicy-guide.md b/docs/design/validatingadmissionpolicy-guide.md index c7ef6c45d0..1e2927c472 100644 --- a/docs/design/validatingadmissionpolicy-guide.md +++ b/docs/design/validatingadmissionpolicy-guide.md @@ -114,7 +114,7 @@ spec: expression: "(has(params.data) && ('podCreators' in params.data)) ? params.data['podCreators'].split(',') : []" validations: # (E) - expression: "!variables.isProtected || (request.userInfo.username in variables.allowedCreators)" - messageExpression: "'Pod uses protected collector ServiceAccount \"' + variables.sa + '\" and may only be created by a CLO-managed collector controller'" + messageExpression: "'Pod uses protected ServiceAccount \"' + request.namespace + '/' + variables.sa + '\" which is only allowed for use by authorized ClusterLogForwarders'" reason: Forbidden ``` diff --git a/docs/features/collection.adoc b/docs/features/collection.adoc index 987b229057..0fc60f9b5d 100644 --- a/docs/features/collection.adoc +++ b/docs/features/collection.adoc @@ -97,7 +97,7 @@ a| |https://issues.redhat.com/browse/LOG-3270[TLS Security Profile Compliance] |Comply with OCP cluster-wide cryptographic profiles for internal communication and allow configuration of outbound connection profiles. See link:./tls_security_profile.adoc[details] |https://issues.redhat.com/browse/LOG-7571[Network Policy]| Network policy in place for the collectors that allows all egress and ingress. -|link:../../docs/design/protect-collector-serviceaccounts.md[Protected collector ServiceAccounts]|ValidatingAdmissionPolicy prevents a collector ServiceAccount from being reused by an arbitrary Pod or workload (CVE-2026-10609) +|link:../../docs/design/protect-collector-serviceaccounts.md[Protected collector ServiceAccounts]|ValidatingAdmissionPolicy prevents a collector ServiceAccount from being reused by an arbitrary Pod or workload; requires OCP 4.17+ (CVE-2026-10609) |====== === Tuning diff --git a/hack/test-protected-sa.sh b/hack/test-protected-sa.sh index 88b9dc2b43..933fbdf1ee 100755 --- a/hack/test-protected-sa.sh +++ b/hack/test-protected-sa.sh @@ -34,7 +34,7 @@ CONFIGMAP="${CONFIGMAP:-clo-protected-serviceaccounts}" CLEANUP_ONLY=false NO_CLEANUP=false -usage() { sed -n '2,20p' "$0" | sed 's/^# \?//'; exit "${1:-0}"; } +usage() { sed -n '2,22p' "$0" | sed 's/^# \?//'; exit "${1:-0}"; } while [[ $# -gt 0 ]]; do case "$1" in diff --git a/internal/admission/protected_sa_policy.go b/internal/admission/protected_sa_policy.go index 09cddcfd46..9658c098ab 100644 --- a/internal/admission/protected_sa_policy.go +++ b/internal/admission/protected_sa_policy.go @@ -28,9 +28,13 @@ const ( protectedSAKeyPrefix = "sa_" protectedSAPodCreatorsKey = "podCreators" protectedSAWorkloadCreatorsKey = "workloadCreators" - kubeSystemDaemonSetControllerUser = "system:serviceaccount:kube-system:daemon-set-controller" - kubeSystemReplicaSetControllerUser = "system:serviceaccount:kube-system:replicaset-controller" - kubeSystemDeploymentControllerUser = "system:serviceaccount:kube-system:deployment-controller" + kubeSystemDaemonSetControllerUser = "system:serviceaccount:kube-system:daemon-set-controller" + kubeSystemReplicaSetControllerUser = "system:serviceaccount:kube-system:replicaset-controller" + kubeSystemStatefulSetControllerUser = "system:serviceaccount:kube-system:statefulset-controller" + kubeSystemDeploymentControllerUser = "system:serviceaccount:kube-system:deployment-controller" + kubeSystemJobControllerUser = "system:serviceaccount:kube-system:job-controller" + kubeSystemCronJobControllerUser = "system:serviceaccount:kube-system:cronjob-controller" + kubeSystemReplicationControllerUser = "system:serviceaccount:kube-system:replication-controller" ) //go:embed protected-sa-pods.yaml @@ -59,8 +63,17 @@ func init() { protectedSAWorkloadsBinding = internalruntime.Decode(protectedSAWorkloadsBindingYAML).(*admissionregistrationv1.ValidatingAdmissionPolicyBinding) } -// OperatorNamespace returns the namespace the operator runs in. +// OperatorNamespace returns the namespace the operator pod runs in. +// It reads the projected ServiceAccount namespace (set by the kubelet, always +// present in-cluster) so the result is independent of WATCH_NAMESPACE / +// olm.targetNamespaces, which may list namespaces the operator watches rather +// than the one it is deployed in. func OperatorNamespace() string { + if data, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err == nil { + if ns := strings.TrimSpace(string(data)); ns != "" { + return ns + } + } if ns := os.Getenv("WATCH_NAMESPACE"); ns != "" { return strings.Split(ns, ",")[0] } @@ -123,10 +136,14 @@ func setCreatorKeys(data map[string]string, operatorNS string) { data[protectedSAPodCreatorsKey] = strings.Join([]string{ kubeSystemDaemonSetControllerUser, kubeSystemReplicaSetControllerUser, + kubeSystemStatefulSetControllerUser, + kubeSystemJobControllerUser, + kubeSystemReplicationControllerUser, }, ",") data[protectedSAWorkloadCreatorsKey] = strings.Join([]string{ operatorServiceAccountUser(operatorNS), kubeSystemDeploymentControllerUser, + kubeSystemCronJobControllerUser, }, ",") } diff --git a/internal/admission/protected_sa_policy_test.go b/internal/admission/protected_sa_policy_test.go index ae13c04233..d3d93f359d 100644 --- a/internal/admission/protected_sa_policy_test.go +++ b/internal/admission/protected_sa_policy_test.go @@ -80,10 +80,14 @@ var _ = Describe("Protected collector SA ValidatingAdmissionPolicies", func() { Expect(strings.Split(data[protectedSAPodCreatorsKey], ",")).To(ConsistOf( kubeSystemDaemonSetControllerUser, kubeSystemReplicaSetControllerUser, + kubeSystemStatefulSetControllerUser, + kubeSystemJobControllerUser, + kubeSystemReplicationControllerUser, )) Expect(strings.Split(data[protectedSAWorkloadCreatorsKey], ",")).To(ConsistOf( "system:serviceaccount:openshift-logging:cluster-logging-operator", kubeSystemDeploymentControllerUser, + kubeSystemCronJobControllerUser, )) }) diff --git a/internal/controller/admission/protected_sa_runnable.go b/internal/controller/admission/protected_sa_runnable.go index 5090de0c22..ce59b31f68 100644 --- a/internal/controller/admission/protected_sa_runnable.go +++ b/internal/controller/admission/protected_sa_runnable.go @@ -2,10 +2,10 @@ package admission import ( "context" + "time" log "github.com/ViaQ/logerr/v2/log/static" internaladmission "github.com/openshift/cluster-logging-operator/internal/admission" - "k8s.io/apimachinery/pkg/util/wait" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -24,29 +24,23 @@ func NewProtectedSAAdmissionRunnable(k8sClient client.Client, operatorNS string) func (r *protectedSAAdmissionRunnable) Start(ctx context.Context) error { log.Info("Reconciling protected collector ServiceAccount ValidatingAdmissionPolicies") - var lastErr error - err := wait.ExponentialBackoffWithContext(ctx, internaladmission.AdmissionReconcileBackoff, func(ctx context.Context) (bool, error) { - lastErr = internaladmission.ReconcileProtectedSAPolicies(ctx, r.client, r.operatorNS) - if lastErr == nil { - return true, nil + backoff := internaladmission.AdmissionReconcileBackoff + for { + err := internaladmission.ReconcileProtectedSAPolicies(ctx, r.client, r.operatorNS) + if err == nil { + return nil } - if internaladmission.IsUnsupportedAdmissionPolicyAPI(lastErr) { - lastErr = nil - return true, nil + if internaladmission.IsUnsupportedAdmissionPolicyAPI(err) { + return nil } - log.V(1).Info("retrying protected SA ValidatingAdmissionPolicy reconciliation", "error", lastErr) - return false, nil - }) - if err != nil && !wait.Interrupted(err) { - if lastErr != nil { - log.Error(lastErr, "unable to reconcile protected SA ValidatingAdmissionPolicies", "reason", err) + delay := backoff.Step() + log.V(1).Info("retrying protected SA ValidatingAdmissionPolicy reconciliation", "error", err, "retryAfter", delay) + select { + case <-ctx.Done(): + return nil + case <-time.After(delay): } - return nil } - if lastErr != nil { - log.Error(lastErr, "unable to reconcile protected SA ValidatingAdmissionPolicies after retries") - } - return nil } func (r *protectedSAAdmissionRunnable) NeedLeaderElection() bool { From 275a5317b773824f6364478395f07996ae9f1528 Mon Sep 17 00:00:00 2001 From: Vitalii Parfonov Date: Fri, 11 Sep 2026 12:40:40 +0300 Subject: [PATCH 3/5] fix(security): address PR review feedback for protected-SA VAP Move VAP reconcile helpers to internal/reconcile/admission.go. Move controller files from internal/controller/admission/ up to internal/controller/. Extract CollectorServiceAccounts() to internal/runtime/observability/. Add ConfigMap self-healing via Watches predicate and tests for recreate-after-delete and revert-unauthorized-edit. Replace local test helpers with framework equivalents (test.UniqueName, NewClusterRoleRef, DeploymentBuilder). Signed-off-by: Vitalii Parfonov --- cmd/main.go | 6 +- .../protect-collector-serviceaccounts.md | 6 +- .../design/validatingadmissionpolicy-guide.md | 2 +- internal/admission/policy.go | 71 ----------- .../admission/protected_sa_envtest_test.go | 49 +++----- internal/admission/protected_sa_policy.go | 119 ++++++++---------- .../admission/protected_sa_policy_test.go | 46 +++++-- internal/admission/suite_test.go | 9 +- .../protected_sa_controller.go | 19 ++- .../{admission => }/protected_sa_runnable.go | 5 +- internal/reconcile/admission.go | 72 +++++++++++ .../observability/clusterlogforwarder.go | 34 +++++ 12 files changed, 244 insertions(+), 194 deletions(-) rename internal/controller/{admission => }/protected_sa_controller.go (63%) rename internal/controller/{admission => }/protected_sa_runnable.go (89%) create mode 100644 internal/reconcile/admission.go diff --git a/cmd/main.go b/cmd/main.go index 9b8c7112eb..21d23f5d83 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -13,7 +13,7 @@ import ( internaladmission "github.com/openshift/cluster-logging-operator/internal/admission" internalcontext "github.com/openshift/cluster-logging-operator/internal/api/context" "github.com/openshift/cluster-logging-operator/internal/collector" - admissioncontroller "github.com/openshift/cluster-logging-operator/internal/controller/admission" + internalcontroller "github.com/openshift/cluster-logging-operator/internal/controller" internaltls "github.com/openshift/cluster-logging-operator/internal/tls" "sigs.k8s.io/controller-runtime/pkg/metrics/filters" @@ -261,7 +261,7 @@ func main() { } operatorNS := internaladmission.OperatorNamespace() - if err = (&admissioncontroller.ProtectedSAReconciler{ + if err = (&internalcontroller.ProtectedSAReconciler{ Client: mgr.GetClient(), OperatorNS: operatorNS, }).SetupWithManager(mgr); err != nil { @@ -271,7 +271,7 @@ func main() { //+kubebuilder:scaffold:builder - if err := mgr.Add(admissioncontroller.NewProtectedSAAdmissionRunnable(k8sClient, operatorNS)); err != nil { + if err := mgr.Add(internalcontroller.NewProtectedSAAdmissionRunnable(k8sClient, operatorNS)); err != nil { log.Error(err, "unable to register protected SA admission runnable") os.Exit(1) } diff --git a/docs/design/protect-collector-serviceaccounts.md b/docs/design/protect-collector-serviceaccounts.md index c35483bc67..3800224c8d 100644 --- a/docs/design/protect-collector-serviceaccounts.md +++ b/docs/design/protect-collector-serviceaccounts.md @@ -10,7 +10,7 @@ Scope: Cluster Logging Operator (observability.openshift.io/v1 `ClusterLogForwar Key files: `internal/admission/protected_sa_policy.go`, `internal/admission/protected-sa-{pods,workloads}{,-binding}.yaml`, -`internal/controller/admission/protected_sa_controller.go` / `_runnable.go`; +`internal/controller/protected_sa_controller.go` / `_runnable.go`; manual test `hack/test-protected-sa.sh`. Validated end-to-end against a live OpenShift API server via `hack/test-protected-sa.sh`: a restricted user is denied creating a Pod and a Deployment as a protected collector SA even when @@ -329,8 +329,8 @@ metadata: namespace: openshift-logging # operator namespace data: "sa_app-logging_collector-sa": "" # one key per sa__ - podCreators: "system:serviceaccount:kube-system:daemon-set-controller,system:serviceaccount:kube-system:replicaset-controller" - workloadCreators: "system:serviceaccount:openshift-logging:cluster-logging-operator,system:serviceaccount:kube-system:deployment-controller" + podCreators: "system:serviceaccount:kube-system:daemon-set-controller,system:serviceaccount:kube-system:replicaset-controller,system:serviceaccount:kube-system:statefulset-controller,system:serviceaccount:kube-system:job-controller,system:serviceaccount:kube-system:replication-controller" + workloadCreators: "system:serviceaccount:openshift-logging:cluster-logging-operator,system:serviceaccount:kube-system:deployment-controller,system:serviceaccount:kube-system:cronjob-controller" ``` Policy A — Pods (see `internal/admission/protected-sa-pods.yaml`): diff --git a/docs/design/validatingadmissionpolicy-guide.md b/docs/design/validatingadmissionpolicy-guide.md index 1e2927c472..fe079918f3 100644 --- a/docs/design/validatingadmissionpolicy-guide.md +++ b/docs/design/validatingadmissionpolicy-guide.md @@ -19,7 +19,7 @@ Source of truth (read alongside this doc): - `internal/admission/protected-sa-pods.yaml` + `-binding.yaml` - `internal/admission/protected-sa-workloads.yaml` + `-binding.yaml` - `internal/admission/protected_sa_policy.go` (reconcile + param ConfigMap) -- `internal/controller/admission/protected_sa_controller.go` / `_runnable.go` (when it runs) +- `internal/controller/protected_sa_controller.go` / `_runnable.go` (when it runs) --- diff --git a/internal/admission/policy.go b/internal/admission/policy.go index 5c95e374bc..1ba7062f0b 100644 --- a/internal/admission/policy.go +++ b/internal/admission/policy.go @@ -1,20 +1,9 @@ package admission import ( - "context" - "errors" - "fmt" "time" - log "github.com/ViaQ/logerr/v2/log/static" - admissionregistrationv1 "k8s.io/api/admissionregistration/v1" - "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - apiruntime "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/wait" - "k8s.io/client-go/discovery" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) var AdmissionReconcileBackoff = wait.Backoff{ @@ -23,63 +12,3 @@ var AdmissionReconcileBackoff = wait.Backoff{ Factor: 2.0, Cap: 30 * time.Second, } - -func reconcileValidatingAdmissionPolicy(ctx context.Context, k8sClient client.Client, desired *admissionregistrationv1.ValidatingAdmissionPolicy) error { - current := &admissionregistrationv1.ValidatingAdmissionPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: desired.Name, - }, - } - - op, err := controllerutil.CreateOrUpdate(ctx, k8sClient, current, func() error { - current.Spec = desired.Spec - return nil - }) - if err != nil { - if IsUnsupportedAdmissionPolicyAPI(err) { - log.Info("ValidatingAdmissionPolicy API is unavailable; skipping admission policy", "name", desired.Name) - return nil - } - return fmt.Errorf("reconcile ValidatingAdmissionPolicy %q: %w", desired.Name, err) - } - - log.V(3).Info("reconciled ValidatingAdmissionPolicy", "name", desired.Name, "operation", op) - return nil -} - -func reconcileValidatingAdmissionPolicyBinding(ctx context.Context, k8sClient client.Client, desired *admissionregistrationv1.ValidatingAdmissionPolicyBinding) error { - current := &admissionregistrationv1.ValidatingAdmissionPolicyBinding{ - ObjectMeta: metav1.ObjectMeta{ - Name: desired.Name, - }, - } - - op, err := controllerutil.CreateOrUpdate(ctx, k8sClient, current, func() error { - current.Spec = desired.Spec - return nil - }) - if err != nil { - if IsUnsupportedAdmissionPolicyAPI(err) { - log.Info("ValidatingAdmissionPolicyBinding API is unavailable; skipping admission policy binding", "name", desired.Name) - return nil - } - return fmt.Errorf("reconcile ValidatingAdmissionPolicyBinding %q: %w", desired.Name, err) - } - - log.V(3).Info("reconciled ValidatingAdmissionPolicyBinding", "name", desired.Name, "operation", op) - return nil -} - -func IsUnsupportedAdmissionPolicyAPI(err error) bool { - if err == nil { - return false - } - if meta.IsNoMatchError(err) { - return true - } - if apiruntime.IsNotRegisteredError(err) { - return true - } - var groupDiscoveryErr *discovery.ErrGroupDiscoveryFailed - return errors.As(err, &groupDiscoveryErr) -} diff --git a/internal/admission/protected_sa_envtest_test.go b/internal/admission/protected_sa_envtest_test.go index 1dbed9b8e3..9d80c85186 100644 --- a/internal/admission/protected_sa_envtest_test.go +++ b/internal/admission/protected_sa_envtest_test.go @@ -2,7 +2,6 @@ package admission import ( "context" - "fmt" "os" "time" @@ -10,11 +9,11 @@ import ( . "github.com/onsi/gomega" "github.com/openshift/cluster-logging-operator/internal/constants" internalruntime "github.com/openshift/cluster-logging-operator/internal/runtime" + "github.com/openshift/cluster-logging-operator/test" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" apiruntime "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" @@ -68,7 +67,7 @@ var _ = Describe("Protected SA VAP enforcement (envtest)", Ordered, func() { grantWorkloadRBAC(ctx, adminClient, restrictedUser, - kubeSystemDaemonSetControllerUser, + podControllers[0], operatorServiceAccountUser(operatorNS)) cm := internalruntime.NewConfigMap(operatorNS, ProtectedSAConfigMapName, nil) @@ -80,7 +79,7 @@ var _ = Describe("Protected SA VAP enforcement (envtest)", Ordered, func() { installPolicyAndBinding(ctx, adminClient, protectedSAWorkloadsPolicy, protectedSAWorkloadsBinding, operatorNS) Eventually(func(g Gomega) { - err := restricted.Create(ctx, newTestPod(podNS, uniqueName("canary"), collectorSA)) + err := restricted.Create(ctx, newTestPod(podNS, test.UniqueName("canary"), collectorSA)) g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring("protected ServiceAccount")) }, 90*time.Second, time.Second).Should(Succeed(), "protected-SA Pod policy never became active") @@ -93,39 +92,32 @@ var _ = Describe("Protected SA VAP enforcement (envtest)", Ordered, func() { }) It("denies a Pod referencing the protected SA created by a restricted user", func() { - err := restricted.Create(ctx, newTestPod(podNS, uniqueName("evil-pod"), collectorSA)) + err := restricted.Create(ctx, newTestPod(podNS, test.UniqueName("evil-pod"), collectorSA)) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("protected ServiceAccount")) }) It("denies a Deployment referencing the protected SA created by a restricted user", func() { - err := restricted.Create(ctx, newTestDeployment(podNS, uniqueName("evil-deploy"), collectorSA)) + err := restricted.Create(ctx, newTestDeployment(podNS, test.UniqueName("evil-deploy"), collectorSA)) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("protected ServiceAccount")) }) It("allows a Pod referencing an unprotected SA", func() { - Expect(restricted.Create(ctx, newTestPod(podNS, uniqueName("plain-pod"), unprotectedSA))).To(Succeed()) + Expect(restricted.Create(ctx, newTestPod(podNS, test.UniqueName("plain-pod"), unprotectedSA))).To(Succeed()) }) It("allows a Pod referencing the protected SA when created by an allowed controller", func() { - daemonSetController := clientAsUser(cfg, scheme, kubeSystemDaemonSetControllerUser) - Expect(daemonSetController.Create(ctx, newTestPod(podNS, uniqueName("collector-pod"), collectorSA))).To(Succeed()) + daemonSetController := clientAsUser(cfg, scheme, podControllers[0]) + Expect(daemonSetController.Create(ctx, newTestPod(podNS, test.UniqueName("collector-pod"), collectorSA))).To(Succeed()) }) It("allows a Deployment referencing the protected SA when created by the operator", func() { operator := clientAsUser(cfg, scheme, operatorServiceAccountUser(operatorNS)) - Expect(operator.Create(ctx, newTestDeployment(podNS, uniqueName("collector-deploy"), collectorSA))).To(Succeed()) + Expect(operator.Create(ctx, newTestDeployment(podNS, test.UniqueName("collector-deploy"), collectorSA))).To(Succeed()) }) }) -var envtestNameCounter int - -func uniqueName(prefix string) string { - envtestNameCounter++ - return fmt.Sprintf("%s-%d", prefix, envtestNameCounter) -} - func clientAsUser(cfg *rest.Config, scheme *apiruntime.Scheme, username string) client.Client { impersonated := rest.CopyConfig(cfg) impersonated.Impersonate = rest.ImpersonationConfig{UserName: username} @@ -152,7 +144,7 @@ func grantWorkloadRBAC(ctx context.Context, c client.Client, users ...string) { subjects = append(subjects, rbacv1.Subject{APIGroup: rbacv1.GroupName, Kind: "User", Name: u}) } crb := internalruntime.NewClusterRoleBinding("workload-creator-binding", - rbacv1.RoleRef{APIGroup: rbacv1.GroupName, Kind: "ClusterRole", Name: role.Name}, + internalruntime.NewClusterRoleRef(role.Name), subjects..., ) Expect(c.Create(ctx, crb)).To(Succeed()) @@ -180,18 +172,15 @@ func newTestDeployment(namespace, name, sa string) *appsv1.Deployment { labels := map[string]string{"app": name} replicas := int32(1) deploy := internalruntime.NewDeployment(namespace, name) - deploy.Spec = appsv1.DeploymentSpec{ - Replicas: &replicas, - Selector: &metav1.LabelSelector{MatchLabels: labels}, - Template: corev1.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{Labels: labels}, - Spec: corev1.PodSpec{ - ServiceAccountName: sa, - Containers: []corev1.Container{ - *internalruntime.NewContainer("c", "registry.redhat.io/ubi9/ubi-minimal:latest", corev1.PullIfNotPresent, nil), - }, + internalruntime.NewDeploymentBuilder(deploy). + WithReplicas(&replicas). + WithSelector(labels). + WithTemplateLabels(labels). + WithPodSpec(corev1.PodSpec{ + ServiceAccountName: sa, + Containers: []corev1.Container{ + *internalruntime.NewContainer("c", "registry.redhat.io/ubi9/ubi-minimal:latest", corev1.PullIfNotPresent, nil), }, - }, - } + }) return deploy } diff --git a/internal/admission/protected_sa_policy.go b/internal/admission/protected_sa_policy.go index 9658c098ab..4d3eb3f400 100644 --- a/internal/admission/protected_sa_policy.go +++ b/internal/admission/protected_sa_policy.go @@ -5,16 +5,16 @@ import ( _ "embed" "fmt" "os" - "sort" "strings" log "github.com/ViaQ/logerr/v2/log/static" - obsv1 "github.com/openshift/cluster-logging-operator/api/observability/v1" "github.com/openshift/cluster-logging-operator/internal/constants" + internalreconcile "github.com/openshift/cluster-logging-operator/internal/reconcile" internalruntime "github.com/openshift/cluster-logging-operator/internal/runtime" + runtimeobs "github.com/openshift/cluster-logging-operator/internal/runtime/observability" + "github.com/openshift/cluster-logging-operator/internal/utils/comparators" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) const ( @@ -25,18 +25,28 @@ const ( ProtectedSAWorkloadsPolicyName = "clo-protected-sa-workloads" ProtectedSAWorkloadsBindingName = "clo-protected-sa-workloads-binding" - protectedSAKeyPrefix = "sa_" - protectedSAPodCreatorsKey = "podCreators" - protectedSAWorkloadCreatorsKey = "workloadCreators" - kubeSystemDaemonSetControllerUser = "system:serviceaccount:kube-system:daemon-set-controller" - kubeSystemReplicaSetControllerUser = "system:serviceaccount:kube-system:replicaset-controller" - kubeSystemStatefulSetControllerUser = "system:serviceaccount:kube-system:statefulset-controller" - kubeSystemDeploymentControllerUser = "system:serviceaccount:kube-system:deployment-controller" - kubeSystemJobControllerUser = "system:serviceaccount:kube-system:job-controller" - kubeSystemCronJobControllerUser = "system:serviceaccount:kube-system:cronjob-controller" - kubeSystemReplicationControllerUser = "system:serviceaccount:kube-system:replication-controller" + protectedSAKeyPrefix = "sa_" + protectedSAPodCreatorsKey = "podCreators" + protectedSAWorkloadCreatorsKey = "workloadCreators" ) +// podControllers lists kube-system controllers that create Pods from +// higher-level workload resources matched by the protected-SA VAPs. +var podControllers = []string{ + "system:serviceaccount:kube-system:daemon-set-controller", + "system:serviceaccount:kube-system:replicaset-controller", + "system:serviceaccount:kube-system:statefulset-controller", + "system:serviceaccount:kube-system:job-controller", + "system:serviceaccount:kube-system:replication-controller", +} + +// workloadControllers lists kube-system controllers that create intermediate +// workload resources (e.g. Deployment → ReplicaSet, CronJob → Job). +var workloadControllers = []string{ + "system:serviceaccount:kube-system:deployment-controller", + "system:serviceaccount:kube-system:cronjob-controller", +} + //go:embed protected-sa-pods.yaml var protectedSAPodsPolicyYAML string @@ -88,7 +98,7 @@ func operatorServiceAccountUser(operatorNS string) string { // their bindings exist, and that the param ConfigMap exists with the allowed // creator identities populated. func ReconcileProtectedSAPolicies(ctx context.Context, k8sClient client.Client, operatorNS string) error { - if err := ensureProtectedSAConfigMap(ctx, k8sClient, operatorNS); err != nil { + if err := ensureProtectedSAConfigMap(k8sClient, operatorNS); err != nil { return err } if err := SyncProtectedServiceAccounts(ctx, k8sClient, operatorNS); err != nil { @@ -102,92 +112,65 @@ func ReconcileProtectedSAPolicies(ctx context.Context, k8sClient client.Client, {protectedSAPodsPolicy, protectedSAPodsBinding}, {protectedSAWorkloadsPolicy, protectedSAWorkloadsBinding}, } { - if err := reconcileValidatingAdmissionPolicy(ctx, k8sClient, p.policy); err != nil { + if err := internalreconcile.ValidatingAdmissionPolicy(ctx, k8sClient, p.policy); err != nil { + if internalreconcile.IsUnsupportedAdmissionPolicyAPI(err) { + log.Info("ValidatingAdmissionPolicy API is unavailable; skipping", "name", p.policy.Name) + return nil + } return err } binding := p.binding.DeepCopy() if binding.Spec.ParamRef != nil { binding.Spec.ParamRef.Namespace = operatorNS } - if err := reconcileValidatingAdmissionPolicyBinding(ctx, k8sClient, binding); err != nil { + if err := internalreconcile.ValidatingAdmissionPolicyBinding(ctx, k8sClient, binding); err != nil { + if internalreconcile.IsUnsupportedAdmissionPolicyAPI(err) { + log.Info("ValidatingAdmissionPolicyBinding API is unavailable; skipping", "name", binding.Name) + return nil + } return err } } return nil } -func ensureProtectedSAConfigMap(ctx context.Context, k8sClient client.Client, operatorNS string) error { +func ensureProtectedSAConfigMap(k8sClient client.Client, operatorNS string) error { cm := internalruntime.NewConfigMap(operatorNS, ProtectedSAConfigMapName, nil) internalruntime.SetCommonLabels(cm, constants.ClusterLogging, ProtectedSAConfigMapName, "admission") - _, err := controllerutil.CreateOrUpdate(ctx, k8sClient, cm, func() error { - if cm.Data == nil { - cm.Data = map[string]string{} - } - setCreatorKeys(cm.Data, operatorNS) - return nil - }) - if err != nil { + cm.Data = map[string]string{} + setCreatorKeys(cm.Data, operatorNS) + if err := internalreconcile.Configmap(k8sClient, k8sClient, cm, comparators.CompareLabels); err != nil { return fmt.Errorf("ensure protected SA ConfigMap %s/%s: %w", operatorNS, ProtectedSAConfigMapName, err) } return nil } func setCreatorKeys(data map[string]string, operatorNS string) { - data[protectedSAPodCreatorsKey] = strings.Join([]string{ - kubeSystemDaemonSetControllerUser, - kubeSystemReplicaSetControllerUser, - kubeSystemStatefulSetControllerUser, - kubeSystemJobControllerUser, - kubeSystemReplicationControllerUser, - }, ",") - data[protectedSAWorkloadCreatorsKey] = strings.Join([]string{ - operatorServiceAccountUser(operatorNS), - kubeSystemDeploymentControllerUser, - kubeSystemCronJobControllerUser, - }, ",") + data[protectedSAPodCreatorsKey] = strings.Join(podControllers, ",") + data[protectedSAWorkloadCreatorsKey] = strings.Join( + append([]string{operatorServiceAccountUser(operatorNS)}, workloadControllers...), ",") } // SyncProtectedServiceAccounts rebuilds the param ConfigMap's protected-SA // membership from the full set of ClusterLogForwarders. func SyncProtectedServiceAccounts(ctx context.Context, k8sClient client.Client, operatorNS string) error { - clfList := &obsv1.ClusterLogForwarderList{} - if err := k8sClient.List(ctx, clfList); err != nil { - return fmt.Errorf("list ClusterLogForwarders: %w", err) + refs, err := runtimeobs.CollectorServiceAccounts(ctx, k8sClient) + if err != nil { + return err } - saKeys := map[string]string{} - for i := range clfList.Items { - clf := &clfList.Items[i] - sa := strings.TrimSpace(clf.Spec.ServiceAccount.Name) - if sa == "" { - continue - } - saKeys[protectedSAKeyPrefix+clf.Namespace+"_"+sa] = "" + data := map[string]string{} + for _, ref := range refs { + data[protectedSAKeyPrefix+ref.Namespace+"_"+ref.Name] = "" } + setCreatorKeys(data, operatorNS) cm := internalruntime.NewConfigMap(operatorNS, ProtectedSAConfigMapName, nil) internalruntime.SetCommonLabels(cm, constants.ClusterLogging, ProtectedSAConfigMapName, "admission") - _, err := controllerutil.CreateOrUpdate(ctx, k8sClient, cm, func() error { - data := map[string]string{} - for k, v := range saKeys { - data[k] = v - } - setCreatorKeys(data, operatorNS) - cm.Data = data - return nil - }) - if err != nil { + cm.Data = data + if err := internalreconcile.Configmap(k8sClient, k8sClient, cm, comparators.CompareLabels); err != nil { return fmt.Errorf("sync protected SA ConfigMap: %w", err) } - log.V(3).Info("synced protected collector ServiceAccounts", "count", len(saKeys), "serviceAccounts", sortedKeys(saKeys)) + log.V(3).Info("synced protected collector ServiceAccounts", "count", len(refs), "serviceAccounts", refs) return nil } - -func sortedKeys(m map[string]string) []string { - keys := make([]string, 0, len(m)) - for k := range m { - keys = append(keys, strings.TrimPrefix(k, protectedSAKeyPrefix)) - } - sort.Strings(keys) - return keys -} diff --git a/internal/admission/protected_sa_policy_test.go b/internal/admission/protected_sa_policy_test.go index d3d93f359d..1a8ed88d17 100644 --- a/internal/admission/protected_sa_policy_test.go +++ b/internal/admission/protected_sa_policy_test.go @@ -77,20 +77,46 @@ var _ = Describe("Protected collector SA ValidatingAdmissionPolicies", func() { Expect(data).To(HaveKey("sa_team-b_collector-b")) Expect(data).ToNot(HaveKey("sa_team-c_")) // empty SA name skipped - Expect(strings.Split(data[protectedSAPodCreatorsKey], ",")).To(ConsistOf( - kubeSystemDaemonSetControllerUser, - kubeSystemReplicaSetControllerUser, - kubeSystemStatefulSetControllerUser, - kubeSystemJobControllerUser, - kubeSystemReplicationControllerUser, - )) + Expect(strings.Split(data[protectedSAPodCreatorsKey], ",")).To(ConsistOf(podControllers)) Expect(strings.Split(data[protectedSAWorkloadCreatorsKey], ",")).To(ConsistOf( - "system:serviceaccount:openshift-logging:cluster-logging-operator", - kubeSystemDeploymentControllerUser, - kubeSystemCronJobControllerUser, + append([]string{"system:serviceaccount:openshift-logging:cluster-logging-operator"}, workloadControllers...), )) }) + It("recreates the ConfigMap after external deletion", func() { + fakeClient = newClient(clf("app-logging", "team-a", "collector-a")) + Expect(SyncProtectedServiceAccounts(ctx, fakeClient, operatorNS)).To(Succeed()) + Expect(getCM(fakeClient).Data).To(HaveKey("sa_team-a_collector-a")) + + Expect(fakeClient.Delete(ctx, getCM(fakeClient))).To(Succeed()) + + cm := &corev1.ConfigMap{} + Expect(fakeClient.Get(ctx, client.ObjectKey{Name: ProtectedSAConfigMapName, Namespace: operatorNS}, cm)).ToNot(Succeed()) + + Expect(SyncProtectedServiceAccounts(ctx, fakeClient, operatorNS)).To(Succeed()) + data := getCM(fakeClient).Data + Expect(data).To(HaveKey("sa_team-a_collector-a")) + Expect(data).To(HaveKey(protectedSAPodCreatorsKey)) + Expect(data).To(HaveKey(protectedSAWorkloadCreatorsKey)) + }) + + It("reverts unauthorized ConfigMap edits on re-sync", func() { + fakeClient = newClient(clf("app-logging", "team-a", "collector-a")) + Expect(SyncProtectedServiceAccounts(ctx, fakeClient, operatorNS)).To(Succeed()) + + cm := getCM(fakeClient) + cm.Data["sa_rogue-ns_rogue-sa"] = "" + cm.Data[protectedSAPodCreatorsKey] = "system:serviceaccount:evil:hacker" + Expect(fakeClient.Update(ctx, cm)).To(Succeed()) + Expect(getCM(fakeClient).Data).To(HaveKey("sa_rogue-ns_rogue-sa")) + + Expect(SyncProtectedServiceAccounts(ctx, fakeClient, operatorNS)).To(Succeed()) + data := getCM(fakeClient).Data + Expect(data).ToNot(HaveKey("sa_rogue-ns_rogue-sa")) + Expect(data).To(HaveKey("sa_team-a_collector-a")) + Expect(strings.Split(data[protectedSAPodCreatorsKey], ",")).ToNot(ContainElement("system:serviceaccount:evil:hacker")) + }) + It("removes an SA from the ConfigMap when its CLF is deleted (rebuild-from-list)", func() { fakeClient = newClient(clf("app-logging", "team-a", "collector-a")) Expect(SyncProtectedServiceAccounts(ctx, fakeClient, operatorNS)).To(Succeed()) diff --git a/internal/admission/suite_test.go b/internal/admission/suite_test.go index 4b18a8877f..69bb066d61 100644 --- a/internal/admission/suite_test.go +++ b/internal/admission/suite_test.go @@ -6,6 +6,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + internalreconcile "github.com/openshift/cluster-logging-operator/internal/reconcile" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" "k8s.io/apimachinery/pkg/api/meta" apiruntime "k8s.io/apimachinery/pkg/runtime" @@ -20,17 +21,17 @@ func TestAdmission(t *testing.T) { var _ = Describe("admission policy helpers", func() { It("recognizes unsupported admission policy API errors", func() { - Expect(IsUnsupportedAdmissionPolicyAPI(&meta.NoKindMatchError{ + Expect(internalreconcile.IsUnsupportedAdmissionPolicyAPI(&meta.NoKindMatchError{ GroupKind: schema.GroupKind{Group: admissionregistrationv1.GroupName, Kind: "ValidatingAdmissionPolicy"}, })).To(BeTrue()) - Expect(IsUnsupportedAdmissionPolicyAPI(apiruntime.NewNotRegisteredErrForKind( + Expect(internalreconcile.IsUnsupportedAdmissionPolicyAPI(apiruntime.NewNotRegisteredErrForKind( "test", schema.GroupVersionKind{Group: admissionregistrationv1.GroupName, Version: "v1", Kind: "ValidatingAdmissionPolicy"}, ))).To(BeTrue()) - Expect(IsUnsupportedAdmissionPolicyAPI(&discovery.ErrGroupDiscoveryFailed{ + Expect(internalreconcile.IsUnsupportedAdmissionPolicyAPI(&discovery.ErrGroupDiscoveryFailed{ Groups: map[schema.GroupVersion]error{ {Group: admissionregistrationv1.GroupName, Version: "v1"}: fmt.Errorf("discovery failed"), }, })).To(BeTrue()) - Expect(IsUnsupportedAdmissionPolicyAPI(fmt.Errorf("forbidden"))).To(BeFalse()) + Expect(internalreconcile.IsUnsupportedAdmissionPolicyAPI(fmt.Errorf("forbidden"))).To(BeFalse()) }) }) diff --git a/internal/controller/admission/protected_sa_controller.go b/internal/controller/protected_sa_controller.go similarity index 63% rename from internal/controller/admission/protected_sa_controller.go rename to internal/controller/protected_sa_controller.go index 38f5676331..32ad961005 100644 --- a/internal/controller/admission/protected_sa_controller.go +++ b/internal/controller/protected_sa_controller.go @@ -1,4 +1,4 @@ -package admission +package controller import ( "context" @@ -6,8 +6,13 @@ import ( log "github.com/ViaQ/logerr/v2/log/static" obsv1 "github.com/openshift/cluster-logging-operator/api/observability/v1" internaladmission "github.com/openshift/cluster-logging-operator/internal/admission" + corev1 "k8s.io/api/core/v1" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" ) // ProtectedSAReconciler keeps the protected-ServiceAccount param ConfigMap in @@ -30,6 +35,16 @@ func (r *ProtectedSAReconciler) Reconcile(ctx context.Context, req ctrl.Request) func (r *ProtectedSAReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&obsv1.ClusterLogForwarder{}). - Named("protected-sa-configmap"). + Watches( + &corev1.ConfigMap{}, + handler.EnqueueRequestsFromMapFunc(func(_ context.Context, obj client.Object) []reconcile.Request { + return []reconcile.Request{{NamespacedName: client.ObjectKeyFromObject(obj)}} + }), + builder.WithPredicates(predicate.NewPredicateFuncs(func(obj client.Object) bool { + return obj.GetName() == internaladmission.ProtectedSAConfigMapName && + obj.GetNamespace() == r.OperatorNS + })), + ). + Named("clo-protected-sa"). Complete(r) } diff --git a/internal/controller/admission/protected_sa_runnable.go b/internal/controller/protected_sa_runnable.go similarity index 89% rename from internal/controller/admission/protected_sa_runnable.go rename to internal/controller/protected_sa_runnable.go index ce59b31f68..8f2b4800e7 100644 --- a/internal/controller/admission/protected_sa_runnable.go +++ b/internal/controller/protected_sa_runnable.go @@ -1,4 +1,4 @@ -package admission +package controller import ( "context" @@ -6,6 +6,7 @@ import ( log "github.com/ViaQ/logerr/v2/log/static" internaladmission "github.com/openshift/cluster-logging-operator/internal/admission" + internalreconcile "github.com/openshift/cluster-logging-operator/internal/reconcile" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -30,7 +31,7 @@ func (r *protectedSAAdmissionRunnable) Start(ctx context.Context) error { if err == nil { return nil } - if internaladmission.IsUnsupportedAdmissionPolicyAPI(err) { + if internalreconcile.IsUnsupportedAdmissionPolicyAPI(err) { return nil } delay := backoff.Step() diff --git a/internal/reconcile/admission.go b/internal/reconcile/admission.go new file mode 100644 index 0000000000..faa90e833c --- /dev/null +++ b/internal/reconcile/admission.go @@ -0,0 +1,72 @@ +package reconcile + +import ( + "context" + "errors" + "fmt" + + log "github.com/ViaQ/logerr/v2/log/static" + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + apiruntime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/discovery" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +// ValidatingAdmissionPolicy creates or updates a ValidatingAdmissionPolicy. +func ValidatingAdmissionPolicy(ctx context.Context, k8sClient client.Client, desired *admissionregistrationv1.ValidatingAdmissionPolicy) error { + current := &admissionregistrationv1.ValidatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: desired.Name, + }, + } + + op, err := controllerutil.CreateOrUpdate(ctx, k8sClient, current, func() error { + current.Spec = desired.Spec + return nil + }) + if err != nil { + return fmt.Errorf("reconcile ValidatingAdmissionPolicy %q: %w", desired.Name, err) + } + + log.V(3).Info("reconciled ValidatingAdmissionPolicy", "name", desired.Name, "operation", op) + return nil +} + +// ValidatingAdmissionPolicyBinding creates or updates a ValidatingAdmissionPolicyBinding. +func ValidatingAdmissionPolicyBinding(ctx context.Context, k8sClient client.Client, desired *admissionregistrationv1.ValidatingAdmissionPolicyBinding) error { + current := &admissionregistrationv1.ValidatingAdmissionPolicyBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: desired.Name, + }, + } + + op, err := controllerutil.CreateOrUpdate(ctx, k8sClient, current, func() error { + current.Spec = desired.Spec + return nil + }) + if err != nil { + return fmt.Errorf("reconcile ValidatingAdmissionPolicyBinding %q: %w", desired.Name, err) + } + + log.V(3).Info("reconciled ValidatingAdmissionPolicyBinding", "name", desired.Name, "operation", op) + return nil +} + +// IsUnsupportedAdmissionPolicyAPI returns true when the error indicates the +// ValidatingAdmissionPolicy API is not available on the cluster. +func IsUnsupportedAdmissionPolicyAPI(err error) bool { + if err == nil { + return false + } + if meta.IsNoMatchError(err) { + return true + } + if apiruntime.IsNotRegisteredError(err) { + return true + } + var groupDiscoveryErr *discovery.ErrGroupDiscoveryFailed + return errors.As(err, &groupDiscoveryErr) +} diff --git a/internal/runtime/observability/clusterlogforwarder.go b/internal/runtime/observability/clusterlogforwarder.go index 49298e5c10..721452978e 100644 --- a/internal/runtime/observability/clusterlogforwarder.go +++ b/internal/runtime/observability/clusterlogforwarder.go @@ -1,10 +1,44 @@ package observability import ( + "context" + "fmt" + "strings" + obsv1 "github.com/openshift/cluster-logging-operator/api/observability/v1" "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" ) +// ServiceAccountRef is a namespace + name pair identifying a collector ServiceAccount. +type ServiceAccountRef struct { + Namespace string + Name string +} + +// CollectorServiceAccounts lists all ClusterLogForwarders and returns the +// unique set of ServiceAccount references they declare. +func CollectorServiceAccounts(ctx context.Context, k8sClient client.Reader) ([]ServiceAccountRef, error) { + clfList := &obsv1.ClusterLogForwarderList{} + if err := k8sClient.List(ctx, clfList); err != nil { + return nil, fmt.Errorf("list ClusterLogForwarders: %w", err) + } + seen := map[ServiceAccountRef]struct{}{} + var refs []ServiceAccountRef + for i := range clfList.Items { + sa := strings.TrimSpace(clfList.Items[i].Spec.ServiceAccount.Name) + if sa == "" { + continue + } + ref := ServiceAccountRef{Namespace: clfList.Items[i].Namespace, Name: sa} + if _, ok := seen[ref]; !ok { + seen[ref] = struct{}{} + refs = append(refs, ref) + } + } + return refs, nil +} + // Initializer is a function that knows how to initialize a kubernetes runtime object type Initializer func(o runtime.Object, namespace, name string, visitors ...func(o runtime.Object)) From e62161e6567c84783b69a93963e1b41191c54e3a Mon Sep 17 00:00:00 2001 From: Vitalii Parfonov Date: Wed, 16 Sep 2026 12:19:22 +0300 Subject: [PATCH 4/5] fix(security): address PR review feedback - Move API availability check to manager setup time in cmd/main.go via IsAdmissionPolicyAPIAvailable(); controller registration is skipped if VAP API is unavailable, eliminating per-reconcile checks - Set operator common labels on VAP/binding objects at init() time - Remove ensureProtectedSAConfigMap as separate step; ConfigMap create-or-update is now folded into SyncProtectedServiceAccounts - Remove IsUnsupportedAdmissionPolicyAPI check from protected-SA runnable retry loop; API availability is now gated at startup - Update VAP/binding reconcile functions to copy desired.Labels to current.Labels so operator labels are persisted - Move CollectorServiceAccounts to new internal/runtime/clusterlogforwarder/ package; rename to ListServiceAccounts() for consistency with internal/runtime/service pattern - Add regression test for IsUnsupportedAdmissionPolicyAPI to ensure unrelated group discovery failures don't block reconciliation Signed-off-by: Vitalii Parfonov --- cmd/main.go | 27 +++++++------ internal/admission/protected_sa_policy.go | 37 ++++++------------ internal/admission/suite_test.go | 5 +++ internal/controller/protected_sa_runnable.go | 4 -- internal/reconcile/admission.go | 20 +++++++++- .../clusterlogforwarder.go | 39 +++++++++++++++++++ .../observability/clusterlogforwarder.go | 34 ---------------- 7 files changed, 90 insertions(+), 76 deletions(-) create mode 100644 internal/runtime/clusterlogforwarder/clusterlogforwarder.go diff --git a/cmd/main.go b/cmd/main.go index 21d23f5d83..6a34ab7e4b 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -14,6 +14,7 @@ import ( internalcontext "github.com/openshift/cluster-logging-operator/internal/api/context" "github.com/openshift/cluster-logging-operator/internal/collector" internalcontroller "github.com/openshift/cluster-logging-operator/internal/controller" + internalreconcile "github.com/openshift/cluster-logging-operator/internal/reconcile" internaltls "github.com/openshift/cluster-logging-operator/internal/tls" "sigs.k8s.io/controller-runtime/pkg/metrics/filters" @@ -261,21 +262,25 @@ func main() { } operatorNS := internaladmission.OperatorNamespace() - if err = (&internalcontroller.ProtectedSAReconciler{ - Client: mgr.GetClient(), - OperatorNS: operatorNS, - }).SetupWithManager(mgr); err != nil { - log.Error(err, "unable to create controller", "controller", "ProtectedServiceAccounts") - os.Exit(1) + if internalreconcile.IsAdmissionPolicyAPIAvailable(k8sClient) { + if err = (&internalcontroller.ProtectedSAReconciler{ + Client: mgr.GetClient(), + OperatorNS: operatorNS, + }).SetupWithManager(mgr); err != nil { + log.Error(err, "unable to create controller", "controller", "ProtectedServiceAccounts") + os.Exit(1) + } + + if err := mgr.Add(internalcontroller.NewProtectedSAAdmissionRunnable(k8sClient, operatorNS)); err != nil { + log.Error(err, "unable to register protected SA admission runnable") + os.Exit(1) + } + } else { + log.Info("ValidatingAdmissionPolicy API is unavailable; protected-SA controller disabled") } //+kubebuilder:scaffold:builder - if err := mgr.Add(internalcontroller.NewProtectedSAAdmissionRunnable(k8sClient, operatorNS)); err != nil { - log.Error(err, "unable to register protected SA admission runnable") - os.Exit(1) - } - if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { log.Error(err, "unable to set up health check") os.Exit(1) diff --git a/internal/admission/protected_sa_policy.go b/internal/admission/protected_sa_policy.go index 4d3eb3f400..c039378a08 100644 --- a/internal/admission/protected_sa_policy.go +++ b/internal/admission/protected_sa_policy.go @@ -11,7 +11,7 @@ import ( "github.com/openshift/cluster-logging-operator/internal/constants" internalreconcile "github.com/openshift/cluster-logging-operator/internal/reconcile" internalruntime "github.com/openshift/cluster-logging-operator/internal/runtime" - runtimeobs "github.com/openshift/cluster-logging-operator/internal/runtime/observability" + "github.com/openshift/cluster-logging-operator/internal/runtime/clusterlogforwarder" "github.com/openshift/cluster-logging-operator/internal/utils/comparators" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -71,6 +71,13 @@ func init() { protectedSAPodsBinding = internalruntime.Decode(protectedSAPodsBindingYAML).(*admissionregistrationv1.ValidatingAdmissionPolicyBinding) protectedSAWorkloadsPolicy = internalruntime.Decode(protectedSAWorkloadsPolicyYAML).(*admissionregistrationv1.ValidatingAdmissionPolicy) protectedSAWorkloadsBinding = internalruntime.Decode(protectedSAWorkloadsBindingYAML).(*admissionregistrationv1.ValidatingAdmissionPolicyBinding) + + for _, obj := range []internalruntime.Object{ + protectedSAPodsPolicy, protectedSAPodsBinding, + protectedSAWorkloadsPolicy, protectedSAWorkloadsBinding, + } { + internalruntime.SetCommonLabels(obj, constants.ClusterLogging, ProtectedSAConfigMapName, "admission") + } } // OperatorNamespace returns the namespace the operator pod runs in. @@ -98,9 +105,6 @@ func operatorServiceAccountUser(operatorNS string) string { // their bindings exist, and that the param ConfigMap exists with the allowed // creator identities populated. func ReconcileProtectedSAPolicies(ctx context.Context, k8sClient client.Client, operatorNS string) error { - if err := ensureProtectedSAConfigMap(k8sClient, operatorNS); err != nil { - return err - } if err := SyncProtectedServiceAccounts(ctx, k8sClient, operatorNS); err != nil { log.V(1).Info("initial protected ServiceAccount sync failed; will resync on CLF events", "error", err) } @@ -113,10 +117,6 @@ func ReconcileProtectedSAPolicies(ctx context.Context, k8sClient client.Client, {protectedSAWorkloadsPolicy, protectedSAWorkloadsBinding}, } { if err := internalreconcile.ValidatingAdmissionPolicy(ctx, k8sClient, p.policy); err != nil { - if internalreconcile.IsUnsupportedAdmissionPolicyAPI(err) { - log.Info("ValidatingAdmissionPolicy API is unavailable; skipping", "name", p.policy.Name) - return nil - } return err } binding := p.binding.DeepCopy() @@ -124,37 +124,22 @@ func ReconcileProtectedSAPolicies(ctx context.Context, k8sClient client.Client, binding.Spec.ParamRef.Namespace = operatorNS } if err := internalreconcile.ValidatingAdmissionPolicyBinding(ctx, k8sClient, binding); err != nil { - if internalreconcile.IsUnsupportedAdmissionPolicyAPI(err) { - log.Info("ValidatingAdmissionPolicyBinding API is unavailable; skipping", "name", binding.Name) - return nil - } return err } } return nil } -func ensureProtectedSAConfigMap(k8sClient client.Client, operatorNS string) error { - cm := internalruntime.NewConfigMap(operatorNS, ProtectedSAConfigMapName, nil) - internalruntime.SetCommonLabels(cm, constants.ClusterLogging, ProtectedSAConfigMapName, "admission") - cm.Data = map[string]string{} - setCreatorKeys(cm.Data, operatorNS) - if err := internalreconcile.Configmap(k8sClient, k8sClient, cm, comparators.CompareLabels); err != nil { - return fmt.Errorf("ensure protected SA ConfigMap %s/%s: %w", operatorNS, ProtectedSAConfigMapName, err) - } - return nil -} - func setCreatorKeys(data map[string]string, operatorNS string) { data[protectedSAPodCreatorsKey] = strings.Join(podControllers, ",") data[protectedSAWorkloadCreatorsKey] = strings.Join( append([]string{operatorServiceAccountUser(operatorNS)}, workloadControllers...), ",") } -// SyncProtectedServiceAccounts rebuilds the param ConfigMap's protected-SA -// membership from the full set of ClusterLogForwarders. +// SyncProtectedServiceAccounts rebuilds the param ConfigMap from the full set +// of ClusterLogForwarders. The ConfigMap is created if it does not exist. func SyncProtectedServiceAccounts(ctx context.Context, k8sClient client.Client, operatorNS string) error { - refs, err := runtimeobs.CollectorServiceAccounts(ctx, k8sClient) + refs, err := clusterlogforwarder.ListServiceAccounts(ctx, k8sClient) if err != nil { return err } diff --git a/internal/admission/suite_test.go b/internal/admission/suite_test.go index 69bb066d61..1d5cc3ea23 100644 --- a/internal/admission/suite_test.go +++ b/internal/admission/suite_test.go @@ -33,5 +33,10 @@ var _ = Describe("admission policy helpers", func() { }, })).To(BeTrue()) Expect(internalreconcile.IsUnsupportedAdmissionPolicyAPI(fmt.Errorf("forbidden"))).To(BeFalse()) + Expect(internalreconcile.IsUnsupportedAdmissionPolicyAPI(&discovery.ErrGroupDiscoveryFailed{ + Groups: map[schema.GroupVersion]error{ + {Group: "monitoring.coreos.com", Version: "v1"}: fmt.Errorf("discovery failed"), + }, + })).To(BeFalse(), "unrelated group discovery failure must not be treated as unsupported admission API") }) }) diff --git a/internal/controller/protected_sa_runnable.go b/internal/controller/protected_sa_runnable.go index 8f2b4800e7..d5b71afd9c 100644 --- a/internal/controller/protected_sa_runnable.go +++ b/internal/controller/protected_sa_runnable.go @@ -6,7 +6,6 @@ import ( log "github.com/ViaQ/logerr/v2/log/static" internaladmission "github.com/openshift/cluster-logging-operator/internal/admission" - internalreconcile "github.com/openshift/cluster-logging-operator/internal/reconcile" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -31,9 +30,6 @@ func (r *protectedSAAdmissionRunnable) Start(ctx context.Context) error { if err == nil { return nil } - if internalreconcile.IsUnsupportedAdmissionPolicyAPI(err) { - return nil - } delay := backoff.Step() log.V(1).Info("retrying protected SA ValidatingAdmissionPolicy reconciliation", "error", err, "retryAfter", delay) select { diff --git a/internal/reconcile/admission.go b/internal/reconcile/admission.go index faa90e833c..6699fd6ee4 100644 --- a/internal/reconcile/admission.go +++ b/internal/reconcile/admission.go @@ -24,6 +24,7 @@ func ValidatingAdmissionPolicy(ctx context.Context, k8sClient client.Client, des } op, err := controllerutil.CreateOrUpdate(ctx, k8sClient, current, func() error { + current.Labels = desired.Labels current.Spec = desired.Spec return nil }) @@ -44,6 +45,7 @@ func ValidatingAdmissionPolicyBinding(ctx context.Context, k8sClient client.Clie } op, err := controllerutil.CreateOrUpdate(ctx, k8sClient, current, func() error { + current.Labels = desired.Labels current.Spec = desired.Spec return nil }) @@ -55,6 +57,15 @@ func ValidatingAdmissionPolicyBinding(ctx context.Context, k8sClient client.Clie return nil } +// IsAdmissionPolicyAPIAvailable probes whether the ValidatingAdmissionPolicy +// API is registered on the cluster. Call once at startup to gate controller +// registration. +func IsAdmissionPolicyAPIAvailable(k8sClient client.Client) bool { + list := &admissionregistrationv1.ValidatingAdmissionPolicyList{} + err := k8sClient.List(context.Background(), list, client.Limit(1)) + return !IsUnsupportedAdmissionPolicyAPI(err) +} + // IsUnsupportedAdmissionPolicyAPI returns true when the error indicates the // ValidatingAdmissionPolicy API is not available on the cluster. func IsUnsupportedAdmissionPolicyAPI(err error) bool { @@ -68,5 +79,12 @@ func IsUnsupportedAdmissionPolicyAPI(err error) bool { return true } var groupDiscoveryErr *discovery.ErrGroupDiscoveryFailed - return errors.As(err, &groupDiscoveryErr) + if errors.As(err, &groupDiscoveryErr) { + for gv := range groupDiscoveryErr.Groups { + if gv.Group == admissionregistrationv1.GroupName { + return true + } + } + } + return false } diff --git a/internal/runtime/clusterlogforwarder/clusterlogforwarder.go b/internal/runtime/clusterlogforwarder/clusterlogforwarder.go new file mode 100644 index 0000000000..22b44ee143 --- /dev/null +++ b/internal/runtime/clusterlogforwarder/clusterlogforwarder.go @@ -0,0 +1,39 @@ +package clusterlogforwarder + +import ( + "context" + "fmt" + "strings" + + obsv1 "github.com/openshift/cluster-logging-operator/api/observability/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// ServiceAccountRef is a namespace + name pair identifying a collector ServiceAccount. +type ServiceAccountRef struct { + Namespace string + Name string +} + +// ListServiceAccounts lists all ClusterLogForwarders and returns the +// unique set of ServiceAccount references they declare. +func ListServiceAccounts(ctx context.Context, k8sClient client.Reader) ([]ServiceAccountRef, error) { + clfList := &obsv1.ClusterLogForwarderList{} + if err := k8sClient.List(ctx, clfList); err != nil { + return nil, fmt.Errorf("list ClusterLogForwarders: %w", err) + } + seen := map[ServiceAccountRef]struct{}{} + var refs []ServiceAccountRef + for i := range clfList.Items { + sa := strings.TrimSpace(clfList.Items[i].Spec.ServiceAccount.Name) + if sa == "" { + continue + } + ref := ServiceAccountRef{Namespace: clfList.Items[i].Namespace, Name: sa} + if _, ok := seen[ref]; !ok { + seen[ref] = struct{}{} + refs = append(refs, ref) + } + } + return refs, nil +} diff --git a/internal/runtime/observability/clusterlogforwarder.go b/internal/runtime/observability/clusterlogforwarder.go index 721452978e..49298e5c10 100644 --- a/internal/runtime/observability/clusterlogforwarder.go +++ b/internal/runtime/observability/clusterlogforwarder.go @@ -1,44 +1,10 @@ package observability import ( - "context" - "fmt" - "strings" - obsv1 "github.com/openshift/cluster-logging-operator/api/observability/v1" "k8s.io/apimachinery/pkg/runtime" - "sigs.k8s.io/controller-runtime/pkg/client" ) -// ServiceAccountRef is a namespace + name pair identifying a collector ServiceAccount. -type ServiceAccountRef struct { - Namespace string - Name string -} - -// CollectorServiceAccounts lists all ClusterLogForwarders and returns the -// unique set of ServiceAccount references they declare. -func CollectorServiceAccounts(ctx context.Context, k8sClient client.Reader) ([]ServiceAccountRef, error) { - clfList := &obsv1.ClusterLogForwarderList{} - if err := k8sClient.List(ctx, clfList); err != nil { - return nil, fmt.Errorf("list ClusterLogForwarders: %w", err) - } - seen := map[ServiceAccountRef]struct{}{} - var refs []ServiceAccountRef - for i := range clfList.Items { - sa := strings.TrimSpace(clfList.Items[i].Spec.ServiceAccount.Name) - if sa == "" { - continue - } - ref := ServiceAccountRef{Namespace: clfList.Items[i].Namespace, Name: sa} - if _, ok := seen[ref]; !ok { - seen[ref] = struct{}{} - refs = append(refs, ref) - } - } - return refs, nil -} - // Initializer is a function that knows how to initialize a kubernetes runtime object type Initializer func(o runtime.Object, namespace, name string, visitors ...func(o runtime.Object)) From cb742e3bd68eb7d3598df32b58733c16bdfaa955 Mon Sep 17 00:00:00 2001 From: Vitalii Parfonov Date: Thu, 24 Sep 2026 14:18:57 +0300 Subject: [PATCH 5/5] address review feedback - Move IsUnsupportedAdmissionPolicyAPI tests to separate api_availability_test.go - Document label flow in ValidatingAdmissionPolicy/Binding reconcile functions - Add ServiceAccountRef.String() method to return ConfigMap key format "sa__"; simplifies key generation and removes protectedSAKeyPrefix constant - Pre-compute static creator identity lists at init() time: podCreatorsValue and workloadCreatorsTemplate are now package-level vars, avoiding repeated joins on every sync Signed-off-by: Vitalii Parfonov --- ...cluster-logging.clusterserviceversion.yaml | 15 ++++- internal/admission/api_availability_test.go | 36 +++++++++++ .../admission/protected_sa_envtest_test.go | 3 +- internal/admission/protected_sa_policy.go | 62 ++++++++++--------- internal/admission/suite_test.go | 29 --------- internal/reconcile/admission.go | 6 +- .../clusterlogforwarder.go | 8 +++ 7 files changed, 97 insertions(+), 62 deletions(-) create mode 100644 internal/admission/api_availability_test.go diff --git a/bundle/manifests/cluster-logging.clusterserviceversion.yaml b/bundle/manifests/cluster-logging.clusterserviceversion.yaml index 998d4fcf30..cbd9b5629b 100644 --- a/bundle/manifests/cluster-logging.clusterserviceversion.yaml +++ b/bundle/manifests/cluster-logging.clusterserviceversion.yaml @@ -82,7 +82,7 @@ metadata: categories: OpenShift Optional, Logging & Tracing, Observability certified: "false" containerImage: quay.io/openshift-logging/cluster-logging-operator:latest - createdAt: "2026-09-18T21:15:15Z" + createdAt: "2026-09-24T11:45:31Z" description: The Red Hat OpenShift Logging Operator for OCP provides a means for configuring and managing log collection and forwarding. features.operators.openshift.io/cnf: "false" @@ -2508,6 +2508,19 @@ spec: - subjectaccessreviews verbs: - create + - apiGroups: + - admissionregistration.k8s.io + resources: + - validatingadmissionpolicies + - validatingadmissionpolicybindings + verbs: + - create + - delete + - get + - list + - patch + - update + - watch - apiGroups: - apps resources: diff --git a/internal/admission/api_availability_test.go b/internal/admission/api_availability_test.go new file mode 100644 index 0000000000..c8b0ee6d6b --- /dev/null +++ b/internal/admission/api_availability_test.go @@ -0,0 +1,36 @@ +package admission + +import ( + "fmt" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + internalreconcile "github.com/openshift/cluster-logging-operator/internal/reconcile" + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + "k8s.io/apimachinery/pkg/api/meta" + apiruntime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/discovery" +) + +var _ = Describe("admission policy API availability", func() { + It("recognizes unsupported admission policy API errors", func() { + Expect(internalreconcile.IsUnsupportedAdmissionPolicyAPI(&meta.NoKindMatchError{ + GroupKind: schema.GroupKind{Group: admissionregistrationv1.GroupName, Kind: "ValidatingAdmissionPolicy"}, + })).To(BeTrue()) + Expect(internalreconcile.IsUnsupportedAdmissionPolicyAPI(apiruntime.NewNotRegisteredErrForKind( + "test", schema.GroupVersionKind{Group: admissionregistrationv1.GroupName, Version: "v1", Kind: "ValidatingAdmissionPolicy"}, + ))).To(BeTrue()) + Expect(internalreconcile.IsUnsupportedAdmissionPolicyAPI(&discovery.ErrGroupDiscoveryFailed{ + Groups: map[schema.GroupVersion]error{ + {Group: admissionregistrationv1.GroupName, Version: "v1"}: fmt.Errorf("discovery failed"), + }, + })).To(BeTrue()) + Expect(internalreconcile.IsUnsupportedAdmissionPolicyAPI(fmt.Errorf("forbidden"))).To(BeFalse()) + Expect(internalreconcile.IsUnsupportedAdmissionPolicyAPI(&discovery.ErrGroupDiscoveryFailed{ + Groups: map[schema.GroupVersion]error{ + {Group: "monitoring.coreos.com", Version: "v1"}: fmt.Errorf("discovery failed"), + }, + })).To(BeFalse(), "unrelated group discovery failure must not be treated as unsupported admission API") + }) +}) diff --git a/internal/admission/protected_sa_envtest_test.go b/internal/admission/protected_sa_envtest_test.go index 9d80c85186..445286fbcd 100644 --- a/internal/admission/protected_sa_envtest_test.go +++ b/internal/admission/protected_sa_envtest_test.go @@ -9,6 +9,7 @@ import ( . "github.com/onsi/gomega" "github.com/openshift/cluster-logging-operator/internal/constants" internalruntime "github.com/openshift/cluster-logging-operator/internal/runtime" + "github.com/openshift/cluster-logging-operator/internal/runtime/clusterlogforwarder" "github.com/openshift/cluster-logging-operator/test" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" appsv1 "k8s.io/api/apps/v1" @@ -72,7 +73,7 @@ var _ = Describe("Protected SA VAP enforcement (envtest)", Ordered, func() { cm := internalruntime.NewConfigMap(operatorNS, ProtectedSAConfigMapName, nil) setCreatorKeys(cm.Data, operatorNS) - cm.Data[protectedSAKeyPrefix+podNS+"_"+collectorSA] = "" + cm.Data[clusterlogforwarder.ServiceAccountRef{Namespace: podNS, Name: collectorSA}.String()] = "" Expect(adminClient.Create(ctx, cm)).To(Succeed()) installPolicyAndBinding(ctx, adminClient, protectedSAPodsPolicy, protectedSAPodsBinding, operatorNS) diff --git a/internal/admission/protected_sa_policy.go b/internal/admission/protected_sa_policy.go index c039378a08..ee5ca967e0 100644 --- a/internal/admission/protected_sa_policy.go +++ b/internal/admission/protected_sa_policy.go @@ -25,7 +25,6 @@ const ( ProtectedSAWorkloadsPolicyName = "clo-protected-sa-workloads" ProtectedSAWorkloadsBindingName = "clo-protected-sa-workloads-binding" - protectedSAKeyPrefix = "sa_" protectedSAPodCreatorsKey = "podCreators" protectedSAWorkloadCreatorsKey = "workloadCreators" ) @@ -39,31 +38,31 @@ var podControllers = []string{ "system:serviceaccount:kube-system:job-controller", "system:serviceaccount:kube-system:replication-controller", } - -// workloadControllers lists kube-system controllers that create intermediate -// workload resources (e.g. Deployment → ReplicaSet, CronJob → Job). -var workloadControllers = []string{ - "system:serviceaccount:kube-system:deployment-controller", - "system:serviceaccount:kube-system:cronjob-controller", -} - -//go:embed protected-sa-pods.yaml -var protectedSAPodsPolicyYAML string - -//go:embed protected-sa-pods-binding.yaml -var protectedSAPodsBindingYAML string - -//go:embed protected-sa-workloads.yaml -var protectedSAWorkloadsPolicyYAML string - -//go:embed protected-sa-workloads-binding.yaml -var protectedSAWorkloadsBindingYAML string - var ( - protectedSAPodsPolicy *admissionregistrationv1.ValidatingAdmissionPolicy - protectedSAPodsBinding *admissionregistrationv1.ValidatingAdmissionPolicyBinding - protectedSAWorkloadsPolicy *admissionregistrationv1.ValidatingAdmissionPolicy - protectedSAWorkloadsBinding *admissionregistrationv1.ValidatingAdmissionPolicyBinding + // workloadControllers lists kube-system controllers that create intermediate + // workload resources (e.g. Deployment → ReplicaSet, CronJob → Job). + workloadControllers = []string{ + "system:serviceaccount:kube-system:deployment-controller", + "system:serviceaccount:kube-system:cronjob-controller", + } + // podCreatorsValue is the pre-joined comma-separated list of pod creators. + podCreatorsValue string + // workloadCreatorsTemplate is the pre-joined workload controllers, to be + // prefixed with the operator SA at runtime. + workloadCreatorsTemplate string + //go:embed protected-sa-pods.yaml + protectedSAPodsPolicyYAML string + //go:embed protected-sa-pods-binding.yaml + protectedSAPodsBindingYAML string + //go:embed protected-sa-workloads.yaml + protectedSAWorkloadsPolicyYAML string + + //go:embed protected-sa-workloads-binding.yaml + protectedSAWorkloadsBindingYAML string + protectedSAPodsPolicy *admissionregistrationv1.ValidatingAdmissionPolicy + protectedSAPodsBinding *admissionregistrationv1.ValidatingAdmissionPolicyBinding + protectedSAWorkloadsPolicy *admissionregistrationv1.ValidatingAdmissionPolicy + protectedSAWorkloadsBinding *admissionregistrationv1.ValidatingAdmissionPolicyBinding ) func init() { @@ -78,6 +77,10 @@ func init() { } { internalruntime.SetCommonLabels(obj, constants.ClusterLogging, ProtectedSAConfigMapName, "admission") } + + // Pre-compute static creator identity lists. + podCreatorsValue = strings.Join(podControllers, ",") + workloadCreatorsTemplate = strings.Join(workloadControllers, ",") } // OperatorNamespace returns the namespace the operator pod runs in. @@ -131,9 +134,8 @@ func ReconcileProtectedSAPolicies(ctx context.Context, k8sClient client.Client, } func setCreatorKeys(data map[string]string, operatorNS string) { - data[protectedSAPodCreatorsKey] = strings.Join(podControllers, ",") - data[protectedSAWorkloadCreatorsKey] = strings.Join( - append([]string{operatorServiceAccountUser(operatorNS)}, workloadControllers...), ",") + data[protectedSAPodCreatorsKey] = podCreatorsValue + data[protectedSAWorkloadCreatorsKey] = operatorServiceAccountUser(operatorNS) + "," + workloadCreatorsTemplate } // SyncProtectedServiceAccounts rebuilds the param ConfigMap from the full set @@ -146,7 +148,9 @@ func SyncProtectedServiceAccounts(ctx context.Context, k8sClient client.Client, data := map[string]string{} for _, ref := range refs { - data[protectedSAKeyPrefix+ref.Namespace+"_"+ref.Name] = "" + // Use ConfigMap data as a set: the VAP CEL expression checks key presence + // (saKey in params.data), not value, so empty string is sufficient. + data[ref.String()] = "" } setCreatorKeys(data, operatorNS) diff --git a/internal/admission/suite_test.go b/internal/admission/suite_test.go index 1d5cc3ea23..5f31bad033 100644 --- a/internal/admission/suite_test.go +++ b/internal/admission/suite_test.go @@ -1,42 +1,13 @@ package admission import ( - "fmt" "testing" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - internalreconcile "github.com/openshift/cluster-logging-operator/internal/reconcile" - admissionregistrationv1 "k8s.io/api/admissionregistration/v1" - "k8s.io/apimachinery/pkg/api/meta" - apiruntime "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/client-go/discovery" ) func TestAdmission(t *testing.T) { RegisterFailHandler(Fail) RunSpecs(t, "[internal][admission] Suite") } - -var _ = Describe("admission policy helpers", func() { - It("recognizes unsupported admission policy API errors", func() { - Expect(internalreconcile.IsUnsupportedAdmissionPolicyAPI(&meta.NoKindMatchError{ - GroupKind: schema.GroupKind{Group: admissionregistrationv1.GroupName, Kind: "ValidatingAdmissionPolicy"}, - })).To(BeTrue()) - Expect(internalreconcile.IsUnsupportedAdmissionPolicyAPI(apiruntime.NewNotRegisteredErrForKind( - "test", schema.GroupVersionKind{Group: admissionregistrationv1.GroupName, Version: "v1", Kind: "ValidatingAdmissionPolicy"}, - ))).To(BeTrue()) - Expect(internalreconcile.IsUnsupportedAdmissionPolicyAPI(&discovery.ErrGroupDiscoveryFailed{ - Groups: map[schema.GroupVersion]error{ - {Group: admissionregistrationv1.GroupName, Version: "v1"}: fmt.Errorf("discovery failed"), - }, - })).To(BeTrue()) - Expect(internalreconcile.IsUnsupportedAdmissionPolicyAPI(fmt.Errorf("forbidden"))).To(BeFalse()) - Expect(internalreconcile.IsUnsupportedAdmissionPolicyAPI(&discovery.ErrGroupDiscoveryFailed{ - Groups: map[schema.GroupVersion]error{ - {Group: "monitoring.coreos.com", Version: "v1"}: fmt.Errorf("discovery failed"), - }, - })).To(BeFalse(), "unrelated group discovery failure must not be treated as unsupported admission API") - }) -}) diff --git a/internal/reconcile/admission.go b/internal/reconcile/admission.go index 6699fd6ee4..2889964a9d 100644 --- a/internal/reconcile/admission.go +++ b/internal/reconcile/admission.go @@ -15,7 +15,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) -// ValidatingAdmissionPolicy creates or updates a ValidatingAdmissionPolicy. +// ValidatingAdmissionPolicy creates or updates a ValidatingAdmissionPolicy, +// applying the labels and spec from the desired object. func ValidatingAdmissionPolicy(ctx context.Context, k8sClient client.Client, desired *admissionregistrationv1.ValidatingAdmissionPolicy) error { current := &admissionregistrationv1.ValidatingAdmissionPolicy{ ObjectMeta: metav1.ObjectMeta{ @@ -36,7 +37,8 @@ func ValidatingAdmissionPolicy(ctx context.Context, k8sClient client.Client, des return nil } -// ValidatingAdmissionPolicyBinding creates or updates a ValidatingAdmissionPolicyBinding. +// ValidatingAdmissionPolicyBinding creates or updates a ValidatingAdmissionPolicyBinding, +// applying the labels and spec from the desired object. func ValidatingAdmissionPolicyBinding(ctx context.Context, k8sClient client.Client, desired *admissionregistrationv1.ValidatingAdmissionPolicyBinding) error { current := &admissionregistrationv1.ValidatingAdmissionPolicyBinding{ ObjectMeta: metav1.ObjectMeta{ diff --git a/internal/runtime/clusterlogforwarder/clusterlogforwarder.go b/internal/runtime/clusterlogforwarder/clusterlogforwarder.go index 22b44ee143..be0d680770 100644 --- a/internal/runtime/clusterlogforwarder/clusterlogforwarder.go +++ b/internal/runtime/clusterlogforwarder/clusterlogforwarder.go @@ -15,6 +15,14 @@ type ServiceAccountRef struct { Name string } +// String returns the ConfigMap data key for this ServiceAccount reference. +// The format "sa__" is collision-free because namespaces +// (DNS-1123 label) and ServiceAccount names (DNS-1123 subdomain) forbid '_', +// and ConfigMap keys may not contain '/'. +func (r ServiceAccountRef) String() string { + return fmt.Sprintf("sa_%s_%s", r.Namespace, r.Name) +} + // ListServiceAccounts lists all ClusterLogForwarders and returns the // unique set of ServiceAccount references they declare. func ListServiceAccounts(ctx context.Context, k8sClient client.Reader) ([]ServiceAccountRef, error) {