Merge pull request #21185 from fabianofranz/fix_spdy_roundtripper_verify_hostname

Auto commit by PR queue bot

pkg/util/httpstream/spdy/roundtripper.go

@@ -53,6 +53,10 @@ type SpdyRoundTripper struct {
 
 	// Dialer is the dialer used to connect.  Used if non-nil.
 	Dialer *net.Dialer
+
+	// proxier knows which proxy to use given a request, defaults to http.ProxyFromEnvironment
+	// Used primarily for mocking the proxy discovery in tests.
+	proxier func(req *http.Request) (*url.URL, error)
 }
 
 // NewRoundTripper creates a new SpdyRoundTripper that will use
@@ -70,7 +74,11 @@ func NewSpdyRoundTripper(tlsConfig *tls.Config) *SpdyRoundTripper {
 // dial dials the host specified by req, using TLS if appropriate, optionally
 // using a proxy server if one is configured via environment variables.
 func (s *SpdyRoundTripper) dial(req *http.Request) (net.Conn, error) {
-	proxyURL, err := http.ProxyFromEnvironment(req)
+	proxier := s.proxier
+	if proxier == nil {
+		proxier = http.ProxyFromEnvironment
+	}
+	proxyURL, err := proxier(req)
 	if err != nil {
 		return nil, err
 	}
@@ -106,7 +114,7 @@ func (s *SpdyRoundTripper) dial(req *http.Request) (net.Conn, error) {
 		return rwc, nil
 	}
 
-	host, _, err := net.SplitHostPort(req.URL.Host)
+	host, _, err := net.SplitHostPort(targetHost)
 	if err != nil {
 		return nil, err
 	}
@@ -122,6 +130,11 @@ func (s *SpdyRoundTripper) dial(req *http.Request) (net.Conn, error) {
 		return nil, err
 	}
 
+	// Return if we were configured to skip validation
+	if s.tlsConfig != nil && s.tlsConfig.InsecureSkipVerify {
+		return tlsConn, nil
+	}
+
 	if err := tlsConn.VerifyHostname(host); err != nil {
 		return nil, err
 	}

pkg/util/httpstream/spdy/roundtripper_test.go

@@ -22,20 +22,48 @@ import (
 	"io"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"testing"
 
+	"github.com/elazarl/goproxy"
 	"k8s.io/kubernetes/pkg/util/httpstream"
 )
 
 func TestRoundTripAndNewConnection(t *testing.T) {
-
 	localhostPool := x509.NewCertPool()
 	if !localhostPool.AppendCertsFromPEM(localhostCert) {
 		t.Errorf("error setting up localhostCert pool")
 	}
 
+	httpsServerInvalidHostname := func(h http.Handler) *httptest.Server {
+		cert, err := tls.X509KeyPair(exampleCert, exampleKey)
+		if err != nil {
+			t.Errorf("https (invalid hostname): proxy_test: %v", err)
+		}
+		ts := httptest.NewUnstartedServer(h)
+		ts.TLS = &tls.Config{
+			Certificates: []tls.Certificate{cert},
+		}
+		ts.StartTLS()
+		return ts
+	}
+
+	httpsServerValidHostname := func(h http.Handler) *httptest.Server {
+		cert, err := tls.X509KeyPair(localhostCert, localhostKey)
+		if err != nil {
+			t.Errorf("https (valid hostname): proxy_test: %v", err)
+		}
+		ts := httptest.NewUnstartedServer(h)
+		ts.TLS = &tls.Config{
+			Certificates: []tls.Certificate{cert},
+		}
+		ts.StartTLS()
+		return ts
+	}
+
 	testCases := map[string]struct {
 		serverFunc             func(http.Handler) *httptest.Server
+		proxyServerFunc        func(http.Handler) *httptest.Server
 		clientTLS              *tls.Config
 		serverConnectionHeader string
 		serverUpgradeHeader    string
@@ -78,37 +106,85 @@ func TestRoundTripAndNewConnection(t *testing.T) {
 			shouldError:            false,
 		},
 		"https (invalid hostname + InsecureSkipVerify)": {
-			serverFunc: func(h http.Handler) *httptest.Server {
-				cert, err := tls.X509KeyPair(exampleCert, exampleKey)
-				if err != nil {
-					t.Errorf("https (invalid hostname): proxy_test: %v", err)
-				}
-				ts := httptest.NewUnstartedServer(h)
-				ts.TLS = &tls.Config{
-					Certificates: []tls.Certificate{cert},
-				}
-				ts.StartTLS()
-				return ts
-			},
+			serverFunc:             httpsServerInvalidHostname,
 			clientTLS:              &tls.Config{InsecureSkipVerify: true},
 			serverConnectionHeader: "Upgrade",
 			serverUpgradeHeader:    "SPDY/3.1",
 			serverStatusCode:       http.StatusSwitchingProtocols,
 			shouldError:            false,
 		},
+		"https (invalid hostname + hostname verification)": {
+			serverFunc:             httpsServerInvalidHostname,
+			clientTLS:              &tls.Config{InsecureSkipVerify: false},
+			serverConnectionHeader: "Upgrade",
+			serverUpgradeHeader:    "SPDY/3.1",
+			serverStatusCode:       http.StatusSwitchingProtocols,
+			shouldError:            true,
+		},
 		"https (valid hostname + RootCAs)": {
-			serverFunc: func(h http.Handler) *httptest.Server {
-				cert, err := tls.X509KeyPair(localhostCert, localhostKey)
-				if err != nil {
-					t.Errorf("https (valid hostname): proxy_test: %v", err)
-				}
-				ts := httptest.NewUnstartedServer(h)
-				ts.TLS = &tls.Config{
-					Certificates: []tls.Certificate{cert},
-				}
-				ts.StartTLS()
-				return ts
-			},
+			serverFunc:             httpsServerValidHostname,
+			clientTLS:              &tls.Config{RootCAs: localhostPool},
+			serverConnectionHeader: "Upgrade",
+			serverUpgradeHeader:    "SPDY/3.1",
+			serverStatusCode:       http.StatusSwitchingProtocols,
+			shouldError:            false,
+		},
+		"proxied http->http": {
+			serverFunc:             httptest.NewServer,
+			proxyServerFunc:        httptest.NewServer,
+			serverConnectionHeader: "Upgrade",
+			serverUpgradeHeader:    "SPDY/3.1",
+			serverStatusCode:       http.StatusSwitchingProtocols,
+			shouldError:            false,
+		},
+		"proxied https (invalid hostname + InsecureSkipVerify) -> http": {
+			serverFunc:             httptest.NewServer,
+			proxyServerFunc:        httpsServerInvalidHostname,
+			clientTLS:              &tls.Config{InsecureSkipVerify: true},
+			serverConnectionHeader: "Upgrade",
+			serverUpgradeHeader:    "SPDY/3.1",
+			serverStatusCode:       http.StatusSwitchingProtocols,
+			shouldError:            false,
+		},
+		"proxied https (invalid hostname + hostname verification) -> http": {
+			serverFunc:             httptest.NewServer,
+			proxyServerFunc:        httpsServerInvalidHostname,
+			clientTLS:              &tls.Config{InsecureSkipVerify: false},
+			serverConnectionHeader: "Upgrade",
+			serverUpgradeHeader:    "SPDY/3.1",
+			serverStatusCode:       http.StatusSwitchingProtocols,
+			shouldError:            true, // fails because the client doesn't trust the proxy
+		},
+		"proxied https (valid hostname + RootCAs) -> http": {
+			serverFunc:             httptest.NewServer,
+			proxyServerFunc:        httpsServerValidHostname,
+			clientTLS:              &tls.Config{RootCAs: localhostPool},
+			serverConnectionHeader: "Upgrade",
+			serverUpgradeHeader:    "SPDY/3.1",
+			serverStatusCode:       http.StatusSwitchingProtocols,
+			shouldError:            false,
+		},
+		"proxied https (invalid hostname + InsecureSkipVerify) -> https (invalid hostname)": {
+			serverFunc:             httpsServerInvalidHostname,
+			proxyServerFunc:        httpsServerInvalidHostname,
+			clientTLS:              &tls.Config{InsecureSkipVerify: true},
+			serverConnectionHeader: "Upgrade",
+			serverUpgradeHeader:    "SPDY/3.1",
+			serverStatusCode:       http.StatusSwitchingProtocols,
+			shouldError:            false, // works because the test proxy ignores TLS errors
+		},
+		"proxied https (invalid hostname + hostname verification) -> https (invalid hostname)": {
+			serverFunc:             httpsServerInvalidHostname,
+			proxyServerFunc:        httpsServerInvalidHostname,
+			clientTLS:              &tls.Config{InsecureSkipVerify: false},
+			serverConnectionHeader: "Upgrade",
+			serverUpgradeHeader:    "SPDY/3.1",
+			serverStatusCode:       http.StatusSwitchingProtocols,
+			shouldError:            true, // fails because the client doesn't trust the proxy
+		},
+		"proxied https (valid hostname + RootCAs) -> https (valid hostname + RootCAs)": {
+			serverFunc:             httpsServerValidHostname,
+			proxyServerFunc:        httpsServerValidHostname,
 			clientTLS:              &tls.Config{RootCAs: localhostPool},
 			serverConnectionHeader: "Upgrade",
 			serverUpgradeHeader:    "SPDY/3.1",
@@ -149,20 +225,46 @@ func TestRoundTripAndNewConnection(t *testing.T) {
 		// TODO: Uncomment when fix #19254
 		// defer server.Close()
 
+		serverURL, err := url.Parse(server.URL)
+		if err != nil {
+			t.Fatalf("%s: Error creating request: %s", k, err)
+		}
 		req, err := http.NewRequest("GET", server.URL, nil)
 		if err != nil {
 			t.Fatalf("%s: Error creating request: %s", k, err)
 		}
 
-		spdyTransport := NewRoundTripper(testCase.clientTLS)
+		spdyTransport := NewSpdyRoundTripper(testCase.clientTLS)
+
+		var proxierCalled bool
+		var proxyCalledWithHost string
+		if testCase.proxyServerFunc != nil {
+			proxyHandler := goproxy.NewProxyHttpServer()
+			proxyHandler.OnRequest().HandleConnectFunc(func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) {
+				proxyCalledWithHost = host
+				return goproxy.OkConnect, host
+			})
+			proxy := testCase.proxyServerFunc(proxyHandler)
+
+			spdyTransport.proxier = func(proxierReq *http.Request) (*url.URL, error) {
+				proxyURL, err := url.Parse(proxy.URL)
+				if err != nil {
+					return nil, err
+				}
+				proxierCalled = true
+				return proxyURL, nil
+			}
+			// TODO: Uncomment when fix #19254
+			// defer proxy.Close()
+		}
+
 		client := &http.Client{Transport: spdyTransport}
 
 		resp, err := client.Do(req)
-		if err != nil {
-			t.Fatalf("%s: unexpected error from client.Do: %s", k, err)
+		var conn httpstream.Connection
+		if err == nil {
+			conn, err = spdyTransport.NewConnection(resp)
 		}
-
-		conn, err := spdyTransport.NewConnection(resp)
 		haveErr := err != nil
 		if e, a := testCase.shouldError, haveErr; e != a {
 			t.Fatalf("%s: shouldError=%t, got %t: %v", k, e, a, err)
@@ -200,6 +302,15 @@ func TestRoundTripAndNewConnection(t *testing.T) {
 		if e, a := "hello", string(b[0:n]); e != a {
 			t.Fatalf("%s: expected '%s', got '%s'", k, e, a)
 		}
+
+		if testCase.proxyServerFunc != nil {
+			if !proxierCalled {
+				t.Fatalf("%s: Expected to use a proxy but proxier in SpdyRoundTripper wasn't called", k)
+			}
+			if proxyCalledWithHost != serverURL.Host {
+				t.Fatalf("%s: Expected to see a call to the proxy for backend %q, got %q", k, serverURL.Host, proxyCalledWithHost)
+			}
+		}
 	}
 }