From 3913bcf18f13689d49de7e1f2bca4424c7b97442 Mon Sep 17 00:00:00 2001 From: "STeve (Xin) Huang" Date: Thu, 3 Aug 2023 10:20:10 -0400 Subject: [PATCH] Tighten discovery service permissions (#29717) * Limit Discovery Service permission by cloud origin label * fix ut * disallow dynamic labels for kube cluster --- lib/auth/auth_with_roles.go | 8 ++++ lib/auth/auth_with_roles_test.go | 69 ++++++++++++++++++++++++++++++++ lib/auth/grpcserver.go | 5 ++- lib/auth/permissions.go | 4 +- 4 files changed, 83 insertions(+), 3 deletions(-) diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index 94bfca977780d..8ce829b8e3f79 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -4883,6 +4883,10 @@ func (a *ServerWithRoles) CreateKubernetesCluster(ctx context.Context, cluster t if err := a.checkAccessToKubeCluster(cluster); err != nil { return trace.Wrap(err) } + // Don't allow discovery service to create clusters with dynamic labels. + if a.hasBuiltinRole(types.RoleDiscovery) && len(cluster.GetDynamicLabels()) > 0 { + return trace.AccessDenied("discovered kubernetes cluster must not have dynamic labels") + } return trace.Wrap(a.authServer.CreateKubernetesCluster(ctx, cluster)) } @@ -4903,6 +4907,10 @@ func (a *ServerWithRoles) UpdateKubernetesCluster(ctx context.Context, cluster t if err := a.checkAccessToKubeCluster(cluster); err != nil { return trace.Wrap(err) } + // Don't allow discovery service to create clusters with dynamic labels. + if a.hasBuiltinRole(types.RoleDiscovery) && len(cluster.GetDynamicLabels()) > 0 { + return trace.AccessDenied("discovered kubernetes cluster must not have dynamic labels") + } return trace.Wrap(a.authServer.UpdateKubernetesCluster(ctx, cluster)) } diff --git a/lib/auth/auth_with_roles_test.go b/lib/auth/auth_with_roles_test.go index 3e8d9fb43558f..77dad814886f3 100644 --- a/lib/auth/auth_with_roles_test.go +++ b/lib/auth/auth_with_roles_test.go @@ -25,6 +25,8 @@ import ( "testing" "time" + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/eks" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/google/uuid" @@ -2046,6 +2048,73 @@ func TestDatabasesCRUDRBAC(t *testing.T) { require.Len(t, dbs, 0) } +func TestKubernetesClusterCRUD_DiscoveryService(t *testing.T) { + t.Parallel() + ctx := context.Background() + srv := newTestTLSServer(t) + + discoveryClt, err := srv.NewClient(TestBuiltin(types.RoleDiscovery)) + require.NoError(t, err) + + eksCluster, err := services.NewKubeClusterFromAWSEKS(&eks.Cluster{ + Name: aws.String("eks-cluster1"), + Arn: aws.String("arn:aws:eks:eu-west-1:accountID:cluster/cluster1"), + Status: aws.String(eks.ClusterStatusActive), + }) + require.NoError(t, err) + + // Discovery service must not have access to non-cloud cluster (cluster + // without "cloud" origin label). + nonCloudCluster, err := types.NewKubernetesClusterV3( + types.Metadata{ + Name: "non-cloud", + }, + types.KubernetesClusterSpecV3{}, + ) + require.NoError(t, err) + require.NoError(t, srv.Auth().CreateKubernetesCluster(ctx, nonCloudCluster)) + + // Discovery service cannot create cluster with dynamic labels. + clusterWithDynamicLabels, err := services.NewKubeClusterFromAWSEKS(&eks.Cluster{ + Name: aws.String("eks-cluster2"), + Arn: aws.String("arn:aws:eks:eu-west-1:accountID:cluster/cluster2"), + Status: aws.String(eks.ClusterStatusActive), + }) + require.NoError(t, err) + clusterWithDynamicLabels.SetDynamicLabels(map[string]types.CommandLabel{ + "hostname": &types.CommandLabelV2{ + Period: types.Duration(time.Hour), + Command: []string{"hostname"}, + }, + }) + + t.Run("Create", func(t *testing.T) { + require.NoError(t, discoveryClt.CreateKubernetesCluster(ctx, eksCluster)) + require.True(t, trace.IsAccessDenied(discoveryClt.CreateKubernetesCluster(ctx, nonCloudCluster))) + require.True(t, trace.IsAccessDenied(discoveryClt.CreateKubernetesCluster(ctx, clusterWithDynamicLabels))) + }) + t.Run("Read", func(t *testing.T) { + clusters, err := discoveryClt.GetKubernetesClusters(ctx) + require.NoError(t, err) + require.Equal(t, clusters, []types.KubeCluster{eksCluster}) + }) + t.Run("Update", func(t *testing.T) { + require.NoError(t, discoveryClt.UpdateKubernetesCluster(ctx, eksCluster)) + require.True(t, trace.IsAccessDenied(discoveryClt.UpdateKubernetesCluster(ctx, nonCloudCluster))) + }) + t.Run("Delete", func(t *testing.T) { + require.NoError(t, discoveryClt.DeleteAllKubernetesClusters(ctx)) + clusters, err := discoveryClt.GetKubernetesClusters(ctx) + require.NoError(t, err) + require.Empty(t, clusters) + + // Discovery service cannot delete non-cloud clusters. + clusters, err = srv.Auth().GetKubernetesClusters(ctx) + require.NoError(t, err) + require.Len(t, clusters, 1) + }) +} + func TestGetAndList_DatabaseServers(t *testing.T) { t.Parallel() ctx := context.Background() diff --git a/lib/auth/grpcserver.go b/lib/auth/grpcserver.go index d01886afa3a9a..0f419c99653df 100644 --- a/lib/auth/grpcserver.go +++ b/lib/auth/grpcserver.go @@ -4490,7 +4490,10 @@ func (g *GRPCServer) UpdateKubernetesCluster(ctx context.Context, cluster *types if err != nil { return nil, trace.Wrap(err) } - cluster.SetOrigin(types.OriginDynamic) + // if origin is not set, force it to be dynamic. + if len(cluster.Origin()) == 0 { + cluster.SetOrigin(types.OriginDynamic) + } if err := auth.UpdateKubernetesCluster(ctx, cluster); err != nil { return nil, trace.Wrap(err) } diff --git a/lib/auth/permissions.go b/lib/auth/permissions.go index e94993a7e03b9..c693b445182e8 100644 --- a/lib/auth/permissions.go +++ b/lib/auth/permissions.go @@ -746,8 +746,8 @@ func definitionForBuiltinRole(clusterName string, recConfig types.SessionRecordi types.NewRule(types.KindNode, services.RO()), types.NewRule(types.KindKubernetesCluster, services.RW()), }, - // wildcard any cluster available. - KubernetesLabels: types.Labels{types.Wildcard: []string{types.Wildcard}}, + // Discovery service should only access kubes with "cloud" origin. + KubernetesLabels: types.Labels{types.OriginLabel: []string{types.OriginCloud}}, }, }) }