[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zhe9LB97ik37hM3q@Antony2201.local>
Date: Thu, 11 Apr 2024 12:36:28 +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 Thu, Apr 11, 2024 at 11:24:06AM +0200, Sabrina Dubroca wrote:
> 2024-04-10, 18:59:00 +0200, Antony Antony wrote:
> > On Wed, Apr 10, 2024 at 10:56:34AM +0200, Sabrina Dubroca wrote:
> > > 2024-04-09, 19:23:04 +0200, Antony Antony wrote:
> > > > 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.
>
> Ok, thanks. But that's still an output SA. Setting {oseq,oseq_hi} on
> an input SA is bogus I would think?
Corrrect, It is not allowed in v10.
>
> > > > > 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.
>
> No, I wouldn't introduce validation of other attributes. It doesn't
> belong in this patch(set), and I'm not sure we can add it now as it
> might break userspace (I don't see why userspace would pass
> XFRMA_ALG_AEAD etc on a DELSA request, but if we never rejected it,
> they could).
>
> But rejecting this new attribute from messages that don't handle it
> would be good, and should be done in this patch/series.
Definitely see the value in such feature in general, but it seems ambitious
for this patch set. Currently, only NEWSA, UPDSA, and ALLOCSPI need
XFRMA_SA_DIR. I am wondering how to reject this atrribute in remaining 20-22
messages. Is there a precedent or example in xfrm_user.c for this kind of
validation, or maybe a Netlink feature that lets us restrict NL attributes
for a specific messages like DELSA.
If not, it feels like a seperate patch set for general API cleanup.
Powered by blists - more mailing lists