Merge branch '2725-nsec3param-changes-on-restart' into 'main'

Fix NSEC3 resalt on restart

Closes #2725

See merge request isc-projects/bind9!5126

CHANGES

@@ -1,3 +1,7 @@
+5653.	[bug]		Fixed a bug that caused the NSEC3 salt to be changed
+			for KASP zones on restart.
+			[GL #2725]
+
 5652.	[bug]		Copy and paste error caused the socket option to
 			be enabled instead of disabled. [GL #2746]
 

bin/tests/system/nsec3/tests.sh

@@ -41,8 +41,8 @@ set_nsec3param() {
 	FLAGS=$1
 	ITERATIONS=$2
 	SALTLEN=$3
+	# Reset salt.
 	SALT=""
-	test "$SALTLEN" = "0" && SALT="-"
 }
 
 # The apex NSEC3PARAM record indicates that it is signed.
@@ -115,6 +115,10 @@ _check_nsec3_nsec3param()
 {
 	dig_with_opts +noquestion @$SERVER "${ZONE}" NSEC3PARAM > "dig.out.test$n.nsec3param.$ZONE" || return 1
 	grep "${ZONE}.*0.*IN.*NSEC3PARAM.*1.*0.*${ITERATIONS}.*${SALT}" "dig.out.test$n.nsec3param.$ZONE" > /dev/null || return 1
+
+	if [ -z "$SALT" ]; then
+		SALT=`awk '$4 == "NSEC3PARAM" { print $8 }' dig.out.test$n.nsec3param.$ZONE`
+	fi
 	return 0
 }
 
@@ -128,14 +132,14 @@ _check_nsec3_nxdomain()
 check_nsec3()
 {
 	n=$((n+1))
-	echo_i "check NSEC3PARAM response for zone ${ZONE} ($n)"
+	echo_i "check that NSEC3PARAM 1 0 ${ITERATIONS} is published zone ${ZONE} ($n)"
 	ret=0
 	retry_quiet 10 _check_nsec3_nsec3param || log_error "bad NSEC3PARAM response for ${ZONE}"
 	test "$ret" -eq 0 || echo_i "failed"
 	status=$((status+ret))
 
 	n=$((n+1))
-	echo_i "check NSEC3 response for zone ${ZONE} ($n)"
+	echo_i "check NXDOMAIN response has correct NSEC3 1 ${FLAGS} ${ITERATIONS} ${SALT} for zone ${ZONE} ($n)"
 	ret=0
 	retry_quiet 10 _check_nsec3_nxdomain || log_error "bad NXDOMAIN response for zone ${ZONE}"
 	test "$ret" -eq 0 || echo_i "failed"
@@ -169,24 +173,28 @@ dnssec_verify
 
 # Zone: nsec3-change.kasp.
 set_zone_policy "nsec3-change.kasp" "nsec3"
+set_nsec3param "0" "5" "8"
 echo_i "initial check zone ${ZONE}"
 check_nsec3
 dnssec_verify
 
 # Zone: nsec3-dynamic-change.kasp.
 set_zone_policy "nsec3-dynamic-change.kasp" "nsec3"
+set_nsec3param "0" "5" "8"
 echo_i "initial check zone ${ZONE}"
 check_nsec3
 dnssec_verify
 
 # Zone: nsec3-to-nsec.kasp.
 set_zone_policy "nsec3-to-nsec.kasp" "nsec3"
+set_nsec3param "0" "5" "8"
 echo_i "initial check zone ${ZONE}"
 check_nsec3
 dnssec_verify
 
 # Zone: nsec3-to-optout.kasp.
 set_zone_policy "nsec3-to-optout.kasp" "nsec3"
+set_nsec3param "0" "5" "8"
 echo_i "initial check zone ${ZONE}"
 check_nsec3
 dnssec_verify
@@ -205,7 +213,6 @@ echo_i "initial check zone ${ZONE}"
 check_nsec3
 dnssec_verify
 
-
 # Reconfig named.
 echo_i "reconfig dnssec-policy to trigger nsec3 rollovers"
 copy_setports ns3/named2.conf.in ns3/named.conf
@@ -221,12 +228,14 @@ dnssec_verify
 
 # Zone: nsec3.kasp. (same)
 set_zone_policy "nsec3.kasp" "nsec3"
+set_nsec3param "0" "5" "8"
 echo_i "check zone ${ZONE} after reconfig"
 check_nsec3
 dnssec_verify
 
 # Zone: nsec3-dyamic.kasp. (same)
 set_zone_policy "nsec3-dynamic.kasp" "nsec3"
+set_nsec3param "0" "5" "8"
 echo_i "check zone ${ZONE} after reconfig"
 check_nsec3
 dnssec_verify
@@ -247,6 +256,7 @@ dnssec_verify
 
 # Zone: nsec3-to-nsec.kasp. (reconfigured)
 set_zone_policy "nsec3-to-nsec.kasp" "nsec"
+set_nsec3param "1" "11" "0"
 echo_i "check zone ${ZONE} after reconfig"
 check_nsec
 dnssec_verify
@@ -286,5 +296,33 @@ grep "zone uses dnssec-policy, use rndc dnssec command instead" rndc.signing.tes
 check_nsec3
 dnssec_verify
 
+# Test NSEC3 and NSEC3PARAM is the same after restart
+set_zone_policy "nsec3.kasp" "nsec3"
+set_nsec3param "0" "5" "8"
+echo_i "check zone ${ZONE} before restart"
+check_nsec3
+dnssec_verify
+
+# Restart named, NSEC3 should stay the same.
+ret=0
+echo "stop ns3"
+$PERL ../stop.pl --use-rndc --port ${CONTROLPORT} nsec3 ${DIR} || ret=1
+test "$ret" -eq 0 || echo_i "failed"
+status=$((status+ret))
+
+ret=0
+echo "start ns3"
+start_server --noclean --restart --port ${PORT} nsec3 ${DIR}
+test "$ret" -eq 0 || echo_i "failed"
+status=$((status+ret))
+
+prevsalt="${SALT}"
+set_zone_policy "nsec3.kasp" "nsec3"
+set_nsec3param "0" "5" "8"
+SALT="${prevsalt}"
+echo_i "check zone ${ZONE} after restart has salt ${SALT}"
+check_nsec3
+dnssec_verify
+
 echo_i "exit status: $status"
 [ $status -eq 0 ] || exit 1

doc/notes/notes-current.rst

@@ -76,3 +76,6 @@ Bug Fixes
 - Fix an error that would enable don't fragment socket option instead
   of disabling it leading to errors when sending the oversized UDP
   packets. [GL #2746]
+
+- Fixed a bug that caused the NSEC salt to be changed for KASP zones on
+  every startup. :gl:`#2725`

lib/dns/zone.c

@@ -959,10 +959,13 @@ static const char *dbargv_default[] = { "rbt" };
 
 typedef struct nsec3param nsec3param_t;
 struct nsec3param {
+	dns_rdata_nsec3param_t rdata;
 	unsigned char data[DNS_NSEC3PARAM_BUFFERSIZE + 1];
 	unsigned int length;
 	bool nsec;
 	bool replace;
+	bool resalt;
+	bool lookup;
 	ISC_LINK(nsec3param_t) link;
 };
 typedef ISC_LIST(nsec3param_t) nsec3paramlist_t;
@@ -21250,6 +21253,25 @@ setnsec3param(isc_task_t *task, isc_event_t *event) {
 	dns_zone_idetach(&zone);
 }
 
+static void
+salt2text(unsigned char *salt, uint8_t saltlen, unsigned char *text,
+	  unsigned int textlen) {
+	isc_region_t r;
+	isc_buffer_t buf;
+	isc_result_t result;
+
+	r.base = salt;
+	r.length = (unsigned int)saltlen;
+
+	isc_buffer_init(&buf, text, textlen);
+	result = isc_hex_totext(&r, 2, "", &buf);
+	if (result == ISC_R_SUCCESS) {
+		text[saltlen * 2] = 0;
+	} else {
+		text[0] = 0;
+	}
+}
+
 /*
  * Check whether NSEC3 chain addition or removal specified by the private-type
  * record passed with the event was already queued (or even fully performed).
@@ -21301,6 +21323,53 @@ rss_post(dns_zone_t *zone, isc_event_t *event) {
 
 	CHECK(dns_db_getoriginnode(db, &node));
 
+	/*
+	 * Do we need to look up the NSEC3 parameters?
+	 */
+	if (np->lookup) {
+		dns_rdata_nsec3param_t param;
+		dns_rdata_t nrdata = DNS_RDATA_INIT;
+		dns_rdata_t prdata = DNS_RDATA_INIT;
+		unsigned char nbuf[DNS_NSEC3PARAM_BUFFERSIZE];
+		unsigned char saltbuf[255];
+		isc_buffer_t b;
+
+		param.salt = NULL;
+		result = dns__zone_lookup_nsec3param(zone, &np->rdata, &param,
+						     saltbuf, np->resalt);
+		if (result == ISC_R_SUCCESS) {
+			/*
+			 * Success because the NSEC3PARAM already exists, but
+			 * function returns void, so goto failure to clean up.
+			 */
+			goto failure;
+		}
+		if (result != DNS_R_NSEC3RESALT && result != ISC_R_NOTFOUND) {
+			dnssec_log(zone, ISC_LOG_DEBUG(3),
+				   "setnsec3param:lookup nsec3param -> %s",
+				   isc_result_totext(result));
+			goto failure;
+		}
+
+		INSIST(param.salt != NULL);
+
+		/* Update NSEC3 parameters. */
+		np->rdata.hash = param.hash;
+		np->rdata.flags = param.flags;
+		np->rdata.iterations = param.iterations;
+		np->rdata.salt_length = param.salt_length;
+		np->rdata.salt = param.salt;
+
+		isc_buffer_init(&b, nbuf, sizeof(nbuf));
+		CHECK(dns_rdata_fromstruct(&nrdata, zone->rdclass,
+					   dns_rdatatype_nsec3param, &np->rdata,
+					   &b));
+		dns_nsec3param_toprivate(&nrdata, &prdata, zone->privatetype,
+					 np->data, sizeof(np->data));
+		np->length = prdata.length;
+		np->nsec = false;
+	}
+
 	/*
 	 * Does a private-type record already exist for this chain?
 	 */
@@ -21443,25 +21512,6 @@ failure:
 	INSIST(newver == NULL);
 }
 
-static void
-salt2text(unsigned char *salt, uint8_t saltlen, unsigned char *text,
-	  unsigned int textlen) {
-	isc_region_t r;
-	isc_buffer_t buf;
-	isc_result_t result;
-
-	r.base = salt;
-	r.length = (unsigned int)saltlen;
-
-	isc_buffer_init(&buf, text, textlen);
-	result = isc_hex_totext(&r, 2, "", &buf);
-	if (result == ISC_R_SUCCESS) {
-		text[saltlen * 2] = 0;
-	} else {
-		text[0] = 0;
-	}
-}
-
 /*
  * Check if zone has NSEC3PARAM (and thus a chain) with the right parameters.
  *
@@ -21493,14 +21543,17 @@ dns__zone_lookup_nsec3param(dns_zone_t *zone, dns_rdata_nsec3param_t *lookup,
 	}
 	ZONEDB_UNLOCK(&zone->dblock, isc_rwlocktype_read);
 	if (db == NULL) {
+		result = ISC_R_FAILURE;
 		goto setparam;
 	}
 
 	result = dns_db_findnode(db, &zone->origin, false, &node);
 	if (result != ISC_R_SUCCESS) {
 		dns_zone_log(zone, ISC_LOG_ERROR,
-			     "nsec3param lookup failure: %s",
+			     "dns__zone_lookup_nsec3param:"
+			     "dns_db_findnode -> %s",
 			     dns_result_totext(result));
+		result = ISC_R_FAILURE;
 		goto setparam;
 	}
 	dns_db_currentversion(db, &version);
@@ -21512,7 +21565,8 @@ dns__zone_lookup_nsec3param(dns_zone_t *zone, dns_rdata_nsec3param_t *lookup,
 		INSIST(!dns_rdataset_isassociated(&rdataset));
 		if (result != ISC_R_NOTFOUND) {
 			dns_zone_log(zone, ISC_LOG_ERROR,
-				     "nsec3param lookup failure: %s",
+				     "dns__zone_lookup_nsec3param:"
+				     "dns_db_findrdataset -> %s",
 				     dns_result_totext(result));
 		}
 		goto setparam;
@@ -21552,10 +21606,13 @@ dns__zone_lookup_nsec3param(dns_zone_t *zone, dns_rdata_nsec3param_t *lookup,
 		break;
 	}
 
+	if (result == ISC_R_NOMORE) {
+		result = ISC_R_NOTFOUND;
+	}
+
 setparam:
 	if (result != ISC_R_SUCCESS) {
 		/* Found no match. */
-		result = ISC_R_NOTFOUND;
 		param->hash = lookup->hash;
 		param->flags = lookup->flags;
 		param->iterations = lookup->iterations;
@@ -21563,6 +21620,10 @@ setparam:
 		param->salt = lookup->salt;
 	}
 
+	if (result != ISC_R_NOTFOUND && result != ISC_R_SUCCESS) {
+		goto failure;
+	}
+
 	if (param->salt_length == 0) {
 		DE_CONST("-", param->salt);
 	} else if (resalt || param->salt == NULL) {
@@ -21595,6 +21656,7 @@ setparam:
 		INSIST(result != ISC_R_SUCCESS);
 	}
 
+failure:
 	if (dns_rdataset_isassociated(&rdataset)) {
 		dns_rdataset_disassociate(&rdataset);
 	}
@@ -21646,6 +21708,7 @@ dns_zone_setnsec3param(dns_zone_t *zone, uint8_t hash, uint8_t flags,
 	dns_zone_t *dummy = NULL;
 	isc_buffer_t b;
 	isc_event_t *e = NULL;
+	bool do_lookup = false;
 
 	REQUIRE(DNS_ZONE_VALID(zone));
 
@@ -21668,7 +21731,11 @@ dns_zone_setnsec3param(dns_zone_t *zone, uint8_t hash, uint8_t flags,
 			UNLOCK_ZONE(zone);
 			return (ISC_R_SUCCESS);
 		}
-		INSIST(param.salt != NULL);
+		/*
+		 * Schedule lookup if lookup above failed (may happen if zone
+		 * db is NULL for example).
+		 */
+		do_lookup = (param.salt == NULL) ? true : false;
 	}
 
 	e = isc_event_allocate(zone->mctx, zone, DNS_EVENT_SETNSEC3PARAM,
@@ -21676,8 +21743,9 @@ dns_zone_setnsec3param(dns_zone_t *zone, uint8_t hash, uint8_t flags,
 
 	npe = (struct np3event *)e;
 	np = &npe->params;
-
 	np->replace = replace;
+	np->resalt = resalt;
+	np->lookup = do_lookup;
 	if (hash == 0) {
 		np->length = 0;
 		np->nsec = true;
@@ -21689,22 +21757,32 @@ dns_zone_setnsec3param(dns_zone_t *zone, uint8_t hash, uint8_t flags,
 		param.mctx = NULL;
 		/* nsec3 specific param set in dns__zone_lookup_nsec3param() */
 		isc_buffer_init(&b, nbuf, sizeof(nbuf));
-		CHECK(dns_rdata_fromstruct(&nrdata, zone->rdclass,
-					   dns_rdatatype_nsec3param, &param,
-					   &b));
-		dns_nsec3param_toprivate(&nrdata, &prdata, zone->privatetype,
-					 np->data, sizeof(np->data));
-		np->length = prdata.length;
+
+		if (param.salt != NULL) {
+			CHECK(dns_rdata_fromstruct(&nrdata, zone->rdclass,
+						   dns_rdatatype_nsec3param,
+						   &param, &b));
+			dns_nsec3param_toprivate(&nrdata, &prdata,
+						 zone->privatetype, np->data,
+						 sizeof(np->data));
+			np->length = prdata.length;
+		}
+
+		np->rdata = param;
 		np->nsec = false;
 
 		if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(3))) {
 			unsigned char salttext[255 * 2 + 1];
-			salt2text(param.salt, param.salt_length, salttext,
-				  sizeof(salttext));
+			if (param.salt != NULL) {
+				salt2text(param.salt, param.salt_length,
+					  salttext, sizeof(salttext));
+			}
 			dnssec_log(zone, ISC_LOG_DEBUG(3),
-				   "setnsec3param:nsec3 %u %u %u %s",
+				   "setnsec3param:nsec3 %u %u %u %u:%s",
 				   param.hash, param.flags, param.iterations,
-				   salttext);
+				   param.salt_length,
+				   param.salt == NULL ? "unknown"
+						      : (char *)salttext);
 		}
 	}