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

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

[-- Attachment #1: Type: text/plain, Size: 2624 bytes --]

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

is this sufficient in your opinion? If so, I made this quick update
for man nft(8).

[-- Attachment #2: nft-doc.patch --]
[-- Type: text/x-diff, Size: 651 bytes --]

diff --git a/doc/statements.txt b/doc/statements.txt
index 5becf0cbdbcf..3c5059ead608 100644
--- a/doc/statements.txt
+++ b/doc/statements.txt
@@ -604,6 +604,11 @@ table inet x {
 }
 -------------------------------------
 
+Note that the tproxy statement is non-terminal to allow post-processing of
+packets, such as updating the packet marking. This is a change in behavior
+compared to the legacy iptables TPROXY target which is terminal. To terminate
+the packet processing after the tproxy statement, remember to issue a verdict.
+
 SYNPROXY STATEMENT
 ~~~~~~~~~~~~~~~~~~
 This statement will process TCP three-way-handshake parallel in netfilter