Message ID | 20240913102023.3948-1-pablo@netfilter.org |
---|---|
State | Not Applicable |
Headers | show |
Series | [nf] netfilter: nft_tproxy: make it terminal | expand |
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.
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
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...
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.
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).
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
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).
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
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.
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.
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 .
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
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
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
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(-)