diff --git a/src/opnsense/scripts/captiveportal/cp-background-process.py b/src/opnsense/scripts/captiveportal/cp-background-process.py index b5f76e652eb..d4fd905d992 100755 --- a/src/opnsense/scripts/captiveportal/cp-background-process.py +++ b/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,11 +207,14 @@ 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) + # 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() diff --git a/src/opnsense/scripts/captiveportal/lib/arp.py b/src/opnsense/scripts/captiveportal/lib/arp.py index efd0ae541bf..163b5502f9d 100755 --- a/src/opnsense/scripts/captiveportal/lib/arp.py +++ b/src/opnsense/scripts/captiveportal/lib/arp.py @@ -88,7 +88,9 @@ def reload(self): 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) diff --git a/src/opnsense/scripts/captiveportal/lib/ipfw.py b/src/opnsense/scripts/captiveportal/lib/ipfw.py index 33f5139cfb7..07eb7b38bb7 100755 --- a/src/opnsense/scripts/captiveportal/lib/ipfw.py +++ b/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 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 @@ 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(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) diff --git a/src/opnsense/service/templates/OPNsense/IPFW/ipfw.conf b/src/opnsense/service/templates/OPNsense/IPFW/ipfw.conf index d393ded80a9..11d5d43d4ff 100644 --- a/src/opnsense/service/templates/OPNsense/IPFW/ipfw.conf +++ b/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 #======================================================================================