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

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)
+		}
 	}
 }