fix: Apply deletion permission checks when syncing with replace (argo…

…proj#14161)

Closes argoproj#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

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{{