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] Add Token Caching support for AzureCliCredential and other Azure TokenCredential where possible #32579

Closed
mikequ-taggysoft opened this issue Nov 19, 2022 · 12 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. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that

Comments

@mikequ-taggysoft
Copy link

mikequ-taggysoft commented Nov 19, 2022

Library name

Azure.Identity

Please describe the feature.

Azure.Identity release 1.8.0 finally added token cache for ManagedIdentityCredential, closing out feature request #25361 which has been long awaited by many. @christothes

But as I finally started to migrate from AppAuthentication to Azure.Identity, I realized that the implemented cache seemed to only apply to ManagedIdentityCredential, which is great for production in Azure, but not for local dev.

Many Azure developers who utilize managed identities also naturally utilize credentials such as AzureCliCrendential and VisualStudioCredential (I personally prefer the former as it seems to be faster and more reliable for me) for local dev environments.

Previously, AppAuthentication would just cache everything and the experience is completely seamless.

Now that AzureCliCrendential is not cached, every request for token results in a HTTP call. That makes the dev experience laggier and less pleasant.

The symptom is especially noticeable when using an Azure managed database product such as Azure PostgreSQL or Azure SQL. The connection strings to these services include the access token (such as via EF Core interceptors), and the token is requested on every single connection. Without the cache, every DB request results in a new HTTP token request.

Given that ManagedIdentityCredential has got the fancy token cache implementation (borrowed from MSAL I believe), I'd imagine it should not be much work to extend the feature to other Azure credentials such as AzureCliCredential, which will make Azure.Identity truly a flawless replacement for AppAuthentication.

@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 Nov 19, 2022
@azure-sdk azure-sdk added Azure.Identity Client This issue points to a problem in the data-plane of the library. needs-team-triage Workflow: This issue needs the team to triage. labels Nov 19, 2022
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Nov 19, 2022
@mikequ-taggysoft mikequ-taggysoft changed the title [FEATURE REQ] Add Token Caching support for AzureCliCredential and all other Azure TokenCredential if possible [FEATURE REQ] Add Token Caching support for AzureCliCredential and other Azure TokenCredential where possible Nov 19, 2022
@jsquire
Copy link
Member

jsquire commented Nov 19, 2022

//cc: @christothes

@jsquire jsquire added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-team-triage Workflow: This issue needs the team to triage. labels Nov 19, 2022
@jsquire
Copy link
Member

jsquire commented Nov 19, 2022

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

@lukeskinner
Copy link

lukeskinner commented Nov 28, 2022

I'm surprised this caching is not in place after using other auth libraries from Microsoft with it in.
I have similar issues where methods should take 100ms are taking 5 seconds because each entityframework context open triggers a new access token which is not cached at all. Using VisualStudioCredential.

At the moment I'm resorting to putting my own cache in place in front of this library.

Example setup: https://devblogs.microsoft.com/azure-sdk/azure-identity-with-sql-graph-ef/
In the EF Core integration section.

@christothes
Copy link
Member

Hi @lukeskinner - The process-based development time credentials, such as AzureCliCredential, VisualStudioCredential, and PowerShellCredential do not call into MSAL directly, unlike most other credential implementations. Because they invoke a process, we depend on their internal implementations for caching behavior. As with the ManagedIdentityCredential prior to it being supported in MSAL, we have no plans to implement a custom cache for these cases.

@christothes christothes added the needs-author-feedback Workflow: More information is needed from author to address the issue. label Nov 28, 2022
@ghost ghost removed the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Nov 28, 2022
@mikequ-taggysoft
Copy link
Author

mikequ-taggysoft commented Nov 28, 2022

@christothes Thanks for the clarification on the reasons behind the cache implementation discrepancy.

But if you consider that it was Microsoft's decision to ultimately switch us from AppAuthentication to Azure.Identity, and came up with the TokenCredential abstract class design that encompasses all kinds of credentials (vs. the old RunAs=App and RunAs=Developer constructor based AzureServiceTokenProvider), hopefully you'd see why the current situation is less than ideal.

Many of us have fully functional production apps out there using AppAuthentication, We requested a token, we got a token, and the service just worked. We didn't have to think about things like caching just to connect to Azure. The older library did its job perfectly as a vendor platform SDK. It was secure and convenient. We loved it.

When we were told to migrate to Azure.Identity, we immediately found out that it could not be a direct swap due to the cache issue. Hence #25361 was such a popular request.

We do know that a simple, basic cache wouldn't take more than a couple of dozens of lines of codes to write ourselves in our own codes. But we didn't want to mess with any edge cases along with things like retries and exception handlings etc, again, just to connect to Azure.

Now with #25361 being resolved, we can at least confidently go to production with Azure.Identity. But as @lukeskinner mentioned, the local dev experience is a pain without cache. Since dev operations are a lot less mission-critical, we'd indeed feel more comfortable adding some simple cache ourselves for now.

However, if you think about this setup: we now look at the environment, and if it's dev, we get/set token from this custom cache. Otherwise, we call GetTokenAsync directly. That just seems a bit silly to me.

Meanwhile, those who don't bother doing this would be just spamming the Microsoft/Azure AD token endpoints repeatedly during dev, for no good reasons.

Then if you look at the doc for the GetTokenAsync method on the TokenCredential abstract class, it still says:

Caching and management of the lifespan for the Access Token is considered the responsibility of the caller: each call should request a fresh token being requested.

Compare that to the new 1.8.0 version on ManagedIdentityCredential.GetTokenAsync:

Obtains an Access Token from the Managed Identity service, if available. Acquired tokens are cached by the credential instance. Token lifetime and refreshing is handled automatically. Where possible, reuse credential instances to optimize cache effectiveness.

Don't you think all these discrepancies can be confusing to new Azure developers who would be coming to the library?

So far, I have not even seen new tutorials/docs out there that address this issue. Imagine if you were to write a new "how-to" guide for token-based authentication to Azure PaaS services, do you really want to mention "cache"? If you don't though, the developers will be baffled on why their apps are so laggy when running locally till they realize that they're spamming the token endpoints with every query against Azure SQL.

I can see that the TokenCredential class now has like 20 derived classes underneath it, and can understand why you may not want to write a custom cache for each different implementation.

But maybe if you'd consider adding a "general purpose", in-class cache that can cover most of the use cases for these process-based credentials for development (don't have to be complicated because it's dev after all), similar to how AppAuthentication did it in the past?

@ghost ghost added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-author-feedback Workflow: More information is needed from author to address the issue. labels Nov 28, 2022
@PSanetra
Copy link

I agree with @mikequ-taggysoft. We have solved that issue with a custom TokenCredential implementation that is able to wrap any another TokenCredential implementation, but I would prefer to use a caching approach that is supported natively by the Azure.Identity SDK.

@christothes
Copy link
Member

Thanks for the detailed feedback @mikequ-taggysoft ! Although we still do not plan to implement a cache specific to Azure.Identity , we'll track this as a documentation issue here #32857

Just as a side note, The Azure SDK clients do utilize a token cache, but this scenario is much simpler because the token will exist for only a specific identity and scope. Arbitrary token caching is very complex to do correctly, which is why we delegate this to MSAL.

@christothes christothes added the needs-author-feedback Workflow: More information is needed from author to address the issue. label Dec 2, 2022
@ghost ghost removed the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Dec 2, 2022
@PSanetra
Copy link

PSanetra commented Dec 4, 2022

@christothes can you give examples for what is to consider on arbitrary token caching? There are use cases where the token is not used by an Azure SDK client and therefore there is also no token cache. E.g. when you want to use the token for a PostgreSQL connection: https://learn.microsoft.com/en-us/azure/postgresql/single-server/how-to-connect-with-managed-identity#connecting-using-managed-identity-in-c

@mikequ-taggysoft
Copy link
Author

mikequ-taggysoft commented Dec 5, 2022

@PSanetra Yeah the fundamental issue with both #25361 and this one is the fact that no dedicated Azure SDKs (which do have caching) exist for or apply to certain Azure services (such as the PaaS database ones).

AppAuthentication essentially used to fill in the gap for those services – getting access tokens, with caching and lifetime management included.

However, Microsoft wants to switch the AppAuthentication users to Azure.Identity, when the latter package wasn't designed to support the use case of the former package it replaces.

To put it in another way: when we connect to Azure services with SDKs, such as the Azure storage and messaging related SDKs, that build upon Azure.Identity, we likely never need to touch the underlying access tokens in the first place – this is because those SDKs would directly take a TokenCredential in their configuration extensions.

When we actually need to retrieve the tokens, it implies that we're not using some other Azure SDK, and obviously we always want caching and lifetime management due to the nature of OAuth2 tokens.

@christothes Since currently Azure.Identity's GetTokenAsync already is non-standardized when it comes to caching (some credentials have it, some don't), would it do any harm to implement caching on a "as needed" basis (instead of some very fancy, complicated store that can handle all arbitrary scenarios)?

For example, what about just copying the setup from AppAuthentication for development credentials or any credentials that don't yet have caching?

https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/mgmtcommon/AppAuthentication/Azure.Services.AppAuthentication/Cache/AppAuthResultCache.cs

As for "identity and scope", just make it one cache entry for each TokenRequestContext + tenant Id/object Id combination?

@ghost ghost added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-author-feedback Workflow: More information is needed from author to address the issue. labels Dec 5, 2022
@christothes
Copy link
Member

@christothes Since currently Azure.Identity's GetTokenAsync already is non-standardized when it comes to caching (some credentials have it, some don't), would it do any harm to implement caching on a "as needed" basis (instead of some very fancy, complicated store that can handle all arbitrary scenarios)?

For example, what about just copying the setup from AppAuthentication for development credentials or any credentials that don't yet have caching?

https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/mgmtcommon/AppAuthentication/Azure.Services.AppAuthentication/Cache/AppAuthResultCache.cs

As for "identity and scope", just make it one cache entry for each TokenRequestContext + tenant Id/object Id combination?

Hi @mikequ-taggysoft
Just to clarify, Azure.Identity doesn't implement any token caching, there are just some credentials (mostly development time credentials) that don't call MSAL directly. As mentioned earlier, we have no plans to implement one at this time.

@adstep
Copy link

adstep commented Feb 11, 2023

I came across this issue after following the docs on Azure AD authentication for Application Insights which recommend to use DefaultAzureCredential for local development. For every log that was getting sent out, their library was requesting a new token and it completely bogged down my application.

It looks like they're under the impression that Azure.Identity was shipping a caching solution (see comment in microsoft/ApplicationInsights-dotnet/issues/2539).

@christothes from your statement about what Azure.Identity supports, it seems like this is something the AppInsights team needs to make the decision on - whether they want to implement it themselves or push the problem on to their customers. However, it seems like there's an opportunity here for a common solution for caching these credentials that can be used across libraries Microsoft ships.

@dylan-asos
Copy link

@christothes Since currently Azure.Identity's GetTokenAsync already is non-standardized when it comes to caching (some credentials have it, some don't), would it do any harm to implement caching on a "as needed" basis (instead of some very fancy, complicated store that can handle all arbitrary scenarios)?
For example, what about just copying the setup from AppAuthentication for development credentials or any credentials that don't yet have caching?
https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/mgmtcommon/AppAuthentication/Azure.Services.AppAuthentication/Cache/AppAuthResultCache.cs
As for "identity and scope", just make it one cache entry for each TokenRequestContext + tenant Id/object Id combination?

Hi @mikequ-taggysoft Just to clarify, Azure.Identity doesn't implement any token caching, there are just some credentials (mostly development time credentials) that don't call MSAL directly. As mentioned earlier, we have no plans to implement one at this time.

Pushing responsibility of caching to the caller has no doubt resulted in engineers across many different organisations creating token caching implementations. Caching oauth tokens is a requirement for us all, so it's surprising it's been left out.

I can understand there's a complexity you'd rather not deal with and simply provide the raw functionality for acquiring the tokens, but it's exactly because of that complexity it would've been nice to solve it once, centrally, in the official supported SDK

@github-actions github-actions bot locked and limited conversation to collaborators Jul 11, 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. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that
Projects
None yet
Development

No branches or pull requests

9 participants