Adapter: unify RBAC for Side Effecting Functions#35220
Adapter: unify RBAC for Side Effecting Functions#35220DAlperin wants to merge 1 commit intoMaterializeInc:mainfrom
Conversation
|
Thanks for opening this PR! Here are a few tips to help make the review process smooth for everyone. PR title guidelines
Pre-merge checklist
|
DAlperin
force-pushed
the
dov/unify-rbac-side-effecting-function
branch
2 times, most recently
from
1f82600 to
66b07dd
Compare
During an incident I tried to kill some backends with mz_system and I couldn't. This was because the usually RBAC bypass doesn't work for the special impl in the coord for side effect functions. I'd prefer to unify this where possible and feel it's worth paying an extra coord hop is worth it.
DAlperin
force-pushed
the
dov/unify-rbac-side-effecting-function
branch
from
66b07dd to
129c4ed
Compare
ggevay
left a comment
ggevay
left a comment
There was a problem hiding this comment.
Looks good! Thank you very much for figuring this out and fixing it! And sorry for the bug!
|
|
||
| rbac::check_plan( | ||
| &conn_catalog, | ||
| Some(move |_id: u32| active_conns_role), |
There was a problem hiding this comment.
Making the closure not use _id (just always returning PgCancelBackend's target connection's role) runs the risk that some code written later will call the closure with some other connection, and silently get a wrong role. How about asserting in the closure that the closure's argument is the same as the target conn, i.e., _id == connection_id?
There was a problem hiding this comment.
Or we could maybe even simplify this from a closure to a simple Option<RoleId>, because the closure is never called with a variety of connections, but only ever called on PgCancelBackend's target connection. Also, the closure's generality is not really needed even for further side-effecting funcs in Postgres that we might want to implement at some point: The only other such functions where roles of some other connections than the issuer are relevant are pg_terminate_backend and pg_log_backend_memory_contexts, which also just want to look at the role of a single connection, i.e., their target connection. So, an Option<RoleId> would be enough for them as well. What do you think?
| plan: SideEffectingFunc, | ||
| conn_id: ConnectionId, | ||
| /// The current role of the session, used for RBAC checks. | ||
| current_role: RoleId, |
2 participants
During an incident I tried to kill some backends with mz_system and I couldn't. This was because the usually RBAC bypass doesn't work for the special impl in the coord for side effect functions. I'd prefer to unify this where possible and feel it's worth paying an extra coord hop is worth it.