Merge pull request #101707 from enj/enj/i/bad_cadata

client-go transport: assert that final CA data is valid

hack/.staticcheck_failures

@@ -17,7 +17,6 @@ vendor/k8s.io/apiserver/pkg/storage
 vendor/k8s.io/apiserver/pkg/storage/cacher
 vendor/k8s.io/apiserver/pkg/storage/tests
 vendor/k8s.io/apiserver/pkg/storage/value/encrypt/envelope
-vendor/k8s.io/apiserver/pkg/util/webhook
 vendor/k8s.io/apiserver/pkg/util/wsstream
 vendor/k8s.io/client-go/rest
 vendor/k8s.io/client-go/rest/watch

staging/src/k8s.io/apiserver/pkg/util/webhook/certs_test.go

@@ -67,34 +67,6 @@ ID3CIiCF8hhF5C2hjrW0LTJ6zTlg1c1K0NmmUL1ucsfzEk//c7GsU8dk+FYmtW9A
 VzryJj1NmnSqA3zd3jBMuK37Ei3pRvVbO7Uxf14=
 -----END CERTIFICATE-----`)
 
-var badCAKey = []byte(`-----BEGIN RSA PRIVATE KEY-----
-MIIEowIBAAKCAQEAsmGBQnmBK88DdHezRXed0pX0vtbxMP6WEwaxW1d6x7jOafn2
-HbfxuxC/B1dpewpKswBlQTi4/0LqjUi2/YJIyVJZXGdFRaR5zqYVK4LYNHSBvY8y
-9wyupUOKGiP6fx9bMAXOzafdK0LR7C8P5q9+zxRcjU8G5sK9mnd+oNLKMZJ1gaMU
-3KMYi3gnxi5ebNxxP0LAaPqY+yfpjSm2OZkS/rcMWMXsq42ERPvbclf0rY5o+uau
-XXaNilkjJhUUoEk0bOlqflsh2ol+WVwhS3XIO9MORwspP58drn574JcwhMFlM7aU
-kT4/hVoqnSaOmSdLR3Op3OmQSq+n+J9cbJiVfQIDAQABAoIBAQCZd8n9pwu65R/T
-1CgoXAEsbFdk2QgpXt8+/0MXkuvPaPAtvSBB8T/H8WBosIvPj8s0teJneqWu96NU
-ansFIFH+4xp+pVqz0A37/Ge6R5g7iQEWVV1Dr2WSSclHNC0PsaqCZnzF8uYVkieJ
-S/QiRFqVTq9R4+vMHT+C5cvMEY5jlm5LA35UKOZ0SQRHJ717WJZK+p+XszljaI9g
-9sCZX0uR25nECalhwZVbaK6IB4u1nBM6+67j2sOUV/7udyPbDlAfRE15dQxBR+rW
-Uu30jQz6+GMsPh1bHlfDHy4L/FFPCff/ZI3fbbfuvfUMJHH3G/eJvi+QPtlQN6Xp
-H9xl/NgBAoGBANdU96dAl4N0LlkEagzGXXSQMgxDbuMkOHlDGH1Vm0P+Nq7tZKwF
-EQKvVa6QPaSiAzgeuUwPkiVlIJZ6lMzzutRMKu3fSYnM57X45dnKWfs9AsskDSMp
-Pj2LQ1RBo8Pgd8H/aFDVmCjBdn6w/6xrAPn4TvpYM6mJyQ+KbcMINrXZAoGBANQS
-ADwajz00IXNxE6A86mvQZ68fphNNoMByI6q5bA+T8AorlQ9YaofYfb1ezmHIh6nt
-QPQIHATYF6BmY23ypgHF/aE+lcgK7Nej4u+3Lc9ImJ/0o5wzvxUpX9XFsjOjAGEz
-TentETTbLnp6a3f+bnXAOaM/ofQNPvnqiQnR0OJFAoGAC7Jp4YP4twNQoTVELX15
-BiPvFAt1spD9IFkss2I7FO5yOf5bQZzk16h+lwTu1EqYsiu5FRCjd7SOmJ4AB0IW
-HAInMtS2Qe4HiDMFCVecm7EsvawvqoFLCDzQY3tNUg6XcspU+E8h/NTFgwxKVytY
-2jtKzv6Lj+IUMevrGnUPw8ECgYAGnuE+/x1FreD1d6xDLmOrJgB2qShIJf5Ew8t1
-QwCqo9W0m5O1vO7mes3CIbmTt+z0UyHZ/H7Tb+Oc8FVeU1r3ZzT52bhXXG/0c3tc
-PH3DoOKS69JHyB3JDVeeluNvVUFnx3BBQ1NsMQOMc1Hzlw/fwTaLcCsgMWGr77SD
-h/dbeQKBgD9wanYnE7uQAP9N49FDtCiiMwFyYc8QwULU2jD6umfByjmvo4ocqDE/
-sMwQmu62qrQlhHPMXh28zsobyC3DpkgutlTB9U1xyMoElFGeEBbpd3JhTZFKs7Hd
-RggKDB1828evoGHyfA+Tu1+uh0viEuP0yfIMjRABbJaLjTYAx1YG
------END RSA PRIVATE KEY-----`)
-
 var badCACert = []byte(`-----BEGIN CERTIFICATE-----
 MIIDGTCCAgGgAwIBAgIUINBaI0NGgSo4UqKHuPd/HSBO38EwDQYJKoZIhvcNAQEL
 BQAwGzEZMBcGA1UEAwwQd2ViaG9va190ZXN0c19jYTAgFw0yMDEwMDcxMjMxNDFa

staging/src/k8s.io/apiserver/pkg/util/webhook/webhook_test.go

@@ -167,11 +167,37 @@ func TestKubeConfigFile(t *testing.T) {
 			cluster: &v1.NamedCluster{
 				Cluster: v1.Cluster{
 					Server:                   namedCluster.Cluster.Server,
-					CertificateAuthorityData: caKey,
+					CertificateAuthorityData: caKey, // pretend user put caKey here instead of caCert
 				},
 			},
 			user:     &defaultUser,
-			errRegex: "", // Not an error at parse time, only when using the webhook
+			errRegex: "unable to load root certificates: no valid certificate authority data seen",
+		},
+		{
+			test: "cluster with invalid CA certificate - no PEM",
+			cluster: &v1.NamedCluster{
+				Cluster: v1.Cluster{
+					Server:                   namedCluster.Cluster.Server,
+					CertificateAuthorityData: []byte(`not a cert`),
+				},
+			},
+			user:     &defaultUser,
+			errRegex: "unable to load root certificates: unable to parse bytes as PEM block",
+		},
+		{
+			test: "cluster with invalid CA certificate - parse error",
+			cluster: &v1.NamedCluster{
+				Cluster: v1.Cluster{
+					Server: namedCluster.Cluster.Server,
+					CertificateAuthorityData: []byte(`
+-----BEGIN CERTIFICATE-----
+MIIDGTCCAgGgAwIBAgIUOS2M
+-----END CERTIFICATE-----
+`),
+				},
+			},
+			user:     &defaultUser,
+			errRegex: "unable to load root certificates: failed to parse certificate: asn1: syntax error: data truncated",
 		},
 		{
 			test:    "user with invalid client certificate path",
@@ -242,6 +268,7 @@ func TestKubeConfigFile(t *testing.T) {
 	}
 
 	for _, tt := range tests {
+		t.Run(tt.test, func(t *testing.T) {
 			// Use a closure so defer statements trigger between loop iterations.
 			err := func() error {
 				kubeConfig := v1.Config{}
@@ -282,6 +309,7 @@ func TestKubeConfigFile(t *testing.T) {
 					t.Errorf("%s: unexpected error message to match:\n  Expected: %s\n  Actual:   %s", tt.test, tt.errRegex, err.Error())
 				}
 			}
+		})
 	}
 }
 

staging/src/k8s.io/client-go/transport/transport.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"crypto/tls"
 	"crypto/x509"
+	"encoding/pem"
 	"fmt"
 	"io/ioutil"
 	"net/http"
@@ -79,7 +80,11 @@ func TLSConfigFor(c *Config) (*tls.Config, error) {
 	}
 
 	if c.HasCA() {
-		tlsConfig.RootCAs = rootCertPool(c.TLS.CAData)
+		rootCAs, err := rootCertPool(c.TLS.CAData)
+		if err != nil {
+			return nil, fmt.Errorf("unable to load root certificates: %w", err)
+		}
+		tlsConfig.RootCAs = rootCAs
 	}
 
 	var staticCert *tls.Certificate
@@ -176,18 +181,41 @@ func dataFromSliceOrFile(data []byte, file string) ([]byte, error) {
 
 // rootCertPool returns nil if caData is empty.  When passed along, this will mean "use system CAs".
 // When caData is not empty, it will be the ONLY information used in the CertPool.
-func rootCertPool(caData []byte) *x509.CertPool {
+func rootCertPool(caData []byte) (*x509.CertPool, error) {
 	// What we really want is a copy of x509.systemRootsPool, but that isn't exposed.  It's difficult to build (see the go
 	// code for a look at the platform specific insanity), so we'll use the fact that RootCAs == nil gives us the system values
 	// It doesn't allow trusting either/or, but hopefully that won't be an issue
 	if len(caData) == 0 {
-		return nil
+		return nil, nil
 	}
 
 	// if we have caData, use it
 	certPool := x509.NewCertPool()
-	certPool.AppendCertsFromPEM(caData)
+	if ok := certPool.AppendCertsFromPEM(caData); !ok {
-	return certPool
+		return nil, createErrorParsingCAData(caData)
+	}
+	return certPool, nil
+}
+
+// createErrorParsingCAData ALWAYS returns an error.  We call it because know we failed to AppendCertsFromPEM
+// but we don't know the specific error because that API is just true/false
+func createErrorParsingCAData(pemCerts []byte) error {
+	for len(pemCerts) > 0 {
+		var block *pem.Block
+		block, pemCerts = pem.Decode(pemCerts)
+		if block == nil {
+			return fmt.Errorf("unable to parse bytes as PEM block")
+		}
+
+		if block.Type != "CERTIFICATE" || len(block.Headers) != 0 {
+			continue
+		}
+
+		if _, err := x509.ParseCertificate(block.Bytes); err != nil {
+			return fmt.Errorf("failed to parse certificate: %w", err)
+		}
+	}
+	return fmt.Errorf("no valid certificate authority data seen")
 }
 
 // WrapperFunc wraps an http.RoundTripper when a new transport

staging/src/k8s.io/client-go/transport/transport_test.go

@@ -142,6 +142,14 @@ func TestNew(t *testing.T) {
 				},
 			},
 		},
+		"bad ca data transport": {
+			Err: true,
+			Config: &Config{
+				TLS: TLSConfig{
+					CAData: []byte(rootCACert + "this is not valid"),
+				},
+			},
+		},
 		"ca data overriding bad ca file transport": {
 			TLS: true,
 			Config: &Config{

test/integration/apiserver/podlogs/podlogs_test.go

@@ -39,7 +39,8 @@ func TestInsecurePodLogs(t *testing.T) {
 		ModifyServerRunOptions: func(opts *options.ServerRunOptions) {
 			opts.GenericServerRunOptions.MaxRequestBodyBytes = 1024 * 1024
 			// I have no idea what this cert is, but it doesn't matter, we just want something that always fails validation
-			opts.KubeletConfig.CAData = []byte(`      -----BEGIN CERTIFICATE-----
+			opts.KubeletConfig.CAData = []byte(`
+-----BEGIN CERTIFICATE-----
 MIIDMDCCAhigAwIBAgIIHNPD7sig7YIwDQYJKoZIhvcNAQELBQAwNjESMBAGA1UE
 CxMJb3BlbnNoaWZ0MSAwHgYDVQQDExdhZG1pbi1rdWJlY29uZmlnLXNpZ25lcjAe
 Fw0xOTA1MzAxNTA3MzlaFw0yOTA1MjcxNTA3MzlaMDYxEjAQBgNVBAsTCW9wZW5z