From 7121c78d34dd346a3aeb2c300c40d4b63281a53c Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 25 Nov 2021 13:24:35 -0500 Subject: [PATCH 1/4] ca: update godoc To clarify what to expect from the data stored in this field, and the behaviour of this function. --- agent/consul/leader_connect_ca.go | 10 ++++++---- agent/structs/connect_ca.go | 23 ++++++++++++++++------- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index 0a6b0b47a4..2cf6fce970 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -602,10 +602,12 @@ func (c *CAManager) getLeafSigningCertFromRoot(root *structs.CARoot) string { return root.IntermediateCerts[len(root.IntermediateCerts)-1] } -// secondaryInitializeIntermediateCA runs the routine for generating an intermediate CA CSR and getting -// it signed by the primary DC if the root CA of the primary DC has changed since the last -// intermediate. It should only be called while the state lock is held by setting the state -// to non-ready. +// secondaryInitializeIntermediateCA generates a Certificate Signing Request (CSR) +// for the intermediate CA that is used to sign leaf certificates in the secondary. +// The CSR is signed by the primary DC and then persisted in the state store. +// +// This method should only be called while the state lock is held by setting the +// state to non-ready. func (c *CAManager) secondaryInitializeIntermediateCA(provider ca.Provider, config *structs.CAConfiguration) error { activeIntermediate, err := provider.ActiveIntermediate() if err != nil { diff --git a/agent/structs/connect_ca.go b/agent/structs/connect_ca.go index f3d175d350..4b4549b7ae 100644 --- a/agent/structs/connect_ca.go +++ b/agent/structs/connect_ca.go @@ -76,9 +76,14 @@ type CARoot struct { // SerialNumber is the x509 serial number of the certificate. SerialNumber uint64 - // SigningKeyID is the ID of the public key that corresponds to the private - // key used to sign leaf certificates. Is is the HexString format of the - // raw AuthorityKeyID bytes. + // SigningKeyID is the connect.HexString encoded id of the public key that + // corresponds to the private key used to sign leaf certificates in the + // local datacenter. + // + // The value comes from x509.Certificate.SubjectKeyId of the local leaf + // signing cert. + // + // See https://www.rfc-editor.org/rfc/rfc3280#section-4.2.1.1 for more detail. SigningKeyID string // ExternalTrustDomain is the trust domain this root was generated under. It @@ -192,10 +197,14 @@ type IssuedCert struct { // This is encoded in standard hex separated by :. SerialNumber string - // CertPEM and PrivateKeyPEM are the PEM-encoded certificate and private - // key for that cert, respectively. This should not be stored in the - // state store, but is present in the sign API response. - CertPEM string `json:",omitempty"` + // CertPEM is a PEM encoded bundle of a leaf certificate, optionally followed + // by one or more intermediate certificates that will form a chain of trust + // back to a root CA. + // + // This field is not persisted in the state store, but is present in the + // sign API response. + CertPEM string `json:",omitempty"` + // PrivateKeyPEM is the PEM encoded private key associated with CertPEM. PrivateKeyPEM string `json:",omitempty"` // Service is the name of the service for which the cert was issued. From d0578c6dfc2709ce1e21d4443a1f4498762b78ef Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 25 Nov 2021 16:15:09 -0500 Subject: [PATCH 2/4] ca: extract the lookup of the active primary CA This method had only one caller, which always looked for the active root. This commit moves the lookup into the method to reduce the logic in the one caller. This is being done in preparation for a larger change. Keeping this separate so it is easier to see. The `storedRootID != primaryRoots.ActiveRootID` is being removed because these can never be different. The `storedRootID` comes from `provider.ActiveRoot`, the `primaryRoots.ActiveRootID` comes from the store `CARoot` from the primary. In both cases the source of the data is the primary DC. Technically they could be different if someone modified the provider outside of Consul, but that would break many things, so is not a supported flow. If these were out of sync because of ordering of events then the secondary will soon receive an update to `primaryRoots` and everything will be sorted out again. --- agent/consul/leader_connect_ca.go | 33 +++++++++++++++---------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index 2cf6fce970..6a6a2a7023 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -197,11 +197,18 @@ func (c *CAManager) secondarySetPrimaryRoots(newRoots structs.IndexedCARoots) { c.primaryRoots = newRoots } -func (c *CAManager) secondaryGetPrimaryRoots() structs.IndexedCARoots { +func (c *CAManager) secondaryGetActivePrimaryCARoot() (*structs.CARoot, error) { // TODO: this could be a different lock, as long as its the same lock in secondarySetPrimaryRoots c.stateLock.Lock() - defer c.stateLock.Unlock() - return c.primaryRoots + primaryRoots := c.primaryRoots + c.stateLock.Unlock() + + for _, root := range primaryRoots.Roots { + if root.ID == primaryRoots.ActiveRootID && root.Active { + return root, nil + } + } + return nil, fmt.Errorf("primary datacenter does not have an active root CA for Connect") } // initializeCAConfig is used to initialize the CA config if necessary @@ -658,25 +665,17 @@ func (c *CAManager) secondaryInitializeIntermediateCA(provider ca.Provider, conf } } - // Determine which of the provided PRIMARY representations of roots is the - // active one. We'll use this as a template to generate any new root - // representations meant for this secondary. - var newActiveRoot *structs.CARoot - primaryRoots := c.secondaryGetPrimaryRoots() - for _, root := range primaryRoots.Roots { - if root.ID == primaryRoots.ActiveRootID && root.Active { - newActiveRoot = root - break - } - } - if newActiveRoot == nil { - return fmt.Errorf("primary datacenter does not have an active root CA for Connect") + newActiveRoot, err := c.secondaryGetActivePrimaryCARoot() + if err != nil { + return err } + _ = storedRootID // TODO: will be removed in the next commit + // Get a signed intermediate from the primary DC if the provider // hasn't been initialized yet or if the primary's root has changed. needsNewIntermediate := false - if activeIntermediate == "" || storedRootID != primaryRoots.ActiveRootID { + if activeIntermediate == "" { needsNewIntermediate = true } From 0de7efb31687511855827cb52298efa5361f6bdf Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 25 Nov 2021 16:33:48 -0500 Subject: [PATCH 3/4] ca: remove unused provider.ActiveRoot call In the previous commit the single use of this storedRoot was removed. In this commit the original objective is completed. The Provider.ActiveRoot is being removed because 1. the secondary should get the active root from the Consul primary DC, not the provider, so that secondary DCs do not need to communicate with a provider instance in a different DC. 2. so that the Provider.ActiveRoot interface can be changed without impacting other code paths. --- agent/consul/leader_connect_ca.go | 61 +++++++------------------------ testrpc/wait.go | 1 + 2 files changed, 14 insertions(+), 48 deletions(-) diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index 6a6a2a7023..75ac1df5d4 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -621,48 +621,22 @@ func (c *CAManager) secondaryInitializeIntermediateCA(provider ca.Provider, conf return err } - var ( - storedRootID string - expectedSigningKeyID string - currentSigningKeyID string - activeSecondaryRoot *structs.CARoot - ) - if activeIntermediate != "" { - // In the event that we already have an intermediate, we must have - // already replicated some primary root information locally, so check - // to see if we're up to date by fetching the rootID and the - // signingKeyID used in the secondary. - // - // Note that for the same rootID the primary representation of the root - // will have a different SigningKeyID field than the secondary - // representation of the same root. This is because it's derived from - // the intermediate which is different in all datacenters. - storedRoot, err := provider.ActiveRoot() - if err != nil { - return err - } - - storedRootID, err = connect.CalculateCertFingerprint(storedRoot) - if err != nil { - return fmt.Errorf("error parsing root fingerprint: %v, %#v", err, storedRoot) - } + _, activeRoot, err := c.delegate.State().CARootActive(nil) + if err != nil { + return err + } + var currentSigningKeyID string + if activeRoot != nil { + currentSigningKeyID = activeRoot.SigningKeyID + } + var expectedSigningKeyID string + if activeIntermediate != "" { intermediateCert, err := connect.ParseCert(activeIntermediate) if err != nil { return fmt.Errorf("error parsing active intermediate cert: %v", err) } expectedSigningKeyID = connect.EncodeSigningKeyID(intermediateCert.SubjectKeyId) - - // This will fetch the secondary's exact current representation of the - // active root. Note that this data should only be used if the IDs - // match, otherwise it's out of date and should be regenerated. - _, activeSecondaryRoot, err = c.delegate.State().CARootActive(nil) - if err != nil { - return err - } - if activeSecondaryRoot != nil { - currentSigningKeyID = activeSecondaryRoot.SigningKeyID - } } newActiveRoot, err := c.secondaryGetActivePrimaryCARoot() @@ -670,12 +644,10 @@ func (c *CAManager) secondaryInitializeIntermediateCA(provider ca.Provider, conf return err } - _ = storedRootID // TODO: will be removed in the next commit - // Get a signed intermediate from the primary DC if the provider // hasn't been initialized yet or if the primary's root has changed. - needsNewIntermediate := false - if activeIntermediate == "" { + needsNewIntermediate := activeIntermediate == "" + if activeRoot != nil && newActiveRoot.ID != activeRoot.ID { needsNewIntermediate = true } @@ -694,14 +666,7 @@ func (c *CAManager) secondaryInitializeIntermediateCA(provider ca.Provider, conf } else { // Discard the primary's representation since our local one is // sufficiently up to date. - newActiveRoot = activeSecondaryRoot - } - - // Update the roots list in the state store if there's a new active root. - state := c.delegate.State() - _, activeRoot, err := state.CARootActive(nil) - if err != nil { - return err + newActiveRoot = activeRoot } // Determine whether a root update is needed, and persist the roots/config accordingly. diff --git a/testrpc/wait.go b/testrpc/wait.go index a872668cea..230f2f38a8 100644 --- a/testrpc/wait.go +++ b/testrpc/wait.go @@ -144,6 +144,7 @@ func WaitForTestAgent(t *testing.T, rpc rpcFn, dc string, options ...waitOption) // raft leadership is gained so WaitForLeader isn't sufficient to be sure that // the CA is fully initialized. func WaitForActiveCARoot(t *testing.T, rpc rpcFn, dc string, expect *structs.CARoot) { + t.Helper() retry.Run(t, func(r *retry.R) { args := &structs.DCSpecificRequest{ Datacenter: dc, From d57dec587801a7305eac3151706df3bbdeaec7d0 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 25 Nov 2021 18:09:23 -0500 Subject: [PATCH 4/4] ca: remove unnecessary var, and slightly reduce cyclo complexity `newIntermediate` is always equal to `needsNewIntermediate`, so we can remove the extra variable and use the original directly. Also remove the `activeRoot.ID != newActiveRoot.ID` case from an if, because that case is already checked above, and `needsNewIntermediate` will already be true in that case. This condition now reads a lot better: > Persist a new root if we did not have one before, or if generated a new intermediate. --- agent/consul/leader_connect_ca.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index 75ac1df5d4..51525f507e 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -657,12 +657,10 @@ func (c *CAManager) secondaryInitializeIntermediateCA(provider ca.Provider, conf needsNewIntermediate = true } - newIntermediate := false if needsNewIntermediate { if err := c.secondaryRenewIntermediate(provider, newActiveRoot); err != nil { return err } - newIntermediate = true } else { // Discard the primary's representation since our local one is // sufficiently up to date. @@ -671,7 +669,7 @@ func (c *CAManager) secondaryInitializeIntermediateCA(provider ca.Provider, conf // Determine whether a root update is needed, and persist the roots/config accordingly. var newRoot *structs.CARoot - if activeRoot == nil || activeRoot.ID != newActiveRoot.ID || newIntermediate { + if activeRoot == nil || needsNewIntermediate { newRoot = newActiveRoot } if err := c.persistNewRootAndConfig(provider, newRoot, config); err != nil {