From a6e187a8d514fe7977331a540f1ecacddcfec59b Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 11 Apr 2023 16:10:07 -0700 Subject: [PATCH] further dns_tsigkey API tweaks - remove the 'ring' parameter from dns_tsigkey_createfromkey(), and use dns_tsigkeyring_add() to add key objects to a keyring instead. - add a magic number to dns_tsigkeyring_t - change dns_tsigkeyring_dumpanddetach() to dns_tsigkeyring_dump(); we now call dns_tsigkeyring_detach() separately. - remove 'maxgenerated' from dns_tsigkeyring_t since it never changes. --- bin/dig/dighost.c | 2 +- bin/nsupdate/nsupdate.c | 18 +------ lib/dns/include/dns/tsig.h | 60 ++++++++++++++------- lib/dns/tkey.c | 22 ++++++-- lib/dns/tsig.c | 104 ++++++++++++------------------------- lib/dns/view.c | 8 ++- 6 files changed, 98 insertions(+), 116 deletions(-) diff --git a/bin/dig/dighost.c b/bin/dig/dighost.c index c8db355a26..beac0c0c1b 100644 --- a/bin/dig/dighost.c +++ b/bin/dig/dighost.c @@ -1187,7 +1187,7 @@ setup_file_key(void) { if (hmacname != NULL) { result = dns_tsigkey_createfromkey( dst_key_name(dstkey), hmacname, dstkey, false, false, - NULL, 0, 0, mctx, NULL, &tsigkey); + NULL, 0, 0, mctx, &tsigkey); if (result != ISC_R_SUCCESS) { printf(";; Couldn't create key %s: %s\n", keynametext, isc_result_totext(result)); diff --git a/bin/nsupdate/nsupdate.c b/bin/nsupdate/nsupdate.c index da91da6ab4..424dfce293 100644 --- a/bin/nsupdate/nsupdate.c +++ b/bin/nsupdate/nsupdate.c @@ -674,7 +674,7 @@ setup_keyfile(isc_mem_t *mctx, isc_log_t *lctx) { if (hmacname != NULL) { result = dns_tsigkey_createfromkey( dst_key_name(dstkey), hmacname, dstkey, false, false, - NULL, 0, 0, mctx, NULL, &tsigkey); + NULL, 0, 0, mctx, &tsigkey); dst_key_free(&dstkey); if (result != ISC_R_SUCCESS) { fprintf(stderr, "could not create key from %s: %s\n", @@ -3038,9 +3038,8 @@ start_gssrequest(dns_name_t *primary) { if (gssring != NULL) { dns_tsigkeyring_detach(&gssring); } - gssring = NULL; - result = dns_tsigkeyring_create(gmctx, &gssring); + result = dns_tsigkeyring_create(gmctx, &gssring); if (result != ISC_R_SUCCESS) { fatal("dns_tsigkeyring_create failed: %s", isc_result_totext(result)); @@ -3234,7 +3233,6 @@ recvgss(void *arg) { result = dns_name_fromtext(servname, &buf, dns_rootname, 0, NULL); check_result(result, "dns_name_fromtext"); - tsigkey = NULL; result = dns_tkey_gssnegotiate(tsigquery, rcvmsg, servname, &context, &tsigkey, gssring, &err_message); switch (result) { @@ -3262,18 +3260,6 @@ recvgss(void *arg) { * the TSIG -- this too is a spec violation, but it's * the least insane thing to do. */ -#if 0 - /* - * Verify the signature. - */ - rcvmsg->state = DNS_SECTION_ANY; - dns_message_setquerytsig(rcvmsg, NULL); - result = dns_message_settsigkey(rcvmsg, tsigkey); - check_result(result, "dns_message_settsigkey"); - result = dns_message_checksig(rcvmsg, NULL); - ddebug("tsig verification: %s", isc_result_totext(result)); - check_result(result, "dns_message_checksig"); -#endif /* 0 */ send_update(&tmpzonename, &primary_servers[primary_inuse]); setzoneclass(dns_rdataclass_none); diff --git a/lib/dns/include/dns/tsig.h b/lib/dns/include/dns/tsig.h index a573673749..4c7068acf9 100644 --- a/lib/dns/include/dns/tsig.h +++ b/lib/dns/include/dns/tsig.h @@ -54,7 +54,15 @@ extern const dns_name_t *dns_tsig_hmacsha512_name; */ #define DNS_TSIG_FUDGE 300 +/*% + * Default maximum quota for generated keys. + */ +#ifndef DNS_TSIG_MAXGENERATEDKEYS +#define DNS_TSIG_MAXGENERATEDKEYS 4096 +#endif /* ifndef DNS_TSIG_MAXGENERATEDKEYS */ + struct dns_tsigkeyring { + unsigned int magic; /*%< Magic number. */ dns_rbt_t *keys; unsigned int writecount; isc_rwlock_t lock; @@ -64,7 +72,6 @@ struct dns_tsigkeyring { * list and a maximum size. */ unsigned int generated; - unsigned int maxgenerated; ISC_LIST(dns_tsigkey_t) lru; isc_refcount_t references; }; @@ -111,19 +118,31 @@ dns_tsigkey_createfromkey(const dns_name_t *name, const dns_name_t *algorithm, dst_key_t *dstkey, bool generated, bool restored, const dns_name_t *creator, isc_stdtime_t inception, isc_stdtime_t expire, isc_mem_t *mctx, - dns_tsigkeyring_t *ring, dns_tsigkey_t **key); + dns_tsigkey_t **key); /*%< - * Creates a tsig key structure and saves it in the keyring. If key is - * not NULL, *key will contain a copy of the key. The keys validity - * period is specified by (inception, expire), and will not expire if - * inception == expire. If the key was generated, the creating identity, - * if there is one, should be in the creator parameter. Specifying an - * unimplemented algorithm will cause failure only if dstkey != NULL; this - * allows a transient key with an invalid algorithm to exist long enough - * to generate a BADKEY response. + * Creates a tsig key structure and stores it in *keyp. + * The key's validity period is specified by (inception, expire), + * and will not expire if inception == expire. * - * If dns_tsigkey_createfromkey is successful a new reference to 'dstkey' - * will have been made. + * If generated is true (meaning the key was generated + * via TKEY negotation), the creating identity (if any), should + * be specified in the creator parameter. + * + * If restored is true, this indicates the key was restored from + * a dump file created by dns_tsigkeyring_dumpanddetach(). This is + * used only for logging purposes and doesn't affect the key any + * other way. + * + * Specifying an unimplemented algorithm will cause failure only if + * dstkey != NULL; this allows a transient key with an invalid + * algorithm to exist long enough to generate a BADKEY response. + * + * If dns_tsigkey_createfromkey() is successful, a new reference to + * 'dstkey' will have been made. + * + * dns_tsigkey_create() is a simplified interface that omits + * dstkey, generated, restored, inception, and expired (defaulting + * to NULL, false, false, 0, and 0). * * Requires: *\li 'name' is a valid dns_name_t @@ -201,15 +220,15 @@ dns_tsig_verify(isc_buffer_t *source, dns_message_t *msg, */ isc_result_t -dns_tsigkey_find(dns_tsigkey_t **tsigkey, const dns_name_t *name, +dns_tsigkey_find(dns_tsigkey_t **tsigkeyp, const dns_name_t *name, const dns_name_t *algorithm, dns_tsigkeyring_t *ring); /*%< * Returns the TSIG key corresponding to this name and (possibly) * algorithm. Also increments the key's reference counter. * * Requires: - *\li 'tsigkey' is not NULL - *\li '*tsigkey' is NULL + *\li 'tsigkeyp' is not NULL + *\li '*tsigkeyp' is NULL *\li 'name' is a valid dns_name_t *\li 'algorithm' is a valid dns_name_t or NULL *\li 'ring' is a valid keyring @@ -239,6 +258,11 @@ dns_tsigkeyring_add(dns_tsigkeyring_t *ring, const dns_name_t *name, /*%< * Place a TSIG key onto a key ring. * + * If the key is generated, it is also placed into an LRU queue. + * There is a maximum quota of 4096 generated keys per keyring; + * if this quota is exceeded, the oldest key in the LRU queue is + * deleted. + * * Requires: *\li 'name' and 'ring' are not NULL *\li 'tkey' is a valid TSIG key, which has not been @@ -250,12 +274,12 @@ dns_tsigkeyring_add(dns_tsigkeyring_t *ring, const dns_name_t *name, */ isc_result_t -dns_tsigkeyring_dumpanddetach(dns_tsigkeyring_t **ringp, FILE *fp); +dns_tsigkeyring_dump(dns_tsigkeyring_t *ring, FILE *fp); /*%< - * Dump a TSIG key ring to 'fp' and destroy it. + * Dump a TSIG key ring to 'fp'. * * Requires: - *\li 'ringp' is not NULL + *\li 'ring' is a valid keyring. */ void diff --git a/lib/dns/tkey.c b/lib/dns/tkey.c index 00efdd7091..f1426d40eb 100644 --- a/lib/dns/tkey.c +++ b/lib/dns/tkey.c @@ -265,7 +265,8 @@ process_gsstkey(dns_message_t *msg, dns_name_t *name, dns_rdata_tkey_t *tkeyin, #endif /* HAVE_GSSAPI */ RETERR(dns_tsigkey_createfromkey( name, &tkeyin->algorithm, dstkey, true, false, - principal, now, expire, ring->mctx, ring, &tsigkey)); + principal, now, expire, ring->mctx, &tsigkey)); + RETERR(dns_tsigkeyring_add(ring, name, tsigkey)); dst_key_free(&dstkey); tkeyout->inception = now; tkeyout->expire = expire; @@ -732,13 +733,14 @@ dns_tkey_gssnegotiate(dns_message_t *qmsg, dns_message_t *rmsg, dns_tsigkey_t **outkey, dns_tsigkeyring_t *ring, char **err_message) { dns_rdata_t rtkeyrdata = DNS_RDATA_INIT, qtkeyrdata = DNS_RDATA_INIT; - dns_name_t *tkeyname; + dns_name_t *tkeyname = NULL; dns_rdata_tkey_t rtkey, qtkey, tkey; isc_buffer_t intoken, outtoken; dst_key_t *dstkey = NULL; isc_result_t result; unsigned char array[TEMP_BUFFER_SZ]; bool freertkey = false; + dns_tsigkey_t *tsigkey = NULL; REQUIRE(qmsg != NULL); REQUIRE(rmsg != NULL); @@ -814,9 +816,16 @@ dns_tkey_gssnegotiate(dns_message_t *qmsg, dns_message_t *rmsg, * anything yet. */ - RETERR(dns_tsigkey_createfromkey( - tkeyname, DNS_TSIG_GSSAPI_NAME, dstkey, true, false, NULL, - rtkey.inception, rtkey.expire, ring->mctx, ring, outkey)); + RETERR(dns_tsigkey_createfromkey(tkeyname, DNS_TSIG_GSSAPI_NAME, dstkey, + true, false, NULL, rtkey.inception, + rtkey.expire, ring->mctx, &tsigkey)); + RETERR(dns_tsigkeyring_add(ring, tkeyname, tsigkey)); + if (outkey == NULL) { + dns_tsigkey_detach(&tsigkey); + } else { + *outkey = tsigkey; + } + dst_key_free(&dstkey); dns_rdata_freestruct(&rtkey); return (result); @@ -825,6 +834,9 @@ failure: /* * XXXSRA This probably leaks memory from qtkey. */ + if (tsigkey != NULL) { + dns_tsigkey_detach(&tsigkey); + } if (freertkey) { dns_rdata_freestruct(&rtkey); } diff --git a/lib/dns/tsig.c b/lib/dns/tsig.c index f011f57690..f5c4a149d5 100644 --- a/lib/dns/tsig.c +++ b/lib/dns/tsig.c @@ -39,12 +39,11 @@ #include "tsig_p.h" -#define TSIG_MAGIC ISC_MAGIC('T', 'S', 'I', 'G') -#define VALID_TSIG_KEY(x) ISC_MAGIC_VALID(x, TSIG_MAGIC) +#define TSIGKEYRING_MAGIC ISC_MAGIC('T', 'K', 'R', 'g') +#define VALID_TSIGKEYRING(x) ISC_MAGIC_VALID(x, TSIGKEYRING_MAGIC) -#ifndef DNS_TSIG_MAXGENERATEDKEYS -#define DNS_TSIG_MAXGENERATEDKEYS 4096 -#endif /* ifndef DNS_TSIG_MAXGENERATEDKEYS */ +#define TSIG_MAGIC ISC_MAGIC('T', 'S', 'I', 'G') +#define VALID_TSIGKEY(x) ISC_MAGIC_VALID(x, TSIG_MAGIC) #define is_response(msg) ((msg->flags & DNS_MESSAGEFLAG_QR) != 0) @@ -160,6 +159,9 @@ tsig_log(dns_tsigkey_t *key, int level, const char *fmt, ...) { static void remove_fromring(dns_tsigkey_t *tkey) { + REQUIRE(VALID_TSIGKEY(tkey)); + REQUIRE(VALID_TSIGKEYRING(tkey->ring)); + if (tkey->generated && ISC_LINK_LINKED(tkey, link)) { ISC_LIST_UNLINK(tkey->ring->lru, tkey, link); tkey->ring->generated--; @@ -190,22 +192,20 @@ dns_tsigkey_createfromkey(const dns_name_t *name, const dns_name_t *algorithm, dst_key_t *dstkey, bool generated, bool restored, const dns_name_t *creator, isc_stdtime_t inception, isc_stdtime_t expire, isc_mem_t *mctx, - dns_tsigkeyring_t *ring, dns_tsigkey_t **keyp) { + dns_tsigkey_t **keyp) { dns_tsigkey_t *tkey = NULL; isc_result_t ret; unsigned int dstalg = 0; - REQUIRE(keyp == NULL || *keyp == NULL); + REQUIRE(keyp != NULL && *keyp == NULL); REQUIRE(name != NULL); REQUIRE(algorithm != NULL); REQUIRE(mctx != NULL); - REQUIRE(keyp != NULL || ring != NULL); tkey = isc_mem_get(mctx, sizeof(dns_tsigkey_t)); *tkey = (dns_tsigkey_t){ .generated = generated, .restored = restored, - .ring = ring, .inception = inception, .expire = expire, .name = DNS_NAME_INITEMPTY, @@ -253,21 +253,6 @@ dns_tsigkey_createfromkey(const dns_name_t *name, const dns_name_t *algorithm, isc_refcount_init(&tkey->references, 1); isc_mem_attach(mctx, &tkey->mctx); - if (ring != NULL) { - ret = dns_tsigkeyring_add(ring, name, tkey); - if (ret != ISC_R_SUCCESS) { - goto cleanup_refs; - } - - /* - * If we won't be passing the key back to the caller - * then we we can release a reference now. - */ - if (keyp == NULL) { - dns_tsigkey_unref(tkey); - } - } - /* * Ignore this if it's a GSS key, since the key size is meaningless. */ @@ -297,24 +282,6 @@ dns_tsigkey_createfromkey(const dns_name_t *name, const dns_name_t *algorithm, } return (ISC_R_SUCCESS); -cleanup_refs: - tkey->magic = 0; - isc_refcount_destroy(&tkey->references); - - if (tkey->key != NULL) { - dst_key_free(&tkey->key); - } - if (tkey->creator != NULL) { - dns_name_free(tkey->creator, mctx); - isc_mem_put(mctx, tkey->creator, sizeof(dns_name_t)); - } - if (dns__tsig_algallocated(tkey->algorithm)) { - dns_name_t *tmpname = UNCONST(tkey->algorithm); - if (dns_name_dynamic(tmpname)) { - dns_name_free(tmpname, mctx); - } - isc_mem_put(mctx, tmpname, sizeof(dns_name_t)); - } cleanup_name: dns_name_free(&tkey->name, mctx); isc_mem_put(mctx, tkey, sizeof(dns_tsigkey_t)); @@ -377,6 +344,7 @@ again: static void destroyring(dns_tsigkeyring_t *ring) { + ring->magic = 0; isc_refcount_destroy(&ring->references); dns_rbt_destroy(&ring->keys); isc_rwlock_destroy(&ring->lock); @@ -457,6 +425,7 @@ restore_key(dns_tsigkeyring_t *ring, isc_stdtime_t now, FILE *fp) { dns_fixedname_t fname, fcreator, falgorithm; isc_result_t result; unsigned int dstalg; + dns_tsigkey_t *tkey = NULL; n = fscanf(fp, "%1023s %1023s %u %u %1023s %4095s\n", namestr, creatorstr, &inception, &expire, algorithmstr, keystr); @@ -509,7 +478,11 @@ restore_key(dns_tsigkeyring_t *ring, isc_stdtime_t now, FILE *fp) { result = dns_tsigkey_createfromkey(name, algorithm, dstkey, true, true, creator, inception, expire, - ring->mctx, ring, NULL); + ring->mctx, &tkey); + if (result == ISC_R_SUCCESS) { + result = dns_tsigkeyring_add(ring, name, tkey); + } + dns_tsigkey_detach(&tkey); if (dstkey != NULL) { dst_key_free(&dstkey); } @@ -543,23 +516,15 @@ dump_key(dns_tsigkey_t *tkey, FILE *fp) { } isc_result_t -dns_tsigkeyring_dumpanddetach(dns_tsigkeyring_t **ringp, FILE *fp) { +dns_tsigkeyring_dump(dns_tsigkeyring_t *ring, FILE *fp) { isc_result_t result; dns_rbtnodechain_t chain; dns_name_t foundname; dns_fixedname_t fixedorigin; dns_name_t *origin = NULL; isc_stdtime_t now = isc_stdtime_now(); - dns_tsigkeyring_t *ring = NULL; - REQUIRE(ringp != NULL && *ringp != NULL); - - ring = *ringp; - *ringp = NULL; - - if (isc_refcount_decrement(&ring->references) > 1) { - return (DNS_R_CONTINUE); - } + REQUIRE(VALID_TSIGKEYRING(ring)); dns_name_init(&foundname, NULL); origin = dns_fixedname_initname(&fixedorigin); @@ -590,13 +555,12 @@ dns_tsigkeyring_dumpanddetach(dns_tsigkeyring_t **ringp, FILE *fp) { } destroy: - destroyring(ring); return (result); } const dns_name_t * dns_tsigkey_identity(const dns_tsigkey_t *tsigkey) { - REQUIRE(tsigkey == NULL || VALID_TSIG_KEY(tsigkey)); + REQUIRE(tsigkey == NULL || VALID_TSIGKEY(tsigkey)); if (tsigkey == NULL) { return (NULL); @@ -641,7 +605,7 @@ dns_tsigkey_create(const dns_name_t *name, const dns_name_t *algorithm, } result = dns_tsigkey_createfromkey(name, algorithm, dstkey, false, - false, NULL, 0, 0, mctx, NULL, key); + false, NULL, 0, 0, mctx, key); if (dstkey != NULL) { dst_key_free(&dstkey); } @@ -650,7 +614,7 @@ dns_tsigkey_create(const dns_name_t *name, const dns_name_t *algorithm, static void destroy_tsigkey(dns_tsigkey_t *key) { - REQUIRE(VALID_TSIG_KEY(key)); + REQUIRE(VALID_TSIGKEY(key)); key->magic = 0; dns_name_free(&key->name, key->mctx); @@ -677,8 +641,7 @@ ISC_REFCOUNT_IMPL(dns_tsigkey, destroy_tsigkey); void dns_tsigkey_delete(dns_tsigkey_t *key) { - REQUIRE(VALID_TSIG_KEY(key)); - REQUIRE(key->ring != NULL); + REQUIRE(VALID_TSIGKEY(key)); RWLOCK(&key->ring->lock, isc_rwlocktype_write); remove_fromring(key); @@ -707,7 +670,7 @@ dns_tsig_sign(dns_message_t *msg) { REQUIRE(msg != NULL); key = dns_message_gettsigkey(msg); - REQUIRE(VALID_TSIG_KEY(key)); + REQUIRE(VALID_TSIGKEY(key)); /* * If this is a response, there should be a TSIG in the query with the @@ -1009,7 +972,7 @@ dns_tsig_verify(isc_buffer_t *source, dns_message_t *msg, tsigkey = dns_message_gettsigkey(msg); response = is_response(msg); - REQUIRE(tsigkey == NULL || VALID_TSIG_KEY(tsigkey)); + REQUIRE(tsigkey == NULL || VALID_TSIGKEY(tsigkey)); msg->verify_attempted = 1; msg->verified_sig = 0; @@ -1667,10 +1630,9 @@ dns_tsigkey_find(dns_tsigkey_t **tsigkey, const dns_name_t *name, isc_stdtime_t now = isc_stdtime_now(); isc_result_t result; - REQUIRE(tsigkey != NULL); - REQUIRE(*tsigkey == NULL); REQUIRE(name != NULL); - REQUIRE(ring != NULL); + REQUIRE(VALID_TSIGKEYRING(ring)); + REQUIRE(tsigkey != NULL && *tsigkey == NULL); RWLOCK(&ring->lock, isc_rwlocktype_write); cleanup_ring(ring); @@ -1709,7 +1671,7 @@ free_tsignode(void *node, void *arg ISC_ATTR_UNUSED) { REQUIRE(key != NULL); - if (key->generated && ISC_LINK_LINKED(key, link)) { + if (key->ring != NULL && key->generated && ISC_LINK_LINKED(key, link)) { ISC_LIST_UNLINK(key->ring->lru, key, link); dns_tsigkey_unref(key); } @@ -1722,12 +1684,10 @@ dns_tsigkeyring_create(isc_mem_t *mctx, dns_tsigkeyring_t **ringp) { dns_tsigkeyring_t *ring = NULL; REQUIRE(mctx != NULL); - REQUIRE(ringp != NULL); - REQUIRE(*ringp == NULL); + REQUIRE(ringp != NULL && *ringp == NULL); ring = isc_mem_get(mctx, sizeof(dns_tsigkeyring_t)); *ring = (dns_tsigkeyring_t){ - .maxgenerated = DNS_TSIG_MAXGENERATEDKEYS, .lru = ISC_LIST_INITIALIZER, }; @@ -1741,6 +1701,7 @@ dns_tsigkeyring_create(isc_mem_t *mctx, dns_tsigkeyring_t **ringp) { isc_rwlock_init(&ring->lock); isc_mem_attach(mctx, &ring->mctx); isc_refcount_init(&ring->references, 1); + ring->magic = TSIGKEYRING_MAGIC; *ringp = ring; return (ISC_R_SUCCESS); @@ -1751,8 +1712,8 @@ dns_tsigkeyring_add(dns_tsigkeyring_t *ring, const dns_name_t *name, dns_tsigkey_t *tkey) { isc_result_t result; - REQUIRE(VALID_TSIG_KEY(tkey)); - REQUIRE(tkey->ring == NULL); + REQUIRE(VALID_TSIGKEY(tkey)); + REQUIRE(VALID_TSIGKEYRING(ring)); REQUIRE(name != NULL); RWLOCK(&ring->lock, isc_rwlocktype_write); @@ -1770,6 +1731,7 @@ dns_tsigkeyring_add(dns_tsigkeyring_t *ring, const dns_name_t *name, result = dns_rbt_addname(ring->keys, name, tkey); if (result == ISC_R_SUCCESS) { dns_tsigkey_ref(tkey); + tkey->ring = ring; /* * If this is a TKEY-generated key, add it to the LRU list, @@ -1780,7 +1742,7 @@ dns_tsigkeyring_add(dns_tsigkeyring_t *ring, const dns_name_t *name, if (tkey->generated) { ISC_LIST_APPEND(ring->lru, tkey, link); dns_tsigkey_ref(tkey); - if (ring->generated++ > ring->maxgenerated) { + if (ring->generated++ > DNS_TSIG_MAXGENERATEDKEYS) { remove_fromring(ISC_LIST_HEAD(ring->lru)); } } diff --git a/lib/dns/view.c b/lib/dns/view.c index 5b3ea09e0d..7f1cc7ce8c 100644 --- a/lib/dns/view.c +++ b/lib/dns/view.c @@ -250,11 +250,8 @@ destroy(dns_view_t *view) { if (result == ISC_R_SUCCESS) { (void)isc_file_openuniqueprivate(template, &fp); } - if (fp == NULL) { - dns_tsigkeyring_detach(&view->dynamickeys); - } else { - result = dns_tsigkeyring_dumpanddetach( - &view->dynamickeys, fp); + if (fp != NULL) { + result = dns_tsigkeyring_dump(view->dynamickeys, fp); if (result == ISC_R_SUCCESS) { if (fclose(fp) == 0) { result = isc_file_sanitize( @@ -273,6 +270,7 @@ destroy(dns_view_t *view) { (void)remove(template); } } + dns_tsigkeyring_detach(&view->dynamickeys); } if (view->transports != NULL) { dns_transport_list_detach(&view->transports);