[ca] Separate RootCA signing cert from root certs by cyli · Pull Request #2038 · moby/swarmkit

cyli · 2017-03-14T23:41:34Z

Since in theory, we could be signing with an intermediate certificate, which should not be used as a trust root.

This is useful both for supporting offline roots, and for supporting swarm root CA rotation (in which the root we trust is still the old root, but we will be signing with a cross-signed intermediate, which can easily be switched to the new root.

For more context on how this will be used in terms of root CA rotation, please see #2037. (or just the diff)

I made it a requirement that the signing cert has to chain up through the intermediates to the root pool, so that during root CA rotation the signing cert will actually be the cross-signed intermediate. This doesn't have to be the case (the signing cert could be a root instead) but I thought having direct relationship between the signing cert and the root certs would be easier to validate (can use Certificate.Verify.

Otherwise, the relationship to the root is indirect ; it has to have the same public key/subject as the first intermediate, and the first intermediate has to chain up to the root. But because it doesn't chain up to the root itself, we'd have to do certificate verification (expiry, maybe path len check) ourselves. Maybe this would be more flexible though, since we'd only be manually checking the signing cert and not the whole chain?

codecov · 2017-03-14T23:54:43Z

Codecov Report

Merging #2038 into master will increase coverage by 0.76%.
The diff coverage is 79.5%.

@@            Coverage Diff             @@
##           master    #2038      +/-   ##
==========================================
+ Coverage   53.17%   53.93%   +0.76%     
==========================================
  Files         111      109       -2     
  Lines       19656    19122     -534     
==========================================
- Hits        10452    10314     -138     
+ Misses       7928     7563     -365     
+ Partials     1276     1245      -31

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 fff0ebe...5454ec0. Read the comment docs.

cyli · 2017-03-15T00:57:16Z

(getting intermittent integration test failures - trying to track those down)

cyli · 2017-03-15T17:19:12Z

Update: The test failures do not seem related - https://circleci.com/gh/docker/swarmkit/6536. PTAL cc @diogomonica?

aaronlehmann · 2017-03-15T17:53:20Z

+// CanSign ensures that the signer has all the necessary elements needed to operate
 func (rca *RootCA) CanSign() bool {
-	if rca.Cert == nil || rca.Pool == nil || rca.Signer == nil {
+	if rca.Pool == nil || rca.Signer == nil || rca.Signer.Cert == nil || rca.Signer.Signer == nil {


Perhaps it's better to check len(rca.Signer.Cert) == 0

Good point, thanks!

diogomonica

Minor comments. LGTM.

diogomonica · 2017-03-16T10:56:45Z

 	defer s.mu.Unlock()

-	rootCA, err := NewRootCA(cert, key, certExpiry, nil)
+	signingCert := cert


Comment on what is happening here?

diogomonica · 2017-03-16T11:00:52Z

-		if err != nil {
-			return RootCA{}, err
+	if localSigner != nil && len(parsedIntermediates) > 0 {
+		// If a signer is provided and there are intermediates, then either the first intermediate would BE the signer CA


capital BE?

diogomonica · 2017-03-16T14:59:45Z

+		return nil, err
+	}
+
+	signer, err := local.NewSigner(priv, parsedCerts[0], cfsigner.DefaultSigAlgo(priv), SigningPolicy(certExpiry))


Maybe we should refactor newLocalSigner to also receive priv, cert in this order? (instead of cert, priv)

diogomonica · 2017-03-16T15:00:36Z

+	// If the key was loaded from disk unencrypted, but there is a passphrase set,
+	// ensure it is encrypted, so it doesn't hit raft in plain-text
+	// we don't have to check for nil, because if we couldn't pem-decode the bytes, then parsing above would have failed
+	keyBlock, _ := pem.Decode(keyBytes)


Can't wait to remove all of this env passphrase stuff. I'm sorry I added it in the first place. :(

Not at all, could have been useful depending on which way we decided to go for secrets encryption.

diogomonica · 2017-03-17T13:43:03Z

@aaronlehmann review please ✌️

aaronlehmann · 2017-03-17T17:09:50Z

+	case len(keyBytes) > 0 && len(certBytes) > 0: // possibly valid signer
+	default:
+		return nil, errors.New("must provide both a signing key and a signing cert, or neither")
+	}


I'd suggest moving this check out of the call to newLocalSigner, so we can avoid the return nil, nil case. By convention, a function that returns a pointer and an error will generally return a valid pointer if the error is nil. Violating this isn't the end of the world, but changing newLocalSigner to a function that we only call when we expect it to return a LocalSigner or an error would let us stick to this convention.

You could move this to a separate function like possibleValidSigner if you expect to have multiple call sites for newLocalSigner in the future.

aaronlehmann · 2017-03-17T17:11:53Z

+		Intermediates: intermediatePool,
+	}
+	if _, err := parsedCerts[0].Verify(opts); err != nil {
+		return nil, errors.Wrap(err, "error while validating Signing CA Certificate against roots and intermediates")


Why is Signing CA Certificate capitalized?

I was starting to think of it as a proper noun. :) Fixing.

aaronlehmann · 2017-03-17T17:16:39Z

+	// If we have no signing key, then we shouldn't pass a signing cert either (because we don't want a local
+	// signer at all)
+	signingCert := cert
+	if key == nil {


if len(key) == 0

aaronlehmann · 2017-03-17T17:20:46Z

+	if err != nil {
+		return nil, errors.Wrap(err, "invalid signing CA cert")
+	}
+	if err := validateSignatureAlgorithm(parsedCerts[0]); err != nil {


I believe ParseCertificatesPEM can return an empty slice if you pass in whitespace only. So a bounds check is necessary here.

Good point, thanks

aaronlehmann · 2017-03-17T17:23:27Z

Non-blocking: I notice there's a pattern of calling CanSign to check if it's safe to use rca.Signer.cryptoSigner and rca.Signer.parsedCert. I wonder if it would be clearer/safer to replace CanSign with some kind of signer accessor that returns rca.Signer if and only if all the conditions for signing are satisfied (or an error if they are not).

diogomonica · 2017-03-17T17:25:50Z

@aaronlehmann there are situation where not being able to sign is fine though.

aaronlehmann · 2017-03-17T17:36:30Z

Yeah, not saying the error should be bubbled up. Just saying that structuring the check of whether we can sign like this might prevent accidental uses of localSigner when it's not initialized.

… which could be an intermediate CA. Signed-off-by: cyli <ying.li@docker.com>

cyli · 2017-03-17T20:02:46Z

@aaronlehmann It will probably help prevent panics if we try to refer to RootCA.Signer.Key. Will update - this might mean a bunch of extra test changes in the diff, though.

…signer is defined. Signed-off-by: cyli <ying.li@docker.com>

aaronlehmann · 2017-03-18T15:54:01Z

LGTM

cyli force-pushed the separate-root-certs-from-signing-certs branch from fb78408 to 7c78bfa Compare March 14, 2017 23:42

cyli mentioned this pull request Mar 14, 2017

[ca][api]: Root rotation object in cluster #2037

Merged

cyli force-pushed the separate-root-certs-from-signing-certs branch from 7c78bfa to eea8904 Compare March 14, 2017 23:53

cyli changed the title ~~[ca] Separate RootCA signing cert from root certs~~ WIP: [ca] Separate RootCA signing cert from root certs Mar 15, 2017

cyli changed the title ~~WIP: [ca] Separate RootCA signing cert from root certs~~ [ca] Separate RootCA signing cert from root certs Mar 15, 2017

aaronlehmann reviewed Mar 15, 2017

View reviewed changes

cyli force-pushed the separate-root-certs-from-signing-certs branch from eea8904 to b40e7bd Compare March 16, 2017 01:13

diogomonica approved these changes Mar 16, 2017

View reviewed changes

cyli force-pushed the separate-root-certs-from-signing-certs branch from b40e7bd to a12d15d Compare March 16, 2017 18:28

aaronlehmann reviewed Mar 17, 2017

View reviewed changes

Provide a separate field in which to specify the signing certificate,…

1c34390

… which could be an intermediate CA. Signed-off-by: cyli <ying.li@docker.com>

cyli force-pushed the separate-root-certs-from-signing-certs branch 3 times, most recently from c22bbe3 to cba18fd Compare March 18, 2017 01:54

Provide an accessor for the RootCA signer that raises an error if no …

5454ec0

…signer is defined. Signed-off-by: cyli <ying.li@docker.com>

cyli force-pushed the separate-root-certs-from-signing-certs branch from cba18fd to 5454ec0 Compare March 18, 2017 02:13

aaronlehmann merged commit daa6aff into moby:master Mar 18, 2017

cyli deleted the separate-root-certs-from-signing-certs branch March 20, 2017 17:49

cyli restored the separate-root-certs-from-signing-certs branch March 20, 2017 22:35

cyli deleted the separate-root-certs-from-signing-certs branch March 20, 2017 22:37

This was referenced Mar 23, 2017

[feature] CA Root rotation #1990

Closed

[controlapi] Allow starting and aborting root CA rotations during swarm update API #2052

Merged

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

cyli 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

diogomonica commented Mar 17, 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!

aaronlehmann commented Mar 17, 2017

Uh oh!

diogomonica commented Mar 17, 2017

Uh oh!

aaronlehmann commented Mar 17, 2017

Uh oh!

cyli commented Mar 17, 2017

Uh oh!

aaronlehmann commented Mar 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cyli commented Mar 14, 2017 •

edited

Loading

codecov Bot commented Mar 14, 2017 •

edited

Loading

cyli commented Mar 15, 2017 •

edited

Loading