From: Tobias Brunner Date: Wed, 4 Sep 2019 14:25:18 +0000 (+0200) Subject: proposal: Extract proposal selection code in ike/child_cfg_t X-Git-Tag: 5.8.2dr2~19^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c9599d4101fccd39018fd43698ac8d7ec287e862;p=thirdparty%2Fstrongswan.git proposal: Extract proposal selection code in ike/child_cfg_t Also invert the PREFER_CONFIGURED flag (i.e. make it PREFER_SUPPLIED) so the default, without flags, is what we preferred so far. --- diff --git a/src/libcharon/config/child_cfg.c b/src/libcharon/config/child_cfg.c index 0dc8a742a6..fcc0efc25a 100644 --- a/src/libcharon/config/child_cfg.c +++ b/src/libcharon/config/child_cfg.c @@ -237,54 +237,7 @@ METHOD(child_cfg_t, select_proposal, proposal_t*, private_child_cfg_t*this, linked_list_t *proposals, proposal_selection_flag_t flags) { - enumerator_t *prefer_enum, *match_enum; - proposal_t *proposal, *match, *selected = NULL; - - if (flags & PROPOSAL_PREFER_CONFIGURED) - { - prefer_enum = this->proposals->create_enumerator(this->proposals); - match_enum = proposals->create_enumerator(proposals); - } - else - { - prefer_enum = proposals->create_enumerator(proposals); - match_enum = this->proposals->create_enumerator(this->proposals); - } - - while (prefer_enum->enumerate(prefer_enum, &proposal)) - { - if (flags & PROPOSAL_PREFER_CONFIGURED) - { - proposals->reset_enumerator(proposals, match_enum); - } - else - { - this->proposals->reset_enumerator(this->proposals, match_enum); - } - while (match_enum->enumerate(match_enum, &match)) - { - selected = proposal->select(proposal, match, flags); - if (selected) - { - DBG2(DBG_CFG, "received proposals: %#P", proposals); - DBG2(DBG_CFG, "configured proposals: %#P", this->proposals); - DBG1(DBG_CFG, "selected proposal: %P", selected); - break; - } - } - if (selected) - { - break; - } - } - prefer_enum->destroy(prefer_enum); - match_enum->destroy(match_enum); - if (!selected) - { - DBG1(DBG_CFG, "received proposals: %#P", proposals); - DBG1(DBG_CFG, "configured proposals: %#P", this->proposals); - } - return selected; + return proposal_select(this->proposals, proposals, flags); } METHOD(child_cfg_t, add_traffic_selector, void, diff --git a/src/libcharon/config/ike_cfg.c b/src/libcharon/config/ike_cfg.c index e6fd7e6ae3..c9971882df 100644 --- a/src/libcharon/config/ike_cfg.c +++ b/src/libcharon/config/ike_cfg.c @@ -344,54 +344,7 @@ METHOD(ike_cfg_t, select_proposal, proposal_t*, private_ike_cfg_t *this, linked_list_t *proposals, proposal_selection_flag_t flags) { - enumerator_t *prefer_enum, *match_enum; - proposal_t *proposal, *match, *selected = NULL; - - if (flags & PROPOSAL_PREFER_CONFIGURED) - { - prefer_enum = this->proposals->create_enumerator(this->proposals); - match_enum = proposals->create_enumerator(proposals); - } - else - { - prefer_enum = proposals->create_enumerator(proposals); - match_enum = this->proposals->create_enumerator(this->proposals); - } - - while (prefer_enum->enumerate(prefer_enum, (void**)&proposal)) - { - if (flags & PROPOSAL_PREFER_CONFIGURED) - { - proposals->reset_enumerator(proposals, match_enum); - } - else - { - this->proposals->reset_enumerator(this->proposals, match_enum); - } - while (match_enum->enumerate(match_enum, (void**)&match)) - { - selected = proposal->select(proposal, match, flags); - if (selected) - { - DBG2(DBG_CFG, "received proposals: %#P", proposals); - DBG2(DBG_CFG, "configured proposals: %#P", this->proposals); - DBG1(DBG_CFG, "selected proposal: %P", selected); - break; - } - } - if (selected) - { - break; - } - } - prefer_enum->destroy(prefer_enum); - match_enum->destroy(match_enum); - if (!selected) - { - DBG1(DBG_CFG, "received proposals: %#P", proposals); - DBG1(DBG_CFG, "configured proposals: %#P", this->proposals); - } - return selected; + return proposal_select(this->proposals, proposals, flags); } METHOD(ike_cfg_t, get_dh_group, diffie_hellman_group_t, diff --git a/src/libcharon/sa/ikev1/tasks/aggressive_mode.c b/src/libcharon/sa/ikev1/tasks/aggressive_mode.c index 8a10416554..6ad4a0f4b4 100644 --- a/src/libcharon/sa/ikev1/tasks/aggressive_mode.c +++ b/src/libcharon/sa/ikev1/tasks/aggressive_mode.c @@ -399,10 +399,10 @@ METHOD(task_t, process_r, status_t, } list = sa_payload->get_proposals(sa_payload); - if (lib->settings->get_bool(lib->settings, + if (!lib->settings->get_bool(lib->settings, "%s.prefer_configured_proposals", TRUE, lib->ns)) { - flags = PROPOSAL_PREFER_CONFIGURED; + flags = PROPOSAL_PREFER_SUPPLIED; } this->proposal = this->ike_cfg->select_proposal(this->ike_cfg, list, flags); @@ -644,8 +644,7 @@ METHOD(task_t, process_i, status_t, return send_notify(this, INVALID_PAYLOAD_TYPE); } list = sa_payload->get_proposals(sa_payload); - this->proposal = this->ike_cfg->select_proposal(this->ike_cfg, list, - PROPOSAL_PREFER_CONFIGURED); + this->proposal = this->ike_cfg->select_proposal(this->ike_cfg, list, 0); list->destroy_offset(list, offsetof(proposal_t, destroy)); if (!this->proposal) { diff --git a/src/libcharon/sa/ikev1/tasks/main_mode.c b/src/libcharon/sa/ikev1/tasks/main_mode.c index b1b9334a84..2678a1b4a3 100644 --- a/src/libcharon/sa/ikev1/tasks/main_mode.c +++ b/src/libcharon/sa/ikev1/tasks/main_mode.c @@ -390,10 +390,10 @@ METHOD(task_t, process_r, status_t, { flags |= PROPOSAL_ALLOW_PRIVATE; } - if (lib->settings->get_bool(lib->settings, + if (!lib->settings->get_bool(lib->settings, "%s.prefer_configured_proposals", TRUE, lib->ns)) { - flags |= PROPOSAL_PREFER_CONFIGURED; + flags |= PROPOSAL_PREFER_SUPPLIED; } this->proposal = this->ike_cfg->select_proposal(this->ike_cfg, list, flags); @@ -629,7 +629,7 @@ METHOD(task_t, process_i, status_t, linked_list_t *list; sa_payload_t *sa_payload; auth_method_t method; - proposal_selection_flag_t flags = PROPOSAL_PREFER_CONFIGURED; + proposal_selection_flag_t flags = 0; uint32_t lifetime; sa_payload = (sa_payload_t*)message->get_payload(message, diff --git a/src/libcharon/sa/ikev1/tasks/quick_mode.c b/src/libcharon/sa/ikev1/tasks/quick_mode.c index 6a104a7e03..edd179409a 100644 --- a/src/libcharon/sa/ikev1/tasks/quick_mode.c +++ b/src/libcharon/sa/ikev1/tasks/quick_mode.c @@ -1136,10 +1136,10 @@ METHOD(task_t, process_r, status_t, { flags |= PROPOSAL_ALLOW_PRIVATE; } - if (lib->settings->get_bool(lib->settings, + if (!lib->settings->get_bool(lib->settings, "%s.prefer_configured_proposals", TRUE, lib->ns)) { - flags |= PROPOSAL_PREFER_CONFIGURED; + flags |= PROPOSAL_PREFER_SUPPLIED; } this->proposal = this->config->select_proposal(this->config, list, flags); @@ -1345,7 +1345,7 @@ METHOD(task_t, process_i, status_t, { sa_payload_t *sa_payload; linked_list_t *list = NULL; - proposal_selection_flag_t flags = PROPOSAL_PREFER_CONFIGURED; + proposal_selection_flag_t flags = 0; sa_payload = (sa_payload_t*)message->get_payload(message, PLV1_SECURITY_ASSOCIATION); diff --git a/src/libcharon/sa/ikev2/tasks/child_create.c b/src/libcharon/sa/ikev2/tasks/child_create.c index ace796970f..533cf82c0f 100644 --- a/src/libcharon/sa/ikev2/tasks/child_create.c +++ b/src/libcharon/sa/ikev2/tasks/child_create.c @@ -568,10 +568,10 @@ static status_t select_and_install(private_child_create_t *this, { flags |= PROPOSAL_ALLOW_PRIVATE; } - if (lib->settings->get_bool(lib->settings, "%s.prefer_configured_proposals", - TRUE, lib->ns)) + if (!lib->settings->get_bool(lib->settings, + "%s.prefer_configured_proposals", TRUE, lib->ns)) { - flags |= PROPOSAL_PREFER_CONFIGURED; + flags |= PROPOSAL_PREFER_SUPPLIED; } this->proposal = this->config->select_proposal(this->config, this->proposals, flags); diff --git a/src/libcharon/sa/ikev2/tasks/ike_init.c b/src/libcharon/sa/ikev2/tasks/ike_init.c index bcf757bce6..143ae7bcf5 100644 --- a/src/libcharon/sa/ikev2/tasks/ike_init.c +++ b/src/libcharon/sa/ikev2/tasks/ike_init.c @@ -462,10 +462,10 @@ static void process_sa_payload(private_ike_init_t *this, message_t *message, { flags |= PROPOSAL_ALLOW_PRIVATE; } - if (lib->settings->get_bool(lib->settings, "%s.prefer_configured_proposals", - TRUE, lib->ns)) + if (!lib->settings->get_bool(lib->settings, + "%s.prefer_configured_proposals", TRUE, lib->ns)) { - flags |= PROPOSAL_PREFER_CONFIGURED; + flags |= PROPOSAL_PREFER_SUPPLIED; } this->proposal = ike_cfg->select_proposal(ike_cfg, proposal_list, flags); if (!this->proposal) diff --git a/src/libstrongswan/crypto/proposal/proposal.c b/src/libstrongswan/crypto/proposal/proposal.c index 807ddd083f..c880020de8 100644 --- a/src/libstrongswan/crypto/proposal/proposal.c +++ b/src/libstrongswan/crypto/proposal/proposal.c @@ -485,15 +485,15 @@ METHOD(proposal_t, select_proposal, proposal_t*, return NULL; } - if (flags & PROPOSAL_PREFER_CONFIGURED) + if (flags & PROPOSAL_PREFER_SUPPLIED) { - selected = proposal_create(this->protocol, other->get_number(other)); - selected->set_spi(selected, other->get_spi(other)); + selected = proposal_create(this->protocol, this->number); + selected->set_spi(selected, this->spi); } else { - selected = proposal_create(this->protocol, this->number); - selected->set_spi(selected, this->spi); + selected = proposal_create(this->protocol, other->get_number(other)); + selected->set_spi(selected, other->get_spi(other)); } if (!select_algos(this, other, selected, flags)) @@ -1346,3 +1346,59 @@ proposal_t *proposal_create_from_string(protocol_id_t protocol, const char *algs return &this->public; } + +/* + * Described in header + */ +proposal_t *proposal_select(linked_list_t *configured, linked_list_t *supplied, + proposal_selection_flag_t flags) +{ + enumerator_t *prefer_enum, *match_enum; + proposal_t *proposal, *match, *selected = NULL; + + if (flags & PROPOSAL_PREFER_SUPPLIED) + { + prefer_enum = supplied->create_enumerator(supplied); + match_enum = configured->create_enumerator(configured); + } + else + { + prefer_enum = configured->create_enumerator(configured); + match_enum = supplied->create_enumerator(supplied); + } + + while (prefer_enum->enumerate(prefer_enum, &proposal)) + { + if (flags & PROPOSAL_PREFER_SUPPLIED) + { + configured->reset_enumerator(configured, match_enum); + } + else + { + supplied->reset_enumerator(supplied, match_enum); + } + while (match_enum->enumerate(match_enum, &match)) + { + selected = proposal->select(proposal, match, flags); + if (selected) + { + DBG2(DBG_CFG, "received proposals: %#P", supplied); + DBG2(DBG_CFG, "configured proposals: %#P", configured); + DBG1(DBG_CFG, "selected proposal: %P", selected); + break; + } + } + if (selected) + { + break; + } + } + prefer_enum->destroy(prefer_enum); + match_enum->destroy(match_enum); + if (!selected) + { + DBG1(DBG_CFG, "received proposals: %#P", supplied); + DBG1(DBG_CFG, "configured proposals: %#P", configured); + } + return selected; +} diff --git a/src/libstrongswan/crypto/proposal/proposal.h b/src/libstrongswan/crypto/proposal/proposal.h index edf22d585c..8e952dd4f0 100644 --- a/src/libstrongswan/crypto/proposal/proposal.h +++ b/src/libstrongswan/crypto/proposal/proposal.h @@ -58,8 +58,8 @@ extern enum_name_t *protocol_id_names; enum proposal_selection_flag_t { /** Accept algorithms from a private range. */ PROPOSAL_ALLOW_PRIVATE = (1<<0), - /** Whether to prefer configured or supplied proposals. */ - PROPOSAL_PREFER_CONFIGURED = (1<<1), + /** Whether to prefer configured (default) or supplied proposals. */ + PROPOSAL_PREFER_SUPPLIED = (1<<1), /** Whether to skip and ignore diffie hellman groups. */ PROPOSAL_SKIP_DH = (1<<2), }; @@ -145,7 +145,7 @@ struct proposal_t { * compared. If they have at least one algorithm of each type * in common, a resulting proposal of this kind is created. * - * If the flag PROPOSAL_PREFER_CONFIGURED is set, other is expected to be + * Unless the flag PROPOSAL_PREFER_SUPPLIED is set, other is expected to be * the remote proposal from which to copy SPI and proposal number to the * result, otherwise copy from this proposal. * @@ -255,7 +255,19 @@ proposal_t *proposal_create_default_aead(protocol_id_t protocol); * @param algs algorithms as string * @return proposal_t object */ -proposal_t *proposal_create_from_string(protocol_id_t protocol, const char *algs); +proposal_t *proposal_create_from_string(protocol_id_t protocol, + const char *algs); + +/** + * Select a common proposal from the given lists of proposals. + * + * @param configured list of configured/local proposals + * @param supplied list of supplied/remote proposals + * @param flags flags to consider during proposal selection + * @return selected proposal, or NULL (allocated) + */ +proposal_t *proposal_select(linked_list_t *configured, linked_list_t *supplied, + proposal_selection_flag_t flags); /** * printf hook function for proposal_t. diff --git a/src/libstrongswan/tests/suites/test_proposal.c b/src/libstrongswan/tests/suites/test_proposal.c index 49014344f5..68116d92cd 100644 --- a/src/libstrongswan/tests/suites/test_proposal.c +++ b/src/libstrongswan/tests/suites/test_proposal.c @@ -126,8 +126,7 @@ START_TEST(test_select) select_data[_i].self); other = proposal_create_from_string(select_data[_i].proto, select_data[_i].other); - selected = self->select(self, other, - select_data[_i].flags | PROPOSAL_PREFER_CONFIGURED); + selected = self->select(self, other, select_data[_i].flags); if (select_data[_i].expected) { expected = proposal_create_from_string(select_data[_i].proto, @@ -155,12 +154,12 @@ START_TEST(test_select_spi) other = proposal_create_from_string(PROTO_ESP, "aes128-sha256-modp3072"); other->set_spi(other, 0x12345678); - selected = self->select(self, other, PROPOSAL_PREFER_CONFIGURED); + selected = self->select(self, other, 0); ck_assert(selected); ck_assert_int_eq(selected->get_spi(selected), other->get_spi(other)); selected->destroy(selected); - selected = self->select(self, other, 0); + selected = self->select(self, other, PROPOSAL_PREFER_SUPPLIED); ck_assert(selected); ck_assert_int_eq(selected->get_spi(selected), self->get_spi(self)); selected->destroy(selected); @@ -183,24 +182,98 @@ START_TEST(test_matches) ck_assert(self->matches(self, other, select_data[_i].flags)); ck_assert(other->matches(other, self, select_data[_i].flags)); ck_assert(self->matches(self, other, - select_data[_i].flags | PROPOSAL_PREFER_CONFIGURED)); + select_data[_i].flags | PROPOSAL_PREFER_SUPPLIED)); ck_assert(other->matches(other, self, - select_data[_i].flags | PROPOSAL_PREFER_CONFIGURED)); + select_data[_i].flags | PROPOSAL_PREFER_SUPPLIED)); } else { ck_assert(!self->matches(self, other, select_data[_i].flags)); ck_assert(!other->matches(other, self, select_data[_i].flags)); ck_assert(!self->matches(self, other, - select_data[_i].flags | PROPOSAL_PREFER_CONFIGURED)); + select_data[_i].flags | PROPOSAL_PREFER_SUPPLIED)); ck_assert(!other->matches(other, self, - select_data[_i].flags | PROPOSAL_PREFER_CONFIGURED)); + select_data[_i].flags | PROPOSAL_PREFER_SUPPLIED)); } other->destroy(other); self->destroy(self); } END_TEST +static struct { + protocol_id_t proto; + char *self[5]; + char *other[5]; + char *expected; + proposal_selection_flag_t flags; +} select_proposal_data[] = { + { PROTO_ESP, {}, {}, NULL }, + { PROTO_ESP, { "aes128" }, {}, NULL }, + { PROTO_ESP, {}, { "aes128" }, NULL }, + { PROTO_ESP, { "aes128" }, { "aes256" }, NULL }, + { PROTO_ESP, { "aes128" }, { "aes128" }, "aes128" }, + { PROTO_ESP, { "aes128", "aes256" }, { "aes256", "aes128" }, "aes128" }, + { PROTO_ESP, { "aes128", "aes256" }, { "aes256", "aes128" }, "aes256", + PROPOSAL_PREFER_SUPPLIED }, + { PROTO_ESP, { "aes128-modp1024", "aes256-modp1024" }, + { "aes256-modp2048", "aes128-modp2048" }, NULL }, + { PROTO_ESP, { "aes128-modp1024", "aes256-modp1024" }, + { "aes256-modp2048", "aes128-modp2048" }, "aes128", + PROPOSAL_SKIP_DH }, + { PROTO_ESP, { "aes128-modp1024", "aes256-modp1024" }, + { "aes256-modp2048", "aes128-modp2048" }, "aes256", + PROPOSAL_PREFER_SUPPLIED | PROPOSAL_SKIP_DH }, +}; + +START_TEST(test_select_proposal) +{ + linked_list_t *self, *other; + proposal_t *proposal, *selected, *expected; + int i; + + self = linked_list_create(); + other = linked_list_create(); + + for (i = 0; i < countof(select_proposal_data[_i].self); i++) + { + if (!select_proposal_data[_i].self[i]) + { + break; + } + proposal = proposal_create_from_string(select_proposal_data[_i].proto, + select_proposal_data[_i].self[i]); + self->insert_last(self, proposal); + } + for (i = 0; i < countof(select_proposal_data[_i].other); i++) + { + if (!select_proposal_data[_i].other[i]) + { + break; + } + proposal = proposal_create_from_string(select_proposal_data[_i].proto, + select_proposal_data[_i].other[i]); + other->insert_last(other, proposal); + } + selected = proposal_select(self, other, select_proposal_data[_i].flags); + if (select_proposal_data[_i].expected) + { + expected = proposal_create_from_string(select_proposal_data[_i].proto, + select_proposal_data[_i].expected); + ck_assert(selected); + ck_assert_msg(expected->equals(expected, selected), "proposal %P does " + "not match expected %P", selected, expected); + expected->destroy(expected); + } + else + { + ck_assert(!selected); + } + DESTROY_IF(selected); + other->destroy_offset(other, offsetof(proposal_t, destroy)); + self->destroy_offset(self, offsetof(proposal_t, destroy)); +} +END_TEST + START_TEST(test_promote_dh_group) { proposal_t *proposal; @@ -281,7 +354,7 @@ START_TEST(test_unknown_transform_types_select_fail) other = proposal_create_from_string(PROTO_IKE, "aes128-sha256-ecp256"); other->add_algorithm(other, 242, 42, 0); - selected = self->select(self, other, PROPOSAL_PREFER_CONFIGURED); + selected = self->select(self, other, 0); ck_assert(!selected); other->destroy(other); self->destroy(self); @@ -297,7 +370,7 @@ START_TEST(test_unknown_transform_types_select_fail_subtype) other = proposal_create_from_string(PROTO_IKE, "aes128-sha256-ecp256"); other->add_algorithm(other, 242, 42, 0); - selected = self->select(self, other, PROPOSAL_PREFER_CONFIGURED); + selected = self->select(self, other, 0); ck_assert(!selected); other->destroy(other); self->destroy(self); @@ -314,7 +387,7 @@ START_TEST(test_unknown_transform_types_select_success) other->add_algorithm(other, 242, 42, 128); other->add_algorithm(other, 242, 1, 0); - selected = self->select(self, other, PROPOSAL_PREFER_CONFIGURED); + selected = self->select(self, other, 0); ck_assert(selected); assert_proposal_eq(selected, "IKE:AES_CBC_128/HMAC_SHA2_256_128/PRF_HMAC_SHA2_256/ECP_256/UNKNOWN_242_42_128"); selected->destroy(selected); @@ -358,6 +431,11 @@ Suite *proposal_suite_create() tcase_add_loop_test(tc, test_matches, 0, countof(select_data)); suite_add_tcase(s, tc); + tc = tcase_create("select_proposal"); + tcase_add_loop_test(tc, test_select_proposal, 0, + countof(select_proposal_data)); + suite_add_tcase(s, tc); + tc = tcase_create("promote_dh_group"); tcase_add_test(tc, test_promote_dh_group); tcase_add_test(tc, test_promote_dh_group_already_front);