ca: fix storing the leaf signing cert with Vault provider #11671

dnephin · 2021-11-26T20:38:33Z

We were not saving the leaf signing cert on the CARoot when the Vault provider was initialized. This was only a problem for API users (not envoy and xDS) because this extra code was masking the bug.

In #11661 I'd like to remove that masking code (because it is unnecessary, and because we need to change the Provider interface, so fewer callers makes that easier).

This PR updates an existing test to catch the bug, then fixes the bug. I added a few more assertions to both the primary and secondary test cases to make sure we did not have a similar bug in other places.

dnephin · 2021-11-26T20:44:03Z

agent/consul/leader_connect_test.go

+	store := s1.caManager.delegate.State()
+	_, activeRoot, err := store.CARootActive(nil)
+	require.NoError(err)
+	require.Equal(intermediatePEM, activeRoot.LeafSigningCert())


This is the line that fails before the bug fix. The other test changes are to be sure it did not exist in other places.

dnephin · 2021-11-26T20:46:19Z

agent/consul/leader_connect_ca.go

-	if needsSigningKeyUpdate {
-		rootCA.SigningKeyID = expectedSigningKeyID
-	}


This line was moved up to the place where we set needsSigningKeyUpdate, to keep the logic together.

dnephin · 2021-11-26T20:48:38Z

agent/consul/leader_connect_ca.go

@@ -550,13 +563,10 @@ func (c *CAManager) primaryInitialize(provider ca.Provider, conf *structs.CAConf
 		rootCA.IntermediateCerts = activeRoot.IntermediateCerts
 		c.setCAProvider(provider, rootCA)

+		c.logger.Info("initialized primary datacenter CA from existing CARoot with provider", "provider", conf.Provider)


This is unrelated, but something I noticed while working on this.

I'm pretty sure that previously we would not log anything when a server initializes itself using an existing stored CARoot. I guess this would happen if the raft leader election on any cluster that already had a root setup.

I made the log message slightly different, so that it's clear this initialization was from an existing CARoot.

agent/structs/connect_ca.go

acpana · 2021-11-30T18:09:51Z

agent/consul/leader_connect_ca.go

-		c.logger.Info("Correcting stored SigningKeyID value", "previous", rootCA.SigningKeyID, "updated", expectedSigningKeyID)
+	if activeRoot != nil && rootUpdateRequired {
+		c.logger.Info("Correcting stored CARoot values",
+			"previous-signing-key", rootCA.SigningKeyID, "updated-signing-key", expectedSigningKeyID)


might've gotten confused by the control flows here but wouldn't rootCA.SigningKeyID & expectedSigningKeyID be the same by the time we reach lines 553-54?

Since on line 531 we set:

rootCA.SigningKeyID = expectedSigningKeyID

did I miss something obvious 🤔 ?

Ya, I found this if / else if to be not super obvious.

All of this logic (from about line 523) is about fixing existing data stored in raft and the state store. So line 531 is where we fix the local reference, but we still need to persist that fix back into raft with ApplyCARequest.

So here on line 553 it is correct in the local variable, but we are logging that we are about to (later on below the if / else if) persist it.

I would like to refactor this some more. #11687 makes some more changes in this area (for a different reason), and it moves the activeRoot call earlier in the function. I think once that is done we can probably refactor this to make it a bit more obvious that the logic is:

if nothingToFix { return } applyFixesToRaft return

Does that make sense, or am I missing something?

I think that makes sense and that I'm almost there!

So won't the logging actually log two identical values?

Ah, I see what you are saying now. Yes! I did mess up the logging. I've moved the log up to the if block where we check if it needs update, so it should log the correct thing now.

That allowed me to remove the else if and make it just an if, which is also nice.

agent/consul/leader_connect_test.go

acpana · 2021-11-30T18:18:30Z

Thanks for working on this 🎉 -- left a couple of comments but otherwise LGTM!

This will be used in a follow up commit.

We were not adding the local signing cert to the CARoot. This commit fixes that bug, and also adds support for fixing existing CARoot on upgrade. Also update the tests for both primary and secondary to be more strict. Check the SigningKeyID is correct after initialization and rotation.

As a method on the struct type this would not be safe to call without first checking c.isIntermediateUsedToSignLeaf. So for now, move this logic to the CAMananger, so that it is always correct.

acpana

This LGTM 🚀 !

acpana · 2021-12-02T18:42:30Z

agent/consul/leader_connect_ca.go

 		// This state shouldn't be possible to get into because we update the root and
 		// CA config in the same FSM operation.
 		if activeRoot.ID != rootCA.ID {
 			return fmt.Errorf("stored CA root %q is not the active root (%s)", rootCA.ID, activeRoot.ID)
 		}

+		// TODO: why doesn't this c.setCAProvider(provider, activeRoot) ?


hc-github-team-consul-core · 2021-12-02T21:03:02Z

🍒 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/513905.

hc-github-team-consul-core · 2021-12-02T21:03:06Z

🍒❌ Cherry pick of commit ff45810 onto release/1.10.x failed! Build Log

hc-github-team-consul-core · 2021-12-02T21:03:08Z

🍒❌ Cherry pick of commit ff45810 onto release/1.9.x failed! Build Log

…-intermediate ca: fix storing the leaf signing cert with Vault provider

dnephin added type/bug Feature does not function as expected theme/certificates Related to creating, distributing, and rotating certificates in Consul backport/1.9 labels Nov 26, 2021

github-actions bot removed the theme/certificates Related to creating, distributing, and rotating certificates in Consul label Nov 26, 2021

dnephin force-pushed the dnephin/ca-fix-storing-vault-intermediate branch from 3e2c10a to d155347 Compare November 26, 2021 20:42

vercel bot temporarily deployed to Preview – consul November 26, 2021 20:42 Inactive

vercel bot temporarily deployed to Preview – consul-ui-staging November 26, 2021 20:42 Inactive

dnephin commented Nov 26, 2021

View reviewed changes

dnephin requested a review from a team November 26, 2021 20:49

dnephin mentioned this pull request Nov 26, 2021

ca: remove one call to Provider.ActiveRoot #11661

Merged

dnephin changed the title ~~ca: fix storing of the leaf signing cert with Vault provider~~ ca: fix storing the leaf signing cert with Vault provider Nov 26, 2021

dnephin mentioned this pull request Nov 26, 2021

ca: set the correct SigningKeyID after config update with Vault provider #11672

Merged

dnephin commented Nov 29, 2021

View reviewed changes

agent/structs/connect_ca.go Outdated Show resolved Hide resolved

acpana reviewed Nov 30, 2021

View reviewed changes

agent/consul/leader_connect_test.go Outdated Show resolved Hide resolved

acpana reviewed Nov 30, 2021

View reviewed changes

agent/consul/leader_connect_test.go Show resolved Hide resolved

acpana reviewed Nov 30, 2021

View reviewed changes

agent/consul/leader_connect_test.go Outdated Show resolved Hide resolved

ca: add some godoc and func for finding leaf signing cert

32ef9c5

This will be used in a follow up commit.

dnephin force-pushed the dnephin/ca-fix-storing-vault-intermediate branch from d155347 to b631479 Compare December 1, 2021 00:26

vercel bot temporarily deployed to Preview – consul December 1, 2021 00:26 Inactive

vercel bot temporarily deployed to Preview – consul-ui-staging December 1, 2021 00:26 Inactive

dnephin added 2 commits December 2, 2021 12:42

ca: make getLeafSigningCertFromRoot safer

28a8a64

As a method on the struct type this would not be safe to call without first checking c.isIntermediateUsedToSignLeaf. So for now, move this logic to the CAMananger, so that it is always correct.

dnephin force-pushed the dnephin/ca-fix-storing-vault-intermediate branch from b631479 to 28a8a64 Compare December 2, 2021 17:42

vercel bot temporarily deployed to Preview – consul-ui-staging December 2, 2021 17:43 Inactive

vercel bot temporarily deployed to Preview – consul December 2, 2021 17:43 Inactive

acpana approved these changes Dec 2, 2021

View reviewed changes

dnephin merged commit ff45810 into main Dec 2, 2021

dnephin deleted the dnephin/ca-fix-storing-vault-intermediate branch December 2, 2021 21:02

dnephin added a commit that referenced this pull request Dec 2, 2021

Merge pull request #11671 from hashicorp/dnephin/ca-fix-storing-vault…

93e2074

…-intermediate ca: fix storing the leaf signing cert with Vault provider

dnephin mentioned this pull request Dec 2, 2021

[1.10.x] Backport of CA system bug fixes #11717

Merged

dnephin added a commit that referenced this pull request Dec 2, 2021

Merge pull request #11671 from hashicorp/dnephin/ca-fix-storing-vault…

2706dd9

…-intermediate ca: fix storing the leaf signing cert with Vault provider

This was referenced Dec 2, 2021

[1.9.x] Backport of CA system bug fixes #11718

Merged

ca: improve test coverage for RenewIntermediate #11780

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ca: fix storing the leaf signing cert with Vault provider #11671

ca: fix storing the leaf signing cert with Vault provider #11671

dnephin commented Nov 26, 2021 •

edited

dnephin Nov 26, 2021

dnephin Nov 26, 2021

dnephin Nov 26, 2021

acpana Nov 30, 2021 •

edited

dnephin Nov 30, 2021 •

edited

acpana Dec 1, 2021

dnephin Dec 2, 2021

acpana commented Nov 30, 2021

acpana left a comment

acpana Dec 2, 2021

hc-github-team-consul-core commented Dec 2, 2021

hc-github-team-consul-core commented Dec 2, 2021

hc-github-team-consul-core commented Dec 2, 2021

ca: fix storing the leaf signing cert with Vault provider #11671

ca: fix storing the leaf signing cert with Vault provider #11671

Conversation

dnephin commented Nov 26, 2021 • edited

dnephin Nov 26, 2021

Choose a reason for hiding this comment

dnephin Nov 26, 2021

Choose a reason for hiding this comment

dnephin Nov 26, 2021

Choose a reason for hiding this comment

acpana Nov 30, 2021 • edited

Choose a reason for hiding this comment

dnephin Nov 30, 2021 • edited

Choose a reason for hiding this comment

acpana Dec 1, 2021

Choose a reason for hiding this comment

dnephin Dec 2, 2021

Choose a reason for hiding this comment

acpana commented Nov 30, 2021

acpana left a comment

Choose a reason for hiding this comment

acpana Dec 2, 2021

Choose a reason for hiding this comment

hc-github-team-consul-core commented Dec 2, 2021

hc-github-team-consul-core commented Dec 2, 2021

hc-github-team-consul-core commented Dec 2, 2021

dnephin commented Nov 26, 2021 •

edited

acpana Nov 30, 2021 •

edited

dnephin Nov 30, 2021 •

edited