Fix DNS-over-HTTP(S) implementation issues that arise under heavy
query load. Optimize resource usage for :iscman:`named` instances
that accept queries over DNS-over-HTTP(S).
Previously, :iscman:`named` would process all incoming HTTP/2 data
at once, which could overwhelm the server, especially when dealing
with clients that send requests but don't wait for responses. That
has been fixed. Now, :iscman:`named` handles HTTP/2 data in smaller
chunks and throttles reading until the remote side reads the
response data. It also throttles clients that send too many requests
at once.
Additionally, :iscman:`named` now carefully processes data sent by
some clients, which can be considered "flooding." It logs these
clients and drops connections from them.
:gl:`#4795`
In some cases, :iscman:`named` could leave DNS-over-HTTP(S)
connections in the `CLOSE_WAIT` state indefinitely. That also has
been fixed. ISC would like to thank JF Billaud for thoroughly
investigating the issue and verifying the fix.
:gl:`#5083`
See https://gitlab.isc.org/isc-projects/bind9/-/issues/4795
Closes https://gitlab.isc.org/isc-projects/bind9/-/issues/5083
Backport of !732.
Merge branch 'artem-improve-doh-resource-usage-9.20' into 'v9.20.5-release'
See merge request isc-private/bind9!766
We started using isc_nm_bad_request() more actively throughout
codebase. In the case of HTTP/2 it can lead to a large count of
useless "Bad Request" messages in the BIND log, as often we attempt to
send such request over effectively finished HTTP/2 sessions.
This commit fixes that.
(cherry picked from commit 937b5f8349)
A call to isc_nm_read_stop() would always stop reading timer even in
manual timer control mode which was added with StreamDNS in mind. That
looks like an omission that happened due to how timers are controlled
in StreamDNS where we always stop the timer before pausing reading
anyway (see streamdns_on_complete_dnsmessage()). That would not work
well for HTTP, though, where we might want pause reading without
stopping the timer in the case we want to split incoming data into
multiple chunks to be processed independently.
I suppose that it happened due to NM refactoring in the middle of
StreamDNS development (at the time isc_nm_cancelread() and
isc_nm_pauseread() were removed), as the StreamDNS code seems to be
written as if timers are not stoping during a call to
isc_nm_read_stop().
(cherry picked from commit 4ae4e255cf)
This commit introduces manual read timer control as used by StreamDNS
and its underlying transports. Before that, DoH code would rely on the
timer control provided by TCP, which would reset the timer any time
some data arrived. Now, the timer is restarted only when a full DNS
message is processed in line with other DNS transports.
That change is required because we should not stop the timer when
reading from the network is paused due to throttling. We need a way to
drop timed-out clients, particularly those who refuse to read the data
we send.
(cherry picked from commit 609a41517b)
This commit adds logic to make code better protected against clients
that send valid HTTP/2 data that is useless from a DNS server
perspective.
Firstly, it adds logic that protects against clients who send too
little useful (=DNS) data. We achieve that by adding a check that
eventually detects such clients with a nonfavorable useful to
processed data ratio after the initial grace period. The grace period
is limited to processing 128 KiB of data, which should be enough for
sending the largest possible DNS message in a GET request and then
some. This is the main safety belt that would detect even flooding
clients that initially behave well in order to fool the checks server.
Secondly, in addition to the above, we introduce additional checks to
detect outright misbehaving clients earlier:
The code will treat clients that open too many streams (50) without
sending any data for processing as flooding ones; The clients that
managed to send 1.5 KiB of data without opening a single stream or
submitting at least some DNS data will be treated as flooding ones.
Of course, the behaviour described above is nothing else but
heuristical checks, so they can never be perfect. At the same time,
they should be reasonable enough not to drop any valid clients,
realatively easy to implement, and have negligible computational
overhead.
(cherry picked from commit 3425e4b1d0)
Initially, our DNS-over-HTTP(S) implementation would try to process as
much incoming data from the network as possible. However, that might
be undesirable as we might create too many streams (each effectively
backed by a ns_client_t object). That is too forgiving as it might
overwhelm the server and trash its memory allocator, causing high CPU
and memory usage.
Instead of doing that, we resort to processing incoming data using a
chunk-by-chunk processing strategy. That is, we split data into small
chunks (currently 256 bytes) and process each of them
asynchronously. However, we can process more than one chunk at
once (up to 4 currently), given that the number of HTTP/2 streams has
not increased while processing a chunk.
That alone is not enough, though. In addition to the above, we should
limit the number of active streams: these streams for which we have
received a request and started processing it (the ones for which a
read callback was called), as it is perfectly fine to have more opened
streams than active ones. In the case we have reached or surpassed the
limit of active streams, we stop reading AND processing the data from
the remote peer. The number of active streams is effectively decreased
only when responses associated with the active streams are sent to the
remote peer.
Overall, this strategy is very similar to the one used for other
stream-based DNS transports like TCP and TLS.
(cherry picked from commit 9846f395ad)
When answering queries, don't add data to the additional section if the answer has more than 13 names in the RDATA. This limits the number of lookups into the database(s) during a single client query, reducing query processing load.
Backport of MR !750
See isc-projects/bind9#5034
Merge branch '5034-security-limit-additional-9.20' into 'v9.20.5-release'
See merge request isc-private/bind9!758
When answering queries, don't add data to the additional section if
the answer has more than 13 names in the RDATA. This limits the
number of lookups into the database(s) during a single client query,
reducing query processing load.
Also, don't append any additional data to type=ANY queries. The
answer to ANY is already big enough.
(cherry picked from commit a1982cf1bb)
Instead of running the whole resolver/ns4 server with -T noaa flag,
use it only for the part where it is actually needed. The -T noaa
could interfere with other parts of the test because the answers don't
have the authoritative-answer bit set, and we could have false
positives (or false negatives) in the test because the authoritative
server doesn't follow the DNS protocol for all the tests in the resolver
system test.
(cherry picked from commit e51d4d3b88)
Revert the fix for #4950 for 9.20.
This reverts MR !9438.
History: A performance improvement for NSEC3 closest encloser lookups (#4460) was introduced (in MR !9436) and backported to 9.20 (MR !9438) and to 9.18 in (MR !9439). It was released in 9.18.30 (and 9.20.2 and 9.21.1).
There was a bug in the code (#4950), so we reverted the change in !9611, !9613 and !9614 (not released).
Then a new attempt was merged in main (MR !9610) and backported to 9.20 (MR !9631) and 9.18 (MR !9632). The latter should not have been backported and was reverted in !9689.
We now also revert the fix for 9.20
Related #5108
Merge branch 'revert-4950-bind-logs-expected-covering-nsec3-got-an-exact-match-9.20' into 'bind-9.20'
See merge request isc-projects/bind9!9947
Closes#5115
Backport of MR !9934
Merge branch 'backport-5115-fix-default-iana-root-zone-mirror-configuration-9.20' into 'bind-9.20'
See merge request isc-projects/bind9!9944
Commit 4555a31934 renamed the top-level
"primaries" block in bin/named/config.c to "remote-servers". This
configuration block lists the primary servers used for an IANA root zone
mirror when no primary servers are explicitly specified for it in the
configuration. However, the relevant part of the named_zone_configure()
function only looks for a top-level "primaries" block and not for any of
its synonyms. As a result, configuring an IANA root zone mirror with
just:
zone "." {
type mirror;
};
now results in a cryptic fatal error on startup:
loading configuration: not found
exiting (due to fatal error)
Fix by using the correct top-level block name in named_zone_configure().
(cherry picked from commit 010d2eb436)
Response policy zones (RPZ) and catalog zones were not working correctly if they had an $INCLUDE statement defined. This has been fixed.
Closes#5111
Backport of MR !9930
Merge branch 'backport-5111-includes-disable-rpz-and-catz-fix-9.20' into 'bind-9.20'
See merge request isc-projects/bind9!9941
The code in zone_startload() disables RPZ and CATZ for a zone if
dns_master_loadfile() returns anything other than ISC_R_SUCCESS,
which makes sense, but it's an error because zone_startload() can
also return DNS_R_SEENINCLUDE upon success when the zone had an
$INCLUDE statement.
(cherry picked from commit 3d7a9fba3b)
Add performance tests of DoH using the GET protocol to nightly pipelines.
Backport of MR !9926
Merge branch 'backport-nicki/ci-shotgun-doh-get-9.20' into 'bind-9.20'
See merge request isc-projects/bind9!9939
ThreadSanitizer has trouble understanding that gluelist->glue is
constant after it is assigned to the slabheader with cmpxchg. Help
ThreadSanitizer to understand the code by using CMM_STORE_SHARED and
CMM_LOAD_SHARED on gluelist->glue.
Backport of MR !9929
Merge branch 'backport-ondrej/hint-tsan-in-addglue-9.20' into 'bind-9.20'
See merge request isc-projects/bind9!9936
For performance reasons, the returned GLUE records are cached on the first use. The current implementation could randomly cause a performance drop and increased memory use. This has been fixed.
Closes#5064
Backport of MR !9831
Merge branch 'backport-5064-rewrite-glue-cache-in-qpzone-9.20' into 'bind-9.20'
See merge request isc-projects/bind9!9918
This is a second attempt to rewrite the GLUE cache to not use per
database version hash table. Instead of keeping a hash table indexed by
the node, use a directly linked list of GLUE records for each
slabheader. This was attempted before, but there was a data race caused
by the fact that the thread cleaning the GLUE records could be slower
than accessing the slab headers again and reinitializing the wait-free
stack.
The improved design builds on the previous design, but adds a new
dns_gluelist structure that has a pointer to the database version.
If a dns_gluelist belonging to a different (old) version is detected, it
is just detached from the slabheader and left for the closeversion() to
clean it up later.
(cherry picked from commit 29bde687b5)
Adjust number of zones down to 23 to match those present when testing in FIPS mode.
Closes#5097
Backport of MR !9919
Merge branch 'backport-5097-checking-startup-notify-rate-limit-fails-on-ol-8-fips-9.20' into 'bind-9.20'
See merge request isc-projects/bind9!9935
This commit ensures that BIND enables TLS SNI support for outgoing DoT
connections (when possible) in order to improve compatibility with
other DNS server software.
(cherry picked from commit 740292d3ec)
This commit ensures that dig enables TLS SNI support for outgoing
connections in order to improve compatibility with other DNS server
software.
(cherry picked from commit e04fb30ee6)
This commit adds support for setting SNI hostnames in outgoing
connections over TLS.
Most of the changes are related to either adapting the code to accept
and extra argument in *connect() functions and a couple of changes to
the TLS Stream to actually make use of the new SNI hostname
information.
(cherry picked from commit 6691a1530d)
coccinelle v1.1 trips over a superfluous isc_mem_get() NULL check in
tests/libtest/ns.c and reports the following failure in CI:
EXN: Failure("rule starting on line 26: already tagged token:\nC code context\nFile \"./tests/libtest/ns.c\", line 350, column 1, charpos = 7939\n around = 'if',\n whole content = \tif (qctx != NULL) {") in ./tests/libtest/ns.c
(cherry picked from commit cf76851c75)
coccinelle v1.1 reports the following failure:
EXN: Failure("./lib/dns/sdlz.c: 172: try to delete an expanded token: unsigned") in ./lib/dns/sdlz.c
coccinelle v1.2 reports the following failure:
EXN: Failure("./util/models.c: 21: try to delete an expanded token: unsigned") in ./util/models.c
(cherry picked from commit 7fb6be62e0)
The top blocks 'primaries' and 'parental-agents' are no longer preferred and should be renamed to 'remote-servers'. The zone statements 'parental-agents' and 'primaries' are still used, and may refer to any 'remote-servers' top block.
Closes#4544
Backport of MR !9822
Merge branch 'backport-4544-primaries-block-documentation-issues-9.20' into 'bind-9.20'
See merge request isc-projects/bind9!9911
The 'remote-servers' named.conf reference conflicts with the standard
term from the glossary. Rename the standard term to server-list to
make the docs build.
(cherry picked from commit 726c9cd73b)
Add back the top blocks 'parental-agents', 'primaries', and 'masters'
to the configuration. Do not document them as so many names for the
same clause is confusing.
This has a slight negative side effect that a top block 'primaries'
can be referred to with a zone statement 'parental-agents' for example,
but that shouldn't be a big issue.
(cherry picked from commit 1b2eadb197)
Having zone statements that are also top blocks is confusing, and if
we want to add more in the future (which I suspect will be for
generalized notifications, multi-signer), we need to duplicate a lot
of code.
Remove top blocks 'parental-agents' and 'primaries' and just have one
top block 'remote-servers' that you can refer to with zone statements.
(cherry picked from commit b121f02eac)
Fix the loop terminating condition to get consistent sample sizes and increase the minimum number of samples from 20 to 40.
Closes#5091
Backport of MR !9894
Merge branch 'backport-5091-investigate-checking-startup-notify-rate-limit-failure-9.20' into 'bind-9.20'
See merge request isc-projects/bind9!9909
The terminating conditions for the startup notify test would
occasionally get ~20 records or get +10 seconds of records due to
a bad terminating condition. Additionally 20 samples lead to test
failures. Fix the terminating condition to use the correct conditional
(-eq -> -ge) and increase the minimum number of log entries to
average over to 40.
(cherry picked from commit 46388d07a2)
Closes#5088
Backport of MR !9884
Merge branch 'backport-5088-tests-irs-resconf_test-c-is-missing-check-callbacks-9.20' into 'bind-9.20'
See merge request isc-projects/bind9!9907
gcovr has issues with processing files produced as part of a BIND 9
build with tracing support enabled (--enable-tracing). Depending on the
gcovr version used, these issues may result in either warnings or
failures being reported by that tool. Disable tracing support for
gcovr-enabled builds to work around these issues.
(cherry picked from commit 7048b3ab0d)
Include the recent changes such as:
- changes to running system tests
- gitlab development workflow
- changelog and release note process
Closes#5045
Backport of MR !9784
Merge branch 'backport-5045-update-contributing-9.20' into 'bind-9.20'
See merge request isc-projects/bind9!9902
