From 2edd1028b8911662d10da8eae9b56af92b2aef4c Mon Sep 17 00:00:00 2001 From: Matt Finkel Date: Thu, 31 Oct 2024 18:10:58 +0000 Subject: [PATCH] Enable fine-grained update/delete RBAC enforcement by default (#19988) Change applications resource RBAC to use fine-grained update/delete enforcement by default. This allows us to enforce RBAC on the application itself, separately from the sub-resources related to it. (see also #18124, #20600) Signed-off-by: Matt Finkel --- assets/builtin-policy.csv | 4 +- docs/operator-manual/rbac.md | 15 +-- server/application/application.go | 6 +- server/application/application_test.go | 159 ++++++++++++++++++++++++- 4 files changed, 166 insertions(+), 18 deletions(-) diff --git a/assets/builtin-policy.csv b/assets/builtin-policy.csv index 28f565260d88d..088f5fbd08ad3 100644 --- a/assets/builtin-policy.csv +++ b/assets/builtin-policy.csv @@ -18,7 +18,9 @@ p, role:readonly, logs, get, */*, allow p, role:admin, applications, create, */*, allow p, role:admin, applications, update, */*, allow +p, role:admin, applications, update/*, */*, allow p, role:admin, applications, delete, */*, allow +p, role:admin, applications, delete/*, */*, allow p, role:admin, applications, sync, */*, allow p, role:admin, applications, override, */*, allow p, role:admin, applications, action/*, */*, allow @@ -47,4 +49,4 @@ p, role:admin, gpgkeys, delete, *, allow p, role:admin, exec, create, */*, allow g, role:admin, role:readonly -g, admin, role:admin \ No newline at end of file +g, admin, role:admin diff --git a/docs/operator-manual/rbac.md b/docs/operator-manual/rbac.md index 9b7775a65e3e5..0b485d51ce749 100644 --- a/docs/operator-manual/rbac.md +++ b/docs/operator-manual/rbac.md @@ -114,7 +114,7 @@ The `applications` resource is an [Application-Specific Policy](#application-spe #### Fine-grained Permissions for `update`/`delete` action -The `update` and `delete` actions, when granted on an application, will allow the user to perform the operation on the application itself **and** all of its resources. +The `update` and `delete` actions, when granted on an application, will allow the user to perform the operation on the application itself but not its resources. It can be desirable to only allow `update` or `delete` on specific resources within an application. To do so, when the action if performed on an application's resource, the `` will have the `////` format. @@ -148,15 +148,12 @@ p, example-user, applications, delete, default/prod-app, deny p, example-user, applications, delete/*/Pod/*/*, default/prod-app, allow ``` -!!! note - - It is not possible to deny fine-grained permissions for a sub-resource if the action was **explicitly allowed on the application**. - For instance, the following policies will **allow** a user to delete the Pod and any other resources in the application: +If we want to explicitly allow updates to the application, but deny updates to any sub-resources: - ```csv - p, example-user, applications, delete, default/prod-app, allow - p, example-user, applications, delete/*/Pod/*/*, default/prod-app, deny - ``` +```csv +p, example-user, applications, update, default/prod-app, allow +p, example-user, applications, update/*, default/prod-app, deny +``` #### The `action` action diff --git a/server/application/application.go b/server/application/application.go index 438e9c34064f1..0979517b4e73e 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -1346,12 +1346,10 @@ func (s *Server) getAppResources(ctx context.Context, a *v1alpha1.Application) ( } func (s *Server) getAppLiveResource(ctx context.Context, action string, q *application.ApplicationResourceRequest) (*v1alpha1.ResourceNode, *rest.Config, *v1alpha1.Application, error) { - a, _, err := s.getApplicationEnforceRBACInformer(ctx, action, q.GetProject(), q.GetAppNamespace(), q.GetName()) - if err != nil && errors.Is(err, argocommon.PermissionDeniedAPIError) && (action == rbacpolicy.ActionDelete || action == rbacpolicy.ActionUpdate) { - // If users dont have permission on the whole applications, maybe they have fine-grained access to the specific resources + if action == rbacpolicy.ActionDelete || action == rbacpolicy.ActionUpdate { action = fmt.Sprintf("%s/%s/%s/%s/%s", action, q.GetGroup(), q.GetKind(), q.GetNamespace(), q.GetResourceName()) - a, _, err = s.getApplicationEnforceRBACInformer(ctx, action, q.GetProject(), q.GetAppNamespace(), q.GetName()) } + a, _, err := s.getApplicationEnforceRBACInformer(ctx, action, q.GetProject(), q.GetAppNamespace(), q.GetName()) if err != nil { return nil, nil, nil, err } diff --git a/server/application/application_test.go b/server/application/application_test.go index b1a4d88ec082c..ad2b1f04960d3 100644 --- a/server/application/application_test.go +++ b/server/application/application_test.go @@ -1652,7 +1652,7 @@ func TestDeleteResourcesRBAC(t *testing.T) { p, test-user, applications, delete, default/test-app, allow `) _, err := appServer.DeleteResource(ctx, &req) - assert.EqualError(t, err, expectedErrorWhenDeleteAllowed) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) }) t.Run("delete with application permission but deny subresource", func(t *testing.T) { @@ -1661,7 +1661,7 @@ p, test-user, applications, delete, default/test-app, allow p, test-user, applications, delete/*, default/test-app, deny `) _, err := appServer.DeleteResource(ctx, &req) - assert.EqualError(t, err, expectedErrorWhenDeleteAllowed) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) }) t.Run("delete with subresource", func(t *testing.T) { @@ -1715,7 +1715,7 @@ func TestPatchResourcesRBAC(t *testing.T) { p, test-user, applications, update, default/test-app, allow `) _, err := appServer.PatchResource(ctx, &req) - assert.EqualError(t, err, expectedErrorWhenUpdateAllowed) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) }) t.Run("patch with application permission but deny subresource", func(t *testing.T) { @@ -1724,7 +1724,7 @@ p, test-user, applications, update, default/test-app, allow p, test-user, applications, update/*, default/test-app, deny `) _, err := appServer.PatchResource(ctx, &req) - assert.EqualError(t, err, expectedErrorWhenUpdateAllowed) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) }) t.Run("patch with subresource", func(t *testing.T) { @@ -1754,6 +1754,157 @@ p, test-user, applications, update/fake.io/PodTest/*, default/test-app, deny }) } +func TestUpdateApplicationRBAC(t *testing.T) { + ctx := context.Background() + // nolint:staticcheck + ctx = context.WithValue(ctx, "claims", &jwt.RegisteredClaims{Subject: "test-user"}) + testApp := newTestApp() + appServer := newTestAppServer(t, testApp) + appServer.enf.SetDefaultRole("") + testApp.Spec.Project = "" + + appSpecReq := application.ApplicationUpdateSpecRequest{ + Name: &testApp.Name, + AppNamespace: &testApp.Namespace, + Spec: &testApp.Spec, + } + + t.Run("can update application spec with update allow", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, */*, allow +`) + _, err := appServer.UpdateSpec(ctx, &appSpecReq) + require.NoError(t, err) + }) + + t.Run("cannot update application spec with sub-resource update allow", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update/*, default/test-app, allow +`) + _, err := appServer.UpdateSpec(ctx, &appSpecReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + }) + + resourceReq := application.ApplicationResourcePatchRequest{ + Name: &testApp.Name, + AppNamespace: &testApp.Namespace, + Group: strToPtr("fake.io"), + Kind: strToPtr("PodTest"), + Namespace: strToPtr("fake-ns"), + ResourceName: strToPtr("my-pod-test"), + } + + expectedErrorWhenUpdateAllowed := "rpc error: code = InvalidArgument desc = PodTest fake.io my-pod-test not found as part of application test-app" + + t.Run("can update application spec with update allow, sub-resource update deny", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, allow +p, test-user, applications, update/fake.io/PodTest/*, default/test-app, deny +`) + _, err := appServer.UpdateSpec(ctx, &appSpecReq) + require.NoError(t, err) + _, err = appServer.PatchResource(ctx, &resourceReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + }) + + t.Run("can update application spec with update allow, sub-resource update allow", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, allow +p, test-user, applications, update/fake.io/PodTest/*, default/test-app, allow +`) + _, err := appServer.UpdateSpec(ctx, &appSpecReq) + require.NoError(t, err) + _, err = appServer.PatchResource(ctx, &resourceReq) + assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error()) + }) + + t.Run("cannot update application spec with update deny, sub-resource update allow", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, deny +p, test-user, applications, update/fake.io/PodTest/*, default/test-app, allow +`) + _, err := appServer.UpdateSpec(ctx, &appSpecReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + _, err = appServer.PatchResource(ctx, &resourceReq) + assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error()) + }) + + t.Run("cannot update application spec with update deny, sub-resource update deny", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, deny +p, test-user, applications, update/fake.io/PodTest/*, default/test-app, deny +`) + _, err := appServer.UpdateSpec(ctx, &appSpecReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + _, err = appServer.PatchResource(ctx, &resourceReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + }) + + appReq := application.ApplicationUpdateRequest{ + Application: testApp, + } + + t.Run("update application with generic permission", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, allow +`) + _, err := appServer.Update(ctx, &appReq) + require.NoError(t, err) + }) + + t.Run("cannot update application with sub-resource permission", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update/*, default/test-app, allow +`) + _, err := appServer.Update(ctx, &appReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + }) + + t.Run("can update application with update allow, sub-resource update deny", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, allow +p, test-user, applications, update/fake.io/PodTest/*, default/test-app, deny +`) + _, err := appServer.Update(ctx, &appReq) + require.NoError(t, err) + _, err = appServer.PatchResource(ctx, &resourceReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + }) + + t.Run("can update application with update allow, sub-resource update allow", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, allow +p, test-user, applications, update/fake.io/PodTest/*, default/test-app, allow +`) + _, err := appServer.Update(ctx, &appReq) + require.NoError(t, err) + _, err = appServer.PatchResource(ctx, &resourceReq) + assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error()) + }) + + t.Run("cannot update application with update deny, sub-resource update allow", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, deny +p, test-user, applications, update/fake.io/PodTest/*, default/test-app, allow +`) + _, err := appServer.Update(ctx, &appReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + _, err = appServer.PatchResource(ctx, &resourceReq) + assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error()) + }) + + t.Run("cannot update application with update deny, sub-resource update deny", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, deny +p, test-user, applications, update/fake.io/PodTest/*, default/test-app, deny +`) + _, err := appServer.Update(ctx, &appReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + _, err = appServer.PatchResource(ctx, &resourceReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + }) +} + func TestSyncAndTerminate(t *testing.T) { ctx := context.Background() appServer := newTestAppServer(t)