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

Add a SQL Authentication Method for AzureCli credential #2171

Closed

Conversation

pregress
Copy link

In integration tests we don't want to use the ActiveDirectoryDefault as the whole token chain is being processed.
To improve the speed of integration tests that use Azure CLI to authenticate we would like to add an Authentication method purely for AzureCLI.

@pregress
Copy link
Author

@dotnet-policy-service agree

@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Files Coverage Δ
...Data/SqlClient/SqlAuthenticationProviderManager.cs 57.81% <100.00%> (-3.86%) ⬇️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 73.73% <100.00%> (-0.46%) ⬇️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 68.34% <ø> (-0.39%) ⬇️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 68.70% <100.00%> (-0.30%) ⬇️
.../Microsoft/Data/Common/DbConnectionStringCommon.cs 66.45% <100.00%> (+0.64%) ⬆️
...rc/Microsoft/Data/SqlClient/SqlConnectionString.cs 81.79% <100.00%> (+0.07%) ⬆️
...SqlClient/src/Microsoft/Data/SqlClient/TdsEnums.cs 96.59% <ø> (ø)
...ent/SqlAuthenticationProviderManager.NetCoreApp.cs 30.43% <0.00%> (-0.45%) ⬇️
...Data/SqlClient/SqlAuthenticationProviderManager.cs 49.05% <50.00%> (+0.01%) ⬆️
...core/src/Microsoft/Data/SqlClient/SqlConnection.cs 79.39% <71.42%> (-0.41%) ⬇️
... and 3 more

... and 21 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@David-Engel
Copy link
Contributor

Thanks for the submission.

We've been trying to resist adding public APIs (and connection string settings would be considered public API) that contain "Azure" in them.

Can you not use the new AccessTokenCallback property (#1260)? Or create a custom SqlAuthenticationProvider to accomplish what you want?
Note: A SqlAuthenticationProvider can be a DLL that is configured to be used via another app's config like in SSMS:

<?xml version="1.0" encoding="utf-8"?>
<configuration>
    <configSections>
        <section name="SqlClientAuthenticationProviders"
            type="Microsoft.Data.SqlClient.SqlClientAuthenticationProviderConfigurationSection,Microsoft.Data.SqlClient, Version=3.0.0.0, Culture=neutral, PublicKeyToken=23ec7fc2d6eaa4a5"/>
    </configSections>

    <SqlClientAuthenticationProviders applicationClientId="a94f9c62-97fe-4d19-b06d-472bed8d2bcf">
      <providers>
         <add name="active directory interactive"
           type="Microsoft.SqlServer.Management.UI.AadInteractiveAuthProvider.AadAuthenticationProvider, ConnectionDlg.AadInteractiveAuthProvider"/>
      </providers>
    </SqlClientAuthenticationProviders>
</configuration>

@pregress
Copy link
Author

Hi David,

We feel like like all of the Azure.Identity authentication methods should be more easily supported.
As Identity is clearly the way forward in a passwordless future.
eg:

TokenCredential tokenCredential  = //any of the Azure.Identity token credentials
var connectionString = new ConnectionStringBuilder.AddIdentity(tokenCredential).ToString()

But this would introduce more changes, that's why I added an extra authentication method. Since there are already issues complaining about the DefaultTokenCredential speed, because of chaining see: #2072

This drastically reduces the time of our integration tests. (From 5m to 30s)

If you have 50+ projects you don't want to configure or write AccessTokenCallback for each project, which is custom code = introduces more risk.

We expect this out of the box, as these are both Microsoft projects and SqlClient is atm not aligned with the default way of working with identity in Azure.

Co-authored-by: campersau <buchholz.bastian@googlemail.com>
@pregress
Copy link
Author

pregress commented Mar 20, 2024

@David-Engel the problem is even worse if you use powershell and Invoke-SqlCmd.
Since it's not able to handle parallel processing of the token and you receive: Azure CLI authentication timed out.

You can work around this by providing an -AccessToken to Invoke-SqlCmd. But this are all sub optimal solutions.
If the SDK would support more authentication methods, all consuming products would benefit.

If we want to improve global security, these kind of hassles make it very hard for developers that are starting out.
Honestly It should be easier to connect with managed identity then with sql authentication.

@cheenamalhotra
Copy link
Member

This can be done directly by implementing AccessTokenCallback.
Closing the PR as we don't intend to support this new API as of now.

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 this pull request may close these issues.

4 participants