Merge branch '3808-refactor-isc_nm_xfr_allowed-v9_18' into 'v9_18'

[9.18] Refactor isc_nm_xfr_allowed()

See merge request isc-projects/bind9!7413

CHANGES

@@ -1,3 +1,6 @@
+6074.	[func]		Refactor the isc_nm_xfr_allowed() function to return
+			isc_result_t instead of boolean. [GL #3808]
+
 6072.	[bug]		Avoid the OpenSSL lock contention when initializing
 			Message Digest Contexts by using explicit algorithm
 			fetching, initializing static contexts for every

bin/dig/dighost.c

@@ -3487,20 +3487,19 @@ launch_next_query(dig_query_t *query) {
 
 	xfr = query->lookup->rdtype == dns_rdatatype_ixfr ||
 	      query->lookup->rdtype == dns_rdatatype_axfr;
-	if (xfr && isc_nm_socket_type(query->handle) == isc_nm_tlsdnssocket &&
-	    !isc_nm_xfr_allowed(query->handle))
-	{
-		dighost_error("zone transfers over the "
-			      "established TLS connection are not allowed");
-		dighost_error("as the "
-			      "connection does not meet the requirements "
-			      "enforced by the RFC 9103");
-		isc_refcount_decrement0(&recvcount);
-		isc_nmhandle_detach(&query->readhandle);
-		cancel_lookup(l);
-		lookup_detach(&l);
-		clear_current_lookup();
-		return;
+	if (xfr && isc_nm_socket_type(query->handle) == isc_nm_tlsdnssocket) {
+		isc_result_t result = isc_nm_xfr_checkperm(query->handle);
+		if (result != ISC_R_SUCCESS) {
+			dighost_error("zone transfers over the established TLS "
+				      "connection are not allowed: %s",
+				      isc_result_totext(result));
+			isc_refcount_decrement0(&recvcount);
+			isc_nmhandle_detach(&query->readhandle);
+			cancel_lookup(l);
+			lookup_detach(&l);
+			clear_current_lookup();
+			return;
+		}
 	}
 
 	query_attach(query, &readquery);

lib/dns/xfrin.c

@@ -1233,11 +1233,7 @@ xfrin_connect_done(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
 
 	CHECK(result);
 
-	if (!isc_nm_xfr_allowed(handle)) {
-		/* set the error code so that XFER will fail */
-		result = ISC_R_NOPERM;
-		goto failure;
-	}
+	CHECK(isc_nm_xfr_checkperm(handle));
 
 	zmgr = dns_zone_getmgr(xfr->zone);
 	if (zmgr != NULL) {

lib/isc/include/isc/netmgr.h

@@ -692,10 +692,16 @@ isc_nm_bad_request(isc_nmhandle_t *handle);
  *  \li 'handle' is a valid netmgr handle object.
  */
 
-bool
-isc_nm_xfr_allowed(isc_nmhandle_t *handle);
+isc_result_t
+isc_nm_xfr_checkperm(isc_nmhandle_t *handle);
 /*%<
- * Check if it is possible to do a zone transfer over the given handle.
+ * Check if it is permitted to do a zone transfer over the given handle.
+ *
+ * Returns:
+ * \li	#ISC_R_SUCCESS		Success, permission check passed successfully
+ * \li	#ISC_R_DOTALPNERROR	No permission because of ALPN tag mismatch
+ * \li	#ISC_R_NOPERM		No permission because of other restrictions
+ * \li  any other result indicates failure (i.e. no permission)
  *
  * Requires:
  * \li	'handle' is a valid connection handle.

lib/isc/include/isc/result.h

@@ -94,6 +94,7 @@ typedef enum isc_result {
 	ISC_R_TLSERROR,		      /*%< TLS error */
 	ISC_R_TLSBADPEERCERT, /*%< TLS peer certificate verification failed */
 	ISC_R_HTTP2ALPNERROR, /*%< ALPN for HTTP/2 failed */
+	ISC_R_DOTALPNERROR,   /*%< ALPN for DoT failed */
 
 	DNS_R_LABELTOOLONG = 1 << 16,
 	DNS_R_BADESCAPE,
@@ -221,6 +222,7 @@ typedef enum isc_result {
 	DNS_R_NSEC3BADALG,
 	DNS_R_NSEC3RESALT,
 	DNS_R_INCONSISTENTRR,
+	DNS_R_NOALPN,
 
 	DST_R_UNSUPPORTEDALG = 2 << 16,
 	DST_R_CRYPTOFAILURE,

lib/isc/netmgr/netmgr-int.h

@@ -1715,12 +1715,17 @@ isc__nm_async_tlsdns_set_tlsctx(isc_nmsocket_t *listener, isc_tlsctx_t *tlsctx,
  * Callback handlers for asynchronous TLSDNS events.
  */
 
-bool
-isc__nm_tlsdns_xfr_allowed(isc_nmsocket_t *sock);
+isc_result_t
+isc__nm_tlsdns_xfr_checkperm(isc_nmsocket_t *sock);
 /*%<
- * Check if it is possible to do a zone transfer over the given TLSDNS
+ * Check if it is permitted to do a zone transfer over the given TLSDNS
  * socket.
  *
+ * Returns:
+ * \li	#ISC_R_SUCCESS		Success, permission check passed successfully
+ * \li	#ISC_R_DOTALPNERROR	No permission because of ALPN tag mismatch
+ * \li  any other result indicates failure (i.e. no permission)
+ *
  * Requires:
  * \li	'sock' is a valid TLSDNS socket.
  */

lib/isc/netmgr/netmgr.c

@@ -3640,9 +3640,10 @@ isc_nm_bad_request(isc_nmhandle_t *handle) {
 	}
 }
 
-bool
-isc_nm_xfr_allowed(isc_nmhandle_t *handle) {
-	isc_nmsocket_t *sock;
+isc_result_t
+isc_nm_xfr_checkperm(isc_nmhandle_t *handle) {
+	isc_nmsocket_t *sock = NULL;
+	isc_result_t result = ISC_R_NOPERM;
 
 	REQUIRE(VALID_NMHANDLE(handle));
 	REQUIRE(VALID_NMSOCK(handle->sock));
@@ -3651,16 +3652,16 @@ isc_nm_xfr_allowed(isc_nmhandle_t *handle) {
 
 	switch (sock->type) {
 	case isc_nm_tcpdnssocket:
-		return (true);
+		result = ISC_R_SUCCESS;
+		break;
 	case isc_nm_tlsdnssocket:
-		return (isc__nm_tlsdns_xfr_allowed(sock));
+		result = isc__nm_tlsdns_xfr_checkperm(sock);
+		break;
 	default:
-		return (false);
+		break;
 	}
 
-	UNREACHABLE();
-
-	return (false);
+	return (result);
 }
 
 bool

lib/isc/netmgr/tlsdns.c

@@ -2216,12 +2216,16 @@ isc__nm_async_tlsdnscancel(isc__networker_t *worker, isc__netievent_t *ev0) {
  * The ones requiring strict compatibility with the specification
  * could disable TLSv1.2 in the configuration file.
  */
-bool
-isc__nm_tlsdns_xfr_allowed(isc_nmsocket_t *sock) {
+isc_result_t
+isc__nm_tlsdns_xfr_checkperm(isc_nmsocket_t *sock) {
 	REQUIRE(VALID_NMSOCK(sock));
 	REQUIRE(sock->type == isc_nm_tlsdnssocket);
 
-	return (sock->tls.alpn_negotiated);
+	if (!sock->tls.alpn_negotiated) {
+		return (ISC_R_DOTALPNERROR);
+	}
+
+	return (ISC_R_SUCCESS);
 }
 
 const char *

lib/isc/result.c

@@ -93,6 +93,7 @@ static const char *description[ISC_R_NRESULTS] = {
 	[ISC_R_TLSERROR] = "TLS error",
 	[ISC_R_TLSBADPEERCERT] = "TLS peer certificate verification failed",
 	[ISC_R_HTTP2ALPNERROR] = "ALPN for HTTP/2 failed",
+	[ISC_R_DOTALPNERROR] = "ALPN for DoT failed",
 
 	[DNS_R_LABELTOOLONG] = "label too long",
 	[DNS_R_BADESCAPE] = "bad escape",
@@ -220,6 +221,7 @@ static const char *description[ISC_R_NRESULTS] = {
 	[DNS_R_NSEC3BADALG] = "cannot use NSEC3 with key algorithm",
 	[DNS_R_NSEC3RESALT] = "NSEC3 resalt",
 	[DNS_R_INCONSISTENTRR] = "inconsistent resource record",
+	[DNS_R_NOALPN] = "no ALPN",
 
 	[DST_R_UNSUPPORTEDALG] = "algorithm is unsupported",
 	[DST_R_CRYPTOFAILURE] = "crypto failure",
@@ -340,6 +342,7 @@ static const char *identifier[ISC_R_NRESULTS] = {
 	[ISC_R_TLSERROR] = "ISC_R_TLSERROR",
 	[ISC_R_TLSBADPEERCERT] = "ISC_R_TLSBADPEERCERT",
 	[ISC_R_HTTP2ALPNERROR] = "ISC_R_HTTP2ALPNERROR",
+	[ISC_R_DOTALPNERROR] = "ISC_R_DOTALPNERROR",
 	[DNS_R_LABELTOOLONG] = "DNS_R_LABELTOOLONG",
 	[DNS_R_BADESCAPE] = "DNS_R_BADESCAPE",
 	[DNS_R_EMPTYLABEL] = "DNS_R_EMPTYLABEL",
@@ -466,6 +469,7 @@ static const char *identifier[ISC_R_NRESULTS] = {
 	[DNS_R_NSEC3BADALG] = "DNS_R_NSEC3BADALG",
 	[DNS_R_NSEC3RESALT] = "DNS_R_NSEC3RESALT",
 	[DNS_R_INCONSISTENTRR] = "DNS_R_INCONSISTENTRR",
+	[DNS_R_NOALPN] = "DNS_R_NOALPN",
 
 	[DST_R_UNSUPPORTEDALG] = "DST_R_UNSUPPORTEDALG",
 	[DST_R_CRYPTOFAILURE] = "DST_R_CRYPTOFAILURE",

lib/ns/query.c

@@ -12251,17 +12251,25 @@ ns_query_start(ns_client_t *client, isc_nmhandle_t *handle) {
 				query_error(client, DNS_R_NOTIMP, __LINE__);
 				return;
 			}
-			if (isc_nm_socket_type(handle) == isc_nm_tlsdnssocket &&
-			    !isc_nm_xfr_allowed(handle))
-			{
+			if (isc_nm_socket_type(handle) == isc_nm_tlsdnssocket) {
 				/*
 				 * Currently this code is here for DoT, which
 				 * has more complex requirements for zone
 				 * transfers compared to other stream
 				 * protocols. See RFC 9103 for details.
 				 */
-				query_error(client, DNS_R_REFUSED, __LINE__);
-				return;
+				switch (isc_nm_xfr_checkperm(handle)) {
+				case ISC_R_SUCCESS:
+					break;
+				case ISC_R_DOTALPNERROR:
+					query_error(client, DNS_R_NOALPN,
+						    __LINE__);
+					return;
+				default:
+					query_error(client, DNS_R_REFUSED,
+						    __LINE__);
+					return;
+				}
 			}
 			ns_xfr_start(client, rdataset->type);
 			return;

tests/isc/netmgr_test.c

@@ -2644,7 +2644,7 @@ tlsdns_connect_connect_noalpn(isc_nmhandle_t *handle, isc_result_t eresult,
 	isc_refcount_decrement(&active_cconnects);
 
 	if (eresult != ISC_R_SUCCESS || connect_readcb == NULL ||
-	    !isc_nm_xfr_allowed(handle))
+	    isc_nm_xfr_checkperm(handle) != ISC_R_SUCCESS)
 	{
 		return;
 	}
@@ -2715,7 +2715,7 @@ tls_accept_cb_noalpn(isc_nmhandle_t *handle, isc_result_t eresult,
 
 	atomic_fetch_add(&saccepts, 1);
 
-	if (!isc_nm_xfr_allowed(handle)) {
+	if (isc_nm_xfr_checkperm(handle) != ISC_R_SUCCESS) {
 		return (ISC_R_FAILURE);
 	}