[PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Florian Westphal]

  tcp option 254 length ge 4

... will segfault.
The crash bug is that tcpopt_expr_alloc() can return NULL if we cannot
find a suitable template for the requested kind + field combination,
so add the needed error handling in the bison parser.

However, we can handle this.  NOP and EOL have templates, all other
options (known or unknown) must also have a length field.

So also add a fallback template to handle both kind and length, even
if only a numeric option is given that nft doesn't recognize.

Don't bother with output, above will be printed via raw syntax, i.e.
tcp option @254,8,8 >= 4.

Fixes: 24d8da308342 ("tcpopt: allow to check for presence of any tcp option")
Reported-by: Maciej Żenczykowski <zenczykowski@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 v2: MUST bump exthdr.offset, else this continues to check for 'kind',
 even if 'length' was asked for.
 Also fix the dump file, it was not correct (254,0,8 instead of 254,8,8).

 src/parser_bison.y                            |  4 ++
 src/tcpopt.c                                  | 28 +++++++++----
 .../packetpath/dumps/tcp_options.nft          | 14 +++++++
 tests/shell/testcases/packetpath/tcp_options  | 39 +++++++++++++++++++
 4 files changed, 78 insertions(+), 7 deletions(-)
 create mode 100644 tests/shell/testcases/packetpath/dumps/tcp_options.nft
 create mode 100755 tests/shell/testcases/packetpath/tcp_options

diff --git a/src/parser_bison.y b/src/parser_bison.y
index ee7e9e14c1f2..1a3d64f794cb 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -5828,6 +5828,10 @@ tcp_hdr_expr		:	TCP	tcp_hdr_field
 			|	TCP	OPTION	tcp_hdr_option_kind_and_field
 			{
 				$$ = tcpopt_expr_alloc(&@$, $3.kind, $3.field);
+				if ($$ == NULL) {
+					erec_queue(error(&@1, "Could not find a tcp option template"), state->msgs);
+					YYERROR;
+				}
 			}
 			|	TCP	OPTION	AT	close_scope_at	tcp_hdr_option_type	COMMA	NUM	COMMA	NUM
 			{
diff --git a/src/tcpopt.c b/src/tcpopt.c
index 3fcb2731ae73..93b08c8cc0f2 100644
--- a/src/tcpopt.c
+++ b/src/tcpopt.c
@@ -118,6 +118,13 @@ static const struct exthdr_desc tcpopt_mptcp = {
 		[TCPOPT_MPTCP_SUBTYPE]  = PHT("subtype", 16, 4),
 	},
 };
+
+static const struct exthdr_desc tcpopt_fallback = {
+	.templates	= {
+		[TCPOPT_COMMON_KIND]	= PHT("kind",   0, 8),
+		[TCPOPT_COMMON_LENGTH]	= PHT("length", 8, 8),
+	},
+};
 #undef PHT
 
 const struct exthdr_desc *tcpopt_protocols[] = {
@@ -182,19 +189,24 @@ struct expr *tcpopt_expr_alloc(const struct location *loc,
 		desc = tcpopt_protocols[kind];
 
 	if (!desc) {
-		if (field != TCPOPT_COMMON_KIND || kind > 255)
+		if (kind > 255)
 			return NULL;
 
+		switch (field) {
+		case TCPOPT_COMMON_KIND:
+		case TCPOPT_COMMON_LENGTH:
+			break;
+		default:
+			return NULL;
+		}
+
 		expr = expr_alloc(loc, EXPR_EXTHDR, &integer_type,
 				  BYTEORDER_BIG_ENDIAN, 8);
 
-		desc = tcpopt_protocols[TCPOPT_NOP];
+		desc = &tcpopt_fallback;
 		tmpl = &desc->templates[field];
-		expr->exthdr.desc   = desc;
-		expr->exthdr.tmpl   = tmpl;
-		expr->exthdr.op = NFT_EXTHDR_OP_TCPOPT;
 		expr->exthdr.raw_type = kind;
-		return expr;
+		goto out_finalize;
 	}
 
 	tmpl = &desc->templates[field];
@@ -203,10 +215,12 @@ struct expr *tcpopt_expr_alloc(const struct location *loc,
 
 	expr = expr_alloc(loc, EXPR_EXTHDR, tmpl->dtype,
 			  BYTEORDER_BIG_ENDIAN, tmpl->len);
+
+	expr->exthdr.raw_type = desc->type;
+out_finalize:
 	expr->exthdr.desc   = desc;
 	expr->exthdr.tmpl   = tmpl;
 	expr->exthdr.op     = NFT_EXTHDR_OP_TCPOPT;
-	expr->exthdr.raw_type = desc->type;
 	expr->exthdr.offset = tmpl->offset;
 
 	return expr;
diff --git a/tests/shell/testcases/packetpath/dumps/tcp_options.nft b/tests/shell/testcases/packetpath/dumps/tcp_options.nft
new file mode 100644
index 000000000000..03e50a56e8c9
--- /dev/null
+++ b/tests/shell/testcases/packetpath/dumps/tcp_options.nft
@@ -0,0 +1,14 @@
+table inet t {
+	chain c {
+		type filter hook output priority filter; policy accept;
+		tcp dport != 22345 accept
+		tcp flags syn / fin,syn,rst,ack tcp option @254,8,8 >= 4 counter packets 0 bytes 0 drop
+		tcp flags syn / fin,syn,rst,ack tcp option fastopen length >= 2 reset tcp option fastopen counter packets 0 bytes 0
+		tcp flags syn / fin,syn,rst,ack tcp option sack-perm missing counter packets 0 bytes 0 drop
+		tcp flags syn / fin,syn,rst,ack tcp option sack-perm exists counter packets 1 bytes 60
+		tcp flags syn / fin,syn,rst,ack tcp option maxseg size > 1400 counter packets 1 bytes 60
+		tcp flags syn / fin,syn,rst,ack tcp option nop missing counter packets 0 bytes 0
+		tcp flags syn / fin,syn,rst,ack tcp option nop exists counter packets 1 bytes 60
+		tcp flags syn / fin,syn,rst,ack drop
+	}
+}
diff --git a/tests/shell/testcases/packetpath/tcp_options b/tests/shell/testcases/packetpath/tcp_options
new file mode 100755
index 000000000000..0f1ca2644655
--- /dev/null
+++ b/tests/shell/testcases/packetpath/tcp_options
@@ -0,0 +1,39 @@
+#!/bin/bash
+
+have_socat="no"
+socat -h > /dev/null && have_socat="yes"
+
+ip link set lo up
+
+$NFT -f /dev/stdin <<EOF
+table inet t {
+	chain c {
+		type filter hook output priority 0;
+		tcp dport != 22345 accept
+		tcp flags syn / fin,syn,rst,ack tcp option 254  length ge 4 counter drop
+		tcp flags syn / fin,syn,rst,ack tcp option fastopen length ge 2 reset tcp option fastopen counter
+		tcp flags syn / fin,syn,rst,ack tcp option sack-perm missing counter drop
+		tcp flags syn / fin,syn,rst,ack tcp option sack-perm exists counter
+		tcp flags syn / fin,syn,rst,ack tcp option maxseg size gt 1400 counter
+		tcp flags syn / fin,syn,rst,ack tcp option nop missing counter
+		tcp flags syn / fin,syn,rst,ack tcp option nop exists counter
+		tcp flags syn / fin,syn,rst,ack drop
+	}
+}
+EOF
+
+if [ $? -ne 0 ]; then
+	exit 1
+fi
+
+if [ $have_socat != "yes" ]; then
+	echo "Ran partial test, socat not available (skipped)"
+	exit 77
+fi
+
+# This will fail (drop in output -> connect fails with eperm)
+socat -t 3 -u STDIN TCP:127.0.0.1:22345,connect-timeout=1 < /dev/null > /dev/null
+
+# Indicate success, dump file has incremented packet counter where its
+# expected to match.
+exit 0
-- 
2.41.0

Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Pablo Neira Ayuso]

On Tue, Dec 05, 2023 at 12:56:08PM +0100, Florian Westphal wrote:
>   tcp option 254 length ge 4
> 
> ... will segfault.
> The crash bug is that tcpopt_expr_alloc() can return NULL if we cannot
> find a suitable template for the requested kind + field combination,
> so add the needed error handling in the bison parser.
> 
> However, we can handle this.  NOP and EOL have templates, all other
> options (known or unknown) must also have a length field.
> 
> So also add a fallback template to handle both kind and length, even
> if only a numeric option is given that nft doesn't recognize.
> 
> Don't bother with output, above will be printed via raw syntax, i.e.
> tcp option @254,8,8 >= 4.
> 
> Fixes: 24d8da308342 ("tcpopt: allow to check for presence of any tcp option")
> Reported-by: Maciej Żenczykowski <zenczykowski@gmail.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  v2: MUST bump exthdr.offset, else this continues to check for 'kind',
>  even if 'length' was asked for.
>  Also fix the dump file, it was not correct (254,0,8 instead of 254,8,8).
> 
>  src/parser_bison.y                            |  4 ++
>  src/tcpopt.c                                  | 28 +++++++++----
>  .../packetpath/dumps/tcp_options.nft          | 14 +++++++
>  tests/shell/testcases/packetpath/tcp_options  | 39 +++++++++++++++++++
>  4 files changed, 78 insertions(+), 7 deletions(-)
>  create mode 100644 tests/shell/testcases/packetpath/dumps/tcp_options.nft
>  create mode 100755 tests/shell/testcases/packetpath/tcp_options
> 
> diff --git a/src/parser_bison.y b/src/parser_bison.y
> index ee7e9e14c1f2..1a3d64f794cb 100644
> --- a/src/parser_bison.y
> +++ b/src/parser_bison.y
> @@ -5828,6 +5828,10 @@ tcp_hdr_expr		:	TCP	tcp_hdr_field
>  			|	TCP	OPTION	tcp_hdr_option_kind_and_field
>  			{
>  				$$ = tcpopt_expr_alloc(&@$, $3.kind, $3.field);
> +				if ($$ == NULL) {
> +					erec_queue(error(&@1, "Could not find a tcp option template"), state->msgs);
> +					YYERROR;
> +				}
>  			}
>  			|	TCP	OPTION	AT	close_scope_at	tcp_hdr_option_type	COMMA	NUM	COMMA	NUM
>  			{
> diff --git a/src/tcpopt.c b/src/tcpopt.c
> index 3fcb2731ae73..93b08c8cc0f2 100644
> --- a/src/tcpopt.c
> +++ b/src/tcpopt.c
> @@ -118,6 +118,13 @@ static const struct exthdr_desc tcpopt_mptcp = {
>  		[TCPOPT_MPTCP_SUBTYPE]  = PHT("subtype", 16, 4),
>  	},
>  };
> +
> +static const struct exthdr_desc tcpopt_fallback = {
> +	.templates	= {
> +		[TCPOPT_COMMON_KIND]	= PHT("kind",   0, 8),
> +		[TCPOPT_COMMON_LENGTH]	= PHT("length", 8, 8),
> +	},
> +};
>  #undef PHT
>  
>  const struct exthdr_desc *tcpopt_protocols[] = {
> @@ -182,19 +189,24 @@ struct expr *tcpopt_expr_alloc(const struct location *loc,
>  		desc = tcpopt_protocols[kind];
>  
>  	if (!desc) {
> -		if (field != TCPOPT_COMMON_KIND || kind > 255)
> +		if (kind > 255)
>  			return NULL;

Another suggestion: Remove this NULL, it leaves lhs as NULL in the
relational. kind > 255 cannot ever happen, parser rejects numbers over
255.

ruleset.nft:1:30-32: Error: value too large
add rule inet x y tcp option 256 length 4
                             ^^^

you could turn it into assert().

> +		switch (field) {
> +		case TCPOPT_COMMON_KIND:
> +		case TCPOPT_COMMON_LENGTH:
> +			break;
> +		default:
> +			return NULL;

Probably instead of this code above:

+		desc = &tcpopt_fallback;
+               if (field > TCPOPT_COMMON_LENGTH)
+                       tmpl = &tcpopt_unknown_template;
+               else
+                       tmpl = &desc->templates[field];

so no NULL is ever returned and fall back to the unknown template.

> +		}
> +
>  		expr = expr_alloc(loc, EXPR_EXTHDR, &integer_type,
>  				  BYTEORDER_BIG_ENDIAN, 8);
>  
> -		desc = tcpopt_protocols[TCPOPT_NOP];
> +		desc = &tcpopt_fallback;
>  		tmpl = &desc->templates[field];
> -		expr->exthdr.desc   = desc;
> -		expr->exthdr.tmpl   = tmpl;
> -		expr->exthdr.op = NFT_EXTHDR_OP_TCPOPT;
>  		expr->exthdr.raw_type = kind;
> -		return expr;
> +		goto out_finalize;

Maybe add a helper function to set up expr->exthdr to avoid the goto
trick. But not a deal breaker, it is OK if you prefer it this way.

Thanks!

>  	}
>  
>  	tmpl = &desc->templates[field];
> @@ -203,10 +215,12 @@ struct expr *tcpopt_expr_alloc(const struct location *loc,
>  
>  	expr = expr_alloc(loc, EXPR_EXTHDR, tmpl->dtype,
>  			  BYTEORDER_BIG_ENDIAN, tmpl->len);
> +
> +	expr->exthdr.raw_type = desc->type;
> +out_finalize:
>  	expr->exthdr.desc   = desc;
>  	expr->exthdr.tmpl   = tmpl;
>  	expr->exthdr.op     = NFT_EXTHDR_OP_TCPOPT;
> -	expr->exthdr.raw_type = desc->type;
>  	expr->exthdr.offset = tmpl->offset;
>  
>  	return expr;
> diff --git a/tests/shell/testcases/packetpath/dumps/tcp_options.nft b/tests/shell/testcases/packetpath/dumps/tcp_options.nft
> new file mode 100644
> index 000000000000..03e50a56e8c9
> --- /dev/null
> +++ b/tests/shell/testcases/packetpath/dumps/tcp_options.nft
> @@ -0,0 +1,14 @@
> +table inet t {
> +	chain c {
> +		type filter hook output priority filter; policy accept;
> +		tcp dport != 22345 accept
> +		tcp flags syn / fin,syn,rst,ack tcp option @254,8,8 >= 4 counter packets 0 bytes 0 drop
> +		tcp flags syn / fin,syn,rst,ack tcp option fastopen length >= 2 reset tcp option fastopen counter packets 0 bytes 0
> +		tcp flags syn / fin,syn,rst,ack tcp option sack-perm missing counter packets 0 bytes 0 drop
> +		tcp flags syn / fin,syn,rst,ack tcp option sack-perm exists counter packets 1 bytes 60
> +		tcp flags syn / fin,syn,rst,ack tcp option maxseg size > 1400 counter packets 1 bytes 60
> +		tcp flags syn / fin,syn,rst,ack tcp option nop missing counter packets 0 bytes 0
> +		tcp flags syn / fin,syn,rst,ack tcp option nop exists counter packets 1 bytes 60
> +		tcp flags syn / fin,syn,rst,ack drop
> +	}
> +}
> diff --git a/tests/shell/testcases/packetpath/tcp_options b/tests/shell/testcases/packetpath/tcp_options
> new file mode 100755
> index 000000000000..0f1ca2644655
> --- /dev/null
> +++ b/tests/shell/testcases/packetpath/tcp_options
> @@ -0,0 +1,39 @@
> +#!/bin/bash
> +
> +have_socat="no"
> +socat -h > /dev/null && have_socat="yes"
> +
> +ip link set lo up
> +
> +$NFT -f /dev/stdin <<EOF
> +table inet t {
> +	chain c {
> +		type filter hook output priority 0;
> +		tcp dport != 22345 accept
> +		tcp flags syn / fin,syn,rst,ack tcp option 254  length ge 4 counter drop
> +		tcp flags syn / fin,syn,rst,ack tcp option fastopen length ge 2 reset tcp option fastopen counter
> +		tcp flags syn / fin,syn,rst,ack tcp option sack-perm missing counter drop
> +		tcp flags syn / fin,syn,rst,ack tcp option sack-perm exists counter
> +		tcp flags syn / fin,syn,rst,ack tcp option maxseg size gt 1400 counter
> +		tcp flags syn / fin,syn,rst,ack tcp option nop missing counter
> +		tcp flags syn / fin,syn,rst,ack tcp option nop exists counter
> +		tcp flags syn / fin,syn,rst,ack drop
> +	}
> +}
> +EOF
> +
> +if [ $? -ne 0 ]; then
> +	exit 1
> +fi
> +
> +if [ $have_socat != "yes" ]; then
> +	echo "Ran partial test, socat not available (skipped)"
> +	exit 77
> +fi
> +
> +# This will fail (drop in output -> connect fails with eperm)
> +socat -t 3 -u STDIN TCP:127.0.0.1:22345,connect-timeout=1 < /dev/null > /dev/null
> +
> +# Indicate success, dump file has incremented packet counter where its
> +# expected to match.
> +exit 0
> -- 
> 2.41.0
> 
> 

Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >  	if (!desc) {
> > -		if (field != TCPOPT_COMMON_KIND || kind > 255)
> > +		if (kind > 255)
> >  			return NULL;
> 
> Another suggestion: Remove this NULL, it leaves lhs as NULL in the
> relational. kind > 255 cannot ever happen, parser rejects numbers over
> 255.

We can also feed this via input from udata (typeof).
So I'd rather not assert() or rely on bison checks.

Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Pablo Neira Ayuso]

On Tue, Dec 05, 2023 at 01:20:26PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >  	if (!desc) {
> > > -		if (field != TCPOPT_COMMON_KIND || kind > 255)
> > > +		if (kind > 255)
> > >  			return NULL;
> > 
> > Another suggestion: Remove this NULL, it leaves lhs as NULL in the
> > relational. kind > 255 cannot ever happen, parser rejects numbers over
> > 255.
> 
> We can also feed this via input from udata (typeof).
> So I'd rather not assert() or rely on bison checks.

OK, but then NULL does not help either, that will crash on evaluation too.

You could narrow down kind and field in tcpopt_expr_alloc() to uint8_t.

Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Dec 05, 2023 at 01:20:26PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > >  	if (!desc) {
> > > > -		if (field != TCPOPT_COMMON_KIND || kind > 255)
> > > > +		if (kind > 255)
> > > >  			return NULL;
> > > 
> > > Another suggestion: Remove this NULL, it leaves lhs as NULL in the
> > > relational. kind > 255 cannot ever happen, parser rejects numbers over
> > > 255.
> > 
> > We can also feed this via input from udata (typeof).
> > So I'd rather not assert() or rely on bison checks.
> 
> OK, but then NULL does not help either, that will crash on evaluation too.
> 
> You could narrow down kind and field in tcpopt_expr_alloc() to uint8_t.

Unfortunately, no.  'kind' is overloaded, SACK blocks 1/2/3/4 use values
gt 255, see TCPOPT_KIND_SACK3 at end of enum tcpopt_kind.

Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Pablo Neira Ayuso]

On Tue, Dec 05, 2023 at 02:14:48PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Tue, Dec 05, 2023 at 01:20:26PM +0100, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > >  	if (!desc) {
> > > > > -		if (field != TCPOPT_COMMON_KIND || kind > 255)
> > > > > +		if (kind > 255)
> > > > >  			return NULL;
> > > > 
> > > > Another suggestion: Remove this NULL, it leaves lhs as NULL in the
> > > > relational. kind > 255 cannot ever happen, parser rejects numbers over
> > > > 255.
> > > 
> > > We can also feed this via input from udata (typeof).
> > > So I'd rather not assert() or rely on bison checks.
> > 
> > OK, but then NULL does not help either, that will crash on evaluation too.
> > 
> > You could narrow down kind and field in tcpopt_expr_alloc() to uint8_t.
> 
> Unfortunately, no.  'kind' is overloaded, SACK blocks 1/2/3/4 use values
> gt 255, see TCPOPT_KIND_SACK3 at end of enum tcpopt_kind.

OK, patch is fine then.

Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Thomas Haller]

Hi Florian,

On Tue, 2023-12-05 at 12:56 +0100, Florian Westphal wrote:
>  .../packetpath/dumps/tcp_options.nft          | 14 +++++++

is there a reason not to also generate a .json-nft file?


Thomas

Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> Hi Florian,
> 
> On Tue, 2023-12-05 at 12:56 +0100, Florian Westphal wrote:
> >  .../packetpath/dumps/tcp_options.nft          | 14 +++++++
> 
> is there a reason not to also generate a .json-nft file?

Yes, I am not adding more one-line monsters.

I'll add one once there is a solution in place that has human readable
json dumps that don't fail validation because of identical but
differently formatted output.

Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Thomas Haller]

On Wed, 2023-12-06 at 12:38 +0100, Florian Westphal wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> > Hi Florian,
> > 
> > On Tue, 2023-12-05 at 12:56 +0100, Florian Westphal wrote:
> > >  .../packetpath/dumps/tcp_options.nft          | 14 +++++++
> > 
> > is there a reason not to also generate a .json-nft file?
> 
> Yes, I am not adding more one-line monsters.
> 
> I'll add one once there is a solution in place that has human
> readable
> json dumps that don't fail validation because of identical but
> differently formatted output.
> 

What about the "[PATCH nft 0/2] pretty print .json-nft files" patches?


Thomas

Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> On Wed, 2023-12-06 at 12:38 +0100, Florian Westphal wrote:
> > Thomas Haller <thaller@redhat.com> wrote:
> > > Hi Florian,
> > > 
> > > On Tue, 2023-12-05 at 12:56 +0100, Florian Westphal wrote:
> > > >  .../packetpath/dumps/tcp_options.nft          | 14 +++++++
> > > 
> > > is there a reason not to also generate a .json-nft file?
> > 
> > Yes, I am not adding more one-line monsters.
> > 
> > I'll add one once there is a solution in place that has human
> > readable
> > json dumps that don't fail validation because of identical but
> > differently formatted output.
> > 
> 
> What about the "[PATCH nft 0/2] pretty print .json-nft files" patches?

I'm fine with that. Phil? Pablo? This is re:

https://patchwork.ozlabs.org/project/netfilter-devel/patch/20231124124759.3269219-3-thaller@redhat.com/

Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> > On Wed, 2023-12-06 at 12:38 +0100, Florian Westphal wrote:
> > > Thomas Haller <thaller@redhat.com> wrote:
> > > > Hi Florian,
> > > > 
> > > > On Tue, 2023-12-05 at 12:56 +0100, Florian Westphal wrote:
> > > > >  .../packetpath/dumps/tcp_options.nft          | 14 +++++++
> > > > 
> > > > is there a reason not to also generate a .json-nft file?
> > > 
> > > Yes, I am not adding more one-line monsters.
> > > 
> > > I'll add one once there is a solution in place that has human
> > > readable
> > > json dumps that don't fail validation because of identical but
> > > differently formatted output.
> > > 
> > 
> > What about the "[PATCH nft 0/2] pretty print .json-nft files" patches?
> 
> I'm fine with that. Phil? Pablo? This is re:
> 
> https://patchwork.ozlabs.org/project/netfilter-devel/patch/20231124124759.3269219-3-thaller@redhat.com/

What about making it so we NEVER compare json-nft at all?

Instead, feed the json-nft file to nft, then do a normal list-ruleset,
then compare that vs. normal .nft file.

This avoids any and all formatting issues and also avoids breakage when
the json-nft file is formatted differently.

Eg. postprocessing via json_pp won't match what this patch above
expects.

Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Thomas Haller]

On Wed, 2023-12-06 at 13:04 +0100, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > Thomas Haller <thaller@redhat.com> wrote:
> > > On Wed, 2023-12-06 at 12:38 +0100, Florian Westphal wrote:
> > > > Thomas Haller <thaller@redhat.com> wrote:
> > > > > Hi Florian,
> > > > > 
> > > > > On Tue, 2023-12-05 at 12:56 +0100, Florian Westphal wrote:
> > > > > >  .../packetpath/dumps/tcp_options.nft          | 14 +++++++
> > > > > 
> > > > > is there a reason not to also generate a .json-nft file?
> > > > 
> > > > Yes, I am not adding more one-line monsters.
> > > > 
> > > > I'll add one once there is a solution in place that has human
> > > > readable
> > > > json dumps that don't fail validation because of identical but
> > > > differently formatted output.
> > > > 
> > > 
> > > What about the "[PATCH nft 0/2] pretty print .json-nft files"
> > > patches?
> > 
> > I'm fine with that. Phil? Pablo? This is re:
> > 
> > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20231124124759.3269219-3-thaller@redhat.com/
> 
> What about making it so we NEVER compare json-nft at all?
> 
> Instead, feed the json-nft file to nft, then do a normal list-
> ruleset,
> then compare that vs. normal .nft file.

The .nft and .json-nft files are all fed back into `nft --check -f`. So
that is happening too.

It will also comparing the raw files (after sanitize+prettify), which
is closer to the original thing that is supposed to be tested. That is
why it's done.

> 
> This avoids any and all formatting issues and also avoids breakage
> when
> the json-nft file is formatted differently.
> 
> Eg. postprocessing via json_pp won't match what this patch above
> expects.
> 

What issues do you mean? I don't see any. Did you test/review the two
patches?


Thomas

Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> > Instead, feed the json-nft file to nft, then do a normal list-
> > ruleset,
> > then compare that vs. normal .nft file.
> 
> The .nft and .json-nft files are all fed back into `nft --check -f`. So
> that is happening too.

Not really, this checks that the parser eats the input.

> It will also comparing the raw files (after sanitize+prettify), which
> is closer to the original thing that is supposed to be tested. That is
> why it's done.

"metainfo": {
-        "json_schema_version": 1,
+        "version": "VERSION",
"release_name": "RELEASE_NAME",
-        "version": "VERSION"
+        "json_schema_version": 1
}
},

i.e. it fails validation because the on-record file has a different
layout/ordering than what is expected.

But if you feed it into nft, nft list ruleset will generate the expected
(non-json) output.

> What issues do you mean? I don't see any. Did you test/review the two
> patches?

The first one is applied.  The second one I applied locally.

But its still picky about the formatting.

Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Thomas Haller]

On Wed, 2023-12-06 at 13:16 +0100, Florian Westphal wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> > > Instead, feed the json-nft file to nft, then do a normal list-
> > > ruleset,
> > > then compare that vs. normal .nft file.
> > 
> > The .nft and .json-nft files are all fed back into `nft --check -
> > f`. So
> > that is happening too.
> 
> Not really, this checks that the parser eats the input.
> 
> > It will also comparing the raw files (after sanitize+prettify),
> > which
> > is closer to the original thing that is supposed to be tested. That
> > is
> > why it's done.
> 
> "metainfo": {
> -        "json_schema_version": 1,
> +        "version": "VERSION",
> "release_name": "RELEASE_NAME",
> -        "version": "VERSION"
> +        "json_schema_version": 1
> }
> },
> 
> i.e. it fails validation because the on-record file has a different
> layout/ordering than what is expected.

Does this mean all tests on `master` have this problem?

> 
> But if you feed it into nft, nft list ruleset will generate the
> expected
> (non-json) output.

where do you encounter that? How to reproduce this?

Is this an old libjansson? Since 2.8 (2016), JSON_PRESERVE_ORDER is
implied. Maybe libnftables needs to set JSON_PRESERVE_ORDER flag at a
few places.

> 
> > What issues do you mean? I don't see any. Did you test/review the
> > two
> > patches?
> 
> The first one is applied.  The second one I applied locally.
> 
> But its still picky about the formatting.

That's a problem...

Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> > "metainfo": {
> > -        "json_schema_version": 1,
> > +        "version": "VERSION",
> > "release_name": "RELEASE_NAME",
> > -        "version": "VERSION"
> > +        "json_schema_version": 1
> > }
> > },
> > 
> > i.e. it fails validation because the on-record file has a different
> > layout/ordering than what is expected.
> 
> Does this mean all tests on `master` have this problem?

No, those are all raw binary-like oneline dumps.

> > But if you feed it into nft, nft list ruleset will generate the
> > expected
> > (non-json) output.
> 
> where do you encounter that? How to reproduce this?
> 
> Is this an old libjansson? Since 2.8 (2016), JSON_PRESERVE_ORDER is
> implied. Maybe libnftables needs to set JSON_PRESERVE_ORDER flag at a
> few places.

Just format any existing json dump file with a different formatting
tool, e.g. json_pp.

Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Phil Sutter]

On Wed, Dec 06, 2023 at 01:04:47PM +0100, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > Thomas Haller <thaller@redhat.com> wrote:
> > > On Wed, 2023-12-06 at 12:38 +0100, Florian Westphal wrote:
> > > > Thomas Haller <thaller@redhat.com> wrote:
> > > > > Hi Florian,
> > > > > 
> > > > > On Tue, 2023-12-05 at 12:56 +0100, Florian Westphal wrote:
> > > > > >  .../packetpath/dumps/tcp_options.nft          | 14 +++++++
> > > > > 
> > > > > is there a reason not to also generate a .json-nft file?
> > > > 
> > > > Yes, I am not adding more one-line monsters.
> > > > 
> > > > I'll add one once there is a solution in place that has human
> > > > readable
> > > > json dumps that don't fail validation because of identical but
> > > > differently formatted output.
> > > > 
> > > 
> > > What about the "[PATCH nft 0/2] pretty print .json-nft files" patches?
> > 
> > I'm fine with that. Phil? Pablo? This is re:
> > 
> > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20231124124759.3269219-3-thaller@redhat.com/

What I don't like is that we'll still get these huge patches/mails if
the dumps are converted. Those that remain are still hard to handle in
case of errors.

> What about making it so we NEVER compare json-nft at all?
> 
> Instead, feed the json-nft file to nft, then do a normal list-ruleset,
> then compare that vs. normal .nft file.
> 
> This avoids any and all formatting issues and also avoids breakage when
> the json-nft file is formatted differently.

We may hide problems because nft might inadvertently sanitize the input.
Also, conversion from standard syntax to JSON may be symmetrically
broken, so standard->JSON->standard won't detect the problem.

> Eg. postprocessing via json_pp won't match what this patch above
> expects.

Python natively supports JSON. Converting stuff into comparable strings
(which also look pretty when printed) is a simple matter of:

| import json
| 
| json.dumps(json.loads(<dump as string>), \
| 	   sort_keys = True, indent = 4, \
| 	   separators = (',', ': '))

We rely upon Python for the testsuite already, so I don't see why
there's all the fuss. JSON dump create, load and compare have not been a
problem in the 5 years tests/py does it.

Cheers, Phil

Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Thomas Haller]

On Wed, 2023-12-06 at 14:24 +0100, Florian Westphal wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> > > "metainfo": {
> > > -        "json_schema_version": 1,
> > > +        "version": "VERSION",
> > > "release_name": "RELEASE_NAME",
> > > -        "version": "VERSION"
> > > +        "json_schema_version": 1
> > > }
> > > },
> > > 
> > > i.e. it fails validation because the on-record file has a
> > > different
> > > layout/ordering than what is expected.
> > 
> > Does this mean all tests on `master` have this problem?
> 
> No, those are all raw binary-like oneline dumps.

sorry, I don't follow.

The patch takes the "raw binary-like oneline dump", and prettifies it.
It does so via `json-pretty.sh` script. That script, does of course not
mess up the order of dictionary keys.


> 
> > > But if you feed it into nft, nft list ruleset will generate the
> > > expected
> > > (non-json) output.
> > 
> > where do you encounter that? How to reproduce this?
> > 
> > Is this an old libjansson? Since 2.8 (2016), JSON_PRESERVE_ORDER is
> > implied. Maybe libnftables needs to set JSON_PRESERVE_ORDER flag at
> > a
> > few places.
> 
> Just format any existing json dump file with a different formatting
> tool, e.g. json_pp.

The patch has a way for prettifying JSON (json-pretty.sh). Sure, it
could also use json_pp instead, but what problem would that solve?


Thomas

Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

[Thomas Haller]

On Wed, 2023-12-06 at 15:19 +0100, Phil Sutter wrote:
> On Wed, Dec 06, 2023 at 01:04:47PM +0100, Florian Westphal wrote:
> > Florian Westphal <fw@strlen.de> wrote:
> > > Thomas Haller <thaller@redhat.com> wrote:
> > > > On Wed, 2023-12-06 at 12:38 +0100, Florian Westphal wrote:
> > > > > Thomas Haller <thaller@redhat.com> wrote:
> > > > > > Hi Florian,
> > > > > > 
> > > > > > On Tue, 2023-12-05 at 12:56 +0100, Florian Westphal wrote:
> > > > > > >  .../packetpath/dumps/tcp_options.nft          | 14
> > > > > > > +++++++
> > > > > > 
> > > > > > is there a reason not to also generate a .json-nft file?
> > > > > 
> > > > > Yes, I am not adding more one-line monsters.
> > > > > 
> > > > > I'll add one once there is a solution in place that has human
> > > > > readable
> > > > > json dumps that don't fail validation because of identical
> > > > > but
> > > > > differently formatted output.
> > > > > 
> > > > 
> > > > What about the "[PATCH nft 0/2] pretty print .json-nft files"
> > > > patches?
> > > 
> > > I'm fine with that. Phil? Pablo? This is re:
> > > 
> > > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20231124124759.3269219-3-thaller@redhat.com/
> 
> What I don't like is that we'll still get these huge patches/mails if
> the dumps are converted. Those that remain are still hard to handle
> in
> case of errors.

We can of course do a one-time commit to convert all .json-nft to
multi-line. The patch makes an effort to not require re-generating the
existing files unless necessary.

The only question is which one is preferable.

Regenerating all .json-nft files once, also makes the second patch
slightly simpler (but not so much, that it would be an argument for
doing that).

> 
> > What about making it so we NEVER compare json-nft at all?
> > 
> > Instead, feed the json-nft file to nft, then do a normal list-
> > ruleset,
> > then compare that vs. normal .nft file.
> > 
> > This avoids any and all formatting issues and also avoids breakage
> > when
> > the json-nft file is formatted differently.
> 
> We may hide problems because nft might inadvertently sanitize the
> input.
> Also, conversion from standard syntax to JSON may be symmetrically
> broken, so standard->JSON->standard won't detect the problem.
> 
> > Eg. postprocessing via json_pp won't match what this patch above
> > expects.
> 
> Python natively supports JSON. Converting stuff into comparable
> strings
> (which also look pretty when printed) is a simple matter of:
> 
> > import json
> > 
> > json.dumps(json.loads(<dump as string>), \
> > 	   sort_keys = True, indent = 4, \
> > 	   separators = (',', ': '))
> 
> We rely upon Python for the testsuite already, so I don't see why
> there's all the fuss. JSON dump create, load and compare have not
> been a
> problem in the 5 years tests/py does it.

The current patches intentionally don't try to sort keys. I claim, it's
also not necessary to sort them. I can easily be convinced otherwise,
by showing a counter-example/reproducer.

There is no fuzz, aside you and Florian bringing this topic up.


Thomas