Update DefaultOAuth2ApiService to support multiple token types and client secret without id

[collado-mike]

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 TokenBroker interface does allow for implementations that do. This merely updates the DefaultOAuth2ApiService to delegate to the TokenBroker to support these cases.

[dimas-b]

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?

[dimas-b]

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).

[adutra]

To be complete, RFC 6749 section 2.3.1 permits client secret in the request body, but does not recommend it.

[dimas-b]

Thanks, @adutra . I missed that option 🤦

[dimas-b]

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.

[collado-mike]

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?

I actually just moved the comment from below. But I'll add that the clientId and secret come from the auth header in this case.
https://github.com/apache/polaris/pull/952/files/4cd4130b286d327428bc660e3731234561ab0d1e#diff-3e1d745f65cde4f3699177beac21b387a0346daef83978ec835c7037ddb9b8baL100

[adutra]

also supports sending client secret without a client id.

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.

[collado-mike]

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 :(

[adutra]

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 credential=polaris:${token}.

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.

[dimas-b]

I support @adutra 's suggestion of using fixed (common) userId for authenticating clients that do not have a real ID (as opposed to allowing missing/null ID). This is because both RFC 6749, section-2.3 (POST body) and RFC 2617 (Basic Auth) require the "user ID" to be present. It may be empty, apparently, but should be present, as far as I understand.

[collado-mike]

Ugh. I am torn. I don't like magic made-up prefixes, but I understand the intention. I think the AuthManager API will hopefully make this unnecessary, so I'd vote we refrain from magic prefixes and keep this less-than-ideal support for now until we deprecate it in favor of a better auth client. Thoughts?

[dimas-b]

Where is "magic" here? 😃 IMHO, credential=polaris:${token} conveys intent openly and clearly - Polaris will authenticate this request based on ${token}. Perhaps credential=polaris-token:${token} makes it even more direct?

I wonder whether credential=:${token} (empty client ID) works in practice. If it does, I think it's a viable option too.

My concern with this PR is not so much about null client IDs (I think it's ok to allow them server-side if we have to do that for Iceberg REST clients), but I think Polaris should always get both client ID and client secret from the same source (either the Authorization header or POST body).

[collado-mike]

Where is "magic" here? 😃 IMHO, credential=polaris:${token} conveys intent openly and clearly - Polaris will authenticate this request based on ${token}. Perhaps credential=polaris-token:${token} makes it even more direct?

The prefix itself is magic and arbitrary. E.g., the token itself may not be a Polaris token. It may be an OAuth or SAML token vended by another service entirely. The prefix doesn't actually convey any meaning.

I wonder whether credential=:${token} (empty client ID) works in practice. If it does, I think it's a viable option too.

My very, very quick test suggests this does work, when tested from the Iceberg java client. I'm ok with requiring the : in the Basic auth header if we can accept blank values to indicate token exchange.

[dimas-b]

Shouldn't we validate requestedTokenType? We return TokenType.ACCESS_TOKEN (line 123) even if it was not requested 🤔

[collado-mike]

@dimas-b I added this check. Can you approve again? 🤪

[dimas-b]

nit: The comment makes sense now (thanks for updating)... However, I'm not sure this is the right place to discuss the behaviour of the client in general. Polaris does not control these clients in principle and does not specify how they should behave. If this refers to the current behaviour of REST Catalog client from the Iceberg codebase, I think it would be much clearer if the comment said that explicitly (referencing a version).