diff --git a/pkg/operator/csr/csr_approver.go b/pkg/operator/csr/csr_approver.go index f652743a0a..12525362a3 100644 --- a/pkg/operator/csr/csr_approver.go +++ b/pkg/operator/csr/csr_approver.go @@ -113,7 +113,7 @@ func (c *csrApproverController) sync(ctx context.Context, syncCtx factory.SyncCo } if x509CSR.Subject.CommonName == csr.Spec.Username { - c.denyCSR(ctx, csrCopy, "IllegitimateRequester", "requester cannot request certificates for themselves", syncCtx.Recorder()) + return c.denyCSR(ctx, csrCopy, "IllegitimateRequester", "requester cannot request certificates for themselves", syncCtx.Recorder()) } csrDecision, denyReason, err := c.csrApprover.Approve(csr, x509CSR) @@ -283,8 +283,14 @@ func NewRequestCommonNameFilter(commonNames ...string) *RequestCommonNameFilter return &RequestCommonNameFilter{sets.New(commonNames...)} } -func (f *RequestCommonNameFilter) Match(csr *certapiv1.CertificateSigningRequest) bool { - x509CSR, err := x509.ParseCertificateRequest(csr.Spec.Request) +func (f *RequestCommonNameFilter) Matches(csr *certapiv1.CertificateSigningRequest) bool { + csrPEM, _ := pem.Decode(csr.Spec.Request) + if csrPEM == nil { + klog.V(4).Infof("failed to PEM-parse the CSR block in .spec.request of %q: no CSRs were found", csr.Name) + return false + } + + x509CSR, err := x509.ParseCertificateRequest(csrPEM.Bytes) if err != nil { klog.V(4).Infof("failed to parse the CSR .spec.request of %q: %v", csr.Name, err) return false diff --git a/pkg/operator/csr/csr_approver_test.go b/pkg/operator/csr/csr_approver_test.go index c57dc0f1c0..52c932da6d 100644 --- a/pkg/operator/csr/csr_approver_test.go +++ b/pkg/operator/csr/csr_approver_test.go @@ -399,3 +399,53 @@ func (c fakeSyncContext) QueueKey() string { func (c fakeSyncContext) Recorder() events.Recorder { return c.eventRecorder } + +func TestRequestCommonNameFilter(t *testing.T) { + // the filter has to satisfy CSRFilter to be usable in a filter chain + var _ CSRFilter = NewRequestCommonNameFilter("whatever") + + tests := []struct { + name string + commonNames []string + request []byte + want bool + }{ + { + name: "PEM-armored CSR with a listed CN", + commonNames: []string{"someone-else", "therealyou"}, + request: genCSR(t, "therealyou"), + want: true, + }, + { + name: "PEM-armored CSR with an unlisted CN", + commonNames: []string{"therealyou"}, + request: genCSR(t, "someone-else"), + want: false, + }, + { + name: "no PEM block at all", + commonNames: []string{"therealyou"}, + request: []byte("this is not a PEM block"), + want: false, + }, + { + name: "empty request", + commonNames: []string{"therealyou"}, + request: nil, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := NewRequestCommonNameFilter(tt.commonNames...) + got := f.Matches(&certapiv1.CertificateSigningRequest{ + ObjectMeta: metav1.ObjectMeta{Name: "test-csr"}, + Spec: certapiv1.CertificateSigningRequestSpec{Request: tt.request}, + }) + if got != tt.want { + t.Errorf("Matches() = %v, want %v", got, tt.want) + } + }) + } +}