[Release/5.0] fix validation callback on Windows with Tls 1.3 (#55892)

* do not call RemoteCertificateValidationCallback on the same certificate again (#42836)

* do not call RemoteCertificateValidationCallback on the same certificate agin

* feedback from review

* feedback from review

* make sure we do not use Linq

* adjust renegotiation tests to match product change (#43123)

* adjust renegotiation tests to match product change

* add assert for validationCount

Author: wfurt

src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.SslProtocols.cs

@@ -100,10 +100,12 @@ public static IEnumerable<object[]> GetAsync_AllowedSSLVersion_Succeeds_MemberDa
         [MemberData(nameof(GetAsync_AllowedSSLVersion_Succeeds_MemberData))]
         public async Task GetAsync_AllowedSSLVersion_Succeeds(SslProtocols acceptedProtocol, bool requestOnlyThisProtocol)
         {
+            int count = 0;
             using (HttpClientHandler handler = CreateHttpClientHandler())
             using (HttpClient client = CreateHttpClient(handler))
             {
-                handler.ServerCertificateCustomValidationCallback = TestHelper.AllowAllCertificates;
+                handler.ServerCertificateCustomValidationCallback =
+                    (request, cert, chain, errors) => { count++; return true; };
 
                 if (requestOnlyThisProtocol)
                 {
@@ -131,6 +133,8 @@ await TestHelper.WhenAllCompletedOrAnyFailed(
                         server.AcceptConnectionSendResponseAndCloseAsync(),
                         client.GetAsync(url));
                 }, options);
+
+                Assert.Equal(1, count);
             }
         }
 

src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs

@@ -952,7 +952,17 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot
 
             try
             {
-                _remoteCertificate = CertificateValidationPal.GetRemoteCertificate(_securityContext, out remoteCertificateStore);
+                X509Certificate2? certificate = CertificateValidationPal.GetRemoteCertificate(_securityContext, out remoteCertificateStore);
+
+                if (_remoteCertificate != null && certificate != null &&
+                    certificate.RawData.AsSpan().SequenceEqual(_remoteCertificate.RawData))
+                {
+                    // This is renegotiation or TLS 1.3 and the certificate did not change.
+                    // There is no reason to process callback again as we already established trust.
+                    return true;
+                }
+
+                _remoteCertificate = certificate;
 
                 if (_remoteCertificate == null)
                 {

src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowRenegotiationTests.cs

@@ -54,15 +54,14 @@ public async Task SslStream_AllowRenegotiation_True_Succeeds()
                 Assert.True(ssl.IsAuthenticated);
                 Assert.True(ssl.IsEncrypted);
 
-                // Issue request that triggers regotiation from server.
+                // Issue request that triggers renegotiation from server.
                 byte[] message = Encoding.UTF8.GetBytes("GET /EchoClientCertificate.ashx HTTP/1.1\r\nHost: corefx-net-tls.azurewebsites.net\r\n\r\n");
                 await ssl.WriteAsync(message, 0, message.Length);
 
                 // Initiate Read operation, that results in starting renegotiation as per server response to the above request.
                 int bytesRead = await ssl.ReadAsync(message, 0, message.Length);
 
-                // Renegotiation will trigger another validation callback/
-                Assert.InRange(validationCount, 2, int.MaxValue);
+                Assert.Equal(1, validationCount);
                 Assert.InRange(bytesRead, 1, message.Length);
                 Assert.Contains("HTTP/1.1 200 OK", Encoding.UTF8.GetString(message));
             }

src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs

@@ -220,8 +220,7 @@ public async Task SslStream_NetworkStream_Renegotiation_Succeeds(bool useSync)
                 // Initiate Read operation, that results in starting renegotiation as per server response to the above request.
                 int bytesRead = useSync ? ssl.Read(message, 0, message.Length) : await ssl.ReadAsync(message, 0, message.Length);
 
-                // renegotiation will trigger validation callback again.
-                Assert.InRange(validationCount, 2, int.MaxValue);
+                Assert.Equal(1, validationCount);
                 Assert.InRange(bytesRead, 1, message.Length);
                 Assert.Contains("HTTP/1.1 200 OK", Encoding.UTF8.GetString(message));
             }
@@ -249,6 +248,7 @@ public async Task SslStream_NestedAuth_Throws()
         public async Task SslStream_TargetHostName_Succeeds(bool useEmptyName)
         {
             string targetName = useEmptyName ? string.Empty : Guid.NewGuid().ToString("N");
+            int count = 0;
 
             (Stream clientStream, Stream serverStream) = TestHelper.GetConnectedStreams();
             using (clientStream)
@@ -267,7 +267,7 @@ public async Task SslStream_TargetHostName_Succeeds(bool useEmptyName)
                     {
                         SslStream stream = (SslStream)sender;
                         Assert.Equal(targetName, stream.TargetHostName);
-
+                        count++;
                         return true;
                     };
 
@@ -284,8 +284,12 @@ public async Task SslStream_TargetHostName_Succeeds(bool useEmptyName)
                 await TestConfiguration.WhenAllOrAnyFailedWithTimeout(
                                 client.AuthenticateAsClientAsync(clientOptions),
                                 server.AuthenticateAsServerAsync(serverOptions));
+
+                await TestHelper.PingPong(client, server);
+
                 Assert.Equal(targetName, client.TargetHostName);
                 Assert.Equal(targetName, server.TargetHostName);
+                Assert.Equal(1, count);
             }
         }
 

src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs

@@ -8,6 +8,8 @@
 using System.Security.Cryptography.X509Certificates;
 using System.Security.Cryptography.X509Certificates.Tests.Common;
 using System.Text;
+using System.Threading.Tasks;
+using Xunit;
 
 namespace System.Net.Security.Tests
 {
@@ -37,6 +39,9 @@ public static class TestHelper
         private static readonly X509BasicConstraintsExtension s_eeConstraints =
             new X509BasicConstraintsExtension(false, false, 0, false);
 
+        private static readonly byte[] s_ping = Encoding.UTF8.GetBytes("PING");
+        private static readonly byte[] s_pong = Encoding.UTF8.GetBytes("PONG");
+
         public static (SslStream ClientStream, SslStream ServerStream) GetConnectedSslStreams()
         {
             (Stream clientStream, Stream serverStream) = GetConnectedStreams();
@@ -194,5 +199,33 @@ internal static (X509Certificate2 certificate, X509Certificate2Collection) Gener
 
             return (endEntity, chain);
         }
+
+        internal static async Task PingPong(SslStream client, SslStream server)
+        {
+            byte[] buffer = new byte[s_ping.Length];
+            ValueTask t = client.WriteAsync(s_ping);
+
+            int remains = s_ping.Length;
+            while (remains > 0)
+            {
+                int readLength = await server.ReadAsync(buffer, buffer.Length - remains, remains);
+                Assert.True(readLength > 0);
+                remains -= readLength;
+            }
+            Assert.Equal(s_ping, buffer);
+            await t;
+
+            t = server.WriteAsync(s_pong);
+            remains = s_pong.Length;
+            while (remains > 0)
+            {
+                int readLength = await client.ReadAsync(buffer, buffer.Length - remains, remains);
+                Assert.True(readLength > 0);
+                remains -= readLength;
+            }
+
+            Assert.Equal(s_pong, buffer);
+            await t;
+        }
     }
 }