From c3d5a2a5abece1138962d44ccc41ae5c981b3e7c Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" <4903+rboyer@users.noreply.github.com> Date: Fri, 25 Jun 2021 13:00:00 -0500 Subject: [PATCH] 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 | 3 + agent/agent_test.go | 3 +- agent/auto-config/auto_encrypt_test.go | 23 +--- agent/auto-config/tls.go | 6 +- agent/cache-types/connect_ca_leaf.go | 5 +- agent/connect/ca/provider_aws.go | 5 +- agent/connect/ca/provider_consul.go | 34 ++---- agent/connect/ca/provider_consul_test.go | 6 +- agent/connect/ca/provider_vault.go | 2 + agent/connect/common_names.go | 119 +------------------ agent/connect/common_names_test.go | 115 ------------------- agent/connect/csr.go | 8 +- agent/connect/testing_ca.go | 22 +--- agent/connect/uri_agent.go | 4 - agent/connect/uri_service.go | 4 - agent/connect/x509_patch.go | 126 +++++++++++++++++++++ agent/connect/x509_patch_test.go | 126 +++++++++++++++++++++ agent/consul/auto_encrypt_endpoint_test.go | 11 +- agent/consul/server_connect.go | 9 +- tlsutil/generate.go | 3 +- 20 files changed, 303 insertions(+), 331 deletions(-) create mode 100644 .changelog/10424.txt delete mode 100644 agent/connect/common_names_test.go create mode 100644 agent/connect/x509_patch.go create mode 100644 agent/connect/x509_patch_test.go diff --git a/.changelog/10424.txt b/.changelog/10424.txt new file mode 100644 index 000000000..642eedf32 --- /dev/null +++ b/.changelog/10424.txt @@ -0,0 +1,3 @@ +```release-note:improvement +connect/ca: cease including the common name field in generated x509 non-CA certificates +``` diff --git a/agent/agent_test.go b/agent/agent_test.go index 3726edbc8..584ea1eaa 100644 --- a/agent/agent_test.go +++ b/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]) } diff --git a/agent/auto-config/auto_encrypt_test.go b/agent/auto-config/auto_encrypt_test.go index 2de33f68d..1bcd6bf9a 100644 --- a/agent/auto-config/auto_encrypt_test.go +++ b/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"), diff --git a/agent/auto-config/tls.go b/agent/auto-config/tls.go index 380c9f9f8..08ac6f9c7 100644 --- a/agent/auto-config/tls.go +++ b/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 } diff --git a/agent/cache-types/connect_ca_leaf.go b/agent/cache-types/connect_ca_leaf.go index 86807f316..95f1de538 100644 --- a/agent/cache-types/connect_ca_leaf.go +++ b/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 } diff --git a/agent/connect/ca/provider_aws.go b/agent/connect/ca/provider_aws.go index ffdb11d23..6e5922bc7 100644 --- a/agent/connect/ca/provider_aws.go +++ b/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") } diff --git a/agent/connect/ca/provider_consul.go b/agent/connect/ca/provider_consul.go index f216cf5dc..b8ca90f9a 100644 --- a/agent/connect/ca/provider_consul.go +++ b/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, diff --git a/agent/connect/ca/provider_consul_test.go b/agent/connect/ca/provider_consul_test.go index f0ad804ea..7a58c12fb 100644 --- a/agent/connect/ca/provider_consul_test.go +++ b/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) diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index a03e0c173..e0dd6647e 100644 --- a/agent/connect/ca/provider_vault.go +++ b/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 diff --git a/agent/connect/common_names.go b/agent/connect/common_names.go index a4821a7ae..2915de217 100644 --- a/agent/connect/common_names.go +++ b/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 . 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: -// .svc..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: -// .agnt..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") - } -} diff --git a/agent/connect/common_names_test.go b/agent/connect/common_names_test.go deleted file mode 100644 index 50b0cb161..000000000 --- a/agent/connect/common_names_test.go +++ /dev/null @@ -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) - } - }) - } -} diff --git a/agent/connect/csr.go b/agent/connect/csr.go index 4ba0f1a9f..cc01f991e 100644 --- a/agent/connect/csr.go +++ b/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 diff --git a/agent/connect/testing_ca.go b/agent/connect/testing_ca.go index 4851b9db5..85a72dfac 100644 --- a/agent/connect/testing_ca.go +++ b/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 -// 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) { +func testLeafWithID(t testing.T, spiffeId CertURI, 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) diff --git a/agent/connect/uri_agent.go b/agent/connect/uri_agent.go index 981dee61f..e8e3c8f06 100644 --- a/agent/connect/uri_agent.go +++ b/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) -} diff --git a/agent/connect/uri_service.go b/agent/connect/uri_service.go index 7136df8ab..d19fbe775 100644 --- a/agent/connect/uri_service.go +++ b/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) -} diff --git a/agent/connect/x509_patch.go b/agent/connect/x509_patch.go new file mode 100644 index 000000000..9dcd12de1 --- /dev/null +++ b/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 +} diff --git a/agent/connect/x509_patch_test.go b/agent/connect/x509_patch_test.go new file mode 100644 index 000000000..61278b359 --- /dev/null +++ b/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----- +` diff --git a/agent/consul/auto_encrypt_endpoint_test.go b/agent/consul/auto_encrypt_endpoint_test.go index 9e35e75e9..9d61d4202 100644 --- a/agent/consul/auto_encrypt_endpoint_test.go +++ b/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{ diff --git a/agent/consul/server_connect.go b/agent/consul/server_connect.go index 940168429..080c2f24e 100644 --- a/agent/consul/server_connect.go +++ b/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 diff --git a/tlsutil/generate.go b/tlsutil/generate.go index 8b4ac0f09..61cf40e14 100644 --- a/tlsutil/generate.go +++ b/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