internal/concierge/impersonator: don't mutate ServeHTTP() req

I added that test helper to create an http.Request since I wanted to properly
initialize the http.Request's context.

Signed-off-by: Andrew Keesler <akeesler@vmware.com>

internal/concierge/impersonator/impersonator.go

@@ -105,11 +105,12 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 	log = log.WithValues("userID", userInfo.GetUID())
 
-	newHeaders := getProxyHeaders(userInfo, r.Header)
+	// Never mutate request (see http.Handler docs).
-	r.Header = newHeaders
+	newR := r.WithContext(r.Context())
+	newR.Header = getProxyHeaders(userInfo, r.Header)
 
 	log.Info("proxying authenticated request")
-	p.proxy.ServeHTTP(w, r)
+	p.proxy.ServeHTTP(w, newR)
 }
 
 func getProxyHeaders(userInfo user.Info, requestHeaders http.Header) http.Header {

internal/concierge/impersonator/impersonator_test.go

@@ -4,6 +4,7 @@
 package impersonator
 
 import (
+	"context"
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
@@ -53,6 +54,12 @@ func TestImpersonator(t *testing.T) {
 		BearerToken:     "some-service-account-token",
 		TLSClientConfig: rest.TLSClientConfig{CAData: []byte(testServerCA)},
 	}
+	newRequest := func(h http.Header) *http.Request {
+		r, err := http.NewRequestWithContext(context.Background(), http.MethodGet, validURL.String(), nil)
+		require.NoError(t, err)
+		r.Header = h
+		return r
+	}
 
 	tests := []struct {
 		name            string
@@ -102,61 +109,41 @@ func TestImpersonator(t *testing.T) {
 			wantCreationErr: "could not get in-cluster transport: using a custom transport with TLS certificate options or the insecure flag is not allowed",
 		},
 		{
-			name:          "missing authorization header",
+			name:           "missing authorization header",
-			getKubeconfig: func() (*rest.Config, error) { return &testServerKubeconfig, nil },
+			getKubeconfig:  func() (*rest.Config, error) { return &testServerKubeconfig, nil },
-			request: &http.Request{
+			request:        newRequest(map[string][]string{}),
-				Method: "GET",
-				Header: map[string][]string{},
-				URL:    validURL,
-			},
 			wantHTTPBody:   "invalid token encoding\n",
 			wantHTTPStatus: http.StatusBadRequest,
 			wantLogs:       []string{"\"error\"=\"missing authorization header\" \"msg\"=\"invalid token encoding\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""},
 		},
 		{
-			name:          "authorization header missing bearer prefix",
+			name:           "authorization header missing bearer prefix",
-			getKubeconfig: func() (*rest.Config, error) { return &testServerKubeconfig, nil },
+			getKubeconfig:  func() (*rest.Config, error) { return &testServerKubeconfig, nil },
-			request: &http.Request{
+			request:        newRequest(map[string][]string{"Authorization": {makeTestTokenRequest("foo", "authenticator-one", "test-token")}}),
-				Method: "GET",
-				Header: map[string][]string{"Authorization": {makeTestTokenRequest("foo", "authenticator-one", "test-token")}},
-				URL:    validURL,
-			},
 			wantHTTPBody:   "invalid token encoding\n",
 			wantHTTPStatus: http.StatusBadRequest,
 			wantLogs:       []string{"\"error\"=\"authorization header must be of type Bearer\" \"msg\"=\"invalid token encoding\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""},
 		},
 		{
-			name:          "token is not base64 encoded",
+			name:           "token is not base64 encoded",
-			getKubeconfig: func() (*rest.Config, error) { return &testServerKubeconfig, nil },
+			getKubeconfig:  func() (*rest.Config, error) { return &testServerKubeconfig, nil },
-			request: &http.Request{
+			request:        newRequest(map[string][]string{"Authorization": {"Bearer !!!"}}),
-				Method: "GET",
-				Header: map[string][]string{"Authorization": {"Bearer !!!"}},
-				URL:    validURL,
-			},
 			wantHTTPBody:   "invalid token encoding\n",
 			wantHTTPStatus: http.StatusBadRequest,
 			wantLogs:       []string{"\"error\"=\"invalid base64 in encoded bearer token: illegal base64 data at input byte 0\" \"msg\"=\"invalid token encoding\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""},
 		},
 		{
-			name:          "base64 encoded token is not valid json",
+			name:           "base64 encoded token is not valid json",
-			getKubeconfig: func() (*rest.Config, error) { return &testServerKubeconfig, nil },
+			getKubeconfig:  func() (*rest.Config, error) { return &testServerKubeconfig, nil },
-			request: &http.Request{
+			request:        newRequest(map[string][]string{"Authorization": {"Bearer abc"}}),
-				Method: "GET",
-				Header: map[string][]string{"Authorization": {"Bearer abc"}},
-				URL:    validURL,
-			},
 			wantHTTPBody:   "invalid token encoding\n",
 			wantHTTPStatus: http.StatusBadRequest,
 			wantLogs:       []string{"\"error\"=\"invalid TokenCredentialRequest encoded in bearer token: invalid character 'i' looking for beginning of value\" \"msg\"=\"invalid token encoding\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""},
 		},
 		{
-			name:          "token could not be authenticated",
+			name:           "token could not be authenticated",
-			getKubeconfig: func() (*rest.Config, error) { return &testServerKubeconfig, nil },
+			getKubeconfig:  func() (*rest.Config, error) { return &testServerKubeconfig, nil },
-			request: &http.Request{
+			request:        newRequest(map[string][]string{"Authorization": {"Bearer " + makeTestTokenRequest("", "", "")}}),
-				Method: "GET",
-				Header: map[string][]string{"Authorization": {"Bearer " + makeTestTokenRequest("", "", "")}},
-				URL:    validURL,
-			},
 			wantHTTPBody:   "invalid token\n",
 			wantHTTPStatus: http.StatusUnauthorized,
 			wantLogs:       []string{"\"error\"=\"no such authenticator\" \"msg\"=\"received invalid token\" \"authenticator\"={\"apiGroup\":null,\"kind\":\"\",\"name\":\"\"} \"authenticatorNamespace\"=\"\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""},
@@ -164,11 +151,7 @@ func TestImpersonator(t *testing.T) {
 		{
 			name:          "token authenticates as nil",
 			getKubeconfig: func() (*rest.Config, error) { return &testServerKubeconfig, nil },
-			request: &http.Request{
+			request:       newRequest(map[string][]string{"Authorization": {"Bearer " + makeTestTokenRequest("foo", "authenticator-one", "test-token")}}),
-				Method: "GET",
-				Header: map[string][]string{"Authorization": {"Bearer " + makeTestTokenRequest("foo", "authenticator-one", "test-token")}},
-				URL:    validURL,
-			},
 			expectMockToken: func(t *testing.T, recorder *mocktokenauthenticator.MockTokenMockRecorder) {
 				recorder.AuthenticateToken(gomock.Any(), "test-token").Return(nil, false, nil)
 			},
@@ -180,15 +163,11 @@ func TestImpersonator(t *testing.T) {
 		{
 			name:          "token validates",
 			getKubeconfig: func() (*rest.Config, error) { return &testServerKubeconfig, nil },
-			request: &http.Request{
+			request: newRequest(map[string][]string{
-				Method: "GET",
+				"Authorization":    {"Bearer " + makeTestTokenRequest("foo", "authenticator-one", "test-token")},
-				Header: map[string][]string{
+				"Malicious-Header": {"test-header-value-1"},
-					"Authorization":    {"Bearer " + makeTestTokenRequest("foo", "authenticator-one", "test-token")},
+				"User-Agent":       {"test-user-agent"},
-					"Malicious-Header": {"test-header-value-1"},
+			}),
-					"User-Agent":       {"test-user-agent"},
-				},
-				URL: validURL,
-			},
 			expectMockToken: func(t *testing.T, recorder *mocktokenauthenticator.MockTokenMockRecorder) {
 				userInfo := user.DefaultInfo{
 					Name:   "test-user",
@@ -228,7 +207,9 @@ func TestImpersonator(t *testing.T) {
 			require.NoError(t, err)
 			require.NotNil(t, proxy)
 			w := httptest.NewRecorder()
+			requestBeforeServe := tt.request.Clone(tt.request.Context())
 			proxy.ServeHTTP(w, tt.request)
+			require.Equal(t, requestBeforeServe, tt.request, "ServeHTTP() mutated the request, and it should not per http.Handler docs")
 			if tt.wantHTTPStatus != 0 {
 				require.Equal(t, tt.wantHTTPStatus, w.Code)
 			}