Check access to masked columns in bulk when analyzing masks #23064
Conversation
| for (ColumnSchema columnSchema : columnSchemas) { | ||
| Optional.ofNullable(masks.get(columnSchema)).ifPresent(mask -> { | ||
| if (checkCanSelectFromColumn(name, columnSchema.getName())) { | ||
| if (checkCanSelectFromColumns(name, columnSchemas.keySet())) { |
There was a problem hiding this comment.
We could move the call to accessControl.getColumnMasks into this if block so the column masks are not fetched if they are not going to be processed, wdyt?
A similar logic may be applicable to the row filters; I imagine row filters aren't useful if the session can't select from columns. I am not very familiar with how trino analyses row filters so please double-check.
LE: looking at the test failures for the initial version those were likely caused because checkCanSelectFromColumns was called with all columns rather than just column that have masks. Considering this we can't avoid the getColumnMasks check
There was a problem hiding this comment.
We could move the call to
accessControl.getColumnMasksinto thisifblock so the column masks are not fetched if they are not going to be processed, wdyt?
I had to rework the code a bit, because I was not aware of some special behavior with hide-inaccessible-columns (thankfully Trino has excellent test coverage), so now it's not beneficial anymore. But good point.
A similar logic may be applicable to the row filters; I imagine row filters aren't useful if the session can't select from column. I am not very familiar with how trino analyses row filters so please double-check.
Row filters are on a table level, so that's a little different, and the filtering expression is analyzed as part of the WHERE clause. We could imagine a bulk getRowFilters, though - just as there are tables with thousands of columns, there are some crazy queries that reference thousands of tables.
There was a problem hiding this comment.
Row filters are on a table level, so that's a little different, and the filtering expression is analyzed as part of the
WHEREclause.
On the other hand I may be out of my depth here as well :)
There was a problem hiding this comment.
On the other hand I may be out of my depth here as well :)
It seems I totally am - that method is called before the analyzer knows which columns are referenced by the query, so we get all columns from the table. That access check was introduced to avoid confusing failures when we analyze masks on columns which are not involved in the query at all.
The `getColumnMasks` SPI call was added in order to reduce the number of SPI calls in cases of tables with large number of columns, similar to how `checkCanSelectFromColumns` does it. This is an important performance optimization for access controls like OPA. But, once we discover the column masks, we check whether the current session has access to the masked columns - if not, we skip analyzing it further. The problem was that each column mask was inspected in isolation, resulting in multiple checks to `checkCanSelectFromColumns` with just a single column. I believe this is contrary to the goals of the change which introduced `getColumnMasks`. This PR adds a fast path with a single check for all masked columns instead of one check per each masked column. If that single check fails, though, we enter the slow path, in which we analyze masks on all the accessible columns to see if they reference any other columns. This is relevant when the `hide-inaccessible-columns` option is set, in which case we cannot assume that an inaccessible masked column will fail the query.
ksobolew
force-pushed
the
kudi/batch-access-check-in-analyze-masks
branch
from
38e7032 to
e6d6654
Compare
| @@ -2375,25 +2375,37 @@ private void checkStorageTableNotRedirected(QualifiedObjectName source) | |||
|
|
|||
There was a problem hiding this comment.
I think we need to have something in place to prevent such regressions in future.
There was a problem hiding this comment.
I think we need to have something in place to prevent such regressions in future.
We have some tests which are counting invocations of some interface, like the metastore calls. These are useful when these invocations are known to be slow and we want to have as little of them as possible. But doing something like that for access control would be a big task.
|
|
||
| for (ColumnSchema columnSchema : columnSchemas) { | ||
| Optional.ofNullable(masks.get(columnSchema)).ifPresent(mask -> { | ||
| if (checkCanSelectFromColumn(name, columnSchema.getName())) { |
There was a problem hiding this comment.
I don't know why we were doing a check here in a first place. See io.trino.sql.analyzer.Analyzer#analyze(io.trino.sql.tree.Statement, io.trino.sql.analyzer.QueryType). We used to have similar problem in the past that same tables and columns were checked many times, so we collect then during analysis and check once at the end. I guess we need follow the approach.
There was a problem hiding this comment.
I'm not sure either, but I guess it's because we don't want to do analysis on things the user doesn't have access to because it may in fact be creating a risk of information leakage. I can't think of a scenario off the top of my head, but I think caution is warranted here.
There was a problem hiding this comment.
BTW, see who introduced that check: #6017 (and #6378) :) It was in response to #4496, which suggests this is not strictly a security thing, but a usability thing? - if the column mask's evaluation would cause an error, we can avoid that error by not evaluating it for inaccessible columns. If I understand this right.
There was a problem hiding this comment.
Do you think if we would have #15131 then we would be able to solve #4496? Also I believe with that approach we would be able to access control checks in io.trino.sql.analyzer.Analyzer#analyze(io.trino.sql.tree.Statement, io.trino.sql.analyzer.QueryType) as we could simply do then sooner. But what we need to have is a test for number of access control checks to make sure we don't regress it.
| @@ -2375,25 +2375,37 @@ private void checkStorageTableNotRedirected(QualifiedObjectName source) | |||
|
|
|||
| private void analyzeFiltersAndMasks(Table table, QualifiedObjectName name, RelationType relationType, Scope accessControlScope) | |||
There was a problem hiding this comment.
Notice #15131, which could solve this problem too and then you don't have to think about it and have complex optimization cod in statement analyzer which is not a simple thing on its own.
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time. |
Description
The
getColumnMasksSPI call was added in order to reduce the number of SPI calls in cases of tables with large number of columns, similar to howcheckCanSelectFromColumnsdoes it. This is an important performance optimization for access controls like OPA. But, once we discover the column masks, we check whether the current session has access to the masked columns - if not, we skip analyzing it further. The problem was that each column mask was inspected in isolation, resulting in multiple checks tocheckCanSelectFromColumnswith just a single column. I believe this is contrary to the goals of the change which introducedgetColumnMasks.This PR adds a fast path with a single check for all masked columns instead of one check per each masked column. If that single check fails, though, we enter the slow path, in which we analyze masks on all the accessible columns to see if they reference any other columns. This is relevant when the
hide-inaccessible-columnsoption is set, in which case we cannot assume that an inaccessible masked column will fail the query.Additional context and related issues
The SPI call was added and access control code changed to use it in #21997.
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: