Fix | Enhance certificate validation (#2487)

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs

@@ -24,6 +24,8 @@ internal sealed class SNINpHandle : SNIPhysicalHandle
 
         private readonly string _targetServer;
         private readonly object _sendSync;
+        private readonly string _hostNameInCertificate;
+        private readonly string _serverCertificateFilename;
         private readonly bool _tlsFirst;
         private Stream _stream;
         private NamedPipeClientStream _pipeStream;
@@ -38,7 +40,7 @@ internal sealed class SNINpHandle : SNIPhysicalHandle
         private int _bufferSize = TdsEnums.DEFAULT_LOGIN_PACKET_SIZE;
         private readonly Guid _connectionId = Guid.NewGuid();
 
-        public SNINpHandle(string serverName, string pipeName, TimeoutTimer timeout, bool tlsFirst)
+        public SNINpHandle(string serverName, string pipeName, TimeoutTimer timeout, bool tlsFirst, string hostNameInCertificate, string serverCertificateFilename)
         {
             using (TrySNIEventScope.Create(nameof(SNINpHandle)))
             {
@@ -47,6 +49,8 @@ public SNINpHandle(string serverName, string pipeName, TimeoutTimer timeout, boo
                 _sendSync = new object();
                 _targetServer = serverName;
                 _tlsFirst = tlsFirst;
+                _hostNameInCertificate = hostNameInCertificate;
+                _serverCertificateFilename = serverCertificateFilename;
                 try
                 {
                     _pipeStream = new NamedPipeClientStream(
@@ -369,23 +373,23 @@ public override void DisableSsl()
         /// Validate server certificate
         /// </summary>
         /// <param name="sender">Sender object</param>
-        /// <param name="cert">X.509 certificate</param>
+        /// <param name="serverCertificate">X.509 certificate</param>
         /// <param name="chain">X.509 chain</param>
         /// <param name="policyErrors">Policy errors</param>
         /// <returns>true if valid</returns>
-        private bool ValidateServerCertificate(object sender, X509Certificate cert, X509Chain chain, SslPolicyErrors policyErrors)
+        private bool ValidateServerCertificate(object sender, X509Certificate serverCertificate, X509Chain chain, SslPolicyErrors policyErrors)
         {
             using (TrySNIEventScope.Create(nameof(SNINpHandle)))
-            {
+            {			
                 if (!_validateCert)
                 {
                     SqlClientEventSource.Log.TrySNITraceEvent(nameof(SNINpHandle), EventType.INFO, "Connection Id {0}, Certificate validation not requested.", args0: ConnectionId);
                     return true;
                 }
 
                 SqlClientEventSource.Log.TrySNITraceEvent(nameof(SNINpHandle), EventType.INFO, "Connection Id {0}, Proceeding to SSL certificate validation.", args0: ConnectionId);
-                return SNICommon.ValidateSslServerCertificate(_targetServer, cert, policyErrors);
-            }
+                return SNICommon.ValidateSslServerCertificate(_connectionId, _targetServer, _hostNameInCertificate, serverCertificate, _serverCertificateFilename, policyErrors);
+			}	
         }
 
         /// <summary>

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs

@@ -206,7 +206,7 @@ internal static SNIHandle CreateConnectionHandle(
                         tlsFirst, hostNameInCertificate, serverCertificateFilename);
                     break;
                 case DataSource.Protocol.NP:
-                    sniHandle = CreateNpHandle(details, timeout, parallel, tlsFirst);
+                    sniHandle = CreateNpHandle(details, timeout, parallel, tlsFirst, hostNameInCertificate, serverCertificateFilename);
                     break;
                 default:
                     Debug.Fail($"Unexpected connection protocol: {details._connectionProtocol}");
@@ -362,16 +362,18 @@ private static SNITCPHandle CreateTcpHandle(
         /// <param name="timeout">Timer expiration</param>
         /// <param name="parallel">Should MultiSubnetFailover be used. Only returns an error for named pipes.</param>
         /// <param name="tlsFirst"></param>
+        /// <param name="hostNameInCertificate">Host name in certificate</param>
+        /// <param name="serverCertificateFilename">Used for the path to the Server Certificate</param>
         /// <returns>SNINpHandle</returns>
-        private static SNINpHandle CreateNpHandle(DataSource details, TimeoutTimer timeout, bool parallel, bool tlsFirst)
+        private static SNINpHandle CreateNpHandle(DataSource details, TimeoutTimer timeout, bool parallel, bool tlsFirst, string hostNameInCertificate, string serverCertificateFilename)
         {
             if (parallel)
             {
                 // Connecting to a SQL Server instance using the MultiSubnetFailover connection option is only supported when using the TCP protocol
                 SNICommon.ReportSNIError(SNIProviders.NP_PROV, 0, SNICommon.MultiSubnetFailoverWithNonTcpProtocol, Strings.SNI_ERROR_49);
                 return null;
             }
-            return new SNINpHandle(details.PipeHostName, details.PipeName, timeout, tlsFirst);
+            return new SNINpHandle(details.PipeHostName, details.PipeName, timeout, tlsFirst, hostNameInCertificate, serverCertificateFilename);
         }
 
         /// <summary>
@@ -539,8 +541,10 @@ private void PopulateProtocol()
         internal static string GetLocalDBInstance(string dataSource, out bool error)
         {
             string instanceName = null;
+            // ReadOnlySpan is not supported in netstandard 2.0, but installing System.Memory solves the issue
             ReadOnlySpan<char> input = dataSource.AsSpan().TrimStart();
             error = false;
+            // NetStandard 2.0 does not support passing a string to ReadOnlySpan<char>
             int index = input.IndexOf(LocalDbHost.AsSpan().Trim(), StringComparison.InvariantCultureIgnoreCase);
             if (input.StartsWith(LocalDbHost_NP.AsSpan().Trim(), StringComparison.InvariantCultureIgnoreCase))
             {

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNITcpHandle.cs

@@ -644,7 +644,6 @@ public override uint EnableSsl(uint options)
                     }
                     else
                     {
-                        // TODO: Resolve whether to send _serverNameIndication or _targetServer. _serverNameIndication currently results in error. Why?
                         _sslStream.AuthenticateAsClient(_targetServer, null, s_supportedProtocols, false);
                     }
                     if (_sslOverTdsStream is not null)
@@ -698,33 +697,8 @@ private bool ValidateServerCertificate(object sender, X509Certificate serverCert
                 return true;
             }
 
-            string serverNameToValidate;
-            if (!string.IsNullOrEmpty(_hostNameInCertificate))
-            {
-                serverNameToValidate = _hostNameInCertificate;
-            }
-            else
-            {
-                serverNameToValidate = _targetServer;
-            }
-
-            if (!string.IsNullOrEmpty(_serverCertificateFilename))
-            {
-                X509Certificate clientCertificate = null;
-                try
-                {
-                    clientCertificate = new X509Certificate(_serverCertificateFilename);
-                    return SNICommon.ValidateSslServerCertificate(clientCertificate, serverCertificate, policyErrors);
-                }
-                catch (Exception e)
-                {
-                    // if this fails, then fall back to the HostNameInCertificate or TargetServer validation.
-                    SqlClientEventSource.Log.TrySNITraceEvent(nameof(SNITCPHandle), EventType.INFO, "Connection Id {0}, IOException occurred: {1}", args0: _connectionId, args1: e.Message);
-                }
-            }
-
             SqlClientEventSource.Log.TrySNITraceEvent(nameof(SNITCPHandle), EventType.INFO, "Connection Id {0}, Certificate will be validated for Target Server name", args0: _connectionId);
-            return SNICommon.ValidateSslServerCertificate(serverNameToValidate, serverCertificate, policyErrors);
+            return SNICommon.ValidateSslServerCertificate(_connectionId, _targetServer, _hostNameInCertificate, serverCertificate, _serverCertificateFilename, policyErrors);
         }
 
         /// <summary>

src/Microsoft.Data.SqlClient/src/Resources/Strings.resx

@@ -4737,4 +4737,7 @@
   <data name="SQL_RemoteCertificateNotAvailable" xml:space="preserve">
     <value>Certificate not available while validating the certificate.</value>
   </data>
+  <data name="SQL_RemoteCertificateDoesNotMatchServerCertificate" xml:space="preserve">
+    <value>The certificate provided by the server does not match the certificate provided by the ServerCertificate option.</value>
+  </data>
 </root>

src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs

@@ -12,6 +12,7 @@
 using System.Security;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.SqlServer.TDS.PreLogin;
 using Microsoft.SqlServer.TDS.Servers;
 using Xunit;
 

src/Microsoft.Data.SqlClient/tests/FunctionalTests/TestTdsServer.cs

@@ -65,6 +65,5 @@ public static TestTdsServer StartTestServer(bool enableFedAuth = false, bool ena
         public void Dispose() => _endpoint?.Stop();
 
         public string ConnectionString { get; private set; }
-
     }
 }

src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/ConnectionTestParameters.cs

@@ -0,0 +1,40 @@
+// 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.
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+using Microsoft.SqlServer.TDS.PreLogin;
+
+namespace Microsoft.Data.SqlClient.ManualTesting.Tests.DataCommon
+{
+    public class ConnectionTestParameters
+    {
+        private SqlConnectionEncryptOption _encryptionOption;
+        private TDSPreLoginTokenEncryptionType _encryptionType;
+        private string _hnic;
+        private string _cert;
+        private bool _result;
+        private bool _trustServerCert;
+
+        public SqlConnectionEncryptOption Encrypt => _encryptionOption;
+        public bool TrustServerCertificate => _trustServerCert;
+        public string Certificate => _cert;
+        public string HostNameInCertificate => _hnic;
+        public bool TestResult => _result;
+        public TDSPreLoginTokenEncryptionType TdsEncryptionType => _encryptionType;
+
+        public ConnectionTestParameters(TDSPreLoginTokenEncryptionType tdsEncryptionType, SqlConnectionEncryptOption encryptOption, bool trustServerCert, string cert, string hnic, bool result)
+        {
+            _encryptionOption = encryptOption;
+            _trustServerCert = trustServerCert;
+            _cert = cert;
+            _hnic = hnic;
+            _result = result;
+            _encryptionType = tdsEncryptionType;
+        }
+    }
+}

src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/ConnectionTestParametersData.cs

@@ -0,0 +1,85 @@
+// 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.
+
+using System.Collections.Generic;
+using System.IO;
+using Microsoft.SqlServer.TDS.PreLogin;
+
+namespace Microsoft.Data.SqlClient.ManualTesting.Tests.DataCommon
+{
+    public class ConnectionTestParametersData
+    {
+        private const int CASES = 30;
+        private string _empty = string.Empty;
+        // It was advised to store the client certificate in its own folder.
+        private static readonly string s_fullPathToCer = Path.Combine(Directory.GetCurrentDirectory(), "clientcert", "localhostcert.cer");
+        private static readonly string s_mismatchedcert = Path.Combine(Directory.GetCurrentDirectory(), "clientcert", "mismatchedcert.cer");
+
+        private static readonly string s_hostName = System.Net.Dns.GetHostName();
+        public static ConnectionTestParametersData Data { get; } = new ConnectionTestParametersData();
+        public List<ConnectionTestParameters> ConnectionTestParametersList { get; set; }
+
+        public static IEnumerable<object[]> GetConnectionTestParameters()
+        {
+            for (int i = 0; i < CASES; i++)
+            {
+                yield return new object[] { Data.ConnectionTestParametersList[i] };
+            }
+        }
+
+        public ConnectionTestParametersData()
+        {
+            // Test cases possible field values for connection parameters:
+            // These combinations are based on the possible values of Encrypt, TrustServerCertificate, Certificate, HostNameInCertificate
+            /*
+             * TDSEncryption | Encrypt    | TrustServerCertificate | Certificate  | HNIC         | TestResults
+             * ----------------------------------------------------------------------------------------------
+             *   Off         | Optional   |  true                  | valid        | valid name   | true
+             *   On          | Mandatory  |  false                 | mismatched   | empty        | false
+             *   Required    |            |    x                   | ChainError?  | wrong name?  | 
+             */
+            ConnectionTestParametersList = new List<ConnectionTestParameters>
+            {
+                // TDSPreLoginTokenEncryptionType.Off
+                new(TDSPreLoginTokenEncryptionType.Off, SqlConnectionEncryptOption.Optional, false, _empty, _empty, true),
+                new(TDSPreLoginTokenEncryptionType.Off, SqlConnectionEncryptOption.Mandatory, false, _empty, _empty, false),
+                new(TDSPreLoginTokenEncryptionType.Off, SqlConnectionEncryptOption.Optional, true, _empty, _empty, true),
+                new(TDSPreLoginTokenEncryptionType.Off, SqlConnectionEncryptOption.Mandatory, true, _empty, _empty, true),
+                new(TDSPreLoginTokenEncryptionType.Off, SqlConnectionEncryptOption.Mandatory, false, s_fullPathToCer, _empty, true),
+                new(TDSPreLoginTokenEncryptionType.Off, SqlConnectionEncryptOption.Mandatory, true, s_fullPathToCer, _empty, true),
+                new(TDSPreLoginTokenEncryptionType.Off, SqlConnectionEncryptOption.Mandatory, false, _empty, s_hostName, false),
+                new(TDSPreLoginTokenEncryptionType.Off, SqlConnectionEncryptOption.Mandatory, true, _empty, s_hostName, true),
+
+                // TDSPreLoginTokenEncryptionType.On
+                new(TDSPreLoginTokenEncryptionType.On, SqlConnectionEncryptOption.Optional, false, _empty, _empty, false),
+                new(TDSPreLoginTokenEncryptionType.On, SqlConnectionEncryptOption.Mandatory, false, _empty, _empty, false),
+                new(TDSPreLoginTokenEncryptionType.On, SqlConnectionEncryptOption.Optional, true, _empty, _empty, true),
+                new(TDSPreLoginTokenEncryptionType.On, SqlConnectionEncryptOption.Mandatory, true, _empty, _empty, true),
+                new(TDSPreLoginTokenEncryptionType.On, SqlConnectionEncryptOption.Mandatory, false, s_fullPathToCer, _empty, true),
+                new(TDSPreLoginTokenEncryptionType.On, SqlConnectionEncryptOption.Mandatory, true, s_fullPathToCer, _empty, true),
+                new(TDSPreLoginTokenEncryptionType.On, SqlConnectionEncryptOption.Mandatory, false, _empty, s_hostName, false),
+                new(TDSPreLoginTokenEncryptionType.On, SqlConnectionEncryptOption.Mandatory, true, _empty, s_hostName, true),
+
+                // TDSPreLoginTokenEncryptionType.Required
+                new(TDSPreLoginTokenEncryptionType.Required, SqlConnectionEncryptOption.Optional, false, _empty, _empty, false),
+                new(TDSPreLoginTokenEncryptionType.Required, SqlConnectionEncryptOption.Mandatory, false, _empty, _empty, false),
+                new(TDSPreLoginTokenEncryptionType.Required, SqlConnectionEncryptOption.Optional, true, _empty, _empty, true),
+                new(TDSPreLoginTokenEncryptionType.Required, SqlConnectionEncryptOption.Mandatory, true, _empty, _empty, true),
+                new(TDSPreLoginTokenEncryptionType.Required, SqlConnectionEncryptOption.Mandatory, false, s_fullPathToCer, _empty, true),
+                new(TDSPreLoginTokenEncryptionType.Required, SqlConnectionEncryptOption.Mandatory, true, s_fullPathToCer, _empty, true),
+                new(TDSPreLoginTokenEncryptionType.Required, SqlConnectionEncryptOption.Mandatory, false, _empty, s_hostName, false),
+                new(TDSPreLoginTokenEncryptionType.Required, SqlConnectionEncryptOption.Mandatory, true, _empty, s_hostName, true),
+
+                // Mismatched certificate test
+                new(TDSPreLoginTokenEncryptionType.Off, SqlConnectionEncryptOption.Mandatory, false, s_mismatchedcert, _empty, false),
+                new(TDSPreLoginTokenEncryptionType.Off, SqlConnectionEncryptOption.Mandatory, true, s_mismatchedcert, _empty, false),
+                new(TDSPreLoginTokenEncryptionType.Off, SqlConnectionEncryptOption.Mandatory, true, s_mismatchedcert, _empty, true),
+                new(TDSPreLoginTokenEncryptionType.On, SqlConnectionEncryptOption.Mandatory, false, s_mismatchedcert, _empty, false),
+                new(TDSPreLoginTokenEncryptionType.On, SqlConnectionEncryptOption.Mandatory, true, s_mismatchedcert, _empty, true),
+                new(TDSPreLoginTokenEncryptionType.Required, SqlConnectionEncryptOption.Mandatory, false, s_mismatchedcert, _empty, false),
+                new(TDSPreLoginTokenEncryptionType.Required, SqlConnectionEncryptOption.Mandatory, true, s_mismatchedcert, _empty, true),
+            };
+        }
+    }
+}

src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj

@@ -270,6 +270,8 @@
   <ItemGroup>
     <Compile Include="DataCommon\AADUtility.cs" />
     <Compile Include="DataCommon\AssemblyResourceManager.cs" />
+    <Compile Include="DataCommon\ConnectionTestParameters.cs" />
+    <Compile Include="DataCommon\ConnectionTestParametersData.cs" />
     <Compile Include="DataCommon\DataSourceBuilder.cs" />
     <Compile Include="DataCommon\DataTestUtility.cs" />
     <Compile Include="DataCommon\ProxyServer.cs" />
@@ -287,6 +289,7 @@
     <Compile Include="SQL\Common\SystemDataInternals\TdsParserHelper.cs" />
     <Compile Include="SQL\Common\SystemDataInternals\TdsParserStateObjectHelper.cs" />
     <Compile Include="SQL\ConnectionTestWithSSLCert\CertificateTest.cs" />
+    <Compile Include="SQL\ConnectionTestWithSSLCert\CertificateTestWithTdsServer.cs" />
     <Compile Include="SQL\SqlCommand\SqlCommandStoredProcTest.cs" />
     <Compile Include="TracingTests\TestTdsServer.cs" />
     <Compile Include="XUnitAssemblyAttributes.cs" />
@@ -354,6 +357,15 @@
    </ContentWithTargetPath>
   </ItemGroup>
   <ItemGroup>
+    <None Update="makepfxcert.ps1">
+      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
+    </None>
+    <None Update="mismatchedcert.cer">
+      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
+    </None>
+    <None Update="removecert.ps1">
+      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
+    </None>
     <None Update="SQL\ConnectionTestWithSSLCert\GenerateSelfSignedCertificate.ps1">
       <CopyToOutputDirectory>Always</CopyToOutputDirectory>
     </None>

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionTestWithSSLCert/CertificateTest.cs

@@ -32,7 +32,7 @@ public class CertificateTest : IDisposable
         // InstanceName will get replaced with an instance name in the connection string
         private static string InstanceName = "MSSQLSERVER";
 
-        // InstanceNamePrefix will get replaced with MSSQL$ is there is an instance name in connection string
+        // s_instanceNamePrefix will get replaced with MSSQL$ is there is an instance name in connection string
         private static string InstanceNamePrefix = "";
 
         // SlashInstance is used to override IPV4 and IPV6 defined about so it includes an instance name