Merge pull request #39022 from sttts/sttts-cert-as-ca-only-with-IsCA

Automatic merge from submit-queue

genericapiserver: extract CA cert from server cert and SNI cert chains

Without this PR a matching server cert or SNI cert is directly used as CA cert in the loopback client config. This fails if the cert is no CA cert.

With this PR the loopback client setup code walks through the chains of the server cert and the SNI certs to find a `CA:TRUE` cert. This is then used as the CA in the loopback client config.

pkg/genericapiserver/BUILD

@@ -106,6 +106,7 @@ go_test(
         "//pkg/apis/meta/v1:go_default_library",
         "//pkg/auth/authorizer:go_default_library",
         "//pkg/auth/user:go_default_library",
+        "//pkg/client/clientset_generated/clientset:go_default_library",
         "//pkg/generated/openapi:go_default_library",
         "//pkg/genericapiserver/api/request:go_default_library",
         "//pkg/genericapiserver/options:go_default_library",

pkg/genericapiserver/config_selfclient.go

@@ -75,24 +75,46 @@ func (s *SecureServingInfo) NewSelfClientConfig(token string) (*restclient.Confi
 		BearerToken: token,
 	}
 
-	// find certificate for host: either explicitly given, from the server cert bundle or one of the SNI certs
+	// find certificate for host: either explicitly given, from the server cert bundle or one of the SNI certs,
+	// but only return CA:TRUE certificates.
 	var derCA []byte
 	if s.CACert != nil {
 		derCA = s.CACert.Certificate[0]
 	}
+	if derCA == nil && net.ParseIP(host) == nil {
+		if cert, found := s.SNICerts[host]; found {
+			chain, err := parseChain(cert.Certificate)
+			if err != nil {
+				return nil, fmt.Errorf("failed to parse SNI certificate for host %q: %v", host, err)
+			}
+
+			if trustedChain(chain) {
+				return clientConfig, nil
+			}
+
+			ca, err := findCA(chain)
+			if err != nil {
+				return nil, fmt.Errorf("no CA certificate found in SNI server certificate bundle for host %q: %v", host, err)
+			}
+			derCA = ca.Raw
+		}
+	}
 	if derCA == nil && s.Cert != nil {
-		x509Cert, err := x509.ParseCertificate(s.Cert.Certificate[0])
+		chain, err := parseChain(s.Cert.Certificate)
 		if err != nil {
 			return nil, fmt.Errorf("failed to parse server certificate: %v", err)
 		}
 
-		if (net.ParseIP(host) != nil && certMatchesIP(x509Cert, host)) || certMatchesName(x509Cert, host) {
+		if (net.ParseIP(host) != nil && certMatchesIP(chain[0], host)) || certMatchesName(chain[0], host) {
-			derCA = s.Cert.Certificate[0]
+			if trustedChain(chain) {
+				return clientConfig, nil
 			}
+
+			ca, err := findCA(chain)
+			if err != nil {
+				return nil, fmt.Errorf("no CA certificate found in server certificate bundle: %v", err)
 			}
-	if derCA == nil && net.ParseIP(host) == nil {
+			derCA = ca.Raw
-		if cert, found := s.SNICerts[host]; found {
-			derCA = cert.Certificate[0]
 		}
 	}
 	if derCA == nil {
@@ -107,6 +129,41 @@ func (s *SecureServingInfo) NewSelfClientConfig(token string) (*restclient.Confi
 	return clientConfig, nil
 }
 
+func trustedChain(chain []*x509.Certificate) bool {
+	intermediates := x509.NewCertPool()
+	for _, cert := range chain[1:] {
+		intermediates.AddCert(cert)
+	}
+	_, err := chain[0].Verify(x509.VerifyOptions{
+		Intermediates: intermediates,
+		KeyUsages:     []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
+	})
+	return err == nil
+}
+
+func parseChain(bss [][]byte) ([]*x509.Certificate, error) {
+	var result []*x509.Certificate
+	for _, bs := range bss {
+		x509Cert, err := x509.ParseCertificate(bs)
+		if err != nil {
+			return nil, err
+		}
+		result = append(result, x509Cert)
+	}
+
+	return result, nil
+}
+
+func findCA(chain []*x509.Certificate) (*x509.Certificate, error) {
+	for _, cert := range chain {
+		if cert.IsCA {
+			return cert, nil
+		}
+	}
+
+	return nil, fmt.Errorf("no certificate with CA:TRUE found in chain")
+}
+
 func (s *ServingInfo) NewSelfClientConfig(token string) (*restclient.Config, error) {
 	if s == nil {
 		return nil, nil

pkg/genericapiserver/serve_test.go

@@ -25,13 +25,17 @@ import (
 	"io/ioutil"
 	"net"
 	"os"
+	"reflect"
+	"strconv"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 
+	"k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
 	"k8s.io/kubernetes/pkg/genericapiserver/options"
 	utilcert "k8s.io/kubernetes/pkg/util/cert"
 	"k8s.io/kubernetes/pkg/util/config"
+	"k8s.io/kubernetes/pkg/version"
 )
 
 type TestCertSpec struct {
@@ -229,23 +233,26 @@ NextTest:
 }
 
 func TestServerRunWithSNI(t *testing.T) {
-	tests := []struct {
+	tests := map[string]struct {
 		Cert              TestCertSpec
 		SNICerts          []NamedTestCertSpec
 		ExpectedCertIndex int
 
 		// passed in the client hello info, "localhost" if unset
 		ServerName string
+
+		// optional ip or hostname to pass to NewSelfClientConfig
+		SelfClientBindAddressOverride string
+		ExpectSelfClientError         bool
 	}{
-		{
+		"only one cert": {
-			// only one cert
 			Cert: TestCertSpec{
 				host: "localhost",
+				ips:  []string{"127.0.0.1"},
 			},
 			ExpectedCertIndex: -1,
 		},
-		{
+		"cert with multiple alternate names": {
-			// cert with multiple alternate names
 			Cert: TestCertSpec{
 				host:  "localhost",
 				names: []string{"test.com"},
@@ -254,10 +261,10 @@ func TestServerRunWithSNI(t *testing.T) {
 			ExpectedCertIndex: -1,
 			ServerName:        "test.com",
 		},
-		{
+		"one SNI and the default cert with the same name": {
-			// one SNI and the default cert with the same name
 			Cert: TestCertSpec{
 				host: "localhost",
+				ips:  []string{"127.0.0.1"},
 			},
 			SNICerts: []NamedTestCertSpec{
 				{
@@ -268,10 +275,10 @@ func TestServerRunWithSNI(t *testing.T) {
 			},
 			ExpectedCertIndex: 0,
 		},
-		{
+		"matching SNI cert": {
-			// matching SNI cert
 			Cert: TestCertSpec{
 				host: "localhost",
+				ips:  []string{"127.0.0.1"},
 			},
 			SNICerts: []NamedTestCertSpec{
 				{
@@ -283,13 +290,12 @@ func TestServerRunWithSNI(t *testing.T) {
 			ExpectedCertIndex: 0,
 			ServerName:        "test.com",
 		},
-		{
+		"matching IP in SNI cert and the server cert": {
-			// matching IP in SNI cert and the server cert. But IPs must not be
+			// IPs must not be passed via SNI. Hence, the ServerName in the
-			// passed via SNI. Hence, the ServerName in the HELLO packet is empty
+			// HELLO packet is empty and the server should select the non-SNI cert.
-			// and the server should select the non-SNI cert.
 			Cert: TestCertSpec{
 				host: "localhost",
-				ips:  []string{"10.0.0.1"},
+				ips:  []string{"10.0.0.1", "127.0.0.1"},
 			},
 			SNICerts: []NamedTestCertSpec{
 				{
@@ -302,10 +308,10 @@ func TestServerRunWithSNI(t *testing.T) {
 			ExpectedCertIndex: -1,
 			ServerName:        "10.0.0.1",
 		},
-		{
+		"wildcards": {
-			// wildcards
 			Cert: TestCertSpec{
 				host: "localhost",
+				ips:  []string{"127.0.0.1"},
 			},
 			SNICerts: []NamedTestCertSpec{
 				{
@@ -318,6 +324,79 @@ func TestServerRunWithSNI(t *testing.T) {
 			ExpectedCertIndex: 0,
 			ServerName:        "www.test.com",
 		},
+
+		"loopback: IP for loopback client on SNI cert": {
+			Cert: TestCertSpec{
+				host: "localhost",
+			},
+			SNICerts: []NamedTestCertSpec{
+				{
+					TestCertSpec: TestCertSpec{
+						host: "test.com",
+						ips:  []string{"127.0.0.1"},
+					},
+				},
+			},
+			ExpectedCertIndex:     -1,
+			ExpectSelfClientError: true,
+		},
+		"loopback: IP for loopback client on server and SNI cert": {
+			Cert: TestCertSpec{
+				ips:  []string{"127.0.0.1"},
+				host: "localhost",
+			},
+			SNICerts: []NamedTestCertSpec{
+				{
+					TestCertSpec: TestCertSpec{
+						host: "test.com",
+						ips:  []string{"127.0.0.1"},
+					},
+				},
+			},
+			ExpectedCertIndex: -1,
+		},
+		"loopback: bind to 0.0.0.0 => loopback uses localhost; localhost on server cert": {
+			Cert: TestCertSpec{
+				host: "localhost",
+			},
+			SNICerts: []NamedTestCertSpec{
+				{
+					TestCertSpec: TestCertSpec{
+						host: "test.com",
+					},
+				},
+			},
+			ExpectedCertIndex:             -1,
+			SelfClientBindAddressOverride: "0.0.0.0",
+		},
+		"loopback: bind to 0.0.0.0 => loopback uses localhost; localhost on SNI cert": {
+			Cert: TestCertSpec{
+				host: "test.com",
+			},
+			SNICerts: []NamedTestCertSpec{
+				{
+					TestCertSpec: TestCertSpec{
+						host: "localhost",
+					},
+				},
+			},
+			ExpectedCertIndex:             0,
+			SelfClientBindAddressOverride: "0.0.0.0",
+		},
+		"loopback: bind to 0.0.0.0 => loopback uses localhost; localhost on server and SNI cert": {
+			Cert: TestCertSpec{
+				host: "localhost",
+			},
+			SNICerts: []NamedTestCertSpec{
+				{
+					TestCertSpec: TestCertSpec{
+						host: "localhost",
+					},
+				},
+			},
+			ExpectedCertIndex:             0,
+			SelfClientBindAddressOverride: "0.0.0.0",
+		},
 	}
 
 	tempDir, err := ioutil.TempDir("", "")
@@ -327,16 +406,16 @@ func TestServerRunWithSNI(t *testing.T) {
 	defer os.RemoveAll(tempDir)
 
 NextTest:
-	for i, test := range tests {
+	for title, test := range tests {
 		// create server cert
 		serverCertBundleFile, serverKeyFile, err := createTestCertFiles(tempDir, test.Cert)
 		if err != nil {
-			t.Errorf("%d - failed to create server cert: %v", i, err)
+			t.Errorf("%q - failed to create server cert: %v", title, err)
 			continue NextTest
 		}
 		ca, err := caCertFromBundle(serverCertBundleFile)
 		if err != nil {
-			t.Errorf("%d - failed to extract ca cert from server cert bundle: %v", i, err)
+			t.Errorf("%q - failed to extract ca cert from server cert bundle: %v", title, err)
 			continue NextTest
 		}
 		caCerts := []*x509.Certificate{ca}
@@ -345,7 +424,7 @@ NextTest:
 		var namedCertKeys []config.NamedCertKey
 		serverSig, err := certFileSignature(serverCertBundleFile, serverKeyFile)
 		if err != nil {
-			t.Errorf("%d - failed to get server cert signature: %v", i, err)
+			t.Errorf("%q - failed to get server cert signature: %v", title, err)
 			continue NextTest
 		}
 		signatures := map[string]int{
@@ -354,7 +433,7 @@ NextTest:
 		for j, c := range test.SNICerts {
 			certBundleFile, keyFile, err := createTestCertFiles(tempDir, c.TestCertSpec)
 			if err != nil {
-				t.Errorf("%d - failed to create SNI cert %d: %v", i, j, err)
+				t.Errorf("%q - failed to create SNI cert %d: %v", title, j, err)
 				continue NextTest
 			}
 
@@ -366,7 +445,7 @@ NextTest:
 
 			ca, err := caCertFromBundle(certBundleFile)
 			if err != nil {
-				t.Errorf("%d - failed to extract ca cert from SNI cert %d: %v", i, j, err)
+				t.Errorf("%q - failed to extract ca cert from SNI cert %d: %v", title, j, err)
 				continue NextTest
 			}
 			caCerts = append(caCerts, ca)
@@ -374,7 +453,7 @@ NextTest:
 			// store index in namedCertKeys with the signature as the key
 			sig, err := certFileSignature(certBundleFile, keyFile)
 			if err != nil {
-				t.Errorf("%d - failed get SNI cert %d signature: %v", i, j, err)
+				t.Errorf("%q - failed get SNI cert %d signature: %v", title, j, err)
 				continue NextTest
 			}
 			signatures[sig] = j
@@ -386,6 +465,9 @@ NextTest:
 		etcdserver, config, _ := setUp(t)
 		defer etcdserver.Terminate(t)
 
+		v := version.Get()
+		config.Version = &v
+
 		config.EnableIndex = true
 		_, err = config.ApplySecureServingOptions(&options.SecureServingOptions{
 			ServingOptions: options.ServingOptions{
@@ -401,14 +483,14 @@ NextTest:
 			SNICertKeys: namedCertKeys,
 		})
 		if err != nil {
-			t.Errorf("%d - failed applying the SecureServingOptions: %v", i, err)
+			t.Errorf("%q - failed applying the SecureServingOptions: %v", title, err)
 			continue NextTest
 		}
 		config.InsecureServingInfo = nil
 
 		s, err := config.Complete().New()
 		if err != nil {
-			t.Errorf("%d - failed creating the server: %v", i, err)
+			t.Errorf("%q - failed creating the server: %v", title, err)
 			continue NextTest
 		}
 
@@ -416,7 +498,7 @@ NextTest:
 		s.SecureServingInfo.BindAddress = "127.0.0.1:0"
 
 		if err := s.serveSecurely(stopCh); err != nil {
-			t.Errorf("%d - failed running the server: %v", i, err)
+			t.Errorf("%q - failed running the server: %v", title, err)
 			continue NextTest
 		}
 
@@ -434,7 +516,7 @@ NextTest:
 			ServerName: test.ServerName, // used for SNI in the client HELLO packet
 		})
 		if err != nil {
-			t.Errorf("%d - failed to connect: %v", i, err)
+			t.Errorf("%q - failed to connect: %v", title, err)
 			continue NextTest
 		}
 
@@ -442,13 +524,44 @@ NextTest:
 		sig := x509CertSignature(conn.ConnectionState().PeerCertificates[0])
 		gotCertIndex, found := signatures[sig]
 		if !found {
-			t.Errorf("%d - unknown signature returned from server: %s", i, sig)
+			t.Errorf("%q - unknown signature returned from server: %s", title, sig)
 		}
 		if gotCertIndex != test.ExpectedCertIndex {
-			t.Errorf("%d - expected cert index %d, got cert index %d", i, test.ExpectedCertIndex, gotCertIndex)
+			t.Errorf("%q - expected cert index %d, got cert index %d", title, test.ExpectedCertIndex, gotCertIndex)
 		}
 
 		conn.Close()
+
+		// check that the loopback client can connect
+		host := "127.0.0.1"
+		if len(test.SelfClientBindAddressOverride) != 0 {
+			host = test.SelfClientBindAddressOverride
+		}
+		config.SecureServingInfo.ServingInfo.BindAddress = net.JoinHostPort(host, strconv.Itoa(s.effectiveSecurePort))
+		cfg, err := config.SecureServingInfo.NewSelfClientConfig("some-token")
+		if test.ExpectSelfClientError {
+			if err == nil {
+				t.Errorf("%q - expected error creating loopback client config", title)
+			}
+			continue NextTest
+		}
+		if err != nil {
+			t.Errorf("%q - failed creating loopback client config: %v", title, err)
+			continue NextTest
+		}
+		client, err := clientset.NewForConfig(cfg)
+		if err != nil {
+			t.Errorf("%q - failed to create loopback client: %v", title, err)
+			continue NextTest
+		}
+		got, err := client.ServerVersion()
+		if err != nil {
+			t.Errorf("%q - failed to connect with loopback client: %v", title, err)
+			continue NextTest
+		}
+		if expected := &v; !reflect.DeepEqual(got, expected) {
+			t.Errorf("%q - loopback client didn't get correct version info: expected=%v got=%v", title, expected, got)
+		}
 	}
 }