From 5b327a2b373f0ce94664dfdc81c0f244abdd3089 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Fri, 4 Jun 2021 15:22:01 -0400 Subject: [PATCH] impersonator: remove redundant deleteKnownImpersonationHeaders logic WithImpersonation already deletes impersonation headers and has done so since the early days: https://github.com/kubernetes/kubernetes/pull/36769 ensureNoImpersonationHeaders will still reject any request that has impersonation headers set so we will always fail closed. Signed-off-by: Monis Khan --- .../concierge/impersonator/impersonator.go | 32 ------ .../impersonator/impersonator_test.go | 108 ------------------ 2 files changed, 140 deletions(-) diff --git a/internal/concierge/impersonator/impersonator.go b/internal/concierge/impersonator/impersonator.go index d2aa0280..41ddeec0 100644 --- a/internal/concierge/impersonator/impersonator.go +++ b/internal/concierge/impersonator/impersonator.go @@ -174,10 +174,6 @@ func newInternal( //nolint:funlen // yeah, it's kind of long. })) handler = filterlatency.TrackStarted(handler, "impersonationproxy") - handler = filterlatency.TrackCompleted(handler) - handler = deleteKnownImpersonationHeaders(handler) - handler = filterlatency.TrackStarted(handler, "deleteimpersonationheaders") - // The standard Kube handler chain (authn, authz, impersonation, audit, etc). // See the genericapiserver.DefaultBuildHandlerChain func for details. handler = defaultBuildHandlerChainFunc(handler, c) @@ -372,34 +368,6 @@ func isTokenCredReq(reqInfo *genericapirequest.RequestInfo) bool { return true } -func deleteKnownImpersonationHeaders(delegate http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // remove known impersonation headers while avoiding mutation of input request - // unknown future impersonation headers will still get caught by our later checks - if ensureNoImpersonationHeaders(r) != nil { - r = r.Clone(r.Context()) - - impersonationHeaders := []string{ - transport.ImpersonateUserHeader, - transport.ImpersonateGroupHeader, - } - - for k := range r.Header { - if !strings.HasPrefix(k, transport.ImpersonateUserExtraHeaderPrefix) { - continue - } - impersonationHeaders = append(impersonationHeaders, k) - } - - for _, header := range impersonationHeaders { - r.Header.Del(header) // delay mutation until the end when we are done iterating over the map - } - } - - delegate.ServeHTTP(w, r) - }) -} - // No-op wrapping around RequestFunc to allow for comparisons. type comparableAuthenticator struct { authenticator.RequestFunc diff --git a/internal/concierge/impersonator/impersonator_test.go b/internal/concierge/impersonator/impersonator_test.go index f30615d3..182d3047 100644 --- a/internal/concierge/impersonator/impersonator_test.go +++ b/internal/concierge/impersonator/impersonator_test.go @@ -1628,114 +1628,6 @@ func requireCanBindToPort(t *testing.T, port int) { require.NoError(t, ln.Close()) } -func Test_deleteKnownImpersonationHeaders(t *testing.T) { - tests := []struct { - name string - headers, want http.Header - }{ - { - name: "no impersonation", - headers: map[string][]string{ - "a": {"b"}, - "Accept-Encoding": {"gzip"}, - "User-Agent": {"test-user-agent"}, - }, - want: map[string][]string{ - "a": {"b"}, - "Accept-Encoding": {"gzip"}, - "User-Agent": {"test-user-agent"}, - }, - }, - { - name: "impersonate user header is dropped", - headers: map[string][]string{ - "a": {"b"}, - "Impersonate-User": {"panda"}, - "Accept-Encoding": {"gzip"}, - "User-Agent": {"test-user-agent"}, - }, - want: map[string][]string{ - "a": {"b"}, - "Accept-Encoding": {"gzip"}, - "User-Agent": {"test-user-agent"}, - }, - }, - { - name: "all known impersonate headers are dropped", - headers: map[string][]string{ - "Accept-Encoding": {"gzip"}, - "Authorization": {"Bearer some-service-account-token"}, - "Impersonate-Extra-Extra-1": {"some", "extra", "stuff"}, - "Impersonate-Extra-Extra-2": {"some", "more", "extra", "stuff"}, - "Impersonate-Group": {"test-group-1", "test-group-2"}, - "Impersonate-User": {"test-user"}, - "User-Agent": {"test-user-agent"}, - }, - want: map[string][]string{ - "Accept-Encoding": {"gzip"}, - "Authorization": {"Bearer some-service-account-token"}, - "User-Agent": {"test-user-agent"}, - }, - }, - { - name: "future UID header is not dropped", - headers: map[string][]string{ - "Accept-Encoding": {"gzip"}, - "Authorization": {"Bearer some-service-account-token"}, - "Impersonate-Extra-Extra-1": {"some", "extra", "stuff"}, - "Impersonate-Extra-Extra-2": {"some", "more", "extra", "stuff"}, - "Impersonate-Group": {"test-group-1", "test-group-2"}, - "Impersonate-User": {"test-user"}, - "Impersonate-Uid": {"008"}, - "User-Agent": {"test-user-agent"}, - }, - want: map[string][]string{ - "Accept-Encoding": {"gzip"}, - "Authorization": {"Bearer some-service-account-token"}, - "User-Agent": {"test-user-agent"}, - "Impersonate-Uid": {"008"}, - }, - }, - { - name: "future UID header is not dropped, no other headers", - headers: map[string][]string{ - "Impersonate-Uid": {"009"}, - }, - want: map[string][]string{ - "Impersonate-Uid": {"009"}, - }, - }, - } - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - inputReq := (&http.Request{Header: tt.headers}).WithContext(context.Background()) - inputReqCopy := inputReq.Clone(inputReq.Context()) - - var called bool - delegate := http.HandlerFunc(func(w http.ResponseWriter, outputReq *http.Request) { - called = true - require.Nil(t, w) - - // assert only headers mutated - outputReqCopy := outputReq.Clone(outputReq.Context()) - outputReqCopy.Header = tt.headers - require.Equal(t, inputReqCopy, outputReqCopy) - - require.Equal(t, tt.want, outputReq.Header) - - if ensureNoImpersonationHeaders(inputReq) == nil { - require.True(t, inputReq == outputReq, "expect req to passed through when no modification needed") - } - }) - - deleteKnownImpersonationHeaders(delegate).ServeHTTP(nil, inputReq) - require.Equal(t, inputReqCopy, inputReq) // assert no mutation occurred - require.True(t, called) - }) - } -} - func Test_withBearerTokenPreservation(t *testing.T) { tests := []struct { name string