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.

Sign up for GitHub

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

Jump to bottom

sql: remove default CREATE privilege on public schema [compat with PG 15] #70266

Closed
rafiss opened this issue Sep 15, 2021 · 2 comments · Fixed by #103598
Closed

sql: remove default CREATE privilege on public schema [compat with PG 15] #70266

rafiss opened this issue Sep 15, 2021 · 2 comments · Fixed by #103598
Assignees
@rafiss
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-privileges SQL privilege handling and permission checks. 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

@rafiss
Copy link
Collaborator

rafiss commented Sep 15, 2021 •
edited
Loading

Postgres 15 will include this change:

Revoke PUBLIC CREATE from public schema, now owned by pg_database_owner.

This switches the default ACL to what the documentation has recommended
since CVE-2018-1058.  Upgrades will carry forward any old ownership and
ACL.  Sites that declined the 2018 recommendation should take a fresh
look.  Recipes for commissioning a new database cluster from scratch may
need to create a schema, grant more privileges, etc.  Out-of-tree test
suites may require such updates.
 
Reviewed by Peter Eisentraut.
 
Discussion: https://postgr.es/m/[email protected]

Currently, CockroachDB matches the old Postgres behavior, but since we want our privileges to be more compatible with Postgres, we should adapt this change too. (Let's just make sure that commit doesn't get reverted before PG 15 is released.)

To resolve this issue: change it so non-superuser accounts are not able to create tables in the public schema of databases they don't own. Make sure to update remapPublicSchemas in backupccl/restore_job.go too. This should be controlled by a cluster setting, which defaults to keeping the old behavior.

This should be the new behavior:

$ create database x;
CREATE DATABASE
 
$ create user test;
CREATE ROLE
 
$ create database test with owner test;
CREATE DATABASE

# Quit sql shell

$ psql -U test -d x -c 'create table a (b int)'
ERROR:  permission denied for schema public
LINE 1: create table a (b int)

$ psql -U test -d test -c 'create table a (b int)'
CREATE TABLE

Jira issue: CRDB-10011

Epic CRDB-26874
@rafiss rafiss added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-privileges SQL privilege handling and permission checks. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Sep 15, 2021
@rafiss rafiss changed the title sql: remove default CREATE privilege on public schema sql: remove default CREATE privilege on public schema [compat with PG 15] Dec 7, 2021
@rafiss rafiss mentioned this issue Dec 15, 2021
Merged
craig bot pushed a commit that referenced this issue Dec 17, 2021
@AlexTalks @rafiss
@maryliag @nvanbenschoten @jaylim-crl @otan
Merge #73500 #73862 #73873 #73932 #73943 #73946 #73954
089affc 
73500: kv,storage: persist gateway node id in transaction intents r=AlexTalks a=AlexTalks

This change augments the `TxnMeta` protobuf structure to include the
gateway node ID (responsible for initiating the transaction) when
serializing the intent.  By doing so, this commit enables the Contention
Event Store proposed in #71965, utilizing option 2.

Release note: None

73862: sql: add test asserting CREATE/USAGE on public schema r=otan a=rafiss

refs #70266

The public schema currently always has CREATE/USAGE privileges
for the public role. Add a test that confirms this.

Release note: None

73873: scdeps: tighten dependencies, log more side effects r=postamar a=postamar

This commit reworks the dependency injection for the event logger, among
other declarative schema changer dependencies. It also makes the test
dependencies more chatty in the side effects log.

Release note: None

73932: ui: select grants tab on table details page r=maryliag a=maryliag

Previosuly, when the grants view was selected on the Database
Details page, it was going to the Table Details with the Overview
tab selected.
With this commit, if the view mode selected is Grant, the grant
tab is selected on the Table Details page.

Fixes #68829

Release note: None

73943: cli: support --locality and --max-offset flags with sql tenant pods r=nvanbenschoten a=nvanbenschoten

This commit adds support for the `--locality` and `--max-offset` flags to the `cockroach mt start-sql` command.

The first of these is important because tenant SQL pods should know where they reside. This will be important in the future for multi-region serverless and also for projects like #72593.

The second of these is important because the SQL pod's max-offset setting needs to be the same as the host cluster's. If we want to be able to configure the host cluster's maximum clock offset to some non-default value, we'll need SQL pods to be configured identically.

Validation of plumbing:
```sh
./cockroach start-single-node --insecure --max-offset=250ms
./cockroach sql --insecure -e 'select crdb_internal.create_tenant(2)'

 # verify --max-offset

./cockroach mt start-sql --insecure --tenant-id=2 --sql-addr=:26258 --http-addr=:0
 # CRDB crashes with error "locally configured maximum clock offset (250ms) does not match that of node [::]:62744 (500ms)"

./cockroach mt start-sql --insecure --tenant-id=2 --sql-addr=:26258 --http-addr=:0 --max-offset=250ms
 # successful

 # verify --locality

./cockroach sql --insecure --port=26258 -e 'select gateway_region()'

ERROR: gateway_region(): no region set on the locality flag on this node

./cockroach mt start-sql --insecure --tenant-id=2 --sql-addr=:26258 --http-addr=:0 --max-offset=250ms --locality=region=us-east1

./cockroach sql --insecure --port=26258 -e 'select gateway_region()'

  gateway_region
------------------
  us-east1
```

73946: ccl/sqlproxyccl: fix TestWatchPods under stressrace r=jaylim-crl a=jaylim-crl

Fixes #69220.
Regression from #67452.

In #67452, we omitted DRAINING pods from the tenant directory. Whenever a pod
goes into the DRAINING state, the pod watcher needs time to update the
directory. Not waiting for that while calling EnsureTenantAddr produces a
stale result. This commit updates TestWatchPods by polling on EnsureTenantAddr
until the pod watcher updates the directory.

Release note: None

73954: sqlsmith: don't compare voids for joins r=rafiss a=otan

No comparison expr is defined on voids, so don't generate comparisons
for them.

Resolves #73901
Resolves #73898
Resolves #73777

Release note: None

Co-authored-by: Alex Sarkesian <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Marius Posta <[email protected]>
Co-authored-by: Marylia Gutierrez <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Jay <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
@rafiss
Copy link
Collaborator Author

rafiss commented Jan 9, 2023

See this internal Slack thread for an example where a customer manually ran REVOKE CREATE ON SCHEMA public FROM public in order to remove the default CREATE privilege.

@e-mbrown
Copy link
Contributor

e-mbrown commented May 3, 2023

What is the expected behavior if the cluster setting is switched on and then at a later point switched off? Should new users have default create on public while previous users do not?

@e-mbrown e-mbrown mentioned this issue May 10, 2023
Closed
@exalate-issue-sync exalate-issue-sync bot assigned rafiss and unassigned e-mbrown May 15, 2023
@rafiss rafiss mentioned this issue May 18, 2023
Merged
@craig craig bot closed this as completed in 23de673 May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rafiss rafiss

Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-privileges SQL privilege handling and permission checks. 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
None yet
Milestone
No milestone
2 participants
@rafiss @e-mbrown