Make `tsh db ls` lists available db users. #10458

Tener · 2022-02-18T15:46:20Z

Examples:

> tsh db ls
Name  Description   Allowed Users Labels  Connect
----- ------------- ------------- ------- -------
redis Redis example (none)        env=dev

> tsh db ls
Name  Description   Allowed Users Labels  Connect
----- ------------- ------------- ------- -------
redis Redis example [*]           env=dev

> tsh db ls
Name  Description   Allowed Users      Labels  Connect
----- ------------- ------------------ ------- -------
redis Redis example [*], except: [bob] env=dev

> tsh db ls
Name  Description   Allowed Users          Labels  Connect
----- ------------- ---------------------- ------- -------
redis Redis example [* baz], except: [bob] env=dev

> tsh db ls
Name  Description   Allowed Users Labels  Connect
----- ------------- ------------- ------- -------
redis Redis example [baz]         env=dev

> tsh db ls --verbose
Name  Description   Protocol Type        URI                     Allowed Users Labels                                  Connect Expires
----- ------------- -------- ----------- ----------------------- ------------- --------------------------------------- ------- -------------------
redis Redis example redis    self-hosted rediss://localhost:6666 [* baz]       env=dev,teleport.dev/origin=config-file         Jan  1 00:00:00 UTC

Tener · 2022-02-18T15:47:20Z

I'll make full PR after adding tests.

ravicious

Left some minor questions, but LGTM in general!

tool/tsh/db.go

tool/tsh/db_test.go

r0mant · 2022-02-24T01:58:31Z

tool/tsh/db.go

+	// calculating possible users for db is only done in verbose mode.
+	displayUsersForDb := func(database types.Database) string {
+		return ""
+	}


Can we just make it a default behavior? Add another column to the table with allowed users.

Sure, it makes the code cleaner still. Although given the requested moves it will need to be reworked anyway.

r0mant · 2022-02-24T02:01:17Z

tool/tsh/db.go

 	return nil
 }

+// getUsersForDb finds allowed and denied users given user's effective roleSet and database in consideration. The result is a user-readable string.
+func getUsersForDb(roleSet services.RoleSet, database types.Database) string {


Instead of implementing this in the CLI, this needs to be a part of the API. This way UI will also be able to reuse it for example. I would either make this a part of the GetDatabases API, or introduce a separate API endpoint.

This makes it more complex to implement, but I can see the benefits. I'll look into this after v9 is out.

@Tener I've been thinking, it probably doesn't even need to be an actual API - but we should at least move this function somewhere it can be reused e.g. to lib/services package. This way web API will also be able to use it since it has RoleSet and Database. What do you think?

@r0mant sounds reasonable; I generally agree this should move towards the application's core - either as a callable service or as part of the core library. I'll see which option feels better once I get back to this.

tool/tsh/db.go

r0mant · 2022-02-24T02:11:13Z

tool/tsh/db.go

+	usersOk := make([]string, 0)
+	usersBad := make([]string, 0)


Can we come up with names that are more descriptive than "ok" and "bad"?

tool/tsh/tsh.go

r0mant · 2022-02-24T02:16:30Z

tool/tsh/db.go

+	allUsers := make([]string, 0)
+	allUsers = append(allUsers, allowedUsers...)
+	allUsers = append(allUsers, deniedUsers...)


Suggested change

allUsers := make([]string, 0)

allUsers = append(allUsers, allowedUsers...)

allUsers = append(allUsers, deniedUsers...)

allUsers := append(allowedUsers, deniedUsers...)

Also, it feels like the above function can just return all users right away since allowed/denied aren't used independently here.

smallinsky · 2022-02-24T09:25:23Z

tool/tsh/db.go

+		})
+
+		if err != nil {
+			log.Debugf("Error while calculating available db users: %v", err)


Not sure if the flow should processed when a user is unable to connect to the proxy

Potentially this can also be caused by a lack of permissions. But I'm pretty sure this part will change by moving the calculation logic to the server and exposing it via API.

smallinsky · 2022-02-24T09:58:20Z

tool/tsh/db.go

+		err = client.RetryWithRelogin(cf.Context, tc, func() error {
+			proxy, err := tc.ConnectToProxy(cf.Context)
+			if err != nil {
+				return trace.Wrap(err)
+			}


nit: move to a separate function also the logic can be splitted fetching roles, making a displayUsersForDb function.

Tener · 2022-04-06T14:03:17Z

Per prior requests, the core enumeration logic is now in role.go and tsh db ls is merely a client of that logic. Should be good for reuse in teleterm.

Tener · 2022-04-07T11:37:29Z

@smallinsky added examples, as discussed offline the other day.

ravicious · 2022-04-07T14:09:02Z

tool/tsh/tsh_test.go

+		{
+			name:     "developer allowed any username in stage database except superuser",
+			roles:    services.RoleSet{roleDevStage, roleDevProd},
+			database: dbStage,
+			result:   "[* dev], except: [superuser]",


In this scenario, shouldn't the result be just [*]?

The dev username comes from roleDevProd, it seems like it shouldn't have any bearing on users for dbStage.

This is a subtle point and I'm happy that we have this test. The original context for this feature is important: we have a permissive role (all users allowed: *) and the users want a hint on what users they can use. We go looking for possible usernames in other roles and also allow additional usernames to be provided.

Since we discovered that dev is a possible login, we include it here.

ravicious · 2022-04-07T14:12:46Z

lib/services/role.go

+// In cases where * is listed in set of allowed users, it may be hard for users to figure out the expected username.
+// For this reason the parameter extraUsers provides an extra set of users to be checked against RoleSet.
+// This extra set of users may be sourced e.g. from user connection history.


Was this added with Teleterm in mind? I hope we actually get to use this. 🙊 But I'll think about how to utilize this once I actually get around to incorporating that in Teleterm.

Yep, Teleterm was the planned user for this parameter. I guessed that it has a mechanism for storing recently used resources etc.

Alternatively, this may be a feature of the database itself: on successful auth, store the username which can be queried as a property of the database. This has the benefit of sharing the available usernames across all people who can access that particular database in Teleport.

The second mechanism is perhaps more robust and can be shared with tsh.

Tener · 2022-04-11T09:13:50Z

PTAL @r0mant @smallinsky

ravicious · 2022-04-11T09:55:00Z

By the way, could you backport it to v9? Teleterm is planned to release in 9.2 and it'll need these changes.

smallinsky

Left some minor comments otherwise LGTM.

smallinsky · 2022-04-11T09:32:18Z

lib/services/role.go

@@ -862,6 +862,110 @@ func NewRoleSet(roles ...types.Role) RoleSet {
 // RoleSet is a set of roles that implements access control functionality
 type RoleSet []types.Role

+// EnumerationResult is a result of enumerating a role set against some property, e.g. allowed names or logins.
+type EnumerationResult struct {


This EnumerationResult is quite generic name for the role package. For example roles.EnumerationResult suggest that roles are enumerated.

Any suggestions? RoleSetEnumerationResult is bit too long for my tastes.

lib/services/role.go

smallinsky · 2022-04-11T09:55:23Z

lib/services/role.go

@@ -862,6 +862,110 @@ func NewRoleSet(roles ...types.Role) RoleSet {
 // RoleSet is a set of roles that implements access control functionality
 type RoleSet []types.Role

+// EnumerationResult is a result of enumerating a role set against some property, e.g. allowed names or logins.
+type EnumerationResult struct {
+	allowedDeniedMap map[string]bool


nit: splitting in two separate list instead of aggregating a user into single map should simplify the implemeantion:

Suggested change

allowedDeniedMap map[string]bool

allowedUsers []string

deniedUsers []string

For example the (result *EnumerationResult) filtered can be dropped.

We need a map to deal with duplicates, so either we do it in one go or we need to deduplicate afterward, which makes the code more complex, not less.

The login duplication are already removed in the users = apiutils.Deduplicate(append(users,extraUsers) call

The login duplication are already removed in the users = apiutils.Deduplicate(append(users,extraUsers) call

This is done outside of EnumerationResult code.

I can switch to two slices, but that makes it possible to have an internal representation which is inconsistent. The easiest way to efficiently remove duplicates is with map.

OK. This was just a suggestion. Let's leave the current form.

smallinsky · 2022-04-11T09:57:06Z

lib/services/role.go

+		wildcardAllowed := false
+		wildcardDenied := false
+


It can be moved outside the for loop.

On the contrary: it must be inside the loop, since these two bools refer to particular role.

Tener · 2022-04-11T11:15:30Z

By the way, could you backport it to v9? Teleterm is planned to release in 9.2 and it'll need these changes.

Yep, I'll backport to v9 and v8.

smallinsky · 2022-04-11T11:47:54Z

@klizhentas, @r0mant
Could you take a look at UI tsh db ls changes described in PR description .

Co-authored-by: Marek Smoliński <[email protected]>

r0mant

@Tener @smallinsky I like the UX

* Show available db users in "tsh db ls". Co-authored-by: Marek Smoliński <[email protected]>

ravicious · 2022-07-08T14:54:00Z

tool/tsh/db.go

+	proxy, err := tc.ConnectToProxy(cf.Context)
+	if err != nil {
+		return trace.Wrap(err)
+	}


I'm fixing a bug in Connect where we prematurely close the proxy client when fetching db users. I was looking at how tsh does things (since it was working fine in tsh db ls) and I landed here and noticed that we don't seem to close the proxy client here at all.

Shouldn't we have defer proxy.Close() at the end here just as we have defer cluster.Close() below?

The fix in Connect I was working on: #14230.

We probably should. There are, in fact, a few other places in codebase that are missing the deferred .Close() call on the result of ConnectToProxy().

Tener requested a review from r0mant February 18, 2022 15:46

Tener marked this pull request as ready for review February 21, 2022 14:11

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

github-actions bot requested a review from ravicious February 21, 2022 14:12

ravicious approved these changes Feb 22, 2022

View reviewed changes

tool/tsh/db.go Outdated Show resolved Hide resolved

tool/tsh/db.go Outdated Show resolved Hide resolved

tool/tsh/db_test.go Outdated Show resolved Hide resolved

r0mant reviewed Feb 24, 2022

View reviewed changes

r0mant requested a review from smallinsky February 24, 2022 02:17

smallinsky reviewed Feb 24, 2022

View reviewed changes

Show available db users in "tsh db ls".

8db0401

Tener force-pushed the tener/db-ls-users-pr branch from 9e7cf58 to 8db0401 Compare April 6, 2022 13:58

Merge branch 'master' into tener/db-ls-users-pr

7157a59

Tener requested review from r0mant, ravicious and smallinsky April 6, 2022 13:59

Tener changed the title ~~In verbose mode "tsh db ls" lists available users.~~ Make tsh db ls lists available db users. Apr 6, 2022

ravicious approved these changes Apr 7, 2022

View reviewed changes

Merge branch 'master' into tener/db-ls-users-pr

48b1285

ravicious added the teleterm-backport label Apr 11, 2022

smallinsky approved these changes Apr 11, 2022

View reviewed changes

Merge branch 'master' into tener/db-ls-users-pr

84c0cb3

Tener and others added 2 commits April 11, 2022 14:06

Test allow/deny pair for same username.

1ef8b92

Return nil.

bd8d1ac

Co-authored-by: Marek Smoliński <[email protected]>

r0mant approved these changes Apr 13, 2022

View reviewed changes

Tener added 2 commits April 14, 2022 10:22

Merge branch 'master' into tener/db-ls-users-pr

8dbbb3e

Merge branch 'master' into tener/db-ls-users-pr

aca788d

Tener enabled auto-merge (squash) April 14, 2022 08:32

Tener merged commit 6bd7bf9 into master Apr 14, 2022

Tener deleted the tener/db-ls-users-pr branch April 14, 2022 09:45

Tener added a commit that referenced this pull request Apr 14, 2022

Make tsh db ls lists available db users. (#10458)

52939d8

* Show available db users in "tsh db ls". Co-authored-by: Marek Smoliński <[email protected]>

Tener added a commit that referenced this pull request Apr 14, 2022

Make tsh db ls lists available db users. (#10458)

5cba936

* Show available db users in "tsh db ls". Co-authored-by: Marek Smoliński <[email protected]>

This was referenced Apr 14, 2022

[v8] Make tsh db ls lists available db users. (#10458) #11941

Merged

[v9] Make tsh db ls lists available db users. (#10458) #11942

Merged

Tener added a commit that referenced this pull request Apr 19, 2022

Make tsh db ls lists available db users. (#10458) (#11941)

122c6df

* Show available db users in "tsh db ls". Co-authored-by: Marek Smoliński <[email protected]>

ravicious mentioned this pull request Apr 19, 2022

Expose RoleSet.EnumerateDatabaseUsers to Teleport Terminal #12070

Merged

Tener added a commit that referenced this pull request Apr 20, 2022

Make tsh db ls lists available db users. (#10458) (#11942)

a84ce58

* Show available db users in "tsh db ls". Co-authored-by: Marek Smoliński <[email protected]>

ravicious removed the teleconnect-backport label Apr 25, 2022

This was referenced Apr 29, 2022

Fix tsh db ls for remote clusters. #12281

Merged

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

Merged

webvictim mentioned this pull request Jun 8, 2022

Opened in error #13285

Closed

smallinsky mentioned this pull request Jun 8, 2022

DB access denied error improvement #13293

Closed

Tener mentioned this pull request Jun 9, 2022

Add sshLogins to nodes endpoint on webapi (GET /nodes) #13266

Merged

ravicious reviewed Jul 8, 2022

View reviewed changes

alexatcanva mentioned this pull request Oct 13, 2022

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

Open

	allowedDeniedMap map[string]bool
	allowedUsers []string
	deniedUsers []string

Make tsh db ls lists available db users. #10458

Make tsh db ls lists available db users. #10458

Conversation

Tener commented Feb 18, 2022 • edited Loading

Tener commented Feb 18, 2022

ravicious left a 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

r0mant Mar 4, 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

Choose a reason for hiding this comment

Tener commented Apr 6, 2022

Tener commented Apr 7, 2022

Choose a reason for hiding this comment

Tener Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tener Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Tener commented Apr 11, 2022

ravicious commented Apr 11, 2022

smallinsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tener Apr 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tener commented Apr 11, 2022

smallinsky commented Apr 11, 2022

r0mant left a 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

Make `tsh db ls` lists available db users. #10458

Make `tsh db ls` lists available db users. #10458

Tener commented Feb 18, 2022 •

edited

Loading

r0mant Mar 4, 2022 •

edited

Loading

Tener Apr 7, 2022 •

edited

Loading

Tener Apr 7, 2022 •

edited

Loading

Tener Apr 11, 2022 •

edited

Loading