diff --git a/pkg/util/net/http.go b/pkg/util/net/http.go index 7449cbb0a..7b64e6815 100644 --- a/pkg/util/net/http.go +++ b/pkg/util/net/http.go @@ -446,7 +446,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. diff --git a/pkg/util/net/http_test.go b/pkg/util/net/http_test.go index da483c356..2fd3675d4 100644 --- a/pkg/util/net/http_test.go +++ b/pkg/util/net/http_test.go @@ -330,13 +330,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"}, diff --git a/pkg/util/proxy/upgradeaware.go b/pkg/util/proxy/upgradeaware.go index d007c10b5..39c79be81 100644 --- a/pkg/util/proxy/upgradeaware.go +++ b/pkg/util/proxy/upgradeaware.go @@ -298,6 +298,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) diff --git a/pkg/util/proxy/upgradeaware_test.go b/pkg/util/proxy/upgradeaware_test.go index 710fc6a9c..c1dfb029d 100644 --- a/pkg/util/proxy/upgradeaware_test.go +++ b/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,