Message-ID: <ZhbFVGc8p9u0xQcv@Antony2201.local>
Date: Wed, 10 Apr 2024 18:59:00 +0200
From: Antony Antony <antony@...nome.org>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: Antony Antony <antony@...nome.org>,
	Nicolas Dichtel <nicolas.dichtel@...nd.com>,
	Antony Antony <antony.antony@...unet.com>,
	Herbert Xu <herbert@...dor.apana.org.au>, netdev@...r.kernel.org,
	devel@...ux-ipsec.org, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [devel-ipsec] [PATCH ipsec-next v6] xfrm: Add Direction to the
 SA in or out

On Wed, Apr 10, 2024 at 10:56:34AM +0200, Sabrina Dubroca wrote:
> 2024-04-09, 19:23:04 +0200, Antony Antony wrote:
> > On Mon, Apr 08, 2024 at 03:02:31PM +0200, Sabrina Dubroca wrote:
> > > 2024-04-07, 10:23:21 +0200, Antony Antony wrote:
> > > > Thank you for raising this point.  I thought that introducing a patch for 
> > > > the replay window check could stir more controversy, which might delay the 
> > > > acceptance of the essential 'dir' feature.
> > > 
> > > I'm not convinced it's *that* critical. People have someone managed to
> > 
> > Understood, but from a user's perspective, I've seen significant confusion 
> > around this issue. Labeling it as 'historical' and unchangeable ignores its 
> > real impact on usability. I feel adding "dir"  would help a lot.
> 
> Sure. I meant that also in relation to IPTFS development:
> 
> > > use IPsec without it for all those years. Is the intention to only
> > > allow setting up IPTFS SAs when this new 'dir' attribute is provided?
> > > If not, then this patch is not really blocking for IPTFS.
> 
> 
> [...]
> > > > For non-ESN scenarios, the outbound SA should have a replay window set to 0?
> > > 
> > > Looks ok.
> > > 
> > > > And for ESN 1?
> > > 
> > > Why 1 and not 0?
> > 
> > Current implemenation does not allow 0.
> 
> So we have to pass a replay window even if we know the SA is for
> output? That's pretty bad.

we can default to 1 with ESN and when no replay-window is specified.  

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

> > Supporting 0 is also a long standing argument:)
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/xfrm/xfrm_replay.c#n781
> > 
> > int xfrm_init_replay(struct xfrm_state *x, struct netlink_ext_ack *extack)
> > 
> > 781                         if (replay_esn->replay_window == 0) {
> > 782                                 NL_SET_ERR_MSG(extack, "ESN replay window must be > 0");
> > 783                                 return -EINVAL;
> > 784                         }
> > 
> > > Does setting xfrm_replay_state_esn->{oseq,oseq_hi} on an input SA (and
> > > {seq,seq_hi} on output) make sense?
> > 
> > Good point. I will add  {seq,seq_hi} validation. I don't think we add a for 
> > {oseq,oseq_hi} as it might be used by strongSwan with: ESN  replay-window 1, 
> > and migrating an SA.
> 
> I'm not at all familiar with that. Can you explain the problem?

strongSwan sets ESN and replay-window 1 on "out" SA. Then to migrgate, when 
IKEv2  mobike exchange succeds, it use GETSA read {oseq,oseq_hi} and the 
attributes, delete this SA.  Then create a new SA, with a different end 
point, and with old SA's {oseq,oseq_hi} and other parameters(curlft..).  
While Libreswan and Android use XFRM_MSG_MIGRATE.

> Also, this is a new bit of API. We don't have to accept strange
> configs with it, userspace should adapt to the strict rules we
> require.
> 
> > > And xfrm_state_update probably needs to check that the dir value
> > > matches?  If we get this far we know the new state had matching
> > > direction and properties, but maybe the old one didn't?
> > 
> > Yes. I will add this too.
> > 
> > > In XFRMA_SA_EXTRA_FLAGS, both XFRM_SA_XFLAG_DONT_ENCAP_DSCP and
> > > XFRM_SA_XFLAG_OSEQ_MAY_WRAP look like they're only used on output. A
> > > few of the flags defined with xfrm_usersa_info also seem to work only
> > > in one direction (XFRM_STATE_DECAP_DSCP, XFRM_STATE_NOPMTUDISC,
> > > XFRM_STATE_ICMP).
> > 
> > I'm familiar with one flag, but my knowledge on the rest is limited, still I 
> > believe they aren't direction-specific. If anyone has more specific insight, 
> > please do share. Are any of these flags or x flags direction specific?
> 
> [I typically wait for answers to my questions before I post the next
> version of a patch. Otherwise, reviewers have to do more work, looking
> at each version.]

I will not post v10 yet.

> BTW I just looked at all the flags defined in uapi, and asked cscope
> where they were used. For some, the function names were clearly only
> output path, for some just input, or both directions.

I looked closer at the flags and I noticed  several of them are direction 
specific.  And I am proposing to valide them and a simple data path check 
for directions in v10.

> [...]
> > > I think it would also make sense to only accept this attribute in
> > > xfrm_add_sa, and not for any of the other message types. Sharing
> > 
> > > xfrma_policy is convenient but not all attributes are valid for all
> > > requests. Old attributes can't be changed, but we should try to be
> > > more strict when we introduce new attributes.
> > 
> > To clarify your feedback, are you suggesting the API should not permit 
> > XFRMA_SA_DIR for methods like XFRM_MSG_DELSA, and only allow it for 
> > XFRM_MSG_NEWSA and XFRM_MSG_UPDSA? I added XFRM_MSG_UPDSA, as it's used 
> > equivalently to XFRM_MSG_NEWSA by *swan.
> 
> Not just DELSA, also all the *POLICY, ALLOCSPI, FLUSHSA, etc. NEWSA
> and UPDSA should accept it, but I'm thinking none of the other
> operations should. It's a property of SAs, not of other xfrm objects.

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.

Powered by blists - more mailing lists