Re: [devel-ipsec] [PATCH ipsec-next v6] xfrm: Add Direction to the SA in or out

[Sabrina Dubroca <sd@queasysnail.net>]

2024-04-10, 20:58:33 -0400, Paul Wouters wrote:
> On Wed, 10 Apr 2024, Antony Antony via Devel wrote:
> > > > Though supporting 0 is higly desired
> > > > feature and probably a hard to implement feature in xfrm code.
> > > 
> > > Why would it be hard for outgoing SAs? The replay window should never
> > > be used on those. And xfrm_replay_check_esn and xfrm_replay_check_bmp
> > > already have checks for 0-sized replay window.
> > 
> > That information comes from hall way talks with Steffen. I can't explain
> > it:) May be he can elaborate why 0 is not allowed with ESN.
> 
> With ESN, you use a 64 bit number but only send a 32 bit number over the
> wire. So you need to "track" the parts not being sent to do the proper
> packet authentication that uses the full 64bit number. The
> authentication bit is needed for encrypting and decrypting, so on both
> the incoming and outgoing SA.
> 
> AFAIK, this 64 bit number tracking is done using the replay-window code.
> That is why replay-window cannot be 0 when ESN is enabled in either
> direction of the SA.

It's in the replay-window code, but AFAICT it doesn't use the
replay_window variable at all (xfrm_output calls into the
xfrm_replay_overflow_* functions which only look at oseq, xfrm_input
calls the *check and *advance functions of xfrm_replay.c). So I think
we could accept an unset replay_window for an output SA.

> I have already poked Steffen it would be good to decouple ESN code from
> replay-window code, as often people want to benchmark highspeed links
> by disabling replay protection completely, but then they are also
> unwittingly disabling ESN and causing needing a rekey ever 2 minutes
> or so on a modern 100gbps ipsec link.
> 
> > strongSwan sets ESN and replay-window 1 on "out" SA.
> 
> It has to set a replay-window of non-zero or else ESN won't work.
> It is not related to migration AFAIK.
> 
> > For instance, there isn't a validation for unused XFRMA_SA_EXTRA_FLAGS in
> > DELSA; if set, it's simply ignored. Similarly, if XFRMA_SA_DIR were set in
> > DELSA, it would also be disregarded. Attempting to introduce validations for
> > DELSA and other methods seems like an extensive cleanup task. Do we consider
> > this level of validation within the scope of our current patch? It feels
> > like we are going too far.
> 
> Is there a way where rate limited logging can be introduced, so that
> userlands will clean up their use and after a few years change the API
> to not allow setting bogus values?

Yes, this is doable. Steffen, does that seem reasonable? (for example,
when XFRMA_REPLAY_THRESH is passed to NEWSA, or XFRMA_ALG_AEAD to
DELSA, etc)

(as part of a separate patchset of course)

-- 
Sabrina