From d3ee15479145d4e58a4fab36d32db0b6ed6fed47 Mon Sep 17 00:00:00 2001 From: wfurt Date: Mon, 1 Feb 2021 15:54:31 -0800 Subject: [PATCH 1/8] rebuild context if we use client cert from cache --- .../src/System/Net/Security/SecureChannel.cs | 4 ++ .../SslStreamNetworkStreamTest.cs | 58 +++++++++++++++++++ .../tests/FunctionalTests/TestHelper.cs | 12 +++- 3 files changed, 72 insertions(+), 2 deletions(-) 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 index 8e727849a0381..8c54dbbd71921 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs @@ -591,6 +591,10 @@ private bool AcquireClientCredentials(ref byte[]? thumbPrint) _credentialsHandle = cachedCredentialHandle; _selectedClientCertificate = clientCertificate; cachedCred = true; + if (selectedCert != null) + { + _sslAuthenticationOptions.CertificateContext = SslStreamCertificateContext.Create(selectedCert!); + } } else { diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs index 7768f6f0d4a9f..c2c0e2f8040cb 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Collections.Generic; using System.IO; using System.Net.Sockets; using System.Net.Test.Common; @@ -326,6 +327,63 @@ public async Task SslStream_UntrustedCaWithCustomCallback_Throws(bool customCall } } + [Fact] + public async Task SslStream_ClientCertificate_SendsChain() + { + List streams = new List(); + TestHelper.CleanupCertificates("SslStream_ClinetCertificate_SendsChain"); + (X509Certificate2 clientCertificate, X509Certificate2Collection clientChain) = TestHelper.GenerateCertificates("SslStream_ClinetCertificate_SendsChain", serverCertificate: false); + using (X509Store store = new X509Store(StoreName.CertificateAuthority, StoreLocation.CurrentUser)) + { + // add chain certificate so we can construct chain since there is no way how to pass intermediates directly. + store.Open(OpenFlags.ReadWrite); + store.AddRange(clientChain); + } + + var clientOptions = new SslClientAuthenticationOptions() { TargetHost = "localhost", }; + clientOptions.RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => true; + clientOptions.LocalCertificateSelectionCallback = (sender, target, certificates, remoteCertificate, issuers) => clientCertificate; + + var serverOptions = new SslServerAuthenticationOptions() { ClientCertificateRequired = true }; + serverOptions.ServerCertificateContext = SslStreamCertificateContext.Create(Configuration.Certificates.GetServerCertificate(), null); + serverOptions.RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => + { + // Client should send chain without root CA. There is no good way how to know if the chain was built from certificates + // from wire or from system store. However, SslStream adds certificates from wire to ExtraStore in RemoteCertificateValidationCallback. + // So we verify the operation by checking the ExtraStore. + Assert.Equal(clientChain.Count - 1, chain.ChainPolicy.ExtraStore.Count); + return true; + }; + + // run the test multiple times while holding established SSL so we could hit credential cache. + for (int i = 0; i < 3; i++) + { + (Stream clientStream, Stream serverStream) = TestHelper.GetConnectedStreams(); + SslStream client = new SslStream(clientStream); + SslStream server = new SslStream(serverStream); + + Task t1 = client.AuthenticateAsClientAsync(clientOptions, CancellationToken.None); + Task t2 = server.AuthenticateAsServerAsync(serverOptions, CancellationToken.None); + await Task.WhenAll(t1, t2).TimeoutAfter(TestConfiguration.PassingTestTimeoutMilliseconds); + + // hold to the streams so they stay in credential cache + streams.Add(client); + streams.Add(server); + } + + TestHelper.CleanupCertificates("SslStream_ClinetCertificate_SendsChain"); + clientCertificate.Dispose(); + foreach (X509Certificate c in clientChain) + { + c.Dispose(); + } + + foreach (SslStream s in streams) + { + s.Dispose(); + } + } + private static bool ValidateServerCertificate( object sender, X509Certificate retrievedServerPublicCertificate, diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs index ecdfeb47829a4..f262417ff939f 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs @@ -29,6 +29,14 @@ public static class TestHelper }, false); + private static readonly X509EnhancedKeyUsageExtension s_tlsClientEku = + new X509EnhancedKeyUsageExtension( + new OidCollection + { + new Oid("1.3.6.1.5.5.7.3.2", null) + }, + false); + private static readonly X509BasicConstraintsExtension s_eeConstraints = new X509BasicConstraintsExtension(false, false, 0, false); @@ -107,7 +115,7 @@ internal static void CleanupCertificates(string testName) catch { }; } - internal static (X509Certificate2 certificate, X509Certificate2Collection) GenerateCertificates(string targetName, [CallerMemberName] string? testName = null, bool longChain = false) + internal static (X509Certificate2 certificate, X509Certificate2Collection) GenerateCertificates(string targetName, [CallerMemberName] string? testName = null, bool longChain = false, bool serverCertificate = true) { const int keySize = 2048; if (PlatformDetection.IsWindows && testName != null) @@ -123,7 +131,7 @@ internal static (X509Certificate2 certificate, X509Certificate2Collection) Gener extensions.Add(builder.Build()); extensions.Add(s_eeConstraints); extensions.Add(s_eeKeyUsage); - extensions.Add(s_tlsServerEku); + extensions.Add(serverCertificate ? s_tlsServerEku : s_tlsClientEku); CertificateAuthority.BuildPrivatePki( PkiOptions.IssuerRevocationViaCrl, From c3e4a0d572be77a5bfe64418d491951ec4223752 Mon Sep 17 00:00:00 2001 From: wfurt Date: Tue, 2 Feb 2021 00:07:52 -0800 Subject: [PATCH 2/8] adjust expectation for windows --- .../tests/FunctionalTests/SslStreamNetworkStreamTest.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs index c2c0e2f8040cb..d84cac1ffded3 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs @@ -350,8 +350,8 @@ public async Task SslStream_ClientCertificate_SendsChain() { // Client should send chain without root CA. There is no good way how to know if the chain was built from certificates // from wire or from system store. However, SslStream adds certificates from wire to ExtraStore in RemoteCertificateValidationCallback. - // So we verify the operation by checking the ExtraStore. - Assert.Equal(clientChain.Count - 1, chain.ChainPolicy.ExtraStore.Count); + // So we verify the operation by checking the ExtraStore. On Windows, that includes leaf itself. + Assert.True(chain.ChainPolicy.ExtraStore.Count >= clientChain.Count - 1); return true; }; From a6c7edf676169aa34c25cf6ff78c326bfb18b9b8 Mon Sep 17 00:00:00 2001 From: wfurt Date: Tue, 2 Feb 2021 11:51:33 -0800 Subject: [PATCH 3/8] add ITestOutputHelper --- .../FunctionalTests/SslStreamNetworkStreamTest.cs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs index d84cac1ffded3..112d421ba10f9 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs @@ -12,6 +12,7 @@ using System.Threading.Tasks; using Microsoft.DotNet.XUnitExtensions; using Xunit; +using Xunit.Abstractions; namespace System.Net.Security.Tests { @@ -19,6 +20,7 @@ namespace System.Net.Security.Tests public class SslStreamNetworkStreamTest { + readonly ITestOutputHelper _output; private static readonly X509Certificate2 _serverCert; private static readonly X509Certificate2Collection _serverChain; @@ -28,6 +30,11 @@ static SslStreamNetworkStreamTest() (_serverCert, _serverChain) = TestHelper.GenerateCertificates("localhost", nameof(SslStreamNetworkStreamTest), longChain: true); } + SslStreamNetworkStreamTest(ITestOutputHelper output) + { + _output = output; + } + [ConditionalFact] [PlatformSpecific(TestPlatforms.Linux)] // This only applies where OpenSsl is used. public async Task SslStream_SendReceiveOverNetworkStream_AuthenticationException() @@ -351,6 +358,12 @@ public async Task SslStream_ClientCertificate_SendsChain() // Client should send chain without root CA. There is no good way how to know if the chain was built from certificates // from wire or from system store. However, SslStream adds certificates from wire to ExtraStore in RemoteCertificateValidationCallback. // So we verify the operation by checking the ExtraStore. On Windows, that includes leaf itself. + _output.WriteLine("RemoteCertificateValidationCallback called with {0} and {1} extra certificates", sslPolicyErrors, chain.ChainPolicy.ExtraStore.Count); + foreach (X509Certificate c in chain.ChainPolicy.ExtraStore) + { + _output.WriteLine("received {0}", c.Subject); + } + Assert.True(chain.ChainPolicy.ExtraStore.Count >= clientChain.Count - 1); return true; }; From c4d1b66f51e80386ccf31ff17181ee65991937ff Mon Sep 17 00:00:00 2001 From: wfurt Date: Tue, 2 Feb 2021 22:21:25 -0800 Subject: [PATCH 4/8] use fixture to set up certificates --- .../SslStreamNetworkStreamTest.cs | 47 ++++++++++++------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs index 112d421ba10f9..8a5f658167e18 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs @@ -18,21 +18,36 @@ namespace System.Net.Security.Tests { using Configuration = System.Net.Test.Common.Configuration; - public class SslStreamNetworkStreamTest + public class CertificateSetup : IDisposable { - readonly ITestOutputHelper _output; - private static readonly X509Certificate2 _serverCert; - private static readonly X509Certificate2Collection _serverChain; + public readonly X509Certificate2 serverCert; + public readonly X509Certificate2Collection serverChain; - static SslStreamNetworkStreamTest() + public CertificateSetup() { TestHelper.CleanupCertificates(nameof(SslStreamNetworkStreamTest)); - (_serverCert, _serverChain) = TestHelper.GenerateCertificates("localhost", nameof(SslStreamNetworkStreamTest), longChain: true); + (serverCert, serverChain) = TestHelper.GenerateCertificates("localhost", nameof(SslStreamNetworkStreamTest), longChain: true); } - SslStreamNetworkStreamTest(ITestOutputHelper output) + public void Dispose() + { + serverCert.Dispose(); + foreach (var c in serverChain) + { + c.Dispose(); + } + } + } + + public class SslStreamNetworkStreamTest : IClassFixture + { + readonly ITestOutputHelper _output; + readonly CertificateSetup certificates; + + public SslStreamNetworkStreamTest(ITestOutputHelper output, CertificateSetup setup) { _output = output; + certificates = setup; } [ConditionalFact] @@ -228,22 +243,22 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( public async Task SslStream_UntrustedCaWithCustomCallback_OK(bool usePartialChain) { var rnd = new Random(); - int split = rnd.Next(0, _serverChain.Count - 1); + int split = rnd.Next(0, certificates.serverChain.Count - 1); var clientOptions = new SslClientAuthenticationOptions() { TargetHost = "localhost" }; clientOptions.RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => { // add our custom root CA - chain.ChainPolicy.CustomTrustStore.Add(_serverChain[_serverChain.Count - 1]); + chain.ChainPolicy.CustomTrustStore.Add(certificates.serverChain[certificates.serverChain.Count - 1]); chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust; // Add only one CA to verify that peer did send intermediate CA cert. // In case of partial chain, we need to make missing certs available. if (usePartialChain) { - for (int i = split; i < _serverChain.Count - 1; i++) + for (int i = split; i < certificates.serverChain.Count - 1; i++) { - chain.ChainPolicy.ExtraStore.Add(_serverChain[i]); + chain.ChainPolicy.ExtraStore.Add(certificates.serverChain[i]); } } @@ -261,15 +276,15 @@ public async Task SslStream_UntrustedCaWithCustomCallback_OK(bool usePartialChai serverChain = new X509Certificate2Collection(); for (int i = 0; i < split; i++) { - serverChain.Add(_serverChain[i]); + serverChain.Add(certificates.serverChain[i]); } } else { - serverChain = _serverChain; + serverChain = certificates.serverChain; } - serverOptions.ServerCertificateContext = SslStreamCertificateContext.Create(_serverCert, serverChain); + serverOptions.ServerCertificateContext = SslStreamCertificateContext.Create(certificates.serverCert, certificates.serverChain); (Stream clientStream, Stream serverStream) = TestHelper.GetConnectedStreams(); using (clientStream) @@ -299,7 +314,7 @@ public async Task SslStream_UntrustedCaWithCustomCallback_Throws(bool customCall (sender, certificate, chain, sslPolicyErrors) => { // Add only root CA to verify that peer did send intermediate CA cert. - chain.ChainPolicy.CustomTrustStore.Add(_serverChain[_serverChain.Count -1]); + chain.ChainPolicy.CustomTrustStore.Add(certificates.serverChain[certificates.serverChain.Count - 1]); chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust; // This should work and we should be able to trust the chain. Assert.True(chain.Build((X509Certificate2)certificate)); @@ -316,7 +331,7 @@ public async Task SslStream_UntrustedCaWithCustomCallback_Throws(bool customCall } var serverOptions = new SslServerAuthenticationOptions(); - serverOptions.ServerCertificateContext = SslStreamCertificateContext.Create(_serverCert, _serverChain); + serverOptions.ServerCertificateContext = SslStreamCertificateContext.Create(certificates.serverCert, certificates.serverChain); (Stream clientStream, Stream serverStream) = TestHelper.GetConnectedStreams(); using (clientStream) From 500ba6f345e5fca7066de2fc9505b97110ab5cfc Mon Sep 17 00:00:00 2001 From: wfurt Date: Fri, 5 Feb 2021 13:33:19 -0800 Subject: [PATCH 5/8] add chain verification to the test --- .../SslStreamNetworkStreamTest.cs | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs index 8a5f658167e18..3c76d76f6a261 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs @@ -360,6 +360,27 @@ public async Task SslStream_ClientCertificate_SendsChain() // add chain certificate so we can construct chain since there is no way how to pass intermediates directly. store.Open(OpenFlags.ReadWrite); store.AddRange(clientChain); + store.Close(); + } + + // There is race between adding certificate to the store and building chain + // Make sure we can build chain before proceeding to ssl handshale + using (var chain = new X509Chain()) + { + int count = 10; + chain.ChainPolicy.VerificationFlags = X509VerificationFlags.AllFlags; + chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck; + chain.ChainPolicy.DisableCertificateDownloads = false; + bool chainStatus = chain.Build(clientCertificate); + while (chain.ChainElements.Count != (clientChain.Count + 1) && count > 0) + { + Thread.Sleep(100); + count--; + chainStatus = chain.Build(clientCertificate); + } + + // Verify we can construct full chain + Assert.Equal(clientChain.Count + 1, chain.ChainElements.Count); } var clientOptions = new SslClientAuthenticationOptions() { TargetHost = "localhost", }; From 55b137737a939ef09ef7cf1f0a0e6155d48d0079 Mon Sep 17 00:00:00 2001 From: wfurt Date: Fri, 5 Feb 2021 17:37:40 -0800 Subject: [PATCH 6/8] adjust test --- .../tests/FunctionalTests/SslStreamNetworkStreamTest.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs index 3c76d76f6a261..72faca67839d0 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs @@ -367,7 +367,7 @@ public async Task SslStream_ClientCertificate_SendsChain() // Make sure we can build chain before proceeding to ssl handshale using (var chain = new X509Chain()) { - int count = 10; + int count = 25; chain.ChainPolicy.VerificationFlags = X509VerificationFlags.AllFlags; chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck; chain.ChainPolicy.DisableCertificateDownloads = false; @@ -380,7 +380,7 @@ public async Task SslStream_ClientCertificate_SendsChain() } // Verify we can construct full chain - Assert.Equal(clientChain.Count + 1, chain.ChainElements.Count); + Assert.Equal(clientChain.Count + 1, chain.ChainElements.Count, "chain cannot be built"); } var clientOptions = new SslClientAuthenticationOptions() { TargetHost = "localhost", }; @@ -400,7 +400,7 @@ public async Task SslStream_ClientCertificate_SendsChain() _output.WriteLine("received {0}", c.Subject); } - Assert.True(chain.ChainPolicy.ExtraStore.Count >= clientChain.Count - 1); + Assert.True(chain.ChainPolicy.ExtraStore.Count >= clientChain.Count - 1, "client did not sent expected chain"); return true; }; From d2c9e14060eea06f017625744fa66b07bd0421d0 Mon Sep 17 00:00:00 2001 From: wfurt Date: Fri, 5 Feb 2021 19:40:53 -0800 Subject: [PATCH 7/8] fix assert --- .../tests/FunctionalTests/SslStreamNetworkStreamTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs index 72faca67839d0..a93bc3d56a480 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs @@ -380,7 +380,7 @@ public async Task SslStream_ClientCertificate_SendsChain() } // Verify we can construct full chain - Assert.Equal(clientChain.Count + 1, chain.ChainElements.Count, "chain cannot be built"); + Assert.True(chain.ChainElements.Count > clientChain.Count, "chain cannot be built"); } var clientOptions = new SslClientAuthenticationOptions() { TargetHost = "localhost", }; From 4300afbfa359ab2c89df196c44305a63897947ac Mon Sep 17 00:00:00 2001 From: wfurt Date: Sat, 6 Feb 2021 20:50:50 -0800 Subject: [PATCH 8/8] disable test on macOS --- .../tests/FunctionalTests/SslStreamNetworkStreamTest.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs index a93bc3d56a480..445b68a67dcad 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs @@ -350,6 +350,7 @@ public async Task SslStream_UntrustedCaWithCustomCallback_Throws(bool customCall } [Fact] + [ActiveIssue("https://github.com/dotnet/runtime/issues/46837", TestPlatforms.OSX)] public async Task SslStream_ClientCertificate_SendsChain() { List streams = new List();