Rotate certificate upon valid principals change.

Ignore 0.0.0.0 when checking if certificate needs to be rotated.

lib/service/connect.go

@@ -600,6 +600,37 @@ type rotationStatus struct {
 	ca services.CertAuthority
 }
 
+// checkPrincipals returns a boolean that indicates the host certificate
+// needs to be regenerated.
+func checkPrincipals(conn *Connector, additionalPrincipals []string, dnsNames []string) bool {
+	var principalsChanged bool
+	var dnsNamesChanged bool
+
+	// Remove 0.0.0.0 (meaning advertise_ip has not) if it exists in the list of
+	// principals. The 0.0.0.0 values tells the auth server to "guess" the nodes
+	// IP. If 0.0.0.0 is not removed, a check is performed if it exists in the
+	// list of principals in the certificate. Since it never exists in the list
+	// of principals (auth server will always remove it before issuing a
+	// certificate) regeneration is always requested.
+	principalsToCheck := utils.RemoveFromSlice(additionalPrincipals, defaults.AnyAddress)
+
+	// If advertise_ip, public_addr, or listen_addr in file configuration were
+	// updated, the list of principals (SSH) or DNS names (TLS) on the
+	// certificate need to be updated.
+	if len(additionalPrincipals) != 0 && !conn.ServerIdentity.HasPrincipals(principalsToCheck) {
+		principalsChanged = true
+		log.Debugf("Rotation in progress, adding %v to SSH principals %v.",
+			additionalPrincipals, conn.ServerIdentity.Cert.ValidPrincipals)
+	}
+	if len(dnsNames) != 0 && !conn.ServerIdentity.HasDNSNames(dnsNames) {
+		dnsNamesChanged = true
+		log.Debugf("Rotation in progress, adding %v to x590 DNS names in SAN %v.",
+			dnsNames, conn.ServerIdentity.XCert.DNSNames)
+	}
+
+	return principalsChanged || dnsNamesChanged
+}
+
 // rotate is called to check if rotation should be triggered.
 func (process *TeleportProcess) rotate(conn *Connector, localState auth.StateV2, remote services.Rotation) (*rotationStatus, error) {
 	id := conn.ClientIdentity.ID
@@ -610,31 +641,13 @@ func (process *TeleportProcess) rotate(conn *Connector, localState auth.StateV2,
 		return nil, trace.Wrap(err)
 	}
 
-	additionalPrincipals = utils.ReplaceInSlice(
-		additionalPrincipals,
-		defaults.AnyAddress,
-		defaults.Localhost,
-	)
-
-	// If advertise_ip, public_addr, or listen_addr in file configuration were
-	// updated, the list of principals (SSH) and DNS names (TLS) on the
-	// certificate need to be updated.
-	var principalsChanged bool
-	if len(additionalPrincipals) != 0 && !conn.ServerIdentity.HasPrincipals(additionalPrincipals) {
-		principalsChanged = true
-		log.Debugf("Rotation in progress, updating SSH principals from %v to %v.",
-			conn.ServerIdentity.Cert.ValidPrincipals, additionalPrincipals)
-	}
-	var dnsNamesChanged bool
-	if len(dnsNames) != 0 && !conn.ServerIdentity.HasDNSNames(dnsNames) {
-		log.Debugf("Rotation in progress, updating x590 DNS names in SAN from %v to %v.",
-			conn.ServerIdentity.XCert.DNSNames, dnsNames)
-		dnsNamesChanged = true
-	}
+	// Check if any of the SSH principals or TLS DNS names have changed and the
+	// host credentials need to be regenerated.
+	regenerateCertificate := checkPrincipals(conn, additionalPrincipals, dnsNames)
 
 	// If the local state matches remote state and neither principals or DNS
 	// names changed, nothing to do. CA is in sync.
-	if local.Matches(remote) && !(principalsChanged || dnsNamesChanged) {
+	if local.Matches(remote) && !regenerateCertificate {
 		return &rotationStatus{}, nil
 	}
 
@@ -662,7 +675,7 @@ func (process *TeleportProcess) rotate(conn *Connector, localState auth.StateV2,
 		// that the old node came up and missed the whole rotation
 		// rollback cycle.
 		case "", services.RotationStateStandby:
-			if principalsChanged || dnsNamesChanged {
+			if regenerateCertificate {
 				process.Infof("Service %v has updated principals to %q, DNS Names to %q, going to request new principals and update.", id.Role, additionalPrincipals, dnsNames)
 				identity, err := process.reRegister(conn, additionalPrincipals, dnsNames, remote)
 				if err != nil {

lib/service/service_test.go

@@ -24,6 +24,7 @@ import (
 
 	"strconv"
 
+	"github.com/gravitational/teleport/lib/auth"
 	"github.com/gravitational/teleport/lib/defaults"
 	"github.com/gravitational/teleport/lib/utils"
 
@@ -120,6 +121,68 @@ func (s *ServiceTestSuite) TestMonitor(c *check.C) {
 	c.Assert(err, check.IsNil)
 }
 
+// TestCheckPrincipals checks certificates regeneration only requests
+// regeneration when the principals change.
+func (s *ServiceTestSuite) TestCheckPrincipals(c *check.C) {
+	dataDir := c.MkDir()
+
+	// Create a test auth server to extract the server identity (SSH and TLS
+	// certificates).
+	testAuthServer, err := auth.NewTestAuthServer(auth.TestAuthServerConfig{
+		Dir: dataDir,
+	})
+	c.Assert(err, check.IsNil)
+	tlsServer, err := testAuthServer.NewTestTLSServer()
+	c.Assert(err, check.IsNil)
+	defer tlsServer.Close()
+
+	testConnector := &Connector{
+		ServerIdentity: tlsServer.Identity,
+	}
+
+	var tests = []struct {
+		inPrincipals  []string
+		inDNS         []string
+		outRegenerate bool
+	}{
+		// If nothing has been updated, don't regenerate certificate.
+		{
+			inPrincipals:  []string{},
+			inDNS:         []string{},
+			outRegenerate: false,
+		},
+		// Don't regenerate certificate if the node does not know it's own address.
+		{
+			inPrincipals:  []string{"0.0.0.0"},
+			inDNS:         []string{},
+			outRegenerate: false,
+		},
+		// If a new SSH principal is found, regenerate certificate.
+		{
+			inPrincipals:  []string{"1.1.1.1"},
+			inDNS:         []string{},
+			outRegenerate: true,
+		},
+		// If a new TLS DNS name is found, regenerate certificate.
+		{
+			inPrincipals:  []string{},
+			inDNS:         []string{"server.example.com"},
+			outRegenerate: true,
+		},
+		// Don't regenerate certificate if additional principals is already on the
+		// certificate.
+		{
+			inPrincipals:  []string{"test-tls-server"},
+			inDNS:         []string{},
+			outRegenerate: false,
+		},
+	}
+	for _, tt := range tests {
+		ok := checkPrincipals(testConnector, tt.inPrincipals, tt.inDNS)
+		c.Assert(ok, check.Equals, tt.outRegenerate)
+	}
+}
+
 func waitForStatus(diagAddr string, statusCodes ...int) error {
 	tickCh := time.Tick(250 * time.Millisecond)
 	timeoutCh := time.After(10 * time.Second)