migrate group approver to use subject access reviews

cmd/kube-controller-manager/app/certificates.go

@@ -57,15 +57,11 @@ func startCSRApprovingController(ctx ControllerContext) (bool, error) {
 	if !ctx.AvailableResources[schema.GroupVersionResource{Group: "certificates.k8s.io", Version: "v1beta1", Resource: "certificatesigningrequests"}] {
 		return false, nil
 	}
-	if ctx.Options.ApproveAllKubeletCSRsForGroup == "" {
-		return false, nil
-	}
 	c := ctx.ClientBuilder.ClientOrDie("certificate-controller")
 
 	approver, err := approver.NewCSRApprovingController(
 		c,
 		ctx.InformerFactory.Certificates().V1beta1().CertificateSigningRequests(),
-		ctx.Options.ApproveAllKubeletCSRsForGroup,
 	)
 	if err != nil {
 		// TODO this is failing consistently in test-cmd and local-up-cluster.sh.  Fix them and make it consistent with all others which

cmd/kube-controller-manager/app/options/options.go

@@ -195,7 +195,9 @@ func (s *CMServer) AddFlags(fs *pflag.FlagSet, allControllers []string, disabled
 	fs.StringVar(&s.ClusterSigningCertFile, "cluster-signing-cert-file", s.ClusterSigningCertFile, "Filename containing a PEM-encoded X509 CA certificate used to issue cluster-scoped certificates")
 	fs.StringVar(&s.ClusterSigningKeyFile, "cluster-signing-key-file", s.ClusterSigningKeyFile, "Filename containing a PEM-encoded RSA or ECDSA private key used to sign cluster-scoped certificates")
 	fs.DurationVar(&s.ClusterSigningDuration.Duration, "experimental-cluster-signing-duration", s.ClusterSigningDuration.Duration, "The length of duration signed certificates will be given.")
-	fs.StringVar(&s.ApproveAllKubeletCSRsForGroup, "insecure-experimental-approve-all-kubelet-csrs-for-group", s.ApproveAllKubeletCSRsForGroup, "The group for which the controller-manager will auto approve all CSRs for kubelet client certificates.")
+	var dummy string
+	fs.MarkDeprecated("insecure-experimental-approve-all-kubelet-csrs-for-group", "This flag does nothing.")
+	fs.StringVar(&dummy, "insecure-experimental-approve-all-kubelet-csrs-for-group", s.ApproveAllKubeletCSRsForGroup, "This flag does nothing.")
 	fs.BoolVar(&s.EnableProfiling, "profiling", true, "Enable profiling via web interface host:port/debug/pprof/")
 	fs.BoolVar(&s.EnableContentionProfiling, "contention-profiling", false, "Enable lock contention profiling, if profiling is enabled")
 	fs.StringVar(&s.ClusterName, "cluster-name", s.ClusterName, "The instance prefix for the cluster")

pkg/apis/componentconfig/types.go

@@ -839,12 +839,6 @@ type KubeControllerManagerConfiguration struct {
 	// clusterSigningDuration is the length of duration signed certificates
 	// will be given.
 	ClusterSigningDuration metav1.Duration
-	// approveAllKubeletCSRs tells the CSR controller to approve all CSRs originating
-	// from the kubelet bootstrapping group automatically.
-	// WARNING: this grants all users with access to the certificates API group
-	// the ability to create credentials for any user that has access to the boostrapping
-	// user's credentials.
-	ApproveAllKubeletCSRsForGroup string
 	// enableProfiling enables profiling via web interface host:port/debug/pprof/
 	EnableProfiling bool
 	// enableContentionProfiling enables lock contention profiling, if enableProfiling is true.

pkg/controller/certificates/approver/BUILD

@@ -10,22 +10,29 @@ load(
 
 go_test(
     name = "go_default_test",
-    srcs = ["groupapprove_test.go"],
+    srcs = ["sarapprove_test.go"],
     library = ":go_default_library",
     tags = ["automanaged"],
-    deps = ["//pkg/apis/certificates/v1beta1:go_default_library"],
+    deps = [
+        "//pkg/apis/authorization/v1beta1:go_default_library",
+        "//pkg/apis/certificates/v1beta1:go_default_library",
+        "//pkg/client/clientset_generated/clientset/fake:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
+        "//vendor/k8s.io/client-go/testing:go_default_library",
+    ],
 )
 
 go_library(
     name = "go_default_library",
-    srcs = ["groupapprove.go"],
+    srcs = ["sarapprove.go"],
     tags = ["automanaged"],
     deps = [
+        "//pkg/apis/authorization/v1beta1:go_default_library",
         "//pkg/apis/certificates/v1beta1:go_default_library",
         "//pkg/client/clientset_generated/clientset:go_default_library",
         "//pkg/client/informers/informers_generated/externalversions/certificates/v1beta1:go_default_library",
         "//pkg/controller/certificates:go_default_library",
-        "//vendor/k8s.io/apimachinery/pkg/util/runtime:go_default_library",
     ],
 )
 

pkg/controller/certificates/approver/groupapprove.go

@@ -1,131 +0,0 @@
-/*
-Copyright 2016 The Kubernetes Authors.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-// Package approver implements an automated approver for kubelet certificates.
-package approver
-
-import (
-	"fmt"
-	"reflect"
-	"strings"
-
-	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
-	capi "k8s.io/kubernetes/pkg/apis/certificates/v1beta1"
-	"k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
-	certificatesinformers "k8s.io/kubernetes/pkg/client/informers/informers_generated/externalversions/certificates/v1beta1"
-	"k8s.io/kubernetes/pkg/controller/certificates"
-)
-
-func NewCSRApprovingController(
-	client clientset.Interface,
-	csrInformer certificatesinformers.CertificateSigningRequestInformer,
-	approveAllKubeletCSRsForGroup string,
-) (*certificates.CertificateController, error) {
-	approver := &groupApprover{
-		approveAllKubeletCSRsForGroup: approveAllKubeletCSRsForGroup,
-		client: client,
-	}
-	return certificates.NewCertificateController(
-		client,
-		csrInformer,
-		approver.handle,
-	)
-}
-
-// groupApprover implements AutoApprover for signing Kubelet certificates.
-type groupApprover struct {
-	approveAllKubeletCSRsForGroup string
-	client                        clientset.Interface
-}
-
-func (ga *groupApprover) handle(csr *capi.CertificateSigningRequest) error {
-	// short-circuit if we're already approved or denied
-	if approved, denied := certificates.GetCertApprovalCondition(&csr.Status); approved || denied {
-		return nil
-	}
-	csr, err := ga.autoApprove(csr)
-	if err != nil {
-		return fmt.Errorf("error auto approving csr: %v", err)
-	}
-	_, err = ga.client.Certificates().CertificateSigningRequests().UpdateApproval(csr)
-	if err != nil {
-		return fmt.Errorf("error updating approval for csr: %v", err)
-	}
-	return nil
-}
-
-func (cc *groupApprover) autoApprove(csr *capi.CertificateSigningRequest) (*capi.CertificateSigningRequest, error) {
-	isKubeletBootstrapGroup := false
-	for _, g := range csr.Spec.Groups {
-		if g == cc.approveAllKubeletCSRsForGroup {
-			isKubeletBootstrapGroup = true
-			break
-		}
-	}
-	if !isKubeletBootstrapGroup {
-		return csr, nil
-	}
-
-	x509cr, err := capi.ParseCSR(csr)
-	if err != nil {
-		utilruntime.HandleError(fmt.Errorf("unable to parse csr %q: %v", csr.Name, err))
-		return csr, nil
-	}
-	if !reflect.DeepEqual([]string{"system:nodes"}, x509cr.Subject.Organization) {
-		return csr, nil
-	}
-	if !strings.HasPrefix(x509cr.Subject.CommonName, "system:node:") {
-		return csr, nil
-	}
-	if len(x509cr.DNSNames)+len(x509cr.EmailAddresses)+len(x509cr.IPAddresses) != 0 {
-		return csr, nil
-	}
-	if !hasExactUsages(csr, kubeletClientUsages) {
-		return csr, nil
-	}
-
-	csr.Status.Conditions = append(csr.Status.Conditions, capi.CertificateSigningRequestCondition{
-		Type:    capi.CertificateApproved,
-		Reason:  "AutoApproved",
-		Message: "Auto approving of all kubelet CSRs is enabled on the controller manager",
-	})
-	return csr, nil
-}
-
-var kubeletClientUsages = []capi.KeyUsage{
-	capi.UsageKeyEncipherment,
-	capi.UsageDigitalSignature,
-	capi.UsageClientAuth,
-}
-
-func hasExactUsages(csr *capi.CertificateSigningRequest, usages []capi.KeyUsage) bool {
-	if len(usages) != len(csr.Spec.Usages) {
-		return false
-	}
-
-	usageMap := map[capi.KeyUsage]struct{}{}
-	for _, u := range usages {
-		usageMap[u] = struct{}{}
-	}
-
-	for _, u := range csr.Spec.Usages {
-		if _, ok := usageMap[u]; !ok {
-			return false
-		}
-	}
-
-	return true
-}

pkg/controller/certificates/approver/groupapprove_test.go

@@ -1,71 +0,0 @@
-/*
-Copyright 2017 The Kubernetes Authors.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-package approver
-
-import (
-	"testing"
-
-	api "k8s.io/kubernetes/pkg/apis/certificates/v1beta1"
-)
-
-func TestHasKubeletUsages(t *testing.T) {
-	cases := []struct {
-		usages   []api.KeyUsage
-		expected bool
-	}{
-		{
-			usages:   nil,
-			expected: false,
-		},
-		{
-			usages:   []api.KeyUsage{},
-			expected: false,
-		},
-		{
-			usages: []api.KeyUsage{
-				api.UsageKeyEncipherment,
-				api.UsageDigitalSignature,
-			},
-			expected: false,
-		},
-		{
-			usages: []api.KeyUsage{
-				api.UsageKeyEncipherment,
-				api.UsageDigitalSignature,
-				api.UsageServerAuth,
-			},
-			expected: false,
-		},
-		{
-			usages: []api.KeyUsage{
-				api.UsageKeyEncipherment,
-				api.UsageDigitalSignature,
-				api.UsageClientAuth,
-			},
-			expected: true,
-		},
-	}
-	for _, c := range cases {
-		if hasExactUsages(&api.CertificateSigningRequest{
-			Spec: api.CertificateSigningRequestSpec{
-				Usages: c.usages,
-			},
-		}, kubeletClientUsages) != c.expected {
-			t.Errorf("unexpected result of hasKubeletUsages(%v), expecting: %v", c.usages, c.expected)
-		}
-	}
-}

pkg/controller/certificates/approver/sarapprove.go

@@ -0,0 +1,200 @@
+/*
+Copyright 2016 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+// Package approver implements an automated approver for kubelet certificates.
+package approver
+
+import (
+	"crypto/x509"
+	"fmt"
+	"reflect"
+	"strings"
+
+	authorization "k8s.io/kubernetes/pkg/apis/authorization/v1beta1"
+	capi "k8s.io/kubernetes/pkg/apis/certificates/v1beta1"
+	"k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
+	certificatesinformers "k8s.io/kubernetes/pkg/client/informers/informers_generated/externalversions/certificates/v1beta1"
+	"k8s.io/kubernetes/pkg/controller/certificates"
+)
+
+type csrRecognizer struct {
+	recognize      func(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool
+	permission     authorization.ResourceAttributes
+	successMessage string
+}
+
+type sarApprover struct {
+	client      clientset.Interface
+	recognizers []csrRecognizer
+}
+
+func NewCSRApprovingController(client clientset.Interface, csrInformer certificatesinformers.CertificateSigningRequestInformer) (*certificates.CertificateController, error) {
+	approver := &sarApprover{
+		client:      client,
+		recognizers: recognizers(),
+	}
+	return certificates.NewCertificateController(
+		client,
+		csrInformer,
+		approver.handle,
+	)
+}
+
+func recognizers() []csrRecognizer {
+	return []csrRecognizer{
+		{
+			recognize:      isSelfNodeClientCert,
+			permission:     authorization.ResourceAttributes{Group: "certificates.k8s.io", Resource: "certificatesigningrequests", Verb: "create", Subresource: "selfnodeclient"},
+			successMessage: "Auto approving self kubelet client certificate after SubjectAccessReview.",
+		},
+		{
+			recognize:      isNodeClientCert,
+			permission:     authorization.ResourceAttributes{Group: "certificates.k8s.io", Resource: "certificatesigningrequests", Verb: "create", Subresource: "nodeclient"},
+			successMessage: "Auto approving kubelet client certificate after SubjectAccessReview.",
+		},
+		{
+			recognize:      isSelfNodeServerCert,
+			permission:     authorization.ResourceAttributes{Group: "certificates.k8s.io", Resource: "certificatesigningrequests", Verb: "create", Subresource: "selfnodeserver"},
+			successMessage: "Auto approving self kubelet server certificate after SubjectAccessReview.",
+		},
+	}
+}
+
+func (a *sarApprover) handle(csr *capi.CertificateSigningRequest) error {
+	if len(csr.Status.Certificate) != 0 {
+		return nil
+	}
+	if approved, denied := certificates.GetCertApprovalCondition(&csr.Status); approved || denied {
+		return nil
+	}
+	x509cr, err := capi.ParseCSR(csr)
+	if err != nil {
+		return fmt.Errorf("unable to parse csr %q: %v", csr.Name, err)
+	}
+
+	for _, r := range a.recognizers {
+		if !r.recognize(csr, x509cr) {
+			continue
+		}
+		approved, err := a.authorize(csr, r.permission)
+		if err != nil {
+			return err
+		}
+		if approved {
+			appendApprovalCondition(csr, r.successMessage)
+			_, err = a.client.Certificates().CertificateSigningRequests().UpdateApproval(csr)
+			if err != nil {
+				return fmt.Errorf("error updating approval for csr: %v", err)
+			}
+			return nil
+		}
+	}
+	return nil
+}
+
+func (a *sarApprover) authorize(csr *capi.CertificateSigningRequest, rattrs authorization.ResourceAttributes) (bool, error) {
+	extra := make(map[string]authorization.ExtraValue)
+	for k, v := range csr.Spec.Extra {
+		extra[k] = authorization.ExtraValue(v)
+	}
+
+	sar := &authorization.SubjectAccessReview{
+		Spec: authorization.SubjectAccessReviewSpec{
+			User:               csr.Spec.Username,
+			Groups:             csr.Spec.Groups,
+			Extra:              extra,
+			ResourceAttributes: &rattrs,
+		},
+	}
+	sar, err := a.client.AuthorizationV1beta1().SubjectAccessReviews().Create(sar)
+	if err != nil {
+		return false, err
+	}
+	return sar.Status.Allowed, nil
+}
+
+func appendApprovalCondition(csr *capi.CertificateSigningRequest, message string) {
+	csr.Status.Conditions = append(csr.Status.Conditions, capi.CertificateSigningRequestCondition{
+		Type:    capi.CertificateApproved,
+		Reason:  "AutoApproved",
+		Message: message,
+	})
+}
+
+func hasExactUsages(csr *capi.CertificateSigningRequest, usages []capi.KeyUsage) bool {
+	if len(usages) != len(csr.Spec.Usages) {
+		return false
+	}
+
+	usageMap := map[capi.KeyUsage]struct{}{}
+	for _, u := range usages {
+		usageMap[u] = struct{}{}
+	}
+
+	for _, u := range csr.Spec.Usages {
+		if _, ok := usageMap[u]; !ok {
+			return false
+		}
+	}
+
+	return true
+}
+
+var kubeletClientUsages = []capi.KeyUsage{
+	capi.UsageKeyEncipherment,
+	capi.UsageDigitalSignature,
+	capi.UsageClientAuth,
+}
+
+func isNodeClientCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool {
+	if !reflect.DeepEqual([]string{"system:nodes"}, x509cr.Subject.Organization) {
+		return false
+	}
+	if (len(x509cr.DNSNames) > 0) || (len(x509cr.EmailAddresses) > 0) || (len(x509cr.IPAddresses) > 0) {
+		return false
+	}
+	if !hasExactUsages(csr, kubeletClientUsages) {
+		return false
+	}
+	if !strings.HasPrefix(x509cr.Subject.CommonName, "system:node:") {
+		return false
+	}
+	return true
+}
+
+func isSelfNodeClientCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool {
+	if !isNodeClientCert(csr, x509cr) {
+		return false
+	}
+	if csr.Spec.Username != x509cr.Subject.CommonName {
+		return false
+	}
+	return true
+}
+
+var kubeletServerUsages = []capi.KeyUsage{
+	capi.UsageKeyEncipherment,
+	capi.UsageDigitalSignature,
+	capi.UsageServerAuth,
+}
+
+func isSelfNodeServerCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool {
+	if !hasExactUsages(csr, kubeletServerUsages) {
+		return false
+	}
+	//TODO(jcbsmpsn): implement the rest of this
+	return false
+}

pkg/controller/certificates/approver/sarapprove_test.go

@@ -0,0 +1,296 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package approver
+
+import (
+	"crypto/ecdsa"
+	"crypto/elliptic"
+	"crypto/x509"
+	"crypto/x509/pkix"
+	"encoding/pem"
+	"fmt"
+	"math/rand"
+	"net"
+	"testing"
+
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	testclient "k8s.io/client-go/testing"
+	authorization "k8s.io/kubernetes/pkg/apis/authorization/v1beta1"
+	capi "k8s.io/kubernetes/pkg/apis/certificates/v1beta1"
+	"k8s.io/kubernetes/pkg/client/clientset_generated/clientset/fake"
+)
+
+func TestHasKubeletUsages(t *testing.T) {
+	cases := []struct {
+		usages   []capi.KeyUsage
+		expected bool
+	}{
+		{
+			usages:   nil,
+			expected: false,
+		},
+		{
+			usages:   []capi.KeyUsage{},
+			expected: false,
+		},
+		{
+			usages: []capi.KeyUsage{
+				capi.UsageKeyEncipherment,
+				capi.UsageDigitalSignature,
+			},
+			expected: false,
+		},
+		{
+			usages: []capi.KeyUsage{
+				capi.UsageKeyEncipherment,
+				capi.UsageDigitalSignature,
+				capi.UsageServerAuth,
+			},
+			expected: false,
+		},
+		{
+			usages: []capi.KeyUsage{
+				capi.UsageKeyEncipherment,
+				capi.UsageDigitalSignature,
+				capi.UsageClientAuth,
+			},
+			expected: true,
+		},
+	}
+	for _, c := range cases {
+		if hasExactUsages(&capi.CertificateSigningRequest{
+			Spec: capi.CertificateSigningRequestSpec{
+				Usages: c.usages,
+			},
+		}, kubeletClientUsages) != c.expected {
+			t.Errorf("unexpected result of hasKubeletUsages(%v), expecting: %v", c.usages, c.expected)
+		}
+	}
+}
+
+func TestHandle(t *testing.T) {
+	cases := []struct {
+		message    string
+		allowed    bool
+		recognized bool
+		verify     func(*testing.T, []testclient.Action)
+	}{
+		{
+			recognized: false,
+			allowed:    false,
+			verify: func(t *testing.T, as []testclient.Action) {
+				if len(as) != 0 {
+					t.Errorf("expected no client calls but got: %#v", as)
+				}
+			},
+		},
+		{
+			recognized: false,
+			allowed:    true,
+			verify: func(t *testing.T, as []testclient.Action) {
+				if len(as) != 0 {
+					t.Errorf("expected no client calls but got: %#v", as)
+				}
+			},
+		},
+		{
+			recognized: true,
+			allowed:    false,
+			verify: func(t *testing.T, as []testclient.Action) {
+				if len(as) != 1 {
+					t.Errorf("expected 1 call but got: %#v", as)
+					return
+				}
+				_ = as[0].(testclient.CreateActionImpl)
+			},
+		},
+		{
+			recognized: true,
+			allowed:    true,
+			verify: func(t *testing.T, as []testclient.Action) {
+				if len(as) != 2 {
+					t.Errorf("expected two calls but got: %#v", as)
+					return
+				}
+				_ = as[0].(testclient.CreateActionImpl)
+				a := as[1].(testclient.UpdateActionImpl)
+				if got, expected := a.Verb, "update"; got != expected {
+					t.Errorf("got: %v, expected: %v", got, expected)
+				}
+				if got, expected := a.Resource, (schema.GroupVersionResource{Group: "certificates.k8s.io", Version: "v1beta1", Resource: "certificatesigningrequests"}); got != expected {
+					t.Errorf("got: %v, expected: %v", got, expected)
+				}
+				if got, expected := a.Subresource, "approval"; got != expected {
+					t.Errorf("got: %v, expected: %v", got, expected)
+				}
+				csr := a.Object.(*capi.CertificateSigningRequest)
+				if len(csr.Status.Conditions) != 1 {
+					t.Errorf("expected CSR to have approved condition: %#v", csr)
+				}
+				c := csr.Status.Conditions[0]
+				if got, expected := c.Type, capi.CertificateApproved; got != expected {
+					t.Errorf("got: %v, expected: %v", got, expected)
+				}
+				if got, expected := c.Reason, "AutoApproved"; got != expected {
+					t.Errorf("got: %v, expected: %v", got, expected)
+				}
+			},
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(fmt.Sprintf("recognized:%v,allowed: %v", c.recognized, c.allowed), func(t *testing.T) {
+			client := &fake.Clientset{}
+			client.AddReactor("create", "subjectaccessreviews", func(action testclient.Action) (handled bool, ret runtime.Object, err error) {
+				return true, &authorization.SubjectAccessReview{
+					Status: authorization.SubjectAccessReviewStatus{
+						Allowed: c.allowed,
+					},
+				}, nil
+			})
+			approver := sarApprover{
+				client: client,
+				recognizers: []csrRecognizer{
+					{
+						successMessage: "tester",
+						permission:     authorization.ResourceAttributes{Group: "foo", Resource: "bar", Subresource: "baz"},
+						recognize: func(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool {
+							return c.recognized
+						},
+					},
+				},
+			}
+			csr := makeTestCsr()
+			if err := approver.handle(csr); err != nil {
+				t.Errorf("unexpected err: %v", err)
+			}
+			c.verify(t, client.Actions())
+		})
+	}
+}
+
+func TestRecognizers(t *testing.T) {
+	goodCases := []func(b *csrBuilder){
+		func(b *csrBuilder) {
+		},
+	}
+
+	testRecognizer(t, goodCases, isNodeClientCert, true)
+	testRecognizer(t, goodCases, isSelfNodeClientCert, true)
+
+	badCases := []func(b *csrBuilder){
+		func(b *csrBuilder) {
+			b.cn = "mike"
+		},
+		func(b *csrBuilder) {
+			b.orgs = nil
+		},
+		func(b *csrBuilder) {
+			b.orgs = []string{"system:master"}
+		},
+		func(b *csrBuilder) {
+			b.usages = append(b.usages, capi.UsageServerAuth)
+		},
+	}
+
+	testRecognizer(t, badCases, isNodeClientCert, false)
+	testRecognizer(t, badCases, isSelfNodeClientCert, false)
+
+	// cn different then requestor
+	differentCN := []func(b *csrBuilder){
+		func(b *csrBuilder) {
+			b.requestor = "joe"
+		},
+		func(b *csrBuilder) {
+			b.cn = "system:node:bar"
+		},
+	}
+
+	testRecognizer(t, differentCN, isNodeClientCert, true)
+	testRecognizer(t, differentCN, isSelfNodeClientCert, false)
+}
+
+func testRecognizer(t *testing.T, cases []func(b *csrBuilder), recognizeFunc func(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool, shouldRecognize bool) {
+	for _, c := range cases {
+		b := csrBuilder{
+			cn:        "system:node:foo",
+			orgs:      []string{"system:nodes"},
+			requestor: "system:node:foo",
+			usages: []capi.KeyUsage{
+				capi.UsageKeyEncipherment,
+				capi.UsageDigitalSignature,
+				capi.UsageClientAuth,
+			},
+		}
+		c(&b)
+		t.Run(fmt.Sprintf("csr:%#v", b), func(t *testing.T) {
+			csr := makeFancyTestCsr(b)
+			x509cr, err := capi.ParseCSR(csr)
+			if err != nil {
+				t.Errorf("unexpected err: %v", err)
+			}
+			if recognizeFunc(csr, x509cr) != shouldRecognize {
+				t.Errorf("expected recognized to be %v", shouldRecognize)
+			}
+		})
+	}
+}
+
+// noncryptographic for faster testing
+// DO NOT COPY THIS CODE
+var insecureRand = rand.New(rand.NewSource(0))
+
+func makeTestCsr() *capi.CertificateSigningRequest {
+	return makeFancyTestCsr(csrBuilder{cn: "test-cert"})
+}
+
+type csrBuilder struct {
+	cn        string
+	orgs      []string
+	requestor string
+	usages    []capi.KeyUsage
+	dns       []string
+	emails    []string
+	ips       []net.IP
+}
+
+func makeFancyTestCsr(b csrBuilder) *capi.CertificateSigningRequest {
+	pk, err := ecdsa.GenerateKey(elliptic.P224(), insecureRand)
+	if err != nil {
+		panic(err)
+	}
+	csrb, err := x509.CreateCertificateRequest(insecureRand, &x509.CertificateRequest{
+		Subject: pkix.Name{
+			CommonName:   b.cn,
+			Organization: b.orgs,
+		},
+		DNSNames:       b.dns,
+		EmailAddresses: b.emails,
+		IPAddresses:    b.ips,
+	}, pk)
+	if err != nil {
+		panic(err)
+	}
+	return &capi.CertificateSigningRequest{
+		Spec: capi.CertificateSigningRequestSpec{
+			Username: b.requestor,
+			Usages:   b.usages,
+			Request:  pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csrb}),
+		},
+	}
+}