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 (for example, Data Source=server\instance), DataSource.ResolvedProtocol is Protocol.None. SPN generation only used resolved port for Protocol.TCP, so Protocol.None and Protocol.Admin could incorrectly use instance name in the SPN:

MSSQLSvc/server.fqdn:instancename

instead of using the SSRP-resolved port:

MSSQLSvc/server.fqdn:12345

Why this addresses the review concern

@cheenamalhotra correctly noted that port and instance name can both map to the same SQL Server instance. This change does not claim they are always semantically different targets.

The driver behavior here is protocol-specific for SPN construction:

• Named Pipes (NP): keep instance-name postfix
• TCP-like paths (TCP, None, Admin): use port postfix (resolved via SSRP for named instances)

This gives deterministic SPN formatting for integrated auth on Unix/Linux SSRP flows while preserving NP behavior and explicit ServerSPN overrides.

Code changes

• SniProxy.netcore.cs
  • SPN postfix selection now uses instance name only for Protocol.NP
  • Protocol.TCP, Protocol.None, and Protocol.Admin use ResolvedPort
  • Added resolved protocol/port in trace logging to improve diagnostics
• SniProxyGetSqlServerSPNsTest.cs
  • Recreated and finalized regression tests for:
    • Protocol.None uses resolved port
    • Protocol.TCP uses resolved port
    • Protocol.Admin uses resolved port
    • Protocol.NP uses instance name
    • Custom ServerSPN passthrough

Testing performed

• Targeted SPN unit tests:
  • dotnet test src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj --filter FullyQualifiedName~SniProxyGetSqlServerSPNsTest
  • Result: Passed on net8.0, net9.0, net10.0 (15/15)
• Kerberos-adjacent SSPI unit coverage:
  • dotnet test src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj --filter FullyQualifiedName~SspiTests
  • Result: Passed on net8.0, net9.0, net10.0 (15/15)

Kerberos environment validation status

I will run our existing suite of Kerberos tests and provide results here.

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

[cheenamalhotra]

Not necessarily, both Port and Instance Name can be used interchangeably. Refer to Microsoft Docs They both point to the same SQL Server instance.

Protocol.None points the driver to use Shared Memory protocol when establishing connection - which is supported on Windows, but not on Unix.

[cheenamalhotra]

Has this been tested in Kerberos test suite to verify it doesn't cause any regression in any environment?

[cheenamalhotra]

Plz confirm if testing has been performed on Kerberos environment.

[Copilot]

Copilot wasn't able to review any files in this pull request.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

[Copilot]

These Kerberos manual tests build a new connection string from scratch, which drops important settings present in DataTestUtility.TCPConnectionString (e.g., Initial Catalog, Encrypt/TrustServerCertificate, timeouts, etc.). This can cause the test to fail for reasons unrelated to SPN handling. Consider starting from a SqlConnectionStringBuilder(tcpConnStr), then only overriding DataSource and IntegratedSecurity so the rest of the environment configuration is preserved.

Suggested change

	string protocolNoneConnStr = $"Data Source={hostname}\\{instanceName};Integrated Security=true;";
	SqlConnectionStringBuilder protocolNoneConnStrBuilder = new(tcpConnStr)
	{
	DataSource = $"{hostname}\\{instanceName}",
	IntegratedSecurity = true
	};
	string protocolNoneConnStr = protocolNoneConnStrBuilder.ConnectionString;

[Copilot]

This test claims to rely on SSRP when needed, but it always appends a port (1433 when none is present). Adding an explicit ",1433" disables SSRP resolution and will likely connect to the wrong endpoint for a named instance. Also, if instanceName is empty (default instance), the interpolated Data Source becomes "tcp:host\,port" (trailing backslash) which is invalid. Consider requiring a non-empty instanceName here and omitting the port entirely when it isn't explicitly known, so SSRP can resolve it.

Suggested change

	out string hostname, out int port, out string instanceName))
	{
	return; // Skip test
	}
	KerberosTicketManagemnt.Init(DataTestUtility.KerberosDomainUser, DataTestUtility.KerberosDomainPassword);
	// If an explicit port is available in the test connection string, use it
	// Otherwise, use a typical SQL instance port (1433) and rely on SSRP if needed
	int testPort = port > 0 ? port : 1433;
	string protocolTcpConnStr = $"Data Source=tcp:{hostname}\\{instanceName},{testPort};Integrated Security=true;";
	out string hostname, out int port, out string instanceName) ||
	string.IsNullOrEmpty(instanceName))
	{
	return; // Skip test
	}
	KerberosTicketManagemnt.Init(DataTestUtility.KerberosDomainUser, DataTestUtility.KerberosDomainPassword);
	// Preserve SSRP resolution for named instances when the original test connection string
	// does not specify an explicit port.
	string protocolTcpConnStr = port > 0
	? $"Data Source=tcp:{hostname}\\{instanceName},{port};Integrated Security=true;"
	: $"Data Source=tcp:{hostname}\\{instanceName};Integrated Security=true;";

[Copilot]

The custom ServerSPN constructed here appends the instance name when instanceName is present. For TCP-like connections to named instances, the expected SPN format is typically hostname:port (matching the behavior this PR is enforcing). Using hostname:instancename is likely to be invalid in environments that only register port-based SPNs, making this test brittle. Consider resolving/using the actual port (explicit or SSRP-resolved) when building customSpn, or limit this test to scenarios where a known-valid SPN can be provided/configured.

Suggested change

	// Build the expected SPN for the server
	string fqdn = DataTestUtility.GetMachineFQDN(hostname);
	string customSpn = $"MSSQLSvc/{fqdn}";
	if (!string.IsNullOrEmpty(instanceName))
	{
	customSpn += ":" + instanceName;
	}
	else if (port > 0)
	{
	customSpn += ":" + port;
	// Build the expected TCP SPN for the server.
	// TCP Kerberos SPNs are expected to use hostname[:port]; using instanceName here
	// is brittle because many environments register only port-based SPNs.
	string fqdn = DataTestUtility.GetMachineFQDN(hostname);
	string customSpn = $"MSSQLSvc/{fqdn}";
	if (port > 0)
	{
	customSpn += ":" + port;
	}
	else if (!string.IsNullOrEmpty(instanceName))
	{
	return; // Skip test: named instance without a resolved port cannot produce a reliable custom TCP SPN

[Copilot]

Similar to the TCP test, this builds an admin: Data Source with an explicit port fallback to 1433. If port isn't known, appending 1433 will disable SSRP and is unlikely to work for a named instance (and can also produce an invalid "host\,1433" when instanceName is empty). Consider preserving the base tcpConnStr settings and only changing the DataSource prefix, and omit the port unless you can derive the correct DAC port.

Suggested change

	if (string.IsNullOrEmpty(tcpConnStr) ||
	!DataTestUtility.ParseDataSource(new SqlConnectionStringBuilder(tcpConnStr).DataSource,
	out string hostname, out int port, out string instanceName))
	{
	return; // Skip test
	}
	KerberosTicketManagemnt.Init(DataTestUtility.KerberosDomainUser, DataTestUtility.KerberosDomainPassword);
	int testPort = port > 0 ? port : 1433;
	// Build admin: connection string
	string protocolAdminConnStr = $"Data Source=admin:{hostname}\\{instanceName},{testPort};Integrated Security=true;";
	try
	{
	using SqlConnection conn = new(protocolAdminConnStr);
	if (string.IsNullOrEmpty(tcpConnStr))
	{
	return; // Skip test
	}
	KerberosTicketManagemnt.Init(DataTestUtility.KerberosDomainUser, DataTestUtility.KerberosDomainPassword);
	SqlConnectionStringBuilder builder = new(tcpConnStr);
	if (string.IsNullOrEmpty(builder.DataSource))
	{
	return; // Skip test
	}
	// Preserve the original TCP connection string settings and only switch to the admin: Data Source.
	// Do not append a fallback port because that disables SSRP for named instances and can produce
	// an invalid Data Source when no instance name is present.
	builder.DataSource = $"admin:{builder.DataSource}";
	builder.IntegratedSecurity = true;
	try
	{
	using SqlConnection conn = new(builder.ConnectionString);

[Copilot]

Catching SqlException based on message substrings is fragile (localization and message changes) and the early return will mark the test as passed rather than skipped. Consider using a more stable signal (e.g., specific SqlException.Number/state if available) and using the test framework's skip mechanism so the result is reported as skipped when DAC isn't enabled/accessible.

[Copilot]

These tests use hostnames like "server" which will trigger a DNS lookup inside SniProxy.GetSqlServerSPNs (Dns.GetHostEntry). That can introduce unnecessary network dependency/flakiness in unit tests. Consider using "localhost"/"127.0.0.1" (or another reliably-resolvable name) for ParseServerName inputs so the tests remain deterministic across environments.

[Copilot]

When InstanceName is present and protocol is not NP, this always uses ResolvedPort even if it is still the default (-1). That can produce an invalid SPN postfix like ":-1" in error paths (e.g., SSRP lookup fails before ResolvedPort is set) and in any call sites that haven't populated ResolvedPort. Consider falling back to InstanceName (or leaving postfix null) when ResolvedPort <= 0 / == -1, to avoid generating malformed SPNs and misleading diagnostics.

Suggested change

	// use a port postfix (resolved via SSRP for named instances).
	// https://learn.microsoft.com/en-us/sql/database-engine/configure-windows/register-a-service-principal-name-for-kerberos-connections?view=sql-server-ver17#named-instance
	postfix = dataSource.ResolvedProtocol == DataSource.Protocol.NP ? dataSource.InstanceName : dataSource.ResolvedPort.ToString();
	// use a port postfix (resolved via SSRP for named instances). If the resolved
	// port is not yet available, fall back to the instance name rather than
	// generating an invalid SPN postfix such as ":-1".
	// https://learn.microsoft.com/en-us/sql/database-engine/configure-windows/register-a-service-principal-name-for-kerberos-connections?view=sql-server-ver17#named-instance
	postfix = dataSource.ResolvedProtocol == DataSource.Protocol.NP || dataSource.ResolvedPort <= 0
	? dataSource.InstanceName
	: dataSource.ResolvedPort.ToString();