From 88d97e2f535b07aa4f765e13dd5b61955504dd6e Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Wed, 29 Apr 2026 14:47:21 -0300 Subject: [PATCH 1/3] Fix SPN selection for named-instance SSRP and restore regression tests --- .../SqlClient/ManagedSni/SniProxy.netcore.cs | 18 ++-- .../SniProxyGetSqlServerSPNsTest.cs | 86 +++++++++++++++++++ 2 files changed, 98 insertions(+), 6 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SniProxyGetSqlServerSPNsTest.cs diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniProxy.netcore.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniProxy.netcore.cs index da6885062d..acdb82fe8f 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniProxy.netcore.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniProxy.netcore.cs @@ -116,7 +116,7 @@ internal static SniHandle CreateConnectionHandle( return sniHandle; } - private static ResolvedServerSpn GetSqlServerSPNs(DataSource dataSource, string serverSPN) + internal static ResolvedServerSpn GetSqlServerSPNs(DataSource dataSource, string serverSPN) { Debug.Assert(!string.IsNullOrWhiteSpace(dataSource.ServerName)); if (!string.IsNullOrWhiteSpace(serverSPN)) @@ -132,14 +132,20 @@ private static ResolvedServerSpn GetSqlServerSPNs(DataSource dataSource, string } else if (!string.IsNullOrWhiteSpace(dataSource.InstanceName)) { - postfix = dataSource.ResolvedProtocol == DataSource.Protocol.TCP ? dataSource.ResolvedPort.ToString() : dataSource.InstanceName; + // Per SQL Server Kerberos/SPN guidance, TCP client connections should use + // MSSQLSvc/:, while named pipes/shared-memory use + // MSSQLSvc/: for named instances. For our managed SNI path, + // NP uses instance-name postfix and TCP-like protocols (TCP, None, Admin) + // 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(); } - SqlClientEventSource.Log.TryTraceEvent("SNIProxy.GetSqlServerSPN | Info | ServerName {0}, InstanceName {1}, Port {2}, postfix {3}", dataSource?.ServerName, dataSource?.InstanceName, dataSource?.Port, postfix); + SqlClientEventSource.Log.TryTraceEvent("SNIProxy.GetSqlServerSPN | Info | ServerName {0}, InstanceName {1}, Port {2}, ResolvedPort {3}, ResolvedProtocol {4}, postfix {5}", dataSource?.ServerName, dataSource?.InstanceName, dataSource?.Port, dataSource?.ResolvedPort, dataSource?.ResolvedProtocol, postfix); return GetSqlServerSPNs(hostName, postfix, dataSource.ResolvedProtocol); } - private static ResolvedServerSpn GetSqlServerSPNs(string hostNameOrAddress, string portOrInstanceName, DataSource.Protocol protocol) + internal static ResolvedServerSpn GetSqlServerSPNs(string hostNameOrAddress, string portOrInstanceName, DataSource.Protocol protocol) { Debug.Assert(!string.IsNullOrWhiteSpace(hostNameOrAddress)); IPHostEntry hostEntry = null; @@ -607,8 +613,8 @@ private bool InferNamedPipesInformation() // If the data source starts with "np:servername" if (!_dataSourceAfterTrimmingProtocol.Contains(PipeBeginning)) { - // Assuming that user did not change default NamedPipe name, if the datasource is in the format servername\instance, - // separate servername and instance and prepend instance with MSSQL$ and append default pipe path + // Assuming that user did not change default NamedPipe name, if the datasource is in the format servername\instance, + // separate servername and instance and prepend instance with MSSQL$ and append default pipe path // https://learn.microsoft.com/en-us/sql/tools/configuration-manager/named-pipes-properties?view=sql-server-ver16 if (_dataSourceAfterTrimmingProtocol.Contains(PathSeparator) && ResolvedProtocol == Protocol.NP) { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SniProxyGetSqlServerSPNsTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SniProxyGetSqlServerSPNsTest.cs new file mode 100644 index 0000000000..908ee42163 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SniProxyGetSqlServerSPNsTest.cs @@ -0,0 +1,86 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#if NET + +using Microsoft.Data.SqlClient.ManagedSni; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.ManagedSni +{ + public class SniProxyGetSqlServerSPNsTest + { + [Fact] + public void GetSqlServerSPNs_ProtocolNone_WithResolvedPort_UsesPortNotInstanceName() + { + DataSource dataSource = DataSource.ParseServerName(@"server\instance"); + Assert.NotNull(dataSource); + Assert.Equal(DataSource.Protocol.None, dataSource.ResolvedProtocol); + Assert.Equal("instance", dataSource.InstanceName); + Assert.Equal(-1, dataSource.Port); + + dataSource.ResolvedPort = 12345; + + ResolvedServerSpn spn = SniProxy.GetSqlServerSPNs(dataSource, serverSPN: string.Empty); + + Assert.Contains(":12345", spn.Primary); + Assert.DoesNotContain("instance", spn.Primary, System.StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void GetSqlServerSPNs_ProtocolTcp_WithResolvedPort_UsesPort() + { + DataSource dataSource = DataSource.ParseServerName(@"tcp:server\instance"); + Assert.NotNull(dataSource); + Assert.Equal(DataSource.Protocol.TCP, dataSource.ResolvedProtocol); + + dataSource.ResolvedPort = 54321; + + ResolvedServerSpn spn = SniProxy.GetSqlServerSPNs(dataSource, serverSPN: string.Empty); + + Assert.Contains(":54321", spn.Primary); + Assert.DoesNotContain("instance", spn.Primary, System.StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void GetSqlServerSPNs_ProtocolNp_WithInstanceName_UsesInstanceName() + { + ResolvedServerSpn spn = SniProxy.GetSqlServerSPNs("server", "myinstance", DataSource.Protocol.NP); + + Assert.Contains(":myinstance", spn.Primary); + Assert.Null(spn.Secondary); + } + + [Fact] + public void GetSqlServerSPNs_CustomSpnProvided_UsesCustomSpn() + { + DataSource dataSource = DataSource.ParseServerName(@"server\instance"); + Assert.NotNull(dataSource); + dataSource.ResolvedPort = 12345; + + string customSpn = "MSSQLSvc/myserver.domain.com:1433"; + ResolvedServerSpn spn = SniProxy.GetSqlServerSPNs(dataSource, serverSPN: customSpn); + + Assert.Equal(customSpn, spn.Primary); + Assert.Null(spn.Secondary); + } + + [Fact] + public void GetSqlServerSPNs_ProtocolAdmin_WithResolvedPort_UsesPort() + { + DataSource dataSource = DataSource.ParseServerName(@"admin:server\instance"); + Assert.NotNull(dataSource); + Assert.Equal(DataSource.Protocol.Admin, dataSource.ResolvedProtocol); + + dataSource.ResolvedPort = 11111; + + ResolvedServerSpn spn = SniProxy.GetSqlServerSPNs(dataSource, serverSPN: string.Empty); + + Assert.Contains(":11111", spn.Primary); + Assert.DoesNotContain("instance", spn.Primary, System.StringComparison.OrdinalIgnoreCase); + } + } +} + +#endif From b87bc7b9e3d691e8447e6e89b1c3687765487933 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Wed, 29 Apr 2026 15:05:10 -0300 Subject: [PATCH 2/3] Add comprehensive documentation and integration tests for protocol-specific SPN handling - Add XML class and method documentation to SniProxyGetSqlServerSPNsTest explaining: * Purpose: regression tests for SPN protocol-specific behavior (issue #3566) * Protocol semantics: TCP-like protocols use port, Named Pipes uses instance name * Reference to official Microsoft Learn SPN format documentation * Specific test intentions and assertions - Add inline comments in unit tests clarifying: * DataSource parsing and Protocol.None/TCP/NP/Admin differentiation * SSRP port simulation behavior * Why each assertion validates the correct SPN format per protocol - Add 5 integration tests to KerberosTest.cs for end-to-end Kerberos validation: * ProtocolNone_NamedInstanceWithSsrpResolution - Tests issue #3566 (SSRP-resolved port in SPN) * ProtocolTcp_NamedInstanceWithExplicitPort - Tests TCP protocol with named instances * CustomServerSPN_BypassesAutoGeneration - Tests explicit SPN overrides for custom environments * ProtocolAdmin_DedicatedAdminConnection - Tests DAC protocol SPN behavior * Plus enhanced documentation explaining environment setup requirements Tests verify that Kerberos authentication succeeds by checking auth_scheme='KERBEROS' in sys.dm_exec_connections, confirming that protocol-specific SPN generation enables successful authentication across all protocol types. Addresses Cheena's review concerns about protocol semantics and Kerberos regression testing. --- .../SQL/KerberosTests/KerberosTest.cs | 197 ++++++++++++++++++ .../SniProxyGetSqlServerSPNsTest.cs | 72 ++++++- 2 files changed, 267 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/KerberosTests/KerberosTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/KerberosTests/KerberosTest.cs index c6026088f4..7e44e6bd0e 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/KerberosTests/KerberosTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/KerberosTests/KerberosTest.cs @@ -8,8 +8,25 @@ namespace Microsoft.Data.SqlClient.ManualTesting.Tests { + /// + /// Integration tests for Kerberos authentication with protocol-specific SPN handling. + /// + /// These tests verify that the driver generates protocol-specific SPNs for Kerberos authentication + /// across different protocol types (TCP, None, Named Pipes, Admin). They complement unit tests + /// in SniProxyGetSqlServerSPNsTest by verifying end-to-end authentication behavior in a real + /// Kerberos domain environment. + /// + /// See: https://learn.microsoft.com/en-us/sql/database-engine/configure-windows/register-a-service-principal-name-for-kerberos-connections + /// public class KerberosTests { + /// + /// Baseline Kerberos connectivity test verifying that the Kerberos authentication mechanism works + /// with the configured connection strings. + /// + /// This test runs on Unix platforms with Kerberos credentials and verifies that connections + /// authenticate using the KERBEROS auth_scheme (confirmed via sys.dm_exec_connections). + /// [PlatformSpecific(TestPlatforms.AnyUnix)] [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.IsKerberosTest))] [ClassData(typeof(ConnectionStringsProvider))] @@ -24,8 +41,188 @@ public void IsKerBerosSetupTestAsync(string connectionStr) Assert.True(reader.Read(), "Expected to receive one row data"); Assert.Equal("KERBEROS", reader.GetString(0)); } + + /// + /// Tests Kerberos authentication with Protocol.None (default when no protocol prefix specified). + /// + /// This regression test for GitHub issue #3566 verifies that named instances accessed via + /// Protocol.None (e.g. "Data Source=hostname\instancename") use the correct SPN format: + /// - If SSRP resolves a port: MSSQLSvc/hostname:port (not instance name) + /// - Kerberos should authenticate successfully with the correct SPN + /// + /// Environment: Requires a named instance running on a domain-joined server with an SSRP-resolvable port. + /// + [PlatformSpecific(TestPlatforms.Linux)] + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.IsKerberosTest))] + public void KerberosTest_ProtocolNone_NamedInstanceWithSsrpResolution() + { + // Skip if no TCP connection string with a named instance is available + string tcpConnStr = DataTestUtility.TCPConnectionString; + if (string.IsNullOrEmpty(tcpConnStr) || + !DataTestUtility.ParseDataSource(new SqlConnectionStringBuilder(tcpConnStr).DataSource, + out string hostname, out int port, out string instanceName) || + string.IsNullOrEmpty(instanceName)) + { + return; // Skip test; no named instance available + } + + KerberosTicketManagemnt.Init(DataTestUtility.KerberosDomainUser, DataTestUtility.KerberosDomainPassword); + + // Build a connection string with Protocol.None (no prefix) pointing to the named instance + // SSRP resolution should occur and populate the port in the SPN + string protocolNoneConnStr = $"Data Source={hostname}\\{instanceName};Integrated Security=true;"; + + using SqlConnection conn = new(protocolNoneConnStr); + conn.Open(); // Connection should succeed with Kerberos using the SSRP-resolved port in SPN + + // Verify authentication occurred with KERBEROS auth_scheme + using SqlCommand command = new("SELECT auth_scheme from sys.dm_exec_connections where session_id = @@spid", conn); + using SqlDataReader reader = command.ExecuteReader(); + Assert.True(reader.Read(), "Expected to receive one row data"); + Assert.Equal("KERBEROS", reader.GetString(0)); + } + + /// + /// Tests Kerberos authentication with explicit Protocol.TCP prefix on a named instance. + /// + /// Verifies that "tcp:hostname\instancename" uses the TCP-like SPN format with port: + /// MSSQLSvc/hostname:port (where port is from explicit connection string or SSRP resolution). + /// + /// Environment: Requires a named instance with an explicitly specified or SSRP-resolvable port. + /// + [PlatformSpecific(TestPlatforms.Linux)] + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.IsKerberosTest))] + public void KerberosTest_ProtocolTcp_NamedInstanceWithExplicitPort() + { + string tcpConnStr = DataTestUtility.TCPConnectionString; + 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); + + // 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;"; + + using SqlConnection conn = new(protocolTcpConnStr); + conn.Open(); + + using SqlCommand command = new("SELECT auth_scheme from sys.dm_exec_connections where session_id = @@spid", conn); + using SqlDataReader reader = command.ExecuteReader(); + Assert.True(reader.Read(), "Expected to receive one row data"); + Assert.Equal("KERBEROS", reader.GetString(0)); + } + + /// + /// Tests Kerberos authentication with a custom ServerSPN override. + /// + /// Verifies that explicitly setting ServerSPN in the connection string bypasses auto-generation + /// and uses the provided SPN for Kerberos authentication. This is critical for environments where: + /// - Kerberos SPNs are registered with specific hostnames/ports + /// - SQL Server is behind a proxy or alias + /// - Multi-instance environments with non-standard naming + /// + /// Environment: Requires ability to specify a valid SPN for the target instance. + /// + [PlatformSpecific(TestPlatforms.Linux)] + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.IsKerberosTest))] + public void KerberosTest_CustomServerSPN_BypassesAutoGeneration() + { + string tcpConnStr = DataTestUtility.TCPConnectionString; + 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); + + // 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; + } + + SqlConnectionStringBuilder builder = new(tcpConnStr); + builder.IntegratedSecurity = true; + builder.ServerSPN = customSpn; + + using SqlConnection conn = new(builder.ConnectionString); + conn.Open(); + + using SqlCommand command = new("SELECT auth_scheme from sys.dm_exec_connections where session_id = @@spid", conn); + using SqlDataReader reader = command.ExecuteReader(); + Assert.True(reader.Read(), "Expected to receive one row data"); + Assert.Equal("KERBEROS", reader.GetString(0)); + } + + /// + /// Tests Kerberos authentication with Protocol.Admin (Dedicated Administrator Connection). + /// + /// Verifies that DAC connections (prefix "admin:") to named instances use the TCP-like SPN format: + /// MSSQLSvc/hostname:port (not instance name). DAC uses SSRP resolution similar to TCP. + /// + /// Environment: Requires DAC to be enabled on the target SQL Server instance and admin credentials. + /// Note: May be skipped if DAC is not enabled or not accessible via Kerberos. + /// + [PlatformSpecific(TestPlatforms.Linux)] + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.IsKerberosTest))] + public void KerberosTest_ProtocolAdmin_DedicatedAdminConnection() + { + string tcpConnStr = DataTestUtility.TCPConnectionString; + 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); + conn.Open(); + + using SqlCommand command = new("SELECT auth_scheme from sys.dm_exec_connections where session_id = @@spid", conn); + using SqlDataReader reader = command.ExecuteReader(); + if (reader.Read()) + { + Assert.Equal("KERBEROS", reader.GetString(0)); + } + } + catch (SqlException ex) when (ex.Message.Contains("DAC") || ex.Message.Contains("Dedicated")) + { + // DAC may not be enabled or accessible; skip this test without failing + return; + } + } } + /// + /// Provides connection strings from DataTestUtility for theory-based Kerberos tests. + /// + /// Each connection string is tested in a separate Kerberos context to ensure protocol-specific + /// SPN behavior works across all available SQL Server configurations in the test environment. + /// public class ConnectionStringsProvider : IEnumerable { public IEnumerator GetEnumerator() diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SniProxyGetSqlServerSPNsTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SniProxyGetSqlServerSPNsTest.cs index 908ee42163..20373310c3 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SniProxyGetSqlServerSPNsTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SniProxyGetSqlServerSPNsTest.cs @@ -9,74 +9,142 @@ namespace Microsoft.Data.SqlClient.UnitTests.ManagedSni { + /// + /// Regression tests for SPN (Service Principal Name) selection logic in . + /// + /// These tests verify that the driver generates protocol-specific SPNs for Kerberos authentication: + /// - TCP-like protocols (TCP, None, Admin) use MSSQLSvc/hostname:port + /// - Named Pipes uses MSSQLSvc/hostname:instancename + /// - Custom ServerSPN overrides are always respected + /// + /// This addresses GitHub issue #3566: named instances connecting without a protocol prefix + /// (Protocol.None) should use the SSRP-resolved port in the SPN, not the instance name. + /// + /// See: https://learn.microsoft.com/en-us/sql/database-engine/configure-windows/register-a-service-principal-name-for-kerberos-connections + /// public class SniProxyGetSqlServerSPNsTest { + /// + /// Verifies that Protocol.None (default when no prefix specified, e.g. "server\instance") + /// uses the SSRP-resolved port in the SPN, not the instance name. + /// + /// This is a regression test for GitHub issue #3566. On Linux with SSRP, a named instance + /// connection string like "Data Source=server\instance" requires the resolved TCP port + /// from SSRP to be used in the SPN for Kerberos authentication to succeed. + /// [Fact] public void GetSqlServerSPNs_ProtocolNone_WithResolvedPort_UsesPortNotInstanceName() { + // Arrange: parse "server\instance" which sets Protocol.None and IsSsrpRequired DataSource dataSource = DataSource.ParseServerName(@"server\instance"); Assert.NotNull(dataSource); Assert.Equal(DataSource.Protocol.None, dataSource.ResolvedProtocol); Assert.Equal("instance", dataSource.InstanceName); - Assert.Equal(-1, dataSource.Port); + Assert.Equal(-1, dataSource.Port); // No explicit port in connection string + // Simulate SSRP resolution setting the port (as CreateTcpHandle would do) dataSource.ResolvedPort = 12345; + // Act: generate SPN for this named instance with resolved port ResolvedServerSpn spn = SniProxy.GetSqlServerSPNs(dataSource, serverSPN: string.Empty); + // Assert: SPN should contain the resolved port, NOT the instance name Assert.Contains(":12345", spn.Primary); Assert.DoesNotContain("instance", spn.Primary, System.StringComparison.OrdinalIgnoreCase); } + /// + /// Verifies that Protocol.TCP (explicit "tcp:" prefix) uses the resolved port in the SPN. + /// + /// This was the original fix for GitHub issue #2187 and ensures TCP protocol behavior + /// is consistent across all platforms. + /// [Fact] public void GetSqlServerSPNs_ProtocolTcp_WithResolvedPort_UsesPort() { + // Arrange: parse "tcp:server\instance" which sets Protocol.TCP DataSource dataSource = DataSource.ParseServerName(@"tcp:server\instance"); Assert.NotNull(dataSource); Assert.Equal(DataSource.Protocol.TCP, dataSource.ResolvedProtocol); + // Simulate SSRP resolution setting the port dataSource.ResolvedPort = 54321; + // Act: generate SPN for this TCP named instance ResolvedServerSpn spn = SniProxy.GetSqlServerSPNs(dataSource, serverSPN: string.Empty); + // Assert: SPN should use the resolved port Assert.Contains(":54321", spn.Primary); Assert.DoesNotContain("instance", spn.Primary, System.StringComparison.OrdinalIgnoreCase); } + /// + /// Verifies that Protocol.NP (Named Pipes) uses the instance name in the SPN, + /// not a port number. + /// + /// Named Pipes protocol requires instance-name-based SPNs per SQL Server guidelines. + /// This test ensures NP behavior is preserved when the general protocol logic is updated. + /// [Fact] public void GetSqlServerSPNs_ProtocolNp_WithInstanceName_UsesInstanceName() { + // Arrange & Act: test the lower-level overload directly with NP protocol + // Named Pipes data sources go through a different parsing path that doesn't + // populate InstanceName in the same way, so we call the helper directly. ResolvedServerSpn spn = SniProxy.GetSqlServerSPNs("server", "myinstance", DataSource.Protocol.NP); + // Assert: SPN should use the instance name, not a port Assert.Contains(":myinstance", spn.Primary); - Assert.Null(spn.Secondary); + Assert.Null(spn.Secondary); // NP does not generate a secondary SPN } + /// + /// Verifies that explicit ServerSPN overrides (via connection string) are used as-is, + /// bypassing all auto-generation logic. + /// + /// This is critical for Kerberos environments where custom SPNs may be required + /// (e.g., non-standard ports, aliased hostnames, or specific service accounts). + /// [Fact] public void GetSqlServerSPNs_CustomSpnProvided_UsesCustomSpn() { + // Arrange: parse a named instance, but provide a custom SPN override DataSource dataSource = DataSource.ParseServerName(@"server\instance"); Assert.NotNull(dataSource); dataSource.ResolvedPort = 12345; string customSpn = "MSSQLSvc/myserver.domain.com:1433"; + + // Act: generate SPN with explicit override ResolvedServerSpn spn = SniProxy.GetSqlServerSPNs(dataSource, serverSPN: customSpn); + // Assert: custom SPN is used exactly as provided, without modification Assert.Equal(customSpn, spn.Primary); Assert.Null(spn.Secondary); } + /// + /// Verifies that Protocol.Admin (Dedicated Administrator Connection, DAC) + /// uses the resolved port in the SPN, not the instance name. + /// + /// DAC also uses SSRP resolution and should follow the same protocol-based logic + /// as Protocol.TCP and Protocol.None for SPN generation. + /// [Fact] public void GetSqlServerSPNs_ProtocolAdmin_WithResolvedPort_UsesPort() { + // Arrange: parse "admin:server\instance" which sets Protocol.Admin DataSource dataSource = DataSource.ParseServerName(@"admin:server\instance"); Assert.NotNull(dataSource); Assert.Equal(DataSource.Protocol.Admin, dataSource.ResolvedProtocol); + // Simulate SSRP resolution setting the port dataSource.ResolvedPort = 11111; + // Act: generate SPN for this DAC connection to a named instance ResolvedServerSpn spn = SniProxy.GetSqlServerSPNs(dataSource, serverSPN: string.Empty); + // Assert: SPN should use the resolved port, not the instance name Assert.Contains(":11111", spn.Primary); Assert.DoesNotContain("instance", spn.Primary, System.StringComparison.OrdinalIgnoreCase); } From c2710f0f7b49290fcffff6ed7f41b867e6d95403 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Thu, 30 Apr 2026 08:39:59 -0300 Subject: [PATCH 3/3] Address Copilot review feedback: SPN safety, DNS isolation, and test robustness SniProxy.netcore.cs: - Guard against ResolvedPort <= 0 (e.g. -1 before SSRP resolves) to avoid producing malformed SPNs like ':-1' in error paths. Fall back to instance name when port is not yet available. SniProxyGetSqlServerSPNsTest.cs: - Replace generic 'server' hostnames with 'localhost' in all ParseServerName calls and the NP direct-call test. This avoids real DNS lookups that could cause flakiness in restricted-DNS environments. KerberosTest.cs: - Protocol.None test: build connection string from SqlConnectionStringBuilder(tcpConnStr) instead of from scratch to preserve Encrypt, TrustServerCertificate, etc. - TCP test: skip if no named instance; omit port fallback to 1433 (which disables SSRP and can produce invalid 'host\,port' strings). - Custom SPN test: require explicit port and use TCP-format SPN (host:port) rather than instance name, which is wrong for port-based SPN registrations. - Admin test: build from tcpConnStr base; require named instance; remove fragile catch-by-message-substring that could hide failures. DAC unavailability now correctly surfaces as a test failure instead of silent pass. --- .../SqlClient/ManagedSni/SniProxy.netcore.cs | 6 +- .../SQL/KerberosTests/KerberosTest.cs | 105 +++++++++++------- .../SniProxyGetSqlServerSPNsTest.cs | 26 +++-- 3 files changed, 83 insertions(+), 54 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniProxy.netcore.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniProxy.netcore.cs index acdb82fe8f..a327dfd289 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniProxy.netcore.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniProxy.netcore.cs @@ -137,8 +137,12 @@ internal static ResolvedServerSpn GetSqlServerSPNs(DataSource dataSource, string // MSSQLSvc/: for named instances. For our managed SNI path, // NP uses instance-name postfix and TCP-like protocols (TCP, None, Admin) // use a port postfix (resolved via SSRP for named instances). + // If SSRP resolution hasn't populated ResolvedPort yet (value is -1), fall back + // to the instance name to avoid producing a malformed SPN like ":-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.InstanceName : dataSource.ResolvedPort.ToString(); + postfix = (dataSource.ResolvedProtocol == DataSource.Protocol.NP || dataSource.ResolvedPort <= 0) + ? dataSource.InstanceName + : dataSource.ResolvedPort.ToString(); } SqlClientEventSource.Log.TryTraceEvent("SNIProxy.GetSqlServerSPN | Info | ServerName {0}, InstanceName {1}, Port {2}, ResolvedPort {3}, ResolvedProtocol {4}, postfix {5}", dataSource?.ServerName, dataSource?.InstanceName, dataSource?.Port, dataSource?.ResolvedPort, dataSource?.ResolvedProtocol, postfix); diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/KerberosTests/KerberosTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/KerberosTests/KerberosTest.cs index 7e44e6bd0e..5a83e7a6b0 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/KerberosTests/KerberosTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/KerberosTests/KerberosTest.cs @@ -68,11 +68,16 @@ public void KerberosTest_ProtocolNone_NamedInstanceWithSsrpResolution() KerberosTicketManagemnt.Init(DataTestUtility.KerberosDomainUser, DataTestUtility.KerberosDomainPassword); - // Build a connection string with Protocol.None (no prefix) pointing to the named instance - // SSRP resolution should occur and populate the port in the SPN - string protocolNoneConnStr = $"Data Source={hostname}\\{instanceName};Integrated Security=true;"; + // Build from the base connection string to preserve environment settings (Encrypt, + // TrustServerCertificate, timeouts, etc.), overriding only DataSource and IntegratedSecurity. + // SSRP resolution should occur and populate the port in the SPN. + SqlConnectionStringBuilder protocolNoneBuilder = new(tcpConnStr) + { + DataSource = $"{hostname}\\{instanceName}", + IntegratedSecurity = true + }; - using SqlConnection conn = new(protocolNoneConnStr); + using SqlConnection conn = new(protocolNoneBuilder.ConnectionString); conn.Open(); // Connection should succeed with Kerberos using the SSRP-resolved port in SPN // Verify authentication occurred with KERBEROS auth_scheme @@ -97,20 +102,30 @@ public void KerberosTest_ProtocolTcp_NamedInstanceWithExplicitPort() string tcpConnStr = DataTestUtility.TCPConnectionString; if (string.IsNullOrEmpty(tcpConnStr) || !DataTestUtility.ParseDataSource(new SqlConnectionStringBuilder(tcpConnStr).DataSource, - out string hostname, out int port, out string instanceName)) + out string hostname, out int port, out string instanceName) || + string.IsNullOrEmpty(instanceName)) { - return; // Skip test + return; // Skip test; no named instance available } 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; + // Build the tcp: data source. Include an explicit port only when the test connection string + // already has one; otherwise leave SSRP to resolve the port for the named instance. + // Do NOT fall back to port 1433: that disables SSRP and is unlikely to be correct for + // named instances, and produces an invalid "host\,1433" when instanceName is empty. + string newDataSource = port > 0 + ? $"tcp:{hostname}\\{instanceName},{port}" + : $"tcp:{hostname}\\{instanceName}"; - string protocolTcpConnStr = $"Data Source=tcp:{hostname}\\{instanceName},{testPort};Integrated Security=true;"; + // Preserve the base connection string settings (Encrypt, TrustServerCertificate, etc.) + SqlConnectionStringBuilder builder = new(tcpConnStr) + { + DataSource = newDataSource, + IntegratedSecurity = true + }; - using SqlConnection conn = new(protocolTcpConnStr); + using SqlConnection conn = new(builder.ConnectionString); conn.Open(); using SqlCommand command = new("SELECT auth_scheme from sys.dm_exec_connections where session_id = @@spid", conn); @@ -142,19 +157,20 @@ public void KerberosTest_CustomServerSPN_BypassesAutoGeneration() return; // Skip test } + // For a reliable custom SPN test, we need to know the exact port so we can construct + // the TCP-format SPN the server expects: MSSQLSvc/fqdn:port. + // Using the instance name here is wrong for TCP environments that register only port-based SPNs. + if (port <= 0) + { + return; // Skip test; cannot construct a valid port-based custom SPN without an explicit port + } + KerberosTicketManagemnt.Init(DataTestUtility.KerberosDomainUser, DataTestUtility.KerberosDomainPassword); - // Build the expected SPN for the server + // Build the TCP-format SPN that matches what the driver would auto-generate. + // TCP Kerberos connections use MSSQLSvc/fqdn:port regardless of instance name. string fqdn = DataTestUtility.GetMachineFQDN(hostname); - string customSpn = $"MSSQLSvc/{fqdn}"; - if (!string.IsNullOrEmpty(instanceName)) - { - customSpn += ":" + instanceName; - } - else if (port > 0) - { - customSpn += ":" + port; - } + string customSpn = $"MSSQLSvc/{fqdn}:{port}"; SqlConnectionStringBuilder builder = new(tcpConnStr); builder.IntegratedSecurity = true; @@ -185,35 +201,38 @@ public void KerberosTest_ProtocolAdmin_DedicatedAdminConnection() string tcpConnStr = DataTestUtility.TCPConnectionString; if (string.IsNullOrEmpty(tcpConnStr) || !DataTestUtility.ParseDataSource(new SqlConnectionStringBuilder(tcpConnStr).DataSource, - out string hostname, out int port, out string instanceName)) + out string hostname, out int port, out string instanceName) || + string.IsNullOrEmpty(instanceName)) { - return; // Skip test + return; // Skip test; no named instance available } KerberosTicketManagemnt.Init(DataTestUtility.KerberosDomainUser, DataTestUtility.KerberosDomainPassword); - int testPort = port > 0 ? port : 1433; + // Build the admin: data source from the base connection string to preserve environment + // settings. Do NOT fall back to port 1433 — the DAC port is separate from the regular + // SQL Server port and must be discovered via SSRP if not explicitly known. + string newDataSource = port > 0 + ? $"admin:{hostname}\\{instanceName},{port}" + : $"admin:{hostname}\\{instanceName}"; - // Build admin: connection string - string protocolAdminConnStr = $"Data Source=admin:{hostname}\\{instanceName},{testPort};Integrated Security=true;"; - - try - { - using SqlConnection conn = new(protocolAdminConnStr); - conn.Open(); - - using SqlCommand command = new("SELECT auth_scheme from sys.dm_exec_connections where session_id = @@spid", conn); - using SqlDataReader reader = command.ExecuteReader(); - if (reader.Read()) - { - Assert.Equal("KERBEROS", reader.GetString(0)); - } - } - catch (SqlException ex) when (ex.Message.Contains("DAC") || ex.Message.Contains("Dedicated")) + SqlConnectionStringBuilder adminBuilder = new(tcpConnStr) { - // DAC may not be enabled or accessible; skip this test without failing - return; - } + DataSource = newDataSource, + IntegratedSecurity = true + }; + + // Note: this test requires DAC to be enabled on the target instance + // (sp_configure 'remote admin connections', 1). If DAC is not enabled, + // the connection will fail with a SqlException and the test will report as failed, + // which is the desired behavior — the test environment should be fixed. + using SqlConnection conn = new(adminBuilder.ConnectionString); + conn.Open(); + + using SqlCommand command = new("SELECT auth_scheme from sys.dm_exec_connections where session_id = @@spid", conn); + using SqlDataReader reader = command.ExecuteReader(); + Assert.True(reader.Read(), "Expected to receive one row data"); + Assert.Equal("KERBEROS", reader.GetString(0)); } } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SniProxyGetSqlServerSPNsTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SniProxyGetSqlServerSPNsTest.cs index 20373310c3..9e80e24e89 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SniProxyGetSqlServerSPNsTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SniProxyGetSqlServerSPNsTest.cs @@ -35,8 +35,10 @@ public class SniProxyGetSqlServerSPNsTest [Fact] public void GetSqlServerSPNs_ProtocolNone_WithResolvedPort_UsesPortNotInstanceName() { - // Arrange: parse "server\instance" which sets Protocol.None and IsSsrpRequired - DataSource dataSource = DataSource.ParseServerName(@"server\instance"); + // Arrange: parse "localhost\instance" which sets Protocol.None and IsSsrpRequired. + // Using "localhost" instead of an arbitrary hostname avoids real DNS lookups + // that would make the test flaky in environments with restricted DNS resolution. + DataSource dataSource = DataSource.ParseServerName(@"localhost\instance"); Assert.NotNull(dataSource); Assert.Equal(DataSource.Protocol.None, dataSource.ResolvedProtocol); Assert.Equal("instance", dataSource.InstanceName); @@ -62,8 +64,9 @@ public void GetSqlServerSPNs_ProtocolNone_WithResolvedPort_UsesPortNotInstanceNa [Fact] public void GetSqlServerSPNs_ProtocolTcp_WithResolvedPort_UsesPort() { - // Arrange: parse "tcp:server\instance" which sets Protocol.TCP - DataSource dataSource = DataSource.ParseServerName(@"tcp:server\instance"); + // Arrange: parse "tcp:localhost\instance" which sets Protocol.TCP. + // Using "localhost" avoids real DNS lookups that would make the test flaky. + DataSource dataSource = DataSource.ParseServerName(@"tcp:localhost\instance"); Assert.NotNull(dataSource); Assert.Equal(DataSource.Protocol.TCP, dataSource.ResolvedProtocol); @@ -88,10 +91,11 @@ public void GetSqlServerSPNs_ProtocolTcp_WithResolvedPort_UsesPort() [Fact] public void GetSqlServerSPNs_ProtocolNp_WithInstanceName_UsesInstanceName() { - // Arrange & Act: test the lower-level overload directly with NP protocol + // Arrange & Act: test the lower-level overload directly with NP protocol. // Named Pipes data sources go through a different parsing path that doesn't // populate InstanceName in the same way, so we call the helper directly. - ResolvedServerSpn spn = SniProxy.GetSqlServerSPNs("server", "myinstance", DataSource.Protocol.NP); + // Using "localhost" avoids real DNS lookups that would make the test flaky. + ResolvedServerSpn spn = SniProxy.GetSqlServerSPNs("localhost", "myinstance", DataSource.Protocol.NP); // Assert: SPN should use the instance name, not a port Assert.Contains(":myinstance", spn.Primary); @@ -108,8 +112,9 @@ public void GetSqlServerSPNs_ProtocolNp_WithInstanceName_UsesInstanceName() [Fact] public void GetSqlServerSPNs_CustomSpnProvided_UsesCustomSpn() { - // Arrange: parse a named instance, but provide a custom SPN override - DataSource dataSource = DataSource.ParseServerName(@"server\instance"); + // Arrange: parse a named instance, but provide a custom SPN override. + // Using "localhost" avoids real DNS lookups that would make the test flaky. + DataSource dataSource = DataSource.ParseServerName(@"localhost\instance"); Assert.NotNull(dataSource); dataSource.ResolvedPort = 12345; @@ -133,8 +138,9 @@ public void GetSqlServerSPNs_CustomSpnProvided_UsesCustomSpn() [Fact] public void GetSqlServerSPNs_ProtocolAdmin_WithResolvedPort_UsesPort() { - // Arrange: parse "admin:server\instance" which sets Protocol.Admin - DataSource dataSource = DataSource.ParseServerName(@"admin:server\instance"); + // Arrange: parse "admin:localhost\instance" which sets Protocol.Admin. + // Using "localhost" avoids real DNS lookups that would make the test flaky. + DataSource dataSource = DataSource.ParseServerName(@"admin:localhost\instance"); Assert.NotNull(dataSource); Assert.Equal(DataSource.Protocol.Admin, dataSource.ResolvedProtocol);