Add Microsoft.Data.SqlClient.Extensions.Azure - #8260
Conversation
Per https://learn.microsoft.com/en-us/sql/connect/ado-net/introduction-microsoft-data-sqlclient-namespace?view=sql-server-ver17#breaking-changes-in-70 Microsoft.Data.SqlClient 7.0 removed Azure.Core and Azure.Identity dependencies and extracted the necessary capabilities to the newly referenced package
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE (pending AppSec sign-off on the new dependency) This is a dependency-only change: Code Review Details
Dependency Changes
Notable transitive movement: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8260 +/- ##
==========================================
+ Coverage 63.23% 68.93% +5.69%
==========================================
Files 2410 2410
Lines 104463 104463
Branches 9458 9458
==========================================
+ Hits 66059 72007 +5948
+ Misses 36150 30072 -6078
- Partials 2254 2384 +130 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
withinfocus
left a comment
There was a problem hiding this comment.
Not sure I follow -- why is this needed?
|
@withinfocus. It's because of the authentication choices made to deploy the SeederAPI. It's been deployed with passwordless authentication to Azure, which was removed from the base SqlClient package in 7.0 Looks like it trashes our integration test setup, though, so I'll have to look into that. |
|
@bitwarden/team-appsec This is a new authentication method SRE wants to use with the SeederAPI. Can you review the dependency? |
| <PackageReference Include="Microsoft.Bot.Connector" Version="[4.23.0]" /> | ||
| <PackageReference Include="Microsoft.Data.SqlClient" Version="[7.0.0]" /> | ||
| <PackageReference Include="Microsoft.Data.SqlClient" Version="[7.0.2]" /> | ||
| <PackageReference Include="Microsoft.Data.SqlClient.Extensions.Azure" Version="[7.0.2]" /> |
There was a problem hiding this comment.
❓ QUESTION: Referencing this from Core adds the MSAL broker + native interop to every service image — is SeederApi the only consumer?
Details
Every project references Core, so this addition lands in all 60 lock files, including self-hosted artifacts (util/Setup, util/MsSqlMigratorUtility). Azure.Core/Azure.Identity were already in the graph, but Microsoft.Identity.Client.Broker 4.84.2 and Microsoft.Identity.Client.NativeInterop 0.20.6 are new everywhere — the latter carries native runtime binaries for interactive broker auth, which no Linux server workload will use.
If the passwordless connection is only for the SeederAPI deployment, a PackageReference in util/SeederApi/SeederApi.csproj would keep the extension assembly in that app's output without widening what the other images ship (util/Seeder/Seeder.csproj already scopes Bogus this way).
If other cloud services are expected to move to Entra SQL auth, keeping it in Core is the right call — just confirming that's the intent, since it also affects the scope AppSec is being asked to approve.
|
AppSec dependency review of ProvenanceMIT, authored and owned by It isn't really new code. These are the Entra Vulnerability postureNo open GitHub advisories against any version this change resolves to. Every transitive move goes forward, and both There's no direct |
withinfocus
left a comment
There was a problem hiding this comment.
60 MB of unreachable native code in every image we ship
Extensions.Azure pulls Microsoft.Identity.Client.Broker 4.84.2, which pulls Microsoft.Identity.Client.NativeInterop 0.20.6, which carries msalruntime native binaries for win-x86/x64/arm64, linux-x64, and osx-x64/arm64.
I did a scratch dotnet publish against 7.0.2 to check, and all of them land in the publish output: 60 MB under runtimes/, including a 38 MB libmsalruntime.so.
Broker (WAM) auth is a Windows interactive-desktop flow, so nothing in a Linux container can reach it. That means unreachable native code in every image in the build.yml matrix, self-hosted included. Can you try trimming the native assets (ExcludeAssets, or a direct PackageReference override on NativeInterop) and confirm the Entra path still works? If it can't be trimmed cleanly, an upstream issue asking why Broker is a hard dependency seems worth filing.
Scope the reference to SeederApi
It's on src/Core/Core.csproj:52 right now, so all 60+ projects inherit it and it ends up in Api, Identity, Admin, Sso, Scim and the self-hosted images, when SeederApi is the only thing that needs it. Please move it to util/SeederApi/SeederApi.csproj.
Combined with the point above, that's the difference between one image carrying the payload and everything we ship carrying it.
Which credential is SRE actually using?
Authentication=Active Directory Default resolves to DefaultAzureCredential, which is a fallback chain: environment variables (AZURE_CLIENT_ID/AZURE_CLIENT_SECRET), workload identity, IMDS at 169.254.169.254, Azure CLI, and so on. Whichever link answers first becomes the identity.
I'd rather we pin the explicit keyword (Active Directory Managed Identity, or workload identity) instead of Default, so a stray environment variable or a leftover developer credential can't quietly become the identity we authenticate to the database with.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-42570
📔 Objective
Microsoft.Data.SqlClient 7.0 removed Azure.Core and Azure.Identity dependencies and extracted the necessary capabilities to the newly referenced package