[ca] Add support for verifying and loading certificate chains which include intermediates by cyli · Pull Request #2000 · moby/swarmkit

cyli · 2017-03-02T06:59:44Z

Part of addressing #1990 (supporting intermediates).

This adds support in validating and loading intermediates that are included with TLS leaf certificates, although ValidateCertChain could also be used to validate a chain of intermediates.

(most of this PR is tests and some additional static certs)

codecov-io · 2017-03-02T07:42:33Z

Codecov Report

Merging #2000 into master will increase coverage by 0.11%.
The diff coverage is 98.24%.

@@            Coverage Diff             @@
##           master    #2000      +/-   ##
==========================================
+ Coverage   53.73%   53.84%   +0.11%     
==========================================
  Files         109      109              
  Lines       18970    18996      +26     
==========================================
+ Hits        10193    10229      +36     
+ Misses       7543     7532      -11     
- Partials     1234     1235       +1

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 717adda...a8e341a. Read the comment docs.

aaronlehmann · 2017-03-02T20:01:24Z

LGTM

diogomonica · 2017-03-02T22:33:53Z

+// certificates (ones that aren't used to form a chain up to the given root pool).  This validation also enforces
+// that none of the certificates in the chain can be expired (unless allowExpired is set to true) or not yet
+// valid. This always returns at least 1 certificate.
+func ValidateCertChain(rootPool *x509.CertPool, certs []byte, allowExpired bool) ([]*x509.Certificate, error) {


I don't recall why we need allowExpired in the first place. Insights on whether we can remove this altogether?

We wanted to be able to load expired certs when forcing a new cluster, because then we are the signer and can produce new certs. Otherwise you can't really recover a cluster easily using force new cluster.

I don't think I follow. If we find expired certificates when forcing a new cluster—and we have valid root key material—why don't we simply issue new certificates for ourselves? Why would we add this to our ValidateCertChain method if we can do the right thing?

We don't have the root key material until the raft data is loaded, and before that gets loaded we get the TLS credentials first (either from disk, or from a remote cluster). If the TLS cert is expired, starting the node fails and we can't load the key material in order to generate new ones. Hence this change lets the node start up with expired certs, which it will then attempt to renew right away using the local signer once the raft data has been loaded and the CA server starts.

#1919 was the original PR that added the the check to verifyCertificate.

Not sure why this got collapsed, I just updated the comment.

diogomonica · 2017-03-02T22:36:41Z

+		}
+
+		if i > 0 {
+			// check that the previous cert was signed by this cert


Why do we need this chain to be in order? Is this just being extra pedantic w/ the input cert format?

Pretty much. Since this function is used to verify certificates on disk and certificates downloaded from a CA, I was trying to enforce the following from the TLS 1.2 spec (it's also in 1.1 and 1.0):

certificate_list This is a sequence (chain) of certificates. The sender's certificate MUST come first in the list. Each following certificate MUST directly certify the one preceding it. Because certificate validation requires that root keys be distributed independently, the self-signed certificate that specifies the root certificate authority MAY be omitted from the chain, under the assumption that the remote end must already possess it in order to validate it in any case.

Since according to that, the chain isn't really valid. (I know Go doesn't actually really care, and even completely unrelated certs after the first cert is ok, but it wouldn't be if used with openssl or some such probably?)

Actually I don't know that openssl enforces this ordering either. I just assumed it did since it was in the spec.

We could be just more open in what we accept, I guess, and not care about the order for validating certs that come in, but make sure we get the correct ordering when we hand out certs. (so intermediates that we are handed must form a complete chain, although maybe out of order, and we append the intermediates to TLS certificates in the correct order).

We could be just more open in what we accept

Spoken like a true cryptographer... Er... WAIT, WHAT? :)

Yep I am totally not a cryptographer. :) Go seems pretty open in what they accept though for certificate order, so long as it can establish some chain. When we parse everything in, it will re-order the certificates correctly. Am trying to see whether openssl or other TLS clients accept out-of-order certificates. The spec doesn't seem to have a specific error type or code for out-of-order certs, so I'm not sure who else enforces only accepting this exact order.

Re-reading that comment, I realize I was being overly flippant, sorry! The original way I was thinking about this, which is why I was checking the chain, is that we should trying to enforce the spec.

What I mean by "we should be more open in what we accept" is that we aren't actually doing any of the TLS crypto - Golang does that.

We are just feeding it enough information for it to do so (given a cert and intermediate and root pools, Go will assemble a certificate chain to use that is ordered correctly, and during the TLS handshake, go will enforce that the certificate chain is presented in the right order and received in the right order). In a sense, we are sort of parsing configuration, so I can sort of see being more open how stuff is stored on disk, etc. (Also to be clear, I'm not convinced about this either :) Just trying to argue the other side)

After more discussion with @diogomonica I think we're going to keep being pedantic about ordering when loading from bytes :)

diogomonica · 2017-03-02T22:38:21Z

+	leaf, intermediate, root := testutils.ECDSACertChain[0], testutils.ECDSACertChain[1], testutils.ECDSACertChain[2]
+	intermediateKey, rootKey := testutils.ECDSACertChainKeys[1], testutils.ECDSACertChainKeys[2] // we don't care about the leaf key
+
+	var chain func(certs ...[]byte) []byte


why var chain vs chain :=?

It's recursive - I can't refer back to itself before it's declared otherwise. I can just change this to an iterative function (I only did it recursively to see if I could declare a recursive function using chain := ... - as it turns out, I can't)

verification and loading to use the verify cert chain function so that intermediates in TLS certs can be supported. Signed-off-by: cyli <ying.li@docker.com>

cyli · 2017-03-03T02:17:46Z

Updated comments to reflect why we are doing some additional verification, and changed that chain function to an iterative one.

cyli force-pushed the support-intermediates-in-tls-certs branch 2 times, most recently from a357fd1 to 7d1ba59 Compare March 2, 2017 07:08

cyli mentioned this pull request Mar 2, 2017

[feature] CA Root rotation #1990

Closed

10 tasks

cyli force-pushed the support-intermediates-in-tls-certs branch from 7d1ba59 to e54f3cf Compare March 2, 2017 07:33

cyli force-pushed the support-intermediates-in-tls-certs branch 3 times, most recently from 8b3fe7d to 3d1817e Compare March 2, 2017 19:33

diogomonica reviewed Mar 2, 2017

View reviewed changes

Add utility function to verify certificate chains. Update all the TLS

a8e341a

verification and loading to use the verify cert chain function so that intermediates in TLS certs can be supported. Signed-off-by: cyli <ying.li@docker.com>

cyli force-pushed the support-intermediates-in-tls-certs branch from 3d1817e to a8e341a Compare March 3, 2017 02:15

diogomonica merged commit d31e6f8 into moby:master Mar 3, 2017

cyli deleted the support-intermediates-in-tls-certs branch March 8, 2017 21:46

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 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

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

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

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

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

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

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

Loading

cyli Mar 2, 2017 •

edited

Loading

cyli Mar 2, 2017 •

edited

Loading

cyli Mar 3, 2017 •

edited

Loading

cyli Mar 2, 2017 •

edited

Loading