-
Notifications
You must be signed in to change notification settings - Fork 281
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
Change | Remove some unneeded references and update Azure.Identity #2577
Conversation
fixes dotnet#2048 removes explicit ref to Microsoft.Identity.Client
@JRahnama FYI |
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.
- Why the removal of the reference to Microsoft.Extensions.Caching.Memory?
- We should keep the reference to Microsoft.Identity.Client as we have a direct reference to it in the ActiveDirectoryAuthenticationProvider..
- I'm fine with the reference change to Azure.Identity in the test projects.
re 1: Does not seem to be needed in the Ref projects !? re 2: Azure.Identity references Microsoft.Identity.Client, so you do not need the explicit reference |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2577 +/- ##
==========================================
- Coverage 72.58% 72.56% -0.03%
==========================================
Files 310 310
Lines 61688 61688
==========================================
- Hits 44779 44765 -14
- Misses 16909 16923 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I realize we have an indirect reference through Azure.Identity (AI). But that seems conceptually wrong when we directly reference Microsoft.Identity.Client (MIC) in the code. The way you are proposing makes if feel like we are saying that consumers of MDS only need MIC because AI needs MIC. I've always favored defining direct dependencies explicitly, but I couldn't find much in the way of documented "best practice" here. My preference isn't a strong one since it works fine either way. I admit, it's cleaner your proposed way. |
Thanks! Also, even if MDS wants to reference a specific version of MIC, the version that AI uses will always win if it is newer, and if MIC version is newer than the one used by AI, you might put yourself in an unsupported scenario. |
I would be ok with removing MIC since we'll never remove AI blindly without considering adding MIC again if it's removed (as we won't be able to compile anyways). In Java world, adding dependencies explicitly for APIs used is required, but that doesn't seem to be in .NET. |
There is a vulnerability detected in System.Formats.Asn1@5.0.0, any chance this will also be patched in "VulnerabilityID": "CVE-2024-38095",
"PkgName": "System.Formats.Asn1",
"PkgIdentifier": {
"PURL": "pkg:nuget/System.Formats.Asn1@5.0.0",
"UID": "c4caf253e24636e4"
},
"InstalledVersion": "5.0.0",
"FixedVersion": "6.0.1, 8.0.1",
"Status": "fixed",
"Layer": {
"DiffID": "sha256:3357da3fec4c9d45210b22e1b208983a625130f26f37192f121048132be4d097"
},
"SeveritySource": "ghsa",
"PrimaryURL": "https://avd.aquasec.com/nvd/cve-2024-38095",
"DataSource": {
"ID": "ghsa",
"Name": "GitHub Security Advisory NuGet",
"URL": "https://github.com/advisories?query=type%3Areviewed+ecosystem%3Anuget"
},
"Title": "dotnet: DoS when parsing X.509 Content and ObjectIdentifiers",
"Description": ".NET and Visual Studio Denial of Service Vulnerability",
"Severity": "HIGH",
"CweIDs": [
"CWE-20"
], |
@ErikEJ coming from #2048 as well, as per #2576 it is a unused reference, (also in the current version 5.2.1 is perhaps the question), so if |
@Ruud-cb They are only un-used in 6.0, not in 5.0 (due to removal of .NET Standard target) |
@ErikEJ Strange, my "Microsoft.Data.SqlClient/5.1.5": {
"dependencies": {
"Azure.Identity": "1.10.3",
"Microsoft.Data.SqlClient.SNI.runtime": "5.1.1",
"Microsoft.Identity.Client": "4.56.0",
"Microsoft.IdentityModel.JsonWebTokens": "7.7.1",
"Microsoft.IdentityModel.Protocols.OpenIdConnect": "7.7.1",
"Microsoft.SqlServer.Server": "1.0.0",
"System.Configuration.ConfigurationManager": "6.0.1",
"System.Diagnostics.DiagnosticSource": "8.0.0",
"System.Runtime.Caching": "6.0.0",
"System.Security.Cryptography.Cng": "5.0.0",
"System.Security.Principal.Windows": "5.0.0",
"System.Text.Encoding.CodePages": "6.0.0",
"System.Text.Encodings.Web": "8.0.0"
},
},
...
"System.Security.Cryptography.Cng/5.0.0": {
"dependencies": {
"System.Formats.Asn1": "5.0.0"
}
}, Seems that the project is not using the latest 5.2.1 version, I don't reference the package directly so I guess |
@Ruud-cb The package is only removed in Microsoft.Data.SqlClient 6.0, and it cannot be removed in Microsoft.Data.SqlClient 5.x |
* Updating Azure.Identity version to 1.11.3 (dotnet#2526) * Fix | Clone of SqlConnection should include AccessTokenCallback (dotnet#2525) * Enhancement | Add trace logs for packet size (dotnet#2522) * Merged PR 4583: eng | Fix policheck errors. Fix policheck errors. Sample pipeline run which did not have policheck errors: https://sqlclientdrivers.visualstudio.com/ADO.Net/_build/results?buildId=88114&view=sariftools.scans.build-tab Related work items: #30279 * Doc | Fix SNI dependencies of 5.1 and 5.2 release notes (dotnet#2537) * Change | Separate tests for NetFx and NetCore - NetFx-Only Connection String Properties (dotnet#2466) * Adding TransparentNetworkIpResolution to list of unsupported on platform connection string error messages Splitting unit test for netfx-only connection string properties such that test does not fail on netcore * Remove DeprecatedSynonymCount since referencing the unsupported array is not possible * Fix | Enhance certificate validation (dotnet#2487) * Hotfix v5.2.1 Release notes (dotnet#2534) * Improve AccessTokenCallback sample code (dotnet#2543) * Merged PR 4621: eng | Fix policheck * Fix | Adjust path for .AssemblyAttributes in obj folder (dotnet#2550) * Fix | Fixed GenerateSspiClientContext to retry negotiation with default port (dotnet#2559) * Strong typed diagnostics (dotnet#2226) * Fix | Replaced System.Runtime.Caching with Microsoft.Extensions.Caching.Memory (dotnet#2493) * Add | Add SourceLink translation (dotnet#2552) * Add | Cache TokenCredential objects to take advantage of token caching (dotnet#2380) * Merged common code base for SqlUtil.cs (dotnet#2533) * Add scope trace for GenerateSspiClientContext (dotnet#2497) * Address conflicts (dotnet#2562) * Addressing conflict (dotnet#2560) * Merge SqlColumnEncryptionCertificateStoreProvider (dotnet#2521) * Add | No-op if engineedition is 6 or 11 due to lack of support for ASSEMBLYPROPERTY function (dotnet#2593) * Change | Remove some unneeded references and update Azure.Identity (dotnet#2577) * Add test for issue 2456 (dotnet#2457) * Merged common code base for AlwaysEncryptedKeyConverter (dotnet#2538) * Merged AlwaysEncryptedKeyConverter.CrossPlatform and AlwaysEncryptedKeyConverter.Cng. * 3 Small Changes (dotnet#2594) * * Port sqlclientx datasource changes * Remove link to missing nuget.config file * Remove root namespaces from sqlclient csproj files * Test to see if namespace changes are breaking the pr build * Reinstate removing the root namespace and fix resource filename generation * Test fixes to accommodate recent infra changes (dotnet#2646) * Test fixes to accomodate recent infra changes * Fix - Don't error when using infinte connect timeout and Entra auth (dotnet#2651) * eng | Add delay signed to official builds (dotnet#2653) * eng | Initial YAML CI pipeline (dotnet#2575) * Fix | Fix decrypt failure to drain data (dotnet#2618) * [Scheduled Run] Localized resource files from OneLocBuild * eng | Add Delay sign to ref csprojs (dotnet#2684) * [Scheduled Run] Localized resource files from OneLocBuild * [Scheduled Run] Localized resource files from OneLocBuild --------- Co-authored-by: Javad Rahnama <v-jarahn@microsoft.com> Co-authored-by: David Engel <v-davidengel@microsoft.com> Co-authored-by: Aris Rellegue <v-arellegue@microsoft.com> Co-authored-by: DavoudEshtehari <61173489+DavoudEshtehari@users.noreply.github.com> Co-authored-by: Benjamin Russell <russellben@microsoft.com> Co-authored-by: Aris Rellegue <134557572+arellegue@users.noreply.github.com> Co-authored-by: dauinsight <145612907+dauinsight@users.noreply.github.com> Co-authored-by: Scott Addie <10702007+scottaddie@users.noreply.github.com> Co-authored-by: Daniel Au <v-audaniel@microsoft.com> Co-authored-by: Wraith <wraith2@gmail.com> Co-authored-by: SqlClient Azure DevOps <sqlclient@microsoft.com> Co-authored-by: Edward Neal <55035479+edwardneal@users.noreply.github.com> Co-authored-by: Erik Ejlskov Jensen <ErikEJ@users.noreply.github.com> Co-authored-by: David Engel <davidengel@microsoft.com>
fixes #2048
fixes #2574
fixes #2568
removes explicit ref to Microsoft.Identity.Client