diff --git a/assets/builtin-policy.csv b/assets/builtin-policy.csv index 28f565260d88d..088f5fbd08ad3 100644 --- a/assets/builtin-policy.csv +++ b/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 \ No newline at end of file +g, admin, role:admin diff --git a/docs/operator-manual/rbac.md b/docs/operator-manual/rbac.md index 9b7775a65e3e5..8648448f49804 100644 --- a/docs/operator-manual/rbac.md +++ b/docs/operator-manual/rbac.md @@ -114,8 +114,8 @@ The `applications` resource is an [Application-Specific Policy](#application-spe #### Fine-grained Permissions for `update`/`delete` action -The `update` and `delete` actions, when granted on an application, will allow the user to perform the operation on the application itself **and** all of its resources. -It can be desirable to only allow `update` or `delete` on specific resources within an application. +The `update` and `delete` actions, when granted on an application, will allow the user to perform the operation on the application itself, +but not on its resources. To do so, when the action if performed on an application's resource, the `` will have the `////` format. @@ -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//kind//` but also `delete///kind/`. - 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,14 +148,30 @@ p, example-user, applications, delete, default/prod-app, deny p, example-user, applications, delete/*/Pod/*/*, default/prod-app, allow ``` -!!! note +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 +``` + +!!! note "Preserve Application permission Inheritance (Since v3.0.0)" + + Prior to v3, `update` and `delete` actions (without a `/*`) were also evaluated + on sub-resources. + + To preserve this behavior, you can set the config value + `server.rbac.disableApplicationFineGrainedRBACInheritance` to `false` in + the Argo CD ConfigMap `argocd-cm`. - 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: + When disabled, 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 + p, example-user, applications, delete/*/Pod/*, default/prod-app, deny ``` #### The `action` action diff --git a/docs/operator-manual/upgrading/2.14-3.0.md b/docs/operator-manual/upgrading/2.14-3.0.md index 8605ef6b2ab22..987625f08a171 100644 --- a/docs/operator-manual/upgrading/2.14-3.0.md +++ b/docs/operator-manual/upgrading/2.14-3.0.md @@ -9,4 +9,16 @@ most recent minor versions (so 2.14 until 3.2 is released, and 2.13 until 3.1 is ## Breaking Changes -So far no breaking changes have been merged. We will update this section as changes are merged. +### Fine-Grained RBAC for application `update` and `delete` sub-resources + +The default behavior of fine-grained policies have changed so they do not apply to sub-resources anymore. +Prior to v3, when `update` or `delete` actions were allowed on an application, it gave the permission to +update and delete the application itself and any of its sub-resources. + +Starting with v3, the `update` or `delete` actions only apply on the application. New policies must be defined +to allow the `update/*` or `delete/*` actions on the application to give permissions on sub-resources. + +The v2 behavior can be preserved by setting the config value `server.rbac.disableApplicationFineGrainedRBACInheritance` +to `false` in the Argo CD ConfigMap `argocd-cm`. + +Read the [RBAC documentation](../rbac.md#fine-grained-permissions-for-updatedelete-action) for more detailed inforamtion. diff --git a/server/application/application.go b/server/application/application.go index 36977c1e88053..6a0959c4b99ad 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -1343,9 +1343,16 @@ func (s *Server) getAppResources(ctx context.Context, a *v1alpha1.Application) ( } 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, argocommon.PermissionDeniedAPIError) && (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()) } diff --git a/server/application/application_test.go b/server/application/application_test.go index 33615fa8a87d6..476dfd195b5f0 100644 --- a/server/application/application_test.go +++ b/server/application/application_test.go @@ -1635,6 +1635,10 @@ func TestDeleteResourcesRBAC(t *testing.T) { appServer := newTestAppServer(t, testApp) appServer.enf.SetDefaultRole("") + argoCM := map[string]string{"server.rbac.disableApplicationFineGrainedRBACInheritance": "false"} + appServerWithRBACInheritance := newTestAppServerWithEnforcerConfigure(t, func(_ *rbac.Enforcer) {}, argoCM, testApp) + appServerWithRBACInheritance.enf.SetDefaultRole("") + req := application.ApplicationResourceDeleteRequest{ Name: &testApp.Name, AppNamespace: &testApp.Namespace, @@ -1651,6 +1655,14 @@ func TestDeleteResourcesRBAC(t *testing.T) { p, test-user, applications, delete, default/test-app, allow `) _, err := appServer.DeleteResource(ctx, &req) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + }) + + t.Run("delete with application permission with inheritance", func(t *testing.T) { + _ = appServerWithRBACInheritance.enf.SetBuiltinPolicy(` +p, test-user, applications, delete, default/test-app, allow +`) + _, err := appServerWithRBACInheritance.DeleteResource(ctx, &req) assert.EqualError(t, err, expectedErrorWhenDeleteAllowed) }) @@ -1660,6 +1672,15 @@ p, test-user, applications, delete, default/test-app, allow p, test-user, applications, delete/*, default/test-app, deny `) _, err := appServer.DeleteResource(ctx, &req) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + }) + + t.Run("delete with application permission but deny subresource with inheritance", func(t *testing.T) { + _ = appServerWithRBACInheritance.enf.SetBuiltinPolicy(` +p, test-user, applications, delete, default/test-app, allow +p, test-user, applications, delete/*, default/test-app, deny +`) + _, err := appServerWithRBACInheritance.DeleteResource(ctx, &req) assert.EqualError(t, err, expectedErrorWhenDeleteAllowed) }) @@ -1680,6 +1701,15 @@ p, test-user, applications, delete/*, default/test-app, allow assert.EqualError(t, err, expectedErrorWhenDeleteAllowed) }) + t.Run("delete with subresource but deny applications with inheritance", func(t *testing.T) { + _ = appServerWithRBACInheritance.enf.SetBuiltinPolicy(` +p, test-user, applications, delete, default/test-app, deny +p, test-user, applications, delete/*, default/test-app, allow +`) + _, err := appServerWithRBACInheritance.DeleteResource(ctx, &req) + assert.EqualError(t, err, expectedErrorWhenDeleteAllowed) + }) + t.Run("delete with specific subresource denied", func(t *testing.T) { _ = appServer.enf.SetBuiltinPolicy(` p, test-user, applications, delete/*, default/test-app, allow @@ -1698,6 +1728,10 @@ func TestPatchResourcesRBAC(t *testing.T) { appServer := newTestAppServer(t, testApp) appServer.enf.SetDefaultRole("") + argoCM := map[string]string{"server.rbac.disableApplicationFineGrainedRBACInheritance": "false"} + appServerWithRBACInheritance := newTestAppServerWithEnforcerConfigure(t, func(_ *rbac.Enforcer) {}, argoCM, testApp) + appServerWithRBACInheritance.enf.SetDefaultRole("") + req := application.ApplicationResourcePatchRequest{ Name: &testApp.Name, AppNamespace: &testApp.Namespace, @@ -1714,6 +1748,14 @@ func TestPatchResourcesRBAC(t *testing.T) { p, test-user, applications, update, default/test-app, allow `) _, err := appServer.PatchResource(ctx, &req) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + }) + + t.Run("patch with application permission with inheritance", func(t *testing.T) { + _ = appServerWithRBACInheritance.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, allow +`) + _, err := appServerWithRBACInheritance.PatchResource(ctx, &req) assert.EqualError(t, err, expectedErrorWhenUpdateAllowed) }) @@ -1723,6 +1765,15 @@ p, test-user, applications, update, default/test-app, allow p, test-user, applications, update/*, default/test-app, deny `) _, err := appServer.PatchResource(ctx, &req) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + }) + + t.Run("patch with application permission but deny subresource with inheritance", func(t *testing.T) { + _ = appServerWithRBACInheritance.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, allow +p, test-user, applications, update/*, default/test-app, deny +`) + _, err := appServerWithRBACInheritance.PatchResource(ctx, &req) assert.EqualError(t, err, expectedErrorWhenUpdateAllowed) }) @@ -1743,6 +1794,15 @@ p, test-user, applications, update/*, default/test-app, allow assert.EqualError(t, err, expectedErrorWhenUpdateAllowed) }) + t.Run("patch with subresource but deny applications with inheritance", func(t *testing.T) { + _ = appServerWithRBACInheritance.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, deny +p, test-user, applications, update/*, default/test-app, allow +`) + _, err := appServerWithRBACInheritance.PatchResource(ctx, &req) + assert.EqualError(t, err, expectedErrorWhenUpdateAllowed) + }) + t.Run("patch with specific subresource denied", func(t *testing.T) { _ = appServer.enf.SetBuiltinPolicy(` p, test-user, applications, update/*, default/test-app, allow diff --git a/util/settings/settings.go b/util/settings/settings.go index bfb378ba419f5..c6dca2bbe12eb 100644 --- a/util/settings/settings.go +++ b/util/settings/settings.go @@ -515,6 +515,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 @@ -863,6 +865,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 true, nil + } + + return strconv.ParseBool(argoCDCM.Data[settingsServerRBACDisableFineGrainedInheritance]) +} + func (mgr *SettingsManager) GetMaxPodLogsToRender() (int64, error) { argoCDCM, err := mgr.getConfigMap() if err != nil { diff --git a/util/settings/settings_test.go b/util/settings/settings_test.go index e7482e29ae95d..b85396e4eb65d 100644 --- a/util/settings/settings_test.go +++ b/util/settings/settings_test.go @@ -280,6 +280,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.True(t, flag) +} + +func TestApplicationFineGrainedRBACInheritanceDisabled(t *testing.T) { + _, settingsManager := fixtures(map[string]string{ + "server.rbac.disableApplicationFineGrainedRBACInheritance": "false", + }) + flag, err := settingsManager.ApplicationFineGrainedRBACInheritanceDisabled() + require.NoError(t, err) + assert.False(t, flag) +} + func TestGetIsIgnoreResourceUpdatesEnabled(t *testing.T) { _, settingsManager := fixtures(nil) ignoreResourceUpdatesEnabled, err := settingsManager.GetIsIgnoreResourceUpdatesEnabled() @@ -303,15 +328,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"},