Merge branch 'fix-ddns' into 'master'

Fix Ddns

Fixes for new tests, tests were modified a bit as well.

src/knot/updates/ddns.c

@@ -20,13 +20,14 @@
 
 #include "knot/updates/ddns.h"
 #include "knot/updates/changesets.h"
+#include "knot/updates/xfr-in.h"
+#include "knot/zone/semantic-check.h"
 #include "libknot/rdata.h"
 #include "common/debug.h"
 #include "libknot/packet/pkt.h"
 #include "libknot/common.h"
 #include "libknot/consts.h"
 #include "common/mempattern.h"
-#include "knot/updates/xfr-in.h"
 #include "common/descriptor.h"
 
 static bool rrset_empty(const knot_rrset_t *rrset)
@@ -261,7 +262,7 @@ static int knot_ddns_check_exist_full(const knot_zone_contents_t *zone,
 		assert(knot_rrset_type(found) == knot_rrset_type(rrset));
 		assert(knot_dname_cmp(knot_rrset_owner(found),
 		                          knot_rrset_owner(rrset)) == 0);
-		if (knot_rrset_rdata_equal(found, rrset) <= 0) {
+		if (!knot_rrset_equal(found, rrset, KNOT_RRSET_COMPARE_WHOLE)) {
 			*rcode = KNOT_RCODE_NXRRSET;
 			return KNOT_EPREREQ;
 		}
@@ -629,8 +630,8 @@ dbg_ddns_exec_detail(
 				assert(knot_rrset_rr_count(rr) != 0);
 
 				// We must check if the RDATA match
-				if (knot_rrset_rdata_equal(rr,
-				                           rrset)) {
+				if (knot_rrset_equal(rr, rrset,
+				                     KNOT_RRSET_COMPARE_WHOLE)) {
 					remove = rrset;
 					rem_node((node_t *)rr_node);
 					(*removed)[(*removed_count)++] = remove;
@@ -674,27 +675,6 @@ static void knot_ddns_check_add_rr(knot_changeset_t *changeset,
 
 /*----------------------------------------------------------------------------*/
 
-static knot_node_t *knot_ddns_get_node(knot_zone_contents_t *zone,
-                                       const knot_rrset_t *rr)
-{
-	assert(zone != NULL);
-	assert(rr != NULL);
-
-	knot_node_t *node = NULL;
-	knot_dname_t *owner = knot_rrset_get_owner(rr);
-
-	dbg_ddns_detail("Searching for node...\n");
-	if (knot_rrset_is_nsec3rel(rr)) {
-		node = knot_zone_contents_get_nsec3_node(zone, owner);
-	} else {
-		node = knot_zone_contents_get_node(zone, owner);
-	}
-
-	return node;
-}
-
-/*----------------------------------------------------------------------------*/
-
 static int knot_ddns_process_add_cname(knot_node_t *node,
                                        const knot_rrset_t *rr,
                                        knot_changeset_t *changeset,
@@ -1043,13 +1023,14 @@ static int knot_ddns_add_rr_to_chgset(const knot_rrset_t *rr,
 /*----------------------------------------------------------------------------*/
 
 static int knot_ddns_process_add(const knot_rrset_t *rr,
-                                 knot_node_t *node,
+                                 knot_node_t **node,
                                  knot_zone_contents_t *zone,
                                  knot_changeset_t *changeset,
                                  knot_changes_t *changes,
                                  knot_rrset_t **rr_copy)
 {
 	assert(rr != NULL);
+	assert(node != NULL);
 	assert(zone != NULL);
 	assert(changeset != NULL);
 	assert(changes != NULL);
@@ -1057,15 +1038,16 @@ static int knot_ddns_process_add(const knot_rrset_t *rr,
 
 	dbg_ddns_verb("Adding RR.\n");
 
-	if (node == NULL) {
+	if (*node == NULL) {
 		// create new node, connect it to the zone nodes
 		dbg_ddns_detail("Node not found. Creating new.\n");
-		int ret = knot_zone_contents_create_node(zone, rr, &node);
+		int ret = knot_zone_contents_create_node(zone, rr, node);
 		if (ret != KNOT_EOK) {
 			dbg_xfrin("Failed to create new node in zone.\n");
 			return ret;
 		}
 	}
+	assert(*node);
 
 	uint16_t type = knot_rrset_type(rr);
 	*rr_copy = NULL;
@@ -1075,22 +1057,21 @@ static int knot_ddns_process_add(const knot_rrset_t *rr,
 	 * First, rule out special cases: CNAME, SOA and adding to CNAME node.
 	 */
 	if (type == KNOT_RRTYPE_CNAME) {
-		/* 1) CNAME */
-		ret = knot_ddns_process_add_cname(node, rr, changeset, changes);
+		ret = knot_ddns_process_add_cname(*node, rr, changeset, changes);
 	} else if (type == KNOT_RRTYPE_SOA) {
-		/* 2) SOA */
-		ret = knot_ddns_process_add_soa(node, rr, changes);
-	} else if (type == KNOT_RRTYPE_NSEC3PARAM
-	           && knot_node_rrset(node, KNOT_RRTYPE_NSEC3PARAM)) {
-		if (!knot_dname_is_equal(node->owner, zone->apex->owner)) {
+		ret = knot_ddns_process_add_soa(*node, rr, changes);
+	} else if (type == KNOT_RRTYPE_NSEC3PARAM) {
+		if (!knot_dname_is_equal((*node)->owner, zone->apex->owner)) {
 			log_zone_error("NSEC3PARAM RR may be added under apex name only!\n");
-			return KNOT_EMALF;
+			return KNOT_EDENIED;
 		}
-		/* 3) NSEC3PARAM - ignore if there is one already in the zone.*/
-		log_zone_warning("NSEC3PARAM already present in the zone. "
-		                 "Ignoring NSEC3PARAM from the UPDATE.\n");
-		return KNOT_EOK;
-	} else if (knot_node_rrset(node, KNOT_RRTYPE_CNAME) != NULL) {
+		if (knot_node_rrset(*node, KNOT_RRTYPE_NSEC3PARAM)) {
+			/* Ignore if there is one already in the zone.*/
+			log_zone_warning("NSEC3PARAM already present in the zone. "
+			                 "Ignoring NSEC3PARAM from the UPDATE.\n");
+			return KNOT_EOK;
+		}
+	} else if (knot_node_rrset(*node, KNOT_RRTYPE_CNAME) != NULL) {
 		/*
 		 * Adding RR to CNAME node. Ignore the UPDATE RR.
 		 *
@@ -1115,7 +1096,7 @@ static int knot_ddns_process_add(const knot_rrset_t *rr,
 
 	/* Add the RRSet to the node (RRSIGs handled in the function). */
 	dbg_ddns_detail("Adding RR to the node.\n");
-	ret = knot_ddns_add_rr(node, rr, changes, rr_copy);
+	ret = knot_ddns_add_rr(*node, rr, changes, rr_copy);
 	if (ret < 0) {
 		dbg_ddns("Failed to add RR to the node.\n");
 		return ret;
@@ -1243,17 +1224,7 @@ static int knot_ddns_process_rem_rr(const knot_rrset_t *rr,
 	       || knot_rrset_rr_count(rrset_copy));
 
 	/*
-	 * 3) Store the removed data in 'changes'.
-	 */
-	ret = knot_changes_add_rrset(changes, rr_remove, KNOT_CHANGES_OLD);
-	if (ret != KNOT_EOK) {
-		knot_rrset_free(&rr_remove, NULL);
-		dbg_ddns_detail("Failed to add data to changes.\n");
-		return ret;
-	}
-
-	/*
-	 * 4) If the RRSet is empty, remove it and store in 'changes'.
+	 * 3) If the RRSet is empty, remove it and store in 'changes'.
 	 */
 	if (knot_rrset_rr_count(to_modify) == 0) {
 		// The RRSet should not be empty if we were removing NSs from
@@ -1263,6 +1234,7 @@ static int knot_ddns_process_rem_rr(const knot_rrset_t *rr,
 		ret = knot_changes_add_rrset(changes, rrset_copy,
 		                             KNOT_CHANGES_OLD);
 		if (ret != KNOT_EOK) {
+		knot_rrset_free(&rr_remove, NULL);
 			dbg_ddns("Failed to add RRSet to changes.\n");
 			return ret;
 		}
@@ -1273,14 +1245,15 @@ static int knot_ddns_process_rem_rr(const knot_rrset_t *rr,
 	}
 
 	/*
-	 * 5) Check if the RR is not in the ADD section. If yes, remove it
+	 * 4) Check if the RR is not in the ADD section. If yes, remove it
 	 *    from there and do not add it to the REMOVE section.
 	 */
 	knot_rrset_t **from_chgset = NULL;
 	size_t from_chgset_count = 0;
 	ret = knot_ddns_check_remove_rr(changeset, knot_node_owner(node),
-	                                 rr, &from_chgset, &from_chgset_count);
+	                                rr, &from_chgset, &from_chgset_count);
 	if (ret != KNOT_EOK) {
+	knot_rrset_free(&rr_remove, NULL);
 		dbg_ddns("Failed to remove possible redundant RRs from ADD "
 		         "section: %s.\n", knot_strerror(ret));
 		free(from_chgset);
@@ -1290,9 +1263,8 @@ static int knot_ddns_process_rem_rr(const knot_rrset_t *rr,
 	assert(from_chgset_count <= 1);
 
 	if (from_chgset_count == 1) {
-		/* Just delete the RRSet. */
 		knot_rrset_free(&(from_chgset[0]), NULL);
-
+		knot_rrset_free(&rr_remove, NULL);
 		/* Finish processing, no adding to changeset. */
 		free(from_chgset);
 		return KNOT_EOK;
@@ -1300,6 +1272,17 @@ static int knot_ddns_process_rem_rr(const knot_rrset_t *rr,
 
 	free(from_chgset);
 
+
+	/*
+	 * 5) Store the removed data in 'changes'.
+	 */
+	ret = knot_changes_add_rrset(changes, rr_remove, KNOT_CHANGES_OLD);
+	if (ret != KNOT_EOK) {
+		knot_rrset_free(&rr_remove, NULL);
+		dbg_ddns_detail("Failed to add data to changes.\n");
+		return ret;
+	}
+
 	/*
 	 * 6) Store the RRSet containing the one RDATA in the changeset. We may
 	 *    use the RRSet from the packet for this - copy it, set CLASS
@@ -1594,30 +1577,46 @@ static int knot_ddns_process_rr(const knot_rrset_t *rr,
 	assert(rr_copy != NULL);
 
 	/* 1) Find node that will be affected. */
-	knot_node_t *node = knot_ddns_get_node(zone, rr);
+	knot_node_t *node = knot_zone_contents_get_node(zone, rr->owner);
 
 	/* 2) Decide what to do. */
-
+	int ret = KNOT_EOK;
 	if (knot_rrset_class(rr) == knot_zone_contents_class(zone)) {
-		return knot_ddns_process_add(rr, node, zone, changeset,
-		                             changes, rr_copy);
+		ret = knot_ddns_process_add(rr, &node, zone, changeset,
+		                            changes, rr_copy);
 	} else if (node == NULL) {
 		// Removing from non-existing node, just ignore the entry
 		return KNOT_EOK;
 	} else if (knot_rrset_class(rr) == KNOT_CLASS_NONE) {
-		return knot_ddns_process_rem_rr(rr, node, zone, changeset,
-			                        changes, qclass);
+		ret = knot_ddns_process_rem_rr(rr, node, zone, changeset,
+		                               changes, qclass);
 	} else if (knot_rrset_class(rr) == KNOT_CLASS_ANY) {
 		if (knot_rrset_type(rr) == KNOT_RRTYPE_ANY) {
-			return knot_ddns_process_rem_all(node, changeset,
-			                                 changes);
+			ret = knot_ddns_process_rem_all(node, changeset,
+			                                changes);
 		} else {
-			return knot_ddns_process_rem_rrset(rr, node, changeset,
-			                                   changes);
+			ret = knot_ddns_process_rem_rrset(rr, node, changeset,
+			                                  changes);
 		}
 	} else {
 		return KNOT_ERROR;
 	}
+
+	if (ret == KNOT_EOK) {
+		assert(node);
+		// Do a semantic check for changed node.
+		err_handler_t handler;
+		err_handler_init(&handler);
+		bool fatal = false;
+		ret = sem_check_node_plain(zone, node, &handler, true, &fatal);
+		if (ret == KNOT_EOK) {
+			if (fatal) {
+				ret = KNOT_EDENIED;
+			}
+		}
+	}
+
+	return ret;
 }
 
 /*----------------------------------------------------------------------------*/
@@ -1727,8 +1726,13 @@ int knot_ddns_process_update(knot_zone_contents_t *zone,
 		if (ret != KNOT_EOK) {
 			dbg_ddns("Failed to process update RR:%s\n",
 			         knot_strerror(ret));
-			*rcode = (ret == KNOT_EMALF) ? KNOT_RCODE_FORMERR
-			                             : KNOT_RCODE_SERVFAIL;
+			if (ret == KNOT_EMALF) {
+				*rcode = KNOT_RCODE_FORMERR;
+			} else if (ret == KNOT_EDENIED) {
+				*rcode = KNOT_RCODE_REFUSED;
+			} else {
+				*rcode = KNOT_RCODE_SERVFAIL;
+			}
 			return ret;
 		}
 

src/libknot/rrset.c

@@ -166,6 +166,45 @@ static int rrset_rdata_compare_one(const knot_rrset_t *rrset1,
 	return cmp;
 }
 
+/*!
+ * \brief RRSet RDATA equality check.
+ *
+ * \param r1  First RRSet.
+ * \param r2  Second RRSet.
+ *
+ * \return True if RRs in r1 are equal to RRs in r2, false otherwise.
+ */
+static bool knot_rrset_rdata_equal(const knot_rrset_t *r1, const knot_rrset_t *r2)
+{
+	if (r1 == NULL || r2 == NULL || (r1->type != r2->type) ||
+	    r1->rrs == NULL || r2->rrs == NULL) {
+		return KNOT_EINVAL;
+	}
+
+	uint16_t r1_rdata_count = knot_rrset_rr_count(r1);
+	uint16_t r2_rdata_count = knot_rrset_rr_count(r2);
+
+	if (r1_rdata_count != r2_rdata_count) {
+		return false;
+	}
+
+	for (uint16_t i = 0; i < r1_rdata_count; i++) {
+		bool found = false;
+		for (uint16_t j = 0; j < r2_rdata_count; j++) {
+			if (rrset_rdata_compare_one(r1, r2, i, j) == 0) {
+				found = true;
+				break;
+			}
+		}
+
+		if (!found) {
+			return false;
+		}
+	}
+
+	return true;
+}
+
 static int knot_rrset_header_to_wire(const knot_rrset_t *rrset, uint32_t ttl,
                                      uint8_t **pos, size_t max_size,
                                      knot_compr_t *compr, size_t *size)
@@ -768,40 +807,6 @@ uint8_t *knot_rrset_rr_rdata(const knot_rrset_t *rrset, size_t pos)
 	}
 }
 
-/*!
- * \brief Compare two RR sets, order of RDATA is not significant.
- */
-bool knot_rrset_rdata_equal(const knot_rrset_t *r1, const knot_rrset_t *r2)
-{
-	if (r1 == NULL || r2 == NULL || (r1->type != r2->type) ||
-	    r1->rrs == NULL || r2->rrs == NULL) {
-		return KNOT_EINVAL;
-	}
-
-	uint16_t r1_rdata_count = knot_rrset_rr_count(r1);
-	uint16_t r2_rdata_count = knot_rrset_rr_count(r2);
-
-	if (r1_rdata_count != r2_rdata_count) {
-		return false;
-	}
-
-	for (uint16_t i = 0; i < r1_rdata_count; i++) {
-		bool found = false;
-		for (uint16_t j = 0; j < r2_rdata_count; j++) {
-			if (rrset_rdata_compare_one(r1, r2, i, j) == 0) {
-				found = true;
-				break;
-			}
-		}
-
-		if (!found) {
-			return false;
-		}
-	}
-
-	return true;
-}
-
 int knot_rrset_to_wire(const knot_rrset_t *rrset, uint8_t *wire, size_t *size,
                        size_t max_size, uint16_t *rr_count, knot_compr_t *compr)
 {

src/libknot/rrset.h

@@ -228,16 +228,6 @@ void knot_rrset_rr_set_ttl(const knot_rrset_t *rrset, size_t pos, uint32_t ttl);
  */
 uint16_t knot_rrset_rr_count(const knot_rrset_t *rrset);
 
-/*!
- * \brief RRSet RDATA equality check.
- *
- * \param r1  First RRSet.
- * \param r2  Second RRSet.
- *
- * \return True if RRs in r1 are equal to RRs in r2, false otherwise.
- */
-bool knot_rrset_rdata_equal(const knot_rrset_t *r1, const knot_rrset_t *r2);
-
 /*!
  * \brief Compares two RRSets for equality.
  *

tests-extra/tests/ddns/basic/test.py

@@ -139,8 +139,9 @@ def do_normal_tests(master, zone, dnssec=False):
     up.add("cname.ddns.", "3600", "A", "1.2.3.4")
     up.send("NOERROR")
     resp = master.dig("cname.ddns.", "ANY")
+    resp.check(rcode="NOERROR")
+    resp.check_record(rtype="A", nordata="1.2.3.4")
     resp.check_record(rtype="CNAME", rdata="mail.ddns.")
-    compare(resp.count(), 1, "Added A when it shouldn't")
     verify(master, zone, dnssec)
 
     # add CNAME to CNAME node, should be replaced
@@ -292,18 +293,6 @@ def do_refusal_tests(master, zone, dnssec=False):
     resp.check(rcode="NXDOMAIN")
     check_soa(master, prev_soa)
 
-    # Add DNAME to CNAME node
-    check_log("Add extra DNAME rollback")
-    up = master.update(zone)
-    up.add("rollback.ddns.", 3600, "TXT", "do not add me")
-    up.add("cname.ddns.", 3600, "DNAME", "ddns.")
-    up.send("REFUSED")
-    resp = master.dig("rollback.ddns", "ANY")
-    resp.check(rcode="NXDOMAIN")
-    resp = master.dig("forbidden.ddns", "ANY")
-    resp.check(rcode="NXDOMAIN")
-    check_soa(master, prev_soa)
-
     # Add DNAME children
     check_log("Add DNAME children rollback")
     up = master.update(zone)
@@ -312,15 +301,6 @@ def do_refusal_tests(master, zone, dnssec=False):
     up.send("REFUSED")
     resp = master.dig("rollback.ddns", "ANY")
     resp.check(rcode="NXDOMAIN")
-    resp = master.dig("forbidden.ddns", "ANY")
-    resp.check(rcode="NXDOMAIN")
-    check_soa(master, prev_soa)
-
-    # Remove SOA, ignore
-    check_log("Remove SOA")
-    up = master.update(zone)
-    up.delete("ddns.", "SOA")
-    up.send("MALFORMED")
     check_soa(master, prev_soa)
 
     # Out-of-zone data
@@ -330,12 +310,26 @@ def do_refusal_tests(master, zone, dnssec=False):
     up.send("NOTZONE")
     check_soa(master, prev_soa)
 
+    # Remove 'all' SOA, ignore
+    check_log("Remove all SOA")
+    up = master.update(zone)
+    up.delete("ddns.", "SOA")
+    up.send("NOERROR")
+    check_soa(master, prev_soa)
+
+    # Remove specific SOA, ignore
+    check_log("Remove specific SOA")
+    up = master.update(zone)
+    up.delete("ddns.", "SOA", "dns1.ddns. hostmaster.ddns. 2011111213 10800 3600 1209600 7200")
+    up.send("NOERROR")
+    check_soa(master, prev_soa)
+
     if dnssec:
         # NSEC3PARAM for non-apex node
         check_log("Non-apex NSEC3PARAM")
         up = master.update(zone)
         up.add("not.apex.ddns.", "0", "NSEC3PARAM", "1 0 10 B8399FF56C1C0C7E")
-        up.send("MALFORMED")
+        up.send("REFUSED")
         resp = master.dig("not.apex.ddns", "NSEC3PARAM")
         resp.check(rcode="NXDOMAIN")
         check_soa(master, prev_soa)
@@ -374,6 +368,8 @@ def do_nsec3param_tests(master, zone):
     resp.check(rcode="NOERROR", rdata="1 0 10 BADDCAFE")
     verify(master, zone, dnssec=True)
 
+    # Normal deletion tested in DNSSEC tests
+
 zone = t.zone("ddns.", storage=".")
 
 master_plain = t.server("knot")