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 Health Check with Managed Identity (Azure Access Token) #150

Closed
johnayoub opened this issue Apr 11, 2019 · 8 comments · Fixed by #664
Closed

SQL Server Health Check with Managed Identity (Azure Access Token) #150

johnayoub opened this issue Apr 11, 2019 · 8 comments · Fixed by #664
Assignees
Labels
enhancement New feature or request

Comments

@johnayoub
Copy link

The current API doesn't allow connecting to Azure SQL Server using managed identity and an access token!

Any plans to add this feature or any suggested workaround in the meantime?

@johnayoub johnayoub changed the title SQL Server Health with Manged Identity (Azure Access Token) SQL Server Health Check with Manged Identity (Azure Access Token) Apr 11, 2019
@unaizorrilla
Copy link
Collaborator

Hi,

Interesting!! Let me check this. Of course contributions are open!! 👍

@johnayoub johnayoub changed the title SQL Server Health Check with Manged Identity (Azure Access Token) SQL Server Health Check with Managed Identity (Azure Access Token) Apr 11, 2019
@unaizorrilla unaizorrilla self-assigned this Apr 17, 2019
@unaizorrilla unaizorrilla added the enhancement New feature or request label Apr 17, 2019
@inkel
Copy link
Contributor

inkel commented Nov 6, 2019

I've similar issues with this and ended up creating a custom SQL Server health check: we use a custom connection manager, so this health check uses that, but while working on that I thought about how this feature could be added to AspNetCore.HealthChecks.SqlServer.

There's a tutorial named Secure Azure SQL Database connection from App Service using a managed identity that does the following once the connection is created:

var conn = (System.Data.SqlClient.SqlConnection)Database.GetDbConnection();
conn.AccessToken = (new Microsoft.Azure.Services.AppAuthentication.AzureServiceTokenProvider()).GetAccessTokenAsync("https://database.windows.net/").Result;

In this case conn could be replaced with conn = SqlConnection(_connectionString) in src/HealthChecks.SqlServer/SqlServerHealthCheck.cs, however that way of getting the access token is only for Azure managed SQL Databases. At my company we get the access token in a different way:

var credential = new ClientCredential(clientId, clientSecret);
var authenticationContext = new AuthenticationContext(databaseAuthority);
var authenticationResult = await authenticationContext.AcquireTokenAsync(target, credential);
var accessToken = authenticationResult.AccessToken;

Given that there might be many different ways to specify the value of SqlConnection.AccessToken, I think that the easiest way to implement this is to change the SqlServerHealthCheck constructor to either accept an optional string accessToken = null value, or an overloaded that accepts an SqlConnection instead.

I think the simplest is definitely adding an additional parameter, however, adding a constructor that accepts SqlConnection can be useful when combined with dependency injection.

Either way, I'll gladly send a PR for this if you like and we agree on how to implement it.

@Nisden
Copy link
Contributor

Nisden commented Sep 1, 2020

@inkel What about adding an Action<SqlConnection> beforeOpen? That would allow users to set the AccessToken, and would be more future proof.

@inkel
Copy link
Contributor

inkel commented Sep 3, 2020

@Nisden that sounds great! Would you like me to submit a PR for this?

@Nisden
Copy link
Contributor

Nisden commented Sep 5, 2020

@inkel If you got the time for it, it would be awesome. Otherwise I could look into it.

@inkel
Copy link
Contributor

inkel commented Sep 9, 2020

@Nisden hey, sorry for the delay in replying.

Yeah, I think I can manage to make the time this or next week. Would that work?

@ghost
Copy link

ghost commented Oct 31, 2020

I could really do with this functionality as well. I will raise a PR today if that is alright? I think I will go for the beforeOpen Action approach since that seems more flexible.

@inkel
Copy link
Contributor

inkel commented Nov 3, 2020

@JoeSainsburys thank you! I really hope they approve it soon and is available in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants