From 4c68050706fb0dfff4f7ae03ec318676aba07c3d Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Tue, 2 Mar 2021 14:56:54 -0800 Subject: [PATCH] Allow all headers besides impersonation-* through impersonation proxy --- .../concierge/impersonator/impersonator.go | 38 ++++++----------- .../impersonator/impersonator_test.go | 42 ++++++++++++------- 2 files changed, 38 insertions(+), 42 deletions(-) diff --git a/internal/concierge/impersonator/impersonator.go b/internal/concierge/impersonator/impersonator.go index e4be50c4..c189daf0 100644 --- a/internal/concierge/impersonator/impersonator.go +++ b/internal/concierge/impersonator/impersonator.go @@ -10,6 +10,7 @@ import ( "net/http" "net/http/httputil" "net/url" + "regexp" "strings" "time" @@ -27,16 +28,8 @@ import ( "go.pinniped.dev/internal/kubeclient" ) -// allowedHeaders are the set of HTTP headers that are allowed to be forwarded through the impersonation proxy. -//nolint: gochecknoglobals -var allowedHeaders = []string{ - "Accept", - "Accept-Encoding", - "User-Agent", - "Connection", - "Upgrade", - "Content-Type", -} +// nolint: gochecknoglobals +var impersonateHeaderRegex = regexp.MustCompile("Impersonate-.*") type proxy struct { cache *authncache.Cache @@ -157,17 +150,9 @@ type httpError struct { func (e *httpError) Error() string { return e.message } func ensureNoImpersonationHeaders(r *http.Request) error { - if _, ok := r.Header[transport.ImpersonateUserHeader]; ok { - return fmt.Errorf("%q header already exists", transport.ImpersonateUserHeader) - } - - if _, ok := r.Header[transport.ImpersonateGroupHeader]; ok { - return fmt.Errorf("%q header already exists", transport.ImpersonateGroupHeader) - } - - for header := range r.Header { - if strings.HasPrefix(header, transport.ImpersonateUserExtraHeaderPrefix) { - return fmt.Errorf("%q header already exists", transport.ImpersonateUserExtraHeaderPrefix) + for key := range r.Header { + if impersonateHeaderRegex.MatchString(key) { + return fmt.Errorf("%q header already exists", key) } } @@ -209,11 +194,12 @@ func getProxyHeaders(userInfo user.Info, requestHeaders http.Header) http.Header //nolint:bodyclose // We return a nil http.Response above, so there is nothing to close. _, _ = transport.NewImpersonatingRoundTripper(impersonateConfig, impersonateHeaderSpy).RoundTrip(fakeReq) - // Copy over the allowed header values from the original request to the new request. - for _, header := range allowedHeaders { - values := requestHeaders.Values(header) - for i := range values { - newHeaders.Add(header, values[i]) + // Copy over all headers except the Authorization header from the original request to the new request. + for key := range requestHeaders { + if key != "Authorization" { + for _, val := range requestHeaders.Values(key) { + newHeaders.Add(key, val) + } } } diff --git a/internal/concierge/impersonator/impersonator_test.go b/internal/concierge/impersonator/impersonator_test.go index 3e590ccf..7a7124b2 100644 --- a/internal/concierge/impersonator/impersonator_test.go +++ b/internal/concierge/impersonator/impersonator_test.go @@ -132,7 +132,16 @@ func TestImpersonator(t *testing.T) { request: newRequest(map[string][]string{"Impersonate-Extra-something": {"something"}}), wantHTTPBody: "impersonation header already exists\n", wantHTTPStatus: http.StatusBadRequest, - wantLogs: []string{"\"msg\"=\"impersonation header already exists\" \"error\"=\"\\\"Impersonate-Extra-\\\" header already exists\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""}, + wantLogs: []string{"\"msg\"=\"impersonation header already exists\" \"error\"=\"\\\"Impersonate-Extra-something\\\" header already exists\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""}, + }, + { + name: "Impersonate-* header already in request", + request: newRequest(map[string][]string{ + "Impersonate-Something": {"some-newfangled-impersonate-header"}, + }), + wantHTTPBody: "impersonation header already exists\n", + wantHTTPStatus: http.StatusBadRequest, + wantLogs: []string{"\"msg\"=\"impersonation header already exists\" \"error\"=\"\\\"Impersonate-Something\\\" header already exists\" \"method\"=\"GET\" \"url\"=\"http://pinniped.dev/blah\""}, }, { name: "missing authorization header", @@ -198,15 +207,15 @@ func TestImpersonator(t *testing.T) { { name: "token validates", request: newRequest(map[string][]string{ - "Authorization": {"Bearer " + impersonationtoken.Make(t, "test-token", &goodAuthenticator, defaultAPIGroup)}, - "User-Agent": {"test-user-agent"}, - "Accept": {"some-accepted-format"}, - "Accept-Encoding": {"some-accepted-encoding"}, - "Connection": {"Upgrade"}, // the value "Upgrade" is handled in a special way by `httputil.NewSingleHostReverseProxy` - "Upgrade": {"some-upgrade"}, - "Content-Type": {"some-type"}, - "Content-Length": {"some-length"}, - "Malicious-Header": {"test-header-value-1"}, // this header should not be forwarded to the Kube API server + "Authorization": {"Bearer " + impersonationtoken.Make(t, "test-token", &goodAuthenticator, defaultAPIGroup)}, + "User-Agent": {"test-user-agent"}, + "Accept": {"some-accepted-format"}, + "Accept-Encoding": {"some-accepted-encoding"}, + "Connection": {"Upgrade"}, // the value "Upgrade" is handled in a special way by `httputil.NewSingleHostReverseProxy` + "Upgrade": {"some-upgrade"}, + "Content-Type": {"some-type"}, + "Content-Length": {"some-length"}, + "Other-Header": {"test-header-value-1"}, // this header will be passed through }), expectMockToken: func(t *testing.T, recorder *mocktokenauthenticator.MockTokenMockRecorder) { userInfo := user.DefaultInfo{ @@ -230,6 +239,7 @@ func TestImpersonator(t *testing.T) { "Connection": {"Upgrade"}, "Upgrade": {"some-upgrade"}, "Content-Type": {"some-type"}, + "Other-Header": {"test-header-value-1"}, }, wantHTTPBody: "successful proxied response", wantHTTPStatus: http.StatusOK, @@ -238,9 +248,8 @@ func TestImpersonator(t *testing.T) { { name: "token validates and the kube API request returns an error", request: newRequest(map[string][]string{ - "Authorization": {"Bearer " + impersonationtoken.Make(t, "test-token", &goodAuthenticator, defaultAPIGroup)}, - "Malicious-Header": {"test-header-value-1"}, - "User-Agent": {"test-user-agent"}, + "Authorization": {"Bearer " + impersonationtoken.Make(t, "test-token", &goodAuthenticator, defaultAPIGroup)}, + "User-Agent": {"test-user-agent"}, }), expectMockToken: func(t *testing.T, recorder *mocktokenauthenticator.MockTokenMockRecorder) { userInfo := user.DefaultInfo{ @@ -269,9 +278,9 @@ func TestImpersonator(t *testing.T) { name: "token validates with custom api group", apiGroupOverride: customAPIGroup, request: newRequest(map[string][]string{ - "Authorization": {"Bearer " + impersonationtoken.Make(t, "test-token", &goodAuthenticator, customAPIGroup)}, - "Malicious-Header": {"test-header-value-1"}, - "User-Agent": {"test-user-agent"}, + "Authorization": {"Bearer " + impersonationtoken.Make(t, "test-token", &goodAuthenticator, customAPIGroup)}, + "Other-Header": {"test-header-value-1"}, + "User-Agent": {"test-user-agent"}, }), expectMockToken: func(t *testing.T, recorder *mocktokenauthenticator.MockTokenMockRecorder) { userInfo := user.DefaultInfo{ @@ -291,6 +300,7 @@ func TestImpersonator(t *testing.T) { "Impersonate-Group": {"test-group-1", "test-group-2"}, "Impersonate-User": {"test-user"}, "User-Agent": {"test-user-agent"}, + "Other-Header": {"test-header-value-1"}, }, wantHTTPBody: "successful proxied response", wantHTTPStatus: http.StatusOK,