ca: when the Root CA is updated in the security config, update all TLS creds too by cyli · Pull Request #1983 · moby/swarmkit

cyli · 2017-02-24T03:13:15Z

So that both can use the new root CA pool to validate peers.

aaronlehmann · 2017-02-24T03:15:39Z

+	if s.ClientTLSCreds != nil {
+		previousConfig := s.ClientTLSCreds.Config()
+		// can't error because we know for sure the root pool is valid
+		newConfig, _ := NewClientTLSConfig(&previousConfig.Certificates[0], rootCA.Pool, previousConfig.ServerName)


Do we know for sure that previousConfig.Certificates has at least one item?

Is there a possibility of a race here? Say the node's certificate gets renewed at the same moment, and there's a call to LoadNewTLSConfig in these calls to Config and LoadNewTLSConfig. Wouldn't we overwrite the new node certificate?

@aaronlehmann Good point - I guess the lock needs to be on the mutable TLS object.

codecov-io · 2017-02-24T03:22:26Z

Codecov Report

Merging #1983 into master will increase coverage by 0.17%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master    #1983      +/-   ##
==========================================
+ Coverage   53.69%   53.86%   +0.17%     
==========================================
  Files         109      109              
  Lines       18925    18943      +18     
==========================================
+ Hits        10161    10203      +42     
+ Misses       7538     7501      -37     
- Partials     1226     1239      +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 a52e9c8...598a7ed. Read the comment docs.

aaronlehmann · 2017-02-24T04:34:56Z

+func (c *MutableTLSCreds) UpdateCAs(rootCAs, clientCAs *x509.CertPool) {
+	c.Lock()
+	defer c.Unlock()
+	// TODO(cyli): on upgrade to go 1.8.x, use c.config.Clone() instead


Clone exists in 1.7.

Would you mind switching to it before merging?

AFAIK there's no reason we need to support Go 1.6. @vdemeester do you know if there is?

Hm... do I have something horribly misconfigured?

$ go test -race github.com/docker/swarmkit/ca ca/transport.go:142: c.config.Clone undefined (type *tls.Config has no field or method Clone, but does have tls.clone) $ go version go version go1.7.4 darwin/amd64

https://github.com/golang/go/blob/release-branch.go1.7/src/crypto/tls/common.go#L426 seems to only have Config.clone(), but https://github.com/golang/go/blob/release-branch.go1.8/src/crypto/tls/common.go#L550 has the exported function (I could just be missing an exported function in 1.7 in a different file, though)

I looked for Clone on both branches to verify, but did not notice it's unexported in 1.7.

I remember https://github.com/docker/docker/blob/master/pkg/tlsconfig dealing with this, and I see this was back in September. I thought it happened before Go 1.7 was released. Apparently it was right after. My apologies.

Anyway, would you mind vendoring/importing github.com/docker/docker/pkg/tlsconfig in this PR?

Not at all - as I've said before, my machine is haunted :) And go vet still produces errors on my machine that no one else seems to be able to replicate so I totally would believe something is wrong with my setup.

And sure, will do.

aaronlehmann · 2017-02-24T18:27:46Z

 	}

+	s.rootCA = &rootCA
 	return err


nit: return nil?

aaronlehmann · 2017-03-01T02:49:41Z

-		return
-	}
-
 	return


How about return cfcsr.ParseRequest(req) for clarity?

aaronlehmann · 2017-03-01T02:53:16Z

+// ExternalCA returns the external CA.
+func (s *SecurityConfig) ExternalCA() *ExternalCA {
+	s.mu.Lock()
+	defer s.mu.Unlock()


I don't believe it's necessary to hold this lock. s.externalCA seems to be set when the SecurityConfig is created, and never changed to point to anything else. updateCluster uses this field without holding the lock.

Good point, thanks!

Do we want to be conservative in both places?

aaronlehmann · 2017-03-01T02:54:23Z

Needs a rebase

…erver, client, and external CA TLS credentials all also use the new root pool. Signed-off-by: cyli <ying.li@docker.com>

diogomonica · 2017-03-02T00:12:52Z

 }

+// ExternalCA returns the external CA.
+func (s *SecurityConfig) ExternalCA() *ExternalCA {


Do we need locking?

Heh: #1983 (comment)

diogomonica · 2017-03-02T00:16:35Z

+func (c *MutableTLSCreds) UpdateCAs(rootCAs, clientCAs *x509.CertPool) {
+	c.Lock()
+	defer c.Unlock()
+	config := tlsconfig.Clone(c.config)


why do we need to clone instead of directly modifying it?

I tried that originally, but it's not really safe to modify a tls.Config while it's being used I think. I ended up with a data race in the tests, since a tls.Config stores some handshake/session state. The comment for tls.Config contains:

After one has been passed to a TLS function it must not be modified.

cyli changed the title ~~When the Root CA is updated in the security config, update client/server creds too~~ ca: when the Root CA is updated in the security config, update client/server creds too Feb 24, 2017

aaronlehmann reviewed Feb 24, 2017

View reviewed changes

cyli force-pushed the update-root-ca-updates-tls-creds branch 2 times, most recently from dd0743c to b043029 Compare February 24, 2017 03:45

aaronlehmann reviewed Feb 24, 2017

View reviewed changes

cyli mentioned this pull request Feb 24, 2017

ca: all managers will update their security configs when cluster root CA updated #1984

Merged

cyli force-pushed the update-root-ca-updates-tls-creds branch from b043029 to a4b6560 Compare February 24, 2017 07:41

aaronlehmann reviewed Feb 24, 2017

View reviewed changes

cyli force-pushed the update-root-ca-updates-tls-creds branch from a4b6560 to 21dbca4 Compare February 24, 2017 18:30

cyli mentioned this pull request Feb 28, 2017

[feature] CA Root rotation #1990

Closed

10 tasks

cyli force-pushed the update-root-ca-updates-tls-creds branch from 21dbca4 to 89f5ff4 Compare February 28, 2017 01:51

cyli changed the title ~~ca: when the Root CA is updated in the security config, update client/server creds too~~ ca: when the Root CA is updated in the security config, update all TLS creds too Feb 28, 2017

aaronlehmann reviewed Mar 1, 2017

View reviewed changes

When the root CA is updated in the security config, ensure that the s…

598a7ed

…erver, client, and external CA TLS credentials all also use the new root pool. Signed-off-by: cyli <ying.li@docker.com>

cyli force-pushed the update-root-ca-updates-tls-creds branch from 89f5ff4 to 598a7ed Compare March 1, 2017 02:59

diogomonica reviewed Mar 2, 2017

View reviewed changes

diogomonica merged commit 1dfa1a9 into moby:master Mar 2, 2017

cyli deleted the update-root-ca-updates-tls-creds branch March 2, 2017 00:30

aaronlehmann mentioned this pull request Mar 7, 2017

Do not use tlsconfig.Clone, which is unsafe in go 1.7 if the config is already being used #2013

Merged

-              		return
-              	}
               	return

Conversation

cyli commented Feb 24, 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!

codecov-io commented Feb 24, 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!

cyli Feb 24, 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!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Mar 1, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Feb 24, 2017 •

edited

Loading

cyli Feb 24, 2017 •

edited

Loading