From d84f7fb7d881eaa83bb926c8afc705a051df0029 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Fri, 1 Jul 2022 06:23:55 -0700 Subject: [PATCH] [v10] Fix resource access requests for apps (#14026) --- lib/services/access_request.go | 125 ++++++++++++++++----------- lib/services/access_request_test.go | 126 ++++++++++++++++++---------- tool/tsh/access_request.go | 48 +---------- 3 files changed, 160 insertions(+), 139 deletions(-) diff --git a/lib/services/access_request.go b/lib/services/access_request.go index 21ef11e4ee6b1..13b2bf2ea6e79 100644 --- a/lib/services/access_request.go +++ b/lib/services/access_request.go @@ -18,11 +18,14 @@ package services import ( "context" + "fmt" "sort" + "strings" "time" "github.com/google/go-cmp/cmp" + "github.com/gravitational/teleport/api/client/proto" apidefaults "github.com/gravitational/teleport/api/defaults" "github.com/gravitational/teleport/api/types" apiutils "github.com/gravitational/teleport/api/utils" @@ -1301,11 +1304,7 @@ func MarshalAccessRequest(accessRequest types.AccessRequest, opts ...MarshalOpti // PruneResourceRequestGetter is the access interface necessary for PruneResourceRequestRoles. type PruneResourceRequestGetter interface { GetRole(ctx context.Context, name string) (types.Role, error) - GetNode(ctx context.Context, namespace string, name string) (types.Server, error) - GetKubeServices(ctx context.Context) ([]types.Server, error) - GetDatabase(ctx context.Context, name string) (types.Database, error) - GetApp(ctx context.Context, name string) (types.Application, error) - GetWindowsDesktops(ctx context.Context, filter types.WindowsDesktopFilter) ([]types.WindowsDesktop, error) + ListResources(ctx context.Context, req proto.ListResourcesRequest) (*types.ListResourcesResponse, error) } // PruneResourceRequestRoles takes an access request and does one of two things: @@ -1450,55 +1449,85 @@ func roleAllowsResource( } func getResources(ctx context.Context, getter PruneResourceRequestGetter, resourceIDs []types.ResourceID) ([]types.ResourceWithLabels, error) { - var resources []types.ResourceWithLabels + resourceNamesByKind := make(map[string][]string) for _, resourceID := range resourceIDs { - switch resourceID.Kind { - case types.KindNode: - node, err := getter.GetNode(ctx, apidefaults.Namespace, resourceID.Name) - if err != nil { - return nil, trace.Wrap(err) - } - resources = append(resources, node) - case types.KindKubernetesCluster: - kubeServices, err := getter.GetKubeServices(ctx) + resourceNamesByKind[resourceID.Kind] = append(resourceNamesByKind[resourceID.Kind], resourceID.Name) + } + var resources []types.ResourceWithLabels + for kind, resourceNames := range resourceNamesByKind { + req := proto.ListResourcesRequest{ + ResourceType: MapResourceKindToListResourcesType(kind), + PredicateExpression: anyNameMatcher(resourceNames), + Limit: int32(len(resourceNames)), + } + resp, err := getter.ListResources(ctx, req) + if err != nil { + return nil, trace.Wrap(err) + } + for _, result := range resp.Resources { + leafResources, err := MapListResourcesResultToLeafResource(result, kind) if err != nil { return nil, trace.Wrap(err) } - for _, kubeService := range kubeServices { - for _, kubeCluster := range kubeService.GetKubernetesClusters() { - if kubeCluster.Name != resourceID.Name { - continue - } - kubeV3, err := types.NewKubernetesClusterV3FromLegacyCluster(kubeService.GetNamespace(), kubeCluster) - if err != nil { - return nil, trace.Wrap(err) - } - resources = append(resources, kubeV3) + resources = append(resources, leafResources...) + } + } + return resources, nil +} + +// anyNameMatcher returns a PredicateExpression which matches any of a given list +// of names. Given names will be escaped and quoted when building the expression. +func anyNameMatcher(names []string) string { + matchers := make([]string, len(names)) + for i := range names { + matchers[i] = fmt.Sprintf(`name == %q`, names[i]) + } + return strings.Join(matchers, " || ") +} + +// MapResourceKindToListResourcesType returns the value to use for ResourceType in a +// ListResourcesRequest based on the kind of resource you're searching for. +// Necessary because some resource kinds don't support ListResources directly, +// so you have to list the parent kind. Use MapListResourcesResultToLeafResource to map back +// to the given kind. +func MapResourceKindToListResourcesType(kind string) string { + switch kind { + case types.KindApp: + return types.KindAppServer + case types.KindDatabase: + return types.KindDatabaseServer + case types.KindKubernetesCluster: + return types.KindKubeService + default: + return kind + } +} + +// MapListResourcesResultToLeafResource is the inverse of +// MapResourceKindToListResourcesType, after the ListResources call it maps the +// result back to the kind we really want. `hint` should be the name of the +// desired resource kind, used to disambiguate normal SSH nodes and kubernetes +// services which are both returned as `types.Server`. +func MapListResourcesResultToLeafResource(resource types.ResourceWithLabels, hint string) (types.ResourcesWithLabels, error) { + switch r := resource.(type) { + case types.AppServer: + return types.ResourcesWithLabels{r.GetApp()}, nil + case types.DatabaseServer: + return types.ResourcesWithLabels{r.GetDatabase()}, nil + case types.Server: + if hint == types.KindKubernetesCluster { + kubeClusters := r.GetKubernetesClusters() + resources := make(types.ResourcesWithLabels, len(kubeClusters)) + for i := range kubeClusters { + resource, err := types.NewKubernetesClusterV3FromLegacyCluster(apidefaults.Namespace, kubeClusters[i]) + if err != nil { + return nil, trace.Wrap(err) } + resources[i] = resource } - case types.KindDatabase: - db, err := getter.GetDatabase(ctx, resourceID.Name) - if err != nil { - return nil, trace.Wrap(err) - } - resources = append(resources, db) - case types.KindApp: - app, err := getter.GetApp(ctx, resourceID.Name) - if err != nil { - return nil, trace.Wrap(err) - } - resources = append(resources, app) - case types.KindWindowsDesktop: - desktops, err := getter.GetWindowsDesktops(ctx, types.WindowsDesktopFilter{ - Name: resourceID.Name, - }) - if err != nil { - return nil, trace.Wrap(err) - } - for _, desktop := range desktops { - resources = append(resources, desktop) - } + return resources, nil } + default: } - return resources, nil + return types.ResourcesWithLabels{resource}, nil } diff --git a/lib/services/access_request_test.go b/lib/services/access_request_test.go index 5f61df5b68616..8a41b5589df5e 100644 --- a/lib/services/access_request_test.go +++ b/lib/services/access_request_test.go @@ -18,8 +18,10 @@ package services import ( "context" + "strings" "testing" + "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/fixtures" @@ -31,13 +33,13 @@ import ( // mockGetter mocks the UserAndRoleGetter interface. type mockGetter struct { - users map[string]types.User - roles map[string]types.Role - nodes map[string]types.Server - kubeServices []types.Server - dbs map[string]types.Database - apps map[string]types.Application - desktops map[string]types.WindowsDesktop + users map[string]types.User + roles map[string]types.Role + nodes map[string]types.Server + kubeServers map[string]types.Server + dbServers map[string]types.DatabaseServer + appServers map[string]types.AppServer + desktops map[string]types.WindowsDesktop } // user inserts a new user with the specified roles and returns the username. @@ -78,40 +80,37 @@ func (m *mockGetter) GetRoles(ctx context.Context) ([]types.Role, error) { return roles, nil } -func (m *mockGetter) GetNode(ctx context.Context, namespace string, name string) (types.Server, error) { - node, ok := m.nodes[name] - if !ok { - return nil, trace.NotFound("no such node: %q", name) +// ListResources is a very dumb implementation for the mockGetter that just +// returns all resources which have names matching the request +// PredicateExpression. +func (m *mockGetter) ListResources(ctx context.Context, req proto.ListResourcesRequest) (*types.ListResourcesResponse, error) { + resp := &types.ListResourcesResponse{} + for nodeName, node := range m.nodes { + if strings.Contains(req.PredicateExpression, nodeName) { + resp.Resources = append(resp.Resources, types.ResourceWithLabels(node)) + } } - return node, nil -} - -func (m *mockGetter) GetKubeServices(ctx context.Context) ([]types.Server, error) { - return append([]types.Server{}, m.kubeServices...), nil -} - -func (m *mockGetter) GetDatabase(ctx context.Context, name string) (types.Database, error) { - db, ok := m.dbs[name] - if !ok { - return nil, trace.NotFound("no such db: %q", name) + for kubeName, kubeService := range m.kubeServers { + if strings.Contains(req.PredicateExpression, kubeName) { + resp.Resources = append(resp.Resources, types.ResourceWithLabels(kubeService)) + } } - return db, nil -} - -func (m *mockGetter) GetApp(ctx context.Context, name string) (types.Application, error) { - app, ok := m.apps[name] - if !ok { - return nil, trace.NotFound("no such app: %q", name) + for dbName, dbServer := range m.dbServers { + if strings.Contains(req.PredicateExpression, dbName) { + resp.Resources = append(resp.Resources, dbServer) + } } - return app, nil -} - -func (m *mockGetter) GetWindowsDesktops(ctx context.Context, filter types.WindowsDesktopFilter) ([]types.WindowsDesktop, error) { - desktop, ok := m.desktops[filter.Name] - if !ok { - return nil, trace.NotFound("no such desktop: %q", filter.Name) + for appName, appServer := range m.appServers { + if strings.Contains(req.PredicateExpression, appName) { + resp.Resources = append(resp.Resources, appServer) + } } - return []types.WindowsDesktop{desktop}, nil + for desktopName, desktop := range m.desktops { + if strings.Contains(req.PredicateExpression, desktopName) { + resp.Resources = append(resp.Resources, desktop) + } + } + return resp, nil } // TestReviewThresholds tests various review threshold scenarios @@ -1044,12 +1043,13 @@ func TestPruneRequestRoles(t *testing.T) { ctx := context.Background() g := &mockGetter{ - roles: make(map[string]types.Role), - users: make(map[string]types.User), - nodes: make(map[string]types.Server), - dbs: make(map[string]types.Database), - apps: make(map[string]types.Application), - desktops: make(map[string]types.WindowsDesktop), + roles: make(map[string]types.Role), + users: make(map[string]types.User), + nodes: make(map[string]types.Server), + kubeServers: make(map[string]types.Server), + dbServers: make(map[string]types.DatabaseServer), + appServers: make(map[string]types.AppServer), + desktops: make(map[string]types.WindowsDesktop), } // set up test roles @@ -1142,6 +1142,12 @@ func TestPruneRequestRoles(t *testing.T) { "owner": "node-admins", }, }, + { + name: "admins-node-2", + labels: map[string]string{ + "owner": "node-admins", + }, + }, { name: "denied-node", }, @@ -1161,7 +1167,7 @@ func TestPruneRequestRoles(t *testing.T) { }, }, nil) require.NoError(t, err) - g.kubeServices = append(g.kubeServices, kube) + g.kubeServers[kube.GetName()] = kube db, err := types.NewDatabaseV3(types.Metadata{ Name: "db", @@ -1170,7 +1176,15 @@ func TestPruneRequestRoles(t *testing.T) { URI: "example.com:3000", }) require.NoError(t, err) - g.dbs[db.GetName()] = db + dbServer, err := types.NewDatabaseServerV3(types.Metadata{ + Name: db.GetName(), + }, types.DatabaseServerSpecV3{ + HostID: "db-server", + Hostname: "db-server", + Database: db, + }) + require.NoError(t, err) + g.dbServers[dbServer.GetName()] = dbServer app, err := types.NewAppV3(types.Metadata{ Name: "app", @@ -1178,7 +1192,9 @@ func TestPruneRequestRoles(t *testing.T) { URI: "example.com:3000", }) require.NoError(t, err) - g.apps[app.GetName()] = app + appServer, err := types.NewAppServerV3FromApp(app, "app-server", "app-server") + require.NoError(t, err) + g.appServers[app.GetName()] = appServer desktop, err := types.NewWindowsDesktopV3("windows", nil, types.WindowsDesktopSpecV3{ Addr: "example.com:3001", @@ -1222,6 +1238,24 @@ func TestPruneRequestRoles(t *testing.T) { // With "responder" login hint, only request node-access. expectRoles: []string{"node-access"}, }, + { + desc: "multiple nodes", + requestResourceIDs: []types.ResourceID{ + { + ClusterName: clusterName, + Kind: types.KindNode, + Name: "admins-node", + }, + { + ClusterName: clusterName, + Kind: types.KindNode, + Name: "admins-node-2", + }, + }, + loginHint: "responder", + // With "responder" login hint, only request node-access. + expectRoles: []string{"node-access"}, + }, { desc: "root login hint", requestResourceIDs: []types.ResourceID{ diff --git a/tool/tsh/access_request.go b/tool/tsh/access_request.go index ee4c77dfb9105..5ca014ea2b530 100644 --- a/tool/tsh/access_request.go +++ b/tool/tsh/access_request.go @@ -26,7 +26,6 @@ import ( "github.com/gravitational/teleport" "github.com/gravitational/teleport/api/client" "github.com/gravitational/teleport/api/client/proto" - apidefaults "github.com/gravitational/teleport/api/defaults" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/asciitable" "github.com/gravitational/teleport/lib/auth" @@ -392,7 +391,7 @@ func onRequestSearch(cf *CLIConf) error { clusterName := clusterNameResource.GetClusterName() req := proto.ListResourcesRequest{ - ResourceType: searchKindFixup(cf.ResourceKind), + ResourceType: services.MapResourceKindToListResourcesType(cf.ResourceKind), Labels: tc.Labels, PredicateExpression: cf.PredicateExpression, SearchKeywords: tc.SearchKeywords, @@ -406,11 +405,11 @@ func onRequestSearch(cf *CLIConf) error { var resources types.ResourcesWithLabels for _, result := range results { - fixedResources, err := resultKindFixup(result, cf.ResourceKind) + leafResources, err := services.MapListResourcesResultToLeafResource(result, cf.ResourceKind) if err != nil { return trace.Wrap(err) } - resources = append(resources, fixedResources...) + resources = append(resources, leafResources...) } rows := [][]string{} @@ -450,44 +449,3 @@ To request access to these resources, run return nil } - -func searchKindFixup(kind string) string { - // Some resource kinds don't support search directly, run the search on the - // parent kind instead. - switch kind { - case types.KindApp: - return types.KindAppServer - case types.KindDatabase: - return types.KindDatabaseServer - case types.KindKubernetesCluster: - return types.KindKubeService - default: - return kind - } -} - -func resultKindFixup(resource types.ResourceWithLabels, hint string) (types.ResourcesWithLabels, error) { - // The inverse of searchKindFixup, after the search map the result back to - // the kind we really want. - switch r := resource.(type) { - case types.AppServer: - return types.ResourcesWithLabels{r.GetApp()}, nil - case types.DatabaseServer: - return types.ResourcesWithLabels{r.GetDatabase()}, nil - case types.Server: - if hint == types.KindKubernetesCluster { - kubeClusters := r.GetKubernetesClusters() - resources := make(types.ResourcesWithLabels, 0, len(kubeClusters)) - for _, kubeCluster := range kubeClusters { - resource, err := types.NewKubernetesClusterV3FromLegacyCluster(apidefaults.Namespace, kubeCluster) - if err != nil { - return nil, trace.Wrap(err) - } - resources = append(resources, resource) - } - return resources, nil - } - default: - } - return types.ResourcesWithLabels{resource}, nil -}