connect: don't colon-hex-encode the AuthorityKeyId and SubjectKeyId f…

…ields in connect certs

The fields in the certs are meant to hold the original binary
representation of this data, not some ascii-encoded version.

The only time we should be colon-hex-encoding fields is for display
purposes or marshaling through non-TLS mediums (like RPC).

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

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})

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).

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
+}

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
 

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,

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,

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)
 		}
 	}
 }

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",

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{

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))
 		}
 	})
 }

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 {

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) {