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

@@ -13,7 +13,6 @@ import (
 	"fmt"
 	"math/big"
 	"net/url"
-	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -65,7 +64,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) {

website/source/api/agent/connect.html.md

@@ -128,12 +128,15 @@ $ curl \
       "ID": "15:bf:3a:7d:ff:ea:c1:8c:46:67:6c:db:b8:81:18:36:ad:e5:d0:c7",
       "Name": "Consul CA Root Cert",
       "SerialNumber": 7,
-      "SigningKeyID": "31:66:3a:39:31:3a:63:61:3a:34:31:3a:38:66:3a:61:63:3a:36:37:3a:62:66:3a:35:39:3a:63:32:3a:66:61:3a:34:65:3a:37:35:3a:35:63:3a:64:38:3a:66:30:3a:35:35:3a:64:65:3a:62:65:3a:37:35:3a:62:38:3a:33:33:3a:33:31:3a:64:35:3a:32:34:3a:62:30:3a:30:34:3a:62:33:3a:65:38:3a:39:37:3a:35:62:3a:37:65",
+      "SigningKeyID": "1f:91:ca:41:8f:ac:67:bf:59:c2:fa:4e:75:5c:d8:f0:55:de:be:75:b8:33:31:d5:24:b0:04:b3:e8:97:5b:7e",
+      "ExternalTrustDomain": "a1499528-fbf6-df7b-05e5-ae81e1873fc4",
       "NotBefore": "2018-05-21T16:33:28Z",
       "NotAfter": "2028-05-18T16:33:28Z",
       "RootCert": "-----BEGIN CERTIFICATE-----\nMIICmDCCAj6gAwIBAgIBBzAKBggqhkjOPQQDAjAWMRQwEgYDVQQDEwtDb25zdWwg\nQ0EgNzAeFw0xODA1MjExNjMzMjhaFw0yODA1MTgxNjMzMjhaMBYxFDASBgNVBAMT\nC0NvbnN1bCBDQSA3MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAER0qlxjnRcMEr\niSGlH7G7dYU7lzBEmLUSMZkyBbClmyV8+e8WANemjn+PLnCr40If9cmpr7RnC9Qk\nGTaLnLiF16OCAXswggF3MA4GA1UdDwEB/wQEAwIBhjAPBgNVHRMBAf8EBTADAQH/\nMGgGA1UdDgRhBF8xZjo5MTpjYTo0MTo4ZjphYzo2NzpiZjo1OTpjMjpmYTo0ZTo3\nNTo1YzpkODpmMDo1NTpkZTpiZTo3NTpiODozMzozMTpkNToyNDpiMDowNDpiMzpl\nODo5Nzo1Yjo3ZTBqBgNVHSMEYzBhgF8xZjo5MTpjYTo0MTo4ZjphYzo2NzpiZjo1\nOTpjMjpmYTo0ZTo3NTo1YzpkODpmMDo1NTpkZTpiZTo3NTpiODozMzozMTpkNToy\nNDpiMDowNDpiMzplODo5Nzo1Yjo3ZTA/BgNVHREEODA2hjRzcGlmZmU6Ly8xMjRk\nZjVhMC05ODIwLTc2YzMtOWFhOS02ZjYyMTY0YmExYzIuY29uc3VsMD0GA1UdHgEB\n/wQzMDGgLzAtgisxMjRkZjVhMC05ODIwLTc2YzMtOWFhOS02ZjYyMTY0YmExYzIu\nY29uc3VsMAoGCCqGSM49BAMCA0gAMEUCIQDzkkI7R+0U12a+zq2EQhP/n2mHmta+\nfs2hBxWIELGwTAIgLdO7RRw+z9nnxCIA6kNl//mIQb+PGItespiHZKAz74Q=\n-----END CERTIFICATE-----\n",
       "IntermediateCerts": null,
       "Active": true,
+      "PrivateKeyType": "",
+      "PrivateKeyBits": 0,
       "CreateIndex": 8,
       "ModifyIndex": 8
     }

website/source/api/connect/ca.html.md

@@ -49,12 +49,15 @@ $ curl \
             "ID": "c7:bd:55:4b:64:80:14:51:10:a4:b9:b9:d7:e0:75:3f:86:ba:bb:24",
             "Name": "Consul CA Root Cert",
             "SerialNumber": 7,
-            "SigningKeyID": "32:64:3a:30:39:3a:35:64:3a:38:34:3a:62:39:3a:38:39:3a:34:62:3a:64:64:3a:65:33:3a:38:38:3a:62:62:3a:39:63:3a:65:32:3a:62:32:3a:36:39:3a:38:31:3a:31:66:3a:34:62:3a:61:36:3a:66:64:3a:34:64:3a:64:66:3a:65:65:3a:37:34:3a:36:33:3a:66:33:3a:37:34:3a:35:35:3a:63:61:3a:62:30:3a:62:35:3a:36:35",
+            "SigningKeyID": "2d:09:5d:84:b9:89:4b:dd:e3:88:bb:9c:e2:b2:69:81:1f:4b:a6:fd:4d:df:ee:74:63:f3:74:55:ca:b0:b5:65",
+            "ExternalTrustDomain": "a1499528-fbf6-df7b-05e5-ae81e1873fc4",
             "NotBefore": "2018-05-25T21:39:23Z",
             "NotAfter": "2028-05-22T21:39:23Z",
             "RootCert": "-----BEGIN CERTIFICATE-----\nMIICmDCCAj6gAwIBAgIBBzAKBggqhkjOPQQDAjAWMRQwEgYDVQQDEwtDb25zdWwg\nQ0EgNzAeFw0xODA1MjUyMTM5MjNaFw0yODA1MjIyMTM5MjNaMBYxFDASBgNVBAMT\nC0NvbnN1bCBDQSA3MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEq4S32Pu0/VL4\nG75gvdyQuAhqMZFsfBRwD3pgvblgZMeJc9KDosxnPR+W34NXtMD/860NNVJIILln\n9lLhIjWPQqOCAXswggF3MA4GA1UdDwEB/wQEAwIBhjAPBgNVHRMBAf8EBTADAQH/\nMGgGA1UdDgRhBF8yZDowOTo1ZDo4NDpiOTo4OTo0YjpkZDplMzo4ODpiYjo5Yzpl\nMjpiMjo2OTo4MToxZjo0YjphNjpmZDo0ZDpkZjplZTo3NDo2MzpmMzo3NDo1NTpj\nYTpiMDpiNTo2NTBqBgNVHSMEYzBhgF8yZDowOTo1ZDo4NDpiOTo4OTo0YjpkZDpl\nMzo4ODpiYjo5YzplMjpiMjo2OTo4MToxZjo0YjphNjpmZDo0ZDpkZjplZTo3NDo2\nMzpmMzo3NDo1NTpjYTpiMDpiNTo2NTA/BgNVHREEODA2hjRzcGlmZmU6Ly83ZjQy\nZjQ5Ni1mYmM3LTg2OTItMDVlZC0zMzRhYTUzNDBjMWUuY29uc3VsMD0GA1UdHgEB\n/wQzMDGgLzAtgis3ZjQyZjQ5Ni1mYmM3LTg2OTItMDVlZC0zMzRhYTUzNDBjMWUu\nY29uc3VsMAoGCCqGSM49BAMCA0gAMEUCIBBBDOWXWApx4S6bHJ49AW87Nw8uQ/gJ\nJ6lvm3HzEQw2AiEA4PVqWt+z8fsQht0cACM42kghL97SgDSf8rgCqfLYMng=\n-----END CERTIFICATE-----\n",
             "IntermediateCerts": null,
             "Active": true,
+            "PrivateKeyType": "",
+            "PrivateKeyBits": 0,
             "CreateIndex": 8,
             "ModifyIndex": 8
         }