- 
                Notifications
    You must be signed in to change notification settings 
- Fork 316
Move AAD/Entra Authentication into new Azure package #3680
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
base: feat/azure-split
Are you sure you want to change the base?
Move AAD/Entra Authentication into new Azure package #3680
Conversation
        
          
                src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlAuthenticationParameters.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      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.
Commentary for reviewers and some TODOs for me.
| Get-ChildItem *.dll -Path $(extractedNugetPath) -Recurse | ForEach-Object VersionInfo | Format-List | ||
| displayName: 'Verify "File Version" matches expected values for DLLs' | ||
| - powershell: | | 
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 removed the TestMicrosoftDataSqlClientVersion build property entirely. It was made redundant by MdsPackageVersion.
| <FileVersion>$(AbstractionsAssemblyFileVersion)</FileVersion> | ||
| <Version>$(AbstractionsPackageVersion)</Version> | ||
|  | ||
| <DocumentationFile>$(Artifacts)/doc/$(TargetFramework)/$(AssemblyName).xml</DocumentationFile> | 
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 will figure out where to put the XML docs in a later PR. For now, they go somewhere ignored by git.
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.
This enum was moved out of TdsEnums.cs as-is.
| namespace Microsoft.Data.SqlClient; | ||
|  | ||
| /// <include file='../doc/SqlAuthenticationParameters.xml' path='docs/members[@name="SqlAuthenticationParameters"]/SqlAuthenticationParameters/*'/> | ||
| public sealed class SqlAuthenticationParameters | 
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.
This and the token class are sealed. There is no need for an app to derive from them.
| string accessToken, | ||
| DateTimeOffset expiresOn) | ||
| { | ||
| if (string.IsNullOrEmpty(accessToken)) | 
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.
A null or empty token is meaningless, so I moved this policy decision out of MDS and into Abstractions. I think this is correct.
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 you move it out, you need to localize the exception message...
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.
Will be localized in a future PR.
        
          
                ...ta.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationTimeoutRetryHelper.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      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.
This is what remained in MDS from the SqlAuthenticationParamaters implementation.
| using System.IO; | ||
| using System.Reflection; | ||
|  | ||
| #nullable enable | 
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.
You will see some new ? and null checks below related to this.
| /// <summary> | ||
| /// Convert from a SqlAuthenticationToken. | ||
| /// </summary> | ||
| internal SqlFedAuthToken(SqlAuthenticationToken token) | 
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.
Moved from SqlAuthenticationToken, since conversion to a SqlFedAuthToken belongs here anyway.
| $(ThisAssemblyClsCompliant) is false. | ||
| --> | ||
| <ThisAssemblyClsCompliantContent Condition="'$(ThisAssemblyClsCompliant)' != 'false'"> | ||
| [assembly: System.CLSCompliant(true)] | 
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.
These new config properties are necessary to avoid collisions now that MDS depends on Abstractions.
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.
Commentary for reviewers.
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 decided to see how things look if we move GetProvider() and SetProvider() from SqlAuthenticationProvider to SqlAuthenticationProviderManager. This is a breaking change, but it separates the provider API definition from the act of managing providers. It was a mistake to combine these two into the provider API originally. I feel liks this is a worthwhile breaking change, and it will only affect apps that were managing providers explicitly via the get/set methods.
If we don't like this change, I can easily revert since it is all in a single commit.
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.
In my opinion the API split itself makes sense (provided the breaking change is acceptable.) The class itself is currently halfway between being a completely static class and a class with a singleton instance, and if it's going to be exposed then I think it'd be best to settle on one approach or the other.
I've got one or two other comments, but they're not central to the public API; it's up to you whether they get pushed out to a later preview.
- AuthenticationEnumFromStringoverlaps with- SqlConnectionStringBuilder's parsing logic, and if the constants line up then we might be able to simplify by using that instead.
- SetProvidercurrently iterates over a HashSet to perform a search for the authentication method; it can just use- Containsinstead.
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.
This will be of lot of impact and will break many customers on upgrade as most customers use this API to register custom provider, I would say we keep it as-is for now if its not necessarily needed.
We need to keep the API impact nil for the Azure Split work, only library inclusion should be needed.
ddc22a7    to
    71fd319      
    Compare
  
    a60a025    to
    7f88ea6      
    Compare
  
    71fd319    to
    283f241      
    Compare
  
            
          
                src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserHelperClasses.cs
          
            Show resolved
            Hide resolved
        
      | Codecov Report❌ Patch coverage is  Additional details and impacted files@@                 Coverage Diff                 @@
##             feat/azure-split    #3680   +/-   ##
===================================================
  Coverage                    ?   76.77%           
===================================================
  Files                       ?      269           
  Lines                       ?    45029           
  Branches                    ?        0           
===================================================
  Hits                        ?    34573           
  Misses                      ?    10456           
  Partials                    ?        0           
 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
7f88ea6    to
    c66066a      
    Compare
  
    - Removed addition of PDBs from <AllowedOutputExtensionsInPackageBuildOutputFolder>. - Removed MDS package ref dependency on Abstractions until pipelines are ready. - Renamed AbstractionsPackage to Abstractions in targets. - Updated BUILDGUIDE based on project ref behaviour. - Added feature branches to CI pipeline triggers. - Added missing/incomplete paths to the trigger. - Added dev/* branches to the CI triggers so PRs that target other dev/ branches can run CI. - Added missing MdsPackageVersion property to signed build pipeline job. - Commented-out failing NuGet tool installer task. - Re-ordered Guardian analysis step _before_ build step to avoid clobbering versioned DLLs. - Added MDS package version to AKV build/package steps. - Restored AKV nuspec ReferenceType property for AKV Official builds. - Explicitly building tooling before analysis. - Fixing validation steps to match XML props files. - Added PR automation triggers, and documented other pipeline sections. - Added ReferenceType throughout the MDS/AKV CI build steps. - Added NuGet.config update to main CI build step for Package reference mode. - Swapped Abstractions download and NuGet.config update to ensure packages/ exists before attempting to re-configure NuGet. - Clean target no longer removes packages/ - Uncommented package refs to Abstractions. - Added separate MDS and AKV project builds to support Package mode. - Added $ReferenceType$ property to MDS .nuspec like it is for AKV. - Removed obsolete version variables from merge conflict. - Adding $ReferenceType$ back to AKV nuspec. - Moved SqlAuthenticationProvider and friends into Abstractions. - Moved ActiveDirectoryAuthenticationProvider into a new Azure package. - Moved a bunch of docs into Abstractions and Azure projects. - Updated Abstractions classes to be abstract with minimal implementation. - Added back the implementations to MDS. - Re-worked some of the docs. - SqlAuthentication* classes separated out into Base classes in Abstractions. - Updated docs accordingly. - MDS compiles. - Some Azure package code moved, but not compiling. - Added an exception class for authentication errors. - Enabled XML doc compilation and validation. - Updated MDS to handle SqlAuthenticationProviderException instead of MSAL exceptions. - Removed Azure.Identity from MDS. - Got existing tests compiling using the new Azure package. - Added Azure package build targets. - Updated docs related to ReferenceType - Got all projects properly adhering to Package reference type. - Removed TestMicrosoftDataSqlClientVersion in favour of MdsPackageVersion. - Fixed tools restore from build.proj. - Fixed missing using for dummy auth provider. - Fixed obsolete warnings for password auth. - Moved SqlAuthenticationParameters entirely into Abstractions, avoiding an empty base class. - Moved SqlAuthenticationToken entirely into Abstractions. - Moved SqlAuthenticationProvider entirely into Abstractions. - Removed SqlAuthenticationProviderBase. - Exposed SqlAuthenticationProviderManager as a public API in MDS. - Moved SetProvider() and GetProvider() to the Manager where they should have been all along (Breaking Change). - Added catch-all exception handlers in prep for proper analysis and specific exception handling. - Some improvements to avoid meaningless nulls. - Added PR/CI pipeline support for the Azure package. - Fixed Abstractions download path. - Updated SqlAuthenticationProviderException to use Method instead of Action. - Fixed NRE in SqlException. - Fixed SqlAuthenticationProviderException use in MDS. - Added chaining Add() to SqlErrorCollection. - Added a useful message when _fedAuthToken is null after attempting to acquire a token. - Fixed auth provider error handling to mimic the old code. - Removing unnecessary database checks that are hanging forever. - Fixed timestamp truncation from long to uint causing cached tokens to be erroneously expired.
87be738    to
    224a316      
    Compare
  
    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.
Commentary for reviewers, and a few things for me to fix.
| platform: '${{parameters.platform }}' | ||
| configuration: '${{parameters.configuration }}' | ||
| msbuildArguments: '-t:BuildTestsNetFx -p:TF=${{parameters.targetFramework }} -p:TestSet=${{parameters.testSet }} -p:ReferenceType=${{parameters.referenceType }} -p:TestMicrosoftDataSqlClientVersion=${{parameters.mdsPackageVersion }} -p:AbstractionsPackageVersion=${{ parameters.abstractionsPackageVersion }}' | ||
| msbuildArguments: '-t:BuildTestsNetFx -p:TF=${{parameters.targetFramework }} -p:TestSet=${{parameters.testSet }} -p:ReferenceType=${{parameters.referenceType }} -p:MdsPackageVersion=${{parameters.mdsPackageVersion }} -p:AbstractionsPackageVersion=${{ parameters.abstractionsPackageVersion }} -p:AzurePackageVersion=${{ parameters.azurePackageVersion }}' | 
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.
For now, the existing tests for AAD/Entra auth will remain within the MDS tests, so they need to depend on the new Azure package where the code was moved.
In a future PR, these tests will move over to the Azure package, and none of the MDS tests will depend on the Azure package.
| targetPath: $(Build.SourcesDirectory)/packages | ||
|  | ||
| # Use the local NuGet.config that references the packages/ directory. | ||
| - pwsh: cp $(Build.SourcesDirectory)/NuGet.config.local $(Build.SourcesDirectory)/NuGet.config | 
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.
This is much simpler than what the existing pipelines do to parse the JSON, update it, and then write it back out via PowerShell foo. We should use this approach when we develop the new pipelines.
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.
Docs for the Abstractions and Azure packages are still considered works in progress. There will be future PRs that provide complete documentation.
        
          
                ...rosoft.Data.SqlClient.Extensions/Abstractions/test/SqlAuthenticationProviderExceptionTest.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/Microsoft.Data.SqlClient.Extensions/Abstractions/test/SqlAuthenticationTokenTest.cs
          
            Show resolved
            Hide resolved
        
      | private static void SetDefaultAuthProviders(SqlAuthenticationProviderManager instance) | ||
| { | ||
| if (instance != null) | ||
| // If our Azure extensions package is present, use its | 
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.
This is the new logic that looks for the Azure package assembly and loads the AAD Auth class from it.
|  | ||
| internal static SqlException CreateException(SqlErrorCollection errorCollection, string serverVersion, Guid conId, Exception innerException = null, SqlBatchCommand batchCommand = null) | ||
| { | ||
| Debug.Assert(errorCollection != null && errorCollection.Count > 0, "no errorCollection?"); | 
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.
Don't use debug asserts to check pre-conditions!
| return exc; | ||
| } | ||
|  | ||
| internal static Exception Azure_ManagedIdentityException(string msg) | 
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.
This was unused before any of these changes. I removed it to avoid future confusion.
| { | ||
| internal string spn; | ||
| internal string stsurl; | ||
| internal string Spn { get; } | 
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.
These fields are now immutable, which provides a stronger class invariant. Same changes to SqlFedAuthToken below.
| try | ||
| { | ||
| sqlConnection.Open(); | ||
| Assert.True(DataTestUtility.IsSupportedDataClassification()); | 
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.
This is already checked by the [ConditionalFact] above.
        
          
                src/Directory.Packages.props
              
                Outdated
          
        
      | <!-- Azure Dependencies --> | ||
|  | ||
| <ItemGroup> | ||
| <PackageVersion Include="Azure.Identity" Version="1.14.2" /> | 
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.
Is there a reason not to use the latest version (1.17.0)?
        
          
                src/Microsoft.Data.SqlClient.Extensions/Azure/src/AuthenticationException.cs
          
            Show resolved
            Hide resolved
        
              
          
                ...crosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs
          
            Show resolved
            Hide resolved
        
      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.
Partial 45/89 review
| buildConfiguration: ${{ parameters.buildConfiguration }} | ||
| displayNamePrefix: Win | ||
| jobNameSuffix: windows | ||
| netFrameworkRuntimes: [net462, net47, net471, net472, net48, net481] | 
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.
Do we really need to test all those combinations? Don't other jobs just target the lowest version we support?
| <AcquireTokenAsync> | ||
| <param name="parameters">The Active Directory authentication parameters passed by the driver to authentication providers.</param> | ||
| <summary>Acquires a security token from the authority.</summary> | ||
| <param name="parameters">The Active Directory authentication parameters passed by the driver to authentication providers.</param> | 
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.
Perfect opportunity to fix this name (Azure AD to Entra ID rebranding a few years ago). In fact, I don't think we even need the second-fourth words:
| <param name="parameters">The Active Directory authentication parameters passed by the driver to authentication providers.</param> | |
| <param name="parameters">The parameters passed by the driver to authentication providers.</param> | 
| <summary> | ||
| Initializes a new instance of the <see cref="T:Microsoft.Data.SqlClient.SqlAuthenticationParameters" /> class using the specified authentication method, server name, database name, resource URI, authority URI, user login name/ID, user password, connection ID and connection timeout value. | ||
| </summary> | ||
| <param name="connectionTimeout">The connection timeout, in seconds.</param> | 
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.
This has always bothered me. This should be the timeout for the authentication step within the overall connection establishment. We might even be within a command timeout during an idle connection recovery event. Calling it "connection timeout" seems inaccurate. I think "authentication timeout" would be better.
        
          
                src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/SqlAuthenticationProvider.cs
          
            Show resolved
            Hide resolved
        
      | string accessToken, | ||
| DateTimeOffset expiresOn) | ||
| { | ||
| if (string.IsNullOrEmpty(accessToken)) | 
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 you move it out, you need to localize the exception message...
| @@ -0,0 +1,34 @@ | |||
| namespace Microsoft.Data.SqlClient.Extensions.Abstractions.Test; | |||
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.
Missing license header
| @@ -0,0 +1,78 @@ | |||
| namespace Microsoft.Data.SqlClient.Extensions.Abstractions.Test; | |||
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.
Missing license header
| @@ -0,0 +1,117 @@ | |||
| namespace Microsoft.Data.SqlClient.Extensions.Abstractions.Test; | |||
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.
Missing license header
        
          
                src/Microsoft.Data.SqlClient.Extensions/Azure/src/AuthenticationException.cs
          
            Show resolved
            Hide resolved
        
      | <Project Sdk="Microsoft.NET.Sdk"> | ||
|  | ||
| <PropertyGroup> | ||
| <TargetFrameworks>net462;net47;net471;net472;net48;net481;net8.0;net9.0</TargetFrameworks> | 
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 think we really only need net462...
…r to maintain API for now. - Addressed review comments.
- Added some config improvements to xUnit runners.
        
          
                src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/SqlAuthenticationParameters.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/SqlAuthenticationProvider.cs
          
            Show resolved
            Hide resolved
        
              
          
                src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/SqlAuthenticationProviderInternal.cs
          
            Show resolved
            Hide resolved
        
              
          
                src/Microsoft.Data.SqlClient.Extensions/Abstractions/test/SqlAuthenticationProviderTest.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      c66066a    to
    287309f      
    Compare
  
    - Reverted SqlAuthenticationParameters authenticationTimeout constructor argument to its original name connectionTimeout and added docs explaining why.
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.
This will be of lot of impact and will break many customers on upgrade as most customers use this API to register custom provider, I would say we keep it as-is for now if its not necessarily needed.
We need to keep the API impact nil for the Azure Split work, only library inclusion should be needed.
        
          
                src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/SqlAuthenticationParameters.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      - Moved some tests around to get access to internals and to compile everywhere.
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 reverted all of the previous public API changes and left notes about what we may want to bring back in the future.
| /// that configuration source. | ||
| /// </summary> | ||
| [Fact] | ||
| [ConditionalFact(typeof(TestUtility), nameof(TestUtility.IsNetFramework))] | 
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.
Prefer conditional execution to conditional compilation.
| // This tests the dynamic assembly loading code in the Abstractions | ||
| // package. | ||
| [Fact] | ||
| public void Abstractions_And_Manager_GetSetProvider_Equivalent() | 
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 moved this to the UnitTests project for easy access to the Manager internals.
        
          
                src/Microsoft.Data.SqlClient/tests/FunctionalTests/DataCommon/TestUtility.cs
          
            Show resolved
            Hide resolved
        
      | } | ||
| // Deal with Msal service exceptions first, retry if 429 received. | ||
| catch (MsalServiceException serviceException) | ||
| catch (SqlAuthenticationProviderException ex) | 
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 mainly seeing differences in the inner exception, but it looks like the outer exceptions that are ultimately thrown should be the same.
Overview
This PR contains changes to move the
ActiveDirectoryAuthenticationProviderout of the MDS package and into the new Azure package. Please refer to the design here:MDS Azure Extension Design
This PR is only concerned with moving the provider code out of MDS and creating the necessary base classes and types in the Abstractions package, which includes:
ActiveDirectoryAuthenticationProvider.csfile into the Azure package.Future PRs will address:
AcquireToken()method.This builds on the previous 2 PRs:
Testing
All existing tests in the MDS project are passing. This confirms that the move didn't break anything that is currently being tested.