ikev2: Add a new state to track rekeyed IKE_SAs

This makes handling such IKE_SAs more specifically compared to keeping them
in state IKE_CONNECTING or IKE_ESTABLISHED (which we did when we lost a
collision - even triggering the ike_updown event), or using IKE_REKEYING for
them, which would also be ambiguous.

For instance, we can now reject anything but DELETES for such SAs.

src/libcharon/sa/ike_sa.c

@@ -71,6 +71,7 @@ ENUM(ike_sa_state_names, IKE_CREATED, IKE_DESTROYING,
 	"ESTABLISHED",
 	"PASSIVE",
 	"REKEYING",
+	"REKEYED",
 	"DELETING",
 	"DESTROYING",
 );
@@ -2356,7 +2357,8 @@ METHOD(ike_sa_t, retransmit, status_t,
 				reestablish(this);
 				break;
 		}
-		if (this->state != IKE_CONNECTING)
+		if (this->state != IKE_CONNECTING &&
+			this->state != IKE_REKEYED)
 		{
 			charon->bus->ike_updown(charon->bus, &this->public, FALSE);
 		}
@@ -2508,6 +2510,7 @@ METHOD(ike_sa_t, roam, status_t,
 		case IKE_DELETING:
 		case IKE_DESTROYING:
 		case IKE_PASSIVE:
+		case IKE_REKEYED:
 			return SUCCESS;
 		default:
 			break;

src/libcharon/sa/ike_sa.h

@@ -308,6 +308,11 @@ enum ike_sa_state_t {
 	 */
 	IKE_REKEYING,
 
+	/**
+	 * IKE_SA has been rekeyed (or is redundant)
+	 */
+	IKE_REKEYED,
+
 	/**
 	 * IKE_SA is in progress of deletion
 	 */

src/libcharon/sa/ike_sa_manager.c

@@ -1415,7 +1415,8 @@ METHOD(ike_sa_manager_t, checkout_by_config, ike_sa_t*,
 		{
 			continue;
 		}
-		if (entry->ike_sa->get_state(entry->ike_sa) == IKE_DELETING)
+		if (entry->ike_sa->get_state(entry->ike_sa) == IKE_DELETING ||
+			entry->ike_sa->get_state(entry->ike_sa) == IKE_REKEYED)
 		{	/* skip IKE_SAs which are not usable, wake other waiting threads */
 			entry->condvar->signal(entry->condvar);
 			continue;

src/libcharon/sa/ikev2/task_manager_v2.c

@@ -535,6 +535,7 @@ METHOD(task_manager_t, initiate, status_t,
 					break;
 				}
 			case IKE_REKEYING:
+			case IKE_REKEYED:
 				if (activate_task(this, TASK_IKE_DELETE))
 				{
 					exchange = INFORMATIONAL;
@@ -611,7 +612,8 @@ METHOD(task_manager_t, initiate, status_t,
 			case FAILED:
 			default:
 				this->initiating.type = EXCHANGE_TYPE_UNDEFINED;
-				if (this->ike_sa->get_state(this->ike_sa) != IKE_CONNECTING)
+				if (this->ike_sa->get_state(this->ike_sa) != IKE_CONNECTING &&
+					this->ike_sa->get_state(this->ike_sa) != IKE_REKEYED)
 				{
 					charon->bus->ike_updown(charon->bus, this->ike_sa, FALSE);
 				}
@@ -909,9 +911,11 @@ static status_t process_request(private_task_manager_t *this,
 	payload_t *payload;
 	notify_payload_t *notify;
 	delete_payload_t *delete;
+	ike_sa_state_t state;
 
 	if (array_count(this->passive_tasks) == 0)
 	{	/* create tasks depending on request type, if not already some queued */
+		state = this->ike_sa->get_state(this->ike_sa);
 		switch (message->get_exchange_type(message))
 		{
 			case IKE_SA_INIT:
@@ -947,8 +951,8 @@ static status_t process_request(private_task_manager_t *this,
 			{	/* FIXME: we should prevent this on mediation connections */
 				bool notify_found = FALSE, ts_found = FALSE;
 
-				if (this->ike_sa->get_state(this->ike_sa) == IKE_CREATED ||
+				if (state == IKE_CREATED ||
-					this->ike_sa->get_state(this->ike_sa) == IKE_CONNECTING)
+					state == IKE_CONNECTING)
 				{
 					DBG1(DBG_IKE, "received CREATE_CHILD_SA request for "
 						 "unestablished IKE_SA, rejected");
@@ -1013,6 +1017,14 @@ static status_t process_request(private_task_manager_t *this,
 						case PLV2_NOTIFY:
 						{
 							notify = (notify_payload_t*)payload;
+							if (state == IKE_REKEYED)
+							{
+								DBG1(DBG_IKE, "received unexpected notify %N "
+									 "for rekeyed IKE_SA, ignored",
+									 notify_type_names,
+									 notify->get_notify_type(notify));
+								break;
+							}
 							switch (notify->get_notify_type(notify))
 							{
 								case ADDITIONAL_IP4_ADDRESS:
@@ -1355,6 +1367,8 @@ METHOD(task_manager_t, process_message, status_t,
 	status_t status;
 	uint32_t mid;
 	bool schedule_delete_job = FALSE;
+	ike_sa_state_t state;
+	exchange_type_t type;
 
 	charon->bus->message(charon->bus, msg, TRUE, FALSE);
 	status = parse_message(this, msg);
@@ -1395,15 +1409,16 @@ METHOD(task_manager_t, process_message, status_t,
 	{
 		if (mid == this->responding.mid)
 		{
-			/* reject initial messages if not received in specific states */
+			/* reject initial messages if not received in specific states,
-			if ((msg->get_exchange_type(msg) == IKE_SA_INIT &&
+			 * after rekeying we only expect a DELETE in an INFORMATIONAL */
-				 this->ike_sa->get_state(this->ike_sa) != IKE_CREATED) ||
+			type = msg->get_exchange_type(msg);
-				(msg->get_exchange_type(msg) == IKE_AUTH &&
+			state = this->ike_sa->get_state(this->ike_sa);
-				 this->ike_sa->get_state(this->ike_sa) != IKE_CONNECTING))
+			if ((type == IKE_SA_INIT && state != IKE_CREATED) ||
+				(type == IKE_AUTH && state != IKE_CONNECTING) ||
+				(state == IKE_REKEYED && type != INFORMATIONAL))
 			{
 				DBG1(DBG_IKE, "ignoring %N in IKE_SA state %N",
-					 exchange_type_names, msg->get_exchange_type(msg),
+					 exchange_type_names, type, ike_sa_state_names, state);
-					 ike_sa_state_names, this->ike_sa->get_state(this->ike_sa));
 				return FAILED;
 			}
 			if (!this->ike_sa->supports_extension(this->ike_sa, EXT_MOBIKE))

src/libcharon/sa/ikev2/tasks/ike_delete.c

@@ -68,7 +68,8 @@ METHOD(task_t, build_i, status_t,
 	delete_payload = delete_payload_create(PLV2_DELETE, PROTO_IKE);
 	message->add_payload(message, (payload_t*)delete_payload);
 
-	if (this->ike_sa->get_state(this->ike_sa) == IKE_REKEYING)
+	if (this->ike_sa->get_state(this->ike_sa) == IKE_REKEYING ||
+		this->ike_sa->get_state(this->ike_sa) == IKE_REKEYED)
 	{
 		this->rekeyed = TRUE;
 	}
@@ -119,11 +120,12 @@ METHOD(task_t, process_r, status_t,
 
 	switch (this->ike_sa->get_state(this->ike_sa))
 	{
+		case IKE_REKEYING:
 		case IKE_ESTABLISHED:
 			this->ike_sa->set_state(this->ike_sa, IKE_DELETING);
 			this->ike_sa->reestablish(this->ike_sa);
 			return NEED_MORE;
-		case IKE_REKEYING:
+		case IKE_REKEYED:
 			this->rekeyed = TRUE;
 			break;
 		case IKE_DELETING:

src/libcharon/sa/ikev2/tasks/ike_rekey.c

@@ -84,7 +84,6 @@ static job_t* check_queued_tasks(ike_sa_t *ike_sa)
 		job = (job_t*)initiate_tasks_job_create(ike_sa->get_id(ike_sa));
 	}
 	enumerator->destroy(enumerator);
-
 	return job;
 }
 
@@ -118,21 +117,11 @@ static void establish_new(private_ike_rekey_t *this)
 		}
 		this->new_sa = NULL;
 		charon->bus->set_sa(charon->bus, this->ike_sa);
+
+		this->ike_sa->set_state(this->ike_sa, IKE_REKEYED);
 	}
 }
 
-METHOD(task_t, process_r_delete, status_t,
-	private_ike_rekey_t *this, message_t *message)
-{
-	return this->ike_delete->task.process(&this->ike_delete->task, message);
-}
-
-METHOD(task_t, build_r_delete, status_t,
-	private_ike_rekey_t *this, message_t *message)
-{
-	return this->ike_delete->task.build(&this->ike_delete->task, message);
-}
-
 METHOD(task_t, build_i_delete, status_t,
 	private_ike_rekey_t *this, message_t *message)
 {
@@ -236,21 +225,12 @@ METHOD(task_t, build_r, status_t,
 	if (this->ike_sa->get_state(this->ike_sa) != IKE_REKEYING)
 	{	/* in case of a collision we let the initiating task handle this */
 		establish_new(this);
-		this->ike_sa->set_state(this->ike_sa, IKE_REKEYING);
+		/* make sure the IKE_SA is gone in case the peer fails to delete it */
+		lib->scheduler->schedule_job(lib->scheduler, (job_t*)
+			delete_ike_sa_job_create(this->ike_sa->get_id(this->ike_sa), TRUE),
+									 90);
 	}
-
+	return SUCCESS;
-	/* rekeying successful, delete the IKE_SA using a subtask */
-	this->ike_delete = ike_delete_create(this->ike_sa, FALSE);
-	this->public.task.build = _build_r_delete;
-	this->public.task.process = _process_r_delete;
-
-	/* the peer does have to delete the IKE_SA. If it does not, we get a
-	 * unusable IKE_SA in REKEYING state without a replacement. We consider
-	 * this a timeout condition by the peer, and trigger a delete actively. */
-	lib->scheduler->schedule_job(lib->scheduler, (job_t*)
-		delete_ike_sa_job_create(this->ike_sa->get_id(this->ike_sa), TRUE), 90);
-
-	return NEED_MORE;
 }
 
 METHOD(task_t, process_i, status_t,
@@ -316,11 +296,13 @@ METHOD(task_t, process_i, status_t,
 				/* peer should delete this SA. Add a timeout just in case. */
 				job_t *job = (job_t*)delete_ike_sa_job_create(
 									other->new_sa->get_id(other->new_sa), TRUE);
-				lib->scheduler->schedule_job(lib->scheduler, job, 10);
+				lib->scheduler->schedule_job(lib->scheduler, job,
+											 HALF_OPEN_IKE_SA_TIMEOUT);
 				DBG1(DBG_IKE, "IKE_SA rekey collision won, waiting for delete "
 					 "for redundant IKE_SA %s[%d]",
 					 other->new_sa->get_name(other->new_sa),
 					 other->new_sa->get_unique_id(other->new_sa));
+				other->new_sa->set_state(other->new_sa, IKE_REKEYED);
 				charon->ike_sa_manager->checkin(charon->ike_sa_manager,
 												other->new_sa);
 				other->new_sa = NULL;
@@ -335,7 +317,8 @@ METHOD(task_t, process_i, status_t,
 				this->new_sa->set_my_host(this->new_sa, host->clone(host));
 				host = this->ike_sa->get_other_host(this->ike_sa);
 				this->new_sa->set_other_host(this->new_sa, host->clone(host));
-				this->ike_sa->set_state(this->ike_sa, IKE_ESTABLISHED);
+				/* IKE_SAs in state IKE_REKEYED are silently deleted, so we use
+				 * IKE_REKEYING */
 				this->new_sa->set_state(this->new_sa, IKE_REKEYING);
 				if (this->new_sa->delete(this->new_sa) == DESTROY_ME)
 				{

src/libcharon/tests/suites/test_ike_rekey.c

@@ -51,7 +51,7 @@ START_TEST(test_regular)
 	assert_hook_rekey(ike_rekey, 1, 3);
 	assert_no_notify(IN, REKEY_SA);
 	exchange_test_helper->process_message(exchange_test_helper, b, NULL);
-	assert_ike_sa_state(b, IKE_REKEYING);
+	assert_ike_sa_state(b, IKE_REKEYED);
 	assert_child_sa_count(b, 0);
 	new_sa = assert_ike_sa_checkout(3, 4, FALSE);
 	assert_ike_sa_state(new_sa, IKE_ESTABLISHED);