Add tests based on data from an actual transfer

dnsjava · ibauersachs · Nov 4, 2023 · Oct 29, 2023 · Oct 30, 2023 · Nov 2, 2023
commit 14f083d58c309f2f4c122d53875425e7b729d2c7
diff --git a/pom.xml b/pom.xml
@@ -433,6 +433,12 @@
             <version>${vertx.version}</version>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>commons-io</groupId>
+            <artifactId>commons-io</artifactId>
+            <version>2.15.0</version>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
     <profiles>

diff --git a/src/main/java/org/xbill/DNS/TCPClient.java b/src/main/java/org/xbill/DNS/TCPClient.java
@@ -14,7 +14,7 @@
 import java.time.Duration;
 import java.time.temporal.ChronoUnit;
 
-final class TCPClient {
+class TCPClient implements AutoCloseable {
   private final long startTime;
   private final Duration timeout;
   private final SelectionKey key;
@@ -144,7 +144,7 @@ private void blockUntil(SelectionKey key) throws IOException {
     }
   }
 
-  void cleanup() throws IOException {
+  public void close() throws IOException {
     key.selector().close();
     key.channel().close();
   }

diff --git a/src/main/java/org/xbill/DNS/TSIG.java b/src/main/java/org/xbill/DNS/TSIG.java
@@ -15,6 +15,7 @@
 import javax.crypto.Mac;
 import javax.crypto.SecretKey;
 import javax.crypto.spec.SecretKeySpec;
+import lombok.Getter;
 import lombok.extern.slf4j.Slf4j;
 import org.xbill.DNS.utils.base64;
 import org.xbill.DNS.utils.hexdump;
@@ -825,6 +826,7 @@ public static class StreamVerifier {
 
     private int nresponses;
     private int lastsigned;
+    @Getter private String errorMessage;
 
     /** Creates an object to verify a multiple message response */
     public StreamVerifier(TSIG tsig, TSIGRecord queryTsig) {
@@ -841,7 +843,8 @@ public StreamVerifier(TSIG tsig, TSIGRecord queryTsig) {
      *
      * <p>This overload assumes that the verified message is not the last one, which is required to
      * have a {@link TSIGRecord}. Use {@link #verify(Message, byte[], boolean)} to explicitly
-     * specify the last message.
+     * specify the last message or check that the message is verified with {@link
+     * Message#isVerified()}.
      *
      * @param message The message
      * @param messageBytes The message in unparsed form
@@ -866,12 +869,23 @@ public int verify(Message message, byte[] messageBytes) {
     public int verify(Message message, byte[] messageBytes, boolean isLastMessage) {
       TSIGRecord tsig = message.getTSIG();
 
+      // https://datatracker.ietf.org/doc/html/rfc8945#section-5.3.1
+      // [...] a client that receives DNS messages and verifies TSIG MUST accept up to 99
+      // intermediary messages without a TSIG and MUST verify that both the first and last message
+      // contain a TSIG.
       nresponses++;
       if (nresponses == 1) {
-        int result = key.verify(message, messageBytes, queryTsig, true, sharedHmac);
-        hmacAddSignature(sharedHmac, tsig);
-        lastsigned = nresponses;
-        return result;
+        if (tsig != null) {
+          int result = key.verify(message, messageBytes, queryTsig, true, sharedHmac);
+          hmacAddSignature(sharedHmac, tsig);
+          lastsigned = nresponses;
+          return result;
+        } else {
+          errorMessage = "missing required signature on first message";
+          log.debug("{}: {}", Rcode.TSIGstring(Rcode.FORMERR), errorMessage);
+          message.tsigState = Message.TSIG_FAILED;
+          return Rcode.FORMERR;
+        }
       }
 
       if (tsig != null) {
@@ -882,15 +896,18 @@ public int verify(Message message, byte[] messageBytes, boolean isLastMessage) {
       } else {
         boolean required = nresponses - lastsigned >= 100;
         if (required) {
-          log.debug("FORMERR: missing required signature on {}th message", nresponses);
+          errorMessage = "Missing required signature on message #" + nresponses;
+          log.debug("{}: {}", Rcode.TSIGstring(Rcode.FORMERR), errorMessage);
           message.tsigState = Message.TSIG_FAILED;
           return Rcode.FORMERR;
         } else if (isLastMessage) {
-          log.debug("FORMERR: missing required signature on last message");
+          errorMessage = "Missing required signature on last message";
+          log.debug("{}: {}", Rcode.TSIGstring(Rcode.FORMERR), errorMessage);
           message.tsigState = Message.TSIG_FAILED;
           return Rcode.FORMERR;
         } else {
-          log.trace("Intermediate message {} without signature", nresponses);
+          errorMessage = "Intermediate message #" + nresponses + " without signature";
+          log.trace("{}: {}", Rcode.TSIGstring(Rcode.FORMERR), errorMessage);
           addUnsignedMessageToMac(message, messageBytes, sharedHmac);
           return Rcode.NOERROR;
         }

diff --git a/src/main/java/org/xbill/DNS/ZoneTransferIn.java b/src/main/java/org/xbill/DNS/ZoneTransferIn.java
@@ -49,17 +49,17 @@ public class ZoneTransferIn {
   private static final int AXFR = 6;
   private static final int END = 7;
 
-  private Name zname;
+  private final Name zname;
   private int qtype;
   private int dclass;
-  private long ixfr_serial;
-  private boolean want_fallback;
+  private final long ixfr_serial;
+  private final boolean want_fallback;
   private ZoneTransferHandler handler;
 
   private SocketAddress localAddress;
-  private SocketAddress address;
+  private final SocketAddress address;
   private TCPClient client;
-  private TSIG tsig;
+  private final TSIG tsig;
   private TSIG.StreamVerifier verifier;
   private Duration timeout = Duration.ofMinutes(15);
 
@@ -155,7 +155,7 @@ public void startIXFRAdds(Record soa) {
     public void handleRecord(Record r) {
       if (ixfr != null) {
         Delta delta = ixfr.get(ixfr.size() - 1);
-        if (delta.adds.size() > 0) {
+        if (!delta.adds.isEmpty()) {
           delta.adds.add(r);
         } else {
           delta.deletes.add(r);
@@ -166,9 +166,7 @@ public void handleRecord(Record r) {
     }
   }
 
-  private ZoneTransferIn() {}
-
-  private ZoneTransferIn(
+  ZoneTransferIn(
       Name zone, int xfrtype, long serial, boolean fallback, SocketAddress address, TSIG key) {
     this.address = address;
     this.tsig = key;
@@ -330,13 +328,17 @@ public void setLocalAddress(SocketAddress addr) {
   }
 
   private void openConnection() throws IOException {
-    client = new TCPClient(timeout);
+    client = createTcpClient(timeout);
     if (localAddress != null) {
       client.bind(localAddress);
     }
     client.connect(address);
   }
 
+  TCPClient createTcpClient(Duration timeout) throws IOException {
+    return new TCPClient(timeout);
+  }
+
   private void sendQuery() throws IOException {
     Record question = Record.newRecord(zname, qtype, dclass);
 
@@ -477,9 +479,10 @@ private void parseRR(Record rec) throws ZoneTransferException {
   private void closeConnection() {
     try {
       if (client != null) {
-        client.cleanup();
+        client.close();
       }
     } catch (IOException e) {
+      // Ignore
     }
   }
 
@@ -490,7 +493,7 @@ private Message parseMessage(byte[] b) throws WireParseException {
       if (e instanceof WireParseException) {
         throw (WireParseException) e;
       }
-      throw new WireParseException("Error parsing message");
+      throw new WireParseException("Error parsing message", e);
     }
   }
 
@@ -499,14 +502,24 @@ private void doxfr() throws IOException, ZoneTransferException {
     while (state != END) {
       byte[] in = client.recv();
       Message response = parseMessage(in);
+      List<Record> answers = response.getSection(Section.ANSWER);
       if (response.getHeader().getRcode() == Rcode.NOERROR && verifier != null) {
-        int error = verifier.verify(response, in);
+        int error =
+            verifier.verify(response, in, answers.get(answers.size() - 1).getType() == Type.SOA);
         if (error != Rcode.NOERROR) {
-          fail("TSIG failure: " + Rcode.TSIGstring(error));
+          if (verifier.getErrorMessage() != null) {
+            fail(
+                "TSIG failure: "
+                    + Rcode.TSIGstring(error)
+                    + " ("
+                    + verifier.getErrorMessage()
+                    + ")");
+          } else {
+            fail("TSIG failure: " + Rcode.TSIGstring(error));
+          }
         }
       }
 
-      List<Record> answers = response.getSection(Section.ANSWER);
       if (state == INITIALSOA) {
         int rcode = response.getRcode();
         if (rcode != Rcode.NOERROR) {
@@ -533,10 +546,6 @@ private void doxfr() throws IOException, ZoneTransferException {
       for (Record answer : answers) {
         parseRR(answer);
       }
-
-      if (state == END && verifier != null && !response.isVerified()) {
-        fail("last message must be signed");
-      }
     }
   }