Add SPIFFE claim support to ZTS ID token exchange#3254
Add SPIFFE claim support to ZTS ID token exchange#3254ctyano wants to merge 4 commits intoAthenZ:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the ZTS (ZMS Token Service) ID token exchange functionality by introducing support for SPIFFE (Secure Production Identity Framework For Everyone) claims. It enables the ZTS to propagate SPIFFE IDs from incoming subject tokens to newly issued ID and Access tokens, ensuring that identity context is maintained across token exchanges. Furthermore, it adds a crucial validation step to confirm that the SPIFFE ID in the subject token aligns with the authenticated Athenz principal, thereby strengthening the security posture of the token exchange process. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request adds support for propagating SPIFFE claims during various token exchange flows, which is a valuable enhancement. The implementation correctly adds the SPIFFE claim to new tokens in impersonation, delegation, and ID token exchange flows. It also introduces a security verification for the SPIFFE claim in the ID token exchange flow. Additionally, the PR includes a fix for an authorization check in the access token delegation flow and corrects the authorization logic in the ID token exchange flow. The test coverage for the new functionality is good. I have one critical comment regarding the principal being used for SPIFFE ID verification and a couple of medium-severity suggestions to enhance security by adding similar verification to other flows.
| if (spiffeId != null) { | ||
| // If we're copying SPIFFE claim into the requested ID token, verify it maps | ||
| // to the authenticated principal to prevent mismatched SPIFFE identities. | ||
| verifySpiffeIdMatchesAthenzPrincipal(spiffeId, principalName, caller, domainName, principalDomain); |
There was a problem hiding this comment.
The verifySpiffeIdMatchesAthenzPrincipal method is called with principalName, which is the identity of the caller making the token exchange request. However, the spiffeId is extracted from the subjectToken, and the new ID token is being issued for the subjectIdentity. The verification should be against the subjectIdentity to ensure the SPIFFE ID correctly corresponds to the subject of the token, preventing a caller from injecting a SPIFFE ID that doesn't belong to the token's subject.
The comment on line 3065 mentions "authenticated principal", which is the caller, but for security, the check should be against the identity for which the claim is being made.
Please note that with this change, the test testIdTokenExchangeSuccessWithSpiffeClaim in ZTSImplIDTokenTest.java will likely fail and will need to be updated. The spiffeId used in that test should correspond to the subject of the token being created.
| verifySpiffeIdMatchesAthenzPrincipal(spiffeId, principalName, caller, domainName, principalDomain); | |
| verifySpiffeIdMatchesAthenzPrincipal(spiffeId, subjectIdentity, caller, domainName, principalDomain); |
| final String spiffeId = extractSpiffeIdFromToken(subjectToken); | ||
| if (spiffeId != null) { | ||
| accessToken.setCustomClaim(IdToken.CLAIM_SPIFFE, spiffeId); | ||
| } |
There was a problem hiding this comment.
For consistency and improved security, it would be best to verify that the SPIFFE ID from the subject token corresponds to the subject of the token, similar to the check in processIdTokenExchangeRequest. This prevents the propagation of a potentially mismatched SPIFFE ID if the subject token were ever compromised or misconfigured.
| final String spiffeId = extractSpiffeIdFromToken(subjectToken); | |
| if (spiffeId != null) { | |
| accessToken.setCustomClaim(IdToken.CLAIM_SPIFFE, spiffeId); | |
| } | |
| final String spiffeId = extractSpiffeIdFromToken(subjectToken); | |
| if (spiffeId != null) { | |
| verifySpiffeIdMatchesAthenzPrincipal(spiffeId, subjectToken.getSubject(), caller, requestDomainName, principalDomain); | |
| accessToken.setCustomClaim(IdToken.CLAIM_SPIFFE, spiffeId); | |
| } |
| final String spiffeId = extractSpiffeIdFromToken(subjectToken); | ||
| if (spiffeId != null) { | ||
| accessToken.setCustomClaim(IdToken.CLAIM_SPIFFE, spiffeId); | ||
| } |
There was a problem hiding this comment.
For consistency and improved security, it would be best to verify that the SPIFFE ID from the subject token corresponds to the subject of the token, similar to the check in processIdTokenExchangeRequest. This prevents the propagation of a potentially mismatched SPIFFE ID if the subject token were ever compromised or misconfigured.
| final String spiffeId = extractSpiffeIdFromToken(subjectToken); | |
| if (spiffeId != null) { | |
| accessToken.setCustomClaim(IdToken.CLAIM_SPIFFE, spiffeId); | |
| } | |
| final String spiffeId = extractSpiffeIdFromToken(subjectToken); | |
| if (spiffeId != null) { | |
| verifySpiffeIdMatchesAthenzPrincipal(spiffeId, subjectToken.getSubject(), caller, requestDomainName, principalDomain); | |
| accessToken.setCustomClaim(IdToken.CLAIM_SPIFFE, spiffeId); | |
| } |
Signed-off-by: Tatsuya Yano <tatyano@lycorp.co.jp> Signed-off-by: CTY <christopher.t.yano@gmail.com> Signed-off-by: ctyano <ctyano@duck.com>
- Add explicit authorization check for source->target token exchange - Fix ID token exchange access check to validate the caller principal - Update unit tests to include token source exchange policy Signed-off-by: CTY <christopher.t.yano@gmail.com> Signed-off-by: ctyano <ctyano@duck.com>
Signed-off-by: CTY <christopher.t.yano@gmail.com> Signed-off-by: ctyano <ctyano@duck.com>
f6993fb to
0a81720
Compare
Signed-off-by: ctyano <ctyano@duck.com>
| // out token delegation from source to target domain | ||
|
|
||
| final String resource = sourceDomainName + ":" + requestDomainName; | ||
| if (!authorizer.access(ZTSConsts.ZTS_ACTION_TOKEN_SOURCE_EXCHANGE, resource, principal, null)) { |
There was a problem hiding this comment.
why do we need this authorization check?
First, we've already verified at the beginnig of the method that princpal name is the same as the subject in the actor token.
Next, when validating the actor subject token in the token request parser, we've already verified that the subject token includes the mayAct claim and the value matches to the subject of the actor token. So the authorization has already been granted and verified so not sure we're adding yet another authorization check here?
| return null; | ||
| } | ||
|
|
||
| final Object spiffeClaim = token.getClaim(IdToken.CLAIM_SPIFFE); |
There was a problem hiding this comment.
so the claim itself is called spiffe but that's only valid for ZTS itself. If we're doing an exchange from another identity provider, then most likely this isn't going to work.
if it's a spiffe svid, then spiffie uri is in the sub claim and that's supported.
So if it's any other claim used by that identity provider then it's not supported. This may not be a problem if we're only dealing with ZTS tokens or spiffe svids but something to be aware of. maybe we can add comments about this limitation
| Principal subjectPrincipal = createPrincipalForName(subjectIdentity); | ||
| for (String idTokenGroup : idTokenGroups) { | ||
| if (!authorizer.access(ZTSConsts.ZTS_ACTION_ID_TOKEN_EXCHANGE, idTokenGroup, subjectPrincipal, null)) { | ||
| if (!authorizer.access(ZTSConsts.ZTS_ACTION_ID_TOKEN_EXCHANGE, idTokenGroup, principal, null)) { |
There was a problem hiding this comment.
why was this changed? it's an incorrect change.
We're checking if the principal in the subject token is authorized for the change not the principal who requested the exchange (which could be some proxy token exchange broker).
a few lines above, we verify that the principal in the request is authorized to request the token exchange on behalf of the subject principal and here we must verify that subject principal is authorized for the requested role
Description
#3233
Contribution Checklist:
Attach Screenshots (Optional)