From dc3e7945ce687a69edf023db780113016b089dfd Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 25 Aug 2026 20:22:02 +0100 Subject: [PATCH 01/15] Create the GitHub Release object automatically on a tag push Nothing in the repository published the Release object. All five packaging workflows run a "Wait for tag release" step that polls `gh release view` twenty times at fifteen-second intervals and then fails the job, so a tag push failed every packaging leg unless someone ran `gh release create` by hand inside that five-minute budget. The previous creator lived in package-openwrt.yml and generated the release body from the commit list, which raced the hand-created object and could strand the written release notes. It was removed for v0.4.2 and nothing replaced it. This adds a dedicated workflow instead, so there is one creator, it runs as early as a tag push allows, and it publishes `docs/releases/release-notes-.md` rather than a generated commit list. It is idempotent in both directions: an existing Release is left untouched, and a Release created concurrently between the check and the create is treated as success. A pre-release tag is marked as such and gets the RC placeholder body, since the written notes are finalized only for the stable tag. (cherry picked from commit 3429fd6064a8b8ea1f87023cfccabd499696a37f) --- .github/workflows/release.yml | 77 +++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 .github/workflows/release.yml diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml new file mode 100644 index 00000000..3c13b858 --- /dev/null +++ b/.github/workflows/release.yml @@ -0,0 +1,77 @@ +name: Release Object +on: + push: + tags: + - "v*" + workflow_dispatch: + inputs: + tag: + description: "Tag to create the Release object for" + required: true + +# Every packaging workflow's upload job runs a `Wait for tag release` step that +# polls `gh release view` 20 times at 15s intervals and then fails the job. Five +# waiters and no creator means a tag push fails all five unless a human runs +# `gh release create` inside that five-minute budget. This job is the creator. +# +# It deliberately does not live in a packaging workflow. The previous creator did +# (package-openwrt.yml, via an action with generate_release_notes), and it raced +# the operator's hand-created object for the right to decide the Release body; +# whichever lost left the written notes stranded. Here there is one creator, it +# publishes the notes file the release wrote, and it is idempotent, so running it +# alongside a hand-created object is a no-op rather than a race. + +permissions: + contents: read + +concurrency: + group: release-object-${{ github.ref }} + +jobs: + create-release: + name: Create the Release object + runs-on: ubuntu-latest + permissions: + contents: write + steps: + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 + with: + ref: ${{ inputs.tag || github.ref }} + + - name: Create the Release object if it does not exist + env: + GH_TOKEN: ${{ github.token }} + TAG: ${{ inputs.tag || github.ref_name }} + run: | + set -euo pipefail + + if gh release view "${TAG}" --repo "${GITHUB_REPOSITORY}" >/dev/null 2>&1; then + echo "Release ${TAG} already exists; leaving it untouched." + exit 0 + fi + + args=(--repo "${GITHUB_REPOSITORY}" --title "FIPS ${TAG}") + + # A tag carrying a pre-release suffix (v0.5.0-rc1) is an RC: it is + # marked as a pre-release and has no written notes of its own, since + # the notes are finalized for the stable tag. + notes="docs/releases/release-notes-${TAG}.md" + if [[ "${TAG}" == *-* ]]; then + args+=(--prerelease --notes "Release candidate - packaging validation only.") + elif [[ -f "${notes}" ]]; then + args+=(--notes-file "${notes}") + else + # Never --generate-notes: an auto-generated commit list published + # under a stable tag is exactly what stranded the written notes + # before. An empty body is visibly wrong and trivially editable. + echo "::warning::${notes} is missing; publishing ${TAG} with an empty body." + args+=(--notes "") + fi + + if ! gh release create "${TAG}" "${args[@]}"; then + # A concurrent creator - the operator by hand, or a re-run - may have + # won between the check above and here. An object that now exists is + # the outcome this job wanted. + gh release view "${TAG}" --repo "${GITHUB_REPOSITORY}" >/dev/null 2>&1 + echo "Release ${TAG} was created concurrently; nothing to do." + fi From 644630551649c9e529e82c8feced8e421546069e Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 25 Aug 2026 20:22:55 +0100 Subject: [PATCH 02/15] Land the reproducible-tarball gate in the source tree Release gate 11 builds the systemd install tarball twice and compares the two byte for byte. packaging/systemd/build-tarball.sh already carries the tar-level determinism work; what never existed in the tree was the wrapper that actually builds twice and compares. The gate ran for the first time at v0.4.2 and passed, but it ran from outside the repository, so the release could not run its own gate. The logic is unchanged from the version that passed at v0.4.2. Only its assumptions about living outside the tree are fixed: the source repo now defaults to the script's own checkout instead of a sibling path, output lands under the gitignored target/ rather than a lab-runs directory, and the checkout test uses `git rev-parse --git-dir` so a run from a linked worktree, whose .git is a file, is not rejected. (cherry picked from commit c3d75639aa0ce45500fab5d32faeae58369b4763) --- testing/repro-tarball-gate.sh | 101 ++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100755 testing/repro-tarball-gate.sh diff --git a/testing/repro-tarball-gate.sh b/testing/repro-tarball-gate.sh new file mode 100755 index 00000000..34f55844 --- /dev/null +++ b/testing/repro-tarball-gate.sh @@ -0,0 +1,101 @@ +#!/usr/bin/env bash +# Gate 11: build the systemd install tarball twice and compare the two byte for byte. +# +# packaging/systemd/build-tarball.sh already carries the tar-level determinism work +# (SOURCE_DATE_EPOCH, --mtime, --sort=name, --numeric-owner --owner=0 --group=0). +# What has never existed at any FIPS release is a wrapper that actually builds twice +# and compares, so this supplies only that. +# +# Builds run in throwaway worktrees, never in a working checkout, so no existing +# target/ cache is destroyed and no checkout is left dirty. +# +# A and B same path, built twice -- this is the gate +# C a different path -- probe only, never gating: there is no +# [profile.release], no .cargo/config.toml and no --remap-path-prefix, +# so an absolute build path can reach the binaries +# +# Usage: testing/repro-tarball-gate.sh [src-repo] [out-dir] +# Exit: 0 if A and B are identical, 1 if they differ, 2 on a setup failure. + +set -euo pipefail + +REF="${1:?usage: repro-tarball-gate.sh [src-repo] [out-dir]}" +REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)" +SRC_REPO="${2:-${REPO_ROOT}}" +# Under target/, which is gitignored, so a gate run never dirties the checkout +# it was launched from. +OUT_DIR="${3:-${REPO_ROOT}/target/repro-tarball}" + +# `--git-dir` rather than a test for a .git directory: a linked worktree carries +# a .git file, and the release runs this out of whatever checkout is to hand. +git -C "${SRC_REPO}" rev-parse --git-dir >/dev/null 2>&1 \ + || { echo "no source checkout at ${SRC_REPO}" >&2; exit 2; } + +SHA="$(git -C "${SRC_REPO}" rev-parse "${REF}")" +# Pin explicitly rather than letting build-tarball.sh derive it, so all three +# builds share one value even if they are cut from different worktrees. +SOURCE_DATE_EPOCH="$(git -C "${SRC_REPO}" log -1 --format=%ct "${SHA}")" +export SOURCE_DATE_EPOCH + +WORK="$(mktemp -d -t repro-gate-XXXXXX)" +mkdir -p "${OUT_DIR}" +trap 'for w in "${WORK}"/*; do [ -d "$w" ] && git -C "${SRC_REPO}" worktree remove --force "$w" 2>/dev/null || true; done; rm -rf "${WORK}"' EXIT + +echo "ref ${REF} (${SHA})" +echo "SOURCE_DATE_EPOCH ${SOURCE_DATE_EPOCH}" +echo "work ${WORK}" +echo + +# Build one tarball in a fresh worktree at $1, leaving it at $2. +build() { + local dir="$1" dest="$2" label="$3" + echo "=== ${label}: ${dir}" + git -C "${SRC_REPO}" worktree add --detach --quiet "${dir}" "${SHA}" + ( cd "${dir}" && ./packaging/systemd/build-tarball.sh ) >"${OUT_DIR}/${label}.log" 2>&1 || { + echo "${label}: build failed, see ${OUT_DIR}/${label}.log" >&2 + tail -20 "${OUT_DIR}/${label}.log" >&2 + exit 2 + } + local tb + tb="$(ls "${dir}"/deploy/*.tar.gz)" + cp "${tb}" "${dest}" + echo "${label}: $(sha256sum "${dest}" | cut -d' ' -f1) $(basename "${tb}")" + git -C "${SRC_REPO}" worktree remove --force "${dir}" +} + +# A and B share one path, so the second reuses nothing: the worktree is removed +# and recreated between them, which is what makes this a real rebuild. +build "${WORK}/same" "${OUT_DIR}/A.tar.gz" A +build "${WORK}/same" "${OUT_DIR}/B.tar.gz" B +build "${WORK}/other-path-for-the-probe" "${OUT_DIR}/C.tar.gz" C + +echo +rc=0 +if cmp -s "${OUT_DIR}/A.tar.gz" "${OUT_DIR}/B.tar.gz"; then + echo "GATE PASS: A and B are byte-identical" +else + echo "GATE FAIL: A and B differ" + rc=1 +fi + +if cmp -s "${OUT_DIR}/A.tar.gz" "${OUT_DIR}/C.tar.gz"; then + echo "PROBE: a different build path changes nothing" +else + echo "PROBE: a different build path changes the tarball (not gating)" +fi + +# Localize any difference to the file level, for both the gate and the probe. +for pair in A:B A:C; do + l="${pair%%:*}"; r="${pair##*:}" + cmp -s "${OUT_DIR}/${l}.tar.gz" "${OUT_DIR}/${r}.tar.gz" && continue + echo + echo "--- per-member digests, ${l} vs ${r}" + for s in "${l}" "${r}"; do + rm -rf "${WORK}/x-${s}"; mkdir -p "${WORK}/x-${s}" + tar -xzf "${OUT_DIR}/${s}.tar.gz" -C "${WORK}/x-${s}" + ( cd "${WORK}/x-${s}" && find . -type f | sort | xargs sha256sum ) >"${WORK}/d-${s}.txt" + done + diff "${WORK}/d-${l}.txt" "${WORK}/d-${r}.txt" || true +done + +exit "${rc}" From 38f003bc6f0fcde71d16892252ff1e4093dfe7a8 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 25 Aug 2026 20:25:43 +0100 Subject: [PATCH 03/15] Make the interop gate's Phase 7 assert a settled mesh-size estimate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 7 was labelled "mesh-size estimate convergence" but recorded each node's first in-band reading: once a node landed inside [0.75N, 1.25N] it was marked OK and never polled again, and the loop exited as soon as every node had been in band once. The sample was therefore taken inside the documented bloom-filter warmup — the design notes put that at roughly five minutes from node start and call estimates unreliable before it — so the phase could report a converged mesh from a measurement that cannot show convergence. The v0.4.2 multihop run printed 6/6 with two of six nodes reporting 5. The phase now waits out the warmup before any reading counts (MESH_SIZE_WARMUP, default 300s, measured from `compose up` rather than from the phase's own start), then requires each node to hold the band unbroken for MESH_SIZE_SETTLE (default 60s). Every node is re-polled every round and an out-of-band reading restarts that node's settle clock, so a node can no longer be credited for a sample it has since contradicted. MESH_SIZE_TIMEOUT is now the extra budget on top of those two windows. The warmup is polled and printed rather than slept through, which records the estimate trajectory no run has ever captured. The band is unchanged: whether the undercounting nodes would reach N given time is not established, and tightening it without that evidence would only add an unexplained red. A closing NOTE reports whether the nodes agree on a value, since a green asserts per-node plausibility and not agreement — which is how a prior release's evidence came to be read. (cherry picked from commit ee978c1a57b2bfc0f7e33a0475806d19fcac3935) --- testing/interop/interop-test.sh | 91 +++++++++++++++++++++++++++------ 1 file changed, 76 insertions(+), 15 deletions(-) diff --git a/testing/interop/interop-test.sh b/testing/interop/interop-test.sh index 83c1ac9f..8c4731a9 100755 --- a/testing/interop/interop-test.sh +++ b/testing/interop/interop-test.sh @@ -53,7 +53,12 @@ # by --topology; empty = streams off. # STREAM_LOSS_MARGIN_PCT rekey-vs-control loss margin (default 1). # CONTROL_STREAM_SECS quiet control-window length (default 12). -# MESH_SIZE_TIMEOUT Phase 7 convergence poll budget (default 180). +# MESH_SIZE_WARMUP Phase 7 bloom warmup, from mesh start, before +# any estimate counts (default 300). +# MESH_SIZE_SETTLE Phase 7 unbroken in-band window a node must +# hold to be credited (default 60). +# MESH_SIZE_TIMEOUT Phase 7 poll budget beyond warmup+settle +# (default 180). # FIPS_INTEROP_KEEP_UP 1 = leave containers running after the test (debug). # REKEY_AFTER_SECS rekey interval to generate configs with (default # 35; multihop-3v-cycle defaults it to 50). @@ -180,9 +185,17 @@ STREAM_LOSS_MARGIN_PCT="${STREAM_LOSS_MARGIN_PCT:-1}" # The rekey-window stream must span Phases 2-5 (both cutovers + reconverge). REKEY_STREAM_SECS=$(( FIRST_REKEY_TIMEOUT + SECOND_REKEY_WAIT + POST_REKEY_TIMEOUT + 15 )) -# Mesh-size estimate convergence (strict ±25% of true N). Generous poll -# budget — the bloom-union estimate converges over minutes. +# Mesh-size estimate convergence (strict ±25% of true N). The archived +# design note puts bloom-filter warmup at ~5 minutes from node start and +# calls the estimate unreliable before that, so nothing sampled inside +# MESH_SIZE_WARMUP is evidence of convergence, and a node is credited +# only after holding the band unbroken for MESH_SIZE_SETTLE afterwards. +# MESH_SIZE_TIMEOUT is the extra budget for reaching that state, on top +# of the warmup and settle windows rather than covering them. +MESH_SIZE_WARMUP="${MESH_SIZE_WARMUP:-300}" +MESH_SIZE_SETTLE="${MESH_SIZE_SETTLE:-60}" MESH_SIZE_TIMEOUT="${MESH_SIZE_TIMEOUT:-180}" +MESH_SIZE_POLL=5 # ── Counters ───────────────────────────────────────────────────────── @@ -512,6 +525,14 @@ wait_for_log_pattern_count() { [ "$(count_log_pattern "$pattern")" -ge "$min_count" ] } +# One node's bloom-union mesh-size estimate, or "null" when the daemon +# has no estimate yet or cannot be reached. +mesh_estimate() { + docker exec "${CONTAINER[$1]}" fipsctl show status 2>/dev/null \ + | python3 -c "import sys,json; v=json.load(sys.stdin).get('estimated_mesh_size'); print(v if v is not None else 'null')" 2>/dev/null \ + || echo null +} + # ── Data-plane continuity streams ──────────────────────────────────── # # A sustained ping6 stream over the overlay (.fips) is data-plane @@ -661,6 +682,9 @@ if ! compose up -d; then echo " FAIL compose up failed" exit 1 fi +# Phase 7 measures bloom-filter warmup from node start, not from its own +# start, so the clock has to be taken here. +MESH_UP_AT=$SECONDS # Optional netem: applied via `docker exec ... tc qdisc` on each # container's eth0 — host bridge qdisc does NOT shape inter-container @@ -948,43 +972,80 @@ echo "" # .estimated_mesh_size) should converge to the true node count across # versions. A mixed-version bloom/tree-encoding divergence shows up as a # node that never produces an in-band estimate (or returns null). Strict -# band = [0.75N, 1.25N]; polled up to MESH_SIZE_TIMEOUT (the estimate -# converges over minutes and is transiently jittery). +# band = [0.75N, 1.25N]. +# +# The verdict is taken on a settled estimate rather than on a node's +# first tolerable reading. Two windows enforce that. Nothing sampled +# before MESH_SIZE_WARMUP has elapsed since the mesh came up counts at +# all, because the estimate is documented as unreliable during warmup; +# after it, a node is credited only once it has held the band unbroken +# for MESH_SIZE_SETTLE. Every node is re-polled every round and any +# out-of-band reading restarts that node's settle clock, so a node +# cannot be credited for a sample it has since contradicted. echo "Phase 7: Mesh-size estimate convergence (strict ±25% of true N=$NUM_NODES)" PASSED=0; FAILED=0 ms_lo="$(awk -v n="$NUM_NODES" 'BEGIN{printf "%.2f", 0.75*n}')" ms_hi="$(awk -v n="$NUM_NODES" 'BEGIN{printf "%.2f", 1.25*n}')" -echo " band [$ms_lo, $ms_hi], poll up to ${MESH_SIZE_TIMEOUT}s" -declare -A MS_EST MS_OK -ms_deadline=$(( SECONDS + MESH_SIZE_TIMEOUT )) +declare -A MS_EST MS_OK MS_INBAND_SINCE +ms_accept_after=$(( MESH_UP_AT + MESH_SIZE_WARMUP )) +echo " band [$ms_lo, $ms_hi], warmup ends $(( ms_accept_after - SECONDS ))s from now, then ${MESH_SIZE_SETTLE}s settled, poll up to ${MESH_SIZE_TIMEOUT}s beyond that" + +# Poll through the warmup as well. Nothing here is asserted on — it is +# the trajectory the phase has never recorded, and it is what an +# undercount that never recovers would show up in. +while [ "$SECONDS" -lt "$ms_accept_after" ]; do + ms_line="" + for n in "${NODES[@]}"; do + MS_EST[$n]="$(mesh_estimate "$n")" + ms_line+=" $n=${MS_EST[$n]}" + done + echo " warmup, $(( ms_accept_after - SECONDS ))s to go:$ms_line" + sleep 30 +done + +ms_deadline=$(( SECONDS + MESH_SIZE_SETTLE + MESH_SIZE_TIMEOUT )) while :; do all_ok=1 for n in "${NODES[@]}"; do - [ "${MS_OK[$n]:-0}" = "1" ] && continue - est="$(docker exec "${CONTAINER[$n]}" fipsctl show status 2>/dev/null \ - | python3 -c "import sys,json; v=json.load(sys.stdin).get('estimated_mesh_size'); print(v if v is not None else 'null')" 2>/dev/null || echo null)" + est="$(mesh_estimate "$n")" MS_EST[$n]="$est" if [ "$est" != "null" ] && awk "BEGIN{exit !($est>=$ms_lo && $est<=$ms_hi)}"; then + [ -z "${MS_INBAND_SINCE[$n]:-}" ] && MS_INBAND_SINCE[$n]="$SECONDS" + else + unset "MS_INBAND_SINCE[$n]" + fi + if [ -n "${MS_INBAND_SINCE[$n]:-}" ] \ + && [ $(( SECONDS - ${MS_INBAND_SINCE[$n]} )) -ge "$MESH_SIZE_SETTLE" ]; then MS_OK[$n]=1 else + MS_OK[$n]=0 all_ok=0 fi done [ "$all_ok" = "1" ] && break [ "$SECONDS" -ge "$ms_deadline" ] && break - sleep 3 + sleep "$MESH_SIZE_POLL" done for n in "${NODES[@]}"; do s="${SLOT_OF[$n]}"; u="$(echo "$s" | tr '[:lower:]' '[:upper:]')" if [ "${MS_OK[$n]:-0}" = "1" ]; then - echo " PASS $n [$u]: estimated_mesh_size=${MS_EST[$n]}" + echo " PASS $n [$u]: estimated_mesh_size=${MS_EST[$n]} (held band ${MESH_SIZE_SETTLE}s+ after warmup)" PASSED=$((PASSED + 1)) else - echo " FAIL $n [$u]: estimated_mesh_size=${MS_EST[$n]:-null} (outside [$ms_lo,$ms_hi] after ${MESH_SIZE_TIMEOUT}s)" + if [ -n "${MS_INBAND_SINCE[$n]:-}" ]; then + ms_why="held band only $(( SECONDS - ${MS_INBAND_SINCE[$n]} ))s of the ${MESH_SIZE_SETTLE}s settle window" + else + ms_why="outside [$ms_lo,$ms_hi]" + fi + echo " FAIL $n [$u]: estimated_mesh_size=${MS_EST[$n]:-null} ($ms_why)" FAILED=$((FAILED + 1)) - INTEROP_FAILURES+=("[mesh-size] node $n ($u ${SLOT_REF[$s]}@${SLOT_SHA[$s]}): estimate=${MS_EST[$n]:-null} outside [$ms_lo,$ms_hi]") + INTEROP_FAILURES+=("[mesh-size] node $n ($u ${SLOT_REF[$s]}@${SLOT_SHA[$s]}): estimate=${MS_EST[$n]:-null} $ms_why") fi done +# The band is per-node, so a green says every node was plausible, not +# that the nodes agreed. Print the spread so nobody has to infer it. +ms_spread="$(printf '%s\n' "${MS_EST[@]}" | sort -n | awk '/^[0-9]+$/{ if (lo=="") lo=$1; hi=$1 } END{ if (lo=="") print "no numeric estimates"; else if (lo==hi) print "all nodes agree on " lo; else print "nodes disagree: " lo " to " hi }')" +echo " NOTE: final estimates — $ms_spread (agreement is reported, not asserted)" phase_result "Mesh-size estimate convergence" echo "" From fca5b2b102531a383db9ca15dbaaca643bcd4ff0 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 25 Aug 2026 20:27:02 +0100 Subject: [PATCH 04/15] Give the interop gate's Phase 5b a threshold it can decide on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 5b compares data-plane loss across the rekey window against a quiet control window and fails the stream when the difference exceeds STREAM_LOSS_MARGIN_PCT. Both windows streamed at 5 Hz, so the control window was 60 packets and a 1% margin was 0.6 of a packet: below the 1.67% quantisation of its own measurement, which meant a single packet's difference decided the verdict. Across eight netem runs the baseline alone ranged from 0.0% to 11.7% on the same nominal impairment, and the check failed on same-version pairs as readily as on mixed ones — so the arm could neither clear a release nor condemn one. The control window cannot simply be lengthened: it has to fit inside a rekey interval to stand a chance of being cutover-free, and it already gets contaminated at 12s. So the sample is raised by rate instead. At 20 Hz the control window is 240 packets and the rekey window ~3100, and the margin becomes 5% — 12 control packets, past three sigma of the sampling spread on a path losing up to 6%, and still well inside the blackhole a rekey that is not hitless would produce, since that runs until the session reconverges and 5% of the rekey window is about 8s. The reasoning is written at both constants, since an unexplained constant is how the 1% got there. The ping interval is now derived from STREAM_RATE_HZ rather than hardcoded at 0.2s alongside it, so the loss denominator cannot drift from the rate actually sent. Not addressed here: Phase 1b still reports through a control window it has detected a cutover in, rather than re-measuring or abstaining. (cherry picked from commit 797728be9bd2a8b69fe164bb56aa6f9cd70351b0) --- testing/interop/interop-test.sh | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/testing/interop/interop-test.sh b/testing/interop/interop-test.sh index 8c4731a9..3d8e41dd 100755 --- a/testing/interop/interop-test.sh +++ b/testing/interop/interop-test.sh @@ -51,7 +51,7 @@ # FIPS_INTEROP_STREAMS data-plane stream pairs (`nid-nid` tokens, both # directions streamed). Enables Phase 1b/5b. Set # by --topology; empty = streams off. -# STREAM_LOSS_MARGIN_PCT rekey-vs-control loss margin (default 1). +# STREAM_LOSS_MARGIN_PCT rekey-vs-control loss margin (default 5). # CONTROL_STREAM_SECS quiet control-window length (default 12). # MESH_SIZE_WARMUP Phase 7 bloom warmup, from mesh start, before # any estimate counts (default 300). @@ -179,9 +179,26 @@ LOG_POLL_INTERVAL=2 # Data-plane continuity stream (control-differential). Streams run a # sustained ping6 over the overlay across the rekey window vs a quiet # control window; loss is compared to prove rekey is hitless. -STREAM_RATE_HZ=5 # ping6 -i 0.2 +# The control window cannot be lengthened much: it has to fit inside one +# rekey interval (REKEY_AFTER_SECS, 35s by default) to stand a chance of +# being cutover-free, and it already gets contaminated sometimes at 12s. +# So the sample is raised by rate instead. At 20 Hz the control window is +# 240 packets and the rekey window ~3100, where at 5 Hz they were 60 and +# ~775 and a 1% margin came to 0.6 of a control packet — below the +# quantisation of its own measurement, so a one-packet difference decided +# the verdict and the netem arm failed on same-version pairs as readily +# as on mixed ones. +STREAM_RATE_HZ=20 CONTROL_STREAM_SECS="${CONTROL_STREAM_SECS:-12}" # quiet pre-rekey window -STREAM_LOSS_MARGIN_PCT="${STREAM_LOSS_MARGIN_PCT:-1}" +# 5% is 12 packets of the 240-packet control window and ~155 of the +# ~3100-packet rekey window. On a path losing up to 6% (the range the +# v0.4.2 netem runs baselined at, under `loss 2%` over several hops) the +# difference of the two windows has a standard deviation below 1.6%, so +# 5% is past three sigma and sampling spread can no longer produce a red. +# It still catches what this check exists for: a rekey that is not +# hitless blackholes until the session reconverges, which the harness +# budgets 45s for, against the ~8s of traffic 5% of the rekey window is. +STREAM_LOSS_MARGIN_PCT="${STREAM_LOSS_MARGIN_PCT:-5}" # The rekey-window stream must span Phases 2-5 (both cutovers + reconverge). REKEY_STREAM_SECS=$(( FIRST_REKEY_TIMEOUT + SECOND_REKEY_WAIT + POST_REKEY_TIMEOUT + 15 )) @@ -551,8 +568,11 @@ _stream_one() { local from="$1" to="$2" dur="$3" outfile="$4" local from_ctr="${CONTAINER[$from]}" to_npub="${NPUB_OF[$to]}" local count=$(( dur * STREAM_RATE_HZ )) - local out tx rx - out=$(docker exec "$from_ctr" ping6 -i 0.2 -c "$count" -W "$PING_TIMEOUT" \ + local out tx rx interval + # Interval and count both derive from STREAM_RATE_HZ, so the packet + # count the margin is reasoned about cannot drift from the rate sent. + interval=$(awk -v hz="$STREAM_RATE_HZ" 'BEGIN{ printf "%.3f", 1/hz }') + out=$(docker exec "$from_ctr" ping6 -i "$interval" -c "$count" -W "$PING_TIMEOUT" \ "${to_npub}.fips" 2>&1) tx=$(echo "$out" | grep -oE '[0-9]+ packets transmitted' | grep -oE '^[0-9]+') rx=$(echo "$out" | grep -oE '[0-9]+ received' | grep -oE '^[0-9]+') From 0726b743daf5dd8bcd413bd21123d96c85e1daef Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 25 Aug 2026 20:24:45 +0100 Subject: [PATCH 05/15] Release the path MTU seeding record when the path it describes goes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `path_mtu_seeded_by` records which transport last link-seeded a destination's `path_mtu_lookup` entry, but nothing ever removed a row: the map grew one entry per peer this node had ever linked with and kept them for the life of the process. Give it a removal site in `path_mtu_lookup_release`, which is where the `path_mtu_lookup` entry it describes is already dropped. Releasing the two together is what makes the removal safe: clearing the seeding record on its own would leave the stored value behind with nothing recording which link it came from, and a peer returning over a wider transport would then be refused by the never-loosen rule for the process lifetime — the exact failure the record exists to prevent. When the peer's link is still up, the reseed that already follows the release writes both back. That alone does not bound the map, because every existing caller of `path_mtu_lookup_release` fires on session state and a peer can link without ever opening an FSP session. So call it from `remove_active_peer` as well, the single choke-point every peer teardown routes through and the symmetric partner of the promotion that writes the seed. The peer is out of `self.peers` by then, so no reseed follows and a departed peer leaves nothing behind. Peer restart during promotion already runs teardown before the fresh promotion seeds, so the ordering there is unchanged. The seeding record is taken after the `path_mtu_lookup` guard is dropped, keeping `seed_path_mtu_for_link_peer` the only site that holds both locks. (cherry picked from commit 8c8d15ec538458d9ccbd7a641fe9ec601233691d) --- src/node/dataplane/dispatch.rs | 10 +++ src/node/handlers/timeout.rs | 10 +-- src/node/mod.rs | 28 ++++++++- src/node/tests/unit.rs | 107 +++++++++++++++++++++++++++++++++ 4 files changed, 148 insertions(+), 7 deletions(-) diff --git a/src/node/dataplane/dispatch.rs b/src/node/dataplane/dispatch.rs index 8238fa52..be389ca6 100644 --- a/src/node/dataplane/dispatch.rs +++ b/src/node/dataplane/dispatch.rs @@ -149,6 +149,16 @@ impl Node { } self.pending_tun_packets.remove(node_addr); + // Release the path MTU state this peer's promotion seeded: the link it + // measured has just gone. The other callers all fire on session state, + // so without this a peer that never opened an FSP session leaves both + // its `path_mtu_lookup` entry and its seeding-transport record behind + // for the life of the process. The peer is already out of `self.peers` + // above, so no reseed follows and the two go together, which is what + // keeps a peer returning over a wider link from being clamped to the + // one it left. + self.path_mtu_lookup_release(node_addr); + let link_id = peer.link_id(); let transport_id = peer.transport_id(); diff --git a/src/node/handlers/timeout.rs b/src/node/handlers/timeout.rs index c8d51420..7f62f9ac 100644 --- a/src/node/handlers/timeout.rs +++ b/src/node/handlers/timeout.rs @@ -518,12 +518,12 @@ impl Node { /// Expire `path_mtu_lookup` entries that nothing else will ever release. /// - /// The three callers of `path_mtu_lookup_release` all fire on session + /// The callers of `path_mtu_lookup_release` all fire on session or peer /// state, so an entry written by the discovery `LookupResponse` carrier - /// for a destination this node never opens a session with has no release - /// path at all. Keep-tighter then makes one such response permanent: a - /// `path_mtu` of 256 pins that destination's SYN-time MSS clamp at 119 - /// bytes until the process restarts. Only those entries carry a + /// for a destination this node neither peers with nor opens a session + /// with has no release path at all. Keep-tighter then makes one such + /// response permanent: a `path_mtu` of 256 pins that destination's + /// SYN-time MSS clamp at 119 bytes until the process restarts. Only those entries carry a /// `learned_ms`, and only they are expired here. /// /// The deadline is the coordinate cache's own TTL, because the same diff --git a/src/node/mod.rs b/src/node/mod.rs index 3f5639a5..808cfe18 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -385,6 +385,11 @@ pub struct Node { /// value that still describes the current path from one that describes a /// path the peer has left. Absent for destinations reached over multiple /// hops: those are never link-seeded, so never-loosen applies unchanged. + /// + /// An entry lives exactly as long as the `path_mtu_lookup` entry it + /// describes: `path_mtu_lookup_release` drops both together, and peer + /// removal is one of its callers, so a peer that never comes back leaves + /// nothing behind. path_mtu_seeded_by: Arc>>, // === Transports & Links === @@ -2896,8 +2901,9 @@ impl Node { /// removal would silently drop a direct peer back to the conservative /// ceiling until its link re-handshakes. /// - /// Two stores describe the same dead path, so this releases both: the - /// `FipsAddress`-keyed map the TCP MSS clamp reads, and the session's own + /// Three stores describe the same dead path, so this releases all of + /// them: the `FipsAddress`-keyed map the TCP MSS clamp reads, the record + /// of which transport last link-seeded that map, and the session's own /// source-side path MTU estimate. fn path_mtu_lookup_release(&mut self, addr: &NodeAddr) { // The evidence that corroborates a reactive MtuExceeded described the @@ -2941,6 +2947,24 @@ impl Node { return; } } + // The seeding record describes the path just released, so it goes with + // it. Taken after the guard above is dropped, so + // `seed_path_mtu_for_link_peer` remains the only site holding both + // locks at once, and before the reseed below, so a peer whose link is + // still up writes its transport straight back in. + match self.path_mtu_seeded_by.write() { + Ok(mut seeded_by) => { + seeded_by.remove(&fips_addr); + } + Err(e) => { + tracing::warn!( + fips_addr = %fips_addr, + error = %e, + "path_mtu_seeded_by write lock poisoned; seeding record not released" + ); + } + } + // The write guard above must be dropped before the seed runs: it takes // the same lock, and `std::sync::RwLock` is not re-entrant. if let Some(peer) = self.peers.get(addr) diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index 2c785b12..9c86a553 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -2141,6 +2141,113 @@ async fn test_seed_path_mtu_keeps_tighter_value_when_reseeding_same_transport() } } +/// The seeding record is bounded by the same lifecycle that writes it. +/// +/// Promotion seeds; release drops. Without the release the map keeps a row +/// per peer this node has ever linked with, for the life of the process, and +/// the two stores drift apart: `path_mtu_lookup` forgets the value while the +/// record still names the transport that supplied it. +#[tokio::test] +async fn test_releasing_a_path_drops_the_seeding_transport_record_with_the_value() { + let mut node = make_node(); + let (packet_tx, packet_rx) = packet_channel(64); + node.supervisor.packet_tx = Some(packet_tx); + node.packet_rx = Some(packet_rx); + + let udp = make_udp_transport_with_mtu(1, 1452).await; + node.transports.insert(TransportId::new(1), udp); + + let peer_addr = make_node_addr(0xE4); + let fips_addr = crate::FipsAddress::from_node_addr(&peer_addr); + let transport_addr = TransportAddr::from_string("10.0.0.11:2121"); + + node.seed_path_mtu_for_link_peer(&peer_addr, TransportId::new(1), &transport_addr); + assert_eq!( + node.path_mtu_seeded_by + .read() + .unwrap() + .get(&fips_addr) + .copied(), + Some(TransportId::new(1)), + "the seed records the transport it came from" + ); + + // No entry in `node.peers`, so nothing reseeds behind the release — the + // departed-peer case. + node.path_mtu_lookup_release(&peer_addr); + + assert!( + node.path_mtu_lookup + .read() + .unwrap() + .get(&fips_addr) + .is_none(), + "release drops the stored value" + ); + assert!( + node.path_mtu_seeded_by + .read() + .unwrap() + .get(&fips_addr) + .is_none(), + "release must drop the seeding record with it, or the map grows for \ + the life of the process" + ); + + for transport in node.transports.values_mut() { + transport.stop().await.ok(); + } +} + +/// The live-link case: release is immediately followed by a reseed, so both +/// stores come back rather than leaving a linked peer on the fallback ceiling. +#[tokio::test] +async fn test_releasing_a_path_for_a_still_linked_peer_reseeds_both_stores() { + let mut node = make_node(); + let (packet_tx, packet_rx) = packet_channel(64); + node.supervisor.packet_tx = Some(packet_tx); + node.packet_rx = Some(packet_rx); + + let udp = make_udp_transport_with_mtu(1, 1452).await; + node.transports.insert(TransportId::new(1), udp); + + let identity = make_peer_identity(); + let peer_addr = *identity.node_addr(); + let fips_addr = crate::FipsAddress::from_node_addr(&peer_addr); + let transport_addr = TransportAddr::from_string("10.0.0.12:2121"); + + let mut peer = crate::peer::ActivePeer::new(identity, LinkId::new(1), 0); + peer.set_current_addr(TransportId::new(1), transport_addr.clone()); + node.peers.insert(peer_addr, peer); + + node.seed_path_mtu_for_link_peer(&peer_addr, TransportId::new(1), &transport_addr); + node.path_mtu_lookup_release(&peer_addr); + + assert_eq!( + node.path_mtu_lookup + .read() + .unwrap() + .get(&fips_addr) + .map(|e| e.mtu), + Some(1452), + "a peer whose link is still up is reseeded from that link" + ); + assert_eq!( + node.path_mtu_seeded_by + .read() + .unwrap() + .get(&fips_addr) + .copied(), + Some(TransportId::new(1)), + "and the seeding record comes back with it, so a later move is still \ + detectable" + ); + + for transport in node.transports.values_mut() { + transport.stop().await.ok(); + } +} + // === Outbound admission gate tests === /// Inject `count` synthetic active peers into `node.peers` so peer_count() From 021928beafce05439928a4d347d01548edbc8e4b Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 25 Aug 2026 20:22:26 +0100 Subject: [PATCH 06/15] Clear every reverse-lookup key a link was inserted under when it is removed `remove_link` rebuilt a single `addr_to_link` key from the link's own remote address, so any entry inserted under a different address form survived the link it named. The cross-connection arms in the peer-action executor key the surviving link on the packet's source address, which need not equal that link's own address: a hostname against its resolved numeric form, or a different source port on a connection-oriented transport. The entry then outlived the link for as long as the node ran, and `msg1_waiver` reads it when deciding handshake admission. Remove by value instead, dropping every entry that still maps to the link being removed. This keeps the old guard that an entry a newer link has claimed is left alone (its value no longer names this link), and it stays symmetric with any future insert without a second removal call to drift out of step. The map holds one entry per registered address, so the scan is bounded by the link count on a path that is not hot. Add a unit test covering the second-address-form case and the newer-link-claimed case. (cherry picked from commit 642523c6bbf612762afa2d9bf5c2a0d36b6dc77c) --- src/node/mod.rs | 27 +++++++++++++------------ src/node/tests/unit.rs | 46 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 13 deletions(-) diff --git a/src/node/mod.rs b/src/node/mod.rs index 808cfe18..43641fa1 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -2532,20 +2532,21 @@ impl Node { /// Remove a link. /// - /// Only removes the addr_to_link reverse lookup if it still points to this - /// link. In cross-connection scenarios, a newer link may have replaced the - /// entry for the same address. + /// Drops every `addr_to_link` entry that still maps to this link, rather + /// than only the key rebuilt from the link's own remote address. A link can + /// be registered under more than one address form: the cross-connection + /// arms key the winner on the *packet's* source address, which need not + /// equal the winner's own (a hostname against its resolved numeric form, or + /// a different source port on a connection-oriented transport). Rebuilding + /// a single key left those entries naming a link that no longer exists, for + /// as long as the node ran. + /// + /// Entries a newer link has already claimed are left alone, since they no + /// longer name this link. pub fn remove_link(&mut self, link_id: &LinkId) -> Option { - if let Some(link) = self.links.remove(link_id) { - // Clean up reverse lookup only if it still maps to this link - let key = (link.transport_id(), link.remote_addr().clone()); - if self.addr_to_link.get(&key) == Some(link_id) { - self.addr_to_link.remove(&key); - } - Some(link) - } else { - None - } + let link = self.links.remove(link_id)?; + self.addr_to_link.retain(|_, mapped| *mapped != *link_id); + Some(link) } /// Single choke-point for dropping a per-peer control machine. Also drops the diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index 9c86a553..044d014a 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -302,6 +302,52 @@ fn test_node_link_management() { ); } +#[test] +fn remove_link_clears_a_reverse_lookup_entry_keyed_on_a_second_address_form() { + let mut node = make_node(); + let transport_id = TransportId::new(1); + + let link_id = node.allocate_link_id(); + node.add_link(Link::connectionless( + link_id, + transport_id, + TransportAddr::from_string("10.128.2.4:2121"), + LinkDirection::Inbound, + Duration::from_millis(50), + )) + .unwrap(); + + // The cross-connection arms key the surviving link on the *packet's* + // source address, which need not be the form the link itself carries. + node.addr_to_link.insert( + (transport_id, TransportAddr::from_string("node-b:2121")), + link_id, + ); + + // An entry another link has claimed is not this link's to remove. + let other_link_id = node.allocate_link_id(); + node.addr_to_link.insert( + (transport_id, TransportAddr::from_string("10.128.2.5:2121")), + other_link_id, + ); + + node.remove_link(&link_id); + + assert!( + node.find_link_by_addr(transport_id, &TransportAddr::from_string("10.128.2.4:2121")) + .is_none() + ); + assert!( + node.find_link_by_addr(transport_id, &TransportAddr::from_string("node-b:2121")) + .is_none(), + "the second address form outlived the link it named" + ); + assert_eq!( + node.find_link_by_addr(transport_id, &TransportAddr::from_string("10.128.2.5:2121")), + Some(other_link_id) + ); +} + #[test] fn test_node_link_limit() { let mut node = make_node_with_max_links(2); From a3ea22454f35a8e9f963381676f2af1eb8810fe8 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 25 Aug 2026 20:28:21 +0100 Subject: [PATCH 07/15] Free the abandoned rekey index and reschedule an ACL-rejected dial Two handshake reject arms on master dropped state that the next line already handles correctly, so bring both to the same shape. The tick-loop `AbandonRekey` arm called `ActivePeer::abandon_rekey` in statement position and threw away the `Option` the callee hands back for freeing. The index was leaked from the allocator and its `pending_outbound` entry, seeded when rekey msg1 went out, was left pointing at the peer's live link. Bind the return, clear both registry entries keyed by it, and free it, matching what the dual-initiation loser and the rekey-msg2 failure arm in `handshake.rs` already do. The outbound ACL reject in `handle_msg2` disposed of the machine, the link, the `pending_outbound` entry and the index, but re-armed nothing. That disposal takes the leg out of the stuck-leg sweep, which is the only thing that otherwise reaches `note_handshake_timeout`, and `close_connection` only drops the transport's pool entry. A configured peer denied once was therefore never dialed again, even after the ACL was relaxed. Reschedule from the arm itself. Both arms get a discriminating test that asserts the state is present before the call and gone after, with a control assertion so neither can pass vacuously and a limb checking the live session and the disposed leg are unaffected. The ACL test needed the responder listed as a configured peer, so the existing denied-msg2 setup is extracted into a helper that takes a config builder rather than being duplicated. (cherry picked from commit 62506f1c0bac50ca0d502eb44b3ee7fbbafa6c17) --- src/node/handlers/handshake.rs | 13 ++++ src/node/handlers/rekey.rs | 34 ++++++++- src/node/tests/acl.rs | 69 ++++++++++++++++- src/node/tests/establish_chartests.rs | 104 ++++++++++++++++++++++++++ 4 files changed, 215 insertions(+), 5 deletions(-) diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index 24833634..437c8256 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -1317,6 +1317,19 @@ impl Node { if let Some(idx) = our_index { let _ = self.index_allocator.free(idx); } + // Put the dial back on the retry schedule. The disposal above takes + // this leg out of the stuck-leg sweep — `has_pending_leg` reads false + // for it from here on, so the reap that normally reaches + // `note_handshake_timeout` never runs — and that reflex is the only + // thing that seeds `retry_pending` for a configured peer. + // `close_connection` only drops the transport's pool entry; it + // schedules nothing. + // + // `peer_identity` is safe to reschedule against here because this is + // an IK dial: the initiator's expected identity is fixed at + // `start_handshake` and `complete_handshake` never overwrites it, so + // it is still the peer we meant to dial and not whoever answered. + self.note_handshake_timeout(*peer_identity.node_addr(), packet.timestamp_ms); self.stats_mut() .record_reject(RejectReason::Handshake(HandshakeReject::BadState)); return; diff --git a/src/node/handlers/rekey.rs b/src/node/handlers/rekey.rs index a9371e2f..0c938d54 100644 --- a/src/node/handlers/rekey.rs +++ b/src/node/handlers/rekey.rs @@ -420,8 +420,38 @@ impl Node { match action { // Abandon rekey cycles that exhausted their retransmission budget. ConnAction::AbandonRekey { peer: node_addr } => { - if let Some(peer) = self.peers.get_mut(&node_addr) { - peer.abandon_rekey(); + // `abandon_rekey` hands back whichever index the abandoned + // cycle owned; dropping the return value orphans it, and the + // `pending_outbound` entry seeded at rekey msg1 with it. Same + // shape as the dual-initiation loser and the rekey-msg2 + // failure arm in `handshake.rs`. + // + // The `peers_by_index` removal is parity with those two arms + // and is a no-op at THIS call site: `AbandonRekey` is emitted + // only from the msg1-resend-budget classification, so no rekey + // msg2 ever arrived and nothing was inserted. + // + // Known exposure, kept for parity rather than closed here: + // `transport_id()` is RE-READ, while the `pending_outbound` + // entry was keyed by whatever it was when rekey msg1 went out. + // A roam in between (`set_current_addr` overwrites + // `send.transport_id`) makes the removal miss, so the index is + // freed with a stale entry still pointing at the peer's live + // link. Walked to its end: a later msg2 naming that index on + // the old transport resolves the stale link, finds the + // promoted peer's machine leg-less, finds no peer with a + // matching `rekey_our_index` (this arm cleared it), and takes + // the "not a rekey" arm, which removes the stale entry and + // records a reject. No teardown, no wrong-peer effect. + // Removing by index VALUE would close it outright. + if let Some(peer) = self.peers.get_mut(&node_addr) + && let Some(idx) = peer.abandon_rekey() + { + if let Some(tid) = peer.transport_id() { + self.peers_by_index.remove(&(tid, idx.as_u32())); + self.pending_outbound.remove(&(tid, idx.as_u32())); + } + let _ = self.index_allocator.free(idx); } debug!( peer = %self.peer_display_name(&node_addr), diff --git a/src/node/tests/acl.rs b/src/node/tests/acl.rs index 5a872605..7cbee4d6 100644 --- a/src/node/tests/acl.rs +++ b/src/node/tests/acl.rs @@ -75,13 +75,25 @@ async fn test_inbound_msg1_denied_by_acl() { assert_eq!(node_b.link_count(), 0); } -#[tokio::test] -async fn test_outbound_msg2_denied_after_acl_reload() { - let (dir, mut node_a) = make_acl_node(); +/// Drive a dialing node up to the instant a genuine msg2 arrives from a +/// responder its denylist has just been reloaded to reject, and hand back the +/// node, the responder's NodeAddr, and the msg2 packet ready to deliver. +/// +/// `config_a` receives the responder's npub so a caller can list it as a +/// configured peer, which is what makes the reject arm's retry reschedule +/// observable. +async fn outbound_denied_at_msg2( + config_a: impl FnOnce(&str) -> Config, +) -> (tempfile::TempDir, Node, NodeAddr, ReceivedPacket) { let node_b = make_node(); + let dir = tempfile::tempdir().unwrap(); + let mut node_a = Node::new(config_a(&node_b.npub())).unwrap(); + node_a.peer_acl = PeerAclReloader::with_paths(allow_path(&dir), deny_path(&dir)); + let transport_id = TransportId::new(1); let remote_addr = TransportAddr::from_string("127.0.0.1:5001"); let peer_b_identity = PeerIdentity::from_pubkey_full(node_b.identity().pubkey_full()); + let node_b_addr = *peer_b_identity.node_addr(); let link_id_a = node_a.allocate_link_id(); let our_index_a = node_a.index_allocator.allocate().unwrap(); @@ -134,6 +146,14 @@ async fn test_outbound_msg2_denied_after_acl_reload() { assert!(node_a.reload_peer_acl().await); let packet = ReceivedPacket::with_timestamp(transport_id, remote_addr, wire_msg2, 1100); + (dir, node_a, node_b_addr, packet) +} + +#[tokio::test] +async fn test_outbound_msg2_denied_after_acl_reload() { + let (_dir, mut node_a, _node_b_addr, packet) = + outbound_denied_at_msg2(|_npub| Config::new()).await; + node_a.handle_msg2(packet).await; assert_eq!(node_a.peer_count(), 0); @@ -142,6 +162,49 @@ async fn test_outbound_msg2_denied_after_acl_reload() { assert!(node_a.pending_outbound.is_empty()); } +/// The outbound ACL reject arm disposes of the whole leg — machine, link and +/// index — which takes it out of the stuck-leg sweep that would otherwise reach +/// `note_handshake_timeout`. For a configured peer the dial must therefore be +/// put back on the retry schedule from the arm itself, or the node never dials +/// again after the ACL is relaxed. +#[tokio::test] +async fn test_outbound_msg2_acl_reject_reschedules_a_configured_peer() { + let (_dir, mut node_a, node_b_addr, packet) = outbound_denied_at_msg2(|npub| { + let mut config = Config::new(); + config.peers.push(crate::config::PeerConfig::new( + npub, + "udp", + "127.0.0.1:5001", + )); + config + }) + .await; + + assert!( + node_a.peering.reconciler.retry_pending.is_empty(), + "control: nothing is scheduled before the reject" + ); + + node_a.handle_msg2(packet).await; + + let state = node_a + .peering + .reconciler + .retry_pending + .get(&node_b_addr) + .expect("the rejected dial is back on the retry schedule"); + assert_eq!(state.retry_count, 1, "counted as a first failure"); + assert!( + state.retry_after_ms > 1100, + "scheduled after the msg2 that triggered it" + ); + + // The reschedule must not resurrect any of the disposed leg. + assert_eq!(node_a.peer_count(), 0); + assert_eq!(node_a.link_count(), 0); + assert!(node_a.pending_outbound.is_empty()); +} + #[tokio::test] async fn test_host_map_hot_reloads_from_tick() { let dir = tempfile::tempdir().unwrap(); diff --git a/src/node/tests/establish_chartests.rs b/src/node/tests/establish_chartests.rs index 1004eba4..eec03aa1 100644 --- a/src/node/tests/establish_chartests.rs +++ b/src/node/tests/establish_chartests.rs @@ -1032,3 +1032,107 @@ async fn chartest_msg1_rekey_dual_init_we_lose_becomes_responder() { "loser (now responder) emits a rekey msg2" ); } + +// =========================================================================== +// Rekey abandon (the tick-loop budget path). Not a `handle_msg1` branch, but it +// reuses this module's rekey arming and establish helpers, so it lives here. +// =========================================================================== + +/// A rekey cycle abandoned for exhausting its msg1 retransmission budget must +/// return its session index to the allocator and clear both registry entries +/// keyed by it, leaving the live session untouched. +/// +/// `handshake_max_resends = 0` fires the abandon on the first +/// `resend_pending_rekeys` call: the classifier tests `resend_count >= +/// max_resends` before it consults the resend-due predicate, so the wall clock +/// is out of the path entirely. +#[tokio::test] +async fn abandoned_rekey_frees_its_index_and_clears_both_registries() { + let mut config = Config::new(); + config.node.rate_limit.handshake_max_resends = 0; + let mut node = make_node_with(config); + let transport_id = TransportId::new(1); + let (peer_sock, peer_addr) = register_udp_with_peer_socket(&mut node, transport_id).await; + + let sender = Identity::generate(); + let sender_addr = establish_active_peer_via_msg1( + &mut node, + &sender, + [9u8; 8], + transport_id, + &peer_addr, + &peer_sock, + 1000, + ) + .await; + + let session_index = node + .get_peer(&sender_addr) + .expect("peer established") + .our_index() + .expect("the established peer holds a session index"); + let rekey_index = arm_local_rekey(&mut node, &sender, &sender_addr, transport_id); + let allocated_before = node.index_allocator.count(); + + // Controls: without these the assertions after the abandon could pass + // against state that was never set up. + assert!( + node.index_allocator.is_allocated(rekey_index), + "control: the armed rekey holds an index" + ); + assert!( + node.peers_by_index + .contains_key(&(transport_id, rekey_index.as_u32())), + "control: the rekey index is registered for dispatch" + ); + assert!( + node.pending_outbound + .contains_key(&(transport_id, rekey_index.as_u32())), + "control: the rekey index is registered for msg2 dispatch" + ); + + // The rekey msg2 never arrives; the first poll spends the (zero) budget. + node.resend_pending_rekeys(2000).await; + + let p = node.get_peer(&sender_addr).expect("peer still present"); + assert!( + !p.rekey_in_progress(), + "control: the abandon actually fired" + ); + assert!(p.rekey_our_index().is_none()); + + assert!( + !node.index_allocator.is_allocated(rekey_index), + "the abandoned cycle must return its index" + ); + assert_eq!( + node.index_allocator.count(), + allocated_before - 1, + "and must free exactly one" + ); + assert!( + !node + .peers_by_index + .contains_key(&(transport_id, rekey_index.as_u32())), + "the abandoned cycle must clear its dispatch registration" + ); + assert!( + !node + .pending_outbound + .contains_key(&(transport_id, rekey_index.as_u32())), + "the abandoned cycle must clear its msg2 dispatch entry" + ); + + // The limb that catches a fix binding the wrong index: the live session is + // untouched. + assert_eq!(node.peer_count(), 1, "the session survives"); + assert!( + node.index_allocator.is_allocated(session_index), + "the established session's own index must not be freed" + ); + assert!( + node.peers_by_index + .contains_key(&(transport_id, session_index.as_u32())), + "the established session stays registered for dispatch" + ); +} From 4e1541f489be31a97f713ece3eb4b2b5c9334b3d Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 25 Aug 2026 20:19:56 +0100 Subject: [PATCH 08/15] Record why the session's handshake hash is not cleared on drop `SymmetricState` gained a `Zeroize` derive with the key-material work, so the handshake hash it holds is erased when the handshake state drops. The value does not stay there: `into_session` copies it into `NoiseSession`, which outlives the handshake by the life of the link and clears nothing. The copy is fine where it is. The hash is a transcript hash, no key is derived from it in this crate, and `handshake_hash()` publishes it to any caller, so treating it as secret in one type while handing it out from another would be incoherent. What was wrong was that the declaration said none of this, leaving the reader to guess whether the omission was an oversight. Say it at the field, and name the change that would make the answer different. (cherry picked from commit 88ca5f09c37fbfc7674c8385c611f9094e7d3a92) --- src/noise/session.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/noise/session.rs b/src/noise/session.rs index 92a8a059..99d17ac5 100644 --- a/src/noise/session.rs +++ b/src/noise/session.rs @@ -14,7 +14,17 @@ pub struct NoiseSession { send_cipher: CipherState, /// Cipher for receiving. recv_cipher: CipherState, - /// Handshake hash. + /// Handshake hash, copied out of the handshake's `SymmetricState`. + /// + /// Deliberately not cleared on drop, unlike the copy it came from. That + /// copy is cleared because it sits beside the chaining key in a type whose + /// contract is that it erases what it holds; this one is a transcript hash + /// that nothing derives a key from, and `handshake_hash()` hands it out to + /// any caller. A type cannot both publish a value and treat it as secret. + /// + /// Revisit if the hash ever becomes key-adjacent: feeding it to the AEAD as + /// associated data, or building channel binding or an exporter on it, would + /// make this session's copy the long-lived home of something worth erasing. handshake_hash: [u8; 32], /// Remote peer's static public key. remote_static: PublicKey, From 0a2ad33ec511ae049d971c1d526fe50ad93b8c6a Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 25 Aug 2026 20:22:29 +0100 Subject: [PATCH 09/15] Set the UDP listen socket's reuse flags after its bind, not before `UdpRawSocket::open` set SO_REUSEPORT and SO_REUSEADDR before binding, on the belief -- asserted in the comment there -- that the flags must precede the bind for the per-peer connected sockets to join the port later. That is true of the joining socket and false of the holder: a holder flagged after its own bind still admits the join. Before the bind, the flags mean something the listen socket never wanted. With outbound_only the bind is 0.0.0.0:0, and a flagged port-zero bind lets the kernel hand back a port another flagged socket already holds. On a configured port it is worse and needs no special posture: a second daemon binding the same address succeeded silently and shared the port, with the kernel splitting inbound datagrams across the two recv loops on the source 4-tuple, where it should have failed to start with EADDRINUSE. Move both calls after the bind and rewrite the comment that recorded the wrong belief. The connected-UDP fast path is unaffected: open_connected_fd is the joiner and keeps its flags before its own bind, which is where they belong. Cover the configured-port case with a test that opens a second socket on a port the first still holds and requires the bind to fail. It fails on the old ordering, where the second open succeeds. The port-zero duplicate is probabilistic and needs hundreds of sockets to show up, so it is left to the measurements in the issue rather than to a flaky test. (cherry picked from commit 7034841da5877f57c233845122bd507467249c14) --- src/node/tests/unit.rs | 4 ++-- src/transport/udp/io/mod.rs | 23 +++++++++++++++++++++++ src/transport/udp/io/unix.rs | 28 ++++++++++++++++++---------- 3 files changed, 43 insertions(+), 12 deletions(-) diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index 044d014a..f26cdbd0 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -3386,8 +3386,8 @@ async fn app_owned_udp_fd_seam_stays_silent_without_a_udp_transport() { } /// A UDP transport that never bound has no fd to hand out. The bind address is -/// deliberately unparseable — a busy port would not do it, since -/// `UdpRawSocket::open` sets `SO_REUSEADDR`/`SO_REUSEPORT` before binding. +/// deliberately unparseable, so the failure is in parsing and cannot depend on +/// what else happens to hold a port while the suite runs. #[cfg(unix)] #[tokio::test] async fn app_owned_udp_fd_seam_stays_silent_when_the_udp_transport_fails_to_start() { diff --git a/src/transport/udp/io/mod.rs b/src/transport/udp/io/mod.rs index e372afe7..8acbb630 100644 --- a/src/transport/udp/io/mod.rs +++ b/src/transport/udp/io/mod.rs @@ -76,6 +76,29 @@ mod tests { assert!(send_buf > 0, "send buffer should be non-zero"); } + /// The listen socket's reuse flags go on after its own bind, so they + /// never license the kernel to hand this socket a port someone else + /// holds. The visible consequence is that a second bind of an occupied + /// port fails loudly instead of silently sharing it and splitting the + /// inbound datagrams between the two recv loops. + #[cfg(unix)] + #[test] + fn a_second_open_of_an_occupied_port_fails_instead_of_sharing_it() { + let holder = UdpRawSocket::open("127.0.0.1:0".parse().unwrap(), 65536, 65536) + .expect("failed to bind the holding socket"); + let addr = holder.local_addr(); + + // `UdpRawSocket` is not `Debug`, so this cannot be `expect_err`. + let Err(err) = UdpRawSocket::open(addr, 65536, 65536) else { + panic!("the port is already held, so the second bind must fail"); + }; + + assert!( + err.to_string().contains("bind failed"), + "the failure must come from bind, not from a later step: {err}", + ); + } + #[tokio::test] async fn test_async_udp_socket_send_recv() { let sock1 = UdpRawSocket::open("127.0.0.1:0".parse().unwrap(), 65536, 65536) diff --git a/src/transport/udp/io/unix.rs b/src/transport/udp/io/unix.rs index 66e8e5e3..60a4cb43 100644 --- a/src/transport/udp/io/unix.rs +++ b/src/transport/udp/io/unix.rs @@ -57,19 +57,27 @@ impl UdpRawSocket { sock.set_nonblocking(true) .map_err(|e| TransportError::StartFailed(format!("set nonblocking failed: {}", e)))?; - // SO_REUSEPORT lets per-peer `ConnectedPeerSocket`s bind - // to the same wildcard port the listen socket holds. Must - // be set BEFORE bind. Without this, the connected-UDP - // activation handler fails with EADDRINUSE on Linux and - // every outbound packet falls back to the wildcard listen - // socket — losing the kernel 5-tuple cache benefit and - // most of the multihop forwarding throughput gain. - let _ = sock.set_reuse_port(true); - let _ = sock.set_reuse_address(true); - sock.bind(&bind_addr.into()) .map_err(|e| TransportError::StartFailed(format!("bind failed: {}", e)))?; + // SO_REUSEPORT lets per-peer `ConnectedPeerSocket`s bind to the same + // port this listen socket holds; without it the connected-UDP + // activation handler fails with EADDRINUSE on Linux and every + // outbound packet falls back to the wildcard listen socket, losing + // the kernel 5-tuple cache benefit and most of the multihop + // forwarding throughput gain. + // + // Set AFTER bind, deliberately. The flags mean different things + // either side of it: before bind, "the kernel may give me a port + // another socket already holds", which for the `outbound_only` + // `0.0.0.0:0` bind means duplicate ephemeral ports, and for a + // configured port means a second daemon silently shares it instead + // of failing to start; after bind, "another socket may later join my + // port", which is the only half the fast path needs. A joiner still + // binds successfully against a holder flagged after its own bind. + let _ = sock.set_reuse_port(true); + let _ = sock.set_reuse_address(true); + // Set socket buffer sizes sock.set_recv_buffer_size(recv_buf_size) .map_err(|e| TransportError::StartFailed(format!("set recv buffer: {}", e)))?; From 9e564a8ddc02aec5fbb908dfdbb1adc94d64b67d Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 25 Aug 2026 20:20:28 +0100 Subject: [PATCH 10/15] Spell out the contract on the app-owned UDP descriptor v0.5.0 is the release that makes the app-owned socket seam public, and raw_fd() has been handing embedders a RawFd whose documented contract stopped at "this is the transport's socket, and it is open right now". That leaves the two questions an embedder actually has unanswered: what it may do with the descriptor, and how long the answer holds. Say both. The permitted uses are the ones the seam exists for (reading it, and setting host-network options such as SO_BINDTODEVICE or SO_MARK); the forbidden ones are those that disturb a descriptor the transport has registered with the tokio reactor, closing it above all, since the number is then free for reuse and every later use addresses something else. Name what invalidates it, and state plainly that nothing notifies the holder when that happens, so a descriptor must be re-fetched after each start rather than cached across a stop. Documentation only; no behaviour change. (cherry picked from commit 8e23e3fc9ccc94e9e2be19baf22cacc847a1441b) --- src/transport/udp/mod.rs | 38 ++++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/src/transport/udp/mod.rs b/src/transport/udp/mod.rs index 9580ed47..0fc65feb 100644 --- a/src/transport/udp/mod.rs +++ b/src/transport/udp/mod.rs @@ -100,11 +100,41 @@ impl UdpTransport { /// Raw file descriptor of the bound listen socket, or `None` before /// `start_async` has bound one. /// + /// The socket stays owned by the transport, so the descriptor is a borrow + /// and not a handover. It is the wildcard listen socket only: the per-peer + /// `connect()`-ed sockets the fast path opens on Linux and macOS are not + /// reachable through here. + /// + /// What a holder may do with it: read it (`getsockname`, `getsockopt`), + /// query it (`SIOCGIFINDEX` and friends), and set the host-network options + /// the seam exists for — binding it to a device (`SO_BINDTODEVICE`), + /// marking it (`SO_MARK`), or attaching it to a routing table. `dup()` is + /// fine as long as the duplicate is closed by whoever made it. + /// + /// What a holder may not do: `close()` it, adopt it into an owning wrapper + /// (`OwnedFd::from_raw_fd`, `UdpSocket::from_raw_fd`) whose `Drop` will + /// close it, `shutdown()` it, re-`bind()` or `connect()` it, or clear + /// `O_NONBLOCK` on it. The transport registered this descriptor with the + /// tokio reactor through `AsyncFd`, and reads packets from it on its own + /// task; any of those actions either stalls the receive loop or, in the + /// case of a close, frees a number the kernel is free to hand to the next + /// socket or file this process opens, at which point every later use of it + /// silently addresses something else. + /// + /// The descriptor is invalidated by anything that drops the transport's + /// socket: `stop_async`, the node stop that calls it, a transport that + /// fails and is torn down, or dropping the transport itself. **Nothing + /// notifies the holder when that happens.** A holder that outlives a stop + /// has to treat the value it kept as stale on its own account — after a + /// restart the transport binds a fresh socket, and the descriptor names a + /// different socket even when the kernel hands back the same number. + /// Fetch it again after each `start_async` rather than caching it + /// across one; embedders that receive it through + /// [`Node::enable_app_owned_udp_fd`](crate::Node::enable_app_owned_udp_fd) + /// get exactly that, one message per successful bind. + /// /// Unix-only: `RawFd` is a unix concept and the Windows backend is built - /// on `tokio::net::UdpSocket` with no descriptor to hand out. The socket - /// stays owned by the transport, so the descriptor is a borrow with no - /// lifetime promise beyond "this is the transport's socket, and it is - /// open right now". + /// on `tokio::net::UdpSocket` with no descriptor to hand out. #[cfg(unix)] pub fn raw_fd(&self) -> Option { use std::os::unix::io::AsRawFd; From be8159ce496e3a3a63ce4253619f2053314f4da6 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 25 Aug 2026 20:26:59 +0100 Subject: [PATCH 11/15] Catch a msg1 pending slot released at acquire time The pending-slot guard's over-release direction is covered: a reject arm that frees a slot it never took reds msg1_reject_arms_do_not_release_another_handshakes_slot. The opposite direction is covered by nothing. Rebinding handle_msg1's `let _slot` to a bare `_` drops the guard the instant the slot is taken, so the limiter's concurrency limb stops bounding anything, and the whole suite stays green: `#[must_use]` does not fire, because the value is used, and every counter a test can read after the handler returns is the same in both worlds, since the slot comes back at the end either way. Until now the invariant rested on the binding being spelled `_slot`. The difference is only visible while the handler is on the stack, so that is where the observation goes: a test-build assertion immediately below the acquire, checking the count it just raised is still raised. Add a test that drives two msg1 through the handler on different paths past the acquire, each asserting the reject counter it must bump so a msg1 refused before the acquire cannot pass this vacuously. No behaviour change: the assertion is `#[cfg(test)]` and the guard itself is untouched. (cherry picked from commit 0693b81cf9fb1866a371b62a30e36497f8b484fb) --- src/node/handlers/handshake.rs | 17 +++++++- src/node/tests/unit.rs | 71 ++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 2 deletions(-) diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index 437c8256..5139e536 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -339,8 +339,8 @@ impl Node { // function returns, and that is what releases the pending slot on // every exit path. Renaming it to a bare `_` drops the guard right // here instead, releasing the slot at acquire time — silently, with - // no test and no clippy lint catching the difference. Do not "tidy" - // this binding. + // no clippy lint catching the difference. Do not "tidy" this + // binding; the assertion below is what reds if it is tidied. let _slot = match self.msg1_rate_limiter.start_handshake(class) { Ok(slot) => slot, Err(reason) => { @@ -354,6 +354,19 @@ impl Node { } }; + // Test-build witness for the paragraph above, and the only thing that + // observes it. A guard released at acquire time leaves this msg1 + // in flight with its slot already back in the pool, which no counter, + // log line or lint reports: by the time any test can look, the count + // has returned to its baseline either way. Sampling it here, on the + // handler's own stack, is what tells the two apart — see + // `msg1_handler_holds_its_pending_slot_while_the_handler_runs`. + #[cfg(test)] + assert!( + self.msg1_rate_limiter.pending_count() > 0, + "the msg1 pending slot was released before the handler ran" + ); + // accept_connections gate. Rekey/restart msg1 on an existing link // is always admitted; the gate only filters truly-fresh connections // from strangers. Without this carve-out, the dual-init tie-breaker diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index f26cdbd0..a6157729 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -2994,6 +2994,77 @@ async fn msg1_reject_arms_do_not_release_another_handshakes_slot() { assert_eq!(node.msg1_rate_limiter.pending_count(), 0); } +/// The msg1 handler keeps its pending slot for as long as it is running. +/// +/// The complement of `msg1_reject_arms_do_not_release_another_handshakes_slot`: +/// that one covers releasing a slot the handler never took, this one covers +/// releasing its own slot too early. Rebinding `handle_msg1`'s `let _slot` to +/// a bare `_` drops the guard at acquire time, so the limiter's concurrency +/// limb stops bounding anything — and every counter this test could read +/// afterwards is identical either way, because the slot comes back at the end +/// of the handler in both worlds. The difference exists only while the handler +/// is on the stack, which is why the observation lives there: the +/// `#[cfg(test)]` assertion in `handle_msg1` immediately below the acquire +/// fires under the premature release and under nothing else. +/// +/// Two packets, so the handler is entered twice on different paths past the +/// acquire, and each arm asserts the reject counter it must bump. Without +/// that, a msg1 refused before the acquire (an empty bucket, say) would leave +/// this test passing while sampling nothing. +#[tokio::test] +async fn msg1_handler_holds_its_pending_slot_while_the_handler_runs() { + use crate::noise::HANDSHAKE_MSG1_SIZE; + use crate::proto::fmp::wire::build_msg1; + use crate::utils::index::SessionIndex; + + // No transport is registered: both arms reject before any send, and the + // absent transport admits past the `accept_connections` gate. + let mut node = make_node(); + let transport_id = TransportId::new(1); + let source = TransportAddr::from_string("198.51.100.9:4141"); + let packet = |data: Vec| ReceivedPacket { + transport_id, + remote_addr: source.clone(), + data, + timestamp_ms: 1000, + }; + + assert_eq!( + node.msg1_rate_limiter.pending_count(), + 0, + "baseline: no handshake in flight" + ); + + // Arm 1: rejected at the header parse, the shortest path past the acquire. + let before = node.stats().handshake.bad_state; + node.handle_msg1(packet(vec![0u8; 8])).await; + assert_eq!( + node.stats().handshake.bad_state, + before + 1, + "arm 1 must reach the invalid-header reject, not a rate-limit refusal" + ); + + // Arm 2: well-formed header, unusable Noise payload — rejected further in, + // after the duplicate short-circuit and the DH attempt. + let before = node.stats().handshake.bad_state; + node.handle_msg1(packet(build_msg1( + SessionIndex::new(0x4242), + &[0u8; HANDSHAKE_MSG1_SIZE], + ))) + .await; + assert_eq!( + node.stats().handshake.bad_state, + before + 1, + "arm 2 must reach the receive_handshake_init reject" + ); + + assert_eq!( + node.msg1_rate_limiter.pending_count(), + 0, + "each handler released its own slot exactly once on the way out" + ); +} + /// The established-link bucket is wired from config at construction: /// derived from `max_peers` by default, overridden when the operator sets /// the key. This is the only test covering the config → limiter path. From 242e619169ed4b72883a0e48b419fe81207104fc Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 25 Aug 2026 20:31:06 +0100 Subject: [PATCH 12/15] Stop two CI gates reporting someone else's failure as ours MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two unrelated harness defects with the same shape: the run's verdict names something other than what actually went wrong. The nostr publish/consume suite treats a dead relay as a product failure. strfry is a third-party container and it has segfaulted mid-run, twelve milliseconds after both nodes connected; everything below that was a correct report of a dead relay, and the run failed on the peer-count wait with the only evidence of the real cause sitting in one container-log line ninety lines above the summary. Add a relay verdict that states the relay's own condition — gone, not running, restarted, or faulted per its log — and print it at the head of every diagnostics dump, which is what every failure path already goes through. A relay whose state or log cannot be read is reported as unestablished rather than as healthy. The verdict does not decide the run: a relay that faulted while the assertions still passed is noted and left passing, since the suite proved what it set out to prove. The setup-bucket refill test raced its own precondition. It delivered exactly three forged setups against a 50/s refill and asserted one had been refused, which holds only if all three finish inside one 20 ms window; on a loaded runner the bucket refilled mid-loop, the third setup was admitted, and the precondition failed on arrangement rather than on behaviour. Under the CI retry policy that reports as green with a flaky count, so nothing surfaced it. Drain against a 2/s refill, which a delivery would have to take 500 ms to outrun, and deliver until a refusal is actually observed rather than assuming three is enough, with a cap that says what a runner slow enough to reach it means. The refill half then waits out a full burst from empty. (cherry picked from commit ec0fadfd136e0f6a64551fab61d042e486861842) --- src/node/tests/session.rs | 37 +++++++++---- testing/check-log-strings.py | 8 +++ testing/nat/scripts/nostr-relay-test.sh | 69 +++++++++++++++++++++++++ 3 files changed, 105 insertions(+), 9 deletions(-) diff --git a/src/node/tests/session.rs b/src/node/tests/session.rs index 56e02382..4e0255e6 100644 --- a/src/node/tests/session.rs +++ b/src/node/tests/session.rs @@ -4276,19 +4276,38 @@ async fn test_forged_setups_from_one_link_peer_stop_creating_session_entries_onc #[tokio::test] async fn test_a_drained_setup_bucket_refills_and_admits_the_next_legitimate_setup() { - // Fast refill: the point is that the denial is transient, and that the - // initiator's own msg1 resend schedule covers a window this short. - let mut nodes = make_setup_limited_pair(2, 50.0).await; + // The refill has to be slow enough that the draining loop below cannot be + // outrun by the refill it is draining against. At the 50/s this test used + // to run at, a token returned every 20 ms, so on a loaded runner the loop + // outlived its own window, the third setup was admitted, and the + // precondition failed on arrangement rather than on behaviour. At 2/s a + // delivery would have to take 500 ms to lose that race. + let mut nodes = make_setup_limited_pair(2, 2.0).await; - for _ in 0..3 { + // Deliver until one is actually refused, rather than assuming three is + // enough: a delivery the refill absorbs costs one more iteration and + // nothing else. The cap is what a runner slow enough to lose even this + // race trips, and it says so rather than reporting a drained bucket that + // was never drained. + let before = nodes[1].node.stats().session.setup_rate_limited; + let mut delivered = 0; + while nodes[1].node.stats().session.setup_rate_limited == before { + assert!( + delivered < 50, + "the bucket must actually be drained before the refill is tested; \ + 50 forged setups drew no refusal, so each delivery is outlasting \ + the 500 ms refill interval" + ); deliver_forged_setup_over_link(&mut nodes).await; + delivered += 1; } - assert!( - nodes[1].node.stats().session.setup_rate_limited > 0, - "the bucket must actually be drained before the refill is tested" - ); - tokio::time::sleep(Duration::from_millis(100)).await; + // A full burst back from empty at 2/s, so the legitimate setup below meets + // the same bucket however many tokens the drain left behind. The point + // being made is that the denial is transient and clears on its own; the + // length of the window is a function of the configured rate, not of the + // claim. + tokio::time::sleep(Duration::from_millis(1200)).await; establish_pair_session(&mut nodes).await; cleanup_nodes(&mut nodes).await; diff --git a/testing/check-log-strings.py b/testing/check-log-strings.py index 7a597f9f..d230eea5 100755 --- a/testing/check-log-strings.py +++ b/testing/check-log-strings.py @@ -37,6 +37,14 @@ ALLOWED = { "ERROR": "tracing's own level token, produced by the subscriber's formatter", " ERROR ": "tracing's own level token, produced by the subscriber's formatter", " WARN ": "tracing's own level token, produced by the subscriber's formatter", + "caught a signal": ( + "read from the strfry relay container's log, not the fips daemon's — " + "strfry's own crash handler writes it" + ), + "terminate called": ( + "read from the strfry relay container's log, not the fips daemon's — " + "the C++ runtime writes it on an uncaught exception" + ), "Bootstrapped 100%": ( "read from the tor-daemon container's log, not the fips daemon's — " "Tor's own bootstrap progress line" diff --git a/testing/nat/scripts/nostr-relay-test.sh b/testing/nat/scripts/nostr-relay-test.sh index 2c321011..7f2e1ce2 100755 --- a/testing/nat/scripts/nostr-relay-test.sh +++ b/testing/nat/scripts/nostr-relay-test.sh @@ -85,7 +85,69 @@ require_test_image() { "$BUILD_SCRIPT" } +# State the relay's own condition, in its own words, before anything else. +# +# The relay is a third-party container and it has died mid-run before: strfry +# took a SIGSEGV twelve milliseconds after both nodes had connected, and every +# symptom under that was a correct report of a dead relay — subscriptions +# dropped, no advert consumed, empty peer lists, and a run that failed on the +# peer-count wait. That verdict is indistinguishable from a product failure +# unless the relay's state is stated, and the only evidence of the real cause +# was one container-log line ninety lines above the summary. This does not +# decide the run; it says whose failure it was, in a line a reader of the +# output can find. +# +# A relay whose state or log cannot be read has not been shown to be healthy, +# so that case is reported as unestablished rather than as "not the relay". +relay_verdict() { + local state="" status="" exit_code="" restarts="" logs="" + + state="$(docker inspect \ + -f '{{.State.Status}} {{.State.ExitCode}} {{.RestartCount}}' \ + "$RELAY_CONTAINER" 2>/dev/null)" || state="" + + if [ -z "$state" ]; then + echo "RELAY FAILURE: $RELAY_CONTAINER is gone; whatever this run" \ + "asserted about peering happened without a relay" >&2 + return 0 + fi + + read -r status exit_code restarts <<<"$state" + + if [ "$status" != "running" ]; then + echo "RELAY FAILURE: $RELAY_CONTAINER is $status (exit $exit_code);" \ + "the assertions in this run are downstream of that, not of the" \ + "nodes" >&2 + return 0 + fi + + if [ "${restarts:-0}" -gt 0 ]; then + echo "RELAY FAILURE: $RELAY_CONTAINER has restarted $restarts time(s)" \ + "during this run; the nodes lost their subscriptions with it" >&2 + return 0 + fi + + if ! logs="$(docker logs "$RELAY_CONTAINER" 2>&1)"; then + echo "RELAY HEALTH NOT ESTABLISHED: could not read" \ + "$RELAY_CONTAINER's logs, so a crash cannot be ruled in or out" >&2 + return 0 + fi + + if grep -Eq "caught a signal|SIGSEGV|SIGABRT|terminate called" <<<"$logs"; then + echo "RELAY FAILURE: $RELAY_CONTAINER faulted during this run:" >&2 + grep -E "caught a signal|SIGSEGV|SIGABRT|terminate called" <<<"$logs" >&2 + return 0 + fi + + echo "relay: $RELAY_CONTAINER running, no fault in its log — this run's" \ + "verdict is about the nodes" + return 1 +} + dump_diagnostics() { + echo "" + echo "=== relay verdict ===" + relay_verdict || true echo "" echo "=== nostr publish/consume diagnostics ===" for c in "$NODE_A" "$NODE_B" "$RELAY_CONTAINER"; do @@ -397,6 +459,13 @@ run_test() { return 1 fi + # A relay that faulted while the assertions still passed is a finding + # about the relay, not about this run, so it is reported and not made a + # failure: the suite proved what it set out to prove. + if relay_verdict; then + echo "NOTE: the assertions above passed despite that." >&2 + fi + cleanup echo "nostr-relay-test passed" } From 9ccad388c8838cbca197070b37bf33917c63e9fe Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 25 Aug 2026 20:26:23 +0100 Subject: [PATCH 13/15] Add fipsctl address, an offline mesh-address query Nothing printed a node's fd00::/8 mesh address without a running daemon: keygen prints the nsec and npub only, and every show path goes through the control socket. That blocks fips-initramfs, whose postinst has to write the address of the identity it just generated into DROPBEAR_OPTIONS at image build time, when no daemon is running and none can be. fipsctl address prints the address and nothing else, so it can be captured in a shell substitution. It takes an npub or a hostname from the hosts file, or --key naming a key file (an nsec) or public key file (an npub); with neither it reads fips.key from the default key directory, falling back to the world-readable fips.pub beside it, since fips.key is mode 0600 and an unprivileged run cannot read it. The derivation is the library's own: PeerIdentity/Identity compute the address from the public key exactly as the daemon computes its own, so the packaging does not gain a second copy free to drift. The command returns before the socket path is resolved, alongside keygen, so no connection is attempted. (cherry picked from commit 6e8a0033dbc9a546a2d24cc295335ccef0cd0ead) --- CHANGELOG.md | 9 ++ docs/reference/cli-fipsctl.md | 28 +++++- src/bin/fipsctl.rs | 162 +++++++++++++++++++++++++++++++++- 3 files changed, 194 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e7a76088..c4fe9e01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -234,6 +234,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 #### Packaging & deployment +- `fipsctl address [npub|hostname]` prints a node's `fd00::/8` mesh address and + nothing else, without contacting the daemon. With no argument it derives the + local node's address from `fips.key` in the default key directory, falling + back to the world-readable `fips.pub` beside it; `--key PATH` names a key or + public key file elsewhere. This lets an installer or image build write a mesh + address into a config file at a point where no node is running and none can + be, and keeps the derivation in one place rather than reimplemented by + whatever needs it. + - `packaging/debian/build-deb.sh --features ` builds the `.deb` with a Cargo feature list, which is how an instrumented package is produced for a measurement run. The auto-derived dev Version gains a matching `+` diff --git a/docs/reference/cli-fipsctl.md b/docs/reference/cli-fipsctl.md index ffa26936..e90c0b9f 100644 --- a/docs/reference/cli-fipsctl.md +++ b/docs/reference/cli-fipsctl.md @@ -16,8 +16,8 @@ one JSON request, and pretty-prints the response. Exits with a non-zero status if the socket cannot be reached, the daemon returns an error, or the request times out. -`fipsctl keygen` is a special case: it does not contact the daemon and -operates purely on local files. +`fipsctl keygen` and `fipsctl address` are special cases: they do not +contact the daemon and operate purely on local files. For the line-delimited JSON wire protocol, see [control-socket.md](control-socket.md). For the YAML configuration @@ -98,6 +98,30 @@ daemon. on Unix. After running `keygen`, set `node.identity.persistent: true` in `fips.yaml` or the daemon will overwrite the keys on next start. +### `address [identity] [options]` + +Print a node's mesh address (`fd00::/8`) and nothing else, so it can be +captured in a shell substitution. Does not contact the daemon, which +makes it usable from an installer or image build where no node is +running. + +| Argument | Description | +| -------- | ----------- | +| `identity` | npub (bech32) or hostname from `/etc/fips/hosts`. Omit to use this node's own identity. | + +| Flag | Argument | Default | Description | +| ---- | -------- | ------- | ----------- | +| `-k`, `--key` | `PATH` | *(none)* | Derive from this key file (an `nsec`) or public key file (an `npub`). Conflicts with `identity`. | + +With neither an `identity` nor `--key`, the address comes from +`fips.key` in the default key directory (the same directory `keygen` +writes to), falling back to `fips.pub` beside it, since `fips.key` is +mode `0600` and an unprivileged run cannot read it. + +The address is derived from the public key exactly as the daemon +derives its own: `fd` followed by the first 15 bytes of +SHA-256(pubkey). + ### `connect
` Tell the daemon to dial a peer over a specific transport. diff --git a/src/bin/fipsctl.rs b/src/bin/fipsctl.rs index 8fb7fe3e..3159c910 100644 --- a/src/bin/fipsctl.rs +++ b/src/bin/fipsctl.rs @@ -7,10 +7,10 @@ //! On Windows, uses a TCP connection to localhost. use clap::{Parser, Subcommand}; -use fips::config::{write_key_file, write_pub_file}; +use fips::config::{read_key_file, write_key_file, write_pub_file}; use fips::upper::hosts::HostMap; use fips::version; -use fips::{Identity, encode_nsec}; +use fips::{ConfigError, Identity, PeerIdentity, encode_nsec}; use std::io::{BufRead, BufReader, IsTerminal, Write}; use std::net::{Ipv6Addr, SocketAddrV6}; use std::path::{Path, PathBuf}; @@ -58,6 +58,15 @@ enum Commands { #[arg(short = 's', long = "stdout")] stdout: bool, }, + /// Print a node's mesh address, without contacting the daemon + Address { + /// npub (bech32) or hostname from /etc/fips/hosts. Defaults to this + /// node's own identity, read from its key files. + identity: Option, + /// Derive from this key file (an nsec) or public key file (an npub) + #[arg(short = 'k', long = "key", conflicts_with = "identity")] + key: Option, + }, /// Connect to a peer Connect { /// Peer identifier: npub (bech32) or hostname from /etc/fips/hosts @@ -423,6 +432,63 @@ fn resolve_peer(peer: &str) -> String { } } +/// Derive the mesh address for whichever identity the arguments name. +/// +/// Precedence is the order the arguments are documented in: an explicit npub +/// or hostname, then an explicit key file, then this node's own key files in +/// the default key directory. Nothing here touches the control socket, so the +/// address is available to a maintainer script with no daemon running. +fn mesh_address(identity: Option<&str>, key: Option<&Path>) -> Result { + match (identity, key) { + (Some(peer), _) => address_from_npub(&resolve_peer(peer)), + (None, Some(path)) => address_from_file(path), + (None, None) => address_from_key_dir(&default_key_dir()), + } +} + +/// Derive a mesh address from a bech32 npub. +fn address_from_npub(npub: &str) -> Result { + let peer = PeerIdentity::from_npub(npub).map_err(|e| format!("invalid npub: {e}"))?; + Ok(peer.address().to_ipv6()) +} + +/// Derive a mesh address from a key file holding an nsec, or from a public +/// key file holding an npub. +/// +/// The file contents are the private key in the first case, so they are held +/// in a guard and cleared on every exit path. +fn address_from_file(path: &Path) -> Result { + let contents = Zeroizing::new(read_key_file(path).map_err(|e| match e { + // ReadFile's own text names the file a config file, which this is not. + ConfigError::ReadFile { source, .. } => { + format!("cannot read {}: {source}", path.display()) + } + other => other.to_string(), + })?); + + if contents.starts_with("npub1") { + return address_from_npub(contents.as_str()); + } + + let identity = Identity::from_secret_str(contents.as_str()) + .map_err(|e| format!("{} does not hold a usable key: {e}", path.display()))?; + Ok(identity.address().to_ipv6()) +} + +/// Derive this node's mesh address from the key files in `dir`. +/// +/// Tries `fips.key` first and falls back to `fips.pub`: the private key is +/// mode 0600, so an unprivileged run can still answer from the world-readable +/// public key beside it. When neither is readable both attempts are reported, +/// since either file would have answered. +fn address_from_key_dir(dir: &Path) -> Result { + match address_from_file(&dir.join("fips.key")) { + Ok(addr) => Ok(addr), + Err(key_err) => address_from_file(&dir.join("fips.pub")) + .map_err(|pub_err| format!("{key_err}\n{pub_err}")), + } +} + fn main() { let cli = Cli::parse(); @@ -492,6 +558,17 @@ fn main() { return; } + if let Commands::Address { identity, key } = &cli.command { + match mesh_address(identity.as_deref(), key.as_deref()) { + Ok(address) => println!("{address}"), + Err(e) => { + eprintln!("error: {e}"); + std::process::exit(1); + } + } + return; + } + let socket_path = cli.socket.unwrap_or_else(default_socket_path); let request = match &cli.command { @@ -566,7 +643,7 @@ fn main() { ProfileTickAction::Status => build_query("profile_tick_status"), }, }, - Commands::Keygen { .. } => unreachable!(), + Commands::Keygen { .. } | Commands::Address { .. } => unreachable!(), }; // For plot output we need to post-process the JSON response rather @@ -1513,6 +1590,85 @@ mod tests { assert_eq!(default_key_dir(), PathBuf::from("/etc/fips")); } + /// Build a key file for `identity` in `dir` and return its path. + fn write_identity_key(dir: &Path, identity: &Identity) -> PathBuf { + let mut keypair = identity.keypair(); + let mut secret_key = keypair.secret_key(); + let nsec = Zeroizing::new(encode_nsec(&secret_key)); + secret_key.non_secure_erase(); + keypair.non_secure_erase(); + let path = dir.join("fips.key"); + write_key_file(&path, &nsec).unwrap(); + path + } + + #[test] + fn the_address_derived_from_an_npub_is_the_one_its_owner_uses() { + let identity = Identity::generate(); + let derived = address_from_npub(&identity.npub()).unwrap(); + assert_eq!(derived, identity.address().to_ipv6()); + assert_eq!(derived.octets()[0], 0xfd); + } + + #[test] + fn a_malformed_npub_is_refused_rather_than_hashed() { + assert!(address_from_npub("npub1notarealkey").is_err()); + assert!(address_from_npub("").is_err()); + } + + #[test] + fn the_address_derived_from_a_key_file_matches_its_npub() { + let dir = tempfile::tempdir().unwrap(); + let identity = Identity::generate(); + let key_path = write_identity_key(dir.path(), &identity); + + let expected = identity.address().to_ipv6(); + assert_eq!(address_from_file(&key_path).unwrap(), expected); + assert_eq!(address_from_key_dir(dir.path()).unwrap(), expected); + assert_eq!( + mesh_address(None, Some(key_path.as_path())).unwrap(), + expected + ); + } + + #[test] + fn the_public_key_file_answers_when_the_private_one_is_absent() { + let dir = tempfile::tempdir().unwrap(); + let identity = Identity::generate(); + write_pub_file(&dir.path().join("fips.pub"), &identity.npub()).unwrap(); + + assert_eq!( + address_from_key_dir(dir.path()).unwrap(), + identity.address().to_ipv6() + ); + } + + #[test] + fn a_missing_key_file_reports_every_path_that_was_tried() { + let dir = tempfile::tempdir().unwrap(); + let err = address_from_key_dir(dir.path()).unwrap_err(); + assert!(err.contains("fips.key"), "{err}"); + assert!(err.contains("fips.pub"), "{err}"); + assert!(address_from_file(&dir.path().join("fips.key")).is_err()); + } + + #[test] + fn an_empty_or_unparsable_key_file_is_refused() { + let dir = tempfile::tempdir().unwrap(); + + let empty = dir.path().join("empty.key"); + std::fs::write(&empty, "\n").unwrap(); + assert!(address_from_file(&empty).unwrap_err().contains("empty")); + + let junk = dir.path().join("junk.key"); + std::fs::write(&junk, "not-a-key\n").unwrap(); + assert!( + address_from_file(&junk) + .unwrap_err() + .contains("does not hold a usable key") + ); + } + #[test] fn test_acl_show_command_name() { assert_eq!(AclCommands::Show.command_name(), "show_acl"); From dce73c2634b245115f396abb757d20b3ea2ca919 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 25 Aug 2026 20:54:58 +0100 Subject: [PATCH 14/15] Record the pre-v0.5.0 fixes in the changelog Four of the thirteen changes in this batch are user-visible and had no entry: the path-MTU seeding record that never had a removal site, the reverse-lookup keys a link removal left behind, the two handshake reject arms that stranded a session index and a retry schedule, and the UDP reuse flags set after the bind they were meant to affect. The rest are harness, CI and documentation changes that a release note should not carry. --- CHANGELOG.md | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c4fe9e01..522f54c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -451,6 +451,36 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +#### Peer and link state + +- A peer that goes away no longer leaves its path-MTU state behind. The record + of which transport link-seeded a destination's path MTU had no removal site + on any lifecycle, so the map grew one entry per peer this node had ever + linked with and held them for the life of the process. Both the stored value + and its seeding record are now released when the path they describe goes, + together rather than separately: releasing the record alone would leave the + value with nothing recording where it came from, and a peer returning over a + wider transport would then be refused by the never-loosen rule indefinitely. + A peer whose link is still up has both written straight back by the reseed + that already follows. + +- Removing a link now clears every reverse-lookup key it was inserted under. + The removal rebuilt one key from the link's own remote address, so an entry + inserted under a different address form — which the cross-connection + promotion arms do, keying on the packet's source address — outlived the link + it named. An entry a newer link has since claimed is still left alone. + +- A rejected handshake no longer strands a session index or a retry schedule. + Two reject arms freed neither, which the `next`-line versions already do; + master now carries the same shape, so an ACL-rejected outbound dial is + rescheduled and an abandoned rekey index is returned rather than held. + +#### Transport + +- The UDP listen socket's address-reuse flags are now set before its bind + rather than after, where they had no effect on the socket they were meant to + configure. + #### Control socket - `disconnect` on the control socket now closes the transport connection From 28be191d007b834c828d9a9a2e19a708fbb27aa8 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Tue, 25 Aug 2026 20:56:24 +0100 Subject: [PATCH 15/15] Record the NixOS module and overlay in the changelog Arjen's flake module landed on master as part of a6567f9 and never got a changelog entry here; the only copy of the text was in the release branch's own [0.5.0] section, written when the release content was prepared. That made the two sections disagree by a bullet in each direction and left a shipped feature undescribed on the branch it shipped from. The entry is verbatim from the release branch, including its statement that nothing here exercises the flake. --- CHANGELOG.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 522f54c7..4be426b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -234,6 +234,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 #### Packaging & deployment +- A NixOS module and an overlay are exposed from the flake, so a flake consumer + enables the daemon with one line rather than hand-rolling a systemd unit. + `overlays.default` adds `pkgs.fips`, and `nixosModules.default` provides + `services.fips.*`: `enable`, `package`, `configFile`, `openFirewall` (UDP 2121 + and TCP 8443) and `dns.enable`, which routes `.fips` to `[::1]:5354` through + systemd-resolved declaratively rather than with setup and teardown scripts. + `packaging/nixos/README.md` documents it with a full consumer `flake.nix`. + Contributed by Arjen. **Unexercised here**: no CI job builds the flake and no + Nix toolchain is present on the machine this release was assembled on, so the + module is untested outside its author's environment. + - `fipsctl address [npub|hostname]` prints a node's `fd00::/8` mesh address and nothing else, without contacting the daemon. With no argument it derives the local node's address from `fips.key` in the default key directory, falling