From a980b13491c7c39cc4c52024d7de0972839dcd07 Mon Sep 17 00:00:00 2001 From: Andrii Korotkov Date: Fri, 8 Nov 2024 06:58:45 -0800 Subject: [PATCH] fix: Apply deletion permission checks when syncing with replace (#14161) Closes #14161 Sync with replace can be quite dangerous, but we currently allow it if the sync is allowed. Since sync with replace deletes resources and recreates them, it's reasonable to check deletion permissions for application and/or specific resources. Let's cherry-pick this to 2.13. Signed-off-by: Andrii Korotkov --- assets/builtin-policy.csv | 17 +++- docs/operator-manual/rbac.md | 15 +++ docs/user-guide/sync-options.md | 3 + server/application/application.go | 25 +++++ server/application/application_test.go | 127 +++++++++++++++++++++++++ 5 files changed, 185 insertions(+), 2 deletions(-) diff --git a/assets/builtin-policy.csv b/assets/builtin-policy.csv index 28f565260d88d..fbf98985d330e 100644 --- a/assets/builtin-policy.csv +++ b/assets/builtin-policy.csv @@ -1,4 +1,5 @@ -# Built-in policy which defines two roles: role:readonly and role:admin, +# Built-in policy which defines several roles: role:readonly, role:applicationsyncer, +# role:applicationdeleter, role:deploymentdeleter, role:resourcesdeleter and role:admin, # and additionally assigns the admin user to the role:admin role. # There are two policy formats: # 1. Applications, applicationsets, logs, and exec (which belong to a project): @@ -16,6 +17,14 @@ p, role:readonly, accounts, get, *, allow p, role:readonly, gpgkeys, get, *, allow p, role:readonly, logs, get, */*, allow +p, role:applicationsyncer, applications, sync, */*, allow + +p, role:applicationdeleter, applications, delete, */*, allow + +p, role:deploymentdeleter, applications, delete/*/Deployment/*, */*, allow + +p, role:resourcesdeleter, applications, delete/*, */*, allow + p, role:admin, applications, create, */*, allow p, role:admin, applications, update, */*, allow p, role:admin, applications, delete, */*, allow @@ -47,4 +56,8 @@ 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, role:applicationsyncer, role:readonly +g, role:applicationdeleter, role:applicationsyncer +g, role:deploymentdeleter, role:applicationsyncer +g, role:resourcesdeleter, role:applicationsyncer +g, admin, role:admin diff --git a/docs/operator-manual/rbac.md b/docs/operator-manual/rbac.md index 9b7775a65e3e5..9a97c4fc0868b 100644 --- a/docs/operator-manual/rbac.md +++ b/docs/operator-manual/rbac.md @@ -158,6 +158,21 @@ p, example-user, applications, delete/*/Pod/*/*, default/prod-app, allow p, example-user, applications, delete/*/Pod/*/*, default/prod-app, deny ``` +!!! note + + The deletion permissions are generally necessary to do a sync with replace, since that deletes and recreates the objects. + However, the checks are not done when replace option is specified via application/resource annotations. This would be hard to + implement, but also may offer a bit of extra protection by PR reviews for example. This is also a way to avoid giving users + too broad delete permissions if only specific resources need to be synced with replace. Here are examples of how you can configure + deletion options necessary for various syncs replace: + + ```csv + p, example-user-whole-app-sync-with-replace, applications, delete/*, */*, allow + p, example-user-deployment-sync-with-replace, applications, delete/*/Deployment/*, */*, allow + ``` + + We don't have to grant a permission to delete a whole application. + #### The `action` action The `action` action corresponds to either built-in resource customizations defined diff --git a/docs/user-guide/sync-options.md b/docs/user-guide/sync-options.md index 206b0b690b0d3..c7795bf31e676 100644 --- a/docs/user-guide/sync-options.md +++ b/docs/user-guide/sync-options.md @@ -186,6 +186,9 @@ If the `Replace=true` sync option is set the Argo CD will use `kubectl replace` During the sync process, the resources will be synchronized using the 'kubectl replace/create' command. This sync option has the potential to be destructive and might lead to resources having to be recreated, which could cause an outage for your application. +!!! warning + Using these annotations would bypass the RBAC checks for resources deletion which are normally applied when syncing with replace. + This can also be configured at individual resource level. ```yaml metadata: diff --git a/server/application/application.go b/server/application/application.go index 438e9c34064f1..9770c5c5dddf0 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -1964,6 +1964,31 @@ func (s *Server) Sync(ctx context.Context, syncReq *application.ApplicationSyncR } } } + + // Extra checks for sync with replace, to see if people have permissions to delete resources. Note, that this doesn't check for + // sync with replace set via resource annotations. For now, people would have to rely on manifests peer reviews to ensure the + // sync with replace would be intended if set that way. The proper permissions check for the later is tricky to support. + if syncOptions.HasOption(common.SyncOptionReplace) { + // If application-level delete policy is allow, we don't check for specific resources' permissions and just allow. + if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceApplications, rbacpolicy.ActionDelete, a.RBACName(s.ns)); err != nil { + if len(resources) == 0 { + // If no resources are specified, check for ability to delete any resource + action := fmt.Sprintf("%s/*/*/*/*", rbacpolicy.ActionDelete) + if resourcesErr := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceApplications, action, a.RBACName(s.ns)); resourcesErr != nil { + return nil, status.Errorf(codes.PermissionDenied, "cannot sync: sync with replace requested, but resource delete permissions denied") + } + } else { + // Otherwise, check for specific resources + for _, resource := range resources { + action := fmt.Sprintf("%s/%s/%s/%s/%s", rbacpolicy.ActionDelete, resource.Group, resource.Kind, resource.Namespace, resource.Name) + if resourceErr := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceApplications, action, a.RBACName(s.ns)); resourceErr != nil { + return nil, status.Errorf(codes.PermissionDenied, "cannot sync: sync with replace requested, but resource delete permissions denied") + } + } + } + } + } + op := v1alpha1.Operation{ Sync: &v1alpha1.SyncOperation{ Revision: revision, diff --git a/server/application/application_test.go b/server/application/application_test.go index b1a4d88ec082c..1d38725e849dc 100644 --- a/server/application/application_test.go +++ b/server/application/application_test.go @@ -1846,6 +1846,133 @@ func TestSyncGit(t *testing.T) { assert.Equal(t, "Unknown user initiated sync locally", events.Items[1].Message) } +func getArtifactsForSyncWithReplacePermissionsTest(t *testing.T, role string) (*Server, *appsv1.Application) { + t.Helper() + f := func(enf *rbac.Enforcer) { + _ = enf.SetBuiltinPolicy(assets.BuiltinPolicyCSV) + enf.SetDefaultRole(fmt.Sprintf("role:%s", role)) + } + deployment := k8sappsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apps/v1", + Kind: "Deployment", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "test", + }, + } + testApp := newTestApp(func(app *appsv1.Application) { + app.Name = "test" + app.Status.Resources = []appsv1.ResourceStatus{ + { + Group: deployment.GroupVersionKind().Group, + Kind: deployment.GroupVersionKind().Kind, + Version: deployment.GroupVersionKind().Version, + Name: deployment.Name, + Namespace: deployment.Namespace, + Status: "Synced", + }, + } + app.Status.History = []appsv1.RevisionHistory{ + { + ID: 0, + Source: appsv1.ApplicationSource{ + TargetRevision: "something-old", + }, + }, + } + }) + testDeployment := kube.MustToUnstructured(&deployment) + return newTestAppServerWithEnforcerConfigure(t, f, map[string]string{}, testApp, testDeployment), testApp +} + +func TestSyncWithReplacePermissionsWithAppDeletionAllowed(t *testing.T) { + ctx := context.Background() + appServer, testApp := getArtifactsForSyncWithReplacePermissionsTest(t, "applicationdeleter") + _, err := appServer.Sync( + ctx, + &application.ApplicationSyncRequest{ + Name: &testApp.Name, + SyncOptions: &application.SyncOptions{Items: []string{synccommon.SyncOptionReplace}}, + }, + ) + require.NoError(t, err) + + unsetSyncRunningOperationState(t, appServer) + + _, err = appServer.Sync( + ctx, + &application.ApplicationSyncRequest{ + Name: &testApp.Name, + SyncOptions: &application.SyncOptions{Items: []string{synccommon.SyncOptionReplace}}, + Resources: []*appv1.SyncOperationResource{{Group: "apps", Kind: "Deployment", Namespace: "test", Name: "test"}}, + }, + ) + require.NoError(t, err) +} + +func TestSyncWithReplacePermissionsWithDeploymentDeletionAllowed(t *testing.T) { + ctx := context.Background() + appServer, testApp := getArtifactsForSyncWithReplacePermissionsTest(t, "deploymentdeleter") + _, err := appServer.Sync( + ctx, + &application.ApplicationSyncRequest{ + Name: &testApp.Name, + SyncOptions: &application.SyncOptions{Items: []string{synccommon.SyncOptionReplace}}, + }, + ) + require.EqualError(t, err, "rpc error: code = PermissionDenied desc = cannot sync: sync with replace requested, but resource delete permissions denied") + + _, err = appServer.Sync( + ctx, + &application.ApplicationSyncRequest{ + Name: &testApp.Name, + SyncOptions: &application.SyncOptions{Items: []string{synccommon.SyncOptionReplace}}, + Resources: []*appv1.SyncOperationResource{ + {Group: "apps", Kind: "Deployment", Namespace: "test", Name: "test"}, + {Group: "", Kind: "ConfigMap", Namespace: "test", Name: "does-not-exist"}, + }, + }, + ) + require.EqualError(t, err, "rpc error: code = PermissionDenied desc = cannot sync: sync with replace requested, but resource delete permissions denied") + + _, err = appServer.Sync( + ctx, + &application.ApplicationSyncRequest{ + Name: &testApp.Name, + SyncOptions: &application.SyncOptions{Items: []string{synccommon.SyncOptionReplace}}, + Resources: []*appv1.SyncOperationResource{{Group: "apps", Kind: "Deployment", Namespace: "test", Name: "test"}}, + }, + ) + require.NoError(t, err) +} + +func TestSyncWithReplacePermissionsWithResourcesDeletionAllowed(t *testing.T) { + ctx := context.Background() + appServer, testApp := getArtifactsForSyncWithReplacePermissionsTest(t, "resourcesdeleter") + _, err := appServer.Sync( + ctx, + &application.ApplicationSyncRequest{ + Name: &testApp.Name, + SyncOptions: &application.SyncOptions{Items: []string{synccommon.SyncOptionReplace}}, + }, + ) + require.NoError(t, err) + + unsetSyncRunningOperationState(t, appServer) + + _, err = appServer.Sync( + ctx, + &application.ApplicationSyncRequest{ + Name: &testApp.Name, + SyncOptions: &application.SyncOptions{Items: []string{synccommon.SyncOptionReplace}}, + Resources: []*appv1.SyncOperationResource{{Group: "apps", Kind: "Deployment", Namespace: "test", Name: "test"}}, + }, + ) + require.NoError(t, err) +} + func TestRollbackApp(t *testing.T) { testApp := newTestApp() testApp.Status.History = []v1alpha1.RevisionHistory{{