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

backupccl: schemas restored as part of a database restore do not get adequate privileges #95456

Closed
adityamaru opened this issue Jan 18, 2023 · 2 comments · Fixed by #95466
Closed

backupccl: schemas restored as part of a database restore do not get adequate privileges #95456

adityamaru opened this issue Jan 18, 2023 · 2 comments · Fixed by #95466
Assignees
@adityamaru
Labels
branch-master Failures and bugs on the master branch. branch-release-22.1 Used to mark GA and release blockers, technical advisories, and bugs for 22.1 branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-disaster-recovery

Comments

@adityamaru
Copy link
Contributor

adityamaru commented Jan 18, 2023 •
edited by cockroach-jira-scripts
Loading

As part of a database restore, we also restore the backed-up schemas. In a DB restore, before we publish the database descriptor into the restoring cluster we give it the base default privileges that include CONNECT to public, and ALL to admin and root. Schemas in this database then "inherit" ALL to admin and root before they are published in the restoring cluster.
This behavior is different from if a user creates a new database in the cluster which results in the following privileges:

root@:26257/defaultdb> create database foo;
CREATE DATABASE


Time: 52ms total (execution 51ms / network 1ms)

root@:26257/defaultdb> show grants on database foo;
  database_name | grantee | privilege_type | is_grantable
----------------+---------+----------------+---------------
  foo           | admin   | ALL            |     true
  foo           | public  | CONNECT        |    false
  foo           | root    | ALL            |     true
(3 rows)


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

root@:26257/defaultdb> show grants on schema foo.public;
  database_name | schema_name | grantee | privilege_type | is_grantable
----------------+-------------+---------+----------------+---------------
  foo           | public      | admin   | ALL            |     true
  foo           | public      | public  | CREATE         |    false
  foo           | public      | public  | USAGE          |    false
  foo           | public      | root    | ALL            |     true
(4 rows)


Time: 5ms total (execution 5ms / network 1ms)

A database restore should be granting public the CREATE and USAGE privileges for the public schema, in addition to ALL for root and admin. All other user defined schemas will only get ALL for root and admin.

Jira issue: CRDB-23526
@adityamaru adityamaru added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. branch-master Failures and bugs on the master branch. branch-release-22.1 Used to mark GA and release blockers, technical advisories, and bugs for 22.1 branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 labels Jan 18, 2023
@blathers-crl blathers-crl bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) T-disaster-recovery labels Jan 18, 2023
@blathers-crl
Copy link

blathers-crl bot commented Jan 18, 2023

cc @cockroachdb/disaster-recovery

@adityamaru adityamaru mentioned this issue Jan 18, 2023
Merged
@adityamaru adityamaru self-assigned this Jan 18, 2023
craig bot pushed a commit that referenced this issue Jan 19, 2023
@maryliag @adityamaru @aliher1911
Merge #95461 #95466 #95467 #95504 #95512
f27fa9d 
95461: ui: remove reset sql stats for non-admin r=maryliag a=maryliag

Continuation from #95303

The previous PR missed the reset on the Transactions tab. This PR removes the reset sql stats for non-admin users.

Fixes #95213

Release note (ui change): Remove `reset sql stats` from Transactions page for non-admins.

95466: ingesting: fixup privileges granted during database restore r=rafiss a=adityamaru

Previously, all schemas and tables that were ingested as part of a database restore would "inherit" the privileges of the database. The database would be granted `CONNECT` for the `public` role and `ALL` to `admin` and `root`, and so all ingested schemas would have `ALL` for `admin` and `root`. Since 21.2 we have moved away from tables/schemas inheriting privileges from the parent database and so this logic is stale and partly incorrect. It is incorrect because the restored `public` schema does not have `CREATE` and `USAGE` granted to the `public` role. These privileges are always granted to `public` schemas of a database and so there is a discrepancy in restore's behaviour.

This change simplifies the logic to grant schemas and tables their default set of privileges if ingested via a database restore. It leaves the logic for cluster and table restores unchanged.

Release note (bug fix): fixes a bug where a database restore would not grant `CREATE` and `USAGE` on the public schema to the public role

Fixes: #95456

95467: schemachanger: a bunch of small fixes r=postamar a=postamar

Informs #88294.

Release note: None

95504: schemachangerccl: rename generated tests r=postamar a=postamar

This commit adds an underscore in the generated tests' name where there previously was one missing.

Informs #88294.

Release note: None

95512: kvserver: fix flaky TestLearnerReplicateQueueRace test r=tbg a=aliher1911

Test was not expecting raft snapshots and injected failure at wrong moments.

Fixes #94993

Release note: None

Co-authored-by: maryliag <[email protected]>
Co-authored-by: adityamaru <[email protected]>
Co-authored-by: Marius Posta <[email protected]>
Co-authored-by: Oleg Afanasyev <[email protected]>
@craig craig bot closed this as completed in 05cfdac Jan 19, 2023
@adityamaru adityamaru mentioned this issue Jan 19, 2023
Merged
adityamaru added a commit to adityamaru/cockroach that referenced this issue Jan 19, 2023
@adityamaru
ingesting: fixup privileges granted during database restore
8ac1746 
Previously, all schemas and tables that were ingested as part
of a database restore would "inherit" the privileges of the
database. The database would be granted `CONNECT` for the `public` role and
`ALL` to `admin` and `root`, and so all ingested schemas would have `ALL`
for `admin` and `root`. Since 21.2 we have moved away from
tables/schemas inheriting privileges from the parent database
and so this logic is stale and partly incorrect. It is incorrect
because the restored `public` schema does not have `CREATE` and
`USAGE` granted to the `public` role. These privileges are always
granted to `public` schemas of a database and so there is a discrepancy
in restore's behaviour.

This change simplifies the logic to grant schemas and tables their
default set of privileges if ingested via a database restore. It leaves
the logic for cluster and table restores unchanged.

Release note (bug fix): fixes a bug where a database restore would not
grant `CREATE` and `USAGE` on the public schema to the public role

Fixes: cockroachdb#95456
adityamaru added a commit to adityamaru/cockroach that referenced this issue Jan 19, 2023
@adityamaru
ingesting: fixup privileges granted during database restore
4ba05f9 
Previously, all schemas and tables that were ingested as part
of a database restore would "inherit" the privileges of the
database. The database would be granted `CONNECT` for the `public` role and
`ALL` to `admin` and `root`, and so all ingested schemas would have `ALL`
for `admin` and `root`. Since 21.2 we have moved away from
tables/schemas inheriting privileges from the parent database
and so this logic is stale and partly incorrect. It is incorrect
because the restored `public` schema does not have `CREATE` and
`USAGE` granted to the `public` role. These privileges are always
granted to `public` schemas of a database and so there is a discrepancy
in restore's behaviour.

This change simplifies the logic to grant schemas and tables their
default set of privileges if ingested via a database restore. It leaves
the logic for cluster and table restores unchanged.

Release note (bug fix): fixes a bug where a database restore would not
grant `CREATE` and `USAGE` on the public schema to the public role

Fixes: cockroachdb#95456
@adityamaru adityamaru mentioned this issue Jan 19, 2023
Merged
@exalate-issue-sync exalate-issue-sync bot added T-disaster-recovery and removed T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Jan 26, 2023
@blathers-crl
Copy link

blathers-crl bot commented Jan 26, 2023

cc @cockroachdb/disaster-recovery

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adityamaru adityamaru

Labels
branch-master Failures and bugs on the master branch. branch-release-22.1 Used to mark GA and release blockers, technical advisories, and bugs for 22.1 branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-disaster-recovery
Projects
No open projects
Disaster Recovery Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

ingesting: fixup privileges granted during database restore adityamaru/cockroach
1 participant
@adityamaru