-
Notifications
You must be signed in to change notification settings - Fork 278
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
Add | Add Workload Identity Support #2159
Conversation
@@ -453,6 +453,27 @@ public static void ActiveDirectoryManagedIdentityWithCredentialsMustFail() | |||
Assert.Contains(expectedMessage, e.Message); | |||
} | |||
|
|||
[ConditionalFact(nameof(IsAADConnStringsSetup))] | |||
public static void ActiveDirectoryWorkloadIdentityWithCredentialsMustFail() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need to write test cases for auth when the test environment supports workload identity, but I need to do more research in how to write that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, kind of new here, but do you have an update on this? It's 2 months old.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our automated test environment is unlikely to ever support workload identity unless there is a way to emulate it without setting up an expensive Kubernetes environment.
@wsugarman - Do you think that (emulation) could ever happen?
Either way, I'm okay with manual testing of this feature for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unaware of any existing emulation of workload identity, but perhaps it may make its way into other AKS-based offerings that could be simpler to set-up, like Azure Container Apps? I think in the meantime, without AKS, it will need to be manually tested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let me know if you have any additional questions or asks. My team has actually been using our own SQL authentication provider in AKS using similar logic to that included in the PR.
if (parameters.AuthenticationMethod == SqlAuthenticationMethod.ActiveDirectoryServicePrincipal) | ||
{ | ||
AccessToken accessToken = await new ClientSecretCredential(audience, parameters.UserId, parameters.Password, tokenCredentialOptions).GetTokenAsync(tokenRequestContext, cts.Token).ConfigureAwait(false); | ||
SqlClientEventSource.Log.TryTraceEvent("AcquireTokenAsync | Acquired access token for Active Directory Service Principal auth mode. Expiry Time: {0}", accessToken.ExpiresOn); | ||
return new SqlAuthenticationToken(accessToken.Token, accessToken.ExpiresOn); | ||
} | ||
|
||
if (parameters.AuthenticationMethod == SqlAuthenticationMethod.ActiveDirectoryManagedIdentity || parameters.AuthenticationMethod == SqlAuthenticationMethod.ActiveDirectoryMSI) | ||
{ | ||
WorkloadIdentityCredentialOptions options = new() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the properties are initialized using environment variables: https://github.com/Azure/azure-sdk-for-net/blob/99cc57c41c7fa2a3df564ad0c6a745c001cd4517/sdk/identity/Azure.Identity/src/Credentials/WorkloadIdentityCredentialOptions.cs#L13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is documented well, it should work I suppose. I'll let @David-Engel take final call to review if this feature fits well in the AAD stack potentially in sync with other drivers as well.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2159 +/- ##
==========================================
- Coverage 72.58% 72.40% -0.19%
==========================================
Files 309 309
Lines 61959 62003 +44
==========================================
- Hits 44975 44894 -81
- Misses 16984 17109 +125
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@wsugarman,I have not investigated the details yet, but couldn’t’ve achieved this by setting a custom provider? I will dig deeper into this next week, but do we have to add every possible scenario to the driver? |
I had no idea this API existed 😅 This would be a great workaround! |
In addition to creating a custom provider that overrides existing authentication options, there is a new, simpler AccessTokenCallback property added in 5.2 preview 3 that makes implementing new token authentication methods much easier. (Not quite as easy as a connection string adjustment, but a-few-lines-of-code easy.)
|
Oh! That will be great! So, it will join the existing That will be super helpful, but I still think a common scenario like workload identity will warrant inclusion in the driver. Otherwise, users will have to create the same workload identity |
Yes. The
The callback gives SqlClient the expiration date and allows it to get a new token when a new physical connection is required, just like the built-in methods.
Oh definitely. It's just difficult to keep up with all the new Azure AD authentication methods they keep coming up with. So a generic method at least takes the pressure off implementing every new one ASAP. You can write a wrapper around SqlConnection to authenticate however you want and still have connection pooling work correctly. |
@@ -453,6 +453,27 @@ public static void ActiveDirectoryManagedIdentityWithCredentialsMustFail() | |||
Assert.Contains(expectedMessage, e.Message); | |||
} | |||
|
|||
[ConditionalFact(nameof(IsAADConnStringsSetup))] | |||
public static void ActiveDirectoryWorkloadIdentityWithCredentialsMustFail() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, kind of new here, but do you have an update on this? It's 2 months old.
@JRahnama - Thanks for merging it! |
Resolves #2158
In this pull request, I make two changes:
User Id
as theClient Id
for workload identity if specified when usingAuthentication = Active Directory Default
ActiveDirectoryWorkloadIdentity
for the enumSqlAuthenticationMethod
that operates very similarly toActiveDirectoryManagedIdentity
except that it usesWorkloadIdentityCredential
instead.User Id
parameter in the connection string for itsClient Id
if specified. But unlike managed identity,WorkloadIdentityCredentialOptions
defaults its value from environment variables:AZURE_TENANT_ID
,AZURE_CLIENT_ID
, andAZURE_FEDERATED_TOKEN_FILE
. As of writing this description, only theClient Id
may be overridden by the connection string.