Ensuring root rotation works #1768

diogomonica · 2016-11-19T00:58:38Z

This PR fixes the client-side certificate rotation to also update the current Root CA being used by the server.

1.14 will bring CA Root Rotation, so it would be ideal if the break in compatibility was 1.12, and not 1.13.

aaronlehmann · 2016-11-19T01:04:15Z

ca/certificates.go

 	Cert []byte
 	Pool *x509.CertPool
 	// Digest of the serialized bytes of the certificate
 	Digest digest.Digest
 	// This signer will be nil if the node doesn't have the appropriate key material
 	Signer cfsigner.Signer
+	// Stores the location on disk where the RootCA lives


Path stores the location...

codecov-io · 2016-11-19T01:07:51Z

Current coverage is 55.06% (diff: 69.38%)

Merging #1768 into master will increase coverage by 0.21%

@@             master      #1768   diff @@
==========================================
  Files           102        102          
  Lines         16854      16873    +19   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           9245       9291    +46   
+ Misses         6459       6420    -39   
- Partials       1150       1162    +12

Powered by Codecov. Last update 53fcdeb...4bcc648

aaronlehmann · 2016-11-19T01:08:52Z

ca/certificates.go

@@ -545,9 +574,9 @@ func CreateRootCA(rootCN string, paths CertPaths) (RootCA, error) {

 // GetRemoteSignedCertificate submits a CSR to a remote CA server address,
 // and that is part of a CA identified by a specific certificate pool.
-func GetRemoteSignedCertificate(ctx context.Context, csr []byte, rootCAPool *x509.CertPool, config CertificateRequestConfig) ([]byte, error) {
+func GetRemoteSignedCertificate(ctx context.Context, csr []byte, rootCAPool *x509.CertPool, config CertificateRequestConfig) ([]byte, []byte, error) {


Maybe this should just return the NodeCertificateStatusResponse.

aaronlehmann · 2016-11-19T01:16:23Z

ca/certificates.go

+	// However, there might have been a server-side root rotation, so we verify this cert with a new pool.
+	// If we got a valid rootCABundle, turn it into a Pool, and verify the newly minted certificate using it.
+	rootCAPool := rca.Pool
+	newRootCA, newRootErr := NewRootCA(rootCABundle, nil, rca.Path, time.Minute)


Shouldn't all this be conditional on rootCABundle not being empty?

Also, is it possible to skip all this, particularly saving the root CA cert to disk, if the received bundle is identical to what we already have?

aaronlehmann · 2016-11-19T01:24:58Z

ca/server.go

-			Certificate: &node.Certificate,
+			Status:       &node.Certificate.Status,
+			Certificate:  &node.Certificate,
+			RootCABundle: s.securityConfig.RootCA().Cert,


Not sure there's any need to fill this field in right now while there's no CA-side support for root rotation - unless it's useful for testing or something.

aaronlehmann · 2016-11-19T01:34:32Z

I think this looks reasonable for 1.13. You'll need to get sign-off from @vieux to put it in after the feature freeze. Since this is more of a forward compatibility change than an actual feature, it might be okay.

I'd suggest setting this up this in such a way that this new code is never triggered in a pure 1.13 environment. For example, if the client only rotates root certs when it receives a RootCABundle, and 1.13's CA never sends a RootCABundle, then this would be a low risk change. During the 1.14 cycle we would have time to fully test root rotation end-to-end and across versions, and only enable it on the CA side once we're confident in it.

diogomonica · 2016-11-19T18:41:20Z

/cc @aluzzardi would be great if this got in for 1.13, ensuring backwards compatibility break on 1.12 for root-rotation.

aluzzardi · 2016-11-19T20:27:44Z

What would we be breaking in 1.12?

/cc @vieux

diogomonica · 2016-11-19T22:49:16Z

@aluzzardi the goal is for 1.14 to support adding a new root CA, reissue all the certificates for all the nodes, and removing the old CA. 1.12 nodes will not be able to add a new root CA, and therefore will not be able to continue participating in the cluster without manual intervention. If we don't get this in, the same will happen for 1.13 nodes.

cyli · 2016-11-21T08:23:46Z

ca/config.go

 	if err == nil {
+		// No errors, swam the current rootCA


what does "swam the current rootCA" mean?

cyli · 2016-11-21T08:26:46Z

ca/certificates_test.go

@@ -339,10 +295,6 @@ func TestRequestAndSaveNewCertificates(t *testing.T) {
 	_, _, err = unencryptedKeyReader.Read()
 	require.Error(t, err)

-	_, _, err = ca.NewKeyReadWriter(tc.Paths.Node, []byte("kek!"), nil).Read()


Any reason to delete this assertion?

cyli · 2016-11-21T08:45:10Z

LGTM aside from minor nitpicks :)

Signed-off-by: Diogo Monica <diogo.monica@gmail.com>

…on NodeStatus Signed-off-by: Diogo Monica <diogo.monica@gmail.com>

aluzzardi · 2016-11-22T01:39:35Z

@vieux @icecrime @thaJeztah:

In 1.14+, there's going to be a way to rotate the root CA. Older agents of course won't be able to perform the rotation, therefore either they'll fail (meh) or we'll just block rotation if any older engine is connected (better).

Pseudo random example:

$ docker swarm update --rotate-ca
Error from daemon: Oh noz! node-1234 is running an older version of engine (1.12.2). Please upgrade before rotating the CA or use --force to rotate anyway

This PR adds agent-side rotation support (e.g. making sure an agent can fetch a new CA bundle from the manager) which means 1.13 agents would be able to perform rotation (so the --rotate-ca command would work with 1.13 and 1.14, only 1.12 would fail).

Question is: Are we comfortable with this kind of change during the freeze?

@diogomonica Did I summarize the problem correctly?

diogomonica · 2016-11-22T19:04:53Z

@aluzzardi perfectly so. <3

Signed-off-by: Diogo Monica <diogo.monica@gmail.com>

aluzzardi · 2016-11-23T01:21:40Z

LGTM - let's merge in master.

Need sign-off from @vieux in order to cherry pick to 1.13.x (rc3 at this point)

…re test Signed-off-by: Diogo Monica <diogo.monica@gmail.com>

diogomonica · 2016-11-24T02:22:31Z

Added one more test, for the case where the server returns a nil CABundle

vieux · 2016-11-24T06:50:12Z

LGTM

cyli · 2016-11-24T07:31:22Z

ca/certificates.go

@@ -594,7 +629,7 @@ func GetRemoteSignedCertificate(ctx context.Context, csr []byte, rootCAPool *x50
 		}

 		// If the certificate was issued, return
-		if statusResponse.Status.State == api.IssuanceStateIssued {
+		if statusResponse.Status != nil && statusResponse.Status.State == api.IssuanceStateIssued {


cyli

LGTM!

thaJeztah · 2016-11-25T16:18:49Z

Question is: Are we comfortable with this kind of change during the freeze?

@diogomonica @aluzzardi is this going to cause issues for 1.13-rc1/rc2? (no top priority, but good to know if there are possible issues)

diogomonica · 2016-11-25T17:56:18Z

@thaJeztah shouldn't have any impact. But we should merge this ASAP.

thaJeztah · 2016-11-25T22:45:55Z

@diogomonica Looks like I don't have permissions in this repo to merge, but I see enough LGTM's, so should be ready to go 😄

thaJeztah · 2016-11-25T22:47:16Z

does this need squashing?

aaronlehmann · 2016-12-07T21:50:53Z

Removing cherry-pick label, since this was backported to 1.13 in a separate PR.

aaronlehmann reviewed Nov 19, 2016

View reviewed changes

cyli reviewed Nov 21, 2016

View reviewed changes

ca/config.go

if err == nil {

// No errors, swam the current rootCA

Copy link

Contributor

cyli Nov 21, 2016

Choose a reason for hiding this comment

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

what does "swam the current rootCA" mean?

cyli reviewed Nov 21, 2016

View reviewed changes

diogomonica force-pushed the ensuring-root-rotation-works branch from 5f76379 to 4349296 Compare November 22, 2016 00:30

diogomonica added 2 commits November 21, 2016 16:31

Added rootCABundle on NodeStatusResponse

f279785

Signed-off-by: Diogo Monica <diogo.monica@gmail.com>

Added path to RootCA, changed creation params, return current bundle …

5867064

…on NodeStatus Signed-off-by: Diogo Monica <diogo.monica@gmail.com>

diogomonica force-pushed the ensuring-root-rotation-works branch 2 times, most recently from faa81a1 to 5d2d0c0 Compare November 22, 2016 00:38

diogomonica force-pushed the ensuring-root-rotation-works branch from 5d2d0c0 to e0807b2 Compare November 23, 2016 01:17

Fixed tests

e2aed4d

Signed-off-by: Diogo Monica <diogo.monica@gmail.com>

diogomonica force-pushed the ensuring-root-rotation-works branch from e0807b2 to e2aed4d Compare November 23, 2016 01:18

Fixed potential crash when server returned nil state and added one mo…

4bcc648

…re test Signed-off-by: Diogo Monica <diogo.monica@gmail.com>

cyli reviewed Nov 24, 2016

View reviewed changes

cyli approved these changes Nov 24, 2016

View reviewed changes

thaJeztah added this to the 1.13.0 milestone Nov 25, 2016

thaJeztah added status/4-merge process/cherry-pick labels Nov 25, 2016

diogomonica merged commit b035a18 into moby:master Nov 26, 2016

diogomonica deleted the ensuring-root-rotation-works branch November 29, 2016 22:12

diogomonica mentioned this pull request Dec 2, 2016

Root rotation backwards compat #1787

Merged

AkihiroSuda mentioned this pull request Dec 5, 2016

[master] Vendor swarmkit moby/moby#29117

Merged

aaronlehmann removed the process/cherry-pick label Dec 7, 2016

This was referenced Dec 9, 2016

Update SecurityConfig RootCA when a new rootCA is downloaded on TLS cert renewal #1801

Closed

Revert "Ensuring root rotation works" #1807

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensuring root rotation works #1768

Ensuring root rotation works #1768

diogomonica commented Nov 19, 2016

aaronlehmann Nov 19, 2016

codecov-io commented Nov 19, 2016 •

edited

Loading

aaronlehmann Nov 19, 2016

diogomonica Nov 22, 2016

aaronlehmann Nov 19, 2016

aaronlehmann Nov 19, 2016

aaronlehmann Nov 19, 2016

aaronlehmann commented Nov 19, 2016

diogomonica commented Nov 19, 2016

aluzzardi commented Nov 19, 2016

diogomonica commented Nov 19, 2016

cyli Nov 21, 2016

cyli Nov 21, 2016

cyli commented Nov 21, 2016

aluzzardi commented Nov 22, 2016

diogomonica commented Nov 22, 2016

aluzzardi commented Nov 23, 2016

diogomonica commented Nov 24, 2016

vieux commented Nov 24, 2016

cyli Nov 24, 2016

cyli left a comment

thaJeztah commented Nov 25, 2016

diogomonica commented Nov 25, 2016

thaJeztah commented Nov 25, 2016

thaJeztah commented Nov 25, 2016

aaronlehmann commented Dec 7, 2016

Ensuring root rotation works #1768

Ensuring root rotation works #1768

Conversation

diogomonica commented Nov 19, 2016

Choose a reason for hiding this comment

codecov-io commented Nov 19, 2016 • edited Loading

Current coverage is 55.06% (diff: 69.38%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann commented Nov 19, 2016

diogomonica commented Nov 19, 2016

aluzzardi commented Nov 19, 2016

diogomonica commented Nov 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyli commented Nov 21, 2016

aluzzardi commented Nov 22, 2016

diogomonica commented Nov 22, 2016

aluzzardi commented Nov 23, 2016

diogomonica commented Nov 24, 2016

vieux commented Nov 24, 2016

Choose a reason for hiding this comment

cyli left a comment

Choose a reason for hiding this comment

thaJeztah commented Nov 25, 2016

diogomonica commented Nov 25, 2016

thaJeztah commented Nov 25, 2016

thaJeztah commented Nov 25, 2016

aaronlehmann commented Dec 7, 2016

codecov-io commented Nov 19, 2016 •

edited

Loading