[Bug] MSAL Suggested cache key can create performance issues to due to over serialization

[RamjotSingh]

Which Version of MSAL are you using ? 4.16.1

Platform netcore

What authentication flow has the issue?

• Desktop / Mobile
  • [ ] Interactive
  • [ ] Integrated Windows Auth
  • [ ] Username Password
  • [ ] Device code flow (browserless)
• Web App
  • [ ] Authorization code
  • [ ] OBO
• Web API
  • [x] OBO

Other? - please describe;

Is this a new or existing app? The app is in production, and I have started using MSAL instead of ADAL

Issue MSAL today in it's suggested cache key makes following suggestions

• App context - ClientId+_AppTokenCache
• User context
  • Silent acquire - User account Id
  • OnBehalfOf exchange - Original token hash

However there are a couple of issues I see with this

Issue with App context cache

We recently onboarded MSAL (shifting away from ADAL) and during rollout we saw a trace (False) MSAL 4.16.1.0 MSAL.NetCore Microsoft Windows 10.0.14393 [07/18/2020 13:52:56] Serializing token cache with 4247 items.

This is happening because all app context tokens are being pushed into one single cache and since the tokens are tenantized, the moment a significant number of tenant hit the call the token cache just keeps on growing.

Issue with User context cache

For user context caches, since the cache key for OnBehalfOf exchange is the original token, as lots of callers make the call using unique clients (we have thousands of clients sending us potentially 100s of unique tokens per user) the token for same user gets cached over and over.

Solution which I propose

Based on the issues above, here is what I recommend as the token caches

• App context - ClientID+TenantId+_AppTokenCache
• User Context - ClientId + HomeAccountIdentifier (i.e. TenantID+UserId)

Additionally TokenCacheNotificationArgs should expose additional information like resourceId since for services which hit a lot of external services even tenant level cache might be too big and they might need to go granular.

[bgavrilMS]

CC @jmprieur @jennyf19

The main driver for this feature is Microsoft.Identity.Web

[jmprieur]

Thanks @RamjotSingh, this is good feedback. @bgavrilMS @jennyf19 @henrik-me let's discuss if we want to take this quickly (before many people use it, as it's a breaking change) . @RamjotSingh : For the OBO case we have to take the token hash for security reason (CA claims might have changed. the home account id is not enough). But could also add the client ID.

[bgavrilMS]

@jmprieur - would it not be enough to use home account id as the cache key? MSAL would still do the right thing and try to acquire OBO token with initial token hash...

[RamjotSingh]

@bgavrilMS @jmprieur What would be the implications of using accountId as cache key but still using token hashes as token reacquire pattern? Does MSAL store along side serialized data which token was used to generate this cache entry? If this is the case then I wonder if using token hash is actually a better approach. The test bed for any key should be, it should have manageable number of tokens given any of the following being true (all of these should be assuming distributed cache and in memory cache both implementations)

• Service runs a lot of servers (100s to 1000s)
• Service is hit by a lot of users and apps (100Ks to millions)
• Service is hit by a lot of varied clients (same user having multiple tokens coming thru multiple pipelines)
• Service makes calls to a lot of outgoing services (10s to 100s)
• Cache value should not be more than ~100KBs (Redis hates more than 100KB values)
• Assume tokens can have lot of scopes on them (imagine a Graph token with 10s of scopes)
• Tokens can have a few non-standard claims (tenant region, wids, incorp etc)
• AAD response can have refresh tokens

These are things which come to my mind right now. Current cache keys don't hold water for a lot of these.

[bgavrilMS]

Yes, using home_account_id as cache key this is what I am also thinking. Moreover, the original assertion for OBO will expire very 1h, so any cache entries that contain it will never be seen. This is not ok for token caches that do not support Time To Live configurations (SQL? ).

MSAL stores the assertion hash along with each access token. When you call OBO with assertion hash A1, we load all the access tokens and filter down only to the ones associated with A1. OBO is one of those few methods that looks in the cache by itself. You do not need to call AcquireTokenSilent, in fact this is not even recommended.

MSAL always knows how to query the cache - even if you give it a cache with 10,000 tokens. Access tokens are keyed by: home_account_id, environment, client_id, tenant_id and scopes and assertion hash. So the problem is only suggesting a distributed cache key that balances the load well.

Cache value <100KB is more than reasonable assumption. A token cache with 1 AT, 1 RT, 1 IdT and 1 Account (with few scopes) is about 7-8 KB (the AT entry is about 4KB). This is based on char counts, and 1 char != 1 byte, but we encode in UTF-8 and generally 1 char = 1 byte.

[bgavrilMS]

By the way @RamjotSingh - we are trying to add a few operational data points to MSAL, such as an enum that tells you where the token comes from (cache or AAD). Do you have suggestion for other data points?

[RamjotSingh]

@bgavrilMS Might be worth exposing information from AAD which is useless for request tracking. MSAL allows setting correlationId headers, but if I remember correctly EvoSTS sends back a bunch of headers which are useless when contacting AAD support.

Other data point might be time taken to acquire token, since one call to MSAL can actually mean multiple calls to underlying service (config discovery etc) might be worth adding these data points.

From token cache perspective, TokenCacheNotificationArgs should include TenantId, ResourceId.

[jmprieur]

Thanks @bgavrilMS and @RamjotSingh for these explanations.

• Given that MSAL.NET will anyway do what is needed (get only the OBO token with the given hash key of the incoming token from the cache once deserialized, whatever partition is realized by the serialization), it's ok to have the homeAccountId + something as the suggested cache key as well for OBO. This would also help @TiagoBrenck in his scenario;
• I don't think that there is FOCI for confidential client applications, and therefore using the clientID would not break it. Alternatively, @RamjotSingh you could have your storage (assuming you've been using a IDistributed cache of something). indexed by the ClientID outside the cache serialization methods. Would that be an option for you? I'm ok with adding the clientID. I just want to make sure that this won't prevent future scenario.

Conclusion: proposing to go ahead with:

• App context - ClientID+TenantId+_AppTokenCache
• User Context - ClientId + HomeAccountIdentifier (i.e. TenantID+UserId)

@jennyf19 do you see any objections?

@bgavrilMS : would it make sense to also expose TenantId and ResourceId in the args? I would suggest raising another issue for this, @RamjotSingh.

[jmprieur]

@RamjotSingh Given our investigation, I don't think that changing the SugestedCacheKey is the problem, at least for the user token cache. Do we want to take the App context change? Is there value ?

for the user context, recommending to keep what we have. I raised another issue for the performance issue you discovered (and the fact that the cache accessors are called twice): https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/1979

[RamjotSingh]

@jmprieur Yes their is still value to app context change. Because regardless of the other cache issues, putting all tokens in a single bucket for app context will have issues no matter what we do. For high traffic services they will be storing and pulling 1000s of token atleast. Which in itself is super bad, MSAL client caching issue or no.

[bgavrilMS]

@jmprieur - question around app context key, where you propose to use the tenant ID as part of the key.

It is possible (although highly not recommended) to start off with the common authority for client creds. So before making the request, the cache key is "clientID_common_AppTokenCache"

After the request comes back from the server, we need to save the token somewhere. But now we have the the actual tenant id, which we cannot use however because the request tenant ID is wrong.

My proposal is to always use the request tenant ID, as that is consistent and should not be "common"

[bgavrilMS]

As discussed with @jmprieur , decided not to change the cache keys.

[bgavrilMS]

We added tenant ID to suggedsted cache keys.