[PATCH nft,RFC] main: remove need to escape quotes

[PATCH nft,RFC] main: remove need to escape quotes

[Pablo Neira Ayuso]

If argv[i] contains spaces, then restore the quotes on this string.

There is one exception though: in case that argc == 2, then assume the
whole input is coming as a quoted string, eg. nft "add rule x ...;add ..."

This patch is adjusting a one test that uses quotes to skip escaping one
semicolon from bash. Two more tests do not need them.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
Currently nft accepts quotes everywhere, which makes things a bit tricky.
I think this provides a model that can be documented and that skips quote
escaping from bash.

 src/main.c                                         | 30 +++++++++++++++++++---
 tests/shell/testcases/flowtable/0007prio_0         |  2 +-
 tests/shell/testcases/sets/0034get_element_0       |  2 +-
 .../testcases/sets/0040get_host_endian_elements_0  | 12 ++++-----
 4 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/src/main.c b/src/main.c
index 74199f93fa66..00ef999eaf4b 100644
--- a/src/main.c
+++ b/src/main.c
@@ -10,6 +10,7 @@
 
 #include <stdlib.h>
 #include <stddef.h>
+#include <ctype.h>
 #include <unistd.h>
 #include <stdio.h>
 #include <errno.h>
@@ -243,10 +244,21 @@ static bool nft_options_check(int argc, char * const argv[])
 	return true;
 }
 
+static bool nft_quoted_string(const char *arg)
+{
+	unsigned int i;
+
+	for (i = 0; i < strlen(arg); i++) {
+		if (isspace(arg[i]))
+			return true;
+	}
+	return false;
+}
+
 int main(int argc, char * const *argv)
 {
+	unsigned int output_flags = 0, quotes;
 	char *buf = NULL, *filename = NULL;
-	unsigned int output_flags = 0;
 	bool interactive = false;
 	unsigned int debug_mask;
 	unsigned int len;
@@ -365,8 +377,14 @@ int main(int argc, char * const *argv)
 	nft_ctx_output_set_flags(nft, output_flags);
 
 	if (optind != argc) {
-		for (len = 0, i = optind; i < argc; i++)
-			len += strlen(argv[i]) + strlen(" ");
+		for (len = 0, i = optind; i < argc; i++) {
+			if (argc != 2 && nft_quoted_string(argv[i]))
+				quotes = 2;
+			else
+				quotes = 0;
+
+			len += strlen(argv[i]) + strlen(" ") + quotes;
+		}
 
 		buf = calloc(1, len);
 		if (buf == NULL) {
@@ -375,7 +393,11 @@ int main(int argc, char * const *argv)
 			exit(EXIT_FAILURE);
 		}
 		for (i = optind; i < argc; i++) {
-			strcat(buf, argv[i]);
+			if (argc != 2 && nft_quoted_string(argv[i]))
+				sprintf(buf + strlen(buf), "\"%s\"", argv[i]);
+			else
+				strcat(buf, argv[i]);
+
 			if (i + 1 < argc)
 				strcat(buf, " ");
 		}
diff --git a/tests/shell/testcases/flowtable/0007prio_0 b/tests/shell/testcases/flowtable/0007prio_0
index 49bbcac7c93b..138fe4d58788 100755
--- a/tests/shell/testcases/flowtable/0007prio_0
+++ b/tests/shell/testcases/flowtable/0007prio_0
@@ -18,7 +18,7 @@ format_offset () {
 $NFT add table t
 for offset in -11 -10 0 10 11
 do
-	$NFT add flowtable t f "{ hook ingress priority filter `format_offset $offset`; devices = { lo }; }"
+	$NFT add flowtable t f { hook ingress priority filter `format_offset $offset`\; devices = { lo }\; }
 	$NFT delete flowtable t f
 done
 
diff --git a/tests/shell/testcases/sets/0034get_element_0 b/tests/shell/testcases/sets/0034get_element_0
index c7e7298a4aac..47f93464b687 100755
--- a/tests/shell/testcases/sets/0034get_element_0
+++ b/tests/shell/testcases/sets/0034get_element_0
@@ -3,7 +3,7 @@
 RC=0
 
 check() { # (elems, expected)
-	out=$($NFT get element ip t s "{ $1 }" 2>/dev/null)
+	out=$($NFT get element ip t s { $1 } 2>/dev/null)
 	out=$(grep "elements =" <<< "$out")
 	out="${out#* \{ }"
 	out="${out% \}}"
diff --git a/tests/shell/testcases/sets/0040get_host_endian_elements_0 b/tests/shell/testcases/sets/0040get_host_endian_elements_0
index caf6a4af326a..889d28780be7 100755
--- a/tests/shell/testcases/sets/0040get_host_endian_elements_0
+++ b/tests/shell/testcases/sets/0040get_host_endian_elements_0
@@ -12,32 +12,32 @@ RULESET="table ip t {
 
 $NFT -f - <<< "$RULESET" || { echo "can't apply basic ruleset"; exit 1; }
 
-$NFT get element ip t s '{ 0x23-0x42 }' || {
+$NFT get element ip t s { 0x23-0x42 } || {
 	echo "can't find existing range 0x23-0x42"
 	exit 1
 }
 
-$NFT get element ip t s '{ 0x26-0x28 }' || {
+$NFT get element ip t s { 0x26-0x28 } || {
 	echo "can't find existing sub-range 0x26-0x28"
 	exit 1
 }
 
-$NFT get element ip t s '{ 0x26-0x99 }' && {
+$NFT get element ip t s { 0x26-0x99 } && {
 	echo "found non-existing range 0x26-0x99"
 	exit 1
 }
 
-$NFT get element ip t s '{ 0x55-0x99 }' && {
+$NFT get element ip t s { 0x55-0x99 } && {
 	echo "found non-existing range 0x55-0x99"
 	exit 1
 }
 
-$NFT get element ip t s '{ 0x55 }' && {
+$NFT get element ip t s { 0x55 } && {
 	echo "found non-existing element 0x55"
 	exit 1
 }
 
-$NFT get element ip t s '{ 0x1337 }' || {
+$NFT get element ip t s { 0x1337 } || {
 	echo "can't find existing element 0x1337"
 	exit 1
 }
-- 
2.11.0

Re: [PATCH nft,RFC] main: remove need to escape quotes

[Phil Sutter]

Hi Pablo,

On Mon, Dec 16, 2019 at 10:41:57PM +0100, Pablo Neira Ayuso wrote:
> If argv[i] contains spaces, then restore the quotes on this string.
> 
> There is one exception though: in case that argc == 2, then assume the
> whole input is coming as a quoted string, eg. nft "add rule x ...;add ..."
> 
> This patch is adjusting a one test that uses quotes to skip escaping one
> semicolon from bash. Two more tests do not need them.

I appreciate your efforts at making my BUGS note obsolete. :)
In this case though, I wonder if this really fixes something: I use
quotes in only two cases:

A) When forced by the parser, e.g. with interface names.
B) To escape the curly braces (and any semi-colons inside) in chain or
   set definitions.

Unless I miss something, case (A) will still need escaped quotes since
interface names usually don't contain whitespace. In case (B), your
patch would typically bite me as I merely quote the braces, like so:

| # nft add chain inet t c '{ type filter hook input priority filter; policy drop; }'

Of course that's a matter of muscle memory, but IIUC, your fix won't
work if one wants to pass flags in addition to a quoted command. Or does
getopt mangle argc?

Cheers, Phil

Re: [PATCH nft,RFC] main: remove need to escape quotes

[Pablo Neira Ayuso]

On Tue, Dec 17, 2019 at 01:42:57AM +0100, Phil Sutter wrote:
> Hi Pablo,
> 
> On Mon, Dec 16, 2019 at 10:41:57PM +0100, Pablo Neira Ayuso wrote:
> > If argv[i] contains spaces, then restore the quotes on this string.
> > 
> > There is one exception though: in case that argc == 2, then assume the
> > whole input is coming as a quoted string, eg. nft "add rule x ...;add ..."
> > 
> > This patch is adjusting a one test that uses quotes to skip escaping one
> > semicolon from bash. Two more tests do not need them.
> 
> I appreciate your efforts at making my BUGS note obsolete. :)
> In this case though, I wonder if this really fixes something:

nft add rule x y log prefix "test: "

instead of

nft add rule x y log prefix \"test: \"

> I use quotes in only two cases:
> 
> A) When forced by the parser, e.g. with interface names.

Interface names have no spaces, so this patch fixes nothing there indeed.

> B) To escape the curly braces (and any semi-colons inside) in chain or
>    set definitions.
> 
> Unless I miss something, case (A) will still need escaped quotes since
> interface names usually don't contain whitespace. In case (B), your
> patch would typically bite me as I merely quote the braces, like so:
> 
> | # nft add chain inet t c '{ type filter hook input priority filter; policy drop; }'

You do this trick not to escape three times, ie.

| #nft add chain inet t c \{ type filter hook input priority filter; policy drop\; \}

Your trick works fine right now because the argv list is not honored
by the main function, your quotes to avoid escaping the values will
result in:

argv[0] = nft
argv[1] = add
argv[2] = chain
argv[3] = inet
argv[4] = t
argv[5] = c
argv[6] = { type filter hook input priority filter; policy drop; }

This is not a problem because main translates this into a plain buffer
to feed the bison parser for the command line mode.

With my patch, this will still work:

| # nft 'add chain inet t c { type filter hook input priority filter; policy drop; }'

So you can still use quotes to avoid escaping, but quotes are
restricted to the whole command OR to use them to really quote a
string.

So I'm debating if it's worth providing a simple and consistent model
we can document on how to use quotes in nft from the command line, in
this patch:

1) You can quote the whole command to avoid escape characters that
   have special semantics in your shell, eg. { and ; in zsh. Or ; in bash.

2) You do not need to escape quotes anymore as in the example above
   for log prefix.

Otherwise, we are allowing for quotes basically anywhere.

If in the future, we decide to stop using bison for whatever reason
and we rely on argc and argv, this might make things harder for a new
parser. Not telling I have an incentive to replace the parser right
now though.

> Of course that's a matter of muscle memory, but IIUC, your fix won't
> work if one wants to pass flags in addition to a quoted command. Or does
> getopt mangle argc?

argc is left untouch, it would need to pass it as a pointer to
getopt_long() to update it. Not related, but getopt mangles argv,
because it reorders options, it is placing them right before the
non-options, so optind points to the beginning of what main passes to
the bison parser. Well actually mangling will not happen anymore if
the patch to enforce options before command is applied (looks like
feedback on the mailing list points to that direction).

Probably not worth the effort and we should start promoting people to
use the interactive interface for `nft -i'. If autocompletion is
supported there, then there would be a real incentive for users to
pick `nft -i'.

Thanks.

Re: [PATCH nft,RFC] main: remove need to escape quotes

[Phil Sutter]

Hi,

On Tue, Dec 17, 2019 at 11:42:51AM +0100, Pablo Neira Ayuso wrote:
> On Tue, Dec 17, 2019 at 01:42:57AM +0100, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Mon, Dec 16, 2019 at 10:41:57PM +0100, Pablo Neira Ayuso wrote:
> > > If argv[i] contains spaces, then restore the quotes on this string.
> > > 
> > > There is one exception though: in case that argc == 2, then assume the
> > > whole input is coming as a quoted string, eg. nft "add rule x ...;add ..."
> > > 
> > > This patch is adjusting a one test that uses quotes to skip escaping one
> > > semicolon from bash. Two more tests do not need them.
> > 
> > I appreciate your efforts at making my BUGS note obsolete. :)
> > In this case though, I wonder if this really fixes something:
> 
> nft add rule x y log prefix "test: "
> 
> instead of
> 
> nft add rule x y log prefix \"test: \"
> 
> > I use quotes in only two cases:
> > 
> > A) When forced by the parser, e.g. with interface names.
> 
> Interface names have no spaces, so this patch fixes nothing there indeed.

Ah, sorry - I mixed that up. There are spots though where quotes are
mandatory, namely in include statement and for ct helper types. The
latter is relevant on cmdline and quotes not being restored there is at
least inconsistent.

Also, we had a longer discussion at NFWS about enforcing quotes for
strings on input (or at least quoting them all on output) to fix for
cases where a recognized keyword was chosen for a name by accident.

> > B) To escape the curly braces (and any semi-colons inside) in chain or
> >    set definitions.
> > 
> > Unless I miss something, case (A) will still need escaped quotes since
> > interface names usually don't contain whitespace. In case (B), your
> > patch would typically bite me as I merely quote the braces, like so:
> > 
> > | # nft add chain inet t c '{ type filter hook input priority filter; policy drop; }'
> 
> You do this trick not to escape three times, ie.
> 
> | #nft add chain inet t c \{ type filter hook input priority filter; policy drop\; \}
> 
> Your trick works fine right now because the argv list is not honored
> by the main function, your quotes to avoid escaping the values will
> result in:
> 
> argv[0] = nft
> argv[1] = add
> argv[2] = chain
> argv[3] = inet
> argv[4] = t
> argv[5] = c
> argv[6] = { type filter hook input priority filter; policy drop; }
> 
> This is not a problem because main translates this into a plain buffer
> to feed the bison parser for the command line mode.
> 
> With my patch, this will still work:
> 
> | # nft 'add chain inet t c { type filter hook input priority filter; policy drop; }'
> 
> So you can still use quotes to avoid escaping, but quotes are
> restricted to the whole command OR to use them to really quote a
> string.
> 
> So I'm debating if it's worth providing a simple and consistent model
> we can document on how to use quotes in nft from the command line, in
> this patch:
> 
> 1) You can quote the whole command to avoid escape characters that
>    have special semantics in your shell, eg. { and ; in zsh. Or ; in bash.
> 
> 2) You do not need to escape quotes anymore as in the example above
>    for log prefix.
> 
> Otherwise, we are allowing for quotes basically anywhere.
> 
> If in the future, we decide to stop using bison for whatever reason
> and we rely on argc and argv, this might make things harder for a new
> parser. Not telling I have an incentive to replace the parser right
> now though.
> 
> > Of course that's a matter of muscle memory, but IIUC, your fix won't
> > work if one wants to pass flags in addition to a quoted command. Or does
> > getopt mangle argc?
> 
> argc is left untouch, it would need to pass it as a pointer to
> getopt_long() to update it. Not related, but getopt mangles argv,
> because it reorders options, it is placing them right before the
> non-options, so optind points to the beginning of what main passes to
> the bison parser. Well actually mangling will not happen anymore if
> the patch to enforce options before command is applied (looks like
> feedback on the mailing list points to that direction).

So this means that neither:

| # nft -a 'add rule t c accept'

nor:

| # nft '-a add rule t c accept'

will work, right?

> Probably not worth the effort and we should start promoting people to
> use the interactive interface for `nft -i'. If autocompletion is
> supported there, then there would be a real incentive for users to
> pick `nft -i'.

ACK.

Cheers, Phil