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

sql: add system privilege for viewing system tables #95756

Closed
maryliag opened this issue Jan 24, 2023 · 9 comments
Closed

sql: add system privilege for viewing system tables #95756

maryliag opened this issue Jan 24, 2023 · 9 comments
Assignees
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@maryliag
Copy link
Contributor

maryliag commented Jan 24, 2023

Currently, non-admin don't have permission to system tables.
We do have use cases where we want to use system tables on the console (as read-only) and we want non-admin users to be able to see it, but we don't want them to have access to all system tables.

This feature request is allow user to grant/revoke access to specific system tables, without the need to be admin.

Jira issue: CRDB-23715

Epic CC-25593

@maryliag maryliag added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-schema-deprecated Use T-sql-foundations instead labels Jan 24, 2023
maryliag added a commit to maryliag/cockroach that referenced this issue Jan 26, 2023
The get the list of fingerprints used by an index,
we use the `system.statement_statistics` directly, but
non-admins don't have access to system table.
Until cockroachdb#95756 or cockroachdb#95770 are done, we need to hide
this feature for non-admins.

Part Of cockroachdb#93087

Release note (ui change): Hide list of used fingerprint per index
on Index Details page, for non-admin users.
craig bot pushed a commit that referenced this issue Jan 26, 2023
94153: sql: Remove type annotations from `column_default` in `information_schema.columns` r=ZhouXing19 a=e-mbrown

Informs: #87774

The type annotation on `column_default` caused confusion in postgres compatible apps. This change formats the `column_default` without the type annotation. It does not address the incorrect type annotation.

Release note (bug fix): The content of `column_default` in `information_schema.columns` no longer have type annotations.

95855: backupccl: run restore/tpce/32tb/aws/nodes=15/cpus=16 once a week r=benbardin a=msbutler

This patch adds the restore/tpce/32TB/aws/nodes=15/cpus=16 test to our weekly
test suite. The backup fixture was generated by initing a tpce cluster with 2
Million customers, and running this workload while 48 incremental backups were
taken at 15 minute increments. The roachtest restores from a system time
captured in the 24th backup.

The cluster used to restore the backup will run on aws with 15 nodes each with
16 vcpus.

To verify this test exists, run:
`roachtest list tag:weekly`

Release note: None

Epic: none

95861: kv: remove lastToReplica and lastFromReplica from raftMu r=nvanbenschoten a=nvanbenschoten

Extracted from #94165 without modification.

----

In 410ef29, we moved these fields from under the Replica.mu to under the Replica.raftMu. This was done to avoid lock contention.

In this commit, we move these fields under their own mutex so that they can be accessed without holding the raftMu. This allows us to send Raft messages from other goroutines.

The commit also switches from calling RawNode.ReportUnreachable directly in sendRaftMessage to using the more flexible unreachablesMu set, which defers the call to ReportUnreachable until the next Raft tick. As a result, the commit closes #84246.

Release note: None
Epic: None

95876: sql: remove round-trips from common DISCARD ALL r=ajwerner a=ajwerner

#### sql: avoid checking if a role exists when setting to the current role

This is an uncached round-trip. It makes `DISCARD` expensive.
In #86485 we implemented
`SET SESSION AUTHORIZATION DEFAULT`, this added an extra sql query to
`DISCARD ALL` to check if the role we'd become exists. This is almost
certainly unintentional in the case where the role we'd become is the
current session role. I think this just happened because of code
consolidation.

We now no longer check if the current session role exists. This removes
the last round-trip from DISCARD ALL.

#### sql: use in-memory session data to decide what to do in DISCARD

In #86246 we introduced logic to discard schemas when running DISCARD ALL and DISCARD TEMP. This logic did expensive kv operations unconditionally; if the session knew it had never created a temporary schema, we'd still fetch all databases and proceed to search all databases for a temp schema. This is very expensive.

Fixes: #95864

Release note (performance improvement): In 22.2, logic was added to make
`SET SESSION AUTHORIZATION DEFAULT` not a no-op. This implementation used
more general code for setting the role for a session which made sure that
the role exists. The check for whether a role exists is currently uncached.
We don't need to check if the role we already are exists. This improves the
performance of `DISCARD ALL` in addition to `SET SESSION AUTHORIZATION
DEFAULT`.


Release note (performance improvement): In 22.2, we introduced support for `DISCARD TEMP` and made `DISCARD ALL` actually discard temp tables. This implementation ran expensive logic to discover temp schemas rather than consulting in-memory data structures. As a result, `DISCARD ALL`, which is issued regularly by connection pools, became an expensive operation when it should be cheap. This problem is now resolved.

95997: ui: hide list of statement for non-admins r=maryliag a=maryliag

To get the list of fingerprints used by an index, we use the `system.statement_statistics` directly, but non-admins don't have access to system table.
Until #95756 or #95770 are done, we need to hide
this feature for non-admins.

Part Of #93087

Release note (ui change): Hide list of used fingerprint per index on Index Details page, for non-admin users.

95998: roachtest: update activerecord blocklist r=ZhouXing19 a=andyyang890

This patch removes a few tests from the blocklist that do not
exist in the test suite for rails 7.0.3, which were being run
prior to the fix for version pinning.

Informs #94211

Release note: None

96003: kv: add MVCC local timestamp details to ReadWithinUncertaintyIntervalError r=nvanbenschoten a=nvanbenschoten

This PR adds details about MVCC local timestamps to ReadWithinUncertaintyIntervalErrors. This provides more information about the cause of uncertainty errors.

The PR also includes a series of minor refactors to clean up this code.

Release note: None
Epic: None

Co-authored-by: e-mbrown <[email protected]>
Co-authored-by: Michael Butler <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: maryliag <[email protected]>
Co-authored-by: Andy Yang <[email protected]>
@ajwerner
Copy link
Contributor

I don't know how reasonable this is. We ought to leverage other mechanisms like views, virtual views or the synthetic privilege infrastructure to achieve these goals. I believe we now do have a mechanism to grant and revoke privileges on virtual tables/views.

This issue as written seems subject to the XY problem.

@maryliag
Copy link
Contributor Author

maryliag commented Mar 7, 2023

What is the advantage of creating a new view on top of a system table if the view is just a select on that system table?
What is the concern about giving read permission on system tables?

As mentioned on the description, we want to make selections on system tables for non-admins, and the need to create views that do just a select on a system table just for that case seems unnecessary.

The problem: we need to query system tables for non-admins.
My suggestion: allow users to have read permission to some system tables (have to manage just the permission)
Your suggestion: creating a new layer (in this case a view), so that the permission can be done on that. (have to manage a new view and the permission on that view)

Would you mind explaining why the view is a better solution?

@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
@knz
Copy link
Contributor

knz commented Jul 31, 2023

The reason why we don't want configurable privileges on system tables is a mix of factors:

  • we want to assert the current state of descriptors (by comparing them to a known reference) during upgrades
  • so we want system table descriptors to be immutable except during upgrades
  • privs are currently stored inside descriptors
  • therefore we cannot change privs on system tables (outside of upgrade migrations)

If we had a mechanism to store privileges outside of descriptors (like comments) we'd be in a better place. But it would be some work. I believe we already have an issue for that.

@rafiss
Copy link
Collaborator

rafiss commented Aug 1, 2023

If we had a mechanism to store privileges outside of descriptors (like comments) we'd be in a better place. But it would be some work. I believe we already have an issue for that.

We do have this mechanism - system privileges (described here). We use this, for example, to implement privileges on virtual tables: https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/syntheticprivilege/vtable_privilege.go. It's also used to implement cluster-wide privileges that can be inherited by role membership (unlike role options which are not inherited):

GlobalPrivileges = List{ALL, BACKUP, RESTORE, MODIFYCLUSTERSETTING, EXTERNALCONNECTION, VIEWACTIVITY, VIEWACTIVITYREDACTED, VIEWCLUSTERSETTING, CANCELQUERY, NOSQLLOGIN, VIEWCLUSTERMETADATA, VIEWDEBUG, EXTERNALIOIMPLICITACCESS, VIEWJOB, MODIFYSQLCLUSTERSETTING, REPLICATION, MANAGETENANT}

@knz
Copy link
Contributor

knz commented Aug 1, 2023

Rafi, these system privileges are not per system table it seems? I was hoping to see things like "this table is accessible via this priv, this other table is accessible via this other priv" or something like that.

@rafiss
Copy link
Collaborator

rafiss commented Aug 1, 2023

The framework can be used to implement what you say. For example, for virtual table privileges, we have used the framework to implement per-table privileges.

In addition to @maryliag's request here, let's also keep in mind the one raised by @jordanlewis -- that being, we want to make sure engineers can access system tables in order to handle support escalations for Cloud clusters. For that, we may indeed want a single privilege that gives read-only access to all system tables.

@knz
Copy link
Contributor

knz commented Aug 2, 2023

We discussed today that indeed the framework can help with the "troubleshooting access" question however we need to be careful about solving the access problem carefully:

  • we need a mechanism that also prevents access to other objects, including careful consideration for public
  • we need something that blocks indirect access to KV through built-in functions.

@rafiss agreed to explore this topic and come back with technical ideas before we continue the discussion.

@knz
Copy link
Contributor

knz commented Aug 3, 2023

Coming back to the issue at top:

We do have use cases where we want to use system tables on the console (as read-only)

We discussed that internally. TLDR: while we may wish to build out system privileges to give access to system tables via SQL to, say, SREs or support engineers, they do not seem to be a viable solution to build a console on top of.

Why:

  • it makes the front-end code responsible for data filtering. This is a bad security architecture; this means privileged data is leaving the cluster using user credentials . A malicious user with non-admin credentials could craft their own front-end code and see this confidential data. This choice would be blocked by our security auditors.
  • it would rely on an extremely high degree of security hygiene on every engineer in the team. This is unrealistic - we'd like to keep the ability to assign projects to newly onboarded or less experienced staff, and we cant' expect that no mistake will ever be made.

craig bot pushed a commit that referenced this issue Aug 25, 2023
108971: roachtest: some tweaks to unoptimized-query-oracle r=yuzefovich a=yuzefovich

This commit adjusts the `unoptimized-query-oracle` roachtest in the following manner:
- we now disable DistSQL for the unoptimized run and then randomize the DistSQL mode for the optimized run
- we no longer ignore internal errors on the unoptimized run.

Epic: None

Release note: None

109174: ui: fix metrics page when viewing a tenant with hyphenated name r=Santamaura a=Santamaura

This PR fixes an issue where no metrics would load in when a tenant had a hyphenated name. This was due to trying to use the `Long.fromString()` conversion on a string with a hyphen which it doesn't support. The logic was incorrect for this case anyway so instead a new function `determineTenantID` is introduced which will set the `tenant_id` to `undefined` in this case since we don't need to pass `tenant_id` when switching to a non-system tenant in global context.

Fixes: #109124

Release note (bug fix): fixed an issue on the metrics page where no metrics would load when viewing the non-system tenant in a global context when the tenant name was hyphenated.

<img width="1422" alt="Screenshot 2023-08-21 at 3 33 31 PM" src="https://github.com/cockroachdb/cockroach/assets/17861665/962ffacb-8763-4b5d-9257-c7e3982f7bd5">


109474: sql: add VIEWSYSTEMTABLE system privilege r=rafiss a=rafiss

This privilege is useful for support situations, where an engineer needs to be able to view system tables without having full admin access.

informs #95756
Release note (sql change): Added the VIEWSYSTEMTABLE system privilege. Users with this privilege have SELECT privileges for all tables in the system database.

109478: mirror: add logging for `go` errors r=liamgillies a=rickystewart

This can fail sometimes and the error message is very unclear. Add
logging so we can gather more data.

Epic: none
Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Alex Santamaura <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
@rafiss rafiss changed the title allow grant/revoke access to specific system tables sql: add system privilege for viewing system tables Aug 31, 2023
@rafiss
Copy link
Collaborator

rafiss commented Aug 31, 2023

I will close this now, but let me know if it needs to stay open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
No open projects
Archived in project
Development

No branches or pull requests

4 participants