dnsdist: Fix a crash when X-Forwarded-For overrides the initial source IP

When both the processing of X-Forwarded-For DNS-over-https headers
(`trustForwardedForHeader=true`) and a maximum number of concurrent
TCP connections per client (`setMaxTCPConnectionsPerClient()`) are
enabled, dnsdist could crash because of an uncaught exception:
```
dnsdist[X]: terminate called after throwing an instance of 'std::runtime_error'
dnsdist[X]:   what():  DOH thread failed to launch: map::at
```
This was caused by the TCP connection being first accounted for with the
initial source IP (from the upstream HTTP proxy) but later released using
the IP extracted from the X-Forwarded-For header, leading to an unexpected
failure to locate the corresponding entry in the map.

We might not actually want to enforce the maximum number of concurrent
TCP connections per client when X-Forwarded-For processing is enabled,
though, because we usually want to rate limit the actual client and
not the HTTP proxy, but X-Forwarded-For being set per HTTP query, instead
of per-connection, makes that pretty much impossible at our level since
the same connection from the HTTP proxy can be reused for several clients.
The proxy protocol would be a better option to enforce that limit.

(cherry picked from commit 9b703b51ca25838eeec19449a1c49cb926aef52a)

diff --git a/pdns/dnsdistdist/doh.cc b/pdns/dnsdistdist/doh.cc
index 8481193d877b901a4c32dbbd63528a154b3b93f8..1c8610255f25dae004cc613353cfd81fe107bb6e 100644 (file)
--- a/pdns/dnsdistdist/doh.cc
+++ b/pdns/dnsdistdist/doh.cc
@@ -951,7 +951,7 @@ static bool getHTTPHeaderValue(const h2o_req_t* req, const std::string& headerNa
 }
 
 /* can only be called from the main DoH thread */
-static void processForwardedForHeader(const h2o_req_t* req, ComboAddress& remote)
+static std::optional<ComboAddress> processForwardedForHeader(const h2o_req_t* req, const ComboAddress& remote)
 {
   static const std::string headerName = "x-forwarded-for";
   std::string_view value;
@@ -969,8 +969,7 @@ static void processForwardedForHeader(const h2o_req_t* req, ComboAddress& remote
           value = value.substr(pos);
         }
       }
-      auto newRemote = ComboAddress(std::string(value));
-      remote = newRemote;
+      return ComboAddress(std::string(value));
     }
     catch (const std::exception& e) {
       vinfolog("Invalid X-Forwarded-For header ('%s') received from %s : %s", std::string(value), remote.toStringWithPort(), e.what());
@@ -979,6 +978,8 @@ static void processForwardedForHeader(const h2o_req_t* req, ComboAddress& remote
       vinfolog("Invalid X-Forwarded-For header ('%s') received from %s : %s", std::string(value), remote.toStringWithPort(), e.reason);
     }
   }
+
+  return std::nullopt;
 }
 
 /*
@@ -1013,14 +1014,18 @@ static int doh_handler(h2o_handler_t *self, h2o_req_t *req)
       h2o_socket_getsockname(sock, reinterpret_cast<struct sockaddr*>(&conn.d_local));
     }
 
+    auto remote = conn.d_remote;
     if (dsc->df->d_trustForwardedForHeader) {
-      processForwardedForHeader(req, conn.d_remote);
+      auto newRemote = processForwardedForHeader(req, remote);
+      if (newRemote) {
+        remote = std::move(*newRemote);
+      }
     }
 
     auto& holders = dsc->holders;
-    if (!holders.acl->match(conn.d_remote)) {
+    if (!holders.acl->match(remote)) {
       ++g_stats.aclDrops;
-      vinfolog("Query from %s (DoH) dropped because of ACL", conn.d_remote.toStringWithPort());
+      vinfolog("Query from %s (DoH) dropped because of ACL", remote.toStringWithPort());
       h2o_send_error_403(req, "Forbidden", "dns query not allowed because of ACL", 0);
       return 0;
     }
@@ -1076,7 +1081,7 @@ static int doh_handler(h2o_handler_t *self, h2o_req_t *req)
       query.reserve(req->entity.len + maxAdditionalSizeForEDNS);
       query.resize(req->entity.len);
       memcpy(query.data(), req->entity.base, req->entity.len);
-      doh_dispatch_query(dsc, self, req, std::move(query), conn.d_local, conn.d_remote, std::move(path));
+      doh_dispatch_query(dsc, self, req, std::move(query), conn.d_local, remote, std::move(path));
     }
     else if(req->query_at != SIZE_MAX && (req->path.len - req->query_at > 5)) {
       auto pos = path.find("?dns=");
@@ -1115,7 +1120,7 @@ static int doh_handler(h2o_handler_t *self, h2o_req_t *req)
           else
             ++dsc->df->d_http1Stats.d_nbQueries;
 
-          doh_dispatch_query(dsc, self, req, std::move(decoded), conn.d_local, conn.d_remote, std::move(path));
+          doh_dispatch_query(dsc, self, req, std::move(decoded), conn.d_local, remote, std::move(path));
         }
       }
       else

diff --git a/regression-tests.dnsdist/test_DOH.py b/regression-tests.dnsdist/test_DOH.py
index 5ed6382085f6f3dbc54c7a970a5850a975cd37c3..15474342f4cfbf28707b0ee2bfc1567deaef5929 100644 (file)
--- a/regression-tests.dnsdist/test_DOH.py
+++ b/regression-tests.dnsdist/test_DOH.py
@@ -1064,6 +1064,9 @@ class TestDOHForwardedFor(DNSDistDOHTest):
 
     setACL('192.0.2.1/32')
     addDOHLocal("127.0.0.1:%s", "%s", "%s", { "/" }, {trustForwardedForHeader=true})
+    -- Set a maximum number of TCP connections per client, to exercise
+    -- that code along with X-Forwarded-For support
+    setMaxTCPConnectionsPerClient(2)
     """
     _config_params = ['_testServerPort', '_dohServerPort', '_serverCert', '_serverKey']