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

Andrew Cagney andrew.cagney at gmail.com
Thu Feb 14 21:58:40 UTC 2019


On Sun, 10 Feb 2019 at 23:18, Paul Wouters <paul at nohats.ca> 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?

Sounds like it should be IKE only, and look at all the SAs.

> 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
> _______________________________________________
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev


More information about the Swan-dev mailing list