fix a bug with the insertion of DS records into existing keynodes

NOTE: the keytable test is still failing because dns_keytable_deletekey()
is looking for exact matches in keynodes containing dst_key objects,
which no keynode has anymore.

lib/dns/include/dns/keytable.h

@@ -357,12 +357,6 @@ dns_keynode_trust(dns_keynode_t *keynode);
  * trusted: no longer an initializing key.
  */
 
-isc_result_t
-dns_keynode_create(isc_mem_t *mctx, dns_keynode_t **target);
-/*%<
- * Allocate space for a keynode
- */
-
 void
 dns_keynode_attach(dns_keynode_t *source, dns_keynode_t **target);
 /*%<

lib/dns/keytable.c

@@ -159,127 +159,78 @@ free_dslist(isc_mem_t *mctx, dns_keynode_t *knode) {
 	knode->dslist = NULL;
 }
 
-/*%
- * Search "node" for an empty or DS-style keynode, or a keynode for the
- * exact same key as the one supplied in "keyp" and, if found, update it
- * accordingly.
- */
 static isc_result_t
-update_keynode(dns_keytable_t *keytable, dns_rbtnode_t *node,
+add_ds(dns_keynode_t *knode, dns_rdata_ds_t *ds, isc_mem_t *mctx) {
-	       dst_key_t **keyp, bool initial)
+	isc_result_t result;
-{
+	dns_rdata_t *rdata = NULL;
-	dns_keynode_t *knode;
+	void *data = NULL;
+	isc_buffer_t b;
 
-	REQUIRE(keyp != NULL && *keyp != NULL);
+	if (knode->dslist == NULL) {
-	REQUIRE(node != NULL);
+		knode->dslist = isc_mem_get(mctx, sizeof(*knode->dslist));
-
+		dns_rdatalist_init(knode->dslist);
-	for (knode = node->data; knode != NULL; knode = knode->next) {
+		knode->dslist->rdclass = dns_rdataclass_in;
-		if (knode->key == NULL) {
+		knode->dslist->type = dns_rdatatype_ds;
-			/*
-			 * Null or DS-style keynode found.  Detach
-			 * the DS rdatalist if present. Attach the
-			 * supplied key to it, transferring key
-			 * ownership to the keytable.
-			 */
-			if (knode->dslist != NULL) {
-				free_dslist(keytable->mctx, knode);
-			}
-
-			knode->key = *keyp;
-			*keyp = NULL;
-		} else if (dst_key_compare(knode->key, *keyp)) {
-			/*
-			 * Key node found for the supplied key.  Free the
-			 * supplied copy of the key and update the found key
-			 * node's flags if necessary.
-			 */
-			dst_key_free(keyp);
-		} else {
-			continue;
-		}
-
-		if (!initial) {
-			dns_keynode_trust(knode);
-		}
-
-		return (ISC_R_SUCCESS);
 	}
 
-	return (ISC_R_NOTFOUND);
+	rdata = isc_mem_get(mctx, sizeof(*rdata));
-}
+	dns_rdata_init(rdata);
 
-/*%
+	data = isc_mem_get(mctx, DNS_DS_BUFFERSIZE);
- * Create a key node for "keyp" (or a null key node if "keyp" is NULL), set
+	isc_buffer_init(&b, data, DNS_DS_BUFFERSIZE);
- * "managed" and "initial" as requested and make the created key node the first
- * one attached to "node" in "keytable".
- */
-static isc_result_t
-prepend_keynode(dst_key_t **keyp, dns_rdata_ds_t *ds,
-		dns_rbtnode_t *node, dns_keytable_t *keytable,
-		bool managed, bool initial)
-{
-	dns_keynode_t *knode = NULL;
-	isc_result_t result;
 
-	REQUIRE(keyp == NULL || *keyp != NULL);
+	result = dns_rdata_fromstruct(rdata, dns_rdataclass_in,
-	REQUIRE(VALID_KEYTABLE(keytable));
+				      dns_rdatatype_ds, ds, &b);
-	REQUIRE(!initial || managed);
-
-	result = dns_keynode_create(keytable->mctx, &knode);
 	if (result != ISC_R_SUCCESS) {
 		return (result);
 	}
 
+	ISC_LIST_APPEND(knode->dslist->rdata, rdata, link);
+
+	if (dns_rdataset_isassociated(&knode->dsset)) {
+		dns_rdataset_disassociate(&knode->dsset);
+	}
+
+	result = dns_rdatalist_tordataset(knode->dslist, &knode->dsset);
+	if (result != ISC_R_SUCCESS) {
+		return (result);
+	}
+
+	knode->dsset.trust = dns_trust_ultimate;
+	return (ISC_R_SUCCESS);
+}
+
+/*%
+ * Create a keynode for "ds" (or a null key node if "ds" is NULL), set
+ * "managed" and "initial" as requested and attach the keynode to
+ * to "node" in "keytable".
+ */
+static isc_result_t
+new_keynode(dns_rdata_ds_t *ds, dns_rbtnode_t *node,
+	    dns_keytable_t *keytable, bool managed, bool initial)
+{
+	dns_keynode_t *knode = NULL;
+	isc_result_t result;
+
+	REQUIRE(VALID_KEYTABLE(keytable));
+	REQUIRE(!initial || managed);
+
+	knode = isc_mem_get(keytable->mctx, sizeof(dns_keynode_t));
+	*knode = (dns_keynode_t) {
+		.magic = KEYNODE_MAGIC
+	};
+
+	dns_rdataset_init(&knode->dsset);
+	isc_refcount_init(&knode->refcount, 1);
+
 	/*
-	 * If a dst_key was supplied, transfer its ownership to the keytable.
+	 * If a DS was supplied, initialize an rdatalist.
-	 * Otherwise, if a DS was supplied, append it to the rdatalist
-	 * (initializing if necessary).
 	 */
-	if (keyp != NULL) {
+	if (ds != NULL) {
-		if (knode->dslist != NULL) {
+		result = add_ds(knode, ds, keytable->mctx);
-			free_dslist(keytable->mctx, knode);
-		}
-		knode->key = *keyp;
-		*keyp = NULL;
-	} else if (ds != NULL) {
-		dns_rdata_t *rdata = NULL;
-		void *data = NULL;
-		isc_buffer_t b;
-
-		if (knode->dslist == NULL) {
-			knode->dslist = isc_mem_get(keytable->mctx,
-						    sizeof(*knode->dslist));
-			dns_rdatalist_init(knode->dslist);
-			knode->dslist->rdclass = dns_rdataclass_in;
-			knode->dslist->type = dns_rdatatype_ds;
-			knode->dslist->ttl = 0;
-		}
-
-		rdata = isc_mem_get(keytable->mctx, sizeof(*rdata));
-		dns_rdata_init(rdata);
-
-		data = isc_mem_get(keytable->mctx, DNS_DS_BUFFERSIZE);
-		isc_buffer_init(&b, data, DNS_DS_BUFFERSIZE);
-
-		result = dns_rdata_fromstruct(rdata, dns_rdataclass_in,
-					      dns_rdatatype_ds, ds, &b);
 		if (result != ISC_R_SUCCESS) {
 			return (result);
 		}
-
-		ISC_LIST_APPEND(knode->dslist->rdata, rdata, link);
-
-		if (dns_rdataset_isassociated(&knode->dsset)) {
-			dns_rdataset_disassociate(&knode->dsset);
-		}
-
-		result = dns_rdatalist_tordataset(knode->dslist,
-						  &knode->dsset);
-		if (result != ISC_R_SUCCESS) {
-			return (result);
-		}
-
-		knode->dsset.trust = dns_trust_ultimate;
 	}
 
 	knode->managed = managed;
@@ -292,14 +243,15 @@ prepend_keynode(dst_key_t **keyp, dns_rdata_ds_t *ds,
 }
 
 /*%
- * Add key "keyp" at "keyname" in "keytable".  If the key already exists at the
+ * Add key trust anchor "ds" at "keyname" in "keytable".  If an anchor
- * requested name, update its flags.  If "keyp" is NULL, add a null key to
+ * already exists at the requested name does not contain "ds", update it.
- * indicate that "keyname" should be treated as a secure domain without
+ * If "ds" is NULL, add a null key to indicate that "keyname" should be
- * supplying key data which would allow the domain to be validated.
+ * treated as a secure domain without supplying key data which would allow
+ * the domain to be validated.
  */
 static isc_result_t
 insert(dns_keytable_t *keytable, bool managed, bool initial,
-       const dns_name_t *keyname, dst_key_t **keyp, dns_rdata_ds_t *ds)
+       const dns_name_t *keyname, dns_rdata_ds_t *ds)
 {
 	dns_rbtnode_t *node = NULL;
 	isc_result_t result;
@@ -313,16 +265,15 @@ insert(dns_keytable_t *keytable, bool managed, bool initial,
 		/*
 		 * There was no node for "keyname" in "keytable" yet, so one
 		 * was created.  Create a new key node for the supplied
-		 * trust anchor (or a null key node if both "keyp" and
+		 * trust anchor (or a null key node if both "ds" is NULL)
-		 * "ds" are NULL) and attach it to the created node.
+		 * and attach it to the created node.
 		 */
-		result = prepend_keynode(keyp, ds, node, keytable,
+		result = new_keynode(ds, node, keytable, managed, initial);
-					 managed, initial);
 	} else if (result == ISC_R_EXISTS) {
 		/*
 		 * A node already exists for "keyname" in "keytable".
 		 */
-		if (keyp == NULL && ds == NULL) {
+		if (ds == NULL) {
 			/*
 			 * We were told to add a null key at "keyname", which
 			 * means there is nothing left to do as there is either
@@ -332,29 +283,14 @@ insert(dns_keytable_t *keytable, bool managed, bool initial,
 			 * "keyname" is already marked as secure.
 			 */
 			result = ISC_R_SUCCESS;
-		} else if (keyp != NULL) {
+		} else {
-			/*
+			dns_keynode_t *knode = node->data;
-			 * We were told to add the key supplied in "keyp" at
+			if (knode == NULL) {
-			 * "keyname".  Try to find an already existing key node
+				result = new_keynode(ds, node, keytable,
-			 * we could reuse for the supplied key (i.e. a null key
+						     managed, initial);
-			 * node or a key node for the exact same key) and, if
+			} else {
-			 * found, update it accordingly.
+				result = add_ds(knode, ds, keytable->mctx);
-			 */
-			result = update_keynode(keytable, node, keyp, initial);
-			if (result == ISC_R_NOTFOUND) {
-				/*
-				 * The node for "keyname" only contains key
-				 * nodes for keys different than the supplied
-				 * one.  Create a new key node for the supplied
-				 * key and prepend it before the others.
-				 */
-				result = prepend_keynode(keyp, NULL,
-							 node, keytable,
-							 managed, initial);
 			}
-		} else if (ds != NULL) {
-			result = prepend_keynode(NULL, ds, node, keytable,
-						 managed, initial);
 		}
 	}
 
@@ -369,12 +305,12 @@ dns_keytable_add(dns_keytable_t *keytable, bool managed, bool initial,
 {
 	REQUIRE(ds != NULL);
 	REQUIRE(!initial || managed);
-	return (insert(keytable, managed, initial, name, NULL, ds));
+	return (insert(keytable, managed, initial, name, ds));
 }
 
 isc_result_t
 dns_keytable_marksecure(dns_keytable_t *keytable, const dns_name_t *name) {
-	return (insert(keytable, true, false, name, NULL, NULL));
+	return (insert(keytable, true, false, name, NULL));
 }
 
 isc_result_t
@@ -853,28 +789,6 @@ dns_keynode_trust(dns_keynode_t *keynode) {
 	keynode->initial = false;
 }
 
-isc_result_t
-dns_keynode_create(isc_mem_t *mctx, dns_keynode_t **target) {
-	dns_keynode_t *knode;
-
-	REQUIRE(target != NULL && *target == NULL);
-
-	knode = isc_mem_get(mctx, sizeof(dns_keynode_t));
-
-	knode->magic = KEYNODE_MAGIC;
-	knode->managed = false;
-	knode->initial = false;
-	knode->key = NULL;
-	knode->dslist = NULL;
-	dns_rdataset_init(&knode->dsset);
-	knode->next = NULL;
-
-	isc_refcount_init(&knode->refcount, 1);
-
-	*target = knode;
-	return (ISC_R_SUCCESS);
-}
-
 void
 dns_keynode_attach(dns_keynode_t *source, dns_keynode_t **target) {
 	REQUIRE(VALID_KEYNODE(source));

lib/dns/tests/keytable_test.c

@@ -191,7 +191,7 @@ create_tables() {
 	/* Add an initializing managed key */
 	dns_test_namefromstring("managed.com", &fn);
 	create_dsstruct(keyname, 257, 3, 5, keystr1, digest, &ds);
-	assert_int_equal(dns_keytable_add(keytable, false, false, keyname, &ds),
+	assert_int_equal(dns_keytable_add(keytable, true, true, keyname, &ds),
 			 ISC_R_SUCCESS);
 
 	/* Add a null key */
@@ -278,7 +278,8 @@ add_test(void **state) {
 
 	/*
 	 * Add a different managed key for managed.com, marking it as an
-	 * initializing key.
+	 * initializing key. Since there is already a trusted key at the
+	 * node, the node should *not* be marked as initializing.
 	 */
 	dns_test_namefromstring("managed.com", &fn);
 	create_dsstruct(keyname, 257, 3, 5, keystr2, digest, &ds);
@@ -287,7 +288,7 @@ add_test(void **state) {
 	assert_int_equal(dns_keytable_find(keytable, str2name("managed.com"),
 					   &keynode),
 			 ISC_R_SUCCESS);
-	assert_int_equal(dns_keynode_initial(keynode), true);
+	assert_int_equal(dns_keynode_initial(keynode), false);
 	dns_keytable_detachkeynode(keytable, &keynode);
 
 	/*
@@ -320,7 +321,9 @@ add_test(void **state) {
 
 	/*
 	 * Add a different managed key for two.com, marking it as a
-	 * non-initializing key.
+	 * non-initializing key. Since there is already an iniitalizing
+	 * trust anchor for two.com and we haven't run dns_keynode_trust(),
+	 * the initialization status should not change.
 	 */
 	create_dsstruct(keyname, 257, 3, 5, keystr2, digest, &ds);
 	assert_int_equal(dns_keytable_add(keytable, true, false, keyname, &ds),
@@ -328,22 +331,7 @@ add_test(void **state) {
 	assert_int_equal(dns_keytable_find(keytable, str2name("two.com"),
 					   &keynode),
 			 ISC_R_SUCCESS);
-	assert_int_equal(dns_keynode_initial(keynode), false);
+	assert_int_equal(dns_keynode_initial(keynode), true);
-	dns_keytable_detachkeynode(keytable, &keynode);
-
-	/*
-	 * Add the first managed key again, but this time mark it as a
-	 * non-initializing key.  Ensure the previously added key is upgraded
-	 * to a non-initializing key and make sure there are still two key
-	 * nodes for two.com, both containing non-initializing keys.
-	 */
-	create_dsstruct(keyname, 257, 3, 5, keystr1, digest, &ds);
-	assert_int_equal(dns_keytable_add(keytable, true, false, keyname, &ds),
-			 ISC_R_SUCCESS);
-	assert_int_equal(dns_keytable_find(keytable, str2name("two.com"),
-					   &keynode),
-			 ISC_R_SUCCESS);
-	assert_int_equal(dns_keynode_initial(keynode), false);
 	dns_keytable_detachkeynode(keytable, &keynode);
 
 	/*

lib/dns/win32/libdns.def.in

@@ -459,7 +459,6 @@ dns_keydata_todnskey
 dns_keyflags_fromtext
 dns_keymgr_run
 dns_keynode_attach
-dns_keynode_create
 dns_keynode_detach
 dns_keynode_detachall
 dns_keynode_dsset