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

SQL Server: consider adding first class support for SqlConnection.AccessToken for AAD #13261

Open
Tracked by #22961
divega opened this issue Sep 7, 2018 · 56 comments
Open
Tracked by #22961

Comments

@divega
Copy link
Contributor

divega commented Sep 7, 2018

SqlConnection.AccessToken is added to SqlClient for .NET Core in 2.2 to enable Azure Active Directory Authentication.

Although it is already possible to use the feature with the current APIs in EF Core and our SQL Server provider, it requires creating the SqlConnection object and passing it to UseSqlServer (or somehow setting the access token just before the connection object is ever opened).

That pattern is a bit more convoluted and forces you to deviate from the common pattern in ASP.NET Core applications that uses AddDbContext with a simple connection string.

This issue is about making this more first-class, for example, by enabling passing the AccessToken in the UseSqlServer call as a separate piece of configuration from the connection string.

We should understand the common patterns to use the feature safely to decide what APIs we actually need:

  • Is the access token obtained using async API?
  • Can we assume that the authentication token will be obtained only once and then can be cached?
@divega
Copy link
Contributor Author

divega commented Oct 23, 2018

If you already have the code necessary to obtain the AccessToken, #11928 (comment) describes a clever approach to set the AccessToken on each instance of the DbContext without changing how the DbContext itself gets registered in DI.

@divega divega changed the title SQL Server: consider adding first class support for SqlConnection.AccessToken SQL Server: consider adding first class support for SqlConnection.AccessToken for AAD Nov 28, 2018
@KalyanChanumolu-MSFT
Copy link
Member

@divega Do you have a code sample that I can refer?
I don't quite get the approach described in the link.

@divega
Copy link
Contributor Author

divega commented Jan 16, 2019

@Appjunkie sorry, I don't have an example at the moment. I need to work on one, but I am not sure when I will get to it. Perhaps the original poster of the workaround can help. cc @cbrianball.

@cbrianball
Copy link

@Appjunkie I posted some sample code

@ajcvickers
Copy link
Member

See also #15247 and consider:

  • When/where/how frequently the call to get an access token should be made
  • Allowing this to be done asynchronously

@ChristopherHaws
Copy link
Contributor

I would love to see this added as a supported feature. In the meantime, I am doing this which seems to work pretty well: https://gist.github.com/ChristopherHaws/b1c54b95838f1513bfb74fa1c8e408f3
Ideally, we wouldn't even need to do this if we could specify something in the connection string and have SqlClient handle it itself.

@svrooij
Copy link

svrooij commented Jul 22, 2019

@ChristopherHaws even with your very nice solution I'm getting an error System.Data.SqlClient.SqlException (0x80131904): Login failed for user 'NT AUTHORITY\ANONYMOUS LOGON'.. Could you show us the used connection string? I think that might be where I made a mistake. Mine looks like

Server=tcp:correct-server.database.windows.net,1433;Initial Catalog=existingdb;Persist Security Info=False;MultipleActiveResultSets=False;Encrypt=True;TrustServerCertificate=False;

@cbrianball
Copy link

@svrooij It's likely not a connection string issue. The error message you gave is, unfortunately, a very generic one that doesn't help (as is usually the case with security-based exceptions). It likely means the user you are trying to connect with doesn't have permissions to connect to the database.

Assuming you're using an MSI (or user-assigned service identity), and you've configured permissions correctly, then it could simply be a waiting game as permissions propagate (I've had to wait hours before it would work in the past).

If you haven't already, I would recommend using a connection string (separate from the database connection string) for AzureServiceTokenProvider. The reason I recommend this is that if you don't, the API will essentially try each of it's own internal providers until it finds one that works, this added 7+ seconds to obtaining a token in the past.

@svrooij
Copy link

svrooij commented Jul 22, 2019

Thank for the tip, I was already using that. Locally it’s set to RunAs=Developer; DeveloperTool=VisualStudio and in Azure it’s set to RunAs=App.
My Azure AD is used in Windows & Visual Studio and can also be used to login to in SQL management studio with the Azure AD with MFA setting, my laptop is Azure AD domain joined.

So it could be either that the token isn’t correct, but I have no way of knowing/checking. Or that the token isn’t transmitted.

Locally it fetches a token from Azure that shows my user ID, has the correct audience and includes the guids of the Azure AD groups.
What claims should be in the token to be valid for SQL?
How can I debug this? At the moment it seems to be a black box to me. Already tried overriding the logging level for Microsoft.EntityFrameworkCore to Debug but that doesn’t help much.

@cbrianball
Copy link

@svrooij Sounds like you are doing everything right. If you are using MSI (instead of user-assigned identity) and you want to see the actual token, you can use the following Power Shell script and execute it on the VM/App Service to output the token (you will want to update the resource query string parameter). If you are using an App Service, I use the App Service editor (there's an 'Open Console' menu item) to execute the Power Shell.

$uri = "$($env:MSI_ENDPOINT)?resource=https://graph.microsoft.com/&api-version=2017-09-01"
$ProgressPreference = "SilentlyContinue"	
# Get an access token for managed identities for Azure resources
$response = Invoke-WebRequest -UseBasicParsing -Uri $uri ` -Headers @{Secret="$env:MSI_SECRET"}
$responseAsString = [System.Text.Encoding]::ASCII.GetString($response.Content)
echo $responseAsString

I'm not sure what would need to change in the script to get it to work with a user assigned identity (I've never used that feature before).

@syndicatedshannon
Copy link

syndicatedshannon commented Sep 8, 2019

In the meantime, I am doing this which seems to work pretty well: https://gist.github.com/ChristopherHaws/b1c54b95838f1513bfb74fa1c8e408f3
quoted from: #13261 (comment)

@ChristopherHaws Hey, Chris, can you comment on why you override CreateMasterConnection? Is that a source of problems in some scenarios?

I prefer to modify as little boilerplate as possible in my gap-filling hacks, but maybe some BadThings (tm) will happen if I don't follow your example here. I wonder why the default SqlConnection implementation doesn't work in Azure, now that SqlClient has support for AccessToken; is it not a clone operation under the covers but rather an ignorant ConnectionString copy? I also wonder if my own application will call CreateMasterConnection through some scenario I'm not prepared for. If so, then I think other recommended approaches (such as this one) also don't account for those same BadThings.

Would love your knowledge here.

@syndicatedshannon
Copy link

Side note, I also found this implementation from @roysanchez, which adds IsMultipleActiveResultSetsEnabled and SupportsAmbientTransactions. These may be Azure-specialized values of interest to other reading this thread as well, I'm not sure.

@ChristopherHaws
Copy link
Contributor

@syndicatedshannon I need to override it because it needs to return a new instance of itself. The default implementation returns a new SqlServerConnection and the overridden one returns an AzureSqlServerConnection. Without overriding it, CreateMasterConnection won't authenticate with an access token. I agree, I don't like overriding it either. I am hoping for a built in solution soon so I can remove my custom implementation of a pubternal type. :)

@syndicatedshannon
Copy link

syndicatedshannon commented Nov 9, 2019

Related comment on the necessity of patching CreateMasterConnection: MicrosoftDocs/azure-docs#38814 (comment)

side note: I get sad when I rediscover problems I've already tackled but not solved. 😢

@syndicatedshannon
Copy link

syndicatedshannon commented Nov 10, 2019

@AndriySvyryd
Copy link
Member

We might not need to do anything here after dotnet/SqlClient#730

@sopelt
Copy link

sopelt commented Sep 29, 2020

Looks nice for production ... a solution using Azure.Identity would avoid having to use a completely different approach for local development.

@sebader
Copy link

sebader commented Sep 29, 2020

@sopelt I think you might have a good point there. Probably worth for you to comment on that PR as well, not just here

@jabbera
Copy link

jabbera commented Sep 29, 2020

The MSI solution only runs on Azure based infrastructure. If arc supported MSI I would be a buyer, but you still can't do local development against an azure database using an AccessToken easily which makes me sad.

@ericsampson
Copy link

ericsampson commented Sep 30, 2020

yeah dotnet/SqlClient#730 is great, but will still need to figure out a good solution for localdev.

@roji
Copy link
Member

roji commented Oct 20, 2020

For anyone here, this blog post shows how to write a trivial interceptor to use Azure Identity. However, that sample causes GetTokenAsync to get executed every time a context is created, which may have serious perf impacts. It may be possible to cache the token if that's a concern, of use context pooling to mitigate that.

Note to implementor: any solution we'd provide out of the box in EF Core would like make use of that library as well under the hood. This would have the consequence of taking a reference on Azure Identity from the SQL Server provider, which isn't ideal for everyone not using Azure. Some sort of plugin may mitigate that, but I'm not sure we currently have the proper extension point for that.

@ErikEJ
Copy link
Contributor

ErikEJ commented Oct 20, 2020

@roji M.D.S. already depends on https://www.nuget.org/packages/Microsoft.Identity.Client/

@roji
Copy link
Member

roji commented Oct 20, 2020

Yeah, in that case no worries to add that dependency at the EF Core level, thanks!

@roji
Copy link
Member

roji commented Oct 20, 2020

Though am not sure what the exact relation is between Microsoft.Identity.Client and Azure.Identity... Seems that the latter depends on the former, so we may need to add an additional dependency (which isn't necessarily a problem).

@ErikEJ
Copy link
Contributor

ErikEJ commented Oct 21, 2020

@roji Some additional information about Azure.Identity and SqlClient dotnet/SqlClient#730 (comment) (probably whole discussion in PR is of interest)

@roji
Copy link
Member

roji commented Oct 21, 2020

Thanks @ErikEJ!

@roji
Copy link
Member

roji commented Oct 23, 2020

tl;dr fetching the access token on each and every connection (i.e. calling GetTokenAsync) is a perf anti-pattern for at least some scenarios, since tokens aren't always internally cached by Azure.Identity.

There are currently various samples for setting up an interceptor to set the access token on SqlConnection before opening it (see this great blog post, as well as this docs PR sample). I've investigated if the pattern shown by these resources is OK from a perf perspective, and got the following response from @schaabs:

While many of the credential types in Azure.Identity do in fact cache tokens, not all do, and if you're directly calling GetTokenAsync the best practice is to cache the tokens returned, especially if they are in a hot code path. We've recently updated our documentation around GetTokenAsync to provide better guidence here, adding the following.

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.

In this case the example from the blog post is using the EnvironmentCredential and the ManagedIdentityCredential. The EnvironmentCredential will cache tokens it obtains, but the ManagedIdentityCredential will not. The ManagedIdentityCredential does make a network call, but it's a loop back call to a local endpoint so the latency wouldn't be terrible. However, if OpenAsync is called with high frequency it's possible that calls will get throttled.

So interceptors (as well as any feature developed here) should be configured with a user-provided timeout, during which they cache the access token.

Note this very useful discussion on how access tokens are handled between SqlClient and SQL Server.

@ericsampson
Copy link

@roji, I'm a little confused - I thought we'd just be able to use the new connection string formats added by SqlClient; are you planning on adding Managed Identity support in EFCore using Azure.Idendity instead (or in addition)?

@roji
Copy link
Member

roji commented Oct 23, 2020

@ercisampson if you just want to do stuff via the connection string, there's nothing you need from EF Core here - you can already pass an access token that way. Unless I'm missing something, this is in order to support setting the new SqlConnection.AccessToken property, programmatically.

Importantly, a full solution here needs to support rotation for the access tokens. Once again, if you want to do everything via the connection string, that means it's your responsibility as a user to change that connection string, integrating a new connection string before the current one expires. A more turn-key solution would automatically take care of getting a new access token when necessary, and assigning that to SqlConnection.AccessToken without the user needing to do anything.

Does that make sense?

@roji
Copy link
Member

roji commented Oct 23, 2020

Note dotnet/SqlClient#771, which is about implementing the bulk of this inside SqlClient, making the Azure.Identity integration available to non-EF Core users as well. Assuming that's done, this issue can track a thin wrapping around that support in the EF SqlServer provider.

@ericsampson
Copy link

ericsampson commented Jun 22, 2021

@roji , I'm not sure if this is what you're looking for because it's been hard for me to track these issues, but SqlClient 3.0 now uses Azure.Identity and adds support for the equivalent of DefaultAzureCredentials:
https://github.com/dotnet/SqlClient/blob/main/release-notes/3.0/3.0.0.md#active-directory-default-authentication-support

Even if it's not the ultimate solution that you were looking for, it would be nice to pull this version in :)

@ErikEJ
Copy link
Contributor

ErikEJ commented Jun 22, 2021

@ericsampson 3.0 is not LTS, so adding that to EF Core will happen with EF Core 7.

@ericsampson
Copy link

3.0 of SqlClient? I didn't realize that package had the concept of LTS or not.

Thanks Erik :)

@ErikEJ
Copy link
Contributor

ErikEJ commented Jun 22, 2021

It does, and EF Core 6 LTS needs LTS dependencies.

@sander1095
Copy link

@ericsampson 3.0 is not LTS, so adding that to EF Core will happen with EF Core 7.

@ErikEJ

Hello Erik!

Currently EF Core Sql Server uses Microsoft.Data.SqlClient 2.0.1 but I couldn't get the Authentication key in my connectionstring to work (for Managed Identity auth) even though 2.0 should support this if I understand correctly.

Not to worry, I just installed 3.0 manually and now everything works.

If I understand correctly, you mean that only EF Core 7 will update to 3.0, not EF Core 6.0? Will 6.0 perhaps update to v2.1 where the Authentication property might work better for my scenario?

Also, do you have a source for Microsoft.Data.SqlClient 3.0 not being LTS? I'd love to read more about this.

Thanks!

@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 21, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment