connect/ca: cease including the common name field in generated certs (#10424)

As part of this change, we ensure that the SAN extensions are marked as
critical when the subject is empty so that AWS PCA tolerates the loss of
common names well and continues to function as a Connect CA provider.

Parts of this currently hack around a bug in crypto/x509 and can be
removed after https://go-review.googlesource.com/c/go/+/329129 lands in
a Go release.

Note: the AWS PCA tests do not run automatically, but the following
passed locally for me:

    ENABLE_AWS_PCA_TESTS=1 go test ./agent/connect/ca -run TestAWS

.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{
+			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",
-					},
-				},
-			},
 			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{
+			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",
-					},
-				},
-			},
 			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.
-	//
+	csr, err = connect.CreateCSR(id, pk, dnsNames, ipAddresses)
-	// 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)
 	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,7 +527,6 @@ 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{
@@ -536,7 +534,6 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest,
 			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()
+	csr, err := connect.CreateCACSR(c.spiffeID, signer)
-	if err != nil {
-		return "", err
-	}
-	cn := connect.CACN("consul", uid, c.clusterID, c.isPrimary)
-
-	csr, err := connect.CreateCACSR(c.spiffeID, cn, 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
+// CACN returns the common name for a CA certificate.
-// details on rationale. A uniqueID is requires because some providers (e.g.
+// 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")
-	}
-}

agent/connect/common_names_test.go

@@ -1,115 +0,0 @@
-package connect
-
-import (
-	"strings"
-	"testing"
-
-	"github.com/stretchr/testify/require"
-)
-
-func TestServiceAndNamespaceTruncation(t *testing.T) {
-	type tcase struct {
-		service   string
-		namespace string
-		// if left as "" then its expected to not be truncated
-		expectedService string
-		// if left as "" then its expected to not be truncated
-		expectedNamespace string
-	}
-
-	cases := map[string]tcase{
-		"short-no-truncation": {
-			service:   "foo",
-			namespace: "bar",
-		},
-		"long-service-no-truncation": {
-			// -3 because thats the length of the namespace
-			service:   strings.Repeat("a", maxServiceAndNamespaceLen-3),
-			namespace: "bar",
-		},
-		"long-namespace-no-truncation": {
-			service: "foo",
-			// -3 because thats the length of the service name
-			namespace: strings.Repeat("b", maxServiceAndNamespaceLen-3),
-		},
-		"truncate-service-only": {
-			// this should force the service name to be truncated
-			service:         strings.Repeat("a", maxServiceAndNamespaceLen-minNamespaceNameLen+5),
-			expectedService: strings.Repeat("a", maxServiceAndNamespaceLen-minNamespaceNameLen),
-			// this is the maximum length that will never be truncated for a namespace
-			namespace: strings.Repeat("b", minNamespaceNameLen),
-		},
-		"truncate-namespace-only": {
-			// this is the maximum length that will never be truncated for a service name
-			service: strings.Repeat("a", minServiceNameLen),
-			// this should force the namespace name to be truncated
-			namespace:         strings.Repeat("b", maxServiceAndNamespaceLen-minServiceNameLen+5),
-			expectedNamespace: strings.Repeat("b", maxServiceAndNamespaceLen-minServiceNameLen),
-		},
-		"truncate-both-even": {
-			// this test would need to be update if the maxServiceAndNamespaceLen variable is updated
-			// I could put some more complex logic into here to prevent that but it would be mostly
-			// duplicating the logic in the function itself and thus not really be testing anything
-			//
-			// The original lengths of 50 / 51 were chose when maxServiceAndNamespaceLen would be 43
-			// Therefore a total of 58 characters must be removed. This was chose so that the value
-			// could be evenly split between the two strings.
-			service:           strings.Repeat("a", 50),
-			expectedService:   strings.Repeat("a", 21),
-			namespace:         strings.Repeat("b", 51),
-			expectedNamespace: strings.Repeat("b", 22),
-		},
-		"truncate-both-odd": {
-			// this test would need to be update if the maxServiceAndNamespaceLen variable is updated
-			// I could put some more complex logic into here to prevent that but it would be mostly
-			// duplicating the logic in the function itself and thus not really be testing anything
-			//
-			// The original lengths of 50 / 57 were chose when maxServiceAndNamespaceLen would be 43
-			// Therefore a total of 57 characters must be removed. This was chose so that the value
-			// could not be evenly removed from both so the namespace should be truncated to a length
-			// 1 character more than the service.
-			service:           strings.Repeat("a", 50),
-			expectedService:   strings.Repeat("a", 21),
-			namespace:         strings.Repeat("b", 50),
-			expectedNamespace: strings.Repeat("b", 22),
-		},
-		"truncate-both-min-svc": {
-			service:           strings.Repeat("a", minServiceNameLen+1),
-			expectedService:   strings.Repeat("a", minServiceNameLen),
-			namespace:         strings.Repeat("b", maxServiceAndNamespaceLen),
-			expectedNamespace: strings.Repeat("b", maxServiceAndNamespaceLen-minServiceNameLen),
-		},
-		"truncate-both-min-ns": {
-			service:           strings.Repeat("a", maxServiceAndNamespaceLen),
-			expectedService:   strings.Repeat("a", maxServiceAndNamespaceLen-minNamespaceNameLen),
-			namespace:         strings.Repeat("b", minNamespaceNameLen+1),
-			expectedNamespace: strings.Repeat("b", minNamespaceNameLen),
-		},
-	}
-
-	for name, tc := range cases {
-		t.Run(name, func(t *testing.T) {
-			actualSvc, actualNamespace := truncateServiceAndNamespace(tc.service, tc.namespace)
-
-			expectedLen := len(tc.service) + len(tc.namespace)
-			if tc.expectedService != "" || tc.expectedNamespace != "" {
-				expectedLen = maxServiceAndNamespaceLen
-			}
-
-			actualLen := len(actualSvc) + len(actualNamespace)
-
-			require.Equal(t, expectedLen, actualLen, "Combined length of %d (svc: %d, ns: %d) does not match expected value of %d", actualLen, len(actualSvc), len(actualNamespace), expectedLen)
-
-			if tc.expectedService != "" {
-				require.Equal(t, tc.expectedService, actualSvc)
-			} else {
-				require.Equal(t, tc.service, actualSvc)
-			}
-			if tc.expectedNamespace != "" {
-				require.Equal(t, tc.expectedNamespace, actualNamespace)
-			} else {
-				require.Equal(t, tc.namespace, actualNamespace)
-			}
-		})
-	}
-}

agent/connect/csr.go

@@ -44,16 +44,16 @@ func SigAlgoForKeyType(keyType string) x509.SignatureAlgorithm {
 
 // CreateCSR returns a CSR to sign the given service with SAN entries
 // along with the PEM-encoded private key for this certificate.
-func CreateCSR(uri CertURI, commonName string, privateKey crypto.Signer,
+func CreateCSR(uri CertURI, privateKey crypto.Signer,
 	dnsNames []string, ipAddresses []net.IP, extensions ...pkix.Extension) (string, error) {
 	template := &x509.CertificateRequest{
 		URIs:               []*url.URL{uri.URI()},
 		SignatureAlgorithm: SigAlgoForKey(privateKey),
 		ExtraExtensions:    extensions,
-		Subject:            pkix.Name{CommonName: commonName},
 		DNSNames:           dnsNames,
 		IPAddresses:        ipAddresses,
 	}
+	HackSANExtensionForCSR(template)
 
 	// Create the CSR itself
 	var csrBuf bytes.Buffer
@@ -72,13 +72,13 @@ func CreateCSR(uri CertURI, commonName string, privateKey crypto.Signer,
 
 // CreateCSR returns a CA CSR to sign the given service along with the PEM-encoded
 // private key for this certificate.
-func CreateCACSR(uri CertURI, commonName string, privateKey crypto.Signer) (string, error) {
+func CreateCACSR(uri CertURI, privateKey crypto.Signer) (string, error) {
 	ext, err := CreateCAExtension()
 	if err != nil {
 		return "", err
 	}
 
-	return CreateCSR(uri, commonName, privateKey, nil, nil, ext)
+	return CreateCSR(uri, privateKey, nil, nil, ext)
 }
 
 // CreateCAExtension creates a pkix.Extension for the x509 Basic Constraints

agent/connect/testing_ca.go

@@ -13,9 +13,10 @@ import (
 	"sync/atomic"
 	"time"
 
-	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/go-uuid"
 	"github.com/mitchellh/go-testing-interface"
+
+	"github.com/hashicorp/consul/agent/structs"
 )
 
 // TestClusterID is the Consul cluster ID for testing.
@@ -182,16 +183,7 @@ func TestCAWithKeyType(t testing.T, xc *structs.CARoot, keyType string, keyBits
 	return testCA(t, xc, keyType, keyBits, 0)
 }
 
-// testCertID is an interface to be implemented the various spiffe ID / CertURI types
+func testLeafWithID(t testing.T, spiffeId CertURI, root *structs.CARoot, keyType string, keyBits int, expiration time.Duration) (string, string, error) {
-// It adds an addition CommonName method to the CertURI interface to prevent the need
-// for any type switching on the actual CertURI's concrete type in order to figure
-// out its common name
-type testCertID interface {
-	CommonName() string
-	CertURI
-}
-
-func testLeafWithID(t testing.T, spiffeId testCertID, root *structs.CARoot, keyType string, keyBits int, expiration time.Duration) (string, string, error) {
 
 	if expiration == 0 {
 		// this is 10 years
@@ -231,7 +223,6 @@ func testLeafWithID(t testing.T, spiffeId testCertID, root *structs.CARoot, keyT
 	// Cert template for generation
 	template := x509.Certificate{
 		SerialNumber:          sn,
-		Subject:               pkix.Name{CommonName: spiffeId.CommonName()},
 		URIs:                  []*url.URL{spiffeId.URI()},
 		SignatureAlgorithm:    SigAlgoForKeyType(rootKeyType),
 		BasicConstraintsValid: true,
@@ -309,16 +300,13 @@ func TestLeafWithNamespace(t testing.T, service, namespace string, root *structs
 // 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,
 	}
 
+	HackSANExtensionForCSR(template)
+
 	// Create the private key we'll use
 	signer, pkPEM := testPrivateKey(t, DefaultPrivateKeyType, DefaultPrivateKeyBits)
 

agent/connect/uri_agent.go

@@ -20,7 +20,3 @@ func (id *SpiffeIDAgent) URI() *url.URL {
 	result.Path = fmt.Sprintf("/agent/client/dc/%s/id/%s", id.Datacenter, id.Agent)
 	return &result
 }
-
-func (id *SpiffeIDAgent) CommonName() string {
-	return AgentCN(id.Agent, id.Host)
-}

agent/connect/uri_service.go

@@ -22,7 +22,3 @@ func (id *SpiffeIDService) URI() *url.URL {
 		id.Namespace, id.Datacenter, id.Service)
 	return &result
 }
-
-func (id *SpiffeIDService) CommonName() string {
-	return ServiceCN(id.Service, id.Namespace, id.Host)
-}

agent/connect/x509_patch.go

@@ -0,0 +1,126 @@
+package connect
+
+import (
+	"crypto/x509"
+	"crypto/x509/pkix"
+	"encoding/asn1"
+	"fmt"
+	"net"
+	"net/url"
+	"unicode"
+)
+
+// NOTE: the contents of this file were lifted from
+// $GOROOT/src/crypto/x509/x509.go from a Go 1.16.5 checkout.
+//
+//
+// After https://go-review.googlesource.com/c/go/+/329129 lands in a Go release
+// we are compiling against we can safely remove all of this code.
+
+var (
+	x509_oidExtensionSubjectAltName = []int{2, 5, 29, 17}
+)
+
+const (
+	x509_nameTypeEmail = 1
+	x509_nameTypeDNS   = 2
+	x509_nameTypeURI   = 6
+	x509_nameTypeIP    = 7
+)
+
+// HackSANExtensionForCSR will create a SAN extension on the CSR off of the
+// convenience fields (DNSNames, EmailAddresses, IPAddresses, URIs) and
+// appropriately marks that SAN extension as critical if the CSR has an empty
+// subject.
+//
+// This is basically attempting to repeat this blob of code from the stdlib
+// ourselves:
+//
+// https://github.com/golang/go/blob/0e67ce3d28320e816dd8e7cf7d701c1804fb977e/src/crypto/x509/x509.go#L1088
+func HackSANExtensionForCSR(template *x509.CertificateRequest) {
+	switch {
+	case len(template.DNSNames) > 0:
+	case len(template.EmailAddresses) > 0:
+	case len(template.IPAddresses) > 0:
+	case len(template.URIs) > 0:
+	default:
+		return
+	}
+
+	if x509_oidInExtensions(x509_oidExtensionSubjectAltName, template.ExtraExtensions) {
+		return
+	}
+
+	value, err := x509_marshalSANs(template.DNSNames, template.EmailAddresses, template.IPAddresses, template.URIs)
+	if err != nil {
+		return
+	}
+
+	ext := pkix.Extension{
+		Id: x509_oidExtensionSubjectAltName,
+		// From RFC 5280, Section 4.2.1.6:
+		// “If the subject field contains an empty sequence ... then
+		// subjectAltName extension ... is marked as critical”
+		//
+		// Since we just cleared the subject above, it's critical.
+		Critical: true,
+		Value:    value,
+	}
+	template.ExtraExtensions = append(template.ExtraExtensions, ext)
+}
+
+// x509_oidInExtensions reports whether an extension with the given oid exists in
+// extensions.
+func x509_oidInExtensions(oid asn1.ObjectIdentifier, extensions []pkix.Extension) bool {
+	for _, e := range extensions {
+		if e.Id.Equal(oid) {
+			return true
+		}
+	}
+	return false
+}
+
+// x509_marshalSANs marshals a list of addresses into a the contents of an X.509
+// SubjectAlternativeName extension.
+func x509_marshalSANs(dnsNames, emailAddresses []string, ipAddresses []net.IP, uris []*url.URL) (derBytes []byte, err error) {
+	var rawValues []asn1.RawValue
+	for _, name := range dnsNames {
+		if err := x509_isIA5String(name); err != nil {
+			return nil, err
+		}
+		rawValues = append(rawValues, asn1.RawValue{Tag: x509_nameTypeDNS, Class: 2, Bytes: []byte(name)})
+	}
+	for _, email := range emailAddresses {
+		if err := x509_isIA5String(email); err != nil {
+			return nil, err
+		}
+		rawValues = append(rawValues, asn1.RawValue{Tag: x509_nameTypeEmail, Class: 2, Bytes: []byte(email)})
+	}
+	for _, rawIP := range ipAddresses {
+		// If possible, we always want to encode IPv4 addresses in 4 bytes.
+		ip := rawIP.To4()
+		if ip == nil {
+			ip = rawIP
+		}
+		rawValues = append(rawValues, asn1.RawValue{Tag: x509_nameTypeIP, Class: 2, Bytes: ip})
+	}
+	for _, uri := range uris {
+		uriStr := uri.String()
+		if err := x509_isIA5String(uriStr); err != nil {
+			return nil, err
+		}
+		rawValues = append(rawValues, asn1.RawValue{Tag: x509_nameTypeURI, Class: 2, Bytes: []byte(uriStr)})
+	}
+	return asn1.Marshal(rawValues)
+}
+
+func x509_isIA5String(s string) error {
+	for _, r := range s {
+		// Per RFC5280 "IA5String is limited to the set of ASCII characters"
+		if r > unicode.MaxASCII {
+			return fmt.Errorf("x509: %q cannot be encoded as an IA5String", s)
+		}
+	}
+
+	return nil
+}

agent/connect/x509_patch_test.go

@@ -0,0 +1,126 @@
+package connect
+
+import (
+	"crypto/rand"
+	"crypto/rsa"
+	"crypto/x509"
+	"encoding/pem"
+	"math/big"
+	"testing"
+)
+
+var testPrivateKey_x509 *rsa.PrivateKey
+
+func TestX509_EmptySubject(t *testing.T) {
+	// NOTE: this test is lifted straight out of the stdlib with no changes. to
+	// show that the cert-only workflow is fine.
+
+	template := x509.Certificate{
+		SerialNumber: big.NewInt(1),
+		DNSNames:     []string{"example.com"},
+	}
+
+	derBytes, err := x509.CreateCertificate(rand.Reader, &template, &template, &testPrivateKey_x509.PublicKey, testPrivateKey_x509)
+	if err != nil {
+		t.Fatalf("failed to create certificate: %s", err)
+	}
+
+	cert, err := x509.ParseCertificate(derBytes)
+	if err != nil {
+		t.Fatalf("failed to parse certificate: %s", err)
+	}
+
+	for _, ext := range cert.Extensions {
+		if ext.Id.Equal(x509_oidExtensionSubjectAltName) {
+			if !ext.Critical {
+				t.Fatal("SAN extension is not critical")
+			}
+			return
+		}
+	}
+
+	t.Fatal("SAN extension is missing")
+}
+
+func TestX509_EmptySubjectInCSR(t *testing.T) {
+	// NOTE: the CSR-only workflow is flawed so we hack around it
+
+	for _, tc := range []struct {
+		name           string
+		hack           bool
+		expectCritical bool
+	}{
+		{name: "unmodified stdlib",
+			hack:           false,
+			expectCritical: false,
+		},
+		{name: "hacked stdlib",
+			hack:           true,
+			expectCritical: true,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			template := x509.CertificateRequest{
+				DNSNames: []string{"example.com"},
+			}
+			if tc.hack {
+				HackSANExtensionForCSR(&template)
+			}
+
+			derBytes, err := x509.CreateCertificateRequest(rand.Reader, &template, testPrivateKey_x509)
+			if err != nil {
+				t.Fatalf("failed to create certificate request: %s", err)
+			}
+
+			csr, err := x509.ParseCertificateRequest(derBytes)
+			if err != nil {
+				t.Fatalf("failed to parse certificate request: %s", err)
+			}
+
+			for _, ext := range csr.Extensions {
+				if ext.Id.Equal(x509_oidExtensionSubjectAltName) {
+					if tc.expectCritical {
+						if !ext.Critical {
+							t.Fatal("SAN extension is not critical")
+						}
+					} else {
+						if ext.Critical {
+							t.Fatal("SAN extension is critical now; maybe we don't need the hack anymore with this version of Go?")
+						}
+					}
+					return
+				}
+			}
+
+			t.Fatal("SAN extension is missing")
+		})
+	}
+}
+
+func init() {
+	block, _ := pem.Decode([]byte(pemPrivateKey_x509))
+
+	var err error
+	testPrivateKey_x509, err = x509.ParsePKCS1PrivateKey(block.Bytes)
+	if err != nil {
+		panic(err)
+	}
+}
+
+var pemPrivateKey_x509 = `
+-----BEGIN RSA PRIVATE KEY-----
+MIICXAIBAAKBgQCxoeCUW5KJxNPxMp+KmCxKLc1Zv9Ny+4CFqcUXVUYH69L3mQ7v
+IWrJ9GBfcaA7BPQqUlWxWM+OCEQZH1EZNIuqRMNQVuIGCbz5UQ8w6tS0gcgdeGX7
+J7jgCQ4RK3F/PuCM38QBLaHx988qG8NMc6VKErBjctCXFHQt14lerd5KpQIDAQAB
+AoGAYrf6Hbk+mT5AI33k2Jt1kcweodBP7UkExkPxeuQzRVe0KVJw0EkcFhywKpr1
+V5eLMrILWcJnpyHE5slWwtFHBG6a5fLaNtsBBtcAIfqTQ0Vfj5c6SzVaJv0Z5rOd
+7gQF6isy3t3w9IF3We9wXQKzT6q5ypPGdm6fciKQ8RnzREkCQQDZwppKATqQ41/R
+vhSj90fFifrGE6aVKC1hgSpxGQa4oIdsYYHwMzyhBmWW9Xv/R+fPyr8ZwPxp2c12
+33QwOLPLAkEA0NNUb+z4ebVVHyvSwF5jhfJxigim+s49KuzJ1+A2RaSApGyBZiwS
+rWvWkB471POAKUYt5ykIWVZ83zcceQiNTwJBAMJUFQZX5GDqWFc/zwGoKkeR49Yi
+MTXIvf7Wmv6E++eFcnT461FlGAUHRV+bQQXGsItR/opIG7mGogIkVXa3E1MCQARX
+AAA7eoZ9AEHflUeuLn9QJI/r0hyQQLEtrpwv6rDT1GCWaLII5HJ6NUFVf4TTcqxo
+6vdM4QGKTJoO+SaCyP0CQFdpcxSAuzpFcKv0IlJ8XzS/cy+mweCMwyJ1PFEc4FX6
+wg/HcAJWY60xZTJDFN+Qfx8ZQvBEin6c2/h+zZi5IVY=
+-----END RSA PRIVATE KEY-----
+`

agent/consul/auto_encrypt_endpoint_test.go

@@ -8,13 +8,14 @@ import (
 	"strings"
 	"testing"
 
+	msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
 	"github.com/hashicorp/consul/agent/connect"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/testrpc"
 	"github.com/hashicorp/consul/tlsutil"
-	msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 func TestAutoEncryptSign(t *testing.T) {
@@ -80,11 +81,9 @@ func TestAutoEncryptSign(t *testing.T) {
 			require.NoError(t, err, info)
 
 			// Create a CSR.
-			cn, err := connect.CNForCertURI(id)
-			require.NoError(t, err)
 			dnsNames := []string{"localhost"}
 			ipAddresses := []net.IP{net.ParseIP("127.0.0.1")}
-			csr, err := connect.CreateCSR(id, cn, pk, dnsNames, ipAddresses)
+			csr, err := connect.CreateCSR(id, pk, dnsNames, ipAddresses)
 			require.NoError(t, err, info)
 			require.NotEmpty(t, csr, info)
 			args := &structs.CASignRequest{

agent/consul/server_connect.go

@@ -8,13 +8,14 @@ import (
 	"strings"
 	"sync"
 
+	memdb "github.com/hashicorp/go-memdb"
+	"golang.org/x/time/rate"
+
 	"github.com/hashicorp/consul/agent/connect"
 	"github.com/hashicorp/consul/agent/connect/ca"
 	"github.com/hashicorp/consul/agent/consul/state"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/lib/semaphore"
-	memdb "github.com/hashicorp/go-memdb"
-	"golang.org/x/time/rate"
 )
 
 type connectSignRateLimiter struct {
@@ -169,7 +170,6 @@ func (s *Server) SignCertificate(csr *x509.CertificateRequest, spiffeID connect.
 			originalURI := agentID.URI()
 
 			agentID.Host = trustDomain
-			csr.Subject.CommonName = connect.AgentCN(agentID.Agent, trustDomain)
 
 			// recreate the URIs list
 			uris := make([]*url.URL, len(csr.URIs))
@@ -208,7 +208,10 @@ func (s *Server) SignCertificate(csr *x509.CertificateRequest, spiffeID connect.
 		defer s.caLeafLimiter.csrConcurrencyLimiter.Release()
 	}
 
+	connect.HackSANExtensionForCSR(csr)
+
 	// All seems to be in order, actually sign it.
+
 	pem, err := provider.Sign(csr)
 	if err == ca.ErrRateLimited {
 		return nil, ErrRateLimited

tlsutil/generate.go

@@ -15,8 +15,9 @@ import (
 	"net"
 	"time"
 
-	"github.com/hashicorp/consul/agent/connect"
 	"net/url"
+
+	"github.com/hashicorp/consul/agent/connect"
 )
 
 // GenerateSerialNumber returns random bigint generated with crypto/rand