From c042ec70d2aa433cc1fdd7f65c06febf3dd2cd82 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= <michal@isc.org>
Date: Sat, 17 Mar 2018 00:12:21 +0100
Subject: [PATCH 1/3] Only log bumped signed serial after a successful secure
 zone update

If a raw zone is modified, but the dns_update_signaturesinc() call in
receive_secure_serial() fails, the corresponding secure zone's database
will not be modified, even though by that time a message containing the
bumped signed serial will already have been logged.  This creates
confusion, because a different secure zone version will be served than
the one announced in the logs.  Move the relevant dns_zone_log() call so
that it is only performed if the secure zone's database is modified.

(cherry picked from commit cfbc8e264d5a276fda2d1c0b15a4725cc293ba65)
(cherry picked from commit cdc7ab42b111a4e6aaaac19e86069d996ea11002)
---
 lib/dns/zone.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lib/dns/zone.c b/lib/dns/zone.c
index b427f57f63..d871700979 100644
--- a/lib/dns/zone.c
+++ b/lib/dns/zone.c
@@ -13870,6 +13870,7 @@ receive_secure_serial(isc_task_t *task, isc_event_t *event) {
 	dns_zone_t *zone;
 	dns_difftuple_t *tuple = NULL, *soatuple = NULL;
 	dns_update_log_t log = { update_log_cb, NULL };
+	isc_uint32_t newserial = 0, desired = 0;
 	isc_time_t timenow;
 
 	UNUSED(task);
@@ -13977,7 +13978,7 @@ receive_secure_serial(isc_task_t *task, isc_event_t *event) {
 				     zone->rss_newver));
 
 		if (soatuple != NULL) {
-			isc_uint32_t oldserial, newserial, desired;
+			isc_uint32_t oldserial;
 
 			CHECK(dns_db_createsoatuple(zone->rss_db,
 						    zone->rss_oldver,
@@ -13996,9 +13997,6 @@ receive_secure_serial(isc_task_t *task, isc_event_t *event) {
 					   zone->rss_newver, &zone->rss_diff));
 			CHECK(do_one_tuple(&soatuple, zone->rss_db,
 					   zone->rss_newver, &zone->rss_diff));
-			dns_zone_log(zone, ISC_LOG_INFO,
-				     "serial %u (unsigned %u)",
-				     newserial, desired);
 		} else
 			CHECK(update_soa_serial(zone->rss_db, zone->rss_newver,
 						&zone->rss_diff, zone->mctx,
@@ -14044,6 +14042,11 @@ receive_secure_serial(isc_task_t *task, isc_event_t *event) {
 	dns_db_closeversion(zone->rss_db, &zone->rss_oldver, ISC_FALSE);
 	dns_db_closeversion(zone->rss_db, &zone->rss_newver, ISC_TRUE);
 
+	if (newserial != 0) {
+		dns_zone_log(zone, ISC_LOG_INFO, "serial %u (unsigned %u)",
+			     newserial, desired);
+	}
+
  failure:
 	isc_event_free(&zone->rss_event);
 	event = ISC_LIST_HEAD(zone->rss_events);

From fcbdeed802e21bc9a761003b9dbf30514807f2dd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= <michal@isc.org>
Date: Sat, 17 Mar 2018 00:12:23 +0100
Subject: [PATCH 2/3] Apply raw zone deltas to yet unsigned secure zones

When inline signing is enabled for a zone without creating signing keys
for it, changes subsequently applied to the raw zone will not be
reflected in the secure zone due to the dns_update_signaturesinc() call
inside receive_secure_serial() failing.  Given that an inline zone will
be served (without any signatures) even with no associated signing keys
being present, keep applying raw zone deltas to the secure zone until
keys become available in an attempt to follow the principle of least
astonishment.

(cherry picked from commit 6acf326969862c68559d699be113c28dc35b35e6)
(cherry picked from commit 8a58a607723682c167d22e1bc5372e84a21e7f1f)
---
 bin/tests/system/inline/clean.sh          |  18 +++
 bin/tests/system/inline/ns2/named.conf.in |  12 ++
 bin/tests/system/inline/ns3/named.conf.in |  25 ++++
 bin/tests/system/inline/ns3/sign.sh       |  12 ++
 bin/tests/system/inline/setup.sh          |   5 +
 bin/tests/system/inline/tests.sh          | 161 +++++++++++++++++++++-
 lib/dns/zone.c                            |  11 +-
 7 files changed, 242 insertions(+), 2 deletions(-)

diff --git a/bin/tests/system/inline/clean.sh b/bin/tests/system/inline/clean.sh
index cf256d2079..2f25df33d8 100644
--- a/bin/tests/system/inline/clean.sh
+++ b/bin/tests/system/inline/clean.sh
@@ -15,6 +15,7 @@
 rm -f */named.conf
 rm -f */named.memstats
 rm -f */named.run
+rm -f */named.run.prev
 rm -f */trusted.conf
 rm -f ns1/K*
 rm -f ns1/dsset-*
@@ -28,6 +29,10 @@ rm -f ns2/inactiveksk.db
 rm -f ns2/inactiveksk.db.jnl
 rm -f ns2/inactivezsk.db
 rm -f ns2/inactivezsk.db.jnl
+rm -f ns2/nokeys.db
+rm -f ns2/nokeys.db.jnl
+rm -f ns2/removedkeys-secondary.db
+rm -f ns2/removedkeys-secondary.db.jnl
 rm -f ns2/retransfer.db
 rm -f ns2/retransfer.db.jnl
 rm -f ns2/retransfer3.db
@@ -65,10 +70,22 @@ rm -f ns3/inactivezsk.bk
 rm -f ns3/inactivezsk.bk.jnl
 rm -f ns3/inactivezsk.bk.signed
 rm -f ns3/inactivezsk.bk.signed.jnl
+rm -f ns3/nokeys.bk
+rm -f ns3/nokeys.bk.jnl
+rm -f ns3/nokeys.bk.signed
+rm -f ns3/nokeys.bk.signed.jnl
 rm -f ns3/nsec3.db
 rm -f ns3/nsec3.db.jnl
 rm -f ns3/nsec3.db.signed
 rm -f ns3/nsec3.db.signed.jnl
+rm -f ns3/removedkeys-primary.db
+rm -f ns3/removedkeys-primary.db.jnl
+rm -f ns3/removedkeys-primary.db.signed
+rm -f ns3/removedkeys-primary.db.signed.jnl
+rm -f ns3/removedkeys-secondary.bk
+rm -f ns3/removedkeys-secondary.bk.jnl
+rm -f ns3/removedkeys-secondary.bk.signed
+rm -f ns3/removedkeys-secondary.bk.signed.jnl
 rm -f ns3/retransfer.bk
 rm -f ns3/retransfer.bk.jnl
 rm -f ns3/retransfer.bk.signed
@@ -106,3 +123,4 @@ rm -f checkgost checkdsa checkecdsa
 rm -f ns3/a-file
 rm -f dig.out.*
 rm -f rndc.out.ns*
+rm -rf ns3/removedkeys
diff --git a/bin/tests/system/inline/ns2/named.conf.in b/bin/tests/system/inline/ns2/named.conf.in
index 81c2072f87..c7841d3321 100644
--- a/bin/tests/system/inline/ns2/named.conf.in
+++ b/bin/tests/system/inline/ns2/named.conf.in
@@ -73,3 +73,15 @@ zone "inactivezsk" {
 	file "inactivezsk.db";
 	allow-update { any; };
 };
+
+zone "nokeys" {
+	type master;
+	file "nokeys.db";
+	allow-update { any; };
+};
+
+zone "removedkeys-secondary" {
+	type master;
+	file "removedkeys-secondary.db";
+	allow-update { any; };
+};
diff --git a/bin/tests/system/inline/ns3/named.conf.in b/bin/tests/system/inline/ns3/named.conf.in
index 1cf78a4f41..4f0b67bfd9 100644
--- a/bin/tests/system/inline/ns3/named.conf.in
+++ b/bin/tests/system/inline/ns3/named.conf.in
@@ -137,3 +137,28 @@ zone "inactivezsk" {
 	auto-dnssec maintain;
 	file "inactivezsk.bk";
 };
+
+zone "nokeys" {
+	type slave;
+	masters { 10.53.0.2; };
+	inline-signing yes;
+	auto-dnssec maintain;
+	file "nokeys.bk";
+};
+
+zone "removedkeys-primary" {
+	type master;
+	inline-signing yes;
+	auto-dnssec maintain;
+	allow-update { any; };
+	also-notify { 10.53.0.2; };
+	file "removedkeys-primary.db";
+};
+
+zone "removedkeys-secondary" {
+	type slave;
+	masters { 10.53.0.2; };
+	inline-signing yes;
+	auto-dnssec maintain;
+	file "removedkeys-secondary.bk";
+};
diff --git a/bin/tests/system/inline/ns3/sign.sh b/bin/tests/system/inline/ns3/sign.sh
index e53681124b..7b8fa59aae 100755
--- a/bin/tests/system/inline/ns3/sign.sh
+++ b/bin/tests/system/inline/ns3/sign.sh
@@ -101,6 +101,18 @@ keyname=`$KEYGEN -q -r $RANDFILE -a RSASHA256 -b 1024 -n zone $zone`
 keyname=`$KEYGEN -q -r $RANDFILE -a RSASHA256 -b 1024 -n zone -f KSK $zone`
 $DSFROMKEY -T 1200 $keyname >> ../ns1/root.db
 
+zone=removedkeys-primary
+rm -f K${zone}.+*+*.key
+rm -f K${zone}.+*+*.private
+keyname=`$KEYGEN -q -r $RANDFILE -a RSASHA1 -b 1024 -n zone $zone`
+keyname=`$KEYGEN -q -r $RANDFILE -a RSASHA1 -b 1024 -n zone -f KSK $zone`
+
+zone=removedkeys-secondary
+rm -f K${zone}.+*+*.key
+rm -f K${zone}.+*+*.private
+keyname=`$KEYGEN -q -r $RANDFILE -a RSASHA1 -b 1024 -n zone $zone`
+keyname=`$KEYGEN -q -r $RANDFILE -a RSASHA1 -b 1024 -n zone -f KSK $zone`
+
 for s in a c d h k l m q z
 do
 	zone=test-$s
diff --git a/bin/tests/system/inline/setup.sh b/bin/tests/system/inline/setup.sh
index f3f1abaafa..434ac5544a 100644
--- a/bin/tests/system/inline/setup.sh
+++ b/bin/tests/system/inline/setup.sh
@@ -26,6 +26,8 @@ touch ns2/trusted.conf
 cp ns2/bits.db.in ns2/bits.db
 cp ns2/bits.db.in ns2/inactiveksk.db
 cp ns2/bits.db.in ns2/inactivezsk.db
+cp ns2/bits.db.in ns2/nokeys.db
+cp ns2/bits.db.in ns2/removedkeys-secondary.db
 cp ns2/bits.db.in ns2/retransfer.db
 cp ns2/bits.db.in ns2/retransfer3.db
 rm -f ns2/bits.db.jnl
@@ -36,6 +38,9 @@ cp ns3/master.db.in ns3/updated.db
 cp ns3/master.db.in ns3/expired.db
 cp ns3/master.db.in ns3/nsec3.db
 cp ns3/master.db.in ns3/externalkey.db
+cp ns3/master.db.in ns3/removedkeys-primary.db
+
+mkdir ns3/removedkeys
 
 touch ns4/trusted.conf
 cp ns4/noixfr.db.in ns4/noixfr.db
diff --git a/bin/tests/system/inline/tests.sh b/bin/tests/system/inline/tests.sh
index 63d9c3c1d5..f6adf03c4b 100755
--- a/bin/tests/system/inline/tests.sh
+++ b/bin/tests/system/inline/tests.sh
@@ -1003,14 +1003,173 @@ grep "DNSKEY 8 1 [0-9]* [0-9]* [0-9]* ${kskid} " dig.out.ns3.test$n > /dev/null
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=`expr $status + $ret`
 
+# Wait until an update to the raw part of a given inline signed zone is fully
+# processed.  As waiting for a fixed amount of time is suboptimal and there is
+# no single message that would signify both a successful modification and an
+# error in a race-free manner, instead wait until either notifies are sent
+# (which means the secure zone was modified) or a receive_secure_serial() error
+# is logged (which means the zone was not modified and will not be modified any
+# further in response to the relevant raw zone update).
+wait_until_raw_zone_update_is_processed() {
+	zone="$1"
+	for i in 1 2 3 4 5 6 7 8 9 10
+	do
+		if nextpart ns3/named.run | egrep "zone ${zone}.*(sending notifies|receive_secure_serial)" > /dev/null; then
+			return
+		fi
+		sleep 1
+	done
+}
+
+n=`expr $n + 1`
+echo_i "checking that changes to raw zone are applied to a previously unsigned secure zone ($n)"
+ret=0
+# Query for bar.nokeys/A and ensure the response is negative.  As this zone
+# does not have any signing keys set up, the response must be unsigned.
+$DIG $DIGOPTS @10.53.0.3 bar.nokeys. A > dig.out.ns3.pre.test$n 2>&1 || ret=1
+grep "status: NOERROR" dig.out.ns3.pre.test$n > /dev/null && ret=1
+grep "RRSIG" dig.out.ns3.pre.test$n > /dev/null && ret=1
+# Ensure the wait_until_raw_zone_update_is_processed() call below will ignore
+# log messages generated before the raw zone is updated.
+nextpart ns3/named.run > /dev/null
+# Add a record to the raw zone on the master.
+$NSUPDATE << EOF || ret=1
+zone nokeys.
+server 10.53.0.2 ${PORT}
+update add bar.nokeys. 0 A 127.0.0.1
+send
+EOF
+wait_until_raw_zone_update_is_processed "nokeys"
+# Query for bar.nokeys/A again and ensure the signer now returns a positive,
+# yet still unsigned response.
+$DIG $DIGOPTS @10.53.0.3 bar.nokeys. A > dig.out.ns3.post.test$n 2>&1
+grep "status: NOERROR" dig.out.ns3.post.test$n > /dev/null || ret=1
+grep "RRSIG" dig.out.ns3.pre.test$n > /dev/null && ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=`expr $status + $ret`
+
+n=`expr $n + 1`
+echo_i "checking that changes to raw zone are not applied to a previously signed secure zone with no keys available (primary) ($n)"
+ret=0
+# Query for bar.removedkeys-primary/A and ensure the response is negative.  As
+# this zone has signing keys set up, the response must be signed.
+$DIG $DIGOPTS @10.53.0.3 bar.removedkeys-primary. A > dig.out.ns3.pre.test$n 2>&1 || ret=1
+grep "status: NOERROR" dig.out.ns3.pre.test$n > /dev/null && ret=1
+grep "RRSIG" dig.out.ns3.pre.test$n > /dev/null || ret=1
+# Remove the signing keys for this zone.
+mv -f ns3/Kremovedkeys-primary* ns3/removedkeys
+# Ensure the wait_until_raw_zone_update_is_processed() call below will ignore
+# log messages generated before the raw zone is updated.
+nextpart ns3/named.run > /dev/null
+# Add a record to the raw zone on the master.
+$NSUPDATE << EOF || ret=1
+zone removedkeys-primary.
+server 10.53.0.3 ${PORT}
+update add bar.removedkeys-primary. 0 A 127.0.0.1
+send
+EOF
+wait_until_raw_zone_update_is_processed "removedkeys-primary"
+# Query for bar.removedkeys-primary/A again and ensure the signer still returns
+# a negative, signed response.
+$DIG $DIGOPTS @10.53.0.3 bar.removedkeys-primary. A > dig.out.ns3.post.test$n 2>&1
+grep "status: NOERROR" dig.out.ns3.post.test$n > /dev/null && ret=1
+grep "RRSIG" dig.out.ns3.pre.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=`expr $status + $ret`
+
+n=`expr $n + 1`
+echo_i "checking that backlogged changes to raw zone are applied after keys become available (primary) ($n)"
+ret=0
+# Restore the signing keys for this zone.
+mv ns3/removedkeys/Kremovedkeys-primary* ns3
+$RNDCCMD 10.53.0.3 loadkeys removedkeys-primary > /dev/null 2>&1
+# Determine what a SOA record with a bumped serial number should look like.
+BUMPED_SOA=`sed -n 's/.*\(add removedkeys-primary.*IN.*SOA\)/\1/p;' ns3/named.run | tail -1 | awk '{$8 += 1; print $0}'`
+# Ensure the wait_until_raw_zone_update_is_processed() call below will ignore
+# log messages generated before the raw zone is updated.
+nextpart ns3/named.run > /dev/null
+# Bump the SOA serial number of the raw zone.
+$NSUPDATE << EOF || ret=1
+zone removedkeys-primary.
+server 10.53.0.3 ${PORT}
+update del removedkeys-primary. SOA
+update ${BUMPED_SOA}
+send
+EOF
+wait_until_raw_zone_update_is_processed "removedkeys-primary"
+# Query for bar.removedkeys-primary/A again and ensure the signer now returns a
+# positive, signed response.
+$DIG $DIGOPTS @10.53.0.3 bar.removedkeys-primary. A > dig.out.ns3.test$n 2>&1
+grep "status: NOERROR" dig.out.ns3.test$n > /dev/null || ret=1
+grep "RRSIG" dig.out.ns3.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=`expr $status + $ret`
+
+n=`expr $n + 1`
+echo_i "checking that changes to raw zone are not applied to a previously signed secure zone with no keys available (secondary) ($n)"
+ret=0
+# Query for bar.removedkeys-secondary/A and ensure the response is negative.  As this
+# zone does have signing keys set up, the response must be signed.
+$DIG $DIGOPTS @10.53.0.3 bar.removedkeys-secondary. A > dig.out.ns3.pre.test$n 2>&1 || ret=1
+grep "status: NOERROR" dig.out.ns3.pre.test$n > /dev/null && ret=1
+grep "RRSIG" dig.out.ns3.pre.test$n > /dev/null || ret=1
+# Remove the signing keys for this zone.
+mv -f ns3/Kremovedkeys-secondary* ns3/removedkeys
+# Ensure the wait_until_raw_zone_update_is_processed() call below will ignore
+# log messages generated before the raw zone is updated.
+nextpart ns3/named.run > /dev/null
+# Add a record to the raw zone on the master.
+$NSUPDATE << EOF || ret=1
+zone removedkeys-secondary.
+server 10.53.0.2 ${PORT}
+update add bar.removedkeys-secondary. 0 A 127.0.0.1
+send
+EOF
+wait_until_raw_zone_update_is_processed "removedkeys-secondary"
+# Query for bar.removedkeys-secondary/A again and ensure the signer still returns a
+# negative, signed response.
+$DIG $DIGOPTS @10.53.0.3 bar.removedkeys-secondary. A > dig.out.ns3.post.test$n 2>&1
+grep "status: NOERROR" dig.out.ns3.post.test$n > /dev/null && ret=1
+grep "RRSIG" dig.out.ns3.pre.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=`expr $status + $ret`
+
+n=`expr $n + 1`
+echo_i "checking that backlogged changes to raw zone are applied after keys become available (secondary) ($n)"
+ret=0
+# Restore the signing keys for this zone.
+mv ns3/removedkeys/Kremovedkeys-secondary* ns3
+$RNDCCMD 10.53.0.3 loadkeys removedkeys-secondary > /dev/null 2>&1
+# Determine what a SOA record with a bumped serial number should look like.
+BUMPED_SOA=`sed -n 's/.*\(add removedkeys-secondary.*IN.*SOA\)/\1/p;' ns2/named.run | tail -1 | awk '{$8 += 1; print $0}'`
+# Ensure the wait_until_raw_zone_update_is_processed() call below will ignore
+# log messages generated before the raw zone is updated.
+nextpart ns3/named.run > /dev/null
+# Bump the SOA serial number of the raw zone on the master.
+$NSUPDATE << EOF || ret=1
+zone removedkeys-secondary.
+server 10.53.0.2 ${PORT}
+update del removedkeys-secondary. SOA
+update ${BUMPED_SOA}
+send
+EOF
+wait_until_raw_zone_update_is_processed "removedkeys-secondary"
+# Query for bar.removedkeys-secondary/A again and ensure the signer now returns
+# a positive, signed response.
+$DIG $DIGOPTS @10.53.0.3 bar.removedkeys-secondary. A > dig.out.ns3.test$n 2>&1
+grep "status: NOERROR" dig.out.ns3.test$n > /dev/null || ret=1
+grep "RRSIG" dig.out.ns3.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=`expr $status + $ret`
+
 n=`expr $n + 1`
 echo_i "check that zonestatus reports 'type: master' for a inline master zone ($n)"
 ret=0
 $RNDCCMD 10.53.0.3 zonestatus master > rndc.out.ns3.test$n
 grep "type: master" rndc.out.ns3.test$n > /dev/null || ret=1
 if [ $ret != 0 ]; then echo_i "failed"; fi
-
 status=`expr $status + $ret`
+
 n=`expr $n + 1`
 echo_i "check that zonestatus reports 'type: slave' for a inline slave zone ($n)"
 ret=0
diff --git a/lib/dns/zone.c b/lib/dns/zone.c
index d871700979..b83df7f500 100644
--- a/lib/dns/zone.c
+++ b/lib/dns/zone.c
@@ -14015,8 +14015,17 @@ receive_secure_serial(isc_task_t *task, isc_event_t *event) {
 		fprintf(stderr, "looping on dns_update_signaturesinc\n");
 		return;
 	}
-	if (result != ISC_R_SUCCESS)
+	/*
+	 * If something went wrong while trying to update the secure zone and
+	 * the latter was already signed before, do not apply raw zone deltas
+	 * to it as that would break existing DNSSEC signatures.  However, if
+	 * the secure zone was not yet signed (e.g. because no signing keys
+	 * were created for it), commence applying raw zone deltas to it so
+	 * that contents of the raw zone and the secure zone are kept in sync.
+	 */
+	if (result != ISC_R_SUCCESS && dns_db_issecure(zone->rss_db)) {
 		goto failure;
+	}
 
 	if (rjournal == NULL)
 		CHECK(dns_journal_open(zone->rss_raw->mctx,

From 821c27bb7cbf13d17ca523d57847ecf8281b1948 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= <michal@isc.org>
Date: Sat, 17 Mar 2018 00:12:24 +0100
Subject: [PATCH 3/3] Add CHANGES entries

4916.	[bug]		Not creating signing keys for an inline signed zone
			prevented changes applied to the raw zone from being
			reflected in the secure zone until signing keys were
			made available. [GL #159]

4915.	[bug]		Bumped signed serial of an inline signed zone was
			logged even when an error occurred while updating
			signatures. [GL #159]

(cherry picked from commit 7d2c09c9050d532ea4504d6f344fe731d95c448e)
(cherry picked from commit e4995efe24a65d43d3e18eb7f42df4ba9101e5c4)
---
 CHANGES | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/CHANGES b/CHANGES
index 9bdb9e8abf..26f466e2fd 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,3 +1,12 @@
+4933.	[bug]		Not creating signing keys for an inline signed zone
+			prevented changes applied to the raw zone from being
+			reflected in the secure zone until signing keys were
+			made available. [GL #159]
+
+4932.	[bug]		Bumped signed serial of an inline signed zone was
+			logged even when an error occurred while updating
+			signatures. [GL #159]
+
 4918.	[bug]		Fix double free after keygen error in dnssec-keygen
 			when OpenSSL >= 1.1.0 is used and RSA_generate_key_ex
 			fails. [GL #109]