Include Intermediate Certificate Resolution for S/MIME and Ensure Trust Validation during BuildCertificateChain() #1124

JoeShook · 2024-12-24T04:39:44Z

Added functionality to resolve intermediate certificates when building an S/MIME, ensuring trust validation.
Implemented tests to verify that Certificate Revocation Lists (CRLs) are resolved during S/MIME creation.
Introduced a new "nochain" certificate that lacks the complete certificate chain, reflecting a common scenario where certificates without the full chain are installed.
Enhanced logic to retrieve intermediates from the store and download CRLs, ensuring these processes are exercised and validated.

Include intermediate certificate resolution. Adding tests to ensure CRLs are resolved outside of a revocation test. A new nochain certificate was created where the cert does not contain the whole certificate chain. This way the resolution can be exercised.

jstedfast · 2024-12-24T14:55:25Z

MimeKit/Cryptography/BouncyCastleSecureMimeContext.cs

 				ValidityModel = PkixParameters.PkixValidityModel,
-				IsRevocationEnabled = false,
+				IsRevocationEnabled = CheckCertificateRevocation,
 				Date = DateTime.UtcNow


The idea of BuildCertificateChain wasn't to validate the chain, it was only to build it.

IMHO, if you really think we need to validate the chain when signing a message, then we should probably do it in the Sign/EncapsulatedSign methods, this way we can sync-vs-async download the CRLs.

I believe we should validate during the signing process. This option is essential for my workflows. One of the key advantages of SMIME is the ability to revoke a bad actor at scale. While this might not be as evident in scenarios where certificates are exchanged manually, it makes a lot of sense in my environment where the public certificate is discoverable via DNS.

I fully expected you to push back on the missing async paths.

I just got to a place where you could see the theme of the PR and tests to support it. I am used to writing the chain validation with System.Security.Cryptography. And there is just a lot going on in here to get familiar with. The thing I like about the BouncyCastle technique is I can control the caching. where with the MS stack on Windows it puts discovered intermediates in the machine store and crls in the file system and caches in memory. You can control the machine store and file system but you have to restart the service to clear the in memory cache. Maybe there is a PInvoke method that could punt the cache but I am not sure.

While this might not be as evident in scenarios where certificates are exchanged manually, it makes a lot of sense in my environment where the public certificate is discoverable via DNS.

Are you signing with a certificate & private key discovered via DNS?

I guess that's unexpected for me.

When I think of signing a message, I think: Okay, the user is about to sign with his/her own private key that they 100% control and so they would know if it is invalid (except, I suppose, if it was invalid due to expiration perhaps). But I imagined they would know if they had revoked it, at least.

When encrypting to recipient certificates, it makes more sense to me to validate the certificate chains because those recipients could have revoked certificates. And obviously it also makes sense to me to validate a chain when verifying a signed message that you received.

jstedfast · 2024-12-24T14:56:59Z

MimeKit/Cryptography/BouncyCastleSecureMimeContext.cs

+
+			if (CheckCertificateRevocation) {
+				DownloadCrls (certificate);
+			}


This is what we do not want in this method. If the user is calling an XyzAsync method, we want to make sure that all I/O goes through async channels.

jstedfast · 2024-12-24T14:57:08Z

MimeKit/Cryptography/BouncyCastleSecureMimeContext.cs

+				anchorStore.Add (intermediate);
+				if (CheckCertificateRevocation)
+					DownloadCrls (intermediate);
+			}


MimeKit/Cryptography/BouncyCastleSecureMimeContext.cs

MimeKit/Cryptography/DefaultSecureMimeContext.cs

jstedfast · 2024-12-24T15:40:17Z

Perhaps these methods could be added to BouncyCastleSecureMimeContext and used in the Sign/SignAsync methods with CmsSigner.CertificateChain, like this:

if (CheckCertificateRevocation && !ValidateCertificateChain (signer.CertificateChain, DateTime.UtcNow, cancellationToken)
    throw new SomeSortOfException ();

/// <summary>
/// Validate an S/MIME certificate chain.
/// </summary>
/// <remarks>
/// <para>Validates an S/MIME certificate chain.</para>
/// <para>Downloads the CRLs for each certificate in the chain and then validates that the chain
/// is both valid and that none of the certificates in the chain have been revoked or compromised
/// in any way.</para>
/// </remarks>
/// <returns><c>true</c> if the certificate chain is valid; otherwise, <c>false</c>.</returns>
/// <param name="chain">The S/MIME certificate chain.</param>
/// <param name="dateTime">The date and time to use for validation.</param>
/// <param name="cancellationToken">The cancellation token.</param>
/// <exception cref="ArgumentNullException">
/// <paramref name="chain"/> is <see langword="null"/>.
/// </exception>
/// <exception cref="ArgumentException">
/// <paramref name="chain"/> is empty or contains a <see langword="null"/> certificate.
/// </exception>
public bool ValidateCertificateChain (X509Certificate[] chain, DateTime dateTime, CancellationToken cancellationToken = default)
{
	if (chain == null)
		throw new ArgumentNullException (nameof (chain));

	if (chain.Length == 0)
		throw new ArgumentException ("The certificate chain must contain at least one certificate.", nameof (chain));

	if (chain.Any (certificate => certificate == null))
		throw new ArgumentException ("The certificate chain contains at least one null certificate.", nameof (chain));

	var selector = new X509CertStoreSelector {
		Certificate = chain[0]
	};

	var trustedAnchors = GetTrustedAnchors ();
	foreach (var anchor in trustedAnchors)
		DownloadCrls (anchor.TrustedCert, cancellationToken);

	var intermediates = new X509CertificateStore ();
	foreach (var certificate in chain) {
		DownloadCrls (certificate, cancellationToken);
		intermediates.Add (certificate);
	}

	var parameters = new PkixBuilderParameters (trustedAnchors, selector) {
		ValidityModel = PkixParameters.PkixValidityModel,
		IsRevocationEnabled = true,
		Date = dateTime
	};

	parameters.AddStoreCert (intermediates);
	parameters.AddStoreCrl (GetCertificateRevocationLists ());

	try {
		var builder = new PkixCertPathBuilder ();
		builder.Build (parameters);
		return true;
	} catch {
		return false;
	}
}

/// <summary>
/// Validate an S/MIME certificate chain.
/// </summary>
/// <remarks>
/// <para>Asynchronously validates an S/MIME certificate chain.</para>
/// <para>Downloads the CRLs for each certificate in the chain and then validates that the chain
/// is both valid and that none of the certificates in the chain have been revoked or compromised
/// in any way.</para>
/// </remarks>
/// <returns><c>true</c> if the certificate chain is valid; otherwise, <c>false</c>.</returns>
/// <param name="chain">The S/MIME certificate chain.</param>
/// <param name="dateTime">The date and time to use for validation.</param>
/// <param name="cancellationToken">The cancellation token.</param>
/// <exception cref="ArgumentNullException">
/// <paramref name="chain"/> is <see langword="null"/>.
/// </exception>
/// <exception cref="ArgumentException">
/// <paramref name="chain"/> is empty or contains a <see langword="null"/> certificate.
/// </exception>
public async Task<bool> ValidateCertificateChainAsync (X509Certificate[] chain, DateTime dateTime, CancellationToken cancellationToken = default)
{
	if (chain == null)
		throw new ArgumentNullException (nameof (chain));

	if (chain.Length == 0)
		throw new ArgumentException ("The certificate chain must contain at least one certificate.", nameof (chain));

	if (chain.Any (certificate => certificate == null))
		throw new ArgumentException ("The certificate chain contains at least one null certificate.", nameof (chain));

	var selector = new X509CertStoreSelector {
		Certificate = chain[0]
	};

	var trustedAnchors = GetTrustedAnchors ();
	foreach (var anchor in trustedAnchors)
		await DownloadCrlsAsync (anchor.TrustedCert, cancellationToken).ConfigureAwait (false);

	var intermediates = new X509CertificateStore ();
	foreach (var certificate in chain) {
		await DownloadCrlsAsync (certificate, cancellationToken).ConfigureAwait (false);
		intermediates.Add (certificate);
	}

	var parameters = new PkixBuilderParameters (trustedAnchors, selector) {
		ValidityModel = PkixParameters.PkixValidityModel,
		IsRevocationEnabled = true,
		Date = dateTime
	};

	parameters.AddStoreCert (intermediates);
	parameters.AddStoreCrl (GetCertificateRevocationLists ());

	try {
		var builder = new PkixCertPathBuilder ();
		builder.Build (parameters);
		return true;
	} catch {
		return false;
	}
}

JoeShook · 2024-12-24T17:39:44Z

Probably won't look at this seriously until the end of the week or weekend. But all great feedback.

Thanks.

jstedfast · 2024-12-24T18:08:05Z

No worries. Have a Merry Christmas! 🎄🎁

Move the IO operations to ValidateCertificateChain and ValidateCertificateChainAsync. Removed IO from BuildCertificateChain.

jstedfast · 2024-12-31T15:46:56Z

I'll try to review this today or tomorrow

jstedfast · 2025-01-01T15:02:51Z

MimeKit/Cryptography/BouncyCastleSecureMimeContext.cs

 				ValidityModel = PkixParameters.PkixValidityModel,
-				IsRevocationEnabled = false,
+				IsRevocationEnabled = CheckCertificateRevocation,
 				Date = DateTime.UtcNow


While this might not be as evident in scenarios where certificates are exchanged manually, it makes a lot of sense in my environment where the public certificate is discoverable via DNS.

Are you signing with a certificate & private key discovered via DNS?

I guess that's unexpected for me.

When I think of signing a message, I think: Okay, the user is about to sign with his/her own private key that they 100% control and so they would know if it is invalid (except, I suppose, if it was invalid due to expiration perhaps). But I imagined they would know if they had revoked it, at least.

When encrypting to recipient certificates, it makes more sense to me to validate the certificate chains because those recipients could have revoked certificates. And obviously it also makes sense to me to validate a chain when verifying a signed message that you received.

jstedfast · 2025-01-01T15:43:37Z

Thanks for this work - the S/MIME stuff really needed some attention.

JoeShook · 2025-01-02T15:01:33Z

Are you signing with a certificate & private key discovered via DNS?

I guess that's unexpected for me.

When I think of signing a message, I think: Okay, the user is about to sign with his/her own private key that they 100% control and so they would know if it is invalid (except, I suppose, if it was invalid due to expiration perhaps). But I imagined they would know if they had revoked it, at least.

When encrypting to recipient certificates, it makes more sense to me to validate the certificate chains because those recipients could have revoked certificates. And obviously it also makes sense to me to validate a chain when verifying a signed message that you received.

Ugh! Everything you mentioned is spot on. I got so caught up in getting the trust part of the code to work that I lost track of the overall activity. I go tunnel vision on the trust mechanics and not on where it should be applied. We should probably revert this and reopen the PR.

We discover the public certificates for each recipient over DNS, and that's when trust must be established for each recipient. The work should be to ensure that trust is tested for each recipient in the Envelope method.

jstedfast · 2025-01-03T02:08:05Z

No sense reverting - we can just submit a new PR based on latest master to fix things.

jstedfast · 2025-01-03T17:29:15Z

I think this commit should fix things: f16687a

JoeShook · 2025-01-03T17:52:01Z

I will try to take a look over the weekend and test it out.

jstedfast · 2025-01-03T18:11:31Z

Would be good if you could verify that all of the cases you think need to be tested are tested as well. I got a little carried away removing some of your tests for signing, but then realized that your tests were based on tests I had added to test verifying revoked certificates used in the signature.

jstedfast · 2025-01-11T18:50:04Z

@JoeShook any updates?

JoeShook temporarily deployed to ci December 24, 2024 04:39 — with GitHub Actions Inactive

JoeShook had a problem deploying to ci December 24, 2024 04:40 — with GitHub Actions Failure

JoeShook temporarily deployed to ci December 24, 2024 04:40 — with GitHub Actions Inactive

jstedfast requested changes Dec 24, 2024

View reviewed changes

ValidateCertificateChain

0feb662

Move the IO operations to ValidateCertificateChain and ValidateCertificateChainAsync. Removed IO from BuildCertificateChain.

JoeShook temporarily deployed to ci December 27, 2024 00:09 — with GitHub Actions Inactive

JoeShook had a problem deploying to ci December 27, 2024 00:09 — with GitHub Actions Failure

Fixup CRL tests

e1599c1

JoeShook temporarily deployed to ci December 27, 2024 17:36 — with GitHub Actions Inactive

JoeShook had a problem deploying to ci December 27, 2024 17:36 — with GitHub Actions Failure

JoeShook temporarily deployed to ci December 27, 2024 17:36 — with GitHub Actions Inactive

jstedfast approved these changes Jan 1, 2025

View reviewed changes

jstedfast merged commit ea87b4d into jstedfast:master Jan 1, 2025
6 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include Intermediate Certificate Resolution for S/MIME and Ensure Trust Validation during BuildCertificateChain() #1124

Include Intermediate Certificate Resolution for S/MIME and Ensure Trust Validation during BuildCertificateChain() #1124

JoeShook commented Dec 24, 2024

jstedfast Dec 24, 2024

JoeShook Dec 24, 2024

jstedfast Jan 1, 2025

jstedfast Dec 24, 2024

jstedfast Dec 24, 2024

jstedfast commented Dec 24, 2024

JoeShook commented Dec 24, 2024

jstedfast commented Dec 24, 2024

jstedfast commented Dec 31, 2024

jstedfast Jan 1, 2025

jstedfast commented Jan 1, 2025

JoeShook commented Jan 2, 2025

jstedfast commented Jan 3, 2025

jstedfast commented Jan 3, 2025

JoeShook commented Jan 3, 2025

jstedfast commented Jan 3, 2025

jstedfast commented Jan 11, 2025

Include Intermediate Certificate Resolution for S/MIME and Ensure Trust Validation during BuildCertificateChain() #1124

Include Intermediate Certificate Resolution for S/MIME and Ensure Trust Validation during BuildCertificateChain() #1124

Conversation

JoeShook commented Dec 24, 2024

jstedfast Dec 24, 2024

Choose a reason for hiding this comment

JoeShook Dec 24, 2024

Choose a reason for hiding this comment

jstedfast Jan 1, 2025

Choose a reason for hiding this comment

jstedfast Dec 24, 2024

Choose a reason for hiding this comment

jstedfast Dec 24, 2024

Choose a reason for hiding this comment

jstedfast commented Dec 24, 2024

JoeShook commented Dec 24, 2024

jstedfast commented Dec 24, 2024

jstedfast commented Dec 31, 2024

jstedfast Jan 1, 2025

Choose a reason for hiding this comment

jstedfast commented Jan 1, 2025

JoeShook commented Jan 2, 2025

jstedfast commented Jan 3, 2025

jstedfast commented Jan 3, 2025

JoeShook commented Jan 3, 2025

jstedfast commented Jan 3, 2025

jstedfast commented Jan 11, 2025