Update DefaultOAuth2ApiService to support multiple token types and client secret without id #952
Conversation
…ient secret without id
collado-mike
requested review from
adutra,
ashvina,
dennishuo,
dimas-b,
eric-maynard,
jackye1995,
jbonofre,
vvcephei,
snazy,
RussellSpitzer,
takidau,
MonkeyCanCode,
flyrain and
ebyhr
as code owners
| // token exchange with client id and client secret means the client has previously | ||
| // attempted to refresh an access token, but refreshing was not supported by the token broker. | ||
| // Accept the client id and secret and treat it as a new token request | ||
| if (authHeader != null && clientSecret == null && authHeader.startsWith("Basic ")) { |
There was a problem hiding this comment.
Sorry, but I'm not sure how this logic aligns with the comment above. "With ... client secret", but the secret is null here?... Would you mind clarifying the comment?
There was a problem hiding this comment.
I'm fine with this change as it is obviously meant to support existing Iceberg REST client, but I do not think this logic follows the client credentials flow in RFC 6749. Specifically, "client secret" is not supposed to be passed in the POST body (from where the initial value for clientSecret comes, if I'm not mistaken).
There was a problem hiding this comment.
To be complete, RFC 6749 section 2.3.1 permits client secret in the request body, but does not recommend it.
There was a problem hiding this comment.
Thanks, @adutra . I missed that option 🤦
There was a problem hiding this comment.
In any case, I think Polaris ought to treat client ID and secret as a tuple and not mix ID from header and secret from POST body (or the other way around)... which seems possible ATM.
| clientSecret = parts[0]; | ||
| } else { | ||
| LOGGER.debug("Don't know how to parse Basic auth header"); | ||
| OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request); |
| @@ -75,43 +74,39 @@ public Response getToken( | |||
| if (!tokenBroker.supportsRequestedTokenType(requestedTokenType)) { | |||
There was a problem hiding this comment.
Why do we check requestedTokenType, but not use it?
| } | ||
| } | ||
| TokenResponse tokenResponse; | ||
| if (subjectToken != null) { |
There was a problem hiding this comment.
nit: In this case, client ID/secret appear to be parsed from the request data, but not used (not validated)... why bother parsing what we do not use?
| } | ||
| TokenResponse tokenResponse; | ||
| if (subjectToken != null) { | ||
| if (!tokenBroker.supportsRequestedTokenType(subjectTokenType)) { |
There was a problem hiding this comment.
why do we apply the "requested token type" check to the "subject token type"?
There was a problem hiding this comment.
IMHO, it is best to delegate all "supports" checks to the TokenBroker implementation.
There was a problem hiding this comment.
Sigh. Too many token types. I removed this check :)
I am a bit sad to see support for this landing in Polaris, because this is one of the many Iceberg deviances from standard OAuth2. We are basically creating a broken server, so that it can understand broken clients. Also, no external IDP that I know of supports client secret without client id. |
No, generally they don't support client secret only for client_credentials flow, but they do support token exchange. Unfortunately, Iceberg has support for token exchange, but not at the catalog initialization. E.g., at https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java#L1120-L1133 , it can use a developer token to exchange for an OAuth token, but that code doesn't execute at initialization. The only way for someone to submit a token for exchange is via the client_secret parameter :( |
Yes, and this is a common trick among catalog implementors :-) – That said, you don't need to support client secret only for this; you can configure the client to send a fixed client id, something like I don't mind going down the route of supporting client secrets without ids, but let's keep in mind that we are getting closer to having the AuthManager API merged into Iceberg core. What would become possible then is to provide an alternate AuthManager that knows how to do initial token exchanges properly, e.g. to support impersonation and delegation scenarios. |
|
I support @adutra 's suggestion of using fixed (common) |
Iceberg supports multiple token types during a token exchange and also supports sending client secret without a client id. The default Polaris implementation doesn't support most of this, but the
TokenBrokerinterface does allow for implementations that do. This merely updates theDefaultOAuth2ApiServiceto delegate to theTokenBrokerto support these cases.