Argo CD users with any cluster secret update access may update out-of-bounds cluster secrets

@@ -8,6 +8,8 @@ import (
 	"time"
 
 	"github.com/sirupsen/logrus"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 // Default service addresses and URLS of Argo CD internal services
@@ -316,3 +318,5 @@ const (
 const TokenVerificationError = "failed to verify the token"
 
 var TokenVerificationErr = errors.New(TokenVerificationError)
+
+var PermissionDeniedAPIError = status.Error(codes.PermissionDenied, "permission denied")

@@ -1,11 +1,10 @@
 package cluster
 
 import (
+	"context"
 	"net/url"
 	"time"
 
-	"context"
-
 	"github.com/argoproj/gitops-engine/pkg/utils/kube"
 	log "github.com/sirupsen/logrus"
 	"google.golang.org/grpc/codes"
@@ -14,6 +13,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/kubernetes"
 
+	"github.com/argoproj/argo-cd/v2/common"
 	"github.com/argoproj/argo-cd/v2/pkg/apiclient/cluster"
 	appv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 	servercache "github.com/argoproj/argo-cd/v2/server/cache"
@@ -135,7 +135,7 @@ func (s *Server) Get(ctx context.Context, q *cluster.ClusterQuery) (*appv1.Clust
 func (s *Server) getClusterWith403IfNotExist(ctx context.Context, q *cluster.ClusterQuery) (*appv1.Cluster, error) {
 	repo, err := s.getCluster(ctx, q)
 	if err != nil || repo == nil {
-		return nil, status.Error(codes.PermissionDenied, "permission denied")
+		return nil, common.PermissionDeniedAPIError
 	}
 	return repo, nil
 }
@@ -221,14 +221,14 @@ func (s *Server) Update(ctx context.Context, q *cluster.ClusterUpdateRequest) (*
 	}
 
 	// verify that user can do update inside project where cluster is located
-	if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceClusters, rbacpolicy.ActionUpdate, createRBACObject(c.Project, q.Cluster.Server)); err != nil {
-		return nil, err
+	if !s.enf.Enforce(ctx.Value("claims"), rbacpolicy.ResourceClusters, rbacpolicy.ActionUpdate, createRBACObject(c.Project, c.Server)) {
+		return nil, common.PermissionDeniedAPIError
 	}
 
 	if len(q.UpdatedFields) == 0 || sets.NewString(q.UpdatedFields...).Has("project") {
 		// verify that user can do update inside project where cluster will be located
-		if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceClusters, rbacpolicy.ActionUpdate, createRBACObject(q.Cluster.Project, q.Cluster.Server)); err != nil {
-			return nil, err
+		if !s.enf.Enforce(ctx.Value("claims"), rbacpolicy.ResourceClusters, rbacpolicy.ActionUpdate, createRBACObject(q.Cluster.Project, c.Server)) {
+			return nil, common.PermissionDeniedAPIError
 		}
 	}
 

@@ -3,6 +3,7 @@ package cluster
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"testing"
 	"time"
 
@@ -49,6 +50,117 @@ func newNoopEnforcer() *rbac.Enforcer {
 	return enf
 }
 
+func TestUpdateCluster_RejectInvalidParams(t *testing.T) {
+	testCases := []struct {
+		name    string
+		request clusterapi.ClusterUpdateRequest
+	}{
+		{
+			name:    "allowed cluster URL in body, disallowed cluster URL in query",
+			request: clusterapi.ClusterUpdateRequest{Cluster: &v1alpha1.Cluster{Name: "", Server: "https://127.0.0.1", Project: "", ClusterResources: true}, Id: &clusterapi.ClusterID{Type: "", Value: "https://127.0.0.2"}, UpdatedFields: []string{"clusterResources", "project"}},
+		},
+		{
+			name:    "allowed cluster URL in body, disallowed cluster name in query",
+			request: clusterapi.ClusterUpdateRequest{Cluster: &v1alpha1.Cluster{Name: "", Server: "https://127.0.0.1", Project: "", ClusterResources: true}, Id: &clusterapi.ClusterID{Type: "name", Value: "disallowed-unscoped"}, UpdatedFields: []string{"clusterResources", "project"}},
+		},
+		{
+			name:    "allowed cluster URL in body, disallowed cluster name in query, changing unscoped to scoped",
+			request: clusterapi.ClusterUpdateRequest{Cluster: &v1alpha1.Cluster{Name: "", Server: "https://127.0.0.1", Project: "allowed-project", ClusterResources: true}, Id: &clusterapi.ClusterID{Type: "", Value: "https://127.0.0.2"}, UpdatedFields: []string{"clusterResources", "project"}},
+		},
+		{
+			name:    "allowed cluster URL in body, disallowed cluster URL in query, changing unscoped to scoped",
+			request: clusterapi.ClusterUpdateRequest{Cluster: &v1alpha1.Cluster{Name: "", Server: "https://127.0.0.1", Project: "allowed-project", ClusterResources: true}, Id: &clusterapi.ClusterID{Type: "name", Value: "disallowed-unscoped"}, UpdatedFields: []string{"clusterResources", "project"}},
+		},
+	}
+
+	db := &dbmocks.ArgoDB{}
+
+	clusters := []v1alpha1.Cluster{
+		{
+			Name:   "allowed-unscoped",
+			Server: "https://127.0.0.1",
+		},
+		{
+			Name:   "disallowed-unscoped",
+			Server: "https://127.0.0.2",
+		},
+		{
+			Name:    "allowed-scoped",
+			Server:  "https://127.0.0.3",
+			Project: "allowed-project",
+		},
+		{
+			Name:    "disallowed-scoped",
+			Server:  "https://127.0.0.4",
+			Project: "disallowed-project",
+		},
+	}
+
+	db.On("ListClusters", mock.Anything).Return(
+		func(ctx context.Context) *v1alpha1.ClusterList {
+			return &v1alpha1.ClusterList{
+				ListMeta: v1.ListMeta{},
+				Items:    clusters,
+			}
+		},
+		func(ctx context.Context) error {
+			return nil
+		},
+	)
+	db.On("UpdateCluster", mock.Anything, mock.Anything).Return(
+		func(ctx context.Context, c *v1alpha1.Cluster) *v1alpha1.Cluster {
+			for _, cluster := range clusters {
+				if c.Server == cluster.Server {
+					return c
+				}
+			}
+			return nil
+		},
+		func(ctx context.Context, c *v1alpha1.Cluster) error {
+			for _, cluster := range clusters {
+				if c.Server == cluster.Server {
+					return nil
+				}
+			}
+			return fmt.Errorf("cluster '%s' not found", c.Server)
+		},
+	)
+	db.On("GetCluster", mock.Anything, mock.Anything).Return(
+		func(ctx context.Context, server string) *v1alpha1.Cluster {
+			for _, cluster := range clusters {
+				if server == cluster.Server {
+					return &cluster
+				}
+			}
+			return nil
+		},
+		func(ctx context.Context, server string) error {
+			for _, cluster := range clusters {
+				if server == cluster.Server {
+					return nil
+				}
+			}
+			return fmt.Errorf("cluster '%s' not found", server)
+		},
+	)
+
+	enf := rbac.NewEnforcer(fake.NewSimpleClientset(test.NewFakeConfigMap()), test.FakeArgoCDNamespace, common.ArgoCDConfigMapName, nil)
+	_ = enf.SetBuiltinPolicy(`p, role:test, clusters, *, https://127.0.0.1, allow
+p, role:test, clusters, *, allowed-project/*, allow`)
+	enf.SetDefaultRole("role:test")
+	server := NewServer(db, enf, newServerInMemoryCache(), &kubetest.MockKubectlCmd{})
+
+	for _, c := range testCases {
+		cc := c
+		t.Run(cc.name, func(t *testing.T) {
+			t.Parallel()
+			out, err := server.Update(context.Background(), &cc.request)
+			require.Nil(t, out)
+			assert.ErrorIs(t, err, common.PermissionDeniedAPIError)
+		})
+	}
+}
+
 func TestGetCluster_UrlEncodedName(t *testing.T) {
 	db := &dbmocks.ArgoDB{}