fix: Apply deletion permission checks when syncing with replace (#14161)

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):
@@ -15,6 +16,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
@@ -43,4 +52,8 @@ p, role:admin, gpgkeys, delete, *, allow
 p, role:admin, exec, create, */*, allow
 
 g, role:admin, role:readonly
-g, admin, role:admin
+g, role:applicationsyncer, role:readonly
+g, role:applicationdeleter, role:applicationsyncer
+g, role:deploymentdeleter, role:applicationsyncer
+g, role:resourcesdeleter, role:applicationsyncer
+g, admin, role:admin

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

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:

server/application/application.go

@@ -1953,6 +1953,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: replace is enabled, 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: replace is enabled, but resource delete permissions denied")
+					}
+				}
+			}
+		}
+	}
+
 	op := appv1.Operation{
 		Sync: &appv1.SyncOperation{
 			Revision:     revision,

server/application/application_test.go

@@ -1863,6 +1863,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: replace is enabled, 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: replace is enabled, 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 = []appsv1.RevisionHistory{{