connect/ca: cease including the common name field in generated certs

.changelog/10424.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+connect/ca: cease including the common name field in generated x509 non-CA certificates
+```

agent/agent_test.go

@@ -5118,10 +5118,9 @@ func TestAgent_AutoEncrypt(t *testing.T) {
 		Datacenter: "dc1",
 		Agent:      "test-client",
 	}
-	expectedCN := connect.AgentCN("test-client", connect.TestClusterID)
 	x509Cert, err := x509.ParseCertificate(aeCert.Certificate[0])
 	require.NoError(t, err)
-	require.Equal(t, expectedCN, x509Cert.Subject.CommonName)
+	require.Empty(t, x509Cert.Subject.CommonName)
 	require.Len(t, x509Cert.URIs, 1)
 	require.Equal(t, id.URI(), x509Cert.URIs[0])
 }

agent/auto-config/auto_encrypt_test.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"crypto/x509"
 	"crypto/x509/pkix"
-	"encoding/asn1"
 	"fmt"
 	"net"
 	"net/url"
@@ -45,16 +44,7 @@ func TestAutoEncrypt_generateCSR(t *testing.T) {
 				AutoEncryptTLS:   true,
 				AutoEncryptIPSAN: []net.IP{net.IPv4(198, 18, 0, 1), net.IPv4(198, 18, 0, 2)},
 			},
-			expectedSubject: pkix.Name{
-				CommonName: connect.AgentCN("test-node", unknownTrustDomain),
-				Names: []pkix.AttributeTypeAndValue{
-					{
-						// 2,5,4,3 is the CommonName type ASN1 identifier
-						Type:  asn1.ObjectIdentifier{2, 5, 4, 3},
-						Value: "testnode.agnt.unknown.consul",
-					},
-				},
-			},
+			expectedSubject:  pkix.Name{},
 			expectedSigAlg:   x509.ECDSAWithSHA256,
 			expectedPubAlg:   x509.ECDSA,
 			expectedDNSNames: defaultDNSSANs,
@@ -77,16 +67,7 @@ func TestAutoEncrypt_generateCSR(t *testing.T) {
 				AutoEncryptTLS:    true,
 				AutoEncryptDNSSAN: []string{"foo.local", "bar.local"},
 			},
-			expectedSubject: pkix.Name{
-				CommonName: connect.AgentCN("test-node", unknownTrustDomain),
-				Names: []pkix.AttributeTypeAndValue{
-					{
-						// 2,5,4,3 is the CommonName type ASN1 identifier
-						Type:  asn1.ObjectIdentifier{2, 5, 4, 3},
-						Value: "testnode.agnt.unknown.consul",
-					},
-				},
-			},
+			expectedSubject:  pkix.Name{},
 			expectedSigAlg:   x509.ECDSAWithSHA256,
 			expectedPubAlg:   x509.ECDSA,
 			expectedDNSNames: append(defaultDNSSANs, "foo.local", "bar.local"),

agent/auto-config/tls.go

@@ -245,11 +245,7 @@ func (ac *AutoConfig) generateCSR() (csr string, key string, err error) {
 	ipAddresses := ac.getIPSANs()
 
 	// Create a CSR.
-	//
-	// 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(ac.config.NodeName, unknownTrustDomain)
-	csr, err = connect.CreateCSR(id, cn, pk, dnsNames, ipAddresses)
+	csr, err = connect.CreateCSR(id, pk, dnsNames, ipAddresses)
 	if err != nil {
 		return "", "", err
 	}

agent/cache-types/connect_ca_leaf.go

@@ -518,7 +518,6 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest,
 
 	// Build the cert uri
 	var id connect.CertURI
-	var commonName string
 	var dnsNames []string
 	var ipAddresses []net.IP
 	if req.Service != "" {
@@ -528,15 +527,13 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest,
 			Namespace:  req.TargetNamespace(),
 			Service:    req.Service,
 		}
-		commonName = connect.ServiceCN(req.Service, req.TargetNamespace(), roots.TrustDomain)
 		dnsNames = append(dnsNames, req.DNSSAN...)
 	} else if req.Agent != "" {
 		id = &connect.SpiffeIDAgent{
 			Host:       roots.TrustDomain,
 			Datacenter: req.Datacenter,
 			Agent:      req.Agent,
 		}
-		commonName = connect.AgentCN(req.Agent, roots.TrustDomain)
 		dnsNames = append([]string{"localhost"}, req.DNSSAN...)
 		ipAddresses = append([]net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("::1")}, req.IPSAN...)
 	} else {
@@ -561,7 +558,7 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest,
 	}
 
 	// Create a CSR.
-	csr, err := connect.CreateCSR(id, commonName, pk, dnsNames, ipAddresses)
+	csr, err := connect.CreateCSR(id, pk, dnsNames, ipAddresses)
 	if err != nil {
 		return result, err
 	}

agent/connect/ca/provider_aws.go

@@ -14,10 +14,11 @@ import (
 	"github.com/aws/aws-sdk-go/service/acmpca"
 	"github.com/mitchellh/mapstructure"
 
+	"github.com/hashicorp/go-hclog"
+
 	"github.com/hashicorp/consul/agent/connect"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/logging"
-	"github.com/hashicorp/go-hclog"
 )
 
 const (
@@ -594,6 +595,8 @@ func (a *AWSProvider) GenerateIntermediate() (string, error) {
 
 // Sign implements Provider
 func (a *AWSProvider) Sign(csr *x509.CertificateRequest) (string, error) {
+	connect.HackSANExtensionForCSR(csr)
+
 	if a.rootPEM == "" {
 		return "", fmt.Errorf("AWS CA provider not fully Initialized")
 	}

agent/connect/ca/provider_consul.go

@@ -14,11 +14,12 @@ import (
 	"sync"
 	"time"
 
+	"github.com/hashicorp/go-hclog"
+
 	"github.com/hashicorp/consul/agent/connect"
 	"github.com/hashicorp/consul/agent/consul/state"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/logging"
-	"github.com/hashicorp/go-hclog"
 )
 
 var (
@@ -227,13 +228,7 @@ func (c *ConsulProvider) GenerateIntermediateCSR() (string, error) {
 		return "", err
 	}
 
-	uid, err := connect.CompactUID()
-	if err != nil {
-		return "", err
-	}
-	cn := connect.CACN("consul", uid, c.clusterID, c.isPrimary)
-
-	csr, err := connect.CreateCACSR(c.spiffeID, cn, signer)
+	csr, err := connect.CreateCACSR(c.spiffeID, signer)
 	if err != nil {
 		return "", err
 	}
@@ -329,6 +324,8 @@ func (c *ConsulProvider) Cleanup(_ bool, _ map[string]interface{}) error {
 // Sign returns a new certificate valid for the given SpiffeIDService
 // using the current CA.
 func (c *ConsulProvider) Sign(csr *x509.CertificateRequest) (string, error) {
+	connect.HackSANExtensionForCSR(csr)
+
 	// Lock during the signing so we don't use the same index twice
 	// for different cert serial numbers.
 	c.Lock()
@@ -362,20 +359,6 @@ func (c *ConsulProvider) Sign(csr *x509.CertificateRequest) (string, error) {
 		return "", err
 	}
 
-	// Parse the SPIFFE ID
-	spiffeId, err := connect.ParseCertURI(csr.URIs[0])
-	if err != nil {
-		return "", err
-	}
-
-	// 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.
-	subject, err := connect.CNForCertURI(spiffeId)
-	if err != nil {
-		return "", err
-	}
-
 	// Parse the CA cert
 	certPEM, err := c.ActiveIntermediate()
 	if err != nil {
@@ -400,7 +383,6 @@ func (c *ConsulProvider) Sign(csr *x509.CertificateRequest) (string, error) {
 	effectiveNow := time.Now().Add(-1 * time.Minute)
 	template := x509.Certificate{
 		SerialNumber: sn,
-		Subject:      pkix.Name{CommonName: subject},
 		URIs:         csr.URIs,
 		Signature:    csr.Signature,
 		// We use the correct signature algorithm for the CA key we are signing with
@@ -487,8 +469,12 @@ func (c *ConsulProvider) SignIntermediate(csr *x509.CertificateRequest) (string,
 	effectiveNow := time.Now().Add(-1 * CertificateTimeDriftBuffer)
 	template := x509.Certificate{
 		SerialNumber:          sn,
-		Subject:               csr.Subject,
+		DNSNames:              csr.DNSNames,
+		EmailAddresses:        csr.EmailAddresses,
+		IPAddresses:           csr.IPAddresses,
 		URIs:                  csr.URIs,
+		ExtraExtensions:       csr.ExtraExtensions,
+		Subject:               csr.Subject,
 		Signature:             csr.Signature,
 		SignatureAlgorithm:    connect.SigAlgoForKey(signer),
 		PublicKeyAlgorithm:    csr.PublicKeyAlgorithm,

agent/connect/ca/provider_consul_test.go

@@ -153,7 +153,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) {
 				parsed, err := connect.ParseCert(cert)
 				require.NoError(err)
 				require.Equal(spiffeService.URI(), parsed.URIs[0])
-				require.Equal(connect.ServiceCN("foo", "default", connect.TestClusterID), parsed.Subject.CommonName)
+				require.Empty(parsed.Subject.CommonName)
 				require.Equal(uint64(2), parsed.SerialNumber.Uint64())
 				subjectKeyID, err := connect.KeyId(csr.PublicKey)
 				require.NoError(err)
@@ -182,7 +182,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) {
 				parsed, err := connect.ParseCert(cert)
 				require.NoError(err)
 				require.Equal(spiffeService.URI(), parsed.URIs[0])
-				require.Equal(connect.ServiceCN("bar", "default", connect.TestClusterID), parsed.Subject.CommonName)
+				require.Empty(parsed.Subject.CommonName)
 				require.Equal(parsed.SerialNumber.Uint64(), uint64(2))
 				requireNotEncoded(t, parsed.SubjectKeyId)
 				requireNotEncoded(t, parsed.AuthorityKeyId)
@@ -210,7 +210,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) {
 				parsed, err := connect.ParseCert(cert)
 				require.NoError(err)
 				require.Equal(spiffeAgent.URI(), parsed.URIs[0])
-				require.Equal(connect.AgentCN("uuid", connect.TestClusterID), parsed.Subject.CommonName)
+				require.Empty(parsed.Subject.CommonName)
 				require.Equal(uint64(2), parsed.SerialNumber.Uint64())
 				requireNotEncoded(t, parsed.SubjectKeyId)
 				requireNotEncoded(t, parsed.AuthorityKeyId)

agent/connect/ca/provider_vault.go

@@ -412,6 +412,8 @@ func (v *VaultProvider) GenerateIntermediate() (string, error) {
 // a new leaf certificate based on the provided CSR, with the issuing
 // intermediate CA cert attached.
 func (v *VaultProvider) Sign(csr *x509.CertificateRequest) (string, error) {
+	connect.HackSANExtensionForCSR(csr)
+
 	var pemBuf bytes.Buffer
 	if err := pem.Encode(&pemBuf, &pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csr.Raw}); err != nil {
 		return "", err

agent/connect/common_names.go

@@ -11,102 +11,6 @@ import (
 
 var invalidDNSNameChars = regexp.MustCompile(`[^a-z0-9]`)
 
-const (
-	// 64 = max length of a certificate common name
-	// 21 = 7 bytes for ".consul", 9 bytes for .<trust domain> and 5 bytes for ".svc."
-	// This ends up being 43 bytes
-	maxServiceAndNamespaceLen = 64 - 21
-	minServiceNameLen         = 15
-	minNamespaceNameLen       = 15
-)
-
-// trucateServiceAndNamespace will take a service name and namespace name and truncate
-// them appropriately so that they would fit within the space alloted for them in the
-// Common Name field of the x509 certificate. That field is capped at 64 characters
-// in length and there is other data that must be a part of the name too. This function
-// takes all of that into account.
-func truncateServiceAndNamespace(serviceName, namespace string) (string, string) {
-	svcLen := len(serviceName)
-	nsLen := len(namespace)
-	totalLen := svcLen + nsLen
-
-	// quick exit when the entirety of both can fit
-	if totalLen <= maxServiceAndNamespaceLen {
-		return serviceName, namespace
-	}
-
-	toRemove := totalLen - maxServiceAndNamespaceLen
-	// now we must figure out how to truncate each one, we need to ensure we don't remove all of either one.
-	if svcLen <= minServiceNameLen {
-		// only remove bytes from the namespace
-		return serviceName, truncateTo(namespace, nsLen-toRemove)
-	} else if nsLen <= minNamespaceNameLen {
-		// only remove bytes from the service name
-		return truncateTo(serviceName, svcLen-toRemove), namespace
-	} else {
-		// we can remove an "equal" amount from each. If the number of bytes to remove is odd we give it to the namespace
-		svcTruncate := svcLen - (toRemove / 2) - (toRemove % 2)
-		nsTruncate := nsLen - (toRemove / 2)
-
-		// checks to ensure we don't reduce one side too much when they are not roughly balanced in length.
-		if svcTruncate <= minServiceNameLen {
-			svcTruncate = minServiceNameLen
-			nsTruncate = maxServiceAndNamespaceLen - minServiceNameLen
-		} else if nsTruncate <= minNamespaceNameLen {
-			svcTruncate = maxServiceAndNamespaceLen - minNamespaceNameLen
-			nsTruncate = minNamespaceNameLen
-		}
-
-		return truncateTo(serviceName, svcTruncate), truncateTo(namespace, nsTruncate)
-	}
-}
-
-// 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, namespace, trustDomain string) string {
-	svc := invalidDNSNameChars.ReplaceAllString(strings.ToLower(serviceName), "")
-
-	svc, namespace = truncateServiceAndNamespace(svc, namespace)
-	return fmt.Sprintf("%s.svc.%s.%s.consul",
-		svc, namespace, 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
@@ -129,8 +33,8 @@ func CompactUID() (string, error) {
 	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.
+// CACN returns the common name for a CA certificate.
+// 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 that CN and just return the current
@@ -163,22 +67,3 @@ func truncateTo(s string, n int) string {
 	}
 	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.Namespace, 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")
-	}
-}