[6.1] Dependency Cleanup #3843
[6.1] Dependency Cleanup #3843
Conversation
- Updated some dependencies, avoiding transitive vulnerabilities. - Updated nuspec files to remove/update dependencies accordingly.
There was a problem hiding this comment.
Pull request overview
This PR performs a dependency cleanup across the Microsoft.Data.SqlClient driver and related test projects. The changes update several dependencies to address transitive vulnerabilities, remove unused dependencies (Microsoft.Bcl.Cryptography, System.Memory, System.Text.Encodings.Web), and update nuspec files to reflect the new dependency structure. The code changes suppress warnings for deliberately used obsolete APIs in Azure Active Directory authentication methods.
- Removed unused dependencies: Microsoft.Bcl.Cryptography, System.Memory, and System.Text.Encodings.Web across driver and test projects
- Updated Azure and identity-related packages to newer versions (Azure.Core 1.50.0, Azure.Identity 1.17.0, Azure.Security.KeyVault.Keys 4.8.0)
- Updated System.Buffers from 4.5.1 to 4.6.1 and System.Text.Json from 8.0.5 to 8.0.6 for net462
- Added pragma warning suppressions for obsolete APIs in authentication code
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/specs/add-ons/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.nuspec | Updated Azure package dependencies and Microsoft.Extensions.Caching.Memory for net9.0 to 9.0.11 |
| tools/specs/Microsoft.Data.SqlClient.nuspec | Updated multiple dependencies across all target frameworks; removed Microsoft.Bcl.Cryptography, System.Text.Encodings.Web, and System.Text.Json from various targets |
| src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Microsoft.Data.SqlClient.TestUtilities.csproj | Removed unused Microsoft.Bcl.Cryptography dependency |
| src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.ExtUtilities/Microsoft.Data.SqlClient.ExtUtilities.csproj | Changed approach from pinning System.Formats.Asn1 to referencing MDS 5.1.8 to avoid transitive vulnerability |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/AADConnectionTest.cs | Added pragma warning suppressions for obsolete AcquireTokenByUsernamePassword API |
| src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj | Removed unused Microsoft.Bcl.Cryptography dependency |
| src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs | Added pragma warning suppressions for obsolete AcquireTokenByUsernamePassword API |
| src/Microsoft.Data.SqlClient/tests/FunctionalTests/Microsoft.Data.SqlClient.FunctionalTests.csproj | Removed unused Microsoft.Bcl.Cryptography dependency |
| src/Microsoft.Data.SqlClient/tests/Directory.Packages.props | Changed approach to reference MDS 5.1.8 instead of pinning System.Formats.Asn1 |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs | Added pragma warning suppressions for obsolete APIs (AcquireTokenByUsernamePassword and SharedTokenCacheUsername) |
| src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj | Removed unused Microsoft.Bcl.Cryptography and System.Memory dependencies |
| src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj | Removed unused Microsoft.Bcl.Cryptography and System.Text.Encodings.Web dependencies |
| src/Microsoft.Data.SqlClient/netfx/ref/Microsoft.Data.SqlClient.csproj | Removed unused Microsoft.Bcl.Cryptography and System.Text.Encodings.Web dependencies |
| src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj | Removed unused Microsoft.Bcl.Cryptography dependency and System.Text.Json reference (provided by framework) |
| src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.csproj | Removed unused Microsoft.Bcl.Cryptography dependency and System.Text.Json reference (provided by framework) |
| src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj | Removed unused System.Text.Encodings.Web dependency |
| src/Directory.Packages.props | Updated package versions and reorganized framework-specific dependencies; removed System.Memory and some test dependencies; updated Microsoft.NET.Test.Sdk, xunit, and other test packages |
tools/specs/add-ons/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.nuspec
Show resolved
Hide resolved
tools/specs/add-ons/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.nuspec
Show resolved
Hide resolved
tools/specs/add-ons/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.nuspec
Show resolved
Hide resolved
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Commentary for reviewers.
| <dependency id="Azure.Identity" version="1.17.0" /> | ||
| <dependency id="Microsoft.Data.SqlClient.SNI.runtime" version="6.0.2" exclude="Compile" /> | ||
| <dependency id="Microsoft.Extensions.Caching.Memory" version="9.0.4" exclude="Compile" /> | ||
| <dependency id="Microsoft.Extensions.Caching.Memory" version="8.0.1" exclude="Compile" /> |
There was a problem hiding this comment.
These 9.0.4 versions were incorrect. The .NET Standard 2.0 target shares the .NET 8.0 package versions.
…to avoid strong-name errors. - Added System.Text.Json back for .NET Standard 2.0.
.../AzureKeyVaultProvider/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.csproj
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/ref/Microsoft.Data.SqlClient.csproj
Outdated
Show resolved
Hide resolved
| <PackageReference Include="Microsoft.IdentityModel.Protocols.OpenIdConnect" /> | ||
| <PackageReference Include="Microsoft.SqlServer.Server" /> | ||
| <PackageReference Include="System.Buffers" /> | ||
| <PackageReference Include="System.Data.Common" /> |
There was a problem hiding this comment.
Not used directly, likely an old transient vulnerability fix.
There was a problem hiding this comment.
It was added with PR: https://github.com/dotnet/SqlClient/pull/2967/files#diff-d6cdac7c9f92790e995dce24e966c09119385fdc076757b8f70b08bd14dde23fR47 since in .NET Framework v4.6.2 the type referenced by us from this namespace (IDbColumnSchemaGenerator) is not available directly in BCL and downloading this package with NuGet is required for applications to work. I would not recommend removing this dependency due to that reason.
There was a problem hiding this comment.
Is this a case where testing using .NET Framework Runtime 4.6.2 would have produced an error? But testing with .NET Framework Runtime 4.8.1 (which is what we do) doesn't?
…T Framework project. - Inhibiting dependency on Microsoft.SqlServer.Server in UDT test projects for .NET Framework. - Fixed duplicate MDS package version in test utilities.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/6.1 #3843 +/- ##
===============================================
- Coverage 66.21% 64.64% -1.57%
===============================================
Files 279 279
Lines 53293 53302 +9
===============================================
- Hits 35286 34458 -828
- Misses 18007 18844 +837
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:
|
...crosoft.Data.SqlClient/tests/FunctionalTests/Microsoft.Data.SqlClient.FunctionalTests.csproj
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/UDTs/Address/Address.csproj
Show resolved
Hide resolved
...s/tools/Microsoft.Data.SqlClient.TestUtilities/Microsoft.Data.SqlClient.TestUtilities.csproj
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj
Show resolved
Hide resolved
.../AzureKeyVaultProvider/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj
Show resolved
Hide resolved
| <PackageReference Include="Microsoft.IdentityModel.Protocols.OpenIdConnect" /> | ||
| <PackageReference Include="System.Buffers" /> | ||
| <PackageReference Include="System.Data.Common" /> | ||
| <PackageReference Include="System.Security.Cryptography.Pkcs" /> |
There was a problem hiding this comment.
This package is referenced in
I would recommend we keep this explicit reference included.
There was a problem hiding this comment.
We only use the SignedCms class from this namespace, and Malcolm had pointed out that it is included with all of the .NET Framework runtimes that we target:
I'm not sure why we would need to explicitly depend on it for .NET Framework.
There was a problem hiding this comment.
- Whether we use only 1 type or more shouldn't discount the importance of package reference IMO.
- The documentation for 'SingleCms' says the library and type is 'Package-provided' so isn't it available with the package only for those runtimes?
We need to consider runtimes that our customers would target, and not those that we target to build the driver. Otherwise our DLLs will reference type from runtime (expecting them to be available in client env), whereas in future .NET Framework runtimes if its not available and is package provided, it would throw exception failing to resolve the type.
System.Data.Common, e.g. is a MUST have reference, cannot be skipped.
There was a problem hiding this comment.
I've added the System.Security.Cryptography.Pkcs package back for all MDS targets. System.Data.Common has been added for .NET Framework 4.6.2.
There was a problem hiding this comment.
I don't think I fully understand this argument. Couldn't any type be moved out to a standalone package at any time? Why would we treat this type differently from any other that can be referenced via package?
The SignedCms type is available via runtime or package for .NET Framework. It's only package exclusive for .NET Standard and .NET. Presumably they added package support for .NET Framework to help people who were multitargeting. But in general, doesn't it complicate our dependency graph to include packages we don't need? Especially if we're not pruning any dependencies. I'd have less of an issue if we did enable pruning.
There was a problem hiding this comment.
After some investigation, I've learned that the .NET Framework DLLs for Pkcs are metadata only and type-forward to the GAC. We take a negligible package size hit to get the metadata. With that in mind, it does feel simpler to unconditionally include the package reference.
For this reason and the reasons you mentioned above, @cheenamalhotra, I think our standard should be to always unconditionally include package references for namespaces that are available via package for .NET Framework.
| <PackageReference Include="Microsoft.IdentityModel.Protocols.OpenIdConnect" /> | ||
| <PackageReference Include="System.Buffers" /> | ||
| <PackageReference Include="System.Security.Cryptography.Pkcs" /> | ||
| <PackageReference Include="System.Text.Encodings.Web" /> |
There was a problem hiding this comment.
Removing System.Text.Encodings.Web seems fine, however we should be including "System.IdentityModel.Tokens.Jwt" NuGet package as we have a direct usage of it here:
There was a problem hiding this comment.
I have added System.IdentityModel.Tokens.Jwt for all MDS targets.
…ier. - Minor tweaks to build tasks to work on Linux with the dotnet CLI.
| <dependency id="System.Security.Cryptography.Pkcs" version="9.0.4" /> | ||
| <dependency id="System.Text.Json" version="9.0.5" /> | ||
| <dependency id="System.Configuration.ConfigurationManager" version="8.0.1" exclude="Compile" /> | ||
| <dependency id="System.Diagnostics.DiagnosticSource" version="6.0.1" /> |
There was a problem hiding this comment.
Inconsistent System.Diagnostics.DiagnosticSource version for netstandard2.0. In src/Directory.Packages.props line 40, this package is specified as version 8.0.1 for netstandard2.0, but here in the nuspec it's specified as 6.0.1. These versions should match to ensure the published package uses the same dependencies as the build.
| <dependency id="System.Diagnostics.DiagnosticSource" version="6.0.1" /> | |
| <dependency id="System.Diagnostics.DiagnosticSource" version="8.0.1" /> |
| inputs: | ||
| solution: build.proj | ||
| msbuildArguments: ' -t:BuildTestsNetFx -p:ReferenceType=${{parameters.referenceType }} -p:TestMicrosoftDataSqlClientVersion=${{parameters.NugetPackageVersion }} -p:TargetNetFxVersion=${{parameters.TargetNetFxVersion }} -p:Configuration=${{parameters.configuration }} -p:Platform=${{parameters.platform }}' | ||
| msbuildArguments: ' -t:BuildTestsNetFx -p:ReferenceType=${{parameters.referenceType }} -p:TestMicrosoftDataSqlClientVersion=${{parameters.NugetPackageVersion }} -p:TF=${{parameters.TargetNetFxVersion }} -p:Configuration=${{parameters.configuration }} -p:Platform=${{parameters.platform }}' |
There was a problem hiding this comment.
$(TF) is the correct way to specify the target framework for add-ons and tests when using build.proj.
| <Import Project="..\..\Directory.Packages.props" /> | ||
|
|
||
| <!-- Test Project Dependencies for all targets. --> | ||
| <!-- Test Common Dependencies --> |
There was a problem hiding this comment.
I moved all of the test-only dependencies here from the main Directory.Packages.props file.
|
|
||
| <!-- Test Project Dependencies for NetFx only. --> | ||
| <ItemGroup Condition="$(TargetFramework.StartsWith('net4'))"> | ||
| <!-- Test .NET Framework Dependencies --> |
There was a problem hiding this comment.
I opted for clarity rather than efficiency here. The combinations of target framework conditions were too hard to reason over, so I made sections for each target framework. We now have some duplicate entries rather than an N-dimensional matrix of conditions.
| <PackageVersion Include="System.Security.Cryptography.Pkcs" Version="8.0.1" /> | ||
| <PackageVersion Include="System.Text.Json" Version="8.0.5" /> | ||
| </ItemGroup> | ||
| <!-- Driver .NET Framework Dependencies --> |
There was a problem hiding this comment.
Same changes here as the test Directory.Packages.props file above. We now have some duplicate entries, but each target framework has a clear section of dependencies that closely matches the .nuspec files and our release notes.
| <dependency id="Microsoft.IdentityModel.Protocols.OpenIdConnect" version="7.7.1" /> | ||
| <dependency id="System.Buffers" version="4.5.1" /> | ||
| <dependency id="System.Buffers" version="4.6.1" /> | ||
| <dependency id="System.Data.Common" version="4.3.0" /> |
There was a problem hiding this comment.
Interestingly, we were advertising dependence on System.Data.Common, but we weren't referencing it in the .NET Framework project. Now that we are referencing it, I had to add an explicit dependency on System.Text.RegularExpressions to avoid a transitive vulnerability (that downstream apps would likely have had to do previously).
| <dependency id="System.Security.Cryptography.Pkcs" version="9.0.4" /> | ||
| <dependency id="System.Text.Json" version="9.0.5" /> | ||
| <dependency id="System.Configuration.ConfigurationManager" version="8.0.1" exclude="Compile" /> | ||
| <dependency id="System.Diagnostics.DiagnosticSource" version="6.0.1" /> |
| <PackageVersion Include="Microsoft.IdentityModel.Protocols.OpenIdConnect" Version="7.7.1" /> | ||
| <PackageVersion Include="Microsoft.SqlServer.Server" Version="1.0.0" /> | ||
| <PackageVersion Include="System.Configuration.ConfigurationManager" Version="8.0.1" /> | ||
| <PackageVersion Include="System.Diagnostics.DiagnosticSource" Version="6.0.1" /> |
There was a problem hiding this comment.
We previously weren't explicitly depending on System.Diagnostics.DiagnosticSource even though we use it directly. .NET Framework and .NET Standard 2.0 were pulling in v8.0.1, and .NET 8 and 9 were pulling v6.0.1. Hence the explicit difference here.
However This is now breaking dotnet CLI project-based restores for the add-on and test projects, since they employ a bunch of properties to choose target frameworks:
C:\Users\paulmedynski\dev\SqlClient\dev\paul\release\6.1\dependencies\src\Microsoft.Data.SqlClient\tests\ManualTests\Microsoft.Data.SqlClient.ManualTesting.Tests.csproj : error NU1109:
Detected package downgrade: System.Diagnostics.DiagnosticSource from 8.0.1 to centrally defined 6.0.1. Update the centrally managed packag
e version to a higher version.
ManualTests -> Microsoft.Data.SqlClient -> System.Diagnostics.DiagnosticSource (>= 8.0.1)
ManualTests -> System.Diagnostics.DiagnosticSource (>= 6.0.1)
The above shows the failure. The restore of the MDS project is incorrectly choosing v8.0.1 when it tries to restore the ManualTests project for .NET 8 or 9. It should be choosing v6.0.1, but the <ProjectReference> gets confused and is assuming a different target framework for MDS.
Builds via build.proj are fine since we always set a bunch of properties that force either .NET Framework or .NET - never both.
I'm not sure what we want to do about this. If I upgrade to v8.0.1 for all targets, the problem goes away. I tried to modify the add-ons and test projects to fix things, but nothing was working for me.
| <GenAPIArgs Condition="'$(GeneratePlatformNotSupportedAssembly)' == 'true' OR '$(GeneratePlatformNotSupportedAssemblyMessage)' != ''">$(GenAPIArgs) -t:"$(GeneratePlatformNotSupportedAssemblyMessage)"</GenAPIArgs> | ||
| <GenAPIArgs Condition="'$(GeneratePlatformNotSupportedAssemblyWithGlobalPrefix)' == 'true'">$(GenAPIArgs) -global</GenAPIArgs> | ||
| <GenAPIPath Condition="'$(MSBuildRuntimeType)' == 'core'"> "$(DotNetCmd) $(ToolsArtifactsDir)$(TargetFramework)\Microsoft.DotNet.GenAPI.dll"</GenAPIPath> | ||
| <GenAPIPath Condition="'$(MSBuildRuntimeType)' == 'core'"> $(DotnetPath)dotnet "$(ToolsArtifactsDir)net8.0\Microsoft.DotNet.GenAPI.dll"</GenAPIPath> |
There was a problem hiding this comment.
Fixes (here and build.proj) to make dotnet CLI builds work.
Description
Details
Transitive-only packages with minor or patch version upgrades are not shown.
MDS - .NET Framework 4.6.2
MDS - .NET Standard 2.0
MDS - .NET 8.0
MDS - .NET 9.0
AKV - .NET Framework 4.6.2
AKV - .NET 8.0
AKV - .NET 9.0
Issues
Resolves #3807.
Testing