Fix Coverity CHECKED_RETURN reports for dst_key_getbool(). In most
cases we do not really care about its return value, but it is prudent
to check it.
In one case, where a dst_key_getbool() error should be treated
identically as success, cast the return value to void and add a relevant
comment.
(cherry picked from commit e645d2ef1e)
When "rndc reconfig" is run, named first configures a fresh set of views
and then tears down the old views. Consider what happens for a single
view with LMDB enabled; "envA" is the pointer to the LMDB environment
used by the original/old version of the view, "envB" is the pointer to
the same LMDB environment used by the new version of that view:
1. mdb_env_open(envA) is called when the view is first created.
2. "rndc reconfig" is called.
3. mdb_env_open(envB) is called for the new instance of the view.
4. mdb_env_close(envA) is called for the old instance of the view.
This seems to have worked so far. However, an upstream change [1] in
LMDB which will be part of its 0.9.26 release prevents the above
sequence of calls from working as intended because the locktable mutexes
will now get destroyed by the mdb_env_close() call in step 4 above,
causing any subsequent mdb_txn_begin() calls to fail (because all of the
above steps are happening within a single named process).
Preventing the above scenario from happening would require either
redesigning the way we use LMDB in BIND, which is not something we can
easily backport, or redesigning the way BIND carries out its
reconfiguration process, which would be an even more severe change.
To work around the problem, set MDB_NOLOCK when calling mdb_env_open()
to stop LMDB from controlling concurrent access to the database and do
the necessary locking in named instead. Reuse the view->new_zone_lock
mutex for this purpose to prevent the need for modifying struct dns_view
(which would necessitate library API version bumps). Drop use of
MDB_NOTLS as it is made redundant by MDB_NOLOCK: MDB_NOTLS only affects
where LMDB reader locktable slots are stored while MDB_NOLOCK prevents
the reader locktable from being used altogether.
[1] https://git.openldap.org/openldap/openldap/-/commit/2fd44e325195ae81664eb5dc36e7d265927c5ebc
(cherry picked from commit 53120279b5)
The ThreadSanitizer found a data race when updating the stale header.
Instead of trying to acquire the write lock and failing occasionally
which would skew the statistics, the dns_rdatasetheader_t.attributes
field has been promoted to use stdatomics. Updating the attributes in
the mark_header_ancient() and mark_header_stale() now uses the cmpxchg
to update the attributes forfeiting the need to hold the write lock on
the tree. Please note that mark_header_ancient() still needs to hold
the lock because .dirty is being updated in the same go.
(cherry picked from commit 81d4230e60)
When we're coming back from recursion fetch_callback does not accept
DNS_R_NXDOMAIN as an rcode - query_gotanswer calls query_nxdomain in
which an assertion fails on qctx->is_zone. Yet, under some
circumstances, qname minimization will return an DNS_R_NXDOMAIN - when
root zone mirror is not yet loaded. The fix changes the DNS_R_NXDOMAIN
answer to DNS_R_SERVFAIL.
When a library is examined, an object file within it can be left out
of the link if it does not provide symbols that the symbol table
needs. Introducing `isc_stdtime_tostring` caused a build failure for
`update_test` because it now requires `libisc.a(stdtime.o)` and that
also exports the `isc_stdtime_get` symbol, meaning we have a
multiple definition error.
Add a local version of `isc_stdtime_tostring`, so that the linker
will not search for it in available object files.
Implement the 'rndc dnssec -status' command that will output
some information about the key states, such as which policy is
used for the zone, what keys are in use, and when rollover is
scheduled.
Add loose testing in the kasp system test, the actual times are
already tested via key file inspection.
(cherry picked from commit 19ce9ec1d4)
I'd like to use the same functionality (pretty print the datetime
of keytime metadata) in the 'rndc dnssec -status' command. So it is
better that this logic is done in a separate function.
Since the stdtime.c code have differernt files for unix and win32,
I think the "#ifdef WIN32" define can be dropped.
(cherry picked from commit 9e03f8e8fe)
The `rndc` argument was always overridden by the static configuration,
because the logic for handling the number of dnstap files to retain
was both backwards and a bit redundant.
(cherry picked from commit 7c07129a51)
With Clang 10.0.0 on FreeBSD 11.4, compiling lib/dns/spnego.c triggers
the following warnings:
spnego.c:361:11: error: converting the result of '<<' to a boolean always evaluates to true [-Werror,-Wtautological-constant-compare]
return (GSS_S_DEFECTIVE_TOKEN);
^
/usr/include/gssapi/gssapi.h:423:41: note: expanded from macro 'GSS_S_DEFECTIVE_TOKEN'
#define GSS_S_DEFECTIVE_TOKEN (9ul << GSS_C_ROUTINE_ERROR_OFFSET)
^
spnego.c:366:11: error: converting the result of '<<' to a boolean always evaluates to true [-Werror,-Wtautological-constant-compare]
return (GSS_S_DEFECTIVE_TOKEN);
^
/usr/include/gssapi/gssapi.h:423:41: note: expanded from macro 'GSS_S_DEFECTIVE_TOKEN'
#define GSS_S_DEFECTIVE_TOKEN (9ul << GSS_C_ROUTINE_ERROR_OFFSET)
^
spnego.c:371:12: error: converting the result of '<<' to a boolean always evaluates to true [-Werror,-Wtautological-constant-compare]
return (GSS_S_DEFECTIVE_TOKEN);
^
/usr/include/gssapi/gssapi.h:423:41: note: expanded from macro 'GSS_S_DEFECTIVE_TOKEN'
#define GSS_S_DEFECTIVE_TOKEN (9ul << GSS_C_ROUTINE_ERROR_OFFSET)
^
spnego.c:376:11: error: converting the result of '<<' to a boolean always evaluates to true [-Werror,-Wtautological-constant-compare]
return (GSS_S_DEFECTIVE_TOKEN);
^
/usr/include/gssapi/gssapi.h:423:41: note: expanded from macro 'GSS_S_DEFECTIVE_TOKEN'
#define GSS_S_DEFECTIVE_TOKEN (9ul << GSS_C_ROUTINE_ERROR_OFFSET)
^
spnego.c:380:11: error: converting the result of '<<' to a boolean always evaluates to true [-Werror,-Wtautological-constant-compare]
return (GSS_S_DEFECTIVE_TOKEN);
^
/usr/include/gssapi/gssapi.h:423:41: note: expanded from macro 'GSS_S_DEFECTIVE_TOKEN'
#define GSS_S_DEFECTIVE_TOKEN (9ul << GSS_C_ROUTINE_ERROR_OFFSET)
^
5 errors generated.
Address by replacing all instances of the GSS_S_DEFECTIVE_TOKEN constant
with a boolean value. Invert the values returned by cmp_gss_type() so
that its only call site reads more naturally in the context of the
comment preceding it.
The wait until zones are signed after rndc reconfig is broken
because the zones are already signed before the reconfig. Fix
by having a different way to ensure the signing of the zone is
complete. This does require a call to the "wait_for_done_signing"
function after each "check_keys" call after the ns6 reconfig.
The "wait_for_done_signing" looks for a (newly added) debug log
message that named will output if it is done signing with a certain
key.
(cherry picked from commit a47192ed5b)
RBTDB node can now appear on the deadnodes lists following the changes
to decrement_reference in 176b23b6cd to
defer checking of node->down when the tree write lock is not held. The
node should be unlinked instead.
(cherry picked from commit 569cc155b8680d8ed12db1fabbe20947db24a0f9)
- clone keynode->dsset rather than return a pointer so that thread
use is independent of each other.
- hold a reference to the dsset (keynode) so it can't be deleted
while in use.
- create a new keynode when removing DS records so that dangling
pointers to the deleted records will not occur.
- use a rwlock when accessing the rdatalist to prevent instabilities
when DS records are added.
(cherry picked from commit e5b2eca1d3)
We were passing client address to dns_resolver_createfetch as a pointer
and it was saved as a pointer. The client (with its address) could be
gone before the fetch is finished, and in a very odd scenario
log_formerr would call isc_sockaddr_format() which first checks if the
address family is valid (and at this point it still is), then the
sockaddr is cleared, and then isc_netaddr_fromsockaddr is called which
fails an assertion as the address family is now invalid.
(cherry picked from commit 175c4d9055)
DS records only belong at delegation points and if present
at the zone apex are invariably the result of administrative
errors. Additionally they can't be queried for with modern
resolvers as the parent servers will be queried.
(cherry picked from commit 35a58d30c9)
When creating the successor, the current active key (predecessor)
should change its goal state to HIDDEN.
Also add two useful debug logs in the keymgr_key_rollover function.
(cherry picked from commit e71d60299f)
Catch a case where if the prepublication time of the successor key
is later than the retire time of the predecessor. If that is the
case we should prepublish as soon as possible, a.k.a. now.
(cherry picked from commit c08d0f7dd6)
The `dns_keymgr_run()` function became quite long, put the logic
that looks if a new key needs to be created (start a key rollover)
in a separate function.
(cherry picked from commit bcf8192438)
The logic in `keymgr_key_has_successor(key, keyring)` is flawed, it
returns true if there is any key in the keyring that has a successor,
while what we really want here is to make sure that the given key
has a successor in the given keyring.
Rather than relying on `keymgr_key_exists_with_state`, walk the
list of keys in the keyring and check if the key is a successor of
the given predecessor key.
(cherry picked from commit 0d578097ef)
This improves keytime testing on CSK rollover. It now
tests for specific times, and also tests for SyncPublish and
Removed keytimes.
Since an "active key" for ZSK and KSK means something
different, this makes it tricky to decide when a CSK is
active. An "active key" intuitively means the key is signing
so we say a CSK is active when it is creating zone signatures.
This change means a lot of timings for the CSK rollover tests
need to be adjusted.
The keymgr code needs a slight change on calculating the
prepublication time: For a KSK we need to include the parent
registration delay, but for CSK we look at the zone signing
property and stick with the ZSK prepublication calculation.
(cherry picked from commit e233433772)
Registration delay is not part of the Iret retire interval, thus
removed from the calculation when setting the Delete time metadata.
Include the registration delay in prepublication time, because
we need to prepublish the key sooner than just the Ipub
publication interval.
(cherry picked from commit 50bbbb76a8)
While kasp relies on key states to determine when a key needs to
be published or be used for signing, the keytimes are used by
operators to get some expectation of key publication and usage.
Update the code such that these keytimes are set appropriately.
That means:
- Print "PublishCDS" and "DeleteCDS" times in the state files.
- The keymgr sets the "Removed" and "PublishCDS" times and derives
those from the dnssec-policy.
- Tweak setting of the "Retired" time, when retiring keys, only
update the time to now when the retire time is not yet set, or is
in the future.
This also fixes a bug in "keymgr_transition_time" where we may wait
too long before zone signatrues become omnipresent or hidden. Not
only can we skip waiting the sign delay Dsgn if there is no
predecessor, we can also skip it if there is no successor.
Finally, this commit moves setting the lifetime, reducing two calls
to one.
(cherry picked from commit 18dc27afd3)
in addition to being more efficient, this prevents a possible crash by
looking up the node name before the tree sructure can be changed when
cleaning up dead nodes in addrdataset().
(cherry picked from commit db9d10e3c1)
If there are more that 5 NS record for a zone only perform a
maximum of 4 address lookups for all the name servers. This
limits the amount of remote lookup performed for server
addresses at each level for a given query.
Originally, every library and binaries got linked to everything, which
creates unnecessary overlinking. This wasn't as straightforward as it
should be as we still support configuration without libtool for 9.16.
Couple of smaller issues related to include headers and an issue where
sanitizer overload dlopen and dlclose symbols, so we were getting false
negatives in the autoconf test.
Matthijs Mekking
Mark Andrews
Michał Kępień
Ondřej SurýandMark Andrews
Witold KręcickiandOndřej Surý
Tony FinchandMark Andrews
Evan Hunt
Tinderbox UserandMichał Kępień
Ondřej SurýandOndřej Surý
Aaron ThompsonandOndřej Surý