[controlapi] Allow starting and aborting root CA rotations during swarm update API by cyli · Pull Request #2052 · moby/swarmkit

cyli · 2017-03-23T20:41:33Z

This adds the ability to specify, using the ClusterSpec, whether the swarm root CA should be rotated, and if so, what the new cert and key should be.

If no desired cert or key are provided, then a pair will be generated so long as the ForceRotate counter is bumped up.

If a desired cert is provided, the ForceRotate counter needs not be bumped, but it would not break anything if it were.

~~This is stacked on top of #2050 and #2051.~~ Update: These have been merged.

Remaining TODOs:

I think it may be possible for the current security config, if a root rotation is in progress, to sign using a different cert and key than the current root. Need to confirm. Update: Yes, my test was not actually updating the test cases for root rotation - this has been fixed.
There might also be a chance for a race condition when updating the cluster - the cluster may have been updated but the security config containing the RootCA may not have been updated yet due to a delay in notifications. So the new join tokens or cross-signed certs may be generating using an outdated RootCA. Update: Fixed in [ca] Make the control API server take a security config instead of RootCA #2051
Validate on ClusterUpdate that at least one of the remote CAs on the list is up.

codecov · 2017-03-23T20:51:43Z

Codecov Report

Merging #2052 into master will increase coverage by 0.3%.
The diff coverage is 89.4%.

@@            Coverage Diff            @@
##           master    #2052     +/-   ##
=========================================
+ Coverage   54.21%   54.51%   +0.3%     
=========================================
  Files         111      112      +1     
  Lines       19354    19497    +143     
=========================================
+ Hits        10492    10629    +137     
+ Misses       7597     7590      -7     
- Partials     1265     1278     +13

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 74a3d19...1b1c030. Read the comment docs.

aaronlehmann · 2017-03-23T21:48:13Z

+	// ForceRotate is a counter that triggers an root CA rotation even if no relevant
+	// parameters have been in the spec. This will force the manager to generate a new
+	// certificate and key, if none have been provided.
+	uint64 force_rotate = 5;}


Missing newline here

aaronlehmann · 2017-03-23T21:50:07Z

+
+	// ForceRotationVersion matches the Cluster Spec's CAConfig's ForceRotation counter.
+	// It indicates when the current CA cert and key were generated (or updated).
+	uint64 force_rotation_version = 6;


Maybe just RotationVersion?

Maybe LastForcedRotation is better, actually, because that is less likely to be confused with the Version field in Meta.

You're right, we probably only need the top level one.

aaronlehmann · 2017-03-23T21:51:14Z

+	bytes cross_signed_ca_cert = 3 [(gogoproto.customname) = "CrossSignedCACert"];
+	// ForceRotationVersion matches the Cluster Spec's CAConfig's ForceRotation counter.
+	// It indicates when the current to-be-rotated CA cert and key were generated (or updated).
+	uint64 force_rotation_version = 6;


Why does this appear in both RootRotation and one level above it? Is that intentional?

Never mind, read the comments... But I'm still surprised we would need to keep track of both. Will read the code next.

aaronlehmann · 2017-03-23T21:55:35Z

ping @aluzzardi for API review

I think this approach makes sense. It lets a client request a rotation in a declarative way, either by providing a new key/cert or asking the manager to generate one. We also discussed passing flags to controlapi's UpdateCluster, but I think doing it in a declarative way is preferable.

diogomonica

In general LGTM, but I would love to have a deeper look at validateAndUpdateCA with a high-level diagram that describes the four big blocks of that function.

diogomonica · 2017-03-24T22:00:16Z

 	repeated ExternalCA external_cas = 2 [(gogoproto.customname) = "ExternalCAs"];
+
+	// SigningCACert is the desired CA certificate to be used as the root and
+	// signing CA for the swarm.  If not provided,


"If not provided,"

diogomonica · 2017-03-24T22:01:20Z

+	// to sign certificates for the swarm
+	bytes signing_ca_key = 4 [(gogoproto.customname) = "SigningCAKey"];
+
+	// ForceRotate is a counter that triggers an root CA rotation even if no relevant


*a root CA rotation

diogomonica · 2017-03-24T22:03:47Z

+
+	// LastForcedRotation matches the Cluster Spec's CAConfig's ForceRotation counter.
+	// It indicates when the current CA cert and key were generated (or updated).
+	uint64 last_forced_rotation = 6;


aren't all rotations forced? why not just last_rotation?

I don't particularly feel strongly - the name is to make it clearer that it corresponds with the ForceRotation value in the config.

diogomonica · 2017-03-24T22:08:50Z

+	"github.com/docker/swarmkit/log"
+)
+
+var minRootExpiration = helpers.OneYear


My first instinct was: this is reasonable. But on further consideration, and given that fact that:

This is the root CA

We have the ability of rotating at any time if compromised

This makes me think that maybe we should help our users not shoot themselves in the foot and increase the minimum a bit more (3 years)? Just a thought though, I'm 100% ok with 1 year.

/cc @sul3n3t

I also don't feel strongly - happy to change it to 3 years too. I just picked some value. :)

diogomonica · 2017-03-24T22:12:13Z

+// Creates and updates the api.RootRotation object and the cross-signed intermediate.
+// This function assumes that a root rotation is not already in progress, and that the root
+// cert and key to be rotated in or the external CAs in the cluster have already been validated
+func updateRootRotationObject(ctx context.Context, securityConfig *ca.SecurityConfig, cluster *api.Cluster, newRootCA ca.RootCA, version uint64) error {


Should this be a method on cluster?

I dislike that this is not a method on cluster but has side-effects on cluster.

The api module is just data, and I don't know if we should be adding this to that module, particular could create import issues since then nothing from ca would be able to import anything from api, since api would then depend in ca.

diogomonica · 2017-03-24T22:45:11Z

+	// whether it's a locally signing CA or an external CA.  The key can be replaced with the desired key (which will
+	// either be the correct key or nil) without having to do any kind of root rotation, so we can abort any outstanding
+	// root rotations and just finish the validation.
+	if bytes.Equal(cluster.RootCA.CACert, newConfig.SigningCACert) {


can we just change cluster at will?

diogomonica · 2017-03-24T22:48:49Z

+		return grpc.Errorf(codes.InvalidArgument, "CA certificate expires too soon")
+	}
+
+	// If the desired cert has the same public key and subject as current root, then the private signer is the same


Isn't this the exact same conditional as the one on line 147?

In this case, the cert has changed, it just has the same subject and public key. One such example is if the cert is renewed. In this case, leaf certs issued by the previous CA cert will still correctly validate, but the cert digest has changed, and hence join tokens need to be re-generated.

So: Root CA Cert is expiring, and we self-sign a longer lived one?

diogomonica · 2017-03-24T22:49:58Z

+		return grpc.Errorf(codes.InvalidArgument, "the desired CA certificate cannot contain multiple certificates")
+	}
+
+	// If the desired cert is the same as the current root CA cert then the private signer is the same


What are the situations for each of these? Why doesn't this one update the tokens?

It's byte-for-byte the exact same cert, so the tokens do not need to be re-generated, since the hash is the same.

diogomonica · 2017-03-24T22:51:31Z

@@ -0,0 +1,623 @@
+package controlapi


A lot of tests 👍

Thanks for making this table-driven.

diogomonica · 2017-03-24T22:52:05Z

+		{
+			rootCA:            initialLocalRootCA,
+			oldCAConfig:       api.CAConfig{ForceRotate: 1},
+			expectErrorString: "ForceRotate value must be nondecreasing",


what about testing the same?

Ignore, the same is a valid case.

Added the same value test case to the valid cases.

diogomonica · 2017-03-24T22:58:00Z

@aaronlehmann @cyli I commented it inline, but why not have a simpler logic of bumping the rotation version to kick a rotation, and if valid Cert/Key are provided we use them, if not, we generate new ones. Not sure I see the advantage of having two mechanisms.

aaronlehmann · 2017-03-27T21:07:05Z

@cyli: Is this still WIP?

cyli · 2017-03-27T21:59:02Z

@aaronlehmann It wasn't before - just pushed the latest :) So it is now, thanks for checking!

aaronlehmann · 2017-03-27T22:08:15Z

+
+// TODO(cyli): in go 1.8 this will be easier as we can use *url.URL.Port() and *url.URL.Hostname()
+func getConnectionStringFromHost(hostport string) string {
+	colon := strings.IndexByte(hostport, ':')


Use net.SplitHostPort

Oh nice! Thanks! I didn't know about that one - go 1.8's code doesn't use it either. :D

aaronlehmann · 2017-03-27T22:08:25Z

+func getConnectionStringFromHost(hostport string) string {
+	colon := strings.IndexByte(hostport, ':')
+	if colon == -1 {
+		return fmt.Sprintf("%s:443", hostport) // no port, default to 443


Use net.JoinHostPort

aaronlehmann · 2017-03-27T22:09:55Z

+	}
+	conn, err := tls.DialWithDialer(dialer, "tcp", getConnectionStringFromHost(parsed.Host), tlsOpts)
+	if conn != nil {
+		defer conn.Close()


It shouldn't be necessary to defer this.

aaronlehmann · 2017-03-27T22:12:03Z

+	case api.CAConfig:
+		return len(b.SigningCACert) > 0 && len(b.SigningCAKey) == 0
+	default:
+		return false


panic here?

aaronlehmann · 2017-03-27T22:14:18Z

+			if err == nil {
+				return true
+			}
+			log.G(ctx).WithError(err).Debugf("external CA # %d is unreachable or invalid", i+1)


Seems like a potentially useful log message. Any downsides to making it a warning?

Not particularly - this may be noisy if we have to re-validate every time, but perhaps if I reflect.DeepEqual(<oldCAConfig spec>, <newCAConfig spec> in UpdateCluster it would be fine? That'd assume nothing else goes and changes api.RootCA in the store (which is probably a fair assumption).

This is in controlapi, so at most you would get a warning per unreachable CA every time the cluster object is updated through the API, right?

Yes. Do we want to warn every time a cluster update is attempted, or just when the the user attempts to change the CAConfig?

I don't see any harm in warning every time the cluster is updated, as long as the unreachable CAs continue to be referenced in the spec. We're checking the CAs anyway, so if we discover that one is unreachable we might as well expose that information.

Sounds good, although this is on attempted spec update as well, not just when the spec update succeeds. I'm ok with that though, just wanted to make sure it's clear.

diogomonica · 2017-03-27T22:14:48Z

-		// Do not copy secret key
+		// Do not copy secret keys
+		redactedSpec := cluster.Spec.Copy()
+		redactedSpec.CAConfig.SigningCAKey = nil


do we have a test for this being redacted?

yup, it's there, never mind!

diogomonica · 2017-03-27T22:15:41Z

-	grpclog.SetLogger(log.New(ioutil.Discard, "", log.LstdFlags))
-	logrus.SetOutput(ioutil.Discard)
-}
+// func init() {


why do we have commented out code?

Oops sorry, I needed this while debugging. Forgot to uncomment it, thanks!

aaronlehmann · 2017-03-27T22:16:19Z

+			JoinTokens: api.JoinTokens{
+				Worker:  ca.GenerateJoinToken(newCARootCA),
+				Manager: ca.GenerateJoinToken(newCARootCA),
+			},


Are the join tokens being regenerated because the digest of the certificate might be changing even if the subject and public key are the same?

I guess the digest is guaranteed to change in this case, because otherwise the bytes.Equal above would return true, right?

Yep, that's correct

After some discussion with @diogomonica, it may not be worth it to check this particular case. We're optimizing for not having to generate a new cross-signed certificate, but it might simplify the code if we don't try to do this optimization and just do a full root rotation object instead. How would you feel about removing this check?

Sounds good.

diogomonica · 2017-03-27T22:16:55Z

+	"github.com/docker/swarmkit/log"
+)
+
+var minRootExpiration = 3 * helpers.OneYear


I regret my comment. Maybe 1 year makes this more flexible?

@sul3n3t

aaronlehmann · 2017-03-27T22:28:47Z

+	"reflect"
+	"testing"
+	"time"
+


extra blank line

aaronlehmann · 2017-03-27T22:29:04Z

+	"github.com/docker/swarmkit/ca"
+	"github.com/docker/swarmkit/ca/testutils"
+	"github.com/docker/swarmkit/manager/state/store"
+	digest "github.com/opencontainers/go-digest"


digest shouldn't be necessary.

Good point, thanks! My auto-importer insists on adding it every time I save. :)

aaronlehmann · 2017-03-27T22:31:32Z

+		require.NoError(t, err, valid.description)
+
+		// ensure that the cluster was not mutated
+		require.True(t, reflect.DeepEqual(valid.rootCA, cluster.RootCA))


BTW require.Equal will do the same thing, and show a diff if there is an unexpected change.

aaronlehmann · 2017-03-27T22:38:55Z

+
+		// Because join tokens are random, we can't predict exactly what it is, so this needs to be manually checked
+		require.Equal(t, valid.expectJoinTokenChange, !reflect.DeepEqual(result.JoinTokens, valid.rootCA.JoinTokens),
+			fmtString, valid.description, result.JoinTokens, valid.rootCA.JoinTokens)


Slightly more verbose, but I think this would be easier to read and more useful as:

if valid.expectJoinTokenChange { require.NotEqual(t, result.JoinTokens, valid.rootCA.JoinTokens, fmtString, valid.description, result.JoinTokens, valid.rootCA.JoinTokens) } else { require.Equal(t, result.JoinTokens, valid.rootCA.JoinTokens, fmtString, valid.description, result.JoinTokens, valid.rootCA.JoinTokens) }

aaronlehmann · 2017-03-27T22:39:48Z

+		if valid.expectedGeneratedCross || valid.expectGeneratedRootRotation { // both generate cross signed certs
+			require.NotNil(t, result.RootRotation, valid.description)
+			require.True(t, len(result.RootRotation.CrossSignedCACert) > 0,
+				"%s: if there is a root rotation object there must be a cross-signed cert", valid.description)


require.Len :)

aaronlehmann · 2017-03-27T22:40:24Z

+		// just assert that the value has changed.
+		if valid.expectGeneratedRootRotation {
+			require.NotNil(t, result.RootRotation, valid.description)
+			require.False(t, reflect.DeepEqual(valid.rootCA.RootRotation, result.RootRotation), valid.description)


require.NotEqual

aaronlehmann · 2017-03-27T22:40:34Z

+
+		require.True(t, reflect.DeepEqual(result, &valid.expectRootCA),
+			"should be equal: "+fmtString, valid.description, result, &valid.expectRootCA)
+	}


require.Equal

aaronlehmann · 2017-03-27T22:42:31Z

+			testcase.rootCA = getRootCAWithRotation(testcase.rootCA, rotationCert, rotationKey, crossSigned)
+			testcases[2].description = "different cert than root rotation contents, even if internal->external, results in replacing root rotation"
+		}
+	}


I don't understand the purpose of this loop.

That's fair - I'm just testing that if the desired cert is different than the current root rotation cert, then the root rotation is replaced. That can just be 1 extra case, instead of 3 extra cases.

Oh sorry, unless you mean because I'm only checking the last one (testCases[2:])? That was something accidentally left over from when I was debugging. Regardless, I probably don't need the extra cases anyway.

Just confused about why you're looping and then doing something different for each value of the loop counter. I think I'm missing something.

I was trying to preserve the original testcases' configurations and expected root CA values (e.g. I was too lazy to type it again). It was to test that whatever the initial RootRotation value was, because the cert is different, it will just be replaced completely by a new root rotation with differentInitialCert, and that any external CAs required by the old root rotation will no longer be required.

I think this can probably be replaced by a single case.

aaronlehmann · 2017-03-27T22:51:08Z

+		return nil, grpc.Errorf(codes.InvalidArgument, "the desired CA certificate cannot contain multiple certificates")
+	}
+
+	// because newRootCA was parsed without error, this should not fail either


Is this saying that NewRootCA parses SigningCACert, so we expect parsing it again will succeed?

Yes - that's much clearer, thanks. I'll update the comment to say that.

My preference is to check the error anyway, as a future-proofing step.

Ok, can do that as well.

aaronlehmann · 2017-03-28T18:55:02Z

+	if err != nil {
+		return err
+	}
+	if port == "" {


I'm not sure this is reachable - it looks like SplitHostPort returns an error in this case. I expected that if SplitHostPort returns an error, we would assume the address does not have a port.

Oh hm... I didn't realize a port was required. It also looks like it returns an error if there are too many colons, though, and that does not seem to be something url.Parse will return an error on: https://play.golang.org/p/lAmzyII0-z.

Poking a bit more I guess go1.8's URL.Hostname() interprets "http://x:1:2" as a single host with no port, at least SplitHostPort will assume the same thing.

And I guess the dialer will fail if it's a weird hostname.

diogomonica · 2017-03-28T22:21:41Z

+		port = "443" //https
+	}
+
+	conn, err := tls.DialWithDialer(dialer, "tcp", net.JoinHostPort(host, port), tlsOpts)


Add comment as to why we use TCP vs HTTP?

Added it to the comment for the function (at the top of this function).

diogomonica · 2017-03-28T22:22:12Z

This LGTM

/cc @aluzzardi

aaronlehmann · 2017-03-28T22:58:44Z

+	if err != nil {
+		// It either has no port or is otherwise invalid (e.g. too many colons).  If it's otherwise invalid the dialer
+		// will error later, so just assume it's no port and set the port to the default HTTPS port.
+		port = "443"


Let's also have host = parsed.Host here. I don't know if SplitHostPort returns the argument as host if it also returns an error (and even if it does, it seems better not to rely on the other values when an error is returned).

aaronlehmann · 2017-03-28T23:38:07Z

LGTM

aaronlehmann · 2017-03-28T23:40:08Z

Feel free to reorganize the commits.

The cluster RootCA object will reflect the current status of the actual cluster RootCA, and the goal is to converge towards the desired CA cert and key. Signed-off-by: cyli <ying.li@docker.com>

…update, which ensures that 1. A minimum of required external CAs are provided and reachable and valid. 2. If a desired signing certificate and/or signing key are provided, that they are valid. If the CAConfig spec differs from the current root CA + current root rotation configuration, this function will also start, update, or abort a root rotation. It returns a new api.RootCA object (which may be completely unchanged from the current one) to replace the existing api.RootCA object in the cluster. Signed-off-by: cyli <ying.li@docker.com>

…Cluster function. Also make sure that when listing and getting clusters, the CAConfig's signing key and the root rotation object's signing key are redacted, if present. Signed-off-by: cyli <ying.li@docker.com>

aluzzardi · 2017-03-29T23:32:49Z

Just had a quick glance, overall LGTM

cyli mentioned this pull request Mar 23, 2017

[feature] CA Root rotation #1990

Closed

10 tasks

aaronlehmann reviewed Mar 23, 2017

View reviewed changes

cyli force-pushed the root-rotation-cluster-update-api branch 3 times, most recently from 1e44ff4 to 4d986d2 Compare March 24, 2017 20:07

diogomonica approved these changes Mar 24, 2017

View reviewed changes

cyli force-pushed the root-rotation-cluster-update-api branch 2 times, most recently from 6e8ae5e to 0bf8867 Compare March 27, 2017 18:42

cyli force-pushed the root-rotation-cluster-update-api branch from 0bf8867 to 60d5cdd Compare March 27, 2017 21:58

cyli changed the title ~~WIP: [controlapi] Allow starting and aborting root CA rotations during swarm update API~~ [controlapi] Allow starting and aborting root CA rotations during swarm update API Mar 27, 2017

cyli force-pushed the root-rotation-cluster-update-api branch from 60d5cdd to 01833ab Compare March 27, 2017 22:04

aaronlehmann reviewed Mar 27, 2017

View reviewed changes

diogomonica reviewed Mar 27, 2017

View reviewed changes

aaronlehmann reviewed Mar 27, 2017

View reviewed changes

diogomonica reviewed Mar 27, 2017

View reviewed changes

aaronlehmann reviewed Mar 27, 2017

View reviewed changes

Comment thread manager/controlapi/ca_rotation_test.go

"reflect"

"testing"

"time"

Copy link
Copy Markdown

Collaborator

aaronlehmann Mar 27, 2017

Choose a reason for hiding this comment

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

extra blank line

aaronlehmann reviewed Mar 27, 2017

View reviewed changes

cyli force-pushed the root-rotation-cluster-update-api branch 4 times, most recently from 8bd77a9 to acae0c6 Compare March 28, 2017 00:29

aaronlehmann reviewed Mar 28, 2017

View reviewed changes

diogomonica reviewed Mar 28, 2017

View reviewed changes

cyli force-pushed the root-rotation-cluster-update-api branch from acae0c6 to 03c8d56 Compare March 28, 2017 22:41

aaronlehmann reviewed Mar 28, 2017

View reviewed changes

cyli force-pushed the root-rotation-cluster-update-api branch from 03c8d56 to 621a36d Compare March 28, 2017 23:04

cyli mentioned this pull request Mar 29, 2017

[ca] Minor cleanups #2067

Merged

cyli force-pushed the root-rotation-cluster-update-api branch from 621a36d to 8f3f0a7 Compare March 29, 2017 00:47

cyli added 3 commits March 29, 2017 16:25

Add the desired certificate and signing key to the cluster spec.

5546c84

The cluster RootCA object will reflect the current status of the actual cluster RootCA, and the goal is to converge towards the desired CA cert and key. Signed-off-by: cyli <ying.li@docker.com>

Use the CAConfig spec validation function in the control api's Update…

1b1c030

…Cluster function. Also make sure that when listing and getting clusters, the CAConfig's signing key and the root rotation object's signing key are redacted, if present. Signed-off-by: cyli <ying.li@docker.com>

cyli force-pushed the root-rotation-cluster-update-api branch from 8f3f0a7 to 1b1c030 Compare March 29, 2017 23:28

diogomonica merged commit 63faf85 into moby:master Mar 30, 2017

cyli deleted the root-rotation-cluster-update-api branch March 30, 2017 00:31

Conversation

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

Uh oh!

Uh oh!

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

aaronlehmann commented Mar 23, 2017

Uh oh!

diogomonica left a comment

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!

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 24, 2017

Uh oh!

aaronlehmann commented Mar 27, 2017

Uh oh!

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

cyli commented Mar 23, 2017 •

edited

Loading

codecov Bot commented Mar 23, 2017 •

edited

Loading