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 authentication to the Preview Web UI #23230

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

Conversation

koszti
Copy link
Member

@koszti koszti commented Sep 2, 2024

Description

Authentication methods for the preview UI. Partially addressing #22697

Technical details

Introducing three new API endpoints designed for enhanced Single Page App compatibility:

  • GET: /ui/preview/auth/info: Provides a lightweight JSON response containing the essential details for authentication type, user information, and login form rendering (if needed). Unlike the current approach, it eliminates the need for server-side HTML rendering and string manipulation.. Example:
$ curl -k https://127.0.0.1:8443/ui/preview/auth/info
{"authType":"form","isPasswordAllowed":true,"isAuthenticated":false}
  • POST: /ui/preview/auth/login: Functionally equivalent to POST: /ui/login, but it exchanges data via JSON instead of HTTP forms and HTML responses.
  • GET: /ui/preview/auth/logout: Functionally equivalent to GET: /ui/logout but it exchanges data via JSON instead of HTTP redirects and HTML response

Supported Authentication Types

  • INSECURE
  • FORM (via HTTP)
  • FORM (via HTTPS)
  • FIXED
  • OAUTH2 (tested with Okta)
  • JWT: - (tested by ingesting jwt tokens into HTTP header as auth bearer token using mitmproxy)
  • KERBEROS
  • CERTIFICATE

Screenshots

1. INSECURE auth type for development. No password.
image

2. FORM auth type via HTTPS
image

3. FIXED
image

4. OAUTH2 Okta as IdP
image

NOTE: In oauth2, the IdP login form is displayed (such as okta login page), whereas for fixed and protocol-based authenticators (jwt, certifacte and kerberos), users are redirected to the main page upon successful authentication without being shown a login form.

5. Periodically fetching /ui/api/stats endpoint after a successful login
image

@cla-bot cla-bot bot added the cla-signed label Sep 2, 2024
@koszti koszti changed the title Webapp Preview: Authentication [WiP] Webapp Preview: Authentication Sep 2, 2024
@github-actions github-actions bot added the ui Web UI label Sep 2, 2024
@koszti koszti force-pushed the webapp-preview-auth branch from 67f054b to 03ae4a8 Compare September 2, 2024 21:25
@koszti koszti force-pushed the webapp-preview-auth branch 5 times, most recently from 3895cf2 to 92310b1 Compare September 14, 2024 21:41
@wendigo wendigo self-requested a review September 15, 2024 05:04
@koszti koszti force-pushed the webapp-preview-auth branch 6 times, most recently from ef2ee0e to f5f4331 Compare September 17, 2024 08:32
@koszti koszti changed the title [WiP] Webapp Preview: Authentication Webapp Preview: Authentication Sep 17, 2024
@koszti koszti force-pushed the webapp-preview-auth branch 2 times, most recently from 6c48bf4 to 8e3d739 Compare September 17, 2024 20:36
@koszti koszti force-pushed the webapp-preview-auth branch from 8e3d739 to ce604df Compare September 30, 2024 09:05
@koszti koszti force-pushed the webapp-preview-auth branch from 75666ed to e2eb9cd Compare October 10, 2024 07:45
@github-actions github-actions bot added release-notes docs jdbc Relates to Trino JDBC driver hudi Hudi connector iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector labels Oct 10, 2024
@koszti koszti force-pushed the webapp-preview-auth branch from a18540f to d9489a3 Compare November 18, 2024 00:11
@wendigo wendigo force-pushed the webapp-preview-auth branch from da79342 to d805fac Compare November 18, 2024 13:24
@wendigo
Copy link
Contributor

wendigo commented Nov 18, 2024

@koszti I've pushed a commit to your PR that tries to simplify the bindings to reduce the scope of changes. Ideally we don't want to change existing classes at all to reduce the chance of introducing security-related bugs.

@koszti koszti force-pushed the webapp-preview-auth branch 4 times, most recently from 0297479 to 46154d2 Compare November 18, 2024 22:43
@koszti
Copy link
Member Author

koszti commented Nov 18, 2024

@koszti I've pushed a commit to your PR that tries to simplify the bindings to reduce the scope of changes. Ideally we don't want to change existing classes at all to reduce the chance of introducing security-related bugs.

thanks for doing that. I was struggling with that part so it was a great help. I made a few other modifications and everything is now working. I've also addressed nearly all your comments and rebased everything into a single commit.

@wendigo
Copy link
Contributor

wendigo commented Dec 12, 2024

@koszti I've pushed a bunch of changes - can you test them? I'm ok with the change as it is right now so I'm willing to merge it.

@koszti
Copy link
Member Author

koszti commented Dec 13, 2024

@wendigo I'm fixing conflicts and testing your commits. I can see you've moved @ResourceSecurity annotations to class level but that causes this exception:

Caused by: IllegalArgumentException: Trino resource is not annotated with @ResourceSecurity: public AuthInfo LoginPreviewResource.getAuthInfo(ContainerRequestContext,SecurityContext)

I've checked other resources and seems like it's always annotated at the endpoint methods and not at the class level. One annotation for each.. Should I modify related classes accordingly so it'd look like this below? Doing that there is no exception:

@Path("")
@Consumes(APPLICATION_JSON)
@Produces(APPLICATION_JSON)
public class LoginPreviewResource
{
...
    @GET
    @Path(UI_PREVIEW_AUTH_INFO)
    @ResourceSecurity(WEB_UI)
    public AuthInfo getAuthInfo(...)
...

    @POST
    @Path(UI_PREVIEW_LOGIN_FORM)
    @ResourceSecurity(WEB_UI)
    public Response login(...)
...

@wendigo
Copy link
Contributor

wendigo commented Dec 13, 2024

@koszti according to the code it should work on the class-level as well

@koszti
Copy link
Member Author

koszti commented Dec 13, 2024

@koszti according to the code it should work on the class-level as well

io.trino.server.ui.PreviewUiQueryRunner isn’t starting with class-level annotations. I’ll try to investigate the cause.
I need a bit more time to resolve all conflicts and align everything with the master branch but will do it in the next couple of days.

@wendigo
Copy link
Contributor

wendigo commented Dec 13, 2024

@koszti there is an invalid check in the ResourceAccessType. There should be a single verifyNotTrinoResource call there by the end of the method. Can you test with that change?

diff --git a/core/trino-main/src/main/java/io/trino/server/security/ResourceAccessType.java b/core/trino-main/src/main/java/io/trino/server/security/ResourceAccessType.java
index 9fe79418b35..b71a2d0b9e0 100644
--- a/core/trino-main/src/main/java/io/trino/server/security/ResourceAccessType.java
+++ b/core/trino-main/src/main/java/io/trino/server/security/ResourceAccessType.java
@@ -49,7 +49,6 @@ public class ResourceAccessType
             // check if the resource class has an access type declared for all methods
             accessType = resourceAccessTypeLoader.getAccessType(resourceInfo.getResourceClass());
             if (accessType.isPresent()) {
-                verifyNotTrinoResource(resourceInfo);
                 return accessType.get();
             }
             // in some cases there the resource is a nested class, so check the parent class
@@ -57,7 +56,6 @@ public class ResourceAccessType
             if (resourceInfo.getResourceClass().getDeclaringClass() != null) {
                 accessType = resourceAccessTypeLoader.getAccessType(resourceInfo.getResourceClass().getDeclaringClass());
                 if (accessType.isPresent()) {
-                    verifyNotTrinoResource(resourceInfo);
                     return accessType.get();
                 }
             }

@koszti
Copy link
Member Author

koszti commented Dec 13, 2024

@wendigo yes, it does work after your changes, thanks! I'm going to continue the rest and will come back soon

@koszti koszti force-pushed the webapp-preview-auth branch 4 times, most recently from a3f0d91 to 2ccf6ec Compare December 16, 2024 01:00
@koszti koszti changed the title Webapp Preview: Authentication Add authentication to the Preview Web UI Dec 16, 2024
@koszti koszti force-pushed the webapp-preview-auth branch from dd3d28b to 2ccf6ec Compare December 16, 2024 01:14
@koszti
Copy link
Member Author

koszti commented Dec 16, 2024

@wendigo this is now ready.

@wendigo wendigo force-pushed the webapp-preview-auth branch from 2ccf6ec to 4d750ff Compare December 16, 2024 10:58
@wendigo
Copy link
Contributor

wendigo commented Dec 16, 2024

@koszti there were unrelated changes in 3 files which I removed and extracted one change to a separate commit.

@@ -49,15 +49,13 @@ public AccessType getAccessType(ResourceInfo resourceInfo)
// check if the resource class has an access type declared for all methods
accessType = resourceAccessTypeLoader.getAccessType(resourceInfo.getResourceClass());
if (accessType.isPresent()) {
verifyNotTrinoResource(resourceInfo);
Copy link
Contributor

Choose a reason for hiding this comment

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

@dain this prohibited @ResourceSecurity at the class level for io.trino Jax-RS resources. Was it on purpose?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bigquery BigQuery connector cla-signed delta-lake Delta Lake connector docs hive Hive connector hudi Hudi connector iceberg Iceberg connector jdbc Relates to Trino JDBC driver mongodb MongoDB connector ui Web UI
Development

Successfully merging this pull request may close these issues.

3 participants