[iptables PATCH 0/7] tests: xlate: generic.txlate to pass replay test

[iptables PATCH 0/7] tests: xlate: generic.txlate to pass replay test

[Phil Sutter]

Instead of dumping the ruleset with xtables-save and creating yet
another string comparison mess by searching the output, use --check
command to leverage iptables' internal rule comparison functionality
when checking that the nftables-created rule parses correctly as the
source of the translation (patch 2).

There was a rub with the above, namely ebtables not supporting --check
in the first place. Gladly the implementation is pretty simple (patch
1) with one caveat: '-C' itself is not available so add the long option
only.

The remaining patches deal with translation details (mostly around
wildcard interface names) until generic.txlate finally passes the replay
test.

Phil Sutter (7):
  ebtables: Implement --check command
  tests: xlate: Use --check to verify replay
  nft: Fix for comparing ifname matches against nft-generated ones
  nft: Fix match generator for '! -i +'
  nft: Recognize INVAL/D interface name
  xtables-translate: Fix for interfaces with asterisk mid-string
  ebtables: Fix MAC address match translation

 extensions/generic.txlate    | 16 ++++++-------
 iptables/nft-bridge.c        |  6 ++---
 iptables/nft-shared.c        | 27 ++++++++++++++++++++-
 iptables/xtables-eb.c        | 12 +++++++---
 iptables/xtables-translate.c |  4 +++-
 xlate-test.py                | 46 ++++++++++++++----------------------
 6 files changed, 67 insertions(+), 44 deletions(-)

-- 
2.38.0

[iptables PATCH 1/7] ebtables: Implement --check command

[Phil Sutter]

Sadly, '-C' is in use already for --change-counters (even though
ebtables-nft does not implement this), so add a long-option only. It is
needed for xlate testsuite in replay mode, which will use '--check'
instead of '-C'.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xtables-eb.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index c5fc338575f67..7214a767ffe96 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -198,6 +198,7 @@ struct option ebt_original_options[] =
 	{ "delete-chain"   , optional_argument, 0, 'X' },
 	{ "init-table"     , no_argument      , 0, 11  },
 	{ "concurrent"     , no_argument      , 0, 13  },
+	{ "check"          , required_argument, 0, 14  },
 	{ 0 }
 };
 
@@ -730,6 +731,7 @@ int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table,
 		case 'N': /* Make a user defined chain */
 		case 'E': /* Rename chain */
 		case 'X': /* Delete chain */
+		case 14:  /* check a rule */
 			/* We allow -N chainname -P policy */
 			if (command == 'N' && c == 'P') {
 				command = c;
@@ -907,7 +909,8 @@ int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table,
 			if (!OPT_COMMANDS)
 				xtables_error(PARAMETER_PROBLEM,
 					      "No command specified");
-			if (command != 'A' && command != 'D' && command != 'I' && command != 'C')
+			if (command != 'A' && command != 'D' &&
+			    command != 'I' && command != 'C' && command != 14)
 				xtables_error(PARAMETER_PROBLEM,
 					      "Command and option do not match");
 			if (c == 'i') {
@@ -1088,7 +1091,7 @@ int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table,
 					      argv[optind]);
 
 			if (command != 'A' && command != 'I' &&
-			    command != 'D' && command != 'C')
+			    command != 'D' && command != 'C' && command != 14)
 				xtables_error(PARAMETER_PROBLEM,
 					      "Extensions only for -A, -I, -D and -C");
 		}
@@ -1109,7 +1112,7 @@ int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table,
 
 	/* Do the final checks */
 	if (command == 'A' || command == 'I' ||
-	    command == 'D' || command == 'C') {
+	    command == 'D' || command == 'C' || command == 14) {
 		for (xtrm_i = cs.matches; xtrm_i; xtrm_i = xtrm_i->next)
 			xtables_option_mfcall(xtrm_i->match);
 
@@ -1161,6 +1164,9 @@ int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table,
 	} else if (command == 'D') {
 		ret = delete_entry(h, chain, *table, &cs, rule_nr - 1,
 				   rule_nr_end, flags & OPT_VERBOSE);
+	} else if (command == 14) {
+		ret = nft_cmd_rule_check(h, chain, *table,
+					 &cs, flags & OPT_VERBOSE);
 	} /*else if (replace->command == 'C') {
 		ebt_change_counters(replace, new_entry, rule_nr, rule_nr_end, &(new_entry->cnt_surplus), chcounter);
 		if (ebt_errormsg[0] != '\0')
-- 
2.38.0

[iptables PATCH 2/7] tests: xlate: Use --check to verify replay

[Phil Sutter]

After applying the translated rule using nft, pass the untranslated rule
to --check instead of dumping the ruleset and performing a string
search. This fixes for mandatory match reordering (e.g. addresses before
interfaces) and minor differences like /32 netmasks or even just
whitespace changes.

Fixes: 223e34b057b95 ("tests: xlate-test: Replay results for reverse direction testing")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 xlate-test.py | 46 ++++++++++++++++++----------------------------
 1 file changed, 18 insertions(+), 28 deletions(-)

diff --git a/xlate-test.py b/xlate-test.py
index 6513b314beb35..4f037ef6ed96d 100755
--- a/xlate-test.py
+++ b/xlate-test.py
@@ -67,6 +67,7 @@ xtables_nft_multi = 'xtables-nft-multi'
     srcwords = sourceline.split()
 
     srccmd = srcwords[0]
+    ipt = srccmd.split('-')[0]
     table_idx = -1
     chain_idx = -1
     table_name = "filter"
@@ -84,16 +85,12 @@ xtables_nft_multi = 'xtables-nft-multi'
 
     if searchline is None:
         # adjust sourceline as required
-        srcwords[chain_idx] = "-A"
-        if table_idx >= 0:
-            srcwords.pop(table_idx)
-            srcwords.pop(table_idx)
-        searchline = " ".join(srcwords[1:])
-    elif not searchline.startswith("-A"):
-        tmp = ["-A", chain_name]
-        if len(searchline) > 0:
-            tmp.extend(searchline)
-        searchline = " ".join(tmp)
+        checkcmd = srcwords[:]
+        checkcmd[0] = ipt
+        checkcmd[chain_idx] = "--check"
+    else:
+        checkcmd = [ipt, "-t", table_name]
+        checkcmd += ["--check", chain_name, searchline]
 
     fam = ""
     if srccmd.startswith("ip6"):
@@ -110,30 +107,23 @@ xtables_nft_multi = 'xtables-nft-multi'
 
     rc, output, error = run_proc([args.nft, "-f", "-"], shell = False, input = "\n".join(nft_input))
     if rc != 0:
-        result.append(name + ": " + red("Fail"))
+        result.append(name + ": " + red("Replay Fail"))
         result.append(args.nft + " call failed: " + error.rstrip('\n'))
         for line in nft_input:
             result.append(magenta("input: ") + line)
         return False
 
-    ipt = srccmd.split('-')[0]
-    rc, output, error = run_proc([xtables_nft_multi, ipt + "-save"])
+    rc, output, error = run_proc([xtables_nft_multi] + checkcmd)
     if rc != 0:
-        result.append(name + ": " + red("Fail"))
-        result.append(ipt + "-save call failed: " + error)
-        return False
-
-    if output.find(searchline) < 0:
-        outline = None
-        for l in output.split('\n'):
-            if l.startswith('-A '):
-                output = l
-                break
-        result.append(name + ": " + red("Replay fail"))
-        result.append(magenta("src: '") + str(expected) + "'")
-        result.append(magenta("exp: '") + searchline + "'")
-        for l in output.split('\n'):
-            result.append(magenta("res: ") + l)
+        result.append(name + ": " + red("Check Fail"))
+        result.append(magenta("check: ") + " ".join(checkcmd))
+        result.append(magenta("error: ") + error)
+        rc, output, error = run_proc([xtables_nft_multi, ipt + "-save"])
+        for l in output.split("\n"):
+            result.append(magenta("ipt: ") + l)
+        rc, output, error = run_proc([args.nft, "list", "ruleset"])
+        for l in output.split("\n"):
+            result.append(magenta("nft: ") + l)
         return False
 
     return True
-- 
2.38.0

[iptables PATCH 3/7] nft: Fix for comparing ifname matches against nft-generated ones

[Phil Sutter]

Since nft adds the interface name as fixed-size string of 16 bytes,
filling a mask based on the length value will not match the mask nft
set.

Fixes: 652b98e793711 ("xtables-compat: fix wildcard detection")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-shared.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index 63d251986f65b..e812a9bcae466 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -279,7 +279,7 @@ static void parse_ifname(const char *name, unsigned int len, char *dst, unsigned
 	memcpy(dst, name, len);
 	if (name[len - 1] == '\0') {
 		if (mask)
-			memset(mask, 0xff, len);
+			memset(mask, 0xff, strlen(name) + 1);
 		return;
 	}
 
-- 
2.38.0

[iptables PATCH 4/7] nft: Fix match generator for '! -i +'

[Phil Sutter]

It's actually nonsense since it will never match, but iptables accepts
it and the resulting nftables rule must behave identically. Reuse the
solution implemented into xtables-translate (by commit e179e87a1179e)
and turn the above match into 'iifname INVAL/D'.

The commit this fixes merely ignored the fact that "any interface" match
might be inverted.

Fixes: 0a8635183edd0 ("xtables-compat: ignore '+' interface name")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-shared.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index e812a9bcae466..bcb6ada34e0fb 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -168,6 +168,9 @@ void add_iniface(struct nft_handle *h, struct nftnl_rule *r,
 	if (iface[iface_len - 1] == '+') {
 		if (iface_len > 1)
 			add_cmp_ptr(r, op, iface, iface_len - 1, reg);
+		else if (op != NFT_CMP_EQ)
+			add_cmp_ptr(r, NFT_CMP_EQ, "INVAL/D",
+				    strlen("INVAL/D") + 1, reg);
 	} else {
 		add_cmp_ptr(r, op, iface, iface_len + 1, reg);
 	}
@@ -185,6 +188,9 @@ void add_outiface(struct nft_handle *h, struct nftnl_rule *r,
 	if (iface[iface_len - 1] == '+') {
 		if (iface_len > 1)
 			add_cmp_ptr(r, op, iface, iface_len - 1, reg);
+		else if (op != NFT_CMP_EQ)
+			add_cmp_ptr(r, NFT_CMP_EQ, "INVAL/D",
+				    strlen("INVAL/D") + 1, reg);
 	} else {
 		add_cmp_ptr(r, op, iface, iface_len + 1, reg);
 	}
-- 
2.38.0

[iptables PATCH 5/7] nft: Recognize INVAL/D interface name

[Phil Sutter]

It is just a hack to translate '! -i +' into a never matching nft rule,
but recognize it anyway for completeness' sake and to make xlate replay
test pass.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-shared.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index bcb6ada34e0fb..b7e073df5a270 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -359,6 +359,21 @@ static int parse_meta_pkttype(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 	return 0;
 }
 
+static void parse_invalid_iface(char *iface, unsigned char *mask,
+				uint8_t *invflags, uint8_t invbit)
+{
+	if (*invflags & invbit || strcmp(iface, "INVAL/D"))
+		return;
+
+	/* nft's poor "! -o +" excuse */
+	*invflags |= invbit;
+	iface[0] = '+';
+	iface[1] = '\0';
+	mask[0] = 0xff;
+	mask[1] = 0xff;
+	memset(mask + 2, 0, IFNAMSIZ - 2);
+}
+
 int parse_meta(struct nft_xt_ctx *ctx, struct nftnl_expr *e, uint8_t key,
 	       char *iniface, unsigned char *iniface_mask,
 	       char *outiface, unsigned char *outiface_mask, uint8_t *invflags)
@@ -393,6 +408,8 @@ int parse_meta(struct nft_xt_ctx *ctx, struct nftnl_expr *e, uint8_t key,
 			*invflags |= IPT_INV_VIA_IN;
 
 		parse_ifname(ifname, len, iniface, iniface_mask);
+		parse_invalid_iface(iniface, iniface_mask,
+				    invflags, IPT_INV_VIA_IN);
 		break;
 	case NFT_META_BRI_OIFNAME:
 	case NFT_META_OIFNAME:
@@ -401,6 +418,8 @@ int parse_meta(struct nft_xt_ctx *ctx, struct nftnl_expr *e, uint8_t key,
 			*invflags |= IPT_INV_VIA_OUT;
 
 		parse_ifname(ifname, len, outiface, outiface_mask);
+		parse_invalid_iface(outiface, outiface_mask,
+				    invflags, IPT_INV_VIA_OUT);
 		break;
 	case NFT_META_MARK:
 		parse_meta_mark(ctx, e);
-- 
2.38.0

[iptables PATCH 6/7] xtables-translate: Fix for interfaces with asterisk mid-string

[Phil Sutter]

For nft, asterisk is special at end of the interface name only. Escaping
it mid-string makes the escape char part of the interface name, so avoid
this.

In the test case, also drop the ticks around interface names in
*-translate command - since there's no shell involved which would eat
them, they become part of the interface name.

Fixes: e179e87a1179e ("xtables-translate: Fix for interface name corner-cases")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 extensions/generic.txlate    | 14 +++++++-------
 iptables/xtables-translate.c |  4 +++-
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/extensions/generic.txlate b/extensions/generic.txlate
index 7e879fd526bb1..d7ddf6a39762e 100644
--- a/extensions/generic.txlate
+++ b/extensions/generic.txlate
@@ -74,17 +74,17 @@ ebtables-translate -I INPUT -p ! Length
 nft 'insert rule bridge filter INPUT ether type >= 0x0600 counter'
 
 # asterisk is not special in iptables and it is even a valid interface name
-iptables-translate -A FORWARD -i '*' -o 'eth*foo'
-nft 'add rule ip filter FORWARD iifname "\*" oifname "eth\*foo" counter'
+iptables-translate -A FORWARD -i * -o eth*foo
+nft 'add rule ip filter FORWARD iifname "\*" oifname "eth*foo" counter'
 
-# escape all asterisks but translate only the first plus character
-iptables-translate -A FORWARD -i 'eth*foo*+' -o 'eth++'
-nft 'add rule ip filter FORWARD iifname "eth\*foo\**" oifname "eth+*" counter'
+# escape only suffix asterisk and translate only the last plus character
+iptables-translate -A FORWARD -i eth*foo*+ -o eth++
+nft 'add rule ip filter FORWARD iifname "eth*foo**" oifname "eth+*" counter'
 
 # skip for always matching interface names
-iptables-translate -A FORWARD -i '+'
+iptables-translate -A FORWARD -i +
 nft 'add rule ip filter FORWARD counter'
 
 # match against invalid interface name to simulate never matching rule
-iptables-translate -A FORWARD ! -i '+'
+iptables-translate -A FORWARD ! -i +
 nft 'add rule ip filter FORWARD iifname "INVAL/D" counter'
diff --git a/iptables/xtables-translate.c b/iptables/xtables-translate.c
index 6b71fcef74b9c..07d6ee40cf727 100644
--- a/iptables/xtables-translate.c
+++ b/iptables/xtables-translate.c
@@ -41,7 +41,9 @@ void xlate_ifname(struct xt_xlate *xl, const char *nftmeta, const char *ifname,
 	for (i = 0, j = 0; i < ifaclen + 1; i++, j++) {
 		switch (ifname[i]) {
 		case '*':
-			iface[j++] = '\\';
+			/* asterisk is non-special mid-string */
+			if (i == ifaclen - 1)
+				iface[j++] = '\\';
 			/* fall through */
 		default:
 			iface[j] = ifname[i];
-- 
2.38.0

[iptables PATCH 7/7] ebtables: Fix MAC address match translation

[Phil Sutter]

If a mask was present, ebtables-translate would emit illegal syntax.

Fixes: 5e2b473a64bc7 ("xtables-compat: extend generic tests for masks and wildcards")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 extensions/generic.txlate | 2 +-
 iptables/nft-bridge.c     | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/extensions/generic.txlate b/extensions/generic.txlate
index d7ddf6a39762e..c24ed1568884b 100644
--- a/extensions/generic.txlate
+++ b/extensions/generic.txlate
@@ -65,7 +65,7 @@ ebtables-translate -A FORWARD ! -i iname --logical-in ilogname -o out+ --logical
 nft 'add rule bridge filter FORWARD iifname != "iname" meta ibrname "ilogname" oifname "out*" meta obrname "lout*" ether daddr 01:02:03:04:de:af counter'
 
 ebtables-translate -I INPUT -p ip -d 1:2:3:4:5:6/ff:ff:ff:ff:00:00
-nft 'insert rule bridge filter INPUT ether type 0x800 ether daddr 01:02:03:04:00:00 and ff:ff:ff:ff:00:00 == 01:02:03:04:00:00 counter'
+nft 'insert rule bridge filter INPUT ether type 0x800 ether daddr and ff:ff:ff:ff:00:00 == 01:02:03:04:00:00 counter'
 
 ebtables-translate -I INPUT -p Length
 nft 'insert rule bridge filter INPUT ether type < 0x0600 counter'
diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index 15dfc585c14ab..8ab21bb54772b 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -861,17 +861,17 @@ static void nft_bridge_xlate_mac(struct xt_xlate *xl, const char *type, bool inv
 
 	xt_xlate_add(xl, "ether %s %s", type, invert ? "!= " : "");
 
-	xlate_mac(xl, mac);
-
 	if (memcmp(mask, one_msk, ETH_ALEN)) {
 		int i;
-		xt_xlate_add(xl, " and ");
+		xt_xlate_add(xl, "and");
 
 		xlate_mac(xl, mask);
 
 		xt_xlate_add(xl, " == %02x", mac[0] & mask[0]);
 		for (i=1; i < ETH_ALEN; i++)
 			xt_xlate_add(xl, ":%02x", mac[i] & mask[i]);
+	} else {
+		xlate_mac(xl, mac);
 	}
 }
 
-- 
2.38.0

Re: [iptables PATCH 0/7] tests: xlate: generic.txlate to pass replay test

[Phil Sutter]

On Thu, Dec 01, 2022 at 05:39:09PM +0100, Phil Sutter wrote:
> Instead of dumping the ruleset with xtables-save and creating yet
> another string comparison mess by searching the output, use --check
> command to leverage iptables' internal rule comparison functionality
> when checking that the nftables-created rule parses correctly as the
> source of the translation (patch 2).
> 
> There was a rub with the above, namely ebtables not supporting --check
> in the first place. Gladly the implementation is pretty simple (patch
> 1) with one caveat: '-C' itself is not available so add the long option
> only.
> 
> The remaining patches deal with translation details (mostly around
> wildcard interface names) until generic.txlate finally passes the replay
> test.
> 
> Phil Sutter (7):
>   ebtables: Implement --check command
>   tests: xlate: Use --check to verify replay
>   nft: Fix for comparing ifname matches against nft-generated ones
>   nft: Fix match generator for '! -i +'
>   nft: Recognize INVAL/D interface name
>   xtables-translate: Fix for interfaces with asterisk mid-string
>   ebtables: Fix MAC address match translation

Series applied.

Re: [iptables PATCH 4/7] nft: Fix match generator for '! -i +'

[Pablo Neira Ayuso]

On Thu, Dec 01, 2022 at 05:39:13PM +0100, Phil Sutter wrote:
> It's actually nonsense since it will never match, but iptables accepts
> it and the resulting nftables rule must behave identically. Reuse the
> solution implemented into xtables-translate (by commit e179e87a1179e)
> and turn the above match into 'iifname INVAL/D'.

Maybe starting bailing out in iptables-nft when ! -i + is used at
ruleset load time?

As you mentioned, this rule is really useless / never matching.

Re: [iptables PATCH 4/7] nft: Fix match generator for '! -i +'

[Phil Sutter]

On Thu, Dec 08, 2022 at 01:23:56PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Dec 01, 2022 at 05:39:13PM +0100, Phil Sutter wrote:
> > It's actually nonsense since it will never match, but iptables accepts
> > it and the resulting nftables rule must behave identically. Reuse the
> > solution implemented into xtables-translate (by commit e179e87a1179e)
> > and turn the above match into 'iifname INVAL/D'.
> 
> Maybe starting bailing out in iptables-nft when ! -i + is used at
> ruleset load time?
> 
> As you mentioned, this rule is really useless / never matching.

Are you fine with doing it in legacy, too?

Cheers, Phil

Re: [iptables PATCH 4/7] nft: Fix match generator for '! -i +'

[Pablo Neira Ayuso]

On Thu, Dec 08, 2022 at 02:19:46PM +0100, Phil Sutter wrote:
> On Thu, Dec 08, 2022 at 01:23:56PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Dec 01, 2022 at 05:39:13PM +0100, Phil Sutter wrote:
> > > It's actually nonsense since it will never match, but iptables accepts
> > > it and the resulting nftables rule must behave identically. Reuse the
> > > solution implemented into xtables-translate (by commit e179e87a1179e)
> > > and turn the above match into 'iifname INVAL/D'.
> > 
> > Maybe starting bailing out in iptables-nft when ! -i + is used at
> > ruleset load time?
> > 
> > As you mentioned, this rule is really useless / never matching.
> 
> Are you fine with doing it in legacy, too?

Have you seen any autogenerated ruleset using this silly ! -i + that
might easily break? Or you are just being conservative while keeping
this around?

Re: [iptables PATCH 1/7] ebtables: Implement --check command

[Pablo Neira Ayuso]

On Thu, Dec 01, 2022 at 05:39:10PM +0100, Phil Sutter wrote:
> Sadly, '-C' is in use already for --change-counters (even though
> ebtables-nft does not implement this), so add a long-option only. It is
> needed for xlate testsuite in replay mode, which will use '--check'
> instead of '-C'.

Hm, yet another of those exotic deviations (from ip{6}tables) in
ebtables.

This -C is not supported by ebtables-nft, right? If so,
according to manpage, ebtables -C takes start_nr[:end_nr].

Maybe there is a chance to get this aligned with other ip{6}tables
tools by checking if optarg is available? Otherwise, really check the
ruleset?

BTW, I'm re-reading the ebtables manpage, not sure how this feature -C
was supposed to be used. Do you understand the usecase?

Re: [iptables PATCH 4/7] nft: Fix match generator for '! -i +'

[Phil Sutter]

On Thu, Dec 08, 2022 at 09:31:56PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Dec 08, 2022 at 02:19:46PM +0100, Phil Sutter wrote:
> > On Thu, Dec 08, 2022 at 01:23:56PM +0100, Pablo Neira Ayuso wrote:
> > > On Thu, Dec 01, 2022 at 05:39:13PM +0100, Phil Sutter wrote:
> > > > It's actually nonsense since it will never match, but iptables accepts
> > > > it and the resulting nftables rule must behave identically. Reuse the
> > > > solution implemented into xtables-translate (by commit e179e87a1179e)
> > > > and turn the above match into 'iifname INVAL/D'.
> > > 
> > > Maybe starting bailing out in iptables-nft when ! -i + is used at
> > > ruleset load time?
> > > 
> > > As you mentioned, this rule is really useless / never matching.
> > 
> > Are you fine with doing it in legacy, too?
> 
> Have you seen any autogenerated ruleset using this silly ! -i + that
> might easily break? Or you are just being conservative while keeping
> this around?

The latter: I was fixing for '-i +' which is legal in iptables but
'iifname "*"' in nftables is not and I also had to find a way to
translate it correctly if inverted.

In theory neither '-i +' nor '! -i +' make sense, from my perspective we
could reject both. Or only the latter since it seems even more bogus
than the former.

I was asking about legacy because I really think we should not change
iptables-nft in a way we wouldn't with legacy. At least rejecting
rulesets which worked fine with legacy is a no go.

Cheers, Phil

Re: [iptables PATCH 1/7] ebtables: Implement --check command

[Phil Sutter]

On Thu, Dec 08, 2022 at 10:40:22PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Dec 01, 2022 at 05:39:10PM +0100, Phil Sutter wrote:
> > Sadly, '-C' is in use already for --change-counters (even though
> > ebtables-nft does not implement this), so add a long-option only. It is
> > needed for xlate testsuite in replay mode, which will use '--check'
> > instead of '-C'.
> 
> Hm, yet another of those exotic deviations (from ip{6}tables) in
> ebtables.
> 
> This -C is not supported by ebtables-nft, right? If so,
> according to manpage, ebtables -C takes start_nr[:end_nr].
> 
> Maybe there is a chance to get this aligned with other ip{6}tables
> tools by checking if optarg is available? Otherwise, really check the
> ruleset?
> 
> BTW, I'm re-reading the ebtables manpage, not sure how this feature -C
> was supposed to be used. Do you understand the usecase?

Yes, it's odd - so fits perfectly the rest of ebtables syntax. ;)

There are two ways to use it:

1) ebtables -C <CHAIN> <RULENO> <PCNT> <BCNT>
2) ebtables -C <CHAIN> <PCNT> <BCNT> <RULESPEC>

So I could check if the two parameters following the chain name are
numbers or not to distinguish between --change-counters and --check, but
it's ugly and with ebtables-nft not supporting one of them makes things
actually worse.

We need --check only for internal purposes, let's please just leave it
like this - there are much more important things to work on.

Cheers, Phil

Re: [iptables PATCH 1/7] ebtables: Implement --check command

[Pablo Neira Ayuso]

On Fri, Dec 09, 2022 at 01:41:24AM +0100, Phil Sutter wrote:
> On Thu, Dec 08, 2022 at 10:40:22PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Dec 01, 2022 at 05:39:10PM +0100, Phil Sutter wrote:
> > > Sadly, '-C' is in use already for --change-counters (even though
> > > ebtables-nft does not implement this), so add a long-option only. It is
> > > needed for xlate testsuite in replay mode, which will use '--check'
> > > instead of '-C'.
> > 
> > Hm, yet another of those exotic deviations (from ip{6}tables) in
> > ebtables.
> > 
> > This -C is not supported by ebtables-nft, right? If so,
> > according to manpage, ebtables -C takes start_nr[:end_nr].
> > 
> > Maybe there is a chance to get this aligned with other ip{6}tables
> > tools by checking if optarg is available? Otherwise, really check the
> > ruleset?
> > 
> > BTW, I'm re-reading the ebtables manpage, not sure how this feature -C
> > was supposed to be used. Do you understand the usecase?
> 
> Yes, it's odd - so fits perfectly the rest of ebtables syntax. ;)
> 
> There are two ways to use it:
> 
> 1) ebtables -C <CHAIN> <RULENO> <PCNT> <BCNT>
> 2) ebtables -C <CHAIN> <PCNT> <BCNT> <RULESPEC>
> 
> So I could check if the two parameters following the chain name are
> numbers or not to distinguish between --change-counters and --check, but
> it's ugly and with ebtables-nft not supporting one of them makes things
> actually worse.
> 
> We need --check only for internal purposes, let's please just leave it
> like this - there are much more important things to work on.

OK, just an idea in case there is a need for getting ebtables more
aligned with other xtables userspace.

Re: [iptables PATCH 1/7] ebtables: Implement --check command

[Phil Sutter]

On Fri, Dec 09, 2022 at 04:23:49PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Dec 09, 2022 at 01:41:24AM +0100, Phil Sutter wrote:
> > On Thu, Dec 08, 2022 at 10:40:22PM +0100, Pablo Neira Ayuso wrote:
> > > On Thu, Dec 01, 2022 at 05:39:10PM +0100, Phil Sutter wrote:
> > > > Sadly, '-C' is in use already for --change-counters (even though
> > > > ebtables-nft does not implement this), so add a long-option only. It is
> > > > needed for xlate testsuite in replay mode, which will use '--check'
> > > > instead of '-C'.
> > > 
> > > Hm, yet another of those exotic deviations (from ip{6}tables) in
> > > ebtables.
> > > 
> > > This -C is not supported by ebtables-nft, right? If so,
> > > according to manpage, ebtables -C takes start_nr[:end_nr].
> > > 
> > > Maybe there is a chance to get this aligned with other ip{6}tables
> > > tools by checking if optarg is available? Otherwise, really check the
> > > ruleset?
> > > 
> > > BTW, I'm re-reading the ebtables manpage, not sure how this feature -C
> > > was supposed to be used. Do you understand the usecase?
> > 
> > Yes, it's odd - so fits perfectly the rest of ebtables syntax. ;)
> > 
> > There are two ways to use it:
> > 
> > 1) ebtables -C <CHAIN> <RULENO> <PCNT> <BCNT>
> > 2) ebtables -C <CHAIN> <PCNT> <BCNT> <RULESPEC>
> > 
> > So I could check if the two parameters following the chain name are
> > numbers or not to distinguish between --change-counters and --check, but
> > it's ugly and with ebtables-nft not supporting one of them makes things
> > actually worse.
> > 
> > We need --check only for internal purposes, let's please just leave it
> > like this - there are much more important things to work on.
> 
> OK, just an idea in case there is a need for getting ebtables more
> aligned with other xtables userspace.

I'd love to, but the syntax is so far off, it's almost futile. :(

Cheers, Phil

Re: [iptables PATCH 1/7] ebtables: Implement --check command

[Pablo Neira Ayuso]

On Fri, Dec 09, 2022 at 05:51:55PM +0100, Phil Sutter wrote:
> On Fri, Dec 09, 2022 at 04:23:49PM +0100, Pablo Neira Ayuso wrote:
> > On Fri, Dec 09, 2022 at 01:41:24AM +0100, Phil Sutter wrote:
> > > On Thu, Dec 08, 2022 at 10:40:22PM +0100, Pablo Neira Ayuso wrote:
> > > > On Thu, Dec 01, 2022 at 05:39:10PM +0100, Phil Sutter wrote:
> > > > > Sadly, '-C' is in use already for --change-counters (even though
> > > > > ebtables-nft does not implement this), so add a long-option only. It is
> > > > > needed for xlate testsuite in replay mode, which will use '--check'
> > > > > instead of '-C'.
> > > > 
> > > > Hm, yet another of those exotic deviations (from ip{6}tables) in
> > > > ebtables.
> > > > 
> > > > This -C is not supported by ebtables-nft, right? If so,
> > > > according to manpage, ebtables -C takes start_nr[:end_nr].
> > > > 
> > > > Maybe there is a chance to get this aligned with other ip{6}tables
> > > > tools by checking if optarg is available? Otherwise, really check the
> > > > ruleset?
> > > > 
> > > > BTW, I'm re-reading the ebtables manpage, not sure how this feature -C
> > > > was supposed to be used. Do you understand the usecase?
> > > 
> > > Yes, it's odd - so fits perfectly the rest of ebtables syntax. ;)
> > > 
> > > There are two ways to use it:
> > > 
> > > 1) ebtables -C <CHAIN> <RULENO> <PCNT> <BCNT>
> > > 2) ebtables -C <CHAIN> <PCNT> <BCNT> <RULESPEC>
> > > 
> > > So I could check if the two parameters following the chain name are
> > > numbers or not to distinguish between --change-counters and --check, but
> > > it's ugly and with ebtables-nft not supporting one of them makes things
> > > actually worse.
> > > 
> > > We need --check only for internal purposes, let's please just leave it
> > > like this - there are much more important things to work on.
> > 
> > OK, just an idea in case there is a need for getting ebtables more
> > aligned with other xtables userspace.
> 
> I'd love to, but the syntax is so far off, it's almost futile. :(

That's just one way to put it.