You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "adutra (via GitHub)" <gi...@apache.org> on 2025/08/19 13:22:33 UTC
[PR] Include principal name in Polaris tokens [polaris]
adutra opened a new pull request, #2389:
URL: https://github.com/apache/polaris/pull/2389
Summary of changes:
- Instead of including the principal id twice in the token, the principal name is now used as the subject claim. While the default authenticator doesn't need the principal name and works with just the principal id, not having the "real" principal name available could be a problem for other authenticator implementations.
- `DecodedToken` has been refactored and renamed to `InternalPolarisCredential`. It is also now a package-private component.
- `TokenBroker.verify()` now returns PolarisCredential.
<!--
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] Include principal name in Polaris tokens [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2353131890
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -60,34 +58,22 @@ public abstract class JWTBroker implements TokenBroker {
public abstract Algorithm getAlgorithm();
@Override
- public DecodedToken verify(String token) {
+ public PolarisCredential verify(String token) {
+ return verifyInternal(token);
+ }
+
+ private InternalPolarisToken verifyInternal(String token) {
JWTVerifier verifier = JWT.require(getAlgorithm()).withClaim(CLAIM_KEY_ACTIVE, true).build();
try {
DecodedJWT decodedJWT = verifier.verify(token);
- return new DecodedToken() {
- @Override
- public Long getPrincipalId() {
- return decodedJWT.getClaim("principalId").asLong();
- }
-
- @Override
- public String getClientId() {
- return decodedJWT.getClaim("client_id").asString();
- }
-
- @Override
- public String getSub() {
- return decodedJWT.getSubject();
- }
-
- @Override
- public String getScope() {
- return decodedJWT.getClaim("scope").asString();
- }
- };
-
- } catch (JWTVerificationException e) {
+ return InternalPolarisToken.of(
+ decodedJWT.getSubject(),
Review Comment:
I think we are fine: the `DefaultAuthenticator` does not use the `sub` claim. It inspects `PolarisCredential.getPrincipalId()` and `PolarisCredential.getPrincipalName()`, and these fields are mapped as follows:
<table>
<tr><th></th><th>Source claim before this PR</th><th>Source claim after this PR</th></tr>
<tr><td>getPrincipalId()</td><td>principal_id</td><td>principal_id</td></tr>
<tr><td>getPrincipalName()</td><td>(none)</td><td>sub</td></tr>
</table>
--
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] Include principal name in Polaris tokens [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2353080081
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -60,34 +58,22 @@ public abstract class JWTBroker implements TokenBroker {
public abstract Algorithm getAlgorithm();
@Override
- public DecodedToken verify(String token) {
+ public PolarisCredential verify(String token) {
+ return verifyInternal(token);
+ }
+
+ private InternalPolarisToken verifyInternal(String token) {
JWTVerifier verifier = JWT.require(getAlgorithm()).withClaim(CLAIM_KEY_ACTIVE, true).build();
try {
DecodedJWT decodedJWT = verifier.verify(token);
- return new DecodedToken() {
- @Override
- public Long getPrincipalId() {
- return decodedJWT.getClaim("principalId").asLong();
- }
-
- @Override
- public String getClientId() {
- return decodedJWT.getClaim("client_id").asString();
- }
-
- @Override
- public String getSub() {
- return decodedJWT.getSubject();
- }
-
- @Override
- public String getScope() {
- return decodedJWT.getClaim("scope").asString();
- }
- };
-
- } catch (JWTVerificationException e) {
+ return InternalPolarisToken.of(
+ decodedJWT.getSubject(),
+ decodedJWT.getClaim(CLAIM_KEY_PRINCIPAL_ID).asLong(),
Review Comment:
Ah, it was not part of the diff in this PR :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] Include principal name in Polaris tokens [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2353140001
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -60,34 +58,22 @@ public abstract class JWTBroker implements TokenBroker {
public abstract Algorithm getAlgorithm();
@Override
- public DecodedToken verify(String token) {
+ public PolarisCredential verify(String token) {
+ return verifyInternal(token);
+ }
+
+ private InternalPolarisToken verifyInternal(String token) {
JWTVerifier verifier = JWT.require(getAlgorithm()).withClaim(CLAIM_KEY_ACTIVE, true).build();
try {
DecodedJWT decodedJWT = verifier.verify(token);
- return new DecodedToken() {
- @Override
- public Long getPrincipalId() {
- return decodedJWT.getClaim("principalId").asLong();
- }
-
- @Override
- public String getClientId() {
- return decodedJWT.getClaim("client_id").asString();
- }
-
- @Override
- public String getSub() {
- return decodedJWT.getSubject();
- }
-
- @Override
- public String getScope() {
- return decodedJWT.getClaim("scope").asString();
- }
- };
-
- } catch (JWTVerificationException e) {
+ return InternalPolarisToken.of(
+ decodedJWT.getSubject(),
Review Comment:
So, if a token generated by an old `JWTBroker` is processed by a new `JWTBroker`, `getPrincipalName()` would return the string representation of the principal ID which would result in a failed lookup, but `getPrincipalId()` would still return the principal ID in numeric form, and that lookup would succeed.
--
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] Include principal name in Polaris tokens [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2353131890
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -60,34 +58,22 @@ public abstract class JWTBroker implements TokenBroker {
public abstract Algorithm getAlgorithm();
@Override
- public DecodedToken verify(String token) {
+ public PolarisCredential verify(String token) {
+ return verifyInternal(token);
+ }
+
+ private InternalPolarisToken verifyInternal(String token) {
JWTVerifier verifier = JWT.require(getAlgorithm()).withClaim(CLAIM_KEY_ACTIVE, true).build();
try {
DecodedJWT decodedJWT = verifier.verify(token);
- return new DecodedToken() {
- @Override
- public Long getPrincipalId() {
- return decodedJWT.getClaim("principalId").asLong();
- }
-
- @Override
- public String getClientId() {
- return decodedJWT.getClaim("client_id").asString();
- }
-
- @Override
- public String getSub() {
- return decodedJWT.getSubject();
- }
-
- @Override
- public String getScope() {
- return decodedJWT.getClaim("scope").asString();
- }
- };
-
- } catch (JWTVerificationException e) {
+ return InternalPolarisToken.of(
+ decodedJWT.getSubject(),
Review Comment:
I think we are fine: the `DefaultAuthenticator` does not use the `sub` claim. It inspects `PolarisCredential.getPrincipalId()` and `PolarisCredential.getPrincipalName()`, and these fields are mapped as follows:
<table>
<tr><td></td><td>Source claim before this PR</td><td>Source claim after this PR</td></tr>
<tr><td>getPrincipalId()</td><td>principal_id</td><td>principal_id</td></tr>
<tr><td>getPrincipalName()</td><td>(none)</td><td>sub</td></tr>
</table>
--
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] Include principal name in Polaris tokens [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2353155919
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -60,34 +58,22 @@ public abstract class JWTBroker implements TokenBroker {
public abstract Algorithm getAlgorithm();
@Override
- public DecodedToken verify(String token) {
+ public PolarisCredential verify(String token) {
+ return verifyInternal(token);
+ }
+
+ private InternalPolarisToken verifyInternal(String token) {
JWTVerifier verifier = JWT.require(getAlgorithm()).withClaim(CLAIM_KEY_ACTIVE, true).build();
try {
DecodedJWT decodedJWT = verifier.verify(token);
- return new DecodedToken() {
- @Override
- public Long getPrincipalId() {
- return decodedJWT.getClaim("principalId").asLong();
- }
-
- @Override
- public String getClientId() {
- return decodedJWT.getClaim("client_id").asString();
- }
-
- @Override
- public String getSub() {
- return decodedJWT.getSubject();
- }
-
- @Override
- public String getScope() {
- return decodedJWT.getClaim("scope").asString();
- }
- };
-
- } catch (JWTVerificationException e) {
+ return InternalPolarisToken.of(
+ decodedJWT.getSubject(),
Review Comment:
Good point! Thanks for the clarification :+1:
--
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] Include principal name in Polaris tokens [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2353075950
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -60,34 +58,22 @@ public abstract class JWTBroker implements TokenBroker {
public abstract Algorithm getAlgorithm();
@Override
- public DecodedToken verify(String token) {
+ public PolarisCredential verify(String token) {
+ return verifyInternal(token);
+ }
+
+ private InternalPolarisToken verifyInternal(String token) {
JWTVerifier verifier = JWT.require(getAlgorithm()).withClaim(CLAIM_KEY_ACTIVE, true).build();
try {
DecodedJWT decodedJWT = verifier.verify(token);
- return new DecodedToken() {
- @Override
- public Long getPrincipalId() {
- return decodedJWT.getClaim("principalId").asLong();
- }
-
- @Override
- public String getClientId() {
- return decodedJWT.getClaim("client_id").asString();
- }
-
- @Override
- public String getSub() {
- return decodedJWT.getSubject();
- }
-
- @Override
- public String getScope() {
- return decodedJWT.getClaim("scope").asString();
- }
- };
-
- } catch (JWTVerificationException e) {
+ return InternalPolarisToken.of(
+ decodedJWT.getSubject(),
+ decodedJWT.getClaim(CLAIM_KEY_PRINCIPAL_ID).asLong(),
Review Comment:
In the `generateTokenString` method:
https://github.com/apache/polaris/blob/95ebdd359ed0956ddc7a771c37683f9a3ecb0176/runtime/service/src/main/java/org/apache/polaris/service/auth/JWTBroker.java#L164-L177
--
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] Include principal name in Polaris tokens [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2285394715
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/InternalPolarisCredential.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth;
+
+import com.google.common.base.Splitter;
+import jakarta.annotation.Nonnull;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.polaris.immutables.PolarisImmutable;
+import org.immutables.value.Value;
+
+/**
+ * A specialized {@link PolarisCredential} used for internal authentication, when Polaris is the
+ * identity provider.
+ *
+ * <p>Such credentials are created by the Polaris service itself, from a JWT token previously issued
+ * by Polaris itself.
+ *
+ * @see JWTBroker
+ */
+@PolarisImmutable
+abstract class InternalPolarisCredential implements PolarisCredential {
Review Comment:
I'm not opposed to other names.
My rationale is: this component represents credentials extracted from an internal polaris token, hence the name I picked.
Let's wait for more feedback from other reviewers and I'll change the name accordingly.
--
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] Include principal name in Polaris tokens [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2353053275
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -60,34 +58,22 @@ public abstract class JWTBroker implements TokenBroker {
public abstract Algorithm getAlgorithm();
@Override
- public DecodedToken verify(String token) {
+ public PolarisCredential verify(String token) {
+ return verifyInternal(token);
+ }
+
+ private InternalPolarisToken verifyInternal(String token) {
JWTVerifier verifier = JWT.require(getAlgorithm()).withClaim(CLAIM_KEY_ACTIVE, true).build();
try {
DecodedJWT decodedJWT = verifier.verify(token);
- return new DecodedToken() {
- @Override
- public Long getPrincipalId() {
- return decodedJWT.getClaim("principalId").asLong();
- }
-
- @Override
- public String getClientId() {
- return decodedJWT.getClaim("client_id").asString();
- }
-
- @Override
- public String getSub() {
- return decodedJWT.getSubject();
- }
-
- @Override
- public String getScope() {
- return decodedJWT.getClaim("scope").asString();
- }
- };
-
- } catch (JWTVerificationException e) {
+ return InternalPolarisToken.of(
+ decodedJWT.getSubject(),
+ decodedJWT.getClaim(CLAIM_KEY_PRINCIPAL_ID).asLong(),
Review Comment:
I might have missed it, but where is this claim set (in issued tokens)?
--
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] Include principal name in Polaris tokens [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2285404512
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/InternalPolarisCredential.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth;
+
+import com.google.common.base.Splitter;
+import jakarta.annotation.Nonnull;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.polaris.immutables.PolarisImmutable;
+import org.immutables.value.Value;
+
+/**
+ * A specialized {@link PolarisCredential} used for internal authentication, when Polaris is the
+ * identity provider.
+ *
+ * <p>Such credentials are created by the Polaris service itself, from a JWT token previously issued
+ * by Polaris itself.
+ *
+ * @see JWTBroker
+ */
+@PolarisImmutable
+abstract class InternalPolarisCredential implements PolarisCredential {
Review Comment:
(or `InternalPolarisToken`)
--
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] Include principal name in Polaris tokens [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2353131890
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -60,34 +58,22 @@ public abstract class JWTBroker implements TokenBroker {
public abstract Algorithm getAlgorithm();
@Override
- public DecodedToken verify(String token) {
+ public PolarisCredential verify(String token) {
+ return verifyInternal(token);
+ }
+
+ private InternalPolarisToken verifyInternal(String token) {
JWTVerifier verifier = JWT.require(getAlgorithm()).withClaim(CLAIM_KEY_ACTIVE, true).build();
try {
DecodedJWT decodedJWT = verifier.verify(token);
- return new DecodedToken() {
- @Override
- public Long getPrincipalId() {
- return decodedJWT.getClaim("principalId").asLong();
- }
-
- @Override
- public String getClientId() {
- return decodedJWT.getClaim("client_id").asString();
- }
-
- @Override
- public String getSub() {
- return decodedJWT.getSubject();
- }
-
- @Override
- public String getScope() {
- return decodedJWT.getClaim("scope").asString();
- }
- };
-
- } catch (JWTVerificationException e) {
+ return InternalPolarisToken.of(
+ decodedJWT.getSubject(),
Review Comment:
I think we are fine: the `DefaultAuthenticator` inspects `PolarisCredential.getPrincipalId()` and `PolarisCredential.getPrincipalName()`, and these fields are mapped as follows:
<table>
<tr><th></th><th>Source claim before this PR</th><th>Source claim after this PR</th></tr>
<tr><td>getPrincipalId()</td><td>principal_id</td><td>principal_id</td></tr>
<tr><td>getPrincipalName()</td><td>(none)</td><td>sub</td></tr>
</table>
--
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] Include principal name in Polaris tokens [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra merged PR #2389:
URL: https://github.com/apache/polaris/pull/2389
--
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] Include principal name in Polaris tokens [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2286065706
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -157,16 +143,18 @@ public TokenResponse generateFromClientSecrets(
if (principal.isEmpty()) {
return new TokenResponse(OAuthTokenErrorResponse.Error.unauthorized_client);
}
- String tokenString = generateTokenString(clientId, scope, principal.get().getId());
+ String tokenString =
+ generateTokenString(principal.get().getName(), principal.get().getId(), clientId, scope);
return new TokenResponse(
tokenString, TokenType.ACCESS_TOKEN.getValue(), maxTokenGenerationInSeconds);
}
- private String generateTokenString(String clientId, String scope, Long principalId) {
+ private String generateTokenString(
+ String principalName, long principalId, String clientId, String scope) {
Instant now = Instant.now();
return JWT.create()
.withIssuer(ISSUER_KEY)
- .withSubject(String.valueOf(principalId))
+ .withSubject(principalName)
Review Comment:
> But other Authenticator implementations could require the principal name to be available
i would like this understand this use case more, is it more from audit purpose ? or are we saying some Token Brokers would prefer look-up by name as names would be like `email-id` for them which is considered to unique ?
--
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] Include principal name in Polaris tokens [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2353083901
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -156,16 +142,18 @@ public TokenResponse generateFromClientSecrets(
if (principal.isEmpty()) {
return new TokenResponse(OAuthTokenErrorResponse.Error.unauthorized_client);
}
- String tokenString = generateTokenString(clientId, scope, principal.get().getId());
+ String tokenString =
+ generateTokenString(principal.get().getName(), principal.get().getId(), clientId, scope);
return new TokenResponse(
tokenString, TokenType.ACCESS_TOKEN.getValue(), maxTokenGenerationInSeconds);
}
- private String generateTokenString(String clientId, String scope, Long principalId) {
+ private String generateTokenString(
+ String principalName, long principalId, String clientId, String scope) {
Instant now = Instant.now();
return JWT.create()
.withIssuer(ISSUER_KEY)
Review Comment:
side note: it might be worth setting `aud` to the realm ID and later validating it (for follow up). WDYT?
--
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] Include principal name in Polaris tokens [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2288013719
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -157,16 +143,18 @@ public TokenResponse generateFromClientSecrets(
if (principal.isEmpty()) {
return new TokenResponse(OAuthTokenErrorResponse.Error.unauthorized_client);
}
- String tokenString = generateTokenString(clientId, scope, principal.get().getId());
+ String tokenString =
+ generateTokenString(principal.get().getName(), principal.get().getId(), clientId, scope);
return new TokenResponse(
tokenString, TokenType.ACCESS_TOKEN.getValue(), maxTokenGenerationInSeconds);
}
- private String generateTokenString(String clientId, String scope, Long principalId) {
+ private String generateTokenString(
+ String principalName, long principalId, String clientId, String scope) {
Instant now = Instant.now();
return JWT.create()
.withIssuer(ISSUER_KEY)
- .withSubject(String.valueOf(principalId))
+ .withSubject(principalName)
Review Comment:
Yes, exactly. Some `Authenticator`s could enforce by-name lookup since principal names must be unique.
--
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] Include principal name in Polaris tokens [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2353149893
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -156,16 +142,18 @@ public TokenResponse generateFromClientSecrets(
if (principal.isEmpty()) {
return new TokenResponse(OAuthTokenErrorResponse.Error.unauthorized_client);
}
- String tokenString = generateTokenString(clientId, scope, principal.get().getId());
+ String tokenString =
+ generateTokenString(principal.get().getName(), principal.get().getId(), clientId, scope);
return new TokenResponse(
tokenString, TokenType.ACCESS_TOKEN.getValue(), maxTokenGenerationInSeconds);
}
- private String generateTokenString(String clientId, String scope, Long principalId) {
+ private String generateTokenString(
+ String principalName, long principalId, String clientId, String scope) {
Instant now = Instant.now();
return JWT.create()
.withIssuer(ISSUER_KEY)
Review Comment:
We could definitely include the realm in the token, but I'm not sure about using the `aud` claim. In all compliance with the specs, this claim should contain the URI of the Polaris service:
https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.3
But, it could also contain a simple string as well, and the interpretation of that value is application-specific.
Maybe a separate claim e.g. `polaris_realm` would be better?
--
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] Include principal name in Polaris tokens [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2353089905
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -60,34 +58,22 @@ public abstract class JWTBroker implements TokenBroker {
public abstract Algorithm getAlgorithm();
@Override
- public DecodedToken verify(String token) {
+ public PolarisCredential verify(String token) {
+ return verifyInternal(token);
+ }
+
+ private InternalPolarisToken verifyInternal(String token) {
JWTVerifier verifier = JWT.require(getAlgorithm()).withClaim(CLAIM_KEY_ACTIVE, true).build();
try {
DecodedJWT decodedJWT = verifier.verify(token);
- return new DecodedToken() {
- @Override
- public Long getPrincipalId() {
- return decodedJWT.getClaim("principalId").asLong();
- }
-
- @Override
- public String getClientId() {
- return decodedJWT.getClaim("client_id").asString();
- }
-
- @Override
- public String getSub() {
- return decodedJWT.getSubject();
- }
-
- @Override
- public String getScope() {
- return decodedJWT.getClaim("scope").asString();
- }
- };
-
- } catch (JWTVerificationException e) {
+ return InternalPolarisToken.of(
+ decodedJWT.getSubject(),
Review Comment:
I guess this might have issues during rolling upgrades as old tokens might still have `sub` == principal ID.
I do not think it is a critical issue, but might be worth mentioning in `CHANGELOG.md`. WDYT?
--
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] Include principal name in Polaris tokens [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2285400260
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -157,16 +143,18 @@ public TokenResponse generateFromClientSecrets(
if (principal.isEmpty()) {
return new TokenResponse(OAuthTokenErrorResponse.Error.unauthorized_client);
}
- String tokenString = generateTokenString(clientId, scope, principal.get().getId());
+ String tokenString =
+ generateTokenString(principal.get().getName(), principal.get().getId(), clientId, scope);
return new TokenResponse(
tokenString, TokenType.ACCESS_TOKEN.getValue(), maxTokenGenerationInSeconds);
}
- private String generateTokenString(String clientId, String scope, Long principalId) {
+ private String generateTokenString(
+ String principalName, long principalId, String clientId, String scope) {
Instant now = Instant.now();
return JWT.create()
.withIssuer(ISSUER_KEY)
- .withSubject(String.valueOf(principalId))
+ .withSubject(principalName)
Review Comment:
> we don't use principalId for lookups from metastore from sub and always use the prinicipal id from the claim ?
Correct.
> how do we plan to leverage principalName from the subject here ?
The `DefaultAuthenticator` only does lookups by name if the principal id is not available. This won't happen with polaris internal tokens, of course. But other `Authenticator` implementations could require the principal name to be available. It's imho bad that the principal name is never surfaced in internal polaris tokens.
--
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] Include principal name in Polaris tokens [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2285354548
##########
runtime/service/src/test/java/org/apache/polaris/service/auth/DefaultAuthenticatorTest.java:
##########
@@ -73,10 +73,9 @@ public void testFetchPrincipalThrowsServiceExceptionOnMetastoreException() {
@Test
public void testFetchPrincipalThrowsNotAuthorizedWhenNotFound() {
- DecodedToken token = Mockito.mock(DecodedToken.class);
+ PolarisCredential token = Mockito.mock(PolarisCredential.class);
long principalId = 100L;
when(token.getPrincipalId()).thenReturn(principalId);
- when(token.getClientId()).thenReturn("abc");
Review Comment:
is this intentional or was just redundant ?
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/InternalPolarisCredential.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth;
+
+import com.google.common.base.Splitter;
+import jakarta.annotation.Nonnull;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.polaris.immutables.PolarisImmutable;
+import org.immutables.value.Value;
+
+/**
+ * A specialized {@link PolarisCredential} used for internal authentication, when Polaris is the
+ * identity provider.
+ *
+ * <p>Such credentials are created by the Polaris service itself, from a JWT token previously issued
+ * by Polaris itself.
+ *
+ * @see JWTBroker
+ */
+@PolarisImmutable
+abstract class InternalPolarisCredential implements PolarisCredential {
Review Comment:
[optional : may be another name ?] isn't it still a decoded bearer token ? maybe its just me but i find it a little bit hard to disassociate with the client crendentials flow semantics, which typically requires client secret
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -157,16 +143,18 @@ public TokenResponse generateFromClientSecrets(
if (principal.isEmpty()) {
return new TokenResponse(OAuthTokenErrorResponse.Error.unauthorized_client);
}
- String tokenString = generateTokenString(clientId, scope, principal.get().getId());
+ String tokenString =
+ generateTokenString(principal.get().getName(), principal.get().getId(), clientId, scope);
return new TokenResponse(
tokenString, TokenType.ACCESS_TOKEN.getValue(), maxTokenGenerationInSeconds);
}
- private String generateTokenString(String clientId, String scope, Long principalId) {
+ private String generateTokenString(
+ String principalName, long principalId, String clientId, String scope) {
Instant now = Instant.now();
return JWT.create()
.withIssuer(ISSUER_KEY)
- .withSubject(String.valueOf(principalId))
+ .withSubject(principalName)
Review Comment:
[for my understanding] we don't use principalId for lookups from metastore from sub and always use the prinicipal id from the claim ? if yes how do we plan to leverage principalName from the subject here ?
--
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] Include principal name in Polaris tokens [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2285403589
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/InternalPolarisCredential.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth;
+
+import com.google.common.base.Splitter;
+import jakarta.annotation.Nonnull;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.polaris.immutables.PolarisImmutable;
+import org.immutables.value.Value;
+
+/**
+ * A specialized {@link PolarisCredential} used for internal authentication, when Polaris is the
+ * identity provider.
+ *
+ * <p>Such credentials are created by the Polaris service itself, from a JWT token previously issued
+ * by Polaris itself.
+ *
+ * @see JWTBroker
+ */
+@PolarisImmutable
+abstract class InternalPolarisCredential implements PolarisCredential {
Review Comment:
Would `PolarisToken` work for you?
--
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] Include principal name in Polaris tokens [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2285388281
##########
runtime/service/src/test/java/org/apache/polaris/service/auth/DefaultAuthenticatorTest.java:
##########
@@ -73,10 +73,9 @@ public void testFetchPrincipalThrowsServiceExceptionOnMetastoreException() {
@Test
public void testFetchPrincipalThrowsNotAuthorizedWhenNotFound() {
- DecodedToken token = Mockito.mock(DecodedToken.class);
+ PolarisCredential token = Mockito.mock(PolarisCredential.class);
long principalId = 100L;
when(token.getPrincipalId()).thenReturn(principalId);
- when(token.getClientId()).thenReturn("abc");
Review Comment:
Redundant :-)
--
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] Include principal name in Polaris tokens [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2285414172
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/InternalPolarisCredential.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth;
+
+import com.google.common.base.Splitter;
+import jakarta.annotation.Nonnull;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.polaris.immutables.PolarisImmutable;
+import org.immutables.value.Value;
+
+/**
+ * A specialized {@link PolarisCredential} used for internal authentication, when Polaris is the
+ * identity provider.
+ *
+ * <p>Such credentials are created by the Polaris service itself, from a JWT token previously issued
+ * by Polaris itself.
+ *
+ * @see JWTBroker
+ */
+@PolarisImmutable
+abstract class InternalPolarisCredential implements PolarisCredential {
Review Comment:
> InternalPolarisToken
works for me ! This comment is totally optional to address, agree for waiting for more peoplr to weigh in :) !
--
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] Include principal name in Polaris tokens [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #2389:
URL: https://github.com/apache/polaris/pull/2389#discussion_r2285414172
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/InternalPolarisCredential.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth;
+
+import com.google.common.base.Splitter;
+import jakarta.annotation.Nonnull;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.polaris.immutables.PolarisImmutable;
+import org.immutables.value.Value;
+
+/**
+ * A specialized {@link PolarisCredential} used for internal authentication, when Polaris is the
+ * identity provider.
+ *
+ * <p>Such credentials are created by the Polaris service itself, from a JWT token previously issued
+ * by Polaris itself.
+ *
+ * @see JWTBroker
+ */
+@PolarisImmutable
+abstract class InternalPolarisCredential implements PolarisCredential {
Review Comment:
> InternalPolarisToken
works for me ! This comment is totally optional to address, agree more weighing in :) !
--
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