Bugfix: Fixed MessageID not preserved when signing/encrypting with S/MIME and/or DKIM

[honorhs]

hi.
I've found a serious bug in the DKIM authentication process.

This is the typical flow for sending mail

1. An internal messageId is generated and applied via fixingMessageId()

currentEmailBuilder.fixingMessageId("<123@456>");

2. DKIM configuration is performed for security enhancement

currentEmailBuilder.signWithDomainKey(
    DkimConfig.builder()
        .dkimPrivateKeyData(byte[] / File / InputStream)
        .dkimSigningDomain("your_domain.org")
        .dkimSelector("your_selector")
        .build()
);

3. The email is sent

mailer.sendMail(email);

---

However, after step 3, the messageId value inside the email object is changed, even though the messageId was fixed in step 1.

The cause of the issue is the following line of code
message.saveChanges()

Here, the messageId is changed by calling message.saveChanges().

In any case, the messageId inside the email object should not be changed if it already exists.

There are two possible solutions

1. Restore the messageId inside the message object to its original value after calling message.saveChanges().
2. If the messageId already exists, do not call message.saveChanges().
   (There is no need to unnecessarily call saveChanges on the mimeMessage created by convertMimeMessage.)

The second solution seems to be the best

[bbottema]

Thank you for your report. Can you please provide evidence that this is not working? fixingMessageId is specifically made for this kind of situation and has been a core feature for many years. When message.saveChanges() is called, the following override is triggered:

MimeMessage message = new MimeMessage(session) {
			@Override
			protected void updateMessageID() throws MessagingException {
				if (valueNullOrEmpty(email.getId())) {
					super.updateMessageID();
				} else {
					setHeader("Message-ID", email.getId());
				}
			}
			...
		};

This is also extensively automatically tested using a real (but for testing) SMTP server (in MailerLiveTest.java).

However, it's possibly a regression bug, but then I would expect the tests to fail on this feature.

[honorhs]

I see the confusion you're having now.
I think I may have caused confusion due to my inadequate explanation

Please take a look at the code a bit further down.

When returning through ModuleLoader, MimeMessage is overridden again. This means that when using features such as Smime or Dkim, MimeMessage is overridden again. code

if (email.getPkcs12ConfigForSmimeSigning() != null) {
	message = ModuleLoader.loadSmimeModule().signMessageWithSmime(session, message, email.getPkcs12ConfigForSmimeSigning());
}

if (email.getX509CertificateForSmimeEncryption() != null) {
	message = ModuleLoader.loadSmimeModule().encryptMessageWithSmime(session, message, email.getX509CertificateForSmimeEncryption());
}

if (email.getDkimConfig() != null) {
	message = ModuleLoader.loadDKIMModule().signMessageWithDKIM(message, email.getDkimConfig(), checkNonEmptyArgument(email.getFromRecipient(), "fromRecipient"));
}

if (email.getBounceToRecipient() != null) {
	// display name not applicable: https://tools.ietf.org/html/rfc5321#section-4.1.2
	message = new ImmutableDelegatingSMTPMessage(message, email.getBounceToRecipient().getAddress());
}

In other words, if use the Dkim feature, the overriding of the updateMessageID method above becomes useless.

[honorhs]

DKIMSigner. signMessageWithDKIM(...)

....
return new DkimMessage(messageToSign, dkimSigner);

DkimMessage (in utils-mail-dkim-3.0.0.jar)

public class DkimMessage extends SMTPMessage {

	private static byte[] NL = { (byte) '\r', (byte) '\n' };

	private DkimSigner signer;

	private String encodedBody;

	/**
	 * Created a new {@code DkimMessage} from the given {@link MimeMessage} and
	 * {@link DkimSigner}.
	 * 
	 * @param message
	 *            The {@link MimeMessage} to be signed.
	 * @param signer
	 *            The {@link DkimSigner} to sign the message with.
	 * @throws MessagingException
	 *             If constructing this {@code DkimMessage} failed.
	 */
	public DkimMessage(MimeMessage message, DkimSigner signer) throws MessagingException {
		super(message);
		this.signer = signer;
	}
	...

DkimMessage is the result of extending Jakarta MimeMessage.

There is no updated version of 'updateMessageId(..)' that you have redefined here.
It simply creates a new messageId unconditionally.

protected void updateMessageID() throws MessagingException {
	setHeader("Message-ID", 
		  "<" + UniqueValue.getUniqueMessageIDValue(session) + ">");
          
}

[bbottema]

Oh wauw, that's a nice find! I'll see about reproducing this in my tests so that's covered. I'll fix this, but I'm not sure how fast. Might be a week or two.

Thank you for bringing this to my attention.

[honorhs]

Thank you for taking the time to fix the bug.
I hope to see the issue resolved quickly. 👍

[honorhs]

@bbottema
Are there any updates?

[bbottema]

Fix released in v8.1.1. Can you please verify?

[kkrupka]

Just if someone else experience a java.lang.ClassNotFoundException: org.simplejavamail.internal.dkimsupport.DkimMessageIdFixingMimeMessage. Up to now, I used nothing from the dkim-module dependency and therefore haven't listed it in my pom. But with this update the dkim-module is required for the runtime otherwise the above exception is thrown when sending signed emails with S/MIME. Error does not occur on compile time.

[bbottema]

Yikes, you're right! I already know why it happens, but a quick work around is indeed to have both dkim and s/mime moduled on the classpath. I'll send out a fix ASAP.

Thank you so much for bringing this to our attention.

[bbottema]

Just if someone else experience a java.lang.ClassNotFoundException: org.simplejavamail.internal.dkimsupport.DkimMessageIdFixingMimeMessage. Up to now, I used nothing from the dkim-module dependency and therefore haven't listed it in my pom. But with this update the dkim-module is required for the runtime otherwise the above exception is thrown when sending signed emails with S/MIME. Error does not occur on compile time.

Fixed in v8.1.2!

[kkrupka]

You are great. Super quick fix! I can confirm, it works as before (without dkim-module dep).