Skip to content
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.

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

Connect: Fetch correct role set when listing db users #13790

Merged
merged 10 commits into from
Jun 27, 2022
32 changes: 32 additions & 0 deletions lib/services/role.go
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,38 @@ func FetchRoles(roleNames []string, access RoleGetter, traits map[string][]strin
return NewRoleSet(roles...), nil
}

// CurrentUserRoleGetter limits the interface of auth.ClientI to methods needed by FetchClusterRoles.
type CurrentUserRoleGetter interface {
GetCurrentUser(context.Context) (types.User, error)
GetRole(ctx context.Context, name string) (types.Role, error)
ravicious marked this conversation as resolved.
Show resolved Hide resolved
}
codingllama marked this conversation as resolved.
Show resolved Hide resolved

// FetchAllClusterRoles fetches all roles available to the user on the specified cluster.
func FetchAllClusterRoles(ctx context.Context, currentUserRoleGetter CurrentUserRoleGetter, defaultRoles []string, defaultTraits wrappers.Traits) (RoleSet, error) {
codingllama marked this conversation as resolved.
Show resolved Hide resolved
roles := defaultRoles
traits := defaultTraits

// Typically, auth.ClientI is passed as currentUserRoleGetter. Older versions of the auth client
// may not implement GetCurrentUser() so we fail gracefully and use default roles and traits instead.
user, err := currentUserRoleGetter.GetCurrentUser(ctx)
if err == nil {
roles = user.GetRoles()
traits = user.GetTraits()
} else {
log.Debugf("Failed to fetch current user information: %v.", err)
}

// get the role definition for all roles of user.
// this may only fail if the role which we are looking for does not exist, or we don't have access to it.
// example scenario when this may happen:
// 1. we have set of roles [foo bar] from profile.
// 2. the cluster is remote and maps the [foo, bar] roles to single role [guest]
// 3. the remote cluster doesn't implement GetCurrentUser(), so we have no way to learn of [guest].
// 4. FetchRoles([foo bar], ..., ...) fails as [foo bar] does not exist on remote cluster.
roleSet, err := FetchRoles(roles, currentUserRoleGetter, traits)
return roleSet, trace.Wrap(err)
}

// ExtractRolesFromCert extracts roles from certificate metadata extensions.
func ExtractRolesFromCert(cert *ssh.Certificate) ([]string, error) {
data, ok := cert.Extensions[teleport.CertExtensionTeleportRoles]
Expand Down
108 changes: 108 additions & 0 deletions lib/services/role_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package services

import (
"bytes"
"context"
"encoding/json"
"fmt"
"strconv"
Expand Down Expand Up @@ -4937,3 +4938,110 @@ func TestHostUsers_CanCreateHostUser(t *testing.T) {
})
}
}

type mockCurrentUserRoleGetter struct {
currentUser types.User
nameToRole map[string]types.Role
}

func (m mockCurrentUserRoleGetter) GetCurrentUser(ctx context.Context) (types.User, error) {
if m.currentUser != nil {
return m.currentUser, nil
} else {
return nil, trace.NotFound("currentUser not set")
}
ravicious marked this conversation as resolved.
Show resolved Hide resolved
}

func (m mockCurrentUserRoleGetter) GetRole(ctx context.Context, name string) (types.Role, error) {
role, ok := m.nameToRole[name]

if ok {
return role, nil
} else {
return nil, trace.NotFound("role not found: %v", name)
}
ravicious marked this conversation as resolved.
Show resolved Hide resolved
}

type mockCurrentUser struct {
types.User
roles []string
traits wrappers.Traits
}

func (u mockCurrentUser) GetRoles() []string {
return u.roles
}

func (u mockCurrentUser) GetTraits() map[string][]string {
return u.traits
}

func TestFetchAllClusterRoles_PrefersRolesAndTraitsFromCurrentUser(t *testing.T) {
defaultRoles := []string{"access", "editor"}
defaultTraits := map[string][]string{
"logins": {"defaultTraitLogin"},
}

user := mockCurrentUser{
roles: []string{"dev", "admin"},
traits: map[string][]string{
"logins": {"currentUserTraitLogin"},
},
}

devRole := newRole(func(r *types.RoleV5) {
r.Metadata.Name = "dev"
r.Spec.Allow.Logins = []string{"{{internal.logins}}"}
})
adminRole := newRole(func(r *types.RoleV5) {
r.Metadata.Name = "admin"
})

currentUserRoleGetter := mockCurrentUserRoleGetter{
nameToRole: map[string]types.Role{
"dev": &devRole,
"admin": &adminRole,
},
currentUser: user,
}

roleSet, err := FetchAllClusterRoles(context.Background(), currentUserRoleGetter,
defaultRoles, defaultTraits)

require.NoError(t, err)

require.Contains(t, roleSet, &devRole, "devRole not found in roleSet")
require.Contains(t, roleSet, &adminRole, "adminRole not found in roleSet")
require.Equal(t, []string{"currentUserTraitLogin"}, roleSet[0].GetLogins(types.Allow))
codingllama marked this conversation as resolved.
Show resolved Hide resolved
}

func TestFetchAllClusterRoles_UsesDefaultRolesAndTraitsIfCurrentUserIsUnavailable(t *testing.T) {
Comment on lines +5012 to +5014
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's combine those 2 test cases in a single table-driven test, so we take advantage of their common setup.

https://github.com/golang/go/wiki/TableDrivenTests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it fine if I leave them as separate tests for now? IMHO updating something that was repeated just once doesn't require that much work vs untangling the code in case we need to add a third case that's just slightly different than the other two.

defaultRoles := []string{"access", "editor"}
defaultTraits := map[string][]string{
"logins": {"defaultTraitLogin"},
}

accessRole := newRole(func(r *types.RoleV5) {
r.Metadata.Name = "access"
r.Spec.Allow.Logins = []string{"{{internal.logins}}"}
})
editorRole := newRole(func(r *types.RoleV5) {
r.Metadata.Name = "editor"
})

currentUserRoleGetter := mockCurrentUserRoleGetter{
nameToRole: map[string]types.Role{
"access": &accessRole,
"editor": &editorRole,
},
}

roleSet, err := FetchAllClusterRoles(context.Background(), currentUserRoleGetter,
defaultRoles, defaultTraits)

require.NoError(t, err)

require.Contains(t, roleSet, &accessRole, "accessRole not found in roleSet")
require.Contains(t, roleSet, &editorRole, "editorRole not found in roleSet")
require.Equal(t, []string{"defaultTraitLogin"}, roleSet[0].GetLogins(types.Allow))
}
2 changes: 1 addition & 1 deletion lib/teleterm/clusters/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ type Cluster struct {
Name string

// Log is a component logger
Log logrus.FieldLogger
Log *logrus.Entry
// dir is the directory where cluster certificates are stored
dir string
// Status is the cluster status
Expand Down
27 changes: 22 additions & 5 deletions lib/teleterm/clusters/cluster_databases.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/gravitational/teleport/api/client/proto"
"github.com/gravitational/teleport/api/defaults"
"github.com/gravitational/teleport/api/types"
"github.com/gravitational/teleport/lib/auth"
"github.com/gravitational/teleport/lib/client"
dbprofile "github.com/gravitational/teleport/lib/client/db"
libdefaults "github.com/gravitational/teleport/lib/defaults"
Expand Down Expand Up @@ -152,17 +153,33 @@ func (c *Cluster) ReissueDBCerts(ctx context.Context, user, dbName string, db ty

// GetAllowedDatabaseUsers returns allowed users for the given database based on the role set.
func (c *Cluster) GetAllowedDatabaseUsers(ctx context.Context, dbURI string) ([]string, error) {
var roleSet services.RoleSet
var err error
var authClient auth.ClientI

err = addMetadataToRetryableError(ctx, func() error {
roleSet, err = services.FetchRoles(c.status.Roles, c.clusterClient, c.status.Traits)
return err
err := addMetadataToRetryableError(ctx, func() error {
proxyClient, err := c.clusterClient.ConnectToProxy(ctx)
if err != nil {
return trace.Wrap(err)
}
defer proxyClient.Close()

authClient, err = proxyClient.ConnectToCluster(ctx, c.clusterClient.SiteName)
if err != nil {
return trace.Wrap(err)
}

return nil
})
if err != nil {
return nil, trace.Wrap(err)
}

defer authClient.Close()

roleSet, err := services.FetchAllClusterRoles(ctx, authClient, c.status.Roles, c.status.Traits)
if err != nil {
return nil, trace.Wrap(err)
}

db, err := c.GetDatabase(ctx, dbURI)
if err != nil {
return nil, trace.Wrap(err)
Expand Down
2 changes: 1 addition & 1 deletion lib/teleterm/clusters/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ type Config struct {
// InsecureSkipVerify is an option to skip TLS cert check
InsecureSkipVerify bool
// Log is a component logger
Log logrus.FieldLogger
Log *logrus.Entry
}

// CheckAndSetDefaults checks the configuration for its validity and sets default values if needed
Expand Down
32 changes: 2 additions & 30 deletions tool/tsh/db.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ limitations under the License.
package main

import (
"context"
"encoding/base64"
"fmt"
"net"
Expand All @@ -28,7 +27,6 @@ import (
"github.com/gravitational/teleport"
"github.com/gravitational/teleport/api/client/proto"
"github.com/gravitational/teleport/api/types"
"github.com/gravitational/teleport/lib/auth"
"github.com/gravitational/teleport/lib/client"
dbprofile "github.com/gravitational/teleport/lib/client/db"
"github.com/gravitational/teleport/lib/client/db/dbcmd"
Expand Down Expand Up @@ -77,7 +75,7 @@ func onListDatabases(cf *CLIConf) error {
return trace.Wrap(err)
}

roleSet, err := fetchRoleSet(cf.Context, cluster, profile)
roleSet, err := services.FetchAllClusterRoles(cf.Context, cluster, profile.Roles, profile.Traits)
if err != nil {
log.Debugf("Failed to fetch user roles: %v.", err)
}
Expand Down Expand Up @@ -140,7 +138,7 @@ func listDatabasesAllClusters(cf *CLIConf) error {
continue
}

roleSet, err := fetchRoleSet(cf.Context, cluster, profile)
roleSet, err := services.FetchAllClusterRoles(cf.Context, cluster, profile.Roles, profile.Traits)
if err != nil {
log.Debugf("Failed to fetch user roles: %v.", err)
}
Expand Down Expand Up @@ -812,32 +810,6 @@ func isMFADatabaseAccessRequired(cf *CLIConf, tc *client.TeleportClient, databas
return mfaResp.GetRequired(), nil
}

// fetchRoleSet fetches a user's roles for a specified cluster.
func fetchRoleSet(ctx context.Context, cluster auth.ClientI, profile *client.ProfileStatus) (services.RoleSet, error) {
codingllama marked this conversation as resolved.
Show resolved Hide resolved
// get roles and traits. default to the set from profile, try to get up-to-date version from server point of view.
roles := profile.Roles
traits := profile.Traits

// GetCurrentUser() may not be implemented, fail gracefully.
user, err := cluster.GetCurrentUser(ctx)
if err == nil {
roles = user.GetRoles()
traits = user.GetTraits()
} else {
log.Debugf("Failed to fetch current user information: %v.", err)
}

// get the role definition for all roles of user.
// this may only fail if the role which we are looking for does not exist, or we don't have access to it.
// example scenario when this may happen:
// 1. we have set of roles [foo bar] from profile.
// 2. the cluster is remote and maps the [foo, bar] roles to single role [guest]
// 3. the remote cluster doesn't implement GetCurrentUser(), so we have no way to learn of [guest].
// 4. services.FetchRoles([foo bar], ..., ...) fails as [foo bar] does not exist on remote cluster.
roleSet, err := services.FetchRoles(roles, cluster, traits)
return roleSet, trace.Wrap(err)
}

// pickActiveDatabase returns the database the current profile is logged into.
//
// If logged into multiple databases, returns an error unless one specified
Expand Down