-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fix tsh db ls
for remote clusters.
#12281
Changes from 12 commits
2cade8a
12b7b72
ecf7595
5dea81f
b1d1371
3acc026
cbe6523
ce015f5
2e28f96
1aa8d5c
984397f
3bca6f2
5ca003f
c2d8756
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,7 +73,27 @@ func onListDatabases(cf *CLIConf) error { | |
return trace.Wrap(err) | ||
} | ||
|
||
roleSet, err := services.FetchRoles(profile.Roles, cluster, profile.Traits) | ||
// 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(cf.Context) | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving the conversation about Teleport Connect so that it doesn't pollute the PR comments.
What I meant in my original comment is that once we get ahold of the auth client in We'd call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, ok, I misunderstood. But yeah, I think that replicating this logic would work just fine. |
||
if err != nil { | ||
log.Debugf("Failed to fetch user roles: %v.", err) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as the user has the role, they are allowed to see its definition (from here):
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofGetCurrentUser
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
teleport/lib/auth/permissions.go
Line 292 in 2292be1
where GetUser already allows to get user's own identity on remote cluster:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that was the first thing I tried.
GetUser
andGetUsers
work against real users, provisioned in one way or another.So
a.authServer.Identity.GetUser(name, withSecrets)
goes to backend, which is different froma.context.User
ora.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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reedloden @russjones PTAL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking, I'll make the change.