authn: fix cache mutation by AuthenticatedGroupAdder #109969

sttts · 2022-05-11T14:45:07Z

The cached token authenticator returns a cache value. The group adder changes it.

Fix a data race in authentication between AuthenticatedGroupAdder and cached token authenticator.

enj

One comment.

staging/src/k8s.io/apiserver/pkg/authentication/group/token_group_adder_test.go

staging/src/k8s.io/apiserver/pkg/authentication/group/group_adder_test.go

The cached token authenticator returns a cache value. The group adder changes it.

enj · 2022-05-11T20:05:31Z

/kind bug
/priority important-soon
/milestone v1.25
/triage accepted

enj · 2022-05-11T20:08:09Z

/lgtm
/approve

k8s-ci-robot · 2022-05-11T20:08:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/apiserver/pkg/authentication/OWNERS~~ [enj,sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

liggitt · 2022-05-11T20:22:57Z

/retest

k8s-triage-robot · 2022-05-11T23:15:46Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

enj · 2022-05-12T10:52:07Z

Confirmed that the test changes catch the mutation:

# after fetching latest master
git checkout v1.24.0 staging/src/k8s.io/apiserver/pkg/authentication/group/authenticated_group_adder.go
git checkout v1.24.0 staging/src/k8s.io/apiserver/pkg/authentication/group/group_adder.go
git checkout v1.24.0 staging/src/k8s.io/apiserver/pkg/authentication/group/token_group_adder.go
git reset
git diff
go test -race k8s.io/apiserver/pkg/authentication/group

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
index 2f2cc6ec5a1..5ac6b2ddf9d 100644
--- 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,16 +51,11 @@ func (g *AuthenticatedGroupAdder) AuthenticateRequest(req *http.Request) (*authe
 		}
 	}
 
-	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{
+	r.User = &user.DefaultInfo{
 		Name:   r.User.GetName(),
 		UID:    r.User.GetUID(),
-		Groups: newGroups,
+		Groups: append(r.User.GetGroups(), user.AllAuthenticated),
 		Extra:  r.User.GetExtra(),
 	}
-	return &ret, true, nil
+	return r, 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
index 1234c595aa3..3079dad6254 100644
--- 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,17 +41,11 @@ func (g *GroupAdder) AuthenticateRequest(req *http.Request) (*authenticator.Resp
 	if err != nil || !ok {
 		return nil, ok, err
 	}
-
-	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{
+	r.User = &user.DefaultInfo{
 		Name:   r.User.GetName(),
 		UID:    r.User.GetUID(),
-		Groups: newGroups,
+		Groups: append(r.User.GetGroups(), g.Groups...),
 		Extra:  r.User.GetExtra(),
 	}
-	return &ret, true, nil
+	return r, true, nil
 }
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
index 51e27a67d96..0ed5ee55965 100644
--- 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,17 +41,11 @@ func (g *TokenGroupAdder) AuthenticateToken(ctx context.Context, token string) (
 	if err != nil || !ok {
 		return nil, ok, err
 	}
-
-	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{
+	r.User = &user.DefaultInfo{
 		Name:   r.User.GetName(),
 		UID:    r.User.GetUID(),
-		Groups: newGroups,
+		Groups: append(r.User.GetGroups(), g.Groups...),
 		Extra:  r.User.GetExtra(),
 	}
-	return &ret, true, nil
+	return r, true, nil
 }

--- FAIL: TestGroupAdder (0.00s)
    group_adder_test.go:48: Expected response from delegate to be unmodified: orig={"Audiences":null,"User":{"Name":"user","UID":"","Groups":["original"],"Extra":null}} got={"Audiences":null,"User":{"Name":"user","UID":"","Groups":["original","added"],"Extra":null}}
--- FAIL: TestAuthenticatedGroupAdder (0.00s)
    group_adder_test.go:125: Expected response from delegate to be unmodified: orig={"Audiences":null,"User":{"Name":"user","UID":"","Groups":["some-group"],"Extra":null}} got={"Audiences":null,"User":{"Name":"user","UID":"","Groups":["some-group","system:authenticated"],"Extra":null}}
--- FAIL: TestTokenGroupAdder (0.00s)
    token_group_adder_test.go:49: Expected response from delegate to be unmodified: orig={"Audiences":null,"User":{"Name":"user","UID":"","Groups":["original"],"Extra":null}} got={"Audiences":null,"User":{"Name":"user","UID":"","Groups":["original","added"],"Extra":null}}
FAIL
FAIL	k8s.io/apiserver/pkg/authentication/group	0.301s
FAIL

…upstream-release-1.22 Automated cherry pick of #109969: authn: fix cache mutation by AuthenticatedGroupAdder

…upstream-release-1.24 Automated cherry pick of #109969: authn: fix cache mutation by AuthenticatedGroupAdder

…upstream-release-1.21 Automated cherry pick of #109969: authn: fix cache mutation by AuthenticatedGroupAdder

…upstream-release-1.23 Automated cherry pick of #109969: authn: fix cache mutation by AuthenticatedGroupAdder

k8s-ci-robot requested review from enj and mikedanese May 11, 2022 14:45

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2022

sttts force-pushed the sttts-auth-group-adder-race branch 2 times, most recently from a0acfb9 to 3439250 Compare May 11, 2022 15:00

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 11, 2022

sttts force-pushed the sttts-auth-group-adder-race branch 3 times, most recently from 99361b0 to 7ec0112 Compare May 11, 2022 19:51

enj suggested changes May 11, 2022

View reviewed changes

authn: fix cache mutation by AuthenticatedGroupAdder

e09e81e

The cached token authenticator returns a cache value. The group adder changes it.

sttts force-pushed the sttts-auth-group-adder-race branch from 7ec0112 to e09e81e Compare May 11, 2022 20:05

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels May 11, 2022

k8s-ci-robot added this to the v1.25 milestone May 11, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels May 11, 2022

k8s-ci-robot removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 11, 2022

k8s-ci-robot assigned enj May 11, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2022

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 11, 2022

k8s-ci-robot merged commit 2d6534c into kubernetes:master May 12, 2022

k8s-ci-robot added a commit that referenced this pull request May 18, 2022

Merge pull request #110006 from enj/automated-cherry-pick-of-#109969-…

1b1046d

…upstream-release-1.22 Automated cherry pick of #109969: authn: fix cache mutation by AuthenticatedGroupAdder

k8s-ci-robot added a commit that referenced this pull request May 18, 2022

Merge pull request #110004 from enj/automated-cherry-pick-of-#109969-…

7c7dcea

…upstream-release-1.24 Automated cherry pick of #109969: authn: fix cache mutation by AuthenticatedGroupAdder

k8s-ci-robot added a commit that referenced this pull request May 18, 2022

Merge pull request #110008 from enj/automated-cherry-pick-of-#109969-…

6045722

…upstream-release-1.21 Automated cherry pick of #109969: authn: fix cache mutation by AuthenticatedGroupAdder

k8s-ci-robot added a commit that referenced this pull request May 18, 2022

Merge pull request #110005 from enj/automated-cherry-pick-of-#109969-…

a2aaa2a

…upstream-release-1.23 Automated cherry pick of #109969: authn: fix cache mutation by AuthenticatedGroupAdder

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

authn: fix cache mutation by AuthenticatedGroupAdder #109969

authn: fix cache mutation by AuthenticatedGroupAdder #109969

sttts commented May 11, 2022 •

edited

enj left a comment

enj commented May 11, 2022

enj commented May 11, 2022

k8s-ci-robot commented May 11, 2022

liggitt commented May 11, 2022

k8s-triage-robot commented May 11, 2022

enj commented May 12, 2022

authn: fix cache mutation by AuthenticatedGroupAdder #109969

authn: fix cache mutation by AuthenticatedGroupAdder #109969

Conversation

sttts commented May 11, 2022 • edited

enj left a comment

Choose a reason for hiding this comment

enj commented May 11, 2022

enj commented May 11, 2022

k8s-ci-robot commented May 11, 2022

liggitt commented May 11, 2022

k8s-triage-robot commented May 11, 2022

enj commented May 12, 2022

sttts commented May 11, 2022 •

edited