From f22f594d34fa5d834f50acbaff05696f08704f6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Fern=C3=A1ndez=20L=C3=B3pez?= Date: Fri, 18 Jan 2019 22:14:07 +0100 Subject: [PATCH] kubeadm: verify that present certificates contain at least the required SANs This avoids ending in a wrong cluster state by assuming that the present certificates will work. It is specially important when we are growing etcd from 1 member to 2, in which case in case of failure upon joining etcd will be unavailable. --- cmd/kubeadm/app/phases/certs/certlist.go | 3 +- cmd/kubeadm/app/phases/certs/certs.go | 27 +++++++++--- cmd/kubeadm/app/phases/certs/certs_test.go | 51 +++++++++++++++++----- cmd/kubeadm/app/util/certs/util.go | 19 ++++---- 4 files changed, 73 insertions(+), 27 deletions(-) diff --git a/cmd/kubeadm/app/phases/certs/certlist.go b/cmd/kubeadm/app/phases/certs/certlist.go index 24b2342c9d..3450bbf843 100644 --- a/cmd/kubeadm/app/phases/certs/certlist.go +++ b/cmd/kubeadm/app/phases/certs/certlist.go @@ -69,10 +69,11 @@ func (k *KubeadmCert) CreateFromCA(ic *kubeadmapi.InitConfiguration, caCert *x50 caCert, cert, key, + cfg, ) if err != nil { - return errors.Wrapf(err, "failed to write certificate %q", k.Name) + return errors.Wrapf(err, "failed to write or validate certificate %q", k.Name) } return nil diff --git a/cmd/kubeadm/app/phases/certs/certs.go b/cmd/kubeadm/app/phases/certs/certs.go index 6a0e659be4..d681fa2f14 100644 --- a/cmd/kubeadm/app/phases/certs/certs.go +++ b/cmd/kubeadm/app/phases/certs/certs.go @@ -220,7 +220,7 @@ func writeCertificateAuthorithyFilesIfNotExist(pkiDir string, baseName string, c // If there already is a certificate file at the given path; kubeadm tries to load it and check if the values in the // existing and the expected certificate equals. If they do; kubeadm will just skip writing the file as it's up-to-date, // otherwise this function returns an error. -func writeCertificateFilesIfNotExist(pkiDir string, baseName string, signingCert *x509.Certificate, cert *x509.Certificate, key *rsa.PrivateKey) error { +func writeCertificateFilesIfNotExist(pkiDir string, baseName string, signingCert *x509.Certificate, cert *x509.Certificate, key *rsa.PrivateKey, cfg *certutil.Config) error { // Checks if the signed certificate exists in the PKI directory if pkiutil.CertOrKeyExist(pkiDir, baseName) { @@ -235,10 +235,11 @@ func writeCertificateFilesIfNotExist(pkiDir string, baseName string, signingCert return errors.Errorf("certificate %s is not signed by corresponding CA", baseName) } - // kubeadm doesn't validate the existing certificate more than this; - // Basically, if we find a certificate file with the same path; and it is signed by - // the expected certificate authority, kubeadm thinks those files are equal and - // doesn't bother writing a new file + // Check if the certificate has the correct attributes + if err := validateCertificateWithConfig(signedCert, baseName, cfg); err != nil { + return err + } + fmt.Printf("[certs] Using the existing %q certificate and key\n", baseName) } else { // Write .crt and .key files to disk @@ -458,3 +459,19 @@ func validatePrivatePublicKey(l certKeyLocation) error { } return nil } + +// validateCertificateWithConfig makes sure that a given certificate is valid at +// least for the SANs defined in the configuration. +func validateCertificateWithConfig(cert *x509.Certificate, baseName string, cfg *certutil.Config) error { + for _, dnsName := range cfg.AltNames.DNSNames { + if err := cert.VerifyHostname(dnsName); err != nil { + return errors.Wrapf(err, "certificate %s is invalid", baseName) + } + } + for _, ipAddress := range cfg.AltNames.IPs { + if err := cert.VerifyHostname(ipAddress.String()); err != nil { + return errors.Wrapf(err, "certificate %s is invalid", baseName) + } + } + return nil +} diff --git a/cmd/kubeadm/app/phases/certs/certs_test.go b/cmd/kubeadm/app/phases/certs/certs_test.go index f83437de1f..79a54863da 100644 --- a/cmd/kubeadm/app/phases/certs/certs_test.go +++ b/cmd/kubeadm/app/phases/certs/certs_test.go @@ -21,6 +21,7 @@ import ( "crypto/sha256" "crypto/x509" "io/ioutil" + "net" "os" "path" "path/filepath" @@ -76,8 +77,8 @@ func TestWriteCertificateAuthorithyFilesIfNotExist(t *testing.T) { }, { // cert exists, but it is not a ca > err setupFunc: func(pkiDir string) error { - cert, key := certstestutil.CreateTestCert(t, setupCert, setupKey) - return writeCertificateFilesIfNotExist(pkiDir, "dummy", setupCert, cert, key) + cert, key, config := certstestutil.CreateTestCert(t, setupCert, setupKey, certutil.AltNames{}) + return writeCertificateFilesIfNotExist(pkiDir, "dummy", setupCert, cert, key, config) }, expectedError: true, }, @@ -125,10 +126,16 @@ func TestWriteCertificateAuthorithyFilesIfNotExist(t *testing.T) { } func TestWriteCertificateFilesIfNotExist(t *testing.T) { + altNames := certutil.AltNames{ + DNSNames: []string{"example.com"}, + IPs: []net.IP{ + net.IPv4(0, 0, 0, 0), + }, + } caCert, caKey := certstestutil.CreateCACert(t) - setupCert, setupKey := certstestutil.CreateTestCert(t, caCert, caKey) - cert, key := certstestutil.CreateTestCert(t, caCert, caKey) + setupCert, setupKey, _ := certstestutil.CreateTestCert(t, caCert, caKey, altNames) + cert, key, config := certstestutil.CreateTestCert(t, caCert, caKey, altNames) var tests = []struct { setupFunc func(pkiDir string) error @@ -138,9 +145,29 @@ func TestWriteCertificateFilesIfNotExist(t *testing.T) { { // cert does not exists > cert written expectedCert: cert, }, - { // cert exists, is signed by the same ca > existing cert used + { // cert exists, is signed by the same ca, missing SANs (dns name) > err setupFunc: func(pkiDir string) error { - return writeCertificateFilesIfNotExist(pkiDir, "dummy", caCert, setupCert, setupKey) + setupCert, setupKey, setupConfig := certstestutil.CreateTestCert(t, caCert, caKey, certutil.AltNames{ + IPs: []net.IP{ + net.IPv4(0, 0, 0, 0), + }, + }) + return writeCertificateFilesIfNotExist(pkiDir, "dummy", caCert, setupCert, setupKey, setupConfig) + }, + expectedError: true, + }, + { // cert exists, is signed by the same ca, missing SANs (IP address) > err + setupFunc: func(pkiDir string) error { + setupCert, setupKey, setupConfig := certstestutil.CreateTestCert(t, caCert, caKey, certutil.AltNames{ + DNSNames: []string{"example.com"}, + }) + return writeCertificateFilesIfNotExist(pkiDir, "dummy", caCert, setupCert, setupKey, setupConfig) + }, + expectedError: true, + }, + { // cert exists, is signed by the same ca, all SANs present > existing cert used + setupFunc: func(pkiDir string) error { + return writeCertificateFilesIfNotExist(pkiDir, "dummy", caCert, setupCert, setupKey, config) }, expectedCert: setupCert, }, @@ -154,9 +181,9 @@ func TestWriteCertificateFilesIfNotExist(t *testing.T) { { // cert exists, is signed by another ca > err setupFunc: func(pkiDir string) error { anotherCaCert, anotherCaKey := certstestutil.CreateCACert(t) - anotherCert, anotherKey := certstestutil.CreateTestCert(t, anotherCaCert, anotherCaKey) + anotherCert, anotherKey, config := certstestutil.CreateTestCert(t, anotherCaCert, anotherCaKey, certutil.AltNames{}) - return writeCertificateFilesIfNotExist(pkiDir, "dummy", anotherCaCert, anotherCert, anotherKey) + return writeCertificateFilesIfNotExist(pkiDir, "dummy", anotherCaCert, anotherCert, anotherKey, config) }, expectedError: true, }, @@ -176,13 +203,13 @@ func TestWriteCertificateFilesIfNotExist(t *testing.T) { } // executes create func - err := writeCertificateFilesIfNotExist(tmpdir, "dummy", caCert, cert, key) + err := writeCertificateFilesIfNotExist(tmpdir, "dummy", caCert, cert, key, config) if !test.expectedError && err != nil { t.Errorf("error writeCertificateFilesIfNotExist failed when not expected to fail: %v", err) continue } else if test.expectedError && err == nil { - t.Error("error writeCertificateFilesIfNotExist didn't failed when expected") + t.Error("error writeCertificateFilesIfNotExist didn't fail when expected") continue } else if test.expectedError { continue @@ -355,7 +382,7 @@ func TestNewCACertAndKey(t *testing.T) { func TestSharedCertificateExists(t *testing.T) { caCert, caKey := certstestutil.CreateCACert(t) - _, key := certstestutil.CreateTestCert(t, caCert, caKey) + _, key, _ := certstestutil.CreateTestCert(t, caCert, caKey, certutil.AltNames{}) publicKey := &key.PublicKey var tests = []struct { @@ -536,7 +563,7 @@ func TestUsingExternalCA(t *testing.T) { func TestValidateMethods(t *testing.T) { caCert, caKey := certstestutil.CreateCACert(t) - cert, key := certstestutil.CreateTestCert(t, caCert, caKey) + cert, key, _ := certstestutil.CreateTestCert(t, caCert, caKey, certutil.AltNames{}) tests := []struct { name string diff --git a/cmd/kubeadm/app/util/certs/util.go b/cmd/kubeadm/app/util/certs/util.go index 3a867d85d9..32fe363945 100644 --- a/cmd/kubeadm/app/util/certs/util.go +++ b/cmd/kubeadm/app/util/certs/util.go @@ -145,17 +145,18 @@ func CreateCACert(t *testing.T) (*x509.Certificate, *rsa.PrivateKey) { return cert, key } -// CreateTestCert makes a generic certficate with the given CA. -func CreateTestCert(t *testing.T, caCert *x509.Certificate, caKey *rsa.PrivateKey) (*x509.Certificate, *rsa.PrivateKey) { - cert, key, err := pkiutil.NewCertAndKey(caCert, caKey, - &certutil.Config{ - CommonName: "testCert", - Usages: []x509.ExtKeyUsage{x509.ExtKeyUsageAny}, - }) +// CreateTestCert makes a generic certificate with the given CA and alternative names. +func CreateTestCert(t *testing.T, caCert *x509.Certificate, caKey *rsa.PrivateKey, altNames certutil.AltNames) (*x509.Certificate, *rsa.PrivateKey, *certutil.Config) { + config := &certutil.Config{ + CommonName: "testCert", + Usages: []x509.ExtKeyUsage{x509.ExtKeyUsageAny}, + AltNames: altNames, + } + cert, key, err := pkiutil.NewCertAndKey(caCert, caKey, config) if err != nil { t.Fatalf("couldn't create test cert: %v", err) } - return cert, key + return cert, key, config } // CertTestCase is a configuration of certificates and whether it's expected to work. @@ -172,7 +173,7 @@ func GetSparseCertTestCases(t *testing.T) []CertTestCase { fpCACert, fpCAKey := CreateCACert(t) etcdCACert, etcdCAKey := CreateCACert(t) - fpCert, fpKey := CreateTestCert(t, fpCACert, fpCAKey) + fpCert, fpKey, _ := CreateTestCert(t, fpCACert, fpCAKey, certutil.AltNames{}) return []CertTestCase{ {