vttablet: improved ACL support#3649
Conversation
vttablet was not correctly verifying ACLs for complex queries because the plan accommodated only one table to be referenced. - [X] Function to build the list of tables and their permissions - [ ] Add Permissions list to planbuilder.Plan - [ ] Use that Permissions list in QueryEngine instead of Table - [ ] Change QueryExecutor to iterate through the list - [ ] Remove obsolete PlanID.MinRole() - [ ] Test
sougou
changed the title
|
@alainjobart this is ready for review. Feel free to add a bug number if applicable. @acharis @rnavarro @demmer and others who currently rely on table ACLs: this is a breaking change because what was previously allowed may now fail. |
|
I love this solution, much more elegant, and correct! Thanks Sugu! |
plan.Authorized is now an array built from plan.Permissions. It's the authoritative list of table acl checks that need to be performed. The previous code that used to chase down table names in the plan is now simplified to only rely on Authorized.
MySQL does not support updatable subqueries. So, a construct like update (select * from a) is invalid. However, there are versions of SQL that support this and interpret as something similar to an updatable view. So, in order to be future-proof, I've made a corresponding modification to require write permission for the table if such constructs are seen.
|
In #3651 a flag was used to enable the extra checks. Could we use a flag here to enable the extra checks on tables 1+ (leaving table 0 check always in), so that we can roll out the change slowly? And in a couple weeks, we can just remove the flag. It is unclear to us what impact this will have in production, so we'd like to have a quick way to disable the new behavior just in case... |
|
That won't take it back to old behavior, because no checks were performed for joins before. |
|
That flag sounds great. I am not sure who else is using table ACLs, and if they care about their roll out. If nobody else is, we can take care of removing the flag when we are done with our roll out. |
|
Thanks @sougou the flag looks good. I'll merge after travis passes. Then in a couple weeks we'll remove the legacy flag. |
| ) | ||
|
|
||
| // TODO(sougou): remove after affected parties have transitioned to new behavior. | ||
| var legacyTableACL = flag.Bool("legacy-table-acl", false, "deprecated: this flag can be used to revert to the older table ACL behavior, which checked access for at most one table") |
There was a problem hiding this comment.
Could you change the default to true?
If set to false and someone creates a rollout without setting the flat to false then they'd roll out the new behavior. And we can't set the flag in our builds until it's present in the code. We can change the default after we've done the rollout.
There was a problem hiding this comment.
We can solve that with undefok, right?
Whichever way we go, we'll see what's more convenient for us and we can fix it in the tree...
There was a problem hiding this comment.
I agree with Alain; We should keep the default as false and use undefok to control the rollout of the new behavior.
6 participants
This is a long overdue fix.
VTTablet was not correctly verifying ACLs for complex queries because the plan had only one 'Table' field. For queries that performed joins, vttablet would simply not perform any ACL checks. Also, subqueries were always ignored.
This change fixes all the above problems: the planbuilder builds a list of 'Permissions' which associate each referenced TableName with a Role. This is then used by QueryEngine to build a list of Authorizations (Authorized). They are all checked against the incoming request's caller id for eligibility.
We still need to upgrade the Table field in the Plan to allow for multiple tables, but that problem is now independent of the ACL feature.