child-rekey: Uninstall old outbound SA earlier on initiator/winner

This is useful for kernel implementations where the ordering of SAs
is unpredictable and the new SA might otherwise not be used until the
DELETE response has been received, which is not ideal as the responder
might not keep the old SA around that long.  On Linux, it makes no
difference as we switch to the new outbound SA immediately because the
updated outbound policy references its SPI.

diff --git a/src/libcharon/sa/ikev2/tasks/child_delete.c b/src/libcharon/sa/ikev2/tasks/child_delete.c
index 0e371189894655ff21e560189fcfed6c15a875f4..f3881ceaa2bc5e825208d927d65a3771a1880ba4 100644 (file)
--- a/src/libcharon/sa/ikev2/tasks/child_delete.c
+++ b/src/libcharon/sa/ikev2/tasks/child_delete.c
@@ -340,11 +340,13 @@ static status_t destroy_and_reestablish(private_child_delete_t *this)
                }
                else
                {
+                       /* the following two calls are only relevant as responder/loser of
+                        * rekeyings as the initiator/winner already did this right after
+                        * the rekeying was completed, either way, we delay destroying
+                        * the CHILD_SA, by default, so we can process delayed packets */
                        install_outbound(this, protocol, child_sa->get_rekey_spi(child_sa));
-                       /* for rekeyed CHILD_SAs we uninstall the outbound SA but don't
-                        * immediately destroy it, by default, so we can process delayed
-                        * packets */
                        child_sa->remove_outbound(child_sa);
+
                        expire = child_sa->get_lifetime(child_sa, TRUE);
                        if (delay && (!expire || ((now + delay) < expire)))
                        {

diff --git a/src/libcharon/sa/ikev2/tasks/child_rekey.c b/src/libcharon/sa/ikev2/tasks/child_rekey.c
index 36d7c4bff08080a72f5123bf461657290ed7708f..37f7ea9bdb0540fc52b5852be9ede76fe59b02bd 100644 (file)
--- a/src/libcharon/sa/ikev2/tasks/child_rekey.c
+++ b/src/libcharon/sa/ikev2/tasks/child_rekey.c
@@ -475,20 +475,18 @@ METHOD(task_t, process_i, status_t,
                        other_ts->destroy(other_ts);
                }
        }
-       if (to_delete != this->child_create->get_child(this->child_create))
-       {       /* invoke rekey hook if rekeying successful */
-               charon->bus->child_rekey(charon->bus, this->child_sa,
-                                                       this->child_create->get_child(this->child_create));
-       }
-       if (to_delete == NULL)
-       {
-               return SUCCESS;
-       }
-       /* disable updown event for redundant CHILD_SA */
        if (to_delete->get_state(to_delete) != CHILD_REKEYED)
-       {
+       {       /* disable updown event for old/redundant CHILD_SA */
                to_delete->set_state(to_delete, CHILD_REKEYED);
        }
+       if (to_delete == this->child_sa)
+       {       /* invoke rekey hook if rekeying successful and remove the old
+                * outbound SA as we installed the new one already above, but might not
+                * be using it yet depending on how SAs/policies are handled */
+               this->child_sa->remove_outbound(this->child_sa);
+               charon->bus->child_rekey(charon->bus, this->child_sa,
+                                                       this->child_create->get_child(this->child_create));
+       }
        spi = to_delete->get_spi(to_delete, TRUE);
        protocol = to_delete->get_protocol(to_delete);
 

diff --git a/src/libcharon/tests/suites/test_child_rekey.c b/src/libcharon/tests/suites/test_child_rekey.c
index b9f6ea0bcb04d52514afb4200239675b07ac159f..563357de6cf4c004e2c9235356d7c8410fda34d3 100644 (file)
--- a/src/libcharon/tests/suites/test_child_rekey.c
+++ b/src/libcharon/tests/suites/test_child_rekey.c
@@ -87,9 +87,9 @@ START_TEST(test_regular)
        assert_hook_called(child_rekey);
        assert_no_notify(IN, REKEY_SA);
        exchange_test_helper->process_message(exchange_test_helper, a, NULL);
-       assert_child_sa_state(a, spi_a, CHILD_DELETING, CHILD_OUTBOUND_INSTALLED);
+       assert_child_sa_state(a, spi_a, CHILD_DELETING, CHILD_OUTBOUND_NONE);
        assert_child_sa_state(a, 3, CHILD_INSTALLED, CHILD_OUTBOUND_INSTALLED);
-       assert_ipsec_sas_installed(a, spi_a, spi_b, 3, 4);
+       assert_ipsec_sas_installed(a, spi_a, 3, 4);
        assert_hook();
 
        /* INFORMATIONAL { D } --> */
@@ -196,9 +196,9 @@ START_TEST(test_regular_ke_invalid)
        assert_hook_called(child_rekey);
        assert_no_notify(IN, REKEY_SA);
        exchange_test_helper->process_message(exchange_test_helper, a, NULL);
-       assert_child_sa_state(a, spi_a, CHILD_DELETING);
+       assert_child_sa_state(a, spi_a, CHILD_DELETING, CHILD_OUTBOUND_NONE);
        assert_child_sa_state(a, 5, CHILD_INSTALLED, CHILD_OUTBOUND_INSTALLED);
-       assert_ipsec_sas_installed(a, spi_a, spi_b, 5, 6);
+       assert_ipsec_sas_installed(a, spi_a, 5, 6);
        assert_hook();
 
        /* INFORMATIONAL { D } --> */
@@ -250,9 +250,9 @@ START_TEST(test_regular_ke_invalid)
        assert_hook_called(child_rekey);
        assert_no_notify(IN, REKEY_SA);
        exchange_test_helper->process_message(exchange_test_helper, a, NULL);
-       assert_child_sa_state(a, 5, CHILD_DELETING, CHILD_OUTBOUND_INSTALLED);
+       assert_child_sa_state(a, 5, CHILD_DELETING, CHILD_OUTBOUND_NONE);
        assert_child_sa_state(a, 7, CHILD_INSTALLED, CHILD_OUTBOUND_INSTALLED);
-       assert_ipsec_sas_installed(a, 5, 6, 7, 8);
+       assert_ipsec_sas_installed(a, 5, 7, 8);
        assert_hook();
 
        /* INFORMATIONAL { D } --> */
@@ -320,9 +320,9 @@ START_TEST(test_regular_responder_ignore_soft_expire)
        assert_hook_called(child_rekey);
        assert_no_notify(IN, REKEY_SA);
        exchange_test_helper->process_message(exchange_test_helper, a, NULL);
-       assert_child_sa_state(a, 1, CHILD_DELETING);
+       assert_child_sa_state(a, 1, CHILD_DELETING, CHILD_OUTBOUND_NONE);
        assert_child_sa_state(a, 3, CHILD_INSTALLED, CHILD_OUTBOUND_INSTALLED);
-       assert_ipsec_sas_installed(a, 1, 2, 3, 4);
+       assert_ipsec_sas_installed(a, 1, 3, 4);
        assert_hook();
 
        /* we don't expect this to get called anymore */
@@ -398,9 +398,9 @@ START_TEST(test_regular_responder_handle_hard_expire)
        assert_hook_called(child_rekey);
        assert_no_notify(IN, REKEY_SA);
        exchange_test_helper->process_message(exchange_test_helper, a, NULL);
-       assert_child_sa_state(a, 1, CHILD_DELETING);
+       assert_child_sa_state(a, 1, CHILD_DELETING, CHILD_OUTBOUND_NONE);
        assert_child_sa_state(a, 3, CHILD_INSTALLED, CHILD_OUTBOUND_INSTALLED);
-       assert_ipsec_sas_installed(a, 1, 2, 3, 4);
+       assert_ipsec_sas_installed(a, 1, 3, 4);
        assert_hook();
 
        /* we don't expect this to get called anymore */
@@ -446,7 +446,7 @@ START_TEST(test_regular_responder_handle_hard_expire)
 END_TEST
 
 /**
- * Both peers initiate the CHILD_SA reekying concurrently and should handle
+ * Both peers initiate the CHILD_SA rekeying concurrently and should handle
  * the collision properly depending on the nonces.
  */
 START_TEST(test_collision)
@@ -521,8 +521,8 @@ START_TEST(test_collision)
                assert_child_sa_state(a, data[_i].spi_a, CHILD_INSTALLED,
                                                          CHILD_OUTBOUND_INSTALLED);
                assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING,
-                                                         CHILD_OUTBOUND_INSTALLED);
-               assert_ipsec_sas_installed(a, 1, 2, 3, 5, 6);
+                                                         CHILD_OUTBOUND_NONE);
+               assert_ipsec_sas_installed(a, 1, 3, 5, 6);
        }
        else
        {
@@ -548,8 +548,8 @@ START_TEST(test_collision)
                assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED,
                                                          CHILD_OUTBOUND_INSTALLED);
                assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING,
-                                                         CHILD_OUTBOUND_INSTALLED);
-               assert_ipsec_sas_installed(b, 1, 2, 4, 5, 6);
+                                                         CHILD_OUTBOUND_NONE);
+               assert_ipsec_sas_installed(b, 2, 4, 5, 6);
        }
        else
        {
@@ -571,7 +571,7 @@ START_TEST(test_collision)
        assert_jobs_scheduled(1);
        exchange_test_helper->process_message(exchange_test_helper, b, NULL);
        assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING,
-                                                 data[_i].spi_del_b == 2 ? CHILD_OUTBOUND_INSTALLED
+                                                 data[_i].spi_del_b == 2 ? CHILD_OUTBOUND_NONE
                                                                                                  : CHILD_OUTBOUND_REGISTERED);
        assert_child_sa_state(b, data[_i].spi_del_a, CHILD_DELETED,
                                                  CHILD_OUTBOUND_NONE);
@@ -580,7 +580,7 @@ START_TEST(test_collision)
        assert_child_sa_count(b, 3);
        if (data[_i].spi_del_b == 2)
        {
-               assert_ipsec_sas_installed(b, 1, 2, 4, 5, 6);
+               assert_ipsec_sas_installed(b, 2, 4, 5, 6);
        }
        else
        {
@@ -591,7 +591,7 @@ START_TEST(test_collision)
        assert_jobs_scheduled(1);
        exchange_test_helper->process_message(exchange_test_helper, a, NULL);
        assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING,
-                                                 data[_i].spi_del_a == 1 ? CHILD_OUTBOUND_INSTALLED
+                                                 data[_i].spi_del_a == 1 ? CHILD_OUTBOUND_NONE
                                                                                                  : CHILD_OUTBOUND_REGISTERED);
        assert_child_sa_state(a, data[_i].spi_del_b, CHILD_DELETED,
                                                  CHILD_OUTBOUND_NONE);
@@ -600,7 +600,7 @@ START_TEST(test_collision)
        assert_child_sa_count(a, 3);
        if (data[_i].spi_del_a == 1)
        {
-               assert_ipsec_sas_installed(a, 1, 2, 3, 5, 6);
+               assert_ipsec_sas_installed(a, 1, 3, 5, 6);
        }
        else
        {
@@ -740,8 +740,8 @@ START_TEST(test_collision_delayed_response)
                assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED,
                                                          CHILD_OUTBOUND_INSTALLED);
                assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING,
-                                                         CHILD_OUTBOUND_INSTALLED);
-               assert_ipsec_sas_installed(b, 1, 2, 4, 5, 6);
+                                                         CHILD_OUTBOUND_NONE);
+               assert_ipsec_sas_installed(b, 2, 4, 5, 6);
        }
        else
        {
@@ -809,8 +809,8 @@ START_TEST(test_collision_delayed_response)
                exchange_test_helper->process_message(exchange_test_helper, a, msg);
                assert_hook();
                assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING,
-                                                         CHILD_OUTBOUND_INSTALLED);
-               assert_ipsec_sas_installed(a, 1, 2, 3, 5, 6);
+                                                         CHILD_OUTBOUND_NONE);
+               assert_ipsec_sas_installed(a, 1, 3, 5, 6);
        }
        else
        {
@@ -937,9 +937,9 @@ START_TEST(test_collision_delayed_request)
        /* CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } --> */
        assert_hook_rekey(child_rekey, 2, 4);
        exchange_test_helper->process_message(exchange_test_helper, b, NULL);
-       assert_child_sa_state(b, 2, CHILD_DELETING, CHILD_OUTBOUND_INSTALLED);
+       assert_child_sa_state(b, 2, CHILD_DELETING, CHILD_OUTBOUND_NONE);
        assert_child_sa_state(b, 4, CHILD_INSTALLED, CHILD_OUTBOUND_INSTALLED);
-       assert_ipsec_sas_installed(b, 1, 2, 4, 5);
+       assert_ipsec_sas_installed(b, 2, 4, 5);
        assert_hook();
 
        /* we don't expect this hook to get called anymore */
@@ -948,7 +948,7 @@ START_TEST(test_collision_delayed_request)
        /* CREATE_CHILD_SA { N(REKEY_SA), SA, Ni, [KEi,] TSi, TSr } --> (delayed) */
        assert_single_notify(OUT, TEMPORARY_FAILURE);
        exchange_test_helper->process_message(exchange_test_helper, b, msg);
-       assert_child_sa_state(b, 2, CHILD_DELETING, CHILD_OUTBOUND_INSTALLED);
+       assert_child_sa_state(b, 2, CHILD_DELETING, CHILD_OUTBOUND_NONE);
        assert_child_sa_state(b, 4, CHILD_INSTALLED, CHILD_OUTBOUND_INSTALLED);
 
        /* <-- INFORMATIONAL { D } */
@@ -1060,9 +1060,9 @@ START_TEST(test_collision_delayed_request_more)
        /* CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } --> */
        assert_hook_rekey(child_rekey, 2, 4);
        exchange_test_helper->process_message(exchange_test_helper, b, NULL);
-       assert_child_sa_state(b, 2, CHILD_DELETING, CHILD_OUTBOUND_INSTALLED);
+       assert_child_sa_state(b, 2, CHILD_DELETING, CHILD_OUTBOUND_NONE);
        assert_child_sa_state(b, 4, CHILD_INSTALLED, CHILD_OUTBOUND_INSTALLED);
-       assert_ipsec_sas_installed(b, 1, 2, 4, 5);
+       assert_ipsec_sas_installed(b, 2, 4, 5);
        assert_hook();
 
        /* we don't expect this hook to get called anymore */
@@ -1236,7 +1236,7 @@ START_TEST(test_collision_ke_invalid)
                assert_child_sa_state(a, data[_i].spi_a, CHILD_INSTALLED,
                                                          CHILD_OUTBOUND_INSTALLED);
                assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING,
-                                                         CHILD_OUTBOUND_INSTALLED);
+                                                         CHILD_OUTBOUND_NONE);
        }
        else
        {
@@ -1259,7 +1259,7 @@ START_TEST(test_collision_ke_invalid)
                assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED,
                                                          CHILD_OUTBOUND_INSTALLED);
                assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING,
-                                                         CHILD_OUTBOUND_INSTALLED);
+                                                         CHILD_OUTBOUND_NONE);
        }
        else
        {
@@ -1279,7 +1279,7 @@ START_TEST(test_collision_ke_invalid)
        assert_jobs_scheduled(1);
        exchange_test_helper->process_message(exchange_test_helper, b, NULL);
        assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING,
-                                                 data[_i].spi_del_b == 2 ? CHILD_OUTBOUND_INSTALLED
+                                                 data[_i].spi_del_b == 2 ? CHILD_OUTBOUND_NONE
                                                                                                  : CHILD_OUTBOUND_REGISTERED);
        assert_child_sa_state(b, data[_i].spi_del_a, CHILD_DELETED,
                                                  CHILD_OUTBOUND_NONE);
@@ -1291,7 +1291,7 @@ START_TEST(test_collision_ke_invalid)
        assert_jobs_scheduled(1);
        exchange_test_helper->process_message(exchange_test_helper, a, NULL);
        assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING,
-                                                 data[_i].spi_del_a == 1 ? CHILD_OUTBOUND_INSTALLED
+                                                 data[_i].spi_del_a == 1 ? CHILD_OUTBOUND_NONE
                                                                                                  : CHILD_OUTBOUND_REGISTERED);
        assert_child_sa_state(a, data[_i].spi_del_b, CHILD_DELETED,
                                                  CHILD_OUTBOUND_NONE);
@@ -1441,7 +1441,7 @@ START_TEST(test_collision_ke_invalid_delayed_retry)
        /* CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } --> */
        assert_hook_rekey(child_rekey, 2, 8);
        exchange_test_helper->process_message(exchange_test_helper, b, NULL);
-       assert_child_sa_state(b, 2, CHILD_DELETING, CHILD_OUTBOUND_INSTALLED);
+       assert_child_sa_state(b, 2, CHILD_DELETING, CHILD_OUTBOUND_NONE);
        assert_child_sa_state(b, 8, CHILD_INSTALLED, CHILD_OUTBOUND_INSTALLED);
        assert_hook();
 
@@ -1451,7 +1451,7 @@ START_TEST(test_collision_ke_invalid_delayed_retry)
        /* CREATE_CHILD_SA { N(REKEY_SA), SA, Ni, [KEi,] TSi, TSr } --> (delayed) */
        assert_single_notify(OUT, TEMPORARY_FAILURE);
        exchange_test_helper->process_message(exchange_test_helper, b, msg);
-       assert_child_sa_state(b, 2, CHILD_DELETING, CHILD_OUTBOUND_INSTALLED);
+       assert_child_sa_state(b, 2, CHILD_DELETING, CHILD_OUTBOUND_NONE);
        assert_child_sa_state(b, 8, CHILD_INSTALLED, CHILD_OUTBOUND_INSTALLED);
 
        /* <-- INFORMATIONAL { D } */