From 377adfa2b7726e8300460c615890a9fa39dab275 Mon Sep 17 00:00:00 2001
From: Jordan Liggitt <liggitt@google.com>
Date: Thu, 16 Apr 2020 00:14:29 -0400
Subject: [PATCH] Make signer admission plugin check on condition update

---
 .../certificates/signing/admission.go         | 10 ++++----
 .../certificates/signing/admission_test.go    | 25 +++++++++++++++++--
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/plugin/pkg/admission/certificates/signing/admission.go b/plugin/pkg/admission/certificates/signing/admission.go
index 4f072f803f9..2cd2b2e2740 100644
--- a/plugin/pkg/admission/certificates/signing/admission.go
+++ b/plugin/pkg/admission/certificates/signing/admission.go
@@ -24,10 +24,10 @@ import (
 
 	"k8s.io/klog/v2"
 
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apiserver/pkg/admission"
 	genericadmissioninit "k8s.io/apiserver/pkg/admission/initializer"
 	"k8s.io/apiserver/pkg/authorization/authorizer"
-
 	api "k8s.io/kubernetes/pkg/apis/certificates"
 	"k8s.io/kubernetes/plugin/pkg/admission/certificates"
 )
@@ -73,10 +73,10 @@ func NewPlugin() *Plugin {
 
 var csrGroupResource = api.Resource("certificatesigningrequests")
 
-// Validate verifies that the requesting user has permission to approve
+// Validate verifies that the requesting user has permission to sign
 // CertificateSigningRequests for the specified signerName.
 func (p *Plugin) Validate(ctx context.Context, a admission.Attributes, o admission.ObjectInterfaces) error {
-	// Ignore all calls to anything other than 'certificatesigningrequests/approval'.
+	// Ignore all calls to anything other than 'certificatesigningrequests/status'.
 	// Ignore all operations other than UPDATE.
 	if a.GetSubresource() != "status" ||
 		a.GetResource().GroupResource() != csrGroupResource {
@@ -92,8 +92,8 @@ func (p *Plugin) Validate(ctx context.Context, a admission.Attributes, o admissi
 		return admission.NewForbidden(a, fmt.Errorf("expected type CertificateSigningRequest, got: %T", a.GetObject()))
 	}
 
-	// only run if the status.certificate field has been changed
-	if reflect.DeepEqual(oldCSR.Status.Certificate, csr.Status.Certificate) {
+	// only run if the status.certificate or status.conditions field has been changed
+	if reflect.DeepEqual(oldCSR.Status.Certificate, csr.Status.Certificate) && apiequality.Semantic.DeepEqual(oldCSR.Status.Conditions, csr.Status.Conditions) {
 		return nil
 	}
 
diff --git a/plugin/pkg/admission/certificates/signing/admission_test.go b/plugin/pkg/admission/certificates/signing/admission_test.go
index 724c3e4dad3..ac2f114d602 100644
--- a/plugin/pkg/admission/certificates/signing/admission_test.go
+++ b/plugin/pkg/admission/certificates/signing/admission_test.go
@@ -48,7 +48,7 @@ func TestPlugin_Validate(t *testing.T) {
 			},
 			allowed: false,
 		},
-		"allowed if the 'certificate' field has not changed": {
+		"allowed if the 'certificate' and conditions field has not changed": {
 			attributes: &testAttributes{
 				resource:    certificatesapi.Resource("certificatesigningrequests"),
 				subresource: "status",
@@ -63,7 +63,7 @@ func TestPlugin_Validate(t *testing.T) {
 			allowed:  true,
 			authzErr: errors.New("faked error"),
 		},
-		"deny request if authz lookup fails": {
+		"deny request if authz lookup fails on certificate change": {
 			allowedName: "abc.com/xyz",
 			attributes: &testAttributes{
 				resource:    certificatesapi.Resource("certificatesigningrequests"),
@@ -84,6 +84,27 @@ func TestPlugin_Validate(t *testing.T) {
 			authzErr: errors.New("test"),
 			allowed:  false,
 		},
+		"deny request if authz lookup fails on condition change": {
+			allowedName: "abc.com/xyz",
+			attributes: &testAttributes{
+				resource:    certificatesapi.Resource("certificatesigningrequests"),
+				subresource: "status",
+				oldObj: &certificatesapi.CertificateSigningRequest{Spec: certificatesapi.CertificateSigningRequestSpec{
+					SignerName: "abc.com/xyz",
+				}},
+				obj: &certificatesapi.CertificateSigningRequest{
+					Spec: certificatesapi.CertificateSigningRequestSpec{
+						SignerName: "abc.com/xyz",
+					},
+					Status: certificatesapi.CertificateSigningRequestStatus{
+						Conditions: []certificatesapi.CertificateSigningRequestCondition{{Type: certificatesapi.CertificateFailed}},
+					},
+				},
+				operation: admission.Update,
+			},
+			authzErr: errors.New("test"),
+			allowed:  false,
+		},
 		"allow request if user is authorized for specific signerName": {
 			allowedName: "abc.com/xyz",
 			attributes: &testAttributes{