PFUI modernisation - #1
Merged
Merged
Conversation
added 11 commits
August 31, 2026 22:31
Four defects that silently dropped or purged DNS updates, plus the invalid Redis DB default. They land together because they share one wire break. TCP reassembly kept only the last two recv chunks and decoded those, so any answer larger than ~2x SOCKET_BUFFER lost its leading bytes and was dropped. The b"EOT" footer that framing relied on was also an in-band sentinel in arbitrary binary lz4 output: those bytes at a chunk tail truncated a message mid-stream, and a footer split across three chunks was invisible to a two-chunk window. Replace both with a 4-byte big-endian length prefix and an exact-length read, so the byte count is known before anything is buffered. Cache-path Redis keys were written with epoch+expires but no ttl, so the scanner's KeyError left db_ttl at 0, the expiry test collapsed to epoch <= now, and any IP seen only via the cache callback was purged within one SCAN_PERIOD. The magnitude heuristic behind that (ttl < 604800) also misread a genuine week-long RR TTL as an absolute timestamp. The sender knows which it has, so it now labels the message kind and the firewall stores exactly one expiry field, deleting the other: hmset merges, and a key holding both ttl and expires is ambiguous to any rule that branches on field presence. Capping decompressed size after lz4.frame.decompress() returned mitigated nothing, since the full expansion was already allocated. Expand incrementally against a 1 MiB ceiling instead. REDIS_DB defaulted to 1024, outside the valid 0-15 range. redis-py connects lazily, so this did not fail at connect; the daemon started healthy and every later command raised ResponseError. The framing and expiry logic move into a shared pfui/ package so the regression tests exercise the same code the daemons run. Both installers ship it beside their daemon. Wire format is incompatible with earlier releases: deploy PFUI_Unbound and PFUI_Firewall from the same commit, and FLUSHDB the PFUI Redis database, whose existing keys predate the kind field and would be purged on the first scan.
…ilure file_pop read the file and wrote its temp copy before taking flock, so the lock guarded nothing and raced file_push; it then locked the stale read fd and returned None on success while its siblings returned True. Its temp file was named from the current second, so two pops within the same second collided. Both mutators now share one blocking flock on a sidecar .lock file, which is never truncated or renamed and so cannot be lost with the file it guards. The previous flock(LOCK_EX|LOCK_NB) plus sleep() busy-poll quantised every contended acquisition; the kernel already queues waiters. file_push runs per DNS answer with every receiver thread serialised on that lock, so it no longer reads the whole file to filter duplicates: PF loads a table as a set, and file_pop's rewrite collapses duplicates on the next scan. file_pop replaces via mkstemp plus os.replace, restoring 0640 because mkstemp creates 0600 and the replacement would otherwise silently drop group read. pfr_addr_struct assigned a.states, which ctypes accepted as a new attribute rather than the pfra_states field it was meant to set. Harmless because ctypes zero-inits, but it hides real typos, and tests/test_ioctl.py copied it. Drops two imports left unused by the framing change.
Every overridden rc_* function ended with sleep, so it returned sleep's status
and never the daemon's. rcctl start printed (ok) when the daemon had failed to
start, and rcctl stop reported success while it was still running. The daemon's
own CLI exits non-zero on each failure path; the wrapper discarded it. The
sleeps were redundant too, since start already waits and re-checks is_running.
rc.subr applies daemon_user only through ${rcexec}, which the default rc_start
invokes. A bare "$daemon start" therefore ran as whoever called rcctl, so the
firewall daemon has been running as root and the _pfui_firewall declaration had
no effect. Route the verbs through ${rcexec} so the declaration is real.
Because that makes the privilege drop actually happen, the pidfile moves to
/var/run/pfui, created by rc_pre for the daemon user; /var/run itself is
root-owned and writable only via the installer's blanket chmod 775. rc_pre also
re-asserts /dev/pf's group and mode, which OpenBSD's MAKEDEV resets during
release upgrades.
pexp could not match: rc.subr uses pgrep -xf, which anchors on the whole
command line, and that begins with the interpreter from the daemon's shebang
rather than "python".
rcctl reload mapped onto the daemon's reload, which was a full stop/start of
every thread and the listening socket. Deleting rc_reload would be worse than
leaving it, because rc.subr's default signals SIGHUP and the daemon installs no
handler, so Python's default disposition would terminate it. Keep an override
that refuses and returns non-zero, and drop the reload alias from the CLI.
The resolver's script loses its rc_start override entirely: Unbound takes its
config as a flag, so rc.subr's default is already correct, and restoring it
means rcctl reload again sends the SIGHUP Unbound implements as a real reload.
It also had a leftover "echo ${rcexec}" printing on every start, hardcoded the
binary path instead of ${daemon}, and started Redis in rc_pre - on the resolver,
where PFUI's Redis does not run, so a host without Redis installed could not
start the resolver at all.
… state Ingress accepted any address inet_pton could parse, so a DNS answer pointing inside the network was installed into a PF table and passed on egress. All three layers that should have stopped that were incomplete in the same direction: the resolver's private-address list covered no IPv6 and no CGNAT, is_ipv4/is_ipv6 accepted anything parseable, and the example pf.conf's NonRoutable table was IPv4-only. Validation is now a whitelist: only globally routable unicast addresses pass, in canonical form. Multicast needs its own test because ipaddress reports 224.0.0.0/4 and ff00::/8 as global, and IPv4-mapped addresses are refused outright rather than landing in a v6 table. Canonicalising here also settles the IPv6 spelling mismatch that made sync_pf_table delete and re-add the same address forever, and replaces the sentinel string set, which could only ever have matched one spelling of 0.0.0.0 and ::. A ttl of 0 is now kept: it means do-not-cache, and discarding it blocked egress to the address entirely. In examples/pf.conf the non-routable blocks moved above the PFUI pass rules. PF is first-match-wins for quick rules, so as written the passes fired first and the packet-layer backstop for exactly this class of address never ran. The persist files move to a daemon-owned /var/spool/pfui/. Root-owned mode-640 files would not have been enough: file_pop's mkstemp and the sidecar .lock both need a writable directory, which root-owned /var/spool is not, and every write would have failed EACCES silently, leaving pf.conf to preload a stale whitelist after a reboot. The installer migrates existing files into place. Both installers now give root ownership of everything the system executes or a daemon merely reads: the rc.d scripts (rcctl runs them as root), the config files, the resolver module, and the two cron-driven update scripts. A file's owner can always chmod it, so daemon-owned mode-555 was not a barrier - either daemon account could have rewritten its own rc script and gained root at the next start. wheel membership is gone, since it also granted su; /dev/pf carries a dedicated group instead, and the blanket chmod 775 of /var/run is dropped now that the pidfile lives in /var/run/pfui. The firewall installer also stops depending on the caller's working directory, having computed DIR and never used it, defines the HOUR it referenced when backing up an existing config, and exits non-zero when a step failed: the ERR trap only ever set a flag that was printed and discarded.
ScanSync.run held the db.keys() call that actually executes every SCAN_PERIOD and feeds both sync functions; the sites listed in the original review were the scan body and two keys=None fallbacks that run() never reaches. Both address families were issuing a blocking O(N) KEYS against Redis every scan, beside a latency-critical resolver. All four sites now use scan_iter. The sync diffs did list membership per element, which is quadratic in table size. Set differences instead, which also means sync_pf_file collapses the duplicate lines file_push's append-only writes can leave behind. db_push sets a defensive EXPIRE so a dead sync thread cannot grow Redis without bound. It is computed from the message kind, because a cache entry's ttl is an absolute timestamp and one ttl * multiplier expression would have set an expiry roughly 170 years out. The stats block and the eight time() probes it consumes now share one flag. Guarding only the block on LOG_LEVEL == DEBUG while the probes stayed under LOGGING would have raised NameError on stime at the end of every DNS update under the shipped default config, which is LOGGING: False with LOG_LEVEL: DEBUG. The eight log lines collapse into one structured line. UDP transmit retried 40 times at SOCKET_TIMEOUT each, stalling the resolver for ~2 minutes when a firewall was down. Retries stay - they are UDP mode's only delivery guarantee, and one lost datagram otherwise leaves the firewall unaware of an answer until the DNS TTL expires - but the ACK wait is now its own sub-second timeout, and both are configurable. DECISIONS.md records the deferrals and accepted risks: the privilege model's real limits, the unauthenticated transport, pfctl -T show per scan, and why reload refuses instead of restarting.
UDP mode replied ACKDATA on receipt, before the datagram had been decoded or validated, which made the listener a blind reflector for a spoofed source address. The ACK now happens once the message has parsed into a valid PFUI structure. UDP mode also refuses to start unless ALLOW_INSECURE_UDP is set, since a datagram source address is not verified at all; the key has a loader default and a config entry, so an existing deployment cannot KeyError on it. One thread per connection had no ceiling. A ThreadPoolExecutor alone would not have fixed that: its work queue is unbounded, so a flood still holds one accepted fd per queued item until fds or memory run out. Connections are admitted through a bounded semaphore and shed with a close when the pool is full, which is the fail-closed choice, since PF still denies the traffic and the resolver sees an ordinary socket failure rather than an unbounded wait. Task exceptions are logged from a done-callback. Once work runs as Futures the exception is stored on an object nobody inspects, so a recurring receiver failure such as Redis being down would otherwise be completely silent, losing the tracebacks the bare Thread version produced. The resolver gets a per-firewall circuit breaker. With a firewall down, every query paid the full SOCKET_TIMEOUT; after BREAKER_FAILURES consecutive failures that firewall is skipped for BREAKER_COOLOFF seconds. The resolver keeps answering and the packet layer keeps denying, so this trades a latency cliff for nothing worse. transmit_all no longer writes a resolved PORT back into the shared pfui_cfg structure it iterates.
The ctypes structs, IOCTL() and table_push/table_pop move to pfui/pf_ioctl.py, so tests import the same definitions the daemon uses instead of a private copy that had already drifted and asserted nothing. tests/test_ioctl.py loses the hardcoded pydevd_pycharm.settrace() that hung on any machine but its author's, and becomes a pytest module gated on OpenBSD that asserts on the counts the ioctl reports rather than writing to real PF tables and checking nothing. The struct layout checks move to tests/test_pf_structs.py because they are host-independent: pfrio_esize tells the kernel how to read the buffer, so a drift there corrupts every call silently. tests/test_unbound.py and test_unbound_faults.py did not import: they ran config loading at module scope, sent json.dumps() output as str, called an undefined log_err, and did not match the wire format. They are now integration suites that speak the real framing and skip unless PFUI_FW_HOST is set, and the fault suite asserts what must be refused: a message with no kind, sentinel and internal addresses, a bad declared length, a truncated or garbage payload, and a decompression bomb, then checks the daemon still serves afterwards. CI runs the host-independent suites on 3.8 and 3.11, compiles both daemons, and syntax-checks the shell and rc.d scripts. The PF ioctl and live-firewall suites skip there by design and need an OpenBSD host.
…back
The OpenBSD source tarballs were fetched with bare curl, so an HTTP error page
would be saved as ports.tar.gz, and signify's exit status was never checked.
With no set -e the ERR trap only set a flag, so tar unpacked whatever had been
downloaded, as root, over /usr/src and /usr/ports. The trees were also deleted
before any download had succeeded, so a failed fetch left the host with no
sources at all. Download with curl -f, verify all three tarballs, and only then
clear and extract; a failed signature aborts with the trees untouched. The
prompt now spells out what is deleted and wants "yes" rather than "y".
Redis stores the whitelist the sync loop pushes into the PF tables, so by the
persist-file argument it is part of the trusted computing base, yet it was
installed and started with stock settings and never checked. Pin bind to
127.0.0.1, ensure protected-mode, back up the config first, and print the
effective bind rather than assuming it.
Both installers now exit non-zero when a step failed, instead of printing
"completed, but with some errors" and returning success.
The FreeBSD branch installed mismatched packages, never built Unbound (that
block is inside the OpenBSD branch) and then ran the OpenBSD-only tail anyway,
so it could only half-install. It exits with a clear message until the path is
real, which is what README already implies by calling it alpha.
SETPFCONF was parsed and never used, so the pf.conf handling it implied never
happened; drop the parameter and say plainly that /etc/pf.conf is not modified.
pfui_unbound.conf was written three times in a row (install, then cp over it,
then chmod). One install, from examples/, which is the copy Q3 keeps.
The PATH line is printed instead of written: assigned in double quotes, it
expanded ${PATH} at install time and appended a frozen snapshot of the
installer's own PATH to root's .zshrc, .kshrc and .bashrc.
The firewall installer built a virtualenv that nothing ever used, and that venv was the only place the daemon's dependencies were installed. The daemon runs under /usr/local/bin/python3 from its shebang, so redis, service, pyyaml and lz4 were not importable there at all. Install them for the system interpreter from pinned requirements files instead. update_root_hints.sh fetched over plaintext FTP with the signature check commented out, and its success test read $? after two comment lines, so it checked curl only by accident. It now uses HTTPS with -f, tests the command directly, and keeps the previous root.hints on failure rather than promoting an error body. update_dns_blocklist.sh gains -f on every download, notes that the sbc.io fallback is unauthenticated plain HTTP, and drops a hardcoded personal 'grep -v reddit.com' from everyone's blocklist. Both scripts' logger tags named files that had been renamed. README's install steps could not work as written: the scripts were shown bare, with no clone, no ./ and no doas, though they are not on PATH and exit 1 unless root. Its sysctl tuning was given as runtime commands, which do not persist on OpenBSD. Three mutually inconsistent latency figures (~1ms on an i7, ~3ms on an i5, and "~700ns in lab", which cannot cover a round trip plus a PF ioctl) collapse into the one measured range, with how to measure it. The DNS-rebinding assurance now says what actually enforces it. Known Issues named pfui_firewall.sh for a .py file. Debugging.md documented a Redis hash holding epoch, ttl AND expires at once, which is exactly the ambiguity the kind field removed, and taught KEYS * against a firewall's Redis. It now shows the current shape, uses SCAN, covers the moved persist files, and explains that both LOGGING and LOG_LEVEL: DEBUG are needed for timings. INSTALL.md's four renamed-file references are fixed. The TCP sender checked nothing about the firewall's reply, treating any 36 bytes as an acknowledgement; it now reports a refusal, which is what a version skew looks like. Remaining TODO comments become statements of fact, with the deferrals recorded in DECISIONS.md. Deletes the unused Cython scaffold (src/, setup.py), the duplicate repo-root pfui_unbound.conf now that the installer uses examples/, and a scratch UDP client that targeted the wrong port. .gitignore listed three tracked paths, which has no effect on already-committed files, and covered none of __pycache__, .pytest_cache, the venv the installer used to create, .DS_Store or .roo/. .gitattributes referenced a pydevd egg that no longer exists.
…pushes pfr_addr_struct caught a failed inet_pton, logged it, and then returned the zeroed struct it had already built, which means an unparseable address was pushed to the kernel as 0.0.0.0 or ::. It raises instead; table_push already treats an IOCTL failure as a fallback path, so the batch fails closed. Ingress validation makes this unreachable in normal operation, which is exactly why it should be loud if it ever happens. pfctl_add_addr and pfctl_del_addr fell off the end of the function on failure, returning None where their callers document an int, and pfctl_del_addr returned 1 regardless of how many addresses it removed. Both now return a count, 0 on failure, and both entry points say so. Replaces the remaining bare excepts, which swallowed KeyboardInterrupt and SystemExit along with everything else.
pfui_unbound.py is a pythonmod plugin: Unbound executes it in the embedded interpreter's __main__ namespace, injects log_info/log_err and the MODULE_* constants, and calls init, init_standard, deinit, inform_super, operate and inplace_cache_callback itself. Nothing in PFUI calls them, and the config load under `if __name__ == "__main__"` is the plugin's load-time initialisation rather than dead code. Executing a file is not importing it, so __file__ need not be defined in the namespace it runs in. The sys.path entry added for the shared pfui package used realpath(__file__), which would raise NameError while the plugin loaded and take DNS resolution down with it. Derive the directory from CONFIG_LOCATION instead, which is where the installer puts the package and is already the one hardcoded path this module trusts. The new contract tests pin the entry points Unbound calls, since no PFUI code references them and a rename would otherwise surface only on a resolver. One of them execs the source in a namespace with no __file__, because importlib always provides one and so cannot reproduce how Unbound loads this file; it fails against the previous revision.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PFUI modernisation