fix: dev: Refactor resolver cache_name() and validated() functions

These functions were excessive in length and complexity, with McCabe complexity values of 110 and 105 respectively, and also included some dead code. They have been cleaned up and split into smaller functions, with a maximum complexity of 27.  A few minor coding errors were discovered and fixed along the way.

Merge branch 'each-refactor-cache-name' into 'main'

See merge request isc-projects/bind9!10198

lib/dns/include/dns/keytable.h

@@ -205,9 +205,9 @@ dns_keytable_finddeepestmatch(dns_keytable_t *keytable, const dns_name_t *name,
  *\li	Any other result indicates an error.
  */
 
-isc_result_t
+bool
 dns_keytable_issecuredomain(dns_keytable_t *keytable, const dns_name_t *name,
-			    dns_name_t *foundname, bool *wantdnssecp);
+			    dns_name_t *foundname);
 /*%<
  * Is 'name' at or beneath a trusted key?
  *
@@ -219,20 +219,11 @@ dns_keytable_issecuredomain(dns_keytable_t *keytable, const dns_name_t *name,
  *
  *\li	'foundanme' is NULL or is a pointer to an initialized dns_name_t
  *
- *\li	'*wantsdnssecp' is a valid bool.
- *
  * Ensures:
  *
- *\li	On success, *wantsdnssecp will be true if and only if 'name'
+ *\li	Returns true if and only if 'name' is at or beneath a trusted key.
- *	is at or beneath a trusted key.  If 'foundname' is not NULL, then
+ *	If 'foundname' is not NULL, then it will be updated to contain
- *	it will be updated to contain the name of the closest enclosing
+ *	the name of the closest enclosing trust anchor.
- *	trust anchor.
- *
- * Returns:
- *
- *\li	ISC_R_SUCCESS
- *
- *\li	Any other result is an error.
  */
 
 isc_result_t

lib/dns/include/dns/ncache.h

@@ -54,26 +54,24 @@
 isc_result_t
 dns_ncache_add(dns_message_t *message, dns_db_t *cache, dns_dbnode_t *node,
 	       dns_rdatatype_t covers, isc_stdtime_t now, dns_ttl_t minttl,
-	       dns_ttl_t maxttl, dns_rdataset_t *addedrdataset);
+	       dns_ttl_t maxttl, bool optout, bool secure,
-isc_result_t
+	       dns_rdataset_t *addedrdataset);
-dns_ncache_addoptout(dns_message_t *message, dns_db_t *cache,
-		     dns_dbnode_t *node, dns_rdatatype_t covers,
-		     isc_stdtime_t now, dns_ttl_t minttl, dns_ttl_t maxttl,
-		     bool optout, dns_rdataset_t *addedrdataset);
 /*%<
  * Convert the authority data from 'message' into a negative cache
  * rdataset, and store it in 'cache' at 'node' with a TTL limited to
  * 'maxttl'.
  *
- * \li dns_ncache_add produces a negative cache entry with a trust of no
+ * \li If 'secure' is true and all the records that make up the entry
- *     more than answer
+ *     are secure, then dns_ncache_add produces a negative cache entry
- * \li dns_ncache_addoptout produces a negative cache entry which will have
+ *     with trust level secure.
- *     a trust of secure if all the records that make up the entry are secure.
+ * \li If 'secure' is false, the negative cache entry's trust level
+ *     will be capped at answer.
  *
  * The 'covers' argument is the RR type whose nonexistence we are caching,
  * or dns_rdatatype_any when caching a NXDOMAIN response.
  *
- * 'optout' parameter indicates if 'optout' attribute should be set.
+ * 'optout' parameter indicates if 'optout' attribute should be set.  This only
+ * applies in secure zones; if 'secure' is false, 'optout' is ignored.
  *
  * Note:
  *\li	If 'addedrdataset' is not NULL, then it will be attached to the added

lib/dns/include/dns/rdataset.h

@@ -673,3 +673,30 @@ dns_rdataset_equals(const dns_rdataset_t *rdataset1,
  * \li	'rdataset1' is a valid rdataset.
  * \li	'rdataset2' is a valid rdataset.
  */
+
+/*%
+ * Returns true if the rdataset is of type 'type', or type RRSIG
+ * and covers 'type'.
+ */
+static inline bool
+dns_rdataset_matchestype(const dns_rdataset_t *rdataset,
+			 const dns_rdatatype_t type) {
+	REQUIRE(DNS_RDATASET_VALID(rdataset));
+
+	return rdataset->type == type ||
+	       (rdataset->type == dns_rdatatype_rrsig &&
+		rdataset->covers == type);
+}
+
+/*%
+ * Returns true if the rdataset is of type 'type', or type RRSIG
+ * and covers 'type'.
+ */
+static inline bool
+dns_rdataset_issigtype(const dns_rdataset_t *rdataset,
+		       const dns_rdatatype_t type) {
+	REQUIRE(DNS_RDATASET_VALID(rdataset));
+
+	return rdataset->type == dns_rdatatype_rrsig &&
+	       rdataset->covers == type;
+}

lib/dns/include/dns/view.h

@@ -985,13 +985,12 @@ dns_view_getsecroots(dns_view_t *view, dns_keytable_t **ktp);
  *\li	ISC_R_NOTFOUND
  */
 
-isc_result_t
+bool
 dns_view_issecuredomain(dns_view_t *view, const dns_name_t *name,
-			isc_stdtime_t now, bool checknta, bool *ntap,
+			isc_stdtime_t now, bool checknta, bool *ntap);
-			bool *secure_domain);
 /*%<
  * Is 'name' at or beneath a trusted key, and not covered by a valid
- * negative trust anchor?  Put answer in '*secure_domain'.
+ * negative trust anchor, and DNSSEC validation is enabled?
  *
  * If 'checknta' is false, ignore the NTA table in determining
  * whether this is a secure domain. If 'checknta' is not false, and if
@@ -1000,10 +999,6 @@ dns_view_issecuredomain(dns_view_t *view, const dns_name_t *name,
  *
  * Requires:
  * \li	'view' is valid.
- *
- * Returns:
- *\li	ISC_R_SUCCESS
- *\li	Any other value indicates failure
  */
 
 bool

lib/dns/keytable.c

@@ -524,13 +524,14 @@ dns_keytable_finddeepestmatch(dns_keytable_t *keytable, const dns_name_t *name,
 	return result;
 }
 
-isc_result_t
+bool
 dns_keytable_issecuredomain(dns_keytable_t *keytable, const dns_name_t *name,
-			    dns_name_t *foundname, bool *wantdnssecp) {
+			    dns_name_t *foundname) {
 	isc_result_t result;
 	dns_qpread_t qpr;
 	dns_keynode_t *keynode = NULL;
 	void *pval = NULL;
+	bool secure = false;
 
 	/*
 	 * Is 'name' at or beneath a trusted key?
@@ -538,7 +539,6 @@ dns_keytable_issecuredomain(dns_keytable_t *keytable, const dns_name_t *name,
 
 	REQUIRE(VALID_KEYTABLE(keytable));
 	REQUIRE(dns_name_isabsolute(name));
-	REQUIRE(wantdnssecp != NULL);
 
 	dns_qpmulti_query(keytable->table, &qpr);
 	result = dns_qp_lookup(&qpr, name, DNS_DBNAMESPACE_NORMAL, NULL, NULL,
@@ -548,16 +548,12 @@ dns_keytable_issecuredomain(dns_keytable_t *keytable, const dns_name_t *name,
 		if (foundname != NULL) {
 			dns_name_copy(&keynode->name, foundname);
 		}
-		*wantdnssecp = true;
+		secure = true;
-		result = ISC_R_SUCCESS;
-	} else if (result == ISC_R_NOTFOUND) {
-		*wantdnssecp = false;
-		result = ISC_R_SUCCESS;
 	}
 
 	dns_qpread_destroy(keytable->table, &qpr);
 
-	return result;
+	return secure;
 }
 
 static isc_result_t

lib/dns/ncache.c

@@ -50,12 +50,6 @@ atomic_getuint8(isc_buffer_t *b) {
 	return ret;
 }
 
-static isc_result_t
-addoptout(dns_message_t *message, dns_db_t *cache, dns_dbnode_t *node,
-	  dns_rdatatype_t covers, isc_stdtime_t now, dns_ttl_t minttl,
-	  dns_ttl_t maxttl, bool optout, bool secure,
-	  dns_rdataset_t *addedrdataset);
-
 static isc_result_t
 copy_rdataset(dns_rdataset_t *rdataset, isc_buffer_t *buffer) {
 	unsigned int count;
@@ -101,23 +95,6 @@ copy_rdataset(dns_rdataset_t *rdataset, isc_buffer_t *buffer) {
 
 isc_result_t
 dns_ncache_add(dns_message_t *message, dns_db_t *cache, dns_dbnode_t *node,
-	       dns_rdatatype_t covers, isc_stdtime_t now, dns_ttl_t minttl,
-	       dns_ttl_t maxttl, dns_rdataset_t *addedrdataset) {
-	return addoptout(message, cache, node, covers, now, minttl, maxttl,
-			 false, false, addedrdataset);
-}
-
-isc_result_t
-dns_ncache_addoptout(dns_message_t *message, dns_db_t *cache,
-		     dns_dbnode_t *node, dns_rdatatype_t covers,
-		     isc_stdtime_t now, dns_ttl_t minttl, dns_ttl_t maxttl,
-		     bool optout, dns_rdataset_t *addedrdataset) {
-	return addoptout(message, cache, node, covers, now, minttl, maxttl,
-			 optout, true, addedrdataset);
-}
-
-static isc_result_t
-addoptout(dns_message_t *message, dns_db_t *cache, dns_dbnode_t *node,
 	       dns_rdatatype_t covers, isc_stdtime_t now, dns_ttl_t minttl,
 	       dns_ttl_t maxttl, bool optout, bool secure,
 	       dns_rdataset_t *addedrdataset) {
@@ -135,14 +112,17 @@ addoptout(dns_message_t *message, dns_db_t *cache, dns_dbnode_t *node,
 	/*
 	 * Convert the authority data from 'message' into a negative cache
 	 * rdataset, and store it in 'cache' at 'node'.
+	 *
+	 * We assume that all data in the authority section has been
+	 * validated by the caller.
 	 */
 
 	REQUIRE(message != NULL);
 
 	/*
-	 * We assume that all data in the authority section has been
+	 * If 'secure' is false, ignore 'optout'.
-	 * validated by the caller.
 	 */
+	optout = optout && secure;
 
 	/*
 	 * Initialize the list.

lib/dns/view.c

@@ -1535,41 +1535,31 @@ dns_view_ntacovers(dns_view_t *view, isc_stdtime_t now, const dns_name_t *name,
 	return dns_ntatable_covered(view->ntatable_priv, now, name, anchor);
 }
 
-isc_result_t
+bool
 dns_view_issecuredomain(dns_view_t *view, const dns_name_t *name,
-			isc_stdtime_t now, bool checknta, bool *ntap,
+			isc_stdtime_t now, bool checknta, bool *ntap) {
-			bool *secure_domain) {
-	isc_result_t result;
 	bool secure = false;
 	dns_fixedname_t fn;
 	dns_name_t *anchor;
 
 	REQUIRE(DNS_VIEW_VALID(view));
 
-	if (view->secroots_priv == NULL) {
+	if (!view->enablevalidation || view->secroots_priv == NULL) {
-		return ISC_R_NOTFOUND;
+		return false;
 	}
 
 	anchor = dns_fixedname_initname(&fn);
-
+	secure = dns_keytable_issecuredomain(view->secroots_priv, name, anchor);
-	result = dns_keytable_issecuredomain(view->secroots_priv, name, anchor,
-					     &secure);
-	if (result != ISC_R_SUCCESS) {
-		return result;
-	}
 
 	SET_IF_NOT_NULL(ntap, false);
 	if (checknta && secure && view->ntatable_priv != NULL &&
 	    dns_ntatable_covered(view->ntatable_priv, now, name, anchor))
 	{
-		if (ntap != NULL) {
+		SET_IF_NOT_NULL(ntap, true);
-			*ntap = true;
-		}
 		secure = false;
 	}
 
-	*secure_domain = secure;
+	return secure;
-	return ISC_R_SUCCESS;
 }
 
 void

tests/dns/keytable_test.c

@@ -544,7 +544,6 @@ ISC_LOOP_TEST_IMPL(find) {
 
 /* check issecuredomain() */
 ISC_LOOP_TEST_IMPL(issecuredomain) {
-	bool issecure;
 	const char **n;
 	const char *names[] = { "example.com", "sub.example.com",
 				"null.example", "sub.null.example", NULL };
@@ -559,22 +558,16 @@ ISC_LOOP_TEST_IMPL(issecuredomain) {
 	 * of installing a null key).
 	 */
 	for (n = names; *n != NULL; n++) {
-		assert_int_equal(dns_keytable_issecuredomain(keytable,
+		assert_true(dns_keytable_issecuredomain(keytable, str2name(*n),
-							     str2name(*n), NULL,
+							NULL));
-							     &issecure),
-				 ISC_R_SUCCESS);
-		assert_true(issecure);
 	}
 
 	/*
 	 * If the key table has no entry (not even a null one) for a domain or
 	 * any of its ancestors, that domain is considered insecure.
 	 */
-	assert_int_equal(dns_keytable_issecuredomain(keytable,
+	assert_false(dns_keytable_issecuredomain(
-						     str2name("example.org"),
+		keytable, str2name("example.org"), NULL));
-						     NULL, &issecure),
-			 ISC_R_SUCCESS);
-	assert_false(issecure);
 
 	destroy_tables();
 
@@ -604,7 +597,7 @@ ISC_LOOP_TEST_IMPL(dump) {
 /* check negative trust anchors */
 ISC_LOOP_TEST_IMPL(nta) {
 	isc_result_t result;
-	bool issecure, covered;
+	bool covered;
 	dns_fixedname_t fn;
 	dns_name_t *keyname = dns_fixedname_name(&fn);
 	unsigned char digest[DNS_DS_BUFFERSIZE];
@@ -636,20 +629,15 @@ ISC_LOOP_TEST_IMPL(nta) {
 	assert_int_equal(result, ISC_R_SUCCESS);
 
 	/* Should be secure */
-	result = dns_view_issecuredomain(myview,
+	assert_true(dns_view_issecuredomain(
-					 str2name("test.secure.example"), now,
+		myview, str2name("test.secure.example"), now, true, &covered));
-					 true, &covered, &issecure);
-	assert_int_equal(result, ISC_R_SUCCESS);
 	assert_false(covered);
-	assert_true(issecure);
 
 	/* Should not be secure */
-	result = dns_view_issecuredomain(myview,
+	assert_false(dns_view_issecuredomain(myview,
-					 str2name("test.insecure.example"), now,
+					     str2name("test.insecure.example"),
-					 true, &covered, &issecure);
+					     now, true, &covered));
-	assert_int_equal(result, ISC_R_SUCCESS);
 	assert_true(covered);
-	assert_false(issecure);
 
 	/* NTA covered */
 	covered = dns_view_ntacovers(myview, now, str2name("insecure.example"),
@@ -662,38 +650,30 @@ ISC_LOOP_TEST_IMPL(nta) {
 	assert_false(covered);
 
 	/* As of now + 2, the NTA should be clear */
-	result = dns_view_issecuredomain(myview,
+	assert_true(dns_view_issecuredomain(myview,
 					    str2name("test.insecure.example"),
-					 now + 2, true, &covered, &issecure);
+					    now + 2, true, &covered));
-	assert_int_equal(result, ISC_R_SUCCESS);
 	assert_false(covered);
-	assert_true(issecure);
 
 	/* Now check deletion */
-	result = dns_view_issecuredomain(myview, str2name("test.new.example"),
+	assert_true(dns_view_issecuredomain(
-					 now, true, &covered, &issecure);
+		myview, str2name("test.new.example"), now, true, &covered));
-	assert_int_equal(result, ISC_R_SUCCESS);
 	assert_false(covered);
-	assert_true(issecure);
 
 	result = dns_ntatable_add(ntatable, str2name("new.example"), false, now,
 				  3600);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
-	result = dns_view_issecuredomain(myview, str2name("test.new.example"),
+	assert_false(dns_view_issecuredomain(
-					 now, true, &covered, &issecure);
+		myview, str2name("test.new.example"), now, true, &covered));
-	assert_int_equal(result, ISC_R_SUCCESS);
 	assert_true(covered);
-	assert_false(issecure);
 
 	result = dns_ntatable_delete(ntatable, str2name("new.example"));
 	assert_int_equal(result, ISC_R_SUCCESS);
 
-	result = dns_view_issecuredomain(myview, str2name("test.new.example"),
+	assert_true(dns_view_issecuredomain(
-					 now, true, &covered, &issecure);
+		myview, str2name("test.new.example"), now, true, &covered));
-	assert_int_equal(result, ISC_R_SUCCESS);
 	assert_false(covered);
-	assert_true(issecure);
 
 	isc_loopmgr_shutdown();