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

server: Don't let non-superusers see other user's sessions #32253

Merged
merged 1 commit into from Nov 13, 2018

Conversation

Projects
None yet
4 participants
@a-robinson
Copy link
Collaborator

a-robinson commented Nov 13, 2018

This effectively means that only the root user can see other user's
sessions except when the enterprise-only RBAC feature is enabled and a
user has been added to the "admin" role.

This also changes the behavior of ListSessions and ListLocalSessions to
list whatever sessions they can even if no username is provided, whereas
previously if no username was provided then no sessions would ever be
returned.

Release note (bug fix): Don't let non-superusers see other user's
sessions and queries via the ListSessions and ListLocalSessions status
server API methods.


This of course needs tests if we want to keep this approach. I've only manually tested it for now. I'd be fine just putting in a simpler version for v2.1.1 that skips the RBAC admin role check if we want to avoid the extra sql planner machinery.

@a-robinson a-robinson requested review from bdarnell and couchand Nov 13, 2018

@a-robinson a-robinson requested a review from cockroachdb/core-prs as a code owner Nov 13, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Nov 13, 2018

This change is Reviewable

@couchand
Copy link
Collaborator

couchand left a comment

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


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

	for _, session := range sessions {
		if !(superUser || sessionUser != session.Username) {

The logic of this seems really weird to me. The superuser gets every session, regardless of the parameter? I suppose with this change, the superuser will only get their own sessions if that's the parameter value, but with any other parameter value they will get all sessions. Still seems weird.


And I know I said I wouldn't be opinionated about the general strategy, but I'm wondering if we need to bother making it fail loud. Would it be sufficient to just change this to:

if req.Username != sessionUser && !s.isSuperUser(ctx, sessionUser) {
    continue
}

And then everyone is just limited to the results that they're allowed to see.


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

	usernames, ok := md[webSessionUserKeyStr]
	if !ok || len(usernames) == 0 {
		return ""

While I'm happy with the logic of the comment about these empty strings, I'm a bit nervous that the implementation here isn't obviously correct, since it's designed to fail open above.

@couchand
Copy link
Collaborator

couchand left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


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

Previously, couchand (Andrew Couch) wrote…

While I'm happy with the logic of the comment about these empty strings, I'm a bit nervous that the implementation here isn't obviously correct, since it's designed to fail open above.

To clarify: I really don't trust that if metadata.FromIncomingContext returns !ok that it must mean the user legitimately has super user access. It's too easy for the context not to be correctly propagated, for instance.

@bdarnell
Copy link
Member

bdarnell left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/server/authentication.go, line 363 at r1 (raw file):

type webSessionIDKey struct{}

const webSessionUserKeyStr = "websessionuser"

What are the implications of this case change? It seems either unsafe or unnecessary to me.


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

Previously, couchand (Andrew Couch) wrote…

The logic of this seems really weird to me. The superuser gets every session, regardless of the parameter? I suppose with this change, the superuser will only get their own sessions if that's the parameter value, but with any other parameter value they will get all sessions. Still seems weird.


And I know I said I wouldn't be opinionated about the general strategy, but I'm wondering if we need to bother making it fail loud. Would it be sufficient to just change this to:

if req.Username != sessionUser && !s.isSuperUser(ctx, sessionUser) {
    continue
}

And then everyone is just limited to the results that they're allowed to see.

Yeah, if a user is given in the request we should only show that user. Superusers should only see everything if no user is provided.


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

Previously, couchand (Andrew Couch) wrote…

To clarify: I really don't trust that if metadata.FromIncomingContext returns !ok that it must mean the user legitimately has super user access. It's too easy for the context not to be correctly propagated, for instance.

Yeah, it's better to explicitly detect the grpc/login-disabled cases and report the user as root/node instead of empty string in that case (and leave empty string for "no user" and disallow it). But unless that's compatible with what we're doing it will probably need to be in 2.2.

In any case, the meaning of the empty string should be commented on this method, not just where it is used.

@a-robinson a-robinson force-pushed the a-robinson:listsessions-user branch from 409372b to a32d558 Nov 13, 2018

@a-robinson
Copy link
Collaborator

a-robinson left a comment

TFTRs, updated. Sounds like there's still a little contention around the reliance on the presence of the context metadata fields, but I'll add a test or two in the meantime while we sort that out.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/server/authentication.go, line 363 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

What are the implications of this case change? It seems either unsafe or unnecessary to me.

Nothing given that MD.Set() calls strings.ToLower on the key provided to it before storing it in its map. There are no other parts of the code that touch these, and as soon as they're used they get converted to all lower case anyway.


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

Previously, bdarnell (Ben Darnell) wrote…

Yeah, if a user is given in the request we should only show that user. Superusers should only see everything if no user is provided.

That code wouldn't be enough to make an empty req.Username work for returning a logged-in users own sessions, but I've cleaned things up a bit. Thanks.


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

Previously, bdarnell (Ben Darnell) wrote…

Yeah, it's better to explicitly detect the grpc/login-disabled cases and report the user as root/node instead of empty string in that case (and leave empty string for "no user" and disallow it). But unless that's compatible with what we're doing it will probably need to be in 2.2.

In any case, the meaning of the empty string should be commented on this method, not just where it is used.

Done @bdarnell's comment.

As for @couchand's comment, we need a way of trusting requests that come in over grpc, because this endpoint is actually used by the SQL SHOW SESSIONS statement under the covers, and we need to trust such requests that don't have a username attached. A little digging shows that such requests do have a non-empty metadata.FromIncomingContext with the following fields:

map[:authority:[alex-laptop.local:26257] content-type:[application/grpc] user-agent:[grpc-go/1.13.0-dev]]

None of these are unique to requests originally sent over grpc, since the content-type and user-agent also get set when requests come in via HTTP / the grpc gateway. That means that unfortunately the lack of a specified username is just about the best thing we have here, unless we want to add an additional piece of metadata in the sql code before making the requests and then check for it here.

Also, if we don't trust the metadata.FromIncomingContext matching whether a request came in over grpc vs http, then the whole server.remote_debugging.mode is fundamentally broken and you'll have to find something else.

I do suppose, though, that as a compromise we could making this endpoint

@couchand
Copy link
Collaborator

couchand left a comment

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


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

Previously, a-robinson (Alex Robinson) wrote…

Done @bdarnell's comment.

As for @couchand's comment, we need a way of trusting requests that come in over grpc, because this endpoint is actually used by the SQL SHOW SESSIONS statement under the covers, and we need to trust such requests that don't have a username attached. A little digging shows that such requests do have a non-empty metadata.FromIncomingContext with the following fields:

map[:authority:[alex-laptop.local:26257] content-type:[application/grpc] user-agent:[grpc-go/1.13.0-dev]]

None of these are unique to requests originally sent over grpc, since the content-type and user-agent also get set when requests come in via HTTP / the grpc gateway. That means that unfortunately the lack of a specified username is just about the best thing we have here, unless we want to add an additional piece of metadata in the sql code before making the requests and then check for it here.

Also, if we don't trust the metadata.FromIncomingContext matching whether a request came in over grpc vs http, then the whole server.remote_debugging.mode is fundamentally broken and you'll have to find something else.

I do suppose, though, that as a compromise we could making this endpoint

That seems reasonable, I took a peek over at GatewayRemoteAllowed again and it's exactly the same, as you said.

I'm curious (and this is probably outside the scope of this PR), it seems like only one of these three conditionals should apply to every gRPC call, and the other two we can safely switch to fail closed? Without looking into the details, maybe it's the !ok of the md[webSessionUserKeyStr]? I think the !ok of the FromIncomingContext call indicates a programming error, and len(usernames) != 1 does as well. Maybe I'm overthinking it.

reviewable!

@bdarnell
Copy link
Member

bdarnell left a comment

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


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

Previously, couchand (Andrew Couch) wrote…

That seems reasonable, I took a peek over at GatewayRemoteAllowed again and it's exactly the same, as you said.

I'm curious (and this is probably outside the scope of this PR), it seems like only one of these three conditionals should apply to every gRPC call, and the other two we can safely switch to fail closed? Without looking into the details, maybe it's the !ok of the md[webSessionUserKeyStr]? I think the !ok of the FromIncomingContext call indicates a programming error, and len(usernames) != 1 does as well. Maybe I'm overthinking it.

Add some comments here about why you're inferring root from the absence of data.

@a-robinson a-robinson force-pushed the a-robinson:listsessions-user branch from a32d558 to dd83594 Nov 13, 2018

@a-robinson
Copy link
Collaborator

a-robinson left a comment

Comments addressed and test added.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


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

I think the !ok of the FromIncomingContext call indicates a programming error, and len(usernames) != 1 does as well. Maybe I'm overthinking it.

I think that's true as well, although I'm not 100% confident about it. However, it's probably better to fail closed (and loudly, with an error) here than to fail open, so I've changed to doing so.

Add some comments here about why you're inferring root from the absence of data.

Done.

@bdarnell
Copy link
Member

bdarnell left a comment

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

server: Don't let non-superusers see other user's sessions
This effectively means that only the root user can see other user's
sessions except when the enterprise-only RBAC feature is enabled and a
user has been added to the "admin" role.

This also changes the behavior of ListSessions and ListLocalSessions to
list whatever sessions they can even if no username is provided, whereas
previously if no username was provided then no sessions would ever be
returned.

Release note (bug fix): Don't let non-superusers see other user's
sessions and queries via the ListSessions and ListLocalSessions status
server API methods.

@a-robinson a-robinson force-pushed the a-robinson:listsessions-user branch from dd83594 to 115a84e Nov 13, 2018

@a-robinson
Copy link
Collaborator

a-robinson left a comment

I was apparently a little too aggressive about failing closed -- metadata.FromIncomingContext can fail on contexts that come from SQL in a bunch of the sql package's unit tests. I've fixed that up.

I'll merge on green.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@a-robinson

This comment has been minimized.

Copy link
Collaborator

a-robinson commented Nov 13, 2018

bors r+

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

Merge #32253
32253: server: Don't let non-superusers see other user's sessions r=a-robinson a=a-robinson

This effectively means that only the root user can see other user's
sessions except when the enterprise-only RBAC feature is enabled and a
user has been added to the "admin" role.

This also changes the behavior of ListSessions and ListLocalSessions to
list whatever sessions they can even if no username is provided, whereas
previously if no username was provided then no sessions would ever be
returned.

Release note (bug fix): Don't let non-superusers see other user's
sessions and queries via the ListSessions and ListLocalSessions status
server API methods.

---

This of course needs tests if we want to keep this approach. I've only manually tested it for now. I'd be fine just putting in a simpler version for v2.1.1 that skips the RBAC admin role check if we want to avoid the extra sql planner machinery.

Co-authored-by: Alex Robinson <alexdwanerobinson@gmail.com>
@couchand
Copy link
Collaborator

couchand left a comment

:lgtm_strong:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@craig

This comment has been minimized.

Copy link

craig bot commented Nov 13, 2018

Build succeeded

@craig craig bot merged commit 115a84e into cockroachdb:master Nov 13, 2018

3 checks passed

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

knz added a commit to knz/cockroach that referenced this pull request Nov 27, 2018

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 cockroachdb#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 added a commit to knz/cockroach that referenced this pull request Nov 27, 2018

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 cockroachdb#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 added a commit to knz/cockroach that referenced this pull request Nov 28, 2018

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 cockroachdb#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 added a commit to knz/cockroach that referenced this pull request Nov 29, 2018

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 cockroachdb#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.

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>

knz added a commit to knz/cockroach that referenced this pull request Nov 29, 2018

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 cockroachdb#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment