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

[REST Catalog] OAuth 2 grant type "refresh_token" not implemented #12196

Open
2 of 3 tasks
nika-qubit opened this issue Feb 7, 2025 · 6 comments · May be fixed by #12362
Open
2 of 3 tasks

[REST Catalog] OAuth 2 grant type "refresh_token" not implemented #12196

nika-qubit opened this issue Feb 7, 2025 · 6 comments · May be fixed by #12362
Labels
improvement PR that improves existing functionality

Comments

@nika-qubit
Copy link

nika-qubit commented Feb 7, 2025

Apache Iceberg version

1.7.1 (latest release)

Query engine

None

Feature Request / Improvement

For the REST Catalog service (after deprecating the oauth endpoint), the refresh token flow (https://datatracker.ietf.org/doc/html/rfc6749#section-6) is not supported yet. The current supported flow is "token-exchange" and used as a way to refresh tokens: https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java#L163.

The refresh token flow should be taking in:

  1. Basic auth header: "Authorization: Basic ${base64_encoded_colon_separated_client_id_and_secret}" (depending on the authorization server implementation, sometimes Bearer token also works).
  2. Form data: "grant_type=refresh_token&refresh_token=..." (optionally plaintext client id and secret if not provided through auth header).

A proposal to support this flow is to allow providing:

  1. refresh_token
  2. client id and secret (or a base64 encoded string of both)

through properties similar to credential or token to work together with the oauth2-server-uri property.

Willingness to contribute

  • I can contribute a fix for this bug independently
  • I would be willing to contribute a fix for this bug with guidance from the Iceberg community
  • I cannot contribute a fix for this bug at this time
@nika-qubit nika-qubit added the bug Something isn't working label Feb 7, 2025
@danielcweeks
Copy link
Contributor

@nika-qubit I think you're confusing two separate things here. The token exchange flow is used by the current implementation to provide a way to get a new token using a valid token. While this is not commonly used, I'm not aware of how the current implementation violates the RFC.

The current implementation does not support the refresh token flow at all (secion-6 you state is implemented incorrectly).

I feel like you should change this to state that you would like refresh token to be supported and tag this as a feature request.

@nika-qubit
Copy link
Author

nika-qubit commented Feb 7, 2025

Thanks, @danielcweeks. Yes, I was confused by the "refreshToken" method naming. It's actually doing token-exchange and the oauth server that I'm interacting with doesn't support this grant type. I would like to make the issue a FR for supporting the refreshing token flow.

Edited the statement, please help me change the labels from "bug" to "improvement".

@danielcweeks danielcweeks added improvement PR that improves existing functionality and removed bug Something isn't working labels Feb 10, 2025
nika-qubit pushed a commit to nika-qubit/iceberg that referenced this issue Feb 13, 2025
Supported refreshing token using the refresh token flow in addition to token exchange flow for OAuth2.

Closes apache#12196
nika-qubit pushed a commit to nika-qubit/iceberg that referenced this issue Feb 18, 2025
Supported refreshing token using the refresh token flow in addition to token exchange flow for OAuth2.

Closes apache#12196
nika-qubit pushed a commit to nika-qubit/iceberg that referenced this issue Feb 18, 2025
Supported refreshing token using the refresh token flow in addition to token exchange flow for OAuth2.

Closes apache#12196
nika-qubit pushed a commit to nika-qubit/iceberg that referenced this issue Feb 18, 2025
Supported refreshing token using the refresh token flow in addition to token exchange flow for OAuth2.

Closes apache#12196
nika-qubit pushed a commit to nika-qubit/iceberg that referenced this issue Feb 18, 2025
Supported refreshing token using the refresh token flow in addition to token exchange flow for OAuth2.

Closes apache#12196
nika-qubit pushed a commit to nika-qubit/iceberg that referenced this issue Feb 18, 2025
Supported refreshing token using the refresh token flow in addition to token exchange flow for OAuth2.

Closes apache#12196
nika-qubit pushed a commit to nika-qubit/iceberg that referenced this issue Feb 20, 2025
Supported refreshing token using the refresh token flow in addition to token exchange flow for OAuth2.

Closes apache#12196
nika-qubit pushed a commit to nika-qubit/iceberg that referenced this issue Feb 20, 2025
Supported refreshing token using the refresh token flow in addition to token exchange flow for OAuth2.

Closes apache#12196
nika-qubit pushed a commit to nika-qubit/iceberg that referenced this issue Feb 20, 2025
Supported refreshing token using the refresh token flow in addition to token exchange flow for OAuth2.

Closes apache#12196
@nika-qubit
Copy link
Author

PR opened: #12362

Rebased to the latest main HEAD at this moment.

Added test is ./gradlew :iceberg-core:test --tests org.apache.iceberg.rest.TestRESTCatalog.testCatalogTokenRefreshByRefreshTokenFlow

@nika-qubit
Copy link
Author

Could someone please review it? Thanks!

@adutra
Copy link
Contributor

adutra commented Feb 21, 2025

Hi, I support this feature request and think this is a great idea.

I can confirm that support for external IDPs is currently broken, as token refreshes generally do not work.

There are a few reasons for that:

  1. The usage of token exchange grant in lieu of the refresh_token grant. Not all IDPs have support for token exchange:
    a. Authelia or Authentik have no support for it.
    b. Keycloak does have support for it, but it must be explicit enabled and is still considered in "preview" state.
    c. Auth0 has a "custom token exchange beta" feature, but it cannot be used to refresh tokens.
  2. The usage of bearer token authentication in lieu of basic authentication. Bearer token authentication per RFC 6750 is meant for accessing the resource server, not the authorization server. All IDPs reject such requests.

We could argue that reason 1 above is a "feature request", but I would qualify reason 2 as a bug.

@adutra
Copy link
Contributor

adutra commented Feb 21, 2025

To expand a bit on why using bearer authentication to refresh tokens should be considered a bug, and a violation of the OAuth2 spec:

A client is not supposed to authenticate against the tokens endpoint using a bearer token that it obtained previously from that same endpoint. According to RFC 6749 section 2.3, valid authentication methods that IDPs must support include: HTTP Basic header (preferred), or client ID + client secret included in the POST request body.

In any case, the request must include the client ID and the client secret. But a typical token refresh request issued by the Iceberg REST client looks like this (slightly arranged for readability):

POST /tokens HTTP/1.1
Host: auth-server.example.com
Authorization: Bearer <current access token>      <-- WRONG
Content-Type: application/x-www-form-urlencoded

grant_type=urn:ietf:params:oauth:grant-type:token-exchange
subject_token_type=urn:ietf:params:oauth:token-type:access_token
subject_token=<current access token>
scope=catalog

Note how it does NOT include the client ID and client secret, and how it uses "bearer token" authentication instead of "basic".

Keycloak and Auth0 both respond with a 401 response. Keycloak also logs a warning:

WARN  [org.keycloak.events] (executor-thread-40) type="TOKEN_EXCHANGE_ERROR", 
  clientId="null", 
  userId="null", 
  error="client_not_found", 
  grant_type="urn:ietf:params:oauth:grant-type:token-exchange"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement PR that improves existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants