From: Tobias Brunner Date: Fri, 26 Nov 2021 10:32:46 +0000 (+0100) Subject: child-rekey: Uninstall old outbound SA earlier on initiator/winner X-Git-Tag: 5.9.5dr3~8 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=dcaf9b38f3c6bdbea56b7a88c894ed29639c606b;p=thirdparty%2Fstrongswan.git 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 0e37118989..f3881ceaa2 100644 --- 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 36d7c4bff0..37f7ea9bdb 100644 --- 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 b9f6ea0bcb..563357de6c 100644 --- 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 } */