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 <[email protected]>

assets/builtin-policy.csv

@@ -1,5 +1,6 @@
-# Built-in policy which defines two roles: role:readonly and role:admin,
-# and additionally assigns the admin user to the role:admin role.
+# Built-in policy which defines several roles: role:readonly, role:applicationdeleter,
+# role:deploymentdeleter 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):
 # p, <role/user/group>, <resource>, <action>, <project>/<object>, <allow/deny>
@@ -15,6 +16,12 @@ p, role:readonly, accounts, get, *, allow
 p, role:readonly, gpgkeys, get, *, allow
 p, role:readonly, logs, get, */*, allow
 
+p, role:applicationdeleter, applications, delete, */*, allow
+p, role:applicationdeleter, applications, sync, */*, allow
+
+p, role:deploymentdeleter, applications, delete/*/Deployment/*, */*, allow
+p, role:deploymentdeleter, applications, sync, */*, allow
+
 p, role:admin, applications, create, */*, allow
 p, role:admin, applications, update, */*, allow
 p, role:admin, applications, delete, */*, allow
@@ -43,4 +50,6 @@ p, role:admin, gpgkeys, delete, *, allow
 p, role:admin, exec, create, */*, allow
 
 g, role:admin, role:readonly
-g, admin, role:admin
+g, role:applicationdeleter, role:readonly
+g, role:deploymentdeleter, role:readonly
+g, admin, role:admin

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,95 @@ 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 TestSyncWithReplacePermissionsWithAppDeletionDenied(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"}},
+		},
+	)
+	require.NoError(t, err)
+}
+
 func TestRollbackApp(t *testing.T) {
 	testApp := newTestApp()
 	testApp.Status.History = []appsv1.RevisionHistory{{