[PATCH iptables v2 0/8] extensions: libxt_NFLOG: use nft back-end for iptables-nft

[PATCH iptables v2 0/8] extensions: libxt_NFLOG: use nft back-end for iptables-nft

[Jeremy Sowden]

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

-- 
2.33.0

[PATCH iptables v2 1/8] nft: fix indentation error.

[Jeremy Sowden]

`add_action` was indented with 7 spaces.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 iptables/nft.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index dc1f5160eb98..5613bc968046 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1320,27 +1320,27 @@ int add_verdict(struct nftnl_rule *r, int verdict)
 int add_action(struct nftnl_rule *r, struct iptables_command_state *cs,
 	       bool goto_set)
 {
-       int ret = 0;
-
-       /* If no target at all, add nothing (default to continue) */
-       if (cs->target != NULL) {
-	       /* Standard target? */
-	       if (strcmp(cs->jumpto, XTC_LABEL_ACCEPT) == 0)
-		       ret = add_verdict(r, NF_ACCEPT);
-	       else if (strcmp(cs->jumpto, XTC_LABEL_DROP) == 0)
-		       ret = add_verdict(r, NF_DROP);
-	       else if (strcmp(cs->jumpto, XTC_LABEL_RETURN) == 0)
-		       ret = add_verdict(r, NFT_RETURN);
-	       else
-		       ret = add_target(r, cs->target->t);
-       } else if (strlen(cs->jumpto) > 0) {
-	       /* Not standard, then it's a go / jump to chain */
-	       if (goto_set)
-		       ret = add_jumpto(r, cs->jumpto, NFT_GOTO);
-	       else
-		       ret = add_jumpto(r, cs->jumpto, NFT_JUMP);
-       }
-       return ret;
+	int ret = 0;
+
+	/* If no target at all, add nothing (default to continue) */
+	if (cs->target != NULL) {
+		/* Standard target? */
+		if (strcmp(cs->jumpto, XTC_LABEL_ACCEPT) == 0)
+			ret = add_verdict(r, NF_ACCEPT);
+		else if (strcmp(cs->jumpto, XTC_LABEL_DROP) == 0)
+			ret = add_verdict(r, NF_DROP);
+		else if (strcmp(cs->jumpto, XTC_LABEL_RETURN) == 0)
+			ret = add_verdict(r, NFT_RETURN);
+		else
+			ret = add_target(r, cs->target->t);
+	} else if (strlen(cs->jumpto) > 0) {
+		/* Not standard, then it's a go / jump to chain */
+		if (goto_set)
+			ret = add_jumpto(r, cs->jumpto, NFT_GOTO);
+		else
+			ret = add_jumpto(r, cs->jumpto, NFT_JUMP);
+	}
+	return ret;
 }
 
 static void nft_rule_print_debug(struct nftnl_rule *r, struct nlmsghdr *nlh)
-- 
2.33.0

[PATCH iptables v2 2/8] extensions: libxt_NFLOG: use nft built-in logging instead of xt_NFLOG

[Jeremy Sowden]

From: Kyle Bowman <kbowman@cloudflare.com>

Replaces the use of xt_NFLOG with the nft built-in log statement.

This additionally adds support for using longer log prefixes of 128
characters in size. Until now NFLOG has truncated the log-prefix to the
64-character limit supported by iptables-legacy. We now use the struct
xtables_target's udata member to store the longer 128-character prefix
supported by iptables-nft.

Signed-off-by: Kyle Bowman <kbowman@cloudflare.com>
Signed-off-by: Alex Forster <aforster@cloudflare.com>
Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/libxt_NFLOG.c |  6 ++++++
 iptables/nft.c           | 28 ++++++++++++++++++++++++++++
 iptables/nft.h           |  1 +
 3 files changed, 35 insertions(+)

diff --git a/extensions/libxt_NFLOG.c b/extensions/libxt_NFLOG.c
index 02a1b4aa35a3..2b78e27808f8 100644
--- a/extensions/libxt_NFLOG.c
+++ b/extensions/libxt_NFLOG.c
@@ -5,6 +5,7 @@
 #include <getopt.h>
 #include <xtables.h>
 
+#include <linux/netfilter/nf_log.h>
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter/xt_NFLOG.h>
 
@@ -53,12 +54,16 @@ static void NFLOG_init(struct xt_entry_target *t)
 
 static void NFLOG_parse(struct xt_option_call *cb)
 {
+	char *nf_log_prefix = cb->udata;
+
 	xtables_option_parse(cb);
 	switch (cb->entry->id) {
 	case O_PREFIX:
 		if (strchr(cb->arg, '\n') != NULL)
 			xtables_error(PARAMETER_PROBLEM,
 				   "Newlines not allowed in --log-prefix");
+
+		snprintf(nf_log_prefix, NF_LOG_PREFIXLEN, "%s", cb->arg);
 		break;
 	}
 }
@@ -149,6 +154,7 @@ static struct xtables_target nflog_target = {
 	.save		= NFLOG_save,
 	.x6_options	= NFLOG_opts,
 	.xlate		= NFLOG_xlate,
+	.udata_size	= NF_LOG_PREFIXLEN
 };
 
 void _init(void)
diff --git a/iptables/nft.c b/iptables/nft.c
index 5613bc968046..53506c9475c0 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -39,6 +39,7 @@
 #include <linux/netfilter/nf_tables_compat.h>
 
 #include <linux/netfilter/xt_limit.h>
+#include <linux/netfilter/xt_NFLOG.h>
 
 #include <libmnl/libmnl.h>
 #include <libnftnl/gen.h>
@@ -1331,6 +1332,8 @@ int add_action(struct nftnl_rule *r, struct iptables_command_state *cs,
 			ret = add_verdict(r, NF_DROP);
 		else if (strcmp(cs->jumpto, XTC_LABEL_RETURN) == 0)
 			ret = add_verdict(r, NFT_RETURN);
+		else if (strcmp(cs->jumpto, "NFLOG") == 0)
+			ret = add_log(r, cs);
 		else
 			ret = add_target(r, cs->target->t);
 	} else if (strlen(cs->jumpto) > 0) {
@@ -1343,6 +1346,31 @@ int add_action(struct nftnl_rule *r, struct iptables_command_state *cs,
 	return ret;
 }
 
+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;
+
+	expr = nftnl_expr_alloc("log");
+	if (!expr)
+		return -ENOMEM;
+
+	if (info->prefix[0] != '\0')
+		nftnl_expr_set_str(expr, NFTNL_EXPR_LOG_PREFIX,
+				   cs->target->udata);
+
+	nftnl_expr_set_u16(expr, NFTNL_EXPR_LOG_GROUP, info->group);
+	if (info->flags & XT_NFLOG_F_COPY_LEN)
+		nftnl_expr_set_u32(expr, NFTNL_EXPR_LOG_SNAPLEN,
+				   info->len);
+	if (info->threshold)
+		nftnl_expr_set_u16(expr, NFTNL_EXPR_LOG_QTHRESHOLD,
+				   info->threshold);
+
+	nftnl_rule_add_expr(r, expr);
+	return 0;
+}
+
 static void nft_rule_print_debug(struct nftnl_rule *r, struct nlmsghdr *nlh)
 {
 #ifdef NLDEBUG
diff --git a/iptables/nft.h b/iptables/nft.h
index ef79b018f783..440b23af68df 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -194,6 +194,7 @@ int add_match(struct nft_handle *h, struct nftnl_rule *r, struct xt_entry_match
 int add_target(struct nftnl_rule *r, struct xt_entry_target *t);
 int add_jumpto(struct nftnl_rule *r, const char *name, int verdict);
 int add_action(struct nftnl_rule *r, struct iptables_command_state *cs, bool goto_set);
+int add_log(struct nftnl_rule *r, struct iptables_command_state *cs);
 char *get_comment(const void *data, uint32_t data_len);
 
 enum nft_rule_print {
-- 
2.33.0

[PATCH iptables v2 3/8] extensions: libxt_NFLOG: don't truncate log prefix on print/save

[Jeremy Sowden]

From: Kyle Bowman <kbowman@cloudflare.com>

When parsing the rule, use a struct with a layout compatible to that of
struct xt_nflog_info, but with a buffer large enough to contain the
whole 128-character nft prefix.

We always send the nflog-group to the kernel since, for nft, log and
nflog targets are handled by the same kernel module, and are
distinguished by whether they define an nflog-group. Therefore, we must
send the group even if it is zero, or the kernel will configure the
target as a log, not an nflog.

Changes to nft_is_expr_compatible were made since only targets which
have an `nflog-group` are compatible. Since nflog targets are
distinguished by having an nflog-group, we ignore targets without one.

We also set the copy-len flag if the snap-len is set since without this,
iptables will mistake `nflog-size` for `nflog-range`.

Signed-off-by: Kyle Bowman <kbowman@cloudflare.com>
Signed-off-by: Alex Forster <aforster@cloudflare.com>
Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 iptables/nft-shared.c | 52 +++++++++++++++++++++++++++++++++++++++++++
 iptables/nft.c        |  4 ++++
 2 files changed, 56 insertions(+)

diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index 4253b08196d2..2430bac44bb0 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -20,8 +20,10 @@
 
 #include <xtables.h>
 
+#include <linux/netfilter/nf_log.h>
 #include <linux/netfilter/xt_comment.h>
 #include <linux/netfilter/xt_limit.h>
+#include <linux/netfilter/xt_NFLOG.h>
 
 #include <libmnl/libmnl.h>
 #include <libnftnl/rule.h>
@@ -595,6 +597,54 @@ static void nft_parse_limit(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 		ctx->h->ops->parse_match(match, ctx->cs);
 }
 
+static void nft_parse_log(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
+{
+	struct xtables_target *target;
+	struct xt_entry_target *t;
+	size_t target_size;
+	/*
+	 * In order to handle the longer log-prefix supported by nft, instead of
+	 * using struct xt_nflog_info, we use a struct with a compatible layout, but
+	 * a larger buffer for the prefix.
+	 */
+	struct xt_nflog_info_nft {
+		__u32 len;
+		__u16 group;
+		__u16 threshold;
+		__u16 flags;
+		__u16 pad;
+		char  prefix[NF_LOG_PREFIXLEN];
+	} info = {
+		.group     = nftnl_expr_get_u16(e, NFTNL_EXPR_LOG_GROUP),
+		.threshold = nftnl_expr_get_u16(e, NFTNL_EXPR_LOG_QTHRESHOLD),
+	};
+	if (nftnl_expr_is_set(e, NFTNL_EXPR_LOG_SNAPLEN)) {
+		info.len = nftnl_expr_get_u32(e, NFTNL_EXPR_LOG_SNAPLEN);
+		info.flags = XT_NFLOG_F_COPY_LEN;
+	}
+	if (nftnl_expr_is_set(e, NFTNL_EXPR_LOG_PREFIX))
+		snprintf(info.prefix, sizeof(info.prefix), "%s",
+			 nftnl_expr_get_str(e, NFTNL_EXPR_LOG_PREFIX));
+
+	target = xtables_find_target("NFLOG", XTF_TRY_LOAD);
+	if (target == NULL)
+		return;
+
+	target_size = XT_ALIGN(sizeof(struct xt_entry_target)) +
+		      XT_ALIGN(sizeof(struct xt_nflog_info_nft));
+
+	t = xtables_calloc(1, target_size);
+	t->u.target_size = target_size;
+	strcpy(t->u.user.name, target->name);
+	t->u.user.revision = target->revision;
+
+	target->t = t;
+
+	memcpy(&target->t->data, &info, sizeof(info));
+
+	ctx->h->ops->parse_target(target, ctx->cs);
+}
+
 static void nft_parse_lookup(struct nft_xt_ctx *ctx, struct nft_handle *h,
 			     struct nftnl_expr *e)
 {
@@ -644,6 +694,8 @@ void nft_rule_to_iptables_command_state(struct nft_handle *h,
 			nft_parse_limit(&ctx, expr);
 		else if (strcmp(name, "lookup") == 0)
 			nft_parse_lookup(&ctx, h, expr);
+		else if (strcmp(name, "log") == 0)
+			nft_parse_log(&ctx, expr);
 
 		expr = nftnl_expr_iter_next(iter);
 	}
diff --git a/iptables/nft.c b/iptables/nft.c
index 53506c9475c0..58943088f832 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -3527,6 +3527,10 @@ static int nft_is_expr_compatible(struct nftnl_expr *expr, void *data)
 	    nftnl_expr_get_u32(expr, NFTNL_EXPR_LIMIT_FLAGS) == 0)
 		return 0;
 
+	if (!strcmp(name, "log") &&
+	    nftnl_expr_is_set(expr, NFTNL_EXPR_LOG_GROUP))
+		return 0;
+
 	return -1;
 }
 
-- 
2.33.0

[PATCH iptables v2 4/8] extensions: libxt_NFLOG: disable `--nflog-range` Python test-cases

[Jeremy Sowden]

From: Kyle Bowman <kbowman@cloudflare.com>

nft has no equivalent to `--nflog-range`, so we cannot emulate it and
the Python unit-tests for it fail.  However, since `--nflog-range` is
broken and doesn't do anything, the tests are not testing anything
useful.

Signed-off-by: Kyle Bowman <kbowman@cloudflare.com>
Signed-off-by: Alex Forster <aforster@cloudflare.com>
Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/libxt_NFLOG.t | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/extensions/libxt_NFLOG.t b/extensions/libxt_NFLOG.t
index 933fa22160e5..eefb058be30e 100644
--- a/extensions/libxt_NFLOG.t
+++ b/extensions/libxt_NFLOG.t
@@ -3,10 +3,12 @@
 -j NFLOG --nflog-group 65535;=;OK
 -j NFLOG --nflog-group 65536;;FAIL
 -j NFLOG --nflog-group 0;-j NFLOG;OK
--j NFLOG --nflog-range 1;=;OK
--j NFLOG --nflog-range 4294967295;=;OK
--j NFLOG --nflog-range 4294967296;;FAIL
--j NFLOG --nflog-range -1;;FAIL
+# `--nflog-range` is broken and only supported by xtables-legacy.  It
+# has been superseded by `--nflog--group`.
+# -j NFLOG --nflog-range 1;=;OK
+# -j NFLOG --nflog-range 4294967295;=;OK
+# -j NFLOG --nflog-range 4294967296;;FAIL
+# -j NFLOG --nflog-range -1;;FAIL
 -j NFLOG --nflog-size 0;=;OK
 -j NFLOG --nflog-size 1;=;OK
 -j NFLOG --nflog-size 4294967295;=;OK
-- 
2.33.0

[PATCH iptables v2 5/8] extensions: libxt_NFLOG: fix `--nflog-prefix` Python test-cases

[Jeremy Sowden]

The `iptables-save` includes an extra space between `--nflog-prefix` and
the prefix.

The maximum length of prefixes includes the trailing NUL character.

NFLOG silently truncates prefixes which exceed the maximum length.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/libxt_NFLOG.t | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/extensions/libxt_NFLOG.t b/extensions/libxt_NFLOG.t
index eefb058be30e..13bbf2bfc5a5 100644
--- a/extensions/libxt_NFLOG.t
+++ b/extensions/libxt_NFLOG.t
@@ -14,10 +14,8 @@
 -j NFLOG --nflog-size 4294967295;=;OK
 -j NFLOG --nflog-size 4294967296;;FAIL
 -j NFLOG --nflog-size -1;;FAIL
-# ERROR: cannot find: iptables -I INPUT -j NFLOG --nflog-prefix  xxxxxx [...]
-# -j NFLOG --nflog-prefix xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;=;OK
-# ERROR: should fail: iptables -A INPUT -j NFLOG --nflog-prefix  xxxxxxx [...]
-#  -j NFLOG --nflog-prefix xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;;FAIL
+-j NFLOG --nflog-prefix  xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;=;OK
+-j NFLOG --nflog-prefix  xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;-j NFLOG --nflog-prefix  xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;OK
 -j NFLOG --nflog-threshold 1;=;OK
 # ERROR: line 13 (should fail: iptables -A INPUT -j NFLOG --nflog-threshold 0
 # -j NFLOG --nflog-threshold 0;;FAIL
-- 
2.33.0

[PATCH iptables v2 6/8] extensions: libxt_NFLOG: remove extra space when saving targets with prefixes

[Jeremy Sowden]

When printing out NFLOG targets an extra space was inserted between
`--nflog-prefix` and the prefix itself:

  $ sudo /usr/sbin/iptables -A INPUT -j NFLOG --nflog-prefix test
  $ sudo /usr/sbin/iptables-save | grep NFLOG
  -A INPUT -j NFLOG --nflog-prefix  test
                                  ^^
Fixes: 73866357e4a7 ("iptables: do not print trailing whitespaces")
Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/libxt_NFLOG.c | 2 +-
 extensions/libxt_NFLOG.t | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/extensions/libxt_NFLOG.c b/extensions/libxt_NFLOG.c
index 2b78e27808f8..6137a68f8cd2 100644
--- a/extensions/libxt_NFLOG.c
+++ b/extensions/libxt_NFLOG.c
@@ -83,7 +83,7 @@ static void NFLOG_check(struct xt_fcheck_call *cb)
 static void nflog_print(const struct xt_nflog_info *info, char *prefix)
 {
 	if (info->prefix[0] != '\0') {
-		printf(" %snflog-prefix ", prefix);
+		printf(" %snflog-prefix", prefix);
 		xtables_save_string(info->prefix);
 	}
 	if (info->group)
diff --git a/extensions/libxt_NFLOG.t b/extensions/libxt_NFLOG.t
index 13bbf2bfc5a5..561ec8c77650 100644
--- a/extensions/libxt_NFLOG.t
+++ b/extensions/libxt_NFLOG.t
@@ -14,8 +14,8 @@
 -j NFLOG --nflog-size 4294967295;=;OK
 -j NFLOG --nflog-size 4294967296;;FAIL
 -j NFLOG --nflog-size -1;;FAIL
--j NFLOG --nflog-prefix  xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;=;OK
--j NFLOG --nflog-prefix  xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;-j NFLOG --nflog-prefix  xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;OK
+-j NFLOG --nflog-prefix xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;=;OK
+-j NFLOG --nflog-prefix xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;-j NFLOG --nflog-prefix xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;OK
 -j NFLOG --nflog-threshold 1;=;OK
 # ERROR: line 13 (should fail: iptables -A INPUT -j NFLOG --nflog-threshold 0
 # -j NFLOG --nflog-threshold 0;;FAIL
-- 
2.33.0

[PATCH iptables v2 7/8] build: replace `AM_PROG_LIBTOOL` and `AC_DISABLE_STATIC` with `LT_INIT`

[Jeremy Sowden]

`AM_PROG_LIBTOOL` is superseded by `LT_INIT`, which also accepts options
to control the defaults for creating shared or static libraries.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 configure.ac | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 00ae60c5cfa1..86c67194825f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -12,9 +12,8 @@ AC_PROG_INSTALL
 AM_INIT_AUTOMAKE([-Wall])
 AC_PROG_CC
 AM_PROG_CC_C_O
-AC_DISABLE_STATIC
 m4_ifdef([AM_PROG_AR], [AM_PROG_AR])
-AM_PROG_LIBTOOL
+LT_INIT([disable-static])
 
 AC_ARG_WITH([kernel],
 	AS_HELP_STRING([--with-kernel=PATH],
-- 
2.33.0

[PATCH iptables v2 8/8] tests: iptables-test: correct misspelt variable

[Jeremy Sowden]

"EXECUTEABLE" -> "EXECUTABLE"

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 iptables-test.py | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/iptables-test.py b/iptables-test.py
index 0ba3d36864fd..95fa11b1475c 100755
--- a/iptables-test.py
+++ b/iptables-test.py
@@ -84,7 +84,7 @@ def run_test(iptables, rule, rule_save, res, filename, lineno, netns):
 
     cmd = iptables + " -A " + rule
     if netns:
-            cmd = "ip netns exec ____iptables-container-test " + EXECUTEABLE + " " + cmd
+            cmd = "ip netns exec ____iptables-container-test " + EXECUTABLE + " " + cmd
 
     ret = execute_cmd(cmd, filename, lineno)
 
@@ -123,7 +123,7 @@ def run_test(iptables, rule, rule_save, res, filename, lineno, netns):
         elif splitted[0] == EBTABLES:
             command = EBTABLES_SAVE
 
-    command = EXECUTEABLE + " " + command
+    command = EXECUTABLE + " " + command
 
     if netns:
             command = "ip netns exec ____iptables-container-test " + command
@@ -168,7 +168,7 @@ def execute_cmd(cmd, filename, lineno):
     '''
     global log_file
     if cmd.startswith('iptables ') or cmd.startswith('ip6tables ') or cmd.startswith('ebtables ') or cmd.startswith('arptables '):
-        cmd = EXECUTEABLE + " " + cmd
+        cmd = EXECUTABLE + " " + cmd
 
     print("command: {}".format(cmd), file=log_file)
     ret = subprocess.call(cmd, shell=True, universal_newlines=True,
@@ -202,12 +202,12 @@ def run_test_file(filename, netns):
         iptables = IPTABLES
     elif "libarpt_" in filename:
         # only supported with nf_tables backend
-        if EXECUTEABLE != "xtables-nft-multi":
+        if EXECUTABLE != "xtables-nft-multi":
            return 0, 0
         iptables = ARPTABLES
     elif "libebt_" in filename:
         # only supported with nf_tables backend
-        if EXECUTEABLE != "xtables-nft-multi":
+        if EXECUTABLE != "xtables-nft-multi":
            return 0, 0
         iptables = EBTABLES
     else:
@@ -245,7 +245,7 @@ def run_test_file(filename, netns):
         if line[0] == "%":
             external_cmd = line.rstrip()[1:]
             if netns:
-                external_cmd = "ip netns exec ____iptables-container-test " + EXECUTEABLE + " " + external_cmd
+                external_cmd = "ip netns exec ____iptables-container-test " + EXECUTABLE + " " + external_cmd
             execute_cmd(external_cmd, filename, lineno)
             continue
 
@@ -366,10 +366,10 @@ def main():
         show_missing()
         return
 
-    global EXECUTEABLE
-    EXECUTEABLE = "xtables-legacy-multi"
+    global EXECUTABLE
+    EXECUTABLE = "xtables-legacy-multi"
     if args.nftables:
-        EXECUTEABLE = "xtables-nft-multi"
+        EXECUTABLE = "xtables-nft-multi"
 
     if os.getuid() != 0:
         print("You need to be root to run this, sorry", file=sys.stderr)
-- 
2.33.0

Re: [PATCH iptables v2 0/8] extensions: libxt_NFLOG: use nft back-end for iptables-nft

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 2019 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH iptables v2 0/8] extensions: libxt_NFLOG: use nft back-end for iptables-nft

[Florian Westphal]

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?

Re: [PATCH iptables v2 0/8] extensions: libxt_NFLOG: use nft back-end for iptables-nft

[Phil Sutter]

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

Re: [PATCH iptables v2 0/8] extensions: libxt_NFLOG: use nft back-end for iptables-nft

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 2257 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH iptables v2 0/8] extensions: libxt_NFLOG: use nft back-end for iptables-nft

[Pablo Neira Ayuso]

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.

Re: [PATCH iptables v2 0/8] extensions: libxt_NFLOG: use nft back-end for iptables-nft

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 2418 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH iptables v2 2/8] extensions: libxt_NFLOG: use nft built-in logging instead of xt_NFLOG

[Florian Westphal]

Jeremy Sowden <jeremy@azazel.net> wrote:
> From: Kyle Bowman <kbowman@cloudflare.com>
> 
> Replaces the use of xt_NFLOG with the nft built-in log statement.
> 
> This additionally adds support for using longer log prefixes of 128
> characters in size. Until now NFLOG has truncated the log-prefix to the
> 64-character limit supported by iptables-legacy. We now use the struct
> xtables_target's udata member to store the longer 128-character prefix
> supported by iptables-nft.

Series applied, thanks for your patience.