From: Evan Hunt Date: Fri, 14 Mar 2025 23:41:47 +0000 (-0700) Subject: optimize key ID check when searching for matching keys X-Git-Tag: v9.21.7~37^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2e6107008dae09d32e3d34fb5423b3d78c4ff651;p=thirdparty%2Fbind9.git optimize key ID check when searching for matching keys when searching a DNSKEY or KEY rrset for the key that matches a particular algorithm and ID, it's a waste of time to convert every key into a dst_key object; it's faster to compute the key ID by checksumming the region, and then only do the full key conversion once we know we've found the correct key. this optimization was already in use in the validator, but it's been refactored for code clarity, and is now also used in query.c and message.c. --- diff --git a/lib/dns/dst_api.c b/lib/dns/dst_api.c index 3f844264c66..ab2d432c182 100644 --- a/lib/dns/dst_api.c +++ b/lib/dns/dst_api.c @@ -162,8 +162,7 @@ computeid(dst_key_t *key); static isc_result_t frombuffer(const dns_name_t *name, unsigned int alg, unsigned int flags, unsigned int protocol, dns_rdataclass_t rdclass, - isc_buffer_t *source, isc_mem_t *mctx, bool no_rdata, - dst_key_t **keyp); + isc_buffer_t *source, isc_mem_t *mctx, dst_key_t **keyp); static isc_result_t algorithm_status(unsigned int alg); @@ -721,13 +720,6 @@ dst_key_todns(const dst_key_t *key, isc_buffer_t *target) { isc_result_t dst_key_fromdns(const dns_name_t *name, dns_rdataclass_t rdclass, isc_buffer_t *source, isc_mem_t *mctx, dst_key_t **keyp) { - return dst_key_fromdns_ex(name, rdclass, source, mctx, false, keyp); -} - -isc_result_t -dst_key_fromdns_ex(const dns_name_t *name, dns_rdataclass_t rdclass, - isc_buffer_t *source, isc_mem_t *mctx, bool no_rdata, - dst_key_t **keyp) { uint8_t alg, proto; uint32_t flags, extflags; dst_key_t *key = NULL; @@ -756,7 +748,7 @@ dst_key_fromdns_ex(const dns_name_t *name, dns_rdataclass_t rdclass, } result = frombuffer(name, alg, flags, proto, rdclass, source, mctx, - no_rdata, &key); + &key); if (result != ISC_R_SUCCESS) { return result; } @@ -775,7 +767,7 @@ dst_key_frombuffer(const dns_name_t *name, unsigned int alg, unsigned int flags, isc_result_t result; result = frombuffer(name, alg, flags, protocol, rdclass, source, mctx, - false, &key); + &key); if (result != ISC_R_SUCCESS) { return result; } @@ -2267,8 +2259,7 @@ computeid(dst_key_t *key) { static isc_result_t frombuffer(const dns_name_t *name, unsigned int alg, unsigned int flags, unsigned int protocol, dns_rdataclass_t rdclass, - isc_buffer_t *source, isc_mem_t *mctx, bool no_rdata, - dst_key_t **keyp) { + isc_buffer_t *source, isc_mem_t *mctx, dst_key_t **keyp) { dst_key_t *key; isc_result_t ret; @@ -2290,12 +2281,10 @@ frombuffer(const dns_name_t *name, unsigned int alg, unsigned int flags, return DST_R_UNSUPPORTEDALG; } - if (!no_rdata) { - ret = key->func->fromdns(key, source); - if (ret != ISC_R_SUCCESS) { - dst_key_free(&key); - return ret; - } + ret = key->func->fromdns(key, source); + if (ret != ISC_R_SUCCESS) { + dst_key_free(&key); + return ret; } } diff --git a/lib/dns/include/dst/dst.h b/lib/dns/include/dst/dst.h index 4bcb026b1c1..f6e1e2e05bb 100644 --- a/lib/dns/include/dst/dst.h +++ b/lib/dns/include/dst/dst.h @@ -460,10 +460,6 @@ dst_key_tofile(const dst_key_t *key, int type, const char *directory); */ isc_result_t -dst_key_fromdns_ex(const dns_name_t *name, dns_rdataclass_t rdclass, - isc_buffer_t *source, isc_mem_t *mctx, bool no_rdata, - dst_key_t **keyp); -isc_result_t dst_key_fromdns(const dns_name_t *name, dns_rdataclass_t rdclass, isc_buffer_t *source, isc_mem_t *mctx, dst_key_t **keyp); /*%< diff --git a/lib/dns/message.c b/lib/dns/message.c index e52dca45331..caeff02f4fc 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -3105,7 +3105,7 @@ dns_message_checksig_async(dns_message_t *msg, dns_view_t *view, isc_result_t dns_message_checksig(dns_message_t *msg, dns_view_t *view) { - isc_buffer_t b, msgb; + isc_buffer_t msgb; REQUIRE(DNS_MESSAGE_VALID(msg)); @@ -3126,7 +3126,7 @@ dns_message_checksig(dns_message_t *msg, dns_view_t *view) { return dns_tsig_verify(&msgb, msg, NULL, NULL); } } else { - dns_rdata_t rdata = DNS_RDATA_INIT; + dns_rdata_t sigrdata = DNS_RDATA_INIT; dns_rdata_sig_t sig; dns_rdataset_t keyset; isc_result_t result; @@ -3134,7 +3134,7 @@ dns_message_checksig(dns_message_t *msg, dns_view_t *view) { result = dns_rdataset_first(msg->sig0); INSIST(result == ISC_R_SUCCESS); - dns_rdataset_current(msg->sig0, &rdata); + dns_rdataset_current(msg->sig0, &sigrdata); /* * This can occur when the message is a dynamic update, since @@ -3143,11 +3143,11 @@ dns_message_checksig(dns_message_t *msg, dns_view_t *view) { * looked for in the additional section, and the dynamic update * meta-records are in the prerequisite and update sections. */ - if (rdata.length == 0) { + if (sigrdata.length == 0) { return ISC_R_UNEXPECTEDEND; } - result = dns_rdata_tostruct(&rdata, &sig, NULL); + result = dns_rdata_tostruct(&sigrdata, &sig, NULL); if (result != ISC_R_SUCCESS) { return result; } @@ -3191,26 +3191,32 @@ dns_message_checksig(dns_message_t *msg, dns_view_t *view) { message_checks < max_message_checks; key_checks++, result = dns_rdataset_next(&keyset)) { + dns_rdata_t keyrdata = DNS_RDATA_INIT; + dns_rdata_key_t ks; dst_key_t *key = NULL; + isc_region_t r; - dns_rdata_reset(&rdata); - dns_rdataset_current(&keyset, &rdata); - isc_buffer_init(&b, rdata.data, rdata.length); - isc_buffer_add(&b, rdata.length); + dns_rdataset_current(&keyset, &keyrdata); + dns_rdata_tostruct(&keyrdata, &ks, NULL); - result = dst_key_fromdns(&sig.signer, rdata.rdclass, &b, - view->mctx, &key); - if (result != ISC_R_SUCCESS) { + if (sig.algorithm != ks.algorithm || + (ks.protocol != DNS_KEYPROTO_DNSSEC && + ks.protocol != DNS_KEYPROTO_ANY)) + { continue; } - if (dst_key_alg(key) != sig.algorithm || - dst_key_id(key) != sig.keyid || - !(dst_key_proto(key) == DNS_KEYPROTO_DNSSEC || - dst_key_proto(key) == DNS_KEYPROTO_ANY)) - { - dst_key_free(&key); + + dns_rdata_toregion(&keyrdata, &r); + if (dst_region_computeid(&r) != sig.keyid) { + continue; + } + + result = dns_dnssec_keyfromrdata(&sig.signer, &keyrdata, + view->mctx, &key); + if (result != ISC_R_SUCCESS) { continue; } + result = dns_dnssec_verifymessage(&msgb, msg, key); dst_key_free(&key); if (result == ISC_R_SUCCESS) { diff --git a/lib/dns/validator.c b/lib/dns/validator.c index 770a549ad0a..f103d8368be 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -1011,59 +1011,46 @@ create_validator(dns_validator_t *val, dns_name_t *name, dns_rdatatype_t type, static isc_result_t select_signing_key(dns_validator_t *val, dns_rdataset_t *rdataset) { isc_result_t result; - dns_rdata_rrsig_t *siginfo = val->siginfo; - isc_buffer_t b; - dns_rdata_t rdata = DNS_RDATA_INIT; - dst_key_t *oldkey = val->key; - bool no_rdata = false; - if (oldkey == NULL) { + if (val->key == NULL) { result = dns_rdataset_first(rdataset); } else { - dst_key_free(&oldkey); + dst_key_free(&val->key); val->key = NULL; result = dns_rdataset_next(rdataset); } - if (result != ISC_R_SUCCESS) { - goto done; + if (result == ISC_R_NOMORE) { + return ISC_R_NOTFOUND; } - do { + for (; result == ISC_R_SUCCESS; result = dns_rdataset_next(rdataset)) { + dns_rdata_rrsig_t *siginfo = val->siginfo; + dns_rdata_t rdata = DNS_RDATA_INIT; + dns_rdata_dnskey_t key; + isc_region_t r; + dns_rdataset_current(rdataset, &rdata); + dns_rdata_tostruct(&rdata, &key, NULL); /* can't fail */ - isc_buffer_init(&b, rdata.data, rdata.length); - isc_buffer_add(&b, rdata.length); - INSIST(val->key == NULL); - result = dst_key_fromdns_ex(&siginfo->signer, rdata.rdclass, &b, - val->view->mctx, no_rdata, - &val->key); - if (result == ISC_R_SUCCESS) { - if (siginfo->algorithm == - (dns_secalg_t)dst_key_alg(val->key) && - siginfo->keyid == - (dns_keytag_t)dst_key_id(val->key) && - (dst_key_flags(val->key) & DNS_KEYFLAG_REVOKE) == - 0 && - dst_key_iszonekey(val->key)) - { - if (no_rdata) { - /* Retry with full key */ - dns_rdata_reset(&rdata); - dst_key_free(&val->key); - no_rdata = false; - continue; - } - /* This is the key we're looking for. */ - goto done; - } - dst_key_free(&val->key); + if (key.algorithm != siginfo->algorithm || + (key.flags & DNS_KEYFLAG_REVOKE) != 0 || + !dns_dnssec_iszonekey(&key)) + { + continue; } - dns_rdata_reset(&rdata); - result = dns_rdataset_next(rdataset); - no_rdata = true; - } while (result == ISC_R_SUCCESS); -done: + dns_rdata_toregion(&rdata, &r); + if (dst_region_computeid(&r) != siginfo->keyid) { + continue; + } + + result = dns_dnssec_keyfromrdata(&siginfo->signer, &rdata, + val->view->mctx, &val->key); + if (result == ISC_R_SUCCESS) { + /* found the key we wanted */ + break; + } + } if (result == ISC_R_NOMORE) { result = ISC_R_NOTFOUND; } diff --git a/lib/ns/query.c b/lib/ns/query.c index 44d13382818..c5e07bfecd7 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -2420,25 +2420,33 @@ get_key(ns_client_t *client, dns_db_t *db, dns_rdata_rrsig_t *rrsig, for (; result == ISC_R_SUCCESS; result = dns_rdataset_next(keyrdataset)) { dns_rdata_t rdata = DNS_RDATA_INIT; - isc_buffer_t b; + dns_rdata_dnskey_t key; + isc_region_t r; dns_rdataset_current(keyrdataset, &rdata); - isc_buffer_init(&b, rdata.data, rdata.length); - isc_buffer_add(&b, rdata.length); - result = dst_key_fromdns(&rrsig->signer, rdata.rdclass, &b, - client->manager->mctx, keyp); - if (result != ISC_R_SUCCESS) { + dns_rdata_tostruct(&rdata, &key, NULL); /* can't fail */ + + if (rrsig->algorithm != key.algorithm || + !dns_dnssec_iszonekey(&key)) + { continue; } - if (rrsig->algorithm == (dns_secalg_t)dst_key_alg(*keyp) && - rrsig->keyid == (dns_keytag_t)dst_key_id(*keyp) && - dst_key_iszonekey(*keyp)) - { + + dns_rdata_toregion(&rdata, &r); + if (dst_region_computeid(&r) != rrsig->keyid) { + continue; + } + + result = dns_dnssec_keyfromrdata(&rrsig->signer, &rdata, + client->manager->mctx, keyp); + if (result == ISC_R_SUCCESS) { secure = true; break; } + dst_key_free(keyp); } + return secure; }