This is an automated email from the ASF dual-hosted git repository. chibenwa pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-jspf.git
commit ea9b7dc2eb5d426a3e3884f6080f34c93e2d3af7 Author: Quan Tran <[email protected]> AuthorDate: Wed Nov 4 21:34:46 2026 +0700 [FIX] SPF executors: temperror for an unexpected error of a lookup The DNS service can fail a lookup with an unchecked exception: dnsjava throws ClassCastException on some malformed answers, and a DNSService of the library user may throw on a name or answer it cannot handle. The three executors passed that exception to the exception catchers, whose catch-all, in SPF, made the check neutral, as for an error of jSPF itself. A DNS error is a temperror (RFC 7208 4.4), as the time out of the same lookup, and as that catch-all already gives for a SocketException: "v=spf1 a:broken.example.com -all" was neutral where the server of broken.example.com caused the error. Read an unexpected exception of the lookup as a lookup error in SynchronousSPFExecutor, AsynchronousSPFExecutor and StagedMultipleSPFExecutor, and log it, also when the asynchronous lookup service throws it instead of failing the lookup; then the staged executor skips a later answer to that lookup, which no check waits for any more, instead of ending its thread. An Error, as a stack overflow, still goes to the exception catchers. Both executors now give temperror for an answer dnsjava cannot normalize (two DNAME records of a name): Lookup throws, LookupSession fails it as invalid zone data. --- .../jspf/executor/AsynchronousSPFExecutor.java | 25 +++++++- .../jspf/executor/StagedMultipleSPFExecutor.java | 17 +++++- .../jspf/executor/SynchronousSPFExecutor.java | 4 ++ .../org/apache/james/jspf/SPFExecutorBaseTest.java | 70 ++++++++++++++++++++-- .../StagedMultipleSPFExecutorIntegrationTest.java | 50 +++++++++++++--- 5 files changed, 149 insertions(+), 17 deletions(-) diff --git a/resolver/src/main/java/org/apache/james/jspf/executor/AsynchronousSPFExecutor.java b/resolver/src/main/java/org/apache/james/jspf/executor/AsynchronousSPFExecutor.java index 0413255..ea6fe91 100644 --- a/resolver/src/main/java/org/apache/james/jspf/executor/AsynchronousSPFExecutor.java +++ b/resolver/src/main/java/org/apache/james/jspf/executor/AsynchronousSPFExecutor.java @@ -21,9 +21,13 @@ package org.apache.james.jspf.executor; import java.io.IOException; import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; +import java.util.concurrent.CompletionStage; import org.apache.james.jspf.core.DNSLookupContinuation; +import org.apache.james.jspf.core.DNSRequest; import org.apache.james.jspf.core.DNSResponse; import org.apache.james.jspf.core.DNSService; import org.apache.james.jspf.core.SPFChecker; @@ -90,7 +94,7 @@ public class AsynchronousSPFExecutor implements SPFExecutor { private void handleCont(SPFSession session, FutureSPFResult result, DNSLookupContinuation cont, SPFChecker checker) { if (cont != null) { // if the checker returns a continuation we return it - dnsProbe.getRecordsAsync(cont.getRequest()) + lookUp(cont.getRequest()) .thenAccept(results -> onResponse(session, result, cont, new DNSResponse(results), checker)) .exceptionally(e -> { if (e instanceof CompletionException && e.getCause() != null) { @@ -106,6 +110,10 @@ public class AsynchronousSPFExecutor implements SPFExecutor { } if (e instanceof TimeoutException) { onResponse(session, result, cont, new DNSResponse((TimeoutException) e), checker); + } else if (e instanceof Exception) { + // a lookup error, as dnsjava's on an answer it cannot handle: temperror (RFC 7208 4.4) + LOGGER.warn("Unexpected error of the DNS lookup of {}", cont.getRequest().getHostname(), e); + onResponse(session, result, cont, new DNSResponse(new TimeoutException(e.toString())), checker); } else { handleErrorAndComplete(session, result, e); } @@ -116,6 +124,21 @@ public class AsynchronousSPFExecutor implements SPFExecutor { } } + /** + * @return the lookup of the DNS service, failed when the service throws instead of failing it + */ + private CompletionStage<List<String>> lookUp(DNSRequest request) { + try { + CompletionStage<List<String>> lookup = dnsProbe.getRecordsAsync(request); + if (lookup == null) { + return CompletableFuture.failedFuture(new IllegalStateException("No lookup of " + request.getHostname())); + } + return lookup; + } catch (RuntimeException e) { + return CompletableFuture.failedFuture(e); + } + } + private void onResponse(SPFSession session, FutureSPFResult result, DNSLookupContinuation cont, DNSResponse response, SPFChecker checker) { try { DNSLookupContinuation dnsLookupContinuation = cont.getListener().onDNSResponse(response, session); diff --git a/resolver/src/main/java/org/apache/james/jspf/executor/StagedMultipleSPFExecutor.java b/resolver/src/main/java/org/apache/james/jspf/executor/StagedMultipleSPFExecutor.java index 51b55b7..bcb2dc7 100644 --- a/resolver/src/main/java/org/apache/james/jspf/executor/StagedMultipleSPFExecutor.java +++ b/resolver/src/main/java/org/apache/james/jspf/executor/StagedMultipleSPFExecutor.java @@ -169,7 +169,12 @@ public class StagedMultipleSPFExecutor implements SPFExecutor, Runnable { sessions.put(new Integer(nextId), session); results.put(new Integer(nextId), result); session.setAttribute(ATTRIBUTE_STAGED_EXECUTOR_CONTINUATION, cont); - dnsProbe.getRecordsAsynch(cont.getRequest(), nextId, responseQueue); + try { + dnsProbe.getRecordsAsynch(cont.getRequest(), nextId, responseQueue); + } catch (RuntimeException e) { + // the lookup service failed instead of answering: the error is its answer + responseQueue.insertResponse(new IResponseImpl(new Integer(nextId), e)); + } } public void run() { @@ -181,6 +186,11 @@ public class StagedMultipleSPFExecutor implements SPFExecutor, Runnable { Integer respId = (Integer)resp.getId(); SPFSession session = sessions.remove(respId); FutureSPFResult result = results.remove(respId); + if (session == null) { + // a second answer to the same lookup, as from a lookup service that queued it, then threw + LOGGER.warn("No check waits for the DNS answer {}", respId); + continue; + } try { onResponse(session, result, resp); @@ -201,8 +211,9 @@ public class StagedMultipleSPFExecutor implements SPFExecutor, Runnable { if (resp.getException() instanceof TimeoutException) { response = new DNSResponse((TimeoutException) resp.getException()); } else if (resp.getException() != null) { - // any other lookup error goes to the exception catcher - throw resp.getException(); + // a lookup error, as dnsjava's on an answer it cannot handle: temperror (RFC 7208 4.4) + LOGGER.warn("Unexpected error of the DNS lookup of {}", cont.getRequest().getHostname(), resp.getException()); + response = new DNSResponse(new TimeoutException(resp.getException().toString())); } else { response = new DNSResponse(resp.getValue()); } diff --git a/resolver/src/main/java/org/apache/james/jspf/executor/SynchronousSPFExecutor.java b/resolver/src/main/java/org/apache/james/jspf/executor/SynchronousSPFExecutor.java index d4dce89..d1fd264 100644 --- a/resolver/src/main/java/org/apache/james/jspf/executor/SynchronousSPFExecutor.java +++ b/resolver/src/main/java/org/apache/james/jspf/executor/SynchronousSPFExecutor.java @@ -60,6 +60,10 @@ public class SynchronousSPFExecutor implements SPFExecutor { .getRequest())); } catch (TimeoutException e) { response = new DNSResponse(e); + } catch (RuntimeException e) { + // a lookup error, as dnsjava's on an answer it cannot handle: temperror (RFC 7208 4.4) + LOGGER.warn("Unexpected error of the DNS lookup of {}", cont.getRequest().getHostname(), e); + response = new DNSResponse(new TimeoutException(e.toString())); } cont = cont.getListener().onDNSResponse(response, session); } diff --git a/resolver/src/test/java/org/apache/james/jspf/SPFExecutorBaseTest.java b/resolver/src/test/java/org/apache/james/jspf/SPFExecutorBaseTest.java index bbdf7e7..7068ab4 100644 --- a/resolver/src/test/java/org/apache/james/jspf/SPFExecutorBaseTest.java +++ b/resolver/src/test/java/org/apache/james/jspf/SPFExecutorBaseTest.java @@ -15,6 +15,7 @@ import org.junit.Test; import org.xbill.DNS.ARecord; import org.xbill.DNS.CNAMERecord; import org.xbill.DNS.DClass; +import org.xbill.DNS.DNAMERecord; import org.xbill.DNS.Flags; import org.xbill.DNS.Lookup; import org.xbill.DNS.MXRecord; @@ -134,6 +135,31 @@ public abstract class SPFExecutorBaseTest { return createCustomSPF(dns).checkSPF("192.0.2.10", "a_user@" + hostname, hostname); } + // the lookups of broken.<hostname> throw, instead of failing, an unchecked exception + private SPFResult checkRecordWithThrowingHost(String hostname, String record) throws IOException { + FakeResolver fakeResolver = new FakeResolver(); + fakeResolver.addRecord(new TXTRecord(Name.fromString(hostname + "."), DClass.IN, 30L, record)); + String brokenHost = "broken." + hostname; + DNSService dns = new DNSServiceXBillImpl(fakeResolver) { + @Override + public List<String> getRecords(DNSRequest request) throws TimeoutException { + if (request.getHostname().equals(brokenHost)) { + throw new IllegalStateException("lookup service failure"); + } + return super.getRecords(request); + } + + @Override + public CompletionStage<List<String>> getRecordsAsync(DNSRequest request) { + if (request.getHostname().equals(brokenHost)) { + throw new IllegalStateException("lookup service failure"); + } + return super.getRecordsAsync(request); + } + }; + return createCustomSPF(dns).checkSPF("192.0.2.10", "a_user@" + hostname, hostname); + } + @Test public void test() { SPF spf = createSPF(); @@ -800,20 +826,54 @@ public abstract class SPFExecutorBaseTest { } @Test(timeout = 10000) - public void shouldEndAsNeutralOnAnUnexpectedLookupErrorInAnInclude() throws IOException { + public void shouldReturnTempErrorOnAnUnexpectedLookupError() throws IOException { + String hostname = "unexpectedError.spfexecutor.fake"; + SPFResult result = checkRecordWithBrokenHost(hostname, "v=spf1 a:broken." + hostname + " -all"); + + // RFC 7208 4.4: a DNS error "other than 0 or 3" ends the check as temperror + assertEquals(SPFErrorConstants.TEMP_ERROR_CONV, result.getResult()); + } + + @Test(timeout = 10000) + public void shouldReturnTempErrorWhenTheLookupServiceThrows() throws IOException { + String hostname = "throwingLookup.spfexecutor.fake"; + SPFResult result = checkRecordWithThrowingHost(hostname, "v=spf1 a:broken." + hostname + " -all"); + + assertEquals(SPFErrorConstants.TEMP_ERROR_CONV, result.getResult()); + } + + @Test(timeout = 10000) + public void shouldReturnTempErrorOnAnAnswerDnsjavaCannotNormalize() throws IOException { + String hostname = "x.dname.spfexecutor.fake"; + // two DNAME records of a parent of the name queried: invalid zone data (RFC 6672 2.4) + Message answer = new Message(); + answer.getHeader().setFlag(Flags.QR); + answer.addRecord(new DNAMERecord(Name.fromString("dname.spfexecutor.fake."), DClass.IN, 30L, + Name.fromString("a.example.")), Section.ANSWER); + answer.addRecord(new DNAMERecord(Name.fromString("dname.spfexecutor.fake."), DClass.IN, 30L, + Name.fromString("b.example.")), Section.ANSWER); + SPF spf = createCustomSPF(new DNSServiceXBillImpl(new AnsweringResolver(answer.toWire()))); + + SPFResult result = spf.checkSPF("192.0.2.10", "a_user@" + hostname, hostname); + + assertEquals(SPFErrorConstants.TEMP_ERROR_CONV, result.getResult()); + } + + @Test(timeout = 10000) + public void shouldReturnTempErrorOnAnUnexpectedLookupErrorInAnInclude() throws IOException { String hostname = "unexpectedErrorInInclude.spfexecutor.fake"; SPFResult result = checkRecordWithBrokenHost(hostname, "v=spf1 include:broken." + hostname + " -all"); - // as at the top level, the exception catcher of SPF makes the unexpected error neutral - assertEquals(SPFErrorConstants.NEUTRAL_CONV, result.getResult()); + // RFC 7208 5.2: a temperror of the included record is the result + assertEquals(SPFErrorConstants.TEMP_ERROR_CONV, result.getResult()); } @Test(timeout = 10000) - public void shouldEndAsNeutralOnAnUnexpectedLookupErrorInARedirect() throws IOException { + public void shouldReturnTempErrorOnAnUnexpectedLookupErrorInARedirect() throws IOException { String hostname = "unexpectedErrorInRedirect.spfexecutor.fake"; SPFResult result = checkRecordWithBrokenHost(hostname, "v=spf1 redirect=broken." + hostname); - assertEquals(SPFErrorConstants.NEUTRAL_CONV, result.getResult()); + assertEquals(SPFErrorConstants.TEMP_ERROR_CONV, result.getResult()); } @Test(timeout = 10000) diff --git a/resolver/src/test/java/org/apache/james/jspf/executor/StagedMultipleSPFExecutorIntegrationTest.java b/resolver/src/test/java/org/apache/james/jspf/executor/StagedMultipleSPFExecutorIntegrationTest.java index 8ae1999..c6f927f 100644 --- a/resolver/src/test/java/org/apache/james/jspf/executor/StagedMultipleSPFExecutorIntegrationTest.java +++ b/resolver/src/test/java/org/apache/james/jspf/executor/StagedMultipleSPFExecutorIntegrationTest.java @@ -72,40 +72,74 @@ public class StagedMultipleSPFExecutorIntegrationTest { }; @Test(timeout = 10000) - public void shouldEndAsNeutralOnAnUnexpectedLookupError() { + public void shouldReturnTempErrorOnAnUnexpectedLookupError() { checkUnexpectedLookupError(false, "broken.example.com"); } @Test(timeout = 10000) - public void shouldEndAsNeutralOnAnUnexpectedLookupErrorWhenMultiThreaded() { + public void shouldReturnTempErrorOnAnUnexpectedLookupErrorWhenMultiThreaded() { checkUnexpectedLookupError(true, "broken.example.com"); } @Test(timeout = 10000) - public void shouldEndAsNeutralOnAnUnexpectedLookupErrorInAnInclude() { + public void shouldReturnTempErrorOnAnUnexpectedLookupErrorInAnInclude() { checkUnexpectedLookupError(false, "include.example.com"); } @Test(timeout = 10000) - public void shouldEndAsNeutralOnAnUnexpectedLookupErrorInAnIncludeWhenMultiThreaded() { + public void shouldReturnTempErrorOnAnUnexpectedLookupErrorInAnIncludeWhenMultiThreaded() { checkUnexpectedLookupError(true, "include.example.com"); } @Test(timeout = 10000) - public void shouldEndAsNeutralOnAnUnexpectedLookupErrorInARedirect() { + public void shouldReturnTempErrorOnAnUnexpectedLookupErrorInARedirect() { checkUnexpectedLookupError(false, "redirect.example.com"); } @Test(timeout = 10000) - public void shouldEndAsNeutralOnAnUnexpectedLookupErrorInARedirectWhenMultiThreaded() { + public void shouldReturnTempErrorOnAnUnexpectedLookupErrorInARedirectWhenMultiThreaded() { checkUnexpectedLookupError(true, "redirect.example.com"); } + @Test(timeout = 10000) + public void shouldReturnTempErrorWhenTheLookupServiceThrows() { + DNSAsynchLookupService simulator = new DNSServiceAsynchSimulator(DNS, false); + // fails instead of answering an error + DNSAsynchLookupService throwing = (request, id, responseQueue) -> { + if (request.getHostname().equals("broken.example.com")) { + throw new IllegalStateException("lookup service failure"); + } + simulator.getRecordsAsynch(request, id, responseQueue); + }; + SPF spf = new SPF(DNS, new StagedMultipleSPFExecutor(throwing)); + + assertEquals(SPFErrorConstants.TEMP_ERROR_CONV, spf.checkSPF("1.2.3.4", "[email protected]", "broken.example.com").getResult()); + assertEquals(SPFErrorConstants.TEMP_ERROR_CONV, spf.checkSPF("1.2.3.4", "[email protected]", "include.example.com").getResult()); + assertEquals(SPFErrorConstants.PASS_CONV, spf.checkSPF("1.2.3.4", "[email protected]", "example.com").getResult()); + } + + @Test(timeout = 10000) + public void shouldGoOnWhenTheLookupServiceThrowsAfterQueuingTheLookup() { + DNSAsynchLookupService simulator = new DNSServiceAsynchSimulator(DNS, false); + // queues the lookup, so that it is answered, then fails + DNSAsynchLookupService throwing = (request, id, responseQueue) -> { + simulator.getRecordsAsynch(request, id, responseQueue); + if (request.getHostname().equals("broken.example.com")) { + throw new IllegalStateException("lookup service failure"); + } + }; + SPF spf = new SPF(DNS, new StagedMultipleSPFExecutor(throwing)); + + assertEquals(SPFErrorConstants.TEMP_ERROR_CONV, spf.checkSPF("1.2.3.4", "[email protected]", "broken.example.com").getResult()); + // the second answer of the failed lookup does not end the executor + assertEquals(SPFErrorConstants.PASS_CONV, spf.checkSPF("1.2.3.4", "[email protected]", "example.com").getResult()); + } + private void checkUnexpectedLookupError(boolean multiThread, String domain) { SPF spf = new SPF(DNS, new StagedMultipleSPFExecutor(new DNSServiceAsynchSimulator(DNS, multiThread))); - // the exception catcher of SPF makes an unexpected error neutral, as with AsynchronousSPFExecutor - assertEquals(SPFErrorConstants.NEUTRAL_CONV, spf.checkSPF("1.2.3.4", "foo@" + domain, domain).getResult()); + // RFC 7208 4.4: a DNS error ends the check as temperror, as with the two other executors + assertEquals(SPFErrorConstants.TEMP_ERROR_CONV, spf.checkSPF("1.2.3.4", "foo@" + domain, domain).getResult()); // the executor still checks assertEquals(SPFErrorConstants.PASS_CONV, spf.checkSPF("1.2.3.4", "[email protected]", "example.com").getResult()); } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
