[ca][api]: Root rotation object in cluster by cyli · Pull Request #2037 · moby/swarmkit

cyli · 2017-03-14T23:38:10Z

Add a field in the API types for the new root certificate, root key, and cross-signed root certificate for use during root rotation. Also add a field to ExternalCA types to specify the root certificate that should be expected from the external CA URL.

Also, when updating the security config on cluster change, take into account whether or not this root rotation object exists.

~~This is stacked on top of #2038.~~ This was merged.

aaronlehmann · 2017-03-15T17:48:14Z

CI is failing:

/tmp/go-build507923640/github.com/docker/swarmkit/integration/_test/_obj_test/node.go:65: rootCA.Cert undefined (type *ca.RootCA has no field or method Cert)
integration/integration_test.go:520: rootCA.Cert undefined (type ca.RootCA has no field or method Cert)

Not sure if this is expected, as the PR is marked WIP.

cyli · 2017-03-15T18:07:58Z

@aaronlehmann Ah sorry, I had failed to rebase this one while I was trying to track down the possible test failure in #2038. Fixing, but this one is still lacking tests for changes.

codecov · 2017-03-15T18:09:43Z

Codecov Report

Merging #2037 into master will decrease coverage by 0.06%.
The diff coverage is 87.8%.

@@            Coverage Diff             @@
##           master    #2037      +/-   ##
==========================================
- Coverage   54.22%   54.15%   -0.07%     
==========================================
  Files         111      111              
  Lines       19289    19332      +43     
==========================================
+ Hits        10460    10470      +10     
- Misses       7571     7612      +41     
+ Partials     1258     1250       -8

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 5ae0a39...563cb1d. Read the comment docs.

diogomonica · 2017-03-20T23:55:47Z

+
+		externalCARootPool := updatedRootCA.Pool
+		if rCA.RootRotation != nil {
+			// the extxernal CA has to trust the new CA cert


typo on external

diogomonica · 2017-03-20T23:58:09Z

@@ -548,17 +551,41 @@ func (s *Server) UpdateRootCA(ctx context.Context, cluster *api.Cluster) {



extra new line?

diogomonica · 2017-03-20T23:58:41Z

 	// If the cluster has a RootCA, let's try to update our SecurityConfig to reflect the latest values
 	rCA := cluster.RootCA
-	if len(rCA.CACert) != 0 && len(rCA.CAKey) != 0 {
+	externalCACert := rCA.CACert


why do we call the current CACert externalCACert?

I added a field to api.ExternalCA specifying which root certificate should be associated with the external URL so we can make sure that only the right external URLs are being added. So during root rotation, only the external URLs with the right root certificate get added to the external URL.

This is the URL to filter for. Although I guess if we are explicitly not supporting external->external rotation, this change is probably not necessary.

I've renamed this variable and added a comment about why it's here

diogomonica · 2017-03-21T00:03:40Z

+	for _, extCA := range cluster.Spec.CAConfig.ExternalCAs {
+		urlsCACert := extCA.CACert
+		if extCA.CACert == nil {
+			urlsCACert = rCA.CACert // if there was no CA cert (it was nil), assume it's the original cert


what's the difference between rCA.CACert and externalCACert?

rCA.CACert is the root of trust we're generally using to validate TLS certificates. externalCACert specifies which cert this external CA will use to sign - we want to match the external CA cert to the signing cert, which may be the new root cert if a root rotation is in progress. I've renamed this variable and added a comment about why it's here.

diogomonica · 2017-03-21T00:04:54Z

+		if extCA.CACert == nil {
+			urlsCACert = rCA.CACert // if there was no CA cert (it was nil), assume it's the original cert
+		}
+		if extCA.Protocol == api.ExternalCA_CAProtocolCFSSL && bytes.Equal(urlsCACert, externalCACert) {


Same thing here rCA is only set one right? externalCACert always == rCA.CACert

It's either the new root rotation cert, which is not set to rCA.CACert yet, or it's the rCA.CACert if there is no outstanding root rotation.

I've renamed this variable and added a comment about why it's here

diogomonica · 2017-03-21T20:42:35Z

LGTM

cyli · 2017-03-21T23:18:11Z

This is ready for another look, whenever you have time @aaronlehmann

aaronlehmann · 2017-03-21T23:50:39Z

+		// We want to support old external CA specifications which did not have a CA cert.  If there is no cert specified,
+		// we assume it's the old cert
+		certForExtCA := extCA.CACert
+		if certForExtCA == nil {


if len(certForExtCA) == 0

aaronlehmann · 2017-03-21T23:53:14Z

-				"cluster.id": cluster.ID,
-				"method":     "(*Server).updateCluster",
-			}).Debugf("Root CA updated successfully")
+			logger.Debugf("Root CA updated successfully")


logger.Debug

aaronlehmann · 2017-03-21T23:54:49Z

+			certForExtCA = rCA.CACert
+		}
+		if extCA.Protocol == api.ExternalCA_CAProtocolCFSSL && bytes.Equal(certForExtCA, wantedExternalCACert) {
+			cfsslURLs = append(cfsslURLs, extCA.URL)


Should we log when we skip external CAs that don't match the current cert? I think it might be difficult to understand why an external CA isn't being used. Or would that be too noisy?

It may be noisy, just because we update every time the cluster object is changed, as opposed to specifically when the RootCA object or external CAs are changed. I wonder if we should copy the RootCA object and externalCA objects to the server so that we can skip doing any changes if the objects themselves haven't changed...

That's a good idea.

Have added this functionality in e870413.

cyli · 2017-03-22T00:21:55Z

After a discussion with @aaronlehmann, going to explore putting the desired signing cert and key in the cluster spec instead of this root rotation object.

diogomonica · 2017-03-22T17:32:42Z

@cyli do we also keep the old cert/keypair for rollbacks?

cyli · 2017-03-22T17:41:22Z

@diogomonica I think I still need to keep the current root CA object in the api.Cluster.RootCA, and the current being rotated to object in api.Cluster.RootCA.RootRotation, but there will just be extra stuff in the spec, so rolling back could use the data in that. I may need to copy external CAs over, because currently there isn't a good way to roll those back if they're changed.

…s track of the root cert + key we want to rotate to, as well as a cross-signed intermediate. Signed-off-by: cyli <ying.li@docker.com>

… in the raft store, take into account whether there is a root rotation object in the RootCA object. Signed-off-by: cyli <ying.li@docker.com>

aaronlehmann · 2017-03-23T00:12:10Z

+	case a != nil && b != nil:
+	default:
+		return false
+	}


This is super nitpicky, but as a former C programmer I find this confusing. How about:

if a == nil && b == nil { return true } if a == nil || b == nil { return false }

aaronlehmann · 2017-03-23T00:19:26Z

 // anything to update the root CA material
 func (s *Server) UpdateRootCA(ctx context.Context, cluster *api.Cluster) {
 	s.mu.Lock()
+	defer s.mu.Unlock()


Wondering if any harm can come from holding the lock for the whole function. I'm not sure whether we weren't before as an optimization, or because there's a long-lived operation here, or one that might deadlock.

It doesn't seem super long-lived, as we're not generating any new creds, so this seems relatively short. I can create a separate lock, though if we like, just for updating the security config maybe.

aaronlehmann · 2017-03-23T00:23:32Z

+}
+
+// ExternalCAsEqualStable compares lists of external CAs and determines whether they are equal
+func ExternalCAsEqualStable(a, b []*api.ExternalCA) bool {


Curious why straight up reflect.DeepEqual doesn't work for this.

We don't actually necessarily want a reflect.DeepEqual of the whole struct - reflect.DeepEqual doesn't treat nil and empty values the same: https://play.golang.org/p/VGgZq6MVik

I figure an empty list of external CA should be the same, for our purposes, as a nil list. It also won't treat an empty map the same as a nil map (and the api.ExternalCA object has a map of options).

Yeah, but can this situation ever come up? I believe we are always assigning s.externalCA from the protobuf object, so I don't see how an empty map could change to a nil or vice versa.

Is the concern about the first call, before we've initialized anything in the Server struct, where nil may actually be equivalent to what we would update it to (if the protobuf object contains an empty list)?

In this case, could the function be shortened to:

if len(a) == 0 && len(b) == 0 { return true } return reflect.DeepEqual(a, b)

That's true - the map is created from the same object. I was writing this from the perspective of this function can be used no matter who creates the external CAs, but happy to make a change and a comment stating the assumption.

I think it would be easier to maintain that way. I can't see any scenario where we care about nil maps vs empty maps.

The same goes for RootCAEqualStable, except in that case you're using subtle.ConstantTimeCompare, so arguably there's a security reason to avoid using reflect.DeepEqual directly (though it's a weak one, and I'm not really sure it's worth the maintenance burden).

I could specifically pull those out to do subtle.ConstantTimeCompare, and reflect.DeepEqual the rest.

SGTM. I guess once the keys are confirmed to be equal, comparing them with reflect.DeepEqual would not yield any side channel information.

aaronlehmann · 2017-03-23T00:24:52Z

-			logger.Debugf("Root CA updated successfully")
+			// only update the server cache if we've successfully updated the root CA
+			logger.Debug("Root CA updated successfully")
+			s.rootCA = &cluster.RootCA


Let's make this cluster.RootCA.Copy() to avoid storing a pointer into the object living in the store. Just in case we were to write into it for some reason...

aaronlehmann · 2017-03-23T00:25:11Z


-	s.securityConfig.externalCA.UpdateURLs(cfsslURLs...)
+		s.securityConfig.externalCA.UpdateURLs(cfsslURLs...)
+		s.externalCAs = cluster.Spec.CAConfig.ExternalCAs


cluster.Spec.CAConfig.ExternalCAs.Copy()

aaronlehmann · 2017-03-23T00:28:06Z

 	securityConfig              *SecurityConfig
 	joinTokens                  *api.JoinTokens
+	rootCA                      *api.RootCA
+	externalCAs                 []*api.ExternalCA


Wondering if we should name these something different to avoid confusing them with actual RootCA and ExternalCA objects. Not sure what to suggest, though.

lastSeenClusterRoot and lastSeenExternalCAs maybe?

aaronlehmann · 2017-03-23T01:23:46Z

@@ -3,6 +3,8 @@ package equality
 import (
 	"reflect"



extra blank line

aaronlehmann · 2017-03-23T01:24:04Z

LGTM

…rnal CA config has changed Signed-off-by: cyli <ying.li@docker.com>

diogomonica · 2017-03-23T02:12:52Z

Are we ready to merge this?

cyli · 2017-03-23T02:45:11Z

I think so, I was just waiting for ci after removing the extra newline.

cyli mentioned this pull request Mar 14, 2017

[ca] Separate RootCA signing cert from root certs #2038

Merged

cyli force-pushed the root-rotation-object-in-cluster branch from c55742d to 4d7bb0d Compare March 14, 2017 23:43

cyli force-pushed the root-rotation-object-in-cluster branch from 4d7bb0d to e79d046 Compare March 15, 2017 18:09

cyli force-pushed the root-rotation-object-in-cluster branch 3 times, most recently from 4a7572d to ffd74b1 Compare March 20, 2017 22:48

cyli changed the title ~~WIP: [ca][api]: Root rotation object in cluster~~ [ca][api]: Root rotation object in cluster Mar 20, 2017

diogomonica reviewed Mar 20, 2017

View reviewed changes

diogomonica reviewed Mar 21, 2017

View reviewed changes

cyli force-pushed the root-rotation-object-in-cluster branch from ffd74b1 to 20a4c58 Compare March 21, 2017 01:00

cyli force-pushed the root-rotation-object-in-cluster branch 2 times, most recently from ba14e1d to fa7b565 Compare March 21, 2017 22:43

aaronlehmann reviewed Mar 21, 2017

View reviewed changes

cyli changed the title ~~[ca][api]: Root rotation object in cluster~~ WIP: [ca][api]: Root rotation object in cluster Mar 22, 2017

cyli added 2 commits March 22, 2017 14:46

Update the RootCA object to contain a RootRotation object, which keep…

cd68ffa

…s track of the root cert + key we want to rotate to, as well as a cross-signed intermediate. Signed-off-by: cyli <ying.li@docker.com>

When updating security config information based on RootCA information…

8555c76

… in the raft store, take into account whether there is a root rotation object in the RootCA object. Signed-off-by: cyli <ying.li@docker.com>

cyli force-pushed the root-rotation-object-in-cluster branch from fa7b565 to e870413 Compare March 22, 2017 23:50

aaronlehmann reviewed Mar 23, 2017

View reviewed changes

cyli force-pushed the root-rotation-object-in-cluster branch from e870413 to 39bd2fc Compare March 23, 2017 00:18

aaronlehmann reviewed Mar 23, 2017

View reviewed changes

cyli force-pushed the root-rotation-object-in-cluster branch from 39bd2fc to 2ff526b Compare March 23, 2017 00:54

cyli changed the title ~~WIP: [ca][api]: Root rotation object in cluster~~ [ca][api]: Root rotation object in cluster Mar 23, 2017

aaronlehmann reviewed Mar 23, 2017

View reviewed changes

Do not update security config unless the actual RootCA object or exte…

563cb1d

…rnal CA config has changed Signed-off-by: cyli <ying.li@docker.com>

cyli force-pushed the root-rotation-object-in-cluster branch from 2ff526b to 563cb1d Compare March 23, 2017 01:41

cyli merged commit bdeeb89 into moby:master Mar 23, 2017

cyli deleted the root-rotation-object-in-cluster branch March 23, 2017 02:45

cyli mentioned this pull request Mar 23, 2017

[feature] CA Root rotation #1990

Closed

10 tasks

		@@ -548,17 +551,41 @@ func (s Server) UpdateRootCA(ctx context.Context, cluster api.Cluster) {

Conversation

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

Uh oh!

Uh oh!

aaronlehmann commented Mar 15, 2017

Uh oh!

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

Uh oh!

Uh oh!

codecov Bot commented Mar 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!

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!

diogomonica commented Mar 21, 2017

Uh oh!

cyli commented Mar 21, 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!

cyli commented Mar 22, 2017

Uh oh!

diogomonica commented Mar 22, 2017

Uh oh!

cyli commented Mar 22, 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!

aaronlehmann Mar 23, 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

cyli commented Mar 14, 2017 •

edited

Loading

cyli commented Mar 15, 2017 •

edited

Loading

codecov Bot commented Mar 15, 2017 •

edited

Loading

aaronlehmann Mar 23, 2017 •

edited

Loading