[PATCH nftables] Allow resetting the include search path

[PATCH nftables] Allow resetting the include search path

[Daniel Gröber]

Currently there is no way to disable searching in DEFAULT_INCLUDE_PATH
first. This is needed when testing nftables configurations spanning
multiple files without overwriting the globally installed ones.
---
 doc/nft.txt | 4 ++--
 src/main.c  | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/doc/nft.txt b/doc/nft.txt
index f7a53ac9..f04c3e20 100644
--- a/doc/nft.txt
+++ b/doc/nft.txt
@@ -55,8 +55,8 @@ understanding of their meaning. You can get information about options by running
 
 *-I*::
 *--includepath directory*::
-	Add the directory 'directory' to the list of directories to be searched for included files. This
-	option may be specified multiple times.
+	Append a directory to the end of the search path for the *include* statement. If the empty
+	string is passed the list is reset. This option may be specified multiple times.
 
 *-c*::
 *--check*::
diff --git a/src/main.c b/src/main.c
index 9bd25db8..f5dd3dba 100644
--- a/src/main.c
+++ b/src/main.c
@@ -411,7 +411,9 @@ int main(int argc, char * const *argv)
 			interactive = true;
 			break;
 		case OPT_INCLUDEPATH:
-			if (nft_ctx_add_include_path(nft, optarg)) {
+			if (strcmp(optarg, "") == 0) {
+				nft_ctx_clear_include_paths(nft);
+			} else if (nft_ctx_add_include_path(nft, optarg)) {
 				fprintf(stderr,
 					"Failed to add include path '%s'\n",
 					optarg);
-- 
2.30.2

Re: [PATCH nftables] Allow resetting the include search path

[Pablo Neira Ayuso]

Hi,

On Tue, Jun 28, 2022 at 12:23:04AM +0200, Daniel Gröber wrote:
> Currently there is no way to disable searching in DEFAULT_INCLUDE_PATH
> first. This is needed when testing nftables configurations spanning
> multiple files without overwriting the globally installed ones.

You can do

# cat x.nft
include "./z.nft"
# cat z.nft
add table x

then:

# nft -f x.nft

using ./ at the beginning of the path overrides DEFAULT_INCLUDE_PATH.

Is this what you are searching for?

> ---
>  doc/nft.txt | 4 ++--
>  src/main.c  | 4 +++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/nft.txt b/doc/nft.txt
> index f7a53ac9..f04c3e20 100644
> --- a/doc/nft.txt
> +++ b/doc/nft.txt
> @@ -55,8 +55,8 @@ understanding of their meaning. You can get information about options by running
>  
>  *-I*::
>  *--includepath directory*::
> -	Add the directory 'directory' to the list of directories to be searched for included files. This
> -	option may be specified multiple times.
> +	Append a directory to the end of the search path for the *include* statement. If the empty
> +	string is passed the list is reset. This option may be specified multiple times.
>  
>  *-c*::
>  *--check*::
> diff --git a/src/main.c b/src/main.c
> index 9bd25db8..f5dd3dba 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -411,7 +411,9 @@ int main(int argc, char * const *argv)
>  			interactive = true;
>  			break;
>  		case OPT_INCLUDEPATH:
> -			if (nft_ctx_add_include_path(nft, optarg)) {
> +			if (strcmp(optarg, "") == 0) {
> +				nft_ctx_clear_include_paths(nft);
> +			} else if (nft_ctx_add_include_path(nft, optarg)) {
>  				fprintf(stderr,
>  					"Failed to add include path '%s'\n",
>  					optarg);
> -- 
> 2.30.2
> 

Re: [PATCH nftables] Allow resetting the include search path

[Pablo Neira Ayuso]

On Tue, Jun 28, 2022 at 07:13:05PM +0200, Pablo Neira Ayuso wrote:
> Hi,
> 
> On Tue, Jun 28, 2022 at 12:23:04AM +0200, Daniel Gröber wrote:
> > Currently there is no way to disable searching in DEFAULT_INCLUDE_PATH
> > first. This is needed when testing nftables configurations spanning
> > multiple files without overwriting the globally installed ones.
> 
> You can do
> 
> # cat x.nft
> include "./z.nft"
> # cat z.nft
> add table x
> 
> then:
> 
> # nft -f x.nft
> 
> using ./ at the beginning of the path overrides DEFAULT_INCLUDE_PATH.

Absolute path also overrides DEFAULT_INCLUDE_PATH:

# cat x.nft
include "/foo/z.nft"

search_in_include_path() deals with this in scanner.l.

Relative path also overrides DEFAULT_INCLUDE_PATH.

Re: [PATCH nftables] Allow resetting the include search path

[Daniel Gröber]

Hi Pablo,

On Tue, Jun 28, 2022 at 07:13:02PM +0200, Pablo Neira Ayuso wrote:
> You can do
> 
> # cat x.nft
> include "./z.nft"
> # cat z.nft
> add table x
> 
> then:
> 
> # nft -f x.nft
> 
> using ./ at the beginning of the path overrides DEFAULT_INCLUDE_PATH.
> 
> Is this what you are searching for?

While that could work its rather a hassle. On my (Debian) system
nftables.service runs in the root directory so I'd have to do ugly stuff
like `include "./etc/nftables/foo.conf"` which I'd rather not. For one the
config would then depend on where `nft -f ...` is run exactly which sucks.

I think my patch is a much cleaner and general solution.

--Daniel

Re: [PATCH nftables] Allow resetting the include search path

[Pablo Neira Ayuso]

On Tue, Jun 28, 2022 at 09:01:01PM +0200, Daniel Gröber wrote:
> Hi Pablo,
> 
> On Tue, Jun 28, 2022 at 07:13:02PM +0200, Pablo Neira Ayuso wrote:
> > You can do
> > 
> > # cat x.nft
> > include "./z.nft"
> > # cat z.nft
> > add table x
> > 
> > then:
> > 
> > # nft -f x.nft
> > 
> > using ./ at the beginning of the path overrides DEFAULT_INCLUDE_PATH.
> > 
> > Is this what you are searching for?
> 
> While that could work its rather a hassle. On my (Debian) system
> nftables.service runs in the root directory so I'd have to do ugly stuff
> like `include "./etc/nftables/foo.conf"` which I'd rather not. For one the
> config would then depend on where `nft -f ...` is run exactly which sucks.

Hm, that's one way to put it, yes.

> I think my patch is a much cleaner and general solution.

I might be missing anything, could you describe your use-case?

You also consider that using absolute path in includes is suboptimal?

Re: [PATCH nftables] Allow resetting the include search path

[Daniel Gröber]

On Wed, Jun 29, 2022 at 07:20:20PM +0200, Pablo Neira Ayuso wrote:
> You also consider that using absolute path in includes is suboptimal?

Yeah sorry forgot to mention, using absolute paths defeats the use-case
entirely.

> > I think my patch is a much cleaner and general solution.
> 
> I might be missing anything, could you describe your use-case?

Ok so what I want to do is load an about to be deployed nftables config
without making it permanent yet as it might be buggy and cause an ssh
lockout. To prevent this I first load the temporary config with `nft -f`,
check ssh still works and only then commit the config to the final location
in /etc.

This works all fine and dandy when only one nftables.conf file is involved,
but as soon as I have includes I need to deploy the entire config directory
tree somewhere out-of-the-way.

If I use absolute paths then I'd have to put the new config in it's
permanent location immediately that defeats the purpose of this :)

If I use relative paths the success of the `nft -f` call depends on its
$PWD which as we've established would work but sucks for usability.

We have this nice search path mechanism already though, but if I just use
just the existing -I option, which appends to the search path, the existing
stuff in /etc takes precedence. Hence this patch, with it I can deploy to
say /tmp/nft.tmp/, load the config with `nft -I "" -I /tmp/nft.tmp -f ...`
and then commit if connectivity checks are successful.

--Daniel

Re: [PATCH nftables] Allow resetting the include search path

[Peter Tirsek]

[-- Attachment #1: Type: text/plain, Size: 2196 bytes --]

On Thu, 30 Jun 2022, Daniel Gröber wrote:

>> You also consider that using absolute path in includes is suboptimal?
> Ok so what I want to do is load an about to be deployed nftables config
> without making it permanent yet as it might be buggy and cause an ssh
> lockout. To prevent this I first load the temporary config with `nft -f`,
> check ssh still works and only then commit the config to the final location
> in /etc.
>
> This works all fine and dandy when only one nftables.conf file is involved,
> but as soon as I have includes I need to deploy the entire config directory
> tree somewhere out-of-the-way.

We're probably getting a little off topic for netfilter-devel, but 
could you do this using a mount namespace? For example (as root, since 
you indicated that you want to really load the actual ruleset into the 
main firewall):

Set up the nft config to test:

     root@nftest /tmp# mkdir -p nft-test/etc/nft.d
     root@nftest /tmp# cat > nft-test/etc/nft.conf <<<'include "/etc/nft.d/nft.included.conf"'
     root@nftest /tmp# cat > nft-test/etc/nft.d/nft.included.conf <<<$'flush ruleset\ncreate table inet filter'

Set up a temporary separate mount namespace. Here, this launches a new 
shell, so if you're automating this, the rest of the commands need to 
be in a separate script, and unshare can invoke that script instead of 
an interactive shell:

     root@nftest /tmp# unshare --mount

Set up the "fake" /etc to allow the absolute paths to work. This could 
probably also be done with an overlay mount if you need to preserve 
visibility of the underlying files in /etc, but it's a little more 
complicated and probably not necessary.

     root@nftest /tmp# mount -o bind nft-test/etc /etc
     root@nftest /tmp# ls -l /etc
     total 4
     -rw-r--r-- 1 0 0 39 Jul  1 00:02 nft.conf
     drwxr-xr-x 2 0 0 60 Jul  1 00:02 nft.d/

Now you can load the ruleset fully using absolute paths, even though 
the files are stored somewhere else:

     root@nftest /tmp# nft flush ruleset
     root@nftest /tmp# nft -f /etc/nft.conf
     root@nftest /tmp# nft list ruleset
     table inet filter {
     }

-- 
Peter Tirsek

Re: [PATCH nftables] Allow resetting the include search path

[Daniel Gröber]

On Thu, Jun 30, 2022 at 08:31:14PM -0500, Peter Tirsek wrote:
> On Thu, 30 Jun 2022, Daniel Gröber wrote:
> > This works all fine and dandy when only one nftables.conf file is involved,
> > but as soon as I have includes I need to deploy the entire config directory
> > tree somewhere out-of-the-way.
> 
> We're probably getting a little off topic for netfilter-devel, but could you
> do this using a mount namespace? For example (as root, since you indicated
> that you want to really load the actual ruleset into the main firewall):

I considered that too, but it's kind of like slicing butter with a chainsaw :)

--Daniel