From 7f8fc5d18931e36cb1d3dbaf479bb1d02bb01d0b Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Thu, 11 Apr 2019 16:32:50 +0300 Subject: [PATCH] kubeadm: check all available CA certs against pinned certs Currently kubeadm produces an error upon parsing multiple certificates stored in the cluster-info configmap. Yet it should check all available certificates in a scenario like CA key rotation. Check all available CA certs against pinned certificate hashes. Fixes https://github.com/kubernetes/kubeadm/issues/1399 --- cmd/kubeadm/app/discovery/token/token.go | 35 +++++++++++++------ cmd/kubeadm/app/discovery/token/token_test.go | 11 +++--- cmd/kubeadm/app/util/pubkeypin/pubkeypin.go | 16 ++++++--- .../app/util/pubkeypin/pubkeypin_test.go | 6 ++-- 4 files changed, 44 insertions(+), 24 deletions(-) diff --git a/cmd/kubeadm/app/discovery/token/token.go b/cmd/kubeadm/app/discovery/token/token.go index bd6d7237d5..89ba556948 100644 --- a/cmd/kubeadm/app/discovery/token/token.go +++ b/cmd/kubeadm/app/discovery/token/token.go @@ -119,14 +119,14 @@ func RetrieveValidatedConfigInfo(cfg *kubeadmapi.JoinConfiguration) (*clientcmda for _, cluster := range insecureConfig.Clusters { clusterCABytes = cluster.CertificateAuthorityData } - clusterCA, err := parsePEMCert(clusterCABytes) + clusterCAs, err := parsePEMCerts(clusterCABytes) if err != nil { return nil, errors.Wrapf(err, "failed to parse cluster CA from the %s configmap", bootstrapapi.ConfigMapClusterInfo) } // Validate the cluster CA public key against the pinned set - err = pubKeyPins.Check(clusterCA) + err = pubKeyPins.CheckAny(clusterCAs) if err != nil { return nil, errors.Wrapf(err, "cluster CA found in %s configmap is invalid", bootstrapapi.ConfigMapClusterInfo) } @@ -226,14 +226,27 @@ func fetchKubeConfigWithTimeout(apiEndpoint string, discoveryTimeout time.Durati } } -// parsePEMCert decodes a PEM-formatted certificate and returns it as an x509.Certificate -func parsePEMCert(certData []byte) (*x509.Certificate, error) { - pemBlock, trailingData := pem.Decode(certData) - if pemBlock == nil { - return nil, errors.New("invalid PEM data") +// parsePEMCerts decodes PEM-formatted certificates into a slice of x509.Certificates +func parsePEMCerts(certData []byte) ([]*x509.Certificate, error) { + var certificates []*x509.Certificate + var pemBlock *pem.Block + + for { + pemBlock, certData = pem.Decode(certData) + if pemBlock == nil { + return nil, errors.New("invalid PEM data") + } + + cert, err := x509.ParseCertificate(pemBlock.Bytes) + if err != nil { + return nil, errors.Wrap(err, "unable to parse certificate") + } + certificates = append(certificates, cert) + + if len(certData) == 0 { + break + } } - if len(trailingData) != 0 { - return nil, errors.New("trailing data after first PEM block") - } - return x509.ParseCertificate(pemBlock.Bytes) + + return certificates, nil } diff --git a/cmd/kubeadm/app/discovery/token/token_test.go b/cmd/kubeadm/app/discovery/token/token_test.go index 8be1147e0e..9572feabc4 100644 --- a/cmd/kubeadm/app/discovery/token/token_test.go +++ b/cmd/kubeadm/app/discovery/token/token_test.go @@ -102,23 +102,24 @@ func TestParsePEMCert(t *testing.T) { }{ {"invalid certificate data", []byte{0}, false}, {"certificate with junk appended", []byte(testCertPEM + "\nABC"), false}, - {"multiple certificates", []byte(testCertPEM + "\n" + testCertPEM), false}, + {"multiple certificates", []byte(testCertPEM + "\n" + testCertPEM), true}, {"valid", []byte(testCertPEM), true}, + {"empty input", []byte{}, false}, } { - cert, err := parsePEMCert(testCase.input) + certs, err := parsePEMCerts(testCase.input) if testCase.expectValid { if err != nil { t.Errorf("failed TestParsePEMCert(%s): unexpected error %v", testCase.name, err) } - if cert == nil { + if certs == nil { t.Errorf("failed TestParsePEMCert(%s): returned nil", testCase.name) } } else { if err == nil { t.Errorf("failed TestParsePEMCert(%s): expected an error", testCase.name) } - if cert != nil { - t.Errorf("failed TestParsePEMCert(%s): expected not to get a certificate back, but got one", testCase.name) + if certs != nil { + t.Errorf("failed TestParsePEMCert(%s): expected not to get a certificate back, but got some", testCase.name) } } } diff --git a/cmd/kubeadm/app/util/pubkeypin/pubkeypin.go b/cmd/kubeadm/app/util/pubkeypin/pubkeypin.go index 16f74dee32..fb157160d6 100644 --- a/cmd/kubeadm/app/util/pubkeypin/pubkeypin.go +++ b/cmd/kubeadm/app/util/pubkeypin/pubkeypin.go @@ -61,12 +61,18 @@ func (s *Set) Allow(pubKeyHashes ...string) error { return nil } -// Check if a certificate matches one of the public keys in the set -func (s *Set) Check(certificate *x509.Certificate) error { - if s.checkSHA256(certificate) { - return nil +// CheckAny checks if at least one certificate matches one of the public keys in the set +func (s *Set) CheckAny(certificates []*x509.Certificate) error { + var hashes []string + + for _, certificate := range certificates { + if s.checkSHA256(certificate) { + return nil + } + + hashes = append(hashes, Hash(certificate)) } - return errors.Errorf("public key %s not pinned", Hash(certificate)) + return errors.Errorf("none of the public keys %q are pinned", strings.Join(hashes, ":")) } // Empty returns true if the Set contains no pinned public keys. diff --git a/cmd/kubeadm/app/util/pubkeypin/pubkeypin_test.go b/cmd/kubeadm/app/util/pubkeypin/pubkeypin_test.go index 4e578a4bdb..a5a0d57a45 100644 --- a/cmd/kubeadm/app/util/pubkeypin/pubkeypin_test.go +++ b/cmd/kubeadm/app/util/pubkeypin/pubkeypin_test.go @@ -121,7 +121,7 @@ func TestSet(t *testing.T) { return } - err = s.Check(testCert(t, testCertPEM)) + err = s.CheckAny([]*x509.Certificate{testCert(t, testCertPEM)}) if err == nil { t.Error("expected test cert to not be allowed (yet)") return @@ -133,13 +133,13 @@ func TestSet(t *testing.T) { return } - err = s.Check(testCert(t, testCertPEM)) + err = s.CheckAny([]*x509.Certificate{testCert(t, testCertPEM)}) if err != nil { t.Errorf("expected test cert to be allowed, but got back: %v", err) return } - err = s.Check(testCert(t, testCert2PEM)) + err = s.CheckAny([]*x509.Certificate{testCert(t, testCert2PEM)}) if err == nil { t.Error("expected the second test cert to be disallowed") return