From 440ea3ef49e0ac77353ceeaebc2aad6c995d5b35 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Mon, 3 May 2021 10:11:54 -0400 Subject: [PATCH] client-go transport: assert that final CA data is valid Signed-off-by: Monis Khan --- hack/.staticcheck_failures | 1 - .../apiserver/pkg/util/webhook/certs_test.go | 28 ------ .../pkg/util/webhook/webhook_test.go | 98 ++++++++++++------- .../k8s.io/client-go/transport/transport.go | 38 ++++++- .../client-go/transport/transport_test.go | 8 ++ .../apiserver/podlogs/podlogs_test.go | 41 ++++---- 6 files changed, 125 insertions(+), 89 deletions(-) diff --git a/hack/.staticcheck_failures b/hack/.staticcheck_failures index 4921e143c71..b49fe0d0cc1 100644 --- a/hack/.staticcheck_failures +++ b/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 diff --git a/staging/src/k8s.io/apiserver/pkg/util/webhook/certs_test.go b/staging/src/k8s.io/apiserver/pkg/util/webhook/certs_test.go index 989c43bba60..8b6d0ad8724 100644 --- a/staging/src/k8s.io/apiserver/pkg/util/webhook/certs_test.go +++ b/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 diff --git a/staging/src/k8s.io/apiserver/pkg/util/webhook/webhook_test.go b/staging/src/k8s.io/apiserver/pkg/util/webhook/webhook_test.go index a61c40bab8f..d2019849eea 100644 --- a/staging/src/k8s.io/apiserver/pkg/util/webhook/webhook_test.go +++ b/staging/src/k8s.io/apiserver/pkg/util/webhook/webhook_test.go @@ -163,15 +163,41 @@ func TestKubeConfigFile(t *testing.T) { errRegex: fmt.Sprintf(errMissingCertPath, "certificate-authority", badCAPath, "", badCAPath), }, { - test: "cluster with invalid CA certificate ", + test: "cluster with invalid CA certificate", 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,46 +268,48 @@ func TestKubeConfigFile(t *testing.T) { } for _, tt := range tests { - // Use a closure so defer statements trigger between loop iterations. - err := func() error { - kubeConfig := v1.Config{} + t.Run(tt.test, func(t *testing.T) { + // Use a closure so defer statements trigger between loop iterations. + err := func() error { + kubeConfig := v1.Config{} - if tt.cluster != nil { - kubeConfig.Clusters = []v1.NamedCluster{*tt.cluster} - } + if tt.cluster != nil { + kubeConfig.Clusters = []v1.NamedCluster{*tt.cluster} + } - if tt.context != nil { - kubeConfig.Contexts = []v1.NamedContext{*tt.context} - } + if tt.context != nil { + kubeConfig.Contexts = []v1.NamedContext{*tt.context} + } - if tt.user != nil { - kubeConfig.AuthInfos = []v1.NamedAuthInfo{*tt.user} - } + if tt.user != nil { + kubeConfig.AuthInfos = []v1.NamedAuthInfo{*tt.user} + } - kubeConfig.CurrentContext = tt.currentContext + kubeConfig.CurrentContext = tt.currentContext - kubeConfigFile, err := newKubeConfigFile(kubeConfig) + kubeConfigFile, err := newKubeConfigFile(kubeConfig) + + if err == nil { + defer os.Remove(kubeConfigFile) + + _, err = NewGenericWebhook(runtime.NewScheme(), scheme.Codecs, kubeConfigFile, groupVersions, retryBackoff, nil) + } + + return err + }() if err == nil { - defer os.Remove(kubeConfigFile) - - _, err = NewGenericWebhook(runtime.NewScheme(), scheme.Codecs, kubeConfigFile, groupVersions, retryBackoff, nil) + if tt.errRegex != "" { + t.Errorf("%s: expected an error", tt.test) + } + } else { + if tt.errRegex == "" { + t.Errorf("%s: unexpected error: %v", tt.test, err) + } else if !regexp.MustCompile(tt.errRegex).MatchString(err.Error()) { + t.Errorf("%s: unexpected error message to match:\n Expected: %s\n Actual: %s", tt.test, tt.errRegex, err.Error()) + } } - - return err - }() - - if err == nil { - if tt.errRegex != "" { - t.Errorf("%s: expected an error", tt.test) - } - } else { - if tt.errRegex == "" { - t.Errorf("%s: unexpected error: %v", tt.test, err) - } else if !regexp.MustCompile(tt.errRegex).MatchString(err.Error()) { - t.Errorf("%s: unexpected error message to match:\n Expected: %s\n Actual: %s", tt.test, tt.errRegex, err.Error()) - } - } + }) } } diff --git a/staging/src/k8s.io/client-go/transport/transport.go b/staging/src/k8s.io/client-go/transport/transport.go index 083364fd6de..b4a7bfa67cd 100644 --- a/staging/src/k8s.io/client-go/transport/transport.go +++ b/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) - return certPool + if ok := certPool.AppendCertsFromPEM(caData); !ok { + 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 diff --git a/staging/src/k8s.io/client-go/transport/transport_test.go b/staging/src/k8s.io/client-go/transport/transport_test.go index d8e75443210..c439c96f81d 100644 --- a/staging/src/k8s.io/client-go/transport/transport_test.go +++ b/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{ diff --git a/test/integration/apiserver/podlogs/podlogs_test.go b/test/integration/apiserver/podlogs/podlogs_test.go index 46a072ca337..d423f014b0e 100644 --- a/test/integration/apiserver/podlogs/podlogs_test.go +++ b/test/integration/apiserver/podlogs/podlogs_test.go @@ -39,26 +39,27 @@ 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----- - MIIDMDCCAhigAwIBAgIIHNPD7sig7YIwDQYJKoZIhvcNAQELBQAwNjESMBAGA1UE - CxMJb3BlbnNoaWZ0MSAwHgYDVQQDExdhZG1pbi1rdWJlY29uZmlnLXNpZ25lcjAe - Fw0xOTA1MzAxNTA3MzlaFw0yOTA1MjcxNTA3MzlaMDYxEjAQBgNVBAsTCW9wZW5z - aGlmdDEgMB4GA1UEAxMXYWRtaW4ta3ViZWNvbmZpZy1zaWduZXIwggEiMA0GCSqG - SIb3DQEBAQUAA4IBDwAwggEKAoIBAQD0dHk23lHRcuq06FzYDOl9J9+s8pnGxqA3 - IPcARI6ag/98aYe3ENwAB5e1i7AU2F2WiDZgj444w374XLdVgIK8zgQEm9yoqrlc - +/ayO7ceKklrKHOMwh63LvGLEOqzhol2nFmBhXAZt+HyIoZHXN0IqlA92196+Dml - 0WOn1F4ce6JbAtEceFHPgLeI7KFmVaPz2796pBXh23ii6r7WvV1Rn9MKlMSBJQR4 - 0LZzu9/j+GdnFXewdLAAMfgPzwEqv6h3PzvtUCjgdraHEm8Rs7s15S3PUmLK4RQS - PsThx5BhJEGd/W6EzQ3BKoQfochhu3mnAQtW1J07CullySQ5Gg9fAgMBAAGjQjBA - MA4GA1UdDwEB/wQEAwICpDAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBQkTaaw - YJSZ5k2Wd+OsM4GFMTGdqzANBgkqhkiG9w0BAQsFAAOCAQEAHK7+zBZPLqK+f9DT - UEnpwRmZ0aeGS4YgbGIkqpjxJymVOwkRd5A1wslvVfGZ6yOQthF6KlCmqnPyJJMR - I7FHw8j0h2ci90fEQ6IS90Y/ZJXkcgiK9Ncwa35GFGs8QrBxN4leGhtm84BnnBHN - cTWpa4zcBwru0CRG7iHc66VX16X8jHB1iFeZ5W/FgY4MsE+G1Vze4mCXSPVI4BZ2 - /qlAgogjBivvSwQ9SFuCszg7IPjvT2ksm+Cf+8eT4YBqW41F85vBGR+FYK14yIla - Bgqc+dJN9xS9Ah5gLiGQJ6C4niUA11piCpvMsy+j/LQ1Erx47KMar5fuMXYk7iPq - 1vqIwg== - -----END CERTIFICATE----- + opts.KubeletConfig.CAData = []byte(` +-----BEGIN CERTIFICATE----- +MIIDMDCCAhigAwIBAgIIHNPD7sig7YIwDQYJKoZIhvcNAQELBQAwNjESMBAGA1UE +CxMJb3BlbnNoaWZ0MSAwHgYDVQQDExdhZG1pbi1rdWJlY29uZmlnLXNpZ25lcjAe +Fw0xOTA1MzAxNTA3MzlaFw0yOTA1MjcxNTA3MzlaMDYxEjAQBgNVBAsTCW9wZW5z +aGlmdDEgMB4GA1UEAxMXYWRtaW4ta3ViZWNvbmZpZy1zaWduZXIwggEiMA0GCSqG +SIb3DQEBAQUAA4IBDwAwggEKAoIBAQD0dHk23lHRcuq06FzYDOl9J9+s8pnGxqA3 +IPcARI6ag/98aYe3ENwAB5e1i7AU2F2WiDZgj444w374XLdVgIK8zgQEm9yoqrlc ++/ayO7ceKklrKHOMwh63LvGLEOqzhol2nFmBhXAZt+HyIoZHXN0IqlA92196+Dml +0WOn1F4ce6JbAtEceFHPgLeI7KFmVaPz2796pBXh23ii6r7WvV1Rn9MKlMSBJQR4 +0LZzu9/j+GdnFXewdLAAMfgPzwEqv6h3PzvtUCjgdraHEm8Rs7s15S3PUmLK4RQS +PsThx5BhJEGd/W6EzQ3BKoQfochhu3mnAQtW1J07CullySQ5Gg9fAgMBAAGjQjBA +MA4GA1UdDwEB/wQEAwICpDAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBQkTaaw +YJSZ5k2Wd+OsM4GFMTGdqzANBgkqhkiG9w0BAQsFAAOCAQEAHK7+zBZPLqK+f9DT +UEnpwRmZ0aeGS4YgbGIkqpjxJymVOwkRd5A1wslvVfGZ6yOQthF6KlCmqnPyJJMR +I7FHw8j0h2ci90fEQ6IS90Y/ZJXkcgiK9Ncwa35GFGs8QrBxN4leGhtm84BnnBHN +cTWpa4zcBwru0CRG7iHc66VX16X8jHB1iFeZ5W/FgY4MsE+G1Vze4mCXSPVI4BZ2 +/qlAgogjBivvSwQ9SFuCszg7IPjvT2ksm+Cf+8eT4YBqW41F85vBGR+FYK14yIla +Bgqc+dJN9xS9Ah5gLiGQJ6C4niUA11piCpvMsy+j/LQ1Erx47KMar5fuMXYk7iPq +1vqIwg== +-----END CERTIFICATE----- `) }, })