feat(rbac): add disable fine-grained inheritance flag (#20600) (#21553)

Signed-off-by: Matt Finkel <finkel.matt@gmail.com>
Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
Co-authored-by: Matt Finkel <finkel.matt@gmail.com>

assets/builtin-policy.csv

@@ -18,7 +18,9 @@ p, role:readonly, logs, get, */*, allow
 
 p, role:admin, applications, create, */*, allow
 p, role:admin, applications, update, */*, allow
+p, role:admin, applications, update/*, */*, allow
 p, role:admin, applications, delete, */*, allow
+p, role:admin, applications, delete/*, */*, allow
 p, role:admin, applications, sync, */*, allow
 p, role:admin, applications, override, */*, allow
 p, role:admin, applications, action/*, */*, allow
@@ -47,4 +49,4 @@ p, role:admin, gpgkeys, delete, *, allow
 p, role:admin, exec, create, */*, allow
 
 g, role:admin, role:readonly
-g, admin, role:admin
+g, admin, role:admin

docs/operator-manual/rbac.md

@@ -130,9 +130,9 @@ p, example-user, applications, delete/*/Pod/*/*, default/prod-app, allow
     Argo CD RBAC does not use `/` as a separator when evaluating glob patterns. So the pattern `delete/*/kind/*`
     will match `delete/<group>/kind/<namespace>/<name>` but also `delete/<group>/<kind>/kind/<name>`.
 
-    The fact that both of these match will generally not be a problem, because resource kinds generally contain capital 
-    letters, and namespaces cannot contain capital letters. However, it is possible for a resource kind to be lowercase. 
-    So it is better to just always include all the parts of the resource in the pattern (in other words, always use four 
+    The fact that both of these match will generally not be a problem, because resource kinds generally contain capital
+    letters, and namespaces cannot contain capital letters. However, it is possible for a resource kind to be lowercase.
+    So it is better to just always include all the parts of the resource in the pattern (in other words, always use four
     slashes).
 
 If we want to grant access to the user to update all resources of an application, but not the application itself:
@@ -148,16 +148,30 @@ p, example-user, applications, delete, default/prod-app, deny
 p, example-user, applications, delete/*/Pod/*/*, default/prod-app, allow
 ```
 
-!!! note
+!!! note "Disable Application permission Inheritance"
 
-    It is not possible to deny fine-grained permissions for a sub-resource if the action was **explicitly allowed on the application**.
+    By default, it is not possible to deny fine-grained permissions for a sub-resource if the action was **explicitly allowed on the application**.
     For instance, the following policies will **allow** a user to delete the Pod and any other resources in the application:
 
     ```csv
     p, example-user, applications, delete, default/prod-app, allow
     p, example-user, applications, delete/*/Pod/*/*, default/prod-app, deny
     ```
 
+    To change this behavior, you can set the config value
+    `server.rbac.disableApplicationFineGrainedRBACInheritance` to `true` in
+    the Argo CD ConfigMap `argocd-cm`.
+
+    When inheritance is disabled, it is now possible to deny fine-grained permissions for a sub-resource
+    if the action was **explicitly allowed on the application**.
+
+    For instance, if we want to explicitly allow updates to the application, but deny updates to any sub-resources:
+
+    ```csv
+    p, example-user, applications, update, default/prod-app, allow
+    p, example-user, applications, update/*, default/prod-app, deny
+    ```
+
 #### The `action` action
 
 The `action` action corresponds to either built-in resource customizations defined

server/application/application.go

@@ -1350,10 +1350,17 @@ func (s *Server) getAppResources(ctx context.Context, a *appv1.Application) (*ap
 	return &tree, nil
 }
 
-func (s *Server) getAppLiveResource(ctx context.Context, action string, q *application.ApplicationResourceRequest) (*appv1.ResourceNode, *rest.Config, *appv1.Application, error) {
+func (s *Server) getAppLiveResource(ctx context.Context, action string, q *application.ApplicationResourceRequest) (*v1alpha1.ResourceNode, *rest.Config, *v1alpha1.Application, error) {
+	fineGrainedInheritanceDisabled, err := s.settingsMgr.ApplicationFineGrainedRBACInheritanceDisabled()
+	if err != nil {
+		return nil, nil, nil, err
+	}
+
+	if fineGrainedInheritanceDisabled && (action == rbacpolicy.ActionDelete || action == rbacpolicy.ActionUpdate) {
+		action = fmt.Sprintf("%s/%s/%s/%s/%s", action, q.GetGroup(), q.GetKind(), q.GetNamespace(), q.GetResourceName())
+	}
 	a, _, err := s.getApplicationEnforceRBACInformer(ctx, action, q.GetProject(), q.GetAppNamespace(), q.GetName())
-	if err != nil && errors.Is(err, permissionDeniedErr) && (action == rbacpolicy.ActionDelete || action == rbacpolicy.ActionUpdate) {
-		// If users dont have permission on the whole applications, maybe they have fine-grained access to the specific resources
+	if !fineGrainedInheritanceDisabled && err != nil && errors.Is(err, argocommon.PermissionDeniedAPIError) && (action == rbacpolicy.ActionDelete || action == rbacpolicy.ActionUpdate) {
 		action = fmt.Sprintf("%s/%s/%s/%s/%s", action, q.GetGroup(), q.GetKind(), q.GetNamespace(), q.GetResourceName())
 		a, _, err = s.getApplicationEnforceRBACInformer(ctx, action, q.GetProject(), q.GetAppNamespace(), q.GetName())
 	}

server/application/application_test.go

@@ -1639,6 +1639,10 @@ func TestDeleteResourcesRBAC(t *testing.T) {
 	appServer := newTestAppServer(t, testApp)
 	appServer.enf.SetDefaultRole("")
 
+	argoCM := map[string]string{"server.rbac.disableApplicationFineGrainedRBACInheritance": "true"}
+	appServerWithoutRBACInheritance := newTestAppServerWithEnforcerConfigure(t, func(_ *rbac.Enforcer) {}, argoCM, testApp)
+	appServerWithoutRBACInheritance.enf.SetDefaultRole("")
+
 	req := application.ApplicationResourceDeleteRequest{
 		Name:         &testApp.Name,
 		AppNamespace: &testApp.Namespace,
@@ -1650,6 +1654,14 @@ func TestDeleteResourcesRBAC(t *testing.T) {
 
 	expectedErrorWhenDeleteAllowed := "rpc error: code = InvalidArgument desc = PodTest fake.io my-pod-test not found as part of application test-app"
 
+	t.Run("delete with application permission without inheritance", func(t *testing.T) {
+		_ = appServerWithoutRBACInheritance.enf.SetBuiltinPolicy(`
+p, test-user, applications, delete, default/test-app, allow
+`)
+		_, err := appServerWithoutRBACInheritance.DeleteResource(ctx, &req)
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
+	})
+
 	t.Run("delete with application permission", func(t *testing.T) {
 		_ = appServer.enf.SetBuiltinPolicy(`
 p, test-user, applications, delete, default/test-app, allow
@@ -1658,6 +1670,15 @@ p, test-user, applications, delete, default/test-app, allow
 		assert.EqualError(t, err, expectedErrorWhenDeleteAllowed)
 	})
 
+	t.Run("delete with application permission but deny subresource without inheritance", func(t *testing.T) {
+		_ = appServerWithoutRBACInheritance.enf.SetBuiltinPolicy(`
+p, test-user, applications, delete, default/test-app, allow
+p, test-user, applications, delete/*, default/test-app, deny
+`)
+		_, err := appServerWithoutRBACInheritance.DeleteResource(ctx, &req)
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
+	})
+
 	t.Run("delete with application permission but deny subresource", func(t *testing.T) {
 		_ = appServer.enf.SetBuiltinPolicy(`
 p, test-user, applications, delete, default/test-app, allow
@@ -1675,6 +1696,15 @@ p, test-user, applications, delete/*, default/test-app, allow
 		assert.EqualError(t, err, expectedErrorWhenDeleteAllowed)
 	})
 
+	t.Run("delete with subresource but deny applications without inheritance", func(t *testing.T) {
+		_ = appServerWithoutRBACInheritance.enf.SetBuiltinPolicy(`
+p, test-user, applications, delete, default/test-app, deny
+p, test-user, applications, delete/*, default/test-app, allow
+`)
+		_, err := appServerWithoutRBACInheritance.DeleteResource(ctx, &req)
+		assert.EqualError(t, err, expectedErrorWhenDeleteAllowed)
+	})
+
 	t.Run("delete with subresource but deny applications", func(t *testing.T) {
 		_ = appServer.enf.SetBuiltinPolicy(`
 p, test-user, applications, delete, default/test-app, deny
@@ -1702,6 +1732,10 @@ func TestPatchResourcesRBAC(t *testing.T) {
 	appServer := newTestAppServer(t, testApp)
 	appServer.enf.SetDefaultRole("")
 
+	argoCM := map[string]string{"server.rbac.disableApplicationFineGrainedRBACInheritance": "true"}
+	appServerWithoutRBACInheritance := newTestAppServerWithEnforcerConfigure(t, func(_ *rbac.Enforcer) {}, argoCM, testApp)
+	appServerWithoutRBACInheritance.enf.SetDefaultRole("")
+
 	req := application.ApplicationResourcePatchRequest{
 		Name:         &testApp.Name,
 		AppNamespace: &testApp.Namespace,
@@ -1713,6 +1747,14 @@ func TestPatchResourcesRBAC(t *testing.T) {
 
 	expectedErrorWhenUpdateAllowed := "rpc error: code = InvalidArgument desc = PodTest fake.io my-pod-test not found as part of application test-app"
 
+	t.Run("patch with application permission without inheritance", func(t *testing.T) {
+		_ = appServerWithoutRBACInheritance.enf.SetBuiltinPolicy(`
+p, test-user, applications, update, default/test-app, allow
+`)
+		_, err := appServerWithoutRBACInheritance.PatchResource(ctx, &req)
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
+	})
+
 	t.Run("patch with application permission", func(t *testing.T) {
 		_ = appServer.enf.SetBuiltinPolicy(`
 p, test-user, applications, update, default/test-app, allow
@@ -1721,6 +1763,15 @@ p, test-user, applications, update, default/test-app, allow
 		assert.EqualError(t, err, expectedErrorWhenUpdateAllowed)
 	})
 
+	t.Run("patch with application permission but deny subresource without inheritance", func(t *testing.T) {
+		_ = appServerWithoutRBACInheritance.enf.SetBuiltinPolicy(`
+p, test-user, applications, update, default/test-app, allow
+p, test-user, applications, update/*, default/test-app, deny
+`)
+		_, err := appServerWithoutRBACInheritance.PatchResource(ctx, &req)
+		assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
+	})
+
 	t.Run("patch with application permission but deny subresource", func(t *testing.T) {
 		_ = appServer.enf.SetBuiltinPolicy(`
 p, test-user, applications, update, default/test-app, allow
@@ -1738,6 +1789,15 @@ p, test-user, applications, update/*, default/test-app, allow
 		assert.EqualError(t, err, expectedErrorWhenUpdateAllowed)
 	})
 
+	t.Run("patch with subresource but deny applications without inheritance", func(t *testing.T) {
+		_ = appServerWithoutRBACInheritance.enf.SetBuiltinPolicy(`
+p, test-user, applications, update, default/test-app, deny
+p, test-user, applications, update/*, default/test-app, allow
+`)
+		_, err := appServerWithoutRBACInheritance.PatchResource(ctx, &req)
+		assert.EqualError(t, err, expectedErrorWhenUpdateAllowed)
+	})
+
 	t.Run("patch with subresource but deny applications", func(t *testing.T) {
 		_ = appServer.enf.SetBuiltinPolicy(`
 p, test-user, applications, update, default/test-app, deny

util/settings/settings.go

@@ -512,6 +512,8 @@ const (
 	inClusterEnabledKey = "cluster.inClusterEnabled"
 	// settingsServerRBACLogEnforceEnable is the key to configure whether logs RBAC enforcement is enabled
 	settingsServerRBACLogEnforceEnableKey = "server.rbac.log.enforce.enable"
+	// settingsServerRBACEDisableFineGrainedInheritance is the key to configure find-grained RBAC inheritance
+	settingsServerRBACDisableFineGrainedInheritance = "server.rbac.disableApplicationFineGrainedRBACInheritance"
 	// MaxPodLogsToRender the maximum number of pod logs to render
 	settingsMaxPodLogsToRender = "server.maxPodLogsToRender"
 	// helmValuesFileSchemesKey is the key to configure the list of supported helm values file schemas
@@ -833,6 +835,19 @@ func (mgr *SettingsManager) GetServerRBACLogEnforceEnable() (bool, error) {
 	return strconv.ParseBool(argoCDCM.Data[settingsServerRBACLogEnforceEnableKey])
 }
 
+func (mgr *SettingsManager) ApplicationFineGrainedRBACInheritanceDisabled() (bool, error) {
+	argoCDCM, err := mgr.getConfigMap()
+	if err != nil {
+		return false, err
+	}
+
+	if argoCDCM.Data[settingsServerRBACDisableFineGrainedInheritance] == "" {
+		return false, nil
+	}
+
+	return strconv.ParseBool(argoCDCM.Data[settingsServerRBACDisableFineGrainedInheritance])
+}
+
 func (mgr *SettingsManager) GetMaxPodLogsToRender() (int64, error) {
 	argoCDCM, err := mgr.getConfigMap()
 	if err != nil {

util/settings/settings_test.go

@@ -245,6 +245,31 @@ func TestGetServerRBACLogEnforceEnableKeyDefaultFalse(t *testing.T) {
 	assert.False(t, serverRBACLogEnforceEnable)
 }
 
+func TestGetServerRBACLogEnforceEnableKey(t *testing.T) {
+	_, settingsManager := fixtures(map[string]string{
+		"server.rbac.log.enforce.enable": "true",
+	})
+	serverRBACLogEnforceEnable, err := settingsManager.GetServerRBACLogEnforceEnable()
+	require.NoError(t, err)
+	assert.True(t, serverRBACLogEnforceEnable)
+}
+
+func TestApplicationFineGrainedRBACInheritanceDisabledDefault(t *testing.T) {
+	_, settingsManager := fixtures(nil)
+	flag, err := settingsManager.ApplicationFineGrainedRBACInheritanceDisabled()
+	require.NoError(t, err)
+	assert.False(t, flag)
+}
+
+func TestApplicationFineGrainedRBACInheritanceDisabled(t *testing.T) {
+	_, settingsManager := fixtures(map[string]string{
+		"server.rbac.disableApplicationFineGrainedRBACInheritance": "true",
+	})
+	flag, err := settingsManager.ApplicationFineGrainedRBACInheritanceDisabled()
+	require.NoError(t, err)
+	assert.True(t, flag)
+}
+
 func TestGetIsIgnoreResourceUpdatesEnabled(t *testing.T) {
 	_, settingsManager := fixtures(nil)
 	ignoreResourceUpdatesEnabled, err := settingsManager.GetIsIgnoreResourceUpdatesEnabled()
@@ -268,15 +293,6 @@ func TestGetIsIgnoreResourceUpdatesEnabledFalse(t *testing.T) {
 	assert.False(t, ignoreResourceUpdatesEnabled)
 }
 
-func TestGetServerRBACLogEnforceEnableKey(t *testing.T) {
-	_, settingsManager := fixtures(map[string]string{
-		"server.rbac.log.enforce.enable": "true",
-	})
-	serverRBACLogEnforceEnable, err := settingsManager.GetServerRBACLogEnforceEnable()
-	require.NoError(t, err)
-	assert.True(t, serverRBACLogEnforceEnable)
-}
-
 func TestGetResourceOverrides(t *testing.T) {
 	ignoreStatus := v1alpha1.ResourceOverride{IgnoreDifferences: v1alpha1.OverrideIgnoreDiff{
 		JSONPointers: []string{"/status"},