Merge pull request #49353 from liggitt/aggregator-tls

Automatic merge from submit-queue

Use specified ServerName in aggregator TLS validation

Fixes #49354

The aggregator sets a ServerName in the proxier tlsConfig, but the code path handling websocket upgrade requests did not honor it, and instead tried to verify TLS using the dialed host

* Honors ServerName if already set in tls.Config
* Adds unit tests for upgrade functionality via the aggregator
* Fixes mutation of shared tlsConfig.ServerName in spdy roundtripper

```release-note
Websocket requests to aggregated APIs now perform TLS verification using the service DNS name instead of the backend server's IP address, consistent with non-websocket requests.
```

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go

@@ -158,15 +158,16 @@ func (s *SpdyRoundTripper) dial(req *http.Request) (net.Conn, error) {
 		return nil, err
 	}
 
-	if s.tlsConfig == nil {
+	tlsConfig := s.tlsConfig
-		s.tlsConfig = &tls.Config{}
+	switch {
+	case tlsConfig == nil:
+		tlsConfig = &tls.Config{ServerName: host}
+	case len(tlsConfig.ServerName) == 0:
+		tlsConfig = tlsConfig.Clone()
+		tlsConfig.ServerName = host
 	}
 
-	if len(s.tlsConfig.ServerName) == 0 {
+	tlsConn := tls.Client(rwc, tlsConfig)
-		s.tlsConfig.ServerName = host
-	}
-
-	tlsConn := tls.Client(rwc, s.tlsConfig)
 
 	// need to manually call Handshake() so we can call VerifyHostname() below
 	if err := tlsConn.Handshake(); err != nil {
@@ -174,11 +175,11 @@ func (s *SpdyRoundTripper) dial(req *http.Request) (net.Conn, error) {
 	}
 
 	// Return if we were configured to skip validation
-	if s.tlsConfig != nil && s.tlsConfig.InsecureSkipVerify {
+	if tlsConfig.InsecureSkipVerify {
 		return tlsConn, nil
 	}
 
-	if err := tlsConn.VerifyHostname(host); err != nil {
+	if err := tlsConn.VerifyHostname(tlsConfig.ServerName); err != nil {
 		return nil, err
 	}
 
@@ -218,6 +219,9 @@ func (s *SpdyRoundTripper) dialWithoutProxy(url *url.URL) (net.Conn, error) {
 	if err != nil {
 		return nil, err
 	}
+	if s.tlsConfig != nil && len(s.tlsConfig.ServerName) > 0 {
+		host = s.tlsConfig.ServerName
+	}
 	err = conn.VerifyHostname(host)
 	if err != nil {
 		return nil, err

staging/src/k8s.io/apimachinery/pkg/util/proxy/dial.go

@@ -94,6 +94,9 @@ func DialURL(url *url.URL, transport http.RoundTripper) (net.Conn, error) {
 
 		// Verify
 		host, _, _ := net.SplitHostPort(dialAddr)
+		if tlsConfig != nil && len(tlsConfig.ServerName) > 0 {
+			host = tlsConfig.ServerName
+		}
 		if err := tlsConn.VerifyHostname(host); err != nil {
 			tlsConn.Close()
 			return nil, err

staging/src/k8s.io/kube-aggregator/pkg/apiserver/BUILD

@@ -17,6 +17,7 @@ go_test(
     library = ":go_default_library",
     tags = ["automanaged"],
     deps = [
+        "//vendor/golang.org/x/net/websocket:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/equality:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy_test.go

@@ -17,21 +17,23 @@ limitations under the License.
 package apiserver
 
 import (
+	"crypto/tls"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"net/http/httputil"
+	"net/url"
 	"reflect"
 	"strings"
 	"testing"
 
+	"golang.org/x/net/websocket"
+
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apiserver/pkg/authentication/user"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
-
 	"k8s.io/kube-aggregator/pkg/apis/apiregistration"
-	"net/url"
 )
 
 type targetHTTPHandler struct {
@@ -92,7 +94,13 @@ func (r *mockedRouter) ResolveEndpoint(namespace, name string) (*url.URL, error)
 
 func TestProxyHandler(t *testing.T) {
 	target := &targetHTTPHandler{}
-	targetServer := httptest.NewTLSServer(target)
+	targetServer := httptest.NewUnstartedServer(target)
+	if cert, err := tls.X509KeyPair(svcCrt, svcKey); err != nil {
+		t.Fatal(err)
+	} else {
+		targetServer.TLS = &tls.Config{Certificates: []tls.Certificate{cert}}
+	}
+	targetServer.StartTLS()
 	defer targetServer.Close()
 
 	tests := map[string]struct {
@@ -120,7 +128,7 @@ func TestProxyHandler(t *testing.T) {
 			expectedStatusCode: http.StatusInternalServerError,
 			expectedBody:       "missing user",
 		},
-		"proxy with user": {
+		"proxy with user, insecure": {
 			user: &user.DefaultInfo{
 				Name:   "username",
 				Groups: []string{"one", "two"},
@@ -147,6 +155,33 @@ func TestProxyHandler(t *testing.T) {
 				"X-Remote-Group":    {"one", "two"},
 			},
 		},
+		"proxy with user, cabundle": {
+			user: &user.DefaultInfo{
+				Name:   "username",
+				Groups: []string{"one", "two"},
+			},
+			path: "/request/path",
+			apiService: &apiregistration.APIService{
+				ObjectMeta: metav1.ObjectMeta{Name: "v1.foo"},
+				Spec: apiregistration.APIServiceSpec{
+					Service:  &apiregistration.ServiceReference{Name: "test-service", Namespace: "test-ns"},
+					Group:    "foo",
+					Version:  "v1",
+					CABundle: testCACrt,
+				},
+			},
+			expectedStatusCode: http.StatusOK,
+			expectedCalled:     true,
+			expectedHeaders: map[string][]string{
+				"X-Forwarded-Proto": {"https"},
+				"X-Forwarded-Uri":   {"/request/path"},
+				"X-Forwarded-For":   {"127.0.0.1"},
+				"X-Remote-User":     {"username"},
+				"User-Agent":        {"Go-http-client/1.1"},
+				"Accept-Encoding":   {"gzip"},
+				"X-Remote-Group":    {"one", "two"},
+			},
+		},
 		"fail on bad serving cert": {
 			user: &user.DefaultInfo{
 				Name:   "username",
@@ -218,3 +253,188 @@ func TestProxyHandler(t *testing.T) {
 		}()
 	}
 }
+
+func TestProxyUpgrade(t *testing.T) {
+	testcases := map[string]struct {
+		APIService   *apiregistration.APIService
+		ExpectError  bool
+		ExpectCalled bool
+	}{
+		"valid hostname + CABundle": {
+			APIService: &apiregistration.APIService{
+				Spec: apiregistration.APIServiceSpec{
+					CABundle: testCACrt,
+					Group:    "mygroup",
+					Version:  "v1",
+					Service:  &apiregistration.ServiceReference{Name: "test-service", Namespace: "test-ns"},
+				},
+			},
+			ExpectError:  false,
+			ExpectCalled: true,
+		},
+		"invalid hostname + insecure": {
+			APIService: &apiregistration.APIService{
+				Spec: apiregistration.APIServiceSpec{
+					InsecureSkipTLSVerify: true,
+					Group:   "mygroup",
+					Version: "v1",
+					Service: &apiregistration.ServiceReference{Name: "invalid-service", Namespace: "invalid-ns"},
+				},
+			},
+			ExpectError:  false,
+			ExpectCalled: true,
+		},
+		"invalid hostname + CABundle": {
+			APIService: &apiregistration.APIService{
+				Spec: apiregistration.APIServiceSpec{
+					CABundle: testCACrt,
+					Group:    "mygroup",
+					Version:  "v1",
+					Service:  &apiregistration.ServiceReference{Name: "invalid-service", Namespace: "invalid-ns"},
+				},
+			},
+			ExpectError:  true,
+			ExpectCalled: false,
+		},
+	}
+
+	for k, tc := range testcases {
+		tcName := k
+		path := "/apis/" + tc.APIService.Spec.Group + "/" + tc.APIService.Spec.Version + "/foo"
+		called := false
+
+		func() { // Cleanup after each test case.
+			backendHandler := http.NewServeMux()
+			backendHandler.Handle(path, websocket.Handler(func(ws *websocket.Conn) {
+				defer ws.Close()
+				body := make([]byte, 5)
+				ws.Read(body)
+				ws.Write([]byte("hello " + string(body)))
+				called = true
+			}))
+
+			backendServer := httptest.NewUnstartedServer(backendHandler)
+			if cert, err := tls.X509KeyPair(svcCrt, svcKey); err != nil {
+				t.Errorf("https (valid hostname): %v", err)
+				return
+			} else {
+				backendServer.TLS = &tls.Config{Certificates: []tls.Certificate{cert}}
+			}
+			backendServer.StartTLS()
+			defer backendServer.Close()
+
+			defer func() {
+				if called != tc.ExpectCalled {
+					t.Errorf("%s: expected called=%v, got %v", tcName, tc.ExpectCalled, called)
+				}
+			}()
+
+			serverURL, _ := url.Parse(backendServer.URL)
+			proxyHandler := &proxyHandler{
+				contextMapper:   &fakeRequestContextMapper{user: &user.DefaultInfo{Name: "username"}},
+				serviceResolver: &mockedRouter{destinationHost: serverURL.Host},
+				proxyTransport:  &http.Transport{},
+			}
+			proxyHandler.updateAPIService(tc.APIService)
+			aggregator := httptest.NewServer(proxyHandler)
+			defer aggregator.Close()
+
+			ws, err := websocket.Dial("ws://"+aggregator.Listener.Addr().String()+path, "", "http://127.0.0.1/")
+			if err != nil {
+				if !tc.ExpectError {
+					t.Errorf("%s: websocket dial err: %s", tcName, err)
+				}
+				return
+			}
+			defer ws.Close()
+			if tc.ExpectError {
+				t.Errorf("%s: expected websocket error, got none", tcName)
+				return
+			}
+
+			if _, err := ws.Write([]byte("world")); err != nil {
+				t.Errorf("%s: write err: %s", tcName, err)
+				return
+			}
+
+			response := make([]byte, 20)
+			n, err := ws.Read(response)
+			if err != nil {
+				t.Errorf("%s: read err: %s", tcName, err)
+				return
+			}
+			if e, a := "hello world", string(response[0:n]); e != a {
+				t.Errorf("%s: expected '%#v', got '%#v'", tcName, e, a)
+				return
+			}
+		}()
+	}
+}
+
+var testCACrt = []byte(`-----BEGIN CERTIFICATE-----
+MIICxDCCAaygAwIBAgIBATANBgkqhkiG9w0BAQsFADASMRAwDgYDVQQDEwd0ZXN0
+LWNhMCAXDTE3MDcyMDIxMTc1MloYDzIxMTcwNjI2MjExNzUzWjASMRAwDgYDVQQD
+Ewd0ZXN0LWNhMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAuv/sT2xH
+VS1/uXVNAEIwvEb2yTMbXwP6FD38LWkc37Ri7YMB9xiXEDBrbr6K1JThsqyitBxU
+22QIl53LUm6I7c/vej1tdYtE2rDVuviiiRgy6omR8imVSv9vU024rgDe+nC9zTT1
+3aNKR03olCG6fkygdcZOghzlyQLhyh8LG75XdnLNksnakum2dNxQ5QIFmBKAuev3
+A069oRMNjudot+t/nFP9UDZ8dL80PNTNPF22bPsnxiau7KLZ4I0Lf7gt6yHlNcue
+Fd5sqzqsw/LUFJR5Xuo1+0e7NV3SwCH5CymG6hkboM4Rf5S3EDDyXTxPbXzbQHf1
+7ksW6gjAxh4x/wIDAQABoyMwITAOBgNVHQ8BAf8EBAMCAqQwDwYDVR0TAQH/BAUw
+AwEB/zANBgkqhkiG9w0BAQsFAAOCAQEATgmDrW1BjFp+Vmw6T+ojVK4lJuIoerGw
+TCCqabHs6O1iWkNi5KsY6vV86tofBIEXsf6S3mV2jcBn87+CIbNHlHFKrXwmcydA
+WOc0LWVqqoeqIvEcMNoWQskzmOOUDTanX9mXkirm8d8BljC351TH17rSjLGzFuNh
+Cy48xyKFM7kPauNZGfCyaZsGbNJP3Keeu35dOLZMDdBJw7ZvYEUqX7MLOO+d7vlO
+JGNA5jsU2uBnSo6qsjxfsbGemk2uRO0nLhplWurw+4qzA79D0lKNLtH9yTn12KZb
+/kUpsOSCtLomjWwp67lQyA/yFXf897pSKMXbnIfZfIlDg51CI3U2Sw==
+-----END CERTIFICATE-----`)
+
+// valid for hostname test-service.test-ns.svc
+// signed by testCACrt
+var svcCrt = []byte(`-----BEGIN CERTIFICATE-----
+MIIDDDCCAfSgAwIBAgIBBDANBgkqhkiG9w0BAQsFADASMRAwDgYDVQQDEwd0ZXN0
+LWNhMCAXDTE3MDcyMDIxMjAzN1oYDzIxMTcwNjI2MjEyMDM4WjAjMSEwHwYDVQQD
+Exh0ZXN0LXNlcnZpY2UudGVzdC1ucy5zdmMwggEiMA0GCSqGSIb3DQEBAQUAA4IB
+DwAwggEKAoIBAQDOKgoTmlVeDhImiBLBccxdniKkS+FZSaoAEtoTvJG1wjk0ewzF
+vKhjbHolJ+/qEANiQ6CpTz4hU3m/Iad6IrnmKd1jnkh9yKEaU32B2Xbh6VaV7Sca
+Hv4cKWTe50sBvufZinTT8hlFcGufFlJIOLXya5t6HH1Ld7Xf2qwNqusHdmFlJko7
+0By8jhTtD7+2OAJsIPQDWfAsXxFa6LeQ/lqS2DCFnp45DirTNetXoIH8ZJvTBjak
+bQuAAA3H+61gRm1blIu8/JjHYTDOcUe5pFyrFLFPgA+eIcpIbzTD61UTNhVlusV2
+eRrBr5BlRM13Zj6ZMcWp0Iiw5QI/W9QU7O4jAgMBAAGjWjBYMA4GA1UdDwEB/wQE
+AwIFoDATBgNVHSUEDDAKBggrBgEFBQcDATAMBgNVHRMBAf8EAjAAMCMGA1UdEQQc
+MBqCGHRlc3Qtc2VydmljZS50ZXN0LW5zLnN2YzANBgkqhkiG9w0BAQsFAAOCAQEA
+kpULlml6Ct0cjOuHgDKUnTboFTUm2FHJY27p4NXUvXUMoipg/eSxk0r5JynzXrPa
+jaJfY2bC45ixLjJv9irp9ER/lGYUcBQ8OHouXy+uKtA5YKHI3wYf8ITZrQwzRpsK
+C5v7qDW+iyb9dn4T6qgpZvylKtkH5cH31hQooNgjZd5aEq3JSFnCM12IVqs/5kjL
+NnbPXzeBQ9CHbC+mx7Mm6eSQVtGcQOy4yXFrh2/vrIB2t4gNeWaI1b+7l4MaJjV/
+kRrOirhZaJ90ow/PdYrILtEAdpeC/2Varpr3l4rIKhkkop4gfPwaFeWhG38shH3E
+eG5PW2waPpxJzEnGBoAWxQ==
+-----END CERTIFICATE-----`)
+
+var svcKey = []byte(`-----BEGIN RSA PRIVATE KEY-----
+MIIEogIBAAKCAQEAzioKE5pVXg4SJogSwXHMXZ4ipEvhWUmqABLaE7yRtcI5NHsM
+xbyoY2x6JSfv6hADYkOgqU8+IVN5vyGneiK55indY55IfcihGlN9gdl24elWle0n
+Gh7+HClk3udLAb7n2Yp00/IZRXBrnxZSSDi18mubehx9S3e139qsDarrB3ZhZSZK
+O9AcvI4U7Q+/tjgCbCD0A1nwLF8RWui3kP5aktgwhZ6eOQ4q0zXrV6CB/GSb0wY2
+pG0LgAANx/utYEZtW5SLvPyYx2EwznFHuaRcqxSxT4APniHKSG80w+tVEzYVZbrF
+dnkawa+QZUTNd2Y+mTHFqdCIsOUCP1vUFOzuIwIDAQABAoIBABiX9z/DZ2+i6hNi
+pCojcyev154V1zoZiYgct5snIZK3Kq/SBgIIsWW66Q9Jplsbseuk+aN46oZ7OMjO
+MPZm8ho84EYj+a3XozBKyWwWDxKADW4xLjr1e4bMgVX97Xq11V6kH6+w78bS1GPT
++9jVuw7CO3fjsiawjye3JFM1Enh/NeRLEpT/oaQoWIV8b0IQB0VyqrdxWOO0rQhd
+xA5w39tAZPDQ79MbMQyNWtPgBy0FuulP0GB12PrEbE+SXxsFhWViEwdB5Qx6Gqsx
+KGn9vB1oaeSuuKIAjyBV0rXszrGektorDchsOY9UQi1mQsPSvvRFTM9T3qqSFIpu
+oPNQLvECgYEA3ox3WJGjEve6VI4RMRt0l6ZFswNbNaHcTMPVsayqsl9KfebG+uyn
+Z7TyyoCRzZZQa+3Z9jjW3hAGM9e7MG8jkeHbZpJpZv9X7eB3dgq3eZ1Zt5dyoDrU
+PTdIPA2efFAf6V1ejyqH9h6RPQMeAb4uFU9nbI4rPagMxRdp5qIveIUCgYEA7Scb
+0zWplDit4EUo+Fq80wzItwJZv64my8KIkEPpW3Fu6UPQvY74qyhE2fCSCwHqRpYJ
+jVylyE0GIMx42kjwBgOpi4yEg8M3uMTal+Iy9SgrxZ5cPetaFpEF3Wk7/tz6ppr+
+wnZQTO2WH3YLzv7JIWVrOKuBNVfNEbguVFWw4IcCgYB54mp2uoSancySBKDLyWKo
+r6raqQrqK7TQ4iyGO6/dMy1EGQF/ad8hgEu8tn+kHh/7jG/kVyruwc3z1MIze5r6
+ib00xxktDMnmgRpMLwBffdsmHq7rrGyS/lT0du0G3ocrszRXqo5+MC2RQcTMZZEt
+oKhfHtn10bT0uKcKZmcjVQKBgEls2WWccMOuhM8yOowic+IYTDC1bpo1Tle6BFQ+
+YoroZQGd+IwoLv+3ORINNPppfmKaY5y7+aw5hNM025oiCQajraPCPukY0TDI6jEq
+XMKgzGSkMkUNkFf6UMmLooK3Yneg94232gbnbJqTDvbo1dccMoVaPGgKpjh9QQLl
+gR0TAoGACFOvhl8txfbkwLeuNeunyOPL7J4nIccthgd2ioFOr3HTou6wzN++vYTa
+a3OF9jH5Z7m6X1rrwn6J1+Gw9sBme38/GeGXHigsBI/8WaTvyuppyVIXOVPoTvVf
+VYsTwo5YgV1HzDkV+BNmBCw1GYcGXAElhJI+dCsgQuuU6TKzgl8=
+-----END RSA PRIVATE KEY-----`)