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

Integrate with Azure.Identity #771

Closed
roji opened this issue Oct 23, 2020 · 16 comments · Fixed by #1010 or #1043
Closed

Integrate with Azure.Identity #771

roji opened this issue Oct 23, 2020 · 16 comments · Fixed by #1010 or #1043
Labels
💡 Enhancement New feature request

Comments

@roji
Copy link
Member

roji commented Oct 23, 2020

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

@Wraith2
Copy link
Contributor

Wraith2 commented Oct 23, 2020

Do any other ADO providers provide this level of integration with federated identity?

@cheenamalhotra
Copy link
Member

Hi Shay,

Ack: We'll update here after a more concrete design decision is made.

@roji
Copy link
Member Author

roji commented Oct 24, 2020

@Wraith2 I'm not sure... But I'm also not aware of a credential rotation scheme such as the one SQL Azure does here - especially where old connections which validated successfully are broken after a while (see #767).

@ErikEJ
Copy link
Contributor

ErikEJ commented Oct 24, 2020

@roji is that really a big issue if you just you connection resiliency (which you always should with Azure SQL DB anyway) ?

@Wraith2
Copy link
Contributor

Wraith2 commented Oct 24, 2020

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.

@roji
Copy link
Member Author

roji commented Oct 25, 2020

@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.

@Wraith2

if that's the case the management of the access token it's revocation, refresh and lifetime should be managed above the driver layer.

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?

@David-Engel
Copy link
Contributor

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.

@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.

@cheenamalhotra
Copy link
Member

cheenamalhotra commented Oct 26, 2020

@roji @Wraith2

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.

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.

Usually customers do this by capturing "Expiry Time" and comparing it before passing the token to driver.
The token is refreshed/renewed with a buffer of ~5 mins to token expiry, depending on client's preferences.

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.

@roji
Copy link
Member Author

roji commented Oct 27, 2020

Sound great @cheenamalhotra. One small note on what you said:

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.

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.

@cheenamalhotra
Copy link
Member

@roji

As of now SqlClient relies on MSAL (Microsoft.Identity.Client) to cache access tokens, the current implementation of ActiveDirectoryAuthenticationProvider in the driver does not cache access tokens. As I said, we only have a small cache when it comes to pooled connections, but that's outside the ActiveDirectoryAuthenticationProvider.

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 cheenamalhotra added this to Needs triage in SqlClient Triage Board via automation Oct 27, 2020
@cheenamalhotra cheenamalhotra moved this from Needs triage to Ideas for Future in SqlClient Triage Board Oct 27, 2020
@roji
Copy link
Member Author

roji commented Oct 27, 2020

@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.

@cheenamalhotra
Copy link
Member

cheenamalhotra commented Oct 27, 2020

@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 :)

@cheenamalhotra
Copy link
Member

Additional notes:

I think you're discussing correct topics there, I agree ManagedIdentityCredential doesn't make use of cache so that's something to think about. I also know Azure's App Authentication library caches tokens from Managed Identity for 24 hours (fixed lifetime) and uses the same for new requests. You may wanna check with them to understand it's pros and cons too.

@roji
Copy link
Member Author

roji commented Oct 28, 2020

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.

@ericsampson
Copy link

As a consumer, what I'd like to be able to do is use the unified Azure.Identity library
just like I can with the Key Vault V4 SDK, and other recently 'Azure-SDK standardized' libraries,
and not to have to think about caching or anything whether using managed identity creds, localdev, etc:
https://www.rahulpnath.com/blog/defaultazurecredential-from-azure-sdk/

@sopelt
Copy link

sopelt commented Jan 8, 2021

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.
I know going towards a solution as proposed in #730 (comment) should not be too bad but if every other project needs to implement that ...

SqlClient v3.0 automation moved this from Review In Progress to Done May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Enhancement New feature request
Projects
No open projects
7 participants