[PATCH nf] netfilter: nft_tproxy: make it terminal

[PATCH nf] netfilter: nft_tproxy: make it terminal

[Pablo Neira Ayuso]

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.

Fixes: 4ed8eb6570a4 ("netfilter: nf_tables: Add native tproxy support")
Reported-by: Antonio Ojea <antonio.ojea.garcia@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nft_tproxy.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/nft_tproxy.c b/net/netfilter/nft_tproxy.c
index 71412adb73d4..f3b563c379d8 100644
--- a/net/netfilter/nft_tproxy.c
+++ b/net/netfilter/nft_tproxy.c
@@ -74,10 +74,13 @@ static void nft_tproxy_eval_v4(const struct nft_expr *expr,
 					   skb->dev, NF_TPROXY_LOOKUP_LISTENER);
 	}
 
-	if (sk && nf_tproxy_sk_is_transparent(sk))
+	if (sk && nf_tproxy_sk_is_transparent(sk)) {
 		nf_tproxy_assign_sock(skb, sk);
-	else
-		regs->verdict.code = NFT_BREAK;
+		regs->verdict.code = NF_ACCEPT;
+		return;
+	}
+
+	regs->verdict.code = NF_DROP;
 }
 
 #if IS_ENABLED(CONFIG_NF_TABLES_IPV6)
@@ -147,10 +150,13 @@ static void nft_tproxy_eval_v6(const struct nft_expr *expr,
 	}
 
 	/* NOTE: assign_sock consumes our sk reference */
-	if (sk && nf_tproxy_sk_is_transparent(sk))
+	if (sk && nf_tproxy_sk_is_transparent(sk)) {
 		nf_tproxy_assign_sock(skb, sk);
-	else
-		regs->verdict.code = NFT_BREAK;
+		regs->verdict.code = NF_ACCEPT;
+		return;
+	}
+
+	regs->verdict.code = NF_DROP;
 }
 #endif
 
-- 
2.30.2

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

[Florian Westphal]

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.

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

[Pablo Neira Ayuso]

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

but it is too late.

Quick idea: extend tproxy extension to add a flag to consume it:

  tproxy to :50080 flags consume

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

[Pablo Neira Ayuso]

On Fri, Sep 13, 2024 at 12:29:02PM +0200, Pablo Neira Ayuso 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
> 
> but it is too late.
> 
> Quick idea: extend tproxy extension to add a flag to consume it:
> 
>   tproxy to :50080 flags consume

Scratch that, this is very silly: it is not different than just adding
'accept' after it...

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

[Florian Westphal]

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.

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

[Pablo Neira Ayuso]

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

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

[Antonio Ojea]

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

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

[Pablo Neira Ayuso]

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

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

[Phil Sutter]

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.

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

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

Cheers, Phil

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

[Pablo Neira Ayuso]

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.

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

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > 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.

Yes, this is more flexible since you can log+drop for instance in next
rule(s) to alert that proxy isn't running for example.

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

Agreed, it should append accept keyword in the translator.

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

[Antonio Ojea]

On Fri, 13 Sept 2024 at 16:18, Florian Westphal <fw@strlen.de> wrote:
>
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > 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.
>
> Yes, this is more flexible since you can log+drop for instance in next
> rule(s) to alert that proxy isn't running for example.
>

This is super useful, in the scenario that the transparent proxy takes
over the DNATed virtual IP, if the transparent proxy process is not
running the packet goes to the DNATed virtual IP so the clients don't
observe any disruption.

> > > > 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.
>
> Agreed, it should append accept keyword in the translator.

I'm not completely following the technical details sorry.

With my current configuration I do set an accept action

   udp dport 53 tproxy ip to 127.0.0.1:46659 accept

and I have dnat statements after that action.

The packet is "proxied" but "nftrace monitor" shows that the
subsequent rules are evaluated and I also can observe that the NAT
entries are added on the conntrack table despite the packet being
proxied.

I think that there has to be a way to indicate that after some point
the subsequent rules can not take actions, per example, the conntrack
entries generated by the DNAT rules, at least with UDP, interfere with
subsequent packets .

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

[Phil Sutter]

Hi Antonio,

On Fri, Sep 13, 2024 at 05:38:21PM +0200, Antonio Ojea wrote:
> On Fri, 13 Sept 2024 at 16:18, Florian Westphal <fw@strlen.de> wrote:
> >
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > 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.
> >
> > Yes, this is more flexible since you can log+drop for instance in next
> > rule(s) to alert that proxy isn't running for example.
> >
> 
> This is super useful, in the scenario that the transparent proxy takes
> over the DNATed virtual IP, if the transparent proxy process is not
> running the packet goes to the DNATed virtual IP so the clients don't
> observe any disruption.

So here's a use-case for why non-terminal tproxy statement is superior
over terminal one. :)

> > > > > 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.
> >
> > Agreed, it should append accept keyword in the translator.
> 
> I'm not completely following the technical details sorry.

In essence, tproxy statement does not set a verdict unless it fails to
find a suitable socket. A sample ruleset illustrating this is:

| table t {
| 	chain c {
| 		type filter hook prerouting priority 0
| 		tproxy to :1234 log "packet tproxied" accept
| 		log "no socket on port 1234 or not transparent?" drop
| 	}
| }

> With my current configuration I do set an accept action
> 
>    udp dport 53 tproxy ip to 127.0.0.1:46659 accept
> 
> and I have dnat statements after that action.

For the record, your ruleset looks like this (quoting from the kselftest
you sent me):

| table inet filter {
|        chain divert {
|                type filter hook prerouting priority -100; policy accept;
|                $ip_proto daddr $virtual_ip udp dport 8080 tproxy ip_proto to :12345 meta mark set 1 accept
|        }
|        # Removing this chain makes the first connection to succeed
|        chain PREROUTING {
|                type nat hook prerouting priority 1; policy accept;
|                $ip_proto daddr $virtual_ip udp dport 8080 dnat to umgen inc mod 2 map { 0 : $ns2_ip , 1: $ns3_ip }
|        }
| }

Foundational lecture: 'accept' verdict covers the current hook only. Like
with iptables, if you accept in e.g. PREROUTING, INPUT may still see the
packet. 'drop' verdict OTOH discards the packet, so no following hooks
will see it (obviously).

Your case is special because of the different types. If I interpret this
correctly, a new connection's packet will get tproxied by divert chain
and dnatted by PREROUTING chain (which sets up a conntrack entry). The
second packet will hit conntrack in prerouting hook at priority -200
(according to the big picture[1]) and your tproxy rule does not match
anymore. The nat type chain does not see the packet as it's not a new
connection. Maybe this explains the behaviour you're seeing.

In order to avoid the inadvertent dnat of tproxied packets, terminating
the divert chain's rule with 'drop' instead of 'accept' should do - if
tproxy fails, it set NFT_BREAK so the packet continues and hits
PREROUTING chain (if the connection is new).

Cheers, Phil

[1] https://people.netfilter.org/pablo/nf-hooks.png

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

[Antonio Ojea]

On Fri, 13 Sept 2024 at 21:35, Phil Sutter <phil@nwl.cc> wrote:
>
> Hi Antonio,
>
> On Fri, Sep 13, 2024 at 05:38:21PM +0200, Antonio Ojea wrote:
> > On Fri, 13 Sept 2024 at 16:18, Florian Westphal <fw@strlen.de> wrote:
> > >
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > > 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.
> > >
> > > Yes, this is more flexible since you can log+drop for instance in next
> > > rule(s) to alert that proxy isn't running for example.
> > >
> >
> > This is super useful, in the scenario that the transparent proxy takes
> > over the DNATed virtual IP, if the transparent proxy process is not
> > running the packet goes to the DNATed virtual IP so the clients don't
> > observe any disruption.
>
> So here's a use-case for why non-terminal tproxy statement is superior
> over terminal one. :)
>
> > > > > > 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.
> > >
> > > Agreed, it should append accept keyword in the translator.
> >
> > I'm not completely following the technical details sorry.
>
> In essence, tproxy statement does not set a verdict unless it fails to
> find a suitable socket. A sample ruleset illustrating this is:
>
> | table t {
> |       chain c {
> |               type filter hook prerouting priority 0
> |               tproxy to :1234 log "packet tproxied" accept
> |               log "no socket on port 1234 or not transparent?" drop
> |       }
> | }
>
> > With my current configuration I do set an accept action
> >
> >    udp dport 53 tproxy ip to 127.0.0.1:46659 accept
> >
> > and I have dnat statements after that action.
>
> For the record, your ruleset looks like this (quoting from the kselftest
> you sent me):
>
> | table inet filter {
> |        chain divert {
> |                type filter hook prerouting priority -100; policy accept;
> |                $ip_proto daddr $virtual_ip udp dport 8080 tproxy ip_proto to :12345 meta mark set 1 accept
> |        }
> |        # Removing this chain makes the first connection to succeed
> |        chain PREROUTING {
> |                type nat hook prerouting priority 1; policy accept;
> |                $ip_proto daddr $virtual_ip udp dport 8080 dnat to umgen inc mod 2 map { 0 : $ns2_ip , 1: $ns3_ip }
> |        }
> | }
>
> Foundational lecture: 'accept' verdict covers the current hook only. Like
> with iptables, if you accept in e.g. PREROUTING, INPUT may still see the
> packet. 'drop' verdict OTOH discards the packet, so no following hooks
> will see it (obviously).
>
> Your case is special because of the different types. If I interpret this
> correctly, a new connection's packet will get tproxied by divert chain
> and dnatted by PREROUTING chain (which sets up a conntrack entry). The
> second packet will hit conntrack in prerouting hook at priority -200
> (according to the big picture[1]) and your tproxy rule does not match
> anymore. The nat type chain does not see the packet as it's not a new
> connection. Maybe this explains the behaviour you're seeing.
>
> In order to avoid the inadvertent dnat of tproxied packets, terminating
> the divert chain's rule with 'drop' instead of 'accept' should do - if
> tproxy fails, it set NFT_BREAK so the packet continues and hits
> PREROUTING chain (if the connection is new).

Awesome, I just add a rule that drops the tproxied marked packets

| meta mark 1 drop

and works perfectly.

Although, since it is not really intuitive you have to do this it will
be nice to have it documented

Thanks

>
> Cheers, Phil
>
> [1] https://people.netfilter.org/pablo/nf-hooks.png