Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate getting groups in Oauth2 authentication #15669

Merged
merged 1 commit into from Feb 22, 2023

Conversation

kokosing
Copy link
Member

@kokosing kokosing commented Jan 11, 2023

Deprecate getting groups in Oauth2 authentication

This feature is not working properly and it is not possible to fix it.
The problem is widely understood impersonation:

  • view SECURITY DEFINER
  • sessionUser JDBC parameter
  • identity from view expression for column masking and row filtering

Using Oauth2 is able to provide groups only during authentication.
However it is unable to provide groups for any impersonated user.

Release notes

  • Configuration property http-server.authentication.oauth2.groups-field got deprecated and it is about to be removed. In order to discourage the usage of it, it was renamed to deprecated.http-server.authentication.oauth2.groups-field.

@cla-bot cla-bot bot added the cla-signed label Jan 11, 2023
@kokosing
Copy link
Member Author

@trinodb/maintainers This is up to the discussion. I would like to hear your opinion about how and if we should retreat this feature.

@kokosing
Copy link
Member Author

CC: @lukasz-walkiewicz

@github-actions github-actions bot added the docs label Jan 11, 2023
Copy link
Contributor

@ksobolew ksobolew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I applaud this change ;)

@kokosing kokosing marked this pull request as draft January 12, 2023 08:10
@mrveera
Copy link

mrveera commented Jan 14, 2023

@kokosing How to get support for groups while using OAuth2 / any other authenticator . Not everyone has static list of groups file or list of groups that can fit in one file. if we create provider which calls api to get groups for user it will be one extra unnecessary http call.

@lukasz-walkiewicz
Copy link
Member

@kokosing How to get support for groups while using OAuth2 / any other authenticator . Not everyone has static list of groups file or list of groups that can fit in one file. if we create provider which calls api to get groups for user it will be one extra unnecessary http call.

You should not rely on authentication method to provide group information. As in the description, it causes many problems when impersonation is involved as the user running a query is a different user than the one authenticated so information about group membership is either lost or incorrect.

@lukasz-walkiewicz
Copy link
Member

Changes to suite-oauth2 are needed.

This feature is not working properly and it is not possible to fix it.
The problem is widely understood impersonation:
 - view SECURITY DEFINER
 - sessionUser JDBC parameter
 - identity from view expression for column masking and row filtering

Using Oauth2 is able to provide groups only during authentication.
However it is unable to provide groups for any impersonated user.
@kokosing
Copy link
Member Author

CC: @trinodb/maintainers

@kokosing kokosing marked this pull request as ready for review February 20, 2023 15:08
@kokosing kokosing merged commit 65a8f11 into trinodb:master Feb 22, 2023
@kokosing kokosing deleted the origin/master/163_groups branch February 22, 2023 11:10
@github-actions github-actions bot added this to the 408 milestone Feb 22, 2023
@connorlwilkes
Copy link

connorlwilkes commented Feb 27, 2023

Apologies for the confusion but what is the intended way to now resolve groups from authenticated users. Are we expected to utilize the file group provider? That doesn't seem ideal to me.

It is mentioned that you cannot resolve groups properly due to impersonation but does this also not just also apply in the same way if you used a file based provider? I don't really understand @Praveen2112

@Praveen2112
Copy link
Member

Impersonation is wrt to views with SECURITY DEFINER - In case of StatementAnalyzer#analyzeView we use groupProvider once again to identify the group of the impersonated user which could be a file based one, it is also pluggable.

@kokosing
Copy link
Member Author

Are we expected to utilize the file group provider?

GroupProvider is defined in Trino SPI. File based group provider is one of its implementations. We need to implement a new one that would be able retrieve groups from Oauth2. However this protocol is not designed to serve this kind of information.

Maybe you can somehow dump information about groups from your IdP to the file and and then file based group provider would be enough? WDYT?

@fabland
Copy link

fabland commented Feb 27, 2023

I just tripped over this change when going from 407 to 408. I am using Keycloak OIDC with trino and groups in the auth token to map users to groups and then file based access control to specify which groups can access what. I wasn't really using any impersonation functionality. If this feature is deprecated, it would be good to describe how to achieve something similar. Seems like having to develop something to dump information from the IDP to some file and then sync that to trino is a bit complicated?

@kokosing
Copy link
Member Author

This feature is not entirely removed. It is deprecated so the config property got renamed to deprecated.http-server.authentication.oauth2.groups-field and documentation for it was removed. You can still use it and it will be there for a while. I just don't want to avoid new usages of it as it will be harder to remove it eventually.

So far there is no date where it will be removed. It is not a big problem to keep it.

@kokosing
Copy link
Member Author

kokosing commented Mar 7, 2023

In case this pull request caused issues on your end. Please see and follow release notes to unblock your installations.

The feature was not removed, it got deprecated. There are no plans to remove it so far. Impersonation is generic term which applies to SET SESSION AUTHORIZATION statement, view SECURITY DEFINER, row filtering and column masking. Also it can refer to impersonating user in remote data source, however this is not affected by groups.

There are cases where this is was used in access control and it looks like it works “good enough” for many installations. However it it is not working fully and there is no way to fix it in current design, hence we had to deprecate and it will be retreated eventually. It got deprecated to avoid new users to onboard with this feature and also to make current users aware of the problem.

I guess mentioned installations are not using views with SECURITY DEFINER, row filters or column masks where access control is configured with using groups assigned to the view owner. Using views is a common thing and that case is not working at all as we are not able to provide groups for view owner because we can obtain groups only during authentication flow for session user.

@kokosing
Copy link
Member Author

There is an idea where we could re-introduce this feature, but it would require implementation of #16539.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

7 participants