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.

diff --git a/src/libcharon/sa/ike_sa.c b/src/libcharon/sa/ike_sa.c
index e482d55eedd836ad6af5df106a2c0244b802e924..f1e3e8fe538b1f9f2758351c50b3a8b0cf4a913c 100644 (file)
--- a/src/libcharon/sa/ike_sa.c
+++ b/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);
-       if (this->task_manager->retransmit(this->task_manager, message_id) != SUCCESS)
+       switch (this->task_manager->retransmit(this->task_manager, message_id))
        {
-               /* send a proper signal to brief interested bus listeners */
-               switch (this->state)
+               case SUCCESS:
+                       this->stats[STAT_OUTBOUND] = time_monotonic(NULL);
+                       return SUCCESS;
+               case INVALID_STATE:
+                       return INVALID_STATE;
+               default:
+                       break;
+       }
+       /* send a proper signal to brief interested bus listeners */
+       switch (this->state)
+       {
+               case IKE_CONNECTING:
                {
-                       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)
                        {
-                               /* 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");
+                               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 (this->version == IKEV1 && array_count(this->child_sas))
+                               /* 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))
                                {
-                                       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)
                                        {
-                                               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);
-                                               }
+                                               charon->bus->child_updown(charon->bus, child_sa,
+                                                                                                 FALSE);
                                        }
-                                       enumerator->destroy(enumerator);
                                }
-                               break;
+                               enumerator->destroy(enumerator);
                        }
-                       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);
+                       break;
                }
-               return DESTROY_ME;
+               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;
        }
-       return SUCCESS;
+       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,

diff --git a/src/libcharon/sa/ike_sa.h b/src/libcharon/sa/ike_sa.h
index 56dd53efe2a560749130e2ea420a0377a3938541..b965a49c97981eedfc09911537a8f5e0cad6383e 100644 (file)
--- a/src/libcharon/sa/ike_sa.h
+++ b/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
-        *                                              - NOT_FOUND if request doesn't have to be retransmitted
+        *                                              - SUCCESS if retransmit was sent
+        *                                              - 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.

diff --git a/src/libcharon/sa/ikev1/task_manager_v1.c b/src/libcharon/sa/ikev1/task_manager_v1.c
index 00f5e6f05e4d4a925c28ba5eef9bdd62a8bc7298..70cebe73557246f1a1da9a0102c52aa10f2f876d 100644 (file)
--- a/src/libcharon/sa/ikev1/task_manager_v1.c
+++ b/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))

diff --git a/src/libcharon/sa/ikev2/task_manager_v2.c b/src/libcharon/sa/ikev2/task_manager_v2.c
index 6e1d00851134a0dc5e6ed7e70b56d8f3b2b291ba..9c7a5378d3a801f8ea5cd46c563b2f0fa026d1fa 100644 (file)
--- a/src/libcharon/sa/ikev2/task_manager_v2.c
+++ b/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,