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

VIEWACTIVITYREDACTED should redact query text literals for SHOW SESSIONS #106588

Open
gtr opened this issue Jul 11, 2023 · 1 comment
Open

VIEWACTIVITYREDACTED should redact query text literals for SHOW SESSIONS #106588

gtr opened this issue Jul 11, 2023 · 1 comment
Labels
A-cluster-observability Related to cluster observability C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-observability

Comments

@gtr
Copy link
Contributor

gtr commented Jul 11, 2023

This issue tracks adding redaction for the SHOW SESSIONS command and the ListSessions endpoint.

In accordance with #103560, users with the VIEWACTIVITYREDACTED privilege should see a redacted version of the active_queries field from the SHOW SESSIONS response.

The current behavior allows a user with VIEWACTIVITYREDACTED to see the full query:

root@localhost:26257/defaultdb> GRANT SYSTEM VIEWACTIVITYREDACTED TO  gerardo;
GRANT

Time: 157ms total (execution 156ms / network 0ms)

root@localhost:26257/defaultdb> SELECT pg_sleep(1000);

Run SHOW SESSIONS from gerardo:

gerardo@localhost:26257/defaultdb> SELECT session_id, active_queries, user_name from [SHOW SESSIONS];
             session_id            |                              active_queries                               | user_name
-----------------------------------+---------------------------------------------------------------------------+------------
  1770d8d0e573a2f00000000000000001 | SELECT pg_sleep(1000)                                                     | root
  1770d8d2f8f465100000000000000001 | SELECT session_id, active_queries, user_name FROM [SHOW CLUSTER SESSIONS] | gerardo
(2 rows)

Time: 8ms total (execution 7ms / network 0ms)

SELECT pg_sleep(1000) should have its literals redacted, e.g. SELECT pg_sleep(_).

Jira issue: CRDB-29633

@gtr gtr added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-cluster-observability A-cluster-observability Related to cluster observability labels Jul 11, 2023
@gtr gtr self-assigned this Jul 11, 2023
@gtr gtr added this to Triage in Cluster Observability via automation Jul 11, 2023
@gtr gtr moved this from Triage to Bugs in Cluster Observability Jul 11, 2023
gtr added a commit to gtr/cockroach that referenced this issue Jul 12, 2023
Fixes cockroachdb#104354.
Partially addresses cockroachdb#106588.

Previously, when a non-admin user was given the `VIEWACTIVITY`
privilege, they were able to see other users' sessions from the SQL
shell but not from the UI.

This commit fixes the ListSessions endpoint to check for the
`VIEWACTIVITY` privilege in addition to the `VIEWACTIVITY` role when
returning a response for the ListSessions endpoint.

Release note (bug fix): users with the `VIEWACTIVITY` privilege should
be able to see other users' sessions from both the CLI and the DB
Console.
@gtr
Copy link
Contributor Author

gtr commented Jul 12, 2023

Update on this issue:

I put the fix for the VIEWACTIVITY system privilege bug in #106590. This should close its linked issue. However, I think there still needs to be some changes in the ListSessions endpoint, particularly from the UI perspective. Currently, the UI does not issue a ListSessionsRequest when refreshing session data in the SQL Activity pages. This means that the request coming into the server's ListSessions endpoint will always have an empty username field.

This is the current behavior:

  • For a non-admin user, a ListSessionsRequest with an empty username field is taken to mean that the user is requesting to view their own sessions:
    // For non-superusers, requests with an empty username is
    // implicitly a request for the client's own sessions.
    if reqUsername.Undefined() {
    reqUsername = sessionUser
    }

    This was the crux of the previous VIEWACTIVITY system privilege bug linked above.
  • For an admin user, a ListSessionsRequest with an empty username field means that the user is requesting to view all sessions:
    // The empty username means "all sessions".
    showAll := reqUsername.Undefined()

Furthermore, for any SHOW SESSIONS request made from the SQL shell, the server misinterprets that the user making the request is root, even if we are logged into the SQL shell from a non-admin user. See: #45018. Effectively, this does not allow a user logged into the SQL shell to access its correct permissions since the ListSessions endpoint will always be interpreted as root. This is the root cause of this issue.

I'm leaning to move away from using this empty username field as an indicator to "show all sessions" and instead relying on the permissions that the given username has to determine which sessions to show. This will also require the UI to attach a username to any calls to ListSessions. It should default to root if running in insecure mode.

gtr added a commit to gtr/cockroach that referenced this issue Jul 23, 2023
Fixes cockroachdb#104354.
Partially addresses cockroachdb#106588.

Previously, when a non-admin user was given the `VIEWACTIVITY`
privilege, they were able to see other users' sessions from the SQL
shell but not from the UI.

This commit fixes the ListSessions endpoint to check for the
`VIEWACTIVITY` privilege in addition to the `VIEWACTIVITY` role when
returning a response for the ListSessions endpoint.

Release note (bug fix): users with the `VIEWACTIVITY` privilege should
be able to see other users' sessions from both the CLI and the DB
Console.
gtr added a commit to gtr/cockroach that referenced this issue Jul 25, 2023
Fixes cockroachdb#104354.
Partially addresses cockroachdb#106588.

Previously, when a non-admin user was given the `VIEWACTIVITY`
privilege, they were able to see other users' sessions from the SQL
shell but not from the UI.

This commit fixes the ListSessions endpoint to check for the
`VIEWACTIVITY` privilege in addition to the `VIEWACTIVITY` role when
returning a response for the ListSessions endpoint.

Release note (bug fix): users with the `VIEWACTIVITY` privilege should
be able to see other users' sessions from both the CLI and the DB
Console.
gtr added a commit to gtr/cockroach that referenced this issue Aug 3, 2023
Fixes cockroachdb#104354.
Partially addresses cockroachdb#106588.

Previously, when a non-admin user was given the `VIEWACTIVITY`
privilege, they were able to see other users' sessions from the SQL
shell but not from the UI.

This commit fixes the ListSessions endpoint to check for the
`VIEWACTIVITY` privilege in addition to the `VIEWACTIVITY` role when
returning a response for the ListSessions endpoint.

Release note (bug fix): users with the `VIEWACTIVITY` privilege should
be able to see other users' sessions from both the CLI and the DB
Console.
gtr added a commit to gtr/cockroach that referenced this issue Aug 7, 2023
Fixes cockroachdb#104354.
Partially addresses cockroachdb#106588.

Previously, when a non-admin user was given the `VIEWACTIVITY`
privilege, they were able to see other users' sessions from the SQL
shell but not from the UI.

This commit fixes the ListSessions endpoint to check for the
`VIEWACTIVITY` privilege in addition to the `VIEWACTIVITY` role when
returning a response for the ListSessions endpoint.

Release note (bug fix): users with the `VIEWACTIVITY` privilege should
be able to see other users' sessions from both the CLI and the DB
Console.
gtr added a commit to gtr/cockroach that referenced this issue Aug 9, 2023
Fixes cockroachdb#104354.
Partially addresses cockroachdb#106588.

Previously, when a non-admin user was given the `VIEWACTIVITY`
privilege, they were able to see other users' sessions from the SQL
shell but not from the UI.

This commit fixes the ListSessions endpoint to check for the
`VIEWACTIVITY` privilege in addition to the `VIEWACTIVITY` role when
returning a response for the ListSessions endpoint.

Release note (bug fix): users with the `VIEWACTIVITY` privilege should
be able to see other users' sessions from both the CLI and the DB
Console.
gtr added a commit to gtr/cockroach that referenced this issue Aug 9, 2023
Fixes cockroachdb#104354.
Partially addresses cockroachdb#106588.

Previously, when a non-admin user was given the `VIEWACTIVITY`
privilege, they were able to see other users' sessions from the SQL
shell but not from the UI.

This commit fixes the ListSessions endpoint to check for the
`VIEWACTIVITY` privilege in addition to the `VIEWACTIVITY` role when
returning a response for the ListSessions endpoint.

Release note (bug fix): users with the `VIEWACTIVITY` privilege should
be able to see other users' sessions from both the CLI and the DB
Console.
gtr added a commit to gtr/cockroach that referenced this issue Aug 9, 2023
Fixes cockroachdb#104354.
Partially addresses cockroachdb#106588.

Previously, when a non-admin user was given the `VIEWACTIVITY`
privilege, they were able to see other users' sessions from the SQL
shell but not from the UI.

This commit fixes the ListSessions endpoint to check for the
`VIEWACTIVITY` privilege in addition to the `VIEWACTIVITY` role when
returning a response for the ListSessions endpoint.

Release note (bug fix): users with the `VIEWACTIVITY` privilege should
be able to see other users' sessions from both the CLI and the DB
Console.
craig bot pushed a commit that referenced this issue Aug 9, 2023
106590: sql: fix VIEWACTIVITY privilege for ListSessions r=gtr a=gtr

Fixes #104354.
Partially addresses #106588.

Previously, when a non-admin user was given the `VIEWACTIVITY`
privilege, they were able to see other users' sessions from the SQL
shell but not from the UI.

This commit fixes the ListSessions endpoint to check for the
`VIEWACTIVITY` privilege in addition to the `VIEWACTIVITY` role when
returning a response for the ListSessions endpoint.

loom: https://www.loom.com/share/8224628f7e7e4af298306c83f158d593

Release note (bug fix): users with the `VIEWACTIVITY` privilege should
be able to see other users' sessions from both the CLI and the DB
Console.

Co-authored-by: gtr <gerardo@cockroachlabs.com>
gtr added a commit to gtr/cockroach that referenced this issue Aug 11, 2023
Fixes cockroachdb#104354.
Partially addresses cockroachdb#106588.

Previously, when a non-admin user was given the `VIEWACTIVITY`
privilege, they were able to see other users' sessions from the SQL
shell but not from the UI.

This commit fixes the ListSessions endpoint to check for the
`VIEWACTIVITY` privilege in addition to the `VIEWACTIVITY` role when
returning a response for the ListSessions endpoint.

Release note (bug fix): users with the `VIEWACTIVITY` privilege should
be able to see other users' sessions from both the CLI and the DB
Console.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cluster-observability Related to cluster observability C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-observability
Projects
No open projects
Development

No branches or pull requests

1 participant