From ac12817b491589f29a44a1b414139369d105057a Mon Sep 17 00:00:00 2001 From: Pratik Jagrut Date: Wed, 3 Dec 2025 22:20:23 +0530 Subject: [PATCH] Backport v2.13: Use correct name while creating SubjectAccessReview for SA impersonation check (#52789) --- pkg/auth/requests/sar/sar.go | 25 ++++++- pkg/auth/requests/sar/sar_test.go | 104 +++++++++++++++++++----------- 2 files changed, 89 insertions(+), 40 deletions(-) diff --git a/pkg/auth/requests/sar/sar.go b/pkg/auth/requests/sar/sar.go index dd9df2357..bf390c3cd 100644 --- a/pkg/auth/requests/sar/sar.go +++ b/pkg/auth/requests/sar/sar.go @@ -2,11 +2,14 @@ package sar import ( "context" + "fmt" "net/http" + "strings" "github.com/sirupsen/logrus" authV1 "k8s.io/api/authorization/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apiserver/pkg/authentication/serviceaccount" v1 "k8s.io/client-go/kubernetes/typed/authorization/v1" ) @@ -142,13 +145,18 @@ func (sar subjectAccessReview) checkUserCanImpersonateExtras(ctx context.Context } func (sar subjectAccessReview) checkUserCanImpersonateServiceAccount(ctx context.Context, sarClient v1.SubjectAccessReviewInterface, user string, sa string) (bool, error) { + saNamespace, saName, err := parseServiceAccountUsername(sa) + if err != nil { + return false, err + } review := authV1.SubjectAccessReview{ Spec: authV1.SubjectAccessReviewSpec{ User: user, ResourceAttributes: &authV1.ResourceAttributes{ - Verb: "impersonate", - Resource: "serviceaccounts", - Name: sa, + Verb: "impersonate", + Resource: "serviceaccounts", + Namespace: saNamespace, + Name: saName, }, }, } @@ -160,3 +168,14 @@ func (sar subjectAccessReview) checkUserCanImpersonateServiceAccount(ctx context logrus.Debugf("Impersonate sa check result: %v", result) return result.Status.Allowed, nil } + +func parseServiceAccountUsername(username string) (namespace string, name string, err error) { + namespacedName := strings.TrimPrefix(username, serviceaccount.ServiceAccountUsernamePrefix) + tokens := strings.Split(namespacedName, ":") + if len(tokens) != 2 { + return "", "", fmt.Errorf("invalid service account username format: expected system:serviceaccount::, but got '%s'", username) + } + namespace = tokens[0] + name = tokens[1] + return namespace, name, nil +} diff --git a/pkg/auth/requests/sar/sar_test.go b/pkg/auth/requests/sar/sar_test.go index 4d38d63bf..db6cc2fe3 100644 --- a/pkg/auth/requests/sar/sar_test.go +++ b/pkg/auth/requests/sar/sar_test.go @@ -1,6 +1,7 @@ package sar import ( + "fmt" "net/http" "testing" @@ -20,7 +21,7 @@ func TestUserCanImpersonateUser(t *testing.T) { user string impUser string expectedAuthed bool - expecterErr error + expectedErr error }{ "can impersonate": { sarClientGetterMock: func(req *http.Request, user string, impUser string) SubjectAccessReviewClientGetter { @@ -49,7 +50,7 @@ func TestUserCanImpersonateUser(t *testing.T) { user: "user", impUser: "impUser", expectedAuthed: true, - expecterErr: nil, + expectedErr: nil, }, "impersonate not allowed": { sarClientGetterMock: func(req *http.Request, user string, impUser string) SubjectAccessReviewClientGetter { @@ -78,7 +79,7 @@ func TestUserCanImpersonateUser(t *testing.T) { user: "user", impUser: "impUser", expectedAuthed: false, - expecterErr: nil, + expectedErr: nil, }, "impersonate error": { sarClientGetterMock: func(req *http.Request, user string, impUser string) SubjectAccessReviewClientGetter { @@ -103,7 +104,7 @@ func TestUserCanImpersonateUser(t *testing.T) { user: "user", impUser: "impUser", expectedAuthed: false, - expecterErr: err, + expectedErr: err, }, } @@ -114,7 +115,7 @@ func TestUserCanImpersonateUser(t *testing.T) { sar := NewSubjectAccessReview(test.sarClientGetterMock(req, test.user, test.impUser)) authed, err := sar.UserCanImpersonateUser(req, test.user, test.impUser) - assert.Equal(t, test.expecterErr, err) + assert.Equal(t, test.expectedErr, err) assert.Equal(t, test.expectedAuthed, authed) }) } @@ -128,7 +129,7 @@ func TestUserCanImpersonateGroup(t *testing.T) { user string group string expectedAuthed bool - expecterErr error + expectedErr error }{ "can impersonate": { sarClientGetterMock: func(req *http.Request, user string, impGroup string) SubjectAccessReviewClientGetter { @@ -157,7 +158,7 @@ func TestUserCanImpersonateGroup(t *testing.T) { user: "user", group: "admin", expectedAuthed: true, - expecterErr: nil, + expectedErr: nil, }, "impersonate not allowed": { sarClientGetterMock: func(req *http.Request, user string, impGroup string) SubjectAccessReviewClientGetter { @@ -186,7 +187,7 @@ func TestUserCanImpersonateGroup(t *testing.T) { user: "user", group: "admin", expectedAuthed: false, - expecterErr: nil, + expectedErr: nil, }, "impersonate error": { sarClientGetterMock: func(req *http.Request, user string, impGroup string) SubjectAccessReviewClientGetter { @@ -211,7 +212,7 @@ func TestUserCanImpersonateGroup(t *testing.T) { user: "user", group: "admin", expectedAuthed: false, - expecterErr: err, + expectedErr: err, }, } @@ -222,7 +223,7 @@ func TestUserCanImpersonateGroup(t *testing.T) { sar := NewSubjectAccessReview(test.sarClientGetterMock(req, test.user, test.group)) authed, err := sar.UserCanImpersonateGroup(req, test.user, test.group) - assert.Equal(t, test.expecterErr, err) + assert.Equal(t, test.expectedErr, err) assert.Equal(t, test.expectedAuthed, authed) }) } @@ -236,7 +237,7 @@ func TestUserCanImpersonateExtras(t *testing.T) { user string extras map[string][]string expectedAuthed bool - expecterErr error + expectedErr error }{ "can impersonate": { sarClientGetterMock: func(req *http.Request, impExtras map[string][]string, user string) SubjectAccessReviewClientGetter { @@ -270,7 +271,7 @@ func TestUserCanImpersonateExtras(t *testing.T) { user: "user", extras: map[string][]string{"extra": {"extra1", "extra2"}}, expectedAuthed: true, - expecterErr: nil, + expectedErr: nil, }, "impersonate not allowed": { sarClientGetterMock: func(req *http.Request, impExtras map[string][]string, user string) SubjectAccessReviewClientGetter { @@ -304,7 +305,7 @@ func TestUserCanImpersonateExtras(t *testing.T) { user: "user", extras: map[string][]string{"extra": {"extra1"}}, expectedAuthed: false, - expecterErr: nil, + expectedErr: nil, }, "impersonate error": { sarClientGetterMock: func(req *http.Request, impExtras map[string][]string, user string) SubjectAccessReviewClientGetter { @@ -333,7 +334,7 @@ func TestUserCanImpersonateExtras(t *testing.T) { user: "user", extras: map[string][]string{"extra": {"extra1"}}, expectedAuthed: false, - expecterErr: err, + expectedErr: err, }, } @@ -344,7 +345,7 @@ func TestUserCanImpersonateExtras(t *testing.T) { sar := NewSubjectAccessReview(test.sarClientGetterMock(req, test.extras, test.user)) authed, err := sar.UserCanImpersonateExtras(req, test.user, test.extras) - assert.Equal(t, test.expecterErr, err) + assert.Equal(t, test.expectedErr, err) assert.Equal(t, test.expectedAuthed, authed) }) } @@ -354,14 +355,16 @@ func TestUserCanImpersonateServiceAccount(t *testing.T) { ctrl := gomock.NewController(t) err := errors.New("unexpected error") tests := map[string]struct { - sarClientGetterMock func(req *http.Request, user string, impSA string) SubjectAccessReviewClientGetter + sarClientGetterMock func(req *http.Request, user string, impSA string, ns string, name string) SubjectAccessReviewClientGetter user string impSA string + saNs string + saName string expectedAuthed bool - expecterErr error + expectedErr error }{ "can impersonate": { - sarClientGetterMock: func(req *http.Request, user string, impSA string) SubjectAccessReviewClientGetter { + sarClientGetterMock: func(req *http.Request, user string, impSA string, ns string, name string) SubjectAccessReviewClientGetter { sarClientMock := mocks.NewMockSubjectAccessReviewInterface(ctrl) sarMock := mocks.NewMockSubjectAccessReviewClientGetter(ctrl) sarMock.EXPECT().SubjectAccessReviewForCluster(req).Return(sarClientMock, nil) @@ -370,9 +373,10 @@ func TestUserCanImpersonateServiceAccount(t *testing.T) { Spec: authV1.SubjectAccessReviewSpec{ User: user, ResourceAttributes: &authV1.ResourceAttributes{ - Verb: "impersonate", - Resource: "serviceaccounts", - Name: impSA, + Verb: "impersonate", + Resource: "serviceaccounts", + Namespace: ns, + Name: name, }, }, } @@ -385,12 +389,14 @@ func TestUserCanImpersonateServiceAccount(t *testing.T) { return sarMock }, user: "user", - impSA: "impSA", + impSA: "system:serviceaccount:example-ns:example-test", + saNs: "example-ns", + saName: "example-test", expectedAuthed: true, - expecterErr: nil, + expectedErr: nil, }, "impersonate not allowed": { - sarClientGetterMock: func(req *http.Request, user string, impSA string) SubjectAccessReviewClientGetter { + sarClientGetterMock: func(req *http.Request, user string, impSA string, ns string, name string) SubjectAccessReviewClientGetter { sarClientMock := mocks.NewMockSubjectAccessReviewInterface(ctrl) sarMock := mocks.NewMockSubjectAccessReviewClientGetter(ctrl) sarMock.EXPECT().SubjectAccessReviewForCluster(req).Return(sarClientMock, nil) @@ -399,9 +405,10 @@ func TestUserCanImpersonateServiceAccount(t *testing.T) { Spec: authV1.SubjectAccessReviewSpec{ User: user, ResourceAttributes: &authV1.ResourceAttributes{ - Verb: "impersonate", - Resource: "serviceaccounts", - Name: impSA, + Verb: "impersonate", + Resource: "serviceaccounts", + Namespace: ns, + Name: name, }, }, } @@ -414,12 +421,14 @@ func TestUserCanImpersonateServiceAccount(t *testing.T) { return sarMock }, user: "user", - impSA: "impSA", + impSA: "system:serviceaccount:example-ns:example-test", + saNs: "example-ns", + saName: "example-test", expectedAuthed: false, - expecterErr: nil, + expectedErr: nil, }, "impersonate error": { - sarClientGetterMock: func(req *http.Request, user string, impSA string) SubjectAccessReviewClientGetter { + sarClientGetterMock: func(req *http.Request, user string, impSA string, ns string, name string) SubjectAccessReviewClientGetter { sarClientMock := mocks.NewMockSubjectAccessReviewInterface(ctrl) sarMock := mocks.NewMockSubjectAccessReviewClientGetter(ctrl) sarMock.EXPECT().SubjectAccessReviewForCluster(req).Return(sarClientMock, nil) @@ -428,9 +437,10 @@ func TestUserCanImpersonateServiceAccount(t *testing.T) { Spec: authV1.SubjectAccessReviewSpec{ User: user, ResourceAttributes: &authV1.ResourceAttributes{ - Verb: "impersonate", - Resource: "serviceaccounts", - Name: impSA, + Verb: "impersonate", + Resource: "serviceaccounts", + Namespace: ns, + Name: name, }, }, } @@ -439,9 +449,25 @@ func TestUserCanImpersonateServiceAccount(t *testing.T) { return sarMock }, user: "user", - impSA: "impUser", + impSA: "system:serviceaccount:example-ns:example-test", + saNs: "example-ns", + saName: "example-test", expectedAuthed: false, - expecterErr: err, + expectedErr: err, + }, + "impersonate parsing error": { + sarClientGetterMock: func(req *http.Request, user string, impSA string, ns string, name string) SubjectAccessReviewClientGetter { + sarClientMock := mocks.NewMockSubjectAccessReviewInterface(ctrl) + sarMock := mocks.NewMockSubjectAccessReviewClientGetter(ctrl) + sarMock.EXPECT().SubjectAccessReviewForCluster(req).Return(sarClientMock, nil) + return sarMock + }, + user: "user", + impSA: "system:serviceaccount:example-test", + saNs: "example-ns", + saName: "example-test", + expectedAuthed: false, + expectedErr: fmt.Errorf("invalid service account username format: expected system:serviceaccount::, but got 'system:serviceaccount:example-test'"), }, } @@ -449,10 +475,14 @@ func TestUserCanImpersonateServiceAccount(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() req := &http.Request{} - sar := NewSubjectAccessReview(test.sarClientGetterMock(req, test.user, test.impSA)) + sar := NewSubjectAccessReview(test.sarClientGetterMock(req, test.user, test.impSA, test.saNs, test.saName)) authed, err := sar.UserCanImpersonateServiceAccount(req, test.user, test.impSA) - assert.Equal(t, test.expecterErr, err) + if test.expectedErr != nil { + assert.EqualError(t, err, test.expectedErr.Error()) + } else { + assert.Nil(t, err) + } assert.Equal(t, test.expectedAuthed, authed) }) }