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.

diff --git a/src/libcharon/sa/ike_sa.c b/src/libcharon/sa/ike_sa.c
index b7d71e4d6d862eee4874a35e4f42a2a5719358e0..e81f6ceef086308a9b7eca9573bece0f63ffba3b 100644 (file)
--- a/src/libcharon/sa/ike_sa.c
+++ b/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;

diff --git a/src/libcharon/sa/ike_sa.h b/src/libcharon/sa/ike_sa.h
index 2122867de7853509a0f23d9357fe01b031d87530..199a32c98c58eedc38a374898ba82078a528b1f2 100644 (file)
--- a/src/libcharon/sa/ike_sa.h
+++ b/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
         */

diff --git a/src/libcharon/sa/ike_sa_manager.c b/src/libcharon/sa/ike_sa_manager.c
index 63c9fdfa8e32936c9c86b24ccc206b1e984f0300..ce44207c4c972930bb039716be6f81b9206aa196 100644 (file)
--- a/src/libcharon/sa/ike_sa_manager.c
+++ b/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;

diff --git a/src/libcharon/sa/ikev2/task_manager_v2.c b/src/libcharon/sa/ikev2/task_manager_v2.c
index 702c383b1c4d71e40e5bdd34b3eb3f6365c46bbb..26e5b48a8e457fbbe1736dda04b2e94a91309ea8 100644 (file)
--- a/src/libcharon/sa/ikev2/task_manager_v2.c
+++ b/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 ||
-                                       this->ike_sa->get_state(this->ike_sa) == IKE_CONNECTING)
+                               if (state == IKE_CREATED ||
+                                       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 */
-                       if ((msg->get_exchange_type(msg) == IKE_SA_INIT &&
-                                this->ike_sa->get_state(this->ike_sa) != IKE_CREATED) ||
-                               (msg->get_exchange_type(msg) == IKE_AUTH &&
-                                this->ike_sa->get_state(this->ike_sa) != IKE_CONNECTING))
+                       /* reject initial messages if not received in specific states,
+                        * after rekeying we only expect a DELETE in an INFORMATIONAL */
+                       type = msg->get_exchange_type(msg);
+                       state = this->ike_sa->get_state(this->ike_sa);
+                       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),
-                                        ike_sa_state_names, this->ike_sa->get_state(this->ike_sa));
+                                        exchange_type_names, type, ike_sa_state_names, state);
                                return FAILED;
                        }
                        if (!this->ike_sa->supports_extension(this->ike_sa, EXT_MOBIKE))

diff --git a/src/libcharon/sa/ikev2/tasks/ike_delete.c b/src/libcharon/sa/ikev2/tasks/ike_delete.c
index e972dba07b6ca608f0fcad0a3e946db741e917fb..72d656aae952c850553292845be989d047555a06 100644 (file)
--- a/src/libcharon/sa/ikev2/tasks/ike_delete.c
+++ b/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:

diff --git a/src/libcharon/sa/ikev2/tasks/ike_rekey.c b/src/libcharon/sa/ikev2/tasks/ike_rekey.c
index b67f28f679084eea8022e8da4062092875fbdddb..334749a61ab8dc4b6645b6aeb7229d071a574ece 100644 (file)
--- a/src/libcharon/sa/ikev2/tasks/ike_rekey.c
+++ b/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,19 +117,9 @@ static void establish_new(private_ike_rekey_t *this)
                }
                this->new_sa = NULL;
                charon->bus->set_sa(charon->bus, this->ike_sa);
-       }
-}
-
-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);
+               this->ike_sa->set_state(this->ike_sa, IKE_REKEYED);
+       }
 }
 
 METHOD(task_t, build_i_delete, status_t,
@@ -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);
        }
-
-       /* 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;
+       return SUCCESS;
 }
 
 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)
                                {

diff --git a/src/libcharon/tests/suites/test_ike_rekey.c b/src/libcharon/tests/suites/test_ike_rekey.c
index d2e365ed1acc560eb81585113cf906f7f3352521..8fa6ea31562c91065c5f76f03b57740b43459798 100644 (file)
--- a/src/libcharon/tests/suites/test_ike_rekey.c
+++ b/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);