[ca] Add utility to create cross-signed roots to the RootCA. by cyli · Pull Request #2001 · moby/swarmkit

cyli · 2017-03-02T07:26:41Z

This PR provides 2 ways of creating cross-signed roots, to be used as intermediates during root rotation:

Take a certificate, and cross-sign the certificate. This can be used when rotating from internal->internal, and internal->external.
Generate a CA CSR from the root certificate, and submit it to an external CA to be signed as an intermediate. This can be used when rotating from external->internal.

external->external cannot be supported without adding an extra API to the external signing server (either taking query parameters, or adding a new URL) in order for the external CA to also just sign an intermediate using just a certificate (because we need the private key material in order to generate an actual CSR).

After further discussion with @diogomonica and @jlhawn , we can probably shelve this use case for now, because a user can rotate from external->internal, and then internal->external again, although it will be slower.

If it does need to be supported, we can add the extra API requirement to the external server.

This PR is part of addressing #1990.

codecov-io · 2017-03-02T07:39:48Z

Codecov Report

Merging #2001 into master will increase coverage by 0.05%.
The diff coverage is 73.33%.

@@            Coverage Diff             @@
##           master    #2001      +/-   ##
==========================================
+ Coverage   53.74%   53.79%   +0.05%     
==========================================
  Files         109      109              
  Lines       19003    19051      +48     
==========================================
+ Hits        10213    10249      +36     
  Misses       7559     7559              
- Partials     1231     1243      +12

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 cefa878...220f504. Read the comment docs.

aaronlehmann · 2017-03-02T18:13:43Z

+
+	newCert, err := helpers.ParseCertificatePEM(otherRootCert)
+	if err != nil {
+		return nil, err


Let's wrap this error with a mention that it's a parse error of the supplied certificate.

aaronlehmann · 2017-03-02T18:13:54Z

+	// create a new cert with exactly the same parameters, including the public key and exact NotBefore and NotAfter
+	rootCert, err := helpers.ParseCertificatePEM(rca.Cert)
+	if err != nil {
+		return nil, err


Let's wrap this error with a mention that it's a parse error of current root certificate.

aaronlehmann · 2017-03-02T18:14:07Z

+	}
+	rootSigner, err := helpers.ParsePrivateKeyPEM(rca.Key)
+	if err != nil {
+		return nil, err


Let's wrap this error with a mention that it's a parse error of the current root key.

cyli · 2017-03-08T20:14:37Z

I think this is finally ready for review.

diogomonica

Overall LGTM!

diogomonica · 2017-03-09T02:26:07Z

+}
+
+// generateSignRequest generates a signer.SignRequest for a cross-signed certificate
+func (rca *RootCA) generateSignRequest() (*cfsigner.SignRequest, error) {


Why is this a method on RootCA? Implies that we're going to be doing this operation to the currently loaded root, when in fact we only do it to a RootCA that is passed as an argument.

No particular reason - happy to move it.

diogomonica · 2017-03-09T02:31:48Z

+		if ext.Id.Equal(BasicConstraintsOID) {
+			req.Extensions = append(req.Extensions, cfsigner.Extension{
+				ID:       config.OID(ext.Id),
+				Critical: ext.Critical,


Technically we the "critical" flag is for extensions which are not standard right? I think this is fine though, since it follows the semantics of "field which is critical for safe operation".

I'm... not sure. :) I was just translating the struct from a pkix.Extension to a signer.Extension object. Should I just leave this out?

I think this is fine like this. Just wanted to see if you had some rationale around it. Worth adding a comment?

diogomonica · 2017-03-09T02:32:57Z

+	}
+	// cfssl actually ignores non subject alt name extensions in the CSR, so we have to add the CA extension in the signing
+	// request as well
+	for _, ext := range rootCert.Extensions {


Wouldn't it be easier to simply add all of the extensions instead of filtering for Basic Constraints?

The other constraints would also have to be whitelisted by the signing policy - it seemed easier to restrict the types of extension added, as opposed to adding every possible extension to the whitelist.

Wouldn't we want a certificate with anything else to fail instead of being silently signed w/ a portion of the original extensions?

I thought all CA certs generally have at least the 3 X509v3 extensions: key usage, basic constraints, and the subject key identifier.

We probably don't need to pass the other 2, because the CA signing policy should generate the correct values for them.

But this cert comes from a RootCA object - if we want to reject CA certs with extra extensions, the validation should probably go into constructor NewRootCA instead. Do we want to do more extensive cert validation?

No, I guess this is fine.

Let me double check to see if this is actually needed - the CFSSL comment suggested that it was, but looking at the comment vs the implementation of <local signer>.Sign that may not be accurate.

Ok, so you DO need to pass the extensions in the sign request if the signer's policy has a CSR whitelist. If there is no CSR whitelist, and the signer just signs whatever the CSR says, then we can extensions exactly as they were in the original root CA and they will be respected by the signer.

So we can use CFSSL's initca.CAPolicy() on the external signer, and just pass the extensions straight through if we like. But if the signer is at all more strict (whitelisting any CSR fields) then that will fail.

…ertificate, and a utility to generate an intermediate by submitting a CSR to an external CA (which must support CA signing). Signed-off-by: cyli <ying.li@docker.com>

diogomonica · 2017-03-10T00:03:20Z

Ping @aaronlehmann to take another look. Let's get this merged.

aaronlehmann · 2017-03-10T01:01:59Z

The mechanics of creating the CSR are a bit over my head, but the concept makes sense, there are unit tests, and I guess we'll find out soon enough if this works in practice.

LGTM

cyli · 2017-03-10T01:04:20Z

The mechanics of creating the CSR are a bit over my head

Mine too :P Let's hope I didn't mess that up :)

cyli mentioned this pull request Mar 2, 2017

[feature] CA Root rotation #1990

Closed

10 tasks

aaronlehmann reviewed Mar 2, 2017

View reviewed changes

cyli force-pushed the support-cross-signed-certs branch 3 times, most recently from d7012e5 to e894eef Compare March 8, 2017 20:04

cyli changed the title ~~[ca] WIP: Add utility to create cross-signed roots to the RootCA.~~ [ca] Add utility to create cross-signed roots to the RootCA. Mar 8, 2017

diogomonica approved these changes Mar 9, 2017

View reviewed changes

Add a utility generate an intermediate by cross-signing an existing c…

220f504

…ertificate, and a utility to generate an intermediate by submitting a CSR to an external CA (which must support CA signing). Signed-off-by: cyli <ying.li@docker.com>

cyli force-pushed the support-cross-signed-certs branch from e894eef to 220f504 Compare March 9, 2017 19:10

aaronlehmann merged commit d930df1 into moby:master Mar 10, 2017

cyli deleted the support-cross-signed-certs branch March 10, 2017 01:02

Conversation

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

Uh oh!

Uh oh!

codecov-io commented Mar 2, 2017 • edited by codecov Bot 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!

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

cyli Mar 10, 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!

diogomonica commented Mar 10, 2017

Uh oh!

aaronlehmann commented Mar 10, 2017

Uh oh!

cyli commented Mar 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cyli commented Mar 2, 2017 •

edited

Loading

codecov-io commented Mar 2, 2017 •

edited by codecov Bot

Loading

cyli Mar 10, 2017 •

edited

Loading