From 4f4d4021b4ed6a72f6cdecde7b58a2b5e7bfce2d Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Thu, 16 Dec 2021 16:34:37 +0100 Subject: [PATCH] ike: Treat action_t as flags so 'start' and 'trap' can be combined While combining the actions could cause duplicates (while the SA is initiated, traffic might trigger the trap and the initiation of another CHILD_SA), the previous commit should avoid most duplicates. If reuse_ikesa is disabled, duplicates can't be prevented, though. --- src/charon-nm/nm/nm_service.c | 4 +- .../backend/android_service.c | 4 +- src/libcharon/config/child_cfg.c | 8 +- src/libcharon/config/child_cfg.h | 10 +- src/libcharon/plugins/stroke/stroke_config.c | 4 +- src/libcharon/plugins/vici/vici_config.c | 196 +++++++++--------- .../processing/jobs/start_action_job.c | 50 +++-- src/libcharon/sa/ike_sa.c | 21 +- src/libcharon/sa/ikev1/tasks/quick_delete.c | 26 ++- src/libcharon/sa/ikev2/tasks/child_delete.c | 22 +- 10 files changed, 172 insertions(+), 173 deletions(-) diff --git a/src/charon-nm/nm/nm_service.c b/src/charon-nm/nm/nm_service.c index 6fae7bf9a3..09107a76b1 100644 --- a/src/charon-nm/nm/nm_service.c +++ b/src/charon-nm/nm/nm_service.c @@ -635,8 +635,8 @@ static gboolean connect_(NMVpnServicePlugin *plugin, NMConnection *connection, }, }, .mode = MODE_TUNNEL, - .dpd_action = ACTION_RESTART, - .close_action = ACTION_RESTART, + .dpd_action = ACTION_START, + .close_action = ACTION_START, }; /** diff --git a/src/frontends/android/app/src/main/jni/libandroidbridge/backend/android_service.c b/src/frontends/android/app/src/main/jni/libandroidbridge/backend/android_service.c index a99d039f8e..77decfd807 100644 --- a/src/frontends/android/app/src/main/jni/libandroidbridge/backend/android_service.c +++ b/src/frontends/android/app/src/main/jni/libandroidbridge/backend/android_service.c @@ -801,8 +801,8 @@ static job_requeue_t initiate(private_android_service_t *this) }, }, .mode = MODE_TUNNEL, - .dpd_action = ACTION_RESTART, - .close_action = ACTION_RESTART, + .dpd_action = ACTION_START, + .close_action = ACTION_START, }; char *type, *remote_id; diff --git a/src/libcharon/config/child_cfg.c b/src/libcharon/config/child_cfg.c index d4fb37cdca..9ea6186c3a 100644 --- a/src/libcharon/config/child_cfg.c +++ b/src/libcharon/config/child_cfg.c @@ -22,10 +22,10 @@ #include -ENUM(action_names, ACTION_NONE, ACTION_RESTART, - "clear", - "hold", - "restart", +ENUM_FLAGS(action_names, ACTION_TRAP, ACTION_START, + "none", + "trap", + "start", ); /** Default replay window size, if not set using charon.replay_window */ diff --git a/src/libcharon/config/child_cfg.h b/src/libcharon/config/child_cfg.h index b176d67bd9..4de978826e 100644 --- a/src/libcharon/config/child_cfg.h +++ b/src/libcharon/config/child_cfg.h @@ -40,11 +40,11 @@ typedef struct child_cfg_create_t child_cfg_create_t; */ enum action_t { /** No action */ - ACTION_NONE, - /** Route config to establish or reestablish on demand */ - ACTION_ROUTE, - /** Start or restart config immediately */ - ACTION_RESTART, + ACTION_NONE = 0, + /** Install trap policy to (re-)establish on demand */ + ACTION_TRAP = (1<<0), + /** Start or restart immediately */ + ACTION_START = (1<<1), }; /** diff --git a/src/libcharon/plugins/stroke/stroke_config.c b/src/libcharon/plugins/stroke/stroke_config.c index 175b6b5495..a15c9f5a45 100644 --- a/src/libcharon/plugins/stroke/stroke_config.c +++ b/src/libcharon/plugins/stroke/stroke_config.c @@ -1042,9 +1042,9 @@ static action_t map_action(int starter_action) switch (starter_action) { case 2: /* =hold */ - return ACTION_ROUTE; + return ACTION_TRAP; case 3: /* =restart */ - return ACTION_RESTART; + return ACTION_START; default: return ACTION_NONE; } diff --git a/src/libcharon/plugins/vici/vici_config.c b/src/libcharon/plugins/vici/vici_config.c index aa6fbafbeb..2c570c69b6 100644 --- a/src/libcharon/plugins/vici/vici_config.c +++ b/src/libcharon/plugins/vici/vici_config.c @@ -1004,10 +1004,10 @@ CALLBACK(parse_action, bool, action_t *out, chunk_t v) { enum_map_t map[] = { - { "start", ACTION_RESTART }, - { "restart", ACTION_RESTART }, - { "route", ACTION_ROUTE }, - { "trap", ACTION_ROUTE }, + { "start", ACTION_START }, + { "restart", ACTION_START }, + { "route", ACTION_TRAP }, + { "trap", ACTION_TRAP }, { "none", ACTION_NONE }, { "clear", ACTION_NONE }, }; @@ -2143,30 +2143,33 @@ CALLBACK(peer_sn, bool, static void run_start_action(private_vici_config_t *this, peer_cfg_t *peer_cfg, child_cfg_t *child_cfg) { - switch (child_cfg->get_start_action(child_cfg)) + action_t action; + + action = child_cfg->get_start_action(child_cfg); + + if (action & ACTION_TRAP) + { + DBG1(DBG_CFG, "installing '%s'", child_cfg->get_name(child_cfg)); + switch (child_cfg->get_mode(child_cfg)) + { + case MODE_PASS: + case MODE_DROP: + charon->shunts->install(charon->shunts, + peer_cfg->get_name(peer_cfg), child_cfg); + /* no need to check for ACTION_START */ + return; + default: + charon->traps->install(charon->traps, peer_cfg, child_cfg); + break; + } + } + + if (action & ACTION_START) { - case ACTION_RESTART: - DBG1(DBG_CFG, "initiating '%s'", child_cfg->get_name(child_cfg)); - charon->controller->initiate(charon->controller, + DBG1(DBG_CFG, "initiating '%s'", child_cfg->get_name(child_cfg)); + charon->controller->initiate(charon->controller, peer_cfg->get_ref(peer_cfg), child_cfg->get_ref(child_cfg), NULL, NULL, 0, FALSE); - break; - case ACTION_ROUTE: - DBG1(DBG_CFG, "installing '%s'", child_cfg->get_name(child_cfg)); - switch (child_cfg->get_mode(child_cfg)) - { - case MODE_PASS: - case MODE_DROP: - charon->shunts->install(charon->shunts, - peer_cfg->get_name(peer_cfg), child_cfg); - break; - default: - charon->traps->install(charon->traps, peer_cfg, child_cfg); - break; - } - break; - default: - break; } } @@ -2181,100 +2184,101 @@ static void clear_start_action(private_vici_config_t *this, char *peer_name, ike_sa_t *ike_sa; uint32_t id = 0, others; array_t *ids = NULL, *ikeids = NULL; + action_t action; char *name; name = child_cfg->get_name(child_cfg); + action = child_cfg->get_start_action(child_cfg); + + if (action & ACTION_TRAP) + { + DBG1(DBG_CFG, "uninstalling '%s'", name); + switch (child_cfg->get_mode(child_cfg)) + { + case MODE_PASS: + case MODE_DROP: + charon->shunts->uninstall(charon->shunts, peer_name, name); + /* no need to check for ACTION_START */ + return; + default: + charon->traps->uninstall(charon->traps, peer_name, name); + break; + } + } - switch (child_cfg->get_start_action(child_cfg)) + if (action & ACTION_START) { - case ACTION_RESTART: - enumerator = charon->controller->create_ike_sa_enumerator( + enumerator = charon->controller->create_ike_sa_enumerator( charon->controller, TRUE); - while (enumerator->enumerate(enumerator, &ike_sa)) + while (enumerator->enumerate(enumerator, &ike_sa)) + { + if (!streq(ike_sa->get_name(ike_sa), peer_name)) { - if (!streq(ike_sa->get_name(ike_sa), peer_name)) - { - continue; - } - others = id = 0; - children = ike_sa->create_child_sa_enumerator(ike_sa); - while (children->enumerate(children, &child_sa)) + continue; + } + others = id = 0; + children = ike_sa->create_child_sa_enumerator(ike_sa); + while (children->enumerate(children, &child_sa)) + { + if (child_sa->get_state(child_sa) != CHILD_DELETING && + child_sa->get_state(child_sa) != CHILD_DELETED) { - if (child_sa->get_state(child_sa) != CHILD_DELETING && - child_sa->get_state(child_sa) != CHILD_DELETED) + if (streq(name, child_sa->get_name(child_sa))) { - if (streq(name, child_sa->get_name(child_sa))) - { - id = child_sa->get_unique_id(child_sa); - } - else - { - others++; - } + id = child_sa->get_unique_id(child_sa); } - } - children->destroy(children); - - if (!ike_sa->get_child_count(ike_sa) || (id && !others)) - { - /* found no children or only matching, delete IKE_SA */ - id = ike_sa->get_unique_id(ike_sa); - array_insert_create_value(&ikeids, sizeof(id), - ARRAY_TAIL, &id); - } - else - { - children = ike_sa->create_child_sa_enumerator(ike_sa); - while (children->enumerate(children, &child_sa)) + else { - if (streq(name, child_sa->get_name(child_sa))) - { - id = child_sa->get_unique_id(child_sa); - array_insert_create_value(&ids, sizeof(id), - ARRAY_TAIL, &id); - } + others++; } - children->destroy(children); } } - enumerator->destroy(enumerator); + children->destroy(children); - if (array_count(ids)) + if (!ike_sa->get_child_count(ike_sa) || (id && !others)) { - while (array_remove(ids, ARRAY_HEAD, &id)) - { - DBG1(DBG_CFG, "closing '%s' #%u", name, id); - charon->controller->terminate_child(charon->controller, - id, NULL, NULL, 0); - } - array_destroy(ids); + /* found no children or only matching, delete IKE_SA */ + id = ike_sa->get_unique_id(ike_sa); + array_insert_create_value(&ikeids, sizeof(id), + ARRAY_TAIL, &id); } - if (array_count(ikeids)) + else { - while (array_remove(ikeids, ARRAY_HEAD, &id)) + children = ike_sa->create_child_sa_enumerator(ike_sa); + while (children->enumerate(children, &child_sa)) { - DBG1(DBG_CFG, "closing IKE_SA #%u", id); - charon->controller->terminate_ike(charon->controller, id, - FALSE, NULL, NULL, 0); + if (streq(name, child_sa->get_name(child_sa))) + { + id = child_sa->get_unique_id(child_sa); + array_insert_create_value(&ids, sizeof(id), + ARRAY_TAIL, &id); + } } - array_destroy(ikeids); + children->destroy(children); } - break; - case ACTION_ROUTE: - DBG1(DBG_CFG, "uninstalling '%s'", name); - switch (child_cfg->get_mode(child_cfg)) + } + enumerator->destroy(enumerator); + + if (array_count(ids)) + { + while (array_remove(ids, ARRAY_HEAD, &id)) { - case MODE_PASS: - case MODE_DROP: - charon->shunts->uninstall(charon->shunts, peer_name, name); - break; - default: - charon->traps->uninstall(charon->traps, peer_name, name); - break; + DBG1(DBG_CFG, "closing '%s' #%u", name, id); + charon->controller->terminate_child(charon->controller, + id, NULL, NULL, 0); } - break; - default: - break; + array_destroy(ids); + } + if (array_count(ikeids)) + { + while (array_remove(ikeids, ARRAY_HEAD, &id)) + { + DBG1(DBG_CFG, "closing IKE_SA #%u", id); + charon->controller->terminate_ike(charon->controller, id, + FALSE, NULL, NULL, 0); + } + array_destroy(ikeids); + } } } diff --git a/src/libcharon/processing/jobs/start_action_job.c b/src/libcharon/processing/jobs/start_action_job.c index 3a0ed879f5..31e154a770 100644 --- a/src/libcharon/processing/jobs/start_action_job.c +++ b/src/libcharon/processing/jobs/start_action_job.c @@ -42,8 +42,7 @@ METHOD(job_t, execute, job_requeue_t, enumerator_t *enumerator, *children; peer_cfg_t *peer_cfg; child_cfg_t *child_cfg; - ipsec_mode_t mode; - char *name; + action_t action; enumerator = charon->backends->create_peer_cfg_enumerator(charon->backends, NULL, NULL, NULL, NULL, IKE_ANY); @@ -52,34 +51,39 @@ METHOD(job_t, execute, job_requeue_t, children = peer_cfg->create_child_cfg_enumerator(peer_cfg); while (children->enumerate(children, &child_cfg)) { - name = child_cfg->get_name(child_cfg); + action = child_cfg->get_start_action(child_cfg); + if (action == ACTION_NONE) + { + continue; + } + + DBG1(DBG_JOB, "start action: %N '%s'", action_names, action, + child_cfg->get_name(child_cfg)); - switch (child_cfg->get_start_action(child_cfg)) + if (action & ACTION_TRAP) { - case ACTION_RESTART: - DBG1(DBG_JOB, "start action: initiate '%s'", name); - charon->controller->initiate(charon->controller, - peer_cfg->get_ref(peer_cfg), - child_cfg->get_ref(child_cfg), - NULL, NULL, 0, FALSE); - break; - case ACTION_ROUTE: - DBG1(DBG_JOB, "start action: route '%s'", name); - mode = child_cfg->get_mode(child_cfg); - if (mode == MODE_PASS || mode == MODE_DROP) - { + switch (child_cfg->get_mode(child_cfg)) + { + case MODE_PASS: + case MODE_DROP: charon->shunts->install(charon->shunts, peer_cfg->get_name(peer_cfg), child_cfg); - } - else - { + /* no need to check for ACTION_START */ + continue; + default: charon->traps->install(charon->traps, peer_cfg, child_cfg); - } - break; - case ACTION_NONE: - break; + break; + } + } + + if (action & ACTION_START) + { + charon->controller->initiate(charon->controller, + peer_cfg->get_ref(peer_cfg), + child_cfg->get_ref(child_cfg), + NULL, NULL, 0, FALSE); } } children->destroy(children); diff --git a/src/libcharon/sa/ike_sa.c b/src/libcharon/sa/ike_sa.c index 3ef5e020b1..a9ea02d955 100644 --- a/src/libcharon/sa/ike_sa.c +++ b/src/libcharon/sa/ike_sa.c @@ -2060,7 +2060,7 @@ static status_t reestablish_children(private_ike_sa_t *this, ike_sa_t *new, } if (force) { - action = ACTION_RESTART; + action = ACTION_START; } else { /* only restart CHILD_SAs that are configured accordingly */ @@ -2073,7 +2073,7 @@ static status_t reestablish_children(private_ike_sa_t *this, ike_sa_t *new, action = child_sa->get_dpd_action(child_sa); } } - if (action == ACTION_RESTART) + if (action & ACTION_START) { child_init_args_t args = { .reqid = child_sa->get_reqid(child_sa), @@ -2150,17 +2150,14 @@ METHOD(ike_sa_t, reestablish, status_t, { action = child_sa->get_dpd_action(child_sa); } - switch (action) + if (action & ACTION_TRAP) { - case ACTION_RESTART: - restart = TRUE; - break; - case ACTION_ROUTE: - charon->traps->install(charon->traps, this->peer_cfg, - child_sa->get_config(child_sa)); - break; - default: - break; + charon->traps->install(charon->traps, this->peer_cfg, + child_sa->get_config(child_sa)); + } + if (action & ACTION_START) + { + restart = TRUE; } } enumerator->destroy(enumerator); diff --git a/src/libcharon/sa/ikev1/tasks/quick_delete.c b/src/libcharon/sa/ikev1/tasks/quick_delete.c index ecfc8dda33..9d09ee42fd 100644 --- a/src/libcharon/sa/ikev1/tasks/quick_delete.c +++ b/src/libcharon/sa/ikev1/tasks/quick_delete.c @@ -152,23 +152,21 @@ static status_t delete_child(private_quick_delete_t *this, child_init_args_t args = { .reqid = child_sa->get_reqid(child_sa), }; + action_t action; + + action = child_sa->get_close_action(child_sa); child_cfg = child_sa->get_config(child_sa); child_cfg->get_ref(child_cfg); - - switch (child_sa->get_close_action(child_sa)) + if (action & ACTION_TRAP) { - case ACTION_RESTART: - child_cfg->get_ref(child_cfg); - status = this->ike_sa->initiate(this->ike_sa, child_cfg, - &args); - break; - case ACTION_ROUTE: - charon->traps->install(charon->traps, - this->ike_sa->get_peer_cfg(this->ike_sa), - child_cfg); - break; - default: - break; + charon->traps->install(charon->traps, + this->ike_sa->get_peer_cfg(this->ike_sa), + child_cfg); + } + if (action & ACTION_START) + { + child_cfg->get_ref(child_cfg); + status = this->ike_sa->initiate(this->ike_sa, child_cfg, &args); } child_cfg->destroy(child_cfg); } diff --git a/src/libcharon/sa/ikev2/tasks/child_delete.c b/src/libcharon/sa/ikev2/tasks/child_delete.c index 5ce251b5ff..13cbee3f5b 100644 --- a/src/libcharon/sa/ikev2/tasks/child_delete.c +++ b/src/libcharon/sa/ikev2/tasks/child_delete.c @@ -372,20 +372,16 @@ static status_t destroy_and_reestablish(private_child_delete_t *this) if (entry->check_delete_action) { /* enforce child_cfg policy if deleted passively */ - switch (action) + if (action & ACTION_TRAP) { - case ACTION_RESTART: - child_cfg->get_ref(child_cfg); - status = this->ike_sa->initiate(this->ike_sa, child_cfg, - &args); - break; - case ACTION_ROUTE: - charon->traps->install(charon->traps, - this->ike_sa->get_peer_cfg(this->ike_sa), - child_cfg); - break; - default: - break; + charon->traps->install(charon->traps, + this->ike_sa->get_peer_cfg(this->ike_sa), + child_cfg); + } + if (action & ACTION_START) + { + child_cfg->get_ref(child_cfg); + status = this->ike_sa->initiate(this->ike_sa, child_cfg, &args); } } child_cfg->destroy(child_cfg); -- 2.47.3