Skip to content

SMIME Allow domain-bound certificates #1112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from

Conversation

JoeShook
Copy link
Contributor

@JoeShook JoeShook commented Dec 5, 2024

I am using both email-bound and domain-bound certificates for SMIME.
Domain-Bound certificates are defined as a binding the domain name to the subjectAltName extension dNSName in the certificate. The Direct Project sectioni 1.4 is an example of this type of usage.

The logic in MimeKit.Cryptography.GetSubjectEmailAddress ignores domain-bound certificates. I haven't found any specs in pure SMIME that addresses this, so the existing behavior is as expected.

There isn't a straight forward way to override this search feature at the moment so I added logic to look for a DnsName SubjectAltName type as that last chance to match a certificate.

Logic that depends on this like the implementations of SecureMimeContext.Import that rely on X509CertificateRecord.SubjectEmail and then to MimeKit.Cryptography.GetSubjectEmailAddress extension allows me to import a domain-bound certificate and then later to find it.

This change does not break any existing tests. It does work for my domain-bound certificates test externally.

Thoughts?

-- Joe

Joseph Shook and others added 5 commits December 5, 2024 08:46
@jstedfast
Copy link
Owner

jstedfast commented Dec 5, 2024

Seems reasonable to me.

Although on second thought... I wonder if perhaps introducing a second method GetSubjectDnsName() might be better.

Of course, if we do that - then we'd also need to update internal usages of GetSubjectEmailAddress() to fall back to GetSubjectDnsName() if there is no email address.

A convenience method of GetSubjectEmailAddressOrDnsName() could be added.

I'm just wondering if this would break expectations in the public API (not that it is necessarily wrong for MimeKit's usage, but someone using GetSubjectEMailAddress() externally to MimeKit might have expectations that it will not return the DnsName ever).

@JoeShook
Copy link
Contributor Author

JoeShook commented Dec 5, 2024

Yes I struggle with this one also. I did rationalize that any user today would have certificates with an E= in subject, or Subject AltName's rfc822Name and those take presidence over the dNSName.

Another idea would be to add another constructor to SecureMimeDigitalCertificate that takes a provider that can be used when provided. That provider could then call GetSubjectEmailAddressOrDnsName instead of the default GetSubjectEmailAddress extension method.

Oh... That isn't so simple as I have to start chasing it up through BouncyCastleSecureMimeContext.

User must set an environment variable to fail over to domain-bound certificates
@jstedfast
Copy link
Owner

Hmmm, looking at this more I'm not sure how database lookups would work.

I understand that with your implementation, when storing the database record for a certificate that uses the DnsName, it would save the DnsName as the SubjectEmailAddress value in the db record.

BUT... when doing a lookup for a MailboxAddress, for example, the lookup would be for the full email address and not match any DnsName record.

@jstedfast jstedfast self-requested a review December 6, 2024 15:30
@JoeShook
Copy link
Contributor Author

JoeShook commented Dec 6, 2024

I created my own simple in memory implementation of IX509CertificateDatabase for my experiment. Our implementation in production uses a web service and it does the resolution for us.

public IEnumerable<X509CertificateRecord> Find(MailboxAddress mailbox, DateTime now, bool requirePrivateKey, X509CertificateRecordFields fields)
{
    var records = _memoryStore.Where(c => c.SubjectEmail == mailbox.Address || c.SubjectEmail == mailbox.Domain);

    if (requirePrivateKey)
    {
        records = records.Where(c => c.PrivateKey != null);
    }

    return records;
}

If I update Direct Project open source reference implementation to use MimeKit for all SMIME then I am motivated to build on the SqlCertificateDatabase implementation.

The SQL fragment might look something like this:

command.AddParameterWithValue ("@SUBJECTEMAIL", mailbox.Address.ToLowerInvariant ());
command.AddParameterWithValue ("@SUBJECTDOMAIN", mailbox.Domain.ToLowerInvariant ());
query = query.Append ("AND (SUBJECTEMAIL = @SUBJECTEMAIL OR SUBJECTMAIL = @SUBJECTDOMAIN)");

And this PR would not be done until we include a domain bound cert and ensure all existing tests pass along with some new ones for domain-bound cert. Or the SQL update could be a future PR.

@jstedfast
Copy link
Owner

Right, but it's actually a little more complicated than that I think.

I think (correct me if you think I'm wrong), but I think that we should always prefer a full email address match over a domain certificate.

And so what we'd really need to do is something more like this (local changes that I've made to TemporarySecureMimeContext.cs):

X509Certificate GetCmsRecipientCertificate (MailboxAddress mailbox)
{
	var secure = mailbox as SecureMailboxAddress;
	X509Certificate domainCertificate = null;
	var now = DateTime.UtcNow;

	foreach (var certificate in certificates) {
		if (certificate.NotBefore > now || certificate.NotAfter < now)
			continue;

		var keyUsage = certificate.GetKeyUsageFlags ();
		if (keyUsage != 0 && (keyUsage & X509KeyUsageFlags.KeyEncipherment) == 0)
			continue;

		if (secure != null) {
			var fingerprint = certificate.GetFingerprint ();

			if (!fingerprint.Equals (secure.Fingerprint, StringComparison.OrdinalIgnoreCase))
				continue;
		} else {
			var address = certificate.GetSubjectEmailAddress ();

			if (address != null && address.Equals (mailbox.Address, StringComparison.OrdinalIgnoreCase))
				break;

			if (domainCertificate != null)
				continue;

			var domain = certificate.GetSubjectDnsName ();

			if (domain != null && domain.Equals (mailbox.Domain, StringComparison.OrdinalIgnoreCase)) {
				// Cache this certificate. We will only use this if we do not find an exact match based on the full email address.
				domainCertificate = certificate;
			}
		}

		return certificate;
	}

	return domainCertificate;
}

We'd need similar logic for matching in the SQL db.

In other words, it's not correct to simply match the first certificate that we find if it matches either the email address or the domain. Instead, we should check if any certificate matches the email address. If not, only then should we fall back to the domain certificate.

@jstedfast
Copy link
Owner

...and before I forget, matching by domain should probably canonicalize the domain to be either Punycode-encoded or Unicode.

This may actually be a bug in the current SubjectEmailAddress matching logic as well.

@JoeShook
Copy link
Contributor Author

JoeShook commented Dec 6, 2024

...and before I forget, matching by domain should probably canonicalize the domain to be either Punycode-encoded or Unicode.

This may actually be a bug in the current SubjectEmailAddress matching logic as well.

I am going to take a stab at adding the Punycode encoding. And the correct fall back logic. The fall back logic you describe is the defined logic that we must certify for every year under the Direct Project. We are on the same page there.

What I am not sure about yet, is whether the feature flag (env var) is something you think needs to be in place or not?

@jstedfast
Copy link
Owner

I think PR #1113 is probably a better solution.

Let me know how this would work out for you.

@jstedfast jstedfast closed this Dec 7, 2024
jstedfast added a commit that referenced this pull request Dec 9, 2024
* Added support for domain-wide S/MIME certificates

Replaces pr #1112

* Added unit test for smime.db upgrade (v1 -> v2)

* Updated to allow for multiple DnsNames per certificate

Also updated code to use constants for the table and column names.

* Generate a certificate with DnsNames and updated tests

* Fixed WindowsSecureMimeContext to work with domain-bound certificates

Added X509CertificateExtensions.GetSubjectDnsNames()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants