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 9a42832f4392d07cb54858f73b97f632c5664ff1 Author: Quan Tran <[email protected]> AuthorDate: Thu Nov 5 11:45:35 2026 +0700 [FIX] MacroExpand: validate each PTR name for %{p} %{p} looked up the addresses of the first PTR name of the client address only: when that name did not have the client address, %{p} was "unknown" although a later name was validated, and of several validated names it took the first, wherever it was. RFC 7208 7.3 finds the validated names as the ptr mechanism does (5.5): "If the <domain> is present in the list of validated domains, it SHOULD be used. Otherwise, if a subdomain of the <domain> is present, it SHOULD be used. Otherwise, any name from the list can be used." Validate the first names of the client address, up to the record limit of the DNS service (RFC 7208 4.6.4), once per check, and choose %{p} among the validated names for the current domain, which an include or a redirect changes: the domain, else the first validated subdomain of it, else the first validated name, else "unknown". A DNS error of an address lookup skips the name (RFC 7208 5.5); one of the PTR lookup still gives "unknown" (RFC 7208 7.3). --- .../org/apache/james/jspf/core/MacroExpand.java | 126 +++++++++++++++------ .../org/apache/james/jspf/core/SPFSession.java | 48 +++++++- .../org/apache/james/jspf/SPFExecutorBaseTest.java | 126 +++++++++++++++++++++ 3 files changed, 265 insertions(+), 35 deletions(-) diff --git a/resolver/src/main/java/org/apache/james/jspf/core/MacroExpand.java b/resolver/src/main/java/org/apache/james/jspf/core/MacroExpand.java index 5d1b064..ca829c6 100644 --- a/resolver/src/main/java/org/apache/james/jspf/core/MacroExpand.java +++ b/resolver/src/main/java/org/apache/james/jspf/core/MacroExpand.java @@ -29,7 +29,6 @@ package org.apache.james.jspf.core; import java.io.UnsupportedEncodingException; import java.net.URLEncoder; import java.util.ArrayList; -import java.util.Iterator; import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -80,6 +79,64 @@ public class MacroExpand { return value.isEmpty() ? 0 : Integer.parseInt(value); } + /** + * The PTR names of the client address left to validate, for %{p} + */ + private static final String ATTRIBUTE_MACRO_EXPAND_PTR_NAMES = "MacroExpand.ptrNames"; + + /** + * The PTR names of the client address validated so far, for %{p} + */ + private static final String ATTRIBUTE_MACRO_EXPAND_VALIDATED = "MacroExpand.validated"; + + /** + * @return the PTR names of the client address validated so far + */ + @SuppressWarnings("unchecked") + private static List<String> validatedNames(SPFSession session) { + return (List<String>) session.getAttribute(ATTRIBUTE_MACRO_EXPAND_VALIDATED); + } + + /** + * @return true when one of the addresses is the client address + * @throws PermErrorException when the client address does not parse + */ + private static boolean hasClientAddress(List<String> addresses, SPFSession session) throws PermErrorException { + String client = IPAddr.getAddress(session.getIpAddress()).toString(); + for (String address : addresses) { + try { + if (client.equals(IPAddr.getAddress(address).toString())) { + return true; + } + } catch (PermErrorException e) { + // no address: not the client one + } + } + return false; + } + + /** + * @return the lookup of the addresses of the next PTR name to validate, or null when none is left: the + * session then holds the validated names + */ + @SuppressWarnings("unchecked") + private static DNSLookupContinuation validateNextPtrName(SPFSession session) { + List<String> names = (List<String>) session.getAttribute(ATTRIBUTE_MACRO_EXPAND_PTR_NAMES); + int type = IPAddr.isIPV6(session.getIpAddress()) ? DNSRequest.AAAA : DNSRequest.A; + while (!names.isEmpty()) { + String name = names.remove(0); + try { + DNSRequest request = new DNSRequest(name, type); + session.setAttribute(ATTRIBUTE_MACRO_EXPAND_CHECKED_RECORD, name); + return new DNSLookupContinuation(request, new AResponseListener()); + } catch (NoneException e) { + // no domain name: not validated + } + } + session.setValidatedClientNames(validatedNames(session)); + return null; + } + /** * Construct MacroExpand * @@ -96,6 +153,10 @@ public class MacroExpand { } + /** + * The addresses of a PTR name of the client address: the name is validated when one is the client address + * (RFC 7208 5.5). A DNS error of the lookup skips the name. + */ private static final class AResponseListener implements SPFCheckerDNSResponseListener { @@ -105,37 +166,39 @@ public class MacroExpand { public DNSLookupContinuation onDNSResponse(DNSResponse response, SPFSession session) throws PermErrorException, NoneException, TempErrorException, NeutralException { - // just return the default "unknown" if we cannot find anything - // later - session.setClientDomain("unknown"); try { - List<String> records = response.getResponse(); - if (records != null && records.size() > 0) { - Iterator<String> i = records.iterator(); - while (i.hasNext()) { - String next = i.next(); - if (IPAddr.getAddress(session.getIpAddress()) - .toString().equals( - IPAddr.getAddress(next).toString())) { - session - .setClientDomain((String) session - .getAttribute(ATTRIBUTE_MACRO_EXPAND_CHECKED_RECORD)); - break; - } - } + List<String> addresses = response.getResponse(); + if (addresses != null && hasClientAddress(addresses, session)) { + validatedNames(session).add((String) session.getAttribute(ATTRIBUTE_MACRO_EXPAND_CHECKED_RECORD)); } } catch (TimeoutException e) { - // just return the default "unknown". + // RFC 7208 5.5: "that domain name is skipped and the search continues" } catch (PermErrorException e) { - // just return the default "unknown". + // a client address that does not parse: no validated name, "unknown" + session.setValidatedClientNames(new ArrayList<>()); + return null; } - return null; + return validateNextPtrName(session); } } + /** + * The PTR names of the client address: the first ones, up to the record limit of the DNS service (RFC 7208 + * 4.6.4), are validated in turn, once for the session, which chooses %{p} among the validated ones for each + * domain (RFC 7208 7.3). A DNS error of the lookup makes %{p} "unknown". + */ private static final class PTRResponseListener implements SPFCheckerDNSResponseListener { + private final int recordLimit; + + /** + * @param recordLimit the number of names validated at most, 0 for all of them + */ + private PTRResponseListener(int recordLimit) { + this.recordLimit = recordLimit; + } + /** * @see org.apache.james.jspf.core.SPFCheckerDNSResponseListener#onDNSResponse(org.apache.james.jspf.core.DNSResponse, org.apache.james.jspf.core.SPFSession) */ @@ -144,24 +207,19 @@ public class MacroExpand { NeutralException { try { - boolean ip6 = IPAddr.isIPV6(session.getIpAddress()); - List<String> records = response.getResponse(); - - if (records != null && records.size() > 0) { - String record = records.get(0); - session.setAttribute(ATTRIBUTE_MACRO_EXPAND_CHECKED_RECORD, - record); - - return new DNSLookupContinuation(new DNSRequest(record, - ip6 ? DNSRequest.AAAA : DNSRequest.A), - new AResponseListener()); + List<String> names = response.getResponse(); + if (names != null && names.size() > 0) { + int validated = recordLimit > 0 ? Math.min(recordLimit, names.size()) : names.size(); + session.setAttribute(ATTRIBUTE_MACRO_EXPAND_PTR_NAMES, new ArrayList<>(names.subList(0, validated))); + session.setAttribute(ATTRIBUTE_MACRO_EXPAND_VALIDATED, new ArrayList<String>()); + return validateNextPtrName(session); } } catch (TimeoutException e) { // just return the default "unknown". } - session.setClientDomain("unknown"); + session.setValidatedClientNames(new ArrayList<>()); return null; } @@ -176,7 +234,7 @@ public class MacroExpand { return new DNSLookupContinuation(new DNSRequest(IPAddr .getAddress(session.getIpAddress()).getReverseIP(), - DNSRequest.PTR), new PTRResponseListener()); + DNSRequest.PTR), new PTRResponseListener(dnsProbe != null ? dnsProbe.getRecordLimit() : 0)); } } return null; diff --git a/resolver/src/main/java/org/apache/james/jspf/core/SPFSession.java b/resolver/src/main/java/org/apache/james/jspf/core/SPFSession.java index d00b8ee..bca2cb4 100644 --- a/resolver/src/main/java/org/apache/james/jspf/core/SPFSession.java +++ b/resolver/src/main/java/org/apache/james/jspf/core/SPFSession.java @@ -21,9 +21,13 @@ package org.apache.james.jspf.core; import org.apache.james.jspf.core.exceptions.PermErrorException; +import org.xbill.DNS.Name; +import org.xbill.DNS.TextParseException; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Stack; import java.util.function.Predicate; @@ -53,6 +57,34 @@ public class SPFSession implements MacroData { return s.substring(start, end); } + /** + * @return the name as a domain name, whatever the case and a trailing dot, or null when it is none + */ + private static Name domainName(String name) { + try { + return name == null ? null : Name.fromString(name, Name.root); + } catch (TextParseException e) { + return null; + } + } + + /** + * @param validatedNames the validated names of the client address, in the order of its PTR records + * @param domain the current domain + * @return %{p}, of RFC 7208 7.3: the domain when it is validated, else the first validated subdomain of it, + * else the first validated name, else "unknown" + */ + private static String clientDomainOf(List<String> validatedNames, String domain) { + Name target = domainName(domain); + Optional<String> exact = validatedNames.stream() + .filter(name -> target != null && target.equals(domainName(name))) + .findFirst(); + Optional<String> subdomain = validatedNames.stream() + .filter(name -> target != null && domainName(name) != null && domainName(name).subdomain(target)) + .findFirst(); + return exact.orElse(subdomain.orElse(validatedNames.stream().findFirst().orElse("unknown"))); + } + private String ipAddress = ""; // also used for (i)<sending-host> private String mailFrom = ""; // (s)<responsible-sender> @@ -67,6 +99,8 @@ public class SPFSession implements MacroData { private String clientDomain = null; // (p) + private List<String> validatedClientNames = null; // the validated names of (p), once looked up + private String senderDomain = ""; // (o) private String readableIP = null; // (c) @@ -185,7 +219,19 @@ public class SPFSession implements MacroData { * @see org.apache.james.jspf.core.MacroData#getClientDomain() */ public String getClientDomain() { - return clientDomain; + if (clientDomain != null || validatedClientNames == null) { + return clientDomain; + } + // chosen for the current domain, which an include or a redirect changes + return clientDomainOf(validatedClientNames, getCurrentDomain()); + } + + /** + * Sets the validated names of the client address, %{p} then being chosen among them for the current domain + * @param validatedClientNames the validated names, in the order of the PTR records, empty for "unknown" + */ + void setValidatedClientNames(List<String> validatedClientNames) { + this.validatedClientNames = validatedClientNames; } /** 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 f678e92..3e07713 100644 --- a/resolver/src/test/java/org/apache/james/jspf/SPFExecutorBaseTest.java +++ b/resolver/src/test/java/org/apache/james/jspf/SPFExecutorBaseTest.java @@ -38,6 +38,7 @@ import java.net.InetAddress; import java.net.UnknownHostException; import java.nio.file.Paths; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.Locale; @@ -49,6 +50,7 @@ import java.util.stream.Collectors; import java.util.stream.IntStream; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; public abstract class SPFExecutorBaseTest { protected final String zonesDir = "src/test/resources/dnszones"; @@ -83,6 +85,24 @@ public abstract class SPFExecutorBaseTest { return fakeResolver; } + // hostname has "v=spf1 exists:%{p}._p.<hostname> -all", where only <expected>._p.<hostname> exists, and 1.2.3.4 the + // PTR names, of which the validated ones have the address 1.2.3.4 + private static FakeResolver pMacroOfTheClient(FakeResolver fakeResolver, String hostname, List<String> ptrNames, + List<String> validated, String expected) throws IOException { + fakeResolver.addRecord(new TXTRecord(Name.fromString(hostname + "."), DClass.IN, 30L, + "v=spf1 exists:%{p}._p." + hostname + " -all")); + for (String ptrName : ptrNames) { + fakeResolver.addRecord(new PTRRecord(Name.fromString("4.3.2.1.in-addr.arpa."), DClass.IN, 30L, + Name.fromString(ptrName + "."))); + } + for (String name : validated) { + fakeResolver.addRecord(new ARecord(Name.fromString(name + "."), DClass.IN, 30L, InetAddress.getByName("1.2.3.4"))); + } + fakeResolver.addRecord(new ARecord(Name.fromString(expected + "._p." + hostname + "."), DClass.IN, 30L, + InetAddress.getByName("127.0.0.2"))); + return fakeResolver; + } + // hostname has "v=spf1 ptr -all", and 1.2.3.4 the 11 PTR names name<i>.<hostname>, of which only name<validated> has // the address 1.2.3.4 private static FakeResolver elevenPtrNamesOfTheClient(String hostname, int validated) throws IOException { @@ -751,6 +771,112 @@ public abstract class SPFExecutorBaseTest { assertEquals(SPFErrorConstants.FAIL_CONV, result.getResult()); } + @Test(timeout = 10000) + public void pMacroShouldBeAValidatedNameAfterOneThatDoesNotValidate() throws IOException { + String hostname = "pmacro.spfexecutor.fake"; + FakeResolver fakeResolver = pMacroOfTheClient(new FakeResolver(), hostname, + Arrays.asList("bad." + hostname, "good." + hostname), Collections.singletonList("good." + hostname), + "good." + hostname); + + SPFResult result = createCustomSPF(new DNSServiceXBillImpl(fakeResolver)).checkSPF("1.2.3.4", "a_user@" + hostname, hostname); + + // RFC 7208 7.3 and 5.5: each PTR name is validated + assertEquals(SPFErrorConstants.PASS_CONV, result.getResult()); + } + + @Test(timeout = 10000) + public void pMacroShouldPreferAValidatedNameUnderTheDomain() throws IOException { + String hostname = "pmacrosub.spfexecutor.fake"; + FakeResolver fakeResolver = pMacroOfTheClient(new FakeResolver(), hostname, + Arrays.asList("mail.other.fake", "mail." + hostname), Arrays.asList("mail.other.fake", "mail." + hostname), + "mail." + hostname); + + SPFResult result = createCustomSPF(new DNSServiceXBillImpl(fakeResolver)).checkSPF("1.2.3.4", "a_user@" + hostname, hostname); + + // RFC 7208 7.3: "if a subdomain of the <domain> is present, it SHOULD be used" + assertEquals(SPFErrorConstants.PASS_CONV, result.getResult()); + } + + @Test(timeout = 10000) + public void pMacroShouldPreferTheDomainToASubdomain() throws IOException { + String hostname = "pmacrodomain.spfexecutor.fake"; + FakeResolver fakeResolver = pMacroOfTheClient(new FakeResolver(), hostname, + Arrays.asList("mail." + hostname, hostname), Arrays.asList("mail." + hostname, hostname), hostname); + + SPFResult result = createCustomSPF(new DNSServiceXBillImpl(fakeResolver)).checkSPF("1.2.3.4", "a_user@" + hostname, hostname); + + // RFC 7208 7.3: "If the <domain> is present in the list of validated domains, it SHOULD be used" + assertEquals(SPFErrorConstants.PASS_CONV, result.getResult()); + } + + @Test(timeout = 10000) + public void pMacroShouldSkipANameWhoseAddressLookupFails() throws IOException { + String hostname = "pmacroerror.spfexecutor.fake"; + // the names outside the zones of fakeResolver, as broken.servfail.fake, get a server failure + AnsweringResolver servfail = AnsweringResolver.withRcode(Rcode.SERVFAIL); + FakeResolver fakeResolver = pMacroOfTheClient(new FakeResolver(servfail), hostname, + Arrays.asList("broken.servfail.fake", "good." + hostname), Collections.singletonList("good." + hostname), + "good." + hostname); + + SPFResult result = createCustomSPF(new DNSServiceXBillImpl(fakeResolver)).checkSPF("1.2.3.4", "a_user@" + hostname, hostname); + + // RFC 7208 5.5: "that domain name is skipped and the search continues" + assertEquals(SPFErrorConstants.PASS_CONV, result.getResult()); + assertTrue(servfail.getQueries() + " queries", servfail.getQueries() > 0); + } + + @Test(timeout = 10000) + public void pMacroShouldValidateTheTenthName() throws IOException { + String hostname = "pmacrotenth.spfexecutor.fake"; + List<String> names = IntStream.rangeClosed(1, 11).mapToObj(i -> "name" + i + "." + hostname) + .collect(Collectors.toList()); + FakeResolver fakeResolver = pMacroOfTheClient(new FakeResolver(), hostname, names, + Collections.singletonList(names.get(9)), names.get(9)); + + SPFResult result = createCustomSPF(new DNSServiceXBillImpl(fakeResolver)).checkSPF("1.2.3.4", "a_user@" + hostname, hostname); + + // RFC 7208 4.6.4: the first 10 PTR names are validated + assertEquals(SPFErrorConstants.PASS_CONV, result.getResult()); + } + + @Test(timeout = 10000) + public void pMacroShouldBeChosenForTheDomainOfAnInclude() throws IOException { + String hostname = "pmacroinclude.spfexecutor.fake"; + String child = "child." + hostname; + FakeResolver fakeResolver = new FakeResolver(); + // the include chooses %{p} = child first, then hostname chooses its own validated name + fakeResolver.addRecord(new TXTRecord(Name.fromString(hostname + "."), DClass.IN, 30L, + "v=spf1 include:" + child + " exists:%{p}._allow." + hostname + " -all")); + fakeResolver.addRecord(new TXTRecord(Name.fromString(child + "."), DClass.IN, 30L, + "v=spf1 exists:%{p}._miss." + child + " -all")); + for (String name : Arrays.asList(hostname, child)) { + fakeResolver.addRecord(new PTRRecord(Name.fromString("4.3.2.1.in-addr.arpa."), DClass.IN, 30L, + Name.fromString(name + "."))); + fakeResolver.addRecord(new ARecord(Name.fromString(name + "."), DClass.IN, 30L, InetAddress.getByName("1.2.3.4"))); + } + fakeResolver.addRecord(new ARecord(Name.fromString(hostname + "._allow." + hostname + "."), DClass.IN, 30L, + InetAddress.getByName("127.0.0.2"))); + + SPFResult result = createCustomSPF(new DNSServiceXBillImpl(fakeResolver)).checkSPF("1.2.3.4", "a_user@" + hostname, hostname); + + // RFC 7208 7.3: %{p} prefers the <domain>, which the include changed and the end of the include restored + assertEquals(SPFErrorConstants.PASS_CONV, result.getResult()); + } + + @Test(timeout = 10000) + public void pMacroShouldIgnoreTheNamesAfterTheTenth() throws IOException { + String hostname = "pmacrolimit.spfexecutor.fake"; + List<String> names = IntStream.rangeClosed(1, 11).mapToObj(i -> "name" + i + "." + hostname) + .collect(Collectors.toList()); + FakeResolver fakeResolver = pMacroOfTheClient(new FakeResolver(), hostname, names, + Collections.singletonList(names.get(10)), names.get(10)); + + SPFResult result = createCustomSPF(new DNSServiceXBillImpl(fakeResolver)).checkSPF("1.2.3.4", "a_user@" + hostname, hostname); + + // RFC 7208 4.6.4: "all records other than the first 10 MUST be ignored", so %{p} is "unknown" + assertEquals(SPFErrorConstants.FAIL_CONV, result.getResult()); + } + @Test(timeout = 10000) public void ptrShouldNotMatchALabelHoldingADotAsASubdomain() throws IOException { String hostname = "ptrlabel.spfexecutor.fake"; --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
