Merge pull request kubernetes#92941 from tallclair/invalid-redirect

Don't return proxied redirects to the client

Author: k8s-ci-robot

staging/src/k8s.io/apimachinery/pkg/util/net/http.go

@@ -453,7 +453,7 @@ redirectLoop:
 
 		// Only follow redirects to the same host. Otherwise, propagate the redirect response back.
 		if requireSameHostRedirects && location.Hostname() != originalLocation.Hostname() {
-			break redirectLoop
+			return nil, nil, fmt.Errorf("hostname mismatch: expected %s, found %s", originalLocation.Hostname(), location.Hostname())
 		}
 
 		// Reset the connection.

staging/src/k8s.io/apimachinery/pkg/util/net/http_test.go

@@ -331,13 +331,13 @@ func TestConnectWithRedirects(t *testing.T) {
 		redirects:   []string{"/1", "/2", "/3", "/4", "/5", "/6", "/7", "/8", "/9", "/10"},
 		expectError: true,
 	}, {
-		desc:              "redirect to different host are prevented",
-		redirects:         []string{"http://example.com/foo"},
-		expectedRedirects: 0,
+		desc:        "redirect to different host are prevented",
+		redirects:   []string{"http://example.com/foo"},
+		expectError: true,
 	}, {
-		desc:              "multiple redirect to different host forbidden",
-		redirects:         []string{"/1", "/2", "/3", "http://example.com/foo"},
-		expectedRedirects: 3,
+		desc:        "multiple redirect to different host forbidden",
+		redirects:   []string{"/1", "/2", "/3", "http://example.com/foo"},
+		expectError: true,
 	}, {
 		desc:              "redirect to different port is allowed",
 		redirects:         []string{"http://HOST/foo"},

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

@@ -304,6 +304,16 @@ func (h *UpgradeAwareHandler) tryUpgrade(w http.ResponseWriter, req *http.Reques
 		rawResponse = headerBytes
 	}
 
+	// If the backend did not upgrade the request, return an error to the client. If the response was
+	// an error, the error is forwarded directly after the connection is hijacked. Otherwise, just
+	// return a generic error here.
+	if backendHTTPResponse.StatusCode != http.StatusSwitchingProtocols && backendHTTPResponse.StatusCode < 400 {
+		err := fmt.Errorf("invalid upgrade response: status code %d", backendHTTPResponse.StatusCode)
+		klog.Errorf("Proxy upgrade error: %v", err)
+		h.Responder.Error(w, req, err)
+		return true
+	}
+
 	// Once the connection is hijacked, the ErrorResponder will no longer work, so
 	// hijacking should be the last step in the upgrade.
 	requestHijacker, ok := w.(http.Hijacker)

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

@@ -512,7 +512,7 @@ func (r *noErrorsAllowed) Error(w http.ResponseWriter, req *http.Request, err er
 	r.t.Error(err)
 }
 
-func TestProxyUpgradeErrorResponse(t *testing.T) {
+func TestProxyUpgradeConnectionErrorResponse(t *testing.T) {
 	var (
 		responder   *fakeResponder
 		expectedErr = errors.New("EXPECTED")
@@ -560,7 +560,7 @@ func TestProxyUpgradeErrorResponse(t *testing.T) {
 
 func TestProxyUpgradeErrorResponseTerminates(t *testing.T) {
 	for _, intercept := range []bool{true, false} {
-		for _, code := range []int{200, 400, 500} {
+		for _, code := range []int{400, 500} {
 			t.Run(fmt.Sprintf("intercept=%v,code=%v", intercept, code), func(t *testing.T) {
 				// Set up a backend server
 				backend := http.NewServeMux()
@@ -620,6 +620,49 @@ func TestProxyUpgradeErrorResponseTerminates(t *testing.T) {
 	}
 }
 
+func TestProxyUpgradeErrorResponse(t *testing.T) {
+	for _, intercept := range []bool{true, false} {
+		for _, code := range []int{200, 300, 302, 307} {
+			t.Run(fmt.Sprintf("intercept=%v,code=%v", intercept, code), func(t *testing.T) {
+				// Set up a backend server
+				backend := http.NewServeMux()
+				backend.Handle("/hello", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+					http.Redirect(w, r, "https://example.com/there", code)
+				}))
+				backendServer := httptest.NewServer(backend)
+				defer backendServer.Close()
+				backendServerURL, _ := url.Parse(backendServer.URL)
+				backendServerURL.Path = "/hello"
+
+				// Set up a proxy pointing to a specific path on the backend
+				proxyHandler := NewUpgradeAwareHandler(backendServerURL, nil, false, false, &fakeResponder{t: t})
+				proxyHandler.InterceptRedirects = intercept
+				proxyHandler.RequireSameHostRedirects = true
+				proxy := httptest.NewServer(proxyHandler)
+				defer proxy.Close()
+				proxyURL, _ := url.Parse(proxy.URL)
+
+				conn, err := net.Dial("tcp", proxyURL.Host)
+				require.NoError(t, err)
+				bufferedReader := bufio.NewReader(conn)
+
+				// Send upgrade request resulting in a non-101 response from the backend
+				req, _ := http.NewRequest("GET", "/", nil)
+				req.Header.Set(httpstream.HeaderConnection, httpstream.HeaderUpgrade)
+				require.NoError(t, req.Write(conn))
+				// Verify we get the correct response and full message body content
+				resp, err := http.ReadResponse(bufferedReader, nil)
+				require.NoError(t, err)
+				assert.Equal(t, fakeStatusCode, resp.StatusCode)
+				resp.Body.Close()
+
+				// clean up
+				conn.Close()
+			})
+		}
+	}
+}
+
 func TestDefaultProxyTransport(t *testing.T) {
 	tests := []struct {
 		name,