ca: Add support for appending intermediates to issued TLS certs by cyli · Pull Request #2027 · moby/swarmkit

cyli · 2017-03-09T22:38:58Z

Part of addressing intermediate support in #1990.

cyli · 2017-03-09T22:40:39Z

 -----END EC PRIVATE KEY-----
 `)

-	// ExpiredCert is an ECDSA certificate that expired in 2007


The test in this PR no longer uses this particular cert, but while I was trying stuff I discovered that I had set the validity times wrong for this cert: it was previously valid from 1987-1977 (not a valid date range). So I regenerated this one.

codecov · 2017-03-09T22:59:31Z

Codecov Report

Merging #2027 into master will decrease coverage by 0.06%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master    #2027      +/-   ##
==========================================
- Coverage   53.92%   53.85%   -0.07%     
==========================================
  Files         109      109              
  Lines       19093    19100       +7     
==========================================
- Hits        10296    10287       -9     
- Misses       7553     7575      +22     
+ Partials     1244     1238       -6

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 0e2d9eb...bdc3703. Read the comment docs.

aaronlehmann · 2017-03-10T02:27:56Z

LGTM

cyli · 2017-03-10T02:36:45Z

 	// Calculate the digest for our Root CA bundle
 	digest := digest.FromBytes(certBytes)

+	// TODO(cyli): We currently do not yet support arbitrary chains of intermediates (e.g. the case of an offline root, with the CA signing with


@diogomonica @aaronlehmann I can probably also handle this case but it may be better to do that in another PR, because there could be a bunch of changes (to distinguishing the root certs the RootCA should trust, vs the signing cert).

Sounds good.

diogomonica

Overall LGTM, but just to be clear: with this change we're enforcing that the intermediate always starts with a cross-signed version of the new root by the old root.

diogomonica · 2017-03-10T18:47:27Z


+	// Intermediates contains a bundle of PEM encoded certificates to append to any issued TLS certificates.
+	// If more than one are provided, the first one must have the same public key and subject as the signing
+	// root certificate (or, if no key is provided, at least one of the root certificates), and the rest


we require the Intermediates to also include the root? Usually certificates don't include the full chain all the way up to the root.

No sorry, my comment was not clear - just that a chain can be formed using the intermediates to one of the root certs in the pool (e.g. the intermediates doesn't have to have the root, but one of them should be signed by one of the roots)

Maybe we add little ASCII Diagrams? We will sendup with a certificate that contains: [Leaf] | [Intermediate] and a root that contains [Root]. We guarantee that there is always a valid chain [Leaf] to [Root] using [Intermediate].

😄

sounds good

diogomonica · 2017-03-10T18:48:35Z

 	digest := digest.FromBytes(certBytes)

+	// TODO(cyli): We currently do not yet support arbitrary chains of intermediates (e.g. the case of an offline root, with the CA signing with
+	// an intermediate) - we currently only only intermediates for which the first intermediate is cross-signed version of the signing (first) root


only only

diogomonica · 2017-03-10T18:49:21Z


+	// TODO(cyli): We currently do not yet support arbitrary chains of intermediates (e.g. the case of an offline root, with the CA signing with
+	// an intermediate) - we currently only only intermediates for which the first intermediate is cross-signed version of the signing (first) root
+	// for the purposes of root rotation.  If we wanted to support offline roots, rather than require at the first intermediate is the cross-signed


at the first

diogomonica · 2017-03-10T18:51:36Z

+	// TODO(cyli): We currently do not yet support arbitrary chains of intermediates (e.g. the case of an offline root, with the CA signing with
+	// an intermediate) - we currently only only intermediates for which the first intermediate is cross-signed version of the signing (first) root
+	// for the purposes of root rotation.  If we wanted to support offline roots, rather than require at the first intermediate is the cross-signed
+	// version of the signing CA, it should *either* be a cross-signed version of the signing CA or it should be the issuer of the signing CA.


There is a lot to unpack in this comment. Not sure I understand this last sentence.

Oh I see what you mean. Ok, how I was interpreting this to work was:

Right now, we require that the first cert of the intermediate be a cross-signed root. But that shouldn't be a requirement if we support the offline root case. I originally thought for the offline root case, we'd append our intermediate signing CA, and then append these intermediates, which would chain up to the root.

But you're right that the intermediate signing CA can then just be included in the list of intermediates in which case it will still have the same public key and subject as the current signing CA, and that would make this logic simpler. I'll update.

diogomonica · 2017-03-10T18:52:11Z

 	// Calculate the digest for our Root CA bundle
 	digest := digest.FromBytes(certBytes)

+	// TODO(cyli): We currently do not yet support arbitrary chains of intermediates (e.g. the case of an offline root, with the CA signing with


Sounds good.

cyli · 2017-03-10T21:33:44Z

+	// signing root certificate, and the rest must form a chain, each one certifying the one above it,
+	// as per RFC5246 section 7.4.2:
+	//
+	// RootCA.Cert:          [ signing CA cert][CA cert][CA cert][CA cert]


@diogomonica Does this diagram make sense?

…sued by a RootCA object or by an external CA object. Signed-off-by: cyli <ying.li@docker.com>

cyli commented Mar 9, 2017

View reviewed changes

cyli force-pushed the support-intermediates-in-ca-server branch from e73a5df to 5b44ccc Compare March 9, 2017 22:43

cyli mentioned this pull request Mar 10, 2017

[feature] CA Root rotation #1990

Closed

10 tasks

cyli force-pushed the support-intermediates-in-ca-server branch 2 times, most recently from 31b30cf to a78c89a Compare March 10, 2017 02:15

cyli commented Mar 10, 2017

View reviewed changes

diogomonica reviewed Mar 10, 2017

View reviewed changes

cyli force-pushed the support-intermediates-in-ca-server branch 3 times, most recently from 7faa2df to 9f8d4f1 Compare March 10, 2017 21:01

diogomonica approved these changes Mar 10, 2017

View reviewed changes

cyli force-pushed the support-intermediates-in-ca-server branch from 9f8d4f1 to 6bb9ab4 Compare March 10, 2017 21:33

cyli commented Mar 10, 2017

View reviewed changes

cyli force-pushed the support-intermediates-in-ca-server branch 2 times, most recently from 64c1b2c to a0befd0 Compare March 11, 2017 01:10

Add support for intermediates that get appended to TLS certificate is…

bdc3703

…sued by a RootCA object or by an external CA object. Signed-off-by: cyli <ying.li@docker.com>

cyli force-pushed the support-intermediates-in-ca-server branch from a0befd0 to bdc3703 Compare March 11, 2017 01:11

diogomonica merged commit 1b08186 into moby:master Mar 11, 2017

cyli deleted the support-intermediates-in-ca-server branch March 11, 2017 01:56

Conversation

cyli commented Mar 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

aaronlehmann commented Mar 10, 2017

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!

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!

3 participants

codecov Bot commented Mar 9, 2017 •

edited

Loading