Re: [iptables RFC PATCH 8/8] nft: Support compat extensions in rule userdata

[Phil Sutter <phil@nwl.cc>]

Hi Pablo,

On Mon, Sep 16, 2024 at 12:13:07AM +0200, Pablo Neira Ayuso wrote:
> I have no better idea to cope with this forward compatibility
> requirements.

From my point of view, it's the best approach among the bad ones (i.e.,
those not providing compatibility for old binaries out of the box).

> On Thu, Aug 01, 2024 at 12:27:03AM +0200, Phil Sutter wrote:
> > Add a mechanism providing forward compatibility for the current and
> > future versions of iptables-nft (and all other nft-variants) by
> > annotating nftnl rules with the extensions they were created for.
> >
> > Upon nftnl rule parsing failure, warn about the situation and perform a
> > second attempt loading the respective compat extensions instead of the
> > native expressions which replace them.
> 
> OK, so this is last resort to interpret the rule.

It is. I had your concerns regarding crafted compat payload in rules in
mind with this. The downside is that we may make subtle changes to VM
code which the old binary won't detect and ignore. Maybe I could feature
it via flag or env var to prefer the userdata extensions. WDYT?

> > The foundational assumption is that libxtables extensions are stable
> > and thus the VM code created on their behalf does not need to be.
> 
> OK, this requires xtables API becomes frozen forever.

Well, not more than it has been: Take iptables-legacy for instance: An
old version may see a newer extension revision than it has support for,
so will fail just like iptables-nft with native code it can't parse. So
effectively iptables-nft becomes *as compatible as* the same version of
iptables-legacy.
Another perspective to this: Extension development gradually slows down
which we'll leverage while at the same time support increased
development in nftables itself and conversion of extensions to VM code.

> > Since nftnl rule userdata attributes are restricted to 255 bytes, the
> > implementation focusses on low memory consumption. Therefore, extensions
> > which remain in the rule as compat expressions are not also added to
> > userdata. In turn, extensions in userdata are annotated by start and end
> > expression number they are replacing. Also, the actual payload is
> > zipped using zlib.
> 
> Binary layout is better than storing text in the userdata area.
> 
> Is this zlib approach sufficient to cope with ebtables among
> extension? Maybe that one is excluded because it is using the set
> infrastructure since the beginning.

Yes, among is not an issue because ebtables-nft never implemented this
as extension.

> I guess you already checked for worst case to make sure compression
> always allows to make things fit into 255 bytes?

Well, we don't convert too many extensions to nftables anymore since
Florian reverted a bunch as a quick countermeasure against the compat
complaints. Things will certainly get worse, but the question is mostly
how many different extensions will "the largest rule" have. I added some
debug output and in shell testsuite for instance the largest compat_ext
userdata I see is 68 bytes. I was able to craft a rule which uses 159
bytes though:

iptables-nft -A FORWARD \
	-m limit --limit 1000/hour \
	-p udp -m udp --sport 1024:65535 --dport 4:65535 \
	-m mark --mark 0xfeedcafe/0xfeedcafe \
	-j NFLOG --nflog-group 23 \
	         --nflog-prefix "this is a damn long log prefix"

The current implementation calls xtables_error() if nftnl_udata_put()
fails. Maybe a better error path would be to only warn the user and not
add compat_ext to userdata. Guess it depends on whether this should be
enabled by default or only upon request - if user asks for compat_ext,
failing to do so should be fatal.

Cheers, Phil