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

ActiveDirectoryDefault authentication ignores environment variables used by DefaultAzureCredential #1332

Closed
smichtch opened this issue Oct 12, 2021 · 2 comments · Fixed by #1360

Comments

@smichtch
Copy link
Contributor

smichtch commented Oct 12, 2021

Describe the bug

SqlAuthenticationMethod.ActiveDirectoryDefault does not fall back to environment variables normally used by DefaultAzureCredentialOptions.
Specifically the code below is clobbering all the environment variable values read by the DefaultAzureCredentialOptions constructor even if the SQL connection string isn't overriding for those keys:

DefaultAzureCredentialOptions defaultAzureCredentialOptions = new DefaultAzureCredentialOptions()
{
AuthorityHost = new Uri(authority),
ManagedIdentityClientId = clientId,
InteractiveBrowserTenantId = tenantId,
SharedTokenCacheTenantId = tenantId,
SharedTokenCacheUsername = clientId,
VisualStudioCodeTenantId = tenantId,
VisualStudioTenantId = tenantId,
ExcludeInteractiveBrowserCredential = true // Force disabled, even though it's disabled by default to respect driver specifications.
};

To reproduce

For an azure service with a user-defined managed service identity we would set the AZURE_CLIENT_ID config key in the ARM template during deployment (the ARM template also creates the managed identity and the appId may not be known ahead of time). Other components that rely on AAD auth via DefaultAzureCredential authenticate as the managed identity as expected but for SQL we need to either

  • Construct/update the SQL connection strings to append a ;User ID={appId} key in the ARM template (complex and poor maintainability).
  • Intercept connection strings at runtime when a SqlConnection is requested and edit them to set UserId=Environment.GetVariable("AZURE_CLIENT_ID")
  • Register custom ActiveDirectoryAuthenticationProvider instances to use the desired client ID.

Expected behavior

SqlAuthenticationMethod.ActiveDirectoryDefault token acquisition behavior matches that of DefaultAzureCredential.

Further technical details

Microsoft.Data.SqlClient version: 3.0.1
.NET target: N/A
SQL Server version: N/A
Operating system: N/A

Additional context
Add any other context about the problem here.

@cheenamalhotra
Copy link
Member

Hi @smichtch

The reason why that happens is because we set client Id for ManagedIdentityClientId which could also be an empty string.. We can look into improving it to only set those options if they have a value set.

I'll try it and let you know soon.

@cheenamalhotra
Copy link
Member

@smichtch

I've made PR #1360 to fix this issue and also tested the same to confirm changes. Please feel free to try if you wish to.
We will include this change in next release.

Regards,
Cheena

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

Successfully merging a pull request may close this issue.

2 participants