Move the authentication provider registry into Abstractions - #4670
paulmedynski wants to merge 12 commits into
Conversation
Introduce the shared signing-key download step and thread isInternalBuild through the CI core so the SqlServer package is strong-name signed on internal Package-mode builds. - Add eng/pipelines/common/steps/download-assembly-signing-key.yml, which exports driverKeyFile or testKeyFile from ADO secure files. - Adopt that step in the OneBranch build and nightly CI package pipelines, renaming keyFile to driverKeyFile. - Declare isInternalBuild in dotnet-sqlclient-ci-core.yml and set it from the CI package- and project-reference pipelines. - Sign the SqlServer package when isInternalBuild is true and referenceType is not Project.
- Quote SigningKeyPath in buildProperties so the secure-file path is robust to spaces, matching build-buildproj-step.yml and sqlclient-ci-package-pipeline.yml. - Move the BuildNumber/FileVersion note directly above buildProperties in both the signed and unsigned pack branches. No change to signing behaviour: signing stays gated on internal Package-reference builds.
- Split download-assembly-signing-key.yml into download-driver-signing-key-step.yml and download-test-signing-key-step.yml, each parameterless with its own output. - Restore "strong-name signing" terminology; always name the driver or test key. - Remove parameter defaults added in this branch; pass values explicitly, including isInternalBuild: false in both PR pipelines. - Fold the driver key download into the existing signing conditional in the pack job.
Thread referenceType and isInternalBuild into the Logging stage and pack job so the Logging package is strong-name signed on internal Package-mode builds. Abstractions references Logging, so Logging must be signed before Abstractions can be; a strong-named assembly referencing a weak-named one fails with CS8002, which is an error here because TreatWarningsAsErrors is enabled. Logging has no test project yet, so no test-assembly signing or signed InternalsVisibleTo grant is added.
- Fix the signing key template reference to download-driver-signing-key-step.yml - Quote SigningKeyPath to tolerate whitespace in the secure file path - Collapse the duplicated isInternalBuild/referenceType conditional - Drop parameter defaults for referenceType and isInternalBuild - Use positive referenceType comparisons (eq 'Package')
…ions Sign the Abstractions package and its test assembly on internal Package-mode builds, and make the Abstractions CI jobs actually honour Package mode. - Thread isInternalBuild through the Abstractions stage into the pack and test jobs, downloading the driver and test signing keys as needed. - Add a signed InternalsVisibleTo grant for the test assembly, and sign the test assembly when a test key is supplied. - Pass packageVersion, loggingArtifactsName and referenceType into the test jobs, and download the Logging artifacts, so Package mode is exercised. - Correct the pack job to use the canonical SqlClientPackageVersion and BuildNumber properties instead of undeclared per-package parameters. - Thread signing and reference-type arguments into the build.proj TestAbstractions target. - Build the Abstractions test project for net462 only on Windows hosts.
Pre-emptively align this PR with the review feedback already addressed lower in the stack, so the same comments aren't raised again. - Reference the split download-driver-signing-key-step.yml and download-test-signing-key-step.yml templates instead of the removed download-assembly-signing-key.yml. - Quote SigningKeyPath and TestSigningKeyPath to tolerate whitespace in the secure file paths. - Fold the driver key download into the existing signing conditional in the pack job, removing the duplicated conditional. - Use positive referenceType comparisons (eq 'Package'). - Drop the referenceType and isInternalBuild parameter defaults; both are already passed explicitly by every caller. - Move the BuildNumber/FileVersion note directly above buildProperties in every pack branch. - Restore "strong-name signing" terminology and name the driver or test key explicitly.
Sign the SqlClient and AKV Provider assemblies, and the SqlClient test assemblies, on internal Package-mode CI builds. SqlClient references Abstractions, Logging and SqlServer.Server, so those packages must already be signed for this to build; a strong-named assembly referencing a weak-named one fails with CS8002. - Thread isInternalBuild from the CI core into the SqlClient package stage and the test stage, and download the driver and test signing keys in the build and test jobs. - Pass signingKeyPath to the project build step and both signing key paths to the test steps. - Thread SigningKeyPath into the build.proj TestSqlClientFunctional and TestSqlClientManual targets, and both key arguments into TestSqlClientUnit.
Pre-emptively align this PR with the review feedback already addressed lower in the stack, so the same comments aren't raised again. - Reference the split download-driver-signing-key-step.yml and download-test-signing-key-step.yml templates instead of the removed download-assembly-signing-key.yml. - Use positive referenceType comparisons (eq 'Package'). - Drop the isInternalBuild parameter defaults; every caller already passes it explicitly. - Use "strong-name signing" terminology and name the driver or test key in the signingKeyPath/testSigningKeyPath parameter docs, correcting the stale note about test-filter categories.
Sign the Azure package and its test assembly on internal Package-mode builds. Azure references Abstractions and Logging, both of which are signed earlier in this stack, so the reference closure is complete. - Thread isInternalBuild from the CI core into the Azure stage, pack job and test jobs, downloading the driver and test signing keys as needed. - Add a signed InternalsVisibleTo grant for the Azure test assembly, which needs internal access for the WAM broker tests, and sign that assembly when a test key is supplied. - Thread signing arguments into the build.proj TestAzure target. - Rename the test job's buildArguments variable to dotnetBuildOpts; Azure Pipelines exposes variables as environment variables and the dotnet CLI injects BUILDARGUMENTS into dotnet build.
Pre-emptively align this PR with the review feedback already addressed lower in the stack, so the same comments aren't raised again. - Reference the split download-driver-signing-key-step.yml and download-test-signing-key-step.yml templates instead of the removed download-assembly-signing-key.yml. - Quote SigningKeyPath and TestSigningKeyPath to tolerate whitespace in the secure file paths. - Fold the driver key download into the existing signing conditional in the pack job, removing the duplicated conditional. - Use positive referenceType comparisons (eq 'Package'). - Drop the isInternalBuild parameter defaults; every caller already passes it explicitly. - Normalize the BuildNumber/FileVersion note across all pack branches. - Restore an accidentally dropped blank line in Azure.Test.csproj.
There was a problem hiding this comment.
🟡 Changes recommended
Lazy bootstrap currently overwrites user providers and direct delegation changes the public failure contract.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Moves authentication-provider state into Abstractions, removing the reflection bridge while retaining driver-side lazy discovery.
Changes:
- Adds the shared registry, lifecycle handling, localization, and direct public API delegation.
- Renames the driver manager to
AuthenticationBootstrapperand lazily initializes providers. - Relocates and expands registry/bootstrapper tests.
File summaries
| File | Description |
|---|---|
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlAuthenticationProviderManagerTests.cs |
Removes superseded manager tests. |
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/DummySqlAuthenticationProvider.cs |
Adds the unit-test config provider. |
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/AuthenticationBootstrapperTests.cs |
Tests bootstrap configuration and provider creation. |
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj |
Copies .NET Framework test configuration. |
src/Microsoft.Data.SqlClient/tests/UnitTests/app.config |
Configures bootstrapper test values and provider. |
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlAuthenticationProviderManagerTests.cs |
Removes relocated functional tests. |
src/Microsoft.Data.SqlClient/tests/FunctionalTests/Microsoft.Data.SqlClient.FunctionalTests.csproj |
Removes obsolete config copying. |
src/Microsoft.Data.SqlClient/tests/FunctionalTests/DataCommon/DummySqlAuthenticationProvider.cs |
Removes relocated dummy provider. |
src/Microsoft.Data.SqlClient/tests/FunctionalTests/app.config |
Removes relocated authentication configuration. |
src/Microsoft.Data.SqlClient/tests/FunctionalTests/AADAuthenticationTests.cs |
Removes relocated provider-registration test. |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs |
Triggers lazy bootstrap before provider lookup. |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AuthenticationBootstrapper.cs |
Retains configuration and Azure discovery in the driver. |
src/Microsoft.Data.SqlClient/ref/Microsoft.Data.SqlClient.cs |
Clarifies reference-assembly nullable conventions. |
src/Microsoft.Data.SqlClient.Extensions/Azure/test/WamBrokerTests.cs |
Removes a global registry test. |
src/Microsoft.Data.SqlClient.Extensions/Azure/test/SqlAuthenticationProviderGlobalCollection.cs |
Renames the global-state test collection. |
src/Microsoft.Data.SqlClient.Extensions/Azure/test/DefaultAuthProviderTests.cs |
Removes superseded bootstrap coverage. |
src/Microsoft.Data.SqlClient.Extensions/Azure/test/Azure.Test.csproj |
Updates test signing configuration. |
src/Microsoft.Data.SqlClient.Extensions/Azure/test/AuthenticationBootstrapperTests.cs |
Tests real Azure-provider construction. |
src/Microsoft.Data.SqlClient.Extensions/Azure/test/AuthenticationBootstrapperGlobalTests.cs |
Tests global Azure-provider bootstrap. |
src/Microsoft.Data.SqlClient.Extensions/Azure/test/AADAuthenticationTests.cs |
Uses the renamed global collection. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/test/SqlAuthenticationProviderTest.cs |
Verifies direct shared-registry delegation. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/test/AuthenticationProviderRegistryTest.cs |
Adds comprehensive registry tests. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Strings.zh-Hant.resx |
Adds Traditional Chinese error text. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Strings.zh-Hans.resx |
Adds Simplified Chinese error text. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Strings.tr.resx |
Adds Turkish error text. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Strings.ru.resx |
Adds Russian error text. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Strings.resx |
Adds the invariant error resource. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Strings.pt-BR.resx |
Adds Brazilian Portuguese error text. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Strings.pl.resx |
Adds Polish error text. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Strings.ko.resx |
Adds Korean error text. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Strings.ja.resx |
Adds Japanese error text. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Strings.it.resx |
Adds Italian error text. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Strings.fr.resx |
Adds French error text. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Strings.es.resx |
Adds Spanish error text. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Strings.Designer.cs |
Adds strongly typed resource access. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Strings.de.resx |
Adds German error text. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Strings.cs.resx |
Adds Czech error text. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/SqlAuthenticationProvider.Internal.cs |
Deletes the reflection bridge. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/SqlAuthenticationProvider.cs |
Delegates directly to the registry. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/IsExternalInit.cs |
Adds an older-target compiler polyfill. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/AuthenticationProviderRegistry.cs |
Implements shared provider storage and precedence. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Abstractions.csproj |
Grants driver and test internal access. |
src/Microsoft.Data.SqlClient.Extensions/Abstractions/doc/SqlAuthenticationProvider.xml |
Updates registry and lifecycle documentation. |
Review details
Files not reviewed (1)
- src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Strings.Designer.cs: Generated file
Suppressed comments (1)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AuthenticationBootstrapper.cs:361
- These calls install the Azure provider with the normal replacement API, so a custom provider registered by the application before its first AD connection is replaced during lazy bootstrap. That reverses the previous ordering (bootstrap first, then user registration) and contradicts the comment that defaults only fill missing methods. Add an atomic “set if absent” registry operation for Azure defaults, while retaining
SetPermanentProviderfor app.config entries.
- Files reviewed: 42/43 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| internal void SetPermanentProvider(SqlAuthenticationMethod authenticationMethod, SqlAuthenticationProvider provider) | ||
| { | ||
| _providers[authenticationMethod] = new ProviderEntry(provider, IsPermanent: true); | ||
| } |
| public static bool SetProvider( | ||
| SqlAuthenticationMethod authenticationMethod, | ||
| SqlAuthenticationProvider provider) | ||
| { | ||
| return Internal.SetProvider(authenticationMethod, provider); | ||
| return AuthenticationProviderRegistry.Instance.SetProvider(authenticationMethod, provider); |
| // Verify that the bootstrapper installs the Azure auth provider for all AAD/Entra | ||
| // authentication methods, and not for any other methods. | ||
| // | ||
| // This project configures neither applicationClientId nor useWamBroker (it has no app.config | ||
| // overrides), so the bootstrapper constructs the Azure extension's |
| // No config (applicationClientId and useWamBroker both unset) -> parameterless ctor -> the | ||
| // first-party id, which enables WAM broker. | ||
| [Fact] | ||
| public void CreateAzureProvider_NoConfig_UsesParameterlessCtor() |
| <!-- Strong name signing ============================================= --> | ||
| <!-- When a test signing key is provided, sign the test assembly so IVT from signed source works. --> | ||
| <PropertyGroup Condition="'$(TestSigningKeyPath)' != ''"> | ||
| <SignAssembly>true</SignAssembly> | ||
| <AssemblyOriginatorKeyFile>$(TestSigningKeyPath)</AssemblyOriginatorKeyFile> | ||
| </PropertyGroup> |
| [Fact] | ||
| public void CreateAzureAuthenticationProvider_NeitherConfigured_UsesParameterlessCtor() |
The public SqlAuthenticationProvider.GetProvider/SetProvider methods lived in the Abstractions package but the registry state they mutate lived in Microsoft.Data.SqlClient. Because the dependency arrow points SqlClient -> Abstractions, the only way to reach the registry from Abstractions was reflection: SqlAuthenticationProvider.Internal used Assembly.Load and MethodInfo.Invoke to call into the internal SqlAuthenticationProviderManager. Move the registry state down into Abstractions, the layer both assemblies already share, so both sides call it directly: - Add AuthenticationProviderRegistry (internal) to Abstractions. It owns the ConcurrentDictionary keyed by SqlAuthenticationMethod, the IsSupported enforcement, the BeforeLoad/BeforeUnload callbacks, and the permanent (application-specified) provider precedence rule. - Delete SqlAuthenticationProvider.Internal.cs. GetProvider/SetProvider now call the registry directly. Their signatures are unchanged. - Split SqlAuthenticationProviderManager into AuthenticationBootstrapper, which stays in the driver and keeps the app.config parsing and Azure extension discovery. It seeds the shared registry through SetPermanentProvider. - Bootstrap lazily from SqlConnectionInternal on the federated auth path instead of eagerly from a static constructor, so the Azure extension is only loaded when a connection actually authenticates. - Grant InternalsVisibleTo to Microsoft.Data.SqlClient (signed and unsigned) solely so the bootstrapper can seed permanent providers; the general get/set path uses the public API. - Add localized Strings resources to Abstractions for the unsupported-method error that previously came from SQL.UnsupportedAuthenticationByProvider. Tests that exercise the bootstrapper move from FunctionalTests to UnitTests, which can see driver internals, and the Azure extension tests are renamed to match the new type names. No public API change.
ba06a60 to
943678b
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Lazy bootstrap can overwrite explicit providers, and the public setter now exposes previously translated exceptions.
Review details
Files not reviewed (1)
- src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Strings.Designer.cs: Generated file
Suppressed comments (2)
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/SqlAuthenticationProvider.cs:35
- Direct delegation changes the public API's failure semantics. The removed reflection bridge caught
TargetInvocationExceptionand returnedfalsefor manager/provider failures, including null and unsupported providers; this now exposesNullReferenceExceptionorNotSupportedException, despite the stated compatibility goal and the XML contract documenting a false result rather than exceptions. Preserve exception-to-false translation at this public boundary and add public-API regression tests.
return AuthenticationProviderRegistry.Instance.SetProvider(authenticationMethod, provider);
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AuthenticationBootstrapper.cs:357
- These calls replace existing overridable entries rather than installing defaults only. With lazy bootstrap, the documented pattern of calling
SqlAuthenticationProvider.SetProvider(...)beforeOpen()first stores the custom provider, thenBootstrap()replaces it with the Azure provider. Previously bootstrap completed before the public set call, so the custom provider won. Add an atomic default-only/TryAdd registry operation and use it for every Azure method, with a regression test for registration before first authentication.
- Files reviewed: 41/42 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Description
Removes the reflection bridge from
Microsoft.Data.SqlClient.Extensions.AbstractionsintoMicrosoft.Data.SqlClientby moving the authentication provider registry down into the layer both assemblies already share.The problem
The public
SqlAuthenticationProvider.GetProvider/SetProvidermethods live in the Abstractions package, but the registry state they mutate lived in the core driver. Because the dependency arrow points SqlClient → Abstractions, Abstractions could not call the internalSqlAuthenticationProviderManagerdirectly without a cycle — so it reflected.SqlAuthenticationProvider.InternalusedAssembly.Load+MethodInfo.Invokeon every get/set.The change
The manager's surface is typed on
SqlAuthenticationProviderandSqlAuthenticationMethod, which already live in Abstractions. So the registry can sit there, and both sides call it with concrete, strongly-typed APIs against the same static state.AuthenticationProviderRegistry(internal) in Abstractions. Owns theConcurrentDictionary<SqlAuthenticationMethod, ProviderEntry>, theIsSupportedenforcement, theBeforeLoad/BeforeUnloadlifecycle callbacks, and the permanent (application-specified) provider precedence rule.SqlAuthenticationProvider.Internal.csdeleted (−198 lines).GetProvider/SetProvidernow call the registry directly. Their signatures are unchanged.SqlAuthenticationProviderManager→AuthenticationBootstrapper. The bootstrap half stays in the driver, where it belongs:app.configsection parsing,SqlAuthenticationInitializercreation, and strong-name-verified Azure extension loading. It seeds the shared registry throughSetPermanentProvider.SqlConnectionInternaltriggers the one-time initialization on the federated/Active Directory auth path rather than from a static constructor, so the Azure extension is loaded only when a connection actually authenticates.Stringsresources added to Abstractions for the unsupported-method error that previously came fromSQL.UnsupportedAuthenticationByProvider.API changes / compatibility
No public API change.
SqlAuthenticationProvider.GetProvider/SetProviderkeep their signatures and semantics; the registry staysinternal.One accepted behavior difference: the default Azure provider now appears once the driver's bootstrap runs — i.e. when a connection authenticates — rather than being eagerly force-loaded from a static constructor. Conversely, a user's
SetProvidercall now works even if the core assembly has not been loaded yet, since the provider sits in the shared singleton until the driver reads it.InternalsVisibleToAbstractions grants
InternalsVisibleTotoMicrosoft.Data.SqlClient(both the unsigned and product-key-signed forms) for one reason: config-specified provider precedence. A provider declared inapp.configmust be registered as non-overridable, so a later userSetProviderreturnsfalseinstead of replacing it. That marking —SetPermanentProvider— is intentionally not public. The general get/set path does not need it.Assembly size and performance
Code shifts from the driver into Abstractions; the former shrinks by roughly what the latter grows. Abstractions is a hard dependency of the driver and never ships without it, so total footprint is unchanged (net negative, after deleting the bridge). Get/set become direct calls instead of
MethodInfo.Invoke, and the one-timeAssembly.Load+ reflection lookup in the static constructor is gone.Issues
Prerequisite for the NativeAOT fix for #4193. This PR removes the Abstractions→SqlClient reflection edge structurally; the SqlClient→Azure discovery reflection is handled separately in the follow-up PR stacked on this one.
Testing
AuthenticationProviderRegistryTest.cs(Abstractions) — get/set,IsSupportedrejection, permanent-provider precedence, override semantics,BeforeLoad/BeforeUnloadinvocation and exception isolation.AuthenticationBootstrapperTests.cs(UnitTests) — config-driven discovery, application client ID,UseWamBrokerparsing, initializer creation, registry seeding.AuthenticationBootstrapperTests.cs/AuthenticationBootstrapperGlobalTests.cs(Azure extension tests) — default provider installation via the bootstrapper.FunctionalTeststoUnitTests, which can see driver internals.SqlAuthenticationProviderManagerTests.csand itsapp.configmove with them.SqlAuthenticationProviderTest.csto exercise the registry instead of the reflection bridge.Validation on this branch:
Microsoft.Data.SqlClient+ Abstractions + UnitTests build clean fornet9.0, 0 warnings.SimulatedServerTests.ConnectionTests.IntegratedAuthConnectionTest, "Cannot generate SSPI context") is pre-existing and environmental — no Kerberos on the Linux test host.Known follow-up
The Azure-extension loading trace calls relocated into
AuthenticationBootstrapper.LoadAzureExtensionProvideruse interpolated strings, so this branch does not carry #4528's parameterized trace-call conversion for that block. That conversion applied only to the code this PR rewrites. Worth re-applying before this leaves draft.Stack
Split out of #4573, which previously combined this structural refactor with the AOT feature-switch work. #4573 is rebased onto this branch and now contains only the AOT changes.