This commit separates TLS context creation code from xfrin_start() as
it has become too large and hard to follow into a new
function (similarly how it is done in dighost.c)
The dead code has been removed from the cleanup section of the TLS
creation code:
* there is no way 'tlsctx' can equal 'found';
* there is no way 'sess_cache' can be non-NULL in the cleanup section.
Also, it fixes a bug in the older version of the code, where TLS
client session context fetched from the cache would not get passed to
isc_nm_tlsdnsconnect().
(cherry picked from commit 98f758ed4f)
The recently added TLS client session cache used
SSL_SESSION_is_resumable() to avoid polluting the cache with
non-resumable sessions. However, it turned out that we cannot provide
a shim for this function across the whole range of OpenSSL versions
due to the fact that OpenSSL 1.1.0 does uses opaque pointers for
SSL_SESSION objects.
The commit replaces the shim for SSL_SESSION_is_resumable() with a non
public approximation of it on systems shipped with OpenSSL 1.1.0. It
is not turned into a proper shim because it does not fully emulate the
behaviour of SSL_SESSION_is_resumable(), but in our case it is good
enough, as it still helps to protect the cache from pollution.
For systems shipped with OpenSSL 1.0.X and derivatives (e.g. older
versions of LibreSSL), the provided replacement perfectly mimics the
function it is intended to replace.
(cherry picked from commit 40be3c9263)
The commit fixes a corner case in client-side DoH code, when a write
attempt is done on a closing socket (session).
The change ensures that the write call-back will be called with a
proper error code (see failed_send_cb() call in client_httpsend()).
(cherry picked from commit 9abb00bb5f)
In such a case it will return UV_EINVAL (-EINVAL), leading to
aborting, as the code expects the function to succeed.
(cherry picked from commit 245f7cec2e)
This commit extends DoT code with TLS client session resumption
support implemented on top of the TLS client session cache.
(cherry picked from commit 86465c1dac)
This commit extends TLS stream code and DoH code with TLS client
session resumption support implemented on top of the TLS client
session cache.
(cherry picked from commit 90bc13a5d5)
This commit extends TLS context cache with TLS client session cache so
that an associated session cache can be stored alongside the TLS
context within the context cache.
(cherry picked from commit 987892d113)
This commit adds an implementation of a client TLS session cache. TLS
client session cache is an object which allows efficient storing and
retrieval of previously saved TLS sessions so that they can be
resumed. This object is supposed to be a foundation for implementing
TLS session resumption - a standard technique to reduce the cost of
re-establishing a connection to the remote server endpoint.
OpenSSL does server-side TLS session caching transparently by
default. However, on the client-side, a TLS session to resume must be
manually specified when establishing the TLS connection. The TLS
client session cache is precisely the foundation for that.
(cherry picked from commit 4ef40988f3)
Before the changes from this commit were introduced, the accept
callback function will get called twice when accepting connection
during two of these stages:
* when accepting the TCP connection;
* when handshake has completed.
That is clearly an error, as it should have been called only once. As
far as I understand it the mistake is a result of TLS DNS transport
being essentially a fork of TCP transport, where calling the accept
callback immediately after accepting TCP connection makes sense.
This commit fixes this mistake. It did not have any very serious
consequences because in BIND the accept callback only checks an ACL
and updates stats.
(cherry picked from commit e616d7f240)
When shutting down, the interface manager can be destroyed
before the `route_connected()` callback is called, which is
unexpected for the latter and can cause a crash.
Move the interface manager attachment code from the callback
to the place before the callback is registered using
`isc_nm_routeconnect()` function, which will make sure that
the interface manager will live at least until the callback
is called.
Make sure to detach the interface manager if the
`isc_nm_routeconnect()` function is not implemented, or when
the callback is called with a result value which differs from
`ISC_R_SUCCESS`.
(cherry picked from commit f6e729635f)
The `ns_interfacemgr_create()` function, when calling
`isc_nm_routeconnect()`, uses the newly created `ns_interfacemgr_t`
instance before initializing its reference count and the magic value.
Defer the `isc_nm_routeconnect()` call until the initializations
are complete.
(cherry picked from commit 1d93fe973b)
Under specific rare timing circumstances the uv_read_start() could
fail with UV_EINVAL when the connection is reset between the connect (or
accept) and the uv_read_start() call on the nmworker loop. Handle such
situation gracefully by propagating the errors from uv_read_start() into
upper layers, so the socket can be internally closed().
(cherry picked from commit b432d5d3bc)
The conversion of `DNS_R_PARTIALMATCH` into `DNS_R_NOTFOUND` is done
in the `dns_rbt_deletename()` function so there is no need to do that
in `dns_fwdtable_delete()`.
Add a possible return value of `ISC_R_NOSPACE` into the header file's
function description comment.
(cherry picked from commit 887aa7a290)
When processing a catalog zone member zone make sure that there is no
configured pre-existing forward zone with that name.
Refactor the `dns_fwdtable_find()` function to not alter the
`DNS_R_PARTIALMATCH` result (coming from `dns_rbt_findname()`) into
`DNS_R_SUCCESS`, so that now the caller can differentiate partial
and exact matches. Patch the calling sites to expect and process
the new return value.
(cherry picked from commit 2aff264fb1)
When processing a catalog zone update, skip processing records with
DNSSEC-related and ZONEMD types, because we are not interested in them
in the context of a catalog zone, and processing them will fail and
produce an unnecessary warning message.
(cherry picked from commit 73d6643137)
If 'prepub' is 0, this has the special meaning that no rollover is
scheduled. If so, don't log "new successor in x seconds".
(cherry picked from commit 955a69109e)
The key lifetime should not be shorter than the time it costs to
introduce the successor key, otherwise keys will be created faster than
they are removed, resulting in a large key set.
The time it takes to replace a key is determined by the publication
interval (Ipub) of the successor key and the retire interval of the
predecessor key (Iret).
For the ZSK, Ipub is the sum of the DNSKEY TTL and zone propagation
delay (and publish safety). Iret is the sum of Dsgn, the maximum zone
TTL and zone propagation delay (and retire safety). The sign delay is
the signature validity period minus the refresh interval: The time to
ensure that all existing RRsets have been re-signed with the new key.
The ZSK lifetime should be larger than both values.
For the KSK, Ipub is the sum of the DNSKEY TTL and zone propagation
delay (and publish safety). Iret is the sum of the DS TTL and parent
zone propagation delay (and retire safety). The KSK lifetime should be
larger than both values.
(cherry picked from commit 8134d46cdb)
The signatures-refresh should not near the signatures-validity value,
to prevent operational instability. Same is true when checking against
signatures-validity-dnskey.
(cherry picked from commit 82fd89107f)
The unit tests are now using a common base, which means that
lib/dns/tests/ code now has to include lib/isc/include/isc/test.h and
link with lib/isc/test.c and lib/ns/tests has to include both libisc and
libdns parts.
Instead of cross-linking code between the directories, move the
/lib/<foo>/test.c to /tests/<foo>.c and /lib/<foo>/include/<foo>test.h
to /tests/include/tests/<foo>.h and create a single libtest.la
convenience library in /tests/.
At the same time, move the /lib/<foo>/tests/ to /tests/<foo>/ (but keep
it symlinked to the old location) and adjust paths accordingly. In few
places, we are now using absolute paths instead of relative paths,
because the directory level has changed. By moving the directories
under the /tests/ directory, the test-related code is kept in a single
place and we can avoid referencing files between libns->libdns->libisc
which is unhealthy because they live in a separate Makefile-space.
In the future, the /bin/tests/ should be merged to /tests/ and symlink
kept, and the /fuzz/ directory moved to /tests/fuzz/.
(cherry picked from commit 2c3b2dabe9)
The unit tests contain a lot of duplicated code and here's an attempt
to reduce code duplication.
This commit does several things:
1. Remove #ifdef HAVE_CMOCKA - we already solve this with automake
conditionals.
2. Create a set of ISC_TEST_* and ISC_*_TEST_ macros to wrap the test
implementations, test lists, and the main test routine, so we don't
have to repeat this all over again. The macros were modeled after
libuv test suite but adapted to cmocka as the test driver.
A simple example of a unit test would be:
ISC_RUN_TEST_IMPL(test1) { assert_true(true); }
ISC_TEST_LIST_START
ISC_TEST_ENTRY(test1)
ISC_TEST_LIST_END
ISC_TEST_MAIN (Discussion: Should this be ISC_TEST_RUN ?)
For more complicated examples including group setup and teardown
functions, and per-test setup and teardown functions.
3. The macros prefix the test functions and cmocka entries, so the name
of the test can now match the tested function name, and we don't have
to append `_test` because `run_test_` is automatically prepended to
the main test function, and `setup_test_` and `teardown_test_` is
prepended to setup and teardown function.
4. Update all the unit tests to use the new syntax and fix a few bits
here and there.
5. In the future, we can separate the test declarations and test
implementations which are going to greatly help with uncluttering the
bigger unit tests like doh_test and netmgr_test, because the test
implementations are not declared static (see `ISC_RUN_TEST_DECLARE`
and `ISC_RUN_TEST_IMPL` for more details.
NOTE: This heavily relies on preprocessor macros, but the result greatly
outweighs all the negatives of using the macros. There's less
duplicated code, the tests are more uniform and the implementation can
be more flexible.
(cherry picked from commit 63fe9312ff)
Typing from libuv structure to isc_region_t is not possible, because
their sizes differ on 64 bit architectures. Little endian machines seems
to be lucky and still result in test passed. But big endian machine such
as s390x fails the test reliably.
Fix by directly creating the buffer as isc_region_t and skipping the
type conversion. More readable and still more correct.
(cherry picked from commit 057438cb45)
Make sure that the key structure is valid when calling the following
functions:
- dst_key_setexternal
- dst_key_isexternal
- dst_key_setmodified
- dst_key_ismodified
(cherry picked from commit 888ec4e0d4)
Setting the sock->write_timeout from the TCP, TCPDNS, and TLSDNS send
functions could lead to (harmless) data race when setting the value for
the first time when the isc_nm_send() function would be called from
thread not-matching the socket we are sending to. Move the setting the
sock->write_timeout to the matching async function which is always
called from the matching thread.
(cherry picked from commit 61117840c1)
Clang added support for the gcc-style fallthrough
attribute (i.e. __attribute__((fallthrough))) in version 10. However,
__has_attribute(fallthrough) will return 1 in C mode in older versions,
even though they only support the C++11 fallthrough attribute. At best,
the unsupported attribute is simply ignored; at worst, it causes errors.
The C2x fallthrough attribute has the advantages of being supported in
the broadest range of clang versions (added in version 9) and being easy
to check for support. Use C2x [[fallthrough]] attribute if possible, and
fall back to not using an attribute for clang versions that don't have
it.
Courtesy of Joshua Root
(cherry picked from commit 14c8d43863)
Add a new parameter to the dst_key structure, mark a key modified if
dst_key_(un)set[bool,num,state,time] is called. Only write out key
files during a keymgr run if the metadata has changed.
(cherry picked from commit 1da91b3ab4)
The ns_statscounter_recursclients counter was previously only
incremented or decremented if client->recursionquota was non-NULL.
This was harmless, because that value should always be non-NULL if
recursion is enabled, but it made the code slightly confusing.
(cherry picked from commit 0201eab655)
Since commit bad5a523c2, when the fetches-per-server quota
was increased or decreased, instead of the value being set to
the newly calculated quota, it was set to the *minimum* of
the new quota or 1 - which effectively meant it was always set to 1.
it should instead have been the maximum, to prevent the value from
ever dropping to zero.
(cherry picked from commit 694bc50273)
When attaching to the trampoline, the isc__trampoline_max was access
unlocked. This would not manifest under normal circumstances because we
initialize 65 trampolines by default and that's enough for most
commodity hardware, but there are ARM machines with 128+ cores where
this would be reported by ThreadSanitizer.
Add locking around the code in isc__trampoline_attach(). This also
requires the lock to leak on exit (along with memory that we already)
because a new thread might be attaching to the trampoline while we are
running the library destructor at the same time.
(cherry picked from commit 933162ae14)
This commit fixes a crash in generic TLS stream code, which could be
reproduced during some runs of the 'sslyze' tool.
The intention of this commit is twofold.
Firstly, it ensures that the TLS socket object cannot be destroyed too
early. Now it is being deleted alongside the underlying TCP socket
object.
Secondly, it ensures that the TLS socket object cannot be destroyed as
a result of calling 'tls_do_bio()' (the primary function which
performs encryption/decryption during the IO) as the code did not
expect that. This code path is fixed now.
(cherry picked from commit a696be6a2d)
inet_ntop result should always protect against empty string accepted
without an error. Make additional check to satisfy coverity scans.
(cherry picked from commit 656a0f076f)
Coverity detected issues:
- var_decl: Declaring variable "diff" without initializer.
- uninit_use_in_call: Using uninitialized value "diff.tuples.head" when
calling "dns_diff_clear".
(cherry picked from commit 67e773c93c)
The DNS catalog zones draft version 5 document requires that catalog
zones consumers must reset the member zone's internal zone state when
its unique label changes (either within the same catalog zone or
during change of ownership performed using the "coo" property).
BIND already behaves like that, and, in fact, doesn't support keeping
the zone state during change of ownership even if the unique label
has been kept the same, because BIND always removes the member zone
and adds it back during unique label renaming or change of ownership.
Document the described behavior and add a log message to inform when
unique label renaming occurs.
Add a system test case with unique label renaming.
(cherry picked from commit 2f2e02ff0c)
We check the `rdclass` to be of type IN in `dns_catz_update_process()`
function, and all the other static functions where similar checks exist
are called after (and in the result of) that function being called,
so they are effectively redundant.
(cherry picked from commit 84d3aba4f3)
The DNS catalog zones draft version 5 document describes various
situations when a catalog zones must be considered as "broken" and
not be processed.
Implement those checks in catz.c and add corresponding system tests.
(cherry picked from commit a8228d5f19)
