Change | Remove some unneeded references and update Azure.Identity

[ErikEJ]

fixes #2048
fixes #2574
fixes #2568

removes explicit ref to Microsoft.Identity.Client

[ErikEJ]

@JRahnama FYI

[David-Engel]

1. Why the removal of the reference to Microsoft.Extensions.Caching.Memory?
2. We should keep the reference to Microsoft.Identity.Client as we have a direct reference to it in the ActiveDirectoryAuthenticationProvider..
3. I'm fine with the reference change to Azure.Identity in the test projects.

[ErikEJ]

@David-Engel

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]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.56%. Comparing base (55f48c5) to head (cf29a2a).

❗ Current head cf29a2a differs from pull request most recent head 91030e5

Please upload reports for the commit 91030e5 to get more accurate results.

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     

Flag	Coverage Δ
addons	92.88% <ø> (ø)
netcore	76.45% <ø> (+0.01%)	⬆️
netfx	70.44% <ø> (-0.05%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[David-Engel]

re 2: Azure.Identity references Microsoft.Identity.Client, so you do not need the explicit reference

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.

[ErikEJ]

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.

[cheenamalhotra]

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.

[Ruud-cb]

There is a vulnerability detected in [email protected], any chance this will also be patched in Microsoft.Data.SqlClient/5.1.x?

                    "VulnerabilityID": "CVE-2024-38095",
                    "PkgName": "System.Formats.Asn1",
                    "PkgIdentifier": {
                        "PURL": "pkg:nuget/[email protected]",
                        "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]

@Ruud-cb what patch version are you proposing? See #2048

[Ruud-cb]

@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 System.Security.Cryptography.Cng is not needed then it should be removed in the current version so that the above vulnerability is resolved as well if I'm not mistaken..

[ErikEJ]

@Ruud-cb They are only un-used in 6.0, not in 5.0 (due to removal of .NET Standard target)

[Ruud-cb]

@ErikEJ Strange, my app.deps.json after build in .NET 8 refers to sql client 5.1.5 that uses System.Security.Cryptography.Cng

      "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 Microsoft.EntityFrameworkCore.* 8.0.7 is not using that version yet? Not sure if the package is removed in5.2.*?

[ErikEJ]

@Ruud-cb The package is only removed in Microsoft.Data.SqlClient 6.0, and it cannot be removed in Microsoft.Data.SqlClient 5.x