-
Notifications
You must be signed in to change notification settings - Fork 286
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 FIC + MSI support #2557
base: main
Are you sure you want to change the base?
Conversation
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.
Just a couple notes for now.
...crosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs
Show resolved
Hide resolved
...crosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2557 +/- ##
==========================================
- Coverage 72.81% 71.71% -1.11%
==========================================
Files 311 308 -3
Lines 61694 62374 +680
==========================================
- Hits 44922 44730 -192
- Misses 16772 17644 +872
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Shall we use |
Since it might take some time for this PR to be release to Microsoft.Data.SqlClient. |
If you're looking for a temporary workaround/solution, ping @yiwwan at Teams, I'll share workaround solution. |
...crosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs
Outdated
Show resolved
Hide resolved
...crosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs
Outdated
Show resolved
Hide resolved
...crosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs
Outdated
Show resolved
Hide resolved
@yhvicey I couldn't find the |
@DavoudEshtehari it's not official env var used by azure.identity (see PR's description), I also worried about what if azure.identity added support for MSI + FIC in the future and didn't use same env var name. Maybe we can switch to a ADO.NET specific env var name. |
We usually don't accept unofficial dependencies to avoid the breaking changes. |
What is the plan to document this new env variable? If this is something that will take time, I would recommend converting this PR to draft or closing if this is not in plan. But the driver only consumes documented APIs, or will not expose a feature until the behavior is documented use of the same. |
Resolves #2556.
This PR adds support for authenticating Azure SQL Server using federated identity credentials (FIC) and managed identity (MSI). It provides similar functionality as the
ActiveDirectoryWorkloadIdentity
, but supports using managed identity directly.It will use values from below environments to finish authentication process:
AZURE_CLIENT_ID
, the client id of multi-tenant Microsoft Entra App id, may be overridden byUser Id
.AZURE_TENANT_ID
, the tenant id where target resource locates.AZURE_MSI_CLIENT_ID
, the client id of managed identity.Note: The first two environment variables are defined in
Azure.Identity
library, the third one is newly defined.