Create a single projection for multiple column masks #14420

weiatwork · 2022-10-01T05:58:22Z

The current column masking behavior is problematic, if there are multiple column masks and some column is being masked itself as well as used as a condition for masking other columns. It's due to the masks being stacked with multiple projections. This creates a "chained masking" behavior, which is hardly anything users expect. The behavior is not predictable because it depends on the order of columns defined in the table.

The problem can be illustrated with examples below. Here's the DDL for table t1.

CREATE TABLE t1 (a int, b int);

Here's the original table content for t1

  a   |  b
------+------
    1 |    2
    3 |    4

Now let's use below file-based access control policy to do column masking for both column a and b.

{
    "tables": [
        {
            "catalog": "hive",
            "schema": "default",
            "table": "t1",
            "privileges": ["SELECT"],
            "columns": [
                {
                    "name": "a",
                    "mask": "IF (a <> 1, a)"
                },
                {
                    "name": "b",
                    "mask": "IF (a <> 1 OR a = 1, b)"
                }
            ]
        }
    ]
}

trino:default> select * from t1;
  a   |  b
------+------
 NULL | NULL
    3 |    4

The above result is wrong because the expected result is:

  a   |  b
------+------
 NULL |    2
    3 |    4

However, with a symmetric policy as below, we get expected result:

{
    "tables": [
        {
            "catalog": "hive",
            "schema": "default",
            "table": "t1",
            "privileges": ["SELECT"],
            "columns": [
                {
                    "name": "a",
                    "mask": "IF (b <> 2 OR b = 2, a)"
                },
                {
                    "name": "b",
                    "mask": "IF (b <> 2, b)"
                }
            ]
        }
    ]
}

trino:default> select * from t1;
  a   |  b
------+------
    1 | NULL
    3 |    4

The only reason for the latter example to (coincidentally) return the correct result is because column a is defined before column b.

Description

Non-technical explanation

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(X) Release notes are required, with the following suggested text:
Fixed possible incorrect result issue when multiple column masks exist.

# Section
* Fix some things. ({issue}`issuenumber`)

kokosing · 2022-10-02T05:52:22Z

The same issue may happen when you define masks together with filters. It can still happen that one thing influence the other.

In order to avoid the problem you need to define the user for masks and filters and then these expression will be executed with privileges of such user so mask and filler won't affect each other.

findepi · 2022-10-03T08:43:11Z

The mask expression should take as input table source columns. The masks shouldn't "stack".
Same for filter - it should take unmasked columns as the input.

kasiafi · 2022-10-03T10:41:25Z

If I understand correctly what @kokosing says in #12262 (comment), it is expected behavior that masks "stack".

findepi · 2022-10-03T10:47:38Z

If I understand correctly what @kokosing says in #12262 (comment), it is expected behavior that masks "stack".

Thanks for the pointer.

It's not expected to me, so i am challenging that :)

weiatwork · 2022-10-03T17:08:07Z

My opinion is consistent to what @findepi described above. We should only use the unmasked/original columns as conditionals for other column masks.

weiatwork · 2022-10-03T17:08:50Z

The same issue may happen when you define masks together with filters. It can still happen that one thing influence the other.

In order to avoid the problem you need to define the user for masks and filters and then these expression will be executed with privileges of such user so mask and filler won't affect each other.

@kokosing To get to the bottom of such issues, maybe we can approximately categorize the use cases as below:
(1) multiple row filters
(2) multiple column masks
(3) combined use of filters and masks
For (1) I think we are fine, since they will be conjuncted and there shouldn't be any side effect. For (2) it is what this PR is about. For (3) do you mind to provide a simple example?

martint · 2022-10-03T17:17:44Z

This is definitely a bug. The masks shouldn't stack. Any references they make to columns should be based on the original un-masked value.

martint · 2022-10-03T17:21:07Z

BTW, the only situations where they might "stack" with respect to other masks is when:

The table is actually a view and it references other tables/columns with their own masks.
The mask expression contains a reference to other tables (via a subquery) and those tables contain their own masks.

martint · 2022-10-03T18:06:18Z

core/trino-main/src/test/java/io/trino/sql/query/TestColumnMask.java

@@ -209,7 +209,8 @@ public void testMultipleMasksOnSameColumn()
                USER,
                new ViewExpression(USER, Optional.empty(), Optional.empty(), "custkey * 2"));

-        assertThat(assertions.query("SELECT custkey FROM orders WHERE orderkey = 1")).matches("VALUES BIGINT '-740'");
+        // When there are multiple masks on the same column, the latter one overrides the previous ones
+        assertThat(assertions.query("SELECT custkey FROM orders WHERE orderkey = 1")).matches("VALUES BIGINT '740'");


This is a change in semantics. Looking at this, I realize we never settled the question of why we'd ever have multiple masks on a single column: #11654 (comment). That still doesn't make a lot of sense to me. cc @kokosing @ksobolew

We can have multiple system access controls and each of them can return mask for a given column.

How are the masks expected to be applied in such a case? There's no guarantee about the order in which the system access controls are invoked.

Also, you're talking about multiple system access controls, but the SystemAccessControl interface allows a single access control interface to return multiple masks for a given column. That does not make much sense -- if only one mask is to be applied, the interface should return a single mask.

There are multiple situations in which more than one mask may apply to a single column. The simplest example is when there are two roles enabled and each of them applies a different column mask on a particular column. This immediately raises the question of ordering, of course. While it looks like Trino does not make such guarantees explicitly, the ordering is in practice deterministic as long as the access controls return them in a deterministic order. The masks and filters are returned and processed as Lists, and this implies ordering. The engine has no trouble dealing with this and applies the masks iteratively.

That still doesn't make a lot of sense to me. Masks are defined in terms of the values in the column. For example, if column is a string that contains only numbers, one might implement a mask function by parsing the number, doing some math (a hash, etc) on it and converting it back to a string. If the the engine applies the function on anything other than the original values (e.g., because another mask got injected in between), the mask will fail. This makes it hard for someone writing a mask function to reason about, as it requires non-local understanding of how every role, user and mask relate to each other.

If multiple masks are possible, it should be up to the connector to decide which one is the most appropriate and go with it. In the case of multiple system access controls returning masks, there are two options:

Pick the first one. This can produce non-deterministic results depending on which order the access controls get called. It could change across restarts of the server

Fail with an error indicating there's ambiguity.

kasiafi · 2022-10-05T08:01:40Z

The behavior is not predictable because it depends on the order of columns defined in the table.

@weiatwork from the earlier discussion #12262 (comment) I understood that this is not the case. The behavior depends on the order of masks applied, but it has nothing to do with the order of column definitions.

If I'm wrong, then this is yet another issue which should be addressed independently from the one here.

weiatwork · 2022-10-05T16:15:01Z

@kasiafi As demonstrated by the two examples above, with the first AC policy the query retuned wrong result because column a was masked first, and then column b was masked using a's masked value. We can see in this case the masking of b (arguably incorrectly) depends on a. Even if we switch the order of the two masks in the AC policy, it still returns the same wrong result, which means the dependence was not based on mask definitions, but on schema definition (since b comes after a in DDL):

                {
                    "name": "b",
                    "mask": "IF (a <> 1 OR a = 1, b)"
                },
                {
                    "name": "a",
                    "mask": "IF (a <> 1, a)"
                }

For the second AC policy in original examples, again column a was masked first, by testing b's values (but this time, b's original unmasked values). And then column b itself was masked, using the value of itself. We got correct result (luckily), mainly because b's masking evaluation happened after a's, due to schema definition.

IMHO, column masking shouldn't have dependency on masking definition in the policy, nor on the schema definition. Each column mask should be independently applied, and if there is a conditional relying on other columns, the unmasked/original values of other columns must be used. That's the main motivation of this PR.

As to the discussion on multiple masks on the same column, it's a separate issue that we can follow up.

weiatwork · 2022-10-07T17:31:50Z

Just to follow up, do we have a conclusion what to do with multiple masks (on the same column or from a dependent column)?

atanasenko · 2022-12-16T20:59:17Z

@weiatwork haven't seen this pr and started implementing the same thing as well in #15443, but there I retained compatibility with multiple masks per field.

weiatwork · 2022-12-20T06:40:18Z

Thanks @atanasenko for the heads-up. I'm not sure if our changes are the same by looking at changes in the unit test. Will your change solve the issue I mentioned in this ticket in the beginning?

atanasenko · 2022-12-20T13:10:32Z

Thanks @atanasenko for the heads-up. I'm not sure if our changes are the same by looking at changes in the unit test. Will your change solve the issue I mentioned in this ticket in the beginning?

@weiatwork I started with exactly the same changes as you, but then modified the code to support multiple masks per field. In this case additional masks will get added to the second projection.
Basically, in my pr masks are spread over projections as follows:

There is a mask for n_regionkey that shows it only for nations starting with A, and there are two masks for nation, first only shows it if it belongs to region=0, second lowercases the name.

Output      | Proj2         | Proj1                             | Scan
------------+---------------+-----------------------------------+---------------
n_nationkey |               |                                   | n_nationkey
n_name      | lower(n_name) | IF(n_regionkey = 0, n_name)       | n_name
n_regionkey |               | IF(n_name like 'A%', n_regionkey) | n_regionkey
n_comment   |               |                                   | n_comment

atanasenko · 2022-12-20T13:54:58Z

@martint what would be the right approach with multiple column masks per field? I think in order to not break some users that depend on it, we should retain the compatibility.

martint · 2022-12-20T19:51:44Z

In what cases would someone be relying on multiple masks per field? I don't think any connectors in Trino support or do that currently. It should be the responsibility of the connector to compose masks in that manner if it knows how to deal with that situation and present a single mask expression to the engine (in general this doesn't make sense, though, as a mask may transform the values in manners that create invalid input for downstream mask expressions)

atanasenko · 2022-12-20T20:08:45Z

Masks can be provided by otherwise unrelated system and connector access controls and then are combined into a single list (per AccessControlManager#getColumnMasks()).

martint · 2022-12-20T22:08:52Z

Masks can be provided by otherwise unrelated system and connector access controls and then are combined into a single list (per AccessControlManager#getColumnMasks()).

That use case is unsupported. There's no guarantee about the order in which masks will be applied and whether the functions are even compatible with each other. For instance, one function might want to parse a string that contains numbers and another function may just replace numbers with Xs. Depending on the order in which the functions are applied, the results may vary or queries may fail altogether.

atanasenko · 2022-12-20T23:29:23Z

So all-in-all we need to decide what to do about this. If we want to explicitly say that we don't support multiple masks per field, we need to throw an error. Otherwise, we can make best-effort to try and stack them (and let query fail if there are any type incompatibilities). We definitely should not silently ignore them.

martint · 2022-12-21T17:24:58Z

Yeah, multiple masks should result in an error. Returning arbitrary results that depend on the order of application, or potentially failing if the given order makes the masks incompatible would lead to surprising behaviors, hard to debug results and confused users.

atanasenko · 2022-12-21T18:38:46Z

@weiatwork is the failing approach good with you? Can you get that in?
I'd close my pr in favour of this one as it's way better documented.

weiatwork · 2022-12-27T22:09:09Z

Sorry for the delay. Sure, that sounds good to me. Just want to clarify, do we want to only disallow multiple masks on the same column when the masks are from both system AC and connector AC? Or do we also want to change AccessControl's getColumnMasks interface and make it singular getColumnMask everywhere, i.e. even multiple masks from a single system or connector AC is not supported?

atanasenko · 2023-01-03T14:11:28Z

@weiatwork I think it should be enough to error out when we detect there are multiple masks in the combined list. Changing SPI is problematic, and maybe we'll have a better approach at mutliple masks per column in the future, like validating expressions that they yield the same type, for instance.

martint · 2023-01-09T19:14:07Z

We should (separately), change the SPI to expect a single mask. We may be able to do this in a backward compatible way by adding the new desired method and falling back, temporarily, to an implementation that composes the masks from the existing methods. We'd mark the existing method as deprecated and remove it in a future version. Plugins would be responsible for composing the masks if they need to and are able to.

weiatwork · 2023-01-09T19:52:51Z

Thanks Martin. That makes sense to me. Do you mind to get this PR merged then? I don't think the only failing check is functionally related (TestHiveFaultTolerantExecutionTest). It looks like a test resource issue relevant to JVM. Or could you trigger a re-run of the tests?

atanasenko · 2023-01-11T12:13:58Z

@martint can we get that merged?

martint

One last minor comment.

core/trino-main/src/main/java/io/trino/sql/planner/RelationPlanner.java

core/trino-main/src/test/java/io/trino/security/TestAccessControlManager.java

core/trino-main/src/main/java/io/trino/sql/planner/RelationPlanner.java

Current approach of stacking multiple projections has correctness issue: It introduces chained dependency on derived/masked columns instead of the original columns. Collapsing multiple projections into one can ensure the consistent dependency on the original columns, as well as make the plan more efficient.

kokosing · 2023-01-11T23:33:21Z

core/trino-main/src/test/java/io/trino/sql/query/TestColumnMask.java

@@ -212,7 +212,8 @@ public void testMultipleMasksOnSameColumn()
                USER,
                new ViewExpression(USER, Optional.empty(), Optional.empty(), "custkey * 2"));

-        assertThat(assertions.query("SELECT custkey FROM orders WHERE orderkey = 1")).matches("VALUES BIGINT '-740'");
+        // When there are multiple masks on the same column, the latter one overrides the previous ones
+        assertThat(assertions.query("SELECT custkey FROM orders WHERE orderkey = 1")).matches("VALUES BIGINT '740'");


Shouldn't this now fail with .hasMessageMatching("Multiple masks on a single column is not supported: column");?

That only happens after the second commit. Perhaps they should be reordered, so that the prohibition on multiple masks goes first.

OTOH, it does pass in the CI apparently

Ah, I see - the test is using TestingAccessControlManager, which replaces the actual AccessControlManager and does not check for multiple masks.

kokosing · 2023-01-11T23:33:42Z

core/trino-main/src/test/java/io/trino/security/TestAccessControlManager.java

@@ -209,6 +209,7 @@ public void testDenyTableFunctionCatalogAccessControl()
        }
    }

+    // TODO: need to properly handle the ordering of multiple masks as they are not allowed currently


I think the TODO and test name is incorrect. I would simply remove TODO and name test to testMultipleColumnMasks

weiatwork · 2023-01-20T19:58:04Z

Subsumed by #15680 thus closing

cla-bot bot added the cla-signed label Oct 1, 2022

weiatwork requested review from martint, kokosing and kasiafi October 1, 2022 05:59

findepi mentioned this pull request Oct 3, 2022

Fix an issue in InlineProjections that may cause query failure when multiple column masks exist on a table #12262

Merged

martint reviewed Oct 3, 2022

View reviewed changes

martint mentioned this pull request Dec 16, 2022

Make column masks insensitive to relation field order #15443

Closed

weiatwork force-pushed the oneprojection branch 2 times, most recently from 8cf5b29 to 66e5d4b Compare January 5, 2023 19:27

martint reviewed Jan 11, 2023

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/planner/RelationPlanner.java Show resolved Hide resolved

kokosing reviewed Jan 11, 2023

View reviewed changes

core/trino-main/src/test/java/io/trino/security/TestAccessControlManager.java Outdated Show resolved Hide resolved

martint requested changes Jan 11, 2023

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/planner/RelationPlanner.java Show resolved Hide resolved

weiatwork added 2 commits January 11, 2023 14:16

Disallow multiple masks on a single column

ec2c794

weiatwork force-pushed the oneprojection branch from 66e5d4b to ec2c794 Compare January 11, 2023 23:03

kokosing reviewed Jan 11, 2023

View reviewed changes

martint mentioned this pull request Jan 12, 2023

Fix issues due to mask aliasing #15680

Merged

weiatwork closed this Jan 20, 2023

weiatwork deleted the oneprojection branch January 20, 2023 19:58

Create a single projection for multiple column masks #14420

Create a single projection for multiple column masks #14420

Conversation

weiatwork commented Oct 1, 2022

Description

Non-technical explanation

Release notes

kokosing commented Oct 2, 2022

findepi commented Oct 3, 2022

kasiafi commented Oct 3, 2022

findepi commented Oct 3, 2022

weiatwork commented Oct 3, 2022

weiatwork commented Oct 3, 2022

martint commented Oct 3, 2022

martint commented Oct 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasiafi commented Oct 5, 2022

weiatwork commented Oct 5, 2022

weiatwork commented Oct 7, 2022

atanasenko commented Dec 16, 2022

weiatwork commented Dec 20, 2022

atanasenko commented Dec 20, 2022 • edited Loading

atanasenko commented Dec 20, 2022

martint commented Dec 20, 2022

atanasenko commented Dec 20, 2022

martint commented Dec 20, 2022

atanasenko commented Dec 20, 2022

martint commented Dec 21, 2022

atanasenko commented Dec 21, 2022

weiatwork commented Dec 27, 2022

atanasenko commented Jan 3, 2023

martint commented Jan 9, 2023 • edited Loading

weiatwork commented Jan 9, 2023

atanasenko commented Jan 11, 2023

martint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiatwork commented Jan 20, 2023

atanasenko commented Dec 20, 2022 •

edited

Loading

martint commented Jan 9, 2023 •

edited

Loading