[iptables-nft RFC 0/5] update iptables-nft dissector

[iptables-nft RFC 0/5] update iptables-nft dissector

[Florian Westphal]

This is n RFC patchset to demonstrate some of the issues
of the xlate-replay mode.

I'm planning to push
 nft-shared: dump errors on stdout to garble output
 xlate-test: extra-escape of '"' for replay mode
 nft: check for unknown meta keys

but not the other changes, at least not yet.

I will try to extend the test script to move beyond
strcmp, see last patch in series:
manually reordering all test files appears to be too error-prone.

Florian Westphal (5):
  nft-shared: dump errors on stdout to garble output
  iptables-nft: do not refuse to decode table with unsupported
    expressions
  nft: check for unknown meta keys
  xlate-test: extra-escape of '"' for replay mode
  generic.xlate: make one replay test case work

 extensions/generic.txlate |  2 +-
 iptables/nft-arp.c        |  9 ++++--
 iptables/nft-bridge.c     |  6 +++-
 iptables/nft-ipv4.c       |  7 +++--
 iptables/nft-ipv6.c       |  7 +++--
 iptables/nft-shared.c     |  6 +++-
 iptables/nft.c            | 66 ++-------------------------------------
 iptables/nft.h            |  2 --
 iptables/xtables-save.c   |  6 +---
 xlate-test.py             |  2 +-
 10 files changed, 31 insertions(+), 82 deletions(-)

-- 
2.37.4

[iptables-nft RFC 1/5] nft-shared: dump errors on stdout to garble output

[Florian Westphal]

Intentionally garble iptables-nft output if we cannot dissect
an expression that we've just encountered, rather than dump an
error message on stderr.

The idea here is that
iptables-save | iptables-restore

will fail, rather than restore an incomplete ruleset.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 iptables/nft-shared.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index 97512e3f43ff..d1f891740c02 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -1169,6 +1169,8 @@ static void nft_parse_lookup(struct nft_xt_ctx *ctx, struct nft_handle *h,
 {
 	if (ctx->h->ops->parse_lookup)
 		ctx->h->ops->parse_lookup(ctx, e);
+	else
+		ctx->errmsg = "cannot handle lookup";
 }
 
 static void nft_parse_range(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
@@ -1245,9 +1247,11 @@ void nft_rule_to_iptables_command_state(struct nft_handle *h,
 			nft_parse_log(&ctx, expr);
 		else if (strcmp(name, "range") == 0)
 			nft_parse_range(&ctx, expr);
+		else
+			printf("unknown expression %s", name);
 
 		if (ctx.errmsg) {
-			fprintf(stderr, "%s", ctx.errmsg);
+			printf("[%s]", ctx.errmsg);
 			ctx.errmsg = NULL;
 		}
 
-- 
2.37.4

Re: [iptables-nft RFC 1/5] nft-shared: dump errors on stdout to garble output

[Phil Sutter]

On Mon, Nov 21, 2022 at 12:19:28PM +0100, Florian Westphal wrote:
> Intentionally garble iptables-nft output if we cannot dissect
> an expression that we've just encountered, rather than dump an
> error message on stderr.
> 
> The idea here is that
> iptables-save | iptables-restore
> 
> will fail, rather than restore an incomplete ruleset.

What I don't like about this is that users won't notice the problem
until they try to restore the ruleset. For us it is clearly beneficial
to see where things break, but I doubt regular users care and we should
just tell them to stop mixing iptables and nft calls.

Can we maybe add "--force" to iptables-nft-save to make it print as much
as possible despite the table being considered incompatible? Not sure
how ugly this is to implement, though.

We still exit(0) in case parsing fails, BTW. Guess this is the most
important thing to fix despite all the above.

Thanks, Phil

Re: [iptables-nft RFC 1/5] nft-shared: dump errors on stdout to garble output

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> What I don't like about this is that users won't notice the problem
> until they try to restore the ruleset. For us it is clearly beneficial
> to see where things break, but I doubt regular users care and we should
> just tell them to stop mixing iptables and nft calls.

So what would you propose...?

> Can we maybe add "--force" to iptables-nft-save to make it print as much
> as possible despite the table being considered incompatible? Not sure
> how ugly this is to implement, though.

I don't see this as useful thing because we already have "nft --debug=netlink".

> We still exit(0) in case parsing fails, BTW. Guess this is the most
> important thing to fix despite all the above.

Huh?
iptables-restore < bla
iptables-restore v1.8.8 (nf_tables): unknown option "--bla"
Error occurred at line: 7 Try `iptables-restore -h' or 'iptables-restore --help' for more information.

... exits with 2.

Can you give an example?

Re: [iptables-nft RFC 1/5] nft-shared: dump errors on stdout to garble output

[Phil Sutter]

On Wed, Nov 23, 2022 at 01:50:32PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > What I don't like about this is that users won't notice the problem
> > until they try to restore the ruleset. For us it is clearly beneficial
> > to see where things break, but I doubt regular users care and we should
> > just tell them to stop mixing iptables and nft calls.
> 
> So what would you propose...?

I like the "table XXX is incompatible" because of how consequent it is.
But this is exactly orthogonal to your emphasis on "print as much as
possible", so not sure if we'll find a compromise. :)

> > Can we maybe add "--force" to iptables-nft-save to make it print as much
> > as possible despite the table being considered incompatible? Not sure
> > how ugly this is to implement, though.
> 
> I don't see this as useful thing because we already have "nft --debug=netlink".

What's your motivation to print parts of the rule which have been parsed
correctly? I assumed it is for debugging purposes.

> > We still exit(0) in case parsing fails, BTW. Guess this is the most
> > important thing to fix despite all the above.
> 
> Huh?
> iptables-restore < bla
> iptables-restore v1.8.8 (nf_tables): unknown option "--bla"
> Error occurred at line: 7 Try `iptables-restore -h' or 'iptables-restore --help' for more information.
> 
> ... exits with 2.
> 
> Can you give an example?

# nft add table ip filter '{ chain FORWARD { \
	type filter hook forward priority filter; \
	ip saddr 10.1.2.3 meta cpu 3 counter accept; }; }'

# nft list ruleset 
table ip filter {
	chain FORWARD {
		type filter hook forward priority filter; policy accept;
		ip saddr 10.1.2.3 meta cpu 3 counter packets 0 bytes 0 accept
	}
}

# iptables-nft -S FORWARD
-P FORWARD ACCEPT
-A FORWARD -s 10.1.2.3/32 -j ACCEPT
# echo $?
0

Note: this is without any of your pending patches applied, I might even
miss pushed commits. I just did this using the current iptables-nft I
have around, but I don't recall any patches changing iptables' return
code if any of the nft_parse_* functions fail.

Cheers, Phil

Re: [iptables-nft RFC 1/5] nft-shared: dump errors on stdout to garble output

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> > Huh?
> > iptables-restore < bla
> > iptables-restore v1.8.8 (nf_tables): unknown option "--bla"
> > Error occurred at line: 7 Try `iptables-restore -h' or 'iptables-restore --help' for more information.
> > 
> > ... exits with 2.
> > 
> > Can you give an example?
> 
> # nft add table ip filter '{ chain FORWARD { \
> 	type filter hook forward priority filter; \
> 	ip saddr 10.1.2.3 meta cpu 3 counter accept; }; }'
> 
> # nft list ruleset 
> table ip filter {
> 	chain FORWARD {
> 		type filter hook forward priority filter; policy accept;
> 		ip saddr 10.1.2.3 meta cpu 3 counter packets 0 bytes 0 accept
> 	}
> }
> 
> # iptables-nft -S FORWARD
> -P FORWARD ACCEPT
> -A FORWARD -s 10.1.2.3/32 -j ACCEPT
> # echo $?
> 0

Ah.  I thought you were talking about iptables-restore/rule parsing.

Re: [iptables-nft RFC 1/5] nft-shared: dump errors on stdout to garble output

[Phil Sutter]

On Wed, Nov 23, 2022 at 02:27:49PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > > Huh?
> > > iptables-restore < bla
> > > iptables-restore v1.8.8 (nf_tables): unknown option "--bla"
> > > Error occurred at line: 7 Try `iptables-restore -h' or 'iptables-restore --help' for more information.
> > > 
> > > ... exits with 2.
> > > 
> > > Can you give an example?
> > 
> > # nft add table ip filter '{ chain FORWARD { \
> > 	type filter hook forward priority filter; \
> > 	ip saddr 10.1.2.3 meta cpu 3 counter accept; }; }'
> > 
> > # nft list ruleset 
> > table ip filter {
> > 	chain FORWARD {
> > 		type filter hook forward priority filter; policy accept;
> > 		ip saddr 10.1.2.3 meta cpu 3 counter packets 0 bytes 0 accept
> > 	}
> > }
> > 
> > # iptables-nft -S FORWARD
> > -P FORWARD ACCEPT
> > -A FORWARD -s 10.1.2.3/32 -j ACCEPT
> > # echo $?
> > 0
> 
> Ah.  I thought you were talking about iptables-restore/rule parsing.

No, my point is that in 'iptables-save | iptables-restore' the first
command should fail already if kernel ruleset is unparseable. :)

Cheers, Phil

[iptables-nft RFC 2/5] iptables-nft: do not refuse to decode table with unsupported expressions

[Florian Westphal]

Plan is to continue and print as much as possible, with a clear
indication/error message when something cannot be decoded.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 iptables/nft.c          | 66 ++---------------------------------------
 iptables/nft.h          |  2 --
 iptables/xtables-save.c |  6 +---
 3 files changed, 3 insertions(+), 71 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 4c0110bb8040..d33591a73616 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -3771,66 +3771,6 @@ uint32_t nft_invflags2cmp(uint32_t invflags, uint32_t flag)
 	return NFT_CMP_EQ;
 }
 
-static const char *supported_exprs[] = {
-	"match",
-	"target",
-	"payload",
-	"meta",
-	"cmp",
-	"bitwise",
-	"counter",
-	"immediate",
-	"lookup",
-	"range",
-};
-
-
-static int nft_is_expr_compatible(struct nftnl_expr *expr, void *data)
-{
-	const char *name = nftnl_expr_get_str(expr, NFTNL_EXPR_NAME);
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(supported_exprs); i++) {
-		if (strcmp(supported_exprs[i], name) == 0)
-			return 0;
-	}
-
-	if (!strcmp(name, "limit") &&
-	    nftnl_expr_get_u32(expr, NFTNL_EXPR_LIMIT_TYPE) == NFT_LIMIT_PKTS &&
-	    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;
-}
-
-static int nft_is_rule_compatible(struct nftnl_rule *rule, void *data)
-{
-	return nftnl_expr_foreach(rule, nft_is_expr_compatible, NULL);
-}
-
-static int nft_is_chain_compatible(struct nft_chain *nc, void *data)
-{
-	struct nftnl_chain *c = nc->nftnl;
-
-	return nftnl_rule_foreach(c, nft_is_rule_compatible, NULL);
-}
-
-bool nft_is_table_compatible(struct nft_handle *h,
-			     const char *table, const char *chain)
-{
-	if (chain) {
-		struct nft_chain *c = nft_chain_find(h, table, chain);
-
-		return c && !nft_is_chain_compatible(c, h);
-	}
-
-	return !nft_chain_foreach(h, table, nft_is_chain_compatible, h);
-}
-
 bool nft_is_table_tainted(struct nft_handle *h, const char *table)
 {
 	const struct builtin_table *t = nft_table_builtin_find(h, table);
@@ -3843,10 +3783,8 @@ void nft_assert_table_compatible(struct nft_handle *h,
 {
 	const char *pfx = "", *sfx = "";
 
-	if (nft_is_table_compatible(h, table, chain)) {
-		if (nft_is_table_tainted(h, table))
-			printf("# Table `%s' contains incompatible base-chains, use 'nft' tool to list them.\n",
-			       table);
+	if (nft_is_table_tainted(h, table)) {
+		printf("# Table `%s' contains incompatible base-chains, use 'nft' tool to list them.\n", table);
 		return;
 	}
 
diff --git a/iptables/nft.h b/iptables/nft.h
index 68b0910c8e18..4f742dbaf180 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -263,8 +263,6 @@ int nft_arp_rule_insert(struct nft_handle *h, const char *chain,
 
 void nft_rule_to_arpt_entry(struct nftnl_rule *r, struct arpt_entry *fw);
 
-bool nft_is_table_compatible(struct nft_handle *h,
-			     const char *table, const char *chain);
 bool nft_is_table_tainted(struct nft_handle *h, const char *table);
 void nft_assert_table_compatible(struct nft_handle *h,
 				 const char *table, const char *chain);
diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 5a82cac5dd7c..c9f87322834b 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -74,11 +74,7 @@ __do_output(struct nft_handle *h, const char *tablename, void *data)
 	if (!nft_table_builtin_find(h, tablename))
 		return 0;
 
-	if (!nft_is_table_compatible(h, tablename, NULL)) {
-		printf("# Table `%s' is incompatible, use 'nft' tool.\n",
-		       tablename);
-		return 0;
-	} else if (nft_is_table_tainted(h, tablename)) {
+	if (nft_is_table_tainted(h, tablename)) {
 		printf("# Table `%s' contains incompatible base-chains, use 'nft' tool to list them.\n",
 		       tablename);
 	}
-- 
2.37.4

[iptables-nft RFC 3/5] nft: check for unknown meta keys

[Florian Westphal]

Set ->errmsg when the meta key isn't supported by iptables-nft instead
of pretending everything is fine.

The old code is good enough to handle rules added by iptables-nft, but
its not enough to handle rules added by native nft.

At least make sure that there is a an error message telling that
iptables-nft could not decode the entire ruleset.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 iptables/nft-arp.c    | 9 ++++++---
 iptables/nft-bridge.c | 6 +++++-
 iptables/nft-ipv4.c   | 7 +++++--
 iptables/nft-ipv6.c   | 7 +++++--
 4 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index e9e111416d79..59f100af2a6b 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -168,11 +168,14 @@ static void nft_arp_parse_meta(struct nft_xt_ctx *ctx,
 	struct arpt_entry *fw = &cs->arp;
 	uint8_t flags = 0;
 
-	parse_meta(ctx, e, reg->meta_dreg.key, fw->arp.iniface, fw->arp.iniface_mask,
+	if (parse_meta(ctx, e, reg->meta_dreg.key, fw->arp.iniface, fw->arp.iniface_mask,
 		   fw->arp.outiface, fw->arp.outiface_mask,
-		   &flags);
+		   &flags) == 0) {
+		fw->arp.invflags |= flags;
+		return;
+	}
 
-	fw->arp.invflags |= flags;
+	ctx->errmsg = "Unknown arp meta key";
 }
 
 static void parse_mask_ipv4(const struct nft_xt_ctx_reg *reg, struct in_addr *mask)
diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index 749cbc6fbbaf..e8ac7a364169 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -197,7 +197,10 @@ static void nft_bridge_parse_meta(struct nft_xt_ctx *ctx,
 	uint8_t invflags = 0;
 	char iifname[IFNAMSIZ] = {}, oifname[IFNAMSIZ] = {};
 
-	parse_meta(ctx, e, reg->meta_dreg.key, iifname, NULL, oifname, NULL, &invflags);
+	if (parse_meta(ctx, e, reg->meta_dreg.key, iifname, NULL, oifname, NULL, &invflags) < 0) {
+		ctx->errmsg = "unknown meta key";
+		return;
+	}
 
 	switch (reg->meta_dreg.key) {
 	case NFT_META_BRI_IIFNAME:
@@ -221,6 +224,7 @@ static void nft_bridge_parse_meta(struct nft_xt_ctx *ctx,
 		snprintf(fw->out, sizeof(fw->out), "%s", oifname);
 		break;
 	default:
+		ctx->errmsg = "unknown bridge meta key";
 		break;
 	}
 }
diff --git a/iptables/nft-ipv4.c b/iptables/nft-ipv4.c
index 92a914f1a4a4..6c62dd46ddda 100644
--- a/iptables/nft-ipv4.c
+++ b/iptables/nft-ipv4.c
@@ -146,9 +146,12 @@ static void nft_ipv4_parse_meta(struct nft_xt_ctx *ctx,
 		break;
 	}
 
-	parse_meta(ctx, e, reg->meta_dreg.key, cs->fw.ip.iniface, cs->fw.ip.iniface_mask,
+	if (parse_meta(ctx, e, reg->meta_dreg.key, cs->fw.ip.iniface, cs->fw.ip.iniface_mask,
 		   cs->fw.ip.outiface, cs->fw.ip.outiface_mask,
-		   &cs->fw.ip.invflags);
+		   &cs->fw.ip.invflags) == 0)
+		return;
+
+	ctx->errmsg = "unknown ipv4 meta key";
 }
 
 static void parse_mask_ipv4(const struct nft_xt_ctx_reg *sreg, struct in_addr *mask)
diff --git a/iptables/nft-ipv6.c b/iptables/nft-ipv6.c
index 7ca9d842f2b1..98c35afa67ad 100644
--- a/iptables/nft-ipv6.c
+++ b/iptables/nft-ipv6.c
@@ -119,9 +119,12 @@ static void nft_ipv6_parse_meta(struct nft_xt_ctx *ctx,
 		break;
 	}
 
-	parse_meta(ctx, e, reg->meta_dreg.key, cs->fw6.ipv6.iniface,
+	if (parse_meta(ctx, e, reg->meta_dreg.key, cs->fw6.ipv6.iniface,
 		   cs->fw6.ipv6.iniface_mask, cs->fw6.ipv6.outiface,
-		   cs->fw6.ipv6.outiface_mask, &cs->fw6.ipv6.invflags);
+		   cs->fw6.ipv6.outiface_mask, &cs->fw6.ipv6.invflags) == 0)
+		return;
+
+	ctx->errmsg = "unknown ipv6 meta key";
 }
 
 static void parse_mask_ipv6(const struct nft_xt_ctx_reg *reg,
-- 
2.37.4

[iptables-nft RFC 4/5] xlate-test: extra-escape of '"' for replay mode

[Florian Westphal]

Before, nft fails to restore some rules because it sees:
insert rule ip filter INPUT iifname iifname ip ...

Add extra escaping for " so that the shell won't remove it and
nft will see 'iifname "iifname"'.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 xlate-test.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xlate-test.py b/xlate-test.py
index f3fcd797af90..5711a6427d78 100755
--- a/xlate-test.py
+++ b/xlate-test.py
@@ -104,7 +104,7 @@ def test_one_replay(name, sourceline, expected, result):
             "flush ruleset",
             "add table " + fam + table_name,
             "add chain " + fam + table_name + " " + chain_name
-    ] + [ l.removeprefix("nft ") for l in expected.split("\n") ]
+    ] + [ l.removeprefix("nft ") for l in expected.replace('\"', '\\"', -1).split("\n") ]
 
     # feed input via the pipe to make sure the shell "does its thing"
     cmd = "echo \"" + "\n".join(nft_input) + "\" | " + args.nft + " -f -"
-- 
2.37.4

Re: [iptables-nft RFC 4/5] xlate-test: extra-escape of '"' for replay mode

[Phil Sutter]

On Mon, Nov 21, 2022 at 12:19:31PM +0100, Florian Westphal wrote:
> Before, nft fails to restore some rules because it sees:
> insert rule ip filter INPUT iifname iifname ip ...
> 
> Add extra escaping for " so that the shell won't remove it and
> nft will see 'iifname "iifname"'.

This is fixing up the wrong side, see:

struct xt_xlate_{mt,tg}_params::escape_quotes

this is set if iptables-translate was called and unset if
iptables-restore-translate was called. I didn't invent this, but the
logic seems to be escape quotes when printing a command, don't when
printing a dump file content.

I have a patch in my queue which extends the conditional quoting to
interface names. Will submit it later today along with other fixes in
that corner.

Cheers, Phil

Re: [iptables-nft RFC 4/5] xlate-test: extra-escape of '"' for replay mode

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> On Mon, Nov 21, 2022 at 12:19:31PM +0100, Florian Westphal wrote:
> > Before, nft fails to restore some rules because it sees:
> > insert rule ip filter INPUT iifname iifname ip ...
> > 
> > Add extra escaping for " so that the shell won't remove it and
> > nft will see 'iifname "iifname"'.
> 
> This is fixing up the wrong side, see:

Not sure what you mean here.

The quotes ARE printed, but the shell strips them away.

> struct xt_xlate_{mt,tg}_params::escape_quotes

Ick.

> this is set if iptables-translate was called and unset if
> iptables-restore-translate was called. I didn't invent this, but the
> logic seems to be escape quotes when printing a command, don't when
> printing a dump file content.
> 
> I have a patch in my queue which extends the conditional quoting to
> interface names. Will submit it later today along with other fixes in
> that corner.

I would prefer to rip this out, I don't think any of the tools should
print '\"' instead of '"'.

Re: [iptables-nft RFC 4/5] xlate-test: extra-escape of '"' for replay mode

[Phil Sutter]

On Tue, Nov 22, 2022 at 05:01:28PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > On Mon, Nov 21, 2022 at 12:19:31PM +0100, Florian Westphal wrote:
> > > Before, nft fails to restore some rules because it sees:
> > > insert rule ip filter INPUT iifname iifname ip ...
> > > 
> > > Add extra escaping for " so that the shell won't remove it and
> > > nft will see 'iifname "iifname"'.
> > 
> > This is fixing up the wrong side, see:
> 
> Not sure what you mean here.
> 
> The quotes ARE printed, but the shell strips them away.
> 
> > struct xt_xlate_{mt,tg}_params::escape_quotes
> 
> Ick.
> 
> > this is set if iptables-translate was called and unset if
> > iptables-restore-translate was called. I didn't invent this, but the
> > logic seems to be escape quotes when printing a command, don't when
> > printing a dump file content.
> > 
> > I have a patch in my queue which extends the conditional quoting to
> > interface names. Will submit it later today along with other fixes in
> > that corner.
> 
> I would prefer to rip this out, I don't think any of the tools should
> print '\"' instead of '"'.

Either way is fine with me. See how I explicitly call 'echo "<input>" |
nft -f -' in xlate-test.py to force evaluation by the shell - an earlier
version of that code would break since nft saw the escapes. So *we*
don't need them, but one could argue it educates users that they'll have
to escape the quotes if they specify them on command line.

Cheers, Phil

Re: [iptables-nft RFC 4/5] xlate-test: extra-escape of '"' for replay mode

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> On Tue, Nov 22, 2022 at 05:01:28PM +0100, Florian Westphal wrote:
> > Phil Sutter <phil@nwl.cc> wrote:
> > > On Mon, Nov 21, 2022 at 12:19:31PM +0100, Florian Westphal wrote:
> > > > Before, nft fails to restore some rules because it sees:
> > > > insert rule ip filter INPUT iifname iifname ip ...
> > > > 
> > > > Add extra escaping for " so that the shell won't remove it and
> > > > nft will see 'iifname "iifname"'.
> > > 
> > > This is fixing up the wrong side, see:
> > 
> > Not sure what you mean here.
> > 
> > The quotes ARE printed, but the shell strips them away.
> > 
> > > struct xt_xlate_{mt,tg}_params::escape_quotes
> > 
> > Ick.
> > 
> > > this is set if iptables-translate was called and unset if
> > > iptables-restore-translate was called. I didn't invent this, but the
> > > logic seems to be escape quotes when printing a command, don't when
> > > printing a dump file content.
> > > 
> > > I have a patch in my queue which extends the conditional quoting to
> > > interface names. Will submit it later today along with other fixes in
> > > that corner.
> > 
> > I would prefer to rip this out, I don't think any of the tools should
> > print '\"' instead of '"'.
> 
> Either way is fine with me. See how I explicitly call 'echo "<input>" |
> nft -f -' in xlate-test.py to force evaluation by the shell - an earlier
> version of that code would break since nft saw the escapes. So *we*
> don't need them, but one could argue it educates users that they'll have
> to escape the quotes if they specify them on command line.

What if we replace:
iptables-translate  -A INPUT -j LOG --log-prefix "foo bar"
nft add rule ip filter INPUT counter log prefix \"foo bar\"

With
nft add rule ip filter INPUT 'counter log prefix "foo bar"'

IOW, get rid of all escaped_quotes code and change:

-       printf("%s\n", xt_xlate_rule_get(xl));
+
+       if (cs->restore)
+               printf("%s\n", xt_xlate_rule_get(xl));
+       else
+               printf("'%s'\n", xt_xlate_rule_get(xl));


... this would always place everything rule-related printed by xtables-translate in
single quotes while leaving iptables-restore-translate alone.

What do you think?

Re: [iptables-nft RFC 4/5] xlate-test: extra-escape of '"' for replay mode

[Phil Sutter]

On Wed, Nov 23, 2022 at 10:31:54AM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > On Tue, Nov 22, 2022 at 05:01:28PM +0100, Florian Westphal wrote:
> > > Phil Sutter <phil@nwl.cc> wrote:
> > > > On Mon, Nov 21, 2022 at 12:19:31PM +0100, Florian Westphal wrote:
> > > > > Before, nft fails to restore some rules because it sees:
> > > > > insert rule ip filter INPUT iifname iifname ip ...
> > > > > 
> > > > > Add extra escaping for " so that the shell won't remove it and
> > > > > nft will see 'iifname "iifname"'.
> > > > 
> > > > This is fixing up the wrong side, see:
> > > 
> > > Not sure what you mean here.
> > > 
> > > The quotes ARE printed, but the shell strips them away.
> > > 
> > > > struct xt_xlate_{mt,tg}_params::escape_quotes
> > > 
> > > Ick.
> > > 
> > > > this is set if iptables-translate was called and unset if
> > > > iptables-restore-translate was called. I didn't invent this, but the
> > > > logic seems to be escape quotes when printing a command, don't when
> > > > printing a dump file content.
> > > > 
> > > > I have a patch in my queue which extends the conditional quoting to
> > > > interface names. Will submit it later today along with other fixes in
> > > > that corner.
> > > 
> > > I would prefer to rip this out, I don't think any of the tools should
> > > print '\"' instead of '"'.
> > 
> > Either way is fine with me. See how I explicitly call 'echo "<input>" |
> > nft -f -' in xlate-test.py to force evaluation by the shell - an earlier
> > version of that code would break since nft saw the escapes. So *we*
> > don't need them, but one could argue it educates users that they'll have
> > to escape the quotes if they specify them on command line.
> 
> What if we replace:
> iptables-translate  -A INPUT -j LOG --log-prefix "foo bar"
> nft add rule ip filter INPUT counter log prefix \"foo bar\"
> 
> With
> nft add rule ip filter INPUT 'counter log prefix "foo bar"'
> 
> IOW, get rid of all escaped_quotes code and change:
> 
> -       printf("%s\n", xt_xlate_rule_get(xl));
> +
> +       if (cs->restore)
> +               printf("%s\n", xt_xlate_rule_get(xl));
> +       else
> +               printf("'%s'\n", xt_xlate_rule_get(xl));
> 
> 
> ... this would always place everything rule-related printed by xtables-translate in
> single quotes while leaving iptables-restore-translate alone.
> 
> What do you think?

This looks like a very nice solution to eliminate the conditional
escaping all over the place! Obviously, it means a mass-update of all
txlate files.

Thanks, Phil

[iptables-nft RFC 5/5] generic.xlate: make one replay test case work

[Florian Westphal]

This is just to demonstrate yet another problem.

For the rule itself it doesn't matter if '-i' or '-s' is passed first,
but the test script has no deeper understanding for the rules and will
do a simple textual comparision, this will fail because as-is the output
is different than the input (options are written out in different
order).

We either need to sanoitize the input or update the test script to
split lines and re-order the options or similar.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 extensions/generic.txlate | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/extensions/generic.txlate b/extensions/generic.txlate
index 6779d6f86dec..e95432552ef8 100644
--- a/extensions/generic.txlate
+++ b/extensions/generic.txlate
@@ -4,7 +4,7 @@ nft insert rule ip filter OUTPUT ip protocol udp ip daddr 8.8.8.8 counter accept
 iptables-translate -F -t nat
 nft flush table ip nat
 
-iptables-translate -I INPUT -i iifname -s 10.0.0.0/8
+iptables-translate -I INPUT -s 10.0.0.0/8 -i iifname
 nft insert rule ip filter INPUT iifname "iifname" ip saddr 10.0.0.0/8 counter
 
 iptables-translate -A INPUT -i iif+ ! -d 10.0.0.0/8
-- 
2.37.4

Re: [iptables-nft RFC 5/5] generic.xlate: make one replay test case work

[Phil Sutter]

On Mon, Nov 21, 2022 at 12:19:32PM +0100, Florian Westphal wrote:
> This is just to demonstrate yet another problem.
> 
> For the rule itself it doesn't matter if '-i' or '-s' is passed first,
> but the test script has no deeper understanding for the rules and will
> do a simple textual comparision, this will fail because as-is the output
> is different than the input (options are written out in different
> order).
> 
> We either need to sanoitize the input or update the test script to
> split lines and re-order the options or similar.

My solution was to add replay records to test files like so:

| diff --git a/extensions/generic.txlate b/extensions/generic.txlate
| index 6779d6f86dec8..8c3b7dbeb7320 100644
| --- a/extensions/generic.txlate
| +++ b/extensions/generic.txlate
| @@ -1,69 +1,70 @@
| -iptables-translate -I OUTPUT -p udp -d 8.8.8.8 -j ACCEPT
| +iptables-translate -I OUTPUT -p udp -d 8.8.8.8 -j ACCEPT;-d 8.8.8.8/32 -p udp -j ACCEPT
|  nft insert rule ip filter OUTPUT ip protocol udp ip daddr 8.8.8.8 counter accept
|  
|  iptables-translate -F -t nat
|  nft flush table ip nat

Since iptables is able to compare rules though, we could utilize this.
So when checking the replay, instead of calling iptables-save and
searching the output, we could call 'iptables -C'. I'll try this, it
sounds simple and doable.

Thanks, Phil