connect/ca: ensure edits to the key type/bits for the connect builtin CA will regenerate the roots #10330
Conversation
rboyer
added
the
theme/connect
| @@ -63,8 +69,7 @@ func (c *ConsulProvider) Configure(cfg ProviderConfig) error { | |||
| return err | |||
| } | |||
| c.config = config | |||
| hash := sha256.Sum256([]byte(fmt.Sprintf("%s,%s,%v", config.PrivateKey, config.RootCert, cfg.IsPrimary))) | |||
| c.id = connect.HexString(hash[:]) | |||
| c.id = hexStringHash(fmt.Sprintf("%s,%s,%s,%d,%v", config.PrivateKey, config.RootCert, config.PrivateKeyType, config.PrivateKeyBits, cfg.IsPrimary)) | |||
There was a problem hiding this comment.
This id is used to key the builtin proxy configuration in the state store (much like a cache-key). Adding the key type/bits here allows it to have two builtin proxy roots defined simultaneously during the rotation.
There was a problem hiding this comment.
At first I thought RootCert should have already been unique here, but after reading the tests I realize now that this input struct is a ConsulCAProviderConfig , not the CARoots, which means in most cases when PrivateKey{Type,Bits} are set, RootCert would be the zero value.
rboyer
force-pushed
the
ca-keyconfig-builtin
branch
from
7fe5b0c
to
092da53
Compare
There was a problem hiding this comment.
LGTM! This seems like it should fix the bug with the Consul CA provider.
I also wonder how we could prevent bugs like this in the future. We could write a test, similar to #9556, which verifies that all the important fields are considered in the hash. We can ignore any existing fields that we don't need in the hash.
| @@ -63,8 +69,7 @@ func (c *ConsulProvider) Configure(cfg ProviderConfig) error { | |||
| return err | |||
| } | |||
| c.config = config | |||
| hash := sha256.Sum256([]byte(fmt.Sprintf("%s,%s,%v", config.PrivateKey, config.RootCert, cfg.IsPrimary))) | |||
| c.id = connect.HexString(hash[:]) | |||
| c.id = hexStringHash(fmt.Sprintf("%s,%s,%s,%d,%v", config.PrivateKey, config.RootCert, config.PrivateKeyType, config.PrivateKeyBits, cfg.IsPrimary)) | |||
There was a problem hiding this comment.
At first I thought RootCert should have already been unique here, but after reading the tests I realize now that this input struct is a ConsulCAProviderConfig , not the CARoots, which means in most cases when PrivateKey{Type,Bits} are set, RootCert would be the zero value.
| }{ | ||
| {connect.DefaultPrivateKeyType, connect.DefaultPrivateKeyBits}, | ||
| {"ec", 256}, | ||
| {"ec", 384}, | ||
| {"rsa", 2048}, | ||
| {"rsa", 4096}, | ||
| } | ||
|
|
||
| for _, src := range types { | ||
| for _, dst := range types { |
There was a problem hiding this comment.
I wish we could test these permutations without having to spin up a full server. Hopefully after a few changes like #10445 we'll be able to test this with a full persistence backend, but without serf and all the other parts that aren't used here.
… CA will regenerate the roots progress on #9572
rboyer
force-pushed
the
ca-keyconfig-builtin
branch
from
7aeb42b
to
9950662
Compare
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/408197. |
progress on #9572