Merge pull request #85308 from deads2k/limited-sni-support

allow an SNI cert to be used to respond for a particular IP

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/BUILD

@@ -57,10 +57,12 @@ go_test(
         "cert_key_test.go",
         "client_ca_test.go",
         "named_certificates_test.go",
+        "server_test.go",
         "tlsconfig_test.go",
     ],
     embed = [":go_default_library"],
     deps = [
+        "//staging/src/k8s.io/client-go/util/cert:go_default_library",
         "//vendor/github.com/davecgh/go-spew/spew:go_default_library",
         "//vendor/github.com/stretchr/testify/assert:go_default_library",
     ],

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/named_certificates.go

@@ -20,6 +20,7 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"fmt"
+	"net"
 	"strings"
 
 	v1 "k8s.io/api/core/v1"
@@ -76,7 +77,10 @@ func getCertificateNames(cert *x509.Certificate) []string {
 	var names []string
 
 	cn := cert.Subject.CommonName
-	if cn == "*" || len(validation.IsDNS1123Subdomain(strings.TrimPrefix(cn, "*."))) == 0 {
+	cnIsIP := net.ParseIP(cn) != nil
+	cnIsValidDomain := cn == "*" || len(validation.IsDNS1123Subdomain(strings.TrimPrefix(cn, "*."))) == 0
+	// don't use the CN if it is a valid IP because our IP serving detection may unexpectedly use it to terminate the connection.
+	if !cnIsIP && cnIsValidDomain {
 		names = append(names, cn)
 	}
 	for _, san := range cert.DNSNames {

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/named_certificates_test.go

@@ -68,6 +68,20 @@ func TestBuiltNamedCertificates(t *testing.T) {
 				"test.com": 0,
 			},
 		},
+		{
+			// ip as cns are ignored
+			certs: []namedtestCertSpec{
+				{
+					testCertSpec: testCertSpec{
+						host:  "1.2.3.4",
+						names: []string{"test.com"},
+					},
+				},
+			},
+			expected: map[string]int{
+				"test.com": 0,
+			},
+		},
 		{
 			// ips are ignored
 			certs: []namedtestCertSpec{

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/server_test.go

@@ -0,0 +1,218 @@
+/*
+Copyright 2019 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package dynamiccertificates
+
+import (
+	"bytes"
+	"crypto/tls"
+	"crypto/x509"
+	"fmt"
+	"net"
+	"net/http"
+	"strings"
+	"testing"
+	"time"
+
+	"k8s.io/client-go/util/cert"
+)
+
+func TestServingCert(t *testing.T) {
+	tlsConfig := &tls.Config{
+		// Can't use SSLv3 because of POODLE and BEAST
+		// Can't use TLSv1.0 because of POODLE and BEAST using CBC cipher
+		// Can't use TLSv1.1 because of RC4 cipher usage
+		MinVersion: tls.VersionTLS12,
+		// enable HTTP2 for go's 1.7 HTTP Server
+		NextProtos: []string{"h2", "http/1.1"},
+	}
+
+	defaultCertProvider, err := createTestTLSCerts(testCertSpec{
+		host:  "172.30.0.1",
+		ips:   []string{"172.30.0.1"},
+		names: []string{"kubernetes", "kubernetes.default.svc.cluster.local"},
+	}, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	var sniCerts []SNICertKeyContentProvider
+	certSpecs := []testCertSpec{
+		{
+			host:  "172.30.0.1",
+			ips:   []string{"172.30.0.1"},
+			names: []string{"openshift", "openshift.default.svc.cluster.local"},
+		},
+		{
+			host:  "127.0.0.1",
+			ips:   []string{"127.0.0.1"},
+			names: []string{"localhost"},
+		},
+		{
+			host:  "2001:abcd:bcda::1",
+			ips:   []string{"2001:abcd:bcda::1"},
+			names: []string{"openshiftv6", "openshiftv6.default.svc.cluster.local"},
+		},
+	}
+
+	for _, certSpec := range certSpecs {
+		names := append([]string{}, certSpec.ips...)
+		names = append(names, certSpec.names...)
+		certProvider, err := createTestTLSCerts(certSpec, names)
+		if err != nil {
+			t.Fatal(err)
+		}
+		sniCerts = append(sniCerts, certProvider)
+	}
+
+	dynamicCertificateController := NewDynamicServingCertificateController(
+		*tlsConfig,
+		&nullCAContent{name: "client-ca"},
+		defaultCertProvider,
+		sniCerts,
+		nil, // TODO see how to plumb an event recorder down in here. For now this results in simply klog messages.
+	)
+	if err := dynamicCertificateController.RunOnce(); err != nil {
+		t.Fatal(err)
+	}
+	tlsConfig.GetConfigForClient = dynamicCertificateController.GetConfigForClient
+	tlsConfig.GetCertificate = func(*tls.ClientHelloInfo) (*tls.Certificate, error) { return nil, fmt.Errorf("positive failure") }
+
+	stopCh := make(chan struct{})
+	defer close(stopCh)
+	server := &http.Server{
+		Handler: http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+			w.WriteHeader(http.StatusOK)
+		}),
+		MaxHeaderBytes: 1 << 20,
+		TLSConfig:      tlsConfig,
+	}
+	listener, _, err := createListener("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatal(err)
+	}
+	apiPort := listener.Addr().String()
+	go func() {
+		t.Logf("listening on %s", listener.Addr().String())
+		listener = tls.NewListener(listener, server.TLSConfig)
+		if err := server.ServeTLS(listener, "", ""); err != nil {
+			t.Error(err)
+			panic(err)
+		}
+
+		<-stopCh
+		server.Close()
+		t.Logf("stopped listening on %s", listener.Addr().String())
+	}()
+
+	time.Sleep(1 * time.Second)
+
+	expectedDefaultCertBytes, _ := defaultCertProvider.CurrentCertKeyContent()
+	expectedServiceCertBytes, _ := sniCerts[0].CurrentCertKeyContent()
+	expectedLocalhostCertBytes, _ := sniCerts[1].CurrentCertKeyContent()
+	expectedServiceV6CertBytes, _ := sniCerts[2].CurrentCertKeyContent()
+
+	tests := []struct {
+		name       string
+		serverName string
+		expected   []byte
+	}{
+		{
+			name:       "default", // the no server name hits 127.0.0.1, which we hope matches by IP
+			serverName: "",
+			expected:   []byte(strings.TrimSpace(string(expectedLocalhostCertBytes))),
+		},
+		{
+			name:       "invalid",
+			serverName: "not-marked",
+			expected:   []byte(strings.TrimSpace(string(expectedDefaultCertBytes))),
+		},
+		{
+			name:       "service by dns",
+			serverName: "openshift",
+			expected:   []byte(strings.TrimSpace(string(expectedServiceCertBytes))),
+		},
+		{
+			name:       "service v6 by dns",
+			serverName: "openshiftv6",
+			expected:   []byte(strings.TrimSpace(string(expectedServiceV6CertBytes))),
+		},
+		{
+			name:       "localhost by dns",
+			serverName: "localhost",
+			expected:   []byte(strings.TrimSpace(string(expectedLocalhostCertBytes))),
+		},
+		{
+			name:       "localhost by IP",
+			serverName: "127.0.0.1", // this can never actually happen, but let's see
+			expected:   []byte(strings.TrimSpace(string(expectedLocalhostCertBytes))),
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			_, actualDefaultCertBytes, err := cert.GetServingCertificates(apiPort, test.serverName)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if e, a := test.expected, actualDefaultCertBytes[0]; !bytes.Equal(e, a) {
+				eCert, _ := cert.ParseCertsPEM(e)
+				aCert, _ := cert.ParseCertsPEM(a)
+				t.Fatalf("expected %v\n, got %v", GetHumanCertDetail(eCert[0]), GetHumanCertDetail(aCert[0]))
+			}
+		})
+	}
+
+}
+
+func createListener(network, addr string) (net.Listener, int, error) {
+	if len(network) == 0 {
+		network = "tcp"
+	}
+	ln, err := net.Listen(network, addr)
+	if err != nil {
+		return nil, 0, fmt.Errorf("failed to listen on %v: %v", addr, err)
+	}
+
+	// get port
+	tcpAddr, ok := ln.Addr().(*net.TCPAddr)
+	if !ok {
+		ln.Close()
+		return nil, 0, fmt.Errorf("invalid listen address: %q", ln.Addr().String())
+	}
+
+	return ln, tcpAddr.Port, nil
+}
+
+type nullCAContent struct {
+	name string
+}
+
+var _ CAContentProvider = &nullCAContent{}
+
+// Name is just an identifier
+func (c *nullCAContent) Name() string {
+	return c.name
+}
+
+// CurrentCABundleContent provides ca bundle byte content
+func (c *nullCAContent) CurrentCABundleContent() (cabundle []byte) {
+	return nil
+}
+
+func (c *nullCAContent) VerifyOptions() (x509.VerifyOptions, bool) {
+	return x509.VerifyOptions{}, false
+}

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/tlsconfig.go

@@ -21,6 +21,7 @@ import (
 	"crypto/x509"
 	"errors"
 	"fmt"
+	"net"
 	"sync/atomic"
 	"time"
 
@@ -94,7 +95,28 @@ func (c *DynamicServingCertificateController) GetConfigForClient(clientHello *tl
 		return nil, errors.New("dynamiccertificates: unexpected config type")
 	}
 
-	return tlsConfig.Clone(), nil
+	tlsConfigCopy := tlsConfig.Clone()
+
+	// if the client set SNI information, just use our "normal" SNI flow
+	if len(clientHello.ServerName) > 0 {
+		return tlsConfigCopy, nil
+	}
+
+	// if the client didn't set SNI, then we need to inspect the requested IP so that we can choose
+	// a certificate from our list if we specifically handle that IP.  This can happen when an IP is specifically mapped by name.
+	host, _, err := net.SplitHostPort(clientHello.Conn.LocalAddr().String())
+	if err != nil {
+		return tlsConfigCopy, nil
+	}
+
+	ipCert, ok := tlsConfigCopy.NameToCertificate[host]
+	if !ok {
+		return tlsConfigCopy, nil
+	}
+	tlsConfigCopy.Certificates = []tls.Certificate{*ipCert}
+	tlsConfigCopy.NameToCertificate = nil
+
+	return tlsConfigCopy, nil
 }
 
 // newTLSContent determines the next set of content for overriding the baseTLSConfig.

staging/src/k8s.io/apiserver/pkg/server/options/serving.go

@@ -180,7 +180,9 @@ func (s *SecureServingOptions) AddFlags(fs *pflag.FlagSet) {
 	fs.Var(cliflag.NewNamedCertKeyArray(&s.SNICertKeys), "tls-sni-cert-key", ""+
 		"A pair of x509 certificate and private key file paths, optionally suffixed with a list of "+
 		"domain patterns which are fully qualified domain names, possibly with prefixed wildcard "+
-		"segments. If no domain patterns are provided, the names of the certificate are "+
+		"segments. The domain patterns also allow IP addresses, but IPs should only be used if "+
+		"the apiserver has visibility to the IP address requested by a client. "+
+		"If no domain patterns are provided, the names of the certificate are "+
 		"extracted. Non-wildcard matches trump over wildcard matches, explicit domain patterns "+
 		"trump over extracted names. For multiple key/certificate pairs, use the "+
 		"--tls-sni-cert-key multiple times. "+