[PATCH v2 1/2] scanner: add support for include directories

[PATCH v2 1/2] scanner: add support for include directories

[Ismo Puustinen]

If a string after "include" keyword points to a directory instead of a
file, consider the directory to contain only nft rule files and try to
load them all. This helps with a use case where services drop their own
firewall configuration files into a directory and nft needs to include
those without knowing the exact file names.

File loading order from the include directory is not specified, so the
files inside an include directory should not depend on each other.

Fixes(Bug 1154 - Allow include statement to operate on directories and/or wildcards).

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

diff --git a/src/scanner.l b/src/scanner.l
index c2c008d..19e12bc 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -10,6 +10,8 @@
 
 %{
 
+#include <dirent.h>
+#include <libgen.h>
 #include <limits.h>
 #include <netinet/in.h>
 #include <arpa/inet.h>
@@ -661,6 +663,82 @@ err:
 	return -1;
 }
 
+static int find_and_add_include_file(void *scanner, const char *filename,
+			 const struct location *loc)
+{
+	DIR *directory = NULL;
+	FILE *f = NULL;
+	struct error_record *erec;
+	struct parser_state *state = yyget_extra(scanner);
+
+	/* The file can be either a simple file or a directory which
+	 * contains files. If it is a directory, assume that all files there
+	 * should be included. */
+
+	directory = opendir(filename);
+
+	if (directory == NULL && errno != ENOTDIR)
+		/* Could not access the directory or file, keep on searching. */
+		return 1;
+	else if (directory != NULL) {
+		/* A directory. */
+		struct dirent *de;
+
+		if (!filename[0] || filename[strlen(filename)-1] != '/') {
+			erec = error(loc, "Include directory name \"%s\" does not end in '/'",
+					filename);
+			goto err;
+		}
+
+		/* If the path is a directory, assume that all files there need
+		 * to be included. */
+		while ((de = readdir(directory))) {
+			char dirbuf[PATH_MAX];
+
+			snprintf(dirbuf, sizeof(dirbuf), "%s/%s", filename, de->d_name);
+
+			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",
+						dirbuf, strerror(errno));
+				goto err;
+			}
+
+			erec = scanner_push_file(scanner, de->d_name, f, loc);
+			if (erec != NULL)
+				goto err;
+		}
+
+		closedir(directory);
+	}
+	else {
+		/* A simple include file. */
+		f = fopen(filename, "r");
+		if (f == NULL) {
+			erec = error(loc, "Could not open file \"%s\": %s\n",
+					filename, strerror(errno));
+			goto err;
+		}
+		erec = scanner_push_file(scanner, filename, f, loc);
+		if (erec != NULL)
+			goto err;
+	}
+
+	return 0;
+
+err:
+	erec_queue(erec, state->msgs);
+
+	if (directory)
+		closedir(directory);
+
+	return -1;
+}
+
 static bool search_in_include_path(const char *filename)
 {
 	return (strncmp(filename, "./", strlen("./")) != 0 &&
@@ -671,40 +749,36 @@ static bool search_in_include_path(const char *filename)
 int scanner_include_file(void *scanner, const char *filename,
 			 const struct location *loc)
 {
-	struct parser_state *state = yyget_extra(scanner);
-	struct error_record *erec;
 	char buf[PATH_MAX];
-	const char *name = buf;
 	unsigned int i;
-	FILE *f;
+	int ret;
+	struct error_record *erec;
+	struct parser_state *state = yyget_extra(scanner);
 
-	f = NULL;
 	if (search_in_include_path(filename)) {
 		for (i = 0; i < INCLUDE_PATHS_MAX; i++) {
 			if (include_paths[i] == NULL)
 				break;
 			snprintf(buf, sizeof(buf), "%s/%s",
 				 include_paths[i], filename);
-			f = fopen(buf, "r");
-			if (f != NULL)
-				break;
+
+			ret = find_and_add_include_file(scanner, buf, loc);
+			if (ret == 0)
+				return 0;
+			else if (ret != 1)
+				/* error has been processed already */
+				return -1;
 		}
-	} else {
-		f = fopen(filename, "r");
-		name = filename;
 	}
-	if (f == NULL) {
-		erec = error(loc, "Could not open file \"%s\": %s",
-			     filename, strerror(errno));
-		goto err;
+	else {
+		ret = find_and_add_include_file(scanner, filename, loc);
+		if (ret == 0)
+			return 0;
+		else if (ret != 1)
+		    return -1;
 	}
 
-	erec = scanner_push_file(scanner, name, f, loc);
-	if (erec != NULL)
-		goto err;
-	return 0;
-
-err:
+	erec = error(loc, "Did not find \"%s\"\n", filename);
 	erec_queue(erec, state->msgs);
 	return -1;
 }
-- 
2.9.4

[PATCH v2 2/2] tests: test include directories

[Ismo Puustinen]

Add tests for:
  * including an empty directory
  * including directory with one or two files in it
  * testing for required trailing slash in directory name
  * testing for detecting non-existent directory
  * testing for a broken file in included directory

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>
---
 tests/shell/testcases/include/0005dir_empty_0      | 29 +++++++++++++
 tests/shell/testcases/include/0006dir_single_0     | 36 +++++++++++++++++
 tests/shell/testcases/include/0007dir_double_0     | 45 +++++++++++++++++++++
 tests/shell/testcases/include/0008dir_no_slash_1   | 29 +++++++++++++
 tests/shell/testcases/include/0009dir_nodir_1      | 31 ++++++++++++++
 .../shell/testcases/include/0010dir_broken_file_1  | 47 ++++++++++++++++++++++
 6 files changed, 217 insertions(+)
 create mode 100755 tests/shell/testcases/include/0005dir_empty_0
 create mode 100755 tests/shell/testcases/include/0006dir_single_0
 create mode 100755 tests/shell/testcases/include/0007dir_double_0
 create mode 100755 tests/shell/testcases/include/0008dir_no_slash_1
 create mode 100755 tests/shell/testcases/include/0009dir_nodir_1
 create mode 100755 tests/shell/testcases/include/0010dir_broken_file_1

diff --git a/tests/shell/testcases/include/0005dir_empty_0 b/tests/shell/testcases/include/0005dir_empty_0
new file mode 100755
index 0000000..f16acf8
--- /dev/null
+++ b/tests/shell/testcases/include/0005dir_empty_0
@@ -0,0 +1,29 @@
+#!/bin/bash
+
+set -e
+
+tmpdir=$(mktemp -d)
+if [ ! -d $tmpdir ] ; then
+        echo "Failed to create tmp directory" >&2
+        exit 0
+fi
+
+tmpfile1=$(mktemp)
+if [ ! -w $tmpfile1 ] ; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+# cleanup if aborted
+trap "rm -rf $tmpfile1 && rmdir $tmpdir" EXIT
+
+RULESET1="include \"$tmpdir/\""
+
+echo "$RULESET1" > $tmpfile1
+
+$NFT -f $tmpfile1
+
+if [ $? -ne 0 ] ; then
+        echo "E: unable to load good ruleset" >&2
+        exit 1
+fi
diff --git a/tests/shell/testcases/include/0006dir_single_0 b/tests/shell/testcases/include/0006dir_single_0
new file mode 100755
index 0000000..ae4fd5f
--- /dev/null
+++ b/tests/shell/testcases/include/0006dir_single_0
@@ -0,0 +1,36 @@
+#!/bin/bash
+
+set -e
+
+tmpdir=$(mktemp -d)
+if [ ! -d $tmpdir ] ; then
+        echo "Failed to create tmp directory" >&2
+        exit 0
+fi
+
+tmpfile1=$(mktemp -p $tmpdir)
+if [ ! -w $tmpfile1 ] ; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+tmpfile2=$(mktemp)
+if [ ! -w $tmpfile2 ] ; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+# cleanup if aborted
+trap "rm -rf $tmpfile1 $tmpfile2 && rmdir $tmpdir" EXIT
+
+RULESET1="add table x"
+RULESET2="include \"$tmpdir/\""
+
+echo "$RULESET1" > $tmpfile1
+echo "$RULESET2" > $tmpfile2
+
+$NFT -f $tmpfile2
+if [ $? -ne 0 ] ; then
+        echo "E: unable to load good ruleset" >&2
+        exit 1
+fi
diff --git a/tests/shell/testcases/include/0007dir_double_0 b/tests/shell/testcases/include/0007dir_double_0
new file mode 100755
index 0000000..0a14ade
--- /dev/null
+++ b/tests/shell/testcases/include/0007dir_double_0
@@ -0,0 +1,45 @@
+#!/bin/bash
+
+set -e
+
+tmpdir=$(mktemp -d)
+if [ ! -d $tmpdir ] ; then
+        echo "Failed to create tmp directory" >&2
+        exit 0
+fi
+
+tmpfile1=$(mktemp -p $tmpdir)
+if [ ! -w $tmpfile1 ] ; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+tmpfile2=$(mktemp -p $tmpdir)
+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
+
+RULESET1="add table x"
+RULESET2="add table 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/0008dir_no_slash_1 b/tests/shell/testcases/include/0008dir_no_slash_1
new file mode 100755
index 0000000..2820dc9
--- /dev/null
+++ b/tests/shell/testcases/include/0008dir_no_slash_1
@@ -0,0 +1,29 @@
+#!/bin/bash
+
+set -e
+
+tmpdir=$(mktemp -d)
+if [ ! -d $tmpdir ] ; then
+        echo "Failed to create tmp directory" >&2
+        exit 0
+fi
+
+tmpfile1=$(mktemp -p $tmpdir)
+if [ ! -w $tmpfile1 ] ; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+# cleanup if aborted
+trap "rm -rf $tmpfile1 && rmdir $tmpdir" EXIT
+
+RULESET1="include \"$tmpdir\""
+
+echo "$RULESET1" > $tmpfile1
+
+$NFT -f $tmpfile1
+
+if [ $? -eq 0 ] ; then
+        echo "E: did not catch missing slash in directory name" >&2
+        exit 1
+fi
diff --git a/tests/shell/testcases/include/0009dir_nodir_1 b/tests/shell/testcases/include/0009dir_nodir_1
new file mode 100755
index 0000000..d7407f4
--- /dev/null
+++ b/tests/shell/testcases/include/0009dir_nodir_1
@@ -0,0 +1,31 @@
+#!/bin/bash
+
+set -e
+
+tmpdir=$(mktemp -d)
+if [ ! -d $tmpdir ] ; then
+        echo "Failed to create tmp directory" >&2
+        exit 0
+fi
+
+# remove the directory
+rmdir $tmpdir
+
+tmpfile1=$(mktemp)
+if [ ! -w $tmpfile1 ] ; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+# cleanup if aborted
+trap "rm -rf $tmpfile1" EXIT
+
+RULESET1="include \"$tmpdir/\""
+
+echo "$RULESET1" > $tmpfile1
+
+$NFT -f $tmpfile1
+if [ $? -eq 0 ] ; then
+        echo "E: Failed to catch a missing include directory/file" >&2
+        exit 1
+fi
diff --git a/tests/shell/testcases/include/0010dir_broken_file_1 b/tests/shell/testcases/include/0010dir_broken_file_1
new file mode 100755
index 0000000..c093974
--- /dev/null
+++ b/tests/shell/testcases/include/0010dir_broken_file_1
@@ -0,0 +1,47 @@
+#!/bin/bash
+
+set -e
+
+tmpdir=$(mktemp -d)
+if [ ! -d $tmpdir ] ; then
+        echo "Failed to create tmp directory" >&2
+        exit 0
+fi
+
+tmpfile1=$(mktemp -p $tmpdir)
+if [ ! -w $tmpfile1 ] ; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+tmpfile2=$(mktemp -p $tmpdir)
+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
+
+RULESET1="add table x"
+
+# do an error in a file
+RULESET2="intentionally broken file"
+RULESET3="include \"$tmpdir/\""
+
+echo "$RULESET1" > $tmpfile1
+echo "$RULESET2" > $tmpfile2
+echo "$RULESET3" > $tmpfile3
+
+$NFT -f $tmpfile3
+
+if [ $? -eq 0 ] ; then
+        echo "E: didn't catch a broken file in directory" >&2
+        exit 1
+fi
-- 
2.9.4

Re: [PATCH v2 1/2] scanner: add support for include directories

[Pablo Neira Ayuso]

Hi Ismo,

On Mon, Jun 05, 2017 at 06:12:32PM +0300, Ismo Puustinen wrote:
> If a string after "include" keyword points to a directory instead of a
> file, consider the directory to contain only nft rule files and try to
> load them all. This helps with a use case where services drop their own
> firewall configuration files into a directory and nft needs to include
> those without knowing the exact file names.
> 
> File loading order from the include directory is not specified, so the
> files inside an include directory should not depend on each other.
> 
> Fixes(Bug 1154 - Allow include statement to operate on directories and/or wildcards).

Just request for comestic changes, see below.

> Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>
> ---
>  src/scanner.l | 116 +++++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 95 insertions(+), 21 deletions(-)
> 
> diff --git a/src/scanner.l b/src/scanner.l
> index c2c008d..19e12bc 100644
> --- a/src/scanner.l
> +++ b/src/scanner.l
> @@ -10,6 +10,8 @@
>  
>  %{
>  
> +#include <dirent.h>
> +#include <libgen.h>
>  #include <limits.h>
>  #include <netinet/in.h>
>  #include <arpa/inet.h>
> @@ -661,6 +663,82 @@ err:
>  	return -1;
>  }
>  
> +static int find_and_add_include_file(void *scanner, const char *filename,
> +			 const struct location *loc)

Probably shorter function: include_dentry() ?

And please, align second line to parens.

static int include_dentry(void *scanner, const char *entryname,
                          const struct location *loc)
                          ^

> +{
> +	DIR *directory = NULL;

No need to set this to NULL.

> +	FILE *f = NULL;

Same thing?

> +	struct error_record *erec;
> +	struct parser_state *state = yyget_extra(scanner);

We prefer to order variable definitions in inverse line length, eg.

	struct parser_state *state = yyget_extra(scanner);
	struct error_record *erec;
	DIR *directory;
	FILE *f;

> +	/* The file can be either a simple file or a directory which
> +	 * contains files. If it is a directory, assume that all files there
> +	 * should be included. */

Preferred coding style for comment:

	/* The file can be either a simple file or a directory which
	 * contains files. If it is a directory, assume that all files there
	 * should be included.
         */

> +
> +	directory = opendir(filename);
> +
> +	if (directory == NULL && errno != ENOTDIR)
> +		/* Could not access the directory or file, keep on searching. */
> +		return 1;

We usually return negative on error, ie. -1.

> +	else if (directory != NULL) {

No need for else? Given we return above.

> +		/* A directory. */

Remove this comment...

Better place this code below in function so we save one level of
indent, for this function name I'd suggest include_directory().

> +		struct dirent *de;
> +
> +		if (!filename[0] || filename[strlen(filename)-1] != '/') {
> +			erec = error(loc, "Include directory name \"%s\" does not end in '/'",
> +					filename);
> +			goto err;
> +		}
> +
> +		/* If the path is a directory, assume that all files there need
> +		 * to be included. */
> +		while ((de = readdir(directory))) {
> +			char dirbuf[PATH_MAX];
> +
> +			snprintf(dirbuf, sizeof(dirbuf), "%s/%s", filename, de->d_name);
> +
> +			if (strcmp(de->d_name, ".") == 0 || strcmp(de->d_name, "..") == 0)
> +				continue;
> +
> +			f = fopen(dirbuf, "r");
> +

No need for empty line.

> +			if (f == NULL) {
> +				erec = error(loc, "Could not open file \"%s\": %s\n",
> +						dirbuf, strerror(errno));
> +				goto err;
> +			}
> +
> +			erec = scanner_push_file(scanner, de->d_name, f, loc);
> +			if (erec != NULL)
> +				goto err;
> +		}
> +
> +		closedir(directory);
> +	}
> +	else {

We prefer this:

        } else {

> +		/* A simple include file. */
> +		f = fopen(filename, "r");
> +		if (f == NULL) {
> +			erec = error(loc, "Could not open file \"%s\": %s\n",
> +					filename, strerror(errno));
> +			goto err;
> +		}
> +		erec = scanner_push_file(scanner, filename, f, loc);
> +		if (erec != NULL)
> +			goto err;
> +	}

Wrap this code above in function, eg. include_file() ?

> +
> +	return 0;
> +
> +err:
> +	erec_queue(erec, state->msgs);
> +
> +	if (directory)
> +		closedir(directory);
> +
> +	return -1;
> +}
> +
>  static bool search_in_include_path(const char *filename)
>  {
>  	return (strncmp(filename, "./", strlen("./")) != 0 &&
> @@ -671,40 +749,36 @@ static bool search_in_include_path(const char *filename)
>  int scanner_include_file(void *scanner, const char *filename,
>  			 const struct location *loc)
>  {
> -	struct parser_state *state = yyget_extra(scanner);
> -	struct error_record *erec;
>  	char buf[PATH_MAX];
> -	const char *name = buf;
>  	unsigned int i;
> -	FILE *f;
> +	int ret;
> +	struct error_record *erec;
> +	struct parser_state *state = yyget_extra(scanner);
>  
> -	f = NULL;
>  	if (search_in_include_path(filename)) {
>  		for (i = 0; i < INCLUDE_PATHS_MAX; i++) {
>  			if (include_paths[i] == NULL)
>  				break;
>  			snprintf(buf, sizeof(buf), "%s/%s",
>  				 include_paths[i], filename);
> -			f = fopen(buf, "r");
> -			if (f != NULL)
> -				break;
> +
> +			ret = find_and_add_include_file(scanner, buf, loc);
> +			if (ret == 0)
> +				return 0;
> +			else if (ret != 1)
> +				/* error has been processed already */
> +				return -1;
>  		}
> -	} else {
> -		f = fopen(filename, "r");
> -		name = filename;
>  	}
> -	if (f == NULL) {
> -		erec = error(loc, "Could not open file \"%s\": %s",
> -			     filename, strerror(errno));
> -		goto err;
> +	else {
> +		ret = find_and_add_include_file(scanner, filename, loc);
> +		if (ret == 0)
> +			return 0;
> +		else if (ret != 1)
> +		    return -1;
                   ^
          wrong indent.

Re: [PATCH v2 2/2] tests: test include directories

[Pablo Neira Ayuso]

On Mon, Jun 05, 2017 at 06:12:33PM +0300, Ismo Puustinen wrote:
> Add tests for:
>   * including an empty directory
>   * including directory with one or two files in it
>   * testing for required trailing slash in directory name
>   * testing for detecting non-existent directory
>   * testing for a broken file in included directory

This looks good to me. Thanks for adding tests!

Will take this once we sort out comestic issues with 1/2.