From 4d1f040e499aa489a817d1026b7fc63245d860c4 Mon Sep 17 00:00:00 2001 From: Mariam Fahmy Date: Wed, 7 Aug 2024 15:46:44 +0300 Subject: [PATCH] fix: add the resource name to the SubjectAccessReview (#10221) Signed-off-by: Mariam Fahmy Co-authored-by: shuting --- api/kyverno/v2beta1/match_resources_types.go | 8 +++ pkg/auth/auth.go | 6 +- pkg/auth/auth_test.go | 61 ++++++++++++++++++-- pkg/auth/checker/auth.go | 2 +- pkg/auth/checker/helpers.go | 2 +- pkg/auth/checker/self.go | 3 +- pkg/auth/checker/subject.go | 3 +- pkg/engine/adapters/dclient.go | 2 +- pkg/policy/auth/auth.go | 8 +-- pkg/validation/cleanuppolicy/validate.go | 53 +++++++++++------ 10 files changed, 116 insertions(+), 32 deletions(-) diff --git a/api/kyverno/v2beta1/match_resources_types.go b/api/kyverno/v2beta1/match_resources_types.go index 7b6ace8ad1..d3a3c995b6 100644 --- a/api/kyverno/v2beta1/match_resources_types.go +++ b/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 diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 1e5c3bda13..0c0b0549a2 100644 --- a/pkg/auth/auth.go +++ b/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 diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index ceabe8ba2a..eb6be4d774 100644 --- a/pkg/auth/auth_test.go +++ b/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) diff --git a/pkg/auth/checker/auth.go b/pkg/auth/checker/auth.go index cc7e5dfed0..e978ec828b 100644 --- a/pkg/auth/checker/auth.go +++ b/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 { diff --git a/pkg/auth/checker/helpers.go b/pkg/auth/checker/helpers.go index cbc0150c01..0d698d57cf 100644 --- a/pkg/auth/checker/helpers.go +++ b/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 } diff --git a/pkg/auth/checker/self.go b/pkg/auth/checker/self.go index 98d541b666..18ac7ffb5e 100644 --- a/pkg/auth/checker/self.go +++ b/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, }, }, } diff --git a/pkg/auth/checker/subject.go b/pkg/auth/checker/subject.go index 6ee15bf663..c5d4a40400 100644 --- a/pkg/auth/checker/subject.go +++ b/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, diff --git a/pkg/engine/adapters/dclient.go b/pkg/engine/adapters/dclient.go index 384574e691..ab0e9b9646 100644 --- a/pkg/engine/adapters/dclient.go +++ b/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 diff --git a/pkg/policy/auth/auth.go b/pkg/policy/auth/auth.go index 1298d67e42..8fff8579b3 100644 --- a/pkg/policy/auth/auth.go +++ b/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 diff --git a/pkg/validation/cleanuppolicy/validate.go b/pkg/validation/cleanuppolicy/validate.go index c77b550663..99da17f89c 100644 --- a/pkg/validation/cleanuppolicy/validate.go +++ b/pkg/validation/cleanuppolicy/validate.go @@ -70,25 +70,44 @@ 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()...) - for kind := range kinds { - checker := auth.NewCanI(client.Discovery(), client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), kind, namespace, "delete", "", config.KyvernoUserName(config.KyvernoServiceAccountName())) - allowedDeletion, _, err := checker.RunAccessCheck(ctx) - if err != nil { - return err - } - if !allowedDeletion { - return fmt.Errorf("cleanup controller has no permission to delete kind %s", kind) + resourceFilters := spec.MatchResources.GetResourceFilters() + for _, res := range resourceFilters { + 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 +} - checker = auth.NewCanI(client.Discovery(), client.GetKubeClient().AuthorizationV1().SubjectAccessReviews(), kind, namespace, "list", "", config.KyvernoUserName(config.KyvernoServiceAccountName())) - allowedList, _, err := checker.RunAccessCheck(ctx) - if err != nil { - return err - } - if !allowedList { - return fmt.Errorf("cleanup controller has no permission to list kind %s", kind) - } +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 + } + if !allowedDeletion { + return fmt.Errorf("cleanup controller has no permission to delete kind %s", kind) + } + + 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 + } + if !allowedList { + return fmt.Errorf("cleanup controller has no permission to list kind %s", kind) } return nil }