[ca] Extricate `ExternalCA` from `SecurityConfig` by cyli · Pull Request #2262 · moby/swarmkit

cyli · 2017-06-15T22:09:05Z

This is the first step towards splitting the CA server's signing functionality from SecurityConfig, which serves more for providing creds for mTLS. Doing so will help unify the root CA update paths in node/node.go.

The only node that ever needs the ExternalCA object is the leader that is running the CA server anyway, so it makes sense for it to go into the CA server. This also better makes it possible at a future point to provide separate creds for contacting the external CA server if we ever needed to do so.

codecov · 2017-06-15T22:19:02Z

Codecov Report

Merging #2262 into master will decrease coverage by 0.08%.
The diff coverage is 89.09%.

@@            Coverage Diff             @@
##           master    #2262      +/-   ##
==========================================
- Coverage   60.35%   60.27%   -0.09%     
==========================================
  Files         128      128              
  Lines       25979    25994      +15     
==========================================
- Hits        15679    15667      -12     
- Misses       8906     8936      +30     
+ Partials     1394     1391       -3

aaronlehmann · 2017-06-15T22:28:52Z

ca/server.go


+// ExternalCA returns the current external CA
+func (s *Server) ExternalCA() *ExternalCA {
+	// don't need a lock because this never gets updated


It does get updated in UpdateRootCA.

Ah right sorry, it wasn't in a previous version, and I forgot to update.

aaronlehmann · 2017-06-15T22:31:03Z

ca/server.go

 }

+// filterAndSaveExternalCAURLS saves a copy of the external CAs to the server, and also returns a list of external CA
+// urls filtered by the desired cert. This function assumes that something else has called s.secConfugMu.Lock() and


I guess it's more like saveAndFilter?

typo: secConfugMu

:D I should probably change that name now to signingMu or something.

aaronlehmann · 2017-06-15T22:38:28Z

ca/server.go

-	if rootCAChanged || externalCAChanged {
+	// we want to update only if the external CA URLS have changed, since if the root CA has changed we already
+	// run similar logic
+	if !rootCAChanged && externalCAChanged {


Could be else if externalCAChanged {, but fine with this if it's more readable to be redundant.

aaronlehmann · 2017-06-15T22:43:56Z

ca/server.go

+// will also unlock it
+func (s *Server) saveAndFilterExternalCAURLS(ctx context.Context, desiredCert, defaultCert []byte, apiExternalCAs []*api.ExternalCA) (urls []string) {
+	desiredCert = NormalizePEMs(desiredCert)
+	s.lastSeenExternalCAs = make([]*api.ExternalCA, len(apiExternalCAs))


I think this is missing a , 0

Oops, yes, sorry, although I actually just left this and instead of doing append, set s.lastSeenExternalCAs[i] = .... Don't feel strongly or anything, just in mind setting the values makes it slightly more clear that we're copying rather than creating something new, but that's just how I tend to read these things.

I have a slight preference for going back to the old code's s.lastSeenExternalCAs = cluster.Spec.CAConfig.Copy().ExternalCAs expression, moved outside this function.

The reason is that saving the URL list and filtering it are different operations, so I think keeping them separate is more understandable.

Makes sense

aaronlehmann · 2017-06-15T22:48:24Z

ca/server.go

+			Certificates: s.securityConfig.ClientTLSCreds.Config().Certificates,
+			RootCAs:      s.externalCAPool,
+			MinVersion:   tls.VersionTLS12,
+		}, urls...)


Can this use NewExternalCATLSConfig to avoid hardocding the minimum version?

aaronlehmann · 2017-06-15T22:48:46Z

manager/controlapi/ca_rotation.go

+			Certificates: securityConfig.ClientTLSCreds.Config().Certificates,
+			RootCAs:      rootPool,
+			MinVersion:   tls.VersionTLS12,
+		}, urls...).CrossSignRootCA(ctx, newCARootCA)


Can this use NewExternalCATLSConfig to avoid hardocding the minimum version?

cyli · 2017-06-20T01:28:32Z

I've added an interface to ca.Server that will update the external CA's TLS creds when the security config's TLS creds are updated. PTAL - will also be happy to split this up in to 2 or 3 PRs, since a lot of files have been changed.

aaronlehmann · 2017-06-20T01:34:08Z

ca/server.go

+
+// SecurityConfigTLSUpdater takes a security config and sets a watch so we can tell when TLS credentials have changed
+type SecurityConfigTLSUpdater struct {
+	s           *SecurityConfig


Let's call it securityConfig to avoid confusion with server.

aaronlehmann · 2017-06-20T01:35:23Z

ca/server.go

+	return s.s.ClientTLSCreds.Config().Certificates
+}
+
+// NotifyTLSChange starts watching for TLS changes in the security config, and returns a channel through which


Comment seems outdated

aaronlehmann · 2017-06-20T01:45:04Z

ca/server.go

+}
+
+// SecurityConfigTLSUpdater takes a security config and sets a watch so we can tell when TLS credentials have changed
+type SecurityConfigTLSUpdater struct {


I'm wondering if this abstraction is really useful. It's only used in one place, and it seems like calling Watch would be clearer and lower this risk of leaking the watch (because the cancel return value would be ignored in that case).

Is there an upcoming change that will make this more useful?

If we keep the abstraction, can we return a cancel function instead of having a Stop method?

I can pass a channel instead to the CA server notifying it when the TLS creds have been updated instead. But the SecurityConfig object already had a watch on it to notify the agent when the credentials or the root CA have changed so the agent session could restart. It seemed easier to bottleneck it through the SecurityConfig, but I could also instead try setting a private watch in the node.go code.

I was just asking if we need a SecurityConfigTLSUpdater rather than having the single place where it's used call the Watch method directly.

I was planning on removing the security config entirely from the CA server at a later point, as part of separating out the security config from the CA server, in which case I just needed some kind of interface from which I can get the updated external CA creds.

Perhaps it'd be better just to pass a channel?

I'd suggest removing the abstraction until we get to that point. Then we can see what makes most sense. Passing a channel may be best, especially if the lifecycle of the watch can be managed by something external to the CA server.

aaronlehmann · 2017-06-20T01:48:13Z

ca/config.go

 }

+// Close closes the security config, freeing up the watch queue resource
+func (s *SecurityConfig) Close() {


Not sure if it makes sense, but having NewSecurityConfig return a cancel function might make it harder to leak these. On the other hand, security configs might always be stored somewhere long-term, in which case it wouldn't help.

In general this change seems to be going in the right direction (I'm happy to see SetWatch go), but making the SecurityConfig something whose resources need to be consciously deallocated is unfortunate.

We mainly create the security config in the node/node.go code, and the watch gets cleaned up at the end of run anyway, so holding onto the cancel seems fine? Returning the cancel may make it harder to leak in tests, though, so: #2274

aaronlehmann · 2017-07-06T17:43:38Z

Coming back to this after awhile. Is it ready for review?

aaronlehmann · 2017-07-06T22:20:19Z

ca/server.go

 	lastSeenClusterRootCA *api.RootCA
 	lastSeenExternalCAs   []*api.ExternalCA
-	secConfigMu           sync.Mutex
+	signingMu             sync.Mutex


I'm not sure signingMu is the right name. It seems to protect s.externalCA, and some aspects of the securityConfig.

Let's try to articulate what this mutex is used for, and from that we can figure out if the name makes sense.

I guess it's supposed to protect the components of the CA server used to issue new certificates, so the rootCA object, and everything needed to connect to an external CA (hence the parts of securityConfig, because it has the TLS credentials needed to connect to the external CA)

Please add this in a comment :)

Ah ok, cool, I was hoping you might have a better name. :) I'm terrible at naming things, which is why I tend to end up with Java names (like RootCAAndExternalCAMutex or something)

aaronlehmann · 2017-07-06T22:24:08Z

ca/server.go

 }

+// ExternalCA returns the current external CA
+func (s *Server) ExternalCA() *ExternalCA {


I see this used in a unit test. Is it called anywhere else?

Nope, just the test. I can make the server test part of the ca package instead of the ca_test package to access it directly, if you prefer.

Update: oh, right import cycle issues. :| nm, I can't actually do that.

Please add a comment saying this is exposed to support unit testing and externalCA is nominally a private field.

cyli · 2017-07-14T16:39:47Z

@aaronlehmann Also, apologies, I spent a while trying to remember what thought I had regarding this PR and I no longer remember it. :) So yes, this is ready for review.

aaronlehmann · 2017-07-14T21:38:36Z

manager/controlapi/ca_rotation.go

+		rootPool.AppendCertsFromPEM(apiRootCA.CACert)
+		crossSignedCert, err = ca.NewExternalCA(nil,
+			ca.NewExternalCATLSConfig(securityConfig.ClientTLSCreds.Config().Certificates, rootPool),
+			urls...).CrossSignRootCA(ctx, newCARootCA)


Please break up this statement into two.

aaronlehmann · 2017-07-14T21:40:14Z

ca/server.go

 			if len(clusters) != 1 {
 				return errors.New("could not find cluster object")
 			}
-			s.UpdateRootCA(ctx, clusters[0]) // call once to ensure that the join tokens are always set


Making a note to close #2241 when this is merged.

There is one more place where we have to change this (in manager) before closing that.

aaronlehmann · 2017-07-14T21:45:59Z

ca/server.go

+		case <-externalTLSCredsChange:
+			s.signingMu.Lock()
+			s.externalCA.UpdateTLSConfig(NewExternalCATLSConfig(
+				s.securityConfig.ClientTLSCreds.Config().Certificates, s.externalCAPool))


It seems like this update to externalCA's TLS config is "eventually consistent", since we might pick up events from the updates channel between the time the credentials change and this is called. Will that cause any problems?

I suppose the use case here is node certificate renewals, so it seems reasonable to expect the old node certificate to keep working for that short interval. Root rotations might be a bit more of a corner case, but I'm not sure that's worth worrying about either.

Sorry for the delay in response! Yes, it is eventually consistent - during that period, signing nodes may fail, but in a recoverable way, so eventually the correct certs will be issued. I've added a comment to that effect.

diogomonica · 2017-07-19T20:59:27Z

Overall I'm good with the refactor 👍

…ficate, rather than a RootCA object. Signed-off-by: Ying Li <ying.li@docker.com>

…irst step toward separating the root CA object used to sign certificates on the CA server from the SecurityConfig, which is mainly used to maintain client and server TLS credentials. Signed-off-by: Ying Li <ying.li@docker.com>

SecurityConfig's TLS credentials change. Signed-off-by: Ying Li <ying.li@docker.com>

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

aaronlehmann reviewed Jun 15, 2017

View reviewed changes

cyli force-pushed the extricate-externalca-from-securityconfig branch from 2c6e06f to 73c0261 Compare June 15, 2017 22:36

aaronlehmann reviewed Jun 15, 2017

View reviewed changes

cyli force-pushed the extricate-externalca-from-securityconfig branch 4 times, most recently from b94298b to 4a0490f Compare June 20, 2017 01:10

cyli changed the title ~~WIP: [ca] Extricate ExternalCA from SecurityConfig~~ [ca] Extricate ExternalCA from SecurityConfig Jun 20, 2017

aaronlehmann reviewed Jun 20, 2017

View reviewed changes

aaronlehmann mentioned this pull request Jun 21, 2017

[ca] Refactor SecurityConfig to automatically create a watch.Queue #2274

Merged

cyli force-pushed the extricate-externalca-from-securityconfig branch from 4a0490f to 108a3f1 Compare June 30, 2017 23:09

aaronlehmann reviewed Jul 6, 2017

View reviewed changes

cyli force-pushed the extricate-externalca-from-securityconfig branch from 34f7bc5 to afac780 Compare July 14, 2017 16:37

cyli force-pushed the extricate-externalca-from-securityconfig branch from afac780 to 2658824 Compare July 14, 2017 16:52

aaronlehmann reviewed Jul 14, 2017

View reviewed changes

cyli added 3 commits July 19, 2017 14:56

Update ExternalCA to take intermediates to append to the signed certi…

e785072

…ficate, rather than a RootCA object. Signed-off-by: Ying Li <ying.li@docker.com>

Update the CA server's external CA's TLS credentials when the

0d8d7c9

SecurityConfig's TLS credentials change. Signed-off-by: Ying Li <ying.li@docker.com>

Remove ExternalCA.Copy() since it's no longer needed

22b5c13

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

cyli force-pushed the extricate-externalca-from-securityconfig branch from 2658824 to 22b5c13 Compare July 19, 2017 22:12

aaronlehmann approved these changes Jul 19, 2017

View reviewed changes

cyli merged commit 64aab74 into moby:master Jul 19, 2017

cyli deleted the extricate-externalca-from-securityconfig branch July 19, 2017 22:40

cyli mentioned this pull request Jul 29, 2017

[ca] Separate server signing root CA from security config root CA #2336

Merged

Conversation

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

Uh oh!

Uh oh!

codecov bot commented Jun 15, 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!

Choose a reason for hiding this comment

Uh oh!

cyli Jun 16, 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!

cyli commented Jun 20, 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!

aaronlehmann commented Jul 6, 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!

cyli commented Jul 14, 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!

cyli commented Jun 15, 2017 •

edited

Loading

codecov bot commented Jun 15, 2017 •

edited

Loading

cyli Jun 16, 2017 •

edited

Loading