From 84c4b43b49fa8feb9ea8be28c5633a52b2ecac1c Mon Sep 17 00:00:00 2001 From: Stephan de Wit Date: Mon, 1 Dec 2025 13:11:37 +0100 Subject: [PATCH] captive portal: re-introduce ipfw for accounting purposes only (#9466) * captive portal: re-introduce ipfw for accounting purposes only * captive portal: review feedback --- plist | 1 + src/opnsense/scripts/captiveportal/allow.py | 1 + .../captiveportal/cp-background-process.py | 81 ++++--------- src/opnsense/scripts/captiveportal/lib/db.py | 71 +++++++++--- .../scripts/captiveportal/lib/ipfw.py | 107 ++++++++++++++++++ src/opnsense/scripts/captiveportal/lib/pf.py | 47 -------- .../scripts/captiveportal/sql/init.sql | 8 +- .../service/templates/OPNsense/IPFW/ipfw.conf | 4 + 8 files changed, 198 insertions(+), 122 deletions(-) create mode 100755 src/opnsense/scripts/captiveportal/lib/ipfw.py diff --git a/plist b/plist index 15cdc7011d..a5d056f271 100644 --- a/plist +++ b/plist @@ -1093,6 +1093,7 @@ /usr/local/opnsense/scripts/captiveportal/lib/arp.py /usr/local/opnsense/scripts/captiveportal/lib/daemonize.py /usr/local/opnsense/scripts/captiveportal/lib/db.py +/usr/local/opnsense/scripts/captiveportal/lib/ipfw.py /usr/local/opnsense/scripts/captiveportal/lib/pf.py /usr/local/opnsense/scripts/captiveportal/listClients.py /usr/local/opnsense/scripts/captiveportal/overlay_template.py diff --git a/src/opnsense/scripts/captiveportal/allow.py b/src/opnsense/scripts/captiveportal/allow.py index 3b7ba37606..add1492dad 100755 --- a/src/opnsense/scripts/captiveportal/allow.py +++ b/src/opnsense/scripts/captiveportal/allow.py @@ -34,6 +34,7 @@ import ujson from lib.db import DB from lib.arp import ARP from lib.pf import PF +from lib.ipfw import IPFW parser = argparse.ArgumentParser() parser.add_argument('--username', help='username', type=str, required=True) diff --git a/src/opnsense/scripts/captiveportal/cp-background-process.py b/src/opnsense/scripts/captiveportal/cp-background-process.py index c2cc3644aa..f55bf58896 100755 --- a/src/opnsense/scripts/captiveportal/cp-background-process.py +++ b/src/opnsense/scripts/captiveportal/cp-background-process.py @@ -40,6 +40,7 @@ from lib import Config from lib.db import DB from lib.arp import ARP from lib.pf import PF +from lib.ipfw import IPFW from lib.daemonize import Daemonize from sqlite3_helper import check_and_repair @@ -57,39 +58,18 @@ class CPBackgroundProcess(object): self.db = DB() self._conf_zone_info = self.cnf.get_zones() - self._accounting_info = {k: {'cur': {}, 'prev': {}, 'reset': False} for k in self.list_zone_ids()} - def list_zone_ids(self): """ return zone numbers """ return self._conf_zone_info.keys() - def get_accounting(self): - """ returns only how much the accounting should add in total - """ - result = {} - for zoneid in self.list_zone_ids(): - self._accounting_info[zoneid]['prev'] = self._accounting_info[zoneid]['cur'] - self._accounting_info[zoneid]['cur'] = PF.list_accounting_info(zoneid) - cur = copy.deepcopy(self._accounting_info[zoneid]['cur'] ) + def _add_client(self, zoneid, ip): + PF.add_to_table(zoneid, ip) + IPFW.add_accounting(ip) - for ip in cur: - # take in/out bytes active time as last access time - cur[ip]['last_accessed'] = max(cur[ip]['in_last_accessed'], cur[ip]['out_last_accessed']) - - if self._accounting_info[zoneid]['reset']: - # anchor has just been synced, we'll add the whole thing outside of this loop - continue - - # pf anchor hasn't yet reset, calculate difference - for key in ['in_pkts', 'in_bytes', 'out_pkts', 'out_bytes']: - if ip in self._accounting_info[zoneid]['prev']: - cur[ip][key] = self._accounting_info[zoneid]['cur'][ip][key] \ - - self._accounting_info[zoneid]['prev'][ip][key] - - result[zoneid] = cur - - return {k: v for subdict in result.values() for k, v in subdict.items()} + def _remove_client(self, zoneid, ip): + PF.remove_from_table(zoneid, ip) + IPFW.del_accounting(ip) def initialize_fixed(self): """ initialize fixed ip / hosts per zone @@ -123,22 +103,22 @@ class CPBackgroundProcess(object): for dbclient in self.db.list_clients(zoneid): if dbclient['authenticated_via'] == '---ip---' \ and dbclient['ipAddress'] not in cpzones[zoneid]['allowedaddresses']: - PF.remove_from_table(zoneid, dbclient['ipAddress']) + self._remove_client(zoneid, dbclient['ipAddress']) self.db.del_client(zoneid, dbclient['sessionId'], 'NAS-Request') elif dbclient['authenticated_via'] == '---mac---' \ and dbclient['macAddress'] not in (a.lower() for a in cpzones[zoneid]['allowedmacaddresses']): if dbclient['ipAddress'] != '': - PF.remove_from_table(zoneid, dbclient['ipAddress']) + self._remove_client(zoneid, dbclient['ipAddress']) self.db.del_client(zoneid, dbclient['sessionId'], 'NAS-Request') - def sync_zone(self, zoneid): + def sync_zone(self, zoneid, registered_addr_accounting): """ Synchronize captiveportal zone. Handles timeouts and administrative changes to this zones sessions """ if zoneid in self._conf_zone_info: # fetch data for this zone cpzone_info = self._conf_zone_info[zoneid] - registered_addresses = PF.list_table(zoneid) + registered_addresses = list(PF.list_table(zoneid)) expected_clients = self.db.list_clients(zoneid) concurrent_users = self.db.find_concurrent_user_sessions(zoneid) @@ -196,19 +176,22 @@ class CPBackgroundProcess(object): if current_ip is not None and db_client['ipAddress'] != current_ip: if db_client['ipAddress'] != '': # remove old ip - PF.remove_from_table(zoneid, db_client['ipAddress']) + self._remove_client(zoneid, db_client['ipAddress']) self.db.update_client_ip(zoneid, db_client['sessionId'], current_ip) - PF.add_to_table(zoneid, current_ip) + self._add_client(zoneid, current_ip) # check session, if it should be active, validate its properties if drop_session_reason is None: - # registered client, but not active or missing accounting according to pf (after reboot) - if cpnet not in registered_addresses: - PF.add_to_table(zoneid, cpnet) + # registered client, but not active in pf or missing accounting according to ipfw (after reboot) + if cpnet and ( + cpnet not in registered_addresses or + cpnet not in registered_addr_accounting + ): + self._add_client(zoneid, cpnet) else: # remove session syslog.syslog(syslog.LOG_NOTICE, drop_session_reason) - PF.remove_from_table(zoneid, cpnet) + self._remove_client(zoneid, cpnet) self.db.del_client(zoneid, db_client['sessionId'], delete_reason) # if there are addresses/networks in the underlying pf table which are not in our administration, @@ -220,20 +203,7 @@ class CPBackgroundProcess(object): address_active = True break if not address_active: - PF.remove_from_table(zoneid, registered_address) - - def sync_accounting(self): - for zoneid in self.list_zone_ids(): - pf_stats = self._accounting_info[zoneid]['cur'] - current_clients = self.db.list_clients(zoneid) - - pf_ips = set(pf_stats.keys()) - db_ips = {entry['ipAddress'] for entry in current_clients} - - self._accounting_info[zoneid]['reset'] = False - if pf_ips != db_ips: - self._accounting_info[zoneid]['reset'] = True - PF.sync_accounting(zoneid) + self._remove_client(zoneid, registered_address) def main(): """ Background process loop, runs as backend daemon for all zones. only one should be active at all times. @@ -259,15 +229,14 @@ def main(): # reload cached arp table contents bgprocess.arp.reload() + accounting_info = IPFW.list_accounting_info() + # process sessions per zone for zoneid in bgprocess.list_zone_ids(): - bgprocess.sync_zone(zoneid) + bgprocess.sync_zone(zoneid, accounting_info) # update accounting info, for all zones - bgprocess.db.update_accounting_info(bgprocess.get_accounting()) - - # sync accounting after db update, resets zone counters if sync is needed - bgprocess.sync_accounting() + bgprocess.db.update_accounting_info(accounting_info) # close the database handle while waiting for the next poll bgprocess.db.close() diff --git a/src/opnsense/scripts/captiveportal/lib/db.py b/src/opnsense/scripts/captiveportal/lib/db.py index c4bdf01f4c..9d480a37fa 100755 --- a/src/opnsense/scripts/captiveportal/lib/db.py +++ b/src/opnsense/scripts/captiveportal/lib/db.py @@ -82,6 +82,24 @@ class DB(object): cur.execute("ALTER TABLE cp_clients ADD COLUMN delete_reason VARCHAR") self._connection.commit() + # migration: ensure prev_* columns are initialized + cur.execute("SELECT count(*) FROM sqlite_master WHERE tbl_name = 'session_info'") + if cur.fetchall()[0][0] > 0: + cur.execute("PRAGMA table_info(session_info)") + # columns exist (from init.sql) but some rows may have NULLs + cur.execute(""" + UPDATE session_info + SET prev_packets_in = COALESCE(prev_packets_in, packets_in), + prev_bytes_in = COALESCE(prev_bytes_in, bytes_in), + prev_packets_out = COALESCE(prev_packets_out, packets_out), + prev_bytes_out = COALESCE(prev_bytes_out, bytes_out) + WHERE prev_packets_in IS NULL + OR prev_bytes_in IS NULL + OR prev_packets_out IS NULL + OR prev_bytes_out IS NULL + """) + self._connection.commit() + cur.close() def sessions_per_address(self, zoneid, ip_address=None, mac_address=None): @@ -273,13 +291,14 @@ class DB(object): return result def update_accounting_info(self, details): - """ update internal accounting database with given pf info (not per zone) - :param details: pf accounting details + """ update internal accounting database with given ipfw info (not per zone) + :param details: ipfw accounting details """ if type(details) == dict: # query registered data - sql = """ select cc.ip_address, cc.zoneid, cc.sessionid, cc.created - , si.rowid si_rowid, si.last_accessed + sql = """ select cc.ip_address, cc.zoneid, cc.sessionid + , si.rowid si_rowid, si.prev_packets_in, si.prev_bytes_in + , si.prev_packets_out, si.prev_bytes_out, si.last_accessed from cp_clients cc left join session_info si on si.zoneid = cc.zoneid and si.sessionid = cc.sessionid order by cc.ip_address, cc.deleted @@ -297,21 +316,27 @@ class DB(object): if prev_record['ip_address'] != record['ip_address'] and record['ip_address'] in details: if record['si_rowid'] is None: # new session, add info object - sql_new = """ insert into session_info(zoneid, sessionid, packets_in, - packets_out, bytes_in, bytes_out, last_accessed) - values (:zoneid, :sessionid, :packets_in, - :packets_out, :bytes_in, :bytes_out, :last_accessed) + sql_new = """ insert into session_info(zoneid, sessionid, prev_packets_in, prev_bytes_in, + prev_packets_out, prev_bytes_out, + packets_in, packets_out, bytes_in, bytes_out, + last_accessed) + values (:zoneid, :sessionid, :packets_in, :bytes_in, :packets_out, :bytes_out, + :packets_in, :packets_out, :bytes_in, :bytes_out, :last_accessed) """ record['packets_in'] = details[record['ip_address']]['in_pkts'] record['bytes_in'] = details[record['ip_address']]['in_bytes'] record['packets_out'] = details[record['ip_address']]['out_pkts'] record['bytes_out'] = details[record['ip_address']]['out_bytes'] - record['last_accessed'] = record['created'] + record['last_accessed'] = details[record['ip_address']]['last_accessed'] cur2.execute(sql_new, record) else: # update session sql_update = """ update session_info set last_accessed = :last_accessed + , prev_packets_in = :prev_packets_in + , prev_packets_out = :prev_packets_out + , prev_bytes_in = :prev_bytes_in + , prev_bytes_out = :prev_bytes_out , packets_in = packets_in + :packets_in , packets_out = packets_out + :packets_out , bytes_in = bytes_in + :bytes_in @@ -319,12 +344,28 @@ class DB(object): where rowid = :si_rowid """ # add usage to session - if details[record['ip_address']]['last_accessed'] != 0: - record['last_accessed'] = details[record['ip_address']]['last_accessed'] - record['packets_in'] = details[record['ip_address']]['in_pkts'] - record['packets_out'] = details[record['ip_address']]['out_pkts'] - record['bytes_in'] = details[record['ip_address']]['in_bytes'] - record['bytes_out'] = details[record['ip_address']]['out_bytes'] + record['last_accessed'] = details[record['ip_address']]['last_accessed'] + if record['prev_packets_in'] <= details[record['ip_address']]['in_pkts'] and \ + record['prev_packets_out'] <= details[record['ip_address']]['out_pkts']: + # ipfw data is still valid, add difference to use + record['packets_in'] = ( + details[record['ip_address']]['in_pkts'] - record['prev_packets_in']) + record['packets_out'] = ( + details[record['ip_address']]['out_pkts'] - record['prev_packets_out']) + record['bytes_in'] = (details[record['ip_address']]['in_bytes'] - record['prev_bytes_in']) + record['bytes_out'] = ( + details[record['ip_address']]['out_bytes'] - record['prev_bytes_out']) + else: + # the data has been reset (reloading rules), add current packet count + record['packets_in'] = details[record['ip_address']]['in_pkts'] + record['packets_out'] = details[record['ip_address']]['out_pkts'] + record['bytes_in'] = details[record['ip_address']]['in_bytes'] + record['bytes_out'] = details[record['ip_address']]['out_bytes'] + + record['prev_packets_in'] = details[record['ip_address']]['in_pkts'] + record['prev_packets_out'] = details[record['ip_address']]['out_pkts'] + record['prev_bytes_in'] = details[record['ip_address']]['in_bytes'] + record['prev_bytes_out'] = details[record['ip_address']]['out_bytes'] cur2.execute(sql_update, record) prev_record = record diff --git a/src/opnsense/scripts/captiveportal/lib/ipfw.py b/src/opnsense/scripts/captiveportal/lib/ipfw.py new file mode 100755 index 0000000000..f4410734e6 --- /dev/null +++ b/src/opnsense/scripts/captiveportal/lib/ipfw.py @@ -0,0 +1,107 @@ +""" + Copyright (c) 2025 Deciso B.V. + All rights reserved. + + Redistribution and use in source and binary forms, with or without + modification, are permitted provided that the following conditions are met: + + 1. Redistributions of source code must retain the above copyright notice, + this list of conditions and the following disclaimer. + + 2. Redistributions in binary form must reproduce the above copyright + notice, this list of conditions and the following disclaimer in the + documentation and/or other materials provided with the distribution. + + THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, + INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY + AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, + OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + POSSIBILITY OF SUCH DAMAGE. + +""" +import os +import subprocess + +class IPFW(object): + @staticmethod + def list_accounting_info(): + """ list accounting info per ip address, 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 + """ + result = dict() + 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]) + ip_address = parts[7] if parts[7] != 'any' else 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 + + @staticmethod + def add_accounting(address): + """ add ip address for accounting + :param address: ip address + :return: added or known rule number + """ + # search for unused rule number + acc_info = IPFW.list_accounting_info() + if address not in acc_info: + 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: + 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) + + return new_rule_id + else: + return acc_info[address]['rule'] + + @staticmethod + def del_accounting(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) diff --git a/src/opnsense/scripts/captiveportal/lib/pf.py b/src/opnsense/scripts/captiveportal/lib/pf.py index 6934ed5557..4b2f0d4665 100755 --- a/src/opnsense/scripts/captiveportal/lib/pf.py +++ b/src/opnsense/scripts/captiveportal/lib/pf.py @@ -51,50 +51,3 @@ class PF(object): # kill associated states to and from this host subprocess.run(['/sbin/pfctl', '-k', f'{address}'], capture_output=True) subprocess.run(['/sbin/pfctl', '-k', '0.0.0.0/0', '-k', f'{address}'], capture_output=True) - - @staticmethod - def sync_accounting(zoneid): - rules = '' - for entry in PF.list_table(zoneid): - rules += f'ether pass in quick proto {{ 0x0800 }} l3 from {entry} to any label "{entry}-in"\n' - rules += f'ether pass out quick proto {{ 0x0800 }} l3 from any to {entry} label "{entry}-out"\n' - - with tempfile.NamedTemporaryFile(mode="w", delete=True) as tmp_file: - tmp_file.write(rules) - tmp_file.flush() - - subprocess.run( - ['/sbin/pfctl', '-a', f'captiveportal_zone_{zoneid}', '-f', tmp_file.name], - text=True, - capture_output=True - ) - - @staticmethod - def list_accounting_info(zoneid): - sp = subprocess.run(['/sbin/pfctl', '-a', f'captiveportal_zone_{zoneid}', '-vvse'], capture_output=True, text=True) - results = {} - stats = {} - prev_line = '' - - for line in sp.stdout.split('\n'): - line = line.strip() - if not line or line[0] != '[': - if prev_line.find(' label ') > -1: - lbl = prev_line.split(' label ')[-1] - if '"' in lbl and lbl.count('"') >= 2: - ip, out_flag = lbl.split('"')[1].split('-') - out = (out_flag == 'out') - stats_key = ('out' if out else 'in') - results.setdefault(ip, {'in_pkts': 0, 'in_bytes': 0, 'out_pkts': 0, 'out_bytes': 0, 'in_last_accessed': 0, 'out_last_accessed': 0}) - results[ip][f'{stats_key}_pkts'] += stats.get('packets', 0) - results[ip][f'{stats_key}_bytes'] += stats.get('bytes', 0) - results[ip][f'{stats_key}_last_accessed'] = stats.get('last_accessed', 0) - prev_line = line - elif line[0] == '[' and line.find('Evaluations') > 0: - parts = line.strip('[ ]').replace(':', ' ').split() - stats.update({parts[i].lower(): int(parts[i+1]) for i in range(0, len(parts)-1, 2) if parts[i+1].isdigit()}) - elif line[0] == '[' and line.find('Last Active Time') > 0: - date_str = line.strip('[ ]').split('Time:')[1].strip() - stats.update({'last_accessed': 0 if date_str == 'N/A' else int(time.mktime(time.strptime(date_str, "%a %b %d %H:%M:%S %Y")))}) - - return results diff --git a/src/opnsense/scripts/captiveportal/sql/init.sql b/src/opnsense/scripts/captiveportal/sql/init.sql index bd2497b318..d8e53c34f5 100755 --- a/src/opnsense/scripts/captiveportal/sql/init.sql +++ b/src/opnsense/scripts/captiveportal/sql/init.sql @@ -22,10 +22,10 @@ create index cp_clients_zone ON cp_clients (zoneid); create table session_info ( zoneid int , sessionid varchar -, prev_packets_in integer -, prev_bytes_in integer -, prev_packets_out integer -, prev_bytes_out integer +, prev_packets_in integer default (0) +, prev_bytes_in integer default (0) +, prev_packets_out integer default (0) +, prev_bytes_out integer default (0) , packets_in integer default (0) , packets_out integer default (0) , bytes_in integer default (0) diff --git a/src/opnsense/service/templates/OPNsense/IPFW/ipfw.conf b/src/opnsense/service/templates/OPNsense/IPFW/ipfw.conf index 767817670e..d393ded80a 100644 --- a/src/opnsense/service/templates/OPNsense/IPFW/ipfw.conf +++ b/src/opnsense/service/templates/OPNsense/IPFW/ipfw.conf @@ -26,6 +26,10 @@ 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 +#====================================================================================== +# 30000 .... 49999 reserved for captive portal accounting rules +#====================================================================================== + #====================================================================================== # traffic shaping section, authorized traffic #======================================================================================