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.

Sign up for GitHub

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

Jump to bottom

Remove dependency on Dropwizard Auth #289

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove dependency on Dropwizard Auth #289

wants to merge 1 commit into from

Conversation

oneonestar
Copy link
Member

Description

Relates to #41

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

@cla-bot cla-bot bot added the cla-signed label Mar 24, 2024
@oneonestar oneonestar marked this pull request as draft March 24, 2024 08:00
@oneonestar oneonestar mentioned this pull request Mar 24, 2024
Open
@oneonestar oneonestar force-pushed the star/remove_dropwizard_auth branch 4 times, most recently from 903873c to bce9cbf Compare March 25, 2024 02:15
@oneonestar oneonestar force-pushed the star/remove_dropwizard_auth branch 9 times, most recently from 3074be9 to b484926 Compare April 11, 2024 23:58
willmostly
willmostly reviewed Apr 12, 2024
View reviewed changes
Copy link
Contributor

@willmostly willmostly left a comment

Choose a reason for hiding this comment

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

Just skimmed and looks good, I will take another pass next week.

Have you tested this change against an OAuth2 server? I'm not overly confident in the mock tests.

gateway-ha/src/main/java/io/trino/gateway/ha/security/util/BasicCredentials.java
import static java.nio.charset.StandardCharsets.ISO_8859_1;
import static java.util.Objects.requireNonNull;

public record BasicCredentials(String username, String password)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to use io.trino.server.security.BasicAuthCredentials here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried that before. There were few problems:

Some Trino authentication logic are mixed into BasicAuthCredentials. The username must not be empty, otherwise it will throw (BasicAuthCredentials.java#L61-L63). That's not required by RFC 2617/7617.

It mixed AuthenticationException and Optional.Empty() to indicate different parsing errors, in order to satisfy different use cases in Trino. That's not needed by gateway and makes the caller's logic become complicated.

It doesn't have unit test coverage in Trino. I don't feel safe to extract and reuse that particular class.

@oneonestar oneonestar force-pushed the star/remove_dropwizard_auth branch 2 times, most recently from 2793b86 to 9416953 Compare April 13, 2024 11:05
@oneonestar
Copy link
Member Author

Have you tested this change against an OAuth2 server? I'm not overly confident in the mock tests.

I didn't. Overall I don't feel we have enough tests to cover the auth part. That's why I'm holding this PR as a draft.
I planned to add some tests in a separate PR to ensure the auth doesn't break during migration.
In particular I'd like to test /findQueryHistory and /userinfo endpoint, LDAP authentication and authorization, and OAuth2.

@oneonestar oneonestar force-pushed the star/remove_dropwizard_auth branch 4 times, most recently from c8e97ca to d288fe2 Compare April 17, 2024 01:20
@oneonestar oneonestar marked this pull request as ready for review April 18, 2024 05:11
@oneonestar
Copy link
Member Author

Test on /userinfo added in #321.
Done a manual test on LDAP and OAuth2.
Not sure how to automate the test / mock the required components.

@willmostly
Copy link
Contributor

I'm not that familiar with it unfortunately, but looks like the Trino OAuth2 tests use Hydra

@oneonestar
Copy link
Member Author

Found some security issues in the current OIDC implementation while I was adding the test using Hydra.
#344 #345 should fix the issues.

@mosabua
Copy link
Member

mosabua commented May 21, 2024

Lets chat about all this in the sync tomorrow. I think we should add them all in.

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

@willmostly willmostly willmostly left review comments

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@oneonestar @willmostly @mosabua