User impersonation does not always load groups

[vagaerg]

Similar to #12953

Depending on the way a user attempts to impersonate another, the checkCanImpersonateUser method on System Access Control SPI may get the original (authenticated principal) user's groups or not.

• Using a session user (e.g. --session-user on the CLI): the authenticated identity's groups are not loaded
• Using SET SESSION AUTHORIZATION: the authenticated identity's groups are loaded - I imagine because the authenticated identity had a session already, which they used to execute the SET SESSION AUTHORIZATION query in the first place

User roles are always loaded such that rules can be written to control impersonation privileges based on roles. However, the same is not done for user groups - which is problematic when trying to move all authorization to a system like OPA (#19532 )

Three ideas that come to mind to fix this:

1. Implement group retrieval on the SessionContext factory and on Session build

There seem to be 2 places where groups would need to be loaded:

• extractAuthorizedIdentity in HttpRequestSessionContextFactory: used in several different contexts including controlling UI operations

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

  Lines 217 to 226 in ba88ce0

  	optionalAuthenticatedIdentity.ifPresent(authenticatedIdentity -> {
  	// only check impersonation if authenticated user is not the same as the explicitly set user
  	if (!authenticatedIdentity.getUser().equals(originalIdentity.getUser())) {
  	// load enabled roles for authenticated identity, so impersonation permissions can be assigned to roles
  	authenticatedIdentity = Identity.from(authenticatedIdentity)
  	.withEnabledRoles(metadata.listEnabledRoles(authenticatedIdentity))
  	.build();
  	accessControl.checkCanImpersonateUser(authenticatedIdentity, originalIdentity.getUser());
  	}
  	});

• createSession in QuerySessionSupplier: used when building the actual session

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

  Lines 78 to 95 in ba88ce0

  	if (context.getAuthenticatedIdentity().isPresent()) {
  	Identity authenticatedIdentity = context.getAuthenticatedIdentity().get();
  	// only check impersonation if authenticated user is not the same as the explicitly set user
  	if (!authenticatedIdentity.getUser().equals(originalIdentity.getUser())) {
  	// add enabled roles for authenticated identity, so impersonation permissions can be assigned to roles
  	authenticatedIdentity = addEnabledRoles(authenticatedIdentity, context.getSelectedRole(), metadata);
  	accessControl.checkCanImpersonateUser(authenticatedIdentity, originalIdentity.getUser());
  	}
  	}
  	Identity identity = context.getIdentity();
  	if (!originalIdentity.getUser().equals(identity.getUser())) {
  	// When the current user (user) and the original user are different, we check if the original user can impersonate current user.
  	// We preserve the information of original user in the originalIdentity,
  	// and it will be used for the impersonation checks and be used as the source of audit information.
  	accessControl.checkCanSetUser(originalIdentity.getPrincipal(), identity.getUser());
  	accessControl.checkCanImpersonateUser(originalIdentity, identity.getUser());
  	}

If we implement group retrieval in both these places, authorization checks should be able to use them

For a draft, see: https://github.com/bloomberg/trino/tree/add-groups-for-impersonation-in-context-and-session
Link to the commit: bloomberg@8f3efba

2. Implement impersonation checks on the SessionContext factory

I am not as familiar with this part of the code, so please correct me if I'm wrong.

It seems to me that SessionContext is built by HttpRequestSessionContextFactory, and most fields are final. As such, we could just refuse to build a SessionContext for this query if the user is unable to impersonate the target user - and remove any impersonation checks in createSession

If we want to add other SessionContextFactory's in the future, they would need to behave in the same way, as the session would no longer be responsible for such impersonation checks.

Would this be an option?

For a draft, see: https://github.com/bloomberg/trino/tree/add-groups-for-impersonation-in-context
Link to the commit: bloomberg@b015266

3. Loading groups when retrieving the authenticated identity

We could add groups to the global authenticated identity set on the servlet request by the AuthenticationFilter

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

Line 105 in ba88ce0

setAuthenticatedIdentity(request, authenticatedIdentity);

While this feels like a nice addition, I feel like it is against the way that the rest of the codebase works (i.e. the servlet utils are quite lightweight and all operations around groups are done at the session level)