From 21d3f66f1c00dcd81726e67be43e98824b861ac2 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 11 Dec 2019 00:09:15 -0800 Subject: [PATCH 1/6] rename dns_keytable_deletekeynode to dns_keytable_deletekey this function is used by dns_view_untrust() to handle revoked keys, so it will still be needed after the keytable/validator refactoring is complete, even though the keytable will be storing DS trust anchors instead of keys. to simplify the way it's called, it now takes a DNSKEY rdata struct instead of a DST key. --- lib/dns/include/dns/keytable.h | 11 +-- lib/dns/include/dns/view.h | 3 +- lib/dns/keytable.c | 44 +++++++++--- lib/dns/tests/keytable_test.c | 121 ++++++++++++++++++++------------- lib/dns/validator.c | 42 ++++++------ lib/dns/view.c | 34 ++++----- lib/dns/win32/libdns.def.in | 2 +- lib/dns/zone.c | 2 +- 8 files changed, 150 insertions(+), 109 deletions(-) diff --git a/lib/dns/include/dns/keytable.h b/lib/dns/include/dns/keytable.h index bcafdc3623..2d8113046f 100644 --- a/lib/dns/include/dns/keytable.h +++ b/lib/dns/include/dns/keytable.h @@ -185,7 +185,7 @@ dns_keytable_marksecure(dns_keytable_t *keytable, const dns_name_t *name); isc_result_t dns_keytable_delete(dns_keytable_t *keytable, const dns_name_t *keyname); /*%< - * Delete node(s) from 'keytable' matching name 'keyname' + * Delete all trust anchors from 'keytable' matching name 'keyname' * * Requires: * @@ -201,15 +201,16 @@ dns_keytable_delete(dns_keytable_t *keytable, const dns_name_t *keyname); */ isc_result_t -dns_keytable_deletekeynode(dns_keytable_t *keytable, dst_key_t *dstkey); +dns_keytable_deletekey(dns_keytable_t *keytable, const dns_name_t *keyname, + dns_rdata_dnskey_t *dnskey); /*%< - * Delete node(s) from 'keytable' containing copies of the key pointed - * to by 'dstkey' + * Remove the trust anchor matching the name 'keyname' and the DNSKEY + * rdata struct 'dnskey' from 'keytable'. * * Requires: * *\li 'keytable' points to a valid keytable. - *\li 'dstkey' is not NULL + *\li 'dnskey' is not NULL * * Returns: * diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h index 85642b72a0..c681d2bac1 100644 --- a/lib/dns/include/dns/view.h +++ b/lib/dns/include/dns/view.h @@ -1221,7 +1221,7 @@ dns_view_ntacovers(dns_view_t *view, isc_stdtime_t now, void dns_view_untrust(dns_view_t *view, const dns_name_t *keyname, - dns_rdata_dnskey_t *dnskey, isc_mem_t *mctx); + dns_rdata_dnskey_t *dnskey); /*%< * Remove keys that match 'keyname' and 'dnskey' from the views trust * anchors. @@ -1235,7 +1235,6 @@ dns_view_untrust(dns_view_t *view, const dns_name_t *keyname, * Requires: * \li 'view' is valid. * \li 'keyname' is valid. - * \li 'mctx' is valid. * \li 'dnskey' is valid. */ diff --git a/lib/dns/keytable.c b/lib/dns/keytable.c index 60c3275f50..bf1df1ed95 100644 --- a/lib/dns/keytable.c +++ b/lib/dns/keytable.c @@ -21,6 +21,7 @@ #include /* Required for HP/UX (and others?) */ #include +#include #include #include #include @@ -403,25 +404,40 @@ dns_keytable_delete(dns_keytable_t *keytable, const dns_name_t *keyname) { } isc_result_t -dns_keytable_deletekeynode(dns_keytable_t *keytable, dst_key_t *dstkey) { +dns_keytable_deletekey(dns_keytable_t *keytable, const dns_name_t *keyname, + dns_rdata_dnskey_t *dnskey) +{ isc_result_t result; - dns_name_t *keyname; dns_rbtnode_t *node = NULL; dns_keynode_t *knode = NULL, **kprev = NULL; + dst_key_t *dstkey = NULL; + unsigned char data[4096]; + isc_buffer_t buffer; + dns_rdata_t rdata = DNS_RDATA_INIT; REQUIRE(VALID_KEYTABLE(keytable)); - REQUIRE(dstkey != NULL); + REQUIRE(dnskey != NULL); - keyname = dst_key_name(dstkey); + /* Convert dnskey to DST key. */ + isc_buffer_init(&buffer, data, sizeof(data)); + dns_rdata_fromstruct(&rdata, dnskey->common.rdclass, + dns_rdatatype_dnskey, dnskey, &buffer); + result = dns_dnssec_keyfromrdata(keyname, &rdata, keytable->mctx, + &dstkey); + if (result != ISC_R_SUCCESS) { + return (result); + } RWLOCK(&keytable->rwlock, isc_rwlocktype_write); result = dns_rbt_findnode(keytable->table, keyname, NULL, &node, NULL, DNS_RBTFIND_NOOPTIONS, NULL, NULL); - if (result == DNS_R_PARTIALMATCH) + if (result == DNS_R_PARTIALMATCH) { result = ISC_R_NOTFOUND; - if (result != ISC_R_SUCCESS) + } + if (result != ISC_R_SUCCESS) { goto finish; + } if (node->data == NULL) { result = ISC_R_NOTFOUND; @@ -430,7 +446,7 @@ dns_keytable_deletekeynode(dns_keytable_t *keytable, dst_key_t *dstkey) { knode = node->data; if (knode->next == NULL && knode->key != NULL && - dst_key_compare(knode->key, dstkey) == true) + dst_key_compare(knode->key, dstkey)) { result = dns_rbt_deletenode(keytable->table, node, false); goto finish; @@ -438,16 +454,19 @@ dns_keytable_deletekeynode(dns_keytable_t *keytable, dst_key_t *dstkey) { kprev = (dns_keynode_t **) &node->data; while (knode != NULL) { - if (knode->key != NULL && - dst_key_compare(knode->key, dstkey) == true) + if (knode->key != NULL && dst_key_compare(knode->key, dstkey)) { break; + } + kprev = &knode->next; knode = knode->next; } if (knode != NULL) { - if (knode->key != NULL) + if (knode->key != NULL) { dst_key_free(&knode->key); + } + /* * This is equivalent to: * dns_keynode_attach(knode->next, &tmp); @@ -458,10 +477,13 @@ dns_keytable_deletekeynode(dns_keytable_t *keytable, dst_key_t *dstkey) { *kprev = knode->next; knode->next = NULL; dns_keynode_detach(keytable->mctx, &knode); - } else + } else { result = DNS_R_PARTIALMATCH; + } + finish: RWUNLOCK(&keytable->rwlock, isc_rwlocktype_write); + dst_key_free(&dstkey); return (result); } diff --git a/lib/dns/tests/keytable_test.c b/lib/dns/tests/keytable_test.c index 0ad5323ea7..b90b1f7df8 100644 --- a/lib/dns/tests/keytable_test.c +++ b/lib/dns/tests/keytable_test.c @@ -103,40 +103,63 @@ str2name(const char *namestr) { } static void -create_key(uint16_t flags, uint8_t proto, uint8_t alg, - const char *keynamestr, const char *keystr, dst_key_t **target) +create_keystruct(uint16_t flags, uint8_t proto, uint8_t alg, + const char *keystr, dns_rdata_dnskey_t *keystruct) { - dns_rdata_dnskey_t keystruct; unsigned char keydata[4096]; isc_buffer_t keydatabuf; - unsigned char rrdata[4096]; - isc_buffer_t rrdatabuf; isc_region_t r; - const dns_rdataclass_t rdclass = dns_rdataclass_in; /* for brevity */ + const dns_rdataclass_t rdclass = dns_rdataclass_in; - keystruct.common.rdclass = rdclass; - keystruct.common.rdtype = dns_rdatatype_dnskey; - keystruct.mctx = NULL; - ISC_LINK_INIT(&keystruct.common, link); - keystruct.flags = flags; - keystruct.protocol = proto; - keystruct.algorithm = alg; + keystruct->common.rdclass = rdclass; + keystruct->common.rdtype = dns_rdatatype_dnskey; + keystruct->mctx = dt_mctx; + ISC_LINK_INIT(&keystruct->common, link); + keystruct->flags = flags; + keystruct->protocol = proto; + keystruct->algorithm = alg; isc_buffer_init(&keydatabuf, keydata, sizeof(keydata)); - isc_buffer_init(&rrdatabuf, rrdata, sizeof(rrdata)); assert_int_equal(isc_base64_decodestring(keystr, &keydatabuf), ISC_R_SUCCESS); isc_buffer_usedregion(&keydatabuf, &r); - keystruct.datalen = r.length; - keystruct.data = r.base; - assert_int_equal(dns_rdata_fromstruct(NULL, keystruct.common.rdclass, - keystruct.common.rdtype, - &keystruct, &rrdatabuf), - ISC_R_SUCCESS); + keystruct->datalen = r.length; + keystruct->data = isc_mem_allocate(dt_mctx, r.length); + memmove(keystruct->data, r.base, r.length); +} - assert_int_equal(dst_key_fromdns(str2name(keynamestr), rdclass, - &rrdatabuf, dt_mctx, target), - ISC_R_SUCCESS); +static void +create_key(uint16_t flags, uint8_t proto, uint8_t alg, + const char *keynamestr, const char *keystr, dst_key_t **target) +{ + isc_result_t result; + dns_rdata_dnskey_t keystruct; + unsigned char rrdata[4096]; + isc_buffer_t rrdatabuf; + const dns_rdataclass_t rdclass = dns_rdataclass_in; + + /* + * Populate DNSKEY rdata structure. + */ + create_keystruct(flags, proto, alg, keystr, &keystruct); + + /* + * Convert to wire format. + */ + isc_buffer_init(&rrdatabuf, rrdata, sizeof(rrdata)); + result = dns_rdata_fromstruct(NULL, keystruct.common.rdclass, + keystruct.common.rdtype, + &keystruct, &rrdatabuf), + assert_int_equal(result, ISC_R_SUCCESS); + + /* + * Convert wire format to DST key. + */ + result = dst_key_fromdns(str2name(keynamestr), rdclass, + &rrdatabuf, dt_mctx, target), + assert_int_equal(result, ISC_R_SUCCESS); + + dns_rdata_freestruct(&keystruct); } /* Common setup: create a keytable and ntatable to test with a few keys */ @@ -149,7 +172,8 @@ create_tables() { result = dns_test_makeview("view", &view); assert_int_equal(result, ISC_R_SUCCESS); - assert_int_equal(dns_keytable_create(dt_mctx, &keytable), ISC_R_SUCCESS); + assert_int_equal(dns_keytable_create(dt_mctx, &keytable), + ISC_R_SUCCESS); assert_int_equal(dns_ntatable_create(view, taskmgr, timermgr, &ntatable), ISC_R_SUCCESS); @@ -439,56 +463,61 @@ delete_test(void **state) { /* delete key nodes from the keytable */ static void -deletekeynode_test(void **state) { - dst_key_t *key = NULL; +deletekey_test(void **state) { + dns_rdata_dnskey_t dnskey; + dns_fixedname_t fn; + dns_name_t *keyname = dns_fixedname_name(&fn); UNUSED(state); create_tables(); /* key name doesn't match */ - create_key(257, 3, 5, "example.org", keystr1, &key); - assert_int_equal(dns_keytable_deletekeynode(keytable, key), + dns_test_namefromstring("example.org", &fn); + create_keystruct(257, 3, 5, keystr1, &dnskey); + assert_int_equal(dns_keytable_deletekey(keytable, keyname, &dnskey), ISC_R_NOTFOUND); - dst_key_free(&key); + dns_rdata_freestruct(&dnskey); /* subdomain match is the same as no match */ - create_key(257, 3, 5, "sub.example.com", keystr1, &key); - assert_int_equal(dns_keytable_deletekeynode(keytable, key), + dns_test_namefromstring("sub.example.org", &fn); + create_keystruct(257, 3, 5, keystr1, &dnskey); + assert_int_equal(dns_keytable_deletekey(keytable, keyname, &dnskey), ISC_R_NOTFOUND); - dst_key_free(&key); + dns_rdata_freestruct(&dnskey); /* name matches but key doesn't match (resulting in PARTIALMATCH) */ - create_key(257, 3, 5, "example.com", keystr2, &key); - assert_int_equal(dns_keytable_deletekeynode(keytable, key), + dns_test_namefromstring("example.com", &fn); + create_keystruct(257, 3, 5, keystr2, &dnskey); + assert_int_equal(dns_keytable_deletekey(keytable, keyname, &dnskey), DNS_R_PARTIALMATCH); - dst_key_free(&key); + dns_rdata_freestruct(&dnskey); /* * exact match. after deleting the node the internal rbt node will be * empty, and any delete or deletekeynode attempt should result in * NOTFOUND. */ - create_key(257, 3, 5, "example.com", keystr1, &key); - assert_int_equal(dns_keytable_deletekeynode(keytable, key), + create_keystruct(257, 3, 5, keystr1, &dnskey); + assert_int_equal(dns_keytable_deletekey(keytable, keyname, &dnskey), ISC_R_SUCCESS); - assert_int_equal(dns_keytable_deletekeynode(keytable, key), + assert_int_equal(dns_keytable_deletekey(keytable, keyname, &dnskey), ISC_R_NOTFOUND); - assert_int_equal(dns_keytable_delete(keytable, - str2name("example.com")), + assert_int_equal(dns_keytable_delete(keytable, keyname), ISC_R_NOTFOUND); - dst_key_free(&key); + dns_rdata_freestruct(&dnskey); /* * A null key node for a name is not deleted when searched by key; * it must be deleted by dns_keytable_delete() */ - create_key(257, 3, 5, "null.example", keystr1, &key); - assert_int_equal(dns_keytable_deletekeynode(keytable, key), + dns_test_namefromstring("null.example", &fn); + create_keystruct(257, 3, 5, keystr1, &dnskey); + assert_int_equal(dns_keytable_deletekey(keytable, keyname, &dnskey), DNS_R_PARTIALMATCH); - assert_int_equal(dns_keytable_delete(keytable, dst_key_name(key)), + assert_int_equal(dns_keytable_delete(keytable, keyname), ISC_R_SUCCESS); - dst_key_free(&key); + dns_rdata_freestruct(&dnskey); destroy_tables(); } @@ -756,7 +785,7 @@ main(void) { _setup, _teardown), cmocka_unit_test_setup_teardown(delete_test, _setup, _teardown), - cmocka_unit_test_setup_teardown(deletekeynode_test, + cmocka_unit_test_setup_teardown(deletekey_test, _setup, _teardown), cmocka_unit_test_setup_teardown(find_test, _setup, _teardown), diff --git a/lib/dns/validator.c b/lib/dns/validator.c index 545c8a2229..3ba10feb0d 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -1356,23 +1356,13 @@ compute_keytag(dns_rdata_t *rdata) { */ static bool selfsigned_dnskey(dns_validator_t *val) { - dns_rdataset_t *rdataset, *sigrdataset; - dns_rdata_t rdata = DNS_RDATA_INIT; - dns_rdata_t sigrdata = DNS_RDATA_INIT; - dns_rdata_dnskey_t key; - dns_rdata_rrsig_t sig; - dns_keytag_t keytag; - dns_name_t *name; + dns_rdataset_t *rdataset = val->event->rdataset; + dns_rdataset_t *sigrdataset = val->event->sigrdataset; + dns_name_t *name = val->event->name; isc_result_t result; - dst_key_t *dstkey; - isc_mem_t *mctx; + isc_mem_t *mctx = val->view->mctx; bool answer = false; - rdataset = val->event->rdataset; - sigrdataset = val->event->sigrdataset; - name = val->event->name; - mctx = val->view->mctx; - if (rdataset->type != dns_rdatatype_dnskey) { return (false); } @@ -1381,15 +1371,24 @@ selfsigned_dnskey(dns_validator_t *val) { result == ISC_R_SUCCESS; result = dns_rdataset_next(rdataset)) { - dns_rdata_reset(&rdata); - dns_rdataset_current(rdataset, &rdata); - result = dns_rdata_tostruct(&rdata, &key, NULL); + dns_rdata_t keyrdata = DNS_RDATA_INIT; + dns_rdata_t sigrdata = DNS_RDATA_INIT; + dns_rdata_dnskey_t key; + dns_rdata_rrsig_t sig; + dns_keytag_t keytag; + + dns_rdata_reset(&keyrdata); + dns_rdataset_current(rdataset, &keyrdata); + result = dns_rdata_tostruct(&keyrdata, &key, NULL); RUNTIME_CHECK(result == ISC_R_SUCCESS); - keytag = compute_keytag(&rdata); + keytag = compute_keytag(&keyrdata); + for (result = dns_rdataset_first(sigrdataset); result == ISC_R_SUCCESS; result = dns_rdataset_next(sigrdataset)) { + dst_key_t *dstkey = NULL; + dns_rdata_reset(&sigrdata); dns_rdataset_current(sigrdataset, &sigrdata); result = dns_rdata_tostruct(&sigrdata, &sig, NULL); @@ -1402,8 +1401,7 @@ selfsigned_dnskey(dns_validator_t *val) { continue; } - dstkey = NULL; - result = dns_dnssec_keyfromrdata(name, &rdata, mctx, + result = dns_dnssec_keyfromrdata(name, &keyrdata, mctx, &dstkey); if (result != ISC_R_SUCCESS) { continue; @@ -1417,11 +1415,13 @@ selfsigned_dnskey(dns_validator_t *val) { if (result != ISC_R_SUCCESS) { continue; } + if ((key.flags & DNS_KEYFLAG_REVOKE) == 0) { answer = true; continue; } - dns_view_untrust(val->view, name, &key, mctx); + + dns_view_untrust(val->view, name, &key); } } diff --git a/lib/dns/view.c b/lib/dns/view.c index 46963da12d..f21d065cfc 100644 --- a/lib/dns/view.c +++ b/lib/dns/view.c @@ -1864,34 +1864,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, isc_mem_t *mctx) + dns_rdata_dnskey_t *dnskey) { isc_result_t result; - unsigned char data[4096]; - dns_rdata_t rdata = DNS_RDATA_INIT; - isc_buffer_t buffer; - dst_key_t *key = NULL; dns_keytable_t *sr = NULL; + 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; - /* Convert dnskey to DST key. */ - isc_buffer_init(&buffer, data, sizeof(data)); - dns_rdata_fromstruct(&rdata, dnskey->common.rdclass, - dns_rdatatype_dnskey, dnskey, &buffer); - - result = dns_dnssec_keyfromrdata(keyname, &rdata, mctx, &key); - if (result != ISC_R_SUCCESS) - return; - result = dns_view_getsecroots(view, &sr); - if (result == ISC_R_SUCCESS) { - result = dns_keytable_deletekeynode(sr, key); + if (result != ISC_R_SUCCESS) { + return; + } + result = dns_keytable_deletekey(sr, keyname, dnskey); + if (result == ISC_R_SUCCESS) { /* * If key was found in secroots, then it was a * configured trust anchor, and we want to fail @@ -1899,14 +1893,10 @@ dns_view_untrust(dns_view_t *view, const dns_name_t *keyname, * then leave a null key so that we can't validate * anymore. */ - - if (result == ISC_R_SUCCESS) - dns_keytable_marksecure(sr, keyname); - - dns_keytable_detach(&sr); + dns_keytable_marksecure(sr, keyname); } - dst_key_free(&key); + dns_keytable_detach(&sr); } /* diff --git a/lib/dns/win32/libdns.def.in b/lib/dns/win32/libdns.def.in index 65c4a7ddec..708b97242c 100644 --- a/lib/dns/win32/libdns.def.in +++ b/lib/dns/win32/libdns.def.in @@ -472,7 +472,7 @@ dns_keytable_attach dns_keytable_attachkeynode dns_keytable_create dns_keytable_delete -dns_keytable_deletekeynode +dns_keytable_deletekey dns_keytable_detach dns_keytable_detachkeynode dns_keytable_dump diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 1de8b8283e..39464bf8a9 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -10173,7 +10173,7 @@ keyfetch_done(isc_task_t *task, isc_event_t *event) { * Remove key from secroots. */ dns_view_untrust(zone->view, keyname, - &dnskey, mctx); + &dnskey); /* If initializing, delete now */ if (keydata.addhd == 0) { From 7fdf40770fec93fd619c4b72dc9be44b24aad58d Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 19 Dec 2019 19:39:33 -0800 Subject: [PATCH 2/6] remove all code that uses non-DS trust anchors as initial-key and static-key trust anchors will now be stored as a DS rrset, code referencing keynodes storing DNSKEY trust anchors will no longer be reached. --- bin/named/server.c | 25 +------ lib/dns/include/dns/keytable.h | 86 +--------------------- lib/dns/keytable.c | 131 +-------------------------------- lib/dns/tests/keytable_test.c | 107 +++------------------------ lib/dns/validator.c | 113 ---------------------------- lib/dns/win32/libdns.def.in | 4 - lib/dns/zone.c | 85 ++------------------- lib/dns/zoneverify.c | 33 --------- lib/ns/query.c | 15 +--- 9 files changed, 28 insertions(+), 571 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index 31bec0ec50..1b8ee399c4 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -6835,11 +6835,7 @@ struct dotat_arg { * reported in the TAT query. */ static isc_result_t -get_tat_qname(dns_name_t *target, dns_name_t *keyname, - dns_keytable_t *keytable, dns_keynode_t *keynode) -{ - dns_keynode_t *firstnode = keynode; - dns_keynode_t *nextnode = NULL; +get_tat_qname(dns_name_t *target, dns_name_t *keyname, dns_keynode_t *keynode) { dns_rdataset_t *dsset = NULL; unsigned int i, n = 0; uint16_t ids[12]; @@ -6866,23 +6862,6 @@ get_tat_qname(dns_name_t *target, dns_name_t *keyname, n++; } } - } else { - do { - dst_key_t *key = dns_keynode_key(keynode); - if (key != NULL) { - if (n < (sizeof(ids)/sizeof(ids[0]))) { - ids[n] = dst_key_id(key); - n++; - } - } - nextnode = NULL; - (void)dns_keytable_nextkeynode(keytable, keynode, - &nextnode); - if (keynode != firstnode) { - dns_keytable_detachkeynode(keytable, &keynode); - } - keynode = nextnode; - } while (keynode != NULL); } if (n == 0) { @@ -6938,7 +6917,7 @@ dotat(dns_keytable_t *keytable, dns_keynode_t *keynode, task = dotat_arg->task; tatname = dns_fixedname_initname(&fixed); - result = get_tat_qname(tatname, keyname, keytable, keynode); + result = get_tat_qname(tatname, keyname, keynode); if (result != ISC_R_SUCCESS) { return; } diff --git a/lib/dns/include/dns/keytable.h b/lib/dns/include/dns/keytable.h index 2d8113046f..4edba81a5b 100644 --- a/lib/dns/include/dns/keytable.h +++ b/lib/dns/include/dns/keytable.h @@ -223,9 +223,8 @@ isc_result_t dns_keytable_find(dns_keytable_t *keytable, const dns_name_t *keyname, dns_keynode_t **keynodep); /*%< - * Search for the first instance of a key named 'name' in 'keytable', - * without regard to keyid and algorithm. Use dns_keytable_nextkeynode() - * to find subsequent instances. + * Search for the first instance of a trust anchor named 'name' in + * 'keytable', without regard to keyid and algorithm. * * Requires: * @@ -243,78 +242,6 @@ dns_keytable_find(dns_keytable_t *keytable, const dns_name_t *keyname, *\li Any other result indicates an error. */ -isc_result_t -dns_keytable_nextkeynode(dns_keytable_t *keytable, dns_keynode_t *keynode, - dns_keynode_t **nextnodep); -/*%< - * Return for the next key after 'keynode' in 'keytable', without regard to - * keyid and algorithm. - * - * Requires: - * - *\li 'keytable' is a valid keytable. - * - *\li 'keynode' is a valid keynode. - * - *\li nextnodep != NULL && *nextnodep == NULL - * - * Returns: - * - *\li ISC_R_SUCCESS - *\li ISC_R_NOTFOUND - * - *\li Any other result indicates an error. - */ - -isc_result_t -dns_keytable_findkeynode(dns_keytable_t *keytable, const dns_name_t *name, - dns_secalg_t algorithm, dns_keytag_t tag, - dns_keynode_t **keynodep); -/*%< - * Search for a key named 'name', matching 'algorithm' and 'tag' in - * 'keytable'. This finds the first instance which matches. Use - * dns_keytable_findnextkeynode() to find other instances. - * - * Requires: - * - *\li 'keytable' is a valid keytable. - * - *\li 'name' is a valid absolute name. - * - *\li keynodep != NULL && *keynodep == NULL - * - * Returns: - * - *\li ISC_R_SUCCESS - *\li DNS_R_PARTIALMATCH the name existed in the keytable. - *\li ISC_R_NOTFOUND - * - *\li Any other result indicates an error. - */ - -isc_result_t -dns_keytable_findnextkeynode(dns_keytable_t *keytable, dns_keynode_t *keynode, - dns_keynode_t **nextnodep); -/*%< - * Search for the next key with the same properties as 'keynode' in - * 'keytable' as found by dns_keytable_findkeynode(). - * - * Requires: - * - *\li 'keytable' is a valid keytable. - * - *\li 'keynode' is a valid keynode. - * - *\li nextnodep != NULL && *nextnodep == NULL - * - * Returns: - * - *\li ISC_R_SUCCESS - *\li ISC_R_NOTFOUND - * - *\li Any other result indicates an error. - */ - isc_result_t dns_keytable_finddeepestmatch(dns_keytable_t *keytable, const dns_name_t *name, dns_name_t *foundname); @@ -413,12 +340,6 @@ dns_keytable_totext(dns_keytable_t *keytable, isc_buffer_t **buf); * Dump the keytable to buffer at 'buf' */ -dst_key_t * -dns_keynode_key(dns_keynode_t *keynode); -/*%< - * Get the DST key associated with keynode. - */ - dns_rdataset_t * dns_keynode_dsset(dns_keynode_t *keynode); /*%< @@ -466,8 +387,7 @@ dns_keynode_attach(dns_keynode_t *source, dns_keynode_t **target); void dns_keynode_detach(isc_mem_t *mctx, dns_keynode_t **target); /*%< - * Detach a single keynode, without touching any keynodes that - * may be pointed to by its 'next' pointer + * Detach a keynode. */ void diff --git a/lib/dns/keytable.c b/lib/dns/keytable.c index bf1df1ed95..2a65c7caab 100644 --- a/lib/dns/keytable.c +++ b/lib/dns/keytable.c @@ -516,123 +516,6 @@ dns_keytable_find(dns_keytable_t *keytable, const dns_name_t *keyname, return (result); } -isc_result_t -dns_keytable_nextkeynode(dns_keytable_t *keytable, dns_keynode_t *keynode, - dns_keynode_t **nextnodep) -{ - /* - * Return the next key after 'keynode', regardless of - * properties. - */ - - REQUIRE(VALID_KEYTABLE(keytable)); - REQUIRE(VALID_KEYNODE(keynode)); - REQUIRE(nextnodep != NULL && *nextnodep == NULL); - - if (keynode->next == NULL) { - return (ISC_R_NOTFOUND); - } - - dns_keytable_attachkeynode(keytable, keynode->next, nextnodep); - - return (ISC_R_SUCCESS); -} - -isc_result_t -dns_keytable_findkeynode(dns_keytable_t *keytable, const dns_name_t *name, - dns_secalg_t algorithm, dns_keytag_t tag, - dns_keynode_t **keynodep) -{ - isc_result_t result; - dns_keynode_t *knode; - void *data; - - /* - * Search for a key named 'name', matching 'algorithm' and 'tag' in - * 'keytable'. - */ - - REQUIRE(VALID_KEYTABLE(keytable)); - REQUIRE(dns_name_isabsolute(name)); - REQUIRE(keynodep != NULL && *keynodep == NULL); - - RWLOCK(&keytable->rwlock, isc_rwlocktype_read); - - /* - * Note we don't want the DNS_R_PARTIALMATCH from dns_rbt_findname() - * as that indicates that 'name' was not found. - * - * DNS_R_PARTIALMATCH indicates that the name was found but we - * didn't get a match on algorithm and key id arguments. - */ - knode = NULL; - data = NULL; - result = dns_rbt_findname(keytable->table, name, 0, NULL, &data); - if (result == ISC_R_SUCCESS) { - INSIST(data != NULL); - - for (knode = data; knode != NULL; knode = knode->next) { - if (knode->key == NULL) { - knode = NULL; - break; - } - if (algorithm == dst_key_alg(knode->key) && - tag == dst_key_id(knode->key)) - { - break; - } - } - if (knode != NULL) { - dns_keytable_attachkeynode(keytable, knode, keynodep); - } else { - result = DNS_R_PARTIALMATCH; - } - } else if (result == DNS_R_PARTIALMATCH) { - result = ISC_R_NOTFOUND; - } - - RWUNLOCK(&keytable->rwlock, isc_rwlocktype_read); - - return (result); -} - -isc_result_t -dns_keytable_findnextkeynode(dns_keytable_t *keytable, dns_keynode_t *keynode, - dns_keynode_t **nextnodep) -{ - isc_result_t result; - dns_keynode_t *knode; - - /* - * Search for the next key with the same properties as 'keynode' in - * 'keytable'. - */ - - REQUIRE(VALID_KEYTABLE(keytable)); - REQUIRE(VALID_KEYNODE(keynode)); - REQUIRE(nextnodep != NULL && *nextnodep == NULL); - - for (knode = keynode->next; knode != NULL; knode = knode->next) { - if (knode->key == NULL) { - knode = NULL; - break; - } - if (dst_key_alg(keynode->key) == dst_key_alg(knode->key) && - dst_key_id(keynode->key) == dst_key_id(knode->key)) - { - break; - } - } - if (knode != NULL) { - dns_keytable_attachkeynode(keytable, knode, nextnodep); - result = ISC_R_SUCCESS; - } else { - result = ISC_R_NOTFOUND; - } - - return (result); -} - isc_result_t dns_keytable_finddeepestmatch(dns_keytable_t *keytable, const dns_name_t *name, dns_name_t *foundname) @@ -674,7 +557,8 @@ dns_keytable_attachkeynode(dns_keytable_t *keytable, dns_keynode_t *source, REQUIRE(VALID_KEYNODE(source)); REQUIRE(target != NULL && *target == NULL); - REQUIRE(atomic_fetch_add_relaxed(&keytable->active_nodes, 1) < UINT32_MAX); + REQUIRE(atomic_fetch_add_relaxed(&keytable->active_nodes, + 1) < UINT32_MAX); dns_keynode_attach(source, target); } @@ -913,8 +797,8 @@ dns_keytable_forall(dns_keytable_t *keytable, } goto cleanup; } - REQUIRE(atomic_fetch_add_relaxed(&keytable->active_nodes, 1) - < UINT32_MAX); + REQUIRE(atomic_fetch_add_relaxed(&keytable->active_nodes, + 1) < UINT32_MAX); for (;;) { dns_rbtnodechain_current(&chain, foundname, origin, &node); if (node->data != NULL) { @@ -939,13 +823,6 @@ dns_keytable_forall(dns_keytable_t *keytable, return (result); } -dst_key_t * -dns_keynode_key(dns_keynode_t *keynode) { - REQUIRE(VALID_KEYNODE(keynode)); - - return (keynode->key); -} - dns_rdataset_t * dns_keynode_dsset(dns_keynode_t *keynode) { REQUIRE(VALID_KEYNODE(keynode)); diff --git a/lib/dns/tests/keytable_test.c b/lib/dns/tests/keytable_test.c index b90b1f7df8..ac3066eafd 100644 --- a/lib/dns/tests/keytable_test.c +++ b/lib/dns/tests/keytable_test.c @@ -219,7 +219,6 @@ static void add_test(void **state) { dst_key_t *key = NULL; dns_keynode_t *keynode = NULL; - dns_keynode_t *next_keynode = NULL; dns_keynode_t *null_keynode = NULL; UNUSED(state); @@ -227,27 +226,24 @@ add_test(void **state) { create_tables(); /* - * Get the keynode for the example.com key. There's no other key for - * the name, so nextkeynode() should return NOTFOUND. + * Getting the keynode for the example.com key should succeed. */ assert_int_equal(dns_keytable_find(keytable, str2name("example.com"), &keynode), ISC_R_SUCCESS); - assert_int_equal(dns_keytable_nextkeynode(keytable, keynode, - &next_keynode), - ISC_R_NOTFOUND); /* - * Try to add the same key. This should have no effect, so - * nextkeynode() should still return NOTFOUND. + * Try to add the same key. This should have no effect but + * report success. */ create_key(257, 3, 5, "example.com", keystr1, &key); assert_int_equal(dns_keytable_add(keytable, false, false, dst_key_name(key), &key, NULL), ISC_R_SUCCESS); - assert_int_equal(dns_keytable_nextkeynode(keytable, keynode, - &next_keynode), - ISC_R_NOTFOUND); + dns_keytable_detachkeynode(keytable, &keynode); + assert_int_equal(dns_keytable_find(keytable, str2name("example.com"), + &keynode), + ISC_R_SUCCESS); /* Add another key (different keydata) */ dns_keytable_detachkeynode(keytable, &keynode); @@ -258,24 +254,16 @@ add_test(void **state) { assert_int_equal(dns_keytable_find(keytable, str2name("example.com"), &keynode), ISC_R_SUCCESS); - assert_int_equal(dns_keytable_nextkeynode(keytable, keynode, - &next_keynode), - ISC_R_SUCCESS); - dns_keytable_detachkeynode(keytable, &next_keynode); dns_keytable_detachkeynode(keytable, &keynode); /* - * Get the keynode for the managed.com key. There's no other key for - * the name, so nextkeynode() should return NOTFOUND. Ensure the + * Get the keynode for the managed.com key. Ensure the * retrieved key is an initializing key, then mark it as trusted using * dns_keynode_trust() and ensure the latter works as expected. */ assert_int_equal(dns_keytable_find(keytable, str2name("managed.com"), &keynode), ISC_R_SUCCESS); - assert_int_equal(dns_keytable_nextkeynode(keytable, keynode, - &next_keynode), - ISC_R_NOTFOUND); assert_int_equal(dns_keynode_initial(keynode), true); dns_keynode_trust(keynode); assert_int_equal(dns_keynode_initial(keynode), false); @@ -283,8 +271,7 @@ add_test(void **state) { /* * Add a different managed key for managed.com, marking it as an - * initializing key. Ensure nextkeynode() no longer returns - * ISC_R_NOTFOUND and that the added key is an initializing key. + * initializing key. */ create_key(257, 3, 5, "managed.com", keystr2, &key); assert_int_equal(dns_keytable_add(keytable, true, true, @@ -293,11 +280,7 @@ add_test(void **state) { assert_int_equal(dns_keytable_find(keytable, str2name("managed.com"), &keynode), ISC_R_SUCCESS); - assert_int_equal(dns_keytable_nextkeynode(keytable, keynode, - &next_keynode), - ISC_R_SUCCESS); assert_int_equal(dns_keynode_initial(keynode), true); - dns_keytable_detachkeynode(keytable, &next_keynode); dns_keytable_detachkeynode(keytable, &keynode); /* @@ -314,22 +297,11 @@ add_test(void **state) { &keynode), ISC_R_SUCCESS); assert_int_equal(dns_keynode_initial(keynode), false); - assert_int_equal(dns_keytable_nextkeynode(keytable, keynode, - &next_keynode), - ISC_R_SUCCESS); - dns_keytable_detachkeynode(keytable, &keynode); - keynode = next_keynode; - next_keynode = NULL; - assert_int_equal(dns_keynode_initial(keynode), false); - assert_int_equal(dns_keytable_nextkeynode(keytable, keynode, - &next_keynode), - ISC_R_NOTFOUND); dns_keytable_detachkeynode(keytable, &keynode); /* * Add a managed key at a new node, two.com, marking it as an - * initializing key. Ensure nextkeynode() returns ISC_R_NOTFOUND and - * that the added key is an initializing key. + * initializing key. */ create_key(257, 3, 5, "two.com", keystr1, &key); assert_int_equal(dns_keytable_add(keytable, true, true, @@ -338,16 +310,12 @@ add_test(void **state) { assert_int_equal(dns_keytable_find(keytable, str2name("two.com"), &keynode), ISC_R_SUCCESS); - assert_int_equal(dns_keytable_nextkeynode(keytable, keynode, - &next_keynode), - ISC_R_NOTFOUND); assert_int_equal(dns_keynode_initial(keynode), true); dns_keytable_detachkeynode(keytable, &keynode); /* * Add a different managed key for two.com, marking it as a - * non-initializing key. Ensure nextkeynode() no longer returns - * ISC_R_NOTFOUND and that the added key is not an initializing key. + * non-initializing key. */ create_key(257, 3, 5, "two.com", keystr2, &key); assert_int_equal(dns_keytable_add(keytable, true, false, @@ -356,10 +324,7 @@ add_test(void **state) { assert_int_equal(dns_keytable_find(keytable, str2name("two.com"), &keynode), ISC_R_SUCCESS); - assert_int_equal(dns_keytable_nextkeynode(keytable, keynode, - &next_keynode), ISC_R_SUCCESS); assert_int_equal(dns_keynode_initial(keynode), false); - dns_keytable_detachkeynode(keytable, &next_keynode); dns_keytable_detachkeynode(keytable, &keynode); /* @@ -376,16 +341,6 @@ add_test(void **state) { &keynode), ISC_R_SUCCESS); assert_int_equal(dns_keynode_initial(keynode), false); - assert_int_equal(dns_keytable_nextkeynode(keytable, keynode, - &next_keynode), - ISC_R_SUCCESS); - dns_keytable_detachkeynode(keytable, &keynode); - keynode = next_keynode; - next_keynode = NULL; - assert_int_equal(dns_keynode_initial(keynode), false); - assert_int_equal(dns_keytable_nextkeynode(keytable, keynode, - &next_keynode), - ISC_R_NOTFOUND); dns_keytable_detachkeynode(keytable, &keynode); /* @@ -403,13 +358,11 @@ add_test(void **state) { &keynode), ISC_R_SUCCESS); assert_int_equal(keynode, null_keynode); /* should be the same node */ - assert_non_null(dns_keynode_key(keynode)); /* now have a key */ dns_keytable_detachkeynode(keytable, &null_keynode); /* * Try to add a null key to a name that already has a key. It's - * effectively no-op, so the same key node is still there, with no - * no next node. + * effectively no-op, so the same key node is still there. * (Note: this and above checks confirm that if a name has a null key * that's the only key for the name). */ @@ -420,10 +373,6 @@ add_test(void **state) { &null_keynode), ISC_R_SUCCESS); assert_int_equal(keynode, null_keynode); - assert_non_null(dns_keynode_key(keynode)); - assert_int_equal(dns_keytable_nextkeynode(keytable, keynode, - &next_keynode), - ISC_R_NOTFOUND); dns_keytable_detachkeynode(keytable, &null_keynode); dns_keytable_detachkeynode(keytable, &keynode); @@ -553,7 +502,6 @@ find_test(void **state) { str2name("null.example"), &keynode), ISC_R_SUCCESS); - assert_int_equal(dns_keynode_key(keynode), NULL); dns_keytable_detachkeynode(keytable, &keynode); /* @@ -581,37 +529,6 @@ find_test(void **state) { ISC_R_SUCCESS); assert_true(dns_name_equal(name, str2name("null.example"))); - /* - * dns_keytable_findkeynode() requires exact name, algorithm, keytag - * match. If algorithm or keytag doesn't match, should result in - * PARTIALMATCH. Same for a node with a null key. - */ - assert_int_equal(dns_keytable_findkeynode(keytable, - str2name("example.org"), - 5, keytag1, &keynode), - ISC_R_NOTFOUND); - assert_int_equal(dns_keytable_findkeynode(keytable, - str2name("sub.example.com"), - 5, keytag1, &keynode), - ISC_R_NOTFOUND); - assert_int_equal(dns_keytable_findkeynode(keytable, - str2name("example.com"), - 4, keytag1, &keynode), - DNS_R_PARTIALMATCH); /* different algorithm */ - assert_int_equal(dns_keytable_findkeynode(keytable, - str2name("example.com"), - 5, keytag1 + 1, &keynode), - DNS_R_PARTIALMATCH); /* different keytag */ - assert_int_equal(dns_keytable_findkeynode(keytable, - str2name("null.example"), - 5, 0, &keynode), - DNS_R_PARTIALMATCH); /* null key */ - assert_int_equal(dns_keytable_findkeynode(keytable, - str2name("example.com"), - 5, keytag1, &keynode), - ISC_R_SUCCESS); /* complete match */ - dns_keytable_detachkeynode(keytable, &keynode); - destroy_tables(); } diff --git a/lib/dns/validator.c b/lib/dns/validator.c index 3ba10feb0d..ee6f98e0d9 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -1684,110 +1684,6 @@ check_signer(dns_validator_t *val, dns_rdata_t *keyrdata, return (result); } -static isc_result_t -anchor_signed(dns_validator_t *val, isc_result_t *resp) { - isc_result_t result; - bool atsep = false; - - /* - * First, see if this key was signed by a trust anchor. - */ - for (result = dns_rdataset_first(val->event->sigrdataset); - result == ISC_R_SUCCESS; - result = dns_rdataset_next(val->event->sigrdataset)) - { - dns_keynode_t *keynode = NULL; - dns_rdata_t sigrdata = DNS_RDATA_INIT; - dns_fixedname_t fixed; - dns_name_t *found; - dst_key_t *dstkey; - dns_rdata_rrsig_t sig; - - found = dns_fixedname_initname(&fixed); - dns_rdata_reset(&sigrdata); - dns_rdataset_current(val->event->sigrdataset, &sigrdata); - result = dns_rdata_tostruct(&sigrdata, &sig, NULL); - RUNTIME_CHECK(result == ISC_R_SUCCESS); - - if (!dns_name_equal(val->event->name, &sig.signer)) { - continue; - } - - result = dns_keytable_findkeynode(val->keytable, - val->event->name, - sig.algorithm, sig.keyid, - &keynode); - if (result == ISC_R_NOTFOUND) { - result = dns_keytable_finddeepestmatch(val->keytable, - val->event->name, - found); - if (result != ISC_R_SUCCESS) { - validator_log(val, ISC_LOG_DEBUG(3), - "not beneath secure root"); - *resp = markanswer(val, "validate_dnskey (1)", - "not beneath secure root"); - return (ISC_R_COMPLETE); - } - continue; - } - - if (result == DNS_R_PARTIALMATCH || result == ISC_R_SUCCESS) { - atsep = true; - } - - while (result == ISC_R_SUCCESS) { - dns_keynode_t *nextnode = NULL; - dstkey = dns_keynode_key(keynode); - if (dstkey == NULL) { - dns_keytable_detachkeynode(val->keytable, - &keynode); - break; - } - - result = verify(val, dstkey, &sigrdata, sig.keyid); - if (result == ISC_R_SUCCESS) { - dns_keytable_detachkeynode(val->keytable, - &keynode); - break; - } - - result = dns_keytable_findnextkeynode(val->keytable, - keynode, - &nextnode); - dns_keytable_detachkeynode(val->keytable, &keynode); - keynode = nextnode; - } - - if (result == ISC_R_SUCCESS) { - marksecure(val->event); - validator_log(val, ISC_LOG_DEBUG(3), - "signed by trusted key; " - "marking as secure"); - *resp = result; - return (ISC_R_COMPLETE); - } - } - - if (atsep) { - /* - * We have not found a key to verify this DNSKEY - * RRset, but there is a trust anchor defined for this - * name, so we have to assume that the RRset is invalid. - */ - char namebuf[DNS_NAME_FORMATSIZE]; - - dns_name_format(val->event->name, namebuf, sizeof(namebuf)); - validator_log(val, ISC_LOG_NOTICE, - "unable to find a DNSKEY which verifies " - "the DNSKEY RRset and also matches a " - "trusted key for '%s'", namebuf); - *resp = DNS_R_NOVALIDKEY; - return (ISC_R_COMPLETE); - } - - return (result); -} - /* * get_dsset is called to look up a DS RRset corresponding to the name * of a DNSKEY record, either in the cache or, if necessary, by starting a @@ -1924,15 +1820,6 @@ validate_dnskey(dns_validator_t *val) { if (val->dsset == NULL) { isc_result_t tresult = ISC_R_SUCCESS; - /* - * First, check whether the key to be validated was - * signed by a trust anchor. - */ - result = anchor_signed(val, &tresult); - if (result == ISC_R_COMPLETE) { - return (tresult); - } - /* * If this is the root name and there was no trusted key, * we can give up now, since there's no DS at the root. diff --git a/lib/dns/win32/libdns.def.in b/lib/dns/win32/libdns.def.in index 708b97242c..42e8ab222a 100644 --- a/lib/dns/win32/libdns.def.in +++ b/lib/dns/win32/libdns.def.in @@ -463,7 +463,6 @@ dns_keynode_detach dns_keynode_detachall dns_keynode_dsset dns_keynode_initial -dns_keynode_key dns_keynode_managed dns_keynode_trust dns_keyring_restore @@ -478,12 +477,9 @@ dns_keytable_detachkeynode dns_keytable_dump dns_keytable_find dns_keytable_finddeepestmatch -dns_keytable_findkeynode -dns_keytable_findnextkeynode dns_keytable_forall dns_keytable_issecuredomain dns_keytable_marksecure -dns_keytable_nextkeynode dns_keytable_totext dns_lib_init dns_lib_shutdown diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 39464bf8a9..80e3085a45 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -3857,12 +3857,9 @@ create_keydata(dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *ver, isc_stdtime_get(&now); /* - * If the keynode has neither a key nor a DS RRset, - * we shouldn't be here. + * If the keynode has no trust anchor set, we shouldn't be here. */ - if (dns_keynode_key(keynode) == NULL && - dns_keynode_dsset(keynode) == NULL) - { + if (dns_keynode_dsset(keynode) == NULL) { return (ISC_R_FAILURE); } @@ -4265,12 +4262,9 @@ addifmissing(dns_keytable_t *keytable, dns_keynode_t *keynode, } /* - * If the keynode has neither a key-style nor a DS-style - * trust anchor, return. + * If the keynode has no trust anchor set, return. */ - if (dns_keynode_dsset(keynode) == NULL && - dns_keynode_key(keynode) == NULL) - { + if (dns_keynode_dsset(keynode) == NULL) { return; } @@ -9823,8 +9817,7 @@ keyfetch_done(isc_task_t *task, isc_event_t *event) { } /* - * If the first keynode has a DS trust anchor, use that for - * verification. + * If the keynode has a DS trust anchor, use it for verification. */ if ((dsset = dns_keynode_dsset(keynode)) != NULL) { for (result = dns_rdataset_first(dnskeysigs); @@ -9897,74 +9890,6 @@ keyfetch_done(isc_task_t *task, isc_event_t *event) { break; } } - goto anchors_done; - } else { - dns_keytable_detachkeynode(secroots, &keynode); - } - - /* - * Validate the DNSKEY set against using the key-style - * trust anchor(s). - */ - for (result = dns_rdataset_first(dnskeysigs); - result == ISC_R_SUCCESS; - result = dns_rdataset_next(dnskeysigs)) - { - result = dns_keytable_find(secroots, keyname, &keynode); - if (result != ISC_R_SUCCESS) { - goto anchors_done; - } - dns_rdata_reset(&sigrr); - dns_rdataset_current(dnskeysigs, &sigrr); - result = dns_rdata_tostruct(&sigrr, &sig, NULL); - RUNTIME_CHECK(result == ISC_R_SUCCESS); - - result = ISC_R_SUCCESS; - while (result == ISC_R_SUCCESS) { - dns_keynode_t *nextnode = NULL; - - dstkey = dns_keynode_key(keynode); - if (dstkey == NULL) { - /* fail_secure() was called */ - break; - } - - if (dst_key_alg(dstkey) == sig.algorithm && - dst_key_id(dstkey) == sig.keyid) - { - result = dns_dnssec_verify(keyname, dnskeys, - dstkey, false, 0, - mctx, &sigrr, NULL); - - dnssec_log(zone, ISC_LOG_DEBUG(3), - "Verifying DNSKEY set " - "for zone '%s' " - "using key %d/%d: %s", - namebuf, sig.keyid, - sig.algorithm, - dns_result_totext(result)); - - if (result == ISC_R_SUCCESS) { - dnskeys->trust = dns_trust_secure; - dnskeysigs->trust = dns_trust_secure; - secure = true; - initial = dns_keynode_initial(keynode); - dns_keynode_trust(keynode); - break; - } - } - - result = dns_keytable_nextkeynode(secroots, keynode, - &nextnode); - if (result == ISC_R_SUCCESS) { - dns_keytable_detachkeynode(secroots, &keynode); - keynode = nextnode; - } - } - dns_keytable_detachkeynode(secroots, &keynode); - if (secure) { - break; - } } anchors_done: diff --git a/lib/dns/zoneverify.c b/lib/dns/zoneverify.c index 265fdf6dbe..c19367b0a0 100644 --- a/lib/dns/zoneverify.c +++ b/lib/dns/zoneverify.c @@ -1614,39 +1614,6 @@ check_dnskey_sigs(vctx_t *vctx, const dns_rdata_dnskey_t *dnskey, goto cleanup; } - /* - * The keynode didn't have any DS trust anchors, so we now try to - * find a matching DNSKEY trust anchor. - */ - result = dns_keytable_findkeynode(vctx->secroots, vctx->origin, - dst_key_alg(key), dst_key_id(key), - &keynode); - if (result != ISC_R_SUCCESS) { - goto cleanup; - } - - /* - * Walk the keynode list until we find a matching key or - * reach the end. - */ - while (result == ISC_R_SUCCESS) { - dns_keynode_t *nextnode = NULL; - - if (dst_key_compare(key, dns_keynode_key(keynode))) { - dns_keytable_detachkeynode(vctx->secroots, &keynode); - dns_rdataset_settrust(&vctx->keyset, dns_trust_secure); - dns_rdataset_settrust(&vctx->keysigs, dns_trust_secure); - *goodkey = true; - - goto cleanup; - } - - result = dns_keytable_findnextkeynode(vctx->secroots, - keynode, &nextnode); - dns_keytable_detachkeynode(vctx->secroots, &keynode); - keynode = nextnode; - } - cleanup: if (keynode != NULL) { dns_keytable_detachkeynode(vctx->secroots, &keynode); diff --git a/lib/ns/query.c b/lib/ns/query.c index 50876a4f84..f5ddfcb88e 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -6634,23 +6634,12 @@ has_ta(query_ctx_t *qctx) { } } - while (result == ISC_R_SUCCESS) { - dns_keynode_t *nextnode = NULL; - dns_keytag_t keyid = dst_key_id(dns_keynode_key(keynode)); - if (keyid == sentinel) { - dns_keytable_detachkeynode(keytable, &keynode); - dns_keytable_detach(&keytable); - return (true); - } - result = dns_keytable_nextkeynode(keytable, keynode, &nextnode); - dns_keytable_detachkeynode(keytable, &keynode); - keynode = nextnode; - } - if (keynode != NULL) { dns_keytable_detachkeynode(keytable, &keynode); } + dns_keytable_detach(&keytable); + return (false); } From b984a4b64733bb5924c7231ebdacb89f2d486577 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Fri, 20 Dec 2019 11:37:11 -0800 Subject: [PATCH 3/6] disable adding keys to keytable; only DS trust anchors can now be added the internal keytable structure has not yet been changed, but insertion of DS anchors is the only method now available. NOTE: the keytable unit test is currently failing because of tests that expect individual keynode objects to contain single DST key objects. --- bin/named/server.c | 145 ++++++++++--------------------- bin/tests/system/dnssec/tests.sh | 12 +-- lib/dns/client.c | 45 +++++----- lib/dns/ds.c | 72 ++++++++------- lib/dns/include/dns/ds.h | 19 +++- lib/dns/include/dns/keytable.h | 27 ++---- lib/dns/keytable.c | 10 +-- lib/dns/tests/keytable_test.c | 107 ++++++++++++----------- lib/dns/win32/libdns.def.in | 1 + lib/dns/zone.c | 35 ++++---- 10 files changed, 218 insertions(+), 255 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index 1b8ee399c4..14088fb3c6 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -700,11 +700,12 @@ configure_view_nametable(const cfg_obj_t *vconfig, const cfg_obj_t *config, } static isc_result_t -ta_fromconfig(const cfg_obj_t *key, bool *initialp, dst_key_t **keyp, - dns_rdata_ds_t **dsp, const char **namestrp, isc_mem_t *mctx) +ta_fromconfig(const cfg_obj_t *key, bool *initialp, const char **namestrp, + unsigned char *digest, dns_rdata_ds_t *ds) { + isc_result_t result; dns_rdata_dnskey_t keystruct; - dns_rdata_ds_t *ds = NULL; + dns_rdata_t rdata = DNS_RDATA_INIT; uint32_t rdata1, rdata2, rdata3; const char *datastr = NULL, *namestr = NULL; unsigned char data[4096]; @@ -715,8 +716,6 @@ ta_fromconfig(const cfg_obj_t *key, bool *initialp, dst_key_t **keyp, dns_fixedname_t fname; dns_name_t *name = NULL; isc_buffer_t namebuf; - isc_result_t result; - dst_key_t *dstkey = NULL; const char *atstr = NULL; enum { INIT_DNSKEY, @@ -726,9 +725,8 @@ ta_fromconfig(const cfg_obj_t *key, bool *initialp, dst_key_t **keyp, TRUSTED } anchortype; - REQUIRE(keyp != NULL && *keyp == NULL); - REQUIRE(dsp != NULL && *dsp == NULL); REQUIRE(namestrp != NULL && *namestrp == NULL); + REQUIRE(ds != NULL); /* if DNSKEY, flags; if DS, key tag */ rdata1 = cfg_obj_asuint32(cfg_tuple_get(key, "rdata1")); @@ -775,6 +773,13 @@ ta_fromconfig(const cfg_obj_t *key, bool *initialp, dst_key_t **keyp, isc_buffer_init(&databuf, data, sizeof(data)); isc_buffer_init(&rrdatabuf, rrdata, sizeof(rrdata)); + *ds = (dns_rdata_ds_t){ + .common.rdclass = dns_rdataclass_in, + .common.rdtype = dns_rdatatype_ds + }; + + ISC_LINK_INIT(&ds->common, link); + switch(anchortype) { case INIT_DNSKEY: case STATIC_DNSKEY: @@ -802,7 +807,7 @@ ta_fromconfig(const cfg_obj_t *key, bool *initialp, dst_key_t **keyp, if (rdata2 > 0xff) { CHECKM(ISC_R_RANGE, "key protocol"); } - if (rdata3> 0xff) { + if (rdata3 > 0xff) { CHECKM(ISC_R_RANGE, "key algorithm"); } @@ -810,30 +815,25 @@ ta_fromconfig(const cfg_obj_t *key, bool *initialp, dst_key_t **keyp, keystruct.protocol = (uint8_t)rdata2; keystruct.algorithm = (uint8_t)rdata3; + if (!dst_algorithm_supported(keystruct.algorithm)) { + CHECK(DST_R_UNSUPPORTEDALG); + } + datastr = cfg_obj_asstring(cfg_tuple_get(key, "data")); CHECK(isc_base64_decodestring(datastr, &databuf)); isc_buffer_usedregion(&databuf, &r); keystruct.datalen = r.length; keystruct.data = r.base; - CHECK(dns_rdata_fromstruct(NULL, keystruct.common.rdclass, + CHECK(dns_rdata_fromstruct(&rdata, keystruct.common.rdclass, keystruct.common.rdtype, &keystruct, &rrdatabuf)); - CHECK(dst_key_fromdns(name, dns_rdataclass_in, - &rrdatabuf, mctx, &dstkey)); - - *keyp = dstkey; + CHECK(dns_ds_fromkeyrdata(name, &rdata, DNS_DSDIGEST_SHA256, + digest, ds)); break; case INIT_DS: case STATIC_DS: - ds = isc_mem_get(mctx, sizeof(*ds)); - ds->common.rdclass = dns_rdataclass_in; - ds->common.rdtype = dns_rdatatype_ds; - ds->mctx = NULL; - - ISC_LINK_INIT(&ds->common, link); - if (rdata1 > 0xffff) { CHECKM(ISC_R_RANGE, "key tag"); } @@ -878,13 +878,10 @@ ta_fromconfig(const cfg_obj_t *key, bool *initialp, dst_key_t **keyp, break; } - ds->mctx = mctx; ds->length = r.length; - ds->digest = isc_mem_allocate(mctx, r.length); + ds->digest = digest; memmove(ds->digest, r.base, r.length); - *dsp = ds; - ds = NULL; break; default: @@ -895,15 +892,6 @@ ta_fromconfig(const cfg_obj_t *key, bool *initialp, dst_key_t **keyp, return (ISC_R_SUCCESS); cleanup: - if (dstkey != NULL) { - dst_key_free(&dstkey); - } - - if (ds != NULL) { - dns_rdata_freestruct(ds); - isc_mem_put(mctx, ds, sizeof(*ds)); - } - return (result); } @@ -921,46 +909,30 @@ ta_fromconfig(const cfg_obj_t *key, bool *initialp, dst_key_t **keyp, static isc_result_t process_key(const cfg_obj_t *key, dns_keytable_t *secroots, const dns_name_t *keyname_match, dns_resolver_t *resolver, - bool managed, isc_mem_t *mctx) + bool managed) { dns_fixedname_t fkeyname; dns_name_t *keyname = NULL; const char *namestr = NULL; - dst_key_t *dstkey = NULL; - dns_rdata_ds_t *ds = NULL; - unsigned int keyalg; + dns_rdata_ds_t ds; isc_result_t result; bool initializing = managed; + unsigned char digest[ISC_MAX_MD_SIZE]; + isc_buffer_t b; - result = ta_fromconfig(key, &initializing, &dstkey, &ds, - &namestr, mctx); + result = ta_fromconfig(key, &initializing, &namestr, digest, &ds); switch (result) { case ISC_R_SUCCESS: /* - * Trust anchor was parsed correctly. If dstkey is - * not NULL, then it was a key anchor, its algorithm - * is supported by the crypto library, and it is not - * revoked. If dstkey is NULL, then it was a DS - * trust anchor instead. + * Trust anchor was parsed correctly. */ - if (dstkey != NULL) { - keyname = dst_key_name(dstkey); - keyalg = dst_key_alg(dstkey); - } else { - isc_buffer_t b; - - INSIST(ds != NULL); - - isc_buffer_constinit(&b, namestr, strlen(namestr)); - isc_buffer_add(&b, strlen(namestr)); - keyname = dns_fixedname_initname(&fkeyname); - result = dns_name_fromtext(keyname, &b, - dns_rootname, 0, NULL); - if (result != ISC_R_SUCCESS) { - return (result); - } - keyalg = ds->algorithm; + isc_buffer_constinit(&b, namestr, strlen(namestr)); + isc_buffer_add(&b, strlen(namestr)); + keyname = dns_fixedname_initname(&fkeyname); + result = dns_name_fromtext(keyname, &b, dns_rootname, 0, NULL); + if (result != ISC_R_SUCCESS) { + return (result); } break; case DST_R_UNSUPPORTEDALG: @@ -1011,7 +983,8 @@ process_key(const cfg_obj_t *key, dns_keytable_t *secroots, * its owner name. If it does not, do not load the key and log a * warning, but do not prevent further keys from being processed. */ - if (!dns_resolver_algorithm_supported(resolver, keyname, keyalg)) { + if (!dns_resolver_algorithm_supported(resolver, keyname, ds.algorithm)) + { cfg_obj_log(key, named_g_lctx, ISC_LOG_WARNING, "ignoring %s for '%s': algorithm is disabled", initializing ? "initial-key" : "static-key", @@ -1026,29 +999,10 @@ process_key(const cfg_obj_t *key, dns_keytable_t *secroots, * managed, so we use 'initializing' twice here, for both the * 'managed' and 'initializing' arguments to dns_keytable_add(). */ - result = dns_keytable_add(secroots, initializing, - initializing, keyname, - dstkey != NULL ? &dstkey : NULL, - ds); + result = dns_keytable_add(secroots, initializing, initializing, + keyname, &ds); done: - /* - * Ensure 'dstkey' does not leak. Note that if dns_keytable_add() - * succeeds, ownership of the key structure is transferred to the key - * table, i.e. 'dstkey' is set to NULL. - */ - if (dstkey != NULL) { - dst_key_free(&dstkey); - } - - /* - * Free 'ds'. - */ - if (ds != NULL) { - dns_rdata_freestruct(ds); - isc_mem_put(mctx, ds, sizeof(*ds)); - } - return (result); } @@ -1059,7 +1013,7 @@ process_key(const cfg_obj_t *key, dns_keytable_t *secroots, */ static isc_result_t load_view_keys(const cfg_obj_t *keys, dns_view_t *view, bool managed, - const dns_name_t *keyname, isc_mem_t *mctx) + const dns_name_t *keyname) { const cfg_listelt_t *elt, *elt2; const cfg_obj_t *keylist; @@ -1078,9 +1032,8 @@ load_view_keys(const cfg_obj_t *keys, dns_view_t *view, bool managed, elt2 != NULL; elt2 = cfg_list_next(elt2)) { - CHECK(process_key(cfg_listelt_value(elt2), - secroots, keyname, view->resolver, - managed, mctx)); + CHECK(process_key(cfg_listelt_value(elt2), secroots, + keyname, view->resolver, managed)); } } @@ -1238,7 +1191,7 @@ configure_view_dnsseckeys(dns_view_t *view, const cfg_obj_t *vconfig, if (builtin_keys != NULL) { CHECK(load_view_keys(builtin_keys, view, true, - dns_rootname, mctx)); + dns_rootname)); } if (!keyloaded(view, dns_rootname)) { @@ -1251,17 +1204,13 @@ configure_view_dnsseckeys(dns_view_t *view, const cfg_obj_t *vconfig, } if (view->rdclass == dns_rdataclass_in) { - CHECK(load_view_keys(view_keys, view, false, NULL, mctx)); - CHECK(load_view_keys(view_trust_anchors, view, true, NULL, - mctx)); - CHECK(load_view_keys(view_managed_keys, view, true, NULL, - mctx)); + CHECK(load_view_keys(view_keys, view, false, NULL)); + CHECK(load_view_keys(view_trust_anchors, view, true, NULL)); + CHECK(load_view_keys(view_managed_keys, view, true, NULL)); - CHECK(load_view_keys(global_keys, view, false, NULL, mctx)); - CHECK(load_view_keys(global_trust_anchors, view, true, - NULL, mctx)); - CHECK(load_view_keys(global_managed_keys, view, true, - NULL, mctx)); + CHECK(load_view_keys(global_keys, view, false, NULL)); + CHECK(load_view_keys(global_trust_anchors, view, true, NULL)); + CHECK(load_view_keys(global_managed_keys, view, true, NULL)); } /* diff --git a/bin/tests/system/dnssec/tests.sh b/bin/tests/system/dnssec/tests.sh index e959412961..0d14b737f7 100644 --- a/bin/tests/system/dnssec/tests.sh +++ b/bin/tests/system/dnssec/tests.sh @@ -3700,12 +3700,12 @@ status=$((status+ret)) # DNSSEC tests related to unsupported, disabled and revoked trust anchors. # -# This nameserver (ns8) is loaded with a bunch of trust anchors. Some of them -# are good (enabled.managed, enabled.trusted, secure.managed, secure.trusted), -# and some of them are bad (disabled.managed, revoked.managed, unsupported.managed, -# disabled.trusted, revoked.trusted, unsupported.trusted). Make sure that the bad -# trust anchors are ignored. This is tested by looking for the corresponding -# lines in the logfile. +# This nameserver (ns8) is loaded with a bunch of trust anchors. Some of +# them are good (enabled.managed, enabled.trusted, secure.managed, +# secure.trusted), and some of them are bad (disabled.managed, +# revoked.managed, unsupported.managed, disabled.trusted, revoked.trusted, +# unsupported.trusted). Make sure that the bad trust anchors are ignored. +# This is tested by looking for the corresponding lines in the logfile. echo_i "checking that keys with unsupported algorithms and disabled algorithms are ignored ($n)" ret=0 grep -q "ignoring static-key for 'disabled\.trusted\.': algorithm is disabled" ns8/named.run || ret=1 diff --git a/lib/dns/client.c b/lib/dns/client.c index 4ffbd6ff29..54e14ce111 100644 --- a/lib/dns/client.c +++ b/lib/dns/client.c @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -1498,6 +1499,7 @@ dns_client_addtrustedkey(dns_client_t *client, dns_rdataclass_t rdclass, dns_keytable_t *secroots = NULL; dns_name_t *name = NULL; char dsbuf[DNS_DS_BUFFERSIZE]; + unsigned char digest[ISC_MAX_MD_SIZE]; dns_rdata_ds_t ds; dns_decompress_t dctx; dns_rdata_t rdata; @@ -1515,33 +1517,28 @@ dns_client_addtrustedkey(dns_client_t *client, dns_rdataclass_t rdclass, DE_CONST(keyname, name); - switch (rdtype) { - case dns_rdatatype_dnskey: - result = dst_key_fromdns(keyname, rdclass, databuf, - client->mctx, &dstkey); - if (result != ISC_R_SUCCESS) { - goto cleanup; - } - CHECK(dns_keytable_add(secroots, false, false, - name, &dstkey, NULL)); - break; - case dns_rdatatype_ds: - isc_buffer_init(&b, dsbuf, sizeof(dsbuf)); - dns_decompress_init(&dctx, -1, DNS_DECOMPRESS_NONE); - dns_rdata_init(&rdata); - isc_buffer_setactive(databuf, isc_buffer_usedlength(databuf)); - CHECK(dns_rdata_fromwire(&rdata, rdclass, rdtype, - databuf, &dctx, 0, &b)); - dns_decompress_invalidate(&dctx); - CHECK(dns_rdata_tostruct(&rdata, &ds, NULL)); - CHECK(dns_keytable_add(secroots, false, false, - name, NULL, &ds)); - break; - - default: + if (rdtype != dns_rdatatype_dnskey && rdtype != dns_rdatatype_ds) { result = ISC_R_NOTIMPLEMENTED; + goto cleanup; } + isc_buffer_init(&b, dsbuf, sizeof(dsbuf)); + dns_decompress_init(&dctx, -1, DNS_DECOMPRESS_NONE); + dns_rdata_init(&rdata); + isc_buffer_setactive(databuf, isc_buffer_usedlength(databuf)); + CHECK(dns_rdata_fromwire(&rdata, rdclass, rdtype, + databuf, &dctx, 0, &b)); + dns_decompress_invalidate(&dctx); + + if (rdtype == dns_rdatatype_ds) { + CHECK(dns_rdata_tostruct(&rdata, &ds, NULL)); + } else { + CHECK(dns_ds_fromkeyrdata(name, &rdata, DNS_DSDIGEST_SHA256, + digest, &ds)); + } + + CHECK(dns_keytable_add(secroots, false, false, name, &ds)); + cleanup: if (dstkey != NULL) { dst_key_free(&dstkey); diff --git a/lib/dns/ds.c b/lib/dns/ds.c index 8958fb89cf..7a9c911544 100644 --- a/lib/dns/ds.c +++ b/lib/dns/ds.c @@ -29,20 +29,17 @@ #include isc_result_t -dns_ds_buildrdata(dns_name_t *owner, dns_rdata_t *key, - dns_dsdigest_t digest_type, unsigned char *buffer, - dns_rdata_t *rdata) +dns_ds_fromkeyrdata(dns_name_t *owner, dns_rdata_t *key, + dns_dsdigest_t digest_type, unsigned char *digest, + dns_rdata_ds_t *dsrdata) { + isc_result_t result; dns_fixedname_t fname; dns_name_t *name; - unsigned char digest[ISC_MAX_MD_SIZE]; unsigned int digestlen; isc_region_t r; - isc_buffer_t b; - dns_rdata_ds_t ds; isc_md_t *md; isc_md_type_t md_type = 0; - isc_result_t ret; REQUIRE(key != NULL); REQUIRE(key->type == dns_rdatatype_dnskey || @@ -73,51 +70,68 @@ dns_ds_buildrdata(dns_name_t *owner, dns_rdata_t *key, name = dns_fixedname_initname(&fname); (void)dns_name_downcase(owner, name, NULL); - memset(buffer, 0, DNS_DS_BUFFERSIZE); - isc_buffer_init(&b, buffer, DNS_DS_BUFFERSIZE); - md = isc_md_new(); if (md == NULL) { return (ISC_R_NOMEMORY); } - ret = isc_md_init(md, md_type); - if (ret != ISC_R_SUCCESS) { + result = isc_md_init(md, md_type); + if (result != ISC_R_SUCCESS) { goto end; } dns_name_toregion(name, &r); - ret = isc_md_update(md, r.base, r.length); - if (ret != ISC_R_SUCCESS) { + result = isc_md_update(md, r.base, r.length); + if (result != ISC_R_SUCCESS) { goto end; } dns_rdata_toregion(key, &r); INSIST(r.length >= 4); - ret = isc_md_update(md, r.base, r.length); - if (ret != ISC_R_SUCCESS) { + result = isc_md_update(md, r.base, r.length); + if (result != ISC_R_SUCCESS) { goto end; } - ret = isc_md_final(md, digest, &digestlen); - if (ret != ISC_R_SUCCESS) { + result = isc_md_final(md, digest, &digestlen); + if (result != ISC_R_SUCCESS) { goto end; } - ds.mctx = NULL; - ds.common.rdclass = key->rdclass; - ds.common.rdtype = dns_rdatatype_ds; - ds.algorithm = r.base[3]; - ds.key_tag = dst_region_computeid(&r); - ds.digest_type = digest_type; - ds.digest = digest; - ds.length = digestlen; + dsrdata->mctx = NULL; + dsrdata->common.rdclass = key->rdclass; + dsrdata->common.rdtype = dns_rdatatype_ds; + dsrdata->algorithm = r.base[3]; + dsrdata->key_tag = dst_region_computeid(&r); + dsrdata->digest_type = digest_type; + dsrdata->digest = digest; + dsrdata->length = digestlen; - ret = dns_rdata_fromstruct(rdata, key->rdclass, dns_rdatatype_ds, - &ds, &b); end: isc_md_free(md); - return (ret); + return (result); +} + +isc_result_t +dns_ds_buildrdata(dns_name_t *owner, dns_rdata_t *key, + dns_dsdigest_t digest_type, unsigned char *buffer, + dns_rdata_t *rdata) +{ + isc_result_t result; + unsigned char digest[ISC_MAX_MD_SIZE]; + dns_rdata_ds_t ds; + isc_buffer_t b; + + result = dns_ds_fromkeyrdata(owner, key, digest_type, digest, &ds); + if (result != ISC_R_SUCCESS) { + return (result); + } + + memset(buffer, 0, DNS_DS_BUFFERSIZE); + isc_buffer_init(&b, buffer, DNS_DS_BUFFERSIZE); + result = dns_rdata_fromstruct(rdata, key->rdclass, dns_rdatatype_ds, + &ds, &b); + return (result); } diff --git a/lib/dns/include/dns/ds.h b/lib/dns/include/dns/ds.h index a1df4a5d57..ae610dae82 100644 --- a/lib/dns/include/dns/ds.h +++ b/lib/dns/include/dns/ds.h @@ -15,6 +15,7 @@ #include +#include #include #define DNS_DSDIGEST_SHA1 (1) @@ -29,16 +30,30 @@ ISC_LANG_BEGINDECLS +isc_result_t +dns_ds_fromkeyrdata(dns_name_t *owner, dns_rdata_t *key, + dns_dsdigest_t digest_type, unsigned char *digest, + dns_rdata_ds_t *dsrdata); +/*%< + * Build a DS rdata structure from a key. + * + * Requires: + *\li key Points to a valid DNSKEY or CDNSKEY record. + *\li buffer Points to a buffer of at least + * #ISC_MAX_MD_SIZE bytes. + */ + isc_result_t dns_ds_buildrdata(dns_name_t *owner, dns_rdata_t *key, dns_dsdigest_t digest_type, unsigned char *buffer, dns_rdata_t *rdata); /*%< - * Build the rdata of a DS record. + * Similar to dns_ds_fromkeyrdata(), but copies the DS into a + * dns_rdata object. * * Requires: *\li key Points to a valid DNSKEY or CDNSKEY record. - *\li buffer Points to a temporary buffer of at least + *\li buffer Points to a buffer of at least * #DNS_DS_BUFFERSIZE bytes. *\li rdata Points to an initialized dns_rdata_t. * diff --git a/lib/dns/include/dns/keytable.h b/lib/dns/include/dns/keytable.h index 4edba81a5b..bf4c414d72 100644 --- a/lib/dns/include/dns/keytable.h +++ b/lib/dns/include/dns/keytable.h @@ -106,13 +106,11 @@ dns_keytable_detach(dns_keytable_t **keytablep); */ isc_result_t -dns_keytable_add(dns_keytable_t *keytable, - bool managed, bool initial, - dns_name_t *name, dst_key_t **keyp, dns_rdata_ds_t *ds); +dns_keytable_add(dns_keytable_t *keytable, bool managed, bool initial, + dns_name_t *name, dns_rdata_ds_t *ds); /*%< * Add a key to 'keytable'. The keynode associated with 'name' - * is updated with either the key referenced in '*keyp' - * or with the DS specified in 'ds'. + * is updated with the DS specified in 'ds'. * * The value of keynode->managed is set to 'managed', and the * value of keynode->initial is set to 'initial'. (Note: 'initial' @@ -123,28 +121,15 @@ dns_keytable_add(dns_keytable_t *keytable, * * Notes: * - *\li Ownership of *keyp is transferred to the keytable. - *\li If 'keyp' is not NULL and DS-style keys already exist - * in the table for this name, they are freed before adding - * the new key. - *\li If 'ds' is not NULL and key-style keys already exist - * in the table for this name, return ISC_R_EXISTS. DS keys - * can be updated to key-style, but not vice versa. - *\li If the key already exists in the table, ISC_R_EXISTS is - * returned and the new key is freed. + *\li If the key already exists in the table, adding it again + * has no effect and ISC_R_SUCCESS is returned. * * Requires: * *\li 'keytable' points to a valid keytable. - * + *\li 'ds' is not NULL. *\li if 'initial' is true then 'managed' must also be true. * - *\li keyp != NULL && *keyp is a valid dst_key_t *. - * - * Ensures: - * - *\li On success, *keyp == NULL - * * Returns: * *\li ISC_R_SUCCESS diff --git a/lib/dns/keytable.c b/lib/dns/keytable.c index 2a65c7caab..91afb052dd 100644 --- a/lib/dns/keytable.c +++ b/lib/dns/keytable.c @@ -364,14 +364,12 @@ insert(dns_keytable_t *keytable, bool managed, bool initial, } isc_result_t -dns_keytable_add(dns_keytable_t *keytable, - bool managed, bool initial, - dns_name_t *name, dst_key_t **keyp, dns_rdata_ds_t *ds) +dns_keytable_add(dns_keytable_t *keytable, bool managed, bool initial, + dns_name_t *name, dns_rdata_ds_t *ds) { - REQUIRE(keyp == NULL || *keyp != NULL); - REQUIRE(keyp != NULL || ds != NULL); + REQUIRE(ds != NULL); REQUIRE(!initial || managed); - return (insert(keytable, managed, initial, name, keyp, ds)); + return (insert(keytable, managed, initial, name, NULL, ds)); } isc_result_t diff --git a/lib/dns/tests/keytable_test.c b/lib/dns/tests/keytable_test.c index ac3066eafd..3f5d2eb210 100644 --- a/lib/dns/tests/keytable_test.c +++ b/lib/dns/tests/keytable_test.c @@ -27,11 +27,12 @@ #include #include +#include #include -#include #include #include +#include #include #include #include @@ -129,44 +130,48 @@ create_keystruct(uint16_t flags, uint8_t proto, uint8_t alg, } static void -create_key(uint16_t flags, uint8_t proto, uint8_t alg, - const char *keynamestr, const char *keystr, dst_key_t **target) +create_dsstruct(dns_name_t *name, uint16_t flags, + uint8_t proto, uint8_t alg, const char *keystr, + unsigned char *digest, dns_rdata_ds_t *dsstruct) { isc_result_t result; - dns_rdata_dnskey_t keystruct; unsigned char rrdata[4096]; isc_buffer_t rrdatabuf; - const dns_rdataclass_t rdclass = dns_rdataclass_in; + dns_rdata_t rdata = DNS_RDATA_INIT; + dns_rdata_dnskey_t dnskey; /* * Populate DNSKEY rdata structure. */ - create_keystruct(flags, proto, alg, keystr, &keystruct); + create_keystruct(flags, proto, alg, keystr, &dnskey); /* * Convert to wire format. */ isc_buffer_init(&rrdatabuf, rrdata, sizeof(rrdata)); - result = dns_rdata_fromstruct(NULL, keystruct.common.rdclass, - keystruct.common.rdtype, - &keystruct, &rrdatabuf), + result = dns_rdata_fromstruct(&rdata, dnskey.common.rdclass, + dnskey.common.rdtype, + &dnskey, &rrdatabuf); assert_int_equal(result, ISC_R_SUCCESS); /* - * Convert wire format to DST key. + * Build DS rdata struct. */ - result = dst_key_fromdns(str2name(keynamestr), rdclass, - &rrdatabuf, dt_mctx, target), + result = dns_ds_fromkeyrdata(name, &rdata, DNS_DSDIGEST_SHA256, + digest, dsstruct); assert_int_equal(result, ISC_R_SUCCESS); - dns_rdata_freestruct(&keystruct); + dns_rdata_freestruct(&dnskey); } /* Common setup: create a keytable and ntatable to test with a few keys */ static void create_tables() { isc_result_t result; - dst_key_t *key = NULL; + unsigned char digest[ISC_MAX_MD_SIZE]; + dns_rdata_ds_t ds; + dns_fixedname_t fn; + dns_name_t *keyname = dns_fixedname_name(&fn); isc_stdtime_t now; result = dns_test_makeview("view", &view); @@ -178,15 +183,15 @@ create_tables() { &ntatable), ISC_R_SUCCESS); /* Add a normal key */ - create_key(257, 3, 5, "example.com", keystr1, &key); - assert_int_equal(dns_keytable_add(keytable, false, false, - dst_key_name(key), &key, NULL), + dns_test_namefromstring("example.com", &fn); + create_dsstruct(keyname, 257, 3, 5, keystr1, digest, &ds); + assert_int_equal(dns_keytable_add(keytable, false, false, keyname, &ds), ISC_R_SUCCESS); /* Add an initializing managed key */ - create_key(257, 3, 5, "managed.com", keystr1, &key); - assert_int_equal(dns_keytable_add(keytable, true, true, - dst_key_name(key), &key, NULL), + 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), ISC_R_SUCCESS); /* Add a null key */ @@ -217,9 +222,12 @@ destroy_tables() { /* add keys to the keytable */ static void add_test(void **state) { - dst_key_t *key = NULL; dns_keynode_t *keynode = NULL; dns_keynode_t *null_keynode = NULL; + unsigned char digest[ISC_MAX_MD_SIZE]; + dns_rdata_ds_t ds; + dns_fixedname_t fn; + dns_name_t *keyname = dns_fixedname_name(&fn); UNUSED(state); @@ -236,9 +244,9 @@ add_test(void **state) { * Try to add the same key. This should have no effect but * report success. */ - create_key(257, 3, 5, "example.com", keystr1, &key); - assert_int_equal(dns_keytable_add(keytable, false, false, - dst_key_name(key), &key, NULL), + dns_test_namefromstring("example.com", &fn); + create_dsstruct(keyname, 257, 3, 5, keystr1, digest, &ds); + assert_int_equal(dns_keytable_add(keytable, false, false, keyname, &ds), ISC_R_SUCCESS); dns_keytable_detachkeynode(keytable, &keynode); assert_int_equal(dns_keytable_find(keytable, str2name("example.com"), @@ -247,9 +255,8 @@ add_test(void **state) { /* Add another key (different keydata) */ dns_keytable_detachkeynode(keytable, &keynode); - create_key(257, 3, 5, "example.com", keystr2, &key); - assert_int_equal(dns_keytable_add(keytable, false, false, - dst_key_name(key), &key, NULL), + create_dsstruct(keyname, 257, 3, 5, keystr2, digest, &ds); + assert_int_equal(dns_keytable_add(keytable, false, false, keyname, &ds), ISC_R_SUCCESS); assert_int_equal(dns_keytable_find(keytable, str2name("example.com"), &keynode), @@ -273,9 +280,9 @@ add_test(void **state) { * Add a different managed key for managed.com, marking it as an * initializing key. */ - create_key(257, 3, 5, "managed.com", keystr2, &key); - assert_int_equal(dns_keytable_add(keytable, true, true, - dst_key_name(key), &key, NULL), + dns_test_namefromstring("managed.com", &fn); + create_dsstruct(keyname, 257, 3, 5, keystr2, digest, &ds); + assert_int_equal(dns_keytable_add(keytable, true, true, keyname, &ds), ISC_R_SUCCESS); assert_int_equal(dns_keytable_find(keytable, str2name("managed.com"), &keynode), @@ -289,9 +296,7 @@ add_test(void **state) { * to a non-initializing key and make sure there are still two key * nodes for managed.com, both containing non-initializing keys. */ - create_key(257, 3, 5, "managed.com", keystr2, &key); - assert_int_equal(dns_keytable_add(keytable, true, false, - dst_key_name(key), &key, NULL), + assert_int_equal(dns_keytable_add(keytable, true, false, keyname, &ds), ISC_R_SUCCESS); assert_int_equal(dns_keytable_find(keytable, str2name("managed.com"), &keynode), @@ -303,9 +308,9 @@ add_test(void **state) { * Add a managed key at a new node, two.com, marking it as an * initializing key. */ - create_key(257, 3, 5, "two.com", keystr1, &key); - assert_int_equal(dns_keytable_add(keytable, true, true, - dst_key_name(key), &key, NULL), + dns_test_namefromstring("two.com", &fn); + create_dsstruct(keyname, 257, 3, 5, keystr1, digest, &ds); + assert_int_equal(dns_keytable_add(keytable, true, true, keyname, &ds), ISC_R_SUCCESS); assert_int_equal(dns_keytable_find(keytable, str2name("two.com"), &keynode), @@ -317,9 +322,8 @@ add_test(void **state) { * Add a different managed key for two.com, marking it as a * non-initializing key. */ - create_key(257, 3, 5, "two.com", keystr2, &key); - assert_int_equal(dns_keytable_add(keytable, true, false, - dst_key_name(key), &key, NULL), + create_dsstruct(keyname, 257, 3, 5, keystr2, 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), @@ -333,9 +337,8 @@ add_test(void **state) { * to a non-initializing key and make sure there are still two key * nodes for two.com, both containing non-initializing keys. */ - create_key(257, 3, 5, "two.com", keystr1, &key); - assert_int_equal(dns_keytable_add(keytable, true, false, - dst_key_name(key), &key, NULL), + 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), @@ -350,9 +353,9 @@ add_test(void **state) { assert_int_equal(dns_keytable_find(keytable, str2name("null.example"), &null_keynode), ISC_R_SUCCESS); - create_key(257, 3, 5, "null.example", keystr2, &key); - assert_int_equal(dns_keytable_add(keytable, false, false, - dst_key_name(key), &key, NULL), + dns_test_namefromstring("null.example", &fn); + create_dsstruct(keyname, 257, 3, 5, keystr2, digest, &ds); + assert_int_equal(dns_keytable_add(keytable, false, false, keyname, &ds), ISC_R_SUCCESS); assert_int_equal(dns_keytable_find(keytable, str2name("null.example"), &keynode), @@ -595,8 +598,11 @@ dump_test(void **state) { static void nta_test(void **state) { isc_result_t result; - dst_key_t *key = NULL; bool issecure, covered; + dns_fixedname_t fn; + dns_name_t *keyname = dns_fixedname_name(&fn); + unsigned char digest[ISC_MAX_MD_SIZE]; + dns_rdata_ds_t ds; dns_view_t *myview = NULL; isc_stdtime_t now; @@ -618,14 +624,13 @@ nta_test(void **state) { result = dns_view_getntatable(myview, &ntatable); assert_int_equal(result, ISC_R_SUCCESS); - create_key(257, 3, 5, "example", keystr1, &key); - result = dns_keytable_add(keytable, false, false, - dst_key_name(key), &key, NULL), + dns_test_namefromstring("example", &fn); + create_dsstruct(keyname, 257, 3, 5, keystr1, digest, &ds); + result = dns_keytable_add(keytable, false, false, keyname, &ds), assert_int_equal(result, ISC_R_SUCCESS); isc_stdtime_get(&now); - result = dns_ntatable_add(ntatable, - str2name("insecure.example"), + result = dns_ntatable_add(ntatable, str2name("insecure.example"), false, now, 1); assert_int_equal(result, ISC_R_SUCCESS); diff --git a/lib/dns/win32/libdns.def.in b/lib/dns/win32/libdns.def.in index 42e8ab222a..3de3985573 100644 --- a/lib/dns/win32/libdns.def.in +++ b/lib/dns/win32/libdns.def.in @@ -336,6 +336,7 @@ dns_dnssecsignstats_create dns_dnssecsignstats_dump dns_dnssecsignstats_increment dns_ds_buildrdata +dns_ds_fromkeyrdata dns_dsdigest_format dns_dsdigest_fromtext dns_dsdigest_totext diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 80e3085a45..f7de8e7fdf 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -3942,35 +3943,34 @@ compute_tag(dns_name_t *name, dns_rdata_dnskey_t *dnskey, isc_mem_t *mctx, */ static void trust_key(dns_zone_t *zone, dns_name_t *keyname, - dns_rdata_dnskey_t *dnskey, bool initial, - isc_mem_t *mctx) + dns_rdata_dnskey_t *dnskey, bool initial) { isc_result_t result; dns_rdata_t rdata = DNS_RDATA_INIT; - unsigned char data[4096]; + unsigned char data[4096], digest[ISC_MAX_MD_SIZE]; isc_buffer_t buffer; dns_keytable_t *sr = NULL; - dst_key_t *dstkey = NULL; + dns_rdata_ds_t ds; - /* Convert dnskey to DST key. */ + result = dns_view_getsecroots(zone->view, &sr); + if (result != ISC_R_SUCCESS) { + return; + } + + /* Build DS record for key. */ isc_buffer_init(&buffer, data, sizeof(data)); dns_rdata_fromstruct(&rdata, dnskey->common.rdclass, dns_rdatatype_dnskey, dnskey, &buffer); + CHECK(dns_ds_fromkeyrdata(keyname, &rdata, DNS_DSDIGEST_SHA256, + digest, &ds)); + CHECK(dns_keytable_add(sr, true, initial, keyname, &ds)); - result = dns_view_getsecroots(zone->view, &sr); - if (result != ISC_R_SUCCESS) - goto failure; - - CHECK(dns_dnssec_keyfromrdata(keyname, &rdata, mctx, &dstkey)); - CHECK(dns_keytable_add(sr, true, initial, - dst_key_name(dstkey), &dstkey, NULL)); dns_keytable_detach(&sr); failure: - if (dstkey != NULL) - dst_key_free(&dstkey); - if (sr != NULL) + if (sr != NULL) { dns_keytable_detach(&sr); + } return; } @@ -4000,7 +4000,6 @@ load_secroots(dns_zone_t *zone, dns_name_t *name, dns_rdataset_t *rdataset) { dns_rdata_t rdata = DNS_RDATA_INIT; dns_rdata_keydata_t keydata; dns_rdata_dnskey_t dnskey; - isc_mem_t *mctx = zone->mctx; int trusted = 0, revoked = 0, pending = 0; isc_stdtime_t now; dns_keytable_t *sr = NULL; @@ -4051,7 +4050,7 @@ load_secroots(dns_zone_t *zone, dns_name_t *name, dns_rdataset_t *rdataset) { /* Add to keytables. */ trusted++; - trust_key(zone, name, &dnskey, (keydata.addhd == 0), mctx); + trust_key(zone, name, &dnskey, (keydata.addhd == 0)); } if (trusted == 0 && pending != 0) { @@ -10271,7 +10270,7 @@ keyfetch_done(isc_task_t *task, isc_event_t *event) { /* Trust this key. */ result = dns_rdata_tostruct(&dnskeyrr, &dnskey, NULL); RUNTIME_CHECK(result == ISC_R_SUCCESS); - trust_key(zone, keyname, &dnskey, false, mctx); + trust_key(zone, keyname, &dnskey, false); } if (secure && !deletekey) { From 678e2d3cfad95fe2d4d627e01225f64c852a5bb0 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Fri, 20 Dec 2019 16:23:25 -0800 Subject: [PATCH 4/6] 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 | 6 - lib/dns/keytable.c | 238 +++++++++++---------------------- lib/dns/tests/keytable_test.c | 28 ++-- lib/dns/win32/libdns.def.in | 1 - 4 files changed, 84 insertions(+), 189 deletions(-) diff --git a/lib/dns/include/dns/keytable.h b/lib/dns/include/dns/keytable.h index bf4c414d72..321fef9543 100644 --- a/lib/dns/include/dns/keytable.h +++ b/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); /*%< diff --git a/lib/dns/keytable.c b/lib/dns/keytable.c index 91afb052dd..76b098cbf7 100644 --- a/lib/dns/keytable.c +++ b/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, - dst_key_t **keyp, bool initial) -{ - dns_keynode_t *knode; +add_ds(dns_keynode_t *knode, dns_rdata_ds_t *ds, isc_mem_t *mctx) { + isc_result_t result; + dns_rdata_t *rdata = NULL; + void *data = NULL; + isc_buffer_t b; - REQUIRE(keyp != NULL && *keyp != NULL); - REQUIRE(node != NULL); - - for (knode = node->data; knode != NULL; knode = knode->next) { - if (knode->key == NULL) { - /* - * 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); + if (knode->dslist == NULL) { + knode->dslist = isc_mem_get(mctx, sizeof(*knode->dslist)); + dns_rdatalist_init(knode->dslist); + knode->dslist->rdclass = dns_rdataclass_in; + knode->dslist->type = dns_rdatatype_ds; } - return (ISC_R_NOTFOUND); -} + rdata = isc_mem_get(mctx, sizeof(*rdata)); + dns_rdata_init(rdata); -/*% - * Create a key node for "keyp" (or a null key node if "keyp" is NULL), set - * "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; + data = isc_mem_get(mctx, DNS_DS_BUFFERSIZE); + isc_buffer_init(&b, data, DNS_DS_BUFFERSIZE); - REQUIRE(keyp == NULL || *keyp != NULL); - REQUIRE(VALID_KEYTABLE(keytable)); - REQUIRE(!initial || managed); - - result = dns_keynode_create(keytable->mctx, &knode); + 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; + 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. - * Otherwise, if a DS was supplied, append it to the rdatalist - * (initializing if necessary). + * If a DS was supplied, initialize an rdatalist. */ - if (keyp != NULL) { - if (knode->dslist != NULL) { - 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 (ds != NULL) { + result = add_ds(knode, ds, keytable->mctx); 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 - * requested name, update its flags. If "keyp" is NULL, add a null key to - * indicate that "keyname" should be treated as a secure domain without - * supplying key data which would allow the domain to be validated. + * Add key trust anchor "ds" at "keyname" in "keytable". If an anchor + * already exists at the requested name does not contain "ds", update it. + * If "ds" is NULL, add a null key to indicate that "keyname" should be + * 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 - * "ds" are NULL) and attach it to the created node. + * trust anchor (or a null key node if both "ds" is NULL) + * and attach it to the created node. */ - result = prepend_keynode(keyp, ds, node, keytable, - managed, initial); + result = new_keynode(ds, node, keytable, 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) { - /* - * We were told to add the key supplied in "keyp" at - * "keyname". Try to find an already existing key node - * we could reuse for the supplied key (i.e. a null key - * node or a key node for the exact same key) and, if - * found, update it accordingly. - */ - 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 { + dns_keynode_t *knode = node->data; + if (knode == NULL) { + result = new_keynode(ds, node, keytable, + managed, initial); + } else { + result = add_ds(knode, ds, keytable->mctx); } - } 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)); diff --git a/lib/dns/tests/keytable_test.c b/lib/dns/tests/keytable_test.c index 3f5d2eb210..e1ab31eaea 100644 --- a/lib/dns/tests/keytable_test.c +++ b/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); - 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); + assert_int_equal(dns_keynode_initial(keynode), true); dns_keytable_detachkeynode(keytable, &keynode); /* diff --git a/lib/dns/win32/libdns.def.in b/lib/dns/win32/libdns.def.in index 3de3985573..d41d0b4a91 100644 --- a/lib/dns/win32/libdns.def.in +++ b/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 From 2d249ebeae319ee829e2f92d689ee979a43a9eea Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Mon, 23 Dec 2019 20:26:03 -0800 Subject: [PATCH 5/6] make dns_keytable_deletekey() work correctly it now removes matching trust anchors from from the dslist while leaving the other trust anchors in place. also cleaned up the API to remove functions that were never being used. --- lib/dns/ds.c | 2 +- lib/dns/include/dns/ds.h | 2 +- lib/dns/include/dns/keytable.h | 44 +---- lib/dns/keytable.c | 310 +++++++++++++++------------------ lib/dns/tests/keytable_test.c | 39 ++--- lib/dns/win32/libdns.def.in | 4 - 6 files changed, 162 insertions(+), 239 deletions(-) diff --git a/lib/dns/ds.c b/lib/dns/ds.c index 7a9c911544..b5e3bb69aa 100644 --- a/lib/dns/ds.c +++ b/lib/dns/ds.c @@ -29,7 +29,7 @@ #include isc_result_t -dns_ds_fromkeyrdata(dns_name_t *owner, dns_rdata_t *key, +dns_ds_fromkeyrdata(const dns_name_t *owner, dns_rdata_t *key, dns_dsdigest_t digest_type, unsigned char *digest, dns_rdata_ds_t *dsrdata) { diff --git a/lib/dns/include/dns/ds.h b/lib/dns/include/dns/ds.h index ae610dae82..9166fde584 100644 --- a/lib/dns/include/dns/ds.h +++ b/lib/dns/include/dns/ds.h @@ -31,7 +31,7 @@ ISC_LANG_BEGINDECLS isc_result_t -dns_ds_fromkeyrdata(dns_name_t *owner, dns_rdata_t *key, +dns_ds_fromkeyrdata(const dns_name_t *owner, dns_rdata_t *key, dns_dsdigest_t digest_type, unsigned char *digest, dns_rdata_ds_t *dsrdata); /*%< diff --git a/lib/dns/include/dns/keytable.h b/lib/dns/include/dns/keytable.h index 321fef9543..5ff7286475 100644 --- a/lib/dns/include/dns/keytable.h +++ b/lib/dns/include/dns/keytable.h @@ -250,33 +250,13 @@ dns_keytable_finddeepestmatch(dns_keytable_t *keytable, const dns_name_t *name, */ void -dns_keytable_attachkeynode(dns_keytable_t *keytable, dns_keynode_t *source, - dns_keynode_t **target); +dns_keytable_detachkeynode(dns_keytable_t *keytable, dns_keynode_t **keynodep); /*%< - * Attach a keynode and and increment the active_nodes counter in a - * corresponding keytable. + * Detach a keynode found via dns_keytable_find(). * * Requires: * - *\li 'keytable' is a valid keytable. - * - *\li 'source' is a valid keynode. - * - *\li 'target' is not null and '*target' is null. - */ - -void -dns_keytable_detachkeynode(dns_keytable_t *keytable, - dns_keynode_t **keynodep); -/*%< - * Give back a keynode found via dns_keytable_findkeynode(). - * - * Requires: - * - *\li 'keytable' is a valid keytable. - * - *\li *keynodep is a valid keynode returned by a call to - * dns_keytable_findkeynode(). + *\li *keynodep is a valid keynode returned by a call to dns_keytable_find(). * * Ensures: * @@ -357,24 +337,6 @@ dns_keynode_trust(dns_keynode_t *keynode); * trusted: no longer an initializing key. */ -void -dns_keynode_attach(dns_keynode_t *source, dns_keynode_t **target); -/*%< - * Attach keynode 'source' to '*target' - */ - -void -dns_keynode_detach(isc_mem_t *mctx, dns_keynode_t **target); -/*%< - * Detach a keynode. - */ - -void -dns_keynode_detachall(isc_mem_t *mctx, dns_keynode_t **target); -/*%< - * Detach a keynode and all its succesors. - */ - isc_result_t dns_keytable_forall(dns_keytable_t *keytable, void (*func)(dns_keytable_t *, dns_keynode_t *, diff --git a/lib/dns/keytable.c b/lib/dns/keytable.c index 76b098cbf7..d6456439dd 100644 --- a/lib/dns/keytable.c +++ b/lib/dns/keytable.c @@ -41,7 +41,6 @@ struct dns_keytable { /* Unlocked. */ unsigned int magic; isc_mem_t *mctx; - atomic_uint_fast32_t active_nodes; isc_refcount_t references; isc_rwlock_t rwlock; /* Locked by rwlock. */ @@ -51,20 +50,58 @@ struct dns_keytable { struct dns_keynode { unsigned int magic; isc_refcount_t refcount; - dst_key_t *key; dns_rdatalist_t *dslist; dns_rdataset_t dsset; bool managed; bool initial; - struct dns_keynode *next; }; +static void +keynode_attach(dns_keynode_t *source, dns_keynode_t **target) { + REQUIRE(VALID_KEYNODE(source)); + isc_refcount_increment(&source->refcount); + *target = source; +} + +static void +keynode_detach(isc_mem_t *mctx, dns_keynode_t **keynodep) { + REQUIRE(keynodep != NULL && VALID_KEYNODE(*keynodep)); + dns_keynode_t *knode = *keynodep; + *keynodep = NULL; + + if (isc_refcount_decrement(&knode->refcount) == 1) { + dns_rdata_t *rdata = NULL; + isc_refcount_destroy(&knode->refcount); + if (knode->dslist != NULL) { + if (dns_rdataset_isassociated(&knode->dsset)) { + dns_rdataset_disassociate(&knode->dsset); + } + + for (rdata = ISC_LIST_HEAD(knode->dslist->rdata); + rdata != NULL; + rdata = ISC_LIST_HEAD(knode->dslist->rdata)) + { + ISC_LIST_UNLINK(knode->dslist->rdata, + rdata, link); + isc_mem_put(mctx, rdata->data, + DNS_DS_BUFFERSIZE); + isc_mem_put(mctx, rdata, sizeof(*rdata)); + } + + isc_mem_put(mctx, knode->dslist, + sizeof(*knode->dslist)); + knode->dslist = NULL; + } + isc_mem_put(mctx, knode, sizeof(dns_keynode_t)); + } +} + static void free_keynode(void *node, void *arg) { dns_keynode_t *keynode = node; isc_mem_t *mctx = arg; - dns_keynode_detachall(mctx, &keynode); + keynode_detach(mctx, &keynode); } isc_result_t @@ -91,7 +128,6 @@ dns_keytable_create(isc_mem_t *mctx, dns_keytable_t **keytablep) { goto cleanup_rbt; } - atomic_init(&keytable->active_nodes, 0); isc_refcount_init(&keytable->references, 1); keytable->mctx = NULL; @@ -112,11 +148,6 @@ dns_keytable_create(isc_mem_t *mctx, dns_keytable_t **keytablep) { void dns_keytable_attach(dns_keytable_t *source, dns_keytable_t **targetp) { - - /* - * Attach *targetp to source. - */ - REQUIRE(VALID_KEYTABLE(source)); REQUIRE(targetp != NULL && *targetp == NULL); @@ -133,7 +164,6 @@ dns_keytable_detach(dns_keytable_t **keytablep) { if (isc_refcount_decrement(&keytable->references) == 1) { isc_refcount_destroy(&keytable->references); - REQUIRE(atomic_load_acquire(&keytable->active_nodes) == 0); dns_rbt_destroy(&keytable->table); isc_rwlock_destroy(&keytable->rwlock); keytable->magic = 0; @@ -142,28 +172,12 @@ dns_keytable_detach(dns_keytable_t **keytablep) { } } -static void -free_dslist(isc_mem_t *mctx, dns_keynode_t *knode) { - dns_rdata_t *rdata = NULL; - - for (rdata = ISC_LIST_HEAD(knode->dslist->rdata); - rdata != NULL; - rdata = ISC_LIST_HEAD(knode->dslist->rdata)) - { - ISC_LIST_UNLINK(knode->dslist->rdata, rdata, link); - isc_mem_put(mctx, rdata->data, DNS_DS_BUFFERSIZE); - isc_mem_put(mctx, rdata, sizeof(*rdata)); - } - - isc_mem_put(mctx, knode->dslist, sizeof(*knode->dslist)); - knode->dslist = NULL; -} - static isc_result_t add_ds(dns_keynode_t *knode, dns_rdata_ds_t *ds, isc_mem_t *mctx) { isc_result_t result; - dns_rdata_t *rdata = NULL; + dns_rdata_t *dsrdata = NULL, *rdata = NULL; void *data = NULL; + bool exists = false; isc_buffer_t b; if (knode->dslist == NULL) { @@ -173,19 +187,35 @@ add_ds(dns_keynode_t *knode, dns_rdata_ds_t *ds, isc_mem_t *mctx) { knode->dslist->type = dns_rdatatype_ds; } - rdata = isc_mem_get(mctx, sizeof(*rdata)); - dns_rdata_init(rdata); + dsrdata = isc_mem_get(mctx, sizeof(*dsrdata)); + dns_rdata_init(dsrdata); data = isc_mem_get(mctx, DNS_DS_BUFFERSIZE); isc_buffer_init(&b, data, DNS_DS_BUFFERSIZE); - result = dns_rdata_fromstruct(rdata, dns_rdataclass_in, + result = dns_rdata_fromstruct(dsrdata, dns_rdataclass_in, dns_rdatatype_ds, ds, &b); if (result != ISC_R_SUCCESS) { return (result); } - ISC_LIST_APPEND(knode->dslist->rdata, rdata, link); + for (rdata = ISC_LIST_HEAD(knode->dslist->rdata); + rdata != NULL; + rdata = ISC_LIST_NEXT(rdata, link)) + { + if (dns_rdata_compare(rdata, dsrdata) == 0) { + exists = true; + break; + } + } + + if (exists) { + isc_mem_put(mctx, dsrdata->data, DNS_DS_BUFFERSIZE); + isc_mem_put(mctx, dsrdata, sizeof(*dsrdata)); + return (ISC_R_SUCCESS); + } + + ISC_LIST_APPEND(knode->dslist->rdata, dsrdata, link); if (dns_rdataset_isassociated(&knode->dsset)) { dns_rdataset_disassociate(&knode->dsset); @@ -200,6 +230,51 @@ add_ds(dns_keynode_t *knode, dns_rdata_ds_t *ds, isc_mem_t *mctx) { return (ISC_R_SUCCESS); } +static isc_result_t +delete_ds(dns_keynode_t *knode, dns_rdata_ds_t *ds, isc_mem_t *mctx) { + isc_result_t result; + dns_rdata_t dsrdata = DNS_RDATA_INIT; + dns_rdata_t *rdata = NULL; + unsigned char data[DNS_DS_BUFFERSIZE]; + bool found = false; + isc_buffer_t b; + + if (knode->dslist == NULL) { + return (ISC_R_SUCCESS); + } + + isc_buffer_init(&b, data, DNS_DS_BUFFERSIZE); + + result = dns_rdata_fromstruct(&dsrdata, dns_rdataclass_in, + dns_rdatatype_ds, ds, &b); + if (result != ISC_R_SUCCESS) { + return (result); + } + + for (rdata = ISC_LIST_HEAD(knode->dslist->rdata); + rdata != NULL; + rdata = ISC_LIST_NEXT(rdata, link)) + { + if (dns_rdata_compare(rdata, &dsrdata) == 0) { + ISC_LIST_UNLINK(knode->dslist->rdata, rdata, link); + isc_mem_put(mctx, rdata->data, DNS_DS_BUFFERSIZE); + isc_mem_put(mctx, rdata, sizeof(*rdata)); + found = true; + break; + } + } + + if (found) { + return (ISC_R_SUCCESS); + } else { + /* + * The keyname must have matched or we wouldn't be here, + * so we use DNS_R_PARTIALMATCH instead of ISC_R_NOTFOUND. + */ + return (DNS_R_PARTIALMATCH); + } +} + /*% * 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 @@ -236,7 +311,6 @@ new_keynode(dns_rdata_ds_t *ds, dns_rbtnode_t *node, knode->managed = managed; knode->initial = initial; - knode->next = node->data; node->data = knode; return (ISC_R_SUCCESS); @@ -265,7 +339,7 @@ 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 "ds" is NULL) + * trust anchor (or a null key node if "ds" is NULL) * and attach it to the created node. */ result = new_keynode(ds, node, keytable, managed, initial); @@ -305,6 +379,7 @@ dns_keytable_add(dns_keytable_t *keytable, bool managed, bool initial, { REQUIRE(ds != NULL); REQUIRE(!initial || managed); + return (insert(keytable, managed, initial, name, ds)); } @@ -325,13 +400,15 @@ dns_keytable_delete(dns_keytable_t *keytable, const dns_name_t *keyname) { result = dns_rbt_findnode(keytable->table, keyname, NULL, &node, NULL, DNS_RBTFIND_NOOPTIONS, NULL, NULL); if (result == ISC_R_SUCCESS) { - if (node->data != NULL) + if (node->data != NULL) { result = dns_rbt_deletenode(keytable->table, node, false); - else + } else { result = ISC_R_NOTFOUND; - } else if (result == DNS_R_PARTIALMATCH) + } + } else if (result == DNS_R_PARTIALMATCH) { result = ISC_R_NOTFOUND; + } RWUNLOCK(&keytable->rwlock, isc_rwlocktype_write); return (result); @@ -343,24 +420,18 @@ dns_keytable_deletekey(dns_keytable_t *keytable, const dns_name_t *keyname, { isc_result_t result; dns_rbtnode_t *node = NULL; - dns_keynode_t *knode = NULL, **kprev = NULL; - dst_key_t *dstkey = NULL; - unsigned char data[4096]; - isc_buffer_t buffer; + dns_keynode_t *knode = NULL; dns_rdata_t rdata = DNS_RDATA_INIT; + unsigned char data[4096], digest[DNS_DS_BUFFERSIZE]; + dns_rdata_ds_t ds; + isc_buffer_t b; REQUIRE(VALID_KEYTABLE(keytable)); REQUIRE(dnskey != NULL); - /* Convert dnskey to DST key. */ - isc_buffer_init(&buffer, data, sizeof(data)); + isc_buffer_init(&b, data, sizeof(data)); dns_rdata_fromstruct(&rdata, dnskey->common.rdclass, - dns_rdatatype_dnskey, dnskey, &buffer); - result = dns_dnssec_keyfromrdata(keyname, &rdata, keytable->mctx, - &dstkey); - if (result != ISC_R_SUCCESS) { - return (result); - } + dns_rdatatype_dnskey, dnskey, &b); RWLOCK(&keytable->rwlock, isc_rwlocktype_write); result = dns_rbt_findnode(keytable->table, keyname, NULL, &node, NULL, @@ -379,45 +450,22 @@ dns_keytable_deletekey(dns_keytable_t *keytable, const dns_name_t *keyname, } knode = node->data; - if (knode->next == NULL && knode->key != NULL && - dst_key_compare(knode->key, dstkey)) - { - result = dns_rbt_deletenode(keytable->table, node, false); + + if (knode->dslist == NULL) { + result = DNS_R_PARTIALMATCH; goto finish; } - kprev = (dns_keynode_t **) &node->data; - while (knode != NULL) { - if (knode->key != NULL && dst_key_compare(knode->key, dstkey)) { - break; - } - - kprev = &knode->next; - knode = knode->next; + result = dns_ds_fromkeyrdata(keyname, &rdata, DNS_DSDIGEST_SHA256, + digest, &ds); + if (result != ISC_R_SUCCESS) { + goto finish; } - if (knode != NULL) { - if (knode->key != NULL) { - dst_key_free(&knode->key); - } + result = delete_ds(knode, &ds, keytable->mctx); - /* - * This is equivalent to: - * dns_keynode_attach(knode->next, &tmp); - * dns_keynode_detach(kprev); - * dns_keynode_attach(tmp, &kprev); - * dns_keynode_detach(&tmp); - */ - *kprev = knode->next; - knode->next = NULL; - dns_keynode_detach(keytable->mctx, &knode); - } else { - result = DNS_R_PARTIALMATCH; - } - - finish: + finish: RWUNLOCK(&keytable->rwlock, isc_rwlocktype_write); - dst_key_free(&dstkey); return (result); } @@ -437,8 +485,7 @@ dns_keytable_find(dns_keytable_t *keytable, const dns_name_t *keyname, DNS_RBTFIND_NOOPTIONS, NULL, NULL); if (result == ISC_R_SUCCESS) { if (node->data != NULL) { - dns_keytable_attachkeynode(keytable, node->data, - keynodep); + keynode_attach(node->data, keynodep); } else { result = ISC_R_NOTFOUND; } @@ -480,26 +527,7 @@ dns_keytable_finddeepestmatch(dns_keytable_t *keytable, const dns_name_t *name, } void -dns_keytable_attachkeynode(dns_keytable_t *keytable, dns_keynode_t *source, - dns_keynode_t **target) -{ - /* - * Give back a keynode found via dns_keytable_findkeynode(). - */ - - REQUIRE(VALID_KEYTABLE(keytable)); - REQUIRE(VALID_KEYNODE(source)); - REQUIRE(target != NULL && *target == NULL); - - REQUIRE(atomic_fetch_add_relaxed(&keytable->active_nodes, - 1) < UINT32_MAX); - - dns_keynode_attach(source, target); -} - -void -dns_keytable_detachkeynode(dns_keytable_t *keytable, dns_keynode_t **keynodep) -{ +dns_keytable_detachkeynode(dns_keytable_t *keytable, dns_keynode_t **keynodep) { /* * Give back a keynode found via dns_keytable_findkeynode(). */ @@ -507,9 +535,7 @@ dns_keytable_detachkeynode(dns_keytable_t *keytable, dns_keynode_t **keynodep) REQUIRE(VALID_KEYTABLE(keytable)); REQUIRE(keynodep != NULL && VALID_KEYNODE(*keynodep)); - REQUIRE(atomic_fetch_sub_release(&keytable->active_nodes, 1) > 0); - - dns_keynode_detach(keytable->mctx, keynodep); + keynode_detach(keytable->mctx, keynodep); } isc_result_t @@ -615,7 +641,7 @@ keynode_dslist_totext(dns_name_t *name, dns_keynode_t *keynode, dns_secalg_format(ds.algorithm, algbuf, sizeof(algbuf)); - snprintf(obuf, sizeof(obuf), "%s/%s/%d ; %s%s (DS)\n", + snprintf(obuf, sizeof(obuf), "%s/%s/%d ; %s%s\n", namebuf, algbuf, ds.key_tag, keynode->initial ? "initializing " : "", keynode->managed ? "managed" : "static"); @@ -655,8 +681,6 @@ dns_keytable_totext(dns_keytable_t *keytable, isc_buffer_t **text) { goto cleanup; } for (;;) { - char pbuf[DST_KEY_FORMATSIZE]; - dns_rbtnodechain_current(&chain, foundname, origin, &node); knode = node->data; @@ -668,24 +692,8 @@ dns_keytable_totext(dns_keytable_t *keytable, isc_buffer_t **text) { } result = keynode_dslist_totext(fullname, knode, text); - goto cleanup; - } - - for (; knode != NULL; knode = knode->next) { - char obuf[DNS_NAME_FORMATSIZE + 200]; - - if (knode->key == NULL) { - continue; - } - - dst_key_format(knode->key, pbuf, sizeof(pbuf)); - snprintf(obuf, sizeof(obuf), "%s ; %s%s\n", pbuf, - knode->initial ? "initializing " : "", - knode->managed ? "managed" : "static"); - - result = putstr(text, obuf); if (result != ISC_R_SUCCESS) { - break; + goto cleanup; } } @@ -698,7 +706,7 @@ dns_keytable_totext(dns_keytable_t *keytable, isc_buffer_t **text) { } } - cleanup: + cleanup: dns_rbtnodechain_invalidate(&chain); RWUNLOCK(&keytable->rwlock, isc_rwlocktype_read); return (result); @@ -731,8 +739,7 @@ dns_keytable_forall(dns_keytable_t *keytable, } goto cleanup; } - REQUIRE(atomic_fetch_add_relaxed(&keytable->active_nodes, - 1) < UINT32_MAX); + for (;;) { dns_rbtnodechain_current(&chain, foundname, origin, &node); if (node->data != NULL) { @@ -749,9 +756,8 @@ dns_keytable_forall(dns_keytable_t *keytable, break; } } - REQUIRE(atomic_fetch_sub_release(&keytable->active_nodes, 1) > 0); - cleanup: + cleanup: dns_rbtnodechain_invalidate(&chain); RWUNLOCK(&keytable->rwlock, isc_rwlocktype_read); return (result); @@ -788,43 +794,3 @@ dns_keynode_trust(dns_keynode_t *keynode) { keynode->initial = false; } - -void -dns_keynode_attach(dns_keynode_t *source, dns_keynode_t **target) { - REQUIRE(VALID_KEYNODE(source)); - isc_refcount_increment(&source->refcount); - *target = source; -} - -void -dns_keynode_detach(isc_mem_t *mctx, dns_keynode_t **keynodep) { - REQUIRE(keynodep != NULL && VALID_KEYNODE(*keynodep)); - dns_keynode_t *knode = *keynodep; - *keynodep = NULL; - - if (isc_refcount_decrement(&knode->refcount) == 1) { - isc_refcount_destroy(&knode->refcount); - if (knode->key != NULL) { - dst_key_free(&knode->key); - } - if (knode->dslist != NULL) { - if (dns_rdataset_isassociated(&knode->dsset)) { - dns_rdataset_disassociate(&knode->dsset); - } - free_dslist(mctx, knode); - } - isc_mem_put(mctx, knode, sizeof(dns_keynode_t)); - } -} - -void -dns_keynode_detachall(isc_mem_t *mctx, dns_keynode_t **keynode) { - dns_keynode_t *next = NULL, *node = *keynode; - REQUIRE(VALID_KEYNODE(node)); - while (node != NULL) { - next = node->next; - dns_keynode_detach(mctx, &node); - node = next; - } - *keynode = NULL; -} diff --git a/lib/dns/tests/keytable_test.c b/lib/dns/tests/keytable_test.c index e1ab31eaea..7b97ab5c87 100644 --- a/lib/dns/tests/keytable_test.c +++ b/lib/dns/tests/keytable_test.c @@ -68,7 +68,6 @@ dns_keytable_t *keytable = NULL; dns_ntatable_t *ntatable = NULL; static const char *keystr1 = "BQEAAAABok+vaUC9neRv8yeT/FEGgN7svR8s7VBUVSBd8NsAiV8AlaAg O5FHar3JQd95i/puZos6Vi6at9/JBbN8qVmO2AuiXxVqfxMKxIcy+LEB 0Vw4NaSJ3N3uaVREso6aTSs98H/25MjcwLOr7SFfXA7bGhZatLtYY/xu kp6Km5hMfkE="; -static const dns_keytag_t keytag1 = 30591; static const char *keystr2 = "BQEAAAABwuHz9Cem0BJ0JQTO7C/a3McR6hMaufljs1dfG/inaJpYv7vH XTrAOm/MeKp+/x6eT4QLru0KoZkvZJnqTI8JyaFTw2OM/ItBfh/hL2lm Cft2O7n3MfeqYtvjPnY7dWghYW4sVfH7VVEGm958o9nfi79532Qeklxh x8pXWdeAaRU="; @@ -434,16 +433,23 @@ deletekey_test(void **state) { dns_rdata_freestruct(&dnskey); /* - * exact match. after deleting the node the internal rbt node will be - * empty, and any delete or deletekeynode attempt should result in - * NOTFOUND. + * exact match: should return SUCCESS on the first try, then + * PARTIALMATCH on the second (because the name existed but + * not a matching key). */ create_keystruct(257, 3, 5, keystr1, &dnskey); assert_int_equal(dns_keytable_deletekey(keytable, keyname, &dnskey), ISC_R_SUCCESS); assert_int_equal(dns_keytable_deletekey(keytable, keyname, &dnskey), - ISC_R_NOTFOUND); + DNS_R_PARTIALMATCH); + + /* + * after deleting the node, any deletekey or delete attempt should + * result in NOTFOUND. + */ assert_int_equal(dns_keytable_delete(keytable, keyname), + ISC_R_SUCCESS); + assert_int_equal(dns_keytable_deletekey(keytable, keyname, &dnskey), ISC_R_NOTFOUND); dns_rdata_freestruct(&dnskey); @@ -691,23 +697,16 @@ nta_test(void **state) { int main(void) { const struct CMUnitTest tests[] = { - cmocka_unit_test_setup_teardown(add_test, - _setup, _teardown), - cmocka_unit_test_setup_teardown(delete_test, - _setup, _teardown), - cmocka_unit_test_setup_teardown(deletekey_test, - _setup, _teardown), - cmocka_unit_test_setup_teardown(find_test, - _setup, _teardown), - cmocka_unit_test_setup_teardown(issecuredomain_test, - _setup, _teardown), - cmocka_unit_test_setup_teardown(dump_test, - _setup, _teardown), - cmocka_unit_test_setup_teardown(nta_test, - _setup, _teardown), + cmocka_unit_test(add_test), + cmocka_unit_test(delete_test), + cmocka_unit_test(deletekey_test), + cmocka_unit_test(find_test), + cmocka_unit_test(issecuredomain_test), + cmocka_unit_test(dump_test), + cmocka_unit_test(nta_test), }; - return (cmocka_run_group_tests(tests, NULL, NULL)); + return (cmocka_run_group_tests(tests, _setup, _teardown)); } #else /* HAVE_CMOCKA */ diff --git a/lib/dns/win32/libdns.def.in b/lib/dns/win32/libdns.def.in index d41d0b4a91..bd7d861845 100644 --- a/lib/dns/win32/libdns.def.in +++ b/lib/dns/win32/libdns.def.in @@ -458,9 +458,6 @@ dns_keydata_fromdnskey dns_keydata_todnskey dns_keyflags_fromtext dns_keymgr_run -dns_keynode_attach -dns_keynode_detach -dns_keynode_detachall dns_keynode_dsset dns_keynode_initial dns_keynode_managed @@ -468,7 +465,6 @@ dns_keynode_trust dns_keyring_restore dns_keytable_add dns_keytable_attach -dns_keytable_attachkeynode dns_keytable_create dns_keytable_delete dns_keytable_deletekey From 6799a222d1f0ed79c0fb3ade304f7457853adcd8 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 24 Dec 2019 13:59:34 -0800 Subject: [PATCH 6/6] keep the keynode attached as long as dsset is in use when using the trust anchor dsset as val->dsset, keep a reference to the keynode so dsset can't be freed. --- lib/dns/include/dns/validator.h | 1 + lib/dns/validator.c | 105 +++++++++++++++++--------------- 2 files changed, 56 insertions(+), 50 deletions(-) diff --git a/lib/dns/include/dns/validator.h b/lib/dns/include/dns/validator.h index 0057e1d5b5..fd1e490649 100644 --- a/lib/dns/include/dns/validator.h +++ b/lib/dns/include/dns/validator.h @@ -128,6 +128,7 @@ struct dns_validator { dns_validator_t * subvalidator; dns_validator_t * parent; dns_keytable_t * keytable; + dns_keynode_t * keynode; dst_key_t * key; dns_rdata_rrsig_t * siginfo; isc_task_t * task; diff --git a/lib/dns/validator.c b/lib/dns/validator.c index ee6f98e0d9..3e1e852531 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -533,6 +533,7 @@ fetch_callback_ds(isc_task_t *task, isc_event_t *event) { "dsset with trust %s", dns_trust_totext(rdataset->trust)); val->dsset = &val->frdataset; + INSIST(val->keynode == NULL); result = validate_dnskey(val); if (result != DNS_R_WAIT) { validator_done(val, result); @@ -1678,14 +1679,16 @@ check_signer(dns_validator_t *val, dns_rdata_t *keyrdata, break; } } + if (dstkey != NULL) { dst_key_free(&dstkey); } + return (result); } /* - * get_dsset is called to look up a DS RRset corresponding to the name + * get_dsset() is called to look up a DS RRset corresponding to the name * of a DNSKEY record, either in the cache or, if necessary, by starting a * fetch. This is done in the context of validating a zone key to build a * trust chain. @@ -1708,6 +1711,7 @@ get_dsset(dns_validator_t *val, dns_name_t *tname, isc_result_t *resp) { * We have a DS RRset. */ val->dsset = &val->frdataset; + INSIST(val->keynode == NULL); if ((DNS_TRUST_PENDING(val->frdataset.trust) || DNS_TRUST_ANSWER(val->frdataset.trust)) && dns_rdataset_isassociated(&val->fsigrdataset)) @@ -1814,34 +1818,42 @@ validate_dnskey(dns_validator_t *val) { } /* - * If that didn't work, see if there's a key-style trust anchor we - * can validate against. If not, look up the DS at the parent. + * No trust anchor for this name, so we look up the DS at the parent. */ if (val->dsset == NULL) { isc_result_t tresult = ISC_R_SUCCESS; /* - * If this is the root name and there was no trusted key, + * If this is the root name and there was no trust anchor, * we can give up now, since there's no DS at the root. */ if (dns_name_equal(val->event->name, dns_rootname)) { if ((val->attributes & VALATTR_TRIEDVERIFY) != 0) { validator_log(val, ISC_LOG_DEBUG(3), "root key failed to validate"); - return (DNS_R_NOVALIDSIG); } else { validator_log(val, ISC_LOG_DEBUG(3), "no trusted root key"); - return (DNS_R_NOVALIDDS); } + result = DNS_R_NOVALIDSIG; + goto cleanup; } /* - * Otherwise, look up the DS RRset for this name. + * Look up the DS RRset for this name. */ result = get_dsset(val, val->event->name, &tresult); if (result == ISC_R_COMPLETE) { - return (tresult); + if (tresult == DNS_R_WAIT) { + /* + * Keep the keynode attached so we don't + * lose val->dsset. + */ + val->keynode = keynode; + } + + result = tresult; + goto cleanup; } } @@ -1935,8 +1947,7 @@ validate_dnskey(dns_validator_t *val) { /* * Find the DNSKEY matching the DS... */ - result = dns_dnssec_matchdskey(val->event->name, - &dsrdata, + result = dns_dnssec_matchdskey(val->event->name, &dsrdata, val->event->rdataset, &keyrdata); if (result != ISC_R_SUCCESS) { @@ -1956,25 +1967,27 @@ validate_dnskey(dns_validator_t *val) { "no RRSIG matching DS key"); } + if (result == ISC_R_SUCCESS) { + marksecure(val->event); + validator_log(val, ISC_LOG_DEBUG(3), "marking as secure (DS)"); + } else if (result == ISC_R_NOMORE && !supported_algorithm) { + validator_log(val, ISC_LOG_DEBUG(3), + "no supported algorithm/digest (DS)"); + result = markanswer(val, "validate_dnskey (3)", + "no supported algorithm/digest (DS)"); + } else { + validator_log(val, ISC_LOG_INFO, + "no valid signature found (DS)"); + result = DNS_R_NOVALIDSIG; + } + + cleanup: if (keynode != NULL) { val->dsset = NULL; dns_keytable_detachkeynode(val->keytable, &keynode); } - if (result == ISC_R_SUCCESS) { - marksecure(val->event); - validator_log(val, ISC_LOG_DEBUG(3), "marking as secure (DS)"); - return (result); - } else if (result == ISC_R_NOMORE && !supported_algorithm) { - validator_log(val, ISC_LOG_DEBUG(3), - "no supported algorithm/digest (DS)"); - return (markanswer(val, "validate_dnskey (3)", - "no supported algorithm/digest (DS)")); - } else { - validator_log(val, ISC_LOG_INFO, - "no valid signature found (DS)"); - return (DNS_R_NOVALIDSIG); - } + return (result); } /*% @@ -3123,10 +3136,6 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, (rdataset == NULL && sigrdataset == NULL && message != NULL)); REQUIRE(validatorp != NULL && *validatorp == NULL); - val = isc_mem_get(view->mctx, sizeof(*val)); - val->view = NULL; - dns_view_weakattach(view, &val->view); - event = (dns_validatorevent_t *) isc_event_allocate(view->mctx, task, DNS_EVENT_VALIDATORSTART, @@ -3134,7 +3143,6 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, sizeof(dns_validatorevent_t)); isc_task_attach(task, &tclone); - event->validator = val; event->result = ISC_R_FAILURE; event->name = name; event->type = type; @@ -3145,32 +3153,23 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, event->optout = false; event->secure = false; + val = isc_mem_get(view->mctx, sizeof(*val)); + *val = (dns_validator_t) { + .event = event, + .options = options, + .task = task, + .action = action, + .arg = arg + }; + + dns_view_weakattach(view, &val->view); isc_mutex_init(&val->lock); - val->event = event; - val->options = options; - val->attributes = 0; - val->fetch = NULL; - val->subvalidator = NULL; - val->parent = NULL; - - val->keytable = NULL; result = dns_view_getsecroots(val->view, &val->keytable); if (result != ISC_R_SUCCESS) { goto cleanup; } - val->key = NULL; - val->siginfo = NULL; - val->task = task; - val->action = action; - val->arg = arg; - val->labels = 0; - val->currentset = NULL; - val->keyset = NULL; - val->dsset = NULL; - val->depth = 0; - val->authcount = 0; - val->authfail = 0; + val->mustbesecure = dns_resolver_getmustbesecure(view->resolver, name); dns_rdataset_init(&val->frdataset); dns_rdataset_init(&val->fsigrdataset); @@ -3180,6 +3179,8 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, ISC_LINK_INIT(val, link); val->magic = VALIDATOR_MAGIC; + event->validator = val; + if ((options & DNS_VALIDATOR_DEFER) == 0) { isc_task_send(task, ISC_EVENT_PTR(&event)); } @@ -3257,10 +3258,15 @@ destroy(dns_validator_t *val) { REQUIRE(val->event == NULL); REQUIRE(val->fetch == NULL); + val->magic = 0; if (val->key != NULL) { dst_key_free(&val->key); } if (val->keytable != NULL) { + if (val->keynode != NULL) { + dns_keytable_detachkeynode(val->keytable, + &val->keynode); + } dns_keytable_detach(&val->keytable); } if (val->subvalidator != NULL) { @@ -3273,7 +3279,6 @@ destroy(dns_validator_t *val) { } isc_mutex_destroy(&val->lock); dns_view_weakdetach(&val->view); - val->magic = 0; isc_mem_put(mctx, val, sizeof(*val)); }