Fix `tsh db ls` for remote clusters. #12281

Tener · 2022-04-27T18:47:10Z

The tsh db ls command needs to process the definition of roles for the current user along with their traits. For local clusters, this information is available in their profile. When connecting to remote clusters, however, the user is mapped to a distinct set of roles and traits. This is done in authorizeRemoteUser:

teleport/lib/auth/permissions.go

Lines 214 to 215 in 2292be1

    
           // authorizeRemoteUser returns checker based on cert authority roles 
        
           func (a *authorizer) authorizeRemoteUser(ctx context.Context, u RemoteUser) (*Context, error) {

Using the roles and traits from the user profile is wrong in the context of the remote cluster; the roles may not even exist or they may not be assigned to the user post-translation.

The fix has several layers. First, we introduce .GetCurrentUser() method. This informs the user of how the server sees them after authentication, making the effective set of roles and traits explicit. Using this information tsh can request correct roles and perform the calculation it needs.

For compatibility reasons, we fall back to roles and traits from the profile if .GetCurrentUser() is not available. This has a risk of inaccurate information being presented which may lead to .FetchRoles() call failing (e.g. user asks about roles they have no access to or which don't exist). In this case, we fall back to displaying "(unknown)" as the list of available users.

Fixes #12239

Caused by #10458

GetCurrentUser returns current user as seen by the server. Useful especially in the context of remote clusters which perform role and trait mapping.

Attempt to resolve remote roles and traits. Fail gracefully if this isn't possible.

r0mant · 2022-04-27T23:51:57Z

lib/auth/auth_with_roles.go

+	if ok {
+		return usr, nil
+	}
+	return nil, trace.Errorf("internal error: unexpected type")


Suggested change

return nil, trace.Errorf("internal error: unexpected type")

return nil, trace.BadParameter("expected types.User when fetching current user information, got %T", xxx)

r0mant · 2022-04-27T23:54:50Z

tool/tsh/db.go

+		roles = user.GetRoles()
+		traits = user.GetTraits()
+	} else {
+		log.Debugf("cluster.GetCurrentUser failed: %v", err)


Suggested change

log.Debugf("cluster.GetCurrentUser failed: %v", err)

log.Debugf("Failed to fetch current user information: %v.", err)

r0mant · 2022-04-27T23:55:14Z

tool/tsh/db.go

 	if err != nil {
-		return trace.Wrap(err)
+		log.Debugf("services.FetchRoles failed: %v", err)


Suggested change

log.Debugf("services.FetchRoles failed: %v", err)

log.Debugf("Failed to fetch user roles: %v.", err)

r0mant · 2022-04-28T00:04:41Z

lib/auth/auth_with_roles.go

@@ -1742,6 +1742,16 @@ func (a *ServerWithRoles) GetUser(name string, withSecrets bool) (types.User, er
 	return a.authServer.Identity.GetUser(name, withSecrets)
 }

+// GetCurrentUser returns current user as seen by the server.
+// Useful especially in the context of remote clusters which perform role and trait mapping.
+func (a *ServerWithRoles) GetCurrentUser(ctx context.Context) (types.User, error) {


Without any additional RBAC checks here, I'm a little worried about privilege escalation since we're potentially letting the root's users expand the scope of what they can learn about the leaf cluster. For example, role names - currently they are not available anywhere in the root so users can only see them if they have proper permissions.

I think we should require at least permissions to either read the user's roles or list roles prior to returning this. If the leaf doesn't allow root users view the roles, they're not also gonna be able to see "allowed database users" - which makes sense.

Any other fields are a part of User object we may want to protect?

As long as the user has the role, they are allowed to see its definition (from here):

// GetRole returns role by name func (a *ServerWithRoles) GetRole(ctx context.Context, name string) (types.Role, error) { // Current-user exception: we always allow users to read roles // that they hold. This requirement is checked first to avoid // misleading denial messages in the logs. if !apiutils.SliceContainsStr(a.context.User.GetRoles(), name) { if err := a.action(apidefaults.Namespace, types.KindRole, types.VerbRead); err != nil { return nil, trace.Wrap(err) } } return a.authServer.GetRole(ctx, name) }

We don't treat the names of roles as secret. The user would also only get the roles they got from mapping in trusted_cluster resource, which are explicitly given and likely very few.

If we were to do any RBAC checks here, I'm not sure what would they be?

The alternative approach would be to move the db user list logic from tsh flow to a backend endpoint. But not sure if we should do it.

Since roles name and trails are public available for a user logged into root cluster not sure what is the reason to hide leaf cluster roles and traits from a user. Thought I think that it would be good idea for now to trim userV2 content and return only to trails and roles name in case of GetCurrentUser preventing the case where userV2 will be extended with some sensitive data.

@Tener
Have you checked if the GetUser can be leveraged here instead of introducing a new GetCurrentUser endpoint. At first glace it looks that authorizeRemoteUser set the correct remote user identity:

teleport/lib/auth/permissions.go

Line 269 in 2292be1

user.SetRoles(roleNames)

teleport/lib/auth/permissions.go

Line 292 in 2292be1

Groups: user.GetRoles(),

where GetUser already allows to get user's own identity on remote cluster:

func (a *ServerWithRoles) GetUser(name string, withSecrets bool) (types.User, error) { ... // if secrets are not being accessed, let users always read // their own info. if err := a.currentUserAction(name); err != nil { // not current user, perform normal permission check. if err := a.action(apidefaults.Namespace, types.KindUser, types.VerbRead); err != nil { return nil, trace.Wrap(err) } } } return a.authServer.Identity.GetUser(name, withSecrets)

Have you checked if the GetUser can be leveraged here instead of introducing a new GetCurrentUser endpoint.

Yeah, that was the first thing I tried. GetUser and GetUsers work against real users, provisioned in one way or another.

So a.authServer.Identity.GetUser(name, withSecrets) goes to backend, which is different from a.context.User or a.context.Identity.

Also, the remote users will never satisfy "look at self" check, because we assume no collisions happen:

teleport/lib/auth/permissions.go

Lines 260 to 262 in 2292be1

// The user is prefixed with "remote-" and suffixed with cluster name with

// the hope that it does not match a real local user.

user, err := types.NewUser(fmt.Sprintf("remote-%v-%v", u.Username, u.ClusterName))

@reedloden @russjones Any objections to exposing the user's own leaf cluster identity in this way, provided we add RBAC checks for the role names? It's needed to fetch leaf's role names so tsh can show "allowed database users" for leaf cluster's databases (needed by Teleport Connect also).

@reedloden @russjones Any objections to exposing the user's own leaf cluster identity in this way, provided we add RBAC checks for the role names? It's needed to fetch leaf's role names so tsh can show "allowed database users" for leaf cluster's databases (needed by Teleport Connect also).

@reedloden @russjones can you take a look as per @r0mant request? It would be good to fix this issue as currently the feature is degraded for remote clusters.

@reedloden @russjones PTAL?

@Tener I talked with @russjones offline about this and there's no objections to exposing user's remote identities to themselves as long as we require read permissions on roles (basically, what I suggested originally). Can you apply the changes? Then we can merge.

@Tener I talked with @russjones offline about this and there's no objections to exposing user's remote identities to themselves as long as we require read permissions on roles (basically, what I suggested originally). Can you apply the changes? Then we can merge.

Thanks for checking, I'll make the change.

Tener · 2022-04-28T10:33:41Z

@ravicious FYI, pretty sure Teleport Connect will need a similar fix.

ravicious · 2022-04-28T11:18:56Z

It looks like for Teleport Connect it's a matter of modifying GetAllowedDatabaseUsers.

teleport/lib/teleterm/clusters/cluster_databases.go

Lines 128 to 143 in d9deaff

    
           // GetAllowedDatabaseUsers returns allowed users for the given database based on the role set. 
        
           func (c *Cluster) GetAllowedDatabaseUsers(ctx context.Context, dbURI string) ([]string, error) { 
        
           	roleSet, err := services.FetchRoles(c.status.Roles, c.clusterClient, 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) 
        
           	} 
        
           	dbUsers := roleSet.EnumerateDatabaseUsers(db) 
        
           	return dbUsers.Allowed(), nil 
        
           }

We need to get ahold of auth client, in a similar manner as we already do here:

teleport/lib/teleterm/clusters/cluster_kubes.go

Lines 36 to 48 in d9deaff

    
           // GetKubes returns kube services 
        
           func (c *Cluster) GetKubes(ctx context.Context) ([]Kube, error) { 
        
           	proxyClient, err := c.clusterClient.ConnectToProxy(ctx) 
        
           	if err != nil { 
        
           		return nil, trace.Wrap(err) 
        
           	} 
        
           	defer proxyClient.Close() 
        
           	authClient, err := proxyClient.ConnectToCluster(ctx, c.clusterClient.SiteName, true) 
        
           	if err != nil { 
        
           		return nil, trace.Wrap(err) 
        
           	} 
        
           	defer authClient.Close()

If roleSet is nil, we can just return an error, it'll be displayed as a small temporary notification to the user as fetching suggested db names is not a critical piece of information.

Do you mind adding this yourself or would you rather have me implement this? I could definitely test the change with Connect if you've never built it yourself.

Tener · 2022-04-28T11:30:36Z

If roleSet is nil, we can just return an error, it'll be displayed as a small temporary notification to the user as fetching suggested db names is not a critical piece of information.

How do you ensure that for remote clusters c.status.Roles is populated properly?

xacrimon

The patch itself in terms of quality seems okay but this feels like a bit of an odd solution to problems we've solved differently in other places. For non-db stuff like sessions, we move the listing logic to a backend endpoint. This exposes minimal information and is the common pattern used as far as I can tell.

Here, that seems like it would involve hitting the current cluster as specified by the tsh profile/--cluster flag and asking it to do the listing instead.

If we decide to stick with the current approach presented here, may I suggest renaming the GetCurrentUser endpoint to something else like GetMappedUser?

Tener · 2022-04-28T12:46:15Z

The patch itself in terms of quality seems okay but this feels like a bit of an odd solution to problems we've solved differently in other places. For non-db stuff like sessions, we move the listing logic to a backend endpoint. This exposes minimal information and is the common pattern used as far as I can tell.

Here, that seems like it would involve hitting the current cluster as specified by the tsh profile/--cluster flag and asking it to do the listing instead.

For maximum compatibility, I wanted to avoid adding new endpoints where it wasn't needed.

If we decide to stick with the current approach presented here, may I suggest renaming the GetCurrentUser endpoint to something else like GetMappedUser?

Mapped in GetMappedUser suggests that mapping takes place, which is an implementation detail concerning remote users, while the endpoint is available to all. To me, this isn't a better name as it requires context to understand what Mapped is referencing here.

There were some other names I considered though, e.g. GetEffectiveUser or GetAuthenticatedUser, but they all had issues. Naming is hard :( Any particular things you don't like about GetCurrentUser?

@xacrimon

ravicious · 2022-04-28T13:02:37Z

tool/tsh/db.go

+	// 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)


Moving the conversation about Teleport Connect so that it doesn't pollute the PR comments.

How do you ensure that for remote clusters c.status.Roles is populated properly?

What I meant in my original comment is that once we get ahold of the auth client in GetAllowedDatabaseUsers before calling FetchRoles, we will be able to use the same logic as tsh uses here, won't we?

We'd call authClient.GetCurrentUser and if that fails we'll just use data from c.status.

Ah, ok, I misunderstood. But yeah, I think that replicating this logic would work just fine.

Tener · 2022-04-29T06:36:43Z

@r0mant @ravicious @xacrimon @smallinsky kind reminder.

smallinsky · 2022-04-29T08:33:21Z

@Tener
I don't have additional comments for this PR. Current approach seems to be reasonable to add support for DB user listing in case of remote clusters . Though exposing remote cluster identity is quite controversial. Can we make a quick regression fix for tsh db ls flow and just role result on PermissionDenied and them we can decide if we want expose the remote user identity ?

Tener · 2022-04-29T09:07:34Z

@Tener I don't have additional comments for this PR. Current approach seems to be reasonable to add support for DB user listing in case of remote clusters . Though exposing remote cluster identity is quite controversial. Can we make a quick regression fix for tsh db ls flow and just role result on PermissionDenied and them we can decide if we want expose the remote user identity ?

So, basically this PR without .GetCurrentUser()? I guess I can do that, even if that is incomplete: we do need to fix the remote cluster access one way or another.

It feels wrong that we are supposed to hide the permissions the user has from that very same user. I have never seen that sentiment anywhere in the codebase and I find it very likely that at least part of that information may already be available in some contexts.

@smallinsky

smallinsky · 2022-04-29T09:52:23Z

@Tener

So, basically this PR without .GetCurrentUser()? I guess I can do that, even if that is incomplete: we do need to fix the remote cluster access one way or another.

Yes, By this fix we can quickly fix the regression even if it is incomplete.

Tener · 2022-04-29T10:04:57Z

@Tener

So, basically this PR without .GetCurrentUser()? I guess I can do that, even if that is incomplete: we do need to fix the remote cluster access one way or another.

Yes, By this fix we can quickly fix the regression even if it is incomplete.

@smallinsky hotfix: #12318

r0mant · 2022-04-29T22:40:21Z

@smallinsky @Tener

exposing remote cluster identity is quite controversial

I think it's ok, the only "controversial" part IMO is what I commented on before - that is, potentially exposing more information to the user than what they currently have access to according to their role mapping. I understand role names don't technically reveal much but currently the user does not have any means of even knowing what role names the leaf has unless they have "list" permissions on them. Which is why I suggested adding that RBAC check. We can always relax it later if it turns out to be cumbersome and limit usefulness of this feature.

Tener · 2022-05-20T07:35:32Z

@r0mant I've added the access check in commit 984397f.

The check is fairly simple, and follows the pattern seen elsewhere of reusing .GetRole() call for this purpose:

teleport/lib/auth/auth_with_roles.go

Lines 1930 to 1937 in 984397f

    
           func (a *ServerWithRoles) GetCurrentUser(ctx context.Context) (types.User, error) { 
        
           	// check access to roles 
        
           	for _, role := range a.context.User.GetRoles() { 
        
           		_, err := a.GetRole(ctx, role) 
        
           		if err != nil { 
        
           			return nil, trace.Wrap(err) 
        
           		} 
        
           	}

I think it is noteworthy that with the current code there is no way for the check to actually fail. This is because .GetRole() gives permissions for roles currently seen in the user context, and we are only checking the roles in the user context.

teleport/lib/auth/auth_with_roles.go

Lines 2930 to 2939 in 984397f

    
           // GetRole returns role by name 
        
           func (a *ServerWithRoles) GetRole(ctx context.Context, name string) (types.Role, error) { 
        
           	// Current-user exception: we always allow users to read roles 
        
           	// that they hold.  This requirement is checked first to avoid 
        
           	// misleading denial messages in the logs. 
        
           	if !apiutils.SliceContainsStr(a.context.User.GetRoles(), name) { 
        
           		if err := a.action(apidefaults.Namespace, types.KindRole, types.VerbRead); err != nil { 
        
           			return nil, trace.Wrap(err) 
        
           		} 
        
           	}

It can be argued, however, that this is a reasonable defensive coding. The particulars of the permission checks in .GetRole() may change in the future. Having this check here ensures that we verify the access, not merely assume it.

github-actions · 2022-05-23T19:43:13Z

@Tener See the table below for backport results.

Branch	Result
branch/v8	Failed
branch/v9	Failed

* New method: GetCurrentUser(). GetCurrentUser returns current user as seen by the server. Useful especially in the context of remote clusters which perform role and trait mapping. * Fix "tsh db ls" for remote clusters. Attempt to resolve remote roles and traits. Fail gracefully if this isn't possible.

#12281 fixed fetching db users for remote clusters for `tsh db ls`. #13617 applied the same fix to `tsh db ls --all` and extracted the `fetchRoleSet` function. This commit extracts it to `lib/client` so that we can reuse it in `lib/teleterm`. Other than accepting `log` as an argument, nothing was changed about that function.

Tener added 2 commits April 27, 2022 20:41

New method: GetCurrentUser().

2cade8a

GetCurrentUser returns current user as seen by the server. Useful especially in the context of remote clusters which perform role and trait mapping.

Fix "tsh db ls" for remote clusters.

12b7b72

Attempt to resolve remote roles and traits. Fail gracefully if this isn't possible.

Tener requested review from r0mant, jakule and smallinsky April 27, 2022 18:47

Merge branch 'master' into tener/remote-cluster-roles

ecf7595

r0mant reviewed Apr 28, 2022

View reviewed changes

Better error messages.

5dea81f

Tener added 2 commits April 28, 2022 13:41

Test GetCurrentUser().

b1d1371

Merge branch 'master' into tener/remote-cluster-roles

3acc026

Tener marked this pull request as ready for review April 28, 2022 11:41

github-actions bot added the tsh tsh - Teleport's command line tool for logging into nodes running Teleport. label Apr 28, 2022

github-actions bot requested a review from xacrimon April 28, 2022 11:42

xacrimon reviewed Apr 28, 2022

View reviewed changes

Tener requested a review from r0mant April 28, 2022 12:52

ravicious reviewed Apr 28, 2022

View reviewed changes

Merge branch 'master' into tener/remote-cluster-roles

cbe6523

Tener mentioned this pull request Apr 29, 2022

Gracefully degrade tsh db ls in case fetching roles fails. #12318

Merged

Tener changed the title ~~Fix "tsh db ls" for remote clusters.~~ Fix tsh db ls for remote clusters. Apr 29, 2022

Tener added the backport/branch/v9 label May 4, 2022

Tener self-assigned this May 5, 2022

Tener added 3 commits May 6, 2022 15:17

Merge branch 'master' into tener/remote-cluster-roles

2e28f96

Merge branch 'master' into tener/remote-cluster-roles

1aa8d5c

Check access to roles.

984397f

Tener requested review from xacrimon and ravicious May 20, 2022 12:55

Merge branch 'master' into tener/remote-cluster-roles

3bca6f2

r0mant approved these changes May 20, 2022

View reviewed changes

smallinsky approved these changes May 23, 2022

View reviewed changes

xacrimon approved these changes May 23, 2022

View reviewed changes

Merge branch 'master' into tener/remote-cluster-roles

5ca003f

Tener enabled auto-merge (squash) May 23, 2022 16:51

Merge branch 'master' into tener/remote-cluster-roles

c2d8756

Tener merged commit 202c431 into master May 23, 2022

Tener mentioned this pull request May 24, 2022

[v9] Fix tsh db ls for remote clusters. (#12281) #12853

Merged

Tener mentioned this pull request May 24, 2022

[v8] Fix tsh db ls for remote clusters. (#12281) #12854

Merged

Tener deleted the tener/remote-cluster-roles branch May 24, 2022 09:42

Tener mentioned this pull request May 24, 2022

Make the TestGetCurrentUser more resilient. #12857

Merged

ravicious mentioned this pull request May 27, 2022

[Search-based access requests] Enforce resource access restrictions #12651

Merged

webvictim mentioned this pull request Jun 8, 2022

Opened in error #13285

Closed

ravicious mentioned this pull request Jun 23, 2022

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

Merged

alexatcanva mentioned this pull request Oct 13, 2022

BUGFIX | Fix Teleport ALPN Proxy not being HTTP CONNECT Proxy Aware alexatcanva/teleport#30

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `tsh db ls` for remote clusters. #12281

Fix `tsh db ls` for remote clusters. #12281

Tener commented Apr 27, 2022 •

edited

Loading

r0mant Apr 27, 2022

r0mant Apr 27, 2022

r0mant Apr 27, 2022

r0mant Apr 28, 2022

Tener Apr 28, 2022

smallinsky Apr 28, 2022 •

edited

Loading

Tener Apr 28, 2022 •

edited

Loading

r0mant May 2, 2022

Tener May 5, 2022

Tener May 11, 2022

r0mant May 19, 2022

Tener May 20, 2022

Tener commented Apr 28, 2022

ravicious commented Apr 28, 2022

Tener commented Apr 28, 2022

xacrimon left a comment

Tener commented Apr 28, 2022 •

edited

Loading

ravicious Apr 28, 2022 •

edited

Loading

Tener Apr 28, 2022

Tener commented Apr 29, 2022

smallinsky commented Apr 29, 2022

Tener commented Apr 29, 2022 •

edited

Loading

smallinsky commented Apr 29, 2022

Tener commented Apr 29, 2022

r0mant commented Apr 29, 2022

Tener commented May 20, 2022

github-actions bot commented May 23, 2022

	// authorizeRemoteUser returns checker based on cert authority roles
	func (a authorizer) authorizeRemoteUser(ctx context.Context, u RemoteUser) (Context, error) {

	return nil, trace.Errorf("internal error: unexpected type")
	return nil, trace.BadParameter("expected types.User when fetching current user information, got %T", xxx)

	log.Debugf("cluster.GetCurrentUser failed: %v", err)
	log.Debugf("Failed to fetch current user information: %v.", err)

	log.Debugf("services.FetchRoles failed: %v", err)
	log.Debugf("Failed to fetch user roles: %v.", err)

	// The user is prefixed with "remote-" and suffixed with cluster name with
	// the hope that it does not match a real local user.
	user, err := types.NewUser(fmt.Sprintf("remote-%v-%v", u.Username, u.ClusterName))

Fix tsh db ls for remote clusters. #12281

Fix tsh db ls for remote clusters. #12281

Conversation

Tener commented Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smallinsky Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Tener Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tener commented Apr 28, 2022

ravicious commented Apr 28, 2022

Tener commented Apr 28, 2022

xacrimon left a comment

Choose a reason for hiding this comment

Tener commented Apr 28, 2022 • edited Loading

ravicious Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tener commented Apr 29, 2022

smallinsky commented Apr 29, 2022

Tener commented Apr 29, 2022 • edited Loading

smallinsky commented Apr 29, 2022

Tener commented Apr 29, 2022

r0mant commented Apr 29, 2022

Tener commented May 20, 2022

github-actions bot commented May 23, 2022

Fix `tsh db ls` for remote clusters. #12281

Fix `tsh db ls` for remote clusters. #12281

Tener commented Apr 27, 2022 •

edited

Loading

smallinsky Apr 28, 2022 •

edited

Loading

Tener Apr 28, 2022 •

edited

Loading

Tener commented Apr 28, 2022 •

edited

Loading

ravicious Apr 28, 2022 •

edited

Loading

Tener commented Apr 29, 2022 •

edited

Loading