From b85007e0a6b3940ee95a96c08a9e96569b0963b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Thu, 24 Jan 2019 12:50:01 +0100 Subject: [PATCH] Move code handling key loading errors into a common function Some values returned by dstkey_fromconfig() indicate that key loading should be interrupted, others do not. There are also certain subsequent checks to be made after parsing a key from configuration and the results of these checks also affect the key loading process. All of this complicates the key loading logic. In order to make the relevant parts of the code easier to follow, reduce the body of the inner for loop in load_view_keys() to a single call to a new function, process_key(). Move dstkey_fromconfig() error handling to process_key() as well and add comments to clearly describe the effects of various key loading errors. --- bin/named/server.c | 164 +++++++++++++++++++++----------- bin/tests/system/mkeys/tests.sh | 6 +- 2 files changed, 112 insertions(+), 58 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index 64b49c9d83..461cf1d9cd 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -700,8 +700,8 @@ configure_view_nametable(const cfg_obj_t *vconfig, const cfg_obj_t *config, } static isc_result_t -dstkey_fromconfig(dns_view_t *view, const cfg_obj_t *vconfig, - const cfg_obj_t *key, bool managed, dst_key_t **target, +dstkey_fromconfig(const cfg_obj_t *vconfig, const cfg_obj_t *key, + bool managed, dst_key_t **target, const char **keynamestrp, isc_mem_t *mctx) { dns_rdataclass_t viewclass; @@ -720,12 +720,14 @@ dstkey_fromconfig(dns_view_t *view, const cfg_obj_t *vconfig, dst_key_t *dstkey = NULL; INSIST(target != NULL && *target == NULL); + INSIST(keynamestrp != NULL && *keynamestrp == NULL); flags = cfg_obj_asuint32(cfg_tuple_get(key, "flags")); proto = cfg_obj_asuint32(cfg_tuple_get(key, "protocol")); alg = cfg_obj_asuint32(cfg_tuple_get(key, "algorithm")); keyname = dns_fixedname_name(&fkeyname); keynamestr = cfg_obj_asstring(cfg_tuple_get(key, "name")); + *keynamestrp = keynamestr; if (managed) { const char *initmethod; @@ -796,39 +798,122 @@ dstkey_fromconfig(dns_view_t *view, const cfg_obj_t *vconfig, CHECK(dst_key_fromdns(keyname, viewclass, &rrdatabuf, mctx, &dstkey)); - if (!dns_resolver_algorithm_supported(view->resolver, keyname, alg)) { - cfg_obj_log(key, named_g_lctx, ISC_LOG_WARNING, - "%s key for '%s': algorithm is disabled", - managed ? "managed" : "trusted", keynamestr); - result = DST_R_UNSUPPORTEDALG; - goto cleanup; - } - *target = dstkey; return (ISC_R_SUCCESS); cleanup: - if (result == DST_R_NOCRYPTO) { + if (dstkey != NULL) { + dst_key_free(&dstkey); + } + + return (result); +} + +/*% + * Parse 'key' in the context of view configuration 'vconfig'. If successful, + * add the key to 'secroots' if both of the following conditions are true: + * + * - 'keyname_match' is NULL or it matches the owner name of 'key', + * - support for the algorithm used by 'key' is not disabled by 'resolver' + * for the owner name of 'key'. + * + * 'managed' is true for managed keys and false for trusted keys. 'mctx' is + * the memory context to use for allocating memory. + */ +static isc_result_t +process_key(const cfg_obj_t *key, const cfg_obj_t *vconfig, + dns_keytable_t *secroots, const dns_name_t *keyname_match, + dns_resolver_t *resolver, bool managed, isc_mem_t *mctx) +{ + const dns_name_t *keyname = NULL; + const char *keynamestr = NULL; + dst_key_t *dstkey = NULL; + unsigned int keyalg; + isc_result_t result; + + result = dstkey_fromconfig(vconfig, key, managed, &dstkey, &keynamestr, + mctx); + + switch (result) { + case ISC_R_SUCCESS: + /* + * Key was parsed correctly, its algorithm is supported by the + * crypto library, and it is not revoked. + */ + keyname = dst_key_name(dstkey); + keyalg = dst_key_alg(dstkey); + break; + case DST_R_UNSUPPORTEDALG: + case DST_R_BADKEYTYPE: + /* + * Key was parsed correctly, but it cannot be used; this is not + * a fatal error - log a warning about this key being ignored, + * but do not prevent any further ones from being processed. + */ + cfg_obj_log(key, named_g_lctx, ISC_LOG_WARNING, + "ignoring %s key for '%s': %s", + managed ? "managed" : "trusted", + keynamestr, isc_result_totext(result)); + return (ISC_R_SUCCESS); + case DST_R_NOCRYPTO: + /* + * Crypto support is not available. + */ cfg_obj_log(key, named_g_lctx, ISC_LOG_ERROR, "ignoring %s key for '%s': no crypto support", managed ? "managed" : "trusted", keynamestr); - } else if (result == DST_R_UNSUPPORTEDALG || - result == DST_R_BADKEYTYPE) { - cfg_obj_log(key, named_g_lctx, ISC_LOG_WARNING, - "skipping %s key for '%s': %s", - managed ? "managed" : "trusted", - keynamestr, isc_result_totext(result)); - } else { + return (result); + default: + /* + * Something unexpected happened; we have no choice but to + * indicate an error so that the configuration loading process + * is interrupted. + */ cfg_obj_log(key, named_g_lctx, ISC_LOG_ERROR, "configuring %s key for '%s': %s", managed ? "managed" : "trusted", keynamestr, isc_result_totext(result)); - result = ISC_R_FAILURE; + return (ISC_R_FAILURE); } - if (dstkey != NULL) + /* + * If the caller requested to only load keys for a specific name and + * the owner name of this key does not match the requested name, do not + * load it. + */ + if (keyname_match != NULL && !dns_name_equal(keyname_match, keyname)) { + goto done; + } + + /* + * Ensure that 'resolver' allows using the algorithm of this key for + * 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)) { + cfg_obj_log(key, named_g_lctx, ISC_LOG_WARNING, + "ignoring %s key for '%s': algorithm is disabled", + managed ? "managed" : "trusted", keynamestr); + goto done; + } + + /* + * Add the key to 'secroots'. This key is taken from the + * configuration, so if it's a managed key then it's an initializing + * key; that's why 'managed' is duplicated below. + */ + result = dns_keytable_add(secroots, managed, managed, &dstkey); + + 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); + } return (result); } @@ -844,8 +929,7 @@ load_view_keys(const cfg_obj_t *keys, const cfg_obj_t *vconfig, const dns_name_t *keyname, isc_mem_t *mctx) { const cfg_listelt_t *elt, *elt2; - const cfg_obj_t *key, *keylist; - dst_key_t *dstkey = NULL; + const cfg_obj_t *keylist; isc_result_t result; dns_keytable_t *secroots = NULL; @@ -861,43 +945,13 @@ load_view_keys(const cfg_obj_t *keys, const cfg_obj_t *vconfig, elt2 != NULL; elt2 = cfg_list_next(elt2)) { - key = cfg_listelt_value(elt2); - result = dstkey_fromconfig(view, vconfig, key, managed, - &dstkey, mctx); - if (result == DST_R_UNSUPPORTEDALG || - result == DST_R_BADKEYTYPE) { - result = ISC_R_SUCCESS; - continue; - } - if (result != ISC_R_SUCCESS) { - goto cleanup; - } - - /* - * If keyname was specified, we only add that key. - */ - if (keyname != NULL && - !dns_name_equal(keyname, dst_key_name(dstkey))) - { - dst_key_free(&dstkey); - continue; - } - - /* - * This key is taken from the configuration, so - * if it's a managed key then it's an - * initializing key; that's why 'managed' - * is duplicated below. - */ - CHECK(dns_keytable_add(secroots, managed, - managed, &dstkey)); + CHECK(process_key(cfg_listelt_value(elt2), vconfig, + secroots, keyname, view->resolver, + managed, mctx)); } } cleanup: - if (dstkey != NULL) { - dst_key_free(&dstkey); - } if (secroots != NULL) { dns_keytable_detach(&secroots); } diff --git a/bin/tests/system/mkeys/tests.sh b/bin/tests/system/mkeys/tests.sh index da6d2643eb..cfee9c89ff 100644 --- a/bin/tests/system/mkeys/tests.sh +++ b/bin/tests/system/mkeys/tests.sh @@ -763,12 +763,12 @@ rm -f ns6/managed-keys.bind* nextpart ns6/named.run > /dev/null $PERL $SYSTEMTESTTOP/start.pl --noclean --restart --port ${PORT} mkeys ns6 # log when an unsupported algorithm is encountered during startup -wait_for_log "skipping managed key for 'unsupported\.': algorithm is unsupported" ns6/named.run +wait_for_log "ignoring managed key for 'unsupported\.': algorithm is unsupported" ns6/named.run if [ $ret != 0 ]; then echo_i "failed"; fi status=`expr $status + $ret` n=`expr $n + 1` -echo_i "skipping unsupported algorithm in managed-keys ($n)" +echo_i "ignoring unsupported algorithm in managed-keys ($n)" ret=0 mkeys_status_on 6 > rndc.out.$n 2>&1 # there should still be only two keys listed (for . and rsasha256.) @@ -793,7 +793,7 @@ if [ $ret != 0 ]; then echo_i "failed"; fi status=`expr $status + $ret` n=`expr $n + 1` -echo_i "skipping unsupported algorithm in rollover ($n)" +echo_i "ignoring unsupported algorithm in rollover ($n)" ret=0 mkeys_reload_on 1 mkeys_refresh_on 6 -- GitLab