privilege: don't include NOSQLLOGIN in ALL check #104685
Conversation
Release note (bug fix): GRANT SYSTEM ALL ... no longer causes the grantee to be unable to login. This was due to a UX oversight/bug where ALL would include the NOSQLLOGIN system privilege. Since NOSQLLOGIN is the only "negative" privilege, it is now excluded from the ALL shorthand, and must be granted explicitly in order to restrict logins.
rafiss
added
backport-22.2.x
backport-23.1.x
|
This change is |
| @@ -513,7 +513,10 @@ func (p PrivilegeDescriptor) CheckPrivilege(user username.SQLUsername, priv priv | |||
| return user.IsNodeUser() | |||
| } | |||
|
|
|||
| if privilege.ALL.IsSetIn(userPriv.Privileges) { | |||
| if privilege.ALL.IsSetIn(userPriv.Privileges) && priv != privilege.NOSQLLOGIN { | |||
There was a problem hiding this comment.
Is there a reason why we have a "negative" privilege? It should probably be reversed to be positive to avoid requiring special casing like this since I think it makes the code hard to reason about.
There was a problem hiding this comment.
probably the reason is that unlike other privileges, we want people to be able to log in by default, and storing and managing a SQLLOGIN privilege for every user would have its own set of complexities that would make the code hard to reason about.
our team wasn't actually the one who added this privilege, so I don't know the specific reason.
but regardless, it would be a very expensive project for us to remove the old privilege and migrate to a new inverse one. and even apart from engineering effort, it would be a disruptive change for operators of CRDB. I don't think the amount of effort of that project is worth it compared to adding a special case to this privilege check.
There was a problem hiding this comment.
Ok I approved this since it looks like it addresses the issue we have currently, but I think we need a better long term fix in order to prevent this from causing future issues.
There was a problem hiding this comment.
in a followup PR i will make a way to make privileges be marked as negative, so it is easy to handle them all in the same way. but i don't want to backport that, since it will lead to more merge conflicts.
|
tftr bors r+ |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 1755e8b to blathers/backport-release-22.2-104685: 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. |
fixes #101292
Release note (bug fix): GRANT SYSTEM ALL ... no longer causes the grantee to be unable to login. This was due to a UX oversight/bug where ALL would include the NOSQLLOGIN system privilege. Since NOSQLLOGIN is the only "negative" privilege, it is now excluded from the ALL shorthand, and must be granted explicitly in order to restrict logins.