Add access control for functions based on FunctionKind by kasiafi · Pull Request #12544 · trinodb/trino

kasiafi · 2022-05-25T14:54:00Z

This is security improvement. It allows to apply different default access rules for functions based on function kind.
This is particularly useful when we add table functions, which are more powerful that other function kinds.

Should be merged before we add table functions:
#12325
#12324
#12502

This is a SPI change:
new ConnectorAccessControl method is added, and existing method is removed. An enum representing the function kind is introduced.

# Security
* Add access control for functions based on function kind.

cc @mosabua @colebow for release notes

core/trino-main/src/main/java/io/trino/metadata/FunctionKind.java

kokosing · 2022-05-26T19:48:57Z

core/trino-main/src/main/java/io/trino/security/AccessControl.java

This method was not used so far. Please simply remove it. The best would be to remove the other method too (the one with function name as String)

@dain the old method was introduced a short while ago. It was only called for table function invocation analysis, but we don't have any table functions yet. I removed it in a separate commit. If you're OK with removing, I'll squash.

@kokosing @kasiafi Do I understand correctly that the newly introduced method

void checkCanExecuteFunction(SecurityContext context, FunctionKind functionKind, QualifiedObjectName functionName);

should eventually replace the old one

void checkCanExecuteFunction(SecurityContext context, String functionName);

, so that all old functionality for AccessControls remains the same for all FunctionKinds which are not FunctionKind.TABLE?

eventually -- yes, i think so

note however that currently function names are not qualified yet.

core/trino-main/src/main/java/io/trino/security/AccessControl.java

core/trino-main/src/main/java/io/trino/security/AccessControlManager.java

core/trino-main/src/main/java/io/trino/security/InjectedConnectorAccessControl.java

core/trino-spi/src/main/java/io/trino/spi/security/FunctionType.java

dain

Good stuff! I have two main concerns that we should discuss:

I have a PR coming up that will move FunctionKind to the SPI, so we should talk about maybe just moving that one class now, so we don't end up with two versions in the future.
I'm not sure how to handle the check for table functions in the Hive SQL standard impelemtnation. I'm leaning towards isAdmin, but we could just deny.

core/trino-main/src/main/java/io/trino/security/AccessControlManager.java

core/trino-main/src/main/java/io/trino/security/InjectedConnectorAccessControl.java

dain · 2022-05-26T20:49:56Z

core/trino-spi/src/main/java/io/trino/spi/security/FunctionType.java

In the next section of my function refactor, I move function framework from the engine to the SPI. As part of that, I move FunctionKind to io.trino.spi.connector.function. I suggest that we just move (or clone) this class to the new location now... assuming are all happy with my proposed target location.

I moved FunctionKind to io.trino.spi.function. Or did you mean I should create a new package function under connector?

dain · 2022-05-26T20:51:39Z

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

This should be deny for table functions until file-based-rules are added that cover functions

dain · 2022-05-26T20:52:41Z

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

This should be deny for table functions until file-based-rules are added that cover functions

dain · 2022-05-26T20:54:47Z

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

This should be deny for table functions. Later when someone adds a table function for Hive we can discuss how to handle this.

dain · 2022-05-26T20:57:36Z

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

I'm not sure what the behavior should be here. My gut instinct it to restrict this to isAdmin(context) or just deny. I think that is admin is a reasonable first step. Also, I don't think Hive has any table functions right now, so even flat deny should be fine until someone adds one.

restricted to admin

kokosing

I would squash deprecation and method removal into single commit

This change adds an AccessControl method for functions using the function kind. The analogous method without the FunctionKind argument is removed. Access control based on function kind is needed because table functions are much more powerful than other function kinds, so it should be possible to easily deny executing all table functions while allowing all other functions. The new method is currently used only for table funcitons. This change also adds default implementations of function access control in SystemAccessControl and ConnectorAccessControl that deny executing table functions.

colebow · 2022-06-01T14:56:44Z

Do we need to document this yet? Or should we just wait for table functions?

Incorporate following changes Introduce access control for functions based on FunctionKind trinodb/trino#12544 trinodb/trino@12dda62

kasiafi · 2022-06-27T10:51:00Z

Do we need to document this yet? Or should we just wait for table functions?

@colebow we should document it, as it is currently used for table functions.
The new method will be eventually used for all kinds of functions (see: #12544 (comment)), but it requires refactoring the way that functions are registered and resolved.

cla-bot bot added the cla-signed label May 25, 2022

github-actions bot added the tests:hive label May 25, 2022

kasiafi force-pushed the 355PTFAccessControl branch from 0d9a4ed to ba946f5 Compare May 25, 2022 18:19

kasiafi mentioned this pull request May 26, 2022

Support query pass-through for JDBC-based connectors #12325

Merged

kasiafi force-pushed the 355PTFAccessControl branch from ba946f5 to 145284d Compare May 26, 2022 11:31

kasiafi requested review from dain and kokosing May 26, 2022 12:16

kokosing reviewed May 26, 2022

View reviewed changes

dain reviewed May 26, 2022

View reviewed changes

kasiafi force-pushed the 355PTFAccessControl branch 2 times, most recently from c1de7f9 to 7908a08 Compare May 27, 2022 09:12

kasiafi requested review from dain and kokosing May 27, 2022 12:08

kokosing approved these changes May 27, 2022

View reviewed changes

kasiafi added 4 commits May 28, 2022 10:32

Remove unused argument

759a036

Introduce new FuntionKind: TABLE

55a1d6d

Move FunctionKind to SPI

dbad1b5

kasiafi force-pushed the 355PTFAccessControl branch from 7908a08 to 011687f Compare May 28, 2022 08:42

kokosing merged commit 12dda62 into trinodb:master May 31, 2022

github-actions bot added this to the 383 milestone May 31, 2022

colebow mentioned this pull request Jun 1, 2022

Add Trino 383 release notes #12565

Merged

utk-spartan added a commit to razorpay/ranger that referenced this pull request Jun 2, 2022

[trino] Update spi to v383

384dfba

Incorporate following changes Introduce access control for functions based on FunctionKind trinodb/trino#12544 trinodb/trino@12dda62

kasiafi mentioned this pull request Jun 14, 2022

Support file based access control for table functions #12833

Open

ssheikin mentioned this pull request Sep 7, 2022

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

Merged

aakashnand mentioned this pull request Sep 8, 2022

Add support for table functions to trino ranger plugin aakashnand/trino-ranger-demo#2

Open

Conversation

kasiafi commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokosing left a comment

Choose a reason for hiding this comment

Uh oh!

colebow commented Jun 1, 2022

Uh oh!

kasiafi commented Jun 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

kasiafi commented May 25, 2022 •

edited

Loading