Skip to content

Add Microsoft.Data.SqlClient.Extensions.Azure - #8260

Open
MGibson1 wants to merge 1 commit into
mainfrom
arch/allow-azure-passwordless-db-auth
Open

Add Microsoft.Data.SqlClient.Extensions.Azure#8260
MGibson1 wants to merge 1 commit into
mainfrom
arch/allow-azure-passwordless-db-auth

Conversation

@MGibson1

@MGibson1 MGibson1 commented Aug 26, 2026

Copy link
Copy Markdown
Member

🎟️ 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

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
@MGibson1 MGibson1 added the t:deps Change Type - Dependencies label Aug 26, 2026
@MGibson1
MGibson1 requested a review from a team August 26, 2026 14:46
@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE (pending AppSec sign-off on the new dependency)

This is a dependency-only change: Microsoft.Data.SqlClient moves 7.0.0 → 7.0.2 and the net-new Microsoft.Data.SqlClient.Extensions.Azure 7.0.2 is added to src/Core/Core.csproj, restoring the Entra/passwordless auth capability that SqlClient 7.0 extracted out of the base package. Lock file hygiene is clean — the single manifest change is reflected in all 60 packages.lock.json files, every resolved version moves forward with no downgrades, and CI (build, tests, Docker images, validation) is green. All added and bumped packages are Microsoft first-party and permissively licensed; the notable graph additions are Microsoft.Identity.Client.Broker 4.84.2 and Microsoft.Identity.Client.NativeInterop 0.20.6, which are new to every project including self-hosted artifacts. No application code changes accompany the reference, so the provider is expected to be selected via connection string configuration at deploy time.

Code Review Details
  • ⚠️ : New dependency Microsoft.Data.SqlClient.Extensions.Azure requires Dependency Review and Approval before merge. Review has been requested from @bitwarden/team-appsec in a PR comment but is not yet recorded as granted; useful detail for that review is that the package also pulls in MSAL broker and its native interop assembly.
    • src/Core/Core.csproj:52
  • ❓ : Package referenced from Core rather than scoped to the SeederAPI deployment that needs it
    • src/Core/Core.csproj:52

Dependency Changes

Package Change Ecosystem
Microsoft.Data.SqlClient 7.0.0 → 7.0.2 NuGet
Microsoft.Data.SqlClient.Extensions.Azure New (7.0.2) NuGet

Notable transitive movement: Azure.Identity 1.11.4 → 1.18.0, Azure.Core 1.47.3 → 1.51.1, Microsoft.Identity.Client 4.66.1 → 4.84.2, plus new Microsoft.Identity.Client.Broker 4.84.2 and Microsoft.Identity.Client.NativeInterop 0.20.6. No direct Azure.Identity API usage exists in the codebase, so the credential-chain behavior changes across those versions are not expected to affect runtime behavior.

@codecov

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.93%. Comparing base (571f880) to head (ea331f1).
⚠️ Report is 4 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@withinfocus withinfocus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow -- why is this needed?

@MGibson1

MGibson1 commented Aug 26, 2026

Copy link
Copy Markdown
Member Author

@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.

@MGibson1
MGibson1 marked this pull request as draft August 26, 2026 18:23
@MGibson1
MGibson1 marked this pull request as ready for review August 26, 2026 20:11
@MGibson1
MGibson1 requested a review from a team August 26, 2026 20:11
@MGibson1

Copy link
Copy Markdown
Member Author

@bitwarden/team-appsec This is a new authentication method SRE wants to use with the SeederAPI. Can you review the dependency?

Comment thread src/Core/Core.csproj
<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]" />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@withinfocus

Copy link
Copy Markdown
Contributor

AppSec dependency review of Microsoft.Data.SqlClient.Extensions.Azure 7.0.2.

Provenance

MIT, authored and owned by Microsoft (verified prefix, co-owner nugetsqltools), NuGet-signed, built from dotnet/SqlClient at commit 8c70cec. ~5.5M downloads.

It isn't really new code. These are the Entra SqlAuthenticationProvider implementations that lived inside Microsoft.Data.SqlClient through 6.x and got split out in 7.0. Managed assemblies only (net462, netstandard2.0).

Vulnerability posture

No open GitHub advisories against any version this change resolves to. Every transitive move goes forward, and both Azure.Identity (1.11.4 to 1.18.0) and Microsoft.Identity.Client (4.66.1 to 4.84.2) were already above the GHSA-m5vv-6r4h-3vj9 floor, so this is currency rather than a fix.

There's no direct Azure.Identity or MSAL usage anywhere in the repo, so nothing changes at runtime today; the auth mode gets picked by connection string at deploy time. No zero-knowledge impact either, since this is database connection authentication and doesn't touch vault data, key handling, or encryption.

@withinfocus withinfocus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t:deps Change Type - Dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants