Skip to content
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.

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

kubeadm: check all available CA certs against pinned certs #76500

Merged
merged 1 commit into from
Apr 23, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 24 additions & 11 deletions cmd/kubeadm/app/discovery/token/token.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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")
}
if len(trailingData) != 0 {
return nil, errors.New("trailing data after first PEM block")
// parsePEMCerts decodes PEM-formatted certificates into a slice of x509.Certificates
func parsePEMCerts(certData []byte) ([]*x509.Certificate, error) {
Copy link
Member

Choose a reason for hiding this comment

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

please modify this function to:

// 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
		}
	}
	return certificates, nil
}

note the a PEM- -> PEM change in the function comment.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! Fixed

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
}
}
return x509.ParseCertificate(pemBlock.Bytes)

return certificates, nil
}
11 changes: 6 additions & 5 deletions cmd/kubeadm/app/discovery/token/token_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}
Expand Down
16 changes: 11 additions & 5 deletions cmd/kubeadm/app/util/pubkeypin/pubkeypin.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions cmd/kubeadm/app/util/pubkeypin/pubkeypin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down