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