Skip to content
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

Session ID is lost during Token Exchange #1312

Closed
tos-ilex opened this issue Jul 2, 2024 · 10 comments
Closed

Session ID is lost during Token Exchange #1312

tos-ilex opened this issue Jul 2, 2024 · 10 comments

Comments

@tos-ilex
Copy link

tos-ilex commented Jul 2, 2024

Which version of Duende IdentityServer are you using?
7.0.5

Which version of .NET are you using?
8

Describe the bug
When calling /connect/token with grant_type urn:ietf:params:oauth:grant-type:token-exchange and an existing access token with a sid claim, the resulting access token does not have a sid claim. A resulting refresh token is not correlated with the session, and an entry with SessionId=null is added to the persisted grant store for each exchange.

This is causing issues for us because we exchange tokens quite frequently and each one creates a fairly long-lived, orphaned refresh token entry in the persisted grant database that remains after logout because its session id is null.

To Reproduce

  1. get access token with a session id in the sid claim and offline_access in its scopes.
  2. make token exchange without specifying scopes while the destination client has offline_access allowed.
  3. observe that resulting access token has no sid claim.
  4. observe that persisted grant store has new refresh_token entry with SessionId=NULL.

Expected behavior

sid of given access token should be put into exchanged token. No extra refresh token should go into the persisted grant store, rather the existing one should be renewed or replaced.

Additional context
Our TokenExchangeGrantValidator:

public async Task ValidateAsync(ExtensionGrantValidationContext context)
    {
        context.Result = new GrantValidationResult(TokenRequestErrors.InvalidRequest);

        ... //some validation that does nothing or lets the request fail

        string? subjectTokenType = context.Request.Raw.Get(OidcConstants.TokenRequest.SubjectTokenType);
        TokenValidationResult? validationResult;
        switch (subjectTokenType)
        {
            case OidcConstants.TokenTypeIdentifiers.AccessToken:
                validationResult = await _validator.ValidateAccessTokenAsync(subjectToken); // Default ITokenValidator
                break;
            default:
                context.Result.ErrorDescription = TokenConstants.Errors.UnsupportedTokenType;
                return;
        }

        string sub = validationResult.Claims.First(c => c.Type == JwtClaimTypes.Subject).Value;

        var customClaims = new List<Claim>();

        string? redacted = context.Request.Raw.Get("redacted");
        if (redacted == null) // issue happens in both cases.
        {
            context.Result = new GrantValidationResult(sub, GrantType, customClaims);
            return;
        }

        customClaims.Add(new Claim("redacted"));

        context.Result = new GrantValidationResult(sub, GrantType, customClaims);
    }
@saithis
Copy link

saithis commented Jul 2, 2024

And this together with the slow token cleanup query is a bad combination #1304

@josephdecock
Copy link
Member

Have you tried copying the session id into the custom claims? We don't automatically copy the sid forward into an exchanged token because depending on the semantics of the exchange that may or may not be appropriate. But I can't think of anything that would prevent that from working.

@josephdecock josephdecock self-assigned this Jul 11, 2024
@tos-ilex
Copy link
Author

I have added this to our IProfileService implementation:

if (context.ValidatedRequest.SessionId != null)
    claims.Add(new Claim("sid", context.ValidatedRequest.SessionId));

context.AddRequestedClaims(claims);

This does result in the exchanged token having a correct sid claim. However, a new refresh token is issued anyway, and its session id in the Persisted Grant store is null:
image
Screenshot from my local test db after I

  • Deleted all persisted grants
  • Went through an auth code flow to get tokens including a refresh token
  • Exchanged the access token via token exchange

Let me clarify that our main problem isn't the missing sid claim (I just thought it was a clear sign that something is wrong). It's that new, "orphaned" refresh tokens are written to the PG store, making our DB fill up rapidly because we do frequent token exchanges. This is causing performance issues.

@AndersAbel
Copy link
Member

I'm revisiting this. I think that you correctly identified the issue in the original post:

No extra refresh token should go into the persisted grant store, rather the existing one should be renewed or replaced.

Token exchange swaps one access token for another. It should not involve any refresh token at all. The request does not include any refresh token from the original session, so (if such a refresh token exists) it shouldn't be updated. The result of the token exchange is another access token - it should not create any new refresh token.

Do you have custom code that is used to create the token that is the result of the token exchange?

@tos-ilex
Copy link
Author

Do you have custom code that is used to create the token that is the result of the token exchange?

We use custom implementations for IProfileService and ITokenExchangeGrantValidator. My original post has the relevant parts of the latter. Our IProfileService implementation only adds some custom claims. Otherwise, no.

@josephdecock
Copy link
Member

@tos-ilex - it's been a while but I took another look at this issue.

Do you have the AllowOfflineAccess flag enabled for the client that is performing the token exchange? If so, do you actually need it on? Normally - or at least what I think of as "typical"/what our token exchange sample does - is that an API is exchanging one token for another. That api is configured as a client itself (since it makes token requests) and that client doesn't have offline_access enabled.

If in your use case, you want a client to perform the exchange and separately obtain refresh tokens, I think there is a way to have that configuration and prevent refresh token creation (though it admittedly is a bit of a hack). Try adding this to your ITokenExchangeGrantValidator:

context.Request.ValidatedResources.Resources.OfflineAccess = false;

The underlying token generation code is checking for that flag to decide if it should issue a refresh token, so disabling it should prevent the unnecessary refresh token generation, if you need offline access on that client elsewhere. If that's the case, I'd love to hear more about your use case, because this is a hard thing to know that you ought to do and if enough folks are trying to do the same thing, it might be a good idea to change how IdentityServer interacts with refresh tokens and token exchange.

Thanks in advance - and sorry for the long delay on this issue!

@tos-ilex
Copy link
Author

Do you have the AllowOfflineAccess flag enabled for the client that is performing the token exchange? If so, do you actually need it on?

The client for which I tested this behavior has OfflineAccessEnabled. The original (pre-exchange) token has the offline_access scope. We do need refresh tokens, as most of our applications are SPAs with a BFF and resource API of their own. We want to allow our users to work without entering their credentials or go back to the IdP at all again, at least for a day. So we use short-lived access tokens and longer-lived (8h) refresh tokens.

Our main use case for token exchanges is this:
Our company has multiple apps. Each app consists of a SPA frontend, a BFF using the Duende.BFF framework and one or more backend APIs that have both "internal" endpoints to be called by the frontend ("through" the BFF) and "public" endpoints to be called by other apps in the company. Each backend API uses jwt bearer authentication with one central Duende IdentityServer as the IdP. Each app has one or more clients with their own set of credentials and allowed scopes.

Our SPAs link to each other and appear like "modules" of the same app to the user. When the user switches from one SPA to the next, they bring the token to the next app and it is exchanged for one that matches the scopes/audiences needed for that app's backend. We want a refresh token after the exchange as well, for the previously mentioned reasons.

Each client has a BackchannelLogoutUrl to ensure that after a session that spanned multiple apps in our ecosystem, when they log out of one, they're logged out of all of them. I assume the "SessionId" in the persisted grants store is meant to be used here: so we can get rid of every grant that was part of that session.

So essentially, we're using token exchanges to facilitate SSO between our apps which all use the same Duende IdentityServer as an IdP. And we like that we get refresh tokens when exchanging. Are we misusing token exchanges? I reviewed RFC8693 and it doesn't seem to be a default case to get refresh tokens from an exchange. Are we supposed to redirect back to the IdP on each app-switch instead?

@AndersAbel
Copy link
Member

AndersAbel commented Nov 19, 2024

So essentially, we're using token exchanges to facilitate SSO between our apps which all use the same Duende IdentityServer as an IdP. And we like that we get refresh tokens when exchanging. Are we misusing token exchanges? I reviewed RFC8693 and it doesn't seem to be a default case to get refresh tokens from an exchange. Are we supposed to redirect back to the IdP on each app-switch instead?

Thank you for the clarification. And the answer is: YES, you are trying to use token exchange for something it was never designed for. If you have multiple clients (with separate client ids) they are all supposed to redirect to the IdentityServer authorize endpoint to initiate their sessions. That will also enlist the clients in the IdentityServer's distributed session, which is what makes them receive logout notifications eventually.

If all of your apps run on the same site as the IdentityServer you may use the BFF silent login feature to not have to do a top level redirect from each SPA. https://docs.duendesoftware.com/identityserver/v7/bff/session/management/silent-login/

@tos-ilex
Copy link
Author

Okay, for these flows, we will just switch to silent logins. Thank you for clarifying.

For the remaining token exchanges we do, we don't need refresh tokens then. However, the token endpoint will still return refresh tokens and unnecessarily fill up the persisted grant store in the process, if the destination client has the offline_access scope allowed and no scopes are explicitly requested or offline_access is in the requested scopes. This is in line with your docs as it will be treated as requesting all allowed scopes for the client if none are specified.

If I understand you correctly, there is no reason for refresh tokens to be issued during a token exchange. According to @AndersAbel 's original reply:

Token exchange swaps one access token for another. It should not involve any refresh token at all. The request does not include any refresh token from the original session, so (if such a refresh token exists) it shouldn't be updated. The result of the token exchange is another access token - it should not create any new refresh token.

So perhaps it would make sense to stop the token endpoint from creating new refresh tokens if the grant type is token exchange? That is what we will do to improve performance on our end at least. If you see no need to change the default behavior on your end, I think this issue can be closed.

@AndersAbel
Copy link
Member

There are two issues that are brought up here that we have now investigated.

  1. The SID column not being populated.
  2. If a refresh token should be issued or not for token exchange scenarios.

The SID column not being populated is caused by the sid claim being added in the wrong way. There is a guard in the DefaultClaimsService to prevent the IProfileService from issuing protocol claims. The sid claims is on this list, so if it is added from the IProfileService it will be discarded.

Even if I add a sid claim from the profile service I do not get any sid claim in the generated access token, do you?

The correct way to add a sid to access tokens generated through token exchange is in the extensions grant validator:

context.Request.SessionId = requestSid;

With this, the sid column in the database is also correctly populated.

In a token exchange request, the new access token created will have the scopes listed in the token exchange request. If no scopes are listed, all scopes that the client has access to will be included. This also applies to the offline_access scope. If no scopes are requested and the client has offline access enabled there will be a refresh token. My personal opinion is that this could have been designed differently. But this is also a very odd situation - token exchange is normally performed by APIs that would not have offline access enabled. Changing the default would be a breaking change so I do not see that happening.

It is possible to disable the generation of a refresh from the extensions grant validator:

context.Request.ValidatedResources.Resources.OfflineAccess = false;

The result of our investigations is thus that there is no bug. The scenario is very specific and thus not well documented. It looks like you've found a solution by disabling the refresh token generation, so we'll close this issue now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants