diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index efe2f0e..e8985bd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -27,17 +27,8 @@ jobs: - name: Check both daemons parse run: python -m py_compile server-python/pfui_firewall.py client-unbound/pfui_unbound.py - server-c: - # Builds the C framing implementation and runs the same conformance vectors - # as the Python one, so the two cannot drift apart unnoticed. - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - name: Build and run protocol vectors - run: make -C server-c test - server-rust: - # Runs on the exact rustc OpenBSD-stable ports ships (rust-version in + # Runs on the exact rustc OpenBSD-stable packages (rust-version in # Cargo.toml), so post-MSRV code fails here rather than on the firewall. # Both vector suites run inside cargo test. runs-on: ubuntu-latest @@ -46,7 +37,7 @@ jobs: working-directory: server-rust steps: - uses: actions/checkout@v4 - - name: Pin the OpenBSD ports toolchain + - name: Pin the OpenBSD packaged toolchain run: | rustup toolchain install 1.94 --profile minimal -c clippy -c rustfmt rustup default 1.94 @@ -88,4 +79,5 @@ jobs: - name: Syntax-check the rc.d scripts run: | sudo apt-get update -qq && sudo apt-get install -y -qq ksh - for f in client-unbound/rc.d/* server-python/rc.d/* server-rust/rc.d/*; do ksh -n "$f"; done + for f in client-unbound/rc.d/* server-python/rc.d/* server-rust/rc.d/* \ + tests/openbsd/*.sh; do ksh -n "$f"; done diff --git a/.github/workflows/openbsd.yml b/.github/workflows/openbsd.yml new file mode 100644 index 0000000..98d1a3c --- /dev/null +++ b/.github/workflows/openbsd.yml @@ -0,0 +1,66 @@ +name: openbsd + +# Runs tests/openbsd/run.sh in a real OpenBSD VM: both installers, then the +# installed resolver against the installed daemon under unveil and pledge. +# The daemon's ioctl, unveil and pledge code compiles on no other platform. + +on: + pull_request: + paths: + - 'server-rust/**' + - 'client-unbound/**' + - 'protocol/**' + - 'install-*.sh' + - 'tests/openbsd/**' + - '.github/workflows/openbsd.yml' + push: + paths: + - 'server-rust/**' + - 'client-unbound/**' + - 'protocol/**' + - 'install-*.sh' + - 'tests/openbsd/**' + - '.github/workflows/openbsd.yml' + workflow_dispatch: + inputs: + src_tree: + description: >- + OpenBSD sources for the Unbound build: 2 = the signed release set + (what the installer does by default), 3 = -current from the git + mirror (unsigned; upstream churn, so never on a pull request) + type: choice + options: ['2', '3'] + default: '2' + unbound_version: + description: 'Unbound to build: latest, master, or a release-* tag' + default: 'latest' + +jobs: + end-to-end: + name: OpenBSD ${{ matrix.release }} + runs-on: ubuntu-latest + timeout-minutes: 150 + strategy: + fail-fast: false + matrix: + # The release the production firewall runs + release: ['7.9'] + env: + PFUI_SRC_TREE: ${{ github.event.inputs.src_tree || '2' }} + UNBOUND_VERSION: ${{ github.event.inputs.unbound_version || 'latest' }} + steps: + - uses: actions/checkout@v4 + - name: Install PFUI, then run the resolver against the daemon + uses: vmactions/openbsd-vm@v1 + with: + release: ${{ matrix.release }} + envs: 'PFUI_SRC_TREE UNBOUND_VERSION' + sync: rsync + copyback: false + # Cached as a VM image; the package set is fixed per release. bash + # is not in base. + cache-after-prepare: true + prepare: | + pkg_add -I bash rust redis swig git curl py3-lz4 py3-yaml + run: | + sh tests/openbsd/run.sh diff --git a/DECISIONS.md b/DECISIONS.md index e138387..5c36255 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -165,7 +165,7 @@ table. It is not on the per-query hot path, so the subprocess cost is accepted. Replacing it needs a `DIOCRGETADDRS` implementation, which belongs with the other ioctl structs; deferred, not forgotten. The Rust daemon closes this deferral: under `CTL: IOCTL` it reads the table with `DIOCRGETADDRS`, and the -subprocess survives only as the `CTL: PFCTL` path and the ioctl-error fallback. +subprocess survives only as the `CTL: PFCTL` path. ## No live configuration reload @@ -249,9 +249,13 @@ binary path. C's only real edge was zero-package builds from base clang; Rust buys memory safety on the one component that parses unauthenticated network input and writes PF tables, serde for the config and payloads, a test framework that carried the whole Python suite across, and cheap -`unveil(2)`/`pledge(2)`. `server-c/` stays as the framing cross-check; its -retirement is a separate decision, taken only after the Rust daemon passes -the same vectors in production use. +`unveil(2)`/`pledge(2)`. + +There is no C server. `server-c/` held a framing implementation and nothing +else, kept as a cross-check against the shared vectors until the Rust daemon +had proven itself in production. It has, so the scaffolding is removed rather +than maintained: `protocol/python` and `server-rust` both run +`vectors/framing.tsv`, which is the cross-check the C code existed for. The daemon is plain threads, no async runtime: the Python design (an accept thread per listener plus a bounded worker pool shedding beyond twice @@ -314,3 +318,105 @@ Whether the `pf` promise covers the table-address ioctls decides the final shape: covered means a pledged daemon, not covered means unveil-only with a privsep follow-up (a pledged network parent feeding a small PF-writer child over a socketpair). + +The unpledged baseline was reached in production in September 2026, and +pledge is now on; the arrangement is under "The sandbox follows CTL" below. +The pass that proves it is no longer manual: the OpenBSD job runs the pledged +daemon under every configuration on every relevant change. + +## CTL: IOCTL is the ioctl and nothing else + +The Python daemon falls back to pfctl on any ioctl error, and the Rust daemon +carried that over for parity. The Rust daemon drops it. A fallback that +succeeds leaves nothing to act on: every message paid a fork and exec of +pfctl, milliseconds against the tens of microseconds an ioctl costs, and the +only trace was an error line per message ending "pfctl served the call", which +reads as reassurance. A firewall whose ioctl path broke would have run +indefinitely at a fraction of its speed with nothing wrong in any dashboard. It +also papered over the one realistic ioctl failure, `ESRCH` for a table absent +from the ruleset: `pfctl -T add` creates a table no rule references, so +addresses were being whitelisted into nothing. + +The ioctl either works or it does not, and the operator hears about it once, +loudly, with the remedy attached. A startup probe reads both tables through the +configured control path before anything is served and refuses to start +otherwise (exit code 7), naming what the errno means on a firewall: `EACCES` +is `/dev/pf` not being group `_pfui_firewall` mode 660, `ESRCH` is the table +missing from the loaded ruleset, `ENOENT` is no `/dev/pf` at all. Every such +message ends with the two remedies, fix that or set `CTL: PFCTL`. The probe +runs as the daemon user after rc.d has dropped privileges, which is the uid +whose permissions matter, and after the sandbox is applied, so it is also the +first exercise of the boundary. Under `CTL: PFCTL` the probe is a `pfctl -T +show` per table, which proves the exec path the same way. + +A runtime ioctl failure is logged with the same guidance and nothing else +happens: the acknowledgement still goes out, as it always has and as the +Python daemon does, and PF keeps denying the traffic, so the client side stays +fail-closed. The Python daemon is unchanged. + +## The sandbox follows CTL, and the table ioctls run in a child + +`unveil(2)` and `pledge(2)` are applied together, after the binds and before +the PF probe. Both are built as data (`platform::unveil_plan`, +`platform::pledge_promises`) and applied separately, so their contents are +tested on every platform even though the calls exist only on OpenBSD. The list +is the security boundary, and it was incomplete with nowhere to notice until +the OpenBSD job ran: `/sbin/pfctl` was unveiled but `/dev/null` was not, and +the spawn opens `/dev/null` for the child's stdin before it execs, so the exec +failed with `ENOENT`. `platform.rs` compiled only on the target. + +The `pf` promise does not cover what PFUI does. `kern_pledge.c` permits +`DIOCADDRULE`, `DIOCGETSTATUS`, `DIOCNATLOOK`, `DIOCRADDTABLES`, +`DIOCRCLRADDRS`, `DIOCRCLRTABLES`, `DIOCRCLRTSTATS`, `DIOCRGETTSTATS`, +`DIOCRSETADDRS`, `DIOCXBEGIN`, `DIOCXCOMMIT` and `DIOCKILLSRCNODES` under it, +and no other ioctl on `/dev/pf` under any promise. `DIOCRADDADDRS`, +`DIOCRDELADDRS` and `DIOCRGETADDRS` are not on the list, so a pledged process +cannot add, remove or read table addresses at all; a daemon that tried was +killed at its first probe. The kill is also silent for a daemon: `pledge_fail` +reports with `uprintf`, which writes to the controlling terminal, so nothing +reaches dmesg or syslog. That is why the probe runs after the pledge and why +the OpenBSD job exists. + +So under `CTL: IOCTL` the ioctls run in a PF child (`pf::privsep`). The parent +forks it before any thread exists and before it pledges; the child opens +`/dev/pf`, unveils the filesystem away, and does nothing but read a fixed-shape +request over a socketpair, run one ioctl, and write the reply. It cannot +pledge, since every promise set forbids its ioctls, but it parses nothing that +did not come from the parent. The parent never holds `/dev/pf`: its plan is +the config, the persist and socket directories, and its pledge is `stdio +rpath wpath cpath flock fattr inet` plus `unix` when a local socket is bound. +It neither forks nor execs after the child exists. `CTL: PFCTL` runs no child: +it adds `/dev/pf`, `/sbin/pfctl` and `/dev/null` to the plan and `proc exec` +to the pledge, because the pfctl child inherits the view, opens the device +itself, and is given `/dev/null` as stdin. `/sbin/pfctl` is statically linked, +so no loader or library is unveiled. `dns` is never pledged: `REDIS_HOST` is resolved +before lockdown and the client holds the address. `execpromises` is NULL. + +`PLEDGE: False` disables the parent's call. It is an emergency switch, not a +mode: a violation on a promise this design missed kills the control plane, and +an operator must be able to restore whitelisting without a rebuild while the +gap is fixed. A daemon started that way logs at error level on every start that +it is running unpledged, so a firewall left in that state does not look normal. + +## OpenBSD runs in CI + +`.github/workflows/openbsd.yml` runs `tests/openbsd/run.sh` inside a real +OpenBSD VM (`vmactions/openbsd-vm`) on every change to the daemon, the +resolver, the protocol, the installers or the job itself, and on demand. It is the only place the OpenBSD-only code compiles, so it is the only +thing that can prove the ioctl, unveil and pledge paths; the unveil gap above +survived precisely because nothing compiled `platform.rs`. + +One VM, one build, many passes: building rust and unbound once and cycling +configurations keeps the job near half an hour instead of multiplying it by +the matrix. Both installers run for real, unattended, with the signed release +sources, which is the path an operator follows; `-current` is reachable only +from `workflow_dispatch`, because upstream churn must not block a pull +request. The pf.conf it +loads declares only the two tables and passes everything, since the shipped +default-deny example would sever the runner's own session. Lookups are made +with `dig @127.0.0.1` rather than ping: resolvd rewrites `resolv.conf` and +ICMP through qemu's NAT is unreliable, so ping is a poor trigger even though +it would work as one. Latency figures are printed, and the acknowledgement is +asserted to precede the total, but no absolute threshold is asserted: the VM +is qemu on a shared runner, and any threshold would flake. Production numbers +come from the production firewall. diff --git a/README.md b/README.md index 86d9d7f..3d756cb 100644 --- a/README.md +++ b/README.md @@ -24,11 +24,10 @@ Ie, Users cannot bypass an administrator's DNS blocking attempts using 'DNS over | [protocol/](protocol/) | The wire protocol: specification, conformance vectors, and the Python reference implementation shared by clients and servers | | [client-unbound/](client-unbound/) | PFUI client as an Unbound pythonmod plugin | | [server-python/](server-python/) | PFUI server for OpenBSD PF, in Python | -| [server-rust/](server-rust/) | PFUI server in Rust: a drop-in replacement for the Python daemon, validated on OpenBSD 7.9 against a live resolver and PF | -| [server-c/](server-c/) | PFUI server in C. Framing only so far | +| [server-rust/](server-rust/) | PFUI server for OpenBSD PF, in Rust (the recommended production version) | | `install-client-unbound.sh` | Installs the Unbound client on a resolver | | `install-server-python.sh` | Installs the Python server on a PF firewall | -| `install-server-rust.sh` | Installs the Rust server on a PF firewall (builds with the ports rustc) | +| `install-server-rust.sh` | Installs the Rust server on a PF firewall (builds with the packaged rustc) | | [examples/pf.conf](examples/pf.conf) | Example PF ruleset, applies to any server implementation | Clients and servers share only the protocol. Adding support for another resolver @@ -296,7 +295,7 @@ https://www.geoghegan.ca/pub/pf-badhost/latest/man/man.txt ``` pytest # protocol, client and server suites -make -C server-c test # the C framing against the shared vectors +cargo test --manifest-path server-rust/Cargo.toml ./client-unbound/tests/container/run.sh # builds Unbound and runs PFUI_Unbound in it ``` @@ -304,6 +303,17 @@ 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. +``` +doas sh tests/openbsd/run.sh # on a scratch OpenBSD host: everything, end to end +``` + +That script is what CI runs inside a real OpenBSD VM +(`.github/workflows/openbsd.yml`): both installers unattended, the daemon's +own tests on the target, then live lookups from the resolver it installed to +the daemon it installed over every transport and control path, checked in the +PF table, Redis and the persist file. It rewrites `/etc/pf.conf` and both PFUI +configurations, so never run it on a firewall that matters. + 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 @@ -368,6 +378,7 @@ SCAN_PERIOD: 60 TTL_MULTIPLIER: 4 CTL: IOCTL DEVPF: /dev/pf +PLEDGE: True # Rust daemon only AF4_TABLE: pfui_ipv4_domains AF4_FILE: /var/db/pfui/ipv4_domains AF6_TABLE: pfui_ipv6_domains @@ -415,6 +426,7 @@ SCAN_PERIOD: 60 TTL_MULTIPLIER: 4 CTL: IOCTL DEVPF: /dev/pf +PLEDGE: True # Rust daemon only AF4_TABLE: pfui_ipv4_domains AF4_FILE: /var/db/pfui/ipv4_domains AF6_TABLE: pfui_ipv6_domains @@ -515,9 +527,19 @@ Supports IPv4 and IPv6. PFUI_Unbound - Supports anything Unbound does (Linux, BSD, etc), requires Python 3. -PFUI_Firewall - Supports OpenBSD (FreeBSD still in alpha). Python is optional: -the default Rust daemon needs none on the firewall, and the Python daemon -requires Python 3. +PFUI_Firewall - OpenBSD only. It drives PF through OpenBSD's table ioctls, and +the Rust daemon also uses `unveil(2)` and `pledge(2)`. FreeBSD has its own PF +interface and could plausibly be supported, but that work has not been done. +Python is optional: the Rust daemon needs none on the firewall, and the Python +daemon requires Python 3. + +The Rust daemon sandboxes itself once it is serving: `unveil(2)` reduces its +filesystem to the config, the persist directory and the socket directory, and +`pledge(2)` reduces its syscalls to what the listeners and the stores need. The +table ioctls are not permitted under any pledge promise, so they run in a small +child process that holds `/dev/pf` and does nothing else. `PLEDGE: False` turns +the pledge off, for an operator who needs whitelisting back before a promise gap +is fixed; a daemon started that way says so at error level on every start. ------ diff --git a/client-unbound/pfui_unbound.py b/client-unbound/pfui_unbound.py index b411715..b94c488 100644 --- a/client-unbound/pfui_unbound.py +++ b/client-unbound/pfui_unbound.py @@ -49,25 +49,20 @@ from socket import inet_ntop, ntohs, socket from socket import timeout as TIMEOUT -# Severities, ordered. log_err() is never gated on these: an error is reported -# whatever LOG_LEVEL says, because the setting selects how much detail -# accompanies a fault rather than whether faults are reported at all. +# Severities, ordered. log_err() is never gated: LOG_LEVEL selects detail, not +# whether faults are reported LOG_LEVELS = {"DEBUG": 10, "INFO": 20, "ERROR": 40} def log_at(level): """True when a message of this severity should be emitted. - LOG_LEVEL is a threshold, not an equality test. Testing it for equality with - DEBUG left every other informational line gated on LOGGING alone, so the - default LOG_LEVEL: ERROR still emitted per-query INFO output. An unrecognised - level reads as ERROR rather than opening the logs up. + LOG_LEVEL is a threshold; an unrecognised level reads as ERROR. """ cfg = globals().get("pfui_cfg") or {} if not cfg.get("LOGGING", False): return False - # Normalised the same way load_config does, so a value that reached pfui_cfg - # without passing through it is read identically + # Same normalisation as load_config threshold = LOG_LEVELS.get( str(cfg.get("LOG_LEVEL", "ERROR")).strip().upper(), LOG_LEVELS["ERROR"] ) @@ -310,11 +305,7 @@ def udp_transmit_close(data, ip, port, blocking): 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". + Only an elapsed cool-off clears the failure count. """ state = _breakers.get(target) if not state: @@ -331,9 +322,7 @@ def breaker_record(target, ok): """Count consecutive failures and open the breaker once the threshold trips.""" state = _breakers.setdefault(target, [0, 0]) if ok: - # Only the transition is reported, not every success: opening the breaker - # was logged while closing it was silent, so a firewall was seen going - # away and never coming back. state[1] set means it had actually opened. + # Only the transition is logged; state[1] set means the breaker had opened if state[1]: if log_at("INFO"): log_info( @@ -394,17 +383,10 @@ def stream_transmit_close(data, family, address, target, blocking): 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. + # The breaker counts acknowledgements, not sends; a refusal is a failure too try: if blocking and sent: # Nothing to acknowledge if the send failed - # Read until the firewall closes rather than taking one segment: a - # reply split in transit compared unequal to ACKUPDATE and was - # charged to the breaker as a refusal + # Read to EOF: a reply may arrive split reply = b"" while len(reply) < 36: chunk = conn.recv(36 - len(reply)) @@ -420,8 +402,6 @@ def stream_transmit_close(data, family, address, target, blocking): f"{refusal_detail(reply)}" ) # A non-blocking send records nothing: the firewall has not answered - # yet, and counting it a success cleared the failures the blocking path - # was accumulating, so the breaker could never open except TIMEOUT: breaker_record(target, ok=False) log_err( @@ -476,11 +456,10 @@ def unix_transmit_close(data, path, blocking): def refusal_detail(reply): - """A refusal with what it usually means, for the log. + """A refusal with what it usually means. - A decode failure is reported with what this resolver sent, which fixes the - direction of a COMPRESS mismatch from this end alone: the firewall is set - the other way. + A decode failure names what this end sent, so the COMPRESS mismatch + direction is known here. """ hint = REFUSAL_HINTS.get(reply) if reply == b"Failed to decode": @@ -591,11 +570,8 @@ def inplace_cache_callback( and the callback's return value is read as a boolean, so returning nothing leaves an error pending for the next query to fail on. - Sent non-blocking whatever BLOCKING is set to. A cache hit means the name was - resolved earlier, so an rr report already installed these addresses for - TTL_MULTIPLIER times the DNS TTL - longer than Unbound keeps the answer - and - the client can already reach them. This report extends an expiry that has not - arrived, so there is nothing to hold the answer for. + Sent non-blocking whatever BLOCKING is: the rr report that released this + answer already installed its addresses, so this only resets the TTL. """ try: if log_at("DEBUG"): @@ -673,9 +649,8 @@ def inform_super(id, qstate, superqstate, qdata): return True -# pythonmod injects these into this module's globals. strmodulevent() is not -# used to name an event: its binding rejects anything outside 'enum module_ev' -# with an OverflowError, and a log line must not be able to fail. +# pythonmod injects these. strmodulevent() is not used: it raises OverflowError +# outside 'enum module_ev', and a log line must not fail EVENT_NAMES = ( "MODULE_EVENT_NEW", "MODULE_EVENT_PASS", @@ -786,10 +761,7 @@ 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. +# Every key this module reads, with the value assumed when the yml omits it CONFIG_DEFAULTS = { "LOGGING": True, "LOG_LEVEL": "ERROR", @@ -838,9 +810,7 @@ def load_config(location=CONFIG_LOCATION): except (TypeError, ValueError): raise ValueError(f"{key} must be a number, not {cfg[key]!r}") from None - # Normalised rather than rejected: an unrecognised level is not worth - # refusing to resolve over, and log_at() reads it as ERROR either way. Said - # out loud because the symptom of a typo is silently quieter logs. + # Normalised, not rejected; an unrecognised level is reported and read as ERROR cfg["LOG_LEVEL"] = str(cfg["LOG_LEVEL"]).strip().upper() if cfg["LOG_LEVEL"] not in LOG_LEVELS: log_err( @@ -849,9 +819,7 @@ def load_config(location=CONFIG_LOCATION): ) cfg["LOG_LEVEL"] = "ERROR" - # Checked here rather than left to the first query: encode_payload would - # raise per answer, and a resolver that answers while telling no firewall - # anything leaves every client denied by PF with nothing in the log to say so + # At load, not per answer: a resolver that cannot encode must fail to start if cfg["COMPRESS"] and not HAVE_LZ4: raise ValueError( "COMPRESS is True but the lz4 package is not installed; install " diff --git a/install-client-unbound.sh b/install-client-unbound.sh index 0fe0837..d05a950 100755 --- a/install-client-unbound.sh +++ b/install-client-unbound.sh @@ -15,9 +15,14 @@ OPENBSD_SRC_REPO="https://github.com/openbsd/src.git" # pinned fallback live in client-unbound/tools/unbound_release.sh, which the # container that tests this build uses too. UNBOUND_VERSION="${UNBOUND_VERSION:-latest}" +# Unattended answers to the two prompts below; unset, the installer asks. +# PFUI_UNBOUND_BUILD: 1 keep Unbound, update the module; 2 build Unbound. +# PFUI_SRC_TREE: 1 keep /usr/src; 2 signed release sources; 3 -current (unsigned). +PFUI_UNBOUND_BUILD="${PFUI_UNBOUND_BUILD:-}" +PFUI_SRC_TREE="${PFUI_SRC_TREE:-}" err=0 -trap 'err=1' ERR +trap 'err=1; echo "$(basename "$0"): failed at line $LINENO: $BASH_COMMAND" >&2' ERR # Abort immediately on a step nothing can proceed without die() { @@ -181,6 +186,13 @@ if [[ "$OS" = "OpenBSD" ]]; then if [ ! -d "${TARGET}" ]; then mkdir -p "${TARGET}" fi + # 'make install-all' writes Unbound's stock example at the conf path when + # none exists, which is not a config worth keeping + if [ -f "${TARGET}/pfui_unbound.conf" ]; then + CONF_PREDATES_BUILD=yes + else + CONF_PREDATES_BUILD=no + fi # Report what is installed, so the choice below is an informed one. A module # upgrade is the common case and needs no rebuild. @@ -202,7 +214,15 @@ if [[ "$OS" = "OpenBSD" ]]; then echo "Unbound resolver (installed: ${HAVE_UNBOUND})" echo " 1) keep it, and update only the PFUI module and configuration" echo " 2) build and install Unbound (${UNBOUND_VERSION}) with the Python module" - read -p "Choose 1 or 2 [${BUILD_DEFAULT}]: " build_choice + if [ -n "${PFUI_UNBOUND_BUILD}" ]; then + case "${PFUI_UNBOUND_BUILD}" in + 1|2) build_choice="${PFUI_UNBOUND_BUILD}" ;; + *) die "PFUI_UNBOUND_BUILD must be 1 or 2, not '${PFUI_UNBOUND_BUILD}'" ;; + esac + echo "Choose 1 or 2 [${BUILD_DEFAULT}]: ${build_choice} (from PFUI_UNBOUND_BUILD)" + else + read -p "Choose 1 or 2 [${BUILD_DEFAULT}]: " build_choice + fi [ -n "${build_choice}" ] || build_choice="${BUILD_DEFAULT}" if [[ "${build_choice}" = "2" ]]; then # Unbound is built with Makefile.bsd-wrapper taken from the system sources, @@ -220,7 +240,15 @@ if [[ "$OS" = "OpenBSD" ]]; then echo " 2) replace it with the signed ${REL} release sources" echo " 3) replace it with -current from the git mirror (unsigned)" echo "Options 2 and 3 delete /usr/src/* first. /usr/ports is never touched." - read -p "Choose 1, 2 or 3 [1]: " src_choice + if [ -n "${PFUI_SRC_TREE}" ]; then + case "${PFUI_SRC_TREE}" in + 1|2|3) src_choice="${PFUI_SRC_TREE}" ;; + *) die "PFUI_SRC_TREE must be 1, 2 or 3, not '${PFUI_SRC_TREE}'" ;; + esac + echo "Choose 1, 2 or 3 [1]: ${src_choice} (from PFUI_SRC_TREE)" + else + read -p "Choose 1, 2 or 3 [1]: " src_choice + fi case "${src_choice}" in 2) fetch_release_src "${REL}" ;; 3) fetch_current_src ;; @@ -360,7 +388,7 @@ if [[ "$OS" = "OpenBSD" ]]; then # The resolver's own config carries an operator's whole ruleset, so an # existing one is never replaced; only a first install lays down the example echo - if [ -f "${TARGET}/pfui_unbound.conf" ]; then + if [ -f "${TARGET}/pfui_unbound.conf" ] && [ "${CONF_PREDATES_BUILD}" = yes ]; then cp -p "${TARGET}/pfui_unbound.conf" "${TARGET}/pfui_unbound.conf.${HOUR}" echo "PFUIDNS: Keeping the existing ${TARGET}/pfui_unbound.conf" echo "PFUIDNS: (backup at ${TARGET}/pfui_unbound.conf.${HOUR}; the example is" @@ -378,14 +406,15 @@ fi echo echo "PFUIDNS: Updating DNS root keys and certs" cd /var/unbound/etc/ || exit -unbound-anchor -a "/var/unbound/db/root.key" +# Exit 1 means the key was written from the built-in anchor; only 2 and above fail +unbound-anchor -a "/var/unbound/db/root.key" || [ $? -eq 1 ] || die "unbound-anchor failed" unbound-control-setup echo "PFUIDNS: Updating DNS root hints" ${TARGET}/update_root_hints.sh norestart echo echo "Checking Unbound configuration" -/usr/local/sbin/unbound-anchor -v +/usr/local/sbin/unbound-anchor -v || [ $? -eq 1 ] || die "unbound-anchor failed" /usr/local/sbin/unbound-checkconf ${TARGET}/pfui_unbound.conf # Not written automatically: the previous version expanded ${PATH} at install diff --git a/install-server-python.sh b/install-server-python.sh index 2824adc..cbeace0 100755 --- a/install-server-python.sh +++ b/install-server-python.sh @@ -4,7 +4,7 @@ # err=0 -trap 'err=1' ERR +trap 'err=1; echo "$(basename "$0"): failed at line $LINENO: $BASH_COMMAND" >&2' ERR # Abort immediately on a step nothing can proceed without die() { diff --git a/install-server-rust.sh b/install-server-rust.sh index 7b41d61..fb2a97e 100755 --- a/install-server-rust.sh +++ b/install-server-rust.sh @@ -7,7 +7,7 @@ # mutually exclusive on one firewall. err=0 -trap 'err=1' ERR +trap 'err=1; echo "$(basename "$0"): failed at line $LINENO: $BASH_COMMAND" >&2' ERR # Abort immediately on a step nothing can proceed without die() { @@ -47,7 +47,7 @@ if [[ $(uname) != "OpenBSD" ]]; then exit 2 fi -# Build here with the ports rustc (which is the MSRV pin), or install a +# Build here with the packaged rustc (which is the MSRV pin), or install a # prebuilt artifact from the release builder: PFUI_BINARY=/path skips the # toolchain and the build entirely. if [[ -n "${PFUI_BINARY}" ]]; then diff --git a/protocol/PROTOCOL.md b/protocol/PROTOCOL.md index df0a6fa..08c87b4 100644 --- a/protocol/PROTOCOL.md +++ b/protocol/PROTOCOL.md @@ -2,7 +2,7 @@ Normative description of what a PFUI client sends to a PFUI server. Any client (`client-unbound/`, or a future client for another resolver) and any server -(`server-python/`, `server-c/`) must agree with this document, and both are +(`server-python/`, `server-rust/`) must agree with this document, and both are tested against the shared vectors in `vectors/`. Version: 2 (`qname` moved from each record to the message). There is no version diff --git a/protocol/README.md b/protocol/README.md index dcc87f4..1d45c92 100644 --- a/protocol/README.md +++ b/protocol/README.md @@ -9,14 +9,13 @@ What every PFUI client and server must agree on. `client-unbound/` and `server-python/` Only code that both a client and a server need lives here. Server-only logic -(expiry, address validation, PF ioctl) belongs to the server, and the C framing -implementation lives with `server-c/` because nothing else uses it. +(expiry, address validation, PF ioctl) belongs to the server. Both implementations run `vectors/framing.tsv`: ``` python -m pytest protocol/python/tests -make -C server-c test +cargo test --manifest-path server-rust/Cargo.toml ``` A change to the framing or the message object means changing PROTOCOL.md, the diff --git a/protocol/python/pfui_wire.py b/protocol/python/pfui_wire.py index a78c96c..3218e72 100644 --- a/protocol/python/pfui_wire.py +++ b/protocol/python/pfui_wire.py @@ -15,10 +15,7 @@ try: import lz4.frame except ImportError: - # Only a compressed payload needs it, and COMPRESS: False is how an operator - # says there will not be one. Importing it unconditionally made the package - # mandatory even then, so a resolver with compression off still could not - # load this module. + # Needed only when COMPRESS is on lz4 = None HAVE_LZ4 = lz4 is not None @@ -45,11 +42,7 @@ class Truncated(WireError): def _codec(): - """lz4.frame, or a WireError naming what to install. - - Raised rather than returned so a caller that asked for compression cannot - silently send plain JSON, which the far end would refuse as a mismatch. - """ + """lz4.frame, or a WireError naming what to install; never plain JSON.""" if not HAVE_LZ4: raise WireError( "COMPRESS is on but the lz4 package is not installed; install " diff --git a/protocol/python/tests/test_vectors.py b/protocol/python/tests/test_vectors.py index 16b5ba7..854742f 100644 --- a/protocol/python/tests/test_vectors.py +++ b/protocol/python/tests/test_vectors.py @@ -1,6 +1,6 @@ """Conformance tests against the shared vectors in ../../vectors/. -The same framing.tsv is read by server-c's test suite, so a divergence between +The same framing.tsv is read by server-rust's test suite, so a divergence between the two implementations fails here or there rather than in production. """ @@ -11,7 +11,7 @@ from pfui_wire import BadLength, MAX_MESSAGE, Truncated, WireError, decode_stream, encode -# The vectors' `expect` column names the rejection, and server-c reports the +# The vectors' `expect` column names the rejection, and server-rust 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. diff --git a/server-c/Makefile b/server-c/Makefile deleted file mode 100644 index ed5e2d6..0000000 --- a/server-c/Makefile +++ /dev/null @@ -1,27 +0,0 @@ -# PFUI_Firewall, C implementation. Framing layer only so far; see README.md. - -CC ?= cc -CFLAGS ?= -std=c99 -O2 -Wall -Wextra -Werror -pedantic -CPPFLAGS += -Iinclude -BUILD := build -VECTORS := ../protocol/vectors/framing.tsv - -.PHONY: all test clean - -all: $(BUILD)/test_vectors - -$(BUILD): - mkdir -p $(BUILD) - -$(BUILD)/pfui_wire.o: src/pfui_wire.c include/pfui_wire.h | $(BUILD) - $(CC) $(CFLAGS) $(CPPFLAGS) -c src/pfui_wire.c -o $@ - -$(BUILD)/test_vectors: tests/test_vectors.c $(BUILD)/pfui_wire.o - $(CC) $(CFLAGS) $(CPPFLAGS) tests/test_vectors.c $(BUILD)/pfui_wire.o -o $@ - -# Runs the same vectors as protocol/python/tests/test_vectors.py -test: $(BUILD)/test_vectors - $(BUILD)/test_vectors $(VECTORS) - -clean: - rm -rf $(BUILD) diff --git a/server-c/README.md b/server-c/README.md deleted file mode 100644 index 9a610ab..0000000 --- a/server-c/README.md +++ /dev/null @@ -1,39 +0,0 @@ -# PFUI_Firewall (C) - -A C implementation of the PFUI server, intended to replace the Python daemon on -the firewall itself. The Python version wraps the PF ioctl structures through -ctypes, so the kernel interface is the part that most wants to be C. - -## Status - -Framing only. `src/pfui_wire.c` implements the length-prefixed frame described in -[../protocol/PROTOCOL.md](../protocol/PROTOCOL.md), and nothing else exists yet: -no listener, no JSON parsing, no Redis, no PF ioctl. - -It is here now so the protocol spec has a second implementation checking it. The -framing vectors in `../protocol/vectors/framing.tsv` are run by both this suite -and the Python one, so the two cannot drift apart unnoticed. - -## Build and test - -``` -make test -``` - -Requires only a C99 compiler. The test binary takes the vector file as its first -argument, defaulting to the shared copy. - -## Still to write - -Roughly in dependency order: - -- JSON payload parsing, and lz4 decompression bounded to `PFUI_MAX_MESSAGE` -- address validation (globally routable unicast only, canonicalised) -- PF table ioctl: `DIOCRADDADDRS`, `DIOCRDELADDRS`, and `DIOCRGETADDRS`, which - the Python server never implemented and currently shells out to `pfctl` for -- expiry state, whether in Redis or in-process -- persist-file writer with the same locking discipline as the Python version -- listener with bounded concurrency, privilege drop, and an rc.d script - -`../server-python/` remains the reference for all of the above, and -`../protocol/PROTOCOL.md` is normative where the two disagree. diff --git a/server-c/include/pfui_wire.h b/server-c/include/pfui_wire.h deleted file mode 100644 index b4cd7cb..0000000 --- a/server-c/include/pfui_wire.h +++ /dev/null @@ -1,47 +0,0 @@ -/* - * PFUI wire framing. See ../../protocol/PROTOCOL.md for the normative spec and - * ../../protocol/vectors/framing.tsv for the shared conformance vectors. - * - * This is the framing layer only: it hands back the payload as opaque bytes and - * takes no view on JSON or lz4. - */ - -#ifndef PFUI_WIRE_H -#define PFUI_WIRE_H - -#include -#include - -#define PFUI_HEADER_LEN 4u -#define PFUI_MAX_MESSAGE (1u << 20) /* 1 MiB, must match the Python side */ - -typedef enum { - PFUI_OK = 0, - PFUI_SHORT, /* no complete header yet; peer closed or more to come */ - PFUI_BAD_LENGTH,/* declared length is zero or above PFUI_MAX_MESSAGE */ - PFUI_TRUNCATED, /* header is complete but the payload is not */ - PFUI_NOSPACE /* caller's buffer cannot hold the frame */ -} pfui_status; - -/* - * Read the declared payload length from a 4-byte big-endian header. Rejects the - * length before a caller has any reason to allocate for it. - */ -pfui_status pfui_header_decode(const uint8_t *buf, size_t len, uint32_t *out_len); - -/* - * Point *payload at the payload inside a complete frame. No copy is made, so - * *payload remains valid only as long as buf does. - */ -pfui_status pfui_frame_decode(const uint8_t *buf, size_t len, - const uint8_t **payload, uint32_t *payload_len); - -/* - * Write header + payload into out. Returns the total bytes written via *written. - */ -pfui_status pfui_frame_encode(uint8_t *out, size_t out_cap, const uint8_t *payload, - uint32_t payload_len, size_t *written); - -const char *pfui_status_str(pfui_status status); - -#endif /* PFUI_WIRE_H */ diff --git a/server-c/src/pfui_wire.c b/server-c/src/pfui_wire.c deleted file mode 100644 index 01e545e..0000000 --- a/server-c/src/pfui_wire.c +++ /dev/null @@ -1,85 +0,0 @@ -#include "pfui_wire.h" - -#include - -pfui_status -pfui_header_decode(const uint8_t *buf, size_t len, uint32_t *out_len) -{ - uint32_t declared; - - if (buf == NULL || out_len == NULL) - return PFUI_SHORT; - if (len < PFUI_HEADER_LEN) - return PFUI_SHORT; - - declared = ((uint32_t)buf[0] << 24) | ((uint32_t)buf[1] << 16) | - ((uint32_t)buf[2] << 8) | (uint32_t)buf[3]; - - /* Checked before the caller reads or allocates the payload */ - if (declared == 0 || declared > PFUI_MAX_MESSAGE) - return PFUI_BAD_LENGTH; - - *out_len = declared; - return PFUI_OK; -} - -pfui_status -pfui_frame_decode(const uint8_t *buf, size_t len, const uint8_t **payload, - uint32_t *payload_len) -{ - uint32_t declared; - pfui_status status; - - if (payload == NULL || payload_len == NULL) - return PFUI_SHORT; - - status = pfui_header_decode(buf, len, &declared); - if (status != PFUI_OK) - return status; - - if (len - PFUI_HEADER_LEN < declared) - return PFUI_TRUNCATED; - - *payload = buf + PFUI_HEADER_LEN; - *payload_len = declared; - return PFUI_OK; -} - -pfui_status -pfui_frame_encode(uint8_t *out, size_t out_cap, const uint8_t *payload, - uint32_t payload_len, size_t *written) -{ - if (out == NULL || payload == NULL || written == NULL) - return PFUI_NOSPACE; - if (payload_len == 0 || payload_len > PFUI_MAX_MESSAGE) - return PFUI_BAD_LENGTH; - if (out_cap < (size_t)payload_len + PFUI_HEADER_LEN) - return PFUI_NOSPACE; - - out[0] = (uint8_t)(payload_len >> 24); - out[1] = (uint8_t)(payload_len >> 16); - out[2] = (uint8_t)(payload_len >> 8); - out[3] = (uint8_t)(payload_len); - memcpy(out + PFUI_HEADER_LEN, payload, payload_len); - - *written = (size_t)payload_len + PFUI_HEADER_LEN; - return PFUI_OK; -} - -const char * -pfui_status_str(pfui_status status) -{ - switch (status) { - case PFUI_OK: - return "ok"; - case PFUI_SHORT: - return "short"; - case PFUI_BAD_LENGTH: - return "length"; - case PFUI_TRUNCATED: - return "truncated"; - case PFUI_NOSPACE: - return "nospace"; - } - return "unknown"; -} diff --git a/server-c/tests/test_vectors.c b/server-c/tests/test_vectors.c deleted file mode 100644 index 1adfd95..0000000 --- a/server-c/tests/test_vectors.c +++ /dev/null @@ -1,124 +0,0 @@ -/* - * Runs the shared framing vectors from ../../protocol/vectors/framing.tsv. - * - * The Python implementation runs the same file, so a divergence between the two - * fails here or in protocol/python/tests/test_vectors.py, not in production. - */ - -#include "pfui_wire.h" - -#include -#include -#include - -#define MAX_HEX (2 * (PFUI_MAX_MESSAGE + PFUI_HEADER_LEN) + 1) - -static int failures; -static int checks; - -static size_t -unhex(const char *hex, uint8_t *out, size_t out_cap) -{ - size_t len = strlen(hex), i; - - if (strcmp(hex, "-") == 0 || len % 2 != 0 || len / 2 > out_cap) - return 0; - for (i = 0; i < len / 2; i++) { - unsigned int byte; - if (sscanf(hex + 2 * i, "%2x", &byte) != 1) - return 0; - out[i] = (uint8_t)byte; - } - return len / 2; -} - -static void -check(const char *name, int condition, const char *detail) -{ - checks++; - if (!condition) { - failures++; - fprintf(stderr, "FAIL %s: %s\n", name, detail); - } -} - -static void -run_vector(const char *name, const char *frame_hex, const char *expect, - const char *payload_hex) -{ - static uint8_t frame[PFUI_MAX_MESSAGE + PFUI_HEADER_LEN]; - static uint8_t want[PFUI_MAX_MESSAGE]; - const uint8_t *payload = NULL; - uint32_t payload_len = 0; - size_t frame_len, want_len; - pfui_status status; - - frame_len = strcmp(frame_hex, "-") == 0 ? 0 : unhex(frame_hex, frame, sizeof(frame)); - status = pfui_frame_decode(frame, frame_len, &payload, &payload_len); - - if (strcmp(expect, "ok") == 0) { - check(name, status == PFUI_OK, pfui_status_str(status)); - if (status != PFUI_OK) - return; - want_len = unhex(payload_hex, want, sizeof(want)); - check(name, payload_len == want_len, "payload length mismatch"); - check(name, payload_len == want_len && - memcmp(payload, want, want_len) == 0, "payload bytes mismatch"); - - /* Re-encoding the payload must reproduce the frame byte for byte */ - { - static uint8_t round[PFUI_MAX_MESSAGE + PFUI_HEADER_LEN]; - size_t written = 0; - status = pfui_frame_encode(round, sizeof(round), payload, - payload_len, &written); - check(name, status == PFUI_OK, "re-encode failed"); - check(name, written == frame_len && - memcmp(round, frame, frame_len) == 0, "round trip differs"); - } - } else { - check(name, strcmp(pfui_status_str(status), expect) == 0, - pfui_status_str(status)); - } -} - -int -main(int argc, char **argv) -{ - const char *path = argc > 1 ? argv[1] : "../protocol/vectors/framing.tsv"; - char line[MAX_HEX + 256]; - FILE *fp; - - fp = fopen(path, "r"); - if (fp == NULL) { - fprintf(stderr, "cannot open vectors: %s\n", path); - return 2; - } - - while (fgets(line, sizeof(line), fp) != NULL) { - char *name, *frame_hex, *expect, *payload_hex, *newline; - - if (line[0] == '#' || line[0] == '\n') - continue; - newline = strchr(line, '\n'); - if (newline != NULL) - *newline = '\0'; - - name = strtok(line, "\t"); - frame_hex = strtok(NULL, "\t"); - expect = strtok(NULL, "\t"); - payload_hex = strtok(NULL, "\t"); - if (name == NULL || frame_hex == NULL || expect == NULL || - payload_hex == NULL) { - fprintf(stderr, "malformed vector line\n"); - failures++; - continue; - } - if (frame_hex[0] == '\0') - frame_hex = "-"; - run_vector(name, frame_hex, expect, payload_hex); - } - fclose(fp); - - printf("%d checks, %d failures\n", checks, failures); - return failures == 0 ? 0 : 1; -} diff --git a/server-python/README.md b/server-python/README.md index 5c8ed41..29510d4 100644 --- a/server-python/README.md +++ b/server-python/README.md @@ -18,5 +18,5 @@ format is specified in [../protocol/PROTOCOL.md](../protocol/PROTOCOL.md); the installer places `pfui_wire.py` beside the daemon, and the `pfui` package holds only what is server-specific. -`../server-c/` is a second implementation of the same protocol, currently +`../server-rust/` is a second implementation of the same protocol, currently framing only. Where the two disagree, the protocol spec is normative. diff --git a/server-python/pfui_firewall.py b/server-python/pfui_firewall.py index 25549d7..4809b97 100644 --- a/server-python/pfui_firewall.py +++ b/server-python/pfui_firewall.py @@ -128,8 +128,7 @@ def load_config(location: str = CONFIG_LOCATION) -> dict: + ", ".join(missing) ) - # Checked at load rather than per message: without the codec every message - # would be refused as undecodable, which reads as a client fault + # At load, not per message if cfg["COMPRESS"] and not HAVE_LZ4: raise ValueError( "COMPRESS is True but the lz4 package is not installed; install " @@ -560,8 +559,7 @@ def __init__(self, *args, **kwargs): self.logger.addHandler( SysLogHandler(address=find_syslog(), facility=SysLogHandler.LOG_DAEMON) ) - # Normalised before it is read anywhere: matching the raw text made - # 'debug' silently select ERROR, self.stats included + # Normalised before self.stats reads it self.cfg["LOG_LEVEL"] = str(self.cfg["LOG_LEVEL"]).strip().upper() # Both the timing probes and the summary below use this one flag @@ -1065,7 +1063,7 @@ def recv_exactly(n): return except BadLength as e: # Reported separately from a truncated payload, as PROTOCOL.md - # documents and server-c already distinguishes: a bad prefix is + # documents and server-rust 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}" diff --git a/server-python/pfui_firewall.yml b/server-python/pfui_firewall.yml index 578c298..b79b765 100644 --- a/server-python/pfui_firewall.yml +++ b/server-python/pfui_firewall.yml @@ -45,8 +45,10 @@ TTL_MULTIPLIER: 4 # Expire entries after RR TTL * TTL_MULTIPLIER (Br # means do-not-cache # PF Tables & Files -CTL: IOCTL # IOCTL = ioctl kernel interface (recommended, requires DEVPF), PFCTL = pfctl cli interface +CTL: IOCTL # IOCTL = ioctl kernel interface (recommended, requires DEVPF), PFCTL = pfctl cli interface. + # No fallback between them: the Rust daemon refuses to start if the chosen path cannot reach the tables DEVPF: /dev/pf # PF ioctl interface +PLEDGE: True # Rust daemon only: pledge(2) after startup. False is logged on every start AF4_TABLE: pfui_ipv4_domains # IPv4 PF Table AF4_FILE: /var/db/pfui/ipv4_domains # IPv4 PF Persist file - Used during PF reload AF6_TABLE: pfui_ipv6_domains # IPv6 PF Table diff --git a/server-rust/README.md b/server-rust/README.md index d48dac9..46ba63a 100644 --- a/server-rust/README.md +++ b/server-rust/README.md @@ -7,20 +7,30 @@ same Redis schema, same reply strings, same rc.d service name, binary at ## Status -Complete and validated on OpenBSD 7.9: TCP, UNIX and UDP listeners, bounded -worker pool, the full receiver decision tree with the frozen reply -vocabulary, PF tables via ioctl or pfctl, Redis, persist files, the -scan/sync expiry loop, the rc.d script, unveil, and the installer +In production on OpenBSD 7.9: TCP, UNIX and UDP listeners, bounded worker +pool, the full receiver decision tree with the frozen reply vocabulary, PF +tables via ioctl or pfctl, Redis, persist files, the scan/sync expiry loop, +the rc.d script, unveil and pledge, and the installer (`../install-server-rust.sh`). -The validation covered the parts no container can reach: the struct layouts -and all three ioctl numbers match `net/pfvar.h` exactly, the table ioctls -were exercised against a live kernel, the rc.d lifecycle behaves (including -`reload` refusing), and a real resolver's answers reached the PF tables, -Redis and the persist files on a firewall carrying live traffic. - -pledge(2) is not enabled yet; it comes only after that pass has been repeated -with it on (see DECISIONS.md). +`CTL` chooses one control path and the daemon never falls back to the other. +`IOCTL` drives `/dev/pf` directly and forks nothing; `PFCTL` runs `pfctl(8)`. +At startup the daemon reads both tables through the chosen path and refuses +to serve if it cannot, naming the cause and the remedy, so a broken path is a +failed `rcctl start` rather than a daemon quietly running slow (DECISIONS.md, +"CTL: IOCTL is the ioctl and nothing else"). + +The sandbox follows `CTL`. The `pf` pledge promise does not permit the +table-address ioctls, so under `IOCTL` they run in a PF child forked before +the parent pledges: the child alone holds `/dev/pf` and answers one ioctl per +request over a socketpair, and the parent's pledge is `stdio rpath wpath cpath +flock fattr inet`, plus `unix` with a local socket. Under `PFCTL` there is no +child; the pledge gains `proc exec` and `/dev/pf` and the pfctl paths are +unveiled. `PLEDGE: False` is an emergency switch and is logged on every start +(DECISIONS.md, "The sandbox follows CTL"). + +Exit codes: 2 config, 3 Redis client, 4 sync thread, 5 UDP gate, 6 bind or +sandbox, 7 PF control path unusable or PF child not started. - `src/wire.rs` — the length-prefixed frame and bounded lz4/JSON decode from [../protocol/PROTOCOL.md](../protocol/PROTOCOL.md) @@ -29,7 +39,10 @@ with it on (see DECISIONS.md). - `src/persist.rs` — persist files under the same flock discipline - `src/pf/` — pfr_table/pfioc_table/pfr_addr layouts, `DIOCRADDADDRS`/ `DIOCRDELADDRS`/`DIOCRGETADDRS` (the last is new; server-python shells out - to `pfctl -T show` instead), and the pfctl fallback + to `pfctl -T show` instead), the PF child that issues them, the pfctl path, + and the startup probe +- `src/platform.rs` — the unveil plan and the pledge promises as data, + tested everywhere; the two system calls behind them on OpenBSD - `src/config.rs` — same yml, same keys, coercive like the Python loader - `src/listener.rs` — accept loops, shed-beyond-2×MAX_WORKERS pool, the fail-closed unix bind path @@ -38,21 +51,22 @@ with it on (see DECISIONS.md). - `src/main.rs` — foreground daemon (`-f config`, `-n` check, `-d` stderr) Both shared vector suites run here — `../protocol/vectors/framing.tsv` (also -run by `server-c` and `protocol/python`) and `../protocol/vectors/messages.json` +run by `protocol/python`) and `../protocol/vectors/messages.json` (also run by `protocol/python`) — plus a checked-in python-lz4-produced frame, so the implementations cannot drift apart unnoticed. ## Build and test -Rust 1.94 (the rustc in OpenBSD-stable ports; `rust-version` in Cargo.toml). +Rust 1.94 (the rustc OpenBSD-stable packages; `rust-version` in Cargo.toml). ``` cargo test cargo build --release --locked ``` -The PF ioctl code, once it exists, is `#[cfg(target_os = "openbsd")]`; -everything else builds and tests on any platform. +The PF ioctl, unveil and pledge calls are `#[cfg(target_os = "openbsd")]`; +everything else, including the unveil plan and promise set, builds and tests +on any platform. The OpenBSD job below is what compiles and runs the rest. ## End-to-end test @@ -67,15 +81,30 @@ a stateful pfctl stub for the tables. Verifies the rr and cache paths, the merged-hash relabelling, multi-record and AAAA answers, sync-loop stability and clean shutdown. +## OpenBSD in CI + +`.github/workflows/openbsd.yml` runs `../tests/openbsd/run.sh` inside a real +OpenBSD VM on every relevant change. It runs both installers +unattended, loads a tables-only pf.conf, runs this crate's tests on the +target (including the live ioctl suite against the real tables), then sends +live answers from the resolver it installed to the daemon it installed over +TCP, the unix socket and both at once, under `IOCTL` and `PFCTL`, and checks +each answer in the PF table, Redis and the persist file. Further passes prove +expiry, that a `pfctl -f` reload repopulates the table from the persist file, +that stop leaves no socket behind, and that a table missing from the ruleset +is refused loudly at start. The same script runs by hand as root on a scratch +OpenBSD machine; it rewrites `/etc/pf.conf` and both PFUI configurations. + ## Still to do -- pledge(2), enabled only after the OpenBSD pass above is repeated with it on - (see DECISIONS.md for the sequencing rationale) +- Per-transport `COMPRESS`, so a local socket can run uncompressed while a + remote resolver over TCP compresses +- Authentication of the TCP transport, which today relies on pf.conf alone ## Release builds OpenBSD amd64 is a Rust tier-3 target with no rustup std, so release -binaries are built natively on an OpenBSD host with the ports rustc: +binaries are built natively on an OpenBSD host with the packaged rustc: ``` pkg_add rust @@ -83,8 +112,8 @@ cargo build --release --locked ``` `--locked` enforces the committed Cargo.lock; `rust-version` in Cargo.toml -is the ports rustc of the current -stable release, so the ports toolchain is -never too old by construction. A firewall that should not carry a toolchain +is the rustc the current -stable release packages, so the packaged toolchain +is never too old by construction. A firewall that should not carry a toolchain installs the artifact instead of building: ``` diff --git a/server-rust/rc.d/pfui_firewall b/server-rust/rc.d/pfui_firewall index eb89da2..a074a3a 100644 --- a/server-rust/rc.d/pfui_firewall +++ b/server-rust/rc.d/pfui_firewall @@ -12,6 +12,8 @@ daemon_user="_pfui_firewall" . /etc/rc.d/rc.subr rc_bg=YES +# Matches the daemon and its PF child; the child ignores the TERM this sends +# and exits when the parent closes their socket pexp="${daemon}.*" rc_pre() { diff --git a/server-rust/src/backends.rs b/server-rust/src/backends.rs index 9ab1f7d..b757d52 100644 --- a/server-rust/src/backends.rs +++ b/server-rust/src/backends.rs @@ -10,7 +10,7 @@ use std::sync::Arc; use crate::config::Config; use crate::logger::Logger; -use crate::pf::{self, PfConfig}; +use crate::pf::{self, PfConfig, SharedDev}; use crate::receiver::Backends; use crate::store::{self, Kind, RedisDb}; @@ -18,6 +18,7 @@ pub struct RealBackends { cfg: Arc, log: Arc, client: redis::Client, + pf_dev: SharedDev, } thread_local! { @@ -30,7 +31,7 @@ impl RealBackends { /// REDIS_HOST is resolved once here, before any unveil or pledge lockdown. /// The client connects lazily, so a firewall booting before Redis still /// starts and whitelists. - pub fn new(cfg: Arc, log: Arc) -> Result { + pub fn new(cfg: Arc, log: Arc, pf_dev: SharedDev) -> Result { use std::net::ToSocketAddrs; let addr = (cfg.redis_host.as_str(), cfg.redis_port) .to_socket_addrs() @@ -39,13 +40,26 @@ impl RealBackends { .ok_or_else(|| format!("REDIS_HOST {} resolved to nothing", cfg.redis_host))?; let url = format!("redis://{}:{}/{}", addr.ip(), addr.port(), cfg.redis_db); let client = redis::Client::open(url).map_err(|e| e.to_string())?; - Ok(RealBackends { cfg, log, client }) + Ok(RealBackends { + cfg, + log, + client, + pf_dev, + }) + } + + /// Read each configured table through the control path, before serving. + pub fn probe_pf(&self) -> Result<(), String> { + pf::probe( + &self.pf_config(), + &[self.cfg.af4_table.as_str(), self.cfg.af6_table.as_str()], + ) } fn pf_config(&self) -> PfConfig<'_> { PfConfig { ctl: self.cfg.ctl, - devpf: &self.cfg.devpf, + dev: &self.pf_dev, pfctl: Path::new(pf::DEFAULT_PFCTL), } } @@ -80,18 +94,11 @@ impl RealBackends { impl Backends for RealBackends { fn table_push(&self, table: &str, ips: &[IpAddr]) { - match pf::table_push(&self.pf_config(), table, ips) { - Ok(outcome) => { - if let Some(ioctl_err) = outcome.ioctl_error { - self.log.error(&format!( - "IOCTL failed installing into PF table {table} ({ioctl_err}); \ - pfctl served the call" - )); - } - } - Err(e) => self.log.error(&format!( + // The ACK still follows; PF keeps denying, so a failed push is loud, not unsafe + if let Err(e) = pf::table_push(&self.pf_config(), table, ips) { + self.log.error(&format!( "Failed to install {ips:?} into PF table {table}: {e}" - )), + )); } } @@ -146,18 +153,7 @@ impl crate::sync::SyncOps for RealBackends { } fn table_show(&self, table: &str) -> Result, String> { - match pf::table_show(&self.pf_config(), table) { - Ok(outcome) => { - if let Some(ioctl_err) = outcome.ioctl_error { - self.log.error(&format!( - "IOCTL failed reading PF table {table} ({ioctl_err}); \ - pfctl served the call" - )); - } - Ok(outcome.value) - } - Err(e) => Err(e.to_string()), - } + pf::table_show(&self.pf_config(), table).map_err(|e| e.to_string()) } fn table_pop(&self, table: &str, entries: &[String]) { @@ -172,18 +168,10 @@ impl crate::sync::SyncOps for RealBackends { if ips.is_empty() { return; } - match pf::table_pop(&self.pf_config(), table, &ips) { - Ok(outcome) => { - if let Some(ioctl_err) = outcome.ioctl_error { - self.log.error(&format!( - "IOCTL failed clearing PF table {table} ({ioctl_err}); \ - pfctl served the call" - )); - } - } - Err(e) => self.log.error(&format!( + if let Err(e) = pf::table_pop(&self.pf_config(), table, &ips) { + self.log.error(&format!( "Failed to clear {ips:?} from PF table {table}: {e}" - )), + )); } } diff --git a/server-rust/src/config.rs b/server-rust/src/config.rs index a601ea8..76742af 100644 --- a/server-rust/src/config.rs +++ b/server-rust/src/config.rs @@ -50,6 +50,8 @@ pub struct Config { pub ttl_multiplier: u32, pub ctl: Ctl, pub devpf: PathBuf, + /// pledge(2) after startup; False is logged at error level on every start. + pub pledge: bool, pub af4_table: String, pub af4_file: PathBuf, pub af6_table: String, @@ -173,9 +175,7 @@ impl Doc { .and_then(as_int) .unwrap_or(default) } - /// An integer that has to fit where it is going. Casting instead truncated - /// silently: SOCKET_PORT: 65536 bound port 0, and REDIS_DB: 256 selected - /// database 0, both without a word in the log. + /// An integer that must fit its destination: refused, never truncated. fn ranged( &self, key: &'static str, @@ -260,8 +260,7 @@ pub fn load_config_str(text: &str) -> Result { return Err(ConfigError::NoListener); } - // Any LOG_LEVEL other than DEBUG or INFO is ERROR. Trimmed and folded to - // upper case first: matching the raw text made 'debug' silently mean ERROR. + // Any LOG_LEVEL other than DEBUG or INFO is ERROR let log_level = match doc .text("LOG_LEVEL", "DEBUG") .trim() @@ -299,6 +298,7 @@ pub fn load_config_str(text: &str) -> Result { ttl_multiplier: doc.ranged("TTL_MULTIPLIER", 2, 0, u32::MAX as i64)? as u32, ctl, devpf: PathBuf::from(doc.text("DEVPF", "/dev/pf")), + pledge: doc.boolean("PLEDGE", true), af4_table: doc.required("AF4_TABLE")?, af4_file: PathBuf::from(doc.required("AF4_FILE")?), af6_table: doc.required("AF6_TABLE")?, @@ -330,9 +330,7 @@ AF6_FILE: /var/db/pfui/ipv6_domains #[test] fn an_integer_that_does_not_fit_is_refused_not_truncated() { - // Casting silently: 65536 bound port 0, and REDIS_DB: 256 selected - // database 0, so the daemon served the wrong port or wrote the wrong - // Redis database with nothing in the log + // Truncation would bind port 0 or select database 0 let listener = "SOCKET_LISTEN: 10.10.1.254\n"; for (key, value) in [ ("SOCKET_PORT", "65536"), @@ -359,10 +357,16 @@ AF6_FILE: /var/db/pfui/ipv6_domains assert_eq!(cfg.redis_db, 255); } + #[test] + fn pledge_is_on_unless_switched_off() { + let listener = "SOCKET_LISTEN: 10.10.1.254\n"; + assert!(with(listener).unwrap().pledge); + assert!(!with(&format!("{listener}PLEDGE: False\n")).unwrap().pledge); + } + #[test] fn log_level_is_read_case_insensitively() { - // Matching the raw text made a lower-case level mean ERROR, so a - // resolver set to debug logged nothing but faults + // Lower case must not read as ERROR let level = |text: &str| { let yaml = format!("SOCKET_LISTEN: 10.10.1.254\nLOG_LEVEL: '{text}'\n"); with(&yaml).unwrap().log_level diff --git a/server-rust/src/listener.rs b/server-rust/src/listener.rs index 54efdef..6a339a9 100644 --- a/server-rust/src/listener.rs +++ b/server-rust/src/listener.rs @@ -349,11 +349,8 @@ impl WorkerPool { guard.recv() }; match job { - // Contained per job: a panic here used to unwind the - // whole worker, so a repeatable fault retired the pool - // one thread at a time until every message was shed and - // nothing was whitelisted at all. The lock is released - // above, so the channel cannot be poisoned by this. + // Caught per job so one fault cannot retire the pool; + // the receive lock is already released Ok(job) => { let run = std::panic::AssertUnwindSafe(|| receiver::handle(job, &ctx)); if let Err(panic) = std::panic::catch_unwind(run) { @@ -418,8 +415,6 @@ impl Submitter { match self.tx.try_send(job) { Ok(()) => Submitted::Accepted, Err(TrySendError::Full(_)) => Submitted::Shed, - // Reported as success before, so a job dropped after shutdown looked - // handled and the loop kept accepting work nothing would ever run Err(TrySendError::Disconnected(_)) => Submitted::PoolStopped, } } @@ -457,10 +452,17 @@ impl StreamListener { Ok((Box::new(conn), peer.to_string())) } StreamListener::Unix(l, path) => { - let (conn, _) = l.accept()?; - conn.set_nonblocking(false)?; - conn.set_read_timeout(Some(timeout))?; - conn.set_write_timeout(Some(timeout))?; + // Named per step: a peer that is already gone fails setsockopt on + // some kernels, and the connection is dropped with its data + let step = + |what: &str, e: io::Error| io::Error::new(e.kind(), format!("{what}: {e}")); + let (conn, _) = l.accept().map_err(|e| step("accept", e))?; + conn.set_nonblocking(false) + .map_err(|e| step("set_nonblocking", e))?; + conn.set_read_timeout(Some(timeout)) + .map_err(|e| step("set_read_timeout", e))?; + conn.set_write_timeout(Some(timeout)) + .map_err(|e| step("set_write_timeout", e))?; // accept() reports no peer on AF_UNIX, so the socket path // names the sender in the log Ok((Box::new(conn), path.display().to_string())) diff --git a/server-rust/src/main.rs b/server-rust/src/main.rs index cf22745..6b2176e 100644 --- a/server-rust/src/main.rs +++ b/server-rust/src/main.rs @@ -1,15 +1,16 @@ //! PFUI_Firewall daemon. Runs in the foreground; rc.d owns backgrounding via //! rc_bg. Exit codes: 2 config, 3 Redis client, 4 sync thread, 5 UDP gate, -//! 6 bind. +//! 6 bind or sandbox, 7 PF control path unusable. use std::path::PathBuf; -use std::sync::atomic::AtomicBool; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; use pfui_firewall::backends::RealBackends; use pfui_firewall::config::{self, Proto}; use pfui_firewall::listener::{self, StreamListener, WorkerPool}; use pfui_firewall::logger::Logger; +use pfui_firewall::pf::{self, Ctl}; use pfui_firewall::receiver::Ctx; const CONFIG_LOCATION: &str = "/etc/pfui_firewall.yml"; @@ -75,9 +76,22 @@ fn run() -> i32 { Logger::syslog(cfg.log_level, cfg.logging) }); + // Under IOCTL the table ioctls run in a child forked now, before any + // thread exists and before the pledge, which permits none of them + let pf_dev: pf::SharedDev = match cfg.ctl { + Ctl::Ioctl => match pf::privsep::spawn(&cfg.devpf) { + Ok(child) => std::sync::Mutex::new(Box::new(child)), + Err(e) => { + log.error(&format!("cannot start the PF child: {e}")); + return 7; + } + }, + Ctl::Pfctl => std::sync::Mutex::new(Box::new(pf::ioctl::DevPf(cfg.devpf.clone()))), + }; + // Resolves REDIS_HOST once and never probes the server: a firewall // booting before Redis must still start and whitelist - let backends = match RealBackends::new(Arc::clone(&cfg), Arc::clone(&log)) { + let backends = match RealBackends::new(Arc::clone(&cfg), Arc::clone(&log), pf_dev) { Ok(b) => Arc::new(b), Err(e) => { log.error(&format!("Failed to set up Redis client: {e}")); @@ -123,6 +137,13 @@ fn run() -> i32 { } None => None, }; + // Every failure from here on has a socket node to remove + let abort = |code: i32| -> i32 { + if let Some(path) = &cfg.socket_unix { + listener::remove_unix_socket(path); + } + code + }; enum Network { Tcp(std::net::TcpListener), @@ -144,10 +165,7 @@ fn run() -> i32 { "Failed to bind TCP {listen}:{}: {e}", cfg.socket_port )); - if let Some((_, path)) = &unix { - listener::remove_unix_socket(path); - } - return 6; + return abort(6); } } } @@ -166,10 +184,7 @@ fn run() -> i32 { "Failed to bind UDP {listen}:{}: {e}", cfg.socket_port )); - if let Some((_, path)) = &unix { - listener::remove_unix_socket(path); - } - return 6; + return abort(6); } } } @@ -177,21 +192,42 @@ fn run() -> i32 { }; // Binds, group lookup and host resolution are done; the filesystem view - // now narrows to what serving needs - if let Err(e) = pfui_firewall::platform::lockdown(&cfg, &opts.config) { - log.error(&format!("Cannot apply unveil: {e}; refusing to serve")); - if let Some(path) = &cfg.socket_unix { - listener::remove_unix_socket(path); + // narrows to what serving needs, then the process is pledged + match pfui_firewall::platform::lockdown(&cfg, &opts.config) { + Ok(sandbox) => match &sandbox.promises { + Some(promises) => log.info(&format!( + "[+] Sandboxed: {} paths unveiled, pledged \"{promises}\"", + sandbox.unveiled + )), + None if cfg.pledge => log.info(&format!("[+] {} paths unveiled", sandbox.unveiled)), + // Error level on every start: an unpledged firewall must not look normal + None => log.error(&format!( + "[!] Running UNPLEDGED (PLEDGE: False in {}); {} paths unveiled", + opts.config.display(), + sandbox.unveiled + )), + }, + Err(e) => { + log.error(&format!("Cannot apply the sandbox: {e}; refusing to serve")); + return abort(6); } - return 6; } - // Background expiry, one thread per address family. Started only once the - // daemon has committed to serving: spawned before the UDP gate and the - // binds, these threads were already rewriting PF tables, Redis and the - // persist files while run() was returning a refusal, and nothing joined - // them on the way out. Starting them after bind_unix also keeps them clear - // of the umask it narrows, which is process-global. + // Proven before anything is accepted: no ACK for an update PF never saw + if let Err(e) = backends.probe_pf() { + log.error(&format!("{e}; refusing to serve")); + return abort(7); + } + log.info(&format!( + "[+] PF tables {} and {} reachable via CTL: {}", + cfg.af4_table, + cfg.af6_table, + cfg.ctl.as_str() + )); + + // Background expiry, one thread per address family. Started last: nothing + // rewrites PF, Redis or the persist files until the daemon is committed to + // serving, and bind_unix has restored the process umask let mut sync_threads = Vec::new(); for (table, file) in pfui_firewall::backends::persist_files(&cfg) { match pfui_firewall::sync::spawn( @@ -205,14 +241,11 @@ fn run() -> i32 { Ok(handle) => sync_threads.push(handle), Err(e) => { log.error(&format!("Scanning thread failed: {e}")); - term.store(true, std::sync::atomic::Ordering::Relaxed); + term.store(true, Ordering::Relaxed); for handle in sync_threads { let _ = handle.join(); } - if let Some(path) = &cfg.socket_unix { - listener::remove_unix_socket(path); - } - return 4; + return abort(4); } } } diff --git a/server-rust/src/pf/ioctl.rs b/server-rust/src/pf/ioctl.rs index bccde8d..0e0f120 100644 --- a/server-rust/src/pf/ioctl.rs +++ b/server-rust/src/pf/ioctl.rs @@ -4,7 +4,6 @@ //! be tested on any platform; only the raw ioctl on /dev/pf is OpenBSD code. use std::net::IpAddr; -use std::path::Path; use super::structs::{ ip_of, pfr_addr_of, PfiocTable, PfrAddr, PfrTable, DIOCRADDADDRS, DIOCRDELADDRS, DIOCRGETADDRS, @@ -18,23 +17,20 @@ pub trait PfDev { } fn io_for(table: &str, size: usize) -> Result { - let table = PfrTable::named(table).ok_or(PfError::TableName)?; - Ok(PfiocTable { - pfrio_table: table, - pfrio_buffer: std::ptr::null_mut(), - pfrio_esize: std::mem::size_of::() as i32, - pfrio_size: size as i32, - pfrio_size2: 0, - pfrio_nadd: 0, - pfrio_ndel: 0, - pfrio_nchange: 0, - pfrio_flags: 0, - pfrio_ticket: 0, - }) + // Zeroed first so the padding is defined: the PF child sends this as bytes + let mut io: PfiocTable = unsafe { std::mem::zeroed() }; + io.pfrio_table = PfrTable::named(table).ok_or(PfError::TableName)?; + io.pfrio_esize = std::mem::size_of::() as i32; + io.pfrio_size = size as i32; + Ok(io) } /// Install addresses; returns the kernel's count of effective additions. -pub fn table_add(dev: &mut impl PfDev, table: &str, ips: &[IpAddr]) -> Result { +pub fn table_add( + dev: &mut (impl PfDev + ?Sized), + table: &str, + ips: &[IpAddr], +) -> Result { let mut buf: Vec = ips.iter().map(pfr_addr_of).collect(); let mut io = io_for(table, buf.len())?; dev.call(DIOCRADDADDRS, &mut io, &mut buf)?; @@ -43,7 +39,11 @@ pub fn table_add(dev: &mut impl PfDev, table: &str, ips: &[IpAddr]) -> Result Result { +pub fn table_del( + dev: &mut (impl PfDev + ?Sized), + table: &str, + ips: &[IpAddr], +) -> Result { let mut buf: Vec = ips.iter().map(pfr_addr_of).collect(); let mut io = io_for(table, buf.len())?; dev.call(DIOCRDELADDRS, &mut io, &mut buf)?; @@ -55,7 +55,7 @@ pub fn table_del(dev: &mut impl PfDev, table: &str, ips: &[IpAddr]) -> Result Result, PfError> { +pub fn table_get(dev: &mut (impl PfDev + ?Sized), table: &str) -> Result, PfError> { let mut io = io_for(table, 0)?; let mut empty: [PfrAddr; 0] = []; dev.call(DIOCRGETADDRS, &mut io, &mut empty)?; @@ -77,37 +77,73 @@ pub fn table_get(dev: &mut impl PfDev, table: &str) -> Result, PfErr Err(PfError::Unstable) } -/// The real /dev/pf, opened per call; it is not a contended resource at these -/// rates. -pub struct DevPf<'a>(pub &'a Path); +/// The real /dev/pf, opened per call. +pub struct DevPf(pub std::path::PathBuf); + +/// The real /dev/pf through a descriptor opened once: the PF child cannot open +/// anything after its lockdown. +pub struct OpenedPf(pub std::fs::File); #[cfg(target_os = "openbsd")] -impl PfDev for DevPf<'_> { +fn raw_call( + fd: std::os::fd::RawFd, + cmd: u64, + io: &mut PfiocTable, + buf: &mut [PfrAddr], +) -> Result<(), PfError> { + if !buf.is_empty() { + io.pfrio_buffer = buf.as_mut_ptr() as *mut std::ffi::c_void; + } + let rc = unsafe { libc::ioctl(fd, cmd as libc::c_ulong, io as *mut _) }; + if rc == -1 { + let err = std::io::Error::last_os_error(); + return Err(PfError::Ioctl { + cmd, + errno: err.to_string(), + raw: err.raw_os_error(), + }); + } + Ok(()) +} + +#[cfg(target_os = "openbsd")] +impl PfDev for DevPf { fn call(&mut self, cmd: u64, io: &mut PfiocTable, buf: &mut [PfrAddr]) -> Result<(), PfError> { use std::os::fd::AsRawFd; let dev = std::fs::OpenOptions::new() .read(true) .write(true) - .open(self.0) - .map_err(|e| PfError::Dev(e.to_string()))?; - if !buf.is_empty() { - io.pfrio_buffer = buf.as_mut_ptr() as *mut std::ffi::c_void; - } - let rc = unsafe { libc::ioctl(dev.as_raw_fd(), cmd as libc::c_ulong, io as *mut _) }; - if rc == -1 { - let err = std::io::Error::last_os_error(); - return Err(PfError::Ioctl { - cmd, - errno: err.to_string(), - raw: err.raw_os_error(), - }); - } - Ok(()) + .open(&self.0) + .map_err(|e| PfError::Dev { + errno: e.to_string(), + raw: e.raw_os_error(), + })?; + raw_call(dev.as_raw_fd(), cmd, io, buf) + } +} + +#[cfg(target_os = "openbsd")] +impl PfDev for OpenedPf { + fn call(&mut self, cmd: u64, io: &mut PfiocTable, buf: &mut [PfrAddr]) -> Result<(), PfError> { + use std::os::fd::AsRawFd; + raw_call(self.0.as_raw_fd(), cmd, io, buf) + } +} + +#[cfg(not(target_os = "openbsd"))] +impl PfDev for DevPf { + fn call( + &mut self, + _cmd: u64, + _io: &mut PfiocTable, + _buf: &mut [PfrAddr], + ) -> Result<(), PfError> { + Err(PfError::Unsupported) } } #[cfg(not(target_os = "openbsd"))] -impl PfDev for DevPf<'_> { +impl PfDev for OpenedPf { fn call( &mut self, _cmd: u64, diff --git a/server-rust/src/pf/mod.rs b/server-rust/src/pf/mod.rs index 737ee3d..68f02d1 100644 --- a/server-rust/src/pf/mod.rs +++ b/server-rust/src/pf/mod.rs @@ -1,16 +1,18 @@ -//! PF table interface, dispatched on CTL: IOCTL tries the kernel interface and -//! falls back to pfctl on error, PFCTL goes straight to the subprocess. -//! -//! Addresses arrive as IpAddr, already validated and canonicalised, so an -//! unparseable string cannot reach the ioctl. +//! PF table interface. CTL selects the kernel ioctl on /dev/pf or the pfctl(8) +//! subprocess; there is no fallback between them. Addresses arrive validated +//! and canonicalised. pub mod ioctl; pub mod pfctl; +pub mod privsep; pub mod structs; use std::fmt; use std::net::IpAddr; use std::path::Path; +use std::sync::Mutex; + +use ioctl::PfDev; pub const DEFAULT_PFCTL: &str = "/sbin/pfctl"; @@ -20,14 +22,25 @@ pub enum Ctl { Pfctl, } +impl Ctl { + pub fn as_str(self) -> &'static str { + match self { + Ctl::Ioctl => "IOCTL", + Ctl::Pfctl => "PFCTL", + } + } +} + +/// Appended to every control-path error. +const REMEDY: &str = "fix that, or set CTL: PFCTL in /etc/pfui_firewall.yml"; + #[derive(Debug)] pub enum PfError { /// Table name cannot fit pfrt_name including its terminator TableName, /// /dev/pf could not be opened - Dev(String), - /// `raw` is kept because ESRCH means the table is absent from the active - /// ruleset, a configuration error worth naming as one. + Dev { errno: String, raw: Option }, + /// `raw` selects the guidance in Display. Ioctl { cmd: u64, errno: String, @@ -37,45 +50,67 @@ pub enum PfError { Unstable, /// The pfctl subprocess failed or could not be executed Pfctl(String), + /// The PF child is gone or answered out of shape + Child(String), /// Built without the OpenBSD ioctl (any other platform) Unsupported, } +/// Operator guidance per errno. +fn explain(raw: Option) -> Option<&'static str> { + match raw { + Some(libc::EACCES) | Some(libc::EPERM) => Some( + "/dev/pf is not writable by this user: it must be group _pfui_firewall, \ + mode 660, which rc.d/pfui_firewall applies on every start", + ), + Some(libc::ENOENT) => Some("/dev/pf does not exist"), + Some(libc::ESRCH) => Some( + "the table is not in the loaded ruleset: declare it in pf.conf as \ + 'table persist file \"...\"' and reload", + ), + Some(libc::ENOTTY) | Some(libc::EINVAL) => { + Some("not a pf device, or a struct layout the kernel does not recognise") + } + _ => None, + } +} + impl fmt::Display for PfError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { PfError::TableName => write!(f, "table name too long for pfr_table"), - PfError::Dev(e) => write!(f, "cannot open pf device: {e}"), + PfError::Dev { errno, raw } => { + write!(f, "cannot open pf device: {errno}")?; + if let Some(why) = explain(*raw) { + write!(f, " ({why}; {REMEDY})")?; + } + Ok(()) + } PfError::Ioctl { cmd, errno, raw } => { write!(f, "pf ioctl {cmd:#x} failed: {errno}")?; - if *raw == Some(libc::ESRCH) { - write!( - f, - " (the table is not in the active ruleset; declare it in \ - pf.conf and reload, or use CTL: PFCTL)" - )?; + if let Some(why) = explain(*raw) { + write!(f, " ({why}; {REMEDY})")?; } Ok(()) } PfError::Unstable => write!(f, "table kept growing across GETADDRS retries"), PfError::Pfctl(e) => write!(f, "{e}"), + PfError::Child(e) => write!(f, "{e}"), PfError::Unsupported => write!(f, "pf ioctl unavailable on this platform"), } } } +/// One device shared by every caller: under IOCTL it is the proxy to the PF +/// child, whose socket carries one call at a time. +pub type SharedDev = Mutex>; + pub struct PfConfig<'a> { pub ctl: Ctl, - pub devpf: &'a Path, + pub dev: &'a SharedDev, pub pfctl: &'a Path, } -/// The count, plus the ioctl error when the pfctl fallback served the call. -pub struct Outcome { - pub value: T, - pub ioctl_error: Option, -} - fn render(ips: &[IpAddr]) -> Vec { ips.iter().map(IpAddr::to_string).collect() } @@ -84,55 +119,57 @@ fn dispatch( cfg: &PfConfig, via_ioctl: impl FnOnce() -> Result, via_pfctl: impl FnOnce() -> Result, -) -> Result, PfError> { +) -> Result { match cfg.ctl { - Ctl::Pfctl => Ok(Outcome { - value: via_pfctl()?, - ioctl_error: None, - }), - Ctl::Ioctl => match via_ioctl() { - Ok(value) => Ok(Outcome { - value, - ioctl_error: None, - }), - Err(ioctl_err) => Ok(Outcome { - value: via_pfctl()?, - ioctl_error: Some(ioctl_err.to_string()), - }), - }, + Ctl::Ioctl => via_ioctl(), + Ctl::Pfctl => via_pfctl(), } } /// Install IPs into the table. Runs before the client is acknowledged. -pub fn table_push(cfg: &PfConfig, table: &str, ips: &[IpAddr]) -> Result, PfError> { +pub fn table_push(cfg: &PfConfig, table: &str, ips: &[IpAddr]) -> Result { dispatch( cfg, - || ioctl::table_add(&mut ioctl::DevPf(cfg.devpf), table, ips), + || ioctl::table_add(&mut **cfg.dev.lock().unwrap(), table, ips), || pfctl::add(cfg.pfctl, table, &render(ips)), ) } /// Remove IPs from the table. -pub fn table_pop(cfg: &PfConfig, table: &str, ips: &[IpAddr]) -> Result, PfError> { +pub fn table_pop(cfg: &PfConfig, table: &str, ips: &[IpAddr]) -> Result { dispatch( cfg, - || ioctl::table_del(&mut ioctl::DevPf(cfg.devpf), table, ips), + || ioctl::table_del(&mut **cfg.dev.lock().unwrap(), table, ips), || pfctl::del(cfg.pfctl, table, &render(ips)), ) } /// Read the table's current contents, canonicalised. -pub fn table_show(cfg: &PfConfig, table: &str) -> Result>, PfError> { +pub fn table_show(cfg: &PfConfig, table: &str) -> Result, PfError> { dispatch( cfg, || { - ioctl::table_get(&mut ioctl::DevPf(cfg.devpf), table) + ioctl::table_get(&mut **cfg.dev.lock().unwrap(), table) .map(|ips| ips.iter().map(IpAddr::to_string).collect()) }, || pfctl::show(cfg.pfctl, table), ) } +/// Read every table through the configured control path. Runs before serving, +/// after the sandbox, so a daemon that cannot reach PF never acknowledges. +pub fn probe(cfg: &PfConfig, tables: &[&str]) -> Result<(), String> { + for table in tables { + table_show(cfg, table).map_err(|e| { + format!( + "PF table {table} is unreachable via CTL: {}: {e}", + cfg.ctl.as_str() + ) + })?; + } + Ok(()) +} + /// Serialises the stub-script tests: a concurrent fork inherits the stub's /// open write fd until it execs, and executing it in that window is ETXTBSY. #[cfg(test)] @@ -145,6 +182,10 @@ mod tests { use std::os::unix::fs::PermissionsExt; use std::path::PathBuf; + fn direct() -> SharedDev { + Mutex::new(Box::new(ioctl::DevPf(PathBuf::from("/dev/pf")))) + } + fn stub(dir: &Path, script: &str) -> PathBuf { let path = dir.join("pfctl"); let mut f = std::fs::File::create(&path).unwrap(); @@ -158,41 +199,103 @@ mod tests { let _serial = crate::pf::STUB_EXEC.lock().unwrap(); let dir = tempfile::tempdir().unwrap(); let pfctl = stub(dir.path(), "exit 0"); + let dev = direct(); let cfg = PfConfig { ctl: Ctl::Pfctl, - devpf: Path::new("/dev/pf"), + dev: &dev, pfctl: &pfctl, }; - let out = table_push(&cfg, "t", &["8.8.8.8".parse().unwrap()]).unwrap(); - assert_eq!(out.value, 1); - assert!(out.ioctl_error.is_none()); + assert_eq!( + table_push(&cfg, "t", &["8.8.8.8".parse().unwrap()]).unwrap(), + 1 + ); } #[test] - fn ioctl_mode_falls_back_to_pfctl_and_reports_the_ioctl_error() { + fn ioctl_mode_never_reaches_for_pfctl() { let _serial = crate::pf::STUB_EXEC.lock().unwrap(); - // Off OpenBSD the ioctl path is the Unsupported stub, which IS the - // fallback scenario: the pfctl stub serves the call + // Table "t" is in no ruleset, so the ioctl fails on every platform: + // ESRCH on OpenBSD, the stub's Unsupported elsewhere. A pfctl that ran + // means a fallback let dir = tempfile::tempdir().unwrap(); - let pfctl = stub(dir.path(), "printf '8.8.8.8\\n'"); + let marker = dir.path().join("pfctl-ran"); + let pfctl = stub( + dir.path(), + &format!("touch {}; printf '8.8.8.8\\n'", marker.display()), + ); + let dev = direct(); let cfg = PfConfig { ctl: Ctl::Ioctl, - devpf: Path::new("/dev/pf"), + dev: &dev, pfctl: &pfctl, }; - let out = table_show(&cfg, "t").unwrap(); - assert_eq!(out.value, ["8.8.8.8"]); - assert!(out.ioctl_error.is_some()); + let shown = table_show(&cfg, "t"); + assert!( + !matches!(shown, Ok(_) | Err(PfError::Pfctl(_))), + "{shown:?}" + ); + let pushed = table_push(&cfg, "t", &["8.8.8.8".parse().unwrap()]); + assert!( + !matches!(pushed, Ok(_) | Err(PfError::Pfctl(_))), + "{pushed:?}" + ); + assert!(!marker.exists(), "pfctl ran under CTL: IOCTL"); } #[test] - fn both_paths_failing_is_an_error() { + fn the_probe_names_the_table_that_failed() { let _serial = crate::pf::STUB_EXEC.lock().unwrap(); + let dir = tempfile::tempdir().unwrap(); + let script = concat!( + "case \"$*\" in *absent*) echo 'Table does not exist.' >&2; exit 1;; esac\n", + "exit 0" + ); + let pfctl = stub(dir.path(), script); + let dev = direct(); let cfg = PfConfig { - ctl: Ctl::Ioctl, - devpf: Path::new("/dev/pf"), - pfctl: Path::new("/nonexistent/pfctl"), + ctl: Ctl::Pfctl, + dev: &dev, + pfctl: &pfctl, }; - assert!(table_pop(&cfg, "t", &["8.8.8.8".parse().unwrap()]).is_err()); + assert!(probe(&cfg, &["present"]).is_ok()); + let err = probe(&cfg, &["present", "absent"]).unwrap_err(); + assert!(err.contains("absent"), "{err}"); + assert!(err.contains("PFCTL"), "{err}"); + } + + #[test] + fn a_control_path_failure_says_what_to_fix() { + let esrch = PfError::Ioctl { + cmd: 0xc4504443, + errno: "No such process".into(), + raw: Some(libc::ESRCH), + } + .to_string(); + assert!(esrch.contains("pf.conf"), "{esrch}"); + assert!(esrch.contains("CTL: PFCTL"), "{esrch}"); + + let eacces = PfError::Dev { + errno: "Permission denied".into(), + raw: Some(libc::EACCES), + } + .to_string(); + assert!(eacces.contains("mode 660"), "{eacces}"); + assert!(eacces.contains("CTL: PFCTL"), "{eacces}"); + + let enoent = PfError::Dev { + errno: "No such file or directory".into(), + raw: Some(libc::ENOENT), + } + .to_string(); + assert!(enoent.contains("/dev/pf does not exist"), "{enoent}"); + + // No guidance for an unrecognised errno + let other = PfError::Ioctl { + cmd: 0xc4504443, + errno: "Input/output error".into(), + raw: Some(libc::EIO), + } + .to_string(); + assert!(other.ends_with("Input/output error"), "{other}"); } } diff --git a/server-rust/src/pf/pfctl.rs b/server-rust/src/pf/pfctl.rs index 1745ef8..6062ef4 100644 --- a/server-rust/src/pf/pfctl.rs +++ b/server-rust/src/pf/pfctl.rs @@ -1,6 +1,5 @@ -//! pfctl(8) subprocess path: the whole story under CTL: PFCTL, and the error -//! fallback under CTL: IOCTL. The binary is passed in by absolute path so -//! tests can point at a stub. +//! pfctl(8) subprocess path (CTL: PFCTL). The binary is passed by absolute path +//! so tests can point at a stub. use std::path::Path; use std::process::Command; diff --git a/server-rust/src/pf/privsep.rs b/server-rust/src/pf/privsep.rs new file mode 100644 index 0000000..c9c0cc4 --- /dev/null +++ b/server-rust/src/pf/privsep.rs @@ -0,0 +1,418 @@ +//! The PF child. The pf pledge promise permits none of DIOCRADDADDRS, +//! DIOCRDELADDRS or DIOCRGETADDRS, and no promise set can, so under CTL: IOCTL +//! a child forked before the parent pledges holds /dev/pf alone and runs each +//! ioctl on the parent's behalf over a socketpair. The child cannot pledge +//! either; it unveils the filesystem away and does nothing but read one +//! request, run one ioctl and write one reply. + +use std::io::{self, Read, Write}; +use std::os::unix::net::UnixStream; +use std::path::Path; +use std::time::Duration; + +use super::ioctl::{OpenedPf, PfDev}; +use super::structs::{PfiocTable, PfrAddr}; +use super::PfError; + +/// Most addresses one request or reply may carry: 52 MiB of pfr_addr. +pub const MAX_ADDRS: usize = 1 << 20; + +/// Request: cmd u64, count u32, reserved u32, then pfioc_table, then count +/// pfr_addr. Reply: status u32, errno i32, count u32, reserved u32, then +/// pfioc_table, then count pfr_addr. +const HEAD: usize = 16; + +const OK: u32 = 0; +const OPEN_FAILED: u32 = 1; +const IOCTL_FAILED: u32 = 2; +const REFUSED: u32 = 3; + +/// The child's end never times out: it blocks on the parent for its whole +/// life. The parent's end does, so a child that has died cannot wedge a worker. +const PARENT_TIMEOUT: Duration = Duration::from_secs(10); + +fn io_bytes(io: &PfiocTable) -> &[u8] { + // repr(C) and built zeroed, so every byte is initialised + unsafe { + std::slice::from_raw_parts( + io as *const PfiocTable as *const u8, + std::mem::size_of::(), + ) + } +} + +fn io_bytes_mut(io: &mut PfiocTable) -> &mut [u8] { + unsafe { + std::slice::from_raw_parts_mut( + io as *mut PfiocTable as *mut u8, + std::mem::size_of::(), + ) + } +} + +fn addr_bytes(buf: &[PfrAddr]) -> &[u8] { + unsafe { std::slice::from_raw_parts(buf.as_ptr() as *const u8, std::mem::size_of_val(buf)) } +} + +fn addr_bytes_mut(buf: &mut [PfrAddr]) -> &mut [u8] { + unsafe { + std::slice::from_raw_parts_mut(buf.as_mut_ptr() as *mut u8, std::mem::size_of_val(buf)) + } +} + +fn u32_at(b: &[u8], at: usize) -> u32 { + u32::from_le_bytes(b[at..at + 4].try_into().unwrap()) +} + +/// The parent's side: a PfDev whose calls run in the child. +pub struct PfProxy { + sock: UnixStream, +} + +/// Fork the child. Must run before any thread exists and before the sandbox: +/// the child inherits neither, and the parent's pledge will not permit fork. +pub fn spawn(devpf: &Path) -> io::Result { + let (parent, child) = UnixStream::pair()?; + match unsafe { libc::fork() } { + -1 => Err(io::Error::last_os_error()), + 0 => { + drop(parent); + child_main(devpf, child) + } + _ => { + drop(child); + parent.set_read_timeout(Some(PARENT_TIMEOUT))?; + parent.set_write_timeout(Some(PARENT_TIMEOUT))?; + Ok(PfProxy { sock: parent }) + } + } +} + +fn child_main(devpf: &Path, sock: UnixStream) -> ! { + // rc.d stops the parent; this process ends when the socket does + unsafe { + libc::signal(libc::SIGTERM, libc::SIG_IGN); + libc::signal(libc::SIGINT, libc::SIG_IGN); + libc::signal(libc::SIGHUP, libc::SIG_IGN); + } + let opened = std::fs::OpenOptions::new() + .read(true) + .write(true) + .open(devpf); + let locked = crate::platform::child_lockdown(); + let code = match (opened, locked) { + (Ok(file), Ok(())) => match serve(&mut OpenedPf(file), sock) { + Ok(()) => 0, + Err(_) => 1, + }, + // Every request is answered with the open failure, so the parent's + // probe reports it with its guidance + (Err(e), _) => match serve(&mut Unopened(e.raw_os_error()), sock) { + Ok(()) => 0, + Err(_) => 1, + }, + (_, Err(_)) => 2, + }; + std::process::exit(code) +} + +/// /dev/pf could not be opened; every call says so. +pub struct Unopened(pub Option); + +impl PfDev for Unopened { + fn call( + &mut self, + _cmd: u64, + _io: &mut PfiocTable, + _buf: &mut [PfrAddr], + ) -> Result<(), PfError> { + Err(PfError::Dev { + errno: os_error(self.0), + raw: self.0, + }) + } +} + +fn os_error(raw: Option) -> String { + match raw { + Some(code) => io::Error::from_raw_os_error(code).to_string(), + None => "unknown error".to_string(), + } +} + +/// The child's loop: one request, one ioctl, one reply, until the parent's end +/// closes. Generic over PfDev so it is testable without a fork or a kernel. +pub fn serve(dev: &mut impl PfDev, mut sock: UnixStream) -> io::Result<()> { + loop { + let mut head = [0u8; HEAD]; + match sock.read_exact(&mut head) { + Ok(()) => {} + Err(e) if e.kind() == io::ErrorKind::UnexpectedEof => return Ok(()), + Err(e) => return Err(e), + } + let cmd = u64::from_le_bytes(head[0..8].try_into().unwrap()); + let count = u32_at(&head, 8) as usize; + let mut io: PfiocTable = unsafe { std::mem::zeroed() }; + sock.read_exact(io_bytes_mut(&mut io))?; + if count > MAX_ADDRS { + // The parent enforces the ceiling too; a request past it is not + // the parent's, and nothing more is read from this socket + reply(&mut sock, REFUSED, 0, &io, &[])?; + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "request over MAX_ADDRS", + )); + } + let mut buf = vec![PfrAddr::zeroed(); count]; + sock.read_exact(addr_bytes_mut(&mut buf))?; + + let (status, errno) = match dev.call(cmd, &mut io, &mut buf) { + Ok(()) => (OK, 0), + Err(PfError::Dev { raw, .. }) => (OPEN_FAILED, raw.unwrap_or(0)), + Err(PfError::Ioctl { raw, .. }) => (IOCTL_FAILED, raw.unwrap_or(0)), + Err(_) => (REFUSED, 0), + }; + reply(&mut sock, status, errno, &io, &buf)?; + } +} + +fn reply( + sock: &mut UnixStream, + status: u32, + errno: i32, + io: &PfiocTable, + buf: &[PfrAddr], +) -> io::Result<()> { + let mut out = + Vec::with_capacity(HEAD + std::mem::size_of::() + addr_bytes(buf).len()); + out.extend_from_slice(&status.to_le_bytes()); + out.extend_from_slice(&errno.to_le_bytes()); + out.extend_from_slice(&(buf.len() as u32).to_le_bytes()); + out.extend_from_slice(&[0u8; 4]); + out.extend_from_slice(io_bytes(io)); + out.extend_from_slice(addr_bytes(buf)); + sock.write_all(&out) +} + +impl PfDev for PfProxy { + fn call(&mut self, cmd: u64, io: &mut PfiocTable, buf: &mut [PfrAddr]) -> Result<(), PfError> { + if buf.len() > MAX_ADDRS { + return Err(PfError::Child(format!( + "{} addresses exceeds the ceiling of {MAX_ADDRS}", + buf.len() + ))); + } + let gone = |e: io::Error| PfError::Child(format!("PF child unreachable: {e}")); + + let mut req = + Vec::with_capacity(HEAD + std::mem::size_of::() + addr_bytes(buf).len()); + req.extend_from_slice(&cmd.to_le_bytes()); + req.extend_from_slice(&(buf.len() as u32).to_le_bytes()); + req.extend_from_slice(&[0u8; 4]); + req.extend_from_slice(io_bytes(io)); + req.extend_from_slice(addr_bytes(buf)); + self.sock.write_all(&req).map_err(gone)?; + + let mut head = [0u8; HEAD]; + self.sock.read_exact(&mut head).map_err(gone)?; + let status = u32_at(&head, 0); + let errno = i32::from_le_bytes(head[4..8].try_into().unwrap()); + let count = u32_at(&head, 8) as usize; + if count != buf.len() { + return Err(PfError::Child(format!( + "reply carries {count} addresses for a request of {}", + buf.len() + ))); + } + // The child's buffer pointer means nothing here; the counters are read + let buffer = io.pfrio_buffer; + self.sock.read_exact(io_bytes_mut(io)).map_err(gone)?; + io.pfrio_buffer = buffer; + self.sock.read_exact(addr_bytes_mut(buf)).map_err(gone)?; + + match status { + OK => Ok(()), + OPEN_FAILED => Err(PfError::Dev { + errno: os_error(Some(errno)), + raw: Some(errno), + }), + IOCTL_FAILED => Err(PfError::Ioctl { + cmd, + errno: os_error(Some(errno)), + raw: Some(errno), + }), + _ => Err(PfError::Child("the child refused the request".to_string())), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::pf::ioctl::{table_add, table_del, table_get}; + use crate::pf::structs::{ip_of, pfr_addr_of, DIOCRADDADDRS, DIOCRDELADDRS, DIOCRGETADDRS}; + use std::net::IpAddr; + + /// A kernel-shaped fake on the child's side of the socket. + struct FakeKernel { + table: Vec, + fail_with: Option, + } + + impl PfDev for FakeKernel { + fn call( + &mut self, + cmd: u64, + io: &mut PfiocTable, + buf: &mut [PfrAddr], + ) -> Result<(), PfError> { + if let Some(raw) = self.fail_with { + return Err(PfError::Ioctl { + cmd, + errno: "fake".into(), + raw: Some(raw), + }); + } + match cmd { + DIOCRADDADDRS => { + let mut added = 0; + for a in buf.iter() { + let ip = ip_of(a).unwrap(); + if !self.table.contains(&ip) { + self.table.push(ip); + added += 1; + } + } + io.pfrio_nadd = added; + } + DIOCRDELADDRS => { + let mut deleted = 0; + for a in buf.iter() { + let ip = ip_of(a).unwrap(); + if let Some(pos) = self.table.iter().position(|t| *t == ip) { + self.table.remove(pos); + deleted += 1; + } + } + io.pfrio_ndel = deleted; + } + DIOCRGETADDRS => { + io.pfrio_size = self.table.len() as i32; + for (slot, ip) in buf.iter_mut().zip(&self.table) { + *slot = pfr_addr_of(ip); + } + } + _ => unreachable!(), + } + Ok(()) + } + } + + fn pair(kernel: FakeKernel) -> (PfProxy, std::thread::JoinHandle>) { + let (parent, child) = UnixStream::pair().unwrap(); + let handle = std::thread::spawn(move || { + let mut kernel = kernel; + serve(&mut kernel, child) + }); + (PfProxy { sock: parent }, handle) + } + + fn ips(list: &[&str]) -> Vec { + list.iter().map(|s| s.parse().unwrap()).collect() + } + + #[test] + fn calls_round_trip_through_the_child() { + let (mut proxy, child) = pair(FakeKernel { + table: Vec::new(), + fail_with: None, + }); + assert_eq!( + table_add(&mut proxy, "t", &ips(&["8.8.8.8", "2001:db8::1"])).unwrap(), + 2 + ); + assert_eq!( + table_get(&mut proxy, "t").unwrap(), + ips(&["8.8.8.8", "2001:db8::1"]) + ); + assert_eq!(table_del(&mut proxy, "t", &ips(&["8.8.8.8"])).unwrap(), 1); + assert_eq!(table_get(&mut proxy, "t").unwrap(), ips(&["2001:db8::1"])); + drop(proxy); + assert!( + child.join().unwrap().is_ok(), + "the child must end cleanly at EOF" + ); + } + + #[test] + fn an_ioctl_error_crosses_with_its_errno() { + let (mut proxy, child) = pair(FakeKernel { + table: Vec::new(), + fail_with: Some(libc::ESRCH), + }); + let err = table_add(&mut proxy, "t", &ips(&["8.8.8.8"])).unwrap_err(); + assert!( + matches!( + err, + PfError::Ioctl { + raw: Some(libc::ESRCH), + .. + } + ), + "{err:?}" + ); + // Composed on the parent's side from the errno alone + assert!(err.to_string().contains("not in the loaded ruleset")); + drop(proxy); + child.join().unwrap().unwrap(); + } + + #[test] + fn an_unopened_device_is_reported_on_every_call() { + let (parent, child_sock) = UnixStream::pair().unwrap(); + let child = + std::thread::spawn(move || serve(&mut Unopened(Some(libc::EACCES)), child_sock)); + let mut proxy = PfProxy { sock: parent }; + let err = table_get(&mut proxy, "t").unwrap_err(); + assert!( + matches!( + err, + PfError::Dev { + raw: Some(libc::EACCES), + .. + } + ), + "{err:?}" + ); + assert!(err.to_string().contains("mode 660"), "{err}"); + drop(proxy); + child.join().unwrap().unwrap(); + } + + #[test] + fn a_dead_child_is_an_error_not_a_hang() { + let (parent, child_sock) = UnixStream::pair().unwrap(); + drop(child_sock); + let mut proxy = PfProxy { sock: parent }; + let err = table_get(&mut proxy, "t").unwrap_err(); + assert!(matches!(err, PfError::Child(_)), "{err:?}"); + } + + #[test] + fn the_ceiling_is_enforced_on_both_sides() { + let (mut proxy, child) = pair(FakeKernel { + table: Vec::new(), + fail_with: None, + }); + let mut io: PfiocTable = unsafe { std::mem::zeroed() }; + let mut too_many = vec![PfrAddr::zeroed(); MAX_ADDRS + 1]; + assert!(matches!( + proxy.call(DIOCRADDADDRS, &mut io, &mut too_many), + Err(PfError::Child(_)) + )); + // Rejected before the send, so the child is still serving + assert_eq!(table_get(&mut proxy, "t").unwrap(), Vec::::new()); + drop(proxy); + child.join().unwrap().unwrap(); + } +} diff --git a/server-rust/src/pf/structs.rs b/server-rust/src/pf/structs.rs index 6ddb384..efe8452 100644 --- a/server-rust/src/pf/structs.rs +++ b/server-rust/src/pf/structs.rs @@ -104,14 +104,9 @@ pub fn pfr_addr_of(ip: &IpAddr) -> PfrAddr { } /// The address back out of a kernel-filled entry, canonicalised so table reads -/// agree with Redis keys. -/// -/// None for anything that is not a plain host entry. A table may also hold -/// networks and negations, from pf.conf or an operator's own pfctl, and those -/// were reported as the bare address they start with: the sync loop then saw an -/// address with no Redis key, called it orphaned, and issued a host delete every -/// scan that could not match the entry it came from. PFUI only ever adds -/// host-width, non-negated addresses, so nothing it manages is hidden by this. +/// agree with Redis keys. None for anything but a plain host entry: PFUI adds +/// host-width, non-negated addresses only, and the sync loop withdraws what it +/// cannot match. pub fn ip_of(a: &PfrAddr) -> Option { if a.pfra_not != 0 { return None; @@ -195,9 +190,7 @@ mod tests { #[test] fn a_network_or_negated_entry_is_not_read_as_a_host() { - // sync_pf_table deletes any table address with no Redis key. Reporting - // 10.0.0.0/8 as 10.0.0.0, or !192.0.2.1 as 192.0.2.1, made the daemon - // try to withdraw entries it never added and could not match. + // A network or negation read as a host draws a delete that never matches let mut network = pfr_addr_of(&"10.0.0.0".parse::().unwrap()); network.pfra_net = 8; assert_eq!(ip_of(&network), None, "a /8 was read as a host"); diff --git a/server-rust/src/platform.rs b/server-rust/src/platform.rs index 348da34..d064497 100644 --- a/server-rust/src/platform.rs +++ b/server-rust/src/platform.rs @@ -1,24 +1,19 @@ -//! OpenBSD hardening. unveil(2) is unconditional. Everything here is a no-op -//! on other platforms. +//! OpenBSD hardening: unveil(2), then pledge(2). The plan and the promise set +//! are data, tested on every platform; only the two system calls are OpenBSD +//! code. use std::path::{Path, PathBuf}; use crate::config::Config; -use crate::pf; +use crate::pf::{self, Ctl}; -/// The runtime linker, and where it finds the shared libraries pfctl needs. -pub const LD_SO: &str = "/usr/libexec/ld.so"; -pub const LIB_DIR: &str = "/usr/lib"; +/// Opened for the pfctl child's stdin by the spawn itself. +pub const DEV_NULL: &str = "/dev/null"; -/// Every path the running daemon may see, and with what rights. -/// -/// Built as a list rather than applied inline so it can be tested off OpenBSD, -/// where unveil(2) does not exist. The list is the security boundary and it was -/// incomplete without anywhere to notice; a missing entry is a runtime failure -/// on one code path, which is the hardest kind to find by hand. +/// Every path the running daemon may see, with its rights. Data rather than +/// calls, so the boundary is testable off OpenBSD. pub fn unveil_plan(cfg: &Config, config_path: &Path) -> Vec<(PathBuf, &'static str)> { - let mut plan: Vec<(PathBuf, &'static str)> = - vec![(config_path.to_path_buf(), "r"), (cfg.devpf.clone(), "rw")]; + let mut plan: Vec<(PathBuf, &'static str)> = vec![(config_path.to_path_buf(), "r")]; // The parent, not the file: this covers the persist file, its .lock sidecar // and the tempfiles a rewrite renames over @@ -36,17 +31,56 @@ pub fn unveil_plan(cfg: &Config, config_path: &Path) -> Vec<(PathBuf, &'static s plan.push((parent_or_root(sock), "rwc")); } - // pfctl, plus what exec'ing it requires. It is dynamically linked, so the - // loader and the libraries the loader maps have to be visible too; granting - // only the binary left the fallback failing at exec. CTL: IOCTL falls back - // to pfctl on any ioctl error, so this carries the default configuration - // exactly when the ioctl is what went wrong. - plan.push((PathBuf::from(pf::DEFAULT_PFCTL), "rx")); - plan.push((PathBuf::from(LD_SO), "rx")); - plan.push((PathBuf::from(LIB_DIR), "r")); + // The pfctl child inherits this view: it opens /dev/pf itself, and the + // spawn opens /dev/null for its stdin. /sbin/pfctl is statically linked. + // Under IOCTL the PF child opened /dev/pf before the lockdown. + if cfg.ctl == Ctl::Pfctl { + plan.push((cfg.devpf.clone(), "rw")); + plan.push((PathBuf::from(pf::DEFAULT_PFCTL), "rx")); + plan.push((PathBuf::from(DEV_NULL), "rw")); + } plan } +/// The pledge(2) promise set, or None when PLEDGE is off. +/// +/// inet is always needed: Redis connects lazily, after the pledge. dns never +/// is: REDIS_HOST is resolved before lockdown. fattr covers the persist +/// tempfile chmod, flock its lock. pf is never asked for: it does not permit +/// the table-address ioctls, which is why the PF child exists. +pub fn pledge_promises(cfg: &Config) -> Option { + if !cfg.pledge { + return None; + } + let mut promises = vec!["stdio", "rpath", "wpath", "cpath", "flock", "fattr", "inet"]; + if cfg.socket_unix.is_some() { + promises.push("unix"); + } + if cfg.ctl == Ctl::Pfctl { + promises.extend(["proc", "exec"]); + } + Some(promises.join(" ")) +} + +/// The PF child's lockdown: no filesystem at all. It holds /dev/pf already, +/// and cannot pledge, since every promise set forbids its ioctls. +#[cfg(target_os = "openbsd")] +pub fn child_lockdown() -> Result<(), String> { + unveil(Path::new("/var/empty"), "")?; + unveil_lock() +} + +#[cfg(not(target_os = "openbsd"))] +pub fn child_lockdown() -> Result<(), String> { + Ok(()) +} + +/// What lockdown applied, for the startup log. +pub struct Sandbox { + pub unveiled: usize, + pub promises: Option, +} + #[cfg(target_os = "openbsd")] fn unveil(path: &Path, permissions: &str) -> Result<(), String> { use std::os::unix::ffi::OsStrExt; @@ -74,21 +108,45 @@ fn unveil_lock() -> Result<(), String> { Ok(()) } -/// Restrict the filesystem to what the running daemon needs. Called after the -/// binds, which need getgrnam and the socket node, and after REDIS_HOST is -/// resolved, so no resolver files have to be unveiled. Children inherit the -/// view, which is what lets the pfctl fallback reach /dev/pf. +/// execpromises is NULL: a pfctl child runs unpledged. #[cfg(target_os = "openbsd")] -pub fn lockdown(cfg: &Config, config_path: &Path) -> Result<(), String> { - for (path, permissions) in unveil_plan(cfg, config_path) { - unveil(&path, permissions)?; +fn pledge(promises: &str) -> Result<(), String> { + let c_promises = std::ffi::CString::new(promises).expect("static promises"); + if unsafe { libc::pledge(c_promises.as_ptr(), std::ptr::null()) } == -1 { + return Err(format!( + "pledge({promises}) failed: {}", + std::io::Error::last_os_error() + )); } - unveil_lock() + Ok(()) +} + +/// Unveil the plan, lock it, pledge. After the binds (getgrnam, the socket +/// node) and REDIS_HOST resolution; before the PF probe. +#[cfg(target_os = "openbsd")] +pub fn lockdown(cfg: &Config, config_path: &Path) -> Result { + let plan = unveil_plan(cfg, config_path); + for (path, permissions) in &plan { + unveil(path, permissions)?; + } + unveil_lock()?; + let promises = pledge_promises(cfg); + if let Some(p) = &promises { + pledge(p)?; + } + Ok(Sandbox { + unveiled: plan.len(), + promises, + }) } +/// Nothing is applied off OpenBSD. #[cfg(not(target_os = "openbsd"))] -pub fn lockdown(_cfg: &Config, _config_path: &Path) -> Result<(), String> { - Ok(()) +pub fn lockdown(_cfg: &Config, _config_path: &Path) -> Result { + Ok(Sandbox { + unveiled: 0, + promises: None, + }) } #[cfg(test)] @@ -96,8 +154,8 @@ mod tests { use super::*; use crate::config::load_config_str; - fn plan_for(extra: &str) -> Vec<(PathBuf, &'static str)> { - let cfg = load_config_str(&format!( + fn config(extra: &str) -> Config { + load_config_str(&format!( " AF4_TABLE: t4 AF4_FILE: /var/db/pfui/ipv4_domains @@ -106,8 +164,11 @@ AF6_FILE: /var/db/pfui/ipv6_domains SOCKET_LISTEN: 10.10.1.254 {extra}" )) - .unwrap(); - unveil_plan(&cfg, Path::new("/etc/pfui_firewall.yml")) + .unwrap() + } + + fn plan_for(extra: &str) -> Vec<(PathBuf, &'static str)> { + unveil_plan(&config(extra), Path::new("/etc/pfui_firewall.yml")) } fn rights(plan: &[(PathBuf, &'static str)], path: &str) -> Option<&'static str> { @@ -117,29 +178,32 @@ SOCKET_LISTEN: 10.10.1.254 } #[test] - fn the_pfctl_fallback_can_actually_exec() { - // Granting the binary alone was not enough: pfctl is dynamically linked, - // so exec failed at the loader, and CTL: IOCTL falls back to pfctl on - // any ioctl error - let plan = plan_for(""); + fn pfctl_mode_can_actually_exec_pfctl() { + let plan = plan_for("CTL: PFCTL\n"); assert_eq!(rights(&plan, pf::DEFAULT_PFCTL), Some("rx")); + // The spawn opens /dev/null for the child's stdin before exec assert_eq!( - rights(&plan, LD_SO), - Some("rx"), - "the loader is not visible" - ); - assert_eq!( - rights(&plan, LIB_DIR), - Some("r"), - "the shared libraries are not visible" + rights(&plan, DEV_NULL), + Some("rw"), + "stdin cannot be opened" ); + // The pfctl child opens /dev/pf + assert_eq!(rights(&plan, "/dev/pf"), Some("rw")); + } + + #[test] + fn ioctl_mode_sees_neither_pfctl_nor_the_device() { + // Nothing to exec, and the PF child holds /dev/pf + let plan = plan_for("CTL: IOCTL\n"); + assert_eq!(rights(&plan, pf::DEFAULT_PFCTL), None); + assert_eq!(rights(&plan, DEV_NULL), None); + assert_eq!(rights(&plan, "/dev/pf"), None); } #[test] fn the_plan_covers_what_serving_needs() { let plan = plan_for("SOCKET_UNIX: /var/run/pfui/pfui_firewall.sock\n"); assert_eq!(rights(&plan, "/etc/pfui_firewall.yml"), Some("r")); - assert_eq!(rights(&plan, "/dev/pf"), Some("rw")); // Directories, so the .lock sidecar and rewrite tempfiles are covered assert_eq!(rights(&plan, "/var/db/pfui"), Some("rwc")); assert_eq!(rights(&plan, "/var/run/pfui"), Some("rwc")); @@ -147,7 +211,42 @@ SOCKET_LISTEN: 10.10.1.254 #[test] fn no_socket_directory_is_granted_when_there_is_no_socket() { - let plan = plan_for(""); - assert_eq!(rights(&plan, "/var/run/pfui"), None); + assert_eq!(rights(&plan_for(""), "/var/run/pfui"), None); + } + + #[test] + fn promises_follow_ctl() { + let ioctl = pledge_promises(&config("CTL: IOCTL\n")).unwrap(); + let pfctl = pledge_promises(&config("CTL: PFCTL\n")).unwrap(); + for base in ["stdio", "rpath", "wpath", "cpath", "flock", "fattr", "inet"] { + assert!(ioctl.split(' ').any(|p| p == base), "IOCTL lacks {base}"); + assert!(pfctl.split(' ').any(|p| p == base), "PFCTL lacks {base}"); + } + // pf is never pledged: it does not cover the address ioctls, and + // IOCTL never forks once the PF child exists + assert!(!ioctl.split(' ').any(|p| p == "pf"), "{ioctl}"); + assert!( + !ioctl.contains("proc") && !ioctl.contains("exec"), + "{ioctl}" + ); + // PFCTL forks and execs pfctl + assert!(pfctl.split(' ').any(|p| p == "proc")); + assert!(pfctl.split(' ').any(|p| p == "exec")); + assert!(!pfctl.split(' ').any(|p| p == "pf"), "{pfctl}"); + // dns is never pledged: REDIS_HOST is resolved before lockdown + assert!(!ioctl.contains("dns") && !pfctl.contains("dns")); + } + + #[test] + fn unix_is_pledged_only_when_a_local_socket_is_bound() { + let without = pledge_promises(&config("")).unwrap(); + let with = pledge_promises(&config("SOCKET_UNIX: /var/run/pfui/s.sock\n")).unwrap(); + assert!(!without.split(' ').any(|p| p == "unix"), "{without}"); + assert!(with.split(' ').any(|p| p == "unix"), "{with}"); + } + + #[test] + fn the_emergency_switch_disables_the_pledge() { + assert!(pledge_promises(&config("PLEDGE: False\n")).is_none()); } } diff --git a/server-rust/src/receiver.rs b/server-rust/src/receiver.rs index 3f1fbf1..6067285 100644 --- a/server-rust/src/receiver.rs +++ b/server-rust/src/receiver.rs @@ -70,14 +70,9 @@ pub trait Backends: Send + Sync { } pub trait Stream: Read + Write + Send { - /// Send FIN on the write half, leaving the read half open. - /// - /// An acknowledgement is not length-prefixed, so a client cannot know it has - /// the whole reply until the connection closes. Half-closing as soon as the - /// ACK is flushed is what releases the resolver at the table update rather - /// than at the end of the message, which is the point of the PF, ACKUPDATE, - /// Redis, persist order. Failure is ignored: a client that has already gone - /// is routine, and the addresses are installed either way. + /// Send FIN on the write half, leaving the read half open. A reply is + /// unframed, so the peer reads to EOF; half-closing at the ACK releases it + /// before Redis and the persist write. Failure is ignored. fn shutdown_write(&mut self); } @@ -313,10 +308,7 @@ fn handle_stream(_kind: StreamKind, conn: &mut dyn Stream, peer: &str, ctx: &Ctx } => { act(ctx, kind, &qname, &af4, &af6, || { reply(conn, ACK_UPDATE); - // Released here, before Redis and the persist file: PF already - // passes the traffic, so the resolver has nothing left to wait - // for. Holding the socket open until this function returned made - // it wait for both, since it reads the reply until EOF. + // Half-close now: the peer reads to EOF, and PF already passes conn.shutdown_write(); if let Some(t0) = started { ctx.log.info(&format!( @@ -380,10 +372,7 @@ fn handle_datagram(data: &[u8], source: SocketAddr, ctx: &Ctx) { mod tests { use super::*; - /// Both stream transports must release the reader, not just TCP: a reply is - /// unframed, so the peer's read ends only at EOF, and handle_stream is shared - /// by both. A no-op impl here would leave a resolver on the local socket - /// waiting for Redis and the persist write. + /// Both stream transports: handle_stream is shared and the peer reads to EOF. #[test] fn shutdown_write_ends_the_readers_reply() { use std::os::unix::net::UnixStream; diff --git a/server-rust/src/store.rs b/server-rust/src/store.rs index ae539a3..8b4a70e 100644 --- a/server-rust/src/store.rs +++ b/server-rust/src/store.rs @@ -69,28 +69,18 @@ pub trait Db { ) -> Result>>, DbError>; } -/// Longest an entry may authorise egress for, whatever the sender asked. -/// -/// A TTL arrives from a DNS answer over an unauthenticated transport, so it is -/// attacker-influenced in two ways: an authoritative server chooses it, and -/// anything permitted to reach the listener can send one. Unbounded, a single -/// large value pins its address in the PF table for the life of the daemon, -/// because both the Redis backstop and the scan loop derive their windows from -/// it. Seven days is far above any legitimate TTL times TTL_MULTIPLIER and far -/// below forever. +/// Ceiling on how long an entry may authorise egress. The TTL is sender-supplied +/// over an unauthenticated transport; without a ceiling one value pins an +/// address for the daemon's lifetime. pub const MAX_ENTRY_LIFETIME: i64 = 7 * 24 * 3600; -/// How long an rr record may live, bounded. Used by the Redis backstop and by -/// the scan loop, which have to agree or an entry lingers in whichever store -/// reads it as fresher. +/// Bounded rr lifetime; the Redis backstop and the scan loop must agree. fn rr_window(ttl: i64, multiplier: u32) -> i64 { ttl.saturating_mul(multiplier as i64) .clamp(0, MAX_ENTRY_LIFETIME) } -/// When a cache record's absolute expiry falls, bounded relative to when it was -/// recorded. A sender-supplied expiry far in the future is capped, and one -/// already in the past stays in the past. +/// Bounded cache expiry, relative to when the record was made. fn cache_deadline(expires: i64, epoch: i64) -> i64 { expires.min(epoch.saturating_add(MAX_ENTRY_LIFETIME)) } @@ -137,8 +127,7 @@ pub fn db_push( }); cmds.push(DbCmd::Expire { seconds: key_lifetime( - // Saturating: a sender-supplied expiry near i64::MIN - // overflowed this subtraction + // Saturating: expires is sender-supplied cache_deadline(*rr_ttl, now).saturating_sub(now).max(0), scan_period, ), @@ -693,8 +682,7 @@ mod tests { #[test] fn a_huge_ttl_cannot_pin_an_address_forever() { - // A TTL arrives from a DNS answer over an unauthenticated transport, so - // it decides how long egress is authorised and cannot be unbounded + // Sender-supplied, so it cannot be unbounded let meta = meta(&[ ("kind", "rr"), ("ttl", &i64::MAX.to_string()), @@ -726,8 +714,7 @@ mod tests { #[test] fn the_redis_backstop_is_bounded_too() { - // The backstop and the scan loop must agree, or an entry lingers in - // whichever store reads it as fresher + // Backstop and scan loop must agree let mut db = FakeDb::default(); push_one(&mut db, i64::MAX, Kind::Rr, 60, 4); let expires: Vec = db @@ -743,8 +730,7 @@ mod tests { #[test] fn a_negative_cache_expiry_does_not_overflow() { - // rr_ttl - now panicked in debug and wrapped to a near-eternal lifetime - // in release for an expiry near i64::MIN + // An expiry near i64::MIN must not overflow the subtraction let mut db = FakeDb::default(); push_one(&mut db, i64::MIN, Kind::Cache, 60, 4); for cmd in &db.cmds { diff --git a/server-rust/src/wire.rs b/server-rust/src/wire.rs index 39be91f..a432a6f 100644 --- a/server-rust/src/wire.rs +++ b/server-rust/src/wire.rs @@ -25,10 +25,8 @@ pub enum WireError { DecompressCorrupt(String), /// Payload is not valid JSON -> "Failed to decode" Json(String), - /// The payload's shape contradicts COMPRESS -> "Failed to decode". - /// Compression is configuration on both ends and is not signalled on the - /// wire, so a mismatch can only surface as a decode failure. The lz4 frame - /// magic says which end is wrong, which is worth saying outright. + /// The payload's shape contradicts COMPRESS: replied as "Failed to decode", + /// logged with which end is wrong. CompressMismatch { compress_configured: bool }, } diff --git a/server-rust/tests/framing_vectors.rs b/server-rust/tests/framing_vectors.rs index 5814b25..f91ecf8 100644 --- a/server-rust/tests/framing_vectors.rs +++ b/server-rust/tests/framing_vectors.rs @@ -1,5 +1,5 @@ //! Framing conformance against ../protocol/vectors/framing.tsv — the same -//! rows server-c and protocol/python run, so the implementations cannot +//! rows protocol/python runs, so the implementations cannot //! drift apart unnoticed. This layer is about bytes, not JSON: several ok //! payloads are deliberately not valid JSON. diff --git a/server-rust/tests/pf_ioctl_live.rs b/server-rust/tests/pf_ioctl_live.rs index 6fafb44..02dec5e 100644 --- a/server-rust/tests/pf_ioctl_live.rs +++ b/server-rust/tests/pf_ioctl_live.rs @@ -16,7 +16,6 @@ #![cfg(target_os = "openbsd")] use std::net::IpAddr; -use std::path::Path; use pfui_firewall::pf::ioctl::{table_add, table_del, table_get, DevPf}; @@ -30,8 +29,8 @@ fn table6() -> String { std::env::var("PFUI_TEST_TABLE6").unwrap_or_else(|_| "pfui_selftest_v6".into()) } -fn dev() -> DevPf<'static> { - DevPf(Path::new(DEVPF)) +fn dev() -> DevPf { + DevPf(std::path::PathBuf::from(DEVPF)) } fn ips(list: &[&str]) -> Vec { @@ -171,7 +170,11 @@ fn a_table_absent_from_the_ruleset_is_a_named_error() { .expect_err("a missing table must fail"); let text = err.to_string(); assert!( - text.contains("not in the active ruleset"), + text.contains("not in the loaded ruleset"), "the error should name the cause: {text}" ); + assert!( + text.contains("CTL: PFCTL"), + "the error should name the remedy: {text}" + ); } diff --git a/server-rust/tests/receiver_unix.rs b/server-rust/tests/receiver_unix.rs index 5d08f18..a75b888 100644 --- a/server-rust/tests/receiver_unix.rs +++ b/server-rust/tests/receiver_unix.rs @@ -252,16 +252,26 @@ fn a_message_over_the_local_socket_is_acknowledged() { conn.read_to_string(&mut reply).unwrap(); assert_eq!(reply, "Empty payload"); - // A peer that leaves before the ack (the EPIPE case) breaks nothing + // A peer that leaves before the ack (the EPIPE case) breaks nothing. It + // leaves once the table is updated, which precedes the ack: a peer gone + // before accept is a different case, and Darwin refuses setsockopt on it { let mut conn = UnixStream::connect(&sock).unwrap(); conn.write_all(&frame).unwrap(); + let deadline = std::time::Instant::now() + Duration::from_secs(5); + while recorder.tables.lock().unwrap().len() < 2 { + assert!(std::time::Instant::now() < deadline, "tables not updated"); + std::thread::sleep(Duration::from_millis(10)); + } } - let deadline = std::time::Instant::now() + Duration::from_secs(5); - while recorder.tables.lock().unwrap().len() < 2 { - assert!(std::time::Instant::now() < deadline, "tables not updated"); - std::thread::sleep(Duration::from_millis(10)); - } + // and the daemon is still serving + let mut conn = UnixStream::connect(&sock).unwrap(); + conn.set_read_timeout(Some(Duration::from_secs(5))).unwrap(); + conn.write_all(&frame).unwrap(); + conn.shutdown(std::net::Shutdown::Write).unwrap(); + let mut reply = String::new(); + conn.read_to_string(&mut reply).unwrap(); + assert_eq!(reply, "ACKUPDATE"); term.store(true, Ordering::Relaxed); accept.join().unwrap(); diff --git a/tests/openbsd/run.sh b/tests/openbsd/run.sh new file mode 100755 index 0000000..a046920 --- /dev/null +++ b/tests/openbsd/run.sh @@ -0,0 +1,479 @@ +#!/bin/sh +# +# End-to-end on a real OpenBSD host: both installers unattended, then the +# installed resolver against the installed daemon over each transport and +# control path, under unveil and pledge. Run by .github/workflows/openbsd.yml; +# runnable by hand as root. Rewrites /etc/pf.conf and both PFUI configs. +# +# PFUI_SRC_TREE 1 keep /usr/src, 2 signed release sources (default), 3 -current (unsigned) +# UNBOUND_VERSION latest (default), master, or a release-* tag + +set -eu + +ROOT=$(cd "$(dirname "$0")/../.." && pwd) +FW_YML=/etc/pfui_firewall.yml +RES_DIR=/var/unbound/etc +RES_YML=$RES_DIR/pfui_unbound.yml +RES_CONF=$RES_DIR/pfui_unbound.conf +SOCK=/var/run/pfui/pfui_firewall.sock +PERSIST4=/var/db/pfui/ipv4_domains +PERSIST6=/var/db/pfui/ipv6_domains +TABLE4=pfui_ipv4_domains +TABLE6=pfui_ipv6_domains +DAEMON_LOG=/var/log/daemon +BUILD_ROOT=/usr/local/pfui-build +NAME=one.one.one.one +# Globally routable, so validation accepts it; harmless on a pass-all test host +CRAFTED_IP=192.0.32.10 + +export PFUI_SRC_TREE="${PFUI_SRC_TREE:-2}" +export UNBOUND_VERSION="${UNBOUND_VERSION:-latest}" +export PFUI_UNBOUND_BUILD=2 + +MARK="" +PASSES=0 + +step() { printf '\n==> %s\n' "$*"; } +note() { printf ' %s\n' "$*"; } + +diagnostics() { + printf '\n--- diagnostics ---\n' + printf '\n# services\n' + rcctl check pfui_firewall || true + rcctl check pfui_unbound || true + printf '\n# daemon log\n' + grep pfui_firewall $DAEMON_LOG | tail -60 || true + printf '\n# resolver log\n' + grep unbound $DAEMON_LOG | tail -40 || true + printf '\n# kernel log (a pledge kill names the syscall here)\n' + dmesg | grep -iE 'pledge|unveil|pfui' | tail -10 || true + grep -hE 'pledge|unveil|pfui_firewall' /var/log/messages 2>/dev/null | tail -10 || true + printf '\n# the daemon in the foreground, as its own user, for five seconds\n' + su -s /bin/sh _pfui_firewall -c 'exec timeout 5 /usr/local/sbin/pfui_firewall -d' \ + > /tmp/pfui-foreground.log 2>&1 || true + tail -20 /tmp/pfui-foreground.log || true + printf '\n# PF tables\n' + pfctl -t $TABLE4 -T show 2>&1 || true + pfctl -t $TABLE6 -T show 2>&1 || true + printf '\n# Redis\n' + rcctl check redis || true + redis-cli ping || true + redis-cli dbsize || true + redis-cli keys '*' || true + tail -5 /var/redis/log 2>/dev/null || tail -5 /var/log/redis/redis.log 2>/dev/null || true + printf '\n# persist files\n' + cat $PERSIST4 2>/dev/null || true + printf '\n# configurations\n' + cat $FW_YML 2>/dev/null || true + cat $RES_YML 2>/dev/null || true +} + +fail() { + printf '\nFAIL: %s\n' "$*" >&2 + diagnostics + exit 1 +} + +# retry : a second apart, then fail with +retry() { + retry_n=$1 + retry_what=$2 + shift 2 + retry_i=0 + while [ "$retry_i" -lt "$retry_n" ]; do + if "$@" >/dev/null 2>&1; then + return 0 + fi + retry_i=$((retry_i + 1)) + sleep 1 + done + fail "$retry_what (after ${retry_n}s)" +} + +# Log lines since the pass began; daemon, resolver and marker share /var/log/daemon +mark() { + MARK="pfui-e2e $1 $(date +%s)" + logger -p daemon.notice -t pfui-e2e "$MARK" + sleep 1 +} +since_mark() { sed -n "/$MARK/,\$p" $DAEMON_LOG; } +logged() { since_mark | grep -q "$1"; } + +redis_up() { [ "$(redis-cli ping 2>/dev/null)" = PONG ]; } + +in_table() { pfctl -t "$1" -T show 2>/dev/null | tr -d ' ' | grep -qx "$2"; } +not_in_table() { ! in_table "$@"; } +in_redis() { [ "$(redis-cli exists "$1^$2")" = "1" ]; } +not_in_redis() { ! in_redis "$@"; } +in_persist() { grep -qx "$2" "$1"; } +not_in_persist() { ! in_persist "$@"; } +redis_kind_is() { [ "$(redis-cli hget "$1^$2" kind)" = "$3" ]; } + +resolve() { + dig +short +time=5 +tries=2 @127.0.0.1 "$1" A \ + | grep -E '^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+$' | head -1 +} + +# ------------------------------------------------------------- configuration + +# write_daemon_config [ttl_multiplier] [af4_table] +write_daemon_config() { + wd_transport=$1 + wd_ctl=$2 + wd_mult=${3:-4} + wd_table4=${4:-$TABLE4} + { + echo "LOGGING: True" + echo "LOG_LEVEL: DEBUG" + if [ "$wd_transport" != unix ]; then + echo "SOCKET_LISTEN: 127.0.0.1" + fi + if [ "$wd_transport" != tcp ]; then + echo "SOCKET_UNIX: $SOCK" + fi + echo "SOCKET_UNIX_GROUP: _pfui" + echo "SOCKET_PROTO: TCP" + echo "SOCKET_PORT: 10001" + echo "SOCKET_TIMEOUT: 3" + echo "COMPRESS: False" + echo "MAX_WORKERS: 8" + echo "REDIS_HOST: 127.0.0.1" + echo "REDIS_PORT: 6379" + echo "REDIS_DB: 0" + # Short: the sync loop must run several times a pass + echo "SCAN_PERIOD: 5" + echo "TTL_MULTIPLIER: $wd_mult" + echo "CTL: $wd_ctl" + echo "DEVPF: /dev/pf" + echo "AF4_TABLE: $wd_table4" + echo "AF4_FILE: $PERSIST4" + echo "AF6_TABLE: $TABLE6" + echo "AF6_FILE: $PERSIST6" + } > $FW_YML +} + +# write_resolver_config +write_resolver_config() { + { + echo "LOGGING: True" + echo "LOG_LEVEL: DEBUG" + echo "SOCKET_PROTO: TCP" + echo "SOCKET_TIMEOUT: 3" + echo "COMPRESS: False" + echo "BLOCKING: True" + echo "DEFAULT_PORT: 10001" + echo "FIREWALLS:" + if [ "$1" != unix ]; then + echo " - HOST: 127.0.0.1" + echo " PORT: 10001" + fi + if [ "$1" != tcp ]; then + echo " - SOCKET: $SOCK" + fi + } > $RES_YML +} + +# Loopback only, forwarding, python module first. Written once. +write_resolver_conf() { + cat > $RES_CONF < /etc/pf.conf < persist file "$PERSIST4" +table <$TABLE6> persist file "$PERSIST6" +pass +EOC + pfctl -e >/dev/null 2>&1 || true + pfctl -f /etc/pf.conf || fail "pf.conf did not load" + pfctl -t $TABLE4 -T show >/dev/null || fail "$TABLE4 is not in the ruleset" + pfctl -t $TABLE6 -T show >/dev/null || fail "$TABLE6 is not in the ruleset" +} + +# --------------------------------------------------------------- lifecycle + +stop_all() { + rcctl stop pfui_unbound >/dev/null 2>&1 || true + rcctl stop pfui_firewall >/dev/null 2>&1 || true +} + +clear_state() { + stop_all + # A dead Redis otherwise reads as an empty database, failing every + # store assertion without saying why + rcctl check redis >/dev/null 2>&1 || rcctl start redis >/dev/null 2>&1 || true + retry 15 "Redis is not answering" redis_up + redis-cli flushdb >/dev/null + pfctl -t $TABLE4 -T flush >/dev/null 2>&1 || true + pfctl -t $TABLE6 -T flush >/dev/null 2>&1 || true + : > $PERSIST4 + : > $PERSIST6 +} + +# start_daemon +start_daemon() { + rcctl start pfui_firewall || fail "pfui_firewall did not start" + retry 20 "pfui_firewall is not running" rcctl check pfui_firewall + retry 20 "the daemon did not log that it started" logged "PFUI_Firewall service started" + if [ "$1" != unix ]; then + retry 20 "TCP 10001 is not listening" nc -z 127.0.0.1 10001 + fi + if [ "$1" != tcp ]; then + retry 20 "$SOCK was not created" test -S $SOCK + fi +} + +start_resolver() { + rcctl start pfui_unbound || fail "pfui_unbound did not start" + retry 20 "pfui_unbound is not running" rcctl check pfui_unbound + # Non-recursive, so the listener is proven without a message to the firewall + retry 20 "the resolver is not answering" \ + dig +norecurse +time=1 +tries=1 @127.0.0.1 . NS +} + +# send_crafted : one rr message and its ACK +send_crafted() { + python3 - "$@" <<'EOP' +import socket +import sys + +sys.path.insert(0, "/var/unbound/etc") +from pfui_wire import encode # noqa: E402 + +target, qname, ip, ttl = sys.argv[1:5] +msg = {"kind": "rr", "qname": qname, "AF4": [{"ip": ip, "ttl": int(ttl)}], "AF6": []} +if target.startswith("/"): + s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + s.settimeout(5) + s.connect(target) +else: + host, port = target.rsplit(":", 1) + s = socket.create_connection((host, int(port)), timeout=5) +s.sendall(encode(msg, compress=False)) +s.shutdown(socket.SHUT_WR) +reply = b"" +while True: + chunk = s.recv(64) + if not chunk: + break + reply += chunk +if reply != b"ACKUPDATE": + sys.exit("the firewall replied %r" % reply) +EOP +} + +# ------------------------------------------------------------------ passes + +# run_pass +run_pass() { + pass_name=$1 + pass_transport=$2 + pass_ctl=$3 + step "pass $pass_name" + clear_state + write_daemon_config "$pass_transport" "$pass_ctl" + write_resolver_config "$pass_transport" + mark "$pass_name" + start_daemon "$pass_transport" + start_resolver + + ip=$(resolve $NAME) + [ -n "$ip" ] || fail "no A answer for $NAME" + note "$NAME -> $ip" + + # All three stores, labelled as a fresh answer + retry 10 "$ip is not in PF table $TABLE4" in_table $TABLE4 "$ip" + retry 10 "$ip is not in Redis" in_redis $TABLE4 "$ip" + [ "$(redis-cli hget "$TABLE4^$ip" qname)" = "$NAME." ] \ + || fail "Redis holds the wrong qname: $(redis-cli hgetall "$TABLE4^$ip")" + redis_kind_is $TABLE4 "$ip" rr || fail "the first answer was not recorded as kind rr" + retry 10 "$ip is not in $PERSIST4" in_persist $PERSIST4 "$ip" + + # The daemon names the peer it heard from + case $pass_transport in + tcp) logged "Received .* from 127.0.0.1:" || fail "nothing arrived over TCP" ;; + unix) logged "Received .* from $SOCK" || fail "nothing arrived over the unix socket" ;; + both) + logged "Received .* from 127.0.0.1:" || fail "nothing arrived over TCP (both)" + logged "Received .* from $SOCK" || fail "nothing arrived over the unix socket (both)" + ;; + esac + logged "PFUIDNS: Query Unblocked" || fail "the resolver did not log the unblock" + logged "PF Table updated for $NAME" || fail "the daemon did not log the table update" + + # A cache hit only resets the TTL and relabels the record + resolve $NAME >/dev/null + retry 10 "the Redis record did not flip to kind cache" redis_kind_is $TABLE4 "$ip" cache + + # The sync loop must have read the table by now + sleep 6 + logged "Scan for expiring $TABLE4" || fail "the sync loop did not run" + ! logged "Failed to read PF table" || fail "the sync loop could not read a table" + ! logged "Failed to install" || fail "a PF install failed" + ! logged "unreachable via CTL" || fail "the control path was reported unreachable" + + PASSES=$((PASSES + 1)) + note "ok" +} + +expiry_pass() { + step "pass expiry: a TTL 1 record leaves every store, and stop leaves no socket" + clear_state + write_daemon_config unix IOCTL 1 + mark expiry + start_daemon unix + send_crafted $SOCK expiry.pfui-e2e. $CRAFTED_IP 1 || fail "the crafted message was not acknowledged" + retry 10 "$CRAFTED_IP was not installed" in_table $TABLE4 $CRAFTED_IP + retry 10 "$CRAFTED_IP was not recorded" in_redis $TABLE4 $CRAFTED_IP + retry 10 "$CRAFTED_IP was not persisted" in_persist $PERSIST4 $CRAFTED_IP + # TTL 1 x multiplier 1, SCAN_PERIOD 5: gone within two scans + retry 30 "$CRAFTED_IP is still in Redis" not_in_redis $TABLE4 $CRAFTED_IP + retry 30 "$CRAFTED_IP is still in the PF table" not_in_table $TABLE4 $CRAFTED_IP + retry 30 "$CRAFTED_IP is still in $PERSIST4" not_in_persist $PERSIST4 $CRAFTED_IP + logged "TTL expired from $TABLE4: $CRAFTED_IP (expiry.pfui-e2e.)" \ + || fail "the expiry was not logged with its domain" + rcctl stop pfui_firewall >/dev/null 2>&1 || fail "pfui_firewall did not stop" + [ ! -e $SOCK ] || fail "$SOCK was left behind after stop" + note "ok" +} + +reload_pass() { + step "pass reload: pf.conf repopulates the table from the persist file" + clear_state + write_daemon_config tcp IOCTL + mark reload + start_daemon tcp + send_crafted 127.0.0.1:10001 reload.pfui-e2e. $CRAFTED_IP 3600 \ + || fail "the crafted message was not acknowledged" + retry 10 "$CRAFTED_IP was not installed" in_table $TABLE4 $CRAFTED_IP + retry 10 "$CRAFTED_IP was not persisted" in_persist $PERSIST4 $CRAFTED_IP + # Stopped first, so only pf.conf can put the address back + rcctl stop pfui_firewall >/dev/null 2>&1 || fail "pfui_firewall did not stop" + pfctl -t $TABLE4 -T flush >/dev/null 2>&1 + not_in_table $TABLE4 $CRAFTED_IP || fail "flush left $CRAFTED_IP in the table" + pfctl -f /etc/pf.conf || fail "pf.conf did not reload" + in_table $TABLE4 $CRAFTED_IP || fail "the reload did not restore $CRAFTED_IP from $PERSIST4" + note "ok" +} + +fail_loud_pass() { + step "pass fail-loud: a table missing from the ruleset refuses to start under IOCTL" + clear_state + write_daemon_config tcp IOCTL 4 pfui_not_in_ruleset + mark fail-loud + if rcctl start pfui_firewall >/dev/null 2>&1; then + sleep 2 + ! rcctl check pfui_firewall >/dev/null 2>&1 \ + || fail "the daemon is serving with a table it cannot reach" + fi + retry 10 "no refusal in the log" logged "pfui_not_in_ruleset is unreachable via CTL: IOCTL" + logged "not in the loaded ruleset" || fail "the ESRCH guidance is missing" + logged "CTL: PFCTL" || fail "the remedy is missing" + note "ok" +} + +# The unix-socket suite as root, and the live ioctl suite against the real +# tables. The installer's build directory is reused. +rust_tests() { + step "cargo test on the target" + cd "$ROOT/server-rust" + export CARGO_HOME=$BUILD_ROOT/cargo + export CARGO_TARGET_DIR=$BUILD_ROOT/target + # Streamed and bounded, so a hang is named rather than silent + { rc=0; timeout 900 cargo test --release --locked || rc=$?; echo "$rc" > /tmp/rc.cargo; } 2>&1 \ + | grep -vE '^\s+(Compiling|Downloaded|Downloading)' | tee /tmp/cargo-test.log + [ "$(cat /tmp/rc.cargo)" = 0 ] || fail "cargo test failed (exit $(cat /tmp/rc.cargo); 124 is the timeout)" + { rc=0; PFUI_TEST_TABLE4=$TABLE4 PFUI_TEST_TABLE6=$TABLE6 \ + timeout 300 cargo test --release --locked --test pf_ioctl_live -- --ignored --test-threads=1 \ + || rc=$?; echo "$rc" > /tmp/rc.live; } 2>&1 | tee /tmp/cargo-live.log + [ "$(cat /tmp/rc.live)" = 0 ] || fail "the live ioctl suite failed (exit $(cat /tmp/rc.live))" + pfctl -t $TABLE4 -T flush >/dev/null 2>&1 || true + pfctl -t $TABLE6 -T flush >/dev/null 2>&1 || true + cd "$ROOT" +} + +# Informational: qemu on a shared runner. Only ack-before-total is asserted. +latency_summary() { + step "latency (informational)" + grep -h "latency microsecs" $DAEMON_LOG | tail -8 || true + grep -h "Query Unblocked" $DAEMON_LOG | tail -4 || true + grep -h "latency microsecs" $DAEMON_LOG | awk ' + /to ack=/ { sub(/.*to ack=/, ""); ack = $1 + 0; have = 1; next } + /total=/ { if (have) { sub(/.*total=/, ""); if (ack > $1 + 0) bad++; n++; have = 0 } } + END { printf " pairs=%d out_of_order=%d\n", n, bad; exit (bad > 0) }' \ + || fail "an acknowledgement was logged after its total" +} + +# ---------------------------------------------------------------- the run + +step "host" +uname -a +[ "$(uname)" = OpenBSD ] || { echo "OpenBSD only"; exit 2; } +[ "$(id -u)" -eq 0 ] || { echo "run as root"; exit 2; } +for cmd in bash cargo redis-cli swig git curl python3 dig; do + command -v "$cmd" >/dev/null \ + || { echo "missing: $cmd (pkg_add bash rust redis swig git curl py3-lz4 py3-yaml)"; exit 2; } +done +df -h / /usr /usr/local /var 2>/dev/null | sort -u +# The base resolver must not hold port 53 +rcctl stop unbound >/dev/null 2>&1 || true +rcctl disable unbound >/dev/null 2>&1 || true + +step "install-server-rust.sh (builds with the packaged rustc)" +{ rc=0; bash "$ROOT/install-server-rust.sh" || rc=$?; echo "$rc" > /tmp/rc.server; } 2>&1 | tee /tmp/install-server.log +[ "$(cat /tmp/rc.server)" = 0 ] || fail "install-server-rust.sh failed" +[ -x /usr/local/sbin/pfui_firewall ] || fail "the daemon was not installed" + +setup_pf +rust_tests + +step "install-client-unbound.sh (PFUI_SRC_TREE=$PFUI_SRC_TREE UNBOUND_VERSION=$UNBOUND_VERSION)" +{ rc=0; bash "$ROOT/install-client-unbound.sh" || rc=$?; echo "$rc" > /tmp/rc.client; } 2>&1 | tee /tmp/install-client.log +[ "$(cat /tmp/rc.client)" = 0 ] || fail "install-client-unbound.sh failed" +/usr/local/sbin/unbound -V | grep -q pythonmodule || fail "the installed Unbound has no Python module" +groupinfo _pfui | grep -qw _unbound || fail "_unbound is not in group _pfui" + +write_resolver_conf +rcctl enable pfui_firewall pfui_unbound + +for ctl in IOCTL PFCTL; do + for transport in tcp unix both; do + run_pass "$transport-$ctl" "$transport" "$ctl" + done +done +expiry_pass +reload_pass +fail_loud_pass +latency_summary +stop_all + +step "complete: $PASSES transport/control passes, expiry, reload and fail-loud all passed"