Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Could you explain why Trino decided to always skip access control on inline and built-in functions..? #19912

Open
okayhooni opened this issue Nov 27, 2023 · 11 comments
Assignees

Comments

@okayhooni
Copy link
Contributor

okayhooni commented Nov 27, 2023

We currently use Apache Ranger for access control on Trino queries, and manage policies to control special privileges to use in-line functions such as some sensitive unhash-like functions. (I know, Ranger is not the officially supported plugin for Trino)

This access control on the in-line functions was working well until v412 (current version of our production cluster), but it's not working anymore on the latest release of Trino(v433) with some breaking changes on the redesigning of access control codes.

I found the reason on the code like below.

private static boolean canExecuteFunction(Session session, AccessControl accessControl, CatalogSchemaFunctionName functionName)
    {
        if (isInlineFunction(functionName) || isBuiltinFunctionName(functionName)) {
            return true;
        }
        return accessControl.canExecuteFunction(
                SecurityContext.of(session),
                new QualifiedObjectName(functionName.getCatalogName(), functionName.getSchemaName(), functionName.getFunctionName()));
    }

It's easy to fix these code lines on our forked repository of Trino,
but I wonder why Trino decided to remove access control on inline function at all.

How about adding option like access-control-on-inline-function-enabled (by default false) ..?
I found FeatureConfig can be injected to FunctionResolver through PlannerContext within LocalQueryRunner. (But it looks ugly to transfer configuration like this..)

@hashhar
Copy link
Member

hashhar commented Nov 27, 2023

@dain

@okayhooni
Copy link
Contributor Author

related PR: #19160

@hashhar
Copy link
Member

hashhar commented Sep 14, 2024

the reason why in-built functions are always allowed is because they are considered "safe". Can you explain which functions you want to disallow and why?

@lordicecream
Copy link

lordicecream commented Nov 18, 2024

@hashhar @dain I have deployed multiple UDFS, that I don't want to expose to everyone, but this issue is allowing all users to access all functions...the functions block is basically pointless if there are no checks being done...
Can you please let me know when this is expected to be patched and if there is any workaround?

Trino upgrades are basically blocked for me and my teams across all environments due to this vulnerability

@hashhar
Copy link
Member

hashhar commented Nov 20, 2024

but UDFs are not built-in functions and those should be controlled via access control. Do you observe otherwise?

Can you enable debug log for the io.trino.security package? And which access control plugin are you using? file-based?

@lordicecream
Copy link

Yes exactly, @hashhar I have around 5-10 UDFs and if I don't put any function rule block, then they should all be by default not accessible right?

I am seeing that those UDFs are not being controlled and everyone can access all functions including the (UDFS) even when I am not adding the block for functions rule.

Using file based access controller.

Will enable the debug logging and share logs.

@lordicecream
Copy link

@hashhar even debug logs didn't show anything useful...I am still not able to have control over who can access what function

@martint
Copy link
Member

martint commented Nov 26, 2024

but I wonder why Trino decided to remove access control on inline function at all.

Inline functions are provided by the user as part of the query, so it doesn't make much sense to check permissions based on their name -- the user could easily bypass the check by changing the function name.

Built in functions (in the synthetic system.builtin schema, are not checked because they are considered safe, as @hashhar indicated. It's worth noting that any functions provided by plugins using the legacy mechanisms (i.e., Plugin.getFunctions()) will be added to that global schema. You need to migrate your implementation to use the catalog function APIs (i.e., ConnectorMetadata.getFunctions(), ConnectorMetadata.getFunctionMetadata(), etc) for your functions to be loaded into the connector's catalog/schema and get access control checks.

@lordicecream
Copy link

@martint thank you for the explanation, I am unable to find any reference to your explanation in the Trino docs or in any release notes.

I also believe a critical point of access control is not just "safety" per se, but having control over who can use what and where for whatever usecase/scenario...due to certain users misfiring queries, we had removed user access to functions like distinct and some regex functions, not because distinct is unsafe, but such is the use case in that environment.

Not to mention the fact that now, after putting in significant time in the Trino upgrade process, we have to now pull in lateral dev teams and ask for changes in their UDFs without any documentation.

Can we have a flag that we can opt for, which can enable us to also control access over the in-line functions?

@lordicecream
Copy link

Hi @hashhar @martint please let me know your thoughts and if we can do something for the inline functions access control

@lordicecream
Copy link

@electrum

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants