Do not pass default properties to access control checks

[kokosing]

Do not pass default properties to access control checks

[findepi]

lowercase here when invoking contains

[kokosing]

It is already lower cased in io.trino.metadata.PropertyUtil#evaluateProperties

[findepi]

Can this class depend on that? maybe it should not, so that the code is "more obviously correct".
i.e. consisten with lowercase 3 lines up

[kokosing]

Also io.trino.spi.session.PropertyMetadata#PropertyMetadata disallows to create a property with non lower cased property name.

[findepi]

Also io.trino.spi.session.PropertyMetadata#PropertyMetadata disallows to create a property with non lower cased property name.

today.

if you rely on that other class guarantees, add a verify here (.peek(...))

[kokosing]

Addressed as fixup

[martint]

What’s the motivation for this change? If the values of default properties are not passed, how would the access control know what value they are supposed to take?

[kokosing]

I am sorry, I was under impression that the change is straightforward so I gave very limited commit message.

The motivation is to make access control for table properties to be more practical and coherent with access for other objects.

If the values of default properties are not passed, how would the access control know what value they are supposed to take?

In my opinion access control does not need to know or control what engine is doing in order to fulfill user requests, but it is more about controlling what users are doing.

As example consider the below (that may happen with existing implementation):

CREATE TABLE t(i int);
ERROR: Access denied: cannot set location to 's3://bucket/data/schema/t'

In such situation, I believe users don't care about location of where data is stored. They may not even know that used catalog is a Hive connector. They just want to create a table. They have CREATE TABLE privilege and they ask engine to do whatever it needs, but please create a table for me. User didn't ask for any specific location.

Few more issues with existing implementation:

• authorization services (like Ranger or file based ones) typically deny everything by default. In such case, if they would like provide access control for table properties, then admins need to control each table property. Even the hidden ones and ones that are even mentioned in any query by users of the system.
• "Ok, but I can grant access to all table properties to bypass the above". Yes but if you would like to deny certain usage of table properties then you need to still enumerate all other table properties to grant access to them so user can CREATE TABLE.
• Considering the above, whenever we add new properties admins will be forced to update they authorization policies to upgrade Trino.
• Hive has 21 table properties now, it if access control requires network communication for each of resource access then it may require some time to do so.
• if access control is tracking audit access log, then simple CREATE TABLE for Hive connector will produce 21 additional events. Which could be surprising because as nobody really used these table properties.
• it leaks hidden table properties to access control (and possibly to audit services)

The logic behind proposed implementation:

• if admin creates a catalog with certain defaults then they say it is ok to CREATE TABLES using these defaults. So anybody who has CREATE TABLE privilege can create them. However, admins can simply disallow to overriding defaults or control which table properties are allowed to be modified by users.
• it does not expose mentioned issues with existing implementation for access control of table properties
• proposed implementation is coherent with how we control session properties. We call access control only when somebody requests SET SESSION..., not when we populate a session.

[kokosing]

Updated tests a bit, to make sure not used properties do not leak.

@martint @findepi Any more comments?

[colebow]

Does this need a release note? @kokosing

[kokosing]

I don't think so.