feat(security): protect collector ServiceAccounts via ValidatingAdmissionPolicy by vparfonov · Pull Request #3405 · openshift/cluster-logging-operator

vparfonov · 2026-08-14T15:42:32Z

Description

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.

/cc
/assign

Links

Depending on PR(s):
GitHub issue:
JIRA:
Enhancement proposal:

…sionPolicy 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 <noreply@anthropic.com>

openshift-ci · 2026-08-14T15:42:37Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

coderabbitai · 2026-08-14T15:42:43Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 04f9c329-261b-4b7d-9273-110e554ebfe5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

_{Comment @coderabbitai help to get the list of available commands.}

openshift-ci · 2026-08-14T15:42:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vparfonov
Once this PR has been reviewed and has the lgtm label, please assign cahartma for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jcantrill · 2026-08-14T19:06:44Z

+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)" \


I'm guessing we could use this for our other admissions tests?

jcantrill · 2026-08-14T19:09:59Z

 	"strings"
 	"time"

+	internaladmission "github.com/openshift/cluster-logging-operator/internal/admission"


We should think if this should be in the controller package? Maybe that package is only for our resources?

jcantrill · 2026-08-14T19:12:27Z

+### 3. A Pod or workload is denied: `uses protected collector ServiceAccount ... may only be created by a CLO-managed collector controller`
+
+The cluster logging operator installs ValidatingAdmissionPolicies that prevent a
+collector ServiceAccount (one referenced by a `ClusterLogForwarder`) from being


...being refused.... It should probably say it is refused if not used as we intend. Something to that affect

jcantrill · 2026-08-18T19:44:06Z

@@ -0,0 +1,2 @@
+resources:
+- ../../internal/admission/manifests


Seeing the generated bundle which provides the RBAC, if these are being managed by a controller and not deployable in the bundle, then we should not require them here. This and the associated Make target can be removed.

jcantrill · 2026-08-18T19:50:58Z

+      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 collector ServiceAccount \"' + variables.sa + '\" and may only be created by a CLO-managed collector controller'"


Depending where this message is manifested it may require the namespace.

Maybe it should also be something like:

"'Pod uses protected ServiceAccount \"' + variables.sa + '\" which is only allowed for use by authorized ClusterLogForwarders'"

jcantrill · 2026-08-18T20:10:03Z

@@ -0,0 +1,34 @@
+package admission


This should be moved to the controller package for consistency

jcantrill · 2026-08-18T20:11:47Z

+func (r *ProtectedSAReconciler) SetupWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewControllerManagedBy(mgr).
+		For(&obsv1.ClusterLogForwarder{}).
+		Named("protected-sa-configmap").


We can name this "clo-protected-sa" and extract the name to a constant as i presume it is used elsewhere

jcantrill · 2026-08-18T20:14:06Z

+
+// OperatorNamespace returns the namespace the operator runs in, used both to
+// locate the param ConfigMap and to build the operator ServiceAccount username.
+func OperatorNamespace() string {


Similar functionality may already exist elsewhere and can be ascertained from the downward API and read from an env var if it is not already provided there

jcantrill · 2026-08-19T17:21:28Z

+		unprotectedSA  = "plain-sa"
+		restrictedUser = "system:serviceaccount:%s:restricted-user"
+
+		clfFmt = `


We have programmatic builders that are better suited then string interpolation

jcantrill · 2026-08-19T17:23:37Z

+		"validatingadmissionpolicy/" + admission.ProtectedSAWorkloadsPolicyName,
+		"validatingadmissionpolicybinding/" + admission.ProtectedSAWorkloadsBindingName,
+	} {
+		out, err := exec.Command("oc", "get", resource).CombinedOutput()


replace with existing utility functions

jcantrill · 2026-08-19T17:23:52Z

+	}, 2*time.Minute, 5*time.Second).Should(Succeed())
+}
+
+func ocCreate(namespace, yaml string) (string, error) {


replace with existing utilities

jcantrill · 2026-08-19T17:24:19Z

+	return string(out), err
+}
+
+func grantWorkloadEditor(namespace string) {


replace or impl into existing utilities

jcantrill · 2026-08-19T17:24:52Z

+// spoofedPodYAML copies the collector's visible metadata to prove that spoofing
+// labels/annotations/name does not bypass the policy.
+func spoofedPodYAML(namespace, sa, name string) string {
+	return fmt.Sprintf(`


replace with existing builders

openshift-ci · 2026-08-20T11:28:23Z

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 14, 2026

jcantrill reviewed Aug 14, 2026

View reviewed changes

jcantrill reviewed Aug 19, 2026

View reviewed changes

openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2026

		@@ -0,0 +1,2 @@
		resources:
		- ../../internal/admission/manifests

Uh oh!

Conversation

vparfonov commented Aug 14, 2026

Description

Links

Uh oh!

openshift-ci Bot commented Aug 14, 2026

Uh oh!

coderabbitai Bot commented Aug 14, 2026

Review skipped

Uh oh!

openshift-ci Bot commented Aug 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants