diff --git a/src/server_config.c b/src/server_config.c index db02b8a9..33b0f488 100644 --- a/src/server_config.c +++ b/src/server_config.c @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -124,6 +125,144 @@ nc_lyd_find_child_optional(const struct lyd_node *ctx_node, const char *child, s lyd_find_path(ctx_node, child, 0, match); } +/** + * @brief Get the instance a created or moved entry of an ordered-by user list should be placed after. + * + * The position is stored in the "key" metadata of the entry as a single key predicate + * (for example "[name='endpt']"), an empty value means the entry belongs first. + * + * @param[in] node List entry from the diff, the list is expected to have exactly one key. + * @param[out] anchor Name of the instance the entry should be placed after, NULL if it belongs first. + * Points into the metadata value, so it is not terminated, use @p anchor_len. + * @param[out] anchor_len Length of @p anchor. + * @param[out] move Whether the diff specifies a position at all. + * @return 0 on success, 1 on error. + */ +static int +nc_server_config_get_userord_anchor(const struct lyd_node *node, const char **anchor, uint32_t *anchor_len, int *move) +{ + struct lyd_meta *meta; + const char *val, *key_end, *val_end; + char quot; + + *anchor = NULL; + *anchor_len = 0; + *move = 0; + + meta = lyd_find_meta(node->meta, NULL, "yang:key"); + if (!meta) { + /* the diff says nothing about the position, for example because it was not created by libyang */ + return 0; + } + + *move = 1; + val = lyd_get_meta_value(meta); + if (!val[0]) { + /* the entry belongs first */ + return 0; + } + + key_end = strchr(val, '='); + if ((val[0] != '[') || !key_end) { + goto error; + } + + /* libyang always quotes the value with a quote it does not contain, so there is no escaping */ + quot = key_end[1]; + if ((quot != '\'') && (quot != '\"')) { + goto error; + } + + val_end = strchr(key_end + 2, quot); + if (!val_end || (val_end[1] != ']')) { + goto error; + } + + *anchor = key_end + 2; + *anchor_len = (uint32_t)(val_end - (key_end + 2)); + return 0; + +error: + ERR(NULL, "Invalid \"key\" metadata value \"%s\" of node \"%s\".", val, LYD_NAME(node)); + return 1; +} + +/** + * @brief Move an entry of an ordered-by user list to the position specified by the diff. + * + * The entries are stored in a sized array in the order they are configured in, so the item is + * moved within the array and the items in between are shifted. If the instance the entry should + * follow is not found, the entry is left where it is. + * + * @param[in] node List entry from the diff, the list is expected to have exactly one key. + * @param[in] index Current index of the entry in @p items. + * @param[in,out] items Sized array of the list entries. + * @param[in] item_size Size of a single item of @p items. + * @param[in] name_offset Offset of the entry name (the list key) within an item of @p items. + * @return 0 on success, 1 on error. + */ +static int +nc_server_config_move_userord_item(const struct lyd_node *node, LY_ARRAY_COUNT_TYPE index, void *items, + uint32_t item_size, uint32_t name_offset) +{ + int move = 0; + const char *anchor = NULL, *name; + uint32_t anchor_len = 0; + LY_ARRAY_COUNT_TYPE count, i, new_index; + char *item, *tmp; + + NC_CHECK_RET(nc_server_config_get_userord_anchor(node, &anchor, &anchor_len, &move)); + if (!move) { + /* keep the current position */ + return 0; + } + + count = LY_ARRAY_COUNT(items); + assert(index < count); + + if (!anchor) { + /* the entry belongs first */ + new_index = 0; + } else { + /* find the instance the entry should follow */ + for (i = 0; i < count; ++i) { + name = *(char **)((char *)items + (i * item_size) + name_offset); + if (!strncmp(name, anchor, anchor_len) && !name[anchor_len]) { + break; + } + } + if (i == count) { + WRN(NULL, "Instance \"%.*s\" to order the node \"%s\" after not found, keeping its position.", + (int)anchor_len, anchor, LYD_NAME(node)); + return 0; + } + + /* the entry is removed from its current position first, which shifts a following anchor down */ + new_index = (i < index) ? i + 1 : i; + } + + if (new_index == index) { + /* already in place */ + return 0; + } + + tmp = malloc(item_size); + NC_CHECK_ERRMEM_RET(!tmp, 1); + + item = (char *)items + (index * item_size); + memcpy(tmp, item, item_size); + if (new_index > index) { + memmove(item, item + item_size, (new_index - index) * item_size); + } else { + memmove((char *)items + ((new_index + 1) * item_size), (char *)items + (new_index * item_size), + (index - new_index) * item_size); + } + memcpy((char *)items + (new_index * item_size), tmp, item_size); + + free(tmp); + return 0; +} + #ifdef NC_ENABLED_SSH_TLS /** @@ -645,6 +784,9 @@ config_local_bind(const struct lyd_node *node, enum nc_operation parent_op, stru LY_ARRAY_NEW_RET(LYD_CTX(node), endpt->binds, bind, 1); /* init the new bind */ bind->sock = -1; + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config local address */ @@ -1001,8 +1143,9 @@ config_ssh_hostkey(const struct lyd_node *node, enum nc_operation parent_op, str name = lyd_get_value(n); assert(name); - if ((op == NC_OP_DELETE) || (op == NC_OP_NONE)) { - /* find the hostkey we are deleting/modifying */ + if ((op == NC_OP_DELETE) || (op == NC_OP_NONE) || (op == NC_OP_REPLACE)) { + /* find the hostkey we are deleting/modifying/moving, the list is ordered-by user so a moved + * entry is reported as replaced, but its contents do not change */ LY_ARRAY_FOR(ssh->hostkeys, i) { if (!strcmp(ssh->hostkeys[i].name, name)) { break; @@ -1016,6 +1159,10 @@ config_ssh_hostkey(const struct lyd_node *node, enum nc_operation parent_op, str } else if (op == NC_OP_CREATE) { /* create a new hostkey */ LY_ARRAY_NEW_RET(LYD_CTX(node), ssh->hostkeys, hostkey, 1); + i = LY_ARRAY_COUNT(ssh->hostkeys) - 1; + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config hostkey name */ @@ -1035,10 +1182,16 @@ config_ssh_hostkey(const struct lyd_node *node, enum nc_operation parent_op, str /* all children processed, we can now delete the hostkey */ if (op == NC_OP_DELETE) { + /* the list is ordered-by user, shift the rest instead of swapping the last hostkey in */ if (i < LY_ARRAY_COUNT(ssh->hostkeys) - 1) { - ssh->hostkeys[i] = ssh->hostkeys[LY_ARRAY_COUNT(ssh->hostkeys) - 1]; + memmove(&ssh->hostkeys[i], &ssh->hostkeys[i + 1], + (LY_ARRAY_COUNT(ssh->hostkeys) - i - 1) * sizeof *ssh->hostkeys); } LY_ARRAY_DECREMENT_FREE(ssh->hostkeys); + } else if ((op == NC_OP_CREATE) || (op == NC_OP_REPLACE)) { + /* the list is ordered-by user, place the hostkey at its configured position */ + NC_CHECK_RET(nc_server_config_move_userord_item(node, i, ssh->hostkeys, + (uint32_t)sizeof *ssh->hostkeys, (uint32_t)offsetof(struct nc_hostkey, name))); } return 0; @@ -1159,6 +1312,9 @@ config_ssh_user_public_key(const struct lyd_node *node, enum nc_operation parent } else if (op == NC_OP_CREATE) { /* create a new public key */ LY_ARRAY_NEW_RET(LYD_CTX(node), user->pubkeys, key, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config public key name */ @@ -1398,6 +1554,9 @@ config_ssh_user(const struct lyd_node *node, enum nc_operation parent_op, struct } else if (op == NC_OP_CREATE) { /* create a new user */ LY_ARRAY_NEW_RET(LYD_CTX(node), ssh->auth_clients, user, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config user name */ @@ -2236,6 +2395,9 @@ config_tls_client_auth_ca_cert(const struct lyd_node *node, } else if (op == NC_OP_CREATE) { /* create a new ca-cert */ LY_ARRAY_NEW_RET(LYD_CTX(node), client_auth->ca_certs, cert, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config ca-cert name */ @@ -2362,6 +2524,9 @@ config_tls_client_auth_ee_cert(const struct lyd_node *node, } else if (op == NC_OP_CREATE) { /* create a new ee-cert */ LY_ARRAY_NEW_RET(LYD_CTX(node), client_auth->ee_certs, cert, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config ee-cert name */ @@ -3246,6 +3411,9 @@ config_unix_user_mapping(const struct lyd_node *node, enum nc_operation parent_o } else if (op == NC_OP_CREATE) { /* create a new user mapping */ LY_ARRAY_NEW_RET(LYD_CTX(node), unix->user_mappings, mapping, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config system-user */ @@ -3385,6 +3553,9 @@ config_endpoint(const struct lyd_node *node, enum nc_operation parent_op, } else if (op == NC_OP_CREATE) { /* create a new endpoint */ LY_ARRAY_NEW_RET(LYD_CTX(node), config->endpts, endpt, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config name */ @@ -3724,8 +3895,9 @@ config_ch_client_endpoint(const struct lyd_node *node, enum nc_operation parent_ name = lyd_get_value(n); assert(name); - if ((op == NC_OP_DELETE) || (op == NC_OP_NONE)) { - /* find the endpoint we are deleting/modifying */ + if ((op == NC_OP_DELETE) || (op == NC_OP_NONE) || (op == NC_OP_REPLACE)) { + /* find the endpoint we are deleting/modifying/moving, the list is ordered-by user so a moved + * entry is reported as replaced, but its contents do not change */ LY_ARRAY_FOR(ch_client->ch_endpts, i) { if (!strcmp(ch_client->ch_endpts[i].name, name)) { break; @@ -3739,6 +3911,10 @@ config_ch_client_endpoint(const struct lyd_node *node, enum nc_operation parent_ } else if (op == NC_OP_CREATE) { /* create a new endpoint and init it */ LY_ARRAY_NEW_RET(LYD_CTX(node), ch_client->ch_endpts, endpt, 1); + i = LY_ARRAY_COUNT(ch_client->ch_endpts) - 1; + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config name */ @@ -3760,10 +3936,16 @@ config_ch_client_endpoint(const struct lyd_node *node, enum nc_operation parent_ /* all children processed, we can now delete the endpoint */ if (op == NC_OP_DELETE) { + /* the list is ordered-by user, shift the rest instead of swapping the last endpoint in */ if (i < LY_ARRAY_COUNT(ch_client->ch_endpts) - 1) { - ch_client->ch_endpts[i] = ch_client->ch_endpts[LY_ARRAY_COUNT(ch_client->ch_endpts) - 1]; + memmove(&ch_client->ch_endpts[i], &ch_client->ch_endpts[i + 1], + (LY_ARRAY_COUNT(ch_client->ch_endpts) - i - 1) * sizeof *ch_client->ch_endpts); } LY_ARRAY_DECREMENT_FREE(ch_client->ch_endpts); + } else if ((op == NC_OP_CREATE) || (op == NC_OP_REPLACE)) { + /* the list is ordered-by user, place the endpoint at its configured position */ + NC_CHECK_RET(nc_server_config_move_userord_item(node, i, ch_client->ch_endpts, + (uint32_t)sizeof *ch_client->ch_endpts, (uint32_t)offsetof(struct nc_ch_endpt, name))); } return 0; @@ -4004,6 +4186,9 @@ config_netconf_client(const struct lyd_node *node, enum nc_operation parent_op, } else if (op == NC_OP_CREATE) { /* create a new client */ LY_ARRAY_NEW_RET(LYD_CTX(node), config->ch_clients, ch_client, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config name */ @@ -4233,6 +4418,9 @@ config_asymmetric_key_cert(const struct lyd_node *node, enum nc_operation parent } else if (op == NC_OP_CREATE) { /* create a new certificate */ LY_ARRAY_NEW_RET(LYD_CTX(node), entry->certs, cert, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config certificate name */ @@ -4308,6 +4496,9 @@ config_asymmetric_key(const struct lyd_node *node, enum nc_operation parent_op, } else if (op == NC_OP_CREATE) { /* create a new asymmetric key entry */ LY_ARRAY_NEW_RET(LYD_CTX(node), keystore->entries, entry, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config asymmetric key name */ @@ -4530,6 +4721,9 @@ config_certificate_bag_cert(const struct lyd_node *node, enum nc_operation paren } else if (op == NC_OP_CREATE) { /* create a new certificate */ LY_ARRAY_NEW_RET(LYD_CTX(node), bag->certs, cert, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config certificate name */ @@ -4591,6 +4785,9 @@ config_certificate_bag(const struct lyd_node *node, enum nc_operation parent_op, } else if (op == NC_OP_CREATE) { /* create a new certificate bag */ LY_ARRAY_NEW_RET(LYD_CTX(node), truststore->cert_bags, bag, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config certificate bag name */ @@ -4725,6 +4922,9 @@ config_public_key_bag_pubkey(const struct lyd_node *node, enum nc_operation pare } else if (op == NC_OP_CREATE) { /* create a new public key */ LY_ARRAY_NEW_RET(LYD_CTX(node), bag->pubkeys, pubkey, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config public key name */ @@ -4786,6 +4986,9 @@ config_public_key_bag(const struct lyd_node *node, enum nc_operation parent_op, } else if (op == NC_OP_CREATE) { /* create a new public key bag */ LY_ARRAY_NEW_RET(LYD_CTX(node), truststore->pubkey_bags, bag, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config public key bag name */ @@ -5027,6 +5230,9 @@ config_cert_exp_notif_interval(const struct lyd_node *node, enum nc_operation pa } else if (op == NC_OP_CREATE) { /* create a new interval */ LY_ARRAY_NEW_RET(LYD_CTX(node), config->cert_exp_notif_intervals, interval, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config anchor */ @@ -5434,6 +5640,17 @@ nc_server_config_reconcile_chclients_dispatch(struct nc_server_config *old_cfg, if (dispatch_new_clients) { /* only dispatch if all required CBs are set */ LY_ARRAY_FOR(new_cfg->ch_clients, struct nc_ch_client, new_ch_client) { + if (!new_ch_client->thread) { + /* the new config may have been built from scratch (::nc_server_config_setup_data()), in which + * case the thread data of an already running client is only present in the old config */ + LY_ARRAY_FOR(old_cfg->ch_clients, struct nc_ch_client, old_ch_client) { + if (!strcmp(old_ch_client->name, new_ch_client->name)) { + new_ch_client->thread = old_ch_client->thread; + break; + } + } + } + if (new_ch_client->thread) { /* already running */ continue; @@ -6171,12 +6388,15 @@ nc_server_config_setup_diff(const struct lyd_node *data) * - avoids concurrent updates * - readers are still allowed to read the old config while we are applying the new one */ - if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { + ERR(NULL, "Timed out waiting for another configuration update to finish, " + "the new configuration was not applied."); return 1; } /* CONFIG RD LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { + ERR(NULL, "Timed out waiting for the configuration lock, the new configuration was not applied."); ret = 1; goto cleanup; } @@ -6207,7 +6427,8 @@ nc_server_config_setup_diff(const struct lyd_node *data) ERR(NULL, "Applying libnetconf2-netconf-server configuration failed."), cleanup); /* CONFIG WR LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { + ERR(NULL, "Timed out waiting for the configuration lock, the new configuration was not applied."); ret = 1; goto cleanup; } @@ -6260,7 +6481,9 @@ nc_server_config_setup_data(const struct lyd_node *data) * - avoids concurrent updates * - readers are still allowed to read the old config while we are applying the new one */ - if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { + ERR(NULL, "Timed out waiting for another configuration update to finish, " + "the new configuration was not applied."); return 1; } @@ -6300,7 +6523,8 @@ nc_server_config_setup_data(const struct lyd_node *data) ERR(NULL, "Applying libnetconf2-netconf-server configuration failed."), cleanup); /* CONFIG LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { + ERR(NULL, "Timed out waiting for the configuration lock, the new configuration was not applied."); ret = 1; goto cleanup; } diff --git a/src/session.c b/src/session.c index 0460716d..4e5e13ef 100644 --- a/src/session.c +++ b/src/session.c @@ -468,7 +468,11 @@ nc_rwlock_lock(pthread_rwlock_t *rwlock, enum nc_rwlock_mode mode, int timeout, if (ret) { if ((ret == EBUSY) || (ret == ETIMEDOUT)) { - /* timeout */ + /* timeout, a trylock (no timeout) being busy is a normal outcome, an expired deadline is not */ + if (timeout > 0) { + ERR(NULL, "%s: timed out after %d ms waiting for the rwlock in %s mode.", func_name, timeout, + mode == NC_RWLOCK_READ ? "read" : "write"); + } return 0; } @@ -523,7 +527,10 @@ nc_mutex_lock(pthread_mutex_t *mutex, int timeout, const char *func_name) if (ret) { if ((ret == EBUSY) || (ret == ETIMEDOUT)) { - /* timeout */ + /* timeout, a trylock (no timeout) being busy is a normal outcome, an expired deadline is not */ + if (timeout > 0) { + ERR(NULL, "%s: timed out after %d ms waiting for the mutex.", func_name, timeout); + } return 0; } diff --git a/src/session_p.h b/src/session_p.h index 8f591a73..446e2be0 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -146,6 +146,16 @@ extern struct nc_server_opts server_opts; */ #define NC_CONFIG_LOCK_TIMEOUT 10000 +/** + * @brief Timeout in msec for the locks acquired while applying a new configuration. + * + * A reader may hold the config_lock for the whole duration of a transport handshake (TCP connect, + * SSH/TLS key exchange and authentication), which is far longer than ::NC_CONFIG_LOCK_TIMEOUT. + * Giving up here means losing the configuration change, which the caller generally cannot recover + * from, so wait much longer than any handshake can take. + */ +#define NC_CONFIG_APPLY_LOCK_TIMEOUT 300000 + /** * @brief Timeout in msec for acquiring session's ch_lock * (just simple session flag checks and updates) diff --git a/src/session_server.c b/src/session_server.c index f6cfaa83..374f7c44 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -1341,8 +1341,9 @@ nc_server_destroy(void) } #endif /* NC_ENABLED_SSH_TLS */ - /* CONFIG UPDATE LOCK */ - if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* CONFIG UPDATE LOCK - the same timeout as the appliers use, destroying the server must not + * fail just because a legitimate configuration apply is in progress */ + if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { rc = 1; goto cleanup; } @@ -1379,6 +1380,9 @@ nc_server_destroy(void) } server_opts.interactive_auth_data = NULL; server_opts.interactive_auth_data_free = NULL; + + /* Call Home dispatch data, its callback data does not have to be valid once the server is destroyed */ + memset(&server_opts.ch_dispatch_data, 0, sizeof server_opts.ch_dispatch_data); #endif /* NC_ENABLED_SSH_TLS */ /* hidden UNIX socket paths */ @@ -3882,11 +3886,13 @@ nc_session_server_ch_client_dispatch_stop(struct nc_ch_client *ch_client) goto cleanup; } - /* CONFIG WRITE LOCK - re-acquire to clear the thread pointer and free the thread data */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* CONFIG WRITE LOCK - re-acquire to clear the thread pointer and free the thread data, + * a reader may be holding the lock for the whole duration of a transport handshake */ + if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { /* if we fail, attempt to lock again in cleanup. * ch thread data will cause a memory leak, but we should avoid a possible crash this way */ - ERRINT; + ERR(NULL, "Timed out waiting for the configuration lock, Call Home client \"%s\" thread data leaked.", + ch_client_name); rc = 1; goto cleanup; } @@ -3906,7 +3912,7 @@ nc_session_server_ch_client_dispatch_stop(struct nc_ch_client *ch_client) cleanup: if (config_lock_mode == NC_RWLOCK_NONE) { /* CONFIG LOCK - lock it back if we unlocked it. It MUST succeed, if the caller holds the config apply mutex */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { ERRINT; } } diff --git a/tests/test_config.c b/tests/test_config.c index a62b7d10..96413d0f 100644 --- a/tests/test_config.c +++ b/tests/test_config.c @@ -22,6 +22,7 @@ #include #include #include +#include #include @@ -1049,6 +1050,457 @@ test_invalid_diff(void **state) lyd_free_all(diff); } +/** + * @brief Moving an entry of an ordered-by user list is reported as a replace operation, + * which must be handled and must not crash. + */ +static void +test_ordered_list_move(void **state) +{ + int ret; + struct lyd_node *tree = NULL, *diff = NULL; + struct ln2_test_ctx *test_ctx = *state; + char *mem = NULL, *mem_filled = NULL; + + /* a diff that only moves entries of the two ordered-by user lists, just like sysrepo + * reports it - the moved list entry is present with its keys only, without the new position */ + const char *move_diff = + "" + " " + " " + " " + " ssh" + " " + " " + " " + " " + " ssh-rsa" + " " + " " + " " + " " + " " + " " + " " + " " + " " + " persistent" + " " + " " + " tls" + " " + " " + " " + " " + "\n"; + + /* read the config file into memory */ + read_config_file(TESTS_DIR "/data/config.xml", &mem); + + /* print the port numbers into the config */ + ret = asprintf(&mem_filled, mem, TEST_PORT_STR, TEST_PORT_2_STR, TEST_PORT_3_STR, + TEST_PORT_4_STR, TEST_PORT_5_STR, TEST_PORT_6_STR); + assert_int_not_equal(ret, -1); + + ret = lyd_parse_data_mem(test_ctx->ctx, mem_filled, LYD_XML, LYD_PARSE_STRICT, LYD_VALIDATE_PRESENT, &tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_setup_data(tree); + assert_int_equal(ret, 0); + + /* the move must be handled, not crash and not fail */ + ret = lyd_parse_data_mem(test_ctx->ctx, move_diff, LYD_XML, LYD_PARSE_ONLY, 0, &diff); + assert_int_equal(ret, 0); + + ret = nc_server_config_setup_diff(diff); + assert_int_equal(ret, 0); + + lyd_free_all(diff); + lyd_free_all(tree); + free(mem); + free(mem_filled); +} + +/** + * @brief Time in seconds the client stalls in its password callback. + * + * The server waits for the authentication while holding the configuration READ lock, so this has to be + * longer than ::NC_CONFIG_LOCK_TIMEOUT (10 s) for the test to be meaningful. + */ +#define TEST_STALL_AUTH_SLEEP 13 + +/** @brief Time in seconds to wait for a Call Home client to report failed connection attempts. */ +#define TEST_CH_WATCH_TIME 4 + +/** @brief Maximum number of distinct Call Home threads the test keeps track of. */ +#define TEST_CH_TID_MAX 8 + +struct test_ch_threads { + pthread_mutex_t lock; + pthread_cond_t cond; + pthread_t tids[TEST_CH_TID_MAX]; + uint32_t tid_count; + char endpt[64]; +}; + +/* acquire ctx cb for the Call Home dispatch */ +static const struct ly_ctx * +test_ch_acquire_ctx_cb(void *cb_data) +{ + return ((struct ln2_test_ctx *)cb_data)->ctx; +} + +/* release ctx cb for the Call Home dispatch */ +static void +test_ch_release_ctx_cb(void *cb_data) +{ + (void) cb_data; +} + +/* new session cb for the Call Home dispatch, never actually called in these tests */ +static int +test_ch_new_session_cb(const char *client_name, struct nc_session *new_session, void *user_data) +{ + (void) client_name; + (void) new_session; + (void) user_data; + return 1; +} + +/** + * @brief Record the thread of every failed Call Home connection attempt. + * + * Called by the Call Home thread itself, so the number of distinct thread IDs seen is the number + * of Call Home threads running for the client. + */ +static void +test_ch_new_session_fail_cb(const char *client_name, const char *endpt_name, uint8_t max_attempts, + uint8_t cur_attempt, void *user_data) +{ + struct test_ch_threads *threads = user_data; + pthread_t self = pthread_self(); + uint32_t i; + + (void) client_name; + (void) max_attempts; + (void) cur_attempt; + + pthread_mutex_lock(&threads->lock); + if (!threads->endpt[0]) { + /* the endpoint of the very first failed attempt is the first one in the configuration */ + strncpy(threads->endpt, endpt_name, sizeof threads->endpt - 1); + } + for (i = 0; i < threads->tid_count; ++i) { + if (pthread_equal(threads->tids[i], self)) { + break; + } + } + if ((i == threads->tid_count) && (threads->tid_count < TEST_CH_TID_MAX)) { + threads->tids[threads->tid_count++] = self; + } + pthread_cond_broadcast(&threads->cond); + pthread_mutex_unlock(&threads->lock); +} + +/** + * @brief Create the YANG data of a Call Home endpoint that can never connect anywhere. + * + * @param[in] ctx libyang context. + * @param[in] client_name Name of the Call Home client. + * @param[in] endpt_name Name of the Call Home endpoint. + * @param[out] tree Created YANG data. + */ +static void +test_create_ch_endpt_data(const struct ly_ctx *ctx, const char *client_name, const char *endpt_name, + struct lyd_node **tree) +{ + int ret; + + /* port 1 is never listening, so the connection is refused immediately */ + ret = nc_server_config_add_ch_address_port(ctx, client_name, endpt_name, NC_TI_SSH, "127.0.0.1", "1", tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_add_ch_ssh_hostkey(ctx, client_name, endpt_name, "hostkey", + TESTS_DIR "/data/key_ecdsa", NULL, tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_add_ch_ssh_user_pubkey(ctx, client_name, endpt_name, "user", "pubkey", + TESTS_DIR "/data/id_ed25519.pub", tree); + assert_int_equal(ret, 0); +} + +/** + * @brief Create the YANG data of a Call Home client that can never connect anywhere. + * + * @param[in] ctx libyang context. + * @param[in] client_name Name of the Call Home client. + * @param[out] tree Created YANG data. + */ +static void +test_create_ch_client_data(const struct ly_ctx *ctx, const char *client_name, struct lyd_node **tree) +{ + int ret; + + test_create_ch_endpt_data(ctx, client_name, "endpt", tree); + + ret = nc_server_config_add_ch_persistent(ctx, client_name, tree); + assert_int_equal(ret, 0); + + /* retry quickly so that the test does not have to wait long */ + ret = nc_server_config_add_ch_reconnect_strategy(ctx, client_name, NC_CH_FIRST_LISTED, 1, 3, tree); + assert_int_equal(ret, 0); +} + +/** + * @brief Applying the whole configuration data again must not dispatch a second thread + * for an already running Call Home client. + */ +static void +test_ch_dispatch_not_duplicated(void **state) +{ + int ret; + uint32_t tid_count; + struct lyd_node *tree = NULL; + struct ln2_test_ctx *test_ctx = *state; + struct test_ch_threads threads = {0}; + struct timespec ts; + + pthread_mutex_init(&threads.lock, NULL); + pthread_cond_init(&threads.cond, NULL); + + nc_server_ch_set_dispatch_data(test_ch_acquire_ctx_cb, test_ch_release_ctx_cb, test_ctx, + test_ch_new_session_cb, NULL); + nc_server_ch_set_new_session_fail_cb(test_ch_new_session_fail_cb, &threads); + + test_create_ch_client_data(test_ctx->ctx, "ch", &tree); + + /* dispatch the Call Home client */ + ret = nc_server_config_setup_data(tree); + assert_int_equal(ret, 0); + + /* wait until its thread reports a failed connection attempt */ + pthread_mutex_lock(&threads.lock); + while (!threads.tid_count) { + clock_gettime(CLOCK_REALTIME, &ts); + ts.tv_sec += 10; + ret = pthread_cond_timedwait(&threads.cond, &threads.lock, &ts); + assert_int_equal(ret, 0); + } + pthread_mutex_unlock(&threads.lock); + + /* apply the very same data again, the client is already running */ + ret = nc_server_config_setup_data(tree); + assert_int_equal(ret, 0); + + /* give a duplicate thread enough time to report a failed connection attempt of its own */ + sleep(TEST_CH_WATCH_TIME); + + pthread_mutex_lock(&threads.lock); + tid_count = threads.tid_count; + pthread_mutex_unlock(&threads.lock); + + /* exactly one Call Home thread may be running for the client */ + assert_int_equal(tid_count, 1); + + lyd_free_all(tree); + pthread_cond_destroy(&threads.cond); + pthread_mutex_destroy(&threads.lock); +} + +/** + * @brief A moved entry of an ordered-by user list must actually change its position. + * + * The Call Home thread always starts with the first endpoint of the client, so the endpoint of the + * first failed connection attempt tells which one that is. + */ +static void +test_ch_endpoint_order(void **state) +{ + int ret; + struct lyd_node *tree = NULL, *diff = NULL; + struct ln2_test_ctx *test_ctx = *state; + struct test_ch_threads threads = {0}; + struct timespec ts; + + /* move the second endpoint to the front, an empty "key" means the entry belongs first */ + const char *move_diff = + "" + " " + " " + " ch" + " " + " " + " second" + " " + " " + " " + " " + "\n"; + + pthread_mutex_init(&threads.lock, NULL); + pthread_cond_init(&threads.cond, NULL); + + /* two endpoints, "first" is listed first */ + test_create_ch_endpt_data(test_ctx->ctx, "ch", "first", &tree); + test_create_ch_endpt_data(test_ctx->ctx, "ch", "second", &tree); + + ret = nc_server_config_add_ch_persistent(test_ctx->ctx, "ch", &tree); + assert_int_equal(ret, 0); + ret = nc_server_config_add_ch_reconnect_strategy(test_ctx->ctx, "ch", NC_CH_FIRST_LISTED, 1, 1, &tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_setup_data(tree); + assert_int_equal(ret, 0); + + ret = lyd_parse_data_mem(test_ctx->ctx, move_diff, LYD_XML, LYD_PARSE_ONLY, 0, &diff); + assert_int_equal(ret, 0); + ret = nc_server_config_setup_diff(diff); + assert_int_equal(ret, 0); + + /* dispatch the client only now, so that it starts with the reordered endpoints */ + nc_server_ch_set_new_session_fail_cb(test_ch_new_session_fail_cb, &threads); + ret = nc_connect_ch_client_dispatch("ch", test_ch_acquire_ctx_cb, test_ch_release_ctx_cb, test_ctx, + test_ch_new_session_cb, NULL); + assert_int_equal(ret, 0); + + /* wait for the first failed connection attempt */ + pthread_mutex_lock(&threads.lock); + while (!threads.endpt[0]) { + clock_gettime(CLOCK_REALTIME, &ts); + ts.tv_sec += 10; + ret = pthread_cond_timedwait(&threads.cond, &threads.lock, &ts); + assert_int_equal(ret, 0); + } + assert_string_equal(threads.endpt, "second"); + pthread_mutex_unlock(&threads.lock); + + lyd_free_all(diff); + lyd_free_all(tree); + pthread_cond_destroy(&threads.cond); + pthread_mutex_destroy(&threads.lock); +} + +/* password callback of the stalling client */ +static char * +test_stall_auth_password(const char *username, const char *hostname, void *priv) +{ + (void) username; + (void) hostname; + (void) priv; + + /* keep the server waiting for the authentication, it holds the configuration READ lock meanwhile */ + sleep(TEST_STALL_AUTH_SLEEP); + + /* a wrong password, the connection is expected to fail */ + return strdup("wrong"); +} + +static void * +test_stall_auth_client_thread(void *arg) +{ + int ret; + struct nc_session *session = NULL; + struct ln2_test_ctx *test_ctx = arg; + + /* skip all hostkey and known_hosts checks */ + nc_client_ssh_set_knownhosts_mode(NC_SSH_KNOWNHOSTS_SKIP); + + ret = nc_client_set_schema_searchpath(MODULES_DIR); + assert_int_equal(ret, 0); + + ret = nc_client_ssh_set_username("stall"); + assert_int_equal(ret, 0); + + nc_client_ssh_set_auth_password_clb(test_stall_auth_password, NULL); + + /* wait for the server to be ready */ + pthread_barrier_wait(&test_ctx->barrier); + + /* the authentication is stalled and then fails */ + session = nc_connect_ssh("127.0.0.1", TEST_PORT, NULL); + assert_null(session); + + return NULL; +} + +static void * +test_stall_auth_server_thread(void *arg) +{ + NC_MSG_TYPE msgtype; + struct nc_session *session = NULL; + struct ln2_test_ctx *test_ctx = arg; + + /* wait for the client to be ready to connect */ + pthread_barrier_wait(&test_ctx->barrier); + + /* the client never authenticates, so this is expected to fail after the stall */ + msgtype = nc_accept(NC_ACCEPT_TIMEOUT, test_ctx->ctx, &session); + assert_int_not_equal(msgtype, NC_MSG_HELLO); + nc_session_free(session, NULL); + + return NULL; +} + +/** + * @brief A configuration update must not be dropped because a session handshake is holding + * the configuration lock. + */ +static void +test_config_update_during_auth(void **state) +{ + int ret, i; + pthread_t tids[2]; + struct lyd_node *tree = NULL, *diff = NULL; + struct ln2_test_ctx *test_ctx = *state; + const struct lys_module *yang_mod; + + yang_mod = ly_ctx_get_module_implemented(test_ctx->ctx, "yang"); + assert_non_null(yang_mod); + + /* a listening SSH endpoint with a password-authenticated user */ + ret = nc_server_config_add_address_port(test_ctx->ctx, "endpt", NC_TI_SSH, "127.0.0.1", TEST_PORT, &tree); + assert_int_equal(ret, 0); + ret = nc_server_config_add_ssh_hostkey(test_ctx->ctx, "endpt", "hostkey", TESTS_DIR "/data/key_ecdsa", + NULL, &tree); + assert_int_equal(ret, 0); + ret = nc_server_config_add_ssh_user_password(test_ctx->ctx, "endpt", "stall", "correct", &tree); + assert_int_equal(ret, 0); + + /* add all the default nodes, the authentication timeout has to be longer than the stall */ + ret = lyd_new_implicit_tree(tree, LYD_IMPLICIT_NO_STATE, NULL); + assert_int_equal(ret, 0); + + ret = nc_server_config_setup_data(tree); + assert_int_equal(ret, 0); + + /* prepare the configuration update in advance so that only the lock wait is measured */ + test_create_ch_client_data(test_ctx->ctx, "ch", &diff); + ret = lyd_new_meta(test_ctx->ctx, diff, yang_mod, "operation", "create", 0, NULL); + assert_int_equal(ret, 0); + + ret = pthread_create(&tids[0], NULL, test_stall_auth_client_thread, test_ctx); + assert_int_equal(ret, 0); + ret = pthread_create(&tids[1], NULL, test_stall_auth_server_thread, test_ctx); + assert_int_equal(ret, 0); + + /* let the key exchange finish, the server is now waiting for the authentication + * while holding the configuration READ lock */ + sleep(2); + + /* this must not be silently dropped */ + ret = nc_server_config_setup_diff(diff); + assert_int_equal(ret, 0); + + for (i = 0; i < 2; i++) { + pthread_join(tids[i], NULL); + } + + lyd_free_all(diff); + lyd_free_all(tree); +} + static void test_config_data_free(void *data) { @@ -1103,6 +1555,10 @@ main(void) cmocka_unit_test_setup_teardown(test_config_cascade_delete, setup_f, ln2_glob_test_teardown), cmocka_unit_test_setup_teardown(test_unusupported_asymkey_format, setup_f, ln2_glob_test_teardown), cmocka_unit_test_setup_teardown(test_invalid_diff, setup_f, ln2_glob_test_teardown), + cmocka_unit_test_setup_teardown(test_ordered_list_move, setup_f, ln2_glob_test_teardown), + cmocka_unit_test_setup_teardown(test_ch_dispatch_not_duplicated, setup_f, ln2_glob_test_teardown), + cmocka_unit_test_setup_teardown(test_ch_endpoint_order, setup_f, ln2_glob_test_teardown), + cmocka_unit_test_setup_teardown(test_config_update_during_auth, setup_f, ln2_glob_test_teardown), }; /* try to get ports from the environment, otherwise use the default */