diff --git a/util/certificate/certificate_manager.go b/util/certificate/certificate_manager.go index 7b07b26a..fbdf4ec7 100644 --- a/util/certificate/certificate_manager.go +++ b/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 -// 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() +// Check that the current certificate on disk satisfies the requests from the +// current template. +// +// Note that extra items in the certificate's SAN or orgs that don't exist in +// the template will not trigger a renewal. +// +// Requires certAccessLock to be locked. +func (m *manager) certSatisfiesTemplateLocked() bool { 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) - return time.Now() + glog.V(2).Infof("Current certificate CN (%s) does not match requested CN (%s)", m.cert.Leaf.Subject.CommonName, template.Subject.CommonName) + 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()) - return time.Now() + glog.V(2).Infof("Current certificate is missing requested DNS names %v", missingDNSNames.List()) + 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()) - return time.Now() + glog.V(2).Infof("Current certificate is missing requested IP addresses %v", missingIPs.List()) + 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 diff --git a/util/certificate/certificate_manager_test.go b/util/certificate/certificate_manager_test.go index 299a1d53..545097ea 100644 --- a/util/certificate/certificate_manager_test.go +++ b/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{