Message-ID: <ZhV5eG2pkrsX0uIV@Antony2201.local>
Date: Tue, 9 Apr 2024 19:23:04 +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 Mon, Apr 08, 2024 at 03:02:31PM +0200, Sabrina Dubroca wrote:
> 2024-04-07, 10:23:21 +0200, Antony Antony wrote:
> > Hi Sabrina,
> > 
> > On Fri, Apr 05, 2024 at 11:56:00PM +0200, Sabrina Dubroca via Devel wrote:
> > > Hi Antony,
> > > 
> > > 2024-04-05, 14:40:07 +0200, Antony Antony wrote:
> > > > This patch introduces the 'dir' attribute, 'in' or 'out', to the
> > > > xfrm_state, SA, enhancing usability by delineating the scope of values
> > > > based on direction. An input SA will now exclusively encompass values
> > > > pertinent to input, effectively segregating them from output-related
> > > > values. 
> > > 
> > > But this patch isn't doing that for existing properties (I'm thinking
> > > of replay window, not sure if any others are relevant [1]). Why not?
> > 
> > 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.

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

> And yes, people will sometimes make comments on patches that cause
> delays in getting the patches accepted. Some patches even end up
> getting rejected. The kernel is better thanks to that process, even if
> it can be annoying to the submitter (including me! it would be a lot
> more relaxing if my patches always just went in at v1 :)).
> 
> Nicolas, since you were objecting to the informational nature of the
> attribute in v5: would you still object to the new attribute (and not
> just limited to offload cases) if it properly restricted attributes
> that don't match the direction?
> 
> >  My primary goal at this stage is 
> > to get this basic feature  in and to convince Chris to integrate the "dir" 
> > attribute into IP-TFS. This patch has partly contributed to the delays in 
> > IP-TFS's development.
> > 
> > Given your input, I'm curious about the specific conditions you have in 
> > mind. See the attached patch.
> 
> I didn't look into details when I wrote that email. The rough idea was
> "Whatever makes the kernel do replay protection on incoming packets"
> (and if any attribute is output-only, skip those on input SAs).
> 
> > 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. Though supporting 0 is higly desired 
feature and probably a hard to implement feature in xfrm code. 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.

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

 - XFRM_STATE_ICMP should not be allowed on "out" SA. This is good point. I 
   have seen users getting confused about this.

> > non-ESN
> > ip xfrm state add src 10.1.3.4 dst 10.1.2.3 proto esp spi 3 reqid 2  \
> > mode tunnel dir out aead 'rfc4106(gcm(aes))' \
> > 0x2222222222222222222222222222222222222222 96 if_id 11 replay-window 10
> > Error: Replay-window too big > 0 for OUT SA.
> 
> I'd probably change the string to "Replay window should not be set for
> OUT SA", that makes a bit more sense to me. "too big" implies that
> some values are valid, which isn't really the case.
>

good point. fixed in v8.

> > The current impelementation does not replay window 0 with ESN.  Even though 
> > disabling replay window with ESN is a desired feature.
> > 
> > ip xfrm state add src 10.1.3.4 dst 10.1.2.3 proto esp spi 3 reqid 2 mode 
> > tunnel dir out flag esn  aead  'rfc4106(gcm(aes))'  \
> > 0x2222222222222222222222222222222222222222 96 if_id 11 replay-window 10
> > Error: ESN replay-window too big > 1 for OUT SA.ww
> > 
> > I wonder would the attached patch get accepted quickly.
> 
> I'm more interested in getting things right if we're going to
> introduce a new bit of API. IMO a new "dir" attribute that doesn't
> fully lock down the options on an SA is worse than no attribute (as
> Nicolas said previously, it's really confusing).
> And I'm not that familiar with all the API for xfrm SAs, so the
> properties I listed above may not be everything we should lock down
> (and maybe some are wrong).
> 
> 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.

> Sorry that I didn't notice this when you posted the previous versions.

thanks your review.

-antony

Powered by blists - more mailing lists