Re: [PATCH nf] netfilter: nft_tproxy: make it terminal

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Fri, Sep 13, 2024 at 02:00:46PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Fri, Sep 13, 2024 at 01:24:25PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Sep 13, 2024 at 01:02:02PM +0200, Antonio Ojea wrote:
> > > On Fri, 13 Sept 2024 at 12:47, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > >
> > > > On Fri, Sep 13, 2024 at 12:41:01PM +0200, Florian Westphal wrote:
> > > > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > > > On Fri, Sep 13, 2024 at 12:23:47PM +0200, Florian Westphal wrote:
> > > > > > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > > > > > tproxy action must be terminal since the intent of the user to steal the
> > > > > > > > traffic and redirect to the port.
> > > > > > > > Align this behaviour to iptables to make it easier to migrate by issuing
> > > > > > > > NF_ACCEPT for packets that are redirect to userspace process socket.
> > > > > > > > Otherwise, NF_DROP packet if socket transparent flag is not set on.
> > > > > > >
> > > > > > > The nonterminal behaviour is intentional. This change will likely
> > > > > > > break existing setups.
> > > > > > >
> > > > > > > nft add rule filter divert tcp dport 80 tproxy to :50080 meta mark set 1 accept
> > > > > > >
> > > > > > > This is a documented example.
> > > > > >
> > > > > > Ouch. Example could have been:
> > > > > >
> > > > > >   nft add rule filter divert tcp dport 80 socket transparent meta set 1 tproxy to :50080
> > > > >
> > > > > Yes, but its not the same.
> > > > >
> > > > > With the statements switched, all tcp dport 80 have the mark set.
> > > > > With original example, the mark is set only if tproxy found a
> > > > > transparent sk.
> > > >
> > > > Indeed, thanks for correcting me.
> > > >
> > > > I'm remembering now why this was done to provide to address the ugly
> > > > mark hack that xt_TPROXY provides.
> > > >
> > > > While this is making harder to migrate, making it non-terminal is
> > > > allowing to make more handling such as ct/meta marking after it.
> > > >
> > > > I think we just have to document this in man nft(8).
> > > 
> > > I think that at this point in time the current state can not be broken
> > > based on this discussion, I just left the comment in the bugzilla
> > > about the possibility but it is clear now that people that have
> > > already started using this feature with nftables must not experience a
> > > disruption.
> > > On the other side, users that need to migrate will have to adapt more
> > > things so I don't think it should be a big deal.
> > > What I really think is that users should have a way to terminate
> > > processing to avoid other rules to interfere with the tproxy
> > > functionality
> > 
> > It is possible to add an explicit 'accept' verdict as the example
> > above displays:
> > 
> >         tcp dport 80 tproxy to :50080 meta mark set 1 accept
> >                                                       ^^^^^^
> 
> I wonder if this is sufficient: The packet will still appear in
> following chains, etc. So shouldn't one use 'drop' verdict instead or
> does that prevent the proxying somehow?
>
> Hmm. Looking at nft_nat.c, 'accept' seems consistent with what nat
> statements do implicitly.

Yes, and xt_TPROXY does NF_ACCEPT.

On the other hand, I can see it does NF_DROP it socket is not
transparent, it does NFT_BREAK instead, so policy keeps evaluating the
packet.

> > is this sufficient in your opinion? If so, I made this quick update
> > for man nft(8).
> 
> Acked-by: Phil Sutter <phil@nwl.cc>
>
> In addition to that, I will update tproxy_tg_xlate() in iptables.git to
> emit a verdict, too.

Thanks, this is very convenient.

> Also I should update the respective wiki article[1] once more with added
> translation testsuite links - at least the one for TPROXY is missing.

Great, thanks.

I still have to update wiki to extend set element timeout with the
recent information I provided in netfilter@vger.kernel.org ML too.