From c3b41948835a9fdea81ba282fddb237241a84693 Mon Sep 17 00:00:00 2001 From: Mridul Pathak Date: Mon, 10 Aug 2026 12:36:24 +0530 Subject: [PATCH] Implemented: Add DKIM signing for outgoing SMTP mail (OFBIZ-13488) OFBiz's outgoing mail (EmailServices.sendMail) was never cryptographically signed, so receiving mail servers had no way to verify a message actually came from the sending domain. This adds DKIM (RFC 6376) signing via org.simplejavamail:utils-mail-dkim, hooked in right before the transport's sendMessage call -- the one place a message is finalized and handed to the wire. Signing config (domain, selector, PKCS#8 PEM private key, enabled flag) lives in a new MailDkimConfig entity, encrypted at rest, resolved inline in EmailServices (no separate resolver class, matching its existing style). Signing fails open for config/key errors -- a missing or invalid MailDkimConfig logs an error and sends the message unsigned rather than blocking mail -- though a write-time signing failure inside the transport's own send call still blocks the send, now with an accurate log message instead of a misleading connection-error one. A companion getDkimDnsRecord service derives the DNS TXT record value an admin needs to publish, from the same stored key, to avoid hand-computing the DKIM public key. --- dependencies.gradle | 1 + framework/common/entitydef/entitymodel.xml | 21 ++ .../common/servicedef/services_email.xml | 8 + .../ofbiz/common/email/EmailServices.java | 122 ++++++- .../common/email/EmailServicesDkimTests.java | 305 ++++++++++++++++++ gradle/libs.versions.toml | 2 + 6 files changed, 458 insertions(+), 1 deletion(-) create mode 100644 framework/common/src/test/java/org/apache/ofbiz/common/email/EmailServicesDkimTests.java diff --git a/dependencies.gradle b/dependencies.gradle index 4d9a331faf9..30586c3e405 100644 --- a/dependencies.gradle +++ b/dependencies.gradle @@ -29,6 +29,7 @@ dependencies { implementation libs.openpdf implementation libs.jakarta.mail.api implementation libs.angus.mail + implementation libs.utils.mail.dkim implementation libs.rome implementation libs.xstream implementation libs.commons.cli diff --git a/framework/common/entitydef/entitymodel.xml b/framework/common/entitydef/entitymodel.xml index 484029341e0..e67a3aa8eaf 100644 --- a/framework/common/entitydef/entitymodel.xml +++ b/framework/common/entitydef/entitymodel.xml @@ -937,4 +937,25 @@ under the License. + + + + Optional link to a MailSmtpConfig row; unset today, ready for per-relay DKIM + once multi-SMTP-config support exists + + Signing domain, e.g. example.com + DKIM selector, e.g. ofbiz + + PKCS#8 PEM RSA private key + + + Y signs outgoing mail; N/unset leaves mail unsigned even if key material is present + + + + + + diff --git a/framework/common/servicedef/services_email.xml b/framework/common/servicedef/services_email.xml index a286320402d..294f46afab6 100644 --- a/framework/common/servicedef/services_email.xml +++ b/framework/common/servicedef/services_email.xml @@ -187,4 +187,12 @@ under the License. Delete a EmailTemplateSetting record + + Derives the DNS TXT record (name and value) to publish for a MailDkimConfig's + signing key, so admins can verify/copy it without hand-computing the DKIM public key. + + + + diff --git a/framework/common/src/main/java/org/apache/ofbiz/common/email/EmailServices.java b/framework/common/src/main/java/org/apache/ofbiz/common/email/EmailServices.java index 940d133141c..1de5b4237c2 100644 --- a/framework/common/src/main/java/org/apache/ofbiz/common/email/EmailServices.java +++ b/framework/common/src/main/java/org/apache/ofbiz/common/email/EmailServices.java @@ -30,6 +30,15 @@ import java.net.URI; import java.net.URISyntaxException; import java.net.URL; +import java.security.GeneralSecurityException; +import java.security.KeyFactory; +import java.security.PrivateKey; +import java.security.PublicKey; +import java.security.interfaces.RSAPrivateCrtKey; +import java.security.spec.InvalidKeySpecException; +import java.security.spec.PKCS8EncodedKeySpec; +import java.security.spec.RSAPublicKeySpec; +import java.util.Base64; import java.util.Date; import java.util.LinkedHashMap; import java.util.LinkedList; @@ -65,7 +74,9 @@ import org.apache.ofbiz.base.util.collections.MapStack; import org.apache.ofbiz.base.util.string.FlexibleStringExpander; import org.apache.ofbiz.entity.Delegator; +import org.apache.ofbiz.entity.GenericEntityException; import org.apache.ofbiz.entity.GenericValue; +import org.apache.ofbiz.entity.util.EntityQuery; import org.apache.ofbiz.entity.util.EntityUtilProperties; import org.apache.ofbiz.service.DispatchContext; import org.apache.ofbiz.service.GenericServiceException; @@ -78,6 +89,11 @@ import org.apache.ofbiz.widget.renderer.ScreenStringRenderer; import org.apache.ofbiz.widget.renderer.VisualTheme; import org.apache.ofbiz.widget.renderer.macro.MacroScreenRenderer; +import org.simplejavamail.utils.mail.dkim.Canonicalization; +import org.simplejavamail.utils.mail.dkim.DkimMessage; +import org.simplejavamail.utils.mail.dkim.DkimSigner; +import org.simplejavamail.utils.mail.dkim.DkimSigningException; +import org.simplejavamail.utils.mail.dkim.SigningAlgorithm; import org.xml.sax.SAXException; import org.eclipse.angus.mail.smtp.SMTPAddressFailedException; @@ -359,10 +375,16 @@ public static Map sendMail(DispatchContext ctx, Map result = ServiceUtil.returnSuccess(); + result.put("recordName", selector + "._domainkey." + domain); + result.put("recordValue", derivePublicKeyRecordValue(privateKey)); + return result; + } catch (GeneralSecurityException e) { + return ServiceUtil.returnError("Could not parse MailDkimConfig [" + mailDkimConfigId + + "] private key: " + e.getMessage()); + } + } + /** class to create a file in memory required for sending as an attachment */ public static class StringDataSource implements DataSource { private String contentType; diff --git a/framework/common/src/test/java/org/apache/ofbiz/common/email/EmailServicesDkimTests.java b/framework/common/src/test/java/org/apache/ofbiz/common/email/EmailServicesDkimTests.java new file mode 100644 index 00000000000..23eb1c9c93d --- /dev/null +++ b/framework/common/src/test/java/org/apache/ofbiz/common/email/EmailServicesDkimTests.java @@ -0,0 +1,305 @@ +/******************************************************************************* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + *******************************************************************************/ +package org.apache.ofbiz.common.email; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.io.ByteArrayOutputStream; +import java.nio.charset.StandardCharsets; +import java.security.GeneralSecurityException; +import java.security.KeyFactory; +import java.security.KeyPair; +import java.security.KeyPairGenerator; +import java.security.PublicKey; +import java.security.interfaces.RSAPrivateCrtKey; +import java.security.spec.X509EncodedKeySpec; +import java.util.Arrays; +import java.util.Base64; +import java.util.Properties; + +import jakarta.mail.Message; +import jakarta.mail.Session; +import jakarta.mail.internet.InternetAddress; +import jakarta.mail.internet.MimeMessage; + +import org.apache.ofbiz.entity.Delegator; +import org.apache.ofbiz.entity.GenericEntityException; +import org.apache.ofbiz.entity.GenericValue; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.simplejavamail.utils.mail.dkim.Canonicalization; + +public final class EmailServicesDkimTests { + + private static KeyPair testKeyPair; + private static String testPrivateKeyPem; + + @BeforeAll + public static void generateTestKeyPair() throws Exception { + KeyPairGenerator generator = KeyPairGenerator.getInstance("RSA"); + generator.initialize(2048); + testKeyPair = generator.generateKeyPair(); + String base64 = Base64.getEncoder().encodeToString(testKeyPair.getPrivate().getEncoded()); + StringBuilder pem = new StringBuilder("-----BEGIN PRIVATE KEY-----\n"); + for (int i = 0; i < base64.length(); i += 64) { + pem.append(base64, i, Math.min(i + 64, base64.length())).append('\n'); + } + pem.append("-----END PRIVATE KEY-----\n"); + testPrivateKeyPem = pem.toString(); + } + + @Test + public void parsePemRsaPrivateKeyParsesValidPem() throws Exception { + RSAPrivateCrtKey parsed = EmailServices.parsePemRsaPrivateKey(testPrivateKeyPem); + RSAPrivateCrtKey original = (RSAPrivateCrtKey) testKeyPair.getPrivate(); + assertEquals(original.getModulus(), parsed.getModulus()); + } + + @Test + public void parsePemRsaPrivateKeyRejectsNonBase64Garbage() { + String badPem = "-----BEGIN PRIVATE KEY-----\nnot valid base64!!!\n-----END PRIVATE KEY-----\n"; + assertThrows(GeneralSecurityException.class, () -> EmailServices.parsePemRsaPrivateKey(badPem)); + } + + @Test + public void parsePemRsaPrivateKeyRejectsWellFormedNonKeyBytes() { + // Valid base64, but not a key -- must fail as GeneralSecurityException, not unchecked. + String fakePem = "-----BEGIN PRIVATE KEY-----\n" + + Base64.getEncoder().encodeToString("this is definitely not a key".getBytes(StandardCharsets.UTF_8)) + + "\n-----END PRIVATE KEY-----\n"; + assertThrows(GeneralSecurityException.class, () -> EmailServices.parsePemRsaPrivateKey(fakePem)); + } + + private static Delegator mockDelegatorReturning(GenericValue... rows) throws GenericEntityException { + Delegator delegator = mock(Delegator.class); + when(delegator.getDelegator()).thenReturn(delegator); + when(delegator.findList(eq("MailDkimConfig"), any(), any(), any(), any(), any(), eq(true))) + .thenReturn(Arrays.asList(rows)); + return delegator; + } + + private static MimeMessage buildTestMessage() throws Exception { + Session session = Session.getInstance(new Properties()); + MimeMessage mail = new MimeMessage(session); + mail.setFrom(new InternetAddress("sender@example.com")); + mail.setRecipients(Message.RecipientType.TO, "recipient@example.com"); + mail.setSubject("Test Subject"); + mail.setText("Test body"); + mail.saveChanges(); + return mail; + } + + @Test + public void dkimSignReturnsOriginalWhenNoConfigRow() throws Exception { + Delegator delegator = mockDelegatorReturning(); + MimeMessage mail = buildTestMessage(); + assertEquals(mail, EmailServices.dkimSign(mail, delegator)); + } + + @Test + public void dkimSignReturnsOriginalWhenNotEnabled() throws Exception { + GenericValue config = mock(GenericValue.class); + when(config.getString("enabled")).thenReturn("N"); + Delegator delegator = mockDelegatorReturning(config); + MimeMessage mail = buildTestMessage(); + assertEquals(mail, EmailServices.dkimSign(mail, delegator)); + } + + @Test + public void dkimSignReturnsOriginalWhenIncompleteConfig() throws Exception { + GenericValue config = mock(GenericValue.class); + when(config.getString("enabled")).thenReturn("Y"); + when(config.getString("domain")).thenReturn("example.com"); + when(config.getString("selector")).thenReturn(""); + when(config.getString("privateKey")).thenReturn(""); + when(config.getString("mailDkimConfigId")).thenReturn("TEST_DKIM_1"); + Delegator delegator = mockDelegatorReturning(config); + MimeMessage mail = buildTestMessage(); + assertEquals(mail, EmailServices.dkimSign(mail, delegator)); + } + + @Test + public void dkimSignReturnsOriginalWhenPrivateKeyIsGarbage() throws Exception { + GenericValue config = mock(GenericValue.class); + when(config.getString("enabled")).thenReturn("Y"); + when(config.getString("domain")).thenReturn("example.com"); + when(config.getString("selector")).thenReturn("ofbiz"); + when(config.getString("privateKey")).thenReturn("not a real key"); + when(config.getString("mailDkimConfigId")).thenReturn("TEST_DKIM_1"); + Delegator delegator = mockDelegatorReturning(config); + MimeMessage mail = buildTestMessage(); + assertEquals(mail, EmailServices.dkimSign(mail, delegator)); + } + + @Test + public void dkimSignWrapsAndSignsWhenFullyConfigured() throws Exception { + GenericValue config = mock(GenericValue.class); + when(config.getString("enabled")).thenReturn("Y"); + when(config.getString("domain")).thenReturn("example.com"); + when(config.getString("selector")).thenReturn("ofbiz"); + when(config.getString("privateKey")).thenReturn(testPrivateKeyPem); + when(config.getString("mailDkimConfigId")).thenReturn("TEST_DKIM_1"); + Delegator delegator = mockDelegatorReturning(config); + MimeMessage mail = buildTestMessage(); + + MimeMessage result = EmailServices.dkimSign(mail, delegator); + + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + result.writeTo(baos); + String raw = baos.toString(StandardCharsets.UTF_8); + assertTrue(raw.contains("DKIM-Signature:"), "expected a DKIM-Signature header, got:\n" + raw); + assertTrue(raw.contains("d=example.com"), "expected d=example.com in signature, got:\n" + raw); + assertTrue(raw.contains("s=ofbiz"), "expected s=ofbiz in signature, got:\n" + raw); + assertTrue(raw.contains("a=rsa-sha256"), "expected a=rsa-sha256 in signature, got:\n" + raw); + assertTrue(raw.contains("c=relaxed/relaxed"), "expected c=relaxed/relaxed in signature, got:\n" + raw); + // Anchored on the real tag separators (" " or "\r\n\t") rather than a bare "l=" substring, + // which could spuriously match unrelated future body/subject/header content. + assertTrue(!raw.contains(" l=") && !raw.contains("\r\n\tl="), + "DKIM-Signature must not include an l= tag (setLengthParam must stay false), got:\n" + raw); + } + + @Test + public void dkimSignatureVerifiesAgainstDerivedPublicKey() throws Exception { + GenericValue config = mock(GenericValue.class); + when(config.getString("enabled")).thenReturn("Y"); + when(config.getString("domain")).thenReturn("example.com"); + when(config.getString("selector")).thenReturn("ofbiz"); + when(config.getString("privateKey")).thenReturn(testPrivateKeyPem); + when(config.getString("mailDkimConfigId")).thenReturn("TEST_DKIM_1"); + Delegator delegator = mockDelegatorReturning(config); + MimeMessage mail = buildTestMessage(); + + MimeMessage result = EmailServices.dkimSign(mail, delegator); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + result.writeTo(baos); + String raw = baos.toString(StandardCharsets.UTF_8); + + int blankLineIndex = raw.indexOf("\r\n\r\n"); + String headerBlock = raw.substring(0, blankLineIndex); + String bodyBlock = raw.substring(blankLineIndex + 4); + + // DkimMessage.writeTo() always writes DKIM-Signature first; capture its full (possibly + // folded) value up to the first CRLF not followed by continuation whitespace. + java.util.regex.Matcher m = java.util.regex.Pattern.compile("^DKIM-Signature: (.*?)\r\n(?=\\S)", java.util.regex.Pattern.DOTALL) + .matcher(headerBlock + "\r\n"); + assertTrue(m.find(), "could not locate DKIM-Signature header in:\n" + headerBlock); + String dkimSignatureRawValue = m.group(1); + + // Anchor on "\r\n\tb=" -- the exact literal DkimSigner.serializeSignature emits before the + // signature (DkimSigner.java:623) -- rather than a bare lastIndexOf("b="). + int bTagIndex = dkimSignatureRawValue.lastIndexOf("\r\n\tb="); + assertTrue(bTagIndex >= 0, "could not find the b= tag boundary in:\n" + dkimSignatureRawValue); + String preSignaturePortion = dkimSignatureRawValue.substring(0, bTagIndex + 5); + String signatureBase64 = dkimSignatureRawValue.substring(bTagIndex + 5).replaceAll("\\s+", ""); + + // DkimSigner's default signed-header set (DEFAULT_HEADERS_TO_SIGN), case-insensitive like the + // library's own TreeSet<>(CASE_INSENSITIVE_ORDER) (DkimSigner.java:115) -- jakarta.mail's own + // header casing varies ("Message-Id" vs "Message-ID"). + java.util.Set headersToSign = new java.util.TreeSet<>(String.CASE_INSENSITIVE_ORDER); + headersToSign.addAll(java.util.Arrays.asList( + "From", "To", "Subject", "Content-Description", "Content-ID", "Content-Type", + "Content-Transfer-Encoding", "Cc", "Date", "In-Reply-To", "List-Subscribe", "List-Post", + "List-Owner", "List-Id", "List-Archive", "List-Help", "List-Unsubscribe", "MIME-Version", + "Message-ID", "Resent-Sender", "Resent-Cc", "Resent-Date", "Resent-To", "Reply-To", + "References", "Resent-Message-ID", "Resent-From", "Sender")); + + // Iterate result (the DkimMessage), not mail: DkimMessage.writeTo() signs itself + // (DkimMessage.java:113), and compileHeadersToSign reverses the header order via add(0, ...). + java.util.List matchedHeaders = new java.util.ArrayList<>(); + java.util.Enumeration allHeaders = result.getAllHeaders(); + while (allHeaders.hasMoreElements()) { + jakarta.mail.Header h = allHeaders.nextElement(); + if (headersToSign.contains(h.getName())) { + matchedHeaders.add(0, h); + } + } + + // Cross-check against the signature's own h= tag first, so a mismatch is a readable diff + // rather than an opaque verify() failure. + StringBuilder reconstructedHeaderNames = new StringBuilder(); + for (jakarta.mail.Header h : matchedHeaders) { + reconstructedHeaderNames.append(h.getName()).append(":"); + } + String hTag = reconstructedHeaderNames.substring(0, reconstructedHeaderNames.length() - 1); + assertTrue(preSignaturePortion.contains("h=" + hTag + ";"), + "reconstructed signed-header list did not match the signature's h= tag -- reconstructed [" + hTag + + "], signature says:\n" + preSignaturePortion); + + StringBuilder signedBytesBuilder = new StringBuilder(); + for (jakarta.mail.Header h : matchedHeaders) { + signedBytesBuilder.append(Canonicalization.RELAXED.canonicalizeHeader(h.getName(), h.getValue())).append("\r\n"); + } + signedBytesBuilder.append(Canonicalization.RELAXED.canonicalizeHeader("DKIM-Signature", preSignaturePortion)); + byte[] signedBytes = signedBytesBuilder.toString().getBytes(StandardCharsets.UTF_8); + + // Sanity check: bh= in the real signature should match our independently-canonicalized body. + String canonicalBody = Canonicalization.RELAXED.canonicalizeBody(bodyBlock); + String expectedBodyHash = Base64.getEncoder().encodeToString( + java.security.MessageDigest.getInstance("SHA-256").digest(canonicalBody.getBytes(StandardCharsets.UTF_8))); + assertTrue(preSignaturePortion.contains("bh=" + expectedBodyHash), + "independently-canonicalized body hash did not match the signature's bh= tag -- got preSignaturePortion:\n" + + preSignaturePortion + "\nexpected bh=" + expectedBodyHash); + + byte[] signatureBytes = Base64.getDecoder().decode(signatureBase64); + + // Verify against the SAME key used to sign (sanity check on our own reconstruction). + java.security.Signature verifier = java.security.Signature.getInstance("SHA256withRSA"); + verifier.initVerify(testKeyPair.getPublic()); + verifier.update(signedBytes); + assertTrue(verifier.verify(signatureBytes), + "reconstructed signed bytes did not verify against the original test key -- our canonicalization " + + "reconstruction is wrong somewhere, not a real signing defect. Report this back rather than " + + "adjusting the assertion."); + + // The point of this test: verify against getDkimDnsRecord's published key too, proving the + // two features agree. + String recordValue = EmailServices.derivePublicKeyRecordValue((RSAPrivateCrtKey) testKeyPair.getPrivate()); + String base64PublicKeyFromRecord = recordValue.substring("v=DKIM1; k=rsa; p=".length()); + java.security.PublicKey publicKeyFromRecord = KeyFactory.getInstance("RSA") + .generatePublic(new java.security.spec.X509EncodedKeySpec(Base64.getDecoder().decode(base64PublicKeyFromRecord))); + java.security.Signature verifier2 = java.security.Signature.getInstance("SHA256withRSA"); + verifier2.initVerify(publicKeyFromRecord); + verifier2.update(signedBytes); + assertTrue(verifier2.verify(signatureBytes), + "DKIM signature did not verify against the public key getDkimDnsRecord would publish for the same " + + "MailDkimConfig -- this would mean the signing feature and the DNS-record helper feature " + + "are inconsistent with each other."); + } + + @Test + public void derivePublicKeyRecordValueMatchesOriginalPublicKey() throws Exception { + RSAPrivateCrtKey privateKey = (RSAPrivateCrtKey) testKeyPair.getPrivate(); + String recordValue = EmailServices.derivePublicKeyRecordValue(privateKey); + + assertTrue(recordValue.startsWith("v=DKIM1; k=rsa; p="), "unexpected record value: " + recordValue); + String base64PublicKey = recordValue.substring("v=DKIM1; k=rsa; p=".length()); + byte[] decoded = Base64.getDecoder().decode(base64PublicKey); + PublicKey reconstructed = KeyFactory.getInstance("RSA").generatePublic(new X509EncodedKeySpec(decoded)); + + assertEquals(testKeyPair.getPublic().getEncoded().length, reconstructed.getEncoded().length); + assertTrue(Arrays.equals(testKeyPair.getPublic().getEncoded(), reconstructed.getEncoded()), + "reconstructed public key bytes did not match the original keypair's public key"); + } +} diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index a377a08245b..0f64d7af0ed 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -29,6 +29,7 @@ icu4j = "76.1" openpdf = "1.4.2" jakarta-mail-api = "2.1.5" angus-mail = "2.0.5" +utils-mail-dkim = "3.2.2" rome = "2.1.0" xstream = "1.4.21" commons-cli = "1.11.0" @@ -127,6 +128,7 @@ icu4j = { module = "com.ibm.icu:icu4j", version.ref = "icu4j" } openpdf = { module = "com.github.librepdf:openpdf", version.ref = "openpdf" } jakarta-mail-api = { module = "jakarta.mail:jakarta.mail-api", version.ref = "jakarta-mail-api" } angus-mail = { module = "org.eclipse.angus:angus-mail", version.ref = "angus-mail" } +utils-mail-dkim = { module = "org.simplejavamail:utils-mail-dkim", version.ref = "utils-mail-dkim" } rome = { module = "com.rometools:rome", version.ref = "rome" } xstream = { module = "com.thoughtworks.xstream:xstream", version.ref = "xstream" } commons-cli = { module = "commons-cli:commons-cli", version.ref = "commons-cli" }