From 0c74b9e00daf04d6d23a8c20d2cd8f19859f0db8 Mon Sep 17 00:00:00 2001 From: Vasili Revelas Date: Wed, 2 Nov 2022 21:21:23 +0200 Subject: [PATCH 1/2] Improve error message when proxy connection fails If the proxy doesn't respond to our CONNECT request with a 2XX status code, we now display the proxy's response rather than the confusing error "tls: first record does not look like a TLS handshake" --- .../apimachinery/pkg/util/httpstream/spdy/roundtripper.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 ea0481799b7..858eafb86f9 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 @@ -184,12 +184,15 @@ func (s *SpdyRoundTripper) dialWithHttpProxy(req *http.Request, proxyURL *url.UR //nolint:staticcheck // SA1019 ignore deprecated httputil.NewProxyClientConn proxyClientConn := httputil.NewProxyClientConn(proxyDialConn, nil) - _, err = proxyClientConn.Do(&proxyReq) + response, err := proxyClientConn.Do(&proxyReq) //nolint:staticcheck // SA1019 ignore deprecated httputil.ErrPersistEOF: it might be // returned from the invocation of proxyClientConn.Do if err != nil && err != httputil.ErrPersistEOF { return nil, err } + if response != nil && response.StatusCode >= 300 || response.StatusCode < 200 { + return nil, fmt.Errorf("CONNECT request to %s returned response: %s", proxyURL.Redacted(), response.Status) + } rwc, _ := proxyClientConn.Hijack() From 23d32cf1bc0f33d0ef2f2332139a2768bb3c99f8 Mon Sep 17 00:00:00 2001 From: Vasili Revelas Date: Wed, 2 Nov 2022 21:45:29 +0200 Subject: [PATCH 2/2] Fix SPDY proxy authentication with special chars The username and password sent in the Proxy-Authorization header are not supposed to be percent escaped prior to being base64 encoded. --- .../pkg/util/httpstream/spdy/roundtripper.go | 7 +++-- .../util/httpstream/spdy/roundtripper_test.go | 30 ++++++++++++------- 2 files changed, 24 insertions(+), 13 deletions(-) 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 858eafb86f9..27c3d2d5645 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 @@ -297,9 +297,10 @@ func (s *SpdyRoundTripper) proxyAuth(proxyURL *url.URL) string { if proxyURL == nil || proxyURL.User == nil { return "" } - credentials := proxyURL.User.String() - encodedAuth := base64.StdEncoding.EncodeToString([]byte(credentials)) - return fmt.Sprintf("Basic %s", encodedAuth) + username := proxyURL.User.Username() + password, _ := proxyURL.User.Password() + auth := username + ":" + password + return "Basic " + base64.StdEncoding.EncodeToString([]byte(auth)) } // RoundTrip executes the Request and upgrades it. After a successful upgrade, 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 10b329594fd..a0b7b169dd5 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 @@ -20,7 +20,6 @@ import ( "context" "crypto/tls" "crypto/x509" - "encoding/base64" "io" "net" "net/http" @@ -291,6 +290,16 @@ func TestRoundTripAndNewConnection(t *testing.T) { serverStatusCode: http.StatusSwitchingProtocols, shouldError: false, }, + "proxied valid https, proxy auth with chars that percent escape -> valid https": { + serverFunc: httpsServerValidHostname(t), + proxyServerFunc: httpsServerValidHostname(t), + proxyAuth: url.UserPassword("proxy user", "proxypasswd%"), + clientTLS: &tls.Config{RootCAs: localhostPool}, + serverConnectionHeader: "Upgrade", + serverUpgradeHeader: "SPDY/3.1", + serverStatusCode: http.StatusSwitchingProtocols, + shouldError: false, + }, } for k, testCase := range testCases { @@ -400,18 +409,19 @@ func TestRoundTripAndNewConnection(t *testing.T) { } } - var expectedProxyAuth string if testCase.proxyAuth != nil { - encodedCredentials := base64.StdEncoding.EncodeToString([]byte(testCase.proxyAuth.String())) - expectedProxyAuth = "Basic " + encodedCredentials - } - if len(expectedProxyAuth) == 0 && proxyCalledWithAuth { + expectedUsername := testCase.proxyAuth.Username() + expectedPassword, _ := testCase.proxyAuth.Password() + username, password, ok := (&http.Request{Header: http.Header{"Authorization": []string{proxyCalledWithAuthHeader}}}).BasicAuth() + if !ok { + t.Fatalf("invalid proxy auth header %s", proxyCalledWithAuthHeader) + } + if username != expectedUsername || password != expectedPassword { + t.Fatalf("expected proxy auth \"%s:%s\", got \"%s:%s\"", expectedUsername, expectedPassword, username, password) + } + } else if 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) - } - }) } }