From 920545835539e32cda3691ba12b47a6fe5634329 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 1 Apr 2025 17:28:28 +0200 Subject: [PATCH] child-create: Use more generic method to pass information from previous SA Besides the previous key exchange method, this will allow us to also reuse the previous traffic selectors. Some data is still passed in separate methods as some are set even when there is no previous SA and others are not set in all cases. The interface for queue_child() now optionally takes the previous Child SA to handle both recreations and initiations from scratch. --- src/libcharon/sa/ike_sa.c | 12 +--- src/libcharon/sa/ikev1/task_manager_v1.c | 16 ++++- src/libcharon/sa/ikev2/task_manager_v2.c | 19 +++++- src/libcharon/sa/ikev2/tasks/child_create.c | 30 ++++++--- src/libcharon/sa/ikev2/tasks/child_create.h | 11 ++-- src/libcharon/sa/ikev2/tasks/child_delete.c | 68 ++++++++++----------- src/libcharon/sa/ikev2/tasks/child_rekey.c | 11 +--- src/libcharon/sa/task_manager.h | 3 +- 8 files changed, 100 insertions(+), 70 deletions(-) diff --git a/src/libcharon/sa/ike_sa.c b/src/libcharon/sa/ike_sa.c index 77e572182..3cfcdcf33 100644 --- a/src/libcharon/sa/ike_sa.c +++ b/src/libcharon/sa/ike_sa.c @@ -1628,7 +1628,7 @@ METHOD(ike_sa_t, initiate, status_t, if (child_cfg) { /* normal IKE_SA with CHILD_SA */ - this->task_manager->queue_child(this->task_manager, child_cfg, args); + this->task_manager->queue_child(this->task_manager, child_cfg, args, NULL); #ifdef ME if (this->peer_cfg->get_mediated_by(this->peer_cfg)) { @@ -2119,20 +2119,12 @@ static status_t reestablish_children(private_ike_sa_t *this, ike_sa_t *new, } if (action & ACTION_START) { - child_init_args_t args = { - .reqid = child_sa->get_reqid_ref(child_sa), - .label = child_sa->get_label(child_sa), - }; child_cfg = child_sa->get_config(child_sa); DBG1(DBG_IKE, "restarting CHILD_SA %s", child_cfg->get_name(child_cfg)); other->task_manager->queue_child(other->task_manager, child_cfg->get_ref(child_cfg), - &args); - if (args.reqid) - { - charon->kernel->release_reqid(charon->kernel, args.reqid); - } + NULL, child_sa); } } enumerator->destroy(enumerator); diff --git a/src/libcharon/sa/ikev1/task_manager_v1.c b/src/libcharon/sa/ikev1/task_manager_v1.c index 672a04927..5afc66ae8 100644 --- a/src/libcharon/sa/ikev1/task_manager_v1.c +++ b/src/libcharon/sa/ikev1/task_manager_v1.c @@ -1691,11 +1691,23 @@ METHOD(task_manager_t, queue_mobike, void, } METHOD(task_manager_t, queue_child, void, - private_task_manager_t *this, child_cfg_t *cfg, child_init_args_t *args) + private_task_manager_t *this, child_cfg_t *cfg, child_init_args_t *args, + child_sa_t *child_sa) { quick_mode_t *task; + uint32_t reqid; - if (args) + if (child_sa) + { + task = quick_mode_create(this->ike_sa, cfg, NULL, NULL, 0); + reqid = child_sa->get_reqid_ref(child_sa); + if (reqid) + { + task->use_reqid(task, reqid); + charon->kernel->release_reqid(charon->kernel, reqid); + } + } + else if (args) { task = quick_mode_create(this->ike_sa, cfg, args->src, args->dst, args->seq); diff --git a/src/libcharon/sa/ikev2/task_manager_v2.c b/src/libcharon/sa/ikev2/task_manager_v2.c index 2180bd909..d46559182 100644 --- a/src/libcharon/sa/ikev2/task_manager_v2.c +++ b/src/libcharon/sa/ikev2/task_manager_v2.c @@ -2197,6 +2197,7 @@ static void trigger_mbb_reauth(private_task_manager_t *this) cfg = child_sa->get_config(child_sa); child_create = child_create_create(new, cfg->get_ref(cfg), FALSE, NULL, NULL, 0); + child_create->recreate_sa(child_create, child_sa); reqid = child_sa->get_reqid_ref(child_sa); if (reqid) { @@ -2369,11 +2370,25 @@ METHOD(task_manager_t, queue_dpd, void, } METHOD(task_manager_t, queue_child, void, - private_task_manager_t *this, child_cfg_t *cfg, child_init_args_t *args) + private_task_manager_t *this, child_cfg_t *cfg, child_init_args_t *args, + child_sa_t *child_sa) { child_create_t *task; + uint32_t reqid; - if (args) + if (child_sa) + { + task = child_create_create(this->ike_sa, cfg, FALSE, NULL, NULL, 0); + task->recreate_sa(task, child_sa); + reqid = child_sa->get_reqid_ref(child_sa); + if (reqid) + { + task->use_reqid(task, reqid); + charon->kernel->release_reqid(charon->kernel, reqid); + } + task->use_label(task, child_sa->get_label(child_sa)); + } + else if (args) { task = child_create_create(this->ike_sa, cfg, FALSE, args->src, args->dst, args->seq); diff --git a/src/libcharon/sa/ikev2/tasks/child_create.c b/src/libcharon/sa/ikev2/tasks/child_create.c index be4662f1c..3020967fd 100644 --- a/src/libcharon/sa/ikev2/tasks/child_create.c +++ b/src/libcharon/sa/ikev2/tasks/child_create.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008-2024 Tobias Brunner + * Copyright (C) 2008-2025 Tobias Brunner * Copyright (C) 2005-2008 Martin Willi * Copyright (C) 2005 Jan Hutter * @@ -1574,7 +1574,11 @@ METHOD(task_t, build_i, status_t, return FAILED; } - if (!no_ke && !this->retry) + if (no_ke) + { /* we might have one set if we are recreating this SA */ + this->ke_method = KE_NONE; + } + else if (!this->retry) { /* during a rekeying the method might already be set */ if (this->ke_method == KE_NONE) { @@ -2080,7 +2084,7 @@ METHOD(task_t, build_r, status_t, return SUCCESS; } - /* check if ike_config_t included non-critical error notifies */ + /* check if ike_config_t task included non-critical error notifies */ enumerator = message->create_payload_enumerator(message); while (enumerator->enumerate(enumerator, &payload)) { @@ -2540,10 +2544,22 @@ METHOD(child_create_t, use_label, void, this->child.label = label ? label->clone(label) : NULL; } -METHOD(child_create_t, use_ke_method, void, - private_child_create_t *this, key_exchange_method_t ke_method) +METHOD(child_create_t, recreate_sa, void, + private_child_create_t *this, child_sa_t *old) { - this->ke_method = ke_method; + if (this->initiator) + { + proposal_t *proposal; + uint16_t ke_method; + + proposal = old->get_proposal(old); + if (proposal->get_algorithm(proposal, KEY_EXCHANGE_METHOD, + &ke_method, NULL)) + { + /* reuse the KE method negotiated previously */ + this->ke_method = ke_method; + } + } } METHOD(child_create_t, get_child, child_sa_t*, @@ -2719,7 +2735,7 @@ child_create_t *child_create_create(ike_sa_t *ike_sa, .use_marks = _use_marks, .use_if_ids = _use_if_ids, .use_label = _use_label, - .use_ke_method = _use_ke_method, + .recreate_sa = _recreate_sa, .abort = _abort_, .task = { .get_type = _get_type, diff --git a/src/libcharon/sa/ikev2/tasks/child_create.h b/src/libcharon/sa/ikev2/tasks/child_create.h index f3ff0cc7d..0c0e6137e 100644 --- a/src/libcharon/sa/ikev2/tasks/child_create.h +++ b/src/libcharon/sa/ikev2/tasks/child_create.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2018-2024 Tobias Brunner + * Copyright (C) 2018-2025 Tobias Brunner * Copyright (C) 2007 Martin Willi * * Copyright (C) secunet Security Networks AG @@ -81,13 +81,12 @@ struct child_create_t { void (*use_label)(child_create_t *this, sec_label_t *label); /** - * Initially propose a specific KE method to override configuration. + * Use data from the given old SA (e.g. KE method and traffic selectors) + * when rekeying/recreating it. * - * This is used during rekeying to prefer the previously negotiated method. - * - * @param ke_method KE method to use + * @param old old CHILD_SA that is getting rekeyed/recreated */ - void (*use_ke_method)(child_create_t *this, key_exchange_method_t ke_method); + void (*recreate_sa)(child_create_t *this, child_sa_t *old); /** * Get the lower of the two nonces, used for rekey collisions. diff --git a/src/libcharon/sa/ikev2/tasks/child_delete.c b/src/libcharon/sa/ikev2/tasks/child_delete.c index 3282a21f7..7987bdd53 100644 --- a/src/libcharon/sa/ikev2/tasks/child_delete.c +++ b/src/libcharon/sa/ikev2/tasks/child_delete.c @@ -151,6 +151,29 @@ static void conclude_rekeying(private_child_delete_t *this, child_sa_t *old) child_rekey_conclude_rekeying(old, child_sa); } +/** + * Queue a task to recreate the given CHILD_SA. + */ +static void queue_child_create(ike_sa_t *ike_sa, child_sa_t *child_sa) +{ + child_create_t *child_create; + child_cfg_t *child_cfg; + uint32_t reqid; + + child_cfg = child_sa->get_config(child_sa); + child_create = child_create_create(ike_sa, child_cfg->get_ref(child_cfg), + FALSE, NULL, NULL, 0); + child_create->recreate_sa(child_create, child_sa); + reqid = child_sa->get_reqid_ref(child_sa); + if (reqid) + { + child_create->use_reqid(child_create, reqid); + charon->kernel->release_reqid(charon->kernel, reqid); + } + child_create->use_label(child_create, child_sa->get_label(child_sa)); + ike_sa->queue_task(ike_sa, (task_t*)child_create); +} + /** * Destroy and optionally reestablish the given CHILD_SA according to config. */ @@ -160,12 +183,9 @@ static status_t destroy_and_reestablish_internal(ike_sa_t *ike_sa, bool delete_action, action_t forced_action) { - child_init_args_t args = {}; child_cfg_t *child_cfg; - protocol_id_t protocol; - uint32_t spi; action_t action; - status_t status = SUCCESS; + bool initiate = FALSE; child_sa->set_state(child_sa, CHILD_DELETED); if (trigger_updown) @@ -173,44 +193,29 @@ static status_t destroy_and_reestablish_internal(ike_sa_t *ike_sa, charon->bus->child_updown(charon->bus, child_sa, FALSE); } - protocol = child_sa->get_protocol(child_sa); - spi = child_sa->get_spi(child_sa, TRUE); - child_cfg = child_sa->get_config(child_sa); - child_cfg->get_ref(child_cfg); - args.reqid = child_sa->get_reqid_ref(child_sa); - args.label = child_sa->get_label(child_sa); - if (args.label) - { - args.label = args.label->clone(args.label); - } - action = forced_action ?: child_sa->get_close_action(child_sa); - DBG1(DBG_IKE, "CHILD_SA %s{%u} closed", child_sa->get_name(child_sa), child_sa->get_unique_id(child_sa)); - ike_sa->destroy_child_sa(ike_sa, protocol, spi); + action = forced_action ?: child_sa->get_close_action(child_sa); if (delete_action) { if (action & ACTION_TRAP) { + child_cfg = child_sa->get_config(child_sa); charon->traps->install(charon->traps, ike_sa->get_peer_cfg(ike_sa), - child_cfg); + child_cfg->get_ref(child_cfg)); } if (action & ACTION_START) { - child_cfg->get_ref(child_cfg); - status = ike_sa->initiate(ike_sa, child_cfg, &args); + queue_child_create(ike_sa, child_sa); + initiate = TRUE; } } - child_cfg->destroy(child_cfg); - if (args.reqid) - { - charon->kernel->release_reqid(charon->kernel, args.reqid); - } - DESTROY_IF(args.label); - return status; + ike_sa->destroy_child_sa(ike_sa, child_sa->get_protocol(child_sa), + child_sa->get_spi(child_sa, TRUE)); + return initiate ? ike_sa->initiate(ike_sa, NULL, NULL) : SUCCESS; } /* @@ -550,13 +555,8 @@ METHOD(task_t, build_i, status_t, if (this->expired) { - child_cfg_t *child_cfg; - - DBG1(DBG_IKE, "scheduling CHILD_SA recreate after hard expire"); - child_cfg = child_sa->get_config(child_sa); - this->ike_sa->queue_task(this->ike_sa, (task_t*) - child_create_create(this->ike_sa, child_cfg->get_ref(child_cfg), - FALSE, NULL, NULL, 0)); + DBG1(DBG_IKE, "queue CHILD_SA recreate after hard expire"); + queue_child_create(this->ike_sa, child_sa); } return NEED_MORE; } diff --git a/src/libcharon/sa/ikev2/tasks/child_rekey.c b/src/libcharon/sa/ikev2/tasks/child_rekey.c index 86161423b..fb4a724f4 100644 --- a/src/libcharon/sa/ikev2/tasks/child_rekey.c +++ b/src/libcharon/sa/ikev2/tasks/child_rekey.c @@ -269,20 +269,14 @@ METHOD(task_t, build_i, status_t, if (!this->child_create) { child_cfg_t *config; - proposal_t *proposal; - uint16_t ke_method; uint32_t reqid; config = this->child_sa->get_config(this->child_sa); this->child_create = child_create_create(this->ike_sa, config->get_ref(config), TRUE, NULL, NULL, 0); - proposal = this->child_sa->get_proposal(this->child_sa); - if (proposal->get_algorithm(proposal, KEY_EXCHANGE_METHOD, - &ke_method, NULL)) - { /* reuse the KE method negotiated previously */ - this->child_create->use_ke_method(this->child_create, ke_method); - } + this->child_create->recreate_sa(this->child_create, this->child_sa); + reqid = this->child_sa->get_reqid_ref(this->child_sa); if (reqid) { @@ -436,6 +430,7 @@ METHOD(task_t, build_r, status_t, if (message->get_exchange_type(message) == CREATE_CHILD_SA) { + this->child_create->recreate_sa(this->child_create, this->child_sa); reqid = this->child_sa->get_reqid_ref(this->child_sa); if (reqid) { diff --git a/src/libcharon/sa/task_manager.h b/src/libcharon/sa/task_manager.h index 69e5a812e..a30fab5a9 100644 --- a/src/libcharon/sa/task_manager.h +++ b/src/libcharon/sa/task_manager.h @@ -202,9 +202,10 @@ struct task_manager_t { * * @param cfg CHILD_SA config to establish * @param args optional arguments for the initiation + * @param child_sa optional CHILD_SA when recreating (instead of args) */ void (*queue_child)(task_manager_t *this, child_cfg_t *cfg, - child_init_args_t *args); + child_init_args_t *args, child_sa_t *child_sa); /** * Queue CHILD_SA rekeying tasks.