Handle included files correctly and labeled as such

nftables/active_table.cgi

@@ -32,6 +32,10 @@ print ui_table_start($text{'active_table_summary'}, "width=100%", 2);
 print ui_table_row($text{'active_table'}, html_escape(nft_table_spec($table)));
 print ui_table_row($text{'active_flags'}, html_escape($table->{'flags'} || "-"));
 print ui_table_row($text{'active_status'}, $text{'active_'.$status_key});
+my ($saved_copy) = grep { table_key($_) eq table_key($table) } @saved_tables;
+print ui_table_row($text{'active_file'},
+		   "<tt>".html_escape($saved_copy->{'file'})."</tt>")
+    if ($saved_copy && $saved_copy->{'file'});
 print ui_table_end();
 
 if (!$is_saved && check_acl('import')) {

nftables/import_table.cgi

@@ -49,6 +49,9 @@ if ($in{'import'}) {
 	my $import = dclone($source);
 	$import->{'name'} = $name;
 	delete($import->{'flags'});
+
+	# It came from the live ruleset, so it has no file of its own yet
+	delete($import->{'file'});
 	assert_table_acl($import);
 	push(@tables, $import);
 	write_configuration(@tables);

nftables/index.cgi

@@ -423,6 +423,15 @@ else {
 	# Identify current table
 	my $curr = $tables[$in{'table'}];
 
+	# Say which file holds it, as the ruleset can be spread over the main
+	# configuration file and the files it includes
+	if ($curr && $curr->{'file'} && !$partial) {
+		print ui_tag('div',
+			text('index_table_file',
+			     "<tt>".html_escape($curr->{'file'})."</tt>"),
+			{'class' => 'nftables_table_file'}), "\n";
+		}
+
 	if ($curr) {
 		my ($sets_html, $chains_html);
 

nftables/lang/en

@@ -164,6 +164,7 @@ setup_eservice=Invalid service selected: $1
 setup_failed=Failed to create ruleset profile: <pre>$1</pre>
 index_profile_setup=Create Ruleset Profile
 index_profile_setupdesc=Create a managed nftables table from a predefined profile.
+index_table_file=Defined in $1
 index_table_create=Create Table
 index_table_createdesc=Add a new nftables table.
 index_table_delete=Delete Table
@@ -335,6 +336,7 @@ active_chains=Chains
 active_sets=Sets
 active_rules=Rules
 active_status=Status
+active_file=Saved in
 active_saved=Saved in configuration
 active_external=Externally managed
 active_unsaved=Not saved

nftables/nftables-lib.pl

@@ -366,32 +366,73 @@ rename_file($legacy, $legacy.".migrated");
 return scalar(@add);
 }
 
+# nftables_include_files(file)
+# Returns the files that a ruleset file pulls in with include directives, in
+# the order nft reads them. Includes are followed recursively, globs are
+# expanded, and a relative path resolves against the including file's own
+# directory
+sub nftables_include_files
+{
+my ($file, $seen) = @_;
+$seen ||= {};
+return () if (!$file || $file =~ /\|\s*$/);
+return () if ($seen->{simplify_path($file)}++);
+return () if (!-r $file);
+my $data = read_file_contents($file);
+return () if (!defined($data));
+my $dir = $file;
+$dir =~ s/\/[^\/]+$//;
+$dir = "/" if ($dir eq "");
+
+my @rv;
+my $depth = 0;
+foreach my $l (split(/\r?\n/, $data)) {
+	$l =~ s/#.*$//;
+	if ($depth) {
+		my $opens = () = $l =~ /\{/g;
+		my $closes = () = $l =~ /\}/g;
+		$depth += $opens - $closes;
+		$depth = 0 if ($depth < 0);
+		next;
+		}
+	if ($l =~ /^\s*table\s+\S+(\s+\S+)?\s*\{/) {
+		$depth = 1;
+		next;
+		}
+
+	# An include only means anything at the top level
+	next if ($l !~ /^\s*include\s+(\S.*?)\s*;?\s*$/);
+	my $spec = $1;
+	if ($spec =~ /^"([^"]*)"$/ || $spec =~ /^'([^']*)'$/) {
+		$spec = $1;
+		}
+	next if ($spec eq "");
+	$spec = $dir."/".$spec if ($spec !~ /^\//);
+	foreach my $inc (nftables_glob($spec)) {
+		next if (!-f $inc);
+		push(@rv, $inc, nftables_include_files($inc, $seen));
+		}
+	}
+return @rv;
+}
+
+# nftables_glob(pattern)
+# Expands one include pattern, without the word splitting that the built-in
+# glob does on paths containing spaces
+sub nftables_glob
+{
+my ($pattern) = @_;
+require File::Glob;
+return File::Glob::bsd_glob($pattern);
+}
+
 # get_nftables_config_files()
 # Returns files that can be manually edited by this module
 sub get_nftables_config_files
 {
-my @files;
-push(@files, nftables_rules_file());
-
-foreach my $sysfile ("/etc/nftables.conf", "/etc/sysconfig/nftables.conf") {
-	push(@files, $sysfile) if (-f $sysfile);
-	}
-
-if (-d "/etc/nftables") {
-	opendir(my $dir, "/etc/nftables");
-	if ($dir) {
-		foreach my $name (sort readdir($dir)) {
-			next if ($name =~ /^\./);
-			next if ($name !~ /\.(?:nft|conf)$/);
-			my $path = "/etc/nftables/$name";
-			push(@files, $path) if (-f $path);
-			}
-		closedir($dir);
-		}
-	}
-
+my $main = nftables_rules_file();
 my %seen;
-return grep { !$seen{$_}++ } @files;
+return grep { !$seen{$_}++ } ($main, nftables_include_files($main));
 }
 
 # list_foreign_firewall_modules()
@@ -412,19 +453,18 @@ foreach my $mod (@mods) {
 return @rv;
 }
 
-# validate_nftables_text(text)
-# Returns an error if nft rejects the supplied ruleset text
-sub validate_nftables_text
+# validate_nftables_files()
+# Returns an error if nft rejects the saved ruleset as it stands on disk.
+# An included file cannot be checked on its own, as it may well use a define
+# from the file that includes it, so the whole ruleset is checked from the
+# top
+sub validate_nftables_files
 {
-my ($text) = @_;
 my $cmd = get_nft_command();
 return text('index_ecommand', "<tt>nft</tt>") if (!$cmd);
-my $tmp = tempname();
-open_tempfile(my $fh, ">$tmp");
-print_tempfile($fh, $text);
-close_tempfile($fh);
-my $out = backquote_logged("$cmd -c -f $tmp 2>&1");
-unlink_file($tmp);
+my $file = nftables_rules_file();
+return if (!-r $file);
+my $out = backquote_logged("$cmd -c -f ".quotemeta($file)." 2>&1");
 return $? ? "<pre>$out</pre>" : undef;
 }
 
@@ -433,9 +473,21 @@ return $? ? "<pre>$out</pre>" : undef;
 sub get_nftables_save
 {
 my ($file) = @_;
-if (!$file) {
-	$file = nftables_rules_file();
+$file ||= nftables_rules_file();
+return () if (!$file);
+my @rv = parse_nftables_file($file);
+foreach my $inc (nftables_include_files($file)) {
+	push(@rv, parse_nftables_file($inc));
 	}
+return @rv;
+}
+
+# parse_nftables_file(file)
+# Returns the tables defined in one ruleset file, each tagged with the file
+# it came from so that it can be written back to the same place
+sub parse_nftables_file
+{
+my ($file) = @_;
 return () if (!$file);
 return () if ($file !~ /\|\s*$/ && !-r $file);
 
@@ -540,6 +592,7 @@ for (my $i = 0 ; $i < @lines ; $i++) {
 		$table = {
 			'name' => $2,
 			'family' => $1,
+			'file' => $file,
 			'line' => $lnum,
 			'rules' => [ ],
 			'chains' => {},
@@ -3793,13 +3846,38 @@ return $rv;
 sub write_configuration
 {
 my (@tables) = @_;
-my $file = nftables_rules_file();
-my ($pre, $post) = get_nftables_extras($file);
-my $out = $pre.dump_nftables_save(@tables).$post;
+my $main = nftables_rules_file();
+my @known = ($main, nftables_include_files($main));
+my %known = map { $_ => 1 } @known;
 
-open_lock_tempfile(my $fh, ">$file");
-print_tempfile($fh, $out);
-close_tempfile($fh);
+# Each table goes back to the file it was read from, so that a table living
+# in an included file is edited there instead of being copied into the main
+# one. Anything without a home, or pointing somewhere we do not manage, goes
+# to the main file
+my %byfile;
+foreach my $t (@tables) {
+	my $f = $t->{'file'};
+	$f = $main if (!$f || !$known{$f});
+	push(@{$byfile{$f}}, $t);
+	}
+
+# Every file that holds tables has to be re-written even if it ends up with
+# none, or a deleted table would survive in its own file
+foreach my $f (@known) {
+	$byfile{$f} ||= [ ];
+	}
+
+foreach my $f (sort keys %byfile) {
+	# Leave a file alone unless its own tables actually changed. Comparing
+	# through the dumper ignores whatever indenting the file happens to
+	# use, so an untouched file is not re-formatted behind the admin's back
+	my $want = dump_nftables_save(@{$byfile{$f}});
+	next if ($want eq dump_nftables_save(parse_nftables_file($f)));
+	my ($pre, $post) = get_nftables_extras($f);
+	open_lock_tempfile(my $fh, ">$f");
+	print_tempfile($fh, $pre.$want.$post);
+	close_tempfile($fh);
+	}
 update_last_config_change();
 return;
 }

nftables/save_manual.cgi

@@ -1,6 +1,6 @@
 #!/usr/bin/perl
 # save_manual.cgi
-# Save the manually edited nftables rules file
+# Save a manually edited nftables configuration file
 
 require './nftables-lib.pl';    ## no critic
 use strict;
@@ -16,13 +16,28 @@ my $file = $in{'file'};
 indexof($file, @files) >= 0 || error($text{'manual_efile'});
 
 $in{'data'} =~ s/\r//g;
-my $err = validate_nftables_text($in{'data'});
-error(text('manual_evalidate', $err)) if ($err);
 
+# Check the saved ruleset as a whole with the new content in place, as an
+# included file on its own may use a define from the file that includes it.
+# Put the old content back if nft rejects the result
+my $old = -r $file ? read_file_contents($file) : undef;
 open_lock_tempfile(my $fh, ">$file");
 print_tempfile($fh, $in{'data'});
 close_tempfile($fh);
 
+my $err = validate_nftables_files();
+if ($err) {
+	if (defined($old)) {
+		open_lock_tempfile(my $rfh, ">$file");
+		print_tempfile($rfh, $old);
+		close_tempfile($rfh);
+		}
+	else {
+		unlink_file($file);
+		}
+	error(text('manual_evalidate', $err));
+	}
+
 update_last_config_change();
 
 webmin_log("manual", undef, $file);

nftables/t/run-tests.t

@@ -602,9 +602,105 @@ like($epre, qr/Uncomment the include statement/,
      'comment-only file is kept ahead of new tables');
 is($epost, '', 'comment-only file leaves nothing trailing');
 
+our ($module_config_directory, $nftables_rules_file_cache);
+
+# A ruleset spread over a main file and the files it includes has to be read
+# from, and written back to, the file each table actually lives in
+my $incdir = "$confdir/nftables.d";
+mkdir($incdir);
+my $incmain = write_ruleset($confdir, 'main.nft', <<'EOF');
+#!/usr/sbin/nft -f
+flush ruleset
+
+define lan = 192.168.0.0/24
+
+include "nftables.d/*.nft"
+
+table inet main_table {
+	chain input {
+		type filter hook input priority 0; policy drop;
+	}
+}
+EOF
+write_ruleset($incdir, '10-web.nft', <<'EOF');
+# web rules
+table inet web {
+	chain input {
+		type filter hook input priority 10; policy accept;
+		tcp dport 80 accept
+	}
+}
+EOF
+# Deliberately non-canonical spacing, so that a needless re-write would show
+write_ruleset($incdir, '20-mail.nft', <<'EOF');
+table inet mail {
+      chain input {
+            type filter hook input priority 20; policy accept;
+            tcp dport 25 accept
+      }
+}
+EOF
+
+is_deeply([ nftables_include_files($incmain) ],
+          [ "$incdir/10-web.nft", "$incdir/20-mail.nft" ],
+          'relative include glob is expanded in order');
+
+{
+    local $nftables_rules_file_cache = $incmain;
+    my @inctables = get_nftables_save();
+    is_deeply([ map { $_->{name} } @inctables ],
+              [ 'main_table', 'web', 'mail' ],
+              'tables are read from the main file and its includes');
+    is($inctables[0]->{file}, $incmain, 'main table is tagged with its file');
+    is($inctables[1]->{file}, "$incdir/10-web.nft",
+       'included table is tagged with the file it came from');
+    is_deeply([ get_nftables_config_files() ],
+              [ $incmain, "$incdir/10-web.nft", "$incdir/20-mail.nft" ],
+              'the manual editor offers exactly the files that are loaded');
+
+    # An included table is saved, so it must not be offered for import
+    is(active_table_status({ family => 'inet', name => 'web' }, \@inctables),
+       'saved', 'a table from an included file counts as saved');
+
+    my $main_before = read_file_contents($incmain);
+    my $mail_before = read_file_contents("$incdir/20-mail.nft");
+
+    # Editing a table in an included file writes it back there
+    my ($web) = grep { $_->{name} eq 'web' } @inctables;
+    push(@{$web->{rules}}, {'text' => 'tcp dport 443 accept',
+                            'chain' => 'input', 'index' => 99});
+    write_configuration(@inctables);
+    like(read_file_contents("$incdir/10-web.nft"), qr/tcp dport 443 accept/,
+         'edit lands in the included file');
+    unlike(read_file_contents($incmain), qr/table inet web/,
+           'edit is not copied into the main file');
+    is(read_file_contents($incmain), $main_before,
+       'the main file is left alone');
+    is(read_file_contents("$incdir/20-mail.nft"), $mail_before,
+       'an untouched included file is not re-written');
+
+    # Deleting it empties that file without disturbing the others
+    my @keep = grep { $_->{name} ne 'web' } get_nftables_save();
+    write_configuration(@keep);
+    unlike(read_file_contents("$incdir/10-web.nft"), qr/table\s/,
+           'deleted table is removed from its own file');
+    like(read_file_contents("$incdir/10-web.nft"), qr/# web rules/,
+         'the emptied file keeps its own comments');
+    is(read_file_contents($incmain), $main_before,
+       'deleting from an include leaves the main file alone');
+    is_deeply([ map { $_->{name} } get_nftables_save() ],
+              [ 'main_table', 'mail' ], 'the deleted table is gone');
+}
+
+# An include loop must not send the parser into a spin
+my $loop_a = write_ruleset($confdir, 'loop-a.nft', "include \"loop-b.nft\"\n");
+write_ruleset($confdir, 'loop-b.nft', "include \"loop-a.nft\"\n");
+is_deeply([ nftables_include_files($loop_a) ],
+          [ "$confdir/loop-b.nft", "$confdir/loop-a.nft" ],
+          'an include loop terminates');
+
 # Upgrades have to move rules out of the module's old private file, or the
 # firewall silently disappears once the private boot action is gone
-our ($module_config_directory, $nftables_rules_file_cache);
 mkdir($module_config_directory) if (!-d $module_config_directory);
 my $legacy = write_ruleset($module_config_directory, 'rules.conf', <<'EOF');
 # This file was auto-generated by the module.