cockroachdb / cockroachdb/cockroach

sql: savetable check for non-root user would be bypassed if query cached disabled

Open
#157,237 1 comment 0 reactions 0 assignees View on GitHub
A-testing branch-master C-bug C-cleanup T-sql-queries
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

We now only check in `optPlanningCtx.buildReusableMemo()` if the savetable feature is enabled, which means this check is only reachable if query cache is enabled. The following test, with query cache disabled, will actually not reach the check.

```sql
statement ok
SET CLUSTER SETTING sql.query_cache.enabled = false;

statement ok
CREATE DATABASE savetables; USE savetables

statement ok
CREATE TABLE t (k INT PRIMARY KEY, str STRING)

statement ok
INSERT INTO t SELECT i, to_english(i) FROM generate_series(1, 5) AS g(i)

# Only root may use the saveTableNode.

statement ok
GRANT ALL ON t TO testuser

user testuser

statement ok
USE savetables

query IT rowsort
SELECT * FROM t
----
1 one
2 two
3 three
4 four
5 five

statement ok
SET save_tables_prefix = 'tt'

statement error sub-expression tables creation may only be used by root
SELECT * FROM t
```

The last `SELECT * FROM t` is expected to return error `sub-expression tables creation may only be used by root`, but because we disabled query cache, this statement would falsely succeed.

I think we should pull the following check outside of the logic that is only reachable when query cache is enabled:

```
if p.SessionData().SaveTablesPrefix != "" && !p.SessionData().User().IsRootUser() {
return nil, memoTypeUnknown, pgerror.New(pgcode.InsufficientPrivilege,
"sub-expression tables creation may only be used by root",
)
}
```

But also as a caveat, we don't want this check of overkill all queries that doesn't interact with the savetable feature. For example, if we enforce this check even for a `SET save_tables_prefix` statement, we will fall into a deadlock -- user wouldn't even be able to disable using savetables.

Jira issue: CRDB-56426

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.