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 6e0c46fc9ca1246345f010fdfc280122e896c41e Author: Quan Tran <[email protected]> AuthorDate: Wed Nov 4 21:32:26 2026 +0700 [FIX] AsynchronousSPFExecutor: temperror for a DNS error With AsynchronousSPFExecutor, a lookup answered with an error rcode ended as no record: dnsjava's LookupSession fails it with a LookupFailedException (ServerFailedException for SERVFAIL, NoSuchRRSetException for NXRRSET), and the executor read every LookupFailedException as an empty answer. A network error other than a time out went to the catch-all, and gave neutral. The synchronous executor, with dnsjava's Lookup, gives temperror for both, as RFC 7208 4.4 asks for "a server failure (RCODE 2) or some other error (RCODE other than 0 or 3)": a sender domain whose server answers SERVFAIL got none instead of temperror. Check the rcode (with its extended bits, RFC 6891 6.1.3) and the question of each answer in the resolver of the lookup, as Lookup does, and read a LookupFailedException as no record only when the domain or the record does not exist (NXDOMAIN, no data, a CNAME loop), where Lookup ends with HOST_NOT_FOUND, TYPE_NOT_FOUND or UNRECOVERABLE. Any other lookup failure, invalid zone data included, and any I/O error, is a temperror. A lookup ended by the bound on queries found no record, as Lookup gives TYPE_NOT_FOUND for a CNAME of another class. The answer of the test of the bound on queries now fails at the first query, so a test with a CNAME of another class, which LookupSession does not follow either, keeps the bound tested. --- .../jspf/executor/AsynchronousSPFExecutor.java | 26 +++- .../james/jspf/impl/DNSServiceXBillImpl.java | 33 ++++- .../apache/james/jspf/DNSServiceXBillImplTest.java | 99 ++++--------- .../org/apache/james/jspf/SPFExecutorBaseTest.java | 50 +++++++ .../james/jspf/helpers/AnsweringResolver.java | 162 +++++++++++++++++++++ 5 files changed, 289 insertions(+), 81 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 125785d..0413255 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 @@ -35,6 +35,9 @@ import org.apache.james.jspf.core.exceptions.TimeoutException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.xbill.DNS.lookup.LookupFailedException; +import org.xbill.DNS.lookup.NoSuchDomainException; +import org.xbill.DNS.lookup.NoSuchRRSetException; +import org.xbill.DNS.lookup.RedirectOverflowException; /** * Asynchronous implementation of SPFExecutor. All queries will get executed asynchronously @@ -42,6 +45,18 @@ import org.xbill.DNS.lookup.LookupFailedException; public class AsynchronousSPFExecutor implements SPFExecutor { private static final Logger LOGGER = LoggerFactory.getLogger(AsynchronousSPFExecutor.class); + /** + * @return true if the lookup failed for want of a record, as when the synchronous Lookup of + * dnsjava ends with HOST_NOT_FOUND, TYPE_NOT_FOUND or UNRECOVERABLE (a CNAME loop): the + * domain or the record does not exist. Invalid zone data, which Lookup cannot normalize + * either, is a DNS error. + */ + private static boolean isNoData(Throwable e) { + return e instanceof NoSuchDomainException + || e instanceof NoSuchRRSetException + || e instanceof RedirectOverflowException; + } + private DNSService dnsProbe; public AsynchronousSPFExecutor(DNSService service) { @@ -81,12 +96,15 @@ public class AsynchronousSPFExecutor implements SPFExecutor { if (e instanceof CompletionException && e.getCause() != null) { e = e.getCause(); } - if (e instanceof IOException && e.getMessage() != null && e.getMessage().startsWith("Timed out ")) { + if (isNoData(e)) { + onResponse(session, result, cont, new DNSResponse(new ArrayList<>()), checker); + return null; + } + if (e instanceof IOException || e instanceof LookupFailedException) { + // a DNS error, as a time out: temperror, as for the synchronous lookup (RFC 7208 4.4) e = new TimeoutException(e.getMessage()); } - if (e instanceof LookupFailedException) { - onResponse(session, result, cont, new DNSResponse(new ArrayList<>()), checker); - } else if (e instanceof TimeoutException) { + if (e instanceof TimeoutException) { onResponse(session, result, cont, new DNSResponse((TimeoutException) e), checker); } else { handleErrorAndComplete(session, result, e); diff --git a/resolver/src/main/java/org/apache/james/jspf/impl/DNSServiceXBillImpl.java b/resolver/src/main/java/org/apache/james/jspf/impl/DNSServiceXBillImpl.java index b6df834..da95454 100644 --- a/resolver/src/main/java/org/apache/james/jspf/impl/DNSServiceXBillImpl.java +++ b/resolver/src/main/java/org/apache/james/jspf/impl/DNSServiceXBillImpl.java @@ -46,6 +46,7 @@ import org.xbill.DNS.MXRecord; import org.xbill.DNS.Message; import org.xbill.DNS.Name; import org.xbill.DNS.PTRRecord; +import org.xbill.DNS.Rcode; import org.xbill.DNS.Record; import org.xbill.DNS.Resolver; import org.xbill.DNS.SPFRecord; @@ -55,6 +56,7 @@ import org.xbill.DNS.TextParseException; import org.xbill.DNS.Type; import org.xbill.DNS.lookup.LookupFailedException; import org.xbill.DNS.lookup.LookupSession; +import org.xbill.DNS.lookup.NoSuchRRSetException; /** * This class contains helper to get all neccassary DNS infos that are needed @@ -74,14 +76,35 @@ public class DNSServiceXBillImpl implements DNSService { * 3.6 LookupSession asks the same question again, without end and without counting a redirect, * for an answer holding a CNAME it does not follow: of another name than the one queried (with * an error rcode, as YXRRSET), or of another class. + * It also fails, as Lookup does, an answer with an rcode other than NOERROR and NXDOMAIN (with its + * extended bits, RFC 6891 6.1.3), or for another question: LookupSession would read an NXRRSET + * answer as no record. */ private static final class BoundedResolver implements Resolver { /** - * @return a failed lookup, as the other lookup errors of LookupSession + * @return a lookup that found no record, as Lookup ends (TYPE_NOT_FOUND) on the answers that make + * LookupSession ask again without end */ private static CompletionStage<Message> tooManyQueries(Message query) { - return CompletableFuture.failedFuture(new LookupFailedException( - "More than " + MAX_QUERIES_PER_LOOKUP + " queries for " + query.getQuestion())); + LOGGER.debug("More than {} queries for {}", MAX_QUERIES_PER_LOOKUP, query.getQuestion()); + return CompletableFuture.failedFuture(new NoSuchRRSetException(query.getQuestion().getName(), + query.getQuestion().getType())); + } + + /** + * @return the answer, or a failed lookup when it is a DNS error (RFC 7208 4.4) + */ + private static CompletionStage<Message> checked(Message query, Message answer) { + int rcode = answer.getRcode(); + if (rcode != Rcode.NOERROR && rcode != Rcode.NXDOMAIN) { + return CompletableFuture.failedFuture(new LookupFailedException( + "Answer " + Rcode.string(rcode) + " for " + query.getQuestion())); + } + if (!query.getQuestion().equals(answer.getQuestion())) { + return CompletableFuture.failedFuture(new LookupFailedException( + "Answer for another question than " + query.getQuestion())); + } + return CompletableFuture.completedFuture(answer); } private final Resolver resolver; @@ -154,7 +177,7 @@ public class DNSServiceXBillImpl implements DNSService { if (queries.incrementAndGet() > MAX_QUERIES_PER_LOOKUP) { return tooManyQueries(query); } - return resolver.sendAsync(query); + return resolver.sendAsync(query).thenCompose(answer -> checked(query, answer)); } /** @@ -164,7 +187,7 @@ public class DNSServiceXBillImpl implements DNSService { if (queries.incrementAndGet() > MAX_QUERIES_PER_LOOKUP) { return tooManyQueries(query); } - return resolver.sendAsync(query, executor); + return resolver.sendAsync(query, executor).thenCompose(answer -> checked(query, answer)); } /** diff --git a/resolver/src/test/java/org/apache/james/jspf/DNSServiceXBillImplTest.java b/resolver/src/test/java/org/apache/james/jspf/DNSServiceXBillImplTest.java index fe798d5..66abff9 100644 --- a/resolver/src/test/java/org/apache/james/jspf/DNSServiceXBillImplTest.java +++ b/resolver/src/test/java/org/apache/james/jspf/DNSServiceXBillImplTest.java @@ -25,34 +25,29 @@ import ch.qos.logback.classic.Logger; import ch.qos.logback.classic.spi.ILoggingEvent; import ch.qos.logback.core.read.ListAppender; import org.apache.james.jspf.core.DNSRequest; +import org.apache.james.jspf.helpers.AnsweringResolver; import org.apache.james.jspf.impl.DNSServiceXBillImpl; import org.junit.Test; import org.slf4j.LoggerFactory; +import org.xbill.DNS.CNAMERecord; import org.xbill.DNS.DClass; -import org.xbill.DNS.EDNSOption; +import org.xbill.DNS.Flags; import org.xbill.DNS.Message; import org.xbill.DNS.Name; import org.xbill.DNS.Record; -import org.xbill.DNS.Resolver; import org.xbill.DNS.SPFRecord; import org.xbill.DNS.Section; -import org.xbill.DNS.TSIG; import org.xbill.DNS.TXTRecord; import org.xbill.DNS.Type; import org.xbill.DNS.lookup.LookupFailedException; import org.xbill.DNS.lookup.LookupSession; -import java.io.IOException; import java.net.UnknownHostException; -import java.time.Duration; import java.util.Base64; import java.util.List; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.CompletionStage; import java.util.concurrent.ExecutionException; -import java.util.concurrent.Executor; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -65,69 +60,6 @@ public class DNSServiceXBillImplTest { // rcode YXRRSET, and the answer ". CNAME .": a CNAME of the root, not of the name queried private static final String CNAME_OF_THE_ROOT = "AAGBJwABAAEAAAAAAAABAAHADAAFAAEAAA4QAAEA"; - // answers every query, on the calling thread, with the given message, its ID and question set to the query's - private static final class AnsweringResolver implements Resolver { - private final byte[] answer; - private final AtomicInteger queries = new AtomicInteger(); - - private AnsweringResolver(byte[] answer) { - this.answer = answer; - } - - @Override - public void setPort(int port) { - } - - @Override - public void setTCP(boolean flag) { - } - - @Override - public void setIgnoreTruncation(boolean flag) { - } - - @Override - public void setEDNS(int version, int payloadSize, int flags, List<EDNSOption> options) { - } - - @Override - public void setTSIGKey(TSIG key) { - } - - @Override - public void setTimeout(Duration timeout) { - } - - @Override - public Message send(Message query) throws IOException { - queries.incrementAndGet(); - Message response = new Message(answer); - response.getHeader().setID(query.getHeader().getID()); - response.removeAllRecords(Section.QUESTION); - response.addRecord(query.getQuestion(), Section.QUESTION); - return response; - } - - @Override - public CompletionStage<Message> sendAsync(Message query) { - try { - return CompletableFuture.completedFuture(send(query)); - } catch (IOException e) { - return CompletableFuture.failedFuture(e); - } - } - - @Override - public CompletionStage<Message> sendAsync(Message query, Executor executor) { - return sendAsync(query); - } - - @Override - public String toString() { - return "AnsweringResolver"; - } - } - @Test public void testGetLocalDomainNames() throws UnknownHostException { assertFalse(new DNSServiceXBillImpl().getLocalDomainNames().isEmpty()); @@ -190,7 +122,30 @@ public class DNSServiceXBillImplTest { assertTrue(e.getCause().toString(), e.getCause() instanceof LookupFailedException); } // the first query, then at most one per redirect LookupSession follows - assertTrue(resolver.queries.get() + " queries", resolver.queries.get() <= LookupSession.DEFAULT_MAX_ITERATIONS + 1); + assertTrue(resolver.getQueries() + " queries", resolver.getQueries() <= LookupSession.DEFAULT_MAX_ITERATIONS + 1); + } + + @Test(timeout = 10000) + public void getRecordsAsyncShouldEndALookupWhoseAnswerHoldsACnameOfAnotherClass() throws Exception { + // NOERROR, and a CNAME of the name queried in class CH, which LookupSession neither follows nor counts + Message message = new Message(); + message.getHeader().setFlag(Flags.QR); + message.addRecord(new CNAMERecord(Name.fromString("example.com."), DClass.CH, 3600L, + Name.fromString("other.example.com.")), Section.ANSWER); + AnsweringResolver resolver = new AnsweringResolver(message.toWire()); + DNSServiceXBillImpl dns = new DNSServiceXBillImpl(resolver); + + CompletableFuture<List<String>> records = dns.getRecordsAsync(new DNSRequest("example.com", DNSRequest.AAAA)) + .toCompletableFuture(); + + try { + records.get(5, TimeUnit.SECONDS); + fail("the lookup should fail"); + } catch (ExecutionException e) { + assertTrue(e.getCause().toString(), e.getCause() instanceof LookupFailedException); + } + // the first query, then at most one per redirect LookupSession follows + assertTrue(resolver.getQueries() + " queries", resolver.getQueries() <= LookupSession.DEFAULT_MAX_ITERATIONS + 1); } @Test(timeout = 10000) 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 613a99e..bbdf7e7 100644 --- a/resolver/src/test/java/org/apache/james/jspf/SPFExecutorBaseTest.java +++ b/resolver/src/test/java/org/apache/james/jspf/SPFExecutorBaseTest.java @@ -5,6 +5,7 @@ import org.apache.james.jspf.core.DNSService; import org.apache.james.jspf.core.exceptions.SPFErrorConstants; import org.apache.james.jspf.core.exceptions.TimeoutException; import org.apache.james.jspf.executor.SPFResult; +import org.apache.james.jspf.helpers.AnsweringResolver; import org.apache.james.jspf.helpers.FakeResolver; import org.apache.james.jspf.impl.DNSServiceXBillImpl; import org.apache.james.jspf.impl.SPF; @@ -12,13 +13,18 @@ import org.apache.james.jspf.policies.local.TrustedForwarderPolicy; import org.junit.Before; import org.junit.Test; import org.xbill.DNS.ARecord; +import org.xbill.DNS.CNAMERecord; import org.xbill.DNS.DClass; +import org.xbill.DNS.Flags; import org.xbill.DNS.Lookup; import org.xbill.DNS.MXRecord; +import org.xbill.DNS.Message; import org.xbill.DNS.Name; import org.xbill.DNS.PTRRecord; +import org.xbill.DNS.Rcode; import org.xbill.DNS.Resolver; import org.xbill.DNS.SPFRecord; +import org.xbill.DNS.Section; import org.xbill.DNS.SimpleResolver; import org.xbill.DNS.TXTRecord; import org.xbill.DNS.TextParseException; @@ -666,6 +672,50 @@ public abstract class SPFExecutorBaseTest { assertEquals(SPFErrorConstants.FAIL_CONV, result.getResult()); } + @Test(timeout = 10000) + public void shouldReturnTempErrorWhenTheLookupGetsAServerFailure() { + SPF spf = createCustomSPF(new DNSServiceXBillImpl(AnsweringResolver.withRcode(Rcode.SERVFAIL))); + + SPFResult result = spf.checkSPF("192.0.2.10", "[email protected]", "servfail.spfexecutor.fake"); + + // RFC 7208 4.4: "a server failure (RCODE 2) or some other error (RCODE other than 0 or 3)" + assertEquals(SPFErrorConstants.TEMP_ERROR_CONV, result.getResult()); + } + + @Test(timeout = 10000) + public void shouldReturnTempErrorWhenTheLookupGetsAnotherErrorRcode() { + SPF spf = createCustomSPF(new DNSServiceXBillImpl(AnsweringResolver.withRcode(Rcode.NXRRSET))); + + SPFResult result = spf.checkSPF("192.0.2.10", "[email protected]", "nxrrset.spfexecutor.fake"); + + assertEquals(SPFErrorConstants.TEMP_ERROR_CONV, result.getResult()); + } + + @Test(timeout = 10000) + public void shouldReturnTempErrorWhenTheLookupGetsANetworkError() { + SPF spf = createCustomSPF(new DNSServiceXBillImpl(AnsweringResolver.unreachable())); + + SPFResult result = spf.checkSPF("192.0.2.10", "[email protected]", "unreachable.spfexecutor.fake"); + + assertEquals(SPFErrorConstants.TEMP_ERROR_CONV, result.getResult()); + } + + @Test(timeout = 10000) + public void shouldReturnNoneWhenTheLookupGetsACnameOfAnotherClass() throws IOException { + String hostname = "cnameOfAnotherClass.spfexecutor.fake"; + // NOERROR, and a CNAME of the name queried in class CH: no record of class IN + Message answer = new Message(); + answer.getHeader().setFlag(Flags.QR); + answer.addRecord(new CNAMERecord(Name.fromString(hostname + "."), DClass.CH, 30L, + Name.fromString("other." + hostname + ".")), Section.ANSWER); + SPF spf = createCustomSPF(new DNSServiceXBillImpl(new AnsweringResolver(answer.toWire()))); + + SPFResult result = spf.checkSPF("192.0.2.10", "a_user@" + hostname, hostname); + + // as for a domain without SPF record (RFC 7208 4.5) + assertEquals(SPFErrorConstants.NONE_CONV, result.getResult()); + } + @Test(timeout = 10000) public void shouldReturnPermErrorOnAIp4CidrOverflowingAnInt() throws IOException { SPFResult result = checkRecord("aIp4CidrOverflow.spfexecutor.fake", "v=spf1 a/4294967296 -all", "1.2.3.4"); diff --git a/resolver/src/test/java/org/apache/james/jspf/helpers/AnsweringResolver.java b/resolver/src/test/java/org/apache/james/jspf/helpers/AnsweringResolver.java new file mode 100644 index 0000000..7d05fca --- /dev/null +++ b/resolver/src/test/java/org/apache/james/jspf/helpers/AnsweringResolver.java @@ -0,0 +1,162 @@ +/**************************************************************** + * 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.james.jspf.helpers; + +import java.io.IOException; +import java.time.Duration; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionStage; +import java.util.concurrent.Executor; +import java.util.concurrent.atomic.AtomicInteger; + +import org.xbill.DNS.EDNSOption; +import org.xbill.DNS.Flags; +import org.xbill.DNS.Message; +import org.xbill.DNS.Resolver; +import org.xbill.DNS.Section; +import org.xbill.DNS.TSIG; + +/** + * Answers every query, on the calling thread, with the same message, its ID and question set to + * the query's: a DNS server giving an unusual answer. + */ +public final class AnsweringResolver implements Resolver { + /** + * @param rcode an rcode + * @return a resolver answering every query with that rcode and no record + */ + public static AnsweringResolver withRcode(int rcode) { + Message answer = new Message(); + answer.getHeader().setFlag(Flags.QR); + answer.getHeader().setRcode(rcode); + return new AnsweringResolver(answer.toWire()); + } + + /** + * @return a resolver failing every query with an I/O error other than a time out + */ + public static AnsweringResolver unreachable() { + return new AnsweringResolver(null); + } + + private final byte[] answer; + private final AtomicInteger queries = new AtomicInteger(); + + /** + * @param answer the message, in wire format + */ + public AnsweringResolver(byte[] answer) { + this.answer = answer; + } + + /** + * @return the number of queries received + */ + public int getQueries() { + return queries.get(); + } + + /** + * @see org.xbill.DNS.Resolver#setPort(int) + */ + @Override + public void setPort(int port) { + } + + /** + * @see org.xbill.DNS.Resolver#setTCP(boolean) + */ + @Override + public void setTCP(boolean flag) { + } + + /** + * @see org.xbill.DNS.Resolver#setIgnoreTruncation(boolean) + */ + @Override + public void setIgnoreTruncation(boolean flag) { + } + + /** + * @see org.xbill.DNS.Resolver#setEDNS(int, int, int, List) + */ + @Override + public void setEDNS(int version, int payloadSize, int flags, List<EDNSOption> options) { + } + + /** + * @see org.xbill.DNS.Resolver#setTSIGKey(TSIG) + */ + @Override + public void setTSIGKey(TSIG key) { + } + + /** + * @see org.xbill.DNS.Resolver#setTimeout(Duration) + */ + @Override + public void setTimeout(Duration timeout) { + } + + /** + * @see org.xbill.DNS.Resolver#send(Message) + */ + @Override + public Message send(Message query) throws IOException { + queries.incrementAndGet(); + if (answer == null) { + throw new IOException("Network is unreachable"); + } + Message response = new Message(answer); + response.getHeader().setID(query.getHeader().getID()); + response.removeAllRecords(Section.QUESTION); + response.addRecord(query.getQuestion(), Section.QUESTION); + return response; + } + + /** + * @see org.xbill.DNS.Resolver#sendAsync(Message) + */ + @Override + public CompletionStage<Message> sendAsync(Message query) { + try { + return CompletableFuture.completedFuture(send(query)); + } catch (IOException e) { + return CompletableFuture.failedFuture(e); + } + } + + /** + * @see org.xbill.DNS.Resolver#sendAsync(Message, Executor) + */ + @Override + public CompletionStage<Message> sendAsync(Message query, Executor executor) { + return sendAsync(query); + } + + /** + * @see java.lang.Object#toString() + */ + @Override + public String toString() { + return "AnsweringResolver"; + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
