authn: fix cache mutation by AuthenticatedGroupAdder

The cached token authenticator returns a cache value. The group adder changes it.
kubernetes · May 12, 2022 · 42d2f2e · 42d2f2e
1 parent f3ad8e6
commit 42d2f2e
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 13 deletions.
diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/group/authenticated_group_adder.go b/staging/src/k8s.io/apiserver/pkg/authentication/group/authenticated_group_adder.go
@@ -51,11 +51,16 @@ func (g *AuthenticatedGroupAdder) AuthenticateRequest(req *http.Request) (*authe
 		}
 	}
 
-	r.User = &user.DefaultInfo{
+	newGroups := make([]string, 0, len(r.User.GetGroups())+1)
+	newGroups = append(newGroups, r.User.GetGroups()...)
+	newGroups = append(newGroups, user.AllAuthenticated)
+
+	ret := *r // shallow copy
+	ret.User = &user.DefaultInfo{
 		Name:   r.User.GetName(),
 		UID:    r.User.GetUID(),
-		Groups: append(r.User.GetGroups(), user.AllAuthenticated),
+		Groups: newGroups,
 		Extra:  r.User.GetExtra(),
 	}
-	return r, true, nil
+	return &ret, true, nil
 }
diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/group/group_adder.go b/staging/src/k8s.io/apiserver/pkg/authentication/group/group_adder.go
@@ -41,11 +41,17 @@ func (g *GroupAdder) AuthenticateRequest(req *http.Request) (*authenticator.Resp
 	if err != nil || !ok {
 		return nil, ok, err
 	}
-	r.User = &user.DefaultInfo{
+
+	newGroups := make([]string, 0, len(r.User.GetGroups())+len(g.Groups))
+	newGroups = append(newGroups, r.User.GetGroups()...)
+	newGroups = append(newGroups, g.Groups...)
+
+	ret := *r // shallow copy
+	ret.User = &user.DefaultInfo{
 		Name:   r.User.GetName(),
 		UID:    r.User.GetUID(),
-		Groups: append(r.User.GetGroups(), g.Groups...),
+		Groups: newGroups,
 		Extra:  r.User.GetExtra(),
 	}
-	return r, true, nil
+	return &ret, true, nil
 }
diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/group/group_adder_test.go b/staging/src/k8s.io/apiserver/pkg/authentication/group/group_adder_test.go
@@ -26,10 +26,14 @@ import (
 )
 
 func TestGroupAdder(t *testing.T) {
+	capacity := make([]string, 0, 1024)
+	response := &authenticator.Response{User: &user.DefaultInfo{Name: "user", Groups: append(capacity, "original")}}
+	orig := toJson(response)
+
 	adder := authenticator.Request(
 		NewGroupAdder(
 			authenticator.RequestFunc(func(req *http.Request) (*authenticator.Response, bool, error) {
-				return &authenticator.Response{User: &user.DefaultInfo{Name: "user", Groups: []string{"original"}}}, true, nil
+				return response, true, nil
 			}),
 			[]string{"added"},
 		),
@@ -39,12 +43,16 @@ func TestGroupAdder(t *testing.T) {
 	if want := []string{"original", "added"}; !reflect.DeepEqual(r.User.GetGroups(), want) {
 		t.Errorf("Unexpected groups\ngot:\t%#v\nwant:\t%#v", r.User.GetGroups(), want)
 	}
+
+	if got := toJson(response); got != orig {
+		t.Errorf("Expected response from delegate to be unmodified: orig=%v got=%v", orig, got)
+	}
 }
 
 func TestAuthenticatedGroupAdder(t *testing.T) {
 	tests := []struct {
 		name         string
-		inputUser    user.Info
+		inputUser    *user.DefaultInfo
 		expectedUser user.Info
 	}{
 		{
@@ -94,10 +102,16 @@ func TestAuthenticatedGroupAdder(t *testing.T) {
 	}
 
 	for _, test := range tests {
+		capacity := make([]string, 0, 1024)
+		user := test.inputUser
+		user.Groups = append(capacity, user.Groups...) // make sure there is capacity in the groups array to trigger potential mutation
+		response := &authenticator.Response{User: user}
+		orig := toJson(response)
+
 		adder := authenticator.Request(
 			NewAuthenticatedGroupAdder(
 				authenticator.RequestFunc(func(req *http.Request) (*authenticator.Response, bool, error) {
-					return &authenticator.Response{User: test.inputUser}, true, nil
+					return response, true, nil
 				}),
 			),
 		)
@@ -106,6 +120,10 @@ func TestAuthenticatedGroupAdder(t *testing.T) {
 		if !reflect.DeepEqual(r.User, test.expectedUser) {
 			t.Errorf("Unexpected user\ngot:\t%#v\nwant:\t%#v", r.User, test.expectedUser)
 		}
+
+		if got := toJson(response); got != orig {
+			t.Errorf("Expected response from delegate to be unmodified: orig=%v got=%v", orig, got)
+		}
 	}
 
 }
diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/group/token_group_adder.go b/staging/src/k8s.io/apiserver/pkg/authentication/group/token_group_adder.go
@@ -41,11 +41,17 @@ func (g *TokenGroupAdder) AuthenticateToken(ctx context.Context, token string) (
 	if err != nil || !ok {
 		return nil, ok, err
 	}
-	r.User = &user.DefaultInfo{
+
+	newGroups := make([]string, 0, len(r.User.GetGroups())+len(g.Groups))
+	newGroups = append(newGroups, r.User.GetGroups()...)
+	newGroups = append(newGroups, g.Groups...)
+
+	ret := *r // shallow copy
+	ret.User = &user.DefaultInfo{
 		Name:   r.User.GetName(),
 		UID:    r.User.GetUID(),
-		Groups: append(r.User.GetGroups(), g.Groups...),
+		Groups: newGroups,
 		Extra:  r.User.GetExtra(),
 	}
-	return r, true, nil
+	return &ret, true, nil
 }
diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/group/token_group_adder_test.go b/staging/src/k8s.io/apiserver/pkg/authentication/group/token_group_adder_test.go
@@ -18,6 +18,7 @@ package group
 
 import (
 	"context"
+	"encoding/json"
 	"reflect"
 	"testing"
 
@@ -26,10 +27,14 @@ import (
 )
 
 func TestTokenGroupAdder(t *testing.T) {
+	capacity := make([]string, 0, 1024)
+	response := &authenticator.Response{User: &user.DefaultInfo{Name: "user", Groups: append(capacity, "original")}}
+	orig := toJson(response)
+
 	adder := authenticator.Token(
 		NewTokenGroupAdder(
 			authenticator.TokenFunc(func(ctx context.Context, token string) (*authenticator.Response, bool, error) {
-				return &authenticator.Response{User: &user.DefaultInfo{Name: "user", Groups: []string{"original"}}}, true, nil
+				return response, true, nil
 			}),
 			[]string{"added"},
 		),
@@ -39,4 +44,13 @@ func TestTokenGroupAdder(t *testing.T) {
 	if !reflect.DeepEqual(r.User.GetGroups(), []string{"original", "added"}) {
 		t.Errorf("Expected original,added groups, got %#v", r.User.GetGroups())
 	}
+
+	if got := toJson(response); got != orig {
+		t.Errorf("Expected response from delegate to be unmodified: orig=%v got=%v", orig, got)
+	}
+}
+
+func toJson(x interface{}) string {
+	b, _ := json.Marshal(x)
+	return string(b)
 }