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

Add support for User Impersonation #309

Merged
merged 1 commit into from
Feb 26, 2020

Conversation

Ralnoc
Copy link
Contributor

@Ralnoc Ralnoc commented Feb 7, 2020

Add ability to declare principle username to support User Impersonation within Presto. This allows the authentication with an user configured for supporting username assumption so that all queries run as the said user.

@elukey elukey mentioned this pull request Feb 20, 2020
pyhive/presto.py Show resolved Hide resolved
@Ralnoc
Copy link
Contributor Author

Ralnoc commented Feb 20, 2020

Absolutely! I actually am going to review my changes as well to make sure no additional work is needed.

The specific use case for this scenario is that WebUIs which leverage pyhive to interact with Presto are more often using SAML for authentication. Because of this, they have a username for the user, but no password because that is not communicated in the SAML Assertion payload. In that situation, when securing Presto they configure a service account to authenticate against Presto with.

Unfortunately this means all security is applied based on that service account, and prevents you from creating user specific permissions when accessing data or user specific resource groups for controlling how much cluster resources they can leverage.

With this change in place, it allows you to authenticate with the service account that is passed through by the application, and then tell Presto to execute the query as if it is run by the principle username. As long as Presto has been configured to allow the service account to execute queries as the principle username, presto will do so, applying the specific permissions and resource groups that are assign to that user.

This allows you to leverage a user based resource allocation for the cluster and a user based permission model on the data being accessed through Presto.

@elukey
Copy link
Contributor

elukey commented Feb 21, 2020

I have a similar use case, really interested in this pr :)

@Ralnoc Ralnoc changed the title Add support to User Impersonation Add support for User Impersonation Feb 22, 2020
@Ralnoc Ralnoc force-pushed the add_presto_user_impersonation branch from 14cef34 to adc3171 Compare February 22, 2020 13:01
@Ralnoc
Copy link
Contributor Author

Ralnoc commented Feb 22, 2020

@bkyryliuk -
I've confirmed that this code change should cover the specific use case and updated the docstring a bit to be more accurate.

In addition, here is the Presto documentation as it relates to this feature: https://docs.starburstdata.com/latest/security/impersonation.html

Please let me know if you would like me to add any more information to the PR.

@elukey
Copy link
Contributor

elukey commented Feb 25, 2020

@bkyryliuk the CI seems having problems with Python 2.7, not sure why, but the pull request seems good.

@bkyryliuk
Copy link
Collaborator

@elukey @Ralnoc - could you rebase the diff ? Master is green.
I am very not familiar with this flow and will land it once the build is fixed.
However could you explain here or in code why it is not possible to use username for this use case?
Looking into the implementation of this feature it seems to me that you can just set username & achieve the same goals. Probably I am missing smth obvious.

@Ralnoc Ralnoc force-pushed the add_presto_user_impersonation branch from adc3171 to a57a9e7 Compare February 26, 2020 18:47
@codecov
Copy link

codecov bot commented Feb 26, 2020

Codecov Report

Merging #309 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #309   +/-   ##
======================================
  Coverage    93.2%   93.2%           
======================================
  Files          14      14           
  Lines        1500    1500           
  Branches      164     164           
======================================
  Hits         1398    1398           
  Misses         75      75           
  Partials       27      27
Impacted Files Coverage Δ
pyhive/presto.py 86.36% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9265b58...11086b4. Read the comment docs.

@Ralnoc
Copy link
Contributor Author

Ralnoc commented Feb 26, 2020

@bkyryliuk -
I've rebased on master which appears to have resolved the testing issues and added a comment to the code that details the need and use case for this change. Please let me know if you need more information.

Add ability to declare principle username to support User Impersonation within Presto. This allows the authentication with an user configured for supporting username assumption so that all queries run as the said user.
@Ralnoc Ralnoc force-pushed the add_presto_user_impersonation branch from 333287a to 11086b4 Compare February 26, 2020 20:52
@bkyryliuk
Copy link
Collaborator

@Ralnoc this is great! Thank you

@bkyryliuk bkyryliuk merged commit 50c5ef7 into dropbox:master Feb 26, 2020
@Ralnoc
Copy link
Contributor Author

Ralnoc commented Feb 27, 2020

Glad to help! Going to dig into using this in a few places as soon as the next version releases.

@tooptoop4
Copy link

@Ralnoc do u use superset? i'm not sure what apache/superset#3404 does if your PR was only just merged now. i'm trying to solve apache/superset#7170

@Ralnoc
Copy link
Contributor Author

Ralnoc commented Apr 16, 2020

@tooptoop4 -

I believe Superset uses SQLAlchemy to interact with Presto. I haven't looked deeper to see if it leverages PyHive through SQLAlchemy. That being said, reading over that issue in Superset, it looks like they implemented impersonation slightly differently. I'd have to read the code more in depth to see exactly how.

I would suggest you make sure that you have the Impersonation Rules defined in Presto. https://docs.starburstdata.com/latest/security/impersonation.html#user-translation-rules Without those rules in place, I believe you will get a 401 error as the attempt to execute a query as another user is denied.

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

Successfully merging this pull request may close these issues.

4 participants