[Swan-dev] Ipcomp and get_sa_info()

[D. Hugh Redelmeier]

| From: Paul Wouters <paul at nohats.ca>

| On Mar 25, 2022, at 23:08, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
| > 
| > |BTW, which code do you suspect?
| 
| Based on commit message, removal of get_sa_info() for ipcomp 

It didn't do anything.  get_sa_info ignored IPCOMP.

All three pairs of calls to get_sa_info were identical, whether they
were prompted by AH, ESP, or IPCOMP.  So the result would be for AH
unless it didn't exist, otherwise ESP, unless it didn't exist,
otherwise nothing.

| > I think that all the code that I changed was about logging or
| > reporting in some way.  It had no effect on actual flows.
| 
| Possible it was broken already but even more now ?

I had / have no idea what it was supposed to do.  I'm unaware of any 
specifications.  I just removed nonsense.

| > Isn't what you describe is kernel behaviour, not Pluto behaviour?
| 
| Yes but it means to set traffic count for an SA with an IPCOMP SA, we 
| might need two get_sa_info() calls.

I don't know what you mean.

Ignoring the inbound/outbound issue, calls to get_sa_info are essentially 
idempotent.  A second call adds nothing.

I suspect that you don't remember that get_sa_info only gets info for 
the first SA in a bundle.  I've renamed it to help make this clear.

As far as I know, IPCOMP is never the first SA in a bundle.  Even if it 
were, get_sa_info would never query for stats of an IPCOMP SA.

Maybe that should change.  I have no idea if the kernel_ops.get_sa works 
for IPCOMP.

| > There were no flow counts reported for ipcomp.  You may want them but
| > they weren't there.  
| 
| Yes that is what I suspect.

I just hacked code in show_established_child_details to show the flow 
through the first SA of the bundle as flow through IPCOMP (if there is an 
IPCOMP SA in the bundle).  This is, of course, wrong: compression changes 
the number of bytes.

| > The code in get_sa_info only dealt with one SA.  ESP if present, and
| > if not, AH, and if not, it went home.  IPCOMP was not considered.
| 
| It should have extended the ipcomp instead of removing it ?

Does kernel_ops.get_sa work for ipcomp?

| > AH+ESP isn't handled
| 
| It is no longer supported or negotiated by us since prob 10-15 years.

Yeah, but the old structure is still there and can represent AH+ESP.
IKEV1 code used to support both at once.  Has that been eliminated?

It would be good to change the representation to only support one
crypto SA and optionally one IPCOMP.  Not worth the effort at any
particular time.

| > My *guess* is that IPCOMP could be present along with ESP or AH, but
| > the get_sa_info code doesn't care.
| 
| Yes I think that is the bug but now you removed code that should have been extended instead ? :)
| 
| Anyway, not a bug deal. We should file a bug so we won’t forget

It's not hard to fix if you know that it is the right thing to do and
you know kernel_ops.get_sa works for ipcomp.