From 8884828293c772b9816d70f8327fc65a53eac021 Mon Sep 17 00:00:00 2001 From: Andrew Melnick Date: Thu, 11 Apr 2024 16:16:54 -0600 Subject: [PATCH] Support for multiple ConfigMaps w/ policy.csv Signed-off-by: Andrew Melnick --- cmd/argocd/commands/admin/settings_rbac.go | 18 +- docs/operator-manual/rbac.md | 25 +++ test/e2e/accounts_test.go | 65 ++++++ test/e2e/app_management_test.go | 243 +++++++++++++++++++++ test/e2e/cluster_test.go | 34 +++ test/e2e/fixture/account/actions.go | 10 + test/e2e/fixture/account/context.go | 4 + test/e2e/fixture/fixture.go | 62 ++++++ test/e2e/scoped_repository_test.go | 43 ++++ util/rbac/rbac.go | 184 ++++++++++++---- util/rbac/rbac_test.go | 111 +++++++++- 11 files changed, 742 insertions(+), 57 deletions(-) diff --git a/cmd/argocd/commands/admin/settings_rbac.go b/cmd/argocd/commands/admin/settings_rbac.go index dc8faf657b520..0eeff4f540a69 100644 --- a/cmd/argocd/commands/admin/settings_rbac.go +++ b/cmd/argocd/commands/admin/settings_rbac.go @@ -348,11 +348,11 @@ func getPolicy(ctx context.Context, policyFile string, kubeClient kubernetes.Int log.Fatalf("could not read policy file: %v", err) } } else { - cm, err := getPolicyConfigMap(ctx, kubeClient, namespace) + cm, extraCMs, err := getPolicyConfigMaps(ctx, kubeClient, namespace) if err != nil { log.Fatalf("could not get configmap: %v", err) } - userPolicy, defaultRole, matchMode = getPolicyFromConfigMap(cm) + userPolicy, defaultRole, matchMode = getPolicyFromConfigMaps(cm, extraCMs...) } return userPolicy, defaultRole, matchMode @@ -379,14 +379,14 @@ func getPolicyFromFile(policyFile string) (string, string, string, error) { if err != nil { userPolicy = string(upol) } else { - userPolicy, defaultRole, matchMode = getPolicyFromConfigMap(upolCM) + userPolicy, defaultRole, matchMode = getPolicyFromConfigMaps(upolCM) } return userPolicy, defaultRole, matchMode, nil } // Retrieve policy information from a ConfigMap -func getPolicyFromConfigMap(cm *corev1.ConfigMap) (string, string, string) { +func getPolicyFromConfigMaps(cm *corev1.ConfigMap, extraCMs ...corev1.ConfigMap) (string, string, string) { var ( defaultRole string ok bool @@ -401,12 +401,16 @@ func getPolicyFromConfigMap(cm *corev1.ConfigMap) (string, string, string) { } // getPolicyConfigMap fetches the RBAC config map from K8s cluster -func getPolicyConfigMap(ctx context.Context, client kubernetes.Interface, namespace string) (*corev1.ConfigMap, error) { +func getPolicyConfigMaps(ctx context.Context, client kubernetes.Interface, namespace string) (*corev1.ConfigMap, []corev1.ConfigMap, error) { cm, err := client.CoreV1().ConfigMaps(namespace).Get(ctx, common.ArgoCDRBACConfigMapName, v1.GetOptions{}) if err != nil { - return nil, err + return nil, nil, err } - return cm, nil + extraCMs, err := client.CoreV1().ConfigMaps(namespace).List(ctx, v1.ListOptions{LabelSelector: rbac.ExtraConfigMapLabelSelector}) + if err != nil { + return nil, nil, err + } + return cm, extraCMs.Items, nil } // checkPolicy checks whether given subject is allowed to execute specified diff --git a/docs/operator-manual/rbac.md b/docs/operator-manual/rbac.md index 63e71c67f001c..267f8235edbb0 100644 --- a/docs/operator-manual/rbac.md +++ b/docs/operator-manual/rbac.md @@ -393,6 +393,31 @@ data: g, my-org:team-qa, role:tester ``` +It is also possible to provide multiple configmaps by labeling those +configmaps with `argocd.argoproj.io/cm-type=policy-csv`. These extra +configmaps use the same rules for keys as the main `argocd-rbac-cm` +configmap. The output CSVs of these configmaps, along with the main +configmap, will be concatenated in alphabetical order of the configmap +name. + +```yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: another-rbac-cm + namespace: argocd + labels: + argocd.argoproj.io/cm-type: policy-csv +data: + policy.yet-another.csv: | + p, role:another-tester, applications, *, */*, allow + p, role:another-tester, projects, *, *, allow + g, my-org:team-qa-2, role:another-tester +``` + +This can be used to create policies which exceed the size limit of a single configmap, which +may occurr in larger multi-tenant environments. + ## Validating and testing your RBAC policies If you want to ensure that your RBAC policies are working as expected, you can diff --git a/test/e2e/accounts_test.go b/test/e2e/accounts_test.go index 7f3f056a952c9..cddfb4f14d66a 100644 --- a/test/e2e/accounts_test.go +++ b/test/e2e/accounts_test.go @@ -96,6 +96,71 @@ func TestCanIGetLogsAllowSwitchOn(t *testing.T) { }) } +func TestCanIGetLogsAllowSwitchOnInExtraConfigMap(t *testing.T) { + ctx := accountFixture.Given(t) + ctx. + Name("test"). + Project(ProjectName). + When(). + Create(). + Login(). + SetExtraPermissions([]ACL{ + { + Resource: "logs", + Action: "get", + Scope: ProjectName + "/*", + }, + { + Resource: "apps", + Action: "get", + Scope: ProjectName + "/*", + }, + }, "log-viewer"). + SetParamInSettingConfigMap("server.rbac.log.enforce.enable", "true"). + CanIGetLogs(). + Then(). + AndCLIOutput(func(output string, err error) { + assert.True(t, strings.Contains(output, "yes")) + }) + + accountFixture.GivenWithSameState(t). + When(). + ClearExtraPermissions(). + CanIGetLogs(). + Then(). + AndCLIOutput(func(output string, err error) { + assert.True(t, strings.Contains(output, "no")) + }) +} + +func TestCanIGetLogsDenyAfterDeleteExtraConfigMap(t *testing.T) { + ctx := accountFixture.Given(t) + ctx. + Name("test"). + Project(ProjectName). + When(). + Create(). + Login(). + SetExtraPermissions([]ACL{ + { + Resource: "logs", + Action: "get", + Scope: ProjectName + "/*", + }, + { + Resource: "apps", + Action: "get", + Scope: ProjectName + "/*", + }, + }, "log-viewer"). + SetParamInSettingConfigMap("server.rbac.log.enforce.enable", "true"). + CanIGetLogs(). + Then(). + AndCLIOutput(func(output string, err error) { + assert.True(t, strings.Contains(output, "yes")) + }) +} + func TestCanIGetLogsAllowSwitchOff(t *testing.T) { ctx := accountFixture.Given(t) ctx. diff --git a/test/e2e/app_management_test.go b/test/e2e/app_management_test.go index 8a5896fbdc560..0b742dd360686 100644 --- a/test/e2e/app_management_test.go +++ b/test/e2e/app_management_test.go @@ -103,6 +103,187 @@ func TestGetLogsDenySwitchOn(t *testing.T) { }) } +func TestGetLogsDenyAfterChangeExtraConfigMap(t *testing.T) { + SkipOnEnv(t, "OPENSHIFT") + + accountFixture.Given(t). + Name("test"). + When(). + Create(). + Login(). + SetExtraPermissions([]fixture.ACL{ + { + Resource: "applications", + Action: "create", + Scope: "*", + }, + { + Resource: "applications", + Action: "get", + Scope: "*", + }, + { + Resource: "applications", + Action: "sync", + Scope: "*", + }, + { + Resource: "projects", + Action: "get", + Scope: "*", + }, + { + Resource: "logs", + Action: "get", + Scope: "*", + }, + }, "app-creator") + + GivenWithSameState(t). + Path("guestbook-logs"). + When(). + CreateApp(). + Sync(). + SetParamInSettingConfigMap("server.rbac.log.enforce.enable", "true"). + Then(). + Expect(HealthIs(health.HealthStatusHealthy)). + And(func(app *Application) { + out, err := RunCliWithRetry(appLogsRetryCount, "app", "logs", app.Name, "--kind", "Deployment", "--group", "", "--name", "guestbook-ui") + assert.NoError(t, err) + assert.Contains(t, out, "Hi") + }). + And(func(app *Application) { + out, err := RunCliWithRetry(appLogsRetryCount, "app", "logs", app.Name, "--kind", "Pod") + assert.NoError(t, err) + assert.Contains(t, out, "Hi") + }). + And(func(app *Application) { + out, err := RunCliWithRetry(appLogsRetryCount, "app", "logs", app.Name, "--kind", "Service") + assert.NoError(t, err) + assert.NotContains(t, out, "Hi") + }) + + accountFixture.GivenWithSameState(t). + Name("test"). + When(). + Create(). + Login(). + SetExtraPermissions([]fixture.ACL{ + { + Resource: "applications", + Action: "create", + Scope: "*", + }, + { + Resource: "applications", + Action: "get", + Scope: "*", + }, + { + Resource: "applications", + Action: "sync", + Scope: "*", + }, + { + Resource: "projects", + Action: "get", + Scope: "*", + }, + }, "app-creator") + + GivenWithSameState(t). + Path("guestbook-logs"). + When(). + CreateApp(). + Sync(). + SetParamInSettingConfigMap("server.rbac.log.enforce.enable", "true"). + Then(). + Expect(HealthIs(health.HealthStatusHealthy)). + And(func(app *Application) { + _, err := RunCliWithRetry(appLogsRetryCount, "app", "logs", app.Name, "--kind", "Deployment", "--group", "", "--name", "guestbook-ui") + assert.Error(t, err) + assert.Contains(t, err.Error(), "permission denied") + }) + +} + +func TestGetLogsDenyAfterDeleteExtraConfigMap(t *testing.T) { + SkipOnEnv(t, "OPENSHIFT") + + accountFixture.Given(t). + Name("test"). + When(). + Create(). + Login(). + SetExtraPermissions([]fixture.ACL{ + { + Resource: "applications", + Action: "create", + Scope: "*", + }, + { + Resource: "applications", + Action: "get", + Scope: "*", + }, + { + Resource: "applications", + Action: "sync", + Scope: "*", + }, + { + Resource: "projects", + Action: "get", + Scope: "*", + }, + { + Resource: "logs", + Action: "get", + Scope: "*", + }, + }, "app-creator") + + GivenWithSameState(t). + Path("guestbook-logs"). + When(). + CreateApp(). + Sync(). + SetParamInSettingConfigMap("server.rbac.log.enforce.enable", "true"). + Then(). + Expect(HealthIs(health.HealthStatusHealthy)). + And(func(app *Application) { + out, err := RunCliWithRetry(appLogsRetryCount, "app", "logs", app.Name, "--kind", "Deployment", "--group", "", "--name", "guestbook-ui") + assert.NoError(t, err) + assert.Contains(t, out, "Hi") + }). + And(func(app *Application) { + out, err := RunCliWithRetry(appLogsRetryCount, "app", "logs", app.Name, "--kind", "Pod") + assert.NoError(t, err) + assert.Contains(t, out, "Hi") + }). + And(func(app *Application) { + out, err := RunCliWithRetry(appLogsRetryCount, "app", "logs", app.Name, "--kind", "Service") + assert.NoError(t, err) + assert.NotContains(t, out, "Hi") + }) + + accountFixture.GivenWithSameState(t). + Name("test"). + When(). + ClearExtraPermissions() + + GivenWithSameState(t). + Path("guestbook-logs"). + When(). + Then(). + And(func(app *Application) { + _, err := RunCliWithRetry(appLogsRetryCount, "app", "logs", app.Name, "--kind", "Deployment", "--group", "", "--name", "guestbook-ui") + assert.Error(t, err) + assert.Contains(t, err.Error(), "permission denied") + }) + +} + func TestGetLogsAllowSwitchOn(t *testing.T) { SkipOnEnv(t, "OPENSHIFT") @@ -164,6 +345,68 @@ func TestGetLogsAllowSwitchOn(t *testing.T) { }) } +func TestGetLogsAllowSwitchOnExtraConfigMap(t *testing.T) { + SkipOnEnv(t, "OPENSHIFT") + + accountFixture.Given(t). + Name("test"). + When(). + Create(). + Login(). + SetExtraPermissions([]fixture.ACL{ + { + Resource: "applications", + Action: "create", + Scope: "*", + }, + { + Resource: "applications", + Action: "get", + Scope: "*", + }, + { + Resource: "applications", + Action: "sync", + Scope: "*", + }, + { + Resource: "projects", + Action: "get", + Scope: "*", + }, + { + Resource: "logs", + Action: "get", + Scope: "*", + }, + }, "app-creator") + + GivenWithSameState(t). + Path("guestbook-logs"). + When(). + CreateApp(). + Sync(). + SetParamInSettingConfigMap("server.rbac.log.enforce.enable", "true"). + Then(). + Expect(HealthIs(health.HealthStatusHealthy)). + And(func(app *Application) { + out, err := RunCliWithRetry(appLogsRetryCount, "app", "logs", app.Name, "--kind", "Deployment", "--group", "", "--name", "guestbook-ui") + assert.NoError(t, err) + assert.Contains(t, out, "Hi") + }). + And(func(app *Application) { + out, err := RunCliWithRetry(appLogsRetryCount, "app", "logs", app.Name, "--kind", "Pod") + assert.NoError(t, err) + assert.Contains(t, out, "Hi") + }). + And(func(app *Application) { + out, err := RunCliWithRetry(appLogsRetryCount, "app", "logs", app.Name, "--kind", "Service") + assert.NoError(t, err) + assert.NotContains(t, out, "Hi") + }) + +} + func TestGetLogsAllowSwitchOff(t *testing.T) { SkipOnEnv(t, "OPENSHIFT") diff --git a/test/e2e/cluster_test.go b/test/e2e/cluster_test.go index 66c5e8e0787e4..f3111915bd936 100644 --- a/test/e2e/cluster_test.go +++ b/test/e2e/cluster_test.go @@ -128,6 +128,40 @@ https://kubernetes.default.svc test-cluster-add-allowed %v Successful }) } +func TestClusterAddAllowedExtraConfigMap(t *testing.T) { + accountFixture.Given(t). + Name("test"). + When(). + Create(). + Login(). + SetExtraPermissions([]fixture.ACL{ + { + Resource: "clusters", + Action: "create", + Scope: ProjectName + "/*", + }, + { + Resource: "clusters", + Action: "get", + Scope: ProjectName + "/*", + }, + }, "org-admin") + + clusterFixture. + GivenWithSameState(t). + Project(ProjectName). + Upsert(true). + Server(KubernetesInternalAPIServerAddr). + When(). + Create(). + List(). + Then(). + AndCLIOutput(func(output string, err error) { + assert.Equal(t, fmt.Sprintf(`SERVER NAME VERSION STATUS MESSAGE PROJECT +https://kubernetes.default.svc test-cluster-add-allowed-extra-config-map %v Successful argo-project`, GetVersions().ServerVersion), output) + }) +} + func TestClusterListDenied(t *testing.T) { accountFixture.Given(t). Name("test"). diff --git a/test/e2e/fixture/account/actions.go b/test/e2e/fixture/account/actions.go index f5708c5606a41..81b9aa6b76923 100644 --- a/test/e2e/fixture/account/actions.go +++ b/test/e2e/fixture/account/actions.go @@ -58,6 +58,16 @@ func (a *Actions) SetPermissions(permissions []fixture.ACL, roleName string) *Ac return a } +func (a *Actions) SetExtraPermissions(permissions []fixture.ACL, roleName string) *Actions { + fixture.SetExtraPermissions(permissions, a.context.name, roleName) + return a +} + +func (a *Actions) ClearExtraPermissions() *Actions { + fixture.ClearExtraPermissions() + return a +} + func (a *Actions) SetParamInSettingConfigMap(key, value string) *Actions { fixture.SetParamInSettingConfigMap(key, value) return a diff --git a/test/e2e/fixture/account/context.go b/test/e2e/fixture/account/context.go index 6007392da6022..59703c9e909ce 100644 --- a/test/e2e/fixture/account/context.go +++ b/test/e2e/fixture/account/context.go @@ -20,6 +20,10 @@ type Context struct { func Given(t *testing.T) *Context { t.Helper() fixture.EnsureCleanState(t) + return GivenWithSameState(t) +} + +func GivenWithSameState(t *testing.T) *Context { // ARGOCE_E2E_DEFAULT_TIMEOUT can be used to override the default timeout // for any context. timeout := env.ParseNumFromEnv("ARGOCD_E2E_DEFAULT_TIMEOUT", 10, 0, 180) diff --git a/test/e2e/fixture/fixture.go b/test/e2e/fixture/fixture.go index f2f366c201240..0088cd616e99a 100644 --- a/test/e2e/fixture/fixture.go +++ b/test/e2e/fixture/fixture.go @@ -17,6 +17,7 @@ import ( jsonpatch "github.com/evanphx/json-patch" log "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" + apierr "k8s.io/apimachinery/pkg/api/errors" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/dynamic" "k8s.io/client-go/kubernetes" @@ -34,6 +35,7 @@ import ( grpcutil "github.com/argoproj/argo-cd/v2/util/grpc" "github.com/argoproj/argo-cd/v2/util/io" "github.com/argoproj/argo-cd/v2/util/rand" + "github.com/argoproj/argo-cd/v2/util/rbac" "github.com/argoproj/argo-cd/v2/util/settings" ) @@ -368,6 +370,46 @@ func updateRBACConfigMap(updater func(cm *corev1.ConfigMap) error) { updateGenericConfigMap(common.ArgoCDRBACConfigMapName, updater) } +const ( + extraRBACConfigMapName = "an-extra-rbac-configmap" +) + +// Convenience wrapper for creating/updating an extra rbac config map +func createOrUpdateExtraRBACConfigMap(updater func(cm *corev1.ConfigMap) error) { + cm, err := KubeClientset.CoreV1().ConfigMaps(TestNamespace()).Get(context.Background(), extraRBACConfigMapName, v1.GetOptions{}) + missing := apierr.IsNotFound(err) + if missing { + log.Infof("extra rbac cm doesn't exist: %s", err.Error()) + cm = &corev1.ConfigMap{ + ObjectMeta: v1.ObjectMeta{ + Namespace: TestNamespace(), + Name: extraRBACConfigMapName, + Labels: map[string]string{ + rbac.ExtraConfigMapLabelKey: rbac.ExtraConfigMapLabelValue, + }, + }, + Data: make(map[string]string), + } + } else { + errors.CheckError(err) + } + err = updater(cm) + errors.CheckError(err) + if missing { + _, err = KubeClientset.CoreV1().ConfigMaps(TestNamespace()).Create(context.Background(), cm, v1.CreateOptions{}) + } else { + _, err = KubeClientset.CoreV1().ConfigMaps(TestNamespace()).Update(context.Background(), cm, v1.UpdateOptions{}) + } + errors.CheckError(err) +} + +func deleteExtraRBACConfigMap() { + err := KubeClientset.CoreV1().ConfigMaps(TestNamespace()).Delete(context.Background(), extraRBACConfigMapName, v1.DeleteOptions{}) + if !apierr.IsNotFound(err) { + errors.CheckError(err) + } +} + // Updates a given config map in argocd-e2e namespace func updateGenericConfigMap(name string, updater func(cm *corev1.ConfigMap) error) { cm, err := KubeClientset.CoreV1().ConfigMaps(TestNamespace()).Get(context.Background(), name, v1.GetOptions{}) @@ -491,6 +533,25 @@ func SetPermissions(permissions []ACL, username string, roleName string) { }) } +func SetExtraPermissions(permissions []ACL, username string, roleName string) { + createOrUpdateExtraRBACConfigMap(func(cm *corev1.ConfigMap) error { + var aclstr string + + for _, permission := range permissions { + aclstr += fmt.Sprintf("p, role:%s, %s, %s, %s, allow \n", roleName, permission.Resource, permission.Action, permission.Scope) + } + + aclstr += fmt.Sprintf("g, %s, role:%s", username, roleName) + cm.Data["policy.csv"] = aclstr + + return nil + }) +} + +func ClearExtraPermissions() { + deleteExtraRBACConfigMap() +} + func SetResourceFilter(filters settings.ResourcesFilter) { updateSettingConfigMap(func(cm *corev1.ConfigMap) error { exclusions, err := yaml.Marshal(filters.ResourceExclusions) @@ -626,6 +687,7 @@ func EnsureCleanState(t *testing.T, opts ...TestOption) { cm.Data = map[string]string{} return nil }) + deleteExtraRBACConfigMap() // We can switch user and as result in previous state we will have non-admin user, this case should be reset LoginAs(adminUsername) diff --git a/test/e2e/scoped_repository_test.go b/test/e2e/scoped_repository_test.go index 8578f2fa90932..3a3756470e81d 100644 --- a/test/e2e/scoped_repository_test.go +++ b/test/e2e/scoped_repository_test.go @@ -130,6 +130,49 @@ func TestDeleteRepositoryRbacAllowed(t *testing.T) { }) } +func TestDeleteRepositoryRbacAllowedExtraConfigMap(t *testing.T) { + accountFixture.Given(t). + Name("test"). + When(). + Create(). + Login(). + SetExtraPermissions([]fixture.ACL{ + { + Resource: "repositories", + Action: "create", + Scope: "argo-project/*", + }, + { + Resource: "repositories", + Action: "delete", + Scope: "argo-project/*", + }, + { + Resource: "repositories", + Action: "get", + Scope: "argo-project/*", + }, + }, "org-admin") + + path := "https://github.com/argoproj/argo-cd.git" + repoFixture.Given(t, true). + When(). + Path(path). + Project("argo-project"). + Create(). + Then(). + And(func(r *Repository, err error) { + assert.Equal(t, r.Repo, path) + assert.Equal(t, r.Project, "argo-project") + }). + When(). + Delete(). + Then(). + AndCLIOutput(func(output string, err error) { + assert.True(t, strings.Contains(output, "Repository 'https://github.com/argoproj/argo-cd.git' removed")) + }) +} + func TestDeleteRepositoryRbacDenied(t *testing.T) { accountFixture.Given(t). Name("test"). diff --git a/util/rbac/rbac.go b/util/rbac/rbac.go index ab45cf5c0d69f..2e5917c168212 100644 --- a/util/rbac/rbac.go +++ b/util/rbac/rbac.go @@ -5,6 +5,7 @@ import ( "encoding/csv" "errors" "fmt" + "slices" "sort" "strings" "sync" @@ -33,12 +34,15 @@ import ( ) const ( - ConfigMapPolicyCSVKey = "policy.csv" - ConfigMapPolicyDefaultKey = "policy.default" - ConfigMapScopesKey = "scopes" - ConfigMapMatchModeKey = "policy.matchMode" - GlobMatchMode = "glob" - RegexMatchMode = "regex" + ConfigMapPolicyCSVKey = "policy.csv" + ConfigMapPolicyDefaultKey = "policy.default" + ConfigMapScopesKey = "scopes" + ConfigMapMatchModeKey = "policy.matchMode" + GlobMatchMode = "glob" + RegexMatchMode = "regex" + ExtraConfigMapLabelKey = "argocd.argoproj.io/cm-type" + ExtraConfigMapLabelValue = "policy-csv" + ExtraConfigMapLabelSelector = ExtraConfigMapLabelKey + "=" + ExtraConfigMapLabelValue defaultRBACSyncPeriod = 10 * time.Minute ) @@ -72,6 +76,8 @@ type Enforcer struct { model model.Model defaultRole string matchMode string + // map from configmap name -> key -> value + policyCSVCache map[string]map[string]string } // cachedEnforcer holds the Casbin enforcer instances and optional custom project policy @@ -169,6 +175,7 @@ func NewEnforcer(clientset kubernetes.Interface, namespace, configmap string, cl model: builtInModel, claimsEnforcerFunc: claimsEnforcer, enabled: true, + policyCSVCache: map[string]map[string]string{}, } } @@ -334,7 +341,7 @@ func (e *Enforcer) SetUserPolicy(policy string) error { return e.LoadPolicy() } -// newInformers returns an informer which watches updates on the rbac configmap +// newInformer returns an informer which watches updates on the rbac configmap func (e *Enforcer) newInformer() cache.SharedIndexInformer { tweakConfigMap := func(options *metav1.ListOptions) { cmFieldSelector := fields.ParseSelectorOrDie(fmt.Sprintf("metadata.name=%s", e.configmap)) @@ -344,6 +351,16 @@ func (e *Enforcer) newInformer() cache.SharedIndexInformer { return v1.NewFilteredConfigMapInformer(e.clientset, e.namespace, defaultRBACSyncPeriod, indexers, tweakConfigMap) } +// newInformer returns an informer which watches updates on the extra rbac configmaps +func (e *Enforcer) newExtraInformer() cache.SharedIndexInformer { + tweakConfigMap := func(options *metav1.ListOptions) { + cmLabelSelector := fields.ParseSelectorOrDie(ExtraConfigMapLabelSelector) + options.LabelSelector = cmLabelSelector.String() + } + indexers := cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc} + return v1.NewFilteredConfigMapInformer(e.clientset, e.namespace, defaultRBACSyncPeriod, indexers, tweakConfigMap) +} + // RunPolicyLoader runs the policy loader which watches policy updates from the configmap and reloads them func (e *Enforcer) RunPolicyLoader(ctx context.Context, onUpdated func(cm *apiv1.ConfigMap) error) error { cm, err := e.clientset.CoreV1().ConfigMaps(e.namespace).Get(ctx, e.configmap, metav1.GetOptions{}) @@ -363,48 +380,68 @@ func (e *Enforcer) RunPolicyLoader(ctx context.Context, onUpdated func(cm *apiv1 func (e *Enforcer) runInformer(ctx context.Context, onUpdated func(cm *apiv1.ConfigMap) error) { cmInformer := e.newInformer() - _, err := cmInformer.AddEventHandler( - cache.ResourceEventHandlerFuncs{ - AddFunc: func(obj interface{}) { - if cm, ok := obj.(*apiv1.ConfigMap); ok { - err := e.syncUpdate(cm, onUpdated) - if err != nil { - log.Error(err) - } else { - log.Infof("RBAC ConfigMap '%s' added", e.configmap) - } - } - }, - UpdateFunc: func(old, new interface{}) { - oldCM := old.(*apiv1.ConfigMap) - newCM := new.(*apiv1.ConfigMap) - if oldCM.ResourceVersion == newCM.ResourceVersion { - return + extraCMInformer := e.newExtraInformer() + funcs := cache.ResourceEventHandlerFuncs{ + AddFunc: func(obj interface{}) { + if cm, ok := obj.(*apiv1.ConfigMap); ok { + err := e.syncUpdate(cm, onUpdated) + if err != nil { + log.Error(err) + } else { + log.Infof("RBAC ConfigMap '%s' added", cm.Name) } - err := e.syncUpdate(newCM, onUpdated) + } + }, + UpdateFunc: func(old, new interface{}) { + oldCM := old.(*apiv1.ConfigMap) + newCM := new.(*apiv1.ConfigMap) + if oldCM.ResourceVersion == newCM.ResourceVersion { + return + } + err := e.syncUpdate(newCM, onUpdated) + if err != nil { + log.Error(err) + } else { + log.Infof("RBAC ConfigMap '%s' updated", newCM.Name) + } + }, + DeleteFunc: func(obj interface{}) { + if cm, ok := obj.(*apiv1.ConfigMap); ok { + err := e.syncRemove(cm, onUpdated) if err != nil { log.Error(err) } else { - log.Infof("RBAC ConfigMap '%s' updated", e.configmap) + log.Infof("RBAC ConfigMap '%s' removed", cm.Name) } - }, + } }, - ) + } + _, err := cmInformer.AddEventHandler(funcs) + if err != nil { + log.Error(err) + } + _, err = extraCMInformer.AddEventHandler(funcs) if err != nil { log.Error(err) } - log.Info("Starting rbac config informer") - cmInformer.Run(ctx.Done()) - log.Info("rbac configmap informer cancelled") + var wait sync.WaitGroup + wait.Add(2) + go func() { + defer wait.Done() + log.Info("Starting rbac config informer") + cmInformer.Run(ctx.Done()) + log.Info("rbac configmap informer cancelled") + }() + go func() { + defer wait.Done() + log.Info("Starting extra rbac config informer") + extraCMInformer.Run(ctx.Done()) + log.Info("extra rbac configmap informer cancelled") + }() + wait.Wait() } -// PolicyCSV will generate the final policy csv to be used -// by Argo CD RBAC. It will find entries in the given data -// that matches the policy key name convention: -// -// policy[.overlay].csv -func PolicyCSV(data map[string]string) string { - var strBuilder strings.Builder +func policyCSV(strBuilder *strings.Builder, data map[string]string) { // add the main policy first if p, ok := data[ConfigMapPolicyCSVKey]; ok { strBuilder.WriteString(p) @@ -426,20 +463,81 @@ func PolicyCSV(data map[string]string) string { strBuilder.WriteString(value) } } +} + +// PolicyCSV will generate the final policy csv to be used +// by Argo CD RBAC from a single configmap. It will find entries +// in the given data that matches the policy key name convention: +// +// policy[.overlay].csv +func PolicyCSV(data map[string]string) string { + var strBuilder strings.Builder + policyCSV(&strBuilder, data) + return strBuilder.String() +} + +// PolicyCSV will generate the final policy csv to be used +// by Argo CD RBAC from many configmaps. It will find entries +// in the given data that matches the policy key name convention: +// +// policy[.overlay].csv +func PolicyCSVMany(datas []map[string]string) string { + var strBuilder strings.Builder + // add the main policy first + first := true + for _, data := range datas { + if _, ok := data[ConfigMapPolicyCSVKey]; ok && !first { + strBuilder.WriteString("\n") + } + first = false + + policyCSV(&strBuilder, data) + } return strBuilder.String() } -// syncUpdate updates the enforcer -func (e *Enforcer) syncUpdate(cm *apiv1.ConfigMap, onUpdated func(cm *apiv1.ConfigMap) error) error { - e.SetDefaultRole(cm.Data[ConfigMapPolicyDefaultKey]) - e.SetMatchMode(cm.Data[ConfigMapMatchModeKey]) - policyCSV := PolicyCSV(cm.Data) +// sync updates the enforcer after a configmap has been upserted or removed +func (e *Enforcer) sync(cm *apiv1.ConfigMap, onUpdated func(cm *apiv1.ConfigMap) error) error { + cmNames := make([]string, 0, len(e.policyCSVCache)) + for key := range e.policyCSVCache { + cmNames = append(cmNames, key) + } + slices.Sort(cmNames) + datas := make([]map[string]string, len(cmNames)) + for ix, name := range cmNames { + datas[ix] = e.policyCSVCache[name] + } + + policyCSV := PolicyCSVMany(datas) if err := onUpdated(cm); err != nil { return err } return e.SetUserPolicy(policyCSV) } +// syncUpdate updates the enforcer to upsert a configmap +func (e *Enforcer) syncUpdate(cm *apiv1.ConfigMap, onUpdated func(cm *apiv1.ConfigMap) error) error { + if cm.Name == e.configmap { + e.SetDefaultRole(cm.Data[ConfigMapPolicyDefaultKey]) + e.SetMatchMode(cm.Data[ConfigMapMatchModeKey]) + } + e.policyCSVCache[cm.Name] = cm.Data + + return e.sync(cm, onUpdated) +} + +// syncUpdate updates the enforcer to remove a configmap +func (e *Enforcer) syncRemove(cm *apiv1.ConfigMap, onUpdated func(cm *apiv1.ConfigMap) error) error { + if cm.Name == e.configmap { + // This should never happen? + // Assume this is user error and ignore it + return nil + } + delete(e.policyCSVCache, cm.Name) + + return e.sync(cm, onUpdated) +} + // ValidatePolicy verifies a policy string is acceptable to casbin func ValidatePolicy(policy string) error { _, err := newEnforcerSafe(globMatchFunc, newBuiltInModel(), newAdapter("", "", policy)) diff --git a/util/rbac/rbac_test.go b/util/rbac/rbac_test.go index f0843952cd2e9..bd6225c38615a 100644 --- a/util/rbac/rbac_test.go +++ b/util/rbac/rbac_test.go @@ -19,8 +19,9 @@ import ( ) const ( - fakeConfigMapName = "fake-cm" - fakeNamespace = "fake-ns" + fakeConfigMapName = "fake-cm" + fakeExtraConfigMapName = "fake-cm-extra" + fakeNamespace = "fake-ns" ) var noOpUpdate = func(cm *apiv1.ConfigMap) error { @@ -42,13 +43,31 @@ func fakeConfigMap() *apiv1.ConfigMap { return &cm } -func TestPolicyCSV(t *testing.T) { +func fakeExtraConfigMap() *apiv1.ConfigMap { + cm := apiv1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + Kind: "ConfigMap", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: fakeExtraConfigMapName, + Namespace: fakeNamespace, + Labels: map[string]string{ + "argocd.argoproj.io/cm-type": "policy-csv", + }, + }, + Data: make(map[string]string), + } + return &cm +} + +func TestPolicyCSVMany(t *testing.T) { t.Run("will return empty string if data has no csv entries", func(t *testing.T) { // given data := make(map[string]string) // when - policy := PolicyCSV(data) + policy := PolicyCSVMany([]map[string]string{data}) // then assert.Equal(t, "", policy) @@ -61,7 +80,7 @@ func TestPolicyCSV(t *testing.T) { data["UnrelatedKey"] = "unrelated value" // when - policy := PolicyCSV(data) + policy := PolicyCSVMany([]map[string]string{data}) // then assert.Equal(t, expectedPolicy, policy) @@ -75,12 +94,37 @@ func TestPolicyCSV(t *testing.T) { data["policy.overlay2.csv"] = "policy3" // when - policy := PolicyCSV(data) + policy := PolicyCSVMany([]map[string]string{data}) + + // then + assert.Regexp(t, "^policy1", policy) + assert.Contains(t, policy, "policy2") + assert.Contains(t, policy, "policy3") + }) + t.Run("will return composed policy provided by multiple policy keys from multiple configmaps", func(t *testing.T) { + // given + data := make(map[string]string) + data[ConfigMapPolicyCSVKey] = "policy1" + data["UnrelatedKey"] = "unrelated value" + data["policy.overlay1.csv"] = "policy2" + data["policy.overlay2.csv"] = "policy3" + + data2 := make(map[string]string) + data2[ConfigMapPolicyCSVKey] = "policy4" + data2["UnrelatedKey"] = "unrelated value" + data2["policy.overlay1.csv"] = "policy5" + data2["policy.overlay3.csv"] = "policy6" + + // when + policy := PolicyCSVMany([]map[string]string{data, data2}) // then assert.Regexp(t, "^policy1", policy) assert.Contains(t, policy, "policy2") assert.Contains(t, policy, "policy3") + assert.Contains(t, policy, "policy4") + assert.Contains(t, policy, "policy5") + assert.Contains(t, policy, "policy6") }) t.Run("will return composed policy in a deterministic order", func(t *testing.T) { // given @@ -92,7 +136,7 @@ func TestPolicyCSV(t *testing.T) { data[ConfigMapPolicyCSVKey] = "policy1" // when - policy := PolicyCSV(data) + policy := PolicyCSVMany([]map[string]string{data}) // then result := strings.Split(policy, "\n") @@ -102,6 +146,38 @@ func TestPolicyCSV(t *testing.T) { assert.Equal(t, "policyb", result[2]) assert.Equal(t, "policyc", result[3]) }) + t.Run("will return composed policy from multiple configmaps in a deterministic order", func(t *testing.T) { + // given + data := make(map[string]string) + data["UnrelatedKey"] = "unrelated value" + data["policy.B.csv"] = "policyb" + data["policy.A.csv"] = "policya" + data["policy.C.csv"] = "policyc" + data[ConfigMapPolicyCSVKey] = "policy1" + + data2 := make(map[string]string) + data2["UnrelatedKey"] = "unrelated value" + data2["policy.C.csv"] = "policy2c" + data2["policy.A.csv"] = "policy2a" + data2["policy.E.csv"] = "policy2e" + data2[ConfigMapPolicyCSVKey] = "policy2" + + // when + policy := PolicyCSVMany([]map[string]string{data, data2}) + + // then + result := strings.Split(policy, "\n") + assert.Len(t, result, 8) + assert.Equal(t, "policy1", result[0]) + assert.Equal(t, "policya", result[1]) + assert.Equal(t, "policyb", result[2]) + assert.Equal(t, "policyc", result[3]) + assert.Equal(t, "policy2", result[4]) + assert.Equal(t, "policy2a", result[5]) + assert.Equal(t, "policy2c", result[6]) + assert.Equal(t, "policy2e", result[7]) + }) + } // TestBuiltinPolicyEnforcer tests the builtin policy rules @@ -109,6 +185,7 @@ func TestBuiltinPolicyEnforcer(t *testing.T) { kubeclientset := fake.NewSimpleClientset() enf := NewEnforcer(kubeclientset, fakeNamespace, fakeConfigMapName, nil) require.NoError(t, enf.syncUpdate(fakeConfigMap(), noOpUpdate)) + require.NoError(t, enf.syncUpdate(fakeExtraConfigMap(), noOpUpdate)) // Without setting builtin policy, this should fail assert.False(t, enf.Enforce("admin", "applications", "get", "foo/bar")) @@ -181,6 +258,7 @@ func TestDefaultRole(t *testing.T) { kubeclientset := fake.NewSimpleClientset() enf := NewEnforcer(kubeclientset, fakeNamespace, fakeConfigMapName, nil) require.NoError(t, enf.syncUpdate(fakeConfigMap(), noOpUpdate)) + require.NoError(t, enf.syncUpdate(fakeExtraConfigMap(), noOpUpdate)) _ = enf.SetBuiltinPolicy(assets.BuiltinPolicyCSV) assert.False(t, enf.Enforce("bob", "applications", "get", "foo/bar")) @@ -434,6 +512,25 @@ p, alice, clusters, get, "https://github.com/argo[a-z]{4}/argo-[a-z]+.git", allo assert.False(t, enf.Enforce("alice", "clusters", "get", "https://github.com/argoproj/1argo-cd.git")) } +func TestPolicyRemovedOnExtraConfigMapDelete(t *testing.T) { + policy1 := `p, alice, clusters, get, "https://github.com/*/*.git", allow` + policy2 := `p, bob, clusters, get, "https://github.com/*/*.git", allow` + cm := fakeConfigMap() + cm.Data[ConfigMapPolicyCSVKey] = policy1 + cm2 := fakeExtraConfigMap() + cm2.Data[ConfigMapPolicyCSVKey] = policy2 + kubeclientset := fake.NewSimpleClientset() + enf := NewEnforcer(kubeclientset, fakeNamespace, fakeConfigMapName, nil) + err := enf.syncUpdate(cm, noOpUpdate) + assert.Nil(t, err) + err = enf.syncUpdate(cm2, noOpUpdate) + assert.Nil(t, err) + assert.Equal(t, policy1+"\n"+policy2, enf.adapter.userDefinedPolicy) + err = enf.syncRemove(cm2, noOpUpdate) + assert.Nil(t, err) + assert.Equal(t, policy1, enf.adapter.userDefinedPolicy) +} + func TestGlobMatchFunc(t *testing.T) { ok, _ := globMatchFunc("arg1") assert.False(t, ok.(bool))