Skip to content

Consolidate file-based authorization docs#10800

Merged
martint merged 1 commit intotrinodb:masterfrom
jhlodin:jl/file-authorization
Feb 3, 2022
Merged

Consolidate file-based authorization docs#10800
martint merged 1 commit intotrinodb:masterfrom
jhlodin:jl/file-authorization

Conversation

@jhlodin
Copy link
Copy Markdown
Contributor

@jhlodin jhlodin commented Jan 25, 2022

The hive-file-based-authorization section should live in /security/file-system-access-control now that it's used by the Iceberg connector as well. We should also discuss how system-level and catalog-level access control rules interact.

Copy link
Copy Markdown
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thing we found that duplication already existed, rather than migration the content into yet another file ;-)

@mosabua
Copy link
Copy Markdown
Member

mosabua commented Jan 26, 2022

Also special note @kokosing .. change as requested...

@hashhar
Copy link
Copy Markdown
Member

hashhar commented Jan 26, 2022

This is not a duplicate IIRC. Both are different mechanisms. Hive connector had it's own file based connector access control (as one of possible connector access controls as listed at https://trino.io/docs/current/connector/hive-security.html).

Trino provides access control both at the system and connector levels. The https://trino.io/docs/current/security/file-system-access-control.html is the former while the Hive specific one being changed here is the latter.

@jhlodin
Copy link
Copy Markdown
Contributor Author

jhlodin commented Jan 26, 2022

@hashhar Gotcha, thanks for the insight. Just to check though, now that the Iceberg connector also supports file-based access control is there any difference between the Hive file and the Iceberg file?

Also, what actually are the differences in syntax/support between the connector and system-level file? If the syntax is the similar or the same, it would be much simpler to have a single doc on how to create the access control file and reference as needed.

@hashhar
Copy link
Copy Markdown
Member

hashhar commented Jan 27, 2022

It's not similar at all. The connector level control allows things like column masking which the system level access control does not care about.

TL; DR: System level access control cares about what catalogs, schemas, tables and operations to allow. Connector level access control provides controls specific to the connector like being able to mask columns, expose some access mechanism present only for that connector.

IMO there is no reason to consolidate them and should be left as is.

As for whether Hive's file-based connector access control differs in any way from Iceberg's I'll defer to @findepi. But it seems Iceberg uses the system level security instead of the connector level one from Hive (from quick glance at code).

@findepi
Copy link
Copy Markdown
Member

findepi commented Jan 27, 2022

As for whether Hive's file-based connector access control differs in any way from Iceberg's I'll defer to @findepi. But it seems Iceberg uses the system level security instead of the connector level one from Hive (from quick glance at code).

@hashhar see #10493 & IcebergSecurityModule

@hashhar
Copy link
Copy Markdown
Member

hashhar commented Jan 27, 2022

Ah, so Iceberg's file-based security is exactly same as Hive's file-based security (which is different from system level file based access control).

@mosabua
Copy link
Copy Markdown
Member

mosabua commented Jan 27, 2022

So the idea is that we pull this out of the Hive connector docs into some common document for whatever connector implements that .. what do you suggest we call this?

File-based access control for connectors

and where do we put that file .. in security or in connectors? Personally I think this should be in the security section

@hashhar
Copy link
Copy Markdown
Member

hashhar commented Jan 27, 2022

So the idea is that we pull this out of the Hive connector docs into some common document for whatever connector implements that .. what do you suggest we call this?

file-based-connector-access-control (File Based Connector Access Control). Maybe the other should be rename to file-based-system-access-control (File Based Global Access Control)?

and where do we put that file .. in security or in connectors? Personally I think this should be in the security section

I'd argue for a security subpage for connectors that have ConnectorAccessControl implemented. Since these configs and docs (connector access control docs) would only apply to specific connectors and not all of them.

Why would any use end up using the connector specific one vs the global one? Why do we have these two implementations?

The names are unfortunate.
There are two levels of access control: system and connector.
For each of those there are multiple backends/providers - one of them is file based.

The ConnectorAccessControl SPI allows you to have different implementation for access controls per-connector. i.e. if someone wanted they could write an implementation that queries the Postgres system tables and applies access control accordingly for the PostgreSQL connector, maybe queries AWS IAM and applies access control policies for ElasticSearch on AWS, or the existing SQL Standard for Hive connector which consults the Hive metastore for policies.

The SystemAccessControl on the other hand applies more uniformly to the entire system as a whole.

@jhlodin
Copy link
Copy Markdown
Contributor Author

jhlodin commented Jan 27, 2022

Noted, I have an idea of how to rework this PR to consolidate & simplify while also keeping a distinction between system and connector-level file access control. Stay tuned.

@jhlodin jhlodin force-pushed the jl/file-authorization branch from e56b4c4 to b64fbe7 Compare January 28, 2022 17:16
@jhlodin jhlodin changed the title Remove duplicate hive file authorization section Consolidate file-based authorization docs Jan 28, 2022
@jhlodin jhlodin force-pushed the jl/file-authorization branch from b64fbe7 to 0eb1f95 Compare January 28, 2022 17:18
@jhlodin
Copy link
Copy Markdown
Contributor Author

jhlodin commented Jan 28, 2022

@hashhar Reworked this PR to consolidate information about file-based access control into the one doc under /security. Please take a look at this approach and see if it makes sense. I also left a couple comments on the file where we could use clarification.

@jhlodin jhlodin force-pushed the jl/file-authorization branch from 0eb1f95 to 9e8f1ac Compare January 31, 2022 21:58
@jhlodin jhlodin requested a review from electrum January 31, 2022 22:01
Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good to me % rule precedence.

Can we emphasize that both types of controls have different rule syntax/grammar/structure? Easy to assume both have same structure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC changes to older release notes don't reflect on the website unless explicitly deployed by someone? @martint would know.

Copy link
Copy Markdown
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any semantic changes?

@jhlodin jhlodin force-pushed the jl/file-authorization branch from 9e8f1ac to 295a226 Compare February 1, 2022 22:51
@jhlodin jhlodin force-pushed the jl/file-authorization branch from 295a226 to 7d462f5 Compare February 2, 2022 16:52
@jhlodin jhlodin requested a review from hashhar February 2, 2022 16:55
Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more comments. Looks good to go otherwise.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* ``filter_environment`` (optional): environment use during filter evaluation.
* ``filter_environment`` (optional): environment used during filter evaluation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link from here to "Filter and mask environment".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think a link is needed with the restructuring/rewording below

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add some text to clarify this is for the value of mask_environment and filter_environment.

@jhlodin jhlodin force-pushed the jl/file-authorization branch 2 times, most recently from 291b285 to 2c3c67d Compare February 3, 2022 20:28
@hashhar hashhar force-pushed the jl/file-authorization branch from fee1470 to 7e188c0 Compare February 3, 2022 20:38
Copy link
Copy Markdown
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. Lets ship this!

@martint martint merged commit ae782ef into trinodb:master Feb 3, 2022
@jhlodin jhlodin deleted the jl/file-authorization branch February 3, 2022 23:29
@github-actions github-actions bot added this to the 370 milestone Feb 4, 2022
@PengCool0904
Copy link
Copy Markdown

mask_environment and filter_environment.
The user specified for the execution environment of a specific table only declares which user is responsible for performing the filtering operation, rather than limiting the filtering operation to that user. Is that correct?

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

Development

Successfully merging this pull request may close these issues.

7 participants