fix(rbac): resolve RBAC regression for project-scoped resources in multi-namespace architecture (#25289)

assets/builtin-policy.csv

@@ -6,29 +6,29 @@
 # 2. All other resources:
 # p, <role/user/group>, <resource>, <action>, <object>, <allow/deny>
 
-p, role:readonly, applications, get, */*, allow
-p, role:readonly, applicationsets, get, */*, allow
+p, role:readonly, applications, get, */*/*, allow
+p, role:readonly, applicationsets, get, */*/*, allow
 p, role:readonly, certificates, get, *, allow
 p, role:readonly, clusters, get, *, allow
 p, role:readonly, repositories, get, *, allow
 p, role:readonly, write-repositories, get, *, allow
 p, role:readonly, projects, get, *, allow
 p, role:readonly, accounts, get, *, allow
 p, role:readonly, gpgkeys, get, *, allow
-p, role:readonly, logs, get, */*, allow
+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
-p, role:admin, applicationsets, get, */*, allow
-p, role:admin, applicationsets, create, */*, allow
-p, role:admin, applicationsets, update, */*, allow
-p, role:admin, applicationsets, delete, */*, 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
+p, role:admin, applicationsets, get, */*/*, allow
+p, role:admin, applicationsets, create, */*/*, allow
+p, role:admin, applicationsets, update, */*/*, allow
+p, role:admin, applicationsets, delete, */*/*, allow
 p, role:admin, certificates, create, *, allow
 p, role:admin, certificates, update, *, allow
 p, role:admin, certificates, delete, *, allow
@@ -47,7 +47,7 @@ p, role:admin, projects, delete, *, allow
 p, role:admin, accounts, update, *, allow
 p, role:admin, gpgkeys, create, *, allow
 p, role:admin, gpgkeys, delete, *, allow
-p, role:admin, exec, create, */*, allow
+p, role:admin, exec, create, */*/*, allow
 
 g, role:admin, role:readonly
 g, admin, role:admin

cmd/argocd/commands/admin/project.go

@@ -15,6 +15,7 @@ import (
 	"github.com/argoproj/argo-cd/v3/util/cli"
 	"github.com/argoproj/argo-cd/v3/util/errors"
 	utilio "github.com/argoproj/argo-cd/v3/util/io"
+	"github.com/argoproj/argo-cd/v3/util/rbac"
 	"github.com/argoproj/argo-cd/v3/util/templates"
 
 	"github.com/argoproj/argo-cd/gitops-engine/pkg/utils/kube"
@@ -93,7 +94,7 @@ func globMatch(pattern string, val string) bool {
 	return false
 }
 
-func getModification(modification string, resource string, scope string, permission string) (func(string, string) string, error) {
+func getModification(modification string, resource string, scope string, permission string, namespace string) (func(string, string) string, error) {
 	switch modification {
 	case "set":
 		if scope == "" {
@@ -103,7 +104,11 @@ func getModification(modification string, resource string, scope string, permiss
 			return nil, stderrors.New("flag --permission cannot be empty if permission should be set in role")
 		}
 		return func(proj string, action string) string {
-			return fmt.Sprintf("%s, %s, %s/%s, %s", resource, action, proj, scope, permission)
+			subresource := fmt.Sprintf("%s/%s", proj, scope)
+			if rbac.NeedNormalization(resource) {
+				subresource = rbac.NormalizeSubresource(subresource, namespace)
+			}
+			return fmt.Sprintf("%s, %s, %s, %s", resource, action, subresource, permission)
 		}, nil
 	case "remove":
 		return func(_ string, _ string) string {
@@ -181,7 +186,7 @@ func NewUpdatePolicyRuleCommand() *cobra.Command {
 			errors.CheckError(err)
 			appclients := appclientset.NewForConfigOrDie(config)
 
-			modification, err := getModification(modificationType, resource, scope, permission)
+			modification, err := getModification(modificationType, resource, scope, permission, namespace)
 			errors.CheckError(err)
 			projIf := appclients.ArgoprojV1alpha1().AppProjects(namespace)
 

cmd/argocd/commands/admin/project_test.go

@@ -1,6 +1,7 @@
 package admin
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -33,7 +34,7 @@ func TestUpdateProjects_FindMatchingProject(t *testing.T) {
 
 	clientset := fake.NewSimpleClientset(newProj("foo", "test"), newProj("bar", "test"))
 
-	modification, err := getModification("set", "*", "*", "allow")
+	modification, err := getModification("set", "*", "*", "allow", namespace)
 	require.NoError(t, err)
 	err = updateProjects(ctx, clientset.ArgoprojV1alpha1().AppProjects(namespace), "ba*", "*", "set", modification, false)
 	require.NoError(t, err)
@@ -47,12 +48,31 @@ func TestUpdateProjects_FindMatchingProject(t *testing.T) {
 	assert.Equal(t, []string{"p, proj:bar:test, *, set, bar/*, allow"}, barProj.Spec.Roles[0].Policies)
 }
 
+func TestUpdateProjects_FindMatchingProjectWithNamespace(t *testing.T) {
+	ctx := t.Context()
+
+	clientset := fake.NewSimpleClientset(newProj("foo", "test"), newProj("bar", "test"))
+
+	modification, err := getModification("set", "*", "test-ns/*", "allow", namespace)
+	require.NoError(t, err)
+	err = updateProjects(ctx, clientset.ArgoprojV1alpha1().AppProjects(namespace), "ba*", "*", "set", modification, false)
+	require.NoError(t, err)
+
+	fooProj, err := clientset.ArgoprojV1alpha1().AppProjects(namespace).Get(ctx, "foo", metav1.GetOptions{})
+	require.NoError(t, err)
+	assert.Empty(t, fooProj.Spec.Roles[0].Policies)
+
+	barProj, err := clientset.ArgoprojV1alpha1().AppProjects(namespace).Get(ctx, "bar", metav1.GetOptions{})
+	require.NoError(t, err)
+	assert.Equal(t, []string{"p, proj:bar:test, *, set, bar/test-ns/*, allow"}, barProj.Spec.Roles[0].Policies)
+}
+
 func TestUpdateProjects_FindMatchingRole(t *testing.T) {
 	ctx := t.Context()
 
 	clientset := fake.NewSimpleClientset(newProj("proj", "foo", "bar"))
 
-	modification, err := getModification("set", "*", "*", "allow")
+	modification, err := getModification("set", "*", "*", "allow", namespace)
 	require.NoError(t, err)
 	err = updateProjects(ctx, clientset.ArgoprojV1alpha1().AppProjects(namespace), "*", "fo*", "set", modification, false)
 	require.NoError(t, err)
@@ -63,21 +83,70 @@ func TestUpdateProjects_FindMatchingRole(t *testing.T) {
 	assert.Empty(t, proj.Spec.Roles[1].Policies)
 }
 
+func TestUpdateProjects_FindMatchingRoleWithNamespace(t *testing.T) {
+	ctx := t.Context()
+
+	clientset := fake.NewSimpleClientset(newProj("proj", "foo", "bar"))
+
+	modification, err := getModification("set", "*", "test-ns/*", "allow", namespace)
+	require.NoError(t, err)
+	err = updateProjects(ctx, clientset.ArgoprojV1alpha1().AppProjects(namespace), "*", "fo*", "set", modification, false)
+	require.NoError(t, err)
+
+	proj, err := clientset.ArgoprojV1alpha1().AppProjects(namespace).Get(ctx, "proj", metav1.GetOptions{})
+	require.NoError(t, err)
+	assert.Equal(t, []string{"p, proj:proj:foo, *, set, proj/test-ns/*, allow"}, proj.Spec.Roles[0].Policies)
+	assert.Empty(t, proj.Spec.Roles[1].Policies)
+}
+
 func TestGetModification_SetPolicy(t *testing.T) {
-	modification, err := getModification("set", "*", "*", "allow")
+	modification, err := getModification("set", "*", "*", "allow", namespace)
 	require.NoError(t, err)
 	policy := modification("proj", "myaction")
 	assert.Equal(t, "*, myaction, proj/*, allow", policy)
 }
 
+func TestGetModification_SetNormalizedPolicy(t *testing.T) {
+	resources := []string{"applications", "applicationsets", "logs", "exec"}
+
+	for _, resource := range resources {
+		modification, err := getModification("set", resource, "*", "allow", namespace)
+		require.NoError(t, err)
+		policy := modification("proj", "myaction")
+		assert.Equal(t, fmt.Sprintf("%s, myaction, proj/%s/*, allow", resource, namespace), policy)
+	}
+}
+
+func TestGetModification_SetPolicyWithNamespace(t *testing.T) {
+	modification, err := getModification("set", "*", "test-ns/*", "allow", namespace)
+	require.NoError(t, err)
+	policy := modification("proj", "myaction")
+	assert.Equal(t, "*, myaction, proj/test-ns/*, allow", policy)
+}
+
 func TestGetModification_RemovePolicy(t *testing.T) {
-	modification, err := getModification("remove", "*", "*", "allow")
+	modification, err := getModification("remove", "*", "*", "allow", namespace)
 	require.NoError(t, err)
 	policy := modification("proj", "myaction")
 	assert.Empty(t, policy)
 }
 
 func TestGetModification_NotSupported(t *testing.T) {
-	_, err := getModification("bar", "*", "*", "allow")
+	_, err := getModification("bar", "*", "*", "allow", namespace)
 	assert.Errorf(t, err, "modification bar is not supported")
 }
+
+func TestGetModification_SetWithNormalization(t *testing.T) {
+	// "applications" is a resource that requires normalization per rbac.NeedNormalization
+	f, err := getModification("set", "applications", "*", "allow", "argocd")
+	require.NoError(t, err)
+	assert.NotNil(t, f)
+
+	result := f("myproject", "get")
+	// The subresource should be normalized (namespace appended)
+	assert.Contains(t, result, "applications")
+	assert.Contains(t, result, "get")
+	assert.Contains(t, result, "allow")
+	// Verify normalization happened: subresource should contain the namespace
+	assert.Contains(t, result, "argocd")
+}

cmd/argocd/commands/admin/settings_rbac.go

@@ -208,7 +208,7 @@ argocd admin settings rbac can someuser create application 'default/app' --defau
 				defaultRole = newDefaultRole
 			}
 
-			res := checkPolicy(subject, action, resource, subResource, builtinPolicy, userPolicy, defaultRole, matchMode, strict)
+			res := checkPolicy(subject, action, resource, subResource, builtinPolicy, userPolicy, defaultRole, matchMode, strict, namespace)
 			if res {
 				if !quiet {
 					fmt.Println("Yes")
@@ -361,7 +361,11 @@ func getPolicyFromConfigMap(cm *corev1.ConfigMap) (string, string, string) {
 		defaultRole = ""
 	}
 
-	return rbac.PolicyCSV(cm.Data), defaultRole, cm.Data[rbac.ConfigMapMatchModeKey]
+	// according to argocd documentation: All resources have to be installed in the Argo CD namespace (by default argocd)
+	// we assume the namespace of the configmap (argocd-rbac-cm) is also in the Argo CD namespace
+	// if not so, "argocd admin settings rbac validate" and "argocd admin settings rbac can" commands can still work, but
+	// the CLI will refer to the user given namespace but not the Argo CD namespace
+	return rbac.PolicyCSV(cm.Data, cm.Namespace), defaultRole, cm.Data[rbac.ConfigMapMatchModeKey]
 }
 
 // getPolicyConfigMap fetches the RBAC config map from K8s cluster
@@ -375,8 +379,8 @@ func getPolicyConfigMap(ctx context.Context, client kubernetes.Interface, namesp
 
 // checkPolicy checks whether given subject is allowed to execute specified
 // action against specified resource
-func checkPolicy(subject, action, resource, subResource, builtinPolicy, userPolicy, defaultRole, matchMode string, strict bool) bool {
-	enf := rbac.NewEnforcer(nil, "argocd", "argocd-rbac-cm", nil)
+func checkPolicy(subject, action, resource, subResource, builtinPolicy, userPolicy, defaultRole, matchMode string, strict bool, namespace string) bool {
+	enf := rbac.NewEnforcer(nil, namespace, "argocd-rbac-cm", nil)
 	enf.SetDefaultRole(defaultRole)
 	enf.SetMatchMode(matchMode)
 	if builtinPolicy != "" {
@@ -411,13 +415,13 @@ func checkPolicy(subject, action, resource, subResource, builtinPolicy, userPoli
 
 	// Some project scoped resources have a special notation - for simplicity's sake,
 	// if user gives no sub-resource (or specifies simple '*'), we construct
-	// the required notation by setting subresource to '*/*'.
+	// the required notation by setting subresource to '*/*/*'.
 	if rbac.ProjectScoped[realResource] {
 		if subResource == "*" || subResource == "" {
-			subResource = "*/*"
+			subResource = "*/*/*"
 		}
 	}
-	return enf.Enforce(subject, realResource, action, subResource)
+	return enf.Enforce(subject, realResource, action, rbac.NormalizeSubresource(subResource, namespace))
 }
 
 // resolveRBACResourceName resolves a user supplied value to a valid RBAC