From b5b9698fbf43152a950504a0b68d0b4b2eda6d7b Mon Sep 17 00:00:00 2001 From: "Lubomir I. Ivanov" Date: Fri, 4 Sep 2020 03:57:16 +0300 Subject: [PATCH] kubeadm: print warnings on invalid cert period instead of erroring out Client side period validation of certificates should not be fatal, as local clock skews are not so uncommon. The validation should be left to the running servers. - Remove this validation from TryLoadCertFromDisk(). - Add a new function ValidateCertPeriod(), that can be used for this purpose on demand. - In phases/certs add a new function CheckCertificatePeriodValidity() that will print warnings if a certificate does not pass period validation, and caches certificates that were already checked. - Use the function in a number of places where certificates are loaded from disk. --- cmd/kubeadm/app/cmd/phases/init/certs.go | 9 ++++- cmd/kubeadm/app/phases/certs/certlist.go | 3 ++ cmd/kubeadm/app/phases/certs/certs.go | 37 +++++++++++++++++++ cmd/kubeadm/app/phases/kubeconfig/BUILD | 1 + .../app/phases/kubeconfig/kubeconfig.go | 10 +++++ cmd/kubeadm/app/util/pkiutil/pki_helpers.go | 25 ++++++++----- 6 files changed, 74 insertions(+), 11 deletions(-) diff --git a/cmd/kubeadm/app/cmd/phases/init/certs.go b/cmd/kubeadm/app/cmd/phases/init/certs.go index 4338ba5d40e..29676b66199 100644 --- a/cmd/kubeadm/app/cmd/phases/init/certs.go +++ b/cmd/kubeadm/app/cmd/phases/init/certs.go @@ -22,6 +22,7 @@ import ( "github.com/pkg/errors" "github.com/spf13/pflag" + kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm" kubeadmscheme "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/scheme" kubeadmapiv1beta2 "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1beta2" @@ -228,7 +229,9 @@ func runCAPhase(ca *certsphase.KubeadmCert) func(c workflow.RunData) error { return nil } - if _, err := pkiutil.TryLoadCertFromDisk(data.CertificateDir(), ca.BaseName); err == nil { + if cert, err := pkiutil.TryLoadCertFromDisk(data.CertificateDir(), ca.BaseName); err == nil { + certsphase.CheckCertificatePeriodValidity(ca.BaseName, cert) + if _, err := pkiutil.TryLoadKeyFromDisk(data.CertificateDir(), ca.BaseName); err == nil { fmt.Printf("[certs] Using existing %s certificate authority\n", ca.BaseName) return nil @@ -261,11 +264,15 @@ func runCertPhase(cert *certsphase.KubeadmCert, caCert *certsphase.KubeadmCert) } if certData, _, err := pkiutil.TryLoadCertAndKeyFromDisk(data.CertificateDir(), cert.BaseName); err == nil { + certsphase.CheckCertificatePeriodValidity(cert.BaseName, certData) + caCertData, err := pkiutil.TryLoadCertFromDisk(data.CertificateDir(), caCert.BaseName) if err != nil { return errors.Wrapf(err, "couldn't load CA certificate %s", caCert.Name) } + certsphase.CheckCertificatePeriodValidity(caCert.BaseName, caCertData) + if err := certData.CheckSignatureFrom(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/certlist.go b/cmd/kubeadm/app/phases/certs/certlist.go index 7b1a57c02fb..521ddf86df0 100644 --- a/cmd/kubeadm/app/phases/certs/certlist.go +++ b/cmd/kubeadm/app/phases/certs/certlist.go @@ -124,6 +124,9 @@ func (t CertificateTree) CreateTree(ic *kubeadmapi.InitConfiguration) error { caCert, err := pkiutil.TryLoadCertFromDisk(ic.CertificatesDir, ca.BaseName) if err == nil { + // Validate period + CheckCertificatePeriodValidity(ca.BaseName, caCert) + // Cert exists already, make sure it's valid if !caCert.IsCA { return errors.Errorf("certificate %q is not a CA", ca.Name) diff --git a/cmd/kubeadm/app/phases/certs/certs.go b/cmd/kubeadm/app/phases/certs/certs.go index 094f4303375..fee0e23e823 100644 --- a/cmd/kubeadm/app/phases/certs/certs.go +++ b/cmd/kubeadm/app/phases/certs/certs.go @@ -22,6 +22,7 @@ import ( "fmt" "os" "path/filepath" + "sync" "github.com/pkg/errors" "k8s.io/client-go/util/keyutil" @@ -32,6 +33,12 @@ import ( kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants" ) +var ( + // certPeriodValidation is used to store if period validation was done for a certificate + certPeriodValidationMutex sync.Mutex + certPeriodValidation = map[string]struct{}{} +) + // CreatePKIAssets will create and write to disk all PKI assets necessary to establish the control plane. // If the PKI assets already exists in the target folder, they are used only if evaluated equal; otherwise an error is returned. func CreatePKIAssets(cfg *kubeadmapi.InitConfiguration) error { @@ -166,6 +173,8 @@ func LoadCertificateAuthority(pkiDir string, baseName string) (*x509.Certificate if err != nil { return nil, nil, errors.Wrapf(err, "failure loading %s certificate authority", baseName) } + // Validate period + CheckCertificatePeriodValidity(baseName, caCert) // Make sure the loaded CA cert actually is a CA if !caCert.IsCA { @@ -189,6 +198,8 @@ func writeCertificateAuthorityFilesIfNotExist(pkiDir string, baseName string, ca if err != nil { return errors.Wrapf(err, "failure loading %s certificate", baseName) } + // Validate period + CheckCertificatePeriodValidity(baseName, caCert) // Check if the existing cert is a CA if !caCert.IsCA { @@ -223,6 +234,8 @@ func writeCertificateFilesIfNotExist(pkiDir string, baseName string, signingCert if err != nil { return errors.Wrapf(err, "failure loading %s certificate", baseName) } + // Validate period + CheckCertificatePeriodValidity(baseName, signedCert) // Check if the existing cert is signed by the given CA if err := signedCert.CheckSignatureFrom(signingCert); err != nil { @@ -364,6 +377,8 @@ func validateCACert(l certKeyLocation) error { if err != nil { return errors.Wrapf(err, "failure loading certificate for %s", l.uxName) } + // Validate period + CheckCertificatePeriodValidity(l.uxName, caCert) // Check if cert is a CA if !caCert.IsCA { @@ -394,6 +409,8 @@ func validateSignedCert(l certKeyLocation) error { if err != nil { return errors.Wrapf(err, "failure loading certificate authority for %s", l.uxName) } + // Validate period + CheckCertificatePeriodValidity(l.uxName, caCert) return validateSignedCertWithCA(l, caCert) } @@ -405,6 +422,8 @@ func validateSignedCertWithCA(l certKeyLocation, caCert *x509.Certificate) error if err != nil { return errors.Wrapf(err, "failure loading certificate for %s", l.uxName) } + // Validate period + CheckCertificatePeriodValidity(l.uxName, signedCert) // Check if the cert is signed by the CA if err := signedCert.CheckSignatureFrom(caCert); err != nil { @@ -438,3 +457,21 @@ func validateCertificateWithConfig(cert *x509.Certificate, baseName string, cfg } return nil } + +// CheckCertificatePeriodValidity takes a certificate and prints a warning if its period +// is not valid related to the current time. It does so only if the certificate was not validated already +// by keeping track with a cache. +func CheckCertificatePeriodValidity(baseName string, cert *x509.Certificate) { + certPeriodValidationMutex.Lock() + if _, exists := certPeriodValidation[baseName]; exists { + certPeriodValidationMutex.Unlock() + return + } + certPeriodValidation[baseName] = struct{}{} + certPeriodValidationMutex.Unlock() + + klog.V(5).Infof("validating certificate period for %s certificate", baseName) + if err := pkiutil.ValidateCertPeriod(cert, 0); err != nil { + klog.Warningf("WARNING: could not validate bounds for certificate %s: %v", baseName, err) + } +} diff --git a/cmd/kubeadm/app/phases/kubeconfig/BUILD b/cmd/kubeadm/app/phases/kubeconfig/BUILD index b52f9f5011f..066bf88dbbb 100644 --- a/cmd/kubeadm/app/phases/kubeconfig/BUILD +++ b/cmd/kubeadm/app/phases/kubeconfig/BUILD @@ -16,6 +16,7 @@ go_library( deps = [ "//cmd/kubeadm/app/apis/kubeadm:go_default_library", "//cmd/kubeadm/app/constants:go_default_library", + "//cmd/kubeadm/app/phases/certs:go_default_library", "//cmd/kubeadm/app/util:go_default_library", "//cmd/kubeadm/app/util/kubeconfig:go_default_library", "//cmd/kubeadm/app/util/pkiutil:go_default_library", diff --git a/cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go b/cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go index 785376b3a9a..2adfce61078 100644 --- a/cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go +++ b/cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go @@ -34,6 +34,7 @@ import ( "k8s.io/klog/v2" kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm" kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants" + certsphase "k8s.io/kubernetes/cmd/kubeadm/app/phases/certs" kubeadmutil "k8s.io/kubernetes/cmd/kubeadm/app/util" kubeconfigutil "k8s.io/kubernetes/cmd/kubeadm/app/util/kubeconfig" pkiutil "k8s.io/kubernetes/cmd/kubeadm/app/util/pkiutil" @@ -124,6 +125,9 @@ func getKubeConfigSpecs(cfg *kubeadmapi.InitConfiguration) (map[string]*kubeConf if err != nil { return nil, errors.Wrap(err, "couldn't create a kubeconfig; the CA files couldn't be loaded") } + // Validate period + certsphase.CheckCertificatePeriodValidity(kubeadmconstants.CACertAndKeyBaseName, caCert) + configs, err := getKubeConfigSpecsBase(cfg) if err != nil { return nil, err @@ -265,6 +269,8 @@ func WriteKubeConfigWithClientCert(out io.Writer, cfg *kubeadmapi.InitConfigurat if err != nil { return errors.Wrap(err, "couldn't create a kubeconfig; the CA files couldn't be loaded") } + // Validate period + certsphase.CheckCertificatePeriodValidity(kubeadmconstants.CACertAndKeyBaseName, caCert) controlPlaneEndpoint, err := kubeadmutil.GetControlPlaneEndpoint(cfg.ControlPlaneEndpoint, &cfg.LocalAPIEndpoint) if err != nil { @@ -292,6 +298,8 @@ func WriteKubeConfigWithToken(out io.Writer, cfg *kubeadmapi.InitConfiguration, if err != nil { return errors.Wrap(err, "couldn't create a kubeconfig; the CA files couldn't be loaded") } + // Validate period + certsphase.CheckCertificatePeriodValidity(kubeadmconstants.CACertAndKeyBaseName, caCert) controlPlaneEndpoint, err := kubeadmutil.GetControlPlaneEndpoint(cfg.ControlPlaneEndpoint, &cfg.LocalAPIEndpoint) if err != nil { @@ -344,6 +352,8 @@ func ValidateKubeconfigsForExternalCA(outDir string, cfg *kubeadmapi.InitConfigu if err != nil { return errors.Wrapf(err, "the CA file couldn't be loaded") } + // Validate period + certsphase.CheckCertificatePeriodValidity(kubeadmconstants.CACertAndKeyBaseName, caCert) controlPlaneEndpoint, err := kubeadmutil.GetControlPlaneEndpoint(cfg.ControlPlaneEndpoint, &cfg.LocalAPIEndpoint) if err != nil { diff --git a/cmd/kubeadm/app/util/pkiutil/pki_helpers.go b/cmd/kubeadm/app/util/pkiutil/pki_helpers.go index f5e78ded7c5..aa8a76da131 100644 --- a/cmd/kubeadm/app/util/pkiutil/pki_helpers.go +++ b/cmd/kubeadm/app/util/pkiutil/pki_helpers.go @@ -240,7 +240,7 @@ func TryLoadCertAndKeyFromDisk(pkiPath, name string) (*x509.Certificate, crypto. return cert, key, nil } -// TryLoadCertFromDisk tries to load the cert from the disk and validates that it is valid +// TryLoadCertFromDisk tries to load the cert from the disk func TryLoadCertFromDisk(pkiPath, name string) (*x509.Certificate, error) { certificatePath := pathForCert(pkiPath, name) @@ -253,15 +253,6 @@ func TryLoadCertFromDisk(pkiPath, name string) (*x509.Certificate, error) { // TODO: Support multiple certs here in order to be able to rotate certs cert := certs[0] - // Check so that the certificate is valid now - now := time.Now() - if now.Before(cert.NotBefore) { - return nil, errors.New("the certificate is not valid yet") - } - if now.After(cert.NotAfter) { - return nil, errors.New("the certificate has expired") - } - return cert, nil } @@ -619,3 +610,17 @@ func RemoveDuplicateAltNames(altNames *certutil.AltNames) { } altNames.IPs = ips } + +// ValidateCertPeriod checks if the certificate is valid relative to the current time +// (+/- offset) +func ValidateCertPeriod(cert *x509.Certificate, offset time.Duration) error { + period := fmt.Sprintf("NotBefore: %v, NotAfter: %v", cert.NotBefore, cert.NotAfter) + now := time.Now().Add(offset) + if now.Before(cert.NotBefore) { + return errors.Errorf("the certificate is not valid yet: %s", period) + } + if now.After(cert.NotAfter) { + return errors.Errorf("the certificate has expired: %s", period) + } + return nil +}