Merge pull request #341 from kramaranya/group-header-separation

Put groups into separate header entries
brancz · Dec 12, 2024 · 91a1759 · 91a1759
2 parents a47005c + 816c12d
commit 91a1759
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 20 deletions.
diff --git a/pkg/authn/identityheaders/identityheaders.go b/pkg/authn/identityheaders/identityheaders.go
@@ -21,6 +21,7 @@ import (
 	"strings"
 
 	"k8s.io/apiserver/pkg/endpoints/request"
+	"k8s.io/klog/v2"
 )
 
 // AuthnHeaderConfig contains authentication header settings which enable more information about the user identity to be sent to the upstream
@@ -52,7 +53,15 @@ func WithAuthHeaders(handler http.Handler, cfg *AuthnHeaderConfig) http.Handler
 			// Seemingly well-known headers to tell the upstream about user's identity
 			// so that the upstream can achieve the original goal of delegating RBAC authn/authz to kube-rbac-proxy
 			req.Header.Set(cfg.UserFieldName, u.GetName())
-			req.Header.Set(cfg.GroupsFieldName, strings.Join(u.GetGroups(), cfg.GroupSeparator))
+
+			if cfg.GroupSeparator == "" {
+				for _, group := range u.GetGroups() {
+					req.Header.Add(cfg.GroupsFieldName, group)
+				}
+			} else {
+				filteredGroups := filterGroups(u.GetGroups(), cfg.GroupSeparator)
+				req.Header.Set(cfg.GroupsFieldName, strings.Join(filteredGroups, cfg.GroupSeparator))
+			}
 		}
 
 		handler.ServeHTTP(w, req)
@@ -62,3 +71,15 @@ func WithAuthHeaders(handler http.Handler, cfg *AuthnHeaderConfig) http.Handler
 func HasIdentityHeadersEnabled(cfg *AuthnHeaderConfig) bool {
 	return len(cfg.GroupsFieldName) > 0 || len(cfg.UserFieldName) > 0
 }
+
+func filterGroups(groups []string, separator string) []string {
+	var validGroups []string
+	for _, group := range groups {
+		if strings.Contains(group, separator) {
+			klog.Infof("Dropping group %q because it contains the group separator %q", group, separator)
+			continue
+		}
+		validGroups = append(validGroups, group)
+	}
+	return validGroups
+}
diff --git a/pkg/authn/identityheaders/identityheaders_test.go b/pkg/authn/identityheaders/identityheaders_test.go
@@ -21,7 +21,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/http/httputil"
-	"strings"
+	"reflect"
 	"testing"
 
 	"k8s.io/apiserver/pkg/authentication/authenticator"
@@ -43,16 +43,16 @@ func TestWithAuthHeaders(t *testing.T) {
 	groupKey := "Group"
 	groupValue := "utzer"
 
-	defaultUserHeader := map[string]string{
-		userKey:  userValue,
-		groupKey: groupValue,
+	defaultUserHeader := map[string][]string{
+		userKey:  {userValue},
+		groupKey: {groupValue},
 	}
 
 	for _, tt := range []struct {
 		name   string
 		cfg    *identityheaders.AuthnHeaderConfig
 		req    *http.Request
-		header map[string]string
+		header map[string][]string
 	}{
 		{
 			name:   "should pass through",
@@ -67,16 +67,41 @@ func TestWithAuthHeaders(t *testing.T) {
 				GroupsFieldName: groupKey,
 			},
 			header: defaultUserHeader,
-			req:    testRequest(t, withUserContext(userValue, groupValue)),
+			req:    testRequest(t, withUserContext(userValue, []string{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{},
+			req:    testRequest(t, withHeader(map[string][]string{userKey: {"admin"}, groupKey: {"system:admin"}})),
+			header: map[string][]string{},
+		},
+		{
+			name: "should include group containing comma",
+			cfg: &identityheaders.AuthnHeaderConfig{
+				UserFieldName:   userKey,
+				GroupsFieldName: groupKey,
+			},
+			req: testRequest(t, withUserContext(userValue, []string{groupValue, "group,with,comma", "anothergroup"})),
+			header: map[string][]string{
+				userKey:  {userValue},
+				groupKey: {groupValue, "group,with,comma", "anothergroup"},
+			},
+		},
+		{
+			name: "should drop group containing group separator",
+			cfg: &identityheaders.AuthnHeaderConfig{
+				UserFieldName:   userKey,
+				GroupsFieldName: groupKey,
+				GroupSeparator:  ";",
+			},
+			req: testRequest(t, withUserContext(userValue, []string{groupValue, "group;with;separator", "anothergroup"})),
+			header: map[string][]string{
+				userKey:  {userValue},
+				groupKey: {groupValue + ";" + "anothergroup"},
+			},
 		},
 	} {
 		tt := tt
@@ -91,8 +116,8 @@ func TestWithAuthHeaders(t *testing.T) {
 
 			if len(tt.header) > 0 {
 				for k, v := range tt.header {
-					if tt.req.Header[k][0] != v {
-						t.Errorf("want: %s\nhave: %s", v, tt.req.Header[k][0])
+					if !reflect.DeepEqual(tt.req.Header.Values(k), v) {
+						t.Errorf("want: %v,\nhave: %v", v, tt.req.Header.Values(k))
 					}
 				}
 			}
@@ -106,7 +131,7 @@ func TestProxyWithOIDCSupport(t *testing.T) {
 		GroupsFieldName: "groups",
 	}
 
-	fakeUser := user.DefaultInfo{Name: "Foo Bar", Groups: []string{"foo-bars"}}
+	fakeUser := user.DefaultInfo{Name: "Foo Bar", Groups: []string{"foo-bars", "admins"}}
 	authenticator := fakeOIDCAuthenticator(t, &fakeUser)
 
 	for _, v := range []testCase{
@@ -163,12 +188,12 @@ func TestProxyWithOIDCSupport(t *testing.T) {
 
 			if v.verifyUser {
 				user := v.req.Header.Get(cfg.UserFieldName)
-				groups := v.req.Header.Get(cfg.GroupsFieldName)
+				groups := v.req.Header.Values(cfg.GroupsFieldName)
 				if user != fakeUser.GetName() {
 					t.Errorf("User in the response header does not match authenticated user. Expected : %s, received : %s ", fakeUser.GetName(), user)
 				}
-				if groups != strings.Join(fakeUser.GetGroups(), cfg.GroupSeparator) {
-					t.Errorf("Groupsr in the response header does not match authenticated user groups. Expected : %s, received : %s ", fakeUser.GetName(), groups)
+				if !reflect.DeepEqual(groups, fakeUser.GetGroups()) {
+					t.Errorf("Groups in the response header do not match authenticated user groups. Expected : %v, received : %v ", fakeUser.GetGroups(), groups)
 				}
 			}
 		})
@@ -237,24 +262,26 @@ func testRequest(t *testing.T, withOpts ...func(*http.Request) (*http.Request, e
 	return req
 }
 
-func withHeader(header map[string]string) func(*http.Request) (*http.Request, error) {
+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)
+		for key, values := range header {
+			for _, value := range values {
+				req.Header.Add(key, value)
+			}
 		}
 
 		return req, nil
 	}
 }
 
-func withUserContext(userValue, groupValue string) func(*http.Request) (*http.Request, error) {
+func withUserContext(userValue string, groupValues []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},
+					Groups: groupValues,
 				},
 			),
 		), nil