This commit adds an implementation and documentation for a
'masterfile-format auto' feature which automatically recognizes the
existing master file's format or uses the default format for the
particular zone type when there is no existing master file.
This commit changes the DoH code in such a way that it makes no
assumptions regarding which headers are expected to be processed
first. In particular, the code expected the :method: pseudo-header to
be processed early, which might not be true.
Add a statschannel test case to confirm that when keys are removed
(in this case because of a dnssec-policy change), the corresponding
dnssec-sign stats are cleared and are no longer shown in the
statistics.
Clear the key slots for dnssec-sign statistics for keys that are
removed. This way, the number of slots will stabilize to the maximum
key usage in a zone and will not grow every time a key rollover is
triggered.
Add a test case that has more than four keys (the initial number of
key slots that are created for dnssec-sign statistics). We shouldn't
be expecting weird values.
This fixes some errors in the manykeys zone configuration (keys
were created for algorithm RSASHA256, but the policy expected RSASHA1,
and the zone was not allowing dynamic updates).
This also fixes an error in the calls to 'zones-json.pl': The perl
script excepts an index number where the zone can be found, rather
than the zone name.
We have introduced dnssec-sign statistics to the zone statistics. This
introduced an operational issue because when using zone-statistics
full, the memory usage was going through the roof. We fixed this by
by allocating just four key slots per zone. If a zone exceeds the
number of keys for example through a key rollover, the keys will be
rotated out on a FIFO basis.
This works for most cases, and fixes the immediate problem of high
memory usage, but if you sign your zone with many, many keys, or are
sign with a ZSK/KSK double algorithm strategy you may experience weird
statistics. A better strategy is to grow the number of key slots per
zone on key rollover events.
That is what this commit is doing: instead of rotating the four slots
to track sign statistics, named now grows the number of key slots
during a key rollover (or via some other method that introduces new
keys).
Add a new function to resize the number of counters in a statistics
counter structure. This will be needed when we keep track of DNSSEC
sign statistics and new keys are introduced due to a rollover.
Add a simple stats unit test that tests the existing library functions
isc_stats_ncounters, isc_stats_increment, isc_stats_decrement,
isc_stats_set, and isc_stats_update_if_greater.
After a reload, if the zone hasn't changed, this will log a
DNS_R_UNCHANGED error. This should not be at error level because it
happens on every reload.
Add a test case for migrating CSK to dnssec-policy. The keymgr has no
way of telling that the key is used as a CSK, but if there is only one
key to migrate it is going to assume it must be a CSK.
Previously, when dnssec-cds copied CDS records to make DS records,
its -a algorithm option did not have any effect. This means that if
the child zone is signed with older software that generates SHA-1 CDS
records, dnssec-cds would (by default) create SHA-1 DS records in
violation of RFC 8624.
This change makes the dnssec-cds -a option apply to CDS records as
well as CDNSKEY records. In the CDS case, the -a algorithms are the
acceptable subset of possible CDS algorithms. If none of the CDS
records are acceptable, dnssec-cds tries to generate DS records from
CDNSKEY records.
Instead of disabling the fragmentation on the UDP sockets, we now
disable the Path MTU Discovery by setting IP(V6)_MTU_DISCOVER socket
option to IP_PMTUDISC_OMIT on Linux and disabling IP(V6)_DONTFRAG socket
option on FreeBSD. This option sets DF=0 in the IP header and also
ignores the Path MTU Discovery.
As additional mitigation on Linux, we recommend setting
net.ipv4.ip_no_pmtu_disc to Mode 3:
Mode 3 is a hardend pmtu discover mode. The kernel will only accept
fragmentation-needed errors if the underlying protocol can verify
them besides a plain socket lookup. Current protocols for which pmtu
events will be honored are TCP, SCTP and DCCP as they verify
e.g. the sequence number or the association. This mode should not be
enabled globally but is only intended to secure e.g. name servers in
namespaces where TCP path mtu must still work but path MTU
information of other protocols should be discarded. If enabled
globally this mode could break other protocols.
The client->rcode_override was originally created to force the server
to send SERVFAIL in some cases when it would normally have sent FORMERR.
More recently, it was used in a3ba95116e
commit (part of GL #2790) to force the sending of a TC=1 NOERROR
response, triggering a retry via TCP, when a UDP packet could not be
sent due to ISC_R_MAXSIZE.
This ran afoul of a pre-existing INSIST in ns_client_error() when
RRL was in use. the INSIST was based on the assumption that
ns_client_error() could never result in a non-error rcode. as
that assumption is no longer valid, the INSIST has been removed.
The additional processing method has been expanded to take the
owner name of the record, as HTTPS and SVBC need it to process "."
in service form.
The additional section callback can now return the RRset that was
added. We use this when adding CNAMEs. Previously, the recursion
would stop if it detected that a record you added already exists. With
CNAMEs this rule doesn't work, as you ultimately care about the RRset
at the target of the CNAME and not the presence of the CNAME itself.
Returning the record allows the caller to restart with the target
name. As CNAMEs can form loops, loop protection was added.
As HTTPS and SVBC can produce infinite chains, we prevent this by
tracking recursion depth and stopping if we go too deep.
Add a test case for GL #2845 where a zone is in two views, one base
view and one "in-view" and that zone is using an $INCLUDE. Make sure
that there is a jnl file (have ixfr-from-differences enabled and do a
dynamic update). Then freeze and make updates in the included file
(this requires the test.db file also to be updated because 'rndc freeze'
causes the zone file to be overwritten). Finally reload and ensure that
the edit in the included file has been loaded.
string.endswith("label.sequence") doesn't check for the implict
period before "label.sequence" when matching longer strings.
"foo.label.sequence" should match but "foolabel.sequence shouldn't".
When looking up a zonecut in cache, we use 'dns_rbt_findnode' to find
the closest matching node. This function however does not take into
account stale nodes. When we do find a stale node and use it, this
has implications for subsequent lookups. For example, this may break
QNAME minimization because we are using a deeper zonecut than we should
have.
Check the header for staleness and if so, and stale entries are not
accepted, look for the deepest zonecut from this node up.
There are some occurrences where we check if a header exists in the
rbtdb. These cases require that the header is also not marked as
ancient (aka ready for cleanup). These cases involve finding certain
data in cache.
Add test cases for GL #2665: The QNAME minimization (if enabled) should
also occur on the second query, after the RRsets have expired from
cache. BIND will still have the entries in cache, but marked stale.
These stale entries should not prevent the resolver from minimizing
the QNAME. We query for the test domain a.b.stale. in all cases (QNAME
minimization off, strict mode, and relaxed mode) and expect it to
behave the same the second time we have a stale delegation structure in
cache.
The commit fixes the doh_recv_send() because occasionally it would
fail because it did not wait for all responses to be sent, making the
check for ssends value to nit pass.
This commit changes TLS stream behaviour in such a way, that it is now
optimised for small writes. In the case there is a need to write less
or equal to 512 bytes, we could avoid calling the memory allocator at
the expense of possibly slight increase in memory usage. In case of
larger writes, the behviour remains unchanged.
At least at this point doing memory copying is not required. Probably
it was a workaround for some problem in the earlier days of DoH, at
this point it appears to be a waste of CPU cycles.
This commit significantly simplifies the code in http_send_outgoing()
as it was unnecessary complicated, because it was dealing with
multiple statically and dynamically allocated buffers, making it
extremely hard to follow, as well as making it to do unnecessary
memory copying in some situations. This commit fixes these issues,
while retaining the high level buffering logic.
When an HTTP/2 client terminates a session it means that it is about
to close the underlying connection. However, we were not doing that.
As a result, with the latest changes to the test suite, which made it
to limit amount of requests per a transport connection, the tests
using quota would hang for quite a while. This commit fixes that.
This commit ensures that only a limited number of requests is going to
be sent over a single HTTP/2 connection. Before that change was
introduced, it was possible to complete all of the planned sends via
only one transport connection, which undermines the purpose of the
tests using the quota facility.
The function should not be called here because it is, in general,
supposed to be called at the end of the transport level callbacks to
perform I/O, and thus, calling it here is clearly a mistake because it
breaks other code expectations. As a result of the call to
http_do_bio() from within isc__nm_http_request() the unit tests were
running slower than expected in some situations.
In this particular situation http_do_bio() is going to be called at
the end of the transport_connect_cb() (initially), or http_readcb(),
sending all of the scheduled requests at once.
This change affects only the test suite because it is the only place
in the codebase where isc__nm_http_request() is used in order to
ensure that the server is able to handle multiple HTTP/2 streams at
once.
This commit fixes a crash in DoH caused by transport handle to be
detached too early when sending outgoing data.
We need to attach to the session->handle earlier because as an
indirect result of the nghttp2_session_mem_send() the session might
get closed and the handle detached. However, there is still might be
some outgoing data to handle. Besides, even when the underlying socket
was closed via the handle, we still should try to attempt to send
outgoing data via isc_nm_send() to let it call write callback, passed
to the http_send_outgoing().
This commit gets rid of custom code taking care of response buffering
by replacing the custom code with isc_buffer_t. Also, it gets rid of
an unnecessary memory copying when sending a response.
This commit replaces the ad-hoc 64K buffer for incoming POST data with
isc_buffer_t backed by dynamically allocated buffer sized accordingly
to the value in the "Content-Length" header.
The commit replaces an ad-hoc incoming DNS-message buffer in the
client-side DoH code with isc_buffer_t.
The commit also fixes a timing issue in the unit tests revealed by the
change.
This commit replaces a static ad-hoc HTTP/2 session's temporary buffer
with a realloc-able isc_buffer_t object, which is being allocated on
as needed basis, lowering the memory consumption somewhat. The buffer
is needed in very rare cases, so allocating it prematurely is not
wise.
Also, it fixes a bug in http_readcb() where the ad-hoc buffer appeared
to be improperly used, leading to a situation when the processed data
from the receiving regions can be processed twice, while unprocessed
data will never be processed.
When copying metadata from one dst_key to another, when the source
dst_key has a boolean metadata unset, the destination dst_key will
have a numeric metadata unset instead.
This means that if a key has KSK or ZSK unset, we may be clearing the
Predecessor or Successor metadata in the destination dst_key.
Add a test case to the dnssec system test to check that:
- a zone with a prepublished key is only signed with the active key.
- a zone with an inactive key but valid signatures retains those
signatures and does not add signatures from successor key.
- signatures are swapped in a zone when signatures of predecessor
inactive key are within the refresh interval.
When signing with a ZSK, check if it has a predecessor. If so, and if
the predecessor key is sane (same algorithm, key id matches predecessor
value, is zsk), check if the RRset is signed with this key. If so, skip
signing with this successor key. Otherwise, do sign with the successor
key.
This change means we also need to apply the interval to keys that are
not actively signing. In other words, 'expired' is always
'isc_serial_gt(now + cycle, rrsig.timeexpire)'.
Fix a print style issue ("removing signature by ..." was untabbed).
In the "Migrating from NSEC to NSEC3" section, it says:
dnssec-policy "standard" {
nsec3param iterations optout no salt-length 16;
};
There should be an integer after "iterations". Based on the following
text, the number of iterations should be 10.
This commit gets rid of RW locks in a hot path of the DoH code. In the
original design, it was implied that we add new endpoints after the
HTTP listener was created. Such a design implies some locking. We do
not need such flexibility, though. Instead, we could build a set of
endpoints before the HTTP listener gets created. Such a design does
not need RW locks at all.
This commit increases the idle TCP timeout to let the DoH quota system
test pass on some platforms (namely FreeBSD 11). It turned out to run
slow enough on the CI under load for the idle TCP timeout to kick in.
This commit refactors the DoH quota system test to make it more
reliable.
The test tries to establish dummy TCP connections to stress the quota
one by one instead of in bulk until the BIND instance cannot answer
queries anymore. This design is better because the test itself does
not need to be aware of the actual quota size.
respdiff needs to be run regularly to identify problems with query
responses discrepancies sooner than after tagging a release.
MAX_DISAGREEMENTS_PERCENTAGE variable is set to 0.5 on the main branch
to make room for a greater number of response disagreements between a
relatively old baseline version and the Development Version.
On the isc_mem water change the old water_t structure could be used
after free. Instead of introducing reference counting on the hot-path
we are going to introduce additional constraints on the
isc_mem_setwater. Once it's set for the first time, the additional
calls have to be made with the same water and water_arg arguments.
Increasing the nodelock count had major impact on the memory footprint
in scenarios where multiple rbtdb structure would be created like
hosting many zones in a single server.
This reverts commit 0344684385 and sets
the nodelock count to previously used values.
Since the forced removal of gcc:sid:i386 in 0aacabc6, we lacked a 32-bit
environment to build and test BIND 9 in the CI. gcc:buster:amd64cross32
adds an environment to cross-compile BIND 9 to 32-bits on Debian Buster
amd64 image with 32-bit BIND 9 dependencies. Commit also adds sanity
checks to ensure that compiled objects are not of the build platform
triplet type.
The support for stat.pl's --restart option was incomplete in run.sh.
This change makes sure it's handled properly and that named.run file is
not being removed by clean.sh when the --restart option is used.
When named failed to start and produced core dump, the core file wasn't
processed by GDB because of run.sh script exiting immediately. This
remedies the limitation, simplifies the surrounding code, and makes the
script shellcheck clean.
Anchor lets the user see the full command logged in GitLab CI:
${CONFIGURE} --disable-maintainer-mode --enable-developer ...
Instead of a folded multi-line when literal block is used:
${CONFIGURE} \ # collapsed multi-line command
Make DoH-quota separate and configurable, make it possible to limit the number of HTTP/2 streams per connection
See merge request isc-projects/bind9!5036
The system tests stress out the DoH quota by opening many TCP
connections and then running dig instances against the "overloaded"
server to perform some queries. The processes cannot make any
resolutions because the quota is exceeded. Then the opened connections
are getting closed in random order allowing the queries to proceed.
This commit makes number of concurrent HTTP/2 streams per connection
configurable as a mean to fight DDoS attacks. As soon as the limit is
reached, BIND terminates the whole session.
The commit adds a global configuration
option (http-streams-per-connection) which can be overridden in an
http <name> {...} statement like follows:
http local-http-server {
...
streams-per-connection 100;
...
};
For now the default value is 100, which should be enough (e.g. NGINX
uses 128, but it is a full-featured WEB-server). When using lower
numbers (e.g. ~70), it is possible to hit the limit with
e.g. flamethrower.
This commit adds support for http-listener-clients global options as
well as ability to override the default in an HTTP server description,
like:
http local-http-server {
...
listener-clients 100;
...
};
This way we have ability to specify per-listener active connections
quota globally and then override it when required. This is exactly
what AT&T requested us: they wanted a functionality to specify quota
globally and then override it for specific IPs. This change
functionality makes such a configuration possible.
It makes sense: for example, one could have different quotas for
internal and external clients. Or, for example, one could use BIND's
internal ability to serve encrypted DoH with some sane quota value for
internal clients, while having un-encrypted DoH listener without quota
to put BIND behind a load balancer doing TLS offloading for external
clients.
Moreover, the code no more shares the quota with TCP, which makes
little sense anyway (see tcp-clients option), because of the nature of
interaction of DoH clients: they tend to keep idle opened connections
for longer periods of time, preventing the TCP and TLS client from
being served. Thus, the need to have a separate, generally larger,
quota for them.
Also, the change makes any option within "http <name> { ... };"
statement optional, making it easier to override only required default
options.
By default, the DoH connections are limited to 300 per listener. I
hope that it is a good initial guesstimate.
This commit adds the code (and some tests) which allows verifying
validity of HTTP paths both in incoming HTTP requests and in BIND's
configuration file.
Extend the "chain" system test with AUTHORITY section checks for signed,
secure delegations. This complements the checks for signed, insecure
delegations added by commit 26ec4b9a89.
Extend the existing AUTHORITY section checks for signed, insecure
delegations to ensure nonexistence of DS RRsets in such responses.
Adjust comments accordingly.
Ensure dig failures cause the "chain" system test to fail.
It has been noticed that commit 7a87bf468b
did not only fix NSEC record handling in signed, insecure delegations
prepared using both wildcard expansion and CNAME chaining - it also
inadvertently fixed DS record handling in signed, secure delegations
of that flavor. This is because the 'rdataset' variable in the relevant
location in query_addds() can be either a DS RRset or an NSEC RRset.
Update a code comment in query_addds() to avoid confusion.
Update the comments describing the purpose of query_addds() so that they
also mention NSEC(3) records.
If we have a CDS or CDNSKEY we at least need to have a DNSKEY with the
same algorithm published and signing the CDS RRset. Same for CDNSKEY
of course.
This relaxes the zone_cdscheck function, because before the CDS or
CDNSKEY had to match a DNSKEY, now only the algorithm has to match.
This allows a provider in a multisigner model to update the CDS/CDNSKEY
RRset in the zone that is served by the other provider.
Add tests to the nsupdate system test to make sure that CDS and/or
CDNSKEY that match an algorithm in the DNSKEY RRset are allowed. Also
add tests that updates are rejected if the algorithm does not match.
Remove the now redundant test cases from the dnssec system test.
Update the checkzone system test: Change the algorithm of the CDS and
CDNSKEY records so that the zone is still rejected.
An unhandled code path left GET query string data uninitialised (equal
to NULL) and led to a crash during the requests' base64 data
decoding. This commit fixes that.
As we don't set the thread affinity, the cpu test would consistently
fail. Disable it, but don't remove it as we might restore setting the
affinity in the future versions of BIND 9.
It was discovered that setting the thread affinity on both the netmgr
and netthread threads lead to inconsistent recursive performance because
sometimes the netmgr and netthread threads would compete over single
resource and sometimes not.
Removing setting the affinity causes a slight dip in the authoritative
performance around 5% (the measured range was from 3.8% to 7.8%), but
the recursive performance is now consistently good.
On OpenBSD and more generally on platforms without either jemalloc or
malloc_(usable_)size, we need to increase the alignment for the memory
to sizeof(max_align_t) as with plain sizeof(void *), the compiled code
would be crashing when accessing the returned memory.
It was discovered that on some platforms (f.e. Alpine Linux with MUSL)
the result of isc_os_ncpus() call differ when called before and after we
drop privileges. This commit changes the isc_os_ncpus() call to cache
the result from the first call and thus always return the same value
during the runtime of the named. The first call to isc_os_ncpus() is
made as soon as possible on the library initalization.
The isc_mem_get(), isc_mem_allocate() and isc_mem_reallocate() can
return NULL ptr in case where the allocation size is NULL. Remove the
nonnull attribute from the functions' declarations.
This stems from the following definition in the C11 standard:
> If the size of the space requested is zero, the behavior is
> implementation-defined: either a null pointer is returned, or the
> behavior is as if the size were some nonzero value, except that the
> returned pointer shall not be used to access an object.
In this case, we return NULL as it's easier to detect errors when
accessing pointer from zero-sized allocation which should obviously
never happen.
In the rallocx() shim for OpenBSD (that's the only platform that doesn't
have malloc_size() or malloc_usable_size() equivalent), the newly
allocated size was missing the extra size_t member for storing the
allocation size leading to size_t sized overflow at the end of the
reallocated memory chunk.
2607
43. tainted_argument: Calling function journal_read_xhdr taints argument xhdr.size. [show details]
2608 result = journal_read_xhdr(j1, &xhdr);
44. Condition rewrite, taking true branch.
45. Condition result == 29, taking false branch.
2609 if (rewrite && result == ISC_R_NOMORE) {
2610 break;
2611 }
46. Condition result != 0, taking false branch.
2612 CHECK(result);
2613
47. var_assign_var: Assigning: size = xhdr.size. Both are now tainted.
2614 size = xhdr.size;
CID 331088 (#3 of 3): Untrusted allocation size (TAINTED_SCALAR)
48. tainted_data: Passing tainted expression size to isc__mem_get, which uses it as an allocation size. [show details]
Ensure that tainted values are properly sanitized, by checking that their values are within a permissible range.
2615 buf = isc_mem_get(mctx, size);
In the jemalloc merge request, we missed the fact that ah_frees and ah_handles
are reallocated which is not compatible with using isc_mem_get() for allocation
and isc_mem_put() for deallocation. This commit reverts that part and restores
use of isc_mem_allocate() and isc_mem_free().
The proper way how to disable the water limit in the isc_mem context is
to call:
isc_mem_setwater(ctx, NULL, NULL, 0, 0);
this ensures that the old water callback is called with ISC_MEM_LOWATER
if the callback was called with ISC_MEM_HIWATER before.
Historically, there were some places where the limits were disabled by
calling:
isc_mem_setwater(ctx, water, water_arg, 0, 0);
which would also call the old callback, but it also causes the water_t
to be allocated and extra check to be executed because water callback is
not NULL.
This commits unifies the calls to disable water to the preferred form.
The Address and Thread Sanitizers both intercept the malloc calls and
using the extended jemalloc API interferes with that. This commit
disables the use of jemalloc for both ASAN and TSAN enabled builds to
eliminate both false positives and false negatives.
Previously, the isc_mem_allocate() and isc_mem_free() would be used for
isc_mem_total test, but since we now use the real allocation
size (sallocx, malloc_size, malloc_usable_size) to track the allocation
size, it's impossible to get the test value right. Changing the test to
use isc_mem_get() and isc_mem_put() will use the exact size provided, so
the test would work again on all the platforms even when jemalloc is not
being used.
It was discovered that softhsm2.4 has a bug that causes invalid free()
call to be called when unloading libsofthsm.so.2 library. The native
PKCS#11 API is scheduled to removed in the 9.17+ release, we could
safely just disable jemalloc for this particular build.
This commit refactors the water mechanism in the isc_mem API to use
single pointer to a water_t structure that can be swapped with
atomic_exchange operation instead of having four different
values (water, water_arg, hi_water, lo_water) in the flat namespace.
This reduces the need for locking and prevents a race when water and
water_arg could be desynchronized.
Calls to jemalloc extended API with size == 0 ends up in undefined
behaviour. This commit makes the isc_mem_get() and friends calls
more POSIX aligned:
If size is 0, either a null pointer or a unique pointer that can be
successfully passed to free() shall be returned.
We picked the easier route (which have been already supported in the old
code) and return NULL on calls to the API where size == 0.
This commit adds support for systems where the jemalloc library is not
available as a package, here's the quick summary:
* On Linux - the jemalloc is usually available as a package, if
configured --without-jemalloc, the shim would be used around
malloc(), free(), realloc() and malloc_usable_size()
* On macOS - the jemalloc is available from homebrew or macports, if
configured --without-jemalloc, the shim would be used around
malloc(), free(), realloc() and malloc_size()
* On FreeBSD - the jemalloc is *the* system allocator, we just need
to check for <malloc_np.h> header to get access to non-standard API
* On NetBSD - the jemalloc is *the* system allocator, we just need to
check for <jemalloc/jemalloc.h> header to get access to non-standard
API
* On a system hostile to users and developers (read OpenBSD) - the
jemalloc API is emulated by using ((size_t *)ptr)[-1] field to hold
the size information. The OpenBSD developers care only for
themselves, so why should we care about speed on OpenBSD?
- isc_mempool_get() can no longer fail; when there are no more objects
in the pool, more are always allocated. checking for NULL return is
no longer necessary.
- the isc_mempool_setmaxalloc() and isc_mempool_getmaxalloc() functions
are no longer used and have been removed.
Current mempools are kind of hybrid structures - they serve two
purposes:
1. mempool with a lock is basically static sized allocator with
pre-allocated free items
2. mempool without a lock is a doubly-linked list of preallocated items
The first kind of usage could be easily replaced with jemalloc small
sized arena objects and thread-local caches.
The second usage not-so-much and we need to keep this (in
libdns:message.c) for performance reasons.
Previously, we only had capability to trace the mempool gets and puts,
but for debugging, it's sometimes also important to keep track how many
and where do the memory pools get created and destroyed. This commit
adds such tracking capability.
The isc_mem_allocate() comes with additional cost because of the memory
tracking. In this commit, we replace the usage with isc_mem_get()
because we track the allocated sizes anyway, so it's possible to also
replace isc_mem_free() with isc_mem_put().
The jemalloc non-standard API fits nicely with our memory contexts, so
just rewrite the memory context internals to use the non-public API.
There's just one caveat - since we no longer track the size of the
allocation for isc_mem_allocate/isc_mem_free combination, we need to use
sallocx() to get real allocation size in both allocator and deallocator
because otherwise the sizes would not match.
The ISC_MEM_DEBUGSIZE and ISC_MEM_DEBUGCTX did sanity checks on matching
size and memory context on the memory returned to the allocator. Those
will no longer needed when most of the allocator will be replaced with
jemalloc.
There's global variable called `malloc_conf` that can be used to
configure jemalloc behaviour at the program startup. We use following
configuration:
* xmalloc:true - abort-on-out-of-memory enabled.
* background_thread:true - Enable internal background worker threads
to handle purging asynchronously.
* metadata_thp:auto - allow jemalloc to use transparent huge page
(THP) for internal metadata initially, but may begin to do so when
metadata usage reaches certain level.
* dirty_decay_ms:30000 - Approximate time in milliseconds from the
creation of a set of unused dirty pages until an equivalent set of
unused dirty pages is purged and/or reused.
* muzzy_decay_ms:30000 - Approximate time in milliseconds from the
creation of a set of unused muzzy pages until an equivalent set of
unused muzzy pages is purged and/or reused.
More information about the specific meaning can be found in the jemalloc
manpage or online at http://jemalloc.net/jemalloc.3.html
The jemalloc allocator is scalable high performance allocator, this is
the first in the series of commits that will add jemalloc as a memory
allocator for BIND 9.
This commit adds configure.ac check and Makefile modifications to use
jemalloc as BIND 9 allocator.
Previously, we only had capability to trace the memory gets and puts,
but for debugging, it's sometimes also important to keep track how many
and where do the memory contexts get created and destroyed. This commit
adds such tracking capability.
This commit makes BIND return HTTP status codes for malformed or too
small requests.
DNS request processing code would ignore such requests. Such an
approach works well for other DNS transport but does not make much
sense for HTTP, not allowing it to complete the request/response
sequence.
Suppose execution has reached the point where DNS message handling
code has been called. In that case, it means that the HTTP request has
been successfully processed, and, thus, we are expected to respond to
it either with a message containing some DNS payload or at least to
return an error status code. This commit ensures that BIND behaves
this way.
This error code fits better than the more generic "Internal Server
Error" (500) which implies that the problem is on the server.
Also, do not end the whole HTTP/2 session on a bad request.
We were too strict regarding the value and presence of "Accept" HTTP
header, slightly breaking compatibility with the specification.
According to RFC8484 client SHOULD add "Accept" header to the requests
but MUST be able to handle "application/dns-message" media type
regardless of the value of the header. That basically suggests we
ignore its value.
Besides, verifying the value of the "Accept" header is a bit tricky
because it could contain multiple media types, thus requiring proper
parsing. That is doable but does not provide us with any benefits.
Among other things, not verifying the value also fixes compatibility
with clients, which could advertise multiple media types as supported,
which we should accept. For example, it is possible for a perfectly
valid request to contain "application/dns-message", "application/*",
and "*/*" in the "Accept" header value. Still, we would treat such a
request as invalid.
The commit fixes BIND hanging when browsers end HTTP/2 streams
prematurely (for example, by sending RST_STREAM). It ensures that
isc__nmsocket_prep_destroy() will be called for an HTTP/2 stream,
allowing it to be properly disposed.
The problem was impossible to reproduce using dig or DoH benchmarking
software (e.g. flamethrower) because these do not tend to end HTTP/2
streams prematurely.
This commit adds two new autoconf options `--enable-doh` (enabled by
default) and `--with-libnghttp2` (mandatory when DoH is enabled).
When DoH support is disabled the library is not linked-in and support
for http(s) protocol is disabled in the netmgr, named and dig.
if a control channel listener was configured with more than one
key algorithm, message verification would be attempted with each
algorithm in turn. if the first key failed due to the wrong
signature length, the entire verification process was aborted,
rather than continuing on to try with another key.
The isc/platform.h header was left empty which things either already
moved to config.h or to appropriate headers. This is just the final
cleanup commit.
The last remaining defines needed for platforms without NAME_MAX and
PATH_MAX (I'm looking at you, GNU Hurd) were moved to isc/dir.h where
it's prevalently used.
The ISC_STRERRORSIZE was defined in isc/platform.h header as the
value was different between Windows and POSIX platforms. Now that
Windows is gone, move the define to where it belongs.
The function 'private_type_record()' is now used in multiple system
setup scripts and should be moved to the common configuration script
conf.sh.common.
The old approach where each zone structure has its own mutex that
a thread needs to obtain multiple locks to do safe keyfile I/O
operations lead to a race condition ending in a possible deadlock.
Consider a zone in two views. Each such zone is stored in a separate
zone structure. A thread that needs to read or write the key files for
this zone needs to obtain both mutexes in seperate structures. If
another thread is working on the same zone in a different view, they
race to get the locks. It would be possible that thread1 grabs the
lock of the zone in view1, while thread2 wins the race for the lock
of the zone in view2. Now both threads try to get the other lock, both
of them are already locked.
Ideally, when a thread wants to do key file operations, it only needs
to lock a single mutex. This commit introduces a key management hash
table, stored in the zonemgr structure. Each time a zone is being
managed, an object is added to the hash table (and removed when the
zone is being released). This object is identified by the zone name
and contains a mutex that needs to be locked prior to reading or
writing key files.
(cherry-picked from commit ef4619366d49efd46f9fae5f75c4a67c246ba2e6)
Similar to notify, add code to send and keep track of checkds requests.
On every zone_rekey event, we will check the DS at parental agents
(but we will only actually query parental agents if theree is a DS
scheduled to be published/withdrawn).
On a zone_rekey event, we will first clear the ongoing checkds requests.
Reset the counter, to avoid continuing KSK rollover premature.
This has the risk that if zone_rekey events happen too soon after each
other, there are redundant DS queries to the parental agents. But
if TTLs and the configured durations in the dnssec-policy are sane (as
in not ridiculous short) the chance of this happening is low.
This code gathers DNSSEC keys from key files and from the DNSKEY RRset.
It is used for the 'rndc dnssec -status' command, but will also be
needed for "checkds". Turn it into a function.
Change the static function 'get_ksk_zsk' to a library function that
can be used to determine the role of a dst_key. Add checks if the
boolean parameters to store the role are not NULL. Rename to
'dst_key_role'.
Add a Pytest based system test for the 'checkds' feature. There is
one nameserver (ns9, because it should be started the latest) that
has configured several zones with dnssec-policy. The zones are set
in such a state that they are waiting for DS publication or DS
withdrawal.
Then several other name servers act as parent servers that either have
the DS for these published, or not. Also one server in the mix is
to test a badly configured parental-agent.
There are tests for DS publication, DS publication error handling,
DS withdrawal and DS withdrawal error handling.
The tests ensures that the zone is DNSSEC valid, and that the
DSPublish/DSRemoved key metadata is set (or not in case of the error
handling).
It does not test if the rollover continues, this is already tested in
the kasp system test (that uses 'rndc -dnssec checkds' to set the
DSPublish/DSRemoved key metadata).
Add checks for "parental-agents" configuration, checking for the option
being at wrong type of zone (only allowed for primaries and
secondaries), duplicate definitions, duplicate references, and
undefined parental clauses (the name referenced in the zone clause
does not have a matching "parental-agent" clause).
When performing the 'setnsec3param' task, zones that are not loaded will have
their task rescheduled. We should do this only if the zone load is still
pending, this prevents zones that failed to load get stuck in a busy wait and
causing a hang on shutdown.
Add a zone to the configuration file that uses NSEC3 with dnssec-policy
and fails to load. This will cause setnsec3param to go into a busy wait
and will cause a hang on shutdown.
The Makefile.tests was modifying global AM_CFLAGS and LDADD and could
accidentally pull /usr/include to be listed before the internal
libraries, which is known to cause problems if the headers from the
previous version of BIND 9 has been installed on the build machine.
We should drop the HISTORY file because it's confusing and the same
information is covered by the release notes for .0 releases (or at
least they should be).
Remove references to the HISTORY file, update the README to tell
people go look somewhere else.
This was written down in the outdated doc/dev/release documentation.
Since the rest of that file can go, add these steps to a separate file
and update it to current standards (e.g. use git commands).
The util/, doc/design/, and doc/dev/ directories included couple of
tools or documents there were completely outdated because they either
refered the the VCS we no longer use (cvs) or described processes that
have been redesigned and they are documented elsewhere.
When backporting the Don't Fragment UDP socket option, it was noticed
that the edns-udp-size probing uses 1432 as one of the values to be
probed and the documentation would be recommending 1400 as the safe
value. As the safe value can be from the 1400-1500 interval, the
documentation has been changed to match the probed value, so we do not
skip it.
- add an 'nsupdate -C' option to override resolv.conf file for nsupdate
- set resolv.conf to use two test servers, the first one of which will
return REFUSED for a query for 'example'.
when nsupdate sends an SOA query to a resolver, if it fails
with REFUSED, nsupdate will now try the next server rather than
aborting the update completely.
In DNS Flag Day 2020, we started setting the DF (Don't Fragment socket
option on the UDP sockets. It turned out, that this code was incomplete
leading to dropping the outgoing UDP packets.
This has been now remedied, so it is possible to disable the
fragmentation on the UDP sockets again as the sending error is now
handled by sending back an empty response with TC (truncated) bit set.
This reverts commit 66eefac78c.
When the fragmentation is disabled on UDP sockets, the uv_udp_send()
call can fail with UV_EMSGSIZE for messages larger than path MTU.
Previously, this error would end with just discarding the response. In
this commit, a proper handling of such case is added and on such error,
a new DNS response with truncated bit set is generated and sent to the
client.
This change allows us to disable the fragmentation on the UDP
sockets again.
Add three more test cases that detect a configuration error if the
key-directory is inherited but has the same value for a zone in a
different view with a deviating DNSSEC policy.
This commit adds a unittest that tests private rdataset_getownercase()
and rdataset_setownercase() methods from rbtdb.c. The test setups
minimal mock dns_rbtdb_t and dns_rbtdbnode_t data structures.
As the rbtdb methods are generally hidden behind layers and layers, we
include the "rbtdb.c" directly from rbtdb_test.c, and thus we can use
the private methods and data structures directly. This also opens up
opportunity to add more unittest for the rbtdb private functions without
going through all the layers.
This check intermittently failed:
I:serve-stale:check not in cache longttl.example times out...
I:serve-stale:failed
This corresponds to this query in the test:
$DIG -p ${PORT} +tries=1 +timeout=3 @10.53.0.3 longttl.example TXT
Looking at the dig output for a failed test, the query actually got a
response from the authoritative server (in one specific example the
query time was 2991 msec, close to 3 seconds).
After doing the query for the test, we enable the authoritative
server after a sleep of three seconds. If we bump this sleep to 4
seconds, the race will be more in favor of the query timing out,
making it unlikely that this test will fail intermittently.
Bump the subsequent wait_for_log checks also with one second.
In the code that rdataset_setownercase() and rdataset_getownercase() we
now use tolower()/toupper()/isupper() functions appropriately instead of
rolling our own code.
Previously, we would set the locale on a global level and that could
possibly lead to different behaviour in underlying functions. In this
commit, we change to code to use the system locale only when calling the
libidn2 functions and reset the locale back to "POSIX" when exiting the
libidn2 code.
Expand the description of mirror zones in the ARM by adding a brief
discussion of how the validation process works for AXFR and IXFR. Move
the paragraph mentioning the "file" option higher up. Apply minor
stylistic and whitespace-related tweaks to the relevant section of the
ARM.
Apply minor stylistical and whitespace-related tweaks to the
descriptions of the "tcp-receive-buffer", "udp-receive-buffer",
"tcp-send-buffer", and "udp-send-buffer" options in the ARM.
The ARM contains typos in the names of the following two options:
- "tcp-receive-buffer"
- "udp-receive-buffer"
Fix the ARM so that it contains proper option names.
Improve the description of the "max-cache-size" option in the ARM by
focusing on its meaning for multiple views and default values.
Add mention of a hash table preallocation.
This commit adds a set of tests to verify that BIND will not crash
when some opcodes are sent over DoT or DoH, leading to marking network
handle in question as sequential.
Previously, each protocol (TCPDNS, TLSDNS) has specified own function to
disable pipelining on the connection. An oversight would lead to
assertion failure when opcode is not query over non-TCPDNS protocol
because the isc_nm_tcpdns_sequential() function would be called over
non-TCPDNS socket. This commit removes the per-protocol functions and
refactors the code to have and use common isc_nm_sequential() function
that would either disable the pipelining on the socket or would handle
the request in per specific manner. Currently it ignores the call for
HTTP sockets and causes assertion failure for protocols where it doesn't
make sense to call the function at all.
The built-in "_bind" view does not allow recursion and therefore does
not need a large cache database. However, as "max-cache-size" is not
explicitly set for that view in the default configuration, it inherits
that setting from global options. Set "max-cache-size" for the built-in
"_bind" view to a fixed value (2 MB, i.e. the smallest allowed value) to
prevent needlessly preallocating memory for its cache RBT hash table.
Currently the implicit default for the "max-cache-size" option is "90%".
As this option is inherited by all configured views, using multiple
views can lead to memory exhaustion over time due to overcommitment.
The "max-cache-size 90%;" default also causes cache RBT hash tables to
be preallocated for every configured view, which does not really make
sense for views which do not allow recursion.
To limit this problem's potential for causing operational issues, use a
minimal-sized cache for views which do not allow recursion and do not
have "max-cache-size" explicitly set (either in global configuration or
in view configuration).
For configurations which include multiple views allowing recursion,
adjusting "max-cache-size" appropriately is still left to the operator.
When locking key files for a zone, we iterate over all the views and
lock a mutex inside the zone structure. However, if we envounter an
in-view zone, we will try to lock the key files twice, one time for
the home view and one time for the in-view view. This will lead to
a deadlock because one thread is trying to get the same lock twice.
When "max-cache-size" is changed to "unlimited" (or "0") for a running
named instance (using "rndc reconfig"), the hash table size limit for
each affected cache DB is not reset to the maximum possible value,
preventing those hash tables from being allowed to grow as a result of
new nodes being added.
Extend dns_rbt_adjusthashsize() to interpret "size" set to 0 as a signal
to remove any previously imposed limits on the hash table size. Adjust
API documentation for dns_db_adjusthashsize() accordingly. Move the
call to dns_db_adjusthashsize() from dns_cache_setcachesize() so that it
also happens when "size" is set to 0.
Upon creation, each dns_rbt_t structure has its "maxhashbits" field
initialized to the value of the RBT_HASH_MAX_BITS preprocessor macro,
i.e. 32. When the dns_rbt_adjusthashsize() function is called for the
first time for a given RBT (for cache RBTs, this happens when they are
first created, i.e. upon named startup), it lowers the value of the
"maxhashbits" field to the number of bits required to index the
requested number of hash table slots. When a larger hash table size is
subsequently requested, the value of the "maxhashbits" field should be
increased accordingly, up to RBT_HASH_MAX_BITS. However, the loop in
the rehash_bits() function currently ensures that the number of bits
necessary to index the resized hash table will not be larger than
rbt->maxhashbits instead of RBT_HASH_MAX_BITS, preventing the hash table
from being grown once the "maxhashbits" field of a given dns_rbt_t
structure is set to any value lower than RBT_HASH_MAX_BITS.
Fix by tweaking the loop guard condition in the rehash_bits() function
so that it compares the new number of bits used for indexing the hash
table against RBT_HASH_MAX_BITS rather than rbt->maxhashbits.
The timeout originally picked for "rndc status" invocations (2 seconds)
in the test attempting to reproduce a deadlock caused by running
multiple "rndc addzone", "rndc modzone", and "rndc delzone" commands
concurrently causes intermittent failures of the "addzone" system test
in GitLab CI. Increase the timeout to 10 seconds to make such failures
less probable. Adjust code comments accordingly.
The requirements for BIND 9.17+ now requires C11 support from the
compiler, so we can safely drop most of the stdatomic.h shims from
lib/isc/unix/include/stdatomic.h.
This commit removes support for clang atomic builtins (clang >= 3.6.0
includes stdatomic.h header) and for Gcc __sync builtins.
The only compatibility shim that remains is support for __atomic
builtins for Gcc >= 4.7.0 since CentOS 7 still includes only Gcc 4.8.1
and the proper stdatomic.h header was only introduced in Gcc >= 4.9.
The warning was produced by an ASAN build:
runtime error: null pointer passed as argument 2, which is declared to
never be null
This commit fixes it by checking if nghttp2_session_mem_send() has
actually returned anything.
Resolve "ThreadSanitizer: data race lib/isc/task.c:435 in task_send (unprotected access to `task->threadid`)"
Closes#2739
See merge request isc-projects/bind9!5149
This change sets the mentioned fields properly and gets rid of klusges
added in the times when we were keeping pointers to isc_sockaddr_t
instead of copies. Among other things it helps to avoid a situation
when garbage instead of an address appears in dig output.
We cannot use DoH for zone transfers. According to RFC8484 a DoH
request contains exactly one DNS message (see Section 6: Definition of
the "application/dns-message" Media Type,
https://datatracker.ietf.org/doc/html/rfc8484#section-6). This makes
DoH unsuitable for zone transfers as often (and usually!) these need
more than one DNS message, especially for larger zones.
As zone transfers over DoH are not (yet) standardised, nor discussed
in RFC8484, the best thing we can do is to return "not implemented."
Technically DoH can be used to transfer small zones which fit in one
message, but that is not enough for the generic case.
Also, this commit makes the server-side DoH code ensure that no
multiple responses could be attempted to be sent over one HTTP/2
stream. In HTTP/2 one stream is mapped to one request/response
transaction. Now the write callback will be called with failure error
code in such a case.
Support a situation in header processing callback when client side
code could receive a belated response or part of it. That could
happen when the HTTP/2 session was already closed, but there were some
response data from server in flight. Other client-side nghttp2
callbacks code already handled this case.
The bug became apparent after HTTP/2 write buffering was supported,
leading to rare unit test failures.
This commit ensures that sock->h2.connect.cstream gets nullified when
the object in question is deleted. This fixes a nasty crash in dig
exposed when receiving large responses leading to double free()ing.
Also, it refactors how the client-side code keeps track of client
streams (hopefully) preventing from similar errors appearing in the
future.
This commit makes NM code to report HTTP as a stream protocol. This
makes it possible to handle large responses properly. Like:
dig +https @127.0.0.1 A cmts1-dhcp.longlines.com
When answering a query requires wildcard expansion, the AUTHORITY
section of the response needs to include NSEC(3) record(s) proving that
the QNAME does not exist.
When a response to a query is an insecure delegation, the AUTHORITY
section needs to include an NSEC(3) proof that no DS record exists at
the parent side of the zone cut.
These two conditions combined trip up the NSEC part of the logic
contained in query_addds(), which expects the NS RRset to be owned by
the first name found in the AUTHORITY section of a delegation response.
This may not always be true, for example if wildcard expansion causes an
NSEC record proving QNAME nonexistence to be added to the AUTHORITY
section before the delegation is added to the response. In such a case,
named incorrectly omits the NSEC record proving nonexistence of QNAME
from the AUTHORITY section.
The same block of code is affected by another flaw: if the same NSEC
record proves nonexistence of both the QNAME and the DS record at the
parent side of the zone cut, this NSEC record will be added to the
AUTHORITY section twice.
Fix by looking for the NS RRset in the entire AUTHORITY section and
adding the NSEC record to the delegation using query_addrrset() (which
handles duplicate RRset detection).
Add a set of system tests which check the contents of the AUTHORITY
section for signed, insecure delegation responses constructed from CNAME
records and wildcards, both for zones using NSEC and NSEC3.
Instead of checking the value of the variable modified two lines earlier
(the number of SOA records present at the apex of the old version of the
zone), one of the RUNTIME_CHECK() assertions in zone_postload() checks
the number of SOA records present at the apex of the new version of the
zone, which is already checked before. Fix the assertion by making it
check the correct variable.
The Windows support has been completely removed from the source tree
and BIND 9 now no longer supports native compilation on Windows.
We might consider reviewing mingw-w64 port if contributed by external
party, but no development efforts will be put into making BIND 9 compile
and run on Windows again.
When named restarts, it will examine signed zones and checks if the
current denial of existence strategy matches the dnssec-policy. If not,
it will schedule to create a new NSEC(3) chain.
However, on startup the zone database may not be read yet, fooling
BIND that the denial of existence chain needs to be created. This
results in a replacement of the previous NSEC(3) chain.
Change the code such that if the NSEC3PARAM lookup failed (the result
did not return in ISC_R_SUCCESS or ISC_R_NOTFOUND), we will try
again later. The nsec3param structure has additional variables to
signal if the lookup is postponed. We also need to save the signal
if an explicit resalt was requested.
In addition to the two added boolean variables, we add a variable to
store the NSEC3PARAM rdata. This may have a yet to be determined salt
value. We can't create the private data yet because there may be a
mismatch in salt length and the NULL salt value.
Add a test case where 'named' is restarted and ensure that an already
signed zone does not change its NSEC3 parameters.
The test case first tests the current zone and saves the used salt
value. Then after restart it checks if the salt (and other parameters)
are the same as before the restart.
This test case changes 'set_nsec3param'. This will now reset the salt
value, and when checking for NSEC3PARAM we will store the salt and
use it when testing the NXDOMAIN response. This does mean that for
every test case we now have to call 'set_nsec3param' explicitly (and
can not omit it because it is the same as the previous zone).
Finally, slightly changed some echo output to make debugging friendlier.
When we rewrote the zone dumping to use the separate threadpool, the
dumping would acquire the read lock for the whole time the zone dumping
process is dumping the zone.
When combined with incoming IXFR that tries to acquire the write lock on
the same rwlock, we would end up blocking all the other readers.
In this commit, we pause the dbiterator every time we get next record
and before start dumping it to the disk.
Make sure an incoming IXFR containing an SOA record which is not placed
at the apex of the transferred zone does not result in a broken version
of the zone being served by named and/or a subsequent crash.
While cleaning up the usage of HAVE_UV_<func> macros, we forgot to
cleanup the HAVE_UV_UDP_CONNECT in the actual code and
HAVE_UV_TRANSLATE_SYS_ERROR and this was causing Windows build to fail
on uv_udp_send() because the socket was already connected and we were
falsely assuming that it was not.
The platforms with autoconf support were not affected, because we were
still checking for the functions from the configure.
This commit adds the ability to consolidate HTTP/2 write requests if
there is already one in flight. If it is the case, the code will
consolidate multiple subsequent write request into a larger one
allowing to utilise the network in a more efficient way by creating
larger TCP packets as well as by reducing TLS records overhead (by
creating large TLS records instead of multiple small ones).
This optimisation is especially efficient for clients, creating many
concurrent HTTP/2 streams over a transport connection at once. This
way, the code might create a small amount of multi-kilobyte requests
instead of many 50-120 byte ones.
In fact, it turned out to work so well that I had to add a work-around
to the code to ensure compatibility with the flamethrower, which, at
the time of writing, does not support TLS records larger than two
kilobytes. Now the code tries to flush the write buffer after 1.5
kilobyte, which is still pretty adequate for our use case.
Essentially, this commit implements a recommendation given by nghttp2
library:
https://nghttp2.org/documentation/nghttp2_session_mem_send.html
Add a call to posix_fadvise() to indicate to the kernel, that `named`
won't be needing the dumped zone files any time soon with:
* POSIX_FADV_DONTNEED - The specified data will not be accessed in the
near future.
Notes:
POSIX_FADV_DONTNEED attempts to free cached pages associated with the
specified region. This is useful, for example, while streaming large
files. A program may periodically request the kernel to free cached
data that has already been used, so that more useful cached pages are
not discarded instead.
Previously, dumping the zones to the files were quantized, so it doesn't
slow down network IO processing. With the introduction of network
manager asynchronous threadpools, we can move the IO intensive work to
use that API and we don't have to quantize the work anymore as it the
file IO won't block anything except other zone dumping processes.
The libuv has a support for running long running tasks in the dedicated
threadpools, so it doesn't affect networking IO.
This commit adds isc_nm_work_enqueue() wrapper that would wraps around
the libuv API and runs it on top of associated worker loop.
The only limitation is that the function must be called from inside
network manager thread, so the call to the function should be wrapped
inside a (bound) task.
Instead of having a configure check for every missing function that has
been added in later version of libuv, we now use UV_VERSION_HEX to
decide whether we need the shim or not.
The uv_req_get_data() and uv_req_set_data() functions were introduced in
libuv >= 1.19.0, so we need to add compatibility shims with older libuv
versions.
Commit bdb777b2a2 updated the man pages
to contents produced using:
- Sphinx 4.0.2
- sphinx-rtd-theme 0.5.2
- docutils 0.17.1
However, sphinx-rtd-theme 0.5.2 is incompatible with versions 0.17+ of
the docutils package. This problem was addressed in the Docker image
used for building man pages by downgrading the docutils package to
version 0.16.
Regenerate the man pages again, this time using:
- Sphinx 4.0.2
- sphinx-rtd-theme 0.5.2
- docutils 0.16
This is necessary to prevent the "docs" GitLab CI job from failing.
Rather than having an expensive 'expired' (fka 'stale_ttl') in the
rdataset structure, that is only used to be printed in a comment on
ancient RRsets, reuse the TTL field of the RRset.
Commit a83c8cb0af updated masterdump so
that stale records in "rndc dumpdb" output no longer shows 0 TTLs. In
this commit we change the name of the `rdataset->stale_ttl` field to
`rdataset->expired` to make its purpose clearer, and set it to zero in
cases where it's unused.
Add 'rbtdb->serve_stale_ttl' to various checks so that stale records
are not purged from the cache when they've been stale for RBTDB_VIRTUAL
(300) seconds.
Increment 'ns_statscounter_usedstale' when a stale answer is used.
Note: There was a question of whether 'overmem_purge' should be
purging ancient records, instead of stale ones. It is left as purging
stale records, since stale records could take up the majority of the
cache.
This submission is copyrighted Akamai Technologies, Inc. and provided
under an MPL 2.0 license.
This commit was originally authored by Kevin Chen, and was updated by
Matthijs Mekking to match recent serve-stale developments.
Once we resume a query, we should clear DNS_FETCHOPT_TRYSTALE_ONTIMEOUT
from the options to prevent triggering the stale-answer-client-timeout
on subsequent fetches.
If we don't this may cause a crash when for example when prefetch is
triggered after a query restart.
Add a test case where a client request is received and the stale
timeout occurs, but it is not served stale data because there is no entry
in the cache, then is served an authoritative answer once the background
fetch completes. This ensures that a stale timeout only affects a
subsequent response if the client was answered.
when a serve-stale answer has been sent, the client continues waiting
for a proper answer. if a final completion event for the client does
arrive, it can just be cleaned up without sending a response, similar
to a canceled fetch.
- send a query for an AAAA which will be resolved as a mapped A
- disable authoritative responses
- wait for the negative AAAA response to become stale
- send another query, wait for the stale answer
- re-enable authorative responses so that a real answer arrives
- currently, this triggers an assertion in query.c
On some platforms, the __attribute__ constructor and destructor won't
take priorities and the compilation failed. On such platform would be
macOS. For this reason, the constructor/destructor in the libisc was
reworked to not use priorities, but have a single constructor and
destructor that calls the appropriate routines in correct order.
This commit removes the extra priority because it's now not needed and
it also breaks a compilation on macOS with GCC 10.
In the shutdown system test multiple queries are sent to a resolver
instance, in the meantime we terminate the same resolver process for
which the queries were sent to, either via rndc stop or a SIGTERM
signal, that means the resolver may not be able to answer all those
queries, since it has initiated the shutdown process.
The dnspython library raises a dns.resolver.NoNameservers exception when
a resolver object fails to receive an answer from the specified list
of nameservers (resolver.nameservers list), we need to handle this
exception as this is something that may happen since we asked the
resolver to terminate, as a result it may not answer clients even if
an answer is available, as the operation will be canceled.
configuring with --enable-mutex-atomics flagged these incorrectly
initialised variables on systems where pthread_mutex_init doesn't
just zero out the structure.
The size of the array holding the pointers to clientmgr was created so
big it could hold the actual clientmgr objects, not just the pointer.
This commit fixes the size to be just the ncpus * sizeof(pointer).
The isc_nmiface_t type was holding just a single isc_sockaddr_t,
so we got rid of the datatype and use plain isc_sockaddr_t in place
where isc_nmiface_t was used before. This means less type-casting and
shorter path to access isc_sockaddr_t members.
At the same time, instead of keeping the reference to the isc_sockaddr_t
that was passed to us when we start listening, we will keep a local
copy. This prevents the data race on destruction of the ns_interface_t
objects where pending nmsockets could reference the sockaddr of already
destroyed ns_interface_t object.
* dns_journal_next() leaves the read point in the journal after the
transaction header so journal_seek() should be inside the loop.
* we need to recover from transaction header inconsistencies
Additionally when correcting for <size, serial0, serial1, 0> the
correct consistency check is isc_serial_gt() rather than
isc_serial_ge(). All instances updated.
BIND installation should be done by setting DESTDIR during "make
install" not by setting prefix via ./configure.
Make sure that installation with DESTDIR=<PATH> works by checking that
named binary and it's respective man page were installed and that
well-known BIND9 directories - and only them - are present in DESTDIR.
Also rename install path variable from BIND_INSTALL_PATH to
INSTALL_PATH to avoid namespace clash in stress tests which use
BIND_INSTALL_PATH variable to configure path to BIND9 binaries.
Ubuntu 16.04 (Xenial Xerus) is reaching End of Standard Support in April
2021 thus we are removing it from the list of supported platforms and
replacing it with Ubuntu 18.04 LTS (Bionic Beaver).
According to the measurements (recorded on GL!5085), the fillcount of 2
for namepool and fillcount of 4 for rdspool can fit 99.99% of request
for tested scenarios.
This was discovered by perf recording the single second recursive test
using flamethrower where the initial malloc lit up like a flare.
Previously, as a way of reducing the contention between threads a
clientmgr object would be created for each interface/IP address.
We tasks being more strictly bound to netmgr workers, this is no longer
needed and we can just create clientmgr object per worker queue (ncpus).
Each clientmgr object than would have a single task and single memory
context.
Similarly, the resolver code would create hundreds of memory contexts
just on the resolver setup. The contention will be reduced directly in
the allocator, so for now just attach to the view memory instead of
creating separate memory context for each bucket.
Since a client object is bound to a netmgr handle, each client
will always be processed by the same netmgr worker, so we can
simplify the code by binding client->task to the same thread as
the client. Since ns__client_request() now runs in the same event
loop as client->task events, is no longer necessary to pause the
task manager before launching them.
Also removed some functions in isc_task that were not used.
The number of memory contexts created in the clientmgr was enormous. It
could easily create thousands of memory contexts because the formula was:
nprotocols * ncpus * ninterfaces * CLIENT_NMCTXS_PERCPU (8)
The original goal was to reduce the contention when allocating the
memory, but after a while nobody noticed that the amount of memory
context allocated would not reduce contention at all.
This commit removes the whole mctxpool and just uses the mctx from
clientmgr as the contention will be reduced directly in the allocator.
Running gcc:tarball CI job for merge requests is consistent with how we
run gcc:out-of-tree CI job and should help identify problems with the
build system during the review process, not once merged during daily
runs. For the sake of time, unit and system tests associated with the
gcc:tarball CI job are excluded from merge requests.
It's a common pattern to spawn CI jobs only for pipelines triggered by
schedules, tags, and web. There should be an anchor so that the rules
are not repeated.
the idle timeout for rndc connections was set to 10 seconds, but this
caused intermittent system failures of the 'rndc' system test on slow
platforms, since 'rndc reconfig' could time out before reconfiguration
was complete.
this commit restores the original timeout value of 60 seconds, which was
changed inadvertently after rndc was updated to use the network manager.
even with this change, however, the test can still time out under
TSAN because loading the huge zone can take a very long time (upwards
of two minutes). so the test is modified here to generate a smaller zone
file when running under TSAN.
dns_name_copy() has been replaced nearly everywhere with
dns_name_copynf(). this commit changes the last two uses of
the original function. afterward, we can remove the old
dns_name_copy() implementation, and replace it with _copynf().
dns_message_gettempname() returns an initialized name with a dedicated
buffer, associated with a dns_fixedname object. Using dns_name_copynf()
to write a name into this object will actually copy the name data
from a source name. dns_name_clone() merely points target->ndata to
source->ndata, so it is faster, but it can lead to a use-after-free if
the source is freed before the target object is released via
dns_message_puttempname().
In a few places, clone was being used where copynf should have been;
this is now fixed.
As a side note, no memory was lost, because the ndata buffer used in
the dns_fixedname_t is internal to the structure, and is freed when
the dns_fixedname_t is freed regardless of the .ndata contents.
When executed in "legacy mode" (i.e. without the '-r' parameter)
run.sh invokes make with a modified environment.
SYSTEMTEST_FORCE_COLOR is now preserved for use by the individual test
scripts.
CYGWIN is now preserved for named, as it controls behavior relating to
crash reporting.
This restores legacy behavior in bin/tests/system where running:
SYSTEMTEST_NO_CLEAN=1 ./run.sh <testname>
would run the test and preserve the output files.
This has been broken since the change that has run.sh invoke "make",
due to SYSTEMTEST_NO_CLEAN not being preserved in the environment
that's set up for "make".
Another option would be to completely remove SYSTEMTEST_NO_CLEAN.
This seems to be the only behavior-changing environment variable
not accounted for in the call to "make".
I don't think this needs a CHANGES entry.
The default value of the "man_make_section_directory" Sphinx option was
changed in Sphinx 4.0.1, which broke building man pages in maintainer
mode as the shell code in doc/man/Makefile.am expects man pages to be
built in doc/man/_build/man/, not doc/man/_build/man/<section_number>/.
The aforementioned change in defaults was reverted in Sphinx 4.0.2, but
this issue should still be prevented from reoccurring in the future.
Ensure that by explicitly setting the "man_make_section_directory"
option to False.
The man pages produced by Sphinx 4.0.2 are slightly different than those
produced by Sphinx 3.5.4. As Sphinx 4.0.2 is now used in GitLab CI,
update all doc/man/*in files so that they reflect what that version of
Sphinx produces, in order to prevent GitLab CI job failures.
The last rdataset_getownercase() left it in a state where the code was
mix of microoptimizations (manual loop unrolling, complicated bitshifts)
with a code that would always rewrite the character even if it stayed
the same after transformation.
This commit makes sure that we modify only the characters that actually
need to change, removes the manual loop unrolling, and replaces the
weird bit arithmetics with a simple shift and bit-and.
dns_message_gettempname() now returns a pointer to an initialized
name associated with a dns_fixedname_t object. it is no longer
necessary to allocate a buffer for temporary names associated with
the message object.
Also, add "set -e" to all shell scripts of the views test to exit when
any command fails or is unknown, e.g., this on OpenBSD:
tests.sh[174]: seq: not found
The seq command is not defined in the POSIX standard and is missing on
OpenBSD. Given that the system test code is meant to be POSIX-compliant
replace it with a shell construct.
This function has never been used since it was added to the source tree
by commit 686b27bfd3 back in 1999. As
the dns_zoneflg_t type is only defined in lib/dns/zone.c, no function
external to that file would be able to use dns_zone_setflag() properly
anyway - the DNS_ZONE_SETFLAG() and DNS_ZONE_CLRFLAG() macros should be
used instead. Zone options that can be set from outside zone.c are set
using dns_zone_setoption().
Add two tests to make sure named-checkconf catches key-directory issues
where a zone in multiple views uses the same directory but has
different dnssec-policies. One test sets the key-directory specifically,
the other inherits the default key-directory (NULL, aka the working
directory).
Also update the good.conf test to allow zones in different views
with the same key-directory if they use the same dnssec-policy.
Also allow zones in different views with different key-directories if
they use different dnssec-policies.
Also allow zones in different views with the same key-directories if
only one view uses a dnssec-policy (the other is set to "none").
Also allow zones in different views with the same key-directories if
no views uses a dnssec-policy (zone in both views has the dnssec-policy
set to "none").
Don't allow the same zone with different dnssec-policies in separate
views have the same key-directory.
Track zones plus key-directory in a symtab and if there is a match,
check the offending zone's dnssec-policy name. If the name is "none"
(there is no kasp for the offending zone), or if the name is the same
(the zone shares keys), it is fine, otherwise it is an error (zones
in views using different policies cannot share the same key-directory).
Resolve "Misleading diagnostic in update_soa_serial indicates BIND will use increment but it doesn't"
Closes#2696
See merge request isc-projects/bind9!5029
if dns_updatemethod_date is used do that the returned method is only
set to dns_updatemethod_increment if the new serial does not encode
the current day (YYYYMMDDXX).
PyLint 2.8.2 reports the following suggestions for two Python scripts
used in the system test suite:
************* Module tests_rndc_deadlock
bin/tests/system/addzone/tests_rndc_deadlock.py:71:4: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
************* Module tests-shutdown
bin/tests/system/shutdown/tests-shutdown.py:68:4: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
bin/tests/system/shutdown/tests-shutdown.py:154:8: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
Implement the above suggestions by using
concurrent.futures.ThreadPoolExecutor() and subprocess.Popen() as
context managers.
Add an item to the CVE issue template which calls for drafting the
security advisory early in the security incident handling process. The
intention is to ensure there is enough time to review and polish ISC
security advisories before they get published.
Tweak the release checklist to make sure we carefully consider all
confidential issues before opening them up to the public. This change
is intended as a safeguard against accidentally disclosing too much
information about a security vulnerability before our users get a chance
to patch it.
Instead of using fixed quantum, this commit adds atomic counter for
number of items on each queue and uses the number of netievents
scheduled to run as the limit of maximum number of netievents for a
single process_queue() run.
This prevents the endless loops when the netievent would schedule more
netievents onto the same loop, but we don't have to pick "magic" number
for the quantum.
OpenBSD changed the name of the pytest script from py.test-3 in OpenBSD
6.8 to py.test in OpenBSD 6.9.
The py.test-3 name which was added in d5562a3e for the sake of OpenBSD
and CentOS is still required for CentOS.
This commit adds a new configuration option to set the receive and send
buffer sizes on the TCP and UDP netmgr sockets. The default is `0`
which doesn't set any value and just uses the value set by the operating
system.
There's no magic value here - set it too small and the performance will
drop, set it too large, the buffers can fill-up with queries that have
already timeouted on the client side and nobody is interested for the
answer and this would just make the server clog up even more by making
it produce useless work.
The `netstat -su` can be used on POSIX systems to monitor the receive
and send buffer errors.
Unit test run for out-of-tree builds used to fail to find
masterXX.data.in files:
/usr/bin/perl -w /builds/mnowak/bind9/lib/dns/tests/mkraw.pl < testdata/master/master12.data.in > testdata/master/master12.data
/bin/bash: testdata/master/master12.data.in: No such file or directory
make[4]: *** [Makefile:1910: testdata/master/master12.data] Error 1
The outgoing UDP socket selection would pick unintialized children
socket on Windows, because we have more netmgr workers than we have
listening sockets. This commit fixes the selection by keeping the
outgoing socket the same, so it's always run on existing socket.
The initial intent was to limit the number of concurrent streams by
the value of 100 but due to the error when reading the documentation
it was set to the maximum possible number of streams per session.
This could lead to security issues, e.g. a remote attacker could have
taken down the BIND instance by creating lots of sessions via low
number of transport connections. This commit fixes that.
We should not call nghttp2_session_terminate_session() in server-side
code after all of the active HTTP/2 streams are processed. The
underlying transport connection is expected to remain opened at least
for some time in this case for new HTTP/2 requests to arrive. That is
what flamethrower was expecting and it makes perfect sense from the
HTTP/2 perspective.
During the stress testing, it was discovered that the default netmgr
quantum of 128 is not enough and there was a performance drop for TCP on
FreeBSD. Bumping the default quantum to 1024 solves the performance
issue and is still enough to prevent the endless loops.
all privileged tasks are complete by the time we return from
isc_task_endexclusive(), so it makes sense to reset the taskmgr
mode to non-privileged right then.
We were clearing the pointer to taskmgr as soon as isc_taskmgr_destroy()
would be called and before all tasks were finished. Unfortunately, some
tasks would use global named_g_taskmgr objects from inside the events
and this would cause either a data race or NULL pointer dereference.
This commit fixes the data race by moving the destruction of the
referenced pointer to the time after all tasks are finished.
Network manager events that require interlock (pause, resume, listen)
are now always executed in the same worker thread, mgr->workers[0],
to prevent races.
"stoplistening" events no longer require interlock.
- ensure isc_nm_pause() and isc_nm_resume() work the same whether
run from inside or outside of the netmgr.
- promote 'stop' events to the priority event level so they can
run while the netmgr is pausing or paused.
- when pausing, drain the priority queue before acquiring an
interlock; this prevents a deadlock when another thread is waiting
for us to complete a task.
- release interlock after pausing, reacquire it when resuming, so
that stop events can happen.
some incidental changes:
- use a function to enqueue pause and resume events (this was part of a
different change attempt that didn't work out; I kept it because I
thought was more readable).
- make mgr->nworkers a signed int to remove some annoying integer casts.
The netmgr listening, stoplistening, pausing and resuming functions
now use barriers for synchronization, which makes the code much simpler.
isc/barrier.h defines isc_barrier macros as a front-end for uv_barrier
on platforms where that works, and pthread_barrier where it doesn't
(including TSAN builds).
When isc__nm_http_stoplistening() is run from inside the netmgr, we need
to make sure it's run synchronously. This commit is just a band-aid
though, as the desired behvaior for isc_nm_stoplistening() is not always
the same:
1. When run from outside user of the interface, the call must be
synchronous, e.g. the calling code expects the call to really stop
listening on the interfaces.
2. But if there's a call from listen<proto> when listening fails,
that needs to be scheduled to run asynchronously, because
isc_nm_listen<proto> is being run in a paused (interlocked)
netmgr thread and we could get stuck.
The proper solution would be to make isc_nm_stoplistening()
behave like uv_close(), i.e., to have a proper callback.
all zone loading tasks have the privileged flag, but we only want
them to run as privileged tasks when the server is being initialized;
if we privilege them the rest of the time, the server may hang for a
long time after a reload/reconfig. so now we call isc_taskmgr_setmode()
to turn privileged execution mode on or off in the task manager.
isc_task_privileged() returns true if the task's privilege flag is
set *and* the taskmgr is in privileged execution mode. this is used
to determine in which netmgr event queue the task should be run.
This workarounds couple of races where the current_lookup would be
already detached during shutting down the dig, but still processing the
pending reads.
The start_udp() function didn't properly attach to the query and thus
a callback with ISC_R_CANCELED would end with wrong accounting on the
query object.
Usually, this doesn't happen because underlying libuv API
uv_udp_connect() is synchronous, but isc_nm_udpconnect() could return
ISC_R_CANCELED in case it's called while the netmgr is shutting down.
There was a theoretical possibility of clogging up the queue processing
with an endless loop where currently processing netievent would schedule
new netievent that would get processed immediately. This wasn't such a
problem when only netmgr netievents were processed, but with the
addition of the tasks, there are at least two situation where this could
happen:
1. In lib/dns/zone.c:setnsec3param() the task would get re-enqueued
when the zone was not yet fully loaded.
2. Tasks have internal quantum for maximum number of isc_events to be
processed, when the task quantum is reached, the task would get
rescheduled and then immediately processed by the netmgr queue
processing.
As the isc_queue doesn't have a mechanism to atomically move the queue,
this commit adds a mechanism to quantize the queue, so enqueueing new
netievents will never stop processing other uv_loop_t events.
The default quantum size is 128.
Since the queue used in the network manager allows items to be enqueued
more than once, tasks are now reference-counted around task_ready()
and task_run(). task_ready() now has a public API wrapper,
isc_task_ready(), that the netmgr can use to reschedule processing
of a task if the quantum has been reached.
Incidental changes: Cleaned up some unused fields left in isc_task_t
and isc_taskmgr_t after the last refactoring, and changed atomic
flags to atomic_bools for easier manipulation.
With taskmgr running on top of netmgr, the ordering of how the tasks and
netmgr shutdown interacts was wrong as previously isc_taskmgr_destroy()
was waiting until all tasks were properly shutdown and detached. This
responsibility was moved to netmgr, so we now need to do the following:
1. shutdown all the tasks - this schedules all shutdown events onto
the netmgr queue
2. shutdown the netmgr - this also makes sure all the tasks and
events are properly executed
3. Shutdown the taskmgr - this now waits for all the tasks to finish
running before returning
4. Shutdown the netmgr - this call waits for all the netmgr netievents
to finish before returning
This solves the race when the taskmgr object would be destroyed before
all the tasks were finished running in the netmgr loops.
Previously, netmgr, taskmgr, timermgr and socketmgr all had their own
isc_<*>mgr_create() and isc_<*>mgr_destroy() functions. The new
isc_managers_create() and isc_managers_destroy() fold all four into a
single function and makes sure the objects are created and destroy in
correct order.
Especially now, when taskmgr runs on top of netmgr, the correct order is
important and when the code was duplicated at many places it's easy to
make mistake.
The former isc_<*>mgr_create() and isc_<*>mgr_destroy() functions were
made private and a single call to isc_managers_create() and
isc_managers_destroy() is required at the program startup / shutdown.
Fix flawed DoH unit tests logic and some corner cases in the DoH code. Fix doh_test failure on FreeBSD 13.0
Closes#2632
See merge request isc-projects/bind9!5005
Under some circumstances a situation might occur when server-side
session gets finished while there are still active HTTP/2
streams. This would lead to isc_nm_httpsocket object leaks.
This commit fixes this behaviour as well as refactors failed_read_cb()
to allow better code reuse.
This commit fixes a situation when a cstream object could get unlinked
from the list as a result of a cstream->read_cb call. Thus, unlinking
it after the call could crash the program.
... the last handle has been detached after calling write
callback. That makes it possible to detach from the underlying socket
and not to keep the socket object alive for too long. This issue was
causing TLS tests with quota to fail because quota might not have been
detached on time (because it was still referenced by the underlying
TCP socket).
One could say that this commit is an ideological continuation of:
513cdb52ec.
This way we create less netievent objects, not bombarding NM with the
messages in case of numerous low-level errors (like too many open
files) in e.g. unit tests.
This change ensures that a TCP connect callback is called from within
the context of a worker thread in case of a low-level error when
descriptors cannot be created (e.g. when there are too many open file
descriptors).
When looking for key files, we could use isdigit rather than checking
if the character is within the range [0-9].
Use (unsigned char) cast to ensure the value is representable in the
unsigned char type (as suggested by the isdigit manpage).
Change " & 0xff" occurrences to the recommended (unsigned char) type
cast.
Just like with dynamic and/or inline-signing zones, check if no two
or more zone configurations set the same filename. In these cases,
the zone files are not read-only and named-checkconf should catch
a configuration where multiple zone statements write to the same file.
Add some bad configuration tests where KASP zones reference the same
zone file.
Update the good-kasp test to allow for two zones configure the same
file name, dnssec-policy none.
When we introduced "dnssec-policy insecure" we could have removed the
'strcmp' check for "none", because if it was set to "none", the 'kasp'
variable would have been set to NULL.
Add a test for default.kasp that if we remove the private key file,
no successor key is created for it. We need to update the kasp script
to deal with a missing private key. If this is the case, skip checks
for private key files.
Add a test with a zone for which the private key of the ZSK is missing.
Add a test with a zone for which the private key of the KSK is missing.
BIND 9 is smart about when to sign with what key. If a key is offline,
BIND will delete the old signature anyway if there is another key to
sign the RRset with.
With KASP we don't want to fallback to the KSK if the ZSK is missing,
only for the SOA RRset. If the KSK is missing, but we do have a ZSK,
deleting the signature is fine. Otherwise it depends on if we use KASP
or not. Update the 'delsig_ok' function to reflect that.
When checking the current DNSSEC state against the policy, consider
offline keys. If we didn't found an active key, check if the key is
offline by checking the public key list. If there is a match in the
public key list (the key data is retrieved from the .key and the
.state files), treat the key as offline and don't create a successor
key for it.
The rndc command 'dnssec -status' only considered keys from
'dns_dnssec_findmatchingkeys' which only includes keys with accessible
private keys. Change it so that offline keys are also listed in the
status.
The function 'dns_dnssec_keylistfromrdataset()' creates a keylist from
the DNSKEY RRset. If we attempt to read the private key, we also store
the key state. However, if the private key is offline, the key state
will not be stored. To fix this, first attempt to read the public key
file. If then reading the private key file fails, and we do have a
public key, add that to the keylist, with appropriate state. If we
also failed to read the public key file, add the DNSKEY to the keylist,
as we did before.
The kasp system test performs for each zone a couple of checks to make
sure the zone is signed correctly. To avoid test failures caused by
timing issues, there is first a check to ensure the zone is done
signing, 'wait_for_done_signing'. This function waits with the DNSSEC
checks until a "zone_rekey done" log message is seen for a specific
key.
Unfortunately this is not sufficient to avoid test failures due to
timing issues, because there is a small amount of time in between this
log message and the newly signed zone actually being served.
Therefore, in 'check_apex', retry for three seconds the DNSKEY query
check. After that, additional checks should pass without retries,
because at that point we know for sure the zone has been resigned with
the expected keys.
Also reduce the number of redundant 'check_signatures'
This commit adds support for generating backtraces on Windows and
refactors the isc_backtrace API to match the Linux/BSD API (without
the isc_ prefix)
* isc_backtrace_gettrace() was renamed to isc_backtrace(), the third
argument was removed and the return type was changed to int
* isc_backtrace_symbols() was added
* isc_backtrace_symbols_fd() was added and used as appropriate
On Windows, the iocompletionport_createthreads() didn't use
isc_thread_create() to create new threads for processing IO, but just a
simple CreateThread() function that completely circumvent the
isc_trampoline mechanism to initialize global isc_tid_v. This lead to
segmentation fault in isc_hp API because '-1' isn't valid index to the
hazard pointer array.
This commit changes the iocompletionport_createthreads() to use
isc_thread_create() instead of CreateThread() to properly initialize
isc_tid_v.
The nsupdate system test did not record failures from the
'update_test.pl' Perl script. This was because the 'ret' value was
not being saved outside the '{ $PERL ... || ret=1 } cat_i' scope.
Change this piece to store the output in a separate file and then
cat its contents. Now the 'ret' value is being saved.
Also record failures in 'update_test.pl' if sending the update
failed.
Add missing 'n' incrementals to 'nsupdate/test.sh' to keep track of
test numbers.
By default readthedocs.org uses Sphinx 1.8.5, but MR !4563 has
introduced depedency on ReferenceRole class which is available only in
Sphinx 2.0.0.
Path to doc/arm/requirements.txt needs to be configured in
readthedocs.org.
Add a test case when a dnssec-policy is reconfigured to "none",
without setting it to "insecure" first. This is unsupported behavior,
but we want to make sure the behavior is somewhat expected. The
zone should remain signed (but will go bogus once the signatures
expire).
Update the ARM to mention the new built-in "insecure" policy. Update
the DNSSEC guide recipe "Revert to unsigned" to add the additional
step of reconfiguring the zone to "insecure" (instead of immediately
set it to "none").
While it is meant to be used for transitioning a zone to insecure,
add a test case where a zone uses the "insecure" policy immediately.
The zone will go through DNSSEC maintenance, but the outcome should
be the same as 'dnssec-policy none;', that is the zone should be
unsigned.
The tests for going insecure should be changed to use the built-in
"insecure" policy.
The function that checks dnssec status output should again check
for the special case "none".
Add a new built-in policy "insecure", to be used to gracefully unsign
a zone. Previously you could just remove the 'dnssec-policy'
configuration from your zone statement, or remove it.
The built-in policy "none" (or not configured) now actually means
no DNSSEC maintenance for the corresponding zone. So if you
immediately reconfigure your zone from whatever policy to "none",
your zone will temporarily be seen as bogus by validating resolvers.
This means we can remove the functions 'dns_zone_use_kasp()' and
'dns_zone_secure_to_insecure()' again. We also no longer have to
check for the existence of key state files to figure out if a zone
is transitioning to insecure.
* The location of the digest type field has changed to where the
reserved field was.
* The reserved field is now called scheme and is where the digest
type field was.
* Digest type 2 has been defined (SHA256).
dnstap_test produces TSAN errors which originate in libfstrm.so. Unless
libfstrm is TSAN clean or a workaround is placed in libfstrm sources,
suppressing TSAN coming from libfstrm is necessary to test DNSTAP under
TSAN.
All platforms but OpenBSD have dnstap dependencies readily in their
respective repositories, and dnstap thus can be tested there. Given that
majority of images have dnstap dependencies available, it seems fitting
to make dnstap enabled by default.
The pytest "cacheprovider" plugin produces a .cache/v/cache/lastfailed
file, which holds a Python dictionary structure with failed tests.
However, on Ubuntu 16.04 (Xenial) the file is created even though the
test passed and the file contains just an empty dictionary ("{}").
Given that we are not interested in this feature, disabling the
"cacheprovider" plugin globally and removing per-test removals of the
.cache directory seems like the best course of action.
Define a :gl: Sphinx role that takes a GitLab issue/MR number as an
argument and creates a hyperlink to the relevant ISC GitLab URL. This
makes it easy to reach ISC GitLab pages directly from the release notes.
Make all GitLab references in the release notes use the new Sphinx role.
When the `named` would hang on startup it would be killed with SIGKILL
leaving us with no information about the state the process was in.
This commit changes the start.pl script to send SIGABRT instead, so we
can properly collect and process the coredump from the hung named
process.
When reducing the number of NSEC3 iterations to 150, commit
aa26cde2ae added tests for dnssec-policy
to check that a too high iteration count is a configuration failure.
The test is not sufficient because 151 was always too high for
ECDSAP256SHA256. The test should check for a different algorithm.
There was an existing test case that checks for NSEC3 iterations.
Update the test with the new maximum values.
Update the code in 'kaspconf.c' to allow at most 150 iterations.
[CVE-2021-25215] Properly answer queries for DNAME records that require the DNAME to be processed to resolve itself
See merge request isc-private/bind9!253
When answering a query, named should never attempt to add the same RRset
to the ANSWER section more than once. However, such a situation may
arise when chasing DNAME records: one of the DNAME records placed in the
ANSWER section may turn out to be the final answer to a client query,
but there is no way to know that in advance. Tweak the relevant INSIST
assertion in query_respond() so that it handles this case properly.
qctx->rdataset is freed later anyway, so there is no need to clean it up
in query_respond().
If a zone transfer results in a zone not having any NS records, named
stops serving it because such a zone is broken. Do the same if an
incoming zone transfer results in a zone lacking an SOA record at the
apex or containing more than one SOA record.
An IXFR containing SOA records with owner names different than the
transferred zone's origin can result in named serving a version of that
zone without an SOA record at the apex. This causes a RUNTIME_CHECK
assertion failure the next time such a zone is refreshed. Fix by
immediately rejecting a zone transfer (either an incremental or
non-incremental one) upon detecting an SOA record not placed at the apex
of the transferred zone.
While working on the serve-stale backports, I noticed the following
oddities:
1. In the serve-stale system test, in one case we keep track of the
time how long it took for dig to complete. In commit
aaed7f9d8c, the code removed the
exception to check for result == ISC_R_SUCCESS on stale found
answers, and adjusted the test accordingly. This failed to update
the time tracking accordingly. Move the t1/t2 time track variables
back around the two dig commands to ensure the lookups resolved
faster than the resolver-query-timeout.
2. We can remove the setting of NS_QUERYATTR_STALEOK and
DNS_RDATASETATTR_STALE_ADDED on the "else if (stale_timeout)"
code path, because they are added later when we know we have
actually found a stale answer on a stale timeout lookup.
3. We should clear the NS_QUERYATTR_STALEOK flag from the client
query attributes instead of DNS_RDATASETATTR_STALE_ADDED (that
flag is set on the rdataset attributes).
4. In 'bin/named/config.c' we should set the configuration options
in alpabetical order.
5. In the ARM, in the backports we have added "(stale)" between
"cached" and "RRset" to make more clear a stale RRset may be
returned in this scenario.
Exerting excessive I/O load on the host running system tests should be
avoided in order to limit the number of false positives reported by the
system test suite. In some cases, running named with "-d 99" (which is
the default for system tests) results in a massive amount of logs being
generated, most of which are useless. Implement a log file size check
to draw developers' attention to overly verbose named instances used in
system tests. The warning threshold of 200,000 lines was chosen
arbitrarily.
The regression test for CVE-2020-8620 causes a lot of useless messages
to be logged. However, globally decreasing the log level for the
affected named instance would be a step too far as debugging information
may be useful for troubleshooting other checks in the "tcp" system test.
Starting a separate named instance for a single check should be avoided
when possible and thus is also not a good solution. As a compromise,
run "rndc trace 1" for the affected named instance before starting the
regression test for CVE-2020-8620.
The system test framework starts all named instances with the "-d 99"
command line option (unless it is overridden by a named.args file in a
given instance's working directory). This causes a lot of log messages
to be written to named.run files - currently over 5 million lines for a
single test suite run. While debugging information preserved in the log
files is essential for troubleshooting intermittent test failures, some
system tests involve sending hundreds or even thousands of queries,
which causes the relevant log files to explode in size. When multiple
tests (or even multiple test suites) are run in parallel, excessive
logging contributes considerably to the I/O load on the test host,
increasing the odds of intermittent test failures getting triggered.
Decrease the debug level for the seven most verbose named instances:
- use "-d 3" for ns2 in the "cacheclean" system test (it is the lowest
logging level at which the test still passes without the need to
apply any changes to tests.sh),
- use "-d 1" for the other six named instances.
This roughly halves the number of lines logged by each test suite run
while still leaving enough information in the logs to allow at least
basic troubleshooting in case of test failures.
This approach was chosen as it results in a greater decrease in the
number of lines logged than running all named instances with "-d 3",
without causing any test failures.
The malloc attribute allows compiler to do some optmizations on
functions that behave like malloc/calloc, like assuming that the
returned pointer do not alias other pointers.
There is no possibility for mpctx->items to be NULL at the point where
the code was removed, since we enforce that fillcount > 0, if
mpctx->items == NULL when isc_mempool_get is called, then we will
allocate fillcount more items and add to the mpctx->items list.
_query_detach function was incorrectly unliking the query object from
the lookup->q query list, this made it impossible to follow a query
lookup failure with the next one in the list (possibly using a separate
resolver), as the link to the next query in the list was dissolved.
Fix by unliking the node only when the query object is about to be
destroyed, i.e. there is no more references to the object.
When the keymgr needs to create new keys, it is possible it needs to
create multiple keys. The keymgr checks for keyid conflicts with
already existing keys, but it should also check against that it just
created.
GitLab CI pipelines do not currently include a Linux job that would have
GSSAPI support disabled. Add the "--without-gssapi" option to the
./configure invocation on Debian 9 to address that deficiency and also
to continuously test that build-time switch.
If "tkey-gssapi-credential" is set in the configuration and GSSAPI
support is not available, named will refuse to start. As the test
system framework does not support starting named instances
conditionally, ensure that "tkey-gssapi-credential" is only present in
named.conf if GSSAPI support is available.
as with TLS, the destruction of a client stream on failed read
needs to be conditional: if we reached failed_read_cb() as a
result of a timeout on a timer which has subsequently been
reset, the stream must not be closed.
Add a check to the "dnssec" system test which ensures that RRSIG(SOA)
RRsets present anywhere else than at the zone apex are automatically
removed after a zone containing such RRsets is loaded.
If there happens to be a RRSIG(SOA) that is not at the zone apex
for any reason it should not be considered as a stopping condition
for incremental zone signing.
the destruction of the socket in tls_failed_read_cb() needs to be
conditional; if reached due to a timeout on a timer that has
subsequently been reset, the socket must not be destroyed.
this is similar in structure to the UDP timeout recovery test.
this commit adds a new mechanism to the netmgr test allowing the
listen socket to accept incoming TCP connections but never send
a response. this forces the client to time out on read.
when running read callbacks, if the event result is not ISC_R_SUCCESS,
the callback is always run asynchronously. this is a problem on timeout,
because there's no chance to reset the timer before the socket has
already been destroyed. this commit allows read callbacks to run
synchronously for both ISC_R_SUCCESS and ISC_R_TIMEDOUT result codes.
this test sets up a server socket that listens for UDP connections
but never responds. the client will always time out; it should retry
five times before giving up.
The test spawns 4 parallel workers that keep adding, modifying and
deleting zones, the main thread repeatedly checks wheter rndc
status responds within a reasonable period.
While environment and timing issues may affect the test, in most
test cases the deadlock that was taking place before the fix used to
trigger in less than 7 seconds in a machine with at least 2 cores.
It follows a description of the steps that were leading to the deadlock:
1. `do_addzone` calls `isc_task_beginexclusive`.
2. `isc_task_beginexclusive` waits for (N_WORKERS - 1) halted tasks,
this blocks waiting for those (no. workers -1) workers to halt.
...
isc_task_beginexclusive(isc_task_t *task0) {
...
while (manager->halted + 1 < manager->workers) {
wake_all_queues(manager);
WAIT(&manager->halt_cond, &manager->halt_lock);
}
```
3. It is possible that in `task.c / dispatch()` a worker is running a
task event, if that event blocks it will not allow this worker to
halt.
4. `do_addzone` acquires `LOCK(&view->new_zone_lock);`,
5. `rmzone` event is called from some worker's `dispatch()`, `rmzone`
blocks waiting for the same lock.
6. `do_addzone` calls `isc_task_beginexclusive`.
7. Deadlock triggered, since:
- `rmzone` is wating for the lock.
- `isc_task_beginexclusive` is waiting for (no. workers - 1) to
be halted
- since `rmzone` event is blocked it won't allow the worker to halt.
To fix this, we updated do_addzone code to call isc_task_beginexclusive
before the lock is acquired, we postpone locking to the nearest required
place, same for isc_task_beginexclusive.
The same could happen with rndc modzone, so that was addressed as well.
Four named instances in the "nsupdate" system test have GSS-TSIG support
enabled. All of them currently use "tkey-gssapi-keytab". Configure two
of them with "tkey-gssapi-credential" to test that option.
As "tkey-gssapi-keytab" and "tkey-gssapi-credential" both provide the
same functionality, no test modifications are required. The difference
between the two options is that the value of "tkey-gssapi-keytab" is an
explicit path to the keytab file to acquire credentials from, while the
value of "tkey-gssapi-credential" is the name of the principal whose
credentials should be used; those credentials are looked up in the
keytab file expected by the Kerberos library, i.e. /etc/krb5.keytab by
default. The path to the default keytab file can be overridden using by
setting the KRB5_KTNAME environment variable. Utilize that variable to
use existing keytab files with the "tkey-gssapi-credential" option.
The KRB5_KTNAME environment variable should not interfere with the
"tkey-gssapi-keytab" option. Nevertheless, rename one of the keytab
files used with "tkey-gssapi-keytab" to something else than the contents
of the KRB5_KTNAME environment variable in order to make sure that both
"tkey-gssapi-keytab" and "tkey-gssapi-credential" are actually tested.
This mostly removes stuff that's either deprecated, obsolete or not used
at all:
* Update the minimal autoconf version to 2.69
* AC_PROG_CC_C99 is deprecated, just use AC_PROG_CC as we require C11
anyway
* AC_HEADER_TIME is deprecated, both <sys/time.h> and <time.h> can be
included at the same time, and we don't use the macros that
AC_HEADER_TIME defines anywhere
* AC_HEADER_STDC checks for ISO C90 and we require at least C11
* Replace AC_TRY_*([]) with AC_*_IFELSE([AC_LANG_PROGRAM()])
* Update m4/ax_check_openssl.m4 from serial 10 to serial 11
* Update m4/ax_gcc_func_attribute.m4 from serial 10 to serial 13
* Update m4/ax_pthread.m4 from serial 24 to serial 30
* Add early AC_CANONICAL_TARGET call to prevent warning from AX_PTHREAD
This commit changes the taskmgr to run the individual tasks on the
netmgr internal workers. While an effort has been put into keeping the
taskmgr interface intact, couple of changes have been made:
* The taskmgr has no concept of universal privileged mode - rather the
tasks are either privileged or unprivileged (normal). The privileged
tasks are run as a first thing when the netmgr is unpaused. There
are now four different queues in in the netmgr:
1. priority queue - netievent on the priority queue are run even when
the taskmgr enter exclusive mode and netmgr is paused. This is
needed to properly start listening on the interfaces, free
resources and resume.
2. privileged task queue - only privileged tasks are queued here and
this is the first queue that gets processed when network manager
is unpaused using isc_nm_resume(). All netmgr workers need to
clean the privileged task queue before they all proceed normal
operation. Both task queues are processed when the workers are
finished.
3. task queue - only (traditional) task are scheduled here and this
queue along with privileged task queues are process when the
netmgr workers are finishing. This is needed to process the task
shutdown events.
4. normal queue - this is the queue with netmgr events, e.g. reading,
sending, callbacks and pretty much everything is processed here.
* The isc_taskmgr_create() now requires initialized netmgr (isc_nm_t)
object.
* The isc_nm_destroy() function now waits for indefinite time, but it
will print out the active objects when in tracing mode
(-DNETMGR_TRACE=1 and -DNETMGR_TRACE_VERBOSE=1), the netmgr has been
made a little bit more asynchronous and it might take longer time to
shutdown all the active networking connections.
* Previously, the isc_nm_stoplistening() was a synchronous operation.
This has been changed and the isc_nm_stoplistening() just schedules
the child sockets to stop listening and exits. This was needed to
prevent a deadlock as the the (traditional) tasks are now executed on
the netmgr threads.
* The socket selection logic in isc__nm_udp_send() was flawed, but
fortunatelly, it was broken, so we never hit the problem where we
created uvreq_t on a socket from nmhandle_t, but then a different
socket could be picked up and then we were trying to run the send
callback on a socket that had different threadid than currently
running.
When we are reading from the xfrin socket, and the transfer would be
shutdown, the shutdown function would call `xfrin_fail()` which in turns
calls `xfrin_cancelio()` that causes the read callback to be invoked
with `ISC_R_CANCELED` status code and that caused yet another
`xfrin_fail()` call.
The fix here is to ensure the `xfrin_fail()` would be run only once
properly using better synchronization on xfr->shuttingdown flag.
Since all the libraries are internal now, just cleanup the ISCAPI remnants
in isc_socket, isc_task and isc_timer APIs. This means, there's one less
layer as following changes have been done:
* struct isc_socket and struct isc_socketmgr have been removed
* struct isc__socket and struct isc__socketmgr have been renamed
to struct isc_socket and struct isc_socketmgr
* struct isc_task and struct isc_taskmgr have been removed
* struct isc__task and struct isc__taskmgr have been renamed
to struct isc_task and struct isc_taskmgr
* struct isc_timer and struct isc_timermgr have been removed
* struct isc__timer and struct isc__timermgr have been renamed
to struct isc_timer and struct isc_timermgr
* All the associated code that dealt with typing isc_<foo>
to isc__<foo> and back has been removed.
When resolve.c was moved from lib/samples to bin/tests/system, the
resolve.vcxproj.in would still contain old paths to the directory
root. This commit adds one more ..\ to match the directory depth.
Additionally, fixup the path in BINDInstall.vcxproj.in to be
bin/tests/system and not bin/tests/samples.
When setnsec3param() is schedule from zone_postload() there's no
guarantee that `zone->db` is not `NULL` yet. Thus when the
setnsec3param() is called, we need to check for `zone->db` existence and
reschedule the task, because calling `rss_post()` on a zone with empty
`.db` ends up with no-op (the function just returns).
Previously, the taskmgr, timermgr and socketmgr had a constructor
variant, that would create the mgr on top of existing appctx. This was
no longer true and isc_<*>mgr was just calling isc_<*>mgr_create()
directly without any extra code.
This commit just cleans up the extra function.
"resolve" is used by the resolver system tests, and I'm not
certain whether delv exercises the same code, so rather than
remove it, I moved it to bin/tests/system.
sample code for export libraries is no longer needed and
this code is not used for any internal tests. also, sample-gai.c
had already been removed but there were some dangling references.
the libdns client API is no longer being maintained for
external use, we can remove the code that isn't being used
internally, as well as the related tests.
Too much logic was cramped inside the dns_journal_rollforward() that
made it harder to follow. The dns_journal_rollforward() was refactored
to work over already opened journal and some of the previous logic was
moved to new static zone_journal_rollforward() that separates the
journal "rollforward" logic from the "zone" logic.
when dns_journal_rollforward returned ISC_R_RECOVERABLE the distintion
between 'up to date' and 'success' was lost, as a consequence
zone_needdump() was called writing out the zone file when it shouldn't
have been. This change restores that distintion. Adjust system
test to reflect visible changes.
It fixes a corner case which was causing dig to print annoying
messages like:
14-Apr-2021 18:48:37.099 SSL error in BIO: 1 TLS error (errno:
0). Arguments: received_data: (nil), send_data: (nil), finish: false
even when all the data was properly processed.
Before this fix underlying TCP sockets could remain opened for longer
than it is actually required, causing unit tests to fail with lots of
ISC_R_TOOMANYOPENFILES errors.
The change also enables graceful SSL shutdown (before that it would
happen only in the case when isc_nm_cancelread() were called).
This commit merges TLS tests into the common Network Manager unit
tests suite and extends the unit test framework to include support for
additional "ping-pong" style tests where all data could be sent via
lesser number of connections (the behaviour of the old test
suite). The tests for TCP and TLS were extended to make use of the new
mode, as this mode better translates to how the code is used in DoH.
Both TLS and TCP tests now share most of the unit tests' code, as they
are expected to function similarly from a users's perspective anyway.
Additionally to the above, the TLS test suite was extended to include
TLS tests using the connections quota facility.
Due to the lack of "match-clients" clauses in ns4/named2.conf.in, the
same view is incorrectly chosen for all queries received by ns4 in the
"keymgr2kasp" system test. This causes only one version of the
"view-rsasha256.kasp" zone to actually be checked. Add "match-clients"
clauses to ns4/named2.conf.in to ensure the test really checks what it
claims to.
Use identical view names ("ext", "int") in ns4/named.conf.in and
ns4/named2.conf.in so that it is easier to quickly identify the
differences between these two files.
Update tests.sh to account for the above changes. Also fix a copy-paste
error in a comment to prevent confusion.
The test case for a zone with a missing include file was wrong for two
reasons:
1. It was loading the wrong file (master5 instead of master6)
2. It did actually not set the $ret variable to 1 if the test failed
(it should default to ret=1 and clear the variable if the
appropriate log is found).
Add a test case for inline-signing for a zone with an $INCLUDE
statement. There is already a test for a missing include file, this
one adds a test for a zone with an include file that does exist.
Test if the record in the included file is loaded.
The draft says that the NSEC(3) TTL must have the same TTL value
as the minimum of the SOA MINIMUM field and the SOA TTL. This was
always the intended behaviour.
Update the zone structure to also track the SOA TTL. Whenever we
use the MINIMUM value to determine the NSEC(3) TTL, use the minimum
of MINIMUM and SOA TTL instead.
There is no specific test for this, however two tests need adjusting
because otherwise they failed: They were testing for NSEC3 records
including the TTL. Update these checks to use 600 (the SOA TTL),
rather than 3600 (the SOA MINIMUM).
It is more intuitive to have the countdown 'max-stale-ttl' as the
RRset TTL, instead of 0 TTL. This information was already available
in a comment "; stale (will be retained for x more seconds", but
Support suggested to put it in the TTL field instead.
Before binding an RRset, check the time and see if this record is
stale (or perhaps even ancient). Marking a header stale or ancient
happens only when looking up an RRset in cache, but binding an RRset
can also happen on other occasions (for example when dumping the
database).
Check the time and compare it to the header. If according to the
time the entry is stale, but not ancient, set the STALE attribute.
If according to the time is ancient, set the ANCIENT attribute.
We could mark the header stale or ancient here, but that requires
locking, so that's why we only compare the current time against
the rdh_ttl.
Adjust the test to check the dump-db before querying for data. In the
dumped file the entry should be marked as stale, despite no cache
lookup happened since the initial query.
When introducing change 5149, "rndc dumpdb" started to print a line
above a stale RRset, indicating how long the data will be retained.
At that time, I thought it should also be possible to load
a cache from file. But if a TTL has a value of 0 (because it is stale),
stale entries wouldn't be loaded from file. So, I added the
'max-stale-ttl' to TTL values, and adjusted the $DATE accordingly.
Since we actually don't have a "load cache from file" feature, this
is premature and is causing confusion at operators. This commit
changes the 'max-stale-ttl' adjustments.
A check in the serve-stale system test is added for a non-stale
RRset (longttl.example) to make sure the TTL in cache is sensible.
Also, the comment above stale RRsets could have nonsensical
values. A possible reason why this may happen is when the RRset was
marked a stale but the 'max-stale-ttl' has passed (and is actually an
RRset awaiting cleanup). This would lead to the "will be retained"
value to be negative (but since it is stored in an uint32_t, you would
get a nonsensical value (e.g. 4294362497).
To mitigate against this, we now also check if the header is not
ancient. In addition we check if the stale_ttl would be negative, and
if so we set it to 0. Most likely this will not happen because the
header would already have been marked ancient, but there is a possible
race condition where the 'rdh_ttl + serve_stale_ttl' has passed,
but the header has not been checked for staleness.
When system tests are run on Windows, they are assigned port ranges that
are 100 ports wide and start from port number 5000. This is a different
port assignment method than the one used on Unix systems. Drop the "-p"
command line option from bin/tests/system/run.sh invocations used for
starting system tests on Windows to unify the port assignment method
used across all operating systems.
The get_ports.sh script is used for determining the range of ports a
given system test should use. It first determines the start of the port
range to return (the base port); it can either be specified explicitly
by the caller or chosen randomly. Subsequent ports are picked
sequentially, starting from the base port. To ensure no single port is
used by multiple tests, a state file (get_ports.state) containing the
last assigned port is maintained by the script. Concurrent access to
the state file is protected by a lock file (get_ports.lock); if one
instance of the script holds the lock file while another instance tries
to acquire it, the latter retries its attempt to acquire the lock file
after sleeping for 1 second; this retry process can be repeated up to 10
times before the script returns an error.
There are some problems with this approach:
- the sleep period in case of failure to acquire the lock file is
fixed, which leads to a "thundering herd" type of problem, where
(depending on how processes are scheduled by the operating system)
multiple system tests try to acquire the lock file at the same time
and subsequently sleep for 1 second, only for the same situation to
likely happen the next time around,
- the lock file is being locked and then unlocked for every single
port assignment made, not just once for the entire range of ports a
system test should use; in other words, the lock file is currently
locked and unlocked 13 times per system test; this increases the
odds of the "thundering herd" problem described above preventing a
system test from getting one or more ports assigned before the
maximum retry count is reached (assuming multiple system tests are
run in parallel); it also enables the range of ports used by a given
system test to be non-sequential (which is a rather cosmetic issue,
but one that can make log interpretation harder than necessary when
test failures are diagnosed),
- both issues described above cause unnecessary delays when multiple
system tests are started in parallel (due to high lock file
contention among the system tests being started),
- maintaining a state file requires ensuring proper locking, which
complicates the script's source code.
Rework the get_ports.sh script so that it assigns non-overlapping port
ranges to its callers without using a state file or a lock file:
- add a new command line switch, "-t", which takes the name of the
system test to assign ports for,
- ensure every instance of get_ports.sh knows how many ports all
system tests which form the test suite are going to need in total
(based on the number of subdirectories found in bin/tests/system/),
- in order to ensure all instances of get_ports.sh work on the same
global port range (so that no port range collisions happen), a
stable (throughout the expected run time of a single system test
suite) base port selection method is used instead of the random one;
specifically, the base port, unless specified explicitly using the
"-p" command line switch, is derived from the number of hours which
passed since the Unix Epoch time,
- use the name of the system test to assign ports for (passed via the
new "-t" command line switch) as a unique index into the global
system test range, to ensure all system tests use disjoint port
ranges.
The fromhex.pl script needs to be copied from the source directory to
the build directory before any test is run, otherwise the out-of-tree
fails to find it. Given that the script is used only in system test,
move it to bin/tests/system/.
Even if a call to gss_accept_sec_context() fails, it might still cause a
GSS-API response token to be allocated and left for the caller to
release. Make sure the token is released before an early return from
dst_gssapi_acceptctx().
Update the system to include a recoverable managed.keys journal created
with <size,serial0,serial1,0> transactions and test that it has been
updated as part of the start up process.
Previously, dns_journal_begin_transaction() could reserve the wrong
amount of space. We now check that the transaction is internally
consistent when upgrading / downgrading a journal and we also handle the
bad transaction headers.
Instead of journal_write(), use correct format call journal_write_xhdr()
to write the dummy transaction header which looks at j->header_ver1 to
determine which transaction header to write instead of always writing a
zero filled journal_rawxhdr_t header.
The isc_nm_tlsdnsconnect() call could end up with two connect callbacks
called when the timeout fired and the TCP connection was aborted,
but the TLS handshake was not complete yet. isc__nm_connecttimeout_cb()
forgot to clean up sock->tls.pending_req when the connect callback was
called with ISC_R_TIMEDOUT, leading to a second callback running later.
A new argument has been added to the isc__nm_*_failed_connect_cb and
isc__nm_*_failed_read_cb functions, to indicate whether the callback
needs to run asynchronously or not.
We already skip most of the recv_send tests in CI because they are
too timing-related to be run in overloaded environment. This commit
adds a similar change to tls_test before we merge tls_test into
netmgr_test.
if a test failed at the beginning of nm_teardown(), the function
would abort before isc_nm_destroy() or isc_tlsctx_free() were reached;
we would then abort when nm_setup() was run for the next test case.
rearranging the teardown function prevents this problem.
The isc_nm_*connect() functions were refactored to always return the
connection status via the connect callback instead of sometimes returning
the hard failure directly (for example, when the socket could not be
created, or when the network manager was shutting down).
This commit changes the connect functions in all the network manager
modules, and also makes the necessary refactoring changes in places
where the connect functions are called.
dig previously ran isc_nm_udpconnect() three times before giving
up, to work around a freebsd bug that caused connect() to return
a spurious transient EADDRINUSE. this commit moves the retry code
into the network manager itself, so that isc_nm_udpconnect() no
longer needs to return a result code.
The TCP module has been updated to use the generic functions from
netmgr.c instead of its own local copies. This brings the module
mostly up to par with the TCPDNS and TLSDNS modules.
Serveral problems were discovered and fixed after the change in
the connection timeout in the previous commits:
* In TLSDNS, the connection callback was not called at all under some
circumstances when the TCP connection had been established, but the
TLS handshake hadn't been completed yet. Additional checks have
been put in place so that tls_cycle() will end early when the
nmsocket is invalidated by the isc__nm_tlsdns_shutdown() call.
* In TCP, TCPDNS and TLSDNS, new connections would be established
even when the network manager was shutting down. The new
call isc__nm_closing() has been added and is used to bail out
early even before uv_tcp_connect() is attempted.
Similarly to the read timeout, it's now possible to recover from
ISC_R_TIMEDOUT event by restarting the timer from the connect callback.
The change here also fixes platforms that missing the socket() options
to set the TCP connection timeout, by moving the timeout code into user
space. On platforms that support setting the connect timeout via a
socket option, the timeout has been hardcoded to 2 minutes (the maximum
value of tcp-initial-timeout).
Previously, when the client timed out on read, the client socket would
be automatically closed and destroyed when the nmhandle was detached.
This commit changes the logic so that it's possible for the callback to
recover from the ISC_R_TIMEDOUT event by restarting the timer. This is
done by calling isc_nmhandle_settimeout(), which prevents the timeout
handling code from destroying the socket; instead, it continues to wait
for data.
One specific use case for multiple timeouts is serve-stale - the client
socket could be created with shorter timeout (as specified with
stale-answer-client-timeout), so we can serve the requestor with stale
answer, but keep the original query running for a longer time.
The full netmgr test suite is unstable when run in CI due to various
timing issues. Previously, we enabled the full test suite only when
CI_ENABLE_ALL_TESTS environment variable was set, but that went against
original intent of running the full suite when an individual developer
would run it locally.
This change disables the full test suite only when running in the CI and
the CI_ENABLE_ALL_TESTS is not set.
Using "stale-answer-client-timeout" turns out to have unforeseen
negative consequences, and thus it is better to disable the feature
by default for the time being.
Fix race between zone_maintenance and dns_zone_notifyreceive functions,
zone_maintenance was attempting to read a zone flag calling
DNS_ZONE_FLAG(zone, flag) while dns_zone_notifyreceive was updating
a flag in the same zone calling DNS_ZONE_SETFLAG(zone, ...).
The code reading the flag in zone_maintenance was not protected by the
zone's lock, to avoid a race the zone's lock is now being acquired
before an attempt to read the zone flag is made.
When a unit test binary hangs, the GitLab CI job in which it is run is
stuck until its run time limit is exceeded. Furthermore, it is not
trivial to determine which test(s) hung in a given GitLab CI job based
on its log. To prevent these issues, enforce a run time limit on every
binary executed by the lib/unit-test-driver.sh script. Use a timeout of
5 minutes for consistency with older BIND 9 branches, which employed
Kyua for running unit tests. Report an exit code of 124 when the run
time limit is exceeded for a unit test binary, for consistency with the
"timeout" tool included in GNU coreutils.
See "BUGS" section at:
https://www.openssl.org/docs/man1.1.1/man3/SSL_get_error.html
It is mentioned there that when TLS status equals SSL_ERROR_SYSCALL
AND errno == 0 it means that underlying transport layer returned EOF
prematurely. However, we are managing the transport ourselves, so we
should just resume reading from the TCP socket.
It seems that this case has been handled properly on modern versions
of OpenSSL. That being said, the situation goes in line with the
manual: it is briefly mentioned there that SSL_ERROR_SYSCALL might be
returned not only in a case of low-level errors (like system call
failures).
When we are recursing, RPZ processing is not allowed. But when we are
performing a lookup due to "stale-answer-client-timeout", we are still
recursing. This effectively means that RPZ processing is disabled on
such a lookup.
In this case, bail the "stale-answer-client-timeout" lookup and wait
for recursion to complete, as we we can't perform the RPZ rewrite
rules reliably.
The dboption DNS_DBFIND_STALEONLY caused confusion because it implies
we are looking for stale data **only** and ignore any active RRsets in
the cache. Rename it to DNS_DBFIND_STALETIMEOUT as it is more clear
the option is related to a lookup due to "stale-answer-client-timeout".
Rename other usages of "staleonly", instead use "lookup due to...".
Also rename related function and variable names.
When doing a staleonly lookup we don't want to fallback to recursion.
After all, there are obviously problems with recursion, otherwise we
wouldn't do a staleonly lookup.
When resuming from recursion however, we should restore the
RECURSIONOK flag, allowing future required lookups for this client
to recurse.
When implementing "stale-answer-client-timeout", we decided that
we should only return positive answers prematurely to clients. A
negative response is not useful, and in that case it is better to
wait for the recursion to complete.
To do so, we check the result and if it is not ISC_R_SUCCESS, we
decide that it is not good enough. However, there are more return
codes that could lead to a positive answer (e.g. CNAME chains).
This commit removes the exception and now uses the same logic that
other stale lookups use to determine if we found a useful stale
answer (stale_found == true).
This means we can simplify two test cases in the serve-stale system
test: nodata.example is no longer treated differently than data.example.
The NS_QUERYATTR_ANSWERED attribute is to prevent sending a response
twice. Without the attribute, this may happen if a staleonly lookup
found a useful answer and sends a response to the client, and later
recursion ends and also tries to send a response.
The attribute was also used to mask adding a duplicate RRset. This is
considered harmful. When we created a response to the client with a
stale only lookup (regardless if we actually have send the response),
we should clear the rdatasets that were added during that lookup.
Mark such rdatasets with the a new attribute,
DNS_RDATASETATTR_STALE_ADDED. Set a query attribute
NS_QUERYATTR_STALEOK if we may have added rdatasets during a stale
only lookup. Before creating a response on a normal lookup, check if
we can expect rdatasets to have been added during a staleonly lookup.
If so, clear the rdatasets from the message with the attribute
DNS_RDATASETATTR_STALE_ADDED set.
With stale-answer-client-timeout, we may send a response to the client,
but we may want to hold on to the network manager handle, because
recursion is going on in the background, or we need to refresh a
stale RRset.
Simplify the setting of 'nodetach':
* During a staleonly lookup we should not detach the nmhandle, so just
set it prior to 'query_lookup()'.
* During a staleonly "stalefirst" lookup set the 'nodetach' to true
if we are going to refresh the RRset.
Now there is no longer the need to clear the 'nodetach' if we go
through the "dbfind_stale", "stale_refresh_window", or "stale_only"
paths.
When doing a staleonly lookup, ignore active RRsets from cache. If we
don't, we may add a duplicate RRset to the message, and hit an
assertion failure in query.c because adding the duplicate RRset to the
ANSWER section failed.
This can happen on a race condition. When a client query is received,
the recursion is started. When 'stale-answer-client-timeout' triggers
around the same time the recursion completes, the following sequence
of events may happen:
1. Queue the "try stale" fetch_callback() event to the client task.
2. Add the RRsets from the authoritative response to the cache.
3. Queue the "fetch complete" fetch_callback() event to the client task.
4. Execute the "try stale" fetch_callback(), which retrieves the
just-inserted RRset from the database.
5. In "ns_query_done()" we are still recursing, but the "staleonly"
query attribute has already been cleared. In other words, the
query will resume when recursion ends (it already has ended but is
still on the task queue).
6. Execute the "fetch complete" fetch_callback(). It finds the answer
from recursion in the cache again and tries to add the duplicate to
the answer section.
This commit changes the logic for finding stale answers in the cache,
such that on "stale_only" lookups actually only stale RRsets are
considered. It refactors the code so that code paths for "dbfind_stale",
"stale_refresh_window", and "stale_only" are more clear.
First we call some generic code that applies in all three cases,
formatting the domain name for logging purposes, increment the
trystale stats, and check if we actually found stale data that we can
use.
The "dbfind_stale" lookup will return SERVFAIL if we didn't found a
usable answer, otherwise we will continue with the lookup
(query_gotanswer()). This is no different as before the introduction of
"stale-answer-client-timeout" and "stale-refresh-time".
The "stale_refresh_window" lookup is similar to the "dbfind_stale"
lookup: return SERVFAIL if we didn't found a usable answer, otherwise
continue with the lookup (query_gotanswer()).
Finally the "stale_only" lookup.
If the "stale_only" lookup was triggered because of an actual client
timeout (stale-answer-client-timeout > 0), and if database lookup
returned a stale usable RRset, trigger a response to the client.
Otherwise return and wait until the recursion completes (or the
resolver query times out).
If the "stale_only" lookup is a "stale-anwer-client-timeout 0" lookup,
preferring stale data over a lookup. In this case if there was no stale
data, or the data was not a positive answer, retry the lookup with the
stale options cleared, a.k.a. a normal lookup. Otherwise, continue
with the lookup (query_gotanswer()) and refresh the stale RRset. This
will trigger a response to the client, but will not detach the handle
because a fetch will be created to refresh the RRset.
The stale-answer-client-timeout feature introduced a dependancy on
when a client may be detached from the handle. The dboption
DNS_DBFIND_STALEONLY was reused to track this attribute. This overloads
the meaning of this database option, and actually introduced a bug
because the option was checked in other places. In particular, in
'ns_query_done()' there is a check for 'RECURSING(qctx->client) &&
(!QUERY_STALEONLY(&qctx->client->query) || ...' and the condition is
satisfied because recursion has not completed yet and
DNS_DBFIND_STALEONLY is already cleared by that time (in
query_lookup()), because we found a useful answer and we should detach
the client from the handle after sending the response.
Add a new boolean to the client structure to keep track of client
detach from handle is allowed or not. It is only disallowed if we are
in a staleonly lookup and we didn't found a useful answer.
This commit fixes crash in dig when it encounters non-expected header
value. The bug was introduced at some point late in the last DoH
development cycle. Also, refactors the relevant code a little bit to
ensure better incoming data validation for client-side DoH
connections.
Tag the libraries with check_ to prevent them being installed
by "make install". Additionally make check requires .so to be
create which requires .lai files to be constructed which, in
turn, requires -rpath <dir> as part of "linking" the .la file.
The gcc:tarball CI job may identify problems with tarballs created by
"make dist" of the tarball-create CI job. Enabling the gcc:tarball CI
job in web-triggered pipelines provides developers with a test vector.
Some man pages (e.g. dnstap-read.1, named-nzd2nzf.1) should only be
installed conditionally (when the relevant features are enabled in a
given BIND 9 build). This is achieved using Automake conditionals.
However, while all source reStructuredText files are included in
tarballs produced by "make dist" (distribution tarballs) as they should
be, the list of pre-generated man pages included in distribution
tarballs incorrectly depends on the ./configure switches used for the
build for which "make dist" is run. Meanwhile, distribution tarballs
should always contain all the files necessary to build any flavor of
BIND 9.
Here is an example scenario which fails to work as intended:
autoreconf -i
./configure --disable-maintainer-mode
make dist
tar --extract --file bind-9.17.11.tar.xz
cd bind-9.17.11
./configure --disable-maintainer-mode --enable-dnstap
make
Fix by always including pre-generated versions of all conditionally
installed man pages in EXTRA_DIST. While this may cause some of them to
appear in EXTRA_DIST more than once (depending on the ./configure
switches used for the build for which "make dist" is run), it seems to
not be a problem for Automake.
add matching macros to pass arguments from called methods
to generic methods. This will reduce the amount of work
required when extending methods.
Also cleanup unnecessary UNUSED declarations.
util.h requires ISC_CONSTRUCTOR definition, which depends on config.h
inclusion. It does not include it from isc/util.h (or any other header).
Using isc/util.h fails hard when isc/util.h is used without including
bind's config.h.
Move the check to c file, where ISC_CONSTRUCTOR is used. Ensure config.h
is included there.
Added tests to ensure that dig won't retry sending a query over tcp
(+tcp) when a TCP connection is closed prematurely (EOF is read) if
either +tries=1 or retry=0 is specified on the command line.
Now that premature EOF on tcp connections take +tries and +retry into
account, the dig system tests handling TCP EOF with +tries=1 were
expecting dig to do a second attempt in handling the tcp query, which
doesn't happen anymore.
To make the test work as expected +tries value was adjusted to 2, to
make it behave as before after the new update on dig.
Before this commit, a premature EOF (connection closed) on tcp queries
was causing dig to automatically attempt to send the query again, even
if +tries=1 or +retries=0 was provided on command line.
This commit fix the problem by taking into account the no. of retries
specified by the user when processing a premature EOF on tcp
connections.
Add kasp.sh to the list of scripts copied from the source directory to
the build directory before any test is run. This will fix
the out-of-tree test failures introduced in commit
ecb073bdd6 on the 'main' branch.
When calling "rndc dnssec -checkds", it may take some milliseconds
before the appropriate changes have been written to the state file.
Add retry_quiet mechanisms to allow the write operation to finish.
Also retry_quiet the check for the next key event. A "rndc dnssec"
command may trigger a zone_rekey event and this will write out
a new "next key event" log line, but it may take a bit longer than
than expected in the tests.
Call 'dns_zone_rekey' after a 'rndc dnssec -checkds' or 'rndc dnssec
-rollover' command is received, because such a command may influence
the next key event. Updating the keys immediately avoids unnecessary
rollover delays.
The kasp system test no longer needs to call 'rndc loadkeys' after
a 'rndc dnssec -checkds' or 'rndc dnssec -rollover' command.
CDS/CDNSKEY DELETE records are only useful if they are signed,
otherwise the parent cannot verify these RRsets anyway. So once the DS
has been removed (and signaled to BIND), we can remove the DNSKEY and
RRSIG records, and at this point we can also remove the CDS/CDNSKEY
records.
Change the 'check_keys' function to try three times. Some intermittent
kasp test failures are because we are inspecting the key files
before the actual change has happen. The 'retry_quiet' approach allows
for a bit more time to let the write operation finish.
This MR introduces a new system test 'keymgr2kasp' to test
migration to 'dnssec-policy'. It moves some existing tests from
the 'kasp' system test to here.
Also a common script 'kasp.sh', to be used in kasp specific tests,
is introduced.
The 'keymgr_key_init()' function initializes key states if they have
not been set previously. It looks at the key timing metadata and
determines using the given times whether a state should be set to
RUMOURED or OMNIPRESENT.
However, the DNSKEY and ZRRSIG states were mixed up: When looking
at the Activate timing metadata we should set the ZRRSIG state, and
when looking at the Published timing metadata we should set the
DNSKEY state.
Add two test zones that migrate to dnssec-policy. Test if the key
states are set accordingly given the timing metadata.
The rumoured.kasp zone has its Publish/Active/SyncPublish times set
not too long ago so the key states should be set to RUMOURED. The
omnipresent.kasp zone has its Publish/Active/SyncPublish times set
long enough to set the key states to OMNIPRESENT.
Slightly change the init_migration_keys function to set the
key lifetime to "none" (legacy keys don't have lifetime). Then in the
test case set the expected key lifetime explicitly.
This commit is somewhat editorial as it does not introduce something
new nor fixes anything.
The layout in keymgr2kasp/tests.sh has been changed, with the
intention to make more clear where a test scenario ends and begins.
The publication time of some ZSKs has been changed. It makes a more
clear distinction between publication time and activation time.
The kasp system test was getting pretty large, and more tests are on
the way. Time to split up. Move tests that are related to migrating
to dnssec-policy to a separate directory 'keymgr2kasp'.
The named-checkzone tool can also be invoked as named-compilezone. Make
sure a man page is installed for that alias. Move and rename the
"man_named-checkzone" label to prevent a Sphinx duplicate label warning
from being raised (see commit 84862e96c1
for more information).
The named-nzd2nzf utility is only built and installed for LMDB-enabled
builds. Adjust the relevant Makefile.am file to make sure the
named-nzd2nzf.1 man page is also only built and installed for
LMDB-enabled builds.
The dnstap-read utility is only built and installed for dnstap-enabled
builds. Adjust the relevant Makefile.am file to make sure the
dnstap-read.1 man page is also only built and installed for
dnstap-enabled builds.
Issue #2575 was merged to 9.16 only as change 5603, but a placeholder
was not added to CHANGES in the main branch. This commit adds the
placeholder and renumbers the two subsequent changes.
Resolve "dig -u is extremely inaccurate, especially on machines with the kernel timer tick set at 100Hz"
Closes#2592
See merge request isc-projects/bind9!4826
The TIME_NOW macro calls isc_time_now which uses CLOCK_REALTIME_COARSE
for getting the current time. This is perfectly fine for millisecond,
however when the user request microsecond resolutiuon, they are going
to get very inaccurate results. This is especially true on a server
class machine where the clock ticks may be set to 100HZ.
This changes dig to use the new TIME_NOW_HIRES macro that uses the
CLOCK_MONOTONIC_RAW that is more expensive, but gets the *actual*
current time rather than the at the last kernel time tick.
The current isc_time_now uses CLOCK_REALTIME_COARSE which only updates
on a timer tick. This clock is generally fine for millisecond accuracy,
but on servers with 100hz clocks, this clock is nowhere near accurate
enough for microsecond accuracy.
This commit adds a new isc_time_now_hires function that uses
CLOCK_REALTIME, which gives the current time, though it is somewhat
expensive to call. When microsecond accuracy is required, it may be
required to use extra resources for higher accuracy.
In CMocka versions << 1.1.3, the skip() function would cause the whole
unit test to abort when CMOCKA_TEST_ABORT is set. As this is problem
only in Debian 9 Stretch and Ubuntu 16.04 Xenial, we just require the
CMocka >= 1.1.3 and disable the unit testing on Debian 9 Stretch until
we can pull the libcmocka-dev from stretch-backports and remove the
Ubuntu 16.04 Xenial from the CI as it is reaching End of Standard
Support at the end of April 2021.
When NETMGR_TRACE(_VERBOSE) is enabled, the build would fail on some
non-Linux non-glibc platforms because:
* Use <stdint.h> print macros because uint_fast32_t is not always
unsigned long
* The header <execinfo.h> is not available on non-glibc, thus commit
adds dummy backtrace() and backtrace_symbols_fd() functions for
platforms without HAVE_BACKTRACE
The netmgr unit tests were designed to push the system limits to maximum
by sending as many queries as possible in the busy loop from multiple
threads. This mostly works with UDP, but in the stateful protocol where
establishing the connection takes more time, it failed quite often in
the CI. On FreeBSD, this happened more often, because the socket() call
would fail spuriosly making the problem even worse.
This commit does several things to improve reliability:
* return value of isc_nm_<proto>connect() is always checked and retried
when scheduling the connection fails
* The busy while loop has been slowed down with usleep(1000); so the
netmgr threads could schedule the work and get executed.
* The isc_thread_yield() was replaced with usleep(1000); also to allow
the other threads to do any work.
* Instead of waiting on just one variable, we wait for multiple
variables to reach the final value
* We are wrapping the netmgr operations (connects, reads, writes,
accepts) with reference counting and waiting for all the callbacks to
be accounted for.
This has two effects:
a) the isc_nm_t is always clean of active sockets and handles when
destroyed, so it will prevent the spurious INSIST(references == 1)
from isc_nm_destroy()
b) the unit test now ensures that all the callbacks are always called
when they should be called, so any stuck test means that there was
a missing callback call and it is always a real bug
These changes allows us to remove the workaround that would not run
certain tests on systems without port load-balancing.
In tls_error(), we now call isc__nm_tlsdns_failed_read() instead of just
stopping timer and reading from the socket. This allows us to properly
cleanup any pending operation on the socket.
When shutting down, calling the isc__nm_failed_connect_cb() was delayed
until the connect callback would be called. It turned out that the
connect callback might not get called at all when the socket is being
shut down. Call the failed_connect_cb() directly in the
tlsdns_shutdown() instead of waiting for the connect callback to call it.
After a partial write the tls.senddata buffer would be rearranged to
contain only the data tha wasn't sent and the len part would be made
shorter, which would lead to attempt to free only part of a socket's
tls.senddata buffer.
The tlsdns_cycle() might call uv_write() to write data to the socket,
when this happens and the socket is shutdown before the callback
completes, the uvreq structure was not freed because the callback would
be called with non-zero status code.
The RFC7828 specifies the keepalive interval to be 16-bit, specified in
units of 100 milliseconds and the configuration options tcp-*-timeouts
are following the suit. The units of 100 milliseconds are very
unintuitive and while we can't change the configuration and presentation
format, we should not follow this weird unit in the API.
This commit changes the isc_nm_(get|set)timeouts() functions to work
with milliseconds and convert the values to milliseconds before passing
them to the function, not just internally.
The udp, tcpdns and tlsdns contained lot of cut&paste code or code that
was very similar making the stack harder to maintain as any change to
one would have to be copied to the the other protocols.
In this commit, we merge the common parts into the common functions
under isc__nm_<foo> namespace and just keep the little differences based
on the socket type.
After the TCPDNS refactoring the initial and idle timers were broken and
only the tcp-initial-timeout was always applied on the whole TCP
connection.
This broke any TCP connection that took longer than tcp-initial-timeout,
most often this would affect large zone AXFRs.
This commit changes the timeout logic in this way:
* On TCP connection accept the tcp-initial-timeout is applied
and the timer is started
* When we are processing and/or sending any DNS message the timer is
stopped
* When we stop processing all DNS messages, the tcp-idle-timeout
is applied and the timer is started again
The system tests were missing a test that would test tcp-initial-timeout
and tcp-idle-timeout.
This commit adds new "timeouts" system test that adds:
* Test that waits longer than tcp-initial-timeout and then checks
whether the socket was closed
* Test that sends and receives DNS message then waits longer than
tcp-initial-timeout but shorter time than tcp-idle-timeout than
sends DNS message again than waits longer than tcp-idle-timeout
and checks whether the socket was closed
* Similar test, but bursting 25 DNS messages than waiting longer than
tcp-initial-timeout and shorter than tcp-idle-timeout than do second
25 DNS message burst
* Check whether transfer longer than tcp-initial-timeout succeeds
Add a test for freezing, manually updating, and then thawing a dynamic
zone with "dnssec-policy". In the kasp system test we add parameters
to the "update_is_signed" check to signal the indicated IP addresses
for the labels "a" and "d". If set to '-', the test is skipped.
After nsupdating the dynamic.kasp zone, we revert the update (with
nsupdate) and update the zone again, but now with the freeze/thaw
approach.
Dynamic zones with dnssec-policy could not be thawed because KASP
zones were considered always dynamic. But a dynamic KASP zone should
also check whether updates are disabled.
This commit fixes loading the certificate chain files so that the full
chain could be sent to the clients which require that for
verification. Before that fix only the top most certificate would be
loaded from the chain and sent to clients preventing some of them to
perform certificate validation (e.g. Windows 10 DoH client).
The transport should also be detached when we skip a master, otherwise
named will crash when sending a SOA query to the next master over TLS,
because the transport must be NULL when we enter
'dns_view_gettransport'.
When we query the resolver for a domain name that is in the same zone
for which is already one or more fetches outstanding, we could
potentially hit the fetch limits. If so, recursion fails immediately
for the incoming query and if serve-stale is enabled, we may try to
return a stale answer.
If the resolver is also is authoritative for the parent zone (for
example the root zone), first a delegation is found, but we first
check the cache for a better response.
Nothing is found in the cache, so we try to recurse to find the
answer to the query.
Because of fetch-limits 'dns_resolver_createfetch()' returns an error,
which 'ns_query_recurse()' propagates to the caller,
'query_delegation_recurse()'.
Because serve-stale is enabled, 'query_usestale()' is called,
setting 'qctx->db' to the cache db, but leaving 'qctx->version'
untouched. Now 'query_lookup()' is called to search for stale data
in the cache database with a non-NULL 'qctx->version'
(which is set to a zone db version), and thus we hit an assertion
in rbtdb.
This crash was introduced in 'main' by commit
8bcd7fe69e.
Commit 9fb6d11abb (which converted BIND 9
documentation from DocBook to Sphinx) inadvertently removed a paragraph
from the description of the "max-ixfr-ratio" option. Add the missing
paragraph back.
Unfortunately, it's not possible to disable Pull Requests on the
mirrored repository on the GitHub, so this commit adds external action
that closes any new open Issue or Pull Requests instead letting them rot
unnoticed.
- use a value less than 2^32 for DNS_ZONEFLG_FIXJOURNAL; a larger value
could cause problems in some build environments. the zone flag
DNS_ZONEFLG_DIFFONRELOAD, which was no longer in use, has now been
deleted and its value reused for _FIXJOURNAL.
*** CID 329157: Null pointer dereferences (REVERSE_INULL)
/lib/dns/journal.c: 754 in journal_open()
748 j->header.index_size * sizeof(journal_rawpos_t));
749 }
750 if (j->index != NULL) {
751 isc_mem_put(j->mctx, j->index,
752 j->header.index_size * sizeof(journal_pos_t));
753 }
CID 329157: Null pointer dereferences (REVERSE_INULL)
Null-checking "j->filename" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
754 if (j->filename != NULL) {
755 isc_mem_free(j->mctx, j->filename);
756 }
757 if (j->fp != NULL) {
758 (void)isc_stdio_close(j->fp);
759 }
- rename dot to doth, as it now covers both dot and doh.
- merge xot into doth as it's closely related.
- added long-lived key and cert files (expiring 2121).
- add tests with https-get, https-post, http-plain, alternate
endpoints, and both static and ephemeral TLS configuration.
- incidentally fixed a memory leak in dig that occurred if +https
was specified more than once.
It is advisable to disable Nagle's algorithm for HTTP/2 connections
because multiple HTTP/2 streams could be multiplexed over one
transport connection. Thus, delays when delivering small packets could
bring down performance for the whole session. HTTP/2 is meant to be
used this way.
when called from within the context of a network thread,
isc_nm_tlsconnect() hangs. it is waiting for the socket's
result code to be updated, but that update is supposed to happen
asynchronously in the network thread, and if we're already blocking
in the network thread, it can never occur.
we can kluge around this by setting the socket result code
early; this works for most clients (including "dig"), but it causes
inconsistent behaviors that manifest as test failures in the DoH unit
test.
so we kluged around it even more by setting the socket result code
early *only when running in the network thread*. we need a better
solution for this problem, but this will do for now.
This commit makes the server-side code polite.
It fixes the error handling code on the server side and fixes
returning error code in responses (there was a nasty bug which could
potentially crash the server).
Also, in this commit we limit max size POST request data to 96K, max
processed data size in headers to 128K (should be enough to handle any
GET requests).
If these limits are surpassed, server will terminate the request with
RST_STREAM without responding with error code. Otherwise it politely
responds with error code.
This commit also limits number of concurrent HTTP/2 streams per
transport connection on server to 100 (as nghttp2 advises by default).
Ideally, these parameters should be configurable both globally and per
every HTTP endpoint description in the configuration file, but for now
putting sane limits should be enough.
- style, cleanup, and removal of unnecessary code.
- combined isc_nm_http_add_endpoint() and isc_nm_http_add_doh_endpoint()
into one function, renamed isc_http_endpoint().
- moved isc_nm_http_connect_send_request() into doh_test.c as a helper
function; remove it from the public API.
- renamed isc_http2 and isc_nm_http2 types and functions to just isc_http
and isc_nm_http, for consistency with other existing names.
- shortened a number of long names.
- the caller is now responsible for determining the peer address.
in isc_nm_httpconnect(); this eliminates the need to parse the URI
and the dependency on an external resolver.
- the caller is also now responsible for creating the SSL client context,
for consistency with isc_nm_tlsdnsconnect().
- added setter functions for HTTP/2 ALPN. instead of setting up ALPN in
isc_tlsctx_createclient(), we now have a function
isc_tlsctx_enable_http2client_alpn() that can be run from
isc_nm_httpconnect().
- refactored isc_nm_httprequest() into separate read and send functions.
isc_nm_send() or isc_nm_read() is called on an http socket, it will
be stored until a corresponding isc_nm_read() or _send() arrives; when
we have both halves of the pair the HTTP request will be initiated.
- isc_nm_httprequest() is renamed isc__nm_http_request() for use as an
internal helper function by the DoH unit test. (eventually doh_test
should be rewritten to use read and send, and this function should
be removed.)
- added implementations of isc__nm_tls_settimeout() and
isc__nm_http_settimeout().
- increased NGHTTP2 header block length for client connections to 128K.
- use isc_mem_t for internal memory allocations inside nghttp2, to
help track memory leaks.
- send "Cache-Control" header in requests and responses. (note:
currently we try to bypass HTTP caching proxies, but ideally we should
interact with them: https://tools.ietf.org/html/rfc8484#section-5.1)
Simple typecast to size_t should be enough to silence the warning on
ARMv7, even though the code is in fact correct, because the readlen is
checked for being < 0 in the block before the warning.
The C standard actually doesn't define char as signed or unsigned, and
it could be either according to underlying architecture. It turns out
that while it's usually signed type, it isn't on arm64 where it's
unsigned.
isc_commandline_parse() return int, just use that instead of the char.
tests that version 1 journal files containing version 1 transaction
headers are rolled forward correctly on server startup, then updated
into version 2 journals. also checks journal file consistency and
'max-journal-size' behavior.
'named-journalprint -x' now prints the journal's index table and
the offset of each transaction in the journal, so that index consistency
can be confirmed.
when the 'max-ixfr-ratio' option was added, journal transaction
headers were revised to include a count of RR's in each transaction.
this made it impossible to read old journal files after an upgrade.
this branch restores the ability to read version 1 transaction
headers. when rolling forward, printing journal contents, if
the wrong transaction header format is found, we can switch.
when dns_journal_rollforward() detects a version 1 transaction
header, it returns DNS_R_RECOVERABLE. this triggers zone_postload()
to force a rewrite of the journal file in the new format, and
also to schedule a dump of the zone database with minimal delay.
journal repair is done by dns_journal_compact(), which rewrites
the entire journal, ignoring 'max-journal-size'. journal size is
corrected later.
newly created journal files now have "BIND LOG V9.2" in their headers
instead of "BIND LOG V9". files with the new version string cannot be
read using the old transaction header format. note that this means
newly created journal files will be rejected by older versions of named.
named-journalprint now takes a "-x" option, causing it to print
transaction header information before each delta, including its
format version.
Call the libisc isc__initialize() constructor and isc__shutdown()
destructor from DllMain instead of having duplicate code between
those and DllMain() code.
When AddressSanitizer is in use, disable the internal mempool
implementation and redirect the isc_mempool_get to isc_mem_get
(and similarly for isc_mempool_put). This is the method recommended
by the AddressSanitizer authors for tracking allocations and
deallocations instead of custom poison/unpoison code (see
https://github.com/google/sanitizers/wiki/AddressSanitizerManualPoisoning).
The pthread_self(), thrd_current() or GetCurrentThreadId() could
actually be a pointer, so we should rather convert the value into
uintptr_t instead of unsigned long.
Convert the isc_hp API to use the globally available isc_tid_v instead
of locally defined tid_v. This should solve most of the problems on
machines with many number of cores / CPUs.
The current isc_hp API uses internal tid_v variable that gets
incremented for each new thread using hazard pointers. This tid_v
variable is then used as a index to global shared table with hazard
pointers state. Since the tid_v is only incremented and never
decremented the table could overflow very quickly if we create set of
threads for short period of time, they finish the work and cease to
exist. Then we create identical set of threads and so on and so on.
This is not a problem for a normal `named` operation as the set of
threads is stable, but the problematic place are the unit tests where we
test network manager or other APIs (task, timer) that create threads.
This commits adds a thin wrapper around any function called from
isc_thread_create() that adds unique-but-reusable small digit thread id
that can be used as index to f.e. hazard pointer tables. The trampoline
wrapper ensures that the thread ids will be reused, so the highest
thread_id number doesn't grow indefinitely when threads are created and
destroyed and then created again. This fixes the hazard pointer table
overflow on machines with many cores. [GL #2396]
When a staleonly lookup doesn't find a satisfying answer, it should
not try to respond to the client.
This is not true when the initial lookup is staleonly (that is when
'stale-answer-client-timeout' is set to 0), because no resolver fetch
has been created at this point. In this case continue with the lookup
normally.
Fix a crash that can happen in the following scenario:
A client request is received. There is no data for it in the cache,
(not even stale data). A resolver fetch is created as part of
recursion.
Some time later, the fetch still hasn't completed, and
stale-answer-client-timeout is triggered. A staleonly lookup is
started. It will also find no data in the cache.
So 'query_lookup()' will call 'query_gotanswer()' with ISC_R_NOTFOUND,
so this will call 'query_notfound()' and this will start recursion.
We will eventually end up in 'ns_query_recurse()' and that requires
the client query fetch to be NULL:
REQUIRE(client->query.fetch == NULL);
If the previously started fetch is still running this assertion
fails.
The crash is easily prevented by not requiring recursion for
staleonly lookups.
Also remove a redundant setting of the staleonly flag at the end of
'query_lookup_staleonly()' before destroying the query context.
Add a system test to catch this case.
When applying dnssec-policy on a dynamic zone (e.g. that allows Dynamic
Updates), the NSEC3 parameters were put on the queue, but they were
not being processed (until a reload of the zone or reconfiguration).
Process the NSEC3PARAM queue on zone postload when handling a
dynamic zone.
The BIND 9 libraries on Windows define DllMain() optional entry point
into a dynamic-link library (DLL). When the system starts or terminates
a process or thread, it calls the entry-point function for each loaded
DLL using the first thread of the process.
When the DLL is being loaded into the virtual address space of the
current process as a result of the process starting up, we make a call
to DisableThreadLibraryCalls() which should disable the
DLL_THREAD_ATTACH and DLL_THREAD_DETACH notifications for the specified
dynamic-link library (DLL).
This seems not be the case because we never check the return value of
the DisableThreadLibraryCalls() call, and it could in fact fail. The
DisableThreadLibraryCalls() function fails if the DLL specified by
hModule has active static thread local storage, or if hModule is an
invalid module handle.
In this commit, we remove the safe-guard assertion put in place for the
DLL_THREAD_ATTACH and DLL_THREAD_DETACH events and we just ignore them.
BIND 9 doesn't create/destroy enough threads for it actually to make any
difference, and in fact we do use static thread local storage in the
code.
The 'checknames' field wasn't initialized in dns_view_create(), but it
should otherwise AddressSanitizer identifies the following runtime error
in query_test.c.
runtime error: load of value 190, which is not a valid value for type '_Bool'
'checknames' field of struct dns_view is not initialized by
dns_view_create(). ASAN identified this as runtime error:
runtime error: load of value 190, which is not a valid value for type '_Bool'
Removing stderr from the pict tool serves no purpose and drops valuable
information, we might use when debugging failed pairwise CI job, such
as:
Input Error: A parameter names must be unique
*** CID 320481: Null pointer dereferences (REVERSE_INULL)
/bin/tests/wire_test.c: 261 in main()
255 process_message(input);
256 }
257 } else {
258 process_message(input);
259 }
260
CID 320481: Null pointer dereferences (REVERSE_INULL)
Null-checking "input" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
261 if (input != NULL) {
262 isc_buffer_free(&input);
263 }
264
265 if (printmemstats) {
266 isc_mem_stats(mctx, stdout);
remove redundant 'inst != NULL' test
162cleanup:
CID 281450 (#1 of 1): Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking inst suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
163 if (result != ISC_R_SUCCESS && inst != NULL) {
164 plugin_destroy((void **)&inst);
165 }
Removed redundant 'listener != NULL' check.
1191cleanup:
CID 304936 (#1 of 1): Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking listener suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
1192 if (listener != NULL) {
1193 isc_refcount_decrement(&listener->refs);
1194 listener->exiting = true;
1195 free_listener(listener);
1196 }
Two minor fixes in the kasp system test:
1. A wrong comment in ns3/setup.sh (we are subtracting 2 hours, not
adding them).
2. 'get_keyids' used bad parameters "$1" "$2" when 'check_numkeys'
failed. Also, 'check_numkeys' can use $DIR, $ZONE, and $NUMKEYS
directly, no need to pass them.
Add some more zones to the kasp system test to test the 'purge-keys'
option. Three zones test that the predecessor key files are removed
after the purge keys interval, one test checks that the key files
are retained if 'purge-keys' is disabled. For that, we change the
times to 90 days in the past (the default value for 'purge-keys').
On each keymgr run, we now also check if key files can be removed.
The 'purge-keys' interval determines how long keys should be retained
after they have become completely hidden.
Key files should not be removed if it has a state that is set to
something else then HIDDEN, if purge-keys is 0 (disabled), if
the key goal is set to OMNIPRESENT, or if the key is unused (a key is
unused if no timing metadata set, and no states are set or if set,
they are set to HIDDEN).
If the last changed timing metadata plus the purge-keys interval is
in the past, the key files may be removed.
Add a dst_key_t variable 'purge' to signal that the key file should
not be written to file again.
Add a new option 'purge-keys' to 'dnssec-policy' that will purge key
files for deleted keys. The option determines how long key files
should be retained prior to removing the corresponding files from
disk.
If set to 0, the option is disabled and 'named' will not remove key
files from disk.
dns_dt_open() is not currently called with mode dns_dtmode_unix.
*** CID 281489: Resource leaks (RESOURCE_LEAK)
/lib/dns/dnstap.c: 983 in dns_dt_open()
977
978 if (!dnstap_file(handle->reader)) {
979 CHECK(DNS_R_BADDNSTAP);
980 }
981 break;
982 case dns_dtmode_unix:
CID 281489: Resource leaks (RESOURCE_LEAK)
Variable "handle" going out of scope leaks the storage it points to.
983 return (ISC_R_NOTIMPLEMENTED);
984 default:
985 INSIST(0);
986 ISC_UNREACHABLE();
987 }
988
The addition of lib/isc/tls_p.h to the source tree was not accounted for
in the relevant variable in lib/isc/Makefile.am and thus the former file
is not being included in release tarballs prepared using "make dist".
Fix by tweaking the libisc_la_SOURCES list in lib/isc/Makefile.am
accordingly.
The build-time requirement for libtool was introduced inadvertently:
1. Commit 1628f5865a added a check to
configure.ac which claims to test whether the libtool script is
available. There are two problems with that check:
- it is effectively a no-op as the AC_PROG_LIBTOOL() macro always
sets the LIBTOOL variable [1],
- this check was intended to be performed before autoreconf is
run, not when ./configure is run; the libtool script is supposed
to be dynamically generated by ./configure on the build host and
thus there is no need for a standalone libtool script to be
installed system-wide on every host attempting to build BIND 9
e.g. from a tarball produced by "make dist".
2. Commit a7982d14dd was based on the
incorrect assumption that the AC_PROG_LIBTOOL() macro looks for the
libtool binary in PATH and sets the LIBTOOL variable accordingly,
which is what other AC_PROG_*() macros do. Meanwhile, the
AC_PROG_LIBTOOL() macro only initializes libtool for use with
Automake. It is not necessary for a standalone libtool script to be
available in PATH on the build host when ./configure is run.
Do not look for libtool in PATH at build time as it prevents hosts
without a libtool script available system-wide from building BIND 9 from
source tarballs prepared using "make dist". Note that libtool m4
macros, utilities, etc. still need to be present on a given host if
autoreconf is to be run on it.
[1] https://git.savannah.gnu.org/cgit/libtool.git/tree/m4/libtool.m4?h=v2.4.6#n89
Instead of calling isc_tls_initialize()/isc_tls_destroy() explicitly use
gcc/clang attributes on POSIX and DLLMain on Windows to initialize and
shutdown OpenSSL library.
This resolves the issue when isc_nm_create() / isc_nm_destroy() was
called multiple times and it would call OpenSSL library destructors from
isc_nm_destroy().
At the same time, since we now have introduced the ctor/dtor for libisc,
this commit moves the isc_mem API initialization (the list of the
contexts) and changes the isc_mem_checkdestroyed() to schedule the
checking of memory context on library unload instead of executing the
code immediately.
Disables the DLL_THREAD_ATTACH and DLL_THREAD_DETACH notifications for
the specified dynamic-link library (DLL). This can reduce the size of
the working set for some applications.
Although harmless, the memmove() in tlsdns and tcpdns was guarded by a
current message length variable that was always bigger than 0 instead of
correct current buffer length remainder variable.
Since we now require both libcrypto and libssl to be initialized for
netmgr, we move all the OpenSSL initialization code except the engine
initialization to isc_tls API.
The isc_tls_initialize() and isc_tls_destroy() has been made idempotent,
so they could be called multiple time. However when isc_tls_destroy()
has been called, the isc_tls_initialize() could not be called again.
The ISC_MEM_CHECKOVERRUN would add canary byte at the end of every
allocations and check whether the canary byte hasn't been changed at the
free time. The AddressSanitizer and valgrind memory checks surpases
simple checks like this, so there's no need to actually keep the code
inside the allocator.
Previously, the mem_{get,put} benchmark would pass the allocation size
as thread_create argument. This has been now changed, so the allocation
size is stored and decremented (divided) in atomic variable and the
thread create routing is given a memory context. This will allow to
write tests where each thread is given different memory context and do
the same for mempool benchmarking.
The two memory debugging features: ISC_MEM_DEFAULTFILL
(ISC_MEMFLAG_FILL) and ISC_MEM_TRACKLINES were always enabled in all
builds and the former was only disabled in `named`.
This commits disables those two features in non-developer build to make
the memory allocator significantly faster.
This is yet another step into unlocking some parts of the memory
contexts. All the regularly updated variables has been turned into
atomic types, so we can later remove the locks when updating various
counters.
Also unlock as much code as possible without breaking anything.
On 24-core machine, the tests would crash because we would run out of
the hazard pointers. We now adjust the number of hazard pointers to be
in the <128,256> interval based on the number of available cores.
Note: This is just a band-aid and needs a proper fix.
Previously, the applications using libisc would be able to override the
internal memory methods with own implementation. This was no longer
possible, but the extra level of indirection was not removed. This
commit removes the extra level of indirection for the memory methods and
the default_memalloc() and default_memfree().
The internal memory allocator had an extra code to keep a list of blocks
for small size allocation. This would help to reduce the interactions
with the system malloc as the memory would be already allocated from the
system, but there's an extra cost associated with that - all the
allocations/deallocations must be locked, effectively eliminating any
optimizations in the system allocator targeted at multi-threaded
applications. While the isc_mem API is still using locks pretty heavily,
this is a first step into reducing the memory allocation/deallocation
contention.
feature-test tool location needs to be determined by its associated
variable; otherwise, the tool is not found on Windows:
setup.sh: line 22: ../feature-test: No such file or directory
Any CI job:
- I:dnssec:file dnssec/ns1/trusted.keys not removed
- I:rpzrecurse:file rpzrecurse/ns3/named.run.prev not removed
system:clang:freebsd11:amd64:
- I:tkey:file tkey/ns1/named.conf-e not removed
system:gcc:sid:amd64:
- I:mirror:file mirror/ns3/_default.nzf not removed
system:gcc:xenial:amd64:
- I:rpzextra:file rpzextra/.cache/v/cache/lastfailed not removed
- I:rpzrecurse:file rpzrecurse/ns3/named.run.prev not removed
- I:shutdown:file shutdown/.cache/v/cache/lastfailed not removed
Run this check only when in Git repository, because the run.sh function
which produces the "file not removed" warning is run only when build
directory is the same as the source directory, that is only for in-tree
builds.
In DNS Flag Day 2020, the development branch started setting the
IP_DONTFRAG option on the UDP sockets. It turned out, that this
code was incomplete leading to dropping the outgoing UDP packets.
Henceforth this commit rolls back this setting until we have a
proper fix that would send back empty response with TC flag set.
updated the parser to allow the "port", "tls" and "http"
paramters to "listen-on" and "listen-on-v6" to be specified in any
order. previously the parser would throw an error if any other order
was used than port, tls, http.
unencrypted DoH connections may be used in some operational
environments where encryption is handled by a reverse proxy,
but it's going to be relatively rare, so we shouldn't make it
easy to do by mistake. this commit changes the syntax for
listen-on and listen-on-v6 so that if "http" is specified, "tls"
must also be specified; for unencrypted listeners, "tls none"
can be used.
As libssl depends on libcrypto, -lssl needs to precede -lcrypto in
linker invocations or else the build will fail with static OpenSSL
libraries. Adjust m4/ax_check_openssl.m4 to prevent this issue from
getting triggered when pkg-config files for OpenSSL are not available.
Commit fa505bfb0e omitted two unit tests
while introducing the SKIP_TEST_EXIT_CODE preprocessor macro. Fix the
outliers to make use of SKIP_TEST_EXIT_CODE consistent across all unit
tests. Also make sure lib/dns/tests/dnstap_test returns an exit code
that indicates a skipped test when dnstap is not enabled.
The only reason for including the gssapi.h from the dst/gssapi.h header
was to get the typedefs of gss_cred_id_t and gss_ctx_id_t. Instead of
using those types directly this commit introduces dns_gss_cred_id_t and
dns_gss_ctx_id_t types that are being used in the public API and
privately retyped to their counterparts when we actually call the gss
api.
This also conceals the gssapi headers, so users of the libdns library
doesn't have to add GSSAPI_CFLAGS to the Makefile when including libdns
dst API.
The lmdb.h doesn't have to be included from the dns/view.h header as it
is separately included where used. This stops exposing the inclusion of
lmdb.h from the libdns headers.
The <isc/readline.h> header provided a compatibility shim to use when
other non-GNU readline libraries are in use. The two places where
readline library is being used is nslookup and nsupdate, so the header
file has been moved to bin/dig directory and it's directly included from
bin/nsupdate.
This also conceals any readline headers exposed from the libisc headers.
The extra library CFLAGS were causing the headers to be included in
wrong order possibly pulling header files from previously installed
BIND 9 version.
This commit cleans up the extra <foo>_CFLAGS from the includes in favor
of not exposing 3rd party headers in our own header files.
This commit fix a leak which was happening every time an inline-signed
zone was added to the configuration, followed by a rndc reconfig.
During the reconfig process, the secure version of every inline-signed
zone was "moved" to a new view upon a reconfig and it "took the raw
version along", but only once the secure version was freed (at shutdown)
was prev_view for the raw version detached from, causing the old view to
be released as well.
This caused dangling references to be kept for the previous view, thus
keeping all resources used by that view in memory.
Descriptions of UNTESTED and SKIPPED system test results are very
similar to one another and it may be confusing when to pick one and
when the other. Merging these two system test results removes the
confusion and also makes system test more aligned with Automake,
which does not know about UNTESTED test result.
When system test execution was ported to Automake, SKIPPED and UNTESTED
system test result were not made to match Automake expectations,
therefore a skipped test is recorded by Automake as "PASS":
$ make check TESTS=cpu V=1
I:cpu:cpu test only runs on Linux, skipping test
I:cpu:Prerequisites missing, skipping test.
R:cpu:SKIPPED
E:cpu:2020-12-16T11:36:58+0000
PASS: cpu
====================================================================
Testsuite summary for BIND 9.17.7
====================================================================
# TOTAL: 1
# PASS: 1
For a test to be recorded by Automake as skipped, the test, or it's test
driver, needs to exit with code 77:
$ make check TESTS=cpu V=1
I:cpu:cpu test only runs on Linux, skipping test
I:cpu:Prerequisites missing, skipping test.
R:cpu:SKIPPED
E:cpu:2020-12-16T11:39:10+0000
SKIP: cpu
====================================================================
Testsuite summary for BIND 9.17.7
====================================================================
# TOTAL: 1
# PASS: 0
# SKIP: 1
The strlcat() call was wrong.
*** CID 316608: Memory - corruptions (OVERRUN)
/lib/dns/resolver.c: 5017 in fctx_create()
5011 * Make fctx->info point to a copy of a formatted string
5012 * "name/type".
5013 */
5014 dns_name_format(name, buf, sizeof(buf));
5015 dns_rdatatype_format(type, typebuf, sizeof(typebuf));
5016 p = strlcat(buf, "/", sizeof(buf));
>>> CID 316608: Memory - corruptions (OVERRUN)
>>> Calling "strlcat" with "buf + p" and "1036UL" is suspicious because "buf" points into a buffer of 1036 bytes and the function call may access "(char *)(buf + p) + 1035UL". [Note: The source code implementation of the function has been overridden by a builtin model.]
5017 strlcat(buf + p, typebuf, sizeof(buf));
5018 fctx->info = isc_mem_strdup(mctx, buf);
5019
5020 FCTXTRACE("create");
5021 dns_name_init(&fctx->name, NULL);
5022 dns_name_dup(name, mctx, &fctx->name);
The --enable-option-checking=fatal option prevents ./configure from
proceeding when an unknown option is used in the ./configure step in CI.
This change will avoid adding unsupported ./configure options or options
with typo or typo in pairwise testing "# [pairwise: ...]" marker.
As we generate manual pages from reStructuredText sources, we don't have
absolute control on manual page output and therefore 'mandoc -Tlint' may
always report warnings we can't eliminate. In light of this some mandoc
warnings need to be ignored.
Man pages are currently only generated from reStructuredText sources
when "make man" is run in the doc/man/ directory. Tweak
doc/man/Makefile.am so that running "make doc" in the top-level
directory also causes man pages to be generated, so that all potential
documentation building problems can be detected by a single make
invocation.
Coverity assumes that the memory holding any value read using byte
swapping is tainted. As we store the NSEC3PARAM records in wire
form and iterations is byte swapped the memory holding the record
is marked as tainted. nsec3->salt_length is marked as tainted
transitively. To remove the taint the value need to be range checked.
For a correctly formatted record region.length should match
nsec3->salt_length and provides a convenient value to check the field
against.
*** CID 316507: Insecure data handling (TAINTED_SCALAR)
/lib/dns/rdata/generic/nsec3param_51.c: 241 in tostruct_nsec3param()
235 region.length = rdata->length;
236 nsec3param->hash = uint8_consume_fromregion(®ion);
237 nsec3param->flags = uint8_consume_fromregion(®ion);
238 nsec3param->iterations = uint16_consume_fromregion(®ion);
239
240 nsec3param->salt_length = uint8_consume_fromregion(®ion);
>>> CID 316507: Insecure data handling (TAINTED_SCALAR)
>>> Passing tainted expression "nsec3param->salt_length" to "mem_maybedup", which uses it as an offset.
241 nsec3param->salt = mem_maybedup(mctx, region.base,
242 nsec3param->salt_length);
243 if (nsec3param->salt == NULL) {
244 return (ISC_R_NOMEMORY);
245 }
246 isc_region_consume(®ion, nsec3param->salt_length);
Coverity assumes that the memory holding any value read using byte
swapping is tainted. As we store the NSEC3 records in wire form
and iterations is byte swapped the memory holding the record is
marked as tainted. nsec3->salt_length and nsec3->next_length are
marked as tainted transitively. To remove the taint the values need
to be range checked. Valid values for these should never exceed
region.length so that is becomes a reasonable value to check against.
*** CID 316509: (TAINTED_SCALAR)
/lib/dns/rdata/generic/nsec3_50.c: 312 in tostruct_nsec3()
306 if (nsec3->salt == NULL) {
307 return (ISC_R_NOMEMORY);
308 }
309 isc_region_consume(®ion, nsec3->salt_length);
310
311 nsec3->next_length = uint8_consume_fromregion(®ion);
>>> CID 316509: (TAINTED_SCALAR)
>>> Passing tainted expression "nsec3->next_length" to "mem_maybedup", which uses it as an offset.
312 nsec3->next = mem_maybedup(mctx, region.base, nsec3->next_length);
313 if (nsec3->next == NULL) {
314 goto cleanup;
315 }
316 isc_region_consume(®ion, nsec3->next_length);
317
/lib/dns/rdata/generic/nsec3_50.c: 305 in tostruct_nsec3()
299 region.length = rdata->length;
300 nsec3->hash = uint8_consume_fromregion(®ion);
301 nsec3->flags = uint8_consume_fromregion(®ion);
302 nsec3->iterations = uint16_consume_fromregion(®ion);
303
304 nsec3->salt_length = uint8_consume_fromregion(®ion);
>>> CID 316509: (TAINTED_SCALAR)
>>> Passing tainted expression "nsec3->salt_length" to "mem_maybedup", which uses it as an offset.
305 nsec3->salt = mem_maybedup(mctx, region.base, nsec3->salt_length);
306 if (nsec3->salt == NULL) {
307 return (ISC_R_NOMEMORY);
308 }
309 isc_region_consume(®ion, nsec3->salt_length);
310
If an invalid key name (e.g. "a..b") in a primaries list in named.conf
is specified the wrong size is passed to isc_mem_put resulting in the
returned memory being put on the wrong freed list.
*** CID 316784: Incorrect expression (SIZEOF_MISMATCH)
/bin/named/config.c: 636 in named_config_getname()
630 isc_buffer_constinit(&b, objstr, strlen(objstr));
631 isc_buffer_add(&b, strlen(objstr));
632 dns_fixedname_init(&fname);
633 result = dns_name_fromtext(dns_fixedname_name(&fname), &b, dns_rootname,
634 0, NULL);
635 if (result != ISC_R_SUCCESS) {
CID 316784: Incorrect expression (SIZEOF_MISMATCH)
Passing argument "*namep" of type "dns_name_t *" and argument "8UL /* sizeof (*namep) */" to function "isc__mem_put" is suspicious.
636 isc_mem_put(mctx, *namep, sizeof(*namep));
637 *namep = NULL;
638 return (result);
639 }
640 dns_name_dup(dns_fixedname_name(&fname), mctx, *namep);
641
Test for Ed25519 and Ed448. If both algorithms are not supported, skip
test. If only one algorithm is supported, run test, skip the
unsupported algorithm. If both are supported, run test normally.
Create new ns3. This will test Ed448 specifically, while now ns2 only
tests Ed25519. This moves some files from ns2/ to ns3/.
The number of queries to use in the burst can be reduced, as we have
a very low fetch limit of 1.
The dig command in 'wait_for_fetchlimits()' should time out sooner as
we expect a SERVFAIL to be returned promptly.
Enabling serve-stale can be done before hitting fetch-limits. This
reduces the chance that the resolver queries time out and fetch count
is reset. The chance of that happening is already slim because
'resolver-query-timeout' is 10 seconds, but better to first let the
data become stale rather than doing that while attempting to resolve.
The 'query_usestale()' function was only called when in
'query_gotanswer()' and an unexpected error occurred. This may have
been "quota reached", and thus we were in some cases returning
stale data on fetch-limits (and if serve-stale enabled of course).
But we can also hit fetch-limits when recursing because we are
following a referral (in 'query_notfound()' and
'query_delegation_recurse()'). Here we should also check for using
stale data in case an error occurred.
Specifically don't check for using stale data when refetching a
zero TTL RRset from cache.
Move the setting of DNS_DBFIND_STALESTART into the 'query_usestale()'
function to avoid code duplication.
Three small cleanups:
1. Remove an unused keystr/dst_key_format.
2. Initialize a dst_key_state_t state with NA.
3. Update false comment about local policy (local policy only adds
barrier on transitions to the RUMOURED state, not the UNRETENTIVE
state).
There was a bug in function 'keymgr_ds_hidden_or_chained()'.
The funcion 'keymgr_ds_hidden_or_chained()' implements (3e) of rule2
as defined in the "Flexible and Robust Key Rollover" paper. The rules
says: All DS records need to be in the HIDDEN state, or if it is not
there must be a key with its DNSKEY and KRRSIG in OMNIPRESENT, and
its DS in the same state as the key in question. In human langauge,
if all keys have their DS in HIDDEN state you can do what you want,
but if a DS record is available to some validators, there must be
a chain of trust for it.
Note that the barriers on transitions first check if the current
state is valid, and then if the next state is valid too. But
here we falsely updated the 'dnskey_omnipresent' (now 'dnskey_chained')
with the next state. The next state applies to 'key' not to the state
to be checked. Updating the state here leads to (true) always, because
the key that will move its state will match the falsely updated
expected state. This could lead to the assumption that Key 2 would be
a valid chain of trust for Key 1, while clearly the presence of any
DS is uncertain.
The fix here is to check if the DNSKEY and KRRSIG are in OMNIPRESENT
state for the key that does not have its DS in the HIDDEN state, and
only if that is not the case, ensure that there is a key with the same
algorithm, that provides a valid chain of trust, that is, has its
DNSKEY, KRRSIG, and DS in OMNIPRESENT state.
The changes in 'keymgr_dnskey_hidden_or_chained()' are only cosmetical,
renaming 'rrsig_omnipresent' to 'rrsig_chained' and removing the
redundant initialization of the DST_KEY_DNSKEY expected state to NA.
The previous commit changed the function definition of
'keymgr_key_is_successor()', this commit updates the code where
this function is called.
In 'keymgr_key_exists_with_state()' the logic is also updated slightly
to become more readable. First handle the easy cases:
- If the key does not match the state, continue with the next key.
- If we found a key with matching state, and there is no need to
check the successor relationship, return (true).
- Otherwise check the successor relationship.
In 'keymgr_key_has_successor()' it is enough to check if a key has
a direct successor, so instead of calling 'keymgr_key_is_successor()',
we can just check 'keymgr_direct_dep()'.
In 'dns_keymgr_run()', we want to make sure that there is no
dependency on the keys before retiring excess keys, so replace
'keymgr_key_is_successor()' with 'keymgr_dep()'.
So far the key manager could only deal with two keys in a rollover,
because it used a simplified version of the successor relationship
equation from "Flexible and Robust Key Rollover" paper. The simplified
version assumes only two keys take part in the key rollover and it
for that it is enough to check the direct relationship between two
keys (is key x the direct predecessor of key z and is key z the direct
successor of key x?).
But when a third key (or more keys) comes into the equation, the key
manager would assume that one key (or more) is redundant and removed
it from the zone prematurely.
Fix by implementing Equation(2) correctly, where we check for
dependencies on keys:
z ->T x: Dep(x, T) = ∅ ∧
(x ∈ Dep(z, T) ∨
∃ y ∈ Dep(z, T)(y != z ∧ y ->T x ∧ DyKyRySy = DzKzRzSz))
This says: key z is a successor of key x if:
- key x depends on key z if z is a direct successor of x,
- or if there is another key y that depends on key z that has identical
key states as key z and key y is a successor of key x.
- Also, key x may not have any other keys depending on it.
This is still a simplified version of Equation(2) (but at least much
better), because the paper allows for a set of keys to depend on a
key. This is defined as the set Dep(x, T). Keys in the set Dep(x, T)
have a dependency on key x for record type T. The BIND implementation
can only have one key in the set Dep(x, T). The function
'keymgr_dep()' stores this key in 'uint32_t *dep' if there is a
dependency.
There are two scenarios where multiple keys can depend on a single key:
1. Rolling keys is faster than the time required to finish the
rollover procedure. This scenario is covered by the recursive
implementation, and checking for a chain of direct dependencies
will suffice.
2. Changing the policy, when a zone is requested to be signed with
a different key length for example. BIND 9 will not mark successor
relationships in this case, but tries to move towards the new
policy. Since there is no successor relationship, the rules are
even more strict, and the DNSSEC reconfiguration is actually slower
than required.
Note: this commit breaks the build, because the function definition
of 'keymgr_key_is_successor' changed. This will be fixed in the
following commit.
*** CID 318094: Null pointer dereferences (REVERSE_INULL)
/lib/dns/rbtdb.c: 1389 in newversion()
1383 version->xfrsize = rbtdb->current_version->xfrsize;
1384 RWUNLOCK(&rbtdb->current_version->rwlock, isc_rwlocktype_read);
1385 rbtdb->next_serial++;
1386 rbtdb->future_version = version;
1387 RBTDB_UNLOCK(&rbtdb->lock, isc_rwlocktype_write);
1388
CID 318094: Null pointer dereferences (REVERSE_INULL)
Null-checking "version" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
1389 if (version == NULL) {
1390 return (result);
1391 }
1392
1393 *versionp = version;
1394
Building sid-i386 in Docker no longer works and we don't have a viable
alternative now, so dropping gcc:sid:i386 is our only option in this
very moment.
removed the isc_cfg_http_t and isc_cfg_tls_t structures
and the functions that loaded and accessed them; this can
be done using normal config parser functions.
This commit contains fixes to unit tests to make them work well on
various platforms (in particular ones shipping old versions of
OpenSSL) and for different configurations.
It also updates the generated manpage to include DoH configuration
options.
This commit completes the support for DNS-over-HTTP(S) built on top of
nghttp2 and plugs it into the BIND. Support for both GET and POST
requests is present, as required by RFC8484.
Both encrypted (via TLS) and unencrypted HTTP/2 connections are
supported. The latter are mostly there for debugging/troubleshooting
purposes and for the means of encryption offloading to third-party
software (as might be desirable in some environments to simplify TLS
certificates management).
This commit includes work-in-progress implementation of
DNS-over-HTTP(S).
Server-side code remains mostly untested, and there is only support
for POST requests.
This commit adds stub parser support and tests for:
- an "http" global option for HTTP/2 endpoint configuration.
- command line options to set http or https port numbers by
specifying -p http=PORT or -p https=PORT. (NOTE: this change
only affects syntax; specifying HTTP and HTTPS ports on the
command line currently has no effect.)
- named.conf options "http-port" and "https-port"
- HTTPSPORT environment variable for use when running tests.
This commit resurrects the old TLS code from
8f73c70d23.
It also includes numerous stability fixes and support for
isc_nm_cancelread() for the TLS layer.
The code was resurrected to be used for DoH.
In order to prevent documentation building issues from being glossed
over, pass the -W command line switch to all sphinx-build invocations.
This causes the latter to return with a non-zero exit code whenever any
Sphinx warnings are triggered.
Both doc/man/ddns-confgen.rst and doc/man/tsig-keygen.rst include
bin/confgen/tsig-keygen.rst, which defines a "man_tsig-keygen" label.
This triggers the following warning when running sphinx-build with the
-W command line switch in the doc/man/ directory:
../../bin/confgen/tsig-keygen.rst:27: WARNING: duplicate label man_tsig-keygen, other instance in /tmp/bind9/doc/man/ddns-confgen.rst
Move the offending label from bin/confgen/tsig-keygen.rst to the proper
spot in doc/arm/manpages.rst to avoid effectively defining it twice in
different source documents while still allowing the relevant man page to
be referenced in the ARM. Also rename that label so that it more
closely matches the content it points to. As the label no longer
immediately precedes a section title in its new location, use
:ref:`Title <label>` syntax for the only reference to the
tsig-keygen/ddns-confgen man page in the ARM.
Simultaneously starting multiple sphinx-build instances with the -d
command line switch set to a common value (which is what happens when
e.g. "make -j6 doc" is run) causes intermittent problems which we failed
to notice before because they only trigger Sphinx warnings, not errors,
e.g.:
WARNING: toctree contains ref to nonexisting file 'reference'
The message above is not triggered because doc/arm/reference.rst is
actually missing from disk at any point, but rather because a temporary
file created by one sphinx-build instance gets truncated by another one
working in parallel (the confusing message quoted above is logged
because of an overly broad "except" statement in Sphinx code).
Prevent this problem from being triggered by making each sphinx-build
process use its own dedicated cache directory.
The 'key_init()' function is used to initialize a state file for keys
that don't have one yet. This can happen if you are migrating from a
'auto-dnssec' or 'inline-signing' to a 'dnssec-policy' configuration.
It did not look at the "Inactive" and "Delete" timing metadata and so
old keys left behind in the key directory would also be considered as
a possible active key. This commit fixes this and now explicitly sets
the key goal to OMNIPRESENT for keys that have their "Active/Publish"
timing metadata in the past, but their "Inactive/Delete" timing
metadata in the future. If the "Inactive/Delete" timing metadata is
also in the past, the key goal is set to HIDDEN.
If the "Inactive/Delete" timing metadata is in the past, also the
key states are adjusted to either UNRETENTIVE or HIDDEN, depending on
how far in the past the metadata is set.
The 'legacy-keys.kasp' test checks that a zone with key files but not
yet state files is signed correctly. This test is expanded to cover
the case where old key files still exist in the key directory. This
covers bug #2406 where keys with the "Delete" timing metadata are
picked up by the keymgr as active keys.
Fix the 'legacy-keys.kasp' test, by creating the right key files
(for zone 'legacy-keys.kasp', not 'legacy,kasp').
Use a unique policy for this zone, using shorter lifetimes.
Create two more keys for the zone, and use 'dnssec-settime' to set
the timing metadata in the past, long enough ago so that the keys
should not be considered by the keymgr.
Update the 'key_unused()' test function, and consider keys with
their "Delete" timing metadata in the past as unused.
Extend the test to ensure that the keys to be used are not the old
predecessor keys (with their "Delete" timing metadata in the past).
Update the test so that the checks performed are consistent with the
newly configured policy.
- [ ] Ensure the merge request from the previous step is reviewed by SWENG staff and has no outstanding discussions
- [ ] Ensure the documentation changes introduced by the merge request addressing the problem are reviewed by Support and Marketing staff
- [ ] Prepare backports of the merge request addressing the problem for all affected (and still maintained) BIND branches (backporting might affect the issue's scope and/or description)
- [ ] Prepare a standalone patch for the last stable release of each affected (and still maintained) BIND branch
### Release-specific actions
- [ ] Create/update the private issue containing links to fixes & reproducers for all CVEs fixed in a given release cycle
- [ ] Reserve a block of `CHANGES` placeholders once the complete set of vulnerabilities fixed in a given release cycle is determined
- [ ] Ensure the merge requests containing CVE fixes are merged into `security-*` branches in CVE identifier order
- [ ]***(Support)*** Publish links to downloads on ISC website.
- [ ]***(Support)*** Write release email to *bind-announce*.
- [ ]***(Support)*** Write email to *bind-users* (if a major release).
- [ ]***(Support)*** Send eligible customers updated links to the Subscription Edition.
- [ ]***(Support)*** Send eligible customers updated links to the Subscription Edition (update the -S edition delivery tickets, even if those links were provided earlier via an ASN ticket).
- [ ]***(Support)*** Update tickets in case of waiting support customers.
- [ ]***(QA)*** Build and test any outstanding private packages.
- [ ]***(QA)*** Build public packages (`*.deb`, RPMs).
@@ -85,7 +87,9 @@
- [ ]***(QA)*** Merge the automatically prepared `prep 9.x.y` commit which updates `version` and documentation on the release branch into the relevant maintenance branch (`v9_x`).
- [ ]***(QA)*** For each maintained branch, update the `BIND_BASELINE_VERSION` variable for the `abi-check` job in `.gitlab-ci.yml` to the latest published BIND version tag for a given branch.
- [ ]***(QA)*** Prepare empty release notes for the next set of releases.
- [ ]***(QA)*** Sanitize all confidential issues assigned to the release milestone and make them public.
- [ ]***(QA)*** Sanitize confidential issues which are assigned to the current release milestone and do not describe a security vulnerability, then make them public.
- [ ]***(QA)*** Sanitize confidential issues which are assigned to older release milestones and describe security vulnerabilities, then make them public if appropriate[^2].
- [ ]***(QA)*** Update QA tools used in GitLab CI (e.g. Flake8, PyLint) by modifying the relevant `Dockerfile`.
[^1]: If not, use the time remaining until the tagging deadline to ensure all outstanding issues are either resolved or moved to a different milestone.
[^2]: As a rule of thumb, security vulnerabilities which have reproducers merged to the public repository are considered okay for full disclosure.
Some files were not shown because too many files have changed in this diff
Show More
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
