From 3677d6afcf86750e333f51da4af2f38020216a5d Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Wed, 11 May 2022 16:17:29 +0200 Subject: [PATCH] authn: fix cache mutation by AuthenticatedGroupAdder The cached token authenticator returns a cache value. The group adder changes it. Kubernetes-commit: e09e81e4f6e62f9fef89736e79d04983a77e695f --- .../group/authenticated_group_adder.go | 11 ++++++--- pkg/authentication/group/group_adder.go | 12 +++++++--- pkg/authentication/group/group_adder_test.go | 24 ++++++++++++++++--- pkg/authentication/group/token_group_adder.go | 12 +++++++--- .../group/token_group_adder_test.go | 16 ++++++++++++- 5 files changed, 62 insertions(+), 13 deletions(-) diff --git a/pkg/authentication/group/authenticated_group_adder.go b/pkg/authentication/group/authenticated_group_adder.go index 5ac6b2ddf..2f2cc6ec5 100644 --- a/pkg/authentication/group/authenticated_group_adder.go +++ b/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/pkg/authentication/group/group_adder.go b/pkg/authentication/group/group_adder.go index 3079dad62..1234c595a 100644 --- a/pkg/authentication/group/group_adder.go +++ b/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/pkg/authentication/group/group_adder_test.go b/pkg/authentication/group/group_adder_test.go index b84f07b90..2f191f62a 100644 --- a/pkg/authentication/group/group_adder_test.go +++ b/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/pkg/authentication/group/token_group_adder.go b/pkg/authentication/group/token_group_adder.go index 0ed5ee559..51e27a67d 100644 --- a/pkg/authentication/group/token_group_adder.go +++ b/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/pkg/authentication/group/token_group_adder_test.go b/pkg/authentication/group/token_group_adder_test.go index e9565a21c..e0cb24336 100644 --- a/pkg/authentication/group/token_group_adder_test.go +++ b/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) }