[ca] Root rotation reconciliation loop in the CA server by cyli · Pull Request #2100 · moby/swarmkit

cyli · 2017-04-06T07:52:52Z

The CA server, when it detects a root rotation, will start a reconciliation loop that tells all nodes to rotate their TLS certificates, and once all are done, completes the root rotation.

~~This is stacked on top of #2097~~ Merged.

Addresses #1990.

Note that this does not throttle how quickly the nodes get TLS certificates yet. This should be throttled in the dispatcher, not the CA server. For instance, if it detects a root rotation, possibly it could delay the session message for every node by a random number of seconds. This seems more likely to correctly delay nodes than setting the issuance state in batches, since the dispatcher can set the delay based on the number of nodes connected to it.

Update: Was thinking through how to delay things on the dispatcher side, and we don't want to delay session messages just because of the certificate issuance change - in case other things such as peers or network keys have changed. So right now we batch up the updates to the nodes for certificate issuance changes, so that not all are rotated at once.

codecov · 2017-04-06T08:04:02Z

Codecov Report

Merging #2100 into master will increase coverage by 0.25%.
The diff coverage is 87.74%.

@@            Coverage Diff             @@
##           master    #2100      +/-   ##
==========================================
+ Coverage   59.32%   59.58%   +0.25%     
==========================================
  Files         117      118       +1     
  Lines       19362    19536     +174     
==========================================
+ Hits        11487    11641     +154     
  Misses       6556     6556              
- Partials     1319     1339      +20

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 358df31...0ba0da4. Read the comment docs.

aaronlehmann · 2017-04-06T17:18:55Z

+	s.mu.Unlock()
+
+	s.wg.Add(1)
+	logger := log.G(ctx).WithField("method", "(*Server).reconileNodeRootsAndCerts")


aaronlehmann · 2017-04-06T18:51:35Z

+	})
+}
+
+func (s *Server) reconileNodeRootsAndCerts() {


There is a typo in the name. reconile should be reconcile.

Ah, thanks! I was having trouble spotting that.

aaronlehmann · 2017-04-06T18:57:20Z

+		}
+
+		var signerCert []byte
+		if len(cluster.RootCA.RootRotation.CAKey) > 0 {


Don't we need to check that RootRotation is not nil?

The previous block checks that the root CA is the same as our expected root CA, modulo join tokens. The expected root CA is what one loop of the reconcileNodeRootsAndCerts attempts to converge on, and that loop checks to make sure the root rotation isn't nil. So if the cluster's root CA is the same as the root CA we are trying to finish a rotation for, then it should have a root rotation that isn't nil.

I should probably add a comment saying that at the top of this function, though.

Sorry, I don't follow. We are assigning cluster with cluster := store.GetCluster(tx, clusterID) above. It's possible something else changed the cluster in the store, so we can't rely on checks that happen before the GetCluster.

Another possibility is that there is a root rotation, but the certs in the root rotation have changed (because someone wants to rotate to yet a different cert). So for each cycle of the root rotation reconciliation, we store the (api).RootCA value we're trying to converge to, and ensure that all the nodes are converged on that particular one.

When we call finishRootRotation, we pass that saved RootCA value. If the cluster's RootCA value has changed in the meanwhile, we abort the update (and hence abort finishing the root rotation), because either someone else set RootRotation to nil, or they changed the values for RootRotation, so we want to wait for the next cycle of reconciliation.

Since that stored RootCA that we're trying to make sure we converge on has been checked to make sure it does have a RootRotation value, if it and the cluster's RootCA value are equivalent modulo join tokens (which must be true before we proceed with the update), then the cluster's RootCA must have the same non-nil RootRotation .

I get it now. I had missed the implications of the RootCAEqualStable check. I think this is fine as-is.

aaronlehmann · 2017-04-06T19:28:42Z

+	}, opsTimeout))
+}
+
+func TestSuccessfulRootRotation(t *testing.T) {


t.Parallel() please

aaronlehmann · 2017-04-06T19:28:48Z

+	}, opsTimeout))
+}
+
+func TestRepeatedRootRotation(t *testing.T) {


t.Parallel() please

aaronlehmann · 2017-04-06T20:47:44Z

 	return false
 }
+
+func (s *Server) finishRootRotation(expectedRootCA *api.RootCA) error {


Random cosmetic thought: If we make this take a store.Tx argument, it would remove a level of indentation. The call to finishRootRotation could be merged into the store.Batch above it.

aaronlehmann · 2017-04-06T20:53:05Z

I noticed that one of the integration tests takes a long time:

--- PASS: TestSuccessfulRootRotation (29.60s)
--- PASS: TestRepeatedRootRotation (7.09s)

If there's an easy way to speed it up, I think it would be worth the effort. It's difficult to balance exhaustive testing with fast test cyles, but I really like that the swarmkit integration testsuite can run in ~25s. It encourages people to run the tests early and frequently.

aaronlehmann · 2017-04-06T20:57:04Z

+			s.mu.Unlock()
+		}()
+
+		for {


Would it add a lot of complexity to do this as a Watch that monitors node creations, node updates, and cluster updates? (edit: and node deletions)

Basically, you'd have a map where the keys are the nodes that still need to rotate their certificates. When you see a node rotate to the new certificate, you remove it from the map. When the size of the map reaches 0, you've converged.

I don't think there's anything inherently wrong with this polling-based approach, but I kind of like event-driven models. It might start making a difference in very large clusters.

I can try. :D good thing is that we have tests already. We might be able to piggyback off of our existing watch of nodes and clusters.

I have created a mapping of all the nodes that is updated by events. On a root CA update, if the issuer has changed and there's a root rotation present, it will start a reconciliation loop similar to the previous one, except it no longer polls the store, it just checks its in-memory map of nodes.

…es all the nodes to have an IssuanceStateRotate to trigger all the nodes to get new certificates. When all the nodes have rotated their certificates to be signed by the desired issuer, complete root rotation. Signed-off-by: cyli <ying.li@docker.com>

Signed-off-by: cyli <ying.li@docker.com>

cyli · 2017-04-07T01:16:42Z

@aaronlehmann Not killing the leader in that long-running test seems to bring the time down to 7.90s

aaronlehmann · 2017-04-07T01:56:46Z

+		// close to the latest versions of all the nodes.  If not, the node will updated later and the
+		// next batch of updates should catch it.
+		for _, n := range toUpdate {
+			if err := batch.Update(func(tx store.Tx) error { return store.UpdateNode(tx, n) }); err != nil {


How about formatting it like this:

if err := batch.Update(func(tx store.Tx) error { return store.UpdateNode(tx, n) }); err != nil {

aaronlehmann · 2017-04-07T02:02:16Z

+						continue
+					}
+					iState := n.Certificate.Status.State
+					if iState != api.IssuanceStateRenew&iState && iState != api.IssuanceStatePending && iState != api.IssuanceStateRotate {


I think the &iState in here is unintentional

Oh nice, I didn't even see that, thanks

aaronlehmann · 2017-04-07T02:25:57Z

+		defer func() {
+			r.wg.Done()
+			r.mu.Lock()
+			r.isReconciling = false


Race: if runReconcilerLoop gets called after this goroutine decides to return, but before this lock is acquired and isReconciling is set to false.

One way to solve this would be to have a channel or context that can stop the goroutine, and to always stop an existing goroutine (if any) and then start a new one.

That makes sense - we only call this if something has changed.

Updated, PTAL

aaronlehmann · 2017-04-07T20:01:07Z

+
+	"bytes"
+
+	"reflect"


Cosmetic: some extra blank lines

aaronlehmann · 2017-04-07T20:02:43Z

+		return nil, errors.Wrap(err, "invalid certificate in cluster root CA object")
+	}
+	if len(issuerCerts) == 0 {
+		return nil, errors.Wrap(err, "invalid certificate in cluster root CA object")


err would be nil here

aaronlehmann · 2017-04-07T20:05:34Z

+func (r *rootRotationReconciler) DeleteNode(node *api.Node) {
+	if node == nil {
+		return
+	}


It looks like this function is never called with a nil node.

aaronlehmann · 2017-04-07T20:15:02Z

+				log.G(r.ctx).Infof("completed root rotation on cluster %s", r.clusterID)
+				return
+			}
+			log.G(r.ctx).WithError(err).Errorf("could not complete root rotation")


aaronlehmann · 2017-04-07T20:30:48Z

+			},
+		},
+		{
+			descr: ("If all nodes have the right TLS info or are already rotated (or are not members), the " +


aaronlehmann · 2017-04-07T20:38:54Z

+		// Directly update the nodes rather than get + update, and ignore version errors.  Since
+		// `rootRotationReconciler` should be hooked up to all node update/delete/create vents, we should have
+		// close to the latest versions of all the nodes.  If not, the node will updated later and the
+		// next batch of updates should catch it.


BTW, this is what the allocator should be doing, ideally. Right now it overwrites fields in the current version of the object, and that's bad.

aaronlehmann · 2017-04-07T20:39:37Z

+			if err := batch.Update(func(tx store.Tx) error {
+				return store.UpdateNode(tx, n)
+			}); err != nil {
+				log.G(r.ctx).WithError(err).Debugf("unable to update node %s to request a certificate rotation")


It would be a good idea to suppress these messages when it's just a version conflict.

aaronlehmann · 2017-04-07T20:43:01Z

+		log.WithField(ctx, "method", "(*Server).rootRotationReconciler"),
+		s.securityConfig.ClientTLSCreds.Organization(),
+		s.rootReconciliationRetryInterval,
+		s.store, s.lastSeenClusterRootCA, nodes)


I'd prefer to just inline the contents of newReconciler here. Passing so many things through function arguments loses clarity over initializing a structure directly.

aaronlehmann · 2017-04-07T20:53:09Z

+	currentRootCA    *api.RootCA
+	currentIssuer    IssuerInfo
+	allNodes         map[string]*api.Node
+	unconvergedNodes map[string]struct{}


I feel like it should be possible to only have unconvergedNodes map[string]*api.Node and reinitialize it from the store when we start a new root rotation, so in the steady state we aren't maintaining a copy of every node here, but I can't justify why that would be better, so I'm not asking for the change. Just thought I'd mention the idea.

I was trying to check the store as little as possible, but that makes sense to not have 2 copies of everything.

cyli · 2017-04-07T22:19:36Z

(still working)

cyli · 2017-04-07T22:48:03Z

@aaronlehmann I think I have addressed all of your comments, if you wouldn't mind taking another look whenever you are free :) (+ one typo)

aaronlehmann · 2017-04-08T00:06:01Z

+				return r.finishRootRotation(tx, loopRootCA)
+			})
+			if err == nil {
+				log.G(r.ctx).Infof("completed root rotation on cluster %s", r.clusterID)


Let's just say "completed root rotation" for now. We don't support multiple cluster objects yet so the cluster ID isn't meaningful to the user.

aaronlehmann · 2017-04-08T00:23:16Z

LGTM

ping @diogomonica

diogomonica · 2017-04-10T01:03:18Z

+)
+
+// IssuanceStateRotateBatchSize is the maximum number of nodes we'll tell to rotate their certificates in any given update
+const IssuanceStateRotateBatchSize = 30


If this is the maximum, shouldn't it have max in the name?

diogomonica · 2017-04-10T01:05:18Z

+	}, nil
+}
+
+var errRootRotationChanged = errors.New("target root rotation has changed")


Move the error declaration and struct before the IssuerFromAPIRootCA?

diogomonica · 2017-04-10T01:11:12Z

+		return
+	}
+	// If the issuer has changed, iterate through all the nodes to figure out which ones need rotation
+	if newRootCA.RootRotation != nil {


In what situation would we have an issuer mismatch, but no RootRotation?

If the root rotation were abandoned, for instance. Previously the issuer would have been the new root cert, but if before the root rotation finished someone rotated the desired cert back to the original cert, the root rotation could be done.

diogomonica · 2017-04-10T01:30:10Z

+	if hasIssuer(node, &r.currentIssuer) {
+		delete(r.unconvergedNodes, node.ID)
+	} else {
+		r.unconvergedNodes[node.ID] = node


In wich situation would a node not be in this r.unconvergedNodes map but UpdateNode gets called for it?

If everything is working correctly, then it's unlikely, but if you are rotating the root cert back and forth really quickly, it's possible that the root ca in the reconciler finishes updating before the root CA in the signer finishes updating, and the node could get a new TLS cert signed with the previous root.

Previously there was a bug where this update took too long, and blocked the signer root CA update, and the nodes got a few TLS cert updates that were signed with the wrong key, and it eventually recovered.

diogomonica · 2017-04-10T01:31:54Z

+			var toUpdate []*api.Node
+			for _, n := range r.unconvergedNodes {
+				iState := n.Certificate.Status.State
+				if iState != api.IssuanceStateRenew&iState && iState != api.IssuanceStatePending && iState != api.IssuanceStateRotate {


what is this IssuanceStateRenew&iState about?

Boo, I thought I fixed this already when @aaronlehmann pointed it out - I must have reverted, sorry.

diogomonica · 2017-04-10T01:35:17Z

+	}
+
+	var signerCert []byte
+	if len(cluster.RootCA.RootRotation.CAKey) > 0 {


Are we guaranteed to not have a nil cluster.RootCA and cluster.RootCA.RootRotation?

Yes, because we compare it to the expectedRootCA first, which is guaranteed not to be nil or have a nil RootRotation. If the current root CA is not equal to the expected one, we abort.

…ng the store for all nodes at intervals, rely on the cluster and node watches to update an in-memory mapping of the current nodes. At regular intervals, update the store to tell a throttled number of the unconverged nodes to rotate their certificates. Also, remove the leader rotation part of the root rotation integration test, as that takes a very long time. There are server tests to ensure that multiple CA servers running reconciliation loops, and starting a CA server from a stopped state, does not break root reconciliation. Signed-off-by: cyli <ying.li@docker.com>

cyli mentioned this pull request Apr 6, 2017

[feature] CA Root rotation #1990

Closed

10 tasks

cyli force-pushed the root-rotation-reconciliation-loop branch from 28e7bcb to 0ead5dc Compare April 6, 2017 08:03

aaronlehmann reviewed Apr 6, 2017

View reviewed changes

cyli force-pushed the root-rotation-reconciliation-loop branch 3 times, most recently from 0a4bcae to 25d69c8 Compare April 6, 2017 21:48

cyli added 2 commits April 6, 2017 17:53

Add root rotation integration tests

225bea5

Signed-off-by: cyli <ying.li@docker.com>

cyli force-pushed the root-rotation-reconciliation-loop branch 2 times, most recently from 8bc5c04 to 31e85fe Compare April 7, 2017 01:15

aaronlehmann reviewed Apr 7, 2017

View reviewed changes

cyli force-pushed the root-rotation-reconciliation-loop branch 3 times, most recently from ac8cb55 to 2a91941 Compare April 7, 2017 18:37

aaronlehmann reviewed Apr 7, 2017

View reviewed changes

Comment thread ca/reconciler.go Outdated

"bytes"

"reflect"

Copy link
Copy Markdown

Collaborator

aaronlehmann Apr 7, 2017

Choose a reason for hiding this comment

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

Cosmetic: some extra blank lines

aaronlehmann reviewed Apr 7, 2017

View reviewed changes

cyli mentioned this pull request Apr 7, 2017

WIP: [cli] Interactive root rotation #2104

Closed

2 tasks

cyli force-pushed the root-rotation-reconciliation-loop branch from 2a91941 to 65a0053 Compare April 7, 2017 21:59

cyli force-pushed the root-rotation-reconciliation-loop branch from 65a0053 to 0837aea Compare April 7, 2017 22:38

cyli force-pushed the root-rotation-reconciliation-loop branch from 0837aea to 2d3d90f Compare April 7, 2017 22:49

aaronlehmann reviewed Apr 8, 2017

View reviewed changes

cyli force-pushed the root-rotation-reconciliation-loop branch 2 times, most recently from 3b3ee0b to cb71dbd Compare April 8, 2017 05:17

diogomonica reviewed Apr 10, 2017

View reviewed changes

cyli force-pushed the root-rotation-reconciliation-loop branch from cb71dbd to 0ba0da4 Compare April 10, 2017 17:24

diogomonica merged commit f2bf2b1 into moby:master Apr 10, 2017

cyli deleted the root-rotation-reconciliation-loop branch April 10, 2017 17:59

Conversation

cyli commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyli Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Apr 6, 2017

Uh oh!

aaronlehmann Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyli commented Apr 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyli commented Apr 6, 2017 •

edited

Loading

codecov Bot commented Apr 6, 2017 •

edited

Loading

cyli Apr 6, 2017 •

edited

Loading

aaronlehmann Apr 6, 2017 •

edited

Loading

cyli commented Apr 7, 2017 •

edited

Loading