From de8821acd3ada299a17d1d636df4ddb6bb89bd8b Mon Sep 17 00:00:00 2001 From: Robbie McMichael <2044464+robbiemcmichael@users.noreply.github.com> Date: Thu, 24 Dec 2020 23:41:10 +0800 Subject: [PATCH 1/2] kubeadm: support certificate chain validation Fixes an issue where some kubeadm phases fail if a certificate file contains a certificate chain with one or more intermediate CA certificates. The validation algorithm has been changed from requiring that a certificate was signed directly by the root CA to requiring that there is a valid certificate chain back to the root CA. --- cmd/kubeadm/app/cmd/phases/init/certs.go | 4 +-- cmd/kubeadm/app/phases/certs/certs.go | 27 ++++++++++---- cmd/kubeadm/app/util/pkiutil/pki_helpers.go | 39 +++++++++++++++++++++ 3 files changed, 61 insertions(+), 9 deletions(-) diff --git a/cmd/kubeadm/app/cmd/phases/init/certs.go b/cmd/kubeadm/app/cmd/phases/init/certs.go index 29676b66199..f2cf429cade 100644 --- a/cmd/kubeadm/app/cmd/phases/init/certs.go +++ b/cmd/kubeadm/app/cmd/phases/init/certs.go @@ -263,7 +263,7 @@ func runCertPhase(cert *certsphase.KubeadmCert, caCert *certsphase.KubeadmCert) return nil } - if certData, _, err := pkiutil.TryLoadCertAndKeyFromDisk(data.CertificateDir(), cert.BaseName); err == nil { + if certData, intermediates, err := pkiutil.TryLoadCertChainFromDisk(data.CertificateDir(), cert.BaseName); err == nil { certsphase.CheckCertificatePeriodValidity(cert.BaseName, certData) caCertData, err := pkiutil.TryLoadCertFromDisk(data.CertificateDir(), caCert.BaseName) @@ -273,7 +273,7 @@ func runCertPhase(cert *certsphase.KubeadmCert, caCert *certsphase.KubeadmCert) certsphase.CheckCertificatePeriodValidity(caCert.BaseName, caCertData) - if err := certData.CheckSignatureFrom(caCertData); err != nil { + if err := pkiutil.VerifyCertChain(certData, intermediates, caCertData); err != nil { return errors.Wrapf(err, "[certs] certificate %s not signed by CA certificate %s", cert.BaseName, caCert.BaseName) } diff --git a/cmd/kubeadm/app/phases/certs/certs.go b/cmd/kubeadm/app/phases/certs/certs.go index 4e15dc9c957..a17db314447 100644 --- a/cmd/kubeadm/app/phases/certs/certs.go +++ b/cmd/kubeadm/app/phases/certs/certs.go @@ -229,8 +229,14 @@ func writeCertificateFilesIfNotExist(pkiDir string, baseName string, signingCert // Checks if the signed certificate exists in the PKI directory if pkiutil.CertOrKeyExist(pkiDir, baseName) { - // Try to load signed certificate .crt and .key from the PKI directory - signedCert, _, err := pkiutil.TryLoadCertAndKeyFromDisk(pkiDir, baseName) + // Try to load key from the PKI directory + _, err := pkiutil.TryLoadKeyFromDisk(pkiDir, baseName) + if err != nil { + return errors.Wrapf(err, "failure loading %s key", baseName) + } + + // Try to load certificate from the PKI directory + signedCert, intermediates, err := pkiutil.TryLoadCertChainFromDisk(pkiDir, baseName) if err != nil { return errors.Wrapf(err, "failure loading %s certificate", baseName) } @@ -238,7 +244,7 @@ func writeCertificateFilesIfNotExist(pkiDir string, baseName string, signingCert CheckCertificatePeriodValidity(baseName, signedCert) // Check if the existing cert is signed by the given CA - if err := signedCert.CheckSignatureFrom(signingCert); err != nil { + if err := pkiutil.VerifyCertChain(signedCert, intermediates, signingCert); err != nil { return errors.Errorf("certificate %s is not signed by corresponding CA", baseName) } @@ -416,10 +422,17 @@ func validateSignedCert(l certKeyLocation) error { return validateSignedCertWithCA(l, caCert) } -// validateSignedCertWithCA tries to load a certificate and validate it with the given caCert +// validateSignedCertWithCA tries to load a certificate and private key and +// validates that the cert is signed by the given caCert func validateSignedCertWithCA(l certKeyLocation, caCert *x509.Certificate) error { - // Try to load key and signed certificate - signedCert, _, err := pkiutil.TryLoadCertAndKeyFromDisk(l.pkiDir, l.baseName) + // Try to load key from the PKI directory + _, err := pkiutil.TryLoadKeyFromDisk(l.pkiDir, l.baseName) + if err != nil { + return errors.Wrapf(err, "failure loading key for %s", l.baseName) + } + + // Try to load certificate from the PKI directory + signedCert, intermediates, err := pkiutil.TryLoadCertChainFromDisk(l.pkiDir, l.baseName) if err != nil { return errors.Wrapf(err, "failure loading certificate for %s", l.uxName) } @@ -427,7 +440,7 @@ func validateSignedCertWithCA(l certKeyLocation, caCert *x509.Certificate) error CheckCertificatePeriodValidity(l.uxName, signedCert) // Check if the cert is signed by the CA - if err := signedCert.CheckSignatureFrom(caCert); err != nil { + if err := pkiutil.VerifyCertChain(signedCert, intermediates, caCert); err != nil { return errors.Wrapf(err, "certificate %s is not signed by corresponding CA", l.uxName) } return nil diff --git a/cmd/kubeadm/app/util/pkiutil/pki_helpers.go b/cmd/kubeadm/app/util/pkiutil/pki_helpers.go index aa8a76da131..d988f446f38 100644 --- a/cmd/kubeadm/app/util/pkiutil/pki_helpers.go +++ b/cmd/kubeadm/app/util/pkiutil/pki_helpers.go @@ -256,6 +256,21 @@ func TryLoadCertFromDisk(pkiPath, name string) (*x509.Certificate, error) { return cert, nil } +// TryLoadCertChainFromDisk tries to load the cert chain from the disk +func TryLoadCertChainFromDisk(pkiPath, name string) (*x509.Certificate, []*x509.Certificate, error) { + certificatePath := pathForCert(pkiPath, name) + + certs, err := certutil.CertsFromFile(certificatePath) + if err != nil { + return nil, nil, errors.Wrapf(err, "couldn't load the certificate file %s", certificatePath) + } + + cert := certs[0] + intermediates := certs[1:] + + return cert, intermediates, nil +} + // TryLoadKeyFromDisk tries to load the key from the disk and validates that it is valid func TryLoadKeyFromDisk(pkiPath, name string) (crypto.Signer, error) { privateKeyPath := pathForKey(pkiPath, name) @@ -624,3 +639,27 @@ func ValidateCertPeriod(cert *x509.Certificate, offset time.Duration) error { } return nil } + +// VerifyCertChain verifies that a certificate has a valid chain of +// intermediate CAs back to the root CA +func VerifyCertChain(cert *x509.Certificate, intermediates []*x509.Certificate, root *x509.Certificate) error { + rootPool := x509.NewCertPool() + rootPool.AddCert(root) + + intermediatePool := x509.NewCertPool() + for _, c := range intermediates { + intermediatePool.AddCert(c) + } + + verifyOptions := x509.VerifyOptions{ + Roots: rootPool, + Intermediates: intermediatePool, + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageAny}, + } + + if _, err := cert.Verify(verifyOptions); err != nil { + return err + } + + return nil +} From 9022f24aed954cc459f8b14957d79e1ebb2d0a75 Mon Sep 17 00:00:00 2001 From: Robbie McMichael <2044464+robbiemcmichael@users.noreply.github.com> Date: Thu, 24 Dec 2020 23:49:04 +0800 Subject: [PATCH 2/2] kubeadm: tests for certificate chain validation --- cmd/kubeadm/app/util/pkiutil/pki_helpers.go | 84 +++++++- .../app/util/pkiutil/pki_helpers_test.go | 204 +++++++++++++++++- 2 files changed, 272 insertions(+), 16 deletions(-) diff --git a/cmd/kubeadm/app/util/pkiutil/pki_helpers.go b/cmd/kubeadm/app/util/pkiutil/pki_helpers.go index d988f446f38..1c4f148fb71 100644 --- a/cmd/kubeadm/app/util/pkiutil/pki_helpers.go +++ b/cmd/kubeadm/app/util/pkiutil/pki_helpers.go @@ -17,6 +17,7 @@ limitations under the License. package pkiutil import ( + "bytes" "crypto" "crypto/ecdsa" "crypto/elliptic" @@ -79,14 +80,33 @@ func NewCertificateAuthority(config *CertConfig) (*x509.Certificate, crypto.Sign return cert, key, nil } +// NewIntermediateCertificateAuthority creates new certificate and private key for an intermediate certificate authority +func NewIntermediateCertificateAuthority(parentCert *x509.Certificate, parentKey crypto.Signer, config *CertConfig) (*x509.Certificate, crypto.Signer, error) { + key, err := NewPrivateKey(config.PublicKeyAlgorithm) + if err != nil { + return nil, nil, errors.Wrap(err, "unable to create private key while generating intermediate CA certificate") + } + + cert, err := NewSignedCert(config, key, parentCert, parentKey, true) + if err != nil { + return nil, nil, errors.Wrap(err, "unable to sign intermediate CA certificate") + } + + return cert, key, nil +} + // NewCertAndKey creates new certificate and key by passing the certificate authority certificate and key func NewCertAndKey(caCert *x509.Certificate, caKey crypto.Signer, config *CertConfig) (*x509.Certificate, crypto.Signer, error) { + if len(config.Usages) == 0 { + return nil, nil, errors.New("must specify at least one ExtKeyUsage") + } + key, err := NewPrivateKey(config.PublicKeyAlgorithm) if err != nil { return nil, nil, errors.Wrap(err, "unable to create private key") } - cert, err := NewSignedCert(config, key, caCert, caKey) + cert, err := NewSignedCert(config, key, caCert, caKey, false) if err != nil { return nil, nil, errors.Wrap(err, "unable to sign certificate") } @@ -142,6 +162,26 @@ func WriteCert(pkiPath, name string, cert *x509.Certificate) error { return nil } +// WriteCertBundle stores the given certificate bundle at the given location +func WriteCertBundle(pkiPath, name string, certs []*x509.Certificate) error { + for i, cert := range certs { + if cert == nil { + return errors.Errorf("found nil certificate at position %d when writing bundle to file", i) + } + } + + certificatePath := pathForCert(pkiPath, name) + encoded, err := EncodeCertBundlePEM(certs) + if err != nil { + return errors.Wrapf(err, "unable to marshal certificate bundle to PEM") + } + if err := certutil.WriteCert(certificatePath, encoded); err != nil { + return errors.Wrapf(err, "unable to write certificate bundle to file %s", certificatePath) + } + + return nil +} + // WriteKey stores the given key at the given location func WriteKey(pkiPath, name string, key crypto.Signer) error { if key == nil { @@ -548,6 +588,24 @@ func EncodeCertPEM(cert *x509.Certificate) []byte { return pem.EncodeToMemory(&block) } +// EncodeCertBundlePEM returns PEM-endcoded certificate bundle +func EncodeCertBundlePEM(certs []*x509.Certificate) ([]byte, error) { + buf := bytes.Buffer{} + + block := pem.Block{ + Type: CertificateBlockType, + } + + for _, cert := range certs { + block.Bytes = cert.Raw + if err := pem.Encode(&buf, &block); err != nil { + return nil, err + } + } + + return buf.Bytes(), nil +} + // EncodePublicKeyPEM returns PEM-encoded public data func EncodePublicKeyPEM(key crypto.PublicKey) ([]byte, error) { der, err := x509.MarshalPKIXPublicKey(key) @@ -571,7 +629,7 @@ func NewPrivateKey(keyType x509.PublicKeyAlgorithm) (crypto.Signer, error) { } // NewSignedCert creates a signed certificate using the given CA certificate and key -func NewSignedCert(cfg *CertConfig, key crypto.Signer, caCert *x509.Certificate, caKey crypto.Signer) (*x509.Certificate, error) { +func NewSignedCert(cfg *CertConfig, key crypto.Signer, caCert *x509.Certificate, caKey crypto.Signer, isCA bool) (*x509.Certificate, error) { serial, err := cryptorand.Int(cryptorand.Reader, new(big.Int).SetInt64(math.MaxInt64)) if err != nil { return nil, err @@ -579,8 +637,10 @@ func NewSignedCert(cfg *CertConfig, key crypto.Signer, caCert *x509.Certificate, if len(cfg.CommonName) == 0 { return nil, errors.New("must specify a CommonName") } - if len(cfg.Usages) == 0 { - return nil, errors.New("must specify at least one ExtKeyUsage") + + keyUsage := x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature + if isCA { + keyUsage |= x509.KeyUsageCertSign } RemoveDuplicateAltNames(&cfg.AltNames) @@ -590,13 +650,15 @@ func NewSignedCert(cfg *CertConfig, key crypto.Signer, caCert *x509.Certificate, CommonName: cfg.CommonName, Organization: cfg.Organization, }, - DNSNames: cfg.AltNames.DNSNames, - IPAddresses: cfg.AltNames.IPs, - SerialNumber: serial, - NotBefore: caCert.NotBefore, - NotAfter: time.Now().Add(kubeadmconstants.CertificateValidity).UTC(), - KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature, - ExtKeyUsage: cfg.Usages, + DNSNames: cfg.AltNames.DNSNames, + IPAddresses: cfg.AltNames.IPs, + SerialNumber: serial, + NotBefore: caCert.NotBefore, + NotAfter: time.Now().Add(kubeadmconstants.CertificateValidity).UTC(), + KeyUsage: keyUsage, + ExtKeyUsage: cfg.Usages, + BasicConstraintsValid: true, + IsCA: isCA, } certDERBytes, err := x509.CreateCertificate(cryptorand.Reader, &certTmpl, caCert, key.Public(), caKey) if err != nil { diff --git a/cmd/kubeadm/app/util/pkiutil/pki_helpers_test.go b/cmd/kubeadm/app/util/pkiutil/pki_helpers_test.go index ca411adf45f..9c489d4fd64 100644 --- a/cmd/kubeadm/app/util/pkiutil/pki_helpers_test.go +++ b/cmd/kubeadm/app/util/pkiutil/pki_helpers_test.go @@ -199,12 +199,27 @@ func TestWriteCert(t *testing.T) { actual := WriteCert(tmpdir, "foo", caCert) if actual != nil { t.Errorf( - "failed WriteCertAndKey with an error: %v", + "failed WriteCert with an error: %v", actual, ) } } +func TestWriteCertBundle(t *testing.T) { + tmpdir, err := ioutil.TempDir("", "") + if err != nil { + t.Fatalf("Couldn't create tmpdir") + } + defer os.RemoveAll(tmpdir) + + certs := []*x509.Certificate{{}, {}} + + actual := WriteCertBundle(tmpdir, "foo", certs) + if actual != nil { + t.Errorf("failed WriteCertBundle with an error: %v", actual) + } +} + func TestWriteKey(t *testing.T) { tmpdir, err := ioutil.TempDir("", "") if err != nil { @@ -309,14 +324,14 @@ func TestTryLoadCertAndKeyFromDisk(t *testing.T) { Config: certutil.Config{CommonName: "kubernetes"}, }) if err != nil { - t.Errorf( + t.Fatalf( "failed to create cert and key with an error: %v", err, ) } err = WriteCertAndKey(tmpdir, "foo", caCert, caKey) if err != nil { - t.Errorf( + t.Fatalf( "failed to write cert and key with an error: %v", err, ) @@ -366,14 +381,14 @@ func TestTryLoadCertFromDisk(t *testing.T) { Config: certutil.Config{CommonName: "kubernetes"}, }) if err != nil { - t.Errorf( + t.Fatalf( "failed to create cert and key with an error: %v", err, ) } err = WriteCert(tmpdir, "foo", caCert) if err != nil { - t.Errorf( + t.Fatalf( "failed to write cert and key with an error: %v", err, ) @@ -412,6 +427,91 @@ func TestTryLoadCertFromDisk(t *testing.T) { } } +func TestTryLoadCertChainFromDisk(t *testing.T) { + tmpdir, err := ioutil.TempDir("", "") + if err != nil { + t.Fatalf("Couldn't create tmpdir") + } + defer os.RemoveAll(tmpdir) + + caCert, caKey, err := NewCertificateAuthority(&CertConfig{ + Config: certutil.Config{CommonName: "Intermediate CA"}, + }) + if err != nil { + t.Fatalf("failed to create intermediate CA cert and key with an error: %v", err) + } + + cert, _, err := NewCertAndKey(caCert, caKey, &CertConfig{ + Config: certutil.Config{ + CommonName: "kubernetes", + Usages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + }, + }) + if err != nil { + t.Fatalf("failed to create leaf cert and key with an error: %v", err) + } + + err = WriteCert(tmpdir, "leaf", cert) + if err != nil { + t.Fatalf("failed to write cert: %v", err) + } + + bundle := []*x509.Certificate{cert, caCert} + err = WriteCertBundle(tmpdir, "bundle", bundle) + if err != nil { + t.Fatalf("failed to write cert bundle: %v", err) + } + + var tests = []struct { + desc string + path string + name string + expected bool + intermediates int + }{ + { + desc: "empty path and name", + path: "", + name: "", + expected: false, + intermediates: 0, + }, + { + desc: "leaf certificate", + path: tmpdir, + name: "leaf", + expected: true, + intermediates: 0, + }, + { + desc: "certificate bundle", + path: tmpdir, + name: "bundle", + expected: true, + intermediates: 1, + }, + } + for _, rt := range tests { + t.Run(rt.desc, func(t *testing.T) { + _, intermediates, actual := TryLoadCertChainFromDisk(rt.path, rt.name) + if (actual == nil) != rt.expected { + t.Errorf( + "failed TryLoadCertChainFromDisk:\n\texpected: %t\n\t actual: %t", + rt.expected, + (actual == nil), + ) + } + if len(intermediates) != rt.intermediates { + t.Errorf( + "TryLoadCertChainFromDisk returned the wrong number of intermediate certificates:\n\texpected: %d\n\t actual: %d", + rt.intermediates, + len(intermediates), + ) + } + }) + } +} + func TestTryLoadKeyFromDisk(t *testing.T) { var tests = []struct { @@ -804,3 +904,97 @@ func TestRemoveDuplicateAltNames(t *testing.T) { } } } + +func TestVerifyCertChain(t *testing.T) { + tmpdir, err := ioutil.TempDir("", "") + if err != nil { + t.Fatalf("Couldn't create tmpdir") + } + defer os.RemoveAll(tmpdir) + + rootCert1, rootKey1, err := NewCertificateAuthority(&CertConfig{ + Config: certutil.Config{CommonName: "Root CA 1"}, + }) + if err != nil { + t.Errorf("failed to create root CA cert and key with an error: %v", err) + } + + leafCert1, _, err := NewCertAndKey(rootCert1, rootKey1, &CertConfig{ + Config: certutil.Config{ + CommonName: "Leaf Certificate 1", + Usages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + }, + }) + if err != nil { + t.Errorf("failed to create leaf cert and key with an error: %v", err) + } + + rootCert2, rootKey2, err := NewCertificateAuthority(&CertConfig{ + Config: certutil.Config{CommonName: "Root CA 2"}, + }) + if err != nil { + t.Errorf("failed to create root CA cert and key with an error: %v", err) + } + + intCert2, intKey2, err := NewIntermediateCertificateAuthority(rootCert2, rootKey2, &CertConfig{ + Config: certutil.Config{ + CommonName: "Intermediate CA 2", + Usages: []x509.ExtKeyUsage{}, + }, + }) + if err != nil { + t.Errorf("failed to create intermediate CA cert and key with an error: %v", err) + } + + leafCert2, _, err := NewCertAndKey(intCert2, intKey2, &CertConfig{ + Config: certutil.Config{ + CommonName: "Leaf Certificate 2", + Usages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + }, + }) + if err != nil { + t.Errorf("failed to create leaf cert and key with an error: %v", err) + } + + var tests = []struct { + desc string + leaf *x509.Certificate + intermediates []*x509.Certificate + root *x509.Certificate + expected bool + }{ + { + desc: "without any intermediate CAs", + leaf: leafCert1, + intermediates: []*x509.Certificate{}, + root: rootCert1, + expected: true, + }, + { + desc: "missing intermediate CA", + leaf: leafCert2, + intermediates: []*x509.Certificate{}, + root: rootCert2, + expected: false, + }, + { + desc: "with one intermediate CA", + leaf: leafCert2, + intermediates: []*x509.Certificate{intCert2}, + root: rootCert2, + expected: true, + }, + } + for _, rt := range tests { + t.Run(rt.desc, func(t *testing.T) { + actual := VerifyCertChain(rt.leaf, rt.intermediates, rt.root) + if (actual == nil) != rt.expected { + t.Errorf( + "failed VerifyCertChain:\n\texpected: %t\n\t actual: %t", + rt.expected, + (actual == nil), + ) + } + }) + } +}