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 | 200 +++++++++--------- .../processing/jobs/start_action_job.c | 52 ++--- 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, 175 insertions(+), 176 deletions(-) diff --git a/src/charon-nm/nm/nm_service.c b/src/charon-nm/nm/nm_service.c index 6fae7bf9a..09107a76b 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 a99d039f8..77decfd80 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 d4fb37cdc..9ea6186c3 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 b176d67bd..4de978826 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 175b6b549..a15c9f5a4 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 aa6fbafbe..2c570c69b 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) { - case ACTION_RESTART: - DBG1(DBG_CFG, "initiating '%s'", child_cfg->get_name(child_cfg)); - charon->controller->initiate(charon->controller, + 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) + { + 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); - switch (child_cfg->get_start_action(child_cfg)) + if (action & ACTION_TRAP) { - case ACTION_RESTART: - enumerator = charon->controller->create_ike_sa_enumerator( + 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; + } + } + + if (action & ACTION_START) + { + 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)) + { + if (child_sa->get_state(child_sa) != CHILD_DELETING && + child_sa->get_state(child_sa) != CHILD_DELETED) { - continue; + if (streq(name, child_sa->get_name(child_sa))) + { + id = child_sa->get_unique_id(child_sa); + } + else + { + others++; + } } - others = id = 0; + } + 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)) { - 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); + array_insert_create_value(&ids, sizeof(id), + ARRAY_TAIL, &id); } } children->destroy(children); + } + } + enumerator->destroy(enumerator); - 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)) - { - 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); - } - } - children->destroy(children); - } - } - enumerator->destroy(enumerator); - - if (array_count(ids)) + if (array_count(ids)) + { + while (array_remove(ids, ARRAY_HEAD, &id)) { - 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); + DBG1(DBG_CFG, "closing '%s' #%u", name, id); + charon->controller->terminate_child(charon->controller, + id, NULL, NULL, 0); } - if (array_count(ikeids)) + array_destroy(ids); + } + if (array_count(ikeids)) + { + while (array_remove(ikeids, ARRAY_HEAD, &id)) { - 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); + DBG1(DBG_CFG, "closing IKE_SA #%u", id); + charon->controller->terminate_ike(charon->controller, id, + FALSE, NULL, NULL, 0); } - break; - case ACTION_ROUTE: - 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); - break; - default: - charon->traps->uninstall(charon->traps, peer_name, name); - break; - } - break; - default: - break; + array_destroy(ikeids); + } } } diff --git a/src/libcharon/processing/jobs/start_action_job.c b/src/libcharon/processing/jobs/start_action_job.c index 3a0ed879f..31e154a77 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); - - switch (child_cfg->get_start_action(child_cfg)) + action = child_cfg->get_start_action(child_cfg); + if (action == ACTION_NONE) { - 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) - { + continue; + } + + DBG1(DBG_JOB, "start action: %N '%s'", action_names, action, + child_cfg->get_name(child_cfg)); + + if (action & ACTION_TRAP) + { + 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 3ef5e020b..a9ea02d95 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 ecfc8dda3..9d09ee42f 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 5ce251b5f..13cbee3f5 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);