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.

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,24 @@ func (s *Server) Sync(ctx context.Context, syncReq *application.ApplicationSyncR
 			}
 		}
 	}
+
+	if syncOptions.HasOption(common.SyncOptionReplace) {
+		// If application-level delete policy is allow, we don't check for specific resources even if present. This kind of behavior is documented.
+		if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceApplications, rbacpolicy.ActionDelete, a.RBACName(s.ns)); err != nil {
+			// An edge case if no specific resources are specified, e.g. can happen in some tests.
+			if len(resources) == 0 {
+				return nil, status.Errorf(codes.PermissionDenied, "cannot sync: replace is enabled, but delete permissions denied")
+			}
+			// If application-level delete is denied, check specific resources' delete permissions.
+			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 delete permissions denied")
+				}
+			}
+		}
+	}
+
 	op := appv1.Operation{
 		Sync: &appv1.SyncOperation{
 			Revision:     revision,

server/application/application_test.go

@@ -1863,6 +1863,94 @@ 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) {
+	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 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{{