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.

Author: paulmedynski

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

@@ -137,8 +137,12 @@ internal static ResolvedServerSpn GetSqlServerSPNs(DataSource dataSource, string
                 // MSSQLSvc/<FQDN>:<instancename> 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);

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));
         }
     }
 

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);