adutra commented on PR #12362: URL: https://github.com/apache/iceberg/pull/12362#issuecomment-2675073348
Hi @nika-qubit thank you so much for starting working on this, it's been a while that I wanted to tackle this myself. However I am not sure I agree with the direction taken in this PR: First off, refresh tokens should not be exposed as a client configuration option, for a few reasons: 1. Contrary to an access token, refresh tokens are intended for the client acting on behalf of the resource owner (the user), but not for the users themselves. IOW, it should stay internal to the communication between the client and the authorization server. 2. Moreover, refresh tokens are typically long-lived, and thus have a higher risk of being compromised. 3. And finally, refresh tokens are generally revoked by the IDP when they are first used, and a new refresh token is issued and returned to the client. This is part of the security recommendations from [RFC 9700](https://www.rfc-editor.org/rfc/rfc9700#name-refresh-token-protection). So it doesn't really make sense to provide a refresh token via configuration, knowing that it would be probably revoked when used the first time. Secondly, I would argue that since the primary grant type is always `client_credentials`, **we shouldn't have to use refresh tokens at all**. Let me expand on that: The `refresh_token` grant type defined in [RFC 6749](https://datatracker.ietf.org/doc/html/rfc6749#section-6) is meant for grant types that involve human interaction, such as `authorization_code`. This is to allow the client to request another access token without asking the user to login again, which would be really annoying. But in the `client_credentials` grant, there is no user interaction; it is just as simple for the client to send another `client_credentials` request and fetch another token. That's why the RFC 6749 [explicitly states](https://datatracker.ietf.org/doc/html/rfc6749#section-4.4.3) that "a refresh token SHOULD NOT be included" in responses to the `client_credentials` flow. Auth0, for instance, does not issue refresh tokens for the `client_credentials` grant type. Keycloak can be configured to do so, but does not by default. So I would suggest instead: * if `token` is provided, then it should be used as is and never refreshed (as it is the case today) * if `credential` is provided, then I would introduce a flag to determine how to refresh an expired access token automatically: * if the flag value is `legacy`, then use the current token exchange flow to refresh the token; * if the flag value is `strict`, then discard the token and use `client_credentials` to fetch a new one. Does that make sense? \cc @danielcweeks @nastra -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
