[Bug] Race conditions when CCA is singleton cause cache misses

[spongessuck]

Which version of MSAL.NET are you using?
Microsoft.Identity.Client 4.42.0
Microsoft.Identity.Client.Extensions.Msal 2.16.5
Microsoft.Identity.Web.TokenCache 1.23.1

Overview

1. Use a confidential client app in a web site scenario (S2S should repro this too). The app must be a singleton
2. Configure some distributed cache (can be MemoryDistributedCache, but I recommend a file based DistributedCache)
3. Make many requests in parallel

Actual: race conditions occur which make 2 accounts land in a single cache node. This causes cache misses later.

Possible cause: cache accessor objects are set per application, and not per request. Multiple requests share the same accessor.

Workaround: create the application object for each request.

Repro
We have a .NET Framework MVC app using hybrid flow with OWIN to authenticate users and then retrieve access tokens via authorization code/refresh token. The tokens are fetched for each of 3 downstream APIs with separate app registrations in Azure. The app used to use ADAL, and at that time it had a naive token cache that was serialized and written to the auth cookie, but after adding a third API endpoint the cookie would end up too large and cause a 400 error, so we moved to MSAL and a distributed token cache using Azure Cache for Redis with the default L1/L2 setup. The tokens are fetched with a ConfidentialClientApplication that is singleton-scoped. We have no session state provider; "state" is light, and values are stored as claims in the identity and persist in the auth cookie.

We've had persistent issues with exceptions related to refresh tokens not being found in the token cache. Because of this, the app is set up to ensure a token for each of these APIs is able to be acquired silently at the beginning of each (MVC) request, and if it is not (via caught MsalUiRequiredExceptions), the user is redirected through the MS auth endpoint to get a new auth code and attempt to fetch and re-cache new tokens that can then be acquired silently. We've also implemented an eager refresh to check the expiration of the token and force refresh to get a new one if it will expire in less than 30 minutes. Even with these measures in place, frequent MsalUiRequiredExceptions persist and, rarely, more pernicious issues, like auth loops and "cannot log you in" pages if the tokens are repeatedly unable to be acquired silently.

One trend that we've seen is that the issues seem to be exacerbated by successive cache hits in a short duration. A theory I've had is that somehow there's contention while writing to the cache if, say, 2 requests happen at the same time for the same user, the token cannot be acquired silently, there are 2 calls for a new auth code, and a new refresh token is acquired for both, but maybe the first is invalidated because the second was provided and the first ends up in the cache. I'm grasping at straws, as I'm not sure how the underlying cache sequence works, if after the token fetched in L1 is tried and fails if it looks in L2 or goes straight to throwing an MsalUiRequiredException, if when a new token is cached if it's written to L1 first and then pushed to Redis, or what.

Another thing we've discovered is that we're seeing users' tokens cached under a key that doesn't match the user's account ID. This again leads me to think that there's some kind of thread-unsafe behavior with the cache, rescoping the CCA in DI to per-request and transient did not help prevent this from happening.

Expected behavior
A valid refresh token is always available in the token cache.
An MsalUiRequiredException is almost never thrown after a user has signed into the app.

Actual behavior
Frequent MsalUiRequiredExceptions.
Rare authentication loops and "Could not sign you in" pages after tokens for downstream APIs are repeatedly unable to be acquired silently even after auth code redemption.

[spongessuck]

Here's a snippet from the OWIN middleware that ensures tokens for each downstream API can be fetched silently:

        public override async Task Invoke(IOwinContext context)
        {
            var request = new HttpRequestWrapper(HttpContext.Current.Request);
            var claimsPrincipal = context.Authentication.User;
            
            if (request.HttpMethod == "GET"
                && !request.IsAjaxRequest()
                && HttpContext.Current.CurrentHandler is MvcHandler handler
                && !controllersToAvoidChecking.Contains(handler.RequestContext.RouteData.Values["controller"].ToString())
                // Object ID claim will be null until auth'd
                && claimsPrincipal?.GetObjectId() != null
                && !await this.CanAcquireTokenSilent(claimsPrincipal.GetMsalAccountId()))
            {
                context.Authentication.Challenge(OpenIdConnectAuthenticationDefaults.AuthenticationType);
                return;
            }

            await Next.Invoke(context);
        }

        public virtual async Task<bool> CanAcquireTokenSilent(string msalAccountId)
        {
            var account = await clientApp.GetAccountAsync(msalAccountId);
            if (account == null)
                return false;

            try
            {
                foreach (var resource in this.config.Resources)
                {
                    await clientApp.AcquireTokenSilent(new[] { $"{resource.ClientId}/.default" }, account).ExecuteAsync();
                }
            }
            catch (MsalException ex)
            {
                // Ensure auth challenge
                return false;
            }

            return true;
        }

[bgavrilMS]

Is it possible to enabled verbose logging and capture a trace for when MsalUiRequiredException happens?

Tokens are cached with key home_tenant_id.home_obj_id. For normal users (i.e, not guest users), claimsPrincipal.GetMsalAccountId(), which is really tid.oid matches. But for guest users, this no longer holds and results in cache miss. Our ASP.NET classic sample does not handle guest users.

I believe @jmprieur is looking at adding OWIN support to Microsoft.Identity.Web, might be worth following that.

[bgavrilMS]

Also, you should not need Microsoft.Identity.Client.Extensions.Msal 2.16.5, this is for desktop apps.

[spongessuck]

@bgavrilMS I've attached verbose logs.

I've also attached an example token cache value with 2 users' token info under a single user's key. This was while pointing IDistributedCache to a local redis instance.

All of our users are in our home tenant so there shouldn't be any guest users in our application, so I don't think that's the issue.

Thanks for the info about Microsoft.Identity.Client.Extensions.Msal. It's actually a dependency of another package, so I don't think it's an active part of our auth pipeline.

MSALLogs - redacted.txt
tokenCacheEntry.txt

[bgavrilMS]

How did you setup your cache? Are you using Microsoft.Identity.Web.TokenCache ? Or did you write your own - in which case can we see the code? Are you ever clearing the cache?

Are there any eviction policies on L1 / L2 ?

The cache access is not syncronized, i.e. if 2 requests for the same user happen at the same time, there's no guarantee for the outcome. Generally this is not a problem.

[spongessuck]

I am using Microsoft.Identity.Web.TokenCache, I left the defaults in terms of settings. Here is how it's set up:

        instance.RegisterSelf(() =>
        {
            var openIdConnectAuthConfig = oidcConfigGetter();

            var clientApp = ConfidentialClientApplicationBuilder
                .Create(openIdConnectAuthConfig.ClientId)
                .WithClientSecret(openIdConnectAuthConfig.ClientSecret)
                .WithRedirectUri(openIdConnectAuthConfig.RedirectUri)
                .WithAuthority(new Uri(openIdConnectAuthConfig.Authority))
                .WithLegacyCacheCompatibility(false)
                .Build();

#if !NETCOREAPP2_1_OR_GREATER
            clientApp.AddDistributedTokenCache(services =>
            {
                services.AddStackExchangeRedisCache(options =>
                {
                    options.Configuration = distributedCacheConfigGetter().ConnectionString;
                });
            });
#endif

            return clientApp;
        });

[spongessuck]

Is there a better way for me to configure the cache?

Would this issue make more sense in the microsoft-identity-web repo?

[jmprieur]

which flow are you using, @spongessuck ? would it be AcquireTokenOnBehalfOf?

[spongessuck]

Authorization code flow. It's a web app that requests tokens for downstream APIs.

[jmprieur]

Are you using Microsoft.Identity.Web? Is the cache missed only with guest accounts? @spongessuck?

[spongessuck]

I'm using Microsoft.Identity.Web.TokenCache but not the main package, as it's a .NET Framework app. My auth pipeline is done via OWIN.

…

On Mon, Apr 25, 2022, 14:08 Jean-Marc Prieur ***@***.***> wrote: Are you using Microsoft.Identity.Web? — Reply to this email directly, view it on GitHub <#3285 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABQTITWDN6DHJX3UUYJW7HLVG3NRNANCNFSM5TQ5YAYQ> . You are receiving this because you were mentioned.Message ID: <AzureAD/microsoft-authentication-library-for-dotnet/issues/3285/1108882033 @github.com>

[spongessuck]

@jmprieur there are no guest accounts, this is a single-tenant app and it's all internal users.

[Mek7]

Hi,
we have the same problem - we use Redis cache to cache tokens, and our Redis records have different keys but the value is the same - there are tokens of all users of all AD organizations that ever logged in to our web application. We use the authorization code flow and Microsoft.Identity.Client library (so, not the Microsoft.Identity.Web.TokenCache)
and this is our code:

            var app = ConfidentialClientApplicationBuilder
                .Create(ConfigurationSettings.ActiveDirectoryClientId)
                .WithClientSecret(ConfigurationSettings.ActiveDirectoryClientSecret)
                .WithTenantId(ConfigurationSettings.AzureSubscriptionId)
                .WithRedirectUri(redirectUri())
                .Build();

            // use Redis cache for token storage, this is needed to support instance scaling in Azure (by default tokens are stored only in memory)
            app.UserTokenCache.SetBeforeAccess(args =>
            {
                // read data from cache
                var data = CacheFactory.CreateCacheProvider().GetObject<byte[]>(args.SuggestedCacheKey);
                args.TokenCache.DeserializeMsalV3(data);
            });

            app.UserTokenCache.SetAfterAccess(args =>
            {
                // write data to cache with 3 months expiration
                if (args.HasStateChanged)
                {
                    var data = args.TokenCache.SerializeMsalV3();
                    if (data == null || !args.HasTokens)
                    {
                        CacheFactory.CreateCacheProvider().Invalidate(args.SuggestedCacheKey);
                    }
                    else
                    {
                        CacheFactory.CreateCacheProvider().AddOrUpdate(args.SuggestedCacheKey, data, a => data, expirationMode: ExpirationMode.Absolute, expireAfter: TimeSpan.FromDays(30 * 3));
                    }
                }
            });

            return app;

Currently, one record in Redis cache has around 5 MB, and this record is repeated under different cache keys (obj_id.tenant_id) for every user that logs in using AD. Our Redis cache quickly filled up and reading these records from Redis slows down our app.
It seems that args.TokenCache always contains all records, not just the one of the user that is being logged in. But args.SuggestedCacheKey is two guids connected with dot (obj_id.tenant_id)
What is interesting - our application is hosted in 3 cloud services (with extended support) on Azure, where 2 of the cloud services exhibit this problem, and 1 saves records into Redis correctly - one record per user, as we would expect. But the code is the same. We can't find any differences of AD app registrations in Azure configuration. App registrations are different for these 3 cloud services, but there is only one AD tenant (organization).
Any ideas?