diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a8c4817e..5481ea77 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -79,6 +79,17 @@ jobs: run: bash testing/check-comment-refs.sh - name: Check no non-test code uses std 64-bit atomics run: python3 testing/check-portable-atomics.py + # The OpenWrt Package workflow runs this too, but only on trunk pushes, + # tags and pull requests; here a branch push sees a finding first. + # Kept in step with ci-local.sh's run_shellcheck by hand. + - name: Install shellcheck (if missing) + run: | + if ! command -v shellcheck >/dev/null 2>&1; then + sudo apt-get update && sudo apt-get install -y --no-install-recommends shellcheck + fi + shellcheck --version + - name: Check the OpenWrt package's shell scripts with shellcheck + run: bash testing/check-shellcheck.sh # Hermetic: synthetic ping functions, no containers, ~45s. Lives beside # the other two so both runners gate on it identically — putting it in # only one would create exactly the drift check-ci-parity.sh exists to @@ -86,15 +97,20 @@ jobs: # a matrix suite. - name: Run convergence-gate unit tests run: bash testing/lib/wait-converge-test.sh - # Runs package-linux.yml's own version derivation on a release tag, a - # candidate tag and a branch. Not a matrix suite either, so it is kept in - # step with ci-local.sh's run_deb_version by hand. - - name: Check the Debian version derived for tags and branches - run: bash testing/check-deb-version.sh + # Runs the packaging workflows' own version derivations on a release + # tag, a candidate tag and a branch. Not a matrix suite either, so it is + # kept in step with ci-local.sh's run_package_versions by hand. + - name: Check the package versions derived for tags and branches + run: bash testing/check-package-versions.sh # The unit-test jobs' flaky-test reporter, against recorded nextest # reports. Kept in step with ci-local.sh's run_nextest_flaky by hand. - name: Check the flaky-test reporter against its fixtures run: bash testing/nextest-flaky/test.sh + # The glibc floor check's cases, built from the host's own true + # executable. Not a matrix suite, so it is kept in step with + # ci-local.sh's run_glibc_floor by hand. + - name: Check the glibc floor check against its cases + run: bash testing/glibc-floor/test.sh fmt: name: Format check @@ -342,7 +358,9 @@ jobs: ${{ runner.os }}-cargo- - name: Install cargo-nextest - uses: taiki-e/install-action@nextest + uses: taiki-e/install-action@fcf5432d9f50d67e37ee6e29bdb7a224ff67b4a7 # v2 + with: + tool: nextest # The cache restores target/, including the previous run's report. Remove # it so the flaky-test check below reads only this run's, and reports a @@ -455,7 +473,9 @@ jobs: ${{ runner.os }}-cargo- - name: Install cargo-nextest - uses: taiki-e/install-action@nextest + uses: taiki-e/install-action@fcf5432d9f50d67e37ee6e29bdb7a224ff67b4a7 # v2 + with: + tool: nextest # The Darwin half of the address-less presence contract. The Linux legs # pin that `getifaddrs` reports an interface with no addresses as @@ -611,7 +631,9 @@ jobs: ${{ runner.os }}-cargo- - name: Install cargo-nextest - uses: taiki-e/install-action@nextest + uses: taiki-e/install-action@fcf5432d9f50d67e37ee6e29bdb7a224ff67b4a7 # v2 + with: + tool: nextest # The cache restores target/, including the previous run's report. Remove # it so the flaky-test check below reads only this run's, and reports a diff --git a/.github/workflows/package-linux.yml b/.github/workflows/package-linux.yml index d9f20e17..a5fad5d8 100644 --- a/.github/workflows/package-linux.yml +++ b/.github/workflows/package-linux.yml @@ -46,7 +46,7 @@ jobs: # dpkg reads X.Y.Z-rcN as revision rcN of X.Y.Z and sorts it above # the release; X.Y.Z~rcN sorts below it. git refuses '~' in a ref # name, so the tag carries '-' and this maps it, for the .deb only. - # testing/check-deb-version.sh runs this step's text. + # testing/check-package-versions.sh runs this step's text. DEB_VERSION="$VERSION" if [[ "$GITHUB_REF" == refs/tags/* ]] \ && [[ "$VERSION" =~ ^([0-9]+\.[0-9]+\.[0-9]+)-((alpha|beta|pre|rc)[0-9]*)$ ]]; then diff --git a/.github/workflows/package-openwrt.yml b/.github/workflows/package-openwrt.yml index c9945dd6..ee26a6ae 100644 --- a/.github/workflows/package-openwrt.yml +++ b/.github/workflows/package-openwrt.yml @@ -25,6 +25,7 @@ jobs: outputs: package_version: ${{ steps.version.outputs.package_version }} apk_version: ${{ steps.version.outputs.apk_version }} + ipk_version: ${{ steps.version.outputs.ipk_version }} release_channel: ${{ steps.channel.outputs.release_channel }} steps: - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 @@ -41,15 +42,28 @@ jobs: # in the .apk metadata. apk_version is built directly from the same # structured inputs (tag, or commit height) — no reparse of the # flattened package_version. See packaging/openwrt-apk/apk-version.sh. + # + # ipk_version is the .ipk control Version. opkg compares versions as + # dpkg does, so it reads a tag's vX.Y.Z-rcN as revision rcN and sorts + # it above the release; vX.Y.Z~rcN sorts below it. git refuses '~' in + # a ref name, so the tag carries '-' and this maps it. The leading 'v' + # stays: every released .ipk carries it, and under opkg 0.5.4 sorts + # below v0.5.3. testing/check-package-versions.sh runs this step's text. if [[ "$GITHUB_REF" == refs/tags/* ]]; then echo "package_version=${GITHUB_REF_NAME}" >> "$GITHUB_OUTPUT" echo "apk_version=$(sh packaging/openwrt-apk/apk-version.sh tag "${GITHUB_REF_NAME}")" >> "$GITHUB_OUTPUT" + IPK_VERSION="${GITHUB_REF_NAME}" + if [[ "$GITHUB_REF_NAME" =~ ^(v[0-9]+\.[0-9]+\.[0-9]+)-((alpha|beta|pre|rc)[0-9]*)$ ]]; then + IPK_VERSION="${BASH_REMATCH[1]}~${BASH_REMATCH[2]}" + fi + echo "ipk_version=${IPK_VERSION}" >> "$GITHUB_OUTPUT" else BRANCH=$(echo "$GITHUB_REF_NAME" | sed 's|/|-|g') HEIGHT=$(git rev-list --count HEAD) HASH=$(git rev-parse --short HEAD) echo "package_version=${BRANCH}.${HEIGHT}.${HASH}" >> "$GITHUB_OUTPUT" echo "apk_version=$(sh packaging/openwrt-apk/apk-version.sh dev "${HEIGHT}")" >> "$GITHUB_OUTPUT" + echo "ipk_version=${BRANCH}.${HEIGHT}.${HASH}" >> "$GITHUB_OUTPUT" fi - name: Determine release channel @@ -120,8 +134,9 @@ jobs: - name: Install Rust toolchain (nightly, Tier 3) if: matrix.rust_channel == 'nightly' - uses: dtolnay/rust-toolchain@nightly + uses: dtolnay/rust-toolchain@be39649afda95dbf70f87cce95f68b8d5797b296 # nightly with: + toolchain: nightly components: rust-src - name: Cache Cargo registry + build @@ -331,6 +346,7 @@ jobs: - name: Build .ipk env: PKG_VERSION: ${{ needs.determine-versioning.outputs.package_version }} + IPK_VERSION: ${{ needs.determine-versioning.outputs.ipk_version }} run: ./packaging/openwrt-ipk/build-ipk.sh --arch ${{ matrix.build_arch }} --bin-dir "$GITHUB_WORKSPACE/bins" - name: Install shellcheck (if missing) @@ -341,52 +357,12 @@ jobs: fi shellcheck --version - # Its own step, and its own shell dialect. The shipped-scripts lint below - # runs --shell=sh with the OpenWrt rc.common exclude set, which misfires - # on a bash script; install-nak.sh is also not shipped in the package. - - name: Lint install-nak.sh + # The scripts this package ships, as sh, and install-nak.sh, as bash. + # The guard is the one copy of the lint; ci.yml and ci-local.sh run it + # too, so a branch push sees a finding before it reaches a trunk. + - name: Lint shell scripts shell: bash - run: shellcheck --shell=bash .github/scripts/install-nak.sh - - - name: Lint shipped shell scripts - shell: bash - run: | - set -euo pipefail - FILES_DIR=packaging/openwrt-ipk/files - # Scripts shipped inside the .ipk. The init scripts use the OpenWrt - # `#!/bin/sh /etc/rc.common` shebang; tell shellcheck to treat them - # as POSIX sh and silence the unrecognized-shebang warning (SC1008). - # SC2317 (unreachable command) fires on rc.common's externally-invoked - # start_service/stop_service/reload_service hooks. - TARGETS=( - "$FILES_DIR/etc/init.d/fips" - "$FILES_DIR/etc/init.d/fips-gateway" - "$FILES_DIR/etc/fips/firewall.sh" - "$FILES_DIR/etc/hotplug.d/net/99-fips" - "$FILES_DIR/etc/uci-defaults/90-fips-setup" - "$FILES_DIR/usr/bin/fips-mesh-setup" - "$FILES_DIR/usr/bin/fips-ap-setup" - ) - fail=0 - for f in "${TARGETS[@]}"; do - if [ ! -f "$f" ]; then - echo "FAIL: missing $f" - fail=1 - continue - fi - echo "==> shellcheck $f" - if shellcheck --shell=sh --exclude=SC1008,SC2317,SC2034,SC3043,SC2086,SC2089,SC2090 "$f"; then - echo " PASS" - else - echo " FAIL" - fail=1 - fi - done - if [ "$fail" -ne 0 ]; then - echo "shellcheck FAILED" - exit 1 - fi - echo "shellcheck PASS (${#TARGETS[@]} scripts)" + run: bash testing/check-shellcheck.sh - name: Sysctl drop-in syntax check shell: bash diff --git a/CHANGELOG.md b/CHANGELOG.md index 33bf9156..75e5d490 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -355,6 +355,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 the release. The tarball, artifact and `.deb` file names keep the tag's `-rcN`. +#### Native datagram API + +- An accepted native API flow that its program closes without ever sending on + now stays open, holding its port and a slot against + `node.native_api.max_flows`, until its listener is closed. + +#### OpenWrt + +- Release-candidate OpenWrt `.ipk` packages are now versioned `vX.Y.Z~rcN`, so + opkg sorts them below the final release and the release upgrades a router + that ran the candidate. The package file name keeps the tag's `-rcN`. + #### Windows - `install-service.ps1` stops when `\etc\fips\fips.key` exists on the system @@ -543,6 +555,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 through `Requires=fips.service`, then restarted only fips, so `.fips` resolution stayed down and the gateway stayed stopped until started by hand. +#### macOS + +- If an encrypt worker thread exits, the daemon no longer stops once that + worker's send queue fills. Packets for that worker are now dropped instead + of blocking forever. This applies to the default sender. + +#### Native datagram API + +- On macOS, a flow accepted through the native API could arrive already + closed, losing the peer's first datagram, when the kernel's descriptor + garbage collector ran before the client read the arrival message. The same + exposure on connect and listen replies is closed too. + #### OpenWrt - dnsmasq forwards `.fips` to fips-gateway only while the gateway is @@ -555,6 +580,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 stopped, keeps the gateway's state across an upgrade, and keeps an edited `/etc/fips/fips.yaml`. The SDK's package scan also no longer stops on the `Makefile`'s architecture check, which had kept the package out of the build. +- The shipped config and README now agree that the LAN Ethernet transport + binds the LAN bridge (`br-lan`). A socket on a bridge member port does not + receive FIPS frames, whether or not br_netfilter is loaded. If you followed + the earlier README and changed the `lan` entry in `/etc/fips/fips.yaml` to a + member port (for example `lan1` or `eth1`), change it back to `br-lan`. Your + edited config is kept across upgrades, along with its old "physical port + names, NOT bridge names" comment, so an upgrade alone will not correct it. #### Sessions and rekey @@ -596,6 +628,32 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Security +#### Links and transports + +- An inbound TCP or Tor onion connection is now dropped when it goes longer + than the node's own link-liveness bound without delivering a complete frame. + The bound is about 64 seconds at default settings. A remote that sends one + frame and then goes silent no longer holds an inbound connection slot + indefinitely. The bound follows `node.heartbeat_interval_secs`, + `node.link_dead_timeout_secs`, `node.tick_interval_secs` and the handshake + resend settings. + +#### Routing and discovery + +- A single forged or reflected `PathBroken` signal no longer deletes + coordinates a node verified by lookup. A verified entry is kept while a + fresh lookup re-validates it. It is demoted to an unverified hint only when + `PathBroken` signals naming the destination arrive over two different links + within 15 seconds. The vote is the authenticated link peer the signal + arrived over, not the reporter it names, so a sender on one link cannot + reach the quorum by inventing reporters. Forged reports that arrive over two + different links still demote the entry. The re-lookup now runs on every such + signal, and when the destination's identity is not cached the node first + caches it from the session's key so the answer can be verified. New + error-signal counters `broken_below_quorum`, `broken_demoted`, + `broken_link_mismatch` and `broken_reporter_mismatch` appear in + `show_routing`; the last two only count and never refuse a signal. + #### Sessions and rekey - A copy of a peer's link rekey msg1 can no longer stop link key rotation. A @@ -617,6 +675,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 address arrives. A msg1 from a node this one holds no link with, or one that carries a different startup epoch, still starts a new link and is answered at its source, as any new connection is. +- The SHA-256 and HMAC states used by the Noise handshake are now cleared when + dropped. The connection's handshake slot is cleared when a handshake + completes, and its session slot when a rekey session is taken out. The + handshake keypair is erased in place on every early return from starting a + handshake. The security reference now states what clearing key material in + memory does and does not cover in a release build, including copies left + behind by moves and the identity loaded from a secret string. ## [0.5.2] - 2026-09-28 diff --git a/Cargo.lock b/Cargo.lock index 9aa7807d..a25698cb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -333,6 +333,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d2f6c7dbe95a6ed67ad9f18e57daf93a2f034c524b99fd2b76d18fdfeb6660aa" dependencies = [ "hybrid-array", + "zeroize", ] [[package]] @@ -1028,6 +1029,7 @@ dependencies = [ "const-oid", "crypto-common 0.2.2", "ctutils", + "zeroize", ] [[package]] @@ -1175,6 +1177,7 @@ dependencies = [ "futures", "hex", "hkdf", + "hmac 0.13.0", "libc", "libm", "mdns-sd", diff --git a/Cargo.toml b/Cargo.toml index dd4d8a53..996c4064 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,8 +22,12 @@ profiling = [] [dependencies] ratatui = "0.30" secp256k1 = { version = "0.30", features = ["rand", "global-context"] } -sha2 = "0.11" +# The `zeroize` features clear the SHA-256 and HMAC states on drop. `hmac`'s +# feature only forwards to `digest`'s, which `sha2`'s also turns on; it is +# listed so the HMAC inside `hkdf` keeps it if `sha2`'s ever stops doing so. +sha2 = { version = "0.11", features = ["zeroize"] } hkdf = "0.13" +hmac = { version = "0.13", features = ["zeroize"] } ring = "0.17" libm = "0.2" zeroize = { version = "1.9", features = ["zeroize_derive"] } diff --git a/docs/design/fips-mesh-operation.md b/docs/design/fips-mesh-operation.md index 6ecbf9b0..5d92cd78 100644 --- a/docs/design/fips-mesh-operation.md +++ b/docs/design/fips-mesh-operation.md @@ -272,18 +272,28 @@ follows the same path as the request. Greedy tree routing toward the `origin_coords` is used only as a fallback if the reverse-path entry has expired. -**Originator check first**: a node tests its own outstanding lookups before -consulting `recent_requests`. A request is flooded to every bloom-matching -tree peer, and a bloom false positive can send a copy out into the wider -network and back to the originator, which would otherwise file its own -`request_id` as an ordinary transit entry and relay its own answer away. The -originator arm therefore wins: a response naming a target with a lookup +**Originator check**: on the response path, a node tests its own outstanding +lookups before consulting `recent_requests`. A request is flooded to every +bloom-matching tree peer, and a bloom false positive can send a copy out into +the wider network and back to the originator, which would otherwise file its +own `request_id` as an ordinary transit entry and relay its own answer away. +The originator arm therefore wins: a response naming a target with a lookup outstanding, carrying a `request_id` that lookup issued, is accepted here -whatever the dedup cache holds. A returning copy of the request is likewise -dropped rather than recorded, so the originator's id never enters the transit -cache. The drop is counted as `req_own_loopback` rather than as a duplicate: a -returning copy has a nonzero floor in healthy operation and says nothing about -the peer that delivered it. +whatever the dedup cache holds. A returning copy of the request is dropped +rather than recorded, so while the check recognises the id, it never enters the +transit cache. The drop is counted as `req_own_loopback` rather than as a +duplicate, because the cause differs: the node's own fan-out returning, not a +request id it has already recorded arriving again. Neither counter identifies +the peer that delivered the copy. The request path runs the two tests the other +way round, the dedup test first, and the order is immaterial there: an id the +originator check recognises is never in `recent_requests`, because the +originator records the ids it issues only in its pending lookups and a request +is recorded only after it has passed the check. Separately, the check +recognises an id only while its lookup is outstanding and the id is among the +last `MAX_RECORDED_IDS` (eight) that the lookup's retry ladder issued for that +target. A copy that returns after the lookup completed or timed out, or a copy +of an older attempt on a longer ladder, is recorded and forwarded as transit, +and a later copy of it is dropped as a duplicate. **Response-forwarded flag**: Each `recent_requests` entry tracks whether a response has already been forwarded for that `request_id`. If a second @@ -372,10 +382,27 @@ source. 1. Immediately send a standalone CoordsWarmup (0x14) message (rate-limited, same per-destination interval as CoordsRequired response) -2. Remove stale coordinates from cache -3. Initiate discovery for the destination +2. Handle the cached coordinates by where they came from. Unverified + coordinates (a hint copied off a passing packet, or a lookup result + whose verification has aged out) are removed. Coordinates a lookup + verified are kept while discovery re-validates them, because the signal + is unauthenticated and removing them would let the next forged warm + replace them. They are demoted to an unverified hint, keeping their value, + only when PathBroken signals naming the destination arrive over two + different links within 15 seconds. The vote is the authenticated link + peer, not the reporter the signal names, which the sender chooses. A node + whose signals all arrive over one link never demotes this way; its + verified coordinates last until discovery replaces them or their + verification ages out after 300 seconds. +3. Initiate discovery for the destination. If its identity is not cached, + cache it first from the session's key, so the response can be verified 4. Reset CP warmup counter +The source also counts, without refusing anything, a PathBroken that +arrives over a link other than its forward link to the destination, and one +whose reporter is not closer to the destination than the source is. Both +have a non-zero healthy floor. + ### MtuExceeded **Trigger**: A transit node receives a SessionDatagram but the total diff --git a/docs/design/fips-native-api.md b/docs/design/fips-native-api.md index 26ce5ea6..c16fcc55 100644 --- a/docs/design/fips-native-api.md +++ b/docs/design/fips-native-api.md @@ -133,9 +133,12 @@ and silent drops on its listeners. **Not a connection in the TCP sense.** A successful `connect` is a local registration and contacts no peer. There is no handshake, no keepalive and no notification that a peer went away. A flow ends when its descriptor closes, and -in no other way. In particular **a peer cannot end your flow: it has no close to -send.** That single fact shapes every program written against this interface, -and the consequences are drawn out in +in no other way. The one delay is an accepted flow never sent on, which ends +only once its listener has closed as well (see +[../reference/native-api.md](../reference/native-api.md#fipslistener)). In +particular **a peer cannot end your flow: it has no close to send.** That +single fact shapes every program written against this interface, and the +consequences are drawn out in [../how-to/use-the-native-datagram-api.md](../how-to/use-the-native-datagram-api.md#four-things-that-will-bite-you). ## See also diff --git a/docs/how-to/use-the-native-datagram-api.md b/docs/how-to/use-the-native-datagram-api.md index 355ba093..b7a19493 100644 --- a/docs/how-to/use-the-native-datagram-api.md +++ b/docs/how-to/use-the-native-datagram-api.md @@ -227,6 +227,16 @@ leaves the flows already accepted from it untouched. A program that parks streams in a `Vec` and never removes them holds ports and flow slots exactly as if it had leaked descriptors. +**An accepted flow you drop without ever sending on stays open until you +drop its listener.** Until your program has sent on an accepted flow, the +daemon keeps its own copy of the flow's descriptor, because on macOS the +kernel can otherwise destroy the flow while its descriptor is still on the +way to you. Your first `send` on the flow, or dropping the listener, lets +that copy go. So a server that refuses flows by dropping them unanswered +holds a port and a flow slot for each one until its listener goes, and a +long-lived listener that refuses many flows can walk the node into its flow +ceiling. + **Nothing peer-driven ever ends a flow, so your program has to.** The v1 wire carries no half-close. Nothing closes the daemon's half of a live accepted flow, so a loop written as "echo until the flow closes", or one diff --git a/docs/how-to/write-a-native-api-client.md b/docs/how-to/write-a-native-api-client.md index a128874a..a45c96ad 100644 --- a/docs/how-to/write-a-native-api-client.md +++ b/docs/how-to/write-a-native-api-client.md @@ -124,7 +124,7 @@ one producer on the surface, which is what lets a caller read it. ## Step 6: Keep descriptor hygiene -Five rules. Each one leaks a flow or loses one when broken. +Six rules. Each one leaks a flow or loses one when broken. **Request close-on-exec** with `MSG_CMSG_CLOEXEC` on the `recvmsg`, rather than setting it afterwards. Without it the descriptor survives an `exec` into a @@ -140,7 +140,15 @@ descriptors rather than dropping them on the floor. **Lift the descriptor out of an arrival you cannot parse** before discarding the message. Refusing a flow is closing its descriptor; discarding the message -without taking it leaks the flow instead. +without taking it leaks the flow instead. A refused flow ends only when the +listener closes, though, unless you wrote on it first: the daemon keeps its +own copy of an accepted flow's descriptor until your first write or the +listener's close. + +**Close the setup connection once you have the reply.** The daemon keeps its +own copy of the descriptor in its last reply until your next command on that +connection or the connection's close. A flow or listener you close while the +connection sits idle stays open until one of those happens. **Bound the partial line.** A daemon that stopped sending newlines would otherwise grow your buffer without end. The shipped client caps it at 64 KiB, diff --git a/docs/reference/native-api.md b/docs/reference/native-api.md index cea846d8..a6a21501 100644 --- a/docs/reference/native-api.md +++ b/docs/reference/native-api.md @@ -195,7 +195,10 @@ That is what lets the blanket reference implementation cover `&str` and One datagram flow, and the descriptor it rides on. A flow is an exact match of both ends and both ports. **The descriptor is the flow**: it lives while a -process holds that descriptor and ends when the last one closes it. +process holds that descriptor and ends when the last one closes it. The one +exception is an accepted flow that has never been sent on: the daemon keeps +its own copy of that descriptor until the first `send` or until the listener +is dropped. See `accept` below. `Send + Sync + 'static`, with no `Arc` and no borrow. There is **no `Clone` and no `try_clone`**. Because `send` and `recv` both take `&self`, a shared borrow @@ -340,6 +343,15 @@ no other way to refuse one. An unparseable arrival is therefore reported only after the descriptor it carried has been taken into ownership, so a parse failure refuses the flow rather than leaking it. +**A refused flow stays open until its listener is dropped**, unless the +program sent on it first. Until the first `send` on an accepted flow, the +daemon keeps its own copy of the flow's descriptor, because on macOS the +kernel can otherwise destroy a socket whose descriptor is still in an unread +arrival. That copy goes at the first `send`, when the listener is dropped, or +when the flow ends any other way, and the flow then ends with the program's +own close. Until then a dropped flow holds its port and its slot against +`max_flows`. + **`incoming()`** returns an `Incoming<'_>`, which borrows the listener for the iterator's lifetime, so the listener cannot be moved or dropped mid-iteration. @@ -360,7 +372,8 @@ none either. A bounded accept is `set_nonblocking` plus a wait of the caller's own on the descriptor. **Dropping** closes the descriptor and unbinds the port. Flows already accepted -from it are untouched; flows still pending on it go with it. +from it and still held are untouched; flows still pending on it go with it, and +so do flows accepted from it and dropped without ever being sent on. ### Incoming @@ -613,6 +626,20 @@ own half non-blocking and leaves the client's half blocking. `SOCK_SEQPACKET` is what preserves message boundaries in both directions, which is why the payload needs no framing. +**The daemon keeps a copy of the client's half after sending it.** While a +descriptor sits unread in a message, the message can be its only reference, +and the macOS kernel's descriptor collector destroys a socket in that state: +the client then receives a flow that reads as end of file with its datagrams +gone. So the daemon keeps its copy until one of these: + +- For a `connect` or `listen` reply, at the client's next command on the same + connection, or when that connection closes. +- For an arrival on a listener, at the client's first write on the flow, when + the listener closes, or when the flow ends any other way. + +A flow or listener the client closes before then ends when the daemon's copy +goes, not at the client's close. + A refused `connect` leaves the port free: the socket pair is built before the port is claimed, so a failure to build it needs no rollback. @@ -626,7 +653,11 @@ returning. **The connection owns nothing.** Closing it releases no flow and no listener, and a descriptor kept across the close keeps working. What owns the flow is the -descriptor. +descriptor. The connection does delay one thing: a descriptor from its last +reply that the client closes while the connection is still open, with no +further command sent, stays open until the next command or the connection's +close (see Passing the descriptor). The shipped client closes the connection +as soon as it has the reply, so it never meets this. ## Command reference diff --git a/docs/reference/security.md b/docs/reference/security.md index b69b5ea7..71a2b13f 100644 --- a/docs/reference/security.md +++ b/docs/reference/security.md @@ -1,9 +1,10 @@ # Security Reference Consolidated security reference covering the nftables baseline, peer -ACL file format, cryptographic primitives, rekey defaults, replay -window, filesystem permissions, threat-resistance matrix, and default -network exposures per transport. For the threat-model design and +ACL file format, cryptographic primitives, key material in memory, +rekey defaults, replay window, filesystem permissions, +threat-resistance matrix, and default network exposures per +transport. For the threat-model design and rationale, see [../design/fips-security.md](../design/fips-security.md). For the operator activation steps and drop-in recipes, see [../how-to/enable-mesh-firewall.md](../how-to/enable-mesh-firewall.md). @@ -86,6 +87,68 @@ Domain separation and DH binding survive through the chaining key `ck`, which `h` is maintained at every step and is never fed to the AEAD, so it binds nothing. +## Key Material in Memory + +The daemon clears the copies of secret material that its own code +holds once they are no longer needed: the node's long-term private +key when the identity is dropped, the static and ephemeral keypairs a +Noise handshake holds, the chaining key and handshake hash, the +per-message Diffie-Hellman results, the key-derivation outputs and the +two session keys derived from them, the retained key on each cipher +state, the bech32 and hex encodings of a secret, and the configuration +text that carries `node.identity.nsec`. The SHA-256 state that hashes +each Diffie-Hellman result and the HMAC states inside HKDF clear +themselves on drop, through the opt-in `zeroize` features of `sha2` +and `hmac`, which the daemon turns on. A completed session keeps its +own copy of the handshake hash and does not clear it, on purpose: +nothing derives a key from it, and the session hands it out to any +caller. + +Each erase is a volatile write, and the optimiser does not remove a +volatile write as a dead store. `secp256k1`'s erase follows the write +with a compiler fence, and `zeroize`'s with an optimisation barrier (an +empty `asm!` block on x86_64); those limit how the compiler may reorder +code around the write, but it is the volatile write that keeps the +store. This was checked against generated code rather than assumed: in +an x86_64 release build (Rust 1.94.1, `secp256k1` 0.30.0, `zeroize` +1.9.0), every erase in the Noise handshake and identity code that is +linked into the daemon is present as stores in the machine code. + +An erase reaches only the place it is called on. What it does not +reach: + +- **Copies left by moves.** Moving a value copies its bytes and leaves + the old bytes where they were. A handshake state is built on the + stack and moved several times between being created and being + dropped. Each of those moves leaves behind, in a stack frame that is + no longer in use, a copy of the node's long-term private key and, + once the handshake has started, of its ephemeral key and chaining + key. Two moves out of the slots on a connection's control machine + are cleared: when a completed handshake leaves the slot that held it, + and when a session is taken out of its slot for a rekey, the slot is + overwritten as the value leaves. Other moves are not. When a + connection is promoted to an active peer, or reaped as stale, its + whole handshake state is moved off its control machine, which leaves + the session's two traffic keys, or an unfinished handshake's private + keys, in the heap memory the machine occupies. +- **Loading the identity from a secret string.** Building the node's + identity from its key file or from `node.identity.nsec` leaves + copies of the private key, among them a whole intermediate identity, + in that constructor's stack frame, and they are not cleared. +- **Registers and spilled temporaries**, which no code in the daemon + can name. +- **Library state.** The cipher keys cached inside `ring`'s + `LessSafeKey` have no clearing route. The daemon cannot clear the + internal temporaries of the `libsecp256k1` C library either; the + library clears some of its own, such as the nonce and secret scalar + used in signing, on a best-effort basis. + +Clearing therefore shortens how long secret material stays in memory +and removes it from the places the daemon's own code keeps it; it does +not guarantee that a secret is gone from the process. Reading what +remains requires access to the daemon's memory, or to a core dump or +swap image of it. + ## Rekey Defaults Both link-layer and session-layer Noise sessions rekey under one of @@ -245,12 +308,14 @@ machine. **The file descriptor carries the grant, not the connection.** A setup call hands the client a socket descriptor and the connection it was made on is then -closed; the flow or the held port lives until that descriptor is closed. A -descriptor is an ordinary kernel object, so it survives `fork`, survives -`exec` unless the client asked for it close-on-exec when it received it, and -can be handed to another process over `SCM_RIGHTS`. A process holding one can -send as this node on that flow, or receive on that port, without ever opening -the API socket and without being in the `fips` group. +closed; the flow or the held port lives until that descriptor is closed and +the daemon has let go of the copy it keeps while the descriptor is being +handed over. A descriptor is an ordinary kernel object, so it survives +`fork`, survives `exec` unless the client asked for it close-on-exec when it +received it, and can be handed to another process over `SCM_RIGHTS`. A +process holding one can send as this node on that flow, or receive on that +port, without ever opening the API socket and without being in the `fips` +group. Nothing revokes a descriptor already handed out. Restarting the daemon closes its own halves and ends every flow and listener at once, and that is the only revocation there is. @@ -269,6 +334,18 @@ peer had sent it, reaching any listener on this node under any peer identity the caller names. Leave it off outside a test harness; a packaged node does not enable it. +**A remote peer can fill the node's flow ceiling through a server that +refuses flows by dropping them.** Until a program first sends on a flow it +accepted, the daemon keeps its own copy of that flow's descriptor, so a flow +accepted and dropped unanswered keeps its slot against the node-wide +`node.native_api.max_flows` until its listener is dropped. A peer that opens +flows to such a listener from many source ports can therefore exhaust the +ceiling, and every other program on the node then gets `EMFILE` on `connect` +and silently loses arrivals on its listeners. This is the accepted cost of +keeping a flow alive while its descriptor is on the way to the program; see +[../how-to/use-the-native-datagram-api.md](../how-to/use-the-native-datagram-api.md) +for what releases the daemon's copy. + The socket is local only. It is not reachable over the network, and nothing about it changes the mesh's own authentication: a peer still verifies the node's signature, which is precisely why a local caller that can send through diff --git a/packaging/openwrt-ipk/Makefile b/packaging/openwrt-ipk/Makefile index a3fdbfa7..3ae6b8dc 100644 --- a/packaging/openwrt-ipk/Makefile +++ b/packaging/openwrt-ipk/Makefile @@ -118,7 +118,9 @@ define Package/fips/install # Firewall helper script (called by UCI include and hotplug) $(INSTALL_BIN) $(CURDIR)/files/etc/fips/firewall.sh $(1)/etc/fips/firewall.sh - # sysctl: enable br_netfilter so AF_PACKET sees frames on bridge member ports + # sysctl: turn off br_netfilter's call hooks (fips-bridge.conf). br_netfilter + # does not make bridge member ports usable for the Ethernet transport; the + # shipped config binds the LAN bridge instead. See fips-bridge.conf. $(INSTALL_DIR) $(1)/etc/sysctl.d $(INSTALL_DATA) $(CURDIR)/files/etc/sysctl.d/fips-bridge.conf $(1)/etc/sysctl.d/fips-bridge.conf $(INSTALL_DATA) $(CURDIR)/files/etc/sysctl.d/fips-gateway.conf $(1)/etc/sysctl.d/fips-gateway.conf diff --git a/packaging/openwrt-ipk/README.md b/packaging/openwrt-ipk/README.md index fac25610..96ce8251 100644 --- a/packaging/openwrt-ipk/README.md +++ b/packaging/openwrt-ipk/README.md @@ -17,7 +17,7 @@ OpenWrt 22.03+ router via the standard `opkg` package system. | `/etc/init.d/fips-gateway` | procd service for the gateway (disabled by default) | | `/etc/fips/fips.yaml` | Node configuration (edit before first start) | | `/etc/fips/firewall.sh` | Firewall helper — accepts traffic on `fips0` | -| `/etc/sysctl.d/fips-bridge.conf` | `br_netfilter` settings for Ethernet transport | +| `/etc/sysctl.d/fips-bridge.conf` | Turns off `br_netfilter`'s firewall call hooks | | `/etc/sysctl.d/fips-gateway.conf` | `proxy_ndp` and IPv6 forwarding for the gateway | | `/etc/hotplug.d/net/99-fips` | Applies firewall rules when `fips0` comes up | | `/etc/uci-defaults/90-fips-setup` | First-boot kernel module, firewall and dnsmasq `.fips` forwarding setup | @@ -38,7 +38,7 @@ OpenWrt 22.03+ router via the standard `opkg` package system. | Requirement | Notes | |---|---| | `kmod-tun` | Required for `fips0` TUN interface | -| `kmod-br-netfilter` | Required for Ethernet transport on bridge member ports | +| `kmod-br-netfilter` | Loaded, hooks off (`fips-bridge.conf`); see notes below | Both kernel modules are listed as package dependencies (`DEPENDS`) and will be installed automatically by `opkg`. @@ -141,11 +141,23 @@ The default config enables: - Ethernet transport, including the `wan`, `wwan` and `lan` entries For Ethernet transport, edit the interface names in the `ethernet:` section to -match your router. **Always use physical port names -(`eth0`, `eth1`, or DSA port names like `wan`/`lan1`), never bridge names -(`br-lan`).** The shipped default WAN port is `eth0` (OpenWrt 24); on OpenWrt -25 (DSA) boards the WAN port is named `wan` — the `.apk` package ships that -default. Run `ip link show` to confirm the names on your board. +match your router. **For the LAN, bind the LAN bridge (`br-lan`), never one of +its member ports** (on DSA boards `lan1`..`lanN`; on others whichever `ethN` the +bridge holds; `bridge link` lists them). A socket on a bridge member port sends +frames but never forms a link, because the bridge takes the frames that arrive +on its members; loading `br_netfilter` does not change that. +A lab test with a two-member Linux bridge found this with `br_netfilter` +unloaded, loaded with its call hooks off, and loaded with them on, while a +socket on `br-lan` worked both with `br_netfilter` unloaded and with it loaded +as shipped. Ports outside any bridge bind by their own name. The shipped +default WAN port is `eth0` (OpenWrt 24); on OpenWrt 25 (DSA) boards the WAN +port is named `wan`, and the `.apk` package ships that default. Run +`ip link show` to confirm the names on your board. + +The lab used software bridges only. A DSA switch with hardware bridge offload +has not been checked, so confirm the LAN entry forms links on such a router. +`kmod-br-netfilter`, `fips-bridge.conf` and the module load in `90-fips-setup` +are kept until their removal has been checked on a router. ## Service management diff --git a/packaging/openwrt-ipk/build-ipk.sh b/packaging/openwrt-ipk/build-ipk.sh index cdd82eec..f960a184 100755 --- a/packaging/openwrt-ipk/build-ipk.sh +++ b/packaging/openwrt-ipk/build-ipk.sh @@ -14,7 +14,13 @@ # arm 32-bit ARM routers (Cortex-A7) # x86_64 x86 routers / VMs # -# Output: dist/fips__.ipk +# Output: dist/fips__.ipk +# +# Environment: +# PKG_VERSION label for the file name (default: git describe) +# IPK_VERSION control file Version (default: PKG_VERSION). CI passes a +# release candidate as vX.Y.Z~rcN, which opkg sorts below the +# release, while PKG_VERSION keeps the tag's vX.Y.Z-rcN. # # Prerequisites: # cargo install cargo-zigbuild @@ -86,8 +92,13 @@ DIST_DIR="$PROJECT_ROOT/dist" PKG_NAME="fips" PKG_VERSION="${PKG_VERSION:-$(cd "$PROJECT_ROOT" && git describe --tags --always --dirty 2>/dev/null || echo "0.1.0")}" +IPK_VERSION="${IPK_VERSION:-$PKG_VERSION}" -echo "==> Building $PKG_NAME $PKG_VERSION for $OPENWRT_ARCH ($RUST_TARGET)" +if [ "$IPK_VERSION" = "$PKG_VERSION" ]; then + echo "==> Building $PKG_NAME $PKG_VERSION for $OPENWRT_ARCH ($RUST_TARGET)" +else + echo "==> Building $PKG_NAME $PKG_VERSION (control Version $IPK_VERSION) for $OPENWRT_ARCH ($RUST_TARGET)" +fi # --------------------------------------------------------------------------- # 1. Obtain binaries @@ -192,7 +203,7 @@ PKG_SIZE=$(du -sk "$DATA_DIR" | cut -f1) cat > "$CONTROL_DIR/control" </dev/null | - awk -v want=":$hex" 'substr($2, length($2) - 4) == want { print $10 }'); do + # cat rather than awk's own file arguments: busybox awk gives up on a + # missing /proc/net/udp6. A while-read loop on the pipe would run in a + # subshell under ash, where the return below could not leave this function. + inodes="$(cat /proc/net/udp /proc/net/udp6 2>/dev/null | + awk -v want=":$hex" 'substr($2, length($2) - 4) == want { print $10 }')" + for inode in $inodes; do for fd in /proc/"$2"/fd/*; do [ "$(readlink "$fd" 2>/dev/null)" = "socket:[$inode]" ] && return 0 done diff --git a/packaging/openwrt-ipk/files/etc/sysctl.d/fips-bridge.conf b/packaging/openwrt-ipk/files/etc/sysctl.d/fips-bridge.conf index ef8b28a8..4a0365a9 100644 --- a/packaging/openwrt-ipk/files/etc/sysctl.d/fips-bridge.conf +++ b/packaging/openwrt-ipk/files/etc/sysctl.d/fips-bridge.conf @@ -1,12 +1,17 @@ # FIPS: bridge netfilter settings # -# kmod-br-netfilter must be loaded for AF_PACKET sockets to receive frames -# on bridge member ports (e.g. eth1 when it is a member of br-lan). -# Without it, the bridge's rx_handler intercepts frames before they reach -# the packet socket layer. +# The package loads br_netfilter (kmod-br-netfilter) and this file turns +# off its IP/IPv6/ARP call hooks, so frames the bridge forwards are not +# also run through the firewall's IP, IPv6 and ARP tables. # -# We load br_netfilter for the AF_PACKET visibility benefit but disable its -# IP/IPv6/ARP call hooks to avoid double-processing of routed traffic. +# Loading br_netfilter does not make a bridge member port usable for the +# FIPS Ethernet transport. A lab test with a two-member Linux bridge found +# that a socket bound to a member port never forms a link, with +# br_netfilter unloaded, loaded with these hooks off, or loaded with them +# on. A socket bound to the bridge itself (br-lan) works, both with +# br_netfilter unloaded and with it loaded as this file sets it. +# Bind the bridge. The module, its dependency and this file stay until +# their removal has been checked on a router. net.bridge.bridge-nf-call-iptables=0 net.bridge.bridge-nf-call-ip6tables=0 net.bridge.bridge-nf-call-arptables=0 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 44965971..2eb43252 100644 --- a/packaging/openwrt-ipk/files/etc/uci-defaults/90-fips-setup +++ b/packaging/openwrt-ipk/files/etc/uci-defaults/90-fips-setup @@ -18,7 +18,10 @@ modprobe tun 2>/dev/null || true echo "tun" > /etc/modules.d/tun -# kmod-br-netfilter makes AF_PACKET visible on bridge member ports. +# br_netfilter is loaded with its call hooks off (fips-bridge.conf). It does +# not make a bridge member port usable for the Ethernet transport: a lab +# test found a member-port socket never forms a link either way, so the +# shipped config binds the LAN bridge. Its removal waits on a router check. modprobe br_netfilter 2>/dev/null || true echo "br_netfilter" > /etc/modules.d/br-netfilter diff --git a/src/cache/coord_cache.rs b/src/cache/coord_cache.rs index d2acd8ae..9390803e 100644 --- a/src/cache/coord_cache.rs +++ b/src/cache/coord_cache.rs @@ -247,6 +247,27 @@ impl CoordCache { self.entries.remove(addr) } + /// Demote an entry to an unverified hint, keeping its coordinates and TTL. + /// + /// The entry goes on routing, but a hint may now replace it. Returns + /// whether an entry existed. + pub fn demote(&mut self, addr: &NodeAddr) -> bool { + match self.entries.get_mut(addr) { + Some(entry) => { + entry.mark_hint(); + true + } + None => false, + } + } + + /// Forget the path MTU stored with an entry, keeping the entry. + pub fn clear_path_mtu(&mut self, addr: &NodeAddr) { + if let Some(entry) = self.entries.get_mut(addr) { + entry.clear_path_mtu(); + } + } + /// Check if an address is cached (and not expired). pub fn contains(&self, addr: &NodeAddr, current_time_ms: u64) -> bool { self.get(addr, current_time_ms).is_some() @@ -844,4 +865,52 @@ mod tests { assert!(cache.contains(&make_node_addr(1), 10)); assert!(cache.contains(&make_node_addr(2), 10)); } + + #[test] + fn demote_keeps_the_value_and_lets_a_hint_replace_it() { + let mut cache = CoordCache::new(100, 1000); + let addr = make_node_addr(1); + let real = make_coords(&[1, 0]); + cache.insert_verified_with_path_mtu(addr, real.clone(), 10, 1400); + assert_eq!( + cache.insert(addr, make_coords(&[1, 2, 0]), 10), + HintOutcome::Rejected + ); + + assert!(cache.demote(&addr)); + + let entry = cache.get_entry(&addr).unwrap(); + assert_eq!(entry.coords(), &real); + assert_eq!(entry.source(), crate::cache::CoordSource::Hint); + assert!(!entry.is_verified(10)); + assert_eq!( + entry.path_mtu(), + Some(1400), + "demote leaves the path MTU to its caller" + ); + assert_eq!( + cache.insert(addr, make_coords(&[1, 2, 0]), 11), + HintOutcome::Changed + ); + } + + #[test] + fn demote_of_an_absent_entry_reports_none() { + let mut cache = CoordCache::new(100, 1000); + assert!(!cache.demote(&make_node_addr(1))); + assert!(cache.is_empty()); + } + + #[test] + fn clear_path_mtu_keeps_the_entry() { + let mut cache = CoordCache::new(100, 1000); + let addr = make_node_addr(1); + cache.insert_verified_with_path_mtu(addr, make_coords(&[1, 0]), 10, 1400); + + cache.clear_path_mtu(&addr); + + let entry = cache.get_entry(&addr).unwrap(); + assert_eq!(entry.path_mtu(), None); + assert!(entry.is_verified(10)); + } } diff --git a/src/cache/entry.rs b/src/cache/entry.rs index 8fe95ae0..6b62f358 100644 --- a/src/cache/entry.rs +++ b/src/cache/entry.rs @@ -133,6 +133,11 @@ impl CacheEntry { self.path_mtu = Some(mtu); } + /// Forget the path MTU, as when the path it described is released. + pub fn clear_path_mtu(&mut self) { + self.path_mtu = None; + } + /// Check if this entry has expired. pub fn is_expired(&self, current_time_ms: u64) -> bool { current_time_ms > self.expires_at diff --git a/src/control/queries.rs b/src/control/queries.rs index a813462e..fe28bfd1 100644 --- a/src/control/queries.rs +++ b/src/control/queries.rs @@ -3238,7 +3238,7 @@ mod tests { /// renders each equal their on-loop oracle byte-for-byte, and all three are /// served off-loop via `snapshot_dispatch`. #[test] - fn snapshot_dispatch_serves_r5_queries() { + fn snapshot_dispatch_serves_acl_and_stats_peer_queries_off_loop_byte_identical() { use super::super::protocol::Request; use super::super::read_handle::snapshot_dispatch; diff --git a/src/control/snapshots/show_routing.json b/src/control/snapshots/show_routing.json index 0e0685da..f64fe2ee 100644 --- a/src/control/snapshots/show_routing.json +++ b/src/control/snapshots/show_routing.json @@ -36,6 +36,10 @@ "resp_unsolicited": 0 }, "error_signals": { + "broken_below_quorum": 0, + "broken_demoted": 0, + "broken_link_mismatch": 0, + "broken_reporter_mismatch": 0, "coords_required": 0, "emit_limiter_at_capacity": 0, "emit_over_dest_interval": 0, diff --git a/src/identity/local.rs b/src/identity/local.rs index 7a494369..e8dad691 100644 --- a/src/identity/local.rs +++ b/src/identity/local.rs @@ -16,9 +16,13 @@ use super::{FipsAddress, IdentityError, NodeAddr, sha256}; /// The keypair is the node's long-term private key. It is erased when the /// identity is dropped, and every constructor below erases the intermediate /// secret it built the identity from. All of that clears the copies this -/// crate owns, not every copy that ever existed: `secp256k1` names its erase -/// non-secure because the compiler may duplicate or move the bytes to places -/// no code here can name. +/// crate owns, not every copy that ever existed. Each erase is a volatile +/// write, so the optimiser keeps it, but it reaches only the place it is +/// called on: a copy made before it runs is not cleared, and that includes +/// the bytes a move leaves behind wherever the value used to be, such as +/// the frame of the constructor that built it. `from_secret_str` is a known +/// case: it leaves copies of the private key, a whole intermediate +/// `Identity` among them, in its own frame. #[derive(Clone)] pub struct Identity { keypair: Keypair, @@ -141,35 +145,36 @@ impl Drop for Identity { } } -/// A `Keypair` copy that is erased when it goes out of scope. +/// Erases a `Keypair` in place when it goes out of scope. /// /// `Keypair` is `Copy` and so cannot clear itself on drop. A frame that holds /// a copy of the node's long-term private key across several exit paths — /// early error returns, `?`, a normal return — would otherwise need an erase -/// written at each one, and a missed path is invisible. Holding the copy here -/// instead makes the clearing structural. +/// written at each one, and a missed path is invisible. Guarding the copy +/// makes the clearing structural. /// -/// This clears the copy this guard owns, not every copy that ever existed: -/// `secp256k1` names its erase non-secure because the compiler may duplicate -/// or move the bytes to places no code here can name. -pub(crate) struct ErasingKeypair(Keypair); +/// The guard borrows the keypair rather than holding one, so making it and +/// moving it copy no key bytes: the erase lands on the caller's own binding, +/// wherever that lies, and there is no second copy for an early return to +/// leave behind. It clears that one binding, not every copy that ever +/// existed. The erase is a volatile write, so the optimiser keeps it, but a +/// copy made before the guard existed, or taken out through +/// [`ErasingKeypair::get`], is not cleared by it. +pub(crate) struct ErasingKeypair<'a>(&'a mut Keypair); -impl ErasingKeypair { - /// Take a copy of `source` into the guard and erase `source` in place, so - /// the caller's own binding does not outlive the move. - pub(crate) fn take(source: &mut Keypair) -> Self { - let guarded = Self(*source); - source.non_secure_erase(); - guarded +impl<'a> ErasingKeypair<'a> { + /// Guard `source` so it is erased where it lies when the guard drops. + pub(crate) fn new(source: &'a mut Keypair) -> Self { + Self(source) } /// Borrow the guarded keypair. pub(crate) fn get(&self) -> &Keypair { - &self.0 + self.0 } } -impl Drop for ErasingKeypair { +impl Drop for ErasingKeypair<'_> { fn drop(&mut self) { self.0.non_secure_erase(); } diff --git a/src/identity/tests.rs b/src/identity/tests.rs index 546c7ff0..df066380 100644 --- a/src/identity/tests.rs +++ b/src/identity/tests.rs @@ -634,3 +634,31 @@ fn test_identity_debug() { assert!(!debug.contains("keypair")); assert!(debug.contains("..")); } + +/// Hold a guard over `keypair` and return early or normally, the way the +/// handshake entry points do. +fn guarded_secret(keypair: &mut Keypair, fail: bool) -> Result<[u8; 32], ()> { + let guard = ErasingKeypair::new(keypair); + if fail { + return Err(()); + } + Ok(guard.get().secret_bytes()) +} + +#[test] +fn test_erasing_keypair_erases_the_callers_binding_in_place_on_every_exit() { + let identity = Identity::generate(); + let original = identity.keypair().secret_bytes(); + // `non_secure_erase` overwrites a keypair with a fixed dummy whose + // secret is 32 bytes of 0x01. + let erased = [1u8; 32]; + assert_ne!(original, erased); + + let mut keypair = identity.keypair(); + assert_eq!(guarded_secret(&mut keypair, false), Ok(original)); + assert_eq!(keypair.secret_bytes(), erased); + + let mut keypair = identity.keypair(); + assert_eq!(guarded_secret(&mut keypair, true), Err(())); + assert_eq!(keypair.secret_bytes(), erased); +} diff --git a/src/native/client/mod.rs b/src/native/client/mod.rs index ee4c8ade..f02ec7f1 100644 --- a/src/native/client/mod.rs +++ b/src/native/client/mod.rs @@ -373,7 +373,10 @@ fn expired(error: io::Error) -> io::Error { /// /// The protocol has no close command. Dropping the stream closes its /// descriptor, and that is what releases the flow and its local port at the -/// daemon. +/// daemon. An accepted flow that was never sent on is the exception: the daemon +/// keeps its own copy of its descriptor until the first [`FipsStream::send`] or +/// until the listener is dropped, so dropping the stream before either releases +/// nothing yet. #[derive(Debug)] pub struct FipsStream { fd: OwnedFd, @@ -627,8 +630,9 @@ impl AsFd for FipsStream { /// **The listener is a descriptor**, which is what makes it pollable: it joins /// an existing `poll`, `select` or `epoll` loop with no new mechanism, and /// [`FipsListener::accept`] is one `recvmsg` on it. Dropping the listener closes -/// that descriptor, which unbinds the port; flows already accepted from it are -/// untouched. +/// that descriptor, which unbinds the port; flows already accepted from it and +/// still held are untouched, and those accepted and dropped without ever being +/// sent on end with it. #[derive(Debug)] pub struct FipsListener { fd: OwnedFd, @@ -676,7 +680,10 @@ impl FipsListener { /// Refusing a flow is dropping the stream, which closes its descriptor. /// There is no other way to refuse one, which is why an unreadable arrival /// message is reported after the descriptor it carried has been taken: the - /// flow is then refused rather than leaked. + /// flow is then refused rather than leaked. A refused flow that was never + /// sent on still holds its port and its slot against the node's flow limit + /// until this listener is dropped, because the daemon keeps its own copy of + /// the descriptor until then. pub fn accept(&self) -> io::Result<(FipsStream, FipsAddr)> { let mut buf = [0u8; CHUNK]; let chunk = fdpass::recv(self.fd.as_raw_fd(), &mut buf)?; diff --git a/src/native/dgram_probe.rs b/src/native/dgram_probe.rs index 6e36952b..3550cc11 100644 --- a/src/native/dgram_probe.rs +++ b/src/native/dgram_probe.rs @@ -20,6 +20,17 @@ //! every unix so the platforms can be compared without the test itself being a //! variable. //! +//! **A second measurement lives here: what the kernel does to a socket that is +//! in flight.** When the daemon hands a flow's descriptor to a listener's +//! client, the descriptor sits inside an `SCM_RIGHTS` message until the client +//! reads it. xnu's descriptor garbage collector takes its roots only from files +//! that are in flight, and treats one whose only references are messages as +//! unreachable unless it is found in the receive buffer of another in-flight +//! socket. The listener's client half is not in flight, so a flow socket whose +//! daemon copy has been closed is flushed by any collection that runs before +//! the client reads it. Linux keeps such a socket. The tests at the end of this +//! file measure that difference directly. +//! //! `SOCK_CLOEXEC` is deliberately not passed in the type argument, though //! `super::seqpacket::pair` does pass it. Linux and FreeBSD accept it there and //! macOS does not, and that difference belongs to the port rather than to this @@ -464,3 +475,91 @@ fn freebsd_seqpacket_drops_a_zero_length_message_instead_of_delivering_it() { diagnosis of the stalled FreeBSD runs is wrong." ); } + +/// How long a collection is given to run after it has been queued. +/// +/// xnu runs its descriptor collector as an asynchronous thread call, so the +/// close that queues it returns before it has run. A fixed wait is the only +/// handle a test has on it; if the Darwin probe below ever misses, this is the +/// first number to raise. +const COLLECTION_WAIT: std::time::Duration = std::time::Duration::from_millis(100); + +/// Give the kernel's descriptor collector a reason to run, then time to run. +/// +/// Freeing any `AF_UNIX` socket queues xnu's collector, so a fresh pair closed +/// at once is enough. That is also why the collector can run at any moment on +/// a busy host: every unix socket any process closes queues it. Linux runs its +/// collector here too: in 6.8, the kernel this was read against, closing a unix +/// socket while any descriptor is in flight runs it before the close returns. +/// So the Linux probe below checks that Linux's collector keeps the socket, +/// rather than only that no collector ran. +pub(super) fn provoke_collection() { + let (a, b) = dgram_pair().expect("AF_UNIX SOCK_DGRAM socketpair"); + drop(a); + drop(b); + std::thread::sleep(COLLECTION_WAIT); +} + +/// Hand a flow's client half across a listener pair the way the daemon does, +/// close the sender's copy, provoke a collection, and read what reaches the +/// receiver. +/// +/// Built from the product's own pair type and hand-off code rather than from +/// [`dgram_pair`], so the measurement is of exactly the sockets the daemon +/// uses. Returns the first read on the received descriptor: the held bytes if +/// the socket survived, zero bytes if the collector flushed it. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn read_after_collection_in_flight() -> io::Result { + use super::{fdpass, seqpacket}; + use std::os::fd::AsFd; + + let (daemon, flow) = seqpacket::pair()?; + let (sender, receiver) = seqpacket::pair()?; + assert_eq!(send(daemon.as_raw_fd(), b"held")?, 4); + + fdpass::try_send(sender.as_raw_fd(), b"arrival", Some(flow.as_fd()))?; + // The message is now the only reference to the flow's client half. + drop(flow); + + provoke_collection(); + + let mut buf = [0u8; 64]; + let chunk = fdpass::recv(receiver.as_raw_fd(), &mut buf)?; + let flow = chunk + .fd + .ok_or_else(|| io::Error::other("the message arrived without its descriptor"))?; + recv(flow.as_raw_fd(), &mut buf) +} + +/// Darwin flushes an in-flight socket whose only reference is the message. +/// +/// This is the defect behind the native API's intermittent macOS failures, in +/// isolation: the listener's client receives a flow descriptor that reads as +/// end of file, with the datagram written to it before the hand-off gone. +/// A failure here means the collector did not run within +/// [`COLLECTION_WAIT`], or that Darwin no longer collects such a socket. In +/// the second case the daemon's hold on a handed-over descriptor is no longer +/// needed there. +#[cfg(target_os = "macos")] +#[test] +fn darwin_collects_an_in_flight_socket_whose_only_reference_is_the_message() { + let read = read_after_collection_in_flight(); + assert!( + matches!(read, Ok(0)), + "expected the collector to flush the in-flight flow socket, so its first \ + read returns end of file; got {read:?}" + ); +} + +/// Linux keeps the same socket through the collection the close provokes: a +/// queue held by a socket that is not in flight counts as a reference to what +/// it holds. +#[cfg(target_os = "linux")] +#[test] +fn linux_keeps_an_in_flight_socket_whose_only_reference_is_the_message() { + let read = read_after_collection_in_flight(); + assert!( + matches!(read, Ok(4)), + "expected the in-flight flow socket to survive with its datagram; got {read:?}" + ); +} diff --git a/src/native/mod.rs b/src/native/mod.rs index 57a772f4..305c5760 100644 --- a/src/native/mod.rs +++ b/src/native/mod.rs @@ -18,7 +18,20 @@ //! replies and then has no further part in anything it opened. A flow lives //! until its own descriptor reaches end of file and a listener until its own //! does, whichever task holds them, which is what makes a descriptor this API -//! hands back behave like one a syscall would have. +//! hands back behave like one a syscall would have. The one exception: the +//! connection keeps a copy of the descriptor in its last reply until the +//! client's next command or its close, for the same reason a listener keeps a +//! copy of a flow it hands over (below). The shipped client closes the +//! connection as soon as it has the reply, so for it the copy is gone at once. +//! +//! **A listener keeps a copy of a flow it handed over.** While a flow's +//! descriptor sits unread in an arrival message, the message can be the only +//! reference to it, and xnu's descriptor collector flushes a socket in that +//! state. So `hand_over` keeps the daemon's copy until the client has written +//! on the flow or closed the listener, and a flow then reaches end of file once +//! both the client and that copy have let go. The cost is that a client which +//! accepts a flow and closes it without writing leaves it open until the +//! listener closes. See `dgram_probe.rs` for the measurement. //! //! **The wire is connected.** A datagram a client writes leaves this node over //! FSP, and one arriving on a held port reaches its flow. `max_payload` is the @@ -233,6 +246,15 @@ mod unix_impl { /// writes into. sock: Arc, counts: Arc, + /// The daemon's copy of the client's half, kept while that half may + /// still be in flight to a listener's client. + /// + /// Holding it keeps the socket reachable from outside the message that + /// carries it, which is what stops xnu's collector flushing it before + /// the client reads the arrival. `None` once released, and always for + /// a connected flow, whose descriptor went back in an RPC reply and is + /// held by the connection that sent it instead; see `Connection::run`. + pin: Option, } /// What `stats` reports about one flow. @@ -283,6 +305,31 @@ mod unix_impl { self.table().remove(&id); } + /// Let go of the daemon's copy of a flow's client half. + /// + /// The copy is dropped after the lock is released: if the client has + /// already closed its own, this is the last reference, and the close it + /// causes is the flow's end of file. + fn unpin(&self, id: u64) { + let pin = self.table().get_mut(&id).and_then(|flow| flow.pin.take()); + drop(pin); + } + + /// Let go of the daemon's copy of every flow on a local port. + /// + /// Called when a listener ends. Every pinned flow on its port came from + /// it: a connected flow carries no pin, and no later listener can hold + /// the port until this one's release has been served. + fn unpin_port(&self, port: u16) { + let pins: Vec = self + .table() + .values_mut() + .filter(|flow| flow.local == port) + .filter_map(|flow| flow.pin.take()) + .collect(); + drop(pins); + } + /// What the debug `stats` command reports, or `None` for a flow this /// node does not hold. fn stats(&self, id: u64) -> Option { @@ -503,6 +550,7 @@ mod unix_impl { key, peer, wiring, + None, &self.outbound, &self.node, &self.flows, @@ -700,6 +748,19 @@ mod unix_impl { #[cfg(test)] impl Connection { + /// Another connection to the same node, sharing its flow table, as a + /// second client of one daemon has. + pub(super) fn sibling(&self) -> Self { + Self::new( + self.node.clone(), + self.outbound.clone(), + Arc::clone(&self.flows), + self.limits, + Arc::clone(&self.npub), + self.debug, + ) + } + /// Build a connection wired to a channel a test serves. pub(super) fn for_test( node: mpsc::Sender, @@ -730,14 +791,24 @@ mod unix_impl { } } - /// Wait until a flow's reader task has observed the client's close. + /// Wait until a flow's reader task has observed the client's close, + /// failing by name if it never does. + /// + /// A flow the node no longer holds counts as closed: the reader sets + /// the flag and forgets the flow in the same turn, so the flag alone is + /// almost never there to be seen. Bounded by the clock rather than by + /// turns of the runtime, for the reason given at `CLOSE_WAIT`. pub(super) async fn settle_closed(&self, flow: u64) { - for _ in 0..1000 { - match self.flows.stats(flow) { - Some(stats) if stats.closed => return, - _ => tokio::task::yield_now().await, - } - } + let seen = super::tests::eventually(async || match self.flows.stats(flow) { + Some(stats) if !stats.closed => None, + _ => Some(()), + }) + .await; + assert!( + seen.is_some(), + "the reader never saw flow {flow} close within {:?}", + super::tests::CLOSE_WAIT + ); } } @@ -780,35 +851,48 @@ mod unix_impl { /// The reader cannot start any earlier: it stamps every datagram it /// forwards with the flow's key and the peer's address, and the local port /// is not known until the registry has answered. + /// + /// The flow is recorded before the reader is spawned. A handed-over client + /// can already have written, and on a multi-thread runtime the reader can + /// run at once, so recording afterwards would let its unpin find no flow + /// and leave the pin in place for the flow's whole life. + #[allow( + clippy::too_many_arguments, + reason = "one hand-off of plumbing to two tasks, with no state to group" + )] fn start( id: u64, key: FlowKey, peer: XOnlyPublicKey, wiring: Wiring, + pin: Option, outbound: &mpsc::Sender, node: &mpsc::Sender, flows: &Arc, ) { let counts = Arc::new(Counts::default()); + let pinned = pin.is_some(); + flows.record( + id, + Flow { + local: key.local, + sock: Arc::clone(&wiring.sock), + counts: Arc::clone(&counts), + pin, + }, + ); tokio::spawn(drain( id, key, peer, Arc::clone(&wiring.sock), - Arc::clone(&counts), + counts, + pinned, outbound.clone(), node.clone(), Arc::clone(flows), )); - tokio::spawn(feed(Arc::clone(&wiring.sock), wiring.inbound)); - flows.record( - id, - Flow { - local: key.local, - sock: wiring.sock, - counts, - }, - ); + tokio::spawn(feed(wiring.sock, wiring.inbound)); } /// Serve one listener until its client closes the descriptor. @@ -872,6 +956,15 @@ mod unix_impl { } debug!(port, "Native API listener closed by its client"); + // Before the release, so the port cannot have passed to another + // listener whose flows this would also let go of. When the client has + // closed the listener, an arrival it never read went with the + // listener's receive queue, so no descriptor from this port is still + // in flight to it. The other two ways out of the loop, the node going + // away and a failed read, can leave the client's half open with + // arrivals unread on it. Both are teardown, and the hold goes with the + // listener rather than outliving it. + flows.unpin_port(port); let _ = node .send(NativeMessage::Release { flows: Vec::new(), @@ -891,6 +984,15 @@ mod unix_impl { /// Both writes are try-sends. They go onto a socket pair whose client half /// has not been sent yet, so no process can read either one and a task that /// parked on one would stop serving this listener entirely. + /// + /// The daemon keeps its copy of the client's half after the arrival is + /// written, until the client writes on the flow or closes the listener. + /// Until the client reads the arrival, the message carrying the descriptor + /// is otherwise its only reference, and xnu's collector flushes a socket in + /// that state: the client then receives a flow that reads as end of file, + /// with its held datagrams gone. Nothing in the protocol says when the + /// client has read the arrival, so a client that closes the flow without + /// writing leaves it open until the listener closes. async fn hand_over( arrival: Arrival, listener: &Seqpacket, @@ -968,13 +1070,11 @@ mod unix_impl { accepted.key, accepted.peer, wiring, + Some(theirs), outbound, node, flows, ); - // Dropping our copy leaves the client holding the only reference to its - // half, so its close tears the flow down. - drop(theirs); } /// What a failed hand-off write says about the client, for the counter. @@ -1098,6 +1198,11 @@ mod unix_impl { /// Counting continues alongside the forwarding: `stats` is how a check /// observes that a datagram reached the daemon, independently of whether it /// then reached a peer. + /// + /// `pinned` says the daemon still holds a copy of the client's half. The + /// first datagram the client writes proves it holds the descriptor, so the + /// copy is let go then, once, and the client's close ends the flow from + /// there on. #[allow( clippy::too_many_arguments, reason = "one hand-off of plumbing to a task, with no state to group" @@ -1108,6 +1213,7 @@ mod unix_impl { peer: XOnlyPublicKey, sock: Arc, counts: Arc, + mut pinned: bool, outbound: mpsc::Sender, node: mpsc::Sender, flows: Arc, @@ -1116,6 +1222,10 @@ mod unix_impl { loop { match sock.recv(&mut buf).await { Ok(Received::Datagram(len)) => { + if pinned { + flows.unpin(id); + pinned = false; + } counts.datagrams.fetch_add(1, Ordering::Relaxed); counts.bytes.fetch_add(len as u64, Ordering::Relaxed); let sent = outbound @@ -1210,21 +1320,42 @@ mod unix_impl { npub: Arc, debug: bool, ) -> Result<(), std::io::Error> { - let mut connection = Connection::new(node, outbound, flows, limits, npub, debug); - let mut reader = BufReader::new(stream); - let mut line = Vec::new(); + Connection::new(node, outbound, flows, limits, npub, debug) + .run(stream) + .await + } - while read_command(&mut reader, &mut line).await? { - let (response, fd) = connection.answer(&line).await; - let mut json = serde_json::to_vec(&response)?; - json.push(b'\n'); - fdpass::reply(reader.get_ref(), &json, fd.as_ref().map(AsFd::as_fd)).await?; - // Dropping our copy leaves the client holding the only reference to - // its half, so its close tears the flow or the listener down. - drop(fd); + impl Connection { + /// Answer the commands on one client connection, in order, until it + /// closes or misbehaves. + /// + /// **The daemon keeps its copy of the descriptor in the last reply** + /// until the client's next command arrives or the connection ends. + /// Until the client reads the reply, the message carrying the + /// descriptor can be its only reference, and xnu's collector flushes a + /// socket in that state, as it does an unread arrival's. A client that + /// sends another command has read the reply first, unless it pipelined, + /// which the shipped client never does. Once the copy is gone the + /// client holds the only reference, so its close tears the flow or the + /// listener down; until then a close it makes waits for the copy. + pub(super) async fn run(mut self, stream: UnixStream) -> Result<(), std::io::Error> { + let mut reader = BufReader::new(stream); + let mut line = Vec::new(); + let mut kept: Option = None; + + while read_command(&mut reader, &mut line).await? { + drop(kept.take()); + let (response, fd) = self.answer(&line).await; + let mut json = serde_json::to_vec(&response)?; + json.push(b'\n'); + fdpass::reply(reader.get_ref(), &json, fd.as_ref().map(AsFd::as_fd)).await?; + kept = fd; + } + + // End of file, or an early return above: either way `kept` goes + // with this frame, and with it the last copy the daemon holds. + Ok(()) } - - Ok(()) } /// Read one newline-terminated command into `line`, refusing an oversized @@ -1365,6 +1496,47 @@ mod tests { sock } + /// How long a test waits for the daemon to notice that a client closed a + /// descriptor. + /// + /// On Linux the close wakes the task reading the daemon's half, so a wait + /// that will succeed does so within a few turns of the runtime. On macOS + /// and FreeBSD nothing wakes it: the reader sees the close only when its + /// bounded wait expires and it retries the read, as much as one + /// `CLOSE_RETRY` interval (`seqpacket.rs`) after the close, and a + /// listener's close that ends a flow's hold costs two of those in a row. + /// Counting turns of the runtime bounds nothing there: a thousand of them + /// ran out well inside one interval on a macOS runner. A wait that is + /// going to succeed still returns as soon as it does; this is how long one + /// that is not takes to say so. + pub(super) const CLOSE_WAIT: std::time::Duration = std::time::Duration::from_secs(5); + + // Four times the longest run of unnoticed closes a test waits through, so + // that lengthening `CLOSE_RETRY` cannot quietly use up the margin. + const _: () = assert!( + CLOSE_WAIT.as_millis() >= 8 * super::seqpacket::CLOSE_LATENCY.as_millis(), + "CLOSE_WAIT no longer covers two unnoticed closes four times over" + ); + + /// Retry `attempt` until it produces a value, for up to [`CLOSE_WAIT`]. + /// + /// `None` means the bound ran out. Attempts are a millisecond apart rather + /// than a yield apart, so a wait that lasts a quarter second on macOS is + /// not spent spinning, which for [`rebind`] would mean opening and closing + /// a socket pair on every turn. + pub(super) async fn eventually(mut attempt: impl AsyncFnMut() -> Option) -> Option { + tokio::time::timeout(CLOSE_WAIT, async { + loop { + if let Some(value) = attempt().await { + return value; + } + tokio::time::sleep(std::time::Duration::from_millis(1)).await; + } + }) + .await + .ok() + } + /// Send one command that opens a flow, returning the reply and descriptor. async fn open(connection: &mut Connection, line: &str) -> (serde_json::Value, StdUnixStream) { let (response, fd) = connection.answer(line.as_bytes()).await; @@ -1726,6 +1898,332 @@ mod tests { assert_eq!(&buf[..3], &[0x00, 0xff, 0x10]); } + /// Wait until a listener descriptor has an arrival to read. + /// + /// Polled without blocking and yielding in between, because the arrival is + /// written by the listener's task on this same runtime and a blocking wait + /// would stop the task it is waiting for. A readable listener means + /// `hand_over` has finished: it writes the arrival and settles what happens + /// to the daemon's copy of the descriptor in one synchronous step. + async fn readable(listener: &StdUnixStream) { + for _ in 0..1000 { + let mut poll = libc::pollfd { + fd: listener.as_raw_fd(), + events: libc::POLLIN, + revents: 0, + }; + // SAFETY: `poll` points at one live pollfd and the call cannot block. + let rc = unsafe { libc::poll(&mut poll, 1, 0) }; + if rc > 0 && (poll.revents & libc::POLLIN) != 0 { + return; + } + tokio::task::yield_now().await; + } + panic!("no arrival became readable on the listener"); + } + + /// Wait until the node no longer holds a flow, failing if it never lets go. + /// + /// The reader marks a flow closed before it gives the registry entry back + /// and forgets the flow, so `stats` can still find a closed flow for a few + /// turns of the runtime, and on macOS and FreeBSD the reader notices the + /// close itself only when it next retries. Bounded by [`CLOSE_WAIT`], so a + /// flow that is never released names itself. + async fn forgotten(connection: &mut Connection, flow: u64) { + let line = format!(r#"{{"command":"stats","params":{{"flow_id":{flow}}}}}"#); + let mut last = serde_json::Value::Null; + let gone = eventually(async || { + last = ask(connection, &line).await; + (last["data"]["errno"] == "ENOENT").then_some(()) + }) + .await; + assert!( + gone.is_some(), + "the node still holds flow {flow} after {CLOSE_WAIT:?}: {last}" + ); + } + + /// Bind a listener on a port a closed one held, retrying until the closed + /// one has given it back. + /// + /// The release reaches the registry from the closed listener's own task + /// once that task has noticed the close, which takes a few turns of the + /// runtime, or on macOS and FreeBSD up to a retry interval. Bounded by + /// [`CLOSE_WAIT`], so a port that is never given back fails here by name. + async fn rebind(connection: &mut Connection, port: u16) -> OwnedFd { + let line = format!(r#"{{"command":"listen","params":{{"local_port":{port}}}}}"#); + eventually(async || { + let (response, fd) = connection.answer(line.as_bytes()).await; + (serde_json::to_value(response).unwrap()["status"] == "ok") + .then(|| fd.expect("a rebound listener still gets a descriptor")) + }) + .await + .unwrap_or_else(|| { + panic!("the closed listener never gave port {port} back within {CLOSE_WAIT:?}") + }) + } + + /// Bind a listener on 4242, deliver one datagram to it from a new peer, + /// and accept the flow that announced, returning everything a test needs. + async fn arrive_and_accept( + connection: &mut Connection, + ) -> (StdUnixStream, u64, serde_json::Value, StdUnixStream) { + let (_value, listener) = listen(connection, 4242).await; + let value = ask(connection, &arrival(5000, 4242, "00ff10")).await; + assert_eq!(value["data"]["outcome"], "announced", "{value}"); + let flow = value["data"]["flow_id"].as_u64().unwrap(); + let (message, client) = accept(&listener); + (listener, flow, message, client) + } + + #[tokio::test] + async fn an_arrival_survives_a_kernel_collection_before_its_client_reads_it() { + // The macOS failure, made deterministic. Between the daemon writing the + // arrival and the client reading it, the flow's descriptor exists only + // inside the arrival message. xnu's descriptor collector flushes a + // socket in that state, so unless the daemon still holds its own copy, + // the client receives a flow that reads as end of file with its held + // datagram gone. On Linux the kernel keeps the socket either way, so + // this test can only fail on macOS. + let (mut connection, _outbound) = connect(); + let (_value, listener) = listen(&mut connection, 4242).await; + let value = ask(&mut connection, &arrival(5000, 4242, "00ff10")).await; + assert_eq!(value["data"]["outcome"], "announced", "{value}"); + + readable(&listener).await; + super::dgram_probe::provoke_collection(); + + let (message, mut client) = accept(&listener); + assert_eq!(message["held"], 1); + let mut buf = [0u8; 64]; + assert_eq!( + client.read(&mut buf).unwrap(), + 3, + "the accepted flow lost its held datagram to the kernel's collector" + ); + assert_eq!(&buf[..3], &[0x00, 0xff, 0x10]); + } + + #[tokio::test] + async fn a_handed_over_flow_outlives_its_dropped_descriptor_until_its_listener_closes() { + // The daemon keeps its copy of a handed-over descriptor until the + // client has shown it holds one, because dropping it is what exposes + // the descriptor to the macOS collector. On Linux the hold is + // observable this way: a client that closes the flow without ever + // writing does not end it while the listener that produced it is open. + let (mut connection, _outbound) = connect(); + let (listener, flow, _message, client) = arrive_and_accept(&mut connection).await; + + drop(client); + still_open( + &mut connection, + flow, + "the flow ended while the daemon should still hold its descriptor", + ) + .await; + + // Closing the listener ends the hold: whatever the client did with the + // arrival, the descriptor is no longer in flight in a live socket. + drop(listener); + forgotten(&mut connection, flow).await; + } + + #[tokio::test] + async fn a_handed_over_flow_closes_with_its_descriptor_once_its_client_has_written() { + // A datagram from the client proves it holds the descriptor, so the + // daemon lets its copy go and the client's close ends the flow at once, + // with the listener still open. + let (mut connection, _outbound) = connect(); + let (listener, flow, _message, mut client) = arrive_and_accept(&mut connection).await; + + client.write_all(b"x").unwrap(); + connection.settle(flow, 1).await; + drop(client); + forgotten(&mut connection, flow).await; + drop(listener); + } + + #[tokio::test] + async fn a_handed_over_flow_its_client_still_holds_outlives_its_listener_and_closes_with_its_descriptor() + { + // Closing the listener lets the daemon's copy go, and the client's own + // copy then carries the flow by itself: it keeps working after the + // listener has gone, and the client's close ends it with no write ever + // made. Letting the copy go must not end a flow the client still holds. + let (mut connection, _outbound) = connect(); + let (listener, flow, _message, mut client) = arrive_and_accept(&mut connection).await; + + drop(listener); + // The port comes back only after the listener's task has let its + // flows' copies go, so a rebound port means that has happened. + let _rebound = rebind(&mut connection, 4242).await; + + let mut buf = [0u8; 64]; + assert_eq!(client.read(&mut buf).unwrap(), 3); + let value = ask( + &mut connection, + &format!(r#"{{"command":"inject","params":{{"flow_id":{flow},"data":"ab"}}}}"#), + ) + .await; + assert_eq!(value["status"], "ok", "{value}"); + assert_eq!(client.read(&mut buf).unwrap(), 1); + assert_eq!(buf[0], 0xab); + + drop(client); + forgotten(&mut connection, flow).await; + } + + /// Serve a sibling of `connection` over a real socket, the way the daemon + /// serves a client, returning the client's end and the serving task. + /// + /// Through `run` rather than `answer`, because what these tests observe is + /// what the serving loop does with the descriptor in a reply it has sent. + fn serve_socket( + connection: &Connection, + ) -> (StdUnixStream, tokio::task::JoinHandle>) { + let (ours, theirs) = StdUnixStream::pair().expect("AF_UNIX socketpair"); + ours.set_nonblocking(true).expect("the socket is open"); + let ours = tokio::net::UnixStream::from_std(ours).expect("inside a runtime"); + let task = tokio::spawn(connection.sibling().run(ours)); + (bounded(theirs), task) + } + + /// Write one command on a client socket and read its reply line, with the + /// descriptor it carried. + async fn call(client: &StdUnixStream, line: &str) -> (serde_json::Value, Option) { + let mut writer = client; + writer.write_all(line.as_bytes()).unwrap(); + writer.write_all(b"\n").unwrap(); + readable(client).await; + let mut buf = [0u8; 4096]; + let chunk = super::fdpass::recv(client.as_raw_fd(), &mut buf) + .expect("a reply should be readable on the connection"); + let reply = buf[..chunk.len] + .strip_suffix(b"\n") + .expect("one whole reply line per read"); + (serde_json::from_slice(reply).unwrap(), chunk.fd) + } + + /// Open a flow through a served socket, returning its id and descriptor. + async fn connect_over(client: &StdUnixStream) -> (u64, OwnedFd) { + let line = format!( + r#"{{"command":"connect","params":{{"peer":"{PEER}","remote_port":4242,"local_port":4243}}}}"# + ); + let (value, fd) = call(client, &line).await; + assert_eq!(value["status"], "ok", "{value}"); + let flow = value["data"]["flow_id"].as_u64().unwrap(); + ( + flow, + fd.expect("a connect reply carries the flow's descriptor"), + ) + } + + /// Assert that the node still holds a flow open, after giving its reader + /// every chance to notice a close. + /// + /// Where a close wakes the reader, a few turns of the runtime are that + /// chance. On macOS and FreeBSD the reader notices a close only when its + /// bounded wait expires and it retries, so a check made sooner passes + /// whether or not the flow has closed. There this also waits out two of + /// those intervals: a reader that parked just before a close has retried + /// by then, with one interval to spare for a loaded runner. Elsewhere the + /// interval is zero and the wait costs nothing. + async fn still_open(connection: &mut Connection, flow: u64, why: &str) { + for _ in 0..1000 { + tokio::task::yield_now().await; + } + tokio::time::sleep(super::seqpacket::CLOSE_LATENCY * 2).await; + let value = ask( + connection, + &format!(r#"{{"command":"stats","params":{{"flow_id":{flow}}}}}"#), + ) + .await; + assert_eq!(value["status"], "ok", "{why}: {value}"); + assert_eq!(value["data"]["closed"], false, "{why}: {value}"); + } + + /// Any command at all, sent only so the serving loop reads one. + const NEXT: &str = r#"{"command":"stats","params":{"flow_id":0}}"#; + + #[tokio::test] + async fn a_descriptor_sent_in_a_reply_is_kept_until_the_clients_next_command() { + // Until the client reads a reply, the message carrying its descriptor + // can be the only reference to it, which is what xnu's collector + // flushes. So the daemon keeps its copy until the client sends another + // command, which it does only after reading the reply. On Linux the + // hold is observable as a flow that outlives the client's close. + let (mut probe, _outbound) = connect(); + let (client, _task) = serve_socket(&probe); + let (flow, fd) = connect_over(&client).await; + + drop(fd); + still_open( + &mut probe, + flow, + "the flow ended while the connection should still hold its descriptor", + ) + .await; + + call(&client, NEXT).await; + forgotten(&mut probe, flow).await; + } + + #[tokio::test] + async fn a_descriptor_sent_in_a_reply_is_let_go_when_the_connection_ends() { + // The connection's close ends its hold. The client's own copy then + // carries the flow alone, so the flow survives the connection and ends + // with the client's close. + let (mut probe, _outbound) = connect(); + let (client, task) = serve_socket(&probe); + let (flow, fd) = connect_over(&client).await; + + drop(client); + task.await + .unwrap() + .expect("a connection closed between commands ends cleanly"); + still_open( + &mut probe, + flow, + "the flow ended with the connection while the client still holds it", + ) + .await; + + drop(fd); + forgotten(&mut probe, flow).await; + } + + #[tokio::test] + async fn a_flow_its_client_closes_after_its_next_command_ends_at_once() { + // Once the client has sent another command the hold is over, so the + // client's close is the flow's end of file with the connection still + // open. + let (mut probe, _outbound) = connect(); + let (client, _task) = serve_socket(&probe); + let (flow, fd) = connect_over(&client).await; + + call(&client, NEXT).await; + drop(fd); + forgotten(&mut probe, flow).await; + drop(client); + } + + #[tokio::test] + async fn a_flow_whose_reply_is_never_followed_by_a_command_ends_with_the_connection() { + // A client that closes the flow and then the connection, sending + // nothing more, still ends the flow: the connection's end of file is + // the last point at which the daemon lets its copy go. + let (mut probe, _outbound) = connect(); + let (client, task) = serve_socket(&probe); + let (flow, fd) = connect_over(&client).await; + + drop(fd); + drop(client); + task.await + .unwrap() + .expect("a connection closed between commands ends cleanly"); + forgotten(&mut probe, flow).await; + } + #[test] fn a_listener_that_closed_and_one_that_stopped_reading_are_counted_apart() { // Both take the same cleanup, so the counter is the only place the @@ -1946,19 +2444,8 @@ mod tests { // The unbind is what `close(listen_fd)` means in Berkeley, and the // daemon can only observe it by reading its own half. Without the read - // arm the port is held for the node's lifetime and every attempt below - // fails. - for _ in 0..1000 { - let (response, fd) = connection - .answer(br#"{"command":"listen","params":{"local_port":4242}}"#) - .await; - if serde_json::to_value(response).unwrap()["status"] == "ok" { - assert!(fd.is_some(), "a rebound listener still gets a descriptor"); - return; - } - tokio::task::yield_now().await; - } - panic!("the closed listener never gave its port back"); + // arm the port is held for the node's lifetime and every attempt fails. + rebind(&mut connection, 4242).await; } #[tokio::test] diff --git a/src/native/seqpacket.rs b/src/native/seqpacket.rs index d2537a27..a9bd60b8 100644 --- a/src/native/seqpacket.rs +++ b/src/native/seqpacket.rs @@ -180,6 +180,19 @@ pub fn set_sndbuf(fd: &OwnedFd, bytes: usize) -> io::Result<()> { #[cfg(any(target_os = "macos", target_os = "freebsd"))] const CLOSE_RETRY: Duration = Duration::from_millis(250); +/// How long a reader can leave a closed peer unnoticed, for the native API +/// tests that assert a flow is still open and so must wait long enough to have +/// seen it close. +/// +/// `CLOSE_RETRY` where the reactor cannot see a close, because the reader then +/// notices one only when that bound expires and it retries the read. +#[cfg(all(test, any(target_os = "macos", target_os = "freebsd")))] +pub(super) const CLOSE_LATENCY: Duration = CLOSE_RETRY; + +/// Zero where a close wakes the reader itself. +#[cfg(all(test, not(any(target_os = "macos", target_os = "freebsd"))))] +pub(super) const CLOSE_LATENCY: Duration = Duration::ZERO; + /// The daemon's half of a flow's or a listener's socket pair, registered with /// the reactor. pub struct Seqpacket { diff --git a/src/node/decrypt_worker.rs b/src/node/decrypt_worker.rs index a5d3e7aa..d4d5173c 100644 --- a/src/node/decrypt_worker.rs +++ b/src/node/decrypt_worker.rs @@ -1,11 +1,10 @@ //! Off-task FMP + FSP decrypt + delivery worker. //! -//! First incremental step of the data-plane shard restructure (per the -//! architectural plan): each worker now **owns its session state -//! directly** in a local `HashMap`, with no `Arc>` -//! cache on the Node side and no `Arc>` shared -//! with the rx_loop. The worker is the sole authority over the replay -//! window and the recv-side ciphers for every session it owns. +//! Each worker **owns its session state directly** in a local +//! `HashMap`, with no `Arc>` cache on the Node side and +//! no `Arc>` shared with the rx_loop. The worker is +//! the sole authority over the replay window and the recv-side ciphers +//! for every session it owns. //! //! Dispatch is **deterministic by session key**: rx_loop computes //! `worker_idx = hash(cache_key) % N` and routes both @@ -484,8 +483,8 @@ fn handle_job( // **decrypted-in-place** FMP plaintext back to rx_loop. // // Two problems with that path: - // 1. After the shard-owned-sessions refactor (01f6c62), the FSP - // replay window is owned by **this worker thread**. Once we + // 1. Since sessions became shard-owned, the FSP replay + // window is owned by **this worker thread**. Once we // `state.fsp_replay.accept(fsp_counter)`, the rx_loop's // `noise::Session::replay_window` is stale — it still has // old counters. When rx_loop tries to FSP-decrypt the @@ -511,11 +510,9 @@ fn handle_job( // still offloads the FMP AEAD (~half the per-packet decrypt // CPU). Correctness over micro-optimisation. // - // The DataShard end-state (per the architectural plan) re- - // introduces the EndpointData fast path correctly by having the - // shard worker also own the rx_loop side for its sessions — at - // that point there's no "rx_loop legacy path" for the worker to - // conflict with. + // A worker that also owned the rx_loop side of its sessions could + // restore the EndpointData fast path correctly: there would then + // be no "rx_loop legacy path" for the worker to conflict with. // Pass the buffer through by ownership + offset/length. No // per-packet allocation; rx_loop slices into `packet_data`. let _ = link_msg; // sanity-check borrow before sending buffer onward diff --git a/src/node/encrypt_worker.rs b/src/node/encrypt_worker.rs index 1c94a268..8f5e462d 100644 --- a/src/node/encrypt_worker.rs +++ b/src/node/encrypt_worker.rs @@ -56,15 +56,17 @@ use crate::transport::udp::io::AsyncUdpSocket; #[cfg(not(target_os = "macos"))] use crossbeam_channel::{Receiver, SendError, Sender, TrySendError, bounded}; use ring::aead::{Aad, LessSafeKey, Nonce}; +#[cfg(any(target_os = "macos", test))] +use std::collections::VecDeque; #[cfg(target_os = "macos")] -use std::collections::{BTreeMap, HashMap, VecDeque}; +use std::collections::{BTreeMap, HashMap}; use std::net::SocketAddr; #[cfg(unix)] use std::os::unix::io::AsRawFd; use std::sync::Arc; use std::sync::OnceLock; -#[cfg(target_os = "macos")] -use std::sync::{Condvar, Mutex}; +#[cfg(any(target_os = "macos", test))] +use std::sync::{Condvar, Mutex, PoisonError}; use tracing::{debug, trace, warn}; /// A pre-cooked FMP-encrypt-and-send job. All state-touching work @@ -218,43 +220,42 @@ impl QueuedFmpSendJob { /// same rationale as the bounded endpoint_commands channel upstream. const WORKER_CHANNEL_CAP: usize = 1024; -#[cfg(target_os = "macos")] -struct MacWorkerSender { - inner: Arc, +#[cfg(any(target_os = "macos", test))] +struct MacWorkerSender { + inner: Arc>, } -#[cfg(target_os = "macos")] -struct MacWorkerReceiver { - inner: Arc, +#[cfg(any(target_os = "macos", test))] +struct MacWorkerReceiver { + inner: Arc>, } -#[cfg(target_os = "macos")] -struct MacWorkerQueueInner { - state: Mutex, +#[cfg(any(target_os = "macos", test))] +struct MacWorkerQueueInner { + state: Mutex>, not_empty: Condvar, not_full: Condvar, cap: usize, } -#[cfg(target_os = "macos")] -#[derive(Default)] -struct MacWorkerQueueState { - queue: VecDeque, +#[cfg(any(target_os = "macos", test))] +struct MacWorkerQueueState { + queue: VecDeque, waiting: bool, closed: bool, } -#[cfg(target_os = "macos")] -enum MacWorkerTryPushError { - Full(Box), +#[cfg(any(target_os = "macos", test))] +enum MacWorkerTryPushError { + Full(Box), Closed, } -#[cfg(target_os = "macos")] +#[cfg(any(target_os = "macos", test))] struct MacWorkerPushError; -#[cfg(target_os = "macos")] -fn mac_worker_channel(cap: usize) -> (MacWorkerSender, MacWorkerReceiver) { +#[cfg(any(target_os = "macos", test))] +fn mac_worker_channel(cap: usize) -> (MacWorkerSender, MacWorkerReceiver) { let inner = Arc::new(MacWorkerQueueInner { state: Mutex::new(MacWorkerQueueState { queue: VecDeque::with_capacity(cap), @@ -273,9 +274,9 @@ fn mac_worker_channel(cap: usize) -> (MacWorkerSender, MacWorkerReceiver) { ) } -#[cfg(target_os = "macos")] -impl MacWorkerSender { - fn try_push(&self, job: QueuedFmpSendJob) -> Result<(), MacWorkerTryPushError> { +#[cfg(any(target_os = "macos", test))] +impl MacWorkerSender { + fn try_push(&self, job: T) -> Result<(), MacWorkerTryPushError> { let mut state = self .inner .state @@ -298,7 +299,7 @@ impl MacWorkerSender { Ok(()) } - fn push_blocking(&self, job: QueuedFmpSendJob) -> Result<(), MacWorkerPushError> { + fn push_blocking(&self, job: T) -> Result<(), MacWorkerPushError> { let mut state = self .inner .state @@ -328,8 +329,8 @@ impl MacWorkerSender { } } -#[cfg(target_os = "macos")] -impl Drop for MacWorkerSender { +#[cfg(any(target_os = "macos", test))] +impl Drop for MacWorkerSender { fn drop(&mut self) { let mut state = self .inner @@ -343,9 +344,34 @@ impl Drop for MacWorkerSender { } } -#[cfg(target_os = "macos")] -impl MacWorkerReceiver { - fn recv_batch(&self, batch: &mut Vec, max: usize) -> bool { +/// Closing from the receiver side matters because a sender waiting in +/// `push_blocking` on a full queue sleeps on `not_full`, and only the +/// receiver draining the queue wakes it. If the worker thread exits (a +/// panic unwinding included), nothing else would, and the rx_loop behind +/// that sender would block forever. +#[cfg(any(target_os = "macos", test))] +impl Drop for MacWorkerReceiver { + fn drop(&mut self) { + // This can run while the worker thread unwinds from a panic, where + // a second panic would abort the process, so tolerate poisoning. + let mut state = self + .inner + .state + .lock() + .unwrap_or_else(PoisonError::into_inner); + state.closed = true; + let queued = std::mem::take(&mut state.queue); + drop(state); + // Queued jobs hold key copies and sockets; free them outside the lock. + drop(queued); + self.inner.not_full.notify_all(); + self.inner.not_empty.notify_all(); + } +} + +#[cfg(any(target_os = "macos", test))] +impl MacWorkerReceiver { + fn recv_batch(&self, batch: &mut Vec, max: usize) -> bool { debug_assert!(batch.is_empty()); let mut state = self .inner @@ -378,7 +404,7 @@ impl MacWorkerReceiver { } #[cfg(target_os = "macos")] -type WorkerSender = MacWorkerSender; +type WorkerSender = MacWorkerSender; #[cfg(not(target_os = "macos"))] type WorkerSender = Sender; @@ -397,7 +423,7 @@ type WorkerSender = Sender; /// /// **Ordering: hash-by-destination** so single-flow TCP keeps its /// FIFO ordering (round-robin caused 8000 retransmits in an earlier -/// experiment — see the git log for the 56e0ca8 fix). Multi-peer / +/// experiment, which is why dispatch hashes by destination). Multi-peer / /// multi-flow benches still get parallelism since different /// destinations hash to different workers. #[derive(Clone)] @@ -955,7 +981,7 @@ fn run_worker(idx: usize, rx: Receiver) { } #[cfg(target_os = "macos")] -fn run_worker_macos(idx: usize, rx: MacWorkerReceiver) { +fn run_worker_macos(idx: usize, rx: MacWorkerReceiver) { trace!(worker = idx, "FMP encrypt worker thread starting"); let batch_size = macos_worker_batch_size(); @@ -2402,3 +2428,153 @@ fn send_one_raw( Ok(r as usize) } } + +/// Tests for the bounded worker queue the macOS encrypt pool uses. The +/// queue is generic, so these run on every platform against small item +/// types. Every wait is bounded so a regression fails instead of hanging. +#[cfg(test)] +mod mac_queue_tests { + use super::*; + use std::panic::{AssertUnwindSafe, catch_unwind}; + use std::sync::mpsc; + use std::thread; + use std::time::Duration; + + const WAIT: Duration = Duration::from_secs(5); + + /// Run `push_blocking(item)` on a helper thread and return a channel + /// that yields its result. + fn spawn_pusher( + tx: MacWorkerSender, + item: T, + ) -> mpsc::Receiver> { + let (done_tx, done_rx) = mpsc::channel(); + thread::spawn(move || { + let result = tx.push_blocking(item); + let _ = done_tx.send(result); + }); + done_rx + } + + #[test] + fn push_blocking_returns_error_when_worker_thread_panics_with_full_queue() { + let (tx, rx) = mac_worker_channel::(2); + assert!(tx.try_push(1).is_ok()); + assert!(tx.try_push(2).is_ok()); + match tx.try_push(9) { + Err(MacWorkerTryPushError::Full(job)) => assert_eq!(*job, 9), + _ => panic!("try_push on a full queue should hand the job back"), + } + + // The worker owns the receiver and dies without draining, once the + // pusher below has had time to start waiting for space. + let (die_tx, die_rx) = mpsc::channel::<()>(); + let worker = thread::spawn(move || { + let _rx = rx; + let _ = die_rx.recv(); + panic!("simulated encrypt worker panic"); + }); + let done = spawn_pusher(tx, 3); + thread::sleep(Duration::from_millis(200)); + assert!( + matches!(done.try_recv(), Err(mpsc::TryRecvError::Empty)), + "push_blocking returned while the queue was full and the worker alive" + ); + die_tx + .send(()) + .expect("worker thread gone before its signal"); + + let result = done + .recv_timeout(WAIT) + .expect("push_blocking still blocked after the worker thread died"); + assert!(matches!(result, Err(MacWorkerPushError))); + assert!(worker.join().is_err(), "worker thread should have panicked"); + } + + #[test] + fn try_push_returns_closed_after_receiver_dropped() { + let (tx, rx) = mac_worker_channel::(2); + drop(rx); + assert!(matches!(tx.try_push(1), Err(MacWorkerTryPushError::Closed))); + let done = spawn_pusher(tx, 2); + let result = done + .recv_timeout(WAIT) + .expect("push_blocking blocked on a queue whose receiver is gone"); + assert!(matches!(result, Err(MacWorkerPushError))); + } + + #[test] + fn receiver_drop_releases_queued_items() { + let marker = Arc::new(()); + let (tx, rx) = mac_worker_channel::>(4); + assert!(tx.try_push(Arc::clone(&marker)).is_ok()); + assert!(tx.try_push(Arc::clone(&marker)).is_ok()); + assert_eq!(Arc::strong_count(&marker), 3); + drop(rx); + assert_eq!( + Arc::strong_count(&marker), + 1, + "queued items must be freed when the receiver goes away" + ); + drop(tx); + } + + #[test] + fn push_blocking_completes_when_worker_drains_full_queue() { + let (tx, rx) = mac_worker_channel::(2); + assert!(tx.try_push(1).is_ok()); + assert!(tx.try_push(2).is_ok()); + let done = spawn_pusher(tx, 3); + thread::sleep(Duration::from_millis(100)); + assert!( + matches!(done.try_recv(), Err(mpsc::TryRecvError::Empty)), + "push_blocking returned while the queue was still full" + ); + + let mut batch = Vec::new(); + assert!(rx.recv_batch(&mut batch, 16)); + assert_eq!(batch, vec![1, 2]); + let result = done + .recv_timeout(WAIT) + .expect("push_blocking not woken after the worker drained"); + assert!(result.is_ok()); + + batch.clear(); + assert!(rx.recv_batch(&mut batch, 16)); + assert_eq!(batch, vec![3]); + } + + #[test] + fn recv_batch_drains_then_reports_closed_after_sender_drop() { + let (tx, rx) = mac_worker_channel::(4); + for i in 1..=3 { + assert!(tx.try_push(i).is_ok()); + } + drop(tx); + let mut batch = Vec::new(); + assert!(rx.recv_batch(&mut batch, 16)); + assert_eq!(batch, vec![1, 2, 3]); + batch.clear(); + assert!(!rx.recv_batch(&mut batch, 16)); + assert!(batch.is_empty()); + } + + #[test] + fn receiver_drop_does_not_panic_on_poisoned_lock() { + let (tx, rx) = mac_worker_channel::(2); + // The sender's own Drop still expects an unpoisoned lock, so it must + // never run here: a panic there while a failed assertion unwinds + // would abort the whole test binary. + let _tx = std::mem::ManuallyDrop::new(tx); + let inner = Arc::clone(&rx.inner); + let poisoner = thread::spawn(move || { + let _guard = inner.state.lock().unwrap(); + panic!("poison the queue lock"); + }); + assert!(poisoner.join().is_err()); + assert!(rx.inner.state.is_poisoned()); + + let dropped = catch_unwind(AssertUnwindSafe(move || drop(rx))); + assert!(dropped.is_ok(), "receiver drop panicked on a poisoned lock"); + } +} diff --git a/src/node/handlers/lookup.rs b/src/node/handlers/lookup.rs index 90a45208..88cae3c0 100644 --- a/src/node/handlers/lookup.rs +++ b/src/node/handlers/lookup.rs @@ -141,11 +141,11 @@ impl Node { } RequestOutcome::OwnRequestLooped => { // Our own flooded request, come back to us through a bloom - // false positive. Counted apart from ReqDuplicate: that one - // describes a peer resending, and this one describes our own - // fan-out returning, so folding them together would put a - // permanent healthy floor on a counter an operator reads as - // neighbour misbehaviour. + // false positive. Counted apart from ReqDuplicate by cause: + // this is our own fan-out returning, that is a request id we + // already recorded, seen again. Neither counter says which + // peer delivered the copy; this debug line is the only + // per-peer record. self.metrics() .lookup .record_reject(DiscoveryReject::ReqOwnLoopback); @@ -375,6 +375,9 @@ impl Node { now_ms, path_mtu, } => { + // Reports about the path this verified value replaces are + // not evidence against it. + self.broken_quorum.clear(&target); // The annotation is unsigned and accumulates hop by hop, so // any forwarder on the reverse path can lower it. A value // below the actionable floor cannot describe a usable path, diff --git a/src/node/handlers/rekey.rs b/src/node/handlers/rekey.rs index 7379c5c8..434f48fd 100644 --- a/src/node/handlers/rekey.rs +++ b/src/node/handlers/rekey.rs @@ -79,6 +79,36 @@ fn ladder_ms(rate_limit: &crate::config::RateLimitConfig) -> u64 { total } +/// The longest silence, in milliseconds, the node tolerates on a link it +/// keeps. +/// +/// A peer's handshake resend ladder, three ticks of scheduling slack, and +/// the longer of one heartbeat interval and the link-dead timeout. The +/// link-dead reap is suppressed while a rekey still has msg1 resends left, +/// which is what the ladder term covers. 64 s at stock settings. +pub(in crate::node) fn link_silence_ms(node: &crate::config::NodeConfig) -> u64 { + let after_ms = node + .heartbeat_interval_secs + .max(node.link_dead_timeout_secs) + .saturating_mul(1000); + ladder_ms(&node.rate_limit) + .saturating_add(node.tick_interval_secs.saturating_mul(3000)) + .saturating_add(after_ms) +} + +/// The inbound idle deadline for stream transports: how long an accepted +/// connection may go without delivering a complete frame once it has +/// delivered one. +/// +/// Set to `link_silence_ms`, so a connection carrying a link the node would +/// keep is never dropped by it, whatever the heartbeat, link-dead, tick and +/// resend settings are; a connection carrying no live link is reclaimed. +pub(in crate::node) fn inbound_idle_timeout( + node: &crate::config::NodeConfig, +) -> std::time::Duration { + std::time::Duration::from_millis(link_silence_ms(node)) +} + /// How long a rekey responder holds a pending session its initiator has /// not adopted before retiring it. /// @@ -92,18 +122,13 @@ fn ladder_ms(rate_limit: &crate::config::RateLimitConfig) -> u64 { /// interval later, and the link-dead reap if every frame from the /// initiator is lost. Both are allowed one more tick. Retiring before /// either turns a late but legitimate adoption into a split. That floor -/// is 64 s at stock settings; the drain ceiling, which bounds residence -/// for the same recovering-peer reason, is 120 s and is used unless the -/// configured timers push the floor above it. +/// is `link_silence_ms`, 64 s at stock settings; the drain ceiling, which +/// bounds residence for the same recovering-peer reason, is 120 s and is +/// used unless the configured timers push the floor above it. pub(in crate::node) fn pending_hold(node: &crate::config::NodeConfig) -> std::time::Duration { - let after_ms = node - .heartbeat_interval_secs - .max(node.link_dead_timeout_secs) - .saturating_mul(1000); - let floor_ms = ladder_ms(&node.rate_limit) - .saturating_add(node.tick_interval_secs.saturating_mul(3000)) - .saturating_add(after_ms); - std::time::Duration::from_millis(drain_max_retention_ms(&node.rate_limit).max(floor_ms)) + std::time::Duration::from_millis( + drain_max_retention_ms(&node.rate_limit).max(link_silence_ms(node)), + ) } impl Node { diff --git a/src/node/handlers/session.rs b/src/node/handlers/session.rs index cb343bce..4d4c1f7b 100644 --- a/src/node/handlers/session.rs +++ b/src/node/handlers/session.rs @@ -18,6 +18,7 @@ use crate::noise::{ use crate::proto::fmp::wire::{ ESTABLISHED_HEADER_SIZE, FLAG_KEY_EPOCH, FLAG_SP, build_established_header, }; +use crate::proto::fsp::quorum::QuorumVerdict; use crate::proto::fsp::wire::{ FSP_COMMON_PREFIX_SIZE, FSP_FLAG_CP, FSP_FLAG_K, FSP_HEADER_SIZE, FSP_PHASE_ESTABLISHED, FSP_PHASE_MSG1, FSP_PHASE_MSG2, FSP_PHASE_MSG3, FSP_PORT_HEADER_SIZE, FSP_PORT_IPV6_SHIM, @@ -193,7 +194,8 @@ impl Node { self.handle_coords_required(src_addr, error_body).await; } Some(RoutingSignalType::PathBroken) => { - self.handle_path_broken(src_addr, error_body).await; + self.handle_path_broken(src_addr, link_peer, error_body) + .await; } Some(RoutingSignalType::MtuExceeded) => { self.handle_mtu_exceeded(src_addr, error_body).await; @@ -1919,12 +1921,28 @@ impl Node { /// Handle a PathBroken error signal from a transit router. /// /// The router has coordinates but still can't route to the destination. - /// Send a standalone CoordsWarmup immediately (rate-limited), invalidate - /// cached coordinates, trigger re-discovery, and reset the warmup counter. + /// Send a standalone CoordsWarmup immediately (rate-limited), re-validate + /// the destination's coordinates by lookup, release its path MTU, and + /// reset the warmup counter. + /// + /// Cached coordinates that are only a hint are removed. Coordinates a + /// lookup verified are kept while the lookup re-validates them, and are + /// demoted to a hint only once reports arriving over distinct links reach + /// the quorum: the signal is unauthenticated, and deleting a verified + /// entry on one report is what let the next forged warm replace it. /// /// `src_addr` is the datagram's claimed source and is not - /// end-to-end authenticated; see `signal_verdict`. - pub(in crate::node) async fn handle_path_broken(&mut self, src_addr: &NodeAddr, inner: &[u8]) { + /// end-to-end authenticated; see `signal_verdict`. `link_peer` is the + /// authenticated peer the datagram arrived over. It is the report's vote + /// in the quorum, because the body's reporter is whatever the sender + /// wrote, and it is compared with the forward path to count mismatches; + /// it never refuses the signal. + pub(in crate::node) async fn handle_path_broken( + &mut self, + src_addr: &NodeAddr, + link_peer: &NodeAddr, + inner: &[u8], + ) { self.metrics().errors.path_broken.inc(); let msg = match PathBroken::decode(inner) { @@ -1936,11 +1954,10 @@ impl Node { }; // The premise: this signal carries no end-to-end authentication, so - // the body's `dest_addr` is attacker-chosen. `plan_path_broken` emits - // its coord-cache invalidation unconditionally, and the path-MTU - // release below is likewise unguarded, so both act on whatever address - // the body names unless the gate refuses it here, in the shell, which - // is the only layer that knows who sent the datagram. + // the body's `dest_addr` is attacker-chosen. The coord-cache action, + // the lookup and the path-MTU release below all act on whatever + // address the body names unless the gate refuses it here, in the + // shell, which is the only layer that knows who sent the datagram. let verdict = self.signal_verdict(src_addr, &msg.dest_addr); if verdict != SignalVerdict::Admit { debug!(src = %src_addr, dest = %msg.dest_addr, reporter = %msg.reporter, @@ -1960,6 +1977,7 @@ impl Node { reporter = %msg.reporter, "PathBroken: transit router reports routing failure" ); + self.count_path_broken_mismatches(link_peer, &msg); // Send standalone CoordsWarmup immediately (rate-limited) if self @@ -1978,46 +1996,69 @@ impl Node { "PathBroken response rate-limited, skipping standalone CoordsWarmup"); } - // Invalidate stale cached coordinates, then (only if the target's - // identity is cached — else the LookupResponse proof cannot be verified, - // e.g. when the XK responder receives PathBroken before msg3 completes) - // trigger re-discovery. The core emits invalidate-then-lookup in order. - let has_cached_identity = self.has_cached_identity(&msg.dest_addr); - let actions = self - .fsp - .plan_path_broken(msg.dest_addr, has_cached_identity); + // Only a live verified entry has anything for the quorum to protect, + // so only a report against one is recorded; that also bounds the + // quorum's keys by the destinations this node has looked up. The vote + // is the link the report arrived over, so a sender on one link counts + // once however many reporters it names. + let now = Self::now_ms(); + let verified = self + .coord_cache + .get_entry(&msg.dest_addr) + .is_some_and(|e| e.is_verified(now)); + let quorum = if verified { + self.broken_quorum.record(msg.dest_addr, *link_peer, now) + } else { + QuorumVerdict::Below { distinct: 0 } + }; + let actions = self.fsp.plan_path_broken(msg.dest_addr, verified, quorum); for action in actions { match action { FspAction::InvalidateCoords { addr } => { self.coord_cache.remove(&addr); } + FspAction::DemoteCoords { addr } => { + self.coord_cache.demote(&addr); + self.metrics().errors.broken_demoted.inc(); + debug!(dest = %addr, + "PathBroken quorum reached; demoted verified coordinates to a hint"); + } FspAction::InitiateLookup { dest } => { + self.cache_session_identity(&dest); self.maybe_initiate_lookup(&dest).await; } _ => {} } } + if let QuorumVerdict::Below { distinct } = quorum + && verified + { + self.metrics().errors.broken_below_quorum.inc(); + debug!(dest = %msg.dest_addr, distinct, + "PathBroken below quorum; keeping verified coordinates pending the lookup"); + } + // The path this destination's stored MTU described is gone, so release // it rather than carrying it onto whatever path replaces it. Rate // limited per destination on its own budget: PathBroken is // unauthenticated, and an unlimited release discards a genuinely // learned bottleneck as fast as it is relearned. The budget is not // shared with any other signal, so nothing else can spend it. + // + // A cache entry kept above still carries the MTU the lookup stored + // with it, which describes the same path; clear it with the map so + // the two do not disagree. if self .path_mtu_release_limiter .should_send(&msg.dest_addr, Self::now_ms()) { self.path_mtu_lookup_release(&msg.dest_addr); + self.coord_cache.clear_path_mtu(&msg.dest_addr); } else { trace!(dest = %msg.dest_addr, "PathBroken path MTU release rate-limited, keeping the stored value"); } - if !has_cached_identity { - debug!(dest = %msg.dest_addr, - "Skipping discovery after PathBroken: no cached identity for target"); - } - // Reset coords warmup counter so the next N packets include // COORDS_PRESENT, re-warming transit caches along the new path. let n = self.config().node.session.coords_warmup_packets; @@ -2031,6 +2072,75 @@ impl Node { } } + /// Cache the identity of `dest` from its session entry if the identity + /// cache has none. + /// + /// A lookup's answer is verified against the target's cached key, so a + /// lookup for a destination whose identity has been evicted runs to its + /// timeout and drops the packets queued for it as unreachable. A session + /// already holds the key: the one this node initiated to, or the one the + /// responder handshake authenticated. + fn cache_session_identity(&mut self, dest: &NodeAddr) { + if self.has_cached_identity(dest) { + return; + } + if let Some(pubkey) = self.sessions.get(dest).map(|e| *e.remote_pubkey()) { + self.register_identity(*dest, pubkey); + } + } + + /// Count, without acting on, the two ways an admitted PathBroken can + /// disagree with this node's own view of the path. + /// + /// Advisory only: both checks read state an attacker can influence and + /// both have a non-zero healthy floor, so they size the problem rather + /// than refuse anything. + /// + /// - Link: the signal arrived over a link other than the one this node + /// would forward to the destination on. A genuine report can also do + /// this when the reverse path differs from the forward one. + /// - Reporter: the reporter is this node or the destination, or its known + /// coordinates are not strictly closer to the destination than this + /// node's. Forwarding makes strict progress under each forwarder's own + /// view of the destination, and a PathBroken arises exactly where that + /// view may differ from this node's, so a genuine report can count here + /// too. A reporter's coordinates are rarely known unless it is a direct + /// peer, so this mostly reads as unknown and is not counted. + fn count_path_broken_mismatches(&self, link_peer: &NodeAddr, msg: &PathBroken) { + let now = Self::now_ms(); + let dest = &msg.dest_addr; + let reporter = &msg.reporter; + + if let (Some(hop), _) = self.preview_next_hop(dest, now) + && hop.node_addr != *link_peer + { + self.metrics().errors.broken_link_mismatch.inc(); + debug!(dest = %dest, link_peer = %link_peer, next_hop = %hop.node_addr, + "PathBroken arrived off the forward link"); + } + + let implausible = if reporter == self.node_addr() || reporter == dest { + true + } else { + let dest_coords = self.coord_cache.get(dest, now); + let reporter_coords = self + .tree_state + .peer_coords(reporter) + .or_else(|| self.coord_cache.get(reporter, now)); + match (dest_coords, reporter_coords) { + (Some(d), Some(r)) => { + r.distance_to(d) >= self.tree_state.my_coords().distance_to(d) + } + _ => false, + } + }; + if implausible { + self.metrics().errors.broken_reporter_mismatch.inc(); + debug!(dest = %dest, reporter = %reporter, + "PathBroken reporter is not closer to the destination than this node"); + } + } + /// Handle an MtuExceeded error signal from a transit router. /// /// A transit router couldn't forward our packet because it exceeded the diff --git a/src/node/metrics.rs b/src/node/metrics.rs index 77f9ca6d..2ea8039e 100644 --- a/src/node/metrics.rs +++ b/src/node/metrics.rs @@ -1,10 +1,10 @@ //! Lock-free metric counters backed by atomics. //! //! Mirrors the `NodeStats` counter surface (`stats.rs`) but stores each -//! counter in an `AtomicU64`, so it can be bumped through `&self` and, in -//! a later step, sampled without dispatching through the rx_loop task. The -//! hottest counters are cache-line padded to avoid false sharing once -//! reads move off-thread. +//! counter in an `AtomicU64`, so it can be bumped through `&self` and read +//! off the rx_loop task (the control read handle serves `show_metrics` from +//! it directly). The hottest counters are cache-line padded to avoid false +//! sharing between the writer and off-thread readers. //! //! The forwarding, discovery, tree, bloom, congestion, and error families //! live here exclusively and are both written and served from the registry. @@ -43,11 +43,10 @@ impl Counter { /// Cache-line padding wrapper for the hottest counters. /// -/// Padding keeps a hot counter off shared cache lines so that concurrent -/// reads (introduced when metric sampling moves off the rx_loop task) do -/// not false-share with the writer. With a single writer today the padding -/// is forward-looking insurance. Derefs to the inner counter so the call -/// sites are identical to an unpadded one. +/// Padding keeps a hot counter off shared cache lines so that reads from +/// off the rx_loop task (the control read handle) do not false-share with +/// the single writer. Derefs to the inner counter so the call sites are +/// identical to an unpadded one. #[repr(align(64))] #[derive(Default)] pub struct Padded(pub T); @@ -536,6 +535,31 @@ pub struct ErrorMetrics { /// rising count is a forged or stale reactive signal. pub mtu_exceeded_uncorroborated: Counter, pub unbound: UnboundSignals, + /// Admitted `PathBroken` signals against coordinates a lookup verified + /// that left them in place, because reports over distinct links had not + /// yet reached the quorum. The lookup still ran. A genuine failure's + /// reports usually arrive over one link, so this is the ordinary outcome + /// of a real broken path as well as of signals forged or reflected + /// through one neighbour. + pub broken_below_quorum: Counter, + /// Verified coordinates demoted to a hint because reports arriving over + /// distinct links reached the quorum. A genuine failure reported from two + /// directions produces this. Forged reports produce it only when they + /// arrive over two different links. + pub broken_demoted: Counter, + /// Admitted `PathBroken` signals that arrived over a link other than the + /// one this node would forward to the destination on. Counted, never + /// refused. The healthy floor is not zero: 4 to 10 percent of genuine + /// reports arrived off the forward link in a loopback measurement, and a + /// real topology with asymmetric paths will see more. + pub broken_link_mismatch: Counter, + /// Admitted `PathBroken` signals whose reporter is this node, the + /// destination, or a node whose known coordinates are no closer to the + /// destination than this node's. Counted, never refused. The healthy + /// floor is not zero, since the reporter's view of the destination can + /// differ from this node's, and most reporters' coordinates are unknown + /// here and are not counted at all. + pub broken_reporter_mismatch: Counter, /// Routing errors this node declined to emit because the authenticated /// link peer that induced them had spent its budget. A rising count is /// either a peer flooding unroutable traffic or a hub relaying more @@ -565,6 +589,10 @@ impl ErrorMetrics { unbound_broken: self.unbound.broken.get(), unbound_mtu: self.unbound.mtu.get(), unbound_forged: self.unbound.forged.get(), + broken_below_quorum: self.broken_below_quorum.get(), + broken_demoted: self.broken_demoted.get(), + broken_link_mismatch: self.broken_link_mismatch.get(), + broken_reporter_mismatch: self.broken_reporter_mismatch.get(), emit_over_peer_budget: self.emit_over_peer_budget.get(), emit_over_dest_interval: self.emit_over_dest_interval.get(), emit_limiter_at_capacity: self.emit_limiter_at_capacity.get(), diff --git a/src/node/mod.rs b/src/node/mod.rs index b561235d..2ec6ebfd 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -55,6 +55,7 @@ use crate::proto::fmp::wire::{ build_established_header, prepend_inner_header, }; use crate::proto::fsp::Fsp; +use crate::proto::fsp::quorum::LinkQuorum; use crate::proto::lookup::{Lookup, LookupBackoff, LookupForwardRateLimiter}; use crate::proto::mmp::Mmp; use crate::proto::routing::{self, Router, RoutingErrorRateLimiter}; @@ -668,6 +669,11 @@ pub struct Node { /// not a bound on this one, and one PathBroken drives both responses, so /// a shared limiter would let the coord-warmup arm pay for the release. path_mtu_release_limiter: RoutingErrorRateLimiter, + /// Distinct links that recently delivered a PathBroken naming a + /// destination whose coordinates a lookup verified. A verified entry is + /// demoted only when this reaches its quorum; any number of reports over + /// one link leave it in place while the lookup re-validates it. + broken_quorum: LinkQuorum, // === Peering Homeostasis === /// Owner of the peering-reconciler state relocated off `Node`: the sans-IO @@ -930,6 +936,7 @@ impl Node { path_mtu_release_limiter: RoutingErrorRateLimiter::with_interval_ms( handlers::session::PATH_MTU_RELEASE_MIN_INTERVAL.as_millis() as u64, ), + broken_quorum: LinkQuorum::new(), probes: handlers::probe::ProbeRegistry::new(), lookup: Lookup::new( LookupBackoff::with_params(backoff_base_secs, backoff_max_secs), @@ -1099,6 +1106,7 @@ impl Node { path_mtu_release_limiter: RoutingErrorRateLimiter::with_interval_ms( handlers::session::PATH_MTU_RELEASE_MIN_INTERVAL.as_millis() as u64, ), + broken_quorum: LinkQuorum::new(), probes: handlers::probe::ProbeRegistry::new(), lookup: Lookup::new(LookupBackoff::new(), LookupForwardRateLimiter::new()), discovery_sign_limiter: LookupSignRateLimiter::new(), @@ -1208,10 +1216,15 @@ impl Node { // raising `node.limits.max_connections` actually raises the inbound // ceiling rather than being silently capped at the transport default. let node_max_connections = self.config().node.limits.max_connections; + // Inbound stream connections are dropped after this long without a + // complete frame. Derived from the node's own liveness timers so it + // cannot drop a connection whose link the node would keep. + let idle_timeout = handlers::rekey::inbound_idle_timeout(&self.config().node); for (name, tcp_config) in tcp_instances { let transport_id = self.allocate_transport_id(); let mut tcp = TcpTransport::new(transport_id, name, tcp_config, packet_tx.clone()); tcp.set_node_max_connections(node_max_connections); + tcp.set_inbound_idle_timeout(idle_timeout); transports.push(TransportHandle::Tcp(tcp)); } @@ -1226,7 +1239,8 @@ impl Node { for (name, tor_config) in tor_instances { let transport_id = self.allocate_transport_id(); - let tor = TorTransport::new(transport_id, name, tor_config, packet_tx.clone()); + let mut tor = TorTransport::new(transport_id, name, tor_config, packet_tx.clone()); + tor.set_inbound_idle_timeout(idle_timeout); transports.push(TransportHandle::Tor(tor)); } @@ -1890,7 +1904,8 @@ impl Node { // advance together. What is published is data, not a rendered response, // and it is published only here, rather than as a monolithic per-tick // rebuild of every query's result. It also is not gated behind any slow - // I/O on the tick the way the abandoned 2edc8a1 republish was. + // I/O on the tick, which was the shape of an earlier, abandoned + // republish design. // Per-stats-history-peer metadata. `show_stats_peers` / // `show_stats_history_all_peers` need each tracked peer's live // membership (`is_active`), resolved npub, and display name — all diff --git a/src/node/reject.rs b/src/node/reject.rs index 34ed5e32..ab2a270a 100644 --- a/src/node/reject.rs +++ b/src/node/reject.rs @@ -123,9 +123,12 @@ pub enum DiscoveryReject { /// forwarded to the peer that looped it. Tracked via /// [`DiscoveryStats::req_own_loopback`](crate::node::stats::DiscoveryStats). /// - /// **This has a nonzero floor in healthy operation** and rises with the - /// bloom fill ratio. It says nothing about the peer that delivered the - /// copy, which is why it is not counted as [`Self::ReqDuplicate`]. + /// **This can occur in healthy operation**: a bloom false positive is + /// enough to send a copy back, so the rate rises with the bloom fill + /// ratio. It is counted apart from [`Self::ReqDuplicate`] by cause: this + /// is the node's own fan-out returning, while `ReqDuplicate` is a request + /// id the node has already recorded, seen again. Neither identifies the + /// peer that delivered the copy. ReqOwnLoopback, /// Request dedup cache (`recent_requests`) is at capacity, so the /// `LookupRequest` is dropped without being forwarded. Tracked via diff --git a/src/node/stats.rs b/src/node/stats.rs index e4fd7d04..d85fed07 100644 --- a/src/node/stats.rs +++ b/src/node/stats.rs @@ -460,6 +460,10 @@ pub struct ErrorSignalStatsSnapshot { pub unbound_broken: u64, pub unbound_mtu: u64, pub unbound_forged: u64, + pub broken_below_quorum: u64, + pub broken_demoted: u64, + pub broken_link_mismatch: u64, + pub broken_reporter_mismatch: u64, pub emit_over_peer_budget: u64, pub emit_over_dest_interval: u64, pub emit_limiter_at_capacity: u64, diff --git a/src/node/tests/coord_forgery.rs b/src/node/tests/coord_forgery.rs new file mode 100644 index 00000000..879eb20e --- /dev/null +++ b/src/node/tests/coord_forgery.rs @@ -0,0 +1,860 @@ +//! Forged routing signals against a destination whose coordinates a lookup +//! verified. +//! +//! The attack these tests describe: a `PathBroken` naming a destination this +//! node has a session with, followed by a forged `SessionSetup` carrying a +//! different position for that destination under the same root. Every packet +//! is driven through `handle_session_datagram`, the entry point a real one +//! reaches, and the observable is the next hop `find_next_hop` picks. + +use super::*; +use crate::node::session::EndToEndState; +use crate::node::tests::spanning_tree::{TestNode, cleanup_nodes, run_tree_test}; +use crate::proto::fsp::SessionSetup; +use crate::proto::link::SessionDatagram; +use crate::proto::routing::PathBroken; +use crate::proto::stp::TreeCoordinate; + +/// Index of the victim in the fixture's node vector. +const V: usize = 0; +/// Index of the peer the destination genuinely sits under. +const P1: usize = 1; +/// Index of the peer the forged coordinates point at. +const P2: usize = 2; + +/// The victim, its two tree peers, and a destination it holds a session +/// with whose real and forged coordinates differ in the peer they hang off. +struct Fixture { + nodes: Vec, + dest: NodeAddr, + p1: NodeAddr, + p2: NodeAddr, + real: TreeCoordinate, + forged: TreeCoordinate, +} + +/// Wall-clock milliseconds, the clock `handle_path_broken` and +/// `find_next_hop` read. A verification stamped on any other clock reads as +/// aged out to the handler. +fn wall_ms() -> u64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_millis() as u64 +} + +/// `dest` hung directly under `parent`, sharing its root. +fn coords_under(dest: NodeAddr, parent: &TreeCoordinate) -> TreeCoordinate { + let mut addrs = vec![dest]; + addrs.extend(parent.node_addrs().copied()); + TreeCoordinate::from_addrs(addrs).unwrap() +} + +/// Install the entry `initiate_session` creates for `remote`, which is what +/// makes the admission gate accept a signal naming it. +fn install_initiating(node: &mut Node, remote: &Identity) { + use crate::noise::HandshakeState; + + let handshake = + HandshakeState::new_xk_initiator(node.identity().keypair(), remote.pubkey_full()); + let entry = crate::node::session::SessionEntry::new( + *remote.node_addr(), + remote.pubkey_full(), + EndToEndState::Initiating(handshake), + 1000, + true, + ); + node.sessions.insert(*remote.node_addr(), entry); +} + +/// Build the fixture and assert the two preconditions every test relies on. +async fn fixture() -> Fixture { + let mut nodes = run_tree_test(3, &[(V, P1), (V, P2)], false).await; + let p1 = *nodes[P1].node.node_addr(); + let p2 = *nodes[P2].node.node_addr(); + + let remote = Identity::generate(); + let dest = *remote.node_addr(); + install_initiating(&mut nodes[V].node, &remote); + + let real = coords_under(dest, nodes[P1].node.tree_state().my_coords()); + let forged = coords_under(dest, nodes[P2].node.tree_state().my_coords()); + nodes[V] + .node + .coord_cache_mut() + .insert_verified(dest, real.clone(), wall_ms()); + + let mut fx = Fixture { + nodes, + dest, + p1, + p2, + real, + forged, + }; + assert_eq!( + fx.next_hop(), + Some(fx.p1), + "precondition: the verified coordinates route via P1" + ); + let forged_hop = fx.nodes[V] + .node + .tree_state() + .find_next_hop(&fx.forged, &std::collections::BTreeSet::new()); + assert_eq!( + forged_hop, + Some(fx.p2), + "precondition: the forged coordinates would route via P2, so a \ + successful plant is visible as a flip" + ); + fx +} + +impl Fixture { + /// The next hop the victim picks toward the destination. + fn next_hop(&mut self) -> Option { + let dest = self.dest; + self.nodes[V] + .node + .find_next_hop(&dest) + .map(|p| *p.node_addr()) + } + + /// The victim's cache entry for the destination: its value and whether it + /// is still verified on the handler's clock. + fn entry(&self) -> Option<(TreeCoordinate, bool)> { + self.nodes[V] + .node + .coord_cache() + .get_entry(&self.dest) + .map(|e| (e.coords().clone(), e.is_verified(wall_ms()))) + } + + /// Deliver a `PathBroken` naming the destination, claiming `reporter` as + /// both the datagram source and the body's reporter, arriving over the + /// link to `link_peer`. + async fn path_broken(&mut self, reporter: NodeAddr, link_peer: NodeAddr) { + self.path_broken_from(reporter, reporter, link_peer).await; + } + + /// Deliver a `PathBroken` naming the destination from datagram source + /// `src`, whose body names `reporter`, arriving over the link to + /// `link_peer`. Both identities are the sender's to choose. + async fn path_broken_from(&mut self, src: NodeAddr, reporter: NodeAddr, link_peer: NodeAddr) { + let victim = *self.nodes[V].node.node_addr(); + let payload = PathBroken::new(self.dest, reporter).encode(); + let encoded = SessionDatagram::new(src, victim, payload).encode(); + self.nodes[V] + .node + .handle_session_datagram(&link_peer, &encoded[1..], false) + .await; + } + + /// Deliver a forged `SessionSetup` in transit, claiming to be from the + /// destination and carrying the forged coordinates for it, over the link + /// to P2. + async fn forged_setup(&mut self) { + let payload = SessionSetup::new(self.forged.clone(), self.forged.clone()).encode(); + let encoded = SessionDatagram::new(self.dest, self.dest, payload).encode(); + let p2 = self.p2; + self.nodes[V] + .node + .handle_session_datagram(&p2, &encoded[1..], false) + .await; + } +} + +/// One forged warm against a verified destination changes nothing: the +/// precedence rule refuses the hint, and the route stays on P1. +#[tokio::test] +async fn a_forged_same_root_warm_does_not_move_the_route_to_a_verified_destination() { + let mut fx = fixture().await; + let rejected = fx.nodes[V] + .node + .metrics() + .forwarding + .coord_hint_rejected + .get(); + + fx.forged_setup().await; + + assert_eq!(fx.next_hop(), Some(fx.p1), "a forged warm moved the route"); + assert!( + fx.nodes[V] + .node + .metrics() + .forwarding + .coord_hint_rejected + .get() + > rejected, + "the refused hint should be counted" + ); + cleanup_nodes(&mut fx.nodes).await; +} + +/// One forged `PathBroken` followed by one forged warm: the two-packet attack. +/// The signal must not strip the verification that refuses the warm. +#[tokio::test] +async fn a_forged_path_broken_then_a_forged_warm_does_not_move_the_route_to_a_verified_destination() +{ + let mut fx = fixture().await; + let p2 = fx.p2; + + fx.path_broken(make_node_addr(0xB1), p2).await; + fx.forged_setup().await; + + assert_eq!( + fx.next_hop(), + Some(fx.p1), + "the forged warm moved the route" + ); + assert_eq!( + fx.entry(), + Some((fx.real.clone(), true)), + "the verified entry must survive one PathBroken with its value and \ + its verification" + ); + let errors = &fx.nodes[V].node.metrics().errors; + assert_eq!(errors.broken_below_quorum.get(), 1); + assert_eq!(errors.broken_demoted.get(), 0); + cleanup_nodes(&mut fx.nodes).await; +} + +/// A direct forger on one link inventing two reporters is still one vote: the +/// entry stays verified and the forged warm that follows is refused. Keyed on +/// the body's reporter, the two invented names would have reached the quorum +/// and the warm would have moved the route. +#[tokio::test] +async fn a_forger_inventing_two_reporters_over_one_link_does_not_demote_a_verified_entry() { + let mut fx = fixture().await; + let p2 = fx.p2; + + fx.path_broken(make_node_addr(0xB1), p2).await; + fx.path_broken(make_node_addr(0xB2), p2).await; + fx.forged_setup().await; + + assert_eq!( + fx.entry(), + Some((fx.real.clone(), true)), + "two invented reporters over one link demoted the verified entry" + ); + assert_eq!( + fx.next_hop(), + Some(fx.p1), + "the forged warm moved the route" + ); + let errors = &fx.nodes[V].node.metrics().errors; + assert_eq!(errors.broken_below_quorum.get(), 2); + assert_eq!(errors.broken_demoted.get(), 0); + cleanup_nodes(&mut fx.nodes).await; +} + +/// Two genuinely different reporters whose signals both arrive over one link +/// are one vote too: what counts is the authenticated link, not the +/// reporter, nor the pair of the two. +#[tokio::test] +async fn two_reporters_over_the_same_link_do_not_reach_the_quorum() { + let mut fx = fixture().await; + let (p1, p2) = (fx.p1, fx.p2); + + fx.path_broken(p1, p1).await; + fx.path_broken(p2, p1).await; + + assert_eq!(fx.entry(), Some((fx.real.clone(), true))); + let errors = &fx.nodes[V].node.metrics().errors; + assert_eq!(errors.broken_below_quorum.get(), 2); + assert_eq!(errors.broken_demoted.get(), 0); + cleanup_nodes(&mut fx.nodes).await; +} + +/// Reports arriving over two different links reach the quorum and demote the +/// entry, and the forged warm that follows then moves the route. This is the +/// residual the quorum leaves: forged reports demote the entry once they +/// arrive over two different links, as a genuine failure reported from two +/// directions does. Asserted so that it stays visible. +#[tokio::test] +async fn reports_over_two_different_links_demote_a_verified_entry_and_a_following_warm_then_moves_the_route() + { + let mut fx = fixture().await; + let (p1, p2) = (fx.p1, fx.p2); + + fx.path_broken(make_node_addr(0xB1), p1).await; + fx.path_broken(make_node_addr(0xB2), p2).await; + + assert_eq!( + fx.entry(), + Some((fx.real.clone(), false)), + "a demoted entry keeps its value and loses its verification" + ); + assert_eq!( + fx.nodes[V] + .node + .coord_cache() + .get_entry(&fx.dest) + .map(|e| e.source()), + Some(crate::cache::CoordSource::Hint) + ); + assert_eq!( + fx.next_hop(), + Some(fx.p1), + "demotion alone does not move the route" + ); + let errors = &fx.nodes[V].node.metrics().errors; + assert_eq!(errors.broken_demoted.get(), 1); + assert_eq!(errors.broken_below_quorum.get(), 1); + + fx.forged_setup().await; + + assert_eq!( + fx.next_hop(), + Some(fx.p2), + "once demoted, the entry is a hint and a forged warm replaces it" + ); + cleanup_nodes(&mut fx.nodes).await; +} + +/// A node with a single peer receives every report over one link, so it never +/// demotes by quorum, however many reporters the reports name. What bounds a +/// stale verified entry there is its verification age: one still inside +/// `VERIFIED_TTL_MS` is kept, one past it is removed by the next report like +/// any hint. +#[tokio::test] +async fn on_a_single_link_node_only_the_verification_age_bounds_a_verified_entry() { + use crate::cache::VERIFIED_TTL_MS; + + // V - P1, and V holds a session with a destination beyond P1. + let mut nodes = run_tree_test(2, &[(V, P1)], false).await; + let p1 = *nodes[P1].node.node_addr(); + let victim = *nodes[V].node.node_addr(); + let remote = Identity::generate(); + let dest = *remote.node_addr(); + install_initiating(&mut nodes[V].node, &remote); + let coords = coords_under(dest, nodes[P1].node.tree_state().my_coords()); + + let report = |reporter: NodeAddr| { + let payload = PathBroken::new(dest, reporter).encode(); + SessionDatagram::new(reporter, victim, payload).encode() + }; + let verified = |node: &Node| { + node.coord_cache() + .get_entry(&dest) + .map(|e| e.is_verified(wall_ms())) + }; + + // A cache TTL long enough that expiry never removes the entry, so every + // removal below is the handler's. Ten seconds of margin inside the + // bound, so the test's own run time cannot age the entry out before the + // reports land. + let cache = nodes[V].node.coord_cache_mut(); + cache.set_default_ttl_ms(4 * VERIFIED_TTL_MS); + cache.insert_verified(dest, coords.clone(), wall_ms() - VERIFIED_TTL_MS + 10_000); + for r in 0..5u8 { + let encoded = report(make_node_addr(0xB0 + r)); + nodes[V] + .node + .handle_session_datagram(&p1, &encoded[1..], false) + .await; + } + assert_eq!( + verified(&nodes[V].node), + Some(true), + "reports over one link demoted or removed an entry still inside its \ + verification" + ); + let errors = &nodes[V].node.metrics().errors; + assert_eq!(errors.broken_below_quorum.get(), 5); + assert_eq!(errors.broken_demoted.get(), 0); + + // Past the bound the entry no longer refuses anything, and the next + // report removes it. + nodes[V].node.coord_cache_mut().insert_verified( + dest, + coords, + wall_ms() - VERIFIED_TTL_MS - 1_000, + ); + assert_eq!( + verified(&nodes[V].node), + Some(false), + "precondition: the entry is present and its verification has aged out" + ); + let encoded = report(make_node_addr(0xC0)); + nodes[V] + .node + .handle_session_datagram(&p1, &encoded[1..], false) + .await; + assert_eq!( + verified(&nodes[V].node), + None, + "an aged-out entry is removed" + ); + cleanup_nodes(&mut nodes).await; +} + +/// One reporter repeating itself over one link is one vote: the entry stays +/// verified and the forged warm is still refused. +#[tokio::test] +async fn one_reporter_repeating_a_path_broken_does_not_reach_the_quorum() { + let mut fx = fixture().await; + let p2 = fx.p2; + + fx.path_broken(make_node_addr(0xB1), p2).await; + fx.path_broken(make_node_addr(0xB1), p2).await; + fx.forged_setup().await; + + assert_eq!(fx.next_hop(), Some(fx.p1)); + assert_eq!(fx.entry(), Some((fx.real.clone(), true))); + let errors = &fx.nodes[V].node.metrics().errors; + assert_eq!(errors.broken_below_quorum.get(), 2); + assert_eq!(errors.broken_demoted.get(), 0); + cleanup_nodes(&mut fx.nodes).await; +} + +/// A verification that has aged out protects nothing, so the entry is +/// removed as a hint would be. +#[tokio::test] +async fn a_path_broken_removes_a_verified_entry_whose_verification_has_aged_out() { + let mut fx = fixture().await; + let dest = fx.dest; + let real = fx.real.clone(); + let p1 = fx.p1; + let cache = fx.nodes[V].node.coord_cache_mut(); + // A TTL long enough that the entry is still live when its verification + // is not, so the removal below is the handler's and not expiry's. + cache.set_default_ttl_ms(4 * crate::cache::VERIFIED_TTL_MS); + cache.insert_verified( + dest, + real, + wall_ms() - crate::cache::VERIFIED_TTL_MS - 1_000, + ); + assert_eq!( + fx.entry().map(|(_, verified)| verified), + Some(false), + "precondition: the entry is present and no longer verified" + ); + + fx.path_broken(make_node_addr(0xB1), p1).await; + + assert!(fx.entry().is_none(), "an unverified entry is removed"); + let errors = &fx.nodes[V].node.metrics().errors; + assert_eq!(errors.broken_below_quorum.get(), 0); + assert_eq!(errors.broken_demoted.get(), 0); + cleanup_nodes(&mut fx.nodes).await; +} + +/// When the path-MTU release fires, a kept entry forgets the MTU the lookup +/// stored with it, so it does not go on displaying a released value. +#[tokio::test] +async fn a_kept_entry_forgets_its_path_mtu_when_the_release_fires() { + let mut fx = fixture().await; + let dest = fx.dest; + let real = fx.real.clone(); + let p2 = fx.p2; + fx.nodes[V] + .node + .coord_cache_mut() + .insert_verified_with_path_mtu(dest, real, wall_ms(), 1200); + + fx.path_broken(make_node_addr(0xB1), p2).await; + + let entry = fx.nodes[V].node.coord_cache().get_entry(&dest).unwrap(); + assert!( + entry.is_verified(wall_ms()), + "precondition: the entry was kept" + ); + assert_eq!(entry.path_mtu(), None); + cleanup_nodes(&mut fx.nodes).await; +} + +/// When the path-MTU release is rate limited, a kept entry keeps its MTU, as +/// the path-MTU map does: the two describe the same path and must not +/// disagree. +#[tokio::test] +async fn a_kept_entry_keeps_its_path_mtu_when_the_release_is_rate_limited() { + let mut fx = fixture().await; + let dest = fx.dest; + let real = fx.real.clone(); + let p2 = fx.p2; + let fips = crate::FipsAddress::from_node_addr(&dest); + + // The first report spends the release budget for this destination. + fx.path_broken(make_node_addr(0xB1), p2).await; + + // A value learned again since, in both stores. + fx.nodes[V] + .node + .coord_cache_mut() + .insert_verified_with_path_mtu(dest, real, wall_ms(), 1200); + fx.nodes[V].node.path_mtu_lookup_insert(fips, 1200); + + // A second report inside the release interval (same link, so it stays + // below the quorum and the entry is kept). + fx.path_broken(make_node_addr(0xB2), p2).await; + + let node = &fx.nodes[V].node; + let entry = node.coord_cache().get_entry(&dest).unwrap(); + assert!( + entry.is_verified(wall_ms()), + "precondition: the entry was kept" + ); + assert_eq!( + node.path_mtu_lookup_get(&fips), + Some(1200), + "precondition: the release was rate limited" + ); + assert_eq!( + entry.path_mtu(), + Some(1200), + "a rate-limited release cleared the kept entry's MTU but not the map's" + ); + cleanup_nodes(&mut fx.nodes).await; +} + +/// The sum of the lookup counters `maybe_initiate_lookup` moves on every +/// outcome, so a test can see that it ran whatever it decided. +fn lookup_attempts(node: &Node) -> u64 { + let l = &node.metrics().lookup; + l.req_initiated.get() + + l.req_bloom_miss.get() + + l.req_deduplicated.get() + + l.req_backoff_suppressed.get() +} + +/// An admitted `PathBroken` re-validates the destination by lookup even when +/// its identity is not cached, which is the fixture's state: the session entry +/// alone does not register the identity. +#[tokio::test] +async fn an_admitted_path_broken_starts_a_lookup_without_a_cached_identity() { + let mut fx = fixture().await; + let dest = fx.dest; + assert!( + !fx.nodes[V].node.has_cached_identity(&dest), + "precondition: the destination's identity is not cached" + ); + let before = lookup_attempts(&fx.nodes[V].node); + let p1 = fx.p1; + + fx.path_broken(make_node_addr(0xB1), p1).await; + + assert!( + lookup_attempts(&fx.nodes[V].node) > before, + "the PathBroken should have run the lookup path" + ); + cleanup_nodes(&mut fx.nodes).await; +} + +/// The lookup a PathBroken starts must be answerable when the destination's +/// identity is not cached. The session already holds the destination's key, +/// so the lookup's answer can be verified with it: the position is learned, +/// the lookup does not run to its timeout, and packets queued for the +/// destination are not dropped as unreachable when it would have. +#[tokio::test] +async fn a_path_broken_without_a_cached_identity_starts_a_lookup_this_node_can_verify() { + // V - P1 - D, with D a real node V has a session with but whose identity + // V has not cached. + let mut nodes = run_tree_test(3, &[(0, 1), (1, 2)], false).await; + let p1 = *nodes[1].node.node_addr(); + let dest = *nodes[2].node.node_addr(); + let dest_pubkey = nodes[2].node.identity().pubkey_full(); + let real: Vec = nodes[2] + .node + .tree_state() + .my_coords() + .node_addrs() + .copied() + .collect(); + let handshake = crate::noise::HandshakeState::new_xk_initiator( + nodes[0].node.identity().keypair(), + dest_pubkey, + ); + nodes[0].node.sessions.insert( + dest, + crate::node::session::SessionEntry::new( + dest, + dest_pubkey, + EndToEndState::Initiating(handshake), + 1000, + true, + ), + ); + assert!( + !nodes[0].node.has_cached_identity(&dest), + "precondition: the destination's identity is not cached" + ); + nodes[0] + .node + .queue_pending_tun_packet_for_test(dest, vec![0x60; 40]); + + let lookup = &nodes[0].node.metrics().lookup; + let (accepted, miss, timed_out) = ( + lookup.resp_accepted.get(), + lookup.resp_identity_miss.get(), + lookup.resp_timed_out.get(), + ); + + let victim = *nodes[0].node.node_addr(); + let payload = PathBroken::new(dest, p1).encode(); + let encoded = SessionDatagram::new(p1, victim, payload).encode(); + let start = wall_ms(); + nodes[0] + .node + .handle_session_datagram(&p1, &encoded[1..], false) + .await; + for _ in 0..10 { + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + crate::node::tests::spanning_tree::process_available_packets(&mut nodes).await; + } + + let lookup = &nodes[0].node.metrics().lookup; + assert_eq!( + lookup.resp_identity_miss.get(), + miss, + "the lookup's answer could not be verified for want of an identity" + ); + assert!( + lookup.resp_accepted.get() > accepted, + "the lookup's answer must be verified and accepted" + ); + let entry = nodes[0].node.coord_cache().get_entry(&dest).unwrap(); + assert_eq!( + entry.coords().node_addrs().copied().collect::>(), + real + ); + assert!(entry.is_verified(wall_ms())); + + // Run the lookup schedule well past its end: an answered lookup has + // nothing left to time out, so the queued packet survives. + for step in 1..=6 { + nodes[0] + .node + .check_pending_lookups(start + step * 20_000) + .await; + } + assert_eq!( + nodes[0].node.metrics().lookup.resp_timed_out.get(), + timed_out, + "the lookup ran to its timeout" + ); + assert_eq!( + nodes[0].node.pending_tun_total_packets(), + 1, + "the packet queued for the destination was dropped" + ); + cleanup_nodes(&mut nodes).await; +} + +/// The healthy path for keeping a verified entry below quorum: a genuine +/// failure reported once is still recovered, because the lookup the report +/// starts replaces the stale value with the destination's real position. +#[tokio::test] +async fn a_genuine_path_broken_still_replaces_stale_verified_coordinates_by_lookup() { + // V - P1 - D, with D a real node V has a session with, and a second peer + // P2 on its own, so a later report can arrive over a different link. + let mut nodes = run_tree_test(4, &[(0, 1), (1, 2), (0, 3)], false).await; + let p1 = *nodes[1].node.node_addr(); + let p2 = *nodes[3].node.node_addr(); + let dest = *nodes[2].node.node_addr(); + let dest_pubkey = nodes[2].node.identity().pubkey_full(); + let real: Vec = nodes[2] + .node + .tree_state() + .my_coords() + .node_addrs() + .copied() + .collect(); + nodes[0].node.register_identity(dest, dest_pubkey); + let handshake = crate::noise::HandshakeState::new_xk_initiator( + nodes[0].node.identity().keypair(), + dest_pubkey, + ); + nodes[0].node.sessions.insert( + dest, + crate::node::session::SessionEntry::new( + dest, + dest_pubkey, + EndToEndState::Initiating(handshake), + 1000, + true, + ), + ); + + // D "moved": V holds a verified position for it that is no longer true. + let stale = coords_under(dest, nodes[0].node.tree_state().my_coords()); + assert_ne!(stale.node_addrs().copied().collect::>(), real); + nodes[0] + .node + .coord_cache_mut() + .insert_verified(dest, stale, wall_ms()); + + let lookup = &nodes[0].node.metrics().lookup; + let (initiated, accepted) = (lookup.req_initiated.get(), lookup.resp_accepted.get()); + + let victim = *nodes[0].node.node_addr(); + let payload = PathBroken::new(dest, p1).encode(); + let encoded = SessionDatagram::new(p1, victim, payload).encode(); + nodes[0] + .node + .handle_session_datagram(&p1, &encoded[1..], false) + .await; + assert_eq!(nodes[0].node.metrics().errors.broken_below_quorum.get(), 1); + + for _ in 0..10 { + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + crate::node::tests::spanning_tree::process_available_packets(&mut nodes).await; + } + + let lookup = &nodes[0].node.metrics().lookup; + assert!( + lookup.req_initiated.get() > initiated, + "the report must start a lookup" + ); + assert!( + lookup.resp_accepted.get() > accepted, + "the lookup must be answered" + ); + let entry = nodes[0].node.coord_cache().get_entry(&dest).unwrap(); + assert_eq!( + entry.coords().node_addrs().copied().collect::>(), + real, + "the lookup must replace the stale position with the real one" + ); + assert!(entry.is_verified(wall_ms())); + + // The lookup verified the destination again, so the report about the old + // path no longer counts: a report over a second link now starts a new + // quorum rather than completing the old one. + let payload = PathBroken::new(dest, make_node_addr(0xB2)).encode(); + let encoded = SessionDatagram::new(make_node_addr(0xB2), victim, payload).encode(); + nodes[0] + .node + .handle_session_datagram(&p2, &encoded[1..], false) + .await; + let errors = &nodes[0].node.metrics().errors; + assert_eq!( + errors.broken_demoted.get(), + 0, + "a report from before the re-verification combined with one after it" + ); + assert_eq!(errors.broken_below_quorum.get(), 2); + cleanup_nodes(&mut nodes).await; +} + +/// A PathBroken arriving off the forward link is counted and still acted on +/// in full: the advisory check refuses nothing. +#[tokio::test] +async fn a_path_broken_off_the_forward_link_is_counted_and_still_acted_on() { + let mut fx = fixture().await; + let dest = fx.dest; + let p2 = fx.p2; + fx.nodes[V] + .node + .sessions + .get_mut(&dest) + .unwrap() + .set_coords_warmup_remaining(0); + let lookups = lookup_attempts(&fx.nodes[V].node); + + fx.path_broken(make_node_addr(0xB1), p2).await; + + let node = &fx.nodes[V].node; + assert_eq!(node.metrics().errors.broken_link_mismatch.get(), 1); + assert_eq!( + node.sessions.get(&dest).unwrap().coords_warmup_remaining(), + node.config().node.session.coords_warmup_packets, + "the warmup counter is reset whatever the link" + ); + assert!( + node.config().node.session.coords_warmup_packets > 0, + "precondition: a reset is distinguishable from the zero set above" + ); + assert!( + lookup_attempts(node) > lookups, + "the lookup runs whatever the link" + ); + assert_eq!(node.metrics().errors.broken_below_quorum.get(), 1); + cleanup_nodes(&mut fx.nodes).await; +} + +/// The same report over the forward link is not counted. +#[tokio::test] +async fn a_path_broken_over_the_forward_link_is_not_counted_as_a_mismatch() { + let mut fx = fixture().await; + let p1 = fx.p1; + + fx.path_broken(make_node_addr(0xB1), p1).await; + + let errors = &fx.nodes[V].node.metrics().errors; + assert_eq!(errors.broken_link_mismatch.get(), 0); + assert_eq!( + errors.broken_below_quorum.get(), + 1, + "the signal was admitted and handled, so the zero above is a real zero" + ); + cleanup_nodes(&mut fx.nodes).await; +} + +/// The reporter-mismatch count after one PathBroken naming the fixture's +/// destination, reported by the address `pick` chooses, over the forward link. +async fn reporter_mismatches(pick: impl FnOnce(&Fixture) -> NodeAddr) -> u64 { + mismatches_from(|fx| { + let reporter = pick(fx); + (reporter, reporter) + }) + .await +} + +/// The reporter-mismatch count after one PathBroken naming the fixture's +/// destination over the forward link, with the datagram source and the +/// body's reporter that `pick` returns, in that order. +async fn mismatches_from(pick: impl FnOnce(&Fixture) -> (NodeAddr, NodeAddr)) -> u64 { + let mut fx = fixture().await; + let (src, reporter) = pick(&fx); + let p1 = fx.p1; + fx.path_broken_from(src, reporter, p1).await; + let errors = &fx.nodes[V].node.metrics().errors; + assert_eq!( + errors.broken_below_quorum.get(), + 1, + "precondition: the signal reached the handler's verified-entry path, \ + so the count below observed it" + ); + let n = errors.broken_reporter_mismatch.get(); + cleanup_nodes(&mut fx.nodes).await; + n +} + +#[tokio::test] +async fn a_reporter_farther_from_the_destination_than_this_node_is_counted() { + // P2 is a direct peer on the far side of this node from P1, under which + // the destination sits. + assert_eq!(reporter_mismatches(|fx| fx.p2).await, 1); +} + +#[tokio::test] +async fn a_reporter_closer_to_the_destination_than_this_node_is_not_counted() { + assert_eq!(reporter_mismatches(|fx| fx.p1).await, 0); +} + +#[tokio::test] +async fn a_reporter_with_unknown_coordinates_is_not_counted() { + assert_eq!(reporter_mismatches(|_| make_node_addr(0xB1)).await, 0); +} + +#[tokio::test] +async fn this_node_named_as_the_reporter_is_counted() { + assert_eq!( + reporter_mismatches(|fx| *fx.nodes[V].node.node_addr()).await, + 1 + ); +} + +/// The destination named as its own reporter cannot be reporting a broken +/// path to itself. The datagram source differs from the reporter here, +/// because a datagram claiming to come from the destination is refused by the +/// admission gate before this check is reached. +#[tokio::test] +async fn the_destination_named_as_the_reporter_is_counted() { + assert_eq!( + mismatches_from(|fx| (make_node_addr(0xB1), fx.dest)).await, + 1 + ); +} diff --git a/src/node/tests/mod.rs b/src/node/tests/mod.rs index 09785f56..22b838e7 100644 --- a/src/node/tests/mod.rs +++ b/src/node/tests/mod.rs @@ -13,6 +13,7 @@ mod bloom_poison; mod bootstrap; mod connected_udp; mod control; +mod coord_forgery; mod decrypt_failure; mod disconnect; mod discovery; diff --git a/src/node/tests/session.rs b/src/node/tests/session.rs index aba0d69a..8bb70718 100644 --- a/src/node/tests/session.rs +++ b/src/node/tests/session.rs @@ -4306,7 +4306,9 @@ async fn test_path_broken_releases_path_mtu_lookup_entry() { assertion below observes nothing" ); - tn.node.handle_path_broken(&reporter, inner).await; + tn.node + .handle_path_broken(&reporter, &reporter, inner) + .await; assert_eq!( tn.node.path_mtu_lookup_get(&dest_fips), @@ -4361,7 +4363,9 @@ async fn test_path_broken_resets_the_session_source_path_mtu() { assertion below observes nothing" ); - tn.node.handle_path_broken(&reporter, inner).await; + tn.node + .handle_path_broken(&reporter, &reporter, inner) + .await; assert_eq!( tn.node @@ -4699,7 +4703,8 @@ async fn test_path_broken_naming_a_dest_with_no_session_does_not_flush_cached_co let _ = node.coord_cache_mut().insert(dest, coords, 1000); let encoded = PathBroken::new(dest, reporter).encode(); - node.handle_path_broken(&reporter, &encoded[5..]).await; + node.handle_path_broken(&reporter, &reporter, &encoded[5..]) + .await; assert!( node.coord_cache().get(&dest, 1000).is_some(), @@ -4746,7 +4751,8 @@ async fn test_path_broken_naming_a_dest_whose_entry_is_an_unauthenticated_respon install_halfopen(&mut node, dest); let encoded = PathBroken::new(dest, reporter).encode(); - node.handle_path_broken(&reporter, &encoded[5..]).await; + node.handle_path_broken(&reporter, &reporter, &encoded[5..]) + .await; assert!( node.coord_cache().get(&dest, 1000).is_some(), @@ -4781,7 +4787,8 @@ async fn test_path_broken_for_a_session_we_initiated_still_flushes_cached_coords let _ = node.coord_cache_mut().insert(dest, coords, 1000); let encoded = PathBroken::new(dest, reporter).encode(); - node.handle_path_broken(&reporter, &encoded[5..]).await; + node.handle_path_broken(&reporter, &reporter, &encoded[5..]) + .await; assert!( node.coord_cache().get(&dest, 1000).is_none(), @@ -6993,7 +7000,7 @@ async fn deliver_path_broken( let encoded = PathBroken::new(dest, reporter).encode(); nodes[at] .node - .handle_path_broken(&reporter, &encoded[5..]) + .handle_path_broken(&reporter, &reporter, &encoded[5..]) .await; } @@ -8994,7 +9001,7 @@ async fn a_path_broken_flood_releases_the_stored_path_mtu_only_once_per_interval let inner = &encoded[5..]; node.path_mtu_lookup_insert(dest_fips, 700); - node.handle_path_broken(&reporter, inner).await; + node.handle_path_broken(&reporter, &reporter, inner).await; assert_eq!( node.path_mtu_lookup_get(&dest_fips), None, @@ -9002,7 +9009,7 @@ async fn a_path_broken_flood_releases_the_stored_path_mtu_only_once_per_interval ); node.path_mtu_lookup_insert(dest_fips, 700); - node.handle_path_broken(&reporter, inner).await; + node.handle_path_broken(&reporter, &reporter, inner).await; assert_eq!( node.path_mtu_lookup_get(&dest_fips), Some(700), diff --git a/src/node/tests/tcp.rs b/src/node/tests/tcp.rs index 6c14bbf5..03fb9020 100644 --- a/src/node/tests/tcp.rs +++ b/src/node/tests/tcp.rs @@ -12,7 +12,8 @@ use crate::transport::{ ConnectionState, TransportAddr, TransportHandle, TransportId, packet_channel, }; use spanning_tree::{ - TestNode, cleanup_nodes, drain_all_packets, initiate_handshake, verify_tree_convergence, + TestNode, cleanup_nodes, drain_all_packets, initiate_handshake, process_available_packets, + verify_tree_convergence, }; use std::time::Duration; @@ -29,6 +30,13 @@ async fn make_test_node_tcp() -> TestNode { /// so immutable fields (e.g. heartbeat/link-dead timeouts) are set before the /// `NodeContext` is built rather than poked afterward. async fn make_test_node_tcp_with(config: Config) -> TestNode { + make_test_node_tcp_idle(config, None).await +} + +/// Like `make_test_node_tcp_with`, and also sets the transport's inbound +/// idle deadline when `idle` is given, as `create_transports` does for a +/// node built from config. +async fn make_test_node_tcp_idle(config: Config, idle: Option) -> TestNode { let mut node = make_node_with(config); let transport_id = TransportId::new(1); @@ -40,6 +48,9 @@ async fn make_test_node_tcp_with(config: Config) -> TestNode { let (packet_tx, packet_rx) = packet_channel(256); let mut transport = TcpTransport::new(transport_id, None, config, packet_tx); + if let Some(d) = idle { + transport.set_inbound_idle_timeout(d); + } transport.start_async().await.unwrap(); let local_addr = transport @@ -218,6 +229,82 @@ async fn test_tcp_connection_loss_detection() { cleanup_nodes(&mut nodes).await; } +/// A TCP link whose only traffic is heartbeats outlives several inbound idle +/// deadlines when the deadline is the one the node derives from its own +/// liveness timers. +/// +/// This is the healthy-path check for the idle deadline: it uses the real +/// derivation (`inbound_idle_timeout`), not a hand-picked value, and +/// short timers so three deadlines fit in a test. Break-check: give the +/// listening node's transport an idle deadline below the heartbeat interval +/// and its inbound connection is dropped, so the inbound count falls to 0. +#[tokio::test] +async fn tcp_link_kept_alive_only_by_heartbeats_survives_several_idle_deadlines() { + use crate::node::handlers::rekey::inbound_idle_timeout; + + let mut config = Config::new(); + config.node.heartbeat_interval_secs = 1; + config.node.link_dead_timeout_secs = 3; + config.node.rate_limit.handshake_resend_interval_ms = 100; + config.node.rate_limit.handshake_max_resends = 1; + let idle = inbound_idle_timeout(&config.node); + // 100 ms of ladder, three 1 s ticks, 3 s link-dead. + assert_eq!(idle, Duration::from_millis(6100)); + let mut nodes = vec![ + make_test_node_tcp_idle(config.clone(), Some(idle)).await, + make_test_node_tcp_idle(config, Some(idle)).await, + ]; + + // Node 0 dials node 1, so node 1 holds the inbound connection. + initiate_handshake(&mut nodes, 0, 1).await; + assert!(drain_all_packets(&mut nodes, false).await > 0); + + let addr_0 = *nodes[0].node.node_addr(); + let addr_1 = *nodes[1].node.node_addr(); + let inbound = |nodes: &[TestNode]| match nodes[1].node.transports.get(&nodes[1].transport_id) { + Some(TransportHandle::Tcp(t)) => t.stats().pool_inbound_count(), + _ => panic!("node 1 should have its TCP transport"), + }; + assert_eq!( + inbound(&nodes), + 1, + "node 1 should hold one inbound connection" + ); + + // Heartbeats only, for three idle deadlines and a little more. + let start = tokio::time::Instant::now(); + let mut processed = 0; + while start.elapsed() < idle * 3 + Duration::from_millis(500) { + nodes[0].node.check_link_heartbeats().await; + nodes[1].node.check_link_heartbeats().await; + processed += process_available_packets(&mut nodes).await; + assert_eq!( + inbound(&nodes), + 1, + "the inbound connection was dropped after {:?} of heartbeat-only traffic", + start.elapsed() + ); + tokio::time::sleep(Duration::from_millis(250)).await; + } + + // Sibling control: heartbeats were actually exchanged, so the kept + // connection is not an artefact of nothing having run. + assert!( + processed >= 18, + "expected at least one heartbeat per second each way, processed {processed}" + ); + assert!( + nodes[0].node.get_peer(&addr_1).is_some(), + "node 0 lost node 1" + ); + assert!( + nodes[1].node.get_peer(&addr_0).is_some(), + "node 1 lost node 0" + ); + + cleanup_nodes(&mut nodes).await; +} + /// TCP reconnection after link death: connect-on-send re-establishes the link. /// /// After both peers detect a dead link and remove each other, a fresh diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index 5f89bf4a..ba623b0e 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -3854,6 +3854,110 @@ async fn a_node_without_ble_config_draws_no_ble_warning() { ); } +/// The inbound idle deadline is the node's link-silence bound: 64 s at stock +/// settings, moving one for one with the link-dead timeout, and never below +/// the link-dead timeout plus a tick, the shortest silence after which the +/// node itself would reap the link. +#[test] +fn the_inbound_idle_deadline_is_the_link_silence_bound_and_tracks_the_link_dead_timeout() { + use crate::node::handlers::rekey::inbound_idle_timeout; + use crate::transport::tcp::INBOUND_IDLE_TIMEOUT; + + // 31 s of msg1 ladder (1+2+4+8+16), three 1 s ticks, 30 s link-dead. + let stock = crate::config::NodeConfig::default(); + assert_eq!(inbound_idle_timeout(&stock), Duration::from_secs(64)); + // The transport default a TCP or Tor instance holds before the node sets + // it must be the same stock value, so the two cannot drift apart. + assert_eq!(inbound_idle_timeout(&stock), INBOUND_IDLE_TIMEOUT); + + let raised = crate::config::NodeConfig { + link_dead_timeout_secs: 300, + ..Default::default() + }; + assert_eq!( + inbound_idle_timeout(&raised), + inbound_idle_timeout(&stock) + Duration::from_secs(270) + ); + + for node in [ + stock, + raised, + crate::config::NodeConfig { + heartbeat_interval_secs: 90, + ..Default::default() + }, + crate::config::NodeConfig { + tick_interval_secs: 5, + link_dead_timeout_secs: 10, + ..Default::default() + }, + ] { + let floor = Duration::from_secs( + node.link_dead_timeout_secs + .max(node.heartbeat_interval_secs) + + node.tick_interval_secs, + ); + assert!( + inbound_idle_timeout(&node) > floor, + "idle deadline {:?} must exceed {:?} for heartbeat {} s, link-dead {} s, tick {} s", + inbound_idle_timeout(&node), + floor, + node.heartbeat_interval_secs, + node.link_dead_timeout_secs, + node.tick_interval_secs, + ); + } +} + +/// `create_transports` hands every TCP and Tor instance the idle deadline +/// derived from the node's own liveness timers, not the transport default. +/// +/// Break-check: remove either `set_inbound_idle_timeout` call and that +/// transport keeps `INBOUND_IDLE_TIMEOUT`, which the non-default timers here +/// are chosen to differ from. +#[tokio::test] +async fn create_transports_sets_the_inbound_idle_deadline_from_the_node_liveness_timers() { + use crate::node::handlers::rekey::inbound_idle_timeout; + use crate::transport::tcp::INBOUND_IDLE_TIMEOUT; + + let mut config = crate::Config::new(); + config.node.control.enabled = false; + config.node.heartbeat_interval_secs = 5; + config.node.link_dead_timeout_secs = 120; + config.transports.tcp = + crate::config::TransportInstances::Single(crate::config::TcpConfig::default()); + config.transports.tor = + crate::config::TransportInstances::Single(crate::config::TorConfig::default()); + let expected = inbound_idle_timeout(&config.node); + // 31 s of ladder, three 1 s ticks, 120 s link-dead. + assert_eq!(expected, Duration::from_secs(154)); + assert_ne!(expected, INBOUND_IDLE_TIMEOUT); + + let mut node = make_node_with(config); + let (tx, _rx) = packet_channel(8); + let transports = node.create_transports(&tx).await; + + let mut seen = (0, 0); + for handle in &transports { + match handle { + crate::transport::TransportHandle::Tcp(t) => { + assert_eq!(t.inbound_idle_timeout(), expected, "TCP idle deadline"); + seen.0 += 1; + } + crate::transport::TransportHandle::Tor(t) => { + assert_eq!(t.inbound_idle_timeout(), expected, "Tor idle deadline"); + seen.1 += 1; + } + _ => {} + } + } + assert_eq!( + seen, + (1, 1), + "one TCP and one Tor transport should be built" + ); +} + #[cfg(all(ble_available, any(target_os = "android", test)))] mod test_radio { use crate::transport::ble::addr::BleAddr; diff --git a/src/noise/handshake.rs b/src/noise/handshake.rs index 035d4b4a..58ccc9da 100644 --- a/src/noise/handshake.rs +++ b/src/noise/handshake.rs @@ -344,10 +344,10 @@ impl HandshakeState { SecretKey::from_slice(&secret_bytes).expect("32 random bytes is valid secret key"); self.ephemeral_keypair = Some(Keypair::from_secret_key(&self.secp, &secret_key)); secret_bytes.zeroize(); - // The erase is called non-secure because the private key may sit in - // further copies this function cannot name. It still clears the copy - // this frame owns; the keypair the key was just stored in is cleared - // by `Drop for HandshakeState`. + // This clears the copy this frame owns, with a volatile write the + // optimiser keeps; a copy made before it, such as one a call above + // left in a register or a stack slot, is not reached. The keypair the + // key was just stored in is cleared by `Drop for HandshakeState`. secret_key.non_secure_erase(); } @@ -364,9 +364,9 @@ impl HandshakeState { /// caller below binds the value `Keypair::secret_key` hands back and /// erases that binding once the DH is done, because the returned /// `SecretKey` is a whole private key rather than a handle to one. Those - /// erases clear the copies this crate owns; `secp256k1` names its erase - /// non-secure because the compiler may hold further copies that no code - /// here can name. + /// erases clear the copies this crate owns. Each is a volatile write the + /// optimiser keeps, but a copy the compiler made before it runs, in a + /// register or a stack slot, is not reached. fn ecdh(&self, our_secret: &SecretKey, their_public: &PublicKey) -> [u8; 32] { // Get raw (x, y) coordinates (64 bytes) without any hashing let mut point = shared_secret_point(their_public, our_secret); @@ -1098,9 +1098,15 @@ impl Drop for HandshakeState { /// where clearing them matters most. `Keypair` is `Copy` and so cannot /// clear itself on drop; `HandshakeState` is not, so it does it for both. /// - /// This clears the copies this crate owns, not every copy that ever - /// existed: `secp256k1` names its erase non-secure because the compiler - /// may duplicate or move the bytes to places no code here can name. + /// This clears the copy being dropped, not every copy that ever existed. + /// Each erase is a volatile write, so the optimiser keeps it, but a + /// `HandshakeState` is built on the stack and moved several times before + /// it is dropped, and every move leaves the old bytes, both private keys + /// included, where the value used to be. A plain `take()` out of an + /// `Option` leaves its full contents in the slot. When a connection's + /// handshake completes, the connection's heap slot is cleared with + /// `clear_slot` once the handshake has left it; a move of the whole + /// connection state out of its machine does not clear anything. fn drop(&mut self) { self.static_keypair.non_secure_erase(); if let Some(ephemeral) = self.ephemeral_keypair.as_mut() { diff --git a/src/noise/mod.rs b/src/noise/mod.rs index 85366e4f..653eb89b 100644 --- a/src/noise/mod.rs +++ b/src/noise/mod.rs @@ -489,5 +489,49 @@ pub(crate) fn open( Ok(buf) } +/// Move the value out of `slot`, leaving `None`, and clear the bytes the +/// value occupied. +/// +/// `Option::take` moves the value out but writes only the `None` marker, so +/// the old value's bytes stay where they were. For a slot holding a +/// handshake or a session that is a full copy of its keys, which nothing +/// will clear because nothing owns it any more. Here, once the value has +/// moved out, the slot is cleared with [`clear_slot`]. +/// +/// This clears the slot only. The moved value is the caller's to clear, and +/// any copy made on the way out, such as in a register or a stack slot, is +/// out of reach as it is for every other move. A caller that unwraps the +/// result straight away should use `take().expect(..)` followed by +/// [`clear_slot`] instead: unwrapping after the slot has been cleared makes +/// the optimiser keep a second copy of the value on the stack, because it +/// can no longer copy the value straight from the slot to where it ends up. +pub(crate) fn take_cleared(slot: &mut Option) -> Option { + let value = slot.take(); + clear_slot(slot); + value +} + +/// Drop whatever `slot` holds, overwrite every byte of it with zeros, and +/// leave it `None`. +/// +/// The zeros are volatile writes, which the optimiser keeps. Meant for a +/// slot whose value has just been moved out, so the bytes the move left +/// behind are cleared. +pub(crate) fn clear_slot(slot: &mut Option) { + *slot = None; + let raw: *mut Option = slot; + // SAFETY: `raw` comes from a live `&mut`, so it is valid and aligned for + // `size_of::>()` bytes. Seen as `MaybeUninit`, those bytes have + // no drop glue, own nothing and may be all zero, which is what + // `zeroize_flat_type` asks of its target. The slot holds `None` after the + // assignment above, so zeroing it discards nothing, and writing `None` + // (which does not drop the zeroed bytes) leaves the slot valid before + // anything reads it again. + unsafe { + zeroize::zeroize_flat_type(raw.cast::>>()); + raw.write(None); + } +} + #[cfg(test)] mod tests; diff --git a/src/noise/session.rs b/src/noise/session.rs index 99d17ac5..c2436451 100644 --- a/src/noise/session.rs +++ b/src/noise/session.rs @@ -7,6 +7,14 @@ use std::fmt; /// Provides bidirectional authenticated encryption with replay protection. /// The send counter is monotonically incremented; received counters are /// validated against a sliding window to prevent replay attacks. +/// +/// The two traffic keys are cleared when their `CipherState`s are dropped, +/// which clears only the copy being dropped. A session is moved out of +/// `HandshakeState::into_session` and into a connection's session slot, and +/// every move leaves both keys where the value used to be. Taking it out of +/// that slot through `take_cleared`, as the machine's `take_session` does, +/// clears the slot; a plain `take()`, or a move of the whole connection +/// state out of its machine, leaves both keys there. pub struct NoiseSession { /// Our role in the original handshake. role: HandshakeRole, diff --git a/src/noise/tests.rs b/src/noise/tests.rs index 4c5e86b3..952772b2 100644 --- a/src/noise/tests.rs +++ b/src/noise/tests.rs @@ -915,3 +915,134 @@ fn test_xk_invalid_msg3_size() { .is_err() ); } + +/// The `zeroize` features of `sha2` and `hmac` are on, so the SHA-256 state +/// that hashes each Diffie-Hellman result, and the two SHA-256 cores inside +/// the HMAC that HKDF runs on, are cleared when dropped. Without `sha2`'s +/// feature this does not compile; `hmac`'s forwards to the same `digest` +/// feature, so dropping it alone changes nothing while `sha2`'s is on. +#[test] +fn test_sha256_states_used_by_hashing_and_hkdf_are_cleared_on_drop() { + fn clears_on_drop() {} + clears_on_drop::(); + clears_on_drop::<::Core>(); +} + +/// A value that counts its drops, so a test can tell a value dropped once +/// from one dropped twice or leaked. +struct DropCounter<'a> { + bytes: [u8; 48], + heap: Vec, + drops: &'a std::cell::Cell, +} + +impl Drop for DropCounter<'_> { + fn drop(&mut self) { + self.drops.set(self.drops.get() + 1); + } +} + +#[test] +fn test_take_cleared_moves_the_value_out_intact_and_leaves_the_slot_none() { + let drops = std::cell::Cell::new(0); + let mut slot = Some(DropCounter { + bytes: [0xa5; 48], + heap: vec![7u8; 100], + drops: &drops, + }); + + let taken = take_cleared(&mut slot).expect("the slot held a value"); + + assert!(slot.is_none()); + assert_eq!(taken.bytes, [0xa5; 48]); + assert_eq!(taken.heap, vec![7u8; 100]); + assert_eq!(drops.get(), 0, "clearing the slot must not drop the value"); + drop(taken); + assert_eq!(drops.get(), 1); +} + +#[test] +fn test_take_cleared_slot_is_reusable_and_an_empty_slot_stays_empty() { + let drops = std::cell::Cell::new(0); + let mut slot: Option> = None; + assert!(take_cleared(&mut slot).is_none()); + assert!(slot.is_none()); + + slot = Some(DropCounter { + bytes: [1; 48], + heap: vec![2; 3], + drops: &drops, + }); + let first = take_cleared(&mut slot).expect("first value"); + slot = Some(DropCounter { + bytes: [3; 48], + heap: vec![4; 5], + drops: &drops, + }); + let second = take_cleared(&mut slot).expect("second value"); + assert_eq!((first.bytes[0], second.bytes[0]), (1, 3)); + assert!(slot.is_none()); + drop((first, second)); + assert_eq!(drops.get(), 2); +} + +/// For `Option`, all-zero bytes read as `Some(false)`, not `None`. +/// The slot must still come out as `None`, so zeroing alone is not enough. +#[test] +fn test_take_cleared_leaves_none_where_zero_bytes_would_read_as_some() { + let mut slot = Some(true); + assert_eq!(take_cleared(&mut slot), Some(true)); + assert_eq!(slot, None); + + let mut slot = Some((true, [9u8; 32])); + assert_eq!(take_cleared(&mut slot), Some((true, [9u8; 32]))); + assert_eq!(slot, None); +} + +#[test] +fn test_take_cleared_session_still_decrypts_what_its_peer_encrypts() { + let initiator_keypair = generate_keypair(); + let responder_keypair = generate_keypair(); + let mut initiator = + HandshakeState::new_initiator(initiator_keypair, responder_keypair.public_key()); + let mut responder = HandshakeState::new_responder(responder_keypair); + initiator.set_local_epoch(generate_epoch()); + responder.set_local_epoch(generate_epoch()); + let msg1 = initiator.write_message_1().unwrap(); + responder.read_message_1(&msg1).unwrap(); + let msg2 = responder.write_message_2().unwrap(); + + let mut handshake_slot = Some(initiator); + let mut initiator = take_cleared(&mut handshake_slot).expect("handshake in slot"); + assert!(handshake_slot.is_none()); + initiator.read_message_2(&msg2).unwrap(); + + let mut session_slot = Some(initiator.into_session().unwrap()); + let mut sender = take_cleared(&mut session_slot).expect("session in slot"); + assert!(session_slot.is_none()); + let mut receiver = responder.into_session().unwrap(); + let ciphertext = sender.encrypt(b"after the move").unwrap(); + assert_eq!(receiver.decrypt(&ciphertext).unwrap(), b"after the move"); +} + +#[test] +fn test_clear_slot_drops_a_present_value_once_and_leaves_none() { + let drops = std::cell::Cell::new(0); + let mut slot = Some(DropCounter { + bytes: [5; 48], + heap: vec![6; 7], + drops: &drops, + }); + clear_slot(&mut slot); + assert!(slot.is_none()); + assert_eq!(drops.get(), 1); + + clear_slot(&mut slot); + assert!(slot.is_none()); + assert_eq!(drops.get(), 1, "an empty slot has nothing to drop"); + + let mut flag = Some(true); + flag.take(); + clear_slot(&mut flag); + assert_eq!(flag, None); +} diff --git a/src/peer/machine.rs b/src/peer/machine.rs index 684ab952..74626e4d 100644 --- a/src/peer/machine.rs +++ b/src/peer/machine.rs @@ -622,8 +622,9 @@ impl PeerMachine { ) -> Result, NoiseError> { // The parameter is this frame's own copy of the node's long-term // private key, and the state checks below return before it is used. - // The guard clears it on every exit path. - let our_keypair = ErasingKeypair::take(&mut our_keypair); + // The guard clears it in place on every exit path, and makes no copy + // of its own for an early return to leave behind. + let our_keypair = ErasingKeypair::new(&mut our_keypair); let msg1 = { let direction = self.conn.direction(); @@ -668,8 +669,9 @@ impl PeerMachine { current_time_ms: u64, ) -> Result, NoiseError> { // Same as `start_handshake`: the parameter copy outlives two early - // returns, so the guard owns it rather than an erase per exit path. - let our_keypair = ErasingKeypair::take(&mut our_keypair); + // returns, so the guard clears it in place rather than an erase per + // exit path. + let our_keypair = ErasingKeypair::new(&mut our_keypair); let (msg2, learned_identity, remote_epoch) = { let direction = self.conn.direction(); @@ -738,10 +740,15 @@ impl PeerMachine { }); } + // The slot is heap memory that outlives this call; clearing it once + // the handshake has left keeps both private keys from staying + // there. Unwrapping before clearing lets the handshake move + // straight from the slot to `hs`, with no second stack copy. let mut hs = leg .noise_handshake .take() .expect("noise handshake must exist in SentMsg1 state"); + noise::clear_slot(&mut leg.noise_handshake); hs.read_message_2(message)?; @@ -766,7 +773,11 @@ impl PeerMachine { pub(crate) fn take_session(&mut self) -> Option { // The session exists iff the handshake reached `Complete`, so taking it // unconditionally is byte-equivalent to the old `== Complete` gate. - self.leg.as_mut().and_then(|leg| leg.noise_session.take()) + // The slot is cleared as the session leaves, so its two traffic keys + // do not stay behind in the machine. + self.leg + .as_mut() + .and_then(|leg| noise::take_cleared(&mut leg.noise_session)) } /// Check if we have a completed session ready to take. diff --git a/src/perf_profile.rs b/src/perf_profile.rs index 66442954..550c2b08 100644 --- a/src/perf_profile.rs +++ b/src/perf_profile.rs @@ -1,9 +1,3 @@ -// Some entry points (e.g. `stamp`, `record_since`) are only called from -// paths that aren't yet wired up in this PR (FSP-pipelined dispatch, -// per-stage worker telemetry). Keep them in tree so the follow-up wiring -// PR is a pure call-site change. -#![allow(dead_code)] - //! Runtime perf profiler for the FMP/FSP hot path and queue handoffs. //! //! Avoids external dependencies (`perf`, samply, etc.) by instrumenting @@ -191,19 +185,9 @@ pub(crate) fn enabled() -> bool { }) } -/// Capture a timestamp for a future queue-wait measurement. Returns -/// `None` when tracing is disabled so callers can store it cheaply in -/// packet/job structs without paying `Instant::now()` in production. -#[inline] -pub(crate) fn stamp() -> Option { - if enabled() { - Some(Instant::now()) - } else { - None - } -} - /// Record time elapsed since a previously captured stamp. +// Unix-only because its one caller, the encrypt worker, is. +#[cfg(unix)] #[inline] pub(crate) fn record_since(stage: Stage, start: Option) { if let Some(start) = start { diff --git a/src/proto/fsp/core.rs b/src/proto/fsp/core.rs index 33fc8b62..f45526a0 100644 --- a/src/proto/fsp/core.rs +++ b/src/proto/fsp/core.rs @@ -15,6 +15,7 @@ //! the plain-data [`DecryptSlot`] mirror before it reaches [`Fsp::classify_epoch`]. use super::limits::FSP_CUTOVER_DELAY_MS; +use super::quorum::QuorumVerdict; use crate::proto::stp::TreeCoordinate; use crate::{FipsAddress, NodeAddr}; @@ -95,12 +96,17 @@ pub(crate) enum FspAction { /// Invalidate the shared cached coordinates for `addr` /// (`coord_cache.remove`). InvalidateCoords { addr: NodeAddr }, + /// Mark the shared cached coordinates for `addr` as an unverified hint, + /// keeping the value (`coord_cache.demote`). The entry goes on routing, + /// but no longer refuses a hint that replaces it. + DemoteCoords { addr: NodeAddr }, /// Write `mtu` into the shared `FipsAddress`-keyed path-MTU lookup, keeping /// the tighter of existing-or-new (the shell applies the write under the /// `path_mtu_lookup` guard). TightenPathMtuLookup { fips_addr: FipsAddress, mtu: u16 }, - /// Trigger discovery toward `dest` (`maybe_initiate_lookup`); emitted only - /// when the target's identity is cached. + /// Trigger discovery toward `dest` (`maybe_initiate_lookup`). The + /// `CoordsRequired` decision emits it only when the target's identity is + /// cached; the `PathBroken` decision emits it always. InitiateLookup { dest: NodeAddr }, } @@ -445,20 +451,35 @@ impl Fsp { } } - /// Decide the reaction to a `PathBroken` signal: unconditionally invalidate - /// the stale cached coordinates for `dest`, then (only when the identity is - /// cached) trigger re-discovery. Order is invalidate-then-lookup, matching - /// the pre-refactor handler. The warmup send and counter reset stay shell. + /// Decide the reaction to an admitted `PathBroken` signal for `dest`. + /// + /// The signal is unauthenticated, so it may not discard coordinates a + /// lookup verified on its own say-so. `verified` is whether the cached + /// entry for `dest` is verified and still within its verification window; + /// `quorum` is what this report did to the destination's link quorum + /// (ignored when the entry is not verified). + /// + /// - Not verified: remove the entry, as a hint can be replaced by any warm + /// anyway, then look it up. + /// - Verified, quorum reached: demote the entry to a hint, keeping the + /// value, then look it up. + /// - Verified, below quorum: leave the entry alone and look it up. A + /// successful lookup replaces the value whatever it was. + /// + /// The lookup is emitted in every case. The warmup send, path-MTU release + /// and warmup-counter reset stay shell-side. pub(crate) fn plan_path_broken( &self, dest: NodeAddr, - has_cached_identity: bool, + verified: bool, + quorum: QuorumVerdict, ) -> Vec { - let mut actions = vec![FspAction::InvalidateCoords { addr: dest }]; - if has_cached_identity { - actions.push(FspAction::InitiateLookup { dest }); + let lookup = FspAction::InitiateLookup { dest }; + match (verified, quorum) { + (false, _) => vec![FspAction::InvalidateCoords { addr: dest }, lookup], + (true, QuorumVerdict::Reached) => vec![FspAction::DemoteCoords { addr: dest }, lookup], + (true, QuorumVerdict::Below { .. }) => vec![lookup], } - actions } /// Decide whether a path-MTU update should tighten the shared lookup: emit diff --git a/src/proto/fsp/mod.rs b/src/proto/fsp/mod.rs index 6ebb179c..7c9b37f5 100644 --- a/src/proto/fsp/mod.rs +++ b/src/proto/fsp/mod.rs @@ -17,11 +17,14 @@ //! `classify_epoch`, the initiation tie-break, and the pure MTU-clamp / //! bounded-queue / ECN transforms. No clock/crypto/I/O/tracing. //! - `limits.rs` — the session-rekey timing constants. +//! - `quorum.rs` — the distinct-link quorum that gates demoting verified +//! coordinates on `PathBroken`. Clock injected. //! - `wire.rs` — the FSP session wire codec and message types. Clock-free, //! crypto-free. pub(crate) mod core; pub(crate) mod limits; +pub(crate) mod quorum; pub(crate) mod wire; #[cfg(test)] diff --git a/src/proto/fsp/quorum.rs b/src/proto/fsp/quorum.rs new file mode 100644 index 00000000..b7a08e8c --- /dev/null +++ b/src/proto/fsp/quorum.rs @@ -0,0 +1,126 @@ +//! Distinct-link quorum for `PathBroken` signals. +//! +//! A `PathBroken` carries no end-to-end authentication, so one report is not +//! enough evidence to discard coordinates a lookup verified. This module +//! counts, per destination, the distinct links that delivered a report naming +//! it within a window, and says when enough of them have. +//! +//! The vote is keyed on the authenticated link peer the datagram arrived over, +//! not on the reporter the body names: the reporter is plaintext the sender +//! chooses, so a sender on one link could invent as many reporters as the +//! quorum needs. Every datagram from one neighbour, whether its own or relayed +//! through it, arrives over the same link and is one vote. +//! +//! Sans-IO: the caller passes the time in, and nothing here logs or counts. + +use std::collections::BTreeMap; + +use crate::NodeAddr; + +/// Distinct links that must deliver a report naming a destination within +/// [`QUORUM_WINDOW_MS`] before its verified coordinates are demoted. +/// +/// Two is the smallest value that one neighbour cannot reach alone, whether it +/// forges the reports or relays a reflection. No measurement has counted how +/// many links a genuine failure's reports arrive over; a larger value only +/// keeps a genuinely stale verified entry verified for longer, while the +/// lookup every report starts replaces it anyway. +/// +/// A node whose reports all arrive over one link, such as a leaf with a +/// single peer, never reaches this. A stale verified entry there lasts until +/// a lookup that a report started answers and replaces it, or at most until +/// its verification ages out after [`crate::cache::VERIFIED_TTL_MS`]; from +/// then on it no longer refuses a hint, and the next report removes it. +pub(crate) const QUORUM_LINKS: usize = 2; + +/// Window within which distinct links count toward one quorum, in +/// milliseconds. +/// +/// The sum of the default lookup attempt schedule (1 + 2 + 4 + 8 s), so the +/// reports that arrive during one re-validation cycle count together. An +/// anchor, not a measurement. +pub(crate) const QUORUM_WINDOW_MS: u64 = 15_000; + +/// What one report did to its destination's quorum. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum QuorumVerdict { + /// Not enough distinct links yet; `distinct` counts this one. + Below { distinct: usize }, + /// This report completed the quorum. The destination's record is cleared, + /// so the next quorum starts from nothing. + Reached, +} + +/// Distinct links seen per destination, each with the time it was first seen +/// inside the current window. +#[derive(Debug, Default)] +pub(crate) struct LinkQuorum { + seen: BTreeMap>, + last_sweep_ms: u64, +} + +impl LinkQuorum { + /// An empty quorum tracker. + pub(crate) fn new() -> Self { + Self::default() + } + + /// Record that a report naming `dest` arrived over the link to + /// `link_peer` at `now_ms`. + /// + /// A link already counted for `dest` inside the window does not count + /// again, and its first-seen time is not refreshed, so one neighbour + /// repeating itself can neither reach the quorum nor keep a record alive. + pub(crate) fn record( + &mut self, + dest: NodeAddr, + link_peer: NodeAddr, + now_ms: u64, + ) -> QuorumVerdict { + self.sweep(now_ms); + let seen = self.seen.entry(dest).or_default(); + seen.retain(|&(_, at)| live(at, now_ms)); + if !seen.iter().any(|&(l, _)| l == link_peer) { + seen.push((link_peer, now_ms)); + } + let distinct = seen.len(); + if distinct >= QUORUM_LINKS { + self.seen.remove(&dest); + QuorumVerdict::Reached + } else { + QuorumVerdict::Below { distinct } + } + } + + /// Forget every report about `dest`. + /// + /// Called when a lookup verifies `dest` again: reports about the path the + /// fresh value replaced are not evidence against it. + pub(crate) fn clear(&mut self, dest: &NodeAddr) { + self.seen.remove(dest); + } + + /// Number of destinations with a record. + #[cfg(test)] + pub(crate) fn len(&self) -> usize { + self.seen.len() + } + + /// Drop destinations with no live link, at most once per window, so + /// the map holds only destinations reported within the last window or so. + fn sweep(&mut self, now_ms: u64) { + if now_ms.saturating_sub(self.last_sweep_ms) < QUORUM_WINDOW_MS { + return; + } + self.last_sweep_ms = now_ms; + self.seen.retain(|_, seen| { + seen.retain(|&(_, at)| live(at, now_ms)); + !seen.is_empty() + }); + } +} + +/// Whether a report first seen at `at` still counts at `now_ms`. +fn live(at: u64, now_ms: u64) -> bool { + now_ms.saturating_sub(at) <= QUORUM_WINDOW_MS +} diff --git a/src/proto/fsp/tests/core.rs b/src/proto/fsp/tests/core.rs index 0e80f3af..a4b0150d 100644 --- a/src/proto/fsp/tests/core.rs +++ b/src/proto/fsp/tests/core.rs @@ -737,22 +737,43 @@ fn plan_coords_required_lookup_gated_on_identity() { } #[test] -fn plan_path_broken_invalidates_then_lookups() { +fn plan_path_broken_removes_an_unverified_entry_then_looks_it_up() { + use crate::proto::fsp::quorum::QuorumVerdict; + let fsp = Fsp::new(); + let dest = make_node_addr(6); + let expected = vec![ + FspAction::InvalidateCoords { addr: dest }, + FspAction::InitiateLookup { dest }, + ]; + // The quorum is irrelevant to a hint: removed either way. + for quorum in [QuorumVerdict::Below { distinct: 0 }, QuorumVerdict::Reached] { + assert_eq!(fsp.plan_path_broken(dest, false, quorum), expected); + } +} + +#[test] +fn plan_path_broken_keeps_a_verified_entry_below_quorum_and_looks_it_up() { + use crate::proto::fsp::quorum::QuorumVerdict; let fsp = Fsp::new(); let dest = make_node_addr(6); - // Cached identity: invalidate, then lookup — in that order. assert_eq!( - fsp.plan_path_broken(dest, true), + fsp.plan_path_broken(dest, true, QuorumVerdict::Below { distinct: 1 }), + vec![FspAction::InitiateLookup { dest }] + ); +} + +#[test] +fn plan_path_broken_demotes_a_verified_entry_at_quorum_then_looks_it_up() { + use crate::proto::fsp::quorum::QuorumVerdict; + let fsp = Fsp::new(); + let dest = make_node_addr(6); + assert_eq!( + fsp.plan_path_broken(dest, true, QuorumVerdict::Reached), vec![ - FspAction::InvalidateCoords { addr: dest }, + FspAction::DemoteCoords { addr: dest }, FspAction::InitiateLookup { dest }, ] ); - // No cached identity: invalidate only (still unconditional). - assert_eq!( - fsp.plan_path_broken(dest, false), - vec![FspAction::InvalidateCoords { addr: dest }] - ); } #[test] diff --git a/src/proto/fsp/tests/mod.rs b/src/proto/fsp/tests/mod.rs index e0f8f26e..51820b2a 100644 --- a/src/proto/fsp/tests/mod.rs +++ b/src/proto/fsp/tests/mod.rs @@ -1,2 +1,3 @@ mod core; +mod quorum; mod wire; diff --git a/src/proto/fsp/tests/quorum.rs b/src/proto/fsp/tests/quorum.rs new file mode 100644 index 00000000..2e153932 --- /dev/null +++ b/src/proto/fsp/tests/quorum.rs @@ -0,0 +1,118 @@ +//! Unit tests for the `PathBroken` link quorum. + +use crate::NodeAddr; +use crate::proto::fsp::quorum::{LinkQuorum, QUORUM_LINKS, QUORUM_WINDOW_MS, QuorumVerdict}; + +fn addr(v: u8) -> NodeAddr { + let mut bytes = [0u8; 16]; + bytes[0] = v; + NodeAddr::from_bytes(bytes) +} + +const T0: u64 = 1_000_000; + +#[test] +fn the_quorum_needs_two_links() { + // The tests below are written for this value; a change to it should be a + // deliberate edit here too. + assert_eq!(QUORUM_LINKS, 2); +} + +#[test] +fn one_report_stays_below_the_quorum() { + let mut q = LinkQuorum::new(); + assert_eq!( + q.record(addr(1), addr(0xA), T0), + QuorumVerdict::Below { distinct: 1 } + ); +} + +#[test] +fn two_distinct_links_inside_the_window_reach_the_quorum() { + let mut q = LinkQuorum::new(); + let _ = q.record(addr(1), addr(0xA), T0); + assert_eq!( + q.record(addr(1), addr(0xB), T0 + QUORUM_WINDOW_MS), + QuorumVerdict::Reached + ); +} + +#[test] +fn the_same_link_repeated_never_reaches_the_quorum() { + let mut q = LinkQuorum::new(); + for i in 0..10 { + assert_eq!( + q.record(addr(1), addr(0xA), T0 + i), + QuorumVerdict::Below { distinct: 1 }, + "repeat {i} counted again" + ); + } +} + +#[test] +fn links_further_apart_than_the_window_do_not_combine() { + let mut q = LinkQuorum::new(); + let _ = q.record(addr(1), addr(0xA), T0); + assert_eq!( + q.record(addr(1), addr(0xB), T0 + QUORUM_WINDOW_MS + 1), + QuorumVerdict::Below { distinct: 1 } + ); +} + +#[test] +fn a_repeat_does_not_refresh_its_links_first_seen_time() { + let mut q = LinkQuorum::new(); + let _ = q.record(addr(1), addr(0xA), T0); + let _ = q.record(addr(1), addr(0xA), T0 + QUORUM_WINDOW_MS); + // Had the repeat refreshed A's time, this would combine with it. + assert_eq!( + q.record(addr(1), addr(0xB), T0 + QUORUM_WINDOW_MS + 1), + QuorumVerdict::Below { distinct: 1 } + ); +} + +#[test] +fn reaching_the_quorum_clears_the_destination() { + let mut q = LinkQuorum::new(); + let _ = q.record(addr(1), addr(0xA), T0); + assert_eq!(q.record(addr(1), addr(0xB), T0 + 1), QuorumVerdict::Reached); + assert_eq!(q.len(), 0); + assert_eq!( + q.record(addr(1), addr(0xA), T0 + 2), + QuorumVerdict::Below { distinct: 1 }, + "the next demotion must need a fresh quorum" + ); +} + +#[test] +fn clear_forgets_the_reports_about_a_destination() { + let mut q = LinkQuorum::new(); + let _ = q.record(addr(1), addr(0xA), T0); + q.clear(&addr(1)); + assert_eq!( + q.record(addr(1), addr(0xB), T0 + 1), + QuorumVerdict::Below { distinct: 1 } + ); +} + +#[test] +fn destinations_are_counted_separately() { + let mut q = LinkQuorum::new(); + let _ = q.record(addr(1), addr(0xA), T0); + assert_eq!( + q.record(addr(2), addr(0xB), T0), + QuorumVerdict::Below { distinct: 1 } + ); +} + +#[test] +fn the_sweep_drops_destinations_with_no_live_link() { + let mut q = LinkQuorum::new(); + for d in 0..100 { + let _ = q.record(addr(d), addr(0xA), T0); + } + assert_eq!(q.len(), 100); + // One report a window and a millisecond later sweeps the stale records. + let _ = q.record(addr(200), addr(0xA), T0 + QUORUM_WINDOW_MS + 1); + assert_eq!(q.len(), 1); +} diff --git a/src/proto/lookup/core.rs b/src/proto/lookup/core.rs index 2aa57ac2..5c5b8b71 100644 --- a/src/proto/lookup/core.rs +++ b/src/proto/lookup/core.rs @@ -146,13 +146,19 @@ pub(crate) fn plan_initiate(request: &LookupRequest, rv: &impl RoutingView) -> V /// Classification of an inbound LookupRequest, decided from Lookup state. pub(crate) enum RequestOutcome { - /// request_id already in the dedup cache — drop. + /// request_id already in the dedup cache — drop. The test is on the id + /// alone, so one flood reaching this node through two neighbours lands + /// here just as a request sent twice does; it does not identify the peer + /// that delivered the copy. Duplicate, /// A request this node originated, looped back to it — drop without - /// recording. Kept separate from `Duplicate`, which means another node - /// resent a request, so the two do not share a rejection counter: this - /// one has a nonzero floor in healthy operation and says nothing about - /// the peer that delivered it. + /// recording. Kept apart from `Duplicate` by cause: this is the node's + /// own fan-out returning, while `Duplicate` is a request id the node has + /// already recorded, seen again. Neither identifies the delivering peer. + /// Recognised only while the lookup is outstanding and the id is among + /// the last `MAX_RECORDED_IDS` it issued; an own copy outside that reach + /// is recorded and forwarded as transit, and a later copy of it is + /// dropped as a duplicate. OwnRequestLooped, /// We are the lookup target — the shell generates + sends the response. RespondAsTarget, diff --git a/src/proto/lookup/tests/core.rs b/src/proto/lookup/tests/core.rs index 66f4e21c..917616b4 100644 --- a/src/proto/lookup/tests/core.rs +++ b/src/proto/lookup/tests/core.rs @@ -520,6 +520,95 @@ fn classify_request_drops_our_own_request_looped_back_to_us() { )); } +#[test] +fn classify_request_records_our_own_id_as_transit_once_its_lookup_has_timed_out() { + // The own-request guard reaches only as far as the pending lookup. Once + // the ladder times out and the entry is dropped, a late copy of our own + // request is indistinguishable from transit: it is recorded and forwarded, + // and a later copy of it is a duplicate. + let mut lookup = empty_lookup(); + let looping_peer = make_node_addr(0x01); + let my_addr = make_node_addr(0x99); + let target = make_node_addr(0xAA); + let t0 = 1000u64; + let mut pending = PendingLookup::new(t0); + pending.record(77); + lookup.pending_lookups.insert(target, pending); + + // A one-rung ladder of 1s: the entry times out at t0 + 1000. + let now = t0 + 1000; + let outcome = poll_pending(&mut lookup, now, &[1]); + assert_eq!(outcome.timeouts.len(), 1, "the lookup must time out"); + + let request = make_request_id(77, target, 3); + let first = classify_request( + &mut lookup, + &request, + &looping_peer, + &my_addr, + now, + 5000, + 4096, + 1, + ); + assert!(matches!(first.outcome, RequestOutcome::Forward)); + assert!(lookup.recent_requests.contains_key(&77)); + + let second = classify_request( + &mut lookup, + &request, + &looping_peer, + &my_addr, + now, + 5000, + 4096, + 1, + ); + assert!(matches!(second.outcome, RequestOutcome::Duplicate)); +} + +#[test] +fn classify_request_records_an_own_id_older_than_the_recorded_window_as_transit() { + // A pending lookup keeps only the last MAX_RECORDED_IDS (eight) ids its + // ladder issued. A returning copy of an earlier attempt is outside the + // guard's reach and goes through as transit; a recent one is still caught. + let mut lookup = empty_lookup(); + let looping_peer = make_node_addr(0x01); + let my_addr = make_node_addr(0x99); + let target = make_node_addr(0xAA); + let mut pending = PendingLookup::new(1000); + for id in 1..=9 { + pending.record(id); + } + lookup.pending_lookups.insert(target, pending); + + let oldest = classify_request( + &mut lookup, + &make_request_id(1, target, 3), + &looping_peer, + &my_addr, + 1000, + 5000, + 4096, + 1, + ); + assert!(matches!(oldest.outcome, RequestOutcome::Forward)); + assert!(lookup.recent_requests.contains_key(&1)); + + let newest = classify_request( + &mut lookup, + &make_request_id(9, target, 3), + &looping_peer, + &my_addr, + 1000, + 5000, + 4096, + 1, + ); + assert!(matches!(newest.outcome, RequestOutcome::OwnRequestLooped)); + assert!(!lookup.recent_requests.contains_key(&9)); +} + #[test] fn classify_request_still_transits_a_foreign_id_for_a_target_we_are_looking_up() { // The guard keys on the id, not the target: another node's lookup for the diff --git a/src/transport/nym/mod.rs b/src/transport/nym/mod.rs index f1a5dea0..7557dd3c 100644 --- a/src/transport/nym/mod.rs +++ b/src/transport/nym/mod.rs @@ -692,14 +692,14 @@ fn parse_target_addr(addr: &TransportAddr) -> Result( /// hoisted into each per-transport wrapper (tor carries a `direction` field /// nym lacks), so this loop is silent on exit. /// -/// `first_frame_timeout` bounds the wait for the *first* complete frame only. -/// It is `Some` for a connection that takes a capped inbound slot from accept +/// `deadline` bounds the wait for every complete frame: the first-frame +/// deadline until one arrives, the idle deadline for each one after. It is +/// `Some` for a connection that takes a capped inbound slot from accept /// — today only tor's onion listener — and `None` everywhere else, which /// covers every outbound connection and the whole of the nym transport (nym /// is outbound-only and keeps no counted slots). A deadline expiry is not a @@ -268,7 +269,7 @@ pub(crate) async fn proxied_receive_loop( mtu: u16, stats: Arc, label: &'static str, - first_frame_timeout: Option, + deadline: Option, ready_rx: Option>, on_remove: impl Fn(&S, &M), ) { @@ -290,11 +291,13 @@ pub(crate) async fn proxied_receive_loop( if admitted { let mut first = true; loop { - let read = match first_frame_timeout { - // Bound the first read only. A silent remote otherwise holds its - // inbound slot for as long as it keeps the socket open. - Some(d) if first => { - match tokio::time::timeout(d, read_fmp_packet(&mut reader, mtu)).await { + let read = match deadline { + // Bound every read. A remote that goes silent, before or after + // its first frame, otherwise holds its inbound slot for as long + // as it keeps the socket open. + Some(d) => { + let limit = d.for_read(first); + match tokio::time::timeout(limit, read_fmp_packet(&mut reader, mtu)).await { Ok(result) => result, Err(_) => { // Not a recv error: `record_recv_error` means framing @@ -303,15 +306,16 @@ pub(crate) async fn proxied_receive_loop( debug!( transport_id = %transport_id, remote_addr = %remote_addr, - timeout_secs = d.as_secs_f64(), - "No complete frame within the first-frame deadline, dropping inbound {} connection", + deadline = InboundDeadline::phase(first), + timeout_secs = limit.as_secs_f64(), + "No complete frame within the inbound deadline, dropping inbound {} connection", label ); break; } } } - _ => read_fmp_packet(&mut reader, mtu).await, + None => read_fmp_packet(&mut reader, mtu).await, }; first = false; @@ -372,6 +376,7 @@ mod tests { use crate::transport::packet_channel; use crate::transport::stream::{next_conn_id, park_writer}; use portable_atomic::{AtomicU64, Ordering}; + use std::time::Duration; use tokio::io::AsyncReadExt; use tokio::net::TcpListener; use tokio::time::timeout; diff --git a/src/transport/tcp/mod.rs b/src/transport/tcp/mod.rs index cadea129..9e82f84a 100644 --- a/src/transport/tcp/mod.rs +++ b/src/transport/tcp/mod.rs @@ -89,6 +89,9 @@ pub struct TcpTransport { /// Deadline from accept to the first complete inbound frame. Defaults to /// `INBOUND_FIRST_FRAME_TIMEOUT`; overridable only from tests. first_frame_timeout: Duration, + /// Longest wait for each later complete inbound frame. Defaults to + /// `INBOUND_IDLE_TIMEOUT`; the node sets it from its liveness timers. + idle_timeout: Duration, /// Transport statistics. stats: Arc, } @@ -113,6 +116,7 @@ impl TcpTransport { local_addr: None, node_max_connections: None, first_frame_timeout: INBOUND_FIRST_FRAME_TIMEOUT, + idle_timeout: INBOUND_IDLE_TIMEOUT, stats: Arc::new(TcpStats::new()), } } @@ -128,6 +132,22 @@ impl TcpTransport { self.first_frame_timeout = d; } + /// Set the inbound idle deadline: the longest an accepted connection may + /// go, after its first frame, without delivering another complete frame. + /// + /// The node derives it from its own link-liveness timers, so it never + /// drops a connection carrying a link the node would keep. Takes effect + /// at the next `start_async()`. + pub fn set_inbound_idle_timeout(&mut self, d: Duration) { + self.idle_timeout = d; + } + + /// The inbound idle deadline the accept loop will use. + #[cfg(test)] + pub(crate) fn inbound_idle_timeout(&self) -> Duration { + self.idle_timeout + } + /// Set the node-wide `node.limits.max_connections` value. /// /// Used as the inbound-cap fallback when this transport instance has no @@ -216,7 +236,10 @@ impl TcpTransport { keepalive_secs: self.config.keepalive_secs(), recv_buf: self.config.recv_buf_size(), send_buf: self.config.send_buf_size(), - first_frame_timeout: self.first_frame_timeout, + deadline: InboundDeadline { + first_frame: self.first_frame_timeout, + idle: self.idle_timeout, + }, }; let accept_task = tokio::spawn(async move { @@ -822,6 +845,45 @@ impl Transport for TcpTransport { /// TOML surface. pub(crate) const INBOUND_FIRST_FRAME_TIMEOUT: Duration = Duration::from_secs(30); +/// Default deadline for each complete inbound frame after the first. +/// +/// Without it, a remote that sends one well-formed frame and then goes +/// silent holds its inbound slot for as long as it keeps the socket open: +/// a frame that names no session is dropped by the node without closing +/// the transport. The node replaces this with the bound derived from its +/// own liveness timers (`link_silence_ms`); 64 s is that bound at stock +/// settings, kept here so a transport built outside the node still has a +/// deadline. +pub(crate) const INBOUND_IDLE_TIMEOUT: Duration = Duration::from_secs(64); + +/// Read deadlines for an inbound connection, which holds a capped pool +/// slot from accept. +/// +/// Each deadline covers one complete frame, not a byte: a remote that +/// drips a frame slower than the deadline is dropped. The idle deadline +/// re-arms on every frame, so a connection carrying a live link, which +/// receives at least a heartbeat per interval, is never dropped by it. +#[derive(Clone, Copy, Debug)] +pub(crate) struct InboundDeadline { + /// Deadline from accept to the first complete frame. + pub(crate) first_frame: Duration, + /// Deadline for each later complete frame, from the end of the last. + pub(crate) idle: Duration, +} + +impl InboundDeadline { + /// The deadline for the next read: `first` is true until a frame has + /// been received. + pub(crate) fn for_read(&self, first: bool) -> Duration { + if first { self.first_frame } else { self.idle } + } + + /// The log word for an expiry of the deadline `for_read(first)` gave. + pub(crate) fn phase(first: bool) -> &'static str { + if first { "first-frame" } else { "idle" } + } +} + /// Socket configuration parameters passed to the accept loop. struct AcceptConfig { mtu: u16, @@ -830,7 +892,7 @@ struct AcceptConfig { keepalive_secs: u64, recv_buf: usize, send_buf: usize, - first_frame_timeout: Duration, + deadline: InboundDeadline, } /// TCP accept loop — runs as a spawned task when bind_addr is configured. @@ -850,7 +912,7 @@ async fn accept_loop( keepalive_secs, recv_buf, send_buf, - first_frame_timeout, + deadline, } = cfg; debug!(transport_id = %transport_id, "TCP accept loop starting"); @@ -965,7 +1027,7 @@ async fn accept_loop( conn_mtu, recv_stats, Direction::Inbound, - Some(first_frame_timeout), + Some(deadline), Some(ready_rx), ) .await; @@ -1114,9 +1176,10 @@ async fn tcp_send_loop( /// `pool_outbound` counter regardless of whether the matching pool /// entry survived to be removed. /// -/// `first_frame_timeout` bounds the wait for the *first* complete frame -/// only, and is `Some` for inbound connections (which hold a capped pool -/// slot from accept) and `None` for outbound ones. `ready_rx`, when +/// `deadline` bounds the wait for every complete frame: the first-frame +/// deadline until one arrives, the idle deadline for each one after. It is +/// `Some` for inbound connections (which hold a capped pool slot from +/// accept) and `None` for outbound ones. `ready_rx`, when /// present, is the accept loop's readiness barrier: the loop must not run /// its cleanup before the accept loop has inserted the pool entry. /// @@ -1138,7 +1201,7 @@ async fn tcp_receive_loop( mtu: u16, stats: Arc, direction: Direction, - first_frame_timeout: Option, + deadline: Option, ready_rx: Option>, ) { let remote_addr = &key.remote; @@ -1159,11 +1222,13 @@ async fn tcp_receive_loop( if admitted { let mut first = true; loop { - let read = match first_frame_timeout { - // Bound the first read only. A silent remote otherwise holds - // its inbound slot for as long as it keeps the socket open. - Some(d) if first => { - match tokio::time::timeout(d, read_fmp_packet(&mut reader, mtu)).await { + let read = match deadline { + // Bound every read. A remote that goes silent, before or after + // its first frame, otherwise holds its inbound slot for as + // long as it keeps the socket open. + Some(d) => { + let limit = d.for_read(first); + match tokio::time::timeout(limit, read_fmp_packet(&mut reader, mtu)).await { Ok(result) => result, Err(_) => { // Not a recv error: `record_recv_error` means @@ -1172,14 +1237,15 @@ async fn tcp_receive_loop( debug!( transport_id = %transport_id, remote_addr = %remote_addr, - timeout_secs = d.as_secs_f64(), - "No complete frame within the first-frame deadline, dropping inbound connection" + deadline = InboundDeadline::phase(first), + timeout_secs = limit.as_secs_f64(), + "No complete frame within the inbound deadline, dropping inbound connection" ); break; } } } - _ => read_fmp_packet(&mut reader, mtu).await, + None => read_fmp_packet(&mut reader, mtu).await, }; first = false; @@ -1390,7 +1456,7 @@ fn read_mss_mtu(stream: &std::net::TcpStream, default_mtu: u16) -> u16 { mod tests { use super::pool::PoolMap; use super::*; - use crate::transport::framing::build_msg1_frame; + use crate::transport::framing::{build_established_frame, build_msg1_frame}; use crate::transport::packet_channel; use crate::transport::stream::park_writer; use tokio::time::{Duration, timeout}; @@ -2322,18 +2388,170 @@ mod tests { transport.stop_async().await.unwrap(); } - /// Regression guard, not evidence that the fix works. + /// The smallest frame the reader accepts as established: a 16-byte + /// header, no payload, a 16-byte tag. The node drops one naming no + /// session without closing the transport, so it is what a squatter + /// sends to get past the first-frame deadline. + fn squatter_frame() -> Vec { + let frame = build_established_frame(0); + assert_eq!(frame.len(), crate::proto::fmp::wire::ENCRYPTED_MIN_SIZE); + frame + } + + /// A remote that sends one well-formed frame and then goes silent must + /// lose its inbound slot at the idle deadline, without disconnecting. /// - /// The deadline is scoped to the first iteration, so an established - /// connection that then goes quiet cannot be dropped by it: this test - /// passes by construction under the current design. It is kept so that a - /// future general (every-read) idle deadline cannot silently start - /// reaping quiet links without a test going red. + /// The first-frame deadline is set far above the idle one, so the + /// release can only be the idle deadline's doing. Break-check: scope the + /// deadline back to the first read only and the count stays at 1. #[tokio::test] - async fn established_connection_survives_long_idle() { + async fn inbound_connection_that_goes_silent_after_one_established_frame_releases_its_slot() { + let (tx, mut rx) = packet_channel(100); + let mut transport = TcpTransport::new(TransportId::new(1), None, make_config(), tx); + transport.set_first_frame_timeout(Duration::from_secs(5)); + transport.set_inbound_idle_timeout(Duration::from_millis(300)); + transport.start_async().await.unwrap(); + let listen = transport.local_addr().unwrap(); + + let mut squatter = TcpStream::connect(listen).await.unwrap(); + squatter.write_all(&squatter_frame()).await.unwrap(); + let packet = timeout(Duration::from_secs(2), rx.recv()) + .await + .expect("timeout waiting for the squatter's frame") + .expect("packet channel closed"); + assert_eq!(packet.data, squatter_frame()); + assert_eq!( + transport.stats().pool_inbound_count(), + 1, + "the connection should hold its slot once its first frame is in" + ); + + assert!( + wait_until( + || transport.stats().pool_inbound_count() == 0, + Duration::from_secs(2) + ) + .await, + "a connection silent after its first frame should lose its slot at the idle deadline" + ); + assert!( + transport.pool.lock().await.is_empty(), + "the pool entry should go with the slot" + ); + + drop(squatter); + transport.stop_async().await.unwrap(); + } + + /// With the cap filled by a one-frame squatter, a genuine peer is refused + /// until the idle deadline frees the slot, and admitted afterwards. + /// + /// Break-check: without the idle deadline the squatter never releases, + /// so the genuine peer's frame is never delivered. + #[tokio::test] + async fn inbound_cap_filled_by_one_frame_squatters_admits_a_genuine_peer_after_the_idle_deadline() + { + let (tx, mut rx) = packet_channel(100); + let mut transport = TcpTransport::new(TransportId::new(1), None, capped_config(1), tx); + transport.set_first_frame_timeout(Duration::from_secs(5)); + transport.set_inbound_idle_timeout(Duration::from_secs(1)); + transport.start_async().await.unwrap(); + let listen = transport.local_addr().unwrap(); + + let mut squatter = TcpStream::connect(listen).await.unwrap(); + squatter.write_all(&squatter_frame()).await.unwrap(); + timeout(Duration::from_secs(2), rx.recv()) + .await + .expect("timeout waiting for the squatter's frame") + .expect("packet channel closed"); + assert_eq!(transport.stats().pool_inbound_count(), 1); + + // While the cap is full a genuine peer is rejected outright. The + // 250 ms window ends well inside the squatter's 1 s idle deadline. + let mut early = TcpStream::connect(listen).await.unwrap(); + let _ = early.write_all(&build_msg1_frame()).await; + assert!( + timeout(Duration::from_millis(250), rx.recv()) + .await + .is_err(), + "a peer arriving while the cap is full must not be admitted" + ); + drop(early); + + assert!( + wait_until( + || transport.stats().pool_inbound_count() == 0, + Duration::from_secs(3) + ) + .await, + "the idle deadline should free the slot the squatter took" + ); + + let mut genuine = TcpStream::connect(listen).await.unwrap(); + genuine.write_all(&build_msg1_frame()).await.unwrap(); + let packet = timeout(Duration::from_secs(2), rx.recv()) + .await + .expect("timeout waiting for the genuine peer's frame") + .expect("packet channel closed"); + assert_eq!(packet.data, build_msg1_frame()); + + drop(squatter); + drop(genuine); + transport.stop_async().await.unwrap(); + } + + /// The healthy path: a connection that delivers a frame more often than + /// the idle deadline keeps its slot across many deadlines, and every + /// frame is delivered. This is the shape of a link kept alive only by + /// heartbeats. + /// + /// Break-check: a deadline that does not re-arm on each frame (a single + /// deadline from accept) drops the connection after the first second. + #[tokio::test] + async fn inbound_connection_sending_a_frame_every_interval_below_the_idle_deadline_is_kept() { + let (tx, mut rx) = packet_channel(100); + let mut transport = TcpTransport::new(TransportId::new(1), None, make_config(), tx); + transport.set_first_frame_timeout(Duration::from_secs(1)); + transport.set_inbound_idle_timeout(Duration::from_secs(1)); + transport.start_async().await.unwrap(); + let listen = transport.local_addr().unwrap(); + + let mut peer = TcpStream::connect(listen).await.unwrap(); + // Twelve frames 250 ms apart: 3 s, three idle deadlines, with 750 ms + // of slack between each frame and the deadline it re-arms. + for i in 0..12 { + peer.write_all(&squatter_frame()).await.unwrap(); + let packet = timeout(Duration::from_secs(2), rx.recv()) + .await + .unwrap_or_else(|_| panic!("timeout waiting for frame {i}")) + .expect("packet channel closed"); + assert_eq!(packet.data, squatter_frame()); + assert_eq!( + transport.stats().pool_inbound_count(), + 1, + "a connection delivering frames inside the idle deadline must keep its slot (frame {i})" + ); + tokio::time::sleep(Duration::from_millis(250)).await; + } + assert_eq!(transport.stats().pool_inbound_count(), 1); + assert!(!transport.pool.lock().await.is_empty()); + + drop(peer); + transport.stop_async().await.unwrap(); + } + + /// An established connection quiet for longer than the first-frame + /// deadline, but not the idle deadline, keeps its slot: once a frame is + /// in, the first-frame deadline no longer applies. + /// + /// Break-check: apply the first-frame deadline to every read and the + /// connection is dropped after 200 ms of quiet. + #[tokio::test] + async fn established_connection_quiet_past_the_first_frame_deadline_is_kept() { let (tx, mut rx) = packet_channel(100); let mut transport = TcpTransport::new(TransportId::new(1), None, make_config(), tx); transport.set_first_frame_timeout(Duration::from_millis(200)); + transport.set_inbound_idle_timeout(Duration::from_secs(5)); transport.start_async().await.unwrap(); let listen = transport.local_addr().unwrap(); @@ -2345,7 +2563,7 @@ mod tests { .expect("packet channel closed"); assert_eq!(packet.data, build_msg1_frame()); - // Four deadlines' worth of silence after the first frame. + // Four first-frame deadlines' worth of silence after the first frame. tokio::time::sleep(Duration::from_millis(800)).await; assert_eq!( @@ -2359,6 +2577,54 @@ mod tests { transport.stop_async().await.unwrap(); } + /// The idle deadline covers a complete frame, not its first bytes: a + /// frame after the first whose prefix arrives inside the deadline and + /// whose remainder arrives after it is not delivered, and the slot is + /// released. + /// + /// Break-check: bound only the prefix read and the body read waits + /// forever, so the late frame is delivered. + #[tokio::test] + async fn inbound_connection_dripping_a_frame_slower_than_the_idle_deadline_is_dropped() { + let (tx, mut rx) = packet_channel(100); + let mut transport = TcpTransport::new(TransportId::new(1), None, make_config(), tx); + transport.set_first_frame_timeout(Duration::from_secs(5)); + transport.set_inbound_idle_timeout(Duration::from_millis(300)); + transport.start_async().await.unwrap(); + let listen = transport.local_addr().unwrap(); + + let mut peer = TcpStream::connect(listen).await.unwrap(); + peer.write_all(&squatter_frame()).await.unwrap(); + timeout(Duration::from_secs(2), rx.recv()) + .await + .expect("timeout waiting for the first frame") + .expect("packet channel closed"); + + let frame = squatter_frame(); + // Prefix inside the idle deadline, remainder well past it. + peer.write_all(&frame[..4]).await.unwrap(); + tokio::time::sleep(Duration::from_millis(600)).await; + let _ = peer.write_all(&frame[4..]).await; + + assert!( + timeout(Duration::from_millis(500), rx.recv()) + .await + .is_err(), + "a frame completing after the idle deadline must not be delivered" + ); + assert!( + wait_until( + || transport.stats().pool_inbound_count() == 0, + Duration::from_secs(2) + ) + .await, + "the dripped connection should have released its slot" + ); + + drop(peer); + transport.stop_async().await.unwrap(); + } + /// A genuine peer that is slow to start, but finishes its first frame /// inside the deadline, is admitted. #[tokio::test] @@ -2471,7 +2737,10 @@ mod tests { 1400, stats.clone(), Direction::Inbound, - Some(Duration::from_millis(50)), + Some(InboundDeadline { + first_frame: Duration::from_millis(50), + idle: Duration::from_millis(50), + }), Some(ready_rx), ) .await; diff --git a/src/transport/tor/mod.rs b/src/transport/tor/mod.rs index ed93dcb6..d3000d92 100644 --- a/src/transport/tor/mod.rs +++ b/src/transport/tor/mod.rs @@ -35,7 +35,7 @@ use crate::transport::socks5::{ proxied_send_loop, }; use crate::transport::stream::{ConnId, WRITER_DRAIN_TIMEOUT, drain_writer, next_conn_id}; -use crate::transport::tcp::INBOUND_FIRST_FRAME_TIMEOUT; +use crate::transport::tcp::{INBOUND_FIRST_FRAME_TIMEOUT, INBOUND_IDLE_TIMEOUT, InboundDeadline}; use control::{ControlAuth, TorControlClient, TorMonitoringInfo}; use stats::TorStats; @@ -151,6 +151,10 @@ pub struct TorTransport { cached_monitoring: Arc>>, /// Background monitoring task handle. monitoring_task: Option>, + /// Longest wait for each complete inbound onion frame after the first. + /// Defaults to `INBOUND_IDLE_TIMEOUT`; the node sets it from its + /// liveness timers. + idle_timeout: Duration, } impl TorTransport { @@ -175,9 +179,27 @@ impl TorTransport { control_client: None, cached_monitoring: Arc::new(std::sync::RwLock::new(None)), monitoring_task: None, + idle_timeout: INBOUND_IDLE_TIMEOUT, } } + /// Set the inbound idle deadline for the onion listener: the longest an + /// accepted connection may go, after its first frame, without delivering + /// another complete frame. + /// + /// The node derives it from its own link-liveness timers, so it never + /// drops a connection carrying a link the node would keep. Takes effect + /// at the next `start_async()`. + pub fn set_inbound_idle_timeout(&mut self, d: Duration) { + self.idle_timeout = d; + } + + /// The inbound idle deadline the onion accept loop will use. + #[cfg(test)] + pub(crate) fn inbound_idle_timeout(&self) -> Duration { + self.idle_timeout + } + /// Get the instance name (if configured as a named instance). pub fn name(&self) -> Option<&str> { self.name.as_deref() @@ -359,6 +381,10 @@ impl TorTransport { let mtu = self.config.mtu(); let max_inbound = self.config.max_inbound_connections(); let stats = self.stats.clone(); + let deadline = InboundDeadline { + first_frame: INBOUND_FIRST_FRAME_TIMEOUT, + idle: self.idle_timeout, + }; let accept_handle = tokio::spawn(async move { tor_accept_loop( @@ -368,7 +394,7 @@ impl TorTransport { pool, mtu, max_inbound, - INBOUND_FIRST_FRAME_TIMEOUT, + deadline, stats, ) .await; @@ -1107,9 +1133,9 @@ impl Transport for TorTransport { /// below zero). `direction` is retained for the terminal "receive loop /// stopped" debug field the shared loop deliberately leaves to each transport. /// -/// `first_frame_timeout` is `Some` for an inbound connection, which holds a -/// capped pool slot from the moment it is accepted, and `None` for an -/// outbound one, which holds no such slot. `ready_rx`, when present, is the +/// `deadline` is `Some` for an inbound connection, which holds a capped pool +/// slot from the moment it is accepted, and `None` for an outbound one, +/// which holds no such slot. `ready_rx`, when present, is the /// accept loop's readiness barrier: the loop must not run its cleanup before /// the accept loop has inserted the pool entry and bumped its counter. #[allow(clippy::too_many_arguments)] @@ -1123,7 +1149,7 @@ async fn tor_receive_loop( mtu: u16, stats: Arc, direction: Direction, - first_frame_timeout: Option, + deadline: Option, ready_rx: Option>, ) { proxied_receive_loop( @@ -1136,7 +1162,7 @@ async fn tor_receive_loop( mtu, stats, "Tor", - first_frame_timeout, + deadline, ready_rx, |stats, meta| match meta { Direction::Inbound => stats.record_pool_inbound_removed(), @@ -1164,11 +1190,11 @@ async fn tor_receive_loop( /// socket options, split the stream, and spawn a per-connection /// receive task. /// -/// `first_frame_timeout` is the deadline from accept to the first complete -/// inbound frame, handed to each spawned receive loop. An accepted socket -/// takes an inbound slot against `max_inbound` before any byte is read, so -/// without it a remote that connects and stays silent holds that slot for as -/// long as it keeps the socket open. +/// `deadline` holds the first-frame and idle deadlines handed to each spawned +/// receive loop. An accepted socket takes an inbound slot against +/// `max_inbound` before any byte is read, so without them a remote that +/// connects and stays silent, or sends one frame and then goes silent, holds +/// that slot for as long as it keeps the socket open. #[allow(clippy::too_many_arguments)] async fn tor_accept_loop( listener: TcpListener, @@ -1177,7 +1203,7 @@ async fn tor_accept_loop( pool: ProxiedPool, mtu: u16, max_inbound: usize, - first_frame_timeout: Duration, + deadline: InboundDeadline, stats: Arc, ) { debug!( @@ -1272,7 +1298,7 @@ async fn tor_accept_loop( mtu, recv_stats, Direction::Inbound, - Some(first_frame_timeout), + Some(deadline), Some(ready_rx), ) .await; @@ -2044,7 +2070,8 @@ mod tests { fn spawn_onion_accept_loop( listener: TcpListener, packet_tx: PacketTx, - first_frame_timeout: Duration, + first_frame: Duration, + idle: Duration, ) -> (ProxiedPool, Arc, JoinHandle<()>) { let pool: ProxiedPool = Arc::new(Mutex::new(HashMap::new())); let stats = Arc::new(TorStats::new()); @@ -2055,7 +2082,7 @@ mod tests { pool.clone(), 1400, 64, - first_frame_timeout, + InboundDeadline { first_frame, idle }, stats.clone(), )); (pool, stats, handle) @@ -2070,8 +2097,12 @@ mod tests { let (tx, _rx) = packet_channel(32); let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); let listen = listener.local_addr().unwrap(); - let (pool, stats, accept) = - spawn_onion_accept_loop(listener, tx, Duration::from_millis(200)); + let (pool, stats, accept) = spawn_onion_accept_loop( + listener, + tx, + Duration::from_millis(200), + Duration::from_secs(5), + ); // Held open for the whole test: any release is the deadline's doing. let squatter = TcpStream::connect(listen).await.unwrap(); @@ -2099,8 +2130,12 @@ mod tests { let (tx, mut rx) = packet_channel(32); let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); let listen = listener.local_addr().unwrap(); - let (_pool, stats, accept) = - spawn_onion_accept_loop(listener, tx, Duration::from_millis(300)); + let (_pool, stats, accept) = spawn_onion_accept_loop( + listener, + tx, + Duration::from_millis(300), + Duration::from_secs(5), + ); let frame = build_msg1_frame(); let mut peer = TcpStream::connect(listen).await.unwrap(); @@ -2124,18 +2159,99 @@ mod tests { accept.abort(); } - /// The healthy path, and a regression guard as for TCP: the deadline is - /// scoped to the first iteration, so an established onion connection that - /// then goes quiet keeps its slot. It exists so a future general idle - /// deadline cannot start reaping quiet onion links without a test going - /// red. + /// The smallest frame the reader accepts as established, which the node + /// drops without closing the transport when it names no session. + fn squatter_frame() -> Vec { + let frame = crate::transport::framing::build_established_frame(0); + assert_eq!(frame.len(), crate::proto::fmp::wire::ENCRYPTED_MIN_SIZE); + frame + } + + /// Mirror of the TCP case: an onion-side remote that sends one frame and + /// then goes silent must lose its slot at the idle deadline. The + /// first-frame deadline is far above the idle one, so the release is the + /// idle deadline's doing. Break-check: scope the deadline in the shared + /// loop back to the first read and the count stays at 1. #[tokio::test] - async fn established_onion_connection_survives_long_idle() { + async fn onion_connection_that_goes_silent_after_one_frame_releases_its_slot() { + let (tx, mut rx) = packet_channel(32); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let listen = listener.local_addr().unwrap(); + let (pool, stats, accept) = spawn_onion_accept_loop( + listener, + tx, + Duration::from_secs(5), + Duration::from_millis(300), + ); + + let mut squatter = TcpStream::connect(listen).await.unwrap(); + squatter.write_all(&squatter_frame()).await.unwrap(); + let packet = tokio::time::timeout(Duration::from_secs(2), rx.recv()) + .await + .expect("timeout waiting for the squatter's frame") + .expect("packet channel closed"); + assert_eq!(packet.data, squatter_frame()); + assert_eq!(stats.pool_inbound_count(), 1); + + assert!( + wait_until(|| stats.pool_inbound_count() == 0, Duration::from_secs(2)).await, + "an onion connection silent after its first frame should lose its slot at the idle deadline" + ); + assert!(pool.lock().await.is_empty()); + + drop(squatter); + accept.abort(); + } + + /// The healthy path: an onion connection delivering a frame more often + /// than the idle deadline keeps its slot across many deadlines. + /// + /// Break-check: a deadline that does not re-arm on each frame (a single + /// deadline from accept) drops the connection after the first second. + #[tokio::test] + async fn onion_connection_sending_a_frame_every_interval_below_the_idle_deadline_is_kept() { let (tx, mut rx) = packet_channel(32); let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); let listen = listener.local_addr().unwrap(); let (pool, stats, accept) = - spawn_onion_accept_loop(listener, tx, Duration::from_millis(200)); + spawn_onion_accept_loop(listener, tx, Duration::from_secs(1), Duration::from_secs(1)); + + let mut peer = TcpStream::connect(listen).await.unwrap(); + // Twelve frames 250 ms apart: 3 s, three idle deadlines, with 750 ms + // of slack between each frame and the deadline it re-arms. + for i in 0..12 { + peer.write_all(&squatter_frame()).await.unwrap(); + let packet = tokio::time::timeout(Duration::from_secs(2), rx.recv()) + .await + .unwrap_or_else(|_| panic!("timeout waiting for frame {i}")) + .expect("packet channel closed"); + assert_eq!(packet.data, squatter_frame()); + assert_eq!( + stats.pool_inbound_count(), + 1, + "an onion connection delivering frames inside the idle deadline must keep its slot (frame {i})" + ); + tokio::time::sleep(Duration::from_millis(250)).await; + } + assert!(!pool.lock().await.is_empty()); + + drop(peer); + accept.abort(); + } + + /// An established onion connection quiet for longer than the first-frame + /// deadline, but not the idle deadline, keeps its slot. + #[tokio::test] + async fn established_onion_connection_quiet_past_the_first_frame_deadline_is_kept() { + let (tx, mut rx) = packet_channel(32); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let listen = listener.local_addr().unwrap(); + let (pool, stats, accept) = spawn_onion_accept_loop( + listener, + tx, + Duration::from_millis(200), + Duration::from_secs(5), + ); let mut peer = TcpStream::connect(listen).await.unwrap(); peer.write_all(&build_msg1_frame()).await.unwrap(); @@ -2145,7 +2261,7 @@ mod tests { .expect("packet channel closed"); assert_eq!(packet.data, build_msg1_frame()); - // Four deadlines' worth of silence after the first frame. + // Four first-frame deadlines' worth of silence after the first frame. tokio::time::sleep(Duration::from_millis(800)).await; assert_eq!( @@ -2210,7 +2326,10 @@ mod tests { 1400, stats.clone(), Direction::Inbound, - Some(Duration::from_millis(50)), + Some(InboundDeadline { + first_frame: Duration::from_millis(50), + idle: Duration::from_millis(50), + }), Some(ready_rx), ) .await; @@ -2266,7 +2385,10 @@ mod tests { 1400, recv_stats, Direction::Inbound, - Some(Duration::from_secs(5)), + Some(InboundDeadline { + first_frame: Duration::from_secs(5), + idle: Duration::from_secs(5), + }), Some(ready_rx), ) .await; @@ -2359,7 +2481,10 @@ mod tests { pool.clone(), 1400, 64, - Duration::from_secs(5), + InboundDeadline { + first_frame: Duration::from_secs(5), + idle: Duration::from_secs(5), + }, stats.clone(), )); @@ -2467,7 +2592,8 @@ mod tests { let client_addr = sock.local_addr().unwrap().as_socket().unwrap(); let remote = TransportAddr::from_string(&client_addr.to_string()); - let (pool, stats, accept) = spawn_onion_accept_loop(listener, tx, Duration::from_secs(5)); + let (pool, stats, accept) = + spawn_onion_accept_loop(listener, tx, Duration::from_secs(5), Duration::from_secs(5)); sock.connect(&listen.into()).unwrap(); let std_stream: std::net::TcpStream = sock.into(); @@ -2543,7 +2669,10 @@ mod tests { pool.clone(), 1400, 64, - Duration::from_secs(5), + InboundDeadline { + first_frame: Duration::from_secs(5), + idle: Duration::from_secs(5), + }, stats.clone(), )); diff --git a/testing/chaos/scenarios/bloom-storm.README.md b/testing/chaos/scenarios/bloom-storm.README.md index 146ab79d..f02cb118 100644 --- a/testing/chaos/scenarios/bloom-storm.README.md +++ b/testing/chaos/scenarios/bloom-storm.README.md @@ -30,7 +30,7 @@ actual assignment. A spanning-tree update that changes only an internal path edge — no root change, no depth change — must not produce a sustained bloom announce storm at downstream nodes. The original regression -(rolled-back `0caef2a`, fixed in master `4cdf382`) had this property: +(since rolled back, and fixed in master `4cdf382`) had this property: in the field, a single mid-chain ancestor swap on an upstream node caused every downstream node in its subtree to issue a bloom announce on every parent re-evaluation tick of the upstream node, @@ -74,7 +74,7 @@ including a regressed one. ## Threshold derivation -The original `issues/2026-0019-repro/` reproduction harness measured +The original reproduction harness, kept outside this repository, measured (90s flap window, ~21 induced parent switches at the mid-chain node): @@ -93,8 +93,8 @@ n01=5 n02=5 n03=4 n04=12 n05=6 n06=0 n04 (the flapping node) is the highest because it is legitimately re-sending its filter on its own parent changes. n06 (the depth-4 -"tail") sees 0, matching the calm post-fix behavior recorded in -`issues/2026-0019-repro/RESULTS.md` for the `fix2` variant. +"tail") sees 0, matching the calm post-fix behavior that harness recorded +for its `fix2` variant. In the field, the regression's mesh-wide rate scaled ~480x above steady state. A `30 / 30s / node` ceiling sits ~2.5x above the @@ -132,7 +132,7 @@ applied to every chaos-spawned container. Rationale for ceiling = 40: lab max 30 + ~2σ headroom (≈ 39.4) rounds to 40, giving 33 % margin over the observed lab maximum while still firing -loud on a regression-class storm (the original `0caef2a` regression +loud on a regression-class storm (the original regression scaled mesh-wide bloom traffic ~480× above steady state, far above any plausible jitter band). @@ -140,11 +140,11 @@ plausible jitter band). - The bloom-storm regression has not been confirmed-failing here on a regressed binary in this harness directly; the threshold is - inferred from the values measured in the dedicated - `issues/2026-0019-repro/` post-mortem harness against - `0caef2a`. To gain that confirmation, check out `0caef2a` - (or the `backup-broadcast-gate-bloom-storm` branch if still - retained), build, copy binaries into `testing/docker/`, and rerun + inferred from the values measured in the dedicated post-mortem + harness against a regressed build whose commit is not in this + repository. To gain that confirmation, build a binary that + carries the regression and predates the `4cdf382` fix, copy + binaries into `testing/docker/`, and rerun this scenario; the bloom-rate assertion is expected to fail loud with n05/n06 deltas well above 40. diff --git a/testing/check-action-pins.sh b/testing/check-action-pins.sh index 63a9e2cd..751c43a0 100755 --- a/testing/check-action-pins.sh +++ b/testing/check-action-pins.sh @@ -14,8 +14,7 @@ # # What counts as a violation: any `uses:` reference that is not # * `owner/repo@<40 hex> # ` — the required form, comment mandatory; or -# * a local action, `./path` or `docker://...`; or -# * one of the individually justified references listed below. +# * a local action, `./path` or `docker://...`. # # WHAT THIS GUARD DOES NOT COVER, so a green run is not read as "the workflows # fetch nothing unverified": @@ -43,19 +42,13 @@ REPO_ROOT="$SCRIPT_DIR/.." # it cannot describe, and a pin nobody can read is a pin nobody updates. PINNED_RE='^[^@]+@[0-9a-f]{40} +#.*$' -# Individually justified unpinned references. Each entry is the exact ref text. -# -# Both of these actions read the tool they install from the ref name itself -# (`github.action_ref`), so replacing the ref with a SHA hands them a 40-hex -# string where a toolchain or tool name belongs and the step fails outright. -# They are not pinnable without also moving the selection into `with:`, which -# changes which toolchain resolves, and that is a separate decision from -# pinning. Note what stays exposed: both remain repointable by their upstream -# owners. -ALLOWED_REFS=( - 'dtolnay/rust-toolchain@nightly' - 'taiki-e/install-action@nextest' -) +# There are no exceptions. Some actions select what they install from the ref +# they are called at: a per-tool `taiki-e/install-action` tag, or a +# `dtolnay/rust-toolchain` channel branch, sets the selection input's default +# in that ref's action.yml. Pin such an action by SHA and pass the selection as +# an explicit `with:` input (`tool:`, `toolchain:`). That form works at a SHA +# from any of the action's refs; a bare SHA does not, because their `v2` and +# `master` trees declare the input required with no default. if ! command -v git >/dev/null 2>&1; then echo "check-action-pins: git not available, cannot sweep" >&2 @@ -83,15 +76,6 @@ if [[ ${#files[@]} -eq 0 ]]; then exit 2 fi -# True when this ref is one of the justified references above. -allowed_ref() { - local ref="$1" entry - for entry in "${ALLOWED_REFS[@]}"; do - [[ "$ref" == "$entry" ]] && return 0 - done - return 1 -} - violations=0 checked=0 @@ -117,7 +101,6 @@ for f in "${files[@]}"; do [[ "$ref" == ./* ]] && continue [[ "$ref" == docker://* ]] && continue [[ "$ref" =~ $PINNED_RE ]] && continue - allowed_ref "$ref" && continue echo "$f:$n: $ref" violations=$((violations + 1)) @@ -141,5 +124,5 @@ if [[ $violations -gt 0 ]]; then exit 1 fi -echo "check-action-pins: all $checked action reference(s) pinned or justified" +echo "check-action-pins: all $checked action reference(s) pinned" exit 0 diff --git a/testing/check-comment-refs.sh b/testing/check-comment-refs.sh index 118e6560..3213025a 100755 --- a/testing/check-comment-refs.sh +++ b/testing/check-comment-refs.sh @@ -1,7 +1,7 @@ #!/bin/bash -# ── Source comment reference guard ────────────────────────────────────────── -# Every reference a source comment makes must resolve for a reader who has -# only this repository. +# ── Comment and document reference guard ──────────────────────────────────── +# Every reference a source comment or committed document makes must resolve +# for a reader who has only this repository. # # A comment that names a private planning artifact — by identifier, by file # path, or in programme vocabulary that has no in-repo referent — is dead text @@ -22,17 +22,42 @@ # documents nobody has thought of. Scoped to src/ because resolving the # whole tree's relative documentation links against the repository root # is a different checker with its own population to triage first. -# 3. programme vocabulary, scoped to src/. Phase, rung, milestone and step -# labels that name a plan a reader cannot open. The pattern is shared -# verbatim with the sweep that produced today's clean tree, so the two -# cannot drift apart. +# 3. programme vocabulary, repo-wide in two scopes. Phase, rung, +# milestone and step labels, and phrases such as "architectural plan", +# that name a plan a reader cannot open. VOCAB_PAT runs over the whole +# tree except this file, which would match itself. SRC_VOCAB_PAT adds +# two alternatives that run over src/ only, because outside src/ they +# are legitimate prose: "in Step N" is the documentation's own step +# cross-referencing, and "this PR" is how CONTRIBUTING.md and +# PR-REVIEW.md talk about the PR under review. This file is the only +# copy of both patterns. +# +# The tree-wide alternatives had no hit outside src/ when the scope was +# widened. The one most likely to catch legitimate prose later is +# \b[RQ][0-9]\b, which matches "Q4" or "R2" as words. If it does, +# rephrase the text or move that alternative to SRC_VOCAB_PAT; do not +# exclude the file or directory, which drops every other alternative +# with it. # # Known coverage gaps, recorded rather than discovered: # - a bare "milestone" in some other phrasing (the narrow alternatives keep # the Tor bootstrap loop in src/transport/tor/mod.rs out of check 3); # - the CATEGORY_D / CATEGORY_E code identifiers and test names, which # check 3's case-sensitive Category-[A-Z] deliberately does not match; -# - programme vocabulary, or a document path, outside src/; +# - a document path outside src/. Check 2 resolves a token against the +# repository root, but documentation links outside src/ are relative to +# the citing file, so widening it means a different resolver and a +# triage of the links that fail it; +# - a private-workspace directory cited in lowercase (an issues/... or +# tasks/... path with no .md token). Check 1's identifier prefix is +# case-sensitive and check 2 only resolves *.md tokens; +# - programme phrasing too common to deny, such as "a later step" or +# "the plan": an alternative for it would red legitimate text, so it +# passes check 3 by design; +# - bare commit hashes. Whether a hash resolves depends on the clone's +# object database, and GitHub CI clones are shallow, so a check would +# read differently on different hosts. The rule applied by hand is that +# a comment cites a commit only if a trunk branch contains it; # - a reference to a private artifact made in free prose with no marker at # all, at any scope: no check here matches it; # - a pathspec typo introduced after this file lands. git grep returns 1 @@ -67,23 +92,23 @@ cd "$ROOT" || exit 2 # fires the assertion. This covers checks 2 and 3 only; check 1's pathspec is # "." and stays non-empty from anywhere, so its wrong-directory case is covered # by the --show-prefix assertion above and its mistyped-pathspec case by -# nothing. +# nothing. Check 3's tree-wide half uses "." with exclusions and is in the +# same position as check 1. n=$(git ls-tree -r --name-only HEAD -- src/ | wc -l) (( n > 0 )) || { echo "check-comment-refs: pathspec src/ matched no files" >&2; exit 2; } -# Deliberately a superset of the sweep's own acceptance regex: the year group -# is optional, so the three-digit form is caught as well, and the separator is -# optional so underscores and spaces are caught alongside hyphens. Narrowing -# this is how a whole class goes unguarded while every break-check still -# passes. +# Deliberately broad: the year group is optional, so the three-digit form is +# caught as well, and the separator is optional so underscores and spaces are +# caught alongside hyphens. Narrowing this is how a whole class goes unguarded +# while every break-check still passes. ID_PAT='(TASK|ISSUE|IDEA|QUICK|RECUR)[-_ ]?(20[0-9]{2}[-_ ])?[0-9]{3,4}' -# Verbatim from the sweep's enumerating pattern. Do not edit one without the -# other. If check 3's scope is ever widened beyond src/, this text matches -# itself through six of its alternatives and the widening must add -# ':(exclude)testing/check-comment-refs.sh' — never an exclusion of testing/, -# which holds real check-1 hits a directory-wide exclusion would drop. -VOCAB_PAT='\b[RQ][0-9]\b|Category-[A-Z]|\bumbrella\b|refactor steps?|\bpre-scopes\b|R0 stub|read-isolation|cut over yet|Cutover begins|in Step [0-9]|(the|this) milestone|Milestone-' +# VOCAB_PAT runs tree-wide. This text matches itself, so the tree-wide run +# excludes this one file by name; never exclude testing/, which holds real +# hits a directory-wide exclusion would drop. SRC_VOCAB_PAT is VOCAB_PAT +# plus the alternatives that are legitimate prose outside src/. +VOCAB_PAT='\b[RQ][0-9]\b|Category-[A-Z]|\bumbrella\b|refactor steps?|\bpre-scopes\b|R0 stub|read-isolation|cut over yet|Cutover begins|(the|this) milestone|Milestone-|[Aa]rchitectural plan|[Ff]ollow-up wiring' +SRC_VOCAB_PAT="$VOCAB_PAT"'|in Step [0-9]|\b[Tt]his PR\b' MD_PAT='[A-Za-z0-9_./-]+\.md\b' @@ -157,22 +182,29 @@ if (( ${#CITES[@]} > 0 )); then done < <(printf '%s\n' "${!CITES[@]}" | sort) fi -# ── Check 3: programme vocabulary under src/ ──────────────────────────────── -rc=0 -out=$(git grep -nEI "$VOCAB_PAT" HEAD -- src/) || rc=$? -if (( rc > 1 )); then - echo "check-comment-refs: check 3 grep failed (rc=$rc)" >&2 - exit 2 -fi -if [[ -n "$out" ]]; then - printf '%s\n' "$out" \ - | sed -E 's|^HEAD:([^:]*):([0-9]+):|\1:\2: names a plan this repository does not carry: |' - FAILED=1 -fi +# ── Check 3: programme vocabulary, src/ and the rest of the tree ──────────── +vocab_check() { + local pat=$1 + shift + local rc=0 out + out=$(git grep -nEI "$pat" HEAD -- "$@") || rc=$? + if (( rc > 1 )); then + echo "check-comment-refs: check 3 grep failed (rc=$rc)" >&2 + exit 2 + fi + if [[ -n "$out" ]]; then + printf '%s\n' "$out" \ + | sed -E 's|^HEAD:([^:]*):([0-9]+):|\1:\2: names a plan this repository does not carry: |' + FAILED=1 + fi + return 0 +} +vocab_check "$SRC_VOCAB_PAT" src/ +vocab_check "$VOCAB_PAT" . ':(exclude)src/' ':(exclude)testing/check-comment-refs.sh' if (( FAILED != 0 )); then echo "" >&2 - echo "check-comment-refs: a source comment references something a reader holding" >&2 + echo "check-comment-refs: a comment or document references something a reader holding" >&2 echo "only this repository cannot resolve. Rewrite the comment to say what the" >&2 echo "code does, citing nothing outside the tree." >&2 exit 1 diff --git a/testing/check-deb-version.sh b/testing/check-deb-version.sh deleted file mode 100755 index a18b747b..00000000 --- a/testing/check-deb-version.sh +++ /dev/null @@ -1,175 +0,0 @@ -#!/bin/bash -# ── Debian package version derivation check ───────────────────────────────── -# A release candidate is tagged vX.Y.Z-rcN, because git refuses '~' in a ref -# name. dpkg reads X.Y.Z-rcN as revision rcN of X.Y.Z and sorts it ABOVE the -# release, so a host that installed the candidate is never upgraded by the -# release. package-linux.yml's "Derive Linux package version" step therefore -# maps the tag's pre-release suffix to '~' for the .deb, which dpkg sorts below. -# -# This runs that step's own text, taken from the workflow, rather than a copy, -# so the check and the workflow cannot drift apart. Cases: -# refs/tags/v0.5.3 both versions 0.5.3 -# refs/tags/v0.5.3-rc1 deb 0.5.3~rc1, tarball and artifact 0.5.3-rc1, and -# dpkg orders 0.5.2 < 0.5.3~rc1 < 0.5.3 -# refs/heads/maint deb equals the tarball version, which is -# +maint.. -# -# It also checks the wiring the derivation needs to have any effect: the job -# declares the deb_package_version output, the .deb build passes it as -# --version, and that step renames a '~' in the package file name to '-' (a -# GitHub release renames an asset with special characters in its name, which -# would leave checksums-linux.txt naming a file the release does not have). -# Those three are read from the text, not executed. -# -# Exit 0 = clean. Exit 1 = a case or a wiring check failed. Exit 2 = the check -# could not run (no dpkg, no PyYAML, the step not found, or an output missing); -# never treated as a pass. -# ───────────────────────────────────────────────────────────────────────────── -set -uo pipefail - -SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" -PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" -WORKFLOW="$PROJECT_ROOT/.github/workflows/package-linux.yml" - -cant() { - echo "check-deb-version: $*; cannot verify the Debian version derivation" >&2 - exit 2 -} - -[ -f "$WORKFLOW" ] || cant "missing $WORKFLOW" -command -v dpkg >/dev/null 2>&1 || cant "dpkg not found" -command -v python3 >/dev/null 2>&1 || cant "python3 not found" -python3 -c "import yaml" >/dev/null 2>&1 || cant "python3 module 'yaml' not found" - -WORK=$(mktemp -d) || cant "mktemp failed" -trap 'rm -rf "$WORK"' EXIT - -# Pull the derivation step's run text, the job's declared outputs and the -# .deb build step's run text out of the workflow. -if ! python3 - "$WORKFLOW" "$WORK" <<'PY' -import sys -from pathlib import Path - -import yaml - -workflow, work = sys.argv[1], Path(sys.argv[2]) -doc = yaml.safe_load(Path(workflow).read_text(encoding="utf-8")) -jobs = doc.get("jobs") or {} - - -def step_run(job, name): - for step in (jobs.get(job) or {}).get("steps") or []: - if step.get("name") == name: - return step.get("run") - return None - - -derive = step_run("determine-versioning", "Derive Linux package version") -build = step_run("build", "Build Debian package in the pinned container") -if not derive or not build: - missing = "derivation" if not derive else "Debian build" - print(f"check-deb-version: the {missing} step was not found in {workflow}", - file=sys.stderr) - sys.exit(2) -(work / "derive.sh").write_text(derive, encoding="utf-8") -(work / "build.sh").write_text(build, encoding="utf-8") -outputs = (jobs.get("determine-versioning") or {}).get("outputs") or {} -(work / "outputs").write_text( - "".join(f"{k}={v}\n" for k, v in outputs.items()), encoding="utf-8") -PY -then - exit 2 -fi - -FAILED=0 -ok() { echo " PASS $*"; } -bad() { echo " FAIL $*"; FAILED=$((FAILED + 1)); } - -# Run the derivation for one ref and load its outputs into LINUX and DEB. -# Exits 2 when the step fails or does not write both outputs: no version was -# established, which is not a failed case. -derive() { - local ref="$1" out="$WORK/github_output" - : > "$out" - if ! (cd "$PROJECT_ROOT" && GITHUB_OUTPUT="$out" GITHUB_REF="$ref" \ - GITHUB_REF_NAME="${ref#refs/*/}" bash -eo pipefail "$WORK/derive.sh") >"$WORK/derive.log" 2>&1; then - echo "check-deb-version: the derivation failed for $ref:" >&2 - cat "$WORK/derive.log" >&2 - exit 2 - fi - LINUX=$(sed -n 's/^linux_package_version=//p' "$out") - DEB=$(sed -n 's/^deb_package_version=//p' "$out") - if [ -z "$LINUX" ] || [ -z "$DEB" ]; then - cant "the derivation for $ref did not write both outputs (linux '$LINUX', deb '$DEB')" - fi - return 0 -} - -# Record whether dpkg orders $1 $2 $3 (lt, gt, ...). -expect_order() { - if dpkg --compare-versions "$1" "$2" "$3"; then - ok "dpkg: $1 $2 $3" - else - bad "dpkg: $1 $2 $3 does not hold" - fi - return 0 -} - -# Record whether a derived version equals the expected one. -expect_eq() { - local label="$1" got="$2" want="$3" - if [ "$got" = "$want" ]; then - ok "$label: $got" - else - bad "$label: $got (want $want)" - fi - return 0 -} - -echo "Release tag refs/tags/v0.5.3" -derive refs/tags/v0.5.3 -expect_eq "linux_package_version" "$LINUX" "0.5.3" -expect_eq "deb_package_version" "$DEB" "0.5.3" - -echo "Candidate tag refs/tags/v0.5.3-rc1" -derive refs/tags/v0.5.3-rc1 -expect_eq "linux_package_version" "$LINUX" "0.5.3-rc1" -expect_eq "deb_package_version" "$DEB" "0.5.3~rc1" -expect_order "$DEB" lt 0.5.3 -expect_order "$DEB" gt 0.5.2 - -echo "Branch refs/heads/maint" -derive refs/heads/maint -CRATE=$(awk -F'"' '/^version = /{print $2; exit}' "$PROJECT_ROOT/Cargo.toml") -HEIGHT=$(git -C "$PROJECT_ROOT" rev-list --count HEAD) || cant "git rev-list failed" -HASH=$(git -C "$PROJECT_ROOT" rev-parse --short HEAD) || cant "git rev-parse failed" -[ -n "$CRATE" ] || cant "no version in Cargo.toml" -expect_eq "linux_package_version" "$LINUX" "${CRATE}+maint.${HEIGHT}.${HASH}" -expect_eq "deb_package_version" "$DEB" "$LINUX" - -echo "Wiring" -# shellcheck disable=SC2016 # the ${{ }} expressions are workflow text, not shell -if grep -qxF 'deb_package_version=${{ steps.linux_version.outputs.deb_package_version }}' "$WORK/outputs"; then - ok "determine-versioning declares the deb_package_version output" -else - bad "determine-versioning does not declare deb_package_version from the derivation step" -fi -# shellcheck disable=SC2016 -if grep -qF -- '--version "${{ needs.determine-versioning.outputs.deb_package_version }}"' "$WORK/build.sh"; then - ok "the .deb build passes deb_package_version as --version" -else - bad "the .deb build does not pass deb_package_version as --version" -fi -if grep -qF "tr '~' '-'" "$WORK/build.sh"; then - ok "the .deb build renames a '~' in the package file name" -else - bad "the .deb build does not rename a '~' in the package file name" -fi - -echo -if [ "$FAILED" -eq 0 ]; then - echo "check-deb-version: all checks passed" - exit 0 -fi -echo "check-deb-version: $FAILED check(s) failed" -exit 1 diff --git a/testing/check-glibc-floor.sh b/testing/check-glibc-floor.sh index 1ca81deb..cdce9506 100755 --- a/testing/check-glibc-floor.sh +++ b/testing/check-glibc-floor.sh @@ -16,17 +16,37 @@ # Anything else is treated as a single ELF binary. # # Reads the floor from packaging/build-floor.env unless FIPS_GLIBC_FLOOR is set. +# +# Exit 0 = every input was examined and none is above the floor. Exit 1 = a +# binary needs a newer glibc than the floor. Exit 2 = an input could not be +# examined (missing, not an ELF object, a .deb that would not unpack or holds +# no binaries), or the check could not run at all; never treated as a pass, +# and never reported as a binary above the floor. set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" REPO_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" +# A missing or empty floor means the check cannot run, so it exits 2 here +# rather than letting `set -e` or a `:?` expansion end the run with status 1, +# which callers would read as a binary above the floor. +FLOOR_ENV="$REPO_ROOT/packaging/build-floor.env" if [ -z "${FIPS_GLIBC_FLOOR:-}" ]; then - # shellcheck source=../packaging/build-floor.env - . "$REPO_ROOT/packaging/build-floor.env" + [ -r "$FLOOR_ENV" ] || { + echo "check-glibc-floor: cannot read $FLOOR_ENV and FIPS_GLIBC_FLOOR is not set;" >&2 + echo " there is no floor to check against." >&2 + exit 2 + } + # shellcheck source-path=SCRIPTDIR source=../packaging/build-floor.env + . "$FLOOR_ENV" fi -FLOOR="${FIPS_GLIBC_FLOOR:?no floor declared}" +if [ -z "${FIPS_GLIBC_FLOOR:-}" ]; then + echo "check-glibc-floor: $FLOOR_ENV declares no FIPS_GLIBC_FLOOR;" >&2 + echo " there is no floor to check against." >&2 + exit 2 +fi +FLOOR="$FIPS_GLIBC_FLOOR" for tool in readelf dpkg dpkg-deb; do command -v "$tool" >/dev/null 2>&1 || { @@ -65,6 +85,18 @@ max_glibc_need() { FAILED=0 CHECKED=0 +UNCHECKED=0 +UNCHECKED_LIST=() + +# unchecked