Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

connect: connect CA Roots in the primary datacenter should use a SigningKeyID derived from their local intermediate #9428

Merged
merged 7 commits into from
Feb 8, 2021

Conversation

rboyer
Copy link
Member

@rboyer rboyer commented Dec 17, 2020

This fixes an issue where leaf certificates issued in primary
datacenters using Vault as a Connect CA would be reissued very
frequently (every ~20 seconds) because the logic meant to detect root
rotation was errantly triggering.

The hash of the rootCA was being compared against a hash of the
intermediateCA and always failing. This doesn't apply to the Consul
built-in CA provider because there is no intermediate in use in the
primary DC.

This is reminiscent of #6513.

@rboyer rboyer added type/bug Feature does not function as expected theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies backport/1.8 labels Dec 17, 2020
@rboyer rboyer requested review from kyhavlov and a team December 17, 2020 21:24
@rboyer rboyer self-assigned this Dec 17, 2020
@github-actions github-actions bot removed the theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies label Dec 17, 2020
@rboyer
Copy link
Member Author

rboyer commented Dec 17, 2020

This needs to be backported to 1.9, 1.8, and 1.7. The 1.7 backport will need to be manual.

agent/proxycfg/state.go Outdated Show resolved Hide resolved
agent/consul/leader_connect_ca.go Show resolved Hide resolved
agent/consul/leader_connect_test.go Outdated Show resolved Hide resolved
@ravitejb
Copy link

@rboyer is there any chance that we can include this fix in the upcoming release?

@rboyer rboyer force-pushed the primary-leaf-churn-vault branch from 9434d0e to 1f4f4bc Compare January 25, 2021 23:27
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging January 25, 2021 23:28 Inactive
@vercel vercel bot temporarily deployed to Preview – consul January 25, 2021 23:28 Inactive
@vercel vercel bot temporarily deployed to Preview – consul January 26, 2021 21:47 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging January 26, 2021 21:47 Inactive
Copy link
Contributor

@kyhavlov kyhavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one comment about version numbers 👍

agent/consul/leader_connect_ca.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – consul February 8, 2021 17:22 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 8, 2021 17:22 Inactive
rboyer and others added 7 commits February 8, 2021 11:22
…ingKeyID derived from their local intermediate

This fixes an issue where leaf certificates issued in primary
datacenters using Vault as a Connect CA would be reissued very
frequently (every ~20 seconds) because the logic meant to detect root
rotation was errantly triggering.

The hash of the rootCA was being compared against a hash of the
intermediateCA and always failing. This doesn't apply to the Consul
built-in CA provider because there is no intermediate in use in the
primary DC.

This is reminiscent of #6513.
Co-authored-by: Freddy <freddygv@users.noreply.github.com>
@rboyer rboyer force-pushed the primary-leaf-churn-vault branch from 55cc4c0 to 89d8f85 Compare February 8, 2021 17:24
@vercel vercel bot temporarily deployed to Preview – consul February 8, 2021 17:24 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 8, 2021 17:24 Inactive
@rboyer rboyer merged commit 39e4ae2 into master Feb 8, 2021
@rboyer rboyer deleted the primary-leaf-churn-vault branch February 8, 2021 19:18
@hashicorp-ci
Copy link
Contributor

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

@hashicorp-ci
Copy link
Contributor

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

@hashicorp-ci
Copy link
Contributor

🍒❌ Cherry pick of commit 39e4ae2 onto release/1.8.x failed! Build Log

rboyer added a commit that referenced this pull request Feb 8, 2021
…ingKeyID derived from their local intermediate (#9428)

1.9.x backport of #9428
rboyer added a commit that referenced this pull request Feb 8, 2021
…ingKeyID derived from their local intermediate (#9428)

1.8.x backport of #9428
rboyer added a commit that referenced this pull request Feb 8, 2021
…ingKeyID derived from their local intermediate (#9428)

1.7.x backport of #9428
rboyer added a commit that referenced this pull request Feb 9, 2021
…ingKeyID derived from their local intermediate (#9428) (#9733)

1.9.x backport of #9428
rboyer added a commit that referenced this pull request Feb 9, 2021
…ingKeyID derived from their local intermediate (#9428) (#9734)

1.8.x backport of #9428
rboyer added a commit that referenced this pull request Feb 10, 2021
…ingKeyID derived from their local intermediate (#9428) (#9735)

1.7.x backport of #9428
dizzyup pushed a commit that referenced this pull request Apr 21, 2021
…ingKeyID derived from their local intermediate (#9428) (#9733)

1.9.x backport of #9428
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Feature does not function as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants