From fbb0b99b1ac3361b253052bd30259fa43a520945 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Date: Thu, 16 Feb 2023 09:07:57 -0500 Subject: [PATCH] Merge pull request from GHSA-3jfq-742w-xg8j fix test name Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --- common/common.go | 4 ++ server/cluster/cluster.go | 14 ++--- server/cluster/cluster_test.go | 112 +++++++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 7 deletions(-) diff --git a/common/common.go b/common/common.go index c026ab3c22d44..c22064e94757b 100644 --- a/common/common.go +++ b/common/common.go @@ -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") diff --git a/server/cluster/cluster.go b/server/cluster/cluster.go index 2225a5d6ff9fc..f39396f0ca736 100644 --- a/server/cluster/cluster.go +++ b/server/cluster/cluster.go @@ -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 } } diff --git a/server/cluster/cluster_test.go b/server/cluster/cluster_test.go index dec7d97b8d263..c5b07a4ee7055 100644 --- a/server/cluster/cluster_test.go +++ b/server/cluster/cluster_test.go @@ -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{}