Add table functions support to File-based access control #13713

huberty89 · 2022-08-17T10:33:00Z

Description

This PR adds File-based access control support for table functions.
Before this PR with File-based access control enabled every table functions was denied.

By default every rule kind (access to catalogs, schemas, tables) is allow when user does not specify any rule.
The same behavior is also here. After upgrading Trino to newer version and without making any changes to access rules, access to every table function will change and by default it will be allow. To restore previous behavior user needs to add:

{
    "functions": [
        {
            "function_kinds": [
                "SCALAR",
                "AGGREGATE",
                "WINDOW"
            ],
            "privileges": [
                "EXECUTE",
                "GRANT_EXECUTE"
            ]
        },
        {
            "function_kinds": [
                "TABLE"
            ],
            "privileges": []
        }
    ]
}

Examples System-level access control file

Allow postgresql.system.query for admin user.

{
    "functions": [
        {
            "function_kinds": [
                "SCALAR",
                "AGGREGATE",
                "WINDOW"
            ],
            "privileges": [
                "EXECUTE",
                "GRANT_EXECUTE"
            ]
        },
        {
            "user": "admin",
            "catalog": "postgresql",
            "schema": "system",
            "function_kinds": [
                "TABLE"
            ],
            "function": "query",
            "privileges": [
                "EXECUTE",
                "GRANT_EXECUTE"
            ]
        }
    ]
}

Allow query table function from every catalog & schema for admin user.

{
    "functions": [
        {
            "function_kinds": [
                "SCALAR",
                "AGGREGATE",
                "WINDOW"
            ],
            "privileges": [
                "EXECUTE",
                "GRANT_EXECUTE"
            ]
        },
        {
            "user": "admin",
            "function_kinds": [
                "TABLE"
            ],
            "function": "query",
            "privileges": [
                "EXECUTE",
                "GRANT_EXECUTE"
            ]
        }
    ]
}

Deny mysql.system.query for anyone.

{
    "functions": [
        {
            "function_kinds": [
                "SCALAR",
                "AGGREGATE",
                "WINDOW"
            ],
            "privileges": [
                "EXECUTE",
                "GRANT_EXECUTE"
            ]
        },
        {
            "catalog": "mysql",
            "schema": "system",
            "function_kinds": [
                "TABLE"
            ],
            "function": "query",
            "privileges": []
        }
    ]
}

Possible privileges values: EXECUTE and GRANT_EXECUTE - the second one allows to execute a function from SECURITY DEFINER view
Possible function_kinds values: SCALAR, AGGREGATE, WINDOW and TABLES, it's required to provide at least one value, also when user define a rule for AGGREGATE, SCALAR or WINDOW function, catalog and schema are disallowed

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
(x) Documentation PR is available with #14774.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# Section
* Add support for PTF in File-based access control. ({issue}`13713`)

cla-bot · 2022-08-17T10:33:02Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Hubert Łojek.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2022-08-17T10:37:54Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Hubert Łojek.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

kokosing

I think you can continue with testing.

looks good to me

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

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

kokosing · 2022-08-17T11:01:07Z

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

@@ -273,6 +284,7 @@ private static SystemAccessControl create(String configFileName)
                    .setTableRules(rules.getTableRules().orElse(ImmutableList.of(CatalogTableAccessControlRule.ALLOW_ALL)))
                    .setSessionPropertyRules(rules.getSessionPropertyRules().orElse(ImmutableList.of(SessionPropertyAccessControlRule.ALLOW_ALL)))
                    .setCatalogSessionPropertyRules(rules.getCatalogSessionPropertyRules().orElse(ImmutableList.of(CatalogSessionPropertyAccessControlRule.ALLOW_ALL)))
+                    .setTableFunctionRules(rules.getTableFunctionRules().orElse(ImmutableList.of(CatalogTableFunctionAccessControlRule.ALLOW_ALL)))


This is breaking change. Previously it was denied by default.

However I think it is safe to go this way as it is coherent with other access control checks here and so far I think nobody used file based access control to use table functions.

CC: @kasiafi @dain

ssheikin · 2022-08-29T10:32:52Z

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

@@ -953,7 +965,7 @@ public void checkCanExecuteFunction(SystemSecurityContext systemSecurityContext,
    @Override
    public void checkCanExecuteFunction(SystemSecurityContext systemSecurityContext, FunctionKind functionKind, CatalogSchemaRoutineName functionName)
    {
-        if (functionKind == TABLE) {
+        if (functionKind == TABLE && !checkTableFunctionPermission(systemSecurityContext, functionName)) {


May be that's good enough for table functions, but IIRC the eventual goal is to use CatalogSchemaRoutineName for functionName for other FunctionKinds too.

The proposed config syntax is too specific to table functions too:

{ "table_functions": [ { "user": "admin", "catalog": "postgresql", "schema": "system", "table_function": "query" } ] }

I think it worth to go with something like

{ "function": [ { "user": "admin", "function_kind": "TABLE", "catalog": "postgresql", "schema": "system", "function": "query" } ] }

I'd base code for functions on the code for tables, e.g.

@Override public void checkCanExecuteFunction(SystemSecurityContext systemSecurityContext, FunctionKind functionKind, CatalogSchemaRoutineName function) { // TODO config to disable for old function kinds AccessMode requiredCatalogAccess = FunctionKind.TABLE.equals(functionKind) ? ALL : READ_ONLY; if (!checkFunctionPermission(systemSecurityContext, function, requiredCatalogAccess, FunctionPrivilege.SELECT)) { denyExecuteFunction(function.toString()); } } private boolean checkFunctionPermission(SystemSecurityContext context, CatalogSchemaRoutineName function, AccessMode requiredCatalogAccess, FunctionPrivilege checkPrivilege) { if (!canAccessCatalog(context, function.getCatalogName(), requiredCatalogAccess)) { return false; } if (INFORMATION_SCHEMA_NAME.equals(function.getSchemaName())) { return true; } Identity identity = context.getIdentity(); for (CatalogFunctionAccessControlRule rule : functionRules) { if (rule.matches(identity.getUser(), identity.getEnabledRoles(), identity.getGroups(), function)) { return rule.getPrivileges().contains(checkPrivilege); } } return false; }

I think it's a valid concern. If the plan is to have other catalog/schema function kinds in the future then the current syntax would require backward incompatible change or keeping TABLE as the default function kind. Neither of which is desired.

@lukasz-walkiewicz I confirm that the discussed method checkCanExecuteFunction() will be used for all function kinds in the future.

@ssheikin, regarding this part:

AccessMode requiredCatalogAccess = FunctionKind.TABLE.equals(functionKind) ? ALL : READ_ONLY; if (!checkFunctionPermission(systemSecurityContext, function, requiredCatalogAccess, FunctionPrivilege.SELECT)) { denyExecuteFunction(function.toString()); }

requiring catalog access ALL is safe, but might be unnecessarily restrictive for some Table Functions (think: sequence generator, random generator etc. which do not access any data even for reading).

I also wonder how it fits the future model where all built-in functions are provided by the Global catalog. The built-in table functions should be easily available to anyone, without granting them that level of access to the catalog.

Ideally, the required catalog access should be set for each table function independently.

What does FunctionPrivilege.SELECT refer to?

Ugly initial draft based on file based access control of tables: #13903 . NOT to review. Only for sake of history.

@dain your suggestions would be very helpful here:

What catalog access should we require for table functions? How about the built-in functions provided by the Global catalog?

Could we possibly specify the required access per table function? Some functions read data (query pass-through), some might modify the catalog, and some don't touch anything (sequence).

I add support for every function kind

@kasiafi

Ideally, the required catalog access should be set for each table function independently.

Now I think this makes sense in case of UDFs as well.
I will take a look into it.

In theory user might prepare UDF which will make some modifications - this is such edge case probably we should not take care of this. Also we do not have an information which catalog would be modified in such UDF.

I guess it's time for that eventually #12544 (comment)

or at least

this is such edge case probably we should not take care of this

.... YET

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

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

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

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

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

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

ssheikin · 2022-09-06T08:50:31Z

This PR resolves #12833

ssheikin · 2022-09-07T08:14:06Z

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

    {
        this.schemaRules = schemaRules.orElse(ImmutableList.of(SchemaAccessControlRule.ALLOW_ALL));
        this.tableRules = tableRules.orElse(ImmutableList.of(TableAccessControlRule.ALLOW_ALL));
        this.sessionPropertyRules = sessionPropertyRules.orElse(ImmutableList.of(SessionPropertyAccessControlRule.ALLOW_ALL));
+        this.functionRules = functionRules.orElse(ImmutableList.of(FunctionAccessControlRule.ALLOW_ALL));


Before this PR with File-based access control enabled every table functions was denied.

By default every rule kind (access to catalogs, schemas, tables) is allow when user does not specify any rule.
The same behavior is also here. After upgrading Trino to newer version and without making any changes to access rules, access to every table function will change and by default it will be allow. Is that good behavior? Maybe in case of table functions new default should be deny but this makes those new rules inconsistent with the rest.

Let's discuss this in the separate thread.

Before this PR with File-based access control enabled every table functions was denied.

I think it's a correct implementation, fast one, but which was not really thought that deep to be very compatible with FBAC.

Maybe in case of table functions new default should be deny but this makes those new rules inconsistent with the rest.

I guess access to ptf should obey the same rules as other entities of FBAC do.

By default every rule kind (access to catalogs, schemas, tables) is allow when user does not specify any rule.
The same behavior is also here. After upgrading Trino to newer version and without making any changes to access rules, access to every table function will change and by default it will be allow. Is that good behavior?

ptf were just introduced and with only one built in ptf.Query function. It's questionable if there are cases where FBAC with PTF is used by someone.

Consistency it's a long term thing.

ksobolew · 2022-09-20T14:16:01Z

I finally got to reviewing this PR ;) As I understand it, it's blocked on adding additional methods to the access control interfaces, right?

Also, tests will be needed.

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

huberty89 · 2022-10-14T14:03:02Z

@dain @findepi @kasiafi Would you like to take a look?

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

...trino-plugin-toolkit/src/test/resources/file-based-disallow-function-rule-combination-2.json

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

huberty89 · 2022-10-20T10:32:00Z

I added more real case scenario test TestViewsWithPtfFileBasedSystemAccessControl

ssheikin · 2022-10-20T11:49:51Z

...rino-tests/src/test/java/io/trino/security/TestViewsWithPtfFileBasedSystemAccessControl.java

+    @Test
+    public void testPtfViewsCreatedByAlice()
+    {
+        QueryError queryError = executeAsUser("CREATE VIEW blackhole.default.view_ptf_alice_security_definer SECURITY DEFINER AS SELECT * FROM TABLE(mock.system.simple_table_function())", ALICE_USER);


inline everywhere except where it is reused.

...rino-tests/src/test/java/io/trino/security/TestViewsWithPtfFileBasedSystemAccessControl.java

lukasz-walkiewicz

LGTM

lukasz-walkiewicz · 2022-10-20T15:52:02Z

...ts/src/test/java/io/trino/security/TestFunctionsInViewsWithFileBasedSystemAccessControl.java

+        assertQuerySucceeds(BOB_USER, "CREATE VIEW blackhole.default.view_ptf_bob_security_definer SECURITY DEFINER AS SELECT * FROM TABLE(mock.system.simple_table_function())");
+        String securityDefinerQuery = "SELECT * FROM blackhole.default.view_ptf_bob_security_definer";
+        assertAccessDenied(ALICE_USER, securityDefinerQuery, "View owner does not have sufficient privileges: 'bob' cannot grant 'mock\\.system\\.simple_table_function' execution to user 'alice'");
+        assertQuerySucceeds(BOB_USER, securityDefinerQuery);


That's an interesting behaviour, that Bob himself can actually query the view but no one else can. It's probably because view owner = view invoker.

io.trino.sql.analyzer.StatementAnalyzer.Visitor#analyzeView

ssheikin · 2022-10-23T07:11:44Z

🎉

huberty89 force-pushed the fbac-table-function-support branch from 00a16fb to 710d83f Compare August 17, 2022 10:37

huberty89 force-pushed the fbac-table-function-support branch from 710d83f to 67de34c Compare August 17, 2022 10:42

cla-bot bot added the cla-signed label Aug 17, 2022

huberty89 requested review from findepi, kasiafi, dain and kokosing August 17, 2022 10:43

kokosing reviewed Aug 17, 2022

View reviewed changes

kokosing mentioned this pull request Aug 19, 2022

Remove deprecated Elasticsearch passthrough query #13390

Merged

ssheikin suggested changes Aug 29, 2022

View reviewed changes

ssheikin reviewed Aug 29, 2022

View reviewed changes

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

huberty89 force-pushed the fbac-table-function-support branch from 67de34c to 2cd9937 Compare September 1, 2022 11:07

ssheikin suggested changes Sep 1, 2022

View reviewed changes

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

ssheikin reviewed Sep 1, 2022

View reviewed changes

huberty89 force-pushed the fbac-table-function-support branch from 2cd9937 to a39b483 Compare September 1, 2022 14:07

huberty89 force-pushed the fbac-table-function-support branch 2 times, most recently from 1a2553b to a47bfe9 Compare September 6, 2022 19:57

ssheikin reviewed Sep 7, 2022

View reviewed changes

huberty89 requested review from ksobolew and andrewdibiasio6 September 7, 2022 12:59

huberty89 mentioned this pull request Sep 19, 2022

Add checkCanGrantExecuteFunctionPrivilege to ConnectorAccessControl #14191

Merged

huberty89 force-pushed the fbac-table-function-support branch 2 times, most recently from dc3ff73 to 8528641 Compare October 12, 2022 12:23

huberty89 commented Oct 12, 2022

View reviewed changes

...in-toolkit/src/main/java/io/trino/plugin/base/security/CatalogFunctionAccessControlRule.java Outdated Show resolved Hide resolved

huberty89 force-pushed the fbac-table-function-support branch from 8528641 to c4059fd Compare October 14, 2022 13:45

ssheikin reviewed Oct 19, 2022

View reviewed changes

...no-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FunctionAccessControlRule.java Outdated Show resolved Hide resolved

huberty89 force-pushed the fbac-table-function-support branch 2 times, most recently from 42f0289 to fb19a4d Compare October 19, 2022 15:15

ssheikin reviewed Oct 19, 2022

View reviewed changes

...no-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FunctionAccessControlRule.java Outdated Show resolved Hide resolved

ssheikin reviewed Oct 19, 2022

View reviewed changes

...no-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FunctionAccessControlRule.java Outdated Show resolved Hide resolved

ssheikin reviewed Oct 19, 2022

View reviewed changes

...trino-plugin-toolkit/src/test/resources/file-based-disallow-function-rule-combination-2.json Outdated Show resolved Hide resolved

ssheikin approved these changes Oct 19, 2022

View reviewed changes

lukasz-walkiewicz reviewed Oct 20, 2022

View reviewed changes

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

...in-toolkit/src/main/java/io/trino/plugin/base/security/CatalogFunctionAccessControlRule.java Outdated Show resolved Hide resolved

huberty89 force-pushed the fbac-table-function-support branch 3 times, most recently from 067f379 to 36ac692 Compare October 20, 2022 10:28

huberty89 force-pushed the fbac-table-function-support branch from 36ac692 to 2d7a769 Compare October 20, 2022 11:00

ssheikin reviewed Oct 20, 2022

View reviewed changes

huberty89 force-pushed the fbac-table-function-support branch 4 times, most recently from 380877c to ce723a3 Compare October 20, 2022 13:31

ssheikin approved these changes Oct 20, 2022

View reviewed changes

...rino-tests/src/test/java/io/trino/security/TestViewsWithPtfFileBasedSystemAccessControl.java Outdated Show resolved Hide resolved

huberty89 force-pushed the fbac-table-function-support branch from ce723a3 to 5e5e563 Compare October 20, 2022 14:09

Add functions support to File-based access control

4db3ca7

huberty89 force-pushed the fbac-table-function-support branch from 5e5e563 to 4db3ca7 Compare October 20, 2022 14:48

lukasz-walkiewicz approved these changes Oct 20, 2022

View reviewed changes

kokosing approved these changes Oct 22, 2022

View reviewed changes

kokosing merged commit 96d3e52 into trinodb:master Oct 22, 2022

github-actions bot added this to the 401 milestone Oct 22, 2022

colebow mentioned this pull request Oct 25, 2022

Add Trino 401 release notes #14675

Merged

huberty89 mentioned this pull request Oct 26, 2022

Document function rules for File-based Access Control #14774

Merged

kokosing mentioned this pull request Sep 29, 2023

Unable to Hide Schema Metadata from User Despite Applying Access Limitations #18370

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add table functions support to File-based access control #13713

Add table functions support to File-based access control #13713

huberty89 commented Aug 17, 2022 •

edited

Loading

cla-bot bot commented Aug 17, 2022

cla-bot bot commented Aug 17, 2022

kokosing left a comment

kokosing Aug 17, 2022

ssheikin Aug 29, 2022

lukasz-walkiewicz Aug 29, 2022

kasiafi Aug 29, 2022

ssheikin Aug 29, 2022

kasiafi Aug 29, 2022

huberty89 Sep 6, 2022

huberty89 Sep 7, 2022

huberty89 Sep 7, 2022 •

edited

Loading

ssheikin Sep 7, 2022

ssheikin Sep 7, 2022

ssheikin commented Sep 6, 2022

ssheikin Sep 7, 2022

ksobolew commented Sep 20, 2022

huberty89 commented Oct 14, 2022

huberty89 commented Oct 20, 2022

ssheikin Oct 20, 2022

lukasz-walkiewicz left a comment

lukasz-walkiewicz Oct 20, 2022

lukasz-walkiewicz Oct 20, 2022

ssheikin commented Oct 23, 2022

Add table functions support to File-based access control #13713

Add table functions support to File-based access control #13713

Conversation

huberty89 commented Aug 17, 2022 • edited Loading

Description

Documentation

Release notes

cla-bot bot commented Aug 17, 2022

cla-bot bot commented Aug 17, 2022

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huberty89 Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssheikin commented Sep 6, 2022

Choose a reason for hiding this comment

ksobolew commented Sep 20, 2022

huberty89 commented Oct 14, 2022

huberty89 commented Oct 20, 2022

Choose a reason for hiding this comment

lukasz-walkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssheikin commented Oct 23, 2022

huberty89 commented Aug 17, 2022 •

edited

Loading

huberty89 Sep 7, 2022 •

edited

Loading