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]>
andrii-korotkov-verkada · Nov 8, 2024 · 0a8b1c2 · 0a8b1c2
1 parent e2bc96b
commit 0a8b1c2
Show file tree

Hide file tree

Showing 5 changed files with 185 additions and 2 deletions.
diff --git 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):
@@ -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
diff --git 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
@@ -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
@@ -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,

diff --git a/server/application/application_test.go b/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{{