fix: add the resource name to the SubjectAccessReview (#10221)

Signed-off-by: Mariam Fahmy <mariam.fahmy@nirmata.com>
Co-authored-by: shuting <shuting@nirmata.com>

api/kyverno/v2beta1/match_resources_types.go

@@ -18,6 +18,14 @@ type MatchResources struct {
 	All kyvernov1.ResourceFilters `json:"all,omitempty" yaml:"all,omitempty"`
 }
 
+// GetResourceFilters returns all resource filters
+func (m *MatchResources) GetResourceFilters() kyvernov1.ResourceFilters {
+	var filters kyvernov1.ResourceFilters
+	filters = append(filters, m.All...)
+	filters = append(filters, m.Any...)
+	return filters
+}
+
 // GetKinds returns all kinds
 func (m *MatchResources) GetKinds() []string {
 	var kinds []string

pkg/auth/auth.go

@@ -32,13 +32,15 @@ type canIOptions struct {
 	gvk         string
 	subresource string
 	user        string
+	name        string
 	discovery   Discovery
 	checker     checker.AuthChecker
 }
 
 // NewCanI returns a new instance of operation access controller evaluator
-func NewCanI(discovery Discovery, sarClient authorizationv1client.SubjectAccessReviewInterface, gvk, namespace, verb, subresource string, user string) CanIOptions {
+func NewCanI(discovery Discovery, sarClient authorizationv1client.SubjectAccessReviewInterface, gvk, namespace, name, verb, subresource string, user string) CanIOptions {
 	return &canIOptions{
+		name:        name,
 		namespace:   namespace,
 		verb:        verb,
 		gvk:         gvk,
@@ -72,7 +74,7 @@ func (o *canIOptions) RunAccessCheck(ctx context.Context) (bool, string, error)
 		return false, "", fmt.Errorf("failed to get the Group Version Resource for kind %s", o.gvk)
 	}
 	logger := logger.WithValues("kind", kind, "namespace", o.namespace, "gvr", gvr.String(), "verb", o.verb)
-	result, err := o.checker.Check(ctx, gvr.Group, gvr.Version, gvr.Resource, o.subresource, o.namespace, o.verb)
+	result, err := o.checker.Check(ctx, gvr.Group, gvr.Version, gvr.Resource, o.subresource, o.namespace, o.name, o.verb)
 	if err != nil {
 		logger.Error(err, "failed to check permissions")
 		return false, "", err

pkg/auth/auth_test.go

@@ -17,6 +17,7 @@ func TestNewCanI(t *testing.T) {
 	type args struct {
 		client    dclient.Interface
 		kind      string
+		name      string
 		namespace string
 		verb      string
 	}
@@ -27,14 +28,24 @@ func TestNewCanI(t *testing.T) {
 		name: "deployments",
 		args: args{
 			client:    dclient.NewEmptyFakeClient(),
+			name:      "",
 			kind:      "Deployment",
 			namespace: "default",
 			verb:      "test",
 		},
+	}, {
+		name: "secrets",
+		args: args{
+			client:    dclient.NewEmptyFakeClient(),
+			name:      "test-secret",
+			kind:      "Secret",
+			namespace: "default",
+			verb:      "test",
+		},
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := NewCanI(tt.args.client.Discovery(), tt.args.client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), tt.args.kind, tt.args.namespace, tt.args.verb, "", "admin")
+			got := NewCanI(tt.args.client.Discovery(), tt.args.client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), tt.args.kind, tt.args.namespace, tt.args.verb, tt.args.name, "", "admin")
 			assert.NotNil(t, got)
 		})
 	}
@@ -48,6 +59,7 @@ func (d *discovery) GetGVRFromGVK(schema.GroupVersionKind) (schema.GroupVersionR
 
 func TestCanIOptions_DiscoveryError(t *testing.T) {
 	type fields struct {
+		name      string
 		namespace string
 		verb      string
 		kind      string
@@ -62,16 +74,28 @@ func TestCanIOptions_DiscoveryError(t *testing.T) {
 		name: "deployments",
 		fields: fields{
 			discovery: &discovery{},
+			name:      "",
 			kind:      "Deployment",
 			namespace: "default",
 			verb:      "test",
 		},
 		want:    false,
 		wantErr: true,
+	}, {
+		name: "secrets",
+		fields: fields{
+			discovery: &discovery{},
+			name:      "test-secret",
+			kind:      "Secret",
+			namespace: "default",
+			verb:      "test",
+		},
+		want:    false,
+		wantErr: true,
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			o := NewCanI(tt.fields.discovery, nil, tt.fields.kind, tt.fields.namespace, tt.fields.verb, "", "admin")
+			o := NewCanI(tt.fields.discovery, nil, tt.fields.kind, tt.fields.namespace, tt.fields.name, tt.fields.verb, "", "admin")
 			got, _, err := o.RunAccessCheck(context.TODO())
 			if tt.wantErr {
 				assert.Error(t, err)
@@ -91,6 +115,7 @@ func (d *sar) Create(_ context.Context, _ *v1.SubjectAccessReview, _ metav1.Crea
 
 func TestCanIOptions_SsarError(t *testing.T) {
 	type fields struct {
+		name      string
 		namespace string
 		verb      string
 		kind      string
@@ -107,16 +132,29 @@ func TestCanIOptions_SsarError(t *testing.T) {
 		fields: fields{
 			discovery: dclient.NewEmptyFakeClient().Discovery(),
 			sarClient: &sar{},
+			name:      "",
 			kind:      "Deployment",
 			namespace: "default",
 			verb:      "test",
 		},
 		want:    false,
 		wantErr: true,
+	}, {
+		name: "secrets",
+		fields: fields{
+			discovery: dclient.NewEmptyFakeClient().Discovery(),
+			sarClient: &sar{},
+			name:      "test-secret",
+			kind:      "Secret",
+			namespace: "default",
+			verb:      "test",
+		},
+		want:    false,
+		wantErr: true,
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			o := NewCanI(tt.fields.discovery, tt.fields.sarClient, tt.fields.kind, tt.fields.namespace, tt.fields.verb, "", "admin")
+			o := NewCanI(tt.fields.discovery, tt.fields.sarClient, tt.fields.kind, tt.fields.namespace, tt.fields.name, tt.fields.verb, "", "admin")
 			got, _, err := o.RunAccessCheck(context.TODO())
 			if tt.wantErr {
 				assert.Error(t, err)
@@ -130,6 +168,7 @@ func TestCanIOptions_SsarError(t *testing.T) {
 
 func TestCanIOptions_RunAccessCheck(t *testing.T) {
 	type fields struct {
+		name      string
 		namespace string
 		verb      string
 		kind      string
@@ -144,6 +183,7 @@ func TestCanIOptions_RunAccessCheck(t *testing.T) {
 		name: "deployments",
 		fields: fields{
 			client:    dclient.NewEmptyFakeClient(),
+			name:      "",
 			kind:      "Deployment",
 			namespace: "default",
 			verb:      "test",
@@ -154,6 +194,7 @@ func TestCanIOptions_RunAccessCheck(t *testing.T) {
 		name: "unknown",
 		fields: fields{
 			client:    dclient.NewEmptyFakeClient(),
+			name:      "",
 			kind:      "Unknown",
 			namespace: "default",
 			verb:      "test",
@@ -164,16 +205,28 @@ func TestCanIOptions_RunAccessCheck(t *testing.T) {
 		name: "v2 pods",
 		fields: fields{
 			client:    dclient.NewEmptyFakeClient(),
+			name:      "",
 			kind:      "v2/Pod",
 			namespace: "default",
 			verb:      "test",
 		},
 		want:    false,
 		wantErr: true,
+	}, {
+		name: "secrets",
+		fields: fields{
+			client:    dclient.NewEmptyFakeClient(),
+			name:      "test-secret",
+			kind:      "Secret",
+			namespace: "default",
+			verb:      "test",
+		},
+		want:    false,
+		wantErr: false,
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			o := NewCanI(tt.fields.client.Discovery(), tt.fields.client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), tt.fields.kind, tt.fields.namespace, tt.fields.verb, "", "admin")
+			o := NewCanI(tt.fields.client.Discovery(), tt.fields.client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), tt.fields.kind, tt.fields.namespace, tt.fields.name, tt.fields.verb, "", "admin")
 			got, _, err := o.RunAccessCheck(context.TODO())
 			if tt.wantErr {
 				assert.Error(t, err)

pkg/auth/checker/auth.go

@@ -16,7 +16,7 @@ type AuthResult struct {
 // AuthChecker provides utility to check authorization
 type AuthChecker interface {
 	// Check checks if the caller can perform an operation
-	Check(ctx context.Context, group, version, resource, subresource, namespace, verb string) (*AuthResult, error)
+	Check(ctx context.Context, group, version, resource, subresource, namespace, name, verb string) (*AuthResult, error)
 }
 
 func NewSelfChecker(client authorizationv1client.SelfSubjectAccessReviewInterface) AuthChecker {

pkg/auth/checker/helpers.go

@@ -6,7 +6,7 @@ import (
 
 func Check(ctx context.Context, checker AuthChecker, group, version, resource, subresource, namespace string, verbs ...string) (bool, error) {
 	for _, verb := range verbs {
-		result, err := checker.Check(ctx, group, version, resource, subresource, namespace, verb)
+		result, err := checker.Check(ctx, group, version, resource, subresource, namespace, "", verb)
 		if err != nil {
 			return false, err
 		}

pkg/auth/checker/self.go

@@ -12,7 +12,7 @@ type self struct {
 	client authorizationv1client.SelfSubjectAccessReviewInterface
 }
 
-func (c self) Check(ctx context.Context, group, version, resource, subresource, namespace, verb string) (*AuthResult, error) {
+func (c self) Check(ctx context.Context, group, version, resource, subresource, namespace, name, verb string) (*AuthResult, error) {
 	review := &authorizationv1.SelfSubjectAccessReview{
 		Spec: authorizationv1.SelfSubjectAccessReviewSpec{
 			ResourceAttributes: &authorizationv1.ResourceAttributes{
@@ -22,6 +22,7 @@ func (c self) Check(ctx context.Context, group, version, resource, subresource,
 				Subresource: subresource,
 				Namespace:   namespace,
 				Verb:        verb,
+				Name:        name,
 			},
 		},
 	}

pkg/auth/checker/subject.go

@@ -14,7 +14,7 @@ type subject struct {
 	groups []string
 }
 
-func (c subject) Check(ctx context.Context, group, version, resource, subresource, namespace, verb string) (*AuthResult, error) {
+func (c subject) Check(ctx context.Context, group, version, resource, subresource, namespace, name, verb string) (*AuthResult, error) {
 	review := &authorizationv1.SubjectAccessReview{
 		Spec: authorizationv1.SubjectAccessReviewSpec{
 			ResourceAttributes: &authorizationv1.ResourceAttributes{
@@ -24,6 +24,7 @@ func (c subject) Check(ctx context.Context, group, version, resource, subresourc
 				Subresource: subresource,
 				Namespace:   namespace,
 				Verb:        verb,
+				Name:        name,
 			},
 			User:   c.user,
 			Groups: c.groups,

pkg/engine/adapters/dclient.go

@@ -70,7 +70,7 @@ func (a *dclientAdapter) IsNamespaced(group, version, kind string) (bool, error)
 }
 
 func (a *dclientAdapter) CanI(ctx context.Context, kind, namespace, verb, subresource, user string) (bool, string, error) {
-	canI := auth.NewCanI(a.client.Discovery(), a.client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), kind, namespace, verb, subresource, user)
+	canI := auth.NewCanI(a.client.Discovery(), a.client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), kind, namespace, "", verb, subresource, user)
 	ok, reason, err := canI.RunAccessCheck(ctx)
 	if err != nil {
 		return false, reason, err

pkg/policy/auth/auth.go

@@ -39,7 +39,7 @@ func NewAuth(client dclient.Interface, user string, log logr.Logger) *Auth {
 
 // CanICreate returns 'true' if self can 'create' resource
 func (a *Auth) CanICreate(ctx context.Context, gvk, namespace, subresource string) (bool, error) {
-	canI := auth.NewCanI(a.client.Discovery(), a.client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), gvk, namespace, "create", "", a.user)
+	canI := auth.NewCanI(a.client.Discovery(), a.client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), gvk, namespace, "", "create", "", a.user)
 	ok, _, err := canI.RunAccessCheck(ctx)
 	if err != nil {
 		return false, err
@@ -49,7 +49,7 @@ func (a *Auth) CanICreate(ctx context.Context, gvk, namespace, subresource strin
 
 // CanIUpdate returns 'true' if self can 'update' resource
 func (a *Auth) CanIUpdate(ctx context.Context, gvk, namespace, subresource string) (bool, error) {
-	canI := auth.NewCanI(a.client.Discovery(), a.client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), gvk, namespace, "update", "", a.user)
+	canI := auth.NewCanI(a.client.Discovery(), a.client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), gvk, namespace, "", "update", "", a.user)
 	ok, _, err := canI.RunAccessCheck(ctx)
 	if err != nil {
 		return false, err
@@ -59,7 +59,7 @@ func (a *Auth) CanIUpdate(ctx context.Context, gvk, namespace, subresource strin
 
 // CanIDelete returns 'true' if self can 'delete' resource
 func (a *Auth) CanIDelete(ctx context.Context, gvk, namespace, subresource string) (bool, error) {
-	canI := auth.NewCanI(a.client.Discovery(), a.client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), gvk, namespace, "delete", "", a.user)
+	canI := auth.NewCanI(a.client.Discovery(), a.client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), gvk, namespace, "", "delete", "", a.user)
 	ok, _, err := canI.RunAccessCheck(ctx)
 	if err != nil {
 		return false, err
@@ -69,7 +69,7 @@ func (a *Auth) CanIDelete(ctx context.Context, gvk, namespace, subresource strin
 
 // CanIGet returns 'true' if self can 'get' resource
 func (a *Auth) CanIGet(ctx context.Context, gvk, namespace, subresource string) (bool, error) {
-	canI := auth.NewCanI(a.client.Discovery(), a.client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), gvk, namespace, "get", "", a.user)
+	canI := auth.NewCanI(a.client.Discovery(), a.client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), gvk, namespace, "", "get", "", a.user)
 	ok, _, err := canI.RunAccessCheck(ctx)
 	if err != nil {
 		return false, err

pkg/validation/cleanuppolicy/validate.go

@@ -70,9 +70,29 @@ func validatePolicy(clusterResources sets.Set[string], policy kyvernov2.CleanupP
 func validateAuth(ctx context.Context, client dclient.Interface, policy kyvernov2.CleanupPolicyInterface) error {
 	namespace := policy.GetNamespace()
 	spec := policy.GetSpec()
-	kinds := sets.New(spec.MatchResources.GetKinds()...)
+	resourceFilters := spec.MatchResources.GetResourceFilters()
-	for kind := range kinds {
+	for _, res := range resourceFilters {
-		checker := auth.NewCanI(client.Discovery(), client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), kind, namespace, "delete", "", config.KyvernoUserName(config.KyvernoServiceAccountName()))
+		for _, kind := range res.Kinds {
+			if len(res.Names) == 0 {
+				err := canI(ctx, client, kind, namespace, "", "")
+				if err != nil {
+					return err
+				}
+			} else {
+				for _, name := range res.Names {
+					err := canI(ctx, client, kind, namespace, name, "")
+					if err != nil {
+						return err
+					}
+				}
+			}
+		}
+	}
+	return nil
+}
+
+func canI(ctx context.Context, client dclient.Interface, kind, namespace, name, subresource string) error {
+	checker := auth.NewCanI(client.Discovery(), client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), kind, namespace, name, "delete", subresource, config.KyvernoUserName(config.KyvernoServiceAccountName()))
 	allowedDeletion, _, err := checker.RunAccessCheck(ctx)
 	if err != nil {
 		return err
@@ -81,7 +101,7 @@ func validateAuth(ctx context.Context, client dclient.Interface, policy kyvernov
 		return fmt.Errorf("cleanup controller has no permission to delete kind %s", kind)
 	}
 
-		checker = auth.NewCanI(client.Discovery(), client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), kind, namespace, "list", "", config.KyvernoUserName(config.KyvernoServiceAccountName()))
+	checker = auth.NewCanI(client.Discovery(), client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), kind, namespace, name, "list", subresource, config.KyvernoUserName(config.KyvernoServiceAccountName()))
 	allowedList, _, err := checker.RunAccessCheck(ctx)
 	if err != nil {
 		return err
@@ -89,7 +109,6 @@ func validateAuth(ctx context.Context, client dclient.Interface, policy kyvernov
 	if !allowedList {
 		return fmt.Errorf("cleanup controller has no permission to list kind %s", kind)
 	}
-	}
 	return nil
 }