Captive Portal: re-introduce hash lookup for accounting purposes

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

@@ -64,13 +64,15 @@ def list_zone_ids(self):
         """
         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 @@ def sync_zone(self, zoneid, registered_addr_accounting):
                 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]
                 else:
                     # if authenticated via 'allowed addresses', check if we should update the mac address (for display purposes)
@@ -205,18 +207,19 @@ def sync_zone(self, zoneid, registered_addr_accounting):
                 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)
+                    self._add_client(zoneid, to_add)
 
             # remove any address from pf that isn't expected
             expected_addresses = set()
             for db_client in expected_clients:
                 expected_addresses.update(self.db.list_session_ips(zoneid, db_client['sessionId']))
 
-            for registered_address in registered_addresses_pf:
+            for registered_address in registered_addresses_pf | registered_addresses_ipfw:
                 if registered_address not in expected_addresses:
                     self._remove_client(zoneid, registered_address)
 

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 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,79 @@ def list_accounting_info():
         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()
+        a_missing = set()
+        for address in addresses:
+            if address not in acc_info:
+                a_missing.add(address)
+            else:
+                a_present.add(address)
+
+        present_rules = [(acc_info[address]['rule'], address) for address in sorted(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()
+        ipfw_tbl = IPFW.list_table(table_number)
+
         if address in acc_info:
             subprocess.run(['/sbin/ipfw', 'delete', str(acc_info[address]['rule'])], capture_output=True)
+
+        if address in ipfw_tbl:
+            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
 #======================================================================================