Skip to content

Commit

Permalink
Merge pull request from GHSA-3cqf-953p-h5cp
Browse files Browse the repository at this point in the history 
* fix: prevent enumerating by cluster name, return exact error for case when cluster exists and not

Signed-off-by: pashakostohrys <pavel@codefresh.io>

* fix: prevent cluster enumeration by name

Signed-off-by: pashakostohrys <pavel@codefresh.io>

* fix: prevent cluster enumeration by name

Signed-off-by: pashakostohrys <pavel@codefresh.io>

* fix linter and add unit test

Signed-off-by: pashakostohrys <pavel@codefresh.io>

* fix linter and add unit test

Signed-off-by: pashakostohrys <pavel@codefresh.io>

* fix linter and add unit test

Signed-off-by: pashakostohrys <pavel@codefresh.io>

* fix linter and add unit test

Signed-off-by: pashakostohrys <pavel@codefresh.io>

* fix linter and add unit test

Signed-off-by: pashakostohrys <pavel@codefresh.io>

---------

Signed-off-by: pashakostohrys <pavel@codefresh.io>
  • Loading branch information
@pasha-codefresh
pasha-codefresh authored Jun 6, 2024
1 parent 256d901 commit c264705
Show file tree
Hide file tree
Showing 2 changed files with 192 additions and 21 deletions.
51 changes: 30 additions & 21 deletions server/cluster/cluster.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,15 +187,11 @@ func (s *Server) Create(ctx context.Context, q *cluster.ClusterCreateRequest) (*

// Get returns a cluster from a query
func (s *Server) Get(ctx context.Context, q *cluster.ClusterQuery) (*appv1.Cluster, error) {
c, err := s.getClusterWith403IfNotExist(ctx, q)
c, err := s.getClusterAndVerifyAccess(ctx, q, rbacpolicy.ActionGet)
if err != nil {
return nil, err
}

if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceClusters, rbacpolicy.ActionGet, CreateClusterRBACObject(c.Project, q.Server)); err != nil {
return nil, err
}

return s.toAPIResponse(c), nil
}

Expand All @@ -207,6 +203,21 @@ func (s *Server) getClusterWith403IfNotExist(ctx context.Context, q *cluster.Clu
return repo, nil
}

func (s *Server) getClusterAndVerifyAccess(ctx context.Context, q *cluster.ClusterQuery, action string) (*appv1.Cluster, error) {
c, err := s.getClusterWith403IfNotExist(ctx, q)
if err != nil {
return nil, err
}

// verify that user can do the specified action inside project where cluster is located
if !s.enf.Enforce(ctx.Value("claims"), rbacpolicy.ResourceClusters, action, CreateClusterRBACObject(c.Project, c.Server)) {
log.WithField("cluster", q.Server).Warnf("encountered permissions issue while processing request: %v", err)
return nil, common.PermissionDeniedAPIError
}

return c, nil
}

func (s *Server) getCluster(ctx context.Context, q *cluster.ClusterQuery) (*appv1.Cluster, error) {
if q.Id != nil {
q.Server = ""
Expand Down Expand Up @@ -278,20 +289,16 @@ var clusterFieldsByPath = map[string]func(updated *appv1.Cluster, existing *appv

// Update updates a cluster
func (s *Server) Update(ctx context.Context, q *cluster.ClusterUpdateRequest) (*appv1.Cluster, error) {
c, err := s.getClusterWith403IfNotExist(ctx, &cluster.ClusterQuery{
c, err := s.getClusterAndVerifyAccess(ctx, &cluster.ClusterQuery{
Server: q.Cluster.Server,
Name: q.Cluster.Name,
Id: q.Id,
})
}, rbacpolicy.ActionUpdate)

if err != nil {
return nil, err
}

// verify that user can do update inside project where cluster is located
if !s.enf.Enforce(ctx.Value("claims"), rbacpolicy.ResourceClusters, rbacpolicy.ActionUpdate, CreateClusterRBACObject(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 !s.enf.Enforce(ctx.Value("claims"), rbacpolicy.ResourceClusters, rbacpolicy.ActionUpdate, CreateClusterRBACObject(q.Cluster.Project, c.Server)) {
Expand Down Expand Up @@ -341,7 +348,8 @@ func (s *Server) Delete(ctx context.Context, q *cluster.ClusterQuery) (*cluster.
if q.Name != "" {
servers, err := s.db.GetClusterServersByName(ctx, q.Name)
if err != nil {
return nil, err
log.WithField("cluster", q.Name).Warnf("failed to get cluster servers by name: %v", err)
return nil, common.PermissionDeniedAPIError
}
for _, server := range servers {
if err := enforceAndDelete(s, ctx, server, c.Project); err != nil {
Expand All @@ -359,7 +367,8 @@ func (s *Server) Delete(ctx context.Context, q *cluster.ClusterQuery) (*cluster.

func enforceAndDelete(s *Server, ctx context.Context, server, project string) error {
if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceClusters, rbacpolicy.ActionDelete, CreateClusterRBACObject(project, server)); err != nil {
return err
log.WithField("cluster", server).Warnf("encountered permissions issue while processing request: %v", err)
return common.PermissionDeniedAPIError
}
if err := s.db.DeleteCluster(ctx, server); err != nil {
return err
Expand All @@ -378,16 +387,19 @@ func (s *Server) RotateAuth(ctx context.Context, q *cluster.ClusterQuery) (*clus
if q.Name != "" {
servers, err = s.db.GetClusterServersByName(ctx, q.Name)
if err != nil {
return nil, status.Errorf(codes.NotFound, "failed to get cluster servers by name: %v", err)
log.WithField("cluster", q.Name).Warnf("failed to get cluster servers by name: %v", err)
return nil, common.PermissionDeniedAPIError
}
for _, server := range servers {
if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceClusters, rbacpolicy.ActionUpdate, CreateClusterRBACObject(clust.Project, server)); err != nil {
return nil, status.Errorf(codes.PermissionDenied, "encountered permissions issue while processing request: %v", err)
log.WithField("cluster", server).Warnf("encountered permissions issue while processing request: %v", err)
return nil, common.PermissionDeniedAPIError
}
}
} else {
if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceClusters, rbacpolicy.ActionUpdate, CreateClusterRBACObject(clust.Project, q.Server)); err != nil {
return nil, status.Errorf(codes.PermissionDenied, "encountered permissions issue while processing request: %v", err)
log.WithField("cluster", q.Server).Warnf("encountered permissions issue while processing request: %v", err)
return nil, common.PermissionDeniedAPIError
}
servers = append(servers, q.Server)
}
Expand Down Expand Up @@ -467,13 +479,10 @@ func (s *Server) toAPIResponse(clust *appv1.Cluster) *appv1.Cluster {

// InvalidateCache invalidates cluster cache
func (s *Server) InvalidateCache(ctx context.Context, q *cluster.ClusterQuery) (*appv1.Cluster, error) {
cls, err := s.getClusterWith403IfNotExist(ctx, q)
cls, err := s.getClusterAndVerifyAccess(ctx, q, rbacpolicy.ActionUpdate)
if err != nil {
return nil, err
}
if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceClusters, rbacpolicy.ActionUpdate, CreateClusterRBACObject(cls.Project, q.Server)); err != nil {
return nil, err
}
now := v1.Now()
cls.RefreshRequestedAt = &now
cls, err = s.db.UpdateCluster(ctx, cls)
Expand Down
162 changes: 162 additions & 0 deletions server/cluster/cluster_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import (
"context"
"encoding/json"
"fmt"
"github.com/argoproj/argo-cd/v2/server/rbacpolicy"
"github.com/argoproj/argo-cd/v2/util/assets"
"github.com/golang-jwt/jwt/v4"
"reflect"
"testing"
"time"
Expand Down Expand Up @@ -51,6 +54,16 @@ func newNoopEnforcer() *rbac.Enforcer {
return enf
}

func newEnforcer() *rbac.Enforcer {
enforcer := rbac.NewEnforcer(fake.NewSimpleClientset(test.NewFakeConfigMap()), test.FakeArgoCDNamespace, common.ArgoCDRBACConfigMapName, nil)
_ = enforcer.SetBuiltinPolicy(assets.BuiltinPolicyCSV)
enforcer.SetDefaultRole("role:test")
enforcer.SetClaimsEnforcerFunc(func(claims jwt.Claims, rvals ...interface{}) bool {
return true
})
return enforcer
}

func TestUpdateCluster_RejectInvalidParams(t *testing.T) {
testCases := []struct {
name string
Expand Down Expand Up @@ -604,3 +617,152 @@ func TestListCluster(t *testing.T) {
})
}
}

func TestGetClusterAndVerifyAccess(t *testing.T) {
t.Run("GetClusterAndVerifyAccess - No Cluster", func(t *testing.T) {
db := &dbmocks.ArgoDB{}

mockCluster := v1alpha1.Cluster{
Name: "test/ing",
Server: "https://127.0.0.1",
Namespaces: []string{"default", "kube-system"},
}
mockClusterList := v1alpha1.ClusterList{
ListMeta: v1.ListMeta{},
Items: []v1alpha1.Cluster{
mockCluster,
},
}

db.On("ListClusters", mock.Anything).Return(&mockClusterList, nil)

server := NewServer(db, newNoopEnforcer(), newServerInMemoryCache(), &kubetest.MockKubectlCmd{})
cluster, err := server.getClusterAndVerifyAccess(context.Background(), &clusterapi.ClusterQuery{
Name: "test/not-exists",
}, rbacpolicy.ActionGet)

assert.Nil(t, cluster)
assert.ErrorIs(t, err, common.PermissionDeniedAPIError)
})

t.Run("GetClusterAndVerifyAccess - Permissions Denied", func(t *testing.T) {
db := &dbmocks.ArgoDB{}

mockCluster := v1alpha1.Cluster{
Name: "test/ing",
Server: "https://127.0.0.1",
Namespaces: []string{"default", "kube-system"},
}
mockClusterList := v1alpha1.ClusterList{
ListMeta: v1.ListMeta{},
Items: []v1alpha1.Cluster{
mockCluster,
},
}

db.On("ListClusters", mock.Anything).Return(&mockClusterList, nil)

server := NewServer(db, newEnforcer(), newServerInMemoryCache(), &kubetest.MockKubectlCmd{})
cluster, err := server.getClusterAndVerifyAccess(context.Background(), &clusterapi.ClusterQuery{
Name: "test/ing",
}, rbacpolicy.ActionGet)

assert.Nil(t, cluster)
assert.ErrorIs(t, err, common.PermissionDeniedAPIError)
})
}

func TestNoClusterEnumeration(t *testing.T) {
db := &dbmocks.ArgoDB{}

mockCluster := v1alpha1.Cluster{
Name: "test/ing",
Server: "https://127.0.0.1",
Namespaces: []string{"default", "kube-system"},
}
mockClusterList := v1alpha1.ClusterList{
ListMeta: v1.ListMeta{},
Items: []v1alpha1.Cluster{
mockCluster,
},
}

db.On("ListClusters", mock.Anything).Return(&mockClusterList, nil)
db.On("GetCluster", mock.Anything, mock.Anything).Return(&mockCluster, nil)

server := NewServer(db, newEnforcer(), newServerInMemoryCache(), &kubetest.MockKubectlCmd{})

t.Run("Get", func(t *testing.T) {
_, err := server.Get(context.Background(), &clusterapi.ClusterQuery{
Name: "cluster-not-exists",
})
assert.Error(t, err)
assert.Equal(t, common.PermissionDeniedAPIError.Error(), err.Error(), "error message must be _only_ the permission error, to avoid leaking information about cluster existence")

_, err = server.Get(context.Background(), &clusterapi.ClusterQuery{
Name: "test/ing",
})
assert.Error(t, err)
assert.Equal(t, common.PermissionDeniedAPIError.Error(), err.Error(), "error message must be _only_ the permission error, to avoid leaking information about cluster existence")
})

t.Run("Update", func(t *testing.T) {
_, err := server.Update(context.Background(), &clusterapi.ClusterUpdateRequest{
Cluster: &v1alpha1.Cluster{
Name: "cluster-not-exists",
},
})
assert.Error(t, err)
assert.Equal(t, common.PermissionDeniedAPIError.Error(), err.Error(), "error message must be _only_ the permission error, to avoid leaking information about cluster existence")

_, err = server.Update(context.Background(), &clusterapi.ClusterUpdateRequest{
Cluster: &v1alpha1.Cluster{
Name: "test/ing",
},
})
assert.Error(t, err)
assert.Equal(t, common.PermissionDeniedAPIError.Error(), err.Error(), "error message must be _only_ the permission error, to avoid leaking information about cluster existence")
})

t.Run("Delete", func(t *testing.T) {
_, err := server.Delete(context.Background(), &clusterapi.ClusterQuery{
Server: "https://127.0.0.2",
})
assert.Error(t, err)
assert.Equal(t, common.PermissionDeniedAPIError.Error(), err.Error(), "error message must be _only_ the permission error, to avoid leaking information about cluster existence")

_, err = server.Delete(context.Background(), &clusterapi.ClusterQuery{
Server: "https://127.0.0.1",
})
assert.Error(t, err)
assert.Equal(t, common.PermissionDeniedAPIError.Error(), err.Error(), "error message must be _only_ the permission error, to avoid leaking information about cluster existence")
})

t.Run("RotateAuth", func(t *testing.T) {
_, err := server.RotateAuth(context.Background(), &clusterapi.ClusterQuery{
Server: "https://127.0.0.2",
})
assert.Error(t, err)
assert.Equal(t, common.PermissionDeniedAPIError.Error(), err.Error(), "error message must be _only_ the permission error, to avoid leaking information about cluster existence")

_, err = server.RotateAuth(context.Background(), &clusterapi.ClusterQuery{
Server: "https://127.0.0.1",
})
assert.Error(t, err)
assert.Equal(t, common.PermissionDeniedAPIError.Error(), err.Error(), "error message must be _only_ the permission error, to avoid leaking information about cluster existence")
})

t.Run("InvalidateCache", func(t *testing.T) {
_, err := server.InvalidateCache(context.Background(), &clusterapi.ClusterQuery{
Server: "https://127.0.0.2",
})
assert.Error(t, err)
assert.Equal(t, common.PermissionDeniedAPIError.Error(), err.Error(), "error message must be _only_ the permission error, to avoid leaking information about cluster existence")

_, err = server.InvalidateCache(context.Background(), &clusterapi.ClusterQuery{
Server: "https://127.0.0.1",
})
assert.Error(t, err)
assert.Equal(t, common.PermissionDeniedAPIError.Error(), err.Error(), "error message must be _only_ the permission error, to avoid leaking information about cluster existence")
})
}

0 comments on commit c264705

Please sign in to comment.