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 703f4eaa6d473dca1bcb69005cc6dbb4ecb78ece Author: Quan Tran <[email protected]> AuthorDate: Thu Nov 5 10:16:18 2026 +0700 [FIX] DNSServiceXBillImpl: follow the first of several CNAMEs A name has one CNAME record at most (RFC 1034 3.6.2, RFC 2181 10.1). For an answer holding several, dnsjava 3.6 Message.normalize means to keep the first, but removes the record at the index the RRset has in the answer instead: the first of two, so both lookups followed the second CNAME, and one of three, so Lookup followed the second while LookupSession failed the answer as invalid zone data. A domain with three CNAMEs got the result of the second record with the synchronous executor and temperror with the asynchronous one. Keep only the first CNAME record of a name before dnsjava reads the answer, in getRecords and getRecordsAsync: both follow it, whatever the number of CNAMEs. The answer is rebuilt from its RRsets, so the CNAME kept has the lowest TTL of its RRset, as dnsjava caches it (RFC 2181 5.2), and a legacy SIG record covering the CNAME, which dnsjava puts in the RRset of the CNAME, is dropped rather than taken for it. --- .../james/jspf/impl/DNSServiceXBillImpl.java | 44 ++++++- .../apache/james/jspf/DNSServiceXBillImplTest.java | 131 +++++++++++++++++++++ .../org/apache/james/jspf/SPFExecutorBaseTest.java | 20 ++++ 3 files changed, 193 insertions(+), 2 deletions(-) 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 2090467..c82491d 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 @@ -32,6 +32,7 @@ import java.util.concurrent.CompletionException; import java.util.concurrent.CompletionStage; import java.util.concurrent.Executor; import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; import org.apache.james.jspf.core.DNSRequest; import org.apache.james.jspf.core.DNSService; @@ -47,10 +48,12 @@ import org.xbill.DNS.MXRecord; import org.xbill.DNS.Message; import org.xbill.DNS.Name; import org.xbill.DNS.PTRRecord; +import org.xbill.DNS.RRset; import org.xbill.DNS.Rcode; 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.TextParseException; @@ -82,6 +85,7 @@ public class DNSServiceXBillImpl implements DNSService { * extended bits, RFC 6891 6.1.3), or for another question: LookupSession would read an NXRRSET * answer as no record. Its {@link #send(Message)}, of the synchronous Lookup, is not bounded but * checks the answers the same way: Lookup reads only the 4 bits of the rcode in the header. + * Both keep the first of several CNAME records of a name, which Message.normalize means to do. */ private static final class BoundedResolver implements Resolver { /** @@ -108,6 +112,42 @@ public class DNSServiceXBillImpl implements DNSService { return null; } + /** + * @return the records kept of an RRset of the answer: of a CNAME RRset of several records, its first + * CNAME record only, without the other CNAME records nor a legacy SIG record covering them, + * which dnsjava puts in the RRset too, and Message.normalize would take for the CNAME + */ + private static List<Record> keptRecords(RRset rrset) { + if (rrset.getType() != Type.CNAME || rrset.size() < 2) { + return rrset.rrs(false); + } + List<Record> cname = rrset.rrs(false).stream() + .filter(record -> record.getType() == Type.CNAME) + .limit(1) + .collect(Collectors.toList()); + return cname.isEmpty() ? rrset.rrs(false) : cname; + } + + /** + * @return the answer with only the first of several CNAME records of a name (RFC 2181 10.1 allows + * one), as Message.normalize means to keep: dnsjava 3.6 removes another one, the first of + * two, and LookupSession fails an answer of three where Lookup follows one. Such an answer + * is rebuilt from its RRsets, whose records have the lowest TTL of their RRset (RFC 2181 5.2) + */ + private static Message withFirstCnames(Message answer) { + List<RRset> rrsets = answer.getSectionRRsets(Section.ANSWER); + if (rrsets.stream().noneMatch(rrset -> rrset.getType() == Type.CNAME && rrset.size() > 1)) { + return answer; + } + Message kept = answer.clone(); + kept.removeAllRecords(Section.ANSWER); + rrsets.forEach(rrset -> { + keptRecords(rrset).forEach(record -> kept.addRecord(record, Section.ANSWER)); + rrset.sigs().forEach(sig -> kept.addRecord(sig, Section.ANSWER)); + }); + return kept; + } + /** * @return the answer, or a failed lookup when it is a DNS error (RFC 7208 4.4) */ @@ -116,7 +156,7 @@ public class DNSServiceXBillImpl implements DNSService { if (error != null) { return CompletableFuture.failedFuture(new LookupFailedException(error)); } - return CompletableFuture.completedFuture(answer); + return CompletableFuture.completedFuture(withFirstCnames(answer)); } private final Resolver resolver; @@ -185,7 +225,7 @@ public class DNSServiceXBillImpl implements DNSService { if (error != null) { throw new IOException(error); } - return answer; + return withFirstCnames(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 0f0e1ab..8e6f934 100644 --- a/resolver/src/test/java/org/apache/james/jspf/DNSServiceXBillImplTest.java +++ b/resolver/src/test/java/org/apache/james/jspf/DNSServiceXBillImplTest.java @@ -38,17 +38,21 @@ import org.xbill.DNS.Message; import org.xbill.DNS.Name; import org.xbill.DNS.Rcode; import org.xbill.DNS.Record; +import org.xbill.DNS.SIGRecord; 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; import org.xbill.DNS.Type; import org.xbill.DNS.lookup.LookupFailedException; import org.xbill.DNS.lookup.LookupSession; import java.net.InetAddress; import java.net.UnknownHostException; +import java.time.Instant; import java.util.Base64; +import java.util.Collections; import java.util.List; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; @@ -68,6 +72,52 @@ 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"; + /** + * @return an answer holding two CNAME records of example.com, and the TXT record of each target + */ + private static Message twoCnamesOfTheName() throws TextParseException { + Message message = new Message(); + message.getHeader().setFlag(Flags.QR); + Name name = Name.fromString("example.com."); + message.addRecord(new CNAMERecord(name, DClass.IN, 3600L, Name.fromString("first.example.com.")), Section.ANSWER); + message.addRecord(new CNAMERecord(name, DClass.IN, 3600L, Name.fromString("second.example.com.")), Section.ANSWER); + message.addRecord(new TXTRecord(Name.fromString("first.example.com."), DClass.IN, 3600L, "first"), Section.ANSWER); + message.addRecord(new TXTRecord(Name.fromString("second.example.com."), DClass.IN, 3600L, "second"), Section.ANSWER); + return message; + } + + /** + * @return an answer holding a legacy SIG record (RFC 2535) of the CNAME of example.com before it, which + * dnsjava puts in the RRset of the CNAME, the CNAME, and the TXT record of its target + */ + private static Message sigBeforeTheCname() throws TextParseException { + Message message = new Message(); + message.getHeader().setFlag(Flags.QR); + Name name = Name.fromString("example.com."); + // algorithm 8, RSA/SHA-256: the signature is not checked + message.addRecord(new SIGRecord(name, DClass.IN, 3600L, Type.CNAME, 8, 3600L, Instant.ofEpochSecond(3600L), + Instant.ofEpochSecond(0L), 1, name, new byte[] {1}), Section.ANSWER); + message.addRecord(new CNAMERecord(name, DClass.IN, 3600L, Name.fromString("target.example.com.")), Section.ANSWER); + message.addRecord(new TXTRecord(Name.fromString("target.example.com."), DClass.IN, 3600L, "target"), Section.ANSWER); + return message; + } + + /** + * @return an answer holding the CNAME of example.com, a legacy SIG record of it after it (RFC 2535), and the + * TXT record of its target + */ + private static Message sigAfterTheCname() throws TextParseException { + Message message = new Message(); + message.getHeader().setFlag(Flags.QR); + Name name = Name.fromString("example.com."); + message.addRecord(new CNAMERecord(name, DClass.IN, 3600L, Name.fromString("target.example.com.")), Section.ANSWER); + // algorithm 8, RSA/SHA-256: the signature is not checked + message.addRecord(new SIGRecord(name, DClass.IN, 3600L, Type.CNAME, 8, 3600L, Instant.ofEpochSecond(3600L), + Instant.ofEpochSecond(0L), 1, name, new byte[] {1}), Section.ANSWER); + message.addRecord(new TXTRecord(Name.fromString("target.example.com."), DClass.IN, 3600L, "target"), Section.ANSWER); + return message; + } + @Test public void testGetLocalDomainNames() throws UnknownHostException { assertFalse(new DNSServiceXBillImpl().getLocalDomainNames().isEmpty()); @@ -188,6 +238,87 @@ public class DNSServiceXBillImplTest { } } + @Test(timeout = 10000) + public void getRecordsShouldFollowTheFirstOfSeveralCnames() throws Exception { + DNSServiceXBillImpl dns = new DNSServiceXBillImpl(new AnsweringResolver(twoCnamesOfTheName().toWire())); + Lookup.getDefaultCache(DClass.IN).clearCache(); + + List<String> records = dns.getRecords(new DNSRequest("example.com", DNSRequest.TXT)); + + // RFC 2181 10.1: a name has one CNAME at most; the first is followed, as Message.normalize means to + assertEquals(Collections.singletonList("first"), records); + } + + @Test(timeout = 10000) + public void getRecordsAsyncShouldFollowTheFirstOfSeveralCnames() throws Exception { + DNSServiceXBillImpl dns = new DNSServiceXBillImpl(new AnsweringResolver(twoCnamesOfTheName().toWire())); + + List<String> records = dns.getRecordsAsync(new DNSRequest("example.com", DNSRequest.TXT)) + .toCompletableFuture().get(5, TimeUnit.SECONDS); + + assertEquals(Collections.singletonList("first"), records); + } + + @Test(timeout = 10000) + public void getRecordsShouldNotCacheTheCnameFollowedPastTheLowestTtlOfItsRRset() throws Exception { + // the first CNAME for an hour, the other one for no time: the RRset for no time (RFC 2181 5.2) + Message message = new Message(); + message.getHeader().setFlag(Flags.QR); + Name name = Name.fromString("ttl.example.com."); + message.addRecord(new CNAMERecord(name, DClass.IN, 3600L, Name.fromString("first.example.com.")), Section.ANSWER); + message.addRecord(new CNAMERecord(name, DClass.IN, 0L, Name.fromString("second.example.com.")), Section.ANSWER); + message.addRecord(new TXTRecord(Name.fromString("first.example.com."), DClass.IN, 3600L, "first"), Section.ANSWER); + AnsweringResolver resolver = new AnsweringResolver(message.toWire()); + DNSServiceXBillImpl dns = new DNSServiceXBillImpl(resolver); + Lookup.getDefaultCache(DClass.IN).clearCache(); + + dns.getRecords(new DNSRequest("ttl.example.com", DNSRequest.TXT)); + dns.getRecords(new DNSRequest("ttl.example.com", DNSRequest.TXT)); + + // the second lookup asks again: the CNAME followed is not cached + assertEquals(2, resolver.getQueries()); + } + + @Test(timeout = 10000) + public void getRecordsShouldFollowACnameAfterASigRecordOfIt() throws Exception { + DNSServiceXBillImpl dns = new DNSServiceXBillImpl(new AnsweringResolver(sigBeforeTheCname().toWire())); + Lookup.getDefaultCache(DClass.IN).clearCache(); + + List<String> records = dns.getRecords(new DNSRequest("example.com", DNSRequest.TXT)); + + assertEquals(Collections.singletonList("target"), records); + } + + @Test(timeout = 10000) + public void getRecordsShouldFollowACnameBeforeASigRecordOfIt() throws Exception { + DNSServiceXBillImpl dns = new DNSServiceXBillImpl(new AnsweringResolver(sigAfterTheCname().toWire())); + Lookup.getDefaultCache(DClass.IN).clearCache(); + + List<String> records = dns.getRecords(new DNSRequest("example.com", DNSRequest.TXT)); + + assertEquals(Collections.singletonList("target"), records); + } + + @Test(timeout = 10000) + public void getRecordsAsyncShouldFollowACnameBeforeASigRecordOfIt() throws Exception { + DNSServiceXBillImpl dns = new DNSServiceXBillImpl(new AnsweringResolver(sigAfterTheCname().toWire())); + + List<String> records = dns.getRecordsAsync(new DNSRequest("example.com", DNSRequest.TXT)) + .toCompletableFuture().get(5, TimeUnit.SECONDS); + + assertEquals(Collections.singletonList("target"), records); + } + + @Test(timeout = 10000) + public void getRecordsAsyncShouldFollowACnameAfterASigRecordOfIt() throws Exception { + DNSServiceXBillImpl dns = new DNSServiceXBillImpl(new AnsweringResolver(sigBeforeTheCname().toWire())); + + List<String> records = dns.getRecordsAsync(new DNSRequest("example.com", DNSRequest.TXT)) + .toCompletableFuture().get(5, TimeUnit.SECONDS); + + assertEquals(Collections.singletonList("target"), records); + } + @Test(timeout = 10000) public void getRecordsAsyncShouldEndALookupWhoseAnswerHoldsACnameOfAnotherName() throws Exception { AnsweringResolver resolver = new AnsweringResolver(Base64.getDecoder().decode(CNAME_OF_THE_ROOT)); 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 0da1c37..31294d1 100644 --- a/resolver/src/test/java/org/apache/james/jspf/SPFExecutorBaseTest.java +++ b/resolver/src/test/java/org/apache/james/jspf/SPFExecutorBaseTest.java @@ -761,6 +761,26 @@ public abstract class SPFExecutorBaseTest { assertEquals(SPFErrorConstants.TEMP_ERROR_CONV, result.getResult()); } + @Test(timeout = 10000) + public void shouldFollowTheFirstOfSeveralCnamesOfTheDomain() throws IOException { + String hostname = "cnames.spfexecutor.fake"; + // three CNAME records of the domain, where RFC 2181 10.1 allows one, each to its own record + Message answer = new Message(); + answer.getHeader().setFlag(Flags.QR); + String[] records = {"v=spf1 -all", "v=spf1 +all", "v=spf1 ?all"}; + for (int i = 0; i < records.length; i++) { + Name target = Name.fromString("target" + i + "." + hostname + "."); + answer.addRecord(new CNAMERecord(Name.fromString(hostname + "."), DClass.IN, 30L, target), Section.ANSWER); + answer.addRecord(new TXTRecord(target, DClass.IN, 30L, records[i]), Section.ANSWER); + } + SPF spf = createCustomSPF(new DNSServiceXBillImpl(new AnsweringResolver(answer.toWire()))); + + SPFResult result = spf.checkSPF("192.0.2.10", "a_user@" + hostname, hostname); + + // the record of the first CNAME, whatever the executor + assertEquals(SPFErrorConstants.FAIL_CONV, result.getResult()); + } + @Test(timeout = 10000) public void shouldReturnNoneWhenTheLookupGetsACnameOfAnotherClass() throws IOException { String hostname = "cnameOfAnotherClass.spfexecutor.fake"; --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
