Allow hive table owner to change ownership #23842
Conversation
|
@guyco33 pls sketch a bit a scenario where it makes sense to change ownership of the hive table/view and outline the current limitations of the system without your change. |
|
This is related to #16691 |
| @@ -317,7 +317,7 @@ public void checkCanAlterColumn(ConnectorSecurityContext context, SchemaTableNam | |||
| @Override | |||
| public void checkCanSetTableAuthorization(ConnectorSecurityContext context, SchemaTableName tableName, TrinoPrincipal principal) | |||
| { | |||
| if (!isAdmin(context)) { | |||
| if (!isTableOwner(context, tableName)) { | |||
There was a problem hiding this comment.
This is insecure. IMO it would introduce a vector attack for Trino that was removed in #10351 (it is on purpose not having all details).
It is not enough to see you can own the object, but we need to make sure that to what users we can grant it. See some thinking here #16691 (review)
There was a problem hiding this comment.
Table owner can grant any permission to any user including the WITH GRANT OPTION, so I don't understand how changing ownership that actually add the privilege to DROP the table will add a new security risk.
Same for views. If the view security mode was set to DEFINER than all masking functions and filter rules will run on behalf of the new owner privileges, so if the new owner is blocked from viewing some specific sensitive columns, all executed queries on the view will result with masked data.
Views with INVOKER security mode are executed with the privileges of the authenticated users running the queries - so no issue here.
There was a problem hiding this comment.
if the new owner is blocked from viewing some specific sensitive columns
Yes. But access could be different and so it could happen that things that here not visible to original user are now accessible to that user by querying the view they created and altered (transferred ownerhsip).
There was a problem hiding this comment.
But access could be different and so it could happen that things that here not visible to original user are now accessible to that user by querying the view they created and altered
You are right. The view owner can change the ownership to any privileged user, so we should allow it only for views created with INVOKER security mode
There was a problem hiding this comment.
so we should allow it only for views created with INVOKER security mode
I am not a fan of such conditions. I would prefer to have secure security model instead. However I am not sure if it is doable in hive sql-standard.
There was a problem hiding this comment.
Views can be recreated by a user with proper privileges to become the new owner, so I can skip this change for views and keep it for tables only.
We are using hive with Currently there is no option to transfer table ownership by the table owner. |
guyco33
changed the title
guyco33
force-pushed
the
allow_owner_set_authorization
branch
from
48d5664 to
c0d8a16
Compare
guyco33
force-pushed
the
allow_owner_set_authorization
branch
from
c0d8a16 to
a2a8dd5
Compare
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time. |
Description
Hive sql-standard security mode restricts the possibility to change ownership to
adminusers only, making the authorization rules almost useless.With this PR, Hive table owners will be able to transfer the ownership to other users, similar to community databases like Postgres.
For views, only admin users will be able to transfer ownership, as by letting owners to do it, a security breach will happen while they might transfer the ownership to users that able to select sensitive masked columns (or more filtered rows) and expose themself to data that they are not allowed to query.
Actually the option for transferring ownership to other users can be achieved by letting the new owner a grant to show view code and recreate the view and become the new owner.
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: