Add support for row filtering and column masking access control #24277

BryanCutler · 2024-12-18T19:07:03Z

Description

This adds support for row filtering and column masking to access control as part of governance requirements. Filters/masks are supplied as part of an access control implementation and then applied to queries according to matching table and column names supplied by access control.

These changes are cherry-picked from the following commits:

Adding support for row filters
Cherry-pick of trinodb/trino@fae3147
Co-authored-by: Martin Traverso mtraverso@gmail.com

Adding support for column masks
Cherry-pick of trinodb/trino@7e0d88e
Co-authored-by: Martin Traverso mtraverso@gmail.com

Disallow multiple masks on a given column
Cherry-pick of trinodb/trino@bdd1cb5
Co-authored-by: Martin Traverso mtraverso@gmail.com

Add access control SPI function for table column masks.
Cherry-pick of trinodb/trino@5da64a9
Co-authored-by: Cristian Osiac mosiac@bloomberg.net

Update core access control to fetch column masks in bulk
Cherry-pick of trinodb/trino@1dbbcb3
Co-authored-by: Cristian Osiac mosiac@bloomberg.net

For original IBM Presto port
Co-authored-by: Reetika Agrawal agrawal.reetika786@gmail.com

From #24278

Motivation and Context

Governance requirements include the ability to restrict access to certain rows of tables or sensitive data in certain columns. This adds the ability to provide filters/masks as expressions to Presto where they will then be applied to relevant queries.

Impact

Additions to SPI include access control interfaces to get row filters and column masks.

ConnectorAccessControl

    /**
     * Get row filters associated with the given table and identity.
     * <p>
     * Each filter must be a scalar SQL expression of boolean type over the columns in the table.
     *
     * @return the list of filters, or empty list if not applicable
     */
    default List<ViewExpression> getRowFilters(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName)
    {
        return Collections.emptyList();
    }

    /**
     * Bulk method for getting column masks for a subset of columns in a table.
     * <p>
     * Each mask must be a scalar SQL expression of a type coercible to the type of the column being masked. The expression
     * must be written in terms of columns in the table.
     *
     * @return a mapping from columns to masks, or an empty map if not applicable. The keys of the return Map are a subset of {@code columns}.
     */
    default Map<ColumnMetadata, ViewExpression> getColumnMasks(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName, List<ColumnMetadata> columns)
    {
        return Collections.emptyMap();
    }

Test Plan

Unit tests added.

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Security Changes
* Add support for row filtering and column masking in access control.

BryanCutler · 2024-12-18T19:26:11Z

There are some followup commits that I did not include here, we can add these in or look into as a followup:

Apply filters and masks for view object
trinodb/trino@6721cfa

Apply filters and masks for tables referenced by views
trinodb/trino@84429d8

Add test for join on masked column
trinodb/trino@705ed21

Record filter and mask information in query event
trinodb/trino@1b55b86

jaystarshot · 2024-12-18T22:15:45Z

@tdcmeehan Its high time that we consider merging this stuff, as we discussed last time in the meeting doing in connector optimizer is not feasible for all cases and offerrs no benefit

tdcmeehan · 2024-12-20T20:12:54Z

I'm in favor of this approach. While it does increase our SPI footprint, I think it's nice to align with Trino's approach in this case because it makes porting plugins easier for folks. I think this is worth the small increase in footprint.

BryanCutler · 2025-01-07T22:57:05Z

Updated to include followup commits to retrieve a list of row filters and multiple column masks with a single call to the interface.

uarturm · 2025-01-10T01:09:10Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

+        {
+            String column = columnMetadata.getName();
+            if (analysis.hasColumnMask(tableName, column, currentIdentity)) {
+                throw new PrestoException(INVALID_ROW_FILTER, format("Column mask for '%s.%s' is recursive", tableName, column), null);


nit: Can you please use INVALID_COLUMN_MASK error code in this method?

uarturm · 2025-01-10T01:24:20Z

presto-main/src/main/java/com/facebook/presto/sql/planner/RelationPlanner.java

+
+                planBuilder = subqueryPlanner.handleSubqueries(planBuilder, mask, mask, context);
+
+                Map<VariableReferenceExpression, RowExpression> assignments = new LinkedHashMap<>();


Can you please move the creation of assignments and the new planBuilder outside the for loop? This will allow invoking planBuilder.withNewRoot only once per method invocation, rather than creating a new planBuilder for each mask. It will improve efficiency and make the logic cleaner.

Yeah, that sounds better.

I think I got it working, but still new working with these parts so let me know if it can be improved.

uarturm · 2025-01-10T01:33:10Z

presto-main/src/main/java/com/facebook/presto/sql/planner/RelationPlanner.java

+        Map<String, Expression> columnMasks = analysis.getColumnMasks(table);
+
+        List<VariableReferenceExpression> mappings = plan.getFieldMappings();
+        TranslationMap translations = new TranslationMap(plan, analysis, lambdaDeclarationToVariableMap);


nit: We can simplify this section by reusing some code. Instead of directly using translations.rewrite(mask), you could initialize the PlanBuilder with PlanBuilder planBuilder = initializePlanBuilder(plan);. Then, use planBuilder.rewrite(mask) to leverage the translations map already stored internally within planBuilder.

BryanCutler · 2025-02-18T19:45:32Z

Thanks for reviewing @uarturm , there is still some discussion on the RFC, then I'll remove from draft.

I am debating including the cherry pick of trinodb/trino@6721cfa with this. I don't want to deviate too much from the initial design, but this could be useful as well. What are you thoughts on it? cc @tdcmeehan @aaneja

BryanCutler · 2025-03-13T23:04:52Z

The test failure seems to be sporadic, or maybe from some state not getting reset. I'll have to look into it further.

BryanCutler · 2025-04-17T23:45:31Z

presto-docs/src/main/sphinx/develop/system-access-control.rst

+column names. The returned filters and masks will be in the form of a ``ViewExpression``
+that is then applied to the query plan before execution. Filters and masks can also be
+supplied at the connector level from a ``ConnectorAccessControl`` implementation.
+


@steveburnett please take a look at the doc additions, thanks!

steveburnett

LGTM! (docs)

Pull branch, local doc build, looks good. Thanks!

BryanCutler · 2025-04-22T17:59:14Z

@jaystarshot is this something you are available to review and help merge?

Cherry-pick of trinodb/trino@fae3147 This adds support for getting row filters from access control and applying them to a query plan. Co-authored-by: Martin Traverso <mtraverso@gmail.com>

Cherry-pick of trinodb/trino@7e0d88e This adds support for getting column masks from access control and applying them to a query plan. With additional new tests for plan validation of row filters and column masks. Co-authored-by: Martin Traverso <mtraverso@gmail.com>

Cherry-pick of trino trinodb/trino@bdd1cb5 Co-authored-by: Martin Traverso <mtraverso@gmail.com>

Cherry-pick of trinodb/trino@827de57 Cherry-pick of trinodb/trino@ae66a8b Allow returning multiple filters and masks in ConnectorAccessControl and SystemAccessControl. Co-authored-by: Krzysztof Sobolewski <krzysztof.sobolewski@starburstdata.com>

ZacBlanco

Great improvement, just a few nits and a couple tests for complex types would be great

presto-main-base/src/main/java/com/facebook/presto/security/FileBasedSystemAccessControl.java

presto-main-base/src/main/java/com/facebook/presto/security/AllowAllSystemAccessControl.java

presto-main-base/src/main/java/com/facebook/presto/security/FileBasedSystemAccessControl.java

presto-main-base/src/test/java/com/facebook/presto/security/TestAccessControlManager.java

presto-main-base/src/test/java/com/facebook/presto/sql/query/TestColumnMask.java

ZacBlanco · 2025-04-24T21:53:03Z

presto-main-base/src/test/java/com/facebook/presto/sql/query/TestColumnMask.java

+            assertions.assertFails("DELETE FROM orders", "\\Qline 1:1: Delete from table with column mask is not supported\\E");
+        });
+    }
+}


Can we also do a few tests with complex/nested types such as ROW and ARRAY types

I suppose it would be good to make sure a ViewExpression can apply to a complex type. It would take me some time to work this out, would that be ok to work on as a followup?

Can you at least open a ticket and link it to this discussion so we don't lose track of this?

Issue created: #24998

presto-main-base/src/test/java/com/facebook/presto/sql/query/TestRowFilter.java

...n-toolkit/src/main/java/com/facebook/presto/plugin/base/security/FileBasedAccessControl.java

Cherry-pick of trinodb/trino@1dbbcb3 With related commit: Add access control SPI function for table column masks Cherry-pick of trinodb/trino@5da64a9 Co-authored-by: Cristian Osiac <mosiac@bloomberg.net>

This improves plan rewrite for column masks to only build a new root node once, instead of creating a plan builder for each mask. Add documentation for row filters and column masks access control implementation.

ZacBlanco

LGTM, just open the ticket for complex types first

ZacBlanco · 2025-04-28T15:37:55Z

presto-main-base/src/test/java/com/facebook/presto/sql/query/TestColumnMask.java

+            assertions.assertFails("DELETE FROM orders", "\\Qline 1:1: Delete from table with column mask is not supported\\E");
+        });
+    }
+}


Can you at least open a ticket and link it to this discussion so we don't lose track of this?

presto-main-base/src/test/java/com/facebook/presto/sql/query/TestColumnMask.java

prestodb-ci added the from:IBM PR from IBM label Dec 18, 2024

prestodb-ci requested review from a team, imjalpreet and wanglinsong and removed request for a team December 18, 2024 19:07

BryanCutler mentioned this pull request Dec 18, 2024

Add support for row filtering and column masking access control #24278

Closed

BryanCutler force-pushed the qaag-forward-fit-trino branch 2 times, most recently from 0c28887 to 3d1ecda Compare December 19, 2024 19:35

BryanCutler force-pushed the qaag-forward-fit-trino branch from 3d1ecda to 1aefdf0 Compare December 20, 2024 20:23

agrawalreetika mentioned this pull request Dec 21, 2024

Add support for row filtering #16955

Closed

aaneja self-requested a review December 23, 2024 05:53

uarturm reviewed Jan 10, 2025

View reviewed changes

BryanCutler force-pushed the qaag-forward-fit-trino branch from 74b9a9f to f1039f8 Compare February 18, 2025 19:42

BryanCutler force-pushed the qaag-forward-fit-trino branch 2 times, most recently from 3321f0f to 83db787 Compare March 7, 2025 22:59

BryanCutler marked this pull request as ready for review March 8, 2025 00:55

BryanCutler requested review from jaystarshot, feilong-liu, ClarenceThreepwood and a team as code owners March 8, 2025 00:55

BryanCutler requested a review from presto-oss March 8, 2025 00:55

prestodb-ci requested a review from a team March 8, 2025 00:55

ScrapCodes self-requested a review March 13, 2025 15:26

BryanCutler force-pushed the qaag-forward-fit-trino branch from 83db787 to 1ebad17 Compare March 13, 2025 16:57

BryanCutler dismissed aaneja’s stale review via b8b256f April 17, 2025 23:41

BryanCutler force-pushed the qaag-forward-fit-trino branch from 987a1b5 to b8b256f Compare April 17, 2025 23:41

BryanCutler requested review from steveburnett and elharo as code owners April 17, 2025 23:41

BryanCutler commented Apr 17, 2025

View reviewed changes

steveburnett previously approved these changes Apr 18, 2025

View reviewed changes

BryanCutler force-pushed the qaag-forward-fit-trino branch from b8b256f to 76f2b2b Compare April 18, 2025 18:19

BryanCutler requested a review from aaneja April 21, 2025 17:04

aaneja previously approved these changes Apr 22, 2025

View reviewed changes

BryanCutler force-pushed the qaag-forward-fit-trino branch from 76f2b2b to d95968f Compare April 24, 2025 18:21

BryanCutler and others added 4 commits April 24, 2025 11:24

Add support for row filtering

77929e7

Cherry-pick of trinodb/trino@fae3147 This adds support for getting row filters from access control and applying them to a query plan. Co-authored-by: Martin Traverso <mtraverso@gmail.com>

Disallow multiple masks on a given column

28d2cd6

Cherry-pick of trino trinodb/trino@bdd1cb5 Co-authored-by: Martin Traverso <mtraverso@gmail.com>

BryanCutler force-pushed the qaag-forward-fit-trino branch from d95968f to f043bdc Compare April 24, 2025 18:34

ZacBlanco requested changes Apr 24, 2025

View reviewed changes

BryanCutler and others added 2 commits April 25, 2025 13:51

Fetch column masks in bulk

b6d88ef

Cherry-pick of trinodb/trino@1dbbcb3 With related commit: Add access control SPI function for table column masks Cherry-pick of trinodb/trino@5da64a9 Co-authored-by: Cristian Osiac <mosiac@bloomberg.net>

Improve addColumnMasks efficiency

5af314f

This improves plan rewrite for column masks to only build a new root node once, instead of creating a plan builder for each mask. Add documentation for row filters and column masks access control implementation.

BryanCutler dismissed stale reviews from aaneja and steveburnett via 5af314f April 25, 2025 20:51

BryanCutler force-pushed the qaag-forward-fit-trino branch from f043bdc to 5af314f Compare April 25, 2025 20:52

ZacBlanco reviewed Apr 28, 2025

View reviewed changes

BryanCutler mentioned this pull request Apr 28, 2025

Add tests for row filtering and column masking for complex types #24998

Open

ZacBlanco approved these changes Apr 28, 2025

View reviewed changes

BryanCutler merged commit 1abad21 into prestodb:master Apr 28, 2025
99 checks passed

BryanCutler deleted the qaag-forward-fit-trino branch April 28, 2025 20:29

ZacBlanco mentioned this pull request May 29, 2025

Add release notes for 0.293 #25222

Merged

21 tasks

This was referenced May 30, 2025

Add release notes for 0.293 unix280/presto#31

Closed

Add release notes for 0.293 unix280/presto#32

Closed


		planBuilder = subqueryPlanner.handleSubqueries(planBuilder, mask, mask, context);

		Map<VariableReferenceExpression, RowExpression> assignments = new LinkedHashMap<>();

Add support for row filtering and column masking access control #24277

Add support for row filtering and column masking access control #24277

Uh oh!

Conversation

BryanCutler commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

BryanCutler commented Dec 18, 2024

Uh oh!

jaystarshot commented Dec 18, 2024

Uh oh!

tdcmeehan commented Dec 20, 2024

Uh oh!

BryanCutler commented Jan 7, 2025

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!

BryanCutler commented Feb 18, 2025

Uh oh!

BryanCutler commented Mar 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

BryanCutler commented Apr 22, 2025

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BryanCutler commented Dec 18, 2024 •

edited

Loading