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

[Antony Antony]

On Sun, Feb 10, 2019 at 11:18:12PM -0500, Paul Wouters wrote:
> 
> 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?

I think check originaly was only for OE. My guess is the call is to decide 
end of life cycle of connection as whole. 

do pluto allow non oe to have this event? I thought we limited it to OE.  
non oe would have to auto=route and rekey=no? that is my re-collection.
may be things changed.

> 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? 

that is confusing. I thought one reason we added this whole thing was to 
expire unused OE IKE and Child at once.


> 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.

I would look at it if there are a couple of test cases for it.
OE ones we already have test case(s).

And this remids me again to commit whack commands rekey events!

-antony