Support SET SESSION AUTHORIZATION and RESET SESSION AUTHORIZATION #16067

baohe-zhang · 2023-02-11T00:23:33Z

Description

This PR adds support for impersonating as another user by running SET SESSION AUTHIORIZATION user SQL command. And it's related to the issue #2512. After this PR gets merged, user u1 can run SET SESSION AUTHORIZATION u2 to impersonate u2. And the user can run RESET SESSION AUTHORIZATION to switch back to the original user u1. This SQL command can allow a superuser to temporarily become an unprivileged user and later switch back to being a superuser.

New request headers:

X-Trino-Original-User: store the original logged-in user, the X-Trino-User can be updated with SET SESSION AUTHORIZATION user command.

New response headers:

X-Trino-Set-Authorization-User: instruct the client to set X-Trino-User to another user.
X-Trino-Reset-Authorization-User: instruct the client to reset X-Trino-User to the original user.

The workflow of the set session authorization:

User u1 starts a trino session, initially the request header will have <X-Trino-User: u1> and <X-Trino-Original-User: u1>.
User u1 runs SET SESSION AUTHORIZATION u2.
Trino server parses this SQL command and checks if the original user u1 can impersonate u2. If the check passes the server will add <X-Trino-Set-Authorization-User:u2> in the response header.
Trino client processes the response and it finds X-Trino-Set-Authorization-User exist in the header, thus updating the ClientSession.authorizationUser field to u2.
Trino client submits a sql query and adds <X-Trino-User: u2> and <X-Trino-Original-User: u1> in the request header.
Trino server detects X-Trino-User and X-Trino-Original-User are different in the request header, thus it will check if original user u1 can impersonate u2. If the check passes the query will be executed as u2 (use u2 to create session identity).

Since the Session's identity will be overridden by authorizationIdentity in step 5, we would need a new session field originalIdentity to preserve the information of the original user. This originalIdentity will be used for the impersonation checks during query session creation and It will also be used as the source of audit information.

Additional context and related issues

workflow:

trino> select current_user;
  _col0   
----------
 baozhang 
(1 row)

trino> set session authorization prestodv;
SET SESSION AUTHORIZATION
trino> 
trino> select current_user;
  _col0   
----------
 prestodv 
(1 row)

trino> reset session authorization;
RESET SESSION AUTHORIZATION
trino> 
trino> select current_user;
  _col0   
----------
 baozhang 
(1 row)

trino-python-client support is added in trinodb/trino-python-client#349

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:

# General
* Add support for `SET SESSION AUTHORIZATION` and `RESET SESSION AUTHORIZATION` syntax. ({issue}`16067`)

phd3 · 2023-02-14T00:23:01Z

Please look at test failures - seem related

core/trino-main/src/main/java/io/trino/server/HttpRequestSessionContextFactory.java

phd3 · 2023-02-14T02:04:46Z

core/trino-main/src/main/java/io/trino/server/HttpRequestSessionContextFactory.java

@@ -209,22 +218,36 @@ public Identity extractAuthorizedIdentity(
            throw badRequest(e.getMessage());
        }

-        Identity identity = buildSessionIdentity(optionalAuthenticatedIdentity, protocolHeaders, headers);
+        Identity originalIdentity = buildSessionOriginalIdentity(optionalAuthenticatedIdentity, protocolHeaders, headers);


can we add tests in TestResourceSecurity and TestUserImpersonationAccessControl to validate the impersonation ?

We can add unit tests in TestUserImpersonationAccessControl after we have added client side support. And I think the unit test there should be enough to validate the impersonation.

The unit tests are added in testing/trino-tests/src/test/java/io/trino/execution/TestSetSessionAuthorization.java

Adding to TestResourceSecurity still seems valuable as UI access (e.g. QueryResource access goes through extractAuthorizedIdentity without going through the query flow - and would be good to also test it out in this codepath - in case someone sends a X-Trino-Authorization-User header that way)

Sounds good. Will take a look and try to add some unit tests for UI access impersonation.

Added a unit test in TestResourceSecurity to verify the code path of UI impersonation.

kokosing · 2023-02-17T06:55:39Z

core/trino-main/src/main/java/io/trino/SessionRepresentation.java

@@ -308,6 +326,22 @@ public Identity toIdentity(Map<String, String> extraCredentials)
                .build();
    }

+    public Identity toOriginalIdentity()


In HttpRequestSessionContextFactory we are also using authenticatedIdentity. I think the flow is already complex, it would be nice to document them and differences between them somewhere. One thing is to document it https://trino.io/docs/current/develop/client-protocol.html, but I think also it would be nice to document authentication flow. Maybe https://github.com/trinodb/trino/wiki/HTTP-Protocol

Added docs for new client headers.

kokosing · 2023-02-17T06:57:20Z

core/trino-main/src/main/java/io/trino/Session.java

@@ -115,6 +117,7 @@ public Session(
        this.transactionId = requireNonNull(transactionId, "transactionId is null");
        this.clientTransactionSupport = clientTransactionSupport;
        this.identity = requireNonNull(identity, "identity is null");
+        this.originalIdentity = requireNonNull(originalIdentity, "originalIdentity is null");


Did you mean principalIdentity or authenticationIdentity. The identity that is used for authentication, while the other one should be called sessionIdentity or authorizationIdentity. Am I right?

You're right - but since the "executing" identity is referred to as "identity" in the code currently - we can refer the "authorization" identity as "identity" with this change ... and rename the original (or authentication) identity, wdyt ?

docs/src/main/sphinx/sql/set-session-authorization.rst

phd3

some initial comments

phd3 · 2023-02-15T20:20:08Z

core/trino-main/src/main/java/io/trino/execution/QueryStateMachine.java

+    public void setSetAuthorizationUser(String authorizationUser)
+    {
+        requireNonNull(authorizationUser, "Authorization user cannot be null or empty");
+        this.setAuthorizationUser.set(authorizationUser);


nit: redundant this

phd3 · 2023-02-17T14:17:58Z

core/trino-main/src/main/java/io/trino/execution/QueryStateMachine.java

@@ -825,6 +828,12 @@ public String getSetPath()
        return setPath.get();
    }

+    public void setSetAuthorizationUser(String authorizationUser)
+    {
+        requireNonNull(authorizationUser, "Authorization user cannot be null or empty");


nit: should have isNullOrEmpty check based on the error message

core/trino-main/src/main/java/io/trino/execution/SetSessionAuthorizationTask.java

phd3 · 2023-02-17T14:21:18Z

core/trino-main/src/main/java/io/trino/execution/SetSessionAuthorizationTask.java

+
+        String user;
+        Expression userExpression = statement.getUser();
+        if (userExpression instanceof Identifier) {


nit: use Java 17 syntax where you don't need to cast again

e.g.

if (userExpression instanceof Identifier identifier) { user = identifier.getValue(); }

phd3 · 2023-02-17T14:21:34Z

core/trino-main/src/main/java/io/trino/execution/SetSessionAuthorizationTask.java

+        else {
+            throw new IllegalArgumentException("Unsupported user expression: " + userExpression.getClass().getName());
+        }
+        requireNonNull(user, "Authorization user cannot be null or empty");


same comment about nullOrEmpty

phd3 · 2023-02-17T15:37:10Z

docs/src/main/sphinx/sql/reset-session-authorization.rst

+Description
+-----------
+
+Resets the current user back to the original user.


nit: we need to add more details in terms of what the "original" user means here (i.e. sessionUser vs principal etc)

Added more details.

phd3 · 2023-02-17T15:48:02Z

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

@@ -419,6 +426,11 @@ private void processResponse(Headers headers, QueryResults results)
            this.setAuthorizationUser.set(setAuthorizationUser);
        }

+        String resetAuthorizationUserStr = headers.get(TRINO_HEADERS.responseResetAuthorizationUser());


resetAuthorizationUser should be fine - generally we avoid abbreviations in codebase

core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4

phd3 · 2023-02-17T16:15:02Z

core/trino-main/src/main/java/io/trino/server/HttpRequestSessionContextFactory.java

@@ -209,22 +218,36 @@ public Identity extractAuthorizedIdentity(
            throw badRequest(e.getMessage());
        }

-        Identity identity = buildSessionIdentity(optionalAuthenticatedIdentity, protocolHeaders, headers);
+        Identity originalIdentity = buildSessionOriginalIdentity(optionalAuthenticatedIdentity, protocolHeaders, headers);


Adding to TestResourceSecurity still seems valuable as UI access (e.g. QueryResource access goes through extractAuthorizedIdentity without going through the query flow - and would be good to also test it out in this codepath - in case someone sends a X-Trino-Authorization-User header that way)

phd3 · 2023-02-17T16:20:31Z

core/trino-main/src/main/java/io/trino/Session.java

@@ -115,6 +117,7 @@ public Session(
        this.transactionId = requireNonNull(transactionId, "transactionId is null");
        this.clientTransactionSupport = clientTransactionSupport;
        this.identity = requireNonNull(identity, "identity is null");
+        this.originalIdentity = requireNonNull(originalIdentity, "originalIdentity is null");


You're right - but since the "executing" identity is referred to as "identity" in the code currently - we can refer the "authorization" identity as "identity" with this change ... and rename the original (or authentication) identity, wdyt ?

phd3 · 2023-02-24T21:21:07Z

Adding some discussion here that we had over slack with @kokosing :

With regards to extra credentials, we do not transfer them today in case of SECURITY DEFINER in views (which is also some kind of impersonation). So if we want to transfer them in SET SESSION AUTHORIZATION we should consider transferring it in all other forms of impersonation (like row filters, column masks). We can start with not transferring them then - we can tackle the extracreds in a common way across these sql commands if a usecase demands it.
For groups -> since they can be used for access control, we'll need to derive new groups for authorizationUser using GroupProvider#getGroups() - so that AccessControl can use proper groups corresponding to the authorizationUser for making the checks. I think this is consistent with the fact that we're using authorizationUser instead of the originalUser for making the AccessControl checks.

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

baohe-zhang · 2023-02-28T23:42:44Z

@phd3
for groups, we already have

        Identity identity = optionalAuthorizationUser.map(authorizationUser -> Identity.from(originalIdentity)
                        .withUser(authorizationUser)
                        .withGroups(groupProvider.getGroups(authorizationUser))
                        .build())
                .orElse(originalIdentity);

to get the authorizationUser's groups. I wonder do we need to change withGroups to withAdditionalGroups to preserve the original user's groups in the authorization user's identity?

cla-bot · 2023-06-28T14:59:40Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Baohe Zhang.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2023-06-28T15:01:55Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Baohe Zhang.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2023-06-28T15:18:07Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Baohe Zhang.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

phd3

LGTM! Minor comments.

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

core/trino-main/src/main/java/io/trino/SessionRepresentation.java

core/trino-main/src/main/java/io/trino/server/HttpRequestSessionContextFactory.java

phd3 · 2023-08-03T01:35:18Z

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticator.java

+            userHeader = detectProtocol(alternateHeaderName, headers.keySet()).requestOriginalUser();
+            if (headers.getFirst(userHeader) == null || headers.getFirst(userHeader).isEmpty()) {
+                userHeader = detectProtocol(alternateHeaderName, headers.keySet()).requestUser();
+            }


I'd extract this out in a method so that the logic of preferring originalUser and fallback on user remains in one place. Also let's add a comment on why we do this

docs/src/main/sphinx/sql/set-session-authorization.rst

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

client/trino-jdbc/src/test/java/io/trino/jdbc/TestTrinoDriver.java

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

core/trino-main/src/main/java/io/trino/server/HttpRequestSessionContextFactory.java

core/trino-main/src/main/java/io/trino/server/QuerySessionSupplier.java

phd3

LGTM. @baohe-zhang Could you please squash commits? Or open access for me to be able to push to your branch, which I'm currently not.

phd3 · 2023-08-31T01:17:53Z

Merged, thanks @baohe-zhang @martint !

findepi · 2023-09-14T08:42:33Z

The new statements should fail when client does not support the new headers. Please update ClientCapabilities.

findepi · 2023-09-14T19:42:58Z

The new statements should fail when client does not support the new headers. Please update ClientCapabilities.

@baohe-zhang @phd3 can you please follow-up on this?

cla-bot bot added the cla-signed label Feb 11, 2023

baohe-zhang requested a review from phd3 February 11, 2023 00:24

hashhar requested a review from martint February 13, 2023 12:15

phd3 reviewed Feb 14, 2023

View reviewed changes

baohe-zhang force-pushed the add_originalIdentity_to_session branch from e15af08 to 91ae181 Compare February 14, 2023 02:39

baohe-zhang changed the title ~~Add originalIdentity to Session to support SET SESSION AUTHORIZATION command~~ Add X-Trino-Authorization-User header to support executing query as another user Feb 14, 2023

baohe-zhang force-pushed the add_originalIdentity_to_session branch 2 times, most recently from 5ed9e0f to 5c77e88 Compare February 15, 2023 00:06

baohe-zhang changed the title ~~Add X-Trino-Authorization-User header to support executing query as another user~~ [WIP] Support SET SESSION AUTHORIZATION sql command Feb 15, 2023

baohe-zhang changed the title ~~[WIP] Support SET SESSION AUTHORIZATION sql command~~ [WIP] Support SET SESSION AUTHORIZATION Feb 15, 2023

github-actions bot added the jdbc Relates to Trino JDBC driver label Feb 16, 2023

baohe-zhang force-pushed the add_originalIdentity_to_session branch 2 times, most recently from a9a6af1 to 0f26e2c Compare February 17, 2023 02:17

baohe-zhang changed the title ~~[WIP] Support SET SESSION AUTHORIZATION~~ Support SET SESSION AUTHORIZATION and RESET SESSION AUTHORIZATION Feb 17, 2023

github-actions bot added the docs label Feb 17, 2023

kokosing reviewed Feb 17, 2023

View reviewed changes

phd3 reviewed Feb 17, 2023

View reviewed changes

baohe-zhang force-pushed the add_originalIdentity_to_session branch 2 times, most recently from a56a25e to 10d6bfc Compare February 18, 2023 00:35

phd3 reviewed Feb 24, 2023

View reviewed changes

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

baohe-zhang force-pushed the add_originalIdentity_to_session branch from 10d6bfc to fbe6f3c Compare February 28, 2023 19:39

baohe-zhang force-pushed the add_originalIdentity_to_session branch from fbe6f3c to 9d01526 Compare March 3, 2023 00:45

kokosing force-pushed the master branch from 3f05134 to 58d6356 Compare March 14, 2023 11:34

baohe-zhang force-pushed the add_originalIdentity_to_session branch from 9d01526 to 46d49e7 Compare March 17, 2023 22:04

baohe-zhang mentioned this pull request Mar 18, 2023

Support SET SESSION AUTHORIZATION on trino-python-client trinodb/trino-python-client#349

Merged

baohe-zhang force-pushed the add_originalIdentity_to_session branch from 46d49e7 to 8c8e2b9 Compare March 18, 2023 08:06

baohe-zhang force-pushed the add_originalIdentity_to_session branch from 8c8e2b9 to 087e2ec Compare April 22, 2023 00:07

baohe-zhang force-pushed the add_originalIdentity_to_session branch from 00ef8b5 to c27b7db Compare June 28, 2023 14:59

baohe-zhang force-pushed the add_originalIdentity_to_session branch from c27b7db to eac060d Compare June 28, 2023 15:01

baohe-zhang force-pushed the add_originalIdentity_to_session branch from eac060d to 47e8055 Compare June 28, 2023 15:18

baohe-zhang force-pushed the add_originalIdentity_to_session branch from 47e8055 to 2a97e95 Compare June 28, 2023 15:21

cla-bot bot added the cla-signed label Jun 28, 2023

baohe-zhang force-pushed the add_originalIdentity_to_session branch 4 times, most recently from e71d0b0 to 0c73513 Compare June 29, 2023 08:21

baohe-zhang force-pushed the add_originalIdentity_to_session branch 2 times, most recently from d1b4b76 to 9011784 Compare July 12, 2023 23:26

phd3 approved these changes Aug 4, 2023

View reviewed changes

baohe-zhang force-pushed the add_originalIdentity_to_session branch 2 times, most recently from 2748bf4 to ce689dc Compare August 15, 2023 23:38

phd3 approved these changes Aug 29, 2023

View reviewed changes

Support SET SESSION AUTHORIZATION and RESET SESSION AUTHORIZATION

4a2a489

baohe-zhang force-pushed the add_originalIdentity_to_session branch from ce689dc to 4a2a489 Compare August 29, 2023 05:54

phd3 merged commit 6c35c9c into trinodb:master Aug 31, 2023
89 checks passed

github-actions bot added this to the 426 milestone Aug 31, 2023

findepi mentioned this pull request Sep 1, 2023

Support SET SESSION AUTHORIZATION #2512

Closed

colebow mentioned this pull request Sep 5, 2023

Add Trino 426 release notes #18867

Merged

b-slim mentioned this pull request Oct 1, 2023

Enforce Client capabilities when Session Authorization is not supported #19217

Merged

findepi mentioned this pull request Nov 15, 2023

Fix X-Trino-Authorization-User typo #19763

Merged

hashhar mentioned this pull request Feb 22, 2024

Add a session property to set the user #2308

Closed

Support SET SESSION AUTHORIZATION and RESET SESSION AUTHORIZATION #16067

Support SET SESSION AUTHORIZATION and RESET SESSION AUTHORIZATION #16067

Conversation

baohe-zhang commented Feb 11, 2023 • edited by phd3 Loading

Description

Additional context and related issues

Release notes

phd3 commented Feb 14, 2023

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

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

phd3 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phd3 commented Feb 24, 2023

baohe-zhang commented Feb 28, 2023

cla-bot bot commented Jun 28, 2023

cla-bot bot commented Jun 28, 2023

cla-bot bot commented Jun 28, 2023

phd3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phd3 left a comment

Choose a reason for hiding this comment

phd3 commented Aug 31, 2023

findepi commented Sep 14, 2023

findepi commented Sep 14, 2023

baohe-zhang commented Feb 11, 2023 •

edited by phd3

Loading