From: Remi Gacogne Date: Thu, 30 Jun 2022 11:40:58 +0000 (+0200) Subject: dnsdist: Speed up DoH handling by preventing allocations and copies X-Git-Tag: rec-4.9.0-alpha0~15^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a8161cdbb3112d1d4462f17ca73de28b893f6c66;p=thirdparty%2Fpdns.git dnsdist: Speed up DoH handling by preventing allocations and copies --- diff --git a/pdns/dnsdist-lua.cc b/pdns/dnsdist-lua.cc index c81cb6cbde..d6c6740995 100644 --- a/pdns/dnsdist-lua.cc +++ b/pdns/dnsdist-lua.cc @@ -2441,6 +2441,10 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) frontend->d_sendCacheControlHeaders = boost::get((*vars)["sendCacheControlHeaders"]); } + if (vars->count("keepIncomingHeaders")) { + frontend->d_keepIncomingHeaders = boost::get((*vars)["keepIncomingHeaders"]); + } + if (vars->count("trustForwardedForHeader")) { frontend->d_trustForwardedForHeader = boost::get((*vars)["trustForwardedForHeader"]); } diff --git a/pdns/dnsdistdist/dnsdist-lua-ffi.cc b/pdns/dnsdistdist/dnsdist-lua-ffi.cc index 4a61e942f7..208c7eaf4b 100644 --- a/pdns/dnsdistdist/dnsdist-lua-ffi.cc +++ b/pdns/dnsdistdist/dnsdist-lua-ffi.cc @@ -319,19 +319,22 @@ size_t dnsdist_ffi_dnsquestion_get_edns_options(dnsdist_ffi_dnsquestion_t* dq, c totalCount += option.second.values.size(); } - dq->ednsOptionsVect.clear(); - dq->ednsOptionsVect.resize(totalCount); + if (!dq->ednsOptionsVect) { + dq->ednsOptionsVect = std::make_unique>(); + } + dq->ednsOptionsVect->clear(); + dq->ednsOptionsVect->resize(totalCount); size_t pos = 0; for (const auto& option : *dq->dq->ednsOptions) { for (const auto& entry : option.second.values) { - fill_edns_option(entry, dq->ednsOptionsVect.at(pos)); - dq->ednsOptionsVect.at(pos).optionCode = option.first; + fill_edns_option(entry, dq->ednsOptionsVect->at(pos)); + dq->ednsOptionsVect->at(pos).optionCode = option.first; pos++; } } if (totalCount > 0) { - *out = dq->ednsOptionsVect.data(); + *out = dq->ednsOptionsVect->data(); } return totalCount; @@ -344,21 +347,28 @@ size_t dnsdist_ffi_dnsquestion_get_http_headers(dnsdist_ffi_dnsquestion_t* dq, c } #ifdef HAVE_DNS_OVER_HTTPS - dq->httpHeaders = dq->dq->du->getHTTPHeaders(); - dq->httpHeadersVect.clear(); - dq->httpHeadersVect.resize(dq->httpHeaders.size()); + auto headers = dq->dq->du->getHTTPHeaders(); + if (headers.size() == 0) { + return 0; + } + dq->httpHeaders = std::make_unique>(std::move(headers)); + if (!dq->httpHeadersVect) { + dq->httpHeadersVect = std::make_unique>(); + } + dq->httpHeadersVect->clear(); + dq->httpHeadersVect->resize(dq->httpHeaders->size()); size_t pos = 0; - for (const auto& header : dq->httpHeaders) { - dq->httpHeadersVect.at(pos).name = header.first.c_str(); - dq->httpHeadersVect.at(pos).value = header.second.c_str(); + for (const auto& header : *dq->httpHeaders) { + dq->httpHeadersVect->at(pos).name = header.first.c_str(); + dq->httpHeadersVect->at(pos).value = header.second.c_str(); ++pos; } - if (!dq->httpHeadersVect.empty()) { - *out = dq->httpHeadersVect.data(); + if (!dq->httpHeadersVect->empty()) { + *out = dq->httpHeadersVect->data(); } - return dq->httpHeadersVect.size(); + return dq->httpHeadersVect->size(); #else return 0; #endif @@ -370,23 +380,26 @@ size_t dnsdist_ffi_dnsquestion_get_tag_array(dnsdist_ffi_dnsquestion_t* dq, cons return 0; } - dq->tagsVect.clear(); - dq->tagsVect.resize(dq->dq->qTag->size()); + if (!dq->tagsVect) { + dq->tagsVect = std::make_unique>(); + } + dq->tagsVect->clear(); + dq->tagsVect->resize(dq->dq->qTag->size()); size_t pos = 0; for (const auto& tag : *dq->dq->qTag) { - auto& entry = dq->tagsVect.at(pos); + auto& entry = dq->tagsVect->at(pos); entry.name = tag.first.c_str(); entry.value = tag.second.c_str(); ++pos; } - if (!dq->tagsVect.empty()) { - *out = dq->tagsVect.data(); + if (!dq->tagsVect->empty()) { + *out = dq->tagsVect->data(); } - return dq->tagsVect.size(); + return dq->tagsVect->size(); } void dnsdist_ffi_dnsquestion_set_result(dnsdist_ffi_dnsquestion_t* dq, const char* str, size_t strSize) diff --git a/pdns/dnsdistdist/dnsdist-lua-ffi.hh b/pdns/dnsdistdist/dnsdist-lua-ffi.hh index 6971d3cd77..b60589de77 100644 --- a/pdns/dnsdistdist/dnsdist-lua-ffi.hh +++ b/pdns/dnsdistdist/dnsdist-lua-ffi.hh @@ -46,17 +46,17 @@ struct dnsdist_ffi_dnsquestion_t } DNSQuestion* dq{nullptr}; - std::vector ednsOptionsVect; - std::vector httpHeadersVect; - std::vector tagsVect; - std::unordered_map httpHeaders; - std::string trailingData; ComboAddress maskedRemote; + std::string trailingData; boost::optional result{boost::none}; boost::optional httpPath{boost::none}; boost::optional httpQueryString{boost::none}; boost::optional httpHost{boost::none}; boost::optional httpScheme{boost::none}; + std::unique_ptr> ednsOptionsVect; + std::unique_ptr> httpHeadersVect; + std::unique_ptr> tagsVect; + std::unique_ptr> httpHeaders; }; // dnsdist_ffi_dnsresponse_t is a lightuserdata diff --git a/pdns/dnsdistdist/docs/reference/config.rst b/pdns/dnsdistdist/docs/reference/config.rst index 82cf84142b..659bdf117a 100644 --- a/pdns/dnsdistdist/docs/reference/config.rst +++ b/pdns/dnsdistdist/docs/reference/config.rst @@ -120,6 +120,7 @@ Listen Sockets .. versionchanged:: 1.8.0 ``certFile`` now accepts a TLSCertificate object or a list of such objects (see :func:`newTLSCertificate`) + ``keepIncomingHeaders`` option added. Listen on the specified address and TCP port for incoming DNS over HTTPS connections, presenting the specified X.509 certificate. If no certificate (or key) files are specified, listen for incoming DNS over HTTP connections instead. @@ -160,6 +161,7 @@ Listen Sockets * ``maxConcurrentTCPConnections=0``: int - Maximum number of concurrent incoming TCP connections. The default is 0 which means unlimited. * ``releaseBuffers=true``: bool - Whether OpenSSL should release its I/O buffers when a connection goes idle, saving roughly 35 kB of memory per connection. * ``enableRenegotiation=false``: bool - Whether secure TLS renegotiation should be enabled. Disabled by default since it increases the attack surface and is seldom used for DNS. + * ``keepIncomingHeaders``: bool - Whether to retain the incoming headers in memory, to be able to use :func:`HTTPHeaderRule` or :meth:`DNSQuestion.getHTTPHeaders`. Default is false. Before 1.8.0 the headers were always kept in-memory. .. function:: addTLSLocal(address, certFile(s), keyFile(s) [, options]) diff --git a/pdns/dnsdistdist/docs/reference/dq.rst b/pdns/dnsdistdist/docs/reference/dq.rst index d80a5bb927..8055269570 100644 --- a/pdns/dnsdistdist/docs/reference/dq.rst +++ b/pdns/dnsdistdist/docs/reference/dq.rst @@ -110,8 +110,11 @@ This state can be modified from the various hooks. .. method:: DNSQuestion:getHTTPHeaders() -> table .. versionadded:: 1.4.0 + .. versionchanged:: 1.8.0 + see ``keepIncomingHeaders`` on :func:`addDOHLocal` Return the HTTP headers for a DoH query, as a table whose keys are the header names and values the header values. + Since 1.8.0 it is necessary to set the ``keepIncomingHeaders`` option to true on :func:`addDOHLocal` to be able to use this method. :returns: A table of HTTP headers diff --git a/pdns/dnsdistdist/docs/rules-actions.rst b/pdns/dnsdistdist/docs/rules-actions.rst index 1fede55e76..9ac9c7289a 100644 --- a/pdns/dnsdistdist/docs/rules-actions.rst +++ b/pdns/dnsdistdist/docs/rules-actions.rst @@ -469,7 +469,11 @@ These ``DNSRule``\ s be one of the following items: .. versionadded:: 1.4.0 + .. versionchanged:: 1.8.0 + see ``keepIncomingHeaders`` on :func:`addDOHLocal` + Matches DNS over HTTPS queries with a HTTP header ``name`` whose content matches the regular expression ``regex``. + Since 1.8.0 it is necessary to set the ``keepIncomingHeaders`` option to true on :func:`addDOHLocal` to be able to use this rule. :param str name: The case-insensitive name of the HTTP header to match on :param str regex: A regular expression to match the content of the specified header diff --git a/pdns/dnsdistdist/doh.cc b/pdns/dnsdistdist/doh.cc index 4a7f498df6..7aedbd6ba8 100644 --- a/pdns/dnsdistdist/doh.cc +++ b/pdns/dnsdistdist/doh.cc @@ -213,7 +213,7 @@ struct DOHServerConfig DOHServerConfig& operator=(const DOHServerConfig&) = delete; LocalHolders holders; - std::set paths; + std::set> paths; h2o_globalconf_t h2o_config; h2o_context_t h2o_ctx; std::shared_ptr accept_ctx{nullptr}; @@ -829,28 +829,30 @@ static void doh_dispatch_query(DOHServerConfig* dsc, h2o_handler_t* self, h2o_re { try { /* we only parse it there as a sanity check, we will parse it again later */ - uint16_t qtype; - DNSName qname(reinterpret_cast(query.data()), query.size(), sizeof(dnsheader), false, &qtype); + DNSPacketMangler mangler(reinterpret_cast(query.data()), query.size()); + mangler.skipDomainName(); + mangler.skipBytes(4); - auto du = std::make_unique(); + /* we are doing quite some copies here, sorry about that, + but we can't keep accessing the req object once we are in a different thread + because the request might get killed by h2o at pretty much any time */ + auto du = std::make_unique(std::move(query), std::move(path), std::string(req->authority.base, req->authority.len)); du->dsc = dsc; du->req = req; du->ids.origDest = local; du->ids.origRemote = remote; du->rsock = dsc->dohresponsepair[0]; - du->query = std::move(query); - du->path = std::move(path); - /* we are doing quite some copies here, sorry about that, - but we can't keep accessing the req object once we are in a different thread - because the request might get killed by h2o at pretty much any time */ if (req->scheme != nullptr) { du->scheme = std::string(req->scheme->name.base, req->scheme->name.len); } - du->host = std::string(req->authority.base, req->authority.len); du->query_at = req->query_at; - du->headers.reserve(req->headers.size); - for (size_t i = 0; i < req->headers.size; ++i) { - du->headers[std::string(req->headers.entries[i].name->base, req->headers.entries[i].name->len)] = std::string(req->headers.entries[i].value.base, req->headers.entries[i].value.len); + + if (dsc->df->d_keepIncomingHeaders) { + du->headers = std::make_unique>(); + du->headers->reserve(req->headers.size); + for (size_t i = 0; i < req->headers.size; ++i) { + (*du->headers)[std::string(req->headers.entries[i].name->base, req->headers.entries[i].name->len)] = std::string(req->headers.entries[i].value.base, req->headers.entries[i].value.len); + } } #ifdef HAVE_H2O_SOCKET_GET_SSL_SERVER_NAME @@ -886,7 +888,7 @@ static void doh_dispatch_query(DOHServerConfig* dsc, h2o_handler_t* self, h2o_re } } } - catch(const std::exception& e) { + catch (const std::exception& e) { vinfolog("Had error parsing DoH DNS packet from %s: %s", remote.toStringWithPort(), e.what()); h2o_send_error_400(req, "Bad Request", "The DNS query could not be parsed", 0); } @@ -1006,10 +1008,7 @@ static int doh_handler(h2o_handler_t *self, h2o_req_t *req) } if (dsc->df->d_exactPathMatching) { - // would be nice to be able to use a pdns_string_view there, but we would need heterogeneous lookups - // (having string in the set and compare them to string_view, for example. Note that comparing - // two boost::string_view uses the pointer, not the content). - const std::string pathOnly(req->path_normalized.base, req->path_normalized.len); + const std::string_view pathOnly(req->path_normalized.base, req->path_normalized.len); if (dsc->paths.count(pathOnly) == 0) { h2o_send_error_404(req, "Not Found", "there is no endpoint configured for this path", 0); return 0; @@ -1116,11 +1115,11 @@ HTTPHeaderRule::HTTPHeaderRule(const std::string& header, const std::string& reg bool HTTPHeaderRule::matches(const DNSQuestion* dq) const { - if (!dq->du) { + if (!dq->du || !dq->du->headers) { return false; } - for (const auto& header : dq->du->headers) { + for (const auto& header : *dq->du->headers) { if (header.first == d_header) { return d_regex.match(header.second); } @@ -1179,10 +1178,12 @@ string HTTPPathRegexRule::toString() const std::unordered_map DOHUnit::getHTTPHeaders() const { std::unordered_map results; - results.reserve(headers.size()); + if (headers) { + results.reserve(headers->size()); - for (const auto& header : headers) { - results.insert(header); + for (const auto& header : *headers) { + results.insert(header); + } } return results; diff --git a/pdns/doh.hh b/pdns/doh.hh index 167f214866..d717f05e35 100644 --- a/pdns/doh.hh +++ b/pdns/doh.hh @@ -118,6 +118,7 @@ struct DOHFrontend /* whether we require tue query path to exactly match one of configured ones, or accept everything below these paths. */ bool d_exactPathMatching{true}; + bool d_keepIncomingHeaders{false}; time_t getTicketsKeyRotationDelay() const { @@ -191,10 +192,11 @@ struct DownstreamState; struct DOHUnit { - DOHUnit() + DOHUnit(PacketBuffer&& q, std::string&& p, std::string&& h): path(std::move(p)), host(std::move(h)), query(std::move(q)) { ids.ednsAdded = false; } + DOHUnit(const DOHUnit&) = delete; DOHUnit& operator=(const DOHUnit&) = delete; @@ -227,10 +229,10 @@ struct DOHUnit std::string scheme; std::string host; std::string contentType; - std::unordered_map headers; PacketBuffer query; PacketBuffer response; std::shared_ptr downstream{nullptr}; + std::unique_ptr> headers; st_h2o_req_t* req{nullptr}; DOHUnit** self{nullptr}; DOHServerConfig* dsc{nullptr}; diff --git a/regression-tests.dnsdist/test_DOH.py b/regression-tests.dnsdist/test_DOH.py index 4f7d1ec600..e3198ecf54 100644 --- a/regression-tests.dnsdist/test_DOH.py +++ b/regression-tests.dnsdist/test_DOH.py @@ -24,7 +24,7 @@ class TestDOH(DNSDistDOHTest): _config_template = """ newServer{address="127.0.0.1:%s"} - addDOHLocal("127.0.0.1:%s", "%s", "%s", { "/", "/coffee", "/PowerDNS", "/PowerDNS2", "/PowerDNS-999" }, {customResponseHeaders={["access-control-allow-origin"]="*",["user-agent"]="derp",["UPPERCASE"]="VaLuE"}}) + addDOHLocal("127.0.0.1:%s", "%s", "%s", { "/", "/coffee", "/PowerDNS", "/PowerDNS2", "/PowerDNS-999" }, {customResponseHeaders={["access-control-allow-origin"]="*",["user-agent"]="derp",["UPPERCASE"]="VaLuE"}, keepIncomingHeaders=true}) dohFE = getDOHFrontend(0) dohFE:setResponsesMap({newDOHResponseMapEntry('^/coffee$', 418, 'C0FFEE', {['FoO']='bar'})}) @@ -1002,7 +1002,7 @@ class TestDOHFFI(DNSDistDOHTest): _config_template = """ newServer{address="127.0.0.1:%s"} - addDOHLocal("127.0.0.1:%s", "%s", "%s", { "/" }, {customResponseHeaders={["access-control-allow-origin"]="*",["user-agent"]="derp",["UPPERCASE"]="VaLuE"}}) + addDOHLocal("127.0.0.1:%s", "%s", "%s", { "/" }, {customResponseHeaders={["access-control-allow-origin"]="*",["user-agent"]="derp",["UPPERCASE"]="VaLuE"}, keepIncomingHeaders=true}) local ffi = require("ffi")