From e0b8113348e5256c29b4afddd4dfe57766cd0f0d Mon Sep 17 00:00:00 2001 From: Joe Cooper Date: Sat, 5 Sep 2026 18:26:24 -0500 Subject: [PATCH] Handle included files correctly and labeled as such --- nftables/active_table.cgi | 4 + nftables/import_table.cgi | 3 + nftables/index.cgi | 9 +++ nftables/lang/en | 2 + nftables/nftables-lib.pl | 156 ++++++++++++++++++++++++++++---------- nftables/save_manual.cgi | 21 ++++- nftables/t/run-tests.t | 98 +++++++++++++++++++++++- 7 files changed, 250 insertions(+), 43 deletions(-) diff --git a/nftables/active_table.cgi b/nftables/active_table.cgi index d66fcc071..dc7b7f4de 100755 --- a/nftables/active_table.cgi +++ b/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'}, + "".html_escape($saved_copy->{'file'})."") + if ($saved_copy && $saved_copy->{'file'}); print ui_table_end(); if (!$is_saved && check_acl('import')) { diff --git a/nftables/import_table.cgi b/nftables/import_table.cgi index 242a9cddc..cbd3ea355 100755 --- a/nftables/import_table.cgi +++ b/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); diff --git a/nftables/index.cgi b/nftables/index.cgi index cee147085..7f7e3be8d 100755 --- a/nftables/index.cgi +++ b/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', + "".html_escape($curr->{'file'}).""), + {'class' => 'nftables_table_file'}), "\n"; + } + if ($curr) { my ($sets_html, $chains_html); diff --git a/nftables/lang/en b/nftables/lang/en index da0cd4a08..fae9793d0 100644 --- a/nftables/lang/en +++ b/nftables/lang/en @@ -164,6 +164,7 @@ setup_eservice=Invalid service selected: $1 setup_failed=Failed to create ruleset profile: 
$1
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 diff --git a/nftables/nftables-lib.pl b/nftables/nftables-lib.pl index d73650dbb..7edfa4560 100644 --- a/nftables/nftables-lib.pl +++ b/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', "nft") 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 $? ? "
$out
" : undef; } @@ -433,9 +473,21 @@ return $? ? "
$out
" : 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; } diff --git a/nftables/save_manual.cgi b/nftables/save_manual.cgi index 85e6e7a3a..151b381f6 100755 --- a/nftables/save_manual.cgi +++ b/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); diff --git a/nftables/t/run-tests.t b/nftables/t/run-tests.t index eb796bdcd..2d017b14d 100755 --- a/nftables/t/run-tests.t +++ b/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.