fix(security): CVE-2026-10609 verify CLF creator authorization for SA token usage (LOG-9441) - #3383
fix(security): CVE-2026-10609 verify CLF creator authorization for SA token usage (LOG-9441)#3383vparfonov wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughChangesClusterLogForwarder ServiceAccount authorization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ClusterLogForwarderWebhook
participant SubjectAccessReview
participant ClusterLogForwarderValidation
User->>ClusterLogForwarderWebhook: Submit ClusterLogForwarder
ClusterLogForwarderWebhook->>ClusterLogForwarderWebhook: Store modifier UserInfo
ClusterLogForwarderWebhook->>SubjectAccessReview: Check ServiceAccount use permission
SubjectAccessReview-->>ClusterLogForwarderWebhook: Return authorization status
ClusterLogForwarderWebhook->>ClusterLogForwarderValidation: Validate pipeline permissions
ClusterLogForwarderValidation-->>User: Accept or reject resource
🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vparfonov The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
internal/validations/observability/validate_permissions_test.go (2)
304-312: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the shadowed
clfServiceAccountdeclaration.The enclosing
Contextat Line 87 already declaresclfServiceAccountwith the same name, namespace, and type. This inner declaration shadows it and duplicates the fixture. Delete it and use the outer variable.♻️ Proposed change
Context("when validating SA usage authorization", func() { - var ( - clfServiceAccount = &corev1.ServiceAccount{ - ObjectMeta: v1.ObjectMeta{ - Name: "test-serviceAccount", - Namespace: constants.OpenshiftNS, - }, - } - ) - It("should pass when modifier annotation is set and user is authorized to use SA", func() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/validations/observability/validate_permissions_test.go` around lines 304 - 312, Remove the inner clfServiceAccount declaration from the “when validating SA usage authorization” Context and reuse the existing outer clfServiceAccount fixture declared by the enclosing Context, preserving its current name, namespace, and type.
314-430: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated LokiStack output spec and add a malformed-annotation case.
Two points:
- The three specs repeat the same LokiStack output block. Extract a helper such as
lokiStackSATokenOutput()and reuse it.- No spec covers a malformed modifier annotation.
validateModifierCanUseSAreturns an error for invalid JSON at Lines 166-168 ofinternal/validations/observability/validate_permissions.go, and that branch stays untested. Add a spec that setsconstants.AnnotationModifierto"not-json"and expectsReasonServiceAccountUsageNotAuthorized.The second point matters more, because a malformed annotation must fail closed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/validations/observability/validate_permissions_test.go` around lines 314 - 430, The validation tests repeat the LokiStack service-account output configuration and omit malformed modifier coverage. Add a shared helper such as lokiStackSATokenOutput() and reuse it in the three affected specs, then add a case setting constants.AnnotationModifier to "not-json" and assert ValidatePermissions produces an unauthorized condition with ReasonServiceAccountUsageNotAuthorized.internal/validations/observability/validate_permissions.go (3)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
ParseModifierUserInfoout of the webhook package.The validation package now depends on the webhook package for one annotation parser. The dependency direction is inverted: admission code and validation code should both depend on a shared helper, not on each other. Place
ParseModifierUserInfonext toconstants.AnnotationModifieror ininternal/api/observability, then let both callers use it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/validations/observability/validate_permissions.go` at line 19, Move ParseModifierUserInfo from the webhook package into a shared package alongside constants.AnnotationModifier or under internal/api/observability, then update both the admission and validation callers to use the shared symbol and remove the validation package’s direct webhook dependency.
164-190: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPass the caller context instead of
context.TODO().
validateModifierCanUseSAissues a blocking API call to create theSubjectAccessReview. Withcontext.TODO()the call carries no deadline and no cancellation. If the API server stalls, the reconcile worker blocks.internalcontext.ForwarderContextis already available inValidatePermissions, so thread acontext.Contextthrough the call.As per coding guidelines: "context.Context for cancellation and timeouts".
♻️ Proposed change
-func validateModifierCanUseSA(k8sClient client.Client, clf obs.ClusterLogForwarder, serviceAccount corev1.ServiceAccount) error { +func validateModifierCanUseSA(ctx context.Context, k8sClient client.Client, clf obs.ClusterLogForwarder, serviceAccount corev1.ServiceAccount) error { userInfo, err := clfwebhook.ParseModifierUserInfo(clf.Annotations) @@ - if err := k8sClient.Create(context.TODO(), sar); err != nil { + if err := k8sClient.Create(ctx, sar); err != nil { return fmt.Errorf("failed to check if user %q can use service account %q: %w", userInfo.Username, serviceAccount.Name, err) }Update the call site:
- if err = validateModifierCanUseSA(k8sClient, *clf, *serviceAccount); err != nil { + if err = validateModifierCanUseSA(ctx, k8sClient, *clf, *serviceAccount); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/validations/observability/validate_permissions.go` around lines 164 - 190, Thread the caller’s context from ValidatePermissions through validateModifierCanUseSA and use it for the blocking k8sClient.Create call that submits the SubjectAccessReview, replacing context.TODO(). Preserve the existing authorization checks and error handling while ensuring the API request inherits cancellation and deadlines from internalcontext.ForwarderContext.Source: Path instructions
169-173: 🔒 Security & Privacy | 🔵 TrivialAdd observability for the legacy bypass.
The legacy path allows any
ClusterLogForwarderthat carries no modifier annotation. The comment explains the reasoning, and the mutating webhook restores the annotation on the next write. The bypass therefore lasts until the resource is next modified, which can be indefinite.Two operational suggestions:
- Emit a metric or a
Warningevent, not only a log line, so administrators can count unverifiedClusterLogForwarderresources after upgrade.- Set the
Authorizedcondition message to state that strict enforcement is pending, so the state is visible inoc get clf -o yaml.
[operational]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/validations/observability/validate_permissions.go` around lines 169 - 173, Enhance the legacy bypass in the userInfo nil branch of the permissions validation flow to emit a countable metric or Warning event in addition to the existing log. Also set the ClusterLogForwarder Authorized condition message to clearly indicate that strict RBAC enforcement is pending until the resource is modified, while preserving the current allow behavior.internal/webhook/clusterlogforwarder_webhook_test.go (2)
261-285: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the SAR contents inside the mocks.
Both mocks set
Status.Allowedwithout checking the request. A regression that sends the wrongVerb,Resource,Namespace, orNamestill passes. The mocks ininternal/validations/observability/validate_permissions_test.goalready gate onResource == "serviceaccounts"andVerb == "use". Apply the same gate here.♻️ Proposed change
func (c *mockSARAllowClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { sar, ok := obj.(*authorizationapi.SubjectAccessReview) if !ok { return fmt.Errorf("unexpected object type: %T", obj) } + Expect(sar.Spec.ResourceAttributes).ToNot(BeNil()) + Expect(sar.Spec.ResourceAttributes.Resource).To(Equal("serviceaccounts")) + Expect(sar.Spec.ResourceAttributes.Verb).To(Equal("use")) sar.Status.Allowed = true return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/webhook/clusterlogforwarder_webhook_test.go` around lines 261 - 285, Update mockSARAllowClient.Create and mockSARDenyClient.Create to validate the incoming SubjectAccessReview’s Resource and Verb fields before setting Status.Allowed, matching the existing serviceaccount/use gate used by the observability permission mocks; reject mismatched requests so incorrect SAR contents cannot pass the tests.
149-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd specs for the error paths of
validateSAUsage.The current specs cover allow, deny, and skip. Three error paths stay uncovered:
admission.RequestFromContextfails, which happens when the context carries no admission request.v.Client.Createreturns an error for theSubjectAccessReview.clf.Spec.ServiceAccount.Nameis empty, which currently allows the request.The third case matters most. It documents the intentional bypass at Lines 91-93 of
internal/webhook/clusterlogforwarder_webhook.go.I can generate these specs if you want.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/webhook/clusterlogforwarder_webhook_test.go` around lines 149 - 257, Extend the validator specs around ValidateCreate and validateSAUsage to cover the three missing paths: a context without an admission request, a Client.Create error while creating the SubjectAccessReview, and an empty clf.Spec.ServiceAccount.Name that remains allowed. Use suitable mock clients and contexts, assert the expected error or successful validation, and preserve the existing allow, deny, and skip cases.test/e2e/collection/sa_authorization/sa_authorization_test.go (1)
141-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffRegister the
oc-created RBAC objects for cleanup and tolerate re-creation.
grantCLFAccessandgrantSAUsagecreate aRoleand aRoleBindingwith fixed names and callFailon any error, includingAlreadyExists. Building these objects with the typed client ande2e.Test.Recreate, astest/framework/e2e/auth.godoes forClusterRoleBinding, removes the ordering dependence and gives automatic cleanup. It also removes the dependence on anocbinary in the test environment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/collection/sa_authorization/sa_authorization_test.go` around lines 141 - 182, Replace the oc-based creation in grantCLFAccess and grantSAUsage with typed RBAC client object creation, registering each Role and RoleBinding through e2e.Test.Recreate so existing fixed-name objects are safely reused and automatically cleaned up. Follow the established pattern in the framework’s ClusterRoleBinding setup, preserving the current permissions, subjects, namespaces, and resource names while removing direct exec.Command and Fail-on-AlreadyExists behavior.internal/webhook/clusterlogforwarder_webhook.go (1)
95-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSet
Groupexplicitly on theResourceAttributes.
serviceaccountsbelongs to the core API group, so the emptyGroupvalue is correct today. State it explicitly to document intent and to avoid a wrong default if this block is copied for a non-core resource.internal/validations/observability/validate_permissions.gobuilds its SAR throughcreateSubjectAccessReview, which always passes a group; the two paths differ in style.♻️ Proposed change
ResourceAttributes: &authorizationapi.ResourceAttributes{ + Group: "", Verb: "use", Resource: "serviceaccounts", Name: saName, Namespace: clf.Namespace, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/webhook/clusterlogforwarder_webhook.go` around lines 95 - 107, Update the ResourceAttributes construction in the SubjectAccessReview within the webhook handler to set Group explicitly to the empty core API group value. Keep the existing serviceaccounts resource authorization unchanged, and only document the intended core-group setting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/default/manager_webhook_patch.yaml`:
- Around line 7-23: Update the container entry in the webhook patch to target
the existing cluster-logging-operator container instead of appending a new
manager container. Preserve the webhook port and certificate volume settings,
and add readOnlyRootFilesystem: true plus the required CPU and memory limits to
that container.
In `@config/webhook/service.yaml`:
- Around line 6-14: Update the webhook Service selector to use name:
cluster-logging-operator, matching the manager Deployment pod label instead of
control-plane. Also update the webhook patch target from manager to
cluster-logging-operator so containerPort 9443 applies to the correct container,
preserving connectivity for both fail-closed webhooks.
In `@internal/validations/observability/validate_permissions.go`:
- Around line 152-153: Update the obs.InputTypeReceiver branch in the permission
validation logic to restore the previous receiver classification instead of
inserting obs.InputTypeInfrastructure. Preserve the token fallback’s
collect-infrastructure-logs handling for receiver-only CLFs so existing
collector deployments remain enabled, while retaining the CVE-related
ServiceAccount authorization changes elsewhere.
In `@test/e2e/collection/sa_authorization/sa_authorization_test.go`:
- Around line 107-117: The spec titled “should allow legacy CLF without modifier
annotation to continue running” currently verifies webhook annotation injection
instead of legacy handling. Either rename the test to describe the
annotation-present behavior, or update its setup to remove
constants.AnnotationModifier through a direct API write that bypasses admission
before validating continued operation; alternatively cover the nil-annotation
case in validateModifierCanUse using the existing unit-test pattern.
- Around line 100-104: Update the annotation verification in the sa
authorization test to treat the value from ocGetAnnotation as serialized
UserInfo JSON rather than a plain username. Unmarshal annotation into the
appropriate user-info structure and assert its Username field equals user, while
preserving the existing error checks.
- Around line 134-139: Update ocGetAnnotation so the jsonpath annotation key is
passed unchanged inside the bracket-quoted expression; remove the
strings.ReplaceAll call while preserving the existing command and output
handling. Keep the strings import only if other code, such as strings.NewReader,
still uses it.
---
Nitpick comments:
In `@internal/validations/observability/validate_permissions_test.go`:
- Around line 304-312: Remove the inner clfServiceAccount declaration from the
“when validating SA usage authorization” Context and reuse the existing outer
clfServiceAccount fixture declared by the enclosing Context, preserving its
current name, namespace, and type.
- Around line 314-430: The validation tests repeat the LokiStack service-account
output configuration and omit malformed modifier coverage. Add a shared helper
such as lokiStackSATokenOutput() and reuse it in the three affected specs, then
add a case setting constants.AnnotationModifier to "not-json" and assert
ValidatePermissions produces an unauthorized condition with
ReasonServiceAccountUsageNotAuthorized.
In `@internal/validations/observability/validate_permissions.go`:
- Line 19: Move ParseModifierUserInfo from the webhook package into a shared
package alongside constants.AnnotationModifier or under
internal/api/observability, then update both the admission and validation
callers to use the shared symbol and remove the validation package’s direct
webhook dependency.
- Around line 164-190: Thread the caller’s context from ValidatePermissions
through validateModifierCanUseSA and use it for the blocking k8sClient.Create
call that submits the SubjectAccessReview, replacing context.TODO(). Preserve
the existing authorization checks and error handling while ensuring the API
request inherits cancellation and deadlines from
internalcontext.ForwarderContext.
- Around line 169-173: Enhance the legacy bypass in the userInfo nil branch of
the permissions validation flow to emit a countable metric or Warning event in
addition to the existing log. Also set the ClusterLogForwarder Authorized
condition message to clearly indicate that strict RBAC enforcement is pending
until the resource is modified, while preserving the current allow behavior.
In `@internal/webhook/clusterlogforwarder_webhook_test.go`:
- Around line 261-285: Update mockSARAllowClient.Create and
mockSARDenyClient.Create to validate the incoming SubjectAccessReview’s Resource
and Verb fields before setting Status.Allowed, matching the existing
serviceaccount/use gate used by the observability permission mocks; reject
mismatched requests so incorrect SAR contents cannot pass the tests.
- Around line 149-257: Extend the validator specs around ValidateCreate and
validateSAUsage to cover the three missing paths: a context without an admission
request, a Client.Create error while creating the SubjectAccessReview, and an
empty clf.Spec.ServiceAccount.Name that remains allowed. Use suitable mock
clients and contexts, assert the expected error or successful validation, and
preserve the existing allow, deny, and skip cases.
In `@internal/webhook/clusterlogforwarder_webhook.go`:
- Around line 95-107: Update the ResourceAttributes construction in the
SubjectAccessReview within the webhook handler to set Group explicitly to the
empty core API group value. Keep the existing serviceaccounts resource
authorization unchanged, and only document the intended core-group setting.
In `@test/e2e/collection/sa_authorization/sa_authorization_test.go`:
- Around line 141-182: Replace the oc-based creation in grantCLFAccess and
grantSAUsage with typed RBAC client object creation, registering each Role and
RoleBinding through e2e.Test.Recreate so existing fixed-name objects are safely
reused and automatically cleaned up. Follow the established pattern in the
framework’s ClusterRoleBinding setup, preserving the current permissions,
subjects, namespaces, and resource names while removing direct exec.Command and
Fail-on-AlreadyExists behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7362f811-bc02-4343-8a8f-2ac6a2e0ee95
📒 Files selected for processing (14)
api/observability/v1/conditions.gocmd/main.goconfig/default/kustomization.yamlconfig/default/manager_webhook_patch.yamlconfig/webhook/kustomization.yamlconfig/webhook/manifests.yamlconfig/webhook/service.yamlinternal/constants/annotations.gointernal/validations/observability/validate_permissions.gointernal/validations/observability/validate_permissions_test.gointernal/webhook/clusterlogforwarder_webhook.gointernal/webhook/clusterlogforwarder_webhook_test.gotest/e2e/collection/sa_authorization/sa_authorization_test.gotest/e2e/collection/sa_authorization/suite_test.go
|
/test e2e-target |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
PR-Agent: could not find a component named |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bundle/manifests/cluster-logging.clusterserviceversion.yaml`:
- Around line 2375-2384: Use the declared cluster-logging-operator Deployment as
the sole webhook workload: in
bundle/manifests/cluster-logging.clusterserviceversion.yaml lines 2375-2384,
remove the image-less manager container and move its webhook-server port and
certificate volume mount onto the existing cluster-logging-operator container;
at lines 2456-2496, set both deploymentName values to cluster-logging-operator;
update bundle/manifests/cluster-logging-operator-webhook-service_v1_service.yaml
lines 9-14 so its selector matches that Deployment’s template labels.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5e884923-90da-42e5-81ff-9141d4a6a824
📒 Files selected for processing (9)
bundle/manifests/cluster-logging-operator-webhook-service_v1_service.yamlbundle/manifests/cluster-logging.clusterserviceversion.yamlcmd/main.goconfig/default/manager_webhook_patch.yamlconfig/webhook/service.yamlinternal/validations/observability/validate_permissions.gointernal/validations/observability/validate_permissions_test.gointernal/webhook/clusterlogforwarder_webhook.gotest/e2e/collection/sa_authorization/sa_authorization_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- config/webhook/service.yaml
- cmd/main.go
- internal/validations/observability/validate_permissions_test.go
- internal/webhook/clusterlogforwarder_webhook.go
| - name: manager | ||
| ports: | ||
| - containerPort: 9443 | ||
| name: webhook-server | ||
| protocol: TCP | ||
| resources: {} | ||
| volumeMounts: | ||
| - mountPath: /etc/webhook-server/serving-certs | ||
| name: cert | ||
| readOnly: true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Use one valid webhook workload across the bundle.
The CSV defines only cluster-logging-operator, but the webhook configuration targets cluster-logging-operator-webhook. The added manager container also has no image, so Kubernetes rejects the Deployment. The executable cluster-logging-operator container does not receive the certificate mount.
bundle/manifests/cluster-logging.clusterserviceversion.yaml#L2375-L2384: Move the port and certificate mount to the existingcluster-logging-operatorcontainer. Do not add an image-lessmanagercontainer.bundle/manifests/cluster-logging.clusterserviceversion.yaml#L2456-L2496: Set bothdeploymentNamevalues to the declared Deployment name, or add a complete matching webhook Deployment.bundle/manifests/cluster-logging-operator-webhook-service_v1_service.yaml#L9-L14: Change the selector to match the selected Deployment template labels.
📍 Affects 2 files
bundle/manifests/cluster-logging.clusterserviceversion.yaml#L2375-L2384(this comment)bundle/manifests/cluster-logging.clusterserviceversion.yaml#L2456-L2496bundle/manifests/cluster-logging-operator-webhook-service_v1_service.yaml#L9-L14
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bundle/manifests/cluster-logging.clusterserviceversion.yaml` around lines
2375 - 2384, Use the declared cluster-logging-operator Deployment as the sole
webhook workload: in bundle/manifests/cluster-logging.clusterserviceversion.yaml
lines 2375-2384, remove the image-less manager container and move its
webhook-server port and certificate volume mount onto the existing
cluster-logging-operator container; at lines 2456-2496, set both deploymentName
values to cluster-logging-operator; update
bundle/manifests/cluster-logging-operator-webhook-service_v1_service.yaml lines
9-14 so its selector matches that Deployment’s template labels.
| return nil, fmt.Errorf("expected ClusterLogForwarder, got %T", obj) | ||
| } | ||
|
|
||
| if !internalobs.Outputs(clf.Spec.Outputs).NeedServiceAccountToken() { |
There was a problem hiding this comment.
We care about the SA usage regardless of the output since it can be granted SCC permissions and mounted to any workload
There was a problem hiding this comment.
My main concern is that non-cluster-admin CLF creates will break without new RBAC. The webhook requires use on the referenced ServiceAccount. So anyone who is not cluster-admin and without a role granting use will start getting not authorized after upgrade.
@jcantrill
| return nil | ||
| } | ||
|
|
||
| func (v *ClusterLogForwarderValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { |
There was a problem hiding this comment.
Introducing this webhook will allow us to move most all our "post create validations" here in future.
| return nil, nil | ||
| } | ||
|
|
||
| sar := &authorizationapi.SubjectAccessReview{ |
There was a problem hiding this comment.
Do we already have internal/runtime and buiders that do this from adding SAR to metric requests?
| } | ||
|
|
||
| // validateModifierCanUseSA checks that the user who last modified the CLF has permission to use the referenced SA. | ||
| // Legacy CLFs created before the webhook was deployed will not have the modifier annotation. |
There was a problem hiding this comment.
This validation runs post admission. The webhook runs on 'create' and 'update' which I don't believe either would run on upgrade of the operator. I think if we went this route it would be acceptible to validate permission on update and reject; this is something we should be able to note in the release notes. I'm not certain any of the following would be required then because it would already be validated in the webhook
There was a problem hiding this comment.
removed the reconciler checking
There was a problem hiding this comment.
My main concern is that non-cluster-admin CLF creates will break without new RBAC. The webhook requires use on the referenced ServiceAccount. So anyone who is not cluster-admin and without a role granting use will start getting not authorized after upgrade.
Release notes must cover this, probably with a breaking changes mark.
There was a problem hiding this comment.
@r2d2rnd Maybe you should know about this too and it would be interesting to hear your opinion
|
/test e2e-target |
|
PR-Agent: could not find a component named |
|
/test e2e-target |
|
PR-Agent: could not find a component named |
84d543b to
2c987c7
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
PR Summary by QodoFix CVE-2026-10609: validate ServiceAccount 'use' for CLF via webhook
AI Description
Diagram
High-Level Assessment
Files changed (16)
|
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
Code Review by Qodo
Context used✅ Compliance rules (platform):
9 rules 1.
|
![qodo-for-rh-openshift[bot]](https://avatars.githubusercontent.com/u/792337?s=60&v=4){kind=small}
a592b25 to
beb14f7
Compare
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Code review by qodo was updated up to the latest commit 7552c0d |
a5371ab to
5b796cc
Compare
…usage in CLF (LOG-9441) Add a ValidatingAdmissionWebhook that performs a SubjectAccessReview to verify the requesting user has 'use' permission on the ServiceAccount referenced in a ClusterLogForwarder. This prevents unauthorized users from forwarding SA tokens to external log outputs. Signed-off-by: Vitalii Parfonov <vparfono@redhat.com>
|
@vparfonov: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
2 participants
CVE-2026-10609: Authorize ServiceAccount usage in ClusterLogForwarder
Description
This PR addresses CVE-2026-10609, a privilege escalation vulnerability where users could reference any
ServiceAccountin aClusterLogForwarder (CLF)and forward its token to external log outputs without having explicit permission to use thatServiceAccount.Root Cause
The operator did not validate whether a user had permission to use the
ServiceAccountspecified in a CLF. This allowed privilege escalation if a cluster admin had granted a user's pod SCC permissions but not SA usage rights the user could then create a CLF to steal that SA's token.Solution
CREATEandUPDATEoperations forClusterLogForwarderresources.ServiceAccount.etcdwill continue to run without interruption upon operator upgrade.Additionally
hack/run-linterscript shebangs to#!/usr/bin/env bashfor better portability/cc @Clee2691
/assign @jcantrill
Links
Summary by CodeRabbit
New Features
Bug Fixes
Tests