[Swan-dev] WIP: supporting xfrm SA expire

[Antony Antony]

I have rebased the branches a couple days ago. minor fixes to ignore
acquire SA expire. GiB...EiB support.

On Sun, Nov 28, 2021 at 05:21:36PM -0500, Paul Wouters wrote:
> On Nov 27, 2021, at 14:03, Antony Antony <antony at phenome.org> wrote:
> > 
> > Hi,
> > I rebased this branch and improved expire handling.
> > 
> > #sa-expire or #sa-expire-20211127
> > https://github.com/antonyantony/libreswan/tree/sa-expire
> 
> Awesome. I will have a look.
> 
> > I renamed keywords to salifebytes= salifepackets= 
> 
> You had those names before. I wrote at the time:
> 
> I noticed you used salifebytes= and salifepackets=. I think it would be
> more intuitive to call these maxbytes= and maxpackets. Or limit-bytes=
> or bytelimit= and packet-limit= ?

right. I choose samaxbytes and maxpackets.  maxbytes does not feel future 
proof to me.  It could be max bytes something else. maxpackets also feel 
vauge to me. Even though samaxbytes is not super clear. It is distinct.


> Similarly, where strongswan has inactivity= I think idletimeout= or
> idle-timeout= would be more clear? I wouldn't call in inactive because
> the tunnel is "active" (or "up") - there is just no traffic happening.
> 
> I do understand why you added the "sa" prefix, because we in theory also
> have these options on the IKE SA (for FIPS compliance), but I think
> those maximums could just be hardcoded to a much lower count and might
> never need to be user configurable? Like wouldn't 1Gbyte of IKE
> traffic be a good time to re-auth or rekey-with-pfs ? In which case it
> might make sense to omit the "sa" prefix for the regular admin?
> 
> 
> 
> 
> I still think using an “sa” prefix is less intuitive, especially because an IKE SA is also an SA anyway. Which is why we have lifetime= and ikelifetime=  (but we also have salifetime= being the same as lifetime=)

as mentioned above I feel strongly to add sa prefixes!
I undersand it is less intuitive, other option "maxbytes" feel confusing.

> > added few basic checks to avoid corner cases those netlink calls will return error:
> > - do not send delete request to kernel for a "hard" expired SA (one directional). xfrm already deleted it.
> > - do not query traffic,  get_sa_info() in delete_state() on hard expired SA
> 
> These are good although I hope our code never hits hard expire ever. It should only hit when there is a bug that prevented the soft timer to not rekey in time.
> 
> > When there is expire store the last known traffic.
> > Also added fuzzing to soft bytes, and packet limits using c->sa_rekey_fuzz.
> 
> That’s a good point !
> 
> > At the moment I am leaving the responder with hard expire == soft expire
> 
> Do you really mean hard and soft here and not fuzzing the soft bytes ? If so I think that is wrong. A responder can also have rekey=yes and shouldn’t end up with a traffic leak during its rekey.
> 
> > I want give preference to initiator. I will change the responder soft limit 
> > calculation. I am still trying to come up with a simple calculation.
> > Ideally the inititiator should trigger the rekey first. If both sides have 
> > the same softlimit, both sides will trigger rekey on the same packet.
> > It would lead to extra SA. That is why it is important to have initiator 
> > value entirely less than the responder values.
> 
> Yes this part I totally agree with.
> 
> > One thing decide as group is how to represent big number (2^64) bytes and 
> > packets, especially the default 2^64  will appear in "ipsec status:  
> > output.
> > 18446744073709551615 look ugly:) 

after Andrew suggestio now it is 16EiB. and 16Ei  for packets

from ipsec status 

ipsec_max_bytes: 2Ki; ipsec_max_packets: 16Ei

> make it display 0 or <unset>   ?

-antony