Fix SPN using instance name instead of resolved port for Protocol.None and Protocol.Admin

[paulmedynski]

Fix

Fixes #3566

Problem

When connecting to a named instance without a tcp: prefix (e.g. Data Source=server\instance), the DataSource.ResolvedProtocol is set to Protocol.None rather than Protocol.TCP. The SPN generation in GetSqlServerSPNs only checked for Protocol.TCP when deciding whether to use the SSRP-resolved port number, so Protocol.None (and Protocol.Admin) connections incorrectly used the instance name in the SPN:

MSSQLSvc/server.fqdn:instancename   (incorrect)

instead of:

MSSQLSvc/server.fqdn:12345          (correct)

This is the same class of bug as #2187. The original fix addressed Protocol.TCP but missed Protocol.None, which is the most common case (no protocol prefix in the connection string).

Fix

Inverted the condition in GetSqlServerSPNs from checking == Protocol.TCP (use port) to checking == Protocol.NP (use instance name). Named Pipes is the only protocol that should use the instance name in the SPN. All TCP-like protocols (TCP, None, Admin) now correctly use the SSRP-resolved port.

Testing

• Added 5 unit tests in SniProxyGetSqlServerSPNsTest covering Protocol.None, Protocol.TCP, Protocol.Admin, custom SPN passthrough, and Named Pipes
• Confirmed the Protocol.None and Protocol.Admin tests fail before the fix and pass after
• Full unit test suite passes (649/650, 1 pre-existing unrelated failure in IntegratedAuthConnectionTest)

Changes

• SniProxy.netcore.cs: Changed visibility of GetSqlServerSPNs(DataSource, string) from private to internal for testability; inverted protocol check from == TCP to == NP
• SniProxyGetSqlServerSPNsTest.cs: New unit test class

[Copilot]

Pull request overview

Fixes SPN generation for named-instance connections when the connection string omits a protocol prefix (Protocol.None) or uses DAC (Protocol.Admin), ensuring TCP-like protocols use the SSRP-resolved port rather than the instance name in the SPN.

Changes:

• Updated SniProxy.GetSqlServerSPNs(DataSource, string) to use the instance name only for Named Pipes (NP); all other protocols use the SSRP-resolved port.
• Changed GetSqlServerSPNs(DataSource, string) visibility from private to internal to enable direct unit testing.
• Added unit tests covering Protocol.None/TCP/Admin and custom SPN passthrough behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniProxy.netcore.cs	Adjusts SPN postfix selection logic so TCP-like protocols use SSRP-resolved port; exposes method for unit tests.
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SniProxyGetSqlServerSPNsTest.cs	Adds regression tests for SPN formatting across protocols and custom SPN passthrough.

Comments suppressed due to low confidence (1)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniProxy.netcore.cs:144

• The trace event logs dataSource.Port but the postfix/SPN decision for named instances now depends on dataSource.ResolvedPort. Consider logging ResolvedPort as well (or instead) so diagnostics reflect the value actually used to build the SPN.

                postfix = dataSource.ResolvedProtocol == DataSource.Protocol.NP ? dataSource.InstanceName : dataSource.ResolvedPort.ToString();
            }

            SqlClientEventSource.Log.TryTraceEvent("SNIProxy.GetSqlServerSPN | Info | ServerName {0}, InstanceName {1}, Port {2}, postfix {3}", dataSource?.ServerName, dataSource?.InstanceName, dataSource?.Port, postfix);
            return GetSqlServerSPNs(hostName, postfix, dataSource.ResolvedProtocol);

[Copilot]

This [Fact] test method is empty (only comments) so it will always pass without validating Named Pipes SPN behavior. Either implement a real assertion (e.g., parse an np:server\\instance data source and assert the generated SPN uses :instance), or remove the test if it can’t be expressed through the current API surface.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.04%. Comparing base (52c7149) to head (f027453).
⚠️ Report is 5 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (52c7149) and HEAD (f027453). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (52c7149)	HEAD (f027453)
CI-SqlClient	1	0

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4180      +/-   ##
==========================================
- Coverage   74.27%   65.04%   -9.23%     
==========================================
  Files         279      274       -5     
  Lines       42980    65799   +22819     
==========================================
+ Hits        31922    42800   +10878     
- Misses      11058    22999   +11941     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	65.04% <100.00%> (?)

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.