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@xxxxxxxxxxxxx> wrote: > > > > > > > > On Fri, Sep 13, 2024 at 12:41:01PM +0200, Florian Westphal wrote: > > > > > Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote: > > > > > > On Fri, Sep 13, 2024 at 12:23:47PM +0200, Florian Westphal wrote: > > > > > > > Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> 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@xxxxxx> > > 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@xxxxxxxxxxxxxxx ML too.
