From e1069c64956a43f628d8ae2fcd9107959747c1c6 Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Fri, 4 Mar 2022 16:08:58 -0800 Subject: [PATCH] Don't follow redirects with spdy --- pkg/kubelet/server/server_test.go | 8 +- .../pkg/util/httpstream/spdy/roundtripper.go | 41 +- .../util/httpstream/spdy/roundtripper_test.go | 1069 ++++++++--------- .../k8s.io/apimachinery/pkg/util/net/http.go | 99 -- .../apimachinery/pkg/util/net/http_test.go | 157 --- .../k8s.io/client-go/transport/spdy/spdy.go | 8 +- 6 files changed, 499 insertions(+), 883 deletions(-) diff --git a/pkg/kubelet/server/server_test.go b/pkg/kubelet/server/server_test.go index f1911c7c313..5df58ad988a 100644 --- a/pkg/kubelet/server/server_test.go +++ b/pkg/kubelet/server/server_test.go @@ -863,7 +863,7 @@ func TestServeExecInContainerIdleTimeout(t *testing.T) { url := fw.testHTTPServer.URL + "/exec/" + podNamespace + "/" + podName + "/" + expectedContainerName + "?c=ls&c=-a&" + api.ExecStdinParam + "=1" - upgradeRoundTripper := spdy.NewRoundTripper(nil, true, true) + upgradeRoundTripper := spdy.NewRoundTripper(nil) c := &http.Client{Transport: upgradeRoundTripper} resp, err := c.Do(makeReq(t, "POST", url, "v4.channel.k8s.io")) @@ -1019,7 +1019,7 @@ func testExecAttach(t *testing.T, verb string) { upgradeRoundTripper httpstream.UpgradeRoundTripper c *http.Client ) - upgradeRoundTripper = spdy.NewRoundTripper(nil, true, true) + upgradeRoundTripper = spdy.NewRoundTripper(nil) c = &http.Client{Transport: upgradeRoundTripper} resp, err = c.Do(makeReq(t, "POST", url, "v4.channel.k8s.io")) @@ -1115,7 +1115,7 @@ func TestServePortForwardIdleTimeout(t *testing.T) { url := fw.testHTTPServer.URL + "/portForward/" + podNamespace + "/" + podName - upgradeRoundTripper := spdy.NewRoundTripper(nil, true, true) + upgradeRoundTripper := spdy.NewRoundTripper(nil) c := &http.Client{Transport: upgradeRoundTripper} req := makeReq(t, "POST", url, "portforward.k8s.io") @@ -1214,7 +1214,7 @@ func TestServePortForward(t *testing.T) { c *http.Client ) - upgradeRoundTripper = spdy.NewRoundTripper(nil, true, true) + upgradeRoundTripper = spdy.NewRoundTripper(nil) c = &http.Client{Transport: upgradeRoundTripper} req := makeReq(t, "POST", url, "portforward.k8s.io") diff --git a/staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go b/staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go index 0597c6620fd..9ec3685015f 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go @@ -67,12 +67,6 @@ type SpdyRoundTripper struct { // Used primarily for mocking the proxy discovery in tests. proxier func(req *http.Request) (*url.URL, error) - // followRedirects indicates if the round tripper should examine responses for redirects and - // follow them. - followRedirects bool - // requireSameHostRedirects restricts redirect following to only follow redirects to the same host - // as the original request. - requireSameHostRedirects bool // pingPeriod is a period for sending Ping frames over established // connections. pingPeriod time.Duration @@ -84,22 +78,18 @@ var _ utilnet.Dialer = &SpdyRoundTripper{} // NewRoundTripper creates a new SpdyRoundTripper that will use the specified // tlsConfig. -func NewRoundTripper(tlsConfig *tls.Config, followRedirects, requireSameHostRedirects bool) *SpdyRoundTripper { +func NewRoundTripper(tlsConfig *tls.Config) *SpdyRoundTripper { return NewRoundTripperWithConfig(RoundTripperConfig{ - TLS: tlsConfig, - FollowRedirects: followRedirects, - RequireSameHostRedirects: requireSameHostRedirects, + TLS: tlsConfig, }) } // NewRoundTripperWithProxy creates a new SpdyRoundTripper that will use the // specified tlsConfig and proxy func. -func NewRoundTripperWithProxy(tlsConfig *tls.Config, followRedirects, requireSameHostRedirects bool, proxier func(*http.Request) (*url.URL, error)) *SpdyRoundTripper { +func NewRoundTripperWithProxy(tlsConfig *tls.Config, proxier func(*http.Request) (*url.URL, error)) *SpdyRoundTripper { return NewRoundTripperWithConfig(RoundTripperConfig{ - TLS: tlsConfig, - FollowRedirects: followRedirects, - RequireSameHostRedirects: requireSameHostRedirects, - Proxier: proxier, + TLS: tlsConfig, + Proxier: proxier, }) } @@ -110,11 +100,9 @@ func NewRoundTripperWithConfig(cfg RoundTripperConfig) *SpdyRoundTripper { cfg.Proxier = utilnet.NewProxierWithNoProxyCIDR(http.ProxyFromEnvironment) } return &SpdyRoundTripper{ - tlsConfig: cfg.TLS, - followRedirects: cfg.FollowRedirects, - requireSameHostRedirects: cfg.RequireSameHostRedirects, - proxier: cfg.Proxier, - pingPeriod: cfg.PingPeriod, + tlsConfig: cfg.TLS, + proxier: cfg.Proxier, + pingPeriod: cfg.PingPeriod, } } @@ -127,9 +115,6 @@ type RoundTripperConfig struct { // PingPeriod is a period for sending SPDY Pings on the connection. // Optional. PingPeriod time.Duration - - FollowRedirects bool - RequireSameHostRedirects bool } // TLSClientConfig implements pkg/util/net.TLSClientConfigHolder for proper TLS checking during @@ -365,13 +350,9 @@ func (s *SpdyRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) err error ) - if s.followRedirects { - conn, rawResponse, err = utilnet.ConnectWithRedirects(req.Method, req.URL, header, req.Body, s, s.requireSameHostRedirects) - } else { - clone := utilnet.CloneRequest(req) - clone.Header = header - conn, err = s.Dial(clone) - } + clone := utilnet.CloneRequest(req) + clone.Header = header + conn, err = s.Dial(clone) if err != nil { return nil, err } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper_test.go b/staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper_test.go index 779d03b9679..a437e080ec8 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper_test.go @@ -21,15 +21,12 @@ import ( "crypto/tls" "crypto/x509" "encoding/base64" - "fmt" "io" "net" "net/http" "net/http/httptest" "net/url" "strconv" - "strings" - "sync/atomic" "testing" "github.com/armon/go-socks5" @@ -119,304 +116,300 @@ func localhostCertPool(t *testing.T) *x509.CertPool { // be sure to unset environment variable https_proxy (if exported) before testing, otherwise the testing will fail unexpectedly. func TestRoundTripAndNewConnection(t *testing.T) { - for _, redirect := range []bool{false, true} { - t.Run(fmt.Sprintf("redirect = %t", redirect), func(t *testing.T) { - localhostPool := localhostCertPool(t) + localhostPool := localhostCertPool(t) - testCases := map[string]struct { - serverFunc func(http.Handler) *httptest.Server - proxyServerFunc func(http.Handler) *httptest.Server - proxyAuth *url.Userinfo - clientTLS *tls.Config - serverConnectionHeader string - serverUpgradeHeader string - serverStatusCode int - shouldError bool - }{ - "no headers": { - serverFunc: httptest.NewServer, - serverConnectionHeader: "", - serverUpgradeHeader: "", - serverStatusCode: http.StatusSwitchingProtocols, - shouldError: true, - }, - "no upgrade header": { - serverFunc: httptest.NewServer, - serverConnectionHeader: "Upgrade", - serverUpgradeHeader: "", - serverStatusCode: http.StatusSwitchingProtocols, - shouldError: true, - }, - "no connection header": { - serverFunc: httptest.NewServer, - serverConnectionHeader: "", - serverUpgradeHeader: "SPDY/3.1", - serverStatusCode: http.StatusSwitchingProtocols, - shouldError: true, - }, - "no switching protocol status code": { - serverFunc: httptest.NewServer, - serverConnectionHeader: "Upgrade", - serverUpgradeHeader: "SPDY/3.1", - serverStatusCode: http.StatusForbidden, - shouldError: true, - }, - "http": { - serverFunc: httptest.NewServer, - serverConnectionHeader: "Upgrade", - serverUpgradeHeader: "SPDY/3.1", - serverStatusCode: http.StatusSwitchingProtocols, - shouldError: false, - }, - "https (invalid hostname + InsecureSkipVerify)": { - serverFunc: httpsServerInvalidHostname(t), - clientTLS: &tls.Config{InsecureSkipVerify: true}, - serverConnectionHeader: "Upgrade", - serverUpgradeHeader: "SPDY/3.1", - serverStatusCode: http.StatusSwitchingProtocols, - shouldError: false, - }, - "https (invalid hostname + hostname verification)": { - serverFunc: httpsServerInvalidHostname(t), - clientTLS: &tls.Config{InsecureSkipVerify: false}, - serverConnectionHeader: "Upgrade", - serverUpgradeHeader: "SPDY/3.1", - serverStatusCode: http.StatusSwitchingProtocols, - shouldError: true, - }, - "https (valid hostname + RootCAs)": { - serverFunc: httpsServerValidHostname(t), - 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(t), - clientTLS: &tls.Config{InsecureSkipVerify: true}, - serverConnectionHeader: "Upgrade", - serverUpgradeHeader: "SPDY/3.1", - serverStatusCode: http.StatusSwitchingProtocols, - shouldError: false, - }, - "proxied https with auth (invalid hostname + InsecureSkipVerify) -> http": { - serverFunc: httptest.NewServer, - proxyServerFunc: httpsServerInvalidHostname(t), - proxyAuth: url.UserPassword("proxyuser", "proxypasswd"), - 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(t), - 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(t), - clientTLS: &tls.Config{RootCAs: localhostPool}, - serverConnectionHeader: "Upgrade", - serverUpgradeHeader: "SPDY/3.1", - serverStatusCode: http.StatusSwitchingProtocols, - shouldError: false, - }, - "proxied https with auth (valid hostname + RootCAs) -> http": { - serverFunc: httptest.NewServer, - proxyServerFunc: httpsServerValidHostname(t), - proxyAuth: url.UserPassword("proxyuser", "proxypasswd"), - 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(t), - proxyServerFunc: httpsServerInvalidHostname(t), - 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 with auth (invalid hostname + InsecureSkipVerify) -> https (invalid hostname)": { - serverFunc: httpsServerInvalidHostname(t), - proxyServerFunc: httpsServerInvalidHostname(t), - proxyAuth: url.UserPassword("proxyuser", "proxypasswd"), - 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(t), - proxyServerFunc: httpsServerInvalidHostname(t), - 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(t), - proxyServerFunc: httpsServerValidHostname(t), - clientTLS: &tls.Config{RootCAs: localhostPool}, - serverConnectionHeader: "Upgrade", - serverUpgradeHeader: "SPDY/3.1", - serverStatusCode: http.StatusSwitchingProtocols, - shouldError: false, - }, - "proxied https with auth (valid hostname + RootCAs) -> https (valid hostname + RootCAs)": { - serverFunc: httpsServerValidHostname(t), - proxyServerFunc: httpsServerValidHostname(t), - proxyAuth: url.UserPassword("proxyuser", "proxypasswd"), - clientTLS: &tls.Config{RootCAs: localhostPool}, - serverConnectionHeader: "Upgrade", - serverUpgradeHeader: "SPDY/3.1", - serverStatusCode: http.StatusSwitchingProtocols, - shouldError: false, + testCases := map[string]struct { + serverFunc func(http.Handler) *httptest.Server + proxyServerFunc func(http.Handler) *httptest.Server + proxyAuth *url.Userinfo + clientTLS *tls.Config + serverConnectionHeader string + serverUpgradeHeader string + serverStatusCode int + shouldError bool + }{ + "no headers": { + serverFunc: httptest.NewServer, + serverConnectionHeader: "", + serverUpgradeHeader: "", + serverStatusCode: http.StatusSwitchingProtocols, + shouldError: true, + }, + "no upgrade header": { + serverFunc: httptest.NewServer, + serverConnectionHeader: "Upgrade", + serverUpgradeHeader: "", + serverStatusCode: http.StatusSwitchingProtocols, + shouldError: true, + }, + "no connection header": { + serverFunc: httptest.NewServer, + serverConnectionHeader: "", + serverUpgradeHeader: "SPDY/3.1", + serverStatusCode: http.StatusSwitchingProtocols, + shouldError: true, + }, + "no switching protocol status code": { + serverFunc: httptest.NewServer, + serverConnectionHeader: "Upgrade", + serverUpgradeHeader: "SPDY/3.1", + serverStatusCode: http.StatusForbidden, + shouldError: true, + }, + "http": { + serverFunc: httptest.NewServer, + serverConnectionHeader: "Upgrade", + serverUpgradeHeader: "SPDY/3.1", + serverStatusCode: http.StatusSwitchingProtocols, + shouldError: false, + }, + "https (invalid hostname + InsecureSkipVerify)": { + serverFunc: httpsServerInvalidHostname(t), + clientTLS: &tls.Config{InsecureSkipVerify: true}, + serverConnectionHeader: "Upgrade", + serverUpgradeHeader: "SPDY/3.1", + serverStatusCode: http.StatusSwitchingProtocols, + shouldError: false, + }, + "https (invalid hostname + hostname verification)": { + serverFunc: httpsServerInvalidHostname(t), + clientTLS: &tls.Config{InsecureSkipVerify: false}, + serverConnectionHeader: "Upgrade", + serverUpgradeHeader: "SPDY/3.1", + serverStatusCode: http.StatusSwitchingProtocols, + shouldError: true, + }, + "https (valid hostname + RootCAs)": { + serverFunc: httpsServerValidHostname(t), + 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(t), + clientTLS: &tls.Config{InsecureSkipVerify: true}, + serverConnectionHeader: "Upgrade", + serverUpgradeHeader: "SPDY/3.1", + serverStatusCode: http.StatusSwitchingProtocols, + shouldError: false, + }, + "proxied https with auth (invalid hostname + InsecureSkipVerify) -> http": { + serverFunc: httptest.NewServer, + proxyServerFunc: httpsServerInvalidHostname(t), + proxyAuth: url.UserPassword("proxyuser", "proxypasswd"), + 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(t), + 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(t), + clientTLS: &tls.Config{RootCAs: localhostPool}, + serverConnectionHeader: "Upgrade", + serverUpgradeHeader: "SPDY/3.1", + serverStatusCode: http.StatusSwitchingProtocols, + shouldError: false, + }, + "proxied https with auth (valid hostname + RootCAs) -> http": { + serverFunc: httptest.NewServer, + proxyServerFunc: httpsServerValidHostname(t), + proxyAuth: url.UserPassword("proxyuser", "proxypasswd"), + 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(t), + proxyServerFunc: httpsServerInvalidHostname(t), + 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 with auth (invalid hostname + InsecureSkipVerify) -> https (invalid hostname)": { + serverFunc: httpsServerInvalidHostname(t), + proxyServerFunc: httpsServerInvalidHostname(t), + proxyAuth: url.UserPassword("proxyuser", "proxypasswd"), + 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(t), + proxyServerFunc: httpsServerInvalidHostname(t), + 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(t), + proxyServerFunc: httpsServerValidHostname(t), + clientTLS: &tls.Config{RootCAs: localhostPool}, + serverConnectionHeader: "Upgrade", + serverUpgradeHeader: "SPDY/3.1", + serverStatusCode: http.StatusSwitchingProtocols, + shouldError: false, + }, + "proxied https with auth (valid hostname + RootCAs) -> https (valid hostname + RootCAs)": { + serverFunc: httpsServerValidHostname(t), + proxyServerFunc: httpsServerValidHostname(t), + proxyAuth: url.UserPassword("proxyuser", "proxypasswd"), + clientTLS: &tls.Config{RootCAs: localhostPool}, + serverConnectionHeader: "Upgrade", + serverUpgradeHeader: "SPDY/3.1", + serverStatusCode: http.StatusSwitchingProtocols, + shouldError: false, + }, + } + + for k, testCase := range testCases { + t.Run(k, func(t *testing.T) { + server := testCase.serverFunc(serverHandler( + t, serverHandlerConfig{ + shouldError: testCase.shouldError, + statusCode: testCase.serverStatusCode, + connectionHeader: testCase.serverConnectionHeader, + upgradeHeader: testCase.serverUpgradeHeader, }, + )) + defer server.Close() + + serverURL, err := url.Parse(server.URL) + if err != nil { + t.Fatalf("error creating request: %s", err) + } + req, err := http.NewRequest("GET", server.URL, nil) + if err != nil { + t.Fatalf("error creating request: %s", err) } - for k, testCase := range testCases { - t.Run(k, func(t *testing.T) { - server := testCase.serverFunc(serverHandler( - t, serverHandlerConfig{ - shouldError: testCase.shouldError, - statusCode: testCase.serverStatusCode, - connectionHeader: testCase.serverConnectionHeader, - upgradeHeader: testCase.serverUpgradeHeader, - }, - )) - defer server.Close() + spdyTransport := NewRoundTripper(testCase.clientTLS) - serverURL, err := url.Parse(server.URL) - if err != nil { - t.Fatalf("error creating request: %s", err) - } - req, err := http.NewRequest("GET", server.URL, nil) - if err != nil { - t.Fatalf("error creating request: %s", err) - } + var proxierCalled bool + var proxyCalledWithHost string + var proxyCalledWithAuth bool + var proxyCalledWithAuthHeader string + if testCase.proxyServerFunc != nil { + proxyHandler := goproxy.NewProxyHttpServer() - spdyTransport := NewRoundTripper(testCase.clientTLS, redirect, redirect) - - var proxierCalled bool - var proxyCalledWithHost string - var proxyCalledWithAuth bool - var proxyCalledWithAuthHeader string - if testCase.proxyServerFunc != nil { - proxyHandler := goproxy.NewProxyHttpServer() - - proxyHandler.OnRequest().HandleConnectFunc(func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) { - proxyCalledWithHost = host - - proxyAuthHeaderName := "Proxy-Authorization" - _, proxyCalledWithAuth = ctx.Req.Header[proxyAuthHeaderName] - proxyCalledWithAuthHeader = ctx.Req.Header.Get(proxyAuthHeaderName) - return goproxy.OkConnect, host - }) - - proxy := testCase.proxyServerFunc(proxyHandler) - - spdyTransport.proxier = func(proxierReq *http.Request) (*url.URL, error) { - proxierCalled = true - proxyURL, err := url.Parse(proxy.URL) - if err != nil { - return nil, err - } - proxyURL.User = testCase.proxyAuth - return proxyURL, nil - } - defer proxy.Close() - } - - client := &http.Client{Transport: spdyTransport} - - resp, err := client.Do(req) - var conn httpstream.Connection - if err == nil { - conn, err = spdyTransport.NewConnection(resp) - } - haveErr := err != nil - if e, a := testCase.shouldError, haveErr; e != a { - t.Fatalf("shouldError=%t, got %t: %v", e, a, err) - } - if testCase.shouldError { - return - } - defer conn.Close() - - if resp.StatusCode != http.StatusSwitchingProtocols { - t.Fatalf("expected http 101 switching protocols, got %d", resp.StatusCode) - } - - stream, err := conn.CreateStream(http.Header{}) - if err != nil { - t.Fatalf("error creating client stream: %s", err) - } - - n, err := stream.Write([]byte("hello")) - if err != nil { - t.Fatalf("error writing to stream: %s", err) - } - if n != 5 { - t.Fatalf("expected to write 5 bytes, but actually wrote %d", n) - } - - b := make([]byte, 5) - n, err = stream.Read(b) - if err != nil { - t.Fatalf("error reading from stream: %s", err) - } - if n != 5 { - t.Fatalf("expected to read 5 bytes, but actually read %d", n) - } - if e, a := "hello", string(b[0:n]); e != a { - t.Fatalf("expected '%s', got '%s'", e, a) - } - - if testCase.proxyServerFunc != nil { - if !proxierCalled { - t.Fatal("expected to use a proxy but proxier in SpdyRoundTripper wasn't called") - } - if proxyCalledWithHost != serverURL.Host { - t.Fatalf("expected to see a call to the proxy for backend %q, got %q", serverURL.Host, proxyCalledWithHost) - } - } - - var expectedProxyAuth string - if testCase.proxyAuth != nil { - encodedCredentials := base64.StdEncoding.EncodeToString([]byte(testCase.proxyAuth.String())) - expectedProxyAuth = "Basic " + encodedCredentials - } - if len(expectedProxyAuth) == 0 && proxyCalledWithAuth { - t.Fatalf("proxy authorization unexpected, got %q", proxyCalledWithAuthHeader) - } - if proxyCalledWithAuthHeader != expectedProxyAuth { - t.Fatalf("expected to see a call to the proxy with credentials %q, got %q", testCase.proxyAuth, proxyCalledWithAuthHeader) - } + proxyHandler.OnRequest().HandleConnectFunc(func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) { + proxyCalledWithHost = host + proxyAuthHeaderName := "Proxy-Authorization" + _, proxyCalledWithAuth = ctx.Req.Header[proxyAuthHeaderName] + proxyCalledWithAuthHeader = ctx.Req.Header.Get(proxyAuthHeaderName) + return goproxy.OkConnect, host }) + + proxy := testCase.proxyServerFunc(proxyHandler) + + spdyTransport.proxier = func(proxierReq *http.Request) (*url.URL, error) { + proxierCalled = true + proxyURL, err := url.Parse(proxy.URL) + if err != nil { + return nil, err + } + proxyURL.User = testCase.proxyAuth + return proxyURL, nil + } + defer proxy.Close() } + + client := &http.Client{Transport: spdyTransport} + + resp, err := client.Do(req) + var conn httpstream.Connection + if err == nil { + conn, err = spdyTransport.NewConnection(resp) + } + haveErr := err != nil + if e, a := testCase.shouldError, haveErr; e != a { + t.Fatalf("shouldError=%t, got %t: %v", e, a, err) + } + if testCase.shouldError { + return + } + defer conn.Close() + + if resp.StatusCode != http.StatusSwitchingProtocols { + t.Fatalf("expected http 101 switching protocols, got %d", resp.StatusCode) + } + + stream, err := conn.CreateStream(http.Header{}) + if err != nil { + t.Fatalf("error creating client stream: %s", err) + } + + n, err := stream.Write([]byte("hello")) + if err != nil { + t.Fatalf("error writing to stream: %s", err) + } + if n != 5 { + t.Fatalf("expected to write 5 bytes, but actually wrote %d", n) + } + + b := make([]byte, 5) + n, err = stream.Read(b) + if err != nil { + t.Fatalf("error reading from stream: %s", err) + } + if n != 5 { + t.Fatalf("expected to read 5 bytes, but actually read %d", n) + } + if e, a := "hello", string(b[0:n]); e != a { + t.Fatalf("expected '%s', got '%s'", e, a) + } + + if testCase.proxyServerFunc != nil { + if !proxierCalled { + t.Fatal("expected to use a proxy but proxier in SpdyRoundTripper wasn't called") + } + if proxyCalledWithHost != serverURL.Host { + t.Fatalf("expected to see a call to the proxy for backend %q, got %q", serverURL.Host, proxyCalledWithHost) + } + } + + var expectedProxyAuth string + if testCase.proxyAuth != nil { + encodedCredentials := base64.StdEncoding.EncodeToString([]byte(testCase.proxyAuth.String())) + expectedProxyAuth = "Basic " + encodedCredentials + } + if len(expectedProxyAuth) == 0 && proxyCalledWithAuth { + t.Fatalf("proxy authorization unexpected, got %q", proxyCalledWithAuthHeader) + } + if proxyCalledWithAuthHeader != expectedProxyAuth { + t.Fatalf("expected to see a call to the proxy with credentials %q, got %q", testCase.proxyAuth, proxyCalledWithAuthHeader) + } + }) } } @@ -440,321 +433,184 @@ func (i *Interceptor) Rewrite(ctx context.Context, req *socks5.Request) (context func TestRoundTripSocks5AndNewConnection(t *testing.T) { localhostPool := localhostCertPool(t) - for _, redirect := range []bool{false, true} { - t.Run(fmt.Sprintf("redirect = %t", redirect), func(t *testing.T) { - socks5Server := func(creds *socks5.StaticCredentials, interceptor *Interceptor) *socks5.Server { - var conf *socks5.Config - if creds != nil { - authenticator := socks5.UserPassAuthenticator{Credentials: creds} - conf = &socks5.Config{ - AuthMethods: []socks5.Authenticator{authenticator}, - Rewriter: interceptor, - } - } else { - conf = &socks5.Config{Rewriter: interceptor} - } - - ts, err := socks5.New(conf) - if err != nil { - t.Errorf("failed to create sock5 server: %v", err) - } - return ts + socks5Server := func(creds *socks5.StaticCredentials, interceptor *Interceptor) *socks5.Server { + var conf *socks5.Config + if creds != nil { + authenticator := socks5.UserPassAuthenticator{Credentials: creds} + conf = &socks5.Config{ + AuthMethods: []socks5.Authenticator{authenticator}, + Rewriter: interceptor, } + } else { + conf = &socks5.Config{Rewriter: interceptor} + } - testCases := map[string]struct { - clientTLS *tls.Config - proxyAuth *url.Userinfo - serverConnectionHeader string - serverFunc serverFunc - serverStatusCode int - serverUpgradeHeader string - shouldError bool - }{ - "proxied without auth -> http": { - serverFunc: httptest.NewServer, - serverConnectionHeader: "Upgrade", - serverStatusCode: http.StatusSwitchingProtocols, - serverUpgradeHeader: "SPDY/3.1", - shouldError: false, - }, - "proxied with invalid auth -> http": { - serverFunc: httptest.NewServer, - proxyAuth: url.UserPassword("invalid", "auth"), - serverConnectionHeader: "Upgrade", - serverStatusCode: http.StatusSwitchingProtocols, - serverUpgradeHeader: "SPDY/3.1", - shouldError: true, - }, - "proxied with valid auth -> http": { - serverFunc: httptest.NewServer, - proxyAuth: url.UserPassword("proxyuser", "proxypasswd"), - serverConnectionHeader: "Upgrade", - serverStatusCode: http.StatusSwitchingProtocols, - serverUpgradeHeader: "SPDY/3.1", - shouldError: false, - }, - "proxied with valid auth -> https (invalid hostname + InsecureSkipVerify)": { - serverFunc: httpsServerInvalidHostname(t), - proxyAuth: url.UserPassword("proxyuser", "proxypasswd"), - clientTLS: &tls.Config{InsecureSkipVerify: true}, - serverConnectionHeader: "Upgrade", - serverUpgradeHeader: "SPDY/3.1", - serverStatusCode: http.StatusSwitchingProtocols, - shouldError: false, - }, - "proxied with valid auth -> https (invalid hostname + hostname verification)": { - serverFunc: httpsServerInvalidHostname(t), - proxyAuth: url.UserPassword("proxyuser", "proxypasswd"), - clientTLS: &tls.Config{InsecureSkipVerify: false}, - serverConnectionHeader: "Upgrade", - serverUpgradeHeader: "SPDY/3.1", - serverStatusCode: http.StatusSwitchingProtocols, - shouldError: true, - }, - "proxied with valid auth -> https (valid hostname + RootCAs)": { - serverFunc: httpsServerValidHostname(t), - proxyAuth: url.UserPassword("proxyuser", "proxypasswd"), - clientTLS: &tls.Config{RootCAs: localhostPool}, - serverConnectionHeader: "Upgrade", - serverUpgradeHeader: "SPDY/3.1", - serverStatusCode: http.StatusSwitchingProtocols, - shouldError: false, - }, - } - - for name, testCase := range testCases { - t.Run(name, func(t *testing.T) { - server := testCase.serverFunc(serverHandler( - t, serverHandlerConfig{ - shouldError: testCase.shouldError, - statusCode: testCase.serverStatusCode, - connectionHeader: testCase.serverConnectionHeader, - upgradeHeader: testCase.serverUpgradeHeader, - }, - )) - defer server.Close() - - req, err := http.NewRequest("GET", server.URL, nil) - if err != nil { - t.Fatalf("error creating request: %s", err) - } - - spdyTransport := NewRoundTripper(testCase.clientTLS, redirect, redirect) - var proxierCalled bool - var proxyCalledWithHost string - - interceptor := &Interceptor{proxyCalledWithHost: &proxyCalledWithHost} - - proxyHandler := socks5Server(nil, interceptor) - - if testCase.proxyAuth != nil { - proxyHandler = socks5Server(&socks5.StaticCredentials{ - "proxyuser": "proxypasswd", // Socks5 server static credentials when client authentication is expected - }, interceptor) - } - - closed := make(chan struct{}) - isClosed := func() bool { - select { - case <-closed: - return true - default: - return false - } - } - - l, err := net.Listen("tcp", "127.0.0.1:0") - if err != nil { - t.Fatalf("socks5Server: proxy_test: Listen: %v", err) - } - defer l.Close() - - go func(shoulderror bool) { - conn, err := l.Accept() - if err != nil { - if isClosed() { - return - } - - t.Errorf("error accepting connection: %s", err) - } - - if err := proxyHandler.ServeConn(conn); err != nil && !shoulderror { - // If the connection request is closed before the channel is closed - // the test will fail with a ServeConn error. Since the test only return - // early if expects shouldError=true, the channel is closed at the end of - // the test, just before all the deferred connections Close() are executed. - if isClosed() { - return - } - - t.Errorf("ServeConn error: %s", err) - } - }(testCase.shouldError) - spdyTransport.proxier = func(proxierReq *http.Request) (*url.URL, error) { - proxierCalled = true - return &url.URL{ - Scheme: "socks5", - Host: net.JoinHostPort("127.0.0.1", strconv.Itoa(l.Addr().(*net.TCPAddr).Port)), - User: testCase.proxyAuth, - }, nil - } - - client := &http.Client{Transport: spdyTransport} - - resp, err := client.Do(req) - haveErr := err != nil - if e, a := testCase.shouldError, haveErr; e != a { - t.Fatalf("shouldError=%t, got %t: %v", e, a, err) - } - if testCase.shouldError { - return - } - - conn, err := spdyTransport.NewConnection(resp) - haveErr = err != nil - if e, a := testCase.shouldError, haveErr; e != a { - t.Fatalf("shouldError=%t, got %t: %v", e, a, err) - } - if testCase.shouldError { - return - } - - defer conn.Close() - - if resp.StatusCode != http.StatusSwitchingProtocols { - t.Fatalf("expected http 101 switching protocols, got %d", resp.StatusCode) - } - - stream, err := conn.CreateStream(http.Header{}) - if err != nil { - t.Fatalf("error creating client stream: %s", err) - } - - n, err := stream.Write([]byte("hello")) - if err != nil { - t.Fatalf("error writing to stream: %s", err) - } - if n != 5 { - t.Fatalf("expected to write 5 bytes, but actually wrote %d", n) - } - - b := make([]byte, 5) - n, err = stream.Read(b) - if err != nil { - t.Fatalf("error reading from stream: %s", err) - } - if n != 5 { - t.Fatalf("expected to read 5 bytes, but actually read %d", n) - } - if e, a := "hello", string(b[0:n]); e != a { - t.Fatalf("expected '%s', got '%s'", e, a) - } - - if !proxierCalled { - t.Fatal("xpected to use a proxy but proxier in SpdyRoundTripper wasn't called") - } - - serverURL, err := url.Parse(server.URL) - if err != nil { - t.Fatalf("error creating request: %s", err) - } - if proxyCalledWithHost != serverURL.Host { - t.Fatalf("expected to see a call to the proxy for backend %q, got %q", serverURL.Host, proxyCalledWithHost) - } - - authMethod, authUser := interceptor.GetAuthContext() - - if testCase.proxyAuth != nil { - expectedSocks5AuthMethod := 2 - expectedSocks5AuthUser := "proxyuser" - - if expectedSocks5AuthMethod != authMethod { - t.Fatalf("socks5 Proxy authorization unexpected, got %d, expected %d", authMethod, expectedSocks5AuthMethod) - } - - if expectedSocks5AuthUser != authUser["Username"] { - t.Fatalf("socks5 Proxy authorization user unexpected, got %q, expected %q", authUser["Username"], expectedSocks5AuthUser) - } - } else { - if authMethod != 0 { - t.Fatalf("proxy authentication method unexpected, got %d", authMethod) - } - if len(authUser) != 0 { - t.Fatalf("unexpected proxy user: %v", authUser) - } - } - - // The channel must be closed before any of the connections are closed - close(closed) - }) - } - }) + ts, err := socks5.New(conf) + if err != nil { + t.Errorf("failed to create sock5 server: %v", err) + } + return ts } -} -func TestRoundTripRedirects(t *testing.T) { - tests := []struct { - redirects int32 - expectSuccess bool + testCases := map[string]struct { + clientTLS *tls.Config + proxyAuth *url.Userinfo + serverConnectionHeader string + serverFunc serverFunc + serverStatusCode int + serverUpgradeHeader string + shouldError bool }{ - {0, true}, - {1, true}, - {9, true}, - {10, false}, + "proxied without auth -> http": { + serverFunc: httptest.NewServer, + serverConnectionHeader: "Upgrade", + serverStatusCode: http.StatusSwitchingProtocols, + serverUpgradeHeader: "SPDY/3.1", + shouldError: false, + }, + "proxied with invalid auth -> http": { + serverFunc: httptest.NewServer, + proxyAuth: url.UserPassword("invalid", "auth"), + serverConnectionHeader: "Upgrade", + serverStatusCode: http.StatusSwitchingProtocols, + serverUpgradeHeader: "SPDY/3.1", + shouldError: true, + }, + "proxied with valid auth -> http": { + serverFunc: httptest.NewServer, + proxyAuth: url.UserPassword("proxyuser", "proxypasswd"), + serverConnectionHeader: "Upgrade", + serverStatusCode: http.StatusSwitchingProtocols, + serverUpgradeHeader: "SPDY/3.1", + shouldError: false, + }, + "proxied with valid auth -> https (invalid hostname + InsecureSkipVerify)": { + serverFunc: httpsServerInvalidHostname(t), + proxyAuth: url.UserPassword("proxyuser", "proxypasswd"), + clientTLS: &tls.Config{InsecureSkipVerify: true}, + serverConnectionHeader: "Upgrade", + serverUpgradeHeader: "SPDY/3.1", + serverStatusCode: http.StatusSwitchingProtocols, + shouldError: false, + }, + "proxied with valid auth -> https (invalid hostname + hostname verification)": { + serverFunc: httpsServerInvalidHostname(t), + proxyAuth: url.UserPassword("proxyuser", "proxypasswd"), + clientTLS: &tls.Config{InsecureSkipVerify: false}, + serverConnectionHeader: "Upgrade", + serverUpgradeHeader: "SPDY/3.1", + serverStatusCode: http.StatusSwitchingProtocols, + shouldError: true, + }, + "proxied with valid auth -> https (valid hostname + RootCAs)": { + serverFunc: httpsServerValidHostname(t), + proxyAuth: url.UserPassword("proxyuser", "proxypasswd"), + clientTLS: &tls.Config{RootCAs: localhostPool}, + serverConnectionHeader: "Upgrade", + serverUpgradeHeader: "SPDY/3.1", + serverStatusCode: http.StatusSwitchingProtocols, + shouldError: false, + }, } - for _, test := range tests { - t.Run(fmt.Sprintf("with %d redirects", test.redirects), func(t *testing.T) { - var redirects int32 = 0 - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - if redirects < test.redirects { - atomic.AddInt32(&redirects, 1) - http.Redirect(w, req, "redirect", http.StatusFound) - return - } - streamCh := make(chan httpstream.Stream) - responseUpgrader := NewResponseUpgrader() - spdyConn := responseUpgrader.UpgradeResponse(w, req, func(s httpstream.Stream, replySent <-chan struct{}) error { - streamCh <- s - return nil - }) - if spdyConn == nil { - t.Fatalf("unexpected nil spdyConn") - } - defer spdyConn.Close() - - stream := <-streamCh - io.Copy(stream, stream) - })) + for name, testCase := range testCases { + t.Run(name, func(t *testing.T) { + server := testCase.serverFunc(serverHandler( + t, serverHandlerConfig{ + shouldError: testCase.shouldError, + statusCode: testCase.serverStatusCode, + connectionHeader: testCase.serverConnectionHeader, + upgradeHeader: testCase.serverUpgradeHeader, + }, + )) defer server.Close() req, err := http.NewRequest("GET", server.URL, nil) if err != nil { - t.Fatalf("Error creating request: %s", err) + t.Fatalf("error creating request: %s", err) + } + + spdyTransport := NewRoundTripper(testCase.clientTLS) + var proxierCalled bool + var proxyCalledWithHost string + + interceptor := &Interceptor{proxyCalledWithHost: &proxyCalledWithHost} + + proxyHandler := socks5Server(nil, interceptor) + + if testCase.proxyAuth != nil { + proxyHandler = socks5Server(&socks5.StaticCredentials{ + "proxyuser": "proxypasswd", // Socks5 server static credentials when client authentication is expected + }, interceptor) + } + + closed := make(chan struct{}) + isClosed := func() bool { + select { + case <-closed: + return true + default: + return false + } + } + + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("socks5Server: proxy_test: Listen: %v", err) + } + defer l.Close() + + go func(shoulderror bool) { + conn, err := l.Accept() + if err != nil { + if isClosed() { + return + } + + t.Errorf("error accepting connection: %s", err) + } + + if err := proxyHandler.ServeConn(conn); err != nil && !shoulderror { + // If the connection request is closed before the channel is closed + // the test will fail with a ServeConn error. Since the test only return + // early if expects shouldError=true, the channel is closed at the end of + // the test, just before all the deferred connections Close() are executed. + if isClosed() { + return + } + + t.Errorf("ServeConn error: %s", err) + } + }(testCase.shouldError) + spdyTransport.proxier = func(proxierReq *http.Request) (*url.URL, error) { + proxierCalled = true + return &url.URL{ + Scheme: "socks5", + Host: net.JoinHostPort("127.0.0.1", strconv.Itoa(l.Addr().(*net.TCPAddr).Port)), + User: testCase.proxyAuth, + }, nil } - spdyTransport := NewRoundTripper(nil, true, true) client := &http.Client{Transport: spdyTransport} resp, err := client.Do(req) - if test.expectSuccess { - if err != nil { - t.Fatalf("error calling Do: %v", err) - } - } else { - if err == nil { - t.Fatalf("expecting an error") - } else if !strings.Contains(err.Error(), "too many redirects") { - t.Fatalf("expecting too many redirects, got %v", err) - } + haveErr := err != nil + if e, a := testCase.shouldError, haveErr; e != a { + t.Fatalf("shouldError=%t, got %t: %v", e, a, err) + } + if testCase.shouldError { return } conn, err := spdyTransport.NewConnection(resp) - if err != nil { - t.Fatalf("error calling NewConnection: %v", err) + haveErr = err != nil + if e, a := testCase.shouldError, haveErr; e != a { + t.Fatalf("shouldError=%t, got %t: %v", e, a, err) } + if testCase.shouldError { + return + } + defer conn.Close() if resp.StatusCode != http.StatusSwitchingProtocols { @@ -771,7 +627,7 @@ func TestRoundTripRedirects(t *testing.T) { t.Fatalf("error writing to stream: %s", err) } if n != 5 { - t.Fatalf("Expected to write 5 bytes, but actually wrote %d", n) + t.Fatalf("expected to write 5 bytes, but actually wrote %d", n) } b := make([]byte, 5) @@ -780,11 +636,48 @@ func TestRoundTripRedirects(t *testing.T) { t.Fatalf("error reading from stream: %s", err) } if n != 5 { - t.Fatalf("Expected to read 5 bytes, but actually read %d", n) + t.Fatalf("expected to read 5 bytes, but actually read %d", n) } if e, a := "hello", string(b[0:n]); e != a { t.Fatalf("expected '%s', got '%s'", e, a) } + + if !proxierCalled { + t.Fatal("xpected to use a proxy but proxier in SpdyRoundTripper wasn't called") + } + + serverURL, err := url.Parse(server.URL) + if err != nil { + t.Fatalf("error creating request: %s", err) + } + if proxyCalledWithHost != serverURL.Host { + t.Fatalf("expected to see a call to the proxy for backend %q, got %q", serverURL.Host, proxyCalledWithHost) + } + + authMethod, authUser := interceptor.GetAuthContext() + + if testCase.proxyAuth != nil { + expectedSocks5AuthMethod := 2 + expectedSocks5AuthUser := "proxyuser" + + if expectedSocks5AuthMethod != authMethod { + t.Fatalf("socks5 Proxy authorization unexpected, got %d, expected %d", authMethod, expectedSocks5AuthMethod) + } + + if expectedSocks5AuthUser != authUser["Username"] { + t.Fatalf("socks5 Proxy authorization user unexpected, got %q, expected %q", authUser["Username"], expectedSocks5AuthUser) + } + } else { + if authMethod != 0 { + t.Fatalf("proxy authentication method unexpected, got %d", authMethod) + } + if len(authUser) != 0 { + t.Fatalf("unexpected proxy user: %v", authUser) + } + } + + // The channel must be closed before any of the connections are closed + close(closed) }) } } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/net/http.go b/staging/src/k8s.io/apimachinery/pkg/util/net/http.go index 04432b175d6..8cc1810af13 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/net/http.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/net/http.go @@ -17,7 +17,6 @@ limitations under the License. package net import ( - "bufio" "bytes" "context" "crypto/tls" @@ -446,104 +445,6 @@ type Dialer interface { Dial(req *http.Request) (net.Conn, error) } -// ConnectWithRedirects uses dialer to send req, following up to 10 redirects (relative to -// originalLocation). It returns the opened net.Conn and the raw response bytes. -// If requireSameHostRedirects is true, only redirects to the same host are permitted. -func ConnectWithRedirects(originalMethod string, originalLocation *url.URL, header http.Header, originalBody io.Reader, dialer Dialer, requireSameHostRedirects bool) (net.Conn, []byte, error) { - const ( - maxRedirects = 9 // Fail on the 10th redirect - maxResponseSize = 16384 // play it safe to allow the potential for lots of / large headers - ) - - var ( - location = originalLocation - method = originalMethod - intermediateConn net.Conn - rawResponse = bytes.NewBuffer(make([]byte, 0, 256)) - body = originalBody - ) - - defer func() { - if intermediateConn != nil { - intermediateConn.Close() - } - }() - -redirectLoop: - for redirects := 0; ; redirects++ { - if redirects > maxRedirects { - return nil, nil, fmt.Errorf("too many redirects (%d)", redirects) - } - - req, err := http.NewRequest(method, location.String(), body) - if err != nil { - return nil, nil, err - } - - req.Header = header - - intermediateConn, err = dialer.Dial(req) - if err != nil { - return nil, nil, err - } - - // Peek at the backend response. - rawResponse.Reset() - respReader := bufio.NewReader(io.TeeReader( - io.LimitReader(intermediateConn, maxResponseSize), // Don't read more than maxResponseSize bytes. - rawResponse)) // Save the raw response. - resp, err := http.ReadResponse(respReader, nil) - if err != nil { - // Unable to read the backend response; let the client handle it. - klog.Warningf("Error reading backend response: %v", err) - break redirectLoop - } - - switch resp.StatusCode { - case http.StatusFound: - // Redirect, continue. - default: - // Don't redirect. - break redirectLoop - } - - // Redirected requests switch to "GET" according to the HTTP spec: - // https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.3 - method = "GET" - // don't send a body when following redirects - body = nil - - resp.Body.Close() // not used - - // Prepare to follow the redirect. - redirectStr := resp.Header.Get("Location") - if redirectStr == "" { - return nil, nil, fmt.Errorf("%d response missing Location header", resp.StatusCode) - } - // We have to parse relative to the current location, NOT originalLocation. For example, - // if we request http://foo.com/a and get back "http://bar.com/b", the result should be - // http://bar.com/b. If we then make that request and get back a redirect to "/c", the result - // should be http://bar.com/c, not http://foo.com/c. - location, err = location.Parse(redirectStr) - if err != nil { - return nil, nil, fmt.Errorf("malformed Location header: %v", err) - } - - // Only follow redirects to the same host. Otherwise, propagate the redirect response back. - if requireSameHostRedirects && location.Hostname() != originalLocation.Hostname() { - return nil, nil, fmt.Errorf("hostname mismatch: expected %s, found %s", originalLocation.Hostname(), location.Hostname()) - } - - // Reset the connection. - intermediateConn.Close() - intermediateConn = nil - } - - connToReturn := intermediateConn - intermediateConn = nil // Don't close the connection when we return it. - return connToReturn, rawResponse.Bytes(), nil -} - // CloneRequest creates a shallow copy of the request along with a deep copy of the Headers. func CloneRequest(req *http.Request) *http.Request { r := new(http.Request) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/net/http_test.go b/staging/src/k8s.io/apimachinery/pkg/util/net/http_test.go index c3f4d4db06a..5d3588070d1 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/net/http_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/net/http_test.go @@ -20,15 +20,11 @@ limitations under the License. package net import ( - "bufio" - "bytes" "crypto/tls" "fmt" "io" - "io/ioutil" "net" "net/http" - "net/http/httptest" "net/url" "os" "reflect" @@ -36,8 +32,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "k8s.io/apimachinery/pkg/util/wait" netutils "k8s.io/utils/net" ) @@ -293,157 +287,6 @@ func TestJoinPreservingTrailingSlash(t *testing.T) { } } -func TestConnectWithRedirects(t *testing.T) { - tests := []struct { - desc string - redirects []string - method string // initial request method, empty == GET - expectError bool - expectedRedirects int - newPort bool // special case different port test - }{{ - desc: "relative redirects allowed", - redirects: []string{"/ok"}, - expectedRedirects: 1, - }, { - desc: "redirects to the same host are allowed", - redirects: []string{"http://HOST/ok"}, // HOST replaced with server address in test - expectedRedirects: 1, - }, { - desc: "POST redirects to GET", - method: http.MethodPost, - redirects: []string{"/ok"}, - expectedRedirects: 1, - }, { - desc: "PUT redirects to GET", - method: http.MethodPut, - redirects: []string{"/ok"}, - expectedRedirects: 1, - }, { - desc: "DELETE redirects to GET", - method: http.MethodDelete, - redirects: []string{"/ok"}, - expectedRedirects: 1, - }, { - desc: "9 redirects are allowed", - redirects: []string{"/1", "/2", "/3", "/4", "/5", "/6", "/7", "/8", "/9"}, - expectedRedirects: 9, - }, { - desc: "10 redirects are forbidden", - 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"}, - expectError: true, - }, { - 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"}, - expectedRedirects: 1, - newPort: true, - }} - - const resultString = "Test output" - for _, test := range tests { - t.Run(test.desc, func(t *testing.T) { - redirectCount := 0 - s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - // Verify redirect request. - if redirectCount > 0 { - expectedURL, err := url.Parse(test.redirects[redirectCount-1]) - require.NoError(t, err, "test URL error") - assert.Equal(t, req.URL.Path, expectedURL.Path, "unknown redirect path") - assert.Equal(t, http.MethodGet, req.Method, "redirects must always be GET") - } - if redirectCount < len(test.redirects) { - http.Redirect(w, req, test.redirects[redirectCount], http.StatusFound) - redirectCount++ - } else if redirectCount == len(test.redirects) { - w.Write([]byte(resultString)) - } else { - t.Errorf("unexpected number of redirects %d to %s", redirectCount, req.URL.String()) - } - })) - defer s.Close() - - u, err := url.Parse(s.URL) - require.NoError(t, err, "Error parsing server URL") - host := u.Host - - // Special case new-port test with a secondary server. - if test.newPort { - s2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - w.Write([]byte(resultString)) - })) - defer s2.Close() - u2, err := url.Parse(s2.URL) - require.NoError(t, err, "Error parsing secondary server URL") - - // Sanity check: secondary server uses same hostname, different port. - require.Equal(t, u.Hostname(), u2.Hostname(), "sanity check: same hostname") - require.NotEqual(t, u.Port(), u2.Port(), "sanity check: different port") - - // Redirect to the secondary server. - host = u2.Host - - } - - // Update redirect URLs with actual host. - for i := range test.redirects { - test.redirects[i] = strings.Replace(test.redirects[i], "HOST", host, 1) - } - - method := test.method - if method == "" { - method = http.MethodGet - } - - netdialer := &net.Dialer{ - Timeout: wait.ForeverTestTimeout, - KeepAlive: wait.ForeverTestTimeout, - } - dialer := DialerFunc(func(req *http.Request) (net.Conn, error) { - conn, err := netdialer.Dial("tcp", req.URL.Host) - if err != nil { - return conn, err - } - if err = req.Write(conn); err != nil { - require.NoError(t, conn.Close()) - return nil, fmt.Errorf("error sending request: %v", err) - } - return conn, err - }) - conn, rawResponse, err := ConnectWithRedirects(method, u, http.Header{} /*body*/, nil, dialer, true) - if test.expectError { - require.Error(t, err, "expected request error") - return - } - - require.NoError(t, err, "unexpected request error") - assert.NoError(t, conn.Close(), "error closing connection") - - resp, err := http.ReadResponse(bufio.NewReader(bytes.NewReader(rawResponse)), nil) - require.NoError(t, err, "unexpected request error") - - result, err := ioutil.ReadAll(resp.Body) - assert.NoError(t, err) - require.NoError(t, resp.Body.Close()) - if test.expectedRedirects < len(test.redirects) { - // Expect the last redirect to be returned. - assert.Equal(t, http.StatusFound, resp.StatusCode, "Final response is not a redirect") - assert.Equal(t, test.redirects[len(test.redirects)-1], resp.Header.Get("Location")) - assert.NotEqual(t, resultString, string(result), "wrong content") - } else { - assert.Equal(t, resultString, string(result), "stream content does not match") - } - }) - } -} - func TestAllowsHTTP2(t *testing.T) { testcases := []struct { Name string diff --git a/staging/src/k8s.io/client-go/transport/spdy/spdy.go b/staging/src/k8s.io/client-go/transport/spdy/spdy.go index 406d3cc19ce..f50b68e5ffb 100644 --- a/staging/src/k8s.io/client-go/transport/spdy/spdy.go +++ b/staging/src/k8s.io/client-go/transport/spdy/spdy.go @@ -44,11 +44,9 @@ func RoundTripperFor(config *restclient.Config) (http.RoundTripper, Upgrader, er proxy = config.Proxy } upgradeRoundTripper := spdy.NewRoundTripperWithConfig(spdy.RoundTripperConfig{ - TLS: tlsConfig, - FollowRedirects: true, - RequireSameHostRedirects: false, - Proxier: proxy, - PingPeriod: time.Second * 5, + TLS: tlsConfig, + Proxier: proxy, + PingPeriod: time.Second * 5, }) wrapper, err := restclient.HTTPWrappersForConfig(config, upgradeRoundTripper) if err != nil {