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 pluggable Custom Presto Authenticators #24111

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

imsayari404
Copy link
Contributor

@imsayari404 imsayari404 commented Nov 21, 2024

Description

A pluggable authenticator in Presto allows the authentication process to be customized based on specific use cases, such as integrating with different identity providers or token validation strategies.

Motivation and Context

Fixes #24052

Test Plan

Added Unit Tests

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Security Changes
* Add support for pluggable Custom Presto Authenticators :pr:`#24111`

@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Nov 21, 2024
@prestodb-ci prestodb-ci requested review from a team and pramodsatya and removed request for a team November 21, 2024 10:39
Copy link
Contributor

@pramodsatya pramodsatya left a comment

Choose a reason for hiding this comment

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

Thanks @imsayari404, could you please add a test? Will the JwtAuthenticator and PasswordAuthenticator be refactored, as suggested in #24053 (review), in this PR?


import static java.util.Objects.requireNonNull;

public class PrestoAuthenticator
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PrestoAuthenticator class does not need to be an interface because it serves as a concrete adapter between the Airlift Authenticator interface and the pluggable design provided by PrestoAuthenticator and PrestoAuthenticatorFactory SPI. Its role is to delegate authentication to the PrestoAuthenticatorManager.

Copy link
Member

Choose a reason for hiding this comment

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

@pramodsatya I understand why you might be confused. We have an interface called PrestoAuthenticator in the presto-spi package with the same name.

@tdcmeehan Currently, we've followed the same naming convention as was done for the PasswordAuthenticator SPI. This could lead to confusion since we have an SPI with the same name as the PasswordAuthenticator class in the presto-main package. Do you think we should refactor the class names in presto-main to eliminate the confusion?

@tdcmeehan tdcmeehan self-assigned this Nov 21, 2024
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

Thanks for the doc! A couple of rephrasing suggestions for conciseness.

Also - because this is a new page - you must add

develop/presto-authenticator

to https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/develop.rst

so the new page will show up on the Developer Guide index page https://prestodb.io/docs/current/develop.html .

Custom Presto Authenticator
===========================

Presto supports authentication via a custom presto authenticator
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Presto supports authentication via a custom presto authenticator
Presto supports authentication through a custom Presto authenticator

Don't use via. See the GitLab documentation style guide word list entry for via for discussion and alternate working suggestions, if the suggested through isn't a good fit for you.

Implementation
--------------

``PrestoAuthenticatorFactory`` is responsible for creating a
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
``PrestoAuthenticatorFactory`` is responsible for creating a
``PrestoAuthenticatorFactory`` creates a

Additionally, the coordinator must be configured to use custom authentication
and have HTTPS enabled.

Below property needs to be added to the coordinator's ``config.properties`` file:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Below property needs to be added to the coordinator's ``config.properties`` file:
Add the property shown below to the coordinator's ``config.properties`` file:

Co-authored-by: Namya Sehgal <sehgalnamya@gmail.com>
Add TestingPrestoAuthenticatorFactory
@imjalpreet
Copy link
Member

Will the JwtAuthenticator and PasswordAuthenticator be refactored, as suggested in #24053 (review), in this PR?

@pramodsatya There has been a slight change since my last comment on this issue. After discussions with @tdcmeehan, we introduced a new and more generic SPI that is independent of the current Authenticators. This SPI can be used to implement any type of custom authenticator, without being limited to just a custom implementation of JWT or Password Authenticators.

We decided to keep the current implementation of Password and JWT authenticators unchanged to avoid introducing a breaking change.

@imjalpreet imjalpreet marked this pull request as ready for review November 25, 2024 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:IBM PR from IBM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pluggable JWT Authenticator in Presto
6 participants