Make ValidateCertificateChain/Async() private for now

These might be valuable to make public at some point, but when we do,
we might want to provide more info as to why they did not validate.

Unfortunately, BouncyCastle does not make that easy to programatically
determine via their APIs, so maybe we'd have to live with exceptions.
Either way, true/false might not be ideal for a public API.

Unit Test changes are all code formatting/style related changes.

Author: jstedfast

MimeKit/Cryptography/BouncyCastleSecureMimeContext.cs

@@ -27,6 +27,7 @@
 using System;
 using System.IO;
 using System.Net;
+using System.Linq;
 using System.Net.Http;
 using System.Threading;
 using System.Threading.Tasks;
@@ -56,8 +57,6 @@
 using IssuerAndSerialNumber = Org.BouncyCastle.Asn1.Cms.IssuerAndSerialNumber;
 
 using MimeKit.IO;
-using System.Linq;
-using Org.BouncyCastle.Tls;
 
 namespace MimeKit.Cryptography {
 	/// <summary>
@@ -68,6 +67,7 @@ namespace MimeKit.Cryptography {
 	/// </remarks>
 	public abstract class BouncyCastleSecureMimeContext : SecureMimeContext
 	{
+		static readonly X509CertStoreSelector MatchAllCertificates = new X509CertStoreSelector ();
 		static readonly string RsassaPssOid = PkcsObjectIdentifiers.IdRsassaPss.Id;
 		static readonly HttpClient SharedHttpClient = new HttpClient ();
 
@@ -171,9 +171,9 @@ protected virtual HttpClient HttpClient {
 		/// generally issued by a certificate authority (CA).</para>
 		/// <para>This method is used to build a certificate chain while verifying
 		/// signed content.</para>
-		/// <para>It is critical to always load the designated trust anchors
-		/// and not the anchor in the end certificate when building a certificate chain
-		/// to validated trust.</para>
+		/// <para>It is critical to always load the designated trust anchors,
+		/// and not the anchor in the end certificate, when building a certificate chain
+		/// when validating trust.</para>
 		/// </remarks>
 		/// <returns>The trusted anchors.</returns>
 		protected abstract ISet<TrustAnchor> GetTrustedAnchors ();
@@ -348,7 +348,7 @@ Stream Sign (CmsSigner signer, Stream content, bool encapsulate, CancellationTok
 		async Task<Stream> SignAsync (CmsSigner signer, Stream content, bool encapsulate, CancellationToken cancellationToken)
 		{
 			if (CheckCertificateRevocation)
-				await ValidateCertificateChainAsync (signer.CertificateChain, DateTime.UtcNow, cancellationToken);
+				await ValidateCertificateChainAsync (signer.CertificateChain, DateTime.UtcNow, cancellationToken).ConfigureAwait (false);
 
 			var signedData = CreateSignedDataGenerator (signer);
 			var memory = new MemoryBlockStream ();
@@ -713,9 +713,8 @@ protected IList<X509Certificate> BuildCertificateChain (X509Certificate certific
 			var issuerStore = GetTrustedAnchors ();
 			var anchorStore = new X509CertificateStore ();
 
-			foreach (var anchor in issuerStore) {
+			foreach (var anchor in issuerStore)
 				anchorStore.Add (anchor.TrustedCert);
-			}
 
 			var parameters = new PkixBuilderParameters (issuerStore, selector) {
 				ValidityModel = PkixParameters.PkixValidityModel,
@@ -726,7 +725,7 @@ protected IList<X509Certificate> BuildCertificateChain (X509Certificate certific
 
 			var intermediateStore = GetIntermediateCertificates ();
 
-			foreach (var intermediate in intermediateStore.EnumerateMatches (new X509CertStoreSelector ()))
+			foreach (var intermediate in intermediateStore.EnumerateMatches (MatchAllCertificates))
 				anchorStore.Add (intermediate);
 
 			parameters.AddStoreCert (anchorStore);
@@ -761,7 +760,7 @@ protected IList<X509Certificate> BuildCertificateChain (X509Certificate certific
 		/// <exception cref="ArgumentException">
 		/// <paramref name="chain"/> is empty or contains a <see langword="null"/> certificate.
 		/// </exception>
-		public bool ValidateCertificateChain (X509CertificateChain chain, DateTime dateTime, CancellationToken cancellationToken = default)
+		bool ValidateCertificateChain (X509CertificateChain chain, DateTime dateTime, CancellationToken cancellationToken = default)
 		{
 			if (chain == null)
 				throw new ArgumentNullException (nameof (chain));
@@ -780,9 +779,8 @@ public bool ValidateCertificateChain (X509CertificateChain chain, DateTime dateT
 			var issuerStore = GetTrustedAnchors ();
 			var anchorStore = new X509CertificateStore ();
 
-			foreach (var anchor in issuerStore) {
+			foreach (var anchor in issuerStore)
 				anchorStore.Add (anchor.TrustedCert);
-			}
 
 			var parameters = new PkixBuilderParameters (issuerStore, selector) {
 				ValidityModel = PkixParameters.PkixValidityModel,
@@ -798,7 +796,7 @@ public bool ValidateCertificateChain (X509CertificateChain chain, DateTime dateT
 
 			var intermediateStore = GetIntermediateCertificates ();
 
-			foreach (var intermediate in intermediateStore.EnumerateMatches (new X509CertStoreSelector ())) {
+			foreach (var intermediate in intermediateStore.EnumerateMatches (MatchAllCertificates)) {
 				anchorStore.Add (intermediate);
 				if (CheckCertificateRevocation)
 					DownloadCrls (intermediate, cancellationToken);
@@ -837,7 +835,7 @@ public bool ValidateCertificateChain (X509CertificateChain chain, DateTime dateT
 		/// <exception cref="ArgumentException">
 		/// <paramref name="chain"/> is empty or contains a <see langword="null"/> certificate.
 		/// </exception>
-		public async Task<bool> ValidateCertificateChainAsync (X509CertificateChain chain, DateTime dateTime, CancellationToken cancellationToken = default)
+		async Task<bool> ValidateCertificateChainAsync (X509CertificateChain chain, DateTime dateTime, CancellationToken cancellationToken = default)
 		{
 			if (chain == null)
 				throw new ArgumentNullException (nameof (chain));
@@ -856,9 +854,8 @@ public async Task<bool> ValidateCertificateChainAsync (X509CertificateChain chai
 			var issuerStore = GetTrustedAnchors ();
 			var anchorStore = new X509CertificateStore ();
 
-			foreach (var anchor in issuerStore) {
+			foreach (var anchor in issuerStore)
 				anchorStore.Add (anchor.TrustedCert);
-			}
 
 			var parameters = new PkixBuilderParameters (issuerStore, selector) {
 				ValidityModel = PkixParameters.PkixValidityModel,
@@ -874,7 +871,7 @@ public async Task<bool> ValidateCertificateChainAsync (X509CertificateChain chai
 
 			var intermediateStore = GetIntermediateCertificates ();
 
-			foreach (var intermediate in intermediateStore.EnumerateMatches (new X509CertStoreSelector ())) {
+			foreach (var intermediate in intermediateStore.EnumerateMatches (MatchAllCertificates)) {
 				anchorStore.Add (intermediate);
 				if (CheckCertificateRevocation)
 					await DownloadCrlsAsync (intermediate, cancellationToken).ConfigureAwait (false);
@@ -1169,7 +1166,7 @@ static IEnumerable<string> EnumerateCrlDistributionPointUrls (X509Certificate ce
 			}
 		}
 
-		void DownloadCrls (X509Certificate certificate, CancellationToken cancellationToken = default)
+		void DownloadCrls (X509Certificate certificate, CancellationToken cancellationToken)
 		{
 			var nextUpdate = GetNextCertificateRevocationListUpdate (certificate.IssuerDN);
 			var now = DateTime.UtcNow;
@@ -1305,9 +1302,8 @@ DigitalSignatureCollection GetDigitalSignatures (CmsSignedDataParser parser, Can
 					foreach (var anchor in anchors)
 						DownloadCrls (anchor.TrustedCert, cancellationToken);
 
-					foreach (X509Certificate intermediate in intermediates.EnumerateMatches(new X509CertStoreSelector())) {
+					foreach (var intermediate in intermediates.EnumerateMatches (MatchAllCertificates))
 						DownloadCrls (intermediate, cancellationToken);
-					}
 				}
 
 				try {
@@ -1364,9 +1360,8 @@ async Task<DigitalSignatureCollection> GetDigitalSignaturesAsync (CmsSignedDataP
 					foreach (var anchor in anchors)
 						await DownloadCrlsAsync (anchor.TrustedCert, cancellationToken).ConfigureAwait (false);
 
-					foreach (X509Certificate intermediate in intermediates.EnumerateMatches (new X509CertStoreSelector ())) {
-						await DownloadCrlsAsync (intermediate, cancellationToken);
-					}
+					foreach (var intermediate in intermediates.EnumerateMatches (MatchAllCertificates))
+						await DownloadCrlsAsync (intermediate, cancellationToken).ConfigureAwait (false);
 				}
 
 				try {

MimeKit/Cryptography/DefaultSecureMimeContext.cs

@@ -409,11 +409,13 @@ protected override AsymmetricKeyParameter GetPrivateKey (ISelector<X509Certifica
 		protected override ISet<TrustAnchor> GetTrustedAnchors ()
 		{
 			var anchors = new HashSet<TrustAnchor> ();
-			var selector = new X509CertStoreSelector ();
 			var keyUsage = new bool[9];
 
 			keyUsage[(int) X509KeyUsageBits.KeyCertSign] = true;
-			selector.KeyUsage = keyUsage;
+
+			var selector = new X509CertStoreSelector {
+				KeyUsage = keyUsage
+			};
 
 			foreach (var record in dbase.Find (selector, true, X509CertificateRecordFields.Certificate))
 				anchors.Add (new TrustAnchor (record.Certificate, null));
@@ -433,11 +435,13 @@ protected override ISet<TrustAnchor> GetTrustedAnchors ()
 		protected override IStore<X509Certificate> GetIntermediateCertificates ()
 		{
 			var intermediates = new X509CertificateStore ();
-			var selector = new X509CertStoreSelector ();
 			var keyUsage = new bool[9];
 
 			keyUsage[(int) X509KeyUsageBits.KeyCertSign] = true;
-			selector.KeyUsage = keyUsage;
+
+			var selector = new X509CertStoreSelector {
+				KeyUsage = keyUsage
+			};
 
 			foreach (var record in dbase.Find (selector, false, X509CertificateRecordFields.Certificate)) {
 				if (!record.Certificate.IsSelfSigned ())

UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

@@ -35,19 +35,6 @@
 using MimeKit.Cryptography;
 
 using BCX509Certificate = Org.BouncyCastle.X509.X509Certificate;
-using Org.BouncyCastle.Cms;
-using Org.BouncyCastle.Crypto;
-using Org.BouncyCastle.OpenSsl;
-
-using Org.BouncyCastle.Crypto;
-using Org.BouncyCastle.OpenSsl;
-using Org.BouncyCastle.Security;
-using Org.BouncyCastle.X509;
-using Org.BouncyCastle.Pkcs;
-using Org.BouncyCastle.Cms;
-using Org.BouncyCastle.Crypto.Encodings;
-using Org.BouncyCastle.Crypto.Engines;
-using Org.BouncyCastle.Crypto.Parameters;
 
 namespace UnitTests.Cryptography {
 	[TestFixture]

UnitTests/Cryptography/SecureMimeTests.cs

@@ -80,7 +80,8 @@ public abstract class SecureMimeTestsBase
 		public const string ThunderbirdName = "[email protected]";
 
 		public static readonly string[] RelativeConfigFilePaths = {
-			"certificate-authority.cfg", "intermediate1.cfg", "intermediate2.cfg", "dnsnames/smime.cfg", "dsa/smime.cfg", "ec/smime.cfg", "nochain/smime.cfg", "revoked/smime.cfg", "revokednochain/smime.cfg", "rsa/smime.cfg"
+			"certificate-authority.cfg", "intermediate1.cfg", "intermediate2.cfg", "dnsnames/smime.cfg", "dsa/smime.cfg",
+			"ec/smime.cfg", "nochain/smime.cfg", "revoked/smime.cfg", "revokednochain/smime.cfg", "rsa/smime.cfg"
 		};
 
 		public static readonly string[] StartComCertificates = {
@@ -2993,7 +2994,6 @@ protected async Task VerifyRevokedCertificateAsync (BouncyCastleSecureMimeContex
 				AssertCrlsRequested (mockHttpMessageHandler);
 			else 
 				AssertCrlsNotRequested (mockHttpMessageHandler);
-			
 
 			Assert.That (multipart.Count, Is.EqualTo (2), "The multipart/signed has an unexpected number of children.");
 
@@ -3056,11 +3056,10 @@ protected async Task VerifyRevokedCertificateAsync (BouncyCastleSecureMimeContex
 			}
 		}
 
-		protected void VerifyCrlsResolvedWithBuildCertificateChain (BouncyCastleSecureMimeContext ctx,
-			Mock<HttpMessageHandler> mockHttpMessageHandler)
+		protected void VerifyCrlsResolvedWithBuildCertificateChain (BouncyCastleSecureMimeContext ctx, Mock<HttpMessageHandler> mockHttpMessageHandler)
 		{
 			var body = new TextPart ("plain") { Text = "This is some cleartext that we'll end up signing..." };
-			var certificate = SupportedCertificates.Single(c => c.EmailAddress == "[email protected]");
+			var certificate = SupportedCertificates.Single (c => c.EmailAddress == "[email protected]");
 
 			var signer = new CmsSigner (certificate.FileName, "no.secret");
 			var multipart = MultipartSigned.Create (ctx, signer, body);
@@ -3081,8 +3080,7 @@ protected void VerifyCrlsResolvedWithBuildCertificateChain (BouncyCastleSecureMi
 			AssertValidSignatures (ctx, signatures);
 		}
 
-		protected void VerifyCrlsResolved (BouncyCastleSecureMimeContext ctx,
-			Mock<HttpMessageHandler> mockHttpMessageHandler)
+		protected void VerifyCrlsResolved (BouncyCastleSecureMimeContext ctx, Mock<HttpMessageHandler> mockHttpMessageHandler)
 		{
 			var body = new TextPart ("plain") { Text = "This is some cleartext that we'll end up signing..." };
 			var certificate = SupportedCertificates.Single (c => c.EmailAddress == "[email protected]");
@@ -3136,8 +3134,6 @@ protected void VerifyMimeEncapsulatedSigningWithContext (BouncyCastleSecureMimeC
 			Mock<HttpMessageHandler> mockHttpMessageHandler)
 		{
 			var cleartext = new TextPart ("plain") { Text = "This is some text that we'll end up signing..." };
-
-
 			var certificate = SupportedCertificates.Single (c => c.EmailAddress == "[email protected]");
 
 			var self = new MailboxAddress ("MimeKit UnitTests", certificate.EmailAddress);
@@ -3275,10 +3271,9 @@ public void TestVerifyCrlsResolved ()
 		[Test]
 		public void TestMissingRootCrl ()
 		{
-			var responses = new HttpResponseMessage[]
-			{
-			new HttpResponseMessage(HttpStatusCode.OK) { Content = new ByteArrayContent(CurrentCrls[1].GetEncoded()) },
-			new HttpResponseMessage(HttpStatusCode.OK) { Content = new ByteArrayContent(CurrentCrls[2].GetEncoded()) }
+			var responses = new HttpResponseMessage[] {
+				new HttpResponseMessage (HttpStatusCode.OK) { Content = new ByteArrayContent (CurrentCrls[1].GetEncoded ()) },
+				new HttpResponseMessage (HttpStatusCode.OK) { Content = new ByteArrayContent (CurrentCrls[2].GetEncoded ()) }
 			};
 			var crlUrlIndexes = new[] { 1, 2 };
 			var errorContent = RootCertificate.SubjectDN.ToString ();
@@ -3289,10 +3284,9 @@ public void TestMissingRootCrl ()
 		[Test]
 		public void TestMissingPrimaryIntermediateCrl ()
 		{
-			var responses = new HttpResponseMessage[]
-			{
-			new HttpResponseMessage(HttpStatusCode.OK) { Content = new ByteArrayContent(CurrentCrls[0].GetEncoded()) },
-			new HttpResponseMessage(HttpStatusCode.OK) { Content = new ByteArrayContent(CurrentCrls[2].GetEncoded()) }
+			var responses = new HttpResponseMessage[] {
+				new HttpResponseMessage (HttpStatusCode.OK) { Content = new ByteArrayContent (CurrentCrls[0].GetEncoded ()) },
+				new HttpResponseMessage (HttpStatusCode.OK) { Content = new ByteArrayContent (CurrentCrls[2].GetEncoded ()) }
 			};
 			var crlUrlIndexes = new[] { 0, 2 };
 			var errorContent = IntermediateCertificate1.SubjectDN.ToString ();
@@ -3303,10 +3297,9 @@ public void TestMissingPrimaryIntermediateCrl ()
 		[Test]
 		public void TestMissingSecondaryIntermediateCrl ()
 		{
-			var responses = new HttpResponseMessage[]
-			{
-			new HttpResponseMessage(HttpStatusCode.OK) { Content = new ByteArrayContent(CurrentCrls[0].GetEncoded()) },
-			new HttpResponseMessage(HttpStatusCode.OK) { Content = new ByteArrayContent(CurrentCrls[1].GetEncoded()) }
+			var responses = new HttpResponseMessage[] {
+				new HttpResponseMessage (HttpStatusCode.OK) { Content = new ByteArrayContent (CurrentCrls[0].GetEncoded ()) },
+				new HttpResponseMessage (HttpStatusCode.OK) { Content = new ByteArrayContent (CurrentCrls[1].GetEncoded ()) }
 			};
 			var crlUrlIndexes = new[] { 0, 1 };
 			var errorContent = IntermediateCertificate2.SubjectDN.ToString ();
@@ -3365,7 +3358,7 @@ public MySecureMimeContext (string database, string password, Mock<HttpMessageHa
 			{
 				CheckCertificateRevocation = false;
 
-				MockHttpMessageHandler = mockHttpMessageHandler??  CreateMockHttpMessageHandler (RevokedCertificateResponses ());
+				MockHttpMessageHandler = mockHttpMessageHandler ?? CreateMockHttpMessageHandler (RevokedCertificateResponses ());
 				client = new HttpClient (MockHttpMessageHandler.Object);
 			}
 

UnitTests/Cryptography/X509CertificateGenerator.cs

@@ -309,6 +309,7 @@ public GeneratorOptions ()
 			{
 				IssuerPassword = string.Empty;
 				Password = string.Empty;
+				IncludeChain = true;
 			}
 
 			public string BasicConstraints { get; set; }
@@ -327,7 +328,7 @@ public GeneratorOptions ()
 
 			public string SignatureAlgorithm { get; set; }
 
-			public bool IncludeChain { get; set; } = true;
+			public bool IncludeChain { get; set; }
 		}
 
 		public static X509Certificate[] Generate (GeneratorOptions options, PrivateKeyOptions privateKeyOptions, CertificateOptions certificateOptions, out AsymmetricKeyParameter privateKey)
@@ -529,7 +530,7 @@ public static X509Certificate[] Generate (GeneratorOptions options, PrivateKeyOp
 
 
 			for (int i = 0; i < chain.Length; i++) {
-				if(options.IncludeChain)
+				if (options.IncludeChain)
 					chainEntries[i + 1] = new X509CertificateEntry (chain[i]);
 				certificates[i + 1] = chain[i];
 			}
@@ -675,7 +676,6 @@ public static X509Certificate[] Generate (string cfg, out AsymmetricKeyParameter
 						case "includechain":
 							options.IncludeChain = bool.Parse (value);
 							break;
-
 						default:
 							throw new FormatException ($"Unknown [Generator] property: {property}");
 						}