Verify authorization access for SET AUTHORIZATION statements #16691

kokosing · 2023-03-23T11:34:39Z

With this pull request we are going to assume from all pluggable access control to make sure that SET AUTHORIZATION statements are secure to be performed. Previously it was not clear if it is engine or access control responsibility. Hence we had legacy.allow-set-view-authorization config property.

To accomplish that now:

hive.security=sql-standard requires admin role privilege to perform ALTER ... SET AUTHORIZATION... statements
File based access control have now new authorization rules that verify to whom AUTHORIZATION can be set.

Thanks to the above legacy.allow-set-view-authorization can be removed.

Release notes:
Hive

The hive.security=sql-standardsecurty requires admin role privilege to performALTER ... SET AUTHORIZATION...` statements

Security:

Introduce authorization rules to file based access controls to control ALTER ... SET AUTHORIZATION... to whom authorization can be set.
Remove legacy.allow-set-view-authorization configuration property

ksobolew

There are two concerns here:

Whether impersonation is the right precondition for this
- We discussed this when we worked on this last time, and this was one of the suggestions. I think it makes sense, but I'm no strongly sold on it yet.
This requires all access control to implement this in a correct way, which is error prone.
- I would suggest moving the logic to AccessControlManager and remove the checkCanSetViewAuthorization from the access control interface(s).

Bonus concern:

The commit message seems malformed :) It should explain why impersonation is the correct thing to do, not just that it's delegated to the access control.

core/trino-main/src/main/java/io/trino/execution/SetViewAuthorizationTask.java

core/trino-main/src/test/java/io/trino/security/TestFileBasedSystemAccessControl.java

...trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedAccessControl.java

kokosing · 2023-04-13T05:55:10Z

AC, PTAL

ksobolew

So file-based access controls will have their own model for this? We were discussing things like checking for impersonation, does that mean this is not viable here or just undesirable?

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AuthorizationRule.java

...trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedAccessControl.java

...plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedSystemAccessControl.java

ksobolew · 2023-04-13T09:26:55Z

...n-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedSystemAccessControlRules.java

@@ -83,6 +86,11 @@ public Optional<List<SystemInformationRule>> getSystemInformationRules()
        return systemInformationRules;
    }

+    public List<AuthorizationRule> getAuthorizationRules()


Why not return Optional like from all the others?

Because this one if it is not present is denied by default.

...oolkit/src/test/java/io/trino/plugin/base/security/BaseFileBasedSystemAccessControlTest.java

ksobolew · 2023-04-13T09:49:46Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/SqlStandardAccessControl.java

-        if (!isTableOwner(context, viewName)) {
-            denySetViewAuthorization(viewName.toString(), principal);
-        }
+        denySetViewAuthorization(viewName.toString(), principal);


Does this warrant a separate commit?

ksobolew · 2023-04-13T09:53:46Z

...kit/src/test/java/io/trino/plugin/base/security/BaseFileBasedConnectorAccessControlTest.java

+    @Test
+    public void testSchemaAuthorization()
+    {
+        ConnectorAccessControl accessControl = createAccessControl("authorization-no-roles.json");


Why no roles in connector access control tests?

there is also authorization.json, however file based connector access control does not support roles.

checkArgument(!rules.hasRoleRules(), "File connector access control does not support role rules");

...kit/src/test/java/io/trino/plugin/base/security/BaseFileBasedConnectorAccessControlTest.java

kokosing · 2023-05-29T15:54:04Z

AC, PTAL

martint · 2023-05-29T15:57:33Z

docs/src/main/sphinx/security/file-system-access-control.rst

@@ -678,7 +678,51 @@ as ``[email protected]``, you can use the following rules.
      ]
    }

+<<<<<<< HEAD


Unresolved merge conflict?

dain

The message on the second commit should be rewritten as the new solution is to add a file based access control rule for set authorization checks. Also, I think the PR message and release notes will need a rewrite.

core/trino-main/src/main/java/io/trino/execution/SetViewAuthorizationTask.java

...trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedAccessControl.java

Using new rules will allow to control who can grant AUTHORIZATION (move ownership) to whom. In order to perform the SET AUTHORIZATION statement one has also ownership access to referred entity. So to be authorized to perform: ALTER TABLE T SET AUTHORIZATION USER U; one has to have access to modify the table T and proper authorization rule to move ownership to user U:

This property is no longer needed. It is required that plugin that implements access control make sure that control for SET AUTHORIZATION is implemented securely.

cla-bot bot added the cla-signed label Mar 23, 2023

kokosing requested review from ksobolew and dain March 23, 2023 11:34

github-actions bot added hive Hive connector tests:hive labels Mar 23, 2023

kokosing force-pushed the origin/master/177_alter_view_authorization branch from c4bb91a to 173f26e Compare March 23, 2023 13:47

ksobolew reviewed Mar 23, 2023

View reviewed changes

kokosing force-pushed the origin/master/177_alter_view_authorization branch from 173f26e to f4d4e3a Compare April 13, 2023 05:53

kokosing requested review from Laonel and lukasz-walkiewicz April 13, 2023 05:55

github-actions bot added the docs label Apr 13, 2023

ksobolew reviewed Apr 13, 2023

View reviewed changes

kokosing mentioned this pull request May 19, 2023

File based access control tests cleanup #17571

Merged

kokosing force-pushed the origin/master/177_alter_view_authorization branch from f4d4e3a to f71b287 Compare May 29, 2023 15:54

martint reviewed May 29, 2023

View reviewed changes

kokosing force-pushed the origin/master/177_alter_view_authorization branch from f71b287 to 94acb6e Compare May 30, 2023 13:29

dain reviewed May 30, 2023

View reviewed changes

core/trino-main/src/main/java/io/trino/execution/SetViewAuthorizationTask.java Show resolved Hide resolved

...trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedAccessControl.java Show resolved Hide resolved

kokosing force-pushed the origin/master/177_alter_view_authorization branch 4 times, most recently from 8edefc5 to 8083741 Compare June 4, 2023 20:55

kokosing changed the title ~~Require impersonation for VIEW SET AUTHORIZATION~~ Verify authorization access for SET AUTHORIZATION statements Jun 5, 2023

kokosing force-pushed the origin/master/177_alter_view_authorization branch 2 times, most recently from ae08d58 to bcd8574 Compare June 5, 2023 14:21

dain approved these changes Jun 5, 2023

View reviewed changes

kokosing force-pushed the origin/master/177_alter_view_authorization branch from bcd8574 to 1c98782 Compare June 6, 2023 14:28

kokosing added 2 commits June 6, 2023 21:41

Require admin privileges for SET AUTHORIZATION in SqlStandard

8ef6cfe

Remove legacy.allow-set-view-authorization

c04f7c2

This property is no longer needed. It is required that plugin that implements access control make sure that control for SET AUTHORIZATION is implemented securely.

kokosing force-pushed the origin/master/177_alter_view_authorization branch from 1c98782 to c04f7c2 Compare June 6, 2023 19:41

kokosing merged commit 0c928ac into trinodb:master Jun 7, 2023

kokosing deleted the origin/master/177_alter_view_authorization branch June 7, 2023 14:43

github-actions bot added this to the 420 milestone Jun 7, 2023

ksobolew mentioned this pull request Jun 19, 2023

Add support for Open Policy Agent #17940

Closed

5 tasks

colebow mentioned this pull request Jun 21, 2023

Add Trino 420 release notes #17997

Merged

kokosing mentioned this pull request Oct 22, 2024

Allow hive table owner to change ownership #23842

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify authorization access for SET AUTHORIZATION statements #16691

Verify authorization access for SET AUTHORIZATION statements #16691

kokosing commented Mar 23, 2023 •

edited

Loading

ksobolew left a comment

kokosing commented Apr 13, 2023

ksobolew left a comment

ksobolew Apr 13, 2023

kokosing May 29, 2023

ksobolew Apr 13, 2023

ksobolew Apr 13, 2023

kokosing May 29, 2023

kokosing commented May 29, 2023

martint May 29, 2023

dain left a comment

Verify authorization access for SET AUTHORIZATION statements #16691

Verify authorization access for SET AUTHORIZATION statements #16691

Conversation

kokosing commented Mar 23, 2023 • edited Loading

ksobolew left a comment

Choose a reason for hiding this comment

kokosing commented Apr 13, 2023

ksobolew left a comment

Choose a reason for hiding this comment

ksobolew Apr 13, 2023

Choose a reason for hiding this comment

kokosing May 29, 2023

Choose a reason for hiding this comment

ksobolew Apr 13, 2023

Choose a reason for hiding this comment

ksobolew Apr 13, 2023

Choose a reason for hiding this comment

kokosing May 29, 2023

Choose a reason for hiding this comment

kokosing commented May 29, 2023

martint May 29, 2023

Choose a reason for hiding this comment

dain left a comment

Choose a reason for hiding this comment

kokosing commented Mar 23, 2023 •

edited

Loading