NO-JIRA: csr: fix RequestCommonNameFilter and the self-signing denial path

[arpitjain099]

Three small fixes in pkg/operator/csr/csr_approver.go. All of them are correctness issues, not security issues, since every one of the broken paths fails closed. Details below.

RequestCommonNameFilter doesn't satisfy CSRFilter

The method is declared as Match, but the interface wants Matches. AndFilter, OrFilter, LabelFilter and NamesFilter all declare Matches, so this one is the odd one out and the type can't actually be passed to NewCSRApproverController. Renamed it.

This is technically a rename of an exported method, but since the type never satisfied the interface it was meant for, there's no working filter-chain usage to break. Nothing in the tree calls it.

RequestCommonNameFilter parses PEM as DER

.spec.request is PEM-armored, and the filter handed it straight to x509.ParseCertificateRequest. That parse always fails, so the filter logged at V(4) and returned false for everything. sync() a bit further up gets this right: it runs pem.Decode first and bails if the block is nil. Did the same here, returning false on a nil block rather than erroring, which matches how the other filters behave.

Fail-closed, so the worst case was a filter that silently matched nothing.

Self-signing denial falls through to approval

In sync(), the x509CSR.Subject.CommonName == csr.Spec.Username branch called denyCSR but threw away the error and had no return, so it kept going and ran the approver. The two other denyCSR calls right below it (the CSRApprovingFailed and CSRDenied cases) both return. Added the return.

Worth being clear about the impact: this is not a bypass. The API server enforces mutual exclusion between Approved and Denied conditions, so the follow-up approval update doesn't land on an already-denied CSR. The real bug is that a failed denial got swallowed instead of surfacing, and the sync did extra work it shouldn't have.

Testing

Added TestRequestCommonNameFilter covering a PEM CSR with a listed CN, one with an unlisted CN, a non-PEM request, and an empty request, plus a var _ CSRFilter = ... assertion.

Checked it fails before the fix, both parts. With the method renamed back to Match:

pkg/operator/csr/csr_approver_test.go:405:20: cannot use NewRequestCommonNameFilter("whatever") (value of type *RequestCommonNameFilter) as CSRFilter value in variable declaration: *RequestCommonNameFilter does not implement CSRFilter (missing method Matches)

And with only the pem.Decode reverted (keeping the rename so it compiles):

--- FAIL: TestRequestCommonNameFilter (0.02s)
    --- FAIL: TestRequestCommonNameFilter/PEM-armored_CSR_with_a_listed_CN (0.00s)
        csr_approver_test.go:447: Matches() = false, want true

After the fix, go build ./... is clean and go test -count=1 ./pkg/operator/csr/... passes:

ok  	github.com/openshift/library-go/pkg/operator/csr	3.645s

The existing Test_csrApproverController_sync cases still pass with the added return.

Summary by CodeRabbit

• Bug Fixes

  • Self-requested certificate signing requests are now denied immediately, preventing further approval processing.
  • Common Name filtering now correctly handles PEM-encoded certificate requests.
  • Invalid or empty certificate requests are safely rejected instead of causing processing errors.

• Tests

  • Added coverage for matching and non-matching Common Names, malformed requests, and empty requests.

[openshift-ci-robot]

@arpitjain099: This pull request explicitly references no jira issue.

Details

In response to this:

Three small fixes in pkg/operator/csr/csr_approver.go. All of them are correctness issues, not security issues, since every one of the broken paths fails closed. Details below.

RequestCommonNameFilter doesn't satisfy CSRFilter

The method is declared as Match, but the interface wants Matches. AndFilter, OrFilter, LabelFilter and NamesFilter all declare Matches, so this one is the odd one out and the type can't actually be passed to NewCSRApproverController. Renamed it.

This is technically a rename of an exported method, but since the type never satisfied the interface it was meant for, there's no working filter-chain usage to break. Nothing in the tree calls it.

RequestCommonNameFilter parses PEM as DER

.spec.request is PEM-armored, and the filter handed it straight to x509.ParseCertificateRequest. That parse always fails, so the filter logged at V(4) and returned false for everything. sync() a bit further up gets this right: it runs pem.Decode first and bails if the block is nil. Did the same here, returning false on a nil block rather than erroring, which matches how the other filters behave.

Fail-closed, so the worst case was a filter that silently matched nothing.

Self-signing denial falls through to approval

In sync(), the x509CSR.Subject.CommonName == csr.Spec.Username branch called denyCSR but threw away the error and had no return, so it kept going and ran the approver. The two other denyCSR calls right below it (the CSRApprovingFailed and CSRDenied cases) both return. Added the return.

Worth being clear about the impact: this is not a bypass. The API server enforces mutual exclusion between Approved and Denied conditions, so the follow-up approval update doesn't land on an already-denied CSR. The real bug is that a failed denial got swallowed instead of surfacing, and the sync did extra work it shouldn't have.

Testing

Added TestRequestCommonNameFilter covering a PEM CSR with a listed CN, one with an unlisted CN, a non-PEM request, and an empty request, plus a var _ CSRFilter = ... assertion.

Checked it fails before the fix, both parts. With the method renamed back to Match:

pkg/operator/csr/csr_approver_test.go:405:20: cannot use NewRequestCommonNameFilter("whatever") (value of type *RequestCommonNameFilter) as CSRFilter value in variable declaration: *RequestCommonNameFilter does not implement CSRFilter (missing method Matches)

And with only the pem.Decode reverted (keeping the rename so it compiles):

--- FAIL: TestRequestCommonNameFilter (0.02s)
   --- FAIL: TestRequestCommonNameFilter/PEM-armored_CSR_with_a_listed_CN (0.00s)
       csr_approver_test.go:447: Matches() = false, want true

After the fix, go build ./... is clean and go test -count=1 ./pkg/operator/csr/... passes:

ok  	github.com/openshift/library-go/pkg/operator/csr	3.645s

The existing Test_csrApproverController_sync cases still pass with the added return.

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: arpitjain099
Once this PR has been reviewed and has the lgtm label, please assign p0lyn0mial 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:

• pkg/operator/OWNERS

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

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f565b6e5-ed65-427f-9cb0-b8820ca74f05

📥 Commits

Reviewing files that changed from the base of the PR and between 7965c84 and 83347e1.

📒 Files selected for processing (2)

• pkg/operator/csr/csr_approver.go
• pkg/operator/csr/csr_approver_test.go

---

Walkthrough

The CSR approver now returns after denying self-requesting CSRs. RequestCommonNameFilter implements the CSRFilter interface with Matches, validates PEM encoding, parses certificate requests, and has expanded unit test coverage.

Changes

CSR approver corrections

Layer / File(s)	Summary
Self-request denial flow pkg/operator/csr/csr_approver.go	The controller returns the result of denyCSR(...) when the CSR common name matches its username, preventing approval logic from running.
Common-name filter contract and parsing pkg/operator/csr/csr_approver.go, pkg/operator/csr/csr_approver_test.go	RequestCommonNameFilter now exposes Matches, PEM-decodes requests before parsing, rejects invalid or empty requests, and tests listed and unlisted common names.
Estimated code review effort: 3 (Moderate)	~20 minutes

🚥 Pre-merge checks | ✅ 15

✅ Passed checks (15 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the two main fixes in the PR: the common-name filter and the self-signing denial path.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	New test titles are static and deterministic; no dynamic values or brittle run-specific identifiers appear in the added names.
Test Structure And Quality	✅ Passed	The new table-driven unit test is isolated, deterministic, uses no cluster ops or waits, and matches existing repo patterns.
Microshift Test Compatibility	✅ Passed	Added test is a plain Go unit test (testing.T), not Ginkgo e2e, and it uses no MicroShift-unsupported APIs or cluster assumptions.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the change is a unit test and CSR controller logic, so SNO assumptions are not applicable.
Topology-Aware Scheduling Compatibility	✅ Passed	Only CSR approval/parsing logic and unit tests changed; no replicas, node selectors, affinity, topology spread, or PDBs were introduced.
Ote Binary Stdout Contract	✅ Passed	The PR only adds klog.Infof inside a normal method and no main/init/TestMain/RunSpecs setup or stdout prints appear in the touched files.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No Ginkgo/e2e tests were added; the new test is a local unit test with no IPv4 hardcodes or external connectivity.
No-Weak-Crypto	✅ Passed	Touched CSR code uses only standard x509/pem/RSA helpers; no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or secret comparisons found.
Container-Privileges	✅ Passed	Only csr approver code/tests changed; no manifests or privilege-related settings were added in the diff.
No-Sensitive-Data-In-Logs	✅ Passed	PASS: The new V(4) logs only emit CSR names and parse errors; no request payloads, secrets, tokens, PII, or other sensitive data are logged.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[openshift-ci]

Hi @arpitjain099. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.