From ab49205af37ffafb70a3ba112897152f31a7a83d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Tue, 4 Jan 2022 15:41:46 +0100 Subject: [PATCH 1/5] Check unsigned serial number in signed zone files All signed zone files present in bin/tests/system/inline/ns8 should contain the unsigned serial number in the raw-format header. Add a check to ensure that is the case. Extend the dnssec-signzone command line in ns8/sign.sh with the -L option to allow the zones initially signed there to pass the newly added check. Add another zone to the configuration for the ns8 named instance to ensure the check also passes when multiple zones are inline-signed by a single named instance. --- bin/tests/system/inline/ns8/named.conf.in | 7 ++ bin/tests/system/inline/ns8/sign.sh | 15 ++-- .../system/inline/tests_signed_zone_files.py | 68 +++++++++++++++++++ util/copyrights | 1 + 4 files changed, 84 insertions(+), 7 deletions(-) create mode 100755 bin/tests/system/inline/tests_signed_zone_files.py diff --git a/bin/tests/system/inline/ns8/named.conf.in b/bin/tests/system/inline/ns8/named.conf.in index 242c4dc870..b6ee1bf16a 100644 --- a/bin/tests/system/inline/ns8/named.conf.in +++ b/bin/tests/system/inline/ns8/named.conf.in @@ -151,3 +151,10 @@ zone example { auto-dnssec maintain; file "example.db"; }; + +zone "unsigned-serial-test" { + type primary; + inline-signing yes; + auto-dnssec maintain; + file "unsigned-serial-test.db"; +}; diff --git a/bin/tests/system/inline/ns8/sign.sh b/bin/tests/system/inline/ns8/sign.sh index 9033c72079..d8702fd2bc 100755 --- a/bin/tests/system/inline/ns8/sign.sh +++ b/bin/tests/system/inline/ns8/sign.sh @@ -21,12 +21,13 @@ do keyname=`$KEYGEN -q -a $DEFAULT_ALGORITHM -b $DEFAULT_BITS -n zone $zone` keyname=`$KEYGEN -q -a $DEFAULT_ALGORITHM -b $DEFAULT_BITS -n zone -f KSK $zone` cp example.com.db.in ${zone}.db - $SIGNER -S -T 3600 -O raw -o ${zone} ${zone}.db > /dev/null 2>&1 + $SIGNER -S -T 3600 -O raw -L 2000042407 -o ${zone} ${zone}.db > /dev/null 2>&1 done -zone=example -rm -f K${zone}.+*+*.key -rm -f K${zone}.+*+*.private -keyname=`$KEYGEN -q -a $DEFAULT_ALGORITHM -b $DEFAULT_BITS -n zone $zone` -keyname=`$KEYGEN -q -a $DEFAULT_ALGORITHM -b $DEFAULT_BITS -n zone -f KSK $zone` -cp ${zone}.db.in ${zone}.db +for zone in example unsigned-serial-test; do + rm -f K${zone}.+*+*.key + rm -f K${zone}.+*+*.private + keyname=`$KEYGEN -q -a $DEFAULT_ALGORITHM -b $DEFAULT_BITS -n zone $zone` + keyname=`$KEYGEN -q -a $DEFAULT_ALGORITHM -b $DEFAULT_BITS -n zone -f KSK $zone` + cp example.db.in ${zone}.db +done diff --git a/bin/tests/system/inline/tests_signed_zone_files.py b/bin/tests/system/inline/tests_signed_zone_files.py new file mode 100755 index 0000000000..0abbb987be --- /dev/null +++ b/bin/tests/system/inline/tests_signed_zone_files.py @@ -0,0 +1,68 @@ +############################################################################ +# Copyright (C) Internet Systems Consortium, Inc. ("ISC") +# +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, you can obtain one at https://mozilla.org/MPL/2.0/. +# +# See the COPYRIGHT file distributed with this work for additional +# information regarding copyright ownership. +############################################################################ + +import glob +import struct + + +class RawFormatHeader(dict): + ''' + A dictionary of raw-format header fields read from a zone file. + ''' + + fields = [ + 'format', + 'version', + 'dumptime', + 'flags', + 'sourceserial', + 'lastxfrin', + ] + + def __init__(self, file_name): + header = struct.Struct('>IIIIII') + with open(file_name, 'rb') as data: + header_data = data.read(header.size) + super().__init__(zip(self.fields, header.unpack_from(header_data))) + + +def test_unsigned_serial_number(): + + ''' + Check whether all signed zone files in the "ns8" subdirectory contain the + serial number of the unsigned version of the zone in the raw-format header. + The test assumes that all "*.signed" files in the "ns8" subdirectory are in + raw format. + + Notes: + + - The actual zone signing and dumping happens while the tests.sh phase of + the "inline" system test is set up and run. This check only verifies + the outcome of those events; it does not initiate any signing or + dumping itself. + + - example[0-9][0-9].com.db.signed files are initially signed by + dnssec-signzone while the others - by named. + ''' + + zones_with_unsigned_serial_missing = [] + + for signed_zone in sorted(glob.glob('ns8/*.signed')): + raw_header = RawFormatHeader(signed_zone) + # Ensure the unsigned serial number is placed where it is expected. + assert raw_header['format'] == 2 + assert raw_header['version'] == 1 + # Check whether the header flags indicate that the unsigned serial + # number is set and that the latter is indeed set. + if raw_header['flags'] & 0x02 == 0 or raw_header['sourceserial'] == 0: + zones_with_unsigned_serial_missing.append(signed_zone) + + assert not zones_with_unsigned_serial_missing diff --git a/util/copyrights b/util/copyrights index e57b83189e..b1752be8ce 100644 --- a/util/copyrights +++ b/util/copyrights @@ -426,6 +426,7 @@ ./bin/tests/system/inline/ns8/sign.sh SH 2020,2021,2022 ./bin/tests/system/inline/setup.sh SH 2011,2012,2013,2014,2016,2017,2018,2019,2020,2021,2022 ./bin/tests/system/inline/tests.sh SH 2011,2012,2013,2014,2016,2017,2018,2019,2020,2021,2022 +./bin/tests/system/inline/tests_signed_zone_files.py PYTHON 2022 ./bin/tests/system/integrity/clean.sh SH 2017,2018,2019,2020,2021,2022 ./bin/tests/system/integrity/setup.sh SH 2018,2019,2020,2021,2022 ./bin/tests/system/integrity/tests.sh SH 2017,2018,2019,2020,2021,2022 From 1064b2fc47d1835a9aee03076190200c352543a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 5 Jan 2022 17:49:59 +0100 Subject: [PATCH 2/5] Revert "Ensure the correct ordering zone_shutdown() vs zone_gotwritehandle()" This reverts commit cc1d4e1aa63fdc7e82a00fd7b06d50fb4dbc8e96. --- lib/dns/zone.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 2241f99089..2e54038b3c 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -11947,9 +11947,6 @@ dump_done(void *arg, isc_result_t result) { dns_dumpctx_detach(&zone->dctx); } zonemgr_putio(&zone->writeio); - if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_SHUTDOWN) && zone->raw != NULL) { - dns_zone_detach(&zone->raw); - } UNLOCK_ZONE(zone); if (again) { (void)zone_dump(zone, false); @@ -15031,7 +15028,7 @@ zone_shutdown(isc_task_t *task, isc_event_t *event) { */ DNS_ZONE_SETFLAG(zone, DNS_ZONEFLG_SHUTDOWN); free_needed = exit_check(zone); - if (inline_secure(zone) && !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) { + if (inline_secure(zone)) { raw = zone->raw; zone->raw = NULL; } From ef625f5f0689456c5759592292810c673b41fce3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Tue, 4 Jan 2022 15:41:46 +0100 Subject: [PATCH 3/5] Do not detach raw zone until dumping is complete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the signed version of an inline-signed zone is dumped to disk, the serial number of the unsigned version of the zone is stored in the raw-format header so that the contents of the signed zone can be resynchronized after named restart if the unsigned zone file is modified while named is not running. In order for the serial number of the unsigned zone to be determined during the dump, zone->raw must be set to a non-NULL value. This should always be the case as long as the signed version of the zone is used for anything by named. However, a scenario exists in which the signed version of the zone has zone->raw set to NULL while it is being dumped: 1. Zone dump is requested; zone_dump() is invoked. 2. Another zone dump is already in progress, so the dump gets deferred until I/O is available (see zonemgr_getio()). 3. The last external reference to the zone is released. zone_shutdown() gets queued to the zone's task. 4. I/O becomes available for zone dumping. zone_gotwritehandle() gets queued to the zone's task. 5. The zone's task runs zone_shutdown(). zone->raw gets set to NULL. 6. The zone's task runs zone_gotwritehandle(). zone->raw is determined to be NULL, causing the serial number of the unsigned version of the zone to be omitted from the raw-format dump of the signed zone file. Note that the naïve solution - deferring the dns_zone_detach() call for zone->raw until zone_free() gets called for the secure version of the zone - does not work because it leads to a chicken-and-egg problem when the inline-signed zone is about to get freed: the raw zone holds a weak reference to the secure zone and that reference does not get released until the reference count for the raw zone reaches zero, which in turn would not happen until all weak references to the secure zone were released. Defer detaching from zone->raw in zone_shutdown() if the zone is in the process of being dumped to disk. Ensure zone->raw gets detached from after the dump is finished if detaching gets deferred. Prevent zone dumping from being requeued upon failure if the zone is in the process of being cleaned up as it opens up possibilities for the zone->raw reference to leak, triggering a shutdown hang. --- lib/dns/zone.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 2e54038b3c..4c79635193 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -11925,7 +11925,22 @@ dump_done(void *arg, isc_result_t result) { if (compact) { DNS_ZONE_SETFLAG(zone, DNS_ZONEFLG_NEEDCOMPACT); } - if (result != ISC_R_SUCCESS && result != ISC_R_CANCELED) { + if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_SHUTDOWN)) { + /* + * If DNS_ZONEFLG_SHUTDOWN is set, all external references to + * the zone are gone, which means it is in the process of being + * cleaned up, so do not reschedule dumping. + * + * Detach from the raw version of the zone in case this + * operation has been deferred in zone_shutdown(). + */ + if (zone->raw != NULL) { + dns_zone_detach(&zone->raw); + } + if (result == ISC_R_SUCCESS) { + DNS_ZONE_CLRFLAG(zone, DNS_ZONEFLG_FLUSH); + } + } else if (result != ISC_R_SUCCESS && result != ISC_R_CANCELED) { /* * Try again in a short while. */ @@ -15028,7 +15043,13 @@ zone_shutdown(isc_task_t *task, isc_event_t *event) { */ DNS_ZONE_SETFLAG(zone, DNS_ZONEFLG_SHUTDOWN); free_needed = exit_check(zone); - if (inline_secure(zone)) { + /* + * If a dump is in progress for the secure zone, defer detaching from + * the raw zone as it may prevent the unsigned serial number from being + * stored in the raw-format dump of the secure zone. In this scenario, + * dump_done() takes care of cleaning up the zone->raw reference. + */ + if (inline_secure(zone) && !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) { raw = zone->raw; zone->raw = NULL; } From 5f369481763407a6b3d9495743ea6be566c09d5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 5 Jan 2022 17:50:51 +0100 Subject: [PATCH 4/5] Revert "Add CHANGES and release note for [GL #3071]" This reverts commit 7a6365d02875ca1344013ad16ae2d33a51307bec. --- CHANGES | 6 ------ doc/notes/notes-current.rst | 6 ------ 2 files changed, 12 deletions(-) diff --git a/CHANGES b/CHANGES index 0c40162026..c64bd9437c 100644 --- a/CHANGES +++ b/CHANGES @@ -5,12 +5,6 @@ 5787. [doc] Update 'auto-dnssec' documentation, it may only be activated at zone level. [GL #3023] -5786. [bug] Defer detaching from zone->raw in zone_shutdown() if - the zone is in the process of being dumped to disk to - ensure that the unsigned serial number information is - always written in the raw-format header of the signed - version on an inline-signed zone. [GL #3071] - 5785. [bug] named could leak memory when two dnssec-policy clauses had the same name. named failed to log this error. [GL #3085] diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 5241f5558f..dbf3266abd 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -43,9 +43,3 @@ Bug Fixes - On FreeBSD, a TCP connection would leak a small amount of heap memory leading to out-of-memory problem in a long run. This has been fixed. :gl:`#3051` - -- Under certain circumstances, the signed version of an inline-signed zone could - be dumped to disk without the serial number of the unsigned version of the - zone being saved. This could prevent resynchronization of zone contents after - ``named`` restarted, if the unsigned zone file had been modified while - ``named`` was not running. This has been fixed. :gl:`#3071` From ff8d37cbdb020e696c6b396e3342c786ada6d81d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 3 Jan 2022 13:30:40 +0100 Subject: [PATCH 5/5] Add CHANGES and release note for [GL #3071] --- CHANGES | 6 ++++++ doc/notes/notes-current.rst | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/CHANGES b/CHANGES index c64bd9437c..0e6b6307ba 100644 --- a/CHANGES +++ b/CHANGES @@ -5,6 +5,12 @@ 5787. [doc] Update 'auto-dnssec' documentation, it may only be activated at zone level. [GL #3023] +5786. [bug] Defer detaching from zone->raw in zone_shutdown() if + the zone is in the process of being dumped to disk, to + ensure that the unsigned serial number information is + always written in the raw-format header of the signed + version on an inline-signed zone. [GL #3071] + 5785. [bug] named could leak memory when two dnssec-policy clauses had the same name. named failed to log this error. [GL #3085] diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index dbf3266abd..741b70453b 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -43,3 +43,9 @@ Bug Fixes - On FreeBSD, a TCP connection would leak a small amount of heap memory leading to out-of-memory problem in a long run. This has been fixed. :gl:`#3051` + +- Under certain circumstances, the signed version of an inline-signed + zone could be dumped to disk without the serial number of the unsigned + version of the zone, preventing resynchronization of zone contents + after ``named`` restart in case the unsigned zone file gets modified + while ``named`` is not running. This has been fixed. :gl:`#3071`