pkg/{authn,authz}: rm consumed headers, add FIXME for rest

ibihim · Sep 11, 2024 · 797e950 · 797e950
1 parent 95d49dc
commit 797e950
Show file tree

Hide file tree

Showing 3 changed files with 70 additions and 25 deletions.
diff --git a/pkg/authn/identityheaders/identityheaders.go b/pkg/authn/identityheaders/identityheaders.go
@@ -43,6 +43,10 @@ func WithAuthHeaders(handler http.Handler, cfg *AuthnHeaderConfig) http.Handler
 	}
 
 	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+		// We don't want the client to impersonate someone else.
+		req.Header.Del(cfg.UserFieldName)
+		req.Header.Del(cfg.GroupsFieldName)
+
 		u, ok := request.UserFrom(req.Context())
 		if ok {
 			// Seemingly well-known headers to tell the upstream about user's identity

diff --git a/pkg/authn/identityheaders/identityheaders_test.go b/pkg/authn/identityheaders/identityheaders_test.go
@@ -43,57 +43,56 @@ func TestWithAuthHeaders(t *testing.T) {
 	groupKey := "Group"
 	groupValue := "utzer"
 
+	defaultUserHeader := map[string]string{
+		userKey:  userValue,
+		groupKey: groupValue,
+	}
+
 	for _, tt := range []struct {
 		name   string
 		cfg    *identityheaders.AuthnHeaderConfig
-		ctx    context.Context
-		header map[string][]string
+		req    *http.Request
+		header map[string]string
 	}{
 		{
 			name:   "should pass through",
 			cfg:    &identityheaders.AuthnHeaderConfig{},
-			header: map[string][]string{},
+			req:    testRequest(t, withHeader(defaultUserHeader)),
+			header: defaultUserHeader,
 		},
 		{
 			name: "should set username in header",
 			cfg: &identityheaders.AuthnHeaderConfig{
 				UserFieldName:   userKey,
 				GroupsFieldName: groupKey,
 			},
-			header: map[string][]string{
-				userKey:  {userValue},
-				groupKey: {groupValue},
+			header: defaultUserHeader,
+			req:    testRequest(t, withUserContext(userValue, groupValue)),
+		},
+		{
+			name: "should not pass client header",
+			cfg: &identityheaders.AuthnHeaderConfig{
+				UserFieldName:   userKey,
+				GroupsFieldName: groupKey,
 			},
+			req:    testRequest(t, withHeader(map[string]string{userKey: "admin", groupKey: "system:admin"})),
+			header: map[string]string{},
 		},
 	} {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
-			req, err := http.NewRequest(http.MethodGet, "http://example.com", nil)
-			if err != nil {
-				t.Fatal(err)
-			}
-			req = req.WithContext(
-				request.WithUser(
-					req.Context(),
-					&user.DefaultInfo{
-						Name:   userValue,
-						Groups: []string{groupValue},
-					},
-				),
-			)
-
 			rec := httptest.NewRecorder()
-			identityheaders.WithAuthHeaders(okHandler, tt.cfg).ServeHTTP(rec, req)
+			identityheaders.WithAuthHeaders(okHandler, tt.cfg).ServeHTTP(rec, tt.req)
 
-			if len(req.Header) != len(tt.header) {
-				t.Errorf("want: %+v\nhave:%+v", tt.header, req.Header)
+			if len(tt.req.Header) != len(tt.header) {
+				t.Errorf("want: %+v\nhave:%+v", tt.header, tt.req.Header)
 				return
 			}
 
 			if len(tt.header) > 0 {
 				for k, v := range tt.header {
-					if req.Header[k][0] != v[0] {
-						t.Errorf("want: %s\nhave: %s", v[0], req.Header[k][0])
+					if tt.req.Header[k][0] != v {
+						t.Errorf("want: %s\nhave: %s", v, tt.req.Header[k][0])
 					}
 				}
 			}
@@ -221,3 +220,43 @@ type testCase struct {
 	expected
 	description string
 }
+
+func testRequest(t *testing.T, withOpts ...func(*http.Request) (*http.Request, error)) *http.Request {
+	req, err := http.NewRequest(http.MethodGet, "http://example.com", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	for _, opt := range withOpts {
+		req, err = opt(req)
+		if err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	return req
+}
+
+func withHeader(header map[string]string) func(*http.Request) (*http.Request, error) {
+	return func(req *http.Request) (*http.Request, error) {
+		for key, value := range header {
+			req.Header.Set(key, value)
+		}
+
+		return req, nil
+	}
+}
+
+func withUserContext(userValue, groupValue string) func(*http.Request) (*http.Request, error) {
+	return func(req *http.Request) (*http.Request, error) {
+		return req.WithContext(
+			request.WithUser(
+				req.Context(),
+				&user.DefaultInfo{
+					Name:   userValue,
+					Groups: []string{groupValue},
+				},
+			),
+		), nil
+	}
+}
diff --git a/pkg/authorization/rewrite/middleware.go b/pkg/authorization/rewrite/middleware.go
@@ -48,6 +48,8 @@ func WithKubeRBACProxyParamsHandler(handler http.Handler, config *RewriteAttribu
 func requestToParams(config *RewriteAttributesConfig, req *http.Request) []string {
 	params := []string{}
 
+	// FIXME / TODO: We should add a flag (--insecure-pass-through) that is required in order
+	//               to not remove the query / header attributes after consumption.
 	if config.Rewrites.ByQueryParameter != nil && config.Rewrites.ByQueryParameter.Name != "" {
 		if ps, ok := req.URL.Query()[config.Rewrites.ByQueryParameter.Name]; ok {
 			params = append(params, ps...)