From 175a3eb19364d1ecd69730c09c47bbfd63154e31 Mon Sep 17 00:00:00 2001 From: wfurt Date: Wed, 12 Jun 2024 12:41:56 -0700 Subject: [PATCH 1/3] use also SslCertificateTrust when constructing CertificateContext --- .../SslStreamCertificateContext.Android.cs | 1 + .../SslStreamCertificateContext.Linux.cs | 1 + .../SslStreamCertificateContext.OSX.cs | 1 + .../SslStreamCertificateContext.Windows.cs | 1 + .../Security/SslStreamCertificateContext.cs | 27 +++++++++++++++++-- .../SslStreamNetworkStreamTest.cs | 16 ++++++++--- 6 files changed, 42 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Android.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Android.cs index fec08b42818f32..4edbfaa2672ade 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Android.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Android.cs @@ -9,6 +9,7 @@ namespace System.Net.Security public partial class SslStreamCertificateContext { private const bool TrimRootCertificate = true; + private const bool ChainBuildNeedsTrustedRoot = true; private SslStreamCertificateContext(X509Certificate2 target, ReadOnlyCollection intermediates, SslCertificateTrust? trust) { diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs index 6b91066290b6f0..7f0e2e9a9fad8d 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs @@ -23,6 +23,7 @@ public partial class SslStreamCertificateContext internal static TimeSpan RefreshAfterFailureBackOffInterval => TimeSpan.FromSeconds(5); private const bool TrimRootCertificate = true; + private const bool ChainBuildNeedsTrustedRoot = false; internal readonly ConcurrentDictionary SslContexts; internal readonly SafeX509Handle CertificateHandle; internal readonly SafeEvpPKeyHandle KeyHandle; diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.OSX.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.OSX.cs index 9b79e28ceb213f..73feca46fef6ff 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.OSX.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.OSX.cs @@ -10,6 +10,7 @@ public partial class SslStreamCertificateContext { // No leaf, no root. private const bool TrimRootCertificate = true; + private const bool ChainBuildNeedsTrustedRoot = false; private SslStreamCertificateContext(X509Certificate2 target, ReadOnlyCollection intermediates, SslCertificateTrust? trust) { diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Windows.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Windows.cs index 33d8cd18e2d193..c13424c6fdb8a6 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Windows.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Windows.cs @@ -10,6 +10,7 @@ public partial class SslStreamCertificateContext { // No leaf, include root. private const bool TrimRootCertificate = false; + private const bool ChainBuildNeedsTrustedRoot = false; internal static SslStreamCertificateContext Create(X509Certificate2 target) { diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs index dd148599f48329..195ce945eb73fb 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs @@ -51,9 +51,19 @@ internal static SslStreamCertificateContext Create( { if (additionalCertificates != null) { - foreach (X509Certificate cert in additionalCertificates) + chain.ChainPolicy.ExtraStore.AddRange(additionalCertificates); + } + + if (trust != null) + { + chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust; + if (trust._store != null) + { + chain.ChainPolicy.CustomTrustStore.AddRange(trust._store.Certificates); + } + if (trust._trustList != null) { - chain.ChainPolicy.ExtraStore.Add(cert); + chain.ChainPolicy.CustomTrustStore.AddRange(trust._trustList); } } @@ -67,6 +77,19 @@ internal static SslStreamCertificateContext Create( NetEventSource.Error(null, $"Failed to build chain for {target.Subject}"); } + if (!chainStatus && ChainBuildNeedsTrustedRoot && additionalCertificates != null) + { + // Some platforms like Android may not be able to build the chain unless the chain root is trusted. + // We can try to rebuild the chain with making all extra certificates trused. + // We do not try to evaluate trust here, we jsut need to construct the chain so it should not matter. + chain.ChainPolicy.CustomTrustStore.AddRange(additionalCertificates); + chainStatus = chain.Build(target); + if (!chainStatus && NetEventSource.Log.IsEnabled()) + { + NetEventSource.Error(null, $"Failed to build chain for {target.Subject}"); + } + } + int count = chain.ChainElements.Count - 1; // Some platforms (e.g. Android) can't ignore all verification and will return zero diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs index 052e2d9d3ec8ac..d7b69f953f1dfe 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs @@ -914,19 +914,29 @@ public async Task SslStream_ClientCertificate_SendsChain() } } - [Fact] + [Theory] + [InlineData(true)] + [InlineData(false)] [ActiveIssue("https://github.com/dotnet/runtime/issues/68206", TestPlatforms.Android)] - public async Task SslStream_ClientCertificateContext_SendsChain() + public async Task SslStream_ClientCertificateContext_SendsChain(bool useTrust) { (X509Certificate2 clientCertificate, X509Certificate2Collection clientChain) = Configuration.Certificates.GenerateCertificates(nameof(SslStream_ClientCertificateContext_SendsChain), serverCertificate: false); TestHelper.CleanupCertificates(nameof(SslStream_ClientCertificateContext_SendsChain)); + SslCertificateTrust? trust = null; + if (useTrust) + { + // This is simplification. We make all the intermediates trusted, + // normally just the root would go here. + trust = SslCertificateTrust.CreateForX509Collection(clientChain, false); + } + var clientOptions = new SslClientAuthenticationOptions() { TargetHost = "localhost", }; clientOptions.RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => true; - clientOptions.ClientCertificateContext = SslStreamCertificateContext.Create(clientCertificate, clientChain); + clientOptions.ClientCertificateContext = SslStreamCertificateContext.Create(clientCertificate, useTrust ? null : clientChain, offline:true, trust); await SslStream_ClientSendsChain_Core(clientOptions, clientChain); From 716bb3ccec72afd8f6fa458224179c661c6ad44b Mon Sep 17 00:00:00 2001 From: wfurt Date: Wed, 12 Jun 2024 14:11:47 -0700 Subject: [PATCH 2/3] 'build --- .../tests/UnitTests/System.Net.Security.Unit.Tests.csproj | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Security/tests/UnitTests/System.Net.Security.Unit.Tests.csproj b/src/libraries/System.Net.Security/tests/UnitTests/System.Net.Security.Unit.Tests.csproj index 46e18e22b86ae4..241461ac2817cf 100644 --- a/src/libraries/System.Net.Security/tests/UnitTests/System.Net.Security.Unit.Tests.csproj +++ b/src/libraries/System.Net.Security/tests/UnitTests/System.Net.Security.Unit.Tests.csproj @@ -54,7 +54,6 @@ - @@ -145,7 +144,6 @@ Link="Common\System\Net\Security\TlsAlertMessage.cs" /> - + Link="ProductionCode\System\Net\SslStreamContext.cs" /> + Date: Thu, 20 Jun 2024 11:18:25 -0700 Subject: [PATCH 3/3] feedback --- .../src/System/Net/Security/SslStreamCertificateContext.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs index 195ce945eb73fb..2bcc96b4d9a62e 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs @@ -83,10 +83,11 @@ internal static SslStreamCertificateContext Create( // We can try to rebuild the chain with making all extra certificates trused. // We do not try to evaluate trust here, we jsut need to construct the chain so it should not matter. chain.ChainPolicy.CustomTrustStore.AddRange(additionalCertificates); + chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust; chainStatus = chain.Build(target); if (!chainStatus && NetEventSource.Log.IsEnabled()) { - NetEventSource.Error(null, $"Failed to build chain for {target.Subject}"); + NetEventSource.Error(null, $"Failed to build chain for {target.Subject} while trusting additional certificates"); } }