You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "collado-mike (via GitHub)" <gi...@apache.org> on 2025/02/06 01:29:04 UTC
[PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
collado-mike opened a new pull request, #952:
URL: https://github.com/apache/polaris/pull/952
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.
<!--
Possible security vulnerabilities: STOP here and contact security@apache.org instead!
Please update the title of the PR with a meaningful message - do not leave it "empty" or "generated"
Please update this summary field:
The summary should cover these topics, if applicable:
* the motivation for the change
* a description of the status quo, for example the current behavior
* the desired behavior
* etc
PR checklist:
- Do a self-review of your code before opening a pull request
- Make sure that there's good test coverage for the changes included in this PR
- Run tests locally before pushing a PR (./gradlew check)
- Code should have comments where applicable. Particularly hard-to-understand
areas deserve good in-line documentation.
- Include changes and enhancements to the documentation (in site/content/in-dev/unreleased)
- For Work In Progress Pull Requests, please use the Draft PR feature.
Make sure to add the information BELOW this comment.
Everything in this comment will NOT be added to the PR description.
-->
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #952:
URL: https://github.com/apache/polaris/pull/952#issuecomment-2640744701
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](https://www.rfc-editor.org/rfc/rfc6749#section-2.3) (POST body) and RFC [2617](https://www.rfc-editor.org/rfc/rfc2617) (Basic Auth) require the "user ID" to be present. It may be empty, apparently, but should be present, as far as I understand.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #952:
URL: https://github.com/apache/polaris/pull/952#discussion_r1947063267
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -75,43 +74,39 @@ public Response getToken(
if (!tokenBroker.supportsRequestedTokenType(requestedTokenType)) {
Review Comment:
Yeah, we ought to update the `TokenBroker` API to take `requestedTokenType` as a method parameter.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #952:
URL: https://github.com/apache/polaris/pull/952#discussion_r1947097160
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -75,43 +74,39 @@ public Response getToken(
if (!tokenBroker.supportsRequestedTokenType(requestedTokenType)) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
}
- if (authHeader == null && clientId == null) {
+ if (authHeader == null && clientSecret == null) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
}
- if (authHeader != null && clientId == null && authHeader.startsWith("Basic ")) {
+ // 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 ")) {
String credentials = new String(Base64.decodeBase64(authHeader.substring(6)), UTF_8);
- if (!credentials.contains(":")) {
- return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
- }
LOGGER.debug("Found credentials in auth header - treating as client_credentials");
String[] parts = credentials.split(":", 2);
- clientId = parts[0];
- clientSecret = parts[1];
+ if (parts.length == 2) {
+ clientId = parts[0];
+ clientSecret = parts[1];
+ } else if (parts.length == 1) {
+ clientSecret = parts[0];
+ } else {
+ LOGGER.debug("Don't know how to parse Basic auth header");
+ OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
+ }
+ }
+ TokenResponse tokenResponse;
+ if (subjectToken != null) {
Review Comment:
Reordered this check
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #952:
URL: https://github.com/apache/polaris/pull/952#discussion_r1944553942
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -75,43 +74,39 @@ public Response getToken(
if (!tokenBroker.supportsRequestedTokenType(requestedTokenType)) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
}
- if (authHeader == null && clientId == null) {
+ if (authHeader == null && clientSecret == null) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
}
- if (authHeader != null && clientId == null && authHeader.startsWith("Basic ")) {
+ // 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 ")) {
Review Comment:
To be complete, [RFC 6749 section 2.3.1]( https://www.rfc-editor.org/rfc/rfc6749#section-2.3.1) permits client secret in the request body, but does not recommend it.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #952:
URL: https://github.com/apache/polaris/pull/952#discussion_r1944956141
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -75,43 +74,39 @@ public Response getToken(
if (!tokenBroker.supportsRequestedTokenType(requestedTokenType)) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
}
- if (authHeader == null && clientId == null) {
+ if (authHeader == null && clientSecret == null) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
}
- if (authHeader != null && clientId == null && authHeader.startsWith("Basic ")) {
+ // 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 ")) {
Review 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.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #952:
URL: https://github.com/apache/polaris/pull/952#discussion_r1947336357
##########
service/common/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -98,8 +98,12 @@ public String getScope() {
@Override
public TokenResponse generateFromToken(
- TokenType tokenType, String subjectToken, String grantType, String scope) {
- if (!TokenType.ACCESS_TOKEN.equals(tokenType)) {
+ TokenType subjectTokenType,
+ String subjectToken,
+ String grantType,
+ String scope,
+ TokenType requestedTokenType) {
+ if (!TokenType.ACCESS_TOKEN.equals(subjectTokenType)) {
Review Comment:
Shouldn't we validate `requestedTokenType`? We return `TokenType.ACCESS_TOKEN` (line 123) even if it was not requested :thinking:
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -75,43 +74,39 @@ public Response getToken(
if (!tokenBroker.supportsRequestedTokenType(requestedTokenType)) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
}
- if (authHeader == null && clientId == null) {
+ if (authHeader == null && clientSecret == null) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
}
- if (authHeader != null && clientId == null && authHeader.startsWith("Basic ")) {
+ // token exchange with client id and client secret in the authorization header 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
Review Comment:
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 client 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).
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike merged PR #952:
URL: https://github.com/apache/polaris/pull/952
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #952:
URL: https://github.com/apache/polaris/pull/952#discussion_r1944036363
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -75,43 +74,39 @@ public Response getToken(
if (!tokenBroker.supportsRequestedTokenType(requestedTokenType)) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
}
- if (authHeader == null && clientId == null) {
+ if (authHeader == null && clientSecret == null) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
}
- if (authHeader != null && clientId == null && authHeader.startsWith("Basic ")) {
+ // 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 ")) {
Review 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?
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -75,43 +74,39 @@ public Response getToken(
if (!tokenBroker.supportsRequestedTokenType(requestedTokenType)) {
Review Comment:
Why do we check `requestedTokenType`, but not use it?
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -75,43 +74,39 @@ public Response getToken(
if (!tokenBroker.supportsRequestedTokenType(requestedTokenType)) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
}
- if (authHeader == null && clientId == null) {
+ if (authHeader == null && clientSecret == null) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
}
- if (authHeader != null && clientId == null && authHeader.startsWith("Basic ")) {
+ // 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 ")) {
String credentials = new String(Base64.decodeBase64(authHeader.substring(6)), UTF_8);
- if (!credentials.contains(":")) {
- return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
- }
LOGGER.debug("Found credentials in auth header - treating as client_credentials");
String[] parts = credentials.split(":", 2);
- clientId = parts[0];
- clientSecret = parts[1];
+ if (parts.length == 2) {
+ clientId = parts[0];
+ clientSecret = parts[1];
+ } else if (parts.length == 1) {
+ clientSecret = parts[0];
+ } else {
+ LOGGER.debug("Don't know how to parse Basic auth header");
+ OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
+ }
+ }
+ TokenResponse tokenResponse;
+ if (subjectToken != null) {
Review 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?
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -75,43 +74,39 @@ public Response getToken(
if (!tokenBroker.supportsRequestedTokenType(requestedTokenType)) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
}
- if (authHeader == null && clientId == null) {
+ if (authHeader == null && clientSecret == null) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
}
- if (authHeader != null && clientId == null && authHeader.startsWith("Basic ")) {
+ // 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 ")) {
String credentials = new String(Base64.decodeBase64(authHeader.substring(6)), UTF_8);
- if (!credentials.contains(":")) {
- return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
- }
LOGGER.debug("Found credentials in auth header - treating as client_credentials");
String[] parts = credentials.split(":", 2);
- clientId = parts[0];
- clientSecret = parts[1];
+ if (parts.length == 2) {
+ clientId = parts[0];
+ clientSecret = parts[1];
+ } else if (parts.length == 1) {
+ clientSecret = parts[0];
+ } else {
+ LOGGER.debug("Don't know how to parse Basic auth header");
+ OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
+ }
+ }
+ TokenResponse tokenResponse;
+ if (subjectToken != null) {
+ if (!tokenBroker.supportsRequestedTokenType(subjectTokenType)) {
Review Comment:
why do we apply the "requested token type" check to the "subject token type"?
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -75,43 +74,39 @@ public Response getToken(
if (!tokenBroker.supportsRequestedTokenType(requestedTokenType)) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
}
- if (authHeader == null && clientId == null) {
+ if (authHeader == null && clientSecret == null) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
}
- if (authHeader != null && clientId == null && authHeader.startsWith("Basic ")) {
+ // 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 ")) {
Review 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](https://www.rfc-editor.org/rfc/rfc6749#page-40). 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).
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -75,43 +74,39 @@ public Response getToken(
if (!tokenBroker.supportsRequestedTokenType(requestedTokenType)) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
}
- if (authHeader == null && clientId == null) {
+ if (authHeader == null && clientSecret == null) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
}
- if (authHeader != null && clientId == null && authHeader.startsWith("Basic ")) {
+ // 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 ")) {
String credentials = new String(Base64.decodeBase64(authHeader.substring(6)), UTF_8);
- if (!credentials.contains(":")) {
- return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
- }
LOGGER.debug("Found credentials in auth header - treating as client_credentials");
String[] parts = credentials.split(":", 2);
- clientId = parts[0];
- clientSecret = parts[1];
+ if (parts.length == 2) {
+ clientId = parts[0];
+ clientSecret = parts[1];
+ } else if (parts.length == 1) {
+ clientSecret = parts[0];
+ } else {
+ LOGGER.debug("Don't know how to parse Basic auth header");
+ OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
Review Comment:
should this be returned?
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #952:
URL: https://github.com/apache/polaris/pull/952#discussion_r1945338798
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -75,43 +74,39 @@ public Response getToken(
if (!tokenBroker.supportsRequestedTokenType(requestedTokenType)) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
}
- if (authHeader == null && clientId == null) {
+ if (authHeader == null && clientSecret == null) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
}
- if (authHeader != null && clientId == null && authHeader.startsWith("Basic ")) {
+ // 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 ")) {
String credentials = new String(Base64.decodeBase64(authHeader.substring(6)), UTF_8);
- if (!credentials.contains(":")) {
- return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
- }
LOGGER.debug("Found credentials in auth header - treating as client_credentials");
String[] parts = credentials.split(":", 2);
- clientId = parts[0];
- clientSecret = parts[1];
+ if (parts.length == 2) {
+ clientId = parts[0];
+ clientSecret = parts[1];
+ } else if (parts.length == 1) {
+ clientSecret = parts[0];
+ } else {
+ LOGGER.debug("Don't know how to parse Basic auth header");
+ OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
+ }
+ }
+ TokenResponse tokenResponse;
+ if (subjectToken != null) {
+ if (!tokenBroker.supportsRequestedTokenType(subjectTokenType)) {
Review Comment:
Sigh. Too many token types. I removed this check :)
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #952:
URL: https://github.com/apache/polaris/pull/952#issuecomment-2643708348
Where is "magic" here? :smiley: IMHO, `credential=polaris:${token}` convey intent openly and clearly - Polaris will authenticate this request based on `${token}`.
I wonder whether `credential=:${token}` (empty client ID) works in practice. If it does, I think it's 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).
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #952:
URL: https://github.com/apache/polaris/pull/952#discussion_r1947339254
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -75,43 +74,39 @@ public Response getToken(
if (!tokenBroker.supportsRequestedTokenType(requestedTokenType)) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
}
- if (authHeader == null && clientId == null) {
+ if (authHeader == null && clientSecret == null) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
}
- if (authHeader != null && clientId == null && authHeader.startsWith("Basic ")) {
+ // token exchange with client id and client secret in the authorization header 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
Review Comment:
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).
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on PR #952:
URL: https://github.com/apache/polaris/pull/952#issuecomment-2643698178
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?
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #952:
URL: https://github.com/apache/polaris/pull/952#discussion_r1947048966
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -75,43 +74,39 @@ public Response getToken(
if (!tokenBroker.supportsRequestedTokenType(requestedTokenType)) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
}
- if (authHeader == null && clientId == null) {
+ if (authHeader == null && clientSecret == null) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
}
- if (authHeader != null && clientId == null && authHeader.startsWith("Basic ")) {
+ // 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 ")) {
Review 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?
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
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #952:
URL: https://github.com/apache/polaris/pull/952#issuecomment-2640684613
> 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.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on PR #952:
URL: https://github.com/apache/polaris/pull/952#issuecomment-2643767326
> 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.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #952:
URL: https://github.com/apache/polaris/pull/952#discussion_r1953455000
##########
service/common/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -98,8 +98,12 @@ public String getScope() {
@Override
public TokenResponse generateFromToken(
- TokenType tokenType, String subjectToken, String grantType, String scope) {
- if (!TokenType.ACCESS_TOKEN.equals(tokenType)) {
+ TokenType subjectTokenType,
+ String subjectToken,
+ String grantType,
+ String scope,
+ TokenType requestedTokenType) {
+ if (!TokenType.ACCESS_TOKEN.equals(subjectTokenType)) {
Review Comment:
@dimas-b I added this check. Can you approve again? ðĪŠ
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #952:
URL: https://github.com/apache/polaris/pull/952#discussion_r1944949296
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -75,43 +74,39 @@ public Response getToken(
if (!tokenBroker.supportsRequestedTokenType(requestedTokenType)) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
}
- if (authHeader == null && clientId == null) {
+ if (authHeader == null && clientSecret == null) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
}
- if (authHeader != null && clientId == null && authHeader.startsWith("Basic ")) {
+ // 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 ")) {
Review Comment:
Thanks, @adutra . I missed that option :facepalm:
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #952:
URL: https://github.com/apache/polaris/pull/952#discussion_r1944060199
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -75,43 +74,39 @@ public Response getToken(
if (!tokenBroker.supportsRequestedTokenType(requestedTokenType)) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
}
- if (authHeader == null && clientId == null) {
+ if (authHeader == null && clientSecret == null) {
return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
}
- if (authHeader != null && clientId == null && authHeader.startsWith("Basic ")) {
+ // 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 ")) {
String credentials = new String(Base64.decodeBase64(authHeader.substring(6)), UTF_8);
- if (!credentials.contains(":")) {
- return OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_client);
- }
LOGGER.debug("Found credentials in auth header - treating as client_credentials");
String[] parts = credentials.split(":", 2);
- clientId = parts[0];
- clientSecret = parts[1];
+ if (parts.length == 2) {
+ clientId = parts[0];
+ clientSecret = parts[1];
+ } else if (parts.length == 1) {
+ clientSecret = parts[0];
+ } else {
+ LOGGER.debug("Don't know how to parse Basic auth header");
+ OAuthUtils.getResponseFromError(OAuthTokenErrorResponse.Error.invalid_request);
+ }
+ }
+ TokenResponse tokenResponse;
+ if (subjectToken != null) {
+ if (!tokenBroker.supportsRequestedTokenType(subjectTokenType)) {
Review Comment:
IMHO, it is best to delegate all "supports" checks to the `TokenBroker` implementation.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #952:
URL: https://github.com/apache/polaris/pull/952#issuecomment-2639550875
> 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.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update DefaultOAuth2ApiService to support multiple token types and client secret without id [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on PR #952:
URL: https://github.com/apache/polaris/pull/952#issuecomment-2640629727
> 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 :(
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org