diff --git a/agent/cache-types/connect_ca_leaf.go b/agent/cache-types/connect_ca_leaf.go index 955ce4d33a76..f9983dd29632 100644 --- a/agent/cache-types/connect_ca_leaf.go +++ b/agent/cache-types/connect_ca_leaf.go @@ -599,7 +599,7 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest, return result, err } // Set the CA key ID so we can easily tell when a active root has changed. - state.authorityKeyID = connect.HexString(cert.AuthorityKeyId) + state.authorityKeyID = connect.EncodeSigningKeyID(cert.AuthorityKeyId) result.Value = &reply // Store value not pointer so we don't accidentally mutate the cache entry diff --git a/agent/connect/ca/provider_consul.go b/agent/connect/ca/provider_consul.go index 9f4fa5fa5734..74b2c9f60e5d 100644 --- a/agent/connect/ca/provider_consul.go +++ b/agent/connect/ca/provider_consul.go @@ -11,7 +11,6 @@ import ( "fmt" "math/big" "net/url" - "strings" "sync" "time" @@ -48,7 +47,7 @@ func (c *ConsulProvider) Configure(clusterID string, isRoot bool, rawConfig map[ } c.config = config hash := sha256.Sum256([]byte(fmt.Sprintf("%s,%s,%v", config.PrivateKey, config.RootCert, isRoot))) - c.id = strings.Replace(fmt.Sprintf("% x", hash), " ", ":", -1) + c.id = connect.HexString(hash[:]) c.clusterID = clusterID c.isRoot = isRoot c.spiffeID = connect.SpiffeIDSigningForCluster(&structs.CAConfiguration{ClusterID: clusterID}) diff --git a/agent/connect/ca/provider_consul_test.go b/agent/connect/ca/provider_consul_test.go index 774dcc2649fd..b6f1d4871e68 100644 --- a/agent/connect/ca/provider_consul_test.go +++ b/agent/connect/ca/provider_consul_test.go @@ -71,6 +71,11 @@ func testConsulCAConfig() *structs.CAConfiguration { } } +func requireNotEncoded(t *testing.T, v []byte) { + t.Helper() + require.False(t, connect.IsHexString(v)) +} + func TestConsulCAProvider_Bootstrap(t *testing.T) { t.Parallel() @@ -94,6 +99,8 @@ func TestConsulCAProvider_Bootstrap(t *testing.T) { parsed, err := connect.ParseCert(root) require.NoError(err) require.Equal(parsed.URIs[0].String(), fmt.Sprintf("spiffe://%s.consul", conf.ClusterID)) + requireNotEncoded(t, parsed.SubjectKeyId) + requireNotEncoded(t, parsed.AuthorityKeyId) } func TestConsulCAProvider_Bootstrap_WithCert(t *testing.T) { @@ -152,6 +159,8 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) { require.Equal(parsed.URIs[0], spiffeService.URI()) require.Equal(parsed.Subject.CommonName, "foo") require.Equal(uint64(2), parsed.SerialNumber.Uint64()) + requireNotEncoded(t, parsed.SubjectKeyId) + requireNotEncoded(t, parsed.AuthorityKeyId) // Ensure the cert is valid now and expires within the correct limit. now := time.Now() @@ -176,6 +185,8 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) { require.Equal(parsed.URIs[0], spiffeService.URI()) require.Equal(parsed.Subject.CommonName, "bar") require.Equal(parsed.SerialNumber.Uint64(), uint64(2)) + requireNotEncoded(t, parsed.SubjectKeyId) + requireNotEncoded(t, parsed.AuthorityKeyId) // Ensure the cert is valid now and expires within the correct limit. require.True(time.Until(parsed.NotAfter) < 3*24*time.Hour) @@ -202,6 +213,8 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) { require.Equal(spiffeAgent.URI(), parsed.URIs[0]) require.Equal("uuid", parsed.Subject.CommonName) require.Equal(uint64(2), parsed.SerialNumber.Uint64()) + requireNotEncoded(t, parsed.SubjectKeyId) + requireNotEncoded(t, parsed.AuthorityKeyId) // Ensure the cert is valid now and expires within the correct limit. now := time.Now() @@ -240,22 +253,30 @@ func testCrossSignProviders(t *testing.T, provider1, provider2 Provider) { newRoot, err := connect.ParseCert(newRootPEM) require.NoError(err) oldSubject := newRoot.Subject.CommonName + requireNotEncoded(t, newRoot.SubjectKeyId) + requireNotEncoded(t, newRoot.AuthorityKeyId) newInterPEM, err := provider2.ActiveIntermediate() require.NoError(err) newIntermediate, err := connect.ParseCert(newInterPEM) require.NoError(err) + requireNotEncoded(t, newIntermediate.SubjectKeyId) + requireNotEncoded(t, newIntermediate.AuthorityKeyId) // Have provider1 cross sign our new root cert. xcPEM, err := provider1.CrossSignCA(newRoot) require.NoError(err) xc, err := connect.ParseCert(xcPEM) require.NoError(err) + requireNotEncoded(t, xc.SubjectKeyId) + requireNotEncoded(t, xc.AuthorityKeyId) oldRootPEM, err := provider1.ActiveRoot() require.NoError(err) oldRoot, err := connect.ParseCert(oldRootPEM) require.NoError(err) + requireNotEncoded(t, oldRoot.SubjectKeyId) + requireNotEncoded(t, oldRoot.AuthorityKeyId) // AuthorityKeyID should now be the signing root's, SubjectKeyId should be kept. require.Equal(oldRoot.AuthorityKeyId, xc.AuthorityKeyId) @@ -284,6 +305,8 @@ func testCrossSignProviders(t *testing.T, provider1, provider2 Provider) { cert, err := connect.ParseCert(leafPEM) require.NoError(err) + requireNotEncoded(t, cert.SubjectKeyId) + requireNotEncoded(t, cert.AuthorityKeyId) // Check that the leaf signed by the new cert can be verified by either root // certificate by using the new intermediate + cross-signed cert. @@ -357,6 +380,8 @@ func testSignIntermediateCrossDC(t *testing.T, provider1, provider2 Provider) { cert, err := connect.ParseCert(leafPEM) require.NoError(err) + requireNotEncoded(t, cert.SubjectKeyId) + requireNotEncoded(t, cert.AuthorityKeyId) // Check that the leaf signed by the new cert can be verified using the // returned cert chain (signed intermediate + remote root). diff --git a/agent/connect/parsing.go b/agent/connect/parsing.go index ff0f0813dbd0..9e700e53521b 100644 --- a/agent/connect/parsing.go +++ b/agent/connect/parsing.go @@ -7,8 +7,10 @@ import ( "crypto/sha1" "crypto/sha256" "crypto/x509" + "encoding/hex" "encoding/pem" "fmt" + "math/big" "strings" ) @@ -111,13 +113,35 @@ func KeyId(raw interface{}) ([]byte, error) { return nil, err } - // String formatted kID := sha256.Sum256(bs) - return []byte(strings.Replace(fmt.Sprintf("% x", kID), " ", ":", -1)), nil + return kID[:], nil } +// EncodeSerialNumber encodes the given serial number as a colon-hex encoded +// string. +func EncodeSerialNumber(serial *big.Int) string { + return HexString(serial.Bytes()) +} + +// EncodeSigningKeyID encodes the given AuthorityKeyId or SubjectKeyId into a +// colon-hex encoded string suitable for using as a SigningKeyID value. +func EncodeSigningKeyID(keyID []byte) string { return HexString(keyID) } + // HexString returns a standard colon-separated hex value for the input // byte slice. This should be used with cert serial numbers and so on. func HexString(input []byte) string { return strings.Replace(fmt.Sprintf("% x", input), " ", ":", -1) } + +// IsHexString returns true if the input is the output of HexString(). Meant +// for use in tests. +func IsHexString(input []byte) bool { + s := string(input) + if strings.Count(s, ":") < 5 { // 5 is arbitrary + return false + } + + s = strings.ReplaceAll(s, ":", "") + _, err := hex.DecodeString(s) + return err == nil +} diff --git a/agent/connect/testing_ca.go b/agent/connect/testing_ca.go index a1deeeedbf2b..a2b447e0b008 100644 --- a/agent/connect/testing_ca.go +++ b/agent/connect/testing_ca.go @@ -35,7 +35,7 @@ func testCA(t testing.T, xc *structs.CARoot, keyType string, keyBits int) *struc // Create the private key we'll use for this CA cert. signer, keyPEM := testPrivateKey(t, keyType, keyBits) result.SigningKey = keyPEM - result.SigningKeyID = HexString(testKeyID(t, signer.Public())) + result.SigningKeyID = EncodeSigningKeyID(testKeyID(t, signer.Public())) result.PrivateKeyType = keyType result.PrivateKeyBits = keyBits diff --git a/agent/consul/connect_ca_endpoint.go b/agent/consul/connect_ca_endpoint.go index fbcdc403cb42..5b5e8e7d2068 100644 --- a/agent/consul/connect_ca_endpoint.go +++ b/agent/consul/connect_ca_endpoint.go @@ -526,7 +526,7 @@ func (s *ConnectCA) Sign( // Set the response *reply = structs.IssuedCert{ - SerialNumber: connect.HexString(cert.SerialNumber.Bytes()), + SerialNumber: connect.EncodeSerialNumber(cert.SerialNumber), CertPEM: pem, ValidAfter: cert.NotBefore, ValidBefore: cert.NotAfter, diff --git a/agent/consul/leader_connect.go b/agent/consul/leader_connect.go index c53b88730d3d..02ad215569db 100644 --- a/agent/consul/leader_connect.go +++ b/agent/consul/leader_connect.go @@ -85,7 +85,7 @@ func parseCARoot(pemValue, provider, clusterID string) (*structs.CARoot, error) ID: id, Name: fmt.Sprintf("%s CA Root Cert", strings.Title(provider)), SerialNumber: rootCert.SerialNumber.Uint64(), - SigningKeyID: connect.HexString(rootCert.SubjectKeyId), + SigningKeyID: connect.EncodeSigningKeyID(rootCert.SubjectKeyId), ExternalTrustDomain: clusterID, NotBefore: rootCert.NotBefore, NotAfter: rootCert.NotAfter, diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go index 3a30c7c1a030..e041b1468ece 100644 --- a/agent/consul/leader_connect_test.go +++ b/agent/consul/leader_connect_test.go @@ -1066,7 +1066,7 @@ h1IHCbxWsUT3AiARwj5/D/CUppy6BHIFkvcpOCQoVyo= // just to make sure these two are not the same require.NotEqual(t, rootCert.AuthorityKeyId, rootCert.SubjectKeyId) - require.Equal(t, connect.HexString(rootCert.SubjectKeyId), root.SigningKeyID) + require.Equal(t, connect.EncodeSigningKeyID(rootCert.SubjectKeyId), root.SigningKeyID) } } } diff --git a/agent/proxycfg/testing.go b/agent/proxycfg/testing.go index 3c484bf0cf19..a21739bb2ea9 100644 --- a/agent/proxycfg/testing.go +++ b/agent/proxycfg/testing.go @@ -103,7 +103,7 @@ func TestLeafForCA(t testing.T, ca *structs.CARoot) *structs.IssuedCert { require.NoError(t, err) return &structs.IssuedCert{ - SerialNumber: connect.HexString(leafCert.SerialNumber.Bytes()), + SerialNumber: connect.EncodeSerialNumber(leafCert.SerialNumber), CertPEM: leafPEM, PrivateKeyPEM: pkPEM, Service: "web", diff --git a/api/watch/funcs_test.go b/api/watch/funcs_test.go index 4c3c9987f8cc..80a9777b57f9 100644 --- a/api/watch/funcs_test.go +++ b/api/watch/funcs_test.go @@ -14,7 +14,6 @@ import ( "fmt" "math/big" "net/url" - "strings" "sync" "testing" "time" @@ -77,7 +76,8 @@ func updateConnectCA(t *testing.T, client *api.Client) { bs, err = x509.MarshalPKIXPublicKey(pk.Public()) require.NoError(t, err) - kID := []byte(strings.Replace(fmt.Sprintf("% x", sha256.Sum256(bs)), " ", ":", -1)) + hash := sha256.Sum256(bs) + kID := hash[:] // Create the CA cert template := x509.Certificate{ diff --git a/connect/service_test.go b/connect/service_test.go index 58e771e91cd9..b71f854c8d2e 100644 --- a/connect/service_test.go +++ b/connect/service_test.go @@ -181,8 +181,8 @@ func TestService_ServerTLSConfig(t *testing.T) { // After some time, both root and leaves should be different but both should // still be correct. oldRootSubjects := bytes.Join(tlsCfg.RootCAs.Subjects(), []byte(", ")) - oldLeafSerial := connect.HexString(cert.SerialNumber.Bytes()) - oldLeafKeyID := connect.HexString(cert.SubjectKeyId) + oldLeafSerial := cert.SerialNumber + oldLeafKeyID := cert.SubjectKeyId retry.Run(t, func(r *retry.R) { updatedCfg := service.ServerTLSConfig() @@ -198,13 +198,13 @@ func TestService_ServerTLSConfig(t *testing.T) { cert, err := x509.ParseCertificate(leaf.Certificate[0]) r.Check(err) - if oldLeafSerial == connect.HexString(cert.SerialNumber.Bytes()) { + if oldLeafSerial.Cmp(cert.SerialNumber) == 0 { r.Fatalf("leaf certificate should have changed, got serial %s", - oldLeafSerial) + connect.EncodeSerialNumber(oldLeafSerial)) } - if oldLeafKeyID == connect.HexString(cert.SubjectKeyId) { + if bytes.Equal(oldLeafKeyID, cert.SubjectKeyId) { r.Fatalf("leaf should have a different key, got matching SubjectKeyID = %s", - oldLeafKeyID) + connect.HexString(oldLeafKeyID)) } }) } diff --git a/connect/tls.go b/connect/tls.go index fb60d122b0c4..8f41c8307854 100644 --- a/connect/tls.go +++ b/connect/tls.go @@ -235,7 +235,7 @@ func newServerSideVerifier(client *api.Client, serviceName string) verifierFunc req := &api.AgentAuthorizeParams{ Target: serviceName, ClientCertURI: certURI.URI().String(), - ClientCertSerial: connect.HexString(leaf.SerialNumber.Bytes()), + ClientCertSerial: connect.EncodeSerialNumber(leaf.SerialNumber), } resp, err := client.Agent().ConnectAuthorize(req) if err != nil { diff --git a/tlsutil/generate.go b/tlsutil/generate.go index b164dca1b643..bd4b727d4ed4 100644 --- a/tlsutil/generate.go +++ b/tlsutil/generate.go @@ -10,11 +10,11 @@ import ( "crypto/x509/pkix" "encoding/pem" "fmt" - "github.com/hashicorp/consul/agent/connect" "math/big" "net" - "strings" "time" + + "github.com/hashicorp/consul/agent/connect" ) // GenerateSerialNumber returns random bigint generated with crypto/rand @@ -144,7 +144,7 @@ func keyID(raw interface{}) ([]byte, error) { // String formatted kID := sha256.Sum256(bs) - return []byte(strings.Replace(fmt.Sprintf("% x", kID), " ", ":", -1)), nil + return kID[:], nil } func parseCert(pemValue string) (*x509.Certificate, error) {