Pass users original role for impersonation by homar · Pull Request #25371 · trinodb/trino

homar · 2025-03-20T23:09:58Z

Description

Pass users original role for impersonation

Without this the way impersonation work is broken.
Considering followin scenarion

there are 2 users: alice and bob
there is a role alice_role that allows for bob impersonation

If alice sets alice_role and then calls SET SESSION AUTHORIZATION bob
access control will make a check canImpersonateUser, because alice_role
allows for that impersonation it will succeed.
However every next query will also call this canImpersonateUser check
and because alice_role is not assigned anymore it will fail every time.

The idea behind the fix is to pass roles of the original user, alice
from the example above, between server and clients using new headers
X-Trino-Set-Original-Roles. Then roles obtained in this way can be used
in the checkCanImpersonate to make sure it passes

Additional context and related issues

Release notes

( ) 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.
(x) Release notes are required, with the following suggested text:

## Section 
* Fix impersonation access control when access is granted via through the role. [issue](https://github.com/trinodb/trino/issues/25166)

Fixes #25166

kokosing · 2025-03-21T12:17:10Z

This relates to #25166

kokosing · 2025-03-21T13:20:21Z

How do we pass original user info?

Via sessionUser AFAIK, see

trino/client/trino-client/src/main/java/io/trino/client/StatementClientV1.java

Line 124 in c8a173a

this.originalUser = Stream.of(session.getSessionUser(), session.getUser())

kokosing · 2025-03-21T13:22:08Z

extract this lambda as method

kokosing · 2025-03-21T13:22:45Z

Optional collection is smelly. Can we remove the optional here?

ok so actually this is a little hack. The idea is to have one field instead of two to make code simpler.

if optional is empty this impersonation mechanism is disabled

it it is not empty but it doesn't contain checked role then impersonation check fails

if it contains role then impersonation is allowed.
Alternatively you would need one variable to check if this mechanism is enabled at all and if it is enabled then a Set to check if specific role allows for impersonation.
If you believe having 2 variables is better I will change.

kokosing · 2025-03-21T13:26:32Z

Remove the comment

Why? It is explains why it is done this way. Assuming we will stay with Optional<Set>>

I see exactly the same in line below in java. I mean the comment is obvious. It is a nit btw.

kokosing · 2025-03-21T13:28:46Z

testImpersonationAllowedByRole

kokosing · 2025-03-21T13:31:23Z

Also this should be something that holds authorization trinity. You need to have subject, action (here it is know that it is impersonation) and entity. So you need to hold pair of who (subject) can impersonate who (entity). And subject here is an (user with his roles), entity is just user.

Why? That would make this much more complex. Here I only store roles that allow impersonations, this is for tests only and trino doesn't even have good support for roles in general..

Let's sync on that.

I mean there is some thinking here in the class and you are introducing new concept of access control to this testing class which makes it harder. I would like us to reuse what we have and rewrite the test.

kokosing · 2025-03-21T13:31:39Z

Add also testImpersonationAllowedByUser

What is the point? If I understand correctly there are such tests here: TestSetSessionAuthorization

trino/testing/trino-tests/src/test/java/io/trino/execution/TestSetSessionAuthorization.java

Line 42 in e9ab226

public class TestSetSessionAuthorization

The entire point of this change is to fix the scenario that is role based not user based.

kokosing · 2025-03-21T13:33:17Z

How do these two differ?

kokosing · 2025-03-21T13:34:06Z

I think we have some utilities for that in trino-jdbc tests.

kokosing · 2025-03-28T15:58:01Z

@wendigo can you please take a look too?

kokosing · 2025-03-28T15:54:05Z

separate commit for the refactor or undo

I wouldn't say it is a refactor but sure I will undo this

kokosing · 2025-03-28T15:55:12Z

It can depend on scope tests right?

kokosing · 2025-03-28T15:55:54Z

Please add more tests. For impersonation with user, with role none and all roles.

I am not sure I understand the part about impersonation with user :/

I mean when grant for impersonation is given to the user directly, not to the role.

This PR is about roles not grants, so I am not sure why this would matter here

I am not sure I even know how to do that. TestingSystemSecurityMetadata doesn't support that and TestingAccessControlManager only supports denies

kokosing · 2025-03-28T15:56:48Z

I see exactly the same in line below in java. I mean the comment is obvious. It is a nit btw.

kokosing · 2025-03-28T15:57:49Z

Let's sync on that.

Without this the way impersonation work is broken. Considering followin scenarion - there are 2 users: alice and bob - there is a role alice_role that allows for bob impersonation If alice sets alice_role and then calls SET SESSION AUTHORIZATION `bob` access control will make a check `canImpersonateUser`, because alice_role allows for that impersonation it will succeed. However every next query will also call this `canImpersonateUser` check and because alice_role is not assigned anymore it will fail every time. The idea behind the fix is to pass roles of the original user, alice from the example above, between server and clients using new headers X-Trino-Set-Original-Roles. Then roles obtained in this way can be used in the checkCanImpersonate to make sure it passes.

kokosing

Good job!

kokosing · 2025-04-18T10:58:25Z

@homar let's rename original roles to authorization roles to match the authorization user term and terminology in #2512

Also let's add a test for RESET AUTHORIZATION to make sure authorization roles and user are cleared.

cla-bot Bot added the cla-signed label Mar 20, 2025

github-actions Bot added the jdbc Relates to Trino JDBC driver label Mar 20, 2025

homar force-pushed the homar/add_set_original_role branch 2 times, most recently from 32c54d4 to e1f25b2 Compare March 21, 2025 09:06

github-actions Bot added the docs label Mar 21, 2025

homar force-pushed the homar/add_set_original_role branch from e1f25b2 to e66a3e1 Compare March 21, 2025 09:09

homar changed the title ~~Homar/add set original role~~ Pass users original role for impersonation Mar 21, 2025

homar force-pushed the homar/add_set_original_role branch from e66a3e1 to bd91d3a Compare March 21, 2025 09:59

kokosing reviewed Mar 21, 2025

View reviewed changes

homar force-pushed the homar/add_set_original_role branch from bd91d3a to 02a102f Compare March 21, 2025 21:34

homar marked this pull request as ready for review March 24, 2025 13:37

kokosing reviewed Mar 28, 2025

View reviewed changes

homar force-pushed the homar/add_set_original_role branch 2 times, most recently from c8dcf21 to 8d27298 Compare March 31, 2025 22:53

homar force-pushed the homar/add_set_original_role branch 3 times, most recently from 2b21afd to e5dda50 Compare April 8, 2025 07:25

homar requested a review from kokosing April 8, 2025 10:58

kokosing approved these changes Apr 10, 2025

View reviewed changes

kokosing merged commit 3b251c6 into trinodb:master Apr 10, 2025
286 of 289 checks passed

github-actions Bot added this to the 475 milestone Apr 10, 2025

findinpath mentioned this pull request Apr 10, 2025

Add support for passing user's original role for impersonation trinodb/trino-python-client#547

Open

1 task

kokosing mentioned this pull request Apr 18, 2025

Roles of original users are not persisted through SET SESSION AUTHORIZATION calls #25166

Closed

Conversation

homar commented Mar 20, 2025 • edited by kokosing Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context and related issues

Release notes

Uh oh!

kokosing commented Mar 21, 2025

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!

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!

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!

kokosing commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

kokosing left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kokosing commented Apr 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

homar commented Mar 20, 2025 •

edited by kokosing

Loading

kokosing commented Mar 28, 2025 •

edited

Loading