-
Notifications
You must be signed in to change notification settings - Fork 299
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
Integrate with Azure.Identity #771
Comments
Do any other ADO providers provide this level of integration with federated identity? |
Hi Shay, Ack: We'll update here after a more concrete design decision is made. |
@roji is that really a big issue if you just you connection resiliency (which you always should with Azure SQL DB anyway) ? |
Looking into it AAD looks like it uses the same model as openid and if that's the case the management of the access token it's revocation, refresh and lifetime should be managed above the driver layer. |
@ErikEJ this isn't a transient error, since the user must start using a new access token (ideally before any exception occurs). In other words, it's not enough to just retry the operation - you must set SqlConnection.AccessToken to a new value (which must be fetched and cached appropriately too). It's true that the user could handle this by using the same token until the exception occurs, and use that as a trigger to rotate the token - though that's weird; the user has all the information to know beforehand that the previous trigger is about to get invalidated and could avoid the exceptions. Another possible point is that I'm not sure access token authentication is only for Azure SQL - maybe it can be used with regular SQL Server too (on-premises or cloud-hosted)? If so, then you're much less expected to use connection resiliency in general.
Why do you say that, is there some rule somewhere saying where this should managed? Also, OpenID is a general auth framework that's relevant to a wide multitude of services, whereas here we're talking about something that's specific to SQL Azure. Any reason to require all SQL Azure users to go through the same extra hoops rather than just doing it for them in the driver for that specific service? |
@roji I've definitely heard talk of federated authentication potentially coming to on-premise servers. So I've been trying to keep that in mind when directing feature work in the drivers. |
I'll try to simply thoughts as per current behavior. If customer provides access token via "Access Token" connection property, then it's validity is in the hands of user (who is also the token provider). So whenever customer opens a new connection, it is required to provide a "valid" access token. Driver does not intercept this access token and passes the same directly to server, letting customer applications take full control of it. Since the user provided "access token" is also part of Pool Key, a new access token creates a new corresponding connection pool.
Usually customers do this by capturing "Expiry Time" and comparing it before passing the token to driver. On the other hand, if user only provides credentials/ auth mode etc. and it is the driver that requests token from MSAL/Azure.Identity (third-party libs), that is where SqlClient works along with libraries to "maintain" this access token that's owned by the driver. This token is cached/refreshed/renewed as needed. Most of the caching logic is done in third-party libs, but SqlClient also has a tiny cache when it comes to acquiring tokens for new connections in the same pool. Here, since access token is owned internally, renewal of tokens does not create new connection pool. |
Sound great @cheenamalhotra. One small note on what you said:
For Azure.Identity specifically, it seems that the responsibility of caching the token is with the user of the library - it's a perf anti-pattern to just call GetTokenAsync every time you want to open a connection, and expect Azure.Identity to cache internally (see dotnet/efcore#13261 (comment)). If SqlClient integrates with Azure.Identity, then SqlClient becomes the "user", so if I understand everything correctly, it would have to cache the token internally. |
As of now SqlClient relies on MSAL (Microsoft.Identity.Client) to cache access tokens, the current implementation of For perf reasons, I agree it may be possible to cache tokens in the driver, but honestly none of the SQL Drivers do that actually. Also as Azure SDK and MSAL libraries have advanced their caches, there're less network calls made anyways, so I'm also not sure how much benefit we will get from implementing our own cache. |
@cheenamalhotra I made that comment on the necessity of caching the results of GetTokenAsync from Azure.Identity because I just had that discussion with @schaabs (quoted in dotnet/efcore#13261 (comment)), that's their recommendation. But we may be discussing different things. |
@roji I'd also recommend syncing with Azure Identity teams to understand how Azure Identity is intended to be used by their client applications and whether or not caching is required. e.g. This blog post is very helpful for Azure Client apps: Best practices for using Azure SDK with ASP.NET Core that recommends using "DefaultAzureCredential" which also internally tries to fetch token from SharedTokenCacheCredential that contains token cache. There are also customization options you can use to take benefit from this cache, i.e. DefaultAzureCredentialOptions Now these are definitely password-less solutions and new recommendations and do not offer everything/every mode of token authentication. But maybe you don't even need that in EF Core and could only use this to provide token implicitly to SqlClient. If at all you need a cache, you may think about that but I don't feel that's highly essential given there is that capability from Azure.Identity already. If users want to go for password/service principal modes, they could directly specify that in SqlClient connection string and why needing that support from EF Core. Also there's a possibility that these modes will come to SqlClient too in future, so doing a lot of work in EF Core doesn't seem very valuable. Edit: I later noticed you're engaged with correct team, but I'll let this comment stay :) |
Additional notes: I think you're discussing correct topics there, I agree |
I think we're saying the same thing. SqlClient doesn't seem to have any issue at the moment with regards to SqlConnection.AccessToken - it's a relatively low-level API that makes it the user's concern to fetch, cache and refresh the access token. So you don't have much to worry about. If, however, you do decide to expose higher-level integration with Azure.Identity, in order to remove that burden from the user and make it more straightforward to use access tokens, then at least with Azure.Identity it would be necessary to implement some form of simple caching (according to the info I got). Ideally, the user just configures a TokenCredential and a refresh interval, once at startup, and SqlClient takes care of all the rest (hopefully I'm getting things right). I don't know about other authentication methods and libraries - some may be indeed doing their own caching fully internally. |
As a consumer, what I'd like to be able to do is use the unified Azure.Identity library |
Hi @cheenamalhotra, hi @roji, I hope you'll continue your discussion and find a way to combine Azure.Identity and SqlAuthenticationProvider in some way. I appreciate all the work in these areas and think there are lots of customers on Azure that would benefit from a first class solution. Right now (with connection string support for AAD integrated/interactive/MSI) many scenarios are solvable quite well but some more complex ones (e.g. home tenant not the same as resource tenant - does that work? integrated/cached authentication with non-federated/pure AAD accounts) would really benefit from the added flexibility. |
SqlClient already allows an access token to be set, either in the connection string or programmatically on SqlConnection. However, the actual work of retrieving (and refreshing, and caching) that token is left to the user. Azure.Identity handles the retrieving of tokens, and SqlClient could integrate with it
This was originally requested by @sopelt in #730 (comment). @cheenamalhotra, below that you made the point that Azure.Identity doesn't support .NET Framework like SqlClient. However, wouldn't it be possible to do this integration for .NET Standard 2.0, allowing .NET Core users to take advantage of this?
Note that if this is done, for good perf SqlClient would be responsible for caching the access token for a delay configured by the user (see dotnet/efcore#13261 (comment)).
/cc @ajcvickers
The text was updated successfully, but these errors were encountered: