Feat: Access Token with Multiple Domain Supported for Full Token Exchange - #3407
Feat: Access Token with Multiple Domain Supported for Full Token Exchange#3407mlajkim wants to merge 33 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for cross-domain scopes in access tokens within Athenz ZTS, allowing clients to request roles across multiple domains. It updates token validation, authorization checks, and scope generation to handle multiple domains, and adds comprehensive unit tests. The review feedback highlights three key improvement opportunities: refining getSubjectTokenRolesForDomain to prevent non-role scopes (like openid or service scopes) from being incorrectly treated as roles, trimming the scope claim in tokenExchangeRequestedRoles to handle leading/trailing whitespace robustly, and generalizing the domain limit check in OAuthTokenScope to support arbitrary maxDomains values.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for cross-domain access token requests and refactors token exchange requested roles logic in ZTS. The reviewer identified a potential security risk and NullPointerException vulnerability if the subject token lacks an audience claim. To address this, the reviewer suggested explicitly validating that the source domain name is not null or empty, which in turn allows simplifying downstream helper methods and requires updating associated unit tests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for cross-domain and multiple-domain scopes in access token requests within Athenz ZTS. It updates token parsing and validation logic to handle multiple domains, aggregates roles across domains, and calculates the minimum token timeout. Review feedback suggests improving the robustness of the token timeout calculation by using Integer.MAX_VALUE as an initial value instead of a zero sentinel, preventing empty role names from being added to requested roles, and adding a null check in a test helper to avoid a potential NullPointerException.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for access token requests with multiple scope domains (cross-domain scopes) and an audience. It updates the token validation, exchange, and generation logic in ZTSImpl to handle multiple domains, increases the domain limit in AccessTokenScope and OAuthTokenScope, and adds comprehensive unit tests. The review feedback suggests removing unused XML validation imports in ZTSImpl and optimizing the retrieval of single elements from domainNames in AccessTokenScope and OAuthTokenScope by using iterator().next() instead of streaming.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for requesting access tokens with scopes spanning multiple domains. It refactors token processing, validation, and exchange logic in ZTSImpl to handle multiple scope domains, and updates AccessTokenScope and OAuthTokenScope to allow more than one domain. The review feedback highlights a potential Denial of Service (DoS) vulnerability due to setting the domain limit to Integer.MAX_VALUE, suggesting a lower default limit instead. Additionally, a redundant check for multiple domains in OpenID scope requests was identified in ZTSImpl since this validation is already handled during AccessTokenScope construction.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for requesting access tokens with scopes spanning multiple domains. It updates the ZTS server to parse, validate, and authorize multi-domain scopes (up to a configurable limit, defaulting to 20), updates the token exchange logic to handle cross-domain scopes, and updates the documentation and unit tests accordingly. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@havetisyan please let me know what your thoughts on this PR. it is a big change in terms of the direction but all I really want to implement is to have multiple domain token exchange and if you think there are better ways to implement this, I would love to hear (or during the Inc meeting) |
There was a problem hiding this comment.
Pull request overview
This PR extends ZTS Access Token scope handling to support multiple scope domains when an explicit audience is provided, enabling token-exchange chains to carry and validate cross-domain delegated role scopes while preserving legacy single-domain behavior.
Changes:
- Update access token issuance to emit audience-domain roles as short names and non-audience roles as fully-qualified
<domain>:role.<role>scopes. - Update token-exchange scope validation to understand fully-qualified scopes and derive per-domain requested roles from the subject token.
- Introduce a configurable max-domains limit for access-token scopes, with tests and documentation updates.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| servers/zts/src/test/java/com/yahoo/athenz/zts/ZTSTest.java | Adds a basic factory instantiation test for ZTSImplFactory. |
| servers/zts/src/test/java/com/yahoo/athenz/zts/ZTSImplAccessTokenTest.java | Adds/updates tests for cross-domain access-token scopes and token-exchange parsing/validation. |
| servers/zts/src/test/java/com/yahoo/athenz/zts/token/AccessTokenScopeTest.java | Updates scope parsing tests to allow multiple domains and adds max-domain coverage. |
| servers/zts/src/main/java/com/yahoo/athenz/zts/ZTSImpl.java | Implements multi-domain access-token scope issuance and updates token-exchange scope validation logic. |
| servers/zts/src/main/java/com/yahoo/athenz/zts/ZTSConsts.java | Adds athenz.zts.access_token_max_domains constant. |
| servers/zts/src/main/java/com/yahoo/athenz/zts/token/OAuthTokenScope.java | Fixes single-domain detection and max-domain enforcement behavior. |
| servers/zts/src/main/java/com/yahoo/athenz/zts/token/AccessTokenScope.java | Adds configurable max-domains for access-token scopes and enforces openid single-domain limitation. |
| servers/zts/conf/zts.properties | Documents the new access-token max-domains configuration property. |
| docs/zts_access_token_guide.md | Updates guide to describe multi-domain scopes, required audience, and new scp semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e00d154 to
6cf9dfa
Compare
|
@havetisyan (cc: @ctyano) Hey, I think this PR is ready to be reviewed! Let me know what you think! |
Signed-off-by: Aaron Jeongwoo Kim <53258958+mlajkim@users.noreply.github.com>
Signed-off-by: Aaron Jeongwoo Kim <53258958+mlajkim@users.noreply.github.com>
Signed-off-by: Aaron Jeongwoo Kim <53258958+mlajkim@users.noreply.github.com>
Signed-off-by: Aaron Jeongwoo Kim <53258958+mlajkim@users.noreply.github.com>
Signed-off-by: Aaron Jeongwoo Kim <53258958+mlajkim@users.noreply.github.com>
Signed-off-by: Jeongwoo Kim - jekim <jekim@lycorp.co.jp>
6cf9dfa to
93e08fb
Compare
|
(rebased) |
|
(update branch; still reviewable) |
|
(update branch; still reviewable) |
|
I don't mean to rush you, but we are trying to implement the token exchange cases internally so I would greatly appreciate your feedback on this when possible. Thanks 🙏 |
| // even when their audience is a different source domain. Keep the legacy source-domain | ||
| // resource for existing deployments that still authorize exchanges that way. | ||
|
|
||
| final String targetRoleResource = targetDomainName + ":" + |
There was a problem hiding this comment.
didn't quite understand why the targetDomainName is preferred here where we're doing the check in the targetDomainName itself. the idea is to tie the source and target domains in the authorization and your change completely removes that.
There was a problem hiding this comment.
Oops, I didn't mean to add such a breaking change.
Just removed it and roll backed to where it has been now:
| private static boolean supportRolesWithoutDomain = Boolean.parseBoolean( | ||
| System.getProperty(ZTSConsts.ZTS_PROP_SCOPE_ROLE_WOUT_DOMAIN, "false")); | ||
| private static int maxDomains = Integer.parseInt( | ||
| System.getProperty(ZTSConsts.ZTS_PROP_ACCESS_TOKEN_MAX_DOMAINS, "20")); |
There was a problem hiding this comment.
the default value must be 1 to maintain existing functionality.
| public static String OBJECT_ROLES = "roles"; | ||
|
|
||
| Set<String> domainNames = new HashSet<>(); | ||
| Set<String> domainScopeNames = new HashSet<>(); |
There was a problem hiding this comment.
not sure if I followed what was the reason to add a new hashset for every token scope object? the only place it's used is in getRoleNames call though I'm not sure if it does anything since originally we were not setting the roleNames when sendScopeResponse was true, but that logic was changed
There was a problem hiding this comment.
Good point. The reason was to handle mixed multi-domain scopes such as:
sports:domain weather:role.reader
With the old global sendScopeResponse behavior, once sports:domain is present we would not keep roleNames at all. That works for the old single-domain case, but for multi-domain scopes it would also drop the weather:role.reader filter and make weather unfiltered as well.
That said, I agree we don't need to keep this as object state. I removed the field and now use it only as local parsing state to remove role/group filters for domains that requested {domain}:domain.
| final String requestDomainName, String[] requestedRoles) { | ||
|
|
||
| for (String requestedRole : requestedRoles) { | ||
| if (subjectScopes.contains(requestDomainName + OAuthTokenScope.OBJECT_ROLE + requestedRole)) { |
There was a problem hiding this comment.
we should use ResourceUtils.roleResourceName for consistency
| } | ||
|
|
||
| // first retrieve our domain data object from the cache | ||
| final String domainName = StringUtil.isEmpty(accessTokenRequest.getAudience()) ? |
There was a problem hiding this comment.
I'm somewhat concerned about this change. Unless I'm missing something - this could be pretty much any valid domain name and got nothing to do with the scope domains. That is not acceptable. Right now the applications are assuming that the audience in the access token is their domain name and then scope is the list of roles. Now, I can just create an access token with audience sports and then include roles from my domain and present that token to sports. The service may assume that since the audience is sports the user must have access to some role within the sports domain which is not the case. Am I missing some validation?
There was a problem hiding this comment.
You're right. I removed that carrier-token behavior as the following:
6aba3c0
For standard access token requests, when audience is specified it must now be one of the requested scope domains. Multi-domain scope requests still require an explicit audience, but that audience can no longer be unrelated to the scope domains. I also added a negative test for audience=sports&scope=weather:role.readers
|
Hey, thank you for the review! I took some time to thoroughly go through the feedback and make the updates. Please let me know what you think, and no rush. |

Background
Athenz Access Tokens currently work well when all requested role scopes belong to the same domain as the token audience. This has been enough for most existing use cases, as token exchange was not a primary flow and was only recently introduced to support ID-JAG.
With AI agent and MCP-style flows, token exchange becomes a core part of the request chain. For example:
In this flow, the first Access Token is issued by
home.mlajkim.claudefor the MCP Hub audience, but the caller also needs to explicitly delegate API access that the MCP server may exchange later.Problem
Today, the original subject token cannot cleanly carry scopes from multiple domains. This blocks flows where the token audience is
mcp-hub, but the delegated downstream permission is in another domain, such asapi:role.docs-getter.Relying solely on the subject's role membership for token exchange is not an option. We strictly cannot allow downstream services to exchange a token for scopes not present in the original token, as this would result in permission escalation. The original requester must explicitly define which permissions are passed into the exchange chain.
Therefore, the first access token must carry the exact role scopes that are allowed to be exchanged later, even when those scopes belong to domains other than the audience domain.
Changes
This PR adds multi-domain role scope support for Athenz Access Tokens.
When an Access Token is requested with an explicit audience:
Example:
audience=mcp-hubscope=mcp-hub:role.api-mcp-accessor api:role.docs-gettercan produce a token with:
{ "aud": "mcp-hub", "scp": [ "api-mcp-accessor", // implies it belongs to the aud "mcp-hub" "api:role.docs-getter" // not yet need for the aud "mcp-hub", but the "mcp-hub" will need it to exchange ] }This keeps existing single-domain behavior backward compatible while allowing explicit cross-domain delegation for token exchange.
Minor notes
athenz.zts.access_token_max_domainsaudonly when the requested scopes contain 2 or more different domainsscope=demo:role.readersscope=api:domainorscope=api:domain mcp-hub:role.accessors & audience=mcp-hub{ aud": "mcp-hub", "scp": ["api:role.reader"] }Contribution Checklist: