From c6142a4188f4e63fcb9d7efbc576d15d0b7b17da Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 27 Sep 2026 16:18:36 +0000 Subject: [PATCH 01/10] Run the AUR package's check() in the AUR build job makepkg runs the PKGBUILD's check() (cargo test --frozen --lib) for every AUR install, and the build job skipped it with --nocheck on the claim that ci.yml already covered the tests. ci.yml runs the library tests, but not the way an AUR install does: frozen and offline against the dependencies prepare() fetched, with the Arch toolchain, inside the Arch container. A test that fails only there would first be seen by users installing the package. Drop --nocheck so the build job runs check() the same way. --- .github/workflows/aur-publish.yml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/.github/workflows/aur-publish.yml b/.github/workflows/aur-publish.yml index 022b5d62..af5c49a7 100644 --- a/.github/workflows/aur-publish.yml +++ b/.github/workflows/aur-publish.yml @@ -138,9 +138,11 @@ jobs: bash namcap-gate.sh PKGBUILD echo "::endgroup::" echo "::group::makepkg build" - # --nocheck: skip the PKGBUILD check() (cargo test --lib); the test - # suite is already covered by ci.yml. This job validates packaging. - makepkg -s --noconfirm --nocheck + # No --nocheck. makepkg runs check() by default, so every AUR user + # runs it and this job must too. ci.yml runs the tests, but not this + # way: frozen and offline against what prepare fetched, with the + # Arch toolchain, in this container. + makepkg -s --noconfirm echo "::endgroup::" echo "::group::namcap built package" bash namcap-gate.sh ./*.pkg.tar.* From d7f079618fac8300976ff20d8941c1fe623b1248 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 27 Sep 2026 16:24:33 +0000 Subject: [PATCH 02/10] Document that an empty datagram sent just before a close reads as the close on Linux The native API's receive rule cannot tell a zero-length datagram that is the last message before a close from the close itself on SOCK_SEQPACKET, which Linux uses: reading it drains the queue, and every observation then matches a bare end of file. macOS and FreeBSD carry the flow on SOCK_DGRAM, where the empty datagram is delivered and the close is reported by the read after it. The doc comment on Received::Datagram said an empty datagram is never a close, which contradicted the limitation stated a few hundred lines below it. It now says where the exception applies, and the recv_once rationale, the FipsStream::recv rustdoc, the native API reference and the client how-to scope the limitation to Linux. The reference page's list of places where data disappears gains the send-side consequence: a program that sends an empty datagram and then drops its stream may have the daemon read it as the close, so it never reaches the peer. The datagram how-to, which counts those places, is updated to match. A new test pins the behaviour per socket type, branching on the module's socket-type constant rather than on the OS, so the documentation and the test cannot drift apart. --- docs/how-to/use-the-native-datagram-api.md | 5 +- docs/how-to/write-a-native-api-client.md | 16 +++--- docs/reference/native-api.md | 26 ++++++--- src/native/client/mod.rs | 10 ++-- src/native/seqpacket.rs | 63 +++++++++++++++++++--- 5 files changed, 91 insertions(+), 29 deletions(-) diff --git a/docs/how-to/use-the-native-datagram-api.md b/docs/how-to/use-the-native-datagram-api.md index adc87334..355ba093 100644 --- a/docs/how-to/use-the-native-datagram-api.md +++ b/docs/how-to/use-the-native-datagram-api.md @@ -286,10 +286,11 @@ port from one dropped because a client was not reading fast enough. For the response shape, see [../reference/control-socket.md](../reference/control-socket.md#read-only-queries). -Reach for this when datagrams go missing. **Four places lose data with +Reach for this when datagrams go missing. **Five places lose data with nothing reported to your program**: a full per-flow queue, a listener that does not accept fast enough, an outbound datagram sent before a session -exists, and an outbound datagram after the transport MTU has fallen. +exists, an outbound datagram after the transport MTU has fallen, and, on +Linux, an empty datagram sent just before a close. [../reference/native-api.md](../reference/native-api.md#where-data-disappears) describes each and what bounds it. diff --git a/docs/how-to/write-a-native-api-client.md b/docs/how-to/write-a-native-api-client.md index bde73efa..a128874a 100644 --- a/docs/how-to/write-a-native-api-client.md +++ b/docs/how-to/write-a-native-api-client.md @@ -85,13 +85,15 @@ that costs a real payload if you get it wrong. A client that sends an empty datagram, then a message, then closes, leaves both queued, and a reader that trusts `POLLHUP` alone discards the message. -**One case has no answer, and you should design around it rather than solve -it.** A zero-length datagram that is the last message before a close is -indistinguishable from the close: reading it drains the queue, and `FIONREAD` -then reports zero because a zero-length message contributes no bytes. If your -protocol gives a zero-length payload a meaning, do not send it as a zero-length -socket message. Carry a one-byte discriminator, and keep the zero-byte read for -end of file alone. +**One case has no answer on Linux, where the pair is `SOCK_SEQPACKET`, and you +should design around it rather than solve it.** A zero-length datagram that is +the last message before a close is indistinguishable from the close: reading it +drains the queue, and `FIONREAD` then reports zero because a zero-length message +contributes no bytes. If your protocol gives a zero-length payload a meaning, do +not send it as a zero-length socket message. Carry a one-byte discriminator, and +keep the zero-byte read for end of file alone. On macOS and FreeBSD the pair is +`SOCK_DGRAM`: the empty datagram reads as zero bytes, and the close is reported +by the read after it. Both directions of the mistake are real. Reading an empty datagram as a close lets a peer tear down a live flow by sending nothing, and presents as a diff --git a/docs/reference/native-api.md b/docs/reference/native-api.md index 61fe1748..cea846d8 100644 --- a/docs/reference/native-api.md +++ b/docs/reference/native-api.md @@ -260,13 +260,15 @@ because it latches while messages are still queued: a read that trusted it would report the close early and discard whatever was waiting. The `EPIPE` that `recv` returns means the daemon went away, never that a peer finished. -**One case is reported as `EPIPE` although it is a datagram.** A zero-length -datagram that is the last message before a close is indistinguishable from the -close, because reading it drains the queue and `FIONREAD` then reports zero: a -zero-length message contributes no bytes. Do not give a zero-length payload a -meaning of its own on this API. Carry a one-byte discriminator instead. -Separating the two needs a payload that is never zero bytes on the wire, which -is a protocol change rather than a receive-path one. +**On Linux, one case is reported as `EPIPE` although it is a datagram.** A +zero-length datagram that is the last message before a close is +indistinguishable from the close, because reading it drains the queue and +`FIONREAD` then reports zero: a zero-length message contributes no bytes. macOS +and FreeBSD carry the flow on `SOCK_DGRAM`, where the datagram is delivered as +`Ok(0)` and the close follows it. Do not give a zero-length payload a meaning +of its own on this API. Carry a one-byte discriminator instead. Separating the +two needs a payload that is never zero bytes on the wire, which is a protocol +change rather than a receive-path one. **`peer_addr()`** and **`local_addr()`** return `FipsAddr`, not `io::Result`, unlike their `TcpStream` counterparts. These are field @@ -438,7 +440,7 @@ There is no acknowledgement, no retransmission, no ordering guarantee and no flow control between the two ends. A program that needs confirmation gets it from the peer, in the payload. -Four places lose data with nothing reported to the client. +Five places lose data with nothing reported to the client. **A full per-flow queue.** Inbound datagrams beyond `pending_per_flow` are dropped with a trace log and no client-visible signal. A program that stops @@ -467,6 +469,14 @@ snapshot taken at setup. The daemon re-checks each outbound datagram against the node's current limit and drops it silently if the transport MTU has since fallen. +**An empty datagram sent just before a close, on Linux.** A program that sends +a zero-length datagram and then drops its `FipsStream` may have the daemon read +that datagram as the close: the daemon frees the flow and the empty datagram +never reaches the peer. In the other direction, an empty datagram from the peer +that the daemon delivers immediately before its own half of the flow closes, +as when the daemon stops, reaches `recv` as `EPIPE` rather than `Ok(0)`. macOS +and FreeBSD are not affected. See `recv` above. + ### The drop causes, and what they mean An inbound datagram can be refused for eight reasons, which render as seven diff --git a/src/native/client/mod.rs b/src/native/client/mod.rs index 1fb85681..ee4c8ade 100644 --- a/src/native/client/mod.rs +++ b/src/native/client/mod.rs @@ -488,10 +488,12 @@ impl FipsStream { /// [`bytes_queued`](super::seqpacket::bytes_queued) reports nothing behind /// it. The daemon half applies the identical rule, on the same pair. /// - /// One case survives both checks: a zero-length datagram that is the last - /// message before the close is indistinguishable from the close, because - /// reading it drains the queue and a zero-length message contributes no - /// bytes. Do not give a zero-length payload a meaning of its own. + /// One case survives both checks on Linux, where the pair is + /// `SOCK_SEQPACKET`: a zero-length datagram that is the last message before + /// the close is indistinguishable from the close, because reading it drains + /// the queue and a zero-length message contributes no bytes, so it is + /// reported as `EPIPE`. On macOS and FreeBSD it is delivered as `Ok(0)` and + /// the close follows. Do not give a zero-length payload a meaning of its own. /// /// A datagram longer than `buf` is truncated and the remainder discarded, /// which is `SOCK_SEQPACKET` behaviour. Size `buf` at diff --git a/src/native/seqpacket.rs b/src/native/seqpacket.rs index 7e17e2cb..d2537a27 100644 --- a/src/native/seqpacket.rs +++ b/src/native/seqpacket.rs @@ -46,7 +46,11 @@ use tokio::io::unix::AsyncFd; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Received { /// A datagram of this many bytes. Zero is a legitimate value: a client may - /// send an empty datagram, and that is not the same as closing. + /// send an empty datagram, and while it keeps its half open that is not the + /// same as closing. On Linux, where the pair is `SOCK_SEQPACKET`, an empty + /// datagram that is the last thing a client sends before it closes reads as + /// [`Received::Eof`] instead; `recv_once` says why that one case cannot be + /// told apart. Datagram(usize), /// The peer closed its half of the pair. Eof, @@ -308,14 +312,18 @@ impl Seqpacket { /// message behind it is never taken. /// /// This matters because reading an empty datagram as a close would let a client -/// tear down its own flow by sending nothing, and the defect would present as a -/// spurious disconnect. +/// tear down its own flow by sending nothing while it is still connected, and the +/// defect would present as a spurious disconnect. /// -/// **One case survives and cannot be fixed here.** A zero-length datagram that -/// is the last message before a close is indistinguishable from the close: -/// reading it drains the queue, and every observation then matches a bare end of -/// file. Separating those needs a payload that is never zero bytes on the wire, -/// which is a protocol change rather than a receive-path one. +/// **One case survives on `SOCK_SEQPACKET`, which is Linux, and cannot be fixed +/// here.** A zero-length datagram that is the last message before a close is +/// indistinguishable from the close: reading it drains the queue, and every +/// observation then matches a bare end of file. The flow ends early and the empty +/// datagram is never delivered. Separating those needs a payload that is never +/// zero bytes on the wire, which is a protocol change rather than a receive-path +/// one. On `SOCK_DGRAM`, which macOS and FreeBSD use, the empty datagram is +/// delivered and the close is reported by the read after it; the tests below +/// pin both behaviours. /// /// **`ECONNRESET` is treated as end of file too**, for the platform whose /// datagram sockets report a close that way rather than through `POLLHUP`. Both @@ -671,6 +679,45 @@ mod tests { assert_eq!(recv_bounded(&daemon, &mut buf).await, Received::Eof); } + #[tokio::test] + async fn a_trailing_empty_datagram_before_a_close_reads_as_the_close_only_on_seqpacket() { + // The case recv_once documents as unfixable, pinned per socket type so + // the documentation cannot drift from what the kernel does. On + // SOCK_SEQPACKET the empty datagram is the last thing queued when the + // client closes, so reading it drains the queue with POLLHUP latched + // and it is indistinguishable from the close. On SOCK_DGRAM it is + // delivered and the close is reported by the read after it. The branch + // is on SOCK_TYPE rather than on the OS, because the socket type is the + // property the limitation depends on. + let (daemon, theirs) = pair().unwrap(); + let daemon = Seqpacket::new(daemon).unwrap(); + let theirs = client(theirs); + + // SAFETY: the descriptor is open and owned by `theirs`. + let sent = unsafe { libc::send(theirs.as_raw_fd(), std::ptr::null(), 0, 0) }; + assert_eq!(sent, 0, "{}", io::Error::last_os_error()); + drop(theirs); + + let mut buf = [0u8; 64]; + if SOCK_TYPE == libc::SOCK_SEQPACKET { + assert_eq!( + recv_bounded(&daemon, &mut buf).await, + Received::Eof, + "a trailing empty datagram on SOCK_SEQPACKET was delivered rather than \ + read as the close; the documented limitation no longer holds on this \ + kernel, so re-measure it and correct the docs that describe it" + ); + } else { + assert_eq!( + recv_bounded(&daemon, &mut buf).await, + Received::Datagram(0), + "a trailing empty datagram on SOCK_DGRAM was not delivered; the docs \ + say only SOCK_SEQPACKET platforms lose it" + ); + assert_eq!(recv_bounded(&daemon, &mut buf).await, Received::Eof); + } + } + #[test] fn both_halves_of_a_pair_are_close_on_exec() { // Asserted rather than assumed because the platforms disagree on how it From b4e9bfbc087c22d0e7dcbe53da3baef10995913c Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 27 Sep 2026 16:26:42 +0000 Subject: [PATCH 03/10] Stop writing the identity key file in ephemeral mode An ephemeral node wrote the private key of an identity it discards on every restart to fips.key, and overwrote any key already at that path, including an operator's key in the case where persistent: true was forgotten. It now writes only fips.pub, so the running npub stays visible, and holds the private key in memory only. A fips.key found at an ephemeral start is moved aside to fips.key.unused with a warning rather than used or overwritten, so an operator's key is recoverable and a stale key from an earlier release stops being read by fipsctl address. If that name is already taken or the rename fails, the file is left in place, a warning says so, and the start continues. Persistent and explicit-nsec identities are unchanged. The keygen note, the Windows service installer's closing note, and the documentation that described the old write are corrected, including the persistent identity how-to, which told operators to start once in ephemeral mode and then pin the key it wrote. --- docs/how-to/persistent-identity.md | 40 ++-- docs/reference/cli-fips.md | 12 +- docs/reference/configuration.md | 6 +- packaging/nixos/README.md | 4 +- packaging/systemd/README.install.md | 4 +- packaging/windows/build-zip.ps1 | 6 +- src/bin/fips.rs | 3 +- src/bin/fipsctl.rs | 8 +- src/config/mod.rs | 295 +++++++++++++++++++++++++--- 9 files changed, 312 insertions(+), 66 deletions(-) diff --git a/docs/how-to/persistent-identity.md b/docs/how-to/persistent-identity.md index 975ee503..015c6772 100644 --- a/docs/how-to/persistent-identity.md +++ b/docs/how-to/persistent-identity.md @@ -35,19 +35,12 @@ nodes, and tests where you actively want a fresh identity per run. The Debian/Ubuntu `.deb` and the Arch `fips` AUR package both ship a default `/etc/fips/fips.yaml` with `node.identity.persistent` left as -the upstream default (false), so the daemon writes a fresh keypair to -`/etc/fips/fips.{key,pub}` on every start until you set -`persistent: true`. To pin the current keypair: +the upstream default (false). In that mode the daemon generates a +fresh keypair on every start, holds the private key only in memory, +and writes only `/etc/fips/fips.pub`. To give the node a stable +identity: -1. Install the package and start the daemon once so it generates - `fips.key` / `fips.pub`: - - ```sh - sudo systemctl start fips - sudo systemctl status fips # confirm it came up - ``` - -2. Edit `/etc/fips/fips.yaml` and set: +1. Install the package, then edit `/etc/fips/fips.yaml` and set: ```yaml node: @@ -55,22 +48,31 @@ the upstream default (false), so the daemon writes a fresh keypair to persistent: true ``` -3. Restart the daemon and verify the identity is reused: +2. Start or restart the daemon. On this first persistent start it + generates a keypair and saves it to `/etc/fips/fips.key`: ```sh sudo systemctl restart fips + sudo systemctl status fips # confirm it came up + ``` + +3. Verify the identity: + + ```sh fipsctl show status | grep -E '"npub"|"node_addr"' cat /etc/fips/fips.pub ``` The npub printed by `fipsctl show status` should match - `/etc/fips/fips.pub` and remain stable across subsequent restarts. + `/etc/fips/fips.pub`. If the daemon was already running + ephemeral, the npub changes once at this restart and is stable + from then on. -The package's `postinst` script does **not** generate the keypair — -the daemon does, on first start. This means the keypair is only -present after the first successful daemon start. If the daemon never -came up cleanly (config error, permission problem), the key files -will be missing. +The package's `postinst` script does **not** generate the keypair. +The first successful daemon start with `persistent: true` does, so +`fips.key` is only present after that start. If the daemon never came +up cleanly (config error, permission problem), the key file will be +missing. ### macOS note diff --git a/docs/reference/cli-fips.md b/docs/reference/cli-fips.md index 653291a1..b147a0f7 100644 --- a/docs/reference/cli-fips.md +++ b/docs/reference/cli-fips.md @@ -76,16 +76,18 @@ first, then `/usr/local/etc/fips`, so the packaged file wins over a leftover `/etc/fips` copy from an earlier install. Windows likewise probes `\etc\fips` on the current drive, then `C:\ProgramData\fips`. -Adjacent to the highest-priority config file the daemon reads (or -writes, on first start) the identity files: +Adjacent to the highest-priority config file the daemon keeps the +identity files: | File | Mode | Purpose | | ---- | ---- | ------- | -| `fips.key` | `0600` | Bech32 nsec for the persistent identity (Unix; on Windows the file takes its directory's ACL, which `install-service.ps1` restricts to SYSTEM and Administrators). | -| `fips.pub` | `0644` | Bech32 npub corresponding to `fips.key`. | +| `fips.key` | `0600` | Bech32 nsec for the persistent identity, written only in persistent mode (Unix; on Windows the file takes its directory's ACL, which `install-service.ps1` restricts to SYSTEM and Administrators). | +| `fips.pub` | `0644` | Bech32 npub of the running identity, written on every start. In persistent mode it corresponds to `fips.key`. | When `node.identity.persistent` is `false` (the default), a fresh -keypair is written to these files on every start. +keypair is generated on every start and only `fips.pub` is written. +A `fips.key` found there is moved aside to `fips.key.unused` and a +warning is logged. On Windows the service writes its log to `C:\ProgramData\fips\fips.log`, rolled at 10 MiB with four old files kept; a foreground run logs to the diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index 1641fa5c..117f82fd 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -108,8 +108,10 @@ Identity resolution follows a three-tier priority: 3. **Ephemeral** — when `persistent: false` (default) and no `nsec`, generates a fresh keypair on each start -Key files (`fips.key` with mode 0600, `fips.pub` with mode 0644) are written adjacent -to the highest-priority config file for operator visibility, even in ephemeral mode. +`fips.pub` (mode 0644) is written adjacent to the highest-priority config file +on every start. `fips.key` (mode 0600) is written only in persistent mode. In +ephemeral mode a `fips.key` found at startup is moved aside to +`fips.key.unused` with a warning. ### General diff --git a/packaging/nixos/README.md b/packaging/nixos/README.md index 87f8cd2d..2f6f52b1 100644 --- a/packaging/nixos/README.md +++ b/packaging/nixos/README.md @@ -70,8 +70,8 @@ operator-editable runtime state: | Path | Purpose | Writable | Seeded from | |---|---|---|---| | `/var/lib/fips/fips.yaml` | Main config | yes | `services.fips.configFile` (first run only) | -| `/var/lib/fips/fips.key` | Node identity (private) | yes | generated by fips on first start | -| `/var/lib/fips/fips.pub` | Node identity (public) | yes | generated by fips on first start | +| `/var/lib/fips/fips.key` | Node identity (private) | yes | generated by fips on first start with `persistent: true` | +| `/var/lib/fips/fips.pub` | Node identity (public) | yes | written by fips on every start | | `/etc/fips/hosts` | Static hostname → npub map | yes | shipped `hosts` (first run only) | | `/etc/fips/peers.allow` | Peer allowlist (ACL) | yes | operator-created | | `/etc/fips/peers.deny` | Peer denylist (ACL) | yes | operator-created | diff --git a/packaging/systemd/README.install.md b/packaging/systemd/README.install.md index 2448dbf5..01ebfd0a 100644 --- a/packaging/systemd/README.install.md +++ b/packaging/systemd/README.install.md @@ -17,8 +17,8 @@ sudo ./install.sh | fipstop (TUI) | /usr/local/bin/fipstop | | fips-gateway (LAN bridge) | /usr/local/bin/fips-gateway | | Configuration | /etc/fips/fips.yaml | -| Identity key | /etc/fips/fips.key (auto-generated) | -| Public key | /etc/fips/fips.pub (auto-generated) | +| Identity key | /etc/fips/fips.key (generated on first start with `persistent: true`) | +| Public key | /etc/fips/fips.pub (written on every start) | | Hosts file | /etc/fips/hosts | | Firewall baseline | /etc/fips/fips.nft | | Firewall drop-in directory | /etc/fips/fips.d/ | diff --git a/packaging/windows/build-zip.ps1 b/packaging/windows/build-zip.ps1 index 532cca75..ae3731ae 100644 --- a/packaging/windows/build-zip.ps1 +++ b/packaging/windows/build-zip.ps1 @@ -101,9 +101,9 @@ Control Socket: Configuration: The service reads C:\ProgramData\fips\fips.yaml, where - install-service.ps1 puts it, and keeps fips.key, hosts, - peers.allow and peers.deny beside it. Edit fips.yaml there - before starting the service. + install-service.ps1 puts it, and keeps hosts, peers.allow, + peers.deny and, with node.identity.persistent: true, fips.key + beside it. Edit fips.yaml there before starting the service. install-service.ps1 restricts C:\ProgramData\fips to SYSTEM and Administrators before writing into it. Reading or editing diff --git a/src/bin/fips.rs b/src/bin/fips.rs index 8b9572bf..52bfaf00 100644 --- a/src/bin/fips.rs +++ b/src/bin/fips.rs @@ -484,7 +484,8 @@ mod service { "Configuration: the service reads {}", dir.join("fips.yaml").display() ); - println!(" keep fips.key, hosts, peers.allow and peers.deny beside it."); + println!(" keep hosts, peers.allow and peers.deny beside it, and fips.key"); + println!(" too when node.identity.persistent is true."); println!( "Logs: the service writes {}", dir.join("fips.log").display() diff --git a/src/bin/fipsctl.rs b/src/bin/fipsctl.rs index 5ee737db..3df15b8e 100644 --- a/src/bin/fipsctl.rs +++ b/src/bin/fipsctl.rs @@ -593,8 +593,12 @@ fn main() { eprintln!("{npub}"); eprintln!("Key files written to: {}/", dir.display()); eprintln!(); - eprintln!("NOTE: Set 'node.identity.persistent: true' in fips.yaml"); - eprintln!(" or these keys will be overwritten on next daemon start."); + eprintln!( + "NOTE: Set 'node.identity.persistent: true' in fips.yaml before the next daemon start." + ); + eprintln!( + " Without it the daemon does not use this key: it moves fips.key aside to fips.key.unused." + ); return; } diff --git a/src/config/mod.rs b/src/config/mod.rs index 72e95a8c..f323214b 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -595,13 +595,60 @@ pub fn write_pub_file(path: &Path, npub: &str) -> Result<(), ConfigError> { Ok(()) } +/// Move a key file found at an ephemeral start to `.unused` beside it, +/// so it is neither used nor overwritten and stays recoverable. +/// +/// Returns the new path when the file was moved. Does nothing when no file is +/// at `key_path`; a dangling symlink counts as a file and is moved as a link. +/// An existing file at the aside path is never replaced: the key is left in +/// place and a warning says so, as it does when the rename fails. Neither case +/// stops the start. +fn retire_key(key_path: &Path) -> Option { + // symlink_metadata rather than exists: a dangling symlink at the key path + // reports exists() == false but is still a file the operator put there. + key_path.symlink_metadata().ok()?; + + let mut name = key_path.file_name()?.to_os_string(); + name.push(".unused"); + let aside = key_path.with_file_name(name); + + let failure = match aside.symlink_metadata() { + Ok(_) => "a file already exists at the aside path".to_string(), + Err(e) if e.kind() != std::io::ErrorKind::NotFound => e.to_string(), + Err(_) => match std::fs::rename(key_path, &aside) { + Ok(()) => { + tracing::warn!( + path = %key_path.display(), + moved_to = %aside.display(), + config_key = "node.identity.persistent", + "An identity key file was found in ephemeral mode and moved aside, not used; \ + set node.identity.persistent: true and move it back to use it" + ); + return Some(aside); + } + Err(e) => e.to_string(), + }, + }; + + tracing::warn!( + path = %key_path.display(), + aside = %aside.display(), + error = %failure, + config_key = "node.identity.persistent", + "An identity key file was found in ephemeral mode and could not be moved aside; \ + it is not used; set node.identity.persistent: true to use it, or remove it" + ); + None +} + /// Resolve identity from config and key file. /// /// Behavior depends on `node.identity.persistent`: /// /// - **`persistent: false`** (default): generate a fresh ephemeral keypair -/// every start. Key files are written for operator visibility but overwritten -/// on each restart. +/// every start. Only `fips.pub` is written, so the running npub is visible; +/// the private key is never written. A `fips.key` already at the path is +/// moved aside to `fips.key.unused` with a warning, not used or overwritten. /// /// - **`persistent: true`**: use three-tier resolution: /// 1. Explicit nsec in config — highest priority @@ -737,8 +784,8 @@ pub fn resolve_identity( } } } else { - // Ephemeral mode (default): fresh keypair every start, write key files - // for operator visibility + // Ephemeral mode (default): a fresh keypair every start, held only in + // memory. Only the public key file is written. let identity = Identity::generate(); // `keypair()` and `secret_key()` each hand back a whole private key // rather than a handle, so both temporaries are bound and erased. @@ -753,25 +800,8 @@ pub fn resolve_identity( let _ = std::fs::create_dir_all(parent); } - // symlink_metadata rather than exists: a dangling symlink at the key - // path reports exists() == false but is still an existing file the - // write is about to act on. - if key_path.symlink_metadata().is_ok() { - tracing::warn!( - path = %key_path.display(), - config_key = "node.identity.persistent", - "An existing key file at this path is being replaced by a fresh ephemeral \ - identity; set node.identity.persistent: true to keep the existing identity" - ); - } + retire_key(&key_path); - if let Err(e) = write_key_file(&key_path, &nsec) { - tracing::warn!( - path = %key_path.display(), - error = %e, - "Failed to write the ephemeral key file" - ); - } if let Err(e) = write_pub_file(&pub_path, &npub) { tracing::warn!( path = %pub_path.display(), @@ -2020,29 +2050,64 @@ node: assert_eq!(fs::read_to_string(&victim).unwrap(), "victim contents\n"); } + /// The names in `dir`, sorted, so a test can assert on the whole directory. + fn dir_names(dir: &Path) -> Vec { + let mut names: Vec = fs::read_dir(dir) + .unwrap() + .map(|e| e.unwrap().file_name().to_string_lossy().into_owned()) + .collect(); + names.sort(); + names + } + + /// The npub a resolved identity runs as. + fn resolved_npub(resolved: &ResolvedIdentity) -> String { + crate::Identity::from_secret_str(&resolved.nsec) + .unwrap() + .npub() + } + #[test] - fn test_ephemeral_over_existing_key_warns() { + fn ephemeral_start_moves_an_existing_key_aside_intact() { let temp_dir = TempDir::new().unwrap(); let config_path = temp_dir.path().join("fips.yaml"); let key_path = temp_dir.path().join("fips.key"); + let aside_path = temp_dir.path().join("fips.key.unused"); fs::write(&config_path, "node:\n identity: {}\n").unwrap(); let identity = crate::Identity::generate(); let existing = crate::encode_nsec(&identity.keypair().secret_key()); write_key_file(&key_path, &existing).unwrap(); + let planted = fs::read(&key_path).unwrap(); let config = Config::load_file(&config_path).unwrap(); let (resolved, logs) = capture_logs(|| resolve_identity(&config, std::slice::from_ref(&config_path)).unwrap()); assert_ne!(resolved.nsec, existing); + assert!( + key_path.symlink_metadata().is_err(), + "the key file must be moved away from the path a persistent start reads" + ); + assert_eq!( + fs::read(&aside_path).unwrap(), + planted, + "the key set aside must hold the planted bytes exactly" + ); + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + assert_eq!(fs::metadata(&aside_path).unwrap().mode() & 0o777, 0o600); + } let warnings = logs.warnings(); assert!( warnings .iter() .any(|w| w.contains(&key_path.display().to_string()) - && w.contains("node.identity.persistent")), - "expected a warning naming the key path and the config key, got {warnings:?}" + && w.contains(&aside_path.display().to_string()) + && w.contains("node.identity.persistent") + && w.contains("and moved aside, not used")), + "expected the moved-aside warning naming both paths and the config key, got {warnings:?}" ); } @@ -2052,6 +2117,7 @@ node: let temp_dir = TempDir::new().unwrap(); let config_path = temp_dir.path().join("fips.yaml"); let key_path = temp_dir.path().join("fips.key"); + let aside_path = temp_dir.path().join("fips.key.unused"); let target = temp_dir.path().join("absent-target"); fs::write(&config_path, "node:\n identity: {}\n").unwrap(); @@ -2065,8 +2131,21 @@ node: assert!( warnings .iter() - .any(|w| w.contains(&key_path.display().to_string())), - "expected a warning naming the key path, got {warnings:?}" + .any(|w| w.contains(&key_path.display().to_string()) + && w.contains("and moved aside, not used")), + "expected the moved-aside warning naming the key path, got {warnings:?}" + ); + assert!( + key_path.symlink_metadata().is_err(), + "the dangling symlink must be moved away from the key path" + ); + assert!( + aside_path + .symlink_metadata() + .unwrap() + .file_type() + .is_symlink(), + "the symlink itself must be what was moved aside, not a file written through it" ); assert!( !target.exists(), @@ -2074,6 +2153,155 @@ node: ); } + #[test] + fn ephemeral_start_leaves_a_key_in_place_when_the_aside_name_is_taken() { + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("fips.yaml"); + let key_path = temp_dir.path().join("fips.key"); + let aside_path = temp_dir.path().join("fips.key.unused"); + + fs::write(&config_path, "node:\n identity: {}\n").unwrap(); + let current = crate::encode_nsec(&crate::Identity::generate().keypair().secret_key()); + let earlier = crate::encode_nsec(&crate::Identity::generate().keypair().secret_key()); + write_key_file(&key_path, ¤t).unwrap(); + write_key_file(&aside_path, &earlier).unwrap(); + let key_bytes = fs::read(&key_path).unwrap(); + let aside_bytes = fs::read(&aside_path).unwrap(); + + let config = Config::load_file(&config_path).unwrap(); + let (resolved, logs) = + capture_logs(|| resolve_identity(&config, std::slice::from_ref(&config_path)).unwrap()); + + assert!(matches!(resolved.source, IdentitySource::Ephemeral)); + assert_ne!(resolved.nsec, current); + assert_eq!( + fs::read(&key_path).unwrap(), + key_bytes, + "the key must be left in place, not overwritten" + ); + assert_eq!( + fs::read(&aside_path).unwrap(), + aside_bytes, + "the file already at the aside path must not be replaced" + ); + let warnings = logs.warnings(); + assert!( + warnings + .iter() + .any(|w| w.contains(&key_path.display().to_string()) + && w.contains(&aside_path.display().to_string()) + && w.contains("node.identity.persistent") + && w.contains("could not be moved aside")), + "expected the could-not-move warning naming both paths and the config key, got {warnings:?}" + ); + } + + #[cfg(unix)] + #[test] + fn ephemeral_start_proceeds_when_the_key_cannot_be_moved() { + use std::os::unix::fs::PermissionsExt; + + /// Puts the directory's mode back when dropped, so a failing + /// assertion does not leave a read-only directory behind that + /// `TempDir` cannot remove. + struct RestoreMode<'a>(&'a Path); + impl Drop for RestoreMode<'_> { + fn drop(&mut self) { + let _ = fs::set_permissions(self.0, fs::Permissions::from_mode(0o755)); + } + } + + // Coverage gap: root bypasses directory permissions, so the rename + // succeeds and this branch goes unexercised when the suite runs as + // root. The aside-name-taken test still covers the same warning. + if unsafe { libc::geteuid() } == 0 { + eprintln!("skipped: running as root, which a read-only directory does not stop"); + return; + } + + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("fips.yaml"); + let key_path = temp_dir.path().join("fips.key"); + + fs::write(&config_path, "node:\n identity: {}\n").unwrap(); + let existing = crate::encode_nsec(&crate::Identity::generate().keypair().secret_key()); + write_key_file(&key_path, &existing).unwrap(); + let planted = fs::read(&key_path).unwrap(); + let config = Config::load_file(&config_path).unwrap(); + + fs::set_permissions(temp_dir.path(), fs::Permissions::from_mode(0o555)).unwrap(); + let _restore = RestoreMode(temp_dir.path()); + + let (resolved, logs) = + capture_logs(|| resolve_identity(&config, std::slice::from_ref(&config_path)).unwrap()); + + assert!(matches!(resolved.source, IdentitySource::Ephemeral)); + assert_ne!(resolved.nsec, existing); + assert_eq!( + fs::read(&key_path).unwrap(), + planted, + "a key that cannot be moved must be left as it was, not overwritten" + ); + let warnings = logs.warnings(); + assert!( + warnings + .iter() + .any(|w| w.contains(&key_path.display().to_string()) + && w.contains("could not be moved aside") + && w.contains("os error")), + "expected a warning naming the key path and the rename error, got {warnings:?}" + ); + } + + #[test] + fn ephemeral_restarts_never_leave_a_private_key_file() { + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("fips.yaml"); + let pub_path = temp_dir.path().join("fips.pub"); + + fs::write(&config_path, "node:\n identity: {}\n").unwrap(); + let config = Config::load_file(&config_path).unwrap(); + + for start in 1..=3 { + let resolved = resolve_identity(&config, std::slice::from_ref(&config_path)).unwrap(); + assert_eq!( + dir_names(temp_dir.path()), + ["fips.pub", "fips.yaml"], + "after start {start} the directory must hold only the config and the public key" + ); + assert_eq!( + fs::read_to_string(&pub_path).unwrap().trim(), + resolved_npub(&resolved), + "after start {start} fips.pub must name the running identity" + ); + } + } + + #[test] + fn persistent_start_ignores_a_key_set_aside() { + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("fips.yaml"); + let key_path = temp_dir.path().join("fips.key"); + let aside_path = temp_dir.path().join("fips.key.unused"); + + fs::write(&config_path, "node:\n identity:\n persistent: true\n").unwrap(); + let earlier = crate::encode_nsec(&crate::Identity::generate().keypair().secret_key()); + write_key_file(&aside_path, &earlier).unwrap(); + let aside_bytes = fs::read(&aside_path).unwrap(); + + let config = Config::load_file(&config_path).unwrap(); + let resolved = resolve_identity(&config, std::slice::from_ref(&config_path)).unwrap(); + + assert!(matches!(resolved.source, IdentitySource::Generated(_))); + assert_ne!(resolved.nsec, earlier); + assert_eq!(read_key_file(&key_path).unwrap(), resolved.nsec); + assert_eq!( + fs::read(&aside_path).unwrap(), + aside_bytes, + "a persistent start must leave the key set aside untouched" + ); + } + #[cfg(unix)] #[test] fn test_persistent_permissive_key_warns() { @@ -2205,11 +2433,18 @@ node: let resolved = resolve_identity(&config, std::slice::from_ref(&config_path)).unwrap(); assert!(matches!(resolved.source, IdentitySource::Ephemeral)); - // Key files should still be written for operator visibility + // Only the public key is written: an ephemeral private key lives in + // memory and nowhere else. let key_path = temp_dir.path().join("fips.key"); let pub_path = temp_dir.path().join("fips.pub"); - assert!(key_path.exists()); - assert!(pub_path.exists()); + assert_eq!( + key_path.symlink_metadata().unwrap_err().kind(), + std::io::ErrorKind::NotFound + ); + assert_eq!( + fs::read_to_string(&pub_path).unwrap().trim(), + resolved_npub(&resolved) + ); } #[test] From 6b17d6ee0083cad223b0b26145447f06663d4a5a Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 27 Sep 2026 17:08:52 +0000 Subject: [PATCH 04/10] Create empty peer ACL files in the Windows installer and stop on legacy ones The service still reads peers.allow and peers.deny from \etc\fips on the system drive when they are missing from C:\ProgramData\fips, and any local user can create files there, so a planted list was enforced. install-service.ps1 now creates both files in C:\ProgramData\fips, empty, which allows every peer, so the service no longer falls back to the old location. It stops when either file exists under \etc\fips and is missing from C:\ProgramData\fips, which is exactly when the service would enforce the old file, so an upgrader's list is neither enforced nor dropped without an administrator reviewing it. The check runs after the directory is restricted and before the binaries are copied or the service is registered, and every refusal is decided before any file is created; an existing file is never truncated. The zip README says the installer creates the files, that a list is cleared by emptying its file rather than deleting it, and what to do when the installer stops on an old file. A structural test pins the conditions and their order in the script. --- packaging/windows/build-zip.ps1 | 11 ++ packaging/windows/install-service.ps1 | 25 +++ src/packaging_tests.rs | 224 ++++++++++++++++++++++++-- 3 files changed, 244 insertions(+), 16 deletions(-) diff --git a/packaging/windows/build-zip.ps1 b/packaging/windows/build-zip.ps1 index ae3731ae..a098fff4 100644 --- a/packaging/windows/build-zip.ps1 +++ b/packaging/windows/build-zip.ps1 @@ -105,6 +105,17 @@ Configuration: peers.deny and, with node.identity.persistent: true, fips.key beside it. Edit fips.yaml there before starting the service. + install-service.ps1 creates empty peers.allow and peers.deny + there, which allow every peer until you add entries. To clear + a list, empty the file; do not delete it. While either file + is missing from C:\ProgramData\fips, the service still reads + that file from \etc\fips on the system drive, where earlier + releases kept it and any local user can create it. The + installer stops if it finds a file there with none in + C:\ProgramData\fips: review that file, move it into + C:\ProgramData\fips or delete it, and run install-service.ps1 + again. + install-service.ps1 restricts C:\ProgramData\fips to SYSTEM and Administrators before writing into it. Reading or editing files there, fipsctl keygen, fipsctl address with no argument, diff --git a/packaging/windows/install-service.ps1 b/packaging/windows/install-service.ps1 index 8b4bf214..ee293629 100644 --- a/packaging/windows/install-service.ps1 +++ b/packaging/windows/install-service.ps1 @@ -130,6 +130,31 @@ foreach ($item in @(Get-ChildItem -LiteralPath $ConfigDir -Force)) { Write-Host " Restricted $ConfigDir to SYSTEM and Administrators" +# Releases before this one read peers.allow and peers.deny from \etc\fips on +# the system drive, where any local user can create files, and the service +# still reads a file there when it is missing from the config directory. +# Stop rather than enforce, or silently drop, a list nobody has reviewed. +$legacyAclDir = "$env:SystemDrive\etc\fips" +foreach ($name in @("peers.allow", "peers.deny")) { + $legacy = Join-Path $legacyAclDir $name + $current = "$ConfigDir\$name" + if ((Test-Path -LiteralPath $legacy) -and -not (Test-Path -LiteralPath $current)) { + Write-Error "$legacy exists and $current does not, so the service would enforce the old file. Earlier releases read it, and any local user can write there. Review it, then move it to $current or delete it, and run install-service.ps1 again." + exit 1 + } +} + +# Empty peer ACL files allow every peer. Having them here means the service +# never falls back to the \etc\fips copies. Empty them to clear a list; do not +# delete them. +foreach ($name in @("peers.allow", "peers.deny")) { + $aclFile = "$ConfigDir\$name" + if (-not (Test-Path -LiteralPath $aclFile)) { + New-Item -ItemType File -Path $aclFile | Out-Null + Write-Host " Created empty $aclFile (allows every peer until you add entries)" + } +} + # Copy binaries $Binaries = @("fips.exe", "fipsctl.exe", "fipstop.exe") foreach ($bin in $Binaries) { diff --git a/src/packaging_tests.rs b/src/packaging_tests.rs index 7812c7d1..a361af42 100644 --- a/src/packaging_tests.rs +++ b/src/packaging_tests.rs @@ -475,6 +475,54 @@ fn ps_lines(ps1: &str) -> Vec { .collect() } +/// Asserts that line `i` of install-service.ps1's code lines is an `if` whose +/// body is `Write-Error` then `exit 1`, so the condition it tests stops the +/// install rather than only reporting it. +fn refuses_at(lines: &[String], i: usize, what: &str) { + let cond = &lines[i]; + assert!( + cond.starts_with("if (") && cond.ends_with('{'), + "install-service.ps1: the {what} is not an if statement: {cond}" + ); + let body = lines.get(i + 1..i + 3).unwrap_or_default(); + assert!( + body.len() == 2 && body[0].starts_with("Write-Error ") && body[1] == "exit 1", + "install-service.ps1: the {what} is not followed by Write-Error then exit 1: \ + {cond}\n then: {body:?}" + ); +} + +/// Returns the index of the line that closes the block opened on line +/// `start`, found by brace depth. Braces inside single- or double-quoted +/// strings are not counted. +fn block_end(lines: &[String], start: usize) -> usize { + let mut depth = 0i64; + for (i, line) in lines.iter().enumerate().skip(start) { + let mut quote = None; + for c in line.chars() { + match (quote, c) { + (None, '"' | '\'') => quote = Some(c), + (Some(q), _) if c == q => quote = None, + (None, '{') => depth += 1, + (None, '}') => depth -= 1, + _ => {} + } + } + if depth <= 0 { + assert!( + i > start, + "install-service.ps1: no block opens at code line {start}: {}", + lines[start] + ); + return i; + } + } + panic!( + "install-service.ps1: the block at code line {start} never closes: {}", + lines[start] + ) +} + /// Guards the order in which install-service.ps1 secures `C:\ProgramData\fips`. /// /// The directory inherits `C:\ProgramData`'s access, under which any local @@ -664,19 +712,6 @@ fn windows_installer_restricts_config_dir_before_any_path_inside_it() { #[test] fn windows_installer_refusal_conditions_stop_the_install() { let lines = ps_lines(&repo_file("packaging/windows/install-service.ps1")); - let refuses_at = |i: usize, what: &str| { - let cond = &lines[i]; - assert!( - cond.starts_with("if (") && cond.ends_with('{'), - "install-service.ps1: the {what} is not an if statement: {cond}" - ); - let body = lines.get(i + 1..i + 3).unwrap_or_default(); - assert!( - body.len() == 2 && body[0].starts_with("Write-Error ") && body[1] == "exit 1", - "install-service.ps1: the {what} is not followed by Write-Error then exit 1: \ - {cond}\n then: {body:?}" - ); - }; let find = |what: &str, pred: &dyn Fn(&str) -> bool| -> Vec { let found: Vec = lines .iter() @@ -708,7 +743,7 @@ fn windows_installer_refusal_conditions_stop_the_install() { for i in find("owner refusal", &|l| { l == "if ($trustedOwners -notcontains $ownerSid) {" }) { - refuses_at(i, "owner refusal"); + refuses_at(&lines, i, "owner refusal"); } let dir_links = find("refusal of $ConfigDir as a link", &|l| { @@ -722,7 +757,7 @@ fn windows_installer_refusal_conditions_stop_the_install() { dir_links.len() ); for i in dir_links { - refuses_at(i, "refusal of $ConfigDir as a link"); + refuses_at(&lines, i, "refusal of $ConfigDir as a link"); } for i in find("refusal of a link or folder entry", &|l| { @@ -737,7 +772,7 @@ fn windows_installer_refusal_conditions_stop_the_install() { "install-service.ps1: an entry must be refused if it is a link or a folder, \ either one: {cond}" ); - refuses_at(i, "refusal of a link or folder entry"); + refuses_at(&lines, i, "refusal of a link or folder entry"); } } @@ -806,6 +841,163 @@ fn windows_installer_icacls_calls_act_on_links_and_check_exit_codes() { } } +/// Guards the peer ACL files install-service.ps1 creates, and its refusal of +/// legacy ones. +/// +/// Earlier releases read `peers.allow` and `peers.deny` from `\etc\fips` on +/// the system drive, where any local user can create files, and the service +/// still reads a file there when it is missing from `C:\ProgramData\fips`. So +/// for each of the two files the installer must stop when the legacy file +/// exists and the current one does not, which is exactly when the service +/// would enforce the legacy file, and otherwise create the current file empty +/// if it is missing, without truncating one that exists. Every refusal is +/// decided before any file is created, and all of it happens after the config +/// directory is secured and before the binaries are copied or the service is +/// registered, so a refusal leaves an existing install's binaries, config and +/// service as they were. Each check is bounded by its loop's closing brace, +/// since a statement moved out of its loop runs for the last file only. +#[test] +fn windows_installer_creates_empty_peer_acl_files_and_refuses_legacy_ones() { + let lines = ps_lines(&repo_file("packaging/windows/install-service.ps1")); + let all = |pred: &dyn Fn(&str) -> bool| -> Vec { + lines + .iter() + .enumerate() + .filter(|(_, l)| pred(l)) + .map(|(i, _)| i) + .collect() + }; + let first = |what: &str, pred: &dyn Fn(&str) -> bool| -> usize { + all(pred) + .first() + .copied() + .unwrap_or_else(|| panic!("install-service.ps1: no line {what}")) + }; + + let legacy_dir = first("assigning $legacyAclDir", &|l| { + l.starts_with("$legacyAclDir = ") + }); + assert_eq!( + lines[legacy_dir], r#"$legacyAclDir = "$env:SystemDrive\etc\fips""#, + "install-service.ps1: the legacy peer ACL directory is not \\etc\\fips on the \ + system drive" + ); + + let loop_line = r#"foreach ($name in @("peers.allow", "peers.deny")) {"#; + let loops = all(&|l| { + l.starts_with("foreach") && (l.contains("peers.allow") || l.contains("peers.deny")) + }); + assert_eq!( + loops.len(), + 2, + "install-service.ps1: expected two loops over the peer ACL files, the refusal \ + and the creation, found {}", + loops.len() + ); + for &i in &loops { + assert_eq!( + lines[i], loop_line, + "install-service.ps1: a peer ACL loop does not cover both files" + ); + } + let (refusal, creation) = (loops[0], loops[1]); + let (refusal_end, creation_end) = (block_end(&lines, refusal), block_end(&lines, creation)); + let refusal_body = refusal + 1..refusal_end; + let creation_body = creation + 1..creation_end; + + for text in [ + "$legacy = Join-Path $legacyAclDir $name", + r#"$current = "$ConfigDir\$name""#, + ] { + assert!( + lines[refusal_body.clone()].iter().any(|l| l == text), + "install-service.ps1: the refusal loop has no line {text}" + ); + } + let check = refusal_body + .clone() + .find(|&i| lines[i].starts_with("if (") && lines[i].contains("$legacy")) + .unwrap_or_else(|| { + panic!("install-service.ps1: the refusal loop does not test the legacy file") + }); + assert_eq!( + lines[check], + "if ((Test-Path -LiteralPath $legacy) -and -not (Test-Path -LiteralPath $current)) {", + "install-service.ps1: the refusal must hold only when the legacy file exists and \ + the current one does not" + ); + refuses_at(&lines, check, "refusal of a legacy peer ACL file"); + assert!( + block_end(&lines, check) < refusal_end, + "install-service.ps1: the refusal of a legacy peer ACL file does not close inside \ + the refusal loop" + ); + + let aclfile_line = r#"$aclFile = "$ConfigDir\$name""#; + let guard_line = "if (-not (Test-Path -LiteralPath $aclFile)) {"; + let create = creation_body + .clone() + .find(|&i| lines[i].contains("New-Item") && lines[i].contains("-ItemType File")) + .unwrap_or_else(|| panic!("install-service.ps1: the creation loop does not create a file")); + let new_item = &lines[create]; + assert!( + new_item.contains("$aclFile") && !new_item.to_ascii_lowercase().contains("-force"), + "install-service.ps1: the peer ACL file must be created at $aclFile without -Force, \ + which would truncate an existing list: {new_item}" + ); + assert!( + lines[creation + 1..create] + .iter() + .any(|l| l == aclfile_line), + "install-service.ps1: the creation loop does not set $aclFile to the file in $ConfigDir" + ); + let guard = create - 1; + assert!( + lines[guard] == guard_line && block_end(&lines, guard) < creation_end, + "install-service.ps1: the peer ACL file is not created only when it is missing" + ); + for l in &lines[creation_body] { + assert!( + [aclfile_line, guard_line, new_item.as_str(), "}"].contains(&l.as_str()) + || l.starts_with("Write-Host "), + "install-service.ps1: the creation loop does more than create a missing file: {l}" + ); + } + + let is_check = |l: &str| l == "& $refuseEntries"; + let order = [ + ("check after the reset", all(&is_check).get(2).copied()), + ("$legacyAclDir", Some(legacy_dir)), + ("refusal loop", Some(refusal)), + ("refusal of a legacy file", Some(check)), + ("end of the refusal loop", Some(refusal_end)), + ("creation loop", Some(creation)), + ("creation of a peer ACL file", Some(create)), + ("end of the creation loop", Some(creation_end)), + ( + "binary copy", + all(&|l| l.contains("$Binaries")).first().copied(), + ), + ( + "service registration", + all(&|l| l.contains("--install-service")).first().copied(), + ), + ]; + for pair in order.windows(2) { + let [(a, ia), (b, ib)] = pair else { + unreachable!("windows(2) yields pairs") + }; + let (ia, ib) = ( + ia.unwrap_or_else(|| panic!("install-service.ps1: no {a}")), + ib.unwrap_or_else(|| panic!("install-service.ps1: no {b}")), + ); + assert!( + ia < ib, + "install-service.ps1: {a} (code line {ia}) must come before {b} (code line {ib})" + ); + } +} + const COMMON_CONFIG: &str = "packaging/common/fips.yaml"; const OPENWRT_CONFIG: &str = "packaging/openwrt-ipk/files/etc/fips/fips.yaml"; From 8d9c5268ce2fd0e7dfb80b9c030aa1ecf61e23b4 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 27 Sep 2026 19:35:48 +0000 Subject: [PATCH 05/10] Stop the trailing-log check reading a longer function name as a call The "if ... ; then" pattern let its \S* end inside another name, so `if text=$(read_gateway_log ...); then` counted as a status-testing call of every function named `log`, and two test harnesses' log helpers were flagged for callers they do not have. The name must now start at an identifier boundary. --- testing/check-trailing-log.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/testing/check-trailing-log.py b/testing/check-trailing-log.py index 990dc34c..d2592167 100755 --- a/testing/check-trailing-log.py +++ b/testing/check-trailing-log.py @@ -132,7 +132,9 @@ def status_tested(name: str, all_text: str, defining_file: Path) -> list[str]: (rf"^\s*{re.escape(name)}\s*\|\|", " ||"), (rf"^\s*{re.escape(name)}\s+[^\n|&]*&&", " &&"), (rf"^\s*{re.escape(name)}\s*&&", " &&"), - (rf"\bif\s+\S*\s*{re.escape(name)}\b.*;\s*then", "if ... ; then"), + # The lookbehind keeps `\S*` from ending inside a longer name, so + # `if text=$(read_gateway_log ...); then` is not read as a call of `log`. + (rf"\bif\s+\S*\s*(? ; then"), # Command substitution. Found 2026-07-23 while fixing a function this # check reported clean: `total=$(count_log_pattern "$p") || { ... }` # consumes the status, but the line begins with the variable, so none From 84976b1fa2944118897c05fb3659c3d17e27699a Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 27 Sep 2026 19:35:48 +0000 Subject: [PATCH 06/10] fips-gateway: exit when the DNS listener cannot bind or stops The resolver ran as a detached task whose error was only logged, so a gateway whose DNS port was taken stayed up with .fips resolution dead and every health check passing. The listener is now bound before the address pool, NAT table and routes are created, and a bind failure exits with status 1. When the port is already in use, the error names the service most likely to hold it and how to find the holder with ss or netstat. A resolver task that stops while the gateway runs also ends the process, after NAT and routes are torn down, so systemd or procd restarts it or shows it failed. The resolver now forwards to the upstream address the startup probe resolved, so an upstream written as a hostname no longer passes the probe and then kills the resolver. The dns-resolver harness gains a check that a gateway configured on the daemon's DNS port exits with the hint before it creates the pool or the NAT table. The troubleshooting guide describes the new failure, and the exit-code table no longer lists a control-socket bind failure, which only warns. --- docs/how-to/troubleshoot-gateway.md | 61 ++++++++--- docs/reference/cli-fips-gateway.md | 2 +- src/bin/fips-gateway.rs | 81 ++++++++++----- src/config/gateway.rs | 6 ++ src/gateway/dns.rs | 155 +++++++++++++++++++++++++++- testing/dns-resolver/test.sh | 69 +++++++++++++ 6 files changed, 329 insertions(+), 45 deletions(-) diff --git a/docs/how-to/troubleshoot-gateway.md b/docs/how-to/troubleshoot-gateway.md index 6b532ded..a101a7d4 100644 --- a/docs/how-to/troubleshoot-gateway.md +++ b/docs/how-to/troubleshoot-gateway.md @@ -81,28 +81,57 @@ for the full flag list. ### Port conflict on the DNS listen port -Symptom: gateway fails to start with "address already in use" on -the configured `gateway.dns.listen` address. +Symptom: the gateway exits at startup, before it creates the address +pool or the NAT table, and the log carries an error such as this one, +wrapped here for reading: -The default `[::1]:5353` is loopback-only on an unprivileged port and -should not collide with any standard resolver. If you have overridden -`dns.listen` to bind port 53 (or a LAN-side address) and another DNS -server (systemd-resolved, dnsmasq, BIND) is already bound there, -identify it: - -```sh -sudo ss -tulnp | grep ':53' +```text +cannot bind the gateway DNS listener on [::]:53: Address already in +use (os error 98); another DNS server holds port 53: dnsmasq, +systemd-resolved's stub listener, unbound or BIND; find the holder +with `ss -ulpn 'sport = :53'` or `netstat -ulnp`, or set +gateway.dns.listen to a free port and point the resolver that +forwards .fips at it ``` -Two options: +Under systemd the unit restarts every five seconds and fails the same +way each time; under procd on OpenWrt the service stops respawning +after five failures within an hour. The gateway also exits, after +removing its NAT table and routes, when the DNS resolver stops while +the gateway is running; that log line reads "Gateway DNS resolver +stopped; exiting so the service manager restarts the gateway". -- **Stay on the loopback default.** Drop the override and let the - gateway use `[::1]:5353`. Configure the existing resolver to - forward `.fips` queries to it (the canonical OpenWrt deployment - works this way out of the box). +The error names the service most likely to hold the port: + +- **53**: another DNS server, such as dnsmasq, systemd-resolved's stub + listener, unbound or BIND. +- **5353**: mDNS. The fips daemon's LAN rendezvous + (`node.rendezvous.lan`), avahi-daemon or systemd-resolved's + MulticastDNS. +- **5354**: the fips daemon's own DNS responder. `gateway.dns.listen` + must not be the daemon's DNS port. +- **5355**: LLMNR, held by systemd-resolved unless `LLMNR=no`. +- **Any other port**: another process. + +Find the actual holder, replacing the port with your own: + +```sh +sudo ss -ulpn 'sport = :53' +# OpenWrt ships netstat but not ss: +netstat -ulnp +``` + +The default listen address, `[::1]:5353`, is loopback-only on an +unprivileged port. Two options: + +- **Move the gateway.** Set `gateway.dns.listen` to a free port and + point the resolver that forwards `.fips` at the same port. With the + loopback default, configure the existing resolver to forward `.fips` + queries to `[::1]:5353` (the canonical OpenWrt deployment works this + way out of the box). - **Relocate the conflicting resolver.** Move it to a different port - (or disable it if not needed) and let the gateway bind 53. + (or disable it if not needed) and let the gateway bind the port. Practical for systemd-resolved (set `DNSStubListener=no` in `/etc/systemd/resolved.conf`); rarely worth it for production resolvers. diff --git a/docs/reference/cli-fips-gateway.md b/docs/reference/cli-fips-gateway.md index 5a764fce..572ecee8 100644 --- a/docs/reference/cli-fips-gateway.md +++ b/docs/reference/cli-fips-gateway.md @@ -73,7 +73,7 @@ Linux host) and | Code | Meaning | | ---- | ------- | | `0` | Clean shutdown after `SIGINT` / `SIGTERM`. | -| `1` | Non-Linux platform, configuration load failure, missing or invalid `gateway:` block, NAT/network setup failure, or control-socket bind failure. The reason is printed to stderr or the log before exit. | +| `1` | Non-Linux platform, configuration load failure, missing or invalid `gateway:` block, the DNS listener could not bind or stopped while running, or NAT/network setup failure. The reason is printed to stderr or the log before exit. A control-socket bind failure is logged as a warning and the gateway continues without the socket. | ## Environment diff --git a/src/bin/fips-gateway.rs b/src/bin/fips-gateway.rs index fb2c6164..589deb6e 100644 --- a/src/bin/fips-gateway.rs +++ b/src/bin/fips-gateway.rs @@ -251,8 +251,9 @@ async fn main() { std::process::exit(1); } - // Check DNS upstream reachability (proves the FIPS daemon is running) - { + // Check DNS upstream reachability (proves the FIPS daemon is running). + // The resolver later forwards to the address this probe reached. + let upstream_addr = { let upstream = gw_config.dns.upstream(); info!(upstream = %upstream, "Checking DNS upstream reachability"); @@ -363,6 +364,24 @@ async fn main() { ); std::process::exit(1); } + upstream_addr + }; + + // --- Bind the DNS listener --- + // + // Before the pool, NAT table and routes exist, so a port that is already + // taken ends the gateway with nothing to tear down, and a service manager + // restarting it does not churn nftables. + let dns_socket = match dns::bind_listener(gw_config.dns.listen()).await { + Ok(socket) => socket, + Err(e) => { + error!("{e}"); + std::process::exit(1); + } + }; + match dns_socket.local_addr() { + Ok(addr) => info!(addr = %addr, "Gateway DNS resolver listening"), + Err(_) => info!(addr = %gw_config.dns.listen(), "Gateway DNS resolver listening"), } // --- Initialize components --- @@ -421,27 +440,16 @@ async fn main() { // --- Start DNS resolver task --- - let dns_pool = Arc::clone(&ip_pool); - let dns_event_tx = event_tx.clone(); - let dns_shutdown = shutdown_rx.clone(); - let dns_listen = gw_config.dns.listen().to_string(); - let dns_upstream = gw_config.dns.upstream().to_string(); - let dns_ttl = gw_config.dns.ttl(); - - let dns_task = tokio::spawn(async move { - if let Err(e) = dns::run_dns_resolver( - &dns_listen, - &dns_upstream, - dns_ttl, - dns_pool, - dns_event_tx, - dns_shutdown, - ) - .await - { - error!(error = %e, "DNS resolver error"); - } - }); + // Held in an Option because the main loop may see it complete, and a + // completed JoinHandle panics if it is polled again. + let mut dns_task = Some(tokio::spawn(dns::serve( + dns_socket, + upstream_addr, + gw_config.dns.ttl(), + Arc::clone(&ip_pool), + event_tx.clone(), + shutdown_rx.clone(), + ))); // --- Snapshot channel for control socket --- @@ -531,6 +539,7 @@ async fn main() { info!("fips-gateway running"); + let mut exit_code = 0; loop { tokio::select! { Some(event) = event_rx.recv() => { @@ -559,6 +568,25 @@ async fn main() { } } } + // The resolver ends only on shutdown, which has not been + // signalled while this loop runs, so any completion here means + // .fips resolution has stopped. Exit non-zero so systemd or procd + // restarts the gateway or shows it failed. + result = async { dns_task.as_mut().expect("guarded by the precondition").await }, + if dns_task.is_some() => { + dns_task = None; + let cause = match result { + Ok(Ok(())) => "the resolver returned without an error".to_string(), + Ok(Err(e)) => e.to_string(), + Err(e) => e.to_string(), + }; + error!( + cause = %cause, + "Gateway DNS resolver stopped; exiting so the service manager restarts the gateway" + ); + exit_code = 1; + break; + } _ = tokio::signal::ctrl_c() => { info!("Received SIGINT, shutting down"); break; @@ -582,7 +610,9 @@ async fn main() { task.abort(); let _ = task.await; } - let _ = dns_task.await; + if let Some(task) = dns_task { + let _ = task.await; + } let _ = tick_task.await; // Log final pool status @@ -606,4 +636,7 @@ async fn main() { } info!("fips-gateway shutdown complete"); + if exit_code != 0 { + std::process::exit(exit_code); + } } diff --git a/src/config/gateway.rs b/src/config/gateway.rs index b9a96330..f7d4cf00 100644 --- a/src/config/gateway.rs +++ b/src/config/gateway.rs @@ -160,6 +160,12 @@ impl GatewayDnsConfig { pub fn ttl(&self) -> u32 { self.ttl.unwrap_or(DEFAULT_DNS_TTL) } + + /// The port of a listen address: the digits after its last `:`, or + /// `None` when they do not form a port. Works on a hostname form too. + pub(crate) fn port_of(listen: &str) -> Option { + listen.rsplit_once(':')?.1.parse().ok() + } } /// Conntrack timeout overrides (`gateway.conntrack.*`). diff --git a/src/gateway/dns.rs b/src/gateway/dns.rs index 4d9a069e..97054a5c 100644 --- a/src/gateway/dns.rs +++ b/src/gateway/dns.rs @@ -17,6 +17,7 @@ use tracing::{debug, info, trace, warn}; use super::pool::{PoolEvent, VirtualIpPool}; use crate::NodeAddr; +use crate::config::GatewayDnsConfig; /// Timeout for upstream DNS queries. const UPSTREAM_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(5); @@ -156,25 +157,108 @@ fn build_aaaa_response(query: &Packet, virtual_ip: Ipv6Addr, ttl: u32) -> Option response.build_bytes_vec_compressed().ok() } +/// The gateway DNS listener could not be bound. +/// +/// The message names the listen address and, when the port is already in +/// use, the service most likely to hold it and how to find the holder. +#[derive(Debug, thiserror::Error)] +#[error("cannot bind the gateway DNS listener on {listen}: {source}{}", in_use_hint(.listen, .source))] +pub struct ListenError { + listen: String, + source: std::io::Error, +} + +impl ListenError { + /// The kind of the underlying bind error. + pub fn kind(&self) -> std::io::ErrorKind { + self.source.kind() + } +} + +/// The suffix `ListenError`'s message carries for an address-in-use error: +/// the likely holder of the port, and how to find the actual one. +fn in_use_hint(listen: &str, source: &std::io::Error) -> String { + if source.kind() != std::io::ErrorKind::AddrInUse { + return String::new(); + } + let (holder, port) = match GatewayDnsConfig::port_of(listen) { + Some(port) => (holder_hint(port), port.to_string()), + None => (holder_hint(0), "".to_string()), + }; + format!( + "; {holder}; find the holder with `ss -ulpn 'sport = :{port}'` or `netstat -ulnp`, \ + or set gateway.dns.listen to a free port and point the resolver that forwards .fips at it" + ) +} + +/// The service most likely to hold a DNS listen port that is already in use. +pub(crate) fn holder_hint(port: u16) -> &'static str { + match port { + 53 => { + "another DNS server holds port 53: dnsmasq, systemd-resolved's stub listener, unbound or BIND" + } + 5353 => { + "port 5353 is mDNS: the fips daemon's LAN rendezvous (node.rendezvous.lan), \ + avahi-daemon or systemd-resolved's MulticastDNS may hold it" + } + 5354 => { + "the fips daemon's own DNS responder listens on 5354 by default; \ + gateway.dns.listen must not be the daemon's DNS port" + } + 5355 => "port 5355 is LLMNR, held by systemd-resolved unless LLMNR=no", + 5365 => "another fips-gateway may already be running", + _ => "another process holds it", + } +} + +/// Bind the gateway DNS listener. +/// +/// Called before the gateway creates anything it would have to tear down, so +/// a port that is already taken stops the gateway before it starts. +pub async fn bind_listener(listen: &str) -> Result { + UdpSocket::bind(listen).await.map_err(|source| ListenError { + listen: listen.to_string(), + source, + }) +} + /// Run the gateway DNS resolver. /// -/// Listens for DNS queries, forwards `.fips` queries to the upstream -/// daemon resolver, allocates virtual IPs, and returns them to clients. +/// Binds `listen_addr`, then serves as [`serve`] does. The gateway binary +/// binds and serves separately so that a bind failure stops it at startup. pub async fn run_dns_resolver( listen_addr: &str, upstream_addr: &str, ttl: u32, pool: std::sync::Arc>, event_tx: tokio::sync::mpsc::Sender, - mut shutdown: watch::Receiver, + shutdown: watch::Receiver, ) -> Result<(), std::io::Error> { - let socket = UdpSocket::bind(listen_addr).await?; + let socket = bind_listener(listen_addr) + .await + .map_err(|e| std::io::Error::new(e.kind(), e))?; info!(addr = %listen_addr, "Gateway DNS resolver listening"); let upstream: SocketAddr = upstream_addr .parse() .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidInput, e))?; + serve(socket, upstream, ttl, pool, event_tx, shutdown).await +} + +/// Serve DNS queries on a bound listener until shutdown. +/// +/// Forwards `.fips` queries to the upstream daemon resolver, allocates +/// virtual IPs, and returns them to clients. Returns `Ok` on shutdown and +/// `Err` when receiving from the listener fails. +pub async fn serve( + socket: UdpSocket, + upstream: SocketAddr, + ttl: u32, + pool: std::sync::Arc>, + event_tx: tokio::sync::mpsc::Sender, + mut shutdown: watch::Receiver, +) -> Result<(), std::io::Error> { let mut buf = vec![0u8; MAX_DNS_SIZE]; loop { @@ -807,6 +891,69 @@ mod tests { )); } + #[test] + fn an_in_use_hint_names_the_mdns_responders_for_5353() { + let hint = holder_hint(5353); + assert!(hint.contains("mDNS"), "{hint}"); + assert!(hint.contains("node.rendezvous.lan"), "{hint}"); + assert!(hint.contains("avahi-daemon"), "{hint}"); + } + + #[test] + fn an_in_use_hint_names_llmnr_for_5355() { + let hint = holder_hint(5355); + assert!(hint.contains("LLMNR"), "{hint}"); + assert!(!hint.contains("mDNS"), "{hint}"); + } + + #[test] + fn an_in_use_hint_names_the_daemon_for_5354() { + let hint = holder_hint(5354); + assert!(hint.contains("fips daemon's own DNS responder"), "{hint}"); + } + + #[test] + fn an_in_use_hint_names_a_dns_server_for_53() { + let hint = holder_hint(53); + assert!(hint.contains("another DNS server"), "{hint}"); + assert!(hint.contains("dnsmasq"), "{hint}"); + } + + #[test] + fn an_in_use_hint_names_another_gateway_for_the_default_port() { + let hint = holder_hint(5365); + assert!(hint.contains("another fips-gateway"), "{hint}"); + } + + #[test] + fn an_in_use_hint_names_another_process_for_an_unknown_port() { + assert_eq!(holder_hint(40000), "another process holds it"); + } + + #[tokio::test] + async fn binding_a_held_port_fails_with_addr_in_use_and_names_the_port_ss_and_netstat() { + let holder = UdpSocket::bind("[::1]:0").await.unwrap(); + let port = holder.local_addr().unwrap().port(); + let listen = format!("[::1]:{port}"); + + let err = bind_listener(&listen) + .await + .expect_err("binding a held port must fail"); + assert_eq!(err.kind(), std::io::ErrorKind::AddrInUse); + let message = err.to_string(); + assert!(message.contains(&listen), "{message}"); + assert!(message.contains(&format!("sport = :{port}")), "{message}"); + assert!(message.contains("ss -ulpn"), "{message}"); + assert!(message.contains("netstat -ulnp"), "{message}"); + assert!(message.contains(holder_hint(port)), "{message}"); + } + + #[tokio::test] + async fn binding_a_free_port_returns_a_bound_socket() { + let socket = bind_listener("[::1]:0").await.expect("bind a free port"); + assert_ne!(socket.local_addr().unwrap().port(), 0); + } + #[test] fn test_extract_fips_name() { // Build a simple AAAA query for test.fips diff --git a/testing/dns-resolver/test.sh b/testing/dns-resolver/test.sh index 038a1190..8aea1bb9 100755 --- a/testing/dns-resolver/test.sh +++ b/testing/dns-resolver/test.sh @@ -701,6 +701,73 @@ DOCKERFILE # prepare_binaries) and are copied into each per-distro runtime image. # ───────────────────────────────────────────────────────────────────── +# Print a fips-gateway log from the container with terminal colour codes +# removed, so structured fields can be matched as plain "key=value" text. +# Fails when the log cannot be read. +read_gateway_log() { + local name="$1" log="$2" + local text + text=$(docker exec "$name" cat "$log" 2>/dev/null) || return 1 + printf '%s\n' "$text" | sed 's/\x1b\[[0-9;]*m//g' + return 0 +} + +# The gateway exits at the DNS bind when its listen port is held. The +# daemon in this container holds [::1]:5354, so a gateway configured to +# listen there must exit non-zero with the hint naming the daemon, before +# it creates the address pool or the NAT table. Any build that gets past +# the bind logs one of the pool or NAT lines below, whichever way NAT goes +# in this container, so their absence shows the exit came first. +check_gateway_exits_on_held_port() { + local name="$1" + local log=/var/log/fips-gateway-held.log + local fail_before=$FAIL + docker exec "$name" bash -c 'cat > /tmp/gateway-held.yaml <$log 2>&1; echo \"EXIT=\$?\" >>$log" + + local text + if ! text=$(read_gateway_log "$name" "$log"); then + fail "could not read $log, so the held-port exit was not observed" + return + fi + local rc + rc=$(printf '%s\n' "$text" | sed -n 's/^EXIT=//p' | tail -n 1) + if [ -z "$rc" ]; then + fail "the held-port gateway run left no exit status in $log" + elif [ "$rc" = "0" ] || [ "$rc" = "124" ]; then + fail "fips-gateway on a held DNS port exited $rc (expected non-zero, not the timeout)" + else + pass "fips-gateway on a held DNS port exits $rc" + fi + if printf '%s\n' "$text" | grep -qF "the fips daemon's own DNS responder listens on 5354"; then + pass "the bind error names the daemon as the likely holder of 5354" + else + fail "the bind error does not carry the 5354 hint" + fi + local line + for line in "Failed to create virtual IP pool" "Failed to create nftables table" "Created nftables table"; do + if printf '%s\n' "$text" | grep -qF "$line"; then + fail "fips-gateway reached a step after the DNS bind: '$line'" + else + pass "fips-gateway stopped before '$line'" + fi + done + if [ "$FAIL" -gt "$fail_before" ]; then + echo " --- $log ---" + printf '%s\n' "$text" | tail -20 + fi +} + # Args: # distro_label: short tag for container/image names (e.g. "debian12") # docker_base_image: e.g. "debian:12", "ubuntu:26.04" @@ -918,6 +985,8 @@ EOF' # care that the upstream reachability step succeeded). docker exec "$name" pkill -f fips-gateway 2>/dev/null || true + check_gateway_exits_on_held_port "$name" + # Teardown via the script: backend config file must be removed # (path varies by backend selected above). local teardown_path From 28f9fc007a606184c4832839c140f4ef26081cb5 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 27 Sep 2026 19:22:08 +0000 Subject: [PATCH 07/10] Move the gateway's default DNS port from 5353 to 5365 5353 is the mDNS port, which the daemon's LAN rendezvous, Avahi and systemd-resolved can hold. On an OpenWrt access point with the gateway enabled, the gateway lost the port to the daemon's mDNS responder and dnsmasq sent .fips queries to a responder that does not answer them. 5365 is unassigned by IANA and not used by any common resolver. The OpenWrt init script now points dnsmasq at the port gateway.dns.listen actually sets, falling back to the new default, and when it swaps the .fips forwarding it clears every loopback .fips entry rather than four fixed ones, so a stale entry for an old or custom port does not linger. Forwards to other hosts and other domains are kept. The shipped OpenWrt config and the example config leave the listen line at the default. fips.yaml is a conffile on OpenWrt, and fips-ap-setup edits it, so routers that set up an access point would keep the old explicit listen: "[::1]:5353" across an upgrade and stay broken after the default moved. The first-boot setup script, which every install and upgrade path runs before the services start, now rewrites that exact shipped line to the line a fresh install ships and logs that it did; any other value is left as configured. The script is sourced rather than executed on the SDK-feed and sysupgrade paths, so the migration runs last, cannot end the script early and cannot change its exit status. The script also removes stale loopback .fips forwarding entries for port 5353. The gateway warns at startup when it is configured on 5353, whether or not the bind succeeds, since an mDNS responder can take the port later. The OpenWrt scenario harness checks the port the init script reads, that its default matches the gateway's, and the swap's cleanup against a uci stub. It also runs the real setup script executed by both package managers' upgrade scripts and sourced in a subshell, and checks the negative, missing-file, repeat-run and stale-entry cases. The dns-resolver and deb-install harnesses check that the gateway binds the new default. --- docs/design/fips-gateway.md | 8 +- docs/how-to/deploy-gateway.md | 13 +- docs/how-to/troubleshoot-gateway.md | 13 +- docs/reference/configuration.md | 4 +- docs/tutorials/deploy-fips-gateway.md | 13 +- packaging/common/fips.yaml | 2 +- .../openwrt-ipk/files/etc/fips/fips.yaml | 8 +- .../openwrt-ipk/files/etc/init.d/fips-gateway | 64 ++- .../files/etc/uci-defaults/90-fips-setup | 52 ++- src/bin/fips-gateway.rs | 5 + src/config/gateway.rs | 56 ++- testing/deb-install/test.sh | 38 ++ testing/dns-resolver/test.sh | 48 ++- testing/openwrt/fixtures/released-fips.yaml | 190 ++++++++ testing/openwrt/scenarios.sh | 407 +++++++++++++++++- 15 files changed, 869 insertions(+), 52 deletions(-) create mode 100644 testing/openwrt/fixtures/released-fips.yaml diff --git a/docs/design/fips-gateway.md b/docs/design/fips-gateway.md index 51ce4714..47165abb 100644 --- a/docs/design/fips-gateway.md +++ b/docs/design/fips-gateway.md @@ -130,7 +130,7 @@ There is no `fipsctl gateway` subcommand; clients (including │ │ │ ┌──────────────┐ ┌───────────┐ │ │ │ DNS proxy │ │ Virtual │ │ - │ │ ([::1]:5353) │─▶│ IP pool │ │ + │ │ ([::1]:5365) │─▶│ IP pool │ │ │ │ .fips only │ │ (state │ │ │ └──────┬───────┘ │ machine) │ │ │ │ └─────┬─────┘ │ @@ -182,8 +182,10 @@ involving the DNS proxy or the pool. ### DNS Resolution Flow 1. A LAN client sends a DNS query to the gateway's listener (default - `[::1]:5353`, configurable via `gateway.dns.listen`). The default - is loopback-only on an unprivileged port: the canonical deployment + `[::1]:5365`, configurable via `gateway.dns.listen`). The default + is not 5353, the mDNS port, which the daemon's LAN rendezvous and + other mDNS responders hold. It is loopback-only on an unprivileged + port: the canonical deployment has another resolver on the host (dnsmasq, systemd-resolved, BIND) holding port 53 and forwarding `.fips` queries to the gateway over loopback. Operators on a host without a pre-existing resolver on diff --git a/docs/how-to/deploy-gateway.md b/docs/how-to/deploy-gateway.md index cbd80d7d..e1bb3595 100644 --- a/docs/how-to/deploy-gateway.md +++ b/docs/how-to/deploy-gateway.md @@ -143,7 +143,7 @@ virtual IPs, which is the gateway's hard cap regardless of CIDR width. This minimum config is enough to start the gateway. The `dns.*` block -is optional and defaults to `listen: "[::1]:5353"` and +is optional and defaults to `listen: "[::1]:5365"` and `upstream: "[::1]:5354"`. The full block — including `dns.*`, `pool_grace_period`, `conntrack.*`, and `port_forwards[]` — is documented in @@ -196,7 +196,7 @@ Constraints: ```yaml gateway: dns: - listen: "[::1]:5353" + listen: "[::1]:5365" upstream: "[::1]:5354" ttl: 60 ``` @@ -204,15 +204,16 @@ gateway: Common cases: - **Another resolver on the host (the canonical case):** the default - `listen: "[::1]:5353"` is loopback-only on an unprivileged port, + `listen: "[::1]:5365"` is loopback-only on an unprivileged port, so it never conflicts with dnsmasq, systemd-resolved, or BIND holding 53. Configure the existing resolver to forward `.fips` - queries to `[::1]:5353` and you are done — this is what the - OpenWrt ipk does automatically. + queries to `[::1]:5365` and you are done — this is what the + OpenWrt ipk does automatically. On OpenWrt the init script reads + `gateway.dns.listen` and points dnsmasq at whatever port it sets. - **No other resolver on the host:** set `listen: "[::]:53"` explicitly and LAN clients can query the gateway directly. - **systemd-resolved is on port 53:** the default already side-steps - this — leave the listen address at `[::1]:5353` and configure the + this — leave the listen address at `[::1]:5365` and configure the stub or a small forwarder to delegate `.fips` to the gateway. If you would rather have the gateway on 53 directly, disable the systemd stub listener (`DNSStubListener=no` in diff --git a/docs/how-to/troubleshoot-gateway.md b/docs/how-to/troubleshoot-gateway.md index a101a7d4..719c8868 100644 --- a/docs/how-to/troubleshoot-gateway.md +++ b/docs/how-to/troubleshoot-gateway.md @@ -111,6 +111,7 @@ The error names the service most likely to hold the port: - **5354**: the fips daemon's own DNS responder. `gateway.dns.listen` must not be the daemon's DNS port. - **5355**: LLMNR, held by systemd-resolved unless `LLMNR=no`. +- **5365**, the default: another fips-gateway already running. - **Any other port**: another process. Find the actual holder, replacing the port with your own: @@ -121,13 +122,17 @@ sudo ss -ulpn 'sport = :53' netstat -ulnp ``` -The default listen address, `[::1]:5353`, is loopback-only on an -unprivileged port. Two options: +The default listen address, `[::1]:5365`, is loopback-only on an +unprivileged port. Releases before 0.5.2 defaulted to `[::1]:5353`, +the mDNS port; a config that still sets it explicitly keeps it, and +the gateway warns at startup. On OpenWrt, an upgrade rewrites the +previously shipped `listen: "[::1]:5353"` line to the new default. +Two options: - **Move the gateway.** Set `gateway.dns.listen` to a free port and point the resolver that forwards `.fips` at the same port. With the loopback default, configure the existing resolver to forward `.fips` - queries to `[::1]:5353` (the canonical OpenWrt deployment works this + queries to `[::1]:5365` (the canonical OpenWrt deployment works this way out of the box). - **Relocate the conflicting resolver.** Move it to a different port @@ -240,7 +245,7 @@ not running or not enabled. Check that the daemon config has **Step 2.** Verify the gateway is listening on its DNS port: ```sh -sudo ss -tulnp | grep -E ':(53|5353)\b' +sudo ss -tulnp | grep -E ':(53|5365)\b' ``` If nothing is listening on the configured `dns.listen` address, the diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index 117f82fd..5d0ac209 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -889,7 +889,7 @@ Non-`.fips` queries are answered with `REFUSED`. | Parameter | Type | Default | Description | |-----------|------|---------|-------------| -| `gateway.dns.listen` | string | `"[::1]:5353"` | DNS listen address. The default binds IPv6 loopback on an unprivileged port, matching the canonical deployment where another resolver on the host (dnsmasq, systemd-resolved, BIND) holds port 53 and forwards `.fips` queries to the gateway over loopback. Bind on the LAN-side IP (e.g., `"192.168.1.1:53"`) or wildcard (`"[::]:53"`) only on hosts with no other resolver on 53 and where LAN clients query the gateway directly. See [../how-to/troubleshoot-gateway.md](../how-to/troubleshoot-gateway.md). | +| `gateway.dns.listen` | string | `"[::1]:5365"` | DNS listen address. The default binds IPv6 loopback on an unprivileged port, not 5353, the mDNS port, matching the canonical deployment where another resolver on the host (dnsmasq, systemd-resolved, BIND) holds port 53 and forwards `.fips` queries to the gateway over loopback. Bind on the LAN-side IP (e.g., `"192.168.1.1:53"`) or wildcard (`"[::]:53"`) only on hosts with no other resolver on 53 and where LAN clients query the gateway directly. See [../how-to/troubleshoot-gateway.md](../how-to/troubleshoot-gateway.md). | | `gateway.dns.upstream` | string | `"[::1]:5354"` | Upstream FIPS daemon resolver. **Must match the daemon's `dns.bind_addr` and `dns.port`.** Defaults match the daemon defaults (`::1:5354`). A v4 upstream (`"127.0.0.1:5354"`) cannot reach a daemon bound on `[::1]:5354` — Linux IPv6 sockets bound to explicit `::1` do not accept v4-mapped traffic. If you change the daemon's `dns.bind_addr`, update this field accordingly. | | `gateway.dns.ttl` | u32 | `60` | TTL in seconds on AAAA responses returned to LAN clients. Smaller values let the gateway recycle pool addresses faster; larger values reduce LAN-side query traffic. | @@ -936,7 +936,7 @@ gateway: pool: "fd01::/112" lan_interface: "enp3s0" dns: - listen: "[::1]:5353" + listen: "[::1]:5365" upstream: "[::1]:5354" ttl: 60 pool_grace_period: 60 diff --git a/docs/tutorials/deploy-fips-gateway.md b/docs/tutorials/deploy-fips-gateway.md index d505eab2..2328d6df 100644 --- a/docs/tutorials/deploy-fips-gateway.md +++ b/docs/tutorials/deploy-fips-gateway.md @@ -132,7 +132,7 @@ gateway: pool: "fd01::/112" # virtual IP range (up to 65535 addresses) lan_interface: "br-lan" # LAN-facing interface for proxy NDP dns: - listen: "[::1]:5353" # gateway DNS bind (IPv6 loopback only) + # listen: "[::1]:5365" # the default; the init script points dnsmasq at this port upstream: "[::1]:5354" # FIPS daemon DNS resolver (matches daemon default) ttl: 60 # DNS TTL and mapping lifetime (seconds) pool_grace_period: 60 # seconds after last session before reclaiming @@ -147,9 +147,10 @@ Three things to notice: - `lan_interface: "br-lan"` — the OpenWrt LAN bridge. The gateway installs proxy-NDP entries on this interface so LAN clients can ARP-equivalent for pool addresses. -- `dns.listen: "[::1]:5353"` — the gateway's DNS bind, pinned to - IPv6 loopback only. dnsmasq, which owns LAN port 53, forwards - `.fips` queries to it. The init script wires up that forwarding; +- `dns.listen`, commented out — the gateway's DNS bind, left at its + default `[::1]:5365`, IPv6 loopback only. dnsmasq, which owns LAN + port 53, forwards `.fips` queries to it. The init script reads + `gateway.dns.listen` and points dnsmasq at whatever port it sets; you don't bind to a LAN address yourself. For the full reference, see @@ -172,7 +173,7 @@ Behind that single command, the init script `/etc/sysctl.d/fips-gateway.conf`. 2. **Reconfigures dnsmasq via UCI** so `.fips` queries arriving at the LAN's port 53 are forwarded to the gateway's loopback - listener on port 5353 instead of going straight to the daemon's + listener on port 5365 instead of going straight to the daemon's resolver on port 5354. (Dnsmasq still owns 53; the gateway sits in front of the daemon for `.fips` only.) 3. **Adds a global-scope IPv6 prefix** to `br-lan`. Without a @@ -242,7 +243,7 @@ Expectations: > **What just happened end to end.** Your client asked dnsmasq for > `test-us01.fips`. Dnsmasq forwarded the query to the gateway's -> loopback listener on port 5353. The gateway forwarded the query on +> loopback listener on port 5365. The gateway forwarded the query on > to the daemon's resolver on port 5354. The daemon answered with > `test-us01`'s mesh address (`fd97:...`). The gateway allocated a > virtual IP from `fd01::/112`, installed nftables DNAT/SNAT/ diff --git a/packaging/common/fips.yaml b/packaging/common/fips.yaml index e23b4bf1..885bec7e 100644 --- a/packaging/common/fips.yaml +++ b/packaging/common/fips.yaml @@ -133,7 +133,7 @@ transports: # pool: "fd01::/112" # lan_interface: "eth0" # dns: -# listen: "[::1]:5353" +# listen: "[::1]:5365" # # upstream must match the daemon's dns.bind_addr above. The # # default "[::1]:5354" matches the daemon's default. If you set # # the daemon to bind on a wildcard ("::") or specific address, diff --git a/packaging/openwrt-ipk/files/etc/fips/fips.yaml b/packaging/openwrt-ipk/files/etc/fips/fips.yaml index 2f15160d..ea0bb1de 100644 --- a/packaging/openwrt-ipk/files/etc/fips/fips.yaml +++ b/packaging/openwrt-ipk/files/etc/fips/fips.yaml @@ -164,15 +164,15 @@ transports: # No BLE transport: OpenWrt builds target musl, which has no BlueZ backend. -# Outbound LAN gateway. dnsmasq forwards .fips queries to listen=[::1]:5353 -# while it runs (configured by the fips-gateway init script). Requires IPv6 -# forwarding enabled. +# Outbound LAN gateway. While it runs, the fips-gateway init script points +# dnsmasq's .fips forwarding at the port dns.listen sets (default [::1]:5365). +# Requires IPv6 forwarding enabled. gateway: enabled: true pool: "fd01::/112" lan_interface: "br-lan" dns: - listen: "[::1]:5353" + # listen: "[::1]:5365" # the default; the init script points dnsmasq at this port upstream: "[::1]:5354" ttl: 60 pool_grace_period: 60 diff --git a/packaging/openwrt-ipk/files/etc/init.d/fips-gateway b/packaging/openwrt-ipk/files/etc/init.d/fips-gateway index e9e16879..2c12326a 100755 --- a/packaging/openwrt-ipk/files/etc/init.d/fips-gateway +++ b/packaging/openwrt-ipk/files/etc/init.d/fips-gateway @@ -15,8 +15,10 @@ STOP=09 PROG=/usr/bin/fips-gateway CONFIG=/etc/fips/fips.yaml -# Port the gateway DNS listens on (must match dns.listen in fips.yaml). -GW_DNS_PORT=5353 +# Port the gateway DNS listens on when gateway.dns.listen is not set. Must +# match DEFAULT_DNS_LISTEN in the gateway's source; the scenario harness checks +# the two agree. The port actually used comes from gateway_dns_port. +GW_DNS_DEFAULT=5365 # Port the FIPS daemon DNS listens on. DAEMON_DNS_PORT=5354 @@ -40,11 +42,11 @@ start_service() { # Load conntrack module for /proc/net/nf_conntrack. modprobe nf_conntrack 2>/dev/null || true - # Redirect dnsmasq .fips forwarding from daemon (5354) to gateway (5353) - # so LAN clients get virtual IPs instead of raw mesh addresses. - # Done early and synchronously so dnsmasq is ready before the gateway - # starts accepting DNS queries. - dnsmasq_swap_fips_upstream "$GW_DNS_PORT" + # Redirect dnsmasq .fips forwarding from the daemon (5354) to the port the + # gateway listens on, so LAN clients get virtual IPs instead of raw mesh + # addresses. Done early and synchronously so dnsmasq is ready before the + # gateway starts accepting DNS queries. + dnsmasq_swap_fips_upstream "$(gateway_dns_port)" sleep 1 # Add a global-scope IPv6 prefix to br-lan so Android/Chrome clients @@ -87,6 +89,35 @@ gateway_config_enabled() { awk '/^gateway:/{found=1; next} found && /^[^ ]/{found=0} found && /enabled:/{gsub(/.*enabled:[[:space:]]*/, ""); gsub(/["'"'"']/, ""); print; exit}' "$CONFIG" } +# Print the port the gateway's DNS listener will bind: the digits after the +# last ":" of the "listen:" value inside the top-level "gateway:" block of +# fips.yaml, or $GW_DNS_DEFAULT when there is no such line or its value does +# not end in a port. Commented lines are skipped, and "listen_port:" (a port +# forward key) does not match. +# +# Block-style YAML only: a flow-style "dns: {listen: ...}" reads as the +# default. The dnsmasq entry this port feeds is always ::1#, so a +# gateway listening only on 127.0.0.1 is still not reachable through it. +gateway_dns_port() { + local port + port="$(awk ' + /^[A-Za-z_]/ { top = $1 } + top != "gateway:" { next } + /^[[:space:]]*#/ { next } + /^[[:space:]]+listen:/ { + v = $0 + sub(/^[[:space:]]+listen:[[:space:]]*/, "", v) + sub(/[[:space:]]+#.*$/, "", v) + gsub(/["'"'"']/, "", v) + sub(/[[:space:]]+$/, "", v) + n = split(v, part, ":") + if (n > 1 && part[n] ~ /^[0-9]+$/) print part[n] + exit + } + ' "$CONFIG" 2>/dev/null)" + echo "${port:-$GW_DNS_DEFAULT}" +} + # Extract the gateway pool CIDR from fips.yaml. # Looks for "pool:" indented under the top-level "gateway:" block. gateway_pool_cidr() { @@ -178,13 +209,20 @@ gateway_remove_global_prefix() { # $1 = target port number dnsmasq_swap_fips_upstream() { local port="$1" + local server - # Remove both possible entries, then add the correct one. - uci -q del_list dhcp.@dnsmasq[0].server="/fips/127.0.0.1#${DAEMON_DNS_PORT}" 2>/dev/null - uci -q del_list dhcp.@dnsmasq[0].server="/fips/127.0.0.1#${GW_DNS_PORT}" 2>/dev/null - # Also handle IPv6 loopback variants. - uci -q del_list dhcp.@dnsmasq[0].server="/fips/::1#${DAEMON_DNS_PORT}" 2>/dev/null - uci -q del_list dhcp.@dnsmasq[0].server="/fips/::1#${GW_DNS_PORT}" 2>/dev/null + # Remove every loopback .fips forward, then add the one for $port. That + # covers the daemon's port, this gateway's, and a stale entry for any other + # local port, such as the old default 5353 or a changed gateway.dns.listen. + # A .fips forward to another host and servers for other domains are kept. + # uci prints a list on one line separated by spaces. + for server in $(uci -q get 'dhcp.@dnsmasq[0].server' 2>/dev/null); do + case "$server" in + "/fips/::1#"* | "/fips/127.0.0.1#"*) + uci -q del_list dhcp.@dnsmasq[0].server="$server" 2>/dev/null + ;; + esac + done uci add_list dhcp.@dnsmasq[0].server="/fips/::1#${port}" uci commit dhcp diff --git a/packaging/openwrt-ipk/files/etc/uci-defaults/90-fips-setup b/packaging/openwrt-ipk/files/etc/uci-defaults/90-fips-setup index a89c27b5..44965971 100644 --- a/packaging/openwrt-ipk/files/etc/uci-defaults/90-fips-setup +++ b/packaging/openwrt-ipk/files/etc/uci-defaults/90-fips-setup @@ -1,8 +1,14 @@ #!/bin/sh # FIPS first-boot setup — runs once after package installation. # Configures the firewall and kernel modules for FIPS operation. -# This script is executed by /etc/rc.d/S19sysctl on first boot and -# then deleted by the UCI defaults mechanism. +# The UCI defaults mechanism runs it once and deletes it when it ends with +# status 0. +# +# It is executed by the package's postinst, but sourced, not executed, by +# OpenWrt's default_postinst for a package built from the SDK feed and by the +# first-boot uci-defaults run after a sysupgrade. Nothing here may exit early, +# change directory or set shell options, and it must end with status 0: a +# non-zero status leaves the script in place to run again on every boot. # --------------------------------------------------------------------------- # 1. Kernel modules @@ -63,9 +69,13 @@ uci commit firewall # dnsmasq init script builds its config from UCI and loads no directory under # /etc. The daemon's DNS responder binds ::1. The 127.0.0.1 del_list removes # the entry older packages added. While fips-gateway runs, its init script -# points this entry at the gateway's DNS port instead. +# points this entry at the gateway's DNS port instead. The two del_lists after +# the first remove the gateway's entry for its old default port, left behind +# by a gateway that stopped without its init script's stop running. uci -q del_list dhcp.@dnsmasq[0].server="/fips/127.0.0.1#5354" 2>/dev/null || true +uci -q del_list dhcp.@dnsmasq[0].server="/fips/::1#5353" 2>/dev/null || true +uci -q del_list dhcp.@dnsmasq[0].server="/fips/127.0.0.1#5353" 2>/dev/null || true uci -q del_list dhcp.@dnsmasq[0].server="/fips/::1#5354" 2>/dev/null || true uci add_list dhcp.@dnsmasq[0].server="/fips/::1#5354" uci -q del_list dhcp.@dnsmasq[0].rebind_domain="fips" 2>/dev/null || true @@ -86,4 +96,40 @@ grep -qxF 'nf_conntrack' /etc/modules.d/nf-conntrack 2>/dev/null || \ # proxy NDP entries are actually added. sysctl -p /etc/sysctl.d/fips-gateway.conf 2>/dev/null || true +# --------------------------------------------------------------------------- +# 5. Gateway DNS listen port +# --------------------------------------------------------------------------- +# Every release up to 0.5.1 shipped the gateway's DNS listener on 5353, the +# mDNS port, which the daemon's LAN rendezvous can hold. fips.yaml is a +# conffile and fips-ap-setup edits it, so an upgrade keeps the old line. +# Rewrite exactly that shipped line, four-space indent and nothing after the +# closing quote, inside the top-level gateway block, to the line a fresh +# install ships. Any other value is left as configured; the gateway warns at +# startup when it is still on the mDNS port. +# +# Runs last, and cannot fail the script: see the note at the top. +fips_migrate_gateway_dns_listen() { + local cfg=/etc/fips/fips.yaml + local old=' listen: "[::1]:5353"' + local new=' # listen: "[::1]:5365" # the default; the init script points dnsmasq at this port' + local msg='fips: moved gateway.dns.listen off the mDNS port 5353 to the default [::1]:5365' + + if [ -f "$cfg" ] && grep -qxF "$old" "$cfg" 2>/dev/null; then + if awk -v old="$old" -v new="$new" ' + /^[A-Za-z_]/ { top = $1 } + top == "gateway:" && $0 == old { print new; changed = 1; next } + { print } + END { exit changed ? 0 : 1 } + ' "$cfg" > "$cfg.tmp" 2>/dev/null && + chmod 600 "$cfg.tmp" 2>/dev/null && + mv -f "$cfg.tmp" "$cfg" 2>/dev/null; then + logger -t fips "$msg" 2>/dev/null || true + echo "$msg" + else + rm -f "$cfg.tmp" 2>/dev/null || true + fi + fi +} +fips_migrate_gateway_dns_listen + exit 0 diff --git a/src/bin/fips-gateway.rs b/src/bin/fips-gateway.rs index 589deb6e..3d2b784d 100644 --- a/src/bin/fips-gateway.rs +++ b/src/bin/fips-gateway.rs @@ -372,6 +372,11 @@ async fn main() { // Before the pool, NAT table and routes exist, so a port that is already // taken ends the gateway with nothing to tear down, and a service manager // restarting it does not churn nftables. + if gw_config.dns.is_mdns() { + warn!( + "gateway.dns.listen uses port 5353, the mDNS port; an mDNS responder (the fips daemon's LAN rendezvous, avahi) will conflict with it; the default is now [::1]:5365" + ); + } let dns_socket = match dns::bind_listener(gw_config.dns.listen()).await { Ok(socket) => socket, Err(e) => { diff --git a/src/config/gateway.rs b/src/config/gateway.rs index f7d4cf00..22c383d6 100644 --- a/src/config/gateway.rs +++ b/src/config/gateway.rs @@ -9,7 +9,10 @@ use serde::{Deserialize, Serialize}; /// Default gateway DNS listen address. /// -/// Loopback-only on the unprivileged port 5353. The canonical +/// Loopback-only on the unprivileged port 5365, which IANA leaves +/// unassigned and no common resolver uses. It is not 5353, the mDNS +/// port, which the daemon's LAN rendezvous, avahi-daemon and +/// systemd-resolved can hold. The canonical /// gateway deployment is a host already serving DHCP/DNS to a LAN /// segment (e.g., an OpenWrt AP), where port 53 is taken by the /// existing resolver and `.fips` queries are forwarded to the @@ -21,7 +24,7 @@ use serde::{Deserialize, Serialize}; /// explicit `::1` do not accept v4-mapped traffic. Forwarders that /// reach the gateway over IPv4 loopback (`127.0.0.1`) need to be /// pointed at an explicit IPv4 listen address instead. -const DEFAULT_DNS_LISTEN: &str = "[::1]:5353"; +const DEFAULT_DNS_LISTEN: &str = "[::1]:5365"; /// Default upstream DNS resolver (FIPS daemon). /// @@ -131,7 +134,7 @@ pub struct PortForward { /// Gateway DNS resolver configuration (`gateway.dns.*`). #[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct GatewayDnsConfig { - /// Listen address and port (default: `[::1]:5353`). + /// Listen address and port (default: `[::1]:5365`). #[serde(default, skip_serializing_if = "Option::is_none")] pub listen: Option, @@ -146,7 +149,7 @@ pub struct GatewayDnsConfig { } impl GatewayDnsConfig { - /// Get the listen address (default: `[::1]:5353`). + /// Get the listen address (default: `[::1]:5365`). pub fn listen(&self) -> &str { self.listen.as_deref().unwrap_or(DEFAULT_DNS_LISTEN) } @@ -166,6 +169,12 @@ impl GatewayDnsConfig { pub(crate) fn port_of(listen: &str) -> Option { listen.rsplit_once(':')?.1.parse().ok() } + + /// Whether the listen address is on the mDNS port, which an mDNS + /// responder can take from the gateway at any time. + pub fn is_mdns(&self) -> bool { + Self::port_of(self.listen()) == Some(5353) + } } /// Conntrack timeout overrides (`gateway.conntrack.*`). @@ -224,7 +233,7 @@ lan_interface: "eth0" assert!(!config.enabled); assert_eq!(config.pool, "fd01::/112"); assert_eq!(config.lan_interface, "eth0"); - assert_eq!(config.dns.listen(), "[::1]:5353"); + assert_eq!(config.dns.listen(), "[::1]:5365"); assert_eq!(config.dns.upstream(), "[::1]:5354"); assert_eq!(config.dns.ttl(), 60); assert_eq!(config.grace_period(), 60); @@ -232,6 +241,43 @@ lan_interface: "eth0" assert_eq!(config.conntrack.udp_timeout(), 30); } + #[test] + fn a_listen_on_5353_is_flagged_as_mdns() { + for listen in [ + "[::1]:5353", + "[::]:5353", + "127.0.0.1:5353", + "localhost:5353", + ] { + let dns = GatewayDnsConfig { + listen: Some(listen.to_string()), + ..Default::default() + }; + assert!(dns.is_mdns(), "{listen} must be flagged as the mDNS port"); + } + } + + #[test] + fn the_default_and_other_ports_are_not_flagged_as_mdns() { + assert!(!GatewayDnsConfig::default().is_mdns()); + for listen in [ + "[::1]:5365", + "[::]:53", + "192.168.1.1:53", + "[::]:5355", + "localhost", + ] { + let dns = GatewayDnsConfig { + listen: Some(listen.to_string()), + ..Default::default() + }; + assert!( + !dns.is_mdns(), + "{listen} must not be flagged as the mDNS port" + ); + } + } + #[test] fn test_gateway_config_custom() { let yaml = r#" diff --git a/testing/deb-install/test.sh b/testing/deb-install/test.sh index 9b46c0b4..a1419491 100755 --- a/testing/deb-install/test.sh +++ b/testing/deb-install/test.sh @@ -304,6 +304,42 @@ EOF return } +# The installed gateway, on the default config, serves .fips on its default +# listen address. Each check needs the gateway running: a gateway that exited +# fails the first one rather than letting the others pass on nothing. +# Args: , the daemon's npub to resolve through the gateway. +check_gateway_default_listener() { + local name="$1" npub="$2" + local journal="" _i + for _i in $(seq 1 5); do + journal=$(docker exec "$name" journalctl -u fips-gateway.service --no-pager 2>/dev/null) || journal="" + printf '%s\n' "$journal" | grep -q "fips-gateway running" && break + sleep 1 + done + if ! printf '%s\n' "$journal" | grep -q "fips-gateway running"; then + fail "the installed fips-gateway did not reach 'fips-gateway running', so its default listener was not observed" + echo " --- fips-gateway journal ---" + printf '%s\n' "$journal" | tail -15 + return + fi + + local sockets + sockets=$(docker exec "$name" ss -Hulnp 'sport = :5365' 2>/dev/null) || sockets="" + if printf '%s\n' "$sockets" | grep -F '[::1]:5365' | grep -q 'fips-gateway'; then + pass "fips-gateway listens on its default [::1]:5365" + else + fail "no fips-gateway socket on [::1]:5365: '$sockets'" + fi + + local answer + answer=$(docker exec "$name" dig +short +tries=1 +time=3 @::1 -p 5365 AAAA "${npub}.fips" 2>&1) + if printf '%s\n' "$answer" | grep -qE '^fd01::[0-9a-f]{1,4}$'; then + pass "the gateway answers ${npub}.fips on [::1]:5365 from its fd01::/112 pool" + else + fail "the gateway did not answer ${npub}.fips on [::1]:5365 from its pool: '$answer'" + fi +} + # Purge the package with the DNS routing file planted and fips-dns stopped, and # check that postrm removes the file and restarts systemd-resolved. # @@ -709,6 +745,8 @@ DOCKERFILE docker exec "$name" journalctl -u fips-gateway.service --no-pager 2>&1 | tail -15 fi + check_gateway_default_listener "$name" "$npub" + check_purge_clears_dns "$name" "$expected_backend" cleanup_container "$name" diff --git a/testing/dns-resolver/test.sh b/testing/dns-resolver/test.sh index 8aea1bb9..07f72df9 100755 --- a/testing/dns-resolver/test.sh +++ b/testing/dns-resolver/test.sh @@ -712,6 +712,47 @@ read_gateway_log() { return 0 } +# The gateway on its default config binds [::1]:5365 and gets past the +# bind to the NAT step, logging one of the two NAT lines whichever way NAT +# goes in this container. Those are the lines the held-port check requires +# to be absent, so this shows the gateway still emits them in that text. +check_gateway_default_bind() { + local name="$1" + local log=/var/log/fips-gateway.log + local text="" read_ok=0 listening=0 nat_step=0 _i + for _i in $(seq 1 5); do + if text=$(read_gateway_log "$name" "$log"); then + read_ok=1 + if printf '%s\n' "$text" | grep -q 'Gateway DNS resolver listening.*addr=\[::1\]:5365'; then + listening=1 + fi + if printf '%s\n' "$text" | grep -qE 'Created nftables table|Failed to create nftables table'; then + nat_step=1 + break + fi + fi + sleep 1 + done + if [ "$read_ok" = "0" ]; then + fail "could not read $log, so the gateway's default bind was not observed" + return + fi + if [ "$listening" = "1" ]; then + pass "fips-gateway listens on its default [::1]:5365" + else + fail "fips-gateway did not log listening on [::1]:5365" + fi + if [ "$nat_step" = "1" ]; then + pass "fips-gateway gets past the DNS bind to the NAT step" + else + fail "fips-gateway logged neither NAT-step line after the DNS bind" + fi + if [ "$listening" = "0" ] || [ "$nat_step" = "0" ]; then + echo " --- $log ---" + printf '%s\n' "$text" | tail -20 + fi +} + # The gateway exits at the DNS bind when its listen port is held. The # daemon in this container holds [::1]:5354, so a gateway configured to # listen there must exit non-zero with the hint naming the daemon, before @@ -980,9 +1021,10 @@ EOF' echo " --- fips-gateway log ---" docker exec "$name" tail -20 /var/log/fips-gateway.log 2>&1 || true fi - # Stop the gateway (it will likely have failed past the upstream - # check on something unrelated in this minimal container — we only - # care that the upstream reachability step succeeded). + check_gateway_default_bind "$name" + + # Stop the gateway (it may have failed after the DNS bind on something + # unrelated in this minimal container). docker exec "$name" pkill -f fips-gateway 2>/dev/null || true check_gateway_exits_on_held_port "$name" diff --git a/testing/openwrt/fixtures/released-fips.yaml b/testing/openwrt/fixtures/released-fips.yaml new file mode 100644 index 00000000..2f15160d --- /dev/null +++ b/testing/openwrt/fixtures/released-fips.yaml @@ -0,0 +1,190 @@ +# FIPS Node Configuration + +node: + identity: + # By default, a new ephemeral keypair is generated on each start. + # Uncomment persistent to keep the same identity across restarts; + # on first start a keypair is saved to fips.key/fips.pub next to + # this config file (mode 0600/0644). + # persistent: true + # + # Or set an explicit key (overrides persistent): + # nsec: "nsec1..." + # Mesh-lookup protocol (node.lookup.*): the overlay coordinate-lookup engine + # (mesh address -> coordinates). Defaults shown; uncomment to override. + # lookup: + # ttl: 64 + # attempt_timeouts_secs: [1, 2, 4, 8] + # recent_expiry_secs: 10 + # backoff_base_secs: 0 + # backoff_max_secs: 0 + # forward_min_interval_secs: 2 + rendezvous: + # Optional Nostr-mediated overlay endpoint rendezvous. + # nostr: + # enabled: true + # policy: configured_only # disabled | configured_only | open + # open_discovery_max_pending: 64 # caps queued open-rendezvous retries + # app: "fips-overlay-v1" + # advertise: true + # advert_relays: + # - "wss://relay.damus.io" + # - "wss://nos.lol" + # - "wss://offchain.pub" + # dm_relays: + # - "wss://relay.damus.io" + # - "wss://nos.lol" + # - "wss://offchain.pub" + # # Optional override. If omitted, FIPS uses the built-in STUN list. + # # Built-in relay/STUN defaults are best-effort and should be + # # overridden by operators for production use. + # stun_servers: + # - "stun:stun.l.google.com:19302" + # - "stun:stun.cloudflare.com:3478" + # - "stun:global.stun.twilio.com:3478" + + # mDNS/DNS-SD peer rendezvous on the local link. Ships commented (the + # daemon default is off); 'fips-ap-setup' uncomments it when creating + # the access SSID — phone FIPS apps cannot see raw-Ethernet beacons, + # so mDNS is how they find this router's daemon. Daemon-wide switch, + # left enabled on 'fips-ap-setup remove'. + # lan: + # enabled: true + +tun: + enabled: true + name: fips0 + mtu: 1280 + +dns: + enabled: true + # bind_addr defaults to "::1" (IPv6 loopback). The shipped + # fips-dns-setup script configures systemd-resolved with a global + # /etc/systemd/resolved.conf.d/fips.conf drop-in pointing at + # [::1]:5354. + # + # Set "::" to expose the responder to mesh peers as well (e.g. for + # gateway hosts that resolve .fips on behalf of LAN clients). The + # mesh-interface filter in src/upper/dns.rs will still defend + # /etc/fips/hosts aliases from cross-mesh enumeration. + # bind_addr: "::1" + port: 5354 + +transports: + udp: + # Dual-stack wildcard, not "0.0.0.0": access-SSID clients (phones) learn + # this node's addresses from the mDNS advert and prefer the IPv6 + # link-local — a v4-only bind silently drops their Noise msg1. + # OpenWrt is Linux (bindv6only=0), so "[::]" accepts v4 too. + bind_addr: "[::]:2121" + # advertise_on_nostr: true + # public: false # false => advertise udp:nat; true => advertise bound host:port + # accept_connections: true # default; refuse inbound msg1 when false + # outbound_only: false # true => bind ephemeral, no listener on a + # # known port. Forces advertise_on_nostr=false + # # and accept_connections=false. Pure-client + # # posture; bind_addr is ignored. + + tcp: + # Accepts inbound connections. No static outbound peers. + bind_addr: "0.0.0.0:8443" + # advertise_on_nostr: true + + # Ethernet transport — physical port names, NOT bridge names. + # Run 'ip link show' on the router to identify port names. + ethernet: + wan: + interface: "eth0" + listen: true + announce: true + auto_connect: true + accept_connections: true + wwan: + interface: "phy0-sta0" + listen: true + announce: true + auto_connect: true + accept_connections: true + lan: + interface: "br-lan" + listen: true + announce: true + auto_connect: true + accept_connections: true + + # 802.11s mesh backhaul between FIPS routers. These entries ship + # commented out so a stock install that never creates fips-mesh* + # logs no per-boot "interface missing" bind warning. Running + # 'fips-mesh-setup ' creates the interface AND uncomments the + # matching block here (once per radio; radio0 -> fips-mesh0, radio1 -> + # fips-mesh1); 'fips-mesh-setup remove' re-comments it. Restart fips + # after — a transport whose interface is missing at startup is skipped, + # not retried. Dual-band routers can mesh on both bands at once — + # failover, not multipath: FIPS keeps one active link per peer, the + # other band stands by. The mesh runs OPEN (no SAE) with 802.11s + # forwarding off: FIPS's Noise handshake is the encryption and + # authentication, and FIPS is the routing layer. See + # docs/how-to/set-up-80211s-mesh-backhaul.md. + # mesh0: + # interface: "fips-mesh0" + # listen: true + # announce: true + # auto_connect: true + # accept_connections: true + # mesh1: + # interface: "fips-mesh1" + # listen: true + # announce: true + # auto_connect: true + # accept_connections: true + + # Open "!FIPS" access SSID for phones and laptops running FIPS. These + # entries ship commented out so a stock install that never creates + # fips-ap* logs no per-boot "interface missing" bind warning. Running + # 'fips-ap-setup ' creates the interface AND uncomments the + # matching block here (once per radio; radio0 -> fips-ap0, radio1 -> + # fips-ap1); 'fips-ap-setup remove' re-comments it. Restart fips after + # — a transport whose interface is missing at startup is skipped, not + # retried. The SSID is OPEN and isolated on purpose: FIPS's Noise + # handshake is the only security layer, and associated clients reach + # nothing but the FIPS handshake surface. See + # docs/how-to/set-up-open-access-ssid.md. + # ap0: + # interface: "fips-ap0" + # listen: true + # announce: true + # auto_connect: true + # accept_connections: true + # ap1: + # interface: "fips-ap1" + # listen: true + # announce: true + # auto_connect: true + # accept_connections: true + + # No BLE transport: OpenWrt builds target musl, which has no BlueZ backend. + +# Outbound LAN gateway. dnsmasq forwards .fips queries to listen=[::1]:5353 +# while it runs (configured by the fips-gateway init script). Requires IPv6 +# forwarding enabled. +gateway: + enabled: true + pool: "fd01::/112" + lan_interface: "br-lan" + dns: + listen: "[::1]:5353" + upstream: "[::1]:5354" + ttl: 60 + pool_grace_period: 60 + +peers: [] + # Static peers for bootstrapping (UDP or TCP): + # - npub: "npub1qmc3cvfz0yu2hx96nq3gp55zdan2qclealn7xshgr448d3nh6lks7zel98" + # alias: "gateway" + # via_nostr: true + # addresses: + # - transport: udp + # addr: "test-us01.fips.network:2121" # IP or hostname (e.g., "peer.example.com:2121") + # - transport: udp + # addr: "nat" # Use node.rendezvous.nostr for Nostr/STUN hole punching + # connect_policy: auto_connect diff --git a/testing/openwrt/scenarios.sh b/testing/openwrt/scenarios.sh index 6c7f4007..fe78548f 100755 --- a/testing/openwrt/scenarios.sh +++ b/testing/openwrt/scenarios.sh @@ -28,9 +28,22 @@ RELEASED_PRERM="$REPO/testing/openwrt/fixtures/released-prerm" INIT_GATEWAY="$REPO/packaging/openwrt-ipk/files/etc/init.d/fips-gateway" APK_SCRIPTS="${APK_SCRIPTS:-}" SHIPPED_YAML="$REPO/packaging/openwrt-ipk/files/etc/fips/fips.yaml" +# The fips.yaml every release up to 0.5.1 shipped, from before the gateway's +# default DNS port moved. +RELEASED_YAML="$REPO/testing/openwrt/fixtures/released-fips.yaml" +GATEWAY_RS="$REPO/src/config/gateway.rs" +SETUP_SCRIPT="$REPO/packaging/openwrt-ipk/files/etc/uci-defaults/90-fips-setup" +LEGACY_LISTEN=' listen: "[::1]:5353"' +SHIPPED_LISTEN=' # listen: "[::1]:5365" # the default; the init script points dnsmasq at this port' +MIGRATED_MSG='fips: moved gateway.dns.listen off the mDNS port 5353 to the default [::1]:5365' WORK=/tmp/fips-openwrt-scenarios UPGRADE_MARKER=/tmp/fips-prerm-upgrade +# The uci stub's state and the directory holding the executable stubs. Fixed +# paths, because the .apk scripts run with only PATH in their environment. +UCI_DIR=/tmp/fips-openwrt-uci +STUB_BIN=/tmp/fips-openwrt-bin +DNSMASQ_OPT='dhcp.@dnsmasq[0].server' FAILURES=0 CASES=0 @@ -124,6 +137,27 @@ assert_not_called() { return 0 } +assert_none_called_with_prefix() { + # assert_none_called_with_prefix + # Fails if any recorded call begins with , whatever follows it. + if [ ! -r "$CALLS" ]; then + bad "$2 — the call log $CALLS cannot be read" + return 0 + fi + matched="" + while IFS= read -r line; do + case "$line" in + "$1"*) matched="$matched$line;" ;; + esac + done < "$CALLS" + if [ -n "$matched" ]; then + bad "$2 — called: $matched" + else + ok "$2" + fi + return 0 +} + assert_file_is() { # assert_file_is got="$(cat "$1" 2>/dev/null)" @@ -182,6 +216,76 @@ assert_order() { return 0 } +# Install an executable uci that keeps each option as a file of values, one per +# line, under $UCI_DIR. Like the real uci, "get" prints a list on one line +# separated by single spaces and fails for an option with no values, and +# del_list removes every copy of the value. Other commands are only logged. +install_uci_stub() { + rm -rf "$UCI_DIR" + mkdir -p "$UCI_DIR" "$STUB_BIN" + { + echo '#!/bin/sh' + echo "dir=$UCI_DIR" + cat <<'STUB' +[ "${1:-}" = "-q" ] && shift +cmd="${1:-}" +[ $# -gt 0 ] && shift +echo "uci $cmd $*" >> "$dir/log" +file_of() { + printf '%s/%s' "$dir" "$(printf '%s' "$1" | sed 's/[^A-Za-z0-9._-]/_/g')" +} +case "$cmd" in +get) + f="$(file_of "$1")" + [ -s "$f" ] || exit 1 + tr '\n' ' ' < "$f" | sed 's/ $//' + echo + ;; +add_list) + echo "${1#*=}" >> "$(file_of "${1%%=*}")" + ;; +del_list) + f="$(file_of "${1%%=*}")" + if [ -f "$f" ]; then + grep -vxF -- "${1#*=}" "$f" > "$f.new" + mv "$f.new" "$f" + fi + ;; +esac +exit 0 +STUB + } > "$STUB_BIN/uci" + chmod 0755 "$STUB_BIN/uci" + + cat > /etc/init.d/dnsmasq <<'STUB' +#!/bin/sh +echo "dnsmasq $1" >> "$CALLS" +STUB + chmod 0755 /etc/init.d/dnsmasq + return 0 +} + +uci_seed() { + # uci_seed