From 54f92e29243a4ea61248dfa02e97180b85665d91 Mon Sep 17 00:00:00 2001 From: Todd Radel Date: Mon, 11 Nov 2019 12:11:54 -0500 Subject: [PATCH] Make all Connect Cert Common Names valid FQDNs (#6423) --- agent/cache-types/connect_ca_leaf.go | 5 +- agent/connect/ca/provider_consul.go | 31 +++-- agent/connect/ca/provider_consul_test.go | 24 ++-- agent/connect/ca/provider_vault.go | 19 +-- agent/connect/ca/provider_vault_test.go | 3 +- agent/connect/common_names.go | 133 +++++++++++++++++++++ agent/connect/csr.go | 8 +- agent/connect/testing_ca.go | 7 +- agent/consul/auto_encrypt.go | 6 +- agent/consul/auto_encrypt_endpoint_test.go | 4 +- 10 files changed, 201 insertions(+), 39 deletions(-) create mode 100644 agent/connect/common_names.go diff --git a/agent/cache-types/connect_ca_leaf.go b/agent/cache-types/connect_ca_leaf.go index 5e8c91f956..7c402ee2e1 100644 --- a/agent/cache-types/connect_ca_leaf.go +++ b/agent/cache-types/connect_ca_leaf.go @@ -507,6 +507,7 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest, // Build the cert uri var id connect.CertURI + var commonName string if req.Service != "" { id = &connect.SpiffeIDService{ Host: roots.TrustDomain, @@ -514,12 +515,14 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest, Namespace: "default", Service: req.Service, } + commonName = connect.ServiceCN(req.Service, roots.TrustDomain) } else if req.Agent != "" { id = &connect.SpiffeIDAgent{ Host: roots.TrustDomain, Datacenter: req.Datacenter, Agent: req.Agent, } + commonName = connect.ServiceCN(req.Agent, roots.TrustDomain) } else { return result, errors.New("URI must be either service or agent") } @@ -542,7 +545,7 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest, } // Create a CSR. - csr, err := connect.CreateCSR(id, pk) + csr, err := connect.CreateCSR(id, commonName, pk) if err != nil { return result, err } diff --git a/agent/connect/ca/provider_consul.go b/agent/connect/ca/provider_consul.go index 1f5508a1f0..3ddf25b33a 100644 --- a/agent/connect/ca/provider_consul.go +++ b/agent/connect/ca/provider_consul.go @@ -188,7 +188,13 @@ func (c *ConsulProvider) GenerateIntermediateCSR() (string, error) { return "", err } - csr, err := connect.CreateCACSR(c.spiffeID, signer) + uid, err := connect.CompactUID() + if err != nil { + return "", err + } + cn := connect.CACN("consul", uid, c.clusterID, c.isRoot) + + csr, err := connect.CreateCACSR(c.spiffeID, cn, signer) if err != nil { return "", err } @@ -313,14 +319,12 @@ func (c *ConsulProvider) Sign(csr *x509.CertificateRequest) (string, error) { return "", err } - subject := "" - switch id := spiffeId.(type) { - case *connect.SpiffeIDService: - subject = id.Service - case *connect.SpiffeIDAgent: - subject = id.Agent - default: - return "", fmt.Errorf("SPIFFE ID in CSR must be a service ID") + // 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 @@ -583,8 +587,6 @@ func (c *ConsulProvider) generateCA(privateKey string, sn uint64) (string, error return "", fmt.Errorf("error parsing private key %q: %s", privateKey, err) } - name := fmt.Sprintf("Consul CA %d", sn) - // The URI (SPIFFE compatible) for the cert id := connect.SpiffeIDSigningForCluster(config) keyId, err := connect.KeyId(privKey.Public()) @@ -593,11 +595,16 @@ func (c *ConsulProvider) generateCA(privateKey string, sn uint64) (string, error } // Create the CA cert + uid, err := connect.CompactUID() + if err != nil { + return "", err + } + cn := connect.CACN("consul", uid, c.clusterID, c.isRoot) serialNum := &big.Int{} serialNum.SetUint64(sn) template := x509.Certificate{ SerialNumber: serialNum, - Subject: pkix.Name{CommonName: name}, + Subject: pkix.Name{CommonName: cn}, URIs: []*url.URL{id.URI()}, BasicConstraintsValid: true, KeyUsage: x509.KeyUsageCertSign | diff --git a/agent/connect/ca/provider_consul_test.go b/agent/connect/ca/provider_consul_test.go index eac076c0dc..c85deda30e 100644 --- a/agent/connect/ca/provider_consul_test.go +++ b/agent/connect/ca/provider_consul_test.go @@ -143,7 +143,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) { require.NoError(provider.GenerateRoot()) spiffeService := &connect.SpiffeIDService{ - Host: "node1", + Host: connect.TestClusterID + ".consul", Namespace: "default", Datacenter: "dc1", Service: "foo", @@ -161,8 +161,8 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) { parsed, err := connect.ParseCert(cert) require.NoError(err) - require.Equal(parsed.URIs[0], spiffeService.URI()) - require.Equal(parsed.Subject.CommonName, "foo") + require.Equal(spiffeService.URI(), parsed.URIs[0]) + require.Equal(connect.ServiceCN("foo", connect.TestClusterID), parsed.Subject.CommonName) require.Equal(uint64(2), parsed.SerialNumber.Uint64()) requireNotEncoded(t, parsed.SubjectKeyId) requireNotEncoded(t, parsed.AuthorityKeyId) @@ -187,8 +187,8 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) { parsed, err := connect.ParseCert(cert) require.NoError(err) - require.Equal(parsed.URIs[0], spiffeService.URI()) - require.Equal(parsed.Subject.CommonName, "bar") + require.Equal(spiffeService.URI(), parsed.URIs[0]) + require.Equal(connect.ServiceCN("bar", connect.TestClusterID), parsed.Subject.CommonName) require.Equal(parsed.SerialNumber.Uint64(), uint64(2)) requireNotEncoded(t, parsed.SubjectKeyId) requireNotEncoded(t, parsed.AuthorityKeyId) @@ -199,7 +199,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) { } spiffeAgent := &connect.SpiffeIDAgent{ - Host: "node1", + Host: connect.TestClusterID + ".consul", Datacenter: "dc1", Agent: "uuid", } @@ -216,7 +216,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) { parsed, err := connect.ParseCert(cert) require.NoError(err) require.Equal(spiffeAgent.URI(), parsed.URIs[0]) - require.Equal("uuid", parsed.Subject.CommonName) + require.Equal(connect.AgentCN("uuid", connect.TestClusterID), parsed.Subject.CommonName) require.Equal(uint64(2), parsed.SerialNumber.Uint64()) requireNotEncoded(t, parsed.SubjectKeyId) requireNotEncoded(t, parsed.AuthorityKeyId) @@ -297,7 +297,11 @@ func testCrossSignProviders(t *testing.T, provider1, provider2 Provider) { requireNotEncoded(t, oldRoot.AuthorityKeyId) // AuthorityKeyID should now be the signing root's, SubjectKeyId should be kept. - require.Equal(oldRoot.AuthorityKeyId, xc.AuthorityKeyId) + require.Equal(oldRoot.SubjectKeyId, xc.AuthorityKeyId, + "newSKID=%x\nnewAKID=%x\noldSKID=%x\noldAKID=%x\nxcSKID=%x\nxcAKID=%x", + newRoot.SubjectKeyId, newRoot.AuthorityKeyId, + oldRoot.SubjectKeyId, oldRoot.AuthorityKeyId, + xc.SubjectKeyId, xc.AuthorityKeyId) require.Equal(newRoot.SubjectKeyId, xc.SubjectKeyId) // Subject name should not have changed. @@ -308,7 +312,7 @@ func testCrossSignProviders(t *testing.T, provider1, provider2 Provider) { // Get a leaf cert so we can verify against the cross-signed cert. spiffeService := &connect.SpiffeIDService{ - Host: "node1", + Host: connect.TestClusterID + ".consul", Namespace: "default", Datacenter: "dc1", Service: "foo", @@ -396,7 +400,7 @@ func testSignIntermediateCrossDC(t *testing.T, provider1, provider2 Provider) { // Have provider2 sign a leaf cert and make sure the chain is correct. spiffeService := &connect.SpiffeIDService{ - Host: "node1", + Host: connect.TestClusterID + ".consul", Namespace: "default", Datacenter: "dc1", Service: "foo", diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index e2213daa86..f522d7a637 100644 --- a/agent/connect/ca/provider_vault.go +++ b/agent/connect/ca/provider_vault.go @@ -11,7 +11,6 @@ import ( "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/structs" - "github.com/hashicorp/go-uuid" vaultapi "github.com/hashicorp/vault/api" "github.com/mitchellh/mapstructure" ) @@ -25,7 +24,7 @@ type VaultProvider struct { config *structs.VaultCAProviderConfig client *vaultapi.Client isRoot bool - clusterId string + clusterID string spiffeID *connect.SpiffeIDSigning } @@ -41,7 +40,7 @@ func vaultTLSConfig(config *structs.VaultCAProviderConfig) *vaultapi.TLSConfig { } // Configure sets up the provider using the given configuration. -func (v *VaultProvider) Configure(clusterId string, isRoot bool, rawConfig map[string]interface{}) error { +func (v *VaultProvider) Configure(clusterID string, isRoot bool, rawConfig map[string]interface{}) error { config, err := ParseVaultCAConfig(rawConfig) if err != nil { return err @@ -63,8 +62,8 @@ func (v *VaultProvider) Configure(clusterId string, isRoot bool, rawConfig map[s v.config = config v.client = client v.isRoot = isRoot - v.clusterId = clusterId - v.spiffeID = connect.SpiffeIDSigningForCluster(&structs.CAConfiguration{ClusterID: clusterId}) + v.clusterID = clusterID + v.spiffeID = connect.SpiffeIDSigningForCluster(&structs.CAConfiguration{ClusterID: clusterID}) return nil } @@ -98,12 +97,12 @@ func (v *VaultProvider) GenerateRoot() error { fallthrough case ErrBackendNotInitialized: - uuid, err := uuid.GenerateUUID() + uid, err := connect.CompactUID() if err != nil { return err } _, err = v.client.Logical().Write(v.config.RootPKIPath+"root/generate/internal", map[string]interface{}{ - "common_name": fmt.Sprintf("Vault CA Root Authority %s", uuid), + "common_name": connect.CACN("vault", uid, v.clusterID, v.isRoot), "uri_sans": v.spiffeID.URI().String(), "key_type": v.config.PrivateKeyType, "key_bits": v.config.PrivateKeyBits, @@ -174,8 +173,12 @@ func (v *VaultProvider) generateIntermediateCSR() (string, error) { } // Generate a new intermediate CSR for the root to sign. + uid, err := connect.CompactUID() + if err != nil { + return "", err + } data, err := v.client.Logical().Write(v.config.IntermediatePKIPath+"intermediate/generate/internal", map[string]interface{}{ - "common_name": "Vault CA Intermediate Authority", + "common_name": connect.CACN("vault", uid, v.clusterID, v.isRoot), "key_type": v.config.PrivateKeyType, "key_bits": v.config.PrivateKeyBits, "uri_sans": v.spiffeID.URI().String(), diff --git a/agent/connect/ca/provider_vault_test.go b/agent/connect/ca/provider_vault_test.go index fb64adf19c..8b8932c91c 100644 --- a/agent/connect/ca/provider_vault_test.go +++ b/agent/connect/ca/provider_vault_test.go @@ -80,7 +80,7 @@ func TestVaultCAProvider_Bootstrap(t *testing.T) { require.NoError(err) require.True(parsed.IsCA) require.Len(parsed.URIs, 1) - require.Equal(parsed.URIs[0].String(), fmt.Sprintf("spiffe://%s.consul", provider.clusterId)) + require.Equal(fmt.Sprintf("spiffe://%s.consul", provider.clusterID), parsed.URIs[0].String()) } } @@ -349,7 +349,6 @@ func testVaultProviderWithConfig(t *testing.T, isRoot bool, rawConf map[string]i t.Fatalf("err: %v", err) } } - return provider, testVault } diff --git a/agent/connect/common_names.go b/agent/connect/common_names.go new file mode 100644 index 0000000000..8d7b06510d --- /dev/null +++ b/agent/connect/common_names.go @@ -0,0 +1,133 @@ +package connect + +import ( + "crypto/rand" + "encoding/binary" + "fmt" + "regexp" + "strconv" + "strings" +) + +var invalidDNSNameChars = regexp.MustCompile(`[^a-z0-9]`) + +// ServiceCN returns the common name for a service's certificate. We can't use +// SPIFFE URIs because some CAs require valid FQDN format. We can't use SNI +// values because they are often too long than the 64 bytes allowed by +// CommonNames. We could attempt to encode more information into this to make +// identifying which instance/node it was issued to in a management tool easier +// but that just introduces more complications around length. It's also strange +// that the Common Name would encode more information than the actual +// identifying URI we use to assert anything does and my lead to bad assumptions +// that the common name is in some way "secure" or verified - there is nothing +// inherently provable here except that the requestor had ACLs for that service +// name in that DC. +// +// Format is: +// .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, trustDomain string) string { + svc := invalidDNSNameChars.ReplaceAllString(strings.ToLower(serviceName), "") + // 20 = 7 bytes for ".consul", 8 bytes for trust domain, 5 bytes for ".svc." + return fmt.Sprintf("%s.svc.%s.consul", + truncateTo(svc, 64-20), truncateTo(trustDomain, 8)) +} + +// AgentCN returns the common name for an agent certificate. See ServiceCN for +// more details on rationale. +// +// Format is: +// .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 +// of collision. It is not suitable for UUIDs for very frequent events. It's +// main purpose is to assign unique values to CA certificate Common Names which +// need to be unique in some providers - see CACN - but without using up large +// amounts of the limited 64 character Common Name. It also makes the values +// more easily digestable by humans considering there are likely to be few of +// them ever in use. +func CompactUID() (string, error) { + // 48 bits (6 bytes) is enough to fill 8 bytes in base36 but it's simpler to + // have a whole uint8 to convert from. + var raw [8]byte + _, err := rand.Read(raw[:]) + if err != nil { + return "", err + } + + i := binary.LittleEndian.Uint64(raw[:]) + return truncateTo(strconv.FormatInt(int64(i), 36), 8), nil +} + +// CACN returns the common name for a CA certificate. See ServiceCN for more +// details on rationale. A uniqueID is requires because some providers (e.g. +// Vault) cache by subject and so produce incorrect results - for example they +// won't cross-sign an older CA certificate with the same common name since they +// think they already have a valid cert for than CN and just return the current +// root. +// +// This can be generated by any means but will be truncated to 8 chars and +// sanitised to DNS-safe chars. CompactUID generates suitable UIDs for this +// specific purpose. +// +// Format is: +// {provider}-{uniqueID-8-b36}.{root|int}.ca..consul +// +// trust domain is truncated to keep the whole name short +func CACN(provider, uniqueID, trustDomain string, isRoot bool) string { + providerSan := invalidDNSNameChars.ReplaceAllString(strings.ToLower(provider), "") + typ := "root" + if !isRoot { + typ = "int" + } + // 33 = 7 bytes for ".consul", 8 bytes for trust domain, 9 bytes for + // ".root.ca.", 9 bytes for "-{uniqueID-8-b36}" + uidSAN := invalidDNSNameChars.ReplaceAllString(strings.ToLower(uniqueID), "") + return fmt.Sprintf("%s-%s.%s.ca.%s.consul", typ, truncateTo(uidSAN, 8), + truncateTo(providerSan, 64-33), truncateTo(trustDomain, 8)) +} + +func truncateTo(s string, n int) string { + if len(s) > n { + return s[:n] + } + return s +} + +// CNForCertURI returns the correct common name for a given cert URI type. It +// doesn't work for CA Signing IDs since more context is needed and CA Providers +// always know their CN from their own context. +func CNForCertURI(uri CertURI) (string, error) { + // Even though leafs should be from our own CSRs which should have the same CN + // logic as here, override anyway to account for older version clients that + // didn't include the Common Name in the CSR. + switch id := uri.(type) { + case *SpiffeIDService: + return ServiceCN(id.Service, id.Host), nil + case *SpiffeIDAgent: + return AgentCN(id.Agent, id.Host), nil + case *SpiffeIDSigning: + return "", fmt.Errorf("CertURI is a SpiffeIDSigning, not enough context to generate Common Name") + default: + return "", fmt.Errorf("CertURI type not recognized") + } +} diff --git a/agent/connect/csr.go b/agent/connect/csr.go index 8fc57840e2..cfe37b78ae 100644 --- a/agent/connect/csr.go +++ b/agent/connect/csr.go @@ -43,11 +43,13 @@ func SigAlgoForKeyType(keyType string) x509.SignatureAlgorithm { // CreateCSR returns a CSR to sign the given service along with the PEM-encoded // private key for this certificate. -func CreateCSR(uri CertURI, privateKey crypto.Signer, extensions ...pkix.Extension) (string, error) { +func CreateCSR(uri CertURI, commonName string, privateKey crypto.Signer, + extensions ...pkix.Extension) (string, error) { template := &x509.CertificateRequest{ URIs: []*url.URL{uri.URI()}, SignatureAlgorithm: SigAlgoForKey(privateKey), ExtraExtensions: extensions, + Subject: pkix.Name{CommonName: commonName}, } // Create the CSR itself @@ -67,13 +69,13 @@ func CreateCSR(uri CertURI, privateKey crypto.Signer, extensions ...pkix.Extensi // CreateCSR returns a CA CSR to sign the given service along with the PEM-encoded // private key for this certificate. -func CreateCACSR(uri CertURI, privateKey crypto.Signer) (string, error) { +func CreateCACSR(uri CertURI, commonName string, privateKey crypto.Signer) (string, error) { ext, err := CreateCAExtension() if err != nil { return "", err } - return CreateCSR(uri, privateKey, ext) + return CreateCSR(uri, commonName, privateKey, ext) } // CreateCAExtension creates a pkix.Extension for the x509 Basic Constraints diff --git a/agent/connect/testing_ca.go b/agent/connect/testing_ca.go index 1e2b563c66..50dc3334c6 100644 --- a/agent/connect/testing_ca.go +++ b/agent/connect/testing_ca.go @@ -211,7 +211,7 @@ func testLeaf(t testing.T, service string, root *structs.CARoot, keyType string, // Cert template for generation template := x509.Certificate{ SerialNumber: sn, - Subject: pkix.Name{CommonName: service}, + Subject: pkix.Name{CommonName: ServiceCN(service, TestClusterID)}, URIs: []*url.URL{spiffeId.URI()}, SignatureAlgorithm: SigAlgoForKeyType(rootKeyType), BasicConstraintsValid: true, @@ -262,7 +262,12 @@ func TestLeaf(t testing.T, service string, root *structs.CARoot) (string, string // TestCSR returns a CSR to sign the given service along with the PEM-encoded // private key for this certificate. func TestCSR(t testing.T, uri CertURI) (string, string) { + cn, err := CNForCertURI(uri) + if err != nil { + t.Fatalf("TestCSR failed to get Common Name: %s", err) + } template := &x509.CertificateRequest{ + Subject: pkix.Name{CommonName: cn}, URIs: []*url.URL{uri.URI()}, SignatureAlgorithm: x509.ECDSAWithSHA256, } diff --git a/agent/consul/auto_encrypt.go b/agent/consul/auto_encrypt.go index e3a0e78c54..183aed5ddc 100644 --- a/agent/consul/auto_encrypt.go +++ b/agent/consul/auto_encrypt.go @@ -65,7 +65,11 @@ func (c *Client) RequestAutoEncryptCerts(servers []string, port int, token strin } // Create a CSR. - csr, err := connect.CreateCSR(id, pk) + // + // The Common Name includes the dummy trust domain for now but Server will + // override this when it is signed anyway so it's OK. + cn := connect.AgentCN(string(c.config.NodeName), dummyTrustDomain) + csr, err := connect.CreateCSR(id, cn, pk) if err != nil { return errFn(err) } diff --git a/agent/consul/auto_encrypt_endpoint_test.go b/agent/consul/auto_encrypt_endpoint_test.go index cc260b515e..93bf8f2ec2 100644 --- a/agent/consul/auto_encrypt_endpoint_test.go +++ b/agent/consul/auto_encrypt_endpoint_test.go @@ -75,7 +75,9 @@ func TestAutoEncryptSign(t *testing.T) { require.NoError(t, err, info) // Create a CSR. - csr, err := connect.CreateCSR(id, pk) + cn, err := connect.CNForCertURI(id) + require.NoError(t, err) + csr, err := connect.CreateCSR(id, cn, pk) require.NoError(t, err, info) require.NotEmpty(t, csr, info) args := &structs.CASignRequest{