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 0dc8a74..fcc0efc 100644 (file)
--- 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 e6fd7e6..c997188 100644 (file)
--- 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 8a10416..6ad4a0f 100644 (file)
--- 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 b1b9334..2678a1b 100644 (file)
--- 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 6a104a7..edd1794 100644 (file)
--- 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 ace7969..533cf82 100644 (file)
--- 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 bcf757b..143ae7b 100644 (file)
--- 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 807ddd0..c880020 100644 (file)
--- 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 edf22d5..8e952dd 100644 (file)
--- 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 4901434..68116d9 100644 (file)
--- 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);