From 21d3f66f1c00dcd81726e67be43e98824b861ac2 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 11 Dec 2019 00:09:15 -0800 Subject: [PATCH] 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) {