| Message ID | 20211001174142.1267726-1-jeremy@azazel.net |
|---|---|
| Headers | show |
| Series | extensions: libxt_NFLOG: use nft back-end for iptables-nft | expand |
On 2021-10-01, at 18:41:34 +0100, Jeremy Sowden wrote: > nftables supports 128-character prefixes for nflog whereas legacy > iptables only supports 64 characters. This patch series converts > iptables-nft to use the nft back-end in order to take advantage of the > longer prefixes. > > * Patches 1-5 implement the conversion and update some related Python > unit-tests. > * Patch 6 fixes an minor bug in the output of nflog prefixes. > * Patch 7 contains a couple of libtool updates. > * Patch 8 fixes some typo's. I note that Florian merged the first patch in this series recently. Feedback on the rest of it would be much appreciated. J. > Changes since v1: > > * Patches 1 and 5-8 are new. > * White-space fixes in patches 2 and 3. > * Fixes for typo's in commit-messages of patches 2 and 4. > * Removal of stray `struct xt_nflog_info` allocation from > `nft_parse_log` in patch 3. > * Leave commented-out `--nflog-range` test-cases in libxt_NFLOG.t > with an explanatory comment in patch 4. > > Jeremy Sowden (5): > nft: fix indentation error. > extensions: libxt_NFLOG: fix `--nflog-prefix` Python test-cases > extensions: libxt_NFLOG: remove extra space when saving targets with > prefixes > build: replace `AM_PROG_LIBTOOL` and `AC_DISABLE_STATIC` with > `LT_INIT` > tests: iptables-test: correct misspelt variable > > Kyle Bowman (3): > extensions: libxt_NFLOG: use nft built-in logging instead of xt_NFLOG > extensions: libxt_NFLOG: don't truncate log prefix on print/save > extensions: libxt_NFLOG: disable `--nflog-range` Python test-cases > > configure.ac | 3 +- > extensions/libxt_NFLOG.c | 8 ++++- > extensions/libxt_NFLOG.t | 16 ++++----- > iptables-test.py | 18 +++++----- > iptables/nft-shared.c | 52 ++++++++++++++++++++++++++++ > iptables/nft.c | 74 ++++++++++++++++++++++++++++------------ > iptables/nft.h | 1 + > 7 files changed, 131 insertions(+), 41 deletions(-) > > -- > 2.33.0 > >
Jeremy Sowden <jeremy@azazel.net> wrote: > On 2021-10-01, at 18:41:34 +0100, Jeremy Sowden wrote: > > nftables supports 128-character prefixes for nflog whereas legacy > > iptables only supports 64 characters. This patch series converts > > iptables-nft to use the nft back-end in order to take advantage of the > > longer prefixes. > > > > * Patches 1-5 implement the conversion and update some related Python > > unit-tests. > > * Patch 6 fixes an minor bug in the output of nflog prefixes. > > * Patch 7 contains a couple of libtool updates. > > * Patch 8 fixes some typo's. > > I note that Florian merged the first patch in this series recently. Yes, because it was a cleanup not directly related to the rest. I've now applied the last patch as well for the same reason. > Feedback on the rest of it would be much appreciated. THe patches look ok to me BUT there is the political issue that we will now divert, afaict this means that you can now create iptables-nft rulesets that won't ever work in iptables-legacy. IMO its ok and preferrable to extending xt_(NF)LOG with a new revision, but it does set some precedence, so I'm leaning towards just applying the rest too. Pablo, Phil, others -- what is your take?
On Sun, Jan 16, 2022 at 08:08:15PM +0100, Florian Westphal wrote: > Jeremy Sowden <jeremy@azazel.net> wrote: > > On 2021-10-01, at 18:41:34 +0100, Jeremy Sowden wrote: > > > nftables supports 128-character prefixes for nflog whereas legacy > > > iptables only supports 64 characters. This patch series converts > > > iptables-nft to use the nft back-end in order to take advantage of the > > > longer prefixes. > > > > > > * Patches 1-5 implement the conversion and update some related Python > > > unit-tests. > > > * Patch 6 fixes an minor bug in the output of nflog prefixes. > > > * Patch 7 contains a couple of libtool updates. > > > * Patch 8 fixes some typo's. > > > > I note that Florian merged the first patch in this series recently. > > Yes, because it was a cleanup not directly related to the rest. > I've now applied the last patch as well for the same reason. > > > Feedback on the rest of it would be much appreciated. > > THe patches look ok to me BUT there is the political issue > that we will now divert, afaict this means that you can now create > iptables-nft rulesets that won't ever work in iptables-legacy. > > IMO its ok and preferrable to extending xt_(NF)LOG with a new revision, > but it does set some precedence, so I'm leaning towards just applying > the rest too. > > Pablo, Phil, others -- what is your take? I think the change is OK if existing rulesets will continue to work just as before and remain compatible with legacy. IMHO, new rulesets created using iptables-nft may become incompatible if users explicitly ask for it (e.g. by specifying an exceedingly long log prefix. What about --nflog-range? This series seems to drop support for it, at least in the sense that ruleset dumps won't contain the option. In theory, users could depend on identifying a specific rule via nflog range value. Cheers, Phil
On 2022-01-17, at 11:40:51 +0100, Phil Sutter wrote: > On Sun, Jan 16, 2022 at 08:08:15PM +0100, Florian Westphal wrote: > > Jeremy Sowden <jeremy@azazel.net> wrote: > > > On 2021-10-01, at 18:41:34 +0100, Jeremy Sowden wrote: > > > > nftables supports 128-character prefixes for nflog whereas > > > > legacy iptables only supports 64 characters. This patch series > > > > converts iptables-nft to use the nft back-end in order to take > > > > advantage of the longer prefixes. > > > > > > > > * Patches 1-5 implement the conversion and update some related > > > > Python unit-tests. > > > > * Patch 6 fixes an minor bug in the output of nflog prefixes. > > > > * Patch 7 contains a couple of libtool updates. > > > > * Patch 8 fixes some typo's. > > > > > > I note that Florian merged the first patch in this series > > > recently. > > > > Yes, because it was a cleanup not directly related to the rest. > > I've now applied the last patch as well for the same reason. Thanks for that. > > > Feedback on the rest of it would be much appreciated. > > > > THe patches look ok to me BUT there is the political issue that we > > will now divert, afaict this means that you can now create > > iptables-nft rulesets that won't ever work in iptables-legacy. > > > > IMO its ok and preferrable to extending xt_(NF)LOG with a new > > revision, Indeed. The original proposal from Cloudflare was to extend xt_NFLOG, but Pablo requested that iptables-nft be modified instead. Hence this series. > > but it does set some precedence, so I'm leaning towards just > > applying the rest too. > > > > Pablo, Phil, others -- what is your take? > > I think the change is OK if existing rulesets will continue to work > just as before and remain compatible with legacy. IMHO, new rulesets > created using iptables-nft may become incompatible if users explicitly > ask for it (e.g. by specifying an exceedingly long log prefix. > > What about --nflog-range? This series seems to drop support for it, at > least in the sense that ruleset dumps won't contain the option. In > theory, users could depend on identifying a specific rule via nflog > range value. Fair enough. I'll add a check so that nft is not used for targets that specify `--nflog-range`. J.
On Mon, Jan 17, 2022 at 09:54:52PM +0000, Jeremy Sowden wrote: > On 2022-01-17, at 11:40:51 +0100, Phil Sutter wrote: > > On Sun, Jan 16, 2022 at 08:08:15PM +0100, Florian Westphal wrote: [...] > > > Pablo, Phil, others -- what is your take? > > > > I think the change is OK if existing rulesets will continue to work > > just as before and remain compatible with legacy. IMHO, new rulesets > > created using iptables-nft may become incompatible if users explicitly > > ask for it (e.g. by specifying an exceedingly long log prefix. > > > > What about --nflog-range? This series seems to drop support for it, at > > least in the sense that ruleset dumps won't contain the option. In > > theory, users could depend on identifying a specific rule via nflog > > range value. > > Fair enough. I'll add a check so that nft is not used for targets that > specify `--nflog-range`. --nflog-range does work? --nflog-size is used and can be mapped to 'snaplen' in nft_log. Manpage also discourages the usage of --nflog-range for long time. Not sure it is worth to add a different path for this case.
On 2022-01-18, at 02:23:46 +0100, Pablo Neira Ayuso wrote: > On Mon, Jan 17, 2022 at 09:54:52PM +0000, Jeremy Sowden wrote: > > On 2022-01-17, at 11:40:51 +0100, Phil Sutter wrote: > > > On Sun, Jan 16, 2022 at 08:08:15PM +0100, Florian Westphal wrote: > [...] > > > > Pablo, Phil, others -- what is your take? > > > > > > I think the change is OK if existing rulesets will continue to > > > work just as before and remain compatible with legacy. IMHO, new > > > rulesets created using iptables-nft may become incompatible if > > > users explicitly ask for it (e.g. by specifying an exceedingly > > > long log prefix. > > > > > > What about --nflog-range? This series seems to drop support for > > > it, at least in the sense that ruleset dumps won't contain the > > > option. In theory, users could depend on identifying a specific > > > rule via nflog range value. > > > > Fair enough. I'll add a check so that nft is not used for targets > > that specify `--nflog-range`. > > --nflog-range does work? No. > --nflog-size is used and can be mapped to 'snaplen' in nft_log. Correct. > Manpage also discourages the usage of --nflog-range for long time. > > Not sure it is worth to add a different path for this case. Yes, there have been warnings about `--nflog-range` since `--nflog-size` was added in 2016. I wasn't entirely happy dropping `--nflog-range` support and introducing the divergence between -legacy and -nft as an incidental side-effect, so when Phil brought it up, I had a look and it turns out that the code to preserve support for it is quite small: --- a/iptables/nft.c +++ b/iptables/nft.c @@ -1366,6 +1366,12 @@ int add_log(struct nftnl_rule *r, struct iptables_command_state *cs) struct nftnl_expr *expr; struct xt_nflog_info *info = (struct xt_nflog_info *)cs->target->t->data; + if (info->len && !(info->flags & XT_NFLOG_F_COPY_LEN)) + /* + * nft doesn't support --nflog-range + */ + return add_target(r, cs->target->t); + expr = nftnl_expr_alloc("log"); if (!expr) return -ENOMEM; Perhaps, we could put this in now, add something to the release notes for the next release formally deprecating `--nflog-range` and then remove it altogether in the following release. Or we could just apply the current patches if you're not that bothered. :) J.
nftables supports 128-character prefixes for nflog whereas legacy iptables only supports 64 characters. This patch series converts iptables-nft to use the nft back-end in order to take advantage of the longer prefixes. * Patches 1-5 implement the conversion and update some related Python unit-tests. * Patch 6 fixes an minor bug in the output of nflog prefixes. * Patch 7 contains a couple of libtool updates. * Patch 8 fixes some typo's. Changes since v1: * Patches 1 and 5-8 are new. * White-space fixes in patches 2 and 3. * Fixes for typo's in commit-messages of patches 2 and 4. * Removal of stray `struct xt_nflog_info` allocation from `nft_parse_log` in patch 3. * Leave commented-out `--nflog-range` test-cases in libxt_NFLOG.t with an explanatory comment in patch 4. Jeremy Sowden (5): nft: fix indentation error. extensions: libxt_NFLOG: fix `--nflog-prefix` Python test-cases extensions: libxt_NFLOG: remove extra space when saving targets with prefixes build: replace `AM_PROG_LIBTOOL` and `AC_DISABLE_STATIC` with `LT_INIT` tests: iptables-test: correct misspelt variable Kyle Bowman (3): extensions: libxt_NFLOG: use nft built-in logging instead of xt_NFLOG extensions: libxt_NFLOG: don't truncate log prefix on print/save extensions: libxt_NFLOG: disable `--nflog-range` Python test-cases configure.ac | 3 +- extensions/libxt_NFLOG.c | 8 ++++- extensions/libxt_NFLOG.t | 16 ++++----- iptables-test.py | 18 +++++----- iptables/nft-shared.c | 52 ++++++++++++++++++++++++++++ iptables/nft.c | 74 ++++++++++++++++++++++++++++------------ iptables/nft.h | 1 + 7 files changed, 131 insertions(+), 41 deletions(-)