ike: Only track actually sent retransmits as outbound packets

Retransmission jobs for old requests for which we already received a
response previously left the impression that messages were sent more
recently than was actually the case.

task_manager_t always defined INVALID_STATE as possible return value if
no retransmit was sent, this just was never actually returned.

I guess we could further differentiate between actual invalid states
(e.g. if we already received the response) and when we don't send a
retransmit for other reasons e.g. because the IKE_SA became stale.

src/libcharon/sa/ike_sa.c

@@ -2426,81 +2426,86 @@ METHOD(ike_sa_t, retransmit, status_t,
 	{
 		return INVALID_STATE;
 	}
-	this->stats[STAT_OUTBOUND] = time_monotonic(NULL);
+	switch (this->task_manager->retransmit(this->task_manager, message_id))
-	if (this->task_manager->retransmit(this->task_manager, message_id) != SUCCESS)
 	{
-		/* send a proper signal to brief interested bus listeners */
+		case SUCCESS:
-		switch (this->state)
+			this->stats[STAT_OUTBOUND] = time_monotonic(NULL);
-		{
+			return SUCCESS;
-			case IKE_CONNECTING:
+		case INVALID_STATE:
-			{
+			return INVALID_STATE;
-				/* retry IKE_SA_INIT/Main Mode if we have multiple keyingtries */
+		default:
-				uint32_t tries = this->peer_cfg->get_keyingtries(this->peer_cfg);
+			break;
-				charon->bus->alert(charon->bus, ALERT_PEER_INIT_UNREACHABLE,
-								   this->keyingtry);
-				this->keyingtry++;
-				if (tries == 0 || tries > this->keyingtry)
-				{
-					DBG1(DBG_IKE, "peer not responding, trying again (%d/%d)",
-						 this->keyingtry + 1, tries);
-					reset(this, TRUE);
-					resolve_hosts(this);
-					return this->task_manager->initiate(this->task_manager);
-				}
-				DBG1(DBG_IKE, "establishing IKE_SA failed, peer not responding");
-
-				if (this->version == IKEV1 && array_count(this->child_sas))
-				{
-					enumerator_t *enumerator;
-					child_sa_t *child_sa;
-
-					/* if reauthenticating an IKEv1 SA failed (assumed for an SA
-					 * in this state with CHILD_SAs), try again from scratch */
-					DBG1(DBG_IKE, "reauthentication failed, trying to "
-						 "reestablish IKE_SA");
-					reestablish(this);
-					/* trigger down events for the CHILD_SAs, as no down event
-					 * is triggered below for IKE SAs in this state */
-					enumerator = array_create_enumerator(this->child_sas);
-					while (enumerator->enumerate(enumerator, &child_sa))
-					{
-						if (child_sa->get_state(child_sa) != CHILD_REKEYED &&
-							child_sa->get_state(child_sa) != CHILD_DELETED)
-						{
-							charon->bus->child_updown(charon->bus, child_sa,
-													  FALSE);
-						}
-					}
-					enumerator->destroy(enumerator);
-				}
-				break;
-			}
-			case IKE_DELETING:
-				DBG1(DBG_IKE, "proper IKE_SA delete failed, peer not responding");
-				if (has_condition(this, COND_REAUTHENTICATING) &&
-					!lib->settings->get_bool(lib->settings,
-										"%s.make_before_break", FALSE, lib->ns))
-				{
-					DBG1(DBG_IKE, "delete during reauthentication failed, "
-						 "trying to reestablish IKE_SA anyway");
-					reestablish(this);
-				}
-				break;
-			case IKE_REKEYING:
-				DBG1(DBG_IKE, "rekeying IKE_SA failed, peer not responding");
-				/* FALL */
-			default:
-				reestablish(this);
-				break;
-		}
-		if (this->state != IKE_CONNECTING &&
-			this->state != IKE_REKEYED)
-		{
-			charon->bus->ike_updown(charon->bus, &this->public, FALSE);
-		}
-		return DESTROY_ME;
 	}
-	return SUCCESS;
+	/* send a proper signal to brief interested bus listeners */
+	switch (this->state)
+	{
+		case IKE_CONNECTING:
+		{
+			/* retry IKE_SA_INIT/Main Mode if we have multiple keyingtries */
+			uint32_t tries = this->peer_cfg->get_keyingtries(this->peer_cfg);
+			charon->bus->alert(charon->bus, ALERT_PEER_INIT_UNREACHABLE,
+							   this->keyingtry);
+			this->keyingtry++;
+			if (tries == 0 || tries > this->keyingtry)
+			{
+				DBG1(DBG_IKE, "peer not responding, trying again (%d/%d)",
+					 this->keyingtry + 1, tries);
+				reset(this, TRUE);
+				resolve_hosts(this);
+				return this->task_manager->initiate(this->task_manager);
+			}
+			DBG1(DBG_IKE, "establishing IKE_SA failed, peer not responding");
+
+			if (this->version == IKEV1 && array_count(this->child_sas))
+			{
+				enumerator_t *enumerator;
+				child_sa_t *child_sa;
+
+				/* if reauthenticating an IKEv1 SA failed (assumed for an SA
+				 * in this state with CHILD_SAs), try again from scratch */
+				DBG1(DBG_IKE, "reauthentication failed, trying to "
+					 "reestablish IKE_SA");
+				reestablish(this);
+				/* trigger down events for the CHILD_SAs, as no down event
+				 * is triggered below for IKE SAs in this state */
+				enumerator = array_create_enumerator(this->child_sas);
+				while (enumerator->enumerate(enumerator, &child_sa))
+				{
+					if (child_sa->get_state(child_sa) != CHILD_REKEYED &&
+						child_sa->get_state(child_sa) != CHILD_DELETED)
+					{
+						charon->bus->child_updown(charon->bus, child_sa,
+												  FALSE);
+					}
+				}
+				enumerator->destroy(enumerator);
+			}
+			break;
+		}
+		case IKE_DELETING:
+			DBG1(DBG_IKE, "proper IKE_SA delete failed, peer not responding");
+			if (has_condition(this, COND_REAUTHENTICATING) &&
+				!lib->settings->get_bool(lib->settings,
+									"%s.make_before_break", FALSE, lib->ns))
+			{
+				DBG1(DBG_IKE, "delete during reauthentication failed, "
+					 "trying to reestablish IKE_SA anyway");
+				reestablish(this);
+			}
+			break;
+		case IKE_REKEYING:
+			DBG1(DBG_IKE, "rekeying IKE_SA failed, peer not responding");
+			/* FALL */
+		default:
+			reestablish(this);
+			break;
+	}
+	if (this->state != IKE_CONNECTING &&
+		this->state != IKE_REKEYED)
+	{
+		charon->bus->ike_updown(charon->bus, &this->public, FALSE);
+	}
+	return DESTROY_ME;
 }
 
 METHOD(ike_sa_t, set_auth_lifetime, status_t,

src/libcharon/sa/ike_sa.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2006-2019 Tobias Brunner
+ * Copyright (C) 2006-2020 Tobias Brunner
  * Copyright (C) 2006 Daniel Roethlisberger
  * Copyright (C) 2005-2009 Martin Willi
  * Copyright (C) 2005 Jan Hutter
@@ -872,10 +872,11 @@ struct ike_sa_t {
 	 *
 	 * @param message_id	ID of the request to retransmit
 	 * @return
-	 *						- SUCCESS
+	 *						- SUCCESS if retransmit was sent
-	 *						- NOT_FOUND if request doesn't have to be retransmitted
+	 *						- INVALID_STATE if no retransmit required
+	 *						- DESTROY_ME if this IKE_SA MUST be deleted
 	 */
-	status_t (*retransmit) (ike_sa_t *this, uint32_t message_id);
+	status_t (*retransmit)(ike_sa_t *this, uint32_t message_id);
 
 	/**
 	 * Sends a DPD request to the peer.

src/libcharon/sa/ikev1/task_manager_v1.c

@@ -404,7 +404,7 @@ static status_t retransmit_packet(private_task_manager_t *this, uint32_t seqnr,
 METHOD(task_manager_t, retransmit, status_t,
 	private_task_manager_t *this, uint32_t seqnr)
 {
-	status_t status = SUCCESS;
+	status_t status = INVALID_STATE;
 
 	if (seqnr == this->initiating.seqnr &&
 		array_count(this->initiating.packets))

src/libcharon/sa/ikev2/task_manager_v2.c

@@ -409,7 +409,7 @@ METHOD(task_manager_t, retransmit, status_t,
 						 "deferred");
 					this->ike_sa->set_condition(this->ike_sa, COND_STALE, TRUE);
 					this->initiating.deferred = TRUE;
-					return SUCCESS;
+					return INVALID_STATE;
 				}
 				else if (mobike->is_probing(mobike))
 				{
@@ -443,7 +443,7 @@ METHOD(task_manager_t, retransmit, status_t,
 					 "deferred");
 				this->ike_sa->set_condition(this->ike_sa, COND_STALE, TRUE);
 				this->initiating.deferred = TRUE;
-				return SUCCESS;
+				return INVALID_STATE;
 			}
 		}
 
@@ -451,8 +451,9 @@ METHOD(task_manager_t, retransmit, status_t,
 		job = (job_t*)retransmit_job_create(this->initiating.mid,
 											this->ike_sa->get_id(this->ike_sa));
 		lib->scheduler->schedule_job_ms(lib->scheduler, job, timeout);
+		return SUCCESS;
 	}
-	return SUCCESS;
+	return INVALID_STATE;
 }
 
 METHOD(task_manager_t, initiate, status_t,