[agent/dispatcher] Keep track of a node's TLS info and provide root CA changes for an agent by cyli · Pull Request #2077 · moby/swarmkit

cyli · 2017-03-30T18:37:17Z

This PR does the following:

Whenever a session is established with a dispatcher, the agent sends the current node's TLS info (the issuer of its TLS certificate, which root certs it trusts) as part of the node description to the dispatcher, which can update it in the store.
Whenever a node renews its TLS certificate and the cert's issuer has changed, or when the node changes the roots that it trusts, the session is restarted and the updated node TLS info sent to the dispatcher.
Whenever a dispatcher notices a root CA change, it pushes the new root certs down to the agent as part of the session message. The agent, if it detects a change from the root it trusts, will notify the node, which (if the node is a worker) will trust the new root instead.

If it's a manager, the manager will update the root CA on its own and doesn't need info from the dispatcher - however, this doesn't cause the agent to immediately re-establish a session. The periodic node update should pick it up, however.

Note that the tests will probably fail - also, the agent/dispatcher code is completely untested.

Part of #1990.

aaronlehmann · 2017-03-30T18:40:22Z

 	"sync"
 	"time"

+	"github.com/docker/distribution/digest"


Renamed to "github.com/opencontainers/go-digest"

aaronlehmann · 2017-03-30T18:40:45Z

 	// AssignmentChange is a set of changes to apply on this node.
 	repeated AssignmentChange changes = 4;
-}
+}


The last newline was removed from the file.

aaronlehmann · 2017-03-30T18:41:26Z

@@ -0,0 +1,90 @@
+package main


Looks like this was added by accident

cyli · 2017-03-30T18:41:31Z

Also note I am probably going to pull the security config changes out into a separate PR, if this approach generally makes sense

aaronlehmann · 2017-03-30T18:41:48Z

@@ -0,0 +1,10 @@
+-----BEGIN CERTIFICATE-----


This looks unwanted as well

Yep, sorry, these two have been removed.

aaronlehmann · 2017-03-30T18:46:24Z

+					CertIssuerPublicKey: newIssuer.PublicKey,
+					CertIssuerSubject:   newIssuer.Subject,
+					TrustRootHash:       newRootCA.Digest.String(),
+				}


I get worried about blocking writes like this that aren't wrapped in a select. We've had a few bugs on shutdown where writes to channels can hang. In this case, I think there will be problems if the agent shuts down at the moment we try to write on this channel. Let's make sure there's a strategy to ensure that either a channel write is guaranteed to have a goroutine receiving it, or there's another case in a select that handles the receiver going away.

aaronlehmann · 2017-03-30T18:48:24Z

+	NotifyTLSChange <-chan *api.NodeTLSInfo
+
+	// NotifyRootChange channel receives new trust root certificate changes from session assignments
+	NotifyRootChange chan<- []byte


I don't see any code that initializes this. Not sure if it's an oversight or if this part is just not written yet.

Now I do see it. Maybe it wasn't in the version I was looking at a few seconds ago.

(this should be in node.go - apologies, I had some of the changes partially stashed - should be pushed now)

aaronlehmann · 2017-03-30T18:50:44Z

 		}
 	}
+	if len(message.RootCA) > 0 && digest.FromBytes(message.RootCA).String() != currentTLSInfo.TrustRootHash {
+		a.config.NotifyRootChange <- message.RootCA


I believe this channel send is okay. The goroutine in node that receives from it won't return until the agent shuts down.

aaronlehmann · 2017-03-30T19:03:13Z

 	keyMgrUpdates, keyMgrCancel := d.keyMgrQueue.Watch()
 	defer keyMgrCancel()
+	rootCAUpdates, rootCACancel := d.rootCAQueue.Watch()
+	defer rootCACancel()


The dispatcher changes look fine overall, but I'm wondering if we might combine these three channels into one. Each queue spawns its own goroutine. We could send a structure over the single channel that has fields for managers, the node object, and the root CA. Having such a structure would also make it clearer what types are involved.

aaronlehmann · 2017-03-30T23:39:00Z

+	manager            *manager.Manager
+	notifyNodeChange   chan *api.Node        // used to send role updates from the dispatcher api on promotion/demotion
+	notifyTLSChange    chan *api.NodeTLSInfo // used to send tls info to the agent on TLS credential when the node changes its trust root
+	notifyRootCAChange chan []byte           // used to send role updates from the dispatcher api on root CA change


This doesn't seem to be initialized. I think it should be created in New like notifyNodeChange.

aaronlehmann · 2017-03-30T23:44:21Z

 				renewCert()
+			case newRoot := <-n.notifyRootCAChange:
+				n.Lock()
+				role := n.role


notifyNodeChange is a buffered channel. If the role is changed at the same time as the root CA, this case and the one above might happen in either order. It's possible to skip this root CA change even if we are no longer a manager, or to handle it even if we have just become a manager.

I feel like it would be safer to combine these two channels into a single channel, that passes a struct with a node pointer (which may be nil) and a new root (which may be empty).

I think that on managers, we should only skip the call to UpdateRootCA, and not the send on n.notifyTLSChange. Otherwise, the agent will continue to report to the dispatcher that it is using the old TLS root. I think there will be less potential for bugs if all nodes report up-to-date information to the dispatcher. It might simplify our logic for deciding when the root rotation is complete.

cyli · 2017-04-01T03:43:43Z

(still in progress)

diogomonica · 2017-04-02T22:43:31Z

+			return
+		}
+		// get the current node description
+		newNodeDescription, err := a.nodeDescriptionWithHostname(ctx, nodeTLSInfo)


so, a.nodeDescriptionWithHostname can return nil, nil? Shouldn't we simply return inside of the if err != nil?

Yes, I'm not entirely sure about this one...

diogomonica · 2017-04-02T22:44:09Z

+		if !reflect.DeepEqual(nodeDescription, newNodeDescription) {
+			nodeDescription = newNodeDescription
+			// close the session
+			log.G(ctx).Info("agent: found node update")


found sounds weird. Received?

It's actually sending the node update

diogomonica · 2017-04-02T22:47:23Z

+			log.G(ctx).WithError(err).Error("node configure failed")
+		}
+	}
+	if len(message.RootCA) > 0 && (nti == nil || digest.FromBytes(message.RootCA).String() != nti.TrustRootHash) {


Didn't we talk about not using hashes here, and instead compare the bytes of the pub key itself?

The agent doesn't actually have access to the root bytes - node is going to do that.

diogomonica · 2017-04-02T22:52:29Z

 	repeated EncryptionKey network_bootstrap_keys = 4;
+
+	// Which root certificates to trust
+	bytes RootCA = 5;


Why does the SessionMessage need a new RootCA field? Don't the nodes know exactly who to trust from the RootCA inside of the message that comes from the renewal of the certificate?

The NodeCertificateStatusResponse does not actually contain the root cert bytes, only the TLS certificate. We could add it to the response, but it seemed less expensive to push the trusted root cert down this way, rather than make the all the nodes have to contact the CA and all renew their TLS certificates a second time just to get the root cert.

Sounds good.

diogomonica · 2017-04-02T22:53:36Z

+
+message NodeTLSInfo {
+	// Information about which root certs the node trusts
+	string trust_root_hash = 1;


Why the inconsistency of using bytes for the current TLS cert, but a hash for the root hash?

Do you mean bytes on the SessionMessage vs the hash here? No particular reason to use the hash instead - I can just send the bytes. But on the SessionMessage it has to be bytes because that's how the node gets the new root certificate to trust.

diogomonica · 2017-04-02T22:55:26Z

 }

+// SetWatch sets a queue on which changes to security config (roots and certificate issuer info) are published
+func (s *SecurityConfig) SetWatch(q *watch.Queue) {


Why would we like to override the internal queue?

It seemed easier to set watch on the security config, so the setter can clean it up, as opposed to adding a cleanup function to the security config so the watch can be closed when it's done. If we prefer that I can add it instead, but it means a lot more changes to the tests, etc.

I don't see this used anywhere yet. I guess that's coming later?

Yes, sorry, this PR is still in progress

diogomonica · 2017-04-02T22:57:50Z

 	defer s.mu.Unlock()
-	return s.updateTLSCredentials(tlsKeyPair, issuerInfo)
+	err = s.updateTLSCredentials(tlsKeyPair, issuerInfo)
+	if err != nil && s.queue != nil {


Can we create a constructor for SecurityConfigs and always ensure there is a queue? (instead of sprinkling checks for nil queues throughout the code?

(see above comment w.r.t. closing the queue)

diogomonica · 2017-04-02T23:03:09Z

 	agent            *agent.Agent
 	manager          *manager.Manager
-	notifyNodeChange chan *api.Node // used to send role updates from the dispatcher api on promotion/demotion
+	notifyNodeChange chan *agent.NodeChanges // used to send role updates from the dispatcher api on promotion/demotion


Are we doing this (having node changes and TLSChanges separate) so that nodes don't rotate their certificates twice for a root rotation?

The nodeTLSChange channel is so that the agent can report a TLS change to the dispatcher after the security config has changed. The notifyNodeChange channel is for the dispatcher to notify the node that it needs to change something about it's TLS configuration (either renew the cert, because the node's status has changed, or replace its root certificate).

Maybe the comment could say "used by the agent to relay node updates from the dispatcher Session stream to (*Node).run"

aaronlehmann · 2017-04-03T16:53:17Z

 	"github.com/docker/swarmkit/ca"
 	"github.com/docker/swarmkit/ca/testutils"
 	"github.com/docker/swarmkit/connectionbroker"
+	raftutils "github.com/docker/swarmkit/manager/state/raft/testutils"


Non-blocking: at some point we should reorganize packages so that PollFuncWithTimeout is not in raftutils.

aaronlehmann · 2017-04-03T16:54:31Z

+		require.FailNow(t, fmt.Sprintf("starting agent errored with: %v", err))
+	case <-agent.Ready():
+	case <-time.After(5 * time.Second):
+		require.FailNow(t, "agent not ready withou 500 milliseconds")


aaronlehmann · 2017-04-03T16:56:25Z

+	}()
+	select {
+	case err := <-getErr:
+		require.FailNow(t, fmt.Sprintf("starting agent errored with: %v", err))


I believe the Sprintf is not necessary. It looks like FailNow can accept printf-style formatting arguments.

It can, but for some reason go vet does not like it :|

Hm... maybe I just didn't update my go vet - it seems to pass now.

aaronlehmann · 2017-04-03T16:58:18Z

-	return s.updateTLSCredentials(s.certificate, s.issuerInfo)
+	err := s.updateTLSCredentials(s.certificate, s.issuerInfo)
+	if err != nil && s.queue != nil {
+		s.queue.Publish(rootCA)


rootCA is only published on error?

Nope, that's an error in the logic. It will be fixed in the next commit.

aaronlehmann · 2017-04-03T17:08:34Z

 	manager          *manager.Manager
-	notifyNodeChange chan *api.Node // used to send role updates from the dispatcher api on promotion/demotion
+	notifyNodeChange chan *agent.NodeChanges // used to send role updates from the dispatcher api on promotion/demotion
+	notifyTLSChange  chan *api.NodeTLSInfo   // used to send tls info to the agent on TLS credential when the node changes its trust root


Where does this get initialized?

Sorry the node/security config parts of the PR aren't quite done - I was going to set this to be the watch channel instead.

aaronlehmann · 2017-04-03T17:10:52Z

+						CertIssuerPublicKey: newIssuer.PublicKey,
+						CertIssuerSubject:   newIssuer.Subject,
+						TrustRootHash:       newRootCA.Digest.String(),
+					}


This needs to be in a select along with <-agentDone to avoid a deadlock.

aaronlehmann · 2017-04-03T17:11:28Z

+				CertIssuerPublicKey: tlsIssuerInfo.PublicKey,
+				CertIssuerSubject:   tlsIssuerInfo.Subject,
+				TrustRootHash:       securityConfig.RootCA().Digest.String(),
+			}


There's a potential deadlock here if the agent exits before this goroutine does. Put this in a select along with <-agentDone.

codecov · 2017-04-03T22:15:44Z

Codecov Report

Merging #2077 into master will increase coverage by 2.26%.
The diff coverage is 83.56%.

@@            Coverage Diff             @@
##           master    #2077      +/-   ##
==========================================
+ Coverage   54.71%   56.98%   +2.26%     
==========================================
  Files         114      113       -1     
  Lines       19749    19783      +34     
==========================================
+ Hits        10806    11273     +467     
+ Misses       7659     7201     -458     
- Partials     1284     1309      +25

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 b9dc05b...d9aa599. Read the comment docs.

cyli · 2017-04-03T22:30:54Z

+)
+
+// TestExecutor is executor for integration tests
+type TestExecutor struct {


Note: TestExecutor and TestController are just the functions from integration/exec.go, modified so that we can inject arbitrary node descriptions.

cyli · 2017-04-03T22:50:40Z

Apologies for taking so long - @aaronlehmann @diogomonica I think this is finally done, if you wouldn't mind taking another look.

aaronlehmann · 2017-04-03T23:01:03Z

+		require.FailNow(t, "starting agent errored with: %v", err)
+	case <-agent.Ready():
+	case <-time.After(5 * time.Second):
+		require.FailNow(t, "agent not ready without 500 milliseconds")


The failure message looks questionable

aaronlehmann · 2017-04-03T23:04:09Z

-// updateTLSCredentials updates the client, server, and TLS credentials on a security config.  This function expects
-// something else to have taken out a lock on the SecurityConfig.
+// UpdateTLSCredentials updates the client, server, and TLS credentials on a security config.
+func (s *SecurityConfig) UpdateTLSCredentials(certificate *tls.Certificate, issuerInfo *IssuerInfo) error {


Is this meant to be used from other packages?

aaronlehmann · 2017-04-03T23:05:52Z

+			CertIssuerPublicKey: s.issuerInfo.PublicKey,
+			CertIssuerSubject:   s.issuerInfo.Subject,
+		})
+	}


It seems like both places that call updateTLSCredentials have this code just afterwards. Can it be folded into updateTLSCredentials?

Sure - I just updated it because just seemed a little weird to have the update be handled from a completely different function as well, but that's probably fine.

Hm... no, nm, that was weird... Fixing I guess again? :)

Oh wait, no, this comment just got moved to a weird place.

aaronlehmann · 2017-04-03T23:06:30Z

+			CertIssuerPublicKey: issuer.PublicKey,
+			CertIssuerSubject:   issuer.Subject,
+		}, nodeTLSInfo)
+	case <-time.After(200 * time.Millisecond):


I recommend a slightly higher timeout, perhaps 1 second.

aaronlehmann · 2017-04-03T23:06:36Z

+			CertIssuerPublicKey: issuer.PublicKey,
+			CertIssuerSubject:   issuer.Subject,
+		}, nodeTLSInfo)
+	case <-time.After(200 * time.Millisecond):


I recommend a slightly higher timeout, perhaps 1 second.

aaronlehmann · 2017-04-03T23:07:00Z


 	"golang.org/x/net/context"

+	"os"


os can go with the other standard library imports above.

aaronlehmann · 2017-04-03T23:07:25Z

        target: 0
    changes: false
+ignore:
+  -**/testutils


This file is missing a final newline.

Have added the newline to the end

aaronlehmann · 2017-04-03T23:13:41Z

+					// If the server is sending us a ForceRenewal State, renew
+					if nodeChanges.Node.Certificate.Status.State == api.IssuanceStateRotate {
+						renewCert()
+						continue


continue doesn't seem right here, as that would ignore a change to the root certificate.

Splitting this code into a separate functions for handling node changes and root certificate changes may help make this clearer, but I fully admit I probably wouldn't bother with this myself.

aaronlehmann · 2017-04-03T23:13:52Z

+					}
+					if n.role == role {
+						n.Unlock()
+						continue


continue doesn't seem right here.

aaronlehmann · 2017-04-03T23:23:03Z

 					n.Unlock()
-					continue
+
+					if role == ca.ManagerRole || bytes.Equal(nodeChanges.RootCert, securityConfig.RootCA().Certs) {


Might be worth a comment explaining that managers pick up root certificate changes through raft.

Suppose the manager stops running because it is demoted, but the role change is not seen yet (this is the <-m.RemovedFromRaft() case in runManager). Would we miss a root certificate update that comes just after this?

We would miss the immediate one, but the next session message that changes the role will also come with the desired root cert, and since it is different than the one currently in security config, it should update. E.g. it should eventually get the root certificate update, although perhaps not immediately.

But couldn't this potentially block the root rotation indefinitely?

Apologies, could you elaborate on what would block it? Is it because there might not be a new session message, even if the role has changed?

I misread what you said before. This sounds good.

aaronlehmann · 2017-04-04T00:36:48Z

LGTM

…TLS state, namely what roots the node trusts and the signer info of its current TLS certificate. Also update the session message to include the desired root CA for the node. Signed-off-by: cyli <ying.li@docker.com>

… certificate to all connected agents. Signed-off-by: cyli <ying.li@docker.com>

… can be notified of any changes. Signed-off-by: cyli <ying.li@docker.com>

…cription, and to restart the session if the information has changed. Signed-off-by: cyli <ying.li@docker.com>

…ave the root CA certs. Signed-off-by: cyli <ying.li@docker.com>

sets up a security config watch queue and passes the watch channel to the agent, so the agent can be notified of TLS updates. Also, when the agent notifies the node that the root CA has changed, node updates the TLS config if the node is a worker. Signed-off-by: cyli <ying.li@docker.com>

cyli · 2017-04-04T01:10:55Z

(rebased to make sure there are no semantic conflicts)