Captive Portal: re-introduce hash lookup for accounting purposes (#10275)

* Captive Portal: re-introduce hash lookup for accounting purposes

Table redirection allowed for constant time lookups, with the
migration to pf this was changed to a linear time lookup. This commit
changes this back, but retrofits it on top of the ipv6 compatibility change.

While here:

- fix a small edge case that kills states for ips
flipping primary IPs according to hostwatch.
- make sure only the most recent arp entry is considered
- make sure to clear empty addresses from the set

Cherry-picked from 1bf1c69
Cherry-picked from 3c2780e

src/opnsense/mvc/app/controllers/OPNsense/CaptivePortal/Api/ServiceController.php

@@ -45,6 +45,24 @@ class ServiceController extends ApiMutableServiceControllerBase
     protected static $internalServiceTemplate = 'OPNsense/Captiveportal';
     protected static $internalServiceName = 'captiveportal';
 
+    public function reconfigureAction()
+    {
+        $status = ['status' => 'failed'];
+
+        if ($this->request->isPost()) {
+            $backend = new Backend();
+            $backend->configdRun('template reload OPNsense/IPFW');
+            $result = trim($backend->configdRun("ipfw reload"));
+            if ($result != "OK") {
+                return ["status" => "error reloading ipfw (" . $result . ")"];
+            }
+
+            $status = parent::reconfigureAction();
+        }
+
+        return $status;
+    }
+
     protected function serviceEnabled()
     {
         return  $this->getModel()->isEnabled();

src/opnsense/scripts/captiveportal/cp-background-process.py

@@ -64,13 +64,15 @@ class CPBackgroundProcess(object):
         """
         return self._conf_zone_info.keys()
 
-    def _add_client(self, zoneid, ip):
-        PF.add_to_table(zoneid, ip)
-        IPFW.add_accounting(ip)
+    def _add_client(self, zoneid, ips):
+        # Add a single client, one or more ips
+        for ip in ips:
+            PF.add_to_table(zoneid, ip)
+        IPFW.add_accounting(zoneid, ips)
 
     def _remove_client(self, zoneid, ip):
         PF.remove_from_table(zoneid, ip)
-        IPFW.del_accounting(ip)
+        IPFW.del_accounting(zoneid, ip)
 
     def initialize_fixed(self):
         """ initialize fixed ip / hosts per zone
@@ -186,11 +188,11 @@ class CPBackgroundProcess(object):
                 if db_client['authenticated_via'] != '---ip---':
                     current_ips = self.arp.get_all_addresses_by_mac(db_client['macAddress'])
                     if len(current_ips) > 0 and db_client['ipAddress'] != current_ips[0]:
-                        if db_client['ipAddress'] != '':
-                            # remove old ip
+                        if not allow_roaming and db_client['ipAddress'] != '':
+                            # Remove old ip, but only if non-roaming, otherwise, we're state killing traffic from a different IP which may be valid.
+                            # Unused addresses are cleared through arp/hostwatch automatically when roaming.
                             self._remove_client(zoneid, db_client['ipAddress'])
                         self.db.update_client_ip(zoneid, db_client['sessionId'], current_ips[0])
-                        self._add_client(zoneid, current_ips[0])
                         db_client['ipAddress'] = current_ips[0]
 
                 # session should be active, validate its properties
@@ -200,11 +202,14 @@ class CPBackgroundProcess(object):
                 else:
                     # may have been updated if primary IP changed
                     session_ips = {db_client['ipAddress']}
+                # discard empty IPs (authenticated via MAC, but no IP known)
+                session_ips.discard("")
 
                 to_add = (session_ips - registered_addresses_pf) | (session_ips - registered_addresses_ipfw)
                 if session_ips and to_add:
-                    for ip in to_add:
-                        self._add_client(zoneid, ip)
+                    # intentionally add the whole set of IPs so the accounting rules in ipfw sync up properly,
+                    # but only do so if there is a change (to_add is not empty)
+                    self._add_client(zoneid, session_ips)
 
             # remove any address from pf that isn't expected
             expected_addresses = set()

src/opnsense/scripts/captiveportal/lib/arp.py

@@ -88,7 +88,9 @@ class ARP(object):
                 entry["first_seen"] = datetime.strptime(row[4], "%Y-%m-%d %H:%M:%S")
                 entry["last_seen"]  = datetime.strptime(row[5], "%Y-%m-%d %H:%M:%S")
 
-            self._table[ip] = entry
+            # only first one is relevant
+            if not ip in self._table:
+                self._table[ip] = entry
 
     def get_by_ipaddress(self, address):
         return self._table.get(address, None)

src/opnsense/scripts/captiveportal/lib/ipfw.py

@@ -1,5 +1,6 @@
 """
-    Copyright (c) 2025 Deciso B.V.
+    Copyright (c) 2025-2026 Deciso B.V.
+    Copyright (c) 2015-2019 Ad Schellevis <ad@opnsense.org>
     All rights reserved.
 
     Redistribution and use in source and binary forms, with or without
@@ -24,18 +25,33 @@
     POSSIBILITY OF SUCH DAMAGE.
 
 """
-import os
 import subprocess
-import ipaddress
 
 class IPFW(object):
     @staticmethod
-    def _is_ipv6(address):
-        try:
-            ipaddress.IPv6Address(address)
-            return True
-        except (ValueError, AttributeError):
-            return False
+    def list_table(table_number):
+        """ list ipfw table
+        :param table_number: ipfw table number
+        :return: dict (key value address + rule_number)
+        """
+        result = dict()
+        sp = subprocess.run(['/sbin/ipfw', 'table', str(table_number), 'list'], capture_output=True, text=True)
+        for line in sp.stdout.split('\n'):
+            if line.split(' ')[0].strip() != "":
+                parts = line.split()
+                address = parts[0]
+                rulenum = parts[1] if len(parts) > 1 else None
+                # process /32 and /128 nets as single addresses to align better with the rule syntax
+                # and local administration.
+                prefix = address.split('/')[-1]
+                if prefix == '32' or prefix == '128':
+                    # single IP address
+                    result[address.split('/')[0]] = rulenum
+                elif not line.startswith('-'):
+                    # network
+                    result[address] = rulenum
+
+        return result
 
     @staticmethod
     def list_accounting_info():
@@ -75,43 +91,72 @@ class IPFW(object):
         return result
 
     @staticmethod
-    def add_accounting(address):
-        """ add ip address for accounting
+    def add_accounting(table_number, addresses):
+        """ add ip addresses for accounting
+        this function assumes all addresses passed belong to the same client and will
+        therefore assign the same rule number to these addresses.
         :param address: ip address
         :return: added or known rule number
         """
-        # search for unused rule number
+        ipfw_tbl = IPFW.list_table(table_number)
         acc_info = IPFW.list_accounting_info()
-        if address not in acc_info:
+
+        a_present = set(addresses) & acc_info.keys()
+        a_missing = set(addresses) - acc_info.keys()
+        present_rules = sorted([(acc_info[address]['rule'], address) for address in a_present])
+        first = present_rules[0] if present_rules else None
+        rule_number = None
+
+        if first is not None:
+            # Re-use this rule number, check if the rest use this rule number.
+            # If not, delete those and add to the missing set to sync
+            rule_number = first[0]
+            rest = present_rules[1:]
+            for r_rulenr, addr in rest:
+                if r_rulenr != rule_number:
+                    IPFW.del_accounting(table_number, addr)
+                    a_missing.add(addr)
+        else:
+            # find unused rule number
             rule_ids = list()
             for ip_address in acc_info:
                 if acc_info[ip_address]['rule'] not in rule_ids:
                     rule_ids.append(acc_info[ip_address]['rule'])
-
             new_rule_id = -1
             for ruleId in range(30000, 50000):
                 if ruleId not in rule_ids:
                     new_rule_id = ruleId
                     break
-
-            # add accounting rule
             if new_rule_id != -1:
-                proto = 'ip6' if IPFW._is_ipv6(address) else 'ip'
-                subprocess.run(['/sbin/ipfw', 'add', str(new_rule_id), 'count', proto, 'from', address, 'to', 'any'],
-                               capture_output=True)
-                subprocess.run(['/sbin/ipfw', 'add', str(new_rule_id), 'count', proto, 'from', 'any', 'to', address],
-                               capture_output=True)
+                rule_number = new_rule_id
+        
+        if rule_number is not None:
+            for address in a_missing:
+                subprocess.run(['/sbin/ipfw', 'add', str(rule_number), 'count', 'ip', 'from', address, 'to', 'any'],
+                            capture_output=True)
+                subprocess.run(['/sbin/ipfw', 'add', str(rule_number), 'count', 'ip', 'from', 'any', 'to', address],
+                            capture_output=True)
 
-                return new_rule_id
-        else:
-            return acc_info[address]['rule']
+                if address not in ipfw_tbl:
+                    subprocess.run(['/sbin/ipfw', 'table', str(table_number), 'add', address, str(rule_number)], capture_output=True)
+                elif str(ipfw_tbl[address] != str(rule_number)):
+                    # update table when accounting rule mismatches table entry
+                    subprocess.run(['/sbin/ipfw', 'table', str(table_number), 'del', address], capture_output=True)
+                    subprocess.run(['/sbin/ipfw', 'table', str(table_number), 'add', address, str(rule_number)], capture_output=True)
+            if len(a_missing) > 0:
+                # end of accounting block lives at rule number 50000
+                subprocess.run(['/sbin/ipfw', 'add', str(rule_number), 'skipto', '60000', 'ip', 'from', 'any', 'to', 'any'], capture_output=True)
 
     @staticmethod
-    def del_accounting(address):
+    def del_accounting(table_number, address):
         """ remove ip address from accounting rules
         :param address: ip address
         :return: None
         """
         acc_info = IPFW.list_accounting_info()
+
         if address in acc_info:
             subprocess.run(['/sbin/ipfw', 'delete', str(acc_info[address]['rule'])], capture_output=True)
+        
+        # no-op if address not in table
+        subprocess.run(['/sbin/ipfw', 'table', str(table_number), 'del', address], capture_output=True)

src/opnsense/service/templates/OPNsense/IPFW/ipfw.conf

@@ -1,5 +1,22 @@
 {# Macro import #}
 {% from 'OPNsense/IPFW/rules.macro' import convert_address %}
+{# collect interfaces list (with / without captive portal enabled) #}
+{% set cp_interface_list = [] %}
+{% if helpers.exists('OPNsense.captiveportal.zones.zone') %}
+{% for intf_key,interface in interfaces.items()%}
+{%      set is_cp=[] %}
+{%      for cp_item in  helpers.toList('OPNsense.captiveportal.zones.zone') %}
+{%        for cp_intf in cp_item.interfaces.split(',') %}
+{%          if intf_key == cp_intf %}
+{%              if cp_item.enabled|default('0') == '1' %}
+{%                  do cp_interface_list.append({'zone':cp_item.description, 'zoneid':cp_item.zoneid,'if':interface.if, 'obj':cp_item}) %}
+{%                  do is_cp.append(1) %}
+{%              endif %}
+{%          endif %}
+{%        endfor %}
+{%      endfor %}
+{% endfor %}
+{% endif %}
 
 #======================================================================================
 # flush ruleset
@@ -26,6 +43,15 @@ add 201 skipto 60000 ipv4 from 127.0.0.0/8 to any
 add 202 skipto 60000 ipv6 from any to ::1
 add 203 skipto 60000 ipv4 from any to 127.0.0.0/8
 
+{% for item in cp_interface_list %}
+#===================================================================================
+# zone {{item.zone}} ({{item.zoneid}}) / {{item.if}} configuration
+#===================================================================================
+{# accounting lookup #}
+add {{3000 + item.zoneid|int }} skipto tablearg ip from table({{item.zoneid|int}}) to any via {{item.if}}
+add {{3000 + item.zoneid|int }} skipto tablearg ip from any to table({{item.zoneid|int}}) via {{item.if}}
+{% endfor %}
+
 #======================================================================================
 # 30000 .... 49999 reserved for captive portal accounting rules
 #======================================================================================