Local socket and review fixes - #4
Merged
Merged
Conversation
added 3 commits
September 1, 2026 11:30
A full review of the protocol, both daemons, the ops scripts and the configs. Each of these was silent in normal operation, which is why they had survived. update_dns_blocklist.sh published whatever it managed to download and then restarted the resolver, so a failed fetch left an empty or truncated list in force and applied it - DNS filtering off, with a service restart to make sure. Its sibling update_root_hints.sh already guarded its single download. Now every source is checked, a source with a local copy is reused rather than dropped, and the merged list has to clear an absolute floor and a shrink guard before it replaces the published one. Nothing is restarted unless something was published. db_push applied an undocumented max(ttl, 3600) to every RR TTL. That contradicts the expiry rule in PROTOCOL.md and, with the shipped TTL_MULTIPLIER of 4, held a 60 second answer for four hours; it also raised the ttl of 0 that means do-not-cache, the one value validate.extract goes out of its way to keep. The TTL is now stored as sent. TTL_MULTIPLIER is the only knob for holding entries longer than the record says, and the Redis EXPIRE backstop is floored at one second because Redis reads EXPIRE 0 as delete-now. The circuit breaker never opened. breaker_open() cleared the failure count every time it found the breaker closed, and transmit_all consults it once per query, so the count never passed 1: any BREAKER_FAILURES above 1 - including the shipped default of 3 - left the resolver paying a full SOCKET_TIMEOUT per query for a firewall that was plainly down. Only an elapsed cool-off clears the count now. The breaker also counts acknowledgements rather than sends, so a firewall that completes the handshake and never replies trips it instead of looking healthy. Both daemons defaulted only some of the keys they read, so a config predating an option raised KeyError from inside a query - after Unbound had loaded the module and before ext_state was set. Each now has one load_config with a default for every key it reads, and validation for the ones that cannot have a default. SOCKET_PROTO is normalised and checked, because an unrecognised value matched neither listener branch, fell through to the shutdown path and exited 0 as though it had served; a lowercase 'udp' also slipped past the ALLOW_INSECURE_UDP gate. SOCKET_LISTEN is refused rather than quietly becoming 0.0.0.0 on a port that injects PF whitelist entries without authentication. sync_pf_table ignored pfctl's exit status. A failing pfctl writes nothing, which is indistinguishable from an empty table, so the diff found nothing to expire and tried to re-add every live IP: real entries were never removed and nothing said why. The status is checked and a failed read skips the cycle loudly. file_pop rewrote the persist file with a hardcoded 0640, reverting a tightened permission on every scan and loosening a 0600 file - and these files are PF's whitelist. It now carries over the mode of the file it replaces. The frame layer collapsed a bad length prefix and a truncated payload into one "Bad frame", so two of the refusals PROTOCOL.md documents could never be sent. They are distinct WireError subclasses now, matching what server-c and the shared vectors already separated. In the message layer the shape check ran after the 'kind' check, so a payload that was not a message object at all was reported as a version skew and the branch meant for it was dead code; a payload of JSON null was reported as an empty payload. Shape is checked first, and a well-formed message with no routable records gets its own reason rather than borrowing "Invalid datatype". logger() read every record's trailing bytes as an address, so the debug dump printed a fabricated IPv4 line for any signed answer - the defect read_rr was fixed for, still present in the dump beside it. encode_payload ran per firewall inside transmit_all's loop, so a CARP pair paid for two lz4 passes over identical bytes on the blocking DNS path. Encoded once per reply, and only when there is a firewall to send it to. test_conn_options asserted getsockopt(TCP_NODELAY) == 1. The kernel may report its own flag bits: Darwin returns 4, so the suite was red anywhere but Linux.
The installer carried UNBOUND_BRANCH="branch-1.18.0" as the stable fallback for when the development head would not configure. That fallback could never have worked. git clone --depth 20 implies --single-branch, so the later git checkout had no such ref to find, and upstream prunes its branch-<version> heads anyway - nothing before 1.23 still exists. The failure was silent either way, because configure's exit status was tested with $? after an echo, and make and make install-all then ran regardless of what configure had done. Which Unbound to build is now resolved at install time: the newest release-* tag from the upstream repository, cloned directly with --branch, which does accept a tag and so lands a single-commit clone on it. Release tags are permanent, unlike the branches. UNBOUND_VERSION pins a release or selects master, and every step from clone to install is checked. The resolution lives in client-unbound/tools/unbound_release.sh so the installer and its test cannot disagree about which release they mean; it sorts with awk rather than sort -V, which is a GNU extension OpenBSD does not have. Nothing tested that Unbound still built. No distribution ships it with --with-pythonmodule, so that build belongs to PFUI alone, and upstream pythonmod drift surfaced only when an operator ran the installer and it failed. client-unbound/tests/container builds Unbound with the installer's flag set and runs PFUI_Unbound inside the resulting resolver, against nsd as authoritative for a test zone and a stub firewall on loopback. It checks the resolver loads the module, that a really resolved answer reaches the wire as a valid PFUI message, and that the resolver still answers afterwards. CI runs it against the latest release, and against master for information only, since upstream head being occasionally unstable is what the old fallback existed for. It is Debian rather than OpenBSD, so it does not prove the OpenBSD build. Two things it caught immediately are ones OpenBSD supplies from base and so would have hidden: building from the git tree needs flex and bison, because util/configlexer.c is generated and not committed, and configure --with-pythonmodule looks for 'python' rather than 'python3', which is what makes the installer's symlink step load-bearing. It also settles the assumption read_rr's TTL labelling rested on. Against 1.26.0 and against master, the reply path reports a relative TTL and the cache path an absolute unix timestamp, so 'kind' means what PROTOCOL.md says it means.
When PFUI_Unbound runs on the firewall itself there is no reason to go through loopback TCP. The client opens one connection per DNS answer, so on a single host that costs a handshake, a TIME_WAIT entry on the firewall and a slice of the ephemeral port range for every reply. SOCKET_UNIX binds a local stream listener carrying exactly the framing and the replies TCP does, so no message-level change was needed to speak it. Both listeners can run at once. A CARP node with its own resolver needs precisely that - the firewall on this box over its socket, the peer over the network - which is also why the client now chooses the transport per FIREWALLS entry, with SOCKET: or HOST:, rather than once for the whole resolver. SOCKET_PROTO now describes only the network entries. SOCKET_LISTEN becomes optional, and leaving it out is how a same-host deployment says "local socket only"; it is still never defaulted to 0.0.0.0, and a configuration naming no listener at all is refused rather than starting up serving nothing. The server binds one accept loop per listener and runs each in a thread, rather than polling them together. Each loop already blocked only for SOCKET_TIMEOUT before re-checking for SIGTERM, and the worker pool, the slot semaphore and the shed path are shared and thread-safe, so each accept path is otherwise as it was. Every listener is bound before any is served, so a bind failure is a startup failure rather than a thread that quietly died. There is no packet on this transport, so the pf.conf source restriction does not apply and cannot: the socket's ownership and mode are the whole control on who may inject PF whitelist entries. That is a different security model in a different place, enforced by a different subsystem, and a server serving both is only as restricted as the weaker one. _pfui is a group of its own holding _pfui_firewall and _unbound, rather than reusing either account's group, because membership means "may authorise egress" and that should not be implied by merely running as the resolver, nor acquired by anything later added to _unbound's group for an unrelated reason. install-server-python.sh creates it; install-client-unbound.sh adds _unbound when it finds a firewall installed on the same host. The bind path fails closed throughout, since each of these would otherwise leave the socket reachable by more than intended. The umask is narrowed around bind(), which creates the node, rather than fixing the mode with a chmod afterwards, when it would be connectable by everyone in between. A missing SOCKET_UNIX_GROUP, or a chown or chmod that does not take, exits and unlinks the socket instead of serving on it. A world-writable parent is refused unless it is sticky, because anyone could otherwise replace the socket there whatever mode it has; rc.d creates the directory 0750 to that group, so it gates traversal as well. A socket left by an unclean stop is reclaimed, but only after a probe connect shows nothing is listening, since unlinking a live daemon's socket would leave it running and unreachable. SOCKET_UNIX is length-checked at load because sockaddr_un.sun_path is 104 bytes on OpenBSD, which otherwise surfaces as an AF_UNIX path too long out of bind(). One behavioural difference is worth knowing about. A cache report is sent with blocking=False, so the client has gone by the time the server writes ACKUPDATE. Loopback TCP absorbs that into a buffer nobody reads; a local socket reports EPIPE at once. The tolerance in disconnect() was already there and is now load-bearing on this transport, with a test pinning it. Nothing is lost, because the addresses are installed before the acknowledgement is attempted. The container now configures both transports and asserts the same message arrives on each, byte-identical. Unbound really runs as _unbound there against a really 0660 :_pfui socket, so it exercises the group model rather than only the code.
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.
Local socket and review fixes