Enable EDNS for DNS A queries and reverse IPv4 lookups (#1864)

This change brings Squid code in sync with existing dns_packet_max
directive documentation and allows admins to enable a useful performance
optimization for still very popular IPv4-related DNS queries.

An enabled dns_packet_max feature may now break Squid compatibility with
more buggy nameservers (that appeared to "work" before), but we should
not penalize many Squid deployments (or complicate configuration and
spend a lot of development time) to accommodate a few exceptional ones,
at least until such heroic efforts are proven to be necessary.

diff --git a/doc/release-notes/release-7.sgml.in b/doc/release-notes/release-7.sgml.in
index 881ad8f141e772d52841e36b931f7912b30a1f92..35fc35d9586712653e757cd284a6e9f5f9a82c7b 100644 (file)
--- a/doc/release-notes/release-7.sgml.in
+++ b/doc/release-notes/release-7.sgml.in
@@ -172,6 +172,17 @@ This section gives an account of those changes in three categories:
        <p>Removed the <em>non_peers</em> action. See the Cache Manager
        <ref id="mgr" name="section"> for details.
 
+       <tag>dns_packet_max</tag>
+       <p>Honor positive <em>dns_packet_max</em> values when sending DNS A queries
+       and PTR queries containing IPv4 addresses. Prior to this change, Squid did
+       not add EDNS extension (RFC 6891) to those DNS queries because 2010 tests
+       revealed compatibility problems with some DNS resolvers. We hope that those
+       problems are now sufficiently rare to enable this useful optimization for
+       all DNS queries, as originally intended. Squid still sends EDNS extension
+       with DNS AAAA queries and PTR queries containing IPv6 addresses (when
+       dns_packet_max is set to a positive value). Rare deployments that must use
+       buggy DNS resolvers should not set <em>dns_packet_max</em>.
+
        <tag>access_log</tag>
        <p>Built-in <em>common</em> and <em>combined</em> logformats now always
        receive a dash character ("-") in the position of what used to be a

diff --git a/src/dns/rfc3596.cc b/src/dns/rfc3596.cc
index 997e5d94a2f3a552b50fcb2401c3461309a187e1..37a10bf9e079a13aaea8cf3b28716784864cad7c 100644 (file)
--- a/src/dns/rfc3596.cc
+++ b/src/dns/rfc3596.cc
@@ -9,6 +9,7 @@
 #include "squid.h"
 #include "dns/rfc2671.h"
 #include "dns/rfc3596.h"
+#include "SquidConfig.h"
 #include "util.h"
 
 #if HAVE_UNISTD_H
@@ -54,7 +55,7 @@
  * Returns the size of the query
  */
 ssize_t
-rfc3596BuildHostQuery(const char *hostname, char *buf, size_t sz, unsigned short qid, rfc1035_query * query, int qtype, ssize_t edns_sz)
+rfc3596BuildHostQuery(const char *hostname, char *buf, size_t sz, unsigned short qid, rfc1035_query * query, int qtype)
 {
     static rfc1035_message h;
     size_t offset = 0;
@@ -64,7 +65,10 @@ rfc3596BuildHostQuery(const char *hostname, char *buf, size_t sz, unsigned short
     h.rd = 1;
     h.opcode = 0;               /* QUERY */
     h.qdcount = (unsigned int) 1;
+
+    const auto edns_sz = Config.dns.packet_max;
     h.arcount = (edns_sz > 0 ? 1 : 0);
+
     offset += rfc1035HeaderPack(buf + offset, sz - offset, &h);
     offset += rfc1035QuestionPack(buf + offset,
                                   sz - offset,
@@ -93,9 +97,9 @@ rfc3596BuildHostQuery(const char *hostname, char *buf, size_t sz, unsigned short
  * \return the size of the query
  */
 ssize_t
-rfc3596BuildAQuery(const char *hostname, char *buf, size_t sz, unsigned short qid, rfc1035_query * query, ssize_t edns_sz)
+rfc3596BuildAQuery(const char *hostname, char *buf, size_t sz, unsigned short qid, rfc1035_query * query)
 {
-    return rfc3596BuildHostQuery(hostname, buf, sz, qid, query, RFC1035_TYPE_A, edns_sz);
+    return rfc3596BuildHostQuery(hostname, buf, sz, qid, query, RFC1035_TYPE_A);
 }
 
 /**
@@ -107,9 +111,9 @@ rfc3596BuildAQuery(const char *hostname, char *buf, size_t sz, unsigned short qi
  * \return the size of the query
  */
 ssize_t
-rfc3596BuildAAAAQuery(const char *hostname, char *buf, size_t sz, unsigned short qid, rfc1035_query * query, ssize_t edns_sz)
+rfc3596BuildAAAAQuery(const char *hostname, char *buf, size_t sz, unsigned short qid, rfc1035_query * query)
 {
-    return rfc3596BuildHostQuery(hostname, buf, sz, qid, query, RFC1035_TYPE_AAAA, edns_sz);
+    return rfc3596BuildHostQuery(hostname, buf, sz, qid, query, RFC1035_TYPE_AAAA);
 }
 
 /**
@@ -121,7 +125,7 @@ rfc3596BuildAAAAQuery(const char *hostname, char *buf, size_t sz, unsigned short
  * \return the size of the query
  */
 ssize_t
-rfc3596BuildPTRQuery4(const struct in_addr addr, char *buf, size_t sz, unsigned short qid, rfc1035_query * query, ssize_t edns_sz)
+rfc3596BuildPTRQuery4(const struct in_addr addr, char *buf, size_t sz, unsigned short qid, rfc1035_query * query)
 {
     static char rev[RFC1035_MAXHOSTNAMESZ];
     unsigned int i;
@@ -133,11 +137,11 @@ rfc3596BuildPTRQuery4(const struct in_addr addr, char *buf, size_t sz, unsigned
              (i >> 16) & 255,
              (i >> 24) & 255);
 
-    return rfc3596BuildHostQuery(rev, buf, sz, qid, query, RFC1035_TYPE_PTR, edns_sz);
+    return rfc3596BuildHostQuery(rev, buf, sz, qid, query, RFC1035_TYPE_PTR);
 }
 
 ssize_t
-rfc3596BuildPTRQuery6(const struct in6_addr addr, char *buf, size_t sz, unsigned short qid, rfc1035_query * query, ssize_t edns_sz)
+rfc3596BuildPTRQuery6(const struct in6_addr addr, char *buf, size_t sz, unsigned short qid, rfc1035_query * query)
 {
     static char rev[RFC1035_MAXHOSTNAMESZ];
     const uint8_t* r = addr.s6_addr;
@@ -152,6 +156,6 @@ rfc3596BuildPTRQuery6(const struct in6_addr addr, char *buf, size_t sz, unsigned
 
     snprintf(p,10,"ip6.arpa.");
 
-    return rfc3596BuildHostQuery(rev, buf, sz, qid, query, RFC1035_TYPE_PTR, edns_sz);
+    return rfc3596BuildHostQuery(rev, buf, sz, qid, query, RFC1035_TYPE_PTR);
 }
 

diff --git a/src/dns/rfc3596.h b/src/dns/rfc3596.h
index 10e49e4708fe29f82434bc396f1a3dda4aabc24b..3ebad864156d496d4961042dacbdd58adaec9f2f 100644 (file)
--- a/src/dns/rfc3596.h
+++ b/src/dns/rfc3596.h
@@ -16,29 +16,25 @@ ssize_t rfc3596BuildAQuery(const char *hostname,
                            char *buf,
                            size_t sz,
                            unsigned short qid,
-                           rfc1035_query * query,
-                           ssize_t edns_sz);
+                           rfc1035_query *);
 
 ssize_t rfc3596BuildAAAAQuery(const char *hostname,
                               char *buf,
                               size_t sz,
                               unsigned short qid,
-                              rfc1035_query * query,
-                              ssize_t edns_sz);
+                              rfc1035_query *);
 
 ssize_t rfc3596BuildPTRQuery4(const struct in_addr,
                               char *buf,
                               size_t sz,
                               unsigned short qid,
-                              rfc1035_query * query,
-                              ssize_t edns_sz);
+                              rfc1035_query *);
 
 ssize_t rfc3596BuildPTRQuery6(const struct in6_addr,
                               char *buf,
                               size_t sz,
                               unsigned short qid,
-                              rfc1035_query * query,
-                              ssize_t edns_sz);
+                              rfc1035_query *);
 
 /* RFC3596 library implements RFC1035 generic host interface */
 ssize_t rfc3596BuildHostQuery(const char *hostname,
@@ -46,8 +42,7 @@ ssize_t rfc3596BuildHostQuery(const char *hostname,
                               size_t sz,
                               unsigned short qid,
                               rfc1035_query * query,
-                              int qtype,
-                              ssize_t edns_sz);
+                              int qtype);
 
 /* RFC3596 section 2.1 defines new RR type AAAA as 28 */
 #define RFC1035_TYPE_AAAA 28

diff --git a/src/dns_internal.cc b/src/dns_internal.cc
index 1dcad7cb770b4727040d56ac1dd2c590d17259bb..8b4d98e9c88a1ecb1014eb088fe1b10c15e22419 100644 (file)
--- a/src/dns_internal.cc
+++ b/src/dns_internal.cc
@@ -233,13 +233,6 @@ static hash_table *idns_lookup_hash = nullptr;
 /*
  * Notes on EDNS:
  *
- * IPv4:
- *   EDNS as specified may be sent as an additional record for any request.
- *   early testing has revealed that it works on common devices, but cannot
- *   be reliably used on any A or PTR requet done for IPv4 addresses.
- *
- * As such the IPv4 packets are still hard-coded not to contain EDNS (0)
- *
  * Squid design:
  *   Squid is optimized to generate one packet and re-send it to all NS
  *   due to this we cannot customize the EDNS size per NS.
@@ -1296,8 +1289,7 @@ idnsGrokReply(const char *buf, size_t sz, int /*from_ns*/)
             // Build new query
             q->query_id = idnsQueryID();
             debugs(78, 3, "idnsGrokReply: Trying A Query for " << q->name);
-            // see EDNS notes at top of file why this sends 0
-            q->sz = rfc3596BuildAQuery(q->name, q->buf, sizeof(q->buf), q->query_id, &q->query, 0);
+            q->sz = rfc3596BuildAQuery(q->name, q->buf, sizeof(q->buf), q->query_id, &q->query);
             if (q->sz < 0) {
                 /* problem with query data -- query not sent */
                 idnsCallback(q, "Internal error");
@@ -1740,7 +1732,7 @@ idnsSendSlaveAAAAQuery(idns_query *master)
     memcpy(q->orig, master->orig, sizeof(q->orig));
     q->master = master;
     q->query_id = idnsQueryID();
-    q->sz = rfc3596BuildAAAAQuery(q->name, q->buf, sizeof(q->buf), q->query_id, &q->query, Config.dns.packet_max);
+    q->sz = rfc3596BuildAAAAQuery(q->name, q->buf, sizeof(q->buf), q->query_id, &q->query);
 
     debugs(78, 3, "buf is " << q->sz << " bytes for " << q->name <<
            ", id = 0x" << asHex(q->query_id));
@@ -1796,8 +1788,7 @@ idnsALookup(const char *name, IDNSCB * callback, void *data)
         debugs(78, 3, "idnsALookup: searchpath used for " << q->name);
     }
 
-    // see EDNS notes at top of file why this sends 0
-    q->sz = rfc3596BuildAQuery(q->name, q->buf, sizeof(q->buf), q->query_id, &q->query, 0);
+    q->sz = rfc3596BuildAQuery(q->name, q->buf, sizeof(q->buf), q->query_id, &q->query);
 
     if (q->sz < 0) {
         /* problem with query data -- query not sent */
@@ -1829,12 +1820,11 @@ idnsPTRLookup(const Ip::Address &addr, IDNSCB * callback, void *data)
     if (addr.isIPv6()) {
         struct in6_addr addr6;
         addr.getInAddr(addr6);
-        q->sz = rfc3596BuildPTRQuery6(addr6, q->buf, sizeof(q->buf), q->query_id, &q->query, Config.dns.packet_max);
+        q->sz = rfc3596BuildPTRQuery6(addr6, q->buf, sizeof(q->buf), q->query_id, &q->query);
     } else {
         struct in_addr addr4;
         addr.getInAddr(addr4);
-        // see EDNS notes at top of file why this sends 0
-        q->sz = rfc3596BuildPTRQuery4(addr4, q->buf, sizeof(q->buf), q->query_id, &q->query, 0);
+        q->sz = rfc3596BuildPTRQuery4(addr4, q->buf, sizeof(q->buf), q->query_id, &q->query);
     }
 
     if (q->sz < 0) {