From f05cefb44cf5677769c5f4ee0962c8802ae18c16 Mon Sep 17 00:00:00 2001 From: Jeffrey Stedfast Date: Wed, 1 Jan 2025 10:41:37 -0500 Subject: [PATCH] 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. --- .../BouncyCastleSecureMimeContext.cs | 41 ++++++++----------- .../Cryptography/DefaultSecureMimeContext.cs | 12 ++++-- .../Cryptography/ApplicationPkcs7MimeTests.cs | 13 ------ UnitTests/Cryptography/SecureMimeTests.cs | 37 +++++++---------- .../Cryptography/X509CertificateGenerator.cs | 6 +-- 5 files changed, 44 insertions(+), 65 deletions(-) diff --git a/MimeKit/Cryptography/BouncyCastleSecureMimeContext.cs b/MimeKit/Cryptography/BouncyCastleSecureMimeContext.cs index d8885c26ae..e5616f4085 100644 --- a/MimeKit/Cryptography/BouncyCastleSecureMimeContext.cs +++ b/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 { /// @@ -68,6 +67,7 @@ namespace MimeKit.Cryptography { /// 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). /// This method is used to build a certificate chain while verifying /// signed content. - /// 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. + /// 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. /// /// The trusted anchors. protected abstract ISet GetTrustedAnchors (); @@ -348,7 +348,7 @@ Stream Sign (CmsSigner signer, Stream content, bool encapsulate, CancellationTok async Task 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 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 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 BuildCertificateChain (X509Certificate certific /// /// is empty or contains a certificate. /// - 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 /// /// is empty or contains a certificate. /// - public async Task ValidateCertificateChainAsync (X509CertificateChain chain, DateTime dateTime, CancellationToken cancellationToken = default) + async Task ValidateCertificateChainAsync (X509CertificateChain chain, DateTime dateTime, CancellationToken cancellationToken = default) { if (chain == null) throw new ArgumentNullException (nameof (chain)); @@ -856,9 +854,8 @@ public async Task 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 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 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 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 { diff --git a/MimeKit/Cryptography/DefaultSecureMimeContext.cs b/MimeKit/Cryptography/DefaultSecureMimeContext.cs index 19606e965b..caca450c85 100644 --- a/MimeKit/Cryptography/DefaultSecureMimeContext.cs +++ b/MimeKit/Cryptography/DefaultSecureMimeContext.cs @@ -409,11 +409,13 @@ protected override AsymmetricKeyParameter GetPrivateKey (ISelector GetTrustedAnchors () { var anchors = new HashSet (); - 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 GetTrustedAnchors () protected override IStore 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 ()) diff --git a/UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs b/UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs index 87bac04e0e..e695b859d4 100644 --- a/UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs +++ b/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] diff --git a/UnitTests/Cryptography/SecureMimeTests.cs b/UnitTests/Cryptography/SecureMimeTests.cs index 385949fdb8..4a0f4de638 100644 --- a/UnitTests/Cryptography/SecureMimeTests.cs +++ b/UnitTests/Cryptography/SecureMimeTests.cs @@ -80,7 +80,8 @@ public abstract class SecureMimeTestsBase public const string ThunderbirdName = "fejj@gnome.org"; 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 mockHttpMessageHandler) + protected void VerifyCrlsResolvedWithBuildCertificateChain (BouncyCastleSecureMimeContext ctx, Mock mockHttpMessageHandler) { var body = new TextPart ("plain") { Text = "This is some cleartext that we'll end up signing..." }; - var certificate = SupportedCertificates.Single(c => c.EmailAddress == "nochain@mimekit.net"); + var certificate = SupportedCertificates.Single (c => c.EmailAddress == "nochain@mimekit.net"); 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 mockHttpMessageHandler) + protected void VerifyCrlsResolved (BouncyCastleSecureMimeContext ctx, Mock mockHttpMessageHandler) { var body = new TextPart ("plain") { Text = "This is some cleartext that we'll end up signing..." }; var certificate = SupportedCertificates.Single (c => c.EmailAddress == "nochain@mimekit.net"); @@ -3136,8 +3134,6 @@ protected void VerifyMimeEncapsulatedSigningWithContext (BouncyCastleSecureMimeC Mock mockHttpMessageHandler) { var cleartext = new TextPart ("plain") { Text = "This is some text that we'll end up signing..." }; - - var certificate = SupportedCertificates.Single (c => c.EmailAddress == "nochain@mimekit.net"); 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