azure-sdk-for-net icon indicating copy to clipboard operation
azure-sdk-for-net copied to clipboard

Implement consistent Token Caching support that builds on MSAL cache

Open christothes opened this issue 2 years ago • 23 comments

MSAL's cache implements some fairly complex logic that we do not ever intend to duplicate in Azure.Identity.

Ideally we would utilize the underlying MSAL library's cache to offer a consistent caching experience. MSAL currently it does not expose its cache directly. This may be something that becomes available in the future as a feature enhancement.

christothes avatar Nov 16 '21 23:11 christothes

This is a key feature and without it, we cannot comfortably migrate from AppAuthentication to Azure.Identity for Azure apps that are not natively integrated with the latter SDK.

This includes using managed identity with things like various Azure PaaS database services (Azure SQL, Azure PostgreSQL) or authenticating against Azure AD protected web apps/function apps etc.

Hopefully it will be implemented in Azure.Identity soon and then this library can truly and totally replace the legacy one.

mikequ-taggysoft avatar Nov 17 '21 16:11 mikequ-taggysoft

Whether you roll your own cache or borrow from BearerTokenAuthenticationPolicy in some way there is a problem with freely choosing the tokenRefreshOffset. BearerTokenAuthenticationPolicy has a default of 5m which happens to be the same as what some of the the underlying TokenCredential variants uses. Those with MSAL i guess, but yes, that code was complex enough that I could not find where its equivalent to tokenRefreshOffset was set or what it was set to, I only observed the behaviour...

If you choose something longer than 5m (like I happened to do with 10m) you end up with the code trying to refresh the token but getting the same token back from the TokenCredential so for the difference of the duration your cache is suddenly "read-through" since the newly fetched token has the same Expiry time as the old. With the logic in BearerTokenAuthenticationPolicy this does not affect the caller with a performance hit, but some additional CPU will be used and if you have AppInsights active you will get a lot of unexpected logging from the TokenCredential code.

Can someone add some advice on how to best choose the tokenRefreshOffset or should we just assume 5m and hope for the best in the future?

cadwal avatar Nov 19 '21 16:11 cadwal

Requesting a token more frequently from AAD does not guarantee that a new token will be received since it does its own caching as well. The only benefit that BearerTokenAuthenticationPolicy attempts to provide is to reduce the chance that a network call is required in the hot path of TokenCredential.GetToken.

christothes avatar Nov 19 '21 16:11 christothes

I would actually be very surprised if two requests for tokens from AAD returned the same token unless I hammered it hard with requests and hit an output cache or something like that. More likely to get a 429 response in that case would be my guess. For the kind of access token this concerns and for requests 50m apart it seems very unlikely. But of course anything is possible.

cadwal avatar Nov 19 '21 19:11 cadwal

@cadwal In that case be very surprised. Managed Identity returns the same token for up to 24 hours, which has been a real pain when making changes to roles as they don't take effect until a new token is generated.

MatthewSteeples avatar Nov 19 '21 19:11 MatthewSteeples

No, that is not really a surprise.

I assume you are talking about AppServices, Functions or AKS Linux Pods here and to aquire a token for the MI:s involved there, the library you use have to (the last time I looked at least) do a network call to the sidecar that has access to the MI and its secret (I don't think the MSI_SECRET env variable is the actual MI secret at least) and can do the actual AAD call to aquire the token. There might be more levels of libraries involved in the sidecar that I don't know about since that code is not exactly public as far as I know. All that code might do whatever it wants in the way of caching causing that behaviour, but it also means that those MI:s have a different token policy with a longer lifetime than the usual 1h, wonder why that was done and if one can change that... Might there also be a difference between System Assigned and User Assigned?

Though I also wonder if the innermost layer there uses MSAL and has that same issue with that cache not allowing for new tokens until 5m before expiry. Using Windows AKS Pods here that don't have Pod Indentities yet so have not come across more details on that...

But essentially I guess then there is no known better choice than 5m as a RefreshOffset for any top level cache as discussed here.

cadwal avatar Nov 19 '21 21:11 cadwal

@christothes, I think this one is also related to: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/2597

Meertman avatar Nov 23 '21 15:11 Meertman

The lack of this feature makes a migration away from AppAuthentication unreasonable for at least SQL Server Authentication use cases.

The migration below causes 3 calls to the token endpoint, to check for migrations and apply them if needed and each call is 10 seconds right now (I'm not sure why it's taking so long though).

image

Legacy

var tokenProvider = new AzureServiceTokenProvider();
return await tokenProvider.GetAccessTokenAsync(ResourceId);

New

var context = new TokenRequestContext(new[] { ResourceId + "/.default" });
var credential = new DefaultAzureCredential();
var accessToken = await credential.GetTokenAsync(context);
return accessToken.Token;

eluchsinger avatar Feb 14 '22 10:02 eluchsinger

The migration below causes 3 calls to the token endpoint, to check for migrations and apply them if needed and each call is 10 seconds right now (I'm not sure why it's taking so long though).

The delay you are experiencing is a known issue unrelated to caching - see #24767

christothes avatar Feb 14 '22 15:02 christothes

@christothes it did sound like it was unrelated to caching. Still, the migration is not an option for an app that should scale. Unless someone wants to build a custom caching mechanism.

eluchsinger avatar Feb 14 '22 16:02 eluchsinger

Thanks for the feedback. We're also working on a caching strategy for ManagedIdentityCredential involving improved support from MSAL. I don't have an exact timeframe to share, but it is on the radar for sure.

christothes avatar Feb 14 '22 16:02 christothes

This missing cache also prevents us from fully migrating away from AppAuthentication.

I'd love to also have an option in the cache that automatically refreshes the tokens in the background before they expire, so that no actual user-facing request ever faces the penalty of waiting for the token response.

We're currently doing this with AppAuthentication where a IHostedService just calls GetAccessTokenAsync(resource, forceRefresh: true) on each dependant AAD resource every 45 min. This helped a lot with making our response times more consistent.

cwe1ss avatar Mar 17 '22 19:03 cwe1ss

@eluchsinger

The lack of this feature makes a migration away from AppAuthentication unreasonable for at least SQL Server Authentication use cases.

The migration below causes 3 calls to the token endpoint, to check for migrations and apply them if needed and each call is 10 seconds right now (I'm not sure why it's taking so long though).

For SQL Server authentication you don't need to get the token by yourself anymore, but use the new Active Directory Default authentication provider, which can you specify in the connection string like this Authentication=Active Directory Default;

Rookian avatar Jul 01 '22 11:07 Rookian

If anyone lands here and needs a band-aid solution you can use this to wrap any TokenCredential.

using System.Text.Json;
using Azure.Core;
using Microsoft.Extensions.Caching.Memory;

public class CachedTokenCredential : TokenCredential {
    private readonly IMemoryCache cache;
    private readonly TokenCredential credential;

    public CachedTokenCredential(IMemoryCache cache, TokenCredential credential) {
        this.cache = cache;
        this.credential = credential;
    }

    public override AccessToken GetToken(TokenRequestContext requestContext, CancellationToken cancellationToken) {
        if (!this.cache.TryGetValue(GetCacheKey(requestContext), out string token)) {
            AccessToken tokenModel = this.credential.GetToken(
                requestContext,
                cancellationToken
            );

            MemoryCacheEntryOptions? options = new MemoryCacheEntryOptions().SetAbsoluteExpiration(
                tokenModel.ExpiresOn.AddMinutes(-1)
            );

            token = JsonSerializer.Serialize(tokenModel);

            this.cache.Set(GetCacheKey(requestContext), token, options);
        }

        return JsonSerializer.Deserialize<AccessToken>(token);
    }

    public override async ValueTask<AccessToken> GetTokenAsync(TokenRequestContext requestContext, CancellationToken cancellationToken) {
        if (!this.cache.TryGetValue(GetCacheKey(requestContext), out string token)) {
            AccessToken tokenModel = await this.credential.GetTokenAsync(
                requestContext,
                cancellationToken
            );

            MemoryCacheEntryOptions? options = new MemoryCacheEntryOptions().SetAbsoluteExpiration(
                tokenModel.ExpiresOn.AddMinutes(-1)
            );

            token = JsonSerializer.Serialize(tokenModel);

            this.cache.Set(GetCacheKey(requestContext), token, options);
        }

        return JsonSerializer.Deserialize<AccessToken>(token);
    }

    private static string GetCacheKey(TokenRequestContext tokenRequestContext) =>
        $"{tokenRequestContext.TenantId}{string.Join('-', tokenRequestContext.Scopes)}";
}

Tiberriver256 avatar Aug 16 '22 13:08 Tiberriver256

Thanks for the feedback. We're also working on a caching strategy for ManagedIdentityCredential involving improved support from MSAL. I don't have an exact timeframe to share, but it is on the radar for sure.

It seems there's a lot of interest in Chris' comment, so I want to provide an update. The ManagedIdentityCredential token caching feature shipped in the 1.7.0-beta.1 release of Azure.Identity. We expect to GA it in October.

scottaddie avatar Aug 17 '22 15:08 scottaddie

It seems there's a lot of interest in Chris' comment, so I want to provide an update. The ManagedIdentityCredential token caching feature shipped in the 1.7.0-beta.1 release of Azure.Identity. We expect to GA it in October.

Hey @scottaddie, I see that the token caching feature was removed from the 1.7.0 release of Azure.Identity. Is it still planned to be GA'ed in October?

amirschw avatar Sep 22 '22 16:09 amirschw

@amirschw The 1.8.0 release will GA in October and will include token caching for ManagedIdentityCredential. We had to ship the 1.7.0 release sooner than expected to include this feature for multi-tenant apps. The 1.8.0-beta.1 release, which reintroduces token caching, is expected early next week.

scottaddie avatar Sep 23 '22 18:09 scottaddie

UPDATE: The 1.8.0 GA release has slipped to November.

Why? The Azure SDK team strives to achieve feature parity across the most popular programming languages we support. We have numerous customers who use the Azure Identity library across multiple languages. Consistency is a key ingredient in their developer experience. In this situation, our Azure Identity for JavaScript library's unit tests for Managed Identity token caching still need some work. And we don't want to rush a feature out the door that doesn't meet our quality bar. Thanks for your patience.

scottaddie avatar Sep 30 '22 02:09 scottaddie

Any plans to also support token caching with InteractiveBrowserCredential? Our scenario is console app that uses Azure SDK and we want to be able to have similar expereince to az cli where user logs in and can execute mutliple CLI commands without needing to re-authenticate. We could implement our own tokencredential that does aquiresilent and acquire interactive with MSAL but we don't want to take on the burden of securing/maintaining secure implementation of token cache.

rguthriemsft avatar Oct 10 '22 22:10 rguthriemsft

Any plans to also support token caching with InteractiveBrowserCredential? Our scenario is console app that uses Azure SDK and we want to be able to have similar expereince to az cli where user logs in and can execute mutliple CLI commands without needing to re-authenticate. We could implement our own tokencredential that does aquiresilent and acquire interactive with MSAL but we don't want to take on the burden of securing/maintaining secure implementation of token cache.

Does the following sample work for your scenario? https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/identity/Azure.Identity/samples/TokenCache.md

christothes avatar Oct 12 '22 15:10 christothes

UPDATE: The 1.8.0 GA release shipped today!

scottaddie avatar Nov 09 '22 19:11 scottaddie

@scottaddie the release notes don't say anything about Managed Identity token caching 🧐

amirschw avatar Nov 09 '22 19:11 amirschw

@amirschw It's mentioned in the changelog entry for the initial 1.8.0 Beta release: https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/identity/Azure.Identity/CHANGELOG.md#180-beta1-2022-10-13

scottaddie avatar Nov 09 '22 19:11 scottaddie

Could anyone explain how to use this feature with DefaultAzureCredential? Obviously, the DefaultAzureCredentialOptions does not implement ITokenCacheOptions

kaledkowski avatar Nov 18 '22 13:11 kaledkowski

Hi @kaledkowski - The feature is to enable caching where there was previously none. Managed Identity does not support the advanced cache features related to ITokenCacheOptions.

christothes avatar Nov 18 '22 16:11 christothes