Skip to content

Tighten discovery service permissions #29717

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
greedy52 merged 3 commits into from
Aug 3, 2023
Merged

Tighten discovery service permissions #29717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
512a838
Limit Discovery Service permission by cloud origin label
greedy52 Jul 27, 2023
448c12b
fix ut
greedy52 Jul 28, 2023
98d53a7
disallow dynamic labels for kube cluster
greedy52 Jul 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions lib/auth/auth_with_roles.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5402,6 +5402,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))
}

Expand All @@ -5422,6 +5426,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))
}

Expand Down Expand Up @@ -5532,6 +5540,10 @@ func (a *ServerWithRoles) CreateDatabase(ctx context.Context, database types.Dat
if err := a.checkAccessToDatabase(database); err != nil {
return trace.Wrap(err)
}
// Don't allow discovery service to create databases with dynamic labels.
if a.hasBuiltinRole(types.RoleDiscovery) && len(database.GetDynamicLabels()) > 0 {
Copy link
Copy Markdown
Contributor

@tigrato tigrato Jul 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please include the same check into kube endpoints?

Copy link
Copy Markdown
Contributor

@smallinsky smallinsky Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is identity was generated for DBDiscovery and DBDatabse so the a.hasBuiltinRole(types.RoleDiscovery) && a.hasBuiltinRole(types.RoleDatabase) == true and CreateDatabase method was called from db agent process ?

Copy link
Copy Markdown
Contributor

@GavinFrazar GavinFrazar Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is identity was generated for DBDiscovery and DBDatabse so the a.hasBuiltinRole(types.RoleDiscovery) && a.hasBuiltinRole(types.RoleDatabase) == true and CreateDatabase method was called from db agent process ?

DB service doesn't call CreateDatabase right? Worth testing this scenario but I don't think it should be a problem

Copy link
Copy Markdown
Contributor Author

@greedy52 greedy52 Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please include the same check into kube endpoints?

Thanks! I missed as somehow i thought kube cluster doesn't have dynamic labels. Added now in 98d53a7

What is identity was generated for DBDiscovery and DBDatabse so the a.hasBuiltinRole(types.RoleDiscovery) && a.hasBuiltinRole(types.RoleDatabase) == true and CreateDatabase method was called from db agent process ?

DB service doesn't call CreateDatabase right? Worth testing this scenario but I don't think it should be a problem

Yes, database service doesn't call CreateDatabase. Also, even if it does it wouldn't be a problem:

teleport/lib/service/db.go

Lines 42 to 45 in 8a1d0e2

func (process *TeleportProcess) initDatabases() {
process.RegisterWithAuthServer(types.RoleDatabase, DatabasesIdentityEvent)
process.RegisterCriticalFunc("db.init", process.initDatabaseService)
}

Discovery vs database services in the same process use its own auth connector with a single system role.

return trace.AccessDenied("discovered database must not have dynamic labels")
}
return trace.Wrap(a.authServer.CreateDatabase(ctx, database))
}

Expand All @@ -5552,6 +5564,10 @@ func (a *ServerWithRoles) UpdateDatabase(ctx context.Context, database types.Dat
if err := a.checkAccessToDatabase(database); err != nil {
return trace.Wrap(err)
}
// Don't allow discovery service to create databases with dynamic labels.
if a.hasBuiltinRole(types.RoleDiscovery) && len(database.GetDynamicLabels()) > 0 {
return trace.AccessDenied("discovered database must not have dynamic labels")
}
return trace.Wrap(a.authServer.UpdateDatabase(ctx, database))
}

Expand Down
144 changes: 138 additions & 6 deletions lib/auth/auth_with_roles_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import (
"testing"
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/eks"
"github.com/coreos/go-semver/semver"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
Expand Down Expand Up @@ -1827,18 +1829,148 @@ func TestDatabasesCRUDRBAC(t *testing.T) {
// Dev should only be able to delete dev database.
err = devClt.DeleteAllDatabases(ctx)
require.NoError(t, err)
dbs, err = adminClt.GetDatabases(ctx)
mustGetDatabases(t, adminClt, []types.Database{adminDatabase})

// Admin should be able to delete all.
err = adminClt.DeleteAllDatabases(ctx)
require.NoError(t, err)
require.Empty(t, cmp.Diff([]types.Database{adminDatabase}, dbs,
mustGetDatabases(t, adminClt, nil)

t.Run("discovery service", func(t *testing.T) {
t.Cleanup(func() {
require.NoError(t, adminClt.DeleteAllDatabases(ctx))
})

// Prepare discovery service client.
discoveryClt, err := srv.NewClient(TestBuiltin(types.RoleDiscovery))
require.NoError(t, err)

cloudDatabase, err := types.NewDatabaseV3(types.Metadata{
Name: "cloud1",
Labels: map[string]string{"env": "prod", types.OriginLabel: types.OriginCloud},
}, types.DatabaseSpecV3{
Protocol: libdefaults.ProtocolMySQL,
URI: "localhost:3306",
})
require.NoError(t, err)

// Create a non-cloud database.
require.NoError(t, adminClt.CreateDatabase(ctx, adminDatabase))
mustGetDatabases(t, adminClt, []types.Database{adminDatabase})

t.Run("cannot create non-cloud database", func(t *testing.T) {
require.True(t, trace.IsAccessDenied(discoveryClt.CreateDatabase(ctx, devDatabase)))
require.True(t, trace.IsAccessDenied(discoveryClt.UpdateDatabase(ctx, adminDatabase)))
})
t.Run("cannot create database with dynamic labels", func(t *testing.T) {
cloudDatabaseWithDynamicLabels, err := types.NewDatabaseV3(types.Metadata{
Name: "cloud2",
Labels: map[string]string{"env": "prod", types.OriginLabel: types.OriginCloud},
}, types.DatabaseSpecV3{
Protocol: libdefaults.ProtocolMySQL,
URI: "localhost:3306",
DynamicLabels: map[string]types.CommandLabelV2{
"hostname": types.CommandLabelV2{
Period: types.Duration(time.Hour),
Command: []string{"hostname"},
},
},
})
require.NoError(t, err)
require.True(t, trace.IsAccessDenied(discoveryClt.CreateDatabase(ctx, cloudDatabaseWithDynamicLabels)))
})
t.Run("can create cloud database", func(t *testing.T) {
require.NoError(t, discoveryClt.CreateDatabase(ctx, cloudDatabase))
require.NoError(t, discoveryClt.UpdateDatabase(ctx, cloudDatabase))
})
t.Run("can get only cloud database", func(t *testing.T) {
mustGetDatabases(t, discoveryClt, []types.Database{cloudDatabase})
})
t.Run("can delete only cloud database", func(t *testing.T) {
require.NoError(t, discoveryClt.DeleteAllDatabases(ctx))
mustGetDatabases(t, discoveryClt, nil)
mustGetDatabases(t, adminClt, []types.Database{adminDatabase})
})
})
}

func mustGetDatabases(t *testing.T, client *Client, wantDatabases []types.Database) {
t.Helper()

actualDatabases, err := client.GetDatabases(context.Background())
require.NoError(t, err)

require.Empty(t, cmp.Diff(wantDatabases, actualDatabases,
cmpopts.IgnoreFields(types.Metadata{}, "ID"),
cmpopts.EquateEmpty(),
))
}

// Admin should be able to delete all.
err = adminClt.DeleteAllDatabases(ctx)
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)
dbs, err = adminClt.GetDatabases(ctx)

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.Len(t, dbs, 0)
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) {
Expand Down
5 changes: 4 additions & 1 deletion lib/auth/grpcserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4897,7 +4897,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)
}
Expand Down
6 changes: 3 additions & 3 deletions lib/authz/permissions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -874,9 +874,9 @@ func definitionForBuiltinRole(clusterName string, recConfig types.SessionRecordi
types.NewRule(types.KindDatabase, services.RW()),
types.NewRule(types.KindServerInfo, services.RW()),
},
// wildcard any cluster available.
KubernetesLabels: types.Labels{types.Wildcard: []string{types.Wildcard}},
DatabaseLabels: types.Labels{types.Wildcard: []string{types.Wildcard}},
// Discovery service should only access kubes/dbs with "cloud" origin.
KubernetesLabels: types.Labels{types.OriginLabel: []string{types.OriginCloud}},
DatabaseLabels: types.Labels{types.OriginLabel: []string{types.OriginCloud}},
},
})
case types.RoleOkta:
Expand Down
6 changes: 5 additions & 1 deletion lib/srv/discovery/discovery_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1210,7 +1210,11 @@ func TestDiscoveryDatabase(t *testing.T) {

select {
case <-waitForReconcile:
actualDatabases, err := authClient.GetDatabases(ctx)
// Use tlsServer.Auth() instead of authClient to compare
// databases stored in auth. authClient was created with
// types.RoleDiscovery and it does not have permissions to
// access non-cloud databases.
actualDatabases, err := tlsServer.Auth().GetDatabases(ctx)
require.NoError(t, err)
require.Empty(t, cmp.Diff(tc.expectDatabases, actualDatabases,
cmpopts.IgnoreFields(types.Metadata{}, "ID"),
Expand Down