From a41f655468f27325dff62e6a021aa3d05c64e713 Mon Sep 17 00:00:00 2001 From: Tomas Weinfurt Date: Mon, 10 Jul 2023 10:38:39 -0700 Subject: [PATCH] fix IsMutuallyAuthenticated with NegotiateClientCertificateAsync (#88488) --- .../src/System/Net/Security/SslStream.IO.cs | 7 -- .../src/System/Net/Security/SslStream.Protocol.cs | 7 +- .../SslStreamMutualAuthenticationTest.cs | 79 ++++++++++++++++++++++ 3 files changed, 85 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs index f2c4207..8481861 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs @@ -516,13 +516,6 @@ namespace System.Net.Security return true; } - if (_selectedClientCertificate != null && !CertificateValidationPal.IsLocalCertificateUsed(_credentialsHandle, _securityContext!)) - { - // We may select client cert but it may not be used. - // This is primarily an issue on Windows with credential caching. - _selectedClientCertificate = null; - } - #if TARGET_ANDROID // On Android, the remote certificate verification can be invoked from Java TrustManager's callback // during the handshake process. If that has occurred, we shouldn't run the validation again and diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs index 545cdf1..8c258b6 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs @@ -52,7 +52,12 @@ namespace System.Net.Security { get { - return _selectedClientCertificate; + if (_selectedClientCertificate != null && CertificateValidationPal.IsLocalCertificateUsed(_credentialsHandle, _securityContext!)) + { + return _selectedClientCertificate; + } + + return null; } } diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs index 758c72f..7a858a6 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs @@ -40,6 +40,19 @@ namespace System.Net.Security.Tests CertificateContext } + public static TheoryData CertSourceData() + { + TheoryData data = new(); + + foreach (var source in Enum.GetValues()) + { + data.Add(source); + } + + return data; + } + + public static TheoryData BoolAndCertSourceData() { TheoryData data = new(); @@ -143,6 +156,72 @@ namespace System.Net.Security.Tests } } + [ConditionalTheory(typeof(TestConfiguration), nameof(TestConfiguration.SupportsRenegotiation))] + [MemberData(nameof(CertSourceData))] + [PlatformSpecific(TestPlatforms.Windows | TestPlatforms.Linux)] + public async Task SslStream_NegotiateClientCertificate_IsMutuallyAuthenticatedCorrect(ClientCertSource certSource) + { + SslStreamCertificateContext context = SslStreamCertificateContext.Create(_serverCertificate, null); + var clientOptions = new SslClientAuthenticationOptions + { + TargetHost = Guid.NewGuid().ToString("N") + }; + + for (int round = 0; round < 3; round++) + { + (Stream stream1, Stream stream2) = TestHelper.GetConnectedStreams(); + using (var client = new SslStream(stream1, false, AllowAnyCertificate)) + using (var server = new SslStream(stream2, false, AllowAnyCertificate)) + { + + switch (certSource) + { + case ClientCertSource.ClientCertificate: + clientOptions.ClientCertificates = new X509CertificateCollection() { _clientCertificate }; + break; + case ClientCertSource.SelectionCallback: + clientOptions.LocalCertificateSelectionCallback = ClientCertSelectionCallback; + break; + case ClientCertSource.CertificateContext: + clientOptions.ClientCertificateContext = SslStreamCertificateContext.Create(_clientCertificate, new()); + break; + } + + Task t2 = client.AuthenticateAsClientAsync(clientOptions); + Task t1 = server.AuthenticateAsServerAsync(new SslServerAuthenticationOptions + { + ServerCertificateContext = context, + ClientCertificateRequired = false, + EnabledSslProtocols = SslProtocols.Tls12, + + }); + + await TestConfiguration.WhenAllOrAnyFailedWithTimeout(t1, t2); + + if (round >= 0 && server.RemoteCertificate != null) + { + // TLS resumed + Assert.True(client.IsMutuallyAuthenticated, "client.IsMutuallyAuthenticated"); + Assert.True(server.IsMutuallyAuthenticated, "server.IsMutuallyAuthenticated"); + continue; + } + + Assert.False(client.IsMutuallyAuthenticated, "client.IsMutuallyAuthenticated"); + Assert.False(server.IsMutuallyAuthenticated, "server.IsMutuallyAuthenticated"); + + var t = client.ReadAsync(new byte[1]); + await server.NegotiateClientCertificateAsync(); + Assert.NotNull(server.RemoteCertificate); + await server.WriteAsync(new byte[1]); + await t; + + Assert.NotNull(server.RemoteCertificate); + Assert.True(client.IsMutuallyAuthenticated, "client.IsMutuallyAuthenticated"); + Assert.True(server.IsMutuallyAuthenticated, "server.IsMutuallyAuthenticated"); + } + } + } + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindows7))] [ClassData(typeof(SslProtocolSupport.SupportedSslProtocolsTestData))] public async Task SslStream_ResumedSessionsClientCollection_IsMutuallyAuthenticatedCorrect( -- 2.7.4