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

Store primaries root in secondary after intermediate signature #6333

Merged
merged 2 commits into from
Aug 30, 2019

Conversation

mkeeler
Copy link
Member

@mkeeler mkeeler commented Aug 15, 2019

This ensures that the intermediate exists within the CA root stored in raft and not just in the CA provider state. This has the very nice benefit of actually outputting the intermediate cert within the ca roots HTTP/RPC endpoints.

This change means that if signing the intermediate fails it will not set the root within raft. So far I have not come up with a reason why that is bad. The secondary CA roots watch will pull the root again and go through all the motions. So as soon as getting an intermediate CA works the root will get set.

@mkeeler mkeeler added this to the 1.6.1 milestone Aug 15, 2019
@mkeeler mkeeler requested a review from a team August 15, 2019 20:41
Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

This makes total sense I think.

I'm trying to think if there was a specific reason we made the root active before we got the signing cert but I can't think of one since nothing can use it yet.

The only thing that would be changed that I can think of would be that while a secondary DC is bootstrapping, before this change it would already be able to accept connections from the primary (or another bootstrapped secondary) and validate them even before it's own secondary CA cert is signed.

But that doesn't seem like a big win or issue - signing the CA cert should happen in O(second) of the secondary cluster first coming up and only once for it's whole lifetime so I don't think it's significant that we might possibly reject connections we might have been able to validate for that extra few milliseconds on initial bootstrap - it's likely no workloads even exist in the cluster yet if the servers are only just bootstrapping anyway unless it's an upgrade to an existing cluster that enabled Connect so I think it's reasonable that you have to wait for CA init to complete before connect works!

@mkeeler
Copy link
Member Author

mkeeler commented Aug 16, 2019

@banks Those were my thoughts exactly. Even if a proxy could validate the cert of the incoming service the other end wouldn't be able to validate the cert in the secondary so the connection would fail anyways.

@rboyer rboyer closed this Aug 26, 2019
@rboyer rboyer reopened this Aug 26, 2019
@rboyer rboyer changed the base branch from release/1-6 to master August 26, 2019 17:53
This ensures that the intermediate exists within the CA root stored in raft and not just in the CA provider state. This has the very nice benefit of actually outputting the intermediate cert within the ca roots HTTP/RPC endpoints.

This change means that if signing the intermediate fails it will not set the root within raft. So far I have not come up with a reason why that is bad. The secondary CA roots watch will pull the root again and go through all the motions. So as soon as getting an intermediate CA works the root will get set.
@mkeeler mkeeler force-pushed the bugfix/store-ca-root-intermediates branch from a837026 to c97be45 Compare August 28, 2019 20:25
@mkeeler
Copy link
Member Author

mkeeler commented Aug 29, 2019

I keep trying to get CI to pass but other unrelated flaky tests are proving to be problematic.

I am not sure this is the full fix but it seems to help for me.
Copy link
Member

@hanshasselberg hanshasselberg left a comment

Choose a reason for hiding this comment

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

LGTM

@mkeeler mkeeler merged commit 42d6085 into master Aug 30, 2019
@freddygv freddygv deleted the bugfix/store-ca-root-intermediates branch August 30, 2019 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants