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?