Evan Hunt [Fri, 24 Jan 2025 01:16:30 +0000 (17:16 -0800)]
Check whether a rejected rrset is different
Add a new dns_rdataset_equals() function to check whether two
rdatasets are equal in DNSSEC terms.
When an rdataset being cached is rejected because its trust
level is lower than the existing rdataset, we now check to see
whether the rejected data was identical to the existing data.
This allows us to cache a potentially useful RRSIG when handling
CD=1 queries, while still rejecting RRSIGs that would definitely
have resulted in a validation failure.
Evan Hunt [Wed, 19 Feb 2025 23:43:41 +0000 (23:43 +0000)]
fix: dev: Clean up dns_rdataslab module
Rdata slabs used in the QP databases are usually prepended with a slab header, but are sometimes "raw", containing only the rdata and no header. Previously, to allow for them to be used both ways, functions that operated on them took a `reservelen` argument, which would be set to either the header length or to zero, and skipped over that many bytes at the beginning of the buffer. Most such functions were never used on the raw form. To make the code clearer, each of these functions now operates on full slabs with headers, and an alternate "raw" version of the function has been added in cases where that was needed.
In addition, the `dns_rdataslab_merge()` and `_subtract()` functions have been rewritten for clarity and efficiency, and a minor bug has been fixed in `dns_rdataslab_equal()` and `_equalx()`, which could cause an incorrect result if both slabs being compared had zero length.
Merge branch 'each-refactor-rdataslab' into 'main'
Ondřej Surý [Wed, 19 Feb 2025 12:59:23 +0000 (13:59 +0100)]
Remove the "raw" version of the dns_slabheader API
The "raw" version of the header was used for the noqname and the closest
proofs to save around 152 bytes of the dns_slabheader_t while bringing
an additional complexity. Remove the raw version of the dns_slabheader
API at the slight expense of having unused dns_slabheader_t data sitting
in front of the proofs.
Evan Hunt [Mon, 10 Feb 2025 04:43:39 +0000 (20:43 -0800)]
refactor dns_rdataslab_subtract() for efficiency
reduce the number of rdata comparisons needed by walking
through the original slab once to determine whether the rdata
in it is duplicated in the slab to be subtracted, and then
write out the rdatas that aren't. previously, this was
done twice: once when determining the size of the target buffer
and then again when copying data into it.
Evan Hunt [Mon, 10 Feb 2025 04:03:01 +0000 (20:03 -0800)]
refactor dns_rdataslab_merge() for efficiency
when merging two rdata slabs, we now check once to see
whether an item in the new slab has a duplicate in the
old. previously this was done twice; once to determine the
size of the target buffer required, and then again when
copying the data into it.
we also minimize the number of rdata comparisons necessary,
by remembering which items in the old slab have already been
found to be duplicates.
The function name dns_slabheader_fromrdataset() was too similar
to dns_rdataslab_fromrdataset(). Instead, we now have an rdataset
method 'getheader' which is implemented for slab-type rdatasets.
A new NOHEADER rdataset attribute is set for rdatasets using
raw slabs (i.e., noqname and closest encloser proofs); when
called on rdatasets with that flag set, dns_rdataset_getheader()
returns NULL.
Evan Hunt [Sat, 8 Feb 2025 05:06:34 +0000 (21:06 -0800)]
initialize header in dns_rdataslab_fromrdataset()
when dns_rdataslab_fromrdataset() is run, in addition to
allocating space for a slab header, it also partially
initializes it, setting the type match rdataset->type and
rdataset->covers, the trust to rdataset->trust, and the TTL to
rdataset->ttl.
Evan Hunt [Sat, 8 Feb 2025 04:07:53 +0000 (20:07 -0800)]
clarify dns_rdataslab_fromrdataset()
there are now two functions for creating an rdataslab from an
rdataset: dns_rdataslab_fromrdataset() creates a full slab (including
space for a slab header), and dns_rdataslab_raw_fromrdataset() creates
a raw slab.
Evan Hunt [Sat, 8 Feb 2025 05:21:52 +0000 (21:21 -0800)]
more rdataslab refactoring
- there are now two functions for getting rdataslab size:
dns_rdataslab_size() is for full slabs and dns_rdataslab_sizeraw()
for raw slabs. there is no longer a need for a reservelen parameter.
- dns_rdataslab_count() also no longer takes a reservelen parameter.
(currently it's never used for raw slabs, so there is no _countraw()
function.)
- dns_rdataslab_rdatasize() has been removed, because
dns_rdataslab_sizeraw() can do the same thing.
- dns_rdataslab_merge() and dns_rdataslab_subtract() both take
slabheader parameters instead of character buffers, and the
reservelen parameter has been removed.
Evan Hunt [Thu, 6 Feb 2025 23:50:52 +0000 (15:50 -0800)]
fix and simplify dns_rdataset_equal() and _equalx()
if both rdataslabs being compared have zero length, return true.
also, since these functions are only ever called on slabheaders
with sizeof(dns_slabheader_t) as the reserve length, we can
simplify the API: remove the reservelen argument, and pass the
slabs as type dns_slabheader_t * instead of unsigned char *.
Ondřej Surý [Fri, 31 Jan 2025 14:47:33 +0000 (15:47 +0100)]
Add .up pointer to slabheader
The dns_slabheader object uses the 'next' pointer for two purposes.
In the first header for any given type, 'next' points to the first
header for the next type. But 'down' points to the next header of
the same type, and in that record, 'next' points back up.
This design made the code confusing to read. We now use a union
so that the 'next' pointer can also be called 'up'.
Artem Boldariev [Wed, 19 Feb 2025 16:39:36 +0000 (16:39 +0000)]
fix: dev: Post [CVE-2024-12705] Performance Drop Fixes
This merge request fixes a [performance drop](https://gitlab.isc.org/isc-projects/bind9/-/pipelines/216728) after merging the fixes for #4795, in particular in 9.18.
The MR [fixes the problem](https://gitlab.isc.org/isc-projects/bind9/-/pipelines/219825) without affecting performance for the newer versions, in particular for [the development version](https://gitlab.isc.org/isc-projects/bind9/-/pipelines/220619).
Merge branch 'artem-doh-performance-drop' into 'main'
Artem Boldariev [Wed, 19 Feb 2025 10:28:37 +0000 (12:28 +0200)]
DoH: http_send_outgoing() return value is not used
The value returned by http_send_outgoing() is not used anywhere, so we
make it not return anything (void). Probably it is an omission from
older times.
Artem Boldariev [Thu, 13 Feb 2025 13:05:10 +0000 (15:05 +0200)]
DoH: change how the active streams number is calculated
This commit changes the way how the number of active HTTP streams is
calculated and allows it to scale with the values of the maximum
amount of streams per connection, instead of effectively capping at
STREAM_CLIENTS_PER_CONN.
The original limit, which is intended to define the pipelining limit
for TCP/DoT. However, it appeared to be too restrictive for DoH, as it
works quite differently and implements pipelining at protocol level by
the means of multiplexing multiple streams. That renders each stream
to be effectively a separate connection from the point of view of the
rest of the codebase.
Artem Boldariev [Thu, 13 Feb 2025 12:53:18 +0000 (14:53 +0200)]
DoH: Track the amount of in flight outgoing data
Previously we would limit the amount of incoming data to process based
solely on the presence of not completed send requests. That worked,
however, it was found to severely degrade performance in certain
cases, as was revealed during extended testing.
Now we switch to keeping track of how much data is in flight (or ready
to be in flight) and limit the amount of processed incoming data when
the amount of in flight data surpasses the given threshold, similarly
to like we do in other transports.
Ondřej Surý [Sun, 2 Feb 2025 23:06:48 +0000 (00:06 +0100)]
Unify how we handle database version in the cache
Database versions are not used in cache databases. Some places in
qpcache.c required the version argument to be NULL; others marked it
as UNUSED. Unify all cases to require version to be NULL.
Ondřej Surý [Sun, 2 Feb 2025 19:22:29 +0000 (20:22 +0100)]
Refactor the search in qpcache_findrdataset()
Add new related_headers() function that simplifies the code
flow in qpcache_findrdataset(). Also use check_stale_header() function
to remove code duplication.
Ondřej Surý [Sun, 2 Feb 2025 19:07:42 +0000 (20:07 +0100)]
Refactor simple slabheader matching
Add a helper function both_headers() that unifies the slabheader
matching for simple type: it returns true when both the type and
the matching RRSIG have been found.
Ondřej Surý [Sun, 2 Feb 2025 18:37:21 +0000 (19:37 +0100)]
Add new helper maybe_update_headers() function
The new maybe_update_headers() function unifies the LRU updates to the
slabheaders that was scattered all over the place. More calls to update
headers after bindrdatasets() were also added for completeness.
Ondřej Surý [Sun, 2 Feb 2025 18:29:53 +0000 (19:29 +0100)]
Add bindrdatasets() function that binds both rdatasets
This removes code duplication between the dual bindrdataset() calls. It
also unifies the handling as there were small differences between the
calls: one variant was checking for !NEGATIVE(found) condition and one
wasn't, and it is technically ok to do the check for all variants.
Ondřej Surý [Sun, 2 Feb 2025 18:21:44 +0000 (19:21 +0100)]
Refactor check_stale_header() function
The check_stale_header() function now updates header_prev directly
so it doesn't have to be handled in the outer loop; it's always
set to the correct value of the previous header in the chain.
Evan Hunt [Thu, 23 Jan 2025 07:08:04 +0000 (23:08 -0800)]
clean up unnecessary code in qpcache
some code was left in the cache database implementation after
it was separated from the zone database, and can be cleaned up
and refactored now:
- the DNS_SLABHEADERATTR_IGNORE flag is never set in the cache
- support for loading the cache from was removed, but the add()
function still had a 'loading' flag that's always false
- two different macros were used for checking the
DNS_SLABHEADERATTR_NONEXISTENT flag - EXISTS() and NONEXISTENT().
it's clearer to just use EXISTS().
- the cache doesn't support versions, so it isn't necessary to
walk down the 'down' pointer chain when iterating through the
cache or looking for a header to update. 'down' now only points
to records that are deleted from the cache but have not yet been
purged from memory. this allows us to simplify both the iterator
and the add() function.
Petr Menšík [Wed, 10 Jan 2024 20:25:52 +0000 (21:25 +0100)]
Do HTTPS record query from host in addition
Unless explicitly specified type from host command, do fourth query for
type HTTPS RR. It is expected it will become more common and some
systems already query that record for every name.
Evan Hunt [Wed, 12 Feb 2025 00:08:29 +0000 (16:08 -0800)]
fix dns_qp_insert() checks in qpzone
in some places there were checks for failures of dns_qp_insert()
after dns_qp_getname(). such failures could only happen if another
thread inserted a node between the two calls, and that can't happen
because the calls are serialized with dns_qpmulti_write(). we can
simplify the code and just add an INSIST.
Arаm Sаrgsyаn [Mon, 17 Feb 2025 18:24:02 +0000 (18:24 +0000)]
fix: usr: Use named Service Parameter Keys (SvcParamKeys) by default
When converting SVCB records to text representation `named` now uses named
`SvcParamKeys` values unless backward-compatible mode is activated,
in which case the values which were not defined initially in
RFC9460 and were added later (see [1]) are converted to opaque
"keyNNNN" syntax, like, for example, "key7" instead of "dohpath".
Also a new `+[no]svcparamkeycompat` option is implemented for `dig`,
which enables the backward-compatible mode and uses the opaque
syntax, if required for interoperability with other software or
scripts. By default, the compatibility mode is disabled.
Aram Sargsyan [Mon, 10 Feb 2025 13:25:57 +0000 (13:25 +0000)]
Test dig +svcparamkeycompat
This check in the nsupdate system test expects the opaque
representation of the "dohpath" Service Parameter Key. Use
the +svcparamkeycompat dig option to enable it.
Aram Sargsyan [Mon, 10 Feb 2025 13:22:58 +0000 (13:22 +0000)]
Implement dig +[no]svcparamkeycompat
The new +[no]svcparamkeycompat option for dig enables the
backward-compatible mode for the Service Parameter Keys'
(SvcParamKeys) representation format. See the previous commit
for more information.
Aram Sargsyan [Mon, 10 Feb 2025 13:11:18 +0000 (13:11 +0000)]
Use named Service Parameter Keys (SvcParamKeys) by default
When converting SVCB records to text representation use named
SvcParamKeys values unless backward-compatible mode is activated,
in which case the values which were not defined initially in
RFC9460 and were added later (see [1]) are converted to opaque
"keyN" syntax, like, for example, "key7" instead of "dohpath".
Alessio Podda [Mon, 17 Feb 2025 12:30:24 +0000 (12:30 +0000)]
chg: dev: Refactor and simplify isc_symtab
This commit does several changes to isc_symtab:
1. Rewrite the isc_symtab to internally use isc_hashmap instead of
hand-stiched hashtable.
2. Create a new isc_symtab_define_and_return() api, which returns
the already defined symvalue on ISC_R_EXISTS; this allows users
of the API to skip the isc_symtab_lookup()+isc_symtab_define()
calls and directly call isc_symtab_define_and_return().
3. Merge isccc_symtab into isc_symtab - the only missing function
was isccc_symtab_foreach() that was merged into isc_symtab API.
4. Add full set of unit tests for the isc_symtab API.
Closes #5103
Merge branch '5103-use-isc_hashmap-for-isc_symtab' into 'main'
alessio [Fri, 29 Nov 2024 09:02:13 +0000 (10:02 +0100)]
Refactor and simplify isc_symtab
This commit does several changes to isc_symtab:
1. Rewrite the isc_symtab to internally use isc_hashmap instead of
hand-stiched hashtable.
2. Create a new isc_symtab_define_and_return() api, which returns
the already defined symvalue on ISC_R_EXISTS; this allows users
of the API to skip the isc_symtab_lookup()+isc_symtab_define()
calls and directly call isc_symtab_define_and_return().
3. Merge isccc_symtab into isc_symtab - the only missing function
was isccc_symtab_foreach() that was merged into isc_symtab API.
4. Add full set of unit tests for the isc_symtab API.
Mark Andrews [Sun, 16 Feb 2025 23:36:25 +0000 (23:36 +0000)]
fix: usr: Fix deferred validation of unsigned DS and DNSKEY records
When processing a query with the "checking disabled" bit set (CD=1), `named` stores the unvalidated result in the cache, marked "pending". When the same query is sent with CD=0, the cached data is validated, and either accepted as an answer, or ejected from the cache as invalid. This deferred validation was not attempted for DS and DNSKEY records if they had no cached signatures, causing spurious validation failures. We now complete the deferred validation in this scenario.
Also, if deferred validation fails, we now re-query the data to find out whether the zone has been corrected since the invalid data was cached.
Closes #5066
Merge branch '5066-fix-strip-dnssec-rrsigs' into 'main'
Mark Andrews [Fri, 20 Dec 2024 09:24:05 +0000 (20:24 +1100)]
Re-fetch pending records that failed validation
If a deferred validation on data that was originally queried with
CD=1 fails, we now repeat the query, since the zone data may have
changed in the meantime.
Mark Andrews [Fri, 20 Dec 2024 09:24:05 +0000 (20:24 +1100)]
Complete the deferred validation if there are no RRSIGs
When a query is made with CD=1, we store the result in the
cache marked pending so that it can be validated later, at
which time it will either be accepted as an answer or removed
from the cache as invalid. Deferred validation was not
attempted when there were no cached RRSIGs for DNSKEY and
DS. We now complete the deferred validation in this scenario.
Mark Andrews [Fri, 14 Feb 2025 02:23:52 +0000 (02:23 +0000)]
fix: usr: "CNAME and other data check" not applied to all types
An incorrect optimization caused "CNAME and other data" errors not to be detected if certain types were at the same node as a CNAME. This has been fixed.
Closes #5150
Merge branch '5150-cname-and-other-data-check-not-applied-to-all-types' into 'main'
Mark Andrews [Thu, 30 Jan 2025 22:09:33 +0000 (09:09 +1100)]
Fix "CNAME and other data" detection
prio_type was being used in the wrong place to optimize cname_and_other.
We have to first exclude and accepted types and we also have to
determine that the record exists before we can check if we are at
a point where a later CNAME cannot appear.
Michal Nowak [Wed, 5 Feb 2025 15:01:03 +0000 (15:01 +0000)]
fix: ci: Do not evaluate $CI_PROJECT_DIR in generate-stress-test-configs.py
GitLab CI Runner's $builds_dir variable is set to "/builds" by default.
For technical reasons, the FreeBSD Runners, using the "instance"
executor, sets the path differently.
The value of $CI_PROJECT_DIR is based on $builds_dir, so if the
generate-stress-test-configs.py script generates jobs with
$CI_PROJECT_DIR (or variables like $INSTALL_PATH that are based on it)
evaluated, it is calcified to whatever was the value in the particular
environment, disregarding the FreeBSD "instance" executor specifics in
the child pipeline.
Instead of evaluating $CI_PROJECT_DIR in the script, evaluate it in the
runtime environment.
Merge branch 'mnowak/fix-CI_PROJECT_DIR-variable-evaluation' into 'main'
Michal Nowak [Wed, 5 Feb 2025 08:59:58 +0000 (09:59 +0100)]
Do not evaluate $CI_PROJECT_DIR in generate-stress-test-configs.py
GitLab CI Runner's $builds_dir variable is set to "/builds" by default.
For technical reasons, the FreeBSD Runners, using the "instance"
executor, sets the path differently.
The value of $CI_PROJECT_DIR is based on $builds_dir, so if the
generate-stress-test-configs.py script generates jobs with
$CI_PROJECT_DIR (or variables like $INSTALL_PATH that are based on it)
evaluated, it is calcified to whatever was the value in the particular
environment, disregarding the FreeBSD "instance" executor specifics in
the child pipeline.
Instead of evaluating $CI_PROJECT_DIR in the script, evaluate it in the
runtime environment.
Ondřej Surý [Tue, 4 Feb 2025 22:24:20 +0000 (22:24 +0000)]
fix: dev: Reduce the false sharing the dns_qpcache and dns_qpzone
Instead of having many node_lock_count * sizeof(<member>) arrays, pack all
the members into a qpcache_bucket_t that is cacheline aligned to prevent
false sharing between RWLocks.
Merge branch 'ondrej/prevent-nodelock-false-sharing' into 'main'
Ondřej Surý [Tue, 4 Feb 2025 20:15:22 +0000 (21:15 +0100)]
Switch the locknum generation for qpznode to random
Instead of using on hash of the name modulo number of the buckets,
assign the locknum randomly with isc_random_uniform(). This makes
the locknum assignment aligned with qpcache and allows the bucket
number to be non-prime in the future.
Ondřej Surý [Tue, 4 Feb 2025 17:02:29 +0000 (18:02 +0100)]
Rely on call_rcu() to destroy the qpzone outside of locks
Reduce the number of qpzone_ref() and qpzone_unref() calls in
qpzone_detachnode() by relying on the call_rcu to delay
the destruction of the lock buckets.
Ondřej Surý [Tue, 4 Feb 2025 15:50:27 +0000 (16:50 +0100)]
Reduce false sharing in dns_qpzone
Instead of having many node_lock_count * sizeof(<member>) arrays, pack
all the members into a qpzone_bucket_t that is cacheline aligned and have
a single array of those.
Ondřej Surý [Mon, 3 Feb 2025 12:36:27 +0000 (13:36 +0100)]
Reduce false sharing in dns_qpcache
Instead of having many node_lock_count * sizeof(<member>) arrays, pack
all the members into a qpcache_bucket_t struct that is cacheline aligned
and have a single array of those.
Additionaly, make both the head and the tail of isc_queue_t padded, not
just the head, to prevent false sharing of the lock-free structure with
the lock that follows it.
Nicki Křížek [Fri, 24 Jan 2025 09:35:06 +0000 (10:35 +0100)]
Allow to use an arbitrary numeric identifier for NamedInstance
In some cases, the numeric identifier doesn't correspond to the
directory name (i.e. `resolver` server in `shutdown` test, which is
supposed to be 10.53.0.3). These are typically servers that shouldn't be
auto-started by the runner, thus avoiding the typical `*ns<X>` name.
Support these server by allowing a fallback initialization with custom
numeric identifier in case it can't be parsed from the directory name.
Nicki Křížek [Thu, 23 Jan 2025 15:00:36 +0000 (16:00 +0100)]
Add start/stop wrappers to control NamedInstance
The start()/stop() functions can be used in the pytests in the same way
as start_server and stop_server functions were used in shell tests. Note
that the servers obtained through the servers fixture are still started
and stopped by the test runner at the start and end of the test. This
makes these functions mostly useful for restarting the server(s)
mid-test.
Nicki Křížek [Thu, 23 Jan 2025 14:04:30 +0000 (15:04 +0100)]
Move shell and perl util functions to isctest.run
Previously, these functions have been provided as fixtures. This was
limiting re-use, because it wasn't possible to call these outside of
tests / other fixtures without passing these utility functions around.
Move them into isctest.run package instead.
Mark Andrews [Tue, 4 Feb 2025 02:35:33 +0000 (02:35 +0000)]
fix: test: Fix 'ans' servers so they respond with consistent answers to NS queries at QNAME.
The ANS servers were not to written to handle NS queries at the QNAME, resulting in gratuitous protocol errors that will break tests when NS requests are made for the QNAME: i.e., NXDOMAIN for NS vs data for expected type, CNAME not being returned for all query types.
Prerequisite for !9155
Closes #5062
Merge branch '5062-fix-ans-servers-ns-at-qname' into 'main'
Mark Andrews [Thu, 18 Jul 2024 03:35:41 +0000 (13:35 +1000)]
Fix gratuitious DNS protocol errors in the ANS servers
The ANS servers were not to written to handle NS queries at the
QNAME resulting in gratuitious protocol errors that will break tests
when NS requests are made for the QNAME.
Ondřej Surý [Mon, 3 Feb 2025 10:15:15 +0000 (11:15 +0100)]
Print the expiration time of the stale records (not ancient)
In #1870, the expiration time of ANCIENT records were printed, but
actually the ancient records are very short lived, and the information
carries a little value.
Instead of printing the expiration of ANCIENT records, print the
expiration time of STALE records.
Ondřej Surý [Mon, 3 Feb 2025 13:06:37 +0000 (14:06 +0100)]
Restore the .ttl field for slabheader in dns_qpzone
The original .ttl field was actually used as TTL in the dns_qpzone unit.
Restore the field by adding it to union with the .expire struct member
and cleanup all the code that added or subtracted 'now' from the ttl
field as that was misleading as 'now' would be always 0 for qpzone
database.
Ondřej Surý [Sun, 2 Feb 2025 13:07:18 +0000 (14:07 +0100)]
Remove duplicate 'now' argument from find_coveringnsec()
The find_coveringnsec() was getting the 'now' from two sources -
search->now and separate now argument. Things like this are ticking
bombs, remove the extra 'now' argument and use single source of 'now'.