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

Persist one CA per cluster #467

Merged
merged 30 commits into from
Mar 15, 2019
Merged

Conversation

sebgl
Copy link
Contributor

@sebgl sebgl commented Mar 4, 2019

This PR sets up reconciliation for one CA per cluster, used to issue certificates to the cluster.

What we had before this PR:

  • one global CA per operator (for all managed clusters)
  • re-created on every operator restart, forcing rotation of all certs

What we have with this PR:

  • one CA per managed cluster
  • reconciled at every reconciliation iteration
  • persisted (both CA cert and private key) in the apiserver for reuse
  • when running the operator with make run, the CA cert will expire in 10 hours. Otherwise, default to 1 year.
  • CA is rotated if invalid, or if it expires in less than 1 day (1 hour in dev mode)

Related to #399, #459.
Fixes #457.

Copy link
Contributor

@thbkrkr thbkrkr left a comment

Choose a reason for hiding this comment

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

LGTM.
Left some nits.

operators/pkg/controller/common/certificates/ca.go Outdated Show resolved Hide resolved
operators/pkg/controller/elasticsearch/nodecerts/ca.go Outdated Show resolved Hide resolved
operators/pkg/controller/elasticsearch/driver/default.go Outdated Show resolved Hide resolved
operators/pkg/controller/elasticsearch/nodecerts/ca.go Outdated Show resolved Hide resolved
operators/pkg/controller/elasticsearch/nodecerts/ca.go Outdated Show resolved Hide resolved
operators/pkg/controller/elasticsearch/nodecerts/ca.go Outdated Show resolved Hide resolved
Copy link
Member

@nkvoll nkvoll left a comment

Choose a reason for hiding this comment

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

We're missing a part where time triggers a reconciliation loop iteration here. If everything is static, the certs will now silently expire and the cluster will enter the unknown state.

I think we also need two followup-issues:

  1. Re-use private keys if possible when renewing?
  2. Key wrapping.
  3. (this is not new from this PR, but) we still need to deal with overlaps in the ca file. E.g have both old/new certs available for a certain duration (or cross-sign?).

Copy link
Member

@nkvoll nkvoll left a comment

Choose a reason for hiding this comment

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

I think the review got borked a little by my use of the back-button. Comments are in the previous review

@sebgl
Copy link
Contributor Author

sebgl commented Mar 12, 2019

@nkvoll Re: trigger reconciliation loop before expiration. I think the controller-runtime defaults to triggering a reconciliation execution every 10 hours. I was implicitly relying on that to make sure certs do not expire.
But I guess it's cleaner to just trigger a reconciliation X time before the certificate expires. Not sure what's the best way to do that though (go func(){ time.After(X, enqueueReconcile(cluster))?)

Edit: handled with result.RequeueAfter.

@sebgl
Copy link
Contributor Author

sebgl commented Mar 12, 2019

Follow-up on private key rotation (should be an easy one): #507
Follow up on securing the private key (wrapping being one option there): #508
Follow-up on cross-signing certs during rotation: #509

@sebgl sebgl mentioned this pull request Mar 14, 2019
Copy link
Member

@nkvoll nkvoll left a comment

Choose a reason for hiding this comment

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

LGTM

-rotate-before etc doesn't sound like the best name, but since we couldn't come up with anything better, let's go with it. It should be easy to change if something more fitting pops up.

@sebgl sebgl merged commit 4f70b25 into elastic:master Mar 15, 2019
@pebrc pebrc mentioned this pull request Dec 2, 2019
2 tasks
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.

3 participants