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

sql,server: fix the listing of sessions for admin/root users #32629

Merged
merged 1 commit into from Nov 29, 2018

Conversation

Projects
None yet
3 participants
@knz
Copy link
Member

knz commented Nov 27, 2018

Fixes #32599.

Admin users should be able to list all sessions not just their own.
This was broken in #32253 and thus in 2.1.1 (was ok in 2.1.0 and
prior). This patch fixes this and also clarifies the code responsible.

Release note (bug fix): CockroachDB now again enables admin users,
including root, to list all user sessions besides their own.

@knz knz requested a review from a-robinson Nov 27, 2018

@knz knz added this to Triage in SQL Front-end, Lang & Semantics via automation Nov 27, 2018

@knz knz requested review from cockroachdb/core-prs as code owners Nov 27, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Nov 27, 2018

This change is Reviewable

@knz

This comment has been minimized.

Copy link
Member

knz commented Nov 27, 2018

This still needs a test. Will add test as soon as @a-robinson confirms the approach does not break non-SQL scenarios.

@knz knz force-pushed the knz:20181127-sessions branch from 08faf49 to 984f3f8 Nov 27, 2018

@knz

This comment has been minimized.

Copy link
Member

knz commented Nov 27, 2018

actually I had made a mistake, which the (newly added) test helped me resolve. ready for review.

@knz knz moved this from Triage to Current milestone in SQL Front-end, Lang & Semantics Nov 27, 2018

@a-robinson
Copy link
Collaborator

a-robinson left a comment

Thanks for fixing this, @knz. But why doesn't SQL just send the request with the req.Username field empty if all sessions are desired?

@knz

This comment has been minimized.

Copy link
Member

knz commented Nov 27, 2018

Thanks for fixing this, @knz. But why doesn't SQL just send the request with the req.Username field empty if all sessions are desired?

The crdb_internal vtable sends the server.ListSessions() request using a root gRPC request, even if the SQL session itself is owned by non-root.

So from the perspective of the admin RPC, all incoming requests from SQL are issued by root, even those from non-root users.

We need a way to distinguish them. That's what this patch is doing.

The alternative is to make the crdb_internal issue the admin RPC using a non-root RPC session, but I don't know how to do that / whether it's possible. Any suggestion?

@a-robinson

This comment has been minimized.

Copy link
Collaborator

a-robinson commented Nov 27, 2018

What I was thinking was that if the user is root (and thus "all sessions are desired" based on the contract we've created with users), then the Username field wouldn't be populated where it is now. In other words, we'd change the couple of call points in crdb_internal.go from

req := serverpb.ListSessionsRequest{Username: p.SessionData().User}

to something more like:

req := serverpb.ListSessionsRequest{Username: p.SessionData().User} 
if req.Username == security.RootUser {
  req.Username = ""
}

to codify that SQL wants all sessions back in such cases.

I'm ok with what you've written, though, if there's some reason we wouldn't want to change the SQL code. One reason, I suppose would be cross-version compatibility; changing the SQL code wouldn't fix cross-version situations where a 2.0 SQL gateway sends a request to a >=2.1.1 status server.

@knz

This comment has been minimized.

Copy link
Member

knz commented Nov 27, 2018

What I was thinking was that if the user is root (and thus "all sessions are desired" based on the contract we've created with users), then the Username field wouldn't be populated where it is now.

oh that's clever. I hadn't thought about that!

Let me try this and get back to you.

@knz

This comment has been minimized.

Copy link
Member

knz commented Nov 28, 2018

Done, RFAL.

@knz knz force-pushed the knz:20181127-sessions branch from 984f3f8 to 77bdc74 Nov 28, 2018

@knz knz requested a review from cockroachdb/sql-wiring-prs as a code owner Nov 28, 2018

@a-robinson
Copy link
Collaborator

a-robinson left a comment

:lgtm: thanks again! Can you make sure to backport this?

Reviewed 4 of 4 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained


pkg/server/status.go, line 1379 at r1 (raw file):

	// There are two ways to query "all sessions":
	// - an empty username in the request (subject to the checks above).
	// - the request is internal (coming from crdb_internal.{node,cluster}_sessions)

Isn't this comment incorrect now?


pkg/sql/show_test.go, line 784 at r1 (raw file):

			count++
		}
		if count == 0 {

I don't think it's a big deal, but this check could succeed even if root doesn't see its own session -- it might have only seen the non-root session.

You don't need to change it, but I'd probably have written this as a loop that accumulates all the usernames it sees into a map[string]int called users and then checks that users["root"] > 0 and users["nonroot"] > 0, printing out the contents of the map to aid in debugging if either check fails.

sql,server: fix the listing of sessions for admin/root users
Admin users should be able to list all sessions not just their own.
This was broken in #32253 and thus in 2.1.1 (was ok in 2.1.0 and
prior). This patch fixes this and also clarifies the code responsible.

Release note (bug fix): CockroachDB now again enables admin users,
including `root`, to list all user sessions besides their own.

@knz knz force-pushed the knz:20181127-sessions branch from 77bdc74 to 2760413 Nov 29, 2018

@knz

This comment has been minimized.

Copy link
Member

knz commented Nov 29, 2018

bors r+

@knz
Copy link
Member

knz left a comment

TFYR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/server/status.go, line 1379 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Isn't this comment incorrect now?

Done.


pkg/sql/show_test.go, line 784 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

I don't think it's a big deal, but this check could succeed even if root doesn't see its own session -- it might have only seen the non-root session.

You don't need to change it, but I'd probably have written this as a loop that accumulates all the usernames it sees into a map[string]int called users and then checks that users["root"] > 0 and users["nonroot"] > 0, printing out the contents of the map to aid in debugging if either check fails.

Done.

craig bot pushed a commit that referenced this pull request Nov 29, 2018

Merge #32629
32629: sql,server: fix the listing of sessions for admin/root users r=knz a=knz

Fixes #32599.

Admin users should be able to list all sessions not just their own.
This was broken in #32253 and thus in 2.1.1 (was ok in 2.1.0 and
prior). This patch fixes this and also clarifies the code responsible.

Release note (bug fix): CockroachDB now again enables admin users,
including `root`, to list all user sessions besides their own.

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
@craig

This comment has been minimized.

Copy link

craig bot commented Nov 29, 2018

Build succeeded

@craig craig bot merged commit 2760413 into cockroachdb:master Nov 29, 2018

2 of 3 checks passed

GitHub CI (Cockroach) TeamCity build started
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@knz knz moved this from Current milestone to Finished (m2.2-2) in SQL Front-end, Lang & Semantics Dec 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment