[PATCH 1/3] scanner: add files in include dirs in alphabetical order.

[PATCH 1/3] scanner: add files in include dirs in alphabetical order.

[Ismo Puustinen]

This means that if you have a directory structure like this

    /foo
    /foo/02_rules.nft
    /foo/01_rules.nft

where *.nft files in directory /foo are nft scripts, then an include
statement in another nft script like this

    include "/foo/"

guarantees that "01_rules.nft" is loaded before "02_rules.nft".

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>
---
 src/scanner.l | 102 ++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 75 insertions(+), 27 deletions(-)

diff --git a/src/scanner.l b/src/scanner.l
index fe65e0c..4050cba 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -13,10 +13,12 @@
 #include <dirent.h>
 #include <libgen.h>
 #include <limits.h>
+#include <unistd.h>
 #include <netinet/in.h>
 #include <arpa/inet.h>
 #include <linux/types.h>
 #include <linux/netfilter.h>
+#include <sys/stat.h>
 
 #include <nftables.h>
 #include <erec.h>
@@ -668,14 +670,39 @@ int scanner_read_file(void *scanner, const char *filename,
 	return include_file(scanner, filename, loc);
 }
 
+static int directoryfilter(const struct dirent *de)
+{
+	if (strcmp(de->d_name, ".") == 0 ||
+			strcmp(de->d_name, "..") == 0)
+		return 0;
+
+	/* Accept other filenames. If we want to enable filtering based on
+	 * filename suffix (*.nft), this would be the place to do it.
+	 */
+
+	return 1;
+}
+
+static void free_scandir_des(struct dirent **des, int n_des)
+{
+	int i;
+
+	for (i = 0; i < n_des; i++) {
+		free(des[i]);
+	}
+
+	free(des);
+}
+
 static int include_directory(void *scanner, const char *dirname,
-			     DIR *directory, const struct location *loc)
+			     const struct location *loc)
 {
 	struct parser_state *state = yyget_extra(scanner);
+	struct dirent **des = NULL;
 	struct error_record *erec;
-	struct dirent *de;
+	int ret, n_des = 0, i;
+	char dirbuf[PATH_MAX];
 	FILE *f;
-	int ret;
 
 	if (!dirname[0] || dirname[strlen(dirname)-1] != '/') {
 		erec = error(loc, "Include directory name \"%s\" does not end in '/'",
@@ -684,23 +711,32 @@ static int include_directory(void *scanner, const char *dirname,
 	}
 
 	/* If the path is a directory, assume that all files there need
-	 * to be included.
+	 * to be included. Sort the file list in alphabetical order.
 	 */
-	while ((de = readdir(directory))) {
-		char dirbuf[PATH_MAX];
+	n_des = scandir(dirname, &des, directoryfilter, alphasort);
+	if (n_des < 0) {
+		erec = error(loc, "Failed to scan directory contents for \"%s\"",
+			     dirname);
+		goto err;
+	}
+	else if (n_des == 0) {
+		/* nothing to do */
+		free(des);
+		return 0;
+	}
 
+	/* We need to push the files in reverse order, so that they will be
+	 * popped in the correct order.
+	 */
+	for (i = n_des-1; i >= 0; i--) {
 		ret = snprintf(dirbuf, sizeof(dirbuf), "%s/%s", dirname,
-			       de->d_name);
+			       des[i]->d_name);
 		if (ret < 0 || ret >= PATH_MAX) {
 			erec = error(loc, "Too long file path \"%s/%s\"\n",
-				     dirname, de->d_name);
+				     dirname, des[i]->d_name);
 			goto err;
 		}
 
-		if (strcmp(de->d_name, ".") == 0 ||
-		    strcmp(de->d_name, "..") == 0)
-			continue;
-
 		f = fopen(dirbuf, "r");
 		if (f == NULL) {
 			erec = error(loc, "Could not open file \"%s\": %s\n",
@@ -708,12 +744,14 @@ static int include_directory(void *scanner, const char *dirname,
 			goto err;
 		}
 
-		erec = scanner_push_file(scanner, de->d_name, f, loc);
+		erec = scanner_push_file(scanner, des[i]->d_name, f, loc);
 		if (erec != NULL)
 			goto err;
 	}
+	free_scandir_des(des, n_des);
 	return 0;
 err:
+	free_scandir_des(des, n_des);
 	erec_queue(erec, state->msgs);
 	return -1;
 }
@@ -721,27 +759,37 @@ err:
 static int include_dentry(void *scanner, const char *filename,
 			  const struct location *loc)
 {
-	DIR *directory;
+	struct parser_state *state = yyget_extra(scanner);
+	struct error_record *erec;
+	struct stat st;
 	int ret;
 
-	/* The file can be either a simple file or a directory which
-	 * contains files.
-	 */
-	directory = opendir(filename);
-	if (directory == NULL &&
-	    errno != ENOTDIR) {
-		/* Could not access the directory or file, keep on searching.
+	/* stat the file */
+	ret = stat(filename, &st);
+
+	if (ret == -1 && errno == ENOENT)
+		/* Could not find the directory or file, keep on searching.
 		 * Return value '1' indicates to the caller that we should still
 		 * search in the next include directory.
 		 */
-		ret = 1;
-	} else if (directory != NULL) {
-		ret = include_directory(scanner, filename, directory, loc);
-		closedir(directory);
-	} else {
-		ret = include_file(scanner, filename, loc);
+		return 1;
+	else if (ret == 0) {
+		if (S_ISDIR(st.st_mode))
+			return include_directory(scanner, filename, loc);
+		else if (S_ISREG(st.st_mode))
+			return include_file(scanner, filename, loc);
+		else {
+			errno = EINVAL;
+			ret = -1;
+		}
 	}
 
+	/* Process error for failed stat and cases where the file is not a
+	 * directory or (a link to) a regular file.
+	 */
+	erec = error(loc, "Failed to access file \"%s\": %s\n",
+			filename, strerror(errno));
+	erec_queue(erec, state->msgs);
 	return ret;
 }
 
-- 
2.9.4

[PATCH 2/3] man: add include directory documentation.

[Ismo Puustinen]

Short include directory introduction is added to the man page.

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>
---
 doc/nft.xml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/doc/nft.xml b/doc/nft.xml
index f613f69..9364133 100644
--- a/doc/nft.xml
+++ b/doc/nft.xml
@@ -209,6 +209,10 @@ vi:ts=4 sw=4
 				The directories to be searched for include files can be specified using
 				the <option>-I/--includepath</option> option.
 			</para>
+			<para>
+				If the <literal>filename</literal> parameter is a directory, then all files in
+				the directory are loaded in alphabetical order.
+			</para>
 		</refsect2>
 		<refsect2>
 			<title>Symbolic variables</title>
-- 
2.9.4

[PATCH 3/3] tests: added tests for ordering files in include dirs.

[Ismo Puustinen]

Test that the files are ordered properly by introducing included files
which have internal dependencies.

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>
---
 tests/shell/testcases/include/0011dir_dependency_0 | 48 ++++++++++++++++++++
 tests/shell/testcases/include/0012dir_dependency_1 | 51 ++++++++++++++++++++++
 2 files changed, 99 insertions(+)
 create mode 100755 tests/shell/testcases/include/0011dir_dependency_0
 create mode 100755 tests/shell/testcases/include/0012dir_dependency_1

diff --git a/tests/shell/testcases/include/0011dir_dependency_0 b/tests/shell/testcases/include/0011dir_dependency_0
new file mode 100755
index 0000000..8ee193f
--- /dev/null
+++ b/tests/shell/testcases/include/0011dir_dependency_0
@@ -0,0 +1,48 @@
+#!/bin/bash
+
+set -e
+
+tmpdir=$(mktemp -d)
+if [ ! -d $tmpdir ] ; then
+        echo "Failed to create tmp directory" >&2
+        exit 0
+fi
+
+tmpfile1="$tmpdir/01_file.nft"
+touch $tmpfile1
+if [ ! -w $tmpfile1 ] ; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+tmpfile2="$tmpdir/02_file.nft"
+touch $tmpfile2
+if [ ! -w $tmpfile2 ] ; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+tmpfile3=$(mktemp)
+if [ ! -w $tmpfile3 ] ; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+# cleanup if aborted
+trap "rm -rf $tmpfile1 $tmpfile2 $tmpfile3 && rmdir $tmpdir" EXIT
+
+# add interdependent rulesets
+RULESET1="add table x"
+RULESET2="add chain x y"
+RULESET3="include \"$tmpdir/\""
+
+echo "$RULESET1" > $tmpfile1
+echo "$RULESET2" > $tmpfile2
+echo "$RULESET3" > $tmpfile3
+
+$NFT -f $tmpfile3
+
+if [ $? -ne 0 ] ; then
+        echo "E: unable to load good ruleset" >&2
+        exit 1
+fi
diff --git a/tests/shell/testcases/include/0012dir_dependency_1 b/tests/shell/testcases/include/0012dir_dependency_1
new file mode 100755
index 0000000..bd27e2a
--- /dev/null
+++ b/tests/shell/testcases/include/0012dir_dependency_1
@@ -0,0 +1,51 @@
+#!/bin/bash
+
+set -e
+
+tmpdir=$(mktemp -d)
+if [ ! -d $tmpdir ] ; then
+        echo "Failed to create tmp directory" >&2
+        exit 0
+fi
+
+tmpfile1="$tmpdir/01_file.nft"
+touch $tmpfile1
+if [ ! -w $tmpfile1 ] ; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+tmpfile2="$tmpdir/02_file.nft"
+touch $tmpfile2
+if [ ! -w $tmpfile2 ] ; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+tmpfile3=$(mktemp)
+if [ ! -w $tmpfile3 ] ; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+# cleanup if aborted
+trap "rm -rf $tmpfile1 $tmpfile2 $tmpfile3 && rmdir $tmpdir" EXIT
+
+# add interdependent rulesets
+RULESET1="add table x"
+RULESET2="add chain x y"
+RULESET3="include \"$tmpdir/\""
+
+# Note different order when compared with 0011dir_depencency_0. The idea
+# here is to introduce wrong order to get the loading fail.
+echo "$RULESET1" > $tmpfile2
+echo "$RULESET2" > $tmpfile1
+echo "$RULESET3" > $tmpfile3
+
+$NFT -f $tmpfile3
+
+if [ $? -eq 0 ] ; then
+        echo "E: did not catch wrong file order in include directory" >&2
+        exit 1
+fi
+
-- 
2.9.4

Re: [PATCH 1/3] scanner: add files in include dirs in alphabetical order.

[Pablo Neira Ayuso]

On Wed, Jun 07, 2017 at 11:35:57AM +0300, Ismo Puustinen wrote:
> This means that if you have a directory structure like this
> 
>     /foo
>     /foo/02_rules.nft
>     /foo/01_rules.nft
> 
> where *.nft files in directory /foo are nft scripts, then an include
> statement in another nft script like this
> 
>     include "/foo/"
> 
> guarantees that "01_rules.nft" is loaded before "02_rules.nft".

OK, that was fast.

Applied with comestic coding style changes, thanks Ismo!

Do not hesitate to follow up in case of any fallout from this update.

Re: [PATCH 2/3] man: add include directory documentation.

[Pablo Neira Ayuso]

On Wed, Jun 07, 2017 at 11:35:58AM +0300, Ismo Puustinen wrote:
> Short include directory introduction is added to the man page.

Also applied.

Re: [PATCH 1/3] scanner: add files in include dirs in alphabetical order.

[Arturo Borrero Gonzalez]

On 7 June 2017 at 10:35, Ismo Puustinen <ismo.puustinen@intel.com> wrote:
>
> +static int directoryfilter(const struct dirent *de)
> +{
> +       if (strcmp(de->d_name, ".") == 0 ||
> +                       strcmp(de->d_name, "..") == 0)
> +               return 0;
> +
> +       /* Accept other filenames. If we want to enable filtering based on
> +        * filename suffix (*.nft), this would be the place to do it.
> +        */
> +

This filter by suffix is good to have IMHO.
I guess that forcing users to explicitly create a file for nftables
(or at least give a specific suffix) reduces chances for user errors.

Re: [PATCH 1/3] scanner: add files in include dirs in alphabetical order.

[Pablo Neira Ayuso]

On Wed, Jun 07, 2017 at 09:40:53PM +0200, Arturo Borrero Gonzalez wrote:
> On 7 June 2017 at 10:35, Ismo Puustinen <ismo.puustinen@intel.com> wrote:
> >
> > +static int directoryfilter(const struct dirent *de)
> > +{
> > +       if (strcmp(de->d_name, ".") == 0 ||
> > +                       strcmp(de->d_name, "..") == 0)
> > +               return 0;
> > +
> > +       /* Accept other filenames. If we want to enable filtering based on
> > +        * filename suffix (*.nft), this would be the place to do it.
> > +        */
> > +
> 
> This filter by suffix is good to have IMHO.
> I guess that forcing users to explicitly create a file for nftables
> (or at least give a specific suffix) reduces chances for user errors.

You mean, this new include directory feature just takes *.nft files,
right?

Then, to keep it consistent, we should also display a warning in
include file with no .nft postfix. At deprecate the existing behaviour
at some point, ie. bail out if you include a file that has no trailing
.nft in its name.

If we follow this path, all ruleset file will end up using .nft as
a trailer in the name.

Is there any other similar software following this approach? How is
'ferm' doing this?

Re: [PATCH 1/3] scanner: add files in include dirs in alphabetical order.

[Arturo Borrero Gonzalez]

On 8 June 2017 at 12:17, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Jun 07, 2017 at 09:40:53PM +0200, Arturo Borrero Gonzalez wrote:
>> On 7 June 2017 at 10:35, Ismo Puustinen <ismo.puustinen@intel.com> wrote:
>> >
>> > +static int directoryfilter(const struct dirent *de)
>> > +{
>> > +       if (strcmp(de->d_name, ".") == 0 ||
>> > +                       strcmp(de->d_name, "..") == 0)
>> > +               return 0;
>> > +
>> > +       /* Accept other filenames. If we want to enable filtering based on
>> > +        * filename suffix (*.nft), this would be the place to do it.
>> > +        */
>> > +
>>
>> This filter by suffix is good to have IMHO.
>> I guess that forcing users to explicitly create a file for nftables
>> (or at least give a specific suffix) reduces chances for user errors.
>
> You mean, this new include directory feature just takes *.nft files,
> right?
>

Yes,

> Then, to keep it consistent, we should also display a warning in
> include file with no .nft postfix. At deprecate the existing behaviour
> at some point, ie. bail out if you include a file that has no trailing
> .nft in its name.
>
> If we follow this path, all ruleset file will end up using .nft as
> a trailer in the name.
>

but perhaps it makes sense to differentiate two cases:
 * include a single file: accept arbitrary names
 * include a whole dir: accept only files ending in .nft

This seems to be what sysctl(8) does when loading a single file vs a directory.
I'm thinking in a case where you have a README in the directory or
other unrelated file.

If the idea is to allow drop files (a good idea indeed), then being
explicit is a good approach.

> Is there any other similar software following this approach? How is
> 'ferm' doing this?

ferm seems to load arbitrary files. In the docs they suggest using
.ferm files but the code
seems to allow whatever.
However, they have a set of regexp hardcoded to avoid loading things
like backups file an the like.
So, yes, probably forcing to .nft is sensible.

Re: [PATCH 1/3] scanner: add files in include dirs in alphabetical order.

[Pablo Neira Ayuso]

On Thu, Jun 08, 2017 at 06:37:57PM +0200, Arturo Borrero Gonzalez wrote:
> On 8 June 2017 at 12:17, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
[...]
> > Then, to keep it consistent, we should also display a warning in
> > include file with no .nft postfix. At deprecate the existing behaviour
> > at some point, ie. bail out if you include a file that has no trailing
> > .nft in its name.
> >
> > If we follow this path, all ruleset file will end up using .nft as
> > a trailer in the name.
> >
> 
> but perhaps it makes sense to differentiate two cases:
>  * include a single file: accept arbitrary names
>  * include a whole dir: accept only files ending in .nft
> 
> This seems to be what sysctl(8) does when loading a single file vs a directory.
> I'm thinking in a case where you have a README in the directory or
> other unrelated file.

I see, it makes sense indeed to have a way to skip files you don't
want. But I still would like this behaviour is consistent.

> If the idea is to allow drop files (a good idea indeed), then being
> explicit is a good approach.
> 
> > Is there any other similar software following this approach? How is
> > 'ferm' doing this?
> 
> ferm seems to load arbitrary files. In the docs they suggest using
> .ferm files but the code
> seems to allow whatever.
> However, they have a set of regexp hardcoded to avoid loading things
> like backups file an the like.
> So, yes, probably forcing to .nft is sensible.

I would go for glob support, ie.

include ./nft-ruleset-files/*.nft

so you can explicit indicate what file pattern you want to load.