-
Notifications
You must be signed in to change notification settings - Fork 317
[6.0] Dependency Cleanup #3840
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: release/6.0
Are you sure you want to change the base?
[6.0] Dependency Cleanup #3840
Conversation
- Updated some dependencies, avoiding transitive vulnerabilities. - Updated nuspec files to remove/update dependencies accordingly.
| <PackageReference Include="Newtonsoft.Json" Version="$(NewtonsoftJsonVersion)" /> | ||
| <PackageReference Include="Microsoft.SqlServer.SqlManagementObjects" Version="$(MicrosoftSqlServerSqlManagementObjectsVersion)" /> | ||
| <!-- | ||
| Explicitly reference the latest published MDS 5.1.x version here to avoid |
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 took a long time to figure out.
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.
Pull request overview
This pull request performs a comprehensive dependency cleanup for the 6.0 branch, removing unused dependencies and updating others to address security vulnerabilities. The changes ensure consistency across NuGet package specifications, version property files, and project references for both the main SqlClient driver and the AlwaysEncrypted AzureKeyVaultProvider add-on.
Key changes include:
- Removal of unused dependencies (
Microsoft.Bcl.Cryptography,System.Text.Encodings.Web, and several test-only packages) - Updates to identity and security-related packages (Microsoft.IdentityModel.* from 7.5.0 to 7.7.1, Azure.* packages, System.Buffers, System.Text.Json)
- Addition of an explicit MDS 5.1.8 reference in ExtUtilities to mitigate vulnerable transitive dependencies
- Change from version ranges to fixed versions for Azure.Core and Azure.Security.KeyVault.Keys in the AKV provider
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/specs/add-ons/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.nuspec | Updated Azure.Core and Azure.Security.KeyVault.Keys from version ranges to fixed versions; updated Microsoft.Extensions.Caching.Memory for net9.0 |
| tools/specs/Microsoft.Data.SqlClient.nuspec | Removed Microsoft.Bcl.Cryptography and System.Text.Encodings.Web; updated Microsoft.IdentityModel.*, System.Buffers, System.Text.Json, and Microsoft.Extensions.Caching.Memory versions |
| tools/props/VersionsNet9OrLater.props | Removed Microsoft.Bcl.Cryptography version property; updated Microsoft.Extensions.Caching.Memory to 9.0.11 |
| tools/props/Versions.props | Removed properties for unused dependencies (Microsoft.Bcl.Cryptography, System.Text.Encodings.Web, test dependencies); updated versions for IdentityModel, System.Buffers, System.Text.Json, Azure packages, and test SDKs |
| src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Microsoft.Data.SqlClient.TestUtilities.csproj | Removed unused System.Formats.Asn1 and System.Security.Cryptography.Cng package references |
| src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.ExtUtilities/Microsoft.Data.SqlClient.ExtUtilities.csproj | Added explicit MDS 5.1.8 reference with explanatory comment to avoid vulnerable transitive dependency |
| src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj | Removed Microsoft.Bcl.Cryptography package reference |
| src/Microsoft.Data.SqlClient/tests/FunctionalTests/Microsoft.Data.SqlClient.Tests.csproj | Removed Microsoft.Bcl.Cryptography package reference |
| src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj | Removed System.Text.Encodings.Web and Microsoft.Bcl.Cryptography package references |
| src/Microsoft.Data.SqlClient/netfx/ref/Microsoft.Data.SqlClient.csproj | Removed System.Text.Encodings.Web and Microsoft.Bcl.Cryptography package references |
| src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj | Removed Microsoft.Bcl.Cryptography package reference |
| src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.csproj | Removed Microsoft.Bcl.Cryptography package reference |
| src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj | Removed System.Text.Encodings.Web package reference |
tools/specs/add-ons/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.nuspec
Outdated
Show resolved
Hide resolved
tools/specs/add-ons/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.nuspec
Outdated
Show resolved
Hide resolved
tools/specs/add-ons/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.nuspec
Outdated
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/6.0 #3840 +/- ##
============================================
Coverage 62.58% 62.59%
============================================
Files 285 285
Lines 59152 59152
============================================
+ Hits 37019 37024 +5
+ Misses 22133 22128 -5
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:
|
| <PackageReference Include="Microsoft.IdentityModel.JsonWebTokens" Version="$(MicrosoftIdentityModelJsonWebTokensVersion)" /> | ||
| <PackageReference Include="System.Configuration.ConfigurationManager" Version="$(SystemConfigurationConfigurationManagerVersion)" /> | ||
| <PackageReference Include="System.Security.Cryptography.Pkcs" Version="$(SystemSecurityCryptographyPkcsVersion)" /> | ||
| <PackageReference Include="Microsoft.Bcl.Cryptography" Version="$(MicrosoftBclCryptographyVersion)" /> |
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 supposed to provide the System.Security.Cryptography.X509Certificates.X509CertificateLoader type, which became required in .NET 9. But because we ended up only making the change conditionally for .NET 9, Framework code was unchanged and didn't need this type: https://github.com/dotnet/SqlClient/blob/main/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/VirtualSecureModeEnclaveProvider.cs#L172
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.
Yes, saw the #if NET_9_0_Or_Greater conditionals and was able to remove this package without incident.
| <MicrosoftExtensionsHosting>8.0.1</MicrosoftExtensionsHosting> | ||
| <MicrosoftNETFrameworkReferenceAssembliesVersion>1.0.3</MicrosoftNETFrameworkReferenceAssembliesVersion> | ||
| <MicrosoftNETTestSdkVersion>17.11.1</MicrosoftNETTestSdkVersion> | ||
| <MicrosoftNETTestSdkVersion>17.12.0</MicrosoftNETTestSdkVersion> |
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.
👀 note to self to double check that tests still run
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.
Please do - I eyeballed a bunch of the jobs, and tests are running. I didn't do an exhaustive comparison with a previous run.
| <dependency id="Microsoft.IdentityModel.JsonWebTokens" version="7.7.1" /> | ||
| <dependency id="Microsoft.IdentityModel.Protocols.OpenIdConnect" version="7.7.1" /> | ||
| <dependency id="System.Buffers" version="4.6.1" /> | ||
| <dependency id="System.Security.Cryptography.Pkcs" version="8.0.1"/> |
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 get System.Security.Cryptography.Pkcs for free in net462: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.pkcs?view=netframework-4.6.2
- Cleaned up MSS project and package refs.
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.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Description
Details
MDS
AKV
Issues
Resolves #3808.
Testing