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

observability: ui regression introduced by fixing #42567 #44033

Closed
mattcrdb opened this issue Jan 15, 2020 · 7 comments · Fixed by #44167
Closed

observability: ui regression introduced by fixing #42567 #44033

mattcrdb opened this issue Jan 15, 2020 · 7 comments · Fixed by #44167

Comments

@mattcrdb
Copy link

@mattcrdb mattcrdb commented Jan 15, 2020

BACKGROUND
the recent security fix in #42567 that was backported to 19.2.2 created a discrepancy between the behavior of show tables from the sql shell and viewing the databases/tables page on the web-ui.

the release note mentions the following: Certain web UI pages (like the list of databases or tables) now restrict their content to match the privileges of the logged-in user.

DESCRIPTION
Even when granted the necessary select privileges on the respective database and table, the UI still returns this database has no tables. even though the database and tables are accessible from the sql shell.

the trick here is that when you run SHOW TABLES, the query that is executed is select * from <currentdb>.information_schema.tables which is always possible if the user has the correct permissions on the db.

but the UI endpoint executes select * from system.xxx where the logged-in user, if not admin, does not have permission to access directly.

Currently, on a non-enterprise enabled cluster only the root user has the admin role. which means that this page will never be accessible as the root user can not log in to the UI.

this fallout is also documented here cockroachdb/docs#6321

A related issue is #43870

WORKAROUND (FOR USERS)

Use an admin user to log in to the UI and view the information.

Core users without a license can create a non-root admin users as follows:

  1. obtain temporary eval license
  2. create user and grant admin role
  3. let license expire; the admin user will continue to be valid

A patch revision will be issued for the affected release to correct this limitation.

SOLUTION (INTERNAL)

rework the ui query to match the sql query issued from show tables


big thanks to @knz for explaining this to me and detailing the proposals in the above PR and issue.

note from knz: it was not designed to be used by non-admin users to start with, and it kinda worked "by accident" until we patched the security problem

@solongordon

This comment has been minimized.

Copy link
Contributor

@solongordon solongordon commented Jan 15, 2020

Per @knz, the fix here is either to to change the SQL query on the admin UI side or to tweak the relevant virtual table iterator.

@otan

This comment has been minimized.

Copy link
Collaborator

@otan otan commented Jan 16, 2020

i cannot find where the UI does select * from "".info_schema.tables where db = <currentdb> -- any pointers?

@knz

This comment has been minimized.

Copy link
Member

@knz knz commented Jan 16, 2020

It's not exactly as I had imagined

  1. see
func (s *adminServer) TableDetails(...)
func (s *adminServer) DatabaseDetails(...)
func (s *adminServer) Databases(...)

These do use the regular SHOW commands but using the credentials of the logged-in user. I think (Cursory look at internal.go) their "current db" as per internalExecFixedUserSession is "system" but I am not sure.

  1. The question is why these queries are returning no results.

  2. I recommend you first write the unit test: start a test server, create a auth session manually in system.web_sessions (see my code from #43872 if needed), then use the cookie to access the TableDetails API. At this point the test should fail

  3. use your test to explore the problem with ample logging and find the solution

@knz

This comment has been minimized.

Copy link
Member

@knz knz commented Jan 16, 2020

I see that TestServer already has (ts *TestServer) GetAuthenticatedHTTPClient() so you can use that. Make sure to use isAdmin = false.

@otan

This comment has been minimized.

Copy link
Collaborator

@otan otan commented Jan 16, 2020

so the problem in database details is

path, err := s.queryDescriptorIDPath(ctx, userName, []string{req.Database})

which eventually calls

cockroach/pkg/server/admin.go

Lines 2205 to 2214 in e7f83bf

func (s *adminServer) queryNamespaceID(
ctx context.Context, userName string, parentID sqlbase.ID, name string,
) (sqlbase.ID, error) {
const query = `SELECT id FROM system.namespace WHERE "parentID" = $1 AND name = $2`
rows, _ /* cols */, err := s.server.internalExecutor.QueryWithUser(
ctx, "admin-query-namespace-ID", nil /* txn */, userName, query, parentID, name,
)
if err != nil {
return 0, err
}

to which the user does not have access to system.namespace. should we be querying this as admin? or is there an alternative way of querying this data?

fwiw, the error in this case is user authentic_user_noadmin does not have SELECT privilege on relation namespace

@otan

This comment has been minimized.

Copy link
Collaborator

@otan otan commented Jan 16, 2020

Alternatively, we could avoid displaying this data altogether.

Although I imagine most if not all queries on system.X would have issues... there's another one to system.zones in this instance, e.g.

id, zone, zoneExists, err := s.queryZonePath(ctx, userName, path)

@knz

This comment has been minimized.

Copy link
Member

@knz knz commented Jan 16, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
SQL Features
  
Done
4 participants
You can’t perform that action at this time.