From 1a9673f97857a6d201c69a8bb76d5605506387b1 Mon Sep 17 00:00:00 2001 From: wfurt Date: Wed, 22 Jul 2020 22:21:25 -0700 Subject: [PATCH 1/5] fix certificate ctx on windows --- .../SslStreamCertificateContext.Linux.cs | 10 +- .../SslStreamCertificateContext.OSX.cs | 9 ++ .../SslStreamCertificateContext.Windows.cs | 93 +++++++++++++++++++ .../Security/SslStreamCertificateContext.cs | 25 +++-- .../SslStreamNetworkStreamTest.cs | 36 ++++--- .../tests/FunctionalTests/TestHelper.cs | 52 ++++++++++- 6 files changed, 193 insertions(+), 32 deletions(-) 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 e4260aeaac7885..0c879db20d15d3 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 @@ -7,6 +7,14 @@ namespace System.Net.Security { public partial class SslStreamCertificateContext { + // No leaf, no root. + private const int _trimChain = 2; + + private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[] intermediates) + { + Certificate = target; + IntermediateCertificates = intermediates; + } + internal static SslStreamCertificateContext Create(X509Certificate2 target) => Create(target, null); - } } 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 d616c9e8e6b9b8..75f6c7fd657eb3 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 @@ -7,6 +7,15 @@ namespace System.Net.Security { public partial class SslStreamCertificateContext { + // No leaf, no root. + private const int _trimChain = 2; + + private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[] intermediates) + { + Certificate = target; + IntermediateCertificates = intermediates; + } + internal static SslStreamCertificateContext Create(X509Certificate2 target) { // On OSX we do not need to build chain unless we are asked for it. 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 5906a13a68b734..69ae22f398555d 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 @@ -7,10 +7,103 @@ namespace System.Net.Security { public partial class SslStreamCertificateContext { + // No leaf, include root. + private const int _trimChain = 1; + internal static SslStreamCertificateContext Create(X509Certificate2 target) { // On Windows we do not need to build chain unless we are asked for it. return new SslStreamCertificateContext(target, Array.Empty()); } + + + private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[] intermediates) + { + if (intermediates.Length > 0) + { + using (X509Chain chain = new X509Chain()) + { + chain.ChainPolicy.VerificationFlags = X509VerificationFlags.AllFlags; + chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck; + chain.ChainPolicy.DisableCertificateDownloads = true; + bool osCanBuildChain = chain.Build(target); + + int count = 0; + foreach (X509ChainStatus status in chain.ChainStatus) + { + if (status.Status.HasFlag(X509ChainStatusFlags.PartialChain) || status.Status.HasFlag(X509ChainStatusFlags.NotSignatureValid)) + { + osCanBuildChain = false; + break; + } + + count++; + } + + // OS failed to build the chain but we have at least some intermediates. + // We will try to add them to "Intermediate Certification Authorities" store. + if (!osCanBuildChain) + { + X509Store? store = new X509Store(StoreName.CertificateAuthority, StoreLocation.LocalMachine); + + try + { + store.Open(OpenFlags.ReadWrite); + } + catch + { + // If using system store fails, try to fall-back to user store. + store.Dispose(); + store = new X509Store(StoreName.CertificateAuthority, StoreLocation.CurrentUser); + try + { + store.Open(OpenFlags.ReadWrite); + } + catch + { + store.Dispose(); + store = null; + if (NetEventSource.IsEnabled) + { + NetEventSource.Error(this, $"Failed to open certificate store for intermediates."); + } + } + } + + if (store != null) + { + using (store) + { + // Add everything except the root + for (int index = count; index < intermediates.Length - 1; index++) + { + store.Add(intermediates[index]); + } + + osCanBuildChain = chain.Build(target); + foreach (X509ChainStatus status in chain.ChainStatus) + { + if (status.Status.HasFlag(X509ChainStatusFlags.PartialChain) || status.Status.HasFlag(X509ChainStatusFlags.NotSignatureValid)) + { + osCanBuildChain = false; + break; + } + } + + if (!osCanBuildChain) + { + // Add also root to Intermediate CA store so OS can complete building chain. + // (This does not make it trusted. + store.Add(intermediates[intermediates.Length - 1]); + } + } + } + } + } + } + + Certificate = target; + IntermediateCertificates = intermediates; + } } } 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 621ab20079d514..054f68642d25e9 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 @@ -18,7 +18,6 @@ public static SslStreamCertificateContext Create(X509Certificate2 target, X509Ce } X509Certificate2[] intermediates = Array.Empty(); - using (X509Chain chain = new X509Chain()) { if (additionalCertificates != null) @@ -32,11 +31,14 @@ public static SslStreamCertificateContext Create(X509Certificate2 target, X509Ce chain.ChainPolicy.VerificationFlags = X509VerificationFlags.AllFlags; chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck; chain.ChainPolicy.DisableCertificateDownloads = offline; - chain.Build(target); + bool chainStatus = chain.Build(target); - // No leaf, no root. - int count = chain.ChainElements.Count - 2; + if (!chainStatus && NetEventSource.IsEnabled) + { + NetEventSource.Error(null, $"Failed to build chain for {target.Subject}"); + } + int count = chain.ChainElements.Count - _trimChain; foreach (X509ChainStatus status in chain.ChainStatus) { if (status.Status.HasFlag(X509ChainStatusFlags.PartialChain)) @@ -48,7 +50,7 @@ public static SslStreamCertificateContext Create(X509Certificate2 target, X509Ce } // Count can be zero for a self-signed certificate, or a cert issued directly from a root. - if (count > 0) + if (count > 0 & chain.ChainElements.Count > 1) { intermediates = new X509Certificate2[count]; for (int i = 0; i < count; i++) @@ -61,19 +63,16 @@ public static SslStreamCertificateContext Create(X509Certificate2 target, X509Ce chain.ChainElements[0].Certificate.Dispose(); // Dispose the last cert, if we didn't include it. - for (int i = count + 1; i < chain.ChainElements.Count; i++) + if (_trimChain == 1) { - chain.ChainElements[i].Certificate.Dispose(); + for (int i = count + 1; i < chain.ChainElements.Count; i++) + { + chain.ChainElements[i].Certificate.Dispose(); + } } } return new SslStreamCertificateContext(target, intermediates); } - - private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[] intermediates) - { - Certificate = target; - IntermediateCertificates = intermediates; - } } } diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs index fffc62a5a36ee2..e8513315222cbd 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs @@ -7,6 +7,7 @@ using System.Security.Authentication; using System.Security.Cryptography.X509Certificates; using System.Text; +using System.Threading; using System.Threading.Tasks; using Xunit; @@ -17,11 +18,16 @@ namespace System.Net.Security.Tests public class SslStreamNetworkStreamTest { private readonly X509Certificate2 _serverCert; - private readonly X509CertificateCollection _serverChain; + private readonly X509Certificate2Collection _serverChain; public SslStreamNetworkStreamTest() { - (_serverCert, _serverChain) = TestHelper.GenerateCertificates("localhost"); + (_serverCert, _serverChain) = TestHelper.GenerateCertificates("localhost", this.GetType().Name); + } + + ~SslStreamNetworkStreamTest() + { + TestHelper.ClenupCertificates(this.GetType().Name); } [Fact] @@ -269,14 +275,12 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( } [Fact] - [PlatformSpecific(TestPlatforms.AnyUnix)] public async Task SslStream_UntrustedCaWithCustomCallback_OK() { - var options = new SslClientAuthenticationOptions() { TargetHost = "localhost" }; - options.RemoteCertificateValidationCallback = + var clientOptions = new SslClientAuthenticationOptions() { TargetHost = "localhost" }; + clientOptions.RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => { - chain.ChainPolicy.ExtraStore.AddRange(_serverChain); chain.ChainPolicy.CustomTrustStore.Add(_serverChain[_serverChain.Count -1]); chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust; @@ -286,28 +290,29 @@ public async Task SslStream_UntrustedCaWithCustomCallback_OK() return result; }; + var serverOptions = new SslServerAuthenticationOptions(); + serverOptions.ServerCertificateContext = SslStreamCertificateContext.Create(_serverCert, _serverChain); + (Stream clientStream, Stream serverStream) = TestHelper.GetConnectedStreams(); using (clientStream) using (serverStream) using (SslStream client = new SslStream(clientStream)) using (SslStream server = new SslStream(serverStream)) { - Task t1 = client.AuthenticateAsClientAsync(options, default); - Task t2 = server.AuthenticateAsServerAsync(_serverCert); + Task t1 = client.AuthenticateAsClientAsync(clientOptions, CancellationToken.None); + Task t2 = server.AuthenticateAsServerAsync(serverOptions, CancellationToken.None); await TestConfiguration.WhenAllOrAnyFailedWithTimeout(t1, t2); } } [Fact] - [PlatformSpecific(TestPlatforms.AnyUnix)] public async Task SslStream_UntrustedCaWithCustomCallback_Throws() { - var options = new SslClientAuthenticationOptions() { TargetHost = "localhost" }; - options.RemoteCertificateValidationCallback = + var clientOptions = new SslClientAuthenticationOptions() { TargetHost = "localhost" }; + clientOptions.RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => { - chain.ChainPolicy.ExtraStore.AddRange(_serverChain); chain.ChainPolicy.CustomTrustStore.Add(_serverChain[_serverChain.Count -1]); chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust; // This should work and we should be able to trust the chain. @@ -316,14 +321,17 @@ public async Task SslStream_UntrustedCaWithCustomCallback_Throws() return false; }; + var serverOptions = new SslServerAuthenticationOptions(); + serverOptions.ServerCertificateContext = SslStreamCertificateContext.Create(_serverCert, _serverChain); + (Stream clientStream, Stream serverStream) = TestHelper.GetConnectedStreams(); using (clientStream) using (serverStream) using (SslStream client = new SslStream(clientStream)) using (SslStream server = new SslStream(serverStream)) { - Task t1 = client.AuthenticateAsClientAsync(options, default); - Task t2 = server.AuthenticateAsServerAsync(_serverCert); + Task t1 = client.AuthenticateAsClientAsync(clientOptions, CancellationToken.None); + Task t2 = server.AuthenticateAsServerAsync(serverOptions, CancellationToken.None); await Assert.ThrowsAsync(() => t1); // Server side should finish since we run custom callback after handshake is done. diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs index 4e9a9a8ea37a7e..5b4e4ec72ea00a 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs @@ -72,14 +72,53 @@ internal static (VirtualNetworkStream ClientStream, VirtualNetworkStream ServerS return (new VirtualNetworkStream(vn, isServer: false), new VirtualNetworkStream(vn, isServer: true)); } - internal static (X509Certificate2 certificate, X509Certificate2Collection) GenerateCertificates(string name, string? testName = null) + internal static void ClenupCertificates(string testName) { - X509Certificate2Collection chain = new X509Certificate2Collection(); + string caName = $"o={testName}"; + try + { + using (X509Store store = new X509Store(StoreName.CertificateAuthority, StoreLocation.LocalMachine)) + { + store.Open(OpenFlags.ReadWrite); + foreach (X509Certificate2 cert in store.Certificates) + { + if (cert.Subject.Contains(caName)) + { + store.Remove(cert); + } + } + } + } + catch { }; + + try + { + using (X509Store store = new X509Store(StoreName.CertificateAuthority, StoreLocation.CurrentUser)) + { + store.Open(OpenFlags.ReadWrite); + foreach (X509Certificate2 cert in store.Certificates) + { + if (cert.Subject.Contains(caName)) + { + store.Remove(cert); + } + } + } + } + catch { }; + } + internal static (X509Certificate2 certificate, X509Certificate2Collection) GenerateCertificates(string targetName, string? testName = null) + { + if (PlatformDetection.IsWindows && testName != null) + { + ClenupCertificates(testName); + } + X509Certificate2Collection chain = new X509Certificate2Collection(); X509ExtensionCollection extensions = new X509ExtensionCollection(); SubjectAlternativeNameBuilder builder = new SubjectAlternativeNameBuilder(); - builder.AddDnsName(name); + builder.AddDnsName(targetName); extensions.Add(builder.Build()); extensions.Add(s_eeConstraints); extensions.Add(s_eeKeyUsage); @@ -91,7 +130,7 @@ internal static (X509Certificate2 certificate, X509Certificate2Collection) Gener out CertificateAuthority root, out CertificateAuthority intermediate, out X509Certificate2 endEntity, - subjectName: name, + subjectName: targetName, testName: testName, keySize: 2048, extensions: extensions); @@ -103,6 +142,11 @@ internal static (X509Certificate2 certificate, X509Certificate2Collection) Gener root.Dispose(); intermediate.Dispose(); + if (PlatformDetection.IsWindows) + { + endEntity = new X509Certificate2(endEntity.Export(X509ContentType.Pfx)); + } + return (endEntity, chain); } } From eed8a7abe65eeaa548f66438047361ba4246eb49 Mon Sep 17 00:00:00 2001 From: wfurt Date: Wed, 22 Jul 2020 23:16:20 -0700 Subject: [PATCH 2/5] fix linux --- .../Net/Security/SslStreamCertificateContext.Linux.cs | 1 + .../src/System/Net/Security/SslStreamCertificateContext.cs | 7 ++----- 2 files changed, 3 insertions(+), 5 deletions(-) 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 0c879db20d15d3..19529caaea01e4 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 @@ -17,4 +17,5 @@ private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[] } internal static SslStreamCertificateContext Create(X509Certificate2 target) => Create(target, null); + } } 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 054f68642d25e9..80f979eed5f896 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 @@ -63,12 +63,9 @@ public static SslStreamCertificateContext Create(X509Certificate2 target, X509Ce chain.ChainElements[0].Certificate.Dispose(); // Dispose the last cert, if we didn't include it. - if (_trimChain == 1) + for (int i = count + 1; i < chain.ChainElements.Count; i++) { - for (int i = count + 1; i < chain.ChainElements.Count; i++) - { - chain.ChainElements[i].Certificate.Dispose(); - } + chain.ChainElements[i].Certificate.Dispose(); } } From b85ff7c1e58e334c72d842afcab54bd7119c9bf0 Mon Sep 17 00:00:00 2001 From: wfurt Date: Mon, 27 Jul 2020 21:16:44 -0700 Subject: [PATCH 3/5] fix osx pal --- .../Security/Pal.OSX/SafeDeleteSslContext.cs | 74 +++++-------------- .../Pal.OSX/SafeFreeSslCredentials.cs | 22 ++---- .../src/System/Net/Security/SecureChannel.cs | 13 +++- .../Security/SslStreamCertificateContext.cs | 6 ++ .../System/Net/Security/SslStreamPal.OSX.cs | 4 +- .../System/Net/Security/SslStreamPal.Unix.cs | 4 +- .../Net/Security/SslStreamPal.Windows.cs | 6 +- 7 files changed, 49 insertions(+), 80 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs index ae52439d162935..06ecc8d9ed1d72 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs @@ -118,9 +118,9 @@ private static SafeSslHandle CreateSslContext(SafeFreeSslCredentials credential, SetProtocols(sslContext, credential.Protocols); } - if (credential.Certificate != null) + if (credential.CertificateContext != null) { - SetCertificate(sslContext, credential.Certificate); + SetCertificate(sslContext, credential.CertificateContext); } Interop.AppleCrypto.SslBreakOnServerAuth(sslContext, true); @@ -315,68 +315,34 @@ private static void SetProtocols(SafeSslHandle sslContext, SslProtocols protocol Interop.AppleCrypto.SslSetMaxProtocolVersion(sslContext, maxProtocolId); } - private static void SetCertificate(SafeSslHandle sslContext, X509Certificate2 certificate) + private static void SetCertificate(SafeSslHandle sslContext, SslStreamCertificateContext context) { Debug.Assert(sslContext != null, "sslContext != null"); - Debug.Assert(certificate != null, "certificate != null"); - Debug.Assert(certificate.HasPrivateKey, "certificate.HasPrivateKey"); - X509Chain chain = TLSCertificateExtensions.BuildNewChain( - certificate, - includeClientApplicationPolicy: false)!; - using (chain) - { - X509ChainElementCollection elements = chain.ChainElements; - - // We need to leave off the EE (first) and root (last) certificate from the intermediates. - X509Certificate2[] intermediateCerts = elements.Count < 3 - ? Array.Empty() - : new X509Certificate2[elements.Count - 2]; - - // Build an array which is [ - // SecIdentityRef for EE cert, - // SecCertificateRef for intermed0, - // SecCertificateREf for intermed1, - // ... - // ] - IntPtr[] ptrs = new IntPtr[intermediateCerts.Length + 1]; - - for (int i = 0; i < intermediateCerts.Length; i++) - { - X509Certificate2 intermediateCert = elements[i + 1].Certificate!; + IntPtr[] ptrs = new IntPtr[context!.IntermediateCertificates!.Length + 1]; - if (intermediateCert.HasPrivateKey) - { - // In the unlikely event that we get a certificate with a private key from - // a chain, clear it to the certificate. - // - // The current value of intermediateCert is still in elements, which will - // get Disposed at the end of this method. The new value will be - // in the intermediate certs array, which also gets serially Disposed. - intermediateCert = new X509Certificate2(intermediateCert.RawData); - } + for (int i = 0; i < context.IntermediateCertificates.Length; i++) + { + X509Certificate2 intermediateCert = context.IntermediateCertificates[i]; - intermediateCerts[i] = intermediateCert; - ptrs[i + 1] = intermediateCert.Handle; + if (intermediateCert.HasPrivateKey) + { + // In the unlikely event that we get a certificate with a private key from + // a chain, clear it to the certificate. + // + // The current value of intermediateCert is still in elements, which will + // get Disposed at the end of this method. The new value will be + // in the intermediate certs array, which also gets serially Disposed. + intermediateCert = new X509Certificate2(intermediateCert.RawData); } - ptrs[0] = certificate.Handle; - - Interop.AppleCrypto.SslSetCertificate(sslContext, ptrs); + ptrs[i + 1] = intermediateCert.Handle; + } - // The X509Chain created all new certs for us, so Dispose them. - // And since the intermediateCerts could have been new instances, Dispose them, too - for (int i = 0; i < elements.Count; i++) - { - elements[i].Certificate!.Dispose(); + ptrs[0] = context!.Certificate!.Handle; - if (i < intermediateCerts.Length) - { - intermediateCerts[i].Dispose(); - } - } - } + Interop.AppleCrypto.SslSetCertificate(sslContext, ptrs); } } } diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeFreeSslCredentials.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeFreeSslCredentials.cs index 9a894cb811221e..0cb9ddbde7e883 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeFreeSslCredentials.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeFreeSslCredentials.cs @@ -10,30 +10,22 @@ namespace System.Net { internal sealed class SafeFreeSslCredentials : SafeFreeCredentials { - public SafeFreeSslCredentials(X509Certificate certificate, SslProtocols protocols, EncryptionPolicy policy) + public SafeFreeSslCredentials(SslStreamCertificateContext? certificateContext, SslProtocols protocols, EncryptionPolicy policy) : base(IntPtr.Zero, true) { - Debug.Assert( - certificate == null || certificate is X509Certificate2, - "Only X509Certificate2 certificates are supported at this time"); - - X509Certificate2? cert = (X509Certificate2?)certificate; - - if (cert != null) + if ( certificateContext != null) { - Debug.Assert(cert.HasPrivateKey, "cert.HasPrivateKey"); - // Make a defensive copy of the certificate. In some async cases the // certificate can have been disposed before being provided to the handshake. // // This meshes with the Unix (OpenSSL) PAL, because it extracts the private key // and cert handle (which get up-reffed) to match the API expectations. - cert = new X509Certificate2(cert); + certificateContext = certificateContext.Duplicate(); - Debug.Assert(cert.HasPrivateKey, "cert clone.HasPrivateKey"); + Debug.Assert(certificateContext.Certificate.HasPrivateKey, "cert clone.HasPrivateKey"); } - Certificate = cert; + CertificateContext = certificateContext; Protocols = protocols; Policy = policy; } @@ -42,13 +34,13 @@ public SafeFreeSslCredentials(X509Certificate certificate, SslProtocols protocol public SslProtocols Protocols { get; } - public X509Certificate2? Certificate { get; } + public SslStreamCertificateContext? CertificateContext { get; } public override bool IsInvalid => false; protected override bool ReleaseHandle() { - Certificate?.Dispose(); + CertificateContext?.Certificate.Dispose(); return true; } } 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 b0af7167142eab..58a973711e98e9 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 @@ -577,14 +577,18 @@ private bool AcquireClientCredentials(ref byte[]? thumbPrint) { if (NetEventSource.Log.IsEnabled()) NetEventSource.Log.UsingCachedCredential(this); - _credentialsHandle = cachedCredentialHandle; _selectedClientCertificate = clientCertificate; cachedCred = true; } else { - _credentialsHandle = SslStreamPal.AcquireCredentialsHandle(selectedCert!, _sslAuthenticationOptions.EnabledSslProtocols, _sslAuthenticationOptions.EncryptionPolicy, _sslAuthenticationOptions.IsServer); + if (selectedCert != null) + { + _sslAuthenticationOptions.CertificateContext = SslStreamCertificateContext.Create(selectedCert!); + } + _credentialsHandle = SslStreamPal.AcquireCredentialsHandle(_sslAuthenticationOptions.CertificateContext, + _sslAuthenticationOptions.EnabledSslProtocols, _sslAuthenticationOptions.EncryptionPolicy, _sslAuthenticationOptions.IsServer); thumbPrint = guessedThumbPrint; // Delay until here in case something above threw. _selectedClientCertificate = clientCertificate; @@ -592,7 +596,7 @@ private bool AcquireClientCredentials(ref byte[]? thumbPrint) } finally { - if (selectedCert != null) + if (selectedCert != null && _sslAuthenticationOptions.CertificateContext != null) { _sslAuthenticationOptions.CertificateContext = SslStreamCertificateContext.Create(selectedCert); } @@ -692,7 +696,8 @@ private bool AcquireServerCredentials(ref byte[]? thumbPrint) } else { - _credentialsHandle = SslStreamPal.AcquireCredentialsHandle(selectedCert, _sslAuthenticationOptions.EnabledSslProtocols, _sslAuthenticationOptions.EncryptionPolicy, _sslAuthenticationOptions.IsServer); + _credentialsHandle = SslStreamPal.AcquireCredentialsHandle(_sslAuthenticationOptions.CertificateContext, _sslAuthenticationOptions.EnabledSslProtocols, + _sslAuthenticationOptions.EncryptionPolicy, _sslAuthenticationOptions.IsServer); thumbPrint = guessedThumbPrint; } 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 80f979eed5f896..989a327c52739b 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 @@ -71,5 +71,11 @@ public static SslStreamCertificateContext Create(X509Certificate2 target, X509Ce return new SslStreamCertificateContext(target, intermediates); } + + internal SslStreamCertificateContext Duplicate() + { + return new SslStreamCertificateContext(new X509Certificate2(Certificate), IntermediateCertificates); + + } } } diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs index 4f5a9906985150..5f1c1bb346676e 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs @@ -53,12 +53,12 @@ public static SecurityStatusPal InitializeSecurityContext( } public static SafeFreeCredentials AcquireCredentialsHandle( - X509Certificate certificate, + SslStreamCertificateContext? certificateContext, SslProtocols protocols, EncryptionPolicy policy, bool isServer) { - return new SafeFreeSslCredentials(certificate, protocols, policy); + return new SafeFreeSslCredentials(certificateContext, protocols, policy); } internal static byte[]? GetNegotiatedApplicationProtocol(SafeDeleteContext? context) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs index 4864d32b63c6ed..aeae1edc9fe946 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs @@ -36,10 +36,10 @@ public static SecurityStatusPal InitializeSecurityContext(ref SafeFreeCredential return HandshakeInternal(credential!, ref context, inputBuffer, ref outputBuffer, sslAuthenticationOptions); } - public static SafeFreeCredentials AcquireCredentialsHandle(X509Certificate? certificate, + public static SafeFreeCredentials AcquireCredentialsHandle(SslStreamCertificateContext? certificateContext, SslProtocols protocols, EncryptionPolicy policy, bool isServer) { - return new SafeFreeSslCredentials(certificate, protocols, policy); + return new SafeFreeSslCredentials(certificateContext?.Certificate, protocols, policy); } public static SecurityStatusPal EncryptMessage(SafeDeleteContext securityContext, ReadOnlyMemory input, int headerSize, int trailerSize, ref byte[] output, out int resultSize) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs index 1056c21ec711ed..21442baf359016 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs @@ -110,12 +110,12 @@ public static SecurityStatusPal InitializeSecurityContext(ref SafeFreeCredential return SecurityStatusAdapterPal.GetSecurityStatusPalFromNativeInt(errorCode); } - public static SafeFreeCredentials AcquireCredentialsHandle(X509Certificate? certificate, SslProtocols protocols, EncryptionPolicy policy, bool isServer) + public static SafeFreeCredentials AcquireCredentialsHandle(SslStreamCertificateContext? certificateContext, SslProtocols protocols, EncryptionPolicy policy, bool isServer) { // New crypto API supports TLS1.3 but it does not allow to force NULL encryption. return !UseNewCryptoApi || policy == EncryptionPolicy.NoEncryption ? - AcquireCredentialsHandleSchannelCred(certificate, protocols, policy, isServer) : - AcquireCredentialsHandleSchCredentials(certificate, protocols, policy, isServer); + AcquireCredentialsHandleSchannelCred(certificateContext?.Certificate, protocols, policy, isServer) : + AcquireCredentialsHandleSchCredentials(certificateContext?.Certificate, protocols, policy, isServer); } // This is legacy crypto API used on .NET Framework and older Windows versions. From 9a604280dbdce5f882bc49d5c148ab29d3605cb4 Mon Sep 17 00:00:00 2001 From: wfurt Date: Tue, 28 Jul 2020 21:39:03 -0700 Subject: [PATCH 4/5] feedback from review --- .../Net/Security/SslStreamCertificateContext.Linux.cs | 3 +-- .../System/Net/Security/SslStreamCertificateContext.OSX.cs | 2 +- .../Net/Security/SslStreamCertificateContext.Windows.cs | 2 +- .../src/System/Net/Security/SslStreamCertificateContext.cs | 4 ++-- .../tests/FunctionalTests/SslStreamNetworkStreamTest.cs | 6 +++--- .../System.Net.Security/tests/FunctionalTests/TestHelper.cs | 6 +++--- 6 files changed, 11 insertions(+), 12 deletions(-) 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 19529caaea01e4..f5e9dd2114d615 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 @@ -7,8 +7,7 @@ namespace System.Net.Security { public partial class SslStreamCertificateContext { - // No leaf, no root. - private const int _trimChain = 2; + private const bool TrimRootCertificate = true; private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[] intermediates) { 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 75f6c7fd657eb3..4579f15407fdea 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 @@ -8,7 +8,7 @@ namespace System.Net.Security public partial class SslStreamCertificateContext { // No leaf, no root. - private const int _trimChain = 2; + private const bool TrimRootCertificate = true; private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[] intermediates) { 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 69ae22f398555d..9793495076c509 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 @@ -8,7 +8,7 @@ namespace System.Net.Security public partial class SslStreamCertificateContext { // No leaf, include root. - private const int _trimChain = 1; + private const bool TrimRootCertificate = 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 989a327c52739b..4cba3232be530f 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 @@ -38,7 +38,7 @@ public static SslStreamCertificateContext Create(X509Certificate2 target, X509Ce NetEventSource.Error(null, $"Failed to build chain for {target.Subject}"); } - int count = chain.ChainElements.Count - _trimChain; + int count = chain.ChainElements.Count - (TrimRootCertificate ? 1 : 2); foreach (X509ChainStatus status in chain.ChainStatus) { if (status.Status.HasFlag(X509ChainStatusFlags.PartialChain)) @@ -50,7 +50,7 @@ public static SslStreamCertificateContext Create(X509Certificate2 target, X509Ce } // Count can be zero for a self-signed certificate, or a cert issued directly from a root. - if (count > 0 & chain.ChainElements.Count > 1) + if (count > 0 && chain.ChainElements.Count > 1) { intermediates = new X509Certificate2[count]; for (int i = 0; i < count; i++) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs index e8513315222cbd..fcf54ddf126307 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs @@ -15,7 +15,7 @@ namespace System.Net.Security.Tests { using Configuration = System.Net.Test.Common.Configuration; - public class SslStreamNetworkStreamTest + public class SslStreamNetworkStreamTest : IDisposable { private readonly X509Certificate2 _serverCert; private readonly X509Certificate2Collection _serverChain; @@ -25,9 +25,9 @@ public SslStreamNetworkStreamTest() (_serverCert, _serverChain) = TestHelper.GenerateCertificates("localhost", this.GetType().Name); } - ~SslStreamNetworkStreamTest() + public void Dispose() { - TestHelper.ClenupCertificates(this.GetType().Name); + TestHelper.CleanupCertificates(this.GetType().Name); } [Fact] diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs index 5b4e4ec72ea00a..b16bcac29825de 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs @@ -72,9 +72,9 @@ internal static (VirtualNetworkStream ClientStream, VirtualNetworkStream ServerS return (new VirtualNetworkStream(vn, isServer: false), new VirtualNetworkStream(vn, isServer: true)); } - internal static void ClenupCertificates(string testName) + internal static void CleanupCertificates(string testName) { - string caName = $"o={testName}"; + string caName = $"O={testName}"; try { using (X509Store store = new X509Store(StoreName.CertificateAuthority, StoreLocation.LocalMachine)) @@ -111,7 +111,7 @@ internal static (X509Certificate2 certificate, X509Certificate2Collection) Gener { if (PlatformDetection.IsWindows && testName != null) { - ClenupCertificates(testName); + CleanupCertificates(testName); } X509Certificate2Collection chain = new X509Certificate2Collection(); From fe706b9531043df8815eda28cf4a3a626553bc5b Mon Sep 17 00:00:00 2001 From: wfurt Date: Tue, 4 Aug 2020 20:22:49 -0700 Subject: [PATCH 5/5] feedback from review --- .../src/System/Net/Security/Pal.OSX/SafeFreeSslCredentials.cs | 2 +- .../System/Net/Security/SslStreamCertificateContext.Windows.cs | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeFreeSslCredentials.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeFreeSslCredentials.cs index 0cb9ddbde7e883..24a4e7be7843a6 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeFreeSslCredentials.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeFreeSslCredentials.cs @@ -13,7 +13,7 @@ internal sealed class SafeFreeSslCredentials : SafeFreeCredentials public SafeFreeSslCredentials(SslStreamCertificateContext? certificateContext, SslProtocols protocols, EncryptionPolicy policy) : base(IntPtr.Zero, true) { - if ( certificateContext != null) + if (certificateContext != null) { // Make a defensive copy of the certificate. In some async cases the // certificate can have been disposed before being provided to the handshake. 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 9793495076c509..0827eca16d572e 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 @@ -16,7 +16,6 @@ internal static SslStreamCertificateContext Create(X509Certificate2 target) return new SslStreamCertificateContext(target, Array.Empty()); } - private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[] intermediates) { if (intermediates.Length > 0)