You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by bt...@apache.org on 2025/04/18 06:50:10 UTC
(james-project) branch master updated: [ENHANCEMENT] DKIMHook - improve logging (#2702)
This is an automated email from the ASF dual-hosted git repository.
btellier pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git
The following commit(s) were added to refs/heads/master by this push:
new 7d32661c47 [ENHANCEMENT] DKIMHook - improve logging (#2702)
7d32661c47 is described below
commit 7d32661c47136c8dcf7b873dfc3fe9d447e30f4b
Author: Benoit TELLIER <bt...@linagora.com>
AuthorDate: Fri Apr 18 08:50:05 2025 +0200
[ENHANCEMENT] DKIMHook - improve logging (#2702)
* [ENHANCEMENT] DKIMHook: fix inacurate warning log when multiple signature
The previous 'OR' was logging a failure if the first dtoken is not the
expected one which is misleading.
We also add a debug log upon hook
* [ENHANCEMENT] DKIMHook: log messageId
This ease correlation with 3rd party email system logs
---------
Co-authored-by: Rene Cordier <rc...@linagora.com>
---
.../org/apache/james/DKIMHookIntegrationTest.java | 2 +-
.../java/org/apache/james/smtpserver/DKIMHook.java | 68 +++++++++++-----------
.../org/apache/james/smtpserver/DKIMHookTest.java | 38 ++++++------
3 files changed, 53 insertions(+), 55 deletions(-)
diff --git a/server/apps/memory-app/src/test/java/org/apache/james/DKIMHookIntegrationTest.java b/server/apps/memory-app/src/test/java/org/apache/james/DKIMHookIntegrationTest.java
index ac092e7aae..33bb4a3193 100644
--- a/server/apps/memory-app/src/test/java/org/apache/james/DKIMHookIntegrationTest.java
+++ b/server/apps/memory-app/src/test/java/org/apache/james/DKIMHookIntegrationTest.java
@@ -187,7 +187,7 @@ public class DKIMHookIntegrationTest {
server.write(ByteBuffer.wrap(("QUIT\r\n").getBytes(StandardCharsets.UTF_8)));
assertThat(new String(dataResponse, StandardCharsets.UTF_8))
- .contains("530 DKIM check failed. Wrong d token. Expecting avocat.fr");
+ .contains("530 DKIM check failed. Wrong d token. Expecting [avocat.fr]. Got [preprod-avocat.fr]");
}
@Test
diff --git a/server/protocols/protocols-smtp-dkim/src/main/java/org/apache/james/smtpserver/DKIMHook.java b/server/protocols/protocols-smtp-dkim/src/main/java/org/apache/james/smtpserver/DKIMHook.java
index 9425ba9206..746a86ca42 100644
--- a/server/protocols/protocols-smtp-dkim/src/main/java/org/apache/james/smtpserver/DKIMHook.java
+++ b/server/protocols/protocols-smtp-dkim/src/main/java/org/apache/james/smtpserver/DKIMHook.java
@@ -146,29 +146,19 @@ public class DKIMHook implements JamesMessageHook {
@FunctionalInterface
interface SignatureRecordValidation {
static SignatureRecordValidation and(SignatureRecordValidation a, SignatureRecordValidation b) {
- return (sender, records) -> {
- HookResult hookResult = a.validate(sender, records);
+ return (sender, messageId, records) -> {
+ HookResult hookResult = a.validate(sender, messageId, records);
if (hookResult.equals(HookResult.DECLINED)) {
- return b.validate(sender, records);
+ return b.validate(sender, messageId, records);
}
return hookResult;
};
}
- static SignatureRecordValidation or(SignatureRecordValidation a, SignatureRecordValidation b) {
- return (sender, records) -> {
- HookResult hookResult = a.validate(sender, records);
- if (hookResult.equals(HookResult.DECLINED)) {
- return hookResult;
- }
- return b.validate(sender, records);
- };
- }
-
static SignatureRecordValidation signatureRequired(boolean required) {
- return (sender, records) -> {
+ return (sender, messageId, records) -> {
if (required && (records == null || records.isEmpty())) {
- LOGGER.warn("DKIM check failed. Expecting DKIM signatures. Got none.");
+ LOGGER.warn("DKIM check failed for {}. Expecting DKIM signatures. Got none.", messageId);
return HookResult.builder()
.hookReturnCode(HookReturnCode.deny())
.smtpReturnCode(AUTH_REQUIRED)
@@ -179,24 +169,35 @@ public class DKIMHook implements JamesMessageHook {
};
}
- static SignatureRecordValidation expectedDToken(String dToken) {
- return (sender, records) -> {
- if (records.stream().anyMatch(record -> record.getDToken().equals(dToken))) {
- return HookResult.DECLINED;
- }
- LOGGER.warn("DKIM check failed. Wrong d token. Expecting {}. Got {}.", dToken,
- records.stream().map(SignatureRecord::getDToken).collect(ImmutableSet.toImmutableSet()));
- return HookResult.builder()
- .hookReturnCode(HookReturnCode.deny())
- .smtpReturnCode(AUTH_REQUIRED)
- .smtpDescription("DKIM check failed. Wrong d token. Expecting " + dToken)
- .build();
- };
+ SignatureRecordValidation ALLOW_ALL = (sender, messageId, records) -> HookResult.DECLINED;
+
+ HookResult validate(MaybeSender maybeSender, String messageId, List<SignatureRecord> records);
+ }
+
+ static class DTokenSignatureRecordValidation implements SignatureRecordValidation {
+ private final List<String> expectedDTokens;
+
+ DTokenSignatureRecordValidation(List<String> expectedDTokens) {
+ this.expectedDTokens = expectedDTokens;
}
- SignatureRecordValidation ALLOW_ALL = (sender, records) -> HookResult.DECLINED;
+ DTokenSignatureRecordValidation(String expectedDToken) {
+ this(ImmutableList.of(expectedDToken));
+ }
- HookResult validate(MaybeSender maybeSender, List<SignatureRecord> records);
+ @Override
+ public HookResult validate(MaybeSender maybeSender, String messageId, List<SignatureRecord> records) {
+ if (records.stream().anyMatch(r -> expectedDTokens.stream().anyMatch(d -> r.getDToken().equals(d)))) {
+ return HookResult.DECLINED;
+ }
+ ImmutableSet<CharSequence> actualDToken = records.stream().map(SignatureRecord::getDToken).collect(ImmutableSet.toImmutableSet());
+ LOGGER.warn("DKIM check failed for {}. Wrong d token. Expecting {}. Got {}.", messageId, expectedDTokens, actualDToken);
+ return HookResult.builder()
+ .hookReturnCode(HookReturnCode.deny())
+ .smtpReturnCode(AUTH_REQUIRED)
+ .smtpDescription("DKIM check failed. Wrong d token. Expecting " + expectedDTokens + ". Got " + actualDToken)
+ .build();
+ }
}
public static class Config {
@@ -260,10 +261,7 @@ public class DKIMHook implements JamesMessageHook {
SignatureRecordValidation signatureRecordValidation() {
return SignatureRecordValidation.and(
SignatureRecordValidation.signatureRequired(signatureRequired),
- expectedDToken.map(tokens -> tokens.stream()
- .map(SignatureRecordValidation::expectedDToken)
- .reduce(SignatureRecordValidation::or)
- .get()).orElse(SignatureRecordValidation.ALLOW_ALL));
+ expectedDToken.<SignatureRecordValidation>map(DTokenSignatureRecordValidation::new).orElse(SignatureRecordValidation.ALLOW_ALL));
}
private ImmutableList<DKIMCheckNeeded> computeDKIMChecksNeeded(Domain domain) {
@@ -326,7 +324,7 @@ public class DKIMHook implements JamesMessageHook {
return HookResult.DECLINED;
}
try {
- return signatureRecordValidation.validate(mail.getMaybeSender(),
+ return signatureRecordValidation.validate(mail.getMaybeSender(), mail.getMessage().getMessageID(),
verifier.verify(mail.getMessage(), config.forceCRLF));
} catch (MessagingException e) {
LOGGER.warn("Error while verifying DKIM signatures", e);
diff --git a/server/protocols/protocols-smtp-dkim/src/test/java/org/apache/james/smtpserver/DKIMHookTest.java b/server/protocols/protocols-smtp-dkim/src/test/java/org/apache/james/smtpserver/DKIMHookTest.java
index 710dc09bf5..3838c5a967 100644
--- a/server/protocols/protocols-smtp-dkim/src/test/java/org/apache/james/smtpserver/DKIMHookTest.java
+++ b/server/protocols/protocols-smtp-dkim/src/test/java/org/apache/james/smtpserver/DKIMHookTest.java
@@ -122,7 +122,7 @@ class DKIMHookTest {
@Test
void shouldDenyWhenNoDkimRecordsWhenTrue() {
assertThat(DKIMHook.SignatureRecordValidation.signatureRequired(true)
- .validate(MaybeSender.getMailSender("bob@localhost"), ImmutableList.of()))
+ .validate(MaybeSender.getMailSender("bob@localhost"), "messageId", ImmutableList.of()))
.satisfies(hookResult -> assertThat(hookResult.getResult()).isEqualTo(HookReturnCode.deny()))
.satisfies(hookResult -> assertThat(hookResult.getSmtpDescription()).isEqualTo("DKIM check failed. Expecting DKIM signatures. Got none."));
}
@@ -130,28 +130,28 @@ class DKIMHookTest {
@Test
void shouldDeclineWhenNoDkimRecordWhenTrue() {
assertThat(DKIMHook.SignatureRecordValidation.signatureRequired(false)
- .validate(MaybeSender.getMailSender("bob@localhost"), ImmutableList.of()))
+ .validate(MaybeSender.getMailSender("bob@localhost"), "messageId", ImmutableList.of()))
.satisfies(hookResult -> assertThat(hookResult.getResult()).isEqualTo(HookReturnCode.declined()));
}
@Test
void shouldDeclineWhenDkimRecordWhenTrue() {
assertThat(DKIMHook.SignatureRecordValidation.signatureRequired(true)
- .validate(MaybeSender.getMailSender("bob@localhost"), ImmutableList.of(SIGNATURE_RECORD_1)))
+ .validate(MaybeSender.getMailSender("bob@localhost"), "messageId", ImmutableList.of(SIGNATURE_RECORD_1)))
.satisfies(hookResult -> assertThat(hookResult.getResult()).isEqualTo(HookReturnCode.declined()));
}
@Test
void allShouldDeclineWhenNoDkimRecord() {
assertThat(DKIMHook.SignatureRecordValidation.ALLOW_ALL
- .validate(MaybeSender.getMailSender("bob@localhost"), ImmutableList.of()))
+ .validate(MaybeSender.getMailSender("bob@localhost"), "messageId", ImmutableList.of()))
.satisfies(hookResult -> assertThat(hookResult.getResult()).isEqualTo(HookReturnCode.declined()));
}
@Test
void allShouldDeclineWhenDkimRecord() {
assertThat(DKIMHook.SignatureRecordValidation.ALLOW_ALL
- .validate(MaybeSender.getMailSender("bob@localhost"), ImmutableList.of(SIGNATURE_RECORD_1)))
+ .validate(MaybeSender.getMailSender("bob@localhost"), "messageId", ImmutableList.of(SIGNATURE_RECORD_1)))
.satisfies(hookResult -> assertThat(hookResult.getResult()).isEqualTo(HookReturnCode.declined()));
}
@@ -160,7 +160,7 @@ class DKIMHookTest {
assertThat(DKIMHook.SignatureRecordValidation.and(
DKIMHook.SignatureRecordValidation.ALLOW_ALL,
DKIMHook.SignatureRecordValidation.ALLOW_ALL)
- .validate(MaybeSender.getMailSender("bob@localhost"), ImmutableList.of()))
+ .validate(MaybeSender.getMailSender("bob@localhost"), "messageId", ImmutableList.of()))
.satisfies(hookResult -> assertThat(hookResult.getResult()).isEqualTo(HookReturnCode.declined()));
}
@@ -169,7 +169,7 @@ class DKIMHookTest {
assertThat(DKIMHook.SignatureRecordValidation.and(
DKIMHook.SignatureRecordValidation.signatureRequired(true),
DKIMHook.SignatureRecordValidation.ALLOW_ALL)
- .validate(MaybeSender.getMailSender("bob@localhost"), ImmutableList.of()))
+ .validate(MaybeSender.getMailSender("bob@localhost"), "messageId", ImmutableList.of()))
.satisfies(hookResult -> assertThat(hookResult.getResult()).isEqualTo(HookReturnCode.deny()))
.satisfies(hookResult -> assertThat(hookResult.getSmtpDescription()).isEqualTo("DKIM check failed. Expecting DKIM signatures. Got none."));
}
@@ -179,7 +179,7 @@ class DKIMHookTest {
assertThat(DKIMHook.SignatureRecordValidation.and(
DKIMHook.SignatureRecordValidation.ALLOW_ALL,
DKIMHook.SignatureRecordValidation.signatureRequired(true))
- .validate(MaybeSender.getMailSender("bob@localhost"), ImmutableList.of()))
+ .validate(MaybeSender.getMailSender("bob@localhost"), "messageId", ImmutableList.of()))
.satisfies(hookResult -> assertThat(hookResult.getResult()).isEqualTo(HookReturnCode.deny()))
.satisfies(hookResult -> assertThat(hookResult.getSmtpDescription()).isEqualTo("DKIM check failed. Expecting DKIM signatures. Got none."));
}
@@ -188,38 +188,38 @@ class DKIMHookTest {
void andShouldKeepOnlyTheFirstSMTPDescription() {
assertThat(DKIMHook.SignatureRecordValidation.and(
DKIMHook.SignatureRecordValidation.signatureRequired(true),
- DKIMHook.SignatureRecordValidation.expectedDToken("wrong.com"))
- .validate(MaybeSender.getMailSender("bob@localhost"), ImmutableList.of()))
+ new DKIMHook.DTokenSignatureRecordValidation("wrong.com"))
+ .validate(MaybeSender.getMailSender("bob@localhost"), "messageId", ImmutableList.of()))
.satisfies(hookResult -> assertThat(hookResult.getResult()).isEqualTo(HookReturnCode.deny()))
.satisfies(hookResult -> assertThat(hookResult.getSmtpDescription()).isEqualTo("DKIM check failed. Expecting DKIM signatures. Got none."));
}
@Test
void expectedDTokenShouldDenyWhenWrongDToken() {
- assertThat(DKIMHook.SignatureRecordValidation.expectedDToken("wrong.com")
- .validate(MaybeSender.getMailSender("bob@localhost"), ImmutableList.of(SIGNATURE_RECORD_1)))
+ assertThat(new DKIMHook.DTokenSignatureRecordValidation("wrong.com")
+ .validate(MaybeSender.getMailSender("bob@localhost"), "messageId", ImmutableList.of(SIGNATURE_RECORD_1)))
.satisfies(hookResult -> assertThat(hookResult.getResult()).isEqualTo(HookReturnCode.deny()))
- .satisfies(hookResult -> assertThat(hookResult.getSmtpDescription()).isEqualTo("DKIM check failed. Wrong d token. Expecting wrong.com"));
+ .satisfies(hookResult -> assertThat(hookResult.getSmtpDescription()).isEqualTo("DKIM check failed. Wrong d token. Expecting [wrong.com]. Got [linagora.com]"));
}
@Test
void expectedDTokenShouldDeclineWhenGoodDToken() {
- assertThat(DKIMHook.SignatureRecordValidation.expectedDToken("linagora.com")
- .validate(MaybeSender.getMailSender("bob@localhost"), ImmutableList.of(SIGNATURE_RECORD_1)))
+ assertThat(new DKIMHook.DTokenSignatureRecordValidation("linagora.com")
+ .validate(MaybeSender.getMailSender("bob@localhost"), "messageId", ImmutableList.of(SIGNATURE_RECORD_1)))
.satisfies(hookResult -> assertThat(hookResult.getResult()).isEqualTo(HookReturnCode.declined()));
}
@Test
void expectedDTokenShouldDeclineWhenGoodFirstDToken() {
- assertThat(DKIMHook.SignatureRecordValidation.expectedDToken("linagora.com")
- .validate(MaybeSender.getMailSender("bob@localhost"), ImmutableList.of(SIGNATURE_RECORD_1, SIGNATURE_RECORD_2)))
+ assertThat(new DKIMHook.DTokenSignatureRecordValidation("linagora.com")
+ .validate(MaybeSender.getMailSender("bob@localhost"), "messageId", ImmutableList.of(SIGNATURE_RECORD_1, SIGNATURE_RECORD_2)))
.satisfies(hookResult -> assertThat(hookResult.getResult()).isEqualTo(HookReturnCode.declined()));
}
@Test
void expectedDTokenShouldDeclineWhenGoodSecondDToken() {
- assertThat(DKIMHook.SignatureRecordValidation.expectedDToken("linagora.com")
- .validate(MaybeSender.getMailSender("bob@localhost"), ImmutableList.of(SIGNATURE_RECORD_2, SIGNATURE_RECORD_1)))
+ assertThat(new DKIMHook.DTokenSignatureRecordValidation("linagora.com")
+ .validate(MaybeSender.getMailSender("bob@localhost"), "messageId", ImmutableList.of(SIGNATURE_RECORD_2, SIGNATURE_RECORD_1)))
.satisfies(hookResult -> assertThat(hookResult.getResult()).isEqualTo(HookReturnCode.declined()));
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org