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

KNOX-3028 - add support for OAuth Token Exchange to KNOXTOKEN #900

Merged
merged 3 commits into from
Apr 15, 2024

Conversation

lmccay
Copy link
Contributor

@lmccay lmccay commented Apr 9, 2024

What changes were proposed in this pull request?

This change will extend the existing TokenResource for KNOXTOKEN service to include OAuth specifics such as expected URL, error messages and flows to support Token Exchange Flow and Token Refresh.

This is being driven by a specific need to proxy access to the Iceberg REST Catalog API. In this specific usecase, we need to intercept the use of the following endpoint URLs and serve the token exchange flow for the authenticating user.

/v1/oauth/tokens
Details for these requirements can be found in the openapi description for the catalog API [1].

In addition to this usecase, we should add generic support for the token exchange flow with more generic URL that better aligns with what others use.

/oauth/v1/token
We will support the use of the "oauth" service name within the existing KNOXTOKEN service with an extension of the TokenResource which adapts the existing KNOXTOKEN behavior to the expectations of clients on OAuth responses.

In order to support both URLs, the deployment contributor will need to register a url pattern for each usecase and the resource path within the jersey service will need to accommodate the dynamic nature of the Iceberg REST Catalog API which will add the catalog API service name as well.

/icecli/v1/oauth/tokens/
Where "icecli" may be some configurable service name and need to match to the incoming URL.
We will wildcard that by making it a regex matched path param.

We will also need to accommodate a first-class Knox pattern and service name of "oauth" and only allow "token" or "oauth" after the v1 with the remaining path fragment being optional for the iceberg specific "tokens".

Not pretty but it will work.

  1. https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml

How was this patch tested?

Ran existing tests and added a new unit test to existing TokenServiceResourceTest for OAuth token changes

Also exercised the API with various curl commands for the supported URLs:

curl -ivku admin:admin-password -X POST https://localhost:8443/gateway/sandbox/oauth/v1/token - SUCCESS
curl -ivku admin:admin-password -X POST https://localhost:8443/gateway/sandbox/oauth/v1/tokens - 404
curl -ivku admin:admin-password -X POST https://localhost:8443/gateway/sandbox/oauthsdf/v1/token - 404
curl -ivku admin:admin-password -X POST https://localhost:8443/gateway/sandbox/oauth/v1/token - 404
curl -ivku admin:admin-password -X POST https://localhost:8443/gateway/sandbox/mdsbfdsf/v1/oauth/tokens - SUCCESS

Note that the above pattern with arbitrary service name is required for the Iceberg REST Catalog API.

The above represent the expected behavior.

Please review Knox Contributing Process before opening a pull request.

Copy link
Contributor

@pzampino pzampino left a comment

Choose a reason for hiding this comment

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

This is kind of difficult to review with all the refactoring, but I'm getting there. I can certainly see how challenging this was. I've gone through a good bit of it, and I'm hoping our test coverage is sufficient to have caught any errors. I will continue some review, but I need to give my eyes a break.

  1. There are a number of new inner classes which allow direct access to their fields. I'm curious why this is done versus more typical getter methods.
  2. I don't love the name of the TokenResponse class, but I can't think of a more appropriate name at the moment.
  3. Seems like there are a bunch of formatting changes, where things don't quite line up like they used to, but perhaps that's just the review presentation.

Copy link
Contributor

@pzampino pzampino left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work on the refactoring.

@lmccay lmccay merged commit d74fb4f into apache:master Apr 15, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants