do not call RemoteCertificateValidationCallback on the same certifica…

…te again (#42836) * do not call RemoteCertificateValidationCallback on the same certificate agin * feedback from review * feedback from review * make sure we do not use Linq
dotnet · Sep 30, 2020 · e82488d · e82488d
1 parent 118eee9
commit e82488d
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 3 deletions.
diff --git a/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.SslProtocols.cs b/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);
  }
  }
 

diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs
@@ -954,7 +954,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)
  {

diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs
@@ -230,6 +230,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)
@@ -248,7 +249,7 @@ public async Task SslStream_TargetHostName_Succeeds(bool useEmptyName)
  {
  SslStream stream = (SslStream)sender;
  Assert.Equal(targetName, stream.TargetHostName);
-
+ count++;
  return true;
  };
 
@@ -265,8 +266,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);
  }
  }
 

diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs
@@ -9,6 +9,8 @@
 using System.Security.Cryptography.X509Certificates.Tests.Common;
 using System.Runtime.CompilerServices;
 using System.Text;
+using System.Threading.Tasks;
+using Xunit;
 
 namespace System.Net.Security.Tests
 {
@@ -30,6 +32,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();
@@ -151,5 +156,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;
+ }
  }
 }