certificate_manager: Check that template differs from current cert before rotation

With the current behavior, when kubelet starts, a `templateChanged`
event is always fired off because it only checks if `getLastRequest`
matches `getTemplate`. The last request only exists in memory and thus
is initially `nil` and can't ever match the current template during
startup.

This causes kubelet to request the signing of a new CSR every time it's
restarted. This commit changes the behavior so that `templateChanged` is
only fired off if the currently template doesn't match both the current
certificate and the last template.

Fixes #69471

Signed-off-by: Andrew Gunnerson <andrew.gunnerson@us.ibm.com>

Kubernetes-commit: b9ab65d689cc48353ca5dae9f210ff408726a0d2

util/certificate/certificate_manager.go

@@ -274,7 +274,7 @@ func (m *manager) Start() {
 	if m.dynamicTemplate {
 		go wait.Forever(func() {
 			// check if the current template matches what we last requested
-			if !reflect.DeepEqual(m.getLastRequest(), m.getTemplate()) {
+			if !m.certSatisfiesTemplate() && !reflect.DeepEqual(m.getLastRequest(), m.getTemplate()) {
 				// if the template is different, queue up an interrupt of the rotation deadline loop.
 				// if we've requested a CSR that matches the new template by the time the interrupt is handled, the interrupt is disregarded.
 				templateChanged <- struct{}{}
@@ -389,35 +389,30 @@ func (m *manager) rotateCerts() (bool, error) {
 	return true, nil
 }
 
-// nextRotationDeadline returns a value for the threshold at which the
+// Check that the current certificate on disk satisfies the requests from the
-// current certificate should be rotated, 80%+/-10% of the expiration of the
+// current template.
-// certificate.
+//
-func (m *manager) nextRotationDeadline() time.Time {
+// Note that extra items in the certificate's SAN or orgs that don't exist in
-	// forceRotation is not protected by locks
+// the template will not trigger a renewal.
-	if m.forceRotation {
+//
-		m.forceRotation = false
+// Requires certAccessLock to be locked.
-		return time.Now()
+func (m *manager) certSatisfiesTemplateLocked() bool {
-	}
-
-	m.certAccessLock.RLock()
-	defer m.certAccessLock.RUnlock()
 	if m.cert == nil {
-		return time.Now()
+		return false
 	}
 
-	// Ensure the currently held certificate satisfies the requested subject CN and SANs
 	if template := m.getTemplate(); template != nil {
 		if template.Subject.CommonName != m.cert.Leaf.Subject.CommonName {
-			glog.V(2).Infof("Current certificate CN (%s) does not match requested CN (%s), rotating now", m.cert.Leaf.Subject.CommonName, template.Subject.CommonName)
+			glog.V(2).Infof("Current certificate CN (%s) does not match requested CN (%s)", m.cert.Leaf.Subject.CommonName, template.Subject.CommonName)
-			return time.Now()
+			return false
 		}
 
 		currentDNSNames := sets.NewString(m.cert.Leaf.DNSNames...)
 		desiredDNSNames := sets.NewString(template.DNSNames...)
 		missingDNSNames := desiredDNSNames.Difference(currentDNSNames)
 		if len(missingDNSNames) > 0 {
-			glog.V(2).Infof("Current certificate is missing requested DNS names %v, rotating now", missingDNSNames.List())
+			glog.V(2).Infof("Current certificate is missing requested DNS names %v", missingDNSNames.List())
-			return time.Now()
+			return false
 		}
 
 		currentIPs := sets.NewString()
@@ -430,9 +425,43 @@ func (m *manager) nextRotationDeadline() time.Time {
 		}
 		missingIPs := desiredIPs.Difference(currentIPs)
 		if len(missingIPs) > 0 {
-			glog.V(2).Infof("Current certificate is missing requested IP addresses %v, rotating now", missingIPs.List())
+			glog.V(2).Infof("Current certificate is missing requested IP addresses %v", missingIPs.List())
-			return time.Now()
+			return false
 		}
+
+		currentOrgs := sets.NewString(m.cert.Leaf.Subject.Organization...)
+		desiredOrgs := sets.NewString(template.Subject.Organization...)
+		missingOrgs := desiredOrgs.Difference(currentOrgs)
+		if len(missingOrgs) > 0 {
+			glog.V(2).Infof("Current certificate is missing requested orgs %v", missingOrgs.List())
+			return false
+		}
+	}
+
+	return true
+}
+
+func (m *manager) certSatisfiesTemplate() bool {
+	m.certAccessLock.RLock()
+	defer m.certAccessLock.RUnlock()
+	return m.certSatisfiesTemplateLocked()
+}
+
+// nextRotationDeadline returns a value for the threshold at which the
+// current certificate should be rotated, 80%+/-10% of the expiration of the
+// certificate.
+func (m *manager) nextRotationDeadline() time.Time {
+	// forceRotation is not protected by locks
+	if m.forceRotation {
+		m.forceRotation = false
+		return time.Now()
+	}
+
+	m.certAccessLock.RLock()
+	defer m.certAccessLock.RUnlock()
+
+	if !m.certSatisfiesTemplateLocked() {
+		return time.Now()
 	}
 
 	notAfter := m.cert.Leaf.NotAfter

util/certificate/certificate_manager_test.go

@@ -22,6 +22,7 @@ import (
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"fmt"
+	"net"
 	"strings"
 	"testing"
 	"time"
@@ -212,6 +213,170 @@ func TestSetRotationDeadline(t *testing.T) {
 	}
 }
 
+func TestCertSatisfiesTemplate(t *testing.T) {
+	testCases := []struct {
+		name          string
+		cert          *x509.Certificate
+		template      *x509.CertificateRequest
+		shouldSatisfy bool
+	}{
+		{
+			name:          "No certificate, no template",
+			cert:          nil,
+			template:      nil,
+			shouldSatisfy: false,
+		},
+		{
+			name:          "No certificate",
+			cert:          nil,
+			template:      &x509.CertificateRequest{},
+			shouldSatisfy: false,
+		},
+		{
+			name: "No template",
+			cert: &x509.Certificate{
+				Subject: pkix.Name{
+					CommonName: "system:node:fake-node-name",
+				},
+			},
+			template:      nil,
+			shouldSatisfy: true,
+		},
+		{
+			name: "Mismatched common name",
+			cert: &x509.Certificate{
+				Subject: pkix.Name{
+					CommonName: "system:node:fake-node-name-2",
+				},
+			},
+			template: &x509.CertificateRequest{
+				Subject: pkix.Name{
+					CommonName: "system:node:fake-node-name",
+				},
+			},
+			shouldSatisfy: false,
+		},
+		{
+			name: "Missing orgs in certificate",
+			cert: &x509.Certificate{
+				Subject: pkix.Name{
+					Organization: []string{"system:nodes"},
+				},
+			},
+			template: &x509.CertificateRequest{
+				Subject: pkix.Name{
+					Organization: []string{"system:nodes", "foobar"},
+				},
+			},
+			shouldSatisfy: false,
+		},
+		{
+			name: "Extra orgs in certificate",
+			cert: &x509.Certificate{
+				Subject: pkix.Name{
+					Organization: []string{"system:nodes", "foobar"},
+				},
+			},
+			template: &x509.CertificateRequest{
+				Subject: pkix.Name{
+					Organization: []string{"system:nodes"},
+				},
+			},
+			shouldSatisfy: true,
+		},
+		{
+			name: "Missing DNS names in certificate",
+			cert: &x509.Certificate{
+				Subject:  pkix.Name{},
+				DNSNames: []string{"foo.example.com"},
+			},
+			template: &x509.CertificateRequest{
+				Subject:  pkix.Name{},
+				DNSNames: []string{"foo.example.com", "bar.example.com"},
+			},
+			shouldSatisfy: false,
+		},
+		{
+			name: "Extra DNS names in certificate",
+			cert: &x509.Certificate{
+				Subject:  pkix.Name{},
+				DNSNames: []string{"foo.example.com", "bar.example.com"},
+			},
+			template: &x509.CertificateRequest{
+				Subject:  pkix.Name{},
+				DNSNames: []string{"foo.example.com"},
+			},
+			shouldSatisfy: true,
+		},
+		{
+			name: "Missing IP addresses in certificate",
+			cert: &x509.Certificate{
+				Subject:     pkix.Name{},
+				IPAddresses: []net.IP{net.ParseIP("192.168.1.1")},
+			},
+			template: &x509.CertificateRequest{
+				Subject:     pkix.Name{},
+				IPAddresses: []net.IP{net.ParseIP("192.168.1.1"), net.ParseIP("192.168.1.2")},
+			},
+			shouldSatisfy: false,
+		},
+		{
+			name: "Extra IP addresses in certificate",
+			cert: &x509.Certificate{
+				Subject:     pkix.Name{},
+				IPAddresses: []net.IP{net.ParseIP("192.168.1.1"), net.ParseIP("192.168.1.2")},
+			},
+			template: &x509.CertificateRequest{
+				Subject:     pkix.Name{},
+				IPAddresses: []net.IP{net.ParseIP("192.168.1.1")},
+			},
+			shouldSatisfy: true,
+		},
+		{
+			name: "Matching certificate",
+			cert: &x509.Certificate{
+				Subject: pkix.Name{
+					CommonName:   "system:node:fake-node-name",
+					Organization: []string{"system:nodes"},
+				},
+				DNSNames:    []string{"foo.example.com"},
+				IPAddresses: []net.IP{net.ParseIP("192.168.1.1")},
+			},
+			template: &x509.CertificateRequest{
+				Subject: pkix.Name{
+					CommonName:   "system:node:fake-node-name",
+					Organization: []string{"system:nodes"},
+				},
+				DNSNames:    []string{"foo.example.com"},
+				IPAddresses: []net.IP{net.ParseIP("192.168.1.1")},
+			},
+			shouldSatisfy: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			var tlsCert *tls.Certificate
+
+			if tc.cert != nil {
+				tlsCert = &tls.Certificate{
+					Leaf: tc.cert,
+				}
+			}
+
+			m := manager{
+				cert:        tlsCert,
+				getTemplate: func() *x509.CertificateRequest { return tc.template },
+			}
+
+			result := m.certSatisfiesTemplate()
+			if result != tc.shouldSatisfy {
+				t.Errorf("cert: %+v, template: %+v, certSatisfiesTemplate returned %v, want %v", m.cert, tc.template, result, tc.shouldSatisfy)
+			}
+		})
+	}
+}
+
 func TestRotateCertCreateCSRError(t *testing.T) {
 	now := time.Now()
 	m := manager{