Skip to content

[ca] Add support for verifying and loading certificate chains which include intermediates #2000

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
diogomonica merged 1 commit into from
Mar 3, 2017
Merged

[ca] Add support for verifying and loading certificate chains which include intermediates #2000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 107 additions & 35 deletions ca/certificates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,15 @@ type RootCA struct {
// Key will only be used by the original manager to put the private
// key-material in raft, no signing operations depend on it.
Key []byte
// Cert includes the PEM encoded Certificate for the Root CA

// Cert contains a bundle of PEM encoded Certificate for the Root CA, the first one of which
// must correspond to the key, if provided
Cert []byte
Pool *x509.CertPool
// Digest of the serialized bytes of the certificate

// Digest of the serialized bytes of the certificate(s)
Digest digest.Digest

// This signer will be nil if the node doesn't have the appropriate key material
Signer cfsigner.Signer
}
Expand Down Expand Up @@ -154,25 +158,6 @@ func (rca *RootCA) IssueAndSaveNewCertificates(kw KeyWriter, cn, ou, org string)
return &tlsKeyPair, nil
}

// Normally we can just call cert.Verify(opts), but since we actually want more information about
// whether a certificate is not yet valid or expired, we also need to perform the expiry checks ourselves.
func verifyCertificate(cert *x509.Certificate, opts x509.VerifyOptions, allowExpired bool) error {
_, err := cert.Verify(opts)
if invalidErr, ok := err.(x509.CertificateInvalidError); ok && invalidErr.Reason == x509.Expired {
now := time.Now().UTC()
if now.Before(cert.NotBefore) {
return errors.Wrapf(err, "certificate not valid before %s, and it is currently %s",
cert.NotBefore.UTC().Format(time.RFC1123), now.Format(time.RFC1123))
}
if allowExpired {
return nil
}
return errors.Wrapf(err, "certificate expires at %s, and it is currently %s",
cert.NotAfter.UTC().Format(time.RFC1123), now.Format(time.RFC1123))
}
return err
}

// RequestAndSaveNewCertificates gets new certificates issued, either by signing them locally if a signer is
// available, or by requesting them from the remote server at remoteAddr.
func (rca *RootCA) RequestAndSaveNewCertificates(ctx context.Context, kw KeyWriter, config CertificateRequestConfig) (*tls.Certificate, error) {
Expand Down Expand Up @@ -208,20 +193,9 @@ func (rca *RootCA) RequestAndSaveNewCertificates(ctx context.Context, kw KeyWrit
// Доверяй, но проверяй.
// Before we overwrite our local key + certificate, let's make sure the server gave us one that is valid
// Create an X509Cert so we can .Verify()
certBlock, _ := pem.Decode(signedCert)
if certBlock == nil {
return nil, errors.New("failed to parse certificate PEM")
}
X509Cert, err := x509.ParseCertificate(certBlock.Bytes)
if err != nil {
return nil, err
}
// Include our current root pool
opts := x509.VerifyOptions{
Roots: rca.Pool,
}
// Check to see if this certificate was signed by our CA, and isn't expired
if err := verifyCertificate(X509Cert, opts, false); err != nil {
parsedCerts, err := ValidateCertChain(rca.Pool, signedCert, false)
if err != nil {
return nil, err
}

Expand All @@ -233,7 +207,8 @@ func (rca *RootCA) RequestAndSaveNewCertificates(ctx context.Context, kw KeyWrit

var kekUpdate *KEKData
for i := 0; i < 5; i++ {
kekUpdate, err = rca.getKEKUpdate(ctx, X509Cert, tlsKeyPair, config.ConnBroker)
// ValidateCertChain will always return at least 1 cert, so indexing at 0 is safe
kekUpdate, err = rca.getKEKUpdate(ctx, parsedCerts[0], tlsKeyPair, config.ConnBroker)
if err == nil {
break
}
Expand Down Expand Up @@ -415,6 +390,103 @@ func NewRootCA(certBytes, keyBytes []byte, certExpiry time.Duration) (RootCA, er
return RootCA{Signer: signer, Key: keyBytes, Digest: digest, Cert: certBytes, Pool: pool}, nil
}

// ValidateCertChain checks checks that the certificates provided chain up to the root pool provided. In addition
// it also enforces that every cert in the bundle certificates form a chain, each one certifying the one above,
// as per RFC5246 section 7.4.2, and that every certificate (whether or not it is necessary to form a chain to the root
// pool) is currently valid and not yet expired (unless allowExpiry is set to true).
// This is additional validation not required by go's Certificate.Verify (which allows invalid certs in the
// intermediate pool), because this function is intended to be used when reading certs from untrusted locations such as
// from disk or over a network when a CSR is signed, so it is extra pedantic.
// This function always returns all the parsed certificates in the bundle in order, which means there will always be
// at least 1 certificate if there is no error.
func ValidateCertChain(rootPool *x509.CertPool, certs []byte, allowExpired bool) ([]*x509.Certificate, error) {
// Parse all the certificates in the cert bundle
parsedCerts, err := helpers.ParseCertificatesPEM(certs)
if err != nil {
return nil, err
}
if len(parsedCerts) == 0 {
return nil, errors.New("no certificates to validate")
}
now := time.Now()
// ensure that they form a chain, each one being signed by the one after it
var intermediatePool *x509.CertPool
for i, cert := range parsedCerts {
// Manual expiry validation because we want more information on which certificate in the chain is expired, and
// because this is an easier way to allow expired certs.
if now.Before(cert.NotBefore) {
return nil, errors.Wrapf(
x509.CertificateInvalidError{
Cert: cert,
Reason: x509.Expired,
},
"certificate (%d - %s) not valid before %s, and it is currently %s",
i+1, cert.Subject.CommonName, cert.NotBefore.UTC().Format(time.RFC1123), now.Format(time.RFC1123))
}
if !allowExpired && now.After(cert.NotAfter) {
return nil, errors.Wrapf(
x509.CertificateInvalidError{
Cert: cert,
Reason: x509.Expired,
},
"certificate (%d - %s) not valid after %s, and it is currently %s",
i+1, cert.Subject.CommonName, cert.NotAfter.UTC().Format(time.RFC1123), now.Format(time.RFC1123))
}

if i > 0 {
// check that the previous cert was signed by this cert
Copy link
Copy Markdown
Contributor

@diogomonica diogomonica Mar 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@cyli cyli Mar 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Copy Markdown
Contributor Author

@cyli cyli Mar 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@cyli cyli Mar 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Collaborator

@aaronlehmann aaronlehmann Mar 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could be just more open in what we accept

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

Copy link
Copy Markdown
Contributor Author

@cyli cyli Mar 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@cyli cyli Mar 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

@cyli cyli Mar 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

prevCert := parsedCerts[i-1]
if err := prevCert.CheckSignatureFrom(cert); err != nil {
return nil, errors.Wrapf(err, "certificates do not form a chain: (%d - %s) is not signed by (%d - %s)",
i, prevCert.Subject.CommonName, i+1, cert.Subject.CommonName)
}

if intermediatePool == nil {
intermediatePool = x509.NewCertPool()
}
intermediatePool.AddCert(cert)

}
}

verifyOpts := x509.VerifyOptions{
Roots: rootPool,
Intermediates: intermediatePool,
CurrentTime: now,
}

// If we accept expired certs, try to build a valid cert chain using some subset of the certs. We start off using the
// first certificate's NotAfter as the current time, thus ensuring that the first cert is not expired. If the chain
// still fails to validate due to expiry issues, continue iterating over the rest of the certs.
// If any of the other certs has an earlier NotAfter time, use that time as the current time instead. This insures that
// particular cert, and any that came before it, are not expired. Note that the root that the certs chain up to
// should also not be expired at that "current" time.
if allowExpired {
verifyOpts.CurrentTime = parsedCerts[0].NotAfter.Add(time.Hour)
for _, cert := range parsedCerts {
if !cert.NotAfter.Before(verifyOpts.CurrentTime) {
continue
}
verifyOpts.CurrentTime = cert.NotAfter

_, err = parsedCerts[0].Verify(verifyOpts)
if err == nil {
return parsedCerts, nil
}
}
if invalid, ok := err.(x509.CertificateInvalidError); ok && invalid.Reason == x509.Expired {
return nil, errors.New("there is no time span for which all of the certificates, including a root, are valid")
}
return nil, err
}

_, err = parsedCerts[0].Verify(verifyOpts)
if err != nil {
return nil, err
}
return parsedCerts, nil
}

func ensureCertKeyMatch(cert *x509.Certificate, key crypto.PublicKey) error {
switch certPub := cert.PublicKey.(type) {
case *rsa.PublicKey:
Expand Down
146 changes: 146 additions & 0 deletions ca/certificates_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,9 @@ func TestRequestAndSaveNewCertificates(t *testing.T) {
require.NoError(t, err)
}

// TODO(cyli): add test for RequestAndSaveNewCertificates but with intermediates - this involves adding
// support for appending intermediates on the CA server first

func TestIssueAndSaveNewCertificates(t *testing.T) {
tc := testutils.NewTestCA(t)
defer tc.Stop()
Expand Down Expand Up @@ -741,3 +744,146 @@ func TestNewRootCAWithPassphrase(t *testing.T) {
assert.Contains(t, string(anotherNewRootCA.Key), "Proc-Type: 4,ENCRYPTED")

}

type certTestCase struct {
cert []byte
errorStr string
root []byte
allowExpiry bool
}

func TestValidateCertificateChain(t *testing.T) {
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

chain := func(certs ...[]byte) []byte {
var all []byte
for _, cert := range certs {
all = append(all, cert...)
}
return all
}

now := time.Now()
expiredLeaf := testutils.ReDateCert(t, leaf, intermediate, intermediateKey, now.Add(-10*time.Hour), now.Add(-1*time.Minute))
expiredIntermediate := testutils.ReDateCert(t, intermediate, root, rootKey, now.Add(-10*time.Hour), now.Add(-1*time.Minute))
notYetValidLeaf := testutils.ReDateCert(t, leaf, intermediate, intermediateKey, now.Add(time.Hour), now.Add(2*time.Hour))
notYetValidIntermediate := testutils.ReDateCert(t, intermediate, root, rootKey, now.Add(time.Hour), now.Add(2*time.Hour))

rootPool := x509.NewCertPool()
rootPool.AppendCertsFromPEM(root)

invalids := []certTestCase{
{
cert: nil,
root: root,
errorStr: "no certificates to validate",
},
{
cert: []byte("malformed"),
root: root,
errorStr: "Failed to decode certificate",
},
{
cert: chain(leaf, intermediate, leaf),
root: root,
errorStr: "certificates do not form a chain",
},
{
cert: chain(leaf, intermediate),
root: testutils.ECDSA256SHA256Cert,
errorStr: "unknown authority",
},
{
cert: chain(expiredLeaf, intermediate),
root: root,
errorStr: "not valid after",
},
{
cert: chain(leaf, expiredIntermediate),
root: root,
errorStr: "not valid after",
},
{
cert: chain(notYetValidLeaf, intermediate),
root: root,
errorStr: "not valid before",
},
{
cert: chain(leaf, notYetValidIntermediate),
root: root,
errorStr: "not valid before",
},

// if we allow expiry, we still don't allow not yet valid certs or expired certs that don't chain up to the root
{
cert: chain(notYetValidLeaf, intermediate),
root: root,
allowExpiry: true,
errorStr: "not valid before",
},
{
cert: chain(leaf, notYetValidIntermediate),
root: root,
allowExpiry: true,
errorStr: "not valid before",
},
{
cert: chain(expiredLeaf, intermediate),
root: testutils.ECDSA256SHA256Cert,
allowExpiry: true,
errorStr: "unknown authority",
},

// construct a weird cases where one cert is expired, we allow expiry, but the other cert is not yet valid at the first cert's expiry
// (this is not something that can happen unless we allow expiry, because if the cert periods don't overlap, one or the other will
// be either not yet valid or already expired)
{
cert: chain(
testutils.ReDateCert(t, leaf, intermediate, intermediateKey, now.Add(-3*helpers.OneDay), now.Add(-2*helpers.OneDay)),
testutils.ReDateCert(t, intermediate, root, rootKey, now.Add(-1*helpers.OneDay), now.Add(helpers.OneDay))),
root: root,
allowExpiry: true,
errorStr: "there is no time span",
},
// similarly, but for root pool
{
cert: chain(expiredLeaf, expiredIntermediate),
root: testutils.ReDateCert(t, root, root, rootKey, now.Add(-3*helpers.OneYear), now.Add(-2*helpers.OneYear)),
allowExpiry: true,
errorStr: "there is no time span",
},
}

for _, invalid := range invalids {
pool := x509.NewCertPool()
pool.AppendCertsFromPEM(invalid.root)
_, err := ca.ValidateCertChain(pool, invalid.cert, invalid.allowExpiry)
require.Error(t, err, invalid.errorStr)
require.Contains(t, err.Error(), invalid.errorStr)
}

// these will default to using the root pool, so we don't have to specify the root pool
valids := []certTestCase{
{cert: chain(leaf, intermediate, root)},
{cert: chain(leaf, intermediate)},
{cert: intermediate},
{
cert: chain(expiredLeaf, intermediate),
allowExpiry: true,
},
{
cert: chain(leaf, expiredIntermediate),
allowExpiry: true,
},
{
cert: chain(expiredLeaf, expiredIntermediate),
allowExpiry: true,
},
}

for _, valid := range valids {
_, err := ca.ValidateCertChain(rootPool, valid.cert, valid.allowExpiry)
require.NoError(t, err)
}
}
20 changes: 1 addition & 19 deletions ca/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
cryptorand "crypto/rand"
"crypto/tls"
"crypto/x509"
"encoding/pem"
"fmt"
"math/big"
"math/rand"
Expand Down Expand Up @@ -290,25 +289,8 @@ func LoadSecurityConfig(ctx context.Context, rootCA RootCA, krw *KeyReadWriter,
return nil, err
}

// Create an x509 certificate out of the contents on disk
certBlock, _ := pem.Decode([]byte(cert))
if certBlock == nil {
return nil, errors.New("failed to parse certificate PEM")
}

// Create an X509Cert so we can .Verify()
X509Cert, err := x509.ParseCertificate(certBlock.Bytes)
if err != nil {
return nil, err
}

// Include our root pool
opts := x509.VerifyOptions{
Roots: rootCA.Pool,
}

// Check to see if this certificate was signed by our CA, and isn't expired
if err := verifyCertificate(X509Cert, opts, allowExpired); err != nil {
if _, err := ValidateCertChain(rootCA.Pool, cert, allowExpired); err != nil {
return nil, err
}

Expand Down
Loading