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.

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;
+	return proposal_select(this->proposals, proposals, flags);
-	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;
 }
 
 METHOD(child_cfg_t, add_traffic_selector, void,

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;
+	return proposal_select(this->proposals, proposals, flags);
-	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;
 }
 
 METHOD(ike_cfg_t, get_dh_group, diffie_hellman_group_t,

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,
+		this->proposal = this->ike_cfg->select_proposal(this->ike_cfg, list, 0);
-													PROPOSAL_PREFER_CONFIGURED);
 		list->destroy_offset(list, offsetof(proposal_t, destroy));
 		if (!this->proposal)
 		{

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,

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);

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",
+	if (!lib->settings->get_bool(lib->settings,
-								TRUE, lib->ns))
+							"%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);

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",
+	if (!lib->settings->get_bool(lib->settings,
-								TRUE, lib->ns))
+							"%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)

src/libstrongswan/crypto/proposal/proposal.c

@@ -485,16 +485,16 @@ 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));
-	}
-	else
 	{
 		selected = proposal_create(this->protocol, this->number);
 		selected->set_spi(selected, this->spi);
 	}
+	else
+	{
+		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;
+}

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. */
+	/** Whether to prefer configured (default) or supplied proposals. */
-	PROPOSAL_PREFER_CONFIGURED = (1<<1),
+	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.

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,
+	selected = self->select(self, other, select_data[_i].flags);
-							select_data[_i].flags | PROPOSAL_PREFER_CONFIGURED);
 	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);