Captive portal: optimise ipfw rule parsing. for #3559 (#3608)

Our current generated ruleset creates two count rules to match incoming and outgoing traffic to and from the client for accounting purposes. Since ipfw doesn't support table stats, the options are limited to know the amount of traffic processed and last accessed times.

This patch basically replaces the accounting section with seperate blocks, which are jumped to using the exising table (which contains address + rulenumber now), logically this would lower the time needed to parse the accounting section (since only the count rules for the specif ip's are evaulated now).

In terms of ruleset, this will generate 3 rules per address (count from, count to and jump to end of ruleset), like:

```
30001   342    27744 count ip from xxx.xxx.xxx.xxx to any
30001  1194   225783 count ip from any to xxx.xxx.xxx.xxx
30001  1536   253527 skipto 60000 ip from any to any       [ <--- NEW ]
```

Since we need the address to collect rules, we can't simply this count to one rule (IPFW.list_accounting_info() parses the address from the ruleset).

Our per zone "skipto" section, uses a tablearg in stead of static rule number now:

```
03001  2362   386004 skipto tablearg ip from table(1) to any via em2
03001  5701  4761746 skipto tablearg ip from any to table(1) via em2
```

(cherry picked from commit 440f957)

src/opnsense/scripts/OPNsense/CaptivePortal/allow.py

@@ -61,12 +61,7 @@
                                ip_address=parameters['ip_address'],
                                mac_address=mac_address
                                )
-    # check if address is not already registered before adding it to the ipfw table
-    if not cpIPFW.ip_or_net_in_table(table_number=parameters['zoneid'], address=parameters['ip_address']):
-        cpIPFW.add_to_table(table_number=parameters['zoneid'], address=parameters['ip_address'])
-
-    # add accounting for this ip address
-    cpIPFW.add_accounting(parameters['ip_address'])
+    cpIPFW.add_to_table(table_number=parameters['zoneid'], address=parameters['ip_address'])
     response['clientState'] = 'AUTHORIZED'
 else:
     response = {'clientState': 'UNKNOWN'}

src/opnsense/scripts/OPNsense/CaptivePortal/cp-background-process.py

@@ -161,17 +161,12 @@ def sync_zone(self, zoneid):
                             self.ipfw.delete(zoneid, db_client['ipAddress'])
                         self.db.update_client_ip(zoneid, db_client['sessionId'], current_ip)
                         self.ipfw.add_to_table(zoneid, current_ip)
-                        self.ipfw.add_accounting(current_ip)
 
                 # check session, if it should be active, validate its properties
                 if drop_session_reason is None:
-                    # registered client, but not active according to ipfw (after reboot)
-                    if cpnet not in registered_addresses:
+                    # registered client, but not active or missing accounting according to ipfw (after reboot)
+                    if cpnet not in registered_addresses or cpnet not in registered_addr_accounting:
                         self.ipfw.add_to_table(zoneid, cpnet)
-
-                    # is accounting rule still available? need to reapply after reload / reboot
-                    if cpnet not in registered_addr_accounting:
-                        self.ipfw.add_accounting(cpnet)
                 else:
                     # remove session
                     syslog.syslog(syslog.LOG_NOTICE, drop_session_reason)
@@ -226,7 +221,10 @@ def main():
 
             # process accounting messages (uses php script, for reuse of Auth classes)
             try:
-                subprocess.call(['/usr/local/opnsense/scripts/OPNsense/CaptivePortal/process_accounting_messages.php'])
+                subprocess.run(
+                    ['/usr/local/opnsense/scripts/OPNsense/CaptivePortal/process_accounting_messages.php'],
+                    capture_output=True
+                )
             except OSError:
                 # if accounting script crashes don't exit backgroung process
                 pass

src/opnsense/scripts/OPNsense/CaptivePortal/lib/arp.py

@@ -24,7 +24,6 @@
     POSSIBILITY OF SUCH DAMAGE.
 
 """
-import tempfile
 import subprocess
 
 
@@ -47,31 +46,29 @@ def _fetch_arp_table(self):
         """
         # parse arp table
         self._arp_table = dict()
-        with tempfile.NamedTemporaryFile() as output_stream:
-            subprocess.check_call(['/usr/sbin/arp', '-an'], stdout=output_stream, stderr=subprocess.STDOUT)
-            output_stream.seek(0)
-            for line in output_stream:
-                line_parts = line.decode().split()
-
-                if len(line_parts) < 6 or line_parts[2] != 'at' or line_parts[4] != 'on':
-                    continue
-                elif len(line_parts[1]) < 2 or line_parts[1][0] != '(' or line_parts[1][-1] != ')':
-                    continue
-
-                address = line_parts[1][1:-1]
-                physical_intf = line_parts[5]
-                mac = line_parts[3]
-                expires = -1
-
-                for index in range(len(line_parts) - 3):
-                    if line_parts[index] == 'expires' and line_parts[index + 1] == 'in':
-                        if line_parts[index + 2].isdigit():
-                            expires = int(line_parts[index + 2])
-
-                if address in self._arp_table:
-                    self._arp_table[address]['intf'].append(physical_intf)
-                elif mac.find('incomplete') == -1:
-                    self._arp_table[address] = {'mac': mac, 'intf': [physical_intf], 'expires': expires}
+        sp = subprocess.run(['/usr/sbin/arp', '-an'], capture_output=True, text=True)
+        for line in sp.stdout.split("\n"):
+            line_parts = line.split()
+
+            if len(line_parts) < 6 or line_parts[2] != 'at' or line_parts[4] != 'on':
+                continue
+            elif len(line_parts[1]) < 2 or line_parts[1][0] != '(' or line_parts[1][-1] != ')':
+                continue
+
+            address = line_parts[1][1:-1]
+            physical_intf = line_parts[5]
+            mac = line_parts[3]
+            expires = -1
+
+            for index in range(len(line_parts) - 3):
+                if line_parts[index] == 'expires' and line_parts[index + 1] == 'in':
+                    if line_parts[index + 2].isdigit():
+                        expires = int(line_parts[index + 2])
+
+            if address in self._arp_table:
+                self._arp_table[address]['intf'].append(physical_intf)
+            elif mac.find('incomplete') == -1:
+                self._arp_table[address] = {'mac': mac, 'intf': [physical_intf], 'expires': expires}
 
     def list_items(self):
         """ return parsed arp list

src/opnsense/scripts/OPNsense/CaptivePortal/lib/ipfw.py

@@ -25,7 +25,6 @@
 
 """
 import os
-import tempfile
 import subprocess
 
 
@@ -37,27 +36,26 @@ def __init__(self):
     def list_table(table_number):
         """ list ipfw table
         :param table_number: ipfw table number
-        :return: list
+        :return: dict (key value address + rule_number)
         """
         devnull = open(os.devnull, 'w')
-        result = list()
-        with tempfile.NamedTemporaryFile() as output_stream:
-            subprocess.check_call(['/sbin/ipfw', 'table', str(table_number), 'list'],
-                                  stdout=output_stream,
-                                  stderr=devnull)
-            output_stream.seek(0)
-            for line in output_stream:
-                line = line.decode()
-                if line.split(' ')[0].strip() != "":
-                    # process / 32 nets as single addresses to align better with the rule syntax
-                    # and local administration.
-                    if line.split(' ')[0].split('/')[-1] == '32':
-                        # single IPv4 address
-                        result.append(line.split(' ')[0].split('/')[0])
-                    else:
-                        # network
-                        result.append(line.split(' ')[0])
-            return result
+        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.
+                if address.split('/')[-1] == '32':
+                    # single IPv4 address
+                    result[address.split('/')[0]] = rulenum
+                elif not line.startswith('-'):
+                    # network
+                    result[address] = rulenum
+
+        return result
 
     def ip_or_net_in_table(self, table_number, address):
         """ check if address or net is in this zone's table
@@ -71,15 +69,21 @@ def ip_or_net_in_table(self, table_number, address):
 
         return False
 
-    @staticmethod
-    def add_to_table(table_number, address):
-        """ add new entry to ipfw table
+    def add_to_table(self, table_number, address):
+        """ add/update entry to ipfw table
         :param table_number: ipfw table number
         :param address: ip address or net to add to table
         :return:
         """
-        devnull = open(os.devnull, 'w')
-        subprocess.call(['/sbin/ipfw', 'table', table_number, 'add', address], stdout=devnull, stderr=devnull)
+        subprocess.run(['/sbin/ipfw', 'table', str(table_number), 'add', address], capture_output=True)
+        ipfw_tbl = self.list_table(table_number)
+        rule_number = str(self._add_accounting(address))
+        if address not in ipfw_tbl:
+            subprocess.run(['/sbin/ipfw', 'table', str(table_number), 'add', address, rule_number], capture_output=True)
+        elif str(ipfw_tbl[address]) != str(table_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, rule_number], capture_output=True)
 
     @staticmethod
     def delete_from_table(table_number, address):
@@ -88,59 +92,52 @@ def delete_from_table(table_number, address):
         :param address: ip address or net to add to table
         :return:
         """
-        devnull = open(os.devnull, 'w')
-        subprocess.call(['/sbin/ipfw', 'table', table_number, 'delete', address], stdout=devnull, stderr=devnull)
+        subprocess.run(['/sbin/ipfw', 'table', str(table_number), 'delete', address], capture_output=True)
 
     @staticmethod
     def list_accounting_info():
         """ list accounting info per ip addres, addresses can't overlap in zone's so we just output all we know here
         instead of trying to map addresses back to zones.
         :return: list accounting info per ip address
         """
-        devnull = open(os.devnull, 'w')
         result = dict()
-        with tempfile.NamedTemporaryFile() as output_stream:
-            subprocess.check_call(['/sbin/ipfw', '-aT', 'list'],
-                                  stdout=output_stream,
-                                  stderr=devnull)
-            output_stream.seek(0)
-            for line in output_stream:
-                parts = line.decode().split()
-                if len(parts) > 5:
-                    if 30001 <= int(parts[0]) <= 50000 and parts[4] == 'count':
-                        line_pkts = int(parts[1])
-                        line_bytes = int(parts[2])
-                        last_accessed = int(parts[3])
-                        if parts[7] != 'any':
-                            ip_address = parts[7]
-                        else:
-                            ip_address = parts[9]
-
-                        if ip_address not in result:
-                            result[ip_address] = {'rule': int(parts[0]),
-                                                  'last_accessed': 0,
-                                                  'in_pkts': 0,
-                                                  'in_bytes': 0,
-                                                  'out_pkts': 0,
-                                                  'out_bytes': 0
-                                                  }
-                        result[ip_address]['last_accessed'] = max(result[ip_address]['last_accessed'],
-                                                                  last_accessed)
-                        if parts[7] != 'any':
-                            # count input
-                            result[ip_address]['in_pkts'] = line_pkts
-                            result[ip_address]['in_bytes'] = line_bytes
-                        else:
-                            # count output
-                            result[ip_address]['out_pkts'] = line_pkts
-                            result[ip_address]['out_bytes'] = line_bytes
-
-            return result
-
-    def add_accounting(self, address):
+        sp = subprocess.run(['/sbin/ipfw', '-aT', 'list'], capture_output=True, text=True)
+        for line in sp.stdout.split('\n'):
+            parts = line.split()
+            if len(parts) > 5 and 30000 <= int(parts[0]) <= 50000 and parts[4] == 'count':
+                line_pkts = int(parts[1])
+                line_bytes = int(parts[2])
+                last_accessed = int(parts[3])
+                if parts[7] != 'any':
+                    ip_address = parts[7]
+                else:
+                    ip_address = parts[9]
+
+                if ip_address not in result:
+                    result[ip_address] = {'rule': int(parts[0]),
+                                          'last_accessed': 0,
+                                          'in_pkts': 0,
+                                          'in_bytes': 0,
+                                          'out_pkts': 0,
+                                          'out_bytes': 0
+                                          }
+                result[ip_address]['last_accessed'] = max(result[ip_address]['last_accessed'],
+                                                          last_accessed)
+                if parts[7] != 'any':
+                    # count input
+                    result[ip_address]['in_pkts'] = line_pkts
+                    result[ip_address]['in_bytes'] = line_bytes
+                else:
+                    # count output
+                    result[ip_address]['out_pkts'] = line_pkts
+                    result[ip_address]['out_bytes'] = line_bytes
+
+        return result
+
+    def _add_accounting(self, address):
         """ add ip address for accounting
         :param address: ip address
-        :return: None
+        :return: added or known rule number
         """
         # search for unused rule number
         acc_info = self.list_accounting_info()
@@ -151,29 +148,36 @@ def add_accounting(self, address):
                     rule_ids.append(acc_info[ip_address]['rule'])
 
             new_rule_id = -1
-            for ruleId in range(30001, 50000):
+            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:
-                devnull = open(os.devnull, 'w')
-                subprocess.call(['/sbin/ipfw', 'add', str(new_rule_id), 'count', 'ip', 'from', address, 'to', 'any'],
-                                stdout=devnull, stderr=devnull)
-                subprocess.call(['/sbin/ipfw', 'add', str(new_rule_id), 'count', 'ip', 'from', 'any', 'to', address],
-                                stdout=devnull, stderr=devnull)
-
-    def del_accounting(self, address):
+                subprocess.run(['/sbin/ipfw', 'add', str(new_rule_id), 'count', 'ip', 'from', address, 'to', 'any'],
+                               capture_output=True)
+                subprocess.run(['/sbin/ipfw', 'add', str(new_rule_id), 'count', 'ip', 'from', 'any', 'to', address],
+                               capture_output=True)
+
+                # end of accounting block lives at rule number 50000
+                subprocess.run(
+                    ['/sbin/ipfw', 'add', str(new_rule_id), 'skipto', '60000', 'ip', 'from', 'any', 'to', 'any'],
+                    capture_output=True
+                )
+
+                return new_rule_id
+        else:
+            return acc_info[address]['rule']
+
+    def _del_accounting(self, address):
         """ remove ip address from accounting rules
         :param address: ip address
         :return: None
         """
         acc_info = self.list_accounting_info()
         if address in acc_info:
-            devnull = open(os.devnull, 'w')
-            subprocess.call(['/sbin/ipfw', 'delete', str(acc_info[address]['rule'])],
-                            stdout=devnull, stderr=devnull)
+            subprocess.run(['/sbin/ipfw', 'delete', str(acc_info[address]['rule'])], capture_output=True)
 
     def delete(self, table_number, address):
         """ remove entry from both ipfw table and accounting rules
@@ -182,4 +186,4 @@ def delete(self, table_number, address):
         :return:
         """
         self.delete_from_table(table_number, address)
-        self.del_accounting(address)
+        self._del_accounting(address)

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

@@ -114,8 +114,8 @@ add {{loop.index  + 1000}} skipto 60000 icmp from any to { 255.255.255.255 or me
 # zone {{item.zone}} ({{item.zoneid}}) / {{item.if}} configuration
 #===================================================================================
 {# authenticated clients #}
-add {{3000 + item.zoneid|int }}  skipto {{10001 + item.zoneid|int * 1000  }} ip from table({{item.zoneid|int}}) to any via {{item.if}}
-add {{3000 + item.zoneid|int }}  skipto {{10001 + item.zoneid|int * 1000  }} ip from any to table({{item.zoneid|int}}) via {{item.if}}
+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 %}
 
 
@@ -144,20 +144,8 @@ add 6199 skipto 60000 all from any to any
 
 
 #======================================================================================
-# setup zone accounting section
+# 30000 .... 49999 reserved for captive portal accounting rules
 #======================================================================================
-{% for item in cp_interface_list %}
-# zone {{item.zone}} ({{item.zoneid}})
-add {{ (item.zoneid|int * 1000) + 10001 }} count ip from any to any via {{item.if}}
-add {{ (item.zoneid|int * 1000) + 10998 }} skipto 30000 all from any to any via {{item.if}}
-add {{ (item.zoneid|int * 1000) + 10999 }} deny all from any to any not via {{item.if}}
-{% endfor %}
-
-
-#======================================================================================
-# setup accounting section, first rule is counting all CP traffic
-#======================================================================================
-add 30000 set 0 count ip from any to any
 
 
 #======================================================================================