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

ingesting: fixup privileges granted during database restore #95466

Merged
merged 1 commit into from
Jan 19, 2023

Conversation

adityamaru
Copy link
Contributor

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

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 requested a review from rafiss January 18, 2023 21:59
@adityamaru adityamaru requested a review from a team as a code owner January 18, 2023 21:59
@adityamaru adityamaru requested review from a team and rhu713 and removed request for a team January 18, 2023 21:59
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the fix!

@adityamaru
Copy link
Contributor Author

Flake is /cockroach-go-testserver-22_1-22_2_test that is being tracked elsewhere.

TFTR!

bors r=rafiss

@data-matt
Copy link

@adityamaru , does it grant CREATE/USAGE to public role blindly?

We have customers revoking CREATE and USAGE from the public role

@adityamaru
Copy link
Contributor Author

adityamaru commented Jan 19, 2023

does it grant CREATE/USAGE to public role blindly

Yes, a database restore will now grant CREATE/USAGE on the public schema to the public role. This is in line with what happens if you were to create a database using CREATE DATABASE so restores behavior was irregular.

@data-matt
Copy link

I don't think thats a great idea, that will create surprises.

Imagine you are a user.

You revoke from this access from the public role.

You restore and CREATE/USAGE comes back..

@craig
Copy link
Contributor

craig bot commented Jan 19, 2023

Build failed (retrying...):

@adityamaru
Copy link
Contributor Author

You revoke from this access from the public role.

When a user revokes these privileges it will be from a public schema of an existing database foo. They will not be able to restore database foo without dropping database foo before running the restore. Instead of a restore, had they run CREATE DATABASE foo after the drop they would also end up with a foo.public schema with public having CREATE and USAGE. So I'm not sure I follow how we're expanding the privilege set after the user has revoked them.
A restore is essentially a CREATE + backfill of data.

@rafiss
Copy link
Collaborator

rafiss commented Jan 19, 2023

@data-matt one thing to clarify: for a cluster-level RESTORE, all entities and privileges on those entities will be restored exactly as they were at the time of the BACKUP.

But this PR is for the database RESTORE. The scenario you are describing also applies to tables. (If a customer had granted privileges on a table, then does a database RESTORE, the tables will be created with the default privileges and the custom grants will be lost. Is my understanding right @adityamaru?)

@data-matt
Copy link

Yeah, I realised my point is moot, since we only restore grants for full cluster backups. So, the users have an action to put the grants back anyway. Please ignore me :D

@craig craig bot merged commit f27fa9d into cockroachdb:master Jan 19, 2023
@craig
Copy link
Contributor

craig bot commented Jan 19, 2023

Build succeeded:

@blathers-crl
Copy link

blathers-crl bot commented Jan 19, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 05cfdac to blathers/backport-release-22.1-95466: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.


error creating merge commit from 05cfdac to blathers/backport-release-22.2-95466: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

backupccl: schemas restored as part of a database restore do not get adequate privileges
4 participants