Allow ViewExpression use session user explicitly by kokosing · Pull Request #16436 · trinodb/trino

kokosing · 2023-03-08T14:40:35Z

Allow ViewExpression use session user explicitly

Before the change if access control is not returning any dedicated user to evaluate
the view expression they just pass the session user. However for the
engine it is not clear that is a session user and so engine needs to
retrieve groups for that user again and possibly some session roles are
lost.

After this change access control may decide to return empty identity.
That would be in line of view SECURITY INVOKER. Then engine can simply
reuse the session identity.

ksobolew

It needs some additional test cases. But yeah, I like it (I worked in this area before, and it fixes one significant pain point).

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java

ksobolew · 2023-03-09T10:39:45Z

Also, and I will repeat it again, ideally the ViewExpression::identity field would be an Identity, not String, so that the access controls can have more control over it.

ksobolew · 2023-03-09T10:40:33Z

This is also an SPI break. Just wanted to note this :)

kokosing · 2023-03-09T14:22:37Z

This is also an SPI break. Just wanted to note this :)

Addressed.

ksobolew · 2023-03-09T16:28:07Z

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

Hm. I requested a test for this, but now I realize that it's actually difficult to test. The error message here (I think?) comes from the regular check if the SELECT has access to the columns, not from the check if the expression in the filter has access to them. A more proper test would require that the Identity of the current session has some additional attributes (like roles or groups) that grant it access, but an Identity with just the user does not. In such a case, previously the check for SELECT would be done for the former, which would pass, and the check for the mask expression would be done using the latter, which would fail. After this change the former would be reused for the latter, so both would pass.

Added tests to TestAccessControl

dain

~~I don't think we need this.~~ commit was removed

core/trino-spi/src/main/java/io/trino/spi/Internal.java

findepi

"Remove obselete backward compability SPI exceptions"

findepi · 2023-03-10T15:29:06Z

core/trino-spi/pom.xml

above (<old>method void io.airlift.slice.SliceOutput::write(byte[]) throws java.io.IOException</old>) can also be removed

That is not maven said. I removed that and then I had to restore it.

findepi

"Convert classes to records" LGTM

findepi

"Simplify assertions" LGTM

findepi

"Improve method names"

findepi · 2023-03-10T15:30:49Z

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

What does "with user" mean? Did you mean "unprivileged user"?

"NotAccessible" -> "inaccessible"

unprivileged user. I changed them again. Hopefully now is better.

findepi

"Introduce @internal SPI annotation"

findepi · 2023-03-10T15:35:56Z

core/trino-spi/src/main/java/io/trino/spi/security/ViewExpression.java

I generally like the idea of being able to say which "direction" does given class work.

How would we enforce our connectors do not call internal APIs?
How should external connectors' maintainers to that?

In this particular case, what if one connector needs, for some reason, to rewrap a ViewExpression returned by some other connector. Can it do that? Is it illegal?

https://github.com/starburstdata/error-prone-checks can be a rescue.

Anyway I am removing this commit as it not the point of this PR and it requires its own discussion.

findepi

"Allow ViewExpression use session user explicitly" LGTM

would it be possible to make the change simpler?
For example, in StatamentAnalyzer have a logic conditional on session user equals view Expression's identity?

findepi · 2023-03-10T15:39:28Z

core/trino-spi/src/main/java/io/trino/spi/security/ViewExpression.java

Sometimes we have to introduce breaking changes.
So far we haven't been ultra strict about that, so I think it's fine to just change this method and add exclusion to revapi check. Let's defer intro of @Internal until we have more clairty what we want to achieve.

Sometimes we have to introduce breaking changes.

I added a new method instead. I think we can avoid breaking change here. It is not a big deal.

findepi · 2023-03-10T15:42:14Z

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

NotAccessible -> Inaccessible

"RowFilterAsInvoker" -- what does this mean?
do we have RUN AS INVOKER and RUN AS DEFINER row filers?

do we have RUN AS INVOKER and RUN AS DEFINER row filers?

Not at the moment, but we're trying to introduce that, basically. The general idea is that column masks and row filters are view-like constructs, which should behave similarly to views. Views have the SECURITY clause, and with filters and masks the equivalent is the ViewExpression::identity. Currently access controls are forced to always provide this identity, which is as if all views were SECURITY DEFINER and the owner was the current session's user. Making this Optional is effectively adding SECURITY INVOKER to filters and masks. Though it could probably be named differently.

"RowFilterAsInvoker" -- what does this mean?

I changed this to RunFilterAsSessionUser.

NotAccessible -> Inaccessible

The entire file is using NotAccessible. I will change that.

Not at the moment, but we're trying to introduce that, basically.

Can we make the PR title convey that?

At first this looked like an optimization -- if filter owner is dumbly filled with session user, skip re-getting groups.

Can we make the PR title convey that?

I was under impression it already does convey that. See Allow ViewExpression use session user explicitly where using session user is basically what SECURITY INVOKER is for views.

You're right. It seems some things are obvious to you while they are not so obvious to me.

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

findepi · 2023-03-10T15:43:46Z

...trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/TableAccessControlRule.java

nice!

why tell the engine what the user is :)

Yeah... engine knows it better.

findepi · 2023-03-10T15:45:04Z

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java

Can there be any difference between cases

when ViewExpression has empty identity

when ViewExpression has identity equal to current session user?

is it worth having a code comment why we don't check equality between them?

FWIW I tried to make it so that when the expression's identity is the same as the session's user then the latter is re-used, and it was vetoed. But I think making it explicit that it should be reused is a better way to go.

Also, extending the view analogy, with views we do have special handling if the current session's user is the same as the view's owner (some restrictions are relaxed then). So there may be a difference between these two, but I don't think there would be any meaningful difference in semantics.

I tried to make it so that when the expression's identity is the same as the session's user then the latter is re-used, and it was vetoed. But I think making it explicit that it should be reused is a better way to go.

We used have that for views and it was also causing issues. When session had limited set of roles, so they couldn't select the view because view wanted more roles. It got removed. So now in views the there is no custom logic for when view owner is the same as same user. "Impersonation" works the same way for any user now for views.

This commit is going to make something similar for filters and masks.

Can there be any difference between cases

when ViewExpression has empty identity

when ViewExpression has identity equal to current session user?

Yes. The former will impersonate the user to itself, and so the information about roles could change.

kokosing · 2023-03-11T07:10:04Z

I don't think we need this.

@dain Can you please elaborate? Please notice io.trino.plugin.base.security.TableAccessControlRule#getColumnMask where file based access control has no defined user for mask and we just take the session user. In such case engine was still doing unnecessary "impersonation" which can lead to issues like #15669 and others.

Before the change if access control is not returning any dedicated user to evaluate the view expression they just pass the session user. However for the engine it is not clear that is a session user and so engine needs to retrieve groups for that user again and possibly some session roles are lost. After this change access control may decide to return empty identity. That would be in line of view SECURITY INVOKER. Then engine can simply reuse the session identity.

The @internal annotation commit was removed

kokosing · 2023-03-14T08:56:02Z

For example, in StatamentAnalyzer have a logic conditional on session user equals view Expression's identity?

I think we have been in this place with views where we had such condition. It has its own issues so that condition was removed from views. I don't want to introduce it for masks&filters.

colebow · 2023-03-14T22:18:56Z

Does this need release notes?

kokosing · 2023-03-15T21:49:40Z

No. Thank you for asking.

cla-bot bot added the cla-signed label Mar 8, 2023

kokosing marked this pull request as draft March 8, 2023 14:44

ksobolew reviewed Mar 9, 2023

View reviewed changes

kokosing force-pushed the origin/master/176_optional_user branch 2 times, most recently from 6bb2403 to 20b1128 Compare March 9, 2023 14:22

kokosing requested review from findepi and martint March 9, 2023 14:35

kokosing marked this pull request as ready for review March 9, 2023 14:36

ksobolew reviewed Mar 9, 2023

View reviewed changes

dain previously requested changes Mar 10, 2023

View reviewed changes

core/trino-spi/src/main/java/io/trino/spi/Internal.java Outdated Show resolved Hide resolved

findepi reviewed Mar 10, 2023

View reviewed changes

findepi approved these changes Mar 10, 2023

View reviewed changes

kokosing force-pushed the origin/master/176_optional_user branch from 20b1128 to 710e393 Compare March 11, 2023 07:04

kokosing added 4 commits March 13, 2023 21:47

Convert classes to records

e75827b

Simplify assertions

79a2a0b

Improve test method names

4492850

kokosing force-pushed the origin/master/176_optional_user branch from 710e393 to 16c3e44 Compare March 13, 2023 20:47

kokosing merged commit 6099de8 into trinodb:master Mar 14, 2023

github-actions bot added this to the 411 milestone Mar 14, 2023

colebow mentioned this pull request Mar 14, 2023

Add Trino 411 release notes #16552

Merged

kokosing deleted the origin/master/176_optional_user branch March 15, 2023 21:48

Conversation

kokosing commented Mar 8, 2023

Uh oh!

ksobolew left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ksobolew commented Mar 9, 2023

Uh oh!

ksobolew commented Mar 9, 2023

Uh oh!

kokosing commented Mar 9, 2023

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!

dain left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

findepi left a comment

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!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

findepi left a comment

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!

findepi left a comment

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!

findepi left a comment

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!

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!

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!

dain left a comment •

edited

Loading

kokosing commented Mar 15, 2023 •

edited

Loading