-
Notifications
You must be signed in to change notification settings - Fork 303
Add SPIFFE claim support to ZTS ID token exchange #3254
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
32c4c9c
4f1bdb3
0a81720
5ff47be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2620,6 +2620,27 @@ AccessTokenResponse processAccessTokenExchangeRequest(ResourceContext ctx, Princ | |||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| String extractSpiffeIdFromToken(final OAuth2Token token) { | ||||||||||||||||||||
| if (token == null) { | ||||||||||||||||||||
| return null; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| final Object spiffeClaim = token.getClaim(IdToken.CLAIM_SPIFFE); | ||||||||||||||||||||
| if (spiffeClaim != null) { | ||||||||||||||||||||
| final String spiffeId = spiffeClaim.toString(); | ||||||||||||||||||||
| if (!StringUtil.isEmpty(spiffeId) && spiffeId.startsWith(ZTSConsts.ZTS_CERT_SPIFFE_URI)) { | ||||||||||||||||||||
| return spiffeId; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| final String subject = token.getSubject(); | ||||||||||||||||||||
| if (!StringUtil.isEmpty(subject) && subject.startsWith(ZTSConsts.ZTS_CERT_SPIFFE_URI)) { | ||||||||||||||||||||
| return subject; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| return null; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| AccessTokenResponse processAccessTokenImpersonationRequest(ResourceContext ctx, Principal principal, | ||||||||||||||||||||
| AccessTokenRequest accessTokenRequest, final String principalDomain, final String caller) { | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -2710,6 +2731,11 @@ AccessTokenResponse processAccessTokenImpersonationRequest(ResourceContext ctx, | |||||||||||||||||||
| accessToken.setIssuer(issuerResolver.getAccessTokenIssuer(ctx.request(), accessTokenRequest.isUseOpenIDIssuer())); | ||||||||||||||||||||
| accessToken.setScope(new ArrayList<>(roles)); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| final String spiffeId = extractSpiffeIdFromToken(subjectToken); | ||||||||||||||||||||
| if (spiffeId != null) { | ||||||||||||||||||||
| accessToken.setCustomClaim(IdToken.CLAIM_SPIFFE, spiffeId); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
+2734
to
+2737
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| // if we have a certificate used for mTLS authentication then | ||||||||||||||||||||
| // we're going to bind the certificate to the access token | ||||||||||||||||||||
| // and the optional proxy principals if specified | ||||||||||||||||||||
|
|
@@ -2831,6 +2857,15 @@ AccessTokenResponse processAccessTokenDelegationRequest(ResourceContext ctx, Pri | |||||||||||||||||||
| caller, requestDomainName, principalDomain); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // now let's verify that our principal is authorized to carry | ||||||||||||||||||||
| // 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)) { | ||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||||||||||
| throw forbiddenError("Principal not authorized for token delegation from source domain", | ||||||||||||||||||||
| caller, requestDomainName, principalDomain); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // make sure our principal is authorized to request a token | ||||||||||||||||||||
| // exchange for the given roles | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -2862,6 +2897,11 @@ AccessTokenResponse processAccessTokenDelegationRequest(ResourceContext ctx, Pri | |||||||||||||||||||
| accessToken.setIssuer(issuerResolver.getAccessTokenIssuer(ctx.request(), accessTokenRequest.isUseOpenIDIssuer())); | ||||||||||||||||||||
| accessToken.setScope(new ArrayList<>(roles)); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| final String spiffeId = extractSpiffeIdFromToken(subjectToken); | ||||||||||||||||||||
| if (spiffeId != null) { | ||||||||||||||||||||
| accessToken.setCustomClaim(IdToken.CLAIM_SPIFFE, spiffeId); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
+2900
to
+2903
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| // include the act claim in our response. we're going to use | ||||||||||||||||||||
| // the act claim from the original token and then add our new | ||||||||||||||||||||
| // actor on top of it | ||||||||||||||||||||
|
|
@@ -2995,14 +3035,13 @@ AccessTokenResponse processIdTokenExchangeRequest(ResourceContext ctx, Principal | |||||||||||||||||||
| List<String> idTokenGroups = processIdTokenRoles(subjectIdentity, tokenScope, domainName, true, | ||||||||||||||||||||
| false, principalDomain, caller); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // make sure our principal is authorized to request a jag token | ||||||||||||||||||||
| // make sure our principal is authorized to request an id token | ||||||||||||||||||||
| // exchange for the given roles | ||||||||||||||||||||
|
|
||||||||||||||||||||
| 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)) { | ||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||||||||||||||||
| LOGGER.error("access check failure:({}, {}, {})", ZTSConsts.ZTS_ACTION_ID_TOKEN_EXCHANGE, | ||||||||||||||||||||
| idTokenGroup, subjectPrincipal); | ||||||||||||||||||||
| idTokenGroup, principal); | ||||||||||||||||||||
| throw forbiddenError("Principal not authorized for token exchange for the requested role", | ||||||||||||||||||||
| caller, domainName, principalDomain); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
@@ -3020,6 +3059,14 @@ AccessTokenResponse processIdTokenExchangeRequest(ResourceContext ctx, Principal | |||||||||||||||||||
| idToken.setIssueTime(iat); | ||||||||||||||||||||
| idToken.setAuthTime(iat); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| final String spiffeId = extractSpiffeIdFromToken(subjectToken); | ||||||||||||||||||||
| 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); | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The 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
Suggested change
|
||||||||||||||||||||
| idToken.setSpiffe(spiffeId); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // for user principals we're going to use the default 1 hour while for | ||||||||||||||||||||
| // service principals 12 hours as the max timeout, unless the client | ||||||||||||||||||||
| // is explicitly asking for something smaller. | ||||||||||||||||||||
|
|
@@ -3035,6 +3082,35 @@ AccessTokenResponse processIdTokenExchangeRequest(ResourceContext ctx, Principal | |||||||||||||||||||
| .setIssued_token_type(ZTSConsts.OAUTH_TOKEN_TYPE_ID).setExpires_in(tokenTimeout); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| void verifySpiffeIdMatchesAthenzPrincipal(final String spiffeId, final String principalName, | ||||||||||||||||||||
| final String caller, final String domainName, final String principalDomain) { | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (StringUtil.isEmpty(spiffeId) || StringUtil.isEmpty(principalName)) { | ||||||||||||||||||||
| return; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // if the principal itself is a spiffe uri (jwt-svid spiffe-subject) then it must match exactly | ||||||||||||||||||||
| if (principalName.startsWith(ZTSConsts.ZTS_CERT_SPIFFE_URI)) { | ||||||||||||||||||||
| if (!spiffeId.equals(principalName)) { | ||||||||||||||||||||
| throw requestError("SPIFFE ID does not match authenticated principal", | ||||||||||||||||||||
| caller, domainName, principalDomain); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| return; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| final String principalSpiffeDomain = AthenzUtils.extractPrincipalDomainName(principalName); | ||||||||||||||||||||
| final String principalSpiffeService = AthenzUtils.extractPrincipalServiceName(principalName); | ||||||||||||||||||||
| if (principalSpiffeDomain == null || principalSpiffeService == null) { | ||||||||||||||||||||
| throw requestError("Invalid principal name for SPIFFE validation: " + principalName, | ||||||||||||||||||||
| caller, domainName, principalDomain); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (!spiffeUriManager.validateServiceCertUri(spiffeId, principalSpiffeDomain, principalSpiffeService, null)) { | ||||||||||||||||||||
| throw requestError("SPIFFE ID does not match authenticated principal", | ||||||||||||||||||||
| caller, domainName, principalDomain); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| AccessTokenResponse processJAGTokenIssueRequest(ResourceContext ctx, Principal principal, | ||||||||||||||||||||
| AccessTokenRequest accessTokenRequest, final String principalDomain, final String caller) { | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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