Merge branch '3981-reduce-dnssec-verify-calls' into 'main'

Reduce dns_dnssec_verify calls made checking for revoked trust anchor

Closes #3981

See merge request isc-projects/bind9!7763

CHANGES

@@ -1,3 +1,7 @@
+6142.	[bug]		Reduce the number of dns_dnssec_verify calls made
+			determining if revoked keys needs to be removed from
+			the trust anchors. [GL #3981]
+
 6141.	[bug]		Fix several issues in nsupdate timeout handling and
 			update the -t option's documentation. [GL #3674]
 

doc/notes/notes-current.rst

@@ -64,7 +64,8 @@ Feature Changes
 Bug Fixes
 ~~~~~~~~~
 
-- None.
+- Performance of DNSSEC validation in zones with many DNSKEY records
+  has been improved. :gl:`#3981`
 
 Known Issues
 ~~~~~~~~~~~~

lib/dns/include/dns/view.h

@@ -1045,7 +1045,7 @@ dns_view_ntacovers(dns_view_t *view, isc_stdtime_t now, const dns_name_t *name,
 
 void
 dns_view_untrust(dns_view_t *view, const dns_name_t *keyname,
-		 dns_rdata_dnskey_t *dnskey);
+		 const dns_rdata_dnskey_t *dnskey);
 /*%<
  * Remove keys that match 'keyname' and 'dnskey' from the views trust
  * anchors.
@@ -1062,6 +1062,19 @@ dns_view_untrust(dns_view_t *view, const dns_name_t *keyname,
  * \li	'dnskey' is valid.
  */
 
+bool
+dns_view_istrusted(dns_view_t *view, const dns_name_t *keyname,
+		   const dns_rdata_dnskey_t *dnskey);
+/*%<
+ * Determine if the key defined by 'keyname' and 'dnskey' is
+ * trusted by 'view'.
+ *
+ * Requires:
+ * \li	'view' is valid.
+ * \li	'keyname' is valid.
+ * \li	'dnskey' is valid.
+ */
+
 isc_result_t
 dns_view_setnewzones(dns_view_t *view, bool allow, void *cfgctx,
 		     void (*cfg_destroy)(void **), uint64_t mapsize);

lib/dns/keytable.c

@@ -460,10 +460,6 @@ dns_keytable_deletekey(dns_keytable_t *keytable, const dns_name_t *keyname,
 	REQUIRE(VALID_KEYTABLE(keytable));
 	REQUIRE(dnskey != NULL);
 
-	isc_buffer_init(&b, data, sizeof(data));
-	dns_rdata_fromstruct(&rdata, dnskey->common.rdclass,
-			     dns_rdatatype_dnskey, dnskey, &b);
-
 	RWLOCK(&keytable->rwlock, isc_rwlocktype_write);
 	result = dns_rbt_findnode(keytable->table, keyname, NULL, &node, NULL,
 				  DNS_RBTFIND_NOOPTIONS, NULL, NULL);
@@ -490,6 +486,13 @@ dns_keytable_deletekey(dns_keytable_t *keytable, const dns_name_t *keyname,
 	}
 	RWUNLOCK(&knode->rwlock, isc_rwlocktype_read);
 
+	isc_buffer_init(&b, data, sizeof(data));
+	result = dns_rdata_fromstruct(&rdata, dnskey->common.rdclass,
+				      dns_rdatatype_dnskey, dnskey, &b);
+	if (result != ISC_R_SUCCESS) {
+		goto finish;
+	}
+
 	result = dns_ds_fromkeyrdata(keyname, &rdata, DNS_DSDIGEST_SHA256,
 				     digest, &ds);
 	if (result != ISC_R_SUCCESS) {

lib/dns/validator.c

@@ -1287,26 +1287,50 @@ selfsigned_dnskey(dns_validator_t *val) {
 				continue;
 			}
 
+			/*
+			 * If the REVOKE bit is not set we have a
+			 * theoretically self signed DNSKEY RRset.
+			 * This will be verified later.
+			 */
+			if ((key.flags & DNS_KEYFLAG_REVOKE) == 0) {
+				answer = true;
+				continue;
+			}
+
 			result = dns_dnssec_keyfromrdata(name, &keyrdata, mctx,
 							 &dstkey);
 			if (result != ISC_R_SUCCESS) {
 				continue;
 			}
 
-			result = dns_dnssec_verify(name, rdataset, dstkey, true,
-						   val->view->maxbits, mctx,
-						   &sigrdata, NULL);
+			/*
+			 * If this RRset is pending and it is trusted,
+			 * see if it was self signed by this DNSKEY.
+			 */
+			if (DNS_TRUST_PENDING(rdataset->trust) &&
+			    dns_view_istrusted(val->view, name, &key))
+			{
+				result = dns_dnssec_verify(
+					name, rdataset, dstkey, true,
+					val->view->maxbits, mctx, &sigrdata,
+					NULL);
+				if (result == ISC_R_SUCCESS) {
+					/*
+					 * The key with the REVOKE flag has
+					 * self signed the RRset so it is no
+					 * good.
+					 */
+					dns_view_untrust(val->view, name, &key);
+				}
+			} else if (rdataset->trust >= dns_trust_secure) {
+				/*
+				 * We trust this RRset so if the key is
+				 * marked revoked remove it.
+				 */
+				dns_view_untrust(val->view, name, &key);
+			}
+
 			dst_key_free(&dstkey);
-			if (result != ISC_R_SUCCESS) {
-				continue;
-			}
-
-			if ((key.flags & DNS_KEYFLAG_REVOKE) == 0) {
-				answer = true;
-				continue;
-			}
-
-			dns_view_untrust(val->view, name, &key);
 		}
 	}
 

lib/dns/view.c

@@ -1639,26 +1639,28 @@ dns_view_issecuredomain(dns_view_t *view, const dns_name_t *name,
 
 void
 dns_view_untrust(dns_view_t *view, const dns_name_t *keyname,
-		 dns_rdata_dnskey_t *dnskey) {
+		 const dns_rdata_dnskey_t *dnskey) {
 	isc_result_t result;
 	dns_keytable_t *sr = NULL;
+	dns_rdata_dnskey_t tmpkey;
 
 	REQUIRE(DNS_VIEW_VALID(view));
 	REQUIRE(keyname != NULL);
 	REQUIRE(dnskey != NULL);
 
-	/*
-	 * Clear the revoke bit, if set, so that the key will match what's
-	 * in secroots now.
-	 */
-	dnskey->flags &= ~DNS_KEYFLAG_REVOKE;
-
 	result = dns_view_getsecroots(view, &sr);
 	if (result != ISC_R_SUCCESS) {
 		return;
 	}
 
-	result = dns_keytable_deletekey(sr, keyname, dnskey);
+	/*
+	 * Clear the revoke bit, if set, so that the key will match what's
+	 * in secroots now.
+	 */
+	tmpkey = *dnskey;
+	tmpkey.flags &= ~DNS_KEYFLAG_REVOKE;
+
+	result = dns_keytable_deletekey(sr, keyname, &tmpkey);
 	if (result == ISC_R_SUCCESS) {
 		/*
 		 * If key was found in secroots, then it was a
@@ -1673,6 +1675,88 @@ dns_view_untrust(dns_view_t *view, const dns_name_t *keyname,
 	dns_keytable_detach(&sr);
 }
 
+bool
+dns_view_istrusted(dns_view_t *view, const dns_name_t *keyname,
+		   const dns_rdata_dnskey_t *dnskey) {
+	isc_result_t result;
+	dns_keytable_t *sr = NULL;
+	dns_keynode_t *knode = NULL;
+	bool answer = false;
+	dns_rdataset_t dsset;
+
+	REQUIRE(DNS_VIEW_VALID(view));
+	REQUIRE(keyname != NULL);
+	REQUIRE(dnskey != NULL);
+
+	result = dns_view_getsecroots(view, &sr);
+	if (result != ISC_R_SUCCESS) {
+		return (false);
+	}
+
+	dns_rdataset_init(&dsset);
+	result = dns_keytable_find(sr, keyname, &knode);
+	if (result == ISC_R_SUCCESS) {
+		if (dns_keynode_dsset(knode, &dsset)) {
+			dns_rdata_t rdata = DNS_RDATA_INIT;
+			unsigned char data[4096], digest[DNS_DS_BUFFERSIZE];
+			dns_rdata_dnskey_t tmpkey = *dnskey;
+			dns_rdata_ds_t ds;
+			isc_buffer_t b;
+			dns_rdataclass_t rdclass = tmpkey.common.rdclass;
+
+			/*
+			 * Clear the revoke bit, if set, so that the key
+			 * will match what's in secroots now.
+			 */
+			tmpkey.flags &= ~DNS_KEYFLAG_REVOKE;
+
+			isc_buffer_init(&b, data, sizeof(data));
+			result = dns_rdata_fromstruct(&rdata, rdclass,
+						      dns_rdatatype_dnskey,
+						      &tmpkey, &b);
+			if (result != ISC_R_SUCCESS) {
+				goto finish;
+			}
+
+			result = dns_ds_fromkeyrdata(keyname, &rdata,
+						     DNS_DSDIGEST_SHA256,
+						     digest, &ds);
+			if (result != ISC_R_SUCCESS) {
+				goto finish;
+			}
+
+			dns_rdata_reset(&rdata);
+			isc_buffer_init(&b, data, sizeof(data));
+			result = dns_rdata_fromstruct(
+				&rdata, rdclass, dns_rdatatype_ds, &ds, &b);
+			if (result != ISC_R_SUCCESS) {
+				goto finish;
+			}
+
+			result = dns_rdataset_first(&dsset);
+			while (result == ISC_R_SUCCESS) {
+				dns_rdata_t this = DNS_RDATA_INIT;
+				dns_rdataset_current(&dsset, &this);
+				if (dns_rdata_compare(&rdata, &this) == 0) {
+					answer = true;
+					break;
+				}
+				result = dns_rdataset_next(&dsset);
+			}
+		}
+	}
+
+finish:
+	if (dns_rdataset_isassociated(&dsset)) {
+		dns_rdataset_disassociate(&dsset);
+	}
+	if (knode != NULL) {
+		dns_keytable_detachkeynode(sr, &knode);
+	}
+	dns_keytable_detach(&sr);
+	return (answer);
+}
+
 /*
  * Create path to a directory and a filename constructed from viewname.
  * This is a front-end to isc_file_sanitize(), allowing backward