diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c470722..b8f5de3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,13 +36,34 @@ jobs: - name: Build and run protocol vectors run: make -C server-c test + unbound-container: + # The only job that answers "does Unbound still build with the Python module, + # and does its pythonmod still call PFUI's entry points". Everything else + # tests PFUI against fakes; this builds the Unbound release the installer + # resolves and runs PFUI_Unbound inside it, so upstream API drift surfaces + # here rather than during someone's install. + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + # 'latest' is what the installer builds by default. 'master' is what it + # builds on request, and is informational: upstream head is occasionally + # unstable, which is why the installer can be pinned to a release. + unbound: [latest, master] + continue-on-error: ${{ matrix.unbound == 'master' }} + steps: + - uses: actions/checkout@v4 + - name: Build Unbound with the Python module and run PFUI_Unbound in it + run: ./client-unbound/tests/container/run.sh ${{ matrix.unbound }} + shell: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - name: Syntax-check the installers and ops scripts run: | - for f in *.sh client-unbound/tools/*.sh; do bash -n "$f"; done + for f in *.sh client-unbound/tools/*.sh \ + client-unbound/tests/container/*.sh; do bash -n "$f"; done - name: Syntax-check the rc.d scripts run: | sudo apt-get update -qq && sudo apt-get install -y -qq ksh diff --git a/DECISIONS.md b/DECISIONS.md index cdd793e..ba41205 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -16,6 +16,68 @@ now as a larger change to the daemon's startup path. On OpenBSD `/dev` is a static on-disk directory, so the mode persists across reboots; what resets it is `MAKEDEV` during a release upgrade. +## RR TTLs are stored exactly as sent + +`db_push` records the TTL the client reported, with no floor. An earlier +`max(ttl, 3600)` contradicted the expiry rule in `PROTOCOL.md`, and with the +shipped `TTL_MULTIPLIER: 4` it turned a 60 second answer into four hours of +authorised egress. It also raised the `ttl` of 0 that means do-not-cache, which +`validate.extract` deliberately preserves. + +`TTL_MULTIPLIER` is the only knob for holding entries longer than the record +says, which is the right place for it: the operator sets it knowing why (browsers +caching past the TTL), rather than inheriting a hidden minimum. The Redis +`EXPIRE` written alongside each key stays a backstop for the scan loop and is +floored at one second, because Redis reads `EXPIRE 0` as "delete now". + +## Unbound is built from a release tag, resolved at install time + +`install-client-unbound.sh` asks NLnet Labs for its newest `release-*` tag rather +than carrying a version. Three things follow from that choice: + +Tags, not branches. The `branch-` heads are pruned upstream (nothing +before 1.23 survives), so a version named in the script goes stale and then +vanishes. Release tags are permanent. + +`git ls-remote`, not the GitHub releases API. It needs no token, has no rate +limit, and uses the git the installer already requires. The API would also depend +on a release being marked "latest" by hand. + +Git, not the signed release tarball. The tarball is PGP-signed, which the clone +is not, and that is a real gap. It stays a clone because the OpenBSD build needs +`Makefile.bsd-wrapper` dropped into a source tree, and because `configure` is +committed upstream so no autoreconf is needed. Verifying the source is worth +doing and is not done here. + +## The Unbound build is tested in a Linux container + +`client-unbound/tests/container/` builds Unbound with `--with-pythonmodule` and +runs PFUI_Unbound inside it, on Debian. It cannot prove the OpenBSD build, and it +is not trying to: what it protects is the pythonmod interface. No distribution +ships Unbound with the Python module, so PFUI is the only consumer of that build, +and an upstream API change used to surface as an operator's install failing. + +Two things it caught immediately, both of which the OpenBSD installer gets from +base and would not have revealed: building from the git tree needs flex and +bison, and `configure --with-pythonmodule` looks for `python`, not `python3`. + +It also settles what the TTL labelling in `read_rr` rests 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. That +was previously an assumption. + +Unbound really runs as `_unbound` in the container, and the local socket really is +`0660 :_pfui` under a `0750` directory, so the container also exercises the group +model that permits a same-host deployment rather than just the code paths. + +Known and benign: on shutdown the resolver logs `pythonmod: Exception occurred in +function deinit` / `TypeError: 'NoneType' object cannot be interpreted as an +integer`. It is not PFUI's — it persists with `deinit`'s body reduced to +`return True`, so nothing PFUI executes can be setting it, and it happens after +`service stopped`. The container prints any resolver `error:` line even when every +check passes, which is how this was noticed; it is recorded here so it is not +re-investigated. + ## UDP has a message-size ceiling `UDP_DGRAM_CEILING` (1400 bytes) bounds a PFUI message over UDP. This is not a @@ -25,6 +87,67 @@ message above the ceiling is logged and dropped rather than truncated silently. Accepted because UDP is lab-only and gated behind `ALLOW_INSECURE_UDP`; the fix for a real deployment is TCP. +## A local socket for a same-host deployment + +When PFUI_Unbound runs on the firewall itself, `SOCKET_UNIX` on the server and a +`SOCKET:` entry on the client replace loopback TCP. That is not a micro- +optimisation: the client opens one connection per DNS answer (see below), so +loopback TCP costs a handshake, a `TIME_WAIT` entry on the firewall and a slice of +the ephemeral port range for every reply. A unix socket has none of those. + +Both listeners can run at once, and a CARP node with its own resolver wants +exactly that: the firewall on this box over the socket, the peer over TCP. That is +also why the transport is chosen **per `FIREWALLS` entry** on the client rather +than by one global `SOCKET_PROTO` — one resolver genuinely needs both at the same +time. `SOCKET_PROTO` now describes only the network entries. + +The server binds one accept loop per listener, in a thread each, rather than +polling them together. Each loop already blocks 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 this leaves each accept path as it was. + +### The filesystem is the access control + +There is no packet on this transport, so the `pf.conf` source restriction does not +apply and cannot. The socket's own ownership and mode are the whole control on who +may inject PF whitelist entries, which is a meaningfully different security model +from the network listener's, in a different place, enforced by a different +subsystem. A server serving both is only as restricted as the weaker one. + +`_pfui` is a dedicated group holding `_pfui_firewall` and `_unbound`, rather than +reusing either account's own group. Membership means "may authorise egress", and +that should not be implied by merely running as the resolver, or be acquired by +anything later added to `_unbound`'s group for an unrelated reason. + +The bind path fails closed throughout, because every one of these leaves the +socket reachable by more than intended: + +- `bind()` creates the node with the process umask, so the umask is narrowed + around it rather than the mode being fixed by a `chmod` afterwards. Otherwise + the socket is connectable by everyone for the moment in between. +- A missing `SOCKET_UNIX_GROUP`, or a `chown`/`chmod` that does not take, exits + the daemon and unlinks the socket instead of serving on it. +- A world-writable parent directory is refused unless it is sticky: anyone could + otherwise replace the socket and be handed the resolver's messages, whatever + mode the socket itself has. The parent is `0750 _pfui_firewall:_pfui`, so it is + a second gate in front of the socket. +- A socket file left by an unclean stop is removed, but only after a probe + connect shows nothing is listening. Unlinking a live daemon's socket would + leave it running and unreachable. + +`SOCKET_UNIX` is length-checked at config load because `sockaddr_un.sun_path` is +104 bytes on OpenBSD; without it the failure is an `AF_UNIX path too long` from +`bind()` rather than a named configuration error. + +### EPIPE is visible here where TCP hid it + +A cache report is sent with `blocking=False`, so the client has closed by the time +the server writes `ACKUPDATE`. Loopback TCP absorbs that write into a buffer +nobody reads; a unix socket reports `EPIPE` at once. The tolerance in +`disconnect()` was already there and is now load-bearing on this transport, and +there is a test for it. Nothing is lost: the addresses are installed before the +acknowledgement is attempted. + ## Unauthenticated transport Neither the TCP nor the UDP transport authenticates or encrypts. IPs must reach diff --git a/INSTALL.md b/INSTALL.md index eb1f4e1..35cf750 100644 --- a/INSTALL.md +++ b/INSTALL.md @@ -46,6 +46,19 @@ OpenBSD PF Firewall(s); Configure PFUI_Firewall `/etc/pfui_firewall.yml` Unbound DNS Resolver(s); Configure PFUI_Unbound `/var/unbound/etc/pfui_unbound.yml` ``` +### Transports +The firewall needs at least one listener, and may run both: + +| Config | Serves | +|--------|--------| +| `SOCKET_LISTEN` + `SOCKET_PROTO` | Resolvers on other hosts. Restrict the port in `pf.conf` to the known resolvers. | +| `SOCKET_UNIX` | A resolver on **this** host. No `pf.conf` rule; group `_pfui` and the socket's `0660` mode are the access control. | + +`SOCKET_LISTEN` is never defaulted to `0.0.0.0` — leaving it out is how a +same-host deployment says "local socket only". On the resolver, each `FIREWALLS` +entry picks its own transport with `HOST:` or `SOCKET:`. See +[Same-host deployment](README.md#samehost). + Warning; UDP mode (`SOCKET_PROTO: UDP`) is _not_ recommended (experimental) as Unbound's Python Module executes every DNS lookup, using a unique network socket to PFUI_Firewall for each lookup. With UDP's default timers, the socket remains (5mins) after the connection/PFUI_Firewall is updated, thus blocking subsequent connections until timeout. @@ -83,10 +96,25 @@ pkg_add -i swig git bash cmake libconfig libiconv bison gawk mawk python3 -m pip install -r ./client-unbound/requirements.txt ### PFUI_Unbound - Download Unbound source +Clone the release you mean to build, rather than the default branch. `--branch` +accepts a tag, so a single-commit clone lands directly on it; cloning shallowly +and then checking out another ref does not work, because `--depth` implies +`--single-branch`. +``` +# The tag the installer would resolve, newest release first +git ls-remote --tags --refs https://github.com/NLnetLabs/unbound.git 'release-*' \ + | sed 's#.*refs/tags/##' | grep -E '^release-[0-9.]+$' | sort -t. -k2,2n -k3,3n | tail -1 + +git clone --depth 1 --branch release-1.26.0 https://github.com/NLnetLabs/unbound.git /tmp/unbound +``` +`client-unbound/tools/unbound_release.sh` does this resolution for the installer, +and prints the ref it picked: ``` -git clone --depth 20 https://github.com/NLnetLabs/unbound.git /tmp/unbound -# --depth 20 helps with shallow clone errors +./client-unbound/tools/unbound_release.sh # latest release tag +./client-unbound/tools/unbound_release.sh master # passes through unchanged ``` +Note that `release-*` tags are permanent but the `branch-*` heads are not: NLnet +Labs prunes them, and nothing before 1.23 still exists. #### Default Unbound build options in OpenBSD port, `unbound -V` (ref only); ``` diff --git a/README.md b/README.md index bd73921..79933ef 100644 --- a/README.md +++ b/README.md @@ -91,8 +91,20 @@ adds latency). A fully recursive DNS query can take tens to hundreds of millisec ------ ## PFUI Installation -Tested from OpenBSD 7.0, Unbound 1.16, Python 3.8, -up to OpenBSD 7.4, Unbound 1.18, Python 3.10 +Tested on OpenBSD from 7.0 (Unbound 1.16, Python 3.8) to 7.4 (Unbound 1.18, Python 3.10). + +`install-client-unbound.sh` builds the **latest Unbound release** by default: it +resolves the newest `release-*` tag from NLnet Labs at install time rather than +carrying a version in the script. Every release from 1.23 onwards is a candidate, +so pin one if you need a known build: + +``` +UNBOUND_VERSION=release-1.25.2 doas ./install-client-unbound.sh # a specific release +UNBOUND_VERSION=master doas ./install-client-unbound.sh # upstream head +``` + +Unbound 1.26.0 with `--with-pythonmodule` is built and exercised on every commit; +see [Tests](#tests). ### 1) Install PFUI_Firewall on OpenBSD PF Firewall(s) ``` @@ -223,6 +235,68 @@ Jordan's unbound-adblock installation guide for reference;\ https://www.geoghegan.ca/pub/unbound-adblock/latest/install/openbsd.txt \ https://www.geoghegan.ca/pub/pf-badhost/latest/man/man.txt +------ + +### Tests; + +``` +pytest # protocol, client and server suites +make -C server-c test # the C framing against the shared vectors +./client-unbound/tests/container/run.sh # builds Unbound and runs PFUI_Unbound in it +``` + +The PF ioctl suites skip unless they are run on OpenBSD, and +`client-unbound/tests/test_unbound*.py`'s live cases skip unless `PFUI_FW_HOST` +points at a running PFUI_Firewall. + +The container is the one test that builds Unbound from source with +`--with-pythonmodule` and runs the real resolver against a local authoritative +server and a stub firewall. No distribution packages Unbound with the Python +module, so that build is PFUI's alone, and pythonmod API drift in a new Unbound +release would otherwise only show up when someone ran the installer. It takes a +few minutes and needs Docker; nothing runs on the host. CI runs it against the +latest release, and against upstream `master` for information only. + +------ + +### Same-host deployment (local socket); + +When PFUI_Unbound and PFUI_Firewall run on the **same machine**, the resolver can +reach the firewall over a unix domain socket instead of loopback TCP. The client +opens one connection per DNS answer, so loopback TCP costs a handshake, a +`TIME_WAIT` entry on the firewall and an ephemeral port for every reply; a local +socket costs none of them, and needs no `pf.conf` rule because there is no packet +to filter. + +On the firewall, in `/etc/pfui_firewall.yml`: +``` +SOCKET_UNIX: /var/run/pfui/pfui_firewall.sock +SOCKET_UNIX_GROUP: _pfui +# SOCKET_LISTEN may be omitted entirely if no remote resolver needs to reach this +# firewall. Keep it to serve both, which is what a CARP node wants. +``` +On the resolver, in `/var/unbound/etc/pfui_unbound.yml`: +``` +FIREWALLS: + - SOCKET: /var/run/pfui/pfui_firewall.sock # this host, over the local socket + - HOST: 10.10.1.253 # the CARP peer, over the network + PORT: 10001 +``` +The transport is per entry, so one resolver can use both at once. `SOCKET_PROTO` +applies only to the `HOST` entries. + +**Access control moves from PF to the filesystem.** The socket is `0660`, owned by +group `_pfui`, inside a directory only that group may traverse, and the resolver's +account (`_unbound`) must be a member — so membership of `_pfui` is what authorises +injecting PF whitelist entries. Both installers manage the group: +`install-server-python.sh` creates it, and `install-client-unbound.sh` adds +`_unbound` to it when it finds a firewall installed on the same host. **Unbound +must be restarted** for a new group membership to take effect; a resolver that is +not in the group fails to connect with `EACCES`. + +If the firewall is on a different machine, none of this applies: use `HOST` and +restrict the listening port in `pf.conf` as before. + ------ ### Compatibility; diff --git a/client-unbound/pfui_unbound.py b/client-unbound/pfui_unbound.py index 995abad..8474648 100644 --- a/client-unbound/pfui_unbound.py +++ b/client-unbound/pfui_unbound.py @@ -36,6 +36,7 @@ from socket import ( AF_INET, AF_INET6, + AF_UNIX, IPPROTO_TCP, SO_SNDBUF, SOCK_DGRAM, @@ -106,10 +107,14 @@ def logger(qstate): log_info("rrsig") log_info("") log_info(f"HEX: {data_to_hex(str(d.rr_data[j]))}") - if rk.type_str == "A": - log_info(f"IPv4: {inet_ntop(AF_INET, d.rr_data[j][-4:])}") - if rk.type_str == "AAAA": - log_info(f"IPv6: {inet_ntop(AF_INET6, d.rr_data[j][-16:])}") + # Only the first d.count records hold addresses; the rest are + # signatures. Reading their trailing bytes as an address printed + # a fabricated one, the same defect read_rr was fixed for + if j < d.count: + if rk.type_str == "A": + log_info(f"IPv4: {inet_ntop(AF_INET, d.rr_data[j][-4:])}") + if rk.type_str == "AAAA": + log_info(f"IPv6: {inet_ntop(AF_INET6, d.rr_data[j][-16:])}") log_info("-" * 100) @@ -238,7 +243,7 @@ def udp_transmit_close(data, ip, port, blocking): # transmit pf firewall data reply = udp_transmit(soc, data, ip, port, int(pfui_cfg["UDP_RETRY"])) - breaker_record(ip, port, ok=(reply == b"ACKDATA")) + breaker_record(f"{ip}:{port}", ok=(reply == b"ACKDATA")) # wait for pf firewall update if blocking: # Wait for secondary ACKUPDATE @@ -254,24 +259,35 @@ def udp_transmit_close(data, ip, port, blocking): soc.close() -# Per-firewall circuit breaker state: {(ip, port): [consecutive_failures, open_until]} +# Per-firewall circuit breaker state: {target: [consecutive_failures, open_until]} +# Keyed on the target's label rather than an (ip, port) pair, so a local socket +# path is as much a distinct destination as an address and port are _breakers = {} -def breaker_open(ip, port): - """True while this firewall is in its cool-off window.""" - state = _breakers.get((ip, port)) +def breaker_open(target): + """True while this firewall is in its cool-off window. + + A closed breaker keeps its failure count. Clearing it on every check, which is + what this did, reset the count once per query and so held it at 1 forever: with + any BREAKER_FAILURES above 1 the breaker could never trip, and every query kept + paying the full timeout for a firewall that was plainly down. Only an elapsed + cool-off clears the count, which is the one case that means "probe again". + """ + state = _breakers.get(target) if not state: return False + if not state[1]: # Closed and counting failures + return False if state[1] > time(): return True state[0], state[1] = 0, 0 # Cool-off elapsed, probe again return False -def breaker_record(ip, port, ok): +def breaker_record(target, ok): """Count consecutive failures and open the breaker once the threshold trips.""" - state = _breakers.setdefault((ip, port), [0, 0]) + state = _breakers.setdefault(target, [0, 0]) if ok: state[0], state[1] = 0, 0 return @@ -279,102 +295,191 @@ def breaker_record(ip, port, ok): if state[0] >= int(pfui_cfg["BREAKER_FAILURES"]): state[1] = time() + float(pfui_cfg["BREAKER_COOLOFF"]) log_err( - f"PFUIDNS: {ip}:{port} unreachable {state[0]}x, skipping it for " + f"PFUIDNS: {target} unreachable {state[0]}x, skipping it for " f"{pfui_cfg['BREAKER_COOLOFF']}s (PF still denies the traffic)" ) -def tcp_transmit_close(data, ip, port, blocking): - conn = socket(AF_INET, SOCK_STREAM) +def stream_transmit_close(data, family, address, target, blocking): + """Send one framed message over a stream socket and wait for the ACK. + + Shared by the TCP and the local-socket paths: both carry the same + length-prefixed frame and get the same reply, so only the socket family, the + address and the latency controls differ. + """ + conn = socket(family, SOCK_STREAM) conn.settimeout(pfui_cfg["SOCKET_TIMEOUT"]) - conn.setsockopt( - IPPROTO_TCP, TCP_NODELAY, True - ) # Disable Nagle - conn.setsockopt( - SOL_SOCKET, SO_REUSEADDR, True - ) # Fast Socket reuse - # No SO_SNDBUF here: setting it to 0 does not mean "send immediately", the - # kernel clamps it to its minimum, and a small send buffer only adds syscalls - # and blocking on large messages. TCP_NODELAY above is the latency control. + if family == AF_INET: + conn.setsockopt( + IPPROTO_TCP, TCP_NODELAY, True + ) # Disable Nagle + conn.setsockopt( + SOL_SOCKET, SO_REUSEADDR, True + ) # Fast Socket reuse + # No SO_SNDBUF here: setting it to 0 does not mean "send immediately", the + # kernel clamps it to its minimum, and a small send buffer only adds syscalls + # and blocking on large messages. TCP_NODELAY above is the latency control. + # A local socket needs neither: there is no Nagle to disable and no + # TIME_WAIT to reuse, which is most of why it is faster than loopback TCP. sent = False try: - conn.connect((ip, port)) + conn.connect(address) conn.sendall(frame(data)) sent = True - breaker_record(ip, port, ok=True) except TIMEOUT: - breaker_record(ip, port, ok=False) + breaker_record(target, ok=False) log_err( - "PFUIDNS: TCP Socket Timeout to firewall! Check pfui_firewall is running." + f"PFUIDNS: Socket Timeout to firewall {target}! Check pfui_firewall is running." ) - except ERROR: - breaker_record(ip, port, ok=False) + except ERROR as e: + breaker_record(target, ok=False) log_err( - "PFUIDNS: TCP Socket Error! Check pfui_firewall is running." + f"PFUIDNS: Socket Error to firewall {target} ({e})! Check pfui_firewall " + f"is running, and that this user may write the socket if it is local." ) except Exception as e: - breaker_record(ip, port, ok=False) - log_err(f"PFUIDNS: Unknown TCP Socket Exception! {e}") - + breaker_record(target, ok=False) + log_err(f"PFUIDNS: Unknown Socket Exception to {target}! {e}") + + # The breaker counts a successful acknowledgement, not a successful send. A + # firewall that completes the handshake and then never replies looked healthy + # to the old accounting, so the breaker never opened and every subsequent + # query paid SOCKET_TIMEOUT in full, forever. A refusal counts as a failure + # too: the firewall is reachable but is not whitelisting anything, and PF + # keeps denying the traffic either way. try: if blocking and sent: # Nothing to acknowledge if the send failed reply = conn.recv(36) # Wait for pfui_firewall to ACK + breaker_record(target, ok=(reply == b"ACKUPDATE")) if reply != b"ACKUPDATE": # The firewall replies with a reason when it refuses a message, # e.g. a version skew that leaves the wire format mismatched log_err( - f"PFUIDNS: {ip}:{port} did not confirm the update: {reply!r}" + f"PFUIDNS: {target} did not confirm the update: {reply!r}" ) + elif sent: + # Non-blocking: delivery is the only thing observable from here + breaker_record(target, ok=True) except TIMEOUT: + breaker_record(target, ok=False) log_err( - "PFUIDNS: Timeout waiting for pfui_firewall ACK." + f"PFUIDNS: Timeout waiting for pfui_firewall ACK from {target}." ) except Exception as e: - log_err(f"PFUIDNS: Unknown TCP Socket Exception while reading! {e}") + breaker_record(target, ok=False) + log_err(f"PFUIDNS: Unknown Socket Exception while reading {target}! {e}") finally: conn.close() +def tcp_transmit_close(data, ip, port, blocking): + stream_transmit_close( + data=data, + family=AF_INET, + address=(ip, int(port)), + target=f"{ip}:{port}", + blocking=blocking, + ) + + +def unix_transmit_close(data, path, blocking): + """Send to a PFUI_Firewall on this same host over its local socket. + + There is no PF rule guarding this transport, because there is no packet: the + permissions on the socket are the whole access control, so a connect() that + fails with EACCES means this resolver's user is not in the firewall's + SOCKET_UNIX_GROUP. + """ + stream_transmit_close( + data=data, + family=AF_UNIX, + address=path, + target=path, + blocking=blocking, + ) + + +def firewall_target(fw): + """How to reach one FIREWALLS entry, or None if it names no firewall. + + Returns (kind, target, address). 'target' is the label the circuit breaker and + the logs key on, so it must identify one destination uniquely. + + An entry carries either SOCKET, for a PFUI_Firewall on this same host, or + HOST, for one reached over the network with SOCKET_PROTO. Per entry rather + than per resolver, because a CARP node runs both at once: the local firewall + over its socket, and the peer over TCP. + """ + path = fw.get("SOCKET") + if path: + return "UNIX", str(path), str(path) + host = fw.get("HOST") + if not host: + return None + # 'or' rather than a get() default: a 'PORT:' left empty in the yml parses as + # None, which is not the same as absent + port = int(fw.get("PORT") or pfui_cfg["DEFAULT_PORT"]) + return pfui_cfg["SOCKET_PROTO"], f"{host}:{port}", (host, port) + + def transmit_all(pfui_dict, blocking=True): """PFUI: Transmits IP and TTL data to PF Firewalls running pfui_firewall.""" if pfui_cfg["LOGGING"]: start = time() + # Encoded once for all firewalls, and only if there is one to send to: the + # bytes are identical per destination, and this runs on the blocking DNS path + # where a second lz4 pass per firewall was pure duplicated work + pfui_data = None + for fw in pfui_cfg["FIREWALLS"]: - if fw["HOST"]: - port = fw.get("PORT", pfui_cfg["DEFAULT_PORT"]) # Do not mutate pfui_cfg - if breaker_open(fw["HOST"], port): - log_info( - f"PFUIDNS: Skipping {fw['HOST']}:{port}, circuit breaker open" - ) - continue - if pfui_cfg["LOGGING"]: - log_info(f"PFUIDNS: Sending '{pfui_dict}' to {fw['HOST']}:{port}") + destination = firewall_target(fw) + if destination is None: + continue + kind, target, address = destination + + if breaker_open(target): + log_info(f"PFUIDNS: Skipping {target}, circuit breaker open") + continue + if pfui_cfg["LOGGING"]: + log_info(f"PFUIDNS: Sending '{pfui_dict}' to {target}") - # JSON, optionally lz4 compressed. TCP adds a length prefix below + # JSON, optionally lz4 compressed. Both stream transports add a length + # prefix below; only UDP sends the payload alone + if pfui_data is None: pfui_data = encode_payload(pfui_dict, compress=pfui_cfg["COMPRESS"]) - # TODO Update Multiple Firewalls in parallel (test Thread setup performance vs serial send) - # Serial today: with BLOCKING each firewall's full round trip is added - # to the query. The circuit breaker keeps an unreachable one from - # contributing its timeout, but a healthy CARP pair still doubles the - # block time. + # TODO Update Multiple Firewalls in parallel (test Thread setup performance vs serial send) + # Serial today: with BLOCKING each firewall's full round trip is added + # to the query. The circuit breaker keeps an unreachable one from + # contributing its timeout, but a healthy CARP pair still doubles the + # block time. - if pfui_cfg["SOCKET_PROTO"] == "UDP": - udp_transmit_close( + if kind == "UNIX": + unix_transmit_close(data=pfui_data, path=address, blocking=blocking) + elif kind == "UDP": + udp_transmit_close( data=pfui_data, - ip=fw["HOST"], - port=port, - blocking=blocking + ip=address[0], + port=address[1], + blocking=blocking, ) - elif pfui_cfg["SOCKET_PROTO"] == "TCP": - tcp_transmit_close( + elif kind == "TCP": + tcp_transmit_close( data=pfui_data, - ip=fw["HOST"], - port=port, - blocking=blocking + ip=address[0], + port=address[1], + blocking=blocking, + ) + else: + # Unreachable: the config load validates this. Kept so a proto + # that reaches here is loud, rather than silently whitelisting + # nothing while the resolver looks perfectly healthy + log_err( + f"PFUIDNS: SOCKET_PROTO '{pfui_cfg['SOCKET_PROTO']}' is not TCP " + f"or UDP; {target} was not told about {pfui_dict}" ) if pfui_cfg["LOGGING"]: @@ -516,26 +621,83 @@ def operate(id, event, qstate, qdata): return True +# Every key this module reads, with the value assumed when the yml omits it. +# Partial defaults meant a config predating an option raised KeyError from inside +# a query - after Unbound had already loaded the module and before ext_state was +# set - so the resolver failed per lookup rather than at start. +CONFIG_DEFAULTS = { + "LOGGING": True, + "LOG_LEVEL": "ERROR", + "COMPRESS": True, + "SOCKET_PROTO": "TCP", + "SOCKET_TIMEOUT": 3, + "BLOCKING": True, + "UDP_RETRY": 3, + "UDP_ACK_TIMEOUT": 0.5, + "BREAKER_FAILURES": 3, + "BREAKER_COOLOFF": 30, + "DEFAULT_PORT": 10001, + "FIREWALLS": [], # Nothing to send to; warned about below rather than guessed +} + + +def load_config(location=CONFIG_LOCATION): + """Read the yml, apply the defaults, and reject a config that cannot work. + + Raises ValueError on a SOCKET_PROTO the transmit path does not implement, + which would otherwise leave the resolver answering normally while telling no + firewall anything. + """ + cfg = safe_load(open(location)) or {} + for key, value in CONFIG_DEFAULTS.items(): + cfg.setdefault(key, value) + cfg["SOCKET_PROTO"] = str(cfg["SOCKET_PROTO"]).strip().upper() + if cfg["SOCKET_PROTO"] not in ("TCP", "UDP"): + raise ValueError( + f"SOCKET_PROTO must be TCP or UDP, not '{cfg['SOCKET_PROTO']}'" + ) + + # SOCKET_PROTO governs the network entries only. A SOCKET entry is always a + # local stream socket carrying the same frames, so UDP does not apply to it. + for index, fw in enumerate(cfg["FIREWALLS"] or []): + if not isinstance(fw, dict): + raise ValueError(f"FIREWALLS[{index}] is not a mapping: {fw!r}") + socket_path, host = fw.get("SOCKET"), fw.get("HOST") + if socket_path and host: + raise ValueError( + f"FIREWALLS[{index}] sets both SOCKET ({socket_path}) and HOST " + f"({host}); one firewall is reached one way or the other" + ) + if socket_path and not str(socket_path).startswith("/"): + raise ValueError( + f"FIREWALLS[{index}] SOCKET must be an absolute path, " + f"not '{socket_path}'" + ) + # An entry with neither is skipped by firewall_target rather than + # rejected: a commented-out placeholder left with an empty HOST has + # always been tolerated, and refusing to load would break configs that + # work today. It is worth saying out loud, though + if not socket_path and not host: + log_err( + f"PFUIDNS: FIREWALLS[{index}] names neither SOCKET nor HOST " + f"({fw}); it will be skipped" + ) + return cfg + + if __name__ == "__main__": try: - pfui_cfg = safe_load(open(CONFIG_LOCATION)) - if "SOCKET_PROTO" not in pfui_cfg: - pfui_cfg["SOCKET_PROTO"] = "TCP" - if "BLOCKING" not in pfui_cfg: - pfui_cfg["BLOCKING"] = True - if "UDP_RETRY" not in pfui_cfg: - pfui_cfg["UDP_RETRY"] = 3 - if "UDP_ACK_TIMEOUT" not in pfui_cfg: - pfui_cfg["UDP_ACK_TIMEOUT"] = 0.5 - if "BREAKER_FAILURES" not in pfui_cfg: - pfui_cfg["BREAKER_FAILURES"] = 3 - if "BREAKER_COOLOFF" not in pfui_cfg: - pfui_cfg["BREAKER_COOLOFF"] = 30 - + pfui_cfg = load_config() except Exception as e: log_err( f"PFUIDNS: Yaml Config File (pfui_unbound.yml) not found or cannot load: {e}" ) exit(1) + if not pfui_cfg["FIREWALLS"]: + log_err( + "PFUIDNS: No FIREWALLS configured; resolved addresses will not be " + f"whitelisted anywhere. Add them to {CONFIG_LOCATION}" + ) + log_info("PFUIDNS: python module for Unbound loaded.") diff --git a/client-unbound/pfui_unbound.yml b/client-unbound/pfui_unbound.yml index 24c781d..723f168 100644 --- a/client-unbound/pfui_unbound.yml +++ b/client-unbound/pfui_unbound.yml @@ -15,11 +15,22 @@ BREAKER_COOLOFF: 30 # Seconds to skip an unreachable firewall before probing COMPRESS: True # Compress and Decompress PFUI_Unbound->PFUI_Firewall data DEFAULT_PORT: 10001 # Default port where PFUI_Firewall is listening + +# Each entry names one PFUI_Firewall, reached either over the network (HOST, with +# SOCKET_PROTO above) or over a local socket (SOCKET) when it runs on THIS host. +# One entry, one transport: set HOST or SOCKET, not both. FIREWALLS: # List of Firewall Hosts targeting OpenBSD PF Firewalls running PFUI_Firewall - HOST: 127.0.0.1 # Inside IP of PF Firewall running PFUI_Firewall PORT: 10001 # Port for PFUI_Firewall running on PF Firewall # - HOST: # IP of second PF Firewall running PFUI_Firewall (Eg, CARP Host) # PORT: # Port for second PFUI_Firewall running on PF Firewall +# Same-host deployment: PFUI_Firewall on this machine, over its local socket. +# Faster than loopback TCP and needs no pf.conf rule, but the resolver's user must +# be in the firewall's SOCKET_UNIX_GROUP (see install-client-unbound.sh). +# - SOCKET: /var/run/pfui/pfui_firewall.sock + # When operating PF Firewall Clusters using CARP, both firewall nodes should be configured as PF Table data is # NOT synced by PF-Sync # TODO Verify since recent CARP re-write +# On a CARP node running its own resolver, that means one SOCKET entry for the +# firewall on this box and one HOST entry for the peer. diff --git a/client-unbound/tests/container/Dockerfile b/client-unbound/tests/container/Dockerfile new file mode 100644 index 0000000..3e9a1f6 --- /dev/null +++ b/client-unbound/tests/container/Dockerfile @@ -0,0 +1,106 @@ +# PFUI_Unbound build-and-run test container. +# +# Builds Unbound from source with --with-pythonmodule, the same release and the +# same flags install-client-unbound.sh uses, then runs PFUI_Unbound inside that +# resolver against a local authoritative server and a stub PFUI_Firewall. +# +# This is Debian, not OpenBSD, so it does not prove the OpenBSD build. What it +# does prove is the part a version bump risks and that nothing else covers: +# that the Unbound release the installer resolves still configures and builds +# with the Python module, that its pythonmod interface still calls PFUI's entry +# points, and that a real resolved answer still reaches the wire as the message +# PFUI_Firewall expects. Before this existed, "does Unbound still build" was +# only ever answered on an operator's machine, by the install failing. +# +# Build context is the repository root; see run.sh. + +FROM debian:bookworm-slim + +# Which Unbound to build: "latest", "master", or a tag such as release-1.25.2. +# Resolved by the same helper the installer calls, so the two cannot disagree. +ARG UNBOUND_VERSION=latest + +ENV DEBIAN_FRONTEND=noninteractive + +# python3-yaml and python3-lz4 come from apt rather than pip because the resolver +# imports them through its embedded interpreter, which is the system one - the +# same reason the installer does not use a virtualenv +# +# flex and bison are needed because this builds from the git repository, where +# util/configlexer.c is generated rather than committed. OpenBSD has lex and yacc +# in base, which is why install-client-unbound.sh does not name them. +RUN apt-get update && apt-get install -y --no-install-recommends \ + bison \ + build-essential \ + ca-certificates \ + flex \ + git \ + libevent-dev \ + libexpat1-dev \ + libssl-dev \ + python3 \ + python3-dev \ + python3-lz4 \ + python3-yaml \ + swig \ + nsd \ + dnsutils \ + && rm -rf /var/lib/apt/lists/* + +# Mirrors the OpenBSD account Unbound drops to after binding, and the shared group +# install-server-python.sh creates for the local PFUI socket. Unbound really does +# run as _unbound here, so the resolver only reaches the socket if that membership +# and the socket's mode are right - the same thing an operator depends on +RUN id _unbound >/dev/null 2>&1 || useradd -r -s /usr/sbin/nologin _unbound \ + && groupadd -f _pfui \ + && usermod -aG _pfui _unbound + +# configure --with-pythonmodule looks for 'python', not 'python3', and fails +# outright without it. install-client-unbound.sh makes the same symlink, so this +# also keeps that step honest +RUN ln -sf "$(command -v python3)" /usr/local/bin/python + +COPY client-unbound/tools/unbound_release.sh /src/tools/unbound_release.sh +RUN UNBOUND_REF="$(/src/tools/unbound_release.sh "${UNBOUND_VERSION}")" \ + && echo "${UNBOUND_REF}" > /src/unbound-ref \ + && echo "Building Unbound ${UNBOUND_REF}" \ + && git clone --depth 1 --branch "${UNBOUND_REF}" \ + https://github.com/NLnetLabs/unbound.git /src/unbound + +# The installer's flag set, less the flags that name OpenBSD-only paths. +# --with-pythonmodule is the one that matters: it is not enabled in any distro +# package, so a pythonmod build break is invisible until someone installs PFUI. +RUN cd /src/unbound \ + && ./configure --enable-allsymbols \ + --with-ssl=/usr \ + --with-libevent=/usr \ + --with-libexpat=/usr \ + --with-pythonmodule \ + --with-chroot-dir=/var/unbound \ + --with-pidfile="" \ + --with-rootkey-file=/var/unbound/db/root.key \ + --with-conf-file=/var/unbound/etc/pfui_unbound.conf \ + --with-username=_unbound \ + --disable-shared \ + --disable-explicit-port-randomisation \ + --without-pthreads \ + && make -j"$(nproc)" \ + && make install + +# PFUI_Unbound, laid out as install-client-unbound.sh lays it out: the module and +# the shared protocol module beside the config, which is what pfui_unbound.py's +# sys.path insertion depends on +RUN install -d -m 755 /var/unbound/etc /var/unbound/db +COPY protocol/python/pfui_wire.py /var/unbound/etc/pfui_wire.py +COPY client-unbound/pfui_unbound.py /var/unbound/etc/pfui_unbound.py +COPY client-unbound/tests/container/pfui_unbound.yml /var/unbound/etc/pfui_unbound.yml +COPY client-unbound/tests/container/unbound.conf /var/unbound/etc/pfui_unbound.conf +RUN chmod 644 /var/unbound/etc/* + +# Authoritative source for the test zone, so a query really resolves +COPY client-unbound/tests/container/nsd.conf /etc/nsd/nsd.conf +COPY client-unbound/tests/container/pfui-test.zone /etc/nsd/pfui-test.zone + +COPY client-unbound/tests/container/verify.py /src/verify.py + +CMD ["python3", "/src/verify.py"] diff --git a/client-unbound/tests/container/nsd.conf b/client-unbound/tests/container/nsd.conf new file mode 100644 index 0000000..6f30552 --- /dev/null +++ b/client-unbound/tests/container/nsd.conf @@ -0,0 +1,19 @@ +# Authoritative server for the test zone, so the resolver has something real to +# resolve. Nothing here is PFUI: it exists only so that an answer reaches the +# iterator and so MODULE_EVENT_MODDONE fires the way it does in production. +# A local-zone in Unbound would not do: those are answered before the module +# chain runs, so PFUI would never see them. + +server: + ip-address: 127.0.0.1@5354 + do-ip6: no + username: "" + zonesdir: "/etc/nsd" + database: "" # Zones are read from the zonefile, no nsd.db needed + pidfile: "/tmp/nsd.pid" + logfile: "/tmp/nsd.log" + verbosity: 1 + +zone: + name: "pfui-test." + zonefile: "pfui-test.zone" diff --git a/client-unbound/tests/container/pfui-test.zone b/client-unbound/tests/container/pfui-test.zone new file mode 100644 index 0000000..96965e8 --- /dev/null +++ b/client-unbound/tests/container/pfui-test.zone @@ -0,0 +1,14 @@ +; Test zone for the PFUI_Unbound container. The addresses are globally routable +; on purpose: a PFUI server drops anything else, so a private address here would +; make the test pass while proving nothing about a real deployment. +$TTL 60 +$ORIGIN pfui-test. +@ IN SOA ns.pfui-test. hostmaster.pfui-test. ( 1 3600 900 604800 60 ) +@ IN NS ns.pfui-test. +ns IN A 127.0.0.1 +www IN A 8.8.8.8 +www IN AAAA 2001:4860:4860::8888 +dual IN A 1.1.1.1 +dual IN AAAA 2606:4700:4700::1111 +many IN A 9.9.9.9 +many IN A 149.112.112.112 diff --git a/client-unbound/tests/container/pfui_unbound.yml b/client-unbound/tests/container/pfui_unbound.yml new file mode 100644 index 0000000..585bf6c --- /dev/null +++ b/client-unbound/tests/container/pfui_unbound.yml @@ -0,0 +1,22 @@ +--- # Yaml + +# PFUI_Unbound configuration for the build-and-run test container. +# The stub PFUI_Firewall verify.py starts listens on 127.0.0.1:10001. + +# DEBUG so the container also exercises the RR debug dump, which runs only at +# this level and is therefore never covered by an ordinary deployment +LOGGING: True +LOG_LEVEL: DEBUG + +SOCKET_TIMEOUT: 3 +COMPRESS: True # Exercises the lz4 path, as a production install does +BLOCKING: True # The resolver waits for the stub's ACKUPDATE + +DEFAULT_PORT: 10001 +# Both transports at once, which is what a CARP node with a local resolver runs: +# the firewall on this host over its socket, a remote one over TCP. verify.py +# stands in for both and checks the same message reaches each. +FIREWALLS: + - SOCKET: /var/run/pfui/pfui_firewall.sock + - HOST: 127.0.0.1 + PORT: 10001 diff --git a/client-unbound/tests/container/run.sh b/client-unbound/tests/container/run.sh new file mode 100755 index 0000000..d903a98 --- /dev/null +++ b/client-unbound/tests/container/run.sh @@ -0,0 +1,31 @@ +#!/usr/bin/env bash +# +# Builds Unbound with Python module support and runs PFUI_Unbound inside it. +# The build takes a few minutes; nothing here touches the host. +# +# ./client-unbound/tests/container/run.sh # latest release +# ./client-unbound/tests/container/run.sh release-1.25.2 # a pinned tag +# ./client-unbound/tests/container/run.sh master # upstream head +# +# The build context is the repository root, because the container needs both the +# client and the shared protocol module. + +set -eu + +UNBOUND_VERSION=${1:-latest} +HERE="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" +ROOT="$( cd "${HERE}/../../.." >/dev/null 2>&1 && pwd )" +IMAGE="pfui-unbound-test:${UNBOUND_VERSION}" + +echo "Building ${IMAGE} from ${ROOT} (Unbound ${UNBOUND_VERSION})" +docker build \ + --build-arg "UNBOUND_VERSION=${UNBOUND_VERSION}" \ + -f "${HERE}/Dockerfile" \ + -t "${IMAGE}" \ + "${ROOT}" + +echo +echo "Running PFUI_Unbound inside it" +# Loopback only: nsd, the resolver and the stub firewall all talk to each other +# inside the container, so nothing is published to the host +docker run --rm "${IMAGE}" diff --git a/client-unbound/tests/container/unbound.conf b/client-unbound/tests/container/unbound.conf new file mode 100644 index 0000000..90e272b --- /dev/null +++ b/client-unbound/tests/container/unbound.conf @@ -0,0 +1,49 @@ +# Unbound configuration for the PFUI_Unbound build-and-run test container. +# +# Deliberately close to examples/pfui_unbound.conf where it matters - the module +# chain, chroot and the Python stanza - and stripped everywhere it does not, so a +# failure points at PFUI rather than at a tuning option. + +server: + # PFUI_Unbound does not support a chroot'ed environment + chroot: "" + directory: "/var/unbound/etc" + username: "_unbound" + pidfile: "" + verbosity: 2 + use-syslog: no + logfile: "" + + interface: 127.0.0.1 + port: 5353 + do-ip4: yes + do-ip6: no + do-udp: yes + do-tcp: yes + + access-control: 127.0.0.0/8 allow + + # The authoritative server for the test zone is on loopback, which Unbound + # refuses to query by default + do-not-query-localhost: no + + # The test zone is unsigned and outside the root, and no trust anchor is + # configured, so the validator treats everything as insecure + domain-insecure: "pfui-test." + + # One query must produce one lookup, so the assertions can be exact + qname-minimisation: no + prefetch: no + cache-min-ttl: 0 + rrset-roundrobin: no + minimal-responses: yes + + # PFUI is invoked by the python module after the iterator finishes + module-config: "validator python iterator" + +python: + python-script: "/var/unbound/etc/pfui_unbound.py" + +forward-zone: + name: "pfui-test." + forward-addr: 127.0.0.1@5354 diff --git a/client-unbound/tests/container/verify.py b/client-unbound/tests/container/verify.py new file mode 100644 index 0000000..3e85127 --- /dev/null +++ b/client-unbound/tests/container/verify.py @@ -0,0 +1,369 @@ +#!/usr/bin/env python3 +"""Runs PFUI_Unbound inside the Unbound this container built. + +The checks the Dockerfile cannot make on its own: + + * the resolver loads the module at all (a pythonmod API change shows up here, + as a resolver that refuses to start) + * a real resolved answer reaches the wire as a valid PFUI message + * `kind` matches the shape of the TTL it labels: relative seconds on the + reply path, an absolute timestamp on the cache path + * both transports carry it: a local unix socket and TCP, configured per + firewall in one config, which is the same-host and CARP-node arrangement + * the resolver keeps answering after the exchange + +Everything runs inside the container: nsd is authoritative for pfui-test., +Unbound forwards to it, and the two stubs below stand in for a PFUI_Firewall on +this host and one across the network. +""" + +import grp +import os +import socket +import stat +import subprocess +import sys +import threading +import time + +# Where the installer puts the shared protocol module, and where pfui_unbound.py +# looks for it +sys.path.insert(0, "/var/unbound/etc") +from pfui_wire import read_frame # noqa: E402 + +UNBOUND = "/usr/local/sbin/unbound" +RESOLVER = ("127.0.0.1", 5353) +AUTHORITATIVE = ("127.0.0.1", 5354) +STUB = ("127.0.0.1", 10001) +# Must match the SOCKET entry in pfui_unbound.yml, and the mode the real server +# binds it with, so the resolver's connect() is exercised against the same +# permissions a deployment would have +STUB_SOCKET = "/var/run/pfui/pfui_firewall.sock" +STUB_SOCKET_MODE = 0o660 +# The group install-server-python.sh creates and puts _unbound in. Unbound drops +# to _unbound before any query, so this membership is what the resolver's +# connect() actually depends on +STUB_SOCKET_GROUP = "_pfui" +COMPRESS = True # Must match COMPRESS in pfui_unbound.yml + +# A relative DNS TTL cannot plausibly be a unix timestamp, and vice versa. The +# gap between them is what makes the two kinds distinguishable at all. +TIMESTAMP_FLOOR = 1_000_000_000 + + +class StubFirewall(threading.Thread): + """Accepts PFUI messages and acknowledges them, recording what arrived. + + With no arguments it listens on TCP, standing in for a firewall across the + network; given a path it listens on a unix socket, standing in for one on this + host. The framing and the reply are identical either way, which is the point. + """ + + daemon = True + + def __init__(self, path=None): + super().__init__() + self.path = path + if path: + gid = grp.getgrnam(STUB_SOCKET_GROUP).gr_gid + parent = os.path.dirname(path) + os.makedirs(parent, exist_ok=True) + # As rc.d/pfui_firewall sets it up: only the group may traverse to the + # socket, which is a second gate in front of the socket's own mode + os.chown(parent, -1, gid) + os.chmod(parent, 0o750) + if os.path.exists(path): + os.unlink(path) + self.listener = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + previous_umask = os.umask(0o177) # Never briefly wider, as the daemon does + try: + self.listener.bind(path) + finally: + os.umask(previous_umask) + os.chown(path, -1, gid) + os.chmod(path, STUB_SOCKET_MODE) + else: + self.listener = socket.socket() + self.listener.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + self.listener.bind(STUB) + self.listener.listen(64) + self.listener.settimeout(0.5) + self.messages = [] + self.errors = [] + self.unacked = 0 # Peers gone before the ACK; see the EPIPE note below + self.stop = threading.Event() + + def run(self): + while not self.stop.is_set(): + try: + conn, _ = self.listener.accept() + except socket.timeout: + continue + except OSError: + return + conn.settimeout(3) + try: + def recv_exactly(n): + buf = bytearray() + while len(buf) < n: + chunk = conn.recv(n - len(buf)) + if not chunk: + return None + buf += chunk + return bytes(buf) + + message = read_frame(recv_exactly, compress=COMPRESS) + if message is not None: + self.messages.append(message) + try: + conn.sendall(b"ACKUPDATE") + except BrokenPipeError: + # Expected, and only on the local socket: a cache report is + # sent with blocking=False, so the resolver has already + # closed by now. Loopback TCP absorbs the write into a + # buffer nobody reads; a unix socket reports EPIPE at once. + # PFUI_Firewall's disconnect() ignores it for exactly this + # reason, so this is not a fault to record as one + self.unacked += 1 + except Exception as exc: # Recorded, never raised into accept() + self.errors.append(repr(exc)) + finally: + conn.close() + + def close(self): + self.stop.set() + self.listener.close() + if self.path and os.path.exists(self.path): + os.unlink(self.path) + + def for_qname(self, qname): + return [m for m in self.messages if m.get("qname") == qname] + + +def wait_for_dns(server, name, seconds=30): + """True once `server` answers for `name`.""" + host, port = server + deadline = time.time() + seconds + while time.time() < deadline: + probe = subprocess.run( + ["dig", f"@{host}", "-p", str(port), name, "A", "+time=1", "+tries=1"], + capture_output=True, + ) + if probe.returncode == 0 and b"status: NOERROR" in probe.stdout: + return True + time.sleep(0.5) + return False + + +def dig(name, rrtype): + """Answer addresses for one query, as text.""" + host, port = RESOLVER + result = subprocess.run( + ["dig", f"@{host}", "-p", str(port), name, rrtype, "+short", "+time=3"], + capture_output=True, + ) + return [ + line for line in result.stdout.decode().split("\n") if line.strip() + ] + + +class Checks: + def __init__(self): + self.failures = [] + + def that(self, condition, description): + print(f"{'PASS' if condition else 'FAIL'} {description}", flush=True) + if not condition: + self.failures.append(description) + + +def main(): + with open("/src/unbound-ref") as f: + built = f.read().strip() + version = subprocess.run([UNBOUND, "-V"], capture_output=True) + print(f"Unbound ref built: {built}") + print(version.stdout.decode().strip(), flush=True) + + checks = Checks() + checks.that( + b"with-pythonmodule" in version.stdout or b"pythonmodule" in version.stdout, + "the built resolver reports the Python module in its configure line", + ) + + checkconf = subprocess.run( + ["/usr/local/sbin/unbound-checkconf", "/var/unbound/etc/pfui_unbound.conf"], + capture_output=True, + ) + checks.that( + checkconf.returncode == 0, + f"unbound-checkconf accepts the PFUI config {checkconf.stderr.decode().strip()}", + ) + if checkconf.returncode != 0: + return 1 + + # Two firewalls, one per transport, as pfui_unbound.yml's FIREWALLS names them + stub = StubFirewall() + stub.start() + local = StubFirewall(path=STUB_SOCKET) + local.start() + + nsd = subprocess.Popen(["nsd", "-d", "-c", "/etc/nsd/nsd.conf"], + stdout=subprocess.PIPE, stderr=subprocess.STDOUT) + resolver = None + try: + if not wait_for_dns(AUTHORITATIVE, "www.pfui-test.", seconds=20): + print("FAIL nsd never answered for the test zone") + print((nsd.stdout.read() or b"").decode()) + return 1 + print("PASS the authoritative server answers for the test zone", flush=True) + + resolver = subprocess.Popen([UNBOUND, "-d"], stdout=subprocess.PIPE, + stderr=subprocess.STDOUT) + if not wait_for_dns(RESOLVER, "www.pfui-test.", seconds=30): + print("FAIL the resolver never answered; the module did not load") + resolver.terminate() + print((resolver.stdout.read() or b"").decode()[-8000:]) + return 1 + print("PASS the resolver loaded pfui_unbound.py and answers queries", + flush=True) + + # A name not yet in the cache: the iterator resolves it, MODDONE fires, + # and the module reports a relative RR TTL + answers = dig("dual.pfui-test.", "A") + checks.that(answers == ["1.1.1.1"], f"the answer resolves ({answers})") + + time.sleep(2) # The message is sent during the query; allow for the ACK + fresh = stub.for_qname("dual.pfui-test.") + checks.that(bool(fresh), "a PFUI message arrived for the resolved name") + if fresh: + message = fresh[0] + print(f" reply-path message: {message}", flush=True) + checks.that(message["kind"] == "rr", + "a freshly resolved answer is labelled kind 'rr'") + checks.that( + [r["ip"] for r in message["AF4"]] == ["1.1.1.1"], + "the resolved address is the one on the wire", + ) + checks.that( + all(0 <= r["ttl"] < TIMESTAMP_FLOOR for r in message["AF4"]), + "an 'rr' TTL is a relative TTL, not a timestamp", + ) + checks.that( + all("qname" not in r for r in message["AF4"]), + "qname is carried once per message, not once per record", + ) + + # The same name again is answered from the cache, which is a different + # call point (inplace_cache_callback) reporting a different kind of TTL + before = len(stub.for_qname("dual.pfui-test.")) + dig("dual.pfui-test.", "A") + time.sleep(2) + cached = [ + m for m in stub.for_qname("dual.pfui-test.")[before:] + ] + checks.that(bool(cached), "a cache hit also reports to the firewall") + if cached: + message = cached[0] + print(f" cache-path message: {message}", flush=True) + checks.that(message["kind"] == "cache", + "a cache hit is labelled kind 'cache'") + checks.that( + all(r["ttl"] >= TIMESTAMP_FLOOR for r in message["AF4"]), + f"a 'cache' TTL is an absolute expiry timestamp " + f"({[r['ttl'] for r in message['AF4']]})", + ) + + # Several addresses in one reply must arrive as one message, per the + # protocol's one-message-per-reply rule + dig("many.pfui-test.", "A") + time.sleep(2) + multi = stub.for_qname("many.pfui-test.") + checks.that(len(multi) >= 1, "a multi-address answer reported") + if multi: + addresses = sorted(r["ip"] for r in multi[0]["AF4"]) + checks.that( + addresses == ["149.112.112.112", "9.9.9.9"], + f"both addresses from one reply arrive in one message ({addresses})", + ) + + # AAAA records travel in AF6 + dig("www.pfui-test.", "AAAA") + time.sleep(2) + v6 = [m for m in stub.for_qname("www.pfui-test.") if m.get("AF6")] + checks.that(bool(v6), "an AAAA answer reported") + if v6: + checks.that( + [r["ip"] for r in v6[0]["AF6"]] == ["2001:4860:4860::8888"], + f"the IPv6 address is reported in AF6 ({v6[0]['AF6']})", + ) + + # The local socket carries the same messages as TCP, from the same + # resolver and the same config, chosen per FIREWALLS entry + checks.that( + bool(local.messages), + f"the firewall on this host was told over {STUB_SOCKET}", + ) + if local.messages and stub.messages: + checks.that( + sorted(m["qname"] for m in local.messages) + == sorted(m["qname"] for m in stub.messages), + "both transports carried the same set of replies", + ) + local_fresh = local.for_qname("dual.pfui-test.") + checks.that( + bool(local_fresh) and bool(fresh) and local_fresh[0] == fresh[0], + "the message over the socket is identical to the one over TCP", + ) + checks.that( + stat.S_IMODE(os.stat(STUB_SOCKET).st_mode) == STUB_SOCKET_MODE, + f"the socket the resolver connected to is {oct(STUB_SOCKET_MODE)}, " + f"not world-writable", + ) + checks.that( + local.errors == [], f"no malformed messages over the socket {local.errors}" + ) + # Informational, not a pass/fail: it depends on how many cache reports the + # run happened to make, and the resolver is right not to wait for those + print( + f" non-blocking reports whose ACK found the peer gone: " + f"socket={local.unacked} tcp={stub.unacked}", + flush=True, + ) + + checks.that(stub.errors == [], f"no malformed messages arrived {stub.errors}") + checks.that( + dig("www.pfui-test.", "A") == ["8.8.8.8"], + "the resolver still answers after the whole exchange", + ) + checks.that(resolver.poll() is None, "the resolver is still running") + finally: + for process in (resolver, nsd): + if process is not None: + process.terminate() + try: + process.wait(timeout=10) + except subprocess.TimeoutExpired: + process.kill() + stub.close() + local.close() + + resolver_log = (resolver.stdout.read() or b"").decode() if resolver else "" + for line in resolver_log.splitlines(): + # Surfaced unconditionally: the resolver logging an error while every + # check passes is exactly the case that would otherwise go unnoticed + if "error:" in line: + print(f" resolver error: {line}", flush=True) + + if checks.failures: + print(f"\nResolver output:\n{resolver_log[-8000:]}") + print(f"\n{len(checks.failures)} check(s) failed:") + for failure in checks.failures: + print(f" - {failure}") + return 1 + + print(f"\nAll checks passed against Unbound {built}") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/client-unbound/tests/test_blocklist_update.py b/client-unbound/tests/test_blocklist_update.py new file mode 100644 index 0000000..17970fd --- /dev/null +++ b/client-unbound/tests/test_blocklist_update.py @@ -0,0 +1,196 @@ +"""update_dns_blocklist.sh must never publish a list it did not really download. + +The script writes the file Unbound loads as its domain filter and then restarts +the resolver. A failed download used to leave an empty or truncated list in place +and restart anyway, so a bad network turned DNS filtering off - silently, and +with a fresh service start to make it take effect. Its sibling +update_root_hints.sh guarded its single download; this one guards every source +and the merged result. + +The script is exercised with stub curl/rcctl/chown on PATH, so no network, +no root, and no OpenBSD are needed. +""" + +import os +import subprocess +from pathlib import Path + +import pytest + +SCRIPT = Path(__file__).resolve().parent.parent / "tools" / "update_dns_blocklist.sh" + +CURL_STUB = r"""#!/usr/bin/env bash +# Stub curl: writes ${CURL_LINES} lines unless the URL matches ${CURL_FAIL}. +dest="" +url="" +while [ $# -gt 0 ]; do + case "$1" in + -o) dest="$2"; shift 2 ;; + -*) shift ;; + *) url="$1"; shift ;; + esac +done +echo "curl $url -> $dest" >> "${CURL_LOG}" +if [ -n "${CURL_FAIL:-}" ]; then + case "$url" in + *${CURL_FAIL}*) exit 22 ;; # What curl -f does on an HTTP error + esac +fi +lines=${CURL_LINES:-1200} +awk -v n="$lines" 'BEGIN { for (i = 0; i < n; i++) print "0.0.0.0 bad" i ".example.com" }' \ + > "$dest" +""" + +RECORDER_STUB = r"""#!/usr/bin/env bash +echo "$(basename "$0") $*" >> "${CURL_LOG}" +exit 0 +""" + + +@pytest.fixture +def harness(tmp_path): + """A stubbed environment; returns a runner and the paths it writes.""" + etc = tmp_path / "etc" + etc.mkdir() + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + log = tmp_path / "calls.log" + log.write_text("") + + (bin_dir / "curl").write_text(CURL_STUB) + for name in ("rcctl", "chown"): + (bin_dir / name).write_text(RECORDER_STUB) + for name in ("curl", "rcctl", "chown"): + os.chmod(bin_dir / name, 0o755) + + published = etc / "dns_blocklist" + + def run(*args, **env): + environment = dict(os.environ) + environment.update( + { + "PATH": f"{bin_dir}:{environment['PATH']}", + "ETC": str(etc), + "CURL_LOG": str(log), + } + ) + environment.update({k: str(v) for k, v in env.items()}) + return subprocess.run( + ["bash", str(SCRIPT), *args], + capture_output=True, + env=environment, + timeout=120, + ) + + class Harness: + def __init__(self): + self.run = run + self.etc = etc + self.published = published + + def calls(self): + return log.read_text() + + def lines(self): + return [ + l for l in published.read_text().splitlines() if l + ] if published.exists() else None + + return Harness() + + +def test_a_good_run_publishes_and_restarts(harness): + result = harness.run() + assert result.returncode == 0, result.stderr.decode() + assert harness.lines(), "nothing was published" + assert "rcctl restart pfui_unbound" in harness.calls() + + +def test_norestart_publishes_without_restarting(harness): + assert harness.run("norestart").returncode == 0 + assert harness.lines() + assert "rcctl" not in harness.calls() + + +def test_a_failed_source_falls_back_to_the_mirror(harness): + """The StevenBlack lists have a second URL; using it is not a failure.""" + result = harness.run(CURL_FAIL="raw.githubusercontent.com") + assert result.returncode == 0, result.stderr.decode() + assert "sbc.io" in harness.calls() + assert harness.lines() + + +def test_a_source_that_cannot_be_downloaded_at_all_aborts(harness): + """No previous copy to fall back on, so there is nothing to publish.""" + result = harness.run(CURL_FAIL="hosts") + assert result.returncode != 0 + assert harness.lines() is None, "published a list built from a failed download" + assert "rcctl" not in harness.calls(), "restarted the resolver anyway" + + +def test_a_source_that_fails_today_is_reused_from_yesterday(harness): + """A source that has a local copy is not a reason to abort or to publish a + list missing its domains: the copy is reused and the run completes.""" + assert harness.run("norestart").returncode == 0 + before = harness.lines() + + result = harness.run("norestart", CURL_FAIL="pgl.yoyo.org") + assert result.returncode == 0, result.stderr.decode() + assert harness.lines() == before, "reusing a cached source lost domains" + + +def test_a_previously_published_list_survives_a_failed_run(harness): + """The case that matters: yesterday's filter must stay in force. Nothing is + cached here, so every source really is unavailable.""" + assert harness.run("norestart").returncode == 0 + before = harness.lines() + for stale in harness.etc.iterdir(): + if stale.name != "dns_blocklist": + stale.unlink() + + result = harness.run(CURL_FAIL="hosts") + assert result.returncode != 0 + assert harness.lines() == before, "a failed run replaced a working blocklist" + assert "rcctl" not in harness.calls(), "restarted the resolver anyway" + + +def test_an_implausibly_short_list_is_not_published(harness): + """A feed that answers with a handful of lines is a broken feed, not a world + where only three domains are malicious.""" + result = harness.run(CURL_LINES=3, MIN_ENTRIES=1000) + assert result.returncode != 0 + assert harness.lines() is None + assert "rcctl" not in harness.calls() + + +def test_a_list_that_collapses_is_not_published(harness): + """Each source answered, so nothing failed outright, but the merged result + lost most of its entries.""" + assert harness.run("norestart", CURL_LINES=1200).returncode == 0 + before = harness.lines() + + result = harness.run("norestart", CURL_LINES=500, MIN_ENTRIES=100, MIN_PERCENT=50) + assert result.returncode != 0 + assert harness.lines() == before + assert b"shrank" in result.stderr + + +def test_a_modest_shrink_is_still_published(harness): + """Blocklists do lose entries; only a collapse is treated as a fault.""" + assert harness.run("norestart", CURL_LINES=1200).returncode == 0 + before = len(harness.lines()) + + assert harness.run("norestart", CURL_LINES=1000).returncode == 0 + assert len(harness.lines()) < before + + +def test_an_empty_download_is_treated_as_a_failure(harness): + result = harness.run(CURL_LINES=0) + assert result.returncode != 0 + assert harness.lines() is None + + +def test_no_staging_files_are_left_behind(harness): + harness.run("norestart", CURL_LINES=3) # Refused + leftovers = [p.name for p in harness.etc.iterdir() if ".new." in p.name] + assert leftovers == [], f"left {leftovers} in place" diff --git a/client-unbound/tests/test_unbound_module.py b/client-unbound/tests/test_unbound_module.py index 04a1183..bfec101 100644 --- a/client-unbound/tests/test_unbound_module.py +++ b/client-unbound/tests/test_unbound_module.py @@ -14,10 +14,16 @@ import importlib.util import inspect +import shutil +import socket import sys +import tempfile +import time from pathlib import Path +from threading import Event, Thread import pytest +import yaml COMPONENT = Path(__file__).resolve().parent.parent @@ -228,6 +234,490 @@ def sendto(self, *a): assert plugin.udp_transmit(Soc(), b"x", "127.0.0.1", 10001, retry=0) is None +def test_shipped_config_needs_no_defaults(plugin): + """Every key load_config defaults must also be a key the shipped yml sets or + deliberately omits, so the example config and the code cannot drift.""" + shipped = yaml.safe_load((COMPONENT / "pfui_unbound.yml").read_text()) + cfg = plugin.load_config(COMPONENT / "pfui_unbound.yml") + for key in plugin.CONFIG_DEFAULTS: + assert key in cfg + assert cfg["FIREWALLS"] == shipped["FIREWALLS"] + + +def test_every_key_read_at_runtime_has_a_default(plugin, tmp_path): + """A config predating an option must load. Each of these keys is read on the + query path, where a KeyError surfaces per lookup rather than at start.""" + minimal = tmp_path / "pfui_unbound.yml" + minimal.write_text("--- # Yaml\nFIREWALLS:\n - HOST: 127.0.0.1\n") + cfg = plugin.load_config(minimal) + for key in ( + "LOGGING", + "LOG_LEVEL", + "COMPRESS", + "SOCKET_PROTO", + "SOCKET_TIMEOUT", + "BLOCKING", + "UDP_RETRY", + "UDP_ACK_TIMEOUT", + "BREAKER_FAILURES", + "BREAKER_COOLOFF", + "DEFAULT_PORT", + ): + assert key in cfg, f"{key} is read at runtime but has no default" + + +def test_empty_config_file_loads(plugin, tmp_path): + """safe_load returns None for an empty document, which used to be indexed.""" + empty = tmp_path / "pfui_unbound.yml" + empty.write_text("") + assert plugin.load_config(empty)["FIREWALLS"] == [] + + +def test_socket_proto_is_normalised_and_validated(plugin, tmp_path): + """The transmit path matches on the exact string, so an unrecognised value + would send nothing at all while the resolver looked healthy.""" + cfg = tmp_path / "pfui_unbound.yml" + cfg.write_text("--- # Yaml\nSOCKET_PROTO: ' tcp '\n") + assert plugin.load_config(cfg)["SOCKET_PROTO"] == "TCP" + + cfg.write_text("--- # Yaml\nSOCKET_PROTO: SCTP\n") + with pytest.raises(ValueError): + plugin.load_config(cfg) + + +def test_moddone_completes_with_a_config_that_omits_optional_keys(plugin, tmp_path): + """The failure this guards: a missing key raised KeyError inside operate(), + before ext_state was set, so Unbound never learned the module had finished.""" + minimal = tmp_path / "pfui_unbound.yml" + minimal.write_text("--- # Yaml\nFIREWALLS: []\n") + plugin.pfui_cfg = plugin.load_config(minimal) + + class QState: + return_msg = None + ext_state = {} + + qstate = QState() + assert plugin.operate(0, plugin.MODULE_EVENT_MODDONE, qstate, None) is True + assert qstate.ext_state[0] == plugin.MODULE_FINISHED + + +def test_logger_does_not_print_signature_bytes_as_an_address(plugin): + """The debug dump had the same defect read_rr was fixed for: it read every + record's trailing bytes as an address, so a signed answer printed an IPv4 + line that no nameserver ever sent.""" + printed = [] + plugin.pfui_cfg = {"LOGGING": True, "LOG_LEVEL": "DEBUG"} + plugin.log_info = lambda msg="": printed.append(str(msg)) + + class FakeData: + count = 1 + rrsig_count = 1 + rr_ttl = [3600, 3600] + rr_data = [ + b"\x00\x04" + bytes([8, 8, 8, 8]), + b"\x00\x88" + bytes([136, 137, 138, 139]), # signature rdata + ] + + class FakeKey: + dname_list, dname_str, flags = [], "signed.example.com.", 0 + type_str, rrset_class_str = "A", "IN" + type = rrset_class = 1 + + class FakeRRset: + rk = FakeKey() + + class entry: + data = FakeData() + + class FakeRep: + flags = qdcount = security = ttl = 0 + rrset_count = 1 + rrsets = [FakeRRset()] + qinfo = FakeKey() + + class QInfo: + qname_str, qtype_str, qclass_str = "signed.example.com.", "A", "IN" + qname_list = [] + qtype = qclass = 1 + + class QState: + qinfo = QInfo() + + class return_msg: + rep = FakeRep() + qinfo = QInfo() + + try: + plugin.logger(QState()) + finally: + plugin.log_info = INJECTED["log_info"] + + addresses = [line for line in printed if line.startswith("IPv4:")] + assert addresses == ["IPv4: 8.8.8.8"], f"signature bytes printed as {addresses}" + + +class AckServer: + """Loopback stand-in for PFUI_Firewall. `reply=None` accepts the connection + and then says nothing, which is the case that used to look like success.""" + + def __init__(self, reply=b"ACKUPDATE", connections=4): + self.listener = socket.socket() + self.listener.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + self.listener.bind(("127.0.0.1", 0)) + self.listener.listen(8) + self.port = self.listener.getsockname()[1] + self.reply = reply + self.received = [] + self._held = [] + self._thread = Thread(target=self._serve, args=(connections,), daemon=True) + self._thread.start() + + def _serve(self, connections): + for _ in range(connections): + try: + conn, _ = self.listener.accept() + except OSError: + return + self.received.append(conn.recv(65536)) + if self.reply is None: + self._held.append(conn) # Never reply, never close + else: + conn.sendall(self.reply) + conn.close() + + def close(self): + self.listener.close() + for conn in self._held: + conn.close() + + +@pytest.fixture +def breaker_cfg(plugin): + plugin.pfui_cfg = { + "LOGGING": False, + "LOG_LEVEL": "ERROR", + "COMPRESS": False, + "SOCKET_PROTO": "TCP", + "SOCKET_TIMEOUT": 0.25, + "BLOCKING": True, + "BREAKER_FAILURES": 2, + "BREAKER_COOLOFF": 30, + "DEFAULT_PORT": 10001, + "FIREWALLS": [], + } + plugin._breakers.clear() + yield plugin.pfui_cfg + plugin._breakers.clear() + + +def queries(plugin, count): + """Drive `count` whole queries through transmit_all. + + Deliberately not calling the transmit functions directly: transmit_all + consults the breaker before each attempt, and it was that consultation which + used to reset the failure count, so a test that skips it cannot see the + breaker fail to trip. + """ + for _ in range(count): + plugin.transmit_all({"kind": "rr", "qname": "a.", "AF4": [], "AF6": []}) + + +def test_breaker_opens_when_the_firewall_never_acknowledges(plugin, breaker_cfg): + """Recording success at sendall meant a firewall that accepted the connection + and never replied looked healthy forever, so the breaker never opened and + every query paid SOCKET_TIMEOUT in full.""" + server = AckServer(reply=None, connections=4) + breaker_cfg["FIREWALLS"] = [{"HOST": "127.0.0.1", "PORT": server.port}] + try: + queries(plugin, 2) # BREAKER_FAILURES + assert plugin.breaker_open(f"127.0.0.1:{server.port}") + finally: + server.close() + + +def test_breaker_counts_across_queries_rather_than_resetting(plugin, breaker_cfg): + """The count has to survive the breaker being consulted. While it did not, a + threshold above 1 could never be reached and the breaker was inert.""" + server = AckServer(reply=None, connections=4) + breaker_cfg["BREAKER_FAILURES"] = 3 + breaker_cfg["FIREWALLS"] = [{"HOST": "127.0.0.1", "PORT": server.port}] + key = f"127.0.0.1:{server.port}" + try: + queries(plugin, 1) + assert plugin._breakers[key][0] == 1 + assert not plugin.breaker_open(key) + queries(plugin, 1) + assert plugin._breakers[key][0] == 2, "the failure count was reset" + assert not plugin.breaker_open(key) + queries(plugin, 1) + assert plugin.breaker_open(key), "three failures did not trip a threshold of 3" + finally: + server.close() + + +def test_a_tripped_breaker_stops_further_attempts(plugin, breaker_cfg): + """The point of the breaker: an unreachable firewall stops costing the + resolver a timeout per query.""" + server = AckServer(reply=None, connections=8) + breaker_cfg["FIREWALLS"] = [{"HOST": "127.0.0.1", "PORT": server.port}] + try: + queries(plugin, 2) # Trips it + connected = len(server.received) + queries(plugin, 3) # Must all be skipped + assert len(server.received) == connected, "kept connecting once tripped" + finally: + server.close() + + +def test_breaker_reopens_for_a_probe_once_the_cooloff_elapses(plugin, breaker_cfg): + server = AckServer(reply=None, connections=8) + breaker_cfg["BREAKER_COOLOFF"] = 0.5 + breaker_cfg["FIREWALLS"] = [{"HOST": "127.0.0.1", "PORT": server.port}] + key = f"127.0.0.1:{server.port}" + try: + queries(plugin, 2) + assert plugin.breaker_open(key) + time.sleep(0.75) + assert not plugin.breaker_open(key), "never probed again after the cool-off" + assert plugin._breakers[key][0] == 0, "the count was not cleared for the probe" + finally: + server.close() + + +def test_breaker_stays_closed_when_the_firewall_acknowledges(plugin, breaker_cfg): + server = AckServer(reply=b"ACKUPDATE", connections=4) + breaker_cfg["FIREWALLS"] = [{"HOST": "127.0.0.1", "PORT": server.port}] + try: + queries(plugin, 3) + assert not plugin.breaker_open(f"127.0.0.1:{server.port}") + assert len(server.received) == 3 + finally: + server.close() + + +def test_breaker_opens_on_a_refusal(plugin, breaker_cfg): + """A reachable firewall that refuses every message is not doing its job, and + PF denies the traffic either way.""" + server = AckServer(reply=b"Missing kind", connections=4) + breaker_cfg["FIREWALLS"] = [{"HOST": "127.0.0.1", "PORT": server.port}] + try: + queries(plugin, 2) + assert plugin.breaker_open(f"127.0.0.1:{server.port}") + finally: + server.close() + + +def test_payload_is_encoded_once_for_all_firewalls(plugin, breaker_cfg): + """encode_payload ran per firewall inside the loop, so a CARP pair paid for + two lz4 passes over identical bytes on the blocking DNS path.""" + calls = [] + real_encode = plugin.encode_payload + plugin.encode_payload = lambda msg, compress=True: ( + calls.append(msg) or real_encode(msg, compress=compress) + ) + sent = [] + real_tcp = plugin.tcp_transmit_close # Restored, not deleted: a del here would + # remove the module's own function and break every later test + plugin.tcp_transmit_close = lambda data, ip, port, blocking: sent.append((ip, port)) + breaker_cfg["FIREWALLS"] = [ + {"HOST": "127.0.0.1", "PORT": 10001}, + {"HOST": "127.0.0.2", "PORT": 10001}, + ] + try: + plugin.transmit_all({"kind": "rr", "qname": "a.", "AF4": [], "AF6": []}) + finally: + plugin.encode_payload = real_encode + plugin.tcp_transmit_close = real_tcp + + assert len(sent) == 2, "both firewalls must still be told" + assert len(calls) == 1, f"payload encoded {len(calls)} times for 2 firewalls" + + +def test_nothing_is_encoded_when_there_is_no_firewall_to_send_to(plugin, breaker_cfg): + calls = [] + real_encode = plugin.encode_payload + plugin.encode_payload = lambda msg, compress=True: ( + calls.append(msg) or real_encode(msg, compress=compress) + ) + try: + plugin.transmit_all({"kind": "rr", "qname": "a.", "AF4": [], "AF6": []}) + finally: + plugin.encode_payload = real_encode + assert calls == [] + + +class UnixAckServer(AckServer): + """The same stand-in bound to a local socket instead of loopback TCP.""" + + def __init__(self, path, reply=b"ACKUPDATE", connections=4): + self.path = str(path) + self.listener = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + self.listener.bind(self.path) + self.listener.listen(8) + self.listener.settimeout(0.5) + self.port = None + self.reply = reply + self.received = [] + self._held = [] + self.stop = Event() + self._thread = Thread(target=self._serve, args=(connections,), daemon=True) + self._thread.start() + + +@pytest.fixture +def short_tmp(): + """Scratch directory with a path short enough for sockaddr_un.sun_path. + + pytest's tmp_path is far too long, especially on macOS where it lives under + /private/var/folders/... + """ + path = Path(tempfile.mkdtemp(prefix="pfui-", dir="/tmp")) + try: + yield path + finally: + shutil.rmtree(path, ignore_errors=True) + + +def test_a_socket_entry_is_reached_over_a_local_socket(plugin, breaker_cfg, short_tmp): + """A resolver on the firewall itself: no port, no address, no PF rule.""" + path = short_tmp / "pfui_firewall.sock" + server = UnixAckServer(path, connections=2) + breaker_cfg["FIREWALLS"] = [{"SOCKET": str(path)}] + try: + plugin.transmit_all({"kind": "rr", "qname": "a.", "AF4": [], "AF6": []}) + time.sleep(0.5) + assert server.received, "nothing arrived on the local socket" + assert not plugin.breaker_open(str(path)) + finally: + server.close() + + +def test_a_local_and_a_remote_firewall_are_both_told(plugin, breaker_cfg, short_tmp): + """The CARP case the per-entry transport exists for: this node's own firewall + over its socket, the peer over TCP, from one resolver and one config.""" + path = short_tmp / "pfui_firewall.sock" + local = UnixAckServer(path, connections=2) + remote = AckServer(connections=2) + breaker_cfg["FIREWALLS"] = [ + {"SOCKET": str(path)}, + {"HOST": "127.0.0.1", "PORT": remote.port}, + ] + calls = [] + real_encode = plugin.encode_payload + plugin.encode_payload = lambda msg, compress=True: ( + calls.append(msg) or real_encode(msg, compress=compress) + ) + try: + plugin.transmit_all({"kind": "rr", "qname": "a.", "AF4": [], "AF6": []}) + time.sleep(0.5) + assert local.received, "the local firewall was not told" + assert remote.received, "the remote firewall was not told" + assert local.received[0] == remote.received[0], "the two got different bytes" + assert len(calls) == 1, "the payload was encoded per transport" + finally: + plugin.encode_payload = real_encode + local.close() + remote.close() + + +def test_the_breaker_tracks_a_local_and_a_remote_firewall_separately( + plugin, breaker_cfg, short_tmp +): + """One key per destination: a dead socket must not shut off a healthy peer.""" + path = short_tmp / "absent.sock" # Nothing bound: every connect fails + remote = AckServer(connections=4) + breaker_cfg["FIREWALLS"] = [ + {"SOCKET": str(path)}, + {"HOST": "127.0.0.1", "PORT": remote.port}, + ] + try: + for _ in range(2): # BREAKER_FAILURES + plugin.transmit_all({"kind": "rr", "qname": "a.", "AF4": [], "AF6": []}) + time.sleep(0.5) + assert plugin.breaker_open(str(path)), "the dead socket did not trip" + assert not plugin.breaker_open(f"127.0.0.1:{remote.port}") + assert len(remote.received) == 2, "the healthy firewall lost a message" + finally: + remote.close() + + +def test_a_socket_entry_ignores_socket_proto(plugin, breaker_cfg, short_tmp): + """SOCKET_PROTO describes the network transport. A local socket is always a + stream carrying the same frames, so UDP must not send a bare datagram at it.""" + path = short_tmp / "pfui_firewall.sock" + server = UnixAckServer(path, connections=2) + breaker_cfg["SOCKET_PROTO"] = "UDP" + breaker_cfg["FIREWALLS"] = [{"SOCKET": str(path)}] + try: + plugin.transmit_all({"kind": "rr", "qname": "a.", "AF4": [], "AF6": []}) + time.sleep(0.5) + assert server.received, "nothing arrived on the local socket" + # Length-prefixed, as the stream framing requires + assert len(server.received[0]) > 4 + finally: + server.close() + + +def test_firewall_target_reads_each_entry_shape(plugin, breaker_cfg): + breaker_cfg["SOCKET_PROTO"] = "TCP" + assert plugin.firewall_target({"SOCKET": "/var/run/pfui/f.sock"}) == ( + "UNIX", + "/var/run/pfui/f.sock", + "/var/run/pfui/f.sock", + ) + assert plugin.firewall_target({"HOST": "10.0.0.1", "PORT": 10002}) == ( + "TCP", + "10.0.0.1:10002", + ("10.0.0.1", 10002), + ) + # An entry with no PORT falls back to DEFAULT_PORT, and one left empty in the + # yml parses as None rather than being absent + assert plugin.firewall_target({"HOST": "10.0.0.1"})[2] == ("10.0.0.1", 10001) + assert plugin.firewall_target({"HOST": "10.0.0.1", "PORT": None})[2] == ( + "10.0.0.1", + 10001, + ) + # A placeholder entry names no firewall and is skipped, as it always was + assert plugin.firewall_target({"HOST": None}) is None + assert plugin.firewall_target({}) is None + + +def test_an_entry_setting_both_socket_and_host_is_refused(plugin, tmp_path): + """Unambiguous misconfiguration: it cannot be a leftover placeholder.""" + cfg = tmp_path / "pfui_unbound.yml" + cfg.write_text( + "--- # Yaml\nFIREWALLS:\n" + " - SOCKET: /var/run/pfui/pfui_firewall.sock\n HOST: 10.0.0.1\n" + ) + with pytest.raises(ValueError): + plugin.load_config(cfg) + + +def test_a_relative_socket_path_is_refused(plugin, tmp_path): + cfg = tmp_path / "pfui_unbound.yml" + cfg.write_text("--- # Yaml\nFIREWALLS:\n - SOCKET: pfui_firewall.sock\n") + with pytest.raises(ValueError): + plugin.load_config(cfg) + + +def test_a_placeholder_entry_still_loads(plugin, tmp_path): + """Refusing here would break configs that work today, where an entry with an + empty HOST is left in place as a comment.""" + cfg = tmp_path / "pfui_unbound.yml" + cfg.write_text("--- # Yaml\nFIREWALLS:\n - HOST:\n PORT:\n") + assert plugin.load_config(cfg)["FIREWALLS"] == [{"HOST": None, "PORT": None}] + + +def test_a_mixed_config_loads(plugin, tmp_path): + cfg = tmp_path / "pfui_unbound.yml" + cfg.write_text( + "--- # Yaml\nFIREWALLS:\n" + " - SOCKET: /var/run/pfui/pfui_firewall.sock\n" + " - HOST: 10.10.1.253\n PORT: 10001\n" + ) + assert len(plugin.load_config(cfg)["FIREWALLS"]) == 2 + + def test_unknown_event_sets_module_error_even_if_logging_fails(plugin): """The debug dump derefs qstate.return_msg; a failure there must not stop the module from reporting MODULE_ERROR back to Unbound.""" diff --git a/client-unbound/tools/unbound_release.sh b/client-unbound/tools/unbound_release.sh new file mode 100755 index 0000000..c2490ae --- /dev/null +++ b/client-unbound/tools/unbound_release.sh @@ -0,0 +1,47 @@ +#!/usr/bin/env bash +# +# Prints the Unbound git ref to build. Used by install-client-unbound.sh and by +# the container that tests that build, so the installer and its test cannot +# disagree about which release they mean. +# +# Usage: unbound_release.sh [latest|master|release-1.26.0] +# UNBOUND_VERSION=release-1.25.2 unbound_release.sh + +UNBOUND_REPO="${UNBOUND_REPO:-https://github.com/NLnetLabs/unbound.git}" + +# Used only when the upstream tag list cannot be read. Bump this when PFUI has +# been tested against a newer release. +UNBOUND_FALLBACK_TAG="release-1.26.0" + +WANTED="${1:-${UNBOUND_VERSION:-latest}}" + +# Newest release tag upstream, empty if the tag list cannot be read. +# +# This resolves a tag rather than a branch because release tags are permanent: +# NLnet Labs prunes its branch- heads (nothing before 1.23 survives), so +# the branch-1.18.0 that PFUI used to fall back to no longer exists at all. +# +# The sort key is built by awk rather than passed to sort -V, which is a GNU +# extension OpenBSD's sort does not have. +latest_tag() { + git ls-remote --tags --refs "${UNBOUND_REPO}" 'release-*' 2>/dev/null \ + | sed 's#.*refs/tags/##' \ + | grep -E '^release-[0-9]+\.[0-9]+(\.[0-9]+)?$' \ + | awk -F'[-.]' '{ printf "%05d%05d%05d %s\n", $2, $3, $4, $0 }' \ + | sort \ + | tail -1 \ + | cut -d' ' -f2 +} + +if [ "${WANTED}" = "latest" ]; then + REF=$(latest_tag) + if [ -z "${REF}" ]; then + echo "unbound_release.sh: cannot read the tag list at ${UNBOUND_REPO}," \ + "falling back to ${UNBOUND_FALLBACK_TAG}" >&2 + REF="${UNBOUND_FALLBACK_TAG}" + fi +else + REF="${WANTED}" +fi + +echo "${REF}" diff --git a/client-unbound/tools/update_dns_blocklist.sh b/client-unbound/tools/update_dns_blocklist.sh index 7db2f7e..73fb634 100755 --- a/client-unbound/tools/update_dns_blocklist.sh +++ b/client-unbound/tools/update_dns_blocklist.sh @@ -2,50 +2,136 @@ # Example DNS BlockList script to download common bad domains from some example well known sources +set -u + args=("$@") -RESTARTPFUI=${args[0]} +RESTARTPFUI=${args[0]:-} + +ETC=${ETC:-/var/unbound/etc} # Overridable so the guards below can be tested +PUBLISHED="${ETC}/dns_blocklist" + +# A short or empty feed must never reach ${PUBLISHED}: Unbound loads it without +# complaint and every domain the list used to block resolves again, so a failed +# download would quietly turn the DNS filter off and then restart the resolver to +# apply it. Nothing is published unless the new list is plausible, and the +# resolver is only restarted when something was published. update_root_hints.sh +# guards its single download the same way. +MIN_ENTRIES=${MIN_ENTRIES:-1000} # Absolute floor on the merged list +MIN_PERCENT=${MIN_PERCENT:-50} # Floor relative to the list in use + +log() { /usr/bin/logger -p "daemon.$1" -t update_dns_blocklist.sh "$2"; } + +die() { + log err "$1" + echo "update_dns_blocklist.sh: $1" >&2 + echo "update_dns_blocklist.sh: ${PUBLISHED} left as it was, resolver not restarted" >&2 + exit 1 +} -/usr/bin/logger -p daemon.info -t update_dns_blocklist.sh "Updating DNS Domain Filter Lists" +# fetch ... +# Tries each URL in turn and only replaces once one has produced a +# non-empty file, so a failure leaves yesterday's copy in place. +fetch() { + dest=$1 + shift + tmp="${dest}.new.$$" + for url in "$@"; do + if curl -fsSL "$url" -o "$tmp" && [ -s "$tmp" ]; then + mv "$tmp" "$dest" + return 0 + fi + log warning "download failed, trying any remaining source: ${url}" + done + rm -f "$tmp" + return 1 +} + +# require +# A source is usable if it was just downloaded or survives from a previous run. +require() { + [ -s "$1" ] || die "$2 is unavailable and no previous copy exists ($1)" +} + +# hosts_to_unbound +hosts_to_unbound() { + grep '^0\.0\.0\.0' "$1" \ + | awk '{print "local-zone: \""$2"\" redirect\nlocal-data: \""$2" A 0.0.0.0\""}' \ + > "$2" +} + +lines() { wc -l < "$1" | tr -d '[:space:]'; } + +log info "Updating DNS Domain Filter Lists" # https://github.com/StevenBlack/hosts -/usr/bin/logger -p daemon.info -t update_dns_blocklist.sh "Downloading StevenBlack Bad Domains - Unified hosts (adware + malware) + fakenews + gambling" -curl -fsSL https://raw.githubusercontent.com/StevenBlack/hosts/master/alternates/fakenews-gambling/hosts -o /var/unbound/etc/stephenblack_adware_malware_fakenews_gambling-raw -if [[ $? != 0 ]]; then - # Fallback is plain HTTP and unauthenticated: content is not verified - curl -fsSL http://sbc.io/hosts/alternates/fakenews-gambling/hosts -o /var/unbound/etc/stephenblack_adware_malware_fakenews_gambling-raw -fi -/usr/bin/logger -p daemon.info -t update_dns_blocklist.sh "Downloading StevenBlack Bad Domains - Unified hosts (adware + malware) + fakenews + gambling + social" -curl -fsSL https://raw.githubusercontent.com/StevenBlack/hosts/master/alternates/fakenews-gambling-social/hosts -o /var/unbound/etc/stephenblack_adware_malware_fakenews_gambling_social-raw -if [[ $? != 0 ]]; then - # Fallback is plain HTTP and unauthenticated: content is not verified - curl -fsSL http://sbc.io/hosts/alternates/fakenews-gambling-social/hosts -o /var/unbound/etc/stephenblack_adware_malware_fakenews_gambling_social-raw -fi -/usr/bin/logger -p daemon.info -t update_dns_blocklist.sh "Converting StevenBlack Bad Domains from RAW format to Unbound config format" -cat /var/unbound/etc/stephenblack_adware_malware_fakenews_gambling-raw | grep '^0\.0\.0\.0' | awk '{print "local-zone: \""$2"\" redirect\nlocal-data: \""$2" A 0.0.0.0\""}' > /var/unbound/etc/stephenblack_adware_malware_fakenews_gambling-unbound -cat /var/unbound/etc/stephenblack_adware_malware_fakenews_gambling_social-raw | grep '^0\.0\.0\.0' | awk '{print "local-zone: \""$2"\" redirect\nlocal-data: \""$2" A 0.0.0.0\""}' > /var/unbound/etc/stephenblack_adware_malware_fakenews_gambling_social-unbound +SB_BASE=https://raw.githubusercontent.com/StevenBlack/hosts/master/alternates +# Fallback is plain HTTP and unauthenticated: content is not verified +SB_MIRROR=http://sbc.io/hosts/alternates + +log info "Downloading StevenBlack Bad Domains - Unified hosts (adware + malware) + fakenews + gambling" +fetch "${ETC}/stephenblack_adware_malware_fakenews_gambling-raw" \ + "${SB_BASE}/fakenews-gambling/hosts" \ + "${SB_MIRROR}/fakenews-gambling/hosts" +require "${ETC}/stephenblack_adware_malware_fakenews_gambling-raw" \ + "StevenBlack fakenews-gambling list" + +log info "Downloading StevenBlack Bad Domains - Unified hosts (adware + malware) + fakenews + gambling + social" +fetch "${ETC}/stephenblack_adware_malware_fakenews_gambling_social-raw" \ + "${SB_BASE}/fakenews-gambling-social/hosts" \ + "${SB_MIRROR}/fakenews-gambling-social/hosts" +require "${ETC}/stephenblack_adware_malware_fakenews_gambling_social-raw" \ + "StevenBlack fakenews-gambling-social list" + +log info "Converting StevenBlack Bad Domains from RAW format to Unbound config format" +hosts_to_unbound "${ETC}/stephenblack_adware_malware_fakenews_gambling-raw" \ + "${ETC}/stephenblack_adware_malware_fakenews_gambling-unbound" +hosts_to_unbound "${ETC}/stephenblack_adware_malware_fakenews_gambling_social-raw" \ + "${ETC}/stephenblack_adware_malware_fakenews_gambling_social-unbound" echo -/usr/bin/logger -p daemon.info -t update_dns_blocklist.sh "Downloading YoYo AdServers Bad Domains" -curl -fsSL "https://pgl.yoyo.org/adservers/serverlist.php?hostformat=unbound&showintro=0&mimetype=plaintext" -o /var/unbound/etc/yoyo_adservers-unbound +log info "Downloading YoYo AdServers Bad Domains" +fetch "${ETC}/yoyo_adservers-unbound" \ + "https://pgl.yoyo.org/adservers/serverlist.php?hostformat=unbound&showintro=0&mimetype=plaintext" +require "${ETC}/yoyo_adservers-unbound" "YoYo AdServers list" echo -/usr/bin/logger -p daemon.info -t update_dns_blocklist.sh "Merging all Bad Domains" -cat /var/unbound/etc/stephenblack_adware_malware_fakenews_gambling_social-unbound > /var/unbound/etc/dns_blocklist_all -cat /var/unbound/etc/yoyo_adservers-unbound >> /var/unbound/etc/dns_blocklist_all +log info "Merging all Bad Domains" +cat "${ETC}/stephenblack_adware_malware_fakenews_gambling_social-unbound" \ + "${ETC}/yoyo_adservers-unbound" > "${ETC}/dns_blocklist_all" \ + || die "cannot write ${ETC}/dns_blocklist_all" echo -/usr/bin/logger -p daemon.info -t update_dns_blocklist.sh "Sorting, filtering and de-duplicating all Bad Domains" +log info "Sorting, filtering and de-duplicating all Bad Domains" # Add 'grep -v -f /var/unbound/etc/dns_blocklist_exceptions' here to allowlist domains -sort -u /var/unbound/etc/dns_blocklist_all > /var/unbound/etc/dns_blocklist -echo -echo "Written DNS-BL to: /var/unbound/etc/dns_blocklist" -echo "Define 'include: /var/unbound/etc/dns_blocklist' in /var/unbound/etc/pfui_unbound.conf" +STAGED="${PUBLISHED}.new.$$" +sort -u "${ETC}/dns_blocklist_all" > "${STAGED}" || die "cannot write ${STAGED}" + +# Verify before publishing, not after +NEW=$(lines "${STAGED}") +if [ "${NEW}" -lt "${MIN_ENTRIES}" ]; then + rm -f "${STAGED}" + die "merged list has only ${NEW} lines, below the ${MIN_ENTRIES} minimum" +fi +if [ -s "${PUBLISHED}" ]; then + OLD=$(lines "${PUBLISHED}") + if [ $((NEW * 100)) -lt $((OLD * MIN_PERCENT)) ]; then + rm -f "${STAGED}" + die "merged list shrank from ${OLD} to ${NEW} lines, more than ${MIN_PERCENT}% down" + fi +fi + +chown root:_unbound "${STAGED}" +chmod 644 "${STAGED}" +mv "${STAGED}" "${PUBLISHED}" || die "cannot publish ${PUBLISHED}" +log info "Published ${NEW} lines to ${PUBLISHED}" echo -/usr/bin/logger -p daemon.info -t update_dns_blocklist.sh "Restarting PFUI_Unbound to apply updates" -chown root:_unbound /var/unbound/etc/dns_blocklist +echo "Written DNS-BL to: ${PUBLISHED} (${NEW} lines)" +echo "Define 'include: ${PUBLISHED}' in ${ETC}/pfui_unbound.conf" if [[ "$RESTARTPFUI" != "norestart" ]]; then + echo + log info "Restarting PFUI_Unbound to apply updates" rcctl restart pfui_unbound fi echo diff --git a/install-client-unbound.sh b/install-client-unbound.sh index 297ed6b..38fb1bf 100755 --- a/install-client-unbound.sh +++ b/install-client-unbound.sh @@ -4,7 +4,15 @@ # https://github.com/NLnetLabs/unbound # -UNBOUND_BRANCH="branch-1.18.0" # Stable Unbound branch to use if HEAD is not building without error +# Exported so unbound_release.sh resolves tags from the same repository this +# clones from, rather than each holding its own copy of the URL +export UNBOUND_REPO="https://github.com/NLnetLabs/unbound.git" +# Which Unbound to build. "latest" asks the upstream repository for its newest +# release tag at run time; set an explicit tag (Eg, UNBOUND_VERSION=release-1.25.2) +# to pin, or "master" to build the development head. The resolution and the +# pinned fallback live in client-unbound/tools/unbound_release.sh, which the +# container that tests this build uses too. +UNBOUND_VERSION="${UNBOUND_VERSION:-latest}" err=0 trap 'err=1' ERR @@ -151,17 +159,42 @@ if [[ "$OS" = "OpenBSD" ]]; then echo read -p "Would you like to build Unbound with Python module support (required) y/n: " yn if [[ "$yn" = "y" ]]; then - echo "PFUIDNS: Building Unbound with Python Module Support" - echo "PFUIDNS: Moving default Unbound source in OpenBSD tree to one side (/usr/src/usr.sbin/unbound.base)" - mv /usr/src/usr.sbin/unbound /usr/src/usr.sbin/unbound.base - echo "PFUIDNS: Downloading latest Unbound Source into /usr/src/usr.bin" - git clone --depth 20 https://github.com/NLnetLabs/unbound.git /usr/src/usr.sbin/unbound + RELEASE_HELPER="${DIR}/client-unbound/tools/unbound_release.sh" + [ -x "${RELEASE_HELPER}" ] || die "${RELEASE_HELPER} is missing or not executable" + echo "PFUIDNS: Resolving which Unbound to build (UNBOUND_VERSION=${UNBOUND_VERSION})" + UNBOUND_REF=$("${RELEASE_HELPER}" "${UNBOUND_VERSION}") \ + || die "cannot resolve which Unbound release to build" + echo "PFUIDNS: Building Unbound ${UNBOUND_REF} with Python Module Support" + + # Only move the pristine port aside once: a second run would otherwise + # overwrite unbound.base with the previous clone, and Makefile.bsd-wrapper + # is the one thing that has to come from base + if [ ! -d /usr/src/usr.sbin/unbound.base ]; then + echo "PFUIDNS: Moving default Unbound source in OpenBSD tree to one side (/usr/src/usr.sbin/unbound.base)" + mv /usr/src/usr.sbin/unbound /usr/src/usr.sbin/unbound.base \ + || die "cannot move /usr/src/usr.sbin/unbound aside" + fi + [ -f /usr/src/usr.sbin/unbound.base/Makefile.bsd-wrapper ] \ + || die "no Makefile.bsd-wrapper in /usr/src/usr.sbin/unbound.base; update the system sources first" + rm -rf /usr/src/usr.sbin/unbound + + echo "PFUIDNS: Downloading Unbound ${UNBOUND_REF} into /usr/src/usr.sbin/unbound" + # --branch takes a tag, so a single-commit clone lands directly on the wanted + # release. The old form cloned the default branch and then tried to check out + # another ref, which --depth had already made unavailable + git clone --depth 1 --branch "${UNBOUND_REF}" "${UNBOUND_REPO}" \ + /usr/src/usr.sbin/unbound \ + || die "cannot clone Unbound ${UNBOUND_REF} (does that tag or branch exist?)" + echo "PFUIDNS: Import OpenBSD make wrapper from base to latest source" - cp /usr/src/usr.sbin/unbound.base/Makefile.bsd-wrapper /usr/src/usr.sbin/unbound/Makefile.bsd-wrapper + cp /usr/src/usr.sbin/unbound.base/Makefile.bsd-wrapper /usr/src/usr.sbin/unbound/Makefile.bsd-wrapper \ + || die "cannot import Makefile.bsd-wrapper" echo "PFUIDNS: Building" - cd /usr/src/usr.sbin/unbound || exit + cd /usr/src/usr.sbin/unbound || die "cannot cd to the Unbound source" # Use same build options as Unbound on OpenBSD, but with pythonmodule enabled + # Every step below is checked: an unnoticed configure failure used to be + # followed by make and install-all anyway, leaving whatever those produced ./configure --enable-allsymbols \ --with-ssl=/usr \ --with-libevent=/usr \ @@ -174,27 +207,10 @@ if [[ "$OS" = "OpenBSD" ]]; then --with-username=_unbound \ --disable-shared \ --disable-explicit-port-randomisation \ - --without-pthreads - if [[ $? != 0 ]]; then - echo "PFUIDNS: Unbound failed to configure with the current HEAD, trying release branch" - git checkout $UNBOUND_BRANCH # HEAD of Unbound is occasionally unstable - make clean - ./configure --enable-allsymbols \ - --with-ssl=/usr \ - --with-libevent=/usr \ - --with-libexpat=/usr \ - --with-pythonmodule \ - --with-chroot-dir=/var/unbound \ - --with-pidfile="" \ - --with-rootkey-file=/var/unbound/db/root.key \ - --with-conf-file=${TARGET}/pfui_unbound.conf \ - --with-username=_unbound \ - --disable-shared \ - --disable-explicit-port-randomisation \ - --without-pthreads - fi - make -f Makefile.bsd-wrapper - make install-all + --without-pthreads \ + || die "Unbound ${UNBOUND_REF} failed to configure. Re-run pinned to a known release, Eg 'UNBOUND_VERSION=release-1.25.2 $0 ${TARGET}'" + make -f Makefile.bsd-wrapper || die "Unbound ${UNBOUND_REF} failed to build" + make install-all || die "Unbound ${UNBOUND_REF} failed to install" make clean fi @@ -217,6 +233,34 @@ if [[ "$OS" = "OpenBSD" ]]; then # root-owned: rcctl runs this as root, and a file's owner can always chmod it install -m 555 -o root -g wheel "${DIR}"/client-unbound/rc.d/pfui_unbound /etc/rc.d/pfui_unbound + # Same-host deployment: if PFUI_Firewall is installed here too, the resolver can + # reach it over the local socket instead of loopback TCP. Group _pfui is what + # permits that, and it only exists once install-server-python.sh has run + echo + if groupinfo _pfui >/dev/null 2>&1; then + echo "PFUIDNS: PFUI_Firewall is installed on this host (group '_pfui' exists)" + if groupinfo _pfui | grep -qw _unbound; then + echo "PFUIDNS: '_unbound' is already in '_pfui'" + else + # -G replaces secondary memberships on OpenBSD. _unbound is a base system + # account with none by default, but any that exist are preserved here + EXISTING=$(id -Gn _unbound 2>/dev/null | tr ' ' '\n' | grep -v '^_unbound$' | paste -sd, -) + if [ -n "${EXISTING}" ]; then + usermod -G "${EXISTING},_pfui" _unbound || die "cannot add _unbound to _pfui" + else + usermod -G _pfui _unbound || die "cannot add _unbound to _pfui" + fi + echo "PFUIDNS: Added '_unbound' to group '_pfui' (permits the local PFUI socket)" + fi + echo "PFUIDNS: To use it, set SOCKET_UNIX in /etc/pfui_firewall.yml and add" + echo " '- SOCKET: /var/run/pfui/pfui_firewall.sock' to FIREWALLS in" + echo " ${TARGET}/pfui_unbound.yml, then restart both services." + echo "PFUIDNS: NB Unbound must be restarted for the new group to take effect." + else + echo "PFUIDNS: No '_pfui' group, so PFUI_Firewall is not installed on this host;" + echo " the resolver will reach its firewall(s) over the network." + fi + echo echo "PFUIDNS: Installing Root Hints and example DNS-BL" [ -f "${TARGET}/update_root_hints.sh" ] && mv "${TARGET}/update_root_hints.sh" "${TARGET}/update_root_hints.sh.${HOUR}" diff --git a/install-server-python.sh b/install-server-python.sh index c2f9dc5..436f138 100755 --- a/install-server-python.sh +++ b/install-server-python.sh @@ -66,6 +66,21 @@ if [[ "$OS" = "OpenBSD" ]]; then useradd -g _pfui_firewall -s /sbin/nologin -d /var/empty _pfui_firewall 2>/dev/null || true id _pfui_firewall >/dev/null || die "daemon user _pfui_firewall was not created" + # Shared group for the local socket (SOCKET_UNIX), which a PFUI_Unbound on this + # same host connects to. It is a group of its own rather than reusing _unbound + # or _pfui_firewall: membership of it means "may inject PF whitelist entries", + # and that should not be implied by running as the resolver or as the daemon. + # install-client-unbound.sh adds _unbound to it when a resolver is installed here. + echo "PFUIFW: Creating group '_pfui' (permits the local PFUI socket)" + groupadd _pfui 2>/dev/null || true + groupinfo _pfui >/dev/null 2>&1 || die "group _pfui was not created" + # -G replaces secondary memberships on OpenBSD, so it is only applied when the + # daemon account is not already in the group. _pfui_firewall is created here and + # has no other secondary groups + if ! groupinfo _pfui | grep -qw _pfui_firewall; then + usermod -G _pfui _pfui_firewall || die "cannot add _pfui_firewall to _pfui" + fi + # PF ioctl access without wheel, which would also grant su. rc.d/pfui_firewall # re-applies this on every start, because MAKEDEV resets it on release upgrades chgrp _pfui_firewall /dev/pf && chmod 660 /dev/pf || die "cannot set /dev/pf ownership" @@ -102,6 +117,12 @@ if [[ "$OS" = "OpenBSD" ]]; then echo "PFUIFW: An example pf.conf file is located at '/etc/pf-pfui-example.conf'" echo "PFUIFW: /etc/pf.conf is NOT modified; merge the PFUI tables and rules yourself" + # Where the daemon's pid file and, if SOCKET_UNIX is configured, its local socket + # live. Group _pfui at 0750 so only that group can traverse to the socket, and so + # the socket inherits the group; rc.d/pfui_firewall re-applies this on every start + install -d -o _pfui_firewall -g _pfui -m 750 /var/run/pfui \ + || die "cannot create /var/run/pfui" + echo "PFUIFW: Updating Persist files /var/spool/pfui/pfui_ipv<*>_domains" # Daemon-owned directory: file_push/file_pop need to create a .lock sidecar and # mkstemp here. pfctl reads the files as root, so no world access is needed. @@ -128,4 +149,9 @@ else fi echo "PFUIFW: Enable service 'rcctl enable pfui_firewall'" echo "PFUIFW: Start service 'rcctl start pfui_firewall'" +echo +echo "PFUIFW: If PFUI_Unbound runs on THIS host, uncomment SOCKET_UNIX in" +echo " /etc/pfui_firewall.yml and add '- SOCKET: /var/run/pfui/pfui_firewall.sock'" +echo " to the resolver's FIREWALLS. No pf.conf rule is needed for that path;" +echo " membership of group '_pfui' is what permits it." diff --git a/protocol/PROTOCOL.md b/protocol/PROTOCOL.md index fcb92cc..df0a6fa 100644 --- a/protocol/PROTOCOL.md +++ b/protocol/PROTOCOL.md @@ -11,14 +11,37 @@ field on the wire; see ## Transport -TCP is the supported transport. UDP exists for lab use and is disabled unless -the server sets `ALLOW_INSECURE_UDP`, because a datagram source address is not -verified and the protocol has no authentication. - -Neither transport is authenticated or encrypted. An IP must reach a PF table +| Transport | Use | +|-----------|-----| +| TCP | The supported transport between hosts. | +| Unix domain stream socket | A client and server on the same host. | +| UDP | Lab use only, and disabled unless the server sets `ALLOW_INSECURE_UDP`, because a datagram source address is not verified and the protocol has no authentication. | + +A server MAY serve more than one transport at once, and `server-python` does: a +firewall can accept messages from a resolver on the same host over a local socket +while accepting them from a remote resolver over TCP. Nothing in the message or +the framing distinguishes them. + +The unix socket is a `SOCK_STREAM` socket carrying **exactly the framing and the +replies TCP does**, so an implementation that speaks TCP needs no message-level +change to speak it. It is the better choice on one host: there is no handshake, +no `TIME_WAIT` entry per message, and no ephemeral-port ceiling, all of which +matter because a client opens one connection per DNS answer (see DECISIONS.md). + +No transport is authenticated or encrypted. An IP must reach a PF table microseconds before the client connects to it, and a handshake would spend that -budget. Access control is therefore the packet filter's job: restrict the -server's listening port to the known resolvers. +budget. Access control is therefore delegated: + +- TCP and UDP: the packet filter's job. Restrict the server's listening port to + the known resolvers. +- Unix socket: the filesystem's job. There is no packet to filter, so the socket's + ownership and mode are the whole control. A server MUST NOT create it + world-writable, and `server-python` binds it `0660` to a configured group, under + a directory only that group may traverse. A client that cannot connect with + `EACCES` is not in that group. + +Because the two are enforced in different places, a server that serves both is +only as restricted as the weaker of them. ## Framing @@ -139,7 +162,13 @@ IPv6 spelling. | `ACKDATA` | UDP only. The datagram decoded to a valid message. Sent **after** validation, never on receipt. | | (silence) | UDP only. A refused datagram gets no reply at all, since its source address is unverified and a reply would make the server a reflector. | | `ACKUPDATE` | The PF tables have been updated. The client may release the DNS answer. | -| any other | Refusal, with a short human-readable reason (`Missing kind`, `Bad frame`, `Bad length`, `Truncated`, `Failed to decode`, `Invalid datatype`, `Empty payload`, `Socket timeout`). | +| any other | Refusal, with a short human-readable reason (`Missing kind`, `Bad frame`, `Bad length`, `Truncated`, `Failed to decode`, `Invalid datatype`, `No records`, `Empty payload`, `Socket timeout`). | + +`Bad length` and `Truncated` are separate reasons because they are separate +faults: the first is a prefix a receiver refuses before buffering anything, the +second is a sender that declared bytes it did not send. `Invalid datatype` is a +payload that decoded to something other than a message object; `No records` is a +well-formed message with no routable address left after validation. A client SHOULD treat anything other than `ACKUPDATE` as a failed update and log the reason, which is what a version skew looks like from the client side. diff --git a/protocol/python/pfui_wire.py b/protocol/python/pfui_wire.py index 3d2ec93..0c4f0f7 100644 --- a/protocol/python/pfui_wire.py +++ b/protocol/python/pfui_wire.py @@ -19,6 +19,19 @@ class WireError(Exception): """A frame could not be decoded, or exceeded MAX_MESSAGE.""" +class BadLength(WireError): + """The declared payload length is zero or above MAX_MESSAGE. + + Distinct from Truncated so a receiver can send the distinct refusal + PROTOCOL.md documents, and to match the C implementation's + PFUI_BAD_LENGTH / PFUI_TRUNCATED, which the shared vectors also separate. + """ + + +class Truncated(WireError): + """The header was complete but the payload stopped short of it.""" + + def encode_payload(msg: dict, compress: bool = True) -> bytes: """Serialise one PFUI message, unframed. UDP datagrams are self-delimiting and carry the payload alone; only the TCP stream needs a length prefix. @@ -65,18 +78,18 @@ def decode(payload: bytes, compress: bool = True) -> dict: def read_frame(recv_exactly, compress: bool = True): """Read one frame using `recv_exactly(n) -> bytes | None`. - Returns None when the peer closed before sending a header; raises WireError - on a bad length or a truncated payload. + Returns None when the peer closed before sending a header; raises BadLength + or Truncated (both WireError) so a caller can report which it was. """ header = recv_exactly(HEADER.size) if header is None: return None (length,) = HEADER.unpack(header) if not 0 < length <= MAX_MESSAGE: - raise WireError(f"declared length {length} out of range") + raise BadLength(f"declared length {length} out of range") payload = recv_exactly(length) if payload is None: - raise WireError(f"truncated payload, expected {length} bytes") + raise Truncated(f"truncated payload, expected {length} bytes") return decode(payload, compress=compress) diff --git a/protocol/python/tests/test_vectors.py b/protocol/python/tests/test_vectors.py index 7714109..16b5ba7 100644 --- a/protocol/python/tests/test_vectors.py +++ b/protocol/python/tests/test_vectors.py @@ -9,7 +9,13 @@ import pytest -from pfui_wire import MAX_MESSAGE, WireError, decode_stream, encode +from pfui_wire import BadLength, MAX_MESSAGE, Truncated, WireError, decode_stream, encode + +# The vectors' `expect` column names the rejection, and server-c reports the +# same distinction as PFUI_BAD_LENGTH / PFUI_TRUNCATED. Asserting the class, +# rather than only WireError, is what keeps the two implementations honest about +# which refusal a receiver sends. +REJECTION = {"length": BadLength, "truncated": Truncated} VECTORS = Path(__file__).resolve().parents[3] / "protocol" / "vectors" @@ -44,7 +50,8 @@ def test_framing_vector(name, blob, expect, payload_hex): # No complete header arrived, so there is no message; not an error assert decode_stream(blob, compress=False) is None else: - with pytest.raises(WireError): + expected = REJECTION.get(expect, WireError) + with pytest.raises(expected): decode_stream(blob, compress=False) diff --git a/server-python/pfui_firewall.py b/server-python/pfui_firewall.py index 8866ddd..1d94821 100644 --- a/server-python/pfui_firewall.py +++ b/server-python/pfui_firewall.py @@ -23,8 +23,10 @@ https://man.openbsd.org/ioctl.2 https://docs.python.org/2/library/fcntl.html """ +import grp import logging import os +import stat import subprocess import sys import tempfile @@ -39,7 +41,7 @@ from service import Service, find_syslog from yaml import safe_load -from socket import AF_INET, AF_INET6, SOCK_DGRAM, SO_REUSEADDR, SOL_SOCKET +from socket import AF_INET, AF_INET6, AF_UNIX, SOCK_DGRAM, SO_REUSEADDR, SOL_SOCKET from socket import SOCK_STREAM, IPPROTO_TCP, TCP_NODELAY, AddressFamily from socket import error as socket_error, timeout as socket_timeout from socket import socket @@ -47,13 +49,129 @@ from pfui.pf_ioctl import table_pop, table_push from pfui.store import expired_keys from pfui.validate import extract -from pfui_wire import MAX_MESSAGE, WireError, decode, read_frame +from pfui_wire import BadLength, MAX_MESSAGE, Truncated, WireError, decode, read_frame CONFIG_LOCATION = "/etc/pfui_firewall.yml" # UDP mode only; see the receive loop in run(). TCP is bounded by MAX_MESSAGE. UDP_DGRAM_CEILING = 1400 +# Mode the local socket ends up with, and the umask that gets it there without +# ever being briefly wider. Not configurable: the socket is the whole access +# control for a local resolver, and the only reason to widen it is a mistake. +UNIX_SOCKET_MODE = 0o660 +UNIX_SOCKET_UMASK = 0o177 + +# Longest usable SOCKET_UNIX path. OpenBSD's sockaddr_un.sun_path is 104 bytes +# including the terminator; Linux allows 108. The smaller limit is checked so the +# failure is a named configuration error at load rather than an "AF_UNIX path too +# long" OSError out of bind() on the platform PFUI targets. +UNIX_PATH_MAX = 103 + +# Every key the daemon reads, with the value assumed when the yml omits it +CONFIG_DEFAULTS = { + "LOGGING": True, + "LOG_LEVEL": "DEBUG", + "SOCKET_PROTO": "TCP", + "SOCKET_PORT": 10001, + # Local stream socket for a resolver on this same host. Empty disables it. + "SOCKET_UNIX": "", + # Group allowed to connect; the resolver's account must be a member + "SOCKET_UNIX_GROUP": "_pfui", + "SOCKET_TIMEOUT": 3, + "SOCKET_BUFFER": 1024, + "SOCKET_BACKLOG": 128, + "COMPRESS": True, + "MAX_WORKERS": 32, + "ALLOW_INSECURE_UDP": False, + "REDIS_HOST": "127.0.0.1", + "REDIS_PORT": 6379, + "REDIS_DB": 0, # Valid range is 0-15 + "SCAN_PERIOD": 60, + "TTL_MULTIPLIER": 2, + "CTL": "IOCTL", + "DEVPF": "/dev/pf", +} + +# Keys with no safe default. SOCKET_LISTEN is deliberately not among them and +# not defaulted either: see the network-listener rule in load_config. +CONFIG_REQUIRED = { + "AF4_TABLE": "IPv4 PF Table", + "AF4_FILE": "IPv4 PF Persist file", + "AF6_TABLE": "IPv6 PF Table", + "AF6_FILE": "IPv6 PF Persist file", +} + + +def load_config(location: str = CONFIG_LOCATION) -> dict: + """Read the yml, apply the defaults, and reject a config that cannot work. + + Raises ValueError rather than exiting, so the daemon owns the exit status and + the rules stay testable off an OpenBSD host. + """ + cfg = safe_load(open(location)) or {} + for key, value in CONFIG_DEFAULTS.items(): + cfg.setdefault(key, value) + + missing = [f"{k} ({v})" for k, v in CONFIG_REQUIRED.items() if k not in cfg] + if missing: + raise ValueError( + "not found in the YAML Config File, please configure: " + + ", ".join(missing) + ) + + # Normalised and validated here because run() selects its network listener by + # exact match. Anything else matched neither branch, fell through to the + # shutdown path and exited 0 as though it had served, and a lowercase 'udp' + # also slipped past the ALLOW_INSECURE_UDP gate + cfg["SOCKET_PROTO"] = str(cfg["SOCKET_PROTO"]).strip().upper() + if cfg["SOCKET_PROTO"] not in ("TCP", "UDP"): + raise ValueError( + f"SOCKET_PROTO must be TCP or UDP, not '{cfg['SOCKET_PROTO']}'" + ) + + cfg["SOCKET_UNIX"] = str(cfg["SOCKET_UNIX"] or "").strip() + if cfg["SOCKET_UNIX"] and not cfg["SOCKET_UNIX"].startswith("/"): + # A relative path would be resolved against whatever directory rc.subr + # happened to start the daemon in + raise ValueError( + f"SOCKET_UNIX must be an absolute path, not '{cfg['SOCKET_UNIX']}'" + ) + if len(cfg["SOCKET_UNIX"].encode("utf-8")) > UNIX_PATH_MAX: + raise ValueError( + f"SOCKET_UNIX is {len(cfg['SOCKET_UNIX'])} bytes, over the " + f"{UNIX_PATH_MAX} byte limit on a socket path: '{cfg['SOCKET_UNIX']}'" + ) + + # A listener has to be asked for explicitly, one way or the other. Both may + # be set: a firewall can serve its own resolver over the local socket and a + # remote one - a CARP peer's - over the network at the same time. + # + # SOCKET_LISTEN is neither required nor defaulted to 0.0.0.0. The port it + # binds injects entries into the PF whitelist and is unauthenticated, so + # publishing it on every interface is never the right guess; leaving it out + # is how a same-host deployment says "local socket only". + if not cfg.get("SOCKET_LISTEN") and not cfg["SOCKET_UNIX"]: + raise ValueError( + "no listener configured: set SOCKET_LISTEN (the inside interface IP, " + "never 0.0.0.0) for remote resolvers, or SOCKET_UNIX for a resolver " + "on this host, or both" + ) + return cfg + + +def _key_lifetime(window: int, cfg: dict) -> int: + """Seconds to keep a Redis key alive, as a backstop only. + + scan_redis_db owns expiry; this just stops a key outliving the daemon that + would have swept it. One SCAN_PERIOD of slack keeps the key present for the + scan that should retire it, and the result is floored at 1 because Redis + treats EXPIRE 0 (or negative) as "delete now", which for a ttl of 0 would + discard the record before its own scan ever saw it. + """ + return max(int(window) + int(cfg["SCAN_PERIOD"]), 1) + + def db_push(logger, log: bool, db, table: str, data: list, kind: str, qname: str, cfg: dict = None): """Store IP(s) and metadata to Redis database table. @@ -62,6 +180,12 @@ def db_push(logger, log: bool, db, table: str, data: list, kind: str, qname: str ("rr") or an absolute Unbound cache-expiry timestamp ("cache"). The losing field is deleted because hmset merges, and a key holding both would be ambiguous to the expiry scan. + + The TTL is stored exactly as the sender reported it. There is no floor: the + protocol says a ttl of 0 means do-not-cache, so raising it here would have + authorised egress the answer explicitly asked not to keep, and any floor + contradicts the documented expiry rule. TTL_MULTIPLIER is the knob for + holding entries longer than the record says. """ if log: @@ -70,32 +194,31 @@ def db_push(logger, log: bool, db, table: str, data: list, kind: str, qname: str try: pipe = db.pipeline() now = int(time()) - for ip, ttl in data: + for ip, rr_ttl in data: key = f"{table}^{ip}" + rr_ttl = int(rr_ttl) if kind == "cache": pipe.hmset( key, - {"epoch": now, "kind": "cache", "expires": int(ttl), "qname": qname}, + {"epoch": now, "kind": "cache", "expires": rr_ttl, "qname": qname}, ) pipe.hdel(key, "ttl") if cfg: - pipe.expire(key, max(int(ttl) - now, 0) + int(cfg["SCAN_PERIOD"])) + pipe.expire(key, _key_lifetime(max(rr_ttl - now, 0), cfg)) else: pipe.hmset( key, { "epoch": now, "kind": "rr", - "ttl": max(int(ttl), 3600), # min ttl = 1 hour + "ttl": rr_ttl, "qname": qname, }, ) pipe.hdel(key, "expires") if cfg: pipe.expire( - key, - max(int(ttl), 3600) * int(cfg["TTL_MULTIPLIER"]) - + int(cfg["SCAN_PERIOD"]), + key, _key_lifetime(rr_ttl * int(cfg["TTL_MULTIPLIER"]), cfg) ) pipe.execute() return True @@ -165,6 +288,9 @@ def file_pop(logger, log: bool, file: str, ip_list: list): try: with _locked(file): with open(file, "r") as f: + # Read from the handle we are about to replace, so a tightened + # mode is carried over rather than reverted to a hardcoded one + mode = stat.S_IMODE(os.fstat(f.fileno()).st_mode) # sorted() keeps content stable, so an unchanged whitelist # produces an unchanged file. PF ignores line order. keep = sorted( @@ -179,7 +305,7 @@ def file_pop(logger, log: bool, file: str, ip_list: list): ) with os.fdopen(fd, "w") as t: t.write("".join(f"{ip}\n" for ip in keep)) - os.chmod(tmp, 0o640) # mkstemp creates 0600; keep the installed mode + os.chmod(tmp, mode) # mkstemp creates 0600, whatever the file was os.replace(tmp, file) # Atomic within one filesystem return True except Exception: @@ -291,12 +417,28 @@ def sync_pf_table(self): # retain an expired entry for one more cycle, which is harmless. # pfctl costs a subprocess, but this runs once per SCAN_PERIOD, not # per query (see DECISIONS.md) - entries = list( - subprocess.Popen( - ["pfctl", "-t", self.table, "-T", "show"], stdout=subprocess.PIPE - ).stdout + # + # The exit status is checked because a failing pfctl produces no + # output, which is indistinguishable from an empty table: the diff + # below would then find nothing to delete and try to re-add every + # live IP, so real table entries would never be expired and nothing + # would say why. Raising here lands in the handler below, which + # logs and skips the cycle. + shown = subprocess.run( + ["pfctl", "-t", self.table, "-T", "show"], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, ) - t_ips = [l.decode("utf-8").strip() for l in entries] + if shown.returncode != 0: + raise RuntimeError( + f"pfctl -t {self.table} -T show exited {shown.returncode}: " + f"{shown.stderr.decode('utf-8', 'replace').strip()}" + ) + t_ips = [ + l.strip() + for l in shown.stdout.decode("utf-8", "replace").splitlines() + if l.strip() + ] keys = list(self.db.scan_iter(match=f"{self.table}^*", count=500)) db_ips = [k.decode("utf-8").split("^")[1] for k in keys] @@ -388,67 +530,12 @@ def __init__(self, *args, **kwargs): self.threads = [] self.soc = None # UDP Listen Socket self.conn = None # TCP Listen Socket + self.unix = None # Local (AF_UNIX) Listen Socket self.db = None # Load YAML Configuration try: - self.cfg = safe_load(open(CONFIG_LOCATION)) - if "LOGGING" not in self.cfg: - self.cfg["LOGGING"] = True - if "LOG_LEVEL" not in self.cfg: - self.cfg["LOG_LEVEL"] = "DEBUG" - if "SOCKET_LISTEN" not in self.cfg: - self.cfg["SOCKET_LISTEN"] = "0.0.0.0" - if "SOCKET_PROTO" not in self.cfg: - self.cfg["SOCKET_PROTO"] = "TCP" - if "SOCKET_PORT" not in self.cfg: - self.cfg["SOCKET_PORT"] = 10001 - if "SOCKET_TIMEOUT" not in self.cfg: - self.cfg["SOCKET_TIMEOUT"] = 3 - if "SOCKET_BUFFER" not in self.cfg: - self.cfg["SOCKET_BUFFER"] = 1024 - if "SOCKET_BACKLOG" not in self.cfg: - self.cfg["SOCKET_BACKLOG"] = 128 - if "COMPRESS" not in self.cfg: - self.cfg["COMPRESS"] = True - if "MAX_WORKERS" not in self.cfg: - self.cfg["MAX_WORKERS"] = 32 - if "ALLOW_INSECURE_UDP" not in self.cfg: - self.cfg["ALLOW_INSECURE_UDP"] = False - if "REDIS_HOST" not in self.cfg: - self.cfg["REDIS_HOST"] = "127.0.0.1" - if "REDIS_PORT" not in self.cfg: - self.cfg["REDIS_PORT"] = 6379 - if "REDIS_DB" not in self.cfg: - self.cfg["REDIS_DB"] = 0 # Valid range is 0-15 - if "SCAN_PERIOD" not in self.cfg: - self.cfg["SCAN_PERIOD"] = 60 - if "TTL_MULTIPLIER" not in self.cfg: - self.cfg["TTL_MULTIPLIER"] = 2 - if "CTL" not in self.cfg: - self.cfg["CTL"] = "IOCTL" - if "DEVPF" not in self.cfg: - self.cfg["DEVPF"] = "/dev/pf" - if "AF4_TABLE" not in self.cfg: - print( - "AF4_TABLE (PF Table) not found in YAML Config File. Please configure. Exiting." - ) - sys.exit(2) - if "AF4_FILE" not in self.cfg: - print( - "AF4_FILE (PF Persist file) not found in YAML Config File. Please configure. Exiting." - ) - sys.exit(2) - if "AF6_TABLE" not in self.cfg: - print( - "AF6_TABLE (PF Table) not found in YAML Config File. Please configure. Exiting." - ) - sys.exit(2) - if "AF6_FILE" not in self.cfg: - print( - "AF6_FILE (PF Persist file) not found in YAML Config File. Please configure. Exiting." - ) - sys.exit(2) + self.cfg = load_config() except Exception as e: print(f"YAML Config File not found or cannot load. {e}") sys.exit(2) @@ -518,7 +605,11 @@ def run(self): self.pool = ThreadPoolExecutor(max_workers=workers) self.slots = BoundedSemaphore(self.max_inflight) - if self.cfg["SOCKET_PROTO"] == "UDP" and not self.cfg["ALLOW_INSECURE_UDP"]: + if ( + self.cfg["SOCKET_PROTO"] == "UDP" + and self.cfg.get("SOCKET_LISTEN") + and not self.cfg["ALLOW_INSECURE_UDP"] + ): self.logger.error( "PFUIFW: UDP mode is spoofable (a datagram source address is not " "verified) and is intended for lab use only. Set " @@ -527,121 +618,318 @@ def run(self): ) sys.exit(5) - # Listen for connections - # Default TCP time_wait = 60; 64000 / 60 = 1,066qps - # sysctl net.inet.tcp.keepidle=10; 64000 / 10 = 6,400qps - if self.cfg["SOCKET_PROTO"] == "TCP": - self.conn = socket(AF_INET, SOCK_STREAM) # TCP Stream Socket - self.conn.setsockopt(IPPROTO_TCP, TCP_NODELAY, True) # Disable Nagle - # self.conn.setsockopt(socket.SOL_TCP, 23, 5) - # 23 = TCP_FASTOPEN, 5 = Max TFO queue (not yet supported in OpenBSD) - self.conn.setsockopt( - SOL_SOCKET, SO_REUSEADDR, True - ) # Fast Listen Socket reuse - self.conn.settimeout( - self.cfg["SOCKET_TIMEOUT"] - ) # accept() connection timeout to check TERM - self.conn.bind((self.cfg["SOCKET_LISTEN"], self.cfg["SOCKET_PORT"])) - self.conn.listen(self.cfg["SOCKET_BACKLOG"]) - - while not self.got_sigterm(): # Watch Socket until TERM - try: - # Hand each accepted connection to the pool - conn, (ip, port) = self.conn.accept() # Waits self.conn.settimeout - self._prepare_conn(conn) - if not self.slots.acquire(blocking=False): - # Shed rather than queue: PF still denies the traffic, and - # PFUI_Unbound sees a socket failure instead of a stall - self.logger.error( - f"PFUIFW: At capacity ({self.max_inflight}), shedding {ip}:{port}" - ) - conn.close() - continue - try: - self._submit(proto="TCP", conn=conn, ip=ip, port=port) - except Exception: - self.slots.release() - conn.close() - self.logger.exception("PFUIFW: Error starting receiver thread") - except socket_timeout: - continue - except socket_error as e: - # accept() can fail transiently: ECONNABORTED when a peer - # goes away, EMFILE/ENFILE under fd pressure. Neither is a - # reason to stop serving. - self.logger.error(f"PFUIFW: accept() failed, continuing. {e}") - sleep(0.1) - continue - except Exception: - self.logger.exception("PFUIFW: Unexpected accept() error, continuing") - sleep(0.5) - continue - - # UDP is experimental and spoofable. The Unbound module runs per lookup, so - # each lookup is a fresh exchange; with UDP defaults that caps out near 213qps - elif self.cfg["SOCKET_PROTO"] == "UDP": - # setup listen socket - self.soc = socket(AF_INET, SOCK_DGRAM) # UDP Datagram Socket - self.soc.setsockopt(SOL_SOCKET, SO_REUSEADDR, True) - self.soc.settimeout( - self.cfg["SOCKET_TIMEOUT"] - ) # recvfrom() data timeout to check TERM - self.soc.bind((self.cfg["SOCKET_LISTEN"], self.cfg["SOCKET_PORT"])) - - while not self.got_sigterm(): # Watch Socket until TERM - try: - # KNOWN LIMITATION, UDP mode only: this buffer is the hard - # ceiling on a PFUI message over UDP. A larger datagram is - # truncated by the kernel, the decode then fails, and the - # answer is never whitelisted. Reading one byte past the - # ceiling is what makes that detectable instead of silent. - # - # Raising the buffer would not make large answers work: a - # datagram above the link MTU fragments, and PF commonly - # drops fragments. UDP mode therefore cannot carry the large - # answers TCP handles via MAX_MESSAGE, and this is one reason - # UDP is gated behind ALLOW_INSECURE_UDP. TCP has no such - # limit; use it. - dgram, (ip, port) = self.soc.recvfrom(UDP_DGRAM_CEILING + 1) - if len(dgram) > UDP_DGRAM_CEILING: - self.logger.error( - f"PFUIFW: Datagram from {ip}:{port} exceeds the " - f"{UDP_DGRAM_CEILING} byte UDP ceiling " - f"({len(dgram)}+ bytes); dropping. This answer cannot " - f"be whitelisted over UDP - switch SOCKET_PROTO to TCP." - ) - continue - # ACKDATA is sent by receiver_thread once the datagram has - # decoded to a valid PFUI structure, so this cannot be used - # as a blind reflector - except socket_timeout: - continue - except socket_error: - continue - except Exception as e: - self.logger.exception(f"PFUIFW: UDP socket exception {e}") - sleep(0.5) - continue - - if dgram: # Hand each datagram to the pool - if not self.slots.acquire(blocking=False): - self.logger.error( - f"PFUIFW: At capacity ({self.max_inflight}), dropping {ip}:{port}" - ) - continue - try: - self._submit(proto="UDP", dgram=dgram, ip=ip, port=port) - except Exception: - self.slots.release() - self.logger.exception("PFUIFW: Error in receiver thread") + # Bind every configured listener before serving any of them, so a bind + # failure is a startup failure rather than a thread that quietly died + servers = [] + try: + if self.cfg["SOCKET_UNIX"]: + self.unix = self._bind_unix(self.cfg["SOCKET_UNIX"]) + servers.append((self._serve_stream, ("UNIX", self.unix))) + if self.cfg.get("SOCKET_LISTEN"): + if self.cfg["SOCKET_PROTO"] == "TCP": + self.conn = self._bind_tcp() + servers.append((self._serve_stream, ("TCP", self.conn))) + else: + self.soc = self._bind_udp() + servers.append((self._serve_udp, (self.soc,))) + except Exception: + # _bind_unix exits by itself, and SystemExit is not an Exception, so + # this is the network listener failing after the local one succeeded: + # unlink it rather than leave a node for the next start to reclaim + self.logger.exception("PFUIFW: Failed to bind a listener") + self._remove_unix_socket() + sys.exit(6) + + if not servers: # load_config refuses this, so reaching it is a bug + self.logger.error("PFUIFW: No listener configured; nothing to serve.") + sys.exit(6) + + # One thread per listener rather than one poll loop over all of them: each + # loop already blocks only for SOCKET_TIMEOUT before re-checking for TERM, + # and the pool, the slot semaphore and the shed path are shared and + # thread-safe, so this keeps each accept path exactly as it was + listen_threads = [] + for target, args in servers: + thread = Thread(target=self._serve, args=(target, args), daemon=True) + thread.start() + listen_threads.append(thread) + for thread in listen_threads: + thread.join() # Shut down self.pool.shutdown(wait=True) for t in self.threads: t.join() + self._remove_unix_socket() self.db.close() self.logger.info("PFUIFW: [-] PFUI_Firewall Service Stopped.") + def _serve(self, target, args): + """Run one listener's loop, surviving anything it raises. + + A listener thread that died silently would leave the daemon running and + apparently healthy while nothing was being whitelisted on that transport. + """ + try: + target(*args) + except Exception: + self.logger.exception("PFUIFW: Listener loop failed") + + def _bind_tcp(self): + """Bind the network stream listener. + + Default TCP time_wait = 60; 64000 / 60 = 1,066qps + sysctl net.inet.tcp.keepidle=10; 64000 / 10 = 6,400qps + """ + conn = socket(AF_INET, SOCK_STREAM) # TCP Stream Socket + conn.setsockopt(IPPROTO_TCP, TCP_NODELAY, True) # Disable Nagle + # conn.setsockopt(socket.SOL_TCP, 23, 5) + # 23 = TCP_FASTOPEN, 5 = Max TFO queue (not yet supported in OpenBSD) + conn.setsockopt(SOL_SOCKET, SO_REUSEADDR, True) # Fast Listen Socket reuse + conn.settimeout( + self.cfg["SOCKET_TIMEOUT"] + ) # accept() connection timeout to check TERM + conn.bind((self.cfg["SOCKET_LISTEN"], self.cfg["SOCKET_PORT"])) + conn.listen(self.cfg["SOCKET_BACKLOG"]) + self.logger.info( + f"PFUIFW: [+] Listening on TCP " + f"{self.cfg['SOCKET_LISTEN']}:{self.cfg['SOCKET_PORT']}" + ) + return conn + + def _bind_udp(self): + """Bind the network datagram listener. + + UDP is experimental and spoofable. The Unbound module runs per lookup, so + each lookup is a fresh exchange; with UDP defaults that caps out near + 213qps. + """ + soc = socket(AF_INET, SOCK_DGRAM) # UDP Datagram Socket + soc.setsockopt(SOL_SOCKET, SO_REUSEADDR, True) + soc.settimeout( + self.cfg["SOCKET_TIMEOUT"] + ) # recvfrom() data timeout to check TERM + soc.bind((self.cfg["SOCKET_LISTEN"], self.cfg["SOCKET_PORT"])) + self.logger.info( + f"PFUIFW: [+] Listening on UDP " + f"{self.cfg['SOCKET_LISTEN']}:{self.cfg['SOCKET_PORT']}" + ) + return soc + + def _bind_unix(self, path): + """Bind the local stream listener for a resolver on this same host. + + The filesystem is the whole access control here, in place of the pf.conf + source restriction that guards the network listener, so this fails closed: + anything that would leave the socket reachable by more than the configured + group exits the daemon instead of serving. + """ + parent = os.path.dirname(path) or "/" + if not os.path.isdir(parent): + self.logger.error( + f"PFUIFW: SOCKET_UNIX directory {parent} does not exist; " + f"rc.d/pfui_firewall creates it on start." + ) + sys.exit(6) + + # A world-writable parent means anyone can replace the socket with their + # own and be handed the resolver's messages, whatever the socket's mode + parent_mode = os.stat(parent).st_mode + if parent_mode & stat.S_IWOTH and not parent_mode & stat.S_ISVTX: + self.logger.error( + f"PFUIFW: SOCKET_UNIX directory {parent} is world-writable " + f"({oct(stat.S_IMODE(parent_mode))}); refusing to bind there." + ) + sys.exit(6) + + self._reclaim_unix_socket(path) + + listener = socket(AF_UNIX, SOCK_STREAM) + listener.settimeout(self.cfg["SOCKET_TIMEOUT"]) # accept() checks TERM + # bind() creates the node with the process umask, so it is narrowed here + # rather than by a chmod afterwards: otherwise the socket is connectable + # by everyone for the moment in between + previous_umask = os.umask(UNIX_SOCKET_UMASK) + try: + listener.bind(path) + finally: + os.umask(previous_umask) + + try: + self._grant_unix_socket(path) + except Exception: + self.logger.exception( + f"PFUIFW: Cannot restrict {path} to group " + f"{self.cfg['SOCKET_UNIX_GROUP']}; refusing to serve on it." + ) + # Explicit path and listener: self.unix is not set until run() has the + # bound socket back, so a refused start would otherwise leave the node + # behind for the next start to reclaim + self._remove_unix_socket(path=path, listener=listener) + sys.exit(6) + + listener.listen(self.cfg["SOCKET_BACKLOG"]) + self.logger.info( + f"PFUIFW: [+] Listening on UNIX {path} " + f"(group {self.cfg['SOCKET_UNIX_GROUP']}, mode {oct(UNIX_SOCKET_MODE)})" + ) + return listener + + def _reclaim_unix_socket(self, path): + """Remove a socket file left behind by an unclean stop. + + bind() fails with EADDRINUSE on a leftover node, so it has to go, but only + once nothing answers on it: unlinking a live daemon's socket would leave + that daemon running and unreachable. + """ + if not os.path.exists(path): + return + probe = socket(AF_UNIX, SOCK_STREAM) + probe.settimeout(1) + try: + probe.connect(path) + except (ConnectionRefusedError, FileNotFoundError): + pass # Nothing is listening; the node is stale + except OSError as e: + self.logger.error( + f"PFUIFW: {path} exists and cannot be tested ({e}); " + f"remove it by hand if no PFUI_Firewall is running." + ) + sys.exit(6) + else: + self.logger.error( + f"PFUIFW: Another PFUI_Firewall is already listening on {path}." + ) + sys.exit(6) + finally: + probe.close() + self.logger.info(f"PFUIFW: Removing stale socket {path}") + os.unlink(path) + + def _grant_unix_socket(self, path): + """Hand the socket to SOCKET_UNIX_GROUP, and to nobody else.""" + group = self.cfg["SOCKET_UNIX_GROUP"] + gid = grp.getgrnam(str(group)).gr_gid # KeyError if the group is missing + os.chown(path, -1, gid) + os.chmod(path, UNIX_SOCKET_MODE) + granted = stat.S_IMODE(os.stat(path).st_mode) + if granted != UNIX_SOCKET_MODE: + raise OSError(f"{path} is mode {oct(granted)}, expected {oct(UNIX_SOCKET_MODE)}") + + def _remove_unix_socket(self, path=None, listener=None): + """Unlink our own socket on the way out, so the next start is clean. + + Defaults to the running daemon's socket; both are passed explicitly by the + bind path, which has to clean up a socket run() has not been handed yet. + """ + path = path or self.cfg.get("SOCKET_UNIX") + listener = listener if listener is not None else self.unix + if not path or listener is None: + return + try: + listener.close() + os.unlink(path) + except FileNotFoundError: + pass + except Exception: + self.logger.exception(f"PFUIFW: Could not remove {path}") + + def _serve_stream(self, kind, listener): + """Accept loop shared by the TCP and UNIX listeners. + + Both carry the same length-prefixed frames and get the same replies, so + the only difference is how a peer is named in the log: an address and port + for TCP, the socket path for UNIX, where accept() reports no peer at all. + """ + while not self.got_sigterm(): # Watch Socket until TERM + try: + # Hand each accepted connection to the pool + conn, address = listener.accept() # Waits listener.settimeout + if kind == "UNIX": + peer, ip, port = self.cfg["SOCKET_UNIX"], None, None + else: + ip, port = address + peer = f"{ip}:{port}" + self._prepare_conn(conn) + if not self.slots.acquire(blocking=False): + # Shed rather than queue: PF still denies the traffic, and + # PFUI_Unbound sees a socket failure instead of a stall + self.logger.error( + f"PFUIFW: At capacity ({self.max_inflight}), shedding {peer}" + ) + conn.close() + continue + try: + self._submit(proto=kind, conn=conn, peer=peer, ip=ip, port=port) + except Exception: + self.slots.release() + conn.close() + self.logger.exception("PFUIFW: Error starting receiver thread") + except socket_timeout: + continue + except socket_error as e: + # accept() can fail transiently: ECONNABORTED when a peer + # goes away, EMFILE/ENFILE under fd pressure. Neither is a + # reason to stop serving. + self.logger.error(f"PFUIFW: accept() failed, continuing. {e}") + sleep(0.1) + continue + except Exception: + self.logger.exception("PFUIFW: Unexpected accept() error, continuing") + sleep(0.5) + continue + + def _serve_udp(self, soc): + """Receive loop for the network datagram listener.""" + while not self.got_sigterm(): # Watch Socket until TERM + try: + # KNOWN LIMITATION, UDP mode only: this buffer is the hard + # ceiling on a PFUI message over UDP. A larger datagram is + # truncated by the kernel, the decode then fails, and the + # answer is never whitelisted. Reading one byte past the + # ceiling is what makes that detectable instead of silent. + # + # Raising the buffer would not make large answers work: a + # datagram above the link MTU fragments, and PF commonly + # drops fragments. UDP mode therefore cannot carry the large + # answers TCP handles via MAX_MESSAGE, and this is one reason + # UDP is gated behind ALLOW_INSECURE_UDP. TCP has no such + # limit; use it, or SOCKET_UNIX on the same host. + dgram, (ip, port) = soc.recvfrom(UDP_DGRAM_CEILING + 1) + if len(dgram) > UDP_DGRAM_CEILING: + self.logger.error( + f"PFUIFW: Datagram from {ip}:{port} exceeds the " + f"{UDP_DGRAM_CEILING} byte UDP ceiling " + f"({len(dgram)}+ bytes); dropping. This answer cannot " + f"be whitelisted over UDP - switch SOCKET_PROTO to TCP." + ) + continue + # ACKDATA is sent by receiver_thread once the datagram has + # decoded to a valid PFUI structure, so this cannot be used + # as a blind reflector + except socket_timeout: + continue + except socket_error: + continue + except Exception as e: + self.logger.exception(f"PFUIFW: UDP socket exception {e}") + sleep(0.5) + continue + + if dgram: # Hand each datagram to the pool + if not self.slots.acquire(blocking=False): + self.logger.error( + f"PFUIFW: At capacity ({self.max_inflight}), dropping {ip}:{port}" + ) + continue + try: + self._submit( + proto="UDP", dgram=dgram, peer=f"{ip}:{port}", ip=ip, port=port + ) + except Exception: + self.slots.release() + self.logger.exception("PFUIFW: Error in receiver thread") + def _prepare_conn(self, conn): """Apply per-connection options to a freshly accepted socket. @@ -652,10 +940,15 @@ def _prepare_conn(self, conn): is not guaranteed across platforms. """ conn.settimeout(float(self.cfg["SOCKET_TIMEOUT"])) - try: - conn.setsockopt(IPPROTO_TCP, TCP_NODELAY, True) - except Exception: # Not fatal; costs latency, not correctness - self.logger.exception("PFUIFW: Could not set TCP_NODELAY on connection") + # Nagle is a TCP algorithm and there is nothing to disable on a local + # socket, where setting it raises rather than being ignored + if conn.family in (AF_INET, AF_INET6): + try: + conn.setsockopt(IPPROTO_TCP, TCP_NODELAY, True) + except Exception: # Not fatal; costs latency, not correctness + self.logger.exception( + "PFUIFW: Could not set TCP_NODELAY on connection" + ) return conn def _submit(self, **kwargs): @@ -671,7 +964,8 @@ def _task_done(self, future): # so without this a recurring receiver failure would be silent self.logger.error(f"PFUIFW: Receiver task failed: {exc!r}", exc_info=exc) - def receiver_thread(self, proto, conn=None, dgram=None, ip=None, port=None): + def receiver_thread(self, proto, conn=None, dgram=None, peer="", ip=None, + port=None): """Receive all data, update PF Table and Redis DB Data Structure: {'kind': 'rr'|'cache', 'qname': qname, 'AF4': [{"ip": ipv4_addr, "ttl": ip_ttl}], 'AF6': [...]} @@ -679,7 +973,12 @@ def receiver_thread(self, proto, conn=None, dgram=None, ip=None, port=None): SOCKET_BUFFER is the read chunk size, not a message limit; message size is bounded by the protocol's MAX_MESSAGE. Raising it costs memory per connection and saves syscalls on large answers. + + 'peer' is how this sender is named in the log. AF_UNIX accept() reports + no peer address at all, so it cannot be derived here; ip and port stay + for the UDP reply, which is the only path that needs to address one. """ + peer = peer or f"{ip}:{port}" def disconnect(proto, soc, conn, msg): if msg: @@ -699,7 +998,7 @@ def disconnect(proto, soc, conn, msg): except Exception: pass # PFUI_Unbound may have closed socket already (non-blocking cache responses) # Do not soc.close(), as this stops the listening socket - elif proto == "TCP": + elif proto in ("TCP", "UNIX"): try: conn.sendall(msg) except Exception: @@ -712,7 +1011,9 @@ def disconnect(proto, soc, conn, msg): # Read data from network data = None - if proto == "TCP": + if proto in ("TCP", "UNIX"): + + received = [] def recv_exactly(n): """Read exactly n bytes; None if the peer closed first.""" @@ -722,29 +1023,49 @@ def recv_exactly(n): if not chunk: return None buf += chunk + received.append(len(buf)) return bytes(buf) try: data = read_frame(recv_exactly, compress=self.cfg["COMPRESS"]) - if data is None: + if data is None and not received: + # read_frame returns None when no header arrived at all, which + # PROTOCOL.md makes a non-error. A payload of JSON null also + # decodes to None, and calling that an empty payload was + # wrong: it falls through to the shape check below instead self.logger.error( - f"PFUIFW: Empty payload, disconnecting {ip}:{port}" + f"PFUIFW: Empty payload, disconnecting {peer}" ) disconnect(proto, self.soc, conn, msg="Empty payload") return except socket_timeout: - self.logger.error(f"PFUIFW: Socket recv timeout {ip}:{port}") + self.logger.error(f"PFUIFW: Socket recv timeout {peer}") disconnect(proto, self.soc, conn, msg="Socket timeout") return + except BadLength as e: + # Reported separately from a truncated payload, as PROTOCOL.md + # documents and server-c already distinguishes: a bad prefix is + # a different diagnosis from a sender that under-delivered + self.logger.error( + f"PFUIFW: Bad frame length from {peer}, disconnecting. {e}" + ) + disconnect(proto, self.soc, conn, msg="Bad length") + return + except Truncated as e: + self.logger.error( + f"PFUIFW: Truncated payload from {peer}, disconnecting. {e}" + ) + disconnect(proto, self.soc, conn, msg="Truncated") + return except WireError as e: self.logger.error( - f"PFUIFW: Bad frame from {ip}:{port}, disconnecting. {e}" + f"PFUIFW: Bad frame from {peer}, disconnecting. {e}" ) disconnect(proto, self.soc, conn, msg="Bad frame") return except Exception: self.logger.exception( - f"PFUIFW: Failed to decode stream, disconnecting {ip}:{port}" + f"PFUIFW: Failed to decode stream, disconnecting {peer}" ) disconnect(proto, self.soc, conn, msg="Failed to decode") return @@ -756,19 +1077,30 @@ def recv_exactly(n): data = decode(dgram, compress=self.cfg["COMPRESS"]) except Exception: self.logger.exception( - f"PFUIFW: Failed to decode datagram {ip}:{port} {dgram}" + f"PFUIFW: Failed to decode datagram {peer} {dgram}" ) disconnect(proto, self.soc, conn, "Failed to decode") return if self.stats: ntime = time() - self.logger.info(f"PFUIFW: Received {data} from {ip}:{port} ({proto})") + self.logger.info(f"PFUIFW: Received {data} from {peer} ({proto})") # Input Request + # Shape first, then contents: a non-dict payload has no 'kind' to be + # missing, and reporting one as a version skew sent the wrong operator + # down the wrong path af4_data, af6_data = [], [] - kind = data.get("kind") if isinstance(data, dict) else None - qname = data.get("qname", "") if isinstance(data, dict) else "" + if not isinstance(data, dict): + self.logger.error( + f"PFUIFW: Message decoded to {type(data).__name__}, not a PFUI " + f"object. Dropping. Non-PFUI_Unbound sender ?" + ) + disconnect(proto, self.soc, conn, msg="Invalid datatype") + return False + + kind = data.get("kind") + qname = data.get("qname", "") if kind not in ("rr", "cache"): self.logger.error( f"PFUIFW: Message has no valid 'kind' ({kind}), dropping. " @@ -776,24 +1108,20 @@ def recv_exactly(n): ) disconnect(proto, self.soc, conn, msg="Missing kind") return False - if isinstance(data, dict): - try: - af4_data = extract(data.get("AF4"), version=4) - af6_data = extract(data.get("AF6"), version=6) - except Exception: - self.logger.exception( - f"PFUIFW: Cannot extract PFUI record from data '{data}' {type(data)}" - ) - else: - self.logger.error(f"PFUIFW: No data in message. Dropping message") - disconnect(proto, self.soc, conn, msg="No data") - return False + + try: + af4_data = extract(data.get("AF4"), version=4) + af6_data = extract(data.get("AF6"), version=6) + except Exception: + self.logger.exception( + f"PFUIFW: Cannot extract PFUI record from data '{data}' {type(data)}" + ) if not af4_data and not af6_data: self.logger.error( - f"PFUIFW: Invalid datatype received {data} {type(data)}. Non-PFUI_Unbound datagram ?" + f"PFUIFW: No routable records in {data}. Nothing to act on." ) - disconnect(proto, self.soc, conn, msg="Invalid datatype") + disconnect(proto, self.soc, conn, msg="No records") return False if proto == "UDP": @@ -801,7 +1129,7 @@ def recv_exactly(n): try: self.soc.sendto(b"ACKDATA", (ip, port)) except Exception: - self.logger.exception(f"PFUIFW: Failed to ACKDATA {ip}:{port}") + self.logger.exception(f"PFUIFW: Failed to ACKDATA {peer}") if self.stats: vtime = time() diff --git a/server-python/pfui_firewall.yml b/server-python/pfui_firewall.yml index b10ed42..52c873f 100644 --- a/server-python/pfui_firewall.yml +++ b/server-python/pfui_firewall.yml @@ -7,7 +7,23 @@ LOGGING: False # Enable verbose logging ('True'/'False') LOG_LEVEL: DEBUG # 'DEBUG', 'INFO' (testing), 'ERROR' (production) # Networking -SOCKET_LISTEN: 10.10.1.254 # Interface(s) to listen on (SET TO INSIDE INTERFACE IP) +# At least one listener must be configured, SOCKET_LISTEN or SOCKET_UNIX. Both may +# be, and a CARP node normally wants both: its own resolver over the local socket, +# its peer's resolver over the network. +SOCKET_LISTEN: 10.10.1.254 # Interface to listen on for remote resolvers (SET TO INSIDE INTERFACE IP). + # Never defaulted to 0.0.0.0: this port injects PF whitelist entries and is + # unauthenticated. Omit it entirely for a local-socket-only deployment +SOCKET_PROTO: TCP # Network transport, TCP (recommended) or UDP. Anything else refuses to + # start. Does not apply to SOCKET_UNIX, which is always a stream + +# Local socket, for a PFUI_Unbound resolver running on THIS host. Faster than +# loopback TCP (no handshake, no TIME_WAIT, no ephemeral port ceiling) and needs +# no pf.conf rule, because there is no packet to filter. Empty disables it. +#SOCKET_UNIX: /var/run/pfui/pfui_firewall.sock +SOCKET_UNIX_GROUP: _pfui # Group permitted to connect. There is no PF source rule on this transport, + # so this group and the socket's 0660 mode are the whole access control. + # The resolver's account (_unbound) must be a member; both installers + # manage this. Set the socket path above to enable it SOCKET_PORT: 10001 # Port to listen on (Permit inbound access to this port from PFUI_Unbound instances) SOCKET_TIMEOUT: 3 # Timeout on Socket session between PFUI_Unbound and PFUI_Firewall (keep small) SOCKET_BUFFER: 1024 # Read chunk size, not a message limit (the protocol caps a message at 1 MiB). @@ -23,7 +39,10 @@ REDIS_HOST: 127.0.0.1 # IP for Redis Server REDIS_PORT: 6379 # Port for Redis Server REDIS_DB: 9 # Redis Database ID Number (0-15) SCAN_PERIOD: 300 # Seconds between PF Table Scans (scrub expired entries from PF, Persist File & Redis) -TTL_MULTIPLIER: 4 # Expire entries after RR TTL * TTL_MULTIPLIER (Browsers tend to cache longer than TTL) +TTL_MULTIPLIER: 4 # Expire entries after RR TTL * TTL_MULTIPLIER (Browsers tend to cache longer than TTL). + # This is the only knob for holding entries longer than the record says: the TTL + # itself is stored exactly as the resolver reported it, including the 0 that + # means do-not-cache # PF Tables & Files CTL: IOCTL # IOCTL = ioctl kernel interface (recommended, requires DEVPF), PFCTL = pfctl cli interface diff --git a/server-python/rc.d/pfui_firewall b/server-python/rc.d/pfui_firewall index 0d3cbf9..79445b5 100755 --- a/server-python/rc.d/pfui_firewall +++ b/server-python/rc.d/pfui_firewall @@ -14,7 +14,17 @@ pexp="/usr/local/bin/python3 ${daemon}.*" rc_pre() { # /var/run is root-owned, and OpenBSD's MAKEDEV resets /dev/pf's mode on # release upgrades, so both are re-established on every start - install -d -o _pfui_firewall -g _pfui_firewall -m 755 /var/run/pfui + # + # Group _pfui when it exists: the local socket is created in this directory + # and, with BSD group inheritance, takes the directory's group. 0750 also + # means only that group can traverse to reach the socket at all, so the + # directory is a second gate in front of the socket's own 0660. Falls back to + # the old ownership so an install predating the _pfui group still starts. + if groupinfo _pfui >/dev/null 2>&1; then + install -d -o _pfui_firewall -g _pfui -m 750 /var/run/pfui + else + install -d -o _pfui_firewall -g _pfui_firewall -m 755 /var/run/pfui + fi chgrp _pfui_firewall /dev/pf && chmod 660 /dev/pf } diff --git a/server-python/tests/test_config.py b/server-python/tests/test_config.py new file mode 100644 index 0000000..3c351d8 --- /dev/null +++ b/server-python/tests/test_config.py @@ -0,0 +1,155 @@ +"""Configuration rules for the daemon. + +Every key the daemon reads must either have a default or stop the daemon. The +two that matter most are the socket keys: an unset SOCKET_LISTEN used to publish +an unauthenticated whitelist-injection port on every interface, and an +unrecognised SOCKET_PROTO used to match neither listener branch, fall through to +the shutdown path, and exit 0 as though the daemon had served. +""" + +from pathlib import Path + +import pytest +import yaml + +from test_file_store import fw # reuses the daemon-import shim + +COMPONENT = Path(__file__).resolve().parent.parent + + +def write(tmp_path, mapping): + path = tmp_path / "pfui_firewall.yml" + path.write_text(yaml.safe_dump(mapping)) + return str(path) + + +def complete(): + """The minimum a working deployment sets.""" + return { + "SOCKET_LISTEN": "10.10.1.254", + "AF4_TABLE": "pfui_ipv4_domains", + "AF4_FILE": "/var/spool/pfui/pfui_ipv4_domains", + "AF6_TABLE": "pfui_ipv6_domains", + "AF6_FILE": "/var/spool/pfui/pfui_ipv6_domains", + } + + +def tables_only(): + """A config with the PF tables but no listener of either kind.""" + cfg = complete() + del cfg["SOCKET_LISTEN"] + return cfg + + +def test_shipped_config_loads(): + """The example config must satisfy its own daemon.""" + cfg = fw.load_config(str(COMPONENT / "pfui_firewall.yml")) + assert cfg["SOCKET_PROTO"] == "TCP" + assert cfg["SOCKET_LISTEN"] + + +def test_every_key_read_at_runtime_has_a_default(tmp_path): + cfg = fw.load_config(write(tmp_path, complete())) + for key in fw.CONFIG_DEFAULTS: + assert key in cfg, f"{key} is read at runtime but has no default" + + +@pytest.mark.parametrize("key", sorted(fw.CONFIG_REQUIRED)) +def test_required_keys_are_refused_when_missing(tmp_path, key): + """Including SOCKET_LISTEN: the table and file keys always hard-exited when + absent, but the listen address silently became 0.0.0.0.""" + cfg = complete() + del cfg[key] + with pytest.raises(ValueError) as raised: + fw.load_config(write(tmp_path, cfg)) + assert key in str(raised.value) + + +def test_no_listener_of_either_kind_is_refused(tmp_path): + """A daemon with nothing listening would start, serve nothing and look fine.""" + with pytest.raises(ValueError) as raised: + fw.load_config(write(tmp_path, tables_only())) + assert "no listener" in str(raised.value) + + +def test_listen_address_is_never_defaulted_to_all_interfaces(tmp_path): + """The invariant that outlives SOCKET_LISTEN becoming optional: a config that + does not name an address must not end up bound to every interface.""" + cfg = tables_only() + cfg["SOCKET_UNIX"] = "/var/run/pfui/pfui_firewall.sock" + loaded = fw.load_config(write(tmp_path, cfg)) + assert not loaded.get("SOCKET_LISTEN") + assert "SOCKET_LISTEN" not in fw.CONFIG_DEFAULTS + + +def test_local_socket_alone_is_a_complete_configuration(tmp_path): + """A resolver on the firewall itself needs no network listener at all.""" + cfg = tables_only() + cfg["SOCKET_UNIX"] = "/var/run/pfui/pfui_firewall.sock" + loaded = fw.load_config(write(tmp_path, cfg)) + assert loaded["SOCKET_UNIX"] == "/var/run/pfui/pfui_firewall.sock" + + +def test_network_listener_alone_is_still_a_complete_configuration(tmp_path): + loaded = fw.load_config(write(tmp_path, complete())) + assert loaded["SOCKET_UNIX"] == "" # Disabled unless asked for + + +def test_both_listeners_together_are_accepted(tmp_path): + """A CARP node serves its own resolver locally and its peer's over TCP.""" + cfg = complete() + cfg["SOCKET_UNIX"] = "/var/run/pfui/pfui_firewall.sock" + loaded = fw.load_config(write(tmp_path, cfg)) + assert loaded["SOCKET_LISTEN"] and loaded["SOCKET_UNIX"] + + +@pytest.mark.parametrize("path", ["pfui.sock", "var/run/pfui.sock", "./pfui.sock"]) +def test_relative_socket_path_is_refused(tmp_path, path): + """It would be resolved against whatever directory rc.subr started us in.""" + cfg = tables_only() + cfg["SOCKET_UNIX"] = path + with pytest.raises(ValueError): + fw.load_config(write(tmp_path, cfg)) + + +def test_socket_group_defaults_to_the_shared_pfui_group(tmp_path): + cfg = tables_only() + cfg["SOCKET_UNIX"] = "/var/run/pfui/pfui_firewall.sock" + assert fw.load_config(write(tmp_path, cfg))["SOCKET_UNIX_GROUP"] == "_pfui" + + +def test_udp_gate_does_not_apply_to_a_local_socket_only_daemon(tmp_path): + """SOCKET_PROTO describes the network listener. With no SOCKET_LISTEN there is + no datagram socket to be spoofed, so a stale SOCKET_PROTO: UDP must not block + a local-socket deployment from starting.""" + cfg = tables_only() + cfg["SOCKET_UNIX"] = "/var/run/pfui/pfui_firewall.sock" + cfg["SOCKET_PROTO"] = "UDP" + loaded = fw.load_config(write(tmp_path, cfg)) + assert loaded["SOCKET_PROTO"] == "UDP" and not loaded.get("SOCKET_LISTEN") + + +@pytest.mark.parametrize("value,expected", [(" tcp ", "TCP"), ("udp", "UDP")]) +def test_socket_proto_is_normalised(tmp_path, value, expected): + """Normalising is what keeps 'udp' behind the ALLOW_INSECURE_UDP gate, which + also matches on the exact string.""" + cfg = complete() + cfg["SOCKET_PROTO"] = value + assert fw.load_config(write(tmp_path, cfg))["SOCKET_PROTO"] == expected + + +@pytest.mark.parametrize("value", ["SCTP", "tcp6", "", "both", 4]) +def test_unsupported_socket_proto_is_refused(tmp_path, value): + cfg = complete() + cfg["SOCKET_PROTO"] = value + with pytest.raises(ValueError): + fw.load_config(write(tmp_path, cfg)) + + +def test_empty_config_file_is_refused(tmp_path): + """safe_load returns None for an empty document; the required keys still + have to be reported rather than the daemon crashing on a None subscript.""" + path = tmp_path / "pfui_firewall.yml" + path.write_text("") + with pytest.raises(ValueError): + fw.load_config(str(path)) diff --git a/server-python/tests/test_conn_options.py b/server-python/tests/test_conn_options.py index 1d059c7..52b75d2 100644 --- a/server-python/tests/test_conn_options.py +++ b/server-python/tests/test_conn_options.py @@ -63,7 +63,10 @@ def test_prepare_conn_disables_nagle(accepted): which is not guaranteed across platforms.""" accepted.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 0) Daemon()._prepare_conn(accepted) - assert accepted.getsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY) == 1 + # Any non-zero value means the option is on. The kernel is free to report + # its own flag bits rather than the 1 that was set: Darwin returns 4, so + # asserting equality made this suite red everywhere but Linux + assert accepted.getsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY) != 0 def test_stalled_peer_hits_the_timeout_instead_of_blocking(accepted): diff --git a/server-python/tests/test_db_push.py b/server-python/tests/test_db_push.py new file mode 100644 index 0000000..a3175d0 --- /dev/null +++ b/server-python/tests/test_db_push.py @@ -0,0 +1,128 @@ +"""What db_push records for one answer. + +The TTL is the whole expiry decision, so what is stored has to be what the sender +said. An undocumented one-hour floor used to be applied to every RR TTL, which +contradicted the expiry rule in PROTOCOL.md and, with the shipped +TTL_MULTIPLIER: 4, turned a 60 second answer into four hours of authorised +egress. It also raised the ttl of 0 that means do-not-cache, the one value +validate.extract goes out of its way to preserve. +""" + +import pytest + +from pfui.store import is_expired +from test_file_store import Log, fw + +CFG = {"SCAN_PERIOD": 300, "TTL_MULTIPLIER": 4} + + +class FakePipeline: + """Records the commands the daemon queues, without a Redis.""" + + def __init__(self): + self.hmset_calls = {} + self.hdel_calls = [] + self.expire_calls = {} + self.executed = False + + def hmset(self, key, mapping): + self.hmset_calls.setdefault(key, {}).update(mapping) + + def hdel(self, key, field): + self.hdel_calls.append((key, field)) + + def expire(self, key, seconds): + self.expire_calls[key] = seconds + + def execute(self): + self.executed = True + + +class FakeRedis: + def __init__(self): + self.pipe = FakePipeline() + + def pipeline(self): + return self.pipe + + +def push(data, kind, cfg=CFG): + db = FakeRedis() + assert fw.db_push(Log(), False, db, "t", data, kind, "example.com.", cfg) is True + assert db.pipe.executed + return db.pipe + + +@pytest.mark.parametrize("ttl", [0, 1, 30, 60, 3599, 3600, 604800]) +def test_rr_ttl_is_stored_exactly_as_sent(ttl): + pipe = push([("8.8.8.8", ttl)], "rr") + assert pipe.hmset_calls["t^8.8.8.8"]["ttl"] == ttl + + +def test_short_ttl_is_not_raised_to_an_hour(): + """A 60 second answer must expire on its own TTL, not on a hidden floor.""" + pipe = push([("8.8.8.8", 60)], "rr") + meta = pipe.hmset_calls["t^8.8.8.8"] + assert meta["ttl"] == 60 + assert meta["kind"] == "rr" + # The recorded window is the sender's TTL times the multiplier, plus one + # scan period of slack for the sweep that retires it + assert pipe.expire_calls["t^8.8.8.8"] == 60 * 4 + 300 + + +def test_do_not_cache_answer_keeps_its_zero_ttl(): + """ttl 0 is valid and means do-not-cache, so the entry must expire at the + next scan rather than be held for an hour times the multiplier.""" + pipe = push([("8.8.8.8", 0)], "rr") + assert pipe.hmset_calls["t^8.8.8.8"]["ttl"] == 0 + assert is_expired( + {b"kind": b"rr", b"epoch": b"1000", b"ttl": b"0"}, now=1000, multiplier=4 + ) + + +def test_zero_ttl_key_is_not_deleted_by_its_own_backstop(): + """Redis treats EXPIRE 0 as delete-now, which would drop the record before + the scan that should retire it ever ran.""" + pipe = push([("8.8.8.8", 0)], "rr") + assert pipe.expire_calls["t^8.8.8.8"] > 0 + + +def test_backstop_is_never_non_positive_even_with_a_zero_scan_period(): + pipe = push([("8.8.8.8", 0)], "rr", cfg={"SCAN_PERIOD": 0, "TTL_MULTIPLIER": 1}) + assert pipe.expire_calls["t^8.8.8.8"] >= 1 + + +def test_cache_entry_stores_the_absolute_expiry_and_drops_the_ttl_field(): + """hmset merges, so the field the other kind uses has to go.""" + pipe = push([("8.8.8.8", 1675846179 + 3600)], "cache") + meta = pipe.hmset_calls["t^8.8.8.8"] + assert meta["kind"] == "cache" + assert meta["expires"] == 1675846179 + 3600 + assert ("t^8.8.8.8", "ttl") in pipe.hdel_calls + + +def test_rr_entry_drops_the_expires_field(): + pipe = push([("8.8.8.8", 3600)], "rr") + assert ("t^8.8.8.8", "expires") in pipe.hdel_calls + + +def test_already_expired_cache_entry_still_gets_a_positive_backstop(): + pipe = push([("8.8.8.8", 1)], "cache") # epoch 1 is long past + assert pipe.expire_calls["t^8.8.8.8"] > 0 + + +def test_qname_is_recorded_per_key(): + pipe = push([("8.8.8.8", 60), ("1.1.1.1", 60)], "rr") + for key in ("t^8.8.8.8", "t^1.1.1.1"): + assert pipe.hmset_calls[key]["qname"] == "example.com." + + +def test_redis_failure_returns_false(): + class Broken: + def pipeline(self): + raise ConnectionError("redis down") + + assert ( + fw.db_push(Log(), False, Broken(), "t", [("8.8.8.8", 60)], "rr", "a.", CFG) + is False + ) diff --git a/server-python/tests/test_file_store.py b/server-python/tests/test_file_store.py index dc6960c..affe6a8 100644 --- a/server-python/tests/test_file_store.py +++ b/server-python/tests/test_file_store.py @@ -75,11 +75,14 @@ def test_pop_dedupes_remaining_entries(persist): assert lines(persist) == ["1.1.1.1", "8.8.8.8"] -def test_pop_preserves_file_mode(persist): - """mkstemp creates 0600; the replacement must keep the installed mode.""" - os.chmod(persist, 0o640) +@pytest.mark.parametrize("mode", [0o600, 0o640, 0o644, 0o660]) +def test_pop_preserves_file_mode(persist, mode): + """mkstemp creates 0600, so the replacement has to carry the mode over. It + used to be hardcoded to 0640, which reverted a tightened permission on the + next scan and loosened a 0600 file - and these files are PF's whitelist.""" + os.chmod(persist, mode) fw.file_pop(Log(), False, persist, ["1.1.1.1"]) - assert oct(os.stat(persist).st_mode & 0o777) == oct(0o640) + assert oct(os.stat(persist).st_mode & 0o777) == oct(mode) def test_pop_leaves_no_temp_files(persist): diff --git a/server-python/tests/test_receiver.py b/server-python/tests/test_receiver.py new file mode 100644 index 0000000..7ef9821 --- /dev/null +++ b/server-python/tests/test_receiver.py @@ -0,0 +1,185 @@ +"""What the daemon replies to a message it will not act on. + +PROTOCOL.md makes each refusal a distinct short reason, because the reason is the +only diagnostic a client gets. Two of them were unreachable: the frame layer +collapsed a bad length and a truncated payload into one "Bad frame", and 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. +""" + +import pytest + +from pfui_wire import HEADER, MAX_MESSAGE, encode, encode_payload +from test_file_store import fw + +COMPRESS = False + + +class Recorder: + def __init__(self): + self.lines = [] + + def info(self, msg, *a, **k): + self.lines.append(str(msg)) + + def error(self, msg, *a, **k): + self.lines.append(str(msg)) + + def exception(self, msg, *a, **k): + self.lines.append(str(msg)) + + +class FakeConn: + """One accepted TCP connection, fed from a byte string.""" + + def __init__(self, blob): + self.blob = blob + self.sent = b"" + self.closed = False + + def recv(self, n): + chunk, self.blob = self.blob[:n], self.blob[n:] + return chunk + + def sendall(self, data): + self.sent += data + + def close(self): + self.closed = True + + +class Daemon: + """Enough of PFUI_Firewall to run receiver_thread with nothing installed.""" + + receiver_thread = fw.PFUI_Firewall.receiver_thread + + def __init__(self): + self.logger = Recorder() + self.soc = None + self.db = None # db_push is recorded, not performed + self.stats = False + self.cfg = { + "LOGGING": False, + "COMPRESS": COMPRESS, + "SOCKET_BUFFER": 1024, + "AF4_TABLE": "pfui_ipv4_domains", + "AF6_TABLE": "pfui_ipv6_domains", + "AF4_FILE": "/nonexistent/af4", + "AF6_FILE": "/nonexistent/af6", + } + + +@pytest.fixture +def daemon(monkeypatch): + """Records the PF, Redis and file writes instead of performing them.""" + performed = {"table": [], "db": [], "file": []} + monkeypatch.setattr(fw, "table_push", lambda **kw: performed["table"].append(kw)) + monkeypatch.setattr(fw, "db_push", lambda **kw: performed["db"].append(kw)) + monkeypatch.setattr(fw, "file_push", lambda **kw: performed["file"].append(kw)) + d = Daemon() + d.performed = performed + return d + + +def deliver(daemon, blob): + """Push raw bytes at the TCP receive path; returns what the daemon replied.""" + conn = FakeConn(blob) + daemon.receiver_thread(proto="TCP", conn=conn, ip="10.10.1.1", port=54321) + assert conn.closed, "the connection was left open" + return conn.sent + + +def test_a_valid_message_is_acknowledged(daemon): + msg = {"kind": "rr", "qname": "a.", "AF4": [{"ip": "8.8.8.8", "ttl": 60}], "AF6": []} + assert deliver(daemon, encode(msg, compress=COMPRESS)) == b"ACKUPDATE" + assert [kw["ip_list"] for kw in daemon.performed["table"]] == [["8.8.8.8"]] + assert daemon.performed["db"] and daemon.performed["file"] + + +def test_zero_declared_length_is_refused_as_a_bad_length(daemon): + assert deliver(daemon, HEADER.pack(0)) == b"Bad length" + + +def test_oversize_declared_length_is_refused_as_a_bad_length(daemon): + """Refused from the prefix alone, before any payload byte is buffered.""" + assert deliver(daemon, HEADER.pack(MAX_MESSAGE + 1) + b"x" * 16) == b"Bad length" + + +def test_short_payload_is_refused_as_truncated(daemon): + """A sender that declared bytes it did not send is a different fault from a + bad prefix, and gets its own reason.""" + assert deliver(daemon, HEADER.pack(64) + b"x" * 8) == b"Truncated" + + +def test_peer_that_closes_before_a_header_gets_the_empty_payload_reason(daemon): + assert deliver(daemon, b"") == b"Empty payload" + + +def test_garbage_payload_is_refused_as_undecodable(daemon): + payload = b"\xde\xad\xbe\xef" * 8 + assert deliver(daemon, HEADER.pack(len(payload)) + payload) == b"Failed to decode" + + +@pytest.mark.parametrize("payload", ["[]", '"a string"', "42", "null"]) +def test_payload_that_is_not_a_message_object_is_an_invalid_datatype(daemon, payload): + """Reported as the wrong shape, not as a missing 'kind': a list has no kind + to be missing, and blaming a version skew sent the operator the wrong way.""" + blob = encode_payload(payload_as_json(payload), compress=COMPRESS) + reply = deliver(daemon, HEADER.pack(len(blob)) + blob) + assert reply == b"Invalid datatype" + assert daemon.performed["table"] == [] + + +def payload_as_json(text): + import json + + return json.loads(text) + + +def test_message_without_kind_is_refused(daemon): + msg = {"qname": "a.", "AF4": [{"ip": "8.8.8.8", "ttl": 60}], "AF6": []} + assert deliver(daemon, encode(msg, compress=COMPRESS)) == b"Missing kind" + + +def test_message_with_an_unrecognised_kind_is_refused(daemon): + msg = {"kind": "guess", "AF4": [{"ip": "8.8.8.8", "ttl": 60}], "AF6": []} + assert deliver(daemon, encode(msg, compress=COMPRESS)) == b"Missing kind" + + +def test_well_formed_message_with_no_records_is_refused(daemon): + msg = {"kind": "rr", "qname": "a.", "AF4": [], "AF6": []} + assert deliver(daemon, encode(msg, compress=COMPRESS)) == b"No records" + + +def test_message_whose_only_records_are_non_global_is_refused(daemon): + """Nothing survives validation, so there is nothing to act on.""" + msg = { + "kind": "rr", + "qname": "a.", + "AF4": [{"ip": "10.0.0.1", "ttl": 60}, {"ip": "0.0.0.0", "ttl": 60}], + "AF6": [{"ip": "::1", "ttl": 60}], + } + assert deliver(daemon, encode(msg, compress=COMPRESS)) == b"No records" + assert daemon.performed["table"] == [] + + +def test_every_refusal_reason_is_documented(): + """The reasons are a protocol surface, so PROTOCOL.md has to list each one.""" + from pathlib import Path + + spec = ( + Path(__file__).resolve().parents[2] / "protocol" / "PROTOCOL.md" + ).read_text() + for reason in ( + "Missing kind", + "Bad frame", + "Bad length", + "Truncated", + "Failed to decode", + "Invalid datatype", + "No records", + "Empty payload", + "Socket timeout", + ): + assert f"`{reason}`" in spec, f"{reason} is sent but not documented" diff --git a/server-python/tests/test_sync.py b/server-python/tests/test_sync.py new file mode 100644 index 0000000..aabbea0 --- /dev/null +++ b/server-python/tests/test_sync.py @@ -0,0 +1,145 @@ +"""ScanSync's diff against the live PF table. + +The sync deletes what is in the table but not in Redis, so it can only ever run +on a complete read. A failing `pfctl -T show` produces no output, which used to +be indistinguishable from an empty table: the diff then found nothing to expire +and tried to re-add every live IP, so a broken pfctl left the table unmanaged +with nothing in the log saying so. +""" + +import subprocess + +import pytest + +from test_file_store import Log, fw + + +class Recorder(Log): + def __init__(self): + self.exceptions = [] + self.errors = [] + + def exception(self, msg, *a, **k): + self.exceptions.append(str(msg)) + + def error(self, msg, *a, **k): + self.errors.append(str(msg)) + + +class FakeRedis: + def __init__(self, ips): + self.ips = ips + + def scan_iter(self, match, count=None): + table = match.split("^")[0] + return [f"{table}^{ip}".encode() for ip in self.ips] + + +class Completed: + """subprocess.CompletedProcess stand-in, so no pfctl is needed.""" + + def __init__(self, returncode, stdout=b"", stderr=b""): + self.returncode, self.stdout, self.stderr = returncode, stdout, stderr + + +@pytest.fixture +def syncer(monkeypatch): + """A ScanSync wired to fakes, with the PF mutations recorded not performed.""" + pushed, popped = [], [] + monkeypatch.setattr(fw, "table_push", lambda **kw: pushed.append(kw["ip_list"])) + monkeypatch.setattr(fw, "table_pop", lambda **kw: popped.append(kw["ip_list"])) + + def make(db_ips, pfctl_result): + monkeypatch.setattr( + fw.subprocess, "run", lambda *a, **k: pfctl_result + ) + sync = fw.ScanSync.__new__(fw.ScanSync) # No thread, no Redis connection + sync.logger = Recorder() + sync.cfg = {"LOGGING": False, "REDIS_DB": 9, "SCAN_PERIOD": 300} + sync.db = FakeRedis(db_ips) + sync.af = 2 + sync.table = "pfui_ipv4_domains" + sync.file = "/nonexistent" + return sync, pushed, popped + + return make + + +def test_expired_table_entry_is_removed(syncer): + sync, pushed, popped = syncer( + db_ips=["1.1.1.1"], pfctl_result=Completed(0, b" 1.1.1.1\n 8.8.8.8\n") + ) + sync.sync_pf_table() + assert popped == [["8.8.8.8"]] # In the table, no Redis record + assert pushed == [] + + +def test_missing_table_entry_is_added(syncer): + sync, pushed, popped = syncer( + db_ips=["1.1.1.1", "8.8.8.8"], pfctl_result=Completed(0, b" 1.1.1.1\n") + ) + sync.sync_pf_table() + assert pushed == [["8.8.8.8"]] + assert popped == [] + + +def test_a_failing_pfctl_does_not_look_like_an_empty_table(syncer): + """The defect: a non-zero exit with no output meant every Redis IP looked + missing from the table, and every table entry looked unmanageable.""" + sync, pushed, popped = syncer( + db_ips=["1.1.1.1"], + pfctl_result=Completed(1, b"", b"pfctl: Table does not exist.\n"), + ) + sync.sync_pf_table() + assert pushed == [], "pushed to the table on the strength of a failed read" + assert popped == [], "deleted from the table on the strength of a failed read" + + +def test_a_failing_pfctl_is_logged(syncer): + sync, _, _ = syncer( + db_ips=["1.1.1.1"], + pfctl_result=Completed(77, b"", b"pfctl: /dev/pf: Permission denied.\n"), + ) + sync.sync_pf_table() + assert sync.logger.exceptions, "a failed table read was silent" + + +def test_a_genuinely_empty_table_still_gets_its_entries_added(syncer): + """The other side of the same coin: exit 0 with no output is an empty table + and must be repopulated.""" + sync, pushed, popped = syncer(db_ips=["1.1.1.1"], pfctl_result=Completed(0, b"")) + sync.sync_pf_table() + assert pushed == [["1.1.1.1"]] + assert popped == [] + + +def test_pfctl_that_cannot_be_executed_is_survived(syncer, monkeypatch): + """OSError from run(), Eg a pfctl that is not installed.""" + sync, pushed, popped = syncer(db_ips=["1.1.1.1"], pfctl_result=Completed(0)) + monkeypatch.setattr( + fw.subprocess, "run", lambda *a, **k: (_ for _ in ()).throw(FileNotFoundError()) + ) + sync.sync_pf_table() + assert pushed == [] and popped == [] + assert sync.logger.exceptions + + +def test_a_sync_cycle_failure_does_not_end_the_thread(syncer): + """One transient fault must not stop expiry for the table forever.""" + sync, _, _ = syncer(db_ips=["1.1.1.1"], pfctl_result=Completed(1, b"", b"boom")) + sync.stop_event = fw.Event() + sync.scan_redis_db = lambda: None + sync.sync_pf_file = lambda: None + calls = [] + real = sync.sync_pf_table + + def counted(): + calls.append(1) + if len(calls) >= 2: + sync.stop_event.set() + real() + + sync.sync_pf_table = counted + sync.cfg["SCAN_PERIOD"] = 1 + sync.run() + assert len(calls) >= 2, "the loop stopped after the first failing cycle" diff --git a/server-python/tests/test_unix_socket.py b/server-python/tests/test_unix_socket.py new file mode 100644 index 0000000..fb18cde --- /dev/null +++ b/server-python/tests/test_unix_socket.py @@ -0,0 +1,384 @@ +"""The local (AF_UNIX) listener for a resolver on the firewall itself. + +There is no packet on this transport, so the pf.conf source restriction that +guards the network listener does not apply and the socket's own permissions are +the entire access control on who may inject PF whitelist entries. Everything here +is about that: the socket is never wider than SOCKET_UNIX_GROUP, it is never +briefly wider on the way to being bound, and anything that cannot be made that +narrow stops the daemon instead of serving. +""" + +import grp +import os +import shutil +import socket +import stat +import tempfile +import threading +from pathlib import Path + +import pytest + +from pfui_wire import encode +from test_file_store import fw + +MODE = fw.UNIX_SOCKET_MODE + + +def resolvable_own_group(): + """Name of a group this process is really in, so chown() is permitted. + + Every gid the process holds is tried, not just the primary one: on a host + joined to a directory service the primary gid often has no entry in the local + group database, which is a property of the test host and not of PFUI. + """ + for gid in [os.getgid()] + list(os.getgroups()): + try: + return grp.getgrgid(gid).gr_name + except (KeyError, OverflowError): + continue + return None + + +OWN_GROUP = resolvable_own_group() + +pytestmark = pytest.mark.skipif( + OWN_GROUP is None, + reason="no gid held by this process resolves to a local group name", +) + + +def own_group(): + return OWN_GROUP + + +class Recorder: + def __init__(self): + self.lines = [] + + def info(self, msg, *a, **k): + self.lines.append(str(msg)) + + error = exception = info + + +class Daemon: + """Enough of PFUI_Firewall to bind and serve a local socket.""" + + _bind_unix = fw.PFUI_Firewall._bind_unix + _reclaim_unix_socket = fw.PFUI_Firewall._reclaim_unix_socket + _grant_unix_socket = fw.PFUI_Firewall._grant_unix_socket + _remove_unix_socket = fw.PFUI_Firewall._remove_unix_socket + _prepare_conn = fw.PFUI_Firewall._prepare_conn + receiver_thread = fw.PFUI_Firewall.receiver_thread + + def __init__(self, path, group=None): + self.unix = None + self.soc = None + self.db = None + self.stats = False + self.logger = Recorder() + self.cfg = { + "LOGGING": False, + "COMPRESS": False, + "SOCKET_UNIX": str(path), + "SOCKET_UNIX_GROUP": group or own_group(), + "SOCKET_TIMEOUT": 3, + "SOCKET_BACKLOG": 8, + "SOCKET_BUFFER": 1024, + "AF4_TABLE": "pfui_ipv4_domains", + "AF6_TABLE": "pfui_ipv6_domains", + "AF4_FILE": "/nonexistent/af4", + "AF6_FILE": "/nonexistent/af6", + } + + +@pytest.fixture +def short_tmp(): + """A short-pathed scratch directory. + + pytest's tmp_path is far too long for sockaddr_un.sun_path, especially on + macOS where it sits under /private/var/folders/..., so socket tests cannot + use it. See UNIX_PATH_MAX and the config rule that reports the limit. + """ + path = Path(tempfile.mkdtemp(prefix="pfui-", dir="/tmp")) + try: + yield path + finally: + shutil.rmtree(path, ignore_errors=True) + + +@pytest.fixture +def sock_path(short_tmp): + return short_tmp / "pfui_firewall.sock" + + +def mode_of(path): + return stat.S_IMODE(os.stat(path).st_mode) + + +def test_socket_is_bound_with_the_intended_mode_and_group(sock_path): + daemon = Daemon(sock_path) + listener = daemon._bind_unix(str(sock_path)) + try: + assert stat.S_ISSOCK(os.stat(sock_path).st_mode) + assert oct(mode_of(sock_path)) == oct(MODE) + assert grp.getgrgid(os.stat(sock_path).st_gid).gr_name == own_group() + finally: + listener.close() + + +def test_socket_is_not_readable_or_writable_by_others(sock_path): + """The whole point: only SOCKET_UNIX_GROUP may inject whitelist entries.""" + daemon = Daemon(sock_path) + listener = daemon._bind_unix(str(sock_path)) + try: + mode = mode_of(sock_path) + assert not mode & stat.S_IRWXO, f"world bits set: {oct(mode)}" + finally: + listener.close() + + +def test_umask_means_the_socket_is_never_wider_than_intended(sock_path, monkeypatch): + """bind() creates the node with the process umask, so the narrowing cannot be + left to a chmod afterwards: between the two the socket would be connectable by + anyone. Observed by failing the chmod and checking what bind alone produced.""" + daemon = Daemon(sock_path) + seen = {} + real_chmod = os.chmod + + def record_then_chmod(path, mode, *a, **k): + seen.setdefault("before_chmod", mode_of(path)) + return real_chmod(path, mode, *a, **k) + + monkeypatch.setattr(fw.os, "chmod", record_then_chmod) + listener = daemon._bind_unix(str(sock_path)) + try: + assert not seen["before_chmod"] & stat.S_IRWXO + assert not seen["before_chmod"] & stat.S_IRWXG + finally: + listener.close() + + +def test_a_stale_socket_from_an_unclean_stop_is_reclaimed(sock_path): + """bind() fails with EADDRINUSE on a leftover node, so a crash must not need + a hand-cleanup before the service will start again.""" + stale = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + stale.bind(str(sock_path)) + stale.close() # Node remains, nothing listening + assert os.path.exists(sock_path) + + daemon = Daemon(sock_path) + listener = daemon._bind_unix(str(sock_path)) + try: + assert oct(mode_of(sock_path)) == oct(MODE) + finally: + listener.close() + + +def test_a_live_daemons_socket_is_not_stolen(sock_path): + """Unlinking a listening daemon's socket would leave it running and + unreachable, which is worse than refusing to start.""" + incumbent = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + incumbent.bind(str(sock_path)) + incumbent.listen(4) + try: + with pytest.raises(SystemExit): + Daemon(sock_path)._bind_unix(str(sock_path)) + assert stat.S_ISSOCK(os.stat(sock_path).st_mode), "the live socket was removed" + finally: + incumbent.close() + + +def test_a_missing_group_stops_the_daemon(sock_path): + """Rather than serving on a socket whose group is whatever we happened to + inherit, which is how a local socket silently becomes too open.""" + daemon = Daemon(sock_path, group="_no_such_group_exists") + with pytest.raises(SystemExit): + daemon._bind_unix(str(sock_path)) + + +def test_a_missing_group_leaves_no_socket_behind(sock_path): + """A socket left bound by a refused start would be reclaimed by the next one + and, worse, might be connectable in the meantime.""" + daemon = Daemon(sock_path, group="_no_such_group_exists") + with pytest.raises(SystemExit): + daemon._bind_unix(str(sock_path)) + assert not os.path.exists(sock_path) + + +def test_a_world_writable_parent_directory_is_refused(short_tmp): + """Whatever the socket's own mode, anyone could replace it there and be handed + the resolver's messages.""" + loose = short_tmp / "loose" + loose.mkdir() + os.chmod(loose, 0o777) + daemon = Daemon(loose / "pfui.sock") + with pytest.raises(SystemExit): + daemon._bind_unix(str(loose / "pfui.sock")) + + +def test_a_sticky_world_writable_parent_is_allowed(short_tmp): + """/tmp semantics: the sticky bit stops one user unlinking another's node.""" + sticky = short_tmp / "sticky" + sticky.mkdir() + os.chmod(sticky, 0o1777) + daemon = Daemon(sticky / "pfui.sock") + listener = daemon._bind_unix(str(sticky / "pfui.sock")) + listener.close() + + +def test_a_missing_parent_directory_stops_the_daemon(short_tmp): + daemon = Daemon(short_tmp / "absent" / "pfui.sock") + with pytest.raises(SystemExit): + daemon._bind_unix(str(short_tmp / "absent" / "pfui.sock")) + + +def test_shutdown_removes_the_socket(sock_path): + daemon = Daemon(sock_path) + daemon.unix = daemon._bind_unix(str(sock_path)) + daemon._remove_unix_socket() + assert not os.path.exists(sock_path) + + +def test_removing_the_socket_is_safe_when_none_was_bound(sock_path): + """Called on the failure path when SOCKET_UNIX is set but nothing bound.""" + daemon = Daemon(sock_path) + daemon._remove_unix_socket() # Must not raise + assert not os.path.exists(sock_path) + + +def test_prepare_conn_sets_a_timeout_without_touching_nagle(sock_path): + """TCP_NODELAY is not a thing on a local socket, and setting it there raises + rather than being ignored, so a spurious exception would be logged for every + single connection.""" + left, right = socket.socketpair(socket.AF_UNIX, socket.SOCK_STREAM) + try: + daemon = Daemon(sock_path) + daemon._prepare_conn(left) + assert left.gettimeout() == 3.0 + assert daemon.logger.lines == [], f"logged: {daemon.logger.lines}" + finally: + left.close() + right.close() + + +def test_a_message_over_the_local_socket_is_acknowledged(sock_path, monkeypatch): + """End to end on the stream path: the same framing and the same reply as TCP, + with the socket path standing in for the peer address accept() cannot give.""" + performed = [] + monkeypatch.setattr(fw, "table_push", lambda **kw: performed.append(kw["ip_list"])) + monkeypatch.setattr(fw, "db_push", lambda **kw: None) + monkeypatch.setattr(fw, "file_push", lambda **kw: None) + + daemon = Daemon(sock_path) + listener = daemon._bind_unix(str(sock_path)) + message = { + "kind": "rr", + "qname": "local.example.com.", + "AF4": [{"ip": "8.8.8.8", "ttl": 60}], + "AF6": [], + } + replies = [] + + def client(): + conn = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + conn.settimeout(5) + try: + conn.connect(str(sock_path)) + conn.sendall(encode(message, compress=False)) + replies.append(conn.recv(64)) + finally: + conn.close() + + thread = threading.Thread(target=client) + thread.start() + try: + served, _ = listener.accept() + daemon._prepare_conn(served) + daemon.receiver_thread(proto="UNIX", conn=served, peer=str(sock_path)) + thread.join(10) + finally: + listener.close() + + assert replies == [b"ACKUPDATE"] + assert performed == [["8.8.8.8"]] + + +def test_a_peer_that_leaves_before_the_ack_is_tolerated(sock_path, monkeypatch): + """A cache report is sent with blocking=False, so the resolver has closed by + the time the acknowledgement is written. Loopback TCP absorbs that write into + a buffer nobody reads; a local socket reports EPIPE immediately, so the + tolerance in disconnect() is load-bearing on this transport and not on TCP. + The whitelisting must still have happened. + """ + performed = [] + monkeypatch.setattr(fw, "table_push", lambda **kw: performed.append(kw["ip_list"])) + monkeypatch.setattr(fw, "db_push", lambda **kw: None) + monkeypatch.setattr(fw, "file_push", lambda **kw: None) + + daemon = Daemon(sock_path) + listener = daemon._bind_unix(str(sock_path)) + message = { + "kind": "cache", + "qname": "cached.example.com.", + "AF4": [{"ip": "8.8.8.8", "ttl": 2000000000}], + "AF6": [], + } + + def client(): + conn = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + conn.settimeout(5) + conn.connect(str(sock_path)) + conn.sendall(encode(message, compress=False)) + conn.close() # Away before the ACK, as the non-blocking path does + + thread = threading.Thread(target=client) + thread.start() + try: + served, _ = listener.accept() + daemon._prepare_conn(served) + thread.join(10) + # Must not raise out of the worker, and must still install the address + daemon.receiver_thread(proto="UNIX", conn=served, peer=str(sock_path)) + finally: + listener.close() + + assert performed == [["8.8.8.8"]] + + +def test_a_refusal_over_the_local_socket_names_the_socket(sock_path, monkeypatch): + """A local sender has no address to be logged, so the diagnostics have to fall + back to the socket path rather than printing None:None.""" + monkeypatch.setattr(fw, "table_push", lambda **kw: None) + monkeypatch.setattr(fw, "db_push", lambda **kw: None) + monkeypatch.setattr(fw, "file_push", lambda **kw: None) + + daemon = Daemon(sock_path) + listener = daemon._bind_unix(str(sock_path)) + replies = [] + + def client(): + conn = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + conn.settimeout(5) + try: + conn.connect(str(sock_path)) + # No 'kind': a version-skewed sender + conn.sendall(encode({"AF4": [{"ip": "8.8.8.8", "ttl": 60}]}, compress=False)) + replies.append(conn.recv(64)) + finally: + conn.close() + + thread = threading.Thread(target=client) + thread.start() + try: + served, _ = listener.accept() + daemon._prepare_conn(served) + daemon.receiver_thread(proto="UNIX", conn=served, peer=str(sock_path)) + thread.join(10) + finally: + listener.close() + + assert replies == [b"Missing kind"] + assert any(str(sock_path) in line for line in daemon.logger.lines) + assert not any("None:None" in line for line in daemon.logger.lines)