[Feature Request] By default confidential client applications should have a cache that behaves better and encourages using serialization adapters

[jmprieur]

Is your feature request related to a problem? Please describe.

In Confidential client applications, our guidance is for developers to partition the token caches using the suggested cache key (we tend to say one cache per user, at least for the user token cache).

However, the default cache provided my MSAL.NET is just a flat list of accounts therefore:

• causing security risks: if in a web app, the confidential client application does `(await GetAccountsAsync()).FirstOrDefault() instead of getting the account for the signed-in user from the tid.iod, the tokens can be a wrong token and this can lead to an elevation of privilege if the first account is an admin one ...
• bringing bad performance, as MSAL will have to look for accounts / tokens in potentially big flat list of accounts/tokens.

Microsoft.Identity.Web provides a good solution, but it's not discoverable when customers are just using MSAL.NET

Describe the solution you'd like

In the description below, we name "incoming token", a token that was passed-in to call a web API, and validated by the web API.

• By default, when instantiating a confidential client application, the cache should behave well. For instance, we could plug-in a default token cache serializer that would be based on a Dictionary<string, byte[]>, which would be a dictionary of caches per cache key. This works well in Microsoft.Identity.Web already (See MsalAbstractTokenCacheProvider and MsalMemoryTokenCacheProvider) , but:

  • customers don't necessarily discover the Microsoft.Identity.Web token cache serializers (even if they now work with .NET Framework in addition to .NET Core)
  • MSAL.NET cannot take a dependency on MemoryCache and IDistributedCache, so we would not bring them to the MSAL.ENT Code anyway.

  This solution would solve the security issue and partially the performance issue (as the cache key would be right). It would not solve the fact that the cache can be big in memory

• Log errors (but do not throw) in the log if the dictionary has more than 200 keys. This error should advise customers to use a token cache serialization, and provide an actionable link:

• Log errors (but do not throw) in the log if the dictionary the time it takes to get a token from the cache takes more than x ms. This error should advise customers to use a token cache serialization, and provide an actionable link:

  The default token cache provided by MSAL is not designed to be performant. Please use token cache serialization. See https://aka.ms/msal-net-cca-token-cache-serialization.

Describe alternatives you've considered

• Send errors in logs and (MATS) telemetry if they don’t subscribe to cache events.
• Or Send errors in logs and server side telemetry if they don’t and there are more than 100 tokens in the cache.
• Throw a meaningful exception when there is a cache IO operation and customers have not overridden the cache events. This is to help customers use the right pattern (aka.ms link to use Ms.Id.Web). But this would be a breaking change.

Additional context

When doing the right portioning of the cache by suggested cache key, the following will happen:

• GetAccountsAsync() will now return an empty collection, as there won't be any longer a flat list of all the accounts. This method should be deprecated. We propose to have an obsolete attribute with an aka.ms link

  GetAccountsAsync() should not be used in confidential client application. Use GetAccountAsync with the identifier of the current user instead. For details, see https://aka.ms/msal-net-cca-token-cache-serialization.

  This will de facto account for [Enhancement] GetAccountsAsync should no longer be exposed in confidential client applications (via deprecation) #1967 which should be processed at the same time as this issue

• In web APIs, AcquireTokenSilent will always throw an MsalUiRequiredException, because it will attempt to get a token for a user, whereas the OBO key is the hash of the token. We'd probably want to indicate to the developer that they should not use AcquireTokenSilent in a web API? maybe by logging an error (same aka.ms link as for the other case). This is an optimizaztion anyway as if customers use the proposed pattern they would end up calling OnBehalfOf, which does the cache lookup with the right key (the hash of the incoming token on the web API). This would account for [Enhancement] AcquireTokenSilent should not return token acquired by OBO #1966 that should be processed at the same time as this issue

See also #2447, which implies that the cache also needs to maintain, for OBO a map of user identifier, and the list of corresponding incoming token hashes.

Work done

• Create the aka.ms link: https://aka.ms/msal-net-cca-token-cache-serialization
• Add a WithCacheSynchronization(bool) that defaults to true on CCA. If set to false, the CacheSemaphore does nothing
• Compare the performance of the current cache and the new dictionary based partition (to have the discussion with partners) - [x] Obsolete attribute on new method GetAccountsAsync on IConfidentialClientApplication (overriding the GetAccountsAsync which is on BaseApplication)
• Update docs.ms to remove the sentence advising users to use AcquireTokenSilent in web APIs. Done PR #148832

Work to be done

• Add a simple mechanism for users to be able to hook up their own cache, smth like WithCache(ITokenCache) which binds both app and user token cache. CCA only (?)
• Add an Adapter which implements ITokenCache and uses partitioning.
• Add an L1 / L2 cache in MSAL, where L1 is in memory and L2 is distributed. Similar to what MIW does. Both should be configurable and rely on our own interfaces. Needs design
• Add the ability to use an in memory static partitioned cache.

[ ] Default in memory cache serializer based on Wilson LRU cache for ALL cca flows.
[ ] LRU cache to utilize the SuggestedExpiration that is available on the NotificationArgs to optimize eviction
[ ] LRU cache to default to 4GB of space

• Roslyn analyzer that detects that the cache is not customized

[bgavrilMS]

Some of the work has been done. Here's what I think is left:

• Fail (exception) if token cache contains tokens for multiple users for OBO and AcquireTokenByAuth code. It is insecure and we should not even encourage usage of a dictionary cache as it does not scale (?). Can we do this in MSAL4?
• Remove cache locks from confidential client. This should be ok if cache is partitioned.
• Add a convenience method similar to Microosft.Identity.Web cache adaptor to encourage caching.

ccaBuilder.WithTokenCaching(cacheProvider)

Thoughts @jmprieur ?

[jmprieur]

• Let's implement a partitioned cache (Wilson's LRU cache) on all confidential client applications flows.
• remove the semaphores by default? Settings in ConfidentialClientApplicationOptions ?
• Roslyn analyzer

[henrik-me]

Some of the work has been done. Here's what I think is left:

• Fail (exception) if token cache contains tokens for multiple users for OBO and AcquireTokenByAuth code. It is insecure and we should not even encourage usage of a dictionary cache as it does not scale (?). Can we do this in MSAL4?
• Remove cache locks from confidential client. This should be ok if cache is partitioned.
• Add a convenience method similar to Microosft.Identity.Web cache adaptor to encourage caching.

@bgavrilMS
Seems like the work items should be updated to what has been done and what is remaining? Having these items inline will not likely be seen, especially not if going back later to explore what we wanted to do.

[pmaytak]

Add the ability to use an in memory static partitioned cache.

@bgavrilMS In what scenarios would a static cache be needed that reusing a CCA instance doesn't cover?

[jmprieur]

Closing as a duplicate it's not processed by:

• [Feature Request] In memory token cache in confidential client application is well partitioned for all flows, non longer serialized and can be shared between instances of confidential client apps #2861
• [Feature Request] In-memory shared cache cache in MSAL (like ADAL), with good performance #2849