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: Add option to RESTORE to preserve grants to specified users #45359

Open
dt opened this issue Feb 25, 2020 · 10 comments
Open

backupccl: Add option to RESTORE to preserve grants to specified users #45359

dt opened this issue Feb 25, 2020 · 10 comments
Labels
A-disaster-recovery C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-disaster-recovery

Comments

@dt
Copy link
Member

dt commented Feb 25, 2020

Currently all BACKUPs include the privilege grant information on individual tables and databases, even if those backups do not include the actual users/roles to which those privileges are granted.

During restore, these grants are wiped and replaced with default privileges as if the restore had created new tables, since it isn't obvious that RESTORE can assume that a grant to user "jsmith" on the old cluster should still apply to user "jsmith" on the new cluster -- they may not be the same person, they may have different access, or may not even exist, etc.

However if can operator is restoring into the same cluster or another cluster that does have the same users configured, it would be nice if they could choose to preserve that grant information instead of being forced to re-grant everything from scratch.

An option like RESTORE mytable ... WITH preserve_grants_for = 'jsmith,dtaylor,...' would allow operators to express when a grant to a given pre-backup user should be preserved and apply to a post-restore user. As a shorthand, preserve_grants_for = '*' could mean for all users (and error if any table in the backup has a grant to a username that does not exist on the restoring cluster).

Epic: CRDB-9127

Jira issue: CRDB-5156

gz#11757

@dt dt added A-disaster-recovery C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) labels Feb 25, 2020
@dt dt added this to Triage in Disaster Recovery Backlog via automation Feb 25, 2020
@dt dt assigned dt and pbardea Feb 25, 2020
@dt dt moved this from Triage to Backlog: Backup/Restore/Dump in Disaster Recovery Backlog Mar 6, 2020
@dt dt unassigned dt and pbardea Apr 24, 2020
@pbardea pbardea assigned pbardea and unassigned pbardea Jul 9, 2021
@pbardea
Copy link
Contributor

pbardea commented Jul 12, 2021

A first step here could be to just add a flag preserve_grants to restore that doesn't reset the grants on the descriptors. It would also perform additional validation:

  • The user performing the restore should be able to DROP all of the descriptors being restored
  • All of the users/roles referenced by the grants on the descriptors should exist. This check should be done transactionally with the creation of the descriptors to avoid races with concurrent user changes while the restore is running.

@pbardea
Copy link
Contributor

pbardea commented Jul 12, 2021

cc @livlobo @mwang1026 for visibility

@mwang1026
Copy link

All of the users/roles referenced by the grants on the descriptors should exist. This check should be done transactionally with the creation of the descriptors to avoid races with concurrent user changes while the restore is running.

Would it throw an error with the invalid user name? And would the UX be that they would have to create that user before the RESTORE would succeed? Alternatives could be

  • Exclude grants for certain individuals (so WITH preserve_grants,exclude_grant_for=(liv,michael) or something like that)
  • The above -- they have to create that user
  • Create that user (I think we should just not consider this without customer pushback, but here for completeness)
  • Something else?

Also, re: restoring of system.users we don't allow that, right? So you'd have to restore into a temp table, then insert into the users table? Would it preserve the passwords as well?

@pbardea pbardea added the E-starter Might be suitable for a starter project for new employees or team members. label Jul 22, 2021
@pbardea pbardea removed their assignment Jul 22, 2021
@gh-casper gh-casper self-assigned this Aug 26, 2021
@dt
Copy link
Member Author

dt commented Sep 9, 2021

@mwang1026 @livlobo and I think @vy-ton: Another question is what we should do with the default grants: do we replace them with the backed up grants (so you get back exactly what was backed up, which is generally the expectation) or do we add the backed up grants to the default grants on the newly created restoring tables, so that root/admin on the new cluster are assured to have the usual grants?

@mwang1026
Copy link

Are the default grants inherited or are they explicit? Like could you revoke select from admin on a table?

@RichardJCai
Copy link
Contributor

Are the default grants inherited or are they explicit? Like could you revoke select from admin on a table?

What does "default grants" mean here?

Like could you revoke select from admin on a table?
You can't do this at all in CockroachDB. It errors if you try.

@mwang1026
Copy link

That that's what I meant by "default" -- that root and admin have grants to everything. I wasn't sure if that's explicit (i.e. the descriptor explicitly says root has access) or whether it's inferred/inherited (i.e. we don't need the descriptor to say so because it is just fact that admin and root have access to everything)

@livlobo
Copy link
Contributor

livlobo commented Sep 15, 2021

Would it throw an error with the invalid user name? And would the UX be that they would have to create that user before the RESTORE would succeed?

  • If the user referenced by the grant on a descriptor does not exist, we could throw an error indicating the invalid usernames. Is it feasible to detect all invalid usernames and message that to the user? @dt
  • The error/notice could include a suggestion that they can either use the exclude_grants_for=(liv, michael) option or create user to continue with restore.

Also, re: restoring of system.users we don't allow that, right? So you'd have to restore into a temp table, then insert into the users table? Would it preserve the passwords as well?

  • Is it feasible to do this automatically under-the-hood? @dt

Another question is what we should do with the default grants: do we replace them with the backed up grants (so you get back exactly what was backed up, which is generally the expectation) or do we add the backed up grants to the default grants on the newly created restoring tables, so that root/admin on the new cluster are assured to have the usual grants?

  • Re: "default grants" on the admin role: The admin role will always have ALL privileges since they cannot be revoked, if I understand correctly. So,admin (and thereby root) will have the usual grants, whether we restore them or not.
  • Re: "default grants" on the public role: It makes sense to replace the privileges on the public role with the backed up privileges, so you get exactly what was backed up, since in my opinion, that would be most intuitive for the user.

@mwang1026
Copy link

IIRC in the demo that we saw from Casper

  • It would error if the user you're trying to restore grants for did not exist in the cluster -- I believe this is desired
  • It would inherit the grants from the database (e.g. if you did RESTORE DATABASE d1 ... WITH preserve_grants=user1), and if user1 had select on d1 but only insert on t1, the user would inherit the select on d1 for table t1 so grants on the restored t1 would be select, insert when on the backup they only had insert. I think we want it so that they only have insert.

@livlobo
Copy link
Contributor

livlobo commented Dec 12, 2021

@gh-casper - Could you comment on the status of this PR?

@adityamaru adityamaru added GA-blocker branch-release-22.1 Used to mark release blockers and technical advisories for 22.1 labels Mar 31, 2022
adityamaru added a commit to adityamaru/cockroach that referenced this issue Mar 31, 2022
Co-authored-by: casper@cockroachlabs.com

Fixes: cockroachdb#45359

Release note (enterprise change): This change adds an option to database
and table level restores to `preserve_grants_for` user(s) when restoring
the table or database.

A cluster restore is not allowed to specify this option since we already
restore all users as part of the `system.users` table, and therefore the descriptor
privileges are restored exactly how they were backed up.

In the case of table and database level restores, the users specified in
the `preserve_grants_for` option must be present in the restoring cluster.
If they are, then the privileges on the restored database, schema, type and
table descriptors for those users will be restored exactly how they were backed
up. Privileges for users not specified in the `preserve_grants_for` list will
continue to be wiped and the descriptors will be restored with the "default"
privileges as they are today.
@adityamaru adityamaru removed GA-blocker branch-release-22.1 Used to mark release blockers and technical advisories for 22.1 labels Apr 11, 2022
@adityamaru adityamaru removed their assignment May 12, 2023
@dt dt removed the E-starter Might be suitable for a starter project for new employees or team members. label Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-disaster-recovery C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-disaster-recovery
Projects
9 participants