Repository navigation
Conversation
| @@ -0,0 +1,334 @@ | |||
| /* | |||
There was a problem hiding this comment.
This header comments are no longer needed.
There was a problem hiding this comment.
Thank you for your comment.
I have just fixed as suggested.
| @@ -0,0 +1,77 @@ | |||
| /* | |||
There was a problem hiding this comment.
The header comments are no longer needed.
There was a problem hiding this comment.
I have just fixed as suggested for here too.
| @@ -0,0 +1,517 @@ | |||
| /* | |||
There was a problem hiding this comment.
The header comments are no longer needed.
There was a problem hiding this comment.
I have just fixed as suggested for here too.
| @JsonProperty("client_id") | ||
| protected String client_id; | ||
|
|
||
| public String getClient_id() { |
There was a problem hiding this comment.
Any reason we use getClient_id here and not getClientId? The json property should handle de/serialization already.
There was a problem hiding this comment.
Thank you for your comment.
I have just fixed as suggested.
| @@ -1401,6 +1407,29 @@ private void generateRefreshToken(boolean offlineTokenRequested) { | |||
| } | |||
| } | |||
|
|
|||
| public AccessTokenResponseBuilder generateIDJag() { | |||
There was a problem hiding this comment.
It is used in exchangeClientToOIDCClient method.
| * @author <a href="mailto:yutaka.obuchi.sd@hitachi.com">Yutaka Obuchi</a> | ||
| * @version $Revision: 1 $ | ||
| */ | ||
| public class IDJAG extends AccessToken { |
There was a problem hiding this comment.
I would not introduce a new type of token
There was a problem hiding this comment.
Hi @graziang,
Thank you for you comment.
I am rethinking about this and I have just remembered why I introduced that.
It is to set ID-JAG's token type to "oauth-id-jag+jwt" in the JWT header, which is determined by the TokenCategory during the token encoding process.
For example, in the AccessToken class, the TokenCategory is TokenCategory.ACCESS. From my understanding, this sets the token type in the JWT header to "JWT" or "at+jwt".
This is why I believe we need a new ID-JAG class with a dedicated TokenCategory to correctly set the header type to "oauth-id-jag+jwt".
Please let me know if I misunderstood anything or if you have any better ideas.
Thank you!
There was a problem hiding this comment.
Still not convinced here, an ID-JAG is not an AccessToken the inheritance is only for reuse, also, the only purpose of TokenCategory.IDJAG is to have the typ: oauth-id-jag+jwt header. Consider an alternative way to set the header without introducing a new TokenCategory if it is possible.
There was a problem hiding this comment.
Thank you for the review, @graziang.
As an alternative, I am thinking of using the base JsonWebToken directly and signing it by explicitly calling the sign method of JWSBuilder within the provider. What do you think about this?
As you are well aware, from my understanding, by doing this, we can explicitly set the custom typ header (oauth-id-jag+jwt) via JWSBuilder.type(...) without needing a new TokenCategory. It also prevents AccessToken-specific noise from being included, allowing us to cleanly construct the exact claims required for ID-JAG.
This approach would completely eliminate the need for the IDJAG class and keep Keycloak's core data model untouched.
I would love to hear your thoughts on this alternative approach!
There was a problem hiding this comment.
@bucchi thanks! Sounds good to use JWSBuilder. UserInfoEndpoint is a good reference, also CIBAAuthenticationRequest.
There was a problem hiding this comment.
@graziang
I have just fixed as your suggestion with removing client attribute/IDJAG.java
| } | ||
|
|
||
| @Override | ||
| public boolean supports(TokenExchangeContext context) { |
There was a problem hiding this comment.
I'm not sure if introducing a new provider is needed, we need to evaluate if it is sufficient to just modify the StandardTokenExchangeProvider, otherwise the new provider should contain only the bare essentials, I see a lot of duplicate code.
There was a problem hiding this comment.
Thank you for your valuable feedback.
I have thoroughly reviewed the source code and removed the duplicated and unnecessary parts to keep this provider as minimal and focused as possible.
Could you please take another look?
There was a problem hiding this comment.
ok to keep the new provider but I think there are still many duplicate parts. To be checked if getTargetAudienceClients() getRequestedScope() can be reused from StandardTokenExchangeProvider if we remove the idjag.* attributes from the client. I think we can also do some refactoring between StandardTokenExchangeProvider and this provider for example in tokenExchange() and exchangeClientToOIDCClient() there are many duplicate parts that could be extracted. Ideally IDJWTTokenExchangeProvider should have only the missing parts that are different from StandardTokenExchangeProvider.
There was a problem hiding this comment.
@graziang
Thank you for the suggestion. I have refactored StandardTokenExchangeProvider's exchangeClientToOIDCClient() and also updated IDJWTTokenExchangeProvider's tokenExchange() to call the existing functions to reduce duplication.
| */ | ||
| @KeycloakIntegrationTest(config = IDJAGTokenExchangeTest.JWTAuthorizationGrantServerConfig.class) | ||
| @TestMethodOrder(MethodOrderer.MethodName.class) | ||
| public class IDJAGTokenExchangeTest { |
There was a problem hiding this comment.
do we need all these oauth clients?, can it be simplified?
There was a problem hiding this comment.
Yes.
I made it simpler.
| public boolean supports(TokenExchangeContext context) { | ||
|
|
||
| String requestedTokenType = context.getFormParams().getFirst(OAuth2Constants.REQUESTED_TOKEN_TYPE); | ||
| if (!requestedTokenType.equals(OAuth2Constants.IDENTITY_ASSERTION_JWT_TOKEN_TYPE)) { |
There was a problem hiding this comment.
I think, the "requested_token_type" is an optional as per RFC 8693, and possible NPEs and breaks all token exchange when the feature is enabled. Note: this fails as per the current factory order().
There was a problem hiding this comment.
Thank you for your comment.
I fixed that as suggested.
| return new CorsErrorResponseException(cors, OAuthErrorException.INVALID_REQUEST, "Client not found for audience identifier: " + audiencePrameterString, Response.Status.BAD_REQUEST); | ||
| }); | ||
|
|
||
| if (targetClient == null) { |
There was a problem hiding this comment.
This block seems unreachable. Since .orElseThrow(...) already guarantees a non-null result.
There was a problem hiding this comment.
I also fixed that as suggested.
There was a problem hiding this comment.
Pull request overview
This PR adds experimental support for issuing an Identity Assertion JWT Authorization Grant (ID-JAG) from Keycloak via the OAuth 2.0 Token Exchange endpoint, including a new token representation/category and an integration test covering successful and error flows.
Changes:
- Introduces a new token exchange provider/factory (
IDJWTTokenExchangeProvider*) selected whenrequested_token_type=urn:ietf:params:oauth:token-type:id-jag. - Adds an
IDJAGtoken representation and a newTokenCategory.IDJAG, wiring encoding/header type and response builder support. - Extends test utilities and adds an integration test validating ID-JAG issuance and common failure cases.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/utils-shared/src/main/java/org/keycloak/testsuite/util/oauth/TokenExchangeRequest.java | Adds ability to override the scope parameter for token exchange test requests. |
| tests/base/src/test/java/org/keycloak/tests/oauth/IDJAGTokenExchangeTest.java | New integration test for ID-JAG issuance via token exchange. |
| services/src/main/resources/META-INF/services/org.keycloak.protocol.oidc.TokenExchangeProviderFactory | Registers the new token exchange provider factory. |
| services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java | Adds ID-JAG generation support to the token response builder. |
| services/src/main/java/org/keycloak/protocol/oidc/tokenexchange/AbstractTokenExchangeProvider.java | Generalizes token exchange flow methods from AccessToken to JsonWebToken. |
| services/src/main/java/org/keycloak/protocol/oidc/tokenexchange/StandardTokenExchangeProvider.java | Updates signatures to match the generalized token type. |
| services/src/main/java/org/keycloak/protocol/oidc/tokenexchange/V1TokenExchangeProvider.java | Updates signatures to match the generalized token type (with existing AccessToken usage). |
| services/src/main/java/org/keycloak/protocol/oidc/tokenexchange/IDJWTTokenExchangeProviderFactory.java | New factory for the ID-JAG token exchange provider. |
| services/src/main/java/org/keycloak/protocol/oidc/tokenexchange/IDJWTTokenExchangeProvider.java | New provider implementing ID-JAG issuance logic and client-attribute based scope/client mapping. |
| services/src/main/java/org/keycloak/jose/jws/DefaultTokenManager.java | Adds token header type/signature selection for the new IDJAG token category. |
| core/src/main/java/org/keycloak/util/TokenUtil.java | Adds a new TOKEN_TYPE_IDJAG constant. |
| core/src/main/java/org/keycloak/TokenCategory.java | Adds new IDJAG token category. |
| core/src/main/java/org/keycloak/representations/IDJAG.java | Adds new token representation extending AccessToken. |
| core/src/main/java/org/keycloak/OAuth2Constants.java | Adds constant for the ID-JAG requested_token_type URN. |
|
Thank you very much @bucchi for all your contributions to this effort, and especially for this PR and for taking it out of draft status. As you can see from the reactions/comments on the issue and the PR, as well as the feedback from community members outside the core team, this is a feature that has generated significant interest across the wider community. Some contributors have even stepped forward to help with follow-up work ( #50288 ) as your colleague mentioned in Slack there is some concern that this PR may not fit within the Keycloak 26.7 release timeline while it is still under his review ( https://cloud-native.slack.com/archives/C09CGDKPTSQ/p1781859663508469?thread_ts=1771504128.722829&cid=C09CGDKPTSQ ). I understand that you and @mposolda would like to focus on driving this PR forward. At the same time, because Keycloak is a community project and this feature has attracted broader community interest, it would be very helpful if the community could better understand the current plan and expected timeline for bringing this PR to completion. It would also be great to know whether you are interested in collaborating with the wider community by engaging with the review comments and, where appropriate, accepting contributions that could help move the work forward. I want to be clear that my intention is not to create unnecessary pressure or add to your workload. I appreciate that priorities and timelines can change. If however it looks unlikely that this PR will be ready for Keycloak 26.7, I would kindly ask that we avoid promoting the feature too broadly until its status is clearer. Historically, the Keycloak project has been careful about marketing experimental features (e.g: OID4VCI). The recent Okta press release ( https://www.okta.com/newsroom/press-releases/okta-announces-cross-app-access-partners/ ) gives the impression that Keycloak already supports XAA/ID-JAG, which is not currently the case. I think it would be beneficial to ask Okta either to remove the reference to Keycloak from the press release or to update it with a clarification that Keycloak does not currently provide complete XAA support, and that XAA may become available as an experimental or preview feature after September/October 2026. |
| String errorMessage = "Permitted scopes not configured for the audience client"; | ||
| event.detail(Details.REASON, errorMessage); | ||
| event.error(Errors.NOT_ALLOWED); | ||
| throw new CorsErrorResponseException(cors, OAuthErrorException.SERVER_ERROR, errorMessage, Response.Status.BAD_REQUEST); |
There was a problem hiding this comment.
I think that the combination of OAuthErrorException.SERVER_ERROR and Response.Status.BAD_REQUEST is inconsistent. The inadequate client configuration in keycloak causes the error. The client request does not cause it.
Considering this point, Response.Status.INTERNAL_SERVER_ERROR seems to be appropriate.
What do you think about that?
There was a problem hiding this comment.
Thank you for your comment.
I fixed this as you suggested.
| List<String> audienceParams = params.getAudience(); | ||
| List<ClientModel> targetAudienceClients = new ArrayList<>(); | ||
| if (audienceParams != null) { | ||
| if(audienceParams.size() == 0) { |
There was a problem hiding this comment.
if(audienceParams.size() == 0)
might be
if (audienceParams.size() == 0)
There was a problem hiding this comment.
I fixed this as suggested.
ab908b3 to
35c0e7a
Compare
35c0e7a to
592bb88
Compare
592bb88 to
2d8dd22
Compare
2d8dd22 to
7d8a65c
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical cross-client and audience failures prevent successful ID-JAG issuance, with additional validation, policy, audit, and test issues unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 3
Open (3)
|
@bucchi Thanks! I did not check the details of ID-JAG , but from quickly checking the PR, it seems that at least As duplicated code is source of issues and bugs in the future (EG. the case when some future bug will be fixed just in the To achieve that, maybe it is possible to use some pattern like "Template method" or something similar to make sure duplications are avoided? Maybe using common super-class for For the inspiration, you can possibly take a look at Also, it would be good if the PR is reviewed and approved by someone from community with the knowledge of ID-JAG. Right now, it was not approved by anyone yet. Especially review/approve from @tnorimat would be nice (currently it is marked as "requesting change"). |
|
Thanks for the feedback, @mposolda. I agree that reducing duplication is important. I think helper methods like getTargetAudienceClients() or getRequestedTokenType() could be refactored more. However, I suspect those may not be the main areas you’re concerned about. @tnorimat, could you please also point out any specific duplicated code in those methods that you think could be refactored? As a side note, to keep the changes minimal for now, I’m considering keeping the existing inheritance structure and adding narrowly scoped hooks to StandardTokenExchangeProvider for the ID-JAG-specific behavior. IDJWTTokenExchangeProvider would then override only the parts that differ. Do you think this would be a reasonable approach? |
|
@bucchi Hello, I used Claude Opus 4.8 to find duplicated codes. The result is as follows.
Duplication 1 —
|
|
Hi @tnorimat, Thank you very much for the comprehensive review and for organizing the feedback into actionable points. Regarding the 4 duplication points you raised, I would like to address "Duplication 1 — First, I want to clarify that simply moving the logic from From my understanding the checks you listed (user-session validity, AuthenticationManager.AuthResult authResult = AuthenticationManager.verifyIdentityToken(
session, realm, session.getContext().getUri(), clientConnection,
true, true, null, false, subjectToken, context.getHeaders(), verifier -> {}
);Since the base provider relies entirely on verifyIdentityToken(), which is strictly hardcoded to verify an AccessToken (expecting typ: Bearer), we cannot pass an IDToken through this method without it failing token-type and deserialization checks. Therefore, to make the ID-JAG token exchange work with an IDToken as the subject, I had to unpack and decompose the internal checks executed inside verifyIdentityToken() and manually convert them to be compatible with IDToken structure directly within IDJWTTokenExchangeProvider. Since the base provider relies on that monolithic 1-line wrapper and has no manual validation checks in its own class body to extract, simply shifting our explicit code to processSubjectToken() keeps the duplicate code size exactly the same. However, while drafting this response, a potential alternative came to mind: Were you suggesting that we try calling the standard verifyIdentityToken() with the checkTokenType parameter set to false to force it to accept and process the IDToken? If that was your underlying intent, I would be happy to investigate whether we can bypass the strict access token constraints of verifyIdentityToken() directly. This would allow us to reuse the base flow and truly eliminate the duplicate verification checks. Please let me know if this is the direction you had in mind, and I will follow up on the remaining duplication points (Duplication 2–4) shortly! |
|
Hi @tnorimat, Thank you for the detailed feedback on "Duplication 2". I would like to explain the design rationale behind our current implementation, and propose a highly simplified solution that combines both of our ideas. 1. Why we chose our current approachWhen designing the ID-JAG assertion, we carefully evaluated three classes to represent it: We intentionally chose For this reason, we avoided using the standard builder directly and constructed the assertion explicitly on a clean 2. Proposed Solution (The Simplified Hybrid Approach)However, I completely agree with your point regarding Keycloak core consistency—routing through To achieve both strict security/leakage prevention and zero code duplication without introducing a brand-new method, I propose we refactor the existing // In the base StandardTokenExchangeProvider class:
protected Response buildTokenExchangeResponse(
org.keycloak.protocol.oidc.TokenManager.AccessTokenResponseBuilder responseBuilder,
String requestedTokenType,
List<ClientModel> targetAudienceClients
) {
// Base implementation builds standard OIDC response...
AccessTokenResponse res = responseBuilder.build();
res.setOtherClaims(OAuth2Constants.ISSUED_TOKEN_TYPE, requestedTokenType);
event.success();
return cors.add(Response.ok(res, MediaType.APPLICATION_JSON_TYPE));
}Then, in our IDJWTTokenExchangeProvider, we can simply override this buildTokenExchangeResponse() method, extract the fully-evaluated token, perform our secure whitelist manual copy into a clean JsonWebToken, sign it, and return the customized response: @Override
protected Response buildTokenExchangeResponse(
org.keycloak.protocol.oidc.TokenManager.AccessTokenResponseBuilder responseBuilder,
String requestedTokenType,
List<ClientModel> targetAudienceClients
) {
// 1. Let the base provider/TokenManager centrally resolve expirations, policies, and mappers
AccessToken transformedToken = responseBuilder.getAccessToken();
// 2. Safely copy ONLY necessary evaluated claims (Whitelist approach)
JsonWebToken idjag = new JsonWebToken();
idjag.id(transformedToken.getId());
idjag.issuer(transformedToken.getIssuer());
idjag.issuedAt(transformedToken.getIssuedAt());
// 3. Extract dynamically evaluated claims from the standard pipeline
if (transformedToken.getOtherClaims() != null) {
}
// 4. Sign only the clean 'idjag' assertion
String encodedToken = new JWSBuilder()
.type(OAuth2Constants.IDENTITY_ASSERTION_JWT_HEADER_TYPE) // oauth-id-jag+jwt
.jsonContent(idjag)
.sign(signer);
// 5. Build AccessTokenResponse with token_type = "N_A" and return
}What do you think about this proposed approach? |
Regarding Duplication 3 & Duplication 4:I agree with your suggestions for both points, and I will refactor them accordingly to fully eliminate these duplications. |
|
@bucchi Hello, Thank you for considering Duplication 1. If your codes in If not, I think it is OK to keep the current overriden |
|
@bucchi Hello, Thank you for considering Duplication 2. Your proposal (The Simplified Hybrid Approach) makes sense to me. |
Hi @tnorimat, |
Hi @tnorimat, |
@tnorimat Duplication 3: getRequestedTokenType() now just returns OAuth2Constants.IDENTITY_ASSERTION_JWT_TOKEN_TYPE, since supports() already restricts requested_token_type to id-jag. |
|
@bucchi Hello, I conformed that you resolved my review comments. Thank you. I'd make approval conditional on item 1 (revert the unneeded
|
…d sends back ID-JAG assertion working as a tokenexchange endpoint Signed-off-by: Yutaka Obuchi <yutaka.obuchi.sd@hitachi.com>
|
Hi @tnorimat,
AbstractTokenExchangeProvider and V1 token exchange are no longer touched by this PR.
Could you please take another look when you have time? |
|
@bucchi Hello, I confirmed that you fixed the issues I commented. I found that the document
It is good to update the guide. For example
What do you think about that? |
|
Hi @tnorimat,
|
closes #48818
This PR adds an ID-JAG issuer to the token exchange endpoint: it receives an ID token and sends back an ID-JAG.
ID-JAG is a type of JWT that can be used as an authorization grant (https://www.rfc-editor.org/rfc/rfc7523.html#section-2.1).
Design overview
The Resource AS is registered as a client of the realm. The
audienceparameter must be theclientIdof that client, so to meet the draft (aud= issuer of the Resource AS), the client must be named with the issuer URL of the Resource AS. Exactly oneaudienceparameter is accepted.client_idcomes from a Hardcoded Claim mapper (JSON type String), and the granted scopes from client scopes. The ID-JAG is built from the evaluated access token, copying only an allowlisted set of claims (iss,sub,aud,iat,exp,jti,scope,auth_time,acr,client_id,amr), plus the Keycloaktypclaim (IDJAG). The JWT header type isoauth-id-jag+jwt.The ID-JAG provider reuses the standard token exchange flow, including the audience check, transient session handling and the
TokenExchangeResponseContextclient policies. It only overrides the subject token verification and the response encoding.Only ID tokens are accepted. The ID token must be bound to an online user session that is not being logged out: ID tokens without
sid(transient sessions) and ID tokens bound only to an offline session are rejected.actor_tokenandresourceare rejected.Administrator responsibilities
subThe requesting client and the Resource AS must use the same subject identifier: either no pairwise identifiers, or a pairwise mapper in a default client scope with the same sector for all clients that take part in ID-JAG.
The mappers producing
subandclient_idneed "Add to lightweight access token" enabled when the requesting client uses lightweight access tokens.The ID-JAG is signed with the ID token signature algorithm of the requesting client; it must be asymmetric.
jwt-claim-enforcerThis executor only supports access tokens as the subject token for token exchange, so it rejects ID-JAG exchanges for the clients it applies to. It should not be applied to clients that request ID-JAGs.