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

backupccl: check privileges on types and schemas during backup #54183

Merged
merged 1 commit into from
Sep 14, 2020

Conversation

pbardea
Copy link
Contributor

@pbardea pbardea commented Sep 10, 2020

This commit adds a check that ensures that the user running the backup
has USAGE privileges on the schemas and types they're backing up.

Note that this check doesn't actually enforce anything for now since
currently BACKUP must be run by an admin, which has to have ALL
privileges on both types and schemas. However, this check is consistent
with the other privilege checks we have for databases and tables.

Part of #53548.

Release note: None

This commit adds a check that ensures that the user running the backup
has USAGE privileges on the schemas and types they're backing up.

Note that this check doesn't actually enforce anything for now since
currently BACKUP must be run by an `admin`, which has to have ALL
privileges on both types and schemas. However, this check is consistent
with the other privilege checks we have for databases and tables.

Release note: None
@pbardea pbardea requested review from solongordon, a team and miretskiy and removed request for a team September 10, 2020 14:55
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@solongordon solongordon left a comment

Choose a reason for hiding this comment

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

Do we have the corresponding checks on the RESTORE side? For instance if a non-admin restores a table which makes use of a pre-existing enum, do we check that the user has USAGE privileges on that enum?

@pbardea
Copy link
Contributor Author

pbardea commented Sep 14, 2020

I've been thinking about this more and since #53650 went in, I think that we need to be careful about allowing non-admins to RESTORE. The tricky part about allowing non-admins to restore is that RESTORE has always assumed that the users in the new cluster might not be the same users in the cluster that backed up the descriptors (even if they share the same name). Although they have the same name, they might not actually represent the same user (since a) BACKUP doesn't include the actual users that it restored and b) you might be restoring into an entirely different cluster).

This becomes an issue when allowing non-admins to RESTORE since we should now be checking if they have sufficient privileges on the descriptors they're restoring. However, to do this check we need to assume that the users on the restoring cluster are the same as the ones in the backing up cluster. Restore has long considered this assumption to be false.

I'm not sure what the best way forward here. I think to fully enable non-admins to RESTORE, we might need to support a more complete story around RESTORE handling users and grants, which might look something like #45359 + #45358. But these changes are a bit much to include during stability.

Also open to any suggestions if you have any ideas.

@solongordon
Copy link
Contributor

I can understand the concern but I'm not convinced that "we should now be checking if they have sufficient privileges on the descriptors they're restoring." Since we enforce that non-admins need to use explicit credentials when restoring, doesn't that imply that they have privileged access to the backups? And in the case of restoring encrypted backups, the user would need to know the encryption_passphrase.

In my thinking, if a non-admin user has explicit permission to access the backups, there's no security issue in giving them permission to restore. After all, there's already nothing stopping such a user from creating a new cluster where they do have admin access and restoring the backup there.

@pbardea
Copy link
Contributor Author

pbardea commented Sep 14, 2020

TFTRs!
bors r=solongordon,dt

@pbardea
Copy link
Contributor Author

pbardea commented Sep 14, 2020

@solongordon, I think that makes sense - I think highlighting that in our docs would be useful, as we allow non-admin users to RESTORE. I can file a docs issues.

@pbardea pbardea removed the request for review from miretskiy September 14, 2020 14:07
@craig
Copy link
Contributor

craig bot commented Sep 14, 2020

Build succeeded:

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.

None yet

4 participants