Prevent recursive hex encoding of keys

[tradel]

When comparing the signing keys on CA certs, Consul converts the bytes to a hex string. Unfortunately it appears that in some circumstances the string is double encoded.

For example, the following output was observed from the /v1/connect/ca/roots endpoint:

33:37:3a:61:34:3a:63:33:3a:65:34:3a:38:35:3a:63:30:3a:61:30:3a:34:34:3a:64:63:3a:35:65:3a:61:66:3a:65:30:3a:61:37:3a:61:35:3a:38:32:3a:34:36:3a:31:66:3a:36:39:3a:65:32:3a:30:63:3a:39:30:3a:62:62:3a:62:38:3a:64:62:3a:37:66:3a:33:37:3a:34:30:3a:34:61:3a:62:66:3a:30:39:3a:37:61:3a:64:35

Decoding this string reveals:

37:a4:c3:e4:85:c0:a0:44:dc:5e:af:e0:a7:a5:82:46:1f:69:e2:0c:90:bb:b8:db:7f:37:40:4a:bf:09:7a:d5

So the key was encoded more than once. This PR stops that by checking to see if the input is already a hex string before encoding it again.

[banks]

Great job and thanks for the PR @tradel.

Do we know which code paths are double encoding?

I don't think we store fields in any other format so it seems like a simple bug if we are accidentally encoding twice somewhere that should be fixed at root (no pun intended) rather than making the function use heuristics to prevent double application.

If there is some really complex corner case where we aren't sure if input from the user is encoded yet or not I could see the argument for this kind of change but IIRC we do all of this internally and only in a handful of places so can we just write tests to make sure they don't double encode?

[tradel]

Good point @banks. I didn't spend a lot of time trying to track it down how it was happening, when the fix took me half a minute to write. I can dig deeper if you like.

[banks]

If you have time to look then feel free seems like we need a fix. If not maybe we can get this into the backlog for someone else to look at.

[rboyer]

I modified the connect.HexString function to panic and ran make test. Here are some places it triggered:

panic: input is already a hex string!

goroutine 410 [running]:
github.com/hashicorp/consul/agent/connect.HexString(0xc00122f4a0, 0x5f, 0x5f, 0x1, 0x1)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/parsing.go:123 +0x13f
github.com/hashicorp/consul/agent/consul.parseCARoot(0xc0012a2700, 0x36b, 0x2e95f71, 0x6, 0xc0007f0571, 0x24, 0x0, 0xc000bf82a0, 0x60)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader_connect.go:88 +0x37c
github.com/hashicorp/consul/agent/consul.(*Server).initializeRootCA(0xc000089500, 0x344b0c0, 0xc000bf82a0, 0xc000160ac0, 0x0, 0x0)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader_connect.go:212 +0x3bd
github.com/hashicorp/consul/agent/consul.(*Server).initializeCA(0xc000089500, 0xc0000aff80, 0x0)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader_connect.go:195 +0x6f1
github.com/hashicorp/consul/agent/consul.(*Server).establishLeadership(0xc000089500, 0x2, 0x2)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader.go:321 +0x163
github.com/hashicorp/consul/agent/consul.(*Server).leaderLoop(0xc000089500, 0xc0008a84e0)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader.go:173 +0x985
github.com/hashicorp/consul/agent/consul.(*Server).monitorLeadership.func1(0xc00071c080, 0xc000089500, 0xc0008a84e0)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader.go:77 +0x5b
created by github.com/hashicorp/consul/agent/consul.(*Server).monitorLeadership
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader.go:75 +0x414

	panic: input is already a hex string!

goroutine 31 [running]:
testing.tRunner.func1(0xc0003e6d00)
	/usr/local/go/src/testing/testing.go:830 +0x392
panic(0x14844a0, 0x18d4050)
	/usr/local/go/src/runtime/panic.go:522 +0x1b5
github.com/hashicorp/consul/agent/connect.HexString(0xc0004466c0, 0x5f, 0x60, 0xc000432bd0, 0xc0004466c0)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/parsing.go:123 +0x13f
github.com/hashicorp/consul/agent/connect.testCA(0x1953360, 0xc0003e6d00, 0x0, 0x16c8e3a, 0x2, 0x100, 0xa0e0da)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/testing_ca.go:38 +0x1e0
github.com/hashicorp/consul/agent/connect.TestCA(...)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/testing_ca.go:133
github.com/hashicorp/consul/agent/cache-types.TestConnectCALeaf_changingRoots(0xc0003e6d00)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/cache-types/connect_ca_leaf_test.go:155 +0x178
testing.tRunner(0xc0003e6d00, 0x17659e0)
	/usr/local/go/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:916 +0x35a

	panic: input is already a hex string!

goroutine 90 [running]:
testing.tRunner.func1(0xc00026a900)
	/usr/local/go/src/testing/testing.go:830 +0x392
panic(0x9900c0, 0xb1c8c0)
	/usr/local/go/src/runtime/panic.go:522 +0x1b5
github.com/hashicorp/consul/agent/connect.HexString(0xc0001b6600, 0x5f, 0x60, 0xc00014a060, 0xc0001b6600)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/parsing.go:123 +0x13f
github.com/hashicorp/consul/agent/connect.testCA(0xb3daa0, 0xc00026a900, 0x0, 0xa61ad7, 0x3, 0x800, 0x4fde4d)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/testing_ca.go:38 +0x1e0
github.com/hashicorp/consul/agent/connect.TestCAWithKeyType(...)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/testing_ca.go:139
github.com/hashicorp/consul/agent/connect.TestSignatureMismatches.func1(0xc00026a900)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/generate_test.go:134 +0x94
testing.tRunner(0xc00026a900, 0xc000264160)
	/usr/local/go/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:916 +0x35a

	panic: input is already a hex string!

goroutine 21 [running]:
testing.tRunner.func1(0xc000158100)
	/usr/local/go/src/testing/testing.go:830 +0x392
panic(0x1008de0, 0x1382920)
	/usr/local/go/src/runtime/panic.go:522 +0x1b5
github.com/hashicorp/consul/agent/connect.HexString(0xc0007900c0, 0x5f, 0x60, 0xc0007690e0, 0xc0007900c0)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/parsing.go:123 +0x13f
github.com/hashicorp/consul/agent/connect.testCA(0x13cfc80, 0xc000158100, 0x0, 0x11d0d16, 0x2, 0x100, 0xc0000c7d80)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/testing_ca.go:38 +0x1e0
github.com/hashicorp/consul/agent/connect.TestCA(...)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/testing_ca.go:133
github.com/hashicorp/consul/agent/connect/ca.TestConsulCAProvider_Bootstrap_WithCert(0xc000158100)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/ca/provider_consul_test.go:104 +0xa8
testing.tRunner(0xc000158100, 0x1238578)
	/usr/local/go/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:916 +0x35a

	panic: input is already a hex string!

goroutine 450 [running]:
testing.tRunner.func1(0xc0004e8100)
	/usr/local/go/src/testing/testing.go:830 +0x392
panic(0xaac5e0, 0xc9e760)
	/usr/local/go/src/runtime/panic.go:522 +0x1b5
github.com/hashicorp/consul/agent/connect.HexString(0xc00001f260, 0x5f, 0x60, 0xc0002ef350, 0xc00001f260)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/parsing.go:123 +0x13f
github.com/hashicorp/consul/agent/connect.testCA(0xcc84c0, 0xc0004e8100, 0x0, 0xbbe105, 0x2, 0x100, 0xb544e0)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/testing_ca.go:38 +0x1e0
github.com/hashicorp/consul/agent/connect.TestCA(...)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/testing_ca.go:133
github.com/hashicorp/consul/agent/connect/ca/plugin.TestProvider_CrossSignCA.func1(0xc0004e8100, 0xc00029a230, 0x7f767e872178, 0xc00029c2a0)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/ca/plugin/plugin_test.go:231 +0xaa
github.com/hashicorp/consul/agent/connect/ca/plugin.testPlugin.func1(0xc0004e8100)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/ca/plugin/plugin_test.go:292 +0x20b
testing.tRunner(0xc0004e8100, 0xc0004d4070)
	/usr/local/go/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:916 +0x35a

panic: input is already a hex string!

goroutine 9 [running]:
github.com/hashicorp/consul/agent/connect.HexString(0xc000494420, 0x5f, 0x5f, 0x1, 0x1)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/parsing.go:123 +0x13f
github.com/hashicorp/consul/agent/consul.parseCARoot(0xc000168a80, 0x36b, 0x1910792, 0x6, 0x193f35e, 0x24, 0x0, 0xc0002b89c0, 0x60)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader_connect.go:88 +0x37c
github.com/hashicorp/consul/agent/consul.(*Server).initializeRootCA(0xc0005ba380, 0x1bf39e0, 0xc0002b89c0, 0xc00052a180, 0x0, 0x0)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader_connect.go:212 +0x3bd
github.com/hashicorp/consul/agent/consul.(*Server).initializeCA(0xc0005ba380, 0xc000536180, 0x0)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader_connect.go:195 +0x6f1
github.com/hashicorp/consul/agent/consul.(*Server).establishLeadership(0xc0005ba380, 0x2, 0x2)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader.go:321 +0x163
github.com/hashicorp/consul/agent/consul.(*Server).leaderLoop(0xc0005ba380, 0xc0000b4360)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader.go:173 +0x985
github.com/hashicorp/consul/agent/consul.(*Server).monitorLeadership.func1(0xc00069d370, 0xc0005ba380, 0xc0000b4360)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader.go:77 +0x5b
created by github.com/hashicorp/consul/agent/consul.(*Server).monitorLeadership
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader.go:75 +0x414

One of the above trips because rootCert.SubjectKeyId is already a hex encoded byte slice:

	return &structs.CARoot{
		ID:                  id,
		Name:                fmt.Sprintf("%s CA Root Cert", strings.Title(provider)),
		SerialNumber:        rootCert.SerialNumber.Uint64(),
		SigningKeyID:        connect.HexString(rootCert.SubjectKeyId),
		ExternalTrustDomain: clusterID,
		NotBefore:           rootCert.NotBefore,
		NotAfter:            rootCert.NotAfter,
		RootCert:            pemValue,
		Active:              true,
	}, nil

Another seems to trip because HexString is run on the result of connect.KeyId() which itself hex encodes the key already. I'm sure there's more. I'm not entirely certain what is and is not actually happening correctly yet, and whether or not anything got persisted super wrong in the state store at all.

[rboyer]

Also if you fire up consul agent -dev:

....(normal startup logs)...
    2019/09/13 10:08:32 [INFO] consul: cluster leadership acquired
    2019/09/13 10:08:32 [INFO] consul: New leader elected: babyshark
panic: input is already a hex string!

goroutine 163 [running]:
github.com/hashicorp/consul/agent/connect.HexString(0xc0000be2a0, 0x5f, 0x5f, 0x1, 0x1)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/connect/parsing.go:125 +0x13f
github.com/hashicorp/consul/agent/consul.parseCARoot(0xc0006ec700, 0x36b, 0x2de0827, 0x6, 0xc000476750, 0x24, 0x0, 0xc000668120, 0x60)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader_connect.go:88 +0x37c
github.com/hashicorp/consul/agent/consul.(*Server).initializeRootCA(0xc0004ee700, 0x338d740, 0xc000668120, 0xc00002e440, 0x0, 0x0)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader_connect.go:212 +0x3bd
github.com/hashicorp/consul/agent/consul.(*Server).initializeCA(0xc0004ee700, 0xc000170840, 0x0)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader_connect.go:195 +0x6f1
github.com/hashicorp/consul/agent/consul.(*Server).establishLeadership(0xc0004ee700, 0x2, 0x2)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader.go:321 +0x163
github.com/hashicorp/consul/agent/consul.(*Server).leaderLoop(0xc0004ee700, 0xc0005af500)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader.go:173 +0x985
github.com/hashicorp/consul/agent/consul.(*Server).monitorLeadership.func1(0xc000314010, 0xc0004ee700, 0xc0005af500)
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader.go:77 +0x5b
created by github.com/hashicorp/consul/agent/consul.(*Server).monitorLeadership
	/home/rboyer/src/go/src/github.com/hashicorp/consul/agent/consul/leader.go:75 +0x414

[rboyer]

I've tried various upgrade scenarios using @tradel's original patch to make sure we aren't actually relying on the double colon-hex-encoded strings being present in certs:

1. dc1 + dc2 on 1.6.1 with a mesh deployed
2. update all nodes to include the original patch
3. verify mesh still works
4. destroy an agent, sidecar, and app
5. recreate that agent, sidecar, and app to trigger leaf cert regeneration and the signing procedure

also

1. dc1 on 1.6.1 with a root ca generated and persisted
2. dc1 upgraded with patch
3. dc1 functions, can issue leaf certs
4. dc2 on 1.6.1 can join the patched primary and replicate the root
5. dc2 on 1.6.1 can issue leaf certs

[rboyer]

Current digging has yielded that the SigningKeyID field seems to always be double encoded, even on the examples on the website:

website/source/api/agent/connect.html.md:      "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",
website/source/api/connect/ca.html.md:            "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",
website/source/docs/connect/ca.html.md:            "SigningKeyID": "31:39:3a:34:35:3a:38:62:3a:33:30:3a:61:31:3a:34:35:3a:38:34:3a:61:65:3a:32:33:3a:35:32:3a:64:62:3a:38:64:3a:31:62:3a:66:66:3a:61:39:3a:30:39:3a:64:62:3a:66:63:3a:32:61:3a:37:32:3a:33:39:3a:61:65:3a:64:61:3a:31:31:3a:35:33:3a:66:34:3a:33:37:3a:35:63:3a:64:65:3a:64:31:3a:36:38:3a:64:38",
website/source/docs/connect/ca.html.md:            "SigningKeyID": "31:39:3a:34:35:3a:38:62:3a:33:30:3a:61:31:3a:34:35:3a:38:34:3a:61:65:3a:32:33:3a:35:32:3a:64:62:3a:38:64:3a:31:62:3a:66:66:3a:61:39:3a:30:39:3a:64:62:3a:66:63:3a:32:61:3a:37:32:3a:33:39:3a:61:65:3a:64:61:3a:31:31:3a:35:33:3a:66:34:3a:33:37:3a:35:63:3a:64:65:3a:64:31:3a:36:38:3a:64:38",

Which process out to:

======================
SigningKeyID  = 31:39:3a:34:35:3a:38:62:3a:33:30:3a:61:31:3a:34:35:3a:38:34:3a:61:65:3a:32:33:3a:35:32:3a:64:62:3a:38:64:3a:31:62:3a:66:66:3a:61:39:3a:30:39:3a:64:62:3a:66:63:3a:32:61:3a:37:32:3a:33:39:3a:61:65:3a:64:61:3a:31:31:3a:35:33:3a:66:34:3a:33:37:3a:35:63:3a:64:65:3a:64:31:3a:36:38:3a:64:38
Decoded       = 19:45:8b:30:a1:45:84:ae:23:52:db:8d:1b:ff:a9:09:db:fc:2a:72:39:ae:da:11:53:f4:37:5c:de:d1:68:d8
DoubleDecoded = <data>
======================
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
Decoded       = 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
DoubleDecoded = <data>
======================
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
Decoded       = 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
DoubleDecoded = <data>

[rboyer]

So I thought this was a double-encoding issue with the SigningKeyID field, and that's surely how this initially presents itself. Depending upon the situation either the x509.Certificate field AuthorityKeyId or SubjectKeyId is encoded and saved as the SigningKeyID.

In the consul CA provider the bytes of either of those two fields are themselves just ascii colon-hex-encoded data. This implies the SigningKeyID field is double encoded. I took a stab at just not doing that anymore and it mostly works out. Instead of calling connect.HexString() those just get replaced with string() to convert the byte slices. It all looks correct right up until you use the vault CA provider. Then you discover the actual problem.

The x509.Certificate fields are supposed to contain binary data, not hex-encoded string data. A peek over in the vault pki stack code confirms this. So really the fix here (assuming it doesn't break Connect backwards compatibility across an upgrade) is to remove almost all calls to connect.HexString everywhere.

[rboyer]

Closing in favor of #6492