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

[FEATURE REQ]Azure.Identity should handle token caching and refreshing when calling .GetTokenAsync just like AppAuthentication does #20011

Closed
felinepc opened this issue Mar 31, 2021 · 7 comments
Assignees
Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. feature-request This issue requires a new behavior in the product in order be resolved.
Milestone

Comments

@felinepc
Copy link

felinepc commented Mar 31, 2021

I was reading this documentation and getting ready to migrate from AppAuthentication to Azure.Identity.

While the new library surely works amazingly well with the new Azure client SDKs, I soon found out that the method to directly obtain access token, TokenCredential.GetTokenAsync, has the following warning:

// This method is called automatically by Azure SDK client libraries. You may call
// this method directly, but you must also handle token caching and token refreshing.

There are plenty of use cases where a managed identity access token needs to be explicitly retrieved. For example, using Entity Framework Core with either Azure SQL or Azure PostgreSQL with managed identity requires that the actual tokens to be passed to the EF provider. Same with authenticating against a custom Azure AD enterprise application via standard HttpClient.

These scenarios are unlikely to get native integration with Azure.Identity anytime soon.

The older library's AzureServiceTokenProvider.GetAccessTokenAsync has built-in token caching and refreshing which works fairly well. It would be great if Azure.Identity can implement the same. Only then it will become a viable full replacement for AppAuthentication.

In the meantime, I believe this difference should be noted in the migration documentation under the "Access token retrieval" section.

@ghost ghost added needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Mar 31, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Apr 1, 2021
@Mohit-Chakraborty Mohit-Chakraborty added the Client This issue points to a problem in the data-plane of the library. label Apr 1, 2021
@ghost ghost added the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Apr 1, 2021
@Mohit-Chakraborty
Copy link
Contributor

Thank you for your feedback. Tagging and routing to the team best able to assist.

@christothes christothes added feature-request This issue requires a new behavior in the product in order be resolved. and removed needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team labels Apr 6, 2021
@ghost ghost added the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Apr 6, 2021
@christothes christothes removed the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Apr 6, 2021
@leidegre
Copy link

leidegre commented Apr 8, 2021

Uhm. There's caching but it's slow. The SharedTokenCacheCredential adds 10 milliseconds. That's a lot when you target is sub 100 milliseconds per request.

image

I wrote this simple in-memory cache which takes less than 10 microseconds as long as the token is valid.

The problem though is that the token I get back (from the SharedTokenCacheCredential) has a validity that I have no control over.

So if the token ExpiresOn goes outside of the tolerance there's no guarantee that I'll get a fresh token back now. More precisely. I cannot ensure that I get a token that has at least 15 minutes of validity at the moment of request.

I currently have these two issues:

  • The cache is crazy slow, it adds 10 milliseconds for each request.
  • I cannot stipulate a minimum access token validity period when I ask for the token.

I need this because I have to set the access token on the SqlConnection instance when the DataContext is created, there isn't a pull based API that can be used to pull in an access token when needed by the DataContext. I can ensure that all "unit of work" patterns that we have complete within 10 minutes but I cannot currently ensure that I get back a token which has at least 10 minutes of validity from the SharedTokenCacheCredential (which is included by default in the DefaultAzureCredential).

I need a way to express that the token I get back has the maximum allowed life-time of so that I can have a fast in-memory cache with some tolerance. And yes, this is because there's no way for the Entity Framework stuff to pull in the access token when needed, we have to put it in place before hand.

And if the DataContext linger for more than a couple of minutes (which can happen in background jobs) we'll get a connection error because the access token will have expired.

Also, I cannot find any documentation on what you might expect on the expiry. If the expiry is with some tolerance already. If so, that would be great but where is this documented!?

Here's my simple in-memory cache. It cannot be used with a tolerance, i.e. .AddMinutes(15) remove this part but it does speed things up when the token you got will do.

class TokenRequestContextEqualityComparer : IEqualityComparer<TokenRequestContext>
{
    static readonly string[] Empty = new string[] { };

    public bool Equals(TokenRequestContext x, TokenRequestContext y)
    {
        var a = x.Scopes ?? Empty;
        var b = y.Scopes ?? Empty;

        if (a.Length != b.Length)
        {
            return false;
        }
        for (int i = 0; i < a.Length; i++)
        {
            if (a[i] != b[i])
            {
                return false;
            }
        }
        return true;
    }

    public int GetHashCode(TokenRequestContext obj)
    {
        var scopes = obj.Scopes ?? Empty;

        int hashCode = 0;
        for (int i = 0; i < scopes.Length; i++)
        {
            hashCode ^= scopes[i].GetHashCode();
        }
        return hashCode;
    }
}

class CachedAzureCredential : DefaultAzureCredential
{
    private readonly ConcurrentDictionary<TokenRequestContext, AccessToken> _cache = new ConcurrentDictionary<TokenRequestContext, AccessToken>(new TokenRequestContextEqualityComparer());

    public override AccessToken GetToken(TokenRequestContext requestContext, CancellationToken cancellationToken = default)
    {
        AccessToken cached;
        if (_cache.TryGetValue(requestContext, out cached) && DateTimeOffset.UtcNow.AddMinutes(15) < cached.ExpiresOn)
        {
            return cached;
        }
        var accessToken = base.GetToken(requestContext, cancellationToken);
        _cache[requestContext] = accessToken;
        return accessToken;
    }

    public override async ValueTask<AccessToken> GetTokenAsync(TokenRequestContext requestContext, CancellationToken cancellationToken = default)
    {
        AccessToken cached;
        if (_cache.TryGetValue(requestContext, out cached) && DateTimeOffset.UtcNow.AddMinutes(15) < cached.ExpiresOn)
        {
            return cached;
        }
        var accessToken = await base.GetTokenAsync(requestContext, cancellationToken);
        _cache[requestContext] = accessToken;
        return accessToken;
    }
}

@felinepc
Copy link
Author

felinepc commented Apr 8, 2021

@leidegre For EF, DbContext is not the only way to inject the token. It can be done via DbConnectionInterceptor, which is the better, non-blocking way. See this (this example actually uses Azure.Identity) and this (this example in romar's answer uses AppAuthentication).

My issue is with the fact that the old AppAuthentication package takes care of refreshing and caching internally on the GetAccessTokenAsync call so that anytime we use it, we are guaranteed to get a fresh token without worrying about any caching/refreshing logic or whatsoever. This is true no matter if it's in local dev (using VS or Azure CLI connection string) or in production (using managed identity connection string). It'd be nice if we can simply have the exact same implementation with Azure.Identity.

I have not used SharedTokenCacheCredential for Azure.Identity, my understanding is that it's for locally shared/cached credentials among MS apps. I use Azure.Identitity for web apps only and I generally use something like new ChainedTokenCredential(new ManagedIdentityCredential(), new AzureCliCredential())) and according to the doc, when I call GetTokenAsync on this I must implement caching/refreshing myself.

@leidegre
Copy link

leidegre commented Apr 8, 2021

I'm currently not on .NET Core, is there anything similar to the DbConnectionInterceptor in EF6?

I don't use refresh tokens but the code I've provided will help with the token acquisition.

@felinepc
Copy link
Author

felinepc commented Apr 8, 2021

@leidegre Sorry I started with EF Core and never used EF6, but it does appear to have IDbConnectionInterceptor? See this and this.

Yeah I know a simple in-memory cache and refresh logic wouldn't take more than a few lines of code, but I just felt that the new package should have this built-in because the old one had it, and I generally trust MS's internal implementation on this sort of things more than my own.

@jsquire jsquire added this to the Backlog milestone Apr 12, 2021
@jsquire jsquire removed the question The issue doesn't require a change to the product in order to be resolved. Most issues start as that label Apr 26, 2021
@felinepc
Copy link
Author

felinepc commented Oct 3, 2021

Dear awesome Microsoft engineers, by any chance we can give this a higher priority? I just did a search for "cache" on this repo and saw a long list of requests asking more or less the same thing:
cache_request

Identity is a huge issue and we do NOT want to stick to the old library. Azure.Identity is not going to create libraries anytime soon for things like Azure SQL/Azure PostgreSQL/Azure App Service Microsoft Authentication, so we need to access this access token for the time being.

The old Azure.Services.AppAuthentication package works very reliably for this and has all the beautiful Microsoft-written caching logic built in. But as we all know since the old package is EOL, we'd really love to move to Azure.Identity, as Microsoft strongly suggested themselves. But we really do need this key feature implemented.

@christothes
Copy link
Member

duplicate of #25361

azure-sdk pushed a commit to azure-sdk/azure-sdk-for-net that referenced this issue Jul 29, 2022
Update network readme.python.md (Azure#20011)

* Update network readme.python.md

* Update readme.python.md

Co-authored-by: Zhenbiao Wei (WICRESOFT NORTH AMERICA LTD) <[email protected]>
Co-authored-by: Yuchao Yan <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. feature-request This issue requires a new behavior in the product in order be resolved.
Projects
None yet
Development

No branches or pull requests

6 participants