Expose SSPI context provider as public #2494

twsouthwick · 2024-05-08T16:24:42Z

This change:

Adds a property to SqlConnection to allow setting a provider
Plumbs that property into the TdsParser so that it can be used if set

codecov · 2025-03-03T22:01:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.06%. Comparing base (63443f4) to head (ee6c4cf).

❗ There is a different number of reports uploaded between BASE (63443f4) and HEAD (ee6c4cf). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (63443f4) HEAD (ee6c4cf)

addons 1 0

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2494       +/-   ##
===========================================
- Coverage   69.90%   59.06%   -10.84%     
===========================================
  Files         276      270        -6     
  Lines       62414    62098      -316     
===========================================
- Hits        43629    36677     -6952     
- Misses      18785    25421     +6636

Flag	Coverage Δ
addons	`?`
netcore	`61.71% <ø> (-11.13%)`	⬇️
netfx	`62.71% <ø> (-6.91%)`	⬇️

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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

src/Microsoft.Data.SqlClient/netfx/ref/Microsoft.Data.SqlClient.cs

src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.cs

benrr101

Overall, really like the goal and implementation. Most of my comments are style related, since I'm sure my colleagues have tackled the bigger questions.

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/SSPIContextProvider.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/NativeSSPIContextProvider.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/SSPIContextProvider.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionPoolKey.cs

paulmedynski

Made an initial pass, and came up with some questions/comments.

doc/snippets/Microsoft.Data.SqlClient/SSPIContextProvider.xml

doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml

src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/NegotiateSSPIContextProvider.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionPoolKey.cs

paulmedynski

Factory vs instance in the netfx code, and a few questions about docs.

doc/snippets/Microsoft.Data.SqlClient/SspiAuthenticationParameters.xml

src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.cs

src/Microsoft.Data.SqlClient/netfx/ref/Microsoft.Data.SqlClient.cs

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

nhart12

Latest package still works for our use-case

twsouthwick · 2025-05-19T17:17:52Z

ping @mdaigle @paulmedynski @benrr101 @David-Engel

mdaigle

A few small things I'd like to see addressed. I will also leave this blocked until we complete a security review. We need to do one soon for our upcoming releases anyway.

doc/snippets/Microsoft.Data.SqlClient/SspiContextProvider.xml

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs

mdaigle · 2025-05-20T21:36:17Z

...SqlClient/tests/ManualTests/SQL/IntegratedAuthenticationTest/IntegratedAuthenticationTest.cs

+            {
+                conn.Open();
+
+                Assert.Fail("Expected to use custom SSPI context provider");


It would be nice to find a way to test the positive case, so that we know we can successfully connect via a custom context provider.

Yeah, I struggled to figure out how to test it even for this. The main problem with a positive case is that the implementation is wildly different between frameworks so testing it starts being difficult. My thought here was to ensure it gets plumbed through, and the existing tests ensure that once it's being called that it works correctly.

If you have ideas here, please let me know how to proceed.

paulmedynski · 2025-07-04T11:49:32Z

@twsouthwick - Need to resolve conflicts here, but I know @benrr101 is also working on merging the .NET and .NET Framework instances of this class, so please coordinate with him.

paulmedynski · 2025-07-09T09:21:23Z

/azp run

azure-pipelines · 2025-07-09T09:21:45Z

Azure Pipelines successfully started running 2 pipeline(s).

twsouthwick · 2025-07-16T15:11:27Z

@paulmedynski @mdaigle the two failures look to be test failures with sql server setup. otherwise it's ready again

twsouthwick · 2025-07-21T18:44:52Z

@mdaigle @paulmedynski any updates on timing here? Are we still waiting for a security review?

mdaigle · 2025-07-23T16:52:01Z

@twsouthwick yes, we still need to set up a security review. We had originally planned on doing that for the 6.1 release but opted to avoid security sensitive changes. I'm hoping I'll have a chance soon to update out threat model diagrams for this feature and then schedule a feature specific review.

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/SspiContextProvider.cs

twsouthwick · 2025-07-23T21:41:45Z

@mdaigle I've created a separate PR to make the non-public changes so that future merge conflicts are minimal: #3511

twsouthwick mentioned this pull request May 8, 2024

Add hook for custom SSPI context for SqlConnection instances #2253

Open

10 tasks

twsouthwick changed the title ~~Expose SSPI context provider as public~~ Expose SSPI context provider as public May 8, 2024

twsouthwick force-pushed the public branch from 2657817 to fbfc339 Compare February 1, 2025 01:10

twsouthwick force-pushed the public branch 3 times, most recently from e16f2c8 to cfcf164 Compare March 3, 2025 19:42

nhart12 reviewed Mar 11, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/netfx/ref/Microsoft.Data.SqlClient.cs Outdated Show resolved Hide resolved

nhart12 reviewed Mar 11, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.cs Outdated Show resolved Hide resolved

benrr101 requested changes Apr 2, 2025

View reviewed changes

paulmedynski requested changes Apr 2, 2025

View reviewed changes

paulmedynski self-assigned this Apr 2, 2025

twsouthwick force-pushed the public branch 6 times, most recently from 380e896 to 2039bd9 Compare May 1, 2025 19:37

twsouthwick force-pushed the public branch from 804e5ba to 9a5fd7c Compare May 13, 2025 00:36

paulmedynski requested changes May 13, 2025

View reviewed changes

twsouthwick force-pushed the public branch 4 times, most recently from 5bb128f to 8937cb7 Compare May 13, 2025 14:58

twsouthwick marked this pull request as ready for review May 13, 2025 18:40

twsouthwick requested review from paulmedynski, benrr101, David-Engel and nhart12 May 13, 2025 18:40

nhart12 approved these changes May 13, 2025

View reviewed changes

0xced mentioned this pull request May 14, 2025

Add NTLM authentication to SqlClient #31

Closed

twsouthwick requested a review from a team as a code owner May 19, 2025 16:01

mdaigle requested changes May 20, 2025

View reviewed changes

twsouthwick requested a review from mdaigle May 21, 2025 19:55

paulmedynski added this to the 7.0-preview1 milestone Jul 4, 2025

paulmedynski previously approved these changes Jul 4, 2025

View reviewed changes

twsouthwick dismissed paulmedynski’s stale review via 7a69729 July 8, 2025 20:28

paulmedynski previously approved these changes Jul 9, 2025

View reviewed changes

Expose SSPI context provider as public

d5621d5

twsouthwick dismissed paulmedynski’s stale review via d5621d5 July 15, 2025 16:44

twsouthwick force-pushed the public branch from 7a69729 to d5621d5 Compare July 15, 2025 16:44

fix merge issue

e894bb1

Merge remote-tracking branch 'origin/main' into public

c678bc6

mdaigle previously approved these changes Jul 23, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/SspiContextProvider.cs Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/main' into public

ee6c4cf

twsouthwick dismissed mdaigle’s stale review via ee6c4cf July 24, 2025 18:04

Expose SSPI context provider as public #2494

Are you sure you want to change the base?

Expose SSPI context provider as public #2494

Conversation

twsouthwick commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

benrr101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nhart12 left a comment

Choose a reason for hiding this comment

Uh oh!

twsouthwick commented May 19, 2025

Uh oh!

mdaigle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdaigle May 20, 2025

Choose a reason for hiding this comment

Uh oh!

twsouthwick May 21, 2025

Choose a reason for hiding this comment

Uh oh!

paulmedynski commented Jul 4, 2025

Uh oh!

paulmedynski commented Jul 9, 2025

Uh oh!

azure-pipelines bot commented Jul 9, 2025

Uh oh!

twsouthwick commented Jul 16, 2025

Uh oh!

twsouthwick commented Jul 21, 2025

Uh oh!

mdaigle commented Jul 23, 2025

Uh oh!

Uh oh!

twsouthwick commented Jul 23, 2025

Uh oh!

Uh oh!

twsouthwick commented May 8, 2024 •

edited

Loading

codecov bot commented Mar 3, 2025 •

edited

Loading