[Swan-dev] The curious case of expire_ike_because_child_not_used()

[Paul Wouters]

I'm looking at fixing ikev2-delete-sa-04 behaviour, where an auto=start
IKEv2 connection receiving a delete doesn't cause a new connection to
initiate. While a bug was found a fixed, it did lead me and Hugh into
a look at expire_ike_because_child_not_used()

The concern here is that looking at the idleness of 1 IPsec SA does not
tell you whether you can delete the IKE SA. Another IPsec SA might be
sharing that IKE SA. Why are we attempting to maybe clean up the IKE SA
instead of letting it go through its life cycle?

It is called from v2_event_sa_rekey() and v2_event_sa_replace()

The calls pass a child st state.

It checks for IS_PARENT_SA_ESTABLISHED(st) which can never happen in
the current code? Maybe pexecpt/assert instead of returning false?

If it is opportunistic, it wont delete the IKE SA. Why is this case
special?

if it has a lease, it will not delete the IKE SA. The comment here is
"#%lu has lease; should not be trying to replace".

Ok I guess fair. that connection should rekey or go away, and not get
replaced. I guess you should not have auto=start with rekey=no, or
timers where rekey is pushed passed expiry. Although it seems harmless
to try and replace it anyway?

Then it checks:

         if (IS_IKE_SA(st)) {
                 ike = pexpect_ike_sa(st);
                 cst = state_with_serialno(c->newest_ipsec_sa);


This seems questionable, as we only ever pass in child states.....


Then the function checks get_sa_info() for idleness of the child sa,
and:

 	/* we observed no traffic, let IPSEC SA and IKE SA expire */

This is wrong. An IKE SA can have multiple IPsec SA's, one of which
might not be idle. Deleting the IKE SA will cause any other active
IPsec SA's to also be torn down.

Why not just delete the IPsec SA and leav ethe IKE SA alone? The delete
code already checks if deleting an IPsec SA would leave the IKE SA
without children and would remove the IKE SA in that case.

In short, I think expire_ike_because_child_not_used() should be removed
and its callers should be updated to not touch the IKE SA.


This code seems to have come from a re-organization in november and
december last year, where it tried to merge the code for
EVENT_SA_REPLACE and EVENT_SA_REPLACE_IF_IDLE.

It seems to have similar logic:

                 } else if (type == EVENT_v2_SA_REPLACE_IF_USED &&
                                 get_sa_info(st, TRUE, &last_used_age) &&
                                 deltaless(c->sa_rekey_margin, last_used_age)) {
                         ikev2_expire_parent(st, last_used_age);
                         break;


This function was created in 2015. I have some vague recollection that
it did fix somekind of bug, but perhaps now that we have improved the 
delete functions, this IKE handling can be removed from the idle IPsec
SA handling.

I'll create a test case that should show this issue, and then see about
removing the IKE SA hanlding from the above to see if it properly fixes
things.

Paul