From c6918fec002e9fcb842903d4dbea2a40622c5bd9 Mon Sep 17 00:00:00 2001
From: Daniel Barlow <dan@telent.net>
Date: Sun, 9 Mar 2025 20:42:02 +0000
Subject: [PATCH] firewall: use extraText for zone set contents

* the lua necessary is quite wordy, but it's less of a hack than
post-processing the rules file with pseudo-sed to get rid of `elements
= { }` lines

* also switch from stop/starting the firewall service to using a
signal, so that we don't go briefly offline every time a new interface
appears
---
 modules/firewall/service.nix  | 42 ++++++++++++++++++++++++++---------
 tests/pppoe/getaddress.expect | 26 +++++++++++++++-------
 2 files changed, 49 insertions(+), 19 deletions(-)

diff --git a/modules/firewall/service.nix b/modules/firewall/service.nix
index 9fe275e..0fd93dc 100644
--- a/modules/firewall/service.nix
+++ b/modules/firewall/service.nix
@@ -21,7 +21,7 @@ let
   inherit (lib.attrsets) mapAttrs' nameValuePair mapAttrsToList;
   inherit (lib.strings) concatStringsSep;
   inherit (lib.lists) flatten;
-  inherit (builtins) concatLists attrValues;
+  inherit (builtins) concatLists toJSON attrValues;
   inherit (liminix) outputRef;
   mkSet =
     family: name:
@@ -29,11 +29,25 @@ let
       kind = "set";
       inherit name family;
       type = "ifname";
-      elements = map (s: "{{ output(${builtins.toJSON s}, \"ifname\", \"\") }}") zones.${name};
-    };
+      extraText = ''
+      {{;
+         local services = { ${concatStringsSep ", " (map toJSON zones.${name})} }
+         local ifnames = {}
+         for _, v in ipairs(services) do
+           local o = output(v, "ifname")
+           if o then table.insert(ifnames, o) end
+         end
+         if (#ifnames > 0) then
+           return "elements = { " .. table.concat(ifnames, ", ") .. " }\n"
+         else
+           return ""
+         end
+      }}
+      '';
+  };
   sets = (mapAttrs' (n: _: mkSet "ip" n) zones) //
          (mapAttrs' (n: _: mkSet "ip6" n) zones);
-  allRules = lib.recursiveUpdate extraRules (lib.recursiveUpdate (builtins.trace sets sets) rules);
+  allRules = lib.recursiveUpdate extraRules (lib.recursiveUpdate sets rules);
   script = firewallgen "firewall1.nft" allRules;
   ifwatch = writeFennel "ifwatch" {
     packages = [
@@ -48,19 +62,25 @@ let
   service = longrun {
     inherit name;
     run = ''
-      mkdir -p /run/${name}; in_outputs ${name}
-      # exec > /dev/console 2>&1
-      echo RESTARTING FIREWALL >/dev/console
       PATH=${nftables}/bin:${lua}/bin:$PATH
-      ${output-template}/bin/output-template '{{' '}}' < ${script} | lua -e 'for x in io.lines() do if not string.match(x, "elements = {%s+}") then print(x) end; end'   > /run/${name}/fw.nft
-      # cat /run/${name}/fw.nft > /dev/console
-      nft -f /run/${name}/fw.nft
-      while sleep 86400 ; do : ; done
+      reload() {
+         echo reloading firewall
+         ${output-template}/bin/output-template '{{' '}}' < ${script}  > /run/${name}/fw.nft;
+         nft -f /run/${name}/fw.nft ;
+      }
+      trap reload SIGUSR1
+      mkdir -p /run/${name}; in_outputs ${name}
+      reload
+      while :; do
+        # signals sent to ash won't interrupt sleep, but will interrupt wait
+        sleep 86400 & wait
+      done
     '';
     finish = "${nftables}/bin/nft flush ruleset";
   };
 in
 svc.secrets.subscriber.build {
+  action = "usr1";
   watch =
     concatLists
       (mapAttrsToList (_zone : services : map (s: outputRef s "ifname") services) zones);
diff --git a/tests/pppoe/getaddress.expect b/tests/pppoe/getaddress.expect
index 7634891..31d0459 100644
--- a/tests/pppoe/getaddress.expect
+++ b/tests/pppoe/getaddress.expect
@@ -8,29 +8,39 @@ send "PS1=\$(echo 'I1JFQURZIyA=' | base64 -d); stty -echo\n"
 expect "#READY#"
 set FINISHED 0
 set EXIT "1" 
-while { $FINISHED < 10 } {
+while { $FINISHED < 20 } {
   send "ip address show dev ppp0 | grep ppp0\n"
   expect {
-    "192.168.100.1" { set FINISHED 20; set EXIT 0; }
+    "192.168.100.1" { set FINISHED 200; set EXIT 0; }
     "can't find device" { send_user "waiting ..." ; sleep 3 }
     "DOWN" { send_user "waiting ..." ; sleep 3 }
   }
   set FINISHED [ expr $FINISHED + 1 ]
 }
 expect "#READY#"
-send "nft list set ip table-ip wan || touch /non/existent\n"
-expect {
-  "ppp0" { puts "ppp0 found " }
-  "{ }" { puts "missing ifname"; exit 1 }
-  "No such file or directory" { exit 1 }
-}  
+send "s6-svwait -U /run/service/wan.link.pppoe\n"
 expect "#READY#"
 
+set timeout 30
 send "nft list set ip table-ip lan || touch /non/existent\n"
 expect {
   "lan" { puts "lan found" }
   "{ }" { puts "missing ifname"; exit 1 }
   "No such file or directory" { exit 1 }
 }  
+
 expect "#READY#"
+
+# if the test fails for no apparent reason, it is quite likely because
+# the firewall hasn't had time to reload after the new interface
+# appears and you just have to make this sleep longer. Ew, yes
+send "sleep 10; nft list set ip table-ip wan || touch /non/existent\n"
+expect {
+  "ppp0" { puts "ppp0 found " }
+  "{ }" { puts "missing ifname"; exit 1 }
+  "No such file or directory" { exit 1 }
+  timeout { exit 1 }
+}  
+expect "#READY#"
+
 exit $EXIT