Make all Connect Cert Common Names valid FQDNs (#6423)

agent/cache-types/connect_ca_leaf.go

@@ -507,6 +507,7 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest,
 
 	// Build the cert uri
 	var id connect.CertURI
+	var commonName string
 	if req.Service != "" {
 		id = &connect.SpiffeIDService{
 			Host:       roots.TrustDomain,
@@ -514,12 +515,14 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest,
 			Namespace:  "default",
 			Service:    req.Service,
 		}
+		commonName = connect.ServiceCN(req.Service, roots.TrustDomain)
 	} else if req.Agent != "" {
 		id = &connect.SpiffeIDAgent{
 			Host:       roots.TrustDomain,
 			Datacenter: req.Datacenter,
 			Agent:      req.Agent,
 		}
+		commonName = connect.ServiceCN(req.Agent, roots.TrustDomain)
 	} else {
 		return result, errors.New("URI must be either service or agent")
 	}
@@ -542,7 +545,7 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest,
 	}
 
 	// Create a CSR.
-	csr, err := connect.CreateCSR(id, pk)
+	csr, err := connect.CreateCSR(id, commonName, pk)
 	if err != nil {
 		return result, err
 	}

agent/connect/ca/provider_consul.go

@@ -188,7 +188,13 @@ func (c *ConsulProvider) GenerateIntermediateCSR() (string, error) {
 		return "", err
 	}
 
-	csr, err := connect.CreateCACSR(c.spiffeID, signer)
+	uid, err := connect.CompactUID()
+	if err != nil {
+		return "", err
+	}
+	cn := connect.CACN("consul", uid, c.clusterID, c.isRoot)
+
+	csr, err := connect.CreateCACSR(c.spiffeID, cn, signer)
 	if err != nil {
 		return "", err
 	}
@@ -313,14 +319,12 @@ func (c *ConsulProvider) Sign(csr *x509.CertificateRequest) (string, error) {
 		return "", err
 	}
 
-	subject := ""
+	// Even though leafs should be from our own CSRs which should have the same CN
-	switch id := spiffeId.(type) {
+	// logic as here, override anyway to account for older version clients that
-	case *connect.SpiffeIDService:
+	// didn't include the Common Name in the CSR.
-		subject = id.Service
+	subject, err := connect.CNForCertURI(spiffeId)
-	case *connect.SpiffeIDAgent:
+	if err != nil {
-		subject = id.Agent
+		return "", err
-	default:
-		return "", fmt.Errorf("SPIFFE ID in CSR must be a service ID")
 	}
 
 	// Parse the CA cert
@@ -583,8 +587,6 @@ func (c *ConsulProvider) generateCA(privateKey string, sn uint64) (string, error
 		return "", fmt.Errorf("error parsing private key %q: %s", privateKey, err)
 	}
 
-	name := fmt.Sprintf("Consul CA %d", sn)
-
 	// The URI (SPIFFE compatible) for the cert
 	id := connect.SpiffeIDSigningForCluster(config)
 	keyId, err := connect.KeyId(privKey.Public())
@@ -593,11 +595,16 @@ func (c *ConsulProvider) generateCA(privateKey string, sn uint64) (string, error
 	}
 
 	// Create the CA cert
+	uid, err := connect.CompactUID()
+	if err != nil {
+		return "", err
+	}
+	cn := connect.CACN("consul", uid, c.clusterID, c.isRoot)
 	serialNum := &big.Int{}
 	serialNum.SetUint64(sn)
 	template := x509.Certificate{
 		SerialNumber:          serialNum,
-		Subject:               pkix.Name{CommonName: name},
+		Subject:               pkix.Name{CommonName: cn},
 		URIs:                  []*url.URL{id.URI()},
 		BasicConstraintsValid: true,
 		KeyUsage: x509.KeyUsageCertSign |

agent/connect/ca/provider_consul_test.go

@@ -143,7 +143,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) {
 			require.NoError(provider.GenerateRoot())
 
 			spiffeService := &connect.SpiffeIDService{
-				Host:       "node1",
+				Host:       connect.TestClusterID + ".consul",
 				Namespace:  "default",
 				Datacenter: "dc1",
 				Service:    "foo",
@@ -161,8 +161,8 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) {
 
 				parsed, err := connect.ParseCert(cert)
 				require.NoError(err)
-				require.Equal(parsed.URIs[0], spiffeService.URI())
+				require.Equal(spiffeService.URI(), parsed.URIs[0])
-				require.Equal(parsed.Subject.CommonName, "foo")
+				require.Equal(connect.ServiceCN("foo", connect.TestClusterID), parsed.Subject.CommonName)
 				require.Equal(uint64(2), parsed.SerialNumber.Uint64())
 				requireNotEncoded(t, parsed.SubjectKeyId)
 				requireNotEncoded(t, parsed.AuthorityKeyId)
@@ -187,8 +187,8 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) {
 
 				parsed, err := connect.ParseCert(cert)
 				require.NoError(err)
-				require.Equal(parsed.URIs[0], spiffeService.URI())
+				require.Equal(spiffeService.URI(), parsed.URIs[0])
-				require.Equal(parsed.Subject.CommonName, "bar")
+				require.Equal(connect.ServiceCN("bar", connect.TestClusterID), parsed.Subject.CommonName)
 				require.Equal(parsed.SerialNumber.Uint64(), uint64(2))
 				requireNotEncoded(t, parsed.SubjectKeyId)
 				requireNotEncoded(t, parsed.AuthorityKeyId)
@@ -199,7 +199,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) {
 			}
 
 			spiffeAgent := &connect.SpiffeIDAgent{
-				Host:       "node1",
+				Host:       connect.TestClusterID + ".consul",
 				Datacenter: "dc1",
 				Agent:      "uuid",
 			}
@@ -216,7 +216,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) {
 				parsed, err := connect.ParseCert(cert)
 				require.NoError(err)
 				require.Equal(spiffeAgent.URI(), parsed.URIs[0])
-				require.Equal("uuid", parsed.Subject.CommonName)
+				require.Equal(connect.AgentCN("uuid", connect.TestClusterID), parsed.Subject.CommonName)
 				require.Equal(uint64(2), parsed.SerialNumber.Uint64())
 				requireNotEncoded(t, parsed.SubjectKeyId)
 				requireNotEncoded(t, parsed.AuthorityKeyId)
@@ -297,7 +297,11 @@ func testCrossSignProviders(t *testing.T, provider1, provider2 Provider) {
 	requireNotEncoded(t, oldRoot.AuthorityKeyId)
 
 	// AuthorityKeyID should now be the signing root's, SubjectKeyId should be kept.
-	require.Equal(oldRoot.AuthorityKeyId, xc.AuthorityKeyId)
+	require.Equal(oldRoot.SubjectKeyId, xc.AuthorityKeyId,
+		"newSKID=%x\nnewAKID=%x\noldSKID=%x\noldAKID=%x\nxcSKID=%x\nxcAKID=%x",
+		newRoot.SubjectKeyId, newRoot.AuthorityKeyId,
+		oldRoot.SubjectKeyId, oldRoot.AuthorityKeyId,
+		xc.SubjectKeyId, xc.AuthorityKeyId)
 	require.Equal(newRoot.SubjectKeyId, xc.SubjectKeyId)
 
 	// Subject name should not have changed.
@@ -308,7 +312,7 @@ func testCrossSignProviders(t *testing.T, provider1, provider2 Provider) {
 
 	// Get a leaf cert so we can verify against the cross-signed cert.
 	spiffeService := &connect.SpiffeIDService{
-		Host:       "node1",
+		Host:       connect.TestClusterID + ".consul",
 		Namespace:  "default",
 		Datacenter: "dc1",
 		Service:    "foo",
@@ -396,7 +400,7 @@ func testSignIntermediateCrossDC(t *testing.T, provider1, provider2 Provider) {
 
 	// Have provider2 sign a leaf cert and make sure the chain is correct.
 	spiffeService := &connect.SpiffeIDService{
-		Host:       "node1",
+		Host:       connect.TestClusterID + ".consul",
 		Namespace:  "default",
 		Datacenter: "dc1",
 		Service:    "foo",

agent/connect/ca/provider_vault.go

@@ -11,7 +11,6 @@ import (
 
 	"github.com/hashicorp/consul/agent/connect"
 	"github.com/hashicorp/consul/agent/structs"
-	"github.com/hashicorp/go-uuid"
 	vaultapi "github.com/hashicorp/vault/api"
 	"github.com/mitchellh/mapstructure"
 )
@@ -25,7 +24,7 @@ type VaultProvider struct {
 	config    *structs.VaultCAProviderConfig
 	client    *vaultapi.Client
 	isRoot    bool
-	clusterId string
+	clusterID string
 	spiffeID  *connect.SpiffeIDSigning
 }
 
@@ -41,7 +40,7 @@ func vaultTLSConfig(config *structs.VaultCAProviderConfig) *vaultapi.TLSConfig {
 }
 
 // Configure sets up the provider using the given configuration.
-func (v *VaultProvider) Configure(clusterId string, isRoot bool, rawConfig map[string]interface{}) error {
+func (v *VaultProvider) Configure(clusterID string, isRoot bool, rawConfig map[string]interface{}) error {
 	config, err := ParseVaultCAConfig(rawConfig)
 	if err != nil {
 		return err
@@ -63,8 +62,8 @@ func (v *VaultProvider) Configure(clusterId string, isRoot bool, rawConfig map[s
 	v.config = config
 	v.client = client
 	v.isRoot = isRoot
-	v.clusterId = clusterId
+	v.clusterID = clusterID
-	v.spiffeID = connect.SpiffeIDSigningForCluster(&structs.CAConfiguration{ClusterID: clusterId})
+	v.spiffeID = connect.SpiffeIDSigningForCluster(&structs.CAConfiguration{ClusterID: clusterID})
 
 	return nil
 }
@@ -98,12 +97,12 @@ func (v *VaultProvider) GenerateRoot() error {
 
 		fallthrough
 	case ErrBackendNotInitialized:
-		uuid, err := uuid.GenerateUUID()
+		uid, err := connect.CompactUID()
 		if err != nil {
 			return err
 		}
 		_, err = v.client.Logical().Write(v.config.RootPKIPath+"root/generate/internal", map[string]interface{}{
-			"common_name": fmt.Sprintf("Vault CA Root Authority %s", uuid),
+			"common_name": connect.CACN("vault", uid, v.clusterID, v.isRoot),
 			"uri_sans":    v.spiffeID.URI().String(),
 			"key_type":    v.config.PrivateKeyType,
 			"key_bits":    v.config.PrivateKeyBits,
@@ -174,8 +173,12 @@ func (v *VaultProvider) generateIntermediateCSR() (string, error) {
 	}
 
 	// Generate a new intermediate CSR for the root to sign.
+	uid, err := connect.CompactUID()
+	if err != nil {
+		return "", err
+	}
 	data, err := v.client.Logical().Write(v.config.IntermediatePKIPath+"intermediate/generate/internal", map[string]interface{}{
-		"common_name": "Vault CA Intermediate Authority",
+		"common_name": connect.CACN("vault", uid, v.clusterID, v.isRoot),
 		"key_type":    v.config.PrivateKeyType,
 		"key_bits":    v.config.PrivateKeyBits,
 		"uri_sans":    v.spiffeID.URI().String(),

agent/connect/ca/provider_vault_test.go

@@ -80,7 +80,7 @@ func TestVaultCAProvider_Bootstrap(t *testing.T) {
 		require.NoError(err)
 		require.True(parsed.IsCA)
 		require.Len(parsed.URIs, 1)
-		require.Equal(parsed.URIs[0].String(), fmt.Sprintf("spiffe://%s.consul", provider.clusterId))
+		require.Equal(fmt.Sprintf("spiffe://%s.consul", provider.clusterID), parsed.URIs[0].String())
 	}
 }
 
@@ -349,7 +349,6 @@ func testVaultProviderWithConfig(t *testing.T, isRoot bool, rawConf map[string]i
 			t.Fatalf("err: %v", err)
 		}
 	}
-
 	return provider, testVault
 }
 

agent/connect/common_names.go

@@ -0,0 +1,133 @@
+package connect
+
+import (
+	"crypto/rand"
+	"encoding/binary"
+	"fmt"
+	"regexp"
+	"strconv"
+	"strings"
+)
+
+var invalidDNSNameChars = regexp.MustCompile(`[^a-z0-9]`)
+
+// ServiceCN returns the common name for a service's certificate. We can't use
+// SPIFFE URIs because some CAs require valid FQDN format. We can't use SNI
+// values because they are often too long than the 64 bytes allowed by
+// CommonNames. We could attempt to encode more information into this to make
+// identifying which instance/node it was issued to in a management tool easier
+// but that just introduces more complications around length. It's also strange
+// that the Common Name would encode more information than the actual
+// identifying URI we use to assert anything does and my lead to bad assumptions
+// that the common name is in some way "secure" or verified - there is nothing
+// inherently provable here except that the requestor had ACLs for that service
+// name in that DC.
+//
+// Format is:
+//   <sanitized_service_name>.svc.<trust-domain-first-8>.consul
+//
+//   service name is sanitized by removing any chars that are not legal in a DNS
+//   name and lower casing. It is truncated to the first X chars to keep the
+//   total at 64.
+//
+//   trust domain is truncated to keep the whole name short
+func ServiceCN(serviceName, trustDomain string) string {
+	svc := invalidDNSNameChars.ReplaceAllString(strings.ToLower(serviceName), "")
+	// 20 = 7 bytes for ".consul", 8 bytes for trust domain, 5 bytes for ".svc."
+	return fmt.Sprintf("%s.svc.%s.consul",
+		truncateTo(svc, 64-20), truncateTo(trustDomain, 8))
+}
+
+// AgentCN returns the common name for an agent certificate. See ServiceCN for
+// more details on rationale.
+//
+// Format is:
+//   <sanitized_node_name>.agnt.<trust-domain-first-8>.consul
+//
+//   node name is sanitized by removing any chars that are not legal in a DNS
+//   name and lower casing. It is truncated to the first X chars to keep the
+//   total at 64.
+//
+//   trust domain is truncated to keep the whole name short
+func AgentCN(node, trustDomain string) string {
+	nodeSan := invalidDNSNameChars.ReplaceAllString(strings.ToLower(node), "")
+	// 21 = 7 bytes for ".consul", 8 bytes for trust domain, 6 bytes for ".agnt."
+	return fmt.Sprintf("%s.agnt.%s.consul",
+		truncateTo(nodeSan, 64-21), truncateTo(trustDomain, 8))
+}
+
+// CompactUID returns a crypto random Unique Identifier string consiting of 8
+// characters of base36 encoded random value. This has roughly 41 bits of
+// entropy so is suitable for infrequently occuring events with low probability
+// of collision. It is not suitable for UUIDs for very frequent events. It's
+// main purpose is to assign unique values to CA certificate Common Names which
+// need to be unique in some providers - see CACN - but without using up large
+// amounts of the limited 64 character Common Name. It also makes the values
+// more easily digestable by humans considering there are likely to be few of
+// them ever in use.
+func CompactUID() (string, error) {
+	// 48 bits (6 bytes) is enough to fill 8 bytes in base36 but it's simpler to
+	// have a whole uint8 to convert from.
+	var raw [8]byte
+	_, err := rand.Read(raw[:])
+	if err != nil {
+		return "", err
+	}
+
+	i := binary.LittleEndian.Uint64(raw[:])
+	return truncateTo(strconv.FormatInt(int64(i), 36), 8), nil
+}
+
+// CACN returns the common name for a CA certificate. See ServiceCN for more
+// details on rationale. A uniqueID is requires because some providers (e.g.
+// Vault) cache by subject and so produce incorrect results - for example they
+// won't cross-sign an older CA certificate with the same common name since they
+// think they already have a valid cert for than CN and just return the current
+// root.
+//
+// This can be generated by any means but will be truncated to 8 chars and
+// sanitised to DNS-safe chars. CompactUID generates suitable UIDs for this
+// specific purpose.
+//
+// Format is:
+//   {provider}-{uniqueID-8-b36}.{root|int}.ca.<trust-domain-first-8>.consul
+//
+//   trust domain is truncated to keep the whole name short
+func CACN(provider, uniqueID, trustDomain string, isRoot bool) string {
+	providerSan := invalidDNSNameChars.ReplaceAllString(strings.ToLower(provider), "")
+	typ := "root"
+	if !isRoot {
+		typ = "int"
+	}
+	// 33 = 7 bytes for ".consul", 8 bytes for trust domain, 9 bytes for
+	// ".root.ca.", 9 bytes for "-{uniqueID-8-b36}"
+	uidSAN := invalidDNSNameChars.ReplaceAllString(strings.ToLower(uniqueID), "")
+	return fmt.Sprintf("%s-%s.%s.ca.%s.consul", typ, truncateTo(uidSAN, 8),
+		truncateTo(providerSan, 64-33), truncateTo(trustDomain, 8))
+}
+
+func truncateTo(s string, n int) string {
+	if len(s) > n {
+		return s[:n]
+	}
+	return s
+}
+
+// CNForCertURI returns the correct common name for a given cert URI type. It
+// doesn't work for CA Signing IDs since more context is needed and CA Providers
+// always know their CN from their own context.
+func CNForCertURI(uri CertURI) (string, error) {
+	// Even though leafs should be from our own CSRs which should have the same CN
+	// logic as here, override anyway to account for older version clients that
+	// didn't include the Common Name in the CSR.
+	switch id := uri.(type) {
+	case *SpiffeIDService:
+		return ServiceCN(id.Service, id.Host), nil
+	case *SpiffeIDAgent:
+		return AgentCN(id.Agent, id.Host), nil
+	case *SpiffeIDSigning:
+		return "", fmt.Errorf("CertURI is a SpiffeIDSigning, not enough context to generate Common Name")
+	default:
+		return "", fmt.Errorf("CertURI type not recognized")
+	}
+}

agent/connect/csr.go

@@ -43,11 +43,13 @@ func SigAlgoForKeyType(keyType string) x509.SignatureAlgorithm {
 
 // CreateCSR returns a CSR to sign the given service along with the PEM-encoded
 // private key for this certificate.
-func CreateCSR(uri CertURI, privateKey crypto.Signer, extensions ...pkix.Extension) (string, error) {
+func CreateCSR(uri CertURI, commonName string, privateKey crypto.Signer,
+	extensions ...pkix.Extension) (string, error) {
 	template := &x509.CertificateRequest{
 		URIs:               []*url.URL{uri.URI()},
 		SignatureAlgorithm: SigAlgoForKey(privateKey),
 		ExtraExtensions:    extensions,
+		Subject:            pkix.Name{CommonName: commonName},
 	}
 
 	// Create the CSR itself
@@ -67,13 +69,13 @@ func CreateCSR(uri CertURI, privateKey crypto.Signer, extensions ...pkix.Extensi
 
 // CreateCSR returns a CA CSR to sign the given service along with the PEM-encoded
 // private key for this certificate.
-func CreateCACSR(uri CertURI, privateKey crypto.Signer) (string, error) {
+func CreateCACSR(uri CertURI, commonName string, privateKey crypto.Signer) (string, error) {
 	ext, err := CreateCAExtension()
 	if err != nil {
 		return "", err
 	}
 
-	return CreateCSR(uri, privateKey, ext)
+	return CreateCSR(uri, commonName, privateKey, ext)
 }
 
 // CreateCAExtension creates a pkix.Extension for the x509 Basic Constraints

agent/connect/testing_ca.go

@@ -211,7 +211,7 @@ func testLeaf(t testing.T, service string, root *structs.CARoot, keyType string,
 	// Cert template for generation
 	template := x509.Certificate{
 		SerialNumber:          sn,
-		Subject:               pkix.Name{CommonName: service},
+		Subject:               pkix.Name{CommonName: ServiceCN(service, TestClusterID)},
 		URIs:                  []*url.URL{spiffeId.URI()},
 		SignatureAlgorithm:    SigAlgoForKeyType(rootKeyType),
 		BasicConstraintsValid: true,
@@ -262,7 +262,12 @@ func TestLeaf(t testing.T, service string, root *structs.CARoot) (string, string
 // TestCSR returns a CSR to sign the given service along with the PEM-encoded
 // private key for this certificate.
 func TestCSR(t testing.T, uri CertURI) (string, string) {
+	cn, err := CNForCertURI(uri)
+	if err != nil {
+		t.Fatalf("TestCSR failed to get Common Name: %s", err)
+	}
 	template := &x509.CertificateRequest{
+		Subject:            pkix.Name{CommonName: cn},
 		URIs:               []*url.URL{uri.URI()},
 		SignatureAlgorithm: x509.ECDSAWithSHA256,
 	}

agent/consul/auto_encrypt.go

@@ -65,7 +65,11 @@ func (c *Client) RequestAutoEncryptCerts(servers []string, port int, token strin
 	}
 
 	// Create a CSR.
-	csr, err := connect.CreateCSR(id, pk)
+	//
+	// The Common Name includes the dummy trust domain for now but Server will
+	// override this when it is signed anyway so it's OK.
+	cn := connect.AgentCN(string(c.config.NodeName), dummyTrustDomain)
+	csr, err := connect.CreateCSR(id, cn, pk)
 	if err != nil {
 		return errFn(err)
 	}

agent/consul/auto_encrypt_endpoint_test.go

@@ -75,7 +75,9 @@ func TestAutoEncryptSign(t *testing.T) {
 			require.NoError(t, err, info)
 
 			// Create a CSR.
-			csr, err := connect.CreateCSR(id, pk)
+			cn, err := connect.CNForCertURI(id)
+			require.NoError(t, err)
+			csr, err := connect.CreateCSR(id, cn, pk)
 			require.NoError(t, err, info)
 			require.NotEmpty(t, csr, info)
 			args := &structs.CASignRequest{